e7fd63c2e60fc1c85cf18ed34bb7b0702e5033de
374
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 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 |
||
|
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 |
||
|
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 |
||
|
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 |
||
|
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 |
||
|
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 |
||
|
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 |
||
|
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 |
||
|
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 |
||
|
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 |
||
|
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 |
||
|
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 |
||
|
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 |
||
|
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 |
||
|
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 |
||
|
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 |
||
|
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 |
||
|
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 |
||
|
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 |
||
|
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 |
||
|
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 |
||
|
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 |
||
|
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 |
||
|
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 |
||
|
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
|
||
|
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 |
||
|
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 |
||
|
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 |
||
|
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 |
||
|
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 |
||
 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>
|
||
|
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 |
||
|
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 |
||
|
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 |
||
|
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
|
||
|
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 |
||
|
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 |
||
|
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 |
||
|
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 |
||
|
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 |
||
|
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 |
||
|
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 |
||
|
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> |
||
|
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 |
||
|
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> |
||
|
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 |
||
|
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> |
||
|
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 |
||
|
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> |
||
|
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 |