submitUrl, label and submissionsAllowed were added to the uploadLink template but SAMPLE_VALUES had no entries for any of them, so an admin opening Email templates, Upload link for a contributor, Preview saw literal {{submitUrl}} in the body — the preview being the only way to check an edit before saving.
The unit test meant to catch exactly this, in emailTemplates.test.ts, iterated a hardcoded KEYS array that predated intakeDraft and uploadLink, so it never checked either template's samples. KEYS is now Object.keys(TEMPLATES) as TemplateKey[], so the guard covers every template automatically and cannot go stale the same way again. intakeDraft already had samples for all its placeholders and passes once included, as expected.
One other test in the same file, "every template can address the customer", asserts that available contains greeting/firstName/lastName — a real invariant of the six customer-facing templates, but not of intakeDraft or uploadLink, which notify the shop and a contributor rather than a customer with a name on file. Switching that test to the new all-templates KEYS would have made it fail for both, so it now uses its own explicit CUSTOMER_FACING_KEYS list instead. That is a deliberate, commented exception: a hardcoded list is correct there because the claim itself does not extend to every template, whereas the SAMPLE_VALUES guard's claim does.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
adminUploadLinks.ts awaits sendMail on the admin's request path, the first awaited send on a user-facing request in this codebase, but the transporter in mailer.ts set no connectionTimeout, greetingTimeout or socketTimeout. nodemailer's defaults then apply: two minutes to connect, ten minutes on the socket. If the SMTP host is unreachable in a way that drops packets rather than refusing, the link row and its token are already committed by the time sendMail is called, the response hangs for up to two minutes, the browser or reverse proxy gives up first, and the token — shown exactly once and unrecoverable — is never rendered. That is the link being lost in exactly the way this feature's central invariant forbids.
All three timeouts are now set to 5000ms, with a comment explaining why a send on a request path has to fail fast rather than inherit nodemailer's fire-and-forget defaults. Five seconds is generous for a reachable host and short enough that a dead one fails while the admin is still willing to wait, leaving them the "not emailed" warning and a link they can still copy instead of a stuck spinner and a token nobody ever saw.
Also corrects the comment directly above the skipped-blocked return, which said "returning as though it sent" — true before #260, and precisely backwards now that the outcome is reported through MailOutcome rather than swallowed. Reworded to describe what the code actually does today, keeping the explanation of why it skips rather than throws.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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>
The address is now a required field beside the label, the links table shows where each link was sent, and the admin is told plainly when the mail did not go — with the link still on screen to copy, which is the case that matters in QA and in local development where there is no mail at all.
Two specs in unrelated features created links with only a label and the route now refuses that, so they are updated here rather than left to go red on somebody else's branch. That is the cost of making the address required, and it is a small one: the compiler and the suite find every call site.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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>
Adds upload_links.contact_email and the uploadLink mail template.
The column is nullable on purpose. Links already exist in QA and a migration cannot invent addresses for them, so they are grandfathered rather than backfilled with something untrue; the requirement belongs in the create route, which is where new links are actually made.
The template requires submitUrl, the same guard verification has on verifyUrl. An email inviting somebody to send in photos, with no way for them to do it, sends perfectly happily and looks fine in the log — it is the one failure here worth making impossible, and a test asserts the default body satisfies the guard it declares.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Review of the Task 1 commit approved the implementation but flagged the third test in mailOutcome.test.ts as a defect carried over from the brief: it set SMTP_USER, SMTP_PASSWORD, and an allowlist the recipient satisfied, so sendMail fell through both early-return guards and reached the real transporter, opening a live TLS connection to smtp.gmail.com:465. The .catch(() => 'threw') wrapper hid a fast auth rejection, a slow timeout, or an accidental real send equally, and on a restricted CI runner it would hang to the Jest timeout rather than fail fast.
The fix deletes that test rather than replacing it. What it was trying to prove — that an allowlisted recipient is not blocked — is already covered hermetically by backend/tests/unit/mailAllowlist.test.ts, which exercises isAllowedRecipient directly across exact matches, plus-suffixes, domains, and refusals. The other two tests in mailOutcome.test.ts are untouched; they cover the two paths that return early, which is the entire point of the change, and neither one reaches the transporter.
The file's top doc comment is updated to match: it now says only the two skip paths are covered here, names mailAllowlist.test.ts as where the allowlist's own behaviour is tested, and spells out why a third test that reaches the transporter does not belong in this file, so nobody adds one back.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
It returned Promise<void> and returned early in two cases that were indistinguishable from success at the call site: no SMTP credentials, and a recipient outside MAIL_ALLOWLIST. A caller could therefore report that it had emailed somebody a message nobody would ever receive, and in QA — which restricts delivery deliberately, as its entire safety property — that is the normal case rather than an edge one.
It now returns a MailOutcome saying which of the three happened. No existing caller changes: there are seven and every one ignores the result, so this is additive. Re-deriving the answer at a second site would have duplicated isAllowedRecipient and the SMTP check, which is exactly the drift the guard-in-one-place comment above them exists to prevent.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Four tasks over the approved design: sendMail gains an outcome, the column and template land together, the route requires an address and reports what the send did, and the admin screen asks for it.
sendMail goes first deliberately. Everything else depends on being able to tell a skipped send from a real one, and it is the only change touching a file seven other things already use — so if it is going to break anything, it should break before three tasks are stacked on top of it.
The plan is explicit that no existing caller changes. Ignoring a returned value is legal, which is what makes widening the return type additive rather than breaking, and re-deriving "would this address be blocked?" in the route would have duplicated isAllowedRecipient and the SMTP check at a second site.
Two specs in unrelated features create links with only a label, and the route will refuse that. They are fixed in the same task as the form rather than left for the suite to find, because the alternative is two unrelated features going red on somebody else's branch. That cost is named in the plan rather than discovered.
One inaccuracy is left in deliberately and said out loud: an SMTP rejection is reported as skipped-unconfigured rather than a fourth outcome. The distinction is real, nothing consumes it, and the admin's next action is identical either way — copy the link and send it by hand.
Self-review caught the failure mode from #281, where tasks referred to helpers that did not exist. Task 4 originally said the created-link state "may not be called created". Reading the component showed it is `issued` and holds a bare URL string with nowhere to put a delivery outcome, so the plan now adds a separate `mailed` state beside it rather than widening the one-time token display. Every name in that task is now one that exists.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The issue asked for the security question to be settled before any code, and it is: the mail carries the working link.
The issue framed that as a loosening comparable to a password reset, and on inspection that framing overstates it. A reset token takes over an account. An upload token grants exactly one capability — submit photos into a queue where a person must approve them before anything reaches the storefront. It reads nothing, it is revocable, max_submissions caps it, and #227 caps the whole intake surface regardless of any single link. The worst outcome of a leaked upload link is junk in the review queue, which is bounded and reversible. That is a reasonable thing to put in an inbox, and this project already makes the much larger bet with reset links.
The address is required for new links while the column stays nullable, which is not a contradiction: links already exist in QA and a migration cannot invent addresses for them, so they are grandfathered, and the requirement belongs in the route where new links are actually made. It lives on the link rather than on a contributor entity, because a link already carries a label naming who it is for and nothing yet suggests the same people submit repeatedly.
A failed send does not roll the link back. The token is shown exactly once, so a rollback would leave the admin retrying and holding a different link, discarding work that succeeded. The link is created, the send is attempted, and the response says which happened — which matters concretely because QA's MAIL_ALLOWLIST silently skips any address outside it and returns as though it sent. Without an explicit outcome, testing this in QA against a contributor's real address looks exactly like success, which is the afternoon the issue warned would otherwise be wasted.
Writing it turned up one thing the design had assumed and the code does not support. sendMail returns Promise<void> and returns early both when SMTP is unconfigured and when the recipient is not allowlisted, so a caller cannot tell either from success. It gains a MailOutcome return value instead. No existing caller changes — there are seven and every one ignores the result — and the alternative would have duplicated isAllowedRecipient and the SMTP check at a second site, which is the drift the guard-in-one-place comment in mailer.ts exists to prevent.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The end-to-end helper opens its own connection to read a password-reset token, and nothing guaranteed it pointed at the database the application was actually using. With -E2eDb the app runs on redefined_e2e at 55501 while the helper kept its redefined_local default at 55500, so the app wrote to one database and the suite read another — and the specs failed for a reason that had nothing to do with them.
start-local.ps1 now records the coordinates it chose in .local/database.json, and run-tests.ps1 reads them into TEST_PGHOST, TEST_PGPORT, TEST_PGUSER, TEST_PGPASSWORD and TEST_PGDATABASE before Playwright starts. The answer now comes from one place, written by the thing that made the decision at the moment it made it.
It is written after Start-Database rather than before, so the file never names a database that failed to come up, and removed by -Stop, so a stopped stack does not leave a record pointing at a container that is gone.
Setting all five closes the second fault in the same change. Invoke-IntegrationSuite sets TEST_PGPORT and PowerShell keeps it for the rest of the session, so a -Suite all run leaked the integration port into the e2e run that followed — with none of the matching credentials, leaving the helper offering redefined_local's password to the integration database. Overwriting every one of them is what makes that leak harmless.
A missing record throws rather than falling back. A default is what produced both faults in the first place: always plausible, silently wrong, and it fails in ways that look like application bugs rather than configuration.
The guard test is the point of the change as much as the fix is. This is the third instance today of two files having to agree with nothing comparing them — #107 and #118 were envValidation against a compose file, #287 was the workflow against start-local.ps1, and this is three files rather than two. The test pins the whole chain: that the writer records the five settings, that the runner reads each of them from the record rather than a default, and that the helper reads no connection variable the runner does not set. Removing a single line from the runner fails four of its assertions, which was checked rather than assumed.
What it cannot do is run PowerShell, so these are text assertions against the scripts. That is weaker than executing them and still catches the drift that actually happened.
Closes#273
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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>
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>
main went red on 86095cb with "Playwright: 157 passed, 3 failed" while every other step in the job passed. The three are the background-removal cases from #281, and they fail for the most boring reason available: REMBG_URL appears nowhere in .gitea/workflows, so isRembgConfigured() is false, both endpoints report backgroundRemoval as false, and neither the submitter's checkbox nor the review queue's per-photo button is rendered for the tests to find. The application is right and the tests are right; the environment they run in was missing one variable.
Deliberately a URL that does not resolve. Nothing in the suite reaches the sidecar — the worker only cuts a background out after a draft is written, and drafting needs an ANTHROPIC_API_KEY this job does not have. Standing up a real rembg in CI would cost a 4.24 GB image and about forty seconds of startup to prove that a control is on screen.
How it got in is worth recording, because the missing line is the symptom. #281 added the variable to scripts/start-local.ps1 and stopped there, and its three end-to-end tests were merged unrun — verified locally against a stack that did have the variable, then handed over as outstanding rather than run before the merge. A test that has never been run is not evidence of anything, and two of the three would have failed on the first honest execution.
The deeper gap is that a variable which changes rendered behaviour now lives in two places nothing compares. composeEnvironment.test.ts exists for exactly this failure and caught it twice before, in #107 and #118, but it guards the deploying compose files rather than the CI workflow. Whether it should reach into .gitea/workflows is a decision rather than another hand-added line, and #287 leaves it open.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Two assertions named a fixture and expected it visible in the unfiltered grid. No paginated catalogue can promise that — the item is on some page, not necessarily the first — so both would have started failing the moment paging landed. They were only ever proxies for "the result set got bigger", and the visible total lets them say that directly, which is what the issue predicted when it asked for a count.
The new cases assert the control and the URL rather than which item is on which page, because the development database never truncates and which item lands where is not something a test may rely on. That is the same trap the two rewritten assertions had fallen into, and repeating it in new tests would have been worse than leaving them alone.
Writing them found a real defect rather than just covering the feature. The control was rendering while the catalogue was still loading, showing "0 items" for a moment before the real count arrived — the empty-state early return only fires once loading has finished, so a mid-load render fell through to the grid branch with a total of zero. It is now suppressed until there is something to count, which is both true and what makes the count usable as a signal in a test. StorefrontPage.totalItems waits for the control for the same reason: reading during the load returned zero and quietly made "the result set shrank" compare against nothing.
The conditional skips carry a file-level eslint exception with its reasoning rather than being left to add four warnings. They are honest about a real limit: against a catalogue of ten items or fewer these cases prove nothing, and if the e2e database is ever seeded that thinly they need fixtures of their own instead.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The page number joins the filters in the URL, which is already the single source of truth for what the storefront is showing. That is the whole reason numbered pages were chosen over infinite scroll: a page is a place you can send someone, and a scroll position is not. The page size deliberately does not go there — it is a preference belonging to one person, and putting it in the URL would mean sharing a link to an item also imposed your page size on whoever opened it.
Changing a filter returns to page one, and it does so for free: filtersToSearchParams builds a fresh URLSearchParams, so applying filters drops the page parameter while goToPage copies the existing params and keeps the filters. That is behaviour worth having rather than an accident to tidy up — landing on page seven of a two-page result is a state a customer cannot get out of without understanding the URL.
The control carries the total, because showing the count was a requirement in its own right and the only count that existed before this was on the filter drawer's "Show N items" button, which is hidden whenever the drawer is closed. It is therefore shown even when everything fits on one page: hiding the control on a single page would hide the count with it.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Every decision paging needs, as pure functions: which page a URL is asking for, which page is actually showable given how much there is, which slice of the items that is, and what page size to use. Pure because that is the only thing this project can unit-test — vitest runs in a node environment with no jsdom and no testing-library, so a hook or a component is only reachable through Playwright. Keeping the rules here means the rules have tests and the React wrapper stays thin enough not to need any, which is the same split filters.ts already uses for the URL.
An unrecognised page size is refused rather than clamped. A stored or hand-edited 5000 would render the entire catalogue in one page, which is the exact failure this issue exists to prevent, and clamping would quietly honour a value nobody offered. Storage access is guarded on both sides because localStorage is absent when there is no window and throws outright in some privacy modes, and neither is a reason for a customer to lose the catalogue — the worst acceptable outcome of a broken preference is the default.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Three tasks over the decisions recorded on the issue: the pure paging rules with their tests, the wiring into the catalogue, and the end-to-end cases including the two assertions this finally lets say what they mean.
Two decisions the issue left open are settled here. The page number goes in the URL beside the filters, because a numbered page being linkable is the whole reason numbered pages were chosen over infinite scroll, and the URL is already the single source of truth for what the storefront is showing. The page size deliberately does not, because it is a preference belonging to one person — putting it in the URL would mean sharing a link to an item also imposed your page size on whoever opened it. It lives in localStorage instead, which also keeps it inside the issue's own scope boundary of not touching the API.
The third open question, whether page size changes with viewport, is answered no and written down as such rather than left silent. A size that moved on rotation would fight the preference the customer had just set.
Everything testable is a pure function, because that is all this project can unit-test: vitest runs in a node environment with no jsdom and no testing-library, so a hook or a component is only reachable through Playwright. That is the same split filters.ts already uses, and it is why the React wrapper is thin enough to need no test of its own.
Writing it turned up one piece of luck worth not breaking: filtersToSearchParams builds a fresh URLSearchParams, so applying a filter already drops the page parameter and returns to page one, while paging copies the existing params and keeps the filters. The plan says so explicitly so nobody 'fixes' it later.
It also confirmed the issue's own warning. filters.spec.ts:165 asserts a named fixture is visible in the unfiltered grid, and favorites-filter.spec.ts:106 does the same — with ten items to a page over thousands, both would start failing the moment paging landed. They were always proxies for 'the result set got bigger', and a visible total lets them assert that directly.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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>
ANTHROPIC_API_KEY's explanatory paragraph already had a dangling continuation trailing after later entries. When REMBG_URL was added, its entry was inserted ahead of that continuation, so the file read as though "put a spend limit on the key in the Anthropic console" belonged to the background-removal sidecar rather than to Anthropic. This file is read during the cutover runbook, so a misattributed sentence there is not just cosmetic.
Reordered the comment lines so ANTHROPIC_API_KEY's full paragraph is contiguous and REMBG_URL's own two-line entry stands on its own at the end. No environment: line was touched — only the comment block above the services: section.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The module comment on backgroundRemoval.ts said "The original file stays on disk and so does every cut-out ever made." The original half is true and load-bearing; the cut-out half is not. cutoutPathFor is deterministic, so a photo that is restored and then cut out again overwrites the previous cut-out at the same path. Harmless — no original is ever touched — but the comment overstated what the module guarantees. Corrected it to say what is actually true.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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>
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>
The DraftQueue background-removal control gated both "Remove background" and "Restore original" on the same `backgroundRemoval` flag, which only reflects whether a sidecar is currently configured. Restoring is a pure database swap and never calls the sidecar, so once photos had already been cut out and REMBG_URL was later removed from the stack, the admin was left looking at a cut-out photo with no control at all and no way back to the original short of a hand-written SQL UPDATE — directly breaking the "the original is always restorable" invariant the feature is built on.
DraftCard now computes `enabled` per photo as `backgroundRemoval || image.original_image_path !== null`, so Restore original stays available whenever a photo has an original regardless of whether the sidecar is configured, while Remove background still requires a configured sidecar. Also corrected the docstring on `DraftQueueResponse.backgroundRemoval` in draftsApi.ts, which claimed the flag hides "the control" generically — it only ever governed the remove-background control.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Adds the per-photo control that closes out the background-removal feature: each photo in the review queue now gets a "Remove background" or "Restore original" button, whichever matches its current state, and the button only appears when the server reports a sidecar is configured. The label is read from original_image_path alone rather than a second flag, so there is nothing that could disagree with what the button actually does.
draftsApi.ts's fetchDrafts now returns { drafts, backgroundRemoval } instead of a bare Draft[], matching the breaking change Task 6 made to GET /api/admin/item-drafts. DraftImage gains original_image_path, and a new setImageBackground(itemId, imageId, action) posts to the remove-background/restore-original endpoints, preferring the server's error message the same way publishDraft does.
Also updates docs/ops/image-background-removal-stack.md: the status line no longer says "evaluated, not adopted", since the feature is adopted here, and the closing "If this is adopted" section is replaced with "How the application uses it", describing the two real entry points (the drafting worker's default-on checkbox, and this per-photo control) and confirming that nothing in the feature deletes a file or a row.
Adds an e2e case asserting the button's label appears on a freshly submitted item's card, scoped to that card by the sender's note per #241. It is unrun in this environment — the local stack was not started, per standing instruction not to run start-local.ps1 or Playwright without the user's supervision.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Adds a checkbox to the public submission page that lets a sender opt out of background removal, ticked by default because most items look better cut out and the reverse default would mean almost nobody got it. It only renders when the server reports the sidecar is configured, matching the intake link's new backgroundRemoval flag from Task 5 — an unconfigured environment gets no checkbox rather than one that would do nothing.
submitItem now takes removeBackground as a required fourth parameter, sent as the multipart string 'true' or 'false' to match the backend's exact-string opt-out contract. Making the parameter required rather than optional was deliberate, so the compiler would catch any call site left unupdated; the frontend build (which also type-checks tests/ via tsconfig.test.json) confirmed the only call site, in Submit.tsx, was updated.
scripts/start-local.ps1 now sets REMBG_URL for the local backend so the checkbox is visible during local and e2e runs; the value need not resolve, since no e2e submission reaches the sidecar without a configured drafting step.
Adds two e2e cases to intake-submit.spec.ts: the checkbox appears ticked by default, and a sender can uncheck it and still submit successfully. Both are written per the task-7 brief but not run in this session, since running Playwright requires the full local stack (database, backend, frontend dev server) which was not started.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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>
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>
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>
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>
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>
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>
Adds the client that will let the intake path remove backgrounds from submitted photos via the rembg sidecar over HTTP. isRembgConfigured() reports whether REMBG_URL is set (unconfigured is a normal, working state, not a failure), and removeBackground() posts a file to /api/remove and resolves with the PNG bytes it gets back, rejecting on every failure — unconfigured, unreachable, a non-2xx response, or a body that fails the same magic-byte PNG check the upload path already uses.
The one hard rule: every request names model=u2net explicitly and this is never configurable. The sidecar's default model, reached simply by omitting the parameter, is bria-rmbg, which is licensed non-commercial — a licensing problem that a shop cannot silently ship, and one that would produce a perfectly good image with nothing in it to reveal the mistake. The test that posts against a real stub HTTP server and asserts model=u2net appears on the wire is the only thing guarding against that regressing.
Wires REMBG_URL into both docker-compose.qa.yml and docker-compose.prod.yml as an optional variable, right after ANTHROPIC_WORKSPACE_ID, following the existing style in each file's environment block and header comment. It is deliberately left out of envValidation.ts's ALWAYS_REQUIRED — requiring it would make an environment with no sidecar refuse to boot, which is exactly the failure mode this feature is designed to avoid.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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>
Eight tasks over the approved design, each ending in something independently testable: the two columns, the sidecar client, the shared swap-and-restore, the worker step, the intake route, the admin endpoints, the submitter's checkbox, and the review queue's per-photo control.
Three things the plan pins down that the spec left to implementation.
The `model=u2net` assertion lives in a unit test against a real stub HTTP server rather than a mocked fetch, because what has to be checked is the shape of the request that reaches the wire. Nothing in the returned image would reveal that the non-commercial default had been used, so that assertion is the only thing standing between this and a licensing problem that produces perfectly good pictures.
Removal in the worker follows drafting rather than running on its own pass, which couples the two: an environment with no ANTHROPIC_API_KEY drafts nothing and so cuts out nothing. That is the deliberate trade — a separate pass would re-attempt an unreachable sidecar on every five-minute sweep for a row that is going to sit at `queued` indefinitely — and the plan says so in the worker's own header comment rather than leaving it to be rediscovered.
`removeImageBackground` is idempotent through the `original_image_path IS NOT NULL` check rather than a separate flag, and that guard is load-bearing twice: it makes a repeat call a no-op, and it stops a second pass recording the cut-out as the original and losing the real one for good.
Writing it turned up two things worth knowing about the existing tests. `drafting.integration.test.ts` has never produced a successful draft — every case in it either has no key or no readable photo — so the worker's new cases need their own file with `draftListing` mocked, rather than a mock added file-wide to a suite that deliberately never reaches the model. And `adminItemDrafts.integration.test.ts` calls `request(app)` directly with no helper, so the plan spells out the seed it needs instead of pointing at one that does not exist.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The design behind #281, written before any code so the decisions can be argued with while they are still cheap to change. The implementation follows on this branch.
Six decisions, each recorded with what it rests on rather than just what it is. The one that matters most is that `model=u2net` goes on every request: the sidecar's default is `bria-rmbg`, which is licensed non-commercial, and it is reached by simply not specifying a model — a silent licensing problem that produces a perfectly good image. A test asserts the parameter is present, because nothing in the output would reveal its absence.
The other consequential one is that the submitter's tick records an intent rather than doing the work during their upload. Inline removal would make them wait, would put a CPU-heavy model run in a path anyone holding a link can trigger — the surface #227 exists to bound — and would force a choice, when the sidecar is unreachable, between failing their submission and silently ignoring what they asked for. Recording the intent means the submission always succeeds and keeps its original photo, and the cut-out arrives with the AI draft seconds later.
Everything else follows the rule the pipeline already runs on: a submission is the only irreplaceable thing here. The original is never destroyed, every failure path leaves the photo exactly as it was, and an unset REMBG_URL means the feature simply does not exist rather than that the environment is broken.
Documents what is not established too — quality on a real photograph is unknown, because the engine evaluation used a generated rectangle on a flat ground. The per-photo control and Restore original are what make a poor result survivable rather than something to prevent.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The list finally arrived from the reporting added earlier, and confirmed what #181 could only suspect: these are not the five eslint-plugin-sonarjs warnings that issue lists. Those were fixed under #261 and the count staying at five was a coincidence. It is six now, 25 minutes of debt, and one of them was mine.
admin.ts imported '../utils' twice — I added readId in #207 without noticing the file already imported from there. One import now.
filters.ts had a redundant `as ItemStatus[]`. TypeScript narrows an array through `.every()` with a type predicate from 5.5, and this project is on 5.9, so the assertion stopped telling the compiler anything. Removed, and the build confirms the narrowing holds without it.
adminSettings.ts was the only CRITICAL: cognitive complexity 18 against a limit of 15, almost all of it three near-identical loops differing only in how they validated. Each validation is now a small pure reader returning a refusal rather than sending one, and the handler is one loop over a table. Adding a setting type means adding a row.
That refactor is deliberately behaviour-preserving. Two things were left alone on purpose: the blanket rejection of empty text, which is wrong for the two settings whose documented default is empty and is filed as #280 rather than folded in where it would be invisible; and the absence of the `count` settings, which no caller submits and which the admin screen has no control for. I had started adding count validation and reverted it — widening behaviour under cover of a complexity fix is how a refactor stops being reviewable.
The three S6478s are render props, not components defined during render. ErrorBoundary's `fallback` is typed `(error: Error) => React.ReactNode` and called as `this.props.fallback(...)`, so React only ever sees returned elements and never a new component type — the subtree destruction the rule describes does not happen, and the rule's own message offers `allowAsProps` for this shape, which cannot be set from here. Hoisted rather than suppressed because none of them closes over anything local, so at module level each is one stable function instead of a new closure per render. That is a mild improvement, not a contortion.
Verified: 402 backend unit, 358 backend integration, 30 frontend unit, 157 e2e, both lints clean, both builds clean. The e2e run matters most here — storefront-errors.spec.ts exercises all three hoisted fallbacks, and it was run on its own first.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Measured rather than described. Everything in here was run against danielgatis/rembg:latest on 2026-09-02 and the numbers are from those runs, with the caveat recorded that a dev box under Docker Desktop is not the NAS.
The finding that matters is the default model. This image downloads bria-rmbg on first use, and BRIA's RMBG models are licensed for non-commercial use — the same trap that ruled out @imgly/background-removal-node, reached silently by making one request. Sending model=u2net explicitly is both the licensing answer and ten times faster: 1.1-2.3s against 14-20s for a 2000x1500 image, and 168MB on disk against 977MB. That speed difference decides the interaction on its own, because a click can wait two seconds and cannot wait twenty.
Also records the volume path, which most documentation online gets wrong for this image: models land in /root/.rembg, not /root/.u2net, and mounting the old path silently re-downloads 168MB on every start.
The output was verified rather than assumed — PNG, four channels, alpha spanning 0 to 255, dimensions preserved, 1.7x the input JPEG's bytes.
Marked as evaluated rather than adopted, since the engine choice is still open, and the three things not established are listed: quality on a real photograph, behaviour on the NAS, and concurrency. The test input was a generated rectangle on a flat ground, which says nothing about a chipped vase on a patterned rug.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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>
The admin version stamp has reported commit "unknown" everywhere. #233 read it out of .git during the build and #235 removed that, because Portainer's build context has no repository history and the COPY failed every deploy — the version stamp became the thing that stopped deployments. #237 then established that building in Gitea Actions does not help: the Dockerfile no longer copies .git, so where the build runs is irrelevant.
So the builder hands the commit over rather than the build going to look for it. ARG GIT_COMMIT, empty by default, passed through to writeBuildInfo, which prefers it and still falls back to reading .git so a local build stamps itself with no argument needed.
An empty value is treated as absent rather than stamped. `--build-arg GIT_COMMIT=` is what an unset shell variable expands to, and a blank commit reads as one that happens to be empty rather than one nobody supplied.
Nothing regresses for Portainer. It cannot pass the argument, so its images keep saying "unknown" exactly as today, and they still deploy — the property #235 was bought with.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Regenerate set state='queued', cleared attempts and ai_error, and answered 200 — then nothing ran the worker, so the row sat until the five-minute sweeper happened along. From the admin's side that is indistinguishable from a dead button, and the obvious response is to press it again.
The submission path has kicked the worker since #223. Both paths put a row into 'queued'; only one asked for it to be drafted. That was an oversight in #225 rather than a decision: the kick was added to the submission path in a later task and the action routes were never revisited.
Fixed in both places that re-queue — the admin route and the signed regenerate link from the notification email, which sets the identical state. Fire and forget with a logged catch, exactly as the submission path does: a slow or failing model call must not become a failed request for the admin, and the sweeper is still the backstop if the kick misses.
The card still does not update itself once the draft lands, since drafting takes a few seconds and the screen has no way to know it finished. That is a UI question rather than this bug, and is noted on #272.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Every draft in QA failed with a 400: "anthropic-workspace-id is required when authenticating with an identity-linked API key". A key issued against a workspace, rather than standing alone, is refused unless the request names the workspace it acts in — and the client was constructed with an API key and nothing else.
Nothing about a key's shape says which kind it is, so no amount of configuration checking would have caught this. Only a real call would, which is exactly what #223's task 8 existed to make.
Sent only when ANTHROPIC_WORKSPACE_ID is set. Plenty of keys need no workspace, and sending an empty header would turn the ordinary case into a different error rather than leaving it working. Both compose files carry it with an empty default so an unset variable cannot fail a deploy, and the cutover doc goes from fifteen interpolated names to sixteen — checked against the file, and every name in the list now matches one in the compose.
The failure handling needed no change and got none. The submission kept its photos, the draft recorded ai_error, and the review queue showed the reason. A model call failing must never lose somebody's consignment, and it did not.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The measures step reported "Quality gate ERROR" and "Code smells 5", which is enough to notice debt and useless for clearing it. It now also prints the failing gate conditions, the open issues with their rule, file, line and effort, and the security hotspots awaiting review.
#181 is why this matters. It assumed the five smells were the five eslint-plugin-sonarjs warnings, on the strength of the counts matching, and hedged that the server's rule set is not the plugin's. The hedge was right: those five warnings were fixed under #261, both workspaces lint at zero, and the analysis of the #261 merge still reported five smells and 24 minutes. They are a different five, and nothing short of the list settles which.
The gate condition list matters for the same reason. "ERROR" sends a reader to a dashboard, which is the thing this script exists to avoid needing.
Issues are capped at 25 rather than paged: past a couple of dozen the answer is not "read the list", and an unbounded fetch on every CI run is a cost with no reader.
Verified against a stub SonarQube serving canned responses, so the parsing and formatting are exercised rather than reasoned about — failing conditions filtered from passing and NO_VALUE ones, components stripped of their project-key prefix, absent metrics rendered as a dash, and every path still exiting 0. The no-server and unreachable-server paths were exercised too; the step still cannot fail the job.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
start-local.ps1 said "something is already listening on 3000; leaving it alone" and carried on. That is safe only while the database has not changed underneath that process. When it has, the old backend is serving a database it no longer owns and its in-memory state describes rows that no longer exist.
The rate limiter is the sharpest example. It keys on customer id and keeps buckets in memory for an hour, so a recreated database restarting ids at 1 hands a brand-new customer a previous run's spent allowance. That is #257: the resend-verification allowance test failing roughly one full run in three, never in isolation, with three refusal toasts where one was expected.
Reproduced deterministically rather than reasoned about. Fresh database and fresh backend: three of three pass. Recreate the database only, leaving the same backend running: the same test fails with exactly the reported "resolved to 3 elements". Control — recreate the ids again but restart the backend as well: passes. So the variable is the process outliving the database, not the id restart on its own.
That also explains why #257 could not find the mechanism. It had ruled out contention, a mis-keyed limiter, a shared fixture and identity reuse in the test helpers, all correctly. The recycling happens outside the suite entirely, in a process the suite never sees.
Now it refuses, names the reason, and says to run -Stop. A run that stops loudly is recoverable; one that quietly tests the wrong thing is not — and a stale listener on 3000 has already produced two wrong measurements in this project, a rate-limiter reading and an e2e run reported as 23 passed when the backend was talking to a deleted database.
DatabaseIsNew is set when -Fresh removes the container, when the container is created, and always under -E2eDb, whose tmpfs storage means it comes up empty whether created or restarted.
Verified by AST-parsing the script and confirming every reference to the flag is script-scoped — a function-local read would see $null and the guard would never fire. The script is deliberately never executed from an agent shell.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The e2e suite ran against the development database and nothing truncated it. Every run seeded more fixtures and left them, so the unfiltered storefront grew monotonically — 1,662 items by the time #186 was filed — until rendering it outran the assertions' timeouts. It failed locally, passed in CI where the database is fresh, and got steadily worse, which is the combination nobody can act on.
start-local.ps1 -E2eDb runs the stack against a separate container on a separate port with tmpfs storage, so it starts empty every time. Migrations already run on every start, so an empty volume is a working one. The development database is untouched, so anything set up there by hand survives.
Deliberately a third database rather than sharing either existing one. The integration suite truncates between tests, so an e2e run sharing with it would have its fixtures deleted underneath it (#116) — different container, different port, different credentials, so the mistake is impossible rather than discouraged.
Verified by recreating the database from the compose file, confirming it came up with zero items, and running the full suite against it: 157 of 157. An earlier attempt at this reported 23 passed and 53 not run, which was worthless — a stale backend from a previous run still held port 3000 and was talking to a database I had already deleted. The port is checked before the run now, and the same mistake produced a wrong rate-limiter measurement earlier in this work.
Pagination is the other half of #186 and is filed separately: a shop that renders its whole catalogue in one page is worth fixing on its own merits, not as a side effect of a test fix.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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>
node-pg-migrate keeps the schema; Drizzle is for queries only. The conventions doc said this was unsettled and now says what was settled and why: drizzle-kit cannot diff expression indexes and emitted six statements for one column, our migrations are mostly prose that generated SQL does not carry, and data migrations cannot be generated at all.
The reasoning in full is on the issue. This is the version a reader converting a query will actually find.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Columns in a sql template render unqualified, so a correlated subquery correlates with itself and returns a plausible wrong number rather than failing. That is worse than the array trap already recorded here, which at least produces invalid SQL — this produces valid SQL and quietly wrong data, and only an integration test asserting a value caught it.
And a driver error code moves when Drizzle wraps it, so a catch keyed on a SQLSTATE still compiles and silently stops matching.
Also records that the camelCase mirror and the snake_case API mean every select must map columns explicitly, because selecting the table changes the JSON contract with nothing to notice.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
All nine sites, chosen because the file is awkward rather than easy: a recursive CTE consumed two ways, a correlated subquery, an array match, and two error paths keyed on a Postgres SQLSTATE. A file of plain CRUD would have produced a flattering number that does not generalise.
The hand-declared row interfaces are gone. CATEGORY_COLUMNS is written once and the row type is inferred from it, which closes the drift itemSelect.ts documents as "KEPT IN STEP BY HAND". That mapping has to be explicit rather than selecting the table: the mirror names columns in camelCase and this API answers in snake_case, so selecting the table directly would have silently changed the JSON contract the admin frontend reads, and no test asserting status codes would have caught it.
strict and noUncheckedIndexedAccess hold with no non-null assertions added. requireRow covers the RETURNING rows and the existing lookup destructures and branches, exactly as before.
Two bugs were introduced and caught by the integration suite, and both are worth recording because neither produced a type error.
Drizzle renders a column reference inside a `sql` template UNQUALIFIED. `${items.categoryId} = ${categories.id}` became `WHERE "category_id" = "id"`, which Postgres resolved against items on both sides — so the item count came back plausible and wrong rather than failing. That is worse than the documented array trap, which at least produces invalid SQL. The fragment is now literal text, which is honest since it binds no values.
And the driver's error code moved. Drizzle wraps errors, so the SQLSTATE that sat on err.code now sits on err.cause.code; the old check compiled, never matched, and turned two 409s into 500s. isUniqueViolation accepts both shapes.
inArray replaced the ANY(...::int[]) match and sidesteps the sql.param trap entirely — there is no template to forget it in, and the builder emits the placeholder list correctly by construction.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
backend/drizzle/ was `drizzle-kit`'s output from the #216 spike, superseded now that pull writes into src/db-drizzle. Removing it also removes something that should never have been on main.
#216's closing comment said the spike branch carried "an experimental condition_note column that must not reach main". Checking backend/migrations for it found nothing, which is where I stopped looking last time — but it was here, as backend/drizzle/0001_add_condition_note.sql. It reached main in the same merge that brought the Tinqer probe #261 removed.
Nothing ran it. node-pg-migrate only executes backend/migrations, so this SQL was inert and no database has the column. It was a loaded gun rather than a fired one, which is the only reason this is a cleanup rather than an incident.
That file is also the evidence #219 needs, so it is quoted in that issue before being deleted: adding one nullable column emitted three DROP INDEX statements and three CREATE UNIQUE INDEX statements alongside it, for the expression indexes drizzle-kit could not diff. On a large table those recreations take real locks, and a generated migration nobody read would have taken them.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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>
The alias check was a negative match on an allowlist of English error strings, which returned True for empty output, for $null, and for "exit status 1: Access is denied." — so an alias switch producing nothing, or failing on the symlink permission error this file's own header warns about, was reported as success while the old version kept running. That is the bug #198 was filed about, narrowed rather than removed, and it also broke whenever nvm reworded an error. It is now a positive match on "Now using node v<what is actually running>".
The floor check moves ahead of the switch and reads the constant rather than the result. Where it sat, $major was always whatever NODE_VERSION says, so it validated the switch it had just made instead of the pin it exists to guard, and could never fire.
Use-NodeLatest is now Use-PinnedNode. In a change whose whole subject is that "latest" means something people do not expect, the name was an avoidable trap.
The restore default moves beside NODE_VERSION. It deliberately is not a param default: a param block runs before the dot-source, so $script:DEFAULT_NODE_VERSION is still $null there and the restore would have quietly restored nothing — leaving the machine on the pinned version, which is the exact failure the restore exists to prevent. It is resolved after the dot-source instead, and an explicit -DefaultNodeVersion still wins.
Three documents described behaviour the code no longer has: README's "both scripts run nvm use latest", run-tests.ps1's .DESCRIPTION, and project-context.md's instruction to agents. All corrected, and project-context.md now also says not to run these scripts from an agent shell, which is how this machine once ended up with no Node at all.
Part 4 of the issue is partly stale: backend/package.json already declares engines >=20.9.0. frontend now matches it. The larger question — whether local should be pinned to the Node 20 that CI and the production image actually run — is a decision rather than an oversight and is left open on the issue.
Verified by parsing all three scripts with the PowerShell AST parser, which does not execute them. They are deliberately never run from an agent shell.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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>
SonarQube Community has no branch analysis. Every scan published under a project key replaces that project's single analysis, whatever revision it came from — so each pull request, and each push to one, overwrote the dashboard's analysis of main with the branch. The new-code period, the gate result, the coverage percentages and the hotspot list then all described whatever was scanned last, with nothing on the dashboard saying which revision that was. A gate that went green on a feature branch read exactly like a gate that went green on main.
It was caught only by luck: #180's hotspots reported line numbers that landed on a comment and a blank line in main, which is the kind of nonsense a person notices. Everything else it misreported would have looked fine.
scripts/scan-local.sh has always refused to do this, defaulting to a scratch key, and its header says why in as many words. CI walked into the hazard that script guards against. Now the two tell the same story.
The suites still run on pull requests, which is where their value is — only publishing is restricted. The measures report is skipped alongside the scan, because with nothing published it would print main's numbers into a pull request's log, which is noise at best and misread as the branch's own at worst.
A test asserts both steps carry the restriction and that the three suites do not, because the failure leaves no trace and the `if:` is one line for somebody to drop.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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>
The count comes from item_drafts rather than a tally. Every submission creates exactly one row in the same transaction that creates the item, so the rows are the truth — a separate counter would be a second thing that can disagree with them, and the one that disagrees silently is always the counter.
windowStart is the part worth testing on its own and the part most likely to be quietly wrong. A reset older than the window must not widen it, which would make the ceiling stricter over time rather than rolling; a reset in the future must not disable it; and a malformed value must not produce an Invalid Date, which compares false against everything and would silently switch off the limit it was set to impose. Each is a test.
Alerts go through sendMail directly rather than the editable templates. An abuse alert is not copy anyone will want to reword, and making it editable means it can be broken — a required placeholder removed from an alert nobody reads until an incident is a poor way to discover the validation.
The throttle is in memory, so a restart during an incident can send one extra alert. That beats writing to admin_settings from the request path on every refused submission; a replicated deployment would have to move it.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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>
Six tasks: the settings, stopping them leaking between test suites, the count, the alerts, the refusal, and the manual reset.
The count is derived from item_drafts rather than a tally, as the issue asks. That forces a decision it left open: a reset cannot delete anything, because the rows are real submissions whose items are sitting in the review queue. So a reset stores a timestamp and the window becomes the later of that and 24 hours ago — one derived count, no second tally, and a reset that is an auditable fact rather than a deletion.
The refusal 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 throw them away, which is the expensive half of the work the ceiling exists to prevent.
The alert throttle is in memory rather than in the database, so a restart during an incident can send one extra alert. That is a better trade than writing to admin_settings from the request path on every refused submission, and it is noted that a replicated deployment would have to move it.
Self-review caught two things against the tree. POST /api/admin/items answers 200 rather than 201, so that assertion was wrong. And resetDb deliberately does not truncate admin_settings — it deletes only the email_ rows — so a ceiling of 1 left behind would make every later suite's submissions refuse with a 503, in files that never mention a ceiling. The comment in that file records the same failure happening once already with an email template. Task 1b widens the cleanup.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A gap in the plan rather than in the code: nothing wired the secret into either stack, so the feature would have shipped with its signed links permanently disabled and nothing saying why. Both compose files now interpolate it, with `:-` so an unset variable stays empty rather than failing the deploy.
QA takes QA_INTAKE_ACTION_SECRET, its own value rather than production's, for the same reason as every other QA_ prefixed credential — and more sharply here, because a link signed with it acts on a draft without a login.
Rotating the secret revokes every outstanding link, which is the intended answer to one leaking.
The cutover doc counted fourteen interpolated names and now counts fifteen. That document says it is checked against the file rather than from memory, so it was: fifteen in the compose file, the same fifteen listed.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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>
Sent after the draft commits, fire and forget. A mail failure must never mark a draft that was written correctly as failed: the review queue is what the admin actually works from, and the email is a convenience on top of it.
Every quiet path returns rather than throws. No recipient configured is not an error — nobody has said where to send it and the draft is waiting regardless. No INTAKE_ACTION_SECRET means the two shortcut links render empty rather than broken, because a link that could not be verified is worse than none. An item with no draft row simply returns.
A missing description is said plainly rather than left blank. An empty paragraph in a notification reads as a bug; "no description was drafted for this item" reads as the fact that it is, and tells the admin what to expect on the screen.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Editable from the settings screen like every other template. reviewUrl is the only required placeholder: a notification with no link in it still sends, still looks fine in the log, and is useless to whoever receives it, which is what the required-placeholder validation exists to catch.
The two signed links are deliberately optional. They are absent whenever INTAKE_ACTION_SECRET is unset, and a template demanding them would leave an unconfigured environment unable to send this at all.
A test asserts the template offers no way to publish. That the email cannot publish is what bounds the risk taken by pricing items on arrival, and it is a property of the copy as much as of the routes — a publish link in the body would be one nobody reviewed.
Where the notification goes is an admin setting rather than an environment variable, for the same reason drafting_model is one: it is changed by whoever runs the shop, not by whoever deploys it. Empty is the default and means do not notify, which is a working configuration.
INTAKE_ACTION_SECRET warns rather than fails at boot, like the drafting key. Being told an item arrived matters far more than being able to discard it in one click.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Signed over the item, the action and the expiry together. Signing any subset would let a link be replayed against a different item or upgraded to a different action, and leaving the expiry out of the payload would let anyone holding an expired link extend it by editing the timestamp in the URL. Each of those is a test.
Compared through a second digest rather than directly, because timingSafeEqual throws when the buffers differ in length, and a truncated link is an ordinary thing to receive rather than an exception. Same idiom as the admin gate.
actionUrl returns null rather than throwing when there is no secret or no PUBLIC_URL. An unconfigured environment still sends the notification with its review link — being told an item arrived matters far more than the shortcuts do — and a link that could not be verified must never be offered in the first place.
Uses the shared trimTrailingSlashes rather than a trailing-slash regex, which is what utils.ts exports it for.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Five tasks: a pure HMAC signer, the editable template and its recipient setting, the send from the drafting worker, the public routes that act on a signed link, and the quiet paths.
The plan makes one decision the issue does not, and it changes the shape of the feature. Mail scanners and 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 — with a valid signature, looking entirely legitimate in the log, and nobody would know to go and recover it. So the signed link is a safe GET that confirms and a POST that acts. It costs one extra click and is cheap to reverse if that is the wrong trade.
Everything about the notification is best-effort. No recipient configured, no INTAKE_ACTION_SECRET, or SMTP down all end in a log line: the review queue is the source of truth, and a draft that was written correctly must never be marked failed because an email did not send.
The email still cannot publish. The two signable actions are exactly the ones whose worst case is a wasted API call or a hide the queue can undo, which is what makes putting them in an inbox acceptable at all.
Branched from feature/225-review-queue rather than main, because the review link has nowhere to land without it.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Seeded through the intake route rather than POST /api/admin/items, which writes no item_drafts row — an item created that way would never appear in a queue that joins that table. Going through the link is also the path a real submission takes, so the test exercises what actually ships.
Two cases. The first edits the price and publishes, which confirms the number, so no dialog appears and the item reaches the storefront at what was typed. The second publishes an untouched price and asserts the confirmation names the figure and the fact that nobody chose it, then cancels and checks the item is still unconfirmed. That second case is the protection this whole screen exists to provide.
Scoped to the card each test creates, located by the sender's note carrying the run id — the item's own name is a submission timestamp and not unique to the test. The dialog is matched on its accessible name because antd nests the confirm title in two elements and getByText resolves to both.
Full suite 157 of 157, up from 155.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The screen that makes the intake pipeline usable. Until now a draft existed only in item_drafts and nothing rendered it, so a successful draft and a failed one looked identical from the admin — the item shows its placeholder submission-timestamp name either way, and telling them apart needed SQL.
The price carries the weight the schema no longer does. It is labelled with where the number came from, anything not set by a person is marked unconfirmed, and publishing at an unconfirmed price asks first rather than reporting afterwards. Editing the field is what confirms it, so opening the card and leaving the price alone is not recorded as approval — the same rule the server applies, which this only has to agree with.
Discard is offered rather than delete, and a discarded card offers Restore in its place.
The e2e page object's AdminTab union is extended alongside the tab itself. It is a closed union, so admin.open('Review queue') would not type-check without it — and the tab strip and that union have to be changed together or the next spec to use it fails to compile.
Both frontend lint and build clean, still at zero warnings.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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>
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>
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>
Pure and separately tested because the failure it guards is silent. Items are priced on arrival, so the schema no longer stops a number nobody chose reaching the storefront — the review queue does, by showing that nobody chose it, and an item selling at a default price looks exactly like one selling at a chosen price.
Editing the number is the only thing that confirms it. Publishing an untouched field deliberately does not, because that would record "I did not look at this" as "I approved this".
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Six tasks: the price provenance rule as a pure unit, the list endpoint, publish, the three state actions, the screen, and an end-to-end pass.
The price field is the reason this screen exists. Items are priced on arrival, so the schema no longer prevents a number nobody chose from reaching the storefront — that protection moves here, into presentation, where it is weaker. So the rule is a pure tested function rather than a line inside a route: editing the number is the only thing that confirms it, publishing an untouched field deliberately does not, and publishing something still unconfirmed asks first rather than reporting afterwards. 80.00 was chosen because it reads as a decision rather than as an obvious sentinel, which is exactly why it has to be called out rather than left to be noticed.
Discard deletes nothing. It is one click from an inbox, and the photos are often the only copy of an item no longer in the sender's hands, so it marks the draft and returns the item to pending. Restore brings it back at the state its own contents justify rather than unconditionally ready, because a submission discarded before it was ever drafted has no copy and must not return claiming otherwise.
Regenerate clears attempts along with the state. The worker only picks up rows below the attempt cap, so re-queueing a draft that already failed three times would otherwise produce a button that appears to work, does nothing, and says nothing.
The end-to-end test seeds through the intake route rather than POST /api/admin/items, which writes no item_drafts row and would never appear in a queue that joins it.
One deviation from the issue is recorded in the plan rather than buried: it asks for the existing admin item components to be reused, and this builds a purpose-made card instead, because the fields differ in kind rather than arrangement. The cost — two places rendering a name, description and price — is named there too.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The standing cleanup, three features behind. Four changes.
Report the measures in CI. This is the one that matters, because the rest was only findable by reading the tree. SonarQube here is 9.9 Community: no Bearer auth, so the official MCP cannot connect, and the host is a CI secret, so hotspots, duplication, debt and coverage existed only on a dashboard — which made "reduce the debt" an instruction nobody could act on without a browser open beside them. scripts/summarize-sonar.js queries the measures API with the secrets the workflow already holds and prints the result into the job log. The scanner masks the URL and token; measures are not secret.
It polls the compute task before reading. The workflow does not set sonar.qualitygate.wait, so the scan step returns once the report is uploaded and the server computes measures afterwards — reading immediately would return the previous analysis, indistinguishable from this one and quietly wrong. When it cannot confirm, it says so in the output rather than presenting stale numbers as current. It is deliberately not guarded with continue-on-error: it exits 0 on every path, and guarding it would oblige it to appear in the final gate, whose job is to fail the build.
Remove the Tinqer spike. #216 evaluated Drizzle against Tinqer and rejected Tinqer, and its closing comment said the throwaway src/db-tinqer/ probe must not reach main. The whole spike commit was merged, so it did. The probe is 71 lines imported by nothing, and @tinqerjs/tinqer, @tinqerjs/pg-promise-adapter and pg-promise were dependencies for a library nobody chose. The condition_note column that warning also named did not reach main.
Clear the lint debt, both projects now at zero warnings from six and two. One of these was a real defect rather than tidiness: the third catch block in shippingAddresses.ts rolled back and returned 500 while discarding the error, so a failed default-address change left nothing behind to say why — the two catch blocks above it in the same file already logged, and this one had simply been missed. The Express namespace augmentation is a false positive and is disabled with the reason written beside it, because an interface that must merge into one Express declares inside a namespace has no ES module spelling.
Dedupe the extension map. backfillImageReencode.ts kept its own .jpg/.png/.webp table whose comment named uploadTypes.ts as the source of truth, directly above duplicating it. That file rewrites stored images, so the two disagreeing would silently skip files it should re-encode.
src/db-drizzle/ deliberately stays. #217 is open to promote exactly those files properly, with tablesFilter and the sql.param() array rule; deleting them here would be doing #217 badly in the wrong issue. Only their unused-symbol warnings are fixed, and if drizzle-kit pull regenerates schema.ts the table warning returns — worth #217 knowing.
Hotspots and coverage are untouched because both numbers are still invisible. They are the next pass, once the step above has printed them once.
Closes#261
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
QA_SMTP_HOST, QA_SMTP_PORT and QA_SMTP_SECURE were set on the stack but nothing read them: the compose file hardcoded all three, so changing one in Portainer had no effect and gave no sign of that. They now interpolate like the credentials beside them.
Each keeps its Brevo value as a default rather than being left to fall through. The mailer's own fallbacks are Gmail's — smtp.gmail.com, 465, implicit TLS — and Brevo is STARTTLS on 587, so an unset variable with no default here would quietly aim QA at Gmail and fail at send time rather than at boot. `:-` supplies the default only when the variable is unset or empty, so setting one still wins.
Verified with `docker compose config` both ways: unset resolves to smtp-relay.brevo.com/587/false, and set resolves to the supplied values. The #107 compose guard still passes.
Production is deliberately untouched. It hardcodes the same three and nobody has asked to vary them there, and a needless change to the production stack is not worth the deploy.
Closes#258
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Playwright matches an accessible name case-insensitively and as a substring unless exact is passed, so `getByRole('button', { name: 'OK' })` matched any button whose name merely contained "ok".
Caught on a full serial run of main:
strict mode violation: getByRole('button', { name: 'OK' }) resolved to 2 elements:
1) aka getByRole('button', { name: 'Freed rmtiq9okg22k9e' })
2) aka getByRole('button', { name: 'OK', exact: true })
A leftover "Freed …" toast was still on screen and the random base36 suffix happened to contain "ok". Roughly one suffix in a few hundred does, which is the profile of a test that fails occasionally and reproduces for nobody.
This is not the contention #241 addressed. It reproduced with workers: 1, serially, on a fresh database — it needs only a stale toast and an unlucky suffix. It is at least part of what #245 skipped a real test to work around.
Also makes the taxonomy modal's OK and the sold-filter 'All' radio exact, the only other targets short enough to appear inside a random suffix. The dozen or so remaining short names are left alone deliberately: they would need a collision in deliberately-chosen test data rather than a random one, and substring matching is load-bearing in some of them.
Verified with two consecutive full e2e passes, 155 of 155 both times.
Closes#253
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
main did not build, so every Portainer deploy failed with `npm run build` exit code 2. My regression from #241.
findOrFail<T>(items: T[], predicate: (item: T) => boolean) infers T from both parameters. The call sites annotate the predicate as documentation, and the arrays come from .json(), which is any and offers no competing candidate — so T became the one-field shape written in the lambda and every caller failed on the field it actually wanted. Array.prototype.find has no such problem, which is why the code this replaced type-checked.
The four responses are now typed at their call sites, so T is inferred from real data and the predicates need no annotation. findOrFail additionally takes NoInfer<T> on its predicate, so a stray annotation can never drive the element type again. The specs are better typed than before this change: `.json()` was plain any, and the annotations only ever documented a shape nothing enforced.
I did not catch this because I verified with a bare `npx tsc --noEmit`, and tsconfig.json is `"include": ["src"]` — it structurally cannot see tests/. The specs are checked by the second command in `npm run build`, which is the step Docker runs and the step that failed. Verified this time with `npm run build` itself, plus lint, the unit suite, and two consecutive full e2e passes.
Closes#254
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
bermudalambandClaude Opus 5