Three tasks, each with its own test cycle: the file operation, the endpoints and the module behind them, then the control in the review queue.
Two things were probed rather than assumed while writing it, and both changed the design. sharp reads a positive angle as clockwise, so left is rotate(-90) and right is rotate(90) — and the direction test asserts a pixel rather than a dimension, because a rectangle's dimensions swap whichever way the turn goes and a reversed sign would pass every size assertion while shipping a control that does the opposite of its label. And a quarter turn of an animated WebP is refused by sharp itself, which is what makes passing the same animated flag reencodeInPlace passes the safe choice: omitting it would read the first frame alone and write a still back over someone's animation while reporting success.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Three tasks: the two per-item functions, the endpoints plus a small admin config route, and the button.
The config route is the piece the spec did not anticipate. The inventory screen has no way to learn the feature is configured — GET /api/admin/item-drafts carries that flag for the review queue, but GET /api/admin/items answers a bare array with several consumers, and reshaping it for one boolean is the worse trade. routes/adminVersion.ts is the precedent for exactly this: a small admin-only GET, deliberately not folded into the public /api/config, with the reason written down beside it.
Two decisions the plan pins that the spec left as prose. The render condition is "configured OR every photo already cut out", not the flag alone, because gating on the flag would hide Restore originals the moment REMBG_URL is unset and strand cut-out photos with no way back — the same shape DraftQueue already uses for the same reason. And the button re-reads the item afterwards, because the editor is a modal changing files on the server while it is open, and without that the thumbnails keep showing the previous files and the button looks inert.
Self-review caught the mistake I have now made three times this session, which is naming something that does not exist. Task 3's end-to-end case called createItem(page, { withImage: true }); createItem actually takes an APIRequestContext rather than a page, and CreateItemOptions has no image field at all. Since the control only renders for an item that has photos, the item now gets seeded through the admin API with a real PNG attached, which is a thing that works rather than a thing that reads well.
It also records a spec requirement that is deliberately not implemented as written. The spec asks for an end-to-end assertion that the control is absent when the feature is unconfigured; that would mean restarting the backend mid-suite, which the run has no way to do and should not gain one. It is covered where it can be, in the integration test for GET /api/admin/config, and the plan says so rather than dropping it quietly.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Four tasks over the approved design: sendMail gains an outcome, the column and template land together, the route requires an address and reports what the send did, and the admin screen asks for it.
sendMail goes first deliberately. Everything else depends on being able to tell a skipped send from a real one, and it is the only change touching a file seven other things already use — so if it is going to break anything, it should break before three tasks are stacked on top of it.
The plan is explicit that no existing caller changes. Ignoring a returned value is legal, which is what makes widening the return type additive rather than breaking, and re-deriving "would this address be blocked?" in the route would have duplicated isAllowedRecipient and the SMTP check at a second site.
Two specs in unrelated features create links with only a label, and the route will refuse that. They are fixed in the same task as the form rather than left for the suite to find, because the alternative is two unrelated features going red on somebody else's branch. That cost is named in the plan rather than discovered.
One inaccuracy is left in deliberately and said out loud: an SMTP rejection is reported as skipped-unconfigured rather than a fourth outcome. The distinction is real, nothing consumes it, and the admin's next action is identical either way — copy the link and send it by hand.
Self-review caught the failure mode from #281, where tasks referred to helpers that did not exist. Task 4 originally said the created-link state "may not be called created". Reading the component showed it is `issued` and holds a bare URL string with nowhere to put a delivery outcome, so the plan now adds a separate `mailed` state beside it rather than widening the one-time token display. Every name in that task is now one that exists.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Three tasks over the decisions recorded on the issue: the pure paging rules with their tests, the wiring into the catalogue, and the end-to-end cases including the two assertions this finally lets say what they mean.
Two decisions the issue left open are settled here. The page number goes in the URL beside the filters, because a numbered page being linkable is the whole reason numbered pages were chosen over infinite scroll, and the URL is already the single source of truth for what the storefront is showing. The page size deliberately does not, because it is a preference belonging to one person — putting it in the URL would mean sharing a link to an item also imposed your page size on whoever opened it. It lives in localStorage instead, which also keeps it inside the issue's own scope boundary of not touching the API.
The third open question, whether page size changes with viewport, is answered no and written down as such rather than left silent. A size that moved on rotation would fight the preference the customer had just set.
Everything testable is a pure function, because that is all this project can unit-test: vitest runs in a node environment with no jsdom and no testing-library, so a hook or a component is only reachable through Playwright. That is the same split filters.ts already uses, and it is why the React wrapper is thin enough to need no test of its own.
Writing it turned up one piece of luck worth not breaking: filtersToSearchParams builds a fresh URLSearchParams, so applying a filter already drops the page parameter and returns to page one, while paging copies the existing params and keeps the filters. The plan says so explicitly so nobody 'fixes' it later.
It also confirmed the issue's own warning. filters.spec.ts:165 asserts a named fixture is visible in the unfiltered grid, and favorites-filter.spec.ts:106 does the same — with ten items to a page over thousands, both would start failing the moment paging landed. They were always proxies for 'the result set got bigger', and a visible total lets them assert that directly.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Eight tasks over the approved design, each ending in something independently testable: the two columns, the sidecar client, the shared swap-and-restore, the worker step, the intake route, the admin endpoints, the submitter's checkbox, and the review queue's per-photo control.
Three things the plan pins down that the spec left to implementation.
The `model=u2net` assertion lives in a unit test against a real stub HTTP server rather than a mocked fetch, because what has to be checked is the shape of the request that reaches the wire. Nothing in the returned image would reveal that the non-commercial default had been used, so that assertion is the only thing standing between this and a licensing problem that produces perfectly good pictures.
Removal in the worker follows drafting rather than running on its own pass, which couples the two: an environment with no ANTHROPIC_API_KEY drafts nothing and so cuts out nothing. That is the deliberate trade — a separate pass would re-attempt an unreachable sidecar on every five-minute sweep for a row that is going to sit at `queued` indefinitely — and the plan says so in the worker's own header comment rather than leaving it to be rediscovered.
`removeImageBackground` is idempotent through the `original_image_path IS NOT NULL` check rather than a separate flag, and that guard is load-bearing twice: it makes a repeat call a no-op, and it stops a second pass recording the cut-out as the original and losing the real one for good.
Writing it turned up two things worth knowing about the existing tests. `drafting.integration.test.ts` has never produced a successful draft — every case in it either has no key or no readable photo — so the worker's new cases need their own file with `draftListing` mocked, rather than a mock added file-wide to a suite that deliberately never reaches the model. And `adminItemDrafts.integration.test.ts` calls `request(app)` directly with no helper, so the plan spells out the seed it needs instead of pointing at one that does not exist.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Six tasks: the settings, stopping them leaking between test suites, the count, the alerts, the refusal, and the manual reset.
The count is derived from item_drafts rather than a tally, as the issue asks. That forces a decision it left open: a reset cannot delete anything, because the rows are real submissions whose items are sitting in the review queue. So a reset stores a timestamp and the window becomes the later of that and 24 hours ago — one derived count, no second tally, and a reset that is an auditable fact rather than a deletion.
The refusal is ordered ahead of uploadImages, for the same reason requireUsableLink is: a refused submission must write zero bytes. Ordering it after would accept the upload, store the files and throw them away, which is the expensive half of the work the ceiling exists to prevent.
The alert throttle is in memory rather than in the database, so a restart during an incident can send one extra alert. That is a better trade than writing to admin_settings from the request path on every refused submission, and it is noted that a replicated deployment would have to move it.
Self-review caught two things against the tree. POST /api/admin/items answers 200 rather than 201, so that assertion was wrong. And resetDb deliberately does not truncate admin_settings — it deletes only the email_ rows — so a ceiling of 1 left behind would make every later suite's submissions refuse with a 503, in files that never mention a ceiling. The comment in that file records the same failure happening once already with an email template. Task 1b widens the cleanup.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Five tasks: a pure HMAC signer, the editable template and its recipient setting, the send from the drafting worker, the public routes that act on a signed link, and the quiet paths.
The plan makes one decision the issue does not, and it changes the shape of the feature. Mail scanners and link-rewriting gateways issue a GET against every URL in a message before a human sees it, so a GET that discarded a draft would fire itself on delivery — with a valid signature, looking entirely legitimate in the log, and nobody would know to go and recover it. So the signed link is a safe GET that confirms and a POST that acts. It costs one extra click and is cheap to reverse if that is the wrong trade.
Everything about the notification is best-effort. No recipient configured, no INTAKE_ACTION_SECRET, or SMTP down all end in a log line: the review queue is the source of truth, and a draft that was written correctly must never be marked failed because an email did not send.
The email still cannot publish. The two signable actions are exactly the ones whose worst case is a wasted API call or a hide the queue can undo, which is what makes putting them in an inbox acceptable at all.
Branched from feature/225-review-queue rather than main, because the review link has nowhere to land without it.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Six tasks: the price provenance rule as a pure unit, the list endpoint, publish, the three state actions, the screen, and an end-to-end pass.
The price field is the reason this screen exists. Items are priced on arrival, so the schema no longer prevents a number nobody chose from reaching the storefront — that protection moves here, into presentation, where it is weaker. So the rule is a pure tested function rather than a line inside a route: editing the number is the only thing that confirms it, publishing an untouched field deliberately does not, and publishing something still unconfirmed asks first rather than reporting afterwards. 80.00 was chosen because it reads as a decision rather than as an obvious sentinel, which is exactly why it has to be called out rather than left to be noticed.
Discard deletes nothing. It is one click from an inbox, and the photos are often the only copy of an item no longer in the sender's hands, so it marks the draft and returns the item to pending. Restore brings it back at the state its own contents justify rather than unconditionally ready, because a submission discarded before it was ever drafted has no copy and must not return claiming otherwise.
Regenerate clears attempts along with the state. The worker only picks up rows below the attempt cap, so re-queueing a draft that already failed three times would otherwise produce a button that appears to work, does nothing, and says nothing.
The end-to-end test seeds through the intake route rather than POST /api/admin/items, which writes no item_drafts row and would never appear in a queue that joins it.
One deviation from the issue is recorded in the plan rather than buried: it asks for the existing admin item components to be reused, and this builds a purpose-made card instead, because the fields differ in kind rather than arrangement. The cost — two places rendering a name, description and price — is named there too.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Only the manual verification against a real photograph is left, and it needs an API key that does not exist yet.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Added mid-execution at Thom's request. Two consequences worth recording: it is a dropdown validated on the server rather than a free-text box, because a mistyped model name fails on every submission and surfaces only as drafts quietly not appearing; and the model list lives in one catalogue shared with the cost table, so the settings dropdown and the per-token rates cannot drift apart.
Rates confirmed against the pricing page rather than recalled. Worth having checked: an increase to $3/$15 had been scheduled for tomorrow and was cancelled, with $2/$10 made permanent.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Eight tasks: the SDK and its optional key, the Zod shape the model must answer in, the prompt, the call, writing a draft back, the worker that drives it, the wiring, and one real photograph to see whether any of it writes something worth reading.
The prompt is the correctness surface and gets its own task with its own tests. On one-of-a-kind stock an invented "1930s hand-thrown stoneware" is not a cosmetic error but a false claim on a storefront, and nothing downstream can tell an invented detail from an observed one — the only place that distinction can be enforced is in the instruction, so the tests assert it is there.
The other governing rule is that a submission is the only irreplaceable thing in the pipeline. The photos are often the only copy of an item no longer in the sender's hands, so a missing key, a failed call, a malformed answer and three exhausted retries all end the same way: the item keeps its photos and waits undrafted. Nothing in the worker deletes anything, and an absent key does not spend an attempt.
Only the suggested price reaches the item, per #220, with price_source recording that a model rather than a person chose it. The name and description stay on the draft row until the review queue in #225 exists.
The monthly spend ceiling is deliberately left out. Task 8 measures what a real call costs first, because a budget set from a guessed number is one nobody trusts.
Every test stubs the Anthropic client. A test that reaches the real API is a defect in the test.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This plan was written on 2026-08-29, before #226 landed. Its Task 2 lists what to move out of routes/admin.ts into the shared image pipeline, and that list is now missing `stripUploadedImages` and the `reencodeInPlace` import it depends on, because neither existed when the list was written.
Executing it as written would have left the re-encode behind in admin.ts, and the public intake route added in Task 5 would then have had an upload path that skips EXIF stripping entirely. That is precisely what #226 exists to prevent — a stranger photographing an item at home publishing the coordinates it was taken at — and nothing in the suite would have failed to say so, because the intake tests are written against a route that does not exist yet.
The task now names the function, says why it matters, and adds a check with a definite answer: after the move, routes/admin.ts must no longer import imageProcessing. If it still does, something was left behind.
Ref #222, #226
Four tasks. A pure findOrFail helper with Vitest coverage, the nine unchecked lookups converted to use it, the expect timeout raised from Playwright's unset default of 5s to 10s, and the test #245 skipped brought back.
The helper deliberately imports nothing and lives apart from support/api.ts. api.ts imports @playwright/test, and vitest.config.ts runs with environment: 'node' over tests/unit only — putting the helper there would drag a browser harness into the unit suite to test six lines of pure logic.
Two things the survey changed. filters.spec.ts:216 is excluded: its .find() searches CSS class names on a string array rather than test data, so it has no missing-row failure mode. And three `expect(row).toBeTruthy()` assertions are deleted rather than kept, because findOrFail has already thrown by then — leaving them would tell the next reader the value might be falsy, which is the confusion the change exists to remove.
Worker-count reduction is explicitly not in this plan. It is a real lever and may still be needed, but applying it at the same time would make it impossible to tell which change fixed anything.
The plan states the criterion it cannot check: CI's load is not reproducible here, so two consecutive green local runs mean the refactor is sound, not that the flakiness is gone. Several consecutive green CI runs are the real bar, and #241 stays open until then.
Ref #241
Five tasks: prove sharp installs where it actually runs, the re-encode policy as a pure module, wiring it into the single middleware every upload path already passes through, the backfill over already-stored photos, and the deployment sequence.
Re-encoding rather than deleting tags. Deleting requires knowing every tag that could carry something sensitive, across formats and camera makers, indefinitely; rebuilding the file from decoded pixels leaves nothing that could have been missed. Same reasoning that makes uploadTypes.ts an allowlist.
Format is preserved rather than normalised to WebP. Converting would compress better but changes every stored extension, and therefore item_images.image_path, turning the backfill into a rename with a window where rows point at files that no longer exist. A privacy fix does not need that risk, and the backfill consequently touches no database rows at all.
The backfill is lossy and irreversible, so it reports by default and needs --apply, writes to a temporary file and renames so an interruption cannot leave a half-written image being served, and is idempotent by construction: a file already stripped and already within bounds is skipped rather than put through a second lossy pass. That property is a pure function with its own unit test, because being wrong about it degrades every image a little more on every run.
Two traps the plan handles that the issue only named. An animated WebP read without the animated flag decodes to a single frame and is silently written back as a still, so the flag is set for WebP and only WebP — it changes how resize reads height, which would be wrong for the other types. And sharp before 0.33 has no withExif, which the tests need to build their fixture; on an older version they fail as though the stripping were broken.
Ref #226
Two tasks added to the slice-1 plan, closing the half of the upload gap the middleware ordering does not.
The ordering fix stops a caller with a bad token writing anything. A caller with a working one can still send six eight-megabyte files per request against a limiter that allows twenty requests a window, and nothing checks whether the volume can take it. That volume is shared with the admin upload path, so intake filling it is a shop outage rather than an intake outage.
Task 8 refuses an upload when less than a gigabyte remains, on the admin item routes as well as intake, failing closed because a volume that cannot be measured is not one to assume is empty. Task 9 makes an absent cap mean the bounded default of twenty-five rather than unlimited: the router as written treated omission as "no limit", so the ordinary act of creating a link produced an unbounded one, and a cap that has to be remembered is not a control.
Two larger findings are filed rather than folded in. Re-encoding uploads to strip EXIF and cut stored bytes (#226) touches the shared pipeline and adds a native dependency; a global ceiling with an abuse alert (#227) needs its own state and an email. The EXIF one is worth stating plainly: nothing strips metadata today, so an uploaded phone photo publishes the coordinates it was taken at, at a public URL. That is already true of the admin path and is not introduced here, but this slice widens who can put such a file there.
Ref #220
The first of four slices from the intake design, and the only one that is worth planning in detail yet — the later slices' shape depends on what this one actually produces.
Seven tasks: the schema, extracting the validated image-upload pipeline out of routes/admin.ts so the public endpoint reuses it rather than growing a near-copy of it, token generation and hashing, the admin API for issuing and revoking links, the public submission endpoint, the submission page, and the admin screen.
Three things the plan settles that the design left open or got wrong. The image caps become the constants already in the codebase rather than the 10-photo and 10 MB figures the design invented, because two different caps on one pipeline is a defect waiting to happen. The feature flag is dropped from this slice: nothing is reachable until a link exists, and the flag earns its keep in slice 2 where a paid API call appears. And the link is resolved before multer runs, so a stranger holding a bad token cannot cause a byte to be written to the uploads volume — cleanup afterwards would leave an unauthenticated caller in control of disk churn, and leans on an unlink that a crash between write and delete would skip. That ordering is asserted by a test, so a later reordering fails loudly rather than silently.
Ref #220
Task 1 leaves 4 tests and Task 2 adds 7, so the running total is 11 rather than 12, and the same off-by-one carried into Tasks 3, 4 and the completion criteria. Caught by the Task 2 implementer, which flagged the mismatch rather than inventing a test to reach the stated number.
Nine tasks, each ending in something independently testable, in an order where every task leaves both screens working. The dimensions are built and unit-tested first, the shell after them, and the two screens are wired last — so the old components stay in place until the thing replacing them is proven.
The two behaviour changes the design accepted are each covered twice: a unit test on the chips that produce them, and an end-to-end assertion on what a person sees. The admin tally reading three for three statuses, and a non-default availability producing a removable chip.
Task 8 carries the deletions, deliberately last. Removing `activeFilterCount` and `hasActiveFilters` turns every remaining caller into a compile error, which is the cheapest way to find them.
Two defects found reviewing the plan against the code rather than against itself: the test fixture omitted `Category.item_count` and would not have compiled, and Task 9 added a page object method that nothing used — `chooseAvailability` and `filterChip` already exist.
The plan also records what must not be read as a regression. Two assertions in the storefront specs fail on every branch because the unpaginated grid cannot render 1,600+ development rows inside Playwright's default timeout, which is #186 and predates this work.
Refs #188
Four tasks, each ending in an independently testable deliverable: the backend endpoint with its own rate limiter, the boundary and fallback components, the three mount points with end-to-end coverage, and the production-gate verification.
Refs #62
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
bermudalambandClaude Opus 5