Validation rules: build/source mode enforcement#1178
Draft
teresaromero wants to merge 29 commits into
Draft
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>
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Enterprise Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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
…port Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Remove the NOTE in NewFromZip claiming build-mode rules were not yet implemented — they are now. Fix the bad_built_missing_input README which was copied from good_built and described the wrong fixture. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Built packages do not contain .link files — they are resolved at build time. The suppression was guarding against a scenario that does not occur in practice; ValidateNoLinkFiles catches any .link file that does appear in a malformed built package. Remove the dead guard and the linkedfiles import it required. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
… and enhance test coverage
Wire TestBuildModeValidation against all seven packages under test/packages/build_mode/ (good_built + six bad_built_* fixtures) that existed without any test coverage. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
teresaromero
force-pushed
the
549-validation-semantic
branch
from
27f57d7 to
6f8a1e8
Compare
💚 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
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
elastic-packagevalidates both source packages (contain_dev/,.linkfiles,external: ecsreferences) and built packages (those artefacts are gone, build-injectedones 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.
New rules
Build mode only
ValidateNoDevFolder_dev/directoryValidateNoLinkFiles.linkfileelastic-package buildValidateNoExternalEcsexternal: ecsValidateStreamInputMaterializedpackage:or missinginput:; policy template inputs withpackage:instead oftype:package:is the source-side composability pointer; build time replaces it withinput:/type:Source mode only
ValidateNoEmbeddedEcsInDynamicTemplates_embedded_ecskeys indynamic_templatesRe-scoped existing rules
ValidateExternalFieldsWithDevFolderlegacy+source_dev/; build mode already rejects survivingexternal:fields viaValidateNoExternalEcsValidateIntegrationInputQualifier(SVR00010)buildonlypackage:with notype:until the dependency is materialisedValidateTestPackageRequirementslegacy+source_dev/deployconfig, which built packages don't haveComposable package false-positive fixes
Both fixes apply in source mode when a policy template input or data stream stream sets
package:.Template existence —
ValidateIntegrationPolicyTemplatesfailed when no localtemplate_path/template_pathswas declared, because the template comes from thedependency and isn't present at source time. Fix: skip the existence check when
package:is set and no explicit template path is declared. If the composable entry declares an
overlay template path it is still validated.
Partial var definitions — composable entries may declare only a partial var override
(e.g. a different default); the full definition lives in the input package. The schema
previously required all var fields on every entry. Fix: base
varsschema relaxed toaccept any array of objects; full validation is restored via
if/thenonly fornon-composable entries (
if: required: [input]on streams,if: required: [type]onpolicy template inputs).
Spec schema changes
Three changes in
spec/integration/manifest.spec.ymlandspec/integration/data_stream/manifest.spec.yml:Partial var schema —
varsbase schema changed from strict$refto permissive(
type: array, items: object); full validation applied conditionally viaif/thenasdescribed above.
required: [type]guards on type-specific var conditionals — theifblocks forpassword,select, anddurationvars usedproperties: { type: { const: … } }.In JSON Schema
propertiesonly constrains present fields, so the condition passedvacuously when
typewas absent, incorrectly triggering type-specificthenconstraints on composable vars that omit
type. Addingrequired: [type]to eachcondition fixes this.
_embedded_ecscomment — resolved the staleTODO: Allow this only on built packagesby documenting the split: source-side rejection is now handled byValidateNoEmbeddedEcsInDynamicTemplates; the schema permits it in built packages.