100 Commits

This Branch

This Branch

All Branches

Author	SHA1	Message	Date
bermudalambandClaude Opus 5	94e4f8c98a	docs(intake): plan the AI drafting worker (#223) ... Eight tasks: the SDK and its optional key, the Zod shape the model must answer in, the prompt, the call, writing a draft back, the worker that drives it, the wiring, and one real photograph to see whether any of it writes something worth reading. The prompt is the correctness surface and gets its own task with its own tests. On one-of-a-kind stock an invented "1930s hand-thrown stoneware" is not a cosmetic error but a false claim on a storefront, and nothing downstream can tell an invented detail from an observed one — the only place that distinction can be enforced is in the instruction, so the tests assert it is there. The other governing rule is that a submission is the only irreplaceable thing in the pipeline. The photos are often the only copy of an item no longer in the sender's hands, so a missing key, a failed call, a malformed answer and three exhausted retries all end the same way: the item keeps its photos and waits undrafted. Nothing in the worker deletes anything, and an absent key does not spend an attempt. Only the suggested price reaches the item, per #220, with price_source recording that a model rather than a person chose it. The name and description stay on the draft row until the review queue in #225 exists. The monthly spend ceiling is deliberately left out. Task 8 measures what a real call costs first, because a budget set from a guessed number is one nobody trusts. Every test stubs the Anthropic client. A test that reaches the real API is a defect in the test. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>	2026-08-31 19:17:12 -05:00
bermudalamb	2e010e6db4	build(intake): give the container an ANTHROPIC_API_KEY (#223) ... The drafting worker needs a credential, and a Portainer stack variable alone does not reach the container — stack variables are interpolated into the compose file as ${VAR}, and a service receives exactly what its own environment block lists. That is how UPLOADS_DIR went missing in #118, and both files say so; this adds the line that makes the variable actually arrive. QA takes it from QA_ANTHROPIC_API_KEY, prefixed like the database and SMTP credentials so production's key cannot be pasted there and silently work. It is also worth a key of its own rather than sharing production's, because this is the only credential in either stack that spends money per call, on a path anybody holding an upload link can trigger. Absent is a working configuration in both, deliberately, which is why prod's line carries `:-` and neither variable joins the always-required list. A submission still arrives, keeps its photos and waits undrafted. Losing somebody's consignment to an expired key would be far worse than an item arriving without its description written, and the photos may be the only copy of something no longer in the sender's hands. USPS is the existing precedent for a credential whose absence degrades rather than fails. The comments say plainly that a spend limit belongs on the key in the Anthropic console, since nothing in this repository can enforce one and #227's submission ceiling bounds the volume rather than the bill. docs/ops/production-stack-cutover.md said the compose file interpolates thirteen names and listed them. It now says fourteen, because that document stakes its usefulness on being checked against the file rather than written from memory — a cutover working from a stale list is how a variable gets left behind, which is the failure the document exists to prevent. Counted from the file: exactly fourteen. composeEnvironment.test.ts passes, 24 tests. It checks that every deployment sets what the validator requires, so adding a variable ahead of a validator entry cannot break it — the entry itself comes with the worker. Ref #223	2026-08-31 19:08:26 -05:00
bermudalamb	93a451dbf0	Merge pull request 'feat(intake): issue named upload links and accept photo submissions (#222)' (#250) from feature/222-upload-links into main ... Reviewed-on: #250	2026-08-31 15:47:09 -05:00
bermudalamb	bcecda9122	fix(intake): stop a throttled sender being told their link is dead (#222) ... Adding e2e specs for the submission page found a defect in the page they were written for, which is what they were for. One limiter counted page loads and submissions against the same twenty-per-quarter-hour allowance, so a sender working through a box of stock ran out after ten items — the exact person the feature exists for, and the exact case the limiter's own comment said must not be refused. The comment said refusing them costs a consignment while the number quietly did it. Worse, the page could not tell a 429 from a 404. `fetchIntakeLink` treated any non-OK response as "no link", so a throttled sender was told "This link is not active" and sent to ask for a replacement — which could not have helped, because the problem was their address and a minute of patience. Two conditions needing opposite reactions were sharing a message. Now two limiters, because the two requests cost different things. Reading a link hits one indexed row and writes nothing, so that allowance is generous at 120: someone re-reading the form or losing their signal should never be told to wait. Submitting writes up to six files, so that is the one worth bounding, at 30 — more than anyone photographing items can manage and far less than a script would want. The page gains a third state. Unknown, revoked and used-up still collapse into one "not active" card, because whether a link exists is not something a stranger needs to learn. Throttled is deliberately kept apart from them, since "wait a moment" and "go and ask for another link" are opposite instructions. Measured rather than assumed, on a freshly started process both times: before, 25 page loads produced 14 rejections; after, 40 produce none. The first attempt at that measurement was wrong and worth recording — the restart had failed with EADDRINUSE, so it read 30 of 30 against the old process's already-exhausted store. The two specs now pass in a full parallel run alongside everything else. They are scoped the way #241 asks: unique run ids, assertions naming only this run's rows, nothing asserted about the table as a whole. Backend: 284 integration, 309 unit. Frontend: build clean, lint unchanged at 2 pre-existing warnings. Ref #222, #241	2026-08-31 15:42:26 -05:00
bermudalamb	9b4d7f2d03	feat(intake): manage upload links from the admin (#222) ... An Upload links tab beside Tags: issue a named link, see how much of its allowance is spent, revoke it. Until now the only way to create one was curl, which is how the earlier tasks were exercised. The token is shown once, in an alert that says so plainly, because the server stores only a digest and genuinely cannot produce it again. A refresh loses it — that is the honest behaviour rather than a bug, so the copy says to revoke and reissue if it is lost instead of leaving somebody hunting for a reveal button. The cap field starts at 25 and unlimited is a checkbox rather than an empty field. Blank-means-unlimited would make the least deliberate action produce the least bounded link, and this screen sends all three cases explicitly so the server's default only ever has to cover callers that are not this screen. Two things came out of driving it in a browser rather than reading it. The revoke confirmation said "OK", and every other destructive confirm in this admin names its action — Delete, Disable, Re-enable — so it now says Revoke, in danger styling. A confirm button reading OK makes the reader go back and re-read the question to find out what they are agreeing to. And Popconfirm turned out to be a component nothing else here uses; the rest use Modal.confirm with an explicit okText. Keeping Popconfirm but matching its labelling to the established pattern seemed the smaller inconsistency, since the interaction is a row action rather than a page-level one. The load-on-mount effect carries the same eslint-disable and reasoning Tags and Categories already use, rather than a new shape. Verified in a browser: create shows the one-time reveal, the row lists as 0 of 25 and Active, revoke flips it to Revoked, and an explicitly unlimited link shows a bare count with no cap. The database then confirmed a default of 25, a null for the unlimited one, and a stamped revoked_at. Frontend: build clean, lint unchanged at 2 pre-existing warnings, 30 unit tests pass. Ref #222	2026-08-31 15:42:26 -05:00
bermudalamb	e7b01fdb36	feat(intake): add the public submission page (#222) ... Where someone with no account sends in photos of one item. Route /submit/:token, outside the authentik gate by design: the token in the URL is the whole access control, which is what #222 chose deliberately over accounts. One state for every refusal, matching the server's single 404. Unknown, revoked and used-up links all render the same "this link is not active" card, because saying which kind of dead it was would tell a stranger whether a link they guessed at exists — the server is careful about that and the page must not undo it. `beforeUpload` returns false so antd keeps the files rather than uploading each one as it is picked. The submission is then a single request the server can accept or refuse as a unit, which is what makes the transaction on the other side meaningful. The accepted types and the six-file cap are stated here so the picker offers exactly what will be taken, but both are checked again server-side, because everything on this page is under the sender's control. The fetch effect guards against a late response from a previous token overwriting the current answer, which is reachable simply by editing the URL. TypeScript caught a real mistake rather than a stylistic one: `.filter((f): f is File => ...)` on antd's originFileObj does not narrow, because RcFile extends File and the predicate would widen rather than narrow. flatMap avoids the predicate entirely. Verified in a browser rather than by inspection: a throwaway Playwright run against the live stack confirmed the form renders for a good token, the inactive card renders for a bad one, and a photo can actually be sent and acknowledged. The database then showed the item at status pending with the default price, the draft carrying the note and its originating link, the image row written, the link's counter at one — and zero storefront-visible items, which is the property that matters most. Ref #222	2026-08-31 15:42:26 -05:00
bermudalamb	1fc632598a	feat(intake): accept photo submissions through a shared link (#222) ... The public way in. Photos of one item plus a free-text note, from someone with no account, landing as an `items` row at status 'pending' — already invisible to every public and storefront query since #90, so nothing is live by accident. Every refusal is a 404. Unknown, revoked and exhausted links are indistinguishable from outside, because whether a link exists is not something a stranger needs to be able to learn — the same reasoning uploads.ts applies to files. The link is resolved *before* multer runs, and that ordering is the point rather than an implementation detail. discardUnlessAccepted would delete the files afterwards, but "written then deleted" is materially worse than "never written" on an endpoint the whole internet can reach: it is disk churn an unauthenticated caller controls, and it leans on an unlink that a crash between write and delete would skip. A test asserts the volume is untouched for a bad token, so a future reordering fails loudly instead of quietly handing that control away. The link counter is incremented inside the transaction and guarded on the same conditions as the lookup, so two submissions racing for the last slot of a capped link cannot both succeed. The response carries no item id: the sender has no business knowing about the catalogue and nothing they could do with it. The AI is deliberately not called here. A slow or failing model request must not turn into a failed upload for someone who did nothing wrong, and the photos may be the only copy — the item is often no longer in the sender's hands. The row waits at state 'queued' for #223. The new limiter keys on the caller alone, since a submission carries no email. keyByCallerAndEmail's comment warns that a bare ip bucket is a shared allowance, and that trade is taken knowingly: the link is the per-caller identity and its cap is the per-caller bound, while this limiter does the different job of bounding what one address can throw at an endpoint that writes files. Twenty per fifteen minutes is deliberately looser than the password-reset allowance — somebody photographing a box of stock legitimately submits several in a row, and refusing them costs a consignment. Because the route mounts the shared uploadImages, it inherits the type allowlist, the magic-byte check and #226's EXIF stripping without asking for any of them. A test asserts the stripping specifically, since this is the route where it matters most: the photo comes from a stranger's phone rather than the shop's own camera. Backend: 284 integration (12 new), 309 unit, lint unchanged at 6 pre-existing warnings, build clean. Ref #222	2026-08-31 15:42:26 -05:00
bermudalamb	3392f6f10d	feat(intake): issue and revoke named upload links (#222) ... Three routes behind the admin gate: list, create, revoke. A link is named because provenance matters more than convenience — when one is shared further than intended the question is which one, and every submission will record the link it arrived through, so revoking kills that link rather than the feature. The token is returned by exactly one response and is unrecoverable afterwards, which is why the admin screen has to present it as a one-time reveal. The listing selects its columns explicitly rather than `SELECT *`, so `token_hash` cannot reach a response the moment somebody adds a convenience — and a test asserts the listing carries neither the token nor the digest. An absent `maxSubmissions` gets a bounded default of 25 rather than null. Absent means nobody decided; an explicit null means unlimited, which is a decision visible in the request. Reading absent as unlimited is what would quietly make every link unbounded, and the common case is the one that has to be safe. Revoking is idempotent through COALESCE, and a test asserts the second call returns the *same* timestamp rather than merely succeeding. The useful fact is when access ended, and a button that errors on a double-click teaches people to distrust it — which is the last thing wanted on the control that contains a leak. Mounted above the `/api/admin` catch-all, which would otherwise swallow the path, and behind requireAdminGate on the router itself per the reasoning in middleware/adminGate.ts. Lint caught me reintroducing something this codebase had already solved: I wrote `.replace(/\/+$/, '')` to trim PUBLIC_URL, and app.ts carried a hand-written loop with a comment explaining that exact regex backtracks. Rather than duplicate the loop, trimTrailingSlashes moved to utils.ts and both callers now share it. Backend: 272 integration (9 new), 308 unit, lint back to its 6 pre-existing warnings, build clean. Ref #222	2026-08-31 15:42:25 -05:00
bermudalamb	2b2cbe119e	feat(intake): generate and hash upload link tokens (#222) ... The token is the entire access control on an endpoint the whole internet can reach, so both halves are pure and tested directly rather than through a request — the same reasoning that has uploadTypes.ts and keyByCallerAndEmail exported for their tests. 32 bytes of CSPRNG output, base64url so the value survives being pasted into a URL, a chat message or a QR code without escaping. That matters for something a person is handed rather than something a machine reads. The collision test runs a thousand generations rather than asserting the obvious, because a repeat would mean one person's link opening another's. SHA-256 rather than bcrypt, and the reasoning inverts the one that governs passwords. A password hash is slow on purpose because a human password carries little entropy and must survive an offline dictionary attack. This is 256 bits from a CSPRNG: there is no dictionary, so slowing the hash buys nothing. Meanwhile the digest is computed on every submission to an unauthenticated endpoint, where a deliberately slow hash would be a denial-of-service surface — #242 is the local proof that cost-12 hashing on a request path is enough to push it past a timeout under load. No timing-safe comparison, deliberately: the lookup is an indexed equality match on the digest rather than a byte-by-byte compare of the secret, and an attacker who could mount a timing attack against a 256-bit random value would still need the value. Backend: 307 unit tests, lint unchanged at 6 pre-existing warnings, build clean. Ref #222	2026-08-31 15:42:25 -05:00
bermudalamb	1a5a8b837b	refactor(uploads): extract the validated image pipeline for a second caller (#222) ... A pure move, no behaviour change. #222's public intake endpoint needs the same path from a multipart request to files on the uploads volume that the admin routes use, and the alternative to sharing it is a near-copy that has to reproduce every safety property exactly: the type allowlist, the magic-byte check after the write, names from a CSPRNG rather than from `originalname`, the re-encode that strips EXIF, and the cleanup of whatever a refused request left behind. A copy that drifted on any of those is the gap #95, #103, #180 and #226 exist to close. `stripUploadedImages` moved with the rest, which the plan originally did not say — it was written before #226 added it. Leaving it behind would have given the intake route an upload path that skips EXIF stripping, and no test would have failed to say so, because the intake tests are written against a route that does not exist yet. The refreshed plan added a check with a definite answer, and it now holds: routes/admin.ts no longer imports imageProcessing at all. Every comment came across verbatim. They record why the code is shaped as it is and are the most valuable part of what moved. Lint caught something the compiler did not: MAX_IMAGES_PER_REQUEST was left imported into admin.ts, where its only use — `upload.array('images', MAX_IMAGES_PER_REQUEST)` — had moved away with the middleware. It stays exported from imageUpload for the intake route's caps, but admin.ts does not need it. Verified as a refactor rather than as a change: the four suites that exercise this path hardest were run before the move and after it, 44 tests both times, same suites, same count. Full backend: 263 integration, 302 unit, lint back to its 6 pre-existing warnings, build clean. Ref #222	2026-08-31 15:42:25 -05:00
bermudalamb	6df32af784	feat(intake): add upload_links and item_drafts, and default an item's price (#222) ... The schema for the intake pipeline. A submission becomes an `items` row at status 'pending' — already invisible to every public and storefront query since #90 — with an `item_drafts` row beside it holding the submitter's note, which link it arrived through, and the fields the drafting worker will fill in later. `upload_links` stores a digest rather than a token, so a leaked database is not also a leaked set of working links, and the admin screen can show a token exactly once. `max_submissions` is nullable for "no cap", but the route will default it to a finite number: an unbounded link should be something asked for, not something that happens when nobody thought about it. `item_drafts.upload_link_id` is ON DELETE SET NULL rather than CASCADE. Deleting a link must not delete the items that arrived through it — provenance is lost, the goods are not. `items.price_cents` keeps NOT NULL and gains a default of 80.00, so an arriving item is always priced. That is the decision taken in the design review over making the column nullable: it costs the schema-level guarantee that nothing can publish at a price nobody chose, and buys not having to teach the cart, the checkout and thirteen other files about an item without a price. The protection moves into the review queue, and `price_source` exists so that queue can say whether a number came from a model, the default, or a person. The number lives in the migration rather than in configuration. Changing a default price is a rare, deliberate act that deserves a record; an environment variable would let it drift silently between environments, and a wrong default is invisible until something has already sold at it. Verified up, down and up again rather than only forwards — an irreversible migration is one that cannot be tested. Then verified by inspection rather than assumption: the default reads 8000, both tables and the state index exist, and an item inserted with no price comes back at 8000. Backend: 263 integration, 302 unit, all passing against the new schema. Ref #222	2026-08-31 15:42:25 -05:00
bermudalamb	7d8ac15ef8	docs(intake): refresh the extraction task for the changes #226 made (#222) ... This plan was written on 2026-08-29, before #226 landed. Its Task 2 lists what to move out of routes/admin.ts into the shared image pipeline, and that list is now missing `stripUploadedImages` and the `reencodeInPlace` import it depends on, because neither existed when the list was written. Executing it as written would have left the re-encode behind in admin.ts, and the public intake route added in Task 5 would then have had an upload path that skips EXIF stripping entirely. That is precisely what #226 exists to prevent — a stranger photographing an item at home publishing the coordinates it was taken at — and nothing in the suite would have failed to say so, because the intake tests are written against a route that does not exist yet. The task now names the function, says why it matters, and adds a check with a definite answer: after the move, routes/admin.ts must no longer import imageProcessing. If it still does, something was left behind. Ref #222, #226	2026-08-31 15:42:25 -05:00
bermudalamb	7100352d98	Merge pull request 'chore(ci): remove the registry spike workflow (#237)' (#249) from chore/237-remove-spike-workflow into main ... Reviewed-on: #249	2026-08-31 15:40:34 -05:00
bermudalamb	f5deb31370	chore(ci): remove the registry spike workflow (#237) ... The spike is answered, so the throwaway goes as it always said it would. What it established. The runner can build images once a docker CLI is installed — the socket was mounted all along and only the client was missing. The container registry works and accepts pushes; it had never been exercised, so that was genuinely unknown. TLS from the runner to the Gitea host is trusted, which also answers the certificate half of the Portainer question. A personal access token with write:package authenticates where the token Actions injects automatically does not. And a full image build and push costs 9m14s on that runner. What it disproved, which was the point. A CI-built image still reports `commit: "unknown"`. #235 removed `COPY .git` from the Dockerfile to stop the version stamp breaking every Portainer deploy, so it does not matter that an Actions checkout has history — the Dockerfile never copies it. Build location was never the problem, and moving builds to CI would have delivered nothing on its own. #248 carries the actual fix, a build arg, which is a few lines and does not need the registry at all. Deleting this from main rather than only from the spike branch: it was merged here in #238, before iteration 2 showed that workflow_dispatch fires from a branch and a spike never needed to reach main at all. Still owed by hand: the spike-trivial and spike-3085970 packages in the registry. Closes #237	2026-08-31 14:45:11 -05:00
bermudalamb	a800533be7	Merge pull request 'test(e2e): design, plan, and the find-or-fail helper (#241)' (#247) from fix/241-e2e-isolation into main ... Reviewed-on: #247	2026-08-31 14:44:14 -05:00
bermudalamb	64324609e1	test(e2e): add a find-or-fail helper for collection lookups (#241) ... Nine sites across five specs do `collection.find(...)` and dereference the result immediately. When the row is missing the test dies with "Cannot read properties of undefined" naming a line of test plumbing, which says nothing about what was expected — and that is exactly how favorites-filter:169 failed without producing a usable signal. The message names what was wanted and how many rows were searched. That distinction carries real diagnostic weight: "0 rows" means the fixture never landed, "37 rows" means it landed and the predicate is wrong, and those are different bugs to chase. It lives in its own module importing nothing, rather than in support/api.ts. That file imports @playwright/test, and vitest.config.ts runs tests/unit with environment: 'node' — putting six lines of pure logic there would drag a browser harness into the unit suite to test them. api.ts re-exports it so specs still reach it through fixtures. Throws rather than returning null, because every caller wants the row: an error at the point of the miss beats a null threaded through three more lines before something unrelated fails. Frontend: 30 unit tests pass, lint unchanged at 2 pre-existing warnings, build clean. Ref #241	2026-08-30 17:02:07 -05:00
bermudalamb	d16aba1647	docs(test): plan the e2e trustworthiness work (#241) ... Four tasks. A pure findOrFail helper with Vitest coverage, the nine unchecked lookups converted to use it, the expect timeout raised from Playwright's unset default of 5s to 10s, and the test #245 skipped brought back. The helper deliberately imports nothing and lives apart from support/api.ts. api.ts imports @playwright/test, and vitest.config.ts runs with environment: 'node' over tests/unit only — putting the helper there would drag a browser harness into the unit suite to test six lines of pure logic. Two things the survey changed. filters.spec.ts:216 is excluded: its .find() searches CSS class names on a string array rather than test data, so it has no missing-row failure mode. And three `expect(row).toBeTruthy()` assertions are deleted rather than kept, because findOrFail has already thrown by then — leaving them would tell the next reader the value might be falsy, which is the confusion the change exists to remove. Worker-count reduction is explicitly not in this plan. It is a real lever and may still be needed, but applying it at the same time would make it impossible to tell which change fixed anything. The plan states the criterion it cannot check: CI's load is not reproducible here, so two consecutive green local runs mean the refactor is sound, not that the flakiness is gone. Several consecutive green CI runs are the real bar, and #241 stays open until then. Ref #241	2026-08-30 16:52:05 -05:00
bermudalamb	f8b2b68f0d	docs(test): design for making the e2e suite trustworthy (#241) ... Five distinct specs failed across two runs of identical code with no overlap between the sets, so which test fails is decided by the scheduler. The cost is already being paid: #245 skipped the only end-to-end check that adding an item reaches the database, purely to get main green. The design separates two mechanisms that had been treated as one. Unchecked lookups into shared collections — `collection.find(...)` dereferenced immediately, found at eight or more sites — are a defect regardless of concurrency: when the row is missing the test dies with "Cannot read properties of undefined" naming test plumbing rather than failing an assertion that says what it wanted. Load-induced timing is the other, and is the larger share of what has actually been observed: three of four local failures and the CI one are assertions in a spec's own browser context that nothing else can touch. Two claims from earlier in this investigation are retracted in the document rather than quietly dropped. The verification-resend limiter is not a shared axis — it is keyed per customer and every test registers its own — and the suite contains no snapshot-style assertions, so there is nothing to convert to web-first. Both were stated as fact on the issue, and both would have justified work that was not needed. Per-worker databases are ruled out structurally: every worker talks to one backend on :3000, so isolation there means N backends, not N databases. Worker-count reduction is deliberately deferred rather than taken now, because applying it at the same time would mask whether fixing the defects worked. The honest limit is recorded too. CI's load cannot be reproduced here on demand, so the timing changes rest on reasoning rather than a red-to-green demonstration, and the success criterion is several consecutive green runs rather than one. Ref #241	2026-08-30 16:45:22 -05:00
bermudalamb	cd4618e266	Merge pull request 'test(e2e): skip the admin save happy-path test while #241 stands (#245)' (#246) from fix/245-skip-flaky-admin-save into main ... Reviewed-on: #246	2026-08-30 16:36:11 -05:00
bermudalamb	66c696ce40	test(e2e): skip the admin save happy-path test while #241 stands (#245) ... `main` has been failing on one e2e test since the sold-filter fix landed, and it is a different test from the one #239 corrected: admin-save-failures' "saves an item successfully when the server accepts it". Skipped rather than fixed, deliberately. It fails in CI and passes locally, and which test fails moves around — a local parallel run of the whole suite on the same commit failed four *different* specs (admin-inventory-filters, auth, favorites-filter, resend-verification) and not this one. That is #241: fullyParallel against a single shared database. Fixing this test on its own would be guessing at a symptom that reappears somewhere else next run. Ruled out before disabling anything: the re-encoding from #226 is not involved. AdminInventory.addItem fills a name and a price and saves, attaching no files, so stripUploadedImages iterates an empty array and the image path is never entered. Checked rather than assumed, because this spec is on the admin save route and that is exactly where a regression of mine would surface. What this stops covering is not trivial, and the comment says so at the call site: it is the only end-to-end check that adding an item actually reaches the database rather than merely firing a toast. #245 exists so that it is un-skipped when #241 lands, rather than left behind. A skipped test on the core admin save path is worse than a red build, because a red build is at least visible. Ref #245, #241	2026-08-30 16:20:28 -05:00
bermudalamb	feb714c49d	Merge pull request 'test(perf): stop hashing test passwords at production cost (#242)' (#243) from fix/242-bcrypt-cost-in-tests into main ... Reviewed-on: #243	2026-08-30 13:32:14 -05:00
bermudalamb	d7dacffa11	test(perf): stop hashing test passwords at production cost (#242) ... The integration suite registers around thirty-five customers and asserts nothing about any of their hashes, yet paid bcrypt cost 12 for every one. bcryptjs is a pure-JS implementation, so it pays that cost several times over compared with a native build, and hashing was most of the suite's wall clock. On a contended runner it pushed adminInventory.integration.test.ts past its twenty-second timeout, which then surfaced as a foreign key violation somewhere else entirely — the test timed out, jest moved on, beforeEach truncated, and the still-in-flight registration wrote a token for a customer that had just been deleted. Measured rather than asserted, warm run against warm run with only the constant changed: 34.5s at cost 12, 9.8s at cost 4. Three and a half times faster, about twenty-five seconds off every integration run, with all 263 tests passing either way. The first attempt at that measurement was wrong and worth recording. Comparing a cold run at cost 4 against a warm run at cost 12 made the change look like a 36% regression-shaped improvement of the wrong size; the difference was ts-jest and Postgres warming up, not the cost factor. Both numbers above are second runs, and the cost-12 figure was taken twice — 34.3s and 34.5s — before being believed. Deliberately not configurable. An environment variable here would be a way to weaken password hashing in production by misconfiguration, and nothing needs to tune it. The only route to the cheap cost is NODE_ENV=test, which a deployed container would announce anyway by refusing to serve the built frontend, since app.ts gates static serving on the same value. A setting that quietly degrades a security property should be unreachable rather than warned about, which is the reasoning that already made DEMO_MODE strict. `hashRoundsFor` is pure and separately tested because the failure it guards against is silent: only the exact string 'test' earns the cheap cost, and an unset NODE_ENV gets the strong one, so the dangerous direction has to be asked for explicitly. Both constants are pinned by assertions too — without that the branch tests pass while the numbers drift to something useless. Closes #242	2026-08-30 12:29:19 -05:00
bermudalamb	2ba35f8732	Merge pull request 'fix(test): correct the sold-filter tally assertion stranded by #188 (#239)' (#240) from fix/239-sold-filter-tally into main ... Reviewed-on: #240	2026-08-29 19:22:12 -05:00
bermudalamb	30227fb1e1	fix(test): correct the sold-filter tally assertion stranded by #188 (#239) ... main has been red since 2026-08-25. Every SonarQube run reported 147 passed, 1 failed, and it was this test every time — expected "Filters", received "Filters (1)". The test is stale, not the code. It was last touched on 2026-08-23 in #137; the tally logic changed on 2026-08-25 in #188, which never touched the spec. #188 redefined the tally as the number of chips and moved the availability preset into the dimension system as a bar dimension — one that still emits a chip for any non-default status, deliberately, because without it `?status=reserved` is an empty grid with no Clear filters button and no way out but editing the URL. The test asserted the rule that held before that change. Counting bar chips differently from drawer chips would restore exactly the per-screen special-casing #188 removed, and the drift it fixed was the admin's tally disagreeing with the storefront's. So the assertion moves, not the tally. The replacement also checks the tally comes back down when the default is restored. The original only ever asserted one direction, which would pass against a count that incremented and never decremented — worth fixing while the test is open rather than leaving a second gap behind the first. There is a real wart left standing: `Filters (1)` opens a drawer with nothing selected in it, because the filter it is counting lives in the bar. That is a cost of #188's design rather than a defect in it, and the comment now says so rather than leaving the next reader to rediscover it. Verified by running the spec in isolation with a single worker: 6 passed. Closes #239	2026-08-29 19:18:27 -05:00
bermudalamb	65fcba24f2	spike(ci): probe whether the runner can build and push an image (#237) ... Throwaway. Deleted once #237 has an answer, whichever way it goes. #233 was designed on an assumption about the Docker build context and broke every Portainer deploy. The chosen fix for the commit stamp — build in Actions, push to Gitea's registry, have Portainer pull — rests on three more assumptions about infrastructure that nothing in this repository can confirm. This probes them instead of designing around them. Whether a job here can run `docker build` at all is genuinely unknown: the other workflows use `services:`, which proves the runner can start containers, not that it can build images. Whether the registry is usable is likewise unconfirmed — the packages API answers but lists nothing, so it has never been exercised. And the real build is timed because backend-integration.yml is manual after a job once held this runner for 3h12m, so what an image build costs here is part of deciding whether building on every merge is tenable at all. The trivial image is built and pushed before the real one on purpose. It separates "can this runner build and push anything" from "does our Dockerfile work here", so a later failure still says which half is broken. workflow_dispatch only, with no push or pull_request trigger, so merging this changes nothing until somebody presses the button. Bounded by timeout-minutes on the same reasoning backend-integration.yml already documents. Ref #237	2026-08-29 18:32:27 -05:00
bermudalamb	71f8a21e45	Merge pull request 'fix(build): stop the version stamp from breaking every Portainer deploy (#235)' (#236) from fix/235-remove-git-copy into main ... Reviewed-on: #236	2026-08-29 17:07:36 -05:00
bermudalamb	1c69ac71f6	fix(build): stop the version stamp from breaking every Portainer deploy (#235) ... `COPY .git ./.git`, added in #233, fails with `"/.git": not found` in Portainer's build context, so every stack deploy died before anything else ran. This is the exact outcome #233 set out to prevent. That issue states that a version stamp must never be the thing that stops a deploy, and the resolution code honours it — every unreadable-.git path returns "unknown" and warns. The guard was simply in the wrong layer: COPY fails at image-build time, long before any of that code executes. Graceful degradation in the application buys nothing once the Dockerfile has refused to build. The assumption came from the local build context, where there is no .dockerignore and .git is therefore present. It was checked with a local `docker build`, which passed, and never against the only environment that actually deploys. Verified properly this time, by building from `git archive HEAD` — a context containing exactly the tracked files and no history, which is what a clean checkout gives. That build now succeeds and stamps `commit: "unknown"` with a real `builtAt`. The failing case is reproduced and fixed rather than reasoned about. `commit` will read "unknown" wherever Portainer builds. `builtAt` is still real, and is the half that matters most: Portainer already reports which commit it cloned, but cannot tell you whether the running container is that build. A build time can, and a stale one is exactly what the QA incident earlier today would have shown. Locally nothing changes — writeBuildInfo reads ../.git directly and still resolves a real commit. Sourcing the real commit inside a Portainer build needs a different mechanism, and #235 records the three candidates rather than guessing at a fourth. Closes #235	2026-08-29 15:51:52 -05:00
bermudalamb	a685229527	Merge pull request 'feat(admin): show the deployed commit and build time in the admin (#233)' (#234) from feature/233-admin-version-stamp into main ... Reviewed-on: #234	2026-08-29 15:45:54 -05:00
bermudalamb	44328d0b5c	feat(admin): show the deployed commit and build time in the admin (#233) ... There was no way to tell which build an environment was running. That is not hypothetical: minutes after #232 merged, `npm run backfill:images` in QA failed with `tsx: not found` because the container was still serving the pre-merge image, and the only thing that revealed it was npm echoing the old script line. Had the change been anywhere other than a package.json script, the container would have looked healthy while running the wrong code. The header now reads something like `a5076cc · built 29 Aug 20:36`. The commit answers "is this the code I expect"; the build time answers "did my redeploy actually rebuild", which is a different question and the one that would have caught the case above. The commit is read out of `.git` directly rather than by shelling out, because node:20-bookworm-slim has no git binary and adding an apt layer so the image can print seven characters is a poor trade. `.git` is copied into the build stage only — verified absent from the final image — so no repository history reaches a deployed container. Resolution is pure and separately tested across every shape that actually occurs: a detached HEAD holding the object name, which is what a checkout of a ref produces; a symbolic HEAD followed to a loose ref file; the same followed to packed-refs, which is what a fresh clone commonly has; peeled `^` tag lines ignored so an annotated tag cannot yield the wrong commit; and every failure path returning `unknown`. That last part is the one that matters most — this runs during a Docker build, and a version stamp must never be the thing that stops a deploy. Served from a gated /api/admin/version rather than folded into /api/config. That endpoint is public, and a commit hash there would tell any storefront visitor exactly which revision of a public repository is deployed. An integration test asserts the gate and asserts the public config does not carry it, because the boundary is the whole point rather than an implementation detail. Verified in the built image rather than argued: the stamp inside it reads a5076cc, matching `git rev-parse --short HEAD`, and a running container serves it from /api/admin/version while /api/config returns only what it did before. Backend: 296 unit, 263 integration, tsc clean, lint unchanged at six pre-existing warnings. Frontend builds clean with its two pre-existing warnings untouched. Closes #233	2026-08-29 15:37:12 -05:00
bermudalamb	a5076cc217	Merge pull request 'fix(uploads): ship the image backfill script in the container image (#231)' (#232) from fix/231-ship-backfill-script into main ... Reviewed-on: #232	2026-08-29 15:10:06 -05:00
bermudalamb	167c8ad97c	fix(uploads): ship the image backfill script in the container image (#231) ... `npm run backfill:images` could not run in QA or production. Three reasons, each sufficient alone: tsconfig includes only `src`, so `scripts/` was never compiled; the Dockerfile copies `dist`, `migrate.js` and `migrations` and never `scripts/`; and `tsx`, which the npm script invoked, is a devDependency that `npm install --omit=dev` strips from the final stage. The half of #226 that closes the exposure on already-stored photos had no way to run where the photos are. Moved to `src/backfillImageReencode.ts` so it compiles into `dist` and ships. Both of its runtime dependencies, sharp and pg, were already production dependencies, so the image needs nothing else. `scripts/bench-hash-latency.ts` was the pattern followed originally, and it is a development tool that never needs to run deployed; this one is an operational task that can only be useful where the images are, which makes `migrate.js` the right precedent instead. The npm script now runs the compiled output rather than tsx, so one command behaves identically on a laptop and inside a container. The entry point is guarded with `require.main === module`: putting a catalogue-wide irreversible rewrite in the same directory the server imports at boot means an accidental import would otherwise run it, and nothing should depend on people continuing not to write that import. Proven in the built production image rather than argued. `node_modules/.bin/tsx` and `scripts/` are both absent from it, and `npm run backfill:images` still runs: report mode found the planted file, `--apply` rewrote it 35760 to 16019 bytes, a second `--apply` reported skipped 1 processed 0, and on the mounted volume the EXIF was gone with the image bounded to 2000x1333 and still JPEG. That is the exact scenario the previous version would have failed. Backend: 285 unit, 260 integration, tsc clean, lint unchanged at six warnings, all six pre-existing. Closes #231	2026-08-29 15:03:44 -05:00
bermudalamb	846646f7ec	Merge pull request 'fix(uploads): re-encode uploaded images to strip EXIF and cut stored bytes (#226)' (#229) from feature/226-strip-exif into main ... Reviewed-on: #229	2026-08-29 14:17:44 -05:00
bermudalamb	32c1f23379	Merge main into feature/226-strip-exif ... main gained the Drizzle spike (#216) after this branch was cut, and both changes add a production dependency, so `backend/package-lock.json` conflicted. `backend/package.json` merged cleanly and carries both `drizzle-orm` and `sharp`. The lockfile was regenerated rather than hand-merged: main's version taken as the base, then `npm install` re-resolved it. That install was deliberately run under Node 24 rather than the machine's default 18.16.1, because sharp's platform binaries are optional dependencies that npm silently omits when the engine check fails — regenerating this file on Node 18 would have quietly dropped every `@img/sharp-*` entry and produced a lockfile that installs a sharp which cannot load. Verified afterwards that linux-x64, linux-arm64 and win32-x64 are all present and that drizzle-orm survived. Backend: 285 unit tests pass, tsc clean. Lint reports six warnings rather than three; the three new ones are in src/db-drizzle from the spike, not from this branch. Ref #226	2026-08-29 14:10:17 -05:00
bermudalamb	fe8f2aed90	Merge pull request 'spike(db): evaluate Drizzle and Tinqer against the hardest query we have (#216)' (#230) from feature/216-drizzle-spike into main ... Reviewed-on: #230	2026-08-29 14:04:45 -05:00
bermudalambandClaude Opus 5	664a0c30ed	spike(db): evaluate Drizzle and Tinqer against the hardest query we have (#216) ... Both libraries converted the same target — `buildItemFilterSql`, six clauses composed at run time, a recursive CTE for the category subtree, an `ANY(...::int[])` tag match with a count equality. Nothing in `src/routes` or `src/itemFilters.ts` is touched; this branch only adds spike artifacts alongside them. Drizzle cleared the blocker the issue named first. `backend/tsconfig.json` is `module: commonjs` and Drizzle is ESM-first, but it compiles under the existing config and requires at runtime, so no ESM migration is hiding inside this one. `drizzle-kit pull` introspected all sixteen tables plus `pgmigrations`, 104 columns, 8 indexes and 20 foreign keys, and got the hard parts right: the self-referencing `categories.parent_id`, and both partial unique indexes with `lower(name)` and their `WHERE` predicates. The converted filter produces byte-equivalent results. Five filter combinations run against the dev database return identical id lists to the current implementation, including the recursive subtree — 1805, 2145, 4, 1918 and 2145 rows respectively. The injection question the issue asked about is answered yes, and it is stronger than expected. In a Drizzle `sql` template `${value}` emits a bind parameter, not text, so there is no way to spell "interpolate this as SQL" by accident. Feeding `"1); DROP TABLE items; --"` as a status produced it in the parameter array and nowhere in the query text. That is the #202 invariant enforced by the type system rather than by a comment and two tests. Two Drizzle findings worth having before committing to 187 call sites. Arrays do not bind the way the raw driver does: `${array}` expands into a placeholder list, so `ANY(($1, $2)::int[])` type-checks, reads correctly, and fails at run time as invalid Postgres. `sql.param()` is required, and nothing warns. And the first generated migration after a pull carried spurious drops and recreations of the three expression indexes; re-running with no schema change reports nothing to migrate, so it settles rather than recurring, but that first migration would need hand-editing. Tinqer is genuinely LINQ-to-SQL — it parses the lambda with OXC at run time and compiles a real expression tree — and it cannot express this query. Compound conditions and array membership work. A ternary fails. A block body with an `if` fails. Those are the only two ways to make a clause optional inside the lambda, and there is no raw-SQL escape hatch in its API, so six independent optional clauses would mean 64 hand-written plans or neutral sentinels that do not exist for the category and tag clauses. Its failure mode compounds that: `defineSelect` parses eagerly and throws, so an unsupported query type-checks cleanly and crashes when the module is first required. `src/db-tinqer/probe.ts` wraps every case in a function for that reason. It is also `0.0.27` with 24 stars, and its Postgres support is a `pg-promise` adapter rather than the `pg` driver already in use. I was wrong earlier to say LINQ-to-SQL is impossible in TypeScript because it needs C# expression trees. Tinqer reconstructs the tree by parsing the lambda source. The claim should have been that it is possible and rare, and the constraint is what the parser accepts. Verified: backend build clean, 280 unit tests and 255 integration tests pass, unchanged by this branch. Refs #216 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>	2026-08-29 14:04:45 -05:00
bermudalamb	502d56d9fd	feat(uploads): backfill re-encoding over already-stored photos (#226) ... Stripping new uploads does nothing for the catalogue that is already on the storefront, which is where the exposure actually lives today. This is the other half. Reports by default and rewrites nothing without --apply, because the transform is lossy and there is no undo. Idempotency comes from `needsProcessing` rather than from a marker or a schema change: a file with no EXIF already inside the bounds is already in its final state, so a second run skips it instead of putting it through another lossy pass. Proven rather than assumed — a second --apply immediately after the first reports skipped 1, processed 0. Verified end to end against a real row and a real file. 3000x2000 carrying GPS EXIF became 2000x1333 with the metadata gone, 35760 bytes down to 16019, the format preserved, no temporary file left behind, and `item_images.image_path` untouched. That last part is what preserving the format bought: the backfill rewrites bytes and writes nothing to the database, so there is no window where a row points at a file that no longer exists. Both degenerate branches are exercised too, since a script that dies partway through a catalogue leaves the rest of it exposed: a row pointing at a missing file and a row with an extension the application would refuse to serve are each reported and counted, and the run continues. `handleRow` is split out of `run` for cognitive complexity, and while doing that a miscount was introduced and caught — incrementing `processed` before the rewrite meant a file that threw would have been counted as both processed and failed, which makes the summary unreadable at the moment it matters most. Ref #226	2026-08-29 13:51:26 -05:00
bermudalamb	aecccef418	feat(uploads): strip metadata from every accepted upload (#226) ... Hooked into uploadImages rather than into the routes. That middleware is where verifyUploadedImages already runs and is the single choke point every upload path passes through, so the admin create and update routes are both covered and the intake route from #222 will inherit it rather than having to remember. The same reasoning discardUnlessAccepted already gives for being a hook instead of a call. Runs after verification, deliberately: re-encoding a file whose bytes do not match its declared type would be work on something already refused, and sharp's error would replace the clearer message that check produces. A re-encode failure refuses the upload rather than storing the original, because the one case where a photo keeps the coordinates it was taken at should not be the case nobody was told about. The test builds a JPEG carrying GPS tags rather than committing a binary fixture, so what it contains is readable, and it asserts the fixture really carries EXIF before asserting the stored file does not — otherwise the test would pass while proving nothing. GPS tags go in IFD3, which is the GPS IFD as libvips names it; sharp's Exif type has no separate GPS key, and putting them in IFD0 would have produced EXIF without producing the tags this issue is about. Backend suites: 285 unit, 260 integration, lint clean, build clean. One caveat worth recording. Across three full integration runs, `uploadValidation` failed once on "removes the upload when the request is refused for its other fields". It is a pre-existing race rather than a regression: discardUnlessAccepted cleans up in an unawaited `void discardUploads(...)` inside a `res.on('close')` handler, so a test asserting on the directory immediately after the response has always been able to observe the state before the unlink lands. Re-encoding adds enough libvips work to lose that race occasionally where it previously did not. The property still holds in production, where the process keeps running and the unlink completes. Filed separately rather than fixed here. Ref #226	2026-08-29 11:54:34 -05:00
bermudalamb	e85be0f970	feat(uploads): re-encode images to strip metadata and bound dimensions (#226) ... Re-encoding rather than deleting tags. Deleting requires knowing every tag that could carry something sensitive, across formats and camera makers, indefinitely; rebuilding the file from decoded pixels leaves nothing that could have been missed. The same reasoning that makes uploadTypes.ts an allowlist rather than a denylist. `needsProcessing` is pure and separately tested because it is the whole of the backfill's idempotency argument: a file with no EXIF already inside the bounds is already in its final state, so a second run skips it instead of putting it through another lossy pass. Being wrong there degrades every image a little more on every run. Anything sharp cannot describe is processed rather than skipped, since a file we understand least is not one to assume is safe. Verified end to end on a real image before wiring anything up: 3000x2000 with EXIF present became 2000x1333 with EXIF absent, and no temporary file was left behind. Corrects something this README claimed an hour ago. Installing under a Node below 20.9.0 does produce a broken sharp, because npm skips the optional platform binary when the engine check fails and still reports success. But once that binary is present sharp loads and runs fine on 18.16.1 — `engines` is enforced at install time, not at require time. The README said the runtime was blocked, which would have sent someone switching Node versions to fix a problem that only the install created. Ref #226	2026-08-29 10:55:33 -05:00
bermudalamb	7d45b69305	build(uploads): add sharp for image re-encoding (#226) ... Verified where it actually has to run rather than only here: the production image builds and `require('sharp')` succeeds inside it on Node v20.20.2, linux/x64, with libvips 8.18.6 and `withExif` available, needing no build toolchain. The architecture question is already settled by this same node:20-bookworm-slim base running in production today, and sharp ships glibc prebuilds for both linux-x64 and linux-arm64, so it adds no constraint that deployment did not already satisfy. Installing it locally found a trap worth recording. sharp requires Node >=20.9.0 and its platform binary is an *optional* dependency, so npm skips it when the engine check fails and still reports success. Installed under this machine's default 18.16.1 the result is a node_modules that looks complete and throws `Could not load the "sharp" module using the win32-x64 runtime` at require time — which reads as a broken package rather than as a wrong Node version. The fix is `npm install --include=optional sharp` under Node 20+, and the prevention is using start-local.ps1 or run-tests.ps1, which switch first. `engines` is now declared so npm at least warns, and the README's existing Node 20 section says what the failure looks like, since the error message names a runtime rather than a version and points nowhere useful. The lockfile carries every platform variant including linux-x64 and linux-arm64, so a build on another platform resolves correctly. The Dockerfile does not copy the lockfile at all and installs fresh, so this matters for contributors rather than for the image. Ref #226	2026-08-29 10:45:01 -05:00
bermudalamb	5a3c0db0bc	Merge pull request 'docs(intake): design and implementation plans for the intake pipeline (#220)' (#221) from feature/220-intake-pipeline-design into main ... Reviewed-on: #221	2026-08-29 10:32:47 -05:00
bermudalamb	65f9d00785	docs(uploads): plan the EXIF stripping and re-encode (#226) ... Five tasks: prove sharp installs where it actually runs, the re-encode policy as a pure module, wiring it into the single middleware every upload path already passes through, the backfill over already-stored photos, and the deployment sequence. Re-encoding rather than deleting tags. Deleting requires knowing every tag that could carry something sensitive, across formats and camera makers, indefinitely; rebuilding the file from decoded pixels leaves nothing that could have been missed. Same reasoning that makes uploadTypes.ts an allowlist. Format is preserved rather than normalised to WebP. Converting would compress better but changes every stored extension, and therefore item_images.image_path, turning the backfill into a rename with a window where rows point at files that no longer exist. A privacy fix does not need that risk, and the backfill consequently touches no database rows at all. The backfill is lossy and irreversible, so it reports by default and needs --apply, writes to a temporary file and renames so an interruption cannot leave a half-written image being served, and is idempotent by construction: a file already stripped and already within bounds is skipped rather than put through a second lossy pass. That property is a pure function with its own unit test, because being wrong about it degrades every image a little more on every run. Two traps the plan handles that the issue only named. An animated WebP read without the animated flag decodes to a single frame and is silently written back as a still, so the flag is set for WebP and only WebP — it changes how resize reads height, which would be wrong for the other types. And sharp before 0.33 has no withExif, which the tests need to build their fixture; on an older version they fail as though the stripping were broken. Ref #226	2026-08-29 10:24:20 -05:00
bermudalamb	c76828122d	docs(intake): guard the uploads volume and bound a link by default (#220) ... Two tasks added to the slice-1 plan, closing the half of the upload gap the middleware ordering does not. The ordering fix stops a caller with a bad token writing anything. A caller with a working one can still send six eight-megabyte files per request against a limiter that allows twenty requests a window, and nothing checks whether the volume can take it. That volume is shared with the admin upload path, so intake filling it is a shop outage rather than an intake outage. Task 8 refuses an upload when less than a gigabyte remains, on the admin item routes as well as intake, failing closed because a volume that cannot be measured is not one to assume is empty. Task 9 makes an absent cap mean the bounded default of twenty-five rather than unlimited: the router as written treated omission as "no limit", so the ordinary act of creating a link produced an unbounded one, and a cap that has to be remembered is not a control. Two larger findings are filed rather than folded in. Re-encoding uploads to strip EXIF and cut stored bytes (#226) touches the shared pipeline and adds a native dependency; a global ceiling with an abuse alert (#227) needs its own state and an email. The EXIF one is worth stating plainly: nothing strips metadata today, so an uploaded phone photo publishes the coordinates it was taken at, at a public URL. That is already true of the admin path and is not introduced here, but this slice widens who can put such a file there. Ref #220	2026-08-29 09:03:33 -05:00
bermudalamb	783d10abc4	docs(intake): plan the upload-link and submission-page slice (#220) ... The first of four slices from the intake design, and the only one that is worth planning in detail yet — the later slices' shape depends on what this one actually produces. Seven tasks: the schema, extracting the validated image-upload pipeline out of routes/admin.ts so the public endpoint reuses it rather than growing a near-copy of it, token generation and hashing, the admin API for issuing and revoking links, the public submission endpoint, the submission page, and the admin screen. Three things the plan settles that the design left open or got wrong. The image caps become the constants already in the codebase rather than the 10-photo and 10 MB figures the design invented, because two different caps on one pipeline is a defect waiting to happen. The feature flag is dropped from this slice: nothing is reachable until a link exists, and the flag earns its keep in slice 2 where a paid API call appears. And the link is resolved before multer runs, so a stranger holding a bad token cannot cause a byte to be written to the uploads volume — cleanup afterwards would leave an unauthenticated caller in control of disk churn, and leans on an unlink that a crash between write and delete would skip. That ordering is asserted by a test, so a later reordering fails loudly rather than silently. Ref #220	2026-08-29 08:09:39 -05:00
bermudalamb	722bade383	docs(intake): price arriving items rather than leaving them unpriced (#220) ... `price_cents` stays NOT NULL and gains a default of 80.00. Where the model suggests a price the worker writes it onto the item; where it does not, the default stands. This is cheaper to build than the nullable design it replaces — the column's type is unchanged, so the fifteen files that read `price_cents`, the cart and the checkout among them, keep working untouched, and the migration adds a default and nothing else. It also gives up a guarantee. The storefront can now be reached by a price the admin never chose, so the protection moves out of the schema and into the review queue, where it is weaker. 80.00 is a plausible number rather than an obvious sentinel, so a default left unnoticed sells the item instead of announcing itself the way "$0.00" would. `price_source` is added to make that legible: the queue labels a price as coming from the model, the default, or the admin, and marks anything unconfirmed as such at the point of publishing. Publishing unconfirmed remains allowed — that is the decision taken — but it is stated rather than silent. Publishing still happens only from the queue, and the notification email still carries no publish button. Ref #220	2026-08-29 07:52:09 -05:00
bermudalamb	dbe6a0cf8f	docs(intake): design the upload-link, AI-draft and review-queue pipeline (#220) ... A named, revocable link lets someone without an account send in photos of one item plus a note; a background worker drafts the listing; the admin is emailed and publishes it deliberately from a review queue. Most of the lifecycle already exists and is reused rather than rebuilt: `pending` has been the unpublished state since #90 and is already excluded from every public query, the upload path already validates magic bytes against a three-type allowlist, mail already has editable templates and an allowlist guard, and node-cron is already the background-work pattern. What is new is a way in for someone with no admin account, the first LLM integration in this codebase, and somewhere to review a draft. The design turns on one invariant: nothing reaches the storefront at a price a model guessed. The suggested price lives on the draft and never on the item, the email carries no publish button, and the publish path refuses an item with no price. That is also why `price_cents` becomes nullable rather than defaulting to zero — a sentinel that formats as "$0.00" is the same class of quiet failure as `DEMO_MODE` once being "demo unless the value is exactly false", and nullability makes the compiler enumerate all fifteen call sites instead. Ref #220	2026-08-29 07:43:47 -05:00
bermudalamb	e933cd19f9	Merge pull request 'fix(cart): say what the demo button does rather than what the shop does (#203)' (#214) from feature/203-demo-notice-wording into main ... Reviewed-on: #214	2026-08-28 14:05:21 -05:00
bermudalambandClaude Opus 5	b897e99363	fix(cart): say what the demo button does rather than what the shop does (#203) ... #195 added a notice reading "Demonstration only — This shop is not taking payments at the moment", gated on `demoMode` alone. That claim is false in a configuration the ops runbook actively steers towards. `demoMode` and `paypalClientId` are independent. `checkDemoMode` and `checkPayPal` only make the PayPal secrets *required* when `DEMO_MODE=false`; nothing forbids them while it is `true`. And `production-stack-cutover.md:65` says flipping to `false` without all three crash-loops the container — so the only safe order is to populate the secrets while demo mode is still on, verify, then flip. In that window `Cart.tsx` renders live PayPal buttons directly beneath a banner telling the customer the shop takes no payments, and it is precisely the window in which someone is clicking around production checking their work. That is the same failure #195 fixed, pointed the other way: silent where a warning was needed, then confidently wrong where a customer can actually be charged. Telling someone nothing will be shipped above a live PayPal button is worse than saying nothing. The notice now describes the button instead of the shop, which is true in both configurations and stays visible in the one with two controls that do different things — where a customer most needs to be told they differ. Gating it on `!paypalClientId` would also have removed the false claim, by hiding the notice exactly there, which is the worse trade. The test for it was also not testing it. "Says so before the customer commits" seeded an address with `isDefault: true`, and the cart auto-selects the default on load — so an address was already selected and the button already rendered when it asserted. It would have passed with the notice moved inside the `selectedAddressId` guard, which is the regression it exists to catch. It now seeds no address and asserts the notice is up while the checkout button is absent, which states the property directly. Mutation-tested rather than assumed: moving the Alert inside that guard fails the new test, and would not have failed the old one. Verified: 3 end-to-end tests pass against a browser, frontend build clean, lint 0 errors (2 pre-existing warnings in `src/filters.ts`). Closes #203 Refs #195 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>	2026-08-28 14:05:21 -05:00
bermudalamb	ec08a73ebc	Merge pull request 'docs(security): put the SQL injection invariant where it is enforced (#202)' (#212) from feature/202-sql-invariant into main ... Reviewed-on: #212	2026-08-28 14:04:21 -05:00
bermudalambandClaude Opus 5	c704c07b89	docs(security): put the SQL injection invariant where it is enforced (#202) ... #180 cleared the three `typescript:S2077` hotspots and marked them Reviewed/Safe on the dashboard, but the repository half never reached main — the branch carrying it was deleted before merge, so the markers are cleared, the issue is closed, and nothing in the code said why. That is the exact state #180 set out to avoid: "the justification has to live in the repository, not only in SonarQube's UI". The three call-site comments are restored, with two corrections a review of the original found. They were in the wrong file. `buildItemFilterSql` is where the rule actually lives: both callers splice its clauses straight into query text, so only a placeholder index may ever be interpolated into one and every value must go onto `params`. That function's header said nothing about it, and it is where a seventh clause would be added. This matters more than ordinary comment placement because of how a cleared hotspot behaves. Reviewed/Safe stays marked and does not re-raise when a *different* file changes, so the one edit that would break this — interpolating a filter value in `itemFilters.ts` — was the one edit that would have got neither a warning nor a fresh marker. `items.ts` gets the same note. It builds `${PUBLIC_ITEM_SELECT} WHERE ${where}` from the identical construct and is reachable without signing in, but SonarQube never flagged it, so the higher-exposure copy was the undocumented one. It also records why joining with AND cannot weaken `EXCLUDE_PENDING`: no fragment carries a top-level OR for the join to re-associate against. The wording was slightly false. "The single interpolation is `$${next}`" — the tags clause also interpolates `$${next + 1}`. Same category, so the argument is untouched, but a reader checking it literally finds a counter-example immediately, and a comment asserting safety cannot afford that. Two tests make the invariant fail a build rather than depend on being read. One feeds values built by hand rather than parsed — `"1); DROP TABLE items; --"` in every field — and asserts none of it reaches the clause text, which states directly that these literals are safe with no parser at all. The other asserts two disjoint filter sets produce byte-identical SQL, which catches a value that happens not to look hostile. Both were mutation-tested rather than assumed: interpolating `filters.minPriceCents` into the price clause — the precise edit the comment forbids — fails both, and one pre-existing test besides. Reverted, and the diff against main for `itemFilters.ts` is comment-only. Verified: backend build clean, 280 unit tests pass. Closes #202 Refs #180 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>	2026-08-28 14:04:21 -05:00
bermudalamb	7421676568	Merge pull request 'fix(email): stop a demo purchase telling real customers an item sold (#206)' (#211) from feature/206-no-demo-sale-emails into main ... Reviewed-on: #211	2026-08-28 14:02:07 -05:00
bermudalambandClaude Opus 5	41840d4890	fix(email): stop a demo purchase telling real customers an item sold (#206) ... A demo purchase called `notifyFavoritersOfSale`, which mails everyone who favorited the item through production's configured SMTP: "An item you favorited has been sold to another customer, so it is no longer available… this one will not be restocked." Nobody bought it and nobody is shipping anything, so both halves are false. It is also the only outbound consequence a demo purchase has — everything #195 and #203 fixed is on screen, in front of the person who clicked and who has now been told it is a demo. These recipients never saw the cart. They just get told something they cared about is gone, and while production runs the demo interim (#191) they are real customers on real SMTP. The demo route no longer notifies. The PayPal capture and webhook paths are untouched, because those are sales. The item is still marked `sold`, so the storefront stays truthful about availability and the favoriter who goes looking finds what the database says. Only the claim that somebody bought it goes away. That a demo purchase permanently consumes real production inventory is a larger question than this issue and is left alone. Removing the call broke two tests and quietly hollowed out three more, which is the more interesting half of this change. Five tests in `favorites.integration.test.ts` used the demo purchase as a convenient way to make a sale happen; with the notification gone, the two asserting mail *is* sent failed, and the three asserting it is *not* sent would have passed for the wrong reason for ever. They were always about who gets told rather than about the demo route, so they now call the notifier the way the PayPal routes do — after the purchase, with the sold ids and the buyer. `buyThenNotify` says so at the point of use. Route-level coverage is unaffected: the admin mark-sold path already had its own test, and the new test asserts the demo route notifies nobody. Both halves were mutation-tested rather than assumed. The new test fails without the fix. Dropping the buyer exclusion from `collectFavoriteRecipients` fails "does not tell the buyer their own purchase is unavailable" and "emails every opted-in favoriter except the buyer" — so the restored tests are guarding the logic again rather than passing on an empty inbox. Verified: 255 integration tests pass (the suite needs `--runInBand`; these share one database), 278 unit tests pass, backend build clean. Closes #206 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>	2026-08-28 14:02:07 -05:00
bermudalamb	a1948a50c9	Merge pull request 'docs(ops): stop the compose header contradicting itself, and name the four variables step 2 dropped (#204)' (#210) from feature/204-ops-doc-accuracy into main ... Reviewed-on: #210	2026-08-28 14:01:29 -05:00
bermudalambandClaude Opus 5	70d04186b1	docs(ops): stop the compose header contradicting itself, and name the four variables step 2 dropped (#204) ... Three corrections a review of #196 found, all in text #196 itself rewrote. **The compose header asserted something it falsified three lines later.** "All must be set in Portainer for this stack. All are secrets except DEMO_MODE" — but three PayPal entries are unused while `DEMO_MODE` is `true`, three more are marked Optional, and `SMTP_FROM` is not a secret. The runbook sends the operator to that exact block as authoritative, so an operator cutting over during the demo interim reads "all must be set", has no live PayPal credentials — which is the whole reason the interim exists — and either stops or invents a `BACKUP_PASSPHRASE`, which is how you get archives nobody can decrypt. The blanket claim is gone; each entry already says whether it is required and when. **Step 2 enumerated nine of the thirteen interpolated names.** `USPS_CLIENT_ID`, `USPS_CLIENT_SECRET`, `UPLOADS_BASE_URL` and `BACKUP_PASSPHRASE` were missing. The USPS pair is the one that matters, and it now gets a sentence of its own: losing it is the only silent failure in this step. Address validation is skipped when those are empty rather than failing, so checkout keeps working and quietly stops validating addresses, with no crash loop and nothing in step 7 that would notice. The list also now says to take everything the stack holds rather than working from the list, because an enumeration reads as a checklist however it is introduced. **The `DB_PASSWORD` failure was described wrongly, and its error names a variable the operator never typed.** It does not fail to authenticate against its data directory — the app never reaches a connection attempt, `checkAlwaysRequired` refuses at boot, and the message says `PGPASSWORD` because the compose file injects it as `PGPASSWORD=${DB_PASSWORD}`. An operator grepping for `DB_PASSWORD` finds nothing. That is now stated. The crash-loop examples were reordered to match the case the section claims to be about. It headlines "the likeliest outcome of a missed step 2", but the only block shown was the PayPal triple, which cannot occur during the demo interim, and #196 replaced it with a `DEMO_MODE`-only block that is not what a missed step 2 produces either. A wholesale miss is two problems led by `PGPASSWORD`; that is now first, with the `DEMO_MODE`-only and mistyped-value forms after it. Every quoted line was verified by running the real validator against production's entry set rather than by reading the source — `2 problem(s)` and `1 problem(s)` counts included — and all four now match character for character. The mistyped-value message is quoted in full rather than truncated, which was the point of the complaint that produced it. `DEMO_MODE` is no longer called "the only one that is not a secret", which was false of `SMTP_FROM` and `UPLOADS_BASE_URL`. It is the only one that is a setting rather than a credential, which is true and a better hook. Verified: 278 backend unit tests pass, including the compose guard that parses this file. Closes #204 Refs #196 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>	2026-08-28 14:01:29 -05:00
bermudalamb	6562208995	Merge pull request 'fix(orders): mark a demo order in the history rather than leaving it to read as real (#205)' (#209) from feature/205-demo-order-history into main ... Reviewed-on: #209	2026-08-28 14:00:52 -05:00
bermudalambandClaude Opus 5	39c82ff3a4	fix(orders): mark a demo order in the history rather than leaving it to read as real (#205) ... #195 and #203 made the cart say a demo order is a demo order. That message is an antd toast lasting about three seconds, after which the cart empties and the card unmounts. Order history is what the customer comes back to when they wonder where their item is, and it said nothing. A demo row was a real row: item name, `$80.00`, status `completed` rendered as a neutral tag because `STATUS_COLORS` has no `completed` key, and `demo` printed raw under a heading reading "Processor". That is not an explanation — a customer has no reason to read `demo` as "this did not happen", and "processor" is not a word they have any reason to know. Two things now say it, for the same reason the cart needed two. The `demo` cell renders as a tag reading "Demo (not charged)", which marks *which* order. A notice above the table, shown only when there is one, says what that means — a tag reading "Demo" still assumes the reader knows what a demo order is, and what they actually want to know is whether to expect a parcel. Nothing changes on the backend: `orders.processor = 'demo'` was already written at checkout and already selected for this page. The row is a real row in a real table and stays visible, because hiding it would be its own kind of lie — the customer did do something, and it did have an effect on the catalogue. Written test-first against a browser: the new case drives a real demo purchase through the cart, opens `/orders`, and failed on both assertions before the change. Verified: 4 end-to-end tests in this spec pass, the 5 existing order-history tests still pass, frontend build clean, lint 0 errors (2 pre-existing warnings in `src/filters.ts`). Closes #205 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>	2026-08-28 12:22:39 -05:00
bermudalamb	f0bdc590c9	Merge pull request 'fix(scripts): switch Node to a pinned version rather than asking nvm for latest (#198)' (#201) from feature/198-nvm-node-version into main ... Reviewed-on: #201	2026-08-27 17:05:48 -05:00
bermudalambandClaude Opus 5	95325c75d4	fix(scripts): switch Node to a pinned version rather than asking nvm for latest (#198) ... `start-local.ps1` failed on a machine that had everything it needed, and then blamed the one thing that was definitely not the problem: "the newest version nvm has installed is too old. Install a newer one" — printed on a machine holding 26.7.0 and 24.13.1, both well past the floor. `nvm use latest` does not mean "the newest version I have installed". nvm-windows resolves `latest` against the remote release list, and `newest` is the alias for the newest installed. The docstring stated the opposite and the code was written against it. Here that resolved to 26.8.1, which is not installed, so nvm reported `activation error: Version not installed`, left v18.16.1 running, and exited 0. Two things had to change, and fixing either alone leaves it broken. The version asked for is now pinned in `NODE_VERSION` rather than chosen by alias, so two machines run the same Node instead of whatever each happens to have installed, and there is one line to bump for both entry points. The alias names are recorded in the docstring anyway, because `latest` and `newest` are easy to swap back by accident and the difference is the whole of this bug. `Use-Node` no longer treats an alias as automatically successful. That special case is why the error was wrong rather than merely unhelpful: it short-circuited on `$Version -eq 'latest'` regardless of what was running, swallowing nvm's `activation error` — which the function had already captured in `$output` for exactly this purpose — and returned success holding v18. The floor check downstream then reported the only explanation left to it. An alias switch is now verified against nvm's own report, so a failure says what nvm said. Keeping that half matters even with a pinned version, because no caller passes an alias today. The bug was someone reaching for one, and the next person reaching for one gets a truthful failure rather than a confident wrong answer. The floor check survives as a backstop against pinning `NODE_VERSION` below 20, and its message now says that rather than describing installed versions — a version that is not installed is `Use-Node`'s error to report, and it reports nvm's reason. Verified from a real v18.16.1 baseline: the pinned switch takes 18.16.1 to 26.7.0; a concrete version that is not installed throws with nvm's reason; `latest` throws instead of silently succeeding. `start-local.ps1` then runs the whole way through — Node switch, migrations, backend build, ready. Parse check clean. Shared by `start-local.ps1` and `run-tests.ps1`, so this broke both and fixes both. Closes #198 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>	2026-08-27 17:05:48 -05:00
bermudalamb	1b39e354b6	Merge pull request 'fix(cart): say it is a demo where the customer can see it (#195)' (#200) from feature/195-demo-checkout-label into main ... Reviewed-on: #200	2026-08-27 17:03:55 -05:00
bermudalambandClaude Opus 5	8572e62514	fix(cart): say it is a demo where the customer can see it (#195) ... Production runs demo mode with no PayPal credentials — that combination is the whole reason #191 turned it on — and in exactly that configuration the storefront rendered a full-width primary button reading plainly `Checkout`. The `(Demo)` suffix was gated on a PayPal client id being present, so the one configuration that needs the word was the only one that never got it. The button is not decorative. It posts to `/demo/purchase`, which marks the item `sold`, writes a `completed` row into `orders` at the real price, and emails everyone who favorited it through production's real SMTP. Nobody is charged, which is what the compose banner promises and is true — but a customer cannot tell they have placed a pretend order, the inventory says otherwise, other customers are told it sold, and nobody is expecting to ship anything. #190, #191 and #192 all reason carefully about not charging by accident; none of them consider accepting an order by accident. Three things now say so, because one of them was never going to be enough: The label is unconditional. `type` still follows the PayPal client id — secondary when real PayPal buttons sit above it, primary when it is the only way to check out — and that distinction is worth keeping, but it is about prominence rather than about what the order is. A notice sits above it for the whole of demo mode, before an address is picked and whether or not PayPal is configured. A parenthesis on the control someone has already decided to press is the weakest possible moment to tell them. The confirmation stopped saying `Order complete!`, which is exactly what a real order says. It now names the two things a customer would otherwise assume: nothing was charged, and nothing will be shipped. Three end-to-end tests, written first and failing first against a browser — the label assertion failed with `Expected "Checkout (Demo)", Received "Checkout"` on an `ant-btn-primary ant-btn-block` element, which is the defect exactly as reported. The suite already runs `DEMO_MODE=true` with no PayPal credentials, so it reproduces production's configuration without any new fixture. `CartPage.checkoutButton` matches the label by prefix rather than in full, deliberately: a locator naming the correct label would have gone looking for the right button and found nothing, which is how a test for this can quietly pass by being wrong in the same direction as the bug. Verified: 3 new tests pass, frontend build clean, lint 0 errors, 25 unit tests pass, and the cart-countdown and orders suites still pass. The favorites and favorites-filter suites fail here, and fail identically with this change stashed — 9 failures without it, 8 with — so they are pre-existing and not from this. Worth their own issue. Closes #195 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>	2026-08-27 17:03:55 -05:00
bermudalamb	ff6cbe18c5	Merge pull request 'docs(ops): make the cutover runbook agree with the compose file about DEMO_MODE (#196)' (#199) from feature/196-runbook-demo-mode into main ... Reviewed-on: #199	2026-08-27 17:02:52 -05:00
bermudalambandClaude Opus 5	2fe3aa055f	docs(ops): make the cutover runbook agree with the compose file about DEMO_MODE (#196) ... #190 turned `DEMO_MODE` from a hardcoded compose value into a Portainer stack variable with no default, and updated the compose header. The runbook that actually creates the stack was not updated with it, so the document and the file it deploys have been disagreeing since — in the three places most likely to be read under pressure. Step 2's list of stack variables to record did not include `DEMO_MODE`, and step 6 says to add the variables from steps 2 and 3. An operator following this literally creates a stack that cannot boot. It is now in the list, with a paragraph of its own: it is the only one of the nine that is not a secret, which is exactly why it is the easy one to skip past. The troubleshooting section said `DEMO_MODE` was hardcoded and therefore could not be missing, so its absence from the error list proved nothing. That was the most dangerous sentence in the file — an unset `DEMO_MODE` is now the *first* thing to check rather than something to rule out. The line now says it moved and why. The crash-loop example was the PayPal triple, which cannot occur while `DEMO_MODE` is `true`. The message an operator will actually see during the demo interim — `DEMO_MODE is required and must be exactly 'true' or 'false'` — appeared nowhere in the runbook. Both forms are shown now, in the order they are likely to be hit. Added what neither document said: Compose only *warns* about an unset variable and deploys anyway. In Portainer's stack UI that warning is easy to miss, and the container then crash-loops under `restart: unless-stopped` — loud in the log, invisible in a glance at the stack list. The container log is the signal, not the deploy output. The compose header carried two claims that #190 falsified and did not correct: that the PayPal secrets are required "because DEMO_MODE is false below", and that only secrets are interpolated. Both now describe the file as it is. This is the drift `composeEnvironment.test.ts` exists to prevent, surfacing in the one place no test can reach — the guard keeps the compose file honest about its own intent and cannot see the runbook beside it. Verified: 278 backend unit tests pass, including the compose guard that parses this file, and a sweep for the stale claims finds none left. Closes #196 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>	2026-08-27 11:35:44 -05:00
bermudalamb	fdb5b7a0be	Merge pull request 'Feature/192 backup directories' (#193) from feature/192-backup-directories into main ... Reviewed-on: #193	2026-08-26 10:58:09 -05:00
bermudalamb	1a59edec18	docs(ops): prove the backups during the cutover rather than waiting for a schedule (#192) ... Step 4b created the directories and step 7 checked the containers were Up. Neither established that a restorable file actually gets written, and those are not the same claim — the database backup does not run until 03:00 and the uploads archive not until Sunday 04:00, so a stack that looks correct at the end of a cutover can be four days from its first evidence. Both tools take a manual trigger, so the wait is unnecessary. The runbook now forces one run of each, checks the sizes are plausible, and greps the dump for `COPY` lines on the real tables — a dump of an empty database succeeds and looks fine, which is the one way this check could otherwise lie. It also confirms the healthchecks agree with where the files landed. A check whose `find` path disagrees with where the tool actually writes reports unhealthy forever while the backups are working perfectly, and that is a thing to discover on the day the stack is built rather than a year later. `starting` corrected to `unhealthy` in the surrounding text: during `start_period` Docker reports `starting`, which is what an operator actually sees and what the previous wording got wrong. Proven against production during the cutover on 2026-08-26 — a 41K dump and a 15M archive, both landing where the healthchecks look.	2026-08-26 10:40:39 -05:00
bermudalamb	724e9ce19d	docs(ops): say that the backup directories have to be created (#192) ... The stack gained two backup services in #147 and nothing has ever told anyone to create the directories they mount. `backup-and-restore.md` reads from both paths and the compose file mounts both, but no document creates them — while the README does exactly that for QA's data directories, ownership notes and all. Production's cutover runbook said nothing. Hit for real during the cutover: both backup containers sat in Created, never started, and `docker logs` on them reported only that nothing matched the filter, because a container that never ran has no output. Portainer showed them beside the healthy ones and the stack looked deployed. That silence is the reason this is worth a step of its own rather than a footnote. A backup regime that never started is indistinguishable from a working one until someone needs a restore, which is the failure mode the healthchecks in #147 exist to catch — and those healthchecks cannot fire on a container that is not running. The cutover runbook gains the directory creation before the stack is created, and step 7 now counts containers rather than only checking the app: four, all Up, with Created called out as the thing to look for. Counted from the compose file rather than from memory — the first draft said five. `backup-and-restore.md` gains the same note where it describes the destinations, since anyone reading that page is already thinking about paths. No `chown`, deliberately stated: both backup images run as root, unlike the Postgres image whose data directory needs uid 999, and an unnecessary chown instruction is how people learn to run them without thinking.	2026-08-26 10:14:10 -05:00
bermudalamb	085684c1d2	Merge pull request 'Feature/191 production demo mode interim' (#192) from feature/191-production-demo-mode-interim into main ... Reviewed-on: #192	2026-08-26 09:17:20 -05:00
bermudalamb	6104ebb459	feat(ops): read DEMO_MODE from the stack rather than the compose file (#190) ... Reverses the position the previous commit took. Hardcoding it made a value that gets flipped without a code change require a commit and a merge to flip, which is backwards — and it is the operator's call, not the file's. No default, deliberately. `${DEMO_MODE:-false}` is the obvious form and the wrong one: a default decides whether the shop takes money on the operator's behalf, silently, whichever way it points. Having none is safe rather than fragile because `checkDemoMode` is strict — an unset stack variable substitutes to an empty string, and anything that is not exactly `true` or `false` refuses to boot naming DEMO_MODE. That strictness is the whole reason interpolating this one is defensible, so the line says so. The compose guard had to learn the difference. It hands each deploying file's entries to the real validator, and a literal `${DEMO_MODE}` is not a value `checkDemoMode` accepts, so both the DEMO_MODE assertion and the validateEnv check failed the moment the file stopped holding a literal. Each deployment now declares the stack variables it supplies, and a bare `${VAR}` named there resolves to the declared value before the file is validated. Every other bare `${VAR}` stays opaque exactly as before — those are secrets, and what is checked of them is that the line exists. Be clear about what that guard can prove. It cannot see Portainer, so it does not verify the stack actually holds `true`; nothing in this repository can. What it does is keep the intent beside the file and make the pair inseparable — hardcode the compose line and the registry disagrees, change the registry and it no longer describes the file. The runtime half is the boot check, which fails loudly rather than falling back. Verified by mutation: hardcoding `false` fails the DEMO_MODE assertion, and deleting the line fails that and `validateEnv`. Restoring real payments is now two Portainer values and a redeploy, with no commit — which is what #190 asks for.	2026-08-26 09:17:20 -05:00
bermudalamb	12b2f09d79	docs(ops): point the demo-mode notes at the right issue (#190) ... The tracking issue was filed as #190; the compose banner and the guard test both said #191, guessing the number before it existed. A note that points at the wrong issue is worse than no note when the thing it tracks is production not taking money.	2026-08-26 09:17:20 -05:00
bermudalamb	0a830cad1f	feat(ops): put production in demo mode to complete the cutover (#191) ... Production could not boot during the cutover to the committed compose file: `DEMO_MODE` is false there, which makes the three PayPal secrets required, and they were not available. Demo mode is the interim the compose file's own header sanctions for exactly this — the whole cart and checkout flow works and nobody is ever charged. Two things made this cost more than it should have, and both are now written down rather than left to be rediscovered. `DEMO_MODE` is hardcoded rather than interpolated, so setting a `DEMO_MODE` stack variable in Portainer does nothing at all — there is no `${...}` for it to substitute into and the file's value wins silently. That hardcoding is right: the one value deciding whether the shop takes money should not be flippable from a web UI without a commit anybody can read. But the failure mode reads as "I set it and it ignored me", so the line now says so. Declaring `PAYPAL_CLIENT_ID=` with an empty value is identical to not declaring it. `isPresent` rejects a blank string deliberately, because set-to-nothing is a mistake rather than a value. The state is loud in both places that can see it. The compose file leads with a banner saying production is taking no money, and `composeEnvironment.test.ts` asserts `DEMO_MODE` is `true` — that assertion is the guard, not a formality: it fails the moment the file and the expectation disagree, in either direction, so this cannot be switched back quietly and cannot be left on unnoticed. #191 restores it.	2026-08-26 09:17:20 -05:00
bermudalamb	5f21d97178	Merge pull request 'docs(ops): name the crash loop the cutover runbook was most likely to cause (#175)' (#191) from feature/190-cutover-runbook-secrets into main ... Reviewed-on: #191	2026-08-26 08:41:27 -05:00
bermudalamb	0cec9b3c1f	docs(ops): name the crash loop the cutover runbook was most likely to cause (#175) ... Step 7 listed a missing `ADMIN_GATE_SECRET` — a warning the container starts through — and said nothing about the three PayPal secrets, which are a hard error that crash-loops it. That is backwards: the secrets are the likeliest thing to be missing after a stack replacement, because Portainer stack variables belong to the stack and are discarded with it, and `DEMO_MODE` is false in production so the app refuses to start without them. Hit for real following the runbook. The verify section now shows the actual log block, says what it means, and gives the way to tell a missing variable from a misnamed one: a hardcoded value cannot be missing, so its absence from the error list proves nothing, while an interpolated variable that stays quiet while others complain proves substitution works and those others are simply unset. That is the reading that turns the log into a diagnosis instead of a list. It also says the loop is harmless while the values are fetched — the container refuses before it serves anything or touches data — and where the values live if the old stack is already gone, since the webhook id in particular is readable rather than only recreatable. Step 2 now names every interpolated variable rather than describing them in general, and states the consequence of each class going missing. A general instruction to record the environment is easy to read as already done.	2026-08-26 08:35:01 -05:00
bermudalamb	f5e3ec3e99	Merge pull request 'Feature/188 filter dimensions' (#189) from feature/188-filter-dimensions into main ... Reviewed-on: #189	2026-08-25 16:25:21 -05:00
bermudalambandClaude Opus 5	3242018782	fix(filters): address the final review findings (#188) ... Restores the escape hatch for a status list matching no preset. filtersFromSearchParams accepts any non-empty subset of {available, reserved, sold}, and only three of those seven lists are presets; the other four reported as the not-sold fallback and produced no chip, so ?status=reserved was an empty grid reading "No items yet - check back soon" with no Clear filters button and no way out but editing the URL. hasActiveFilters covered all seven before this branch, so that was a regression against main. availabilityDimension now emits a chip for any status that is not the not-sold preset, labelled from SALE_STATE_LABELS when the list matches one and from statusLabel when it does not. The Segmented still reads "Not sold" beside such a chip, which is a cosmetic wart and the cheaper half of the trade. Runs the frontend unit suite in CI. The 22 tests added over the dimensions were invoked by nothing: the workflow's only frontend steps were the build and the end-to-end run, and its test:unit:cov step is the backend's. The new step is guarded and named in the gate like every other suite, per the invariant workflowGate.test.ts asserts. Restores the comment explaining why availabilityDimension's render and chips pass different fallbacks to saleStateFromStatuses. The control must read "All" while sold favorites are on screen; chips must stay silent because the customer never chose it. Unifying them would give every signed-in favorites view a phantom All chip and a tally of 2, and nothing said so after the old markup was deleted. Derives the storefront's "is anything filtered" and FilterBar's tally through one exported chipsFor rather than two expressions over two identically-built contexts. They agreed only by convention, which is the exact defect this branch exists to remove. Documents that statusDimension and availabilityDimension are alternatives over one field, since composing both type-checks and would render two controls that double-count it, and asserts the price chip's label text, which was the only chip label nothing checked. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>	2026-08-25 16:15:15 -05:00
bermudalamb	8771deeaca	test(filters): cover the tally and availability chip behaviour changes (#188)	2026-08-25 15:48:09 -05:00
bermudalamb	147e280f88	refactor(filters): compose the storefront from dimensions and delete the old components (#188) ... Replaces App.tsx's hand-written Segmented/Filters-button/ActiveFilterChips region with a single FilterBar composed from five dimensions, and removes the FilterDrawer and ActiveFilterChips components along with the activeFilterCount and hasActiveFilters helpers they were the only callers of. Catalogue now receives a filtered boolean computed the same way FilterBar computes its own chip tally, rather than the ItemFilters object it only ever used for that one check.	2026-08-25 15:39:18 -05:00
bermudalamb	2f0da9afe1	refactor(admin): compose the inventory filters from dimensions (#188)	2026-08-25 15:30:27 -05:00
bermudalamb	50d048a1d6	feat(filters): add FilterBar, one component both screens compose (#188)	2026-08-25 15:26:21 -05:00
bermudalamb	918d6eeab9	refactor(filters): extract the chip row from ActiveFilterChips (#188)	2026-08-25 15:23:25 -05:00
bermudalamb	1d7f928c48	feat(filters): move the availability preset into a bar dimension (#188)	2026-08-25 15:18:23 -05:00
bermudalamb	eb5799dd17	feat(filters): add favorites and status dimensions (#188)	2026-08-25 15:12:35 -05:00
bermudalamb	60b76cae82	docs(plan): correct the cumulative unit test counts (#188) ... Task 1 leaves 4 tests and Task 2 adds 7, so the running total is 11 rather than 12, and the same off-by-one carried into Tasks 3, 4 and the completion criteria. Caught by the Task 2 implementer, which flagged the mismatch rather than inventing a test to reach the stated number.	2026-08-25 15:07:13 -05:00
bermudalamb	abd3dd4e2e	feat(filters): add tag and price dimensions (#188)	2026-08-25 15:06:15 -05:00
bermudalamb	5d0b66a982	test(filters): add a unit runner and the filter dimension contract (#188)	2026-08-25 14:58:21 -05:00
bermudalamb	e695d91670	docs(plan): implementation plan for filter dimensions (#188) ... Nine tasks, each ending in something independently testable, in an order where every task leaves both screens working. The dimensions are built and unit-tested first, the shell after them, and the two screens are wired last — so the old components stay in place until the thing replacing them is proven. The two behaviour changes the design accepted are each covered twice: a unit test on the chips that produce them, and an end-to-end assertion on what a person sees. The admin tally reading three for three statuses, and a non-default availability producing a removable chip. Task 8 carries the deletions, deliberately last. Removing `activeFilterCount` and `hasActiveFilters` turns every remaining caller into a compile error, which is the cheapest way to find them. Two defects found reviewing the plan against the code rather than against itself: the test fixture omitted `Category.item_count` and would not have compiled, and Task 9 added a page object method that nothing used — `chooseAvailability` and `filterChip` already exist. The plan also records what must not be read as a regression. Two assertions in the storefront specs fail on every branch because the unpaginated grid cannot render 1,600+ development rows inside Playwright's default timeout, which is #186 and predates this work. Refs #188	2026-08-25 14:46:06 -05:00
bermudalamb	20a38b66bd	docs(design): filter dimensions, one composable component both screens extend (#188) ... #169 made the filter drawer shared, which was the right first move and not the finish. Per-screen differences are booleans, the bar around the drawer was never shared at all, and the storefront's availability preset sits outside the system because the shared component cannot express "this belongs in the bar, not the drawer". The design replaces the flags with composition: a screen contributes a list of filter dimensions, each declaring where it renders, how to render it, and what chips it contributes. A screen-specific control becomes an ordinary dimension, appearing in the chip row and counting toward the tally without the shared code knowing what it is. Dimensions are plain data rather than components or context, for a concrete reason rather than a stylistic one. The drawer sets destroyOnHidden, so its sections are unmounted whenever it is closed — exactly when the chip row matters most. Anything that registers on mount would lose those chips the moment the drawer closed, which rules out the otherwise-idiomatic context-and-children approach. The tally becomes the number of chips, so the count and the chip row cannot disagree — today they are computed by two routes and agree by coincidence, which the admin already has to correct by hand. Three visible behaviours change as a result, recorded in the spec rather than left to be discovered. Adding vitest is in scope. The design's value rests on chips() being pure, and the frontend has no unit runner at all, so without one the core of it would ship covered only indirectly and expensively through Playwright. The spec also records what is deliberately untouched: the filter state, the URL serialisation, the backend, and the two e2e assertions already failing on #186 — which must not be read as regressions from this work. Refs #188	2026-08-25 14:36:51 -05:00
bermudalamb	2a19238109	Merge pull request 'feat(filters): show a tag's own colour on its active filter chip (#185)' (#187) from feature/185-tag-chip-colour into main ... Reviewed-on: #187	2026-08-25 14:05:34 -05:00
bermudalamb	d6e0942487	feat(filters): show a tag's own colour on its active filter chip (#185) ... A tag carries a colour, and every place a tag appears shows it — a product card, the filter drawer's control, the admin taxonomy screen — except the removable chips beside the Filters button, which rendered every filter as a default grey. Picking `vintage` from a control that showed it in red produced a grey chip of the same name right next to it. Only tags get a colour, because only tags have one. Category, price, favorites and status keep the default, and that asymmetry is the point: in a row mixing four kinds of filter, colour now means "this is a tag". Nothing depends on it — every chip still carries its label — so this reads the same to anyone who cannot distinguish the colours. The close control inherits the tag's text colour, so a coloured chip gets a matching cross rather than a grey one on a coloured ground. A tag not yet in the loaded options has no colour to use and keeps the default, which is the same window the existing `Tag {id}` label fallback covers. The test asserts the chip's colour equals the same tag's colour on a product card, rather than asserting it is red. The colour is derived from the tag's name and free to change; what must hold is that a tag looks like itself wherever it appears, and comparing the two places says that directly. Confirmed to fail without the change — the old grey chip sets no colour class at all. Verified visually as well as by assertion: three tags selected together render in the drawer, in the chip row and on the card in the same colours. Closes #185	2026-08-25 14:05:34 -05:00
bermudalamb	3cffcf772c	Merge pull request 'refactor: remove the duplicated blocks SonarQube found (#182)' (#184) from feature/182-remove-duplication into main ... Reviewed-on: #184	2026-08-25 13:13:28 -05:00
bermudalamb	61c12fd438	refactor: remove the duplicated blocks SonarQube found (#182) ... Three of the four candidates were real. The fourth was my mistake in the issue. **The category tree adapter**, duplicated verbatim between `CategoryTreeSelect.tsx` and `FilterDrawer.tsx`. This one was mine: #139 moved the storefront filter to a `TreeSelect` and copied the admin's adapter rather than sharing it, with a comment saying the shape "matches the admin's CategoryTreeSelect so the two stay comparable" — an argument for one implementation that instead produced two. It now lives in `filters.ts` beside `buildCategoryTree`, which was already shared for exactly the same reason: one meaning, one implementation. `Categories.tsx` keeps its own. It builds a different shape for a real antd `Tree`, keyed rather than valued, with a title that is a React node carrying that screen's buttons. Genuinely different, and folding it in would mean a parameterised adapter that serves neither case clearly. **The `item_images` insert loop**, written separately by create and update and differing only in where the id came from and where the sort order started. Both are parameters now, which also means the `/uploads/` prefix is written once — #103 made that the value `uploadUrl` joins an origin onto, so it is a contract rather than a string. Extracting it turned up two things the inline versions hid. Create indexed `files[i]?.filename ?? ''`, so a missing element would have stored a path pointing at the uploads directory itself; iterating by entry removes the possibility rather than defending against it. And the helper's typed `itemId` surfaced that `req.params.id` is `string | undefined` under `noUncheckedIndexedAccess`, which the old inline `unknown[]` swallowed — now `Number()`, as the `setItemTags` call two lines above already did. **The optional-field guards**, eight identical lines opening both routes. The distinction worth preserving is that `undefined` means "not submitted", which update reads as "leave as-is", so an unparseable value has to be told apart from an absent one. That is what makes it more than a null check and worth stating once. **`TAG_COLORS` was not a duplication.** The issue listed four files on the strength of a grep that also matched `STATUS_TAG_COLORS` in `Admin.tsx` — a status-to-colour map for the inventory table, unrelated to the tag palette. What remains is one definition in `backend/src/utils.ts` and one mirror in `frontend/src/admin/Tags.tsx`, already carrying a comment pointing at the other, which is the same treatment `ALLOWED_IMAGE_TYPES` gets and is correct: there is no shared package, and creating one for a colour list would cost more than it saves. Verified beyond the type checker, since three of these are pure moves that compile either way: 278 unit and 254 integration tests, and the end-to-end specs covering both consumers of the shared adapter — the storefront drawer and the admin item form's category picker, including inline category creation. Closes #182	2026-08-25 13:08:58 -05:00
bermudalamb	76797ee6f6	Merge pull request 'fix(security): stop refused uploads accumulating on the volume, and record the hotspot review (#180)' (#183) from feature/180-security-hotspots into main ... Reviewed-on: #183	2026-08-25 11:50:28 -05:00
bermudalamb	b616b9f0ab	fix(security): stop refused uploads accumulating on the volume, and record the hotspot review (#180) ... SonarQube reported three security hotspots, all in `routes/admin.ts`. A hotspot is not a defect — it marks code that touches something security-sensitive and needs a human decision — so the work is a recorded review, with a change only where the review finds a real gap. It found one. The gap: multer writes every file to disk before any route logic runs, and multer's own cleanup only covers errors it raised itself. Everything after that left the bytes behind with nothing referencing them. A request carrying a perfectly valid photograph and a malformed `category_id` is refused with a 400 after the write, and the file stays on the volume permanently — no database row to find it by, and no bound on how many can accumulate. The same held for a malformed `tags` field, for a database error rolling the transaction back, and for `readHead` itself throwing, which returned no message and so cleaned up nothing. That is the substance of the limits the first hotspot points at. Bounding one request to 8 MB across six files does nothing if every refused request keeps its bytes for ever, and the admin API is the one surface where that is reachable. The fix is a hook rather than a call at each `return`, registered the moment multer succeeds. A route added later inherits it instead of having to remember it, which matters because the failure being prevented is precisely someone adding a fourth early return. It listens on `close` rather than `finish` so an aborted connection is covered, and checks `writableEnded` so a response that never completed is not mistaken for a success whatever its status code reads. `verifyUploadedImages` goes back to checking only. Removing the files there as well would unlink twice and log an ENOENT for every refused upload, and the single mechanism covers the case it used to miss. The other two hotspots are safe, and now say why in the file rather than only in SonarQube's UI — following the precedent of the existing comment that names S5693 by rule number. The upload path is not caller-controlled despite arriving from a request: multer composes it from a server constant and a `randomUUID()` plus an extension looked up from the validated content type, so the caller's `originalname` never reaches the filesystem. That reasoning belongs next to the `fs.open` that depends on it. Three tests, written first and failing first: a refused sibling field, a refused tags field, and the accepted case, which must not be swept up by the same cleanup. 254 integration and 278 unit tests pass. Refs #180	2026-08-25 11:50:28 -05:00
bermudalamb	e3514e8ef4	Merge pull request 'fix(ci): stop the test summarisers failing the job on an unreadable results file (#178)' (#179) from feature/178-resilient-summarisers into main ... Reviewed-on: #179	2026-08-25 11:49:11 -05:00
bermudalamb	5d427bc1a1	fix(ci): stop the test summarisers failing the job on an unreadable results file (#178) ... Run 525 was the first with #174's graceful failure, and it worked: the integration suite failed, the end-to-end suite ran again, and `SonarQube Scan` succeeded for the first time since #154 started. But `Summarize integration tests` failed, and that step should not be able to. Both scripts already carried the principle in a comment — report plainly and exit 0, because the job fails on the real step and a stack trace here would only bury it — and both only implemented it for the file being absent. A file that exists and cannot be read crashed them. Two ways to reach that, both reproduced. `--forceExit`, which the integration script passes to paper over a post-run hang, can end the process around the write and leave partial JSON. And a suite that fails to *run* rather than to assert arrives without the array the failure renderer walks, which is exactly the shape this suite has been producing under #154. Reading is now guarded as thoroughly as `summarize-playwright.js` already guarded its traversal, and that traversal's `|| []` discipline is extended to the jest renderer. `summarize-playwright.js` had the same hole by the narrower path of an unguarded `JSON.parse`. The reason reaches the log rather than being swallowed. "Could not read the results file" with the parse error is diagnostic; a silent empty summary is not. This matters beyond tidiness because of where the failure lands. A crash here reports the job as failing at a step named for summarising rather than for testing, which is the misdirection #142 fixed once already — and a summariser whose job is to make a failing run readable should not crash on the output of the worst failures, which is the moment it is most needed. Verified against a truncated file, a suite entry with no `testResults`, an absent file, and a real 278-test run: the first three now exit 0 naming the reason, the absent case is unchanged, and the happy path still reports its counts. Closes #178	2026-08-25 09:20:34 -05:00
bermudalamb	ed81de3b06	Merge pull request 'docs(ops): write down how to cut production over to the committed compose file (#175)' (#177) from feature/175-prod-cutover-runbook into main ... Reviewed-on: #177	2026-08-25 09:16:15 -05:00
bermudalamb	cf9003bc2d	docs(ops): write down how to cut production over to the committed compose file (#175) ... #118 put `docker-compose.prod.yml` in the repository and argued at length for why it belongs there. It never produced the procedure for getting production from where it is — a stack living in Portainer's web editor — to where that file expects it to be. What existed was scattered and none of it was a procedure. The compose header described the destination: name the stack this, deploy it as a git repository stack with these settings, rename any variables whose names differ. The README's deployment section is a good runbook for the routine redeploy, and assumes the stack is already in the right form. So the riskiest deployment operation this project has was the one with no steps written down. `docs/ops/production-stack-cutover.md` is those steps, in the order that matters, with the two facts that decide whether the operation is safe stated up front rather than left to be inferred from the volumes block. The first is that all persistent data is bind-mounted from the NAS filesystem rather than held in Docker-managed volumes, so deleting the stack cannot lose the database or the product images. That is what makes the cutover recoverable at all, and step 2 verifies it rather than trusting it — the `docker inspect` there discriminates `bind` from `volume` and says to stop on `volume`, because this runbook does not cover that case. The second is that both services set an explicit `container_name`, so the old containers must be gone before the new stack starts or the deploy fails on a collision that reads like a Portainer bug rather than a sequencing mistake. It also captures what is simply lost if not recorded first. Portainer stack variables belong to the stack and are discarded with it; they are all secrets, and a stack brought back up with a different `DB_PASSWORD` than the data directory was initialised with cannot authenticate against its own database. Recording them is step 2 and it is what makes the rollback credible. The verification section separates the boot warnings that are correct in production — no mail allowlist, no uploads origin yet — from the one that means a variable never arrived. That distinction is the whole failure mode #118 was about: a stack variable whose name matches nothing in the file is substituted nowhere and never reaches the container, and the failure reads as "I set it and it says it is not set". Every diagnostic command in it was run rather than written from memory. The mount inspection was checked against a container that genuinely uses a named volume, to confirm it reports the difference the step depends on. The compose header now points at the runbook for the procedure and keeps the rationale that belongs at the point of use — why there is no `build:`, why most values are hardcoded, why `NODE_ENV` is absent. The README says plainly that its section is the routine deploy and links to the other one. Closes #175	2026-08-25 09:16:15 -05:00
bermudalamb	050420858f	Merge pull request 'ci: fail at the end rather than part way through, so the scan still runs (#174)' (#176) from feature/174-graceful-sonarqube-failure into main ... Reviewed-on: #176	2026-08-25 09:15:26 -05:00
bermudalamb	e3842b1a4c	ci: fail at the end rather than part way through, so the scan still runs (#174) ... `sonarqube.yml` already had a documented design for this: run every suite, produce coverage, scan, summarise, then fail at the end from recorded step outcomes. The comments on the gate spell it out and #142 fixed it once already. The integration suite was never wired into it — no `id`, no `continue-on-error`, and absent from the gate, where the unit and end-to-end suites had all three. So it was the one suite whose failure aborted the job. On every push since #154 started, step 9 failed and steps 10 through 15 were skipped, which means there has been no SonarQube analysis at all for the duration — not a degraded one, none. The end-to-end suite has not run in CI either, which is separately why #116 cannot be verified: the step that would demonstrate its fix is skipped rather than failing. Fixing only that step would have left the same shape in four other places, so every step from the first suite to the scan is now guarded and named in the gate: starting the backend, installing browsers, merging frontend coverage, and the scan itself, which until now took the summaries down with it. The preconditions before the suites — checkout, installs, build checks, migrations — still fail hard, because when they fail there is genuinely nothing to analyse. The job still fails. It fails at the end, having produced everything it could. The integration suite also gains the summary the other two already had. It was the only suite without one, so the failure this workflow has been stuck on presented as 36 assertion errors about categories and price filters rather than as a count — #154 records how expensive that misdirection was to read. `summarize-jest.js` already takes a label, so this is reuse. `tests/unit/workflowGate.test.ts` asserts the pairing that makes the design work: a step carries `continue-on-error` so it cannot abort the job, and the gate names it so it can still fail the job. Both halves are needed and nothing connected them, which is how this happened — and the other direction is worse, since a step guarded but unnamed cannot fail the job at all. The test checks the invariant rather than a list of names, for the same reason `composeEnvironment.test.ts` reads the real list rather than a copy. Verified by mutation: dropping the integration suite from the gate, un-guarding it, and removing `always()` each fail it. Confirmed by running the new flag combination rather than assuming it: the integration suite writes `integration-results.json`, the summariser reads it and exits 0, and both that file and `coverage/integration/lcov.info` are still written when the suite fails — which is what makes scanning with a failing suite produce real coverage rather than a fabricated regression. Closes #174	2026-08-25 08:47:42 -05:00
bermudalamb	667a40c4eb	Merge pull request 'feat(security): stop the app origin serving anything it does not recognise, and make the uploads origin configurable (#103)' (#173) from feature/103-uploads-origin into main ... Reviewed-on: #173	2026-08-24 17:38:55 -05:00
bermudalamb	cf1680dbfb	feat(security): stop the app origin serving anything it does not recognise, and make the uploads origin configurable (#103) ... The uploads directory is the only place in this application where content someone else authored is served over HTTP. #95 stopped a dangerous file being stored; this stops a stored file doing damage if one ever gets there anyway — through a gap, a path added later, a restore, or a file written before that validation existed. Two halves, complementary rather than alternative. The app's own origin now serves uploads defensively. An allowlist of the three extensions the upload path can produce, so a `.html` or a `.svg` on disk is simply not a file this application hands out — 404, the same answer as a file that is not there, so the response cannot be used to learn which paths exist. An allowlist rather than a denylist because a denylist has to anticipate every type a browser might execute, which is a moving target across browsers and years, while this only has to know three. The content type is stated explicitly from that same list rather than sniffed or guessed from a name someone else chose, paired with `nosniff`. `default-src 'none'; sandbox` gives a directly-navigated file no capabilities at all, which is the only way one of these can do harm — an `<img>` embed does not execute script. Writes get 405 rather than falling through to a 404 that suggests the path is wrong. The other half is the separate origin, which is the real fix, because the origin is the whole unit of trust in a browser. That needs a hostname and a certificate, which live outside this repository, so what is here is the switch: `UPLOADS_BASE_URL`, sent to the frontend at runtime through `/api/config` and joined onto stored paths by `uploadUrl`. Empty means the app's own origin, which is the default and what local development has, so nothing changes until it is pointed somewhere. Stored paths stay site-relative. A stored value outlives any hostname baked into it, and rewriting them would be a migration to undo the day the hostname changes. Runtime rather than built in, so one image serves every environment — the same reason `paypalClientId` and `demoMode` are already there. `UPLOADS_BASE_URL` has a line in `docker-compose.prod.yml` while still empty, deliberately: a Portainer stack variable with no line there is substituted into the file and never reaches the container, which is exactly how `UPLOADS_DIR` went missing on 2026-08-23. Unset warns at boot, in the same shape as the admin gate — a working configuration with one defence switched off is worth saying out loud. Set without a scheme is refused outright, because a bare hostname joins onto a stored path as if it were relative and breaks every image on the site rather than failing visibly. The compose guard now resolves `${VAR:-default}` to its default, which is what the container actually receives when the stack variable behind it is unset. A bare `${VAR}` is still left opaque, so a required variable referenced that way goes on counting as present — that check is about the line existing, not about the stack being filled in. Closes #103	2026-08-24 17:38:55 -05:00
bermudalamb	5d72c1c88b	Merge pull request 'test(perf): measure what concurrent hashing actually costs a bystander request (#163)' (#172) from feature/163-measure-hash-latency into main ... Reviewed-on: #172	2026-08-24 17:02:52 -05:00
bermudalamb	313b48f582	test(perf): measure what concurrent hashing actually costs a bystander request (#163) ... #163 claimed `bcryptjs` blocks the event loop and that every login stalls every other request in flight. That was asserted without measurement and is wrong: the asynchronous API chunks its work and yields between rounds, and all six call sites in `routes/customers.ts` use it — there is no `hashSync` or `compareSync` anywhere in `src`. A smaller effect is real, though, and this measures it instead of arguing about it. The probe is `GET /api/customers/me` with no cookie, chosen for doing almost nothing: it rejects before touching the database, so nearly all of its latency is time spent waiting for the event loop rather than work of its own. The load is real registrations against the real route, because the question is what a deployed server does rather than what bcrypt does on a bench. Measured on the dev machine, Node 24, cost 12: | Concurrent registrations | Each registration (p50) | Bystander p95 | Bystander worst case | | --- | --- | --- | --- | | idle | — | 0.3 ms | 5.4 ms | | 1 | 213 ms | 0.6 ms | 102 ms | | 4 | 803 ms | 101.9 ms | 405 ms | | 8 | 1626 ms | 15.6 ms | 808 ms | Both columns are linear in the number of queued hashes. Registration is roughly 200 ms times the concurrency, because the hashes serialize onto the one thread. The bystander's worst case is roughly 100 ms times the concurrency, which matches the coarseness of the chunks measured earlier — about 100 ms of un-yielding time per hash. The median stays under a millisecond throughout, so this is a tail-latency characteristic and not the stall the issue described. The benchmark creates real customers and deletes them again, because it is normally pointed at a development database that nothing truncates. Lint now covers `scripts` as well as `src`, so the one file in it is held to the same standard as the rest.	2026-08-24 16:59:33 -05:00