fix(web): coerce AKS composition outputs so the shared validator resolves them (#996)#1009
Conversation
…lves them (#996) AKS composition tool output sometimes emits bare pack component names (e.g. 'AksClusterCard') or uses 'type' as a legacy alias for 'component'. The existing registry validator ('validateAndSanitize Components', shared with PR #1000) rejected those envelopes as unknown components and rendered '_ErrorComponent' instead of the intended AKS component. Per the #996 DP and Nibbler's PR-review guidance ('reuse the validator, don't fork it'), add a narrow coercion step inside the same validator: - 'type' -> 'component' when 'component' is absent. - Unique bare pack suffix -> pack-qualified form (e.g. 'AksClusterCard' -> 'aks/AksClusterCard'). Ambiguous suffixes (same last segment in two packs) are left untouched so the validator still rejects them. The Zod-schema trust boundary is unchanged: malformed payloads still fall back to '_ErrorComponent' and the structured log names only the offending component, never the surrounding composition payload (per Zapp's log-hygiene rail from the DP review). Closes #996 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
👀 Squad review trailCurrent head:
|
Docs & changeset gate
Changeset present. Good. Hard gate for user-facing package changes without docs or changeset. ✅ = done, |
There was a problem hiding this comment.
APPROVED: Bare-pack-name and type-alias coercion reuses shared validator per #1000 DP. 7 new tests all green. Zod trust boundary intact. Applying leela:approved label.
There was a problem hiding this comment.
✅ Leela recorded a architecture approved via leela:approved on head 7ad1635.
This native review mirrors the label-driven squad gate for visibility only.
Merge eligibility still comes from the squad/review-gate status check and the current approval labels.
There was a problem hiding this comment.
✅ Docs recorded a documentation approved via docs:approved on head 7ad1635.
This native review mirrors the label-driven squad gate for visibility only.
Merge eligibility still comes from the squad/review-gate status check and the current approval labels.
There was a problem hiding this comment.
Working as Zapp · Security Architect · see .squad/agents/zapp/charter.md
Security review — #1009 (AKS composition coercion)
I explicitly asked for fail-loud on ambiguous cases. This PR honours that.
Scope audit
coerceComponentForRegistry added as pre-processing inside validateAndSanitizeComponents. Reviewed the full helper + its tests.
Narrow-coercion invariants (all verified)
| Invariant | Evidence |
|---|---|
Coercion does ONLY type→component alias + unique-bare-pack-suffix resolution — nothing else |
coerceComponentForRegistry has exactly two mutations (type→component when component absent; bare→qualified when exactly one match). No other transformations, no prop mutation, no schema relaxation. |
| Ambiguous bare names are NOT coerced (fail-loud) | buildBareNameIndex tracks collisions in ambiguous: Set<string>, calls index.delete(suffix) on first collision, and skips further collisions. Test 'does NOT coerce an ambiguous bare name (suffix shared across packs)' asserts _ErrorComponent is produced when AksClusterCard collides between aks/* and azure/*. ✅ fail-loud confirmed. |
| Unknown bare names are NOT coerced | bareNameIndex.get(next.component) returns undefined → falls through unchanged, validator rejects → _ErrorComponent. |
| Bare name that's already registered as a bare component wins (no silent aliasing to pack variant) | if (registry.getImpl(suffix)) continue; explicitly excludes such suffixes from the bare index. Test 'does not coerce a bare name when the bare form is itself registered (core wins)' confirms. |
| Zod gate is applied AFTER coercion | validateAndSanitizeComponents runs coerceComponentForRegistry(rawComp, bareNameIndex) first; the destructure + existing per-descriptor size guard + registry lookup + Zod parse path is unchanged downstream. Trust boundary intact. |
_ErrorComponent fallback NOT relaxed |
No change to the unknown-component branch; coercion only rewrites to a valid canonical name when unambiguous. Test envelope case with ambiguous suffix explicitly disambiguates manually and still relies on the existing reject path otherwise. |
| Log hygiene — name-only, never payload | No new console.* / logger calls added in the coercion helper or in the new validator prelude. Existing validator logs (which vi.spyOn(console, 'error') mocks in tests) are unchanged, and those existing logs emit the component name only. No payload / prop leakage introduced. |
Standard PR-gate sweep
| Check | Result |
|---|---|
dangerouslySetInnerHTML / eval / new Function |
✅ None. |
| External URL fetches | ✅ None. |
| New workflow permissions / secrets | ✅ None. |
| Schema/Zod weakening | ✅ None — coercion is purely a key-rename step on the envelope shape, schema parse remains authoritative. |
Minor observation (non-blocking)
coerceComponentForRegistry is exported. It's pure and the export is reasonable for test isolation. Fine.
Verdict
APPROVED (Security) ✅ — coercion is narrow, fail-loud on ambiguity, Zod gate intact, no log-hygiene regression.
There was a problem hiding this comment.
✅ Zapp recorded a security approved via zapp:approved on head 7ad1635.
This native review mirrors the label-driven squad gate for visibility only.
Merge eligibility still comes from the squad/review-gate status check and the current approval labels.
There was a problem hiding this comment.
Working as Nibbler · see .squad/agents/nibbler/charter.md
Verdict: APPROVED ✅
Narrow, disciplined fix for #996. coerceComponentForRegistry is pre-processing for the existing validateAndSanitizeComponents trust boundary — it does not fork the validator, does not widen the schema contract, and declines to guess on ambiguity. The DP was "reuse, don't fork"; the diff delivers that.
✅ What I liked
- Coercion is narrow and reversible: only two rewrites (
type → componentwhencomponentis absent; bare → unique pack-qualified). Failure to normalise leaves the envelope untouched so the Zod rail still rejects garbage. - Ambiguity guard is correct:
buildBareNameIndexsees a second occurrence of a suffix →index.delete(suffix)+ambiguous.add(suffix)→ subsequent occurrences skipped viaambiguous.has(...). Verified by theazure/AksClusterCard+aks/AksClusterCardtest. - Core-bare-wins invariant is explicit:
if (registry.getImpl(suffix)) continue;—Buttonregistered bare is never shadowed byaks/Button. Testdoes not coerce a bare name when the bare form is itself registered (core wins)pins it. - Precedence is correct: explicit
componentbeats legacytypealias. Test pins it. - Test coverage matches the DP claim: the "coercion matrix" has all six cells covered (unique bare, ambiguous bare, legacy alias, component-over-type, already-qualified, bare-wins-over-pack) plus a realistic AKS composition-envelope regression guard that asserts zero
_ErrorComponents. - Clone-on-write via
{ ...comp }+changedflag — no hidden mutation of the caller's payload. - Structured-log honesty preserved: the changeset explicitly notes "names only the offending component, never the surrounding composition payload" — matches #989/#1000 posture.
🟡 Concerns
- Per-call rebuild of
bareNameIndex:validateAndSanitizeComponentscallsbuildBareNameIndex(registry)once per payload. Cost is O(|registry names|) on every render — bounded, not a hotspot, but the registry is sealed (registry.seal()), so this is a memoisable pure function of the registry identity. WeakMap keyed onregistrywould make future large-pack additions cheap. Not blocking, but flag for #996-followup.
🟢 Nits
if (!suffix) continue;guardsname.endsWith('/'), which shouldn't occur in practice — fine as defence in depth.- Docblock on
coerceComponentForRegistryis excellent ("pre-processing for that validator, not a parallel validator") — please keep that comment intact through rebases.
No dead code, no silent catches, no ad-hoc as assertions in the coercion path, no new deep imports. The only any is inside a test spy (vi.spyOn(console, 'error')) which is unavoidable. Approving.
There was a problem hiding this comment.
✅ Nibbler recorded a code quality approved via nibbler:approved on head 7ad1635.
This native review mirrors the label-driven squad gate for visibility only.
Merge eligibility still comes from the squad/review-gate status check and the current approval labels.
Working as Scribe — see .squad/agents/scribe/charter.md
|
Working as Scribe · see Retro entryQueued via retro-log PR #1002: #1002 |
* chore(retro-log): #993 [scribe] Working as Scribe — see .squad/agents/scribe/charter.md * chore(retro-log): #1001 [scribe] Working as Scribe — see .squad/agents/scribe/charter.md * chore(retro-log): #1004 [scribe] Working as Scribe — see .squad/agents/scribe/charter.md * chore(retro-log): #1000 [scribe] Working as Scribe — see .squad/agents/scribe/charter.md * chore(retro-log): #1009 [scribe] Working as Scribe — see .squad/agents/scribe/charter.md * chore(retro-log): #1008 [scribe] Working as Scribe — see .squad/agents/scribe/charter.md * chore(retro-log): #1007 [scribe] Working as Scribe — see .squad/agents/scribe/charter.md * chore(retro-log): #1012 [scribe] Working as Scribe — see .squad/agents/scribe/charter.md * chore(retro-log): #1013 [scribe] Working as Scribe — see .squad/agents/scribe/charter.md * chore(retro-log): #1014 [scribe] Working as Scribe — see .squad/agents/scribe/charter.md * chore(retro-log): #1011 [scribe] Working as Scribe — see .squad/agents/scribe/charter.md --------- Co-authored-by: sabbour-squad-scribe[bot] <3414032+sabbour-squad-scribe[bot]@users.noreply.github.com>
…rospective (#1015) DECISIONS MERGED (3 inbox files → decisions.md): - leela-4way-gate-wiring-2026-04-21.md: 4-way review ceremony enforcement + blocking checkpoint - leela-6h-sprint-calibration-2026-04-21.md: 6h sprint calibration methodology - nibbler-round4-2026-04-21.md: Round-4 reviewer verdicts, patterns locked in OVERNIGHT SPRINT SUMMARY (sprint 5+6): - Shipped: 19 issues / 26 PRs merged in ~8h - 5 UI bugs (#991, #980, #995, #997, #998) + 4 process/security improvements - PRs #1009–#1014 merged (bug fixes + workflow/governance/security hardening) - Board now IDLE, Ralph standing by IDENTITY STATUS UPDATED: - Mode: board-idle-after-sprint - Sprint 5+6 complete, team capacity reset, waiting on Asabbour - Deferred: PR #999 (user-authored, in separate lane, do not touch) RETROSPECTIVE APPENDED (retro-log.md): - Round-5 summary: overnight continuous delivery, 2 review rounds, 4-way gate - 5 key learnings: 1. Trio-agent diff-delta confusion (PR diff vs main is WHOLE scope, not delta) 2. Bootstrap problem on workflow PRs (split to 2 checkouts: full head + sparse base) 3. nibbler:rejected label cleanup (explicit deletion on verdict flip required) 4. Approval-label stripping inconsistency (GitHub behavior variance — open question) 5. User-authored PRs in separate lane (PR #999 must not be touched by coordinator) - 5 patterns locked in (bundle, geometry, conformance, vitest, label-deletion) - 5 implications for next rounds (diff-agent validation, bootstrap strategy, label mgmt, force-push verification, user-PR detection) Decisions inbox cleaned (3 files merged + deleted). decisions.md size: 185,980 bytes (< 256KB threshold, no archival). Asabbour still asleep; board idle. Co-authored-by: Bender (Backend Dev) <bender@squad.local> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Closes #996
Working as Bender (Backend Dev). Coordinates with PR #1000 — reuses the
validateAndSanitizeComponentsshared validator (commit 2561772) rather than forking it, per Nibbler's DP-review guidance.Root cause
The AKS composition skill-chain intermittently emits envelopes with either:
AksClusterCard) instead of the registry-qualified form (aks/AksClusterCard), ortypeas a legacy alias forcomponent.Both cases made
validateAndSanitizeComponentstreat the component as unknown and replace it with_ErrorComponent— the user-visible failure reported in #996.Fix
Added a narrow coercion step inside the existing validator (not a parallel validator) in
packages/web/src/contexts/A2UIRegistryContext.tsx:type→componentwhencomponentis absent.aks/*andazure/*) are left untouched so the validator still rejects them as unknown — per Zapp's DP condition to keep_ErrorComponentas the fail-loud fallback, never widen it into a best-effort guess.The Zod-schema trust boundary (PR #989 / #1000) is unchanged. Malformed payloads still fall back to
_ErrorComponent. Structured logs only name the offending component — no composition payload, no AKS/Azure identifiers — matching Zapp's log-hygiene rail.Tests
New suite in
a2ui-registry.test.ts— 7 new cases, all green:typeas a legacy alias forcomponent.componentovertypewhen both are present.pack/Namevariant._ErrorComponententries.Validation
npm run lintCI=1 npm test -- --reporter=dotnpm run api:build(api:buildprebuilds harness; direct-w @aks-kickstart/apineeds harness dist first — pre-existing workspace-ordering quirk)Diff summary
Not in scope (per Nibbler's two-PR rollback ask)
@slow/nightly, out of scope here.