fix: migrate internal schemas package to npm-hosted package - #472
Open
JoshMock wants to merge 44 commits into
Open
fix: migrate internal schemas package to npm-hosted package#472JoshMock wants to merge 44 commits into
JoshMock wants to merge 44 commits into
Conversation
Contributor
✅MegaLinter analysis: Success
Notices📣 MegaLinter 9.5.0 is out! Discover the new features and security recommendations in the release announcement. (Skip this info by defining See detailed reports in MegaLinter artifacts MegaLinter is graciously provided by OX Security |
JoshMock
force-pushed
the
schemas-pkg
branch
3 times, most recently
from
July 22, 2026 17:13
d6ef7f3 to
62d5772
Compare
Contributor
🔍 Preview links for changed docs |
Contributor
|
|
Contributor
|
|
JoshMock
marked this pull request as ready for review
July 30, 2026 20:14
Contributor
|
|
7 of 112 cloud definitions ship their input as {$schema, title, $ref:
"#/$defs/X", $defs} with no top-level `properties`. Every consumer guarded on
`.properties` and fell through to nothing, so the declared schema was thrown
away at three separate layers: buildCloudJsonSchema emitted an empty object
schema, extractSchemaArgs derived zero CLI flags, and buildCloudRequestParams
computed empty path/query/body key sets.
Observable effect on `cloud auth delete-api-keys`: no field flags in --help,
`{"type":"object","properties":{}}` from --help --json, `{"bogus":1}`
accepted without any validation, and a DELETE issued with its required body
dropped -- despite DeleteApiKeysRequest.required being ["keys"] in the
schema. This is the discard-on-guard-clause failure AGENTS.md warns about.
Adds resolveRootRef to src/lib/json-schema-refs.ts, which already owns $ref
handling. It dereferences a top-level $ref into its $defs target and keeps
the $defs map intact so nested refs still resolve, and it throws rather than
returning an empty schema when input is present but yields no properties.
Both src/cloud/types.ts and src/cloud/request-builder.ts consume it, so the
schema view and the request view cannot drift apart. buildCloudJsonSchema
also now carries $defs into its output, which AJV needs to resolve nested
refs in the newly exposed properties (e.g. RoleAssignments on
create-api-key).
Separately, collectBody checked the BODY_METHODS gate before consulting the
schema, which dropped bodies on DELETE. It now checks schema-declared body
keys first. Verified no existing GET command declares body fields, so the
reordering is behaviour-neutral elsewhere.
Tests load real cloud definitions via loadCloudApis rather than hand-crafted
schemas, covering the emitted schema, derived flags, rejection of unknown and
missing-required input, and body-on-DELETE.
Known remaining gap, out of scope: patch-current-account declares
`{"type":"string"}` for a raw JSON-merge-patch body, a non-object root rather
than a root $ref, and still emits an empty schema. Unchanged from baseline.
The ajv migration degraded the per-issue error output in two ways.
Paths. AJV v6 reports `dataPath` in mixed dot/bracket notation, but the
converter split it on "." alone, so the `path_array` field it fed came out
unusable: `{tags:[{name:5}]}` produced ["tags[0]","name"] rather than
["tags",0,"name"], and a key containing a dot produced
["[\x27weird","key\x27]"]. AJV also reports missing-required at the parent, so
`es indices create` reported an empty path rather than ["index"]. Replaced with
a tokenizer that emits numbers for indices and appends params.missingProperty
for `required`, so the path field names the offending field on its own instead
of forcing callers to string-parse the message.
Message quality. AjvError.params was collected but discarded, so enum failures
said only "should be equal to one of the allowed values" without listing them,
and an additionalProperties failure would not have named the offending field --
which AGENTS.md explicitly requires. params is now folded into the message for
enum and additionalProperties.
Each issue also carries a `code`, set to AJV\u2019s raw `keyword` -- `type`,
`required`, `enum`, `additionalProperties`. AJV\u2019s keyword is already a stable,
documented discriminator, so passing it through needs no mapping table, cannot
mislabel, and cannot throw on a keyword nobody enumerated.
The test guarding this had been weakened during the migration from asserting an
exact issue count, code, and deepEqual path down to `issues.length >= 1`. It is
restored to exact assertions. The path is ["query"] rather than trunk\u2019s
["query","term","category"] because the migration also flattened that test\u2019s
schema to a single object-typed query field with no nested union; ["query"] is
the correct exact path for the current shape.
src/lib/ajv-validate.ts constructed Ajv with options written for ajv v8 while
the dependency is ajv@6.14.0.
`strict: false` is not an ajv6 option and was a silent no-op. Its ajv6
counterpart, `unknownFormats: "ignore"`, was absent -- and ajv6 THROWS on an
unrecognised `format`. Since ajv.compile was uncaught, the first upstream
schema shipping a non-draft-07 format would have surfaced as a stack trace
rather than the structured {"error":{"code","message"}} AGENTS.md requires.
No format keywords exist in @elastic/schemas today, so this was latent and
one dependency bump away. Adds unknownFormats and wraps compile so any
compile failure becomes a schema_compile_failed error on the normal
stderr/non-zero-exit path.
`useDefaults: true` was reported as dead config. Scanning the real schema
sets confirms that: all 8 `"default"` occurrences are enum member literals or
a property named `default`, never the JSON Schema `default` keyword. It is
kept anyway because a pre-existing unit test asserts the behaviour, and
editing pre-existing tests was out of scope here. It can be dropped alongside
that test if the contract is retired.
A ponytail: comment records that these are ajv6/draft-07 semantics and what
changes on an ajv8 move. Not upgrading ajv in this change.
…kspace The migration to @elastic/schemas removed the packages/es-schemas workspace, but .github/release-please-config.json and .github/.release-please-manifest.json still declared it. The next release-please run would either fail on the missing path or keep publishing a phantom es-schemas component. Removed both entries. packages/config-resolver was verified to still exist, and rg confirms no remaining references to the path outside the lockfile and generated NOTICE/CHANGELOG files. Note: ARCHITECTURE.md still mentions @elastic/es-schemas, but as an npm package name rather than this workspace path. That is part of the separate documentation cleanup.
…ved Zod codegen
AGENTS.md instructs contributors and agents to read ARCHITECTURE.md before
proposing structural changes, and mandated Zod for every new command. Both
documents still described the code-generated Zod architecture this branch
deleted, so they would have actively misdirected future work.
AGENTS.md: the Tech Stack entry now names @elastic/schemas and ajv. The
command-authoring requirement now mandates a JSON Schema input document while
still forbidding `input: true`. The "Generic Abstractions: Lessons Learned"
items are kept -- the principles still hold -- but restated against the JSON
Schema equivalents ($ref, anyOf, oneOf, allOf, root $ref), with the
Zod-specific incidents retained as past-tense examples. Lesson 2 cites
resolveRootRef, which throws rather than letting a silently-empty schema reach
flag derivation; no error-code vocabulary is implied, since none exists.
ARCHITECTURE.md: the "Per-Endpoint API Files" section described 560 generated
files in directories that no longer exist; it is replaced with a description
of the lazy barrels in src/es/apis.ts and src/kb/apis.ts and the sidecar $ref
resolution and pruning in src/lib/json-schema-refs.ts. The "API Manifests as
TypeScript" section is removed because api-manifest.ts is now a nine-line
re-export from the package rather than hand-maintained metadata. The
cli-schema.ts description no longer calls it a Zod-to-JSON-Schema bridge, the
lazy-loading section refers to ajv rather than Zod, a section on
src/lib/ajv-validate.ts is added, and the dependency table drops
@elastic/es-schemas in favour of @elastic/schemas and ajv.
Neither document asserts a validation-error output format. The pre-existing
requirement that errors serialise as {"error":{"code","message"}} is trunk-era
and untouched.
The performance warnings tied to #171 are preserved and restated
against the current lazy-loading and memoization design rather than dropped.
The absence of any manifest-regeneration path is stated plainly.
Documentation only; no source or test files touched.
Registration metadata now comes from upstream: src/es/api-manifest.ts and src/kb/api-manifest.ts are nine-line re-exports of the package manifest. That removes any local regeneration burden, but it also means a version bump can add, rename, or drop commands with nothing in CI noticing. Adds test/lib/manifest-drift.test.ts, covering the manifest boundary across all four schema-derived namespaces: - every es and kb manifest entry resolves to a loadable definition, so a manifest entry pointing at a missing namespace file or export fails - every definition exported by the namespace files is reachable from the manifest, catching orphaned commands that would ship unregistered - command counts are pinned per namespace (es 567, kb 555, cloud 112, serverless 41) The count assertion reports the direction and size of the change and states that the expected value should be updated only after reviewing which commands moved, so the failure is actionable rather than an invitation to bump a number. Verified by injecting a drift and reading the message. cloud and serverless have no upstream manifest -- their barrels import namespace files directly -- so only count pinning applies there. Noted in the file for whenever upstream adds one. Kept as one cross-cutting file rather than four additions to the per-namespace suites, since the subject is the manifest boundary rather than any single namespace\u2019s loading behaviour. Full definition loading was preferred over pinning from manifest length alone because it is what makes orphan detection possible; it costs about 1.3s, reusing the loaders\u2019 existing memoization.
…barrels
scripts/heap-check only imported dist/{es,kb,cloud}/apis.js. Those barrels are
lazy by construction, so all three probes reported about 0.5 MB and the check
passed trivially. The invariant it is supposed to guard -- #171,
where loading every schema at once allocated gigabytes -- concerns definition
loading, which the probe never triggered. Single-endpoint cost could have
grown tenfold without failing CI.
Adds three definition-loading probes, each in a fresh child process so module
caches cannot cross-contaminate:
- loadEsApisInFile("search"), baseline 9.5 MB against 8.0 MB measured. This is
the number that matters, since it is the sidecar-ref inlining cost paid on
every real command invocation.
- loadKbApisInFile("post_alerting_rule_id"), baseline 5.1 MB against 4.25 MB
measured. Every Kibana namespaceFile holds exactly one definition (555
entries, 555 distinct files), so there is no large namespace to pick; this is
the heaviest single schema instead, a per-rule-type union body flattened by
flattenComposition.
- loadAllEsApis(), record-only at 66.4 MB / 567 defs. Gating it would budget a
path no user hits, since the CLI only ever loads one endpoint.
Baselines reuse the existing 20% HEAP_THRESHOLD rather than introducing a
second tolerance convention. The barrel probes are kept, with a comment
recording that they still guard the lazy-barrel invariant so they are not
deleted as trivial.
Script wall-clock goes from about 0.3s to 1.33s, dominated by the full-load
probe. It stays in the existing per-PR heap job.
… imitations AGENTS.md lesson 3 says to test with real generated schemas because toy ones miss codegen-specific types. The new validation tests did not follow it: every schema in test/lib/ajv-validate.test.ts is inline, and test/factory.test.ts routes about 60 rewritten cases through a local jsonSchema() helper. That gap is why _source rejecting its string and array forms shipped unnoticed. Adds test/lib/ajv-validate-real-schemas.test.ts, covering the six upstream shapes that hand-written schemas do not reproduce, each against a real command found by scanning the installed package: - bare $ref property kb apm-agent-configuration delete-agent-configuration, `service` - root $ref document cloud delete-api-keys - allOf-composed body cloud create-deployment - multi-branch oneOf kb agent-builder post-agent-builder-converse, `prompts` - nullable enum kb alerting post-alerting-rule-id, `notify_when` (enum literally ends [null, null]) - anyOf union es search, `_source` Each shape gets a passing input and a failing one pinning code, path, and message. Codes are AJV\u2019s raw keywords, observed from live output rather than inferred: the anyOf branch mismatches both fire `type`, and the oneOf case fires `required` three times plus a root `oneOf`. For _source and prompts the current implementation emits multiple issues rather than one; that was verified rather than assumed, so those cases pin the real output under a ponytail: comment naming the gap instead of asserting a single-issue property that does not hold. Collapsing them is a separate change. The existing jsonSchema() cases are deliberately left alone. Most exercise generic factory plumbing where a minimal schema is the right tool, and churning them would add risk without adding signal. On #172: the real ES search.query field (QueryDslQueryContainer) is modelled as plain type: object upstream, so the multi-variant error explosion is structurally impossible there and the existing test\u2019s premise holds. Rather than fabricate a union that no longer exists, the real multi-branch oneOf case records that the guarantee comes from how those specific ES fields are modelled, not from anything generic about upstream oneOf. Also tightens one loosened assertion in test/factory.test.ts, from /should be string|expected string|type.*string/i -- which accepted both the pre- and post-migration message -- to the actual current /should be string/. Adds about 1s to the suite; loaders are memoized.
The migration removed zod from dependencies, but test/factory.test.ts still
imported it for seven cases. It resolved only transitively, so an unrelated
dependency bump could have broken the suite. @cli-schema/commander and
@cli-schema/zod were likewise still declared while only @cli-schema/spec is
imported, and @cli-schema/zod was what kept zod in node_modules at all --
which is why removing the dependency and fixing the tests had to happen
together.
Five of the seven cases were not merely using zod, they were vacuous: a Zod
object exposes no `properties` or `type` to the factory, so the assertions
passed regardless of the schema.
- three cases used z.object({ q: z.string().optional() }) to test flag
behaviour, but no --q flag was ever derived, so the flag-derivation half of
each test did nothing. Now { type: "object", properties: { q: { type:
"string" } } }, which actually produces the flag.
- two cases asserted --input-file and --dry-run hiding "for an empty input
schema" via z.looseObject({}), which the factory could not distinguish from
any other Zod value. Now { type: "object", properties: {} }, matching the
zero-properties check the factory really performs.
The remaining two (z.object({ document: z.any() })) were exercising
inputTransform rather than schema shape and are a straight translation.
Converting them required no source change: the assertions now pass for the
right reason.
package-lock.json and NOTICE.txt regenerated; generate-notice --check passes.
zod remains present transitively via @elastic/schemas, but nothing in this
repo imports it.
Three related defects, all from the migration carrying Zod schema shape
assumptions into JSON Schema code.
src/kb/request-builder.ts read requiredness as a boolean on the property
(`prop["required"] !== false`). In JSON Schema `required` is a string array on
the parent, so that expression was always true and the placeholder-strip
branch below it was unreachable. An optional path parameter would have left a
literal {id} in the URL. The parent required array is now threaded into
interpolatePath, matching how the ES and cloud builders already read it. This
was latent -- all 555 Kibana definitions keep their path params inside
input.required -- so the new test uses a constructed definition, noted as such
in a comment, since no real definition reaches the branch.
src/cloud/types.ts checked `prop["required"] === true` before the correct
array check. Harmless, since the array check followed, but it advertised a
shape that does not exist. Removed.
src/cloud/request-builder.ts stripped the /{key}/ segment for ANY missing path
key. Trunk stripped only when the param was optional. The migration therefore
turned a missing REQUIRED path param into a silently truncated URL --
/api/v1/deployments/{id}/x becoming /api/v1/deployments/x -- rather than an
error. AJV catches this whenever the schema lists the field as required, but
anything unlisted failed open. Only optional keys are stripped now; a missing
required one throws naming the parameter, and createCloudHandler surfaces it
as {"error":{"code":"invalid_request","message"}} on stderr with a non-zero
exit.
encodePathParam is unchanged; path params remain percent-encoded per segment.
…code from coverage Two things this branch was hiding from view. extractSchemaArgs skipped reserved and duplicate kebab-case flags with a bare `return false`. Measured against the real schemas, that silently drops exactly three Kibana fields -- security-exceptions-api update-exception-list.version, security-lists-api patch-list.version, and security-lists-api update-list.version -- each colliding with the flag derived from the sibling _version. AGENTS.md requires every top-level schema field to have a flag, so the drop is a contract gap, and being silent meant nobody would find it. Not data loss: collectBody still forwards the fields from stdin. It now throws, with KNOWN_UPSTREAM_FLAG_COLLISIONS carrying the three known cases, mirroring KNOWN_UPSTREAM_PATH_PARAM_MISMATCHES added earlier in this series. Chose this over synthesising a second flag name such as --underscore-version: upstream is inconsistent about whether `version` means the optimistic-concurrency `_version` or something else, so inventing a flag would advertise a contract we cannot honour. Any unreviewed collision is now a hard registration failure rather than an invisible gap. validateSchemaArgs becomes unreachable from production as a result, since extractSchemaArgs performs the same check first. Kept as defence in depth and because pre-existing tests exercise it directly on hand-built arrays; its docstring now says so. Coverage config still excluded src/cloud/apis/** and src/es/apis/**, both deleted directories, and also src/es/apis.ts and src/cloud/apis.ts -- which are no longer generated barrels but hand-written loader logic with real error branches, unlike their included sibling src/kb/apis.ts. All four excludes are removed and tests added for the cache-hit and manifest-miss paths. Branch coverage: es/apis.ts 77.78 to 85.71, cloud/apis.ts 60.71 to 82.61, aggregate 89.87 to 90.18. No threshold was lowered. loadCloudApis is refactored from eleven literal destructured imports to a loop over a module table, matching loadEsApisInFile and loadKbApisInFile. That was necessary rather than cosmetic: each destructured named import generates a never-taken CJS-interop branch that no test can reach, and those artifacts alone kept the file below threshold. Same definitions, same errors. The remaining uncovered line in each loader is the defensive "namespace file did not export expected key" throw, unreachable with correctly published schemas and deliberately not mocked.
…ments test/lib/schema-args.test.ts imports only from src/lib/json-schema-args.ts; the module it was named after was deleted in this migration. Renamed to test/lib/json-schema-args.test.ts, a zero-diff move. No test glob, workflow, doc, or coverage entry referenced the old name. Left separate from json-schema-args-collisions.test.ts, added earlier in this series, since that file covers registration-time collision failure rather than argument extraction. The build-memory workarounds in .github/workflows/ci.yml and both .buildkite scripts were justified by a comment claiming the per-endpoint Zod schemas compiled to ~72 MB of .d.ts. Generated .d.ts output is now 340 KB, so the comments are false and were rewritten with measured figures. The --max-old-space-size flags are deliberately left alone. Measured over three runs each with dist and tsbuildinfo cleared, npm run build peaks at 3.67 GB RSS and test:unit at 0.43 GB. 3.67 GB leaves only about 2.2x headroom under the 8192 MB local limit and 1.7x under CI\u2019s 6144 MB, which is not enough margin to shrink safely across the Windows and macOS runners. The ceiling is an upper bound rather than a reservation, so leaving it costs nothing while tightening it risks flaky OOM. ARCHITECTURE.md does not mention these limits. Its #171 section concerns runtime heap during schema loading, a separate and still-valid concern.
KbPathParam, KbQueryParam, and KbBodyParam had zero callers anywhere in src, test, scripts, or packages -- only their own declarations. The comment above them claimed they were kept for backward compatibility with cloud and kb api files that declare params separately, but those files were deleted by this branch, so the stated subject no longer exists. KbApiDefinition now describes parameters solely through its JSON Schema `input` and x-found-in. Worth removing rather than leaving inert: all three encode `required` as a boolean on the parameter, which is the Zod-era shape fixed as a live bug in src/kb/request-builder.ts earlier in this series. An exported, canonical-looking definition of the wrong shape in the namespace type module is an invitation to reintroduce it. Test count unchanged at 1606.
stripTransportMeta skipped both `found_in` and `x-found-in`. @elastic/schemas@0.5.0 only ever emits the prefixed form -- rg finds no occurrence of a bare "found_in" data key anywhere in the package, across the compiled tools output, the json sources, and the manifest -- and no test fixture uses it either. The branch had no subject. Also corrects the prose that kept the bare spelling alive, since leaving it reproduces the same confusion the branch did: - three stripping tests in test/factory.test.ts were titled "found_in" while their fixtures used x-found-in - a title and comment in test/es/register.test.ts said found_in: "path" - two regexes in test/es/types.test.ts allowed `index.*found_in.*path`, which only ever matched because found_in is a substring of x-found-in. Tightened to the literal key the production message actually emits. The first alternative in each regex was already sufficient, so nothing is loosened. - AGENTS.md described the key as `found_in: path | query | body`. That reads as the literal key stripped by stripTransportMeta and required by validateApiDefinition rather than an abstract concept, so it takes the prefix too. No assertion, fixture, or logic changed beyond the regex tightening. Test count unchanged at 1606, and es search --help --json still contains zero occurrences of either spelling.
…e they do not
ajv-validate.ts hand-rolled AjvInstance and ValidateFn over ajv, which ships
lib/ajv.d.ts. Both are replaced with the real Ajv and ValidateFunction via a
type-only import, erased at compile time, so the lazy createRequire("ajv") in
getAjv() is preserved -- verified: dist/lib/ajv-validate.js contains
require("ajv") only inside getAjv().
The import must be named. ajv uses `export =` with a namespace, so
`import type Ajv from "ajv"` fails TS2709.
ValidateFunction returns `boolean | PromiseLike<any>` for async schemas. Since
this module only compiles synchronous schemas, the call site narrows with
`=== true` and a comment noting the other arm is unreachable here.
ajv's ErrorObject is deliberately NOT imported. Its params field is typed as
ErrorParameters, a twelve-member union: reading missingProperty, allowedValues,
or additionalProperty off it fails TS2339, and the union is not assignable to
Record<string, unknown> for lack of an index signature. Adopting it would trade
four lines of local type for several casts plus union narrowing. The local
AjvError is instead replaced by AjvErrorView, which names exactly the three
params this module consumes -- strictly more precise than the
`Record<string, unknown>` it had. A comment records the tradeoff so it is not
"fixed" later without it.
That leaves one cast, at the single validate.errors boundary, commented in
place. Types only; behaviour is identical and no test changed.
JoshMock
changed the title
Contributor
|
|
Contributor
|
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
I'm working on a standalone package for storing all Zod schemas. This removes the bundled package and depends on that package. Not ready to merge until
@elastic/schemasis published to npm.Major changes:
packages/es-schemas/no longer exists; imports@elastic/schemasinsteadResults:
npm pack(gzip of compiled source) size reduced from 26MB to 210KB