Skip to content

Add manual tokenizer fingerprint comparison beta - #9

Open
phantom5125 wants to merge 1 commit into
mainfrom
codex/tokenizer-fingerprint
Open

phantom5125 wants to merge 1 commit into
mainfrom
codex/tokenizer-fingerprint

Conversation

@phantom5125

Copy link
Copy Markdown
Owner

Summary

  • Add a manually triggered Tokenizer comparison beta in Control Center for saved Kimi, MiniMax, GLM, OpenRouter, and DeepSeek API keys.
  • Send a fixed 30-text probe suite to the selected official inference endpoint with at most one output token per request; show progress, cancellation, sanitized errors, and per-text token-count differences.
  • Rank ten versioned, attributed reference profiles by shift-invariant exact-count agreement. Missing/zero usage, low coverage, and unvaried counts produce no score. The UI explicitly says similarity cannot verify model identity or weights.
  • Keep keys in Keychain and measurements in memory only; include the reference dataset's MIT notice and method documentation.

Verification

  • swift build
  • bash scripts/test.sh --quiet — 269 tests passed before the final provenance-only change; targeted tests passed afterward
  • swift format lint --recursive --strict Sources Tests
  • bash scripts/privacy_scan.sh
  • bash scripts/resource_check.sh — 85,164,032-byte maximum RSS, 10.67 seconds, 7,027,704-byte release executable
  • bash scripts/package_app.sh; confirmed bundled reference JSON and NOTICE and verified the app signature

Review notes

  • This is a diagnostic comparison of tokenization and request-wrapping behavior. It has no calibrated model-family verdict. The reference data was sampled through OpenRouter on 2026-08-29/30 and may differ from direct provider routing.
  • A run makes up to 30 inference requests and can consume plan quota or API credits. It is never scheduled or started by refresh. A balance-only key may be rejected by the inference endpoint.
  • The provider request adapters are fixture-tested; no live user API keys were available for end-to-end provider testing.

Method and reference data: https://arxiv.org/abs/2608.29930 and https://github.com/ictchenbo/which-llm.

@phantom5125

Copy link
Copy Markdown
Owner Author

Review: tokenizer fingerprint comparison beta

CI: 4/4 checks pass. No unresolved comments.

Human confirmation needed (credential handling)
This feature reads the account's Keychain API key and sends it to provider inference endpoints (TokenizerProbeClient.measure, TokenizerLabView.start). The design is sound — explicit user trigger, fixed official hosts via EndpointPolicy, no persistence or diagnostics export — but credential-touching changes need human sign-off before merge.

Findings

  1. CHANGELOG not updated. A new user-visible Control Center route ships while ## Unreleased stays empty, against the file's own convention. → Add an entry under ## Unreleased.
  2. Localization bypass. TokenizerLabView hardcodes ~20 inline en/zh/ja label() triples instead of the app's L10n catalog (Strings.swift), which this PR only extends for the route title. → Move the strings into L10n for consistency.
  3. Magic number 30 duplicated in TokenizerLabView (ProgressView(..., total: 30), "\(session.completed)/30", "…/30 valid counts") even though the catalog exposes prompts. → Use session.prompts.count so a future corpus revision cannot silently desync the UI.

Withholding LGTM pending human confirmation of the credential path.

— Reviewed by unknown model

@phantom5125

Copy link
Copy Markdown
Owner Author

LGTM

The privacy posture is solid: user-initiated only, keychain key read after explicit Run, OAuth credentials excluded, bounded 1-token requests, host allowlist, zero-count treated as missing evidence, honest docs about what the score does and does not prove.

One gap: the repo maintains CHANGELOG.md per release (0.3.0-rc.2 etc.), but this PR adds no entry for the new beta feature. Add an Unreleased entry before merge.

— Reviewed by unknown model

@phantom5125

Copy link
Copy Markdown
Owner Author

Technical review of the tokenizer fingerprint beta: probes are bounded (30 fixed prompts, max_tokens=1, 1 MiB response cap), endpoints are exact-host allowlisted, the feature is manual-only, keys stay in Keychain, measurements stay in memory, and the scoring gates (20 valid / 80% coverage / 5 distinct counts) plus fixture tests look correct. CI is green.

This change reads stored API keys and sends them as Bearer credentials to provider inference endpoints (TokenizerProbeClient.measure in Sources/TokenLinkProviders/Tokenizer/TokenizerFingerprint.swift). Per review policy, credential/token-handling changes need human confirmation before merge, so no LGTM from me — please confirm the credential flow (explicit Keychain key only; Codex CLI / Claude OAuth credentials excluded) as a human reviewer.

— Reviewed by unknown model

@phantom5125

Copy link
Copy Markdown
Owner Author

Withholding LGTM — this change reads saved API keys from Keychain and sends them as Bearer tokens to provider inference endpoints (credential/token handling), so it needs human confirmation before merge.

Notes:

  • Design is otherwise sound: opt-in, manual-trigger only (no launch/refresh/schedule), EndpointPolicy host allowlist, no persistence of keys/responses/measurements, and MIT attribution for the which-llm reference data.
  • Please confirm the 30-probe run's quota/cost impact is acceptable and that a balance-only key being rejected by the inference endpoint is the intended behavior.

CI: all checks pass. No unresolved comments.

— Reviewed by unknown model

@phantom5125

Copy link
Copy Markdown
Owner Author

Review of tokenizer fingerprint beta. Code quality is high and CI is green (4/4 jobs pass). Findings:

  • Design is sound: manual-only trigger, key read from Keychain only after explicit Run, EndpointPolicy host allowlist, missing/zero usage treated as missing evidence, scoring thresholds (20 valid / 80% coverage / 5 distinct counts) prevent garbage scores. NOTICE attribution and docs/tokenizer-comparison.md are thorough.
  • Minor: TokenizerFingerprint.swift:129 — the 1 MiB response cap is checked after the full body is already in memory via the HTTP client; it bounds JSON decoding, not download size. Acceptable for a manual beta, noting for awareness.
  • Minor: TokenizerLabView.swift:75 hardcodes total: 30; consistent with the bundled() validation (prompts.count == 30), so not a bug, but the constant is duplicated.

No LGTM from me on this one: the change reads Keychain API keys and sends them as Bearer tokens to five external inference endpoints, which falls under credential/token handling. Requesting human confirmation of the credential flow before merge.

— Reviewed by unknown model

@phantom5125

Copy link
Copy Markdown
Owner Author

Self-review notes (comment instead of review since GitHub blocks self-review). CI: all 4 checks pass. No unresolved human comments.

Not posting LGTM: this PR adds a new code path that reads saved API keys from Keychain and sends them to provider inference endpoints (credential/token handling), so it needs a human confirmation pass on the credential flow before merge.

What I verified while reviewing:

  • Keys are read only after explicit Run (test fingerprintSessionDoesNotReadKeyOrCallNetworkUntilManualStart); Codex/Claude OAuth credentials are excluded.
  • Transport stays on the existing exact-host, no-redirect EndpointPolicy; model ID is validated (length/control chars) before reaching the request.
  • Zero/missing usage is treated as missing evidence, never scored; non-2xx aborts the run.
  • Reference data is attributed in NOTICE (MIT, which-llm) with source commit pinned.

Two minor items, non-blocking:

  1. TokenizerProbeClient.measure (Sources/TokenLinkProviders/Tokenizer/TokenizerFingerprint.swift): a count outside (1...10_000_000) throws .invalidResponse and aborts the whole 30-probe run, while zero/missing counts are tolerated as nil. Consider treating out-of-range as nil too so one bad gateway response doesn't discard a paid run.
  2. referenceSummary renders profile count and dates but not catalog.source; attribution is in NOTICE/docs, so this is cosmetic only.

— Reviewed by unknown model

@phantom5125

Copy link
Copy Markdown
Owner Author

Verdict: not LGTM yet — human confirmation required.

This feature reads saved provider API keys from Keychain and sends authenticated requests to five provider endpoints, so per review policy it needs a human sign-off on the credential path before merge. CI is green and the design is otherwise sound: explicit user initiation only, EndpointPolicy host allowlist per target, 1 MB response cap, count sanity range, no persistence of keys/responses/counts.

Minor items:

  1. TokenizerFingerprintSession.run maps responseTooLarge, invalidResponse, and unsupportedProvider to .network via the default: branch (Sources/TokenLinkApp/Tokenizer/TokenizerFingerprintSession.swift:207-213). The UI then says "Probe failed. No response data was saved." for an oversized response, which is misleading. Map these to distinct failure cases.
  2. The probe count 30 is hardcoded in three places (catalog validation prompts.count == 30, ProgressView(total: 30), and the "x/30" label). Derive the UI total from session.prompts.count so a future catalog revision cannot show wrong progress.

— Reviewed by unknown model

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