- read_journal now takes an offset, so the hasMore it returns is
actionable — an agent can page a journal longer than 50 entries.
- remove_planting goes through a new service RemovePlanting that stamps
removed_at from s.now() (the injectable clock ClearObject and the fill
path use), instead of the adapter computing the date off the wall clock.
It delegates to UpdatePlanting, so the role check, version guard and
history record are unchanged. Drops the now-unused time import.
Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ
The toolbox could create and move but not delete or resize; write the
journal but not read it; clear a whole bed but not pull one plant; report
seed remaining but not record a purchase. Close those gaps with thin
adapters over the SAME service methods the REST API uses, so they inherit
the permission checks unchanged:
read_journal → ListJournal (the write/read asymmetry, most visible)
update_object → UpdateObject (resize / rotate / rename / plantable)
delete_object → DeleteObject (counterpart to create_object)
remove_planting → UpdatePlanting (soft-remove ONE plop, like clear does)
list_seed_lots → ListSeedLots
record_seed_lot → CreateSeedLot (record a purchase; "I bought 2 packets")
To address a single plop the agent needs its id + version, so
DescribePlanting now carries both — the same way DescribeObject.Version
already lets it edit an object. remove_planting soft-removes (removed_at =
today), mirroring clear_object, so the plant stays in planting history and
the change is undoable.
Deferred deliberately: an undo/revert tool needs a way to list recent
change sets to get a changeSetId, which is a larger addition; noted on the
issue for a follow-up.
Tested through the tool layer (TestCorrectiveTools): resize, single-plop
removal, journal read-back, seed-lot record+list, and delete.
Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ
Gadfly findings on #95:
- Correctness (3 models): the shared inset formula radius-spacing/2
collapses to 0 in grid mode (radius = spacing/2), so grid's outer row
planted flush on / overhanging the bed edge instead of the half-spacing
in that the rule wants. The inset genuinely differs by layout — a grid
plant sits AT the plop centre (inset spacing/2), a clump's plants reach
its rim (inset radius-spacing/2, overhanging by a half). Split it into a
new edgeInset(radius, spacing, layout); hexCenters now takes a
precomputed inset and is pure geometry (no spacing/layout knowledge).
Regression guard: grid plants land at ±25 on a 60cm bed, not ±30.
- Performance (2 findings): the in-loop `existing = append(existing, *p)`
was dead — every plop in one fill shares a radius and sits on a distinct
lattice point, and a plop is "covered" only when wholly inside another,
impossible between equal-radius circles at different centres. Removing it
stops the coveredByExisting scan growing during the fill (an empty-bed
grid fill's check was needlessly quadratic in the plop count).
- Docs: FillRegion/fillLoaded/hexCenters comments and the DESIGN.md bullets
updated for plopRadiusFor/edgeInset (were still citing defaultPlopRadius
and "written out in hexCenters").
- Test hygiene: split the grid + bad-layout cases out of TestFillAndClearAPI
into TestFillLayoutAPI (one concern per test).
The enum-tag finding is a non-issue: majordomo's DefineTool derives its arg
schema from the same struct-tag reflection as Generate (proven by the vision
SeedPacket enum), and the service validates mode regardless.
Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ
Gadfly findings on #94, the real ones:
- scanSeedPacket extends only the READ deadline; a slow upload + a live
vision call runs past the server's absolute 30s WriteTimeout and the
successful response is silently dropped (the #78 failure mode). Extend
the write deadline too (scanWriteTimeout).
- An oversized upload tripping MaxBytesReader was mapped to 400; it's 413.
Detect *http.MaxBytesError and report IMAGE_TOO_LARGE.
- Split imagenorm error mapping: ErrTooLarge->413, ErrUnsupported->400,
genuine read/encode faults (and a failed file.Open)->500, not 400.
- CreateFromPacket discarded the plant it created when the lot then
failed, contradicting its own doc. Roll the new plant back instead so
the confirm is all-or-nothing (a fresh plant has no lots/plantings, so
the delete is safe; log-and-continue on cleanup failure).
- Dedup: packetLotRequest and seedLotCreateRequest shared every lot
field. Extract a seedLotFields base both use. validCategory now reuses
plantCategories. EffectiveConfig resolves agent+vision from one
settings-row read instead of two.
- capabilities swallowed an EffectiveVision error silently; log it.
- vision test hand-copied Extract's body (drift risk). Split generate()
out of Extract so the hermetic test drives the real request builder.
Tests: rollback-on-lot-failure (service), oversized->413 (api).
Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ
Steve chose option 3: keep clumps as the default primitive, add a grid/rows
fill mode, so sketching and planning are different operations with different
outputs rather than one model forced to be both.
A plop is a CLUMP, not a plant — great for "a few plops of garlic in a corner",
useless for drawing a plantable 8-rows-of-garlic bed (that came out as ~15
blobs, #77). FillLayout selects what a fill packs:
- clump (default, unchanged): radius 1.5×spacing, ~7 plants per plop.
- grid: radius spacing/2, pitch = spacing, ONE plant per plop — rows you could
actually plant from.
The geometry is the SAME hexCenters lattice and the SAME #75 half-spacing edge
rule; only the radius→spacing relationship differs (plopRadiusFor). Grid keeps
no 15cm floor — its whole point is true spacing — while clump keeps it so a
tiny-spacing plant doesn't make invisible clumps.
Threaded through FillRegion/FillNamedRegion (empty layout = clump, so existing
callers are unchanged; unknown layout = ErrInvalidInput), the REST /fill
endpoint (`layout`), and the agent's fill_region tool (`mode`, enum clump|grid),
so "plant the bed in rows" works.
Tests: grid produces many more, single-plant plops than clump on the same bed
(radius spacing/2, derived count 1); unknown layout is refused at both the
service and the API. maxFillPlops still caps a grid fill of a huge bed.
No frontend fill affordance exists yet (fill is agent-only in the UI; the fill
UI was deferred in #82), so the mode toggle rides along when that's built —
noted. Docs: DESIGN placement-model decision.
Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ
Photograph a seed packet → it fills in the plant and the purchase. This is the
backend; the scan UI is a follow-up PR.
Vision model config (mirrors the agent model from #79):
- Migration 0011 adds instance_settings.vision_model; PANSY_VISION_MODEL is the
env default. Precedence Settings → env → empty; the KEY stays in the env.
- EffectiveVision resolves it; /capabilities advertises "vision" only when a
model + key are configured, so the UI offers the scan button only when it works.
Extraction is one-shot, NOT an agent loop (internal/vision):
- majordomo.Generate[SeedPacket] derives a JSON schema from the struct tags and
hands the image to the vision model; it can't call a tool, so it can't touch
the garden — it only reads a picture and returns data. Numeric fields are
pointers, so a field the packet doesn't print comes back nil, not a made-up 0.
- Hermetic test: majordomo's fake provider returns canned packet JSON and
Generate unmarshals it, image + derived schema included. No live model.
The image is normalized to JPEG at the upload boundary (imagenorm from #80),
which is where an iPhone HEIC becomes readable — majordomo's media path can't
decode HEIC. imagenorm now links into the binary (~7 MB, the cost #80 deferred).
The hard part is catalog matching, not OCR (internal/service/seed_packet.go):
- A wrong auto-match splits a variety's seed-lot history across duplicate rows,
so the service NEVER auto-creates. matchPlants surfaces RANKED candidates
(exact name → variety-in-name → same species, conservative and name-based),
the user confirms, and CreateFromPacket makes the plant (new or existing) + the
lot. Exactly one of plantId/newPlant, refused otherwise.
- Plants/lots aren't in the undo history (catalog/inventory), so no change set.
- The extractor is injectable (service.WithPacketExtractor) so ExtractSeedPacket
and the /scan endpoint test end to end against a fake, no live model.
Endpoints: POST /seed-lots/scan (multipart image → proposal, reads only; extends
the read deadline for a slow phone upload, caps the body, maps too-large/unreadable
to clear statuses) and POST /seed-lots/from-packet (confirmed proposal → 201).
Docs: README (PANSY_VISION_MODEL), DESIGN (routes + the decision and why the
model can't touch the garden).
Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ
Moves the agent model out of env-only config into an admin-editable Settings
section, and enforces is_admin for the first time — it has been in the schema
since migration 0001, plumbed to the client, and checked nowhere.
Backend:
- Migration 0010: instance_settings, a single-row (CHECK id=1) table — pansy's
first instance-level state. Holds agent_model ('' = inherit env) and
agent_enabled (NULL = inherit env), version-guarded like every mutable row.
SECRETS STAY IN ENV: OLLAMA_CLOUD_API_KEY is never stored here.
- requireAdmin at the service seam (authoritative) plus a cheap middleware
early-403. Non-admin gets 403, not 404 — settings existence isn't masked.
- EffectiveAgent resolves DB-over-env (model, enabled); key always from env.
- The live Runner is hot-swapped, not built once. agentHolder holds it behind
an atomic.Pointer; the chat routes are now registered UNCONDITIONALLY and
nil-check agent.get(), so a settings change turns the assistant on/off/onto a
new model with no restart and no race against in-flight readers. /capabilities
reads the pointer, so it reports what's live, not what booted.
- internal/agentmodel is a new leaf package holding the one place that knows how
to turn a spec into a model. Both agent (to run) and service (to validate a
spec before storing it) import it; it can't live in agent, which imports
service. Settings PATCH validates the spec via Parse, so a typo is a 400 now
rather than a broken assistant on the next turn.
Frontend:
- /settings route (admin guard), a Settings page (model field, tri-state
enabled, live status), nav link shown only to admins.
- useCapabilities drops staleTime:Infinity — the assistant can now change under
a running page — and the settings save invalidates it.
Contract change: chat routes always exist, so "assistant off" is a runtime 503
+ capabilities:false, not a missing route. Updated the test that asserted the
old shape.
Verified live against the built binary: disable flips capabilities to false and
logs it; re-enable with a new model swaps it back; a bad spec is rejected 400;
the setting persists across a restart. Swap is race-clean under `go test -race`.
Docs: README (precedence + key-stays-in-env), DESIGN (decision + routes),
CLAUDE (don't re-add conditional route registration; key never in the DB).
Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ
- fillLoaded's doc listed what it does and omitted the non-finite-region
rejection this PR added to it.
- Trim the half-spacing rule's restatement in DESIGN.md to the decision and a
pointer. The rule, the square-foot arithmetic and the failure mode are
written out once, in hexCenters, rather than near-verbatim in four places.
Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ
- Split hexCenters' doc: the count/limit contract had run straight on from
the #75 anti-regression paragraph with no separator, so its opening "It"
read as referring to the wrong thing.
- Write the stagger as pitch/2 rather than radius. Same value, but the intent
is "half a pitch" and only incidentally "one radius".
- fitAxis's step<=0 guard is unreachable from its only caller. Kept, and now
says so: a helper this small shouldn't need its caller read to be shown
safe, and the failure mode without it is ±Inf into an int conversion.
Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ
- hexCenters now derives its exact point count BEFORE building anything and
returns it alongside the points, refusing over the cap without allocating.
Previously it materialised the whole lattice and fillLoaded checked len()
afterwards — so the "too large" path paid for the thing it was rejecting.
This also makes the preallocation exact, which subsumes the earlier
over-allocation finding I'd declined: staggered rows hold cols-1, so
rows*cols over-reserved by ~12%.
- Region.empty() names the invariant that clampTo expresses "no overlap" by
INVERTING the region rather than zeroing it. A bare `MaxX < MinX` at each
call site was spreading a non-obvious convention across three functions.
The count is now load-bearing (it gates the cap), so the test asserts it
matches what actually gets built.
Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ
- FillRegion's doc still said "half-pitch inset", left over from the first
draft of the fix; the inset is radius - spacing/2. Two docs on the same
function disagreeing is worse than either being terse.
- Reject non-finite region bounds. They survive clamping and the inverted-
region guard (NaN compares false both ways). Nothing corrupt reached the
table — SQLite stores NaN as NULL and NOT NULL refuses it — but NaN
surfaced as a raw store error and +Inf as a silent zero-plop success.
- TestHexCentersTinyRegion used a region symmetric about the origin, so it
could not distinguish "the middle of the region" from "the origin" and
would have passed for an implementation that just returned (0,0). Added an
off-centre case.
Not taken: the finding that `make(..., rows*cols)` over-allocates ~12%
because staggered rows hold cols-1. True, but the slice is capped at
maxFillPlops (5000) and the exact count needs a ceil/floor split for no
measurable gain.
Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ
- clampTo's doc justified itself by stopping hexCenters "looping forever",
which stopped being true when hexCenters became count-bounded. Say what it
actually does now, and note the inversion the new guard relies on.
- Trim the changelog prose from hexCenters' doc down to the one line that
earns its keep: don't re-anchor at the min corner, and why.
- Rename a test local from `max` so it stops shadowing the builtin.
Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ
clampTo INVERTS a region lying wholly outside the object — Max clamps below
Min — rather than emptying it. The old loop-until-past-MaxX form handled that
for free by never entering the loop. Counting positions up front does not:
a region 500cm east of a bed with ±50cm local bounds produced 4 plops at
x=275, a couple of metres off the bed.
Caught by removing the guard and watching the new test fail, not by assuming
it would.
Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ
Filling a bed left the outer row too far from the edge, and staggered rows
worse still. Two defects, both from anchoring the lattice at the region's min
corner:
- Odd rows offset by `radius` started at `MinX + 2·radius`, leaving a bare
strip a whole plop wide down one side of every other row.
- All the leftover slack piled up on the far edge, where plops hung 13cm
outside the bed on a 4×8ft garlic bed. Nothing clips them, so they drew
over the bed outline.
Spacing is a constraint between neighbouring plants competing for the same
soil, light and water. A bed edge is not a competitor, so the outer row owes
it half the spacing — the arithmetic inside every square-foot-gardening chart
(4/square = 6" apart, 3" from the square's edge).
The wrinkle: a plop is a CLUMP, not a plant. defaultPlopRadius is 1.5×spacing,
so keeping the whole circle inside the bed insets the outer row by 1.5
spacings, three times what the rule allows. So centre the lattice and set the
minimum centre-inset to `radius - spacing/2`: the clump may cross the edge by
up to half a spacing, putting its outermost plants exactly the half-spacing
from the edge the rule asks for. Capped there — a clump mostly outside the bed
would be a drawing of plants in the path.
Same bed, same 15 plops, now symmetric with a deliberate 6.5cm overhang inside
the 7.5cm budget instead of an accidental 13cm on one side only. The stagger
falls out of the centring for free: an offset row holds one fewer plop, and
centring that run puts it exactly half a pitch off its neighbours.
TestFillRegionDeterministicPacking expected 4 plops in a 60×60 bed; the fourth
was centred ON the east edge with half of it outside, well past the budget.
It is 3 now — the fix working, not a regression in it.
Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ
Closes#44. Two independent grids (garden + per-bed), each with a size and a snap toggle; snapping defaults off. See PR #45 for details.
Co-authored-by: Steve Dudenhoeffer <[email protected]>
Per-garden public read-only link: unauthenticated GET /api/v1/public/gardens/:token (no requireAuth, no OIDC), owner-only enable/rotate/disable, and a /g/$token page rendering GardenCanvas read-only. Review fixes: redact plant owner ids, Cache-Control: no-store, 400 on malformed body, shared resetTransient store action.
Closes#41.
Co-authored-by: Steve Dudenhoeffer <[email protected]>
Fixes from the PR #28 adversarial review (considered; not graded).
Correctness / API
- PATCH /objects/:id can now clear nullable color/props back to NULL: the
request takes them as json.RawMessage, and ObjectPatch carries an explicit
Set flag so an explicit `null` (clear) is distinguished from an absent
field (unchanged) — the strongest cross-model finding (6 hits). New test.
Maintainability
- store/plantings.go + plants.go use explicit qualified column lists
(qualifyColumns helper) instead of SELECT *, matching gardens/objects and
surviving a future column add.
- Consolidated objectKinds + plantableByDefault into one kind→traits map.
- objectForRole factors the fetch-then-authorize shared by UpdateObject and
DeleteObject; dropped the redundant kind check in CreateObject
(finalizeObject is the single validation point).
- Request→service mapping via toInput()/toPatch() methods (matches gardens).
- Renamed handler gardenFull → getGardenFull (verbNoun); test helper
decodeGarden → decodeMap; generic bind-error messages.
Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01JdQpdYYsTgtkJBxbcpAszi
Garden objects (beds, bags, containers, in-ground, trees, paths,
structures) in one polymorphic table, following the #7 service
conventions.
- store/objects.go: scanObject + Create (RETURNING), Get, ListForGarden
(z_index order), version-guarded Update (RETURNING, returns current row
on conflict), Delete (plantings cascade via FK).
- store/plantings.go + plants.go: the read side /full needs now —
ListActivePlantingsForGarden (removed_at IS NULL) and
ListReferencedPlants (distinct plants used by active plantings). Both
return empty until #14 adds plantings; #12/#14 extend these files.
- service/objects.go: Create/Update(partial patch)/Delete/GardenFull, all
(ctx, actorID, ...) through requireGardenRole(editor for mutations,
viewer for /full). finalizeObject validates kind + shape (rect/circle;
polygon reserved), finite dims in [1cm,100m], NaN/Inf rejection, loose
bbox-overlaps-garden placement (partial overhang OK), hex color, valid
JSON props, name/notes caps; normalizes rotation to [0,360). Plantable
defaults by kind, overridable.
- api/objects.go: POST /gardens/:id/objects, PATCH/DELETE /objects/:id,
GET /gardens/:id/full ({garden,objects,plantings,plants}). props is any
JSON value stored as text; PATCH is partial + required version, 409
returns the current row.
Tests: service (defaults, plantable-by-kind, rotation normalize,
validation rejects incl. off-field/polygon/bad-color/bad-props, partial
patch + version conflict, cross-user ErrNotFound, delete, /full shape) and
api (CRUD + /full, 409 envelope, cross-user 404, polygon/no-kind 400,
props round-trip).
Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01JdQpdYYsTgtkJBxbcpAszi
Fixes from the PR #26 adversarial review (graded 18 real / 0 false positive).
Correctness / security
- maxGardenCM fixed to 10_000 (100 m), matching its comment — it was
100_000 cm (1 km), 10x too lax (5 models flagged this).
- Dimension validation now rejects NaN/Inf (which slip past naive
comparisons) and subnormal-tiny positives, via a finite [1cm, 100m]
check. Name (200) and notes (10_000) are length-capped so untrusted
input can't balloon storage.
- Update version binding is `required,min=1`, so a negative/zero version
is a 400, not a 409.
Maintainability / performance
- One unified writeServiceError (new errors.go) maps every auth + resource
sentinel; writeResourceError removed. writeVersionConflict and
parseIDParam moved to errors.go (shared, not in the gardens feature file).
- Request structs share an embedded gardenFields (one toInput).
- CreateGarden uses INSERT ... RETURNING (one round-trip).
- ListGardensForOwner has a defensive LIMIT (pagination is post-v1).
Tests: name/notes length, NaN/Inf/subnormal dims, dimension-at-cap valid,
negative/zero version -> 400.
Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01JdQpdYYsTgtkJBxbcpAszi
Establishes the patterns every later backend issue copies: the actor
parameter, centralized role checks, and the version-guard/409 sync
protocol. The service layer is the seam both REST handlers and future
agent tools call, so permissions live here, not in handlers.
- service/gardens.go: Service methods take (ctx, actorID, args).
requireGardenRole(ctx, actor, gardenID, min) is THE authorization point
— owner is implicit via owner_id now; #16 extends it to consult
garden_shares. A user with no role gets ErrNotFound (existence masked),
not ErrForbidden. Create/Get/List/Update/Delete with input validation
(name required, 0 dims default to 10 m on create / rejected on update,
negatives always rejected, unit metric|imperial, 100 m cap).
- store/gardens.go: version-guarded UPDATE ... WHERE id=? AND version=?
RETURNING; a no-match re-reads to return (current row,
ErrVersionConflict) vs ErrNotFound. ListGardensForOwner returns a
non-nil slice.
- api/gardens.go: GET,POST /gardens and GET,PATCH,DELETE /gardens/:id
behind requireAuth. writeVersionConflict documents the 409 envelope
({error:{code,message}, current:{...}}) — the contract for every
mutable resource. writeResourceError maps ErrNotFound/Forbidden/
InvalidInput/VersionConflict; parseIDParam guards path ids.
Tests: service (defaults, validation, owned-only list, version
conflict returns current + retry, cross-user ErrNotFound, delete) and
api (full CRUD flow, 409 envelope shape, cross-user 404, auth required,
create validation). Verified against the running binary: create stores
imperial 122x244 cm and list returns it.
Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01JdQpdYYsTgtkJBxbcpAszi
Fixes from the PR #24 adversarial review (graded 23 real / 1 false positive):
Security / correctness
- LinkOIDC no longer overwrites a different stored identity: the UPDATE
matches only when the row has no identity yet or already carries this
exact one, so a second IdP asserting the same verified email can't
hijack or lock out an account (returns ErrOIDCIdentityConflict). Uses
a single UPDATE...RETURNING (also fixes the ignored-RowsAffected /
misleading-ErrNotFound path and the round-trip).
- Provisioning now requires a verified email for BOTH linking and JIT
creation (was: linking only), so an unverified-email identity can't
create an account — nor become the first admin on a fresh instance,
nor squat an email a real user later owns.
- OIDC-identity collisions surface as the dedicated ErrOIDCIdentityConflict
instead of the email-specific ErrEmailTaken.
Robustness
- readOIDCTxCookie requires a non-empty nonce (an empty one would make the
callback's nonce check pass vacuously).
- Callback token exchange + verify run under a 15s context timeout so a
slow IdP can't outlast the server write timeout.
- ensure() performs discovery outside the mutex, so concurrent cold-start
requests don't serialize behind one another's full timeout.
- setOIDCTxCookie returns its marshal error; oidcLogin aborts rather than
redirecting to the IdP with no tx cookie.
Maintainability
- redirectAuthError helper dedups the ~dozen callback redirects (and the
empty-code path now logs like the rest).
- Distinct login error codes (no_email / email_unverified / oidc_conflict)
for the UI; writeServiceError maps the OIDC sentinels; shared test issuer
const.
Tests: unverified email refused for both link and JIT; identity-overwrite
refused while the original identity keeps working.
Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01JdQpdYYsTgtkJBxbcpAszi
OIDC is pansy's primary login path (Authentik the target IdP); local
auth (#4) remains the fallback and both issue the same session cookie.
- deps: github.com/coreos/go-oidc/v3 + golang.org/x/oauth2 (both pure Go;
CGO stays off).
- api/oidc.go: lazy issuer discovery (retried per-request, never crashes
a server that also serves local auth), GET /auth/oidc/login builds an
authorization-code URL with PKCE S256 + random state + nonce stashed in
a short-lived HttpOnly cookie, GET /auth/oidc/callback verifies state
(constant-time), exchanges the code with the PKCE verifier, verifies the
ID token + nonce, and starts a pansy session. Failures redirect to
/login?error=... ; success to /gardens.
- service.LoginOIDC: (issuer,subject) match -> login; else *verified*
email match -> link onto the existing account; else JIT-create (the IdP
gates access, so PANSY_REGISTRATION doesn't apply). Unverified email
colliding with an existing account is refused (takeover guard); no email
is refused (email is the account key). Reuses the atomic CreateUser.
- store: GetUserByOIDC + LinkOIDC (unique-pair backstop).
- config: OIDCReady() (needs issuer+client+BaseURL for the redirect URI);
/auth/providers now reports oidc from it and defaults the button label
to "Sign in with Authentik". OIDC routes are only registered when ready,
so an unconfigured instance 404s them.
- PANSY_LOCAL_AUTH=false rejects/hides local auth but not OIDC.
Tests: service provisioning (JIT, repeat login, link, unverified-collision
refusal, no-email, name fallback, works with local auth off); api
(routes-absent-when-unconfigured, providers reporting, login redirect with
PKCE params + tx cookie via a fake discovery server, callback state/error
paths). Smoke-tested: unreachable issuer degrades to error=oidc_unavailable
with the server still up and local auth working.
Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01JdQpdYYsTgtkJBxbcpAszi
Fixes from the PR #23 adversarial review (graded 35 real / 1 false positive):
Security / correctness
- Race-free registration: is_admin and the registration gate are now
computed atomically inside a single INSERT...SELECT, so concurrent
first registrations can't both become admin or bypass closed
registration (fixed the whole TOCTOU cluster).
- Sliding session now reaches the browser: ResolveSession returns the
current expiry and requireAuth re-sets the cookie, so active users
aren't logged out 30 days after login regardless of activity.
- Login CSRF: csrfGuard rejects state-changing requests whose Origin
doesn't match PANSY_BASE_URL (no-op when unset, so the dev proxy is
unaffected). SameSite=Lax alone didn't cover this.
- argon2id tuned to RFC 9106's second recommended profile (t=3).
- Timing equalizer can't fail open: the dummy hash is derived
deterministically (fixed salt, no RNG) so it's always present.
- Password length (<=1024) enforced in the service for both register
and login, not just HTTP binding tags; login rejects over-long input
before spending argon2 work.
Error handling / robustness
- Login logs a malformed stored hash instead of silently treating it as
a wrong password.
- Best-effort session writes (Touch/Delete during renewal, expiry, and
corrupt-expiry cleanup) now log on failure.
- index sessions.expires_at via new migration 0002 (0001 is immutable).
Maintainability
- Extract startSessionAndRespond and abortUnauthenticated; make
writeServiceError a free function; consistent error handling in
decodeHash; doc/comment fixes.
Tests: over-long password, CSRF guard (cross-origin/same-origin/dev
no-op), and cookie refresh on authenticated requests; migration-version
assertions bumped to 2.
Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01JdQpdYYsTgtkJBxbcpAszi
Implements pansy's local (email + password) authentication and the
session layer that OIDC (#5) will also reuse.
- store: users.go (create/get-by-id/get-by-email/count) and sessions.go
(create/get/touch/delete/delete-expired), scanning the existing 0001
schema.
- service: the business-logic seam. auth.go (Register/Login/session
lifecycle/Providers) + password.go (argon2id, 64 MiB/1/4, PHC-encoded,
constant-time verify) + service.go (Service, clock injection, token
hashing). First user is admin; closed registration still allows the
bootstrap user; unknown-email and wrong-password are indistinguishable
(same error, same argon2 work via a dummy hash).
- api: POST /auth/register|login|logout, GET /auth/me|providers, plus a
requireAuth middleware that resolves the HttpOnly session cookie
(SameSite=Lax, Secure under https) to the actor. Handlers stay thin.
- main: wires the service and a periodic expired-session sweep; sessions
are also dropped lazily on access. Sliding 30-day expiry.
- tests: service (register/login/expiry/renewal/cleanup, password) and
api (cookie flow, middleware, validation, providers).
Verified end-to-end via curl: register -> me -> restart -> session
persists -> logout -> 401.
Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01JdQpdYYsTgtkJBxbcpAszi
steveandClaude Opus 4.8