Pure and separately tested because the failure it guards is silent. Items are priced on arrival, so the schema no longer stops a number nobody chose reaching the storefront — the review queue does, by showing that nobody chose it, and an item selling at a default price looks exactly like one selling at a chosen price.
Editing the number is the only thing that confirms it. Publishing an untouched field deliberately does not, because that would record "I did not look at this" as "I approved this".
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Six tasks: the price provenance rule as a pure unit, the list endpoint, publish, the three state actions, the screen, and an end-to-end pass.
The price field is the reason this screen exists. Items are priced on arrival, so the schema no longer prevents a number nobody chose from reaching the storefront — that protection moves here, into presentation, where it is weaker. So the rule is a pure tested function rather than a line inside a route: editing the number is the only thing that confirms it, publishing an untouched field deliberately does not, and publishing something still unconfirmed asks first rather than reporting afterwards. 80.00 was chosen because it reads as a decision rather than as an obvious sentinel, which is exactly why it has to be called out rather than left to be noticed.
Discard deletes nothing. It is one click from an inbox, and the photos are often the only copy of an item no longer in the sender's hands, so it marks the draft and returns the item to pending. Restore brings it back at the state its own contents justify rather than unconditionally ready, because a submission discarded before it was ever drafted has no copy and must not return claiming otherwise.
Regenerate clears attempts along with the state. The worker only picks up rows below the attempt cap, so re-queueing a draft that already failed three times would otherwise produce a button that appears to work, does nothing, and says nothing.
The end-to-end test seeds through the intake route rather than POST /api/admin/items, which writes no item_drafts row and would never appear in a queue that joins it.
One deviation from the issue is recorded in the plan rather than buried: it asks for the existing admin item components to be reused, and this builds a purpose-made card instead, because the fields differ in kind rather than arrangement. The cost — two places rendering a name, description and price — is named there too.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The standing cleanup, three features behind. Four changes.
Report the measures in CI. This is the one that matters, because the rest was only findable by reading the tree. SonarQube here is 9.9 Community: no Bearer auth, so the official MCP cannot connect, and the host is a CI secret, so hotspots, duplication, debt and coverage existed only on a dashboard — which made "reduce the debt" an instruction nobody could act on without a browser open beside them. scripts/summarize-sonar.js queries the measures API with the secrets the workflow already holds and prints the result into the job log. The scanner masks the URL and token; measures are not secret.
It polls the compute task before reading. The workflow does not set sonar.qualitygate.wait, so the scan step returns once the report is uploaded and the server computes measures afterwards — reading immediately would return the previous analysis, indistinguishable from this one and quietly wrong. When it cannot confirm, it says so in the output rather than presenting stale numbers as current. It is deliberately not guarded with continue-on-error: it exits 0 on every path, and guarding it would oblige it to appear in the final gate, whose job is to fail the build.
Remove the Tinqer spike. #216 evaluated Drizzle against Tinqer and rejected Tinqer, and its closing comment said the throwaway src/db-tinqer/ probe must not reach main. The whole spike commit was merged, so it did. The probe is 71 lines imported by nothing, and @tinqerjs/tinqer, @tinqerjs/pg-promise-adapter and pg-promise were dependencies for a library nobody chose. The condition_note column that warning also named did not reach main.
Clear the lint debt, both projects now at zero warnings from six and two. One of these was a real defect rather than tidiness: the third catch block in shippingAddresses.ts rolled back and returned 500 while discarding the error, so a failed default-address change left nothing behind to say why — the two catch blocks above it in the same file already logged, and this one had simply been missed. The Express namespace augmentation is a false positive and is disabled with the reason written beside it, because an interface that must merge into one Express declares inside a namespace has no ES module spelling.
Dedupe the extension map. backfillImageReencode.ts kept its own .jpg/.png/.webp table whose comment named uploadTypes.ts as the source of truth, directly above duplicating it. That file rewrites stored images, so the two disagreeing would silently skip files it should re-encode.
src/db-drizzle/ deliberately stays. #217 is open to promote exactly those files properly, with tablesFilter and the sql.param() array rule; deleting them here would be doing #217 badly in the wrong issue. Only their unused-symbol warnings are fixed, and if drizzle-kit pull regenerates schema.ts the table warning returns — worth #217 knowing.
Hotspots and coverage are untouched because both numbers are still invisible. They are the next pass, once the step above has printed them once.
Closes#261
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
QA_SMTP_HOST, QA_SMTP_PORT and QA_SMTP_SECURE were set on the stack but nothing read them: the compose file hardcoded all three, so changing one in Portainer had no effect and gave no sign of that. They now interpolate like the credentials beside them.
Each keeps its Brevo value as a default rather than being left to fall through. The mailer's own fallbacks are Gmail's — smtp.gmail.com, 465, implicit TLS — and Brevo is STARTTLS on 587, so an unset variable with no default here would quietly aim QA at Gmail and fail at send time rather than at boot. `:-` supplies the default only when the variable is unset or empty, so setting one still wins.
Verified with `docker compose config` both ways: unset resolves to smtp-relay.brevo.com/587/false, and set resolves to the supplied values. The #107 compose guard still passes.
Production is deliberately untouched. It hardcodes the same three and nobody has asked to vary them there, and a needless change to the production stack is not worth the deploy.
Closes#258
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Playwright matches an accessible name case-insensitively and as a substring unless exact is passed, so `getByRole('button', { name: 'OK' })` matched any button whose name merely contained "ok".
Caught on a full serial run of main:
strict mode violation: getByRole('button', { name: 'OK' }) resolved to 2 elements:
1) aka getByRole('button', { name: 'Freed rmtiq9okg22k9e' })
2) aka getByRole('button', { name: 'OK', exact: true })
A leftover "Freed …" toast was still on screen and the random base36 suffix happened to contain "ok". Roughly one suffix in a few hundred does, which is the profile of a test that fails occasionally and reproduces for nobody.
This is not the contention #241 addressed. It reproduced with workers: 1, serially, on a fresh database — it needs only a stale toast and an unlucky suffix. It is at least part of what #245 skipped a real test to work around.
Also makes the taxonomy modal's OK and the sold-filter 'All' radio exact, the only other targets short enough to appear inside a random suffix. The dozen or so remaining short names are left alone deliberately: they would need a collision in deliberately-chosen test data rather than a random one, and substring matching is load-bearing in some of them.
Verified with two consecutive full e2e passes, 155 of 155 both times.
Closes#253
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
main did not build, so every Portainer deploy failed with `npm run build` exit code 2. My regression from #241.
findOrFail<T>(items: T[], predicate: (item: T) => boolean) infers T from both parameters. The call sites annotate the predicate as documentation, and the arrays come from .json(), which is any and offers no competing candidate — so T became the one-field shape written in the lambda and every caller failed on the field it actually wanted. Array.prototype.find has no such problem, which is why the code this replaced type-checked.
The four responses are now typed at their call sites, so T is inferred from real data and the predicates need no annotation. findOrFail additionally takes NoInfer<T> on its predicate, so a stray annotation can never drive the element type again. The specs are better typed than before this change: `.json()` was plain any, and the annotations only ever documented a shape nothing enforced.
I did not catch this because I verified with a bare `npx tsc --noEmit`, and tsconfig.json is `"include": ["src"]` — it structurally cannot see tests/. The specs are checked by the second command in `npm run build`, which is the step Docker runs and the step that failed. Verified this time with `npm run build` itself, plus lint, the unit suite, and two consecutive full e2e passes.
Closes#254
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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>
Two drivers. The call after a successful submission means a draft is usually waiting by the time anybody looks; the five-minute sweep means a restart mid-draft is recoverable rather than a permanently stalled row, and picks up whatever the first call missed.
Neither is awaited. A slow or failing model must not become a failed upload for someone who did nothing wrong, which is the whole reason drafting does not happen inline — the cost of a dropped call is a few minutes' delay, not a lost submission.
Both catch for themselves. The comment beside the existing schedulers points out that `void` is only safe because those functions handle their own errors, and draftQueued does not: its first query can reject, and an escaping rejection would take the container down.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The governing rule is that a submission is the only irreplaceable thing in this pipeline. The photos are often the only copy of an item no longer in the sender's hands, so a missing key, an unreadable file, a failed call and three exhausted retries all end the same way: the item keeps its photos, stays pending, and waits. Nothing in this file deletes anything.
An absent key returns early and spends no attempt. Counting it as a failure would mean a fortnight without a key exhausted the retries and marked every waiting submission failed, with nothing wrong with any of them.
A failure leaves the row queued while tries remain, so the sweeper picks it up again, and failed once they are spent, so a dead submission stops costing money and waits for a person instead of retrying forever.
Photos are read once and passed down rather than loaded again inside the drafting call — the first read already has to happen to check there is at least one.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The copy goes on item_drafts, never on the item. The item keeps its placeholder name and description until a person approves them in the review queue (#225) — nothing a model wrote reaches the catalogue unreviewed.
The price is the deliberate exception, because #220 chose to price an item on arrival rather than leave it unpriced. price_source records that the number came from a model rather than a person, so the review queue can show it as unconfirmed. With no suggestion the item keeps the migration's 8000 default and price_source stays 'default'; the queue shows both the same way, as a number nobody has chosen yet.
A category is checked against the real table before it is stored. The schema constrains the shape of the answer but cannot enforce membership, and a category the shop does not have would be invisible to every storefront filter — a draft nobody could find, rather than an obvious error.
A successful retry clears ai_error, or a draft that eventually worked would still read as broken in the queue.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The client is a parameter rather than a module import, so every test passes a stub. A test that reaches the real API is a defect in the test: this runs on a route a stranger with a link can trigger, and each call costs money.
getAnthropicClient returns null rather than throwing when there is no key. An unconfigured environment is a working one, and the worker treats null exactly as it treats a failed call — one path rather than two.
parsed_output is guarded, not asserted. The SDK returns null there when the answer did not satisfy the schema, which is what a model replying in prose looks like; failing cleanly leaves the submission queued for a retry, where asserting would crash the worker mid-loop. Absent usage figures are treated as zero for the same reason: undercounting a cost is survivable, throwing away a draft that actually succeeded is not.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The model was going to be an environment variable, which meant a redeploy to change it. It is now an admin setting, so it can be changed from the Settings page like the cart expiry and the greeting.
A dropdown validated on the server, not a free-text field. The API only rejects an unknown model at the point of use, so a typo would be stored happily and then fail on every submission, surfacing as drafts quietly not appearing rather than as an error anybody could act on. The PUT refuses anything outside the offered set, and getSettings falls back rather than handing on a value that is no longer offered — drafting with the default beats drafting with a model the API will refuse.
One catalogue rather than two lists. The dropdown needs the models, costMicros needs their rates, and the price shown beside a model in Admin has to be the price it is actually billed at, which it cannot be if the two are maintained separately. Rates were confirmed against the pricing page rather than recalled: Sonnet 5 $2/$10, Opus 5 $5/$25, Haiku 4.5 $1/$5 per million tokens. The unknown-model fallback is deliberately the most expensive rate and never zero, because a budget that reads as unspent however much was spent is the one failure a spend guard cannot have.
Adding a third setting type pushed getSettings past the cognitive complexity limit, so the per-type resolution moved out into one small function each — the same shape the definitions block above it already argues for.
The exhaustive assertion in the GET test gained the new field rather than being loosened. It exists to catch a setting silently vanishing from the response, and that is worth more than not having to touch it here.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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>
Pure and separately tested, because this is where the correctness of every draft is decided. Nothing downstream can distinguish an observed detail from an invented one — the description arrives as prose either way — so the instruction is the only place that distinction can be enforced, and the tests assert it is actually present.
On a one-of-a-kind item an invented "1930s hand-thrown stoneware" is not a cosmetic error but a false claim on a public shop, and the shop answers for it rather than the model. Visible damage is called out for the same reason in reverse: a buyer finding a chip on arrival is worse than reading about it beforehand.
Categories and tags are listed rather than described, so the model chooses from what exists instead of inventing a taxonomy the storefront filters know nothing about, and declining is explicitly allowed so a model with no matching option does not pick the closest wrong one.
The note is quoted and labelled as the sender's rather than merged into the instruction: it is untrusted text from an unauthenticated stranger and should read as evidence to weigh, not as something the shop asserts. A whitespace-only note counts as no note.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The SDK validates the model's response against this before any of it reaches the database, so a model that answers in prose or invents a field becomes a caught error rather than a row full of nonsense.
Everything the model may decline to answer is nullable, because it is told to say nothing rather than guess. A null category is a better answer than a wrong one, and resolving it is what the review queue is for. The name and description are not nullable: a draft without them is not a partial success worth storing.
The price is an integer, bounded at both ends. A fractional, negative or absurd figure reaching the review queue is a number somebody has to notice is wrong, and being trustworthy at a glance is that queue's whole job.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Both packages go in dependencies rather than devDependencies. The final Docker stage installs with --omit=dev, so the wrong section produces a container that fails on the first submission and nowhere else — which is how sharp went wrong in #226.
ANTHROPIC_API_KEY is a warning, not a requirement. Absent, the container still boots and a submission still arrives, keeps its photos and waits in the queue undrafted. The photos are often the only copy of an item no longer in the sender's hands, so losing a consignment to an expired key would be a worse outcome than an item arriving without its description written. Silence would be wrong too: an operator who believes drafting is on and finds every item undrafted has nothing to tell them why.
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>
The drafting worker needs a credential, and a Portainer stack variable alone does not reach the container — stack variables are interpolated into the compose file as ${VAR}, and a service receives exactly what its own environment block lists. That is how UPLOADS_DIR went missing in #118, and both files say so; this adds the line that makes the variable actually arrive.
QA takes it from QA_ANTHROPIC_API_KEY, prefixed like the database and SMTP credentials so production's key cannot be pasted there and silently work. It is also worth a key of its own rather than sharing production's, because this is the only credential in either stack that spends money per call, on a path anybody holding an upload link can trigger.
Absent is a working configuration in both, deliberately, which is why prod's line carries `:-` and neither variable joins the always-required list. A submission still arrives, keeps its photos and waits undrafted. Losing somebody's consignment to an expired key would be far worse than an item arriving without its description written, and the photos may be the only copy of something no longer in the sender's hands. USPS is the existing precedent for a credential whose absence degrades rather than fails.
The comments say plainly that a spend limit belongs on the key in the Anthropic console, since nothing in this repository can enforce one and #227's submission ceiling bounds the volume rather than the bill.
docs/ops/production-stack-cutover.md said the compose file interpolates thirteen names and listed them. It now says fourteen, because that document stakes its usefulness on being checked against the file rather than written from memory — a cutover working from a stale list is how a variable gets left behind, which is the failure the document exists to prevent. Counted from the file: exactly fourteen.
composeEnvironment.test.ts passes, 24 tests. It checks that every deployment sets what the validator requires, so adding a variable ahead of a validator entry cannot break it — the entry itself comes with the worker.
Ref #223
The lever the plan deliberately held back, applied now that there is evidence it is needed. Better failure messages and a wider assertion timeout were not enough.
Measured on this branch, same commit, same machine. Two parallel runs each failed 3 of 155, and not the same three: password-reset and admin-inventory-filters in one, admin-email-settings, admin-inventory-filters and resend-verification in the next. All 16 tests from those three specs then passed when run serially, and the full suite passed 155 of 155 twice in a row. Failures that move between runs of identical code are contention, not defects — specs asserting over tables other specs are concurrently writing to.
The cost is about three minutes, roughly one minute parallel against 3.9 and 4.0 serially. A red run that means something is worth three minutes; the previous state was a suite whose result nobody could act on, which is what #245 had to skip a real test to work around.
If that time is ever needed back, the cheaper fix is giving each worker its own database rather than raising this number and reopening #241.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Tasks 2 to 4 of the plan. Nine `collection.find(...)` dereferences become findOrFail, so a missing row fails as a named assertion naming what was wanted and how many rows were searched, rather than "Cannot read properties of undefined" pointing at test plumbing. Where the old code followed the lookup with expect(x).toBeTruthy(), that assertion is dropped: findOrFail already guarantees it, and with a better message.
The expect timeout goes from Playwright's default 5s to 10s. It costs nothing on a green run — it bounds how long a failing assertion waits, not how long a passing one takes — and #239 died reporting exactly Timeout: 5000ms on a runner that also builds, migrates and runs three other suites.
The admin-save happy path comes back from the #245 skip. It is the only end-to-end check that adding an item reaches the database rather than merely firing a toast, and it passed both full parallel runs and in isolation.
filters.spec.ts:216 is deliberately untouched: its .find() searches CSS class names on a string array, not test data, and has no missing-row failure mode.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Adding e2e specs for the submission page found a defect in the page they were written for, which is what they were for.
One limiter counted page loads and submissions against the same twenty-per-quarter-hour allowance, so a sender working through a box of stock ran out after ten items — the exact person the feature exists for, and the exact case the limiter's own comment said must not be refused. The comment said refusing them costs a consignment while the number quietly did it.
Worse, the page could not tell a 429 from a 404. `fetchIntakeLink` treated any non-OK response as "no link", so a throttled sender was told "This link is not active" and sent to ask for a replacement — which could not have helped, because the problem was their address and a minute of patience. Two conditions needing opposite reactions were sharing a message.
Now two limiters, because the two requests cost different things. Reading a link hits one indexed row and writes nothing, so that allowance is generous at 120: someone re-reading the form or losing their signal should never be told to wait. Submitting writes up to six files, so that is the one worth bounding, at 30 — more than anyone photographing items can manage and far less than a script would want.
The page gains a third state. Unknown, revoked and used-up still collapse into one "not active" card, because whether a link exists is not something a stranger needs to learn. Throttled is deliberately kept apart from them, since "wait a moment" and "go and ask for another link" are opposite instructions.
Measured rather than assumed, on a freshly started process both times: before, 25 page loads produced 14 rejections; after, 40 produce none. The first attempt at that measurement was wrong and worth recording — the restart had failed with EADDRINUSE, so it read 30 of 30 against the old process's already-exhausted store.
The two specs now pass in a full parallel run alongside everything else. They are scoped the way #241 asks: unique run ids, assertions naming only this run's rows, nothing asserted about the table as a whole.
Backend: 284 integration, 309 unit. Frontend: build clean, lint unchanged at 2 pre-existing warnings.
Ref #222, #241
An Upload links tab beside Tags: issue a named link, see how much of its allowance is spent, revoke it. Until now the only way to create one was curl, which is how the earlier tasks were exercised.
The token is shown once, in an alert that says so plainly, because the server stores only a digest and genuinely cannot produce it again. A refresh loses it — that is the honest behaviour rather than a bug, so the copy says to revoke and reissue if it is lost instead of leaving somebody hunting for a reveal button.
The cap field starts at 25 and unlimited is a checkbox rather than an empty field. Blank-means-unlimited would make the least deliberate action produce the least bounded link, and this screen sends all three cases explicitly so the server's default only ever has to cover callers that are not this screen.
Two things came out of driving it in a browser rather than reading it. The revoke confirmation said "OK", and every other destructive confirm in this admin names its action — Delete, Disable, Re-enable — so it now says Revoke, in danger styling. A confirm button reading OK makes the reader go back and re-read the question to find out what they are agreeing to. And Popconfirm turned out to be a component nothing else here uses; the rest use Modal.confirm with an explicit okText. Keeping Popconfirm but matching its labelling to the established pattern seemed the smaller inconsistency, since the interaction is a row action rather than a page-level one.
The load-on-mount effect carries the same eslint-disable and reasoning Tags and Categories already use, rather than a new shape.
Verified in a browser: create shows the one-time reveal, the row lists as 0 of 25 and Active, revoke flips it to Revoked, and an explicitly unlimited link shows a bare count with no cap. The database then confirmed a default of 25, a null for the unlimited one, and a stamped revoked_at.
Frontend: build clean, lint unchanged at 2 pre-existing warnings, 30 unit tests pass.
Ref #222
Where someone with no account sends in photos of one item. Route /submit/:token, outside the authentik gate by design: the token in the URL is the whole access control, which is what #222 chose deliberately over accounts.
One state for every refusal, matching the server's single 404. Unknown, revoked and used-up links all render the same "this link is not active" card, because saying which kind of dead it was would tell a stranger whether a link they guessed at exists — the server is careful about that and the page must not undo it.
`beforeUpload` returns false so antd keeps the files rather than uploading each one as it is picked. The submission is then a single request the server can accept or refuse as a unit, which is what makes the transaction on the other side meaningful.
The accepted types and the six-file cap are stated here so the picker offers exactly what will be taken, but both are checked again server-side, because everything on this page is under the sender's control.
The fetch effect guards against a late response from a previous token overwriting the current answer, which is reachable simply by editing the URL.
TypeScript caught a real mistake rather than a stylistic one: `.filter((f): f is File => ...)` on antd's originFileObj does not narrow, because RcFile extends File and the predicate would widen rather than narrow. flatMap avoids the predicate entirely.
Verified in a browser rather than by inspection: a throwaway Playwright run against the live stack confirmed the form renders for a good token, the inactive card renders for a bad one, and a photo can actually be sent and acknowledged. The database then showed the item at status pending with the default price, the draft carrying the note and its originating link, the image row written, the link's counter at one — and zero storefront-visible items, which is the property that matters most.
Ref #222
The public way in. Photos of one item plus a free-text note, from someone with no account, landing as an `items` row at status 'pending' — already invisible to every public and storefront query since #90, so nothing is live by accident.
Every refusal is a 404. Unknown, revoked and exhausted links are indistinguishable from outside, because whether a link exists is not something a stranger needs to be able to learn — the same reasoning uploads.ts applies to files.
The link is resolved *before* multer runs, and that ordering is the point rather than an implementation detail. discardUnlessAccepted would delete the files afterwards, but "written then deleted" is materially worse than "never written" on an endpoint the whole internet can reach: it is disk churn an unauthenticated caller controls, and it leans on an unlink that a crash between write and delete would skip. A test asserts the volume is untouched for a bad token, so a future reordering fails loudly instead of quietly handing that control away.
The link counter is incremented inside the transaction and guarded on the same conditions as the lookup, so two submissions racing for the last slot of a capped link cannot both succeed. The response carries no item id: the sender has no business knowing about the catalogue and nothing they could do with it.
The AI is deliberately not called here. A slow or failing model request must not turn into a failed upload for someone who did nothing wrong, and the photos may be the only copy — the item is often no longer in the sender's hands. The row waits at state 'queued' for #223.
The new limiter keys on the caller alone, since a submission carries no email. keyByCallerAndEmail's comment warns that a bare ip bucket is a shared allowance, and that trade is taken knowingly: the link is the per-caller identity and its cap is the per-caller bound, while this limiter does the different job of bounding what one address can throw at an endpoint that writes files. Twenty per fifteen minutes is deliberately looser than the password-reset allowance — somebody photographing a box of stock legitimately submits several in a row, and refusing them costs a consignment.
Because the route mounts the shared uploadImages, it inherits the type allowlist, the magic-byte check and #226's EXIF stripping without asking for any of them. A test asserts the stripping specifically, since this is the route where it matters most: the photo comes from a stranger's phone rather than the shop's own camera.
Backend: 284 integration (12 new), 309 unit, lint unchanged at 6 pre-existing warnings, build clean.
Ref #222
Three routes behind the admin gate: list, create, revoke. A link is named because provenance matters more than convenience — when one is shared further than intended the question is which one, and every submission will record the link it arrived through, so revoking kills that link rather than the feature.
The token is returned by exactly one response and is unrecoverable afterwards, which is why the admin screen has to present it as a one-time reveal. The listing selects its columns explicitly rather than `SELECT *`, so `token_hash` cannot reach a response the moment somebody adds a convenience — and a test asserts the listing carries neither the token nor the digest.
An absent `maxSubmissions` gets a bounded default of 25 rather than null. Absent means nobody decided; an explicit null means unlimited, which is a decision visible in the request. Reading absent as unlimited is what would quietly make every link unbounded, and the common case is the one that has to be safe.
Revoking is idempotent through COALESCE, and a test asserts the second call returns the *same* timestamp rather than merely succeeding. The useful fact is when access ended, and a button that errors on a double-click teaches people to distrust it — which is the last thing wanted on the control that contains a leak.
Mounted above the `/api/admin` catch-all, which would otherwise swallow the path, and behind requireAdminGate on the router itself per the reasoning in middleware/adminGate.ts.
Lint caught me reintroducing something this codebase had already solved: I wrote `.replace(/\/+$/, '')` to trim PUBLIC_URL, and app.ts carried a hand-written loop with a comment explaining that exact regex backtracks. Rather than duplicate the loop, trimTrailingSlashes moved to utils.ts and both callers now share it.
Backend: 272 integration (9 new), 308 unit, lint back to its 6 pre-existing warnings, build clean.
Ref #222
The token is the entire access control on an endpoint the whole internet can reach, so both halves are pure and tested directly rather than through a request — the same reasoning that has uploadTypes.ts and keyByCallerAndEmail exported for their tests.
32 bytes of CSPRNG output, base64url so the value survives being pasted into a URL, a chat message or a QR code without escaping. That matters for something a person is handed rather than something a machine reads. The collision test runs a thousand generations rather than asserting the obvious, because a repeat would mean one person's link opening another's.
SHA-256 rather than bcrypt, and the reasoning inverts the one that governs passwords. A password hash is slow on purpose because a human password carries little entropy and must survive an offline dictionary attack. This is 256 bits from a CSPRNG: there is no dictionary, so slowing the hash buys nothing. Meanwhile the digest is computed on every submission to an unauthenticated endpoint, where a deliberately slow hash would be a denial-of-service surface — #242 is the local proof that cost-12 hashing on a request path is enough to push it past a timeout under load.
No timing-safe comparison, deliberately: the lookup is an indexed equality match on the digest rather than a byte-by-byte compare of the secret, and an attacker who could mount a timing attack against a 256-bit random value would still need the value.
Backend: 307 unit tests, lint unchanged at 6 pre-existing warnings, build clean.
Ref #222
A pure move, no behaviour change. #222's public intake endpoint needs the same path from a multipart request to files on the uploads volume that the admin routes use, and the alternative to sharing it is a near-copy that has to reproduce every safety property exactly: the type allowlist, the magic-byte check after the write, names from a CSPRNG rather than from `originalname`, the re-encode that strips EXIF, and the cleanup of whatever a refused request left behind. A copy that drifted on any of those is the gap #95, #103, #180 and #226 exist to close.
`stripUploadedImages` moved with the rest, which the plan originally did not say — it was written before #226 added it. Leaving it behind would have given the intake route an upload path that skips EXIF stripping, and no test would have failed to say so, because the intake tests are written against a route that does not exist yet. The refreshed plan added a check with a definite answer, and it now holds: routes/admin.ts no longer imports imageProcessing at all.
Every comment came across verbatim. They record why the code is shaped as it is and are the most valuable part of what moved.
Lint caught something the compiler did not: MAX_IMAGES_PER_REQUEST was left imported into admin.ts, where its only use — `upload.array('images', MAX_IMAGES_PER_REQUEST)` — had moved away with the middleware. It stays exported from imageUpload for the intake route's caps, but admin.ts does not need it.
Verified as a refactor rather than as a change: the four suites that exercise this path hardest were run before the move and after it, 44 tests both times, same suites, same count. Full backend: 263 integration, 302 unit, lint back to its 6 pre-existing warnings, build clean.
Ref #222
The schema for the intake pipeline. A submission becomes an `items` row at status 'pending' — already invisible to every public and storefront query since #90 — with an `item_drafts` row beside it holding the submitter's note, which link it arrived through, and the fields the drafting worker will fill in later.
`upload_links` stores a digest rather than a token, so a leaked database is not also a leaked set of working links, and the admin screen can show a token exactly once. `max_submissions` is nullable for "no cap", but the route will default it to a finite number: an unbounded link should be something asked for, not something that happens when nobody thought about it.
`item_drafts.upload_link_id` is ON DELETE SET NULL rather than CASCADE. Deleting a link must not delete the items that arrived through it — provenance is lost, the goods are not.
`items.price_cents` keeps NOT NULL and gains a default of 80.00, so an arriving item is always priced. That is the decision taken in the design review over making the column nullable: it costs the schema-level guarantee that nothing can publish at a price nobody chose, and buys not having to teach the cart, the checkout and thirteen other files about an item without a price. The protection moves into the review queue, and `price_source` exists so that queue can say whether a number came from a model, the default, or a person.
The number lives in the migration rather than in configuration. Changing a default price is a rare, deliberate act that deserves a record; an environment variable would let it drift silently between environments, and a wrong default is invisible until something has already sold at it.
Verified up, down and up again rather than only forwards — an irreversible migration is one that cannot be tested. Then verified by inspection rather than assumption: the default reads 8000, both tables and the state index exist, and an item inserted with no price comes back at 8000.
Backend: 263 integration, 302 unit, all passing against the new schema.
Ref #222
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
The spike is answered, so the throwaway goes as it always said it would.
What it established. The runner can build images once a docker CLI is installed — the socket was mounted all along and only the client was missing. The container registry works and accepts pushes; it had never been exercised, so that was genuinely unknown. TLS from the runner to the Gitea host is trusted, which also answers the certificate half of the Portainer question. A personal access token with write:package authenticates where the token Actions injects automatically does not. And a full image build and push costs 9m14s on that runner.
What it disproved, which was the point. A CI-built image still reports `commit: "unknown"`. #235 removed `COPY .git` from the Dockerfile to stop the version stamp breaking every Portainer deploy, so it does not matter that an Actions checkout has history — the Dockerfile never copies it. Build location was never the problem, and moving builds to CI would have delivered nothing on its own. #248 carries the actual fix, a build arg, which is a few lines and does not need the registry at all.
Deleting this from main rather than only from the spike branch: it was merged here in #238, before iteration 2 showed that workflow_dispatch fires from a branch and a spike never needed to reach main at all.
Still owed by hand: the spike-trivial and spike-3085970 packages in the registry.
Closes#237
Nine sites across five specs do `collection.find(...)` and dereference the result immediately. When the row is missing the test dies with "Cannot read properties of undefined" naming a line of test plumbing, which says nothing about what was expected — and that is exactly how favorites-filter:169 failed without producing a usable signal.
The message names what was wanted and how many rows were searched. That distinction carries real diagnostic weight: "0 rows" means the fixture never landed, "37 rows" means it landed and the predicate is wrong, and those are different bugs to chase.
It lives in its own module importing nothing, rather than in support/api.ts. That file imports @playwright/test, and vitest.config.ts runs tests/unit with environment: 'node' — putting six lines of pure logic there would drag a browser harness into the unit suite to test them. api.ts re-exports it so specs still reach it through fixtures.
Throws rather than returning null, because every caller wants the row: an error at the point of the miss beats a null threaded through three more lines before something unrelated fails.
Frontend: 30 unit tests pass, lint unchanged at 2 pre-existing warnings, build clean.
Ref #241
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 distinct specs failed across two runs of identical code with no overlap between the sets, so which test fails is decided by the scheduler. The cost is already being paid: #245 skipped the only end-to-end check that adding an item reaches the database, purely to get main green.
The design separates two mechanisms that had been treated as one. Unchecked lookups into shared collections — `collection.find(...)` dereferenced immediately, found at eight or more sites — are a defect regardless of concurrency: when the row is missing the test dies with "Cannot read properties of undefined" naming test plumbing rather than failing an assertion that says what it wanted. Load-induced timing is the other, and is the larger share of what has actually been observed: three of four local failures and the CI one are assertions in a spec's own browser context that nothing else can touch.
Two claims from earlier in this investigation are retracted in the document rather than quietly dropped. The verification-resend limiter is not a shared axis — it is keyed per customer and every test registers its own — and the suite contains no snapshot-style assertions, so there is nothing to convert to web-first. Both were stated as fact on the issue, and both would have justified work that was not needed.
Per-worker databases are ruled out structurally: every worker talks to one backend on :3000, so isolation there means N backends, not N databases. Worker-count reduction is deliberately deferred rather than taken now, because applying it at the same time would mask whether fixing the defects worked.
The honest limit is recorded too. CI's load cannot be reproduced here on demand, so the timing changes rest on reasoning rather than a red-to-green demonstration, and the success criterion is several consecutive green runs rather than one.
Ref #241
`main` has been failing on one e2e test since the sold-filter fix landed, and it is a different test from the one #239 corrected: admin-save-failures' "saves an item successfully when the server accepts it".
Skipped rather than fixed, deliberately. It fails in CI and passes locally, and which test fails moves around — a local parallel run of the whole suite on the same commit failed four *different* specs (admin-inventory-filters, auth, favorites-filter, resend-verification) and not this one. That is #241: fullyParallel against a single shared database. Fixing this test on its own would be guessing at a symptom that reappears somewhere else next run.
Ruled out before disabling anything: the re-encoding from #226 is not involved. AdminInventory.addItem fills a name and a price and saves, attaching no files, so stripUploadedImages iterates an empty array and the image path is never entered. Checked rather than assumed, because this spec is on the admin save route and that is exactly where a regression of mine would surface.
What this stops covering is not trivial, and the comment says so at the call site: it is the only end-to-end check that adding an item actually reaches the database rather than merely firing a toast. #245 exists so that it is un-skipped when #241 lands, rather than left behind. A skipped test on the core admin save path is worse than a red build, because a red build is at least visible.
Ref #245, #241
The integration suite registers around thirty-five customers and asserts nothing about any of their hashes, yet paid bcrypt cost 12 for every one. bcryptjs is a pure-JS implementation, so it pays that cost several times over compared with a native build, and hashing was most of the suite's wall clock. On a contended runner it pushed adminInventory.integration.test.ts past its twenty-second timeout, which then surfaced as a foreign key violation somewhere else entirely — the test timed out, jest moved on, beforeEach truncated, and the still-in-flight registration wrote a token for a customer that had just been deleted.
Measured rather than asserted, warm run against warm run with only the constant changed: 34.5s at cost 12, 9.8s at cost 4. Three and a half times faster, about twenty-five seconds off every integration run, with all 263 tests passing either way.
The first attempt at that measurement was wrong and worth recording. Comparing a cold run at cost 4 against a warm run at cost 12 made the change look like a 36% regression-shaped improvement of the wrong size; the difference was ts-jest and Postgres warming up, not the cost factor. Both numbers above are second runs, and the cost-12 figure was taken twice — 34.3s and 34.5s — before being believed.
Deliberately not configurable. An environment variable here would be a way to weaken password hashing in production by misconfiguration, and nothing needs to tune it. The only route to the cheap cost is NODE_ENV=test, which a deployed container would announce anyway by refusing to serve the built frontend, since app.ts gates static serving on the same value. A setting that quietly degrades a security property should be unreachable rather than warned about, which is the reasoning that already made DEMO_MODE strict.
`hashRoundsFor` is pure and separately tested because the failure it guards against is silent: only the exact string 'test' earns the cheap cost, and an unset NODE_ENV gets the strong one, so the dangerous direction has to be asked for explicitly. Both constants are pinned by assertions too — without that the branch tests pass while the numbers drift to something useless.
Closes#242
main has been red since 2026-08-25. Every SonarQube run reported 147 passed, 1 failed, and it was this test every time — expected "Filters", received "Filters (1)".
The test is stale, not the code. It was last touched on 2026-08-23 in #137; the tally logic changed on 2026-08-25 in #188, which never touched the spec. #188 redefined the tally as the number of chips and moved the availability preset into the dimension system as a bar dimension — one that still emits a chip for any non-default status, deliberately, because without it `?status=reserved` is an empty grid with no Clear filters button and no way out but editing the URL. The test asserted the rule that held before that change.
Counting bar chips differently from drawer chips would restore exactly the per-screen special-casing #188 removed, and the drift it fixed was the admin's tally disagreeing with the storefront's. So the assertion moves, not the tally.
The replacement also checks the tally comes back down when the default is restored. The original only ever asserted one direction, which would pass against a count that incremented and never decremented — worth fixing while the test is open rather than leaving a second gap behind the first.
There is a real wart left standing: `Filters (1)` opens a drawer with nothing selected in it, because the filter it is counting lives in the bar. That is a cost of #188's design rather than a defect in it, and the comment now says so rather than leaving the next reader to rediscover it.
Verified by running the spec in isolation with a single worker: 6 passed.
Closes#239
Throwaway. Deleted once #237 has an answer, whichever way it goes.
#233 was designed on an assumption about the Docker build context and broke every Portainer deploy. The chosen fix for the commit stamp — build in Actions, push to Gitea's registry, have Portainer pull — rests on three more assumptions about infrastructure that nothing in this repository can confirm. This probes them instead of designing around them.
Whether a job here can run `docker build` at all is genuinely unknown: the other workflows use `services:`, which proves the runner can start containers, not that it can build images. Whether the registry is usable is likewise unconfirmed — the packages API answers but lists nothing, so it has never been exercised. And the real build is timed because backend-integration.yml is manual after a job once held this runner for 3h12m, so what an image build costs here is part of deciding whether building on every merge is tenable at all.
The trivial image is built and pushed before the real one on purpose. It separates "can this runner build and push anything" from "does our Dockerfile work here", so a later failure still says which half is broken.
workflow_dispatch only, with no push or pull_request trigger, so merging this changes nothing until somebody presses the button. Bounded by timeout-minutes on the same reasoning backend-integration.yml already documents.
Ref #237
bermudalambandClaude Opus 5