Repository navigation
Strip bloated comments workspace-wide (11.2k → 2.4k comment lines) - #4
Merged
Merged
Conversation
Introduce [auth] config (mode = "none" | "client-keys") with argon2id
password-hash verifiers for named client keys. A require_client_auth
layer on the proxy routes verifies Bearer tokens (digest-cached,
spawn_blocking, failed attempts charge the peer's IP bucket); key ids
are stamped onto RequestReceived events as `caller` ("anonymous" under
the loopback no-auth exemption) and key the rate limiter for
authenticated callers. Non-loopback binds without client keys refuse
to start, and invalid verifiers fail closed.
Also clears the pre-existing clippy/insta baseline: cfg-gates the
macOS-only desktop-entry helpers, fixes the contradictory
shell-metacharacter escape test and commits its missing snapshot, and
adds narrowly-scoped #[expect(clippy::too_many_lines)] on the
wire-translation functions. Ignores .z-proflow/ and tracks its
STATE.md run journal.
require_client_auth's doc claimed that once a peer's IP bucket was empty further failures skipped the CPU-costly argon2 verification, but the code ran the full verification on every attempt and the limiter only charged the bucket afterwards — the claimed CPU-exhaustion bound did not exist. Implement the bound instead of weakening the comment: add RateLimiter::would_allow, a non-consuming peek at the peer's refilled balance, and consult it before authenticate() so a peer with an exhausted failure budget gets the uniform 401 without paying argon2. Successful authentications are never charged (they are limited under the key id in prepare_request); failures still drain the bucket via is_allowed so the gate trips. Doc comment now states the exact flow, including that rate_limit_rpm = 0 means no bound applies.
Implement the EventBus trait over SQLite (rusqlite, bundled): a dedicated writer task behind a bounded channel persists every request event to a requests table — token usage, cost, caller, latency, stop reason, error kind, body hash — with indexes on (timestamp), (provider, timestamp), and (caller, timestamp). A full channel drops the event and increments a counter rather than backpressuring responses; the database runs in WAL mode; the writer drains and exits via the new EventBus::close after the server's graceful drain. New [storage] config (path, max_age, max_rows, prune_interval) builds the bus in load_toml_state; absent config keeps the NoopBus default. Retention pruning runs on a timer against the age and row caps.
Fill the llm-proxy-api placeholder with the read-only query surface over the SQLite ledger: GET /usage/summary (window plus group_by provider|model|caller), GET /usage/requests (keyset-paginated raw rows, never request bodies), and GET /usage/keys (per-caller counts and last-seen). Queries run on read-only connections with bound parameters, and costs sum in rust_decimal so SQLite never coerces them to float. Admin auth is a separate [auth.admin] token stored as SHA-256 and compared in constant time; every rejection is a uniform 401 that never echoes the token. The routes mount on a second loopback listener ([server].admin_bind, default main port + 1) by default; admin_on_main_listener = true is the named, loudly-warned opt-in posture. Config validation rejects admin without [storage], a bind shared with the main listener, and empty tokens.
Honor [provider.egress] proxy in provider TOML: the transport builds a per-provider reqwest client (.proxy with socks5h/socks5/http/https, same pool and timeout settings as the shared client), and both upstream send sites in the core pipeline pick it via upstream_client_for. Providers without egress keep today's exact shared client. Config validation rejects empty, scheme-less, and non-allowlisted URLs at load time, and the transport re-validates the scheme before handing the URL to reqwest (which silently accepts scheme-less and ftp URLs). The URL's userinfo is redacted in Debug output and never logged, and an unusable proxy URL is a startup error before the listener binds — never a silent direct fallback. reqwest gains the socks feature (handshake in-tree, no new crates).
Configure cargo-dist 0.32.0 (dist-workspace.toml): shell installer, x86_64- and aarch64-unknown-linux-musl targets, CARGO_HOME install path, GitHub hosting, no updater. The release workflow is generated verbatim by dist generate --mode ci — plan, build, host, and announce jobs on version-tag pushes, with dist plan exercised on pull requests; ci.yml is untouched. Add the [profile.dist] cargo requires (dist passes --profile dist unconditionally) inheriting the release posture. Pin CC_aarch64_unknown_linux_musl to musl-gcc in .cargo/config.toml: dist builds aarch64-musl on arm64 runners with only musl-tools, whose sole compiler is musl-gcc, while the cc crate probes only aarch64-linux-musl-gcc for that target.
Nine tests in three new binaries, all driving the real surfaces (build_router, admin_router, the client-auth layer, SqliteEventBus, the /usage endpoints) with every negative assertion paired with a positive control, so a passing scan proves it read real content. They pin: client and admin tokens, PHC verifiers, and provider keys never appear in events, captured tracing, error responses, or the raw db/-wal/-shm file bytes; /usage rows carry the sha256 body digest but never body content even when the mock upstream reflects the request; the admin surface answers missing, wrong, and non-admin credentials with an identical uniform 401 on both mounts; one caller's key id is never observable by another; and anonymous attribution survives the loopback exemption without swallowing or leaking a stray bearer token. tracing-subscriber joins the server and api dev-dependencies for the log-capture assertions.
hmziqrs
approved these changes
Oct 27, 2026
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
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
Codebase-wide comment-hygiene pass applying a hard comment standard: zero comments by default — only public-API contract docs, convention-required
SAFETY:blocks, and link-carrying last-resort constraint notes survive, each at most 2 lines (3 absolute max). Banned categories are deleted outright, never rewritten shorter: restating comments, section banners, change narration, commented-out code, AI-voice commentary, and docs on private items.The number: 11,174 → 2,373 comment lines across
crates/andapps/— 8,801 removed (79%) over 87 files (+684/−10,150), measured by grep before and after.Stacked on #3 — merge order matters
This branch sits on top of
prx/usage-ledger(PR #3). Until #3 merges, the diff below includes #3's commits as well. Merge #3 first; this PR's diff then collapses to the comment-only changes.What survived, what died
//////!docs on public items — behavior, panics, units, invariants a caller must know — stripped of narration, history, rationale essays, and banners;// SAFETY:blocks (clippy denies their absence); link-carrying constraint notes proving the code cannot express the constraint.missing_docsis enforced through clippy-D warnings: no public item lost its docs entirely — they were trimmed to the contract, never removed wholesale.How it was run
Eight disjoint per-crate batches (core, protocol, provider, storage, api, server-core, server-routes, apps) edited in parallel, with script-run gates between waves, fresh read-only judges per batch per audit round (12 judge passes total, all applying the same ruling), and fix rounds until the judges went silent. One logical commit per batch, explicit paths only.
Verification
cargo fmt --all --check,cargo clippy --workspace --all-targets --all-features --locked -- -D warnings,cargo test --workspace --all-features, andcargo build --lockedall exit 0 at the head of this branch, run by the orchestration script (exit codes, not agent claims).Known residue (disclosed, not hidden)
UsageApi's doc restates the crate doc near-verbatim (crates/llm-proxy-api/src/routes.rs:26-28).