validator: introduce mode-aware constructors and validation API#1177
validator: introduce mode-aware constructors and validation API#1177teresaromero wants to merge 30 commits into
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>
|
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:
📝 WalkthroughWalkthroughAdds a validation mode system (legacy/source/build), exposes mode-aware public validator constructors and an instance-based Validate(), embeds mode into Spec with rule filtering by mode, updates tests to exercise mode behavior (including link-file handling), and records an enhancement in the changelog. ChangesValidation Mode System
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 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 |
- Documented the addition of mode-aware constructors and validation APIs. - Updated changelog to reflect enhancements made in the package specification.
![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
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/spec_test.go (1)
19-40: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winCover the new
NewSpecmode branches.This test only exercises the explicit legacy path. The new empty-mode fallback and invalid-mode error path introduced in
NewSpecare still untested.🤖 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/spec_test.go` around lines 19 - 40, Extend the TestNewSpec table to include two more cases for NewSpec: one calling NewSpec(*semver.MustParse(version), modes.Empty) (the zero/empty-mode fallback) and asserting it behaves like the legacy path (no error and returns *Spec), and another calling NewSpec(*semver.MustParse(version), modes.Mode(999)) (an invalid/unknown mode) and asserting it returns an error containing the invalid-mode message and nil spec; use the same pattern as the existing cases (require.NoError/IsType or require.Error/Contains/IsNil) and reference NewSpec and the modes value you pass to locate the logic under test.
🤖 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/pkg/validator/api_test.go`:
- Around line 183-192: TestLegacyPreservation_FromZip currently only calls
NewFromZip and doesn't verify legacy-preservation behavior; update the test to
call ValidateFromZip(zipPath) and also construct a legacy validator with
newFromZip(ModeLegacy, zipPath).Validate() (or call newFromZip(ModeLegacy,
zipPath) then .Validate()) and assert that the validation results
(errors/warnings or success) match between ValidateFromZip and the legacy-path
Validate() to ensure the deprecated zip path is preserved; reference
TestLegacyPreservation_FromZip, ValidateFromZip, newFromZip, ModeLegacy, and
Validate when adding these assertions.
In `@code/go/pkg/validator/api.go`:
- Around line 45-61: The constructors NewFromPath and NewFromFS should validate
the Mode parameter immediately and return an error for unknown/invalid Mode
values instead of deferring failure to Validate()/internalvalidator.NewSpec; add
a mode check (e.g., allow only ModeBuild, ModeLegacy, ModeSource) at the top of
NewFromPath and NewFromFS (or factor into a helper like validateMode(mode Mode)
error) and return a descriptive fmt.Errorf if the mode is invalid, then proceed
to call buildValidator only for valid modes.
- Around line 23-38: The Validator API uses abbreviated identifiers; rename the
struct field fsys to fileSystem and replace short parameter names like va in
Option closures (e.g., WithWarningsAsErrors) with full names such as validator
or vValidator; update all Option type funcs (Option func(*Validator)) and any
other abbreviated variables (per your note also around the other Option
implementations referenced) to use full, descriptive names and adjust all
internal references accordingly (e.g., change va.warningsAsErrors to
validator.warningsAsErrors and fsys usage to fileSystem) to comply with the repo
naming guidelines.
- Around line 126-132: The current NewFromFS branch skips wrapping when fsys is
already a *linkedfiles.FS, allowing callers to bypass ModeLegacy/ModeBuild
semantics; change the logic in NewFromFS so that if mode is ModeLegacy or
ModeBuild you always wrap/replace any incoming *linkedfiles.FS with the blocking
variant (use linkedfiles.NewBlockFS) and only preserve an existing
*linkedfiles.FS when mode == ModeSource (or explicitly Source behavior is
intended). Update the conditional around the linkedfiles.FS type check to
inspect mode (ModeSource vs ModeLegacy/ModeBuild) and call
linkedfiles.NewFS(location, fsys) for ModeSource or linkedfiles.NewBlockFS(fsys)
for legacy/build modes so ValidateFromFS tests continue to block .link files as
expected.
---
Outside diff comments:
In `@code/go/internal/validator/spec_test.go`:
- Around line 19-40: Extend the TestNewSpec table to include two more cases for
NewSpec: one calling NewSpec(*semver.MustParse(version), modes.Empty) (the
zero/empty-mode fallback) and asserting it behaves like the legacy path (no
error and returns *Spec), and another calling
NewSpec(*semver.MustParse(version), modes.Mode(999)) (an invalid/unknown mode)
and asserting it returns an error containing the invalid-mode message and nil
spec; use the same pattern as the existing cases (require.NoError/IsType or
require.Error/Contains/IsNil) and reference NewSpec and the modes value you pass
to locate the logic under test.
🪄 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: 1731f616-9424-48cd-bb23-ab39ba6340c9
📒 Files selected for processing (9)
.gitignorecode/go/internal/validator/modes/modes.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.go
…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>
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/pkg/validator/api_test.go (1)
29-54: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winRename
pkgNametopackageNameper naming guideline.
pkgName(lines 29, 32, 54) is the exact abbreviation called out in the guidelines. Use the full name.♻️ Proposed rename
- pkgName := filepath.Base(packagePath) + packageName := filepath.Base(packagePath) tmpDir := t.TempDir() - zipPath := filepath.Join(tmpDir, pkgName+".zip") + zipPath := filepath.Join(tmpDir, packageName+".zip")- entryName := filepath.ToSlash(filepath.Join(pkgName, rel)) + entryName := filepath.ToSlash(filepath.Join(packageName, rel))As per coding guidelines: "Go variable and function names must use full names, not abbreviations (e.g.,
packageNamenotpkgName...)".🤖 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 29 - 54, Rename the abbreviated variable pkgName to packageName throughout this test (declaration and all uses) so it follows naming guidelines; specifically change the value assignment using filepath.Base(packagePath), update the zipPath construction that uses pkgName+".zip", and update the entryName computation that joins pkgName with rel (and any other references such as zip entry prefixing) to use packageName instead, ensuring you update variable references in os.Create, filepath.Join, and filepath.ToSlash calls within api_test.go.
🤖 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/pkg/validator/api_test.go`:
- Around line 29-54: Rename the abbreviated variable pkgName to packageName
throughout this test (declaration and all uses) so it follows naming guidelines;
specifically change the value assignment using filepath.Base(packagePath),
update the zipPath construction that uses pkgName+".zip", and update the
entryName computation that joins pkgName with rel (and any other references such
as zip entry prefixing) to use packageName instead, ensuring you update variable
references in os.Create, filepath.Join, and filepath.ToSlash calls within
api_test.go.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 55ee2b22-e089-47e4-85b9-3209f7d29b11
📒 Files selected for processing (5)
code/go/pkg/validator/api.gocode/go/pkg/validator/api_test.gocode/go/pkg/validator/validator.gocode/go/pkg/validator/validator_test.gospec/changelog.yml
i will keep it as is thanks! |
jsoriano
left a comment
jsoriano
left a comment
There was a problem hiding this comment.
Overall looks good. Questioning some small design decisions and doing some nitpicking.
There was a problem hiding this comment.
Nit. "api" is redundant here, any public method is going to be part of the package API 🙂
This could belong to validator.go, as it actually defines the Validator object.
| // | ||
| // For ModeLegacy and ModeSource the filesystem honours linked (.link) files; | ||
| // for ModeBuild linked files are blocked (matching a built package artifact). | ||
| func NewFromPath(mode Mode, packageRootPath string, opts ...Option) (*Validator, error) { |
There was a problem hiding this comment.
Nit. Configuration structs are usually easier to discover than functional options, and use to be simpler.
| func NewFromPath(mode Mode, packageRootPath string, opts ...Option) (*Validator, error) { | |
| type Config struct { | |
| WarningsAsErrors bool | |
| } | |
| func NewFromPath(mode Mode, packageRootPath string, config Config) (*Validator, error) { |
There was a problem hiding this comment.
This config is used for the warnings, which is read from the env, so i removed the options/config and read it directly from the env. Changed the test case too to use t.Env instead of a local variable.
v := &Validator{
mode: mode,
location: location,
fsys: fsys,
warningsAsErrors: common.IsDefinedWarningsAsErrors(),
closer: closer,
}
There was a problem hiding this comment.
Part of the original design in #549 was actually to introduce the "Option C" API shape, a new "Validator" object that can be somehow customized with different options.
With current code we are back to something like "Option A": methods with the mode as parameter. Only that we have one method per mode instead of having the mode as a parameter.
I still think it would be valuable to introduce this "Validator" object. Having the object we can add configuration options later without needing to add more functions to the package, or parameters to these functions.
For this initial version it could have warningsAsErros as single configuration, and it could have two validation methods (build and source) if we prefer this approach instead of modes as a parameter.
| switch mode { | ||
| case ModeSource: | ||
| if !isLinkedFS { | ||
| fsys = linkedfiles.NewFS(location, fsys) |
There was a problem hiding this comment.
On what cases we receive a linked FS here?
Nit. Maybe NewFS and NewBlockFS should decide what to do when called with a linkedfiles FS of each kind, so their callers don't need to care.
| // Always block, even a pre-wrapped *linkedfiles.FS. | ||
| fsys = linkedfiles.NewBlockFS(fsys) |
There was a problem hiding this comment.
Umm, not sure if we should do this at this level. Could we instead reject .lnk files in built packages as we will reject _dev files?
Or we need to do this because the spec always receives the link files resolved?
There was a problem hiding this comment.
i am not sure i understand this, or the lines have moved around. here i am validating the fsys, not any linked files. The fs is checked dependeing on the mode "selected" if it allows or not .link files. For build validation there link should be resolved.
There was a problem hiding this comment.
When validating a built package there should not be any .link files.
I was wondering how we are ensuring that, and if the different FSs used was because of that.
| // Preserve a pre-wrapped *linkedfiles.FS; block everything else. | ||
| // Matches the old validateFromFS behaviour. |
There was a problem hiding this comment.
Not sure if this would be needed now. I think this was used before because NewFromPath generated its own linkedfiles.FS. But not sure if this is the case now.
There was a problem hiding this comment.
i need to review this whole refactor, as i am thinking it will be better to have direct constructors for modes, instead of using the mode as a switch...
- 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.
teresaromero
left a comment
teresaromero
left a comment
There was a problem hiding this comment.
thanks for the feedback
| // Always block, even a pre-wrapped *linkedfiles.FS. | ||
| fsys = linkedfiles.NewBlockFS(fsys) |
There was a problem hiding this comment.
i am not sure i understand this, or the lines have moved around. here i am validating the fsys, not any linked files. The fs is checked dependeing on the mode "selected" if it allows or not .link files. For build validation there link should be resolved.
| // Preserve a pre-wrapped *linkedfiles.FS; block everything else. | ||
| // Matches the old validateFromFS behaviour. |
There was a problem hiding this comment.
i need to review this whole refactor, as i am thinking it will be better to have direct constructors for modes, instead of using the mode as a switch...
- 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>
|
@jsoriano i've changed the aproach, having in mind the mode belongs to the spec as a compliment of the definition. this is ready for re-review. thnks |
…inksBehaviorAcrossModes
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/pkg/validator/validator.go`:
- Around line 98-103: validateFromFS currently preserves an incoming
*linkedfiles.FS which lets build-mode callers pass through link-resolving FSs;
change validateFromFS to make link-blocking depend on the validation mode (e.g.,
add a parameter validationMode or a boolean blockLinks) and ensure that when
validationMode == ModeBuild you unconditionally replace/wrap fsys with
linkedfiles.NewBlockFS(fsys) (do not check the incoming type), while for other
modes keep the existing behavior of only wrapping when fsys is not already a
*linkedfiles.FS; update callers (like NewFromFS) to pass the mode flag
accordingly.
- Around line 39-43: The docstrings for ValidateFromBuildPath and the related
build-mode entrypoint should be updated to match the current behavior:
Spec.rules() is now mode-aware, but since no mode-gated rules are registered
yet, these paths only guarantee .link blocking. Adjust the comments on
ValidateFromBuildPath (and the other build-mode docstring noted in the review)
to remove the claim that source-only artifacts are rejected, and describe only
the validation behavior actually implemented by Spec.rules().
- Around line 70-74: The defer currently ignores the zip.ReadCloser close error;
change the defer r.Close() to a deferred closure that captures and checks the
close error (from r.Close()) and if non-nil merges/wraps it into the function's
returned error (e.g. when err is nil set/return the close error or wrap both
when err is non-nil). Locate the zip.OpenReader call that assigns r and replace
the plain defer with a deferred func that observes cerr := r.Close() and returns
or wraps cerr appropriately (or use a named return error variable so the
deferred func can set it).
🪄 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: aec8c75a-0b94-440c-8c8a-34548b6e6308
📒 Files selected for processing (7)
code/go/internal/validator/modes/modes.gocode/go/internal/validator/modes/modes_test.gocode/go/internal/validator/spec.gocode/go/internal/validator/spec_test.gocode/go/pkg/validator/limits_test.gocode/go/pkg/validator/validator.gocode/go/pkg/validator/validator_test.go
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 3
🤖 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/pkg/validator/validator.go`:
- Around line 98-103: validateFromFS currently preserves an incoming
*linkedfiles.FS which lets build-mode callers pass through link-resolving FSs;
change validateFromFS to make link-blocking depend on the validation mode (e.g.,
add a parameter validationMode or a boolean blockLinks) and ensure that when
validationMode == ModeBuild you unconditionally replace/wrap fsys with
linkedfiles.NewBlockFS(fsys) (do not check the incoming type), while for other
modes keep the existing behavior of only wrapping when fsys is not already a
*linkedfiles.FS; update callers (like NewFromFS) to pass the mode flag
accordingly.
- Around line 39-43: The docstrings for ValidateFromBuildPath and the related
build-mode entrypoint should be updated to match the current behavior:
Spec.rules() is now mode-aware, but since no mode-gated rules are registered
yet, these paths only guarantee .link blocking. Adjust the comments on
ValidateFromBuildPath (and the other build-mode docstring noted in the review)
to remove the claim that source-only artifacts are rejected, and describe only
the validation behavior actually implemented by Spec.rules().
- Around line 70-74: The defer currently ignores the zip.ReadCloser close error;
change the defer r.Close() to a deferred closure that captures and checks the
close error (from r.Close()) and if non-nil merges/wraps it into the function's
returned error (e.g. when err is nil set/return the close error or wrap both
when err is non-nil). Locate the zip.OpenReader call that assigns r and replace
the plain defer with a deferred func that observes cerr := r.Close() and returns
or wraps cerr appropriately (or use a named return error variable so the
deferred func can set it).
🪄 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: aec8c75a-0b94-440c-8c8a-34548b6e6308
📒 Files selected for processing (7)
code/go/internal/validator/modes/modes.gocode/go/internal/validator/modes/modes_test.gocode/go/internal/validator/spec.gocode/go/internal/validator/spec_test.gocode/go/pkg/validator/limits_test.gocode/go/pkg/validator/validator.gocode/go/pkg/validator/validator_test.go
🛑 Comments failed to post (3)
code/go/pkg/validator/validator.go (3)
39-43:
⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAlign the build-mode docstrings with the behavior shipped in this PR.
Spec.rules()is mode-aware now, but there are no mode-gated rules registered yet, so these entrypoints currently guarantee.linkblocking only. Saying build validation rejects source-only artifacts overpromises the API contract.Suggested wording
-// Linked files (.link) are blocked; source-only artifacts are rejected. +// Linked files (.link) are blocked. +// Additional build-mode-only validation rules can be layered on as mode-gated rules are introduced.Also applies to: 66-68
🤖 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.go` around lines 39 - 43, The docstrings for ValidateFromBuildPath and the related build-mode entrypoint should be updated to match the current behavior: Spec.rules() is now mode-aware, but since no mode-gated rules are registered yet, these paths only guarantee .link blocking. Adjust the comments on ValidateFromBuildPath (and the other build-mode docstring noted in the review) to remove the claim that source-only artifacts are rejected, and describe only the validation behavior actually implemented by Spec.rules().
70-74:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winHandle the
zip.ReadCloserclose error instead of dropping it.Line 74 is already flagged by
errcheck, and a close failure is currently ignored even though this function returns an error.One simple fix
func ValidateFromZip(packagePath string) error { r, err := zip.OpenReader(packagePath) if err != nil { return fmt.Errorf("failed to open zip file (%s): %w", packagePath, err) } - defer r.Close() dirs, err := fs.ReadDir(r, ".") if err != nil { + _ = r.Close() return fmt.Errorf("failed to read root directory in zip file (%s): %w", packagePath, err) } if len(dirs) != 1 { + _ = r.Close() return fmt.Errorf("a single directory is expected in zip file, %d found", len(dirs)) } subDir, err := fs.Sub(r, dirs[0].Name()) if err != nil { + _ = r.Close() return err } buildSpec := func(version semver.Version) (*validator.Spec, error) { return validator.NewBuildSpec(version) } - return validateFromFS(packagePath, subDir, buildSpec) + err = validateFromFS(packagePath, subDir, buildSpec) + closeErr := r.Close() + if err != nil { + return err + } + return closeErr }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.func ValidateFromZip(packagePath string) error { r, err := zip.OpenReader(packagePath) if err != nil { return fmt.Errorf("failed to open zip file (%s): %w", packagePath, err) } dirs, err := fs.ReadDir(r, ".") if err != nil { _ = r.Close() return fmt.Errorf("failed to read root directory in zip file (%s): %w", packagePath, err) } if len(dirs) != 1 { _ = r.Close() return fmt.Errorf("a single directory is expected in zip file, %d found", len(dirs)) } subDir, err := fs.Sub(r, dirs[0].Name()) if err != nil { _ = r.Close() return err } buildSpec := func(version semver.Version) (*validator.Spec, error) { return validator.NewBuildSpec(version) } err = validateFromFS(packagePath, subDir, buildSpec) closeErr := r.Close() if err != nil { return err } return closeErr }🧰 Tools
🪛 golangci-lint (2.12.2)
[error] 74-74: Error return value of
r.Closeis not checked(errcheck)
🤖 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.go` around lines 70 - 74, The defer currently ignores the zip.ReadCloser close error; change the defer r.Close() to a deferred closure that captures and checks the close error (from r.Close()) and if non-nil merges/wraps it into the function's returned error (e.g. when err is nil set/return the close error or wrap both when err is non-nil). Locate the zip.OpenReader call that assigns r and replace the plain defer with a deferred func that observes cerr := r.Close() and returns or wraps cerr appropriately (or use a named return error variable so the deferred func can set it).
98-103:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winMake link blocking depend on validation mode, not the incoming FS type.
This helper preserves any pre-wrapped
*linkedfiles.FS. A build-mode caller that passes one will resolve.linkfiles instead of rejecting them, which breaks the new build-mode contract and makesvalidateFromFSunsafe to reuse for build validation.Based on learnings: In
code/go/pkg/validator/api.go(NewFromFS),ModeBuildalways replaces withlinkedfiles.NewBlockFS, even if a*linkedfiles.FSis passed.🤖 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.go` around lines 98 - 103, validateFromFS currently preserves an incoming *linkedfiles.FS which lets build-mode callers pass through link-resolving FSs; change validateFromFS to make link-blocking depend on the validation mode (e.g., add a parameter validationMode or a boolean blockLinks) and ensure that when validationMode == ModeBuild you unconditionally replace/wrap fsys with linkedfiles.NewBlockFS(fsys) (do not check the incoming type), while for other modes keep the existing behavior of only wrapping when fsys is not already a *linkedfiles.FS; update callers (like NewFromFS) to pass the mode flag accordingly.
jsoriano
left a comment
jsoriano
left a comment
There was a problem hiding this comment.
Thanks, it looks simpler now.
Some notes:
- Using specific methods for the validation modes LGTM, at the end I don't think we are going to have more modes beyond Source and Build.
- With the last changes we are losing the "Option C" API shape, that would be a "Validator" object that could have different settings, so we don't need to add additional methods or parameters to these methods if we need more settings in the future.
- Please check that links are not allowed in built packages. They should have been resolved when building the package.
| // | ||
| // For ModeLegacy and ModeSource the filesystem honours linked (.link) files; | ||
| // for ModeBuild linked files are blocked (matching a built package artifact). | ||
| func NewFromPath(mode Mode, packageRootPath string, opts ...Option) (*Validator, error) { |
There was a problem hiding this comment.
Part of the original design in #549 was actually to introduce the "Option C" API shape, a new "Validator" object that can be somehow customized with different options.
With current code we are back to something like "Option A": methods with the mode as parameter. Only that we have one method per mode instead of having the mode as a parameter.
I still think it would be valuable to introduce this "Validator" object. Having the object we can add configuration options later without needing to add more functions to the package, or parameters to these functions.
For this initial version it could have warningsAsErros as single configuration, and it could have two validation methods (build and source) if we prefer this approach instead of modes as a parameter.
| // Always block, even a pre-wrapped *linkedfiles.FS. | ||
| fsys = linkedfiles.NewBlockFS(fsys) |
There was a problem hiding this comment.
When validating a built package there should not be any .link files.
I was wondering how we are ensuring that, and if the different FSs used was because of that.
…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>
There was a problem hiding this comment.
Actionable comments posted: 1
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/spec_test.go (1)
19-40: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winAdd a direct invalid-mode case for
NewSpec.This test now exercises the new signature, but it still only covers version failures. Adding a
NewSpec(validVersion, modes.Mode("invalid"))case would lock down the constructor’s ownmode.Valid()branch and catch regressions even if caller-side validation changes later.🤖 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/spec_test.go` around lines 19 - 40, Add a direct "invalid mode" subcase to TestNewLegacySpec: call NewSpec with a valid semver (e.g., semver.MustParse("1.0.0")) and an invalid mode value created as modes.Mode("invalid"), then assert that it returns an error (require.Error) and that spec is nil (require.Nil) and the error message contains the expected invalid-mode text coming from NewSpec/mode.Valid() branch; this locks the constructor's mode validation (NewSpec and modes.Mode) against regressions.
🤖 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/pkg/validator/modes.go`:
- Line 1: Add the required Elastic License copyright header to the top of the
new Go file that declares package validator (modes.go); insert the canonical
Elastic License header immediately above the existing "package validator" line
so the file begins with the mandated header before any package/imports or code.
---
Outside diff comments:
In `@code/go/internal/validator/spec_test.go`:
- Around line 19-40: Add a direct "invalid mode" subcase to TestNewLegacySpec:
call NewSpec with a valid semver (e.g., semver.MustParse("1.0.0")) and an
invalid mode value created as modes.Mode("invalid"), then assert that it returns
an error (require.Error) and that spec is nil (require.Nil) and the error
message contains the expected invalid-mode text coming from NewSpec/mode.Valid()
branch; this locks the constructor's mode validation (NewSpec and modes.Mode)
against regressions.
🪄 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: 1286ec04-b7c3-4a54-9100-d0fb08be6e9c
📒 Files selected for processing (7)
code/go/internal/validator/modes/modes.gocode/go/internal/validator/spec.gocode/go/internal/validator/spec_test.gocode/go/pkg/validator/limits_test.gocode/go/pkg/validator/modes.gocode/go/pkg/validator/validator.gocode/go/pkg/validator/validator_test.go
I've reintroduced the Validator struct. in order to have the modes as public there is the Mode type which owns the fs for each mode. This way fs are determined. I've kept |
| if v.closer != nil { | ||
| defer func() { | ||
| if cerr := v.closer.Close(); cerr != nil { | ||
| err = errors.Join(err, cerr) | ||
| } | ||
| }() |
There was a problem hiding this comment.
Validate should not close the underlying filesystem. It would be an unexpected side effect.
If Validator creates its own filesystems or resources that need to be cleaned up, it should expose its own Close method.
| return nil, fmt.Errorf("invalid validation mode %q", mode.internal) | ||
| } | ||
| defer r.Close() | ||
| fsys := mode.wrapFS(packageRootPath, os.DirFS(packageRootPath)) |
There was a problem hiding this comment.
Nit. If this wrapFS is only used in this case, it would simplify things to handle it only here.
| fsys := mode.wrapFS(packageRootPath, os.DirFS(packageRootPath)) | |
| fsys := os.DirFS(packageRootPath) | |
| if mode == ModeBuild { | |
| fsys = linkedfiles.NewBlockFS(fsys) | |
| } else { | |
| fsys = linkedfiles.NewFS(packageRootPath, fsys) | |
| } |
There was a problem hiding this comment.
i initially had this if/else but i thought to enforce the fs is owned by the mode, so wrapFs represents the fs asigned by the mode... so this constructor just uses whatever the mode has and is not responsible of creating it, if that makes sense? happy to change it if this is not correct
There was a problem hiding this comment.
Functionality seems to be correct, the only issue I see is that it is adding complexity with an abstraction that is only used in a place.
In general all this handling of links gives me a bit of bad feeling, but I don't have better ideas.
In any case all these are internal details, so let it with the approach you prefer.
There was a problem hiding this comment.
If you keep modes as objects with wrapFS methods I would suggest to convert Mode in an interface, and make wrapFS a method of the implementing objects.
type Mode interface {
mode() modes.Mode
wrapFS(location string, fsys fs.FS) fs.FS
}
type SourceMode struct {}
func (SourceMode) mode() modes.Mode { return mode.Source }
func (SourceMode) wrapFS(location string, fsys fs.FS) fs.FS {
return linkedfiles.NewFS(location, fsys)
}
...This looks more aligned with Go conventions.
|
|
||
| // ValidateFromFS validates a package against the appropiate specification and returns any errors. | ||
| // Package files are obtained through the given filesystem. | ||
| func ValidateFromFS(location string, fsys fs.FS) error { |
There was a problem hiding this comment.
This method is removed, right? This would be a breaking change in the public API.
There was a problem hiding this comment.
ValidateFromFS although is a public method is just being used at tests files. I've reviewed elastic-package and dont see this function used explicitly so, its a breaking change because is no longer available but is not being used 🤔 I will revert and keep it, although i think we should deprecate it in favour of NewFromFS
There was a problem hiding this comment.
We should deprecate all ValidateFrom methods at the package level, and keep in the long term only the Validator object.
In the meantime let's keep all of them, just to follow good practices, just in case.
| if _, ok := fsys.(*linkedfiles.FS); !ok { | ||
| fsys = linkedfiles.NewBlockFS(fsys) | ||
| // Validate runs package validation and returns any errors encountered. | ||
| func (v *Validator) Validate() (err error) { |
There was a problem hiding this comment.
Going back to the API design question. Sorry for the back and forth.
With current design we have the object, but given that it already receives everything in the constructor, it is just like a function with delayed execution.
These two shapes are really not very different, and the first one has more complexity with little value:
v, err := validator.NewFromZip(path, options...)
err = v.Validate()And:
err := validator.ValidateFromZip(path, options...)What would give more value to the API shape, and what I'd say the "Option C" was, is to have a stateless validator that clearly separates validation configuration from the validation of one, or more packages.
Something like this:
// Configuration-time
v, err := validator.New(validator.ModeSource,
validator.WithWarningsAsErrors(true),
validator.WithFilter(filterConfig), // future
validator.WithLogger(log), // future
)
// Verb-time, repeatable, no shared mutable state
err = v.ValidateFromPath("/pkg1")
err = v.ValidateFromPath("/pkg2")
err = v.ValidateFromZip("/pkg.zip")
err = v.ValidateFromFS("loc", fsys)- 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.
- Introduced multiple test cases for ValidateFromFS to handle various file system scenarios, including linked and block file systems. - Added tests for ValidateFromZip to enforce mode restrictions and validate package integrity. - Enhanced documentation comments for validation modes and methods to clarify their usage and constraints. - Implemented helper functions for creating zip files for testing purposes.
jsoriano
left a comment
jsoriano
left a comment
There was a problem hiding this comment.
Looks good overall, added some comments about some small details.
| return errors.New("linked files are not supported in ModeBuild") | ||
| } else if _, ok := fsys.(*linkedfiles.BlockFS); ok && v.mode == ModeSource { | ||
| return errors.New("block linked files are not supported in ModeSource") | ||
| } |
There was a problem hiding this comment.
I still don't like the handling of linked file systems, but this is internal and we can revisit later, I don't have much better ideas now.
| } | ||
| spec.WarningsAsErrors = warningsAsErrors | ||
| spec.WarningsAsErrors = v.warningsAsErrors | ||
|
|
There was a problem hiding this comment.
Maybe we should append a technical preview warning when Source or Build modes are used, at least till we complete the validators.
There was a problem hiding this comment.
I've added just using the log pkg as i havent seen any logger implemented on the package d895e49
i followed the same pattern as in
There was a problem hiding this comment.
Yeah, we don't have an abstraction for warnings, maybe we should add it.
We could leverage warningsAsErrors, something like this:
errs := spec.ValidatePackage(*pkg)
if v.mode != LegacyMode {
err := specerrors.NewStructuredErrorf("validation mode '%s' is in technical preview", v.mode)
if v.warningsAsErrors() {
errs = append(errs, err)
} else {
log.Printf("Warning: %s", err.Message())
}
}
if len(errs) > 0 {
return errs
}
return nil- Renamed mode constants from ModeLegacy, ModeSource, and ModeBuild to LegacyMode, SourceMode, and BuildMode for improved readability. - Updated all references in tests and validation logic to reflect the new naming convention. - Enhanced test cases to ensure proper validation behavior across different modes.
- Renamed NewValidator function to New for improved clarity and consistency. - Updated all references in tests and validation logic to use the new instantiation method. - Ensured that the new method maintains the same functionality while simplifying the API. - Adjusted test cases to reflect the changes in the validator creation process.
- Introduced a log statement to warn users when the validation mode is in technical preview. - Ensured that the logging occurs only when the mode is not set to LegacyMode, enhancing user awareness of the current validation state.
💚 Build Succeeded
History
|
| } | ||
| spec.WarningsAsErrors = warningsAsErrors | ||
| spec.WarningsAsErrors = v.warningsAsErrors | ||
|
|
There was a problem hiding this comment.
Yeah, we don't have an abstraction for warnings, maybe we should add it.
We could leverage warningsAsErrors, something like this:
errs := spec.ValidatePackage(*pkg)
if v.mode != LegacyMode {
err := specerrors.NewStructuredErrorf("validation mode '%s' is in technical preview", v.mode)
if v.warningsAsErrors() {
errs = append(errs, err)
} else {
log.Printf("Warning: %s", err.Message())
}
}
if len(errs) > 0 {
return errs
}
return nil3 participants
What does this PR do?
Introduces a mode-aware validation API in
pkg/validatorand wires validation mode through the internalSpec.Public API (
code/go/pkg/validator/validator.go)Mode(re-exported from internal) with three values:ModeLegacy,ModeSource, andModeBuild.Validatorconfigured viaNewValidator(mode Mode, opts ...Option) (*Validator, error).WithWarningsAsErrors(bool)to control warning promotion independently of thePACKAGE_SPEC_WARNINGS_AS_ERRORSenvironment variable.*Validator:ValidateFromPath(path string)— wraps the disk FS per mode (linkedfiles.NewFSfor legacy/source,linkedfiles.NewBlockFSfor build).ValidateFromZip(zipPath string)— supported inModeLegacyandModeBuildonly; always validates throughBlockFS.ValidateFromFS(location string, fsys fs.FS)— validates caller-supplied FS with mode-specific linked-file semantics and rejects incompatible FS/mode pairings (e.g.linkedfiles.FSin build mode,BlockFSin source mode).ValidateFromPath,ValidateFromZip, andValidateFromFS; each now delegates toNewValidator(ModeLegacy)and preserves legacy behavior for existing callers.Internal plumbing (
code/go/internal/validator/)Modetype andValid()inmodes.go.NewSpec(version, mode)and stores mode onSpec.Spec.rules()with an optional per-rulemodes []Modefilter so follow-up PRs can gate semantic rules without further API changes.Changelog
3.7.0-nextentry for mode-aware constructors and validation APIs.Invalid mode values (including zero-value
Mode("")) are rejected atNewValidatorconstruction time.Why is it important?
The validator currently has no concept of validation mode — all callers get the same rules regardless of whether they are validating a source checkout or a built artifact. This PR lays the API foundation so future PRs can attach mode-specific rules (and enforce distinct source vs. build package shapes per #549) without breaking existing callers.
Existing integrations (
elastic-package, etc.) continue to work via the deprecated entry points, which run inModeLegacy.Checklist
I have added test packages totest/packagesthat prove my change is effective.spec/changelog.yml.Related issues
Tests
New and updated tests cover the mode-aware API:
TestValidinternal/validator/modes_test.goMode.Valid()accepts legacy/source/build and rejects invalid/empty stringsTestNewValidator_RejectsInvalidModepkg/validator/validator_test.goTestLinksBehaviorAcrossModespkg/validator/validator_test.go.linkfiles; source and legacy accept them on diskTestValidateFromFS_rejectsIncompatibleFSpkg/validator/validator_test.golinkedfiles.FSand source +BlockFSare rejected; compatible pairings succeedTestValidateFromFS_legacyPlainFSBlocksLinkspkg/validator/validator_test.goBlockFS; legacy +linkedfiles.FSresolves linksTestValidateFromZip_modeRestrictionspkg/validator/validator_test.goTestValidateFromZip_validatesPackagepkg/validator/validator_test.goTestDeprecatedValidateFromZippkg/validator/validator_test.goValidateFromZipwrapper still worksTestWithWarningsAsErrors_optionpkg/validator/validator_test.goTestLinksAreBlockedpkg/validator/validator_test.goValidateFromFSwith plain mock FS blocks links (legacy behavior)Uses existing test package
test/packages/with_linksfor link-file scenarios. Internalspec_test.goupdated forNewSpec(..., ModeLegacy).Follow-ups / open questions
ModeLegacyvsModeSource: kept as distinct modes for migration safety; may collapse once source-only semantics are fully defined and callers have migrated.modes []Modeon rule registration); individual rules are not yet filtered — that lands in a follow-up PR.elastic-packageand other consumers still use deprecated entry points; reference migration toNewValidator(ModeSource|ModeBuild)is planned separately.