87b0f189be0b8fbf8ef949a7b69bdd222dc81ae6
108
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
  bermudalambandClaude Opus 5
|
36e6057c6e |
fix(tests): close the route guard's factory hole and two regex warnings (#307)
Three of the items on the cleanup issue, and the first is the one that mattered.
routesAreWrapped.test.ts could not see a handler built by a factory. `router.post('/x', rotationRoute('left'))` carries no async token of its own, so the guard read those two lines, found nothing to object to, and passed — which is not the same as finding them wrapped. That is how the rotation routes added in #301 went through a test that exists precisely because this convention had already been half-forgotten once, when thirty handlers were added unwrapped after the wrapper existed. It now follows a call to a function declared in the same file and reads its body the same way it reads a registration, so an unwrapped handler inside a factory is an offender. Proved rather than assumed: unwrapping rotationRoute's handler makes the suite fail naming admin.ts, where before it passed.
Only same-file functions are followed, deliberately. app.ts registers express.json(), cookieParser() and uploadsRouter(), none of which is a handler factory and none of which can be resolved from the file being read — treating an unresolvable name as an offender would trade one hole for a permanently red test, so there is a case asserting those are left alone.
The brace and paren walking is now one function rather than two. Adding the factory reader as a near-copy of registrationAt is what a cleanup commit should not do, and the duplicate carried its own cognitive-complexity and loop-counter warnings with it; parameterising the delimiter pair removes both the copy and the warnings it added.
The schema mirror's table regex used `\s*` where kysely-codegen emits exactly two spaces and one after the colon, and `[A-Za-z0-9_]` where `\w` says the same thing. SonarQube flagged both, and the looser form bought nothing and backtracked for it.
An empty status list would have compiled to `in ()`, which is a Postgres syntax error, where the `= ANY($n::text[])` it replaced in #308 was valid and matched nothing. It is unreachable through parseItemFilters, which refuses a list that names nothing — but the obvious guard is wrong in the opposite direction, because dropping the clause entirely would make an empty status filter match every status rather than none, so the empty case is spelled out as false.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
||
|
bermudalambandClaude Opus 5
|
abe8ac8184 |
test(filters): merge the duplicate itemFilters import (#308)
backend/tests/unit/itemFilters.test.ts had two separate import statements from ../../src/itemFilters; merged into one, with nothing else in the file changed. A companion fix to backend/src/routes/items.ts — reading the by-id route's id with the existing readId helper instead of Number(req.params.id), to close the leniency Number() introduced toward inputs like '5.0', '1e2' and '0x10' — was tried and then reverted, because backend/tests/integration/errorHandling.integration.test.ts deliberately drives that exact route with a non-numeric id to prove that asyncRoute plus the error middleware turn a rejected handler into a 500 rather than hanging the request, and readId's stricter parse would answer 404 before that mechanism ever runs, leaving the test green while silently deleting the coverage it exists for; the route now carries a comment explaining why Number() stays and pointing at #307 for giving that test another trigger before making the switch. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalambandClaude Opus 5
|
ec1020891f |
refactor(db): build the item queries through Kysely (#308)
The two queries the builder was ever wanted for. #294 removed interpolation from seven sites by hoisting each fixed-shape query into a named constant; these two genuinely composed their WHERE at run time and could not be fixed that way, which is why they are the last S2077 hotspots. They were safe, and itemFilters.ts spent sixteen lines explaining why — that the clause fragments are literals, that the only things interpolated into them are placeholder indices, and that every value goes onto params. That argument was correct and it was still an argument, guarded by a comment and two tests, on a route reachable without signing in. All four call sites moved rather than only the two flagged ones. The by-id constants carried no hotspot, but they were built by interpolating the same projection strings the list queries used, so converting only the list queries would have left itemSelect.ts holding a Kysely builder and a raw string that had to produce an identical projection — two spellings to keep in step by hand where the file's own header already warned about one. The second thing this buys may matter more than the first. pool.query<T> asserts a shape TypeScript never checks against the SQL, which is why that header said the selects and their row types are kept in step by hand and the integration suite was the only thing that caught a drop. The projections are built with jsonArrayFrom now, which emits the same coalesce(json_agg(agg), '[]') they hand-wrote, so the row type follows from the projection and a dropped column is a compile error. The row types stay hand-written and exported rather than being inferred from the query. Inference would be tidier and is refused deliberately: these are the shape the frontend reads, and a type that silently becomes whatever the query happens to return is a contract that can change without anyone deciding to change it. The two invariant tests survive and got stronger. They used to inspect the clause strings the builder returned; they now compile the expressions and assert on the SQL Kysely actually emits, with the hostile value appearing in the parameters and nowhere else — the same claim, tested against the real artefact instead of an intermediate one. Closes #308 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalambandClaude Opus 5
|
add28c5f16 |
fix(db): repoint the lint and drift guards at the new mirror (#305)
The eslint config's ignores list and comment still named the deleted src/db-drizzle/schema.ts and relations.ts and never named src/db-kysely/schema.ts, so the generated mirror was being linted for the first time and tripping sonarjs/redundant-type-aliases — exactly the trap the config's own comment already described from #261 and #217. The ignores list now names src/db-kysely/schema.ts and the comment is updated to match. The schema mirror drift test built one flat Set of every two-space-indented key in the whole generated file and asked only whether a live column name appeared anywhere in it, rather than checking it against the specific table it belongs to. Seventeen column names are declared on two or more tables and created_at is on fourteen of eighteen, so a migration adding created_at, updated_at, status, name, sort_order, token, or expires_at to a table that lacks it would pass vacuously. Replaced mirroredTables with mirroredColumns, which reads the DB interface to map each table name to its declaring interface and then reads that interface's own columns, and changed the column-mirroring test to look up columns per table. Verified the guard can actually fail: removing customer_id from the Carts interface made the test fail naming carts.customer_id exactly, and restoring the file made it pass again. The root .gitignore still carried a comment block and two patterns for drizzle-kit pull output under backend/src/db-drizzle, a directory this branch deleted along with backend/drizzle.config.ts. kysely-codegen writes only the single tracked file it's pointed at, so nothing replaces the rule — deleted the block and both patterns. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalambandClaude Opus 5
|
731716f760 |
docs(db): correct the drift test's own stale references (#305)
The #217 doc comment above the describe block still named db-drizzle/schema.ts, drizzle-kit pull, and "Drizzle infers row types" — a file, a command, and a library this same commit had already removed. A comment pointing at deleted paths is worse than no comment at all on a test whose whole job is proving trust in a generated mirror, so it is corrected to name npm run db:types and src/db-kysely/schema.ts while keeping every sentence of the history intact: #217, #222, item_drafts and upload_links, the week nobody noticed. A closing note was added recording that the generator changed in #305 and the test did not, because the drift it guards is a property of generating a mirror at all rather than of any particular library. mirroredTables' regex also gets the same digit fix the column check already had. Both regexes parse the same generated file for the same kind of identifier, and a table name with a digit would otherwise be read out of the DB interface but reported missing by mirroredTables, sending someone to regenerate a file that was never wrong. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalambandClaude Opus 5
|
c71b11e05e |
refactor(db): swap the query builder from Drizzle to Kysely (#305)
One commit, because a main that carries both builders is one where the next person converting a query has to guess which to reach for, and where two generated mirrors of one database can disagree. There was nothing to stage anyway: one file used the builder. The safety property that motivated adopting a builder at all is untouched, and was never the thing being traded. A value interpolated into a sql template becomes a bind parameter in either library, so #202's invariant stays a property of the type system and #180's hotspots retire either way. What changes is the three ways the old library made it easy to be quietly wrong, each verified in #297 against the SQL actually emitted: an array interpolating as a placeholder list unless every site remembered sql.param(), a column reference inside a raw fragment silently losing its table so a correlated subquery correlated with itself, and a camelCase mirror that had to be mapped back at every select or the JSON contract changed with no test noticing. CATEGORY_COLUMNS stops being a translation layer and becomes what it looks like — four column names four selects share. The generated types carry parent_id and sort_order because kysely-codegen emits the database's own names, so there is nothing left to map and nothing left to get wrong by forgetting to. The drift guard survives the swap rather than being rewritten, and loses its library name in the process: it is schemaMirror.integration.test.ts now, so the next such change renames nothing. It also got stricter for free. The Drizzle version had to match each column two ways and its own comment called that deliberately loose; a generated Kysely interface spells the database's name verbatim as a bare key, so one exact match is the whole rule and snakeToCamel is gone. isUniqueViolation keeps accepting both error shapes and now has a test behind it. Kysely uses the pg driver directly and should leave the SQLSTATE on err.code, but "should" is the word that turned two 409s into 500s when the last conversion moved it to err.cause.code with nothing failing to compile. Migrations are untouched. #219 stands, they remain hand-written node-pg-migrate files, and Kysely has no generator to refuse. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalambandClaude Opus 5
|
3891f4fd75 |
feat(admin): endpoints to rotate one photo of an item (#301)
Two POST routes and the module behind them. They live on the item rather than on the draft, and that is the decision that makes the inventory editor free when it follows: an image belongs to an item whether or not a draft row exists, so the second screen to want this is the same call from a different place with no new backend at all. A cut-out and its pristine original turn together. An image that has been through #281 has two files, and rotating only the displayed one would leave them disagreeing — Restore original would then quietly un-rotate the photo, so the undo of one feature becomes a regression of another. 204 rather than 200. Rotation changes no column: the paths are identical afterwards and only the bytes differ, so there is no row worth returning, which is the same reason deleting an image is already a 204. Only "not on this item" is a 404, and it is indistinguishable from an absent id on purpose, because an image id is a serial and this endpoint should not confirm which ones exist. Everything else stays loud as a 500, and the file is untouched in every one of those cases — rotateInPlace renames over the original only once the new file has been written. One asymmetry is recorded rather than engineered around: rotation is not idempotent the way background removal is, so a retry after a failure between the two files turns the displayed one twice. That needs the disk to break between two writes, and the remedy is one press in the other direction. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalambandClaude Opus 5
|
3659608abe |
test(images): release the sharp handle before teardown (#301)
libvips caches input file mappings in memory after a pipeline finishes. On Windows, this keeps an open handle on the input file, and the OS refuses to delete a file with an open handle. The animated-WebP test triggers a pipeline rejection (correctly refusing a multi-page rotation), so the mapping stays in cache and afterEach cannot remove the test directory. Disabling the cache costs these tests nothing: each file is read exactly once during its test, so there is no reuse to cache. With caching disabled, Windows can delete the input files and afterEach succeeds. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalambandClaude Opus 5
|
5c0c7186ff |
feat(images): turn a stored photo a quarter turn (#301)
The file half of the remedy for what #300 could only stop. Fixing the EXIF strip means new uploads arrive the way the sender saw them; it cannot repair what is already stored, because the tag that said which way up the pixels went is gone. Those photos need a person to look at each one and turn it. Rewrites the pixels rather than recording an angle, because an angle obliges every consumer to honour it — the storefront, both admin screens, the drafting worker's photo reader, and the rembg sidecar — and any one that forgets shows the photo sideways. The sidecar is not ours to teach. Left is anticlockwise and right is clockwise, which is rotate(-90) and rotate(90); sharp reads a positive angle as clockwise. The direction test asserts a pixel rather than a dimension, because dimensions swap whichever way the turn goes — a reversed sign would pass every size assertion and ship a control that does the opposite of its label. The animated WebP case is the one that could destroy someone's file quietly. Reading such a file without the animated flag succeeds and hands back the first frame alone, so a rotation that omitted it would write a still back over the animation and report success. Passing the same flag reencodeInPlace passes makes sharp refuse instead — multi-page images turn only by 180° — which is the honest answer and leaves the file untouched. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalambandClaude Opus 5
|
d3ccc1b1f6 |
fix(uploads): apply the EXIF orientation before discarding it (#300)
Photos arrived in the review queue rotated, in an orientation the sender never saw, and we were doing it to them. A camera does not turn its sensor data round. It writes the pixels as the sensor read them and sets an EXIF Orientation tag saying which way up they go, and every viewer honours that — which is why a portrait photograph looks upright to the person who took it and to the person who attached it. The re-encode from #226 rebuilds the file from decoded pixels and drops all metadata, which is right and is the whole point: a product photo should not publish the coordinates it was taken at. But it never applied the orientation first, so the sideways pixels survived and the one piece of information that explained them did not. The fix is sharp's rotate() with no argument, which reads the tag rather than turning the image by a fixed amount, placed before resize. The order matters: resize bounds width and height, and for a portrait photo those are the wrong way round until the rotation has happened, so a 3000x4000 photograph stored as 4000x3000 would otherwise be bounded on the wrong axis. Two tests, one of which is a fixture lesson. The fixture is a 400x200 image tagged Orientation 6 — the shape a portrait photo actually has on disk — and the assertion is that it comes back 200x400. The first version built it with withExif({ IFD0: { Orientation: '6' } }), which sharp reads back as orientation 1: a fixture carrying no orientation at all, which would have passed against the unfixed code and proved nothing. It uses withMetadata({ orientation: 6 }) instead, and the comment says why so the next person does not repeat it. Confirmed by removing rotate() and watching the test fail. The second test pins that the tag itself still goes, so nothing downstream rotates the image a second time. This does not repair the photos already uploaded. Their EXIF is gone, so nothing records which way up they were meant to be, and the originals kept for #281's cut-outs were themselves re-encoded on the way in. Those need a person and a rotate button, which is #301. Closes #300 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalambandClaude Opus 5
|
0b6cc85c4f |
fix(lint): bring the backend test suites into scope (#298)
The backend lint script covered src and scripts; the frontend's has always covered src and tests. So roughly sixty backend test files had never been linted at all. That was a documented deferral rather than an oversight — the config said so in as many words, because tsconfig.json includes only src and type-aware rules had no program to resolve the test files against. tsconfig.test.json is that program, exactly as frontend/tsconfig.test.json was for the same problem in #137. It is separate from tsconfig.json rather than a widening of it, because that one drives the build and emits to dist, and pulling the suite in would ship the tests. The files were already type-checked at run time by ts-jest; this adds nothing to that, only to what the linter can see. Pointing it at tests produced 77 warnings and no errors. Sixty of those were rules that cannot be true in a test, so they are switched off here rather than left to accumulate — #60's argument, that a gate nobody reads is not a gate, and that a rule which cannot be true is noise hiding the rules that can. Forty-one alone were hardcoded passwords, which are the entire point of a test and which this project's own rule says must live only in test paths, which is here. The rest were a stub server on http to a socket the test opened itself, an RFC 5737 documentation IP, os.tmpdir, Math.random for a run id, and sorting two arrays to compare them. What was left was signal, and it found a real one on the first run. testDb.ts cleaned up settings with LIKE 'email\_%', and in a JavaScript string that backslash does nothing: the pattern is 'email_%', and an underscore in SQL LIKE matches any single character. It meant "email plus any one character" rather than "email_". It deleted the right rows only because no other key begins with those letters followed by something else — a setting called emailing_enabled would have been swept away between suites, silently, in a file that never mentions it. It now uses an explicit ESCAPE clause. It also found five dead `const before: string[] = []` declarations in uploadValidation, left over from #228's redesign of that suite. The tests assert properly through filesSettlingTo; the variables did nothing. Seven warnings remain, all in routesAreWrapped and workflowGate, and all judgement calls about guard-test complexity rather than defects. Leaving them visible is the point of having lint here at all. Closes #298 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalambandClaude Opus 5
|
445c9c4a22 |
fix(backgrounds): report a partial restore instead of throwing (#293)
restoreOriginalsForItem rethrew anything that was not NoOriginalToRestoreError, which handed asyncRoute a bare 500 and discarded how far the restore had already got. That breaks the invariant the feature is built on: photos restored before the failure really are back, and an admin standing in front of the modal needs the count to decide whether pressing the button again is worth anything. RestoreSummary now carries `failed` and the loop stops and reports, exactly the shape and the reasoning removeBackgroundsForItem already had. The restore-originals route gains the missing 404 for an item that does not exist — remove-backgrounds always had it, and the two handlers are copy-paste rather than a shared helper, so nothing would have caught them diverging. draftingWorker's .catch is now only reachable if the image-listing query itself throws, since removeBackgroundsForItem no longer rejects over a single photo; its comment says so rather than describing behaviour that has moved. The `failed` branch is covered by a unit test that stubs the database module in its own module registry. It cannot honestly be an integration test: the only failure the function can report is a database fault, and the only way to inject one into a real run is to interfere with the single pool every integration suite in the --runInBand process shares and that afterAll calls pool.end() on. Two tests that did exactly that are removed here — they left the suite reporting a failure against its own afterAll and leaking a handle that stopped it exiting. Nor is the fault reachable through data alone: the swap's WHERE original_image_path IS NOT NULL guarantees the value it writes into the NOT NULL image_path, and item_images carries no unique, check or foreign-key constraint on either column, so no row can be seeded that makes the statement fail. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalambandClaude Opus 5
|
8f35204995 |
feat(admin): remove or restore every background on an item (#293)
Two routes on the item, and a small admin config route so the inventory screen can know whether to offer them. Both answer 200 once the id is valid, even when the sidecar fails, and that is a deliberate departure from the per-photo endpoints in #281. Those act on one image, so the request either worked or it did not and 502 says which. These act on several, so "did it work" has no single answer — two of four is the normal shape of a bad day here, not an exception — and a 502 would throw away the count that is the only thing making the outcome actionable. Non-200 is reserved for not being able to try at all, which here means an unreadable or absent id. No status check on either. A sold item's photos are still the shop's photos and improving them changes nothing about the sale; the guards on unpublish protect a checkout in progress and a completed sale, neither of which is at stake in a photograph's background. The config route follows adminVersion's precedent rather than extending the public /api/config: admin-only, one purpose, and the reason written down. The inventory screen had no other way to learn the feature exists, because GET /api/admin/items answers a bare array with several consumers and reshaping it for one boolean is the worse trade. Also extends the admin item select to carry original_image_path on each image, behind a new ADMIN_IMAGES_SUBQUERY kept separate from the shared IMAGES_SUBQUERY the public select uses. Task 3 needs to derive its restore-button label from that field, and the server never sent it for items before this — only the drafts endpoint carried it, added by #281 for the review queue. It stays admin-only for the same reason itemSelect.ts already names PUBLIC_ITEM_SELECT's columns explicitly: an internal original filename is nobody's business on the storefront, and sharing one subquery would put it in every public item response. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalambandClaude Opus 5
|
6f3e77fa88 |
feat(intake): report what a whole-item background removal actually did (#293)
removeBackgroundsForItem answered void and threw on the first failure, which is enough for the drafting worker — it catches and logs, and a draft is not worth failing over — and not enough for an admin standing in front of a screen who needs to know whether the thing they pressed happened. It now returns a summary: how many photos the item has, how many carry a cut-out, and whether it stopped early. It still stops at the first failure. Six attempts against a sidecar that is not answering helps nobody, and stopping costs nothing because removeImageBackground skips a photo that already has an original recorded, so a retry resumes rather than starting over. The count is what turns that retry into an informed choice instead of a guess. Adds restoreOriginalsForItem alongside it. A photo that was never cut out is skipped rather than refused, because the mixed state a partial removal leaves behind is exactly when somebody reaches for this. The one existing caller does not change: the worker ignores the result, and ignoring a returned value is legal, which is what makes this additive rather than breaking. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalambandClaude Opus 5
|
ba26ee99f0 |
fix(mail-templates): add sample values for the upload-link placeholders (#260)
submitUrl, label and submissionsAllowed were added to the uploadLink template but SAMPLE_VALUES had no entries for any of them, so an admin opening Email templates, Upload link for a contributor, Preview saw literal {{submitUrl}} in the body — the preview being the only way to check an edit before saving.
The unit test meant to catch exactly this, in emailTemplates.test.ts, iterated a hardcoded KEYS array that predated intakeDraft and uploadLink, so it never checked either template's samples. KEYS is now Object.keys(TEMPLATES) as TemplateKey[], so the guard covers every template automatically and cannot go stale the same way again. intakeDraft already had samples for all its placeholders and passes once included, as expected.
One other test in the same file, "every template can address the customer", asserts that available contains greeting/firstName/lastName — a real invariant of the six customer-facing templates, but not of intakeDraft or uploadLink, which notify the shop and a contributor rather than a customer with a name on file. Switching that test to the new all-templates KEYS would have made it fail for both, so it now uses its own explicit CUSTOMER_FACING_KEYS list instead. That is a deliberate, commented exception: a hardcoded list is correct there because the claim itself does not extend to every template, whereas the SAMPLE_VALUES guard's claim does.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
||
|
bermudalambandClaude Opus 5
|
5a9022d8d1 |
test(integration): mock the mailer everywhere an upload link is issued (#260)
intake.integration.test.ts, intakeCeiling.integration.test.ts and uploadLinks.integration.test.ts all create upload links, and since #260 that now sends real mail. env.setup.ts never clears SMTP_USER, SMTP_PASSWORD or MAIL_ALLOWLIST, so with those inherited from a developer's shell these three files opened live TLS connections to smtp.gmail.com:465 and, with no allowlist set, actually delivered to sarah@example.com. This is the exact hazard the "Do not add one back" comment in tests/unit/mailOutcome.test.ts already warns about, reintroduced at the integration layer. All three now mock ../../src/mailer the same way accountDetails.integration.test.ts, favorites.integration.test.ts and resendVerification.integration.test.ts already do. uploadLinks.integration.test.ts is the one place that needs to see specific MailOutcome values come back through the route, so its two outcome tests were restructured to drive the mock's return value directly (sentMail.mockResolvedValueOnce(...)) instead of threading SMTP_USER/MAIL_ALLOWLIST through the real sendMail. That is a cleaner test anyway: it isolates the route's job (reporting whatever outcome sendMail returns) from sendMail's own skip logic, which is already covered hermetically by mailOutcome.test.ts and mailAllowlist.test.ts. Also addresses the related minor finding that nothing asserted the mail actually carried the working link: required: ['submitUrl'] on the template only guards that the placeholder is present in the body, not that the route supplied a correct value for it. A new test in uploadLinks.integration.test.ts inspects the mock's captured call and asserts the html contains the created token's /submit/ URL, and covers the three submissionsAllowed phrasings (a numeric cap, a cap of exactly one, and uncapped). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalambandClaude Opus 5
|
b18b3e3a3d |
feat(intake): require an address for an upload link and send the link to it (#260)
Creating a link now requires a valid email address and mails the link to it, which is the whole point: getting a link to a contributor was previously a copy-and-paste into whatever the admin happened to use. The send is awaited and its outcome reported, unlike every other sender in this codebase, which fires and forgets because nobody is waiting on the answer. Here somebody is. The admin is looking at the screen, and whether they now have to send the link by hand is exactly the thing they need to know — and QA blocks delivery to any address outside MAIL_ALLOWLIST by design, so a link that was never emailed would otherwise look precisely like one that was. A send that could not happen does not roll the link back. The token is displayed exactly once, so a rollback would leave the admin retrying and holding a different link, discarding work that had succeeded. They end up with a usable link and an honest statement about delivery instead. One inaccuracy left deliberately: an SMTP rejection is reported as skipped-unconfigured rather than a fourth outcome of its own. The distinction is real but nothing consumes it, and the admin's next action is identical either way. Also updates the other integration tests that created a link with only a label, since an address is now required, and adds the uploadLink template key that GET /api/admin/email-templates was missing from its list — an omission left by the template's addition in the prior commit on this branch. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalambandClaude Opus 5
|
eaecc43379 |
feat(intake): record where an upload link was sent, and how to say it (#260)
Adds upload_links.contact_email and the uploadLink mail template. The column is nullable on purpose. Links already exist in QA and a migration cannot invent addresses for them, so they are grandfathered rather than backfilled with something untrue; the requirement belongs in the create route, which is where new links are actually made. The template requires submitUrl, the same guard verification has on verifyUrl. An email inviting somebody to send in photos, with no way for them to do it, sends perfectly happily and looks fine in the log — it is the one failure here worth making impossible, and a test asserts the default body satisfies the guard it declares. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalambandClaude Opus 5
|
823796b92a |
test(mail): stop the allowlist test from reaching real Gmail (#260)
Review of the Task 1 commit approved the implementation but flagged the third test in mailOutcome.test.ts as a defect carried over from the brief: it set SMTP_USER, SMTP_PASSWORD, and an allowlist the recipient satisfied, so sendMail fell through both early-return guards and reached the real transporter, opening a live TLS connection to smtp.gmail.com:465. The .catch(() => 'threw') wrapper hid a fast auth rejection, a slow timeout, or an accidental real send equally, and on a restricted CI runner it would hang to the Jest timeout rather than fail fast. The fix deletes that test rather than replacing it. What it was trying to prove — that an allowlisted recipient is not blocked — is already covered hermetically by backend/tests/unit/mailAllowlist.test.ts, which exercises isAllowedRecipient directly across exact matches, plus-suffixes, domains, and refusals. The other two tests in mailOutcome.test.ts are untouched; they cover the two paths that return early, which is the entire point of the change, and neither one reaches the transporter. The file's top doc comment is updated to match: it now says only the two skip paths are covered here, names mailAllowlist.test.ts as where the allowlist's own behaviour is tested, and spells out why a third test that reaches the transporter does not belong in this file, so nobody adds one back. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalambandClaude Opus 5
|
8a35c213fc |
feat(mail): have sendMail say what it actually did (#260)
It returned Promise<void> and returned early in two cases that were indistinguishable from success at the call site: no SMTP credentials, and a recipient outside MAIL_ALLOWLIST. A caller could therefore report that it had emailed somebody a message nobody would ever receive, and in QA — which restricts delivery deliberately, as its entire safety property — that is the normal case rather than an edge one. It now returns a MailOutcome saying which of the three happened. No existing caller changes: there are seven and every one ignores the result, so this is additive. Re-deriving the answer at a second site would have duplicated isAllowedRecipient and the SMTP check, which is exactly the drift the guard-in-one-place comment above them exists to prevent. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalambandClaude Opus 5
|
4e2357356b |
fix(scripts): make the database the e2e suite reads a recorded fact (#273)
The end-to-end helper opens its own connection to read a password-reset token, and nothing guaranteed it pointed at the database the application was actually using. With -E2eDb the app runs on redefined_e2e at 55501 while the helper kept its redefined_local default at 55500, so the app wrote to one database and the suite read another — and the specs failed for a reason that had nothing to do with them. start-local.ps1 now records the coordinates it chose in .local/database.json, and run-tests.ps1 reads them into TEST_PGHOST, TEST_PGPORT, TEST_PGUSER, TEST_PGPASSWORD and TEST_PGDATABASE before Playwright starts. The answer now comes from one place, written by the thing that made the decision at the moment it made it. It is written after Start-Database rather than before, so the file never names a database that failed to come up, and removed by -Stop, so a stopped stack does not leave a record pointing at a container that is gone. Setting all five closes the second fault in the same change. Invoke-IntegrationSuite sets TEST_PGPORT and PowerShell keeps it for the rest of the session, so a -Suite all run leaked the integration port into the e2e run that followed — with none of the matching credentials, leaving the helper offering redefined_local's password to the integration database. Overwriting every one of them is what makes that leak harmless. A missing record throws rather than falling back. A default is what produced both faults in the first place: always plausible, silently wrong, and it fails in ways that look like application bugs rather than configuration. The guard test is the point of the change as much as the fix is. This is the third instance today of two files having to agree with nothing comparing them — #107 and #118 were envValidation against a compose file, #287 was the workflow against start-local.ps1, and this is three files rather than two. The test pins the whole chain: that the writer records the five settings, that the runner reads each of them from the record rather than a default, and that the helper reads no connection variable the runner does not set. Removing a single line from the runner fails four of its assertions, which was checked rather than assumed. What it cannot do is run PowerShell, so these are text assertions against the scripts. That is weaker than executing them and still catches the drift that actually happened. Closes #273 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalambandClaude Opus 5
|
be6800181d |
fix(admin): let a setting whose default is empty actually be cleared (#280)
intakeNotifyEmail and intakeCeilingResetAt both document empty as their default and as a working configuration — no notification address, and no ceiling reset recorded. The validator refused every empty text value, so either could be set and then never removed through the admin at all; the only way back was a DELETE against admin_settings. An admin who turned intake notifications on could not turn them off. Whether empty is a mistake is a fact about the setting rather than about its type, so it is now declared on the setting, in the DEFINITIONS row that already carries its type and fallback. A new setting states it once, in the place someone adding one is already editing, and nothing else has to know. That is what makes this different from special-casing two names in the validator, which would have left the next such setting to rediscover the same bug. The blanket refusal stays the default, because for a setting with a non-empty fallback an empty value really is a mistake: an empty greeting format renders every greeting as nothing at all, which reads as a broken email rather than as something a person cleared. Both those cases keep their tests. Whitespace is normalised to empty rather than stored. Somebody clearing a field they cannot see the end of leaves spaces behind, and they meant cleared. The tests check that the clearing survives the request rather than only being echoed back — the last one sets a value, clears it, and then reads it again through GET, which is the assertion that would have caught this had it existed. Closes #280 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalambandClaude Opus 5
|
d5e599a30e |
fix(admin): answer 404 for an unreadable id instead of 500 (#207)
The issue asked for two things and only one had been done. A well-formed but absent id already answered 404 — the PUT route carries a comment saying so. A malformed one still reached Postgres as text, raised 22P02 on an integer column, and surfaced through the route's catch as a 500, telling the admin the server had broken when the truth is that no such item can exist. That half is now closed everywhere rather than on the three routes that happened to have it. Guarded: DELETE an item, DELETE an image, unpublish, and every route in adminItemDrafts — publish, regenerate, discard, restore, and the two background-removal endpoints added by #281. The last of those were flagged in that feature's own final review as sharing this pre-existing shape, so they are fixed with the rest rather than left to be found again. Routes carrying two ids guard both. A route can guard the first and forget the second, and the forgotten one fails exactly as loudly, so there is a case each way for both image endpoints and for DELETE image. DELETE deliberately still answers 204 for a well-formed id that is absent. The method is idempotent and the caller's intent, that the item should not exist, is satisfied either way; what must not happen is a 500. There is a test pinning that so the distinction is a decision rather than an omission. Also replaced the raw req.params.id and Number(req.params.id) uses that sat inside routes which had already computed a validated id. They were safe, because the guard above them made them safe, but a validated id and a raw one side by side in the same handler is how this bug comes back. The test block named "a non-numeric id on every admin item route" covered two routes. It now covers every route that takes an id, which is what makes its name true. Closes #207 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalambandClaude Opus 5
|
6f8a0db130 |
test(integration): clean up background-removal test-harness leftovers (#281)
backgroundRemoval.integration.test.ts exported seedSubmission for no reason — nothing imports it, since draftingBackgroundRemoval.integration.test.ts and adminItemDrafts.integration.test.ts each wrote their own seeding helpers. Dropped the export, kept the function for local use. All three of these suites create a temporary uploads directory with mkdtemp and point UPLOADS_DIR at it, but none of them removed the directory afterward or restored the previous UPLOADS_DIR value — checked and the leak existed in all three, not just the one the review flagged. Each afterEach now removes its temp directory with fs.rm and restores (or deletes) UPLOADS_DIR to what it held before the test touched it, so this suite no longer leaves rubbish in the OS temp directory or a stale environment variable for whatever runs after it in the same process. This is test scaffolding cleanup, not a feature change — no runtime path in the application deletes anything. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalambandClaude Opus 5
|
dfd900aadd |
fix(admin): stop reporting every removeImageBackground failure as a sidecar failure (#281)
The remove-background route's catch block turned every throw from removeImageBackground into a 502 "the background-removal service did not answer". But removeImageBackground also throws for an unrecognised file extension (a legacy .jpeg), for a file missing from the uploads volume, and when REMBG_URL is not set at all — none of which involve contacting the sidecar. The admin was told to retry a service that was never reached, while the real reason existed only in the server log. Added SidecarRequestError in rembgClient.ts, following the NoOriginalToRestoreError pattern already in backgroundRemoval.ts. It is thrown only for failures that happen after actually attempting to reach the sidecar: the fetch call itself throwing (now wrapped in a try/catch, covering unreachable and timed-out), a non-2xx response, or a response that is not a PNG. It is deliberately not thrown for "REMBG_URL is not set", since that path never attempts contact at all. The remove-background handler now checks err instanceof SidecarRequestError before answering 502; everything else answers 500 with a message that says what actually went wrong. Added a unit test pairing (rembgClient.test.ts) asserting the sidecar-contacted failures are SidecarRequestError and the unconfigured case is not, and an integration test (adminItemDrafts.integration.test.ts) proving a missing upload file answers something other than 502 with a message that does not claim the service did not answer. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalambandClaude Opus 5
|
b06eac4640 |
fix(intake): clear remove_background when an admin restores a photo (#281)
item_drafts.remove_background is written once at intake and never updated afterward. Restoring a photo clears original_image_path, which is exactly what makes the row look "never cut out" to removeImageBackground — so a submitter's ticked checkbox, followed by the worker cutting the photo out, followed by an admin restoring a poor result, followed by a click on Regenerate, would silently re-cut the same photo the admin had just put back. Nothing was lost, but the control the design calls "what makes a poor result survivable" was quietly defeated by the button sitting next to it. restoreImageOriginal now swaps the image's paths back and clears item_drafts.remove_background for that item in one transaction, so a restore that succeeds while the flag update fails cannot reintroduce the bug. An admin restoring any photo on an item is treated as overriding the submitter's original request for the whole item — the flag is per-item while the swap is per-photo, so there is no narrower place to record the decision, and turning off the whole item's auto-removal is the conservative direction: the alternative is re-cutting something a person deliberately undid. Added an integration test in draftingBackgroundRemoval.integration.test.ts that drafts a submission with the intent set, cuts it out, restores it, mirrors what the admin's Regenerate button does (state back to queued, attempts cleared), runs the worker again, and asserts the photo is still not cut out. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalambandClaude Opus 5
|
984e329c47 |
fix(admin): narrow the restore-original catch to the race it exists for (#281)
Round 1 caught every throw from restoreImageOriginal() and reported it as a 404, on the theory that losing the concurrent-restore race is the only way that call fails. But the throw carried nothing to distinguish that race from a genuinely different failure during the same UPDATE — a dropped database connection, a transient outage — so a real failure was now silently reinterpreted as "someone already restored this" instead of surfacing as the loud 500 it was before. backend/src/intake/backgroundRemoval.ts now exports NoOriginalToRestoreError, a named subclass of Error thrown in place of the bare Error restoreImageOriginal previously threw. The message text is unchanged, so backgroundRemoval.integration.test.ts's rejects.toThrow(/no original/) assertion keeps passing without modification. backend/src/routes/adminItemDrafts.ts catches that class specifically in the restore-original handler and rethrows anything else, so a real failure still reaches the app-level error handler and comes back as a 500 instead of being mislabeled as "already done". backend/tests/integration/adminItemDrafts.integration.test.ts adds a test that spies on restoreImageOriginal via jest.spyOn on the module namespace (the project compiles to CommonJS, so the route's call site reads the export off that object at call time, which makes the spy effective without jest.mock) to reject once with a plain Error, and asserts the response is 500 rather than 404 — proving the narrowing changes real behavior, not just internal structure. The spy is restored in a finally block. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalambandClaude Opus 5
|
5129112266 |
fix(admin): answer 404, not 500, when two restores race (#281)
restore-original's precheck (existing.original_image_path === null) and restoreImageOriginal's own guard (WHERE ... AND original_image_path IS NOT NULL) could disagree under a race: two concurrent restores, or a rapid double-click, could both pass the precheck before either commits, and the loser's UPDATE would then match zero rows and throw. The handler had no try/catch around that call, so the throw propagated through asyncRoute to the app-level error handler and the caller got a bare 500, breaking the route's documented 200 | 404 contract even though the row itself was left correct. Wraps the restoreImageOriginal call in a try/catch, matching the shape remove-background already uses in this file, but answering 404 rather than 502: losing this race means another admin already finished the restore, not that a downstream service failed. Adds a comment on the catch explaining why it exists, and a test that fires two restores concurrently and asserts neither comes back 500. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalambandClaude Opus 5
|
9ced34ad19 |
feat(admin): remove and restore a photo's background per image (#281)
Adds two admin-gated endpoints on the review queue router: POST /:itemId/images/:imageId/remove-background and POST /:itemId/images/:imageId/restore-original. Both run synchronously and reuse the same backgroundRemoval module the drafting worker uses, so a cut-out obtained either way is identical and either can be undone by Restore.
Ownership is scoped by item as well as by image (imageOfItem selects on id AND item_id), because the image id is a serial and guessing one is easy — a photo belonging to a different submission must not be reachable through another item's URL. A sidecar failure returns 502, not 500, and leaves the row untouched, since removeImageBackground only writes the row after the cut-out file already exists on disk.
GET /api/admin/item-drafts now returns { drafts, backgroundRemoval } instead of { drafts }, and each image in the payload gains original_image_path, which is what the review queue UI will use to decide between "Remove background" and "Restore original". DRAFT_SELECT's images aggregate is extended accordingly, keeping the deliberate column spelling that guards against the upload_links token digest leaking into the response.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
||
|
bermudalambandClaude Opus 5
|
885a78c572 |
feat(intake): record whether the submitter asked for a cut-out (#281)
The public intake POST now reads a `removeBackground` multipart field and stores it on the new `item_drafts.remove_background` column. The checkbox on the submission page is ticked by default, so a client that sends nothing gets `true` — only the exact string `'false'` opts out, so a stray or unexpected value is treated as consent rather than a silent refusal. The GET now also reports `backgroundRemoval: isRembgConfigured()` alongside the label, so the submission page knows up front whether the feature exists in this environment at all. Neither handler calls the sidecar or the AI — this task only records intent for the drafting worker to act on later, and the existing ordering of `requireUsableLink` and `requireCapacity` ahead of `uploadImages` is unchanged. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalambandClaude Opus 5
|
81524a2849 |
feat(intake): cut out backgrounds in the worker, never in the upload (#281)
Wires removeBackgroundsForItem into draftQueued, gated on the submitter's remove_background intent recorded on item_drafts. The step runs after the draft is committed and catches for itself, so an unreachable or erroring sidecar never turns a draft that was written correctly into a failed one — the photo simply keeps its original, and the admin's per-photo control in the review queue is still there to do it by hand. It is awaited, unlike the notification below it, so a sweep that has returned has finished its work; nothing on the request path waits on it. Adds backend/tests/integration/draftingBackgroundRemoval.integration.test.ts as a new file rather than extending drafting.integration.test.ts, because that suite has never produced a successful draft and therefore has no draftListing mock — adding one there would be file-wide and would change what its existing tests exercise. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalambandClaude Opus 5
|
8d12cb2f2d |
feat(intake): swap a photo for a cut-out, keeping the original (#281)
Adds backend/src/intake/backgroundRemoval.ts, the shared module the drafting worker and the admin endpoints both call so a cut-out obtained either way is undoable the same way. cutoutPathFor is pure and writes a new file beside the original rather than overwriting it, which is what keeps the original restorable and makes the JPEG-to-PNG change free. removeImageBackground only points the row at the new file after it is already on disk, and is idempotent via the original_image_path IS NOT NULL check — load-bearing twice, since it also stops a second pass from recording the cut-out as the original and losing the real one for good. restoreImageOriginal swaps the paths back and deliberately leaves the cut-out file on disk. Extends the Task 1 integration test file with a stub sidecar bound to an ephemeral port and covers the no-op-on-repeat case plus three failure modes (500, non-image body, unreachable), asserting the row is left untouched in every failure case. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalambandClaude Opus 5
|
61e9c239d3 |
feat(intake): talk to the rembg sidecar, always naming u2net (#281)
Adds the client that will let the intake path remove backgrounds from submitted photos via the rembg sidecar over HTTP. isRembgConfigured() reports whether REMBG_URL is set (unconfigured is a normal, working state, not a failure), and removeBackground() posts a file to /api/remove and resolves with the PNG bytes it gets back, rejecting on every failure — unconfigured, unreachable, a non-2xx response, or a body that fails the same magic-byte PNG check the upload path already uses. The one hard rule: every request names model=u2net explicitly and this is never configurable. The sidecar's default model, reached simply by omitting the parameter, is bria-rmbg, which is licensed non-commercial — a licensing problem that a shop cannot silently ship, and one that would produce a perfectly good image with nothing in it to reveal the mistake. The test that posts against a real stub HTTP server and asserts model=u2net appears on the wire is the only thing guarding against that regressing. Wires REMBG_URL into both docker-compose.qa.yml and docker-compose.prod.yml as an optional variable, right after ANTHROPIC_WORKSPACE_ID, following the existing style in each file's environment block and header comment. It is deliberately left out of envValidation.ts's ALWAYS_REQUIRED — requiring it would make an environment with no sidecar refuse to boot, which is exactly the failure mode this feature is designed to avoid. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalambandClaude Opus 5
|
534e3d6228 |
feat(intake): record the background-removal intent and the original path (#281)
Adds the two columns the background-removal feature (#281) is built on: item_drafts.remove_background (boolean, not null, default true) records the submitter's per-submission intent, and item_images.original_image_path (nullable text, no default) records where a cut-out photo came from so it can be restored. The default on remove_background is load-bearing — any row written by a path that does not mention the column behaves like the new default, so no backfill is needed. original_image_path stays null until a photo has actually been cut out, which doubles as the answer to "can this be restored?" rather than needing a separate flag. Also updates the Drizzle mirror in src/db-drizzle/schema.ts by hand (the local dev database was not running to re-pull from) and adds the integration test backgroundRemoval.integration.test.ts, including the exported seedSubmission helper that Task 3 will reuse. Closes #281 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalambandClaude Opus 5
|
813216f971 |
test(integration): say so when the database loses its schema (#154)
When the integration run lost its schema partway through, it presented as 36 assertion errors about categories, price filters and favourite notifications. The real message — `relation "items" does not exist` — was further down the same log, and hours went into chasing the assertions instead. The cause is still open and needs runner-side evidence this cannot reach: whether the Postgres service container is being recreated mid-run, which would come back with an empty data directory. This is the half that can be fixed from here — whatever the cause, the next occurrence reads as "the database lost its schema" on the first line, names which tables are gone, and says that nothing in the suite drops tables so the database was replaced underneath the run. globalSetup asserts once after migrating, which establishes the fact the rest of the run depends on. Without it, a run that never had a schema and one that lost it midway are indistinguishable from the failures they produce. resetDb checks only when its TRUNCATE fails, rather than on every reset. It runs in a beforeEach several hundred times a suite, and an extra round trip each time to guard against a rare event would be paying continuously for it. When the schema is fine, an unrelated failure is passed through untouched rather than dressed up as a schema problem. The diagnostic is tested against a database that has actually lost its schema, not reasoned about. An earlier attempt dropped the schema before the run and proved nothing — globalSetup re-migrates, so it repaired itself and every test passed. The suite drops and rebuilds around each case and restores in afterAll; the full integration suite was then run twice to confirm the restore holds for everything ordered after it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalambandClaude Opus 5
|
a02214108a |
feat(build): pass the commit to the image as a build arg (#248)
The admin version stamp has reported commit "unknown" everywhere. #233 read it out of .git during the build and #235 removed that, because Portainer's build context has no repository history and the COPY failed every deploy — the version stamp became the thing that stopped deployments. #237 then established that building in Gitea Actions does not help: the Dockerfile no longer copies .git, so where the build runs is irrelevant. So the builder hands the commit over rather than the build going to look for it. ARG GIT_COMMIT, empty by default, passed through to writeBuildInfo, which prefers it and still falls back to reading .git so a local build stamps itself with no argument needed. An empty value is treated as absent rather than stamped. `--build-arg GIT_COMMIT=` is what an unset shell variable expands to, and a blank commit reads as one that happens to be empty rather than one nobody supplied. Nothing regresses for Portainer. It cannot pass the argument, so its images keep saying "unknown" exactly as today, and they still deploy — the property #235 was bought with. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalambandClaude Opus 5
|
6c6aaa46eb |
fix(intake): send the workspace id an identity-linked key requires (#271)
Every draft in QA failed with a 400: "anthropic-workspace-id is required when authenticating with an identity-linked API key". A key issued against a workspace, rather than standing alone, is refused unless the request names the workspace it acts in — and the client was constructed with an API key and nothing else. Nothing about a key's shape says which kind it is, so no amount of configuration checking would have caught this. Only a real call would, which is exactly what #223's task 8 existed to make. Sent only when ANTHROPIC_WORKSPACE_ID is set. Plenty of keys need no workspace, and sending an empty header would turn the ordinary case into a different error rather than leaving it working. Both compose files carry it with an empty default so an unset variable cannot fail a deploy, and the cutover doc goes from fifteen interpolated names to sixteen — checked against the file, and every name in the list now matches one in the compose. The failure handling needed no change and got none. The submission kept its photos, the draft recorded ai_error, and the review queue showed the reason. A model call failing must never lose somebody's consignment, and it did not. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
bermudalambandClaude Opus 5
|
2b8be434c0 |
test(uploads): stop the cleanup assertions racing the cleanup (#228)
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>
|
||
|
bermudalambandClaude Opus 5
|
e7fd63c2e6 |
feat(db): land the Drizzle schema, config and conventions (#217)
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> |
||
|
bermudalambandClaude Opus 5
|
f52976dec9 |
fix(admin): answer 404 for an item id that does not exist (#207)
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>
|
||
|
bermudalambandClaude Opus 5
|
0872cad4df |
fix(ci): stop pull request scans overwriting the dashboard's picture of main (#197)
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> |
||
|
bermudalambandClaude Opus 5
|
92c04847d6 |
feat(intake): refuse submissions past the daily ceiling, and reset it from the admin (#227)
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> |
||
|
bermudalambandClaude Opus 5
|
55cb7eaa13 |
feat(intake): count submissions across every link, and alert on abnormal volume (#227)
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> |
||
|
bermudalambandClaude Opus 5
|
22215f32eb |
feat(intake): settings for the submission ceiling (#227)
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> |
||
|
bermudalambandClaude Opus 5
|
346e9eae4c |
feat(intake): act on a signed link from the notification (#224)
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> |
||
|
bermudalambandClaude Opus 5
|
0517faca60 |
feat(intake): add the submission notification template (#224)
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> |
||
|
bermudalambandClaude Opus 5
|
acc1b406d8 |
feat(intake): sign the two actions an email may take (#224)
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> |
||
|
bermudalambandClaude Opus 5
|
d8a7588685 |
feat(intake): regenerate, discard and restore a draft (#225)
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> |
||
|
bermudalambandClaude Opus 5
|
44a9037121 |
feat(intake): publish a reviewed item to the storefront (#225)
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> |
||
|
bermudalambandClaude Opus 5
|
81886f84c2 |
feat(intake): list submitted items waiting for review (#225)
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> |