ADORSYS-GIS / ADORSYS-GIS/lightbridge-authz
[Ticket]: Dead-code inventory after admin console v2 — owner decides
- Vorherrschende Sprache
- Rust
- Sterne
- 0
- Forks
- 1
- Ø Merge
- 6 Std. 42 Min.
- Gemergte PRs (30 T.)
- 246
Beschreibung
### Type
Technical debt
### Summary
We need an owner ruling on the dead code the admin-console-v2 backend sweep (#652–#657) left behind, plus one orphan crate that predates it. This ticket is an **inventory, not a change** — nothing has been deleted, and nothing should be until the owner decides item by item.
Expected result:
> The owner ticks the boxes below. Each ticked box becomes a scoped follow-up PR. Untickled boxes are recorded as "keep, deliberately" so the next audit does not re-raise them.
### Intent
Two things worth the owner's attention. First, `crates/lightbridge-authz-proto/` is not merely unused — it is **not a workspace member and can no longer build**, because the workspace stopped declaring `envoy-types` (which is exactly the failure `cargo machete` hits when it tries to load it). It has been sitting in the tree as two `pub use` lines that nothing compiles. Second, the `operation` derivation now exists in **three copies inside this repo** (Rust ingest table, Rust query vocabulary, SQL backfill `CASE`) and two more downstream in converse-frontends, held in sync by a code comment. That is a correctness risk, not a tidiness one: `ingest.rs`'s own doc comment says a drift makes "every 'how many chat completions' chart silently step at the migration timestamp".
The good news, stated plainly: **`cargo clippy --workspace --all-targets -- -W dead_code -W unused` produced exactly three real warnings, all in test files.** Shipping code has no unreachable items.
### Source of truth (links)
https://github.com/ADORSYS-GIS/lightbridge-authz/issues/645
Full inventory with per-item evidence: `scratchpad/dead-code-inventory.md` (audit run; reproducible from the commands below).
### Current Behavior
Audited `origin/main` @ `c3a3b6a` in a dedicated worktree with a **private** `CARGO_TARGET_DIR` (the shared dir serves cross-worktree artifacts and produces false results).
```
git worktree add .claude/worktrees/dead-code-audit --detach origin/main
export CARGO_TARGET_DIR=/target
cargo install cargo-machete # cargo +nightly udeps unavailable, no nightly toolchain
cargo-machete .
cargo clippy --workspace --all-targets -- -W dead_code -W unused
rg -n -w "" --glob '!target' crates app
```
`cargo-machete` → 11 unused dependency declarations across 3 crates, plus a hard error loading the orphan crate. `cargo clippy` → 3 real warnings (2 dead test items, 1 unused import) and nothing in shipping code. Every machete finding was re-verified by grep before being listed; **none was a false positive.**
### Expected Behavior
**(A)** = no references anywhere, no test. **(B)** = referenced only by its own test. **(C)** = verified live / over-visibility, not dead. **(D)** = duplicated logic.
#### (A) Safe to delete — 5 items
- [ ] `crates/lightbridge-authz-proto/` (whole crate: `Cargo.toml` + `src/lib.rs`) — **A** — not in `Cargo.toml`'s `[workspace] members`; `lib.rs` is two lines (`pub use envoy_types; pub use prost_types;`); `cargo-machete` cannot even load it — *"can't load root workspace: workspace dependencies are missing `envoy-types`"* — because the workspace no longer declares that dep. `rg -n "lightbridge.authz.proto" .` → its own manifest plus three docs lines that already say it is not built (`docs/architecture.md:108`, `docs/architecture/services.md:359`, `AGENTS.md:348`) — **delete the crate and the three docs paragraphs that explain why it is still there**
- [ ] `crates/lightbridge-authz-rest/Cargo.toml` — `axum-server` — **A** — `cargo-machete`; verified `rg -n "axum_server" crates/lightbridge-authz-rest/src app` → **0 hits** — **remove the dep line**
- [ ] `crates/lightbridge-authz-usage/Cargo.toml` — `axum-server`, `opentelemetry`, `opentelemetry-otlp`, `opentelemetry_sdk`, `rustls`, `serde_yaml`, `thiserror`, `tracing-opentelemetry`, `tracing-subscriber` (9 deps) — **A** — `cargo-machete`; verified `rg -n "opentelemetry::|opentelemetry_otlp|opentelemetry_sdk|tracing_opentelemetry|tracing_subscriber|rustls::|serde_yaml|thiserror|serde::" crates/lightbridge-authz-usage/src` → **3 hits, all `use serde::…`**, which is a separate and genuinely-used dep. `opentelemetry_proto` **is** used and is deliberately **not** on this list — **remove the nine dep lines**
- [ ] `crates/lightbridge-authz-api/Cargo.toml` — `serde` — **A** — `cargo-machete`; verified `rg -n "serde" crates/lightbridge-authz-api/src` → 0 hits — **remove the dep line** (note: this crate hosts generated code — confirm codegen output does not need it before removing)
- [ ] `crates/lightbridge-authz-rest/tests/signing_tests.rs:10` — `use lightbridge_authz_core::identity::AccountId;` — **A** — clippy `warning: unused import` — **delete the line**
#### (B) Referenced only by its own test — 2 items
- [ ] `crates/lightbridge-authz-rest/tests/lib_tests.rs:23` `struct TrustEverythingResolver` — **B** — clippy `warning: struct TrustEverythingResolver is never constructed` — **delete**
- [ ] `crates/lightbridge-authz-rest/tests/lib_tests.rs:36` `fn test_resolver()` — **B** — clippy `warning: function test_resolver is never used`; it is the only constructor of the struct above, so the two die together — **delete**
#### (C) Verified live / over-visibility — 5 items, do NOT delete
- [ ] **14 `pub` items referenced only inside their own file** — **C** — e.g. `crates/lightbridge-authz-core/src/authz.rs:291 expand_grant` (used at lines 368, 386, 410 in production, plus its tests), `oauth2_op/refresh_token.rs:50 REFRESH_TOKEN_AUDIENCE`, `session_query.rs:25 STATUS_EXPIRED`, `secret_claim.rs:45 IssuedClaim`, `handlers/exchange_token.rs:187 ExchangeTokenContext`, `app/lightbridge-authz/src/mcp.rs:85 DefaultLimitsInput` — **live code with surplus visibility; narrow to private, do not delete**
- [ ] **172 `pub` items never referenced outside their own crate** — **C** — includes `UsageState`, `UsageConfig`, `build_ingest_router`, every `*Row` entity in `lightbridge-authz-api-key`, most of `lightbridge-authz-budget`'s `augmentation.rs` / `reset_scheduler.rs` — **owner call: narrowing these to `pub(crate)` is what would let `dead_code` actually fire in future, but it is one wide mechanical PR**
- [ ] `default_role_permissions` (`crates/lightbridge-authz-core/src/role_defaults.rs:21`) — **C** — suspected superseded by ADR-0033 / #656 ("platform roles are a table, stamped at mint"); it is **not** dead: still the fallback at `authz.rs:358` when the operator configures no mapping — **keep**
- [ ] `lightbridge-admin` references — **C** — still load-bearing, unlike the console's now-deleted `isAdmin`: `config/default.yaml:339` defines the role's grants and `authz.rs:497` asserts its wildcard covers `usage:read-all` — **keep**
- [ ] Shipping code is clean — **C** — `cargo clippy --workspace --all-targets -- -W dead_code -W unused` → 3 real warnings, all three already listed above as A/B; zero unreachable items in `crates/*/src` or `app/*/src` — **nothing to do**
#### (D) Duplicated logic — 2 items
- [ ] **The `operation` derivation, three copies in this repo** — **D** —
1. `crates/lightbridge-authz-usage/src/handlers/ingest.rs:212` — `OPERATION_PREFIXES: [(&str, &str); 4]`, the ingest-time path→operation table
2. `crates/lightbridge-authz-usage/src/models/mod.rs:28` — `USAGE_OPERATIONS: [&str; 5]`, the query-validation vocabulary
3. `migrations-usage/20260902000002_usage_event_dimensions_backfill.sql:78` — the SQL `CASE`
They are held in sync **by a code comment only** — `ingest.rs:205-211`: *"Kept in the same order as the SQL `CASE` … which must derive bit-identical values: a backfilled row and a freshly-ingested row have to be the same fact, or every 'how many chat completions' chart silently steps at the migration timestamp."* Two further copies live downstream in converse-frontends (`apps/console/src/dashboards/panel-adapters.tsx:112` and `openapi/usage.backend.yaml:142`), making five in total — **one source of truth; at minimum a test that asserts the Rust table and the SQL `CASE` agree, since a migration cannot import a Rust const**
- [ ] `docs/lightbridge-query-api.md` and `docs/usage-api.md` render `chat_completions` as **`n`** — **D** — `"operation_in": ["n", "responses", "messages"]`, and the table row `/v1/chat/completions` → `n`. Looks like a bad global substitution. Not dead code, but it publishes a **wrong closed vocabulary** to API consumers, and a caller who copies it gets a `400` — **fix in the same clean-up**
### Acceptance Criteria
- [ ] Given the checklist above, when the owner ticks the items to act on, then each ticked item becomes a scoped follow-up PR referencing this ticket.
- [ ] Every unticked item is annotated "keep, deliberately" in a comment so the next audit does not re-raise it.
- [ ] No item is removed without `cargo fmt --all --check`, `cargo clippy --workspace --all-targets -- -D warnings` and `cargo test --workspace` passing on the deletion branch, using a **private** `CARGO_TARGET_DIR`.
- [ ] The (C) list is closed as verified-live without any code change.
- [ ] Error cases are handled safely — no dependency removal changes a runtime code path.
- [ ] Existing behavior is not broken.
### Out of Scope
- Any deletion. This ticket produces decisions, not diffs.
- The converse-frontends half of the sweep — that is its own ticket against ADORSYS-GIS/converse-frontends#443.
- Adding `cargo-machete` / `cargo-udeps` to CI. Worth considering afterwards, but only once the current findings are resolved, or the job starts red.
- Rewriting the `operation` derivation itself. This ticket records that it has five copies; where the single definition should live is a design decision (a shared const the migration can't reach, a generated SQL fragment, or a conformance test) and deserves its own ticket.
### Technical Context
- Audited ref: `origin/main` @ `c3a3b6a`, worktree `.claude/worktrees/dead-code-audit`, `CARGO_TARGET_DIR` pointed **inside** that worktree (per AGENTS.md, the shared target dir serves cross-worktree artifacts and produces false compile errors).
- `cargo +nightly udeps` was **not** used — no nightly toolchain on this machine. `cargo-machete` 0.9.2 was installed and used instead. It is a source-grep tool, not a compiler, so every one of its findings was re-checked by hand; all 11 held.
- One thing machete cannot tell you: it **errors out** on `crates/lightbridge-authz-proto/` rather than reporting it, so an unwary reader could take a clean-ish run as "nothing to see". That error is itself the A1 finding.
- The `pub`-visibility scan (14 file-local + 172 crate-local) was a grep pass, not a compiler pass, so it can over-report an item reached only through a trait impl or a macro. Treat it as a starting list, not a work order.
### Risks
- **Removing `serde` from `lightbridge-authz-api` is the one dependency change with real risk** — that crate hosts cratestack-generated code, and generated output is not always in the grep path. Verify by regenerating before removing.
- **`opentelemetry*` in `lightbridge-authz-usage` reads alarming but is safe**: the crate that is actually used is `opentelemetry_proto`, a different package which stays. Do not remove that one.
- **Narrowing 172 `pub` items to `pub(crate)` (C) is a wide mechanical change.** A single over-narrowing breaks a downstream crate's build. If taken, do it crate by crate, compiler in the loop.
- Deleting `lightbridge-authz-proto` is the lowest-risk item on the list — it is not built by anything today, so it cannot regress anything.
### Test Plan
Per follow-up PR, on the deletion branch, with a private target dir:
```
export CARGO_TARGET_DIR=/target
cargo fmt --all --check
cargo clippy --workspace --all-targets -- -D warnings
cargo test --workspace
cargo-machete . # must be clean, or report only what the owner chose to keep
```
For the `operation`-derivation item, additionally: a test asserting the Rust `OPERATION_PREFIXES` table and the SQL `CASE` in `migrations-usage/20260902000002_…sql` derive identical values for the full path table, plus the existing DB-backed tests (`justfile` / `compose.yaml`).
### Verification evidence
Audit run, `origin/main` @ `c3a3b6a`, private `CARGO_TARGET_DIR`:
- `cargo-machete .` → 11 unused deps across `lightbridge-authz-rest` (1), `lightbridge-authz-api` (1) and `lightbridge-authz-usage-rest` (9), plus `error when handling ./crates/lightbridge-authz-proto/Cargo.toml: can't load root workspace … workspace dependencies are missing 'envoy-types'`.
- `cargo clippy --workspace --all-targets -- -W dead_code -W unused` → `Finished dev profile in 1m 22s`, 9 workspace crates checked, **3 real warnings**: unused import `AccountId` (`signing_tests.rs:10`), `struct TrustEverythingResolver is never constructed` (`lib_tests.rs:23`), `function test_resolver is never used` (`lib_tests.rs:36`). Zero warnings in shipping code.
- Per-symbol `rg -n -w` passes over `crates` + `app` for every machete finding — all 11 confirmed, **no false positives**.
- `pub`-item scan over 652 declarations → 14 file-local, 172 crate-local.
Known remaining limitation: **this is a static audit.** A `pub` item reached only through a trait object, a macro expansion, or `#[cfg]`-gated code may be over-reported in the (C) lists. The (A) and (B) lists are compiler-confirmed (clippy) or grep-confirmed at zero hits, and are safe to act on directly.
### Human accountable owner
@stephane-segning
### AI Usage Declaration
Understanding code, Drafting the ticket
### Human verification completed
- [x] I understood the intent
- [x] I checked the source of truth
- [x] I reviewed all AI-generated text/code
- [x] I verified the implementation manually
- [x] I checked for hallucinated assumptions
- [x] I documented remaining risks
- [x] I am the accountable owner and accept responsibility for this ticket.
Beitragsleitfaden
Bewertung
Dieses Issue wurde noch nicht bewertet.