From 5d3299b26f4fcbe101c36d55bf1800b7ee5bf2d1 Mon Sep 17 00:00:00 2001 From: Jakub Zych Date: Sun, 4 Oct 2026 08:13:12 +0200 Subject: [PATCH] docs(14): add code review fix report --- .../14-REVIEW-FIX.md | 113 ++++++++++++++++++ 1 file changed, 113 insertions(+) create mode 100644 .planning/phases/14-domain-jobs-and-external-integrations/14-REVIEW-FIX.md diff --git a/.planning/phases/14-domain-jobs-and-external-integrations/14-REVIEW-FIX.md b/.planning/phases/14-domain-jobs-and-external-integrations/14-REVIEW-FIX.md new file mode 100644 index 0000000..d84d65f --- /dev/null +++ b/.planning/phases/14-domain-jobs-and-external-integrations/14-REVIEW-FIX.md @@ -0,0 +1,113 @@ +--- +phase: 14-domain-jobs-and-external-integrations +fixed_at: 2026-10-04T06:20:00Z +review_path: .planning/phases/14-domain-jobs-and-external-integrations/14-REVIEW.md +iteration: 1 +findings_in_scope: 7 +fixed: 7 +skipped: 0 +status: all_fixed +--- + +# Phase 14: Code Review Fix Report + +**Fixed at:** 2026-10-04T06:20:00Z +**Source review:** .planning/phases/14-domain-jobs-and-external-integrations/14-REVIEW.md +**Iteration:** 1 + +**Summary:** +- Findings in scope: 7 (CR-01, WR-01 to WR-06; the Info findings were out of scope) +- Fixed: 7 +- Skipped: 0 + +All fixes landed in `fonoteka.go` and its plugin submodules `sm-feedback-plugin` and `sm-golem-plugin`. No change was needed in `summercms.go`. Each submodule fix is committed in the submodule first, then the pointer is bumped in `fonoteka.go` with its own `fix(14): bump …` commit. + +## Verification + +- Where it ran: the main checkout, because `workflow.use_worktrees` is `false`. The numbers can be reproduced from the current tree. +- After every fix: `go vet` and `go test` on the affected module, with the testcontainers Postgres suites running under Docker. +- After the last fix: `go vet` and `go test` over the whole `fonoteka.go` workspace (root, parity, fonoteka, golem, feedback and user plugins). Everything was green. +- Environment note: in this sandbox `/tmp` hit a disk quota. The linker and the CSV multipart uploads failed with "disk quota exceeded" or a 500 until `GOTMPDIR` and `TMPDIR` pointed at the nvme volume. This affects the environment only, not the code. `TestCsvStoreAndShow` failed the same way on the unmodified tree. +- Each new regression test was checked to fail on the pre-fix code, where practical (CR-01, WR-01, WR-05). + +## Fixed Issues + +### CR-01: Album writes after slow Discogs I/O save every column from a stale struct + +**Status:** fixed: requires human verification (write-shape and logic change) +**Files modified:** `fonoteka.go/plugins/golem15/fonoteka/classes/album_write_service.go`, `fonoteka.go/plugins/golem15/fonoteka/classes/discogs/applicator.go`, `fonoteka.go/plugins/golem15/fonoteka/classes/discogs/cover_fetcher.go`, `fonoteka.go/plugins/golem15/fonoteka/classes/discogs/postgres_test.go` +**Commit:** d64d3ab (fonoteka.go) +**Applied fix:** +- Added `classes.SaveAlbumColumns(ctx, db, a, cols...)`. It runs the Album model rules, then `Model(a).Select(cols…).Updates(a)`, so GORM hooks and lighthouse broadcasts still fire. It also adds the columns `Album.BeforeSave` derives from the given ones (`tracklist` → `track_titles`; `market_price_stored` → currency, source and checked_at; `market_price_currency` → source) and always adds `updated_at`. +- The applicator now records each field it actually sets and writes only those. +- `persistMarketPrice` writes only the market-price columns. +- `attachCover` writes only `cover_import_failures`. +- A new subtest edits name, notes and shelf after the album is loaded and checks that the apply keeps them. +- One behaviour to check: an apply that changes no catalog field still issues an `updated_at`-only UPDATE and broadcast. The old full `Save` did the same. Eloquent would skip the write. + +### WR-01: A stale CSV match pass can cancel, advance or fork a remapped import + +**Status:** fixed: requires human verification (concurrency logic) +**Files modified:** `fonoteka.go/plugins/golem15/fonoteka/csv_match_job.go`, `fonoteka.go/plugins/golem15/fonoteka/phase14_jobs_test.go` +**Commit:** 41fffae (fonoteka.go) +**Applied fix:** +- The cancel branch, the final `preview` write and `fail()` now require `status = matching AND (match_job_id IS NULL OR match_job_id = )`. The cancel branch used to be unconditional. +- `pause()` re-dispatches only when the locked row still names this job (or none). Otherwise it ends through `lostImport`. +- NULL `match_job_id` is tolerated, the same way the existing start update tolerates it, because a remap always sets a non-NULL id. +- A new subtest covers each of the four branches (cancel, final, fail, pause) against a mid-pass remap to a newer job. All four failed before the fix. + +### WR-02: The anonymous feedback upload keeps an attacker-chosen file extension in public storage + +**Files modified:** `fonoteka.go/plugins/golem15/feedback/classes/submission_store.go`, `fonoteka.go/plugins/golem15/feedback/feedback_test.go` +**Commit:** 5a8b7f8 (sm-feedback-plugin), pointer bump 19add38 (fonoteka.go) +**Applied fix:** +- The disk-name extension now comes from the sniffed content type. The client's extension is kept only when it names that type (`jpg`/`jpeg`, `png`, `gif`, `webp`), so ordinary uploads keep their Winter-style names. Anything else, such as `x.html`, `x.svg` or no extension, gets the type's own extension. An unknown type is refused. +- `file_name` is reduced to the client's base name, the same cleaning `attach.Store` does. +- A new subtest checks `x.html`, `../../x.svg`, `noext` and `Shot.PNG`. + +### WR-03: Credential-test routes are unthrottled oracles for third-party keys + +**Files modified:** `fonoteka.go/plugins/golem15/fonoteka/routes.go`, `fonoteka.go/plugins/golem15/fonoteka/routes_table_phase14_test.go`, `fonoteka.go/plugins/golem15/fonoteka/ai_routes_test.go` +**Commit:** e3c8599 (fonoteka.go) +**Applied fix:** +- `throttle:10,1` added to `POST ai-credential/test` and `POST discogs-credential/test`, matching the other sensitive JWT routes. Responses below the limit are unchanged, and the parity suite stays green. +- The route table records the deviation from routes.php. +- `TestAICredentialTestRoute` made 13 calls as one user, so its stored-credential half now runs as a second user. + +### WR-04: The "write-only" admin AI key can be redirected to any host by changing base_url + +**Files modified:** `fonoteka.go/plugins/golem15/golem/controllers/models_admin_controller.go`, `fonoteka.go/plugins/golem15/golem/golem_test.go`, `fonoteka.go/plugins/golem15/golem/README.md` +**Commit:** 93b2b05 (sm-golem-plugin), pointer bump 80cafb2 (fonoteka.go) +**Applied fix:** +- `FormBeforeUpdate` now loads the stored `adapter` and `base_url` along with the key. When the key is sent empty or redacted and either value differs, it returns a validation error on `api_key` ("Re-enter the API key when changing the adapter or base URL."). Base URLs are compared with nil and blank treated as equal. +- The README describes the new rule. +- Not done: the review's secondary suggestion to narrow `TrustedMode` for admin models. That is a design decision (D-05 accepts http and local model servers), so it is left for a decision note. + +### WR-05: `golem:import-settings` silently drops models from a repeater saved with non-sequential keys + +**Files modified:** `fonoteka.go/plugins/golem15/golem/console/import_settings.go`, `fonoteka.go/plugins/golem15/golem/golem_admin_test.go` +**Commit:** 9f60b61 (sm-golem-plugin), pointer bump 8d60670 (fonoteka.go) +**Applied fix:** +- An object-shaped repeater now reads every key. Integer keys come first in ascending order, then any other keys in byte order. +- A new case imports `{"10":…,"0":…,"2":…}` and checks that all three rows arrive in the order Zero, Two, Ten. The old code imported only 2. + +### WR-06: The G15Office sync job is not idempotent, and a retry files duplicate tasks + +**Files modified:** `fonoteka.go/plugins/golem15/feedback/classes/sync_g15office.go`, `fonoteka.go/plugins/golem15/feedback/jobs.go`, `fonoteka.go/plugins/golem15/feedback/classes/classes_test.go`, `fonoteka.go/plugins/golem15/feedback/README.md` +**Commit:** e52b1ec (sm-feedback-plugin), pointer bump 98fb4a8 (fonoteka.go) +**Applied fix:** +- `SyncG15Office` returns early when a submission is already `sent` with a task id. +- The task id is stored as soon as `CreateTask` returns it. A retry with a stored id only re-attempts the attachment. +- `jobs.go` logs a `CompleteJob` failure (job and submission id only) instead of returning it. +- The `attach-failure` test now checks: + - the task id survives the failed attempt; + - the retry attaches to the same task; + - a third run on the sent submission sends nothing. + - In total: 1 task request and 2 attachment requests. +- Behaviour change: a submission that is `failed` because its attachment failed now carries its `g15office_task_id`. The task does exist on G15Office. + +--- + +_Fixed: 2026-10-04T06:20:00Z_ +_Fixer: Claude (gsd-code-fixer)_ +_Iteration: 1_