Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions cmd/bd/close.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,11 +8,13 @@ import (
"time"

"github.com/spf13/cobra"
"github.com/steveyegge/beads/internal/config"
"github.com/steveyegge/beads/internal/hooks"
"github.com/steveyegge/beads/internal/storage/dolt"
"github.com/steveyegge/beads/internal/types"
"github.com/steveyegge/beads/internal/ui"
"github.com/steveyegge/beads/internal/utils"
"github.com/steveyegge/beads/internal/validation"
)

var closeCmd = &cobra.Command{
Expand Down Expand Up @@ -60,6 +62,19 @@ create, update, show, or close operation).`,
if reason == "" {
reason = "Closed"
}

// Validate close reason if configured
closeValidation := config.GetString("validation.on-close")
if closeValidation == "error" || closeValidation == "warn" {
if err := validation.ValidateCloseReason(reason); err != nil {
if closeValidation == "error" {
FatalErrorRespectJSON("%v", err)
}
// warn mode: print warning but proceed
fmt.Fprintf(os.Stderr, "%s %v\n", ui.RenderWarn("⚠"), err)
}
}

force, _ := cmd.Flags().GetBool("force")
continueFlag, _ := cmd.Flags().GetBool("continue")
noAuto, _ := cmd.Flags().GetBool("no-auto")
Expand Down
2 changes: 1 addition & 1 deletion cmd/bd/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -290,7 +290,7 @@ func showConfigYAMLOverrides(dbConfig map[string]string) {
"sync.mode", "sync.git-remote", "no-push", "no-git-ops",
"git.author", "git.no-gpg-sign",
"create.require-description",
"validation.on-create", "validation.on-sync",
"validation.on-create", "validation.on-close", "validation.on-sync",
"hierarchy.max-depth",
"dolt.idle-timeout",
}
Expand Down
41 changes: 19 additions & 22 deletions cmd/bd/create.go
Original file line number Diff line number Diff line change
Expand Up @@ -240,6 +240,25 @@ var createCmd = &cobra.Command{
metadata = json.RawMessage(metadataJSON)
}

// Validate template based on --validate flag or config
// Uses LintIssue for field-aware validation: checks --acceptance field too (GH#2468 parity)
validateTemplate, _ := cmd.Flags().GetBool("validate")
validationMode := config.GetString("validation.on-create")
if validateTemplate || validationMode == "error" || validationMode == "warn" {
lintIssue := &types.Issue{
IssueType: types.IssueType(issueType).Normalize(),
Description: description,
AcceptanceCriteria: acceptance,
}
if err := validation.LintIssue(lintIssue); err != nil {
if validateTemplate || validationMode == "error" {
FatalError("%v", err)
}
// warn mode: print warning but proceed
fmt.Fprintf(os.Stderr, "%s %v\n", ui.RenderWarn("⚠"), err)
}
}

// Handle --dry-run flag (before --rig to ensure it works with cross-rig creation)
dryRun, _ := cmd.Flags().GetBool("dry-run")
if dryRun {
Expand Down Expand Up @@ -370,28 +389,6 @@ var createCmd = &cobra.Command{
estimatedMinutes = &est
}

// Validate template based on --validate flag or config
validateTemplate, _ := cmd.Flags().GetBool("validate")
if validateTemplate {
// Explicit --validate flag: fail on error
if err := validation.ValidateTemplate(types.IssueType(issueType), description); err != nil {
FatalError("%v", err)
}
} else {
// Check validation.on-create config (bd-t7jq)
validationMode := config.GetString("validation.on-create")
if validationMode == "error" || validationMode == "warn" {
if err := validation.ValidateTemplate(types.IssueType(issueType), description); err != nil {
if validationMode == "error" {
FatalError("%v", err)
} else {
// warn mode: print warning but proceed
fmt.Fprintf(os.Stderr, "%s %v\n", ui.RenderWarn("⚠"), err)
}
}
}
}

// Use global jsonOutput set by PersistentPreRun

// Determine target repository using routing logic
Expand Down
10 changes: 10 additions & 0 deletions cmd/bd/testdata/close_errors.txt
Original file line number Diff line number Diff line change
Expand Up @@ -9,3 +9,13 @@ stderr 'no issue found'
# Close with no args and no last-touched should fail
! bd close
stderr 'no issue ID provided'

# validation.on-close=error should reject terse close reasons
bd create 'Close validation test' --type chore --description 'Testing close reason validation'
bd config set validation.on-close error
exec sh -c 'ID=$(bd list --json | grep -oE "test-[a-z0-9]+" | head -1) && ! bd close "$ID" --reason "done" 2>&1 | grep -q "terse"'

# validation.on-close=warn should warn but still close
bd config set validation.on-close warn
bd create 'Close warn test' --type chore --description 'Testing close reason warn mode'
exec sh -c 'ID=$(bd list --status open --json | grep -oE "test-[a-z0-9]+" | head -1) && bd close "$ID" --reason "done" 2>&1 | grep -q "terse"'
12 changes: 12 additions & 0 deletions cmd/bd/testdata/create_errors.txt
Original file line number Diff line number Diff line change
Expand Up @@ -17,3 +17,15 @@ stderr 'invalid issue type'
# Create with invalid priority should fail
! bd create 'Test' --priority 99
stderr 'invalid priority'

# --validate should fail without acceptance criteria for task
! bd create 'Missing acceptance' --type task --description 'No criteria here' --validate
stderr 'Acceptance Criteria'

# --validate should pass when --acceptance field provides criteria (GH#2468 parity)
bd create 'Has acceptance' --type task --description 'Do the thing' --acceptance 'It works' --validate
stdout 'Created issue:'

# --validate + --dry-run should still validate (not bypass)
! bd create 'Dryrun validate' --type bug --description 'No sections' --validate --dry-run
stderr 'Steps to Reproduce'
1 change: 1 addition & 0 deletions docs/CONFIG.md
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ Tool-level settings you can configure:
| `dolt.auto-commit` | `--dolt-auto-commit` | `BD_DOLT_AUTO_COMMIT` | `on` | (Dolt backend) Automatically create a Dolt commit after successful write commands |
| `create.require-description` | - | `BD_CREATE_REQUIRE_DESCRIPTION` | `false` | Require description when creating issues |
| `validation.on-create` | - | `BD_VALIDATION_ON_CREATE` | `none` | Template validation on create: `none`, `warn`, `error` |
| `validation.on-close` | - | `BD_VALIDATION_ON_CLOSE` | `none` | Close reason quality check: `none`, `warn`, `error` |
| `validation.on-sync` | - | `BD_VALIDATION_ON_SYNC` | `none` | Template validation before sync: `none`, `warn`, `error` |
| `git.author` | - | `BD_GIT_AUTHOR` | (none) | Override commit author for beads commits |
| `git.no-gpg-sign` | - | `BD_GIT_NO_GPG_SIGN` | `false` | Disable GPG signing for beads commits |
Expand Down
1 change: 1 addition & 0 deletions internal/config/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -158,6 +158,7 @@ func Initialize() error {
// - "warn": validate and print warnings but proceed
// - "error": validate and fail on missing sections
v.SetDefault("validation.on-create", "none")
v.SetDefault("validation.on-close", "none")
v.SetDefault("validation.on-sync", "none")

// Metadata schema validation (GH#1416 Phase 2)
Expand Down
9 changes: 9 additions & 0 deletions internal/config/config_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1060,6 +1060,11 @@ func TestValidationConfigDefaults(t *testing.T) {
t.Errorf("GetString(validation.on-create) = %q, want \"none\"", got)
}

// Test validation.on-close default is "none"
if got := GetString("validation.on-close"); got != "none" {
t.Errorf("GetString(validation.on-close) = %q, want \"none\"", got)
}

// Test validation.on-sync default is "none"
if got := GetString("validation.on-sync"); got != "none" {
t.Errorf("GetString(validation.on-sync) = %q, want \"none\"", got)
Expand All @@ -1074,6 +1079,7 @@ func TestValidationConfigFromFile(t *testing.T) {
configContent := `
validation:
on-create: error
on-close: warn
on-sync: warn
`
beadsDir := filepath.Join(tmpDir, ".beads")
Expand All @@ -1098,6 +1104,9 @@ validation:
if got := GetString("validation.on-create"); got != "error" {
t.Errorf("GetString(validation.on-create) = %q, want \"error\"", got)
}
if got := GetString("validation.on-close"); got != "warn" {
t.Errorf("GetString(validation.on-close) = %q, want \"warn\"", got)
}
if got := GetString("validation.on-sync"); got != "warn" {
t.Errorf("GetString(validation.on-sync) = %q, want \"warn\"", got)
}
Expand Down
1 change: 1 addition & 0 deletions internal/config/yaml_config.go
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ var YamlOnlyKeys = map[string]bool{
// Validation settings (bd-t7jq)
// Values: "warn" | "error" | "none"
"validation.on-create": true,
"validation.on-close": true,
"validation.on-sync": true,

// Hierarchy settings (GH#995)
Expand Down
13 changes: 13 additions & 0 deletions internal/validation/template.go
Original file line number Diff line number Diff line change
Expand Up @@ -110,3 +110,16 @@ func LintIssue(issue *types.Issue) error {
templateErr.Missing = remaining
return templateErr
}

// ValidateCloseReason checks if a close reason meets minimum quality standards.
// Returns nil if the reason is acceptable. Used by validation.on-close config.
func ValidateCloseReason(reason string) error {
reason = strings.TrimSpace(reason)
if reason == "" || strings.EqualFold(reason, "closed") {
return fmt.Errorf("close reason is empty or default; provide a summary of what was done")
}
if len(reason) < 20 {
return fmt.Errorf("close reason is terse (%d chars); aim for 20+ characters describing what was done", len(reason))
}
return nil
}
58 changes: 58 additions & 0 deletions internal/validation/template_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -307,3 +307,61 @@ func TestLintIssue(t *testing.T) {
})
}
}

func TestValidateCloseReason(t *testing.T) {
tests := []struct {
name string
reason string
wantErr bool
}{
{
name: "empty reason",
reason: "",
wantErr: true,
},
{
name: "default reason",
reason: "Closed",
wantErr: true,
},
{
name: "default reason case insensitive",
reason: "closed",
wantErr: true,
},
{
name: "terse reason",
reason: "fixed the bug",
wantErr: true,
},
{
name: "adequate reason",
reason: "Fixed the login timeout by increasing session TTL to 24h",
wantErr: false,
},
{
name: "exactly 20 chars",
reason: "12345678901234567890",
wantErr: false,
},
{
name: "whitespace padded but terse",
reason: " done ",
wantErr: true,
},
{
name: "whitespace padded but adequate",
reason: " Fixed the auth flow for SSO users ",
wantErr: false,
},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
err := ValidateCloseReason(tt.reason)
if (err != nil) != tt.wantErr {
t.Errorf("ValidateCloseReason(%q) error = %v, wantErr %v", tt.reason, err, tt.wantErr)
}
})
}
}