Skip to content

Commit 3ba86b0

Browse files
vedanshujainclaude
andcommitted
[Plugin] INC-R3 review: record the products drop in ADR-0015, restore three service-facing assertions
The reviewer verified every invariant and the Decision 5 relocation and found the code sound. Five residuals, none of which change what the write path does: - **ADR-0015 gains a SECOND amendment.** The first is scoped by its own text to three named Orders ids, and its own argument — that enumerating only what an earlier record listed is not a record of what was lost — applies to itself. The Pricing & inventory drop is now recorded on its own terms: `products:remove-stock-review` is not ported, taking the DA-3c bound check, the `REMOVE_STOCK_INVALID_QTY` field-level refusal and the `remove-draft`/`remove-staged` render state; what protects the reachable path instead (the service's guarded decrement refusing with the real on-hand, and the store contract pinning that it never goes negative); and that re-introducing a staged removal means WRITING these checks, not restoring them. Index and changeset mirror it. - **Three deleted assertions restored**, each a claim about what the SERVICE is asked for rather than about a rendering: the admin token on the list and detail GETs (every surviving header assertion was on a write); the absent-token -> 401 fail-closed trigger (the anti-leak contract was otherwise exercised only through a 500); and `active` + `productKind` + `search` travelling together in one query. Each was proven to bite by mutation. - **The two watermarks are guarded symmetrically.** A blank `expectedUpdatedAt` was forwarded and refused downstream while a blank on-hand refused at the boundary; the blank edit watermark now refuses at the boundary too, so a future change to either tier cannot quietly open one of them. - Two stale doc references corrected, and the callerless block-tree half of the console transport is marked as retained pending its own increment. - Recorded as follow-ups, not fixed: the stale-edit refusal's "latest values are shown below" promise is asserted at neither tier, and the edit idempotency key canonicalises absent and explicit-null identically (unreachable across the three shipped forms, a footgun for a fourth). Verification: plugin 893 passed / 42 skipped; admin-react 51 passed; site-staging 1026 passed; typecheck, lint and format clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SDKNwyZU4mgLAaqMnBT7PU
1 parent 52bd2f2 commit 3ba86b0

9 files changed

Lines changed: 314 additions & 33 deletions

.changeset/products-write-path-extraction.md

Lines changed: 37 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,9 @@ removing it is a rewrite and not a deletion.
2424
`onHand` the operator saw is re-read against live truth before any stock
2525
moves, and refuses on a mismatch, with an absent watermark refused fail-closed
2626
and with no re-read); the **edit watermark** (`expectedUpdatedAt` is mandatory,
27-
and a save without one refuses rather than clobbering); **money as integer
27+
and a save without one — or with a blank one — refuses rather than clobbering,
28+
guarded at the same tier as the stock watermark rather than left to the service
29+
to reject); **money as integer
2830
minor units** (an exact decimal parse, a positive amount, a required ISO-4217
2931
currency, and a blank compare-at as an explicit clear rather than a zero); and
3032
**content-derived idempotency keys** with no nonce anywhere.
@@ -40,16 +42,45 @@ removing it is a rewrite and not a deletion.
4042
branches. The suite's behavioural half moves onto the new path as
4143
`products-actions.sandbox.test.ts`, which drives the writes the way the console
4244
does, inside the workerd sandbox.
43-
- **`products:remove-stock-review` is left unported as unreached surface.** It
45+
- **`products:remove-stock-review` is left unported as unreached surface, and
46+
ADR-0015 carries a second amendment recording exactly what went with it.** It
4447
staged a quantity server-side so a second render could draw a confirm button;
4548
the React screen shows that dialog over the values just typed, which is why the
4649
console's gate has excluded the id since the migration and why nothing
47-
reachable has ever called it. One check lived only on that step and so never
48-
ran for any shipped surface — the bound of the quantity against the on-hand
49-
just re-read. An over-removal is refused by the service's guarded decrement
50-
instead, which the new suite asserts.
50+
reachable has ever called it. Three things lived only on that step and so never
51+
ran for any shipped surface: the **DA-3c bound check** of the requested
52+
quantity against the on-hand just re-read, the **`REMOVE_STOCK_INVALID_QTY`**
53+
field-level refusal, and the **`remove-draft`/`remove-staged` render state**.
54+
What protects the reachable path instead is the service's guarded decrement,
55+
which refuses an over-removal with the real on-hand and is surfaced as a named
56+
refusal quoting that count (asserted by the new suite), plus the inventory-store
57+
contract suite pinning that an over-removal removes nothing and never goes
58+
negative, on every adapter. Re-introducing a
59+
server-side staged removal means WRITING these checks, not restoring them.
5160

5261
The console's Pricing & inventory sidebar entry loses its `(new)` suffix. It
5362
existed to tell this screen apart from the Block Kit screen at the same path;
5463
with that screen gone, a single entry marked new against nothing is the
5564
misleading thing (ADR-0015 Decision 1).
65+
66+
**Three read-path assertions were rescued from the deleted suite rather than
67+
written off as render-only**, because each is a claim about what the SERVICE is
68+
asked for and outlives the renderer: the internal admin token travelling on the
69+
list and detail GETs (every surviving header assertion was on a write); the
70+
absent-token → 401 fail-closed trigger (the anti-leak contract was otherwise
71+
exercised only through a 500, and an unconfigured token is the failure an
72+
operator actually meets); and the three filter axes — `active`, `productKind`
73+
and `search` — travelling together in ONE query rather than only one at a time.
74+
75+
**The block-tree half of `console-transport.ts` now has no callers**
76+
`firstNotice`, `forwardConsoleAct`, `forwardedFormSubmit` and `nothingApplied`,
77+
callerless once both consoles are off Block Kit. It is left byte-identical with a
78+
module note saying so; ADR-0015 Decision 1 puts its removal in the increment that
79+
follows, deliberately as its own change.
80+
81+
**Known coverage gap, recorded rather than fixed here.** The stale-edit refusal
82+
tells the operator "the latest values are shown below", and with the Block Kit
83+
re-render gone that promise rests entirely on the React screen re-reading the
84+
product after a refused save. Nothing asserts it at either tier: the plugin suite
85+
ends at the refusal notice, and the React suite does not pin the re-read. The
86+
behaviour is correct today; only the guard against it regressing is missing.

adr/0015-retire-duplicated-block-kit-screens.md

Lines changed: 102 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,14 @@
1010
refusal — is unchanged and still binding. Decision 2's "the staged state a two-step flow
1111
needs" is **moot**, not unaffected: the deletion removes exactly that member. Decisions 1,
1212
4, 5 and 6 stand as written. See "Amended 2026-08-03" at the end of this record.
13+
- Amended: 2026-08-03, second — **Decision 3 again, this time for Pricing & inventory.** The
14+
first amendment above is scoped by its own text to three named ORDERS ids, and its own
15+
argument — that enumerating only what an earlier record listed is not a record of what was
16+
lost — applies to itself. This second amendment records the Pricing & inventory drop:
17+
`products:remove-stock-review` is not ported, taking the DA-3c bound check, the
18+
`REMOVE_STOCK_INVALID_QTY` field-level refusal and the `remove-draft`/`remove-staged` render
19+
state with it. Neither had a reachable caller. The stale-watermark refusal is again
20+
unchanged and still binding. See "Amended 2026-08-03, second" at the end of this record.
1321
- Supersedes: **one clause of [ADR-0014](./0014-second-native-descriptor-for-react-admin.md)**
1422
"The Block Kit screens stay in the tree and stay green until a migration increment replaces
1523
each one" — **as to Orders and Pricing & inventory only**. That clause is the third bullet of
@@ -363,3 +371,97 @@ member.
363371
tests that drove the deleted ids, and each of those pinned behaviour that no longer exists
364372
rather than behaviour that moved somewhere else.
365373
- Reports and Coupons remain unruled; Tax, Shipping and Settings remain Block Kit permanently.
374+
375+
## Amended 2026-08-03, second — Decision 3 again, for Pricing & inventory
376+
377+
Everything above the first amendment is left exactly as written on 2026-08-01, and the first
378+
amendment is left exactly as written. This block records one more change to the same decision.
379+
380+
**Why a second block rather than an edit to the first.** The amendment above is scoped by its
381+
own text to three named Orders ids, and it argues — in its own words — that "a record of what
382+
was lost that enumerates only what an earlier record happened to list is not a record of what
383+
was lost". That argument applies to the amendment itself: it enumerates the Orders drop and
384+
nothing else, so folding the Pricing & inventory drop into it silently, or leaving it out
385+
altogether, repeats exactly the mistake it was written to correct. Decision 3 still says a
386+
dropped refusal needs its own record. This is that record, for the second screen.
387+
388+
### What happened
389+
390+
The Pricing & inventory write-path extraction found the same shape one screen along.
391+
`products:remove-stock-review` was DA-3 state 1 → state 2 for the Block Kit screen: it parsed
392+
a quantity, staged it server-side together with the on-hand watermark, and returned a second
393+
render carrying a confirm button — because a Block Kit form cannot show a dialog over the
394+
values just typed. React can. The React screen composes its own confirm client-side and posts
395+
`products:remove-stock` directly, which is why the console's action gate has EXCLUDED the
396+
review id since the migration increment, and why **no shipped surface has ever reached that
397+
step**. Two reviews confirmed it independently: the id is unreachable, and an over-removal is
398+
refused on the path that is reachable.
399+
400+
The reasoning of the first amendment carries over unchanged and is not restated at length: an
401+
unreachable safety check is not a safety check. It is a claim in the tree that a check is being
402+
made, which a reader has every reason to believe and which the running system does not honour.
403+
404+
### The decision
405+
406+
**`products:remove-stock-review` is not ported, as unreached surface.**
407+
408+
**What goes with it — the whole of it, not only what Decision 3 happened to name:**
409+
410+
- the **DA-3c bound check** — the requested quantity tested against the on-hand JUST re-read,
411+
so that a confirm could never name a quantity already false at the moment it was drawn. It
412+
lived only on the review step;
413+
- the **`REMOVE_STOCK_INVALID_QTY` field-level refusal** — the per-field line an unparseable or
414+
non-positive quantity produced against the staged form's own input. Decision 3 never named
415+
it, which is exactly why it is named here;
416+
- the **`remove-draft` / `remove-staged` render state** — the staged quantity plus watermark
417+
the step handed back for a second render, and the draft that carried the operator's raw text
418+
into a refusal. Both are members of a result shape that only a server-rendered second state
419+
needs; the new outcome (Decision 2, as narrowed by the first amendment) is the flag and the
420+
notice.
421+
422+
None of the three ever ran for any surface that shipped. Dropping them removes no protection
423+
any operator has had.
424+
425+
### What protects the reachable path instead
426+
427+
**The removal that IS reachable is guarded in three places, and the bound is one of them.**
428+
429+
- **The service applies a guarded decrement.** Removing more than is on hand is refused there —
430+
never a negative count, never an oversell — and the refusal comes back with the REAL on-hand,
431+
which the console surfaces to the operator as a named refusal quoting the actual count rather
432+
than as a generic failure. That is a better bound than the deleted one in the respect that
433+
matters: it is taken by the same statement that would have applied the movement, so nothing
434+
can change between the check and the write.
435+
- **The domain contract pins it.** The no-negative-stock behaviour is a contract-suite
436+
invariant, not an implementation detail of one adapter, so it holds for every store the
437+
service runs on.
438+
- **The stale-watermark refusal (DA-3a) still runs first**, on the reachable path, and is
439+
untouched: the on-hand the operator saw is re-read against live truth before anything moves,
440+
a mismatch refuses with nothing posted, and an ABSENT watermark refuses fail-closed with no
441+
re-read at all.
442+
443+
What is genuinely gone is the *earlier* refusal — the one that could tell an operator the
444+
quantity was too large before any request left the plugin. On the reachable path that
445+
conversation now happens one round trip later, and it names the real number when it does.
446+
447+
### The consequence to be clear-eyed about
448+
449+
**Re-introducing a server-side staged removal later means WRITING these checks, not restoring
450+
them.** There is nothing left to restore, and a future flow will not have the same shape: the
451+
deleted bound check assumed a quantity staged between two server round trips and a watermark
452+
carried across them, and the deleted field-level refusal assumed a server-rendered form with a
453+
field to attach itself to. Anyone adding that flow owns all of it as new work. This amendment
454+
is not permission to ship a staged removal without them.
455+
456+
### What is NOT changed
457+
458+
- Decisions 1, 4, 5 and 6 stand as written, and Decision 2 stands as narrowed by the first
459+
amendment — the outcome is the flag and the notice, because no two-step flow remains on
460+
either screen.
461+
- **The stale-watermark refusal is Decision 3's binding requirement and is carried across
462+
verbatim on this screen**, for both of its watermarks: the stock on-hand and the edit's
463+
`expectedUpdatedAt`. A reworded one is still a failed port.
464+
- ADR-0006 Decision 1 is reaffirmed a fourth time: the sandbox suites remain the contract gate.
465+
The Pricing & inventory write path is proven in the workerd sandbox on its new module, and
466+
the retired screen's suite loses only assertions about a rendering that no longer exists.
467+
- Reports and Coupons remain unruled; Tax, Shipping and Settings remain Block Kit permanently.

adr/README.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -44,7 +44,7 @@ than rewriting history.
4444
- [0012. The storefront checkout loads Stripe Elements in the buyer's browser](./0012-storefront-checkout-loads-stripe-elements-in-the-browser.md) — accepted, refines 0003, builds on 0006/0009/0010
4545
- [0013. Product title is CMS-owned; `product_commerce.title` is a derived single-writer cache](./0013-product-title-is-cms-owned.md) — accepted, refines 0001/0002 (the hybrid product model); promotes the queued "one home per field" decision, and completes the 2026-07-29 amendment on ADR-0001 (commercial fields are edited only in the admin console)
4646
- [0014. A second descriptor `otta-console` (native format) may serve React admin screens](./0014-second-native-descriptor-for-react-admin.md) — accepted, amends ADR-0006 **Decision 2 only** (React admin pages on a separate descriptor + separate package); ADR-0006 Decision 1 — the 18 workerd sandbox suites as the contract gate — is reaffirmed unchanged; corrected 2026-08-01 (Decision 7's page count: seven Block Kit pages, not six); **partially superseded** 2026-08-01 by [ADR-0015](./0015-retire-duplicated-block-kit-screens.md) — the "Block Kit screens stay in the tree" clause only, and only for Orders and Pricing & inventory; every other clause, including Decision 6, stands
47-
- [0015. The duplicated Block Kit Orders and Pricing & inventory screens are to be retired](./0015-retire-duplicated-block-kit-screens.md) — accepted, supersedes ADR-0014's "the Block Kit screens stay in the tree" clause **for those two screens only** (Tax/Shipping/Settings stay Block Kit permanently; Reports and Coupons stay unruled); ADR-0006 Decision 1 reaffirmed again — the sandbox suites remain the contract gate. **Authorised, not yet landed:** the removal is conditional on each screen's write path being re-implemented off Block Kit first, so both screens are still in the tree; **amended 2026-08-03** — Decision 3 only: the unreached two-step `-review` pair is deleted, taking the refund-ceiling bound check and the unparseable-amount refusal with it (neither had a reachable caller); the stale-watermark refusal is unchanged
47+
- [0015. The duplicated Block Kit Orders and Pricing & inventory screens are to be retired](./0015-retire-duplicated-block-kit-screens.md) — accepted, supersedes ADR-0014's "the Block Kit screens stay in the tree" clause **for those two screens only** (Tax/Shipping/Settings stay Block Kit permanently; Reports and Coupons stay unruled); ADR-0006 Decision 1 reaffirmed again — the sandbox suites remain the contract gate. **Authorised, not yet landed:** the removal is conditional on each screen's write path being re-implemented off Block Kit first, so both screens are still in the tree; **amended 2026-08-03** — Decision 3 only: the unreached two-step `-review` pair is deleted, taking the refund-ceiling bound check and the unparseable-amount refusal with it (neither had a reachable caller); the stale-watermark refusal is unchanged; **amended again 2026-08-03 (second block)** — Decision 3 for the second screen: `products:remove-stock-review` is not ported, taking the DA-3c bound check, the `REMOVE_STOCK_INVALID_QTY` field-level refusal and the staged/draft render state with it (again no reachable caller — the service's guarded decrement refuses an over-removal with the real on-hand, and the domain contract pins that it never goes negative); the stale-watermark refusal is again unchanged
4848

4949
## Queued (to promote from draft-plans)
5050

packages/plugin/src/admin/console-transport.ts

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,19 @@
2121
* product" on Pricing & inventory — because a refusal that tells an operator to
2222
* reload the wrong thing is worse than one that says nothing.
2323
*
24+
* FOUR SYMBOLS BELOW HAVE NO CALLERS AS OF INC-R3, AND ARE RETAINED ON PURPOSE.
25+
* {@link firstNotice}, {@link forwardConsoleAct}, {@link forwardedFormSubmit}
26+
* and {@link nothingApplied} are the block-tree half of this module — the
27+
* forwarder, the carrier mint, the banner scrape and the empty-tree refusal —
28+
* and both consoles have now been extracted onto structured actions, so nothing
29+
* drives a Block Kit handler any more. They are DEAD, not live: read them as
30+
* history rather than as the way a console write works. ADR-0015 Decision 1 puts
31+
* their removal in the increment that follows this one ("once no caller remains,
32+
* the block-tree half of the console transport" goes), deliberately as its own
33+
* change, so this one stays a single thing. Everything else here — the two
34+
* interaction types, the refusal shape, the refusal constants and
35+
* {@link readConsolePayload} — is live and stays.
36+
*
2437
* G5 APPLIES UNCHANGED, one tier up: every response is HTTP 200 with an outcome
2538
* in the body. A refusal is a value.
2639
*/

packages/plugin/src/admin/money-input.ts

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -12,8 +12,11 @@
1212
* place in this console where drift is acceptable.
1313
*
1414
* `packages/plugin/src/admin/money-input.js` stays as this re-export because it
15-
* is the path `orders-page.ts` and `products-page.ts` already import, and
16-
* `@otta-sh/plugin`'s public `index.ts` re-exports both functions from it.
15+
* is the path several modules already import — `shipping-page.ts`,
16+
* `coupons-page.ts` and `products-actions.ts` — and `@otta-sh/plugin`'s public
17+
* `index.ts` re-exports both functions from it. (The two importers this note
18+
* used to name, `orders-page.ts` and `products-page.ts`, were the retired Block
19+
* Kit screens and are gone; the shim outlived them.)
1720
*
1821
* ONE IDIOM, STATED ONCE (INC-20 review). A module in `src/` that needs a
1922
* shared primitive imports `@otta-sh/admin-presentation` DIRECTLY. This file

packages/plugin/src/admin/percent-input.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -34,7 +34,7 @@ export function parsePercentToBps(input: string): number | null {
3434
/**
3535
* Format integer basis points back to a hundredths-of-a-percent decimal
3636
* string (WITHOUT a trailing "%" — callers add it, mirroring
37-
* `formatPriceMinorUnits` not carrying a currency symbol) for a text input's
37+
* `formatMinorUnitsInput` not carrying a currency symbol) for a text input's
3838
* initial value — pure integer math, no float division. Exported for its own
3939
* unit test.
4040
*/

0 commit comments

Comments
 (0)