Add source and build validation modes#1175
Closed
teresaromero wants to merge 23 commits into
Closed
Conversation
Introduce a modes.Mode enum (Legacy/Source/Build) and wire it into the Spec struct and rulesDef filter loop. Defaulting to Legacy reproduces today's behavior exactly — no rules are re-tagged yet. - New package: code/go/internal/validator/modes - NewSpec now accepts a mode parameter (defaults to Legacy if empty) - rulesDef gains a modes field; nil means "applies in all modes" - Filter loop skips rules whose modes list excludes the current mode All existing tests pass unchanged. No fixture or semantic rule changes. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Add validator.Mode type alias re-exporting internal modes.Mode so callers can write validator.ModeLegacy / ModeSource / ModeBuild. - Add Validator struct with NewFromPath, NewFromZip, NewFromFS constructors and a Validate method. - Add WithWarningsAsErrors functional option. - Rewrite ValidateFromPath/Zip/FS as 3-line wrappers that delegate to NewFromX(ModeLegacy, ...) — zero behaviour change. - Add parametrised legacy-preservation tests across all ~180 fixtures for each constructor path, plus a zip golden test (first zip coverage in this repo). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
… 03) - Tag ValidateExternalFieldsWithDevFolder and ValidateTestPackageRequirements with modes:[Legacy, Source] so they are skipped in build mode. - Add ValidateNoEmbeddedEcsInDynamicTemplates (source mode only, integration packages): walks data_stream/*/manifest.yml and rejects any dynamic_templates entry whose key starts with '_embedded_ecs'. These keys are auto-injected by elastic-package at build time and must not appear in source packages. - Register the new rule with modes:[Source] and types:[integration]; it does not run in legacy mode, preserving byte-for-byte legacy behaviour. - Resolve the TODO at spec/integration/data_stream/manifest.spec.yml:351: replace with a comment pointing to the new validator and issue elastic#549. - Add fixture test/packages/bad_embedded_ecs/: minimal valid integration package containing _embedded_ecs-* entries; passes legacy, fails source. Also serves as JSON schema coverage for patternProperties '^_embedded_ecs'. - Update test/packages/good_v3/data_stream/ecs_import_mappings/manifest.yml: replace 90+ _embedded_ecs-* dynamic template entries with regular ones so good_v3 passes source-mode validation (build artifacts must not appear in source packages). - Add to api_test.go: TestSourceMode_GoodPackages - all good_* fixtures pass ModeSource TestSourceMode_BadEmbeddedEcs - bad_embedded_ecs: legacy pass, source fail (_embedded_ecs), build pass TestBuildMode_SkipsBuildExcludedRules - ValidateExternalFieldsWithDevFolder absent in build mode applyPackageFilter helper - applies validation.yml exclusions so packages with known-excluded codes (e.g. SVR00006) are treated consistently Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Add ValidateNoDevFolder semantic validator (build-mode only)
Walks the package FS and errors on any _dev/ directory, skipping
the subtree after the first hit to avoid duplicate child errors.
- Register {fn: ValidateNoDevFolder, modes: [Build]} in spec.go rulesDef
- Add test/packages/build_mode/good_built/ — canonical minimal built
package fixture (no _dev/, no .link files, no external: ecs)
- Add test/packages/build_mode/bad_built_with_dev/ — copy of good_built
with an empty _dev/build/build.yml planted to trigger the rule
- Add TestBuildMode_NoDevFolder to api_test.go covering:
good_built passes ModeBuild
bad_built_with_dev fails ModeBuild with clear error message
bad_built_with_dev passes ModeSource (_dev/ is allowed in source)
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Add ValidateNoLinkFiles semantic validator that errors on any .link file found in the package tree - Register rule in spec.go with modes: [Build] (build-only) - Add bad_built_with_link fixture: copy of good_built with a .link file planted under data_stream/events/fields/ - Add unit tests for ValidateNoLinkFiles (5 cases) - Add TestBuildMode_NoLinkFiles integration test: build mode rejects .link files; source mode (with_links fixture) is unaffected Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- ValidateNoExternalEcs errors on any field with external: ecs in build mode - Registered in spec.go scoped to modes:[Build] (no types guard, consistent with ValidateNoDevFolder / ValidateNoLinkFiles) - Fixture: test/packages/build_mode/bad_built_external_ecs/ (good_built copy with one host.name field carrying external: ecs) - Unit tests cover: no externals, materialized fields, single/multiple ecs externals, non-ecs external (not rejected) - Integration test: bad fixture fails ModeBuild; good_v3 passes ModeSource Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- ValidateStreamInputMaterialized rejects built packages that still carry
source-only 'package:' fields in two locations:
* data_stream/*/manifest.yml streams[].package — composable-input pattern
* manifest.yml policy_templates[].inputs[].package — package-reference pattern
Both must be materialized (input:/type:) before packaging.
- Also rejects data stream streams missing the required 'input:' field.
- Registered for ModeBuild + integration packages only.
- Unit tests: 9 cases covering both branches.
- Integration tests: 6 cases (3 bad fixtures × fail in build / pass in source).
- New fixtures: bad_built_stream_package, bad_built_missing_input,
bad_built_policy_template_package.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Stray files (.gitkeep, .DS_Store, etc.) sitting directly under data_stream/ were included in the list returned by listDataStreams, causing every caller to attempt opening <file>/manifest.yml and surface a confusing internal error instead of a clean validation message. Filter to directories only using IsDir(). The relative order of entries is unchanged since fs.ReadDir already returns them sorted by filename. Add TestListDataStreamsIgnoresNonDirectories to validate_no_embedded_ecs_test.go to cover the stray-file case end-to-end. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Update all bad_built_* manifests: name/title/description now match their directory instead of copying good_built's values - Fix TestValidateNoLinkFiles: mid-name .link case now creates base-fields.link.yml so filepath.Ext returning .yml is exercised Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
S2: skip 'missing input:' check when 'package:' already flagged on the same stream entry. The root cause is 'package:', not a forgotten 'input:'; emitting both errors was misleading for the composable-input pattern. T1: strengthen good_v3 source-mode sub-test to require.NoError (it is a known-good source fixture). Add explanatory comments to each bad_built_* source-mode sub-test explaining why they cannot use hard assertions — those fixtures have unrelated source-validation errors that are not under test. T2: remove stale '(Tasks 04-07, not yet implemented)' parenthetical from TestBuildMode_SkipsBuildExcludedRules — those rules are now implemented. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- NewFromFS(ModeSource): wrap with linkedfiles.NewFS instead of BlockFS so .link files are resolved transparently, matching the documented mode contract - NewFromZip: drop the mode parameter and always use ModeBuild; zip files are by definition built packages (elastic-package build output / package registry format). Deprecated ValidateFromZip keeps ModeLegacy via internal newFromZip. - folder_item_spec: skip syntactic validation for .link files that BlockFS cannot open, preventing cryptic ErrUnsupportedLinkFile errors before the clean semantic ValidateNoLinkFiles error fires in build mode - Consolidate duplicate packageTypeIntegration constant into integrationPackageType (types.go) across all five files in the semantic package Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Also apply gofmt/goimports formatting to spec.go import order and test struct field alignment caught by make check. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Warning Review limit reached
More reviews will be available in 48 minutes and 1 second. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Enterprise Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds mode-aware validation (Legacy/Source/Build), gates semantic rules by mode, implements build-only validators (_dev, .link, _embedded_ecs, external: ecs, stream input materialization), exposes a public Validator API with mode-aware constructors, updates JSON schemas for composable inputs, and adds comprehensive tests and fixtures. ChangesMode infrastructure and Spec wiring
Build-mode semantic validators
Composable package support and small fixes
Public validator API
JSON schema updates
Test fixtures and integration tests
Sequence Diagram(s) sequenceDiagram
participant Caller
participant ValidatorAPI
participant FS
participant Spec
participant Rule
Caller->>ValidatorAPI: NewFromPath(mode, path)
ValidatorAPI->>FS: wrap/block linked files per mode
Caller->>ValidatorAPI: v.Validate()
ValidatorAPI->>Spec: NewSpec(version, mode)
Spec->>Rule: run gated rules
Rule-->>ValidatorAPI: return validation errors
ValidatorAPI-->>Caller: return aggregated errors
Estimated code review effort 🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
✨ Finishing Touches🧪 Generate unit tests (beta)
|
Both constructors previously returned a nil error unconditionally; they now validate the root path/filesystem at construction time so callers get a clear error immediately rather than a confusing failure deep inside Validate(). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Composable integration streams reference an input package via 'package:'. When no explicit template_path/template_paths is set, all templates come from the referenced input package and only exist after build. Template validation for those streams is skipped — ValidateStreamInputMaterialized already enforces that 'package:' is replaced by 'input:' in build mode. If a composable stream does define its own template_path or template_paths (overlay templates that live in the source package), those files are still validated as before. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…o local templates Mirrors the stream-level fix: policy template inputs referencing an input package via 'package:' do not carry agent/input templates in source — those come from the dependency and are only present after build. Add Package field to policyTemplateInput so the composable case is explicitly recognised. When 'package:' is set and no template_path/ template_paths is declared, validation is skipped. When the composable input defines its own overlay templates, those files are still validated. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…te inputs
In composable packages, streams and policy template inputs reference an
input package via 'package:'. Vars listed there are partial overrides of
the dependency's vars: name and type come from the dependency and are only
present in the built package. Requiring name and type in source mode causes
false validation errors.
Fix: change the vars property on stream items (data_stream/manifest.spec.yml)
and on policy template input items (integration/manifest.spec.yml) from a
direct $ref to the full vars definition to a permissive base schema
({type: array, items: {type: object}}). Full validation (name + type
required, all allOf conditions) is applied via an if/then only when the
stream or input is non-composable (required: [input] / required: [type]).
Also fix four allOf if-conditions in #/definitions/vars to guard on
required: [type] before applying type-specific constraints (select, password,
duration, not-duration). Without this guard, absent type causes all
conditions to fire vacuously in JSON Schema, generating spurious errors.
The #/definitions/vars definition itself is unchanged: it still requires
[name, type] and is used as-is for input packages, package-level vars, and
policy template-level vars — all non-composable contexts.
Spec version compat: before: 3.6.0 patches remove 'package:' and add
input/type to required, so the if: required: [input/type] condition is
always true for older specs and full validation continues to apply.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
In composable source packages, policy template inputs use 'package:' instead of 'type:'. This leaves input.Type as "" for all composable inputs. When a policy template has multiple such inputs, SVR00010 fires spuriously because they all share the same empty type — even though the name is assigned by elastic-package at build time, not in source. Move ValidateIntegrationInputQualifier to modes: [Build]. In build mode, ValidateStreamInputMaterialized has already ensured every 'package:' was replaced by a real 'type:', so the name-uniqueness check is meaningful. Update tests: remove bad_input_qualifier_ambiguous from TestValidateFile (legacy mode) and add TestBuildMode_InputQualifierAmbiguous to verify the rule fires in build mode and is silent in source mode. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/packages/build_mode/bad_built_missing_input/docs/README.md (1)
1-6:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix README content to match the package.
The README describes "Good Built Package" and claims the package is "valid under
ModeBuild" with "noexternal: ecsfield references," but this file is inbad_built_missing_input/and the package'sfields.ymlcontainsexternal: ecson line 5. The README appears to have been copied from a different fixture and not updated.📝 Proposed fix for the README
-# Good Built Package +# Bad Built Package - Missing Input -This is the canonical minimal built-package fixture used for build-mode validation -tests (issue `#549`). It has no `_dev/` directories, no `.link` files, and no -`external: ecs` field references — making it valid under `ModeBuild`. +This is a built-package fixture that is invalid in build mode (issue `#549`). +The data stream manifest declares streams without the required `input` field, +making it invalid under `ModeBuild`.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/packages/build_mode/bad_built_missing_input/docs/README.md` around lines 1 - 6, The README incorrectly describes the fixture as a valid "Good Built Package" for ModeBuild; update the README title and text to reflect that this is the bad_built_missing_input fixture (i.e., it contains a missing input issue), explicitly note that fields.yml includes an external: ecs reference (present on line 5 of fields.yml) and therefore the package is invalid under ModeBuild, and remove or change any claims about no `external: ecs` references so the README accurately documents the package state.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@code/go/internal/validator/semantic/validate_stream_input_materialized_test.go`:
- Around line 127-128: Rename the abbreviated loop variable tc to testCase
across the subtest loop to satisfy style rules: change the range loop "for _, tc
:= range tests" to "for _, testCase := range tests" and update all uses inside
the t.Run closure (e.g., tc.name -> testCase.name) so the subtest name and any
assertions/reference use the new identifier; ensure you update every occurrence
within the loop body including nested closures to avoid unresolved identifiers
or shadowing.
In `@code/go/internal/validator/semantic/validate_stream_input_materialized.go`:
- Around line 76-77: The loop variables use abbreviated names that violate the
no-abbreviations rule: rename dsName to a descriptive name such as
dataStreamName in the loop iterating over dataStreams (the manifestRelPath
construction using dataStreamDir, dataStreamName, "manifest.yml") and rename pt
to a full name such as partitionType (or partition) wherever pt is used around
the code that handles partitions; update all references to these identifiers
within validate_stream_input_materialized.go (including function
validate/processing logic that reads manifest or partition metadata) to the new
names to ensure compilation and consistent naming.
- Around line 125-130: In validatePolicyTemplateInputsMaterialized, the
fs.ReadFile call (reading manifestRelPath) currently swallows all errors; change
the handling so that only fs.ErrNotExist is treated as "no manifest" and returns
nil, while any other error (permission/IO/etc.) is returned (or wrapped) to the
caller so it fails validation; locate the fs.ReadFile invocation and replace the
unconditional return nil with an explicit if errors.Is(err, fs.ErrNotExist) {
return nil } else { return fmt.Errorf("reading %s: %w", manifestRelPath, err) }
(or equivalent propagation) so real filesystem errors aren't masked.
In `@code/go/internal/validator/spec.go`:
- Around line 50-55: The NewSpec function currently defaults an empty mode to
modes.Legacy but accepts any non-empty invalid mode; change NewSpec (function
NewSpec, type modes.Mode) to validate the mode and fail fast by returning an
error when mode.Valid() returns false: after the empty-mode defaulting (if mode
== "" set modes.Legacy), call mode.Valid() and if it is false return a
descriptive error (e.g., mentioning the invalid mode value and that
modes.Valid() failed) so invalid non-empty modes are rejected instead of
silently accepted.
In `@code/go/pkg/validator/api_test.go`:
- Around line 114-120: The test currently defers zw.Close() and f.Close()
without checking errors which can hide write/flush failures; modify the
createPackageZip test to capture and assert Close() errors for both the zip
writer created by zip.NewWriter and the output file from os.Create: call
zw.Close() (or defer a closure that checks its returned error) before closing
the file, assert require.NoError(t, err) on that call, then close the file and
assert require.NoError(t, err) on f.Close() as well, ensuring the writer is
closed before the file to catch any finalization errors.
- Around line 54-55: The test currently uses a direct type assertion (if verrs,
ok := err.(specerrors.ValidationErrors); ok) which fails when err is wrapped;
change this to use errors.As: declare a variable like var verrs
specerrors.ValidationErrors and then use if errors.As(err, &verrs) { msgs :=
make([]string, len(verrs)) ... } and ensure the standard "errors" package is
imported so wrapped ValidationErrors are correctly detected.
In `@code/go/pkg/validator/api.go`:
- Around line 24-27: The fields and locals using short abbreviations should be
renamed for clarity: change the struct field Validator.fsys and any
parameter/local named fsys to fileSystem (or fileSystem fs.FS) and rename any
variable or receiver named va to validatorInstance (or validatorInstance
*Validator); update all references in methods/functions that use Validator.fsys,
the constructor/creator, and callers (including the places corresponding to the
diffs around the usages referenced) so types, parameter names, and call sites
match the new identifiers.
- Around line 85-88: The defer cleanup currently ignores Close() errors (defer
func() { if err != nil { r.Close() } }) and v.closer.Close(), and constructors
accept a Mode without validating unknown values while using abbreviated names
like fsys and va; fix by capturing and handling returned errors from r.Close()
and v.closer.Close() (log or wrap and propagate where appropriate) instead of
discarding them, add validation in NewSpec (or the relevant constructor) to
reject unknown Mode values (return an error) so mode-gated rules don’t silently
behave like ModeSource, and rename abbreviated identifiers (fsys -> filesystem,
va -> validatorApp or similar) to comply with full-name conventions; update any
call sites to handle the new error returns and renamed identifiers.
In `@test/packages/build_mode/bad_built_external_ecs/docs/README.md`:
- Around line 1-5: The README is misleading: update its title and description in
test/packages/build_mode/bad_built_external_ecs/docs/README.md to indicate this
is a "Bad Built Package" (or similar) used to test build-mode rejection and
explicitly mention that the fixture contains an `external: ecs` reference in
fields/fields.yml that triggers the failure under ModeBuild; keep the rest of
the explanatory text but correct the wording to reflect the intentional
invalidity and point to fields/fields.yml as the source of the `external: ecs`
reference.
In `@test/packages/build_mode/bad_built_policy_template_package/docs/README.md`:
- Around line 1-5: The README currently claims this is a "Good Built Package"
but the fixture lives in bad_built_policy_template_package and should document
why it fails ModeBuild; update the README title and description (change "Good
Built Package" to "Bad Built Package" or similar) and list the specific
build-mode failures the fixture is meant to demonstrate (e.g., presence of
`_dev/` directories, `.link` files, and `external: ecs` field references) so
readers/tests understand this is an invalid ModeBuild case.
In `@test/packages/build_mode/bad_built_stream_package/docs/README.md`:
- Around line 1-5: The README currently calls the fixture "Good Built Package"
and claims it's "valid under `ModeBuild`" which contradicts the directory name
bad_built_stream_package; update the README title and body to clearly state this
is a negative fixture (e.g., title "Bad Built Package — negative fixture") and a
short description saying it is intentionally invalid for ModeBuild validation
and will fail build-mode checks for the reasons the test covers; specifically
replace the phrases "Good Built Package" and "valid under `ModeBuild`" with
language that explains the directory exists to fail ModeBuild validation.
In `@test/packages/build_mode/bad_built_with_dev/docs/README.md`:
- Around line 1-5: README currently incorrectly labels the fixture as "Good
Built Package" and claims "no `_dev/` directories" while the fixture is named
`bad_built_with_dev` and is intended to test rejection of source-only `_dev/`
artifacts; update the README content to state that this fixture contains a
`_dev/` directory (a source-only artifact), explains that it should fail
ModeBuild validation, and references issue `#549` so maintainers know it's used to
verify that build-mode validator rejects packages with `_dev/` directories.
In `@test/packages/build_mode/bad_built_with_link/docs/README.md`:
- Around line 1-5: The README currently claims this is a "Good Built Package"
valid under ModeBuild but the fixture is named bad_built_with_link and should
represent an invalid build; update the README text to state that this fixture
contains .link files (and/or external: ecs references) and is intentionally
invalid under ModeBuild to test build-mode rejection rules, referencing the
fixture name bad_built_with_link and ModeBuild so readers understand its
purpose.
---
Outside diff comments:
In `@test/packages/build_mode/bad_built_missing_input/docs/README.md`:
- Around line 1-6: The README incorrectly describes the fixture as a valid "Good
Built Package" for ModeBuild; update the README title and text to reflect that
this is the bad_built_missing_input fixture (i.e., it contains a missing input
issue), explicitly note that fields.yml includes an external: ecs reference
(present on line 5 of fields.yml) and therefore the package is invalid under
ModeBuild, and remove or change any claims about no `external: ecs` references
so the README accurately documents the package state.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 9d201520-7963-401c-bf0c-522080576283
📒 Files selected for processing (95)
.gitignorecode/go/internal/validator/folder_item_spec.gocode/go/internal/validator/modes/modes.gocode/go/internal/validator/semantic/types.gocode/go/internal/validator/semantic/validate_datastream_package_categories.gocode/go/internal/validator/semantic/validate_integration_inputs_deprecated.gocode/go/internal/validator/semantic/validate_integration_policy_template_path.gocode/go/internal/validator/semantic/validate_integration_policy_template_path_test.gocode/go/internal/validator/semantic/validate_no_dev_folder.gocode/go/internal/validator/semantic/validate_no_dev_folder_test.gocode/go/internal/validator/semantic/validate_no_embedded_ecs.gocode/go/internal/validator/semantic/validate_no_embedded_ecs_test.gocode/go/internal/validator/semantic/validate_no_external_ecs.gocode/go/internal/validator/semantic/validate_no_external_ecs_test.gocode/go/internal/validator/semantic/validate_no_link_files.gocode/go/internal/validator/semantic/validate_no_link_files_test.gocode/go/internal/validator/semantic/validate_policy_template_datastream_categories.gocode/go/internal/validator/semantic/validate_stream_input_materialized.gocode/go/internal/validator/semantic/validate_stream_input_materialized_test.gocode/go/internal/validator/spec.gocode/go/internal/validator/spec_test.gocode/go/pkg/validator/api.gocode/go/pkg/validator/api_test.gocode/go/pkg/validator/mode.gocode/go/pkg/validator/validator.gocode/go/pkg/validator/validator_test.gospec/changelog.ymlspec/integration/data_stream/manifest.spec.ymlspec/integration/manifest.spec.ymltest/packages/bad_embedded_ecs/changelog.ymltest/packages/bad_embedded_ecs/data_stream/logs/agent/stream/stream.yml.hbstest/packages/bad_embedded_ecs/data_stream/logs/fields/base-fields.ymltest/packages/bad_embedded_ecs/data_stream/logs/manifest.ymltest/packages/bad_embedded_ecs/docs/README.mdtest/packages/bad_embedded_ecs/manifest.ymltest/packages/build_mode/bad_built_external_ecs/LICENSE.txttest/packages/build_mode/bad_built_external_ecs/changelog.ymltest/packages/build_mode/bad_built_external_ecs/data_stream/events/agent/stream/stream.yml.hbstest/packages/build_mode/bad_built_external_ecs/data_stream/events/fields/base-fields.ymltest/packages/build_mode/bad_built_external_ecs/data_stream/events/fields/fields.ymltest/packages/build_mode/bad_built_external_ecs/data_stream/events/manifest.ymltest/packages/build_mode/bad_built_external_ecs/docs/README.mdtest/packages/build_mode/bad_built_external_ecs/manifest.ymltest/packages/build_mode/bad_built_missing_input/LICENSE.txttest/packages/build_mode/bad_built_missing_input/changelog.ymltest/packages/build_mode/bad_built_missing_input/data_stream/events/agent/stream/stream.yml.hbstest/packages/build_mode/bad_built_missing_input/data_stream/events/fields/base-fields.ymltest/packages/build_mode/bad_built_missing_input/data_stream/events/fields/fields.ymltest/packages/build_mode/bad_built_missing_input/data_stream/events/manifest.ymltest/packages/build_mode/bad_built_missing_input/docs/README.mdtest/packages/build_mode/bad_built_missing_input/manifest.ymltest/packages/build_mode/bad_built_policy_template_package/LICENSE.txttest/packages/build_mode/bad_built_policy_template_package/changelog.ymltest/packages/build_mode/bad_built_policy_template_package/data_stream/events/agent/stream/stream.yml.hbstest/packages/build_mode/bad_built_policy_template_package/data_stream/events/fields/base-fields.ymltest/packages/build_mode/bad_built_policy_template_package/data_stream/events/fields/fields.ymltest/packages/build_mode/bad_built_policy_template_package/data_stream/events/manifest.ymltest/packages/build_mode/bad_built_policy_template_package/docs/README.mdtest/packages/build_mode/bad_built_policy_template_package/manifest.ymltest/packages/build_mode/bad_built_stream_package/LICENSE.txttest/packages/build_mode/bad_built_stream_package/changelog.ymltest/packages/build_mode/bad_built_stream_package/data_stream/events/agent/stream/stream.yml.hbstest/packages/build_mode/bad_built_stream_package/data_stream/events/fields/base-fields.ymltest/packages/build_mode/bad_built_stream_package/data_stream/events/fields/fields.ymltest/packages/build_mode/bad_built_stream_package/data_stream/events/manifest.ymltest/packages/build_mode/bad_built_stream_package/docs/README.mdtest/packages/build_mode/bad_built_stream_package/manifest.ymltest/packages/build_mode/bad_built_with_dev/LICENSE.txttest/packages/build_mode/bad_built_with_dev/_dev/build/build.ymltest/packages/build_mode/bad_built_with_dev/changelog.ymltest/packages/build_mode/bad_built_with_dev/data_stream/events/agent/stream/stream.yml.hbstest/packages/build_mode/bad_built_with_dev/data_stream/events/fields/base-fields.ymltest/packages/build_mode/bad_built_with_dev/data_stream/events/fields/fields.ymltest/packages/build_mode/bad_built_with_dev/data_stream/events/manifest.ymltest/packages/build_mode/bad_built_with_dev/docs/README.mdtest/packages/build_mode/bad_built_with_dev/manifest.ymltest/packages/build_mode/bad_built_with_link/LICENSE.txttest/packages/build_mode/bad_built_with_link/changelog.ymltest/packages/build_mode/bad_built_with_link/data_stream/events/agent/stream/stream.yml.hbstest/packages/build_mode/bad_built_with_link/data_stream/events/fields/base-fields.ymltest/packages/build_mode/bad_built_with_link/data_stream/events/fields/base-fields.yml.linktest/packages/build_mode/bad_built_with_link/data_stream/events/fields/fields.ymltest/packages/build_mode/bad_built_with_link/data_stream/events/manifest.ymltest/packages/build_mode/bad_built_with_link/docs/README.mdtest/packages/build_mode/bad_built_with_link/manifest.ymltest/packages/build_mode/good_built/LICENSE.txttest/packages/build_mode/good_built/changelog.ymltest/packages/build_mode/good_built/data_stream/events/agent/stream/stream.yml.hbstest/packages/build_mode/good_built/data_stream/events/fields/base-fields.ymltest/packages/build_mode/good_built/data_stream/events/fields/fields.ymltest/packages/build_mode/good_built/data_stream/events/manifest.ymltest/packages/build_mode/good_built/docs/README.mdtest/packages/build_mode/good_built/manifest.ymltest/packages/good_requires/data_stream/logs/manifest.ymltest/packages/good_v3/data_stream/ecs_import_mappings/manifest.yml
Supplementing
left a comment
Supplementing
left a comment
Contributor
There was a problem hiding this comment.
Just a few clarification questions/comments, but overall it looks good to me and seems to meet the criteria we decided on as a team 🙂
|
|
||
| // Validation modes. The public API re-exports these as validator.Mode* constants. | ||
| const ( | ||
| Legacy Mode = "legacy" |
Contributor
There was a problem hiding this comment.
We originally discussed that legacy was functionally equivalent to source mode, but then there was some additional discussion about maybe keeping it for some time until we deprecate it and it collapses into source by default. Just wanted to see what the plan was here and if we really even need legacy at all if theyre functionally eq.
Contributor
Author
There was a problem hiding this comment.
I've kept it for the shake of migrating. I felt it was safer to have all options available when using it on elastic-package. We could have a followup cleanup issue to remove it.
Comment on lines
+33
to
+36
| // Keys starting with "_embedded_ecs" are auto-injected by elastic-package at build time | ||
| // when import_mappings is enabled. They must not appear in source packages and are | ||
| // rejected in source validation mode. Built packages (where these keys are expected) | ||
| // should be validated with ModeBuild, not ModeSource. |
Contributor
There was a problem hiding this comment.
This is a general question due to my own lack of understanding, in Jaime's comment here he mentions that we should treat embedded_ecs as a one off as its not usual, so is this something we want to validate against?
Contributor
Author
There was a problem hiding this comment.
i understood it was some validation needed for the build bundle... so that is why i addressed as a build validation rule. @jsoriano perhaps i have not understood correctly what you meant with treating it as "one off" ? should this not be validated?
| // If mode is empty, it defaults to modes.Legacy. | ||
| func NewSpec(version semver.Version, mode modes.Mode) (*Spec, error) { | ||
| if mode == "" { | ||
| mode = modes.Legacy |
Contributor
There was a problem hiding this comment.
Depending on the answer above about whether we need legacy or not (im unbiased here FWIW), this would default to source.
Also, looks like Coderabbit called it out below, but adding a guard clause against invalid modes would be good. In case someone passes 'src' or something not supported.
Contributor
Author
There was a problem hiding this comment.
I accepted to have the Legacy as default to reinforce the idea that the validators in the "new" form should explicitly pass source or build.
yup, i still have to address 🐰 feedback. thanks!
- Added validation for the mode in NewSpec to ensure it is valid, returning an error for invalid modes. - Improved error handling in validatePolicyTemplateInputsMaterialized to return structured errors when reading the manifest fails, while still allowing for the case where the manifest does not exist. - Updated the error handling in the Validator's Validate method to join multiple errors when closing the closer fails. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Rename abbreviated loop variables (dsName -> dataStreamName, pt -> policyTemplate, tc -> testCase) - Use errors.As instead of direct type assertion for wrapped ValidationErrors - Check Close() errors for zip writer and output file in tests - Fix bad_built_* fixture READMEs that incorrectly described themselves as valid packages Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
code/go/pkg/validator/api_test.go (1)
76-79:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse
errors.Ashere too, mirroringextractMessages.This direct type assertion fails on wrapped errors (the same
errorlintfinding already fixed at Line 54). IfValidate()ever returns a wrappedValidationErrors, the filter is silently skipped and the original error is returned.🔧 Proposed fix
- verrs, ok := err.(specerrors.ValidationErrors) - if !ok { + var verrs specerrors.ValidationErrors + if !errors.As(err, &verrs) { return err }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@code/go/pkg/validator/api_test.go` around lines 76 - 79, Replace the direct type assertion on err with errors.As so wrapped specerrors.ValidationErrors are detected the same way as in extractMessages; specifically, change the code that currently does "verrs, ok := err.(specerrors.ValidationErrors)" to use errors.As to populate verrs (and ensure the "errors" package is imported), then proceed with the same return behavior when errors.As fails—this mirrors the fix at Line 54 and ensures Validate() wrapped errors are handled correctly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@code/go/internal/validator/semantic/validate_stream_input_materialized.go`:
- Around line 131-133: The validation error uses the bare manifestRelPath
instead of the package-relative path helper; update the
specerrors.NewStructuredErrorf call to wrap the path with
fsys.Path(manifestRelPath) so the structured error reports the full
package-relative path (use fsys.Path on the manifestRelPath argument passed into
specerrors.NewStructuredErrorf).
---
Duplicate comments:
In `@code/go/pkg/validator/api_test.go`:
- Around line 76-79: Replace the direct type assertion on err with errors.As so
wrapped specerrors.ValidationErrors are detected the same way as in
extractMessages; specifically, change the code that currently does "verrs, ok :=
err.(specerrors.ValidationErrors)" to use errors.As to populate verrs (and
ensure the "errors" package is imported), then proceed with the same return
behavior when errors.As fails—this mirrors the fix at Line 54 and ensures
Validate() wrapped errors are handled correctly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 8eee3ca7-4c0b-4024-8c12-d4d4b0d17379
📒 Files selected for processing (10)
code/go/internal/validator/semantic/validate_stream_input_materialized.gocode/go/internal/validator/semantic/validate_stream_input_materialized_test.gocode/go/internal/validator/spec.gocode/go/pkg/validator/api.gocode/go/pkg/validator/api_test.gotest/packages/build_mode/bad_built_external_ecs/docs/README.mdtest/packages/build_mode/bad_built_policy_template_package/docs/README.mdtest/packages/build_mode/bad_built_stream_package/docs/README.mdtest/packages/build_mode/bad_built_with_dev/docs/README.mdtest/packages/build_mode/bad_built_with_link/docs/README.md
Replace bare manifestRelPath with fsys.Path(manifestRelPath) in the read-error branch so structured errors report the full package-relative path, consistent with the parse-error branch below it. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
💚 Build Succeeded
History
|
jsoriano
reviewed
May 29, 2026
jsoriano
left a comment
jsoriano
left a comment
Contributor
There was a problem hiding this comment.
Could we split this change in two, or more? At least one only with the new validation API, and then another one, or more, for the new semantic validations.
Let's confirm first the new API, and then we can eventually introduce the changes in the validations.
Contributor
Author
|
closing this in favor of:
cc @jsoriano |
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
4 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.
What does this PR do?
Introduces three explicit validation modes —
legacy,source, andbuild— so that source packages and built packages are validated against distinct rule sets.New public API (
pkg/validator):NewFromPath(mode, path),NewFromFS(mode, location, fsys)— mode-aware constructors with eager path/FS validation at construction time (clear errors rather than a confusing failure deep insideValidate())NewFromZip(path)— alwaysModeBuild; zip files are by definition built packagesValidateFromPath,ValidateFromZip,ValidateFromFSpreserved as deprecated wrappers usingModeLegacySource mode rejects
_embedded_ecskeys indynamic_templates(resolves the long-standing TODO atmanifest.spec.yml:350) and skips build-mode-specific checks.Build mode rejects source-only artifacts:
_dev/directory.linkfileexternal: ecsstreams[]withpackage:or missinginput:package:instead oftype:The
legacymode reproduces today's behaviour byte-for-byte and is what all existing callers get through the deprecated wrappers.Composable package support (source mode):
Four false-positive categories that fired when linting composable integration packages (ones that reference input packages via
package:) are now fixed:package:but declare no localtemplate_path/template_pathsskip template-existence checks in source mode. Templates that come from the dependency are not present in source and only materialise at build time. Streams and inputs that do declare explicit overlaytemplate_pathsstill have those files validated.package:set) and composable policy template inputs (package:set) are treated as partial overrides of vars defined in the referenced input package.name,type, and type-specific constraints (options,secret, duration patterns) are not required in source — they are assigned or enforced by elastic-package when it materialises the composable reference. Non-composable streams/inputs (input:/type:set) retain full var validation.ValidateIntegrationInputQualifiernow runs in build mode only. In source, composable inputs carrypackage:instead oftype:, leavinginput.Type == ""for every composable input; when a policy template has multiple such inputs SVR00010 fired spuriously. In build modeValidateStreamInputMaterializedhas already replaced everypackage:with a realtype:, so the name-uniqueness check is meaningful.Why is it important?
The validator today applies a union of all rules regardless of whether a package is a checked-out source tree or a built artifact ready for distribution. This lets malformed source packages build without error and malformed built packages reach the registry undetected. Closes #549.
Checklist
test/packagesthat prove my change is effective.spec/changelog.yml.Things to review
internal/validator/spec.go→rules()): rules withmodes: nilrun in all three modes; rules tagged with specific modes are filtered out. Confirm the tagging onValidateExternalFieldsWithDevFolderandValidateTestPackageRequirements(source+legacy only), andValidateIntegrationInputQualifier(build only) is correct.NewFromZipalwaysModeBuild: this is a new function — no existing callers break. The design decision is thatNewFromZipenforcesModeBuildat the API level rather than leaving it to the caller. The deprecatedValidateFromZipstays onModeLegacyvia an internal helper.NewFromFS(ModeSource): wraps withlinkedfiles.NewFS(location, fsys)so.linkfiles are resolved transparently. Callers passing an in-memory FS should be aware that link resolution hits the real OS filesystem atlocation.good_v3/data_stream/ecs_import_mappings/manifest.yml: the large deletion (466 lines) removes_embedded_ecs-*dynamic template entries that were build artifacts committed into a source fixture. They now live intest/packages/bad_embedded_ecs/which is the fixture that proves source mode rejects them. Theecs_import_mappingsdata stream is left with ordinarydynamic_templatesentries.good_builtfixture (test/packages/build_mode/good_built/): hand-crafted to represent a valid built package. It is the reference for all build-mode passing tests; if it is wrong, the build-mode rules will be miscalibrated.data_stream/manifest.spec.yml+integration/manifest.spec.yml): theif: required: [input]/if: required: [type]guards on stream/input items are the mechanism that switches between permissive (composable) and strict (non-composable) var validation. The fourallOfconditions in#/definitions/vars/itemsnow includerequired: [type]guards to prevent spurious cascades whentypeis absent.How to test
Automated: all existing tests pass against the deprecated
ValidateFrom*wrappers, which useModeLegacyand preserve byte-for-byte identical behaviour withmain.Manual:
elastic-packagewas run with this package-spec pinned against the full integrations repository. All integrations validated and built successfully — no breaking changes to existing callers.A follow-up PR to
elastic-packagewill migrate itscheck/lintcommands toModeSourceand itsbuildcommand toModeBuildusing the new constructor API.Related issues
Summary by CodeRabbit
New Features
_devfolders,.linkfiles, and require stream input materialization.Bug Fixes
Tests
Chores