diff --git a/.agents/skills/create-commit/SKILL.md b/.agents/skills/create-commit/SKILL.md index 790ba1b585..26b23b6a7e 100644 --- a/.agents/skills/create-commit/SKILL.md +++ b/.agents/skills/create-commit/SKILL.md @@ -20,6 +20,17 @@ Before drafting any commit, read `mem:workflow/creating-commits` end-to-end. It is the authoritative source for the commit message format, the emoji menu, subject/body limits, and the `AI-assisted-by` trailer. Follow it exactly. +## Iron Rules (non-negotiable) + +1. **Wrap every body line at 76 characters or fewer.** Count characters, do + not eyeball. Exceptions: `Signed-off-by:` / `AI-assisted-by:` trailers and + lines carrying a URL. This is the rule agents skip most often. +2. **Subject ≤70 chars**, imperative, capitalized, no trailing period. +3. **Blank line between subject and body.** +4. **Run `./scripts/check-commit` and require exit code 0.** It mechanically + checks rules 1–3. A non-zero exit is a hard blocker: fix the message and + re-commit. Never report the commit as done with a failing checker. + ## Workflow 1. **Stage the files** specified by the calling context. Do not ask for @@ -29,12 +40,18 @@ subject/body limits, and the `AI-assisted-by` trailer. Follow it exactly. that does not match the stated intent, **STOP** and tell the user before committing. 3. Draft the message following the format in the memory doc, wrapping the body - at 72 characters per line, and run: + at 76 characters per line, and run: ```bash git commit -m "" -m "" ``` (or `git commit -F -` if the body has unusual characters). -4. The `AI-assisted-by` trailer value is provided by the calling context — use +4. **Verify the message with the checker**: + ```bash + ./scripts/check-commit + ``` + If it fails, amend the message (`git commit --amend`) until it passes. Do + not finish with a failing checker. +5. The `AI-assisted-by` trailer value is provided by the calling context — use it verbatim. ## Constraints @@ -45,3 +62,4 @@ subject/body limits, and the `AI-assisted-by` trailer. Follow it exactly. - Do not amend a commit you did not create in this session, unless explicitly asked. - Do not bypass pre-commit hooks (`--no-verify`) unless explicitly asked. - Do not add untracked files that were not created in this session. +- Do not skip the `scripts/check-commit` verification step (Iron Rule 4). diff --git a/.serena/memories/critical-info.md b/.serena/memories/critical-info.md index 26fcfe2971..d36edd4cfa 100644 --- a/.serena/memories/critical-info.md +++ b/.serena/memories/critical-info.md @@ -11,7 +11,7 @@ You are working on the GitHub project `penpot/penpot`, a monorepo. # Development workflow - Commit/PR/issue creation is **on explicit request only**. Before any of these actions, read the relevant memory — don't infer format from prior examples: - - Before `git commit` → `mem:workflow/creating-commits` (subject format, body, `AI-assisted-by: model-name` trailer) + - Before `git commit` → `mem:workflow/creating-commits` (subject/body format, 76-char body wrapping enforced by `scripts/check-commit`, `AI-assisted-by: model-name` trailer) - Before `gh issue create` → `mem:workflow/creating-issues` (title derivation, body template, labels, Issue Type) - Before `gh pr create` / `gh pr edit` → `mem:workflow/creating-prs` (title format, body structure, "Note:" line) - Before a repo-wide pnpm version update → `mem:workflow/updating-pnpm` (workspace diff --git a/.serena/memories/workflow/creating-commits.md b/.serena/memories/workflow/creating-commits.md index 2fc766d4ad..c3373ba29e 100644 --- a/.serena/memories/workflow/creating-commits.md +++ b/.serena/memories/workflow/creating-commits.md @@ -14,12 +14,32 @@ automatically pull the identity from the local git config `user.name` and `user. :emoji: Subject line (imperative, capitalized, no period, <=70 chars) Body explaining what changed and why. -Wrap lines at 72 characters — git log and tooling -render long lines poorly. Keep each line concise. +Wrap lines at 76 characters — git log adds a +four-space indent, so 76 + 4 fits an 80-column +terminal. Keep each line concise. AI-assisted-by: model-name ``` +## HARD RULES (inexcusable) + +These rules are not advisory. Do not commit until every one holds. A commit +that breaks them is wrong, even if the code is right. + +- **Body lines MUST wrap at 76 characters or fewer.** Measure every line; do + not eyeball it. This is the rule most often skipped. Rationale: `git log` + indents the body four spaces, so 76 + 4 fits an 80-column terminal. +- **Subject MUST be ≤70 chars**, imperative, capitalized, no trailing period. +- **MUST be a blank line** between subject and body. +- **MUST run `scripts/check-commit` and get exit code 0 before finishing.** + It mechanically validates the rules above; a failing run is a blocker. + - It checks `HEAD` by default: `./scripts/check-commit` + - For another commit: `./scripts/check-commit -c ` +- **NEVER** hand-wave the body as "one long line". If a line exceeds 76, + break it at a space. +- Exceptions inside the body (do not wrap these): `Signed-off-by:`, + `Co-authored-by:`, `AI-assisted-by:` trailers, and lines carrying a URL. + **AI-assisted-by trailer rules:** - Use only the model name, e.g. `mimo-v2.5`, `deepseek-v4-flash` - Do NOT add prefixes like `opencode-go/` — use the bare model name diff --git a/AGENTS.md b/AGENTS.md index 148ab20cba..1a9db621d8 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -17,6 +17,9 @@ - **`.claude/skills` is a symlink to `.agents/skills`.** Edit skills only in their canonical location (`.agents/skills`); never edit through `.claude/skills`. +- **Commit message body lines MUST wrap at ≤76 chars** (subject ≤70 chars) and + the commit MUST pass `./scripts/check-commit` with exit code 0 before you + consider it done. This is mechanically checked — do not eyeball it. - **Read the workflow memory BEFORE the corresponding action**: - Before `git commit` → `mem:workflow/creating-commits` (commit format, AI-assisted-by trailer) - Before `gh issue create` → `mem:workflow/creating-issues` (title derivation, body template, Issue Type) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index d905aae9e0..a7aeac3f3f 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -188,8 +188,11 @@ Commit messages must follow this format: - Add clear and concise description on the body - Do not end the subject with a period - Keep the subject to **70 characters** or fewer +- **Wrap body lines at 76 characters or fewer** (trailers and URLs excepted) - Separate the subject from the body with a **blank line** +You can check a commit against these rules with `./scripts/check-commit`. + ### Examples ``` diff --git a/scripts/check-commit b/scripts/check-commit index 478aee5156..3f2792552e 100755 --- a/scripts/check-commit +++ b/scripts/check-commit @@ -5,6 +5,7 @@ Check commit messages against Penpot's commit guidelines. Validates commit messages using the rules defined in: - .github/workflows/commit-checker.yml (regex pattern) - CONTRIBUTING.md (formatting rules, subject length, DCO) + - .serena/memories/workflow/creating-commits.md (body wrapped at 76 chars) By default, checks HEAD. Use --commit to specify a different commit. @@ -38,6 +39,20 @@ COMMIT_PATTERN = re.compile( MERGE_PATTERN = re.compile(r"^(Merge|Revert|Reapply).+[^.]$") +# ── Body line wrapping ─────────────────────────────────────────────────────── +# Commit bodies must wrap at 76 characters (see +# .serena/memories/workflow/creating-commits.md). That leaves room for the +# four-space indent git log adds, fitting an 80-column terminal. Trailers and +# URLs are exempt: they cannot be wrapped without losing meaning. +MAX_BODY_LINE = 76 + +TRAILER_PATTERN = re.compile( + r"^(Signed-off-by|Co-authored-by|Co-developed-by|Reviewed-by|" + r"Acked-by|Tested-by|Reported-by|Suggested-by|AI-assisted-by):" +) + +URL_PATTERN = re.compile(r"https?://\S+") + # ═══════════════════════════════════════════════════════════════════════════════ # Helpers # ═══════════════════════════════════════════════════════════════════════════════ @@ -93,11 +108,11 @@ def check_regex(message): def check_subject_length(message): - """Subject line must be ≤ 90 characters.""" + """Subject line must be ≤ 70 characters.""" first_line = message.split("\n")[0] - if len(first_line) > 90: + if len(first_line) > 70: return False, ( - f"Subject line exceeds 90 characters ({len(first_line)} chars):\n" + f"Subject line exceeds 70 characters ({len(first_line)} chars):\n" f" {first_line}" ) return True, None @@ -148,6 +163,41 @@ def check_body_blank_line(message): return True, None +def check_body_line_length(message): + """Body lines must wrap at 76 characters or fewer. + + The subject (first line) has its own length rule. Trailers (e.g. + Signed-off-by) and lines carrying a URL are exempt, since wrapping them + would break tooling or lose information. + """ + lines = message.split("\n") + offenders = [] + + for line_number, line in enumerate(lines[1:], start=2): + if len(line) <= MAX_BODY_LINE: + continue + if TRAILER_PATTERN.match(line): + continue + if URL_PATTERN.search(line): + continue + # A long token with no whitespace before the limit cannot be wrapped. + if " " not in line[:MAX_BODY_LINE]: + continue + offenders.append((line_number, line)) + + if not offenders: + return True, None + + details = "\n".join( + f" line {line_number} ({len(line)} chars): {line!r}" + for line_number, line in offenders + ) + return False, ( + f"Body lines must wrap at {MAX_BODY_LINE} characters or fewer. " + "Unwrapped line(s):\n" + details + ) + + def check_signed_off_by(message): """Check for the DCO Signed-off-by line (required for code changes).""" if "Signed-off-by:" not in message: @@ -179,10 +229,11 @@ def main(): validators = [ ("Regex pattern", check_regex), - ("Subject ≤ 90 chars", check_subject_length), + ("Subject ≤ 70 chars", check_subject_length), ("No trailing period in subject", check_subject_no_trailing_dot), ("Subject capitalized", check_subject_capitalized), ("Blank line after subject", check_body_blank_line), + ("Body wrapped at 76 chars", check_body_line_length), ] all_ok = True diff --git a/scripts/test_check_commit.py b/scripts/test_check_commit.py new file mode 100644 index 0000000000..4e4940429e --- /dev/null +++ b/scripts/test_check_commit.py @@ -0,0 +1,136 @@ +#!/usr/bin/env python3 +"""Tests for scripts/check-commit. + +Run with: + + python3 scripts/test_check_commit.py + +Covers the body line-wrapping validator added to enforce the commit body +wrap rule documented in .serena/memories/workflow/creating-commits.md. +""" + +import importlib.machinery +import importlib.util +import pathlib +import sys +import unittest + +# Loading scripts/check-commit would otherwise emit scripts/__pycache__/. +sys.dont_write_bytecode = True + +SCRIPT_PATH = pathlib.Path(__file__).resolve().parent / "check-commit" + + +def load_check_commit(): + """Load the extensionless scripts/check-commit as a module.""" + loader = importlib.machinery.SourceFileLoader("check_commit", str(SCRIPT_PATH)) + spec = importlib.util.spec_from_loader("check_commit", loader) + module = importlib.util.module_from_spec(spec) + loader.exec_module(module) + return module + + +check_commit = load_check_commit() + + +class BodyLineLengthTests(unittest.TestCase): + def assert_ok(self, message): + ok, error = check_commit.check_body_line_length(message) + self.assertTrue(ok, error) + self.assertIsNone(error) + + def assert_fail(self, message): + ok, error = check_commit.check_body_line_length(message) + self.assertFalse(ok) + self.assertIsNotNone(error) + return error + + def test_wrapped_body_passes(self): + message = ( + ":bug: Fix crash when opening the file menu\n" + "\n" + "The menu reused a stale reference after the file was\n" + "closed, which raised an exception on reopen.\n" + ) + self.assert_ok(message) + + def test_line_at_limit_passes(self): + line = "x " * 38 # 76 chars, breakable + self.assertEqual(len(line), 76) + self.assert_ok(":bug: Fix crash\n\n" + line + "\n") + + def test_line_one_over_limit_fails(self): + line = "x " * 38 + "x" # 77 chars, breakable + self.assertEqual(len(line), 77) + error = self.assert_fail(":bug: Fix crash\n\n" + line + "\n") + self.assertIn("76", error) + + def test_long_body_line_fails(self): + long_line = "word " * 20 # 100 chars, breakable + error = self.assert_fail(":bug: Fix crash\n\n" + long_line + "\n") + self.assertIn("76", error) + self.assertIn("line 3", error) + + def test_subject_is_not_checked(self): + # The subject has its own length rule; the body validator ignores it. + subject = ":bug: " + "S" * 100 + self.assert_ok(subject + "\n") + + def test_url_line_passes(self): + line = ( + "See https://github.com/penpot/penpot/issues/1234" + "/comments/very/long/fragment" + ) + self.assert_ok(":books: Update docs\n\n" + line + "\n") + + def test_trailer_passes(self): + line = "Signed-off-by: Someone With A Long Name " + self.assert_ok(":bug: Fix crash\n\nBody.\n\n" + line + "\n") + + def test_unbreakable_token_passes(self): + line = "a" * 100 # no whitespace to wrap at + self.assert_ok(":bug: Fix crash\n\n" + line + "\n") + + def test_blank_lines_are_ignored(self): + self.assert_ok(":bug: Fix crash\n\n\n\n") + + def test_multiple_offenders_reported(self): + error = self.assert_fail( + ":bug: Fix crash\n\n" + + ("word " * 20) + + "\n" + + ("other " * 20) + + "\n" + ) + self.assertIn("line 3", error) + self.assertIn("line 4", error) + + +class SubjectRulesRegressionTests(unittest.TestCase): + """Guard the pre-existing validators against accidental breakage.""" + + def test_valid_subject_passes_regex(self): + ok, error = check_commit.check_regex(":bug: Fix crash on startup") + self.assertTrue(ok, error) + + def test_missing_emoji_fails_regex(self): + ok, _ = check_commit.check_regex("Fix crash on startup") + self.assertFalse(ok) + + def test_trailing_dot_fails(self): + ok, _ = check_commit.check_subject_no_trailing_dot(":bug: Fix crash.") + self.assertFalse(ok) + + def test_subject_at_70_chars_passes(self): + # ":bug: " is 6 chars, so 64 chars of text reach exactly 70. + ok, error = check_commit.check_subject_length(":bug: " + "S" * 64) + self.assertTrue(ok, error) + + def test_subject_over_70_chars_fails(self): + ok, error = check_commit.check_subject_length(":bug: " + "S" * 65) + self.assertFalse(ok) + self.assertIn("70", error) + + +if __name__ == "__main__": + unittest.main(verbosity=2)