docs(14): add code review fix report
This commit is contained in:
Jakub Zych
@@ -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): <id> 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 = <this job>)`. 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_
|
||||
Reference in New Issue
Block a user