The e2e suite ran against the development database and nothing truncated it. Every run seeded more fixtures and left them, so the unfiltered storefront grew monotonically — 1,662 items by the time #186 was filed — until rendering it outran the assertions' timeouts. It failed locally, passed in CI where the database is fresh, and got steadily worse, which is the combination nobody can act on.
start-local.ps1 -E2eDb runs the stack against a separate container on a separate port with tmpfs storage, so it starts empty every time. Migrations already run on every start, so an empty volume is a working one. The development database is untouched, so anything set up there by hand survives.
Deliberately a third database rather than sharing either existing one. The integration suite truncates between tests, so an e2e run sharing with it would have its fixtures deleted underneath it (#116) — different container, different port, different credentials, so the mistake is impossible rather than discouraged.
Verified by recreating the database from the compose file, confirming it came up with zero items, and running the full suite against it: 157 of 157. An earlier attempt at this reported 23 passed and 53 not run, which was worthless — a stale backend from a previous run still held port 3000 and was talking to a database I had already deleted. The port is checked before the run now, and the same mistake produced a wrong rate-limiter measurement earlier in this work.
Pagination is the other half of #186 and is filed separately: a shop that renders its whole catalogue in one page is worth fixing on its own merits, not as a side effect of a test fix.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
discardUnlessAccepted unlinks from a res.on('close') handler, so nothing awaits it and nothing can. `await request()` resolves when the response completes, which is when close fires — so a read taken straight afterwards races the unlink it is meant to observe. The property is an eventual one and the assertions were synchronous.
The issue counted three tests. Injecting a 400ms delay into the unlink to make the race deterministic showed five: "leaves nothing on the volume when it refuses the content" and "discards the valid files from a request that also carried an invalid one" race too, and are not in the describe block the issue named.
It also showed that polling alone is not enough. The race runs in both directions, and the second direction is easy to miss: a deletion still pending from the *previous* test corrupts the next test's baseline before its request is even sent. No amount of waiting fixes a baseline that is already wrong. My first attempt waited for the directory to look quiet, which only works while the unlink is faster than the wait — precisely the assumption this issue is about, and it still failed four tests under the injected delay.
So the baseline is removed as a variable: beforeEach empties the directory, every test starts from empty, and the assertions poll for the expected count. Any orphan from an earlier suite goes with it, which is correct — the directory is temporary and nothing outside these tests owns it. discardUploads already catches per-file errors, so a pending unlink finding its file gone logs and moves on.
Verified by injecting the 400ms delay again: four to five failures before, twelve passing after, with the production file restored untouched.
Closes#228
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
node-pg-migrate keeps the schema; Drizzle is for queries only. The conventions doc said this was unsettled and now says what was settled and why: drizzle-kit cannot diff expression indexes and emitted six statements for one column, our migrations are mostly prose that generated SQL does not carry, and data migrations cannot be generated at all.
The reasoning in full is on the issue. This is the version a reader converting a query will actually find.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Columns in a sql template render unqualified, so a correlated subquery correlates with itself and returns a plausible wrong number rather than failing. That is worse than the array trap already recorded here, which at least produces invalid SQL — this produces valid SQL and quietly wrong data, and only an integration test asserting a value caught it.
And a driver error code moves when Drizzle wraps it, so a catch keyed on a SQLSTATE still compiles and silently stops matching.
Also records that the camelCase mirror and the snake_case API mean every select must map columns explicitly, because selecting the table changes the JSON contract with nothing to notice.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
All nine sites, chosen because the file is awkward rather than easy: a recursive CTE consumed two ways, a correlated subquery, an array match, and two error paths keyed on a Postgres SQLSTATE. A file of plain CRUD would have produced a flattering number that does not generalise.
The hand-declared row interfaces are gone. CATEGORY_COLUMNS is written once and the row type is inferred from it, which closes the drift itemSelect.ts documents as "KEPT IN STEP BY HAND". That mapping has to be explicit rather than selecting the table: the mirror names columns in camelCase and this API answers in snake_case, so selecting the table directly would have silently changed the JSON contract the admin frontend reads, and no test asserting status codes would have caught it.
strict and noUncheckedIndexedAccess hold with no non-null assertions added. requireRow covers the RETURNING rows and the existing lookup destructures and branches, exactly as before.
Two bugs were introduced and caught by the integration suite, and both are worth recording because neither produced a type error.
Drizzle renders a column reference inside a `sql` template UNQUALIFIED. `${items.categoryId} = ${categories.id}` became `WHERE "category_id" = "id"`, which Postgres resolved against items on both sides — so the item count came back plausible and wrong rather than failing. That is worse than the documented array trap, which at least produces invalid SQL. The fragment is now literal text, which is honest since it binds no values.
And the driver's error code moved. Drizzle wraps errors, so the SQLSTATE that sat on err.code now sits on err.cause.code; the old check compiled, never matched, and turned two 409s into 500s. isUniqueViolation accepts both shapes.
inArray replaced the ANY(...::int[]) match and sidesteps the sql.param trap entirely — there is no template to forget it in, and the builder emits the placeholder list correctly by construction.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
backend/drizzle/ was `drizzle-kit`'s output from the #216 spike, superseded now that pull writes into src/db-drizzle. Removing it also removes something that should never have been on main.
#216's closing comment said the spike branch carried "an experimental condition_note column that must not reach main". Checking backend/migrations for it found nothing, which is where I stopped looking last time — but it was here, as backend/drizzle/0001_add_condition_note.sql. It reached main in the same merge that brought the Tinqer probe #261 removed.
Nothing ran it. node-pg-migrate only executes backend/migrations, so this SQL was inert and no database has the column. It was a loaded gun rather than a fired one, which is the only reason this is a cleanup rather than an incident.
That file is also the evidence #219 needs, so it is quoted in that issue before being deleted: adding one nullable column emitted three DROP INDEX statements and three CREATE UNIQUE INDEX statements alongside it, for the expression indexes drizzle-kit could not diff. On a large table those recreations take real locks, and a generated migration nobody read would have taken them.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Infrastructure only. No route is converted, nothing changes at run time.
The mirror had already drifted, which settles how it should be maintained. schema.ts was missing item_drafts and upload_links from the moment #222 landed, because the spike pulled into ./drizzle and copied the file into src/ by hand, and nobody had reason to look at the copy for a week. So `out` now points at src/db-drizzle and pull refreshes in place — the copy step that made the drift possible is gone — and tablesFilter excludes pgmigrations, which is node-pg-migrate's bookkeeping and has no business in a model of the application's schema.
A stale mirror is worse than no mirror, because Drizzle infers row types from it: a converted query would type-check against a schema the database does not have and fail at run time on a column that does not exist. drizzleSchema.integration.test.ts fails when the two disagree, on tables and on columns. It was checked by removing item_drafts from the mirror and confirming the test fails naming it, rather than trusting a green run on a file that already matched.
pull also emits 0000_*.sql and meta/ into `out`, because that directory serves both purposes. Both are gitignored: this project's migration history is backend/migrations, hand-written and mostly prose, and #219 has not chosen otherwise — a stray SQL file in src/ is at best noise and at worst mistaken for real history.
db is exported beside pool and shares its connections. Both must work at once, since conversion is file by file across 187 sites; separate pools would make a transaction on one invisible to the other and silently double the configured limits.
The generated files are excluded from linting. #261 hand-fixed an unused-parameter warning in schema.ts and this re-pull put it straight back, which is the argument in one line: linting generated code buys a fix the next regeneration undoes. itemFilters.drizzle.ts, which is hand-written, is still linted.
CONVENTIONS.md records the sql.param() array trap before anyone hits it — the wrong form type-checks, reads correctly and fails at run time as invalid Postgres — and the reason the adoption is worth doing at all, which is that ${value} emits a bind parameter and there is no way to spell "interpolate this as SQL" by accident.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The alias check was a negative match on an allowlist of English error strings, which returned True for empty output, for $null, and for "exit status 1: Access is denied." — so an alias switch producing nothing, or failing on the symlink permission error this file's own header warns about, was reported as success while the old version kept running. That is the bug #198 was filed about, narrowed rather than removed, and it also broke whenever nvm reworded an error. It is now a positive match on "Now using node v<what is actually running>".
The floor check moves ahead of the switch and reads the constant rather than the result. Where it sat, $major was always whatever NODE_VERSION says, so it validated the switch it had just made instead of the pin it exists to guard, and could never fire.
Use-NodeLatest is now Use-PinnedNode. In a change whose whole subject is that "latest" means something people do not expect, the name was an avoidable trap.
The restore default moves beside NODE_VERSION. It deliberately is not a param default: a param block runs before the dot-source, so $script:DEFAULT_NODE_VERSION is still $null there and the restore would have quietly restored nothing — leaving the machine on the pinned version, which is the exact failure the restore exists to prevent. It is resolved after the dot-source instead, and an explicit -DefaultNodeVersion still wins.
Three documents described behaviour the code no longer has: README's "both scripts run nvm use latest", run-tests.ps1's .DESCRIPTION, and project-context.md's instruction to agents. All corrected, and project-context.md now also says not to run these scripts from an agent shell, which is how this machine once ended up with no Node at all.
Part 4 of the issue is partly stale: backend/package.json already declares engines >=20.9.0. frontend now matches it. The larger question — whether local should be pinned to the Node 20 that CI and the production image actually run — is a decision rather than an oversight and is left open on the issue.
Verified by parsing all three scripts with the PowerShell AST parser, which does not execute them. They are deliberately never run from an agent shell.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Three routes in admin.ts answered a miss with a success. PUT /items/:id ran an UPDATE that matched nothing, committed happily, selected nothing back and replied 200 with an empty body — a success the admin client could do nothing with, and no record anywhere that the item was not found. mark-sold and mark-available did the same. The create route beside them has always used requireRow for exactly this, which is why this reads as an oversight rather than a decision.
A garbage id was worse in a different direction. Number('abc') is NaN, the driver sends it to Postgres as the text "NaN", Postgres raises 22P02 for an integer column, and the catch turned that into a 500 — so a caller asking for an item that cannot exist was told the server broke. Both now answer 404, because from the caller's side "/items/abc" identifies no item in exactly the way "/items/999999" does.
readId is shared rather than repeated, and rejects zero, negatives and fractions as well as text: every id in this schema is a positive serial, so anything else identifies nothing.
mark-sold now notifies favouriters only after the row is known to exist, so nobody is told about a sale that did not happen.
The issue asked for the same shape to be checked across the other admin routes. It was: unpublish already looks the item up and 404s, and the tags and categories PUT routes both do an existence check before their UPDATE, so their rows[0] is guaranteed. items.ts already guards the public read. These three were the only ones lying about a miss.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
SonarQube Community has no branch analysis. Every scan published under a project key replaces that project's single analysis, whatever revision it came from — so each pull request, and each push to one, overwrote the dashboard's analysis of main with the branch. The new-code period, the gate result, the coverage percentages and the hotspot list then all described whatever was scanned last, with nothing on the dashboard saying which revision that was. A gate that went green on a feature branch read exactly like a gate that went green on main.
It was caught only by luck: #180's hotspots reported line numbers that landed on a comment and a blank line in main, which is the kind of nonsense a person notices. Everything else it misreported would have looked fine.
scripts/scan-local.sh has always refused to do this, defaulting to a scratch key, and its header says why in as many words. CI walked into the hazard that script guards against. Now the two tell the same story.
The suites still run on pull requests, which is where their value is — only publishing is restricted. The measures report is skipped alongside the scan, because with nothing published it would print main's numbers into a pull request's log, which is noise at best and misread as the branch's own at worst.
A test asserts both steps carry the restriction and that the three suites do not, because the failure leaves no trace and the `if:` is one line for somebody to drop.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The check is ordered ahead of uploadImages, for the same reason requireUsableLink is: a refused submission must write zero bytes. Ordering it after would accept the upload, store the files and then throw them away, which is the expensive half of the work the ceiling exists to prevent. A test asserts nothing is stored.
503, not 403. The sender has done nothing wrong, their link is fine, and the condition clears by itself as the window rolls — so the link stays usable and works again the moment there is room.
The ceiling never touches the admin upload path, which has its own test. Intake being throttled is an inconvenience; the shop being unable to add its own stock is an outage.
reset-ceiling is declared above /:id/revoke because Express matches in order and would otherwise read it as an id and try to revoke a link named "reset-ceiling". That has its own test too.
The per-link alert is a named function rather than the inline IIFE the plan wrote. routesAreWrapped.test.ts flags any async inside a route registration not directly wrapped in asyncRoute, and it cannot tell an inner IIFE from an unwrapped handler — nor should it have to. The guard caught this, and the extraction reads better than what it rejected.
Backend now 373 unit and 337 integration, all passing.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The count comes from item_drafts rather than a tally. Every submission creates exactly one row in the same transaction that creates the item, so the rows are the truth — a separate counter would be a second thing that can disagree with them, and the one that disagrees silently is always the counter.
windowStart is the part worth testing on its own and the part most likely to be quietly wrong. A reset older than the window must not widen it, which would make the ceiling stricter over time rather than rolling; a reset in the future must not disable it; and a malformed value must not produce an Invalid Date, which compares false against everything and would silently switch off the limit it was set to impose. Each is a test.
Alerts go through sendMail directly rather than the editable templates. An abuse alert is not copy anyone will want to reword, and making it editable means it can be broken — a required placeholder removed from an alert nobody reads until an incident is a poor way to discover the validation.
The throttle is in memory, so a restart during an incident can send one extra alert. That beats writing to admin_settings from the request path on every refused submission; a replicated deployment would have to move it.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A count type beside the existing hours, text and choice readers. resolveHours would have worked — it is parseFloat with a positive guard — but calling a submission ceiling an "hours" setting is a lie in the type name that every later reader has to decode. Whole numbers only, so a ceiling of 12.5 is a typo rather than a preference, and a malformed value falls back rather than yielding a NaN that compares false against everything and silently disables the limit.
resetDb is widened to clear intake_ settings as well as email_ ones. It deliberately does not truncate admin_settings, so a ceiling of 1 left behind by one suite would make every later suite's submissions refuse with a 503, in files that never mention a ceiling. The comment there already records that exact failure happening once with an email template subject, which reached the favorite-alert tests and failed five of them somewhere else entirely.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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>
A gap in the plan rather than in the code: nothing wired the secret into either stack, so the feature would have shipped with its signed links permanently disabled and nothing saying why. Both compose files now interpolate it, with `:-` so an unset variable stays empty rather than failing the deploy.
QA takes QA_INTAKE_ACTION_SECRET, its own value rather than production's, for the same reason as every other QA_ prefixed credential — and more sharply here, because a link signed with it acts on a draft without a login.
Rotating the secret revokes every outstanding link, which is the intended answer to one leaking.
The cutover doc counted fourteen interpolated names and now counts fifteen. That document says it is checked against the file rather than from memory, so it was: fifteen in the compose file, the same fifteen listed.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Mounted publicly, deliberately not behind requireAdminGate. These are clicked from an inbox by someone who is not signed in, which is the whole point; the signature is what protects them.
GET confirms and changes nothing, POST acts. Mail scanners and corporate link-rewriting gateways issue a GET against every URL in a message before a human sees it, so a GET that discarded a draft would fire itself on delivery — carrying a valid signature, looking entirely legitimate in the log, and nobody would know to go and recover it. That is the case the split exists for and it has its own test.
Forged, replayed, upgraded and expired links are each refused with the same 403. Distinguishing them would tell somebody probing which of those they had achieved. There is no signable publish, and asking for one finds no handler.
The two registry guard tests are updated rather than worked around: they assert the full set of settings and template keys, so adding either is exactly what should trip them.
Backend now 367 unit and 329 integration, all passing.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Sent after the draft commits, fire and forget. A mail failure must never mark a draft that was written correctly as failed: the review queue is what the admin actually works from, and the email is a convenience on top of it.
Every quiet path returns rather than throws. No recipient configured is not an error — nobody has said where to send it and the draft is waiting regardless. No INTAKE_ACTION_SECRET means the two shortcut links render empty rather than broken, because a link that could not be verified is worse than none. An item with no draft row simply returns.
A missing description is said plainly rather than left blank. An empty paragraph in a notification reads as a bug; "no description was drafted for this item" reads as the fact that it is, and tells the admin what to expect on the screen.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Editable from the settings screen like every other template. reviewUrl is the only required placeholder: a notification with no link in it still sends, still looks fine in the log, and is useless to whoever receives it, which is what the required-placeholder validation exists to catch.
The two signed links are deliberately optional. They are absent whenever INTAKE_ACTION_SECRET is unset, and a template demanding them would leave an unconfigured environment unable to send this at all.
A test asserts the template offers no way to publish. That the email cannot publish is what bounds the risk taken by pricing items on arrival, and it is a property of the copy as much as of the routes — a publish link in the body would be one nobody reviewed.
Where the notification goes is an admin setting rather than an environment variable, for the same reason drafting_model is one: it is changed by whoever runs the shop, not by whoever deploys it. Empty is the default and means do not notify, which is a working configuration.
INTAKE_ACTION_SECRET warns rather than fails at boot, like the drafting key. Being told an item arrived matters far more than being able to discard it in one click.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Signed over the item, the action and the expiry together. Signing any subset would let a link be replayed against a different item or upgraded to a different action, and leaving the expiry out of the payload would let anyone holding an expired link extend it by editing the timestamp in the URL. Each of those is a test.
Compared through a second digest rather than directly, because timingSafeEqual throws when the buffers differ in length, and a truncated link is an ordinary thing to receive rather than an exception. Same idiom as the admin gate.
actionUrl returns null rather than throwing when there is no secret or no PUBLIC_URL. An unconfigured environment still sends the notification with its review link — being told an item arrived matters far more than the shortcuts do — and a link that could not be verified must never be offered in the first place.
Uses the shared trimTrailingSlashes rather than a trailing-slash regex, which is what utils.ts exports it for.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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>
Seeded through the intake route rather than POST /api/admin/items, which writes no item_drafts row — an item created that way would never appear in a queue that joins that table. Going through the link is also the path a real submission takes, so the test exercises what actually ships.
Two cases. The first edits the price and publishes, which confirms the number, so no dialog appears and the item reaches the storefront at what was typed. The second publishes an untouched price and asserts the confirmation names the figure and the fact that nobody chose it, then cancels and checks the item is still unconfirmed. That second case is the protection this whole screen exists to provide.
Scoped to the card each test creates, located by the sender's note carrying the run id — the item's own name is a submission timestamp and not unique to the test. The dialog is matched on its accessible name because antd nests the confirm title in two elements and getByText resolves to both.
Full suite 157 of 157, up from 155.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The screen that makes the intake pipeline usable. Until now a draft existed only in item_drafts and nothing rendered it, so a successful draft and a failed one looked identical from the admin — the item shows its placeholder submission-timestamp name either way, and telling them apart needed SQL.
The price carries the weight the schema no longer does. It is labelled with where the number came from, anything not set by a person is marked unconfirmed, and publishing at an unconfirmed price asks first rather than reporting afterwards. Editing the field is what confirms it, so opening the card and leaving the price alone is not recorded as approval — the same rule the server applies, which this only has to agree with.
Discard is offered rather than delete, and a discarded card offers Restore in its place.
The e2e page object's AdminTab union is extended alongside the tab itself. It is a closed union, so admin.open('Review queue') would not type-check without it — and the tab strip and that union have to be changed together or the next spec to use it fails to compile.
Both frontend lint and build clean, still at zero warnings.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Regenerate clears attempts along with the state. The worker only picks up rows below the attempt cap, so re-queueing a draft that has already failed three times without clearing them would produce a button that appears to work, does nothing, and leaves nothing anywhere to say why.
Discard deletes nothing — not the item, not the photographs. It is one click away in what amounts to an inbox, and the photos are often the only copy of something no longer in the sender's hands, so the destructive reading of the word is deliberately not available here. The item returns to pending, because a discarded submission must not stay on sale.
Restore returns a draft at the state its own contents justify rather than unconditionally ready. A submission discarded before it was ever drafted has no copy, and coming back as ready would present an empty draft as a finished one. Judged on whether a name was ever written, because the state held before discarding is not stored.
Backend now at 346 unit and 317 integration tests, all passing.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The only path from an intake submission to the storefront. It performs what mark-available performs — the status, and clearing the sale and reservation fields — rather than calling that route, because the copy and the publish have to be one transaction: an item published carrying the previous draft's name would be worse than one not published at all.
The price rule is applied here rather than trusted from the client. A changed number becomes the admin's; an unchanged one keeps whatever it was, so publishing without touching the field records that nobody chose it. The row is locked for the transaction so two admins publishing the same submission cannot interleave one's price decision with another's name.
Whole cents only. A fractional value would round somewhere nobody is looking and put the item on sale at a price no one entered.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Columns are spelled out rather than selected with a wildcard, so a column added to item_drafts later does not silently start reaching the browser. That matters most for the join to upload_links, which carries the token digest — only the label is taken, and a test asserts the digest never appears in a response.
Discarded rows are excluded by default rather than deleted. Discard has to be recoverable because it is one click away in what amounts to an inbox, but a discarded row left in the default view would compete for attention with work that still needs doing.
The gate goes on the mount in app.ts rather than inside the router, matching every other admin router. Since ADMIN_GATE_SECRET is unset for integration runs the gate is disabled there, so the test that asserts the mount is actually gated sets the secret for its own duration — leaving requireAdminGate off a new mount is otherwise a silent hole.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Pure and separately tested because the failure it guards is silent. Items are priced on arrival, so the schema no longer stops a number nobody chose reaching the storefront — the review queue does, by showing that nobody chose it, and an item selling at a default price looks exactly like one selling at a chosen price.
Editing the number is the only thing that confirms it. Publishing an untouched field deliberately does not, because that would record "I did not look at this" as "I approved this".
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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>
bermudalambandClaude Opus 5