ADORSYS-GIS / ADORSYS-GIS/lightbridge-authz

[Ticket]: Dead-code inventory after admin console v2 — owner decides

Offen
#660 0 Kommentare 0 Reaktionen 1 zugewiesene Person Beansprucht von @stephane-segning Auf GitHub ansehen
ticket
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

Beitragsleitfaden öffnen

Bewertung

Dieses Issue wurde noch nicht bewertet.

Neue Issues direkt in Ihr Postfach

Eine kurze Übersicht über anfängerfreundliche GitHub-Issues.