refactor: drop single-impl traits, fix all 49 doc-link warnings, dedupe docs - #544
Merged
Conversation
Each of DlpPipeline, RequestRouter, AuditWriter, EventTap and SpendTracking had exactly one implementation whose every method delegated to the concrete type. Four of the five were never used as `dyn` or as a generic bound anywhere: the concrete type was already the field type in AppState. The abstraction cost 320 lines and bought nothing. SpendTracking was the only real `Box<dyn>`, at server/mod.rs:152, but it too had a single implementor. It becomes `Mutex<SpendTracker>`. Its default method bodies existed to keep test mocks no-op, and those mocks were deleted in the previous commit. Two methods lived only inside a trait impl and are now inherent: `DlpEngine::scan_input_enabled`/`scan_output_enabled` (only exercised by mutation-killing tests) and `SpendTracker::provider_breakdown` (called by rpc/budget_ns.rs). Tracer and ProviderAvailability stay: each has two real implementations and is genuinely polymorphic at server/mod.rs:406-411. traits.rs: 458 -> 120 lines. 1727 tests pass, clippy clean with all features and with --no-default-features, doctests pass.
An ADR records a decision and its rationale. ADR-0018 carried 378 lines of sprint material after the decision: target TOML schemas for a schema that does not exist yet, an RE-0..RE-8 phase flowchart, per-phase effort estimates in LoC, pseudocode sketches for five primitives, a full speculative schema reference, and before/after migration examples. None of it is referenced from anywhere else in the tree. Kept: context, drivers, the five options with pros and cons, the decision, consequences, confirmation, internal and external references, the biomimetic glossary (it explains why the code uses engineering names, not animal ones), and the open questions. 805 -> 427 lines. markdownlint clean.
The README files and the feature matrix still advertised the five traits deleted in the previous commit. AGENTS.md claimed '7+ traits enable testing via mock implementations', which was the justification for an abstraction layer whose mocks had zero call sites. Also fixes an inverted claim in src/security/README.md: it described the security registry as per-endpoint and the routing breaker as active. It is the other way round. The two breakers coexist deliberately at different granularities, so the note now says which is which and why neither subsumes the other.
cargo doc emitted 49 warnings, every one a dead reference in published API documentation. The failure modes were: links from a module-level //! block to an item in that same module (rustdoc needs an absolute path there), links to enum variants written as bare names, links to private items from public docs, and two bare URLs in clap help text. Some were simply wrong: prompt_injection.rs pointed at a scan function on a type that does not exist, and dlp/mod.rs referenced scan_end_of_stream without its type. Two module headers listed their own submodules in prose. rustdoc already generates that index, so the list was duplicated maintenance that had drifted; removed from shared/mod.rs, kept as plain text in routing/mod.rs where the surrounding prose carries real information. Adds deny(broken_intra_doc_links) and deny(private_intra_doc_links). A warning nobody reads is not a signal; this makes a dead link fail the build the way missing_docs already does. 49 warnings -> 0. 1727 tests pass, 24 doctests pass, clippy clean.
docs/ had two entry points. docs/README.md was a flat link list, docs/index.md had the same links plus context, a request-flow diagram, and task-oriented tables. Nothing in the repository linked to docs/README.md, and the two had already drifted apart. Merged the eight links only README carried into index.md, then deleted README. Same pattern for tutorials: quickstart.md was a strict subset of getting-started.md and had gone stale, claiming the perf preset configures 'Anthropic + OpenAI + Gemini' when it is Anthropic OAuth with no fallback at all. The three-command path now opens getting-started.md as an 'In a hurry' section, so there is one tutorial instead of two that disagree. The ADR table in index.md was maintained by hand and was wrong in five places: 0018 and 0019 were marked proposed though both are accepted, 0004 and 0028 did not show they were superseded, and 0027, 0028, 0029 were missing entirely. It is now generated from the status field of each ADR. Also linked four docs that no index reached: the protocol fidelity matrix, the OTLP exemplars explanation, the design/ directory, and the hedge-billing verification protocol that ADR-0020 depends on.
Two root commands did the same job. 'grob completions <shell>' printed a script to stdout, 'grob setup-completions' detected the shell and installed it, and a new user had to read both help texts to learn which one they wanted. The shell argument is now optional: 'grob completions' installs for the detected shell, 'grob completions zsh' still writes to stdout for packaging or a custom path. Both code paths are unchanged, only the entry point merged. 'setup-completions' stays as a hidden alias, so scripts and muscle memory keep working while the help text lists one command instead of two. 25 visible root commands become 24.
Destynova2
force-pushed
the
refactor/drop-single-impl-traits
branch
from
August 6, 2026 07:45
53e04db to
50dc91c
Compare
Destynova2
enabled auto-merge (squash)
August 6, 2026 07:45
Destynova2
changed the title
Merged
Destynova2
added a commit
that referenced
this pull request
Aug 6, 2026
## 🤖 New release * `grob`: 0.36.96 -> 0.36.97 <details><summary><i><b>Changelog</b></i></summary><p> <blockquote> ## [0.36.97](v0.36.96...v0.36.97) - 2026-08-06 ### Other - drop single-impl traits, fix all 49 doc-link warnings, dedupe docs ([#544](#544)) </blockquote> </p></details> --- This PR was generated with [release-plz](https://github.com/release-plz/release-plz/).
Contributor
Mutation testing (PR diff sample)Informational — never blocks merge. Full matrix runs on main.
Legend: clean (no survivors), missed (inspect artifact), timed-out (25 min cap reached). Artifact: mutants-pr-results-8ff9d333b492d1f2087a8b152d3c64eb2cd28423. |
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
1 participant
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.
Second pass of the audit. #543 removed the dead mocks; this removes the abstraction they existed to serve, fixes every broken doc link, and collapses the duplicated docs.
Code: the five single-impl traits
DlpPipeline,RequestRouter,AuditWriter,EventTap,SpendTrackingeach had exactly one implementation, and every method in it delegated straight to the concrete type.Four of the five were never used as
dynor as a generic bound anywhere. The concrete type was already the field type inAppState:AppStatefieldRequestRouterrouterRouterAuditWriteraudit_logOption<Arc<AuditLog>>EventTaptap_senderOption<Arc<TapSender>>DlpPipelineDlpEngineSpendTrackingwas the one realBox<dyn>, atserver/mod.rs:152, but also had a single implementor. It becomesMutex<SpendTracker>. Its default method bodies existed only so test mocks could no-op, and those mocks went in #543.TracerandProviderAvailabilitystay. Each has two real implementations and is genuinely polymorphic atserver/mod.rs:406-411.traits.rs: 458 to 120 lines.Docs: 49 broken intra-doc links to zero
Every one was a dead reference in published API documentation. Failure modes: links from a module-level
//!block to an item in that same module (rustdoc needs an absolute path there), enum variants written as bare names, links to private items from public docs, and two bare URLs in clap help text. Two were simply wrong, pointing at functions on types that do not exist.Adds
deny(broken_intra_doc_links)anddeny(private_intra_doc_links). A warning nobody reads is not a signal; a dead link now fails the build the waymissing_docsalready does.Docs: two entry points became one
docs/README.mdanddocs/index.mdwere both indexes of the same directory. Nothing linked to README, and the two had drifted. Merged the eight links only README carried, then deleted it.tutorials/quickstart.mdwas a strict subset ofgetting-started.mdand had gone stale, claiming theperfpreset configures "Anthropic + OpenAI + Gemini" when it is Anthropic OAuth with no fallback. The three-command path is now an "In a hurry" section of the one remaining tutorial.index.mdwas hand-maintained and wrong in five places (0018 and 0019 marked proposed though accepted, 0004 and 0028 not shown as superseded, 0027/0028/0029 missing). Regenerated from each ADR'sstatusfield.design/, and the hedge-billing protocol ADR-0020 depends on.ADR-0018
Carried 378 lines of sprint material after the decision: target schemas for a schema that does not exist, an RE-0..RE-8 flowchart, effort estimates in LoC, pseudocode, migration examples. Nothing references any of it. Kept context, drivers, options, decision, consequences, references, glossary, open questions. 805 to 427 lines.
Usage: one command instead of two
grob completions <shell>printed to stdout;grob setup-completionsdetected the shell and installed. A new user had to read both help texts to find out which they wanted. The shell argument is now optional.setup-completionsstays as a hidden alias, so nothing breaks. 25 visible root commands become 24.Deliberately not done
security::CircuitBreakerRegistryis per provider and backsProviderAvailability;routing::circuit_breakeris per endpoint with lock-free atomics on the hot path. Different granularities, both live.ProviderBaseinGeminiProvider. Worth ~40 LOC, but Gemini'sapi_keyisOption<SecretString>whereProviderBase's isSecretString. Making a shared field optional to suit one provider costs more than the duplication.Verification
1727 tests pass; clippy clean with
--all-features --all-targetsand--no-default-features; doctests pass;cargo docemits zero warnings (was 49); fmt and markdownlint clean; both completion paths exercised by hand.