Review 5438

Review #5438 — unknown integration manifest keys

Versions: Review Loop 1.6.0 · Component Audit Rubric 1.13
Verdict: changes requested

PR

#5438 fix(cli): an unknown manifest key no longer discards the whole integration by josephfarina (bucket: internal)

HEAD REVIEWED

4b6f80c8b64fc72879bcb55f987bc479bcf5228b

VERSIONS

LOOP VERSION: 1.6.0 AUDIT RUBRIC: 1.13

LANE

LANE: full WHY: The CLI project model, a published package subpath, generated documentation, and two diagnostic surfaces require more than one decisive check.

PROBLEM

WHY 1: An integration manifest authored with a newer field is rejected wholesale by an older CLI. WHY 2: Every contribution the older CLI does understand disappears with the one unknown field. WHY 3: Builders cannot discover or use the package's components, templates, codemods, or docs and can mistake a compatibility mismatch for missing product capability.

USER-FACING PROBLEM: A builder upgrades an integration before the project CLI and loses every usable contribution from that package instead of only the newer field. PROBLEM SEVERITY: broken task — the integration's supported contributions become unavailable.

VERDICT: clear — the PR states and reproduces the compatibility failure at the correct system boundary

SOLUTION

The loader keeps fields it understands and records the names it does not. Shared validation reports those names as a warning while treating known fields with invalid values as errors. The package's supported contributions therefore stay available across CLI version skew.

SOLUTION (2 decisions · ~45 runtime statements of 246 changed lines)

1. Strip unknown top-level manifest fields while retaining validation for every known field.
2. Carry unknown field names through loading and report one non-blocking diagnostic from project and validation paths.

BURDEN: low — one bounded key scan at manifest load, one metadata field, and one shared warning; no state, effects, listeners, dependencies, or generated runtime registry. BURDEN MATCH: proportionate — it prevents loss of every supported contribution while preserving errors for malformed known fields.

VERDICT: clear — both decisions directly serve the stated compatibility failure

ARCHITECTURE

OWNER: the integration parser owns the accepted manifest shape; the integration loader owns raw-versus-parsed metadata; shared contribution validation owns diagnostics. TIER 1: CLI authoring/load boundary and project discovery; the existing issue model and skip-on-error policy are reused. TIER 2: none. SEAMS: .ts/.mjs/.js manifests; Project discovery for components/templates/codemods/docs; validate-integration; ordinary-command warning; public ./integration package subpath. BEHAVIOR UNIT: pure utility — key classification and parsing are deterministic and covered by focused tests.

seam	driven result
valid known fields + two unknown fields	all four contribution kinds remain available; one warning contains both names
known field with wrong type	remains an error and contributes nothing
validate-integration	reports unknown_manifest_key as warning and still validates known roots
ordinary warning helper	warning-only issues remain non-fatal
published ./integration subpath	exposes KNOWN_INTEGRATION_KEYS, parseIntegration, and unknownIntegrationKeys at runtime

The behavior lives at the right load boundary. The key census is nevertheless a second hand-maintained source beside the schema, and its two implementation names are exported from a published subpath.

VERDICT: BLOCKS — the compatibility guard can drift from its parser and leaks implementation helpers into the public runtime surface

IMPACT

The intended change lets builders on an older CLI keep every contribution that version understands and gives them a warning naming only the unsupported fields. Existing valid manifests keep the same parsed value; malformed known fields remain errors.

As written, a later known field can work while still being reported as unsupported if the separate key list is missed. JavaScript consumers can also import two new undocumented names from the published subpath, while its mapped TypeScript declaration does not expose them.

VERDICT: BLOCKS — the current fix works, but its next manifest extension can produce a false compatibility warning and accidental public API

API

Existing public call:

parseIntegration({components: './src', futureRoot: './future'});
// before: throws; after: {components: './src'}

	change	public?	class	doc'd?	verdict
~	parseIntegration(input, label?) accepts and strips unknown top-level fields	yes — ./authoring and ./integration	compatibility parsing for versioned manifests	schema doc updated; integration guide stale	finding
+	KNOWN_INTEGRATION_KEYS: string[]	yes at runtime — ./integration	no external caller; only implementation bookkeeping	missing from mapped types and docs	finding
+	unknownIntegrationKeys(input): string[]	yes at runtime — ./integration	no external caller; only the internal loader uses it	missing from mapped types and docs	finding

OSSIFICATION: the two new names serve no external class and were not presented as API. The package export map makes every named export of the mapped module runtime-reachable; publishing them would turn implementation bookkeeping into surface that cannot later be removed quietly.

VERDICT: BLOCKS — two unintended, untyped runtime exports add permanent surface to a bug fix that claims no API signature change

THEMING

No theme target, token, variable, StyleX, CSS, or rendered component path changes.

VERDICT: clear — no theming surface is reachable

BREAKING

BEHAVIOR: intentionally more permissive — unknown fields become warnings while malformed known fields remain errors; parent/head regression tests prove this delta. API: no existing caller breaks, but the published runtime subpath gains two unintended named exports. VISUAL: no JSX, DOM, CSS, geometry, color, or rendered output changes. THEME: no target, token, override, or generated theme artifact changes.

The public CLI integration guide and its generated README still say the manifest is validated by a strict schema. That is now the opposite of the shipped unknown-field contract.

VERDICT: clear — the compatibility change is non-breaking; its stale public documentation is owned by the API contract finding

PERFORMANCE & RESOURCES

EFFECTS: zero. RENDER: no React path; manifest loading adds one Object.keys pass and a five-entry membership check once per integration load. LISTENERS/OBSERVERS: none. LAYOUT: none. BUNDLE: no dependency or package-entry addition.

VERDICT: clear — bounded load-time work only, with no hot-path or resource lifetime

VISUAL EVIDENCE

VISUAL CHECK: not applicable WHY: all changed paths are Node CLI source, tests, schema prose, and a changeset; no rendered UI, CSS, DOM, or visual claim exists. Exact-head pr-a11y and pr-rtl skipped as expected.

VERDICT: clear — nothing renderable changed

REMEDY SEARCH

REMEDY SEARCH: not triggered — no proven visual defect

A11Y & I18N

No DOM, accessibility semantics, focus, keyboard, direction, locale formatting, or translated component string is reachable. The terminal diagnostic uses the existing {code, severity, message} issue contract and warnings retain exit code 0.

VERDICT: clear — no accessibility or localization regression found

JUDGEMENT

slot	verdict
PROBLEM	clear
SOLUTION	clear
ARCHITECTURE	BLOCKS — duplicated key authority plus public helper exports
IMPACT	BLOCKS — future false warning and accidental surface
API	BLOCKS — two unintended runtime exports
THEMING	clear
BREAKING	clear — intended permissive behavior; no existing caller breaks
PERFORMANCE	clear
VISUAL	clear
A11Y & I18N	clear

GOAL: met — 40 focused tests pass at the exact head; the same tests against parent production code fail exactly three new regression cases, proving unknown fields previously rejected/discarded the integration while known invalid fields remain errors.

DISPOSITION:

• public helper exports and a hand-maintained key census → blocks now; confirmed independently by the package export map/runtime import and by the absence of those names from the mapped type/doc surface
• stale strict-schema contract in the integration guide and generated README → blocks now; confirmed at cli-integrations.doc.mjs:175 and generated README.md:600, while readme:check passes because the stale guide is its source

ADVICE: bounded outcome criteria — keep compatibility bookkeeping off public package surfaces, make known-key detection unable to diverge from the parser schema, and make both authored and generated integration docs describe unknown-field warnings accurately.

AUTHOR CAN PROCEED: yes — no system/design choice is open; the acceptance criteria above are implementation-independent and covered by focused tests plus the public-surface/doc checks.

WORST OUTCOME: “a later known field can work while still being reported as unsupported, and two internal helpers become public runtime API” → request changes.

Prior review: none — the PR has no submitted reviews or line comments to reconcile.

JUDGEMENT NEEDED: none — both findings are contract inconsistencies, not API taste decisions.

request changes

1. [BLOCKS] the known-key census is separate from the schema and publicly exported → the next manifest field can work while warning falsely, and consumers see two accidental API names · packages/cli/authoring/integration/parse.mjs:19 — export const KNOWN_INTEGRATION_KEYS = [ confirmed independently by source/data-flow inspection and a package-subpath runtime import

2. [BLOCKS] the public integration guide still promises strict manifest validation → builders are taught the opposite of the compatibility behavior this release ships · packages/cli/assets/docs/cli-integrations.doc.mjs:175 — 'Everything is validated against one strict schema at the load boundary' confirmed independently in the authored guide and generated packages/cli/README.md:600

REVIEW

Thanks — preserving known contributions when an older CLI sees a newer manifest key is the right compatibility model. Before merge, could we keep the helper/key census out of the public ./integration surface and make it unable to drift from the schema, then update the integration guide and generated README's strict-schema claim?

Full review

[Reviewed by Robohands]

INLINE

• packages/cli/authoring/integration/parse.mjs:19 — This is public through ./integration and can drift from the schema. Could it stay internal and derived?

EVIDENCE I DID NOT SPEND

• The exact-head diagnostic groups two unknown keys in authored order and keeps severity at warning.
• Exact-head CI ran 16 check jobs: 13 succeeded; only the expected non-component a11y/RTL and review-anchor jobs skipped.
• Current main has not changed any touched file since the PR base, and a three-way merge is clean.

TIME

TIME total 17m setup/rules 6m full loop kit, rubric/API guidance, safe checkout, fast install build/server 0m not needed for a CLI/static review browser/a11y 0m non-rendering paths; no browser started focused tests 2m 47 exact-head tests + 40-test parent-code regression arm; no full suite code/history 4m body, diff, callers, contracts, docs, linked PRs, current main, CI critique/wiki 4m R16 presentation, two critic gates, fresh wiki record CI wait 0m exact-head workflows were already complete waste 1m recovered one dropped node command and corrected an empty wrong-base diff

WHAT I COULD NOT VERIFY

Nothing material. Remote CI owns the full suite; its test, build, lint, smoke, dependency, docsite, and scope checks all ran successfully on the exact head.

What changed before posting

Posted as drafted at exact head 4b6f80c8b64fc72879bcb55f987bc479bcf5228b: review 5044522965. The posted body is the REVIEW block above, including the full-review link and attribution.

MERGE: hold