feat(#300): rskim-tokens — multi-provider tokenization library API#329
Open
dean0x wants to merge 11 commits into
Open
feat(#300): rskim-tokens — multi-provider tokenization library API#329dean0x wants to merge 11 commits into
dean0x wants to merge 11 commits into
Conversation
…d proptest workspace dep Wave-1 Step-0 coordinated workspace edit — committed once on the integration branch before per-ticket work begins (#301/#302 share this foundation). Changes: - serde_json: enable preserve_order + raw_value features workspace-wide (Decision 1). preserve_order: byte-stable key ordering for #302 round-trip serialization. raw_value: number source-token comparison for #301 byte-faithful equality. NOT arbitrary_precision — would change Value number semantics at 711 call sites. - proptest = "1.4": add as shared [workspace.dependencies] dev-dep (Decision 8). - crossbeam-channel = "0.5": confirmed retained (Decision 9, was line 66). Gate: cargo test --all-features — 5360 pass, 0 fail (exit 0). AC19: cargo tree -e features confirms arbitrary_precision is NOT activated. Co-Authored-By: Claude <noreply@anthropic.com>
Replace Wave-1 Step-0 / Decision-N labels and the Gate annotation with concise rationale comments that match the surrounding dep-comment style. The arbitrary_precision exclusion note is preserved as it is load-bearing context for any future feature enablement.
Ships the new workspace crate `rskim-tokens` (publish = false, matching rskim-search precedent; #324 filed for crates.io follow-up) exposing: - `Counter::new(Encoding)` → `Result<Counter, TokenError>` (fallible construction, infallible counting via `fn count(&self, &str) -> usize`) - Four encodings: Cl100k, O200k, AnthropicOffline (ceil(cl100k×1.25), basis documented in rustdoc), Heuristic (byte_len — provably safe ceiling) - `encoding_for_model(&str) -> Encoding` single-source model→encoding table with two-tier fallback (exact set + family-prefix, never errors on unknown) - `as_closure()` adapter satisfying `Fn(&str)->usize` for truncate_to_token_budget - `net-anthropic` opt-in feature (ureq+rustls, bounded timeout+retries) for AnthropicNetworkCounter — absent from default build (AC9 tree assertion added to ci.yml with `set -o pipefail` per PF-007) - `Counter` is `Send + Sync` (static assertion + 200-thread stress test) Tests: 25 integration (all ACs), 7 doc-tests, 2 unit, 4 net-feature tests. Criterion benchmark for 100KB counting with tracked baseline.
… golden; CI gates tokens.rs internals now delegate to rskim_tokens::Counter (Cl100k), preserving encode_with_special_tokens semantics and the frozen signature `pub(crate) fn count_tokens(text: &str) -> Result<usize>` — zero call-site changes (AC15). TokenStats/format_number remain binary-private (OQ7). The OnceLock<Counter> singleton replaces the old OnceLock<CoreBPE> with identical performance (one init). AC13: captured exact `--show-stats` golden BEFORE migration (`[skim] 65 tokens → 45 tokens (30.8% reduction)`) and checked it in as cli_tokens_golden.rs (assert_cmd string equality), which passes before and after. CI: add dependency-tree isolation check with `set -o pipefail` (PF-007) proving no HTTP/TLS crate appears in default builds of rskim-tokens or skim. Upgrade clippy to `--all-targets` (PF-009) to catch warnings in #[cfg(test)] modules.
- Extract prefilled_lock() and count_bpe() helpers to eliminate three-line OnceLock init repetition and duplicate Cl100k/O200k count() arms in counter.rs - Remove inner module wrapper from net.rs — #[cfg] on struct/impl directly avoids one indentation level and the pub use re-export - Collapse build_counter_with_fallback Heuristic branch from nested match to single unwrap_or_else in tokens.rs - Remove duplicate ac9_missing_api_key_returns_err test that was byte-identical to ac9_empty_api_key_returns_err
- counter.rs: remove unnecessary `unsafe impl Send/Sync` (Counter is auto Send+Sync via OnceLock<CoreBPE>); the hand-written unsafe impls suppressed the compiler's auto-trait check and could mask a future data race. AC11 static assertion still holds.
- counter.rs: extract `build_bpe` error-mapping seam (de-duplicates 3x map_err) and add tests that genuinely EXERCISE the TokenError::TiktokenInit Err arm (AC10 required the Err path be exercised, not merely asserted to exist).
- net.rs: apply rustfmt (pre-existing fmt violation that would fail CI `cargo fmt --check`).
- Cargo.toml: serde_json -> { workspace = true } for feature consistency with the workspace's preserve_order+raw_value set.
- integration.rs: add real proptest for AC6 (arbitrary input) and a non-vacuous AC7 sole-mapping repository gate with a detector self-test (verified it fails on an injected qualified-return parallel mapping).
…4 broken rustdoc intra-doc links that caused cargo doc under RUSTDOCFLAGS=-D warnings to exit 101. Removed links to cfg-gated items and non-existent methods: net::AnthropicNetworkCounter (default-feature build omits the net module), Encoding::counter (no such method), rskim_core::truncate_to_token_budget (cross-crate, out of scope), and counter::Counter::from_raw_bpe (cfg(test)-only, invisible in doc builds). AC10: Add Counter::heuristic() as an infallible public constructor for the Heuristic encoding. Replace the unreachable!() call in tokens.rs build_counter_with_fallback() with Counter::heuristic(), eliminating the panic macro from the provably-dead fallback arm without any behaviour change. AC4: Correct the o200k golden provenance comments. The vectors were produced by tiktoken-rs 0.7.0 (Rust), not an upstream Python tiktoken reference as the comments claimed. Updated to accurately state tiktoken-rs 0.7.0 and explain bytewise equivalence to Python tiktoken (same BPE vocabulary files). Fixed wrong regeneration command: cargo run --example gen_o200k_golden is a non-existent example; correct command is cargo run -p rskim-tokens --bin check_golden. Add ac10_heuristic_constructor_is_infallible_and_counts integration test to exercise Counter::heuristic() and verify the AC6 safe-ceiling invariant holds. All 29 integration + 4 unit + 8 doctests pass. cargo doc -D warnings exits 0. clippy -D warnings and fmt --check are both clean. Co-Authored-By: Claude <noreply@anthropic.com>
- Remove TokenStats::tokens_saved() (dead — #[allow(dead_code)], only called from its own test) and drop the now-pointless test assertion. - Drop the vague inline comment "// Should be around 3-4 tokens" from test_count_tokens; the assert!(count < 10) bound is self-documenting. - Remove 8 redundant #[cfg(feature = "net-anthropic")] gates from net.rs. The module is already gated at the mod declaration in lib.rs, so inner per-item cfg attributes are no-ops that add noise without protection.
- error.rs: remove dead TokenError::NetworkUnavailable variant — defined but constructed nowhere in the workspace (quality: delete dead code). - net.rs: fix two broken rustdoc intra-doc links ([`MAX_RETRIES`], [`REQUEST_TIMEOUT`]) on the public count() method that pointed at private module constants, causing cargo doc --all-features under RUSTDOCFLAGS=-D warnings to exit 101 (AC16). Inline the concrete values as code spans. Verified: clippy --all-features --all-targets clean, cargo doc -D warnings clean (default + all-features), fmt clean, rskim-tokens 41/45 tests pass, full rskim binary suite 2831 pass / 0 fail (AC13 behavior-freeze intact), AC9 dependency-tree isolation holds. Co-Authored-By: Claude <noreply@anthropic.com>
…ng under parallelism The 5 daemon-passthrough tests each spawn the skim binary with a 10s wall-clock timeout. Once rskim-tokens enlarged the parallel workspace suite, those subprocesses were CPU-starved past their timeout, wedging cargo test (the cause of the wave-build stall). #[serial] runs them sequentially. serial_test dev-dep was already wired in a prior commit.
Security / testing (medium): - Add `net_security_key_absent_from_errors` integration test: injects a sentinel key, points counter at loopback:1 (connection-refused), and asserts the key string is absent from TokenError Display and Debug. Grounds the key-never-logged invariant previously enforced only by rustdoc (applies ADR-001, testable per PF-005). - Gate `TokenError::MissingApiKey` behind `#[cfg(feature = "net-anthropic")]` to match its sibling `NetworkRequest` / `ApiResponse` variants (applies ADR-001). - Add `new_for_test` endpoint-override constructor to `AnthropicNetworkCounter` (publish=false crate; `#[doc(hidden)]`) so the security test drives error paths without real network calls. - Add CI step `AC12 latency gate (release build)` running `cargo test --release` to exercise the 25 ms ceiling; previously the release branch of the #[cfg] was dead in CI (avoids PF-005 / PF-007). - Commit criterion ci-baseline under `.bench-baselines/` (CRITERION_HOME redirect) and add CI step `AC12 criterion benchmark (relative regression visibility)` for ADR-003 relative regression guard. Add `.bench-baselines/.gitignore` to suppress report/new/change artifacts. Reliability (medium/low): - Short-circuit retry loop on permanent 4xx client errors (401, 400, 403, 413): only transport errors, 5xx, and 429 are retried, avoiding burning the retry budget on definitively non-transient failures. - Add exponential backoff (250 ms base, capped) between retry attempts. - Bound response body read with `.take(MAX_BODY_BYTES)` (64 KiB) to prevent unbounded allocation from a buggy/hostile server. - Saturating `u64 -> usize` cast for API token count (`usize::try_from(…) .unwrap_or(usize::MAX)`) to avoid silent narrowing on 32-bit targets (avoids PF-004). - Truncate embedded response body in `ApiResponse` error to 512 chars (bounded error messages; parse-at-boundary discipline). Consistency (medium/low): - Add `pub type Result<T> = std::result::Result<T, TokenError>` alias to lib.rs, matching the rskim-core / rskim-search workspace convention. - Drop `o2` family-prefix arm from `encoding.rs` — OpenAI ships no o2 model line; spec (OQ5) and module rustdoc only enumerate o1/o3/o4/chatgpt. - Pin `ureq` to `"2.8"` (was bare `"2"`) to match workspace major.minor convention. Co-Authored-By: Claude <noreply@anthropic.com>
dean0x
added a commit
that referenced
this pull request
Jun 15, 2026
rskim-llm. P1 (AC16 toolchain hygiene) — clippy/fmt regressions introduced by the prior simplify refactor (7cb7ffc): - classify.rs: replace .expect() (denied by expect_used lint) in try_classify_fenced with find('\n')? — single scan, no panic path - parse.rs: replace DoubleEndedIterator::last() char-boundary walk in describe_value with is_char_boundary floor loop (clippy::double_ended_iterator_last) - provider.rs: collapse nested if-let into a let-chain (clippy::collapsible_if) - lib.rs: rustfmt double-space-before-comment (cargo fmt --check) P2 (ADR-004 / AC18 follow-up integrity): - Source referenced #329 as the OpenAI-mutation follow-up, but #329 is an unrelated rskim-tokens issue. Filed the correct tracker (#332) and repointed all 5 references (lib.rs + mutate.rs). Verification (crate-scoped per harness): cargo fmt --check clean, cargo clippy --all-targets --all-features -D warnings clean (PF-009), cargo test --all-features 81 passed / 0 failed (incl. 6 doc-tests).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Implements ticket #300: ships
rskim-tokens, a new additive workspace crate (publish = false, matchingrskim-searchprecedent) exposing a deterministic, panic-free multi-provider token counting API for LLM context-window management. Also migratescrates/rskim/src/tokens.rsto delegate to the new library with zero behavior change (AC13 golden, AC15 frozen signature).Note:
rskim-tokenscrates.io publish follow-up is tracked in #330 (merge gate per ADR-004).Changes
New crate
crates/rskim-tokens/Encodingenum +encoding_for_model(&str) -> Encoding— single source of truth for model→encoding mapping (AC7); two-tier fallback (exact set + family-prefix) never errors on unknown IDs (AC8)Counter::new(Encoding) -> Result<Counter, TokenError>— fallible construction, infallible counting (fn count(&self, &str) -> usize),as_closure()adapter fortruncate_to_token_budget(AC2)Cl100k(tiktoken cl100k_base),O200k(tiktoken o200k_base),AnthropicOffline(ceil(cl100k × 1.25), zero-network, basis documented in rustdoc, AC5),Heuristic(byte_len— provably safe ceiling for any BPE over UTF-8, AC6)net-anthropicnon-default feature:AnthropicNetworkCountervia ureq+rustls, bounded timeout+retries, readsANTHROPIC_API_KEY, never logs it (AC9)Migration
crates/rskim/src/tokens.rscount_tokensinternals torskim_tokens::Counter(Cl100k)pub(crate) fn count_tokens(text: &str) -> Result<usize>is frozen — zero call-site changes (AC15)OnceLock<Counter>singleton replacesOnceLock<CoreBPE>with identical semantics and performance--show-statsoutput captured before migration and asserted byte-identical afterCI (
ci.yml)set -o pipefail(PF-007): proves no HTTP/TLS crate in default builds ofrskim-tokensandskimbinary (AC9)--all-targets(PF-009: catches warnings in#[cfg(test)]modules)Breaking Changes
None. The
tokens.rspublic signature is frozen, all call sites are unchanged, and no existing behavior is altered.Reviewer Focus Areas
src/encoding.rs: model→encoding table is the single source of truth for AC7 — check completeness of the exact-match set and family-prefix fallback logicsrc/counter.rs:OnceLockpre-fill pattern,Send + Syncsafety proof, fallback path incount()for the (practically dead)Nonecasesrc/anthropic_offline.rs:ceil(cl100k × 1.25)formula, widened-integer arithmetic to avoid PF-004 overflowtests/integration.rsAC5/AC6 direction tests: verify heuristic ≥ max(cl100k, o200k) and anthropic ≥ cl100k for all corpus entries including adversarial inputscrates/rskim/src/tokens.rsmigration: confirm no behavior change from the AC13 goldenci.ymldep-tree check: confirmset -o pipefailis correct and the grep pattern covers all HTTP/TLS cratesCloses #300.