Validation rules: build/source mode enforcement#1178
Merged
teresaromero merged 51 commits intoJun 18, 2026
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>
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>
…dfiles.NewFS Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
… handling Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
NewFromZip no longer accepts a mode parameter (always ModeBuild); update the test to use the new signature as a constructor smoke test. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Documented the addition of mode-aware constructors and validation APIs. - Updated changelog to reflect enhancements made in the package specification.
…omZip - NewFromPath/NewFromFS now reject invalid modes at construction time - NewFromFS ModeLegacy preserves a pre-wrapped *linkedfiles.FS (matching old validateFromFS behaviour); ModeBuild always applies BlockFS unconditionally - Collapse private newFromZip into NewFromZip; ValidateFromZip delegates to NewFromZip directly, consistent with the other deprecated wrappers - Replace tautological TestLegacyPreservation_* tests with focused tests for the new API behaviours: invalid mode rejection, ModeBuild link blocking, ModeLegacy/ModeSource linked-FS preservation - Add good_v3 to TestValidateWarnings to exercise the link-resolution path Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
NewFromZip owns the zip reader via closer; without calling Validate() the reader stays open and t.TempDir cleanup fails on Windows with "file is being used by another process". Call Validate() to close the owned handle. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughImplements distinct source and build validation modes by adding three new semantic validators ( ChangesSource and build validation modes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant Spec
participant FolderValidator
participant SemanticRules
Caller->>Spec: ValidatePackage(fsys, mode)
Spec->>FolderValidator: newValidator(mode, spec, fsys)
FolderValidator->>FolderValidator: Validate() — rejects .link and validationMode:source dirs in BuildMode
Spec->>SemanticRules: run mode-gated rules
note over SemanticRules: SourceMode: ValidateNoEmbeddedEcsInDynamicTemplates<br/>BuildMode: ValidateNoExternalFields, ValidateStreamInputBundled
SemanticRules-->>Spec: ValidationErrors
FolderValidator-->>Spec: ValidationErrors
Spec-->>Caller: combined ValidationErrors
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
2 tasks
- Removed the Option parameter from NewFromPath, NewFromZip, and NewFromFS constructors, simplifying their signatures. - Updated internal buildValidator function to reflect the changes, eliminating the need for options. - Adjusted tests to align with the new constructor signatures, ensuring proper validation behavior without options.
- Enhanced godoc comments for all public validator functions (ValidateFromPath, ValidateFromBuildPath, ValidateFromSourcePath, ValidateFromZip) with clearer descriptions of their purpose, which specification they use, and linked file handling - Fixed typo: "appropiate" → "appropriate" in validateFromFS comment - Updated modes documentation to accurately describe the three validation modes (Legacy, Source, Build) instead of referencing non-existent public re-exports - Added deprecation guidance directing users to mode-specific functions Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…inksBehaviorAcrossModes
…cking - Mode becomes a first-class public type (not a string alias) carrying both the internal rule set and a wrapFS factory for path-based construction; external callers use ModeLegacy, ModeSource, ModeBuild only - NewFromPath delegates FS setup entirely to mode.wrapFS, removing the switch-on-mode logic from constructors - NewFromFS is now a "bring your own FS" constructor: mode selects validation rules only, no link-file wrapping is applied; callers are responsible for FS semantics (fixes the silent bypass where a *linkedfiles.FS passed to build mode would resolve instead of block links) - NewFromZip always runs ModeBuild with an explicit BlockFS; its zip reader is owned by the Validator and closed by Validate() with errors.Join (fixes the dropped r.Close() error previously flagged by errcheck) - Expose internal newSpec as NewSpec(version, mode) so Validator.Validate() can pass the stored mode.internal directly without a switch - ValidateFromPath and ValidateFromZip remain as zero-param thin wrappers; ValidateFromBuildPath and ValidateFromSourcePath are removed (replaced by NewFromPath(ModeBuild/ModeSource, ...)) - WithWarningsAsErrors Option overrides PACKAGE_SPEC_WARNINGS_AS_ERRORS env var Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Give each Mode constant its own godoc line so go doc can attach them - Describe Mode in terms of what it controls, not just what it is - Remove ModeLegacy anchor to deprecated ValidateFromPath function - Add rule-gating dimension to ModeSource and ModeBuild docs - Clarify WithWarningsAsErrors overrides the env var in both directions - Note error conditions on NewFromPath, NewFromFS, and NewSpec - Warn NewFromFS callers to wrap with NewBlockFS when using ModeBuild - Move single-call restriction note from Validate to NewFromZip only - Replace undefined term "build specification" with ModeBuild in ValidateFromZip - Expand ValidatePackage doc to mention both syntactic and semantic rules Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
teresaromero
force-pushed
the
549-validation-semantic
branch
from
193359e to
27f57d7
Compare
teresaromero
force-pushed
the
549-validation-semantic
branch
from
27f57d7 to
6f8a1e8
Compare
- Introduced NewValidator function to replace NewFromPath and NewFromFS, simplifying the creation of Validator instances. - Updated validation methods to use ValidateFromPath and ValidateFromFS, enhancing clarity and consistency. - Removed deprecated functions and unnecessary wrapping logic, ensuring better adherence to mode-specific validation rules. - Added tests for link file behavior across validation modes to ensure expected functionality. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Deleted the modes.go file to streamline the codebase. - Moved mode type definitions and constants directly into validator.go for better organization and accessibility.
teresaromero
force-pushed
the
549-validation-semantic
branch
from
6f8a1e8 to
8ccac7b
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/spec.go`:
- Around line 262-268: The five new mode-gated validators
(ValidateNoEmbeddedEcsInDynamicTemplates, ValidateNoDevFolder,
ValidateNoLinkFiles, ValidateNoExternalEcs, and ValidateStreamInputMaterialized)
are missing version constraints that should be present for consistency with
other mode-gated validators in the same section. Add `since:
semver.MustParse("3.6.0")` to each of these five validator definitions, as
source and build modes were introduced in version 3.6.0 and this matches the
pattern used in the preceding mode-gated validators.
In `@code/go/pkg/validator/validator_test.go`:
- Around line 1563-1593: Refactor the test table in TestBuildModeValidation to
follow the validator_test.go file conventions. Convert the tests variable from a
[]struct to a map[string]struct, rename the struct fields from the abbreviated
names (pkg, expectError, errorMustContain) to full names (invalidPkgFilePath and
expectedErrContains respectively), and update the for loop to iterate over the
map entries using a key and value pattern instead of the current index-based
iteration. Additionally, ensure all variable names throughout the test use full
names instead of abbreviations, replacing tc with the actual struct value
reference throughout the test body.
In `@test/packages/bad_embedded_ecs/changelog.yml`:
- Line 5: Replace placeholder GitHub PR URLs in test fixture changelog files
with `TBD` as per guidelines, since the actual PR numbers are not yet finalized.
In test/packages/bad_embedded_ecs/changelog.yml at line 5, replace the URL
`https://github.com/elastic/package-spec/pull/1` with the value `TBD`. In
test/packages/build_mode/bad_built_external_ecs/changelog.yml at line 6, replace
the URL `https://github.com/elastic/integrations/pull/1` with `TBD` and correct
the repository reference from `elastic/integrations` to `elastic/package-spec`.
In `@test/packages/build_mode/bad_built_missing_input/changelog.yml`:
- Line 4: The changelog description in the YAML file is missing a terminal
period, which violates the coding guideline requiring complete sentences. Add a
period to the end of the description value "Initial draft of the package" so it
reads "Initial draft of the package." to make it a complete sentence.
🪄 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: 3c693d54-067c-48b5-8264-025a6f9bf4c2
📒 Files selected for processing (79)
code/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/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_fs_artifacts/LICENSE.txttest/packages/build_mode/bad_built_fs_artifacts/_dev/build/build.ymltest/packages/build_mode/bad_built_fs_artifacts/changelog.ymltest/packages/build_mode/bad_built_fs_artifacts/data_stream/events/agent/stream/stream.yml.hbstest/packages/build_mode/bad_built_fs_artifacts/data_stream/events/fields/base-fields.ymltest/packages/build_mode/bad_built_fs_artifacts/data_stream/events/fields/base-fields.yml.linktest/packages/build_mode/bad_built_fs_artifacts/data_stream/events/fields/fields.ymltest/packages/build_mode/bad_built_fs_artifacts/data_stream/events/manifest.ymltest/packages/build_mode/bad_built_fs_artifacts/docs/README.mdtest/packages/build_mode/bad_built_fs_artifacts/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/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
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
jsoriano
previously requested changes
Jun 16, 2026
jsoriano
left a comment
jsoriano
left a comment
Contributor
There was a problem hiding this comment.
Adding some suggestions and requesting some small changes.
| for _, dataStream := range dataStreams { | ||
| if dataStream.IsDir() { | ||
| list = append(list, dataStream.Name()) | ||
| } |
Contributor
There was a problem hiding this comment.
What is the motivation of this change? Can we have anything under dataStreamDir that is not a directory?
Contributor
Author
There was a problem hiding this comment.
by the spec definition there are only directories on the datastream dir, but, fs.ReadDir is gathering directories and files, so i thought it was interesting to add this guard at this layer to ensure it. based on the docs https://pkg.go.dev/io/fs#DirEntry the direntry interface could be one or the other...
Comment on lines
+114
to
+121
| // Composable inputs reference an input package via 'package:'. When no | ||
| // explicit template_path/template_paths is set, all templates come from | ||
| // the dependency and are only present after build. Skip those. | ||
| // If the composable input defines its own template_path or template_paths | ||
| // (overlay templates that live in the source package), those are validated. | ||
| if input.Package != "" && input.TemplatePath == "" && len(input.TemplatePaths) == 0 { | ||
| continue | ||
| } |
Contributor
There was a problem hiding this comment.
Is this exception needed?
I see two options for that:
- We don't validate template paths on source packages. Then this validator is never run with
input.Package != "". - We validate template paths on source packages. Then the value of
input.Packageshould not matter.
Also, unit tests pass without this code.
Suggested change
| // Composable inputs reference an input package via 'package:'. When no | |
| // explicit template_path/template_paths is set, all templates come from | |
| // the dependency and are only present after build. Skip those. | |
| // If the composable input defines its own template_path or template_paths | |
| // (overlay templates that live in the source package), those are validated. | |
| if input.Package != "" && input.TemplatePath == "" && len(input.TemplatePaths) == 0 { | |
| continue | |
| } |
Contributor
Author
There was a problem hiding this comment.
i removed the check. i prefer the validation runs on source and build. source is run on elastic-package lint - build - install; if there is any missing file we will notice before getting to the bundle.
Comment on lines
+153
to
+163
| // Composable streams reference an input package via 'package:'. When | ||
| // no explicit template_path/template_paths is set on the stream, all | ||
| // templates come from the dependency and are only present after build. | ||
| // Skip those — ValidateStreamInputMaterialized enforces that 'package:' | ||
| // is replaced by 'input:' in build mode. | ||
| // However, if the composable stream defines its own template_path or | ||
| // template_paths, those files must exist in the source package and are | ||
| // validated here. | ||
| if s.Package != "" && s.TemplatePath == "" && len(s.TemplatePaths) == 0 { | ||
| continue | ||
| } |
Contributor
There was a problem hiding this comment.
In this case I guess we need to maintain this condition if we want to validate templates at source packages. This case is different because a default template is expected if none is defined, and for composable packages it would be fine to don't have one in the source.
Contributor
Author
There was a problem hiding this comment.
yes, i am keeping this as the source validation is mantained
Comment on lines
+153
to
+160
| // Composable streams reference an input package via 'package:'. When | ||
| // no explicit template_path/template_paths is set on the stream, all | ||
| // templates come from the dependency and are only present after build. | ||
| // Skip those — ValidateStreamInputMaterialized enforces that 'package:' | ||
| // is replaced by 'input:' in build mode. | ||
| // However, if the composable stream defines its own template_path or | ||
| // template_paths, those files must exist in the source package and are | ||
| // validated here. |
Contributor
There was a problem hiding this comment.
Nit. A bit too verbose.
Suggested change
| // Composable streams reference an input package via 'package:'. When | |
| // no explicit template_path/template_paths is set on the stream, all | |
| // templates come from the dependency and are only present after build. | |
| // Skip those — ValidateStreamInputMaterialized enforces that 'package:' | |
| // is replaced by 'input:' in build mode. | |
| // However, if the composable stream defines its own template_path or | |
| // template_paths, those files must exist in the source package and are | |
| // validated here. | |
| // Don't validate template paths if they use a package as input | |
| // and they don't define any template. |
| // _dev/ is a source-only artifact used during development (tests, deploy | ||
| // configs, build manifests). It must not appear in a built package that is | ||
| // validated with ModeBuild. | ||
| func ValidateNoDevFolder(fsys fspath.FS) specerrors.ValidationErrors { |
Contributor
There was a problem hiding this comment.
There would be any way to define this in the spec?
Maybe folder spec should be also aware of the validation mode, and it should not accept files marked with some "source" marker.
And we could reuse it for link files, or other files that should be there only in source packages.
Contributor
Author
There was a problem hiding this comment.
implemented this from the spec, both dev and .link files are checked based on mode on the spec. i was suggested to use the visibility: private setting which was shared with all _dev folder placements, but i thought this might have more semantical use so i created a sourceOnly boolean property for this case... should we use visbility field instead? i have mixed feelings as i think visibility could mean a broader scope...
| // have 'package:' (composable-input pattern, source-only). | ||
| // - manifest.yml: each policy_template input must have 'type:' set and must NOT | ||
| // have 'package:' (package-reference pattern, source-only). | ||
| func ValidateStreamInputMaterialized(fsys fspath.FS) specerrors.ValidationErrors { |
Contributor
There was a problem hiding this comment.
Nit. I think we call it "Bundled" in other places.
Suggested change
| func ValidateStreamInputMaterialized(fsys fspath.FS) specerrors.ValidationErrors { | |
| func ValidateStreamInputBundled(fsys fspath.FS) specerrors.ValidationErrors { |
| } | ||
|
|
||
| // ValidateStreamInputMaterialized errors when build-mode manifests carry | ||
| // source-only 'package:' fields that the build process should have materialized: |
Contributor
There was a problem hiding this comment.
It would be also great if we could model this in the spec.
Contributor
Author
There was a problem hiding this comment.
same as for ecs fields, this will followup on the field-mode awarenes
Comment on lines
+99
to
+115
| if s.Package != "" { | ||
| errs = append(errs, specerrors.NewStructuredErrorf( | ||
| "file %q: stream[%d] has 'package:' which is source-only; build packages must use 'input:' + 'template_paths:'", | ||
| fullPath, i, | ||
| )) | ||
| // Skip the 'input:' check for this stream: the root cause is the | ||
| // presence of 'package:', not a forgotten 'input:'. Emitting both | ||
| // errors would be misleading — the user intended the composable-input | ||
| // pattern and needs to materialise it, not simply add 'input:'. | ||
| continue | ||
| } | ||
| if s.Input == "" { | ||
| errs = append(errs, specerrors.NewStructuredErrorf( | ||
| "file %q: stream[%d] missing required 'input:' field", | ||
| fullPath, i, | ||
| )) | ||
| } |
Contributor
There was a problem hiding this comment.
This condition is already checked by the spec right? It checks that either package or input should be set.
I think this validator should only check that package is not defined in built packages. The rest is modeled in the spec.
Contributor
Author
There was a problem hiding this comment.
yes, you are right. i've removed the condition so the bundled is only checked for package, if there is no package, by spec its forced to have input. this will be not needed if we implement mode-aware fields. i will add it to the follow up issue
Comment on lines
+16
to
+18
| func ValidateNoExternalEcs(fsys fspath.FS) specerrors.ValidationErrors { | ||
| validateFunc := func(metadata fieldFileMetadata, f field) specerrors.ValidationErrors { | ||
| if f.External != "ecs" { |
Contributor
There was a problem hiding this comment.
No external field should be allowed on built packages.
Suggested change
| func ValidateNoExternalEcs(fsys fspath.FS) specerrors.ValidationErrors { | |
| validateFunc := func(metadata fieldFileMetadata, f field) specerrors.ValidationErrors { | |
| if f.External != "ecs" { | |
| func ValidateNoExternalFields(fsys fspath.FS) specerrors.ValidationErrors { | |
| validateFunc := func(metadata fieldFileMetadata, f field) specerrors.ValidationErrors { | |
| if f.External == "" { |
| t.Parallel() | ||
| // Use the built fixture: source packages have _dev/ and external:ecs which | ||
| // build mode rejects. good_built is the correct built-package counterpart. | ||
| builtPkg := filepath.Join("..", "..", "..", "..", "test", "packages", "build_mode", "good_built") |
Contributor
There was a problem hiding this comment.
Nit. Maybe place these built packages under a different directory.
Suggested change
| builtPkg := filepath.Join("..", "..", "..", "..", "test", "packages", "build_mode", "good_built") | |
| builtPkg := filepath.Join("..", "..", "..", "..", "test", "built_packages", "good_built") |
…Inputs The guard was checking input.Package != "" with no templates to skip validation, but the downstream checks already only validate when template_path or template_paths are explicitly set — so the guard was a no-op. Replace it with a comment explaining the implicit behaviour. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…built fixtures - Rename ValidateStreamInputMaterialized → ValidateStreamInputBundled (and internal helpers) to match the "bundled" terminology used elsewhere; rename the file and test file accordingly; update call site in spec.go. - Simplify the verbose comment in validateAllDataStreamStreamTemplates to the two-line form suggested by the reviewer. - Move test/packages/build_mode/ fixtures to test/built_packages/ to separate built-package fixtures from source-mode fixtures; update both path references in validator_test.go. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…packages Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Replace ValidateNoDevFolder (a recursive filesystem walk) with a spec-level check driven by the new sourceOnly: true annotation. - Add SourceOnly() bool to the ItemSpec interface - Add sourceOnly field to folderItemSpec (distinct from visibility:private, which carries a separate registry/catalog connotation) - Thread Mode into the folder validator; reject sourceOnly items in BuildMode - Set sourceOnly: true on all four _dev/ entries in integration, content, input, and data_stream specs - Remove ValidateNoDevFolder and its unit test - Extend bad_built_fs_artifacts fixture to cover data_stream/_dev Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…semantic walk Replace ValidateNoLinkFiles (a recursive filesystem walk) with a check in the folder validator's main loop, reusing the existing checkLink helper. - In Validate(), reject .link files immediately in BuildMode before findItemSpec - Simplify checkLink to use strings.CutSuffix - Remove ValidateNoLinkFiles and its unit test - Update build_rejects_link_files test: error now comes from the folder validator rather than BlockFS.Open Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
code/go/internal/validator/semantic/validate_stream_input_bundled.go (1)
98-111: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winUse a full variable name for stream entries.
Line 98 uses
s, which violates the full-name naming rule for new Go code.Suggested fix
- for i, s := range manifest.Streams { - if s.Package != "" { + for i, streamEntry := range manifest.Streams { + if streamEntry.Package != "" { errs = append(errs, specerrors.NewStructuredErrorf( "file %q: stream[%d] has 'package:' which is source-only; build packages must use 'input:' + 'template_paths:'", fullPath, i, )) // Skip the 'input:' check for this stream: the root cause is the // presence of 'package:', not a forgotten 'input:'. Emitting both // errors would be misleading — the user intended the composable-input // pattern and needs to materialise it, not simply add 'input:'. continue } - if s.Input == "" { + if streamEntry.Input == "" { errs = append(errs, specerrors.NewStructuredErrorf( "file %q: stream[%d] missing required 'input:' field", fullPath, i, )) } }As per coding guidelines,
code/go/**/*.gorequires full variable/function names instead of abbreviations (excepti,j,err,ok).🤖 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/internal/validator/semantic/validate_stream_input_bundled.go` around lines 98 - 111, The loop variable `s` in the for loop that iterates over manifest.Streams violates the naming conventions which require full variable names instead of abbreviations. Replace the abbreviated variable name `s` with a full descriptive name like `stream` throughout this code block, including in the range clause and all subsequent references to that variable (such as `s.Package`, `s.Input`, and any other uses within the loop body).Source: Coding guidelines
🤖 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.
Outside diff comments:
In `@code/go/internal/validator/semantic/validate_stream_input_bundled.go`:
- Around line 98-111: The loop variable `s` in the for loop that iterates over
manifest.Streams violates the naming conventions which require full variable
names instead of abbreviations. Replace the abbreviated variable name `s` with a
full descriptive name like `stream` throughout this code block, including in the
range clause and all subsequent references to that variable (such as
`s.Package`, `s.Input`, and any other uses within the loop body).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 46f8bd91-658e-4973-a06c-29be0df06ffa
📒 Files selected for processing (65)
code/go/internal/specschema/folder_item_spec.gocode/go/internal/spectypes/item.gocode/go/internal/validator/folder_spec.gocode/go/internal/validator/semantic/validate_integration_policy_template_path.gocode/go/internal/validator/semantic/validate_no_external_fields.gocode/go/internal/validator/semantic/validate_no_external_fields_test.gocode/go/internal/validator/semantic/validate_stream_input_bundled.gocode/go/internal/validator/semantic/validate_stream_input_bundled_test.gocode/go/internal/validator/spec.gocode/go/pkg/validator/validator_test.gospec/content/spec.ymlspec/input/spec.ymlspec/integration/data_stream/spec.ymlspec/integration/spec.ymltest/built_packages/bad_built_external_ecs/LICENSE.txttest/built_packages/bad_built_external_ecs/changelog.ymltest/built_packages/bad_built_external_ecs/data_stream/events/agent/stream/stream.yml.hbstest/built_packages/bad_built_external_ecs/data_stream/events/fields/base-fields.ymltest/built_packages/bad_built_external_ecs/data_stream/events/fields/fields.ymltest/built_packages/bad_built_external_ecs/data_stream/events/manifest.ymltest/built_packages/bad_built_external_ecs/docs/README.mdtest/built_packages/bad_built_external_ecs/manifest.ymltest/built_packages/bad_built_fs_artifacts/LICENSE.txttest/built_packages/bad_built_fs_artifacts/_dev/build/build.ymltest/built_packages/bad_built_fs_artifacts/changelog.ymltest/built_packages/bad_built_fs_artifacts/data_stream/events/_dev/test/.emptytest/built_packages/bad_built_fs_artifacts/data_stream/events/agent/stream/stream.yml.hbstest/built_packages/bad_built_fs_artifacts/data_stream/events/fields/base-fields.ymltest/built_packages/bad_built_fs_artifacts/data_stream/events/fields/base-fields.yml.linktest/built_packages/bad_built_fs_artifacts/data_stream/events/fields/fields.ymltest/built_packages/bad_built_fs_artifacts/data_stream/events/manifest.ymltest/built_packages/bad_built_fs_artifacts/docs/README.mdtest/built_packages/bad_built_fs_artifacts/manifest.ymltest/built_packages/bad_built_missing_input/LICENSE.txttest/built_packages/bad_built_missing_input/changelog.ymltest/built_packages/bad_built_missing_input/data_stream/events/agent/stream/stream.yml.hbstest/built_packages/bad_built_missing_input/data_stream/events/fields/base-fields.ymltest/built_packages/bad_built_missing_input/data_stream/events/fields/fields.ymltest/built_packages/bad_built_missing_input/data_stream/events/manifest.ymltest/built_packages/bad_built_missing_input/docs/README.mdtest/built_packages/bad_built_missing_input/manifest.ymltest/built_packages/bad_built_policy_template_package/LICENSE.txttest/built_packages/bad_built_policy_template_package/changelog.ymltest/built_packages/bad_built_policy_template_package/data_stream/events/agent/stream/stream.yml.hbstest/built_packages/bad_built_policy_template_package/data_stream/events/fields/base-fields.ymltest/built_packages/bad_built_policy_template_package/data_stream/events/fields/fields.ymltest/built_packages/bad_built_policy_template_package/data_stream/events/manifest.ymltest/built_packages/bad_built_policy_template_package/docs/README.mdtest/built_packages/bad_built_policy_template_package/manifest.ymltest/built_packages/bad_built_stream_package/LICENSE.txttest/built_packages/bad_built_stream_package/changelog.ymltest/built_packages/bad_built_stream_package/data_stream/events/agent/stream/stream.yml.hbstest/built_packages/bad_built_stream_package/data_stream/events/fields/base-fields.ymltest/built_packages/bad_built_stream_package/data_stream/events/fields/fields.ymltest/built_packages/bad_built_stream_package/data_stream/events/manifest.ymltest/built_packages/bad_built_stream_package/docs/README.mdtest/built_packages/bad_built_stream_package/manifest.ymltest/built_packages/good_built/LICENSE.txttest/built_packages/good_built/changelog.ymltest/built_packages/good_built/data_stream/events/agent/stream/stream.yml.hbstest/built_packages/good_built/data_stream/events/fields/base-fields.ymltest/built_packages/good_built/data_stream/events/fields/fields.ymltest/built_packages/good_built/data_stream/events/manifest.ymltest/built_packages/good_built/docs/README.mdtest/built_packages/good_built/manifest.yml
💤 Files with no reviewable changes (48)
- test/built_packages/bad_built_external_ecs/changelog.yml
- test/built_packages/bad_built_policy_template_package/changelog.yml
- test/built_packages/bad_built_external_ecs/docs/README.md
- test/built_packages/bad_built_stream_package/changelog.yml
- test/built_packages/bad_built_external_ecs/data_stream/events/fields/base-fields.yml
- test/built_packages/bad_built_fs_artifacts/LICENSE.txt
- test/built_packages/bad_built_external_ecs/data_stream/events/agent/stream/stream.yml.hbs
- test/built_packages/good_built/data_stream/events/agent/stream/stream.yml.hbs
- test/built_packages/bad_built_stream_package/data_stream/events/fields/base-fields.yml
- test/built_packages/bad_built_missing_input/docs/README.md
- test/built_packages/bad_built_external_ecs/data_stream/events/manifest.yml
- test/built_packages/bad_built_external_ecs/data_stream/events/fields/fields.yml
- test/built_packages/good_built/changelog.yml
- test/built_packages/bad_built_stream_package/data_stream/events/agent/stream/stream.yml.hbs
- test/built_packages/bad_built_fs_artifacts/docs/README.md
- test/built_packages/bad_built_missing_input/data_stream/events/manifest.yml
- test/built_packages/good_built/data_stream/events/manifest.yml
- test/built_packages/bad_built_policy_template_package/data_stream/events/manifest.yml
- test/built_packages/bad_built_stream_package/docs/README.md
- test/built_packages/bad_built_fs_artifacts/data_stream/events/fields/fields.yml
- test/built_packages/bad_built_policy_template_package/data_stream/events/agent/stream/stream.yml.hbs
- test/built_packages/good_built/manifest.yml
- test/built_packages/bad_built_policy_template_package/docs/README.md
- test/built_packages/bad_built_missing_input/data_stream/events/fields/fields.yml
- test/built_packages/bad_built_fs_artifacts/data_stream/events/agent/stream/stream.yml.hbs
- test/built_packages/good_built/data_stream/events/fields/base-fields.yml
- test/built_packages/bad_built_external_ecs/LICENSE.txt
- test/built_packages/bad_built_stream_package/data_stream/events/manifest.yml
- test/built_packages/good_built/LICENSE.txt
- test/built_packages/bad_built_fs_artifacts/data_stream/events/manifest.yml
- test/built_packages/good_built/data_stream/events/fields/fields.yml
- test/built_packages/bad_built_stream_package/LICENSE.txt
- test/built_packages/bad_built_policy_template_package/LICENSE.txt
- test/built_packages/good_built/docs/README.md
- test/built_packages/bad_built_stream_package/manifest.yml
- test/built_packages/bad_built_external_ecs/manifest.yml
- test/built_packages/bad_built_missing_input/changelog.yml
- test/built_packages/bad_built_fs_artifacts/data_stream/events/fields/base-fields.yml
- test/built_packages/bad_built_fs_artifacts/changelog.yml
- test/built_packages/bad_built_policy_template_package/manifest.yml
- test/built_packages/bad_built_missing_input/data_stream/events/agent/stream/stream.yml.hbs
- test/built_packages/bad_built_fs_artifacts/manifest.yml
- test/built_packages/bad_built_missing_input/manifest.yml
- test/built_packages/bad_built_missing_input/data_stream/events/fields/base-fields.yml
- test/built_packages/bad_built_policy_template_package/data_stream/events/fields/base-fields.yml
- test/built_packages/bad_built_policy_template_package/data_stream/events/fields/fields.yml
- test/built_packages/bad_built_stream_package/data_stream/events/fields/fields.yml
- test/built_packages/bad_built_missing_input/LICENSE.txt
The spec schema already enforces via oneOf that each stream must have either input: or package:, so the semantic check was duplicate. Tests updated to assert the schema-layer error message instead, and a new source-package fixture covers the same constraint end-to-end. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
code/go/internal/validator/semantic/validate_stream_input_bundled_test.go (1)
144-152: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winUse full variable names instead of abbreviations.
The variables
sbandsubstrviolate the naming guideline requiring full names incode/go/**/*.go.♻️ Proposed refactor
- var sb strings.Builder + var builder strings.Builder for _, e := range errs { - sb.WriteString(e.Error()) - sb.WriteString("\n") + builder.WriteString(e.Error()) + builder.WriteString("\n") } - combined := sb.String() - for _, substr := range testCase.errorContains { - assert.Contains(t, combined, substr) + combined := builder.String() + for _, expectedSubstring := range testCase.errorContains { + assert.Contains(t, combined, expectedSubstring) }As per coding guidelines, Go variable names in
code/go/**must use full names, not abbreviations (exception:i,j,err,ok).🤖 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/internal/validator/semantic/validate_stream_input_bundled_test.go` around lines 144 - 152, Replace the abbreviated variable names with full descriptive names according to the Go coding guidelines. In the string building loop, rename the variable `sb` to a full name like `stringBuilder`. Additionally, in the assertion loop that checks for error substrings, rename the loop variable `substr` to a full name like `substring`. These variable names must be updated consistently throughout the code block where they are used.Source: Coding guidelines
code/go/pkg/validator/validator_test.go (1)
1561-1604: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winAdd missing
invalidPkgFilePathfield to test table.The
TestBuildModeValidationtest table usesmap[string]structbut only includesexpectedErrContains. Per coding guidelines, validator_test.go test tables must include bothinvalidPkgFilePathandexpectedErrContainsfields for consistency with the file's existing test patterns.♻️ Proposed refactor
func TestBuildModeValidation(t *testing.T) { basePath := filepath.Join("..", "..", "..", "..", "test", "built_packages") tests := map[string]struct { + invalidPkgFilePath string expectedErrContains []string }{ - "good_built": {}, + "good_built": { + invalidPkgFilePath: "", + }, "bad_built_external_ecs": { + invalidPkgFilePath: "data_stream/events/fields/fields.yml", expectedErrContains: []string{"has external: ecs reference"}, }, "bad_built_missing_input": { + invalidPkgFilePath: "data_stream/events/manifest.yml", // Caught by schema oneOf (input|package), not the semantic layer. expectedErrContains: []string{"streams.0: input is required"}, }, "bad_built_stream_package": { + invalidPkgFilePath: "data_stream/events/manifest.yml", expectedErrContains: []string{"stream[0] has 'package:' which is source-only"}, }, "bad_built_policy_template_package": { + invalidPkgFilePath: "manifest.yml", expectedErrContains: []string{"input[0] has 'package:' which is source-only"}, }, "bad_built_fs_artifacts": { + invalidPkgFilePath: "_dev", expectedErrContains: []string{ "source-only folder is not allowed in built packages", ".link files are not allowed in built packages", }, }, }As per coding guidelines,
**/validator_test.gorequiresmap[string]structwithinvalidPkgFilePathandexpectedErrContainsfields.🤖 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/validator_test.go` around lines 1561 - 1604, The test table in the TestBuildModeValidation function only defines expectedErrContains in its struct but is missing the invalidPkgFilePath field. Add the invalidPkgFilePath field (as a string) to the struct definition within the tests map, and then populate this field for each test case with the appropriate package path values (they should correspond to the subdirectories in the basePath like "good_built", "bad_built_external_ecs", etc.). This will align with the consistent test pattern used throughout validator_test.go.Source: Coding guidelines
🤖 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.
Outside diff comments:
In `@code/go/internal/validator/semantic/validate_stream_input_bundled_test.go`:
- Around line 144-152: Replace the abbreviated variable names with full
descriptive names according to the Go coding guidelines. In the string building
loop, rename the variable `sb` to a full name like `stringBuilder`.
Additionally, in the assertion loop that checks for error substrings, rename the
loop variable `substr` to a full name like `substring`. These variable names
must be updated consistently throughout the code block where they are used.
In `@code/go/pkg/validator/validator_test.go`:
- Around line 1561-1604: The test table in the TestBuildModeValidation function
only defines expectedErrContains in its struct but is missing the
invalidPkgFilePath field. Add the invalidPkgFilePath field (as a string) to the
struct definition within the tests map, and then populate this field for each
test case with the appropriate package path values (they should correspond to
the subdirectories in the basePath like "good_built", "bad_built_external_ecs",
etc.). This will align with the consistent test pattern used throughout
validator_test.go.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 1e14c7e1-9c95-4623-b09e-5ab99e7530b1
📒 Files selected for processing (3)
code/go/internal/validator/semantic/validate_stream_input_bundled.gocode/go/internal/validator/semantic/validate_stream_input_bundled_test.gocode/go/pkg/validator/validator_test.go
💤 Files with no reviewable changes (1)
- code/go/internal/validator/semantic/validate_stream_input_bundled.go
Contributor
Author
|
Opening issue for a followup on fields mode-aware #1188 |
jsoriano
reviewed
Jun 18, 2026
Comment on lines
+151
to
+152
| // SourceOnly marks items that must not appear in built packages (build mode). | ||
| SourceOnly bool `json:"sourceOnly" yaml:"sourceOnly"` |
Contributor
There was a problem hiding this comment.
Great to see this working 👍 Thanks.
Could we make this more flexible and allow to set the mode the item should appear on? In case we need items that should only appear in built packages.
So instead of setting sourceOnly: true, we set validationMode: source.
Contributor
Author
There was a problem hiding this comment.
yup, makes sense. i was thinking on this while creating the issue for the followup, to declare the "specific mode" instead of booleans. i will update the pr with these change
| } | ||
| // Only validate template files that are explicitly declared; if none are set there | ||
| // is nothing to check (composable inputs without overlay templates source them from | ||
| // the dependency package, which is absent from the source tree). |
Contributor
There was a problem hiding this comment.
Thanks for making these comments more concise 👍
Replaces the boolean `sourceOnly` field with a `validationMode` string
("source", "build", or "" for both) so spec directives can be scoped to
a specific validation mode in either direction.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
code/go/internal/validator/folder_spec.go (1)
125-141:⚠️ Potential issue | 🟠 Major | ⚡ Quick winApply mode filtering before type branching and in required-item checks.
At Line 133,
itemForbiddenInModeis only enforced in the directory branch, so file items carryingvalidationModecan still pass in the wrong mode. Also, at Line 190, required-item validation does not skip mode-forbidden specs, which can produce false “missing required item” errors.Suggested fix
@@ - if file.IsDir() { + if itemForbiddenInMode(itemSpec, v.mode) { + errs = append(errs, specerrors.NewStructuredErrorf( + "file %q: %s-only %s is not allowed in %q packages", + v.pkg.Path(path.Join(v.folderPath, fileName)), + itemSpec.ValidationMode(), + itemSpec.Type(), + string(v.mode), + )) + continue + } + + if file.IsDir() { @@ // validate that required items in spec are all accounted for for _, itemSpec := range v.spec.Contents() { + if itemForbiddenInMode(itemSpec, v.mode) { + continue + } if !itemSpec.Required() { continue }Also applies to: 189-193
🤖 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/internal/validator/folder_spec.go` around lines 125 - 141, The mode validation check using itemForbiddenInMode is currently only applied within the directory branch (after the IsDir check), allowing file items with restrictive validation modes to bypass the check. Move the itemForbiddenInMode validation before the IsDir branching logic so it applies to all item types. Additionally, update the required-item validation logic around line 189-193 to skip specs that are forbidden in the current mode using itemForbiddenInMode, preventing false "missing required item" errors for mode-restricted items.
🤖 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.
Outside diff comments:
In `@code/go/internal/validator/folder_spec.go`:
- Around line 125-141: The mode validation check using itemForbiddenInMode is
currently only applied within the directory branch (after the IsDir check),
allowing file items with restrictive validation modes to bypass the check. Move
the itemForbiddenInMode validation before the IsDir branching logic so it
applies to all item types. Additionally, update the required-item validation
logic around line 189-193 to skip specs that are forbidden in the current mode
using itemForbiddenInMode, preventing false "missing required item" errors for
mode-restricted items.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: dfd033f2-8fd6-47c6-9ef3-bedf863ded48
📒 Files selected for processing (9)
code/go/internal/specschema/folder_item_spec.gocode/go/internal/spectypes/item.gocode/go/internal/validator/folder_item_spec.gocode/go/internal/validator/folder_spec.gocode/go/pkg/validator/validator_test.gospec/content/spec.ymlspec/input/spec.ymlspec/integration/data_stream/spec.ymlspec/integration/spec.yml
…t messages - Validate validationMode at spec-load time (alongside visibility) so invalid values are rejected immediately instead of silently misbehaving - Enforce validationMode for file items, not just directories - Use %s consistently for both mode and validationMode in error messages - Document the string-value coupling between validator.Mode and spectypes.ValidationMode* constants Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
💚 Build Succeeded
History
|
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
3 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?
Wires concrete semantic validation rules to the
source/build/legacymodes introduced in #1177.New build-mode rules — reject source artifacts that must not survive the build:
validationMode: sourcein spec YAML (folder validator)_dev/directory in build mode.linkcheck in folder validator.linkfile in build modeValidateNoExternalFieldsexternal: ecs(build mode)ValidateStreamInputBundledpackage:or missinginput:; policy template inputs withpackage:instead oftype:(build mode, integration type)New source-mode rule:
ValidateNoEmbeddedEcsInDynamicTemplates_embedded_ecskeys indynamic_templates(injected by the build tooling, must not appear in source; integration type)Re-scoped existing rules:
ValidateExternalFieldsWithDevFolderlegacy+sourceValidateIntegrationInputQualifierlegacy+buildValidateTestPackageRequirementslegacy+sourceComposable package false-positive fixes (source mode, entries that set
package:):ValidateIntegrationPolicyTemplates: skip template-existence check when no localtemplate_pathis declared andpackage:is set — the template comes from the dependency.spec/integration/manifest.spec.ymlandspec/integration/data_stream/manifest.spec.yml:varsbase schema relaxed toarray of objects; strict$refvalidation restored conditionally viaif/thenfor non-composable entries only.required: [type]guards toifblocks forpassword,select, anddurationvar conditionals — prevents vacuous matches whentypeis absent (composable vars that omittype).TODO: Allow _embedded_ecs only on built packagescomment in the schema.validationModespec field replaces the old booleansourceOnlyfield with a string ("source","build", or omitted for both modes), so spec directives can be scoped to either validation direction. All_dev/entries now declarevalidationMode: source.Why is it important?
elastic-packagevalidates both source packages (contain_dev/,.linkfiles,external: ecsreferences) and built packages (those artifacts are absent, build-injected ones appear instead). A unified rule set causes false positives in both directions. Composable packages (package:delegation to input packages) add a third dimension: templates and full var definitions only exist after the build materialises the dependency.Checklist
test/packagesthat prove my change is effective.spec/changelog.yml.Related issues
Summary by CodeRabbit
Release Notes
New Features
sourceandbuildvalidation modes, with mode-aware checks.package:-based streams/inputs skip template-path validation when notemplate_pathsare provided.Bug Fixes & Validations
_embedded_ecsdynamic template keys insourcemode.buildmode now blocks source-only filesystem artifacts (_dev,.link) and non-materialized/external ECS references.data_stream/*/directories are considered for stream validation.Documentation