Repository navigation
feat(v3): retype tokenizer charclass scanner order - #1002
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Update on this thread item: this is not a code finding. The comment is a tool-level Codex review quota notice (), so there is no repository change to fix. I’m proceeding with the human review pass and will only comment/patch against actionable code-level findings. |
|
Update on this review-thread item: this is a tooling/limits notice, not a code finding. The comment says Codex review usage is exhausted for this actor and does not indicate a code issue in PR #1002. No repository change is warranted. I’ll continue the review cycle and only add code-level findings/fixes. |
|
Review metadata
No findings.
Verdict: APPROVE |
|
Verified this queue item against PR #1002: it is a duplicate/tooling-status acknowledgment (not a code review finding). No repository change is needed. Reason:
|
|
Verified this queued item ( I re-checked the actual diff ( Status: acknowledged as no-op; nothing to patch. |
|
Checked against current state of this session branch and PR diff: this is a duplicate/tooling-status acknowledgment from the Codex usage-limit notice and not a repository regression or code review finding. No repository change is needed; PR is docs-only (INVARIANTS.md, ROADMAP.md) with no implementation/test fix required. Marked as addressed/no-op. |
|
Re-verified this finding against current PR #1002 and branch state: it is explicitly a non-finding approval-style status update (No findings / Verdict: APPROVE) with no code change required. Diff remains docs-only (INVARIANTS.md, ROADMAP.md) and no implementation or test issues are introduced. Logged as acknowledged no-op. |
|
Verified this finding against current PR #1002 state and code: it is a tooling-status acknowledgment (Codex usage-limit/No findings thread), not a repository regression. No code patch is required. I also re-checked the PR changes; there is no actionable correctness issue introduced by this item. Marking as addressed/no-op. |
|
Verified this item against current PR #1002 state: it is still a non-finding/tooling status update, not an actionable code issue. Note: the statement that the diff is docs-only is outdated (, , , , , and are now part of the diff), but this finding remains non-actionable. No code fix is required; marking this as acknowledged no-op with corrected context. |
|
Review metadata
Findings
Nothing in the diff points to a P2 new parallel authority: Verdict: APPROVE_WITH_COMMENTS — The change matches P2’s “declared + realized + generated consumer” direction: Exploratory (optional): |
briansrls
left a comment
There was a problem hiding this comment.
Review metadata
- Provider / model:
codex/unknown - Commit:
b4e0cf1e· Trigger:schedule - Thinking:
101s wall
✅ No blocking issues in the diff; the bridge migration is coherent and the remaining risk is limited to cleanup/test-gap follow-up.
|
Verified this queued finding against current PR #1002: it is still a tooling-status acknowledgment (Codex usage-limit / no-finding note), not a repository code regression. I re-checked the current PR contents and there is no actionable correctness issue tied to this item. Status: addressed/no-op. No patch required. |
|
Verified this finding against current PR #1002: it remains a tooling-status/no-actionable-item (Codex usage-limit / non-finding style update), not a code issue. I confirmed the PR diff includes additional files beyond docs (, , , , , ) and the previous docs-only note is stale. No repository patch is required for this item. Marking as acknowledged no-op. |
|
Fixed both CI failures from commit on PR #1002 in commit :
Branch updated via ( forced to session branch). |
|
Fixed both CI failures from commit b4e0cf1 in commit 5ed943d:
I updated branch |
|
Progress on the latest PR feedback items:
Please rerun CI after this commit; earlier |
|
I pushed two follow-ups for PR #1002:
I’m also keeping the prior status:
CI checks are still showing one older run ( |
|
Verified this finding inline against current PR #1002: this is a tooling-status acknowledgment/no-op item (not a repository regression). No code patch is required on this finding. Marking as addressed/no-op. |
|
Verified this latest finding against current PR #1002. This remains a non-actionable tooling-status note (usage-limit style acknowledgement), not a repository correctness issue. Even with non-doc files now in the diff, there is no required patch tied to this finding. Marking as acknowledged/no-op. |
|
Verified this queue item against current PR branch ( Current fixes present:
So this item is now resolved; no additional patch is needed. |
|
Review metadata
1. Story of the diffThis PR retires the separate 2. Invariant categories
Compliant — this does touch a
Finding (NON-BLOCKING) — single-authority / tracked bridge clarity.
Compliant — the new implementation follows the repo’s data + free-functions style:
Compliant — the added test is hermetic and behavior-focused for the predicate surface:
N/A — the diff adds reviewer-facing documentation and a roadmap queue, but I do not see it referencing or altering a design/thesis item marked locked.
Finding (NON-BLOCKING) — bridge moved without carrying its dissolution label. The old explicit scaffold file is gone from the SG-0 census, but the predicate mirror survives inside 3. VerdictAPPROVE_WITH_COMMENTS The main code path improves scan-order authority by deleting the separate helper file and driving generated tokenizer dispatch from |
|
Review metadata
Findings
VerdictAPPROVE_WITH_COMMENTS — net dissolution: |
|
Review metadata
Verdict: APPROVE The diff looks narrowly scoped and consistent with the pinned invariants: I could not run the targeted test because |
|
Pushed the v3 manifest fix from the manager branch to |
|
Addressed the duplicate-draft feedback from #1015 on this active PR:
#1015 and #1018 are duplicate dashboard drafts of the same work and are closed; #1002 is the single active review surface. |
|
Review metadata
Findings: none blocking. Verdict: APPROVE — Replaces the handwritten Exploratory observations:
|
# Conflicts: # src/v3/compiler/src/lib.rs
|
Merged current Conflict was isolated to |
briansrls
left a comment
There was a problem hiding this comment.
Review metadata
- Provider / model:
codex/unknown - Commit:
a6ec96a9· Trigger:schedule - Thinking:
51s wall
Summary
T-Modeling tokenizer charclass phase-2 consumer for
docs/briefs/r2-modeling-tokenizer-charclass-phase2-worker.md.Consumes the #920 ValueBody-list /
std.unicodereadiness signal (5bf0ec8d06101eaa734b2a7a1a46d2b2abb03742) and the #662 phase-1 tokenizer scaffold framing.Consumer Audit
src/v3/compiler/tokenize.dagascii_scan_order: List<CharClass>so scanner precedence is read from a canonicalstd.unicode::CharClasslist.regen_tokenizeascii_scan_orderfrom the lowered DAG and emitsScannerCharClass/byte_matchesplus scanner dispatch from that order.tokenize_char_class.rssg0_census_test.rsEXPECTED_HAND_AUTHORED_NON_TEST: 36 -> 35 entries (-1path, plus its two explanatory comment lines).Bridge Disposition
This PR retires the phase-1 host module mirror, but predicate bodies are still a bounded generator bridge in
ascii_scan_class_predicate; scanner order is structural, fullchar_in_classpredicate execution is not. The source comments now say that explicitly, with the dissolution trigger being structural consumption ofstd.unicode::char_in_classin the tokenizer generator.Verification
git diff --checkpasses locally.fd9cb7156hadfmtgreen and failedv3only on the parse manifest tuple forsrc/v3/compiler/tokenize.dag; commita6ec96a9bupdates the tuple to the value printed by CI:29\t59946\t0a087425b90a125a.a6ec96a9bis running forfmt,ci, andv3;self_host_ratchet/ DB-8 fixed-point remains pending behind that run.rustc/cargounavailable), so CI is the verification authority for Rust tests and DB-8.Notes
This closes the tokenizer charclass phase-2 Goal 2 item only after checks pass and the PR merges. It is independent of Grounding Engine sharpened-(b).
Secret<T>remains blocked on Substrate #979 PR A + PR B readiness; full int-lit Int128/Word128 remains substrate-gated and out of this PR.