summercms/.planning/phases/05-data-layer-full-fidelity/05-REVIEW-FIX.md

phase, fixed_at, review_path, iteration, findings_in_scope, fixed, skipped, status

phase	fixed_at	review_path	iteration	findings_in_scope	fixed	skipped	status
05-data-layer-full-fidelity	2026-09-18T22:05:00Z	.planning/phases/05-data-layer-full-fidelity/05-REVIEW.md	1	10	9	1	partial

Phase 5: Code Review Fix Report

Fixed at: 2026-09-18T22:05:00Z Source review: .planning/phases/05-data-layer-full-fidelity/05-REVIEW.md Iteration: 1

Summary:

• Findings in scope: 10 (3 critical, 7 warning; 3 info findings out of scope)
• Fixed: 9
• Skipped: 1 (WR-05, reviewer premise disproved by test)

Commits land in two repositories. summercms.go is the framework repo; fonoteka.go is the sibling application repo. Commit hashes below are prefixed with the repo they belong to.

Verification at the final commit of each repo, with Docker/testcontainers available (full suite, not -short):

• summercms.go: go vet ./... clean, go test ./... all packages ok.
• fonoteka.go: go vet and go test over ./..., ./plugins/golem15/user/..., ./plugins/golem15/fonoteka/... all ok, including parity.

The fonoteka tests were run against the framework worktree carrying the framework fixes (the ../summercms.go replace), so fonoteka.go master depends on summercms.go master containing 3943ea3 and 6d2262d.

Fixed Issues

CR-01: Integer validation rejects *int and JSON numbers, so SaveAlbum year checks fail

Files modified: lagoon/validate.go, lagoon/validate_test.go (summercms.go); plugins/golem15/fonoteka/classes/album_write_service.go, plugins/golem15/fonoteka/classes/album_write_service_db_test.go (fonoteka.go) Commit: summercms.go 3943ea3, fonoteka.go 1c4a09d Status: fixed: requires human verification (logic change) Applied fix: isIntegerValue now dereferences pointers/interfaces and accepts whole float32/float64 values (rejecting fractions and infinities). albumRuleValues no longer overlays raw request values on top of the filled model; it validates the model fields only, as market_price_stored already did. New tests: TestValidateIntegerPointerAndJSONNumber (framework), TestSaveAlbumYearSurvivesNameOnlyUpdate and TestSaveAlbumJSONBodyYear (create with year 1991, name-only update keeps 1991; a json.Unmarshaled body with year passes; year 1700 still yields a year field error). Note: digit strings under integer|between: still hit validator's string-length semantics for min/max. That is pre-existing, not part of this finding, and unreachable through SaveAlbum now that the filled *int is validated.

CR-02: stampMarketPrice has no dirty check

Files modified: plugins/golem15/fonoteka/models/album.go, plugins/golem15/fonoteka/classes/album_write_service.go (comment), plugins/golem15/fonoteka/classes/album_write_service_db_test.go (fonoteka.go) Commit: fonoteka.go f092170 Status: fixed: requires human verification (logic change, PHP parity) Applied fix: Instead of request-key flags (which only cover SaveAlbum), Album now keeps an unexported snapshot of market_price_stored / market_price_currency / market_price_source, taken in AfterFind and re-based in AfterSave (the Go stand-in for Eloquent getRawOriginal/syncOriginal). stampMarketPrice was rewritten line by line from the PHP original:

• price not dirty: return; if only currency is dirty, run the new normalizeCurrencyOnlyChange (normalise, or null currency+source when no amount is stored) without touching checked_at (D-6);
• price dirty and blank/non-numeric: null all four columns;
• price dirty: clear source unless this save set it (KeepMarketPriceSource or a changed value, D-4), normalise currency, and refresh checked_at only when the 4-decimal amount differs from the original (re-sending 12 against 12.0000 is not a fresh check).

The unexported field is invisible to GORM and encoding/json, so no response shape changes. Tests added: name-only save keeps price/currency/source/frozen checked_at; currency-only edit keeps checked_at; changed amount clears source and moves checked_at; same amount in another spelling keeps checked_at; a struct reused across two saves without reload starts the second save clean. Known limits to confirm: an Album built by hand with an ID and saved without ever being loaded has an empty snapshot, so a non-blank price counts as dirty (previous behaviour). If a transaction rolls back after AfterSave, the in-memory snapshot is ahead of the database until the struct is reloaded.

CR-03: User.Password is neither Hidden() nor json:"-"

Files modified: plugins/golem15/user/models/user.go, plugins/golem15/fonoteka/classes/hidden_marshal_test.go (fonoteka.go) Commit: fonoteka.go 2a94e4d Applied fix: Password is tagged json:"-" and User implements Hidden() []string{"password"}. This also closes a leak through Collection.Editors []User. TestHiddenNeverMarshals now runs a name-based backstop on every registered model: any gorm column named password, token_hash or *_hash must be listed in Hidden(). Added TestSecretColumnNames and TestUserPasswordNeverMarshals (direct marshal and marshal via Collection.Editors).

WR-01: Write services hardcode production=false

Files modified: plugins/golem15/fonoteka/classes/album_write_service.go, plugins/golem15/fonoteka/classes/collection_write_service.go, plugins/golem15/fonoteka/plugin.go, plugins/golem15/fonoteka/classes/album_write_service_test.go (fonoteka.go) Commit: fonoteka.go 68fe1a4 Applied fix: The write services default to silent. A package-level atomic.Bool behind classes.SetDebug gates dropped-key logging; Plugin.Boot sets it from app.Config.Bool("app.debug") (nil-safe). SaveAlbum/SaveCollection signatures are unchanged. Test TestWriteServicesSilentUnlessDebug.

WR-02: Fill of Encrypted columns Scans ciphertext, not plaintext

Files modified: lagoon/fill.go, lagoon/fill_test.go (summercms.go) Commit: summercms.go 6d2262d Applied fix: convertValue handles Encrypted destinations (value and pointer fields) by wrapping a request string with NewEncrypted; non-strings are rejected with encrypted value must be a string, and setField no longer falls through to Encrypted.Scan for that type. nil still clears. TestFillEncryptedTakesPlaintext proves plaintext round-trips, another row's ciphertext is stored as literal text rather than decrypted, non-string input is rejected, and nil clears. The Fillable() half of the reviewer's suggestion is handled under WR-07 (service allow-list), leaving model Fillable() as the PHP $fillable mirror.

WR-03: CollectionInvitation.token_hash is not hidden

Files modified: plugins/golem15/fonoteka/models/collection_invitation.go (fonoteka.go) Commit: fonoteka.go 7dc65a0 Applied fix: TokenHash tagged json:"-" and Hidden() returns token_hash. Committed before CR-03 so the widened hidden-marshal test is green at every commit. Deliberately not applied: token_hash was left in Fillable(). PHP $fillable lists it, and the sibling hash models (ApiToken, OAuthRefreshToken) keep token_hash in their Fillable() as line-by-line $fillable ports. Removing it from this one model would break that convention and could break a straight port of InvitationService::create([...'token_hash' => ...]) in Phase 6. The service-level allow-list is the right place to exclude it when that service is written.

WR-04: Thumb interpolates unsanitized mode/ext into fileblob keys

Files modified: lagoon/attach/thumb.go, lagoon/attach/thumb_test.go (summercms.go) Commit: summercms.go ced87a5 Applied fix: Added thumbToken (^[a-z0-9]+$). File.Thumb lowercases the mode (Winter does the same, and defaultResizeImage already compared case-insensitively) and returns an error for a mode or extension outside the alphabet, before any bucket access. ThumbFilename keeps its signature and coerces an unsafe mode to auto and an unsafe ext to jpg, so it can never emit a separator or ... Tests TestThumbFilenameRejectsUnsafeTokens and TestFileThumbRejectsTraversalMode (also asserts nothing is written to the bucket).

WR-06: SaveCollection validates the raw request, not the filled model

Files modified: plugins/golem15/fonoteka/classes/collection_write_service.go, plugins/golem15/fonoteka/classes/collection_write_service_db_test.go (fonoteka.go) Commit: fonoteka.go a02b4e0 Status: fixed: requires human verification (logic change) Applied fix: Validation values are built from the filled collection (name, description). Tests: a description-only update of a named collection succeeds and keeps the name; a blank name and a create without a name still return a name field error.

WR-07: Credential Fillable() includes owner FKs; the only narrowing list is a test helper

Files modified: plugins/golem15/fonoteka/classes/credential_write_service.go (new), plugins/golem15/fonoteka/classes/credential_write_service_test.go (new), plugins/golem15/fonoteka/classes/credential_write_service_fuzz_test.go (fonoteka.go) Commit: fonoteka.go df84c21 Applied fix: New production allow-list classes.CredentialFillFields = {provider, model, base_url} with a doc comment stating that the owner FK comes from the auth context and secrets are set with lagoon.NewEncrypted. The fuzz helper now fills through that list; the test-only credentialWriteFields and the pre-Fill key deletion are gone, so the narrowing the four fuzz targets prove is the one handlers will use. TestCredentialFillFieldsNarrowerThanFillable pins the exclusions. Model Fillable() is unchanged (PHP $fillable mirror, backstop layer). New files were required by the finding. A full credential write service (owner from auth context) belongs with the Phase 6 handlers and was not invented here.

Skipped Issues

WR-05: DeleteKeys prefix thumb_<id>_ collides with later IDs in the same partition

File: lagoon/attach/file.go:60-65 Reason: skipped: false positive, no code change made. The list prefix is thumb_<id>_ including the trailing underscore. thumb_4_ is not a string prefix of thumb_40_… or thumb_42_… (the character after thumb_4 is a digit there, not _), so the collision described cannot occur; the finding's own parenthetical contradicts itself. This was verified rather than argued: TestDeleteKeysThumbPrefixIsIDDelimited (summercms.go c848692, a test-only commit, not counted as a fix) puts originals and thumbs for IDs 4, 14, 40, 41 and 42 in one partition, deletes file 4's keys, and asserts only file 4's original and thumbs are gone. It passes against the unchanged code. The suggested remainder regex (^\d+_\d+_…) was not adopted because it would add risk without benefit: any thumb name the regex fails to anticipate (for example a negative offset, which was not checked against Winter) would be orphaned on force-delete. Original issue: Force-delete lists partition + "thumb_" + id + "_"; the reviewer believed prefix thumb_4_ also matches thumb_40_…, thumb_41_… when two originals share a partition.

---

Fixed: 2026-09-18T22:05:00Z Fixer: Claude (gsd-code-fixer) Iteration: 1