diff --git a/CHANGELOG.md b/CHANGELOG.md index 59afa7d..b177bfc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,18 @@ All notable changes to nightshift are documented in this file. +## [Unreleased] + +### Features +- **Commit message normalizer** — new `internal/commitmsg` package plus a + `nightshift commit-msg` subcommand (`--check`, `--fix`, `--print-spec`, + `--quiet`) that validates and rewrites commit messages against one + Conventional Commits format. `make install-hooks` now also installs a + `commit-msg` git hook, and the orchestrator's plan and implement prompts + quote the same canonical spec so autonomous agents write messages the hook + accepts. Messages git generates itself — merges, reverts and + `fixup!`/`squash!`/`amend!` commits — are exempt from the format. + ## [v0.3.3] - 2026-02-19 ### Features diff --git a/Makefile b/Makefile index 088be01..5bf99a2 100644 --- a/Makefile +++ b/Makefile @@ -75,10 +75,12 @@ help: @echo " check - Run tests and lint" @echo " install - Build and install to Go bin directory" @echo " calibrate-providers - Compare local Claude/Codex session usage for calibration" - @echo " install-hooks - Install git pre-commit hook" + @echo " install-hooks - Install git pre-commit and commit-msg hooks" @echo " help - Show this help" -# Install git pre-commit hook +# Install git hooks install-hooks: @ln -sf ../../scripts/pre-commit.sh .git/hooks/pre-commit @echo "✓ pre-commit hook installed (.git/hooks/pre-commit → scripts/pre-commit.sh)" + @ln -sf ../../scripts/commit-msg.sh .git/hooks/commit-msg + @echo "✓ commit-msg hook installed (.git/hooks/commit-msg → scripts/commit-msg.sh)" diff --git a/README.md b/README.md index 84f92cd..74b07f2 100644 --- a/README.md +++ b/README.md @@ -273,6 +273,38 @@ This symlinks `scripts/pre-commit.sh` into `.git/hooks/pre-commit`. The hook run To bypass in a pinch: `git commit --no-verify` +### Commit message format + +Commits follow [Conventional Commits](https://www.conventionalcommits.org/): + +``` +type(scope)!: subject + +Body, wrapped at 72 columns. + +Trailer-Key: value +``` + +- `type` is lowercase and one of `feat`, `fix`, `docs`, `style`, `refactor`, + `perf`, `test`, `build`, `ci`, `chore`, `revert` +- the whole header line is at most 72 characters, imperative mood, no + trailing period +- trailers form one block at the very end +- messages git writes itself are exempt — headers starting with `Merge `, + `Revert "`, `fixup! `, `squash! ` or `amend! ` always pass, so `git merge` + and `git rebase --autosquash` are unaffected + +`make install-hooks` also installs a `commit-msg` hook that enforces this via +`nightshift commit-msg --check`. You can run the checker yourself: + +```bash +nightshift commit-msg --print-spec # Show the full format +nightshift commit-msg --check .git/COMMIT_EDITMSG +nightshift commit-msg --fix .git/COMMIT_EDITMSG # Rewrite it for you +``` + +To bypass in a pinch: `git commit --no-verify` + ## Uninstalling ```bash diff --git a/cmd/nightshift/commands/commitmsg.go b/cmd/nightshift/commands/commitmsg.go new file mode 100644 index 0000000..3ee6db9 --- /dev/null +++ b/cmd/nightshift/commands/commitmsg.go @@ -0,0 +1,150 @@ +package commands + +import ( + "fmt" + "io" + "os" + + "github.com/spf13/cobra" + + "github.com/marcus/nightshift/internal/commitmsg" +) + +// commitMsgOptions holds the parsed flags for `nightshift commit-msg`. +type commitMsgOptions struct { + check bool + fix bool + printSpec bool + quiet bool +} + +func newCommitMsgCmd() *cobra.Command { + opts := &commitMsgOptions{} + + cmd := &cobra.Command{ + Use: "commit-msg [file|-]", + Short: "Check or normalize a commit message", + Long: `Check a commit message against the repository's commit format, or +rewrite it into that format. + +Reads the message from FILE, or from stdin when FILE is "-" or omitted. +This is what the commit-msg git hook runs; install it with: + + make install-hooks + +Examples: + nightshift commit-msg --print-spec + nightshift commit-msg --check .git/COMMIT_EDITMSG + nightshift commit-msg --fix .git/COMMIT_EDITMSG + printf 'Fixed the thing.' | nightshift commit-msg --fix -`, + Args: cobra.MaximumNArgs(1), + SilenceUsage: true, + RunE: func(cmd *cobra.Command, args []string) error { + path := "-" + if len(args) == 1 { + path = args[0] + } + return runCommitMsg(path, opts, cmd.InOrStdin(), cmd.OutOrStdout(), cmd.ErrOrStderr()) + }, + } + + cmd.Flags().BoolVar(&opts.check, "check", false, "Validate only; exit non-zero when the message has errors") + cmd.Flags().BoolVar(&opts.fix, "fix", false, "Rewrite the message into the canonical format") + cmd.Flags().BoolVar(&opts.printSpec, "print-spec", false, "Print the commit message format and exit") + cmd.Flags().BoolVar(&opts.quiet, "quiet", false, "Suppress issue output; rely on the exit code") + + return cmd +} + +func init() { + rootCmd.AddCommand(newCommitMsgCmd()) +} + +func runCommitMsg(path string, opts *commitMsgOptions, stdin io.Reader, stdout, stderr io.Writer) error { + if opts.printSpec { + fmt.Fprint(stdout, commitmsg.Spec()) + return nil + } + if opts.check && opts.fix { + return fmt.Errorf("--check and --fix are mutually exclusive") + } + + raw, err := readCommitMsg(path, stdin) + if err != nil { + return err + } + + label := path + if label == "-" { + label = "" + } + + if opts.fix { + fixed, issues, err := commitmsg.Normalize(raw, commitMsgValidationOptions()) + if err != nil { + return fmt.Errorf("%s: %w\n\n%s", label, err, commitmsg.Spec()) + } + if path == "-" { + fmt.Fprint(stdout, fixed) + } else if fixed != raw { + if err := os.WriteFile(path, []byte(fixed), 0o644); err != nil { + return fmt.Errorf("writing %s: %w", path, err) + } + } + reportCommitMsgIssues(stderr, label, issues, opts.quiet) + if commitmsg.HasErrors(issues) { + return fmt.Errorf("%s: commit message still has errors after --fix", label) + } + return nil + } + + // Default behaviour, with or without an explicit --check, is to validate. + msg, err := commitmsg.Parse(raw) + if err != nil { + return fmt.Errorf("%s: %w", label, err) + } + issues := commitmsg.ValidateWithOptions(msg, commitMsgValidationOptions()) + reportCommitMsgIssues(stderr, label, issues, opts.quiet) + if commitmsg.HasErrors(issues) { + if !opts.quiet { + fmt.Fprintf(stderr, "\n%s", commitmsg.Spec()) + } + return fmt.Errorf("%s: commit message does not match the required format", label) + } + return nil +} + +// commitMsgValidationOptions returns the validation options the CLI enforces. Nightshift +// trailers are not required of humans, so the defaults are used as-is. +func commitMsgValidationOptions() commitmsg.Options { + return commitmsg.DefaultOptions() +} + +func readCommitMsg(path string, stdin io.Reader) (string, error) { + if path == "-" { + data, err := io.ReadAll(stdin) + if err != nil { + return "", fmt.Errorf("reading stdin: %w", err) + } + return string(data), nil + } + data, err := os.ReadFile(path) + if err != nil { + return "", fmt.Errorf("reading %s: %w", path, err) + } + return string(data), nil +} + +// reportCommitMsgIssues prints issues as "file:line: severity: message (rule)". +func reportCommitMsgIssues(w io.Writer, label string, issues []commitmsg.Issue, quiet bool) { + if quiet { + return + } + for _, i := range issues { + line := "-" + if i.Line > 0 { + line = fmt.Sprintf("%d", i.Line) + } + fmt.Fprintf(w, "%s:%s: %s: %s (%s)\n", label, line, i.Severity, i.Message, i.Rule) + } +} diff --git a/cmd/nightshift/commands/commitmsg_test.go b/cmd/nightshift/commands/commitmsg_test.go new file mode 100644 index 0000000..ce46fee --- /dev/null +++ b/cmd/nightshift/commands/commitmsg_test.go @@ -0,0 +1,200 @@ +package commands + +import ( + "bytes" + "os" + "path/filepath" + "strings" + "testing" +) + +func writeTempMsg(t *testing.T, content string) string { + t.Helper() + path := filepath.Join(t.TempDir(), "COMMIT_EDITMSG") + if err := os.WriteFile(path, []byte(content), 0o644); err != nil { + t.Fatalf("writing temp message: %v", err) + } + return path +} + +func TestRunCommitMsg_PrintSpec(t *testing.T) { + var out, errBuf bytes.Buffer + if err := runCommitMsg("-", &commitMsgOptions{printSpec: true}, strings.NewReader(""), &out, &errBuf); err != nil { + t.Fatalf("runCommitMsg: %v", err) + } + if !strings.Contains(out.String(), "type(scope)") { + t.Errorf("--print-spec output missing the format:\n%s", out.String()) + } +} + +func TestRunCommitMsg_CheckPassesOnGoodFile(t *testing.T) { + path := writeTempMsg(t, "feat(cli): add commit message normalizer\n\nA short body.\n") + var out, errBuf bytes.Buffer + if err := runCommitMsg(path, &commitMsgOptions{check: true}, strings.NewReader(""), &out, &errBuf); err != nil { + t.Fatalf("runCommitMsg: %v\nstderr: %s", err, errBuf.String()) + } + if errBuf.Len() != 0 { + t.Errorf("unexpected diagnostics: %s", errBuf.String()) + } +} + +func TestRunCommitMsg_CheckFailsOnBadFile(t *testing.T) { + path := writeTempMsg(t, "Fixed the thing.\n") + var out, errBuf bytes.Buffer + err := runCommitMsg(path, &commitMsgOptions{check: true}, strings.NewReader(""), &out, &errBuf) + if err == nil { + t.Fatalf("expected an error for a malformed message") + } + if !strings.Contains(errBuf.String(), "header-format") { + t.Errorf("expected header-format diagnostic, got:\n%s", errBuf.String()) + } + if !strings.Contains(errBuf.String(), path+":1:") { + t.Errorf("expected file:line prefixed diagnostics, got:\n%s", errBuf.String()) + } + if !strings.Contains(errBuf.String(), "type(scope)") { + t.Errorf("expected the spec to be printed on failure, got:\n%s", errBuf.String()) + } +} + +func TestRunCommitMsg_CheckQuietSuppressesOutput(t *testing.T) { + path := writeTempMsg(t, "Fixed the thing.\n") + var out, errBuf bytes.Buffer + if err := runCommitMsg(path, &commitMsgOptions{check: true, quiet: true}, strings.NewReader(""), &out, &errBuf); err == nil { + t.Fatalf("expected an error for a malformed message") + } + if errBuf.Len() != 0 { + t.Errorf("--quiet should suppress diagnostics, got:\n%s", errBuf.String()) + } +} + +func TestRunCommitMsg_FixRewritesFileInPlace(t *testing.T) { + path := writeTempMsg(t, "Fixed bug in the parser.\n# a comment git added\n") + var out, errBuf bytes.Buffer + if err := runCommitMsg(path, &commitMsgOptions{fix: true}, strings.NewReader(""), &out, &errBuf); err != nil { + t.Fatalf("runCommitMsg: %v\nstderr: %s", err, errBuf.String()) + } + got, err := os.ReadFile(path) + if err != nil { + t.Fatalf("reading fixed file: %v", err) + } + if string(got) != "fix: fixed bug in the parser\n" { + t.Errorf("fixed file = %q", string(got)) + } + // The rewritten message must now pass --check. + if err := runCommitMsg(path, &commitMsgOptions{check: true, quiet: true}, strings.NewReader(""), &out, &errBuf); err != nil { + t.Errorf("fixed message still fails --check: %v", err) + } +} + +func TestRunCommitMsg_FixFromStdinWritesToStdout(t *testing.T) { + var out, errBuf bytes.Buffer + err := runCommitMsg("-", &commitMsgOptions{fix: true}, strings.NewReader("Fixed bug in the parser."), &out, &errBuf) + if err != nil { + t.Fatalf("runCommitMsg: %v\nstderr: %s", err, errBuf.String()) + } + if out.String() != "fix: fixed bug in the parser\n" { + t.Errorf("stdout = %q", out.String()) + } +} + +func TestRunCommitMsg_CheckFromStdin(t *testing.T) { + var out, errBuf bytes.Buffer + err := runCommitMsg("-", &commitMsgOptions{check: true}, strings.NewReader("Fixed the thing."), &out, &errBuf) + if err == nil { + t.Fatalf("expected an error for a malformed message") + } + if !strings.Contains(errBuf.String(), ":1:") { + t.Errorf("expected label, got:\n%s", errBuf.String()) + } +} + +func TestRunCommitMsg_FixReportsUninferrableType(t *testing.T) { + var out, errBuf bytes.Buffer + err := runCommitMsg("-", &commitMsgOptions{fix: true}, strings.NewReader("zzzqqq wibble frobnicate"), &out, &errBuf) + if err == nil { + t.Fatalf("expected an error when no type can be inferred") + } + if !strings.Contains(err.Error(), "cannot infer") { + t.Errorf("error = %v, want it to explain the inference failure", err) + } +} + +func TestRunCommitMsg_CheckAndFixAreExclusive(t *testing.T) { + var out, errBuf bytes.Buffer + err := runCommitMsg("-", &commitMsgOptions{check: true, fix: true}, strings.NewReader("fix: a thing"), &out, &errBuf) + if err == nil || !strings.Contains(err.Error(), "mutually exclusive") { + t.Errorf("err = %v, want a mutual-exclusion error", err) + } +} + +func TestRunCommitMsg_MissingFile(t *testing.T) { + var out, errBuf bytes.Buffer + err := runCommitMsg(filepath.Join(t.TempDir(), "nope"), &commitMsgOptions{check: true}, strings.NewReader(""), &out, &errBuf) + if err == nil { + t.Fatalf("expected an error for a missing file") + } +} + +func TestCommitMsgCmd_EndToEnd(t *testing.T) { + path := writeTempMsg(t, "Fixed bug in the parser.\n") + + cmd := newCommitMsgCmd() + var out, errBuf bytes.Buffer + cmd.SetOut(&out) + cmd.SetErr(&errBuf) + cmd.SetArgs([]string{"--fix", path}) + if err := cmd.Execute(); err != nil { + t.Fatalf("Execute: %v\nstderr: %s", err, errBuf.String()) + } + + got, err := os.ReadFile(path) + if err != nil { + t.Fatalf("reading fixed file: %v", err) + } + if string(got) != "fix: fixed bug in the parser\n" { + t.Errorf("fixed file = %q", string(got)) + } +} + +func TestRunCommitMsg_AcceptsGitGeneratedMessages(t *testing.T) { + for _, raw := range []string{ + "Merge branch 'side'\n", + "Merge branch 'side'\n\n# Conflicts:\n#\tfile.go\n", + "Revert \"feat(cli): add commit message normalizer\"\n\nThis reverts commit deadbeef.\n", + "fixup! feat(cli): add commit message normalizer\n", + "squash! feat(cli): add commit message normalizer\n", + "amend! feat(cli): add commit message normalizer\n", + } { + path := writeTempMsg(t, raw) + var out, errBuf bytes.Buffer + if err := runCommitMsg(path, &commitMsgOptions{check: true}, strings.NewReader(""), &out, &errBuf); err != nil { + t.Errorf("check %q: %v\nstderr: %s", raw, err, errBuf.String()) + } + } +} + +func TestRunCommitMsg_FixLeavesGitGeneratedAlone(t *testing.T) { + raw := "Merge branch 'side' into main\n" + path := writeTempMsg(t, raw) + var out, errBuf bytes.Buffer + if err := runCommitMsg(path, &commitMsgOptions{fix: true}, strings.NewReader(""), &out, &errBuf); err != nil { + t.Fatalf("fix: %v\nstderr: %s", err, errBuf.String()) + } + got, err := os.ReadFile(path) + if err != nil { + t.Fatalf("reading file: %v", err) + } + if string(got) != raw { + t.Errorf("fixed file = %q, want it unchanged", string(got)) + } +} + +func TestRunCommitMsg_PrintSpecMentionsGitGeneratedExemption(t *testing.T) { + var out, errBuf bytes.Buffer + if err := runCommitMsg("-", &commitMsgOptions{printSpec: true}, strings.NewReader(""), &out, &errBuf); err != nil { + t.Fatalf("print-spec: %v", err) + } + if !strings.Contains(out.String(), "fixup! ") || !strings.Contains(out.String(), "Merge ") { + t.Errorf("spec does not document the git-generated exemption:\n%s", out.String()) + } +} diff --git a/internal/commitmsg/commitmsg.go b/internal/commitmsg/commitmsg.go new file mode 100644 index 0000000..3f3ea69 --- /dev/null +++ b/internal/commitmsg/commitmsg.go @@ -0,0 +1,676 @@ +// Package commitmsg parses, validates and normalizes git commit messages +// against a single Conventional-Commits-style format. +// +// The canonical, human-readable description of that format lives in spec.go +// so the CLI, the git hook, the docs and the agent prompts all quote one +// source of truth. This package uses the standard library only. +package commitmsg + +import ( + "errors" + "fmt" + "regexp" + "strings" + "unicode" +) + +// Severity classifies how badly an issue breaks the format. +type Severity int + +const ( + // SeverityWarning marks a stylistic problem that does not fail --check. + SeverityWarning Severity = iota + // SeverityError marks a problem that fails --check. + SeverityError +) + +// String renders the severity as a lowercase word. +func (s Severity) String() string { + if s == SeverityError { + return "error" + } + return "warning" +} + +// Issue is a single validation finding. +type Issue struct { + // Line is the 1-based line of the normalized message the issue refers to. + // Zero means "the message as a whole". + Line int + Severity Severity + Rule string + Message string +} + +// String renders the issue as "line N: severity: message (rule)". +func (i Issue) String() string { + return fmt.Sprintf("line %d: %s: %s (%s)", i.Line, i.Severity, i.Message, i.Rule) +} + +// Trailer is a single git trailer, e.g. "Nightshift-Task: commit-normalize". +type Trailer struct { + Key string + Value string +} + +// String renders the trailer as "Key: Value". +func (t Trailer) String() string { return t.Key + ": " + t.Value } + +// Message is a parsed commit message. +type Message struct { + Type string + Scope string + Breaking bool + Subject string + Body string + Trailers []Trailer + + // GitGenerated marks a message git wrote itself — a merge, a revert, or a + // fixup!/squash!/amend! commit. Those headers are fixed by git and are + // exempt from every rule below. + GitGenerated bool + + // rawHeader is the header line exactly as written, used to report issues + // about headers that do not match the conventional format at all. + rawHeader string + // noBlankAfterSubject records that the source message ran the body + // straight into the subject line. + noBlankAfterSubject bool +} + +// Options controls validation and normalization. +type Options struct { + // AllowedTypes are the accepted commit types, in canonical order. + AllowedTypes []string + // MaxSubjectLen caps the length of the whole header line. + MaxSubjectLen int + // WrapBody is the column body paragraphs are wrapped at. + WrapBody int + // RequiredTrailers must be present; Normalize appends missing ones. + RequiredTrailers []Trailer +} + +// DefaultOptions returns the format Nightshift enforces on itself. +func DefaultOptions() Options { + return Options{ + AllowedTypes: append([]string(nil), AllowedTypes...), + MaxSubjectLen: MaxSubjectLen, + WrapBody: WrapBody, + } +} + +var ( + headerRe = regexp.MustCompile(`^([A-Za-z][A-Za-z0-9]*)(?:\(([^()]+)\))?(!)?:[ \t]*(.*)$`) + // Git trailer syntax: "Token: value", token may contain letters, digits + // and hyphens. "BREAKING CHANGE" is allowed as a special case. + trailerRe = regexp.MustCompile(`^(BREAKING CHANGE|[A-Za-z][A-Za-z0-9-]*):[ \t]+(.*\S)[ \t]*$`) + // Headers git generates on the user's behalf. Enforcing the conventional + // format on these would break `git merge --no-ff` (which aborts mid-merge + // when commit-msg fails) and every `git commit --fixup`/`--squash`, and so + // `git rebase --autosquash`. + gitGeneratedRe = regexp.MustCompile(`^(Merge |Revert "|fixup! |squash! |amend! )`) +) + +// IsGitGenerated reports whether a header line was written by git itself — +// a merge, a revert, or a fixup!/squash!/amend! commit — and is therefore +// exempt from the commit format. +func IsGitGenerated(header string) bool { + return gitGeneratedRe.MatchString(header) +} + +// knownTrailerKeys are hoisted out of the body even when they appear in the +// middle of a message, so every message ends with one trailer block. +var knownTrailerKeys = map[string]bool{ + "nightshift-task": true, + "nightshift-ref": true, + "co-authored-by": true, + "signed-off-by": true, + "reviewed-by": true, + "acked-by": true, + "tested-by": true, + "reported-by": true, + "refs": true, + "fixes": true, + "closes": true, + "breaking change": true, +} + +// ErrEmptyMessage is returned when nothing but comments or whitespace remains. +var ErrEmptyMessage = errors.New("commit message is empty") + +// Parse reads a raw commit message — including one straight out of a +// COMMIT_EDITMSG file, with git's comment lines and verbose diff attached — +// into a Message. A header that does not follow the conventional format is +// not an error: it parses with an empty Type so Validate can report it. +func Parse(raw string) (*Message, error) { + lines := cleanLines(raw) + if len(lines) == 0 { + return nil, ErrEmptyMessage + } + + m := &Message{rawHeader: lines[0], GitGenerated: IsGitGenerated(lines[0])} + if match := headerRe.FindStringSubmatch(lines[0]); match != nil { + m.Type = match[1] + m.Scope = match[2] + m.Breaking = match[3] == "!" + m.Subject = strings.TrimSpace(match[4]) + } else { + m.Subject = strings.TrimSpace(lines[0]) + } + + rest := lines[1:] + if len(rest) > 0 && strings.TrimSpace(rest[0]) != "" { + m.noBlankAfterSubject = true + } + for len(rest) > 0 && strings.TrimSpace(rest[0]) == "" { + rest = rest[1:] + } + + bodyLines, trailers := splitTrailers(rest) + m.Body = strings.Join(bodyLines, "\n") + m.Body = strings.Trim(m.Body, "\n") + m.Trailers = trailers + return m, nil +} + +// cleanLines strips git's comment lines and any verbose diff, and trims +// trailing whitespace and leading/trailing blank lines. +func cleanLines(raw string) []string { + raw = strings.ReplaceAll(raw, "\r\n", "\n") + var out []string + for _, line := range strings.Split(raw, "\n") { + trimmed := strings.TrimRight(line, " \t") + // git --verbose puts an uncommented diff below a scissors line. + if isScissors(trimmed) || strings.HasPrefix(trimmed, "diff --git ") { + break + } + if strings.HasPrefix(strings.TrimSpace(trimmed), "#") { + continue + } + out = append(out, trimmed) + } + // Trim leading and trailing blank lines. + for len(out) > 0 && strings.TrimSpace(out[0]) == "" { + out = out[1:] + } + for len(out) > 0 && strings.TrimSpace(out[len(out)-1]) == "" { + out = out[:len(out)-1] + } + return out +} + +func isScissors(line string) bool { + t := strings.TrimSpace(strings.TrimPrefix(strings.TrimSpace(line), "#")) + return strings.Contains(t, ">8") && strings.Contains(t, "--") +} + +// splitTrailers separates body lines from trailers. It takes the trailing +// block when it is made entirely of trailers, and additionally hoists any +// well-known trailer that got stranded in the middle of the body. +func splitTrailers(lines []string) ([]string, []Trailer) { + var trailers []Trailer + + // Trailing block: walk back over the last contiguous non-blank run. + end := len(lines) + start := end + for start > 0 && strings.TrimSpace(lines[start-1]) != "" { + start-- + } + if start < end { + allTrailers := true + for _, line := range lines[start:end] { + if trailerRe.FindStringSubmatch(line) == nil { + allTrailers = false + break + } + } + if allTrailers { + for _, line := range lines[start:end] { + match := trailerRe.FindStringSubmatch(line) + trailers = append(trailers, Trailer{Key: match[1], Value: match[2]}) + } + lines = lines[:start] + } + } + + // Hoist stranded well-known trailers, keeping document order ahead of the + // trailing block's. + var body []string + var hoisted []Trailer + for _, line := range lines { + if match := trailerRe.FindStringSubmatch(line); match != nil && knownTrailerKeys[strings.ToLower(match[1])] { + hoisted = append(hoisted, Trailer{Key: match[1], Value: match[2]}) + continue + } + body = append(body, line) + } + return body, append(hoisted, trailers...) +} + +// Header renders just the first line of the message. +func (m *Message) Header() string { + if m.Type == "" { + return m.Subject + } + head := m.Type + if m.Scope != "" { + head += "(" + m.Scope + ")" + } + if m.Breaking { + head += "!" + } + return head + ": " + m.Subject +} + +// String renders the message in canonical layout, without a trailing newline. +func (m *Message) String() string { + var b strings.Builder + b.WriteString(m.Header()) + if body := strings.Trim(m.Body, "\n"); body != "" { + b.WriteString("\n\n") + b.WriteString(body) + } + if len(m.Trailers) > 0 { + b.WriteString("\n\n") + for i, t := range m.Trailers { + if i > 0 { + b.WriteString("\n") + } + b.WriteString(t.String()) + } + } + return b.String() +} + +// Validate checks the message against DefaultOptions. +func Validate(m *Message) []Issue { return ValidateWithOptions(m, DefaultOptions()) } + +// ValidateWithOptions checks the message against opts and returns every +// issue found, errors and warnings alike. +func ValidateWithOptions(m *Message, opts Options) []Issue { + // Git writes merge, revert and fixup!/squash!/amend! headers itself; the + // user cannot choose their format, so there is nothing to enforce. + if m.GitGenerated { + return nil + } + + var issues []Issue + add := func(line int, sev Severity, rule, format string, args ...interface{}) { + issues = append(issues, Issue{Line: line, Severity: sev, Rule: rule, Message: fmt.Sprintf(format, args...)}) + } + + switch { + case m.Type == "": + add(1, SeverityError, "header-format", + "header must be %q, got %q", "type(scope): subject", m.rawHeader) + default: + if strings.ToLower(m.Type) != m.Type { + add(1, SeverityError, "type-lowercase", "type %q must be lowercase", m.Type) + } + if !allowed(strings.ToLower(m.Type), opts.AllowedTypes) { + add(1, SeverityError, "type-allowed", + "unknown type %q; allowed: %s", m.Type, strings.Join(opts.AllowedTypes, ", ")) + } + } + + if strings.TrimSpace(m.Subject) == "" { + add(1, SeverityError, "subject-empty", "subject must not be empty") + } else { + if n := len(m.Header()); n > opts.MaxSubjectLen { + add(1, SeverityError, "subject-length", + "header is %d characters, limit is %d", n, opts.MaxSubjectLen) + } + if strings.HasSuffix(m.Subject, ".") { + add(1, SeverityError, "subject-trailing-period", "subject must not end with a period") + } + if word := firstWord(m.Subject); !isImperative(word) { + add(1, SeverityWarning, "subject-mood", + "subject should use the imperative mood (%q reads as past tense or third person)", word) + } + } + + if m.noBlankAfterSubject { + add(2, SeverityError, "body-blank-line", "subject and body must be separated by a blank line") + } + + for i, line := range strings.Split(m.Body, "\n") { + if !protectedLine(line) && len(line) > opts.WrapBody { + add(bodyLineNumber(i), SeverityWarning, "body-wrap", + "body line is %d characters, wrap at %d", len(line), opts.WrapBody) + } + } + + for _, want := range opts.RequiredTrailers { + if !hasTrailerKey(m.Trailers, want.Key) { + add(0, SeverityError, "trailer-required", "missing required trailer %q", want.Key) + } + } + + return issues +} + +// bodyLineNumber maps a 0-based body line index onto the rendered message, +// where line 1 is the header and line 2 is the blank separator. +func bodyLineNumber(i int) int { return i + 3 } + +func allowed(t string, allowedTypes []string) bool { + for _, a := range allowedTypes { + if a == t { + return true + } + } + return false +} + +func hasTrailerKey(trailers []Trailer, key string) bool { + for _, t := range trailers { + if strings.EqualFold(t.Key, key) { + return true + } + } + return false +} + +// HasErrors reports whether any issue is error severity. +func HasErrors(issues []Issue) bool { + for _, i := range issues { + if i.Severity == SeverityError { + return true + } + } + return false +} + +// Normalize rewrites raw into the canonical format, returning the normalized +// message (with a trailing newline), the issues that remain after everything +// fixable has been fixed, and an error only when the message cannot be +// normalized at all — empty, or with a subject no type can be inferred from. +func Normalize(raw string, opts Options) (string, []Issue, error) { + m, err := Parse(raw) + if err != nil { + return "", nil, err + } + if m.GitGenerated { + // Nothing to rewrite: only git's comments and verbose diff are dropped. + return strings.Join(cleanLines(raw), "\n") + "\n", nil, nil + } + if strings.TrimSpace(m.Subject) == "" { + return "", nil, errors.New("commit message has no subject") + } + + if m.Type == "" { + inferred := inferType(m.Subject) + if inferred == "" { + return "", nil, fmt.Errorf("cannot infer a commit type from subject %q; "+ + "prefix it explicitly, e.g. \"fix: %s\"", m.Subject, m.Subject) + } + m.Type = inferred + } + m.Type = strings.ToLower(m.Type) + m.Subject = normalizeSubject(m.Subject) + m.noBlankAfterSubject = false + m.Body = wrapBody(m.Body, opts.WrapBody) + m.Trailers = mergeTrailers(m.Trailers, opts.RequiredTrailers) + + out := m.String() + "\n" + return out, ValidateWithOptions(m, opts), nil +} + +// normalizeSubject strips trailing periods and lowercases the leading word +// unless it looks like an acronym or identifier. +func normalizeSubject(s string) string { + s = strings.TrimSpace(s) + s = strings.TrimRight(s, ".") + s = strings.TrimSpace(s) + if s == "" { + return s + } + word := firstWord(s) + if isAcronymish(word) { + return s + } + runes := []rune(s) + runes[0] = unicode.ToLower(runes[0]) + return string(runes) +} + +// isAcronymish reports whether a word should keep its capitalization, e.g. +// "API", "HTTPClient", "GitHub". +func isAcronymish(word string) bool { + upper := 0 + for i, r := range word { + if unicode.IsUpper(r) && i > 0 { + return true + } + if unicode.IsUpper(r) { + upper++ + } + } + // A single-letter uppercase word, or a word already lowercase. + return upper > 0 && len([]rune(word)) == 1 +} + +func firstWord(s string) string { + s = strings.TrimSpace(s) + if i := strings.IndexAny(s, " \t"); i >= 0 { + return s[:i] + } + return s +} + +// nonImperative lists common past-tense and third-person subject openers. +var nonImperative = map[string]bool{ + "fixed": true, "fixes": true, "added": true, "adds": true, + "updated": true, "updates": true, "removed": true, "removes": true, + "changed": true, "changes": true, "refactored": true, "refactors": true, + "created": true, "creates": true, "implemented": true, "implements": true, + "bumped": true, "bumps": true, "moved": true, "moves": true, + "renamed": true, "renames": true, "documented": true, "documents": true, + "improved": true, "improves": true, "deleted": true, "deletes": true, + "wrote": true, "made": true, "makes": true, +} + +func isImperative(word string) bool { + w := strings.ToLower(strings.Trim(word, `"'`)) + if nonImperative[w] { + return false + } + // "…ing" openers ("adding a thing") are gerunds, not imperatives. + return !strings.HasSuffix(w, "ing") +} + +// typeByVerb maps a leading verb to a commit type. +var typeByVerb = map[string]string{ + "fix": "fix", "fixed": "fix", "fixes": "fix", "resolve": "fix", + "resolved": "fix", "correct": "fix", "corrected": "fix", "patch": "fix", + "repair": "fix", "handle": "fix", "prevent": "fix", "guard": "fix", + + "add": "feat", "added": "feat", "adds": "feat", "create": "feat", + "created": "feat", "implement": "feat", "implemented": "feat", + "introduce": "feat", "introduced": "feat", "support": "feat", + + "refactor": "refactor", "refactored": "refactor", "rename": "refactor", + "renamed": "refactor", "move": "refactor", "moved": "refactor", + "simplify": "refactor", "simplified": "refactor", "extract": "refactor", + "remove": "refactor", "removed": "refactor", "delete": "refactor", + "deleted": "refactor", "drop": "refactor", "update": "refactor", + "updated": "refactor", "change": "refactor", "changed": "refactor", + + "document": "docs", "documented": "docs", "clarify": "docs", + + "test": "test", "tested": "test", "cover": "test", + + "optimize": "perf", "optimized": "perf", "speed": "perf", + + "bump": "chore", "upgrade": "chore", "upgraded": "chore", "pin": "chore", + "clean": "chore", "cleanup": "chore", "chore": "chore", + + "revert": "revert", "reverted": "revert", +} + +// typeByKeyword maps a distinctive word anywhere in the subject to a type. +// These win over the leading verb: "Update the README" is docs, not refactor. +var typeByKeyword = []struct { + word string + typ string +}{ + {"readme", "docs"}, + {"changelog", "docs"}, + {"documentation", "docs"}, + {"docs", "docs"}, + {"godoc", "docs"}, + {"typo", "docs"}, +} + +// inferType guesses a commit type from a free-form subject, returning "" when +// it cannot make a confident guess. +func inferType(subject string) string { + lower := strings.ToLower(subject) + words := strings.FieldsFunc(lower, func(r rune) bool { return !unicode.IsLetter(r) && !unicode.IsNumber(r) }) + wordSet := make(map[string]bool, len(words)) + for _, w := range words { + wordSet[w] = true + } + + for _, kw := range typeByKeyword { + if wordSet[kw.word] { + return kw.typ + } + } + if len(words) > 0 { + if t, ok := typeByVerb[words[0]]; ok { + return t + } + } + if wordSet["bug"] || wordSet["crash"] || wordSet["panic"] || wordSet["regression"] { + return "fix" + } + if wordSet["test"] || wordSet["tests"] { + return "test" + } + return "" +} + +// mergeTrailers dedupes trailers (case-insensitively on the key, exactly on +// the value) and appends any required trailer that is missing. +func mergeTrailers(existing, required []Trailer) []Trailer { + var out []Trailer + seen := make(map[string]bool) + for _, t := range existing { + key := strings.ToLower(t.Key) + "\x00" + t.Value + if seen[key] { + continue + } + seen[key] = true + out = append(out, t) + } + for _, want := range required { + if !hasTrailerKey(out, want.Key) { + out = append(out, want) + } + } + return out +} + +// wrapBody reflows prose paragraphs at width columns, leaving code blocks, +// indented text, lists, quotes and unbreakable lines (long URLs) alone. +func wrapBody(body string, width int) string { + body = strings.Trim(body, "\n") + if body == "" { + return "" + } + var out []string + var para []string + inFence := false + + flush := func() { + if len(para) == 0 { + return + } + if paragraphProtected(para) { + out = append(out, para...) + } else { + out = append(out, wrapParagraph(strings.Join(para, " "), width)...) + } + para = nil + } + + for _, line := range strings.Split(body, "\n") { + if strings.HasPrefix(strings.TrimSpace(line), "```") { + flush() + inFence = !inFence + out = append(out, line) + continue + } + if inFence { + out = append(out, line) + continue + } + if strings.TrimSpace(line) == "" { + flush() + out = append(out, "") + continue + } + para = append(para, line) + } + flush() + + // Collapse runs of blank lines and trim the edges. + var collapsed []string + for i, line := range out { + if line == "" && i > 0 && out[i-1] == "" { + continue + } + collapsed = append(collapsed, line) + } + return strings.Trim(strings.Join(collapsed, "\n"), "\n") +} + +func paragraphProtected(para []string) bool { + for _, line := range para { + if protectedLine(line) { + return true + } + } + return false +} + +// protectedLine reports whether a line must be preserved verbatim. +func protectedLine(line string) bool { + if line == "" { + return true + } + if strings.HasPrefix(line, " ") || strings.HasPrefix(line, "\t") { + return true + } + switch line[0] { + case '-', '*', '+', '>', '|', '#': + return true + } + if strings.HasPrefix(line, "```") { + return true + } + // A single unbreakable token, e.g. a long URL. + if !strings.ContainsAny(line, " \t") { + return true + } + return false +} + +func wrapParagraph(text string, width int) []string { + words := strings.Fields(text) + if len(words) == 0 { + return nil + } + var lines []string + current := words[0] + for _, w := range words[1:] { + if len(current)+1+len(w) > width { + lines = append(lines, current) + current = w + continue + } + current += " " + w + } + return append(lines, current) +} diff --git a/internal/commitmsg/commitmsg_test.go b/internal/commitmsg/commitmsg_test.go new file mode 100644 index 0000000..5d8f562 --- /dev/null +++ b/internal/commitmsg/commitmsg_test.go @@ -0,0 +1,624 @@ +package commitmsg + +import ( + "strings" + "testing" +) + +func hasRule(issues []Issue, rule string) bool { + for _, i := range issues { + if i.Rule == rule { + return true + } + } + return false +} + +func errorCount(issues []Issue) int { + n := 0 + for _, i := range issues { + if i.Severity == SeverityError { + n++ + } + } + return n +} + +// --- Parsing --- + +func TestParse_FullMessage(t *testing.T) { + raw := `feat(cli): add commit message normalizer + +This adds a normalizer that enforces a single commit format +across the repository. + +Nightshift-Task: commit-normalize +Co-Authored-By: Someone +` + m, err := Parse(raw) + if err != nil { + t.Fatalf("Parse: %v", err) + } + if m.Type != "feat" { + t.Errorf("Type = %q, want feat", m.Type) + } + if m.Scope != "cli" { + t.Errorf("Scope = %q, want cli", m.Scope) + } + if m.Breaking { + t.Errorf("Breaking = true, want false") + } + if m.Subject != "add commit message normalizer" { + t.Errorf("Subject = %q", m.Subject) + } + if !strings.HasPrefix(m.Body, "This adds a normalizer") { + t.Errorf("Body = %q", m.Body) + } + if strings.Contains(m.Body, "Nightshift-Task") { + t.Errorf("body should not contain trailers: %q", m.Body) + } + if len(m.Trailers) != 2 { + t.Fatalf("Trailers = %v, want 2", m.Trailers) + } + if m.Trailers[0].Key != "Nightshift-Task" || m.Trailers[0].Value != "commit-normalize" { + t.Errorf("Trailers[0] = %+v", m.Trailers[0]) + } + if m.Trailers[1].Key != "Co-Authored-By" { + t.Errorf("Trailers[1] = %+v", m.Trailers[1]) + } +} + +func TestParse_BreakingMarker(t *testing.T) { + m, err := Parse("feat(api)!: drop v1 endpoints") + if err != nil { + t.Fatalf("Parse: %v", err) + } + if !m.Breaking { + t.Errorf("Breaking = false, want true") + } + if m.Type != "feat" || m.Scope != "api" { + t.Errorf("got type=%q scope=%q", m.Type, m.Scope) + } + if m.Subject != "drop v1 endpoints" { + t.Errorf("Subject = %q", m.Subject) + } +} + +func TestParse_NoConventionalHeader(t *testing.T) { + m, err := Parse("Fixed the flaky test") + if err != nil { + t.Fatalf("Parse should not fail on a non-conventional header: %v", err) + } + if m.Type != "" { + t.Errorf("Type = %q, want empty", m.Type) + } + if m.Subject != "Fixed the flaky test" { + t.Errorf("Subject = %q", m.Subject) + } +} + +func TestParse_EmptyMessage(t *testing.T) { + for _, raw := range []string{"", " \n\n ", "# just a comment\n# another\n"} { + if _, err := Parse(raw); err == nil { + t.Errorf("Parse(%q) = nil error, want error", raw) + } + } +} + +func TestParse_StripsCommentsAndVerboseDiff(t *testing.T) { + raw := `fix(db): close rows on scan error + +# Please enter the commit message for your changes. Lines starting +# with '#' will be ignored, and an empty message aborts the commit. +# +# ------------------------ >8 ------------------------ +# Do not modify or remove the line above. +diff --git a/internal/db/db.go b/internal/db/db.go +index 1234567..89abcde 100644 +--- a/internal/db/db.go ++++ b/internal/db/db.go +@@ -1,3 +1,4 @@ ++// a change that must never leak into the message +` + m, err := Parse(raw) + if err != nil { + t.Fatalf("Parse: %v", err) + } + if m.Body != "" { + t.Errorf("Body = %q, want empty (comments and diff stripped)", m.Body) + } + if strings.Contains(m.String(), "diff --git") || strings.Contains(m.String(), "#") { + t.Errorf("String() leaked comments/diff:\n%s", m.String()) + } +} + +func TestMessage_String_RoundTrip(t *testing.T) { + raw := "feat(cli): add thing\n\nSome body text.\n\nNightshift-Task: commit-normalize\n" + m, err := Parse(raw) + if err != nil { + t.Fatalf("Parse: %v", err) + } + want := "feat(cli): add thing\n\nSome body text.\n\nNightshift-Task: commit-normalize" + if got := m.String(); got != want { + t.Errorf("String() =\n%q\nwant\n%q", got, want) + } +} + +// --- Validation --- + +func TestValidate_Clean(t *testing.T) { + m, err := Parse("feat(cli): add commit message normalizer\n\nA short body.\n") + if err != nil { + t.Fatalf("Parse: %v", err) + } + if issues := Validate(m); len(issues) != 0 { + t.Errorf("Validate() = %v, want none", issues) + } +} + +func TestValidate_MissingType(t *testing.T) { + m, _ := Parse("Fixed the flaky test") + issues := Validate(m) + if !hasRule(issues, "header-format") { + t.Errorf("expected header-format issue, got %v", issues) + } + if errorCount(issues) == 0 { + t.Errorf("expected at least one error-severity issue, got %v", issues) + } +} + +func TestValidate_UnknownType(t *testing.T) { + m, _ := Parse("feet(cli): add a thing") + if !hasRule(Validate(m), "type-allowed") { + t.Errorf("expected type-allowed issue, got %v", Validate(m)) + } +} + +func TestValidate_UppercaseType(t *testing.T) { + m, _ := Parse("Fix(cli): add a thing") + if !hasRule(Validate(m), "type-lowercase") { + t.Errorf("expected type-lowercase issue, got %v", Validate(m)) + } +} + +func TestValidate_EmptySubject(t *testing.T) { + m, _ := Parse("fix(cli):") + if !hasRule(Validate(m), "subject-empty") && !hasRule(Validate(m), "header-format") { + t.Errorf("expected an issue for an empty subject, got %v", Validate(m)) + } +} + +func TestValidate_SubjectTooLong(t *testing.T) { + long := "feat(cli): " + strings.Repeat("a", 80) + m, _ := Parse(long) + issues := Validate(m) + if !hasRule(issues, "subject-length") { + t.Errorf("expected subject-length issue, got %v", issues) + } + for _, i := range issues { + if i.Rule == "subject-length" && i.Severity != SeverityError { + t.Errorf("subject-length should be an error, got %v", i.Severity) + } + } +} + +func TestValidate_TrailingPeriod(t *testing.T) { + m, _ := Parse("fix(cli): stop panicking on empty input.") + if !hasRule(Validate(m), "subject-trailing-period") { + t.Errorf("expected subject-trailing-period issue, got %v", Validate(m)) + } +} + +func TestValidate_ImperativeMoodWarning(t *testing.T) { + m, _ := Parse("fix(cli): fixed the parser") + issues := Validate(m) + if !hasRule(issues, "subject-mood") { + t.Fatalf("expected subject-mood issue, got %v", issues) + } + for _, i := range issues { + if i.Rule == "subject-mood" && i.Severity != SeverityWarning { + t.Errorf("subject-mood should be a warning, got %v", i.Severity) + } + } +} + +func TestValidate_MissingBlankLineAfterSubject(t *testing.T) { + m, _ := Parse("fix(cli): stop panicking\nthe body starts immediately\n") + if !hasRule(Validate(m), "body-blank-line") { + t.Errorf("expected body-blank-line issue, got %v", Validate(m)) + } +} + +func TestValidate_BodyWrapWarning(t *testing.T) { + m, _ := Parse("fix(cli): wrap it\n\n" + strings.Repeat("word ", 30) + "\n") + issues := Validate(m) + if !hasRule(issues, "body-wrap") { + t.Fatalf("expected body-wrap issue, got %v", issues) + } + for _, i := range issues { + if i.Rule == "body-wrap" && i.Severity != SeverityWarning { + t.Errorf("body-wrap should be a warning, got %v", i.Severity) + } + } +} + +func TestValidateWithOptions_RequiredTrailers(t *testing.T) { + opts := DefaultOptions() + opts.RequiredTrailers = []Trailer{{Key: "Nightshift-Task", Value: "commit-normalize"}} + + m, _ := Parse("fix(cli): stop panicking") + if !hasRule(ValidateWithOptions(m, opts), "trailer-required") { + t.Errorf("expected trailer-required issue, got %v", ValidateWithOptions(m, opts)) + } + + m2, _ := Parse("fix(cli): stop panicking\n\nNightshift-Task: commit-normalize\n") + if hasRule(ValidateWithOptions(m2, opts), "trailer-required") { + t.Errorf("unexpected trailer-required issue: %v", ValidateWithOptions(m2, opts)) + } +} + +func TestIssue_String(t *testing.T) { + i := Issue{Line: 3, Severity: SeverityError, Rule: "subject-length", Message: "too long"} + got := i.String() + if !strings.Contains(got, "3") || !strings.Contains(got, "error") || !strings.Contains(got, "too long") { + t.Errorf("Issue.String() = %q", got) + } +} + +// --- Normalization --- + +func TestNormalize_InfersTypeFromSubject(t *testing.T) { + cases := []struct { + raw string + want string + }{ + {"Fixed bug in X.", "fix: fixed bug in X"}, + {"Added a new report command", "feat: added a new report command"}, + {"Update the README", "docs: update the README"}, + {"Refactored the orchestrator", "refactor: refactored the orchestrator"}, + {"Bump go version", "chore: bump go version"}, + } + for _, tc := range cases { + got, _, err := Normalize(tc.raw, DefaultOptions()) + if err != nil { + t.Errorf("Normalize(%q): %v", tc.raw, err) + continue + } + if strings.TrimRight(got, "\n") != tc.want { + t.Errorf("Normalize(%q) = %q, want %q", tc.raw, strings.TrimRight(got, "\n"), tc.want) + } + } +} + +func TestNormalize_UninferrableTypeIsAnError(t *testing.T) { + if _, issues, err := Normalize("zzzqqq wibble frobnicate", DefaultOptions()); err == nil { + t.Errorf("expected error for uninferrable type, got issues %v", issues) + } +} + +func TestNormalize_LowercasesTypeAndStripsPeriod(t *testing.T) { + got, _, err := Normalize("Fix(CLI): stop panicking on empty input.", DefaultOptions()) + if err != nil { + t.Fatalf("Normalize: %v", err) + } + want := "fix(CLI): stop panicking on empty input\n" + if got != want { + t.Errorf("Normalize() = %q, want %q", got, want) + } +} + +func TestNormalize_PreservesAcronymInSubject(t *testing.T) { + got, _, err := Normalize("Fixed API timeouts", DefaultOptions()) + if err != nil { + t.Fatalf("Normalize: %v", err) + } + if !strings.Contains(got, "API timeouts") { + t.Errorf("Normalize() = %q, want the acronym preserved", got) + } +} + +func TestNormalize_InsertsBlankLineBeforeBody(t *testing.T) { + got, _, err := Normalize("fix(cli): stop panicking\nthe body starts immediately\n", DefaultOptions()) + if err != nil { + t.Fatalf("Normalize: %v", err) + } + want := "fix(cli): stop panicking\n\nthe body starts immediately\n" + if got != want { + t.Errorf("Normalize() = %q, want %q", got, want) + } +} + +func TestNormalize_WrapsBody(t *testing.T) { + body := strings.Repeat("word ", 40) + got, _, err := Normalize("fix(cli): wrap it\n\n"+body+"\n", DefaultOptions()) + if err != nil { + t.Fatalf("Normalize: %v", err) + } + for _, line := range strings.Split(got, "\n") { + if len(line) > DefaultOptions().WrapBody { + t.Errorf("line exceeds wrap width (%d): %q", len(line), line) + } + } +} + +func TestNormalize_DoesNotWrapCodeOrURLs(t *testing.T) { + url := "https://example.com/" + strings.Repeat("a", 90) + raw := "docs: add link\n\n indented code line that is quite long and should not be rewrapped at all\n\n" + url + "\n" + got, _, err := Normalize(raw, DefaultOptions()) + if err != nil { + t.Fatalf("Normalize: %v", err) + } + if !strings.Contains(got, url) { + t.Errorf("URL was rewrapped:\n%s", got) + } + if !strings.Contains(got, " indented code line that is quite long and should not be rewrapped at all") { + t.Errorf("indented line was rewrapped:\n%s", got) + } +} + +func TestNormalize_AppendsRequiredTrailers(t *testing.T) { + opts := DefaultOptions() + opts.RequiredTrailers = []Trailer{ + {Key: "Nightshift-Task", Value: "commit-normalize"}, + {Key: "Nightshift-Ref", Value: "https://github.com/marcus/nightshift"}, + } + got, _, err := Normalize("fix(cli): stop panicking\n\nA body.\n", opts) + if err != nil { + t.Fatalf("Normalize: %v", err) + } + want := "fix(cli): stop panicking\n\nA body.\n\nNightshift-Task: commit-normalize\nNightshift-Ref: https://github.com/marcus/nightshift\n" + if got != want { + t.Errorf("Normalize() =\n%q\nwant\n%q", got, want) + } +} + +func TestNormalize_DedupesTrailersAndPreservesCoAuthors(t *testing.T) { + opts := DefaultOptions() + opts.RequiredTrailers = []Trailer{{Key: "Nightshift-Task", Value: "commit-normalize"}} + raw := `fix(cli): stop panicking + +Nightshift-Task: commit-normalize +Nightshift-Task: commit-normalize +Co-Authored-By: A +Co-Authored-By: B +Co-Authored-By: A +` + got, _, err := Normalize(raw, opts) + if err != nil { + t.Fatalf("Normalize: %v", err) + } + if n := strings.Count(got, "Nightshift-Task:"); n != 1 { + t.Errorf("Nightshift-Task appears %d times:\n%s", n, got) + } + if n := strings.Count(got, "Co-Authored-By: A "); n != 1 { + t.Errorf("duplicate co-author not deduped:\n%s", got) + } + if !strings.Contains(got, "Co-Authored-By: B ") { + t.Errorf("distinct co-author dropped:\n%s", got) + } +} + +func TestNormalize_TrailersEndUpInASingleTrailingBlock(t *testing.T) { + raw := `fix(cli): stop panicking + +Nightshift-Task: commit-normalize + +Some body text that came after a trailer. + +Co-Authored-By: A +` + got, _, err := Normalize(raw, DefaultOptions()) + if err != nil { + t.Fatalf("Normalize: %v", err) + } + lines := strings.Split(strings.TrimRight(got, "\n"), "\n") + // The last block must be contiguous trailers. + if lines[len(lines)-1] != "Co-Authored-By: A " || + lines[len(lines)-2] != "Nightshift-Task: commit-normalize" { + t.Errorf("trailers not collected into one trailing block:\n%s", got) + } + if lines[len(lines)-3] != "" { + t.Errorf("missing blank line before trailer block:\n%s", got) + } +} + +func TestNormalize_Idempotent(t *testing.T) { + opts := DefaultOptions() + opts.RequiredTrailers = []Trailer{{Key: "Nightshift-Task", Value: "commit-normalize"}} + inputs := []string{ + "Fixed bug in X.", + "fix(cli): stop panicking\nbody right here\n", + "feat(api)!: drop v1\n\n" + strings.Repeat("word ", 40) + "\n\nCo-Authored-By: A \n", + } + for _, raw := range inputs { + once, _, err := Normalize(raw, opts) + if err != nil { + t.Fatalf("Normalize(%q): %v", raw, err) + } + twice, _, err := Normalize(once, opts) + if err != nil { + t.Fatalf("Normalize(normalized): %v", err) + } + if once != twice { + t.Errorf("not idempotent for %q:\nfirst:\n%q\nsecond:\n%q", raw, once, twice) + } + } +} + +func TestNormalize_AlreadyNormalIsNoOp(t *testing.T) { + raw := "feat(cli): add commit message normalizer\n\nA tidy body that is already short enough.\n\nNightshift-Task: commit-normalize\n" + got, issues, err := Normalize(raw, DefaultOptions()) + if err != nil { + t.Fatalf("Normalize: %v", err) + } + if got != raw { + t.Errorf("Normalize() changed an already-normal message:\ngot %q\nwant %q", got, raw) + } + if errorCount(issues) != 0 { + t.Errorf("unexpected residual errors: %v", issues) + } +} + +func TestNormalize_ReportsResidualErrorsItCannotFix(t *testing.T) { + long := "feat(cli): " + strings.Repeat("a", 90) + _, issues, err := Normalize(long, DefaultOptions()) + if err != nil { + t.Fatalf("Normalize: %v", err) + } + if !hasRule(issues, "subject-length") { + t.Errorf("expected residual subject-length issue, got %v", issues) + } +} + +func TestNormalize_StripsCommentsAndDiff(t *testing.T) { + raw := "fix(db): close rows\n# a comment\n# ------------------------ >8 ------------------------\ndiff --git a/x b/x\n+leak\n" + got, _, err := Normalize(raw, DefaultOptions()) + if err != nil { + t.Fatalf("Normalize: %v", err) + } + if got != "fix(db): close rows\n" { + t.Errorf("Normalize() = %q", got) + } +} + +// --- Spec --- + +func TestSpec_MentionsCoreRules(t *testing.T) { + s := Spec() + for _, want := range []string{"type(scope)", "72", "feat", "fix"} { + if !strings.Contains(s, want) { + t.Errorf("Spec() missing %q:\n%s", want, s) + } + } +} + +func TestPromptSpec_IncludesTrailers(t *testing.T) { + s := PromptSpec("commit-normalize") + if !strings.Contains(s, "Nightshift-Task: commit-normalize") { + t.Errorf("PromptSpec missing task trailer:\n%s", s) + } + if !strings.Contains(s, "Nightshift-Ref: https://github.com/marcus/nightshift") { + t.Errorf("PromptSpec missing ref trailer:\n%s", s) + } +} + +// --- Git-generated messages --- + +// gitGenerated are headers git itself writes, which the hook must let through +// untouched: rejecting them breaks `git merge --no-ff` and `--autosquash`. +var gitGenerated = []string{ + "Merge branch 'side'", + "Merge branch 'side' into main", + "Merge pull request #186 from marcus/chore/commit-message-normalizer", + "Merge remote-tracking branch 'origin/main'", + `Revert "feat(cli): add commit message normalizer"`, + "fixup! feat(cli): add commit message normalizer", + "squash! feat(cli): add commit message normalizer", + "amend! feat(cli): add commit message normalizer", +} + +func TestIsGitGenerated(t *testing.T) { + for _, header := range gitGenerated { + if !IsGitGenerated(header) { + t.Errorf("IsGitGenerated(%q) = false, want true", header) + } + } + notGenerated := []string{ + "feat: add merge support", + "Merged the branches", + "merge branch 'side'", + "Revert the thing", + "fixup the thing", + "fix: revert \"a change\"", + "Mergebranch 'side'", + } + for _, header := range notGenerated { + if IsGitGenerated(header) { + t.Errorf("IsGitGenerated(%q) = true, want false", header) + } + } +} + +func TestParse_MarksGitGenerated(t *testing.T) { + for _, header := range gitGenerated { + m, err := Parse(header + "\n") + if err != nil { + t.Fatalf("Parse(%q): %v", header, err) + } + if !m.GitGenerated { + t.Errorf("Parse(%q).GitGenerated = false, want true", header) + } + } + m, err := Parse("feat: add a thing\n") + if err != nil { + t.Fatalf("Parse: %v", err) + } + if m.GitGenerated { + t.Error("GitGenerated = true for a conventional header, want false") + } +} + +func TestValidate_SkipsGitGenerated(t *testing.T) { + for _, header := range gitGenerated { + m, err := Parse(header + "\n") + if err != nil { + t.Fatalf("Parse(%q): %v", header, err) + } + if issues := Validate(m); len(issues) != 0 { + t.Errorf("Validate(%q) = %v, want no issues", header, issues) + } + } +} + +func TestValidate_GitGeneratedIgnoresRequiredTrailers(t *testing.T) { + opts := DefaultOptions() + opts.RequiredTrailers = []Trailer{{Key: "Nightshift-Task", Value: "commit-normalize"}} + m, err := Parse("Merge branch 'side'\n") + if err != nil { + t.Fatalf("Parse: %v", err) + } + if issues := ValidateWithOptions(m, opts); len(issues) != 0 { + t.Errorf("ValidateWithOptions = %v, want no issues", issues) + } +} + +func TestValidate_MergeWithConflictBodyIsClean(t *testing.T) { + // What git actually hands the hook for a conflicted merge resolution. + raw := "Merge branch 'side'\n\n# Conflicts:\n#\tfile.go\n" + m, err := Parse(raw) + if err != nil { + t.Fatalf("Parse: %v", err) + } + if issues := Validate(m); len(issues) != 0 { + t.Errorf("Validate = %v, want no issues", issues) + } +} + +func TestNormalize_LeavesGitGeneratedUntouched(t *testing.T) { + opts := DefaultOptions() + opts.RequiredTrailers = []Trailer{{Key: "Nightshift-Task", Value: "commit-normalize"}} + for _, header := range gitGenerated { + raw := header + "\n" + got, issues, err := Normalize(raw, opts) + if err != nil { + t.Fatalf("Normalize(%q): %v", header, err) + } + if got != raw { + t.Errorf("Normalize(%q) = %q, want it unchanged", header, got) + } + if len(issues) != 0 { + t.Errorf("Normalize(%q) issues = %v, want none", header, issues) + } + } +} + +func TestNormalize_PreservesMergeBody(t *testing.T) { + raw := "Merge branch 'side' into main\n\nA long line in a merge body that nobody should be rewrapping at all.\n" + got, _, err := Normalize(raw, DefaultOptions()) + if err != nil { + t.Fatalf("Normalize: %v", err) + } + if got != raw { + t.Errorf("Normalize = %q, want %q", got, raw) + } +} diff --git a/internal/commitmsg/spec.go b/internal/commitmsg/spec.go new file mode 100644 index 0000000..b892212 --- /dev/null +++ b/internal/commitmsg/spec.go @@ -0,0 +1,106 @@ +package commitmsg + +import ( + "fmt" + "strings" +) + +// The tunable limits of the format. These are the single source of truth for +// the validator, the CLI, the git hook and the agent prompts. +const ( + // MaxSubjectLen caps the whole header line, "type(scope): subject". + MaxSubjectLen = 72 + // WrapBody is the column body paragraphs are wrapped at. + WrapBody = 72 + + // TaskTrailerKey identifies the Nightshift task a commit came from. + TaskTrailerKey = "Nightshift-Task" + // RefTrailerKey points back at the Nightshift project. + RefTrailerKey = "Nightshift-Ref" + // RefTrailerValue is the canonical value for RefTrailerKey. + RefTrailerValue = "https://github.com/marcus/nightshift" +) + +// AllowedTypes are the commit types this repository accepts. +var AllowedTypes = []string{ + "feat", + "fix", + "docs", + "style", + "refactor", + "perf", + "test", + "build", + "ci", + "chore", + "revert", +} + +// NightshiftTrailers returns the trailers an autonomous run must attach to +// every commit it makes for the given task type. +func NightshiftTrailers(taskType string) []Trailer { + return []Trailer{ + {Key: TaskTrailerKey, Value: taskType}, + {Key: RefTrailerKey, Value: RefTrailerValue}, + } +} + +// Spec returns the canonical, human-readable description of the commit +// message format. Everything that explains the format — `nightshift +// commit-msg --print-spec`, the commit-msg hook's failure output, the docs +// and the agent prompts — quotes this text. +func Spec() string { + return fmt.Sprintf(`Commit message format + + type(scope)!: subject + + body, wrapped at %d columns + + Trailer-Key: value + +Rules + - type is required and lowercase, one of: %s + - scope is optional and describes the area touched, e.g. (cli), (db) + - "!" after the type/scope marks a breaking change + - the whole header line "type(scope): subject" is at most %d characters + - the subject is imperative mood ("add x", not "added x") and has no + trailing period + - a blank line separates the subject from the body and the body from the + trailers + - body paragraphs wrap at %d columns; code blocks, indented text, lists + and URLs are left alone + - trailers form one block at the very end, one "Key: value" per line + - messages git writes itself are exempt: headers starting with 'Merge ', + 'Revert "', 'fixup! ', 'squash! ' or 'amend! ' are always accepted, so + merges and rebase --autosquash are never blocked + +Example + feat(cli): add commit message normalizer + + Adds a nightshift commit-msg subcommand that checks and rewrites commit + messages so every commit in the repository shares one format. + + Nightshift-Task: commit-normalize + Nightshift-Ref: %s +`, WrapBody, strings.Join(AllowedTypes, ", "), MaxSubjectLen, WrapBody, RefTrailerValue) +} + +// PromptSpec returns the commit instructions handed to autonomous agents for +// a task of the given type: the shared Spec plus the exact trailers the run +// must attach. It is the same format `nightshift commit-msg --check` +// enforces, so agents are told precisely what the hook will accept. +func PromptSpec(taskType string) string { + var b strings.Builder + for _, line := range strings.Split(strings.TrimRight(Spec(), "\n"), "\n") { + if line == "" { + b.WriteString("\n") + continue + } + b.WriteString(" " + line + "\n") + } + b.WriteString("\n Every commit you make must also carry these trailers:\n") + for _, t := range NightshiftTrailers(taskType) { + b.WriteString(" " + t.String() + "\n") + } + return strings.TrimRight(b.String(), "\n") +} diff --git a/internal/orchestrator/orchestrator.go b/internal/orchestrator/orchestrator.go index 6141c95..2eb1511 100644 --- a/internal/orchestrator/orchestrator.go +++ b/internal/orchestrator/orchestrator.go @@ -16,6 +16,7 @@ import ( "github.com/marcus/nightshift/internal/agents" "github.com/marcus/nightshift/internal/budget" + "github.com/marcus/nightshift/internal/commitmsg" "github.com/marcus/nightshift/internal/logging" "github.com/marcus/nightshift/internal/tasks" ) @@ -728,9 +729,8 @@ Description: %s 0. You are running autonomously. If the task is broad or ambiguous, choose a concrete, minimal scope that delivers value and state any assumptions in the description. 1. Work on a new branch and plan to submit a PR. Never work directly on the primary branch.%s 2. Before creating your branch, record the current branch name and plan to switch back after the PR is opened. -3. If you create commits, include a concise message with these git trailers: - Nightshift-Task: %s - Nightshift-Ref: https://github.com/marcus/nightshift +3. If you create commits, use this commit message format: +%s 4. Analyze the task requirements 5. Identify files that need to be modified 6. Create step-by-step implementation plan @@ -741,7 +741,7 @@ Description: %s "files": ["file1.go", "file2.go", ...], "description": "overall approach" } -`, task.ID, task.Title, task.Description, branchInstruction, task.Type) +`, task.ID, task.Title, task.Description, branchInstruction, commitmsg.PromptSpec(string(task.Type))) } func (o *Orchestrator) buildImplementPrompt(task *tasks.Task, plan *PlanOutput, iteration int) string { @@ -771,9 +771,8 @@ Description: %s ## Instructions 0. Before creating your branch, record the current branch name. Create and work on a new branch. Never modify or commit directly to the primary branch.%s When finished, open a PR. After the PR is submitted, switch back to the original branch. If you cannot open a PR, leave the branch and explain next steps. -1. If you create commits, include a concise message with these git trailers: - Nightshift-Task: %s - Nightshift-Ref: https://github.com/marcus/nightshift +1. If you create commits, use this commit message format: +%s 2. Implement the plan step by step 3. Make all necessary code changes 4. Ensure tests pass @@ -783,7 +782,7 @@ Description: %s "files_modified": ["file1.go", ...], "summary": "what was done" } -`, task.ID, task.Title, task.Description, plan.Description, plan.Steps, iterationNote, branchInstruction, task.Type) +`, task.ID, task.Title, task.Description, plan.Description, plan.Steps, iterationNote, branchInstruction, commitmsg.PromptSpec(string(task.Type))) } func (o *Orchestrator) buildReviewPrompt(task *tasks.Task, impl *ImplementOutput) string { diff --git a/internal/orchestrator/orchestrator_commitmsg_test.go b/internal/orchestrator/orchestrator_commitmsg_test.go new file mode 100644 index 0000000..7770ea9 --- /dev/null +++ b/internal/orchestrator/orchestrator_commitmsg_test.go @@ -0,0 +1,81 @@ +package orchestrator + +import ( + "strings" + "testing" + + "github.com/marcus/nightshift/internal/commitmsg" + "github.com/marcus/nightshift/internal/tasks" +) + +func commitMsgTestTask() *tasks.Task { + return &tasks.Task{ + ID: "commit-normalize", + Title: "Commit Message Normalizer", + Description: "Standardize commit message format", + Type: tasks.TaskType("commit-normalize"), + } +} + +// The prompts must keep instructing agents with the exact trailers the +// orchestrator has always required, now sourced from internal/commitmsg. +func TestPrompts_ContainNightshiftTrailers(t *testing.T) { + o := New() + task := commitMsgTestTask() + plan := &PlanOutput{Steps: []string{"step1"}, Description: "test plan"} + + prompts := map[string]string{ + "plan": o.buildPlanPrompt(task), + "implement": o.buildImplementPrompt(task, plan, 1), + } + for name, prompt := range prompts { + if !strings.Contains(prompt, "Nightshift-Task: commit-normalize") { + t.Errorf("%s prompt missing task trailer\nGot:\n%s", name, prompt) + } + if !strings.Contains(prompt, "Nightshift-Ref: https://github.com/marcus/nightshift") { + t.Errorf("%s prompt missing ref trailer\nGot:\n%s", name, prompt) + } + } +} + +// Both prompts must embed the same canonical spec the validator enforces, so +// agents are never told a format the commit-msg hook would reject. +func TestPrompts_EmbedCanonicalSpec(t *testing.T) { + o := New() + task := commitMsgTestTask() + plan := &PlanOutput{Steps: []string{"step1"}, Description: "test plan"} + + spec := commitmsg.PromptSpec("commit-normalize") + for name, prompt := range map[string]string{ + "plan": o.buildPlanPrompt(task), + "implement": o.buildImplementPrompt(task, plan, 1), + } { + if !strings.Contains(prompt, spec) { + t.Errorf("%s prompt does not embed commitmsg.PromptSpec\nGot:\n%s", name, prompt) + } + if !strings.Contains(prompt, "type(scope)") { + t.Errorf("%s prompt missing the header format\nGot:\n%s", name, prompt) + } + } +} + +// The spec handed to agents must itself describe a message the validator +// accepts: the example in the spec is checked against the real rules. +func TestPromptSpecExampleValidates(t *testing.T) { + example := "feat(cli): add commit message normalizer\n\n" + + "Adds a nightshift commit-msg subcommand that checks and rewrites commit\n" + + "messages so every commit in the repository shares one format.\n\n" + + "Nightshift-Task: commit-normalize\n" + + "Nightshift-Ref: https://github.com/marcus/nightshift\n" + + if !strings.Contains(commitmsg.Spec(), strings.TrimRight(example, "\n")[:40]) { + t.Fatalf("spec example drifted; update this test alongside commitmsg.Spec()") + } + m, err := commitmsg.Parse(example) + if err != nil { + t.Fatalf("parsing the spec example: %v", err) + } + if issues := commitmsg.Validate(m); commitmsg.HasErrors(issues) { + t.Errorf("the spec's own example fails validation: %v", issues) + } +} diff --git a/scripts/commit-msg.sh b/scripts/commit-msg.sh new file mode 100755 index 0000000..dffe2aa --- /dev/null +++ b/scripts/commit-msg.sh @@ -0,0 +1,38 @@ +#!/usr/bin/env bash +# commit-msg hook for nightshift +# Install: make install-hooks (or: ln -sf ../../scripts/commit-msg.sh .git/hooks/commit-msg) +# +# Validates the commit message against the format defined in +# internal/commitmsg. Skip with: git commit --no-verify +set -euo pipefail + +MSG_FILE="${1:?usage: commit-msg.sh }" + +REPO_ROOT="$(git rev-parse --show-toplevel)" + +echo "🪡 commit-msg check" + +# Prefer the built binary; fall back to `go run` so the hook works in a fresh +# clone before `make build`. +if [[ -x "$REPO_ROOT/nightshift" ]]; then + RUNNER=("$REPO_ROOT/nightshift") +elif command -v nightshift >/dev/null 2>&1; then + RUNNER=(nightshift) +elif command -v go >/dev/null 2>&1; then + RUNNER=(go run "$REPO_ROOT/cmd/nightshift") +else + echo " – skipped (no nightshift binary and no go toolchain)" + exit 0 +fi + +if "${RUNNER[@]}" commit-msg --check "$MSG_FILE"; then + echo "✅ commit message OK" + exit 0 +fi + +echo "" +echo "❌ Commit message rejected." +echo " Rewrite it automatically with:" +echo " ${RUNNER[*]} commit-msg --fix \"$MSG_FILE\"" +echo " Or skip this check with: git commit --no-verify" +exit 1 diff --git a/website/docs/cli-reference.md b/website/docs/cli-reference.md index d5a2cd4..e1ad936 100644 --- a/website/docs/cli-reference.md +++ b/website/docs/cli-reference.md @@ -73,6 +73,36 @@ nightshift task run lint-fix --provider claude nightshift task run lint-fix --provider codex --dry-run ``` +## Commit Message Commands + +```bash +nightshift commit-msg --print-spec # Print the required format +nightshift commit-msg --check .git/COMMIT_EDITMSG +nightshift commit-msg --fix .git/COMMIT_EDITMSG # Rewrite the file in place +printf 'Fixed the thing.' | nightshift commit-msg --fix - +``` + +`commit-msg` reads the message from the given file, or from stdin when the +argument is `-` or omitted. Without `--fix` it validates, printing issues as +`file:line: severity: message (rule)` and exiting non-zero if any are errors. + +| Flag | Description | +|------|-------------| +| `--check` | Validate only; exit non-zero when the message has errors (the default) | +| `--fix` | Rewrite the message into the canonical format | +| `--print-spec` | Print the commit message format and exit | +| `--quiet` | Suppress issue output; rely on the exit code | + +The expected format is Conventional Commits: `type(scope)!: subject`, a blank +line, a body wrapped at 72 columns, and a single trailing block of git +trailers. Messages git generates itself — headers starting with `Merge `, +`Revert "`, `fixup! `, `squash! ` or `amend! ` — are exempt, so merges and +`git rebase --autosquash` are never blocked. + +`make install-hooks` installs a `commit-msg` hook that runs +`nightshift commit-msg --check` on every commit; bypass it with +`git commit --no-verify`. + ## Budget Commands ```bash