feat: add interactive policy-file wizard (kosli create policy-file)#766
feat: add interactive policy-file wizard (kosli create policy-file)#766dangrondahl wants to merge 51 commits intomainfrom
Conversation
|
Claude finished @dangrondahl's task in 2m 43s —— View job Review of PR #766:
|
| Priority | File | Issue |
|---|---|---|
| Medium | createPolicyFile.go:91 |
File-exists check happens after the entire wizard — if the file exists, the user loses all their work. Move the os.Stat check into validateYAMLExtension in forms.go so users get feedback at stepSaveFile time (inline comment) |
| Low | forms.go:175 |
exists operator still shows Value input that is silently ignored — a Placeholder("(not used)") would reduce confusion (inline comment) |
| Low | forms.go:296 |
extractFormValues — the firstNonEmpty invariant still lacks a comment explaining the implicit coupling (inline comment) |
What's done well
- Clean separation of concerns:
internal/policyhandles data/YAML,internal/policywizardhandles the TUI state machine, and the Cobra command is a thin orchestration layer. This makes the code easy to test and extend. - Thorough test coverage: 70+ unit tests covering the full state machine — advance steps, form value application, expression combining, negation, and edge cases like invalid regex retries and empty values. The tests are well-organized using table-driven patterns where appropriate.
- Good defensive coding: nil guards in
applyExpression,CombineExprszero-arg guard, regex validation, terminal detection, empty-value rejection — these were all addressed in the latest commits. - The
fetchNameListrefactor (createPolicyFile.go:124) nicely deduplicates the two API fetch functions with atransformcallback. - Expression builder design: The unwrap/wrap/combine/negate pipeline is clean and composable. The tests at
expression_test.go:106-111demonstratingCombineAndNegateshow the pieces work well together.
Overall this is solid work — the medium-priority item (file-exists check timing) is the only one I'd want addressed before merge.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
Addressed 5 of the 7 review comments in commits
|
|
Reviewed the latest 5 inline comments — all are informational observations, none blocking:
No changes needed for these. |
Add internal/policy package with Policy structs and ToYAML(). Wildcard name "*" is normalized to omit from YAML output.
Add FlowNameExpr, FlowNameInExpr, ArtifactNameMatchExpr,
ComparisonExpr, CombineExprs, and WrapExpr functions for
constructing ${{ }} policy expressions from structured inputs.
Wire up `kosli create policy-file` as a new subcommand. Adds charmbracelet/huh dependency for interactive TUI wizard. Minimal wizard: provenance + trail-compliance confirms, YAML to stdout.
Add interactive attestation collection with type select (6 built-in types) and name input. Users can add multiple attestations in a loop.
The go.mod conflict resolution during rebase accidentally downgraded kind from v0.31.0 to v0.11.1, breaking ExportKubeConfig API.
dangrondahl
force-pushed
the
create_policy_file
branch
from
7834a82 to
61ff258
Compare
not flow.name == "prod" could be parsed as (not flow.name) == "prod". Using not (flow.name == "prod") is unambiguous.
| huh.NewSelect[string]().Key("op").Title("Operator"). | ||
| Options(huh.NewOptions("==", "!=", ">", "<", ">=", "<=", "matches", "exists")...), | ||
| huh.NewInput().Key("value").Title("Value"). | ||
| Description("The value to compare against (ignored for exists)"), |
There was a problem hiding this comment.
Low: stepExprCustomOp value field accepts empty input for comparison operators
The Value input has no Validate call, so for operators like ==, !=, >, etc., the user can submit an empty string. This produces expressions like ${{ flow.name == "" }} — technically valid but likely unintended.
Consider adding a conditional validator, or at minimum a notEmpty("value") validator (like stepExprFlowTagOp at line 145 uses).
| } | ||
|
|
||
| outPath := filepath.Clean(wm.OutputFile) | ||
| if err := validateOutputFile(outPath); err != nil { |
There was a problem hiding this comment.
UX note: file-exists check happens after the entire wizard completes
validateOutputFile runs after the user has gone through the full wizard. If the file already exists, the user loses all their work. Consider validating the file at wizard time (in stepSaveFile's validator) rather than only after the wizard exits.
One option: add an os.Stat check in validateYAMLExtension (forms.go:259) so the user gets immediate feedback within the wizard.
Without clearing, a failed matches regex validation would leave validationErr set, causing an infinite loop when the user retried with a valid regex or switched to a different operator.
| } | ||
|
|
||
| outPath := filepath.Clean(wm.OutputFile) | ||
| if err := validateOutputFile(outPath); err != nil { |
There was a problem hiding this comment.
UX: file-exists check happens after the entire wizard completes
If the target file already exists, the user loses all their work after completing the full wizard. Consider moving the os.Stat existence check into validateYAMLExtension in forms.go so the user gets immediate feedback within the wizard at stepSaveFile. That way they can pick a different name without restarting.
Something like:
func validateYAMLExtension(s string) error {
if s == "" {
return nil
}
ext := strings.ToLower(filepath.Ext(s))
if ext != ".yaml" && ext != ".yml" {
return fmt.Errorf("file must have a .yaml or .yml extension")
}
if _, err := os.Stat(s); err == nil {
return fmt.Errorf("file %q already exists; choose a different name", s)
}
return nil
}The post-wizard validateOutputFile can remain as a safety net, but the primary check should be at input time.
Empty values produced expressions like ${{ flow.name == "" }} which
are technically valid but likely unintended. The exists operator
correctly allows empty values since it takes no argument.
| return err | ||
| } | ||
|
|
||
| err = os.WriteFile(outPath, yamlBytes, 0644) |
There was a problem hiding this comment.
UX: file-exists check happens after the entire wizard
If the target file already exists, the user loses all their wizard work. Consider moving the os.Stat existence check into validateYAMLExtension in forms.go so the user gets immediate feedback within the wizard at stepSaveFile:
func validateYAMLExtension(s string) error {
if s == "" {
return nil
}
ext := strings.ToLower(filepath.Ext(s))
if ext != ".yaml" && ext != ".yml" {
return fmt.Errorf("file must have a .yaml or .yml extension")
}
if _, err := os.Stat(s); err == nil {
return fmt.Errorf("file %q already exists; choose a different name", s)
}
return nil
}The post-wizard validateOutputFile can remain as a safety net, but the primary check should happen at input time to avoid frustration.
| huh.NewSelect[string]().Key("op").Title("Operator"). | ||
| Options(huh.NewOptions("==", "!=", ">", "<", ">=", "<=", "matches", "exists")...), | ||
| huh.NewInput().Key("value").Title("Value"). | ||
| Description("The value to compare against (ignored for exists)"), |
There was a problem hiding this comment.
Low: exists operator — Value field accepts input that is silently discarded
The description text helps, but users can still type a value and be confused when it doesn't appear in the output. Consider adding a validator that rejects non-empty input when exists is selected, or at least adding Placeholder("(not used)") to reinforce the message.
Low priority — the current description is a reasonable mitigation.
1 participant
Why
Writing Kosli environment policy files by hand is harder than it should be. The YAML format, exception syntax, expression operators (
==,!=,matches(),exists(),not,and,or), and available context fields (flow.name,flow.tags.<key>,artifact.name,artifact.fingerprint) are easy to forget — especially when you're creating your first policy or haven't touched one in a while.This is an attempt to lower the barrier to creating the first (and many subsequent) policy files by providing a guided, interactive experience.
Summary
kosli create policy-filecommand that launches an interactive TUI wizard to build environment policy YAML files conforming to the policy schema${{ }}syntax) with support for:matches())exists())and,or,not).yaml/.ymlextension required, no silent overwritesNew packages
internal/policyinternal/policywizardDependencies added
charmbracelet/huh— form componentscharmbracelet/bubbletea— TUI frameworkcharmbracelet/lipgloss— stylingcharmbracelet/bubbles— spinner