restoreOriginalsForItem rethrew anything that was not NoOriginalToRestoreError, which handed asyncRoute a bare 500 and discarded how far the restore had already got. That breaks the invariant the feature is built on: photos restored before the failure really are back, and an admin standing in front of the modal needs the count to decide whether pressing the button again is worth anything. RestoreSummary now carries `failed` and the loop stops and reports, exactly the shape and the reasoning removeBackgroundsForItem already had.
The restore-originals route gains the missing 404 for an item that does not exist — remove-backgrounds always had it, and the two handlers are copy-paste rather than a shared helper, so nothing would have caught them diverging. draftingWorker's .catch is now only reachable if the image-listing query itself throws, since removeBackgroundsForItem no longer rejects over a single photo; its comment says so rather than describing behaviour that has moved.
The `failed` branch is covered by a unit test that stubs the database module in its own module registry. It cannot honestly be an integration test: the only failure the function can report is a database fault, and the only way to inject one into a real run is to interfere with the single pool every integration suite in the --runInBand process shares and that afterAll calls pool.end() on. Two tests that did exactly that are removed here — they left the suite reporting a failure against its own afterAll and leaking a handle that stopped it exiting. Nor is the fault reachable through data alone: the swap's WHERE original_image_path IS NOT NULL guarantees the value it writes into the NOT NULL image_path, and item_images carries no unique, check or foreign-key constraint on either column, so no row can be seeded that makes the statement fail.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
removeBackgroundsForItem answered void and threw on the first failure, which is enough for the drafting worker — it catches and logs, and a draft is not worth failing over — and not enough for an admin standing in front of a screen who needs to know whether the thing they pressed happened. It now returns a summary: how many photos the item has, how many carry a cut-out, and whether it stopped early.
It still stops at the first failure. Six attempts against a sidecar that is not answering helps nobody, and stopping costs nothing because removeImageBackground skips a photo that already has an original recorded, so a retry resumes rather than starting over. The count is what turns that retry into an informed choice instead of a guess.
Adds restoreOriginalsForItem alongside it. A photo that was never cut out is skipped rather than refused, because the mixed state a partial removal leaves behind is exactly when somebody reaches for this.
The one existing caller does not change: the worker ignores the result, and ignoring a returned value is legal, which is what makes this additive rather than breaking.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The quality gate was red on one condition only — new_security_hotspots_reviewed at 75 against a threshold of 100 — and the outstanding hotspot was admin.ts's `${ADMIN_ITEM_SELECT} WHERE i.id = $1`.
Worth being exact about what was wrong with it, because it was not what it looked like. The value was already parameterized: itemId was bound as $1, travelled through the driver's separate parameter channel, and never entered the query text. What was interpolated was a module constant containing no caller data. S2077 fires on the template literal rather than on the value, because the rule cannot tell a constant from a request field — and neither, at a glance, can a person reading it.
So the fix is not to parameterize something already parameterized. It is to stop interpolating at query call sites at all, which turns a property somebody has to verify into one they can see. Every query whose shape is fixed is now a named constant and every such call passes an identifier: ADMIN_ITEM_BY_ID for the two admin routes, PUBLIC_ITEM_BY_ID, LINK_LIST, and the two draft-queue shapes. Seven interpolating call sites become three.
The three that remain cannot become constants and now say so rather than looking like ones nobody got to. admin.ts and items.ts build their WHERE at run time from buildItemFilterSql, whose fragments are string literals whose only interpolations are placeholder indices; that reasoning was already written down and is unchanged. draftingWorker interpolates a table name, and this is the one query here that genuinely cannot be parameterized in any form — a bound parameter is a value, and Postgres will not accept an identifier as one, so the choice is interpolation or nothing. What makes it safe is the closed 'categories' | 'tags' union, and the comment now says that instead of merely asserting there is nothing to worry about.
Also clears the project's only open Sonar issue, S1854 on adminUploadLinks, which #260 introduced and which I had deferred as a tidiness point. It was more than that: outcome was initialised at its declaration and assigned the same value again in the catch, which made two different failures look like one. A template that will not render, or a stored template that cannot be loaded, is not an SMTP problem, and reporting it as "not configured" sent the admin looking in the wrong place. The SMTP-rejection conflation that was actually agreed stays, and is now the only thing that catch conflates.
Closes#294
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>
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>
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>
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 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>
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>
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>
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>
The governing rule is that a submission is the only irreplaceable thing in this pipeline. The photos are often the only copy of an item no longer in the sender's hands, so a missing key, an unreadable file, a failed call and three exhausted retries all end the same way: the item keeps its photos, stays pending, and waits. Nothing in this file deletes anything.
An absent key returns early and spends no attempt. Counting it as a failure would mean a fortnight without a key exhausted the retries and marked every waiting submission failed, with nothing wrong with any of them.
A failure leaves the row queued while tries remain, so the sweeper picks it up again, and failed once they are spent, so a dead submission stops costing money and waits for a person instead of retrying forever.
Photos are read once and passed down rather than loaded again inside the drafting call — the first read already has to happen to check there is at least one.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The copy goes on item_drafts, never on the item. The item keeps its placeholder name and description until a person approves them in the review queue (#225) — nothing a model wrote reaches the catalogue unreviewed.
The price is the deliberate exception, because #220 chose to price an item on arrival rather than leave it unpriced. price_source records that the number came from a model rather than a person, so the review queue can show it as unconfirmed. With no suggestion the item keeps the migration's 8000 default and price_source stays 'default'; the queue shows both the same way, as a number nobody has chosen yet.
A category is checked against the real table before it is stored. The schema constrains the shape of the answer but cannot enforce membership, and a category the shop does not have would be invisible to every storefront filter — a draft nobody could find, rather than an obvious error.
A successful retry clears ai_error, or a draft that eventually worked would still read as broken in the queue.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The client is a parameter rather than a module import, so every test passes a stub. A test that reaches the real API is a defect in the test: this runs on a route a stranger with a link can trigger, and each call costs money.
getAnthropicClient returns null rather than throwing when there is no key. An unconfigured environment is a working one, and the worker treats null exactly as it treats a failed call — one path rather than two.
parsed_output is guarded, not asserted. The SDK returns null there when the answer did not satisfy the schema, which is what a model replying in prose looks like; failing cleanly leaves the submission queued for a retry, where asserting would crash the worker mid-loop. Absent usage figures are treated as zero for the same reason: undercounting a cost is survivable, throwing away a draft that actually succeeded is not.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The model was going to be an environment variable, which meant a redeploy to change it. It is now an admin setting, so it can be changed from the Settings page like the cart expiry and the greeting.
A dropdown validated on the server, not a free-text field. The API only rejects an unknown model at the point of use, so a typo would be stored happily and then fail on every submission, surfacing as drafts quietly not appearing rather than as an error anybody could act on. The PUT refuses anything outside the offered set, and getSettings falls back rather than handing on a value that is no longer offered — drafting with the default beats drafting with a model the API will refuse.
One catalogue rather than two lists. The dropdown needs the models, costMicros needs their rates, and the price shown beside a model in Admin has to be the price it is actually billed at, which it cannot be if the two are maintained separately. Rates were confirmed against the pricing page rather than recalled: Sonnet 5 $2/$10, Opus 5 $5/$25, Haiku 4.5 $1/$5 per million tokens. The unknown-model fallback is deliberately the most expensive rate and never zero, because a budget that reads as unspent however much was spent is the one failure a spend guard cannot have.
Adding a third setting type pushed getSettings past the cognitive complexity limit, so the per-type resolution moved out into one small function each — the same shape the definitions block above it already argues for.
The exhaustive assertion in the GET test gained the new field rather than being loosened. It exists to catch a setting silently vanishing from the response, and that is worth more than not having to touch it here.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Pure and separately tested, because this is where the correctness of every draft is decided. Nothing downstream can distinguish an observed detail from an invented one — the description arrives as prose either way — so the instruction is the only place that distinction can be enforced, and the tests assert it is actually present.
On a one-of-a-kind item an invented "1930s hand-thrown stoneware" is not a cosmetic error but a false claim on a public shop, and the shop answers for it rather than the model. Visible damage is called out for the same reason in reverse: a buyer finding a chip on arrival is worse than reading about it beforehand.
Categories and tags are listed rather than described, so the model chooses from what exists instead of inventing a taxonomy the storefront filters know nothing about, and declining is explicitly allowed so a model with no matching option does not pick the closest wrong one.
The note is quoted and labelled as the sender's rather than merged into the instruction: it is untrusted text from an unauthenticated stranger and should read as evidence to weigh, not as something the shop asserts. A whitespace-only note counts as no note.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The SDK validates the model's response against this before any of it reaches the database, so a model that answers in prose or invents a field becomes a caught error rather than a row full of nonsense.
Everything the model may decline to answer is nullable, because it is told to say nothing rather than guess. A null category is a better answer than a wrong one, and resolving it is what the review queue is for. The name and description are not nullable: a draft without them is not a partial success worth storing.
The price is an integer, bounded at both ends. A fractional, negative or absurd figure reaching the review queue is a number somebody has to notice is wrong, and being trustworthy at a glance is that queue's whole job.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
