From 7c539cb86a903f56eec51201837f838207c05574 Mon Sep 17 00:00:00 2001
From: Tom Boucher <trekkie@nomorestars.com>
Date: Sun, 24 May 2026 16:32:01 -0400
Subject: [PATCH] docs(227): ADR on input-validation checking semantic shape,
 not just type (#228)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

* docs(227): create ADR for input-validation-shape-not-just-type

Captures the architectural standard that defensive normalization at trust
boundaries must validate both type and semantic shape, with silent
coercion on failure. Concrete cases: parentTraceId UUID v4 fix in
PR #225 and release-version validation in ADR 218.

Closes #227

* docs(227): cross-reference new ADR from ADR 218

Appends a "See also" section at the end of ADR 218 pointing forward to
ADR 227, which generalises the type+semantic-shape validation principle
documented in ADR 218's narrower release-workflow context.

* docs(227): add CONTRIBUTING pointer to new ADR

Adds a "Code Review Lessons → Input validation" section after the
Reviewer Standards block, linking to ADR 227 as the citable reference
for the type+semantic-shape validation standard.
---
 CONTRIBUTING.md                               |   6 +
 docs/adr/218-release-version-validation.md    |   4 +
 ...27-input-validation-shape-not-just-type.md | 110 ++++++++++++++++++
 3 files changed, 120 insertions(+)
 create mode 100644 docs/adr/227-input-validation-shape-not-just-type.md

diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md
index e7f3efffd..883d1a7c2 100644
--- a/CONTRIBUTING.md
+++ b/CONTRIBUTING.md
@@ -758,6 +758,12 @@ Reviewers do not rely solely on CI to verify correctness. Before approving a PR,
 
 **"Tests pass in CI" is not sufficient for merge.** The implementation must correctly solve the problem described in the linked issue.
 
+## Code Review Lessons
+
+### Input validation: check shape, not just type
+
+Defensive normalization at trust boundaries must validate both the value's type and its semantic shape. A `typeof === 'string'` check is necessary but insufficient when the field's contract requires a specific format (UUID v4, semver, file path, etc.). See [ADR 227](docs/adr/227-input-validation-shape-not-just-type.md) for the architectural standard and concrete cases.
+
 ## Code Style
 
 - **CommonJS** (`.cjs`) — the project uses `require()`, not ESM `import`
diff --git a/docs/adr/218-release-version-validation.md b/docs/adr/218-release-version-validation.md
index 2add6a9c3..28cf322d2 100644
--- a/docs/adr/218-release-version-validation.md
+++ b/docs/adr/218-release-version-validation.md
@@ -87,3 +87,7 @@ git push origin --delete release/1.03.0
 - **Leading-zero inputs fail in under 5 seconds** at the `validate-version` job before any branch, install, or publish operation runs.
 - **Duplicate-version inputs fail in under 5 seconds** at `validate-version` rather than after a full install-and-test cycle.
 - **The late dry-run check in `finalize` is unchanged.** It remains a belt-and-suspenders guard; the new precheck does not remove it.
+
+## See also
+
+- ADR 227 (`docs/adr/227-input-validation-shape-not-just-type.md`) generalises the principle this ADR documents in the narrower release-validation context: input validation at trust boundaries must check both type and semantic shape, with silent coercion on failure.
diff --git a/docs/adr/227-input-validation-shape-not-just-type.md b/docs/adr/227-input-validation-shape-not-just-type.md
new file mode 100644
index 000000000..2082e1b27
--- /dev/null
+++ b/docs/adr/227-input-validation-shape-not-just-type.md
@@ -0,0 +1,110 @@
+# ADR 227: Input validation must check semantic shape, not just type
+
+- **Status:** Accepted (2026-05-24)
+- **Date:** 2026-05-24
+
+## Context
+
+Defensive normalization at trust boundaries typically starts with a type check:
+
+```js
+if (typeof value !== 'string') return undefined;
+```
+
+This stops non-string values but accepts any string — including the empty string, garbage payloads, and values that are structurally correct (a string) but contractually invalid (not a UUID v4, not a semver, not a file path). The remaining attack surface is the gap between "is a string" and "satisfies the field's contract."
+
+### The PR #225 trigger
+
+PR #225 (`refactor/178-trace-id-propagation`, P1.4 of ADR-0174 SDK retirement) introduced `parentTraceId` on `DispatchEvent`. The initial implementation normalized the field with a type-only guard:
+
+```js
+parentTraceId: typeof raw.parentTraceId === 'string' ? raw.parentTraceId : undefined,
+```
+
+Codex adversarial-review (commit range `fb94ba8d`–`338d0951`) flagged that this propagated:
+
+- empty strings (`""`) — a correlation key that matches nothing
+- garbage strings (`"not-a-uuid"`, `";"`, `"<script>..."`) — log bloat, downstream parser confusion
+- oversized strings — potential high-cardinality index explosion in tracing back-ends
+
+The field's contract is UUID v4. The fix was a strict regex with silent coercion:
+
+```js
+const UUID_V4 = /^[0-9a-f]{8}-[0-9a-f]{4}-4[0-9a-f]{3}-[89ab][0-9a-f]{3}-[0-9a-f]{12}$/i;
+parentTraceId: UUID_V4.test(raw.parentTraceId) ? raw.parentTraceId : undefined,
+```
+
+### The generalizing precedent
+
+ADR 218 (`docs/adr/218-release-version-validation.md`) documents the same two-layer pattern applied earlier to the release-workflow version input: a type-level format regex was tightened to reject leading-zero segments (semantic-shape enforcement), and a duplicate-version precheck was added at validation time. That ADR is narrowly scoped to the release workflow; this ADR captures the general principle so future contributors can cite it during code review without needing to derive it from the release-workflow incident.
+
+## Decision
+
+Defensive normalization at any trust boundary MUST validate two layers:
+
+1. **Type** — `typeof`, `instanceof`, `Array.isArray`, schema-type check
+2. **Semantic shape** — regex, schema, range, or enum check that proves the value satisfies the field's contract
+
+On failure of **either** layer, the value MUST be silently coerced to the contract's safe default (typically `undefined` or `null`). It MUST NOT be propagated. Throw only if the surrounding codebase treats throws as a normal-flow signal (it usually does not — dispatch pipelines must remain continuous).
+
+Both layers are required. A type check alone is necessary but not sufficient.
+
+## Consequences
+
+### Bug classes avoided
+
+- **Correlation poisoning** — a garbage `parentTraceId` propagated into a trace back-end creates phantom spans that never match a real root.
+- **High-cardinality log bloat** — unsanitized free-form strings as structured-log field values balloon index size in Elasticsearch, Datadog, etc.
+- **Downstream parser crashes** — a field typed as UUID v4 but containing `";"` or a 2 KB payload can crash a consumer that assumed a bounded, well-formed value.
+
+### Cost
+
+One additional regex or predicate per trust-boundary field. For most fields this is a one-liner compiled once at module load. The maintenance burden is low.
+
+### Tradeoff
+
+Silent coercion hides invalid inputs from callers — a buggy upstream component may send garbage and never learn it was rejected. Mitigate with an opt-in debug log (`process.env.GSD_DEBUG`) that surfaces the coercion without affecting production behavior.
+
+## Concrete cases
+
+### Case 1 — `parentTraceId` on `DispatchEvent` (PR #225)
+
+| | Detail |
+|---|---|
+| Field | `parentTraceId: string \| undefined` |
+| Contract | UUID v4 |
+| Initial impl | `typeof raw.parentTraceId === 'string'` — type check only |
+| Codex finding | Commit range `fb94ba8d`–`338d0951`; flagged correlation poisoning and log bloat |
+| Fix | `UUID_V4.test(raw.parentTraceId) ? raw.parentTraceId : undefined` |
+
+### Case 2 — release-workflow `version` input (ADR 218)
+
+| | Detail |
+|---|---|
+| Field | `version: string` (GitHub Actions workflow input) |
+| Contract | Semver `MAJOR.MINOR.0`, no leading zeros |
+| Initial impl | `^[0-9]+\.[0-9]+\.0$` — type regex only; `1.01.0` accepted |
+| Semantic fix | `^(0\|[1-9][0-9]*)\.(0\|[1-9][0-9]*)\.0$` — rejects leading zeros |
+| Duplicate precheck | Added npm `view` call at validation time to reject already-published versions early |
+
+See [ADR 218](218-release-version-validation.md) for the full incident account.
+
+## Alternatives considered
+
+### Type-only checks (status quo before this ADR)
+
+Rejected. The cases above prove that type-only checks leave the harder bug class open. A string is not a UUID; a string is not a semver. The type check is the floor, not the ceiling.
+
+### Schema validation library (Zod, ajv)
+
+Rejected for the current CJS phase. Adding a runtime dependency to the core library violates the project's no-external-dependencies policy for `gsd-tools.cjs`. Project-internal patterns prefer surgical regex/predicate functions. The `src/` TypeScript phase may revisit if Zod is adopted broadly — that is a separate decision.
+
+### Throwing on invalid input
+
+Rejected. Throwing breaks dispatch pipeline reliability. The dispatch pipeline must be continuous; a bad `parentTraceId` must not abort event dispatch. Silent coercion preserves continuity; an opt-in debug warn preserves visibility. Fields where an invalid value is genuinely fatal (not just malformed) may throw — that is a per-field decision, not the general rule.
+
+## Related
+
+- Issue: #227
+- PR introducing: this PR
+- Cross-references: ADR 218 (`docs/adr/218-release-version-validation.md`)