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/backend/src/app/config.clj b/backend/src/app/config.clj index d338bb4813..57c82a859e 100644 --- a/backend/src/app/config.clj +++ b/backend/src/app/config.clj @@ -92,6 +92,7 @@ :quotes-upload-sessions-per-profile 5 :quotes-upload-chunks-per-session 20 + :upload-max-chunk-size (* 1024 1024 30) ; 30MiB ;; SSRF protection :ssrf-allowed-hosts #{} @@ -203,6 +204,7 @@ [:quotes-team-access-requests-per-requester {:optional true} ::sm/int] [:quotes-upload-sessions-per-profile {:optional true} ::sm/int] [:quotes-upload-chunks-per-session {:optional true} ::sm/int] + [:upload-max-chunk-size {:optional true} ::sm/int] [:quotes-media-storage-bytes-per-team {:optional true} ::sm/int] [:auth-token-cookie-name {:optional true} :string] diff --git a/backend/src/app/rpc/commands/media.clj b/backend/src/app/rpc/commands/media.clj index 7bf42c57a9..dbf4a8cb9b 100644 --- a/backend/src/app/rpc/commands/media.clj +++ b/backend/src/app/rpc/commands/media.clj @@ -339,6 +339,9 @@ ;; --- Chunked Upload: Upload a single chunk +(declare ^:private get-upload-chunk) +(declare ^:private check-upload-chunk-slot) + (def ^:private schema:upload-chunk [:map {:title "upload-chunk"} [:session-id ::sm/uuid] @@ -354,9 +357,31 @@ {::doc/added "2.17" ::sm/params schema:upload-chunk ::sm/result schema:upload-chunk-result} - [{:keys [::db/pool] :as cfg} - {:keys [::rpc/profile-id session-id index content] :as _params}] - (let [session (db/get pool :upload-session {:id session-id :profile-id profile-id})] + [cfg {:keys [::rpc/profile-id session-id index content]}] + (let [session (db/tx-run! cfg check-upload-chunk-slot session-id profile-id index content)] + (l/trc :hint "upload-chunk" + :session-id session-id + :chunk (str index "/" (:total-chunks session)) + :size (:size content) + :path (:path content)) + + (let [storage (sto/resolve cfg) + data (sto/content (:path content))] + (sto/put-object! storage + {::sto/content data + ::sto/deduplicate? false + ::sto/touch true + :content-type (:mtype content) + :bucket sto/tempfile-bucket + :upload-id (str session-id) + :chunk-index index})) + + {:session-id session-id + :index index})) + +(defn- check-upload-chunk-slot + [{:keys [::db/conn]} session-id profile-id index content] + (let [session (db/get conn :upload-session {:id session-id :profile-id profile-id} {::db/for-update true})] (when (or (neg? index) (>= index (:total-chunks session))) (ex/raise :type :validation :code :invalid-chunk-index @@ -365,26 +390,23 @@ :total-chunks (:total-chunks session) :index index)) + (when (> (:size content) (cf/get :upload-max-chunk-size)) + (ex/raise :type :validation + :code :chunk-too-large + :hint "chunk size exceeds the maximum allowed" + :session-id session-id + :index index + :size (:size content) + :max-size (cf/get :upload-max-chunk-size))) - (l/trc :hint "upload-chunk" - :session-id session-id - :chunk (str index "/" (:total-chunks session)) - :size (:size content) - :path (:path content))) + (when (get-upload-chunk conn session-id index) + (ex/raise :type :validation + :code :duplicate-chunk-index + :hint "chunk index already uploaded for this session" + :session-id session-id + :index index)) - (let [storage (sto/resolve cfg) - data (sto/content (:path content))] - (sto/put-object! storage - {::sto/content data - ::sto/deduplicate? false - ::sto/touched-at (ct/in-future {:hours 1}) - :content-type (:mtype content) - :bucket sto/tempfile-bucket - :upload-id (str session-id) - :chunk-index index})) - - {:session-id session-id - :index index}) + session)) ;; --- Chunked Upload: shared helpers @@ -400,6 +422,18 @@ [conn session-id] (db/exec! conn [sql:get-upload-chunks (str session-id)])) +(def ^:private sql:get-upload-chunk + "SELECT id + FROM storage_object + WHERE (metadata->>'~:upload-id') = ?::text + AND (metadata->>'~:chunk-index')::integer = ? + AND deleted_at IS NULL + LIMIT 1") + +(defn- get-upload-chunk + [conn session-id index] + (db/exec-one! conn [sql:get-upload-chunk (str session-id) index])) + (defn- concat-chunks "Reads all chunk storage objects in order and writes them to a single temporary file on the local filesystem. Returns a path to that file." @@ -418,18 +452,21 @@ conforming to `media.v/schema:upload` with `:filename`, `:path` and `:size`. - Raises a :validation/:missing-chunks error when the number of stored - chunks does not match `:total-chunks` recorded in the session row. + Raises a :validation/:missing-chunks error when the stored chunk + indices do not form exactly the `0..total-chunks` range recorded in + the session row (wrong count, gaps or duplicates). Raises :not-found when the session does not belong to `profile-id`. Deletes the session row from `upload_session` on success." [{:keys [::db/conn] :as cfg} profile-id session-id] (let [session (db/get conn :upload-session {:id session-id :profile-id profile-id}) - chunks (get-upload-chunks conn session-id)] + chunks (get-upload-chunks conn session-id) + indices (sort (map :chunk-index chunks))] - (when (not= (count chunks) (:total-chunks session)) + (when (or (not= (count chunks) (:total-chunks session)) + (not= indices (range (:total-chunks session)))) (ex/raise :type :validation :code :missing-chunks - :hint "number of stored chunks does not match expected total" + :hint "stored chunks do not match expected total" :session-id session-id :expected (:total-chunks session) :found (count chunks))) diff --git a/backend/test/backend_tests/rpc_media_test.clj b/backend/test/backend_tests/rpc_media_test.clj index 75db78c06b..5a416b3ba4 100644 --- a/backend/test/backend_tests/rpc_media_test.clj +++ b/backend/test/backend_tests/rpc_media_test.clj @@ -681,6 +681,131 @@ (t/is (= :validation (-> out :error ex-data :type))) (t/is (= :missing-chunks (-> out :error ex-data :code)))))) +(t/deftest chunked-upload-assemble-rejects-duplicate-indices + ;; assemble-chunks must validate the index SET, not just the count: a + ;; session declaring 2 chunks but storing [0,0] must fail instead of + ;; assembling a corrupt file. Chunks are written at the storage level + ;; because upload-chunk itself now rejects the second index. + (let [prof (th/create-profile* 1) + _ (th/create-project* 1 {:profile-id (:id prof) + :team-id (:default-team-id prof)}) + file (th/create-file* 1 {:profile-id (:id prof) + :project-id (:default-project-id prof) + :is-shared false}) + session-id (create-session! prof 2) + storage (:app.storage/storage th/*system*) + source-path (th/tempfile "backend_tests/test_files/sample.jpg") + chunks (split-file-into-chunks source-path 312043) + put-chunk! (fn [idx] + (let [mfile (make-chunk-mfile (first chunks) "image/jpeg")] + (sto/put-object! storage + {::sto/content (sto/content (:path mfile)) + ::sto/deduplicate? false + ::sto/touch true + :content-type "image/jpeg" + :bucket sto/tempfile-bucket + :upload-id (str session-id) + :chunk-index idx})))] + (put-chunk! 0) + (put-chunk! 0) + + (let [out (th/command! {::th/type :assemble-file-media-object + ::rpc/profile-id (:id prof) + :session-id session-id + :file-id (:id file) + :is-local true + :name "dupe-indices" + :mtype "image/jpeg"})] + (t/is (some? (:error out))) + (t/is (= :validation (-> out :error ex-data :type))) + (t/is (= :missing-chunks (-> out :error ex-data :code)))))) + +(t/deftest chunked-upload-duplicate-then-assemble + ;; A rejected duplicate must leave the first chunk intact: upload 0, + ;; re-upload 0 (rejected), then assemble succeeds with the original size. + (let [prof (th/create-profile* 1) + _ (th/create-project* 1 {:profile-id (:id prof) + :team-id (:default-team-id prof)}) + file (th/create-file* 1 {:profile-id (:id prof) + :project-id (:default-project-id prof) + :is-shared false}) + session-id (create-session! prof 1) + source-path (th/tempfile "backend_tests/test_files/sample.jpg") + chunks (split-file-into-chunks source-path 312043) + mtype "image/jpeg" + size (alength (first chunks))] + + (let [out (th/command! {::th/type :upload-chunk + ::rpc/profile-id (:id prof) + :session-id session-id + :index 0 + :content (make-chunk-mfile (first chunks) mtype)})] + (t/is (nil? (:error out)))) + + (let [out (th/command! {::th/type :upload-chunk + ::rpc/profile-id (:id prof) + :session-id session-id + :index 0 + :content (make-chunk-mfile (first chunks) mtype)})] + (t/is (some? (:error out))) + (t/is (= :duplicate-chunk-index (-> out :error ex-data :code)))) + + (let [out (th/command! {::th/type :assemble-file-media-object + ::rpc/profile-id (:id prof) + :session-id session-id + :file-id (:id file) + :is-local true + :name "after-dupe" + :mtype mtype})] + (t/is (nil? (:error out))) + (let [storage (:app.storage/storage th/*system*) + mobj (sto/get-object storage (:media-id (:result out)))] + (t/is (= size (:size mobj))))))) + +(t/deftest chunked-upload-rejected-duplicate-keeps-session-usable + ;; Rejecting a duplicate must not poison the session: the remaining + ;; distinct indices still accumulate and assemble normally. + (let [prof (th/create-profile* 1) + _ (th/create-project* 1 {:profile-id (:id prof) + :team-id (:default-team-id prof)}) + file (th/create-file* 1 {:profile-id (:id prof) + :project-id (:default-project-id prof) + :is-shared false}) + session-id (create-session! prof 2) + source-path (th/tempfile "backend_tests/test_files/sample.jpg") + chunks (split-file-into-chunks source-path 110000) + mtype "image/jpeg"] + + (t/is (= 3 (count chunks))) + + (let [out (th/command! {::th/type :upload-chunk + ::rpc/profile-id (:id prof) + :session-id session-id + :index 0 + :content (make-chunk-mfile (nth chunks 0) mtype)})] + (t/is (nil? (:error out)))) + + (let [out (th/command! {::th/type :upload-chunk + ::rpc/profile-id (:id prof) + :session-id session-id + :index 0 + :content (make-chunk-mfile (nth chunks 0) mtype)})] + (t/is (some? (:error out))) + (t/is (= :duplicate-chunk-index (-> out :error ex-data :code)))) + + (let [out (th/command! {::th/type :upload-chunk + ::rpc/profile-id (:id prof) + :session-id session-id + :index 1 + :content (make-chunk-mfile (nth chunks 1) mtype)})] + (t/is (nil? (:error out)))) + + ;; The live store holds exactly the two distinct indices: the + ;; rejected duplicate stored nothing. + (let [rows (th/db-exec! ["SELECT (metadata->>'~:chunk-index')::integer AS idx FROM storage_object WHERE (metadata->>'~:upload-id') = ?::text AND deleted_at IS NULL ORDER BY idx" + (str session-id)])] + (t/is (= [0 1] (mapv :idx rows)))))) + (t/deftest chunked-upload-session-not-found (let [prof (th/create-profile* 1) _ (th/create-project* 1 {:profile-id (:id prof) @@ -767,6 +892,77 @@ (t/is (= :validation (-> out :error ex-data :type))) (t/is (= :invalid-chunk-index (-> out :error ex-data :code)))))) +(t/deftest chunked-upload-duplicate-index-rejected + ;; Uploading the same chunk index twice into one session must fail: + ;; the second call raises :validation / :duplicate-chunk-index and + ;; stores nothing, so one session+index keeps at most one object. + (let [prof (th/create-profile* 1) + session-id (create-session! prof 1) + source-path (th/tempfile "backend_tests/test_files/sample.jpg") + chunks (split-file-into-chunks source-path 312043) + mtype "image/jpeg" + mfile1 (make-chunk-mfile (first chunks) mtype) + mfile2 (make-chunk-mfile (first chunks) mtype)] + + ;; First upload succeeds + (let [out (th/command! {::th/type :upload-chunk + ::rpc/profile-id (:id prof) + :session-id session-id + :index 0 + :content mfile1})] + (t/is (nil? (:error out)))) + + ;; Second upload of the same index must be rejected + (let [out (th/command! {::th/type :upload-chunk + ::rpc/profile-id (:id prof) + :session-id session-id + :index 0 + :content mfile2})] + (t/is (some? (:error out))) + (t/is (= :validation (-> out :error ex-data :type))) + (t/is (= :duplicate-chunk-index (-> out :error ex-data :code)))) + + ;; Exactly one live object stored for that session/index + (let [rows (th/db-exec! ["SELECT id FROM storage_object WHERE (metadata->>'~:upload-id') = ?::text AND (metadata->>'~:chunk-index') = '0' AND deleted_at IS NULL" + (str session-id)])] + (t/is (= 1 (count rows)))))) + +(t/deftest chunked-upload-chunk-too-large + ;; Chunks larger than the configured cap must be rejected with + ;; :validation / :chunk-too-large before anything is stored, while a + ;; chunk exactly at the cap still uploads fine. + (with-mocks [mock {:target 'app.config/get + :return (th/config-get-mock + {:upload-max-chunk-size 1024})}] + (let [prof (th/create-profile* 1) + session-id (create-session! prof 1) + source-path (th/tempfile "backend_tests/test_files/sample.jpg") + chunks (split-file-into-chunks source-path 312043) + mtype "image/jpeg"] + + ;; 312043 bytes exceeds the mocked 1024-byte cap: rejected + (let [out (th/command! {::th/type :upload-chunk + ::rpc/profile-id (:id prof) + :session-id session-id + :index 0 + :content (make-chunk-mfile (first chunks) mtype)})] + (t/is (some? (:error out))) + (t/is (= :validation (-> out :error ex-data :type))) + (t/is (= :chunk-too-large (-> out :error ex-data :code)))) + + ;; Nothing stored for the rejected chunk + (let [rows (th/db-exec! ["SELECT id FROM storage_object WHERE (metadata->>'~:upload-id') = ?::text AND deleted_at IS NULL" + (str session-id)])] + (t/is (= 0 (count rows)))) + + ;; A chunk exactly at the cap still uploads fine + (let [out (th/command! {::th/type :upload-chunk + ::rpc/profile-id (:id prof) + :session-id session-id + :index 0 + :content (make-chunk-mfile (byte-array 1024 (byte 1)) mtype)})] + (t/is (nil? (:error out))))))) + (t/deftest chunked-upload-sessions-per-profile-quota ;; With the session limit set to 2, creating a third session for the ;; same profile must fail with :restriction / :max-quote-reached. 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)