From 9fa07e7468c629ff58ed6183bb49ba2d926164d1 Mon Sep 17 00:00:00 2001 From: Andrey Antukh Date: Fri, 21 Aug 2026 12:18:33 +0200 Subject: [PATCH 1/5] :arrow_up: Update opencode on devenv dockerfile --- docker/devenv/Dockerfile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docker/devenv/Dockerfile b/docker/devenv/Dockerfile index 8a3d99216d..450cff5b5a 100644 --- a/docker/devenv/Dockerfile +++ b/docker/devenv/Dockerfile @@ -100,7 +100,7 @@ RUN set -eux; \ FROM base AS setup-opencode -ENV OPENCODE_VERSION=1.18.18 +ENV OPENCODE_VERSION=1.18.19 RUN set -ex; \ ARCH="$(dpkg --print-architecture)"; \ From dd4a16321796a534c1457508bd6de195aa83568f Mon Sep 17 00:00:00 2001 From: Andrey Antukh Date: Fri, 21 Aug 2026 12:43:23 +0200 Subject: [PATCH 2/5] :bug: Remove internal error details from HTTP error responses (#11288) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * :ambulance: Remove internal error details from HTTP error responses PostgreSQL exceptions, I/O exceptions, and unhandled errors were leaking raw database messages (table names, constraint names, SQLSTATE codes), filesystem paths, and internal exception details to API clients via :hint, :state, and :path response fields. Remove these fields from server-error responses while keeping full error context in server-side logs for operators. Closes #11287 AI-assisted-by: mimo-v2.5-pro * :ambulance: Strip internal fields and map PG errors to safe messages Complete the security fix for GHSA-r8wx-23q6-w3gf by addressing the incomplete redaction found in code review. Add strip-internal-fields helper to dissoc :hint, :state, :path, and :context from error response data in three handlers that previously passed raw ex-data through to clients: - handle-error :internal - handle-exception :default (else branch) - handle-error :assertion (else branch) Add pgsql-state->message to map PostgreSQL SQLSTATE codes to safe, client-facing messages (e.g. 23505 → "A conflicting entry already exists") instead of returning raw PG error text. Include :message in all PSQLException response branches. Add regression tests asserting :hint, :state, :path, :context are absent from responses for :internal and unhandled ex-info errors. Closes #11287 AI-assisted-by: mimo-v2.5-pro * :ambulance: Keep :hint in error protocol, fix unsafe sources Refine the security fix based on code review feedback. Keep :hint as part of the error protocol — it is essential for controlled error communication. Remove it from strip-internal-fields (which now only strips :state, :path, :context). Fix the actual sources of unsafe :hint values: - http/middleware.clj: replace (ex-message cause) with safe static strings for IllegalArgumentException, RequestTooBigException, and EOFException. These :validation errors return ex-data verbatim to clients, so raw exception messages were leaking internals. - PSQLException handler: use :hint instead of :message for the SQLSTATE-mapped messages, staying consistent with the error protocol. Update tests to assert :hint is present (with safe static values) in :internal and unhandled ex-info responses, and absent only from bare RuntimeException and IOException responses. Closes #11287 AI-assisted-by: mimo-v2.5-pro --- backend/src/app/http/errors.clj | 45 +++++++++++----- backend/src/app/http/middleware.clj | 6 +-- .../backend_tests/http_middleware_test.clj | 53 +++++++++++++++++-- 3 files changed, 82 insertions(+), 22 deletions(-) diff --git a/backend/src/app/http/errors.clj b/backend/src/app/http/errors.clj index f1eaea621c..2393abf129 100644 --- a/backend/src/app/http/errors.clj +++ b/backend/src/app/http/errors.clj @@ -34,6 +34,12 @@ (assoc :request/auth-data (dissoc auth :token)) (assoc :frontend/version (or (yreq/get-header request "x-frontend-version") "unknown"))))) +(defn- strip-internal-fields + "Remove fields that leak internal implementation details from error + response data. Full context is preserved in server-side logs." + [data] + (dissoc data :state :path :context)) + (defmulti handle-error (fn [cause _ _] (-> cause ex-data :type))) @@ -136,6 +142,7 @@ (l/error :hint "assertion error" :cause cause) {::yres/status 500 ::yres/body (-> data + (strip-internal-fields) (assoc :type :server-error) (assoc :code :assertion))}))))) @@ -161,9 +168,9 @@ (l/error :hint "internal error" :cause cause) {::yres/status 500 ::yres/body (-> data + (strip-internal-fields) (assoc :type :server-error) - (update :code #(or % :unhandled)) - (assoc :hint (ex-message error)))}))) + (update :code #(or % :unhandled)))}))) (defmethod handle-error :default [error request parent-cause] @@ -178,6 +185,20 @@ (handle-exception (:handling edata) request error) (handle-exception error request parent-cause)))) +(defn- pgsql-state->message + "Map PostgreSQL SQLSTATE codes to safe, client-facing messages. + Returns a user-friendly string that conveys the nature of the error + without exposing table names, constraint names, or other internals." + [state] + (case state + "23505" "A conflicting entry already exists" + "23503" "The referenced item does not exist" + "23502" "A required field is missing" + "23514" "The value violates a data integrity constraint" + "57014" "The operation took too long and was cancelled" + "25P03" "The transaction was idle too long and was cancelled" + "A database error occurred")) + (defmethod handle-exception org.postgresql.util.PSQLException [error request parent-cause] (let [state (.getSQLState ^java.sql.SQLException error) @@ -190,20 +211,19 @@ {::yres/status 504 ::yres/body {:type :server-error :code :statement-timeout - :hint (ex-message error)}} + :hint (pgsql-state->message state)}} (= state "25P03") {::yres/status 504 ::yres/body {:type :server-error :code :idle-in-transaction-timeout - :hint (ex-message error)}} + :hint (pgsql-state->message state)}} :else {::yres/status 500 ::yres/body {:type :server-error - :code :unexpected - :hint (ex-message error) - :state state}})))) + :code :database-error + :hint (pgsql-state->message state)}})))) (defmethod handle-exception :default [error request parent-cause] @@ -216,17 +236,16 @@ (l/error :hint "unexpected error" :cause cause) {::yres/status 500 ::yres/body {:type :server-error - :code :unexpected - :hint (ex-message error)}}) + :code :unexpected}}) :else (binding [l/*context* (request->context request)] (l/error :hint "unhandled error" :cause cause) {::yres/status 500 ::yres/body (-> edata + (strip-internal-fields) (assoc :type :server-error) - (update :code #(or % :unhandled)) - (assoc :hint (ex-message error)))})))) + (update :code #(or % :unhandled)))})))) (defmethod handle-exception java.io.IOException [cause request _] @@ -234,9 +253,7 @@ (l/wrn :hint "io exception" :cause cause) {::yres/status 500 ::yres/body {:type :server-error - :code :io-exception - :hint (ex-message cause) - :path (:path request)}})) + :code :io-exception}})) (defmethod handle-exception java.util.concurrent.CompletionException [cause request _] diff --git a/backend/src/app/http/middleware.clj b/backend/src/app/http/middleware.clj index 31b96927a6..6cb8e6b8b7 100644 --- a/backend/src/app/http/middleware.clj +++ b/backend/src/app/http/middleware.clj @@ -83,18 +83,18 @@ (instance? IllegalArgumentException cause) (ex/raise :type :validation :code :malformed-json - :hint (ex-message cause) + :hint "invalid JSON in request body" :cause cause) (instance? RequestTooBigException cause) (ex/raise :type :validation :code :request-body-too-large - :hint (ex-message cause)) + :hint "request body exceeds size limit") (instance? java.io.EOFException cause) (ex/raise :type :validation :code :malformed-json - :hint (ex-message cause) + :hint "unexpected end of request body" :cause cause) (instance? RuntimeException cause) diff --git a/backend/test/backend_tests/http_middleware_test.clj b/backend/test/backend_tests/http_middleware_test.clj index bd986fc031..bca962d3fc 100644 --- a/backend/test/backend_tests/http_middleware_test.clj +++ b/backend/test/backend_tests/http_middleware_test.clj @@ -6,10 +6,12 @@ (ns backend-tests.http-middleware-test (:require + [app.common.exceptions :as ex] [app.common.time :as ct] [app.db :as db] [app.http :as-alias http] [app.http.access-token] + [app.http.errors :as http-errors] [app.http.middleware :as mw] [app.http.session :as session] [app.main :as-alias main] @@ -300,7 +302,7 @@ (t/is (instance? clojure.lang.ExceptionInfo ex)) (t/is (= :validation (-> ex ex-data :type))) (t/is (= :malformed-json (-> ex ex-data :code))) - (t/is (string? (-> ex ex-data :hint))))) + (t/is (= "invalid JSON in request body" (-> ex ex-data :hint))))) (t/deftest parse-request-request-too-big-exception ;; When RequestTooBigException is raised (e.g. the request body @@ -319,7 +321,7 @@ (t/is (instance? clojure.lang.ExceptionInfo ex)) (t/is (= :validation (-> ex ex-data :type))) (t/is (= :request-body-too-large (-> ex ex-data :code))) - (t/is (string? (-> ex ex-data :hint))))) + (t/is (= "request body exceeds size limit" (-> ex ex-data :hint))))) (t/deftest parse-request-eof-exception ;; When java.io.EOFException is raised (e.g. the body stream @@ -337,7 +339,7 @@ (t/is (instance? clojure.lang.ExceptionInfo ex)) (t/is (= :validation (-> ex ex-data :type))) (t/is (= :malformed-json (-> ex ex-data :code))) - (t/is (string? (-> ex ex-data :hint))))) + (t/is (= "unexpected end of request body" (-> ex ex-data :hint))))) (t/deftest parse-request-runtime-exception-with-cause ;; When a RuntimeException with a non-nil ex-cause is raised, @@ -377,7 +379,7 @@ (t/is (= 500 (::yres/status response))) (t/is (= :server-error (:type body))) (t/is (= :unexpected (:code body))) - (t/is (= "boom" (:hint body))))) + (t/is (nil? (:hint body))))) (t/deftest parse-request-non-runtime-throwable ;; When a non-RuntimeException Throwable is raised (e.g. an @@ -397,4 +399,45 @@ (t/is (= 500 (::yres/status response))) (t/is (= :server-error (:type body))) (t/is (= :io-exception (:code body))) - (t/is (= "network gone" (:hint body))))) + (t/is (nil? (:hint body))))) + +(t/deftest internal-error-strips-sensitive-fields + ;; When an :internal error is raised with :state, :path, and + ;; :context, those fields must not appear in the response body. + ;; :hint is part of the error protocol and is preserved. + (let [cause (ex-info "internal error" + {:type :internal + :code :test-error + :hint "safe user-facing hint" + :state "XX000" + :path "/data/penpot/storage" + :context {:backend :s3 :bucket "prod"}}) + response (http-errors/handle cause {}) + body (::yres/body response)] + (t/is (= 500 (::yres/status response))) + (t/is (= :server-error (:type body))) + (t/is (= :test-error (:code body))) + (t/is (= "safe user-facing hint" (:hint body))) + (t/is (nil? (:state body))) + (t/is (nil? (:path body))) + (t/is (nil? (:context body))))) + +(t/deftest unhandled-exinfo-strips-sensitive-fields + ;; When an ex-info with an unregistered :type (dispatches through + ;; handle-exception :default :else) carries :state and :path, + ;; those fields must not appear in the response body. + ;; :hint is part of the error protocol and is preserved. + (let [cause (ex-info "something broke" + {:type :unregistered-type + :code :custom-code + :hint "safe user-facing hint" + :state "internal-state" + :path "/internal/path"}) + response (http-errors/handle cause {}) + body (::yres/body response)] + (t/is (= 500 (::yres/status response))) + (t/is (= :server-error (:type body))) + (t/is (= :custom-code (:code body))) + (t/is (= "safe user-facing hint" (:hint body))) + (t/is (nil? (:state body))) + (t/is (nil? (:path body))))) From 7c85837290c4e7d6f7d99472b092ad4f7c9d6a97 Mon Sep 17 00:00:00 2001 From: Andrey Antukh Date: Fri, 21 Aug 2026 12:55:24 +0200 Subject: [PATCH 3/5] :bug: Fix session invalidation on logout to prevent token replay (#11317) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Logout only cleared the auth-token cookie but never deleted the server-side row because delete-fn read ::id which wrap-authz no longer sets since 363b4e3778. Make delete-fn delete via ::session/:id attached by wrap-authz so replayed tokens are rejected (CWE-613, GHSA-mj9f-5cwq-7p3q). Add regression tests covering invalidation, idempotency and isolation of other sessions. Fix verified with Red→Green TDD and full backend suite (677 tests). Closes #11316 AI-assisted-by: muse-spark-1.2-contributor --- backend/src/app/http/session.clj | 2 +- backend/test/backend_tests/rpc_auth_test.clj | 106 +++++++++++++++++++ 2 files changed, 107 insertions(+), 1 deletion(-) create mode 100644 backend/test/backend_tests/rpc_auth_test.clj diff --git a/backend/src/app/http/session.clj b/backend/src/app/http/session.clj index 61140a780c..5782b3452e 100644 --- a/backend/src/app/http/session.clj +++ b/backend/src/app/http/session.clj @@ -204,7 +204,7 @@ [{:keys [::manager]}] (assert (manager? manager) "expected valid session manager") (fn [request response] - (some->> (get request ::id) (delete-session manager)) + (some->> (get request ::session) :id (delete-session manager)) (clear-session-cookie response))) (defn decode-token diff --git a/backend/test/backend_tests/rpc_auth_test.clj b/backend/test/backend_tests/rpc_auth_test.clj new file mode 100644 index 0000000000..94adfb0d3a --- /dev/null +++ b/backend/test/backend_tests/rpc_auth_test.clj @@ -0,0 +1,106 @@ +;; This Source Code Form is subject to the terms of the Mozilla Public +;; License, v. 2.0. If a copy of the MPL was not distributed with this +;; file, You can obtain one at http://mozilla.org/MPL/2.0/. +;; +;; Copyright (c) KALEIDOS INC Sucursal en España SL + +(ns backend-tests.rpc-auth-test + (:require + [app.common.uuid :as uuid] + [app.http.session :as session] + [backend-tests.helpers :as th] + [clojure.test :as t] + [yetti.response :as yres])) + +(t/use-fixtures :once th/state-init) +(t/use-fixtures :each th/database-reset) + +(t/deftest logout-invalidates-current-session + (let [prof (th/create-profile* 1) + manager (::session/manager th/*system*) + sid (uuid/random) + _ (th/db-exec-one! ["INSERT INTO http_session_v2 (id, profile_id, user_agent) VALUES (?, ?, ?)" + sid (:id prof) "test-agent"]) + session (session/read-session manager sid)] + + ;; Arrange: session exists before logout + (t/is (some? session) "session should exist before logout") + (t/is (= sid (:id session))) + + ;; Act: simulate Ring request as produced by wrap-authz (has ::session/session) + ;; delete-fn is used as response transform via rph/with-transform in auth/logout + (let [request {::session/session session} + response {} + delete-fn (session/delete-fn th/*system*) + result (delete-fn request response)] + + ;; Assert: server-side session is deleted (CWE-613) + (t/is (nil? (session/read-session manager sid)) + "session must be deleted server-side after logout (GHSA-mj9f-5cwq-7p3q)") + + ;; Assert: cookie is cleared + (t/is (= "" (get-in result [::yres/cookies "auth-token" :value])) + "auth-token cookie should be cleared") + (t/is (= 0 (get-in result [::yres/cookies "auth-token" :max-age])) + "auth-token cookie max-age should be 0")))) + +(t/deftest logout-clears-cookie-even-when-session-missing + (let [manager (::session/manager th/*system*) + sid (uuid/random) + ;; No session inserted, read should be nil + _ (t/is (nil? (session/read-session manager sid))) + request {} + response {} + delete-fn (session/delete-fn th/*system*) + result (delete-fn request response)] + + ;; Should still clear cookie (idempotent) + (t/is (= "" (get-in result [::yres/cookies "auth-token" :value]))) + (t/is (= 0 (get-in result [::yres/cookies "auth-token" :max-age]))))) + +(t/deftest logout-does-not-invalidate-other-sessions + (let [prof (th/create-profile* 1) + manager (::session/manager th/*system*) + sid1 (uuid/random) + sid2 (uuid/random) + _ (th/db-exec-one! ["INSERT INTO http_session_v2 (id, profile_id, user_agent) VALUES (?, ?, ?)" + sid1 (:id prof) "agent-1"]) + _ (th/db-exec-one! ["INSERT INTO http_session_v2 (id, profile_id, user_agent) VALUES (?, ?, ?)" + sid2 (:id prof) "agent-2"]) + s1 (session/read-session manager sid1) + s2 (session/read-session manager sid2)] + + (t/is (some? s1)) + (t/is (some? s2)) + + ;; Logout only sid1 + (let [request {::session/session s1} + response {} + delete-fn (session/delete-fn th/*system*)] + (delete-fn request response)) + + ;; sid1 deleted, sid2 intact + (t/is (nil? (session/read-session manager sid1)) "current session should be deleted") + (t/is (some? (session/read-session manager sid2)) "other sessions should remain"))) + +(t/deftest replay-after-logout-cannot-authenticate + (let [prof (th/create-profile* 1) + manager (::session/manager th/*system*) + sid (uuid/random) + _ (th/db-exec-one! ["INSERT INTO http_session_v2 (id, profile_id, user_agent) VALUES (?, ?, ?)" + sid (:id prof) "test-agent"]) + session (session/read-session manager sid)] + + (t/is (some? session) "session exists before logout") + + ;; Simulate logout + (let [request {::session/session session} + response {} + delete-fn (session/delete-fn th/*system*)] + (delete-fn request response)) + + ;; Replay: attempt to read session with same sid should fail (no profile attached) + (t/is (nil? (session/read-session manager sid)) + "replayed token must not resolve to a valid session after logout"))) + + From 0cacf9bd998db42b405ec5f1f6bf75e5ffe89f83 Mon Sep 17 00:00:00 2001 From: Andrey Antukh Date: Fri, 21 Aug 2026 12:08:26 +0000 Subject: [PATCH 4/5] :recycle: Rename code-review-and-quality skill to code-review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Rename skill directory and update all references to follow the same naming pattern as plan-review. Simplify review.md command from 138 to 25 lines — remove redundant content that duplicated what the skills already define. The command now acts as a thin router; the skills own the methodology. AI-assisted-by: mimo-v2.5-pro --- .opencode/commands/review.md | 125 +----------------- .../SKILL.md | 2 +- .opencode/skills/plan-review/SKILL.md | 6 +- 3 files changed, 10 insertions(+), 123 deletions(-) rename .opencode/skills/{code-review-and-quality => code-review}/SKILL.md (99%) diff --git a/.opencode/commands/review.md b/.opencode/commands/review.md index 23d7941ce7..70bd89ac90 100644 --- a/.opencode/commands/review.md +++ b/.opencode/commands/review.md @@ -3,60 +3,14 @@ Act as a senior software engineer and perform a thorough review. ## Instructions 1. **Determine what is being reviewed** from the provided context: - - **If it is a plan** (implementation plan, design document, task breakdown) → follow the **Plan Review** path below. - - **If it is code** (diff, PR, code change) → follow the **Code Review** path below. + - **If it is a plan** (implementation plan, design document, task breakdown) → load the **`plan-review`** skill. + - **If it is code** (diff, PR, code change) → load the **`code-review`** skill. ---- +2. Read `AGENTS.md` and follow its instructions for finding and reading all related testing documentation from memories before reviewing. -## Code Review Path +3. **Skip generated files, lockfile-only changes, and unrelated modifications** unless they introduce security risks. -1. Load the **`code-review-and-quality`** skill — it defines the five axes, core principles (DRY, KISS, YAGNI), severity taxonomy, and output format. -2. Read `AGENTS.md` and follow its instructions for finding and reading all related testing documentation from memories before reviewing the code. -3. Determine the diff or code to review from the provided context. -4. **Skip generated files, lockfile-only changes, and unrelated modifications** unless they introduce security risks. -5. Read the diff and the surrounding context for each changed file. -6. Review across all five axes: correctness, readability, architecture, security, performance. -7. Produce the review using the **Code Review Format** below. -8. For each finding: - - State the severity (Critical / High / Medium / Low / Suggestion) - - Identify the file and line - - Describe failure circumstances - - **For Critical/High**: Provide a concrete fix with a code snippet showing the corrected code - - **For Medium/Low**: Describe the fix clearly; code snippet optional - - If multiple approaches exist, briefly note trade-offs -9. **Perform a second review pass if the change is complex:** - - **Complex indicators**: Critical/High findings, multiple files (>5), architectural changes, security-sensitive code, >300 lines changed - - **Skip for simple changes**: Typo fixes, formatting, small bug fixes (<50 lines), single-file changes with no findings - - Second pass checks: - - Validate severity assignments: Are Critical/High findings truly blockers? - - Catch missed issues: Edge cases, error paths, test gaps overlooked in first pass - - Remove false positives: Discard findings that aren't real issues - - Verify fixes: Are the proposed solutions actually correct and complete? - ---- - -## Plan Review Path - -1. Load the **`plan-review`** skill — it defines the six axes, severity taxonomy, and output format. -2. Read the full plan from the provided context. -3. Review across all six axes: completeness, task quality, architecture & sequencing, risk coverage, actionability, and proposed code quality (if the plan includes implementation details). -4. Produce the review using the **Plan Review Format** below. -5. For each finding: - - State the severity (Critical / Required / Nit / Optional / FYI) - - Identify the section or task it refers to - - Describe the gap or problem - - **For Critical/Required**: Propose a concrete fix or addition - - **For Nit/Optional**: Describe the improvement; concrete text optional -6. **Perform a second review pass if the plan is complex:** - - **Complex indicators**: Critical findings, >10 tasks, migrations or breaking changes, security-sensitive features - - **Skip for simple plans**: 1–2 tasks, no risks, no code proposals - - Second pass checks: - - Validate severity assignments - - Catch missed gaps: edge cases, missing dependencies, unaddressed risks - - Remove false positives - - Verify proposed remedies are actionable - ---- +4. Follow the loaded skill's process and produce its output format. ## Strong Rules @@ -64,75 +18,8 @@ Act as a senior software engineer and perform a thorough review. 2. Do not modify any code and do not create a commit — this command only reviews. 3. Be specific and constructive. "This could be better" is not helpful — explain why and how. 4. Prioritize by impact. One structural issue outweighs ten nits. -5. Missing tests are an issue, not a suggestion. If tests are missing or inadequate for new functionality, report it as a severity-tagged finding in the findings sections below — High severity (code) or Required (plan) — never as a recommendation. +5. Missing tests are an issue, not a suggestion. Report as a severity-tagged finding — never as a recommendation. ## Context $ARGUMENTS - -## Expected Format — Code Review - -``` -## Review Summary -[1-2 sentences on what the change does and overall assessment] - -## Critical/High Findings - -### [Severity] file.ts:123 -**Issue**: [Description of the problem] -**Impact**: [What could go wrong if this is not fixed] -**Fix**: - -````[language] -// Current code -[problematic code] - -// Fixed code -[corrected code] -[Optional: note trade-offs if multiple approaches exist] -```` - -### [Severity] file.ts:456 -**Issue**: [Description of the problem] -**Impact**: [What could go wrong if this is not fixed] -**Fix**: [Clear description of the fix; code snippet if it clarifies] - -## Other Findings - -### [Severity] file.ts:789 -**Issue**: [Description] -**Impact**: [Minor consequence or risk] -**Fix**: [Clear description; code snippet optional] - -## Positive Observations -[2-3 specific things done well] - -## Verdict -[Approve / Request Changes / Needs Discussion] -[If Request Changes: list the must-fix items] -``` - -## Expected Format — Plan Review - -``` -## Review Summary -[1-2 sentences on the plan's goal and overall assessment] - -## Critical/Required Findings -### [Severity] [Section or Task N] -**Issue**: [Description of the gap or problem] -**Impact**: [What could go wrong during implementation] -**Proposed fix**: [Concrete addition or change to the plan] - -## Other Findings -### [Severity] [Section or Task N] -**Issue**: [Description] -**Proposed fix**: [Clear description; concrete text optional] - -## Strengths -[2-3 specific things done well in the plan] - -## Verdict -[Approve / Request Changes / Needs Discussion] -[If Request Changes: list the must-fix items] -``` diff --git a/.opencode/skills/code-review-and-quality/SKILL.md b/.opencode/skills/code-review/SKILL.md similarity index 99% rename from .opencode/skills/code-review-and-quality/SKILL.md rename to .opencode/skills/code-review/SKILL.md index a0f75e7f99..7f06d90b59 100644 --- a/.opencode/skills/code-review-and-quality/SKILL.md +++ b/.opencode/skills/code-review/SKILL.md @@ -1,5 +1,5 @@ --- -name: code-review-and-quality +name: code-review description: Conducts multi-axis code review. Use before merging any change. Use when reviewing code written by yourself, another agent, or a human. Use when you need to assess code quality across multiple dimensions before it enters the main branch. --- diff --git a/.opencode/skills/plan-review/SKILL.md b/.opencode/skills/plan-review/SKILL.md index 60b61c75e8..4386d701cc 100644 --- a/.opencode/skills/plan-review/SKILL.md +++ b/.opencode/skills/plan-review/SKILL.md @@ -87,7 +87,7 @@ Can an implementer actually execute this? ### 6. Proposed Code Quality *(when the plan includes implementation details)* -If the plan proposes code shapes, function signatures, data structures, or API designs, evaluate those proposals against `code-review-and-quality` criteria: +If the plan proposes code shapes, function signatures, data structures, or API designs, evaluate those proposals against `code-review` criteria: - **Correctness:** Do the proposed types/signatures handle edge cases (null, empty, boundaries)? - **Readability:** Are proposed names descriptive and consistent with project conventions? @@ -215,7 +215,7 @@ Check that the plan can actually confirm it worked: If the plan includes code snippets, types, or API designs: ``` -- Load code-review-and-quality skill for criteria +- Load code-review skill for criteria - Check proposed signatures for edge cases - Verify naming follows project conventions - Confirm abstractions follow existing patterns @@ -310,6 +310,6 @@ If the plan includes code snippets, types, or API designs: ## See Also - For producing plans, use the `planner` skill -- For reviewing implemented code, use `code-review-and-quality` — also the criteria source for axis 6 +- For reviewing implemented code, use `code-review` — also the criteria source for axis 6 - For security-specific concerns, see `security-and-hardening` - For testing strategy guidance, see `testing` From 47d599fe346ac5478df37466135cfe43ff1051a6 Mon Sep 17 00:00:00 2001 From: Andrey Antukh Date: Fri, 21 Aug 2026 14:37:21 +0200 Subject: [PATCH 5/5] :sparkles: Persist binfile manifest and emit workspace audit events (#11106) (#11138) Persist binfile manifest metadata in file_data on import so file statistics are available at open-workspace time. Emit a new open-workspace-file audit event enriched with file statistics: page count, shape count, component count, linked libraries, design tokens, and whether the file is a shared library. Closes #11106 AI-assisted-by: mimo-v2.5-pro --- backend/src/app/binfile/common.clj | 1 + backend/src/app/binfile/v3.clj | 8 +- backend/src/app/config.clj | 2 +- backend/src/app/features/fdata.clj | 20 ++--- backend/test/backend_tests/binfile_test.clj | 23 +++++ common/src/app/common/types/file.cljc | 10 +++ frontend/src/app/main/data/workspace.cljs | 61 +++++++++++++ .../data/workspace_stats_test.cljs | 87 +++++++++++++++++++ 8 files changed, 196 insertions(+), 16 deletions(-) create mode 100644 frontend/test/frontend_tests/data/workspace_stats_test.cljs diff --git a/backend/src/app/binfile/common.clj b/backend/src/app/binfile/common.clj index f984a98550..3e4402be92 100644 --- a/backend/src/app/binfile/common.clj +++ b/backend/src/app/binfile/common.clj @@ -723,6 +723,7 @@ (-> (select-keys file file-attrs) (assoc :data nil) (dissoc :team-id) + (dissoc :metadata) (dissoc :migrations))) (defn- file->file-data-params diff --git a/backend/src/app/binfile/v3.clj b/backend/src/app/binfile/v3.clj index 952cb69e8f..eab49e1eb3 100644 --- a/backend/src/app/binfile/v3.clj +++ b/backend/src/app/binfile/v3.clj @@ -392,7 +392,7 @@ params {:type "penpot/export-files" :version 1 :generated-by (str "penpot/" (:full cf/version)) - :refer "penpot" + :referer "penpot" :files (vec (vals files)) :relations rels}] (write-entry! output "manifest.json" params)))) @@ -734,7 +734,7 @@ :plugin-data plugin-data})) (defn- import-file - [{:keys [::db/conn ::bfc/project-id] :as cfg} {file-id :id file-name :name}] + [{:keys [::db/conn ::bfc/project-id ::manifest] :as cfg} {file-id :id file-name :name}] (let [file-id' (bfc/lookup-index file-id) file (read-file cfg file-id) media (read-file-media cfg file-id) @@ -801,8 +801,10 @@ (assoc :data data) (assoc :name file-name) (assoc :project-id project-id) + (assoc :metadata (d/without-nils + {:generated-by (get manifest :generated-by) + :referer (or (get manifest :referer) (get manifest :refer))})) (dissoc :options)) - file (bfc/process-file cfg file) file (ctf/check-file file)] diff --git a/backend/src/app/config.clj b/backend/src/app/config.clj index bebd5db826..f02136b1ca 100644 --- a/backend/src/app/config.clj +++ b/backend/src/app/config.clj @@ -52,7 +52,7 @@ :redis-uri "redis://redis/0" - :file-data-backend "legacy-db" + :file-data-backend "db" :objects-storage-backend "fs" :objects-storage-fs-directory "assets" diff --git a/backend/src/app/features/fdata.clj b/backend/src/app/features/fdata.clj index 412ca223cf..8e7ff9d978 100644 --- a/backend/src/app/features/fdata.clj +++ b/backend/src/app/features/fdata.clj @@ -12,6 +12,7 @@ [app.common.logging :as l] [app.common.schema :as sm] [app.common.time :as ct] + [app.common.types.file :as ctf] [app.common.types.objects-map :as omap] [app.config :as cf] [app.db :as db] @@ -159,15 +160,17 @@ :content-type "application/octet-stream" :file-id file-id :id id}) - metadata {:storage-ref-id (:id sobject)} + metadata (-> (:metadata params) + (assoc :storage-ref-id (:id sobject))) params (-> params (assoc :metadata metadata) (assoc :data nil))] (upsert-in-database cfg params)) (= backend "db") - (->> (dissoc params :metadata) - (upsert-in-database cfg)) + (let [metadata (dissoc (:metadata params) :storage-ref-id) + params (assoc params :metadata metadata)] + (upsert-in-database cfg params)) (= backend "legacy-db") (cond @@ -213,18 +216,11 @@ [backend] (or backend (cf/get :file-data-backend))) -(def ^:private schema:metadata - [:map {:title "Metadata"} - [:storage-ref-id {:optional true} ::sm/uuid]]) - -(def decode-metadata-with-schema - (sm/decoder schema:metadata sm/json-transformer)) - (defn decode-metadata [metadata] (some-> metadata (db/decode-json-pgobject) - (decode-metadata-with-schema))) + (ctf/decode-file-metadata))) (def ^:private schema:update-params [:map {:closed true} @@ -232,7 +228,7 @@ [:type [:enum "main" "snapshot" "fragment"]] [:file-id ::sm/uuid] [:backend {:optional true} [:enum "db" "legacy-db" "storage"]] - [:metadata {:optional true} [:maybe schema:metadata]] + [:metadata {:optional true} ctf/schema:file-metadata] [:data {:optional true} bytes?] [:created-at {:optional true} ::ct/inst] [:modified-at {:optional true} [:maybe ::ct/inst]] diff --git a/backend/test/backend_tests/binfile_test.clj b/backend/test/backend_tests/binfile_test.clj index 05f1525c5e..c0e45d5429 100644 --- a/backend/test/backend_tests/binfile_test.clj +++ b/backend/test/backend_tests/binfile_test.clj @@ -207,6 +207,29 @@ (t/is (= (count result) 1)) (t/is (every? uuid? result))))) +(t/deftest import-binfile-v3-persists-manifest-metadata + (let [profile (th/create-profile* 1) + file (prepare-simple-file profile) + output (tmp/tempfile :suffix ".zip")] + + (v3/export-files! + (-> th/*system* + (assoc ::bfc/ids #{(:id file)}) + (assoc ::bfc/embed-assets false) + (assoc ::bfc/include-libraries false)) + (io/output-stream output)) + + (let [result (-> th/*system* + (assoc ::bfc/project-id (:default-project-id profile)) + (assoc ::bfc/profile-id (:id profile)) + (assoc ::bfc/input output) + (v3/import-files!)) + imported (bfc/get-file th/*system* (first result))] + + (t/is (= (count result) 1)) + (t/is (some? (get-in imported [:metadata :generated-by]))) + (t/is (= "penpot" (get-in imported [:metadata :referer])))))) + (t/deftest read-obj-rejects-oversized-buffer ;; N1-07: read-obj! must reject objects exceeding max-object-size ;; before attempting to allocate the buffer diff --git a/common/src/app/common/types/file.cljc b/common/src/app/common/types/file.cljc index fe91dca8ad..d7e9feb676 100644 --- a/common/src/app/common/types/file.cljc +++ b/common/src/app/common/types/file.cljc @@ -88,6 +88,12 @@ [:plugin-data {:optional true} schema:plugin-data] [:tokens-lib {:optional true} schema:tokens-lib]]) +(def schema:file-metadata + [:map {:title "Metadata"} + [:storage-ref-id {:optional true} ::sm/uuid] + [:generated-by {:optional true} :string] + [:referer {:optional true} :string]]) + (def schema:file "A schema for validate a file data structure; data is optional because sometimes we want to validate file without the data." @@ -106,6 +112,7 @@ [:data {:optional true} schema:data] [:version :int] [:features ::cfeat/features] + [:metadata {:optional true} schema:file-metadata] [:migrations {:optional true} [::sm/set {:ordered true} :string]]]) @@ -123,6 +130,9 @@ (def check-file-media (sm/check-fn schema:media)) +(def decode-file-metadata + (sm/decoder schema:file-metadata sm/json-transformer)) + ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;; ;; INITIALIZATION ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;; diff --git a/frontend/src/app/main/data/workspace.cljs b/frontend/src/app/main/data/workspace.cljs index 72e5f93f3e..518729d806 100644 --- a/frontend/src/app/main/data/workspace.cljs +++ b/frontend/src/app/main/data/workspace.cljs @@ -17,11 +17,13 @@ [app.common.geom.proportions :as gpp] [app.common.geom.shapes :as gsh] [app.common.logging :as log] + [app.common.math :as mth] [app.common.path-names :as cpn] [app.common.transit :as t] [app.common.types.component :as ctc] [app.common.types.components-list :as ctkl] [app.common.types.shape :as cts] + [app.common.types.tokens-lib :as ctob] [app.common.types.variant :as ctv] [app.common.uuid :as uuid] [app.config :as cf] @@ -266,6 +268,59 @@ (rx/map (fn [_] (mcp/init)))) (rx/empty)))))) +(defn- compute-shape-stats + "Compute shape statistics in a single pass over pages-index. + Returns {:num-shapes N :max-shapes-per-page M}" + [pages-index] + (reduce-kv + (fn [acc _page-id page] + (let [n (count (:objects page))] + (-> acc + (update :num-shapes + n) + (update :max-shapes-per-page max n)))) + {:num-shapes 0 + :max-shapes-per-page 0} + pages-index)) + +(defn compute-file-stats + "Compute file statistics. Returns a map of stats without event keys." + [state file-id] + (let [file (dsh/lookup-file state file-id) + file-data (:data file) + libraries (refs/select-libraries (:files state) file-id) + pages-index (:pages-index file-data) + {:keys [num-shapes max-shapes-per-page]} (compute-shape-stats pages-index) + n-pages (count (:pages file-data)) + n-components (reduce-kv (fn [n _ c] (if (:deleted c) n (inc n))) + 0 (:components file-data)) + n-linked-libs (dec (count libraries)) + tokens-lib (:tokens-lib file-data) + n-tokens (if (some? tokens-lib) + (count (ctob/get-all-tokens tokens-lib)) + 0)] + {:num-pages n-pages + :num-shapes num-shapes + :avg-shapes-per-page (if (pos? n-pages) + (mth/round (/ num-shapes n-pages)) + 0) + :max-shapes-per-page max-shapes-per-page + :num-components n-components + :num-linked-libraries (max 0 n-linked-libs) + :is-library (:is-shared file) + :num-tokens n-tokens})) + +(defn- emit-workspace-file-stats + [file-id team-id] + (ptk/reify ::emit-workspace-file-stats + ptk/WatchEvent + (watch [_ state _] + (let [stats (compute-file-stats state file-id)] + (rx/of (ev/event (assoc stats + ::ev/name "open-workspace-file" + ::ev/origin "workspace" + :file-id file-id + :team-id team-id))))))) + (defn- bundle-fetched [{:keys [file file-id thumbnails] :as bundle}] (ptk/reify ::bundle-fetched @@ -421,6 +476,12 @@ (rx/take 1) (rx/map dwc/set-workspace-visited)) + ;; Emit audit event with file statistics once all libraries are resolved + (->> stream + (rx/filter (ptk/type? ::all-libraries-resolved)) + (rx/take 1) + (rx/map #(emit-workspace-file-stats file-id team-id))) + (when-let [component-id (some-> rparams :component-id uuid/parse)] (->> stream (rx/filter (ptk/type? ::workspace-initialized)) diff --git a/frontend/test/frontend_tests/data/workspace_stats_test.cljs b/frontend/test/frontend_tests/data/workspace_stats_test.cljs new file mode 100644 index 0000000000..c4ac483e5f --- /dev/null +++ b/frontend/test/frontend_tests/data/workspace_stats_test.cljs @@ -0,0 +1,87 @@ +;; This Source Code Form is subject to the terms of the Mozilla Public +;; License, v. 2.0. If a copy of the MPL was not distributed with this +;; file, You can obtain one at http://mozilla.org/MPL/2.0/. +;; +;; Copyright (c) KALEIDOS INC Sucursal en España SL + +(ns frontend-tests.data.workspace-stats-test + (:require + [app.common.test-helpers.files :as cthf] + [app.common.test-helpers.ids-map :as cthi] + [app.common.types.tokens-lib :as ctob] + [app.main.data.workspace :as dw] + [cljs.test :as t :include-macros true] + [frontend-tests.helpers.state :as ths])) + +(t/use-fixtures :each + {:before cthi/reset-idmap!}) + +;; --------------------------------------------------------------------------- +;; Test compute-file-stats with various edge cases +;; --------------------------------------------------------------------------- + +(t/deftest compute-file-stats-empty-file + (t/testing "empty file with no pages" + (let [file (cthf/sample-file :file1 :page-label :page1) + store (ths/setup-store file) + state @store + file-id (:id file) + stats (dw/compute-file-stats state file-id)] + (t/is (= (:num-pages stats) 1)) + (t/is (>= (:num-shapes stats) 0)) + (t/is (>= (:avg-shapes-per-page stats) 0)) + (t/is (>= (:max-shapes-per-page stats) 0)) + (t/is (>= (:num-components stats) 0)) + (t/is (>= (:num-linked-libraries stats) 0)) + (t/is (boolean? (:is-library stats))) + (t/is (>= (:num-tokens stats) 0))))) + +(t/deftest compute-file-stats-with-shapes + (t/testing "file with shapes" + (let [file (-> (cthf/sample-file :file1 :page-label :page1) + (cthf/add-sample-shape :shape1) + (cthf/add-sample-shape :shape2)) + store (ths/setup-store file) + state @store + file-id (:id file) + stats (dw/compute-file-stats state file-id)] + (t/is (= (:num-pages stats) 1)) + (t/is (>= (:num-shapes stats) 2)) + (t/is (>= (:avg-shapes-per-page stats) 2)) + (t/is (>= (:max-shapes-per-page stats) 2))))) + +(t/deftest compute-file-stats-no-tokens + (t/testing "file with no tokens lib" + (let [file (cthf/sample-file :file1 :page-label :page1) + store (ths/setup-store file) + state @store + file-id (:id file) + stats (dw/compute-file-stats state file-id)] + (t/is (= (:num-tokens stats) 0))))) + +(t/deftest compute-file-stats-with-tokens + (t/testing "file with tokens" + (let [tokens-lib (-> (ctob/make-tokens-lib) + (ctob/add-set {:name "global" + :description "Global tokens" + :tokens [{:name "color.primary" + :type :color + :value "#000000"}]})) + file (-> (cthf/sample-file :file1 :page-label :page1) + (assoc-in [:data :tokens-lib] tokens-lib)) + store (ths/setup-store file) + state @store + file-id (:id file) + stats (dw/compute-file-stats state file-id)] + (t/is (= (:num-tokens stats) 1))))) + +(t/deftest compute-file-stats-multiple-pages + (t/testing "file with multiple pages" + (let [file (-> (cthf/sample-file :file1 :page-label :page1) + (cthf/add-sample-page :page2) + (cthf/add-sample-page :page3)) + store (ths/setup-store file) + state @store + file-id (:id file) + stats (dw/compute-file-stats state file-id)] + (t/is (= (:num-pages stats) 3)))))