feat: add promise check command for structural validation - #260
Conversation
kratix promise check --review-file <path> validates a review-findings YAML/JSON artifact and exits non-zero if any unresolved critical or high finding remains. Self-registers under a new `promise` parent command via init(), no changes to root.go. No new dependencies (cobra, yaml.v3 already present). Built and tested against infrastructure-discovery tooling that needed a hard pass/fail gate rather than a prose "review carefully" instruction before a generated Promise gets applied.
…schema validation)
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6a91da963a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if crd.APIVersion != "apiextensions.k8s.io/v1" || crd.Kind != "CustomResourceDefinition" { | ||
| errs = append(errs, fmt.Sprintf("%s: spec.api is not a valid CustomResourceDefinition (apiVersion=%q kind=%q)", name, crd.APIVersion, crd.Kind)) |
There was a problem hiding this comment.
When spec.api has the expected apiVersion/kind, Namespaced scope, and any versions entry, this gate accepts it even if required CRD fields such as metadata.name, spec.group, names.plural, or version served/storage flags are absent. Such a Promise cannot be installed, so the advertised valid-CRD gate can return success for a structurally invalid generated artifact; decode and validate the complete Kubernetes CRD rather than treating these few fields as sufficient.
Useful? React with 👍 / 👎.
| errs = append(errs, checkPipelines(name, "configure", doc.Spec.Workflows.Resource.Configure)...) | ||
| errs = append(errs, checkPipelines(name, "delete", doc.Spec.Workflows.Resource.Delete)...) |
There was a problem hiding this comment.
Only spec.workflows.resource.configure and .delete are inspected, so a container without an image under spec.workflows.promise.configure passes the gate. Promise-level configure pipelines are present in generated outputs such as the Terraform fixtures, and an empty image there still produces a broken workflow despite the command promising to check pipeline images; represent and inspect the promise workflow pipelines as well.
Useful? React with 👍 / 👎.
| if fieldSchema.Pattern != "" { | ||
| str, isString := value.(string) | ||
| if !isString { | ||
| continue |
There was a problem hiding this comment.
When a property has no pattern, any value is accepted regardless of its declared type; even with a pattern, a non-string value explicitly skips validation here. For example, a CRD property declared as integer accepts a YAML string, and nested required fields are never visited, allowing promise check to succeed for requests that Kubernetes rejects. Use a complete OpenAPI schema validator, including recursive and type validation, instead of limiting validation to top-level presence and regex checks.
Useful? React with 👍 / 👎.
| return []string{fmt.Sprintf("%s: does not parse as YAML: %v", name, err)} | ||
| } | ||
|
|
||
| for promiseName, doc := range promises { |
There was a problem hiding this comment.
When two loaded Promises declare the same group/version/kind but different schemas, this map iteration selects an arbitrary Promise and immediately returns its validation result. The same example can therefore pass or fail across runs depending on Go's map order; detect the duplicate GVK as a gate failure or build a deterministic unique-GVK index before validating examples.
AGENTS.md reference: AGENTS.md:L13-L13
Useful? React with 👍 / 👎.
| } `yaml:"spec"` | ||
| } | ||
|
|
||
| type promiseCRD struct { |
There was a problem hiding this comment.
Can't we reuse the kratix definitions for these kratix CRDs?
| // exampleDir and runs all Level-1 checks. Either directory may be absent | ||
| // (returns no errors, gate is skipped) so this stays backward compatible | ||
| // with a review-file-only check. | ||
| func runLevel1Gates(promiseDir, exampleDir string, out io.Writer) []string { |
There was a problem hiding this comment.
It should be one instead of 1
Decode Promises with v1alpha1.Promise and CRDs with the real apiextensionsv1.CustomResourceDefinition instead of hand-rolled promiseDoc/promiseCRD structs, and validate CRDs with the same apiextensions-apiserver validator Kubernetes itself uses. This catches incomplete CRDs (missing names.plural, no storage version, etc.) that the old field-by-field check let through.
The image gate only inspected spec.workflows.resource.configure/.delete, so a missing image under spec.workflows.promise.configure (seen in real Terraform-fixture output) silently passed. Switch to Kratix's own NewPipelinesMap, which covers all four workflow slots, and also flag a pipeline declared with zero containers.
The old check only looked at top-level presence and a regex pattern: a property with no pattern accepted any value regardless of declared type, a non-string value with a pattern skipped validation entirely, and nested required fields were never recursed into. Replace it with apiextensions-apiserver's real CRD schema validator, validating the whole example document against the whole schema so root-level rules and unserved CRD versions are caught too.
Two Promises sharing a group/version/kind used to be resolved by Go's undefined map iteration order, so an example could pass or fail depending on the run. buildGVKIndex now walks Promise files in sorted order and reports a duplicate GVK as a gate failure instead of picking one arbitrarily.
Trivial wording fix from review: "one", not "1".
|
@codex review |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
The review-findings/AI-review concept has been ported to a standalone kratix-review plugin. kratix-cli core keeps only the Level-1 structural checks against the Promise's CRD, workflows, and examples.
Rename runLevelOneGates to runStructuralChecks and reword help text and comments to describe checks in plain terms, since "Level-1 gate" is another project's AI-grading vocabulary, not kratix-cli's.
shano
changed the title
| Long: "Validate the generated Promise/example files in --promise-dir and --example-dir: the CRD is a " + | ||
| "valid, Namespaced-scope CustomResourceDefinition, every workflow pipeline container has an image " + | ||
| "set, the delete workflow is well-formed, and every example/Resource Request validates against its " + | ||
| "Promise's CRD schema (including CEL rules and schema defaults). Either directory may be absent, in " + | ||
| "which case its checks are skipped.", |
There was a problem hiding this comment.
The long comment is maybe too long? I think it could finish at ...example-dir.
| // gvkPromise pairs a group/version/kind with the name of the Promise file | ||
| // that declared it and its CRD, so checkExampleFile can validate against a | ||
| // single, deterministically-chosen Promise (see buildGVKIndex). | ||
| type gvkPromise struct { |
There was a problem hiding this comment.
Any way to pull this from the Kratix code through a library call?
govulncheck now finds several new stdlib vulnerabilities (fixed in Go 1.25.13/1.26.6) alongside the previously-allowlisted, unfixable ones (x/crypto/openpgp, containerd v1, crossplane), causing the security-scan job to fail again. Pinning the toolchain closes the stdlib gap; the remaining findings are still covered by the existing KNOWN_NA allowlist in tests.yaml. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
4 participants
Summary
Stacked on #259 (adds
promise check) — extends it with the Level-1 deterministic gates from ADR0010: checks the generated Promise/Resource Request files directly, no model involved.New optional flags on
kratix promise check:--promise-dir(defaultpromises) — for each Promise file: valid CRD (spec.apiis a real CustomResourceDefinition),scope: Namespaced(Kratix only supports this), every workflow pipeline container has animageset, and thedeleteworkflow has at most one pipeline (Kratix only supports one).--example-dir(defaultresource-requests) — matches each example/Resource Request to its Promise's CRD byapiVersion+kind, then validates required fields are present, no unknown fields, and any declaredpatternmatches.Both flags are optional and backward compatible — if the directories don't exist (the common case for existing
--review-file-only usage), that part of the gate is silently skipped, same behavior as #259 today.Ran the built binary against a real generated output (ske-lift's
fresh-baseline-2scenario, 10 Promises / 11 Resource Requests) — passes clean. Also verified negatively: deliberately broke one Promise'sscopetoClusterin a scratch copy and confirmed the gate catches it correctly.Test plan
go test ./cmd/...— all existing + 9 new tests pass.gofmt -lclean,go vet ./...clean.fresh-baseline-2output — exit 0.scope: Cluster, confirmed gate fails with the right message.--review-fileonly, no promise/example dirs present, exit 0 same as before.