mirror of
https://github.com/penpot/penpot.git
synced 2026-09-11 05:09:34 -04:00
Compare commits
1
Commits
| Author | SHA1 | Date | |
|---|---|---|---|
 Andrey Antukh
|
fa7c8acfa8 |
No files matched your search
@@ -20,17 +20,6 @@ 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
|
||||
@@ -40,18 +29,12 @@ 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 76 characters per line, and run:
|
||||
at 72 characters per line, and run:
|
||||
```bash
|
||||
git commit -m "<subject>" -m "<body>"
|
||||
```
|
||||
(or `git commit -F -` if the body has unusual characters).
|
||||
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
|
||||
4. The `AI-assisted-by` trailer value is provided by the calling context — use
|
||||
it verbatim.
|
||||
|
||||
## Constraints
|
||||
@@ -62,4 +45,3 @@ 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).
|
||||
@@ -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/body format, 76-char body wrapping enforced by `scripts/check-commit`, `AI-assisted-by: model-name` trailer)
|
||||
- Before `git commit` → `mem:workflow/creating-commits` (subject format, body, `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
|
||||
|
||||
@@ -14,32 +14,12 @@ 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 76 characters — git log adds a
|
||||
four-space indent, so 76 + 4 fits an 80-column
|
||||
terminal. Keep each line concise.
|
||||
Wrap lines at 72 characters — git log and tooling
|
||||
render long lines poorly. 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 <ref>`
|
||||
- **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
|
||||
|
||||
@@ -17,9 +17,6 @@
|
||||
- **`.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)
|
||||
|
||||
@@ -188,11 +188,8 @@ 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
|
||||
|
||||
```
|
||||
|
||||
@@ -204,7 +204,7 @@
|
||||
token-id (::actoken/id request)
|
||||
token-type (::actoken/type request)]
|
||||
{:external-session-id session-id
|
||||
:initiator (or key-id "app")
|
||||
:initiator (or (d/name key-id) "app")
|
||||
:access-token-id (some-> token-id str)
|
||||
:access-token-type (some-> token-type str)
|
||||
:client-event-origin client-event-origin
|
||||
|
||||
@@ -390,26 +390,18 @@
|
||||
|
||||
(def ^:private sql:file-comment-users
|
||||
"WITH available_profiles AS (
|
||||
SELECT DISTINCT c.owner_id AS id
|
||||
FROM comment c
|
||||
JOIN comment_thread ct
|
||||
ON ct.id = c.thread_id
|
||||
WHERE ct.file_id = ?::uuid
|
||||
),
|
||||
profile_ids AS (
|
||||
SELECT id FROM available_profiles
|
||||
UNION
|
||||
SELECT ?::uuid
|
||||
SELECT DISTINCT owner_id AS id
|
||||
FROM comment
|
||||
WHERE thread_id IN (SELECT id FROM comment_thread WHERE file_id=?)
|
||||
)
|
||||
SELECT p.id,
|
||||
p.email,
|
||||
p.fullname AS name,
|
||||
p.fullname,
|
||||
p.fullname AS fullname,
|
||||
p.photo_id,
|
||||
p.is_active
|
||||
FROM profile p
|
||||
JOIN profile_ids AS x
|
||||
ON x.id = p.id;")
|
||||
FROM profile AS p
|
||||
WHERE p.id IN (SELECT id FROM available_profiles) OR p.id=?")
|
||||
|
||||
(defn get-file-comments-users
|
||||
[conn file-id profile-id]
|
||||
|
||||
@@ -35,6 +35,21 @@
|
||||
"x-forwarded-for" "127.0.0.44"
|
||||
"x-real-ip" "127.0.0.43"))))
|
||||
|
||||
(t/deftest prepare-context-initiator-is-plain-string
|
||||
;; The initiator must always be a plain string, never a keyword: shared-key
|
||||
;; authenticated callers (exporter, admin-console) arrive as keywords on
|
||||
;; :app.http/auth-key-id and transit would persist them as "~:exporter".
|
||||
(let [base {:headers {"x-forwarded-for" "127.0.0.44"}}]
|
||||
(t/is (= "app" (:initiator (audit/prepare-context-from-request base))))
|
||||
(t/is (= "exporter"
|
||||
(:initiator (audit/prepare-context-from-request
|
||||
(assoc base :app.http/auth-key-id :exporter)))))
|
||||
(t/is (= "admin-console"
|
||||
(:initiator (audit/prepare-context-from-request
|
||||
(assoc base :app.http/auth-key-id :admin-console)))))
|
||||
(t/is (string? (:initiator (audit/prepare-context-from-request
|
||||
(assoc base :app.http/auth-key-id :nexus)))))))
|
||||
|
||||
(t/deftest push-events-1
|
||||
(with-redefs [app.config/flags #{:audit-log}]
|
||||
(let [prof (th/create-profile* 1 {:is-active true})
|
||||
|
||||
@@ -1943,3 +1943,26 @@
|
||||
(t/is (= "bar" (get-in event [:context :foo])))
|
||||
(t/is (= (:full cf/version) (get-in event [:context :version])))
|
||||
(t/is (= "app" (get-in event [:context :initiator]))))))))
|
||||
|
||||
(t/deftest push-audit-events-initiator-is-plain-string
|
||||
;; Shared-key callers (e.g. admin-console) carry :app.http/auth-key-id as a
|
||||
;; keyword; the stored initiator must be a plain string, and a
|
||||
;; caller-supplied initiator must never survive (server context wins).
|
||||
(with-mocks [audit-mock {:target 'app.loggers.audit/submit :return nil}]
|
||||
(binding [cf/flags #{:audit-log}]
|
||||
(let [prof (th/create-profile* 1 {:is-active true})
|
||||
params {::th/type :push-audit-events
|
||||
:events [{:name "context-test"
|
||||
:profile-id (:id prof)
|
||||
:type "action"
|
||||
:context {:custom-key "custom-val"
|
||||
:initiator "spoofed"}}]}
|
||||
params (with-meta params
|
||||
{::http/request (assoc http-request
|
||||
::http/auth-key-id :admin-console)})
|
||||
out (th/management-command! params)]
|
||||
(t/is (nil? (:error out)))
|
||||
(let [[_ event] (:call-args @audit-mock)]
|
||||
(t/is (= "custom-val" (get-in event [:context :custom-key])))
|
||||
(t/is (= "admin-console" (get-in event [:context :initiator])))
|
||||
(t/is (string? (get-in event [:context :initiator]))))))))
|
||||
+4
-55
@@ -5,7 +5,6 @@ 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.
|
||||
|
||||
@@ -39,20 +38,6 @@ 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
|
||||
# ═══════════════════════════════════════════════════════════════════════════════
|
||||
@@ -108,11 +93,11 @@ def check_regex(message):
|
||||
|
||||
|
||||
def check_subject_length(message):
|
||||
"""Subject line must be ≤ 70 characters."""
|
||||
"""Subject line must be ≤ 90 characters."""
|
||||
first_line = message.split("\n")[0]
|
||||
if len(first_line) > 70:
|
||||
if len(first_line) > 90:
|
||||
return False, (
|
||||
f"Subject line exceeds 70 characters ({len(first_line)} chars):\n"
|
||||
f"Subject line exceeds 90 characters ({len(first_line)} chars):\n"
|
||||
f" {first_line}"
|
||||
)
|
||||
return True, None
|
||||
@@ -163,41 +148,6 @@ 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:
|
||||
@@ -229,11 +179,10 @@ def main():
|
||||
|
||||
validators = [
|
||||
("Regex pattern", check_regex),
|
||||
("Subject ≤ 70 chars", check_subject_length),
|
||||
("Subject ≤ 90 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
|
||||
|
||||
@@ -1,136 +0,0 @@
|
||||
#!/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 <someone@example.com>"
|
||||
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)
|
||||
Reference in new issue
Block a user