Hygiene & Documentation¶

Rules for code quality, vendor documentation citations, and structural cleanup.

RULE-004: Protocol Constants Must Be Documented¶

Severity: High

Description: Every protocol constant (command codes, event IDs, packet types, magic values) must be traceable to vendor documentation or provided source material. No guessing from patterns.

Detection pattern:

Enum values or constants without XML doc comment citing source
Sequential or pattern-based values that suggest interpolation
Comments like "probably", "seems to be", "assuming"

Correct pattern:

/// <summary>Zone alarm event. Vendor doc: Table 4.2, Event Code 0x10.</summary>
ZoneAlarm = 0x10,

Violation example:

// WRONG: no source citation, possibly guessed
ZoneAlarm = 16,
ZoneTamper = 17,  // <- is this really 17 or was it guessed from pattern?

Value cross-check (when citation-map.json exists): If a citation map (citation-map.json) exists for the adapter, presence of a citation comment is not sufficient. The reviewer must also cross-check the cited constant's VALUE in code against the value recorded in the map entry (matched by constant name, falling back to value match when the name differs):

Match, values equal → Verified, no finding.
Match, values differ → Critical finding, not just High. A citation comment that points at the right vendor-doc location but carries the wrong byte/code is worse than no citation at all — it looks trustworthy while being wrong. Wrong bytes are a safety hazard (wrong command code sent to a physical panel, wrong event code misrouted). Report the code value, the map value, and the citation-map source/page/section so the human can re-check against the actual vendor doc.
No entry in the map for that name/value → falls back to the presence-only check (RULE-004 base rule): a citation comment is still required, but nothing further to cross-check.

When no citation-map.json exists for the adapter (or it exists but the constant is absent from constants), presence-only checking is the correct and complete verification — do not fabricate a mismatch finding from thin air.

RULE-005: Address Resolution Must Handle Unknown Addresses¶

Severity: Medium

Description: Event routing must handle events from unknown/unconfigured addresses gracefully. Log or publish as unresolved - never silently drop.

Detection pattern:

FindByAddress() returning null with no subsequent handling
Event routing that returns early on null without logging
Missing "unknown address" diagnostic events

Correct pattern:

var zone = FindZone(evt);
if (zone is null)
{
    _logger.LogWarning("Event from unknown zone address {Address}", evt.Address);
    // Optionally: publish diagnostic event
    return;
}
await zone.ProcessEvent(evt);

Violation example:

// WRONG: silently drops event
var zone = FindZone(evt);
if (zone is null) return;

RULE-019: No Empty Partial for Generated Things¶

Severity: Low

Description: For Thing types declared in adapter-registration.yaml, the source generator produces a complete class. An additional user-written partial class with no members adds nothing - it just duplicates the declaration and obscures which Things actually have hand-written logic.

This rule targets only empty partials that shadow a generated Thing class. Empty classes in general (helpers, future placeholders elsewhere) are not the concern.

Why this matters:

Reviewer can't tell at a glance which Things need user code vs. which are generated-only
Duplicate XML docs (in generated + user partial) lead to confusion when they diverge
Hides the intent - was this stub forgotten, or is the Thing fully generated?

Detection pattern:

Class name matches a type_id in adapter-registration.yaml
File contains only namespace + XML doc + empty partial class X { } body
No fields, properties, methods, constants, or nested types

Violation example:

// WRONG: Pq.Adapter.Acme.Panel/Devices/Concentrator.cs
// Concentrator is declared in adapter-registration.yaml → generator produces complete class
namespace Pq.Adapter.Acme.Panel;

/// <summary>
/// Acme bus concentrator module Thing.
/// </summary>
public partial class Concentrator
{
}

Correct approach:

If Thing has no user code: delete the file - the source generator produces a complete class
If Thing needs user code later: add the file when the logic is actually needed
If documentation describes architecture: put it in the adapter's design-notes document instead

Exception: None for generated Things. Empty classes for unrelated purposes (placeholder helpers, marker interfaces) are not covered by this rule.

Verification:

Cross-reference partial class X { } files with type_id: X entries in adapter-registration.yaml
Build still succeeds after deletion (proves the file was redundant)

RULE-023: Adapter Documentation Must Not Drift from Code¶

Severity: Medium

Description: Adapter-local documentation must not contradict current YAML, hand-written code, or generated shape. Stale design decisions are dangerous because future adapter work and reviews use them as evidence.

Detection pattern:

The adapter's design notes, implementation notes, or decision records contradict current code
Design-notes behavior notes contradict current protocol/access-sync implementation
CHECKLIST.md marks a capability complete/partial incorrectly
Adapter docs mention old address types, SDK handles, enum mappings, conversion rules, or lifecycle assumptions

Correct pattern:

Update adapter docs in the same change that changes behavior
Keep only context that explains the active decision
Cite the current source of truth when protocol behavior is derived from docs or SDK inspection

Verification:

Compare the adapter's design notes, implementation notes, or decision records against access-model.yaml, adapter-registration.yaml, hand-written Protocol.cs/AccessSynchronization.cs, and generated files when relevant
Flag contradictions even when the code itself is correct

RULE-033: Persistent Settings Must Be Constructor-Injected Directly¶

Severity: Medium

Description: Thing-local persistent state must use direct IPersistentSettings<T> constructor injection. Adapters must not manually inject or call PersistentSettingsFactory, add optional persistence factory parameters, or add parameterless constructor overloads as a workaround for persistence. The generated tree builder and TreeServiceProvider resolve IPersistentSettings<T> automatically for root and child Things.

Detection pattern:

Hand-written Thing constructor accepts PersistentSettingsFactory
Calls to PersistentSettingsFactory.Create(...) from adapter Thing code
Optional constructor parameters such as PersistentSettingsFactory? factory = null
Extra parameterless constructors added only to avoid DI ambiguity while also using persistence

Correct pattern:

public partial class Panel(IPersistentSettings<PanelPersistentState> state) : IDeviceConnection
{
}

Violation example:

// WRONG: manual factory wiring and fallback constructor
public partial class Panel(PersistentSettingsFactory? factory = null) : IDeviceConnection
{
    private readonly IPersistentSettings<PanelPersistentState>? _state =
        (IPersistentSettings<PanelPersistentState>?)factory?.Create(typeof(IPersistentSettings<PanelPersistentState>), this);
}

public Panel()
{
}

Correct approach:

Inject IPersistentSettings<T> directly into the Thing constructor.
Use a single applicable constructor, like the existing example adapters.
Keep PersistentSettingsFactory usage inside framework infrastructure, not adapter Thing code.

Verification:

Search hand-written adapter code for PersistentSettingsFactory.
Search Thing classes with persistence for multiple constructors.
Build the adapter to catch ActivatorUtilities constructor ambiguity.

RULE-034: Adapter Global Usings Belong in the Project File¶

Severity: Low

Description: New adapter projects should declare adapter-wide global usings in the .csproj with <Using Include="..." /> items. Do not create a hand-written GlobalUsings.cs file for routine framework usings.

Detection pattern:

GlobalUsings.cs exists at the adapter root.
The file contains only global using ...; declarations that could be represented as project <Using> items.

Correct pattern:

<ItemGroup>
  <Using Include="Microsoft.Extensions.DependencyInjection" />
  <Using Include="Microsoft.Extensions.Hosting" />
</ItemGroup>

Violation example:

// WRONG: Pq.Adapter.Acme.Panel/GlobalUsings.cs
global using Microsoft.Extensions.DependencyInjection;
global using Microsoft.Extensions.Hosting;

Verification:

Delete the redundant GlobalUsings.cs.
Add equivalent <Using Include="..." /> entries to the adapter .csproj.
Build the adapter project.

RULE-036: Framework-Bending Smell¶

Severity: High

Description: An adapter that hits a framework limitation must record it as a framework gap for review — never quietly work around it by re-implementing a framework primitive locally. A local re-implementation is not a hygiene nit on its own merits; it is a smell that the framework-gap review was skipped. This rule does not ban workarounds outright — it flags them for review, because a workaround is legitimate only when the developer explicitly records the decision.

Detection pattern (framework primitives duplicated locally):

Manual retry/backoff loops (for/while with Thread.Sleep/Task.Delay around a send-and-wait) that duplicate ProtocolChannel's built-in retry/follow-up handling
Hand-rolled Thing lookup tables/dictionaries keyed by address or ID that duplicate DeviceTreeRegistry resolution instead of resolving Thing IDs through it at execution time
Hand-rolled persistence (manual file/JSON read-write, Dictionary backing fields flushed to disk) instead of injecting IPersistentSettings<T> (see RULE-033)
Collapsing a typed protocol stream into ProtocolBase<object> (or object-typed frame handling) instead of using the generated typed codec/frame split

Corroborating signal:

An undocumented assumption or known documentation gap near the same code path, with no recorded decision explaining why the framework primitive is re-implemented rather than handled through a framework-gap decision. An unresolved gap sitting next to a local re-implementation is strong evidence the review is missing.

Correct pattern:

Hit the limitation → STOP → record the framework gap (what the protocol needs, what the framework offers, options with costs/risks) → complete review → record the decision in the adapter's design notes / plan.
An adapter-local workaround may then exist in code, but only traceable to that recorded decision.

Violation example:

// WRONG: hand-rolled retry loop duplicating ProtocolChannel's retry/follow-up handling,
// with no plan entry or recorded decision explaining why ProtocolChannel wasn't used
for (var attempt = 0; attempt < 3; attempt++)
{
    await _port.WriteAsync(frame, ct);
    if (await TryReadReply(ct)) return;
    await Task.Delay(500, ct);
}

Verdict semantics: This rule never resolves to a silent pass/fail by itself — it always flags for review.

If the plan or a gap-disposition record cites the developer's explicit choice of this workaround, reference that citation and mark it Passed with citation.
If no such record exists, report it as a High finding for review — do not assume it is acceptable, and do not assume it must be deleted; only the user's recorded choice resolves it.

Verification:

Grep hand-written adapter code for retry loops, manual dictionaries keyed by address/ID, manual file/JSON persistence, and ProtocolBase<object>/object-typed frame handling.
Cross-reference each hit against the adapter's design notes / plan file for a recorded framework-gap decision explaining the workaround.

RULE-049: Closed-Set Properties Must Be Enums, Not Strings¶

Severity: High

Description: A property whose valid values are a fixed, closed set (e.g. a direction that is only Entry or Exit; a mode that is only A, B, or C) must be declared as an enum-typed property in adapter-registration.yaml — type: <EnumType> referencing a named enum. Representing a closed set as type: string (or as a bare type: int opcode) is a hard violation, not a style preference. A string erases the closed set: it accepts arbitrary or misspelled values, carries no validation, produces no generated type safety, and hides the domain from the UI, from other adapters, and from reviewers. An enum-typed property makes the generator emit a real enum type with value validation; a string gives none of that.

Detection pattern:

A type: string property whose description enumerates a fixed set of allowed values ("A, B or C", "Entry or Exit", "normal/latching/non-latching", "Server or Client").
Hand-written code that switches or compares a string property against a fixed literal set (switch (mode) { case "A": ... }, if (dir == "Entry")).
A type: int property used only as an opcode for a small named set of choices.

Correct pattern:

// Definitions/ConnectionMode.cs — the closed set as a named enum
public enum ConnectionMode
{
    Server,
    Client,
}

# adapter-registration.yaml — property references the enum type by name
connection_mode:
  type: ConnectionMode
  default: "Server"
  description: "Server = adapter connects to panel, Client = panel connects to adapter"

Violation example:

# WRONG: a closed set modelled as free-text string
connection_mode:
  type: string
  default: "Server"
  description: "Server or Client"   # <- the fixed set proves this must be an enum

Verification:

Scan adapter-registration.yaml for type: string (or opcode-like type: int) properties whose description names a fixed set of values.
Cross-check hand-written code for string/int comparisons against a constant literal set of that property.
Confirm the property references a declared enum type instead.