Go module gitea.stevedudenhoeffer.com/steve/pansy, pure-Go deps (builds with CGO_ENABLED=0).
internal/config: env loader for all PANSY_* vars with defaults (auth values plumbed for #4/#5).
internal/store: modernc SQLite Open (WAL + busy_timeout=5000 + foreign_keys via per-connection DSN pragmas; :memory: pinned to one conn), embedded numbered-migration runner (idempotent, tracked in schema_migrations).
internal/store/migrations/0001_init.sql: full initial schema — users, sessions, gardens, garden_shares, garden_objects, plants, plantings. cm everywhere, version on user-mutable rows, CHECK-enforced enums, FK indexes, reserved shape='polygon' + points.
TanStack Router with all five routes (/login, /register, /gardens, /gardens/$gardenId, /plants) as stubs; TanStack Query provider; zod.
web/src/lib/api.ts: typed /api/v1 fetch wrapper throwing ApiError that carries HTTP status + parsed body (so the optimistic editor can read 409 conflict rows).
Dev proxy /api → :8080; responsive app shell + mobile viewport meta.
internal/webdist: //go:embed all:dist with a committed placeholder index.html.
internal/api/spa.go: SPA fallback — real files served (hashed assets immutable, index.htmlno-cache), client deep links fall back to index.html, unmatched /api/* and missing assets return JSON 404.
CGO_ENABLED=0 go build ./..., go vet ./..., go test ./... all clean (store + SPA tests included).
Binary migrates a fresh DB, second run is a no-op; healthz returns {"ok":true}; all seven tables present with FK enforcement.
tsc --noEmit clean; npm run build emits hashed dist/.
Integrated binary drives correctly in a browser: SPA mounts, / redirects to /gardens, all routes navigate client-side, /gardens/$gardenId resolves the param, deep links + cache headers verified, renders at desktop and 375px.
Implements the Pansy v1 scaffold (epic #20, phase 0).
#1 Backend: Go module (pure-Go, builds with CGO_ENABLED=0), env config,
gin server with slog + recovery + /api/v1/healthz, modernc SQLite store
with WAL/busy_timeout/foreign_keys pragmas, embedded numbered-migration
runner, full initial schema (users, sessions, gardens, garden_shares,
garden_objects, plants, plantings), domain structs + sentinel errors,
graceful shutdown.
#2 Frontend: Vite + React 19 + TS (strict) + Tailwind 4 + TanStack
Router/Query, typed /api/v1 fetch wrapper (ApiError carries status +
body so later issues can read 409 conflict rows), dev proxy /api -> :8080,
responsive app shell with stub pages for all five routes.
#3 Single binary: //go:embed of the web build with a committed placeholder,
SPA fallback (deep links, immutable asset caching, JSON 404 for unmatched
/api and missing assets), Makefile (web/build/dev/test), README quickstart
+ env var table.
Verified: go build/vet/test clean (CGO off); binary migrates idempotently and
serves healthz; web tsc + build clean; integrated binary serves the SPA, deep
links, and correct cache headers; app mounts and navigates all five routes at
desktop and 375px widths.
Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01JdQpdYYsTgtkJBxbcpAszi
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-18 19:29:13Z
#### `claude-code/sonnet` · claude-code — ✅ done
- ✅ **security** — No material issues found
- ✅ **correctness** — No material issues found
- ✅ **maintainability** — Minor issues
- ✅ **performance** — No material issues found
- ✅ **error-handling** — Minor issues
#### `glm-5.2:cloud` · ollama-cloud — ✅ done
- ✅ **security** — No material issues found
- ✅ **correctness** — No material issues found
- ✅ **maintainability** — Minor issues
- ✅ **performance** — No material issues found
- ✅ **error-handling** — Minor issues
#### `kimi-k2.6:cloud` · ollama-cloud — ✅ done
- ✅ **security** — No material issues found
- ✅ **correctness** — Minor issues
- ✅ **maintainability** — No material issues found
- ✅ **performance** — No material issues found
- ✅ **error-handling** — Minor issues
#### `opencode/glm-5.2:cloud` · opencode — ✅ done
- ✅ **security** — No material issues found
- ✅ **correctness** — Minor issues
- ✅ **maintainability** — Minor issues
- ✅ **performance** — Minor issues
- ✅ **error-handling** — Minor issues
#### `opencode/kimi-k2.6:cloud` · opencode — ✅ done
- ✅ **security** — No material issues found
- ✅ **correctness** — Blocking issues found
- ✅ **maintainability** — Minor issues
- ✅ **performance** — No material issues found
- ✅ **error-handling** — Minor issues
<sub>Live status board. Findings are posted in each model's own comment. Advisory only — does not block merge.</sub>
Verdict: Blocking issues found · 14 findings (4 with multi-model agreement)
Finding
Where
Models
Lens
🔴
buildDSN truncates filenames containing ? by treating them as URI query strings
internal/store/sqlite.go:63
4/5
correctness, error-handling
🟠
users schema allows partial OIDC identity (issuer without subject or vice versa)
internal/store/migrations/0001_init.sql:25
2/5
correctness
🟠
parseBody's res.text() runs outside apiFetch's try/catch, so a body-read failure rejects with a raw TypeError instead of an ApiError, breaking the wrapper's status/body contract that the optimistic editor depends on
web/src/lib/api.ts:108
2/5
error-handling
🟡
writeAPIError (standard JSON error envelope) lives in the SPA file; should move to a shared api/errors.go as handlers land
internal/api/spa.go:88
2/5
maintainability
10 single-model findings (lower confidence)
Finding
Where
Model
Lens
🟠
buildDSN loses filename when bare path contains literal '?'
internal/store/sqlite.go:72
kimi-k2.6:cloud
correctness
🟠
buildUrl produces malformed URL when path omits leading slash
web/src/lib/api.ts:47
kimi-k2.6:cloud
error-handling
🟡
PANSY_REGISTRATION=closed is documented as a working option but no code consumes it in Phase 0; row should be marked 'plumbed for #4' like the OIDC rows
README.md:36
opencode/glm-5.2:cloud
maintainability
🟡
RegistrationOpen() method is dead code and shadows the RegistrationOpen constant name
internal/config/config.go:60
claude-code/sonnet
maintainability
🟡
Duplicated default port literal 8080
internal/config/config.go:68
opencode/kimi-k2.6:cloud
maintainability
🟡
ShapePolygon constant is exposed in the v1 domain surface but is explicitly reserved/post-v1; invites callers to emit a value the editor/API reject
internal/domain/domain.go:40
opencode/glm-5.2:cloud
maintainability
🟡
LastInsertId error silently discarded in test, leads to confusing FK failure on driver error
internal/store/store_test.go:89
opencode/kimi-k2.6:cloud
error-handling
🟡
null body serialized as JSON 'null' because guard uses !== undefined instead of != null
web/src/lib/api.ts:89
opencode/kimi-k2.6:cloud
error-handling
⚪
"assets/" prefix literal checked twice in same handler; extract a local to avoid drift
internal/api/spa.go:60
glm-5.2:cloud
maintainability
⚪
applyMigration returns tx.Commit() unwrapped, so a commit failure doesn't identify the migration version
internal/store/migrate.go:93
glm-5.2:cloud
error-handling
Per-model detailclaude-code/sonnet (claude-code) — Minor issues
None found. This Phase 0 scaffold has no state-changing endpoints yet and no auth logic (deferred to #4/#5), so the security surface is small. I checked the areas most likely to carry security risk: the SPA static-file handler for path traversal, the SQLite DSN builder for injection, the trusted-proxy fallback, secret handling, and the frontend fetch wrapper.
VERDICT: No material issues found
internal/api/spa.go:45-51 — path traversal: path.Clean on an absolute request path collapses any .. segments to the root before the fs.Stat/fs.ReadFile lookup against the embedded dist FS, and embed.FS/fstest.MapFS additionally reject non-fs.ValidPath names. Verified by reading the code and reasoning through path.Clean("/../../etc/passwd") == "/etc/passwd"; confirmed test coverage (TestSPAMissingAssetIs404, directory-listing test) exercises adjacent edge cases.
internal/api/api.go:28-34 — trusted-proxy fallback: gin.Engine.SetTrustedProxies(nil) sets trustedCIDRs to nil, and gin's isTrustedProxy treats a nil trustedCIDRs as "trust nothing" (not gin's default trust-all), so the fallback on a parse error is correctly fail-closed, not fail-open. This already reflects the fix noted in the prior "Address Gadfly review findings" commit.
internal/store/sqlite.go — DSN pragma construction is built from a hardcoded pragma list and url.Values, not from untrusted network input (PANSY_DB is operator-set); no injection vector reachable from a request.
internal/config/config.go — PANSY_OIDC_CLIENT_SECRET is loaded into Config.OIDC.ClientSecret and never logged or serialized anywhere in this diff (grepped for all OIDC/ClientSecret usages).
internal/domain/domain.go — PasswordHash, OIDCIssuer, OIDCSubject on User and TokenHash on Session are all tagged json:"-", so a future accidental c.JSON(user) won't leak them.
web/src/lib/api.ts:96 — uses credentials: 'same-origin' (not 'include'), which is the correct choice once cookie-based sessions land; no dangerouslySetInnerHTML/eval in the frontend scaffold.
Nothing exploitable found in this PR's security-relevant surface (SPA file serving, DB DSN handling, proxy trust, secret plumbing). Auth itself isn't implemented yet (explicitly deferred to #4/#5), so there's no authn/authz logic to assess in this diff.
🎯 Correctness — No material issues found
This schema is consistent with the domain structs and matches design conventions throughout — no unit/formula/default mismatches found.
Nothing else stood out as a correctness defect in this scaffold-only PR (no business logic yet — just config loading, DB open/migrate, SPA fallback routing, and typed structs mirroring the schema). I checked the DSN pragma merging logic, in-memory detection, migration idempotency/versioning, the SPA fallback's file-vs-route disambiguation and cache-header branches, the trusted-proxies fallback, and the schema/struct field-type correspondence (nullable vs non-nullable columns matching pointer vs value fields) — all correct and internally consistent.
VERDICT: No material issues found
No correctness defects identified in this lens. The scaffold has no business logic yet, so there's little surface for domain/formula bugs; the mechanical pieces I checked (DSN pragma merging and in-memory detection in internal/store/sqlite.go, migration versioning/idempotency in internal/store/migrate.go, the SPA route/asset/API disambiguation in internal/api/spa.go, and the internal/domain struct fields against the 0001_init.sql schema's nullability) all check out. The Config.RegistrationOpen() method sharing a name with the RegistrationOpen package constant in internal/config/config.go:18,60 is unusual but not a bug — Go resolves the bare identifier inside the method body to the package-level constant, not a recursive call, and the described commit history confirms this already builds/tests clean.
🧹 Code cleanliness & maintainability — Minor issues
VERDICT: Minor issues
internal/config/config.go:60 — (*Config).RegistrationOpen() is dead code: grep -rn "RegistrationOpen()" --include="*.go" . across the whole repo returns only its own definition, zero callers. It also shares its name with the package-level constant RegistrationOpen = "open" declared at config.go:18 in the same file. This compiles (Go keeps method names and package-level identifiers in separate namespaces), but config.RegistrationOpen (string constant) and cfg.RegistrationOpen() (bool method) reading as the same identifier is a genuine readability trap for a future maintainer. Confirmed cfg in api.New (internal/api/api.go:22-40) is used only for TrustedProxies; the other Config fields (Registration, LocalAuth, OIDC, BaseURL) are unused scaffolding for issues #4/#5, consistent with the PR/README description — that part is expected and not itself a finding. Suggest dropping the unused method until #4 needs it, or renaming it (e.g. IsRegistrationOpen()) to avoid the collision.
⚡ Performance — No material issues found
VERDICT: No material issues found
This is a Phase 0 scaffold PR — no data-layer queries, list endpoints, or business logic exist yet, so there's essentially no surface for N+1 queries, missing pagination, or hot-loop/quadratic work. I checked the two runtime code paths that do exist:
internal/api/spa.go:51,65 — RegisterSPA's NoRoute handler calls fs.Stat(dist, name) to decide cache headers/directory-vs-file, then hands off to http.FileServer, which internally does its own Open+Stat on the same embedded file. That's a duplicate stat per static-asset request, but dist is an in-memory embed.FS, so the second stat is a cheap map lookup, not a syscall — not worth flagging as a regression. fileServer and the cached index bytes are both built once in RegisterSPA, not per-request, so no repeated allocation there. Verified by reading the full file.
internal/store/sqlite.go:40-57 — Open pins :memory: DBs to a single connection (correct, and necessary for correctness, not a perf issue) and leaves the file-backed pool at Go's default (unlimited open conns, busy_timeout=5000). That's standard practice for SQLite+WAL and not a regression introduced by this PR.
internal/store/migrate.go and cmd/pansy/main.go — migration loading/execution and server bootstrap only run once at startup, not on a hot path.
web/src/lib/api.ts, web/src/main.tsx — thin fetch wrapper and a QueryClient with sane defaults (staleTime: 30_000, no polling). No loops or repeated work of concern.
Nothing here rises to a material performance finding.
🧯 Error handling & edge cases — Minor issues
Both findings confirmed. buildDSN (internal/store/sqlite.go:68) does silently swallow the url.ParseQuery error with no logging, unlike every other invalid-input path in config.go which calls slog.Warn; and store_test.go has no test for a malformed query string. In api.ts:108, parseBody(res) is indeed called outside the try block (lines 92-106) that wraps fetch(), so a rejection from res.text() would bypass the ApiError wrapping.
Both are real, narrow findings. Verdict and findings stand as drafted.
VERDICT: Minor issues
internal/store/sqlite.go:68 — buildDSN silently swallows the error from url.ParseQuery(rawQuery): if q, err := url.ParseQuery(rawQuery); err == nil { query = q }. If a caller sets PANSY_DB to a DSN with a malformed query string (e.g. a stray % not followed by two hex digits), err != nil, query stays the zero-value url.Values{}, and any user-supplied pragmas/params in that query are silently discarded — no log, no error returned from Open. The required pragmas (busy_timeout, journal_mode, foreign_keys) still get added, so the DB opens "successfully" with different settings than the operator intended, with no signal anything went wrong. This is inconsistent with internal/config/config.go, where every other invalid-input path (envInt, envBool, invalid PANSY_REGISTRATION, out-of-range PANSY_PORT) logs slog.Warn on fallback. store_test.go has no test exercising a malformed query string. Suggested fix: on ParseQuery error, slog.Warn (or return the error from Open) instead of silently continuing with an empty query.
web/src/lib/api.ts:108 — const payload = await parseBody(res) runs outside the try { … } catch { throw new ApiError(...) } block that wraps fetch() (lines 92–106). parseBody calls res.text(), which can reject if the connection drops mid-response. In that case the rejection propagates as a raw error, not an ApiError, breaking the contract described in the file's own header comment (callers branch on error instanceof ApiError / .isConflict / .isUnauthorized). Narrow edge case (no write endpoints exist yet in this PR), but worth wrapping the parseBody call to keep the ApiError contract intact for a body-read failure after the response starts.
Through the security lens, I examined the untrusted-input surfaces introduced by this scaffold and found nothing materially wrong at this stage:
SPA file serving (internal/api/spa.go): directory listing is explicitly blocked (dir → 404), API misses return JSON not HTML, asset misses don't fall back to index.html (avoids the HTML-for-.js MIME confusion), and X-Content-Type-Options: nosniff is set on the SPA path. path.Clean is applied to the request path before fs.Stat, which also defeats .. traversal in the embedded FS. Verified by reading the file.
Trusted proxies (internal/api/api.go:28-34): on parse failure it falls back to trusting none rather than leaving gin's trust-everyone default active — correct, since the latter would allow arbitrary X-Forwarded-For spoofing. Verified.
SQL (internal/store/migrate.go, sqlite.go): migration SQL is fully static/embedded (no interpolation of env or request data); the DSN is built from the operator-controlled PANSY_DB env var only. No injection vector from untrusted input here.
Config / secrets: OIDC.ClientSecret lives in the config struct and is not logged anywhere I could find (grep found no slog/log line touching ClientSecret). The User/Session domain structs tag PasswordHash, OIDCIssuer, OIDCSubject, and TokenHash with json:"-" so they won't serialize to API responses. Verified.
Auth surface: this PR exposes only GET /api/v1/healthz plus the SPA; there are no authn/authz endpoints yet (deferred to #4/#5). Registration defaulting to open is a documented, intentional local-dev default, not a defect.
Two non-blocking observations outside the security lens (no action required from this review): no global security headers (Content-Security-Policy, Strict-Transport-Security, X-Frame-Options) are applied yet — reasonable to defer until the app shell is real; and healthz is unauthenticated by design as a liveness probe.
🎯 Correctness — No material issues found
VERDICT: No material issues found
I reviewed the diff through the correctness lens and verified the suspect areas against the checked-out code.
What I verified and found correct:
internal/store/sqlite.go:buildDSN — #-escaping happens after query parsing (so it can't corrupt the ?-split), memory detection is precise (:memory: exact token or mode=memory, not a substring), and de-duplication keys on pragma name only. The critical concern — that url.Values.Encode() percent-encodes the _pragma values (busy_timeout%285000%29) — is not a bug: store_test.go (TestForeignKeysEnforced for a bare path and TestForeignKeysEnforcedForFileURIDSN for a file: URI) actually open a DB and assert FK enforcement, which only passes if modernc URL-decodes the _pragma values back to busy_timeout(5000). Confirmed.
internal/store/migrate.go — idempotent runner, per-migration transaction with deferred rollback, version parsed from filename prefix, duplicate-version detection. Multi-statement SQL exec works (the test asserts all 7 tables + schema_migrations exist after one run).
internal/api/spa.go — path.Clean collapses .. before fs.Stat, and fs.Sub(dist,…) roots the FS at dist, so there's no path-traversal vector; API paths short-circuit before the SPA; missing assets/ returns JSON 404 (no HTML-200 MIME trap); directories 404 instead of listing. The index.html is read once at startup.
internal/domain/domain.go vs migrations/0001_init.sql — nullable schema columns (points, color, props, plants.owner_id, plantings.count/label/planted_at/removed_at) map to pointer fields; enums match CHECK constraints; version defaults align. The reserved shape='polygon' is allowed by the CHECK and carries a matching domain constant, consistent with the "post-v1" comment.
cmd/pansy/main.go — graceful shutdown: serveErr is buffered(1) so the goroutine never blocks; stop() is idempotent; migration uses a bounded context.
web/src/lib/api.ts — JSON serialization happens before fetch (so a circular-body failure isn't misreported as a network error); AbortError is re-thrown unchanged; 204/empty bodies parse to null.
One unverified minor note (not reported as a finding): serveIndex uses c.Data for HEAD requests while the comment claims net/http suppresses the body; I could not confirm Go's server strips the body for HEAD here, but even if it doesn't this is a protocol nit with no functional impact on a SPA scaffold, so it doesn't rise to a material correctness issue.
🧹 Code cleanliness & maintainability — Minor issues
Verdict: Minor issues
The scaffold is clean overall — DRY page stubs via the shared PageStub component, well-organized domain constants, a nicely factored buildDSN, and good doc comments. A couple of small maintainability nits:
internal/api/spa.go:88 — writeAPIError is described as "pansy's standard JSON error envelope" (a package-wide concern, and the exact shape the frontend api.tsmessageFrom already parses), yet it lives in the SPA-fallback file. As the api package grows real handlers (the package doc in api.go:3-5 explicitly says handlers stay thin and call a service layer), every one of them will reach for this helper and import it from the SPA file — a leaky placement. Confirmed against api.go (only healthz today), so the helper is reachable but misplaced. Suggest moving it (alongside the "NOT_FOUND"/"METHOD_NOT_ALLOWED" codes) into a small shared api/errors.go so spa.go and future handler files both depend on it rather than on each other.
internal/api/spa.go:60 and internal/api/spa.go:72 — strings.HasPrefix(name, "assets/") is evaluated twice in the same handler, once to set immutable cache headers and once to short-circuit a missing-asset 404. The two branches are distinct, but the repeated prefix literal is the kind of thing that drifts (e.g. if Vite is ever configured with a different assetsDir). A one-line isAsset := strings.HasPrefix(name, "assets/") local would keep them in sync and reads more clearly. Trivial.
web/src/pages/* — five nearly-identical one-line wrappers (export function XPage() { return <PageStub …>…</PageStub> }). This is fine for a scaffold and the shared PageStub is the right factoring; no change needed, just noting it's intentional duplication that should collapse as the real pages land.
Nothing else in my lens: naming is consistent (xCm/widthCm JSON tags match the schema x_cm/width_cm), no dead code, no overly long functions, and the migration runner is well decomposed (appliedVersions / applyMigration / loadMigrations / parseVersion).
⚡ Performance — No material issues found
I've reviewed the hot paths through the performance lens. This is a scaffold; the only request paths are /healthz (no DB) and the embedded SPA fallback. Let me confirm the SPA stat cost is in-memory and there's no per-request DB work.
The SPA handler does fs.Stat(dist, name) then fileServer.ServeHTTP (which stats again internally) — but dist is an embed.FS (in-memory map lookups), so this is negligible, not a material regression. index.html is correctly read once at startup. Migrations run once at boot. The DB pool isn't queried by any handler yet.
One forward-looking note (not material to this PR's hot paths): for file-backed DBs Open sets no SetMaxOpenConns cap (only :memory: is pinned to 1). Under future concurrent write load an unbounded SQLite pool can churn connections and starve the single writer, but since no handler touches the DB yet this isn't a current regression.
Verdict: No material issues found
Minor / forward-looking (not blocking):internal/store/sqlite.go:40 — file-backed DBs get an unbounded sql.DB connection pool (no SetMaxOpenConns). Modernc SQLite connections are relatively heavyweight and WAL still serializes writers; once request handlers query the DB, consider capping open conns (e.g. 1 writer + a small read pool) to avoid connection churn under load. Verified: only :memory: is pinned (sqlite.go:50); no handler currently uses the DB (only /healthz registered in api.go:42). Not a regression in this PR's actual hot paths.
🧯 Error handling & edge cases — Minor issues
The draft's single finding is confirmed against the actual code: internal/store/migrate.go:93 returns tx.Commit() unwrapped, while every other error path in applyMigration (lines 81, 86, 91) wraps with the migration version/name. The surrounding correctness claims in the draft (deferred tx.Rollback, rows.Err() check, close-on-defer) all hold against the source.
VERDICT: Minor issues
internal/store/migrate.go:93 — applyMigration returns tx.Commit() unwrapped, so a commit failure (e.g. disk full mid-migration) is reported without identifying which migration version failed, unlike every other error in this function which wraps with the version/name. Impact is diagnostic clarity only; the underlying SQLite error is still surfaced. Suggested fix: if err := tx.Commit(); err != nil { return fmt.Errorf("store: commit migration %d (%s): %w", m.version, m.name, err) }; return nil.
I verified the security-sensitive surfaces introduced in this scaffold:
SPA path handling (internal/api/spa.go:45): path.Clean + strings.TrimPrefix prevents directory traversal. The embedded fs.FS boundary means http.FileServer cannot escape the build artifact root. Directory listings are explicitly rejected (info.IsDir() → 404).
Secrets exposure: PANSY_OIDC_CLIENT_SECRET is read server-side only in internal/config/config.go:50 and never logged or returned. The vite.config.ts proxy only needs the backend port — no OIDC secrets reach the browser.
SQL injection: Migration SQL is embedded from //go:embed migrations/*.sql (trusted build artifacts), not constructed from user input. The DSN builder in internal/store/sqlite.go uses url.Values encoding; filePart# is escaped. No dynamic SQL concatenation exists.
Trusted proxy logic (internal/api/api.go:28): On parse failure it explicitly falls back to trusting no proxies, avoiding gin’s trust-everyone default.
Headers: The SPA fallback sets X-Content-Type-Options: nosniff. Cache-Control on hashed assets is correctly immutable; index.html is no-cache.
Nothing in this diff presents an authn/authz gap, injection vector, SSRF, unsafe deserialization, credential leakage, or insecure default that rises above the “scaffold with no auth endpoints yet” stage.
🎯 Correctness — Minor issues
Minor issues
internal/store/sqlite.go:72-95 — buildDSN mis-splits bare paths on ?, discarding part of the filename.
For a bare path such as /tmp/weird?name.db (where ? is a literal character in the Unix filename), strings.Cut(body, "?") treats name.db as the query string. Because url.ParseQuery("name.db") fails, the query remains empty and the database is created at /tmp/weird instead of the intended path. The existing test (TestBuildDSNAlwaysIncludesPragmas) only asserts pragma presence, so the bug is masked. Fix: only split on ? when the input already starts with file:; for bare paths percent-encode ? (e.g. strings.ReplaceAll(filePart, "?", "%3F")) before constructing the URI.
internal/store/migrations/0001_init.sql:25-26 — users.oidc_issuer/oidc_subject allow semantically invalid partial-NULL states.
The schema permits a row where one column is NULL and the other is NOT NULL (e.g. issuer set without subject, or vice versa). That state is meaningless for OIDC identity linkage and could be created by a bug in the auth code planned for #5. Fix: add a CHECK constraint, e.g.
CHECK((oidc_issuerISNULL)=(oidc_subjectISNULL))
🧹 Code cleanliness & maintainability — No material issues found
No material issues found
⚡ Performance — No material issues found
No material issues found
I scrutinized the PR for efficiency regressions, allocation hotspots, unbounded growth, blocking calls, and connection/resource limits. The scaffold is clean:
SPA handler (internal/api/spa.go): Pre-reads index.html once at startup; fs.Stat + http.FileServer for static assets is standard and the embedded FS keeps it in-memory. No redundant I/O or copying on hot paths.
Migration runner (internal/store/migrate.go): Loads migrations once at boot; schema version table is bounded. No loops or allocations that scale with runtime data.
Frontend fetch wrapper (web/src/lib/api.ts): Straightforward fetch with URLSearchParams; response parsing is O(response size). No pagination needed yet (no list endpoints in this scaffold).
QueryClient config (web/src/main.tsx): staleTime: 30_000 and refetchOnWindowFocus: false are sensible defaults that prevent excessive refetching.
The only latent concern is that store.Open does not cap the sql.DB connection pool for file-backed databases (MaxOpenConns stays at Go’s default of 0, i.e., unlimited). SQLite serializes writers, so under concurrent load an unbounded pool can cause many connections to spin on busy_timeout(5000) rather than queueing gracefully. However, since Phase 0 has no write endpoints and the actual entity query methods are coming in later issues, this is not yet a material regression — it will naturally be addressed when the service layer is wired up and connection limits are tuned for the expected concurrency.
🧯 Error handling & edge cases — Minor issues
Minor issues
web/src/lib/api.ts:47 — buildUrl concatenates BASE + path without ensuring path starts with /. If a caller passes a naked path like apiFetch<Health>('healthz'), the resulting URL is /api/v1healthz (missing slash), which will always 404. This is an unhandled edge case for malformed input. Fix: normalize with a leading slash, e.g. const url = BASE + (path.startsWith('/') ? path : '/' + path), or assert in dev.
internal/store/sqlite.go:63-89 — buildDSN splits the supplied path on the first ? to separate the file part from query parameters. A bare filename that literally contains ? (valid on Unix) is mis-parsed: weird?name.db becomes file:weird?name.db=…, so SQLite opens a database named weird instead of weird?name.db. The docstring explicitly says it accepts bare filenames, so this is an unhandled edge case. Fix: only parse query parameters when the input is already a file: URI; for bare filenames, treat the entire string as the path and append the pragmas.
Through the 🔒 Security lens, I read the actual checked-out code for internal/api/{api.go,spa.go}, internal/config/config.go, internal/store/{sqlite.go,migrate.go,migrations/0001_init.sql}, cmd/pansy/main.go, internal/webdist/dist.go, web/vite.config.ts, and web/src/lib/api.ts to verify each concern.
Verification notes:
Trusted-proxy handling (internal/api/api.go:28-34): on SetTrustedProxies parse failure the code explicitly falls back to SetTrustedProxies(nil) (trust none) rather than letting gin's trust-everyone default stay active — correct, no spoofing path.
Path traversal / SPA fallback (internal/api/spa.go:45-67): path.Clean is applied before TrimPrefix("/"), so ../ segments collapse; the fs.Stat is over an embedded fstest/embed.FS which cannot escape. Directory entries are explicitly 404'd (no listing leak). Missing assets/* returns 404 (no HTML-as-JS MIME confusion). Fine.
Secret handling: OIDC.ClientSecret is read from env into the struct and never logged (grepped all slog.* call sites — only paths/ports/versions/errors are emitted). PasswordHash/OIDCIssuer/OIDCSubject are tagged json:"-". Good.
Migrations: SQL comes exclusively from //go:embed migrations/*.sql; no user-controlled input reaches tx.ExecContext(m.sql). Version parsing is on embedded filenames. No injection surface.
DSN construction (internal/store/sqlite.go:63-90): the only tainted-ish input is PANSY_DB, which is operator-controlled, not untrusted. The #→%23 escape prevents fragment truncation of the pragma query. There is a theoretical foot-gun where an operator supplying _pragma=foreign_keys(0) would suppress the required foreign_keys(1) due to the de-dup by name, but that is operator self-disabling, not an attacker vector — out of scope for a security finding.
HTTP server (cmd/pansy/main.go:53-60): no TLS, but Phase 0 is explicitly intended to sit behind a reverse proxy (per README/DESIGN context); no plaintext-secret transmission occurs in this scaffold (no auth endpoints yet). Not a Phase-0 defect.
CORS/CSRF: same-origin only (embedded SPA + dev proxy); no auth endpoints exist yet, so no CSRF surface to protect. Deferred to #4/#5.
Content-Type sniffing: X-Content-Type-Options: nosniff is set on the SPA fallback path; API JSON errors use gin.JSON which pins application/json. Fine.
VERDICT: No material issues found
🎯 Correctness — Minor issues
Review
VERDICT: Minor issues
Through the correctness lens I verified the migration runner, DSN builder, SPA fallback, config loader, schema, and domain types by reading the actual checked-out code (not just the diff).
Findings
internal/store/sqlite.go:66 — buildDSN mis-parses a ? embedded in the filesystem path. Confirmed by tracing the code against the actual source: for input /tmp/weird?name.db (the test case at store_test.go:132), strings.TrimPrefix leaves body = "/tmp/weird?name.db", then strings.Cut(body, "?") splits it into filePart="/tmp/weird" and rawQuery="name.db". url.ParseQuery("name.db") succeeds and returns {"name.db": [""]}. The pragmas are then added, query.Encode() emits _pragma=...&name.db=, and the final DSN is file:/tmp/weird?_pragma=busy_timeout%285000%29&_pragma=foreign_keys%281%29&_pragma=journal_mode%28WAL%29&name.db=. modernc treats everything after ? as query parameters, so the DB file actually opened is /tmp/weird — not the configured /tmp/weird?name.db. The test at store_test.go:126-141 only asserts the returned string contains the three pragma names via strings.Contains; it never opens the resulting DSN and checks that the file is created at the intended path, so it masks the corruption. The # escaping at line 88 (to avoid it starting a fragment) is asymmetric: ? is split before any escaping could protect it, so a ? in a plain filesystem path can't be recovered. Real-world paths rarely contain ?, so impact is low, but the test gives a false sense of coverage. Suggested fix: only split on ? when the input is a file: URI (where ? legitimately starts a query); for a bare filesystem path, treat the whole string as the filename and don't synthesize a query section.
Things I checked and found correct
Migration runner (migrate.go): idempotent, transactional, version dedup, ascending sort, sentinel-table creation — all verified. applyMigration rolls back on failure (deferred) and commits only after recording the version.
Schema (0001_init.sql): CHECK enums match domain constants; FK actions consistent (CASCADE on owner/parent, RESTRICT on plantings.plant_id); idx_users_oidc UNIQUE on (oidc_issuer, oidc_subject) allows multiple NULL pairs under SQLite's NULL-distinct semantics.
:memory: coherence: SetMaxOpenConns(1) pins the pool so a second connection sees the migrated schema; TestInMemoryDBIsCoherentAcrossQueries proves it.
SPA fallback (spa.go): the assets/ missing-file 404 (no HTML-for-.js), directory-as-404, /api/* JSON 404, and non-GET 405 branches all behave as claimed and are covered by tests. path.Clean + TrimPrefix("/") correctly handles .. and trailing slashes.
Config (config.go): port range clamp, registration enum validation with fallback to open, envStr treating empty string as unset — all correct.
domain struct JSON tags and pointer/omitempty usage mirror the schema's nullable columns.
I could not independently verify how modernc's Open parses a relative file:./pansy.db?... DSN (module cache not materialized), but the project's own tests open a file-backed ./test.db and assert FK enforcement reaches the pool, which transitively exercises the produced DSN end-to-end for the common cases.
🧹 Code cleanliness & maintainability — Minor issues
I've examined the structure thoroughly. Let me check a couple more things about the API error envelope consistency between the Go side and the TS side.
I have enough now. Let me think through cleanliness/maintainability findings.
Let me check the unused TestBuildDSN... and confirm api import alias is unused in main:
VERDICT: Minor issues
Code is generally clean and well-structured. A few small maintainability nits worth addressing:
internal/api/spa.go:88-91 — writeAPIError is misplaced. It is pansy's standard JSON error envelope helper ({"error":{"code":...,"message":...}}), not an SPA-specific concern. Every future API handler (per the package doc in api.go:3-5 — "decode, call the service layer, encode") will need it, but it's currently tucked at the bottom of the SPA fallback file. Move it (and its doc comment) into api.go so it lives with the rest of the API surface; otherwise each new internal/api/*.go will either reach across to spa.go for it or duplicate it. Verified by reading both files.
internal/domain/domain.go:38-40 — ShapePolygon constant is dead code today. The schema and domain comment explicitly say polygon/points are "reserved for post-v1; the v1 editor emits only 'rect' and 'circle'" (schema comment, internal/store/migrations/0001_init.sql:69-70). Exposing the constant in the public domain surface now invites callers to use a value the editor and (per the diff) the API will reject. Either drop it from the v1 set and add it when the polygon issue lands, or at least document it as "reserved, do not emit." Minor, but it's the kind of "plausible-looking magic value" that breeds a real bug later.
README.md env-var table — PANSY_REGISTRATION: closed is documented as usable, but nothing consumes closed yet.config.RegistrationOpen() exists (internal/config/config.go:60-62) and there is no RegistrationClosed()/IsClosed() accessor; the only call site in Phase 0 is healthz. The README presents closed as a working option ("open or closed — gates local self-service signup"). Either land the gating in this PR's scope (it isn't — auth is #4/#6) or annotate the row as "plumbed for #4" the way the OIDC rows are, so operators don't set PANSY_REGISTRATION=closed expecting it to do something. (Verified by grepping for RegistrationOpen/RegistrationClosed callers.)
No material issues with the rest: the DSN builder, migration runner, SPA fallback, and api.ts wrapper are all readable, follow the surrounding patterns, and don't carry duplicated/dead code beyond the above.
⚡ Performance — Minor issues
All three draft findings are confirmed as accurate but explicitly described as non-actionable/not-real-concerns. The draft itself concludes "Nothing material in the performance lens" with an empty findings array. The findings listed are observations, not actual issues. Per verification, the code matches the draft's descriptions exactly. The verdict reflects "Minor issues" but with an empty findings array — the three bullets are explicitly non-actionable. Since no actionable findings survive, the verdict should be "No material issues found" with an empty findings array.
VERDICT: No material issues found
🧯 Error handling & edge cases — Minor issues
VERDICT: Minor issues
web/src/lib/api.ts:108 — body parsing can throw a raw error instead of ApiError, breaking the wrapper's central contract.parseBody(res) calls await res.text() at line 60 (and JSON.parse(text) at line 65, though that one is guarded by a try/catch that falls back to text). The parseBody(res) call at line 108 is outside the try/catch in apiFetch, which only wraps the fetch at lines 92–106. So if the body stream fails mid-read (connection drop, aborted after headers, proxy reset), apiFetch rejects with the raw TypeError rather than an ApiError carrying status/body. The whole point of the wrapper (per the file's own doc comment at lines 3–7) is that callers can read ApiError.status / .isConflict / .isUnauthorized; a non-ApiError rejection defeats the optimistic editor's 409 reconciliation path. Suggested fix: wrap parseBody(res) in a try/catch and re-throw as new ApiError('Malformed response from the pansy server.', res.status, null) so the ApiError invariant holds on every rejection out of apiFetch. Verified by reading api.ts lines 58–113.
All other error paths I checked are handled correctly:
cmd/pansy/main.go: server error vs signal shutdown handled in select; defer db.Close() runs after Shutdown returns (correct ordering); serveErr channel is buffered so the goroutine never blocks.
internal/store/migrate.go: defer tx.Rollback() with //nolint:errcheck after commit — standard idiom; rows.Err() checked after iteration; appliedVersions errors propagate.
internal/store/sqlite.go: Ping failure closes the DB before returning; :memory: pinned to one conn.
internal/api/spa.go: missing index.html panics at startup (appropriate for a packaging bug); directory stat → 404 (no listing leak); path.Clean + embedded FS prevent traversal.
internal/config/config.go: invalid port/registration fall back to defaults with a slog.Warn; never returns an error (documented behavior).
internal/api/api.go: SetTrustedProxies parse failure explicitly falls back to nil (never leaves the trust-everyone default).
opencode/kimi-k2.6:cloud (opencode) — Blocking issues found
Reviewed through the Security lens. The Phase 0 scaffold has a solid security posture for its scope:
SPA path handling — path.Clean plus fs.Sub-rooted embedded FS blocks directory traversal; .. segments are resolved away and the sub-FS rejects escapes. No path traversal vector found.
Database — Only embedded migration SQL is executed; runtime queries use parameterized placeholders (?). No SQL injection from untrusted input exists yet (no user-facing mutating endpoints).
Proxy trust — SetTrustedProxies safely falls back to trusting none on parse failure, preventing X-Forwarded-For spoofing.
Secrets — OIDC client secret is loaded into memory but never logged or serialized. No credential leakage in the diff.
Headers — X-Content-Type-Options: nosniff is present on SPA responses; hashed assets get immutable cache directives while index.html gets no-cache, mitigating cache poisoning on redeploys.
No unsafe eval/DOM sinks — The React frontend contains no dangerouslySetInnerHTML, eval, document.write, or similar sinks.
Nothing in this change constitutes a material authn/authz gap, injection, SSRF, deserialization, secret leakage, or insecure default at this scaffold stage.
🎯 Correctness — Blocking issues found
Blocking issues found
internal/store/sqlite.go:66 — buildDSN unconditionally splits on ? even for bare filesystem paths. A valid path like /tmp/weird?name.db (where ? is a legal Linux filename character) is truncated to /tmp/weird and the remainder is treated as malformed query parameters and discarded. The existing test TestBuildDSNAlwaysIncludesPragmas exercises this exact input but only asserts pragma presence, not filename preservation, so the bug passes undetected. Fix: only split on ? when the path is a URI (starts with file:), or escape ? in the file part before reconstructing the DSN.
internal/store/migrations/0001_init.sql:27 — The users table allows oidc_issuer to be non-NULL while oidc_subject is NULL (or vice versa), despite the documented semantics that OIDC identity is a pair. A missing CHECK ((oidc_issuer IS NULL AND oidc_subject IS NULL) OR (oidc_issuer IS NOT NULL AND oidc_subject IS NOT NULL)) permits invalid partial OIDC records that the application will have to clean up later. Fix: add the paired-nullability CHECK to enforce the invariant at the schema level.
🧹 Code cleanliness & maintainability — Minor issues
Minor issues
internal/config/config.go:68,88-89 — The default port literal 8080 is duplicated: once as the fallback argument to envInt, and again in the validation block's log message and reassignment. If the default ever changes, all three sites must be updated together or they drift. Extract a package-level constant (defaultPort = 8080) and reference it everywhere.
⚡ Performance — No material issues found
No material issues found
Performance lens review:
SPA handler (internal/api/spa.go): The index.html is read once at startup and cached in memory — good. Asset files served via http.FileServer get immutable cache headers. No redundant allocations or I/O in the hot path.
SQLite connection pool (internal/store/sqlite.go): File-backed databases use the default sql.DB pool. While an explicit SetMaxOpenConns could cap connections under extreme load, this is a self-hosted garden planner in Phase 0 with no query layer yet, so the default behavior is not a material concern. :memory: is correctly pinned to one connection.
Migration runner (internal/store/migrate.go): Migrations load from the embedded FS once at startup, sort, and apply idempotently. No quadratic behavior or N+1 issues — schema_migrations is tiny.
Frontend fetch (web/src/lib/api.ts): Reads the full response body before parsing, but this is standard for a JSON API wrapper and not material for expected payload sizes.
Router (web/src/router.tsx): Uses defaultPreload: 'intent' for route preloading, which is a performance optimization. QueryClient disables window-focus refetching and sets a 30s stale time — sensible defaults.
🧯 Error handling & edge cases — Minor issues
Minor issues found.
internal/store/sqlite.go:68 — url.ParseQuery error is silently swallowed in buildDSN. If a user provides a path with malformed query parameters (e.g., invalid percent-encoding like file:x.db?mode=memory%ZZ), the raw query is discarded and replaced with an empty map. This drops any user-supplied _pragma overrides and, more importantly, can miss a mode=memory parameter, causing the in-memory detection (memory = ... || query.Get("mode") == "memory") to return false and skip the SetMaxOpenConns(1) pin that keeps an in-memory DB coherent across the pool. Fix: Surface the parse error: if q, err := url.ParseQuery(rawQuery); err != nil { return "", false, fmt.Errorf("invalid DSN query: %w", err) }.
web/src/lib/api.ts:89 — A null request body is serialized as the JSON string "null" because the guard is body !== undefined instead of body != null. Callers often use null to mean “no body,” but here it triggers Content-Type: application/json and sends a literal null payload, which can break server-side handlers expecting an object or no body at all. Fix: Change the guard to body != null so both null and undefined are treated as absent.
internal/store/store_test.go:89,95 — res.LastInsertId() errors are silently ignored (uid, _ := ...). If the driver ever returns an error, the test continues with uid=0, and the subsequent FK insert fails with a misleading foreign-key violation message instead of the actual LastInsertId error. Fix: Assert the error in tests: uid, err := res.LastInsertId(); if err != nil { t.Fatalf(...) }.
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: Blocking issues found** · 14 findings (4 with multi-model agreement)
| | Finding | Where | Models | Lens |
|--|--|--|--|--|
| 🔴 | buildDSN truncates filenames containing ? by treating them as URI query strings | `internal/store/sqlite.go:63` | 4/5 | correctness, error-handling |
| 🟠 | users schema allows partial OIDC identity (issuer without subject or vice versa) | `internal/store/migrations/0001_init.sql:25` | 2/5 | correctness |
| 🟠 | parseBody's res.text() runs outside apiFetch's try/catch, so a body-read failure rejects with a raw TypeError instead of an ApiError, breaking the wrapper's status/body contract that the optimistic editor depends on | `web/src/lib/api.ts:108` | 2/5 | error-handling |
| 🟡 | writeAPIError (standard JSON error envelope) lives in the SPA file; should move to a shared api/errors.go as handlers land | `internal/api/spa.go:88` | 2/5 | maintainability |
<details><summary>10 single-model findings (lower confidence)</summary>
| | Finding | Where | Model | Lens |
|--|--|--|--|--|
| 🟠 | buildDSN loses filename when bare path contains literal '?' | `internal/store/sqlite.go:72` | kimi-k2.6:cloud | correctness |
| 🟠 | buildUrl produces malformed URL when path omits leading slash | `web/src/lib/api.ts:47` | kimi-k2.6:cloud | error-handling |
| 🟡 | PANSY_REGISTRATION=closed is documented as a working option but no code consumes it in Phase 0; row should be marked 'plumbed for #4' like the OIDC rows | `README.md:36` | opencode/glm-5.2:cloud | maintainability |
| 🟡 | RegistrationOpen() method is dead code and shadows the RegistrationOpen constant name | `internal/config/config.go:60` | claude-code/sonnet | maintainability |
| 🟡 | Duplicated default port literal 8080 | `internal/config/config.go:68` | opencode/kimi-k2.6:cloud | maintainability |
| 🟡 | ShapePolygon constant is exposed in the v1 domain surface but is explicitly reserved/post-v1; invites callers to emit a value the editor/API reject | `internal/domain/domain.go:40` | opencode/glm-5.2:cloud | maintainability |
| 🟡 | LastInsertId error silently discarded in test, leads to confusing FK failure on driver error | `internal/store/store_test.go:89` | opencode/kimi-k2.6:cloud | error-handling |
| 🟡 | null body serialized as JSON 'null' because guard uses !== undefined instead of != null | `web/src/lib/api.ts:89` | opencode/kimi-k2.6:cloud | error-handling |
| ⚪ | "assets/" prefix literal checked twice in same handler; extract a local to avoid drift | `internal/api/spa.go:60` | glm-5.2:cloud | maintainability |
| ⚪ | applyMigration returns tx.Commit() unwrapped, so a commit failure doesn't identify the migration version | `internal/store/migrate.go:93` | glm-5.2:cloud | 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>
None found. This Phase 0 scaffold has no state-changing endpoints yet and no auth logic (deferred to #4/#5), so the security surface is small. I checked the areas most likely to carry security risk: the SPA static-file handler for path traversal, the SQLite DSN builder for injection, the trusted-proxy fallback, secret handling, and the frontend fetch wrapper.
**VERDICT: No material issues found**
- `internal/api/spa.go:45-51` — path traversal: `path.Clean` on an absolute request path collapses any `..` segments to the root before the `fs.Stat`/`fs.ReadFile` lookup against the embedded `dist` FS, and `embed.FS`/`fstest.MapFS` additionally reject non-`fs.ValidPath` names. Verified by reading the code and reasoning through `path.Clean("/../../etc/passwd") == "/etc/passwd"`; confirmed test coverage (`TestSPAMissingAssetIs404`, directory-listing test) exercises adjacent edge cases.
- `internal/api/api.go:28-34` — trusted-proxy fallback: `gin.Engine.SetTrustedProxies(nil)` sets `trustedCIDRs` to `nil`, and gin's `isTrustedProxy` treats a nil `trustedCIDRs` as "trust nothing" (not gin's default trust-all), so the fallback on a parse error is correctly fail-closed, not fail-open. This already reflects the fix noted in the prior "Address Gadfly review findings" commit.
- `internal/store/sqlite.go` — DSN pragma construction is built from a hardcoded pragma list and `url.Values`, not from untrusted network input (`PANSY_DB` is operator-set); no injection vector reachable from a request.
- `internal/config/config.go` — `PANSY_OIDC_CLIENT_SECRET` is loaded into `Config.OIDC.ClientSecret` and never logged or serialized anywhere in this diff (grepped for all `OIDC`/`ClientSecret` usages).
- `internal/domain/domain.go` — `PasswordHash`, `OIDCIssuer`, `OIDCSubject` on `User` and `TokenHash` on `Session` are all tagged `json:"-"`, so a future accidental `c.JSON(user)` won't leak them.
- `web/src/lib/api.ts:96` — uses `credentials: 'same-origin'` (not `'include'`), which is the correct choice once cookie-based sessions land; no `dangerouslySetInnerHTML`/`eval` in the frontend scaffold.
Nothing exploitable found in this PR's security-relevant surface (SPA file serving, DB DSN handling, proxy trust, secret plumbing). Auth itself isn't implemented yet (explicitly deferred to #4/#5), so there's no authn/authz logic to assess in this diff.
</details>
<details><summary><b>🎯 Correctness</b> — No material issues found</summary>
This schema is consistent with the domain structs and matches design conventions throughout — no unit/formula/default mismatches found.
Nothing else stood out as a correctness defect in this scaffold-only PR (no business logic yet — just config loading, DB open/migrate, SPA fallback routing, and typed structs mirroring the schema). I checked the DSN pragma merging logic, in-memory detection, migration idempotency/versioning, the SPA fallback's file-vs-route disambiguation and cache-header branches, the trusted-proxies fallback, and the schema/struct field-type correspondence (nullable vs non-nullable columns matching pointer vs value fields) — all correct and internally consistent.
**VERDICT: No material issues found**
No correctness defects identified in this lens. The scaffold has no business logic yet, so there's little surface for domain/formula bugs; the mechanical pieces I checked (DSN pragma merging and in-memory detection in `internal/store/sqlite.go`, migration versioning/idempotency in `internal/store/migrate.go`, the SPA route/asset/API disambiguation in `internal/api/spa.go`, and the `internal/domain` struct fields against the `0001_init.sql` schema's nullability) all check out. The `Config.RegistrationOpen()` method sharing a name with the `RegistrationOpen` package constant in `internal/config/config.go:18,60` is unusual but not a bug — Go resolves the bare identifier inside the method body to the package-level constant, not a recursive call, and the described commit history confirms this already builds/tests clean.
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary>
VERDICT: Minor issues
- `internal/config/config.go:60` — `(*Config).RegistrationOpen()` is dead code: `grep -rn "RegistrationOpen()" --include="*.go" .` across the whole repo returns only its own definition, zero callers. It also shares its name with the package-level constant `RegistrationOpen = "open"` declared at `config.go:18` in the same file. This compiles (Go keeps method names and package-level identifiers in separate namespaces), but `config.RegistrationOpen` (string constant) and `cfg.RegistrationOpen()` (bool method) reading as the same identifier is a genuine readability trap for a future maintainer. Confirmed `cfg` in `api.New` (`internal/api/api.go:22-40`) is used only for `TrustedProxies`; the other `Config` fields (`Registration`, `LocalAuth`, `OIDC`, `BaseURL`) are unused scaffolding for issues #4/#5, consistent with the PR/README description — that part is expected and not itself a finding. Suggest dropping the unused method until #4 needs it, or renaming it (e.g. `IsRegistrationOpen()`) to avoid the collision.
</details>
<details><summary><b>⚡ Performance</b> — No material issues found</summary>
VERDICT: No material issues found
This is a Phase 0 scaffold PR — no data-layer queries, list endpoints, or business logic exist yet, so there's essentially no surface for N+1 queries, missing pagination, or hot-loop/quadratic work. I checked the two runtime code paths that do exist:
- `internal/api/spa.go:51,65` — `RegisterSPA`'s `NoRoute` handler calls `fs.Stat(dist, name)` to decide cache headers/directory-vs-file, then hands off to `http.FileServer`, which internally does its own `Open`+`Stat` on the same embedded file. That's a duplicate stat per static-asset request, but `dist` is an in-memory `embed.FS`, so the second stat is a cheap map lookup, not a syscall — not worth flagging as a regression. `fileServer` and the cached `index` bytes are both built once in `RegisterSPA`, not per-request, so no repeated allocation there. Verified by reading the full file.
- `internal/store/sqlite.go:40-57` — `Open` pins `:memory:` DBs to a single connection (correct, and necessary for correctness, not a perf issue) and leaves the file-backed pool at Go's default (unlimited open conns, `busy_timeout=5000`). That's standard practice for SQLite+WAL and not a regression introduced by this PR.
- `internal/store/migrate.go` and `cmd/pansy/main.go` — migration loading/execution and server bootstrap only run once at startup, not on a hot path.
- `web/src/lib/api.ts`, `web/src/main.tsx` — thin fetch wrapper and a `QueryClient` with sane defaults (`staleTime: 30_000`, no polling). No loops or repeated work of concern.
Nothing here rises to a material performance finding.
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — Minor issues</summary>
Both findings confirmed. `buildDSN` (internal/store/sqlite.go:68) does silently swallow the `url.ParseQuery` error with no logging, unlike every other invalid-input path in `config.go` which calls `slog.Warn`; and `store_test.go` has no test for a malformed query string. In `api.ts:108`, `parseBody(res)` is indeed called outside the `try` block (lines 92-106) that wraps `fetch()`, so a rejection from `res.text()` would bypass the `ApiError` wrapping.
Both are real, narrow findings. Verdict and findings stand as drafted.
## VERDICT: Minor issues
- **`internal/store/sqlite.go:68`** — `buildDSN` silently swallows the error from `url.ParseQuery(rawQuery)`: `if q, err := url.ParseQuery(rawQuery); err == nil { query = q }`. If a caller sets `PANSY_DB` to a DSN with a malformed query string (e.g. a stray `%` not followed by two hex digits), `err != nil`, `query` stays the zero-value `url.Values{}`, and any user-supplied pragmas/params in that query are silently discarded — no log, no error returned from `Open`. The required pragmas (`busy_timeout`, `journal_mode`, `foreign_keys`) still get added, so the DB opens "successfully" with different settings than the operator intended, with no signal anything went wrong. This is inconsistent with `internal/config/config.go`, where every other invalid-input path (`envInt`, `envBool`, invalid `PANSY_REGISTRATION`, out-of-range `PANSY_PORT`) logs `slog.Warn` on fallback. `store_test.go` has no test exercising a malformed query string. Suggested fix: on `ParseQuery` error, `slog.Warn` (or return the error from `Open`) instead of silently continuing with an empty query.
- **`web/src/lib/api.ts:108`** — `const payload = await parseBody(res)` runs outside the `try { … } catch { throw new ApiError(...) }` block that wraps `fetch()` (lines 92–106). `parseBody` calls `res.text()`, which can reject if the connection drops mid-response. In that case the rejection propagates as a raw error, not an `ApiError`, breaking the contract described in the file's own header comment (callers branch on `error instanceof ApiError` / `.isConflict` / `.isUnauthorized`). Narrow edge case (no write endpoints exist yet in this PR), but worth wrapping the `parseBody` call to keep the `ApiError` contract intact for a body-read failure after the response starts.
</details>
</details>
<details><summary><b>glm-5.2:cloud</b> (ollama-cloud) — Minor issues</summary>
**Verdict: Minor issues** — 5 reviewers: security, correctness, maintainability, performance, error-handling
<details><summary><b>🔒 Security</b> — No material issues found</summary>
## VERDICT: No material issues found
Through the security lens, I examined the untrusted-input surfaces introduced by this scaffold and found nothing materially wrong at this stage:
- **SPA file serving** (`internal/api/spa.go`): directory listing is explicitly blocked (dir → 404), API misses return JSON not HTML, asset misses don't fall back to index.html (avoids the HTML-for-.js MIME confusion), and `X-Content-Type-Options: nosniff` is set on the SPA path. `path.Clean` is applied to the request path before `fs.Stat`, which also defeats `..` traversal in the embedded FS. Verified by reading the file.
- **Trusted proxies** (`internal/api/api.go:28-34`): on parse failure it falls back to trusting none rather than leaving gin's trust-everyone default active — correct, since the latter would allow arbitrary `X-Forwarded-For` spoofing. Verified.
- **SQL** (`internal/store/migrate.go`, `sqlite.go`): migration SQL is fully static/embedded (no interpolation of env or request data); the DSN is built from the operator-controlled `PANSY_DB` env var only. No injection vector from untrusted input here.
- **Config / secrets**: `OIDC.ClientSecret` lives in the config struct and is not logged anywhere I could find (grep found no `slog`/log line touching `ClientSecret`). The `User`/`Session` domain structs tag `PasswordHash`, `OIDCIssuer`, `OIDCSubject`, and `TokenHash` with `json:"-"` so they won't serialize to API responses. Verified.
- **Auth surface**: this PR exposes only `GET /api/v1/healthz` plus the SPA; there are no authn/authz endpoints yet (deferred to #4/#5). Registration defaulting to `open` is a documented, intentional local-dev default, not a defect.
Two non-blocking observations outside the security lens (no action required from this review): no global security headers (`Content-Security-Policy`, `Strict-Transport-Security`, `X-Frame-Options`) are applied yet — reasonable to defer until the app shell is real; and `healthz` is unauthenticated by design as a liveness probe.
</details>
<details><summary><b>🎯 Correctness</b> — No material issues found</summary>
**VERDICT: No material issues found**
I reviewed the diff through the correctness lens and verified the suspect areas against the checked-out code.
What I verified and found correct:
- `internal/store/sqlite.go:buildDSN` — `#`-escaping happens after query parsing (so it can't corrupt the `?`-split), memory detection is precise (`:memory:` exact token or `mode=memory`, not a substring), and de-duplication keys on pragma name only. The critical concern — that `url.Values.Encode()` percent-encodes the `_pragma` values (`busy_timeout%285000%29`) — is *not* a bug: `store_test.go` (`TestForeignKeysEnforced` for a bare path and `TestForeignKeysEnforcedForFileURIDSN` for a `file:` URI) actually open a DB and assert FK enforcement, which only passes if modernc URL-decodes the `_pragma` values back to `busy_timeout(5000)`. Confirmed.
- `internal/store/migrate.go` — idempotent runner, per-migration transaction with deferred rollback, version parsed from filename prefix, duplicate-version detection. Multi-statement SQL exec works (the test asserts all 7 tables + `schema_migrations` exist after one run).
- `internal/api/spa.go` — `path.Clean` collapses `..` before `fs.Stat`, and `fs.Sub(dist,…)` roots the FS at `dist`, so there's no path-traversal vector; API paths short-circuit before the SPA; missing `assets/` returns JSON 404 (no HTML-200 MIME trap); directories 404 instead of listing. The `index.html` is read once at startup.
- `internal/domain/domain.go` vs `migrations/0001_init.sql` — nullable schema columns (`points`, `color`, `props`, `plants.owner_id`, `plantings.count/label/planted_at/removed_at`) map to pointer fields; enums match CHECK constraints; `version` defaults align. The reserved `shape='polygon'` is allowed by the CHECK and carries a matching domain constant, consistent with the "post-v1" comment.
- `cmd/pansy/main.go` — graceful shutdown: `serveErr` is buffered(1) so the goroutine never blocks; `stop()` is idempotent; migration uses a bounded context.
- `web/src/lib/api.ts` — JSON serialization happens before `fetch` (so a circular-body failure isn't misreported as a network error); `AbortError` is re-thrown unchanged; 204/empty bodies parse to `null`.
One unverified minor note (not reported as a finding): `serveIndex` uses `c.Data` for HEAD requests while the comment claims net/http suppresses the body; I could not confirm Go's server strips the body for HEAD here, but even if it doesn't this is a protocol nit with no functional impact on a SPA scaffold, so it doesn't rise to a material correctness issue.
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary>
**Verdict: Minor issues**
The scaffold is clean overall — DRY page stubs via the shared `PageStub` component, well-organized domain constants, a nicely factored `buildDSN`, and good doc comments. A couple of small maintainability nits:
- `internal/api/spa.go:88` — `writeAPIError` is described as "pansy's standard JSON error envelope" (a package-wide concern, and the exact shape the frontend `api.ts` `messageFrom` already parses), yet it lives in the SPA-fallback file. As the `api` package grows real handlers (the package doc in `api.go:3-5` explicitly says handlers stay thin and call a service layer), every one of them will reach for this helper and import it from the SPA file — a leaky placement. Confirmed against `api.go` (only `healthz` today), so the helper is reachable but misplaced. Suggest moving it (alongside the `"NOT_FOUND"`/`"METHOD_NOT_ALLOWED"` codes) into a small shared `api/errors.go` so `spa.go` and future handler files both depend on it rather than on each other.
- `internal/api/spa.go:60` and `internal/api/spa.go:72` — `strings.HasPrefix(name, "assets/")` is evaluated twice in the same handler, once to set immutable cache headers and once to short-circuit a missing-asset 404. The two branches are distinct, but the repeated prefix literal is the kind of thing that drifts (e.g. if Vite is ever configured with a different `assetsDir`). A one-line `isAsset := strings.HasPrefix(name, "assets/")` local would keep them in sync and reads more clearly. Trivial.
- `web/src/pages/*` — five nearly-identical one-line wrappers (`export function XPage() { return <PageStub …>…</PageStub> }`). This is fine for a scaffold and the shared `PageStub` is the right factoring; no change needed, just noting it's intentional duplication that should collapse as the real pages land.
Nothing else in my lens: naming is consistent (`xCm`/`widthCm` JSON tags match the schema `x_cm`/`width_cm`), no dead code, no overly long functions, and the migration runner is well decomposed (`appliedVersions` / `applyMigration` / `loadMigrations` / `parseVersion`).
</details>
<details><summary><b>⚡ Performance</b> — No material issues found</summary>
I've reviewed the hot paths through the performance lens. This is a scaffold; the only request paths are `/healthz` (no DB) and the embedded SPA fallback. Let me confirm the SPA stat cost is in-memory and there's no per-request DB work.
The SPA handler does `fs.Stat(dist, name)` then `fileServer.ServeHTTP` (which stats again internally) — but `dist` is an `embed.FS` (in-memory map lookups), so this is negligible, not a material regression. `index.html` is correctly read once at startup. Migrations run once at boot. The DB pool isn't queried by any handler yet.
One forward-looking note (not material to this PR's hot paths): for file-backed DBs `Open` sets no `SetMaxOpenConns` cap (only `:memory:` is pinned to 1). Under future concurrent write load an unbounded SQLite pool can churn connections and starve the single writer, but since no handler touches the DB yet this isn't a current regression.
**Verdict: No material issues found**
- *Minor / forward-looking (not blocking):* `internal/store/sqlite.go:40` — file-backed DBs get an unbounded `sql.DB` connection pool (no `SetMaxOpenConns`). Modernc SQLite connections are relatively heavyweight and WAL still serializes writers; once request handlers query the DB, consider capping open conns (e.g. 1 writer + a small read pool) to avoid connection churn under load. Verified: only `:memory:` is pinned (`sqlite.go:50`); no handler currently uses the DB (only `/healthz` registered in `api.go:42`). Not a regression in this PR's actual hot paths.
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — Minor issues</summary>
The draft's single finding is confirmed against the actual code: `internal/store/migrate.go:93` returns `tx.Commit()` unwrapped, while every other error path in `applyMigration` (lines 81, 86, 91) wraps with the migration version/name. The surrounding correctness claims in the draft (deferred `tx.Rollback`, `rows.Err()` check, close-on-defer) all hold against the source.
VERDICT: Minor issues
- `internal/store/migrate.go:93` — `applyMigration` returns `tx.Commit()` unwrapped, so a commit failure (e.g. disk full mid-migration) is reported without identifying which migration version failed, unlike every other error in this function which wraps with the version/name. Impact is diagnostic clarity only; the underlying SQLite error is still surfaced. Suggested fix: `if err := tx.Commit(); err != nil { return fmt.Errorf("store: commit migration %d (%s): %w", m.version, m.name, err) }; return nil`.
</details>
</details>
<details><summary><b>kimi-k2.6:cloud</b> (ollama-cloud) — Minor issues</summary>
**Verdict: Minor issues** — 5 reviewers: security, correctness, maintainability, performance, error-handling
<details><summary><b>🔒 Security</b> — No material issues found</summary>
**Verdict: No material issues found**
I verified the security-sensitive surfaces introduced in this scaffold:
- **SPA path handling** (`internal/api/spa.go:45`): `path.Clean` + `strings.TrimPrefix` prevents directory traversal. The embedded `fs.FS` boundary means `http.FileServer` cannot escape the build artifact root. Directory listings are explicitly rejected (`info.IsDir()` → 404).
- **Secrets exposure**: `PANSY_OIDC_CLIENT_SECRET` is read server-side only in `internal/config/config.go:50` and never logged or returned. The `vite.config.ts` proxy only needs the backend port — no OIDC secrets reach the browser.
- **SQL injection**: Migration SQL is embedded from `//go:embed migrations/*.sql` (trusted build artifacts), not constructed from user input. The DSN builder in `internal/store/sqlite.go` uses `url.Values` encoding; `filePart` `#` is escaped. No dynamic SQL concatenation exists.
- **Trusted proxy logic** (`internal/api/api.go:28`): On parse failure it explicitly falls back to trusting no proxies, avoiding gin’s trust-everyone default.
- **Headers**: The SPA fallback sets `X-Content-Type-Options: nosniff`. `Cache-Control` on hashed assets is correctly `immutable`; `index.html` is `no-cache`.
Nothing in this diff presents an authn/authz gap, injection vector, SSRF, unsafe deserialization, credential leakage, or insecure default that rises above the “scaffold with no auth endpoints yet” stage.
</details>
<details><summary><b>🎯 Correctness</b> — Minor issues</summary>
**Minor issues**
- **`internal/store/sqlite.go:72-95` — `buildDSN` mis-splits bare paths on `?`, discarding part of the filename.**
For a bare path such as `/tmp/weird?name.db` (where `?` is a literal character in the Unix filename), `strings.Cut(body, "?")` treats `name.db` as the query string. Because `url.ParseQuery("name.db")` fails, the query remains empty and the database is created at `/tmp/weird` instead of the intended path. The existing test (`TestBuildDSNAlwaysIncludesPragmas`) only asserts pragma presence, so the bug is masked.
**Fix:** only split on `?` when the input already starts with `file:`; for bare paths percent-encode `?` (e.g. `strings.ReplaceAll(filePart, "?", "%3F")`) before constructing the URI.
- **`internal/store/migrations/0001_init.sql:25-26` — `users.oidc_issuer`/`oidc_subject` allow semantically invalid partial-NULL states.**
The schema permits a row where one column is NULL and the other is NOT NULL (e.g. issuer set without subject, or vice versa). That state is meaningless for OIDC identity linkage and could be created by a bug in the auth code planned for #5.
**Fix:** add a `CHECK` constraint, e.g.
```sql
CHECK ((oidc_issuer IS NULL) = (oidc_subject IS NULL))
```
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — No material issues found</summary>
No material issues found
</details>
<details><summary><b>⚡ Performance</b> — No material issues found</summary>
No material issues found
I scrutinized the PR for efficiency regressions, allocation hotspots, unbounded growth, blocking calls, and connection/resource limits. The scaffold is clean:
- **SPA handler** (`internal/api/spa.go`): Pre-reads `index.html` once at startup; `fs.Stat` + `http.FileServer` for static assets is standard and the embedded FS keeps it in-memory. No redundant I/O or copying on hot paths.
- **Migration runner** (`internal/store/migrate.go`): Loads migrations once at boot; schema version table is bounded. No loops or allocations that scale with runtime data.
- **Frontend fetch wrapper** (`web/src/lib/api.ts`): Straightforward `fetch` with `URLSearchParams`; response parsing is O(response size). No pagination needed yet (no list endpoints in this scaffold).
- **QueryClient config** (`web/src/main.tsx`): `staleTime: 30_000` and `refetchOnWindowFocus: false` are sensible defaults that prevent excessive refetching.
The only latent concern is that `store.Open` does not cap the `sql.DB` connection pool for file-backed databases (`MaxOpenConns` stays at Go’s default of 0, i.e., unlimited). SQLite serializes writers, so under concurrent load an unbounded pool can cause many connections to spin on `busy_timeout(5000)` rather than queueing gracefully. However, since Phase 0 has no write endpoints and the actual entity query methods are coming in later issues, this is not yet a material regression — it will naturally be addressed when the service layer is wired up and connection limits are tuned for the expected concurrency.
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — Minor issues</summary>
**Minor issues**
- **`web/src/lib/api.ts:47`** — `buildUrl` concatenates `BASE + path` without ensuring `path` starts with `/`. If a caller passes a naked path like `apiFetch<Health>('healthz')`, the resulting URL is `/api/v1healthz` (missing slash), which will always 404. This is an unhandled edge case for malformed input.
**Fix:** normalize with a leading slash, e.g. `const url = BASE + (path.startsWith('/') ? path : '/' + path)`, or assert in dev.
- **`internal/store/sqlite.go:63-89`** — `buildDSN` splits the supplied path on the first `?` to separate the file part from query parameters. A bare filename that literally contains `?` (valid on Unix) is mis-parsed: `weird?name.db` becomes `file:weird?name.db=…`, so SQLite opens a database named `weird` instead of `weird?name.db`. The docstring explicitly says it accepts bare filenames, so this is an unhandled edge case.
**Fix:** only parse query parameters when the input is already a `file:` URI; for bare filenames, treat the entire string as the path and append the pragmas.
</details>
</details>
<details><summary><b>opencode/glm-5.2:cloud</b> (opencode) — Minor issues</summary>
**Verdict: Minor issues** — 5 reviewers: security, correctness, maintainability, performance, error-handling
<details><summary><b>🔒 Security</b> — No material issues found</summary>
Through the 🔒 Security lens, I read the actual checked-out code for `internal/api/{api.go,spa.go}`, `internal/config/config.go`, `internal/store/{sqlite.go,migrate.go,migrations/0001_init.sql}`, `cmd/pansy/main.go`, `internal/webdist/dist.go`, `web/vite.config.ts`, and `web/src/lib/api.ts` to verify each concern.
Verification notes:
- **Trusted-proxy handling** (`internal/api/api.go:28-34`): on `SetTrustedProxies` parse failure the code *explicitly* falls back to `SetTrustedProxies(nil)` (trust none) rather than letting gin's trust-everyone default stay active — correct, no spoofing path.
- **Path traversal / SPA fallback** (`internal/api/spa.go:45-67`): `path.Clean` is applied before `TrimPrefix("/")`, so `../` segments collapse; the `fs.Stat` is over an embedded `fstest`/`embed.FS` which cannot escape. Directory entries are explicitly 404'd (no listing leak). Missing `assets/*` returns 404 (no HTML-as-JS MIME confusion). Fine.
- **Secret handling**: `OIDC.ClientSecret` is read from env into the struct and never logged (grepped all `slog.*` call sites — only paths/ports/versions/errors are emitted). `PasswordHash`/`OIDCIssuer`/`OIDCSubject` are tagged `json:"-"`. Good.
- **Migrations**: SQL comes exclusively from `//go:embed migrations/*.sql`; no user-controlled input reaches `tx.ExecContext(m.sql)`. Version parsing is on embedded filenames. No injection surface.
- **DSN construction** (`internal/store/sqlite.go:63-90`): the only tainted-ish input is `PANSY_DB`, which is operator-controlled, not untrusted. The `#`→`%23` escape prevents fragment truncation of the pragma query. There is a theoretical foot-gun where an operator supplying `_pragma=foreign_keys(0)` would suppress the required `foreign_keys(1)` due to the de-dup by name, but that is operator self-disabling, not an attacker vector — out of scope for a security finding.
- **HTTP server** (`cmd/pansy/main.go:53-60`): no TLS, but Phase 0 is explicitly intended to sit behind a reverse proxy (per README/DESIGN context); no plaintext-secret transmission occurs in this scaffold (no auth endpoints yet). Not a Phase-0 defect.
- **CORS/CSRF**: same-origin only (embedded SPA + dev proxy); no auth endpoints exist yet, so no CSRF surface to protect. Deferred to #4/#5.
- **Content-Type sniffing**: `X-Content-Type-Options: nosniff` is set on the SPA fallback path; API JSON errors use `gin.JSON` which pins `application/json`. Fine.
VERDICT: No material issues found
</details>
<details><summary><b>🎯 Correctness</b> — Minor issues</summary>
## Review
**VERDICT: Minor issues**
Through the correctness lens I verified the migration runner, DSN builder, SPA fallback, config loader, schema, and domain types by reading the actual checked-out code (not just the diff).
### Findings
- **`internal/store/sqlite.go:66` — `buildDSN` mis-parses a `?` embedded in the filesystem path.** Confirmed by tracing the code against the actual source: for input `/tmp/weird?name.db` (the test case at `store_test.go:132`), `strings.TrimPrefix` leaves `body = "/tmp/weird?name.db"`, then `strings.Cut(body, "?")` splits it into `filePart="/tmp/weird"` and `rawQuery="name.db"`. `url.ParseQuery("name.db")` succeeds and returns `{"name.db": [""]}`. The pragmas are then added, `query.Encode()` emits `_pragma=...&name.db=`, and the final DSN is `file:/tmp/weird?_pragma=busy_timeout%285000%29&_pragma=foreign_keys%281%29&_pragma=journal_mode%28WAL%29&name.db=`. modernc treats everything after `?` as query parameters, so the DB file actually opened is `/tmp/weird` — **not** the configured `/tmp/weird?name.db`. The test at `store_test.go:126-141` only asserts the returned string *contains* the three pragma names via `strings.Contains`; it never opens the resulting DSN and checks that the file is created at the intended path, so it masks the corruption. The `#` escaping at line 88 (to avoid it starting a fragment) is asymmetric: `?` is split before any escaping could protect it, so a `?` in a plain filesystem path can't be recovered. Real-world paths rarely contain `?`, so impact is low, but the test gives a false sense of coverage. Suggested fix: only split on `?` when the input is a `file:` URI (where `?` legitimately starts a query); for a bare filesystem path, treat the whole string as the filename and don't synthesize a query section.
### Things I checked and found correct
- Migration runner (`migrate.go`): idempotent, transactional, version dedup, ascending sort, sentinel-table creation — all verified. `applyMigration` rolls back on failure (deferred) and commits only after recording the version.
- Schema (`0001_init.sql`): CHECK enums match `domain` constants; FK actions consistent (CASCADE on owner/parent, RESTRICT on `plantings.plant_id`); `idx_users_oidc` UNIQUE on `(oidc_issuer, oidc_subject)` allows multiple NULL pairs under SQLite's NULL-distinct semantics.
- `:memory:` coherence: `SetMaxOpenConns(1)` pins the pool so a second connection sees the migrated schema; `TestInMemoryDBIsCoherentAcrossQueries` proves it.
- SPA fallback (`spa.go`): the `assets/` missing-file 404 (no HTML-for-.js), directory-as-404, `/api/*` JSON 404, and non-GET 405 branches all behave as claimed and are covered by tests. `path.Clean` + `TrimPrefix("/")` correctly handles `..` and trailing slashes.
- Config (`config.go`): port range clamp, registration enum validation with fallback to `open`, `envStr` treating empty string as unset — all correct.
- `domain` struct JSON tags and pointer/`omitempty` usage mirror the schema's nullable columns.
I could not independently verify how modernc's `Open` parses a relative `file:./pansy.db?...` DSN (module cache not materialized), but the project's own tests open a file-backed `./test.db` and assert FK enforcement reaches the pool, which transitively exercises the produced DSN end-to-end for the common cases.
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary>
I've examined the structure thoroughly. Let me check a couple more things about the API error envelope consistency between the Go side and the TS side.
I have enough now. Let me think through cleanliness/maintainability findings.
Let me check the unused `TestBuildDSN...` and confirm `api` import alias is unused in main:
## VERDICT: Minor issues
Code is generally clean and well-structured. A few small maintainability nits worth addressing:
- **`internal/api/spa.go:88-91` — `writeAPIError` is misplaced.** It is pansy's *standard JSON error envelope* helper (`{"error":{"code":...,"message":...}}`), not an SPA-specific concern. Every future API handler (per the package doc in `api.go:3-5` — "decode, call the service layer, encode") will need it, but it's currently tucked at the bottom of the SPA fallback file. Move it (and its doc comment) into `api.go` so it lives with the rest of the API surface; otherwise each new `internal/api/*.go` will either reach across to `spa.go` for it or duplicate it. Verified by reading both files.
- **`internal/domain/domain.go:38-40` — `ShapePolygon` constant is dead code today.** The schema and domain comment explicitly say `polygon`/`points` are "reserved for post-v1; the v1 editor emits only 'rect' and 'circle'" (schema comment, `internal/store/migrations/0001_init.sql:69-70`). Exposing the constant in the public domain surface now invites callers to use a value the editor and (per the diff) the API will reject. Either drop it from the v1 set and add it when the polygon issue lands, or at least document it as "reserved, do not emit." Minor, but it's the kind of "plausible-looking magic value" that breeds a real bug later.
- **`README.md` env-var table — `PANSY_REGISTRATION: closed` is documented as usable, but nothing consumes `closed` yet.** `config.RegistrationOpen()` exists (`internal/config/config.go:60-62`) and there is no `RegistrationClosed()`/`IsClosed()` accessor; the only call site in Phase 0 is `healthz`. The README presents `closed` as a working option ("`open` or `closed` — gates local self-service signup"). Either land the gating in this PR's scope (it isn't — auth is #4/#6) or annotate the row as "plumbed for #4" the way the OIDC rows are, so operators don't set `PANSY_REGISTRATION=closed` expecting it to do something. (Verified by grepping for `RegistrationOpen`/`RegistrationClosed` callers.)
No material issues with the rest: the DSN builder, migration runner, SPA fallback, and `api.ts` wrapper are all readable, follow the surrounding patterns, and don't carry duplicated/dead code beyond the above.
</details>
<details><summary><b>⚡ Performance</b> — Minor issues</summary>
All three draft findings are confirmed as accurate but explicitly described as non-actionable/not-real-concerns. The draft itself concludes "Nothing material in the performance lens" with an empty findings array. The findings listed are observations, not actual issues. Per verification, the code matches the draft's descriptions exactly. The verdict reflects "Minor issues" but with an empty findings array — the three bullets are explicitly non-actionable. Since no actionable findings survive, the verdict should be "No material issues found" with an empty findings array.
VERDICT: No material issues found
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — Minor issues</summary>
## VERDICT: Minor issues
- **`web/src/lib/api.ts:108` — body parsing can throw a raw error instead of `ApiError`, breaking the wrapper's central contract.** `parseBody(res)` calls `await res.text()` at line 60 (and `JSON.parse(text)` at line 65, though that one is guarded by a `try/catch` that falls back to text). The `parseBody(res)` call at line 108 is outside the `try/catch` in `apiFetch`, which only wraps the `fetch` at lines 92–106. So if the body stream fails mid-read (connection drop, aborted after headers, proxy reset), `apiFetch` rejects with the raw `TypeError` rather than an `ApiError` carrying `status`/`body`. The whole point of the wrapper (per the file's own doc comment at lines 3–7) is that callers can read `ApiError.status` / `.isConflict` / `.isUnauthorized`; a non-`ApiError` rejection defeats the optimistic editor's 409 reconciliation path. Suggested fix: wrap `parseBody(res)` in a `try/catch` and re-throw as `new ApiError('Malformed response from the pansy server.', res.status, null)` so the `ApiError` invariant holds on every rejection out of `apiFetch`. Verified by reading `api.ts` lines 58–113.
All other error paths I checked are handled correctly:
- `cmd/pansy/main.go`: server error vs signal shutdown handled in `select`; `defer db.Close()` runs after `Shutdown` returns (correct ordering); `serveErr` channel is buffered so the goroutine never blocks.
- `internal/store/migrate.go`: `defer tx.Rollback()` with `//nolint:errcheck` after commit — standard idiom; `rows.Err()` checked after iteration; `appliedVersions` errors propagate.
- `internal/store/sqlite.go`: `Ping` failure closes the DB before returning; `:memory:` pinned to one conn.
- `internal/api/spa.go`: missing `index.html` panics at startup (appropriate for a packaging bug); directory stat → 404 (no listing leak); `path.Clean` + embedded FS prevent traversal.
- `internal/config/config.go`: invalid port/registration fall back to defaults with a `slog.Warn`; never returns an error (documented behavior).
- `internal/api/api.go`: `SetTrustedProxies` parse failure explicitly falls back to `nil` (never leaves the trust-everyone default).
</details>
</details>
<details><summary><b>opencode/kimi-k2.6:cloud</b> (opencode) — Blocking issues found</summary>
**Verdict: Blocking issues found** — 5 reviewers: security, correctness, maintainability, performance, error-handling
<details><summary><b>🔒 Security</b> — No material issues found</summary>
**No material issues found**
Reviewed through the **Security** lens. The Phase 0 scaffold has a solid security posture for its scope:
- **SPA path handling** — `path.Clean` plus `fs.Sub`-rooted embedded FS blocks directory traversal; `..` segments are resolved away and the sub-FS rejects escapes. No path traversal vector found.
- **Database** — Only embedded migration SQL is executed; runtime queries use parameterized placeholders (`?`). No SQL injection from untrusted input exists yet (no user-facing mutating endpoints).
- **Proxy trust** — `SetTrustedProxies` safely falls back to trusting none on parse failure, preventing `X-Forwarded-For` spoofing.
- **Secrets** — OIDC client secret is loaded into memory but never logged or serialized. No credential leakage in the diff.
- **Headers** — `X-Content-Type-Options: nosniff` is present on SPA responses; hashed assets get `immutable` cache directives while `index.html` gets `no-cache`, mitigating cache poisoning on redeploys.
- **No unsafe eval/DOM sinks** — The React frontend contains no `dangerouslySetInnerHTML`, `eval`, `document.write`, or similar sinks.
Nothing in this change constitutes a material authn/authz gap, injection, SSRF, deserialization, secret leakage, or insecure default at this scaffold stage.
</details>
<details><summary><b>🎯 Correctness</b> — Blocking issues found</summary>
**Blocking issues found**
- **`internal/store/sqlite.go:66`** — `buildDSN` unconditionally splits on `?` even for bare filesystem paths. A valid path like `/tmp/weird?name.db` (where `?` is a legal Linux filename character) is truncated to `/tmp/weird` and the remainder is treated as malformed query parameters and discarded. The existing test `TestBuildDSNAlwaysIncludesPragmas` exercises this exact input but only asserts pragma presence, not filename preservation, so the bug passes undetected. Fix: only split on `?` when the path is a URI (starts with `file:`), or escape `?` in the file part before reconstructing the DSN.
- **`internal/store/migrations/0001_init.sql:27`** — The `users` table allows `oidc_issuer` to be non-NULL while `oidc_subject` is NULL (or vice versa), despite the documented semantics that OIDC identity is a pair. A missing `CHECK ((oidc_issuer IS NULL AND oidc_subject IS NULL) OR (oidc_issuer IS NOT NULL AND oidc_subject IS NOT NULL))` permits invalid partial OIDC records that the application will have to clean up later. Fix: add the paired-nullability CHECK to enforce the invariant at the schema level.
</details>
<details><summary><b>🧹 Code cleanliness & maintainability</b> — Minor issues</summary>
Minor issues
- **`internal/config/config.go:68,88-89`** — The default port literal `8080` is duplicated: once as the fallback argument to `envInt`, and again in the validation block's log message and reassignment. If the default ever changes, all three sites must be updated together or they drift. Extract a package-level constant (`defaultPort = 8080`) and reference it everywhere.
</details>
<details><summary><b>⚡ Performance</b> — No material issues found</summary>
No material issues found
**Performance lens review:**
- **SPA handler (`internal/api/spa.go`)**: The `index.html` is read once at startup and cached in memory — good. Asset files served via `http.FileServer` get immutable cache headers. No redundant allocations or I/O in the hot path.
- **SQLite connection pool (`internal/store/sqlite.go`)**: File-backed databases use the default `sql.DB` pool. While an explicit `SetMaxOpenConns` could cap connections under extreme load, this is a self-hosted garden planner in Phase 0 with no query layer yet, so the default behavior is not a material concern. `:memory:` is correctly pinned to one connection.
- **Migration runner (`internal/store/migrate.go`)**: Migrations load from the embedded FS once at startup, sort, and apply idempotently. No quadratic behavior or N+1 issues — `schema_migrations` is tiny.
- **Frontend fetch (`web/src/lib/api.ts`)**: Reads the full response body before parsing, but this is standard for a JSON API wrapper and not material for expected payload sizes.
- **Router (`web/src/router.tsx`)**: Uses `defaultPreload: 'intent'` for route preloading, which is a performance optimization. QueryClient disables window-focus refetching and sets a 30s stale time — sensible defaults.
</details>
<details><summary><b>🧯 Error handling & edge cases</b> — Minor issues</summary>
Minor issues found.
* **`internal/store/sqlite.go:68`** — `url.ParseQuery` error is silently swallowed in `buildDSN`. If a user provides a path with malformed query parameters (e.g., invalid percent-encoding like `file:x.db?mode=memory%ZZ`), the raw query is discarded and replaced with an empty map. This drops any user-supplied `_pragma` overrides and, more importantly, can miss a `mode=memory` parameter, causing the in-memory detection (`memory = ... || query.Get("mode") == "memory"`) to return `false` and skip the `SetMaxOpenConns(1)` pin that keeps an in-memory DB coherent across the pool.
**Fix:** Surface the parse error: `if q, err := url.ParseQuery(rawQuery); err != nil { return "", false, fmt.Errorf("invalid DSN query: %w", err) }`.
* **`web/src/lib/api.ts:89`** — A `null` request body is serialized as the JSON string `"null"` because the guard is `body !== undefined` instead of `body != null`. Callers often use `null` to mean “no body,” but here it triggers `Content-Type: application/json` and sends a literal `null` payload, which can break server-side handlers expecting an object or no body at all.
**Fix:** Change the guard to `body != null` so both `null` and `undefined` are treated as absent.
* **`internal/store/store_test.go:89,95`** — `res.LastInsertId()` errors are silently ignored (`uid, _ := ...`). If the driver ever returns an error, the test continues with `uid=0`, and the subsequent FK insert fails with a misleading foreign-key violation message instead of the actual `LastInsertId` error.
**Fix:** Assert the error in tests: `uid, err := res.LastInsertId(); if err != nil { t.Fatalf(...) }`.
</details>
</details>
</details>
<sub>Automated adversarial review by Gadfly — consensus across the model swarm. Advisory only — does not block merge.</sub>
Applied the fixes warranted by the adversarial review of PR #21 (all
findings graded in the gadfly store):
Store (highest impact):
- buildDSN always merges busy_timeout/journal_mode/foreign_keys pragmas
into the DSN, even for file: URIs or paths with a query string — the
prior passthrough silently disabled FK enforcement for those PANSY_DB
values. Also escape '#' in the path and tighten in-memory detection to
an exact ':memory:' token or mode=memory param (no substring misfire).
- migrate.go: detect duplicate migration versions with a clear error;
wrap appliedVersions' rows.Err() with the store: prefix.
API:
- SetTrustedProxies failure now falls back to trust-none instead of
leaving gin's trust-everyone default (X-Forwarded-For spoofing).
- SPA: 404 embedded directories (was a directory listing), 404 bare /api,
add X-Content-Type-Options: nosniff, cache index.html bytes once at
startup, single fs.Stat existence check, shared writeAPIError helper.
- Move gin.SetMode out of package init() into New().
Config: validate PANSY_PORT range (fall back to 8080 with a warning).
Web:
- api.ts: serialize the request body before the fetch try so a
JSON.stringify failure isn't misreported as a network error.
- AppShell: move state-specific/conflicting utilities into
active/inactiveProps so concatenated Tailwind classes don't collide.
Tests: added store DSN-pragma/memory-detection cases and SPA
directory-404 case. go build/vet/test clean (CGO off); web tsc + build
clean; re-verified in a browser (SPA, deep links, /assets 404, nav).
Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01JdQpdYYsTgtkJBxbcpAszi
steve
merged commit e290adb2e0 into main2026-07-18 19:25:30 +00:00
steve
deleted branch phase-0-scaffold2026-07-18 19:25:30 +00:00
🪰Gadfly consensus review — 14 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** — 14 inline findings on changed lines. See the consensus comment for the full ranked summary.
<sub>Advisory only — does not block merge.</sub>
🟡PANSY_REGISTRATION=closed is documented as a working option but no code consumes it in Phase 0; row should be marked 'plumbed for #4' like the OIDC rows
maintainability · flagged by 1 model
🪰 Gadfly · advisory
🟡 **PANSY_REGISTRATION=closed is documented as a working option but no code consumes it in Phase 0; row should be marked 'plumbed for #4' like the OIDC rows**
_maintainability · flagged by 1 model_
<sub>🪰 Gadfly · advisory</sub>
⚪"assets/" prefix literal checked twice in same handler; extract a local to avoid drift
maintainability · flagged by 1 model
internal/api/spa.go:60 and internal/api/spa.go:72 — strings.HasPrefix(name, "assets/") is evaluated twice in the same handler, once to set immutable cache headers and once to short-circuit a missing-asset 404. The two branches are distinct, but the repeated prefix literal is the kind of thing that drifts (e.g. if Vite is ever configured with a different assetsDir). A one-line isAsset := strings.HasPrefix(name, "assets/") local would keep them in sync and reads more clearly. Trivial.
🪰 Gadfly · advisory
⚪ **"assets/" prefix literal checked twice in same handler; extract a local to avoid drift**
_maintainability · flagged by 1 model_
- `internal/api/spa.go:60` and `internal/api/spa.go:72` — `strings.HasPrefix(name, "assets/")` is evaluated twice in the same handler, once to set immutable cache headers and once to short-circuit a missing-asset 404. The two branches are distinct, but the repeated prefix literal is the kind of thing that drifts (e.g. if Vite is ever configured with a different `assetsDir`). A one-line `isAsset := strings.HasPrefix(name, "assets/")` local would keep them in sync and reads more clearly. Trivial.
<sub>🪰 Gadfly · advisory</sub>
🟡writeAPIError (standard JSON error envelope) lives in the SPA file; should move to a shared api/errors.go as handlers land
maintainability · flagged by 2 models
internal/api/spa.go:88 — writeAPIError is described as "pansy's standard JSON error envelope" (a package-wide concern, and the exact shape the frontend api.tsmessageFrom already parses), yet it lives in the SPA-fallback file. As the api package grows real handlers (the package doc in api.go:3-5 explicitly says handlers stay thin and call a service layer), every one of them will reach for this helper and import it from the SPA file — a leaky placement. Confirmed against api.go (o…
🪰 Gadfly · advisory
🟡 **writeAPIError (standard JSON error envelope) lives in the SPA file; should move to a shared api/errors.go as handlers land**
_maintainability · flagged by 2 models_
- `internal/api/spa.go:88` — `writeAPIError` is described as "pansy's standard JSON error envelope" (a package-wide concern, and the exact shape the frontend `api.ts` `messageFrom` already parses), yet it lives in the SPA-fallback file. As the `api` package grows real handlers (the package doc in `api.go:3-5` explicitly says handlers stay thin and call a service layer), every one of them will reach for this helper and import it from the SPA file — a leaky placement. Confirmed against `api.go` (o…
<sub>🪰 Gadfly · advisory</sub>
🟡RegistrationOpen() method is dead code and shadows the RegistrationOpen constant name
maintainability · flagged by 1 model
internal/config/config.go:60 — (*Config).RegistrationOpen() is dead code: grep -rn "RegistrationOpen()" --include="*.go" . across the whole repo returns only its own definition, zero callers. It also shares its name with the package-level constant RegistrationOpen = "open" declared at config.go:18 in the same file. This compiles (Go keeps method names and package-level identifiers in separate namespaces), but config.RegistrationOpen (string constant) and cfg.RegistrationOpen() (b…
🪰 Gadfly · advisory
🟡 **RegistrationOpen() method is dead code and shadows the RegistrationOpen constant name**
_maintainability · flagged by 1 model_
- `internal/config/config.go:60` — `(*Config).RegistrationOpen()` is dead code: `grep -rn "RegistrationOpen()" --include="*.go" .` across the whole repo returns only its own definition, zero callers. It also shares its name with the package-level constant `RegistrationOpen = "open"` declared at `config.go:18` in the same file. This compiles (Go keeps method names and package-level identifiers in separate namespaces), but `config.RegistrationOpen` (string constant) and `cfg.RegistrationOpen()` (b…
<sub>🪰 Gadfly · advisory</sub>
internal/config/config.go:68,88-89 — The default port literal 8080 is duplicated: once as the fallback argument to envInt, and again in the validation block's log message and reassignment. If the default ever changes, all three sites must be updated together or they drift. Extract a package-level constant (defaultPort = 8080) and reference it everywhere.
🪰 Gadfly · advisory
🟡 **Duplicated default port literal 8080**
_maintainability · flagged by 1 model_
- **`internal/config/config.go:68,88-89`** — The default port literal `8080` is duplicated: once as the fallback argument to `envInt`, and again in the validation block's log message and reassignment. If the default ever changes, all three sites must be updated together or they drift. Extract a package-level constant (`defaultPort = 8080`) and reference it everywhere.
<sub>🪰 Gadfly · advisory</sub>
🟡ShapePolygon constant is exposed in the v1 domain surface but is explicitly reserved/post-v1; invites callers to emit a value the editor/API reject
maintainability · flagged by 1 model
🪰 Gadfly · advisory
🟡 **ShapePolygon constant is exposed in the v1 domain surface but is explicitly reserved/post-v1; invites callers to emit a value the editor/API reject**
_maintainability · flagged by 1 model_
<sub>🪰 Gadfly · advisory</sub>
⚪applyMigration returns tx.Commit() unwrapped, so a commit failure doesn't identify the migration version
error-handling · flagged by 1 model
The draft's single finding is confirmed against the actual code: internal/store/migrate.go:93 returns tx.Commit() unwrapped, while every other error path in applyMigration (lines 81, 86, 91) wraps with the migration version/name. The surrounding correctness claims in the draft (deferred tx.Rollback, rows.Err() check, close-on-defer) all hold against the source.
🪰 Gadfly · advisory
⚪ **applyMigration returns tx.Commit() unwrapped, so a commit failure doesn't identify the migration version**
_error-handling · flagged by 1 model_
The draft's single finding is confirmed against the actual code: `internal/store/migrate.go:93` returns `tx.Commit()` unwrapped, while every other error path in `applyMigration` (lines 81, 86, 91) wraps with the migration version/name. The surrounding correctness claims in the draft (deferred `tx.Rollback`, `rows.Err()` check, close-on-defer) all hold against the source.
<sub>🪰 Gadfly · advisory</sub>
🟠users schema allows partial OIDC identity (issuer without subject or vice versa)
correctness · flagged by 2 models
internal/store/migrations/0001_init.sql:27 — The users table allows oidc_issuer to be non-NULL while oidc_subject is NULL (or vice versa), despite the documented semantics that OIDC identity is a pair. A missing CHECK ((oidc_issuer IS NULL AND oidc_subject IS NULL) OR (oidc_issuer IS NOT NULL AND oidc_subject IS NOT NULL)) permits invalid partial OIDC records that the application will have to clean up later. Fix: add the paired-nullability CHECK to enforce the invariant at the sc…
🪰 Gadfly · advisory
🟠 **users schema allows partial OIDC identity (issuer without subject or vice versa)**
_correctness · flagged by 2 models_
- **`internal/store/migrations/0001_init.sql:27`** — The `users` table allows `oidc_issuer` to be non-NULL while `oidc_subject` is NULL (or vice versa), despite the documented semantics that OIDC identity is a pair. A missing `CHECK ((oidc_issuer IS NULL AND oidc_subject IS NULL) OR (oidc_issuer IS NOT NULL AND oidc_subject IS NOT NULL))` permits invalid partial OIDC records that the application will have to clean up later. Fix: add the paired-nullability CHECK to enforce the invariant at the sc…
<sub>🪰 Gadfly · advisory</sub>
🔴buildDSN truncates filenames containing ? by treating them as URI query strings
correctness, error-handling · flagged by 4 models
internal/store/sqlite.go:66 — buildDSN unconditionally splits on ? even for bare filesystem paths. A valid path like /tmp/weird?name.db (where ? is a legal Linux filename character) is truncated to /tmp/weird and the remainder is treated as malformed query parameters and discarded. The existing test TestBuildDSNAlwaysIncludesPragmas exercises this exact input but only asserts pragma presence, not filename preservation, so the bug passes undetected. Fix: only split on ? when…
🪰 Gadfly · advisory
🔴 **buildDSN truncates filenames containing ? by treating them as URI query strings**
_correctness, error-handling · flagged by 4 models_
- **`internal/store/sqlite.go:66`** — `buildDSN` unconditionally splits on `?` even for bare filesystem paths. A valid path like `/tmp/weird?name.db` (where `?` is a legal Linux filename character) is truncated to `/tmp/weird` and the remainder is treated as malformed query parameters and discarded. The existing test `TestBuildDSNAlwaysIncludesPragmas` exercises this exact input but only asserts pragma presence, not filename preservation, so the bug passes undetected. Fix: only split on `?` when…
<sub>🪰 Gadfly · advisory</sub>
🟠buildDSN loses filename when bare path contains literal '?'
correctness · flagged by 1 model
internal/store/sqlite.go:72-95 — buildDSN mis-splits bare paths on ?, discarding part of the filename. For a bare path such as /tmp/weird?name.db (where ? is a literal character in the Unix filename), strings.Cut(body, "?") treats name.db as the query string. Because url.ParseQuery("name.db") fails, the query remains empty and the database is created at /tmp/weird instead of the intended path. The existing test (TestBuildDSNAlwaysIncludesPragmas) only asserts pragma pre…
🪰 Gadfly · advisory
🟠 **buildDSN loses filename when bare path contains literal '?'**
_correctness · flagged by 1 model_
- **`internal/store/sqlite.go:72-95` — `buildDSN` mis-splits bare paths on `?`, discarding part of the filename.** For a bare path such as `/tmp/weird?name.db` (where `?` is a literal character in the Unix filename), `strings.Cut(body, "?")` treats `name.db` as the query string. Because `url.ParseQuery("name.db")` fails, the query remains empty and the database is created at `/tmp/weird` instead of the intended path. The existing test (`TestBuildDSNAlwaysIncludesPragmas`) only asserts pragma pre…
<sub>🪰 Gadfly · advisory</sub>
🟡LastInsertId error silently discarded in test, leads to confusing FK failure on driver error
error-handling · flagged by 1 model
internal/store/store_test.go:89,95 — res.LastInsertId() errors are silently ignored (uid, _ := ...). If the driver ever returns an error, the test continues with uid=0, and the subsequent FK insert fails with a misleading foreign-key violation message instead of the actual LastInsertId error. Fix: Assert the error in tests: uid, err := res.LastInsertId(); if err != nil { t.Fatalf(...) }.
🪰 Gadfly · advisory
🟡 **LastInsertId error silently discarded in test, leads to confusing FK failure on driver error**
_error-handling · flagged by 1 model_
* **`internal/store/store_test.go:89,95`** — `res.LastInsertId()` errors are silently ignored (`uid, _ := ...`). If the driver ever returns an error, the test continues with `uid=0`, and the subsequent FK insert fails with a misleading foreign-key violation message instead of the actual `LastInsertId` error. **Fix:** Assert the error in tests: `uid, err := res.LastInsertId(); if err != nil { t.Fatalf(...) }`.
<sub>🪰 Gadfly · advisory</sub>
🟠buildUrl produces malformed URL when path omits leading slash
error-handling · flagged by 1 model
web/src/lib/api.ts:47 — buildUrl concatenates BASE + path without ensuring path starts with /. If a caller passes a naked path like apiFetch<Health>('healthz'), the resulting URL is /api/v1healthz (missing slash), which will always 404. This is an unhandled edge case for malformed input. Fix: normalize with a leading slash, e.g. const url = BASE + (path.startsWith('/') ? path : '/' + path), or assert in dev.
🪰 Gadfly · advisory
🟠 **buildUrl produces malformed URL when path omits leading slash**
_error-handling · flagged by 1 model_
- **`web/src/lib/api.ts:47`** — `buildUrl` concatenates `BASE + path` without ensuring `path` starts with `/`. If a caller passes a naked path like `apiFetch<Health>('healthz')`, the resulting URL is `/api/v1healthz` (missing slash), which will always 404. This is an unhandled edge case for malformed input. **Fix:** normalize with a leading slash, e.g. `const url = BASE + (path.startsWith('/') ? path : '/' + path)`, or assert in dev.
<sub>🪰 Gadfly · advisory</sub>
🟡null body serialized as JSON 'null' because guard uses !== undefined instead of != null
error-handling · flagged by 1 model
web/src/lib/api.ts:89 — A null request body is serialized as the JSON string "null" because the guard is body !== undefined instead of body != null. Callers often use null to mean “no body,” but here it triggers Content-Type: application/json and sends a literal null payload, which can break server-side handlers expecting an object or no body at all. Fix: Change the guard to body != null so both null and undefined are treated as absent.
🪰 Gadfly · advisory
🟡 **null body serialized as JSON 'null' because guard uses !== undefined instead of != null**
_error-handling · flagged by 1 model_
* **`web/src/lib/api.ts:89`** — A `null` request body is serialized as the JSON string `"null"` because the guard is `body !== undefined` instead of `body != null`. Callers often use `null` to mean “no body,” but here it triggers `Content-Type: application/json` and sends a literal `null` payload, which can break server-side handlers expecting an object or no body at all. **Fix:** Change the guard to `body != null` so both `null` and `undefined` are treated as absent.
<sub>🪰 Gadfly · advisory</sub>
🟠parseBody's res.text() runs outside apiFetch's try/catch, so a body-read failure rejects with a raw TypeError instead of an ApiError, breaking the wrapper's status/body contract that the optimistic editor depends on
error-handling · flagged by 2 models
web/src/lib/api.ts:108 — body parsing can throw a raw error instead of ApiError, breaking the wrapper's central contract.parseBody(res) calls await res.text() at line 60 (and JSON.parse(text) at line 65, though that one is guarded by a try/catch that falls back to text). The parseBody(res) call at line 108 is outside the try/catch in apiFetch, which only wraps the fetch at lines 92–106. So if the body stream fails mid-read (connection drop, aborted after headers, proxy…
🪰 Gadfly · advisory
🟠 **parseBody's res.text() runs outside apiFetch's try/catch, so a body-read failure rejects with a raw TypeError instead of an ApiError, breaking the wrapper's status/body contract that the optimistic editor depends on**
_error-handling · flagged by 2 models_
- **`web/src/lib/api.ts:108` — body parsing can throw a raw error instead of `ApiError`, breaking the wrapper's central contract.** `parseBody(res)` calls `await res.text()` at line 60 (and `JSON.parse(text)` at line 65, though that one is guarded by a `try/catch` that falls back to text). The `parseBody(res)` call at line 108 is outside the `try/catch` in `apiFetch`, which only wraps the `fetch` at lines 92–106. So if the body stream fails mid-read (connection drop, aborted after headers, proxy…
<sub>🪰 Gadfly · advisory</sub>
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.
Implements Phase 0 (Scaffold) of the v1 tracking epic.
Closes #1
Closes #2
Closes #3
#1 — Backend scaffold
gitea.stevedudenhoeffer.com/steve/pansy, pure-Go deps (builds withCGO_ENABLED=0).internal/config: env loader for allPANSY_*vars with defaults (auth values plumbed for #4/#5).internal/store: modernc SQLiteOpen(WAL +busy_timeout=5000+foreign_keysvia per-connection DSN pragmas;:memory:pinned to one conn), embedded numbered-migration runner (idempotent, tracked inschema_migrations).internal/store/migrations/0001_init.sql: full initial schema —users,sessions,gardens,garden_shares,garden_objects,plants,plantings. cm everywhere,versionon user-mutable rows, CHECK-enforced enums, FK indexes, reservedshape='polygon'+points.internal/domain: structs mirroring the schema + sentinel errors (ErrNotFound,ErrForbidden,ErrVersionConflict).internal/api: gin (slog-gin+ Recovery) +GET /api/v1/healthz.cmd/pansy/main.go: config → open → migrate → serve, with graceful shutdown.#2 — Frontend scaffold
web/: Vite + React 19 + TypeScript (strict) + Tailwind 4 (@tailwindcss/vite)./login,/register,/gardens,/gardens/$gardenId,/plants) as stubs; TanStack Query provider; zod.web/src/lib/api.ts: typed/api/v1fetch wrapper throwingApiErrorthat carries HTTP status + parsed body (so the optimistic editor can read 409 conflict rows)./api→:8080; responsive app shell + mobile viewport meta.#3 — Single-binary build
internal/webdist://go:embed all:distwith a committed placeholderindex.html.internal/api/spa.go: SPA fallback — real files served (hashed assetsimmutable,index.htmlno-cache), client deep links fall back toindex.html, unmatched/api/*and missing assets return JSON 404.Makefile(web/build/dev/test) + README quickstart & env-var table.Verification
CGO_ENABLED=0 go build ./...,go vet ./...,go test ./...all clean (store + SPA tests included).healthzreturns{"ok":true}; all seven tables present with FK enforcement.tsc --noEmitclean;npm run buildemits hasheddist/./redirects to/gardens, all routes navigate client-side,/gardens/$gardenIdresolves the param, deep links + cache headers verified, renders at desktop and 375px.🤖 Generated with Claude Code
🪰 Gadfly — live review status
5/5 reviewers finished · updated 2026-07-18 19:29:13Z
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: Blocking issues found · 14 findings (4 with multi-model agreement)
internal/store/sqlite.go:63internal/store/migrations/0001_init.sql:25web/src/lib/api.ts:108internal/api/spa.go:8810 single-model findings (lower confidence)
internal/store/sqlite.go:72web/src/lib/api.ts:47README.md:36internal/config/config.go:60internal/config/config.go:68internal/domain/domain.go:40internal/store/store_test.go:89web/src/lib/api.ts:89internal/api/spa.go:60internal/store/migrate.go:93Per-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
None found. This Phase 0 scaffold has no state-changing endpoints yet and no auth logic (deferred to #4/#5), so the security surface is small. I checked the areas most likely to carry security risk: the SPA static-file handler for path traversal, the SQLite DSN builder for injection, the trusted-proxy fallback, secret handling, and the frontend fetch wrapper.
VERDICT: No material issues found
internal/api/spa.go:45-51— path traversal:path.Cleanon an absolute request path collapses any..segments to the root before thefs.Stat/fs.ReadFilelookup against the embeddeddistFS, andembed.FS/fstest.MapFSadditionally reject non-fs.ValidPathnames. Verified by reading the code and reasoning throughpath.Clean("/../../etc/passwd") == "/etc/passwd"; confirmed test coverage (TestSPAMissingAssetIs404, directory-listing test) exercises adjacent edge cases.internal/api/api.go:28-34— trusted-proxy fallback:gin.Engine.SetTrustedProxies(nil)setstrustedCIDRstonil, and gin'sisTrustedProxytreats a niltrustedCIDRsas "trust nothing" (not gin's default trust-all), so the fallback on a parse error is correctly fail-closed, not fail-open. This already reflects the fix noted in the prior "Address Gadfly review findings" commit.internal/store/sqlite.go— DSN pragma construction is built from a hardcoded pragma list andurl.Values, not from untrusted network input (PANSY_DBis operator-set); no injection vector reachable from a request.internal/config/config.go—PANSY_OIDC_CLIENT_SECRETis loaded intoConfig.OIDC.ClientSecretand never logged or serialized anywhere in this diff (grepped for allOIDC/ClientSecretusages).internal/domain/domain.go—PasswordHash,OIDCIssuer,OIDCSubjectonUserandTokenHashonSessionare all taggedjson:"-", so a future accidentalc.JSON(user)won't leak them.web/src/lib/api.ts:96— usescredentials: 'same-origin'(not'include'), which is the correct choice once cookie-based sessions land; nodangerouslySetInnerHTML/evalin the frontend scaffold.Nothing exploitable found in this PR's security-relevant surface (SPA file serving, DB DSN handling, proxy trust, secret plumbing). Auth itself isn't implemented yet (explicitly deferred to #4/#5), so there's no authn/authz logic to assess in this diff.
🎯 Correctness — No material issues found
This schema is consistent with the domain structs and matches design conventions throughout — no unit/formula/default mismatches found.
Nothing else stood out as a correctness defect in this scaffold-only PR (no business logic yet — just config loading, DB open/migrate, SPA fallback routing, and typed structs mirroring the schema). I checked the DSN pragma merging logic, in-memory detection, migration idempotency/versioning, the SPA fallback's file-vs-route disambiguation and cache-header branches, the trusted-proxies fallback, and the schema/struct field-type correspondence (nullable vs non-nullable columns matching pointer vs value fields) — all correct and internally consistent.
VERDICT: No material issues found
No correctness defects identified in this lens. The scaffold has no business logic yet, so there's little surface for domain/formula bugs; the mechanical pieces I checked (DSN pragma merging and in-memory detection in
internal/store/sqlite.go, migration versioning/idempotency ininternal/store/migrate.go, the SPA route/asset/API disambiguation ininternal/api/spa.go, and theinternal/domainstruct fields against the0001_init.sqlschema's nullability) all check out. TheConfig.RegistrationOpen()method sharing a name with theRegistrationOpenpackage constant ininternal/config/config.go:18,60is unusual but not a bug — Go resolves the bare identifier inside the method body to the package-level constant, not a recursive call, and the described commit history confirms this already builds/tests clean.🧹 Code cleanliness & maintainability — Minor issues
VERDICT: Minor issues
internal/config/config.go:60—(*Config).RegistrationOpen()is dead code:grep -rn "RegistrationOpen()" --include="*.go" .across the whole repo returns only its own definition, zero callers. It also shares its name with the package-level constantRegistrationOpen = "open"declared atconfig.go:18in the same file. This compiles (Go keeps method names and package-level identifiers in separate namespaces), butconfig.RegistrationOpen(string constant) andcfg.RegistrationOpen()(bool method) reading as the same identifier is a genuine readability trap for a future maintainer. Confirmedcfginapi.New(internal/api/api.go:22-40) is used only forTrustedProxies; the otherConfigfields (Registration,LocalAuth,OIDC,BaseURL) are unused scaffolding for issues #4/#5, consistent with the PR/README description — that part is expected and not itself a finding. Suggest dropping the unused method until #4 needs it, or renaming it (e.g.IsRegistrationOpen()) to avoid the collision.⚡ Performance — No material issues found
VERDICT: No material issues found
This is a Phase 0 scaffold PR — no data-layer queries, list endpoints, or business logic exist yet, so there's essentially no surface for N+1 queries, missing pagination, or hot-loop/quadratic work. I checked the two runtime code paths that do exist:
internal/api/spa.go:51,65—RegisterSPA'sNoRoutehandler callsfs.Stat(dist, name)to decide cache headers/directory-vs-file, then hands off tohttp.FileServer, which internally does its ownOpen+Staton the same embedded file. That's a duplicate stat per static-asset request, butdistis an in-memoryembed.FS, so the second stat is a cheap map lookup, not a syscall — not worth flagging as a regression.fileServerand the cachedindexbytes are both built once inRegisterSPA, not per-request, so no repeated allocation there. Verified by reading the full file.internal/store/sqlite.go:40-57—Openpins:memory:DBs to a single connection (correct, and necessary for correctness, not a perf issue) and leaves the file-backed pool at Go's default (unlimited open conns,busy_timeout=5000). That's standard practice for SQLite+WAL and not a regression introduced by this PR.internal/store/migrate.goandcmd/pansy/main.go— migration loading/execution and server bootstrap only run once at startup, not on a hot path.web/src/lib/api.ts,web/src/main.tsx— thin fetch wrapper and aQueryClientwith sane defaults (staleTime: 30_000, no polling). No loops or repeated work of concern.Nothing here rises to a material performance finding.
🧯 Error handling & edge cases — Minor issues
Both findings confirmed.
buildDSN(internal/store/sqlite.go:68) does silently swallow theurl.ParseQueryerror with no logging, unlike every other invalid-input path inconfig.gowhich callsslog.Warn; andstore_test.gohas no test for a malformed query string. Inapi.ts:108,parseBody(res)is indeed called outside thetryblock (lines 92-106) that wrapsfetch(), so a rejection fromres.text()would bypass theApiErrorwrapping.Both are real, narrow findings. Verdict and findings stand as drafted.
VERDICT: Minor issues
internal/store/sqlite.go:68—buildDSNsilently swallows the error fromurl.ParseQuery(rawQuery):if q, err := url.ParseQuery(rawQuery); err == nil { query = q }. If a caller setsPANSY_DBto a DSN with a malformed query string (e.g. a stray%not followed by two hex digits),err != nil,querystays the zero-valueurl.Values{}, and any user-supplied pragmas/params in that query are silently discarded — no log, no error returned fromOpen. The required pragmas (busy_timeout,journal_mode,foreign_keys) still get added, so the DB opens "successfully" with different settings than the operator intended, with no signal anything went wrong. This is inconsistent withinternal/config/config.go, where every other invalid-input path (envInt,envBool, invalidPANSY_REGISTRATION, out-of-rangePANSY_PORT) logsslog.Warnon fallback.store_test.gohas no test exercising a malformed query string. Suggested fix: onParseQueryerror,slog.Warn(or return the error fromOpen) instead of silently continuing with an empty query.web/src/lib/api.ts:108—const payload = await parseBody(res)runs outside thetry { … } catch { throw new ApiError(...) }block that wrapsfetch()(lines 92–106).parseBodycallsres.text(), which can reject if the connection drops mid-response. In that case the rejection propagates as a raw error, not anApiError, breaking the contract described in the file's own header comment (callers branch onerror instanceof ApiError/.isConflict/.isUnauthorized). Narrow edge case (no write endpoints exist yet in this PR), but worth wrapping theparseBodycall to keep theApiErrorcontract intact for a body-read failure after the response starts.glm-5.2:cloud (ollama-cloud) — Minor issues
Verdict: Minor issues — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — No material issues found
VERDICT: No material issues found
Through the security lens, I examined the untrusted-input surfaces introduced by this scaffold and found nothing materially wrong at this stage:
internal/api/spa.go): directory listing is explicitly blocked (dir → 404), API misses return JSON not HTML, asset misses don't fall back to index.html (avoids the HTML-for-.js MIME confusion), andX-Content-Type-Options: nosniffis set on the SPA path.path.Cleanis applied to the request path beforefs.Stat, which also defeats..traversal in the embedded FS. Verified by reading the file.internal/api/api.go:28-34): on parse failure it falls back to trusting none rather than leaving gin's trust-everyone default active — correct, since the latter would allow arbitraryX-Forwarded-Forspoofing. Verified.internal/store/migrate.go,sqlite.go): migration SQL is fully static/embedded (no interpolation of env or request data); the DSN is built from the operator-controlledPANSY_DBenv var only. No injection vector from untrusted input here.OIDC.ClientSecretlives in the config struct and is not logged anywhere I could find (grep found noslog/log line touchingClientSecret). TheUser/Sessiondomain structs tagPasswordHash,OIDCIssuer,OIDCSubject, andTokenHashwithjson:"-"so they won't serialize to API responses. Verified.GET /api/v1/healthzplus the SPA; there are no authn/authz endpoints yet (deferred to #4/#5). Registration defaulting toopenis a documented, intentional local-dev default, not a defect.Two non-blocking observations outside the security lens (no action required from this review): no global security headers (
Content-Security-Policy,Strict-Transport-Security,X-Frame-Options) are applied yet — reasonable to defer until the app shell is real; andhealthzis unauthenticated by design as a liveness probe.🎯 Correctness — No material issues found
VERDICT: No material issues found
I reviewed the diff through the correctness lens and verified the suspect areas against the checked-out code.
What I verified and found correct:
internal/store/sqlite.go:buildDSN—#-escaping happens after query parsing (so it can't corrupt the?-split), memory detection is precise (:memory:exact token ormode=memory, not a substring), and de-duplication keys on pragma name only. The critical concern — thaturl.Values.Encode()percent-encodes the_pragmavalues (busy_timeout%285000%29) — is not a bug:store_test.go(TestForeignKeysEnforcedfor a bare path andTestForeignKeysEnforcedForFileURIDSNfor afile:URI) actually open a DB and assert FK enforcement, which only passes if modernc URL-decodes the_pragmavalues back tobusy_timeout(5000). Confirmed.internal/store/migrate.go— idempotent runner, per-migration transaction with deferred rollback, version parsed from filename prefix, duplicate-version detection. Multi-statement SQL exec works (the test asserts all 7 tables +schema_migrationsexist after one run).internal/api/spa.go—path.Cleancollapses..beforefs.Stat, andfs.Sub(dist,…)roots the FS atdist, so there's no path-traversal vector; API paths short-circuit before the SPA; missingassets/returns JSON 404 (no HTML-200 MIME trap); directories 404 instead of listing. Theindex.htmlis read once at startup.internal/domain/domain.govsmigrations/0001_init.sql— nullable schema columns (points,color,props,plants.owner_id,plantings.count/label/planted_at/removed_at) map to pointer fields; enums match CHECK constraints;versiondefaults align. The reservedshape='polygon'is allowed by the CHECK and carries a matching domain constant, consistent with the "post-v1" comment.cmd/pansy/main.go— graceful shutdown:serveErris buffered(1) so the goroutine never blocks;stop()is idempotent; migration uses a bounded context.web/src/lib/api.ts— JSON serialization happens beforefetch(so a circular-body failure isn't misreported as a network error);AbortErroris re-thrown unchanged; 204/empty bodies parse tonull.One unverified minor note (not reported as a finding):
serveIndexusesc.Datafor HEAD requests while the comment claims net/http suppresses the body; I could not confirm Go's server strips the body for HEAD here, but even if it doesn't this is a protocol nit with no functional impact on a SPA scaffold, so it doesn't rise to a material correctness issue.🧹 Code cleanliness & maintainability — Minor issues
Verdict: Minor issues
The scaffold is clean overall — DRY page stubs via the shared
PageStubcomponent, well-organized domain constants, a nicely factoredbuildDSN, and good doc comments. A couple of small maintainability nits:internal/api/spa.go:88—writeAPIErroris described as "pansy's standard JSON error envelope" (a package-wide concern, and the exact shape the frontendapi.tsmessageFromalready parses), yet it lives in the SPA-fallback file. As theapipackage grows real handlers (the package doc inapi.go:3-5explicitly says handlers stay thin and call a service layer), every one of them will reach for this helper and import it from the SPA file — a leaky placement. Confirmed againstapi.go(onlyhealthztoday), so the helper is reachable but misplaced. Suggest moving it (alongside the"NOT_FOUND"/"METHOD_NOT_ALLOWED"codes) into a small sharedapi/errors.gosospa.goand future handler files both depend on it rather than on each other.internal/api/spa.go:60andinternal/api/spa.go:72—strings.HasPrefix(name, "assets/")is evaluated twice in the same handler, once to set immutable cache headers and once to short-circuit a missing-asset 404. The two branches are distinct, but the repeated prefix literal is the kind of thing that drifts (e.g. if Vite is ever configured with a differentassetsDir). A one-lineisAsset := strings.HasPrefix(name, "assets/")local would keep them in sync and reads more clearly. Trivial.web/src/pages/*— five nearly-identical one-line wrappers (export function XPage() { return <PageStub …>…</PageStub> }). This is fine for a scaffold and the sharedPageStubis the right factoring; no change needed, just noting it's intentional duplication that should collapse as the real pages land.Nothing else in my lens: naming is consistent (
xCm/widthCmJSON tags match the schemax_cm/width_cm), no dead code, no overly long functions, and the migration runner is well decomposed (appliedVersions/applyMigration/loadMigrations/parseVersion).⚡ Performance — No material issues found
I've reviewed the hot paths through the performance lens. This is a scaffold; the only request paths are
/healthz(no DB) and the embedded SPA fallback. Let me confirm the SPA stat cost is in-memory and there's no per-request DB work.The SPA handler does
fs.Stat(dist, name)thenfileServer.ServeHTTP(which stats again internally) — butdistis anembed.FS(in-memory map lookups), so this is negligible, not a material regression.index.htmlis correctly read once at startup. Migrations run once at boot. The DB pool isn't queried by any handler yet.One forward-looking note (not material to this PR's hot paths): for file-backed DBs
Opensets noSetMaxOpenConnscap (only:memory:is pinned to 1). Under future concurrent write load an unbounded SQLite pool can churn connections and starve the single writer, but since no handler touches the DB yet this isn't a current regression.Verdict: No material issues found
internal/store/sqlite.go:40— file-backed DBs get an unboundedsql.DBconnection pool (noSetMaxOpenConns). Modernc SQLite connections are relatively heavyweight and WAL still serializes writers; once request handlers query the DB, consider capping open conns (e.g. 1 writer + a small read pool) to avoid connection churn under load. Verified: only:memory:is pinned (sqlite.go:50); no handler currently uses the DB (only/healthzregistered inapi.go:42). Not a regression in this PR's actual hot paths.🧯 Error handling & edge cases — Minor issues
The draft's single finding is confirmed against the actual code:
internal/store/migrate.go:93returnstx.Commit()unwrapped, while every other error path inapplyMigration(lines 81, 86, 91) wraps with the migration version/name. The surrounding correctness claims in the draft (deferredtx.Rollback,rows.Err()check, close-on-defer) all hold against the source.VERDICT: Minor issues
internal/store/migrate.go:93—applyMigrationreturnstx.Commit()unwrapped, so a commit failure (e.g. disk full mid-migration) is reported without identifying which migration version failed, unlike every other error in this function which wraps with the version/name. Impact is diagnostic clarity only; the underlying SQLite error is still surfaced. Suggested fix:if err := tx.Commit(); err != nil { return fmt.Errorf("store: commit migration %d (%s): %w", m.version, m.name, err) }; return nil.kimi-k2.6:cloud (ollama-cloud) — Minor issues
Verdict: Minor issues — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — No material issues found
Verdict: No material issues found
I verified the security-sensitive surfaces introduced in this scaffold:
internal/api/spa.go:45):path.Clean+strings.TrimPrefixprevents directory traversal. The embeddedfs.FSboundary meanshttp.FileServercannot escape the build artifact root. Directory listings are explicitly rejected (info.IsDir()→ 404).PANSY_OIDC_CLIENT_SECRETis read server-side only ininternal/config/config.go:50and never logged or returned. Thevite.config.tsproxy only needs the backend port — no OIDC secrets reach the browser.//go:embed migrations/*.sql(trusted build artifacts), not constructed from user input. The DSN builder ininternal/store/sqlite.gousesurl.Valuesencoding;filePart#is escaped. No dynamic SQL concatenation exists.internal/api/api.go:28): On parse failure it explicitly falls back to trusting no proxies, avoiding gin’s trust-everyone default.X-Content-Type-Options: nosniff.Cache-Controlon hashed assets is correctlyimmutable;index.htmlisno-cache.Nothing in this diff presents an authn/authz gap, injection vector, SSRF, unsafe deserialization, credential leakage, or insecure default that rises above the “scaffold with no auth endpoints yet” stage.
🎯 Correctness — Minor issues
Minor issues
internal/store/sqlite.go:72-95—buildDSNmis-splits bare paths on?, discarding part of the filename.For a bare path such as
/tmp/weird?name.db(where?is a literal character in the Unix filename),strings.Cut(body, "?")treatsname.dbas the query string. Becauseurl.ParseQuery("name.db")fails, the query remains empty and the database is created at/tmp/weirdinstead of the intended path. The existing test (TestBuildDSNAlwaysIncludesPragmas) only asserts pragma presence, so the bug is masked.Fix: only split on
?when the input already starts withfile:; for bare paths percent-encode?(e.g.strings.ReplaceAll(filePart, "?", "%3F")) before constructing the URI.internal/store/migrations/0001_init.sql:25-26—users.oidc_issuer/oidc_subjectallow semantically invalid partial-NULL states.The schema permits a row where one column is NULL and the other is NOT NULL (e.g. issuer set without subject, or vice versa). That state is meaningless for OIDC identity linkage and could be created by a bug in the auth code planned for #5.
Fix: add a
CHECKconstraint, e.g.🧹 Code cleanliness & maintainability — No material issues found
No material issues found
⚡ Performance — No material issues found
No material issues found
I scrutinized the PR for efficiency regressions, allocation hotspots, unbounded growth, blocking calls, and connection/resource limits. The scaffold is clean:
internal/api/spa.go): Pre-readsindex.htmlonce at startup;fs.Stat+http.FileServerfor static assets is standard and the embedded FS keeps it in-memory. No redundant I/O or copying on hot paths.internal/store/migrate.go): Loads migrations once at boot; schema version table is bounded. No loops or allocations that scale with runtime data.web/src/lib/api.ts): StraightforwardfetchwithURLSearchParams; response parsing is O(response size). No pagination needed yet (no list endpoints in this scaffold).web/src/main.tsx):staleTime: 30_000andrefetchOnWindowFocus: falseare sensible defaults that prevent excessive refetching.The only latent concern is that
store.Opendoes not cap thesql.DBconnection pool for file-backed databases (MaxOpenConnsstays at Go’s default of 0, i.e., unlimited). SQLite serializes writers, so under concurrent load an unbounded pool can cause many connections to spin onbusy_timeout(5000)rather than queueing gracefully. However, since Phase 0 has no write endpoints and the actual entity query methods are coming in later issues, this is not yet a material regression — it will naturally be addressed when the service layer is wired up and connection limits are tuned for the expected concurrency.🧯 Error handling & edge cases — Minor issues
Minor issues
web/src/lib/api.ts:47—buildUrlconcatenatesBASE + pathwithout ensuringpathstarts with/. If a caller passes a naked path likeapiFetch<Health>('healthz'), the resulting URL is/api/v1healthz(missing slash), which will always 404. This is an unhandled edge case for malformed input.Fix: normalize with a leading slash, e.g.
const url = BASE + (path.startsWith('/') ? path : '/' + path), or assert in dev.internal/store/sqlite.go:63-89—buildDSNsplits the supplied path on the first?to separate the file part from query parameters. A bare filename that literally contains?(valid on Unix) is mis-parsed:weird?name.dbbecomesfile:weird?name.db=…, so SQLite opens a database namedweirdinstead ofweird?name.db. The docstring explicitly says it accepts bare filenames, so this is an unhandled edge case.Fix: only parse query parameters when the input is already a
file:URI; for bare filenames, treat the entire string as the path and append the pragmas.opencode/glm-5.2:cloud (opencode) — Minor issues
Verdict: Minor issues — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — No material issues found
Through the 🔒 Security lens, I read the actual checked-out code for
internal/api/{api.go,spa.go},internal/config/config.go,internal/store/{sqlite.go,migrate.go,migrations/0001_init.sql},cmd/pansy/main.go,internal/webdist/dist.go,web/vite.config.ts, andweb/src/lib/api.tsto verify each concern.Verification notes:
internal/api/api.go:28-34): onSetTrustedProxiesparse failure the code explicitly falls back toSetTrustedProxies(nil)(trust none) rather than letting gin's trust-everyone default stay active — correct, no spoofing path.internal/api/spa.go:45-67):path.Cleanis applied beforeTrimPrefix("/"), so../segments collapse; thefs.Statis over an embeddedfstest/embed.FSwhich cannot escape. Directory entries are explicitly 404'd (no listing leak). Missingassets/*returns 404 (no HTML-as-JS MIME confusion). Fine.OIDC.ClientSecretis read from env into the struct and never logged (grepped allslog.*call sites — only paths/ports/versions/errors are emitted).PasswordHash/OIDCIssuer/OIDCSubjectare taggedjson:"-". Good.//go:embed migrations/*.sql; no user-controlled input reachestx.ExecContext(m.sql). Version parsing is on embedded filenames. No injection surface.internal/store/sqlite.go:63-90): the only tainted-ish input isPANSY_DB, which is operator-controlled, not untrusted. The#→%23escape prevents fragment truncation of the pragma query. There is a theoretical foot-gun where an operator supplying_pragma=foreign_keys(0)would suppress the requiredforeign_keys(1)due to the de-dup by name, but that is operator self-disabling, not an attacker vector — out of scope for a security finding.cmd/pansy/main.go:53-60): no TLS, but Phase 0 is explicitly intended to sit behind a reverse proxy (per README/DESIGN context); no plaintext-secret transmission occurs in this scaffold (no auth endpoints yet). Not a Phase-0 defect.X-Content-Type-Options: nosniffis set on the SPA fallback path; API JSON errors usegin.JSONwhich pinsapplication/json. Fine.VERDICT: No material issues found
🎯 Correctness — Minor issues
Review
VERDICT: Minor issues
Through the correctness lens I verified the migration runner, DSN builder, SPA fallback, config loader, schema, and domain types by reading the actual checked-out code (not just the diff).
Findings
internal/store/sqlite.go:66—buildDSNmis-parses a?embedded in the filesystem path. Confirmed by tracing the code against the actual source: for input/tmp/weird?name.db(the test case atstore_test.go:132),strings.TrimPrefixleavesbody = "/tmp/weird?name.db", thenstrings.Cut(body, "?")splits it intofilePart="/tmp/weird"andrawQuery="name.db".url.ParseQuery("name.db")succeeds and returns{"name.db": [""]}. The pragmas are then added,query.Encode()emits_pragma=...&name.db=, and the final DSN isfile:/tmp/weird?_pragma=busy_timeout%285000%29&_pragma=foreign_keys%281%29&_pragma=journal_mode%28WAL%29&name.db=. modernc treats everything after?as query parameters, so the DB file actually opened is/tmp/weird— not the configured/tmp/weird?name.db. The test atstore_test.go:126-141only asserts the returned string contains the three pragma names viastrings.Contains; it never opens the resulting DSN and checks that the file is created at the intended path, so it masks the corruption. The#escaping at line 88 (to avoid it starting a fragment) is asymmetric:?is split before any escaping could protect it, so a?in a plain filesystem path can't be recovered. Real-world paths rarely contain?, so impact is low, but the test gives a false sense of coverage. Suggested fix: only split on?when the input is afile:URI (where?legitimately starts a query); for a bare filesystem path, treat the whole string as the filename and don't synthesize a query section.Things I checked and found correct
migrate.go): idempotent, transactional, version dedup, ascending sort, sentinel-table creation — all verified.applyMigrationrolls back on failure (deferred) and commits only after recording the version.0001_init.sql): CHECK enums matchdomainconstants; FK actions consistent (CASCADE on owner/parent, RESTRICT onplantings.plant_id);idx_users_oidcUNIQUE on(oidc_issuer, oidc_subject)allows multiple NULL pairs under SQLite's NULL-distinct semantics.:memory:coherence:SetMaxOpenConns(1)pins the pool so a second connection sees the migrated schema;TestInMemoryDBIsCoherentAcrossQueriesproves it.spa.go): theassets/missing-file 404 (no HTML-for-.js), directory-as-404,/api/*JSON 404, and non-GET 405 branches all behave as claimed and are covered by tests.path.Clean+TrimPrefix("/")correctly handles..and trailing slashes.config.go): port range clamp, registration enum validation with fallback toopen,envStrtreating empty string as unset — all correct.domainstruct JSON tags and pointer/omitemptyusage mirror the schema's nullable columns.I could not independently verify how modernc's
Openparses a relativefile:./pansy.db?...DSN (module cache not materialized), but the project's own tests open a file-backed./test.dband assert FK enforcement reaches the pool, which transitively exercises the produced DSN end-to-end for the common cases.🧹 Code cleanliness & maintainability — Minor issues
I've examined the structure thoroughly. Let me check a couple more things about the API error envelope consistency between the Go side and the TS side.
I have enough now. Let me think through cleanliness/maintainability findings.
Let me check the unused
TestBuildDSN...and confirmapiimport alias is unused in main:VERDICT: Minor issues
Code is generally clean and well-structured. A few small maintainability nits worth addressing:
internal/api/spa.go:88-91—writeAPIErroris misplaced. It is pansy's standard JSON error envelope helper ({"error":{"code":...,"message":...}}), not an SPA-specific concern. Every future API handler (per the package doc inapi.go:3-5— "decode, call the service layer, encode") will need it, but it's currently tucked at the bottom of the SPA fallback file. Move it (and its doc comment) intoapi.goso it lives with the rest of the API surface; otherwise each newinternal/api/*.gowill either reach across tospa.gofor it or duplicate it. Verified by reading both files.internal/domain/domain.go:38-40—ShapePolygonconstant is dead code today. The schema and domain comment explicitly saypolygon/pointsare "reserved for post-v1; the v1 editor emits only 'rect' and 'circle'" (schema comment,internal/store/migrations/0001_init.sql:69-70). Exposing the constant in the public domain surface now invites callers to use a value the editor and (per the diff) the API will reject. Either drop it from the v1 set and add it when the polygon issue lands, or at least document it as "reserved, do not emit." Minor, but it's the kind of "plausible-looking magic value" that breeds a real bug later.README.mdenv-var table —PANSY_REGISTRATION: closedis documented as usable, but nothing consumesclosedyet.config.RegistrationOpen()exists (internal/config/config.go:60-62) and there is noRegistrationClosed()/IsClosed()accessor; the only call site in Phase 0 ishealthz. The README presentsclosedas a working option ("openorclosed— gates local self-service signup"). Either land the gating in this PR's scope (it isn't — auth is #4/#6) or annotate the row as "plumbed for #4" the way the OIDC rows are, so operators don't setPANSY_REGISTRATION=closedexpecting it to do something. (Verified by grepping forRegistrationOpen/RegistrationClosedcallers.)No material issues with the rest: the DSN builder, migration runner, SPA fallback, and
api.tswrapper are all readable, follow the surrounding patterns, and don't carry duplicated/dead code beyond the above.⚡ Performance — Minor issues
All three draft findings are confirmed as accurate but explicitly described as non-actionable/not-real-concerns. The draft itself concludes "Nothing material in the performance lens" with an empty findings array. The findings listed are observations, not actual issues. Per verification, the code matches the draft's descriptions exactly. The verdict reflects "Minor issues" but with an empty findings array — the three bullets are explicitly non-actionable. Since no actionable findings survive, the verdict should be "No material issues found" with an empty findings array.
VERDICT: No material issues found
🧯 Error handling & edge cases — Minor issues
VERDICT: Minor issues
web/src/lib/api.ts:108— body parsing can throw a raw error instead ofApiError, breaking the wrapper's central contract.parseBody(res)callsawait res.text()at line 60 (andJSON.parse(text)at line 65, though that one is guarded by atry/catchthat falls back to text). TheparseBody(res)call at line 108 is outside thetry/catchinapiFetch, which only wraps thefetchat lines 92–106. So if the body stream fails mid-read (connection drop, aborted after headers, proxy reset),apiFetchrejects with the rawTypeErrorrather than anApiErrorcarryingstatus/body. The whole point of the wrapper (per the file's own doc comment at lines 3–7) is that callers can readApiError.status/.isConflict/.isUnauthorized; a non-ApiErrorrejection defeats the optimistic editor's 409 reconciliation path. Suggested fix: wrapparseBody(res)in atry/catchand re-throw asnew ApiError('Malformed response from the pansy server.', res.status, null)so theApiErrorinvariant holds on every rejection out ofapiFetch. Verified by readingapi.tslines 58–113.All other error paths I checked are handled correctly:
cmd/pansy/main.go: server error vs signal shutdown handled inselect;defer db.Close()runs afterShutdownreturns (correct ordering);serveErrchannel is buffered so the goroutine never blocks.internal/store/migrate.go:defer tx.Rollback()with//nolint:errcheckafter commit — standard idiom;rows.Err()checked after iteration;appliedVersionserrors propagate.internal/store/sqlite.go:Pingfailure closes the DB before returning;:memory:pinned to one conn.internal/api/spa.go: missingindex.htmlpanics at startup (appropriate for a packaging bug); directory stat → 404 (no listing leak);path.Clean+ embedded FS prevent traversal.internal/config/config.go: invalid port/registration fall back to defaults with aslog.Warn; never returns an error (documented behavior).internal/api/api.go:SetTrustedProxiesparse failure explicitly falls back tonil(never leaves the trust-everyone default).opencode/kimi-k2.6:cloud (opencode) — Blocking issues found
Verdict: Blocking issues found — 5 reviewers: security, correctness, maintainability, performance, error-handling
🔒 Security — No material issues found
No material issues found
Reviewed through the Security lens. The Phase 0 scaffold has a solid security posture for its scope:
path.Cleanplusfs.Sub-rooted embedded FS blocks directory traversal;..segments are resolved away and the sub-FS rejects escapes. No path traversal vector found.?). No SQL injection from untrusted input exists yet (no user-facing mutating endpoints).SetTrustedProxiessafely falls back to trusting none on parse failure, preventingX-Forwarded-Forspoofing.X-Content-Type-Options: nosniffis present on SPA responses; hashed assets getimmutablecache directives whileindex.htmlgetsno-cache, mitigating cache poisoning on redeploys.dangerouslySetInnerHTML,eval,document.write, or similar sinks.Nothing in this change constitutes a material authn/authz gap, injection, SSRF, deserialization, secret leakage, or insecure default at this scaffold stage.
🎯 Correctness — Blocking issues found
Blocking issues found
internal/store/sqlite.go:66—buildDSNunconditionally splits on?even for bare filesystem paths. A valid path like/tmp/weird?name.db(where?is a legal Linux filename character) is truncated to/tmp/weirdand the remainder is treated as malformed query parameters and discarded. The existing testTestBuildDSNAlwaysIncludesPragmasexercises this exact input but only asserts pragma presence, not filename preservation, so the bug passes undetected. Fix: only split on?when the path is a URI (starts withfile:), or escape?in the file part before reconstructing the DSN.internal/store/migrations/0001_init.sql:27— Theuserstable allowsoidc_issuerto be non-NULL whileoidc_subjectis NULL (or vice versa), despite the documented semantics that OIDC identity is a pair. A missingCHECK ((oidc_issuer IS NULL AND oidc_subject IS NULL) OR (oidc_issuer IS NOT NULL AND oidc_subject IS NOT NULL))permits invalid partial OIDC records that the application will have to clean up later. Fix: add the paired-nullability CHECK to enforce the invariant at the schema level.🧹 Code cleanliness & maintainability — Minor issues
Minor issues
internal/config/config.go:68,88-89— The default port literal8080is duplicated: once as the fallback argument toenvInt, and again in the validation block's log message and reassignment. If the default ever changes, all three sites must be updated together or they drift. Extract a package-level constant (defaultPort = 8080) and reference it everywhere.⚡ Performance — No material issues found
No material issues found
Performance lens review:
internal/api/spa.go): Theindex.htmlis read once at startup and cached in memory — good. Asset files served viahttp.FileServerget immutable cache headers. No redundant allocations or I/O in the hot path.internal/store/sqlite.go): File-backed databases use the defaultsql.DBpool. While an explicitSetMaxOpenConnscould cap connections under extreme load, this is a self-hosted garden planner in Phase 0 with no query layer yet, so the default behavior is not a material concern.:memory:is correctly pinned to one connection.internal/store/migrate.go): Migrations load from the embedded FS once at startup, sort, and apply idempotently. No quadratic behavior or N+1 issues —schema_migrationsis tiny.web/src/lib/api.ts): Reads the full response body before parsing, but this is standard for a JSON API wrapper and not material for expected payload sizes.web/src/router.tsx): UsesdefaultPreload: 'intent'for route preloading, which is a performance optimization. QueryClient disables window-focus refetching and sets a 30s stale time — sensible defaults.🧯 Error handling & edge cases — Minor issues
Minor issues found.
internal/store/sqlite.go:68—url.ParseQueryerror is silently swallowed inbuildDSN. If a user provides a path with malformed query parameters (e.g., invalid percent-encoding likefile:x.db?mode=memory%ZZ), the raw query is discarded and replaced with an empty map. This drops any user-supplied_pragmaoverrides and, more importantly, can miss amode=memoryparameter, causing the in-memory detection (memory = ... || query.Get("mode") == "memory") to returnfalseand skip theSetMaxOpenConns(1)pin that keeps an in-memory DB coherent across the pool.Fix: Surface the parse error:
if q, err := url.ParseQuery(rawQuery); err != nil { return "", false, fmt.Errorf("invalid DSN query: %w", err) }.web/src/lib/api.ts:89— Anullrequest body is serialized as the JSON string"null"because the guard isbody !== undefinedinstead ofbody != null. Callers often usenullto mean “no body,” but here it triggersContent-Type: application/jsonand sends a literalnullpayload, which can break server-side handlers expecting an object or no body at all.Fix: Change the guard to
body != nullso bothnullandundefinedare treated as absent.internal/store/store_test.go:89,95—res.LastInsertId()errors are silently ignored (uid, _ := ...). If the driver ever returns an error, the test continues withuid=0, and the subsequent FK insert fails with a misleading foreign-key violation message instead of the actualLastInsertIderror.Fix: Assert the error in tests:
uid, err := res.LastInsertId(); if err != nil { t.Fatalf(...) }.Automated adversarial review by Gadfly — consensus across the model swarm. Advisory only — does not block merge.
🪰 Gadfly consensus review — 14 inline findings on changed lines. See the consensus comment for the full ranked summary.
Advisory only — does not block merge.
@@ -7,0 +33,4 @@```Open http://localhost:8080 — one process serves both the JSON API and the app.🟡 PANSY_REGISTRATION=closed is documented as a working option but no code consumes it in Phase 0; row should be marked 'plumbed for #4' like the OIDC rows
maintainability · flagged by 1 model
🪰 Gadfly · advisory
@@ -0,0 +57,4 @@}// Vite emits content-hashed filenames under assets/, so those are safe// to cache forever; index.html must always revalidate to pick up new builds.if strings.HasPrefix(name, "assets/") {⚪ "assets/" prefix literal checked twice in same handler; extract a local to avoid drift
maintainability · flagged by 1 model
internal/api/spa.go:60andinternal/api/spa.go:72—strings.HasPrefix(name, "assets/")is evaluated twice in the same handler, once to set immutable cache headers and once to short-circuit a missing-asset 404. The two branches are distinct, but the repeated prefix literal is the kind of thing that drifts (e.g. if Vite is ever configured with a differentassetsDir). A one-lineisAsset := strings.HasPrefix(name, "assets/")local would keep them in sync and reads more clearly. Trivial.🪰 Gadfly · advisory
@@ -0,0 +85,4 @@c.Data(http.StatusOK, "text/html; charset=utf-8", index)}// writeAPIError writes pansy's standard JSON error envelope.🟡 writeAPIError (standard JSON error envelope) lives in the SPA file; should move to a shared api/errors.go as handlers land
maintainability · flagged by 2 models
internal/api/spa.go:88—writeAPIErroris described as "pansy's standard JSON error envelope" (a package-wide concern, and the exact shape the frontendapi.tsmessageFromalready parses), yet it lives in the SPA-fallback file. As theapipackage grows real handlers (the package doc inapi.go:3-5explicitly says handlers stay thin and call a service layer), every one of them will reach for this helper and import it from the SPA file — a leaky placement. Confirmed againstapi.go(o…🪰 Gadfly · advisory
@@ -0,0 +57,4 @@}// RegistrationOpen reports whether local self-service signup is allowed.func (c *Config) RegistrationOpen() bool {🟡 RegistrationOpen() method is dead code and shadows the RegistrationOpen constant name
maintainability · flagged by 1 model
internal/config/config.go:60—(*Config).RegistrationOpen()is dead code:grep -rn "RegistrationOpen()" --include="*.go" .across the whole repo returns only its own definition, zero callers. It also shares its name with the package-level constantRegistrationOpen = "open"declared atconfig.go:18in the same file. This compiles (Go keeps method names and package-level identifiers in separate namespaces), butconfig.RegistrationOpen(string constant) andcfg.RegistrationOpen()(b…🪰 Gadfly · advisory
@@ -0,0 +65,4 @@// fails: invalid numeric/boolean values fall back to the default and are logged.func Load() *Config {cfg := &Config{Port: envInt("PANSY_PORT", 8080),🟡 Duplicated default port literal 8080
maintainability · flagged by 1 model
internal/config/config.go:68,88-89— The default port literal8080is duplicated: once as the fallback argument toenvInt, and again in the validation block's log message and reassignment. If the default ever changes, all three sites must be updated together or they drift. Extract a package-level constant (defaultPort = 8080) and reference it everywhere.🪰 Gadfly · advisory
@@ -0,0 +37,4 @@ShapeRect = "rect"ShapeCircle = "circle"ShapePolygon = "polygon" // reserved for post-v1🟡 ShapePolygon constant is exposed in the v1 domain surface but is explicitly reserved/post-v1; invites callers to emit a value the editor/API reject
maintainability · flagged by 1 model
🪰 Gadfly · advisory
@@ -0,0 +90,4 @@); err != nil {return fmt.Errorf("store: record migration %d: %w", m.version, err)}return tx.Commit()⚪ applyMigration returns tx.Commit() unwrapped, so a commit failure doesn't identify the migration version
error-handling · flagged by 1 model
The draft's single finding is confirmed against the actual code:
internal/store/migrate.go:93returnstx.Commit()unwrapped, while every other error path inapplyMigration(lines 81, 86, 91) wraps with the migration version/name. The surrounding correctness claims in the draft (deferredtx.Rollback,rows.Err()check, close-on-defer) all hold against the source.🪰 Gadfly · advisory
@@ -0,0 +22,4 @@version INTEGER NOT NULL DEFAULT 1,created_at TEXT NOT NULL DEFAULT (strftime('%Y-%m-%dT%H:%M:%SZ', 'now')),updated_at TEXT NOT NULL DEFAULT (strftime('%Y-%m-%dT%H:%M:%SZ', 'now')));🟠 users schema allows partial OIDC identity (issuer without subject or vice versa)
correctness · flagged by 2 models
internal/store/migrations/0001_init.sql:27— Theuserstable allowsoidc_issuerto be non-NULL whileoidc_subjectis NULL (or vice versa), despite the documented semantics that OIDC identity is a pair. A missingCHECK ((oidc_issuer IS NULL AND oidc_subject IS NULL) OR (oidc_issuer IS NOT NULL AND oidc_subject IS NOT NULL))permits invalid partial OIDC records that the application will have to clean up later. Fix: add the paired-nullability CHECK to enforce the invariant at the sc…🪰 Gadfly · advisory
@@ -0,0 +60,4 @@// the required pragmas, and reports whether it is an in-memory database. It// accepts a bare filename, a path with existing query parameters, or a full// file: URI, and merges the pragmas without clobbering any the user supplied.func buildDSN(path string) (dsn string, memory bool) {🔴 buildDSN truncates filenames containing ? by treating them as URI query strings
correctness, error-handling · flagged by 4 models
internal/store/sqlite.go:66—buildDSNunconditionally splits on?even for bare filesystem paths. A valid path like/tmp/weird?name.db(where?is a legal Linux filename character) is truncated to/tmp/weirdand the remainder is treated as malformed query parameters and discarded. The existing testTestBuildDSNAlwaysIncludesPragmasexercises this exact input but only asserts pragma presence, not filename preservation, so the bug passes undetected. Fix: only split on?when…🪰 Gadfly · advisory
@@ -0,0 +69,4 @@query = q}// Detect in-memory precisely: the exact ":memory:" token, or an explicit🟠 buildDSN loses filename when bare path contains literal '?'
correctness · flagged by 1 model
internal/store/sqlite.go:72-95—buildDSNmis-splits bare paths on?, discarding part of the filename. For a bare path such as/tmp/weird?name.db(where?is a literal character in the Unix filename),strings.Cut(body, "?")treatsname.dbas the query string. Becauseurl.ParseQuery("name.db")fails, the query remains empty and the database is created at/tmp/weirdinstead of the intended path. The existing test (TestBuildDSNAlwaysIncludesPragmas) only asserts pragma pre…🪰 Gadfly · advisory
@@ -0,0 +86,4 @@if err != nil {t.Fatalf("insert user: %v", err)}uid, _ := res.LastInsertId()🟡 LastInsertId error silently discarded in test, leads to confusing FK failure on driver error
error-handling · flagged by 1 model
internal/store/store_test.go:89,95—res.LastInsertId()errors are silently ignored (uid, _ := ...). If the driver ever returns an error, the test continues withuid=0, and the subsequent FK insert fails with a misleading foreign-key violation message instead of the actualLastInsertIderror. Fix: Assert the error in tests:uid, err := res.LastInsertId(); if err != nil { t.Fatalf(...) }.🪰 Gadfly · advisory
@@ -0,0 +44,4 @@}function buildUrl(path: string, params?: Params): string {const url = BASE + path🟠 buildUrl produces malformed URL when path omits leading slash
error-handling · flagged by 1 model
web/src/lib/api.ts:47—buildUrlconcatenatesBASE + pathwithout ensuringpathstarts with/. If a caller passes a naked path likeapiFetch<Health>('healthz'), the resulting URL is/api/v1healthz(missing slash), which will always 404. This is an unhandled edge case for malformed input. Fix: normalize with a leading slash, e.g.const url = BASE + (path.startsWith('/') ? path : '/' + path), or assert in dev.🪰 Gadfly · advisory
@@ -0,0 +86,4 @@// Serialize before the try so a JSON.stringify failure (e.g. a circular value)// surfaces as itself, not as a misleading "cannot reach the server" error.const requestBody = body !== undefined ? JSON.stringify(body) : undefined🟡 null body serialized as JSON 'null' because guard uses !== undefined instead of != null
error-handling · flagged by 1 model
web/src/lib/api.ts:89— Anullrequest body is serialized as the JSON string"null"because the guard isbody !== undefinedinstead ofbody != null. Callers often usenullto mean “no body,” but here it triggersContent-Type: application/jsonand sends a literalnullpayload, which can break server-side handlers expecting an object or no body at all. Fix: Change the guard tobody != nullso bothnullandundefinedare treated as absent.🪰 Gadfly · advisory
@@ -0,0 +105,4 @@throw new ApiError('Cannot reach the pansy server.', 0, null)}const payload = await parseBody(res)🟠 parseBody's res.text() runs outside apiFetch's try/catch, so a body-read failure rejects with a raw TypeError instead of an ApiError, breaking the wrapper's status/body contract that the optimistic editor depends on
error-handling · flagged by 2 models
web/src/lib/api.ts:108— body parsing can throw a raw error instead ofApiError, breaking the wrapper's central contract.parseBody(res)callsawait res.text()at line 60 (andJSON.parse(text)at line 65, though that one is guarded by atry/catchthat falls back to text). TheparseBody(res)call at line 108 is outside thetry/catchinapiFetch, which only wraps thefetchat lines 92–106. So if the body stream fails mid-read (connection drop, aborted after headers, proxy…🪰 Gadfly · advisory