Skip to content

refactor(llm-access-core): split store.rs into submodules#13

Merged
acking-you merged 1 commit into
masterfrom
refactor/split-core-store
May 31, 2026
Merged

refactor(llm-access-core): split store.rs into submodules#13
acking-you merged 1 commit into
masterfrom
refactor/split-core-store

Conversation

@acking-you
Copy link
Copy Markdown
Owner

What

Split the 4519-line llm-access-core/src/store.rs into a store/mod.rs facade
plus 12 concern-focused submodules. Pure structural move — no logic changes.
Next step in the incremental llm-access* refactor (follows #12 converter, #10
stream, #7 cache_sim, #4 request).

Why it's a thin facade (unlike #12)

store.rs is a data-model + store-interface file (97 structs / 16 *Store
traits / 24 impls — mostly Empty*/Noop* no-ops / 21 fns), and every
struct field is already pub
, so the types move out into submodules with
zero field-visibility changes. mod.rs keeps only the 55 shared default consts
(the canonical store::X constant paths) + re-exports.

Submodule Holds
config AdminRuntimeConfig + partial-update view + billing/cache policy defaults + billable-token computation
keys / groups / proxy admin key / account-group / proxy model + page/query/summary helpers
codex_account / kiro_account per-provider admin account model + query/sort/summary + import-job types
routes AuthenticatedKey, provider route/auth-update types, codex auth-token expiry helpers
public public access keys + community/contribution/sponsor + admin review-queue request types
usage usage-event + analytics + kiro-latency query/view/snapshot types
codex_status codex rate-limit / credits status types
traits the 16 *Store / UsageEventSink trait contracts
empty the 15 Empty*Store / NoopUsageEventSink no-op impls

Module style (per the agreed standard)

  • mod.rs convention; no use super::* in submodules — explicit origin
    imports. Cross-submodule type refs via use super::<mod>::X; shared consts
    via use super::{...}; external crates + crate::{provider::RouteStrategy, usage::UsageEvent} imported per module as used.
  • No pub use X::* globs: mod.rs re-exports the 127 originally-pub
    named items by name, preserving the store::X surface (116 distinct symbols
    consumed across llm-access, llm-access-store, llm-access-codex,
    llm-access-kiro, llm-access-migrations).
  • Minimal visibility: 5 previously-private helper fns referenced cross-module
    (summarize_admin_keys, apply_admin_key_query, summarize_admin_accounts,
    apply_admin_codex_account_query, default_proxy_binding) are bumped to
    pub; the rest stay private.
  • Tests inlined: the 3 billing/config tests move into config, the
    codex-account query test + its helper into codex_account (their super::X
    refs resolve to the owning submodule with no rewrite).

Verification

  • Item inventory diff vs original: 0 dropped, 0 added (the 4 "added" hits
    were //! doc-comment prose — trait contracts/interfaces/plus/with).
  • pub trait count 16 = 16; #[test] count 4 = 4.
  • cargo clippy -p llm-access-core --all-targets -- -D warnings: clean.
  • cargo test -p llm-access-core: 10 passed.
  • Reverse-dep build (llm-access, llm-access-store, llm-access-codex,
    llm-access-kiro, llm-access-migrations): compiles against the unchanged
    surface.
  • rustfmt on the 13 new files only; deps/lance & deps/lancedb stayed clean.

🤖 Generated with Claude Code

Break the 4519-line store.rs into a store/mod.rs facade plus 12
concern-focused submodules, following the agreed module style. Pure structural
move — no logic changes.

Unlike the kiro converter split, store.rs is a data-model + store-interface
file (97 structs / 16 `*Store` traits / 24 impls, mostly `Empty*`/`Noop*` no-op
impls / 21 fns) and every struct field is already `pub`, so the types move out
cleanly into submodules with zero field-visibility changes — the facade stays
thin. mod.rs keeps only the 55 shared default consts (the canonical `store::X`
constant paths) and re-exports the public API by name.

Submodules:
- config: AdminRuntimeConfig + partial-update view + billing/cache policy
  defaults + billable-token computation.
- keys / groups / proxy: admin key, account-group, and proxy data model +
  page/query/summary helpers.
- codex_account / kiro_account: per-provider admin account model + query/sort/
  summary helpers + import-job types.
- routes: AuthenticatedKey, provider route/auth-update types, codex auth-token
  expiry helpers.
- public: public access keys + community/contribution/sponsor + admin review
  queue request types.
- usage: usage-event + analytics + kiro-latency query/view/snapshot types.
- codex_status: codex rate-limit / credits status types.
- traits: the 16 `*Store` / `UsageEventSink` trait contracts.
- empty: the 15 `Empty*Store` / `NoopUsageEventSink` no-op impls.

Module style:
- mod.rs convention; no `use super::*` in submodules — explicit origin imports.
  Cross-submodule type refs via `use super::<mod>::X`; shared consts via
  `use super::{...}`; external crates (async_trait, serde, base64, serde_json)
  and `crate::{provider::RouteStrategy, usage::UsageEvent}` imported per module
  as used.
- No `pub use X::*` globs: mod.rs re-exports the 127 originally-`pub` named
  items by name, preserving the `store::X` surface (116 distinct symbols
  consumed across llm-access, llm-access-store, llm-access-codex,
  llm-access-kiro, llm-access-migrations).
- Minimal visibility: 5 previously-private helper fns referenced cross-module
  (summarize_admin_keys, apply_admin_key_query, summarize_admin_accounts,
  apply_admin_codex_account_query, default_proxy_binding) are bumped to `pub`;
  the rest stay private.
- Tests inlined: the 3 billing/config tests move into config, the codex-account
  query test + its helper into codex_account (their `super::X` refs resolve to
  the owning submodule with no rewrite).

No behavior change: item inventory diff vs the original shows zero dropped and
zero added items (16 = 16 traits), #[test] count 4 = 4; clippy -D warnings clean
(lib + tests), 10 core tests pass, and the reverse-dep crates (llm-access,
llm-access-store, llm-access-codex, llm-access-kiro, llm-access-migrations)
compile against the unchanged public surface.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces a comprehensive set of storage models, traits, and mock implementations for managing Codex and Kiro accounts, API keys, proxies, and usage analytics in the LLM access gateway. The review feedback highlights opportunities to optimize string filtering in codex_account.rs and keys.rs by removing redundant .to_ascii_lowercase() calls on fields that are already stored as lowercase constants, thereby preventing unnecessary allocations on potentially hot paths.

Comment on lines +252 to +253
|| key.provider_type.to_ascii_lowercase().contains(&search)
|| key.status.to_ascii_lowercase().contains(&search)
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Avoid calling .to_ascii_lowercase() on key.provider_type and key.status as they are already known to be lowercase constants (e.g., "codex", "kiro", "active", "disabled"). Calling it causes redundant allocations on a potentially hot filtering path.

Suggested change
|| key.provider_type.to_ascii_lowercase().contains(&search)
|| key.status.to_ascii_lowercase().contains(&search)
|| key.provider_type.contains(&search)
|| key.status.contains(&search)
References
  1. Avoid calling .to_lowercase() on strings or constants that are already known to be lowercase, as it causes redundant allocations.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Holding for a separate perf PR rather than this one. key.provider_type / key.status are runtime String fields loaded from the store, not compile-time constants — field.to_ascii_lowercase().contains(&search) (with search already lowercased) changes match behavior if any row holds a mixed-case value (e.g. a legacy status of "Disabled" would stop matching a "disabled" search). This differs from the const-literal .to_lowercase() removal accepted in #12, which was a provable compile-time no-op.

This PR is a behavior-preserving structural move (verified by item-inventory diff + the same unchanged tests) and these two fns were relocated verbatim, so a behavior-affecting micro-opt doesn't belong here. Happy to do it in a follow-up perf PR after auditing the write/migration paths to confirm the lowercase invariant, with a regression test.

};
let search = search.to_ascii_lowercase();
account.name.to_ascii_lowercase().contains(&search)
|| account.status.to_ascii_lowercase().contains(&search)
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Avoid calling .to_ascii_lowercase() on account.status as it is already known to be a lowercase constant (e.g., "active", "disabled", "unavailable"). Calling it causes redundant allocations on a potentially hot filtering path.

Suggested change
|| account.status.to_ascii_lowercase().contains(&search)
|| account.status.contains(&search)
References
  1. Avoid calling .to_lowercase() on strings or constants that are already known to be lowercase, as it causes redundant allocations.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as the keys.rs thread — deferring to a follow-up perf PR. account.status is a runtime String from the store, so dropping .to_ascii_lowercase() is a behavior change in the mixed-case-data edge case, not a provable no-op like the #12 const-literal case. Out of scope for this behavior-preserving structural move; will revisit with the write-path invariant verified + a test.

Comment on lines +168 to +171
|| account
.route_weight_tier
.to_ascii_lowercase()
.contains(&search)
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Avoid calling .to_ascii_lowercase() on account.route_weight_tier as it is already known to be a lowercase constant (e.g., "auto"). Calling it causes redundant allocations on a potentially hot filtering path.

Suggested change
|| account
.route_weight_tier
.to_ascii_lowercase()
.contains(&search)
|| account.route_weight_tier.contains(&search)
References
  1. Avoid calling .to_lowercase() on strings or constants that are already known to be lowercase, as it causes redundant allocations.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same reasoning — deferring. account.route_weight_tier is a runtime String field; removing the case-fold changes search behavior if any stored value isn't lowercase. Belongs in a follow-up perf PR (invariant audited + tested), not this structural-move PR.

@acking-you acking-you merged commit 6c99f2a into master May 31, 2026
3 checks passed
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