spec-module-review

name

spec-module-review

description

Review methodology for modules implementing a published specification (OTel, W3C, RFC, OTLP, Semantic Conventions). Apply whenever the user asks to review, audit, verify, refactor, or check spec-compliance of a file citing specifications — docstrings with section references like `§3.3.1.3.1` or `spec §3.3`, module paths under `apps/otel_api/lib/otel/api/`, or comments referencing specs under `references/`. Also triggers for requests like "verify citations", "find spec gaps", "check spec compliance", "is this aligned with the spec", or plain "review this file" when the file is spec-aligned. Use this even when the user doesn't explicitly say "review" — any task cross-checking code against a published specification qualifies.

spec-module-review

Review methodology for otel modules implementing published specifications. Distilled from the Otel.API.Trace.TraceState review (PRs #155–#159), where multiple fake citations, hidden spec gaps, and bogus logic were found in code that had already passed normal review.

When this skill applies

Any review of a module that cites a published specification. Signals:

• @doc / @moduledoc with section numbers like §3.3.1.3.1
• Module path under apps/otel_api/lib/otel/api/ (OTel API surface)
• Constants with spec-path comments (e.g. # W3C §3.3.1.1: ...)
• references/ submodules present (opentelemetry-specification, w3c-trace-context, w3c-baggage, opentelemetry-erlang, etc.)

Operating principle

references/ is the only authority. Never use AI-trained spec knowledge.

Verify with primary sources. Never trust a citation because it sounds right, and never trust your own training data about what the spec says. Three of the fake citations caught in the tracestate review (PRs #155–#159) looked entirely plausible. Worse: in the LogRecord attribute audit (PR #304/#305 + post-merge cleanup), prior Agent reviews concluded attributes :: %{String.t() => primitive() | [primitive()]} matched spec — and they were correct for the spec version at that time. Then the spec submodule moved forward and the text changed (v1.53.0, #4794 "Stabilize complex AnyValue attribute value types"). The prior conclusion silently became wrong.

Concrete rules:

• Open the actual file under references/ for every claim — don't infer the spec text from your training corpus, OTel blog posts, common SDK conventions in other languages, or how things "usually work" in OpenTelemetry. Those sources are stale, generalised, or misremembered; only the file is authoritative.
• Memory is unreliable for spec text. Section numbers drift between spec versions. Paraphrases invert meaning (SHOULD ↔ MUST, minimum ↔ maximum, characters ↔ bytes). Re-quote verbatim from the file every time.
• AI training cutoffs lag the spec. OTel spec releases monthly or faster. Anything you "know" about attribute types, LogRecordProcessor callbacks, propagator interfaces, etc. may be 6+ months out of date. The pinned references/ submodule is the current truth.

The submodule under references/opentelemetry-specification/ (plus W3C / RFC / OTLP / semantic-conventions submodules) is the sole authoritative source. If a piece of information is not there, it is not part of this project's spec contract.

Spec evolves. Prior reviews can become stale.

A passed review is valid only against the spec version it was performed on. When the spec submodule advances, every prior conclusion about that area must be re-checked. See Phase 1.0 below for the CHANGELOG sweep, and .claude/rules/workflow.md § Spec submodule update for the cross-cutting workflow that runs whenever the submodule pin moves.

Spec wins over reference implementation.

The erlang reference under references/opentelemetry-erlang/ is informative, not authoritative. A docstring reading "matching opentelemetry-erlang" is not self-justifying — if erlang diverges from spec (whether by workaround, oversight, or pending TODO), our code should match spec, not erlang. This project was scaffolded from the erlang reference, and erlang remains the primary cross-check for Elixir-on-BEAM idioms, but spec compliance takes precedence over reference-implementation fidelity when the two conflict.

The four phases

Work through the four phases in order. Later phases depend on the foundation laid by earlier ones — simplification (Phase 2) is unsafe until spec alignment (Phase 1) is confirmed.

---

Phase 1 — Spec alignment verification

Ensure every claim the code makes about the spec is true.

1.0 Spec evolution sweep (do this first)

Before checking the module's citations, look at what the spec itself has changed since the module was last reviewed.

1. Note the current submodule pin and tag:

   cd references/opentelemetry-specification
   git log --oneline -1
   git describe --tags HEAD
   

   Record both in your scratch notes — every finding in this review is relative to that pin.
2. Look for the module's last verification timestamp:
   • git log -1 --format=%H -- <module-path> and read the commit body for any "verified against spec vX.Y.Z" notes
   • If the module's @moduledoc carries a ## Spec verification line, read it
3. Diff the CHANGELOG between the prior pin and the current one:

   cd references/opentelemetry-specification
   git log <prev-tag>..HEAD CHANGELOG.md
   

   Skim for the signal areas relevant to the module under review:
   • Keywords that change semantics: Stabilize, Extend, Add, Deprecate, Remove, MUST, Breaking
   • Section names matching the module (e.g. "Logs", "Common", "Traces" for a trace module)

If the CHANGELOG shows a relevant change, assume prior conclusions about that area are stale until verified against the new spec text in 1.1–1.5.

Real example (PR #304/#305, LogRecord attributes):

• Prior reviews concluded attributes was narrow (primitive() | [primitive()]).
• v1.50.0 CHANGELOG: "Extending the set of standard attribute value types is no longer a breaking change." (#4614) — opens the door.
• v1.52.0: "Extend the set of attribute value types to support more complex data structures." (#4651) — Development.
• v1.53.0: "Stabilize complex AnyValue attribute value types and related attribute limits." (#4794) — stable.
• Conclusion: any prior review of LogRecord attributes performed on ≤v1.49 needed re-verification against the current pin (v1.55.0+).

If Phase 1.0 finds nothing relevant, you can still proceed — but record that you checked. "I looked at the CHANGELOG between v1.X and v1.Y and found no changes affecting " is a valid finding and worth keeping in the review output.

1.1 Fake citation detection

For each spec citation in docstrings, comments, or commit messages, open the cited spec file under references/ and read the actual section. Flag any of these patterns:

• Citation points to a section that doesn't define the claimed behaviour
• Citation paraphrases spec language but inverts meaning (SHOULD → MUST, minimum → maximum, propagate → reject, characters → bytes)
• Citation is a direct quote that doesn't appear in the spec

Real examples from this project:

Claim	Reality
"parser MUST discard whole"	No such sentence exists in W3C Trace Context
"the header exceeds 512 bytes (W3C §3.3.1.5 size cap)"	§3.3.1.5 says "SHOULD propagate at least 512 characters" — a minimum propagation floor, not a maximum byte cap. Three inversions in one citation (SHOULD→MUST, minimum→maximum, characters→bytes)
"matching opentelemetry-erlang ... individual malformed entries ... are dropped"	Erlang actually whole-rejects on any invalid entry

1.2 Hidden MUST/SHOULD gaps

Open the relevant spec section and walk it linearly. For every MUST and SHOULD clause, ask: does the implementation enforce this? Don't assume; grep for the behaviour.

Real finds:

• W3C §3.5 L379 "Adding a key/value pair MUST NOT result in the same key being present multiple times" — add/3 wasn't checking duplicates
• W3C §3.3.1.1 L273 "right-most list-member should be removed" — add/3 was returning unchanged at 32, not dropping and replacing
• update/3 fallthrough to add-semantics wasn't respecting the 32-cap

1.3 Bogus implementation

Every conditional branch and every validation should trace back to a spec clause OR a documented reference implementation behaviour. If a branch has no basis in either, it's bogus — likely added from a misreading that later review never revisited.

Real example: decode/1 had if byte_size(header) > 512, do: %__MODULE__{}. No W3C clause supports this. opentelemetry-erlang has no such check either. Pure invention.

Checklist for each branch:

• Which spec clause does this implement?
• Does the reference implementation do the same?
• If neither, why is this here?

1.4 Reference implementation cross-check

When cross-checking against opentelemetry-erlang, ask two questions — not one:

1. Does our code match what erlang actually does?
2. Does what erlang actually does match the spec?

A "no" to #1 means our docstring claim ("matching X") is wrong or has drifted — fix the claim or fix the code. A "no" to #2 means erlang itself has a spec gap, in which case our code should follow spec, not inherit erlang's behaviour. Surface the #2 case as a decision point in Phase 3 with trade-offs explicit, and document the intentional divergence per §3.3.

In this project, the reference is references/opentelemetry-erlang/. For Elixir modules implementing OTel primitives, the corresponding Erlang file is almost always the one to cross-check. For W3C wire formats, also cross-check against references/w3c-trace-context/ (pinned to level-2 branch) or references/w3c-baggage/.

Real examples of erlang diverging from spec:

• Baggage propagator percent-encoding (PR #182) — erlang uses form_urlencode with a TODO: call uri_string:percent_encode comment (otel_propagator_baggage.erl L146-L147). W3C spec HTTP_HEADER_FORMAT.md §value L64-L68 requires RFC 3986 percent-encoding; §Definition L32 explicitly lists + (0x2B) as a valid raw baggage-octet character, so the spec treats + as literal plus. Erlang's encoder emits + for space while its decoder treats + as literal, producing an asymmetric round-trip for space-containing values. We switched to strict RFC 3986 via URI.encode/2 &URI.char_unreserved?/1 and URI.decode/1 rather than mirror erlang's pending workaround.

• Composite propagator per-propagator try/catch (PR #181) — erlang's composite wraps each inner propagator call in try ... catch C:E:S -> log + skip (otel_propagator_text_map_composite.erl L73-L95). Spec api-propagators.md L100-L102 already mandates that individual propagators MUST NOT throw on parse failure, so the composite's catch is defensive programming against non-compliant propagators. We rely on the individual-propagator contract and omit the wrapper — consistent with .claude/rules/code-conventions.md §Happy path only.

1.5 Type-level spec drift

Beyond branches and citations (1.1-1.3), @type / @typedoc definitions themselves drift from spec in ways the other sweeps don't catch — there's no fake citation, no missing MUST, no bogus branch, and (often) no erlang to cross-check. The shape of the type alone leaks extra states the spec never defined.

Walk through every @type and @typedoc in the module and hold it against the spec field-by-field. Three patterns to watch for:

Pattern A — Overly permissive field types

Spec describes optional fields as "may be missing" — not "may be null". Elixir's optional(:key) => type already captures "missing or present"; adding | nil creates a second "missing" spelling the spec doesn't define.

Real example (PR #188): Otel.API.Logs.Logger.log_record had six fields typed integer() | nil, String.t() | nil, etc. Spec logs/data-model.md L185-L187 uses "missing", never "null". severity_number was especially leaky — spec L271 designates 0 as the "unspecified" sentinel, so 0..24 | nil created two ways to say unspecified, contradicting the single-sentinel convention. Removed | nil from the six affected fields.

When spec explicitly permits null (e.g. Body is AnyValue and common.md L49-L50 allows language- idiomatic null), the permissive type is correct — verify each field independently.

Pattern B — Value ranges wider than spec

Spec defines a bounded range (e.g. SeverityNumber 0..24, trace_flags 8-bit 0..255) but the type uses integer() or widens it. Narrowing the type to the spec range lets Dialyzer catch out-of-range literals at the call site rather than hoping they fail at runtime.

Pattern C — Optional / required mismatch

A spec-required field typed as optional(:key) => ..., or a spec-optional field typed without optional(). Compare the spec's "MUST provide" / "optional" / "MAY" wording against the typespec's optional() wrapper — the two should match.

Pattern D — Spec evolved while code stood still

The code's @type matches an earlier version of the spec exactly, but the spec submodule has since moved forward and the type has changed. The code is then "correctly drifted" — it correctly implements an outdated rule.

This pattern is invisible to a Phase-1.1–1.3 sweep that only compares current code against current spec text: the code looks well-aligned with whatever version of the spec it was written against. Detection requires Phase 1.0 (CHANGELOG sweep) plus knowing what spec version the module was last verified against.

Real example (PR #304/#305 + cleanup PR): Otel.API.Logs.LogRecord.attributes was typed %{String.t() => primitive() | [primitive()]}. Multiple prior reviews verified this type as spec-aligned, and they were right at the time (spec ≤v1.49 narrowed LogRecord attribute values to primitive or homogeneous primitive arrays). Spec evolution:

• v1.50.0 (#4614): allow extending standard attribute value types without it being a breaking change
• v1.52.0 (#4651): extend value types to support more complex data structures (Development)
• v1.53.0 (#4794): stabilize complex AnyValue attribute value types and related attribute limits

After v1.53.0 the code's narrow type became Pattern-D drift: it correctly implemented a no-longer-current rule. The type needed widening to primitive_any() to match the new spec text, and Otel.LoggerHandler.to_attribute_value/1 needed to be merged into the existing to_primitive_any/1 recursive walker.

Detection signal:

• git log -1 --format=%H -- <module> shows the module's last touch
• cd references/<spec> && git log <date>..HEAD CHANGELOG.md shows spec changes since
• If keywords in the module's domain appear in the diff (for Logs: "Logs", "LogRecord", "attribute", "AnyValue"), re-verify the relevant @types

Pattern D is the most insidious of the four — it presents as "the code matches the spec" if you only look at the current text. Always start with Phase 1.0.

Verification steps

1. For every @type / @typedoc in the module, open the spec section describing that concept and compare field by field.
2. For each | nil / | null variant in a field type, check whether the spec uses "null" / "empty value" / "language-idiomatic null" language. If it only uses "missing" / "optional", the nil is drift.
3. For each integer / range type, verify the bounds match the spec's documented value domain.
4. For each optional(:key), verify the spec actually allows the field to be absent — a required field behind optional(...) invites callers to omit it.

This sweep is type-definition-only — it does not require reading function bodies, running tests, or cross-checking erlang (which may itself be wrongly permissive). Do it alongside 1.1 when reading the module docs.

---

Phase 2 — Simplification

Once alignment is confirmed, look for code doing more than its name promises.

2.1 Single Responsibility

Compare each function's name to what it actually does. decode/1 is supposed to decode — parsing only. If it's also validating, enforcing size caps, and running format checks, those belong elsewhere.

Principle: validation is a mutation-time concern. The API contract "TraceState MUST at all times be valid" (OTel api.md L293) applies to state produced by mutation operations, not to intermediate parsing output. Size caps belong to the constant's consumer (e.g. @max_members used inside add/3).

Real result: decode/1 shrank from 20 lines with if/else branches and helper functions (build_from_header, parse_pair) to a single Enum.reduce pipeline (PRs #158 + #159). Validation moved to add/3 and update/3 where it belongs.

2.2 Happy-path-only

See .claude/rules/code-conventions.md. Remove:

• case _ -> [] fallbacks
• when is_binary(x) guards when @spec already declares the type
• {:ok, _} | :error return tuples
• nil-coalescing on malformed external data
• silent filtering of invalid entries

Use direct pattern match ([k, v] = split(...)) and trust the caller. Malformed input raises MatchError; the finalization phase (see .claude/docs/architecture/error-handling.md) will decide how to surface these.

Not error handling (keep these):

• Spec-mandated fallbacks (e.g. Noop dispatcher when no SDK registered)
• Public-API type gates at entry points
• Exception-recording contracts (Trace.with_span/5's catch)
• Lifecycle flags (:shut_down, supervisor trees)

2.3 YAGNI on API surface

For each public function, grep for lib callsites (exclude tests):

rg 'Module\.function_name' apps/*/lib/

Test-only APIs are suspects. Single-callsite APIs with a specific usage pattern (like size(ts) > 0 for empty-check) often signal that a purpose-built function would express intent better.

Real example: size/1 had one lib callsite — size(ts) > 0 to decide whether to inject a header. Replaced with empty?/1, which reads as intent rather than arithmetic (PR #159). The reverse direction of the if/else branch was flipped too (empty? → carrier vs size > 0 → setter) to avoid negation.

2.4 Visibility & encapsulation

Public types and functions need visibility that matches intent. Audit both directions — what should be hidden and what should be exposed.

Hide (encapsulation):

• If @type t is public, external code can depend on the internal field layout. When the internal representation is not part of the public contract, declare it @opaque. Dialyzer honours this boundary and warns on field access outside the module.
• The same logic applies to defstruct fields being pattern-matched externally — if they leak, provide accessor functions instead.
• Tier classification is part of encapsulation. A function that only SDKs call should be **SDK**, not **Application**; a function only other otel_api modules call should be @doc false + # Internal: comment (if it cannot be defp because of cross-module callers). Mismatched tier markers invite apps to depend on SDK-only surface.

Expose (surface widening):

• A defp predicate that external callers might reasonably want should probably be def. A format validator (valid_key?/1, valid_value?/1) is a canonical example — callers often need to pre-check input before calling mutators. Keeping such predicates private forces callers to duplicate the regex.
• def but unused in lib → belongs to YAGNI (2.3, opposite direction).

Relationship with 2.3:

• 2.3 removes API that has no consumer (shrink).
• 2.4 adds/adjusts visibility where consumers exist but can't reach the API, or where private structure leaks (shape).
• Together they form an API-surface audit: the right shape with the right reach.

Real examples from this project:

• @type t :: %__MODULE__{...} → @opaque t (PR #152 make TraceState opaque). Hides the members: field from external pattern matching; callers must use the public API.
• defp valid_key?/defp valid_value? → def valid_key?/def valid_value? (PR #150 tracestate regex and validator surface). Lets callers pre-validate before add/update.

Checklist:

• Does any public @type reveal internal fields that could change? → consider @opaque.
• Does any defp predicate check a module-owned invariant that callers might also want to check? → consider promoting to def.
• Is any public function unused in lib? → consider removing (2.3).
• Does every public def have a tier marker (**Application**, **SDK**, or @doc false for Tier 3) matching its callsite evidence? See .claude/rules/documentation.md §Tier classification.
• Are Tier 3 helpers marked @doc false with a # Internal: comment above, rather than a public-looking @doc?

---

Phase 3 — Decision methodology

How to handle conflicts between user feedback and project constraints.

3.1 Trade-off presentation

When user feedback conflicts with another project rule — credo checks, code-conventions.md, spec MUSTs, existing architectural decisions — never silently pick. Present 2–3 concrete options with trade-offs and ask.

Real example (from PR #158 development): user asked to inline the parse_pair helper into the Enum.flat_map lambda. Credo's Refactor.Nesting check (max 2) rejected the resulting depth-3 lambda/if structure.

Options presented:

• (A) Use || fallback to avoid inner if — technically passes credo, but introduces cryptic boolean-list-coercion syntax
• (B) Re-introduce a helper with meaningful role (32-member guard clause) — loses pure inline benefit but gains declarative shape
• (C) Raise credo max_nesting to 3 project-wide — weakens the gate for every other file

User picks. Each option's downside is stated explicitly so the decision is informed.

3.2 Spec layer disambiguation

Spec-aligned modules often sit at the intersection of multiple specs. Label every citation with its layer. When two specs appear to conflict, identify the scope of each clause.

Real example: W3C Trace Context §3.3.1.6 grants parsers discretion on partially-parsed pairs ("to the best of its ability"). OTel api.md L293 says "TraceState MUST at all times be valid". Apparent conflict — does the parser have to validate or not?

Resolution: identify the MUST's scope. OTel L293 is about the TraceState object's invariants, which are established by mutation operations (add/update) — not by parsing. Parsing is W3C's territory and §3.3.1.6 discretion applies there. The two specs don't actually conflict once scope is clear.

3.3 Intentional divergence from reference

When our implementation diverges from opentelemetry-erlang to follow spec more strictly, to match an Elixir idiom that serves users better, or to pick a different BEAM-level trade-off, document the divergence explicitly. Silent divergence invites a later reviewer to "fix" the code back to matching erlang under the mistaken assumption that erlang is authoritative.

Placement:

• A single-module trade-off goes in the module's @moduledoc as a short ## Design notes subsection.
• An architectural choice affecting multiple modules or a cross-cutting pattern goes in .claude/docs/architecture/ as its own doc, linked from the relevant @moduledoc. The bar is high — only add a new file when the rationale spans enough modules that repeating it in each moduledoc would be painful and the rationale is not recoverable from the code.

Documentation format (both placements):

• What erlang does — with a file/line reference
• What we do — with a file/line reference (when code exists)
• Why — cite the spec clause, idiom, or use case that justifies our choice. If the choice is deliberately stricter than erlang, name the spec clause; if it's a BEAM idiom, name the alternative and why ours serves users better.

Real example — architecture doc: .claude/docs/architecture/with-span-lifecycle-ownership.md captures why Trace.with_span/5 delegates to a Tracer callback (matching erlang's otel_tracer.erl + otel_tracer_default.erl split) and names the architectural invariant — "whichever layer attaches the context must also detach and handle errors in between" — that makes the choice load-bearing.

Real example — module ## Design notes: apps/otel_api/lib/otel/api/propagator/text_map/baggage.ex @moduledoc enumerates four divergences from erlang — strict RFC 3986 encoding (spec-aligned, diverges from erlang's pending workaround), metadata as opaque string (Otel.API.Baggage API design), extract merge vs replace (Elixir idiom), Limits not enforced at propagator layer (spec says MAY). Each is short-form; the module's behaviour speaks for itself.

---

Phase 4 — Test minimalism

4.1 Structural regression protection

When a boundary is expressed structurally in the code, a dedicated regression test for "this boundary doesn't leak" is redundant.

Real example: @max_members is used only inside add/3. For the 32-cap to re-appear in decode/1, someone would have to explicitly import the constant into a function that currently doesn't reference it. That's a structural barrier code review catches naturally; a dedicated "decode preserves 33 entries" test was adding noise without adding protection.

The test was initially kept as "regression defence" but removed in PR #159 once the structural argument was made explicit.

4.2 Behaviour over counts

Prefer behaviour-centric assertions over count assertions. They catch more regression modes.

Count assertion (narrow)	Behaviour assertion (broader)
size(ts) == 32	get(ts, "dropped_key") == "" + get(ts, "added_key") == value + get(ts, "second_oldest") == "val2"
size(ts) == 0	empty?(ts)
size(ts) == 1 after dedup	encode(ts) == "key=second"

Example: if add/3 had a bug dropping the wrong entry, size == 32 still passes (still 32 entries) but get(ts, "second_oldest") == "val2" fails. The behaviour assertion catches it; the count doesn't.

---

Workflow integration

This skill operates within .claude/rules/workflow.md. After review identifies changes, the standard flow applies:

1. Classify — fix (closing spec gap, removing bogus logic), refactor (SRP, YAGNI), or docs (docstring-only correction)

2. Branch — <type>/<short-description> from main

3. Implement — apply changes

4. Code conventions — verify against .claude/rules/code-conventions.md (full module names, @spec with parameter names, happy-path only)

5. Quality gates — run in order:

   mix clean
   mix format --check-formatted
   mix compile --warnings-as-errors
   mix test --warnings-as-errors --cover
   mix credo --strict
   mix dialyzer
   

6. Update docs — if the change affects an architectural decision, update or add a doc under .claude/docs/architecture/. Otherwise update the relevant module's @moduledoc to reflect the new behaviour.

7. Pin the spec version in commit + moduledoc — every spec-review PR MUST record which spec submodule pin the verification was performed against. Without this, the next reviewer cannot tell whether your conclusions are still valid after a future submodule update (Pattern D in 1.5).

   Commit body line:

   Verified against opentelemetry-specification <tag> (commit <sha>).
   

   For modules that gain a ## Spec verification moduledoc section (recommended for any non-trivial spec-aligned module), include the same pin:

   ## Spec verification
   
   Verified against `opentelemetry-specification` <tag>
   (commit <sha>) on <YYYY-MM-DD>. Re-verify after any submodule
   advance — see `.claude/rules/workflow.md` § Spec submodule update.
   

   Use the actual values from git -C references/opentelemetry-specification describe --tags HEAD and git -C references/opentelemetry-specification log --format=%H -1.

8. PR — single commit preferred; PR title = commit subject, PR body = commit body verbatim (per git-conventions.md and memory/feedback_pr_body_verbatim.md)

Communication style

• Korean first, English for technical terms as needed
• Concise; feedback typically arrives as 질문) or 피드백)
• State findings factually with spec line references (e.g. W3C §3.5 L379); let the user decide direction
• When options conflict (Phase 3.1), present clearly with trade-offs and wait for the pick — don't assume

Related files

• .claude/rules/workflow.md — overall task workflow
• .claude/rules/code-conventions.md — happy-path, @spec, no-alias
• .claude/rules/documentation.md — @doc role markers
• .claude/rules/git-conventions.md — commit/PR format
• .claude/docs/architecture/ — BEAM-specific design decisions not recoverable from code alone (read before proposing changes that might conflict)
• .claude/docs/architecture/error-handling.md — policy for malformed input
• references/ — spec submodules (verify here, always)