docs(giellie): the stock quantity follow-ups left open
Thirty-three findings were triaged and deferred during the build, plus the ones the whole-branch review raised. The task ledger was scratch and gets deleted, so this is the record - and the list is worth having: the bridge guard that rejects pasted lines containing '@collection.', the skill file's family tables covering 68 of 148 tools, and the reorder points that gate the deferred dashboard view.
This commit is contained in:
parent
7bc2ea55a5
commit
867bf3e4cb
1 changed files with 107 additions and 0 deletions
|
|
@ -0,0 +1,107 @@
|
|||
# Giellie stock quantity update — follow-ups
|
||||
|
||||
Follow-ups left open by the `stock_items.set_quantities` work (merged to main as
|
||||
release 7). Each one was triaged during the build and judged safe to defer;
|
||||
this file is the record, because the task ledger was scratch.
|
||||
|
||||
The feature itself is complete. Nothing here blocks the release.
|
||||
|
||||
## Closed during the build
|
||||
|
||||
These came up in review and were fixed before the merge:
|
||||
|
||||
- The dry-run preview labelled rows `applied` before anything was written — the
|
||||
operator could read the plan as the outcome and never say "apply", so the
|
||||
quantities would silently never be written. Fixed in the agent prompt and the
|
||||
tool description.
|
||||
- The approval card crashed on a malformed line (a `null` element), taking the
|
||||
Approve/Reject buttons with it. The card is fed raw model input; the route's
|
||||
guard is in the execute branch and never protected it.
|
||||
- A catalogue-mirror error aborted the whole batch after the quantities had
|
||||
already been written — a retry then double-counted them.
|
||||
- A failed quantity write left an orphan ledger row, and the message told the
|
||||
operator to retry. The write is now compensated by a reversing row, so
|
||||
`SUM(change_qty)` still reconstructs `current_qty`.
|
||||
- `stock_items.adjust` wrote ledger rows with `created_by: ""` and no
|
||||
compensation. It now requires an actor and routes through the audited service.
|
||||
- The `stock_items.create` opening-balance row was unattributed. It is now
|
||||
stamped and reversed on failure.
|
||||
- The two source-of-truth docs (`docs/api-mcp-layer.md`,
|
||||
`docs/AGENT_INSTRUCTIONS.md`) still routed quantity writes to
|
||||
`stock_items.adjust` and said 144 tools. Regenerated and repointed.
|
||||
- The till tile displaced the pre-existing `in cart` count, and a long
|
||||
catalogue SKU clipped the quantity itself. The quantity now sits under the
|
||||
price, on the right.
|
||||
|
||||
## Open
|
||||
|
||||
- allowNegativeQty also drives the ledger-note default, coupling permission and provenance. Ruling: ACCEPTED. /stock never opts in so it always reads "from /stock"; the agent path passes an explicit noteContext anyway (Task 3). Documented in the input type's doc comment. A future non-Giellie caller must pass noteContext explicitly — noted for the final review.
|
||||
- the implementer report overstates equivalence with the brief's literal snippet for the contradictory input (allowNegativeQty=true with no noteContext). Code behaviour is right; the report wording is not.
|
||||
- SAFETY-CONSTRAINT TEST GAP — targetQty is not pinned against a negative under allowNegativeQty, and the negative-path test does not assert the persisted stock_items patch carries current_qty: -3. Both behaviours are correct in code (service:185-190, 233-238). Cheap to close — final review should triage this first.
|
||||
- the reviewer's spec-level point that the original worked example was unreachable. SUPERSEDED by the operator decision — the example now resolves, and the spec transcript was corrected.
|
||||
- the fix report says "the five new tests" but the file rises 9 -> 13 (four new cases, one extended). Report wording only; all five required scenarios are covered.
|
||||
- the text step can now report a line unmatched because its best match is inactive, where it previously said "no active stock item". Task 3's summary wording must reflect that (counts unchanged).
|
||||
- the compensating ledger row assumes a failed stock_items.update means the write did not land. If the response was lost after PB committed, the compensation cancels a change that did land, and the message's "retrying is safe" certainty exceeds what the code knows. Inherent to the mandated fix.
|
||||
- price_list_rows_touched reports 0 when the Nth catalogue update of a SKU fails, even though earlier rows of that SKU were already written. The count is conservative in the partial-update case.
|
||||
- the MCP stock_items.adjust tool has the same orphan-ledger shape — it writes the ledger row first then updates stock_items, with no compensation — and it stamps created_by: "". It bypasses updateStockItemsQuantities so this fix does not cover it. The skill file documents it; the final review should decide whether to route adjust through the service.
|
||||
- a failed write now leaves two ledger rows for one attempted change. Consumers that count ledger rows per adjustment will see two. The reversal note makes it explicit.
|
||||
- the actor-rejection comment is copied from set_pricing and says the ledger write fails AFTER the quantity changed. For this tool the ledger is written first, so a bad actor id aborts BEFORE any quantity change. Comment-only; the direction of the risk is favourable.
|
||||
- a non-finite number (JSON 1e999 parses to Infinity) is rejected but told it "must be a string or a number" while it is a number. Rejected either way; only the message is confusing.
|
||||
- actor is now advertised in the model-facing schema, so the model may populate actor.id itself. The bridge's argument guard validates any model-supplied id against /^[A-Za-z0-9_-]{1,64}$/, so a model-supplied id with punctuation rejects the whole call even though the bridge would have overwritten it. stock_items.set_pricing carries the identical exposure today, so this is a pattern risk newly applicable to this tool, not a regression in the guard.
|
||||
- the JSON-RPC error path drops the request id on every error — pre-existing and shared by every tool in the file.
|
||||
- the duplicate check uses price-list-match's normaliseSku while the resolver uses stock-match's. Verified immaterial: the price-list strip set is a superset, so the check can only over-reject, and zero of 300 stock_items SKUs and 340 catalogue SKUs contain a character the two treat differently.
|
||||
- the prompt sentence "price_list.add_items writes only the supplier catalogue (price_list.stock_qty)" can be misread as "writes only the stock_qty field" — it also writes cost, selling price, unit, category, brand and markup. Bounded; the pricing bullet above already establishes the tool's role.
|
||||
- the stock_items.set_quantities TOOL DESCRIPTION in the MCP route still does not explain that a row reported as "applied" on a dry run means "would be applied". The prompt now covers the rendering, but a reader of the description alone still gets the old ambiguity. Sentence supplied by the implementer; to be folded into the next task that touches the route.
|
||||
- the prompt nowhere states that this workflow itself keeps the price_list stock_qty snapshot in sync — the mirror is in the engine.
|
||||
- the bridge argument guard rejects any string containing "@collection." / "@request.", so a pasted line description containing that text fails the whole set_quantities call rather than reporting that line as unmatched. Pre-existing and shared by every tool, but this tool is the one most likely to receive arbitrary pasted text. FOR THE FINAL REVIEW.
|
||||
- the card never shows dry_run. A payload carrying dry_run true would render the normal table plus a confident "Setting these as the new quantity on hand" note, yet the route writes nothing on a dry run — an operator would believe the stock was updated. UNREACHABLE through the agent path (the policy classifies a dry run as a read, so it runs immediately and never reaches the card), but a latent hazard if that classification ever changes.
|
||||
- an invalid or missing mode is not flagged on the card — it falls through to the "set" wording, while the route rejects anything but set/add. Also unreachable in practice (the schema restricts the enum).
|
||||
- there is still no error boundary in the (app) segment. The fix removes the known crash vector for this tool, but any future renderer throw in the card still takes the approval UI down.
|
||||
- a doc comment overstates — the card's rules mirror the route's PER-LINE rules, not the batch rules (duplicate-SKU rejection, qty truncation), so the card can display a batch the route will reject wholesale.
|
||||
- the fix relies on position sticky on thead while Tailwind v4 preflight sets border-collapse collapse. Chrome pins correctly (verified with a headless geometry probe); Firefox has a long-standing sticky-with-collapsed-borders bug and was not available to test.
|
||||
- the new labelled trigger is size="sm" (32px) while every other composer control is 40px — a sub-44px tap target next to 40px neighbours on a phone-shaped composer. Follows the mandated size.
|
||||
- STOCK_PANEL_ID is a module-level constant, so a second Composer instance would emit a duplicate DOM id and aria-controls would resolve to the wrong panel. Latent — exactly one render site today.
|
||||
- buildStockMessage is exported from a "use client" module. Fine today; a future server-side consumer would get an RSC client reference instead of the function.
|
||||
- the always-visible trigger row makes the composer ~32px taller. Inherent to a visible label.
|
||||
- the stock textarea keeps the default resize handle while the main textarea sets resize-none; and the panel helper paragraph and the Send to Giellie button still share one justify-between row that can crowd at a 320px viewport.
|
||||
- the price block widens when the quantity string is wider than the price string, stealing ~9px from the .sub line's headroom for the in-cart count. No measured combination flips visible to clipped, so it is a narrowing margin rather than a regression - and it is inherent to the operator's chosen layout.
|
||||
- the new assertions are literal source-text regexes, so a semantically equivalent rewrite (renaming stockQty, or splitting the Number.isFinite check) would fail them. Brittle, but it is the price of the behavioural pinning the review demanded.
|
||||
- public/pos/index.html:155 reads caches.open('pos-v1') for the cache-state pill, but the shell cache is pos-v39 - nothing writes pos-v1, so the indicator reads cold on a warm till. Pre-existing.
|
||||
- the SW cache test only asserts the version is >= 33, so it would not have caught a missing bump. The bump was made on judgement, not by a guard. Pre-existing test weakness.
|
||||
## Worth acting on first
|
||||
|
||||
- **The bridge argument guard rejects any pasted line containing
|
||||
`@collection.` or `@request.`.** It is pre-existing and tool-wide, but this is
|
||||
the tool most likely to receive arbitrary pasted text. A pasted description
|
||||
containing that text fails the whole call rather than reporting the line as
|
||||
unmatched. Fail-safe, but worth widening.
|
||||
- **The tool-families tables in `.agents/skills/wfdash-giellie/SKILL.md`
|
||||
document 68 of the 148 tools.** The full index is authoritative, but the
|
||||
families tables are what an agent reads first.
|
||||
- **The POS cache-state pill always reads `cold`.** `public/pos/index.html`
|
||||
opens `pos-v1` while the shell cache is `pos-v39`, so nothing ever writes the
|
||||
cache it looks for.
|
||||
- **`stock_items.adjust`'s response shape changed** to
|
||||
`{ok, item, previous, ledger}`, with a reconstructed `ledger` that has no
|
||||
`notes`, `created_by` or `reference_id`. No in-repo consumer, but an external
|
||||
client reading `ledger.created_by` no longer gets it.
|
||||
- **`stock_items.adjust` on a missing id returns 500, not the documented 404.**
|
||||
The pre-read throws before the service's own not-found mapping can run. Pre-fix
|
||||
the same call also returned 500, so this is not a regression.
|
||||
- **`stock_items.create` can leave a half-created item.** The item is created at
|
||||
qty 0 and the balance is booked after; if the balance write fails the tool
|
||||
errors with the item already existing, so a naive retry creates a duplicate.
|
||||
- **The bridge can reject the whole call on a model-supplied actor id with
|
||||
punctuation.** The bridge overwrites the actor anyway, so it can only
|
||||
over-reject. `stock_items.set_pricing` carries the identical exposure.
|
||||
- **The POS tests are literal source-text regexes.** A semantically equivalent
|
||||
rewrite would fail them. That is the price of the behavioural pinning they
|
||||
replaced.
|
||||
|
||||
## Data work that gates the dashboard view
|
||||
|
||||
Only 48 of the 4,083 sellable items have a reorder point (`min_qty`) set, and
|
||||
522 carry a negative tracked quantity. The "what is low" view has almost
|
||||
nothing to compare against until thresholds are set on the items that matter.
|
||||
That is a data task, not a code one, and it is the prerequisite for the
|
||||
dashboard work the operator deferred to a later spec.
|
||||
Loading…
Reference in a new issue