From 12dae8dd8779a30d4610073a071b0db2ebe00f3a Mon Sep 17 00:00:00 2001 From: shenxianpeng Date: Mon, 25 May 2026 19:15:29 +0300 Subject: [PATCH] fix strict warning validation --- README.md | 32 +++-- internal/cmd/validate_test.go | 39 ++++++ internal/validator/validator.go | 25 ++-- internal/validator/validator_test.go | 183 +++++++++++++++++++++++++++ 4 files changed, 258 insertions(+), 21 deletions(-) create mode 100644 internal/cmd/validate_test.go create mode 100644 internal/validator/validator_test.go diff --git a/README.md b/README.md index 9b35dc8..be9cef1 100644 --- a/README.md +++ b/README.md @@ -1,12 +1,12 @@ # ODS CLI -**Reference CLI tool for Open Delivery Spec validation and generation.** +**Reference CLI tool for Open Delivery Spec validation.** [![CI](https://github.com/open-delivery-spec/cli/actions/workflows/ci.yml/badge.svg)](https://github.com/open-delivery-spec/cli/actions/workflows/ci.yml) [![Go Version](https://img.shields.io/badge/Go-1.23+-00ADD8?logo=go)](https://go.dev) [![License](https://img.shields.io/badge/License-Apache%202.0-blue.svg)](LICENSE) -> **Status**: Early development. `validate` subcommands are functional. Other command groups (`generate`, `review`, `release`, `evidence`, `ci`, `approval`) are stubs that print placeholder output. See [Roadmap](https://github.com/open-delivery-spec/spec/blob/main/ROADMAP.md) for module maturity. +> **Status**: Early development. The production-ready M1 surface is `ods validate branch`, `ods validate commit`, and `ods validate pr`. Other validation commands are schema checks for draft modules. Command groups such as `generate`, `release`, `evidence`, `ci`, and `approval` are experimental and may print placeholder output. See [Roadmap](https://github.com/open-delivery-spec/spec/blob/main/ROADMAP.md) for module maturity. ## Install @@ -32,27 +32,35 @@ ods validate pr --file PR_BODY.md ods validate branch feat/AI-experiment --strict ``` -## Stable Commands +## Stable M1 Commands ### `ods validate` -Validate delivery artifacts against ODS schemas. +Validate the L1 delivery artifacts that are ready for CI enforcement. ```bash -ods validate branch # Validate branch name +ods validate branch # Validate branch name ods validate commit [--file | --stdin] # Validate commit message ods validate pr [--file | --stdin] # Validate PR description -ods validate rollback [--file | --stdin]# Validate rollback plan -ods validate evidence [--file | --stdin]# Validate evidence bundle -ods validate release [--file | --stdin]# Validate release readiness report -ods validate approval-policy [--file] # Validate approval policy ``` -All validate subcommands support `--strict` to treat warnings as errors. +All stable validate subcommands support `--strict` to treat warnings as errors. -## Experimental Commands +## Draft Schema Validation -The following command groups are registered but currently print placeholder output. They will gain real functionality as their corresponding spec modules mature. +These commands validate JSON files against draft module expectations. They are useful for experimentation, but the corresponding workflows are not production gates yet. + +```bash +ods validate rollback [--file | --stdin] # Validate rollback plan JSON +ods validate evidence [--file | --stdin] # Validate evidence bundle JSON +ods validate release [--file | --stdin] # Validate release readiness JSON +ods validate approval-policy [--file | --stdin] # Validate approval policy JSON +ods review validate [--file | --stdin] # Validate AI review JSON +``` + +## Experimental Command Groups + +The following command groups are registered but currently include placeholder output. They will gain real functionality as their corresponding spec modules mature. ### `ods generate` ``` diff --git a/internal/cmd/validate_test.go b/internal/cmd/validate_test.go new file mode 100644 index 0000000..2a800aa --- /dev/null +++ b/internal/cmd/validate_test.go @@ -0,0 +1,39 @@ +package cmd + +import ( + "testing" + + "github.com/open-delivery-spec/cli/internal/validator" +) + +func TestPrintResultStrictTreatsWarningsAsErrors(t *testing.T) { + originalStrict := strict + t.Cleanup(func() { + strict = originalStrict + }) + + strict = true + err := printResult(validator.Result{ + Status: validator.StatusConformantWarnings, + Warnings: []string{"breaking change detected"}, + }) + if err == nil { + t.Fatal("printResult() error = nil, want strict warning failure") + } +} + +func TestPrintResultAllowsWarningsWithoutStrict(t *testing.T) { + originalStrict := strict + t.Cleanup(func() { + strict = originalStrict + }) + + strict = false + err := printResult(validator.Result{ + Status: validator.StatusConformantWarnings, + Warnings: []string{"breaking change detected"}, + }) + if err != nil { + t.Fatalf("printResult() error = %v, want nil", err) + } +} diff --git a/internal/validator/validator.go b/internal/validator/validator.go index ba1a069..3485688 100644 --- a/internal/validator/validator.go +++ b/internal/validator/validator.go @@ -29,6 +29,13 @@ const ( StatusNonConformant ValidationStatus = "non_conformant" ) +func finalizeResult(result Result) Result { + if result.Status == StatusConformant && len(result.Warnings) > 0 { + result.Status = StatusConformantWarnings + } + return result +} + // schema cache var schemas = map[string]interface{}{} @@ -150,7 +157,7 @@ func ValidateBranch(name string) (Result, error) { } } - return result, nil + return finalizeResult(result), nil } // ValidateCommitMessage validates a commit message string. @@ -210,7 +217,7 @@ func ValidateCommitMessage(msg string) (Result, error) { } } - return result, nil + return finalizeResult(result), nil } // ValidatePRDescription validates a PR description string. @@ -233,7 +240,7 @@ func ValidatePRDescription(body string) (Result, error) { } } - return result, nil + return finalizeResult(result), nil } // ValidateRollbackPlan validates a rollback plan JSON. @@ -271,7 +278,7 @@ func ValidateRollbackPlan(body string) (Result, error) { } } - return result, nil + return finalizeResult(result), nil } // ValidateEvidence validates a production release evidence bundle. @@ -298,7 +305,7 @@ func ValidateEvidence(body string) (Result, error) { result.Warnings = append(result.Warnings, "evidence bundle environment should be 'production'") } - return result, nil + return finalizeResult(result), nil } // ValidateReleaseReadiness validates a release readiness report JSON. @@ -327,7 +334,7 @@ func ValidateReleaseReadiness(body string) (Result, error) { } } - return result, nil + return finalizeResult(result), nil } // ValidateAIReview validates an AI change review record against the ODS schema. @@ -361,7 +368,7 @@ func ValidateAIReview(body string) (Result, error) { // validate outcome if outcome, ok := review["outcome"].(string); ok { validOutcomes := map[string]bool{ - "approved": true, + "approved": true, "approved_with_changes": true, "changes_requested": true, "blocked": true, @@ -441,7 +448,7 @@ func ValidateAIReview(body string) (Result, error) { } } - return result, nil + return finalizeResult(result), nil } // ValidateApprovalPolicy validates an approval policy JSON. @@ -471,5 +478,5 @@ func ValidateApprovalPolicy(body string) (Result, error) { } } - return result, nil + return finalizeResult(result), nil } diff --git a/internal/validator/validator_test.go b/internal/validator/validator_test.go new file mode 100644 index 0000000..528018f --- /dev/null +++ b/internal/validator/validator_test.go @@ -0,0 +1,183 @@ +package validator + +import "testing" + +func TestValidateBranch(t *testing.T) { + tests := []struct { + name string + want ValidationStatus + errors int + }{ + {name: "feature/add-oauth-login", want: StatusConformant}, + {name: "main", want: StatusConformant}, + {name: "feature/ai-generated-client", want: StatusConformantWarnings}, + {name: "feature/Add_OAuth", want: StatusNonConformant, errors: 3}, + {name: "unknown/add-oauth-login", want: StatusNonConformant, errors: 1}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got, err := ValidateBranch(tt.name) + if err != nil { + t.Fatalf("ValidateBranch() error = %v", err) + } + if got.Status != tt.want { + t.Fatalf("ValidateBranch() status = %s, want %s; result = %+v", got.Status, tt.want, got) + } + if len(got.Errors) < tt.errors { + t.Fatalf("ValidateBranch() errors = %d, want at least %d; result = %+v", len(got.Errors), tt.errors, got) + } + }) + } +} + +func TestValidateCommitMessage(t *testing.T) { + tests := []struct { + name string + msg string + want ValidationStatus + }{ + { + name: "plain conventional commit", + msg: "feat(auth): add oauth login", + want: StatusConformant, + }, + { + name: "AI assisted commit with tool", + msg: `feat(auth): add oauth login + +AI-assisted: true +AI-tool: GitHub Copilot +AI-scope: auth module`, + want: StatusConformant, + }, + { + name: "AI assisted commit missing tool", + msg: `feat(auth): add oauth login + +AI-assisted: true`, + want: StatusNonConformant, + }, + { + name: "breaking change warning", + msg: `feat(auth)!: replace session format + +BREAKING CHANGE: session tokens must be rotated`, + want: StatusConformantWarnings, + }, + { + name: "invalid type", + msg: "feature(auth): add oauth login", + want: StatusNonConformant, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got, err := ValidateCommitMessage(tt.msg) + if err != nil { + t.Fatalf("ValidateCommitMessage() error = %v", err) + } + if got.Status != tt.want { + t.Fatalf("ValidateCommitMessage() status = %s, want %s; result = %+v", got.Status, tt.want, got) + } + }) + } +} + +func TestValidatePRDescription(t *testing.T) { + valid := `## Summary +Add OAuth login. + +## Type +- [x] Feature + +## AI Disclosure +- [x] This PR contains AI-generated code +- AI Tool: GitHub Copilot + +## Changes +- Added provider integration. + +## Testing +- Unit tests added. + +## Checklist +- [x] Branch follows ODS.` + + got, err := ValidatePRDescription(valid) + if err != nil { + t.Fatalf("ValidatePRDescription() error = %v", err) + } + if got.Status != StatusConformant { + t.Fatalf("ValidatePRDescription() status = %s, want %s; result = %+v", got.Status, StatusConformant, got) + } + + missingTool := `## Summary +Add OAuth login. + +## Type +- [x] Feature + +## AI Disclosure +- [x] This PR contains AI-generated code + +## Changes +- Added provider integration. + +## Testing +- Unit tests added. + +## Checklist +- [x] Branch follows ODS.` + + got, err = ValidatePRDescription(missingTool) + if err != nil { + t.Fatalf("ValidatePRDescription() error = %v", err) + } + if got.Status != StatusNonConformant { + t.Fatalf("ValidatePRDescription() status = %s, want %s; result = %+v", got.Status, StatusNonConformant, got) + } +} + +func TestValidateAIReview(t *testing.T) { + valid := `{ + "pr_number": 42, + "review_level": "L2", + "ai_contribution_percentage": 65, + "reviewer": "jane-doe", + "timestamp": "2026-05-25T10:00:00Z", + "outcome": "approved", + "checklist_results": { + "correctness": { "passed": true, "issues": 0 }, + "security": { "passed": true, "issues": 0 }, + "ai_specific": { "passed": true, "issues": 0 }, + "quality": { "passed": true, "issues": 0 } + } +}` + + got, err := ValidateAIReview(valid) + if err != nil { + t.Fatalf("ValidateAIReview() error = %v", err) + } + if got.Status != StatusConformant { + t.Fatalf("ValidateAIReview() status = %s, want %s; result = %+v", got.Status, StatusConformant, got) + } + + invalid := `{ + "pr_number": 42, + "review_level": "L4", + "reviewer": "jane-doe", + "timestamp": "2026-05-25T10:00:00Z", + "outcome": "pending", + "checklist_results": {} +}` + + got, err = ValidateAIReview(invalid) + if err != nil { + t.Fatalf("ValidateAIReview() error = %v", err) + } + if got.Status != StatusNonConformant { + t.Fatalf("ValidateAIReview() status = %s, want %s; result = %+v", got.Status, StatusNonConformant, got) + } +}