17 KiB
phase, reviewed, depth, files_reviewed, files_reviewed_list, findings, status
| phase | reviewed | depth | files_reviewed | files_reviewed_list | findings | status | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| 12.2-admin-form-fields-date-file-upload-relation-editing-with-def | 2026-10-02T19:12:38Z | standard | 137 |
|
|
issues_found |
Phase 12.2: Code Review Report
Reviewed: 2026-10-02T19:12:38Z Depth: standard Files Reviewed: 137 Status: issues_found
Summary
The phase adds substantial form, deferred-binding, file, date, and relation functionality, but it is not ready to ship. Three blocking defects can lose or silently misapply upload state or permit memory-exhaustion requests, and four additional correctness defects affect upload limits, datetime validation, file ordering, and pending pivot data.
The admin suite passed (60 files, 768 tests), vue-tsc --noEmit passed, and both phase gate scripts passed bash -n. Targeted Go tests were attempted: modules/pact and internal/tools/swagger2openapi passed, while other packages could not complete in this environment because Docker/listener access was denied and the linker reported a disk-quota error. Those infrastructure failures are not counted as findings.
Narrative Findings (AI reviewer)
Critical Issues
CR-01: Form saves can commit before an in-flight upload reaches the deferred session
Classification: BLOCKER
Files:
/media/nvme/dev/golem15/summercms.io/summercms/summercms.go/admin/src/components/form/fields/FileuploadField.vue:375-424/media/nvme/dev/golem15/summercms.io/summercms/summercms.go/admin/src/components/form/formContext.ts:31-49/media/nvme/dev/golem15/summercms.io/summercms/summercms.go/admin/src/views/FormView.vue:205-214/media/nvme/dev/golem15/summercms.io/summercms/summercms.go/admin/src/views/FormView.vue:238-297/media/nvme/dev/golem15/summercms.io/summercms/summercms.go/admin/src/components/relation/RelationChildModal.vue:167-193/media/nvme/dev/golem15/summercms.io/summercms/summercms.go/admin/src/components/relation/RelationChildModal.vue:321-359
Issue: Upload activity is local to FileuploadField; the shared FormSession exposes dirty state but no active-upload state. Both the record form and relation-child form therefore allow submit while uploadWithProgress is still running. The save commits only bindings already present at that instant, clears pendingChanges, and can navigate away. An upload completing just afterwards creates a deferred binding that was not part of the successful save. On create/save-and-close this appears as a successful save with the selected file missing and leaves an invisible pending attachment until purge; on update it silently requires another save that the user is never told to perform.
Fix: Extend FormSession with upload registration (for example beginUpload(): () => void plus a readonly active counter). Register before starting the XHR and release in finally. Disable or await both record and child submission while the counter is non-zero, and prevent navigation until all uploads have reached a known terminal state. Add a test whose upload promise is held pending while Save is clicked.
CR-02: Aborted or network-failed uploads have no idempotency or reconciliation path
Classification: BLOCKER
Files:
/media/nvme/dev/golem15/summercms.io/summercms/summercms.go/admin/src/api/files.ts:128-193/media/nvme/dev/golem15/summercms.io/summercms/summercms.go/admin/src/components/form/fields/FileuploadField.vue:387-434/media/nvme/dev/golem15/summercms.io/summercms/summercms.go/modules/cabana/field_file.go:619-706
Issue: Once an upload request has reached the server, an XHR abort or lost response does not prove the transaction failed. The server may already have stored the blob, file row, and deferred bind, but the client discards the item on abort or marks it retryable on a network error. A later form save can then attach a file the user believes was cancelled, while Retry posts a second independent upload (and can attach both on an attachMany field). There is no client request identifier, unique server constraint, or list reconciliation to distinguish “not stored” from “stored but response lost.” This is a data-integrity and cancellation-semantics failure.
Fix: Generate a stable client upload ID per queued item, send it on every retry, persist it with a uniqueness constraint scoped to admin/session/field, and return the already-created item for duplicate requests. On abort or an ambiguous network failure, reconcile the field list by that ID and explicitly delete a server-side result before treating cancellation as complete. Cover response-loss-after-commit and abort-after-commit cases.
CR-03: Core CRUD, settings, and relation mutation JSON bodies are uncapped
Classification: BLOCKER
Files:
/media/nvme/dev/golem15/summercms.io/summercms/summercms.go/modules/cabana/http.go:51-53/media/nvme/dev/golem15/summercms.io/summercms/summercms.go/modules/cabana/http.go:467-480/media/nvme/dev/golem15/summercms.io/summercms/summercms.go/modules/cabana/http.go:585-594/media/nvme/dev/golem15/summercms.io/summercms/summercms.go/modules/cabana/http.go:751-805/media/nvme/dev/golem15/summercms.io/summercms/summercms.go/modules/cabana/crud.go:648-658/media/nvme/dev/golem15/summercms.io/summercms/summercms.go/modules/cabana/http.go:613-625
Issue: The service explicitly records that Surf does not apply body limits to raw routes, yet decodeObject and decodeRelationMutation decode r.Body directly. Settings update, record create/update, and relation link/unlink can therefore stream arbitrarily large JSON into encoding/json, allowing any authenticated, CSRF-valid admin request (or a compromised admin session) to consume unbounded memory. The new child and file JSON routes already demonstrate the required capped pattern with http.MaxBytesReader, so the protection is inconsistent across the same API.
Fix: Replace these helpers with service methods that accept http.ResponseWriter, wrap r.Body in http.MaxBytesReader(w, r.Body, s.jsonCap()), reject trailing JSON consistently, and map *http.MaxBytesError to 413. Prefer a single raw-route JSON decoder so every current and future handler inherits the cap. Add oversized-body tests for settings, create/update, and link/unlink.
Warnings
WR-01: A field may advertise a maximum file size that its request cap cannot carry
Classification: WARNING
File: /media/nvme/dev/golem15/summercms.io/summercms/summercms.go/modules/cabana/field_file.go:367-381,709-724
Issue: Boot validation accepts maxFilesize == http.body_limits.upload_bytes, but uploadCap caps the complete multipart request at upload_bytes. Multipart framing consumes part of that budget, so a file at (or near) the documented field maximum is rejected with 413 before the file-size validator can accept it. The same problem occurs whenever the gap between the two configured limits is smaller than the multipart overhead. Configuration that passes boot therefore cannot deliver the promised maximum.
Fix: Define the two limits unambiguously. If upload_bytes is a file-content limit, cap the wire body at upload_bytes + multipartOverhead and keep the exact file limit in attach.Store. If it is a request-body limit, make boot reject maxBytes + multipartOverhead > uploadBytes and document the required headroom. Test equality and a gap smaller than 64 KiB.
WR-02: Datetime picker bounds use local days while the server validates UTC days
Classification: WARNING
Files:
/media/nvme/dev/golem15/summercms.io/summercms/summercms.go/admin/src/app/dateFormat.ts:272-290/media/nvme/dev/golem15/summercms.io/summercms/summercms.go/admin/src/components/form/fields/DatepickerField.vue:66-85/media/nvme/dev/golem15/summercms.io/summercms/summercms.go/modules/cabana/field_date.go:270-345
Issue: For a normal datetime, the SPA constructs min/max at midnight/end-of-day in the browser's local time zone and compares the displayed local value to those bounds. The server deliberately compares the saved instant's UTC calendar date. In a positive-offset zone, for example, 2000-01-01 00:30 +01:00 is enabled and considered in range by the picker for minDate: 2000-01-01, but it emits 1999-12-31T23:30:00Z and the server rejects it. Around both boundaries the UI can allow values the server refuses and disable values the server accepts.
Fix: Make the SPA comparison use the same calendar basis as the server: convert non-ignoreTimezone min/max to UTC-day instant bounds before comparing, or change the server contract to validate the displayed local day and pass a trusted zone explicitly. Add tests in positive and negative offsets around midnight for both min and max.
WR-03: Concurrent reorder requests can overwrite the latest order or create a mixed order
Classification: WARNING
Files:
/media/nvme/dev/golem15/summercms.io/summercms/summercms.go/admin/src/components/form/fields/FileuploadField.vue:481-524/media/nvme/dev/golem15/summercms.io/summercms/summercms.go/modules/cabana/field_file.go:1247-1289
Issue: Debouncing limits requests within one burst but sendOrder does not serialize requests. It clears orderBefore before awaiting, so another drag can dispatch a second reorder while the first is in flight. Responses may complete out of order and leave the database in the older order. The server also reads without locking the whole visible set and updates each row separately, so concurrent transactions can interleave row updates into an order that matches neither request. A failure from an older request can additionally restore an obsolete client order over a newer successful move.
Fix: Keep one reorder request in flight and retain only the latest pending ID snapshot; after completion, send that latest snapshot. On the server, lock all visible file rows before deriving and updating sort positions (or use a relation-scoped version/CAS). Ignore stale responses by generation and add a test with deliberately reversed promise completion.
WR-04: Pending pivot hydration discards type-conversion errors
Classification: WARNING
File: /media/nvme/dev/golem15/summercms.io/summercms/summercms.go/modules/cabana/relation_child.go:442-473,553-568
Issue: Both pending-pivot show and update explicitly discard lagoon.Fill errors while rebuilding the pivot model from its deferred envelope. If a deferred row is stale after a schema/type change or otherwise contains a value that no longer fits the pivot column, the API returns a successful response with that field silently replaced by its zero value. Update may then present a response inconsistent with the envelope that will later fail during parent commit. This hides corrupted/incompatible pending state and makes it difficult for the user to recover safely.
Fix: Check the lagoon.Fill result in both paths and return lifecycleFailure(cc, err) (or a field-level 422 if recovery through editing is intended). Add a test with an out-of-range or wrong-type value in a deferred pivot envelope.
Reviewed: 2026-10-02T19:12:38Z Reviewer: the agent (gsd-code-reviewer) Depth: standard