80 Commits

Include renames

Author	SHA1	Message	Date
synAdminandClaude Opus 5	25078417e5	feat(auth): create an account from a Google identity, then ask about consent (#342) ... A Google account nobody here has seen now becomes a customer. The OAuth part of this was the easy half; the problem worth the issue is consent. Registration asks for two consents and stores their wording verbatim, and marketing consent must start unticked. Somebody arriving through Google has never seen those checkboxes and could not have, because the redirect happened before anyone knew whether they were new. Creating the account with both false is legally correct: nobody agreed to anything, and nothing is recorded as though they had. There is no stored wording either, because a wording saved against a false consent is a record of a conversation that never happened. But stopping there would mean a Google sign-up is never asked at all, and a silent no is still a decision made on somebody else's behalf. So the account is created, the customer is signed in, and they land on a step that shows the same two sentences with the same two unticked boxes. It saves through the endpoints registration already uses, which is what keeps the stored text byte-identical rather than merely similar. Not now is offered as an equal option, because consent has to be as easy to withhold as to give, and both can be changed later from the account page. The wording on that screen is imported from the shared constants rather than retyped. Three different wordings were already in circulation once before that was shared, and the record is meant to say what the customer actually saw. The return path is deliberately dropped for a new customer, who lands on the consent step instead. Carrying it through as a query parameter was the alternative and was rejected: the consent page would then redirect somewhere a URL told it to, which is the open-redirect question already answered on the server, asked a second time in a second language on a page an attacker can link to directly. One new customer occasionally landing on the storefront rather than back at their cart is much the cheaper of the two. The customer and the identity are inserted in one transaction. A customer row with no identity is an account nobody can sign in to and nobody can recover, because it has no password either. Signing up is refused when the address already belongs to a customer. Joining those two accounts is linking, it is the most security-sensitive decision in this project, and it belongs to the next issue rather than falling out of an INSERT here. Refusing is the safe half of that decision and the only half available until the policy is written down. The unique index rather than the preceding SELECT is what actually holds when two sign-ins race, so losing that race is treated as the address being taken rather than as an error. Google's assertion about the address is taken only when it is the boolean true. When it holds, the account is marked verified and no confirmation email is sent, because that email exists to prove the customer receives mail at the address and Google has just proved exactly that. When it does not, the account is unverified and goes through the ordinary confirmation, because an unverified assertion is worth nothing. Names from the profile are hints. Registration demands both because every email greets by first name, but Google may return neither and refusing a sign-in over it would be absurd — the greeting already has a fallback for exactly this case. The tests worth reading are the two about a returning customer. One signs in again and reaches the same account; the other changes their Google address first and still reaches it. That second one is the whole reason the identity is keyed on the subject claim: an email match would have created a second account there, and an address that had since been reassigned would have handed the first one to a stranger. Verified: backend tsc clean for src and tests, 590 unit tests pass, lint at the seven warnings that predate this branch, frontend tsc, lint and build clean. The integration suite needs a database this machine has no Docker for. Closes #342 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>	2026-09-10 10:11:16 -05:00
synAdminandClaude Opus 5	87f07baaff	feat(auth): the Google sign-in round trip (#341) ... Two routes, and a customer whose Google identity is already linked can sign in. Creating accounts and linking them are deliberately held back to the next two issues, so this change is about the protocol alone and can be reviewed as such. The authorization code flow, with PKCE. The browser is sent to Google, comes back carrying a code, and this server exchanges it over its own TLS connection, so nothing that can read the page ever holds a token. PKCE goes in even though this is a confidential client with a secret: it costs one hash and closes code interception outright rather than resting the whole flow on the secret staying secret. No JWKS fetch, and the reasoning is written into the module rather than left to be rediscovered. The id token arrives in the response to a request this server made, over TLS, directly to Google's token endpoint, which is exactly the case OpenID Connect permits skipping signature verification for. That removes a key fetch, a cache and a rotation path from the part of the codebase least worth having moving parts in. It removes none of the claim checks, and the comment says plainly that the moment an id token reaches this code from anywhere else, the reasoning stops holding. So the claim checks are load-bearing rather than belt and braces, and each has a test naming what accepting it blindly would allow. A wrong audience is a token minted for another application being replayed here. A wrong nonce is a token from an earlier attempt. A missing subject is an identity row keyed on nothing. Both spellings of the issuer are accepted because Google really does send both, and taking only one produces sign-ins that fail for some customers and not others. email_verified is compared to the boolean and never merely tested for truthiness. The string "false" is truthy, and the linking policy turns entirely on this flag, so that one line is the difference between a policy and an account-takeover path. The attempt cookie is the whole security of the callback, which is a plain GET anyone on the internet can invoke. It carries three secrets, minted separately because they are checked by different parties at different moments: state proves the callback belongs to the request this browser started, nonce proves the token was minted for this attempt, and the verifier proves the code is being spent by whoever asked for it. It is cleared on every path through the callback, so one attempt cannot be replayed even once. SameSite is Lax and not Strict, and that line has the longest comment in the file because it is the most expensive thing here to get wrong. The callback arrives as a cross-site top-level navigation; Strict withholds the cookie, the state check then fails, and every sign-in is refused with an error that looks exactly like tampering. Where the customer returns to survives the round trip in that cookie, and it is a value an attacker can propose. Unchecked, the start route is an open redirect wearing a sign-in flow as a disguise: a link on our own domain, with our own certificate, that lands somewhere else. Its own module, so it can be tested without a database and so the next path needing the same question has an obvious place to ask it. Its first draft used a regex that inverted its own character class and rejected every path, which passed every other test and would have broken every real sign-in — there is now a test for exactly that. Declining at Google's consent screen is a cancellation rather than a failure. The customer goes back where they were with nothing said, the same distinction #41 drew for a dismissed passkey prompt. A disabled account is refused here as well, because enforcing it on some sign-in routes and not others is how a disabled account keeps a way in. Signing in calls the shared function, not a third implementation that agrees today. There is a test that the resulting session is accepted by an unrelated route, which is what makes that sharing worth something rather than merely tidy. Verified: backend tsc clean for src and tests, 590 unit tests pass, lint back to the seven warnings that predate this branch. The integration suite covers the routes end to end with only the token exchange stubbed, and needs a database this machine has no Docker for. Closes #341 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>	2026-09-10 08:49:42 -05:00
synAdminandClaude Opus 5	9cc82002b8	feat(auth): groundwork for signing in with Google (#340) ... Nothing a customer can see. The schema change and the configuration land on their own so the widest-reaching edit in the project can be reviewed for what it is rather than buried inside a feature. The password hash becomes nullable. That is one line and it is not the work; the work is that every read of the column is now a question rather than a fact. Three places compared against it with bcrypt, and all three now ask first through one shared function. That function exists because the alternative is worse than a wrong answer. bcrypt.compare throws on a null hash rather than returning false, so any call site that forgot the check would answer a sign-in attempt with a 500 instead of a refusal. On the login route that is also an oracle, because it would happen for exactly the accounts that have no password. One function rather than a null check repeated three times means the question is asked identically everywhere and a fourth site cannot forget to ask it. Nothing writes a null yet. The first accounts without a password arrive with the sign-up path, which is why this is landed ahead of them. The identities table is a table rather than columns on customers, because one customer may eventually hold more than one. Columns would make a second provider a migration and a third an embarrassment. Its important column is the provider subject, and the comment on it is the whole security posture of the feature in one place: never the email. An email is a display value its owner can change and a provider may reassign; a subject is opaque and stable for the life of the account. Matching on the email would strand a customer who changed theirs and, far worse, hand their account to whoever inherited the old address. Unique across the provider and subject together, not the subject alone. The down migration drops the table and deliberately does not restore the NOT NULL. Re-adding it fails outright once a passwordless customer exists, and a down migration that destroys accounts to satisfy a constraint is far worse than a column that is merely more permissive than it needs to be. The redirect URI is derived from PUBLIC_URL, the same single source the WebAuthn Relying Party ID uses and for the same reason: Google compares it as an exact string and answers a mismatch with a message that says nothing about which half is wrong. Deriving it means the value is correct by construction anywhere the email links already are. The tests are mostly about what must not end up in it, since a trailing slash on PUBLIC_URL is an easy way to produce a URI that is one character from the registered one. The config also reports whether it is enabled at all, so a developer without credentials gets a storefront that works and simply does not offer the button, rather than one that offers it and fails. Absent rather than disabled, the same choice made for a browser without WebAuthn. Environment validation refuses to boot on one credential without the other, matching how the SMTP pair is handled. Half-configured is the case worth catching because the failure otherwise arrives at the moment a customer presses the button. The QA compose file sets both to empty, and the comment there says why at length rather than leaving it to look like an oversight. Google refuses a redirect URI whose host is not under a domain whose ownership has been proved by DNS, and nobody can prove ownership of anything under bermudalamb.synology.me because Synology owns the registrable domain above it. That is the same wall #285 hit with Cloudflare. So QA cannot run this at all until #313 moves it to a subdomain of the real domain, at which point it is two stack variables and one console entry, with no code change either way. Also corrects the record in #332, which lists account deletion as confirming with a password. It does not; the route takes none and the confirmation is a modal in the account page. Deletion needed no change here. Verified: backend tsc clean for src and tests, 550 unit tests pass including new coverage of the config derivation, the null-hash comparison and the environment rules; lint clean apart from warnings that predate this branch. The integration suite needs a database this machine has no Docker for. Closes #340 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>	2026-09-10 08:18:51 -05:00
synAdminandClaude Opus 5	90e372d6bd	feat(admin): move a customer's account to an address they can reach (#337) ... The third step of the only recovery route a customer who has lost their mailbox has. The first two are contacting the shop and being verified against order history. The third had no implementation, so the answer was a hand-written database edit that left no record of who did it or why. The thing to say plainly, because everything here follows from it: this operation and an account takeover are the same operation. They differ only in whether the verification was sound, and nothing in the software can check that. What the software can do is make the change recorded, announced, and complete in its effects. Recorded. The endpoint refuses without a written reason, and the reason is stored against the account. That row is the only thing that tells a genuine recovery from a takeover afterwards, which is why a hand edit was never acceptable and why the field is required by the server rather than merely collected by the form. It is never shown to the customer: it is a note about how somebody was verified and can name things the customer should not be handed back. There is no column for who did it. Admin access is one shared gate secret in front of a single operator, so such a column could only ever hold a constant, and a constant dressed up as an identity is worse than an honest absence. Announced, to the address being replaced. If the recovery was sound that reaches nobody and costs nothing. If it was not, it reaches the real owner, who is the only person in the world who can say so, and that is the only reason this endpoint is safe to have at all. Its own template rather than the self-service one, because that copy says to contact us if you did not make this change, and here somebody already did — the sentence would be addressed to the customer who just did the thing it asks for, while the person who needs to act on it did nothing. The new address is marked unverified and sent a confirmation link. Somebody reading an address out over the phone has not demonstrated they can receive mail at it, and that is the commonest way this goes wrong harmlessly. Complete in its effects. The move signs the customer out everywhere, removes every passkey, and cancels reset links already sent. That is the conclusion #42 reached for password reset, and it applies here with more force: somebody the system cannot identify asked for this change, so a session or a credential surviving it is one the new owner cannot see and cannot revoke, and a reset link sitting in the mailbox being taken away would let whoever still reads it take the account straight back. The password is left alone. What the customer lost was the mailbox, so demanding a new one adds a step for no gain. The verification-email helper moved out of the customers route into its own module, for the reason session creation moved out for passkeys: two implementations that agree today are two that can be changed one at a time, and the one that gets forgotten is whichever the manual testing does not exercise. This path runs perhaps once a year, so it is exactly the one that would rot. The admin drawer gains the action next to the address rather than among the account controls, because it is a thing done to that field by someone already looking at it. It leads with the warning instead of burying it. The history of moves sits on the same drawer and renders nothing at all for the overwhelming majority of customers, who have never been moved. Verified: backend tsc clean for src and tests, 526 unit tests pass, lint clean apart from warnings that predate this branch; frontend tsc, lint and build clean. The integration suite needs a database this machine has no Docker for. It also cannot be proven by CI right now — run 917 has been hung since it started and 24 runs are queued behind it, which is the same hang #154 identifies as the source of the leftover Postgres containers. Closes #337 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>	2026-09-09 16:36:34 -05:00
synAdminandClaude Opus 5	36dbf18916	feat(passkeys): a password reset takes the passkeys with it (#42) ... The last issue in the passkeys project, and the only one that adds no capability. It is the safety net, and it exists because this is the piece most likely to be skipped and most expensive to discover missing. A reset is the recovery path, and recovery has to be complete. The reset already deletes every session on the account, on the reasoning that a reset prompted by a compromise must not leave an intruder signed in for the remaining thirty days of their cookie. A passkey an intruder registered has no expiry at all. Leaving those behind would mean a customer can recover their password and still not have their account back. The obvious objection is that this hands whoever controls the mailbox a way to strip a customer's passkeys. It does, and it costs nothing: anyone who can complete a reset already controls the email address and therefore already controls the account. The passkeys were not protecting anything by that point. Anything in flight goes too. An intruder who pressed add a passkey moments before the reset could otherwise finish that ceremony afterwards and put a credential straight back onto the account the reset had just cleared. Changing a password deliberately does not do this, and the asymmetry is the point. A change requires the current password from someone already signed in, so nothing about it suggests a lockout or a compromise, and it already spares the current session for the same reason. A customer who suspects one particular device revokes that device by name from the account page, which is a better tool than deleting everything. A reset has no idea which credential is the problem, so it takes all of them. The customer is told twice. Before, in the reset email and on the reset form, unconditionally — that form has no session and is never told whether the account has passkeys, because answering that would make the reset page an oracle for it, so the wording has to read the same to someone who has none. After, with a count, and only when the count is not zero. That moment is the only one where the count can be reported: the rows are gone by the time anyone could go and look. A customer told two were removed who only remembers registering one has just learned something they could not otherwise find out. That notice is a panel that waits to be dismissed rather than a toast, because a toast dismisses itself and this is the message a customer needs to still be looking at while they decide what to do about it. The other question this issue asks — whether a reset ends a session established by a passkey — turns out to need no code, because #39 made both paths call one createSession. But "it falls out for free" is a claim, so there is now a test that establishes a session through that exact function and watches the reset end it. docs/ops/account-recovery.md records the whole policy, including the two answers that are not code. Losing an authenticator is not a lockout: the customer signs in with their password and revokes the lost credential themselves, which is why this change implements nothing for it. Losing the email address is a lockout, and there is deliberately no self-service route out — this shop holds no second proof of identity, and anything invented to fill that gap would be a weaker credential than the one it replaced. The manual route runs through the shop owner verifying against order history, and its third step, changing the address from the admin screen, does not exist yet. That is written down as a gap with its own notification and audit questions rather than smuggled in here. Verified: backend tsc clean for src and tests, 521 unit tests pass, lint clean apart from warnings that predate this branch; frontend tsc, lint and build clean. The integration and end-to-end suites need a database this machine has no Docker for, so CI is what proves those. Closes #42 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>	2026-09-09 16:03:39 -05:00
synAdminandClaude Opus 5	6d320fd867	feat(passkeys): schema, dependency and per-environment Relying Party (#37) ... Groundwork only. Nothing reads any of this yet, and no behaviour changes. The Relying Party ID is derived from PUBLIC_URL rather than written down, because it is the one value in this feature that cannot be corrected afterwards: a credential is bound to it permanently, and a wrong one surfaces only as a customer unable to sign in with a passkey that no longer matches anything. PUBLIC_URL is what every customer-facing link is already built from, so the ID is correct wherever those links are, and wrong only where they were already wrong. hostname rather than host, so a port cannot reach an ID that must not contain one. Local development is the exception the issue's table did not cover. envValidation requires PUBLIC_URL only when SMTP is configured, so a local setup that cannot send mail legitimately has none and falls back to localhost, which browsers treat as a secure context. Two origins there rather than one: the app is served by Vite on 5173 during development and by Express on 3000 once built, and those differ only by port, which is not part of the RP ID. The challenge table is separate from customer_tokens, and the reason is structural rather than preference. customer_tokens.customer_id is NOT NULL, and an authentication challenge is issued before anyone is identified — a discoverable-credential sign-in has no customer to attach to at the moment the challenge exists. Storing it there would mean making that column nullable for every other kind of token. Two of the issue's open decisions are deliberately not made here, because they belong to the ceremony that enforces them rather than to the schema. What to do when the signature counter fails to increase is #39's: many synced passkeys report zero forever, so treating a non-increase as cloning is wrong for them and right for a hardware key, and this only has to hold the value. Whether a disabled account can authenticate is also #39's, and the schema takes the position that it should not cost the customer their devices: credentials survive disabling and are refused at the ceremony, so re-enabling does not mean re-registering everything. Deletion is different and is settled here — credentials cascade with the customer, since one outliving its owner could authenticate as an account that no longer exists. signature_counter is BIGINT because the spec allows a 32-bit unsigned value, which overflows a signed INTEGER at half its range. That is the first bigint column in this schema, so the generated mirror gains the Int8 alias with it. Both tables are added to resetDb's TRUNCATE list and to REQUIRED_TABLES, and the schema mirror is updated by hand to match what kysely-codegen emits — placement and all, so a real regenerate produces no diff. Skipping either is how #56 turned a green local run into a red main; the mirror drift guard exists precisely to catch it, and schemaLoss's count moves from 18 to 20 with them. Verified: tsc clean for src and tests, lint 0 errors with no new warnings, 494 unit tests across 34 suites including nine new ones for the RP derivation, frontend build green, and the migration parses. Not verified: the migration has not been run against a database, and the integration suite needs one this machine cannot provide. Worth knowing before this goes further: #313 changes the domain, and every passkey registered before that cutover stops working at it. This code needs no change — it follows PUBLIC_URL — but the credentials do not survive. That is free while production is not live and nobody holds one, and it stops being free the day the shop opens. Closes #37 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>	2026-09-09 14:08:29 -05:00
synAdminandClaude Opus 5	2bc9440b38	test(ci): record which Postgres actually answered (#154) ... This adds the measurement #154 has needed twice and never had. It does not fix the failure and does not guess at it: the issue has been wrong twice from reasoning ahead of evidence, and the point here is to make the next occurrence answer the question rather than reopen it. The observation that rules out every explanation so far is that the schema comes back. A suite fails because orders does not exist, and a later suite truncating that same table passes. A dropped database does not un-drop itself, so this was never one database losing its schema. More than one server answering to one name produces exactly this, and Docker embedded DNS round-robins every container sharing an alias, so a leftover service container from an earlier run fits every observation including the empty dmesg that killed the OOM theory. globalSetup now logs every address the database host resolves to. More than one is the answer outright. One address means this reading is wrong too, and the next suspect is a single container restarted with a fresh data directory. Alongside it, both globalSetup and a failing assertSchemaPresent record which server actually answered. pg_postmaster_start_time is what settles that and needs no special rights: two Postgres instances cannot share one, so differing values within a single run are proof, where a differing inet_server_addr alone could be argued to be one container that moved. The failure message now says to compare the two rather than leaving the reader to know that is the interesting comparison. Logged on a passing run as well as a failing one, deliberately. A failing run's addresses mean nothing without a passing run's to compare them against, and this issue has twice suffered from having only the failure to look at. Neither can throw. A diagnostic that fails the run it was added to explain is worse than no diagnostic, so both are wrapped and both degrade to a printed reason. The failure path costs one extra round trip, taken only when the schema is already known to be missing. assertSchemaPresent is not on the hot path — resetDb calls it only when its TRUNCATE has already failed. Verified: tsc clean, typecheck:tests clean, lint 0 errors with no new warnings, and the four message patterns schemaLoss.integration.test.ts asserts on are all still present. The probe reads only pg_catalog functions, so it still answers against the dropped schema that suite creates. Refs #154 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>	2026-09-09 10:49:03 -05:00
synAdminandClaude Opus 5	5bba28bfda	fix(tests): the error-handling suite would not compile, and nothing local said so (#307) ... The trigger added in #322 does not typecheck. pg declares query with several overloads and jest.spyOn resolves the mock argument against the last of them, whose parameter list is empty, so the inferred type of a rejection value is never and an Error cannot be assigned to it. ts-jest compiles each suite as it runs, so this surfaced as a suite that failed to run rather than as a test that failed — which is why CI reported 446 tests passing, zero failing, and the step red anyway. The cast is the type system rather than a shortcut, and says so in the file: every overload rejects on failure, and the cast only chooses which one to check against. The reason this reached main is the part worth keeping. The backend has a tsconfig.test.json covering scripts and tests, and nothing was running it — build compiles src alone, and I had been reporting tsc clean on that basis while touching test files it never looked at. The frontend build has run its equivalent all along, so the gap was one workspace wide and invisible from the other. It is now a script, typecheck:tests, and it reproduces this failure in seconds against no database. That matters beyond this fix: the standing excuse for integration regressions here has been that the suite needs Postgres and a Node this machine cannot run, and a type error in a test was never actually in that category — it only looked like it because the check that finds it was never invoked. Not added to build. The Dockerfile runs that, and a production image should not fail to build because a test file has a type error. Verified: typecheck:tests clean, build tsc clean, lint 0 errors with no new warnings, 485 unit tests passing. The suite itself still needs CI to run. Refs #307 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>	2026-09-08 17:17:34 -05:00
synAdminandClaude Opus 5	8956b9f122	fix(items): read the id strictly, and stop the error test needing a route that does not (#307) ... Item 5. GET /api/items/:id was the last route reading its id with a bare Number(), so an unreadable id reached Postgres and came back to the caller as a 500 for an item that cannot exist. It answers 404 now, like every other id-taking route since #207. It could not be fixed on its own, which is why it stayed. errorHandling.integration.test.ts used this route's looseness as its way of making a handler reject: tightening the parse would have left that test green while removing the thing it tests. So the test now fails a database call directly, with a spy on pool.query against a route that makes one. That is the failure the error middleware actually exists for, and it does not depend on any route declining to validate — the previous comment's own conclusion, that moving the trigger to cart.ts would only move the wart. Two things fixed along the way that the issue asked about but that switching to readId would not have delivered on its own. readId was not as strict as its name suggests. Number reads 5.0, 1e2, 0x10 and +5 as 5, 100, 16 and 5 — every one a positive integer, so every check readId made passed and the route fetched a real row for a URL nobody wrote. /items/5.0 answered with item 5. This never raised an error and so never announced itself; the issue noticed it only because #308 converted the comparison to a real integer. An id is a string of digits, so it is matched against digits before being parsed. readId is also now bounded at the top of a 32-bit serial. Above that Postgres raises 22003 rather than returning nothing, which is the same wrong answer to the caller as the 22P02 the function was written to prevent — a 500 for an id that identifies nothing. The leak assertion was passing for the wrong reason. It checks the response does not contain "syntax" or "items", and the error it was checking against happened to contain both only by accident of which route was used. The injected failure now contains both words deliberately, and the whole message is asserted against as well, so a future error format cannot slip through by wording itself differently. Verified: tsc clean, lint 0 errors with no new warnings, 485 unit tests passing across 33 suites — seven of them new, covering the inputs above. The integration suite cannot run on this machine, so whether the rewritten error test passes is for CI to say. Item 4, coverage, is not closed by this and cannot be closed yet: the SonarQube scan step has been skipped on every recent run because it is gated on the earlier steps succeeding, and those steps were failing. The dashboard is therefore stale. Reported on the issue rather than guessed at. Refs #307 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>	2026-09-08 15:59:12 -05:00
synAdminandClaude Opus 5	e7196f440b	test(customers): let the public shape guard see analytics_consent (#56) ... The register route now returns analytics_consent, and customers.integration.test.ts asserts the exact key set the public customer shape may contain. That test failed in CI, which is the guard doing its job rather than a problem with it: its whole point is that the shape cannot quietly grow, and a field appearing without someone deciding it belongs there is what it exists to catch. This field does belong there, so the expected set gains it. Three cases added while here, all of them properties the compliance work depends on and none of them observable from a unit test. Analytics consent is off for a registration that does not mention it, which is what Quebec's Law 25 s.8.1 requires and needs the column default, the register route and the stored wording to agree. Opting in to marketing alone leaves analytics off, which is the bundling GDPR treats as invalid and the mistake this branch already made once. And an analytics-only opt-in works with marketing left off, so the granularity holds in both directions rather than only the convenient one. Found by CI rather than locally: the integration suite needs a database this machine has no Docker to run, which was called out as unverified when the change went up. Typechecked, linted and the 478 unit tests still pass, but the assertion itself is only proven by the next CI run. Refs #56 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>	2026-09-08 12:28:17 -05:00
bermudalambandClaude Opus 5	36e6057c6e	fix(tests): close the route guard's factory hole and two regex warnings (#307) ... Three of the items on the cleanup issue, and the first is the one that mattered. routesAreWrapped.test.ts could not see a handler built by a factory. `router.post('/x', rotationRoute('left'))` carries no async token of its own, so the guard read those two lines, found nothing to object to, and passed — which is not the same as finding them wrapped. That is how the rotation routes added in #301 went through a test that exists precisely because this convention had already been half-forgotten once, when thirty handlers were added unwrapped after the wrapper existed. It now follows a call to a function declared in the same file and reads its body the same way it reads a registration, so an unwrapped handler inside a factory is an offender. Proved rather than assumed: unwrapping rotationRoute's handler makes the suite fail naming admin.ts, where before it passed. Only same-file functions are followed, deliberately. app.ts registers express.json(), cookieParser() and uploadsRouter(), none of which is a handler factory and none of which can be resolved from the file being read — treating an unresolvable name as an offender would trade one hole for a permanently red test, so there is a case asserting those are left alone. The brace and paren walking is now one function rather than two. Adding the factory reader as a near-copy of registrationAt is what a cleanup commit should not do, and the duplicate carried its own cognitive-complexity and loop-counter warnings with it; parameterising the delimiter pair removes both the copy and the warnings it added. The schema mirror's table regex used `\s*` where kysely-codegen emits exactly two spaces and one after the colon, and `[A-Za-z0-9_]` where `\w` says the same thing. SonarQube flagged both, and the looser form bought nothing and backtracked for it. An empty status list would have compiled to `in ()`, which is a Postgres syntax error, where the `= ANY($n::text[])` it replaced in #308 was valid and matched nothing. It is unreachable through parseItemFilters, which refuses a list that names nothing — but the obvious guard is wrong in the opposite direction, because dropping the clause entirely would make an empty status filter match every status rather than none, so the empty case is spelled out as false. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>	2026-09-05 08:26:22 -05:00
bermudalambandClaude Opus 5	add28c5f16	fix(db): repoint the lint and drift guards at the new mirror (#305) ... The eslint config's ignores list and comment still named the deleted src/db-drizzle/schema.ts and relations.ts and never named src/db-kysely/schema.ts, so the generated mirror was being linted for the first time and tripping sonarjs/redundant-type-aliases — exactly the trap the config's own comment already described from #261 and #217. The ignores list now names src/db-kysely/schema.ts and the comment is updated to match. The schema mirror drift test built one flat Set of every two-space-indented key in the whole generated file and asked only whether a live column name appeared anywhere in it, rather than checking it against the specific table it belongs to. Seventeen column names are declared on two or more tables and created_at is on fourteen of eighteen, so a migration adding created_at, updated_at, status, name, sort_order, token, or expires_at to a table that lacks it would pass vacuously. Replaced mirroredTables with mirroredColumns, which reads the DB interface to map each table name to its declaring interface and then reads that interface's own columns, and changed the column-mirroring test to look up columns per table. Verified the guard can actually fail: removing customer_id from the Carts interface made the test fail naming carts.customer_id exactly, and restoring the file made it pass again. The root .gitignore still carried a comment block and two patterns for drizzle-kit pull output under backend/src/db-drizzle, a directory this branch deleted along with backend/drizzle.config.ts. kysely-codegen writes only the single tracked file it's pointed at, so nothing replaces the rule — deleted the block and both patterns. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>	2026-09-04 16:08:47 -05:00
bermudalambandClaude Opus 5	731716f760	docs(db): correct the drift test's own stale references (#305) ... The #217 doc comment above the describe block still named db-drizzle/schema.ts, drizzle-kit pull, and "Drizzle infers row types" — a file, a command, and a library this same commit had already removed. A comment pointing at deleted paths is worse than no comment at all on a test whose whole job is proving trust in a generated mirror, so it is corrected to name npm run db:types and src/db-kysely/schema.ts while keeping every sentence of the history intact: #217, #222, item_drafts and upload_links, the week nobody noticed. A closing note was added recording that the generator changed in #305 and the test did not, because the drift it guards is a property of generating a mirror at all rather than of any particular library. mirroredTables' regex also gets the same digit fix the column check already had. Both regexes parse the same generated file for the same kind of identifier, and a table name with a digit would otherwise be read out of the DB interface but reported missing by mirroredTables, sending someone to regenerate a file that was never wrong. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>	2026-09-04 16:08:47 -05:00
bermudalambandClaude Opus 5	c71b11e05e	refactor(db): swap the query builder from Drizzle to Kysely (#305) ... One commit, because a main that carries both builders is one where the next person converting a query has to guess which to reach for, and where two generated mirrors of one database can disagree. There was nothing to stage anyway: one file used the builder. The safety property that motivated adopting a builder at all is untouched, and was never the thing being traded. A value interpolated into a sql template becomes a bind parameter in either library, so #202's invariant stays a property of the type system and #180's hotspots retire either way. What changes is the three ways the old library made it easy to be quietly wrong, each verified in #297 against the SQL actually emitted: an array interpolating as a placeholder list unless every site remembered sql.param(), a column reference inside a raw fragment silently losing its table so a correlated subquery correlated with itself, and a camelCase mirror that had to be mapped back at every select or the JSON contract changed with no test noticing. CATEGORY_COLUMNS stops being a translation layer and becomes what it looks like — four column names four selects share. The generated types carry parent_id and sort_order because kysely-codegen emits the database's own names, so there is nothing left to map and nothing left to get wrong by forgetting to. The drift guard survives the swap rather than being rewritten, and loses its library name in the process: it is schemaMirror.integration.test.ts now, so the next such change renames nothing. It also got stricter for free. The Drizzle version had to match each column two ways and its own comment called that deliberately loose; a generated Kysely interface spells the database's name verbatim as a bare key, so one exact match is the whole rule and snakeToCamel is gone. isUniqueViolation keeps accepting both error shapes and now has a test behind it. Kysely uses the pg driver directly and should leave the SQLSTATE on err.code, but "should" is the word that turned two 409s into 500s when the last conversion moved it to err.cause.code with nothing failing to compile. Migrations are untouched. #219 stands, they remain hand-written node-pg-migrate files, and Kysely has no generator to refuse. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>	2026-09-04 16:08:47 -05:00
bermudalambandClaude Opus 5	3891f4fd75	feat(admin): endpoints to rotate one photo of an item (#301) ... Two POST routes and the module behind them. They live on the item rather than on the draft, and that is the decision that makes the inventory editor free when it follows: an image belongs to an item whether or not a draft row exists, so the second screen to want this is the same call from a different place with no new backend at all. A cut-out and its pristine original turn together. An image that has been through #281 has two files, and rotating only the displayed one would leave them disagreeing — Restore original would then quietly un-rotate the photo, so the undo of one feature becomes a regression of another. 204 rather than 200. Rotation changes no column: the paths are identical afterwards and only the bytes differ, so there is no row worth returning, which is the same reason deleting an image is already a 204. Only "not on this item" is a 404, and it is indistinguishable from an absent id on purpose, because an image id is a serial and this endpoint should not confirm which ones exist. Everything else stays loud as a 500, and the file is untouched in every one of those cases — rotateInPlace renames over the original only once the new file has been written. One asymmetry is recorded rather than engineered around: rotation is not idempotent the way background removal is, so a retry after a failure between the two files turns the displayed one twice. That needs the disk to break between two writes, and the remedy is one press in the other direction. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>	2026-09-04 13:46:29 -05:00
bermudalambandClaude Opus 5	d3ccc1b1f6	fix(uploads): apply the EXIF orientation before discarding it (#300) ... Photos arrived in the review queue rotated, in an orientation the sender never saw, and we were doing it to them. A camera does not turn its sensor data round. It writes the pixels as the sensor read them and sets an EXIF Orientation tag saying which way up they go, and every viewer honours that — which is why a portrait photograph looks upright to the person who took it and to the person who attached it. The re-encode from #226 rebuilds the file from decoded pixels and drops all metadata, which is right and is the whole point: a product photo should not publish the coordinates it was taken at. But it never applied the orientation first, so the sideways pixels survived and the one piece of information that explained them did not. The fix is sharp's rotate() with no argument, which reads the tag rather than turning the image by a fixed amount, placed before resize. The order matters: resize bounds width and height, and for a portrait photo those are the wrong way round until the rotation has happened, so a 3000x4000 photograph stored as 4000x3000 would otherwise be bounded on the wrong axis. Two tests, one of which is a fixture lesson. The fixture is a 400x200 image tagged Orientation 6 — the shape a portrait photo actually has on disk — and the assertion is that it comes back 200x400. The first version built it with withExif({ IFD0: { Orientation: '6' } }), which sharp reads back as orientation 1: a fixture carrying no orientation at all, which would have passed against the unfixed code and proved nothing. It uses withMetadata({ orientation: 6 }) instead, and the comment says why so the next person does not repeat it. Confirmed by removing rotate() and watching the test fail. The second test pins that the tag itself still goes, so nothing downstream rotates the image a second time. This does not repair the photos already uploaded. Their EXIF is gone, so nothing records which way up they were meant to be, and the originals kept for #281's cut-outs were themselves re-encoded on the way in. Those need a person and a rotate button, which is #301. Closes #300 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>	2026-09-04 13:05:08 -05:00
bermudalambandClaude Opus 5	0b6cc85c4f	fix(lint): bring the backend test suites into scope (#298) ... The backend lint script covered src and scripts; the frontend's has always covered src and tests. So roughly sixty backend test files had never been linted at all. That was a documented deferral rather than an oversight — the config said so in as many words, because tsconfig.json includes only src and type-aware rules had no program to resolve the test files against. tsconfig.test.json is that program, exactly as frontend/tsconfig.test.json was for the same problem in #137. It is separate from tsconfig.json rather than a widening of it, because that one drives the build and emits to dist, and pulling the suite in would ship the tests. The files were already type-checked at run time by ts-jest; this adds nothing to that, only to what the linter can see. Pointing it at tests produced 77 warnings and no errors. Sixty of those were rules that cannot be true in a test, so they are switched off here rather than left to accumulate — #60's argument, that a gate nobody reads is not a gate, and that a rule which cannot be true is noise hiding the rules that can. Forty-one alone were hardcoded passwords, which are the entire point of a test and which this project's own rule says must live only in test paths, which is here. The rest were a stub server on http to a socket the test opened itself, an RFC 5737 documentation IP, os.tmpdir, Math.random for a run id, and sorting two arrays to compare them. What was left was signal, and it found a real one on the first run. testDb.ts cleaned up settings with LIKE 'email\_%', and in a JavaScript string that backslash does nothing: the pattern is 'email_%', and an underscore in SQL LIKE matches any single character. It meant "email plus any one character" rather than "email_". It deleted the right rows only because no other key begins with those letters followed by something else — a setting called emailing_enabled would have been swept away between suites, silently, in a file that never mentions it. It now uses an explicit ESCAPE clause. It also found five dead `const before: string[] = []` declarations in uploadValidation, left over from #228's redesign of that suite. The tests assert properly through filesSettlingTo; the variables did nothing. Seven warnings remain, all in routesAreWrapped and workflowGate, and all judgement calls about guard-test complexity rather than defects. Leaving them visible is the point of having lint here at all. Closes #298 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>	2026-09-04 12:41:58 -05:00
bermudalambandClaude Opus 5	445c9c4a22	fix(backgrounds): report a partial restore instead of throwing (#293) ... restoreOriginalsForItem rethrew anything that was not NoOriginalToRestoreError, which handed asyncRoute a bare 500 and discarded how far the restore had already got. That breaks the invariant the feature is built on: photos restored before the failure really are back, and an admin standing in front of the modal needs the count to decide whether pressing the button again is worth anything. RestoreSummary now carries `failed` and the loop stops and reports, exactly the shape and the reasoning removeBackgroundsForItem already had. The restore-originals route gains the missing 404 for an item that does not exist — remove-backgrounds always had it, and the two handlers are copy-paste rather than a shared helper, so nothing would have caught them diverging. draftingWorker's .catch is now only reachable if the image-listing query itself throws, since removeBackgroundsForItem no longer rejects over a single photo; its comment says so rather than describing behaviour that has moved. The `failed` branch is covered by a unit test that stubs the database module in its own module registry. It cannot honestly be an integration test: the only failure the function can report is a database fault, and the only way to inject one into a real run is to interfere with the single pool every integration suite in the --runInBand process shares and that afterAll calls pool.end() on. Two tests that did exactly that are removed here — they left the suite reporting a failure against its own afterAll and leaking a handle that stopped it exiting. Nor is the fault reachable through data alone: the swap's WHERE original_image_path IS NOT NULL guarantees the value it writes into the NOT NULL image_path, and item_images carries no unique, check or foreign-key constraint on either column, so no row can be seeded that makes the statement fail. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>	2026-09-04 12:02:23 -05:00
bermudalambandClaude Opus 5	8f35204995	feat(admin): remove or restore every background on an item (#293) ... Two routes on the item, and a small admin config route so the inventory screen can know whether to offer them. Both answer 200 once the id is valid, even when the sidecar fails, and that is a deliberate departure from the per-photo endpoints in #281. Those act on one image, so the request either worked or it did not and 502 says which. These act on several, so "did it work" has no single answer — two of four is the normal shape of a bad day here, not an exception — and a 502 would throw away the count that is the only thing making the outcome actionable. Non-200 is reserved for not being able to try at all, which here means an unreadable or absent id. No status check on either. A sold item's photos are still the shop's photos and improving them changes nothing about the sale; the guards on unpublish protect a checkout in progress and a completed sale, neither of which is at stake in a photograph's background. The config route follows adminVersion's precedent rather than extending the public /api/config: admin-only, one purpose, and the reason written down. The inventory screen had no other way to learn the feature exists, because GET /api/admin/items answers a bare array with several consumers and reshaping it for one boolean is the worse trade. Also extends the admin item select to carry original_image_path on each image, behind a new ADMIN_IMAGES_SUBQUERY kept separate from the shared IMAGES_SUBQUERY the public select uses. Task 3 needs to derive its restore-button label from that field, and the server never sent it for items before this — only the drafts endpoint carried it, added by #281 for the review queue. It stays admin-only for the same reason itemSelect.ts already names PUBLIC_ITEM_SELECT's columns explicitly: an internal original filename is nobody's business on the storefront, and sharing one subquery would put it in every public item response. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>	2026-09-04 10:26:19 -05:00
bermudalambandClaude Opus 5	6f3e77fa88	feat(intake): report what a whole-item background removal actually did (#293) ... removeBackgroundsForItem answered void and threw on the first failure, which is enough for the drafting worker — it catches and logs, and a draft is not worth failing over — and not enough for an admin standing in front of a screen who needs to know whether the thing they pressed happened. It now returns a summary: how many photos the item has, how many carry a cut-out, and whether it stopped early. It still stops at the first failure. Six attempts against a sidecar that is not answering helps nobody, and stopping costs nothing because removeImageBackground skips a photo that already has an original recorded, so a retry resumes rather than starting over. The count is what turns that retry into an informed choice instead of a guess. Adds restoreOriginalsForItem alongside it. A photo that was never cut out is skipped rather than refused, because the mixed state a partial removal leaves behind is exactly when somebody reaches for this. The one existing caller does not change: the worker ignores the result, and ignoring a returned value is legal, which is what makes this additive rather than breaking. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>	2026-09-04 10:16:04 -05:00
bermudalambandClaude Opus 5	5a9022d8d1	test(integration): mock the mailer everywhere an upload link is issued (#260) ... intake.integration.test.ts, intakeCeiling.integration.test.ts and uploadLinks.integration.test.ts all create upload links, and since #260 that now sends real mail. env.setup.ts never clears SMTP_USER, SMTP_PASSWORD or MAIL_ALLOWLIST, so with those inherited from a developer's shell these three files opened live TLS connections to smtp.gmail.com:465 and, with no allowlist set, actually delivered to sarah@example.com. This is the exact hazard the "Do not add one back" comment in tests/unit/mailOutcome.test.ts already warns about, reintroduced at the integration layer. All three now mock ../../src/mailer the same way accountDetails.integration.test.ts, favorites.integration.test.ts and resendVerification.integration.test.ts already do. uploadLinks.integration.test.ts is the one place that needs to see specific MailOutcome values come back through the route, so its two outcome tests were restructured to drive the mock's return value directly (sentMail.mockResolvedValueOnce(...)) instead of threading SMTP_USER/MAIL_ALLOWLIST through the real sendMail. That is a cleaner test anyway: it isolates the route's job (reporting whatever outcome sendMail returns) from sendMail's own skip logic, which is already covered hermetically by mailOutcome.test.ts and mailAllowlist.test.ts. Also addresses the related minor finding that nothing asserted the mail actually carried the working link: required: ['submitUrl'] on the template only guards that the placeholder is present in the body, not that the route supplied a correct value for it. A new test in uploadLinks.integration.test.ts inspects the mock's captured call and asserts the html contains the created token's /submit/ URL, and covers the three submissionsAllowed phrasings (a numeric cap, a cap of exactly one, and uncapped). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>	2026-09-03 18:30:35 -05:00
bermudalambandClaude Opus 5	b18b3e3a3d	feat(intake): require an address for an upload link and send the link to it (#260) ... Creating a link now requires a valid email address and mails the link to it, which is the whole point: getting a link to a contributor was previously a copy-and-paste into whatever the admin happened to use. The send is awaited and its outcome reported, unlike every other sender in this codebase, which fires and forgets because nobody is waiting on the answer. Here somebody is. The admin is looking at the screen, and whether they now have to send the link by hand is exactly the thing they need to know — and QA blocks delivery to any address outside MAIL_ALLOWLIST by design, so a link that was never emailed would otherwise look precisely like one that was. A send that could not happen does not roll the link back. The token is displayed exactly once, so a rollback would leave the admin retrying and holding a different link, discarding work that had succeeded. They end up with a usable link and an honest statement about delivery instead. One inaccuracy left deliberately: an SMTP rejection is reported as skipped-unconfigured rather than a fourth outcome of its own. The distinction is real but nothing consumes it, and the admin's next action is identical either way. Also updates the other integration tests that created a link with only a label, since an address is now required, and adds the uploadLink template key that GET /api/admin/email-templates was missing from its list — an omission left by the template's addition in the prior commit on this branch. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>	2026-09-03 18:00:15 -05:00
bermudalambandClaude Opus 5	be6800181d	fix(admin): let a setting whose default is empty actually be cleared (#280) ... intakeNotifyEmail and intakeCeilingResetAt both document empty as their default and as a working configuration — no notification address, and no ceiling reset recorded. The validator refused every empty text value, so either could be set and then never removed through the admin at all; the only way back was a DELETE against admin_settings. An admin who turned intake notifications on could not turn them off. Whether empty is a mistake is a fact about the setting rather than about its type, so it is now declared on the setting, in the DEFINITIONS row that already carries its type and fallback. A new setting states it once, in the place someone adding one is already editing, and nothing else has to know. That is what makes this different from special-casing two names in the validator, which would have left the next such setting to rediscover the same bug. The blanket refusal stays the default, because for a setting with a non-empty fallback an empty value really is a mistake: an empty greeting format renders every greeting as nothing at all, which reads as a broken email rather than as something a person cleared. Both those cases keep their tests. Whitespace is normalised to empty rather than stored. Somebody clearing a field they cannot see the end of leaves spaces behind, and they meant cleared. The tests check that the clearing survives the request rather than only being echoed back — the last one sets a value, clears it, and then reads it again through GET, which is the assertion that would have caught this had it existed. Closes #280 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>	2026-09-03 16:51:32 -05:00
bermudalambandClaude Opus 5	d5e599a30e	fix(admin): answer 404 for an unreadable id instead of 500 (#207) ... The issue asked for two things and only one had been done. A well-formed but absent id already answered 404 — the PUT route carries a comment saying so. A malformed one still reached Postgres as text, raised 22P02 on an integer column, and surfaced through the route's catch as a 500, telling the admin the server had broken when the truth is that no such item can exist. That half is now closed everywhere rather than on the three routes that happened to have it. Guarded: DELETE an item, DELETE an image, unpublish, and every route in adminItemDrafts — publish, regenerate, discard, restore, and the two background-removal endpoints added by #281. The last of those were flagged in that feature's own final review as sharing this pre-existing shape, so they are fixed with the rest rather than left to be found again. Routes carrying two ids guard both. A route can guard the first and forget the second, and the forgotten one fails exactly as loudly, so there is a case each way for both image endpoints and for DELETE image. DELETE deliberately still answers 204 for a well-formed id that is absent. The method is idempotent and the caller's intent, that the item should not exist, is satisfied either way; what must not happen is a 500. There is a test pinning that so the distinction is a decision rather than an omission. Also replaced the raw req.params.id and Number(req.params.id) uses that sat inside routes which had already computed a validated id. They were safe, because the guard above them made them safe, but a validated id and a raw one side by side in the same handler is how this bug comes back. The test block named "a non-numeric id on every admin item route" covered two routes. It now covers every route that takes an id, which is what makes its name true. Closes #207 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>	2026-09-03 16:45:51 -05:00
bermudalambandClaude Opus 5	6f8a0db130	test(integration): clean up background-removal test-harness leftovers (#281) ... backgroundRemoval.integration.test.ts exported seedSubmission for no reason — nothing imports it, since draftingBackgroundRemoval.integration.test.ts and adminItemDrafts.integration.test.ts each wrote their own seeding helpers. Dropped the export, kept the function for local use. All three of these suites create a temporary uploads directory with mkdtemp and point UPLOADS_DIR at it, but none of them removed the directory afterward or restored the previous UPLOADS_DIR value — checked and the leak existed in all three, not just the one the review flagged. Each afterEach now removes its temp directory with fs.rm and restores (or deletes) UPLOADS_DIR to what it held before the test touched it, so this suite no longer leaves rubbish in the OS temp directory or a stale environment variable for whatever runs after it in the same process. This is test scaffolding cleanup, not a feature change — no runtime path in the application deletes anything. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>	2026-09-03 13:43:25 -05:00
bermudalambandClaude Opus 5	dfd900aadd	fix(admin): stop reporting every removeImageBackground failure as a sidecar failure (#281) ... The remove-background route's catch block turned every throw from removeImageBackground into a 502 "the background-removal service did not answer". But removeImageBackground also throws for an unrecognised file extension (a legacy .jpeg), for a file missing from the uploads volume, and when REMBG_URL is not set at all — none of which involve contacting the sidecar. The admin was told to retry a service that was never reached, while the real reason existed only in the server log. Added SidecarRequestError in rembgClient.ts, following the NoOriginalToRestoreError pattern already in backgroundRemoval.ts. It is thrown only for failures that happen after actually attempting to reach the sidecar: the fetch call itself throwing (now wrapped in a try/catch, covering unreachable and timed-out), a non-2xx response, or a response that is not a PNG. It is deliberately not thrown for "REMBG_URL is not set", since that path never attempts contact at all. The remove-background handler now checks err instanceof SidecarRequestError before answering 502; everything else answers 500 with a message that says what actually went wrong. Added a unit test pairing (rembgClient.test.ts) asserting the sidecar-contacted failures are SidecarRequestError and the unconfigured case is not, and an integration test (adminItemDrafts.integration.test.ts) proving a missing upload file answers something other than 502 with a message that does not claim the service did not answer. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>	2026-09-03 13:42:47 -05:00
bermudalambandClaude Opus 5	b06eac4640	fix(intake): clear remove_background when an admin restores a photo (#281) ... item_drafts.remove_background is written once at intake and never updated afterward. Restoring a photo clears original_image_path, which is exactly what makes the row look "never cut out" to removeImageBackground — so a submitter's ticked checkbox, followed by the worker cutting the photo out, followed by an admin restoring a poor result, followed by a click on Regenerate, would silently re-cut the same photo the admin had just put back. Nothing was lost, but the control the design calls "what makes a poor result survivable" was quietly defeated by the button sitting next to it. restoreImageOriginal now swaps the image's paths back and clears item_drafts.remove_background for that item in one transaction, so a restore that succeeds while the flag update fails cannot reintroduce the bug. An admin restoring any photo on an item is treated as overriding the submitter's original request for the whole item — the flag is per-item while the swap is per-photo, so there is no narrower place to record the decision, and turning off the whole item's auto-removal is the conservative direction: the alternative is re-cutting something a person deliberately undid. Added an integration test in draftingBackgroundRemoval.integration.test.ts that drafts a submission with the intent set, cuts it out, restores it, mirrors what the admin's Regenerate button does (state back to queued, attempts cleared), runs the worker again, and asserts the photo is still not cut out. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>	2026-09-03 13:42:24 -05:00
bermudalambandClaude Opus 5	984e329c47	fix(admin): narrow the restore-original catch to the race it exists for (#281) ... Round 1 caught every throw from restoreImageOriginal() and reported it as a 404, on the theory that losing the concurrent-restore race is the only way that call fails. But the throw carried nothing to distinguish that race from a genuinely different failure during the same UPDATE — a dropped database connection, a transient outage — so a real failure was now silently reinterpreted as "someone already restored this" instead of surfacing as the loud 500 it was before. backend/src/intake/backgroundRemoval.ts now exports NoOriginalToRestoreError, a named subclass of Error thrown in place of the bare Error restoreImageOriginal previously threw. The message text is unchanged, so backgroundRemoval.integration.test.ts's rejects.toThrow(/no original/) assertion keeps passing without modification. backend/src/routes/adminItemDrafts.ts catches that class specifically in the restore-original handler and rethrows anything else, so a real failure still reaches the app-level error handler and comes back as a 500 instead of being mislabeled as "already done". backend/tests/integration/adminItemDrafts.integration.test.ts adds a test that spies on restoreImageOriginal via jest.spyOn on the module namespace (the project compiles to CommonJS, so the route's call site reads the export off that object at call time, which makes the spy effective without jest.mock) to reject once with a plain Error, and asserts the response is 500 rather than 404 — proving the narrowing changes real behavior, not just internal structure. The spy is restored in a finally block. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>	2026-09-03 13:01:36 -05:00
bermudalambandClaude Opus 5	5129112266	fix(admin): answer 404, not 500, when two restores race (#281) ... restore-original's precheck (existing.original_image_path === null) and restoreImageOriginal's own guard (WHERE ... AND original_image_path IS NOT NULL) could disagree under a race: two concurrent restores, or a rapid double-click, could both pass the precheck before either commits, and the loser's UPDATE would then match zero rows and throw. The handler had no try/catch around that call, so the throw propagated through asyncRoute to the app-level error handler and the caller got a bare 500, breaking the route's documented 200 | 404 contract even though the row itself was left correct. Wraps the restoreImageOriginal call in a try/catch, matching the shape remove-background already uses in this file, but answering 404 rather than 502: losing this race means another admin already finished the restore, not that a downstream service failed. Adds a comment on the catch explaining why it exists, and a test that fires two restores concurrently and asserts neither comes back 500. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>	2026-09-03 12:56:15 -05:00
bermudalambandClaude Opus 5	9ced34ad19	feat(admin): remove and restore a photo's background per image (#281) ... Adds two admin-gated endpoints on the review queue router: POST /:itemId/images/:imageId/remove-background and POST /:itemId/images/:imageId/restore-original. Both run synchronously and reuse the same backgroundRemoval module the drafting worker uses, so a cut-out obtained either way is identical and either can be undone by Restore. Ownership is scoped by item as well as by image (imageOfItem selects on id AND item_id), because the image id is a serial and guessing one is easy — a photo belonging to a different submission must not be reachable through another item's URL. A sidecar failure returns 502, not 500, and leaves the row untouched, since removeImageBackground only writes the row after the cut-out file already exists on disk. GET /api/admin/item-drafts now returns { drafts, backgroundRemoval } instead of { drafts }, and each image in the payload gains original_image_path, which is what the review queue UI will use to decide between "Remove background" and "Restore original". DRAFT_SELECT's images aggregate is extended accordingly, keeping the deliberate column spelling that guards against the upload_links token digest leaking into the response. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>	2026-09-03 12:49:38 -05:00
bermudalambandClaude Opus 5	885a78c572	feat(intake): record whether the submitter asked for a cut-out (#281) ... The public intake POST now reads a `removeBackground` multipart field and stores it on the new `item_drafts.remove_background` column. The checkbox on the submission page is ticked by default, so a client that sends nothing gets `true` — only the exact string `'false'` opts out, so a stray or unexpected value is treated as consent rather than a silent refusal. The GET now also reports `backgroundRemoval: isRembgConfigured()` alongside the label, so the submission page knows up front whether the feature exists in this environment at all. Neither handler calls the sidecar or the AI — this task only records intent for the drafting worker to act on later, and the existing ordering of `requireUsableLink` and `requireCapacity` ahead of `uploadImages` is unchanged. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>	2026-09-03 12:44:20 -05:00
bermudalambandClaude Opus 5	81524a2849	feat(intake): cut out backgrounds in the worker, never in the upload (#281) ... Wires removeBackgroundsForItem into draftQueued, gated on the submitter's remove_background intent recorded on item_drafts. The step runs after the draft is committed and catches for itself, so an unreachable or erroring sidecar never turns a draft that was written correctly into a failed one — the photo simply keeps its original, and the admin's per-photo control in the review queue is still there to do it by hand. It is awaited, unlike the notification below it, so a sweep that has returned has finished its work; nothing on the request path waits on it. Adds backend/tests/integration/draftingBackgroundRemoval.integration.test.ts as a new file rather than extending drafting.integration.test.ts, because that suite has never produced a successful draft and therefore has no draftListing mock — adding one there would be file-wide and would change what its existing tests exercise. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>	2026-09-03 12:38:49 -05:00
bermudalambandClaude Opus 5	8d12cb2f2d	feat(intake): swap a photo for a cut-out, keeping the original (#281) ... Adds backend/src/intake/backgroundRemoval.ts, the shared module the drafting worker and the admin endpoints both call so a cut-out obtained either way is undoable the same way. cutoutPathFor is pure and writes a new file beside the original rather than overwriting it, which is what keeps the original restorable and makes the JPEG-to-PNG change free. removeImageBackground only points the row at the new file after it is already on disk, and is idempotent via the original_image_path IS NOT NULL check — load-bearing twice, since it also stops a second pass from recording the cut-out as the original and losing the real one for good. restoreImageOriginal swaps the paths back and deliberately leaves the cut-out file on disk. Extends the Task 1 integration test file with a stub sidecar bound to an ephemeral port and covers the no-op-on-repeat case plus three failure modes (500, non-image body, unreachable), asserting the row is left untouched in every failure case. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>	2026-09-03 12:34:56 -05:00
bermudalambandClaude Opus 5	534e3d6228	feat(intake): record the background-removal intent and the original path (#281) ... Adds the two columns the background-removal feature (#281) is built on: item_drafts.remove_background (boolean, not null, default true) records the submitter's per-submission intent, and item_images.original_image_path (nullable text, no default) records where a cut-out photo came from so it can be restored. The default on remove_background is load-bearing — any row written by a path that does not mention the column behaves like the new default, so no backfill is needed. original_image_path stays null until a photo has actually been cut out, which doubles as the answer to "can this be restored?" rather than needing a separate flag. Also updates the Drizzle mirror in src/db-drizzle/schema.ts by hand (the local dev database was not running to re-pull from) and adds the integration test backgroundRemoval.integration.test.ts, including the exported seedSubmission helper that Task 3 will reuse. Closes #281 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>	2026-09-03 12:15:38 -05:00
bermudalambandClaude Opus 5	813216f971	test(integration): say so when the database loses its schema (#154) ... When the integration run lost its schema partway through, it presented as 36 assertion errors about categories, price filters and favourite notifications. The real message — `relation "items" does not exist` — was further down the same log, and hours went into chasing the assertions instead. The cause is still open and needs runner-side evidence this cannot reach: whether the Postgres service container is being recreated mid-run, which would come back with an empty data directory. This is the half that can be fixed from here — whatever the cause, the next occurrence reads as "the database lost its schema" on the first line, names which tables are gone, and says that nothing in the suite drops tables so the database was replaced underneath the run. globalSetup asserts once after migrating, which establishes the fact the rest of the run depends on. Without it, a run that never had a schema and one that lost it midway are indistinguishable from the failures they produce. resetDb checks only when its TRUNCATE fails, rather than on every reset. It runs in a beforeEach several hundred times a suite, and an extra round trip each time to guard against a rare event would be paying continuously for it. When the schema is fine, an unrelated failure is passed through untouched rather than dressed up as a schema problem. The diagnostic is tested against a database that has actually lost its schema, not reasoned about. An earlier attempt dropped the schema before the run and proved nothing — globalSetup re-migrates, so it repaired itself and every test passed. The suite drops and rebuilds around each case and restores in afterAll; the full integration suite was then run twice to confirm the restore holds for everything ordered after it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>	2026-09-02 18:43:54 -05:00
bermudalambandClaude Opus 5	2b8be434c0	test(uploads): stop the cleanup assertions racing the cleanup (#228) ... discardUnlessAccepted unlinks from a res.on('close') handler, so nothing awaits it and nothing can. `await request()` resolves when the response completes, which is when close fires — so a read taken straight afterwards races the unlink it is meant to observe. The property is an eventual one and the assertions were synchronous. The issue counted three tests. Injecting a 400ms delay into the unlink to make the race deterministic showed five: "leaves nothing on the volume when it refuses the content" and "discards the valid files from a request that also carried an invalid one" race too, and are not in the describe block the issue named. It also showed that polling alone is not enough. The race runs in both directions, and the second direction is easy to miss: a deletion still pending from the *previous* test corrupts the next test's baseline before its request is even sent. No amount of waiting fixes a baseline that is already wrong. My first attempt waited for the directory to look quiet, which only works while the unlink is faster than the wait — precisely the assumption this issue is about, and it still failed four tests under the injected delay. So the baseline is removed as a variable: beforeEach empties the directory, every test starts from empty, and the assertions poll for the expected count. Any orphan from an earlier suite goes with it, which is correct — the directory is temporary and nothing outside these tests owns it. discardUploads already catches per-file errors, so a pending unlink finding its file gone logs and moves on. Verified by injecting the 400ms delay again: four to five failures before, twelve passing after, with the production file restored untouched. Closes #228 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>	2026-09-02 09:45:44 -05:00
bermudalambandClaude Opus 5	e7fd63c2e6	feat(db): land the Drizzle schema, config and conventions (#217) ... Infrastructure only. No route is converted, nothing changes at run time. The mirror had already drifted, which settles how it should be maintained. schema.ts was missing item_drafts and upload_links from the moment #222 landed, because the spike pulled into ./drizzle and copied the file into src/ by hand, and nobody had reason to look at the copy for a week. So `out` now points at src/db-drizzle and pull refreshes in place — the copy step that made the drift possible is gone — and tablesFilter excludes pgmigrations, which is node-pg-migrate's bookkeeping and has no business in a model of the application's schema. A stale mirror is worse than no mirror, because Drizzle infers row types from it: a converted query would type-check against a schema the database does not have and fail at run time on a column that does not exist. drizzleSchema.integration.test.ts fails when the two disagree, on tables and on columns. It was checked by removing item_drafts from the mirror and confirming the test fails naming it, rather than trusting a green run on a file that already matched. pull also emits 0000_*.sql and meta/ into `out`, because that directory serves both purposes. Both are gitignored: this project's migration history is backend/migrations, hand-written and mostly prose, and #219 has not chosen otherwise — a stray SQL file in src/ is at best noise and at worst mistaken for real history. db is exported beside pool and shares its connections. Both must work at once, since conversion is file by file across 187 sites; separate pools would make a transaction on one invisible to the other and silently double the configured limits. The generated files are excluded from linting. #261 hand-fixed an unused-parameter warning in schema.ts and this re-pull put it straight back, which is the argument in one line: linting generated code buys a fix the next regeneration undoes. itemFilters.drizzle.ts, which is hand-written, is still linted. CONVENTIONS.md records the sql.param() array trap before anyone hits it — the wrong form type-checks, reads correctly and fails at run time as invalid Postgres — and the reason the adoption is worth doing at all, which is that ${value} emits a bind parameter and there is no way to spell "interpolate this as SQL" by accident. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>	2026-09-02 09:33:09 -05:00
bermudalambandClaude Opus 5	f52976dec9	fix(admin): answer 404 for an item id that does not exist (#207) ... Three routes in admin.ts answered a miss with a success. PUT /items/:id ran an UPDATE that matched nothing, committed happily, selected nothing back and replied 200 with an empty body — a success the admin client could do nothing with, and no record anywhere that the item was not found. mark-sold and mark-available did the same. The create route beside them has always used requireRow for exactly this, which is why this reads as an oversight rather than a decision. A garbage id was worse in a different direction. Number('abc') is NaN, the driver sends it to Postgres as the text "NaN", Postgres raises 22P02 for an integer column, and the catch turned that into a 500 — so a caller asking for an item that cannot exist was told the server broke. Both now answer 404, because from the caller's side "/items/abc" identifies no item in exactly the way "/items/999999" does. readId is shared rather than repeated, and rejects zero, negatives and fractions as well as text: every id in this schema is a positive serial, so anything else identifies nothing. mark-sold now notifies favouriters only after the row is known to exist, so nobody is told about a sale that did not happen. The issue asked for the same shape to be checked across the other admin routes. It was: unpublish already looks the item up and 404s, and the tags and categories PUT routes both do an existence check before their UPDATE, so their rows[0] is guaranteed. items.ts already guards the public read. These three were the only ones lying about a miss. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>	2026-09-02 09:33:09 -05:00
bermudalambandClaude Opus 5	92c04847d6	feat(intake): refuse submissions past the daily ceiling, and reset it from the admin (#227) ... The check is ordered ahead of uploadImages, for the same reason requireUsableLink is: a refused submission must write zero bytes. Ordering it after would accept the upload, store the files and then throw them away, which is the expensive half of the work the ceiling exists to prevent. A test asserts nothing is stored. 503, not 403. The sender has done nothing wrong, their link is fine, and the condition clears by itself as the window rolls — so the link stays usable and works again the moment there is room. The ceiling never touches the admin upload path, which has its own test. Intake being throttled is an inconvenience; the shop being unable to add its own stock is an outage. reset-ceiling is declared above /:id/revoke because Express matches in order and would otherwise read it as an id and try to revoke a link named "reset-ceiling". That has its own test too. The per-link alert is a named function rather than the inline IIFE the plan wrote. routesAreWrapped.test.ts flags any async inside a route registration not directly wrapped in asyncRoute, and it cannot tell an inner IIFE from an unwrapped handler — nor should it have to. The guard caught this, and the extraction reads better than what it rejected. Backend now 373 unit and 337 integration, all passing. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>	2026-09-01 16:29:00 -05:00
bermudalambandClaude Opus 5	22215f32eb	feat(intake): settings for the submission ceiling (#227) ... A count type beside the existing hours, text and choice readers. resolveHours would have worked — it is parseFloat with a positive guard — but calling a submission ceiling an "hours" setting is a lie in the type name that every later reader has to decode. Whole numbers only, so a ceiling of 12.5 is a typo rather than a preference, and a malformed value falls back rather than yielding a NaN that compares false against everything and silently disables the limit. resetDb is widened to clear intake_ settings as well as email_ ones. It deliberately does not truncate admin_settings, so a ceiling of 1 left behind by one suite would make every later suite's submissions refuse with a 503, in files that never mention a ceiling. The comment there already records that exact failure happening once with an email template subject, which reached the favorite-alert tests and failed five of them somewhere else entirely. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>	2026-09-01 16:19:45 -05:00
bermudalambandClaude Opus 5	346e9eae4c	feat(intake): act on a signed link from the notification (#224) ... Mounted publicly, deliberately not behind requireAdminGate. These are clicked from an inbox by someone who is not signed in, which is the whole point; the signature is what protects them. GET confirms and changes nothing, POST acts. Mail scanners and corporate link-rewriting gateways issue a GET against every URL in a message before a human sees it, so a GET that discarded a draft would fire itself on delivery — carrying a valid signature, looking entirely legitimate in the log, and nobody would know to go and recover it. That is the case the split exists for and it has its own test. Forged, replayed, upgraded and expired links are each refused with the same 403. Distinguishing them would tell somebody probing which of those they had achieved. There is no signable publish, and asking for one finds no handler. The two registry guard tests are updated rather than worked around: they assert the full set of settings and template keys, so adding either is exactly what should trip them. Backend now 367 unit and 329 integration, all passing. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>	2026-09-01 15:32:07 -05:00
bermudalambandClaude Opus 5	d8a7588685	feat(intake): regenerate, discard and restore a draft (#225) ... Regenerate clears attempts along with the state. The worker only picks up rows below the attempt cap, so re-queueing a draft that has already failed three times without clearing them would produce a button that appears to work, does nothing, and leaves nothing anywhere to say why. Discard deletes nothing — not the item, not the photographs. It is one click away in what amounts to an inbox, and the photos are often the only copy of something no longer in the sender's hands, so the destructive reading of the word is deliberately not available here. The item returns to pending, because a discarded submission must not stay on sale. Restore returns a draft at the state its own contents justify rather than unconditionally ready. A submission discarded before it was ever drafted has no copy, and coming back as ready would present an empty draft as a finished one. Judged on whether a name was ever written, because the state held before discarding is not stored. Backend now at 346 unit and 317 integration tests, all passing. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>	2026-09-01 14:15:22 -05:00
bermudalambandClaude Opus 5	44a9037121	feat(intake): publish a reviewed item to the storefront (#225) ... The only path from an intake submission to the storefront. It performs what mark-available performs — the status, and clearing the sale and reservation fields — rather than calling that route, because the copy and the publish have to be one transaction: an item published carrying the previous draft's name would be worse than one not published at all. The price rule is applied here rather than trusted from the client. A changed number becomes the admin's; an unchanged one keeps whatever it was, so publishing without touching the field records that nobody chose it. The row is locked for the transaction so two admins publishing the same submission cannot interleave one's price decision with another's name. Whole cents only. A fractional value would round somewhere nobody is looking and put the item on sale at a price no one entered. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>	2026-09-01 14:11:51 -05:00
bermudalambandClaude Opus 5	81886f84c2	feat(intake): list submitted items waiting for review (#225) ... Columns are spelled out rather than selected with a wildcard, so a column added to item_drafts later does not silently start reaching the browser. That matters most for the join to upload_links, which carries the token digest — only the label is taken, and a test asserts the digest never appears in a response. Discarded rows are excluded by default rather than deleted. Discard has to be recoverable because it is one click away in what amounts to an inbox, but a discarded row left in the default view would compete for attention with work that still needs doing. The gate goes on the mount in app.ts rather than inside the router, matching every other admin router. Since ADMIN_GATE_SECRET is unset for integration runs the gate is disabled there, so the test that asserts the mount is actually gated sets the secret for its own duration — leaving requireAdminGate off a new mount is otherwise a silent hole. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>	2026-09-01 14:10:38 -05:00
bermudalambandClaude Opus 5	e0d7e92e4b	feat(intake): draft queued submissions without ever losing one (#223) ... The governing rule is that a submission is the only irreplaceable thing in this pipeline. The photos are often the only copy of an item no longer in the sender's hands, so a missing key, an unreadable file, a failed call and three exhausted retries all end the same way: the item keeps its photos, stays pending, and waits. Nothing in this file deletes anything. An absent key returns early and spends no attempt. Counting it as a failure would mean a fortnight without a key exhausted the retries and marked every waiting submission failed, with nothing wrong with any of them. A failure leaves the row queued while tries remain, so the sweeper picks it up again, and failed once they are spent, so a dead submission stops costing money and waits for a person instead of retrying forever. Photos are read once and passed down rather than loaded again inside the drafting call — the first read already has to happen to check there is at least one. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>	2026-09-01 08:32:40 -05:00
bermudalambandClaude Opus 5	511022d248	feat(intake): record a draft without publishing it (#223) ... The copy goes on item_drafts, never on the item. The item keeps its placeholder name and description until a person approves them in the review queue (#225) — nothing a model wrote reaches the catalogue unreviewed. The price is the deliberate exception, because #220 chose to price an item on arrival rather than leave it unpriced. price_source records that the number came from a model rather than a person, so the review queue can show it as unconfirmed. With no suggestion the item keeps the migration's 8000 default and price_source stays 'default'; the queue shows both the same way, as a number nobody has chosen yet. A category is checked against the real table before it is stored. The schema constrains the shape of the answer but cannot enforce membership, and a category the shop does not have would be invisible to every storefront filter — a draft nobody could find, rather than an obvious error. A successful retry clears ai_error, or a draft that eventually worked would still read as broken in the queue. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>	2026-09-01 08:32:40 -05:00
bermudalambandClaude Opus 5	d27dcae62b	feat(admin): choose the drafting model from Settings (#223) ... The model was going to be an environment variable, which meant a redeploy to change it. It is now an admin setting, so it can be changed from the Settings page like the cart expiry and the greeting. A dropdown validated on the server, not a free-text field. The API only rejects an unknown model at the point of use, so a typo would be stored happily and then fail on every submission, surfacing as drafts quietly not appearing rather than as an error anybody could act on. The PUT refuses anything outside the offered set, and getSettings falls back rather than handing on a value that is no longer offered — drafting with the default beats drafting with a model the API will refuse. One catalogue rather than two lists. The dropdown needs the models, costMicros needs their rates, and the price shown beside a model in Admin has to be the price it is actually billed at, which it cannot be if the two are maintained separately. Rates were confirmed against the pricing page rather than recalled: Sonnet 5 $2/$10, Opus 5 $5/$25, Haiku 4.5 $1/$5 per million tokens. The unknown-model fallback is deliberately the most expensive rate and never zero, because a budget that reads as unspent however much was spent is the one failure a spend guard cannot have. Adding a third setting type pushed getSettings past the cognitive complexity limit, so the per-type resolution moved out into one small function each — the same shape the definitions block above it already argues for. The exhaustive assertion in the GET test gained the new field rather than being loosened. It exists to catch a setting silently vanishing from the response, and that is worth more than not having to touch it here. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>	2026-09-01 08:32:40 -05:00
bermudalamb	1fc632598a	feat(intake): accept photo submissions through a shared link (#222) ... The public way in. Photos of one item plus a free-text note, from someone with no account, landing as an `items` row at status 'pending' — already invisible to every public and storefront query since #90, so nothing is live by accident. Every refusal is a 404. Unknown, revoked and exhausted links are indistinguishable from outside, because whether a link exists is not something a stranger needs to be able to learn — the same reasoning uploads.ts applies to files. The link is resolved *before* multer runs, and that ordering is the point rather than an implementation detail. discardUnlessAccepted would delete the files afterwards, but "written then deleted" is materially worse than "never written" on an endpoint the whole internet can reach: it is disk churn an unauthenticated caller controls, and it leans on an unlink that a crash between write and delete would skip. A test asserts the volume is untouched for a bad token, so a future reordering fails loudly instead of quietly handing that control away. The link counter is incremented inside the transaction and guarded on the same conditions as the lookup, so two submissions racing for the last slot of a capped link cannot both succeed. The response carries no item id: the sender has no business knowing about the catalogue and nothing they could do with it. The AI is deliberately not called here. A slow or failing model request must not turn into a failed upload for someone who did nothing wrong, and the photos may be the only copy — the item is often no longer in the sender's hands. The row waits at state 'queued' for #223. The new limiter keys on the caller alone, since a submission carries no email. keyByCallerAndEmail's comment warns that a bare ip bucket is a shared allowance, and that trade is taken knowingly: the link is the per-caller identity and its cap is the per-caller bound, while this limiter does the different job of bounding what one address can throw at an endpoint that writes files. Twenty per fifteen minutes is deliberately looser than the password-reset allowance — somebody photographing a box of stock legitimately submits several in a row, and refusing them costs a consignment. Because the route mounts the shared uploadImages, it inherits the type allowlist, the magic-byte check and #226's EXIF stripping without asking for any of them. A test asserts the stripping specifically, since this is the route where it matters most: the photo comes from a stranger's phone rather than the shop's own camera. Backend: 284 integration (12 new), 309 unit, lint unchanged at 6 pre-existing warnings, build clean. Ref #222	2026-08-31 15:42:26 -05:00
bermudalamb	3392f6f10d	feat(intake): issue and revoke named upload links (#222) ... Three routes behind the admin gate: list, create, revoke. A link is named because provenance matters more than convenience — when one is shared further than intended the question is which one, and every submission will record the link it arrived through, so revoking kills that link rather than the feature. The token is returned by exactly one response and is unrecoverable afterwards, which is why the admin screen has to present it as a one-time reveal. The listing selects its columns explicitly rather than `SELECT *`, so `token_hash` cannot reach a response the moment somebody adds a convenience — and a test asserts the listing carries neither the token nor the digest. An absent `maxSubmissions` gets a bounded default of 25 rather than null. Absent means nobody decided; an explicit null means unlimited, which is a decision visible in the request. Reading absent as unlimited is what would quietly make every link unbounded, and the common case is the one that has to be safe. Revoking is idempotent through COALESCE, and a test asserts the second call returns the *same* timestamp rather than merely succeeding. The useful fact is when access ended, and a button that errors on a double-click teaches people to distrust it — which is the last thing wanted on the control that contains a leak. Mounted above the `/api/admin` catch-all, which would otherwise swallow the path, and behind requireAdminGate on the router itself per the reasoning in middleware/adminGate.ts. Lint caught me reintroducing something this codebase had already solved: I wrote `.replace(/\/+$/, '')` to trim PUBLIC_URL, and app.ts carried a hand-written loop with a comment explaining that exact regex backtracks. Rather than duplicate the loop, trimTrailingSlashes moved to utils.ts and both callers now share it. Backend: 272 integration (9 new), 308 unit, lint back to its 6 pre-existing warnings, build clean. Ref #222	2026-08-31 15:42:25 -05:00
bermudalamb	6df32af784	feat(intake): add upload_links and item_drafts, and default an item's price (#222) ... The schema for the intake pipeline. A submission becomes an `items` row at status 'pending' — already invisible to every public and storefront query since #90 — with an `item_drafts` row beside it holding the submitter's note, which link it arrived through, and the fields the drafting worker will fill in later. `upload_links` stores a digest rather than a token, so a leaked database is not also a leaked set of working links, and the admin screen can show a token exactly once. `max_submissions` is nullable for "no cap", but the route will default it to a finite number: an unbounded link should be something asked for, not something that happens when nobody thought about it. `item_drafts.upload_link_id` is ON DELETE SET NULL rather than CASCADE. Deleting a link must not delete the items that arrived through it — provenance is lost, the goods are not. `items.price_cents` keeps NOT NULL and gains a default of 80.00, so an arriving item is always priced. That is the decision taken in the design review over making the column nullable: it costs the schema-level guarantee that nothing can publish at a price nobody chose, and buys not having to teach the cart, the checkout and thirteen other files about an item without a price. The protection moves into the review queue, and `price_source` exists so that queue can say whether a number came from a model, the default, or a person. The number lives in the migration rather than in configuration. Changing a default price is a rare, deliberate act that deserves a record; an environment variable would let it drift silently between environments, and a wrong default is invisible until something has already sold at it. Verified up, down and up again rather than only forwards — an irreversible migration is one that cannot be tested. Then verified by inspection rather than assumption: the default reads 8000, both tables and the state index exist, and an item inserted with no price comes back at 8000. Backend: 263 integration, 302 unit, all passing against the new schema. Ref #222	2026-08-31 15:42:25 -05:00