Two POST routes and the module behind them. They live on the item rather than on the draft, and that is the decision that makes the inventory editor free when it follows: an image belongs to an item whether or not a draft row exists, so the second screen to want this is the same call from a different place with no new backend at all.
A cut-out and its pristine original turn together. An image that has been through #281 has two files, and rotating only the displayed one would leave them disagreeing — Restore original would then quietly un-rotate the photo, so the undo of one feature becomes a regression of another.
204 rather than 200. Rotation changes no column: the paths are identical afterwards and only the bytes differ, so there is no row worth returning, which is the same reason deleting an image is already a 204.
Only "not on this item" is a 404, and it is indistinguishable from an absent id on purpose, because an image id is a serial and this endpoint should not confirm which ones exist. Everything else stays loud as a 500, and the file is untouched in every one of those cases — rotateInPlace renames over the original only once the new file has been written.
One asymmetry is recorded rather than engineered around: rotation is not idempotent the way background removal is, so a retry after a failure between the two files turns the displayed one twice. That needs the disk to break between two writes, and the remedy is one press in the other direction.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The file half of the remedy for what #300 could only stop. Fixing the EXIF strip means new uploads arrive the way the sender saw them; it cannot repair what is already stored, because the tag that said which way up the pixels went is gone. Those photos need a person to look at each one and turn it.
Rewrites the pixels rather than recording an angle, because an angle obliges every consumer to honour it — the storefront, both admin screens, the drafting worker's photo reader, and the rembg sidecar — and any one that forgets shows the photo sideways. The sidecar is not ours to teach.
Left is anticlockwise and right is clockwise, which is rotate(-90) and rotate(90); sharp reads a positive angle as clockwise. The direction test asserts a pixel rather than a dimension, because dimensions swap whichever way the turn goes — a reversed sign would pass every size assertion and ship a control that does the opposite of its label.
The animated WebP case is the one that could destroy someone's file quietly. Reading such a file without the animated flag succeeds and hands back the first frame alone, so a rotation that omitted it would write a still back over the animation and report success. Passing the same flag reencodeInPlace passes makes sharp refuse instead — multi-page images turn only by 180° — which is the honest answer and leaves the file untouched.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Photos arrived in the review queue rotated, in an orientation the sender never saw, and we were doing it to them.
A camera does not turn its sensor data round. It writes the pixels as the sensor read them and sets an EXIF Orientation tag saying which way up they go, and every viewer honours that — which is why a portrait photograph looks upright to the person who took it and to the person who attached it. The re-encode from #226 rebuilds the file from decoded pixels and drops all metadata, which is right and is the whole point: a product photo should not publish the coordinates it was taken at. But it never applied the orientation first, so the sideways pixels survived and the one piece of information that explained them did not.
The fix is sharp's rotate() with no argument, which reads the tag rather than turning the image by a fixed amount, placed before resize. The order matters: resize bounds width and height, and for a portrait photo those are the wrong way round until the rotation has happened, so a 3000x4000 photograph stored as 4000x3000 would otherwise be bounded on the wrong axis.
Two tests, one of which is a fixture lesson. The fixture is a 400x200 image tagged Orientation 6 — the shape a portrait photo actually has on disk — and the assertion is that it comes back 200x400. The first version built it with withExif({ IFD0: { Orientation: '6' } }), which sharp reads back as orientation 1: a fixture carrying no orientation at all, which would have passed against the unfixed code and proved nothing. It uses withMetadata({ orientation: 6 }) instead, and the comment says why so the next person does not repeat it. Confirmed by removing rotate() and watching the test fail.
The second test pins that the tag itself still goes, so nothing downstream rotates the image a second time.
This does not repair the photos already uploaded. Their EXIF is gone, so nothing records which way up they were meant to be, and the originals kept for #281's cut-outs were themselves re-encoded on the way in. Those need a person and a rotate button, which is #301.
Closes#300
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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>
Two routes on the item, and a small admin config route so the inventory screen can know whether to offer them.
Both answer 200 once the id is valid, even when the sidecar fails, and that is a deliberate departure from the per-photo endpoints in #281. Those act on one image, so the request either worked or it did not and 502 says which. These act on several, so "did it work" has no single answer — two of four is the normal shape of a bad day here, not an exception — and a 502 would throw away the count that is the only thing making the outcome actionable. Non-200 is reserved for not being able to try at all, which here means an unreadable or absent id.
No status check on either. A sold item's photos are still the shop's photos and improving them changes nothing about the sale; the guards on unpublish protect a checkout in progress and a completed sale, neither of which is at stake in a photograph's background.
The config route follows adminVersion's precedent rather than extending the public /api/config: admin-only, one purpose, and the reason written down. The inventory screen had no other way to learn the feature exists, because GET /api/admin/items answers a bare array with several consumers and reshaping it for one boolean is the worse trade.
Also extends the admin item select to carry original_image_path on each image, behind a new ADMIN_IMAGES_SUBQUERY kept separate from the shared IMAGES_SUBQUERY the public select uses. Task 3 needs to derive its restore-button label from that field, and the server never sent it for items before this — only the drafts endpoint carried it, added by #281 for the review queue. It stays admin-only for the same reason itemSelect.ts already names PUBLIC_ITEM_SELECT's columns explicitly: an internal original filename is nobody's business on the storefront, and sharing one subquery would put it in every public item response.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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 revoke route's RETURNING clause omitted contact_email while its result was typed as UploadLinkRow, which declares the field as present. No user-visible effect since the response was never checked for it, but the type asserted something the query did not actually return. Added the column so the two agree.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
Two drivers. The call after a successful submission means a draft is usually waiting by the time anybody looks; the five-minute sweep means a restart mid-draft is recoverable rather than a permanently stalled row, and picks up whatever the first call missed.
Neither is awaited. A slow or failing model must not become a failed upload for someone who did nothing wrong, which is the whole reason drafting does not happen inline — the cost of a dropped call is a few minutes' delay, not a lost submission.
Both catch for themselves. The comment beside the existing schedulers points out that `void` is only safe because those functions handle their own errors, and draftQueued does not: its first query can reject, and an escaping rejection would take the container down.
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>
