From 09736aa4c9737e308dd10145197b4ab4cb938641 Mon Sep 17 00:00:00 2001 From: Andrey Antukh Date: Fri, 11 Sep 2026 08:08:57 +0000 Subject: [PATCH 1/2] :sparkles: Enforce commit body line wrapping Add a body line-length validator to scripts/check-commit. It fails when a body line exceeds 76 characters, exempting trailers, URLs, and unbreakable tokens. The 76 limit leaves room for git log's four-space indent in an 80-column terminal. Align the subject limit with the documented 70 characters; the checker allowed 90 before. Document the rule as a hard, verifiable requirement in AGENTS.md, CONTRIBUTING.md, the create-commit skill, and the workflow memory, and point at scripts/check-commit. Add tests for the validator and the subject length rule. AI-assisted-by: deepseek-flash --- .agents/skills/create-commit/SKILL.md | 22 ++- .serena/memories/critical-info.md | 2 +- .serena/memories/workflow/creating-commits.md | 24 +++- AGENTS.md | 3 + CONTRIBUTING.md | 3 + scripts/check-commit | 59 +++++++- scripts/test_check_commit.py | 136 ++++++++++++++++++ 7 files changed, 240 insertions(+), 9 deletions(-) create mode 100644 scripts/test_check_commit.py 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) From 06239844b10b123b7757969ab7618b201d3aba3b Mon Sep 17 00:00:00 2001 From: Andrey Antukh Date: Fri, 11 Sep 2026 12:10:57 +0200 Subject: [PATCH 2/2] :bug: Fix chunked upload storage amplification and cap chunk size (#11635) * :bug: Reject duplicate chunk index in chunked uploads Repeat uploads of the same chunk index each stored a new object because upload-chunk only checked index bounds. Run the handler in a transaction, lock the session row and reject an already-stored index with :duplicate-chunk-index. Also harden assemble-chunks to require exactly indices 0..n-1 so gaps or duplicates fail instead of assembling a corrupt file. Covers media, fonts and binfile through the shared helper. Closes #11634 AI-assisted-by: muse-spark-1.3-contributor * :sparkles: Cap upload chunk size at 30 MiB by default Chunks were only bounded by the 350 MiB HTTP body limit while the 30 MiB caps applied to the assembled file. Add :upload-max-chunk-size (default 30 MiB, tunable via env) and reject oversize chunks in upload-chunk with :validation/:chunk-too-large before anything is stored. App clients slice at 25/10 MiB, so no frontend change needed. AI-assisted-by: muse-spark-1.3-contributor * :bug: Fix tx-run! call and storage resolve in upload-chunk Pass cfg as first arg to db/tx-run!, which expects [system f & params; without it every chunk upload raised invalid system/cfg provided and no chunk was stored, breaking assemble with missing-chunks. Also resolve storage without reuse-conn: put-object! writes to the backend outside any transaction, so reusing the tx connection gives no atomicity. Media, font and storage suites green, lint and format clean. AI-assisted-by: muse-spark-1.3-contributor --- backend/src/app/config.clj | 2 + backend/src/app/rpc/commands/media.clj | 89 +++++--- backend/test/backend_tests/rpc_media_test.clj | 196 ++++++++++++++++++ 3 files changed, 261 insertions(+), 26 deletions(-) diff --git a/backend/src/app/config.clj b/backend/src/app/config.clj index d514c5c9f4..f3a4b7f517 100644 --- a/backend/src/app/config.clj +++ b/backend/src/app/config.clj @@ -91,6 +91,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 #{} @@ -202,6 +203,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 ffa94d5a6b..51f87c320c 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/touch true - :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 @@ -399,6 +421,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." @@ -417,18 +451,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 d22eabe64b..e22ddb5afd 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.