Coding Standards

Coding Standards

The one canonical statement of how code is written in this repository — the architecture rules, the comment/documentation rule, and the object-oriented rules a change is held to. This wiki page is the single source of truth. The repository's CLAUDE.md, CONTRIBUTING.md, and docs/ARCHITECTURE.md point here; they do not restate these rules, so there is exactly one place to keep current.

These are not aspirations. Where a rule can be made executable it is enforced by a conformance fitness function (a test that runs in dotnet test on every PR) or a greppable lint rule; the rest are enforced by the adversarial Review Gate. Where the code and this page disagree, the code and its conformance tests win — and this page is corrected.

The ordering, when rules pull against each other, is security > minimal code > readability. We never trade a fail-closed guard for fewer lines or a nicer name.

1. Architecture rules

The plugin is one assembly whose behaviour is decomposed into a hexagonal module DAG. The full shape — the Api/<Module> layout, the dependency graph, and the dissolved kernel — is on the Architecture page; the rules a change must satisfy are:

• Module DAG, no cycles. New code lands in the right module folder + namespace (Jellyfin.Plugin.SSO_Auth.Api.<Module>). A module imports only the modules declared as its allowed dependencies; a new import edge is a deliberate change to the conformance test's edge list, and a cycle fails ApiModule_ImportsOnlyItsAllowedApiModules. A genuinely new concern earns a new module (folder + namespace + a DAG case), never a smear across the tree. The flat Api/ root stays empty (FlatApi_HoldsNoSourceFiles_EveryApiTypeLivesInAModule).

• Illegal states unrepresentable. Model decisions with more than two outcomes as closed sum types, not booleans or nullable flags, and let the compiler force exhaustive handling. LoginOutcome (Success / Rejected(PublicReason) / Denied, with no Error case) is the model to follow. The most security-critical guarantees are made constructional: an evidence type such as VerifiedIdentity can only be minted by the protocol validators (private constructor + named factory), so reaching account resolution with an unvalidated response is a compile error, not a review catch (VerifiedIdentity_IsConstructedOnlyByProtocolValidators).

• Fail closed by construction. This is a login path. A missing signature, time-bound, audience, subject, replayed assertion, unresolved identity, or an unmapped outcome case → reject or throw, never default-accept. No default branch that accepts. A throwing accessor turned into a guarded Try* accessor must have its new miss-branch explicitly re-decided as a rejection, with a negative test covering it — silently converting fail-closed into fail-open is the classic regression this rule exists to stop.

• No ambient time. Time enters through an injected clock; time-sensitive keyed state is keyed on UTC, not machine-local time (OidcAuthorizeState_IsKeyedOnUtc_NotMachineLocalTime). No DateTime.Now / ambient wall-clock reads on the decision path.

• Sealed, immutable, typed. Types are sealed unless a genuine is-a needs otherwise; state variants are immutable records (AuthorizeStates_AreImmutableVariants, SamlLoginOutcome_IsImmutable); values are typed rather than raw strings where a raw string invites a category error (ProviderScopedKey, ProviderMode parsed once at the edge). Mutable keyed state lives only inside store-like types (MutableKeyedState_LivesOnlyInsideStoreLikeTypes); the controller holds none (Controller_HoldsNoMutableStaticState).

• Parametrized data access. The plugin persists through Jellyfin's typed configuration model, not hand-built query strings; all config access routes through the ProviderConfigStore facade (a greppable lint rule enforces the single road to disk). The equivalent of "parametrized queries only" here is: no string-concatenated storage keys or query fragments — provider-scoped keys are typed values, not concatenations.

• Deliberate asymmetries stay deliberate. The rate limiter is fail-open by design (mass-lockout protection for a sole-admin server) and its result type is named so it can never be confused with a fail-closed gate. Log-forging sanitization stays inline at every log call and is never extracted into a helper — the CodeQL cs/log-forging sanitizer boundary does not survive a method boundary, so a helper would defeat the analysis, not just the style.

2. Comments and documentation (#864)

• XML documentation is mandatory and complete on the whole surface — public and internal. Every type and member carries an XML doc comment describing what it is for. This is enforced by StyleCop SA1600 with documentInternalElements = true, so the internal helper surface is documented to the same bar as the public plugin contract, not just the types Jellyfin's plugin contract forces public. A conformance test pins both switches (SA1600 at warning-or-error and documentInternalElements = true) so the #864 internal-doc gate cannot be quietly switched off.

• Inline // comments are forbidden, with three explicit exceptions:

  1. load-bearing security "why" comments — the issue-referenced comments that guard a fail-closed decision; these are part of the invariant surface and travel verbatim with the code they guard when it moves, and reviewers treat them as acceptance criteria;
  2. analyzer-suppression justifications — the required rationale next to any deliberate suppression;
  3. conformance-test rationale — the "why this rule exists / why this asymmetry is intentional" notes in the fitness functions.

  Everything else is expressed in the XML doc or in the code itself. A comment that narrates what the code does is deleted along with the mechanics it narrated: the test for introducing a new named type is whether it lets an explanatory comment or a string convention be removed. Comments explain why, never what.

3. Object-oriented rules

• An abstraction must earn its place: ≥ 2 real polymorphic consumers. An interface or abstract base is introduced only when there are two or more genuine polymorphic consumers of it. A single-implementation interface, or an abstraction added "for future extensibility," is premature and is removed. Symmetry between the OIDC and SAML flows is expressed by identical named-step shape, not by a shared inheritance tower.

• Composition over inheritance — including in tests. Shared behaviour lives behind a shared abstraction realised composition- and interface-first, with shallow inheritance only where the relationship is genuinely is-a (never a deep inheritance chain). ProviderConfigBase, shared by OidConfig and SamlConfig, is the model to follow; a third IdP type must slot into that shared abstraction, not fork a third parallel path. This includes test code: there are no test base classes — a test that needs shared setup uses a composition helper (e.g. a factory that returns a configured value), so a test is self-contained and its setup is visible where it is used.

• Helper suffixes are a contract. A pure, single-responsibility leaf is named for what it does — *Validator, *Builder, *Mapper, *Resolver, *Policy, *Probe, *Extractor — and is internal and sealed or static, never part of the plugin's public surface and never an inheritance base. Keyed runtime state is a *Store / *Cache / *Gate / *Limiter. Stateful collaborators are *Service, internal sealed. Verbs carry semantics: TryRedeem is consuming and at-most-once, PeekCurrent is non-consuming; Try* returns nullable/bool and never throws for a client-caused condition. These suffix rules are enforced by the conformance tests.

How these are enforced

Three mechanisms, in order of how early they catch a violation:

1. Conformance fitness functions — the structural rules above run in dotnet test on every PR; a new structural property is locked in as a new fitness function in the same PR that establishes it. This is how the architecture stays consistent rather than drifting.
2. Greppable lint rules (the repo's opengrep invariants) — textual invariants that are not structural: CSPRNG-only randomness, config access only through the store facade, no raw outbound HttpClient, no controller error- detail leak. One rule per invariant, added per regression.
3. The adversarial Review Gate — everything not mechanically decidable (intent, OO fit, comment-necessity, fail-closed reasoning) is checked by the refute-by-default review lenses on every code PR, escalating to the full multi-lens gate on the login / crypto / config / pipeline surface.

See also

• Architecture — the module DAG, the dissolved kernel, and the login-flow code map these rules produce.
• Review Gate — the internal-only merge gate that enforces the non-mechanical rules, class by class.
• Security Model — the fail-closed invariants the rules protect.