From 1c265840a9644394eda93feb0a7f1748bc030208 Mon Sep 17 00:00:00 2001 From: Thom Lamb Date: Thu, 3 Sep 2026 11:18:50 -0500 Subject: [PATCH 01/17] docs(intake): plan the background-removal implementation (#281) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .../plans/2026-09-03-background-removal.md | 2128 +++++++++++++++++ 1 file changed, 2128 insertions(+) create mode 100644 docs/superpowers/plans/2026-09-03-background-removal.md diff --git a/docs/superpowers/plans/2026-09-03-background-removal.md b/docs/superpowers/plans/2026-09-03-background-removal.md new file mode 100644 index 0000000..617954a --- /dev/null +++ b/docs/superpowers/plans/2026-09-03-background-removal.md @@ -0,0 +1,2128 @@ +# Background Removal Implementation Plan + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** Let a submitter ask for the background to be removed from their photos, and let an admin apply or undo it per photo in the review queue — without ever destroying the original. + +**Architecture:** A `rembg` sidecar does the work over HTTP; no Python enters the Node image. One shared module (`backgroundRemoval.ts`) performs the swap and the restore, called from two entry points: the drafting worker, honouring an intent the submitter recorded, and two admin endpoints. The original file is kept and its path recorded, so every failure and every poor result is recoverable. + +**Tech Stack:** Express 4 + TypeScript, `pg`, `node-pg-migrate`, Jest + supertest, React + antd (`antd/es/...` deep imports), Playwright. + +**Spec:** `docs/superpowers/specs/2026-09-03-background-removal-design.md` +**Issue:** #281 +**Ops notes (measured numbers, the API, the licensing trap):** `docs/ops/image-background-removal-stack.md` + +## Global Constraints + +- **`model=u2net` is sent on every rembg request, always.** The sidecar's default is `bria-rmbg`, licensed **non-commercial**, and it is reached by simply not naming a model. A test asserts the parameter is present, because nothing in the output would reveal its absence. +- **Nothing deletes anything.** No task unlinks a file or deletes a row. The submitter's photos are often the only copy of an item no longer in their hands. +- **`REMBG_URL` is optional.** Unset means the feature does not exist — no checkbox, no admin control, no worker step — not that the environment is broken. Same rule as `ANTHROPIC_API_KEY`. +- **Background removal never fails a submission and never fails a draft.** Every failure path leaves the row and the file exactly as they were. +- **antd imports are deep and from `es`:** `import Checkbox from 'antd/es/checkbox'`. Never `import { Checkbox } from 'antd'`. +- **Branch:** `feature/281-background-removal` off `main`. Commit subjects end `(#281)`. Do not push — the user pushes. +- **Verify the frontend with `npm run build`, never bare `npx tsc --noEmit`** — the app tsconfig excludes `tests/`, and a green `tsc` once broke a deploy. +- Integration tests need the test database: `cd backend && npm run db:test:up`. Port 55432 is Hyper-V-reserved on this machine; if it fails to bind, set `TEST_PGPORT` rather than editing the compose file. + +--- + +### Task 1: The schema — two columns and the Drizzle mirror + +**Files:** +- Create: `backend/migrations/1787600000000_add-background-removal.js` +- Modify: `backend/src/db-drizzle/schema.ts` (the `itemImages` and `itemDrafts` blocks) +- Test: `backend/tests/integration/backgroundRemoval.integration.test.ts` (create) + +**Interfaces:** +- Consumes: nothing. +- Produces: `item_drafts.remove_background BOOLEAN NOT NULL DEFAULT true`, `item_images.original_image_path TEXT` (nullable), and the test helper `seedSubmission(imagePath?)`. Every later task reads or writes one of these. + +- [ ] **Step 1: Write the failing test** + +Create `backend/tests/integration/backgroundRemoval.integration.test.ts`: + +```typescript +import { pool } from '../../src/db'; +import { resetDb, closeDb } from './setup/testDb'; + +beforeEach(async () => { + await resetDb(); +}); + +afterAll(async () => { + await pool.end(); + await closeDb(); +}); + +/** An item with a draft row and one image, which is what a submission leaves. */ +export async function seedSubmission( + imagePath = '/uploads/photo.jpg' +): Promise<{ itemId: number; imageId: number }> { + const item = await pool.query<{ id: number }>( + `INSERT INTO items (name, status) VALUES ('Submission placeholder', 'pending') RETURNING id` + ); + const itemId = item.rows[0]!.id; + await pool.query(`INSERT INTO item_drafts (item_id) VALUES ($1)`, [itemId]); + const image = await pool.query<{ id: number }>( + `INSERT INTO item_images (item_id, image_path, sort_order) VALUES ($1, $2, 0) RETURNING id`, + [itemId, imagePath] + ); + return { itemId, imageId: image.rows[0]!.id }; +} + +describe('the background-removal columns', () => { + // Default true because the submitter's checkbox is ticked by default, and + // because a row written by any path that does not mention the column should + // behave like the new default rather than needing a backfill. + it('defaults remove_background to true', async () => { + const { itemId } = await seedSubmission(); + + const { rows } = await pool.query<{ remove_background: boolean }>( + `SELECT remove_background FROM item_drafts WHERE item_id = $1`, + [itemId] + ); + expect(rows[0]?.remove_background).toBe(true); + }); + + // Null is also the answer to "can this be restored?", which is why there is + // no separate flag: one fact, one place. + it('leaves original_image_path null until a photo has been cut out', async () => { + const { imageId } = await seedSubmission(); + + const { rows } = await pool.query<{ original_image_path: string | null }>( + `SELECT original_image_path FROM item_images WHERE id = $1`, + [imageId] + ); + expect(rows[0]?.original_image_path).toBeNull(); + }); +}); +``` + +- [ ] **Step 2: Run it to verify it fails** + +```bash +cd backend && npm run db:test:up && npm run test:integration -- backgroundRemoval +``` + +Expected: FAIL with `column "remove_background" does not exist`. + +- [ ] **Step 3: Write the migration** + +Create `backend/migrations/1787600000000_add-background-removal.js`: + +```javascript +exports.up = (pgm) => { + pgm.sql(` + -- The submitter's intent, per submission, because that is how it is + -- expressed: one checkbox above the send button, ticked by default. + -- + -- The worker acts on it rather than the intake route. Removing inline + -- would make the sender 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. + -- + -- NOT NULL DEFAULT true so a row written before this migration, or by any + -- path that does not mention the column, behaves like the new default. + ALTER TABLE item_drafts + ADD COLUMN IF NOT EXISTS remove_background BOOLEAN NOT NULL DEFAULT true; + + -- Where the photo came from, per image, because that is how it is undone. + -- Null until a photo has been cut out, so it is also the answer to "can + -- this be restored?" — one fact in one place rather than a flag that can + -- disagree with a path. + -- + -- Nullable and with no default: an existing image has no original other + -- than itself, and claiming otherwise would offer a Restore that swapped a + -- photo for a copy of itself. + ALTER TABLE item_images + ADD COLUMN IF NOT EXISTS original_image_path TEXT; + `); +}; + +exports.down = (pgm) => { + pgm.sql(` + ALTER TABLE item_drafts DROP COLUMN IF EXISTS remove_background; + ALTER TABLE item_images DROP COLUMN IF EXISTS original_image_path; + `); +}; +``` + +- [ ] **Step 4: Update the Drizzle mirror** + +`src/db-drizzle/schema.ts` is generated by `drizzle-kit pull`, and the guard test `drizzleSchema.integration.test.ts` asserts it declares **every column of every table** — a column added to a table the mirror already knows about is the drift a table-level check waves through. + +If the local dev database is up, re-pull: + +```bash +cd backend && DRIZZLE_DATABASE_URL=postgres://redefined_local:redefined_local@localhost:55500/redefined_local npx drizzle-kit pull +``` + +Otherwise add the two lines by hand, matching the generated style exactly (tab indentation, double-quoted column names). In the `itemImages` block, after `sortOrder`: + +```typescript + originalImagePath: text("original_image_path"), +``` + +In the `itemDrafts` block, after `submitterNote`: + +```typescript + removeBackground: boolean("remove_background").default(true).notNull(), +``` + +`boolean` and `text` are already imported at the top of that file — do not add imports. + +- [ ] **Step 5: Run the tests to verify they pass** + +```bash +cd backend && npm run test:integration -- backgroundRemoval drizzleSchema +``` + +Expected: PASS, both suites. The Drizzle guard is what proves the mirror is not stale. + +- [ ] **Step 6: Commit** + +```bash +git add backend/migrations/1787600000000_add-background-removal.js backend/src/db-drizzle/schema.ts backend/tests/integration/backgroundRemoval.integration.test.ts +git commit -m "feat(intake): record the background-removal intent and the original path (#281)" +``` + +--- + +### Task 2: The rembg client + +**Files:** +- Create: `backend/src/intake/rembgClient.ts` +- Test: `backend/tests/unit/rembgClient.test.ts` (create) +- Modify: `docker-compose.qa.yml`, `docker-compose.prod.yml` + +**Interfaces:** +- Consumes: `trimTrailingSlashes` from `../utils`; `signatureMatches`, `SIGNATURE_BYTES` from `../uploadTypes`. +- Produces: + - `isRembgConfigured(): boolean` + - `removeBackground(bytes: Buffer, mediaType: string): Promise` — resolves with PNG bytes, rejects with an `Error` on every failure. + +- [ ] **Step 1: Write the failing test** + +Create `backend/tests/unit/rembgClient.test.ts`: + +```typescript +import http from 'http'; +import { AddressInfo } from 'net'; +import { isRembgConfigured, removeBackground } from '../../src/intake/rembgClient'; + +/** A real PNG header, so the client's own signature check sees what it expects. */ +const PNG = Buffer.from([0x89, 0x50, 0x4e, 0x47, 0x0d, 0x0a, 0x1a, 0x0a, 0, 0, 0, 0]); +const JPEG = Buffer.from([0xff, 0xd8, 0xff, 0xe0, 0, 0, 0, 0, 0, 0, 0, 0]); + +interface Capture { + url: string; + body: string; +} + +/** + * A stub sidecar on an ephemeral port. + * + * Port 0 rather than a fixed number: several ports in the 55000s are + * Hyper-V-reserved on the development machine and bind with EACCES, and a + * fixed port would also stop this suite running beside itself. + * + * A real HTTP server rather than a mocked `fetch`, because what is being + * checked is the shape of the request that reaches the wire — above all that + * `model=u2net` is in it. + */ +async function withStub( + handler: (req: http.IncomingMessage, res: http.ServerResponse) => void, + run: (capture: Capture) => Promise +): Promise { + const capture: Capture = { url: '', body: '' }; + const server = http.createServer((req, res) => { + capture.url = req.url ?? ''; + const chunks: Buffer[] = []; + req.on('data', (c: Buffer) => chunks.push(c)); + req.on('end', () => { + capture.body = Buffer.concat(chunks).toString('latin1'); + handler(req, res); + }); + }); + + await new Promise((resolve) => server.listen(0, '127.0.0.1', resolve)); + const { port } = server.address() as AddressInfo; + process.env.REMBG_URL = `http://127.0.0.1:${port}`; + + try { + await run(capture); + } finally { + delete process.env.REMBG_URL; + await new Promise((resolve) => server.close(() => resolve())); + } +} + +function respondWithPng(_req: http.IncomingMessage, res: http.ServerResponse): void { + res.writeHead(200, { 'Content-Type': 'image/png' }); + res.end(PNG); +} + +describe('whether the sidecar is configured', () => { + it('is false when REMBG_URL is unset', () => { + delete process.env.REMBG_URL; + expect(isRembgConfigured()).toBe(false); + }); + + // A variable set to spaces is a configuration mistake, not a value — the + // same reading envValidation applies everywhere else. + it('is false when REMBG_URL is blank', () => { + process.env.REMBG_URL = ' '; + expect(isRembgConfigured()).toBe(false); + delete process.env.REMBG_URL; + }); + + it('is true when REMBG_URL is set', () => { + process.env.REMBG_URL = 'http://rembg-syn:7000'; + expect(isRembgConfigured()).toBe(true); + delete process.env.REMBG_URL; + }); +}); + +describe('asking the sidecar to remove a background', () => { + /** + * The single most important assertion in this file. + * + * The image's default model is `bria-rmbg`, licensed non-commercial, and it + * is selected by simply not naming a model. Nothing about the returned image + * would reveal it had been used, so this is the only place it can be caught. + */ + it('names u2net explicitly, because the default is licensed non-commercial', async () => { + await withStub(respondWithPng, async (capture) => { + await removeBackground(JPEG, 'image/jpeg'); + expect(capture.body).toContain('name="model"'); + expect(capture.body).toContain('u2net'); + }); + }); + + it('posts the file to /api/remove and returns the PNG it gets back', async () => { + await withStub(respondWithPng, async (capture) => { + const out = await removeBackground(JPEG, 'image/jpeg'); + expect(capture.url).toBe('/api/remove'); + expect(capture.body).toContain('name="file"'); + expect(out.subarray(0, 8)).toEqual(PNG.subarray(0, 8)); + }); + }); + + it('rejects rather than returning bytes when the sidecar errors', async () => { + await withStub( + (_req, res) => { + res.writeHead(500); + res.end('boom'); + }, + async () => { + await expect(removeBackground(JPEG, 'image/jpeg')).rejects.toThrow(/500/); + } + ); + }); + + // The failure that would otherwise write an HTML error page over a + // photograph. Checked with the same magic-byte helper the upload path uses, + // rather than by trusting the Content-Type the sidecar sent. + it('rejects a response that is not actually a PNG', async () => { + await withStub( + (_req, res) => { + res.writeHead(200, { 'Content-Type': 'image/png' }); + res.end('not an image'); + }, + async () => { + await expect(removeBackground(JPEG, 'image/jpeg')).rejects.toThrow(/not a PNG/); + } + ); + }); + + it('rejects when it is not configured at all', async () => { + delete process.env.REMBG_URL; + await expect(removeBackground(JPEG, 'image/jpeg')).rejects.toThrow(/REMBG_URL/); + }); +}); +``` + +- [ ] **Step 2: Run it to verify it fails** + +```bash +cd backend && npx jest -c jest.unit.config.js tests/unit/rembgClient.test.ts +``` + +Expected: FAIL with `Cannot find module '../../src/intake/rembgClient'`. + +- [ ] **Step 3: Write the implementation** + +Create `backend/src/intake/rembgClient.ts`: + +```typescript +import { trimTrailingSlashes } from '../utils'; +import { SIGNATURE_BYTES, signatureMatches } from '../uploadTypes'; + +/** + * The one place that talks to the background-removal sidecar. + * + * A sidecar rather than in-process inference: the application runs in a + * container, and putting Python and ONNX into the image would add roughly + * 300 MB to one already over a gigabyte. See + * docs/ops/image-background-removal-stack.md for the measurements. + */ + +/** + * NEVER remove this, and never make it configurable. + * + * The sidecar's default model is `bria-rmbg`, and BRIA's RMBG models are + * licensed for NON-COMMERCIAL use. This is a shop. The default is reached by + * simply not naming a model, so it is a licensing problem that happens + * silently and produces a perfectly good image — there is nothing in the + * output that could reveal it. + * + * `u2net` is Apache-2.0, and also ten times faster (1.1–2.3 s against + * 14–20 s) at a sixth the size, so nothing is being traded away for it. + */ +const MODEL = 'u2net'; + +/** + * Generous on purpose. The sidecar takes about 40 seconds to answer after a + * container start and its first call per model downloads 168 MB, so a tight + * timeout would turn an ordinary cold start into a failure. Nobody is waiting + * on this in the worker's path, and an admin who clicked a button would rather + * wait than be told it did not work. + */ +const TIMEOUT_MS = 120_000; + +/** The configured base URL, or null when there is none. */ +function baseUrl(): string | null { + const raw = process.env.REMBG_URL; + if (raw === undefined || raw.trim() === '') return null; + return trimTrailingSlashes(raw.trim()); +} + +/** + * Whether the feature exists in this environment. + * + * Unconfigured is not a failure. It means the submitter sees no checkbox, the + * admin sees no control and the worker skips the step — an unconfigured + * environment must be a working one, which is the same rule + * `getAnthropicClient` follows by returning null rather than throwing. + */ +export function isRembgConfigured(): boolean { + return baseUrl() !== null; +} + +/** + * The cut-out, as PNG bytes. + * + * Rejects on every failure — unconfigured, unreachable, a non-2xx answer, or a + * body that is not actually a PNG. Every caller catches, and none of them lets + * the rejection reach a submission or a draft. + */ +export async function removeBackground(bytes: Buffer, mediaType: string): Promise { + const base = baseUrl(); + if (base === null) { + throw new Error('REMBG_URL is not set'); + } + + const body = new FormData(); + // A copy through Uint8Array because Buffer is not a BlobPart. The filename is + // a constant: the sidecar does not use it, and passing the stored name would + // put a value from the uploads volume into an outbound request for nothing. + body.append('file', new Blob([new Uint8Array(bytes)], { type: mediaType }), 'photo'); + body.append('model', MODEL); + + const res = await fetch(`${base}/api/remove`, { + method: 'POST', + body, + signal: AbortSignal.timeout(TIMEOUT_MS) + }); + + if (!res.ok) { + throw new Error(`rembg answered ${res.status}`); + } + + const out = Buffer.from(await res.arrayBuffer()); + + // The bytes, not the Content-Type header. A proxy error page served as + // image/png would otherwise be written over a photograph — the same reason + // uploads are checked by signature rather than by what the caller declared. + if (!signatureMatches('image/png', out.subarray(0, SIGNATURE_BYTES))) { + throw new Error('rembg did not return a PNG'); + } + + return out; +} +``` + +- [ ] **Step 4: Run the test to verify it passes** + +```bash +cd backend && npx jest -c jest.unit.config.js tests/unit/rembgClient.test.ts +``` + +Expected: PASS, 8 tests. + +- [ ] **Step 5: Wire REMBG_URL into both compose files** + +In `docker-compose.qa.yml`, in the backend service's `environment:` list, immediately after the `ANTHROPIC_WORKSPACE_ID` line: + +```yaml + # Optional. The background-removal sidecar (#281). Unset means the + # feature does not exist: no checkbox on the submission page, no control + # in the review queue, and the worker skips the step. Empty default so an + # unset stack variable cannot fail a deploy. + - REMBG_URL=${QA_REMBG_URL:-} +``` + +And in the header comment block, beside the other optional variables: + +```yaml +# QA_REMBG_URL — optional. The background-removal sidecar, e.g. +# http://rembg-syn:7000. Unset turns the feature off rather than breaking +# anything. The sidecar must be on the same network as this stack. +``` + +In `docker-compose.prod.yml`, after its `ANTHROPIC_WORKSPACE_ID` line: + +```yaml + # Optional. The background-removal sidecar (#281). + # See docs/ops/image-background-removal-stack.md. + - REMBG_URL=${REMBG_URL:-} +``` + +And in its header comment block: + +```yaml +# REMBG_URL Optional. The background-removal sidecar, e.g. +# http://rembg-syn:7000. Unset turns the feature off. +``` + +`REMBG_URL` is deliberately **not** added to `ALWAYS_REQUIRED` in `envValidation.ts` — it is optional, and requiring it would make an environment without a sidecar refuse to boot. The compose guard only enforces `ALWAYS_REQUIRED`, so no change is needed there either. + +- [ ] **Step 6: Run the compose guard, lint and build** + +```bash +cd backend && npx jest -c jest.unit.config.js tests/unit/composeEnvironment.test.ts && npm run lint && npm run build +``` + +Expected: PASS, clean lint, clean build. + +- [ ] **Step 7: Commit** + +```bash +git add backend/src/intake/rembgClient.ts backend/tests/unit/rembgClient.test.ts docker-compose.qa.yml docker-compose.prod.yml +git commit -m "feat(intake): talk to the rembg sidecar, always naming u2net (#281)" +``` + +--- + +### Task 3: The shared swap and restore + +**Files:** +- Create: `backend/src/intake/backgroundRemoval.ts` +- Test: `backend/tests/unit/backgroundRemoval.test.ts` (create), `backend/tests/integration/backgroundRemoval.integration.test.ts` (extend) + +**Interfaces:** +- Consumes: `removeBackground`, `isRembgConfigured` from `./rembgClient`; `typeForExtension` from `../uploadTypes`; `pool` from `../db`. +- Produces: + - `cutoutPathFor(imagePath: string): string` + - `removeImageBackground(imageId: number): Promise` + - `restoreImageOriginal(imageId: number): Promise` + - `removeBackgroundsForItem(itemId: number): Promise` + +- [ ] **Step 1: Write the failing unit test for the pure part** + +Create `backend/tests/unit/backgroundRemoval.test.ts`: + +```typescript +import { cutoutPathFor } from '../../src/intake/backgroundRemoval'; + +describe('where a cut-out is written', () => { + // A new file rather than a rewrite of the original, which is what makes the + // original restorable at all — and what makes the JPEG-to-PNG change free, + // since no existing path is renamed. + it('sits beside the original with a -cutout suffix and a .png extension', () => { + expect(cutoutPathFor('/uploads/abc-123.jpg')).toBe('/uploads/abc-123-cutout.png'); + }); + + // image_path is a contract, not a string: #103 made the stored value the + // path uploadUrl joins an origin onto. + it('keeps the /uploads/ prefix', () => { + expect(cutoutPathFor('/uploads/x.webp')).toBe('/uploads/x-cutout.png'); + }); + + // The extension is replaced rather than appended, so a second pass cannot + // produce `.png.png`. + it('replaces the extension rather than appending to it', () => { + expect(cutoutPathFor('/uploads/x.png')).toBe('/uploads/x-cutout.png'); + }); +}); +``` + +- [ ] **Step 2: Run it to verify it fails** + +```bash +cd backend && npx jest -c jest.unit.config.js tests/unit/backgroundRemoval.test.ts +``` + +Expected: FAIL with `Cannot find module '../../src/intake/backgroundRemoval'`. + +- [ ] **Step 3: Write the implementation** + +Create `backend/src/intake/backgroundRemoval.ts`: + +```typescript +import { promises as fs } from 'fs'; +import path from 'path'; +import { pool } from '../db'; +import { typeForExtension } from '../uploadTypes'; +import { isRembgConfigured, removeBackground } from './rembgClient'; + +/** + * Swapping a photo for a cut-out of itself, and swapping it back. + * + * One module rather than two, because the worker's path and the admin's path + * must produce identical results: a cut-out obtained either way has to be + * undoable the same way. A near-copy that drifted would mean a photo the + * Restore button could not restore. + * + * Nothing here deletes anything. The original file stays on disk and so does + * every cut-out ever made, because the submitter's photos are often the only + * copy of an item no longer in their hands — the same rule Discard follows in + * the review queue. + */ + +interface ImageRow { + image_path: string; + original_image_path: string | null; +} + +/** + * The path a cut-out of `imagePath` is written to. + * + * Pure, so the naming rule can be checked without a database or a sidecar. + * Always `.png` because the result is transparent, and the storefront's dark + * theme would show a flat white background as a bright box behind every + * product. + */ +export function cutoutPathFor(imagePath: string): string { + const base = path.basename(imagePath, path.extname(imagePath)); + return `/uploads/${base}-cutout.png`; +} + +function uploadsDir(): string { + return process.env.UPLOADS_DIR ?? ''; +} + +/** + * Replaces one image with a cut-out, keeping the original. + * + * Idempotent by way of the `original_image_path IS NOT NULL` check rather than + * a separate flag. That guard is load-bearing twice over: it makes a repeat + * call a no-op, and it stops a second pass from recording the *cut-out* as the + * original and losing the real one for good. + * + * Throws on every failure. Nothing is written to the row unless the file is + * already on disk, so a caller that catches and moves on leaves the photo + * exactly as it was. + */ +export async function removeImageBackground(imageId: number): Promise { + const { rows } = await pool.query( + `SELECT image_path, original_image_path FROM item_images WHERE id = $1`, + [imageId] + ); + const row = rows[0]; + if (!row) { + throw new Error(`no image ${imageId}`); + } + if (row.original_image_path !== null) { + // Already cut out. Doing it again would overwrite the record of where the + // real original went. + return; + } + + // basename only: image_path is stored as '/uploads/' and the directory + // it lives in is a server constant. Same rule readPhotos follows in the + // drafting worker. + const sourceName = path.basename(row.image_path); + const mediaType = typeForExtension(path.extname(sourceName)); + if (mediaType === null) { + throw new Error(`cannot read ${sourceName}: unrecognised extension`); + } + + const cutout = await removeBackground( + await fs.readFile(path.join(uploadsDir(), sourceName)), + mediaType + ); + + const cutoutPath = cutoutPathFor(row.image_path); + await fs.writeFile(path.join(uploadsDir(), path.basename(cutoutPath)), cutout); + + // The row is pointed at the new file only after the file exists. The other + // order would leave a window in which the storefront rendered a broken image. + // + // `original_image_path = image_path` reads the pre-update value, which is how + // Postgres evaluates an UPDATE's right-hand side — so this records where the + // photo came from in the same statement that moves it. + await pool.query( + `UPDATE item_images + SET image_path = $2, original_image_path = image_path + WHERE id = $1 AND original_image_path IS NULL`, + [imageId, cutoutPath] + ); +} + +/** + * Puts the original back. + * + * The cut-out file is left on disk deliberately. Removing a background is + * exactly the operation that produces an occasional bad result on an unusual + * object, so somebody restoring one is quite likely to try again — and this + * module deletes nothing in any case. + */ +export async function restoreImageOriginal(imageId: number): Promise { + const { rowCount } = await pool.query( + `UPDATE item_images + SET image_path = original_image_path, original_image_path = NULL + WHERE id = $1 AND original_image_path IS NOT NULL`, + [imageId] + ); + if (rowCount === 0) { + throw new Error(`image ${imageId} has no original to restore`); + } +} + +/** + * Every photo of one item, in order. + * + * Sequential rather than parallel: the sidecar is assumed to handle one + * request at a time, and the worker it runs inside is not in a hurry. A + * failure on one photo stops the rest, and the caller logs it — the item keeps + * whatever was already done, and nothing is left half-written. + */ +export async function removeBackgroundsForItem(itemId: number): Promise { + if (!isRembgConfigured()) return; + + const { rows } = await pool.query<{ id: number }>( + `SELECT id FROM item_images WHERE item_id = $1 ORDER BY sort_order`, + [itemId] + ); + for (const row of rows) { + await removeImageBackground(row.id); + } +} +``` + +- [ ] **Step 4: Run the unit test to verify it passes** + +```bash +cd backend && npx jest -c jest.unit.config.js tests/unit/backgroundRemoval.test.ts +``` + +Expected: PASS, 3 tests. + +- [ ] **Step 5: Write the failing integration tests** + +In `backend/tests/integration/backgroundRemoval.integration.test.ts`, add these imports beside the existing ones: + +```typescript +import http from 'http'; +import { AddressInfo } from 'net'; +import { promises as fsp } from 'fs'; +import os from 'os'; +import path from 'path'; +import { removeImageBackground, restoreImageOriginal } from '../../src/intake/backgroundRemoval'; +``` + +And append to the end of the file: + +```typescript +const PNG_BYTES = Buffer.from([0x89, 0x50, 0x4e, 0x47, 0x0d, 0x0a, 0x1a, 0x0a, 0, 0, 0, 0]); +const JPEG_BYTES = Buffer.from([0xff, 0xd8, 0xff, 0xe0, 0, 0, 0, 0, 0, 0, 0, 0]); + +interface ImagePaths { + image_path: string; + original_image_path: string | null; +} + +let uploads = ''; +let stub: http.Server | null = null; + +/** + * A real uploads directory and a stub sidecar. + * + * A temporary directory rather than the configured one, because these tests + * write files and a suite that leaves rubbish in a developer's uploads volume + * is a suite people stop running. + * + * No test here contacts the real sidecar. It takes forty seconds to start, and + * a suite that depends on that is broken by construction. + */ +async function startStub( + handler: (req: http.IncomingMessage, res: http.ServerResponse) => void +): Promise { + uploads = await fsp.mkdtemp(path.join(os.tmpdir(), 'bgremoval-')); + process.env.UPLOADS_DIR = uploads; + + stub = http.createServer((req, res) => { + // Drain the body before answering, or the client sees a reset rather than + // the status this test is about. + req.on('data', () => undefined); + req.on('end', () => handler(req, res)); + }); + await new Promise((resolve) => stub!.listen(0, '127.0.0.1', resolve)); + process.env.REMBG_URL = `http://127.0.0.1:${(stub!.address() as AddressInfo).port}`; +} + +function answerWithPng(_req: http.IncomingMessage, res: http.ServerResponse): void { + res.writeHead(200, { 'Content-Type': 'image/png' }); + res.end(PNG_BYTES); +} + +afterEach(async () => { + delete process.env.REMBG_URL; + if (stub) { + await new Promise((resolve) => stub!.close(() => resolve())); + stub = null; + } +}); + +async function seedWithFile(): Promise<{ itemId: number; imageId: number; name: string }> { + const name = 'original.jpg'; + const seeded = await seedSubmission(`/uploads/${name}`); + await fsp.writeFile(path.join(uploads, name), JPEG_BYTES); + return { ...seeded, name }; +} + +async function pathsOf(imageId: number): Promise { + const { rows } = await pool.query( + `SELECT image_path, original_image_path FROM item_images WHERE id = $1`, + [imageId] + ); + return rows[0]!; +} + +describe('removing one photo’s background', () => { + it('points the row at the cut-out and records where the original went', async () => { + await startStub(answerWithPng); + const { imageId } = await seedWithFile(); + + await removeImageBackground(imageId); + + const paths = await pathsOf(imageId); + expect(paths.image_path).toBe('/uploads/original-cutout.png'); + expect(paths.original_image_path).toBe('/uploads/original.jpg'); + }); + + // The original is the only copy of an item that may no longer be in the + // sender's hands. Nothing in this module is allowed to remove it. + it('leaves the original file on disk', async () => { + await startStub(answerWithPng); + const { imageId, name } = await seedWithFile(); + + await removeImageBackground(imageId); + + await expect(fsp.access(path.join(uploads, name))).resolves.toBeUndefined(); + }); + + it('writes the cut-out where the row now says it is', async () => { + await startStub(answerWithPng); + const { imageId } = await seedWithFile(); + + await removeImageBackground(imageId); + + await expect(fsp.readFile(path.join(uploads, 'original-cutout.png'))).resolves.toEqual( + PNG_BYTES + ); + }); + + // The guard that stops a second pass recording the cut-out as the original + // and losing the real one for good. + it('is a no-op on a photo that has already been cut out', async () => { + await startStub(answerWithPng); + const { imageId } = await seedWithFile(); + + await removeImageBackground(imageId); + await removeImageBackground(imageId); + + expect((await pathsOf(imageId)).original_image_path).toBe('/uploads/original.jpg'); + }); +}); + +describe('when the sidecar will not answer', () => { + it('leaves the row untouched on a 500', async () => { + await startStub((_req, res) => { + res.writeHead(500); + res.end('boom'); + }); + const { imageId } = await seedWithFile(); + + await expect(removeImageBackground(imageId)).rejects.toThrow(/500/); + + const paths = await pathsOf(imageId); + expect(paths.image_path).toBe('/uploads/original.jpg'); + expect(paths.original_image_path).toBeNull(); + }); + + it('leaves the row untouched when it returns something that is not an image', async () => { + await startStub((_req, res) => { + res.writeHead(200, { 'Content-Type': 'image/png' }); + res.end('gateway error'); + }); + const { imageId } = await seedWithFile(); + + await expect(removeImageBackground(imageId)).rejects.toThrow(/not a PNG/); + + const paths = await pathsOf(imageId); + expect(paths.image_path).toBe('/uploads/original.jpg'); + expect(paths.original_image_path).toBeNull(); + }); + + it('leaves the row untouched when it is unreachable', async () => { + await startStub(answerWithPng); + // Closed before the call, so the connection is refused rather than hung. + // The URL stays set, which is the case worth modelling: configured, and + // not there. + await new Promise((resolve) => stub!.close(() => resolve())); + stub = null; + const { imageId } = await seedWithFile(); + + await expect(removeImageBackground(imageId)).rejects.toThrow(); + + const paths = await pathsOf(imageId); + expect(paths.image_path).toBe('/uploads/original.jpg'); + expect(paths.original_image_path).toBeNull(); + }); +}); + +describe('putting the original back', () => { + it('swaps the paths back and clears the record', async () => { + await startStub(answerWithPng); + const { imageId } = await seedWithFile(); + await removeImageBackground(imageId); + + await restoreImageOriginal(imageId); + + const paths = await pathsOf(imageId); + expect(paths.image_path).toBe('/uploads/original.jpg'); + expect(paths.original_image_path).toBeNull(); + }); + + it('refuses a photo that was never cut out, rather than blanking its path', async () => { + await startStub(answerWithPng); + const { imageId } = await seedWithFile(); + + await expect(restoreImageOriginal(imageId)).rejects.toThrow(/no original/); + + expect((await pathsOf(imageId)).image_path).toBe('/uploads/original.jpg'); + }); +}); +``` + +- [ ] **Step 6: Run the integration tests to verify they pass** + +```bash +cd backend && npm run test:integration -- backgroundRemoval +``` + +Expected: PASS, 11 tests. + +- [ ] **Step 7: Commit** + +```bash +git add backend/src/intake/backgroundRemoval.ts backend/tests/unit/backgroundRemoval.test.ts backend/tests/integration/backgroundRemoval.integration.test.ts +git commit -m "feat(intake): swap a photo for a cut-out, keeping the original (#281)" +``` + +--- + +### Task 4: The worker honours the submitter's intent + +**Files:** +- Modify: `backend/src/intake/draftingWorker.ts` +- Test: `backend/tests/integration/draftingBackgroundRemoval.integration.test.ts` (create) + +**Interfaces:** +- Consumes: `removeBackgroundsForItem` from `./backgroundRemoval`; `item_drafts.remove_background` from Task 1. +- Produces: no new exports. `draftQueued`'s signature and `SweepResult` are unchanged. + +**Why a new test file.** These cases need a *successful* draft, and `drafting.integration.test.ts` has never produced one — every case in it either has no API key (and is skipped) or has no readable photo (and fails). It therefore has no mock for `draftListing`, and adding one there would be file-wide and would change what those existing tests exercise. A separate file keeps the mock's scope obvious. + +- [ ] **Step 1: Write the failing test** + +Create `backend/tests/integration/draftingBackgroundRemoval.integration.test.ts`: + +```typescript +import http from 'http'; +import { AddressInfo } from 'net'; +import { promises as fsp } from 'fs'; +import os from 'os'; +import path from 'path'; +import { pool } from '../../src/db'; +import { resetDb, closeDb } from './setup/testDb'; +import { draftQueued } from '../../src/intake/draftingWorker'; +import { resetAnthropicClient } from '../../src/intake/anthropicClient'; +import { draftListing } from '../../src/intake/draftListing'; + +/** + * The worker's background-removal step (#281). + * + * The model is mocked rather than reached. What is under test is what the + * worker does *after* a draft is written — which of the two paths it takes, + * and what survives when the sidecar does not answer — and none of that + * depends on what the model said. + */ +jest.mock('../../src/intake/draftListing'); + +const draftListingMock = draftListing as jest.MockedFunction; + +const JPEG_BYTES = Buffer.from([0xff, 0xd8, 0xff, 0xe0, 0, 0, 0, 0, 0, 0, 0, 0]); +const PNG_BYTES = Buffer.from([0x89, 0x50, 0x4e, 0x47, 0x0d, 0x0a, 0x1a, 0x0a, 0, 0, 0, 0]); + +let uploads = ''; +let stub: http.Server | null = null; + +beforeEach(async () => { + await resetDb(); + + draftListingMock.mockResolvedValue({ + draft: { + name: 'Blue stoneware vase', + description: 'Hand-thrown, chipped base.', + category: null, + tags: [], + suggestedPriceCents: 4500 + }, + model: 'claude-sonnet-5', + inputTokens: 1000, + outputTokens: 200, + costMicros: 4000 + }); + + // Non-empty is all that is needed: getAnthropicClient only has to return + // something other than null, and the mock above is what answers. + process.env.ANTHROPIC_API_KEY = 'sk-ant-not-used'; + resetAnthropicClient(); + + uploads = await fsp.mkdtemp(path.join(os.tmpdir(), 'workerbg-')); + process.env.UPLOADS_DIR = uploads; + + stub = http.createServer((req, res) => { + req.on('data', () => undefined); + req.on('end', () => { + res.writeHead(200, { 'Content-Type': 'image/png' }); + res.end(PNG_BYTES); + }); + }); + await new Promise((resolve) => stub!.listen(0, '127.0.0.1', resolve)); + process.env.REMBG_URL = `http://127.0.0.1:${(stub!.address() as AddressInfo).port}`; +}); + +afterEach(async () => { + delete process.env.REMBG_URL; + delete process.env.ANTHROPIC_API_KEY; + resetAnthropicClient(); + if (stub) { + await new Promise((resolve) => stub!.close(() => resolve())); + stub = null; + } +}); + +afterAll(async () => { + await pool.end(); + await closeDb(); +}); + +/** A queued submission with one real file on disk and the intent set. */ +async function seedSubmissionWithPhoto(options: { removeBackground: boolean }): Promise { + const { rows } = await pool.query<{ id: number }>( + `INSERT INTO items (name, status) VALUES ('Submission placeholder', 'pending') RETURNING id` + ); + const itemId = rows[0]!.id; + await pool.query( + `INSERT INTO item_drafts (item_id, submitter_note, remove_background) + VALUES ($1, 'a note', $2)`, + [itemId, options.removeBackground] + ); + await pool.query( + `INSERT INTO item_images (item_id, image_path, sort_order) + VALUES ($1, '/uploads/worker.jpg', 0)`, + [itemId] + ); + await fsp.writeFile(path.join(uploads, 'worker.jpg'), JPEG_BYTES); + return itemId; +} + +async function originalPathOf(itemId: number): Promise { + const { rows } = await pool.query<{ original_image_path: string | null }>( + `SELECT original_image_path FROM item_images WHERE item_id = $1`, + [itemId] + ); + return rows[0]?.original_image_path ?? null; +} + +describe('background removal after a draft', () => { + // The mock has to actually be in play, or the two cases below would both + // pass for the wrong reason — a draft that never happened cuts nothing out. + it('drafts successfully, which is what the removal step follows', async () => { + await seedSubmissionWithPhoto({ removeBackground: false }); + + expect(await draftQueued(1)).toEqual({ drafted: 1, failed: 0, skipped: 0 }); + }); + + // Recorded at submission and acted on here, so the sender never waits and a + // sidecar that is down cannot fail their upload. + it('cuts out the photos when the submitter asked for it', async () => { + const itemId = await seedSubmissionWithPhoto({ removeBackground: true }); + + await draftQueued(1); + + expect(await originalPathOf(itemId)).toBe('/uploads/worker.jpg'); + }); + + it('leaves the photos alone when they did not', async () => { + const itemId = await seedSubmissionWithPhoto({ removeBackground: false }); + + await draftQueued(1); + + expect(await originalPathOf(itemId)).toBeNull(); + }); + + // The governing rule: removal is a convenience on top of a draft that was + // written correctly. A sidecar failure must never turn a good draft into a + // failed one, because the queue is what the admin actually works from. + it('leaves the draft ready when the sidecar fails', async () => { + const itemId = await seedSubmissionWithPhoto({ removeBackground: true }); + // Configured, and nothing listening on it. + process.env.REMBG_URL = 'http://127.0.0.1:1'; + + await draftQueued(1); + + const { rows } = await pool.query<{ state: string }>( + `SELECT state FROM item_drafts WHERE item_id = $1`, + [itemId] + ); + expect(rows[0]?.state).toBe('ready'); + expect(await originalPathOf(itemId)).toBeNull(); + }); +}); +``` + +- [ ] **Step 2: Run it to verify it fails** + +```bash +cd backend && npm run test:integration -- draftingBackgroundRemoval +``` + +Expected: FAIL — `original_image_path` is null in the first case, because nothing calls the removal yet. + +- [ ] **Step 3: Wire it into the worker** + +In `backend/src/intake/draftingWorker.ts`, add the import beside the others: + +```typescript +import { removeBackgroundsForItem } from './backgroundRemoval'; +``` + +Add the column to `QueuedRow`: + +```typescript +interface QueuedRow { + item_id: number; + submitter_note: string | null; + remove_background: boolean; +} +``` + +Add it to the SELECT in `draftQueued`: + +```typescript + `SELECT item_id, submitter_note, remove_background FROM item_drafts + WHERE state = 'queued' AND attempts < $2 + ORDER BY created_at + LIMIT $1`, +``` + +And inside the `for (const row of rows)` loop, immediately after `drafted++;` and before the `notifyDraftReady` call: + +```typescript + // Deliberately after the draft is committed, and catching for itself. + // + // This is the sender's tick from the submission page, honoured here so + // they never waited for it — and a failure must not mark a draft that was + // written correctly as failed. The photo keeps its original in that case, + // and the admin's per-photo control is still there to do it by hand. + // + // Awaited, unlike the notification below, so a sweep that has returned + // has finished its work. Nothing is waiting on this: the worker is off + // the request path, which is the whole reason drafting lives here. + if (row.remove_background) { + await removeBackgroundsForItem(row.item_id).catch((err) => + console.error(`[drafting] background removal for item ${row.item_id}:`, err) + ); + } +``` + +Also extend the file's header comment, after the paragraph beginning "The governing rule is that a submission is the only irreplaceable thing here": + +```typescript + * Background removal (#281) follows drafting rather than running on its own + * pass. That couples the two: an environment with no ANTHROPIC_API_KEY drafts + * nothing, so it cuts out nothing either. That is the intended trade — a + * separate pass would re-attempt an unreachable sidecar on every sweep for a + * row that is going to sit at 'queued' indefinitely — and the admin's per-photo + * control in the review queue is the way to do it by hand meanwhile. +``` + +- [ ] **Step 4: Run the tests to verify they pass** + +```bash +cd backend && npm run test:integration -- drafting backgroundRemoval && npm run build && npm run lint +``` + +Expected: PASS, clean build, clean lint. That pattern runs the new file, the existing `drafting` suite and both `backgroundRemoval` suites — the existing one matters here, because adding a column to its SELECT is exactly the kind of change that breaks a neighbouring test quietly. + +- [ ] **Step 5: Commit** + +```bash +git add backend/src/intake/draftingWorker.ts backend/tests/integration/draftingBackgroundRemoval.integration.test.ts +git commit -m "feat(intake): cut out backgrounds in the worker, never in the upload (#281)" +``` + +--- + +### Task 5: The intake route records the intent + +**Files:** +- Modify: `backend/src/routes/intake.ts` +- Test: `backend/tests/integration/intake.integration.test.ts` + +**Interfaces:** +- Consumes: `isRembgConfigured` from `../intake/rembgClient`. +- Produces: + - `GET /api/intake/:token` → `{ label: string, backgroundRemoval: boolean }` + - `POST /api/intake/:token` accepts a `removeBackground` multipart field; only the exact string `'false'` opts out. + +- [ ] **Step 1: Write the failing test** + +Append to `backend/tests/integration/intake.integration.test.ts`. That file already has `issueLink(label?, maxSubmissions?)` and a module-level `PNG` fixture; it posts photos inline as `request(app).post(...).attach('images', PNG, 'a.png')`. Add one small helper beside `issueLink` so these cases can also send fields: + +```typescript +/** A submission with optional extra multipart fields beside the photo. */ +function postPhoto(token: string, fields: Record = {}) { + const req = request(app).post(`/api/intake/${token}`); + for (const [name, value] of Object.entries(fields)) void req.field(name, value); + return req.attach('images', PNG, 'a.png'); +} +``` + +```typescript +describe('the background-removal intent', () => { + // Ticked by default on the page, so absent means yes. An older client or a + // curl call then behaves like the current default rather than silently + // opting out of something every other submission gets. + it('defaults to true when the field is not sent', async () => { + const token = await issueLink(); + await postPhoto(token); + + const { rows } = await pool.query<{ remove_background: boolean }>( + `SELECT remove_background FROM item_drafts ORDER BY id DESC LIMIT 1` + ); + expect(rows[0]?.remove_background).toBe(true); + }); + + it('records a submitter who unticked it', async () => { + const token = await issueLink(); + await postPhoto(token, { removeBackground: 'false' }); + + const { rows } = await pool.query<{ remove_background: boolean }>( + `SELECT remove_background FROM item_drafts ORDER BY id DESC LIMIT 1` + ); + expect(rows[0]?.remove_background).toBe(false); + }); + + // Only the exact string opts out. A stray value is not a considered "no", + // and reading it as one would quietly deny somebody something they asked for. + it('treats anything other than "false" as consent', async () => { + const token = await issueLink(); + await postPhoto(token, { removeBackground: 'no' }); + + const { rows } = await pool.query<{ remove_background: boolean }>( + `SELECT remove_background FROM item_drafts ORDER BY id DESC LIMIT 1` + ); + expect(rows[0]?.remove_background).toBe(true); + }); +}); + +describe('what the submission page is told', () => { + it('says the feature is off when there is no sidecar', async () => { + delete process.env.REMBG_URL; + const token = await issueLink(); + + const res = await request(app).get(`/api/intake/${token}`); + + expect(res.status).toBe(200); + expect(res.body.backgroundRemoval).toBe(false); + }); + + it('says it is on when there is one', async () => { + process.env.REMBG_URL = 'http://rembg-syn:7000'; + const token = await issueLink(); + + const res = await request(app).get(`/api/intake/${token}`); + + expect(res.body.backgroundRemoval).toBe(true); + delete process.env.REMBG_URL; + }); +}); +``` + +- [ ] **Step 2: Run it to verify it fails** + +```bash +cd backend && npm run test:integration -- intake.integration +``` + +Expected: FAIL — `backgroundRemoval` is undefined on the GET, and the unticked case records `true`. + +- [ ] **Step 3: Write the implementation** + +In `backend/src/routes/intake.ts`, add the import beside the others: + +```typescript +import { isRembgConfigured } from '../intake/rembgClient'; +``` + +Change the GET handler's response: + +```typescript + // The label, and whether the background-removal control has anything behind + // it. Still nothing about the catalogue, the admin, or other links. + res.json({ label: link.label, backgroundRemoval: isRembgConfigured() }); +``` + +In the POST handler, immediately after the `note` line: + +```typescript + // Absent means yes: the checkbox on the page is ticked by default, so a + // client that does not send the field — an older build, or a script — gets + // what every other submission gets rather than silently opting out. + // + // Only the exact string opts out. Multipart fields arrive as strings, and + // reading a stray value as "no" would quietly deny somebody something they + // asked for. + const removeBackground = req.body?.removeBackground !== 'false'; +``` + +And extend the `item_drafts` insert: + +```typescript + await client.query( + `INSERT INTO item_drafts (item_id, upload_link_id, submitter_note, remove_background) + VALUES ($1, $2, $3, $4)`, + [itemId, link.id, note === '' ? null : note, removeBackground] + ); +``` + +Nothing else in this route changes. The AI is still not called here, no bytes are sent to the sidecar here, and the ordering of `requireUsableLink` and `requireCapacity` ahead of `uploadImages` is untouched — that ordering is the whole mitigation that keeps a refused submission from writing a byte to disk. + +- [ ] **Step 4: Run the tests to verify they pass** + +```bash +cd backend && npm run test:integration -- intake.integration && npx jest -c jest.unit.config.js tests/unit/routesAreWrapped.test.ts +``` + +Expected: PASS. The wrapper guard confirms no handler was added unwrapped. + +- [ ] **Step 5: Commit** + +```bash +git add backend/src/routes/intake.ts backend/tests/integration/intake.integration.test.ts +git commit -m "feat(intake): record whether the submitter asked for a cut-out (#281)" +``` + +--- + +### Task 6: The admin endpoints + +**Files:** +- Modify: `backend/src/routes/adminItemDrafts.ts` +- Test: `backend/tests/integration/adminItemDrafts.integration.test.ts` + +**Interfaces:** +- Consumes: `removeImageBackground`, `restoreImageOriginal` from `../intake/backgroundRemoval`; `isRembgConfigured` from `../intake/rembgClient`. +- Produces: + - `GET /api/admin/item-drafts` → `{ drafts: Draft[], backgroundRemoval: boolean }` (**the response shape changes** — Task 8 updates the client) + - each entry of `draft.images` gains `original_image_path: string | null` + - `POST /api/admin/item-drafts/:itemId/images/:imageId/remove-background` → `200 { image_path, original_image_path }` | `404` | `502` + - `POST /api/admin/item-drafts/:itemId/images/:imageId/restore-original` → `200 { image_path, original_image_path }` | `404` + +- [ ] **Step 1: Write the failing test** + +Append to `backend/tests/integration/adminItemDrafts.integration.test.ts`. That file calls `request(app)` directly — there is no authenticated-request helper, and these routes sit behind the same admin gate as the ones already tested there, so nothing extra is needed. + +Its existing `seedDraft` returns only an item id and writes its photo at `/uploads/a.jpg`, so this block needs its own seed that returns the image id too and uses a name matching the cut-out path being asserted. + +```typescript +describe('the review queue’s background-removal control', () => { + const PNG_BYTES = Buffer.from([0x89, 0x50, 0x4e, 0x47, 0x0d, 0x0a, 0x1a, 0x0a, 0, 0, 0, 0]); + const JPEG_BYTES = Buffer.from([0xff, 0xd8, 0xff, 0xe0, 0, 0, 0, 0, 0, 0, 0, 0]); + + let uploads = ''; + let stub: http.Server | null = null; + + /** A stub sidecar on an ephemeral port, and a temporary uploads directory. */ + async function startStub(status: number, body: Buffer | string): Promise { + uploads = await fsp.mkdtemp(path.join(os.tmpdir(), 'adminbg-')); + process.env.UPLOADS_DIR = uploads; + + stub = http.createServer((req, res) => { + req.on('data', () => undefined); + req.on('end', () => { + res.writeHead(status, { 'Content-Type': 'image/png' }); + res.end(body); + }); + }); + await new Promise((resolve) => stub!.listen(0, '127.0.0.1', resolve)); + process.env.REMBG_URL = `http://127.0.0.1:${(stub!.address() as AddressInfo).port}`; + } + + afterEach(async () => { + delete process.env.REMBG_URL; + if (stub) { + await new Promise((resolve) => stub!.close(() => resolve())); + stub = null; + } + }); + + /** A ready draft with one photo, on disk, named to match the assertions. */ + async function seedDraftWithImage(): Promise<{ itemId: number; imageId: number }> { + const { rows } = await pool.query<{ id: number }>( + `INSERT INTO items (name, status) VALUES ('Submission placeholder', 'pending') RETURNING id` + ); + const itemId = rows[0]!.id; + await pool.query(`INSERT INTO item_drafts (item_id, state) VALUES ($1, 'ready')`, [itemId]); + const image = await pool.query<{ id: number }>( + `INSERT INTO item_images (item_id, image_path, sort_order) + VALUES ($1, '/uploads/original.jpg', 0) RETURNING id`, + [itemId] + ); + if (uploads !== '') await fsp.writeFile(path.join(uploads, 'original.jpg'), JPEG_BYTES); + return { itemId, imageId: image.rows[0]!.id }; + } + + it('says whether there is a sidecar behind the control at all', async () => { + process.env.REMBG_URL = 'http://rembg-syn:7000'; + const on = await request(app).get('/api/admin/item-drafts'); + expect(on.body.backgroundRemoval).toBe(true); + + delete process.env.REMBG_URL; + const off = await request(app).get('/api/admin/item-drafts'); + expect(off.body.backgroundRemoval).toBe(false); + }); + + // The UI decides between "Remove background" and "Restore original" from + // this field alone, so it has to be in the payload the queue is built from. + it('includes original_image_path on every image', async () => { + await seedDraftWithImage(); + + const res = await request(app).get('/api/admin/item-drafts'); + + expect(res.body.drafts[0].images[0]).toHaveProperty('original_image_path', null); + }); + + it('cuts out one photo and answers with its new paths', async () => { + await startStub(200, PNG_BYTES); + const { itemId, imageId } = await seedDraftWithImage(); + + const res = await request(app).post( + `/api/admin/item-drafts/${itemId}/images/${imageId}/remove-background` + ); + + expect(res.status).toBe(200); + expect(res.body.image_path).toBe('/uploads/original-cutout.png'); + expect(res.body.original_image_path).toBe('/uploads/original.jpg'); + }); + + it('puts the original back', async () => { + await startStub(200, PNG_BYTES); + const { itemId, imageId } = await seedDraftWithImage(); + await request(app).post( + `/api/admin/item-drafts/${itemId}/images/${imageId}/remove-background` + ); + + const res = await request(app).post( + `/api/admin/item-drafts/${itemId}/images/${imageId}/restore-original` + ); + + expect(res.status).toBe(200); + expect(res.body.image_path).toBe('/uploads/original.jpg'); + expect(res.body.original_image_path).toBeNull(); + }); + + // 502 rather than 500: the request was fine and the app is fine, and saying + // which of the two failed is what stops somebody searching the application + // logs for a fault that is not there. + it('answers 502 when the sidecar will not, and leaves the photo alone', async () => { + await startStub(500, 'boom'); + const { itemId, imageId } = await seedDraftWithImage(); + + const res = await request(app).post( + `/api/admin/item-drafts/${itemId}/images/${imageId}/remove-background` + ); + + expect(res.status).toBe(502); + const { rows } = await pool.query<{ image_path: string }>( + `SELECT image_path FROM item_images WHERE id = $1`, + [imageId] + ); + expect(rows[0]?.image_path).toBe('/uploads/original.jpg'); + }); + + // Scoped by item as well as by image. The id is a serial, so guessing one is + // not hard, and a photo from another submission must not be reachable + // through this item's URL. + it('refuses an image that does not belong to the item', async () => { + await startStub(200, PNG_BYTES); + const first = await seedDraftWithImage(); + const second = await seedDraftWithImage(); + + const res = await request(app).post( + `/api/admin/item-drafts/${first.itemId}/images/${second.imageId}/remove-background` + ); + + expect(res.status).toBe(404); + }); + + it('refuses to restore a photo that was never cut out', async () => { + const { itemId, imageId } = await seedDraftWithImage(); + + const res = await request(app).post( + `/api/admin/item-drafts/${itemId}/images/${imageId}/restore-original` + ); + + expect(res.status).toBe(404); + }); +}); +``` + +Add the imports this needs at the top of that file, beside its existing ones: + +```typescript +import http from 'http'; +import { AddressInfo } from 'net'; +import { promises as fsp } from 'fs'; +import os from 'os'; +import path from 'path'; +``` + +Note that the second seeded item in the ownership case overwrites the same +`original.jpg`, which is harmless — that case never reaches the file. + +- [ ] **Step 2: Run it to verify it fails** + +```bash +cd backend && npm run test:integration -- adminItemDrafts +``` + +Expected: FAIL — `backgroundRemoval` undefined and both POSTs 404. + +- [ ] **Step 3: Write the implementation** + +In `backend/src/routes/adminItemDrafts.ts`, add the imports: + +```typescript +import { removeImageBackground, restoreImageOriginal } from '../intake/backgroundRemoval'; +import { isRembgConfigured } from '../intake/rembgClient'; +``` + +In `DRAFT_SELECT`, extend the images aggregate to carry the restorability: + +```sql + COALESCE(( + SELECT json_agg(json_build_object( + 'id', img.id, + 'image_path', img.image_path, + 'original_image_path', img.original_image_path) + ORDER BY img.sort_order) + FROM item_images img WHERE img.item_id = d.item_id + ), '[]'::json) AS images +``` + +Change the GET handler's response: + +```typescript + // Whether the control has anything behind it, alongside the rows. A second + // endpoint for one boolean would be a round trip the queue already makes. + res.json({ drafts: rows, backgroundRemoval: isRembgConfigured() }); +``` + +Then add, before `export default router;`: + +```typescript +/** + * One photo's current paths, if it belongs to this item. + * + * Scoped by item as well as by image so an image id from a different + * submission cannot be acted on through this item's URL — the id is a serial, + * so guessing one is not hard. + */ +async function imageOfItem( + itemId: string, + imageId: string +): Promise<{ image_path: string; original_image_path: string | null } | null> { + const { rows } = await pool.query<{ image_path: string; original_image_path: string | null }>( + `SELECT image_path, original_image_path + FROM item_images WHERE id = $1 AND item_id = $2`, + [imageId, itemId] + ); + return rows[0] ?? null; +} + +/** + * Remove the background from one photo. + * + * The other half of the submitter's checkbox: for the photos nobody ticked it + * for, and for the ones where the worker could not reach the sidecar. Both go + * through the same module, so a cut-out obtained either way is identical and + * either can be undone by Restore. + * + * Synchronous, unlike the worker's path. A warm request measures 1.1–2.3 s and + * this is an admin who just clicked a button and is watching for the result. + * The reason drafting was moved off the request path — that a stranger can + * trigger it and must never wait — does not apply behind the admin gate. + */ +router.post( + '/:itemId/images/:imageId/remove-background', + asyncRoute(async (req: Request, res: Response) => { + const { itemId = '', imageId = '' } = req.params; + if ((await imageOfItem(itemId, imageId)) === null) { + return res.status(404).json({ error: 'no such photo on this item' }); + } + + try { + await removeImageBackground(Number(imageId)); + } catch (err) { + // 502, not 500. The request was fine and so is this app — the service it + // depends on did not answer. The message says the photo is unchanged, + // because that is the thing the admin actually needs to know. + console.error(`[drafts] background removal for image ${imageId}:`, err); + return res + .status(502) + .json({ error: 'the background-removal service did not answer — the photo is unchanged' }); + } + + res.json(await imageOfItem(itemId, imageId)); + }) +); + +/** + * Put the original photo back. + * + * The reason a cut-out is safe to try at all. Background removal produces the + * occasional poor result on an unusual object, and this makes that survivable + * rather than something to prevent. Nothing is deleted: the cut-out file stays + * on disk, because somebody restoring one is quite likely to try again. + */ +router.post( + '/:itemId/images/:imageId/restore-original', + asyncRoute(async (req: Request, res: Response) => { + const { itemId = '', imageId = '' } = req.params; + const existing = await imageOfItem(itemId, imageId); + if (existing === null || existing.original_image_path === null) { + return res.status(404).json({ error: 'this photo has no original to restore' }); + } + + await restoreImageOriginal(Number(imageId)); + res.json(await imageOfItem(itemId, imageId)); + }) +); +``` + +- [ ] **Step 4: Run the tests to verify they pass** + +```bash +cd backend && npm run test:integration -- adminItemDrafts && npx jest -c jest.unit.config.js tests/unit/routesAreWrapped.test.ts && npm run build && npm run lint +``` + +Expected: PASS, clean build, clean lint. + +- [ ] **Step 5: Commit** + +```bash +git add backend/src/routes/adminItemDrafts.ts backend/tests/integration/adminItemDrafts.integration.test.ts +git commit -m "feat(admin): remove and restore a photo's background per image (#281)" +``` + +--- + +### Task 7: The submitter's checkbox + +**Files:** +- Modify: `frontend/src/intake/intakeApi.ts`, `frontend/src/intake/Submit.tsx`, `scripts/start-local.ps1` +- Test: `frontend/tests/e2e/intake-submit.spec.ts` + +**Interfaces:** +- Consumes: `GET /api/intake/:token` → `{ label, backgroundRemoval }` and the `removeBackground` field from Task 5. +- Produces: `IntakeLink` gains `backgroundRemoval: boolean`; `submitItem(token, files, note, removeBackground)` — **a fourth required parameter**. + +- [ ] **Step 1: Update the API client** + +In `frontend/src/intake/intakeApi.ts`, extend the interface: + +```typescript +export interface IntakeLink { + label: string; + /** + * Whether there is a background-removal sidecar behind the checkbox. False + * hides it entirely rather than showing a control that would do nothing — + * an unconfigured environment is a working one, not a broken one. + */ + backgroundRemoval: boolean; +} +``` + +And the submit function's signature and body, leaving the rest of it unchanged: + +```typescript +export async function submitItem( + token: string, + files: File[], + note: string, + removeBackground: boolean +): Promise { + const body = new FormData(); + // The field name the server's multer instance listens on. Sending several + // under one name is what makes req.files an array. + for (const file of files) body.append('images', file); + body.append('note', note); + // A string, because that is all a multipart field can be. The server treats + // only the exact 'false' as an opt-out, so this is the one value that has to + // be got right. + body.append('removeBackground', removeBackground ? 'true' : 'false'); +``` + +- [ ] **Step 2: Add the checkbox to the page** + +In `frontend/src/intake/Submit.tsx`, add the import with the other antd ones: + +```typescript +import Checkbox from 'antd/es/checkbox'; +``` + +Add the state beside `note`: + +```typescript + // Ticked by default. Most items look better cut out, and a submitter who + // wants their kitchen table in the photograph can say so — the reverse + // default would mean almost nobody got it. + const [removeBackground, setRemoveBackground] = useState(true); +``` + +Pass it in `send`: + +```typescript + const result = await submitItem( + token, + files.flatMap((f) => (f.originFileObj ? [f.originFileObj] : [])), + note, + removeBackground + ); +``` + +Reset it in the "Send another item" handler, beside `setNote('')`: + +```typescript + setRemoveBackground(true); +``` + +And render it between the `TextArea` and the error `Alert`, inside the `Space`: + +```tsx + {state.kind === 'usable' && state.link.backgroundRemoval && ( + setRemoveBackground(e.target.checked)} + > + {/* Described by what it does, not by how. Nobody sending in a + vase knows what a cut-out or an alpha channel is. */} + Remove the background from my photos + + )} +``` + +- [ ] **Step 3: Set REMBG_URL for the local stack** + +The two e2e cases below only pass when the backend has `REMBG_URL` set — the checkbox's visibility depends on nothing else. Add it to the backend environment in `scripts/start-local.ps1`, beside the other optional variables it sets, pointing at a value that need not resolve: no e2e submission reaches the sidecar, because the worker only cuts out after a draft and drafting is not configured locally. + +```powershell +$env:REMBG_URL = 'http://127.0.0.1:7000' +``` + +Read that script and follow its existing style before editing. **Never run it non-interactively** — it prompts for elevation and can strip Node from the machine. Ask the user to run it. + +- [ ] **Step 4: Write the failing e2e test** + +Append to `frontend/tests/e2e/intake-submit.spec.ts`, inside the existing `test.describe`: + +```typescript + // Ticked by default, because that is the decision: most items look better + // cut out, and the reverse default would mean almost nobody got it. + test('offers to remove the background, already ticked', async ({ page }) => { + await page.goto(`/submit/${token}`); + + const control = page.getByRole('checkbox', { name: /remove the background/i }); + await expect(control).toBeVisible(); + await expect(control).toBeChecked(); + }); + + test('lets a sender turn it off and still send', async ({ page }) => { + await page.goto(`/submit/${token}`); + + const png = Buffer.from( + 'iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAYAAAAfFcSJAAAADUlEQVR42mP8z8BQDwAEhQGAhKmMIQAAAABJRU5ErkJggg==', + 'base64' + ); + await page.setInputFiles('input[type="file"]', { + name: `${RUN}-nobg.png`, + mimeType: 'image/png', + buffer: png + }); + await page.getByRole('checkbox', { name: /remove the background/i }).uncheck(); + await page.getByRole('button', { name: 'Send' }).click(); + + await expect(page.getByRole('heading', { name: 'Thank you — it arrived' })).toBeVisible(); + }); +``` + +- [ ] **Step 5: Verify the frontend builds and its unit tests pass** + +```bash +cd frontend && npm run build && npm test +``` + +`npm run build`, not `npx tsc --noEmit`: the app tsconfig excludes `tests/`, and a green bare `tsc` once broke a deploy. Any call site of `submitItem` that was not updated fails here, which is the point of making the fourth parameter required. + +Expected: clean build, unit tests pass. + +- [ ] **Step 6: Run the e2e spec** + +With the local stack running (ask the user to start it): + +```bash +cd frontend && npx playwright test intake-submit --project=chromium +``` + +Expected: PASS, 6 tests. + +- [ ] **Step 7: Commit** + +```bash +git add frontend/src/intake/intakeApi.ts frontend/src/intake/Submit.tsx frontend/tests/e2e/intake-submit.spec.ts scripts/start-local.ps1 +git commit -m "feat(intake): offer background removal on the submission page, ticked (#281)" +``` + +--- + +### Task 8: The admin's per-photo control, and the docs + +**Files:** +- Modify: `frontend/src/admin/draftsApi.ts`, `frontend/src/admin/DraftQueue.tsx` +- Modify: `docs/ops/image-background-removal-stack.md` +- Test: `frontend/tests/e2e/admin-draft-queue.spec.ts` + +**Interfaces:** +- Consumes: the endpoints and response shapes from Task 6. +- Produces: `DraftImage` gains `original_image_path: string | null`; `fetchDrafts` returns `DraftQueueResponse` — **a changed return type**; `setImageBackground(itemId, imageId, action)`. + +- [ ] **Step 1: Update the API client** + +In `frontend/src/admin/draftsApi.ts`: + +```typescript +export interface DraftImage { + id: number; + image_path: string; + /** + * Where this photo came from, once it has been cut out. Null means it never + * was — which is also the answer to whether Restore has anything to do, so + * there is no second flag that could disagree with it. + */ + original_image_path: string | null; +} +``` + +Change `fetchDrafts` and add the action: + +```typescript +export interface DraftQueueResponse { + drafts: Draft[]; + /** + * Whether a background-removal sidecar is configured. False hides the + * control rather than showing one that would answer 502 — an environment + * without a sidecar is a working environment. + */ + backgroundRemoval: boolean; +} + +export async function fetchDrafts(state?: string): Promise { + const query = state ? `?state=${encodeURIComponent(state)}` : ''; + const res = await send(query); + if (!res.ok) throw new Error('could not load the review queue'); + return res.json(); +} + +/** + * Cut one photo out, or put its original back. + * + * The server's message is preferred over a generic one for the same reason + * publishDraft prefers it: a 502 here says the removal service did not answer + * and the photo is unchanged, which is the difference between "try again" and + * "something is wrong with this item". + */ +export async function setImageBackground( + itemId: number, + imageId: number, + action: 'remove-background' | 'restore-original' +): Promise { + const res = await send(`/${itemId}/images/${imageId}/${action}`, { method: 'POST' }); + if (res.ok) return; + + let message = 'could not change this photo'; + try { + message = (await res.json()).error ?? message; + } catch { + // A non-JSON body is a proxy or gateway error rather than the app + // refusing. The generic message is the honest thing to show. + } + throw new Error(message); +} +``` + +- [ ] **Step 2: Add the control to the queue** + +In `frontend/src/admin/DraftQueue.tsx`, extend the import: + +```typescript +import { + Draft, + DraftImage, + PriceSource, + actOnDraft, + fetchDrafts, + publishDraft, + setImageBackground +} from './draftsApi'; +``` + +Add a component above `DraftCard`: + +```tsx +/** + * One photo, with the control that cuts it out or puts it back. + * + * Per photo rather than per item because that is how a poor result is undone: + * background removal produces the occasional bad cut on an unusual object, and + * the answer is to restore that one photograph, not to unpick the submission. + * + * The label is the state. `original_image_path` is the only thing consulted, + * so there is no second flag that could disagree with what the button does. + */ +function DraftPhoto({ + image, + itemId, + enabled, + onChanged +}: Readonly<{ + image: DraftImage; + itemId: number; + enabled: boolean; + onChanged: () => void; +}>) { + const [busy, setBusy] = useState(false); + const cutOut = image.original_image_path !== null; + + const act = async () => { + setBusy(true); + try { + await setImageBackground(itemId, image.id, cutOut ? 'restore-original' : 'remove-background'); + onChanged(); + } catch (err) { + message.error(err instanceof Error ? err.message : 'that did not work'); + } finally { + setBusy(false); + } + }; + + return ( + + + {enabled && ( + + )} + + ); +} +``` + +Give `DraftCard` the flag: + +```tsx +function DraftCard({ + draft, + backgroundRemoval, + onChanged +}: Readonly<{ draft: Draft; backgroundRemoval: boolean; onChanged: () => void }>) { +``` + +and replace its `draft.images.map(...)` block with: + +```tsx + + {draft.images.map((image) => ( + + ))} + +``` + +In `DraftQueue`, hold the flag and pass it down: + +```tsx + const [drafts, setDrafts] = useState([]); + const [backgroundRemoval, setBackgroundRemoval] = useState(false); +``` + +```tsx + const load = useCallback(async () => { + try { + const payload = await fetchDrafts(state); + setDrafts(payload.drafts); + setBackgroundRemoval(payload.backgroundRemoval); + setError(null); + } catch { + setError('Could not load the review queue.'); + } + }, [state]); +``` + +```tsx + {drafts.map((draft) => ( + void load()} + /> + ))} +``` + +- [ ] **Step 3: Write the failing e2e test** + +Append to the existing `test.describe('The review queue', ...)` block in `frontend/tests/e2e/admin-draft-queue.spec.ts`. That file already has `submitAnItem(page, note)`, which seeds through the real intake route, and its tests take the `{ page, admin }` fixtures and call `admin.open('Review queue')`. + +Scope the assertion to the card this test created, by the sender's note — the suite is fullyParallel against a dev database that never truncates, so a queue-wide locator outruns its timeout and fails for reasons unrelated to the behaviour under test (#241). + +```typescript + // The control that makes a poor cut survivable. Its label is its state: + // "Remove background" until an original has been recorded, "Restore + // original" afterwards, read from one field rather than two that could + // disagree. + // + // Only the label is asserted, not a click. Pressing it would need a sidecar, + // and a test that depends on a service taking forty seconds to start is + // broken by construction — the swap itself is covered in the integration + // suite against a stub. + test('offers to remove the background on each photo', async ({ page, admin }) => { + const note = `Cutout ${RUN}`; + await submitAnItem(page, note); + + await admin.open('Review queue'); + + const card = page.locator('.ant-card').filter({ hasText: note }); + await expect(card.getByRole('button', { name: 'Remove background' })).toBeVisible(); + }); +``` + +This passes only when the backend has `REMBG_URL` set, which Task 7 added to `scripts/start-local.ps1`. + +- [ ] **Step 4: Update the ops document** + +`docs/ops/image-background-removal-stack.md` still says **"Status: evaluated, not adopted"** and closes with an "If this is adopted" section. Both are now wrong. Replace the status paragraph near the top with: + +```markdown +**Status: adopted (#281).** The engine choice is settled — see "The one thing +that must not be got wrong" below. Everything here was run and measured on +2026-09-02 against `danielgatis/rembg:latest`, on a Windows dev box under Docker +Desktop. **The NAS is a different machine and will be slower**; treat these as +an upper bound on capability, not a promise. +``` + +And replace the whole "If this is adopted" section at the end with: + +```markdown +## How the application uses it + +`backend/src/intake/rembgClient.ts` is the only thing that talks to the +sidecar. It posts to `/api/remove` and **always sends `model=u2net`**; a unit +test asserts that parameter is present, because nothing about the returned +image would reveal its absence. + +`REMBG_URL` points at it — `http://rembg-syn:7000` on the NAS, where the +container publishes `32700:7000`. The variable is optional in both compose +files: unset means the submission page shows no checkbox, the review queue +shows no control, and the drafting worker skips the step. An environment +without a sidecar is a working environment. + +Two entry points, one module (`backend/src/intake/backgroundRemoval.ts`): + +- **The drafting worker**, honouring the checkbox on `/submit/:token`, which is + ticked by default. The submitter's tick is recorded and acted on later, so + nobody waits on a model run and an unreachable sidecar cannot fail an upload. +- **The review queue**, per photo, synchronously — 1.1–2.3 s warm is a wait an + admin who just clicked a button can absorb. + +Because removal follows drafting in the worker, an environment with no +`ANTHROPIC_API_KEY` drafts nothing and so cuts out nothing. The per-photo +control in the review queue is the way to do it by hand there. + +The original file is never destroyed. `item_images.original_image_path` records +where it went, and **Restore original** swaps it back. Nothing in the feature +deletes a file or a row. +``` + +- [ ] **Step 5: Verify everything** + +```bash +cd frontend && npm run build && npm run lint && npm test +``` + +Then the full suites, with the stack up (ask the user to start it): + +```bash +cd backend && npm run test:unit && npm run test:integration +cd frontend && npx playwright test --project=chromium +``` + +Expected: all green. The e2e suite was 157/157 before this change; it should now be 160. + +- [ ] **Step 6: Commit** + +```bash +git add frontend/src/admin/draftsApi.ts frontend/src/admin/DraftQueue.tsx frontend/tests/e2e/admin-draft-queue.spec.ts docs/ops/image-background-removal-stack.md +git commit -m "feat(admin): remove or restore a photo's background from the review queue (#281)" +``` + +--- + +## After the plan + +- The branch is `feature/281-background-removal`. **Do not push** — the user pushes and merges. +- The PR body closes #281 and should say plainly what is *not* established: quality on a real photograph, and behaviour under concurrent requests. +- Per standing practice, follow this with a separate SonarQube cleanup issue and PR — hotspots, duplication, debt, coverage — never folded into this branch. +- QA needs `QA_REMBG_URL=http://rembg-syn:7000` set in the Portainer stack, and the `rembg-syn` service on the same network as the backend, before any of this does anything there. -- 2.54.0 From 534e3d62282461387a785d32e97a33cfe7b997c4 Mon Sep 17 00:00:00 2001 From: Thom Lamb Date: Thu, 3 Sep 2026 12:15:38 -0500 Subject: [PATCH 02/17] feat(intake): record the background-removal intent and the original path (#281) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .../1787600000000_add-background-removal.js | 35 ++++++++++++ backend/src/db-drizzle/schema.ts | 2 + .../backgroundRemoval.integration.test.ts | 54 +++++++++++++++++++ 3 files changed, 91 insertions(+) create mode 100644 backend/migrations/1787600000000_add-background-removal.js create mode 100644 backend/tests/integration/backgroundRemoval.integration.test.ts diff --git a/backend/migrations/1787600000000_add-background-removal.js b/backend/migrations/1787600000000_add-background-removal.js new file mode 100644 index 0000000..7d0ee0f --- /dev/null +++ b/backend/migrations/1787600000000_add-background-removal.js @@ -0,0 +1,35 @@ +exports.up = (pgm) => { + pgm.sql(` + -- The submitter's intent, per submission, because that is how it is + -- expressed: one checkbox above the send button, ticked by default. + -- + -- The worker acts on it rather than the intake route. Removing inline + -- would make the sender 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. + -- + -- NOT NULL DEFAULT true so a row written before this migration, or by any + -- path that does not mention the column, behaves like the new default. + ALTER TABLE item_drafts + ADD COLUMN IF NOT EXISTS remove_background BOOLEAN NOT NULL DEFAULT true; + + -- Where the photo came from, per image, because that is how it is undone. + -- Null until a photo has been cut out, so it is also the answer to "can + -- this be restored?" — one fact in one place rather than a flag that can + -- disagree with a path. + -- + -- Nullable and with no default: an existing image has no original other + -- than itself, and claiming otherwise would offer a Restore that swapped a + -- photo for a copy of itself. + ALTER TABLE item_images + ADD COLUMN IF NOT EXISTS original_image_path TEXT; + `); +}; + +exports.down = (pgm) => { + pgm.sql(` + ALTER TABLE item_drafts DROP COLUMN IF EXISTS remove_background; + ALTER TABLE item_images DROP COLUMN IF EXISTS original_image_path; + `); +}; diff --git a/backend/src/db-drizzle/schema.ts b/backend/src/db-drizzle/schema.ts index 729744c..21f5c07 100644 --- a/backend/src/db-drizzle/schema.ts +++ b/backend/src/db-drizzle/schema.ts @@ -29,6 +29,7 @@ export const itemImages = pgTable("item_images", { itemId: integer("item_id").notNull(), imagePath: text("image_path").notNull(), sortOrder: integer("sort_order").default(0).notNull(), + originalImagePath: text("original_image_path"), createdAt: timestamp("created_at", { withTimezone: true, mode: 'string' }).defaultNow().notNull(), }, (table) => [ foreignKey({ @@ -233,6 +234,7 @@ export const itemDrafts = pgTable("item_drafts", { itemId: integer("item_id").notNull(), uploadLinkId: integer("upload_link_id"), submitterNote: text("submitter_note"), + removeBackground: boolean("remove_background").default(true).notNull(), state: text().default('queued').notNull(), attempts: integer().default(0).notNull(), model: text(), diff --git a/backend/tests/integration/backgroundRemoval.integration.test.ts b/backend/tests/integration/backgroundRemoval.integration.test.ts new file mode 100644 index 0000000..4233c07 --- /dev/null +++ b/backend/tests/integration/backgroundRemoval.integration.test.ts @@ -0,0 +1,54 @@ +import { pool } from '../../src/db'; +import { resetDb, closeDb } from './setup/testDb'; + +beforeEach(async () => { + await resetDb(); +}); + +afterAll(async () => { + await pool.end(); + await closeDb(); +}); + +/** An item with a draft row and one image, which is what a submission leaves. */ +export async function seedSubmission( + imagePath = '/uploads/photo.jpg' +): Promise<{ itemId: number; imageId: number }> { + const item = await pool.query<{ id: number }>( + `INSERT INTO items (name, status) VALUES ('Submission placeholder', 'pending') RETURNING id` + ); + const itemId = item.rows[0]!.id; + await pool.query(`INSERT INTO item_drafts (item_id) VALUES ($1)`, [itemId]); + const image = await pool.query<{ id: number }>( + `INSERT INTO item_images (item_id, image_path, sort_order) VALUES ($1, $2, 0) RETURNING id`, + [itemId, imagePath] + ); + return { itemId, imageId: image.rows[0]!.id }; +} + +describe('the background-removal columns', () => { + // Default true because the submitter's checkbox is ticked by default, and + // because a row written by any path that does not mention the column should + // behave like the new default rather than needing a backfill. + it('defaults remove_background to true', async () => { + const { itemId } = await seedSubmission(); + + const { rows } = await pool.query<{ remove_background: boolean }>( + `SELECT remove_background FROM item_drafts WHERE item_id = $1`, + [itemId] + ); + expect(rows[0]?.remove_background).toBe(true); + }); + + // Null is also the answer to "can this be restored?", which is why there is + // no separate flag: one fact, one place. + it('leaves original_image_path null until a photo has been cut out', async () => { + const { imageId } = await seedSubmission(); + + const { rows } = await pool.query<{ original_image_path: string | null }>( + `SELECT original_image_path FROM item_images WHERE id = $1`, + [imageId] + ); + expect(rows[0]?.original_image_path).toBeNull(); + }); +}); -- 2.54.0 From 61e9c239d390eb127f8436ab28119508911d6ea1 Mon Sep 17 00:00:00 2001 From: Thom Lamb Date: Thu, 3 Sep 2026 12:22:43 -0500 Subject: [PATCH 03/17] feat(intake): talk to the rembg sidecar, always naming u2net (#281) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- backend/src/intake/rembgClient.ts | 95 ++++++++++++++++++ backend/tests/unit/rembgClient.test.ts | 134 +++++++++++++++++++++++++ docker-compose.prod.yml | 6 ++ docker-compose.qa.yml | 10 ++ 4 files changed, 245 insertions(+) create mode 100644 backend/src/intake/rembgClient.ts create mode 100644 backend/tests/unit/rembgClient.test.ts diff --git a/backend/src/intake/rembgClient.ts b/backend/src/intake/rembgClient.ts new file mode 100644 index 0000000..e93b79d --- /dev/null +++ b/backend/src/intake/rembgClient.ts @@ -0,0 +1,95 @@ +import { trimTrailingSlashes } from '../utils'; +import { SIGNATURE_BYTES, signatureMatches } from '../uploadTypes'; + +/** + * The one place that talks to the background-removal sidecar. + * + * A sidecar rather than in-process inference: the application runs in a + * container, and putting Python and ONNX into the image would add roughly + * 300 MB to one already over a gigabyte. See + * docs/ops/image-background-removal-stack.md for the measurements. + */ + +/** + * NEVER remove this, and never make it configurable. + * + * The sidecar's default model is `bria-rmbg`, and BRIA's RMBG models are + * licensed for NON-COMMERCIAL use. This is a shop. The default is reached by + * simply not naming a model, so it is a licensing problem that happens + * silently and produces a perfectly good image — there is nothing in the + * output that could reveal it. + * + * `u2net` is Apache-2.0, and also ten times faster (1.1–2.3 s against + * 14–20 s) at a sixth the size, so nothing is being traded away for it. + */ +const MODEL = 'u2net'; + +/** + * Generous on purpose. The sidecar takes about 40 seconds to answer after a + * container start and its first call per model downloads 168 MB, so a tight + * timeout would turn an ordinary cold start into a failure. Nobody is waiting + * on this in the worker's path, and an admin who clicked a button would rather + * wait than be told it did not work. + */ +const TIMEOUT_MS = 120_000; + +/** The configured base URL, or null when there is none. */ +function baseUrl(): string | null { + const raw = process.env.REMBG_URL; + if (raw === undefined || raw.trim() === '') return null; + return trimTrailingSlashes(raw.trim()); +} + +/** + * Whether the feature exists in this environment. + * + * Unconfigured is not a failure. It means the submitter sees no checkbox, the + * admin sees no control and the worker skips the step — an unconfigured + * environment must be a working one, which is the same rule + * `getAnthropicClient` follows by returning null rather than throwing. + */ +export function isRembgConfigured(): boolean { + return baseUrl() !== null; +} + +/** + * The cut-out, as PNG bytes. + * + * Rejects on every failure — unconfigured, unreachable, a non-2xx answer, or a + * body that is not actually a PNG. Every caller catches, and none of them lets + * the rejection reach a submission or a draft. + */ +export async function removeBackground(bytes: Buffer, mediaType: string): Promise { + const base = baseUrl(); + if (base === null) { + throw new Error('REMBG_URL is not set'); + } + + const body = new FormData(); + // A copy through Uint8Array because Buffer is not a BlobPart. The filename is + // a constant: the sidecar does not use it, and passing the stored name would + // put a value from the uploads volume into an outbound request for nothing. + body.append('file', new Blob([new Uint8Array(bytes)], { type: mediaType }), 'photo'); + body.append('model', MODEL); + + const res = await fetch(`${base}/api/remove`, { + method: 'POST', + body, + signal: AbortSignal.timeout(TIMEOUT_MS) + }); + + if (!res.ok) { + throw new Error(`rembg answered ${res.status}`); + } + + const out = Buffer.from(await res.arrayBuffer()); + + // The bytes, not the Content-Type header. A proxy error page served as + // image/png would otherwise be written over a photograph — the same reason + // uploads are checked by signature rather than by what the caller declared. + if (!signatureMatches('image/png', out.subarray(0, SIGNATURE_BYTES))) { + throw new Error('rembg response is not a PNG'); + } + + return out; +} diff --git a/backend/tests/unit/rembgClient.test.ts b/backend/tests/unit/rembgClient.test.ts new file mode 100644 index 0000000..1779d86 --- /dev/null +++ b/backend/tests/unit/rembgClient.test.ts @@ -0,0 +1,134 @@ +import http from 'http'; +import { AddressInfo } from 'net'; +import { isRembgConfigured, removeBackground } from '../../src/intake/rembgClient'; + +/** A real PNG header, so the client's own signature check sees what it expects. */ +const PNG = Buffer.from([0x89, 0x50, 0x4e, 0x47, 0x0d, 0x0a, 0x1a, 0x0a, 0, 0, 0, 0]); +const JPEG = Buffer.from([0xff, 0xd8, 0xff, 0xe0, 0, 0, 0, 0, 0, 0, 0, 0]); + +interface Capture { + url: string; + body: string; +} + +/** + * A stub sidecar on an ephemeral port. + * + * Port 0 rather than a fixed number: several ports in the 55000s are + * Hyper-V-reserved on the development machine and bind with EACCES, and a + * fixed port would also stop this suite running beside itself. + * + * A real HTTP server rather than a mocked `fetch`, because what is being + * checked is the shape of the request that reaches the wire — above all that + * `model=u2net` is in it. + */ +async function withStub( + handler: (req: http.IncomingMessage, res: http.ServerResponse) => void, + run: (capture: Capture) => Promise +): Promise { + const capture: Capture = { url: '', body: '' }; + const server = http.createServer((req, res) => { + capture.url = req.url ?? ''; + const chunks: Buffer[] = []; + req.on('data', (c: Buffer) => chunks.push(c)); + req.on('end', () => { + capture.body = Buffer.concat(chunks).toString('latin1'); + handler(req, res); + }); + }); + + await new Promise((resolve) => server.listen(0, '127.0.0.1', resolve)); + const { port } = server.address() as AddressInfo; + process.env.REMBG_URL = `http://127.0.0.1:${port}`; + + try { + await run(capture); + } finally { + delete process.env.REMBG_URL; + await new Promise((resolve) => server.close(() => resolve())); + } +} + +function respondWithPng(_req: http.IncomingMessage, res: http.ServerResponse): void { + res.writeHead(200, { 'Content-Type': 'image/png' }); + res.end(PNG); +} + +describe('whether the sidecar is configured', () => { + it('is false when REMBG_URL is unset', () => { + delete process.env.REMBG_URL; + expect(isRembgConfigured()).toBe(false); + }); + + // A variable set to spaces is a configuration mistake, not a value — the + // same reading envValidation applies everywhere else. + it('is false when REMBG_URL is blank', () => { + process.env.REMBG_URL = ' '; + expect(isRembgConfigured()).toBe(false); + delete process.env.REMBG_URL; + }); + + it('is true when REMBG_URL is set', () => { + process.env.REMBG_URL = 'http://rembg-syn:7000'; + expect(isRembgConfigured()).toBe(true); + delete process.env.REMBG_URL; + }); +}); + +describe('asking the sidecar to remove a background', () => { + /** + * The single most important assertion in this file. + * + * The image's default model is `bria-rmbg`, licensed non-commercial, and it + * is selected by simply not naming a model. Nothing about the returned image + * would reveal it had been used, so this is the only place it can be caught. + */ + it('names u2net explicitly, because the default is licensed non-commercial', async () => { + await withStub(respondWithPng, async (capture) => { + await removeBackground(JPEG, 'image/jpeg'); + expect(capture.body).toContain('name="model"'); + expect(capture.body).toContain('u2net'); + }); + }); + + it('posts the file to /api/remove and returns the PNG it gets back', async () => { + await withStub(respondWithPng, async (capture) => { + const out = await removeBackground(JPEG, 'image/jpeg'); + expect(capture.url).toBe('/api/remove'); + expect(capture.body).toContain('name="file"'); + expect(out.subarray(0, 8)).toEqual(PNG.subarray(0, 8)); + }); + }); + + it('rejects rather than returning bytes when the sidecar errors', async () => { + await withStub( + (_req, res) => { + res.writeHead(500); + res.end('boom'); + }, + async () => { + await expect(removeBackground(JPEG, 'image/jpeg')).rejects.toThrow(/500/); + } + ); + }); + + // The failure that would otherwise write an HTML error page over a + // photograph. Checked with the same magic-byte helper the upload path uses, + // rather than by trusting the Content-Type the sidecar sent. + it('rejects a response that is not actually a PNG', async () => { + await withStub( + (_req, res) => { + res.writeHead(200, { 'Content-Type': 'image/png' }); + res.end('not an image'); + }, + async () => { + await expect(removeBackground(JPEG, 'image/jpeg')).rejects.toThrow(/not a PNG/); + } + ); + }); + + it('rejects when it is not configured at all', async () => { + delete process.env.REMBG_URL; + await expect(removeBackground(JPEG, 'image/jpeg')).rejects.toThrow(/REMBG_URL/); + }); +}); diff --git a/docker-compose.prod.yml b/docker-compose.prod.yml index 9a21546..17b3857 100644 --- a/docker-compose.prod.yml +++ b/docker-compose.prod.yml @@ -98,6 +98,8 @@ # INTAKE_ACTION_SECRET Optional. Signs the regenerate and discard links in the # intake notification email (#224). Absent, the email # still sends and carries no shortcuts. +# REMBG_URL Optional. The background-removal sidecar, e.g. +# http://rembg-syn:7000. Unset turns the feature off. # undrafted, which is a working configuration for the # same reason USPS is. The one credential here that # spends money per call, and on a path anybody holding @@ -238,6 +240,10 @@ services: # keys need no workspace. - ANTHROPIC_WORKSPACE_ID=${ANTHROPIC_WORKSPACE_ID:-} + # Optional. The background-removal sidecar (#281). + # See docs/ops/image-background-removal-stack.md. + - REMBG_URL=${REMBG_URL:-} + # Signs the regenerate and discard links in the intake notification email # (#224). Optional: absent, the notification still sends and links to the # review queue without shortcuts. Rotating it revokes every outstanding diff --git a/docker-compose.qa.yml b/docker-compose.qa.yml index f797512..5e084a5 100644 --- a/docker-compose.qa.yml +++ b/docker-compose.qa.yml @@ -63,6 +63,10 @@ # in the notification email (#224). Absent, the email still # sends and simply carries no shortcuts. Its own value, not # production's: a link signed with it acts without a login. +# QA_REMBG_URL — optional. The background-removal sidecar, e.g. +# http://rembg-syn:7000. Unset turns the feature off rather +# than breaking anything. The sidecar must be on the same +# network as this stack. services: redefined-designs-qa: @@ -164,6 +168,12 @@ services: # would turn the ordinary case into a different error. - ANTHROPIC_WORKSPACE_ID=${QA_ANTHROPIC_WORKSPACE_ID:-} + # Optional. The background-removal sidecar (#281). Unset means the + # feature does not exist: no checkbox on the submission page, no control + # in the review queue, and the worker skips the step. Empty default so an + # unset stack variable cannot fail a deploy. + - REMBG_URL=${QA_REMBG_URL:-} + # Signs the regenerate and discard links in the intake notification email # (#224). Optional: absent, the notification still sends and simply links # to the review queue without shortcuts. Anyone holding a link can act on -- 2.54.0 From 8d12cb2f2da69a627c1e3ecff45bcdcce04e1985 Mon Sep 17 00:00:00 2001 From: Thom Lamb Date: Thu, 3 Sep 2026 12:30:09 -0500 Subject: [PATCH 04/17] feat(intake): swap a photo for a cut-out, keeping the original (#281) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- backend/src/intake/backgroundRemoval.ts | 139 +++++++++++++ .../backgroundRemoval.integration.test.ts | 187 ++++++++++++++++++ backend/tests/unit/backgroundRemoval.test.ts | 22 +++ 3 files changed, 348 insertions(+) create mode 100644 backend/src/intake/backgroundRemoval.ts create mode 100644 backend/tests/unit/backgroundRemoval.test.ts diff --git a/backend/src/intake/backgroundRemoval.ts b/backend/src/intake/backgroundRemoval.ts new file mode 100644 index 0000000..23eb313 --- /dev/null +++ b/backend/src/intake/backgroundRemoval.ts @@ -0,0 +1,139 @@ +import { promises as fs } from 'fs'; +import path from 'path'; +import { pool } from '../db'; +import { typeForExtension } from '../uploadTypes'; +import { isRembgConfigured, removeBackground } from './rembgClient'; + +/** + * Swapping a photo for a cut-out of itself, and swapping it back. + * + * One module rather than two, because the worker's path and the admin's path + * must produce identical results: a cut-out obtained either way has to be + * undoable the same way. A near-copy that drifted would mean a photo the + * Restore button could not restore. + * + * Nothing here deletes anything. The original file stays on disk and so does + * every cut-out ever made, because the submitter's photos are often the only + * copy of an item no longer in their hands — the same rule Discard follows in + * the review queue. + */ + +interface ImageRow { + image_path: string; + original_image_path: string | null; +} + +/** + * The path a cut-out of `imagePath` is written to. + * + * Pure, so the naming rule can be checked without a database or a sidecar. + * Always `.png` because the result is transparent, and the storefront's dark + * theme would show a flat white background as a bright box behind every + * product. + */ +export function cutoutPathFor(imagePath: string): string { + const base = path.basename(imagePath, path.extname(imagePath)); + return `/uploads/${base}-cutout.png`; +} + +function uploadsDir(): string { + return process.env.UPLOADS_DIR ?? ''; +} + +/** + * Replaces one image with a cut-out, keeping the original. + * + * Idempotent by way of the `original_image_path IS NOT NULL` check rather than + * a separate flag. That guard is load-bearing twice over: it makes a repeat + * call a no-op, and it stops a second pass from recording the *cut-out* as the + * original and losing the real one for good. + * + * Throws on every failure. Nothing is written to the row unless the file is + * already on disk, so a caller that catches and moves on leaves the photo + * exactly as it was. + */ +export async function removeImageBackground(imageId: number): Promise { + const { rows } = await pool.query( + `SELECT image_path, original_image_path FROM item_images WHERE id = $1`, + [imageId] + ); + const row = rows[0]; + if (!row) { + throw new Error(`no image ${imageId}`); + } + if (row.original_image_path !== null) { + // Already cut out. Doing it again would overwrite the record of where the + // real original went. + return; + } + + // basename only: image_path is stored as '/uploads/' and the directory + // it lives in is a server constant. Same rule readPhotos follows in the + // drafting worker. + const sourceName = path.basename(row.image_path); + const mediaType = typeForExtension(path.extname(sourceName)); + if (mediaType === null) { + throw new Error(`cannot read ${sourceName}: unrecognised extension`); + } + + const cutout = await removeBackground( + await fs.readFile(path.join(uploadsDir(), sourceName)), + mediaType + ); + + const cutoutPath = cutoutPathFor(row.image_path); + await fs.writeFile(path.join(uploadsDir(), path.basename(cutoutPath)), cutout); + + // The row is pointed at the new file only after the file exists. The other + // order would leave a window in which the storefront rendered a broken image. + // + // `original_image_path = image_path` reads the pre-update value, which is how + // Postgres evaluates an UPDATE's right-hand side — so this records where the + // photo came from in the same statement that moves it. + await pool.query( + `UPDATE item_images + SET image_path = $2, original_image_path = image_path + WHERE id = $1 AND original_image_path IS NULL`, + [imageId, cutoutPath] + ); +} + +/** + * Puts the original back. + * + * The cut-out file is left on disk deliberately. Removing a background is + * exactly the operation that produces an occasional bad result on an unusual + * object, so somebody restoring one is quite likely to try again — and this + * module deletes nothing in any case. + */ +export async function restoreImageOriginal(imageId: number): Promise { + const { rowCount } = await pool.query( + `UPDATE item_images + SET image_path = original_image_path, original_image_path = NULL + WHERE id = $1 AND original_image_path IS NOT NULL`, + [imageId] + ); + if (rowCount === 0) { + throw new Error(`image ${imageId} has no original to restore`); + } +} + +/** + * Every photo of one item, in order. + * + * Sequential rather than parallel: the sidecar is assumed to handle one + * request at a time, and the worker it runs inside is not in a hurry. A + * failure on one photo stops the rest, and the caller logs it — the item keeps + * whatever was already done, and nothing is left half-written. + */ +export async function removeBackgroundsForItem(itemId: number): Promise { + if (!isRembgConfigured()) return; + + const { rows } = await pool.query<{ id: number }>( + `SELECT id FROM item_images WHERE item_id = $1 ORDER BY sort_order`, + [itemId] + ); + for (const row of rows) { + await removeImageBackground(row.id); + } +} diff --git a/backend/tests/integration/backgroundRemoval.integration.test.ts b/backend/tests/integration/backgroundRemoval.integration.test.ts index 4233c07..0832617 100644 --- a/backend/tests/integration/backgroundRemoval.integration.test.ts +++ b/backend/tests/integration/backgroundRemoval.integration.test.ts @@ -1,5 +1,11 @@ +import http from 'http'; +import { AddressInfo } from 'net'; +import { promises as fsp } from 'fs'; +import os from 'os'; +import path from 'path'; import { pool } from '../../src/db'; import { resetDb, closeDb } from './setup/testDb'; +import { removeImageBackground, restoreImageOriginal } from '../../src/intake/backgroundRemoval'; beforeEach(async () => { await resetDb(); @@ -52,3 +58,184 @@ describe('the background-removal columns', () => { expect(rows[0]?.original_image_path).toBeNull(); }); }); + +const PNG_BYTES = Buffer.from([0x89, 0x50, 0x4e, 0x47, 0x0d, 0x0a, 0x1a, 0x0a, 0, 0, 0, 0]); +const JPEG_BYTES = Buffer.from([0xff, 0xd8, 0xff, 0xe0, 0, 0, 0, 0, 0, 0, 0, 0]); + +interface ImagePaths { + image_path: string; + original_image_path: string | null; +} + +let uploads = ''; +let stub: http.Server | null = null; + +/** + * A real uploads directory and a stub sidecar. + * + * A temporary directory rather than the configured one, because these tests + * write files and a suite that leaves rubbish in a developer's uploads volume + * is a suite people stop running. + * + * No test here contacts the real sidecar. It takes forty seconds to start, and + * a suite that depends on that is broken by construction. + */ +async function startStub( + handler: (req: http.IncomingMessage, res: http.ServerResponse) => void +): Promise { + uploads = await fsp.mkdtemp(path.join(os.tmpdir(), 'bgremoval-')); + process.env.UPLOADS_DIR = uploads; + + stub = http.createServer((req, res) => { + // Drain the body before answering, or the client sees a reset rather than + // the status this test is about. + req.on('data', () => undefined); + req.on('end', () => handler(req, res)); + }); + await new Promise((resolve) => stub!.listen(0, '127.0.0.1', resolve)); + process.env.REMBG_URL = `http://127.0.0.1:${(stub!.address() as AddressInfo).port}`; +} + +function answerWithPng(_req: http.IncomingMessage, res: http.ServerResponse): void { + res.writeHead(200, { 'Content-Type': 'image/png' }); + res.end(PNG_BYTES); +} + +afterEach(async () => { + delete process.env.REMBG_URL; + if (stub) { + await new Promise((resolve) => stub!.close(() => resolve())); + stub = null; + } +}); + +async function seedWithFile(): Promise<{ itemId: number; imageId: number; name: string }> { + const name = 'original.jpg'; + const seeded = await seedSubmission(`/uploads/${name}`); + await fsp.writeFile(path.join(uploads, name), JPEG_BYTES); + return { ...seeded, name }; +} + +async function pathsOf(imageId: number): Promise { + const { rows } = await pool.query( + `SELECT image_path, original_image_path FROM item_images WHERE id = $1`, + [imageId] + ); + return rows[0]!; +} + +describe('removing one photo’s background', () => { + it('points the row at the cut-out and records where the original went', async () => { + await startStub(answerWithPng); + const { imageId } = await seedWithFile(); + + await removeImageBackground(imageId); + + const paths = await pathsOf(imageId); + expect(paths.image_path).toBe('/uploads/original-cutout.png'); + expect(paths.original_image_path).toBe('/uploads/original.jpg'); + }); + + // The original is the only copy of an item that may no longer be in the + // sender's hands. Nothing in this module is allowed to remove it. + it('leaves the original file on disk', async () => { + await startStub(answerWithPng); + const { imageId, name } = await seedWithFile(); + + await removeImageBackground(imageId); + + await expect(fsp.access(path.join(uploads, name))).resolves.toBeUndefined(); + }); + + it('writes the cut-out where the row now says it is', async () => { + await startStub(answerWithPng); + const { imageId } = await seedWithFile(); + + await removeImageBackground(imageId); + + await expect(fsp.readFile(path.join(uploads, 'original-cutout.png'))).resolves.toEqual( + PNG_BYTES + ); + }); + + // The guard that stops a second pass recording the cut-out as the original + // and losing the real one for good. + it('is a no-op on a photo that has already been cut out', async () => { + await startStub(answerWithPng); + const { imageId } = await seedWithFile(); + + await removeImageBackground(imageId); + await removeImageBackground(imageId); + + expect((await pathsOf(imageId)).original_image_path).toBe('/uploads/original.jpg'); + }); +}); + +describe('when the sidecar will not answer', () => { + it('leaves the row untouched on a 500', async () => { + await startStub((_req, res) => { + res.writeHead(500); + res.end('boom'); + }); + const { imageId } = await seedWithFile(); + + await expect(removeImageBackground(imageId)).rejects.toThrow(/500/); + + const paths = await pathsOf(imageId); + expect(paths.image_path).toBe('/uploads/original.jpg'); + expect(paths.original_image_path).toBeNull(); + }); + + it('leaves the row untouched when it returns something that is not an image', async () => { + await startStub((_req, res) => { + res.writeHead(200, { 'Content-Type': 'image/png' }); + res.end('gateway error'); + }); + const { imageId } = await seedWithFile(); + + await expect(removeImageBackground(imageId)).rejects.toThrow(/not a PNG/); + + const paths = await pathsOf(imageId); + expect(paths.image_path).toBe('/uploads/original.jpg'); + expect(paths.original_image_path).toBeNull(); + }); + + it('leaves the row untouched when it is unreachable', async () => { + await startStub(answerWithPng); + // Closed before the call, so the connection is refused rather than hung. + // The URL stays set, which is the case worth modelling: configured, and + // not there. + await new Promise((resolve) => stub!.close(() => resolve())); + stub = null; + const { imageId } = await seedWithFile(); + + await expect(removeImageBackground(imageId)).rejects.toThrow(); + + const paths = await pathsOf(imageId); + expect(paths.image_path).toBe('/uploads/original.jpg'); + expect(paths.original_image_path).toBeNull(); + }); +}); + +describe('putting the original back', () => { + it('swaps the paths back and clears the record', async () => { + await startStub(answerWithPng); + const { imageId } = await seedWithFile(); + await removeImageBackground(imageId); + + await restoreImageOriginal(imageId); + + const paths = await pathsOf(imageId); + expect(paths.image_path).toBe('/uploads/original.jpg'); + expect(paths.original_image_path).toBeNull(); + }); + + it('refuses a photo that was never cut out, rather than blanking its path', async () => { + await startStub(answerWithPng); + const { imageId } = await seedWithFile(); + + await expect(restoreImageOriginal(imageId)).rejects.toThrow(/no original/); + + expect((await pathsOf(imageId)).image_path).toBe('/uploads/original.jpg'); + }); +}); diff --git a/backend/tests/unit/backgroundRemoval.test.ts b/backend/tests/unit/backgroundRemoval.test.ts new file mode 100644 index 0000000..23bf41b --- /dev/null +++ b/backend/tests/unit/backgroundRemoval.test.ts @@ -0,0 +1,22 @@ +import { cutoutPathFor } from '../../src/intake/backgroundRemoval'; + +describe('where a cut-out is written', () => { + // A new file rather than a rewrite of the original, which is what makes the + // original restorable at all — and what makes the JPEG-to-PNG change free, + // since no existing path is renamed. + it('sits beside the original with a -cutout suffix and a .png extension', () => { + expect(cutoutPathFor('/uploads/abc-123.jpg')).toBe('/uploads/abc-123-cutout.png'); + }); + + // image_path is a contract, not a string: #103 made the stored value the + // path uploadUrl joins an origin onto. + it('keeps the /uploads/ prefix', () => { + expect(cutoutPathFor('/uploads/x.webp')).toBe('/uploads/x-cutout.png'); + }); + + // The extension is replaced rather than appended, so a second pass cannot + // produce `.png.png`. + it('replaces the extension rather than appending to it', () => { + expect(cutoutPathFor('/uploads/x.png')).toBe('/uploads/x-cutout.png'); + }); +}); -- 2.54.0 From 81524a28495d5060a6218de9e6db882f797c24ee Mon Sep 17 00:00:00 2001 From: Thom Lamb Date: Thu, 3 Sep 2026 12:38:49 -0500 Subject: [PATCH 05/17] feat(intake): cut out backgrounds in the worker, never in the upload (#281) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- backend/src/intake/draftingWorker.ts | 27 +++- ...ftingBackgroundRemoval.integration.test.ts | 153 ++++++++++++++++++ 2 files changed, 179 insertions(+), 1 deletion(-) create mode 100644 backend/tests/integration/draftingBackgroundRemoval.integration.test.ts diff --git a/backend/src/intake/draftingWorker.ts b/backend/src/intake/draftingWorker.ts index 6df031b..670dcec 100644 --- a/backend/src/intake/draftingWorker.ts +++ b/backend/src/intake/draftingWorker.ts @@ -7,6 +7,7 @@ import { getAnthropicClient } from './anthropicClient'; import { draftListing } from './draftListing'; import { applyDraft } from './applyDraft'; import { notifyDraftReady } from './notifyDraft'; +import { removeBackgroundsForItem } from './backgroundRemoval'; /** * Turns queued submissions into drafts. @@ -19,6 +20,13 @@ import { notifyDraftReady } from './notifyDraft'; * The photos are often the only copy of an item no longer in the sender's * hands, so every failure below leaves the row and its images intact and merely * undrafted. Nothing in this file deletes anything. + * + * Background removal (#281) follows drafting rather than running on its own + * pass. That couples the two: an environment with no ANTHROPIC_API_KEY drafts + * nothing, so it cuts out nothing either. That is the intended trade — a + * separate pass would re-attempt an unreachable sidecar on every sweep for a + * row that is going to sit at 'queued' indefinitely — and the admin's per-photo + * control in the review queue is the way to do it by hand meanwhile. */ /** Three tries, then it waits for a person rather than burning money on a loop. */ @@ -32,6 +40,7 @@ type Photo = { mediaType: string; base64: string }; interface QueuedRow { item_id: number; submitter_note: string | null; + remove_background: boolean; } export interface SweepResult { @@ -119,7 +128,7 @@ async function draftOne( export async function draftQueued(limit = DEFAULT_BATCH): Promise { const { rows } = await pool.query( - `SELECT item_id, submitter_note FROM item_drafts + `SELECT item_id, submitter_note, remove_background FROM item_drafts WHERE state = 'queued' AND attempts < $2 ORDER BY created_at LIMIT $1`, @@ -146,6 +155,22 @@ export async function draftQueued(limit = DEFAULT_BATCH): Promise { await draftOne(client, row.item_id, row.submitter_note, photos); drafted++; + // Deliberately after the draft is committed, and catching for itself. + // + // This is the sender's tick from the submission page, honoured here so + // they never waited for it — and a failure must not mark a draft that was + // written correctly as failed. The photo keeps its original in that case, + // and the admin's per-photo control is still there to do it by hand. + // + // Awaited, unlike the notification below, so a sweep that has returned + // has finished its work. Nothing is waiting on this: the worker is off + // the request path, which is the whole reason drafting lives here. + if (row.remove_background) { + await removeBackgroundsForItem(row.item_id).catch((err) => + console.error(`[drafting] background removal for item ${row.item_id}:`, err) + ); + } + // Fire and forget, and deliberately after the draft is committed. A mail // failure must never mark a draft that was written correctly as failed — // the queue is what the admin actually works from, and the email is a diff --git a/backend/tests/integration/draftingBackgroundRemoval.integration.test.ts b/backend/tests/integration/draftingBackgroundRemoval.integration.test.ts new file mode 100644 index 0000000..c4ae9d6 --- /dev/null +++ b/backend/tests/integration/draftingBackgroundRemoval.integration.test.ts @@ -0,0 +1,153 @@ +import http from 'http'; +import { AddressInfo } from 'net'; +import { promises as fsp } from 'fs'; +import os from 'os'; +import path from 'path'; +import { pool } from '../../src/db'; +import { resetDb, closeDb } from './setup/testDb'; +import { draftQueued } from '../../src/intake/draftingWorker'; +import { resetAnthropicClient } from '../../src/intake/anthropicClient'; +import { draftListing } from '../../src/intake/draftListing'; + +/** + * The worker's background-removal step (#281). + * + * The model is mocked rather than reached. What is under test is what the + * worker does *after* a draft is written — which of the two paths it takes, + * and what survives when the sidecar does not answer — and none of that + * depends on what the model said. + */ +jest.mock('../../src/intake/draftListing'); + +const draftListingMock = draftListing as jest.MockedFunction; + +const JPEG_BYTES = Buffer.from([0xff, 0xd8, 0xff, 0xe0, 0, 0, 0, 0, 0, 0, 0, 0]); +const PNG_BYTES = Buffer.from([0x89, 0x50, 0x4e, 0x47, 0x0d, 0x0a, 0x1a, 0x0a, 0, 0, 0, 0]); + +let uploads = ''; +let stub: http.Server | null = null; + +beforeEach(async () => { + await resetDb(); + + draftListingMock.mockResolvedValue({ + draft: { + name: 'Blue stoneware vase', + description: 'Hand-thrown, chipped base.', + category: null, + tags: [], + suggestedPriceCents: 4500 + }, + model: 'claude-sonnet-5', + inputTokens: 1000, + outputTokens: 200, + costMicros: 4000 + }); + + // Non-empty is all that is needed: getAnthropicClient only has to return + // something other than null, and the mock above is what answers. + process.env.ANTHROPIC_API_KEY = 'sk-ant-not-used'; + resetAnthropicClient(); + + uploads = await fsp.mkdtemp(path.join(os.tmpdir(), 'workerbg-')); + process.env.UPLOADS_DIR = uploads; + + stub = http.createServer((req, res) => { + req.on('data', () => undefined); + req.on('end', () => { + res.writeHead(200, { 'Content-Type': 'image/png' }); + res.end(PNG_BYTES); + }); + }); + await new Promise((resolve) => stub!.listen(0, '127.0.0.1', resolve)); + process.env.REMBG_URL = `http://127.0.0.1:${(stub!.address() as AddressInfo).port}`; +}); + +afterEach(async () => { + delete process.env.REMBG_URL; + delete process.env.ANTHROPIC_API_KEY; + resetAnthropicClient(); + if (stub) { + await new Promise((resolve) => stub!.close(() => resolve())); + stub = null; + } +}); + +afterAll(async () => { + await pool.end(); + await closeDb(); +}); + +/** A queued submission with one real file on disk and the intent set. */ +async function seedSubmissionWithPhoto(options: { removeBackground: boolean }): Promise { + const { rows } = await pool.query<{ id: number }>( + `INSERT INTO items (name, status) VALUES ('Submission placeholder', 'pending') RETURNING id` + ); + const itemId = rows[0]!.id; + await pool.query( + `INSERT INTO item_drafts (item_id, submitter_note, remove_background) + VALUES ($1, 'a note', $2)`, + [itemId, options.removeBackground] + ); + await pool.query( + `INSERT INTO item_images (item_id, image_path, sort_order) + VALUES ($1, '/uploads/worker.jpg', 0)`, + [itemId] + ); + await fsp.writeFile(path.join(uploads, 'worker.jpg'), JPEG_BYTES); + return itemId; +} + +async function originalPathOf(itemId: number): Promise { + const { rows } = await pool.query<{ original_image_path: string | null }>( + `SELECT original_image_path FROM item_images WHERE item_id = $1`, + [itemId] + ); + return rows[0]?.original_image_path ?? null; +} + +describe('background removal after a draft', () => { + // The mock has to actually be in play, or the two cases below would both + // pass for the wrong reason — a draft that never happened cuts nothing out. + it('drafts successfully, which is what the removal step follows', async () => { + await seedSubmissionWithPhoto({ removeBackground: false }); + + expect(await draftQueued(1)).toEqual({ drafted: 1, failed: 0, skipped: 0 }); + }); + + // Recorded at submission and acted on here, so the sender never waits and a + // sidecar that is down cannot fail their upload. + it('cuts out the photos when the submitter asked for it', async () => { + const itemId = await seedSubmissionWithPhoto({ removeBackground: true }); + + await draftQueued(1); + + expect(await originalPathOf(itemId)).toBe('/uploads/worker.jpg'); + }); + + it('leaves the photos alone when they did not', async () => { + const itemId = await seedSubmissionWithPhoto({ removeBackground: false }); + + await draftQueued(1); + + expect(await originalPathOf(itemId)).toBeNull(); + }); + + // The governing rule: removal is a convenience on top of a draft that was + // written correctly. A sidecar failure must never turn a good draft into a + // failed one, because the queue is what the admin actually works from. + it('leaves the draft ready when the sidecar fails', async () => { + const itemId = await seedSubmissionWithPhoto({ removeBackground: true }); + // Configured, and nothing listening on it. + process.env.REMBG_URL = 'http://127.0.0.1:1'; + + await draftQueued(1); + + const { rows } = await pool.query<{ state: string }>( + `SELECT state FROM item_drafts WHERE item_id = $1`, + [itemId] + ); + expect(rows[0]?.state).toBe('ready'); + expect(await originalPathOf(itemId)).toBeNull(); + }); +}); -- 2.54.0 From 885a78c57255f2aa340fa08d5a813d877f719bc8 Mon Sep 17 00:00:00 2001 From: Thom Lamb Date: Thu, 3 Sep 2026 12:44:20 -0500 Subject: [PATCH 06/17] feat(intake): record whether the submitter asked for a cut-out (#281) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- backend/src/routes/intake.ts | 21 ++++-- .../integration/intake.integration.test.ts | 66 +++++++++++++++++++ 2 files changed, 82 insertions(+), 5 deletions(-) diff --git a/backend/src/routes/intake.ts b/backend/src/routes/intake.ts index e7dda43..372337c 100644 --- a/backend/src/routes/intake.ts +++ b/backend/src/routes/intake.ts @@ -8,6 +8,7 @@ import { draftQueued } from '../intake/draftingWorker'; import { checkCapacity, countForLinkSince, windowStart } from '../intake/capacity'; import { alertCeilingReached, alertLinkThreshold } from '../intake/abuseAlert'; import { getSettings } from '../adminSettings'; +import { isRembgConfigured } from '../intake/rembgClient'; const router = Router(); @@ -128,8 +129,9 @@ router.get('/:token', intakeViewLimiter, asyncRoute(async (req: Request, res: Re if (!link) { return res.status(404).json({ error: 'not found' }); } - // The label only. Nothing about the catalogue, the admin, or other links. - res.json({ label: link.label }); + // The label, and whether the background-removal control has anything behind + // it. Still nothing about the catalogue, the admin, or other links. + res.json({ label: link.label, backgroundRemoval: isRembgConfigured() }); })); router.post( @@ -159,6 +161,15 @@ router.post( const note = typeof req.body?.note === 'string' ? req.body.note.trim() : ''; + // Absent means yes: the checkbox on the page is ticked by default, so a + // client that does not send the field — an older build, or a script — gets + // what every other submission gets rather than silently opting out. + // + // Only the exact string opts out. Multipart fields arrive as strings, and + // reading a stray value as "no" would quietly deny somebody something they + // asked for. + const removeBackground = req.body?.removeBackground !== 'false'; + const client = await pool.connect(); try { await client.query('BEGIN'); @@ -178,9 +189,9 @@ router.post( await insertItemImages(client, itemId, files, 0); await client.query( - `INSERT INTO item_drafts (item_id, upload_link_id, submitter_note) - VALUES ($1, $2, $3)`, - [itemId, link.id, note === '' ? null : note] + `INSERT INTO item_drafts (item_id, upload_link_id, submitter_note, remove_background) + VALUES ($1, $2, $3, $4)`, + [itemId, link.id, note === '' ? null : note, removeBackground] ); // Counted inside the transaction and guarded on the same conditions as diff --git a/backend/tests/integration/intake.integration.test.ts b/backend/tests/integration/intake.integration.test.ts index 044b877..bc212f4 100644 --- a/backend/tests/integration/intake.integration.test.ts +++ b/backend/tests/integration/intake.integration.test.ts @@ -39,6 +39,13 @@ async function issueLink(label = 'Sarah', maxSubmissions?: number | null): Promi return res.body.token as string; } +/** A submission with optional extra multipart fields beside the photo. */ +function postPhoto(token: string, fields: Record = {}) { + const req = request(app).post(`/api/intake/${token}`); + for (const [name, value] of Object.entries(fields)) void req.field(name, value); + return req.attach('images', PNG, 'a.png'); +} + describe('checking a link before showing the form', () => { it('names the link so the page can greet the sender', async () => { const token = await issueLink('Sarah'); @@ -207,3 +214,62 @@ describe('a submitted item does not reach the storefront', () => { expect(res.body).toHaveLength(0); }); }); + +describe('the background-removal intent', () => { + // Ticked by default on the page, so absent means yes. An older client or a + // curl call then behaves like the current default rather than silently + // opting out of something every other submission gets. + it('defaults to true when the field is not sent', async () => { + const token = await issueLink(); + await postPhoto(token); + + const { rows } = await pool.query<{ remove_background: boolean }>( + `SELECT remove_background FROM item_drafts ORDER BY id DESC LIMIT 1` + ); + expect(rows[0]?.remove_background).toBe(true); + }); + + it('records a submitter who unticked it', async () => { + const token = await issueLink(); + await postPhoto(token, { removeBackground: 'false' }); + + const { rows } = await pool.query<{ remove_background: boolean }>( + `SELECT remove_background FROM item_drafts ORDER BY id DESC LIMIT 1` + ); + expect(rows[0]?.remove_background).toBe(false); + }); + + // Only the exact string opts out. A stray value is not a considered "no", + // and reading it as one would quietly deny somebody something they asked for. + it('treats anything other than "false" as consent', async () => { + const token = await issueLink(); + await postPhoto(token, { removeBackground: 'no' }); + + const { rows } = await pool.query<{ remove_background: boolean }>( + `SELECT remove_background FROM item_drafts ORDER BY id DESC LIMIT 1` + ); + expect(rows[0]?.remove_background).toBe(true); + }); +}); + +describe('what the submission page is told', () => { + it('says the feature is off when there is no sidecar', async () => { + delete process.env.REMBG_URL; + const token = await issueLink(); + + const res = await request(app).get(`/api/intake/${token}`); + + expect(res.status).toBe(200); + expect(res.body.backgroundRemoval).toBe(false); + }); + + it('says it is on when there is one', async () => { + process.env.REMBG_URL = 'http://rembg-syn:7000'; + const token = await issueLink(); + + const res = await request(app).get(`/api/intake/${token}`); + + expect(res.body.backgroundRemoval).toBe(true); + delete process.env.REMBG_URL; + }); +}); -- 2.54.0 From 9ced34ad194c0481ba76e6427b58ea472c8fe1e5 Mon Sep 17 00:00:00 2001 From: Thom Lamb Date: Thu, 3 Sep 2026 12:49:38 -0500 Subject: [PATCH 07/17] feat(admin): remove and restore a photo's background per image (#281) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- backend/src/routes/adminItemDrafts.ts | 89 ++++++++++- .../adminItemDrafts.integration.test.ts | 146 ++++++++++++++++++ 2 files changed, 233 insertions(+), 2 deletions(-) diff --git a/backend/src/routes/adminItemDrafts.ts b/backend/src/routes/adminItemDrafts.ts index 4954413..cf4123a 100644 --- a/backend/src/routes/adminItemDrafts.ts +++ b/backend/src/routes/adminItemDrafts.ts @@ -3,6 +3,8 @@ import { pool } from '../db'; import { asyncRoute } from '../asyncRoute'; import { draftQueued } from '../intake/draftingWorker'; import { nextPriceSource, PriceSource } from '../intake/priceSource'; +import { removeImageBackground, restoreImageOriginal } from '../intake/backgroundRemoval'; +import { isRembgConfigured } from '../intake/rembgClient'; const router = Router(); @@ -27,7 +29,10 @@ const DRAFT_SELECT = ` i.price_cents, i.status, l.label AS upload_link_label, COALESCE(( - SELECT json_agg(json_build_object('id', img.id, 'image_path', img.image_path) + SELECT json_agg(json_build_object( + 'id', img.id, + 'image_path', img.image_path, + 'original_image_path', img.original_image_path) ORDER BY img.sort_order) FROM item_images img WHERE img.item_id = d.item_id ), '[]'::json) AS images @@ -52,7 +57,9 @@ router.get( ? await pool.query(`${DRAFT_SELECT} WHERE d.state = $1 ORDER BY d.created_at DESC`, [state]) : await pool.query(`${DRAFT_SELECT} WHERE d.state <> 'discarded' ORDER BY d.created_at DESC`); - res.json({ drafts: rows }); + // Whether the control has anything behind it, alongside the rows. A second + // endpoint for one boolean would be a round trip the queue already makes. + res.json({ drafts: rows, backgroundRemoval: isRembgConfigured() }); }) ); @@ -220,4 +227,82 @@ router.post( }) ); +/** + * One photo's current paths, if it belongs to this item. + * + * Scoped by item as well as by image so an image id from a different + * submission cannot be acted on through this item's URL — the id is a serial, + * so guessing one is not hard. + */ +async function imageOfItem( + itemId: string, + imageId: string +): Promise<{ image_path: string; original_image_path: string | null } | null> { + const { rows } = await pool.query<{ image_path: string; original_image_path: string | null }>( + `SELECT image_path, original_image_path + FROM item_images WHERE id = $1 AND item_id = $2`, + [imageId, itemId] + ); + return rows[0] ?? null; +} + +/** + * Remove the background from one photo. + * + * The other half of the submitter's checkbox: for the photos nobody ticked it + * for, and for the ones where the worker could not reach the sidecar. Both go + * through the same module, so a cut-out obtained either way is identical and + * either can be undone by Restore. + * + * Synchronous, unlike the worker's path. A warm request measures 1.1–2.3 s and + * this is an admin who just clicked a button and is watching for the result. + * The reason drafting was moved off the request path — that a stranger can + * trigger it and must never wait — does not apply behind the admin gate. + */ +router.post( + '/:itemId/images/:imageId/remove-background', + asyncRoute(async (req: Request, res: Response) => { + const { itemId = '', imageId = '' } = req.params; + if ((await imageOfItem(itemId, imageId)) === null) { + return res.status(404).json({ error: 'no such photo on this item' }); + } + + try { + await removeImageBackground(Number(imageId)); + } catch (err) { + // 502, not 500. The request was fine and so is this app — the service it + // depends on did not answer. The message says the photo is unchanged, + // because that is the thing the admin actually needs to know. + console.error(`[drafts] background removal for image ${imageId}:`, err); + return res + .status(502) + .json({ error: 'the background-removal service did not answer — the photo is unchanged' }); + } + + res.json(await imageOfItem(itemId, imageId)); + }) +); + +/** + * Put the original photo back. + * + * The reason a cut-out is safe to try at all. Background removal produces the + * occasional poor result on an unusual object, and this makes that survivable + * rather than something to prevent. Nothing is deleted: the cut-out file stays + * on disk, because somebody restoring one is quite likely to try again. + */ +router.post( + '/:itemId/images/:imageId/restore-original', + asyncRoute(async (req: Request, res: Response) => { + const { itemId = '', imageId = '' } = req.params; + const existing = await imageOfItem(itemId, imageId); + if (existing === null || existing.original_image_path === null) { + return res.status(404).json({ error: 'this photo has no original to restore' }); + } + + await restoreImageOriginal(Number(imageId)); + res.json(await imageOfItem(itemId, imageId)); + }) +); + export default router; diff --git a/backend/tests/integration/adminItemDrafts.integration.test.ts b/backend/tests/integration/adminItemDrafts.integration.test.ts index 3c76836..89891f5 100644 --- a/backend/tests/integration/adminItemDrafts.integration.test.ts +++ b/backend/tests/integration/adminItemDrafts.integration.test.ts @@ -2,6 +2,11 @@ import request from 'supertest'; import app from '../../src/app'; import { pool } from '../../src/db'; import { resetDb, closeDb } from './setup/testDb'; +import http from 'http'; +import { AddressInfo } from 'net'; +import { promises as fsp } from 'fs'; +import os from 'os'; +import path from 'path'; beforeEach(async () => { await resetDb(); @@ -304,3 +309,144 @@ describe('the other three actions', () => { } }); }); + +describe('the review queue’s background-removal control', () => { + const PNG_BYTES = Buffer.from([0x89, 0x50, 0x4e, 0x47, 0x0d, 0x0a, 0x1a, 0x0a, 0, 0, 0, 0]); + const JPEG_BYTES = Buffer.from([0xff, 0xd8, 0xff, 0xe0, 0, 0, 0, 0, 0, 0, 0, 0]); + + let uploads = ''; + let stub: http.Server | null = null; + + /** A stub sidecar on an ephemeral port, and a temporary uploads directory. */ + async function startStub(status: number, body: Buffer | string): Promise { + uploads = await fsp.mkdtemp(path.join(os.tmpdir(), 'adminbg-')); + process.env.UPLOADS_DIR = uploads; + + stub = http.createServer((req, res) => { + req.on('data', () => undefined); + req.on('end', () => { + res.writeHead(status, { 'Content-Type': 'image/png' }); + res.end(body); + }); + }); + await new Promise((resolve) => stub!.listen(0, '127.0.0.1', resolve)); + process.env.REMBG_URL = `http://127.0.0.1:${(stub!.address() as AddressInfo).port}`; + } + + afterEach(async () => { + delete process.env.REMBG_URL; + if (stub) { + await new Promise((resolve) => stub!.close(() => resolve())); + stub = null; + } + }); + + /** A ready draft with one photo, on disk, named to match the assertions. */ + async function seedDraftWithImage(): Promise<{ itemId: number; imageId: number }> { + const { rows } = await pool.query<{ id: number }>( + `INSERT INTO items (name, status) VALUES ('Submission placeholder', 'pending') RETURNING id` + ); + const itemId = rows[0]!.id; + await pool.query(`INSERT INTO item_drafts (item_id, state) VALUES ($1, 'ready')`, [itemId]); + const image = await pool.query<{ id: number }>( + `INSERT INTO item_images (item_id, image_path, sort_order) + VALUES ($1, '/uploads/original.jpg', 0) RETURNING id`, + [itemId] + ); + if (uploads !== '') await fsp.writeFile(path.join(uploads, 'original.jpg'), JPEG_BYTES); + return { itemId, imageId: image.rows[0]!.id }; + } + + it('says whether there is a sidecar behind the control at all', async () => { + process.env.REMBG_URL = 'http://rembg-syn:7000'; + const on = await request(app).get('/api/admin/item-drafts'); + expect(on.body.backgroundRemoval).toBe(true); + + delete process.env.REMBG_URL; + const off = await request(app).get('/api/admin/item-drafts'); + expect(off.body.backgroundRemoval).toBe(false); + }); + + // The UI decides between "Remove background" and "Restore original" from + // this field alone, so it has to be in the payload the queue is built from. + it('includes original_image_path on every image', async () => { + await seedDraftWithImage(); + + const res = await request(app).get('/api/admin/item-drafts'); + + expect(res.body.drafts[0].images[0]).toHaveProperty('original_image_path', null); + }); + + it('cuts out one photo and answers with its new paths', async () => { + await startStub(200, PNG_BYTES); + const { itemId, imageId } = await seedDraftWithImage(); + + const res = await request(app).post( + `/api/admin/item-drafts/${itemId}/images/${imageId}/remove-background` + ); + + expect(res.status).toBe(200); + expect(res.body.image_path).toBe('/uploads/original-cutout.png'); + expect(res.body.original_image_path).toBe('/uploads/original.jpg'); + }); + + it('puts the original back', async () => { + await startStub(200, PNG_BYTES); + const { itemId, imageId } = await seedDraftWithImage(); + await request(app).post( + `/api/admin/item-drafts/${itemId}/images/${imageId}/remove-background` + ); + + const res = await request(app).post( + `/api/admin/item-drafts/${itemId}/images/${imageId}/restore-original` + ); + + expect(res.status).toBe(200); + expect(res.body.image_path).toBe('/uploads/original.jpg'); + expect(res.body.original_image_path).toBeNull(); + }); + + // 502 rather than 500: the request was fine and the app is fine, and saying + // which of the two failed is what stops somebody searching the application + // logs for a fault that is not there. + it('answers 502 when the sidecar will not, and leaves the photo alone', async () => { + await startStub(500, 'boom'); + const { itemId, imageId } = await seedDraftWithImage(); + + const res = await request(app).post( + `/api/admin/item-drafts/${itemId}/images/${imageId}/remove-background` + ); + + expect(res.status).toBe(502); + const { rows } = await pool.query<{ image_path: string }>( + `SELECT image_path FROM item_images WHERE id = $1`, + [imageId] + ); + expect(rows[0]?.image_path).toBe('/uploads/original.jpg'); + }); + + // Scoped by item as well as by image. The id is a serial, so guessing one is + // not hard, and a photo from another submission must not be reachable + // through this item's URL. + it('refuses an image that does not belong to the item', async () => { + await startStub(200, PNG_BYTES); + const first = await seedDraftWithImage(); + const second = await seedDraftWithImage(); + + const res = await request(app).post( + `/api/admin/item-drafts/${first.itemId}/images/${second.imageId}/remove-background` + ); + + expect(res.status).toBe(404); + }); + + it('refuses to restore a photo that was never cut out', async () => { + const { itemId, imageId } = await seedDraftWithImage(); + + const res = await request(app).post( + `/api/admin/item-drafts/${itemId}/images/${imageId}/restore-original` + ); + + expect(res.status).toBe(404); + }); +}); -- 2.54.0 From 5129112266e1740b9e093717c6c2fbbb527c22de Mon Sep 17 00:00:00 2001 From: Thom Lamb Date: Thu, 3 Sep 2026 12:56:15 -0500 Subject: [PATCH 08/17] fix(admin): answer 404, not 500, when two restores race (#281) restore-original's precheck (existing.original_image_path === null) and restoreImageOriginal's own guard (WHERE ... AND original_image_path IS NOT NULL) could disagree under a race: two concurrent restores, or a rapid double-click, could both pass the precheck before either commits, and the loser's UPDATE would then match zero rows and throw. The handler had no try/catch around that call, so the throw propagated through asyncRoute to the app-level error handler and the caller got a bare 500, breaking the route's documented 200 | 404 contract even though the row itself was left correct. Wraps the restoreImageOriginal call in a try/catch, matching the shape remove-background already uses in this file, but answering 404 rather than 502: losing this race means another admin already finished the restore, not that a downstream service failed. Adds a comment on the catch explaining why it exists, and a test that fires two restores concurrently and asserts neither comes back 500. Co-Authored-By: Claude Opus 5 --- backend/src/routes/adminItemDrafts.ts | 15 ++++++++++++++- .../adminItemDrafts.integration.test.ts | 19 +++++++++++++++++++ 2 files changed, 33 insertions(+), 1 deletion(-) diff --git a/backend/src/routes/adminItemDrafts.ts b/backend/src/routes/adminItemDrafts.ts index cf4123a..1b99a7f 100644 --- a/backend/src/routes/adminItemDrafts.ts +++ b/backend/src/routes/adminItemDrafts.ts @@ -300,7 +300,20 @@ router.post( return res.status(404).json({ error: 'this photo has no original to restore' }); } - await restoreImageOriginal(Number(imageId)); + try { + await restoreImageOriginal(Number(imageId)); + } catch (err) { + // This precheck and restoreImageOriginal's own `original_image_path IS + // NOT NULL` guard can disagree under a race: two concurrent restores (or + // a double-click) can both pass the precheck before either commits, and + // the loser's UPDATE then matches zero rows and throws. That is not a + // sign anything is wrong — it means another request already did the + // restore, so the honest answer is the same 404 the precheck itself + // gives, not the generic 500 an uncaught throw would produce here. + console.error(`[drafts] restore for image ${imageId}:`, err); + return res.status(404).json({ error: 'this photo has no original to restore' }); + } + res.json(await imageOfItem(itemId, imageId)); }) ); diff --git a/backend/tests/integration/adminItemDrafts.integration.test.ts b/backend/tests/integration/adminItemDrafts.integration.test.ts index 89891f5..c808311 100644 --- a/backend/tests/integration/adminItemDrafts.integration.test.ts +++ b/backend/tests/integration/adminItemDrafts.integration.test.ts @@ -406,6 +406,25 @@ describe('the review queue’s background-removal control', () => { expect(res.body.original_image_path).toBeNull(); }); + // The precheck and restoreImageOriginal's own guard can disagree under a + // race. Whichever call loses, the answer must say "already done" rather than + // "something is wrong with this application". + it('does not answer 500 when two restores race', async () => { + await startStub(200, PNG_BYTES); + const { itemId, imageId } = await seedDraftWithImage(); + await request(app).post( + `/api/admin/item-drafts/${itemId}/images/${imageId}/remove-background` + ); + + const results = await Promise.all([ + request(app).post(`/api/admin/item-drafts/${itemId}/images/${imageId}/restore-original`), + request(app).post(`/api/admin/item-drafts/${itemId}/images/${imageId}/restore-original`) + ]); + + const statuses = results.map((r) => r.status).sort((a, b) => a - b); + expect(statuses).toEqual([200, 404]); + }); + // 502 rather than 500: the request was fine and the app is fine, and saying // which of the two failed is what stops somebody searching the application // logs for a fault that is not there. -- 2.54.0 From 984e329c47bdfd355545b6b5e97eead096b1a0b7 Mon Sep 17 00:00:00 2001 From: Thom Lamb Date: Thu, 3 Sep 2026 13:01:36 -0500 Subject: [PATCH 09/17] fix(admin): narrow the restore-original catch to the race it exists for (#281) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- backend/src/intake/backgroundRemoval.ts | 13 +++++++++- backend/src/routes/adminItemDrafts.ts | 25 +++++++++++++------ .../adminItemDrafts.integration.test.ts | 25 +++++++++++++++++++ 3 files changed, 54 insertions(+), 9 deletions(-) diff --git a/backend/src/intake/backgroundRemoval.ts b/backend/src/intake/backgroundRemoval.ts index 23eb313..eb2247b 100644 --- a/backend/src/intake/backgroundRemoval.ts +++ b/backend/src/intake/backgroundRemoval.ts @@ -23,6 +23,17 @@ interface ImageRow { original_image_path: string | null; } +/** + * Thrown when there is no original to put back. + * + * A named class rather than a bare Error because the route has to tell this + * apart from a database that is not answering. The two need opposite replies: + * this one means another admin has already restored the photo and the work is + * done, which is a 404; anything else means the application is in trouble and + * must stay loud rather than being reported as "already done". + */ +export class NoOriginalToRestoreError extends Error {} + /** * The path a cut-out of `imagePath` is written to. * @@ -114,7 +125,7 @@ export async function restoreImageOriginal(imageId: number): Promise { [imageId] ); if (rowCount === 0) { - throw new Error(`image ${imageId} has no original to restore`); + throw new NoOriginalToRestoreError(`image ${imageId} has no original to restore`); } } diff --git a/backend/src/routes/adminItemDrafts.ts b/backend/src/routes/adminItemDrafts.ts index 1b99a7f..8a662d9 100644 --- a/backend/src/routes/adminItemDrafts.ts +++ b/backend/src/routes/adminItemDrafts.ts @@ -3,7 +3,11 @@ import { pool } from '../db'; import { asyncRoute } from '../asyncRoute'; import { draftQueued } from '../intake/draftingWorker'; import { nextPriceSource, PriceSource } from '../intake/priceSource'; -import { removeImageBackground, restoreImageOriginal } from '../intake/backgroundRemoval'; +import { + NoOriginalToRestoreError, + removeImageBackground, + restoreImageOriginal, +} from '../intake/backgroundRemoval'; import { isRembgConfigured } from '../intake/rembgClient'; const router = Router(); @@ -303,13 +307,18 @@ router.post( try { await restoreImageOriginal(Number(imageId)); } catch (err) { - // This precheck and restoreImageOriginal's own `original_image_path IS - // NOT NULL` guard can disagree under a race: two concurrent restores (or - // a double-click) can both pass the precheck before either commits, and - // the loser's UPDATE then matches zero rows and throws. That is not a - // sign anything is wrong — it means another request already did the - // restore, so the honest answer is the same 404 the precheck itself - // gives, not the generic 500 an uncaught throw would produce here. + // Narrow on purpose: only NoOriginalToRestoreError means "another + // request already did this, the work is done". This precheck and + // restoreImageOriginal's own `original_image_path IS NOT NULL` guard can + // disagree under a race — two concurrent restores (or a double-click) + // can both pass the precheck before either commits, and the loser's + // UPDATE then matches zero rows and throws that specific error. Any + // other failure (a dropped connection, a transient outage) must not be + // reported the same way — it needs to stay loud as a 500, so it is + // rethrown here for asyncRoute's app-level handler to catch. + if (!(err instanceof NoOriginalToRestoreError)) { + throw err; + } console.error(`[drafts] restore for image ${imageId}:`, err); return res.status(404).json({ error: 'this photo has no original to restore' }); } diff --git a/backend/tests/integration/adminItemDrafts.integration.test.ts b/backend/tests/integration/adminItemDrafts.integration.test.ts index c808311..29e6f8b 100644 --- a/backend/tests/integration/adminItemDrafts.integration.test.ts +++ b/backend/tests/integration/adminItemDrafts.integration.test.ts @@ -7,6 +7,7 @@ import { AddressInfo } from 'net'; import { promises as fsp } from 'fs'; import os from 'os'; import path from 'path'; +import * as backgroundRemoval from '../../src/intake/backgroundRemoval'; beforeEach(async () => { await resetDb(); @@ -425,6 +426,30 @@ describe('the review queue’s background-removal control', () => { expect(statuses).toEqual([200, 404]); }); + // The catch around restoreImageOriginal exists only for the race above, not + // for every failure. A different kind of throw — a dropped connection, a + // transient outage — must not be reinterpreted as "already restored"; it has + // to stay a loud 500 so it reaches the app-level error handler. + it('stays a 500, not a 404, when restoreImageOriginal fails for a reason other than the race', async () => { + await startStub(200, PNG_BYTES); + const { itemId, imageId } = await seedDraftWithImage(); + await request(app).post( + `/api/admin/item-drafts/${itemId}/images/${imageId}/remove-background` + ); + + const spy = jest + .spyOn(backgroundRemoval, 'restoreImageOriginal') + .mockRejectedValueOnce(new Error('database is down')); + try { + const res = await request(app).post( + `/api/admin/item-drafts/${itemId}/images/${imageId}/restore-original` + ); + expect(res.status).toBe(500); + } finally { + spy.mockRestore(); + } + }); + // 502 rather than 500: the request was fine and the app is fine, and saying // which of the two failed is what stops somebody searching the application // logs for a fault that is not there. -- 2.54.0 From 1902db6d04b037e7407fa46aee98b2f38327e06f Mon Sep 17 00:00:00 2001 From: Thom Lamb Date: Thu, 3 Sep 2026 13:07:17 -0500 Subject: [PATCH 10/17] feat(intake): offer background removal on the submission page, ticked (#281) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- frontend/src/intake/Submit.tsx | 20 ++++++++++++++++- frontend/src/intake/intakeApi.ts | 13 ++++++++++- frontend/tests/e2e/intake-submit.spec.ts | 28 ++++++++++++++++++++++++ scripts/start-local.ps1 | 5 +++++ 4 files changed, 64 insertions(+), 2 deletions(-) diff --git a/frontend/src/intake/Submit.tsx b/frontend/src/intake/Submit.tsx index 2151898..c1332c7 100644 --- a/frontend/src/intake/Submit.tsx +++ b/frontend/src/intake/Submit.tsx @@ -8,6 +8,7 @@ import Input from 'antd/es/input'; import Alert from 'antd/es/alert'; import Spin from 'antd/es/spin'; import Space from 'antd/es/space'; +import Checkbox from 'antd/es/checkbox'; import { UploadOutlined } from '@ant-design/icons'; import type { UploadFile } from 'antd/es/upload/interface'; import { fetchIntakeLink, submitItem } from './intakeApi'; @@ -33,6 +34,10 @@ export default function Submit() { const [checking, setChecking] = useState(true); const [files, setFiles] = useState([]); const [note, setNote] = useState(''); + // Ticked by default. Most items look better cut out, and a submitter who + // wants their kitchen table in the photograph can say so — the reverse + // default would mean almost nobody got it. + const [removeBackground, setRemoveBackground] = useState(true); const [sending, setSending] = useState(false); const [sent, setSent] = useState(false); const [error, setError] = useState(null); @@ -62,7 +67,8 @@ export default function Submit() { // rather than map-then-filter because a type predicate cannot narrow to // File here — antd's RcFile extends it, so the predicate would widen. files.flatMap((f) => (f.originFileObj ? [f.originFileObj] : [])), - note + note, + removeBackground ); setSending(false); @@ -129,6 +135,7 @@ export default function Submit() { onClick={() => { setFiles([]); setNote(''); + setRemoveBackground(true); setSent(false); }} > @@ -171,6 +178,17 @@ export default function Submit() { placeholder="What is it, what is it made of, how big, what condition, where did it come from? Anything you know helps — a photo cannot show any of it." /> + {state.kind === 'usable' && state.link.backgroundRemoval && ( + setRemoveBackground(e.target.checked)} + > + {/* Described by what it does, not by how. Nobody sending in a + vase knows what a cut-out or an alpha channel is. */} + Remove the background from my photos + + )} + {error && } + )} + + ); +} + /** * One submission, with everything needed to judge it. * @@ -33,7 +89,11 @@ function priceLabel(source: PriceSource): string { * so — and 80.00 is a plausible price rather than an obvious sentinel, which is * exactly why it has to be called out rather than left to be noticed. */ -function DraftCard({ draft, onChanged }: Readonly<{ draft: Draft; onChanged: () => void }>) { +function DraftCard({ + draft, + backgroundRemoval, + onChanged +}: Readonly<{ draft: Draft; backgroundRemoval: boolean; onChanged: () => void }>) { const [name, setName] = useState(draft.ai_name ?? draft.item_name); const [description, setDescription] = useState( draft.ai_description ?? draft.item_description ?? '' @@ -82,13 +142,14 @@ function DraftCard({ draft, onChanged }: Readonly<{ draft: Draft; onChanged: () {draft.ai_error && } - + {draft.images.map((image) => ( - ))} @@ -148,12 +209,15 @@ function DraftCard({ draft, onChanged }: Readonly<{ draft: Draft; onChanged: () export default function DraftQueue() { const [drafts, setDrafts] = useState([]); + const [backgroundRemoval, setBackgroundRemoval] = useState(false); const [state, setState] = useState(undefined); const [error, setError] = useState(null); const load = useCallback(async () => { try { - setDrafts(await fetchDrafts(state)); + const payload = await fetchDrafts(state); + setDrafts(payload.drafts); + setBackgroundRemoval(payload.backgroundRemoval); setError(null); } catch { setError('Could not load the review queue.'); @@ -188,7 +252,12 @@ export default function DraftQueue() { {error && } {!error && drafts.length === 0 && } {drafts.map((draft) => ( - void load()} /> + void load()} + /> ))} ); diff --git a/frontend/src/admin/draftsApi.ts b/frontend/src/admin/draftsApi.ts index b8b6e49..aaa3555 100644 --- a/frontend/src/admin/draftsApi.ts +++ b/frontend/src/admin/draftsApi.ts @@ -3,6 +3,12 @@ export type PriceSource = 'default' | 'ai' | 'admin'; export interface DraftImage { id: number; image_path: string; + /** + * Where this photo came from, once it has been cut out. Null means it never + * was — which is also the answer to whether Restore has anything to do, so + * there is no second flag that could disagree with it. + */ + original_image_path: string | null; } export interface Draft { @@ -31,11 +37,21 @@ async function send(path: string, init?: RequestInit): Promise { }); } -export async function fetchDrafts(state?: string): Promise { +export interface DraftQueueResponse { + drafts: Draft[]; + /** + * Whether a background-removal sidecar is configured. False hides the + * control rather than showing one that would answer 502 — an environment + * without a sidecar is a working environment. + */ + backgroundRemoval: boolean; +} + +export async function fetchDrafts(state?: string): Promise { const query = state ? `?state=${encodeURIComponent(state)}` : ''; const res = await send(query); if (!res.ok) throw new Error('could not load the review queue'); - return (await res.json()).drafts; + return res.json(); } export interface PublishInput { @@ -71,3 +87,29 @@ export async function actOnDraft( const res = await send(`/${itemId}/${action}`, { method: 'POST' }); if (!res.ok) throw new Error(`could not ${action} this draft`); } + +/** + * Cut one photo out, or put its original back. + * + * The server's message is preferred over a generic one for the same reason + * publishDraft prefers it: a 502 here says the removal service did not answer + * and the photo is unchanged, which is the difference between "try again" and + * "something is wrong with this item". + */ +export async function setImageBackground( + itemId: number, + imageId: number, + action: 'remove-background' | 'restore-original' +): Promise { + const res = await send(`/${itemId}/images/${imageId}/${action}`, { method: 'POST' }); + if (res.ok) return; + + let message = 'could not change this photo'; + try { + message = (await res.json()).error ?? message; + } catch { + // A non-JSON body is a proxy or gateway error rather than the app + // refusing. The generic message is the honest thing to show. + } + throw new Error(message); +} diff --git a/frontend/tests/e2e/admin-draft-queue.spec.ts b/frontend/tests/e2e/admin-draft-queue.spec.ts index f952f83..5fe3720 100644 --- a/frontend/tests/e2e/admin-draft-queue.spec.ts +++ b/frontend/tests/e2e/admin-draft-queue.spec.ts @@ -98,4 +98,23 @@ test.describe('The review queue', () => { await dialog.getByRole('button', { name: 'Cancel', exact: true }).click(); await expect(card.getByText(/nobody chose this/)).toBeVisible(); }); + + // The control that makes a poor cut survivable. Its label is its state: + // "Remove background" until an original has been recorded, "Restore + // original" afterwards, read from one field rather than two that could + // disagree. + // + // Only the label is asserted, not a click. Pressing it would need a sidecar, + // and a test that depends on a service taking forty seconds to start is + // broken by construction — the swap itself is covered in the integration + // suite against a stub. + test('offers to remove the background on each photo', async ({ page, admin }) => { + const note = `Cutout ${RUN}`; + await submitAnItem(page, note); + + await admin.open('Review queue'); + + const card = page.locator('.ant-card').filter({ hasText: note }); + await expect(card.getByRole('button', { name: 'Remove background' })).toBeVisible(); + }); }); -- 2.54.0 From 786996b7acebcde89baddabd86cf62286bc00b34 Mon Sep 17 00:00:00 2001 From: Thom Lamb Date: Thu, 3 Sep 2026 13:41:55 -0500 Subject: [PATCH 12/17] fix(admin): keep Restore original working after REMBG_URL is unset (#281) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- frontend/src/admin/DraftQueue.tsx | 12 +++++++++++- frontend/src/admin/draftsApi.ts | 7 +++++-- 2 files changed, 16 insertions(+), 3 deletions(-) diff --git a/frontend/src/admin/DraftQueue.tsx b/frontend/src/admin/DraftQueue.tsx index 96f729e..07d65c5 100644 --- a/frontend/src/admin/DraftQueue.tsx +++ b/frontend/src/admin/DraftQueue.tsx @@ -42,6 +42,16 @@ function priceLabel(source: PriceSource): string { * * The label is the state. `original_image_path` is the only thing consulted, * so there is no second flag that could disagree with what the button does. + * + * `enabled` is computed by the caller as `backgroundRemoval || cutOut`, not as + * `backgroundRemoval` alone. The two branches this button offers need opposite + * things: removing calls the sidecar and has nothing to do without one, but + * restoring is a pure database swap that needs no sidecar at all. Gating both + * on `backgroundRemoval` hides Restore the moment REMBG_URL is unset — which + * happens for real when the sidecar is decommissioned after photos were + * already cut out — leaving a cut-out photo with no control and no way back to + * the original short of hand-editing the database. That breaks the invariant + * this whole feature rests on: the original is always restorable. */ function DraftPhoto({ image, @@ -148,7 +158,7 @@ function DraftCard({ key={image.id} image={image} itemId={draft.item_id} - enabled={backgroundRemoval} + enabled={backgroundRemoval || image.original_image_path !== null} onChanged={onChanged} /> ))} diff --git a/frontend/src/admin/draftsApi.ts b/frontend/src/admin/draftsApi.ts index aaa3555..b5a5eaa 100644 --- a/frontend/src/admin/draftsApi.ts +++ b/frontend/src/admin/draftsApi.ts @@ -41,8 +41,11 @@ export interface DraftQueueResponse { drafts: Draft[]; /** * Whether a background-removal sidecar is configured. False hides the - * control rather than showing one that would answer 502 — an environment - * without a sidecar is a working environment. + * remove-background control rather than showing one that would answer 502 + * — an environment without a sidecar is a working environment. It does + * *not* hide Restore original: that endpoint is a pure database swap and + * needs no sidecar, so DraftQueue shows it whenever a photo has an + * original to restore, regardless of this flag. */ backgroundRemoval: boolean; } -- 2.54.0 From b06eac46402e133110f521e6e4dc403216d2c660 Mon Sep 17 00:00:00 2001 From: Thom Lamb Date: Thu, 3 Sep 2026 13:42:24 -0500 Subject: [PATCH 13/17] fix(intake): clear remove_background when an admin restores a photo (#281) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- backend/src/intake/backgroundRemoval.ts | 53 +++++++++++++++---- backend/src/routes/adminItemDrafts.ts | 4 ++ ...ftingBackgroundRemoval.integration.test.ts | 36 +++++++++++++ 3 files changed, 84 insertions(+), 9 deletions(-) diff --git a/backend/src/intake/backgroundRemoval.ts b/backend/src/intake/backgroundRemoval.ts index eb2247b..13eb41d 100644 --- a/backend/src/intake/backgroundRemoval.ts +++ b/backend/src/intake/backgroundRemoval.ts @@ -110,22 +110,57 @@ export async function removeImageBackground(imageId: number): Promise { } /** - * Puts the original back. + * Puts the original back, and turns off the submitter's auto-removal intent + * for the item this photo belongs to. * * The cut-out file is left on disk deliberately. Removing a background is * exactly the operation that produces an occasional bad result on an unusual * object, so somebody restoring one is quite likely to try again — and this * module deletes nothing in any case. + * + * `item_drafts.remove_background` is written once at intake and otherwise + * never updated — without this, a restored photo looks identical to one that + * was simply never cut out, and Regenerate reads the same stale `true` and + * cuts it out again, quietly undoing the admin's decision. An admin restoring + * *any* photo on an item has overridden the submitter's request for that + * item: the flag is per-item while the swap is per-photo, so there is no + * per-photo place to record "leave this one alone" separately. Turning the + * whole item's auto-removal off is the conservative direction — the + * alternative is a worker that re-cuts a photo a person deliberately undid, + * which is the bug this fixes. + * + * Both writes happen in one transaction so a restore that succeeded while the + * flag update failed cannot reintroduce the bug it exists to close. */ export async function restoreImageOriginal(imageId: number): Promise { - const { rowCount } = await pool.query( - `UPDATE item_images - SET image_path = original_image_path, original_image_path = NULL - WHERE id = $1 AND original_image_path IS NOT NULL`, - [imageId] - ); - if (rowCount === 0) { - throw new NoOriginalToRestoreError(`image ${imageId} has no original to restore`); + const client = await pool.connect(); + try { + await client.query('BEGIN'); + const { rows } = await client.query<{ item_id: number }>( + `UPDATE item_images + SET image_path = original_image_path, original_image_path = NULL + WHERE id = $1 AND original_image_path IS NOT NULL + RETURNING item_id`, + [imageId] + ); + const restored = rows[0]; + if (!restored) { + await client.query('ROLLBACK'); + throw new NoOriginalToRestoreError(`image ${imageId} has no original to restore`); + } + + await client.query(`UPDATE item_drafts SET remove_background = false WHERE item_id = $1`, [ + restored.item_id + ]); + + await client.query('COMMIT'); + } catch (err) { + if (!(err instanceof NoOriginalToRestoreError)) { + await client.query('ROLLBACK'); + } + throw err; + } finally { + client.release(); } } diff --git a/backend/src/routes/adminItemDrafts.ts b/backend/src/routes/adminItemDrafts.ts index 8a662d9..7a48238 100644 --- a/backend/src/routes/adminItemDrafts.ts +++ b/backend/src/routes/adminItemDrafts.ts @@ -294,6 +294,10 @@ router.post( * occasional poor result on an unusual object, and this makes that survivable * rather than something to prevent. Nothing is deleted: the cut-out file stays * on disk, because somebody restoring one is quite likely to try again. + * + * restoreImageOriginal also turns off remove_background for this item, so a + * later Regenerate does not silently re-cut a photo the admin just put back — + * see the reasoning on that function. */ router.post( '/:itemId/images/:imageId/restore-original', diff --git a/backend/tests/integration/draftingBackgroundRemoval.integration.test.ts b/backend/tests/integration/draftingBackgroundRemoval.integration.test.ts index c4ae9d6..3180f6e 100644 --- a/backend/tests/integration/draftingBackgroundRemoval.integration.test.ts +++ b/backend/tests/integration/draftingBackgroundRemoval.integration.test.ts @@ -8,6 +8,7 @@ import { resetDb, closeDb } from './setup/testDb'; import { draftQueued } from '../../src/intake/draftingWorker'; import { resetAnthropicClient } from '../../src/intake/anthropicClient'; import { draftListing } from '../../src/intake/draftListing'; +import { restoreImageOriginal } from '../../src/intake/backgroundRemoval'; /** * The worker's background-removal step (#281). @@ -150,4 +151,39 @@ describe('background removal after a draft', () => { expect(rows[0]?.state).toBe('ready'); expect(await originalPathOf(itemId)).toBeNull(); }); + + // The bug this closes: remove_background is written once at intake and + // otherwise never updated, so a restored photo used to look identical to + // one that was simply never cut out — and Regenerate would read the same + // stale `true` and cut it out again, quietly undoing what the admin just + // did. restoreImageOriginal now clears the flag as part of the restore + // itself, so the worker respects the admin's decision on the next pass. + it('does not re-cut a photo the admin restored, even after Regenerate', async () => { + const itemId = await seedSubmissionWithPhoto({ removeBackground: true }); + await draftQueued(1); + expect(await originalPathOf(itemId)).toBe('/uploads/worker.jpg'); + + const { rows: imageRows } = await pool.query<{ id: number }>( + `SELECT id FROM item_images WHERE item_id = $1`, + [itemId] + ); + await restoreImageOriginal(imageRows[0]!.id); + + const { rows: flagRows } = await pool.query<{ remove_background: boolean }>( + `SELECT remove_background FROM item_drafts WHERE item_id = $1`, + [itemId] + ); + expect(flagRows[0]?.remove_background).toBe(false); + + // Mirrors what the admin's Regenerate button does: back to queued, with + // attempts cleared, so the worker picks the item up again. + await pool.query( + `UPDATE item_drafts SET state = 'queued', attempts = 0, ai_error = NULL WHERE item_id = $1`, + [itemId] + ); + + await draftQueued(1); + + expect(await originalPathOf(itemId)).toBeNull(); + }); }); -- 2.54.0 From dfd900aadd748c2f6fc0db5e5f6fd4da659e3be6 Mon Sep 17 00:00:00 2001 From: Thom Lamb Date: Thu, 3 Sep 2026 13:42:47 -0500 Subject: [PATCH 14/17] fix(admin): stop reporting every removeImageBackground failure as a sidecar failure (#281) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- backend/src/intake/rembgClient.ts | 36 +++++++++++++++---- backend/src/routes/adminItemDrafts.ts | 33 +++++++++++++---- .../adminItemDrafts.integration.test.ts | 18 ++++++++++ backend/tests/unit/rembgClient.test.ts | 16 +++++++-- 4 files changed, 87 insertions(+), 16 deletions(-) diff --git a/backend/src/intake/rembgClient.ts b/backend/src/intake/rembgClient.ts index e93b79d..c3eb197 100644 --- a/backend/src/intake/rembgClient.ts +++ b/backend/src/intake/rembgClient.ts @@ -33,6 +33,18 @@ const MODEL = 'u2net'; */ const TIMEOUT_MS = 120_000; +/** + * Thrown only when the sidecar was actually contacted and did not answer + * usably — unreachable, timed out, answered with a non-2xx status, or + * answered with something that is not a PNG. + * + * Deliberately not thrown for "REMBG_URL is not set": that failure happens + * before any attempt to contact anything, so lumping it in here would tell a + * caller "the service did not answer" about a service nothing ever tried to + * reach. A caller distinguishes the two to avoid exactly that (#281 review). + */ +export class SidecarRequestError extends Error {} + /** The configured base URL, or null when there is none. */ function baseUrl(): string | null { const raw = process.env.REMBG_URL; @@ -72,14 +84,24 @@ export async function removeBackground(bytes: Buffer, mediaType: string): Promis body.append('file', new Blob([new Uint8Array(bytes)], { type: mediaType }), 'photo'); body.append('model', MODEL); - const res = await fetch(`${base}/api/remove`, { - method: 'POST', - body, - signal: AbortSignal.timeout(TIMEOUT_MS) - }); + let res: Response; + try { + res = await fetch(`${base}/api/remove`, { + method: 'POST', + body, + signal: AbortSignal.timeout(TIMEOUT_MS) + }); + } catch (err) { + // Unreachable, refused, or timed out — fetch throws for all three rather + // than returning a response, so this is the only place that can catch + // them and mark them as a sidecar failure rather than a generic error. + throw new SidecarRequestError( + `rembg did not answer: ${err instanceof Error ? err.message : String(err)}` + ); + } if (!res.ok) { - throw new Error(`rembg answered ${res.status}`); + throw new SidecarRequestError(`rembg answered ${res.status}`); } const out = Buffer.from(await res.arrayBuffer()); @@ -88,7 +110,7 @@ export async function removeBackground(bytes: Buffer, mediaType: string): Promis // image/png would otherwise be written over a photograph — the same reason // uploads are checked by signature rather than by what the caller declared. if (!signatureMatches('image/png', out.subarray(0, SIGNATURE_BYTES))) { - throw new Error('rembg response is not a PNG'); + throw new SidecarRequestError('rembg response is not a PNG'); } return out; diff --git a/backend/src/routes/adminItemDrafts.ts b/backend/src/routes/adminItemDrafts.ts index 7a48238..5fd4c6d 100644 --- a/backend/src/routes/adminItemDrafts.ts +++ b/backend/src/routes/adminItemDrafts.ts @@ -8,7 +8,7 @@ import { removeImageBackground, restoreImageOriginal, } from '../intake/backgroundRemoval'; -import { isRembgConfigured } from '../intake/rembgClient'; +import { isRembgConfigured, SidecarRequestError } from '../intake/rembgClient'; const router = Router(); @@ -274,13 +274,32 @@ router.post( try { await removeImageBackground(Number(imageId)); } catch (err) { - // 502, not 500. The request was fine and so is this app — the service it - // depends on did not answer. The message says the photo is unchanged, - // because that is the thing the admin actually needs to know. console.error(`[drafts] background removal for image ${imageId}:`, err); - return res - .status(502) - .json({ error: 'the background-removal service did not answer — the photo is unchanged' }); + + // 502 only for a SidecarRequestError: the request was fine and so is + // this app — the service it depends on was actually contacted and did + // not answer usably. The message says the photo is unchanged, because + // that is the thing the admin actually needs to know. + if (err instanceof SidecarRequestError) { + return res + .status(502) + .json({ error: 'the background-removal service did not answer — the photo is unchanged' }); + } + + // Everything else here never reached the sidecar at all — an + // unrecognised file extension (a legacy .jpeg), a file missing from the + // uploads volume, or REMBG_URL not being set. Reporting those as "the + // service did not answer" would send the admin to retry a service that + // was never contacted, and hide the real reason in the server log. The + // photo is still unchanged in every one of these cases too: + // removeImageBackground only writes the row once the cut-out already + // exists on disk. + return res.status(500).json({ + error: + err instanceof Error + ? `this photo could not be processed: ${err.message}` + : 'this photo could not be processed' + }); } res.json(await imageOfItem(itemId, imageId)); diff --git a/backend/tests/integration/adminItemDrafts.integration.test.ts b/backend/tests/integration/adminItemDrafts.integration.test.ts index 29e6f8b..4e4897a 100644 --- a/backend/tests/integration/adminItemDrafts.integration.test.ts +++ b/backend/tests/integration/adminItemDrafts.integration.test.ts @@ -469,6 +469,24 @@ describe('the review queue’s background-removal control', () => { expect(rows[0]?.image_path).toBe('/uploads/original.jpg'); }); + // MINOR 3 (#281 review): a failure that happens before the sidecar is ever + // contacted — here, the file the row points to is missing from the uploads + // volume — must not be reported as "the service did not answer". That sends + // the admin to retry a service that was never reached, and hides the real + // reason in the server log. + it('does not answer 502 when the photo cannot be read, even though a sidecar is configured', async () => { + await startStub(200, PNG_BYTES); + const { itemId, imageId } = await seedDraftWithImage(); + await fsp.unlink(path.join(uploads, 'original.jpg')); + + const res = await request(app).post( + `/api/admin/item-drafts/${itemId}/images/${imageId}/remove-background` + ); + + expect(res.status).not.toBe(502); + expect(res.body.error).not.toMatch(/did not answer/); + }); + // Scoped by item as well as by image. The id is a serial, so guessing one is // not hard, and a photo from another submission must not be reachable // through this item's URL. diff --git a/backend/tests/unit/rembgClient.test.ts b/backend/tests/unit/rembgClient.test.ts index 1779d86..7330931 100644 --- a/backend/tests/unit/rembgClient.test.ts +++ b/backend/tests/unit/rembgClient.test.ts @@ -1,6 +1,6 @@ import http from 'http'; import { AddressInfo } from 'net'; -import { isRembgConfigured, removeBackground } from '../../src/intake/rembgClient'; +import { isRembgConfigured, removeBackground, SidecarRequestError } from '../../src/intake/rembgClient'; /** A real PNG header, so the client's own signature check sees what it expects. */ const PNG = Buffer.from([0x89, 0x50, 0x4e, 0x47, 0x0d, 0x0a, 0x1a, 0x0a, 0, 0, 0, 0]); @@ -108,6 +108,9 @@ describe('asking the sidecar to remove a background', () => { }, async () => { await expect(removeBackground(JPEG, 'image/jpeg')).rejects.toThrow(/500/); + await expect(removeBackground(JPEG, 'image/jpeg')).rejects.toBeInstanceOf( + SidecarRequestError + ); } ); }); @@ -123,12 +126,21 @@ describe('asking the sidecar to remove a background', () => { }, async () => { await expect(removeBackground(JPEG, 'image/jpeg')).rejects.toThrow(/not a PNG/); + await expect(removeBackground(JPEG, 'image/jpeg')).rejects.toBeInstanceOf( + SidecarRequestError + ); } ); }); - it('rejects when it is not configured at all', async () => { + // Deliberately not a SidecarRequestError (MINOR 3, #281 review): this + // failure happens before any attempt to contact the sidecar, and a caller + // has to be able to tell "never tried" apart from "tried and failed". + it('rejects when it is not configured at all, without it being a sidecar failure', async () => { delete process.env.REMBG_URL; await expect(removeBackground(JPEG, 'image/jpeg')).rejects.toThrow(/REMBG_URL/); + await expect(removeBackground(JPEG, 'image/jpeg')).rejects.not.toBeInstanceOf( + SidecarRequestError + ); }); }); -- 2.54.0 From 3edcc7fcfa3d2df7122bd533feebd85fcae98e51 Mon Sep 17 00:00:00 2001 From: Thom Lamb Date: Thu, 3 Sep 2026 13:42:58 -0500 Subject: [PATCH 15/17] docs(intake): correct a false claim about cut-out durability (#281) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- backend/src/intake/backgroundRemoval.ts | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/backend/src/intake/backgroundRemoval.ts b/backend/src/intake/backgroundRemoval.ts index 13eb41d..31bfa11 100644 --- a/backend/src/intake/backgroundRemoval.ts +++ b/backend/src/intake/backgroundRemoval.ts @@ -12,10 +12,14 @@ import { isRembgConfigured, removeBackground } from './rembgClient'; * undoable the same way. A near-copy that drifted would mean a photo the * Restore button could not restore. * - * Nothing here deletes anything. The original file stays on disk and so does - * every cut-out ever made, because the submitter's photos are often the only - * copy of an item no longer in their hands — the same rule Discard follows in - * the review queue. + * Nothing here deletes anything. The original file always stays on disk, + * because the submitter's photos are often the only copy of an item no longer + * in their hands — the same rule Discard follows in the review queue. A + * cut-out is not as durable: `cutoutPathFor` is deterministic, so a photo that + * is restored and then cut out again writes over the previous cut-out at the + * same path. That is harmless — no original is ever touched — but it means + * "every cut-out ever made" is not actually true, so this comment used to + * overstate it. */ interface ImageRow { -- 2.54.0 From 66cafeb89c533c321ce8cef6ebccf27ef922f87c Mon Sep 17 00:00:00 2001 From: Thom Lamb Date: Thu, 3 Sep 2026 13:43:07 -0500 Subject: [PATCH 16/17] docs(compose): keep the prod comment block's sentences with their own variable (#281) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- docker-compose.prod.yml | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/docker-compose.prod.yml b/docker-compose.prod.yml index 17b3857..b73b560 100644 --- a/docker-compose.prod.yml +++ b/docker-compose.prod.yml @@ -92,6 +92,12 @@ # failing, so an empty value is a working configuration. # ANTHROPIC_API_KEY Optional. Drafts a listing from a submitted photo # (#223). Unset means submissions still arrive and wait +# undrafted, which is a working configuration for the +# same reason USPS is. The one credential here that +# spends money per call, and on a path anybody holding +# an upload link can trigger — put a spend limit on the +# key in the Anthropic console, because nothing in this +# repository can enforce one. # ANTHROPIC_WORKSPACE_ID Required alongside the key above when that key is # identity-linked. Without it every draft fails with a # 400 naming the missing header (#271). @@ -100,12 +106,6 @@ # still sends and carries no shortcuts. # REMBG_URL Optional. The background-removal sidecar, e.g. # http://rembg-syn:7000. Unset turns the feature off. -# undrafted, which is a working configuration for the -# same reason USPS is. The one credential here that -# spends money per call, and on a path anybody holding -# an upload link can trigger — put a spend limit on the -# key in the Anthropic console, because nothing in this -# repository can enforce one. # # The names above are what this file reads. A stack variable under any other # name is substituted nowhere and never reaches the container, so reconciling -- 2.54.0 From 6f8a0db1302567305c3309ac0d5233e6207aa242 Mon Sep 17 00:00:00 2001 From: Thom Lamb Date: Thu, 3 Sep 2026 13:43:25 -0500 Subject: [PATCH 17/17] test(integration): clean up background-removal test-harness leftovers (#281) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .../adminItemDrafts.integration.test.ts | 9 +++++++++ .../backgroundRemoval.integration.test.ts | 15 +++++++++++++-- .../draftingBackgroundRemoval.integration.test.ts | 9 +++++++++ 3 files changed, 31 insertions(+), 2 deletions(-) diff --git a/backend/tests/integration/adminItemDrafts.integration.test.ts b/backend/tests/integration/adminItemDrafts.integration.test.ts index 4e4897a..6ebfca8 100644 --- a/backend/tests/integration/adminItemDrafts.integration.test.ts +++ b/backend/tests/integration/adminItemDrafts.integration.test.ts @@ -317,10 +317,12 @@ describe('the review queue’s background-removal control', () => { let uploads = ''; let stub: http.Server | null = null; + let previousUploadsDir: string | undefined; /** A stub sidecar on an ephemeral port, and a temporary uploads directory. */ async function startStub(status: number, body: Buffer | string): Promise { uploads = await fsp.mkdtemp(path.join(os.tmpdir(), 'adminbg-')); + previousUploadsDir = process.env.UPLOADS_DIR; process.env.UPLOADS_DIR = uploads; stub = http.createServer((req, res) => { @@ -340,6 +342,13 @@ describe('the review queue’s background-removal control', () => { await new Promise((resolve) => stub!.close(() => resolve())); stub = null; } + if (uploads !== '') { + await fsp.rm(uploads, { recursive: true, force: true }); + uploads = ''; + } + if (previousUploadsDir === undefined) delete process.env.UPLOADS_DIR; + else process.env.UPLOADS_DIR = previousUploadsDir; + previousUploadsDir = undefined; }); /** A ready draft with one photo, on disk, named to match the assertions. */ diff --git a/backend/tests/integration/backgroundRemoval.integration.test.ts b/backend/tests/integration/backgroundRemoval.integration.test.ts index 0832617..a0eba9c 100644 --- a/backend/tests/integration/backgroundRemoval.integration.test.ts +++ b/backend/tests/integration/backgroundRemoval.integration.test.ts @@ -17,7 +17,7 @@ afterAll(async () => { }); /** An item with a draft row and one image, which is what a submission leaves. */ -export async function seedSubmission( +async function seedSubmission( imagePath = '/uploads/photo.jpg' ): Promise<{ itemId: number; imageId: number }> { const item = await pool.query<{ id: number }>( @@ -69,13 +69,16 @@ interface ImagePaths { let uploads = ''; let stub: http.Server | null = null; +let previousUploadsDir: string | undefined; /** * A real uploads directory and a stub sidecar. * * A temporary directory rather than the configured one, because these tests * write files and a suite that leaves rubbish in a developer's uploads volume - * is a suite people stop running. + * is a suite people stop running. `afterEach` removes it and puts back + * whatever UPLOADS_DIR held before, so this suite does not leak either the + * directory or the environment variable into whatever runs after it. * * No test here contacts the real sidecar. It takes forty seconds to start, and * a suite that depends on that is broken by construction. @@ -84,6 +87,7 @@ async function startStub( handler: (req: http.IncomingMessage, res: http.ServerResponse) => void ): Promise { uploads = await fsp.mkdtemp(path.join(os.tmpdir(), 'bgremoval-')); + previousUploadsDir = process.env.UPLOADS_DIR; process.env.UPLOADS_DIR = uploads; stub = http.createServer((req, res) => { @@ -107,6 +111,13 @@ afterEach(async () => { await new Promise((resolve) => stub!.close(() => resolve())); stub = null; } + if (uploads !== '') { + await fsp.rm(uploads, { recursive: true, force: true }); + uploads = ''; + } + if (previousUploadsDir === undefined) delete process.env.UPLOADS_DIR; + else process.env.UPLOADS_DIR = previousUploadsDir; + previousUploadsDir = undefined; }); async function seedWithFile(): Promise<{ itemId: number; imageId: number; name: string }> { diff --git a/backend/tests/integration/draftingBackgroundRemoval.integration.test.ts b/backend/tests/integration/draftingBackgroundRemoval.integration.test.ts index 3180f6e..f3fbd2a 100644 --- a/backend/tests/integration/draftingBackgroundRemoval.integration.test.ts +++ b/backend/tests/integration/draftingBackgroundRemoval.integration.test.ts @@ -27,6 +27,7 @@ const PNG_BYTES = Buffer.from([0x89, 0x50, 0x4e, 0x47, 0x0d, 0x0a, 0x1a, 0x0a, 0 let uploads = ''; let stub: http.Server | null = null; +let previousUploadsDir: string | undefined; beforeEach(async () => { await resetDb(); @@ -51,6 +52,7 @@ beforeEach(async () => { resetAnthropicClient(); uploads = await fsp.mkdtemp(path.join(os.tmpdir(), 'workerbg-')); + previousUploadsDir = process.env.UPLOADS_DIR; process.env.UPLOADS_DIR = uploads; stub = http.createServer((req, res) => { @@ -72,6 +74,13 @@ afterEach(async () => { await new Promise((resolve) => stub!.close(() => resolve())); stub = null; } + if (uploads !== '') { + await fsp.rm(uploads, { recursive: true, force: true }); + uploads = ''; + } + if (previousUploadsDir === undefined) delete process.env.UPLOADS_DIR; + else process.env.UPLOADS_DIR = previousUploadsDir; + previousUploadsDir = undefined; }); afterAll(async () => { -- 2.54.0