Closes#83 (the seed-lots half; /gardens/:id/years and /capabilities still to do — see below).
/seed-lots was the only handler file without a sibling API test. That's the exact state PATCH/DELETE /journal/:id shipped in: implemented, unit-tested through the service, and completely unreachable, because service tests cannot see a route that was never registered or one registered with the wrong :param.
What's covered
Four tests, chosen around what a broken route would silently take with it:
TestSeedLotCrudAPI — full lifecycle over HTTP: create, GET by id, list, ?plantId= filter, a rejected non-numeric plantId, PATCH with version bump, a stale-version 409 that carries the current row back so the client can rebase without a second request, DELETE, and 404 afterwards.
TestSeedLotRemainingIsDerivedAPI — buys 50, plants 12 against the lot, expects remaining: 38 through the HTTP surface. DESIGN.md makes derivation load-bearing ("a decremented column drifts the moment a planting is edited behind its back") and this number is the whole reason anyone opens the seed shelf, so it's worth pinning at the boundary rather than only in the service.
TestSeedLotsArePrivateAPI — lots are private to the buyer and deliberately never travel with a shared garden. Another user gets 404, not 403, on get/patch/delete, per the project's convention that no-access masks existence. Also asserts they don't leak into the other user's listing, and that the owner still has access afterwards (so the test can't pass by breaking access for everyone).
TestSeedLotsRequireAuthAPI — all five routes 401 unauthenticated rather than returning an empty list.
Verified the tests catch the thing
Unregistering GET /seed-lots/:id — the journal failure mode — fails all four:
--- FAIL: TestSeedLotCrudAPI
get: status 404, body 404 page not found
--- FAIL: TestSeedLotRemainingIsDerivedAPI
get lot: status 404, body 404 page not found
--- FAIL: TestSeedLotsArePrivateAPI
alice lost access to her own lot: 404
--- FAIL: TestSeedLotsRequireAuthAPI
GET /api/v1/seed-lots/1 = 404, want 401
Worth doing given how easily a test like this can pass for the wrong reason — the fixture in #72 is the cautionary case.
Note for reviewers
decodeList is new because seed lot listing returns a bare JSON array, unlike /journal's {"entries": …}. A helper that assumed an object would have decoded nothing and quietly reported zero lots — which several assertions here would then have "passed" on.
Not in this PR
#83 also names GET /gardens/:id/years and GET /capabilities as untested. Leaving those out deliberately: /capabilities is about to become dynamic in #79 and its test should be written against the new behaviour rather than rewritten immediately.
No production code changed. GOWORK=off go test ./... green; gofmt -l internal/ clean.
Closes #83 (the seed-lots half; `/gardens/:id/years` and `/capabilities` still to do — see below).
`/seed-lots` was the only handler file without a sibling API test. That's the exact state PATCH/DELETE `/journal/:id` shipped in: implemented, unit-tested through the service, and completely unreachable, because service tests cannot see a route that was never registered or one registered with the wrong `:param`.
## What's covered
Four tests, chosen around what a broken route would silently take with it:
- **`TestSeedLotCrudAPI`** — full lifecycle over HTTP: create, GET by id, list, `?plantId=` filter, a rejected non-numeric `plantId`, PATCH with version bump, a stale-version 409 that carries the current row back so the client can rebase without a second request, DELETE, and 404 afterwards.
- **`TestSeedLotRemainingIsDerivedAPI`** — buys 50, plants 12 against the lot, expects `remaining: 38` through the HTTP surface. DESIGN.md makes derivation load-bearing ("a decremented column drifts the moment a planting is edited behind its back") and this number is the whole reason anyone opens the seed shelf, so it's worth pinning at the boundary rather than only in the service.
- **`TestSeedLotsArePrivateAPI`** — lots are private to the buyer and deliberately never travel with a shared garden. Another user gets **404, not 403**, on get/patch/delete, per the project's convention that no-access masks existence. Also asserts they don't leak into the other user's listing, and that the owner still has access afterwards (so the test can't pass by breaking access for everyone).
- **`TestSeedLotsRequireAuthAPI`** — all five routes 401 unauthenticated rather than returning an empty list.
## Verified the tests catch the thing
Unregistering `GET /seed-lots/:id` — the journal failure mode — fails all four:
```
--- FAIL: TestSeedLotCrudAPI
get: status 404, body 404 page not found
--- FAIL: TestSeedLotRemainingIsDerivedAPI
get lot: status 404, body 404 page not found
--- FAIL: TestSeedLotsArePrivateAPI
alice lost access to her own lot: 404
--- FAIL: TestSeedLotsRequireAuthAPI
GET /api/v1/seed-lots/1 = 404, want 401
```
Worth doing given how easily a test like this can pass for the wrong reason — the fixture in #72 is the cautionary case.
## Note for reviewers
`decodeList` is new because seed lot listing returns a **bare JSON array**, unlike `/journal`'s `{"entries": …}`. A helper that assumed an object would have decoded nothing and quietly reported zero lots — which several assertions here would then have "passed" on.
## Not in this PR
#83 also names `GET /gardens/:id/years` and `GET /capabilities` as untested. Leaving those out deliberately: `/capabilities` is about to become dynamic in #79 and its test should be written against the new behaviour rather than rewritten immediately.
No production code changed. `GOWORK=off go test ./...` green; `gofmt -l internal/` clean.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ
Every other handler file had a sibling API test; this group had none, which is
the state PATCH/DELETE /journal/:id shipped in — implemented, unit-tested
through the service, and completely unreachable. Service tests cannot see a
route that was never registered or one registered with the wrong :param.
Four tests, covering what a broken route would silently take with it:
- Full CRUD over HTTP, including GET/PATCH/DELETE by id, the ?plantId= filter,
a rejected non-numeric plantId, and a stale-version 409 carrying the current
row so the client can rebase.
- `remaining` is derived through the HTTP surface, not just in the service.
DESIGN.md makes derivation load-bearing, and the number is the whole reason
anyone opens the seed shelf.
- Lots are private to the buyer: another user gets 404 (not 403 — existence is
masked per the project convention) on get/patch/delete, sees none in their
listing, and the owner still has access afterwards.
- The group is behind requireAuth: unauthenticated calls 401 rather than
returning an empty list.
Verified the tests actually catch the failure they exist for by unregistering
GET /seed-lots/:id — all four fail with "404 page not found", the journal
signature.
Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ
Live status board. Findings are posted in each model's own comment. Advisory only — does not block merge.
<!-- gadfly-status-board -->
## 🪰 Gadfly — live review status
5/5 reviewers finished · updated 2026-07-22 02:30:23Z
#### `claude-code/sonnet` · claude-code — ✅ done
- ✅ **security** — No material issues found
- ✅ **correctness** — No material issues found
- ✅ **maintainability** — No material issues found
- ✅ **performance** — No material issues found
- ✅ **error-handling** — Minor issues
#### `glm-5.2:cloud` · ollama-cloud — ✅ done
- ⚠️ **security** — could not complete
- ⚠️ **correctness** — could not complete
- ✅ **maintainability** — Minor issues
- ✅ **performance** — No material issues found
- ⚠️ **error-handling** — could not complete
#### `kimi-k2.6:cloud` · ollama-cloud — ✅ done
- ✅ **security** — No material issues found
- ✅ **correctness** — No material issues found
- ✅ **maintainability** — No material issues found
- ⚠️ **performance** — could not complete
- ⚠️ **error-handling** — could not complete
#### `opencode/glm-5.2:cloud` · opencode — ✅ done
- ✅ **security** — No material issues found
- ✅ **correctness** — No material issues found
- ✅ **maintainability** — No material issues found
- ✅ **performance** — No material issues found
- ⚠️ **error-handling** — could not complete
#### `opencode/kimi-k2.6:cloud` · opencode — ✅ done
- ✅ **security** — No material issues found
- ⚠️ **correctness** — could not complete
- ✅ **maintainability** — No material issues found
- ✅ **performance** — No material issues found
- ⚠️ **error-handling** — could not complete
<sub>Live status board. Findings are posted in each model's own comment. Advisory only — does not block merge.</sub>
Good — SourceURL is already validated at the service level (existing test TestSourceURLValidation covers scheme rejection), and this API test PR doesn't touch or weaken that. It's stored, not fetched server-side, so no SSRF surface. This PR is test-only and everything in it checks out against the real handlers.
Review
VERDICT: No material issues found
Findings from a security-lens pass:
Verified internal/api/seed_lots.go — every handler (listSeedLots, getSeedLot, updateSeedLot, deleteSeedLot) scopes to mustActor(c).ID, matching the "no-access is 404" convention TestSeedLotsArePrivateAPI exercises. plantId query param is parsed with strconv.ParseInt and rejected below 1 before use — no injection surface, consistent with the test's nope/0/-1 → 400 assertions.
Verified the route group sits behind requireAuth per TestSeedLotsRequireAuthAPI's expectations by reading the handler file; no handler reads mustActor optimistically or falls back to an unauthenticated path.
Checked SourceURL (present in the create/patch request structs but not exercised by this new API test) — it's validated server-side by validSourceURL in internal/service/seed_lots.go and already covered by TestSourceURLValidation in the service package (rejects javascript: etc.). Not fetched server-side, so no SSRF risk from an untested field here.
registerAndCookie/authEngine test helpers reuse the existing auth test scaffolding (auth_test.go) — cookies, CSRF guard, and password handling are unchanged and were already covered by prior tests; nothing in this diff weakens them.
No secrets, tokens, or credentials are hardcoded beyond the standard password123 test fixture already used throughout the existing test suite.
This PR adds only test code and exercises existing, already-secure production paths correctly. Nothing in my lane to flag.
🎯 Correctness — No material issues found
Route registration confirmed at internal/api/api.go:181 behind requireAuth(), matching the test's assumptions. Everything in this diff checks out against the actual code.
Review
VERDICT: No material issues found
This PR adds only test code (no production changes), and every behavioral claim the tests make matches what I verified by reading the actual handler/service/store code:
internal/api/seed_lots.go:88 — plantId filter rejects id < 1 or unparseable, so "nope", "0", "-1" all correctly expect 400 (verified against the test's bad-value loop).
internal/service/seed_lots.go:98-123 (fillRemaining) — remaining = quantity - used, uncapped, matching the test's expectation of both 38 (50-12) and -7 (50-57) after over-planting. used is driven by the explicit count field when set (internal/service/seed_lots.go:113-115), so the test's literal count: 12 / count: 45 payloads produce exactly the arithmetic asserted — I traced this through store/seed_lots.go's SeedLotUsage SQL as well, which correctly returns raw count/radius/spacing per active planting rather than pre-aggregating.
internal/service/seed_lots.go:129-138 (ownSeedLot) — cross-owner access returns domain.ErrNotFound, so Bob's get/patch/delete against Alice's lot are all 404s, matching TestSeedLotsArePrivateAPI.
internal/store/seed_lots.go:105-129 (UpdateSeedLot) — a version mismatch fetches and returns the current row alongside ErrVersionConflict, and internal/service/seed_lots.go:204-214 fills remaining onto that returned row before it reaches the client — matching the test's assertion that the 409 body's current.vendor reflects the already-applied first patch.
internal/api/api.go:181 — the /seed-lots group is registered under h.requireAuth(), matching TestSeedLotsRequireAuthAPI.
Planting bounds check (internal/service/plantings.go:285) only requires the plop center within the object's half-extents, so the test's (40,40) placement in a 100×100 bed is valid and won't spuriously fail the over-plant step.
Test helpers (createGardenAPI, objectsPath, objectPlantingsPath, decodeMap, doJSON, authEngine, localCfg, registerAndCookie) all exist with the exact signatures the new file calls them with — no compile break.
No logic bugs, no incorrect magic numbers, and the domain-specific assertions (derived-remaining arithmetic, ACL masking) are correct against the real implementation rather than just plausible-looking.
🧹 Code cleanliness & maintainability — No material issues found
All the helpers this new file relies on (doJSON, decodeMap, authEngine, localCfg, registerAndCookie, createGardenAPI, objectsPath, objectPlantingsPath) already exist and are reused correctly, with no duplication introduced. createPlantAPI and decodeList are genuinely new (nothing equivalent exists elsewhere), and the inline garden/bed-object creation in TestSeedLotRemainingIsDerivedAPI matches the same inline pattern plantings_test.go already uses (there's no existing "create bed" helper to have reused instead). Email-namespacing (alice-lots@…) follows the same per-file collision-avoidance convention seen in gardens_test.go/plants_test.go.
VERDICT: No material issues found
Nothing in this diff raises cleanliness or maintainability concerns: the new test file follows the existing internal/api/*_test.go conventions closely (table-driven sub-cases, t.Fatalf/t.Errorf usage, Path helper naming, doc comments explaining why a test exists), the one new shared helper (decodeList) is justified and documented (bare-array vs {"entries":…} response shape), and no code is duplicated that should have called an existing helper instead.
⚡ Performance — No material issues found
VERDICT: No material issues found
This PR adds only test code (internal/api/seed_lots_test.go) — no production paths, hot loops, or query patterns are touched. Through the performance lens specifically:
All requests in each test are small, fixed-count, sequential HTTP round-trips against an in-memory test router (max ~10 requests per test, one for loop over a 3-element slice at internal/api/seed_lots_test.go:94-98, another over 3 method cases at :218-230). No unbounded growth, no loop scaling with external input, no N+1 pattern.
Planting counts used (count: 12, count: 45) are trivial and don't stress any batch-insert path.
No new helper introduces quadratic behavior — decodeList/decodeMap/createPlantAPI are all O(1) or O(response size) JSON decodes.
Nothing here would move the needle on CI time or resource usage in a way worth flagging under a performance lens. Correctness/test-design questions (e.g., whether the remaining derivation assertions are appropriate) belong to other lenses, not this one.
🧯 Error handling & edge cases — Minor issues
Confirmed against the actual code: decodeMap (internal/api/gardens_test.go:26-34) just unmarshals into a map[string]any and only t.Fatalfs on a JSON parse error — it never checks for specific keys. So on a non-200 error body like {"error": "..."}, it returns cleanly with "remaining" absent, the type assertion .(float64) fails, ok is false, and line 193-194 falls through to a generic t.Errorf("remaining after over-planting = %v, want -7 ...") with no status code. Every other GET/decode pair in the file (lines 173-176, 160-162, etc.) guards with if w.Code != http.StatusOK { t.Fatalf(...) } first — this one at line 192 does not. Finding confirmed.
VERDICT: Minor issues
internal/api/seed_lots_test.go:192-195 — The final GET in TestSeedLotRemainingIsDerivedAPI decodes the response body without first checking w.Code == http.StatusOK, unlike every other GET/decode pair in this file (e.g. lines 173-176, 160-162). If this request fails (e.g. a 404/500 due to a regression), decodeMap (internal/api/gardens_test.go:26-34) will still unmarshal successfully since it only fails on invalid JSON, not missing keys — ["remaining"] will be absent, the assertion falls through to !ok, and the failure reports "remaining after over-planting = <nil>, want -7" with no status code or error body surfaced. That masks the actual failure (an HTTP error) behind a generic "wrong value" message. Suggested fix: add the same w.Code != http.StatusOK guard used at line 174 before decoding.
⚠️ This reviewer failed to complete: agent: step 6: all chain targets failed
ollama-cloud/glm-5.2☁️ ollama-cloud/glm-5.2☁️ HTTP 429: too many concurrent requests
🎯 Correctness — ⚠️ could not complete
⚠️ This reviewer failed to complete: agent: step 7: all chain targets failed
ollama-cloud/glm-5.2☁️ ollama-cloud/glm-5.2☁️ HTTP 429: too many concurrent requests
🧹 Code cleanliness & maintainability — Minor issues
Both findings verified against the actual code.
Verdict: Minor issues
internal/api/seed_lots_test.go:19 — decodeList diverges from the established list-decoding convention. The package's other list-decoding sites decode into []map[string]any inline: gardens_test.go:56-58, plants_test.go:18, and shares_test.go:41-42. The new decodeList helper returns []any instead. It is only used for len() checks in this file, so it works, but it's a slightly leakier type and breaks the visual pattern a reader of this package expects. Either return []map[string]any to match, or keep []any as a deliberate minor inconsistency.
internal/api/seed_lots_test.go:29 — createPlantAPI lives in the wrong file. A plant-creation helper is plant-domain code, but it's declared in seed_lots_test.go. plants_test.go currently inlines custom-plant creation (plants_test.go:46-47 and 85-86) with a near-identical payload rather than calling a shared helper, so this PR introduces a second, parallel pattern instead of promoting a shared one. Grep confirms no createPlant/createPlantAPI helper exists elsewhere; plants_test.go inlines the POST. Low-churn fix: move createPlantAPI into plants_test.go.
No dead code, no deep nesting, and the four test functions follow the same structure and comment style as TestJournalCrudAPI, the closest sibling.
⚡ Performance — No material issues found
VERDICT: No material issues found
This is a test-only PR (new internal/api/seed_lots_test.go, no production code changed). Through the performance lens:
The new helpers (decodeList, createPlantAPI) do a single request/decode each and are called a handful of times per test — no hot loops, no unbounded growth.
decodeList unmarshals into []any and only checks len(); nothing pathological.
Each test spins up its own in-memory DB via the pre-existing authEngine/localCfg pattern — that's the established convention across the sibling *_test.go files, not a regression this PR introduces.
The exercised handlers (listSeedLots, getSeedLot, etc.) are unchanged; the ?plantId= filter path uses a single scoped query, no N+1.
No performance issues in this PR's scope.
🧯 Error handling & edge cases — ⚠️ could not complete
⚠️ This reviewer failed to complete: agent: step 6: all chain targets failed
ollama-cloud/glm-5.2☁️ ollama-cloud/glm-5.2☁️ HTTP 429: too many concurrent requests
kimi-k2.6:cloud (ollama-cloud) — No material issues found
Verdict: No material issues found · ⚠️ 2/5 lens(es) errored — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — No material issues found
No material issues found
This is a pure test-addition PR with no production code changes. From a security lens, the new tests actually improve coverage of authn/authz behavior rather than weaken anything.
TestSeedLotsRequireAuthAPI (l. 247) correctly verifies that all five routes return 401 when called without a session cookie, ensuring the requireAuth middleware is wired.
TestSeedLotsArePrivateAPI (l. 201) verifies cross-user isolation: Bob gets 404 (not 403) on Alice’s lot, consistent with the project’s “mask existence” convention, and confirms the lot doesn’t leak into Bob’s listing.
TestSeedLotCrudAPI (l. 50) exercises input validation by asserting that malformed plantId filters ("nope", "0", "-1") return 400.
There are no hardcoded secrets, no unsafe deserialization of untrusted data (all inputs are hardcoded test fixtures), no SSRF vectors, and no injection points. The test database is in-memory (:memory:) and closed via t.Cleanup, so there is no cross-test state leakage.
🎯 Correctness — No material issues found
No material issues found.
All four tests correctly exercise the HTTP surface for /seed-lots, and the assertions match the verified production behavior:
Route registration (internal/api/api.go:181-186): GET /:id, PATCH /:id, DELETE /:id, GET list, and POST are all registered behind requireAuth(), matching the five routes tested in TestSeedLotsRequireAuthAPI.
Stale-version 409 body (internal/api/seed_lots.go:129-153, internal/api/errors.go:66, internal/service/seed_lots.go:193-224): On ErrVersionConflict, the handler calls writeVersionConflict(c, lot), and the service pre-fills updated with remaining before returning it. The test’s assertion that current.vendor == "Fedco" is therefore correct.
Derived remaining (internal/service/seed_lots.go:98-123): fillRemaining uses explicit Count when present (the planting body here sets "count": 12 and "count": 45), so the arithmetic 50 - 12 = 38 and 50 - 57 = -7 is exact. The test correctly expects negative values to surface unclamped.
ACL masking (internal/service/seed_lots.go:129-138): ownSeedLot returns domain.ErrNotFound for non-owners; the handler maps that to 404, matching the test’s expectation that Bob gets 404 on get/patch/delete and an empty listing.
Bad plantId filter (internal/api/seed_lots.go:84-100): The handler rejects both non-numeric strings ("nope") and out-of-range IDs ("0", "-1") with 400, exactly as the test asserts.
Helpers and symbols: doJSON, decodeMap, registerAndCookie, authEngine, localCfg, createGardenAPI, objectsPath, and objectPlantingsPath all resolve in sibling _test.go files.
The only arithmetic involves small integers represented exactly as float64 after JSON unmarshaling, so comparisons like rem != 38 and version != old+1 are safe.
🧹 Code cleanliness & maintainability — No material issues found
No material issues found
The new test file follows the package's existing patterns closely:
seedLotPath, decodeList, and createPlantAPI are small, well-named helpers with t.Helper() where appropriate.
TestSeedLotCrudAPI, TestSeedLotRemainingIsDerivedAPI, TestSeedLotsArePrivateAPI, and TestSeedLotsRequireAuthAPI mirror the structure and naming conventions of sibling API tests (TestJournalCrudAPI, TestGardenCRUDFlow, TestPlantVisibilityAcrossUsersAPI, etc.).
Cross-file test utilities (decodeMap, doJSON, registerAndCookie, createGardenAPI, objectsPath, objectPlantingsPath) are reused rather than redefined.
The decodeList helper is justified by the PR's note about bare-array responses, and its []any return is honest given it's only used for length checks.
createPlantAPI is local to this file and centralizes four identical calls; it doesn't need to be hoisted to plants_test.go since no other file currently needs it.
Nothing in the change duplicates production logic, introduces leaky abstractions, or departs from surrounding style in a way that would impair future maintenance.
⚡ Performance — ⚠️ could not complete
⚠️ This reviewer failed to complete: agent: step 0: all chain targets failed
ollama-cloud/kimi-k2.6☁️ skipped (backed off until 02:18:14.876)
🧯 Error handling & edge cases — ⚠️ could not complete
⚠️ This reviewer failed to complete: agent: step 0: all chain targets failed
ollama-cloud/kimi-k2.6☁️ ollama-cloud/kimi-k2.6☁️ HTTP 429: too many concurrent requests
opencode/glm-5.2:cloud (opencode) — No material issues found
Verdict: No material issues found · ⚠️ 1/5 lens(es) errored — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — No material issues found
This is a test-only PR (new file internal/api/seed_lots_test.go, no production code touched). I reviewed it through the security lens — authn/authz, privacy/masking, input validation — verifying the tests against the actual handlers and service.
Verified:
seedLots group is registered behind h.requireAuth() at internal/api/api.go:181, so the TestSeedLotsRequireAuthAPI 401 assertions cover all five routes correctly.
ownSeedLot (internal/service/seed_lots.go:129-138) returns domain.ErrNotFound for a non-owner, masking existence — matching the TestSeedLotsArePrivateAPI 404 assertions (not 403), and ListSeedLotsForOwner scopes by owner so bob's listing-empty assertion holds.
listSeedLots (internal/api/seed_lots.go:84-100) validates plantId query with ParseInt and id < 1 rejection → 400; the test's nope/0/-1 cases line up.
doJSON/registerAndCookie/authEngine helpers exist as referenced; cookies are threaded correctly (bob uses bob's cookie, unauth uses nil).
VERDICT: No material issues found
The tests genuinely exercise the security-relevant properties of the route group (require-auth 401, owner-only access with existence masking via 404, no cross-user listing leakage, post-access self-check that the owner isn't locked out). No production code changed, so no new vulnerabilities are introduced, and the assertions correctly reflect the existing auth/ACL behavior in the handlers and service. Nothing in the security lane is materially wrong.
🎯 Correctness — No material issues found
Now let me check the count validation: does the service validate that count is positive / handle 12 vs derived? The test uses count: 12. Let me check derivedCount and how count 12 is stored.
Now I have a good understanding. The test's expected remaining calculation relies on count being stored explicitly (12 and 45), which overrides the derived formula. Let me verify the over-planting: 50 - 12 - 45 = -7. Correct.
Let me check the count field on the planting create request — it's *int. The test passes count: 12 and count: 45 as numbers. JSON decode to *int works.
Now let me check the radiusCm: 20 — but since count is set explicitly, radius doesn't matter for the remaining calc. Good.
Let me verify the ?plantId= filter behavior. The handler rejects id < 1. Test checks "0", "-1", "nope" → 400. Good.
Let me check the version conflict body shape: writeVersionConflict wraps under current, and the test checks decodeMap(...)["current"]. Good.
Let me double-check one subtle thing — the stale version PATCH uses lot["version"] (the original), but the first PATCH already bumped it. So the second PATCH with the original version should conflict. Let me verify the store's version check logic.
The logic is sound. Now let me verify the decodeMap/doJSON/authEngine helpers exist and behave as the test assumes.
Now let me verify registerAndCookie and decodeMap:
Now let me check createGardenAPI and objectsPath and objectPlantingsPath:
All helpers verified. Now let me check the plant create request — does it accept category, color, icon as the test provides? Let me verify the plants handler.
Now, an important check: the category field. The test uses "category": "vegetable". Let me verify valid categories in the service.
Good. Now let me check the plant creation: does it validate spacingCm minimum? The test uses 15, 10, 45 — all reasonable. Let me verify the icon "🌱" is fine (UTF-8).
Now let me verify the objectPlantingsPath and the bed creation request. The test creates a bed with kind: "bed", widthCm: 100, heightCm: 100, plantable: true. Let me check the object create request fields.
Now let me check whether the planting create with count: 12 will actually be accepted. The Count field is *int. JSON 12 decodes to *int pointing to 12. Good. Let me check the service validates count > 0.
Now let me verify the ErrNotFound → 404 path for the privacy test. Bob patches Alice's lot: ownSeedLot checks l.OwnerID != actorID → returns ErrNotFound → writeServiceError → 404. Correct. Same for delete. Good.
Now the seed lot create: the test sends costCents: 495 as a number, but the request struct has CostCents *int with json:"costCents". JSON 495 decodes to *int pointing to 495. Good. And packedForYear: 2026 → *int. Good.
Now let me check one subtle thing in the over-plant test: radiusCm: 20, plant spacing 10. Since count is explicitly set to 45, the derived formula is bypassed. So used = 12 + 45 = 57. remaining = 50 - 57 = -7. Correct.
But wait — the test's first planting uses count: 12. The fillRemaining uses *u.Count when not nil. So used = 12. remaining = 50 - 12 = 38. Correct.
Now let me double check the ?plantId= filter behavior — the handler rejects id < 1. Test checks "0", "-1", "nope" → 400. Good. The store's ListSeedLotsForOwner filters by plantID when not nil.
Let me check whether the decodeList is robust — it unmarshals into []any. A bare array works. But what if the list returns null? The handler does c.JSON(http.StatusOK, lots) where lots is []domain.SeedLot. If empty, gin serializes as []. If nil... let me check ListSeedLots.
Good — initialized as []domain.SeedLot{}, so empty serializes to [], not null. decodeList handles that fine.
Now let me check the remaining field serialization — is it json:"remaining"? Let me check the domain struct.
All field names match. The test reads remaining as float64. JSON numbers decode to float64 in map[string]any. Correct.
Now let me verify the auth test's expected behavior for POST /seed-lots. The test sends nil body. doJSON with nil body → encodeBody returns empty buffer (no body). The requireAuth middleware checks for cookie → 401. But wait — does requireAuth run before body binding? Yes, middleware runs first. So 401. Good.
But there's a subtlety: for POST without a body, does gin's ShouldBindJSON even get reached? No — requireAuth returns 401 and aborts. Good.
Now, one more check: the TestSeedLotsRequireAuthAPI checks POST /api/v1/seed-lots with nil body. doJSON encodes nil → empty body. The request has Content-Type: application/json but no body. requireAuth should 401 before binding. Good.
Let me verify requireAuth actually returns 401 (not 403 or something).
All good. Now let me verify one more thing about the CRUD test: after the stale-version PATCH, the test does DELETE, then GET expecting 404. The DELETE — DeleteSeedLot calls ownSeedLot first. The lot still exists (the stale patch didn't modify it). So delete succeeds → 204. Then GET → ownSeedLot → store returns ErrNotFound → 404. Good.
Wait — one subtle concern: after the first successful PATCH (vendor→Fedco, version bumped), the test then does a stale PATCH with lot["version"] (original). This should fail with 409, and the current should have vendor "Fedco" and version = original+1. The test checks cur["vendor"] != "Fedco". Good.
But the test does NOT update lot["version"] after the first PATCH — it uses the original lot map's version for the stale test. That's intentional (it's stale). Good. And then DELETE uses no version. Good.
Let me now check whether there's a subtle issue with the version comparison in the test:
lot["version"] is from the create response, updated["version"] is from the patch response. Create returns version (likely 1), patch returns version 2. So 2 != 1+1 is false → passes. Good.
Let me verify the create response actually includes version. Yes, the domain struct has Version int64 json:"version". Good.
Now let me run a quick sanity on the test file compiling — check imports are all used and available. The test imports encoding/json, net/http, strconv, testing, gin. All used: json.Unmarshal (decodeList), http.Status*, strconv.FormatInt, testing, gin.Engine. Good.
Let me verify there are no naming collisions with existing helpers. decodeList, createPlantAPI, seedLotPath — are any of these already defined?
No collisions — only defined in the new file. Good.
Let me verify the version field is present in create response. Actually I already confirmed the domain struct serializes it. Now let me check the seedLotCreateRequest validation: PlantID is binding:"required" and Unit is binding:"required". The test's create sends both. The remaining-derivation test sends plantId and unit but no vendor/sku/etc. — those are optional. Good.
Let me now verify one potential issue: the TestSeedLotRemainingIsDerivedAPI creates a bed with widthCm: 100, heightCm: 100. The first planting is at xCm: 0, yCm: 0 (center) with radius 20. Bounds check: |0| > 100/2=50? No. Good. The second planting at xCm: 40, yCm: 40 with radius 20. Bounds: |40| > 50? No. Good. Both pass.
Now let me double-check: does the bed need plantable: true? The test sets it. finalizePlanting doesn't check plantable — that's checked in CreatePlanting at line 84: if !o.Plantable { return ErrInvalidInput }. So the test correctly sets plantable: true. Good.
Let me verify the objectPlantingsPath: objectPlantingsPath(oid) = /api/v1/objects/{oid}/plantings. The route is registered for POST. Let me confirm.
Good — POST /objects/:id/plantings is registered. The path matches.
Now let me verify the objects group is behind requireAuth (so the planting creation requires auth, but the test provides a cookie):
All verified. Now let me consider whether there are any correctness issues in the test itself that would make it pass for the wrong reason or fail to catch a real bug.
One thing to scrutinize: the TestSeedLotRemainingIsDerivedAPI — does it actually verify the derivation is dynamic? It creates a lot (qty 50), plants 12, checks remaining=38. Then plants 45 more, checks remaining=-7. This does verify dynamic recomputation across two states. Good. But does it verify that editing/removing a planting updates remaining? No — the comment mentions "edited behind its back" but the test only adds. However, that's a scope choice, not a correctness bug. The test does verify derivation through the HTTP surface, which is its stated goal.
Let me reconsider the ?plantId=0 and -1 case. The handler:
Now, one thing I want to verify: the TestSeedLotsArePrivateAPI patch case sends "version": 1. Bob patches Alice's lot. The handler calls parseIDParam (succeeds, lot exists at id), then ShouldBindJSON (succeeds, version=1), then svc.UpdateSeedLot(bob, lotID, patch, 1). The service calls ownSeedLot(bob, lotID) → GetSeedLot returns Alice's lot → l.OwnerID != bob → ErrNotFound → 404. Good. The version is never checked because ownership fails first. Correct.
Now let me check the decodeMap(...)["current"] type assertion in the CRUD test:
writeVersionConflict writes gin.H{"error": ..., "current": lot} where lot is *domain.SeedLot. Gin serializes a struct pointer as a JSON object. So current decodes to map[string]any. Good. cur["vendor"] = "Fedco" (the updated value from the successful first PATCH). Correct.
I've thoroughly verified the test logic against the production code. Let me do one final check — the TestSeedLotCrudAPI checks lot["unit"] != "seeds". The create sends "unit": "seeds". The domain struct serializes Unit string json:"unit". So lot["unit"] = "seeds". Good. And lot["vendor"] = "Johnny's". Good.
Everything checks out. Let me verify there are no compile issues by checking that createPlantAPI's type assertion decodeMap(...)["id"].(float64) is safe — id is int64 serialized as JSON number → decodes to float64. Good.
Let me also confirm the test for TestSeedLotsRequireAuthAPI PATCH/DELETE on /seed-lots/1 — these go through requireAuth → 401. But do the routes PATCH /:id and DELETE /:id match /seed-lots/1? Yes, registered at lines 185-186. Good.
I've completed a thorough correctness review. The test file is well-constructed and all assertions match the actual production code behavior. The derived remaining calculations (38 and -7) are arithmetically correct, the version-conflict semantics are correctly asserted, the privacy/404 convention is correctly enforced, and all helpers and routes exist as the test assumes.
One last thing — let me double-check the over-plant creates a second planting with the same lot. Is there any constraint preventing multiple plantings against one lot? No — checkSeedLotForPlanting only validates the lot belongs to the actor and matches the plant. Multiple plantings accumulate in SeedLotUsage. Good.
The test's quantity values (100, 50, 10) are well within bounds. Good.
My review is complete. No correctness issues found.
VERDICT: No material issues found
I verified the new test file against the production code it exercises, re-deriving every constant and assertion from first principles:
remaining arithmetic (TestSeedLotRemainingIsDerivedAPI): Re-derived from fillRemaining (seed_lots.go:118-121: Remaining = Quantity - Used, where Used sums each planting's explicit Count when non-nil, else derivedCount). 50 bought − 12 planted = 38 ✓; 50 − (12+45) = −7 ✓. The negative floor is not clamped, matching the documented intent (seed_lots.go:94-97). The explicit count overrides the π·r²/spacing² formula, so radiusCm: 20 / spacing 10 are irrelevant to the result — confirmed at seed_lots.go:112-116.
Version-conflict semantics (TestSeedLotCrudAPI): Re-traced through store.UpdateSeedLot (store/seed_lots.go:113 WHERE id=? AND version=?), the ErrVersionConflict path returns the current row (store/seed_lots.go:119-124), the service fills its remaining (seed_lots.go:209-213), and writeVersionConflict (errors.go:66-71) wraps it under "current". The stale PATCH uses the original lot["version"] after a successful bump — correctly stale → 409 with vendor: "Fedco" in current ✓.
Privacy / 404-not-403 convention (TestSeedLotsArePrivateAPI): Re-traced ownSeedLot (seed_lots.go:129-138) → l.OwnerID != actorID → ErrNotFound → writeServiceError (errors.go:26-27) → 404. Bob's list is scoped by owner_id in SQL (store/seed_lots.go:68). The "alice still has access" guard prevents the test passing by breaking access for everyone ✓.
?plantId= validation (TestSeedLotCrudAPI): "0"/"-1"/"nope" all hit id < 1 || err != nil → 400 (seed_lots.go:87-91) ✓.
Auth gating (TestSeedLotsRequireAuthAPI): All five routes are under h.requireAuth() (api.go:181-186), which returns 401 (auth.go:126) before any binding runs ✓.
Helper existence: decodeMap, doJSON, authEngine, registerAndCookie, createGardenAPI, objectsPath, objectPlantingsPath all exist in sibling _test.go files with the signatures the new file expects; no name collisions with decodeList/createPlantAPI/seedLotPath.
Bare-array decode: decodeList correctly targets []any; the store initializes lots := []domain.SeedLot{} (store/seed_lots.go:86) so an empty list serializes to [], not null — the helper would not silently read zero.
All assertions match the actual production behavior; no logic bugs, wrong constants, or tests-passing-for-the-wrong-reason issues found.
🧹 Code cleanliness & maintainability — No material issues found
VERDICT: No material issues found
The new helpers (decodeList, seedLotPath, createPlantAPI) are small, well-named, and the only one with a non-obvious purpose (decodeList) is documented with the exact reason it differs from the existing decodeMap (bare array vs {\"entries\":…}). Verified decodeMap lives in gardens_test.go:27 and returns map[string]any, so the bare-array case genuinely needs a separate helper rather than reusing it.
createPlantAPI duplicates a plant-creation sequence that other test files presumably inline; it's local to this file and used 3× here, which is a fair consolidation point. Not worth extracting to shared helpers given the rest of the suite doesn't already share one.
The tests follow the existing patterns in the package (authEngine/localCfg/doJSON/registerAndCookie/decodeMap), use the *_API test-name convention, and the table-driven subtests in the privacy/auth cases match the style used elsewhere.
Clean through the maintainability lens.
⚡ Performance — No material issues found
All helpers resolve. This is a test-only PR adding a new test file. No production code changed, no hot paths, no queries, no allocations in production code. The test file itself is not performance-sensitive.
No material performance issues found — the PR is purely a test addition and my lens covers production efficiency, which is untouched here.
VERDICT: No material issues found
No production code changed in this PR; it adds internal/api/seed_lots_test.go only. From a performance lens there is nothing to regress — no query, allocation, hot-loop, or blocking-path change to scrutinize.
Verified that every helper the new test file calls (decodeMap, doJSON, authEngine, registerAndCookie, createGardenAPI, objectsPath, objectPlantingsPath, localCfg) resolves to an existing definition in the internal/api test helpers, so the file compiles against the checked-out repo.
🧯 Error handling & edge cases — ⚠️ could not complete
⚠️ This reviewer failed to complete: opencode run context deadline exceeded
opencode/kimi-k2.6:cloud (opencode) — No material issues found
Verdict: No material issues found · ⚠️ 2/5 lens(es) errored — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — No material issues found
No material issues found
This PR adds only test code (internal/api/seed_lots_test.go). No production code changed. Through the security lens:
Authn/authz: The new tests correctly assert that unauthenticated callers get 401 (TestSeedLotsRequireAuthAPI) and that cross-user access returns 404 (existence masked, per project convention) rather than 403 (TestSeedLotsArePrivateAPI). These verify the existing requireAuth and service-level ACL enforcement, not weaken them.
Injection / SSRF / deserialization / secret leakage / unsafe defaults: Not applicable — no handler, service, or store logic is modified. The test inputs are hardcoded literals and standard test helpers; no untrusted data is parsed unsafely or reflected into sensitive contexts.
Input validation: The tests exercise the existing handler's validation (e.g., bad plantId filter → 400) and confirm it is enforced at the API boundary.
Nothing in this diff introduces or masks a security issue.
🎯 Correctness — ⚠️ could not complete
⚠️ This reviewer failed to complete: opencode run context deadline exceeded
🧹 Code cleanliness & maintainability — No material issues found
No material issues found
The new test file follows existing patterns rigorously:
seedLotPath matches the camelCase *Path helper convention used across every other handler test (journalPath, plantingPath, gardenPath, etc.).
decodeList is a clean, well-commented local helper that fills a real gap — the test suite has no shared array decoder, and existing list assertions inline their own Unmarshal because they do extra work (filtering, counting) that decodeList wouldn't cover. Keeping it local is appropriate.
createPlantAPI mirrors the existing createGardenAPI in objects_test.go: a pragmatic, package-local helper that hides irrelevant required fields. It does not duplicate any existing symbol.
Test structure is linear, names are consistent with the file (TestSeedLotCrudAPI parallels TestJournalCrudAPI), and comments explain why the tests exist rather than restating what they do.
Imports are used, any vs map[string]any usage aligns with surrounding tests, and magic numbers are inline constants consistent with the rest of the API test suite.
Nothing in the readability, structure, duplication, or abstraction layers raises a concern.
⚡ Performance — No material issues found
No material issues found
The PR adds only test code (internal/api/seed_lots_test.go) with no production changes. From a performance lens:
The tests create a bounded, small number of resources per case (≤2 users, ≤2 plants, ≤2 lots, ≤2 plantings). No unbounded growth.
No loops that amplify database round-trips. The only loops iterate over 3–5 hardcoded test cases (bad filter values, HTTP methods, routes), each issuing a single request.
authEngine spins up a fresh in-memory database per test via t.Cleanup, so there's no cross-test state accumulation.
The production paths exercised (ListSeedLots → fillRemaining → SeedLotUsage) batch lot-ID lookup into a single query with placeholders rather than N+1.
Nothing here that would regress CI runtime or resource usage.
🧯 Error handling & edge cases — ⚠️ could not complete
⚠️ This reviewer failed to complete: opencode run context deadline exceeded
Automated adversarial review by Gadfly — consensus across the model swarm. Advisory only — does not block merge.
<!-- gadfly-consensus -->
## 🪰 Gadfly review — consensus across 5 models
**Verdict: Minor issues** · 3 findings (0 with multi-model agreement)
<details><summary>3 single-model findings (lower confidence)</summary>
| | Finding | Where | Model | Lens |
|--|--|--|--|--|
| 🟡 | createPlantAPI plant-creation helper is declared in seed_lots_test.go instead of plants_test.go, where custom-plant creation is currently inlined | `internal/api/seed_lots_test.go:29` | glm-5.2:cloud | maintainability |
| ⚪ | decodeList returns []any, diverging from the package's []map[string]any list-decoding convention | `internal/api/seed_lots_test.go:19` | glm-5.2:cloud | maintainability |
| ⚪ | Missing status-code check before decoding response body after over-planting, masking the real error on failure | `internal/api/seed_lots_test.go:192` | claude-code/sonnet | error-handling |
</details>
<details><summary>Per-model detail</summary>
<details><summary><b>claude-code/sonnet</b> (claude-code) — Minor issues</summary>
**Verdict: Minor issues** — 5 reviewers: security, correctness, maintainability, performance, error-handling
<details><summary><b>🔒 Security</b> — No material issues found</summary>
Good — SourceURL is already validated at the service level (existing test `TestSourceURLValidation` covers scheme rejection), and this API test PR doesn't touch or weaken that. It's stored, not fetched server-side, so no SSRF surface. This PR is test-only and everything in it checks out against the real handlers.
## Review
**VERDICT: No material issues found**
Findings from a security-lens pass:
- Verified `internal/api/seed_lots.go` — every handler (`listSeedLots`, `getSeedLot`, `updateSeedLot`, `deleteSeedLot`) scopes to `mustActor(c).ID`, matching the "no-access is 404" convention `TestSeedLotsArePrivateAPI` exercises. `plantId` query param is parsed with `strconv.ParseInt` and rejected below 1 before use — no injection surface, consistent with the test's `nope/0/-1` → 400 assertions.
- Verified the route group sits behind `requireAuth` per `TestSeedLotsRequireAuthAPI`'s expectations by reading the handler file; no handler reads `mustActor` optimistically or falls back to an unauthenticated path.
- Checked `SourceURL` (present in the create/patch request structs but not exercised by this new API test) — it's validated server-side by `validSourceURL` in `internal/service/seed_lots.go` and already covered by `TestSourceURLValidation` in the service package (rejects `javascript:` etc.). Not fetched server-side, so no SSRF risk from an untested field here.
- `registerAndCookie`/`authEngine` test helpers reuse the existing auth test scaffolding (`auth_test.go`) — cookies, CSRF guard, and password handling are unchanged and were already covered by prior tests; nothing in this diff weakens them.
- No secrets, tokens, or credentials are hardcoded beyond the standard `password123` test fixture already used throughout the existing test suite.
This PR adds only test code and exercises existing, already-secure production paths correctly. Nothing in my lane to flag.
</details>
<details><summary><b>🎯 Correctness</b> — No material issues found</summary>
Route registration confirmed at `internal/api/api.go:181` behind `requireAuth()`, matching the test's assumptions. Everything in this diff checks out against the actual code.
## Review
**VERDICT: No material issues found**
This PR adds only test code (no production changes), and every behavioral claim the tests make matches what I verified by reading the actual handler/service/store code:
- `internal/api/seed_lots.go:88` — `plantId` filter rejects `id < 1` or unparseable, so `"nope"`, `"0"`, `"-1"` all correctly expect 400 (verified against the test's bad-value loop).
- `internal/service/seed_lots.go:98-123` (`fillRemaining`) — `remaining = quantity - used`, uncapped, matching the test's expectation of both `38` (50-12) and `-7` (50-57) after over-planting. `used` is driven by the explicit `count` field when set (`internal/service/seed_lots.go:113-115`), so the test's literal `count: 12` / `count: 45` payloads produce exactly the arithmetic asserted — I traced this through `store/seed_lots.go`'s `SeedLotUsage` SQL as well, which correctly returns raw `count`/`radius`/`spacing` per active planting rather than pre-aggregating.
- `internal/service/seed_lots.go:129-138` (`ownSeedLot`) — cross-owner access returns `domain.ErrNotFound`, so Bob's get/patch/delete against Alice's lot are all 404s, matching `TestSeedLotsArePrivateAPI`.
- `internal/store/seed_lots.go:105-129` (`UpdateSeedLot`) — a version mismatch fetches and returns the current row alongside `ErrVersionConflict`, and `internal/service/seed_lots.go:204-214` fills `remaining` onto that returned row before it reaches the client — matching the test's assertion that the 409 body's `current.vendor` reflects the already-applied first patch.
- `internal/api/api.go:181` — the `/seed-lots` group is registered under `h.requireAuth()`, matching `TestSeedLotsRequireAuthAPI`.
- Planting bounds check (`internal/service/plantings.go:285`) only requires the plop **center** within the object's half-extents, so the test's `(40,40)` placement in a 100×100 bed is valid and won't spuriously fail the over-plant step.
- Test helpers (`createGardenAPI`, `objectsPath`, `objectPlantingsPath`, `decodeMap`, `doJSON`, `authEngine`, `localCfg`, `registerAndCookie`) all exist with the exact signatures the new file calls them with — no compile break.
No logic bugs, no incorrect magic numbers, and the domain-specific assertions (derived-remaining arithmetic, ACL masking) are correct against the real implementation rather than just plausible-looking.
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — No material issues found</summary>
All the helpers this new file relies on (`doJSON`, `decodeMap`, `authEngine`, `localCfg`, `registerAndCookie`, `createGardenAPI`, `objectsPath`, `objectPlantingsPath`) already exist and are reused correctly, with no duplication introduced. `createPlantAPI` and `decodeList` are genuinely new (nothing equivalent exists elsewhere), and the inline garden/bed-object creation in `TestSeedLotRemainingIsDerivedAPI` matches the same inline pattern `plantings_test.go` already uses (there's no existing "create bed" helper to have reused instead). Email-namespacing (`alice-lots@…`) follows the same per-file collision-avoidance convention seen in `gardens_test.go`/`plants_test.go`.
VERDICT: No material issues found
Nothing in this diff raises cleanliness or maintainability concerns: the new test file follows the existing `internal/api/*_test.go` conventions closely (table-driven sub-cases, `t.Fatalf`/`t.Errorf` usage, `Path` helper naming, doc comments explaining *why* a test exists), the one new shared helper (`decodeList`) is justified and documented (bare-array vs `{"entries":…}` response shape), and no code is duplicated that should have called an existing helper instead.
</details>
<details><summary><b>⚡ Performance</b> — No material issues found</summary>
**VERDICT: No material issues found**
This PR adds only test code (`internal/api/seed_lots_test.go`) — no production paths, hot loops, or query patterns are touched. Through the performance lens specifically:
- All requests in each test are small, fixed-count, sequential HTTP round-trips against an in-memory test router (max ~10 requests per test, one `for` loop over a 3-element slice at `internal/api/seed_lots_test.go:94-98`, another over 3 method cases at `:218-230`). No unbounded growth, no loop scaling with external input, no N+1 pattern.
- Planting counts used (`count: 12`, `count: 45`) are trivial and don't stress any batch-insert path.
- No new helper introduces quadratic behavior — `decodeList`/`decodeMap`/`createPlantAPI` are all O(1) or O(response size) JSON decodes.
Nothing here would move the needle on CI time or resource usage in a way worth flagging under a performance lens. Correctness/test-design questions (e.g., whether the `remaining` derivation assertions are appropriate) belong to other lenses, not this one.
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — Minor issues</summary>
Confirmed against the actual code: `decodeMap` (internal/api/gardens_test.go:26-34) just unmarshals into a `map[string]any` and only `t.Fatalf`s on a JSON parse error — it never checks for specific keys. So on a non-200 error body like `{"error": "..."}`, it returns cleanly with `"remaining"` absent, the type assertion `.(float64)` fails, `ok` is false, and line 193-194 falls through to a generic `t.Errorf("remaining after over-planting = %v, want -7 ...")` with no status code. Every other GET/decode pair in the file (lines 173-176, 160-162, etc.) guards with `if w.Code != http.StatusOK { t.Fatalf(...) }` first — this one at line 192 does not. Finding confirmed.
## VERDICT: Minor issues
- `internal/api/seed_lots_test.go:192-195` — The final GET in `TestSeedLotRemainingIsDerivedAPI` decodes the response body without first checking `w.Code == http.StatusOK`, unlike every other GET/decode pair in this file (e.g. lines 173-176, 160-162). If this request fails (e.g. a 404/500 due to a regression), `decodeMap` (internal/api/gardens_test.go:26-34) will still unmarshal successfully since it only fails on invalid JSON, not missing keys — `["remaining"]` will be absent, the assertion falls through to `!ok`, and the failure reports `"remaining after over-planting = <nil>, want -7"` with no status code or error body surfaced. That masks the actual failure (an HTTP error) behind a generic "wrong value" message. Suggested fix: add the same `w.Code != http.StatusOK` guard used at line 174 before decoding.
</details>
</details>
<details><summary><b>glm-5.2:cloud</b> (ollama-cloud) — Minor issues</summary>
**Verdict: Minor issues · ⚠️ 3/5 lens(es) errored** — 5 reviewers: security, correctness, maintainability, performance, error-handling
<details><summary><b>🔒 Security</b> — ⚠️ could not complete</summary>
⚠️ This reviewer failed to complete: agent: step 6: all chain targets failed
ollama-cloud/glm-5.2:cloud: ollama-cloud/glm-5.2:cloud: HTTP 429: too many concurrent requests
</details>
<details><summary><b>🎯 Correctness</b> — ⚠️ could not complete</summary>
⚠️ This reviewer failed to complete: agent: step 7: all chain targets failed
ollama-cloud/glm-5.2:cloud: ollama-cloud/glm-5.2:cloud: HTTP 429: too many concurrent requests
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary>
Both findings verified against the actual code.
**Verdict: Minor issues**
- **`internal/api/seed_lots_test.go:19` — `decodeList` diverges from the established list-decoding convention.** The package's other list-decoding sites decode into `[]map[string]any` inline: `gardens_test.go:56-58`, `plants_test.go:18`, and `shares_test.go:41-42`. The new `decodeList` helper returns `[]any` instead. It is only used for `len()` checks in this file, so it works, but it's a slightly leakier type and breaks the visual pattern a reader of this package expects. Either return `[]map[string]any` to match, or keep `[]any` as a deliberate minor inconsistency.
- **`internal/api/seed_lots_test.go:29` — `createPlantAPI` lives in the wrong file.** A plant-creation helper is plant-domain code, but it's declared in `seed_lots_test.go`. `plants_test.go` currently inlines custom-plant creation (`plants_test.go:46-47` and `85-86`) with a near-identical payload rather than calling a shared helper, so this PR introduces a second, parallel pattern instead of promoting a shared one. Grep confirms no `createPlant`/`createPlantAPI` helper exists elsewhere; `plants_test.go` inlines the POST. Low-churn fix: move `createPlantAPI` into `plants_test.go`.
No dead code, no deep nesting, and the four test functions follow the same structure and comment style as `TestJournalCrudAPI`, the closest sibling.
</details>
<details><summary><b>⚡ Performance</b> — No material issues found</summary>
**VERDICT: No material issues found**
This is a test-only PR (new `internal/api/seed_lots_test.go`, no production code changed). Through the performance lens:
- The new helpers (`decodeList`, `createPlantAPI`) do a single request/decode each and are called a handful of times per test — no hot loops, no unbounded growth.
- `decodeList` unmarshals into `[]any` and only checks `len()`; nothing pathological.
- Each test spins up its own in-memory DB via the pre-existing `authEngine`/`localCfg` pattern — that's the established convention across the sibling `*_test.go` files, not a regression this PR introduces.
- The exercised handlers (`listSeedLots`, `getSeedLot`, etc.) are unchanged; the `?plantId=` filter path uses a single scoped query, no N+1.
No performance issues in this PR's scope.
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — ⚠️ could not complete</summary>
⚠️ This reviewer failed to complete: agent: step 6: all chain targets failed
ollama-cloud/glm-5.2:cloud: ollama-cloud/glm-5.2:cloud: HTTP 429: too many concurrent requests
</details>
</details>
<details><summary><b>kimi-k2.6:cloud</b> (ollama-cloud) — No material issues found</summary>
**Verdict: No material issues found · ⚠️ 2/5 lens(es) errored** — 5 reviewers: security, correctness, maintainability, performance, error-handling
<details><summary><b>🔒 Security</b> — No material issues found</summary>
**No material issues found**
This is a pure test-addition PR with no production code changes. From a security lens, the new tests actually *improve* coverage of authn/authz behavior rather than weaken anything.
- **`TestSeedLotsRequireAuthAPI`** (l. 247) correctly verifies that all five routes return 401 when called without a session cookie, ensuring the `requireAuth` middleware is wired.
- **`TestSeedLotsArePrivateAPI`** (l. 201) verifies cross-user isolation: Bob gets 404 (not 403) on Alice’s lot, consistent with the project’s “mask existence” convention, and confirms the lot doesn’t leak into Bob’s listing.
- **`TestSeedLotCrudAPI`** (l. 50) exercises input validation by asserting that malformed `plantId` filters (`"nope"`, `"0"`, `"-1"`) return 400.
There are no hardcoded secrets, no unsafe deserialization of untrusted data (all inputs are hardcoded test fixtures), no SSRF vectors, and no injection points. The test database is in-memory (`:memory:`) and closed via `t.Cleanup`, so there is no cross-test state leakage.
</details>
<details><summary><b>🎯 Correctness</b> — No material issues found</summary>
No material issues found.
All four tests correctly exercise the HTTP surface for `/seed-lots`, and the assertions match the verified production behavior:
- **Route registration** (`internal/api/api.go:181-186`): `GET /:id`, `PATCH /:id`, `DELETE /:id`, `GET` list, and `POST` are all registered behind `requireAuth()`, matching the five routes tested in `TestSeedLotsRequireAuthAPI`.
- **Stale-version 409 body** (`internal/api/seed_lots.go:129-153`, `internal/api/errors.go:66`, `internal/service/seed_lots.go:193-224`): On `ErrVersionConflict`, the handler calls `writeVersionConflict(c, lot)`, and the service pre-fills `updated` with remaining before returning it. The test’s assertion that `current.vendor == "Fedco"` is therefore correct.
- **Derived `remaining`** (`internal/service/seed_lots.go:98-123`): `fillRemaining` uses explicit `Count` when present (the planting body here sets `"count": 12` and `"count": 45`), so the arithmetic `50 - 12 = 38` and `50 - 57 = -7` is exact. The test correctly expects negative values to surface unclamped.
- **ACL masking** (`internal/service/seed_lots.go:129-138`): `ownSeedLot` returns `domain.ErrNotFound` for non-owners; the handler maps that to 404, matching the test’s expectation that Bob gets 404 on get/patch/delete and an empty listing.
- **Bad `plantId` filter** (`internal/api/seed_lots.go:84-100`): The handler rejects both non-numeric strings (`"nope"`) and out-of-range IDs (`"0"`, `"-1"`) with 400, exactly as the test asserts.
- **Helpers and symbols**: `doJSON`, `decodeMap`, `registerAndCookie`, `authEngine`, `localCfg`, `createGardenAPI`, `objectsPath`, and `objectPlantingsPath` all resolve in sibling `_test.go` files.
The only arithmetic involves small integers represented exactly as `float64` after JSON unmarshaling, so comparisons like `rem != 38` and `version != old+1` are safe.
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — No material issues found</summary>
**No material issues found**
The new test file follows the package's existing patterns closely:
- `seedLotPath`, `decodeList`, and `createPlantAPI` are small, well-named helpers with `t.Helper()` where appropriate.
- `TestSeedLotCrudAPI`, `TestSeedLotRemainingIsDerivedAPI`, `TestSeedLotsArePrivateAPI`, and `TestSeedLotsRequireAuthAPI` mirror the structure and naming conventions of sibling API tests (`TestJournalCrudAPI`, `TestGardenCRUDFlow`, `TestPlantVisibilityAcrossUsersAPI`, etc.).
- Cross-file test utilities (`decodeMap`, `doJSON`, `registerAndCookie`, `createGardenAPI`, `objectsPath`, `objectPlantingsPath`) are reused rather than redefined.
- The `decodeList` helper is justified by the PR's note about bare-array responses, and its `[]any` return is honest given it's only used for length checks.
- `createPlantAPI` is local to this file and centralizes four identical calls; it doesn't need to be hoisted to `plants_test.go` since no other file currently needs it.
Nothing in the change duplicates production logic, introduces leaky abstractions, or departs from surrounding style in a way that would impair future maintenance.
</details>
<details><summary><b>⚡ Performance</b> — ⚠️ could not complete</summary>
⚠️ This reviewer failed to complete: agent: step 0: all chain targets failed
ollama-cloud/kimi-k2.6:cloud: skipped (backed off until 02:18:14.876)
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — ⚠️ could not complete</summary>
⚠️ This reviewer failed to complete: agent: step 0: all chain targets failed
ollama-cloud/kimi-k2.6:cloud: ollama-cloud/kimi-k2.6:cloud: HTTP 429: too many concurrent requests
</details>
</details>
<details><summary><b>opencode/glm-5.2:cloud</b> (opencode) — No material issues found</summary>
**Verdict: No material issues found · ⚠️ 1/5 lens(es) errored** — 5 reviewers: security, correctness, maintainability, performance, error-handling
<details><summary><b>🔒 Security</b> — No material issues found</summary>
This is a test-only PR (new file `internal/api/seed_lots_test.go`, no production code touched). I reviewed it through the security lens — authn/authz, privacy/masking, input validation — verifying the tests against the actual handlers and service.
Verified:
- `seedLots` group is registered behind `h.requireAuth()` at `internal/api/api.go:181`, so the `TestSeedLotsRequireAuthAPI` 401 assertions cover all five routes correctly.
- `ownSeedLot` (`internal/service/seed_lots.go:129-138`) returns `domain.ErrNotFound` for a non-owner, masking existence — matching the `TestSeedLotsArePrivateAPI` 404 assertions (not 403), and `ListSeedLotsForOwner` scopes by owner so bob's listing-empty assertion holds.
- `listSeedLots` (`internal/api/seed_lots.go:84-100`) validates `plantId` query with `ParseInt` and `id < 1` rejection → 400; the test's `nope`/`0`/`-1` cases line up.
- `doJSON`/`registerAndCookie`/`authEngine` helpers exist as referenced; cookies are threaded correctly (bob uses bob's cookie, unauth uses `nil`).
## VERDICT: No material issues found
The tests genuinely exercise the security-relevant properties of the route group (require-auth 401, owner-only access with existence masking via 404, no cross-user listing leakage, post-access self-check that the owner isn't locked out). No production code changed, so no new vulnerabilities are introduced, and the assertions correctly reflect the existing auth/ACL behavior in the handlers and service. Nothing in the security lane is materially wrong.
</details>
<details><summary><b>🎯 Correctness</b> — No material issues found</summary>
Now let me check the `count` validation: does the service validate that count is positive / handle 12 vs derived? The test uses `count: 12`. Let me check derivedCount and how count 12 is stored.
Now I have a good understanding. The test's expected `remaining` calculation relies on `count` being stored explicitly (12 and 45), which overrides the derived formula. Let me verify the over-planting: 50 - 12 - 45 = -7. Correct.
Let me check the `count` field on the planting create request — it's `*int`. The test passes `count: 12` and `count: 45` as numbers. JSON decode to `*int` works.
Now let me check the `radiusCm: 20` — but since count is set explicitly, radius doesn't matter for the remaining calc. Good.
Let me verify the `?plantId=` filter behavior. The handler rejects `id < 1`. Test checks "0", "-1", "nope" → 400. Good.
Let me check the version conflict body shape: writeVersionConflict wraps under `current`, and the test checks `decodeMap(...)["current"]`. Good.
Let me double-check one subtle thing — the stale version PATCH uses `lot["version"]` (the original), but the first PATCH already bumped it. So the second PATCH with the original version should conflict. Let me verify the store's version check logic.
The logic is sound. Now let me verify the `decodeMap`/`doJSON`/`authEngine` helpers exist and behave as the test assumes.
Now let me verify `registerAndCookie` and `decodeMap`:
Now let me check `createGardenAPI` and `objectsPath` and `objectPlantingsPath`:
All helpers verified. Now let me check the plant create request — does it accept `category`, `color`, `icon` as the test provides? Let me verify the plants handler.
Now, an important check: the `category` field. The test uses `"category": "vegetable"`. Let me verify valid categories in the service.
Good. Now let me check the plant creation: does it validate `spacingCm` minimum? The test uses 15, 10, 45 — all reasonable. Let me verify the `icon` "🌱" is fine (UTF-8).
Now let me verify the `objectPlantingsPath` and the bed creation request. The test creates a bed with `kind: "bed"`, `widthCm: 100`, `heightCm: 100`, `plantable: true`. Let me check the object create request fields.
Now let me check whether the planting create with `count: 12` will actually be accepted. The `Count` field is `*int`. JSON `12` decodes to `*int` pointing to 12. Good. Let me check the service validates count > 0.
Now let me verify the `ErrNotFound` → 404 path for the privacy test. Bob patches Alice's lot: `ownSeedLot` checks `l.OwnerID != actorID` → returns `ErrNotFound` → `writeServiceError` → 404. Correct. Same for delete. Good.
Now the seed lot create: the test sends `costCents: 495` as a number, but the request struct has `CostCents *int` with `json:"costCents"`. JSON 495 decodes to `*int` pointing to 495. Good. And `packedForYear: 2026` → `*int`. Good.
Now let me check one subtle thing in the over-plant test: `radiusCm: 20`, plant spacing 10. Since count is explicitly set to 45, the derived formula is bypassed. So used = 12 + 45 = 57. remaining = 50 - 57 = -7. Correct.
But wait — the test's first planting uses `count: 12`. The `fillRemaining` uses `*u.Count` when not nil. So used = 12. remaining = 50 - 12 = 38. Correct.
Now let me double check the `?plantId=` filter behavior — the handler rejects `id < 1`. Test checks "0", "-1", "nope" → 400. Good. The store's `ListSeedLotsForOwner` filters by plantID when not nil.
Let me check whether the `decodeList` is robust — it unmarshals into `[]any`. A bare array works. But what if the list returns `null`? The handler does `c.JSON(http.StatusOK, lots)` where lots is `[]domain.SeedLot`. If empty, gin serializes as `[]`. If nil... let me check ListSeedLots.
Good — initialized as `[]domain.SeedLot{}`, so empty serializes to `[]`, not `null`. `decodeList` handles that fine.
Now let me check the `remaining` field serialization — is it `json:"remaining"`? Let me check the domain struct.
All field names match. The test reads `remaining` as `float64`. JSON numbers decode to `float64` in `map[string]any`. Correct.
Now let me verify the auth test's expected behavior for POST /seed-lots. The test sends `nil` body. `doJSON` with `nil` body → `encodeBody` returns empty buffer (no body). The `requireAuth` middleware checks for cookie → 401. But wait — does `requireAuth` run before body binding? Yes, middleware runs first. So 401. Good.
But there's a subtlety: for POST without a body, does gin's `ShouldBindJSON` even get reached? No — `requireAuth` returns 401 and aborts. Good.
Now, one more check: the `TestSeedLotsRequireAuthAPI` checks POST `/api/v1/seed-lots` with `nil` body. `doJSON` encodes `nil` → empty body. The request has `Content-Type: application/json` but no body. `requireAuth` should 401 before binding. Good.
Let me verify `requireAuth` actually returns 401 (not 403 or something).
All good. Now let me verify one more thing about the CRUD test: after the stale-version PATCH, the test does DELETE, then GET expecting 404. The DELETE — `DeleteSeedLot` calls `ownSeedLot` first. The lot still exists (the stale patch didn't modify it). So delete succeeds → 204. Then GET → ownSeedLot → store returns ErrNotFound → 404. Good.
Wait — one subtle concern: after the first successful PATCH (vendor→Fedco, version bumped), the test then does a stale PATCH with `lot["version"]` (original). This should fail with 409, and the `current` should have vendor "Fedco" and version = original+1. The test checks `cur["vendor"] != "Fedco"`. Good.
But the test does NOT update `lot["version"]` after the first PATCH — it uses the original `lot` map's version for the stale test. That's intentional (it's stale). Good. And then DELETE uses no version. Good.
Let me now check whether there's a subtle issue with the `version` comparison in the test:
```go
if updated["version"].(float64) != lot["version"].(float64)+1 {
```
`lot["version"]` is from the create response, `updated["version"]` is from the patch response. Create returns version (likely 1), patch returns version 2. So `2 != 1+1` is false → passes. Good.
Let me verify the create response actually includes `version`. Yes, the domain struct has `Version int64 json:"version"`. Good.
Now let me run a quick sanity on the test file compiling — check imports are all used and available. The test imports `encoding/json`, `net/http`, `strconv`, `testing`, `gin`. All used: `json.Unmarshal` (decodeList), `http.Status*`, `strconv.FormatInt`, `testing`, `gin.Engine`. Good.
Let me verify there are no naming collisions with existing helpers. `decodeList`, `createPlantAPI`, `seedLotPath` — are any of these already defined?
No collisions — only defined in the new file. Good.
Let me verify the `version` field is present in create response. Actually I already confirmed the domain struct serializes it. Now let me check the `seedLotCreateRequest` validation: `PlantID` is `binding:"required"` and `Unit` is `binding:"required"`. The test's create sends both. The remaining-derivation test sends `plantId` and `unit` but no `vendor`/`sku`/etc. — those are optional. Good.
Let me now verify one potential issue: the `TestSeedLotRemainingIsDerivedAPI` creates a bed with `widthCm: 100, heightCm: 100`. The first planting is at `xCm: 0, yCm: 0` (center) with radius 20. Bounds check: `|0| > 100/2=50`? No. Good. The second planting at `xCm: 40, yCm: 40` with radius 20. Bounds: `|40| > 50`? No. Good. Both pass.
Now let me double-check: does the bed need `plantable: true`? The test sets it. `finalizePlanting` doesn't check plantable — that's checked in `CreatePlanting` at line 84: `if !o.Plantable { return ErrInvalidInput }`. So the test correctly sets `plantable: true`. Good.
Let me verify the `objectPlantingsPath`:
`objectPlantingsPath(oid)` = `/api/v1/objects/{oid}/plantings`. The route is registered for POST. Let me confirm.
Good — `POST /objects/:id/plantings` is registered. The path matches.
Now let me verify the objects group is behind requireAuth (so the planting creation requires auth, but the test provides a cookie):
All verified. Now let me consider whether there are any correctness issues in the test itself that would make it pass for the wrong reason or fail to catch a real bug.
One thing to scrutinize: the `TestSeedLotRemainingIsDerivedAPI` — does it actually verify the derivation is *dynamic*? It creates a lot (qty 50), plants 12, checks remaining=38. Then plants 45 more, checks remaining=-7. This does verify dynamic recomputation across two states. Good. But does it verify that *editing/removing* a planting updates remaining? No — the comment mentions "edited behind its back" but the test only adds. However, that's a scope choice, not a correctness bug. The test does verify derivation through the HTTP surface, which is its stated goal.
Let me reconsider the `?plantId=0` and `-1` case. The handler:
```go
if raw := c.Query("plantId"); raw != "" {
id, err := strconv.ParseInt(raw, 10, 64)
if err != nil || id < 1 {
writeAPIError(c, http.StatusBadRequest, ...)
}
}
```
`"0"` → ParseInt succeeds, id=0, `0 < 1` true → 400. `"-1"` → ParseInt succeeds, id=-1, `-1 < 1` true → 400. `"nope"` → ParseInt fails → 400. All correct.
Now, one thing I want to verify: the `TestSeedLotsArePrivateAPI` patch case sends `"version": 1`. Bob patches Alice's lot. The handler calls `parseIDParam` (succeeds, lot exists at id), then `ShouldBindJSON` (succeeds, version=1), then `svc.UpdateSeedLot(bob, lotID, patch, 1)`. The service calls `ownSeedLot(bob, lotID)` → `GetSeedLot` returns Alice's lot → `l.OwnerID != bob` → `ErrNotFound` → 404. Good. The version is never checked because ownership fails first. Correct.
Now let me check the `decodeMap(...)["current"]` type assertion in the CRUD test:
```go
if cur, ok := decodeMap(t, w.Body.Bytes())["current"].(map[string]any); !ok || cur["vendor"] != "Fedco" {
```
`writeVersionConflict` writes `gin.H{"error": ..., "current": lot}` where `lot` is `*domain.SeedLot`. Gin serializes a struct pointer as a JSON object. So `current` decodes to `map[string]any`. Good. `cur["vendor"]` = "Fedco" (the updated value from the successful first PATCH). Correct.
I've thoroughly verified the test logic against the production code. Let me do one final check — the `TestSeedLotCrudAPI` checks `lot["unit"] != "seeds"`. The create sends `"unit": "seeds"`. The domain struct serializes `Unit string json:"unit"`. So `lot["unit"]` = "seeds". Good. And `lot["vendor"]` = "Johnny's". Good.
Everything checks out. Let me verify there are no compile issues by checking that `createPlantAPI`'s type assertion `decodeMap(...)["id"].(float64)` is safe — id is `int64` serialized as JSON number → decodes to `float64`. Good.
Let me also confirm the test for `TestSeedLotsRequireAuthAPI` PATCH/DELETE on `/seed-lots/1` — these go through `requireAuth` → 401. But do the routes `PATCH /:id` and `DELETE /:id` match `/seed-lots/1`? Yes, registered at lines 185-186. Good.
I've completed a thorough correctness review. The test file is well-constructed and all assertions match the actual production code behavior. The derived `remaining` calculations (38 and -7) are arithmetically correct, the version-conflict semantics are correctly asserted, the privacy/404 convention is correctly enforced, and all helpers and routes exist as the test assumes.
One last thing — let me double-check the `over-plant` creates a second planting with the same lot. Is there any constraint preventing multiple plantings against one lot? No — `checkSeedLotForPlanting` only validates the lot belongs to the actor and matches the plant. Multiple plantings accumulate in `SeedLotUsage`. Good.
The test's quantity values (100, 50, 10) are well within bounds. Good.
My review is complete. No correctness issues found.
## VERDICT: No material issues found
I verified the new test file against the production code it exercises, re-deriving every constant and assertion from first principles:
- **`remaining` arithmetic** (`TestSeedLotRemainingIsDerivedAPI`): Re-derived from `fillRemaining` (seed_lots.go:118-121: `Remaining = Quantity - Used`, where `Used` sums each planting's explicit `Count` when non-nil, else `derivedCount`). 50 bought − 12 planted = 38 ✓; 50 − (12+45) = −7 ✓. The negative floor is not clamped, matching the documented intent (seed_lots.go:94-97). The explicit `count` overrides the π·r²/spacing² formula, so `radiusCm: 20` / spacing 10 are irrelevant to the result — confirmed at seed_lots.go:112-116.
- **Version-conflict semantics** (`TestSeedLotCrudAPI`): Re-traced through `store.UpdateSeedLot` (store/seed_lots.go:113 `WHERE id=? AND version=?`), the `ErrVersionConflict` path returns the *current* row (store/seed_lots.go:119-124), the service fills its `remaining` (seed_lots.go:209-213), and `writeVersionConflict` (errors.go:66-71) wraps it under `"current"`. The stale PATCH uses the original `lot["version"]` after a successful bump — correctly stale → 409 with `vendor: "Fedco"` in `current` ✓.
- **Privacy / 404-not-403 convention** (`TestSeedLotsArePrivateAPI`): Re-traced `ownSeedLot` (seed_lots.go:129-138) → `l.OwnerID != actorID` → `ErrNotFound` → `writeServiceError` (errors.go:26-27) → 404. Bob's list is scoped by `owner_id` in SQL (store/seed_lots.go:68). The "alice still has access" guard prevents the test passing by breaking access for everyone ✓.
- **`?plantId=` validation** (`TestSeedLotCrudAPI`): `"0"`/`"-1"`/`"nope"` all hit `id < 1 || err != nil` → 400 (seed_lots.go:87-91) ✓.
- **Auth gating** (`TestSeedLotsRequireAuthAPI`): All five routes are under `h.requireAuth()` (api.go:181-186), which returns 401 (auth.go:126) before any binding runs ✓.
- **Helper existence**: `decodeMap`, `doJSON`, `authEngine`, `registerAndCookie`, `createGardenAPI`, `objectsPath`, `objectPlantingsPath` all exist in sibling `_test.go` files with the signatures the new file expects; no name collisions with `decodeList`/`createPlantAPI`/`seedLotPath`.
- **Bare-array decode**: `decodeList` correctly targets `[]any`; the store initializes `lots := []domain.SeedLot{}` (store/seed_lots.go:86) so an empty list serializes to `[]`, not `null` — the helper would not silently read zero.
All assertions match the actual production behavior; no logic bugs, wrong constants, or tests-passing-for-the-wrong-reason issues found.
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — No material issues found</summary>
VERDICT: No material issues found
- The new helpers (`decodeList`, `seedLotPath`, `createPlantAPI`) are small, well-named, and the only one with a non-obvious purpose (`decodeList`) is documented with the exact reason it differs from the existing `decodeMap` (bare array vs `{\"entries\":…}`). Verified `decodeMap` lives in `gardens_test.go:27` and returns `map[string]any`, so the bare-array case genuinely needs a separate helper rather than reusing it.
- `createPlantAPI` duplicates a plant-creation sequence that other test files presumably inline; it's local to this file and used 3× here, which is a fair consolidation point. Not worth extracting to shared helpers given the rest of the suite doesn't already share one.
- The tests follow the existing patterns in the package (`authEngine`/`localCfg`/`doJSON`/`registerAndCookie`/`decodeMap`), use the `*_API` test-name convention, and the table-driven subtests in the privacy/auth cases match the style used elsewhere.
Clean through the maintainability lens.
</details>
<details><summary><b>⚡ Performance</b> — No material issues found</summary>
All helpers resolve. This is a test-only PR adding a new test file. No production code changed, no hot paths, no queries, no allocations in production code. The test file itself is not performance-sensitive.
No material performance issues found — the PR is purely a test addition and my lens covers production efficiency, which is untouched here.
**VERDICT: No material issues found**
- No production code changed in this PR; it adds `internal/api/seed_lots_test.go` only. From a performance lens there is nothing to regress — no query, allocation, hot-loop, or blocking-path change to scrutinize.
- Verified that every helper the new test file calls (`decodeMap`, `doJSON`, `authEngine`, `registerAndCookie`, `createGardenAPI`, `objectsPath`, `objectPlantingsPath`, `localCfg`) resolves to an existing definition in the `internal/api` test helpers, so the file compiles against the checked-out repo.
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — ⚠️ could not complete</summary>
⚠️ This reviewer failed to complete: opencode run context deadline exceeded
</details>
</details>
<details><summary><b>opencode/kimi-k2.6:cloud</b> (opencode) — No material issues found</summary>
**Verdict: No material issues found · ⚠️ 2/5 lens(es) errored** — 5 reviewers: security, correctness, maintainability, performance, error-handling
<details><summary><b>🔒 Security</b> — No material issues found</summary>
**No material issues found**
This PR adds only test code (`internal/api/seed_lots_test.go`). No production code changed. Through the **security** lens:
- **Authn/authz:** The new tests correctly assert that unauthenticated callers get 401 (`TestSeedLotsRequireAuthAPI`) and that cross-user access returns 404 (existence masked, per project convention) rather than 403 (`TestSeedLotsArePrivateAPI`). These verify the existing `requireAuth` and service-level ACL enforcement, not weaken them.
- **Injection / SSRF / deserialization / secret leakage / unsafe defaults:** Not applicable — no handler, service, or store logic is modified. The test inputs are hardcoded literals and standard test helpers; no untrusted data is parsed unsafely or reflected into sensitive contexts.
- **Input validation:** The tests exercise the existing handler's validation (e.g., bad `plantId` filter → 400) and confirm it is enforced at the API boundary.
Nothing in this diff introduces or masks a security issue.
</details>
<details><summary><b>🎯 Correctness</b> — ⚠️ could not complete</summary>
⚠️ This reviewer failed to complete: opencode run context deadline exceeded
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — No material issues found</summary>
No material issues found
The new test file follows existing patterns rigorously:
- `seedLotPath` matches the camelCase `*Path` helper convention used across every other handler test (`journalPath`, `plantingPath`, `gardenPath`, etc.).
- `decodeList` is a clean, well-commented local helper that fills a real gap — the test suite has no shared array decoder, and existing list assertions inline their own `Unmarshal` because they do extra work (filtering, counting) that `decodeList` wouldn't cover. Keeping it local is appropriate.
- `createPlantAPI` mirrors the existing `createGardenAPI` in `objects_test.go`: a pragmatic, package-local helper that hides irrelevant required fields. It does not duplicate any existing symbol.
- Test structure is linear, names are consistent with the file (`TestSeedLotCrudAPI` parallels `TestJournalCrudAPI`), and comments explain *why* the tests exist rather than restating *what* they do.
- Imports are used, `any` vs `map[string]any` usage aligns with surrounding tests, and magic numbers are inline constants consistent with the rest of the API test suite.
Nothing in the readability, structure, duplication, or abstraction layers raises a concern.
</details>
<details><summary><b>⚡ Performance</b> — No material issues found</summary>
**No material issues found**
The PR adds only test code (`internal/api/seed_lots_test.go`) with no production changes. From a performance lens:
- The tests create a bounded, small number of resources per case (≤2 users, ≤2 plants, ≤2 lots, ≤2 plantings). No unbounded growth.
- No loops that amplify database round-trips. The only loops iterate over 3–5 hardcoded test cases (bad filter values, HTTP methods, routes), each issuing a single request.
- `authEngine` spins up a fresh in-memory database per test via `t.Cleanup`, so there's no cross-test state accumulation.
- The production paths exercised (`ListSeedLots` → `fillRemaining` → `SeedLotUsage`) batch lot-ID lookup into a single query with placeholders rather than N+1.
Nothing here that would regress CI runtime or resource usage.
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — ⚠️ could not complete</summary>
⚠️ This reviewer failed to complete: opencode run context deadline exceeded
</details>
</details>
</details>
<sub>Automated adversarial review by Gadfly — consensus across the model swarm. Advisory only — does not block merge.</sub>
- Cover the plantId filter's out-of-range branch (0, -1), not just the
non-numeric one — the handler rejects id < 1.
- Add the documented negative-remaining case: over-planting a lot (57 against
50 bought) drives remaining to -7 through the HTTP surface. The number is a
derived truth about what's committed, not a floor at zero, and that's the
signal a gardener wants.
- Rename objID → oid in the remaining test to match the package convention
(shares_test etc.).
Not taken: relocating createPlantAPI to plants_test.go — it's shared with #89's
branch and the move is cleanest once both land (consolidating with that branch's
makeFillPlant), noted on both PRs. The 409 "current" assertion style is
deliberate and reads clearly; matching journal_test.go's exact phrasing isn't
worth a divergence churn.
Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ
plantId out-of-range branch now covered (0, -1 → 400), not just non-numeric.
Negative-remaining case added: over-planting a lot (57 against 50 bought) drives remaining to -7 through the HTTP surface — the number is a derived truth about what's committed, not a floor at zero.
objID → oid to match the package convention.
Not taken, with reasons: relocating createPlantAPI to plants_test.go — it's shared with #89's branch and the consolidation is cleanest once both land (noted on both PRs); coupling the branches now to share a helper would be worse. The 409 current-assertion style is deliberate and reads clearly on its own.
@gadfly review
Findings addressed:
- **plantId out-of-range branch** now covered (0, -1 → 400), not just non-numeric.
- **Negative-remaining** case added: over-planting a lot (57 against 50 bought) drives `remaining` to -7 through the HTTP surface — the number is a derived truth about what's committed, not a floor at zero.
- **`objID` → `oid`** to match the package convention.
Not taken, with reasons: relocating `createPlantAPI` to `plants_test.go` — it's shared with #89's branch and the consolidation is cleanest once both land (noted on both PRs); coupling the branches now to share a helper would be worse. The 409 `current`-assertion style is deliberate and reads clearly on its own.
(Re-requesting after the CI infra recovery — earlier runs died on a docker.gitea.com outage. Current head has the plantId-boundary and negative-remaining coverage plus the objID→oid rename.)
@gadfly review
(Re-requesting after the CI infra recovery — earlier runs died on a `docker.gitea.com` outage. Current head has the plantId-boundary and negative-remaining coverage plus the `objID`→`oid` rename.)
Gadfly: I was wrong that no plantings-path helper existed — objectPlantingsPath
is defined in plantings_test.go in the same package. Use it for the two
planting POSTs in the remaining test.
Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ
The one finding: I was wrong that no plantings-path helper existed — objectPlantingsPath is defined in plantings_test.go in the same package. Both inline planting POSTs now use it.
@gadfly review
The one finding: I was wrong that no plantings-path helper existed — `objectPlantingsPath` is defined in `plantings_test.go` in the same package. Both inline planting POSTs now use it.
🪰Gadfly consensus review — 3 inline findings on changed lines. See the consensus comment for the full ranked summary.
Advisory only — does not block merge.
<!-- gadfly-inline-review -->
🪰 **Gadfly consensus review** — 3 inline findings on changed lines. See the consensus comment for the full ranked summary.
<sub>Advisory only — does not block merge.</sub>
⚪decodeList returns []any, diverging from the package's []map[string]any list-decoding convention
maintainability · flagged by 1 model
internal/api/seed_lots_test.go:19 — decodeList diverges from the established list-decoding convention. The package's other list-decoding sites decode into []map[string]any inline: gardens_test.go:56-58, plants_test.go:18, and shares_test.go:41-42. The new decodeList helper returns []any instead. It is only used for len() checks in this file, so it works, but it's a slightly leakier type and breaks the visual pattern a reader of this package expects. Either return `[]map[s…
🪰 Gadfly · advisory
⚪ **decodeList returns []any, diverging from the package's []map[string]any list-decoding convention**
_maintainability · flagged by 1 model_
- **`internal/api/seed_lots_test.go:19` — `decodeList` diverges from the established list-decoding convention.** The package's other list-decoding sites decode into `[]map[string]any` inline: `gardens_test.go:56-58`, `plants_test.go:18`, and `shares_test.go:41-42`. The new `decodeList` helper returns `[]any` instead. It is only used for `len()` checks in this file, so it works, but it's a slightly leakier type and breaks the visual pattern a reader of this package expects. Either return `[]map[s…
<sub>🪰 Gadfly · advisory</sub>
🟡createPlantAPI plant-creation helper is declared in seed_lots_test.go instead of plants_test.go, where custom-plant creation is currently inlined
maintainability · flagged by 1 model
internal/api/seed_lots_test.go:29 — createPlantAPI lives in the wrong file. A plant-creation helper is plant-domain code, but it's declared in seed_lots_test.go. plants_test.go currently inlines custom-plant creation (plants_test.go:46-47 and 85-86) with a near-identical payload rather than calling a shared helper, so this PR introduces a second, parallel pattern instead of promoting a shared one. Grep confirms no createPlant/createPlantAPI helper exists elsewhere; `plants_…
🪰 Gadfly · advisory
🟡 **createPlantAPI plant-creation helper is declared in seed_lots_test.go instead of plants_test.go, where custom-plant creation is currently inlined**
_maintainability · flagged by 1 model_
- **`internal/api/seed_lots_test.go:29` — `createPlantAPI` lives in the wrong file.** A plant-creation helper is plant-domain code, but it's declared in `seed_lots_test.go`. `plants_test.go` currently inlines custom-plant creation (`plants_test.go:46-47` and `85-86`) with a near-identical payload rather than calling a shared helper, so this PR introduces a second, parallel pattern instead of promoting a shared one. Grep confirms no `createPlant`/`createPlantAPI` helper exists elsewhere; `plants_…
<sub>🪰 Gadfly · advisory</sub>
⚪Missing status-code check before decoding response body after over-planting, masking the real error on failure
error-handling · flagged by 1 model
internal/api/seed_lots_test.go:192-195 — The final GET in TestSeedLotRemainingIsDerivedAPI decodes the response body without first checking w.Code == http.StatusOK, unlike every other GET/decode pair in this file (e.g. lines 173-176, 160-162). If this request fails (e.g. a 404/500 due to a regression), decodeMap (internal/api/gardens_test.go:26-34) will still unmarshal successfully since it only fails on invalid JSON, not missing keys — ["remaining"] will be absent, the assertion fal…
🪰 Gadfly · advisory
⚪ **Missing status-code check before decoding response body after over-planting, masking the real error on failure**
_error-handling · flagged by 1 model_
- `internal/api/seed_lots_test.go:192-195` — The final GET in `TestSeedLotRemainingIsDerivedAPI` decodes the response body without first checking `w.Code == http.StatusOK`, unlike every other GET/decode pair in this file (e.g. lines 173-176, 160-162). If this request fails (e.g. a 404/500 due to a regression), `decodeMap` (internal/api/gardens_test.go:26-34) will still unmarshal successfully since it only fails on invalid JSON, not missing keys — `["remaining"]` will be absent, the assertion fal…
<sub>🪰 Gadfly · advisory</sub>
steve
merged commit 48057fe1f3 into main2026-07-22 02:53:13 +00:00
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
Closes #83 (the seed-lots half;
/gardens/:id/yearsand/capabilitiesstill to do — see below)./seed-lotswas the only handler file without a sibling API test. That's the exact state PATCH/DELETE/journal/:idshipped in: implemented, unit-tested through the service, and completely unreachable, because service tests cannot see a route that was never registered or one registered with the wrong:param.What's covered
Four tests, chosen around what a broken route would silently take with it:
TestSeedLotCrudAPI— full lifecycle over HTTP: create, GET by id, list,?plantId=filter, a rejected non-numericplantId, PATCH with version bump, a stale-version 409 that carries the current row back so the client can rebase without a second request, DELETE, and 404 afterwards.TestSeedLotRemainingIsDerivedAPI— buys 50, plants 12 against the lot, expectsremaining: 38through the HTTP surface. DESIGN.md makes derivation load-bearing ("a decremented column drifts the moment a planting is edited behind its back") and this number is the whole reason anyone opens the seed shelf, so it's worth pinning at the boundary rather than only in the service.TestSeedLotsArePrivateAPI— lots are private to the buyer and deliberately never travel with a shared garden. Another user gets 404, not 403, on get/patch/delete, per the project's convention that no-access masks existence. Also asserts they don't leak into the other user's listing, and that the owner still has access afterwards (so the test can't pass by breaking access for everyone).TestSeedLotsRequireAuthAPI— all five routes 401 unauthenticated rather than returning an empty list.Verified the tests catch the thing
Unregistering
GET /seed-lots/:id— the journal failure mode — fails all four:Worth doing given how easily a test like this can pass for the wrong reason — the fixture in #72 is the cautionary case.
Note for reviewers
decodeListis new because seed lot listing returns a bare JSON array, unlike/journal's{"entries": …}. A helper that assumed an object would have decoded nothing and quietly reported zero lots — which several assertions here would then have "passed" on.Not in this PR
#83 also names
GET /gardens/:id/yearsandGET /capabilitiesas untested. Leaving those out deliberately:/capabilitiesis about to become dynamic in #79 and its test should be written against the new behaviour rather than rewritten immediately.No production code changed.
GOWORK=off go test ./...green;gofmt -l internal/clean.🤖 Generated with Claude Code
https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ
🪰 Gadfly — live review status
5/5 reviewers finished · updated 2026-07-22 02:30:23Z
claude-code/sonnet· claude-code — ✅ doneglm-5.2:cloud· ollama-cloud — ✅ donekimi-k2.6:cloud· ollama-cloud — ✅ doneopencode/glm-5.2:cloud· opencode — ✅ doneopencode/kimi-k2.6:cloud· opencode — ✅ doneLive status board. Findings are posted in each model's own comment. Advisory only — does not block merge.
🪰 Gadfly review — consensus across 5 models
Verdict: Minor issues · 3 findings (0 with multi-model agreement)
3 single-model findings (lower confidence)
internal/api/seed_lots_test.go:29internal/api/seed_lots_test.go:19internal/api/seed_lots_test.go:192Per-model detail
claude-code/sonnet (claude-code) — Minor issues
Verdict: Minor issues — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — No material issues found
Good — SourceURL is already validated at the service level (existing test
TestSourceURLValidationcovers scheme rejection), and this API test PR doesn't touch or weaken that. It's stored, not fetched server-side, so no SSRF surface. This PR is test-only and everything in it checks out against the real handlers.Review
VERDICT: No material issues found
Findings from a security-lens pass:
internal/api/seed_lots.go— every handler (listSeedLots,getSeedLot,updateSeedLot,deleteSeedLot) scopes tomustActor(c).ID, matching the "no-access is 404" conventionTestSeedLotsArePrivateAPIexercises.plantIdquery param is parsed withstrconv.ParseIntand rejected below 1 before use — no injection surface, consistent with the test'snope/0/-1→ 400 assertions.requireAuthperTestSeedLotsRequireAuthAPI's expectations by reading the handler file; no handler readsmustActoroptimistically or falls back to an unauthenticated path.SourceURL(present in the create/patch request structs but not exercised by this new API test) — it's validated server-side byvalidSourceURLininternal/service/seed_lots.goand already covered byTestSourceURLValidationin the service package (rejectsjavascript:etc.). Not fetched server-side, so no SSRF risk from an untested field here.registerAndCookie/authEnginetest helpers reuse the existing auth test scaffolding (auth_test.go) — cookies, CSRF guard, and password handling are unchanged and were already covered by prior tests; nothing in this diff weakens them.password123test fixture already used throughout the existing test suite.This PR adds only test code and exercises existing, already-secure production paths correctly. Nothing in my lane to flag.
🎯 Correctness — No material issues found
Route registration confirmed at
internal/api/api.go:181behindrequireAuth(), matching the test's assumptions. Everything in this diff checks out against the actual code.Review
VERDICT: No material issues found
This PR adds only test code (no production changes), and every behavioral claim the tests make matches what I verified by reading the actual handler/service/store code:
internal/api/seed_lots.go:88—plantIdfilter rejectsid < 1or unparseable, so"nope","0","-1"all correctly expect 400 (verified against the test's bad-value loop).internal/service/seed_lots.go:98-123(fillRemaining) —remaining = quantity - used, uncapped, matching the test's expectation of both38(50-12) and-7(50-57) after over-planting.usedis driven by the explicitcountfield when set (internal/service/seed_lots.go:113-115), so the test's literalcount: 12/count: 45payloads produce exactly the arithmetic asserted — I traced this throughstore/seed_lots.go'sSeedLotUsageSQL as well, which correctly returns rawcount/radius/spacingper active planting rather than pre-aggregating.internal/service/seed_lots.go:129-138(ownSeedLot) — cross-owner access returnsdomain.ErrNotFound, so Bob's get/patch/delete against Alice's lot are all 404s, matchingTestSeedLotsArePrivateAPI.internal/store/seed_lots.go:105-129(UpdateSeedLot) — a version mismatch fetches and returns the current row alongsideErrVersionConflict, andinternal/service/seed_lots.go:204-214fillsremainingonto that returned row before it reaches the client — matching the test's assertion that the 409 body'scurrent.vendorreflects the already-applied first patch.internal/api/api.go:181— the/seed-lotsgroup is registered underh.requireAuth(), matchingTestSeedLotsRequireAuthAPI.internal/service/plantings.go:285) only requires the plop center within the object's half-extents, so the test's(40,40)placement in a 100×100 bed is valid and won't spuriously fail the over-plant step.createGardenAPI,objectsPath,objectPlantingsPath,decodeMap,doJSON,authEngine,localCfg,registerAndCookie) all exist with the exact signatures the new file calls them with — no compile break.No logic bugs, no incorrect magic numbers, and the domain-specific assertions (derived-remaining arithmetic, ACL masking) are correct against the real implementation rather than just plausible-looking.
🧹 Code cleanliness & maintainability — No material issues found
All the helpers this new file relies on (
doJSON,decodeMap,authEngine,localCfg,registerAndCookie,createGardenAPI,objectsPath,objectPlantingsPath) already exist and are reused correctly, with no duplication introduced.createPlantAPIanddecodeListare genuinely new (nothing equivalent exists elsewhere), and the inline garden/bed-object creation inTestSeedLotRemainingIsDerivedAPImatches the same inline patternplantings_test.goalready uses (there's no existing "create bed" helper to have reused instead). Email-namespacing (alice-lots@…) follows the same per-file collision-avoidance convention seen ingardens_test.go/plants_test.go.VERDICT: No material issues found
Nothing in this diff raises cleanliness or maintainability concerns: the new test file follows the existing
internal/api/*_test.goconventions closely (table-driven sub-cases,t.Fatalf/t.Errorfusage,Pathhelper naming, doc comments explaining why a test exists), the one new shared helper (decodeList) is justified and documented (bare-array vs{"entries":…}response shape), and no code is duplicated that should have called an existing helper instead.⚡ Performance — No material issues found
VERDICT: No material issues found
This PR adds only test code (
internal/api/seed_lots_test.go) — no production paths, hot loops, or query patterns are touched. Through the performance lens specifically:forloop over a 3-element slice atinternal/api/seed_lots_test.go:94-98, another over 3 method cases at:218-230). No unbounded growth, no loop scaling with external input, no N+1 pattern.count: 12,count: 45) are trivial and don't stress any batch-insert path.decodeList/decodeMap/createPlantAPIare all O(1) or O(response size) JSON decodes.Nothing here would move the needle on CI time or resource usage in a way worth flagging under a performance lens. Correctness/test-design questions (e.g., whether the
remainingderivation assertions are appropriate) belong to other lenses, not this one.🧯 Error handling & edge cases — Minor issues
Confirmed against the actual code:
decodeMap(internal/api/gardens_test.go:26-34) just unmarshals into amap[string]anyand onlyt.Fatalfs on a JSON parse error — it never checks for specific keys. So on a non-200 error body like{"error": "..."}, it returns cleanly with"remaining"absent, the type assertion.(float64)fails,okis false, and line 193-194 falls through to a generict.Errorf("remaining after over-planting = %v, want -7 ...")with no status code. Every other GET/decode pair in the file (lines 173-176, 160-162, etc.) guards withif w.Code != http.StatusOK { t.Fatalf(...) }first — this one at line 192 does not. Finding confirmed.VERDICT: Minor issues
internal/api/seed_lots_test.go:192-195— The final GET inTestSeedLotRemainingIsDerivedAPIdecodes the response body without first checkingw.Code == http.StatusOK, unlike every other GET/decode pair in this file (e.g. lines 173-176, 160-162). If this request fails (e.g. a 404/500 due to a regression),decodeMap(internal/api/gardens_test.go:26-34) will still unmarshal successfully since it only fails on invalid JSON, not missing keys —["remaining"]will be absent, the assertion falls through to!ok, and the failure reports"remaining after over-planting = <nil>, want -7"with no status code or error body surfaced. That masks the actual failure (an HTTP error) behind a generic "wrong value" message. Suggested fix: add the samew.Code != http.StatusOKguard used at line 174 before decoding.glm-5.2:cloud (ollama-cloud) — Minor issues
Verdict: Minor issues · ⚠️ 3/5 lens(es) errored — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — ⚠️ could not complete
⚠️ This reviewer failed to complete: agent: step 6: all chain targets failed
ollama-cloud/glm-5.2☁️ ollama-cloud/glm-5.2☁️ HTTP 429: too many concurrent requests
🎯 Correctness — ⚠️ could not complete
⚠️ This reviewer failed to complete: agent: step 7: all chain targets failed
ollama-cloud/glm-5.2☁️ ollama-cloud/glm-5.2☁️ HTTP 429: too many concurrent requests
🧹 Code cleanliness & maintainability — Minor issues
Both findings verified against the actual code.
Verdict: Minor issues
internal/api/seed_lots_test.go:19—decodeListdiverges from the established list-decoding convention. The package's other list-decoding sites decode into[]map[string]anyinline:gardens_test.go:56-58,plants_test.go:18, andshares_test.go:41-42. The newdecodeListhelper returns[]anyinstead. It is only used forlen()checks in this file, so it works, but it's a slightly leakier type and breaks the visual pattern a reader of this package expects. Either return[]map[string]anyto match, or keep[]anyas a deliberate minor inconsistency.internal/api/seed_lots_test.go:29—createPlantAPIlives in the wrong file. A plant-creation helper is plant-domain code, but it's declared inseed_lots_test.go.plants_test.gocurrently inlines custom-plant creation (plants_test.go:46-47and85-86) with a near-identical payload rather than calling a shared helper, so this PR introduces a second, parallel pattern instead of promoting a shared one. Grep confirms nocreatePlant/createPlantAPIhelper exists elsewhere;plants_test.goinlines the POST. Low-churn fix: movecreatePlantAPIintoplants_test.go.No dead code, no deep nesting, and the four test functions follow the same structure and comment style as
TestJournalCrudAPI, the closest sibling.⚡ Performance — No material issues found
VERDICT: No material issues found
This is a test-only PR (new
internal/api/seed_lots_test.go, no production code changed). Through the performance lens:decodeList,createPlantAPI) do a single request/decode each and are called a handful of times per test — no hot loops, no unbounded growth.decodeListunmarshals into[]anyand only checkslen(); nothing pathological.authEngine/localCfgpattern — that's the established convention across the sibling*_test.gofiles, not a regression this PR introduces.listSeedLots,getSeedLot, etc.) are unchanged; the?plantId=filter path uses a single scoped query, no N+1.No performance issues in this PR's scope.
🧯 Error handling & edge cases — ⚠️ could not complete
⚠️ This reviewer failed to complete: agent: step 6: all chain targets failed
ollama-cloud/glm-5.2☁️ ollama-cloud/glm-5.2☁️ HTTP 429: too many concurrent requests
kimi-k2.6:cloud (ollama-cloud) — No material issues found
Verdict: No material issues found · ⚠️ 2/5 lens(es) errored — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — No material issues found
No material issues found
This is a pure test-addition PR with no production code changes. From a security lens, the new tests actually improve coverage of authn/authz behavior rather than weaken anything.
TestSeedLotsRequireAuthAPI(l. 247) correctly verifies that all five routes return 401 when called without a session cookie, ensuring therequireAuthmiddleware is wired.TestSeedLotsArePrivateAPI(l. 201) verifies cross-user isolation: Bob gets 404 (not 403) on Alice’s lot, consistent with the project’s “mask existence” convention, and confirms the lot doesn’t leak into Bob’s listing.TestSeedLotCrudAPI(l. 50) exercises input validation by asserting that malformedplantIdfilters ("nope","0","-1") return 400.There are no hardcoded secrets, no unsafe deserialization of untrusted data (all inputs are hardcoded test fixtures), no SSRF vectors, and no injection points. The test database is in-memory (
:memory:) and closed viat.Cleanup, so there is no cross-test state leakage.🎯 Correctness — No material issues found
No material issues found.
All four tests correctly exercise the HTTP surface for
/seed-lots, and the assertions match the verified production behavior:internal/api/api.go:181-186):GET /:id,PATCH /:id,DELETE /:id,GETlist, andPOSTare all registered behindrequireAuth(), matching the five routes tested inTestSeedLotsRequireAuthAPI.internal/api/seed_lots.go:129-153,internal/api/errors.go:66,internal/service/seed_lots.go:193-224): OnErrVersionConflict, the handler callswriteVersionConflict(c, lot), and the service pre-fillsupdatedwith remaining before returning it. The test’s assertion thatcurrent.vendor == "Fedco"is therefore correct.remaining(internal/service/seed_lots.go:98-123):fillRemaininguses explicitCountwhen present (the planting body here sets"count": 12and"count": 45), so the arithmetic50 - 12 = 38and50 - 57 = -7is exact. The test correctly expects negative values to surface unclamped.internal/service/seed_lots.go:129-138):ownSeedLotreturnsdomain.ErrNotFoundfor non-owners; the handler maps that to 404, matching the test’s expectation that Bob gets 404 on get/patch/delete and an empty listing.plantIdfilter (internal/api/seed_lots.go:84-100): The handler rejects both non-numeric strings ("nope") and out-of-range IDs ("0","-1") with 400, exactly as the test asserts.doJSON,decodeMap,registerAndCookie,authEngine,localCfg,createGardenAPI,objectsPath, andobjectPlantingsPathall resolve in sibling_test.gofiles.The only arithmetic involves small integers represented exactly as
float64after JSON unmarshaling, so comparisons likerem != 38andversion != old+1are safe.🧹 Code cleanliness & maintainability — No material issues found
No material issues found
The new test file follows the package's existing patterns closely:
seedLotPath,decodeList, andcreatePlantAPIare small, well-named helpers witht.Helper()where appropriate.TestSeedLotCrudAPI,TestSeedLotRemainingIsDerivedAPI,TestSeedLotsArePrivateAPI, andTestSeedLotsRequireAuthAPImirror the structure and naming conventions of sibling API tests (TestJournalCrudAPI,TestGardenCRUDFlow,TestPlantVisibilityAcrossUsersAPI, etc.).decodeMap,doJSON,registerAndCookie,createGardenAPI,objectsPath,objectPlantingsPath) are reused rather than redefined.decodeListhelper is justified by the PR's note about bare-array responses, and its[]anyreturn is honest given it's only used for length checks.createPlantAPIis local to this file and centralizes four identical calls; it doesn't need to be hoisted toplants_test.gosince no other file currently needs it.Nothing in the change duplicates production logic, introduces leaky abstractions, or departs from surrounding style in a way that would impair future maintenance.
⚡ Performance — ⚠️ could not complete
⚠️ This reviewer failed to complete: agent: step 0: all chain targets failed
ollama-cloud/kimi-k2.6☁️ skipped (backed off until 02:18:14.876)
🧯 Error handling & edge cases — ⚠️ could not complete
⚠️ This reviewer failed to complete: agent: step 0: all chain targets failed
ollama-cloud/kimi-k2.6☁️ ollama-cloud/kimi-k2.6☁️ HTTP 429: too many concurrent requests
opencode/glm-5.2:cloud (opencode) — No material issues found
Verdict: No material issues found · ⚠️ 1/5 lens(es) errored — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — No material issues found
This is a test-only PR (new file
internal/api/seed_lots_test.go, no production code touched). I reviewed it through the security lens — authn/authz, privacy/masking, input validation — verifying the tests against the actual handlers and service.Verified:
seedLotsgroup is registered behindh.requireAuth()atinternal/api/api.go:181, so theTestSeedLotsRequireAuthAPI401 assertions cover all five routes correctly.ownSeedLot(internal/service/seed_lots.go:129-138) returnsdomain.ErrNotFoundfor a non-owner, masking existence — matching theTestSeedLotsArePrivateAPI404 assertions (not 403), andListSeedLotsForOwnerscopes by owner so bob's listing-empty assertion holds.listSeedLots(internal/api/seed_lots.go:84-100) validatesplantIdquery withParseIntandid < 1rejection → 400; the test'snope/0/-1cases line up.doJSON/registerAndCookie/authEnginehelpers exist as referenced; cookies are threaded correctly (bob uses bob's cookie, unauth usesnil).VERDICT: No material issues found
The tests genuinely exercise the security-relevant properties of the route group (require-auth 401, owner-only access with existence masking via 404, no cross-user listing leakage, post-access self-check that the owner isn't locked out). No production code changed, so no new vulnerabilities are introduced, and the assertions correctly reflect the existing auth/ACL behavior in the handlers and service. Nothing in the security lane is materially wrong.
🎯 Correctness — No material issues found
Now let me check the
countvalidation: does the service validate that count is positive / handle 12 vs derived? The test usescount: 12. Let me check derivedCount and how count 12 is stored.Now I have a good understanding. The test's expected
remainingcalculation relies oncountbeing stored explicitly (12 and 45), which overrides the derived formula. Let me verify the over-planting: 50 - 12 - 45 = -7. Correct.Let me check the
countfield on the planting create request — it's*int. The test passescount: 12andcount: 45as numbers. JSON decode to*intworks.Now let me check the
radiusCm: 20— but since count is set explicitly, radius doesn't matter for the remaining calc. Good.Let me verify the
?plantId=filter behavior. The handler rejectsid < 1. Test checks "0", "-1", "nope" → 400. Good.Let me check the version conflict body shape: writeVersionConflict wraps under
current, and the test checksdecodeMap(...)["current"]. Good.Let me double-check one subtle thing — the stale version PATCH uses
lot["version"](the original), but the first PATCH already bumped it. So the second PATCH with the original version should conflict. Let me verify the store's version check logic.The logic is sound. Now let me verify the
decodeMap/doJSON/authEnginehelpers exist and behave as the test assumes.Now let me verify
registerAndCookieanddecodeMap:Now let me check
createGardenAPIandobjectsPathandobjectPlantingsPath:All helpers verified. Now let me check the plant create request — does it accept
category,color,iconas the test provides? Let me verify the plants handler.Now, an important check: the
categoryfield. The test uses"category": "vegetable". Let me verify valid categories in the service.Good. Now let me check the plant creation: does it validate
spacingCmminimum? The test uses 15, 10, 45 — all reasonable. Let me verify theicon"🌱" is fine (UTF-8).Now let me verify the
objectPlantingsPathand the bed creation request. The test creates a bed withkind: "bed",widthCm: 100,heightCm: 100,plantable: true. Let me check the object create request fields.Now let me check whether the planting create with
count: 12will actually be accepted. TheCountfield is*int. JSON12decodes to*intpointing to 12. Good. Let me check the service validates count > 0.Now let me verify the
ErrNotFound→ 404 path for the privacy test. Bob patches Alice's lot:ownSeedLotchecksl.OwnerID != actorID→ returnsErrNotFound→writeServiceError→ 404. Correct. Same for delete. Good.Now the seed lot create: the test sends
costCents: 495as a number, but the request struct hasCostCents *intwithjson:"costCents". JSON 495 decodes to*intpointing to 495. Good. AndpackedForYear: 2026→*int. Good.Now let me check one subtle thing in the over-plant test:
radiusCm: 20, plant spacing 10. Since count is explicitly set to 45, the derived formula is bypassed. So used = 12 + 45 = 57. remaining = 50 - 57 = -7. Correct.But wait — the test's first planting uses
count: 12. ThefillRemaininguses*u.Countwhen not nil. So used = 12. remaining = 50 - 12 = 38. Correct.Now let me double check the
?plantId=filter behavior — the handler rejectsid < 1. Test checks "0", "-1", "nope" → 400. Good. The store'sListSeedLotsForOwnerfilters by plantID when not nil.Let me check whether the
decodeListis robust — it unmarshals into[]any. A bare array works. But what if the list returnsnull? The handler doesc.JSON(http.StatusOK, lots)where lots is[]domain.SeedLot. If empty, gin serializes as[]. If nil... let me check ListSeedLots.Good — initialized as
[]domain.SeedLot{}, so empty serializes to[], notnull.decodeListhandles that fine.Now let me check the
remainingfield serialization — is itjson:"remaining"? Let me check the domain struct.All field names match. The test reads
remainingasfloat64. JSON numbers decode tofloat64inmap[string]any. Correct.Now let me verify the auth test's expected behavior for POST /seed-lots. The test sends
nilbody.doJSONwithnilbody →encodeBodyreturns empty buffer (no body). TherequireAuthmiddleware checks for cookie → 401. But wait — doesrequireAuthrun before body binding? Yes, middleware runs first. So 401. Good.But there's a subtlety: for POST without a body, does gin's
ShouldBindJSONeven get reached? No —requireAuthreturns 401 and aborts. Good.Now, one more check: the
TestSeedLotsRequireAuthAPIchecks POST/api/v1/seed-lotswithnilbody.doJSONencodesnil→ empty body. The request hasContent-Type: application/jsonbut no body.requireAuthshould 401 before binding. Good.Let me verify
requireAuthactually returns 401 (not 403 or something).All good. Now let me verify one more thing about the CRUD test: after the stale-version PATCH, the test does DELETE, then GET expecting 404. The DELETE —
DeleteSeedLotcallsownSeedLotfirst. The lot still exists (the stale patch didn't modify it). So delete succeeds → 204. Then GET → ownSeedLot → store returns ErrNotFound → 404. Good.Wait — one subtle concern: after the first successful PATCH (vendor→Fedco, version bumped), the test then does a stale PATCH with
lot["version"](original). This should fail with 409, and thecurrentshould have vendor "Fedco" and version = original+1. The test checkscur["vendor"] != "Fedco". Good.But the test does NOT update
lot["version"]after the first PATCH — it uses the originallotmap's version for the stale test. That's intentional (it's stale). Good. And then DELETE uses no version. Good.Let me now check whether there's a subtle issue with the
versioncomparison in the test:lot["version"]is from the create response,updated["version"]is from the patch response. Create returns version (likely 1), patch returns version 2. So2 != 1+1is false → passes. Good.Let me verify the create response actually includes
version. Yes, the domain struct hasVersion int64 json:"version". Good.Now let me run a quick sanity on the test file compiling — check imports are all used and available. The test imports
encoding/json,net/http,strconv,testing,gin. All used:json.Unmarshal(decodeList),http.Status*,strconv.FormatInt,testing,gin.Engine. Good.Let me verify there are no naming collisions with existing helpers.
decodeList,createPlantAPI,seedLotPath— are any of these already defined?No collisions — only defined in the new file. Good.
Let me verify the
versionfield is present in create response. Actually I already confirmed the domain struct serializes it. Now let me check theseedLotCreateRequestvalidation:PlantIDisbinding:"required"andUnitisbinding:"required". The test's create sends both. The remaining-derivation test sendsplantIdandunitbut novendor/sku/etc. — those are optional. Good.Let me now verify one potential issue: the
TestSeedLotRemainingIsDerivedAPIcreates a bed withwidthCm: 100, heightCm: 100. The first planting is atxCm: 0, yCm: 0(center) with radius 20. Bounds check:|0| > 100/2=50? No. Good. The second planting atxCm: 40, yCm: 40with radius 20. Bounds:|40| > 50? No. Good. Both pass.Now let me double-check: does the bed need
plantable: true? The test sets it.finalizePlantingdoesn't check plantable — that's checked inCreatePlantingat line 84:if !o.Plantable { return ErrInvalidInput }. So the test correctly setsplantable: true. Good.Let me verify the
objectPlantingsPath:objectPlantingsPath(oid)=/api/v1/objects/{oid}/plantings. The route is registered for POST. Let me confirm.Good —
POST /objects/:id/plantingsis registered. The path matches.Now let me verify the objects group is behind requireAuth (so the planting creation requires auth, but the test provides a cookie):
All verified. Now let me consider whether there are any correctness issues in the test itself that would make it pass for the wrong reason or fail to catch a real bug.
One thing to scrutinize: the
TestSeedLotRemainingIsDerivedAPI— does it actually verify the derivation is dynamic? It creates a lot (qty 50), plants 12, checks remaining=38. Then plants 45 more, checks remaining=-7. This does verify dynamic recomputation across two states. Good. But does it verify that editing/removing a planting updates remaining? No — the comment mentions "edited behind its back" but the test only adds. However, that's a scope choice, not a correctness bug. The test does verify derivation through the HTTP surface, which is its stated goal.Let me reconsider the
?plantId=0and-1case. The handler:"0"→ ParseInt succeeds, id=0,0 < 1true → 400."-1"→ ParseInt succeeds, id=-1,-1 < 1true → 400."nope"→ ParseInt fails → 400. All correct.Now, one thing I want to verify: the
TestSeedLotsArePrivateAPIpatch case sends"version": 1. Bob patches Alice's lot. The handler callsparseIDParam(succeeds, lot exists at id), thenShouldBindJSON(succeeds, version=1), thensvc.UpdateSeedLot(bob, lotID, patch, 1). The service callsownSeedLot(bob, lotID)→GetSeedLotreturns Alice's lot →l.OwnerID != bob→ErrNotFound→ 404. Good. The version is never checked because ownership fails first. Correct.Now let me check the
decodeMap(...)["current"]type assertion in the CRUD test:writeVersionConflictwritesgin.H{"error": ..., "current": lot}wherelotis*domain.SeedLot. Gin serializes a struct pointer as a JSON object. Socurrentdecodes tomap[string]any. Good.cur["vendor"]= "Fedco" (the updated value from the successful first PATCH). Correct.I've thoroughly verified the test logic against the production code. Let me do one final check — the
TestSeedLotCrudAPIcheckslot["unit"] != "seeds". The create sends"unit": "seeds". The domain struct serializesUnit string json:"unit". Solot["unit"]= "seeds". Good. Andlot["vendor"]= "Johnny's". Good.Everything checks out. Let me verify there are no compile issues by checking that
createPlantAPI's type assertiondecodeMap(...)["id"].(float64)is safe — id isint64serialized as JSON number → decodes tofloat64. Good.Let me also confirm the test for
TestSeedLotsRequireAuthAPIPATCH/DELETE on/seed-lots/1— these go throughrequireAuth→ 401. But do the routesPATCH /:idandDELETE /:idmatch/seed-lots/1? Yes, registered at lines 185-186. Good.I've completed a thorough correctness review. The test file is well-constructed and all assertions match the actual production code behavior. The derived
remainingcalculations (38 and -7) are arithmetically correct, the version-conflict semantics are correctly asserted, the privacy/404 convention is correctly enforced, and all helpers and routes exist as the test assumes.One last thing — let me double-check the
over-plantcreates a second planting with the same lot. Is there any constraint preventing multiple plantings against one lot? No —checkSeedLotForPlantingonly validates the lot belongs to the actor and matches the plant. Multiple plantings accumulate inSeedLotUsage. Good.The test's quantity values (100, 50, 10) are well within bounds. Good.
My review is complete. No correctness issues found.
VERDICT: No material issues found
I verified the new test file against the production code it exercises, re-deriving every constant and assertion from first principles:
remainingarithmetic (TestSeedLotRemainingIsDerivedAPI): Re-derived fromfillRemaining(seed_lots.go:118-121:Remaining = Quantity - Used, whereUsedsums each planting's explicitCountwhen non-nil, elsederivedCount). 50 bought − 12 planted = 38 ✓; 50 − (12+45) = −7 ✓. The negative floor is not clamped, matching the documented intent (seed_lots.go:94-97). The explicitcountoverrides the π·r²/spacing² formula, soradiusCm: 20/ spacing 10 are irrelevant to the result — confirmed at seed_lots.go:112-116.TestSeedLotCrudAPI): Re-traced throughstore.UpdateSeedLot(store/seed_lots.go:113WHERE id=? AND version=?), theErrVersionConflictpath returns the current row (store/seed_lots.go:119-124), the service fills itsremaining(seed_lots.go:209-213), andwriteVersionConflict(errors.go:66-71) wraps it under"current". The stale PATCH uses the originallot["version"]after a successful bump — correctly stale → 409 withvendor: "Fedco"incurrent✓.TestSeedLotsArePrivateAPI): Re-tracedownSeedLot(seed_lots.go:129-138) →l.OwnerID != actorID→ErrNotFound→writeServiceError(errors.go:26-27) → 404. Bob's list is scoped byowner_idin SQL (store/seed_lots.go:68). The "alice still has access" guard prevents the test passing by breaking access for everyone ✓.?plantId=validation (TestSeedLotCrudAPI):"0"/"-1"/"nope"all hitid < 1 || err != nil→ 400 (seed_lots.go:87-91) ✓.TestSeedLotsRequireAuthAPI): All five routes are underh.requireAuth()(api.go:181-186), which returns 401 (auth.go:126) before any binding runs ✓.decodeMap,doJSON,authEngine,registerAndCookie,createGardenAPI,objectsPath,objectPlantingsPathall exist in sibling_test.gofiles with the signatures the new file expects; no name collisions withdecodeList/createPlantAPI/seedLotPath.decodeListcorrectly targets[]any; the store initializeslots := []domain.SeedLot{}(store/seed_lots.go:86) so an empty list serializes to[], notnull— the helper would not silently read zero.All assertions match the actual production behavior; no logic bugs, wrong constants, or tests-passing-for-the-wrong-reason issues found.
🧹 Code cleanliness & maintainability — No material issues found
VERDICT: No material issues found
decodeList,seedLotPath,createPlantAPI) are small, well-named, and the only one with a non-obvious purpose (decodeList) is documented with the exact reason it differs from the existingdecodeMap(bare array vs{\"entries\":…}). VerifieddecodeMaplives ingardens_test.go:27and returnsmap[string]any, so the bare-array case genuinely needs a separate helper rather than reusing it.createPlantAPIduplicates a plant-creation sequence that other test files presumably inline; it's local to this file and used 3× here, which is a fair consolidation point. Not worth extracting to shared helpers given the rest of the suite doesn't already share one.authEngine/localCfg/doJSON/registerAndCookie/decodeMap), use the*_APItest-name convention, and the table-driven subtests in the privacy/auth cases match the style used elsewhere.Clean through the maintainability lens.
⚡ Performance — No material issues found
All helpers resolve. This is a test-only PR adding a new test file. No production code changed, no hot paths, no queries, no allocations in production code. The test file itself is not performance-sensitive.
No material performance issues found — the PR is purely a test addition and my lens covers production efficiency, which is untouched here.
VERDICT: No material issues found
internal/api/seed_lots_test.goonly. From a performance lens there is nothing to regress — no query, allocation, hot-loop, or blocking-path change to scrutinize.decodeMap,doJSON,authEngine,registerAndCookie,createGardenAPI,objectsPath,objectPlantingsPath,localCfg) resolves to an existing definition in theinternal/apitest helpers, so the file compiles against the checked-out repo.🧯 Error handling & edge cases — ⚠️ could not complete
⚠️ This reviewer failed to complete: opencode run context deadline exceeded
opencode/kimi-k2.6:cloud (opencode) — No material issues found
Verdict: No material issues found · ⚠️ 2/5 lens(es) errored — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — No material issues found
No material issues found
This PR adds only test code (
internal/api/seed_lots_test.go). No production code changed. Through the security lens:TestSeedLotsRequireAuthAPI) and that cross-user access returns 404 (existence masked, per project convention) rather than 403 (TestSeedLotsArePrivateAPI). These verify the existingrequireAuthand service-level ACL enforcement, not weaken them.plantIdfilter → 400) and confirm it is enforced at the API boundary.Nothing in this diff introduces or masks a security issue.
🎯 Correctness — ⚠️ could not complete
⚠️ This reviewer failed to complete: opencode run context deadline exceeded
🧹 Code cleanliness & maintainability — No material issues found
No material issues found
The new test file follows existing patterns rigorously:
seedLotPathmatches the camelCase*Pathhelper convention used across every other handler test (journalPath,plantingPath,gardenPath, etc.).decodeListis a clean, well-commented local helper that fills a real gap — the test suite has no shared array decoder, and existing list assertions inline their ownUnmarshalbecause they do extra work (filtering, counting) thatdecodeListwouldn't cover. Keeping it local is appropriate.createPlantAPImirrors the existingcreateGardenAPIinobjects_test.go: a pragmatic, package-local helper that hides irrelevant required fields. It does not duplicate any existing symbol.TestSeedLotCrudAPIparallelsTestJournalCrudAPI), and comments explain why the tests exist rather than restating what they do.anyvsmap[string]anyusage aligns with surrounding tests, and magic numbers are inline constants consistent with the rest of the API test suite.Nothing in the readability, structure, duplication, or abstraction layers raises a concern.
⚡ Performance — No material issues found
No material issues found
The PR adds only test code (
internal/api/seed_lots_test.go) with no production changes. From a performance lens:authEnginespins up a fresh in-memory database per test viat.Cleanup, so there's no cross-test state accumulation.ListSeedLots→fillRemaining→SeedLotUsage) batch lot-ID lookup into a single query with placeholders rather than N+1.Nothing here that would regress CI runtime or resource usage.
🧯 Error handling & edge cases — ⚠️ could not complete
⚠️ This reviewer failed to complete: opencode run context deadline exceeded
Automated adversarial review by Gadfly — consensus across the model swarm. Advisory only — does not block merge.
@gadfly review
Findings addressed:
remainingto -7 through the HTTP surface — the number is a derived truth about what's committed, not a floor at zero.objID→oidto match the package convention.Not taken, with reasons: relocating
createPlantAPItoplants_test.go— it's shared with #89's branch and the consolidation is cleanest once both land (noted on both PRs); coupling the branches now to share a helper would be worse. The 409current-assertion style is deliberate and reads clearly on its own.@gadfly review
(Re-requesting after the CI infra recovery — earlier runs died on a
docker.gitea.comoutage. Current head has the plantId-boundary and negative-remaining coverage plus theobjID→oidrename.)@gadfly review
The one finding: I was wrong that no plantings-path helper existed —
objectPlantingsPathis defined inplantings_test.goin the same package. Both inline planting POSTs now use it.🪰 Gadfly consensus review — 3 inline findings on changed lines. See the consensus comment for the full ranked summary.
Advisory only — does not block merge.
@@ -0,0 +16,4 @@// decodeList decodes a bare JSON array body. Seed lot listing returns the array// directly rather than wrapping it (unlike /journal's {"entries": …}), so a// helper that assumed an object would quietly read nothing.func decodeList(t *testing.T, body []byte) []any {⚪ decodeList returns []any, diverging from the package's []map[string]any list-decoding convention
maintainability · flagged by 1 model
internal/api/seed_lots_test.go:19—decodeListdiverges from the established list-decoding convention. The package's other list-decoding sites decode into[]map[string]anyinline:gardens_test.go:56-58,plants_test.go:18, andshares_test.go:41-42. The newdecodeListhelper returns[]anyinstead. It is only used forlen()checks in this file, so it works, but it's a slightly leakier type and breaks the visual pattern a reader of this package expects. Either return `[]map[s…🪰 Gadfly · advisory
@@ -0,0 +26,4 @@}// createPlantAPI makes a custom plant and returns its id.func createPlantAPI(t *testing.T, r *gin.Engine, cookie *http.Cookie, name string, spacing float64) int64 {🟡 createPlantAPI plant-creation helper is declared in seed_lots_test.go instead of plants_test.go, where custom-plant creation is currently inlined
maintainability · flagged by 1 model
internal/api/seed_lots_test.go:29—createPlantAPIlives in the wrong file. A plant-creation helper is plant-domain code, but it's declared inseed_lots_test.go.plants_test.gocurrently inlines custom-plant creation (plants_test.go:46-47and85-86) with a near-identical payload rather than calling a shared helper, so this PR introduces a second, parallel pattern instead of promoting a shared one. Grep confirms nocreatePlant/createPlantAPIhelper exists elsewhere; `plants_…🪰 Gadfly · advisory
@@ -0,0 +189,4 @@if w.Code != http.StatusCreated {t.Fatalf("over-plant: status %d, body %s", w.Code, w.Body.String())}w = doJSON(t, r, http.MethodGet, seedLotPath(lotID), nil, cookie)⚪ Missing status-code check before decoding response body after over-planting, masking the real error on failure
error-handling · flagged by 1 model
internal/api/seed_lots_test.go:192-195— The final GET inTestSeedLotRemainingIsDerivedAPIdecodes the response body without first checkingw.Code == http.StatusOK, unlike every other GET/decode pair in this file (e.g. lines 173-176, 160-162). If this request fails (e.g. a 404/500 due to a regression),decodeMap(internal/api/gardens_test.go:26-34) will still unmarshal successfully since it only fails on invalid JSON, not missing keys —["remaining"]will be absent, the assertion fal…🪰 Gadfly · advisory