feat(ai): widen the assistant to 18 curated tools, with an eval harness and shipped-bug fixes - #378
Merged
Merged
Conversation
Wraps spoolman.database.stats.usage_stats so the chat assistant can answer "what did I use last month" / "how fast am I going through filament" / spend questions from the real usage-event log. Registers as an always-on read tool; parse_bucket/parse_date are exported for Task 11 (orders.py) to reuse. Updates the four existing tests that hardcoded the exact set of read-tool names offered to a principal, since get_usage_stats now joins that set.
…date parse_date previously stripped tzinfo without converting first, so an offset-aware from_date/to_date (e.g. -05:00) had its wall-clock value treated as if already UTC, silently shifting the usage-stats query window by the offset. SpoolUsageEvent.time is naive-UTC, so the fix converts via astimezone(timezone.utc) before dropping tzinfo; naive input is untouched. Found in review of the get_usage_stats tool (Task 3); the brief's own code had this defect, and the plan has been corrected to match this fix.
Adds spoolman.database.location.get_weight_aggregates (one new grouped query, matched by name like the existing get_aggregates since locations are a name registry with no spool FK) and the find_locations read tool that pairs it with spool_count so the assistant can answer "where do I keep things" and "which shelf has the most left". The remaining-weight clamp uses sqlalchemy.case rather than two-argument func.max: PostgreSQL's MAX is aggregate-only, so the two-arg form would pass on SQLite/MySQL locally and fail on the full DB-matrix integration CI. Updates the three existing tests that hardcode the exact set of read-tool names offered to a principal, since find_locations now joins that set.
location_db.find applies no default ORDER BY, so its limited page was in arbitrary order; sorting that already-truncated page could silently drop the true highest-occupancy location once matches exceeded limit -- directly undermining the tool's advertised "which shelf has the most left" use case. Fetch the (small, bounded) location registry unlimited, rank by remaining weight, then cap -- the same shape find_filaments already uses for low_stock_only. Covering test inserts the heaviest location last (so a naive first-N page would miss it), calls the tool with a small explicit limit, and asserts it still ranks first. Verified the test fails against the prior truncate-first behavior (returned "B" instead of "E") before confirming it passes against the fix.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…rectness Adds test_find_vendors_ranks_by_spool_count_before_truncating_and_fields_not_swapped to verify both regressions are caught: - Truncating before sorting would miss the highest-spool-count vendor - Swapping filament_count and spool_count would return wrong field values The test seeds distinct, non-overlapping counts (1-5 filaments, 10-50 spools) with the highest-spool vendor inserted last, uses limit=2, and asserts both that the first returned vendor is correct and its field values are not swapped. Discrimination verified: - Swapping counts[0]/counts[1] fails with spool_count=5 instead of 50 - Truncating before sorting fails with vendor 'B' instead of 'E' first Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Both are WriteTools with preview/execute and a curated undo descriptor back to a new hidden delete_location/delete_vendor pair (model_facing=False, same precedent as set_spool_used_weight). create_vendor refuses a case-insensitive duplicate via the new resolve_vendor_by_name(ctx, name), which Task 8's create_filament will reuse to decide whether a confirm-card must disclose that it also creates a vendor.
- _execute_create_vendor now re-checks resolve_vendor_by_name itself: the chat-action endpoint calls .execute() directly without ever previewing, so the "duplicate is refused" promise must hold there too, not just in preview. Extracted _require_unique_vendor_name so both call the same check. - Both delete executes now resolve the row via a new _get_location/ _get_vendor helper (mirroring their preview siblings and the get_spool convention) before deleting, so a double-undo or a row removed elsewhere surfaces as a model-facing ToolError instead of an uncaught ItemNotFoundError that aichat.py's generic handler swallows into "That tool failed unexpectedly". - Parametrized the read-only-refusal test over all four inventory write tools (previously only create_location was covered), and added explicit execute-level duplicate and double-undo regression tests.
Lands before create_filament so the model can pull real density/diameter from
the public SpoolmanDB catalog instead of guessing them - a wrong density would
silently corrupt every future weight calculation for that filament.
Reuses spoolintake.load_catalog()/score_candidate() (the same functions
Scan-to-Spool's catalog stage uses) rather than opening a second path to the
data, and runs the difflib scoring off the event loop via asyncio.to_thread,
matching spoolintake.build_matches. A missing/unsynced catalog degrades to
{"count": 0, "matches": []} rather than raising, same as Scan-to-Spool treats it.
Updates the four exact-set read-tool-name assertions (tests/test_ai_tools.py,
tests/integration/test_ai_chat_endpoints.py, tests/integration/test_mcp_endpoints.py)
to include catalog_lookup.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…sure density and diameter are required and never defaulted -- exactly the fields a model would confidently fabricate, and a wrong density silently corrupts every future weight calculation for that filament. vendor_name resolves case-insensitively against existing vendors (Task 6); when none matches, the confirm-card discloses the vendor it will also create before the user approves. Moves find_filaments out of spools.py into a new filaments.py alongside create_filament, completing the split Task 1 deferred. Updates the writer exact-set tool-name test.
…create fails vendor_db.create commits immediately and durably. If the following filament_db.create then raises anything -- a raw IntegrityError included, since filament.create has no try/except of its own around its commit -- the auto-created vendor previously survived as an orphan while the tool reported failure: the same silent vendor creation this tool exists to prevent, reached through the error path. _execute_create_filament now catches broadly (not just the two named exceptions, one of which -- ItemCreateError -- filament.create never actually raises) and, only when this same call created the vendor, rolls back and deletes it before re-raising as ToolError. A pre-existing vendor is never touched, and a failure during cleanup itself is logged rather than masking the original error.
…ields curated_fields and _requested_changes treated an explicit None the same as an absent key, so an undo descriptor carrying a field's prior None value was either silently dropped (other fields still changed) or looked like an empty change set (if it was the only field), making the undo a no-op or a ToolError instead of a genuine revert. Both now key change-detection on presence in args, not on truthiness of the value, so a key that's absent still leaves the column alone while an explicit None clears it.
…ng cascade The confirm-card states the exact spool count (archived included) and marks destructive=True with undo=None; the preview refuses up front when an order line references the filament, since filament_db.delete would otherwise raise ItemDeleteError after the user already confirmed. Also fixes a pre-existing bug the tests exposed: filament_db.delete failed for any filament with spools at all (Spool.filament_id is NOT NULL and the ORM relationship has no delete cascade, so the ORM tried to null the FK on flush and hit a NOT NULL violation). delete now explicitly cascades to the filament's spools and their usage events first, matching spool_db.delete's existing pattern -- otherwise the card's "this deletes N spools" promise would fail on confirmation for the common case. count_order_line_references and count_spools are extracted into spoolman/database/filament.py as the single definitions of those checks.
filament_db.delete now cascades to a filament's spools and their usage
history unconditionally (previous commit), which is correct for the chat
tool -- it already showed a confirm-card and got the user's agreement --
but made the plain REST endpoint silently destructive with zero warning,
where it previously failed safely.
DELETE /filament/{id} now refuses with 409 and states the exact spool
count, that usage history goes too, and that it cannot be undone, unless
the caller passes cascade=true. A filament with no spools deletes exactly
as before: no new required parameter, no behaviour change for existing
callers. The order-line refusal remains a hard refusal that cascade can
never override, and is now double-enforced (endpoint + database layer).
…nt delete filament_db.delete cascades to a filament's spools but only emitted filament_changed(DELETED) -- every other spool-destroying path (spool.delete) notifies per-spool too, so a live client kept showing rows for spools that no longer exist. Now emits spool_changed(spool, DELETED) for each cascaded spool after commit, eager-loading spool.filament/.printer (needed by Spool.from_db) since the cascade's own select() doesn't load them by default. Also: the cascade's blast-radius containment (the WHERE clause scoping deletion to exactly one filament_id) had no test that could fail for it, since every existing test created only one filament per database. Added a second, untouched filament/spool/usage-event to the cascade test and asserted they survive. And added delete_filament -- the most destructive tool in the registry -- to the parametrized read-only guard that exists precisely to catch a missing require_write().
Reads answer "what is on order" / "did my order arrive": status (open/arrived) is derived from each line's arrived_at rather than stored, and orders are filtered by shop (not vendor, which orders have no relationship to). Status and date filtering happen over an unlimited fetch, sorted, then capped, so "what's still open" can't silently drop rows a DB-level LIMIT would cut before ranking -- the same bug shipped and fixed for find_locations/find_vendors. Adds two integration tests (ranking-before-truncation, and shop+status filtering) beyond the task brief's three unit tests, since none of those exercise _run_find_orders itself and so could not have caught the DB-limit regression.
Fills the write half of spoolman/ai_tools/orders.py: create_order (model-facing)
records a filament order from a list of {filament_id, quantity, price_per_unit}
lines, and delete_order (model_facing=False) exists solely as its undo primitive.
parse_lines coerces the model-emitted lines shape and raises a specific,
actionable ToolError for every malformed case, since a vague error costs the
model its whole turn. Updates the exact-set tool-name test in test_ai_tools.py
and adds create_order to the read-only require_write refusal guard.
…er's double-undo Two review findings on create_order/delete_order, both inherited from the task brief's sample code: - parse_lines lost the line index whenever arg_int (shared, list-unaware) raised for a bad filament_id or quantity, so a mid-list error gave a model no way to tell which of several lines to fix. Wrap the per-entry coercions and re-raise with the line index prefixed, preserving the original message. - _execute_delete_order deleted straight through order_db.delete without first converting ItemNotFoundError to ToolError, unlike every sibling delete tool (_execute_delete_location/_execute_delete_vendor). A double-undo therefore leaked a raw ItemNotFoundError past the chat_action endpoint's except ToolError contract. Fetch through get_by_id first, matching the siblings. Also adds delete_order to the parametrized read-only require_write guard (create_order was already covered).
My order arrived turns one sentence into N spools created in the right location, carrying the line price. It's the only non-delete write with no undo: order.arrive both splits lines and creates spools in one call, so the confirm-card carries the full disclosure (count, location, "cannot be undone in one click") and preview fails before the user ever sees it for anything that would make execute fail (missing order, nothing outstanding, unknown location). Also raises the pre-existing MAX_SCHEMA_CHARS budget ceiling from 12,000 to 13,000: the completed 18-tool set's verbatim schema lands at 12,057 chars, over the old ceiling's stale 9 KB estimate. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Review of task 13 found four gaps, all about the confirm-card carrying
the whole weight of an irreversible write:
- arrive_order's card never set destructive=True, so the one irreversible
non-delete write in the tool set rendered as an ordinary, safe-looking
confirmation instead of the red "cannot be undone" styling every other
no-undo write gets.
- execute() didn't pre-check the order exists or has anything outstanding,
unlike its own preview and every other execute in the module: a bad
order_id would 500 via a raw ItemNotFoundError, and re-arriving an
already-arrived order was a silent no-op reported as a success.
- The card echoed the user's raw location string ("shelf b") instead of
the resolved canonical name ("Shelf B") the spools actually land in.
- MAX_SCHEMA_CHARS's raise-and-comment from task 13 recorded the wrong
cause (blamed arrive_order for undercounting the estimate, when the
17-tool set was already 2.4 KB over the old guess); corrected the
comment and tightened the ceiling to 12,500 now that the set is frozen.
Also adds test_every_write_tool_without_an_undo_is_marked_destructive,
deriving the no-undo set from the registry via an AST walk rather than a
hardcoded tool list or a text search -- a first draft using a substring
search on 'destructive=True' passed even with the flag removed, because
the surrounding comment happened to contain that exact text.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The system prompt now tells a writer principal to call catalog_lookup instead of inventing filament density/diameter, prefer arrive_order over hand-built spools, and check find_vendors/find_locations before creating a duplicate. The read-only branch is untouched, so a read-only principal still hears nothing about write tools it cannot see. update_spool and update_filament's descriptions now spell out that an omitted field is left alone but an explicit null clears it -- task 9's presence-based change detection made that distinction load-bearing, and a model emitting null as a lazy "nothing to set" placeholder would otherwise silently wipe a column. _MCP_WRITE_TOOLS widens from 3 to 9 non-destructive writes so external MCP clients get the same curated reach as the in-app chat agent; deletes stay excluded since an MCP client's user never sees Spoolman's confirm-cards.
…-clears claim _list_tools hard-coded destructiveHint=False for every write tool it exposed, so arrive_order -- the one non-delete write with no undo -- was reported to an MCP client as safe even though it can't be confirmed away from a client-side card. WriteTool gains a declared destructive field (set on every delete plus arrive_order, matching what each preview's ConfirmCard already sets), and _list_tools now passes each tool's own value through instead of a hard-coded default. A new test pins the field and the card in agreement for every write tool, in both directions. update_filament's null-clears description was wrong for density/diameter: they route through arg_float directly and raise rather than clear on an explicit null, unlike every other curated field. Reworded to state that exception, and added a test that pins the actual behavior.
poe ai-eval sends 53 fixture prompts through the real 18-tool schema (~12.2 KB serialised) to a live OpenAI-compatible endpoint and checks which tool the model reached for, with a loose per-argument check and a confusion table. Not run in CI -- needs a live model. Measured baseline against a local Ollama 7.6B tool-tuned Qwen2.5-Coder (hhao/qwen2.5-coder-tools): 47/53 (89%) tool selection, 46/53 (87%) arguments, a repeat run measuring 45/53 (85%)/44/53 (83%). Dominant failure mode is declining to call a tool at all, not miscalling one; the catalog_lookup/find_filaments confusion the plan predicted as the weak point did not materialise on this model. Fixed three crash bugs in the brief's runner before trusting any of its output: _args_match blew up (TypeError/ValueError) on a list/dict-valued expectation (create_order's lines) or a non-numeric model response; malformed tool-call JSON propagated instead of scoring as a miss; a transient AIRequestError aborted the whole run instead of one case. All three are exactly the failure modes a small local model produces, so leaving them in would have made the harness crash because it was working, not because it was broken.
…harness bool(got) is want treated any non-empty string as truthy regardless of content, so a garbage value like "nope" scored as a correct match against want=True, and the canonical "false" scored as wrong against want=False. Add _coerce_bool, mirroring ai_tools.base.arg_bool's exact accepted strings (true/yes/1, false/no/0/""); anything else now matches neither True nor False, so it's always scored as a mismatch instead of an accidental pass. Re-ran the live eval after the fix: 49/53 (92%) tool selection, 46/53 (87%) arguments -- this supersedes the two pre-fix runs in the task report. Confirmed directly (not assumed) that the args figure is unchanged here because this model/template happens to always emit native JSON booleans rather than stringified ones through the OpenAI-compatible endpoint, so the buggy comparison was never actually triggered by these three live runs; the fix still matters for other OpenAI-compatible tool-calling templates known to stringify booleans.
…iews Independent defects surfaced by this plan's reviews but deliberately deferred so they would not expand a task mid-flight (Task 17 brief). Traced each with git log -S to confirm origin rather than assume: Pre-existing (predate this plan; shipped with the original #362 chat feature on 2026-07-24/25, before this plan grew the tool layer from 6 to 18 tools): - utc_timezone_naive (database/utils.py) called astimezone() on naive input, which Python treats as system-local time -- silently shifting stored values on any non-UTC host. Naive means UTC everywhere in this codebase; fixed to match stats.parse_date's existing shape (return naive input unchanged, convert offset-aware input to UTC first). - _resolve_pending (aichat.py) ran every pending write on one shared session with no rollback between them, so a commit-level failure left the session needing rollback and poisoned the next unrelated pending write with the generic "failed unexpectedly" message. Fixed once in the loop. - /ai/chat/action's docstring claimed it "grants no capability beyond chat itself", but it resolves any WRITE_TOOLS name including model_facing=False undo-only primitives chat never offers. Reworded to the real contract. - Dead `logger` in ai_tools/base.py, a verbatim carry-over from the pre-split monolith where it was also unused. Removed after confirming no importers. Plan-introduced (this plan's own tasks, 2026-07-28/29): - catalog_lookup's limit description claimed "default 25, capped at 10" when the effective default is 10 (25 is immediately capped). Reworded. - filament.py's delete cascade built spool_id.in_(<python list>), one bound SQL parameter per spool -- older SQLite builds cap around 999. Replaced with a correlated subquery, same statement count, no limit. - catalog._rank passed a catalog entry's raw weight to the shared scorer uncoerced, unlike the sibling spoolintake.match_catalog. Promoted spoolintake's private _coerce_number to a public coerce_number (also used by normalize_extraction) so both callers share the identical coercion -- this also touches spoolintake.py, outside the brief's listed files, since that module backs the unrelated Scan-to-Spool feature. Each fix has a regression test verified to fail when the bug is reintroduced (see task-17-report.md, gitignored under .superpowers/sdd/). Full suite: 1041 passed (was 1031 at base), the exact delta being this task's 10 new tests -- no pre-existing test's expectations changed for any of the seven. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Task 18: five tests that passed without being able to catch the bug they claimed to guard, plus untested behaviour this plan relies on. - The AST destructive-flag guard (test_ai_tool_budget.py) collapsed "found the call and it disagrees" and "couldn't statically determine anything" into the same False. A future tool building its card/result through a helper, an aliased import, or a computed value would silently stop being checked while the test kept passing. Reworked the AST walk to return a tri-state (True/False/None-undeterminable) and made undeterminable fail the test by tool name instead of folding into a quiet "no". - MCP's writer-set assertion (test_mcp_endpoints.py) used <=, so the allowlist could gain tools without ever failing. Switched to exact set-equality; no discrepancy found against the real registry. - _run_get_usage_stats had no direct coverage for MAX_BUCKETS truncation or an empty result set. Added both, pinning that truncation keeps the most recent buckets, not the oldest. - get_weight_aggregates's archived-exclusion, null-initial_weight fallback, zero-spool default, and non-negative clamp were all implemented correctly but unguarded. Added one test per behaviour against a real session. - delete_filament's preview before payload (the blast-radius disclosure) was never asserted. Extended the existing archived-spool-count test to check it. Every gap was verified by injecting the bug it guards against, confirming the new test fails, then reverting -- see task-18-report.md for the full log. Test-only change; no production code touched. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
docs/ai.md still described the 6-tool chat assistant from #362; it now covers the full 18-tool curated layer (spools, filaments, usage stats and spend, orders and arrival, locations, vendors, catalog lookup), the null-clears-a-field/omit-means-unchanged update contract, arrive_order's one no-undo write, and the widened MCP write set with arrive_order annotated destructive. Adds a maintainers section documenting `poe ai-eval` and its measured baseline (49/53 tool selection, 46/53 with correct arguments, one local 7.6B model), cross-linked from CONTRIBUTING.md's local-only-extras list. Also documents the DELETE /filament/{id} cascade+409 safety gate in CHANGELOG.md: the endpoint previously failed outright on any filament with spools; it now cascades behind an explicit cascade=true.
… sentence The null-clears-a-field paragraph in docs/ai.md stated the update contract without its one exception: density and diameter route through arg_float, which rejects an explicit null with a ToolError instead of clearing the column (spoolman/ai_tools/filaments.py:159-162, arg_float at spoolman/ai_tools/base.py:66-76) -- unlike every other filament/spool field, where an explicit null clears and an omitted key leaves it alone. The tool schema already carves this out for the model; the doc now states it in the same sentence as the rule instead of two sentences later, so a user acting on the documented rule doesn't hit a contradicting error.
DELETE /filament/{id} refuses with 409 unless cascade=true is passed once the
filament still has spools, and the web client had no way to send that param --
deletion was silently broken for any such filament, and Refine's default
DeleteButton only offered a bare "Are you sure?".
Click Delete now sends the plain request as before; on 409 the client reads
the server's own spool count and opens an expanded dialog naming the filament,
the spool count, that usage history goes too, and that it cannot be undone --
confirming re-sends with cascade=true. A filament with no spools keeps the
existing single-request popconfirm.
The 409 body carries the count as a proper field (FilamentCascadeRequired,
spool_count: int) rather than folding it into the message the client would
otherwise have had to regex out of prose -- reword the sentence and the old
approach would have silently lost the dialog's count with nothing failing.
message stays exactly as it was for any other consumer already depending on
it. A 409 without the field falls through to the ordinary error notification
instead of guessing a number.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…indings
A final whole-branch review of the 32-commit chat-tool-layer branch found one
critical and eight important issues that per-task reviews could not see:
- C1 (critical): create_filament's undo descriptor named delete_filament, and
/ai/chat/action called any WRITE_TOOLS entry's execute() directly with no
preview -- so clicking Undo on a creation card after adding spools to it
silently cascaded through the filament, its spools, and their usage history
with no count and no confirmation. Fixed by giving the undo descriptor an
only_if_empty flag (never in the model-facing schema) that delete_filament's
execute refuses on when spools exist, and by restricting /ai/chat/action to
an allowlist of exactly the tool names any undo descriptor can emit.
- I2: the cascade's per-spool DELETED websocket notification had zero test
coverage; added one that also proves the joinedload("*") it depends on is
load-bearing, not gratuitous.
- I3: the MCP destructive-tool test was a `startswith("delete_")` heuristic
that arrive_order (destructive, non-delete) sailed straight through;
replaced with a check against each tool's own declared destructive flag.
- I4: the read-only require_write guard hand-enumerated 8 of 15 write tools,
missing create_filament, update_filament, and five spool writes added on
this branch; derived the parametrization from the full registry instead.
- I5: create_order's preview didn't resolve the shop or parse the date execute
needs, so a bad value surfaced only after the user confirmed; both now
validate in preview and the card shows the resolved shop and date.
- I6: the tool-selection eval sent a one-line system prompt instead of
production's; rebuilt it from aichat._system_prompt, fixed two create_order
fixtures that were vacuously "passing" with no expected args, and re-ran it
twice against a live endpoint (47-48/53 tool selection, 44-46/53 with
correct arguments) rather than leaving the old single best-of-three figure
in docs.
- I7: _read_summary rendered five of seven read tools as a bare "Done." in the
chat drawer's transparency line; every read tool now gets its own summary.
- I8: a client test asserted `toMatchObject({ params: {} })`, which matches
any params object regardless of content; tightened to an exact toEqual.
- I9: FilamentDeleteButton's errorNotification let non-suppressed failures
fall through to Refine's default notification, whose description field
notificationProvider never renders -- the real server message (order-line
refusals, failed cascade retries) was silently dropped. It now builds an
explicit message from the error body, with a new translated string in all
31 locales.
Every added/changed test had its guarded mutation injected and confirmed to
fail before being reverted; full details in the (untracked) fix report.
uv run pytest -q tests/: 1066 passed. ruff clean. client: eslint, tsc,
check-i18n, and vitest (774 tests) all clean.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…view
docs/ai.md's Undo paragraph said "two exceptions" but already enumerated
three (deletes, arrive_order, and the refusal to undo a filament creation
once spools were added) — fix the stale count.
filamentDeleteButton.tsx's errorNotification unconditionally interpolated
error?.message into the toast; an error body with no `message` key (e.g.
an auth-layer HTTPException's FastAPI {"detail": ...} shape) left a
dangling, broken-looking string. Fall back to the existing plain
"deleteError" text when the message is missing or empty.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…cards
_preview_create_order and _preview_delete_order put a raw ISO-8601 datetime
(e.g. "2026-01-15T00:00:00") on the confirm-card a person reads before
clicking confirm. Add a shared _format_order_date helper: midnight collapses
to a bare date, a real time component is kept but readable ("2026-01-15
14:30"). Display-only -- execute() still passes parse_date's own datetime to
order_db.create, and order_row's raw ISO string (used by find_orders' sort)
is untouched.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This was referenced Aug 6, 2026
…ght totals The home page crashed with "TypeError: e.toFixed is not a function" (#377), but the crash was the second symptom, not the bug: the totals were already wrong. json-bigint's storeAsString option gates on raw literal length (>15 characters), not on integer-ness, so an ordinary gram value carrying float64 noise — 0.30000000000000004 is 19 characters — parsed as a *string* despite the declared number type. The home page then does `sum + spoolStockWeight(s)`, and once one value is a string, `+` becomes concatenation: every later spool's weight is appended as digits. That concatenation, not any stored value, is where the reporter's 55-digit number came from. Sub-1000-gram values then reached .toFixed() as strings and took the render tree down. Parse with storeAsString: false so long literals arrive as BigNumber instances, which — unlike strings — can be told apart from genuine string fields, and revive each one individually: only whole numbers beyond the safe integer range become strings, preserving the CockroachDB id behaviour (#69) exactly. Also harden formatWeight/formatWeightCompact/formatLength with the Number() + Number.isFinite coercion numberFormatter in the same file already used, so no future bad value can crash a page; and round used_weight to 6 decimals on every write path. Six, not one: the goal is to strip float64 representation noise without discarding real sub-gram increments, since a slicer can legitimately report ~0.03 g per layer. The accumulator stays a single atomic UPDATE — rounding it in Python would reintroduce the read-before-write that deadlocks MariaDB and forces CockroachDB serialization retries — so it rounds in SQL, casting to Numeric first because PostgreSQL has no round(double precision, integer). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Expands Spoolman-NG’s curated AI tool layer (chat + MCP) from a small set to a broader, domain-split registry, while fixing several shipped issues (timezone normalization, safe filament deletion behavior, float/noisy-number handling) and adding an eval harness to measure tool-selection accuracy against live endpoints.
Changes:
- Refactors
spoolman/ai_toolsinto per-domain modules and grows the model-facing tool surface (reads + curated writes), with new invariants/budget tests and MCP exposure updates. - Fixes multiple correctness issues: naive datetime UTC coercion, filament delete cascade gating, weight rounding to avoid float noise, and safer client JSON parsing/formatting.
- Adds tooling/docs:
poe ai-evalharness + fixtures, plus updated documentation and UI deletion flows that reflect server-provided cascade blast radius.
Reviewed changes
Copilot reviewed 81 out of 81 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_database_utils.py | Adds regression tests for utc_timezone_naive behavior under non-UTC host TZ. |
| tests/test_ai.py | Adds system-prompt assertions related to catalog lookup / write vs read-only prompt content. |
| tests/test_ai_tools.py | Updates tool registry expectations and adds invariants around summaries, exports, and change semantics. |
| tests/test_ai_tools_stats.py | Adds unit tests for usage-stats tool argument parsing and aggregation behavior. |
| tests/test_ai_tools_orders.py | Adds unit tests for derived order state and order-line argument parsing. |
| tests/test_ai_tools_inventory.py | Adds unit tests for locations/vendors shaping and create validation. |
| tests/test_ai_tools_filaments.py | Adds unit tests for curated filament field rules (no guessing physics, null-clearing rules). |
| tests/test_ai_tools_catalog.py | Adds unit tests for catalog lookup tool wiring, limits, and scoring behavior. |
| tests/test_ai_tool_budget.py | Adds schema-budget + invariant guards (tool count, destructive/undo pairing, uniqueness). |
| tests/integration/test_mcp_endpoints.py | Expands MCP tool list, adds destructiveHint assertions, and enforces MCP write-set invariants. |
| tests/integration/test_filament_delete_cascade.py | Adds integration coverage for filament delete cascade gate + SQL shape + websocket fanout. |
| tests_integration/tests/spool/test_use.py | Adds integration test preventing float drift on many small use increments. |
| spoolman/spoolintake.py | Makes numeric coercion reusable/public for catalog scoring parity. |
| spoolman/mcp_server.py | Expands MCP curated write tools and propagates per-tool destructiveHint. |
| spoolman/database/utils.py | Fixes utc_timezone_naive so naive datetimes are treated as UTC (no host TZ leak). |
| spoolman/database/spool.py | Adds SQL/Python rounding to strip float noise; refactors used-weight derivation and update handling. |
| spoolman/database/location.py | Adds location remaining-weight aggregation with portable SQL clamping and no N+1 queries. |
| spoolman/database/filament.py | Implements filament delete cascade (spools + usage events) with safety/portability constraints and websocket notifications. |
| spoolman/api/v1/models.py | Adds structured 409 model (FilamentCascadeRequired) with spool_count. |
| spoolman/api/v1/filament.py | Adds cascade query param and 409 gating/structured response for filament deletion when spools exist. |
| spoolman/api/v1/ai.py | Adds undo-action allowlist to restrict /ai/chat/action to known undo-invoked tools. |
| spoolman/aichat.py | Improves system prompt guidance and ensures DB session rollbacks on tool execution failures. |
| spoolman/ai_tools/stats.py | Introduces get_usage_stats tool (bucket/date parsing + aggregation). |
| spoolman/ai_tools/spools.py | Implements spool read/write tools (find/update/consume/create/delete) with presence-based updates. |
| spoolman/ai_tools/orders.py | Implements order read/write tools (find/create/arrive + derived status shaping). |
| spoolman/ai_tools/inventory.py | Implements location/vendor read tools and curated create write tools (with duplicate checks). |
| spoolman/ai_tools/catalog.py | Implements SpoolmanDB catalog lookup tool using shared scorer off-thread. |
| spoolman/ai_tools/base.py | Introduces shared tool primitives (schemas, coercion helpers, confirm cards, execution results). |
| spoolman/ai_tools/init.py | Merges per-domain tool registries and re-exports the public tool-layer surface. |
| scripts/ai_eval.py | Adds live-endpoint eval harness for tool selection + argument correctness scoring. |
| scripts/ai_eval_cases.json | Adds fixture prompts/cases for eval harness. |
| pyproject.toml | Adds poe ai-eval task entry. |
| docs/ai.md | Updates AI documentation to reflect new tool reach, deletion semantics, and eval harness usage. |
| CONTRIBUTING.md | Documents running ai-eval locally. |
| client/src/utils/parsing.tsx | Adds defensive coercion + rounding for weights/lengths to avoid crashes on string/noisy inputs. |
| client/src/utils/parsing.test.ts | Adds tests for defensive coercion and noisy-float rounding behavior. |
| client/src/utils/bigintJson.ts | Reworks JSON parsing to avoid turning long noisy floats into strings while preserving big-int IDs. |
| client/src/utils/bigintJson.test.ts | Adds tests for noisy-float numeric preservation and big-int ID string retention. |
| client/src/pages/home/analytics.test.ts | Adds end-to-end test through real JSON deserialization for noisy-float weight summation. |
| client/src/pages/filaments/show.tsx | Replaces default delete button with cascade-aware filament delete flow. |
| client/src/pages/filaments/functions.ts | Adds shared filamentDisplayName helper for deletion UI. |
| client/src/pages/filaments/filamentDeleteButton.tsx | Implements two-step filament deletion with server-provided spool_count escalation and better error surfacing. |
| client/src/pages/filaments/filamentDeleteButton.test.tsx | Adds thorough component tests for cascade escalation, error handling, and notifications. |
| client/src/pages/filaments/edit.tsx | Adds cascade-aware delete affordance to edit view and disables default delete. |
| client/src/components/dataProvider.ts | Forwards meta.queryParams for delete requests (needed for cascade=true). |
| client/src/components/dataProvider.test.ts | Tests deleteOne query param forwarding behavior. |
| client/public/locales/zh/common.json | Adds cascade-delete strings and deleteErrorDetail notification string (zh). |
| client/public/locales/zh-Hant/common.json | Adds cascade-delete strings and deleteErrorDetail notification string (zh-Hant). |
| client/public/locales/uk/common.json | Adds cascade-delete strings and deleteErrorDetail notification string (uk). |
| client/public/locales/tr/common.json | Adds cascade-delete strings and deleteErrorDetail notification string (tr). |
| client/public/locales/th/common.json | Adds cascade-delete strings and deleteErrorDetail notification string (th). |
| client/public/locales/ta/common.json | Adds cascade-delete strings and deleteErrorDetail notification string (ta). |
| client/public/locales/sv/common.json | Adds cascade-delete strings and deleteErrorDetail notification string (sv). |
| client/public/locales/sl/common.json | Adds cascade-delete strings and deleteErrorDetail notification string (sl). |
| client/public/locales/ru/common.json | Adds cascade-delete strings and deleteErrorDetail notification string (ru). |
| client/public/locales/ro/common.json | Adds cascade-delete strings and deleteErrorDetail notification string (ro). |
| client/public/locales/pt/common.json | Adds cascade-delete strings and deleteErrorDetail notification string (pt). |
| client/public/locales/pt-BR/common.json | Adds cascade-delete strings and deleteErrorDetail notification string (pt-BR). |
| client/public/locales/pl/common.json | Adds cascade-delete strings and deleteErrorDetail notification string (pl). |
| client/public/locales/nl/common.json | Adds cascade-delete strings and deleteErrorDetail notification string (nl). |
| client/public/locales/nb-NO/common.json | Adds cascade-delete strings and deleteErrorDetail notification string (nb-NO). |
| client/public/locales/lt/common.json | Adds cascade-delete strings and deleteErrorDetail notification string (lt). |
| client/public/locales/ko/common.json | Adds cascade-delete strings and deleteErrorDetail notification string (ko). |
| client/public/locales/ja/common.json | Adds cascade-delete strings and deleteErrorDetail notification string (ja). |
| client/public/locales/it/common.json | Adds cascade-delete strings and deleteErrorDetail notification string (it). |
| client/public/locales/hu/common.json | Adds cascade-delete strings and deleteErrorDetail notification string (hu). |
| client/public/locales/hi-Latn/common.json | Adds cascade-delete strings and deleteErrorDetail notification string (hi-Latn). |
| client/public/locales/fr/common.json | Adds cascade-delete strings and deleteErrorDetail notification string (fr). |
| client/public/locales/fa/common.json | Adds cascade-delete strings and deleteErrorDetail notification string (fa). |
| client/public/locales/et/common.json | Adds cascade-delete strings and deleteErrorDetail notification string (et). |
| client/public/locales/es/common.json | Adds cascade-delete strings and deleteErrorDetail notification string (es). |
| client/public/locales/en/common.json | Adds cascade-delete strings and deleteErrorDetail notification string (en). |
| client/public/locales/en-GB/common.json | Adds cascade-delete strings and deleteErrorDetail notification string (en-GB). |
| client/public/locales/el/common.json | Adds cascade-delete strings and deleteErrorDetail notification string (el). |
| client/public/locales/de/common.json | Adds cascade-delete strings and deleteErrorDetail notification string (de). |
| client/public/locales/da/common.json | Adds cascade-delete strings and deleteErrorDetail notification string (da). |
| client/public/locales/cs/common.json | Adds cascade-delete strings and deleteErrorDetail notification string (cs). |
| CHANGELOG.md | Documents the filament delete cascade gate behavior change and compatibility details. |
Suppressed comments (1)
spoolman/database/spool.py:408
- update() rounds a directly-specified used_weight, but if a caller explicitly sends "used_weight": null (allowed by the API schema), round(None, ...) will raise TypeError and 500 the request. This should be rejected explicitly (or coerced) so clients get a predictable 4xx error response.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+24
to
+26
| original_tz = os.environ.get("TZ") | ||
| monkeypatch.setenv("TZ", "America/New_York") # UTC-4/UTC-5, unambiguous if this leaks through | ||
| time.tzset() |
Comment on lines
401
to
+405
| elif k == "remaining_weight": | ||
| if spool.initial_weight is None: | ||
| raise ItemCreateError("remaining_weight can only be used if initial_weight is set.") | ||
| spool.used_weight = max(spool.initial_weight - v, 0) | ||
| new_used_weight = _used_weight_from_remaining(spool.initial_weight, v) | ||
| # Rounded like every other used_weight write path (#377): the subtraction can carry | ||
| # the same float64 noise as the SQL accumulator in use_weight_safe does. | ||
| spool.used_weight = round(new_used_weight, WEIGHT_ROUND_DECIMALS) |
Comment on lines
+474
to
478
| if body.tool not in _CHAT_ACTION_ALLOWLIST: | ||
| raise HTTPException(status_code=400, detail=f"Unknown action '{body.tool}'.") | ||
| tool = ai_tools.WRITE_TOOLS.get(body.tool) | ||
| if tool is None: | ||
| raise HTTPException(status_code=400, detail=f"Unknown action '{body.tool}'.") |
_round6 cast its result back to Float "to match the used_weight column", but that cast never did anything on any supported backend. MySQL/MariaDB cannot CAST to FLOAT, so SQLAlchemy dropped it and warned; on SQLite and PostgreSQL the UPDATE ... SET assignment already coerces to the column type. Compiling every dialect with and without it yields byte-identical SQL, so its only observable effect was the warning. The inner cast to Numeric stays — that one is load-bearing, since PostgreSQL and CockroachDB have no round(double precision, integer) overload. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This was referenced Aug 8, 2026
sherrmann
added a commit
that referenced
this pull request
Aug 8, 2026
`test (cockroachdb)` began failing on PR #378 with the app never starting: the job died during `db Pulling`, before pytest ran. The same commit range passed the same job 24 minutes earlier, the other three database legs pass with identical application code, and the image pulls fine locally -- so the failure tracks the runner's Docker Hub quota, not the commit under test. Anonymous pulls are rate-limited per source IP and GitHub's hosted runners share theirs across the fleet, so any leg can start failing for reasons unrelated to the change being tested. Log in with the credentials the publish job already uses to get the authenticated quota. The step is skipped when the secret is unavailable (fork PRs), leaving those runs exactly as they are rather than failing on a missing secret. The token is hoisted to a job-level env var because the `secrets` context is not available in a step-level `if`. Also pull the database image in its own step. compose-action reports only "Docker Compose command failed with exit code 1" and echoes the pull progress lines, swallowing the message that says why -- diagnosing this failure meant reasoning from which legs passed, because the log never contained the error. A separate pull puts it in the log. sqlite has no `db` service, so the guard reads the compose file's service list rather than matching on the matrix name. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
sherrmann
added a commit
that referenced
this pull request
Aug 8, 2026
`test (cockroachdb)` began failing on PR #378 with the app never starting: the job died during `db Pulling`, before pytest ran. The same commit range passed the same job 24 minutes earlier, the other three database legs pass with identical application code, and the image pulls fine locally -- so the failure tracks the runner's Docker Hub quota, not the commit under test. Anonymous pulls are rate-limited per source IP and GitHub's hosted runners share theirs across the fleet, so any leg can start failing for reasons unrelated to the change being tested. Log in with the credentials the publish job already uses to get the authenticated quota. The step is skipped when the secret is unavailable (fork PRs), leaving those runs exactly as they are rather than failing on a missing secret. The token is hoisted to a job-level env var because the `secrets` context is not available in a step-level `if`. Also pull the database image in its own step. compose-action reports only "Docker Compose command failed with exit code 1" and echoes the pull progress lines, swallowing the message that says why -- diagnosing this failure meant reasoning from which legs passed, because the log never contained the error. A separate pull puts it in the log. sqlite has no `db` service, so the guard reads the compose file's service list rather than matching on the matrix name. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
All five are in the curated tool layer, and three of them are the same shape as C1: an undo descriptor whose replay goes through /ai/chat/action, which calls execute() directly and therefore never renders the preview that would have disclosed the blast radius. I1 — update_filament (and its undo) was broken for any filament stored with an alpha colour. _color_hex demanded exactly 6 hex digits while the rest of the codebase uses 6 or 8 (api/v1/models.py declares min_length=6, max_length=8; _normalize_stored_color_hex accepts both). An update's undo descriptor carries the STORED before-value, so a filament stored as FF0000CC produced an undo that raised on replay -- the user clicked Undo and got a 422. find_filaments also hands the model raw stored colours, so echoing one back failed the call. I2 — undoing a create_spool destroyed the spool's usage history, silently. create_spool's undo named a bare delete_spool, and spool.delete removes every SpoolUsageEvent for the spool: create -> consume -> Undo returned a clean 200 with the history gone, no count and no confirmation. delete_spool now takes an only_if_untouched flag, absent from its model-facing schema and set only by create_spool's own undo descriptor, that refuses and names the event count. An ordinary delete_spool is untouched and still deletes, having shown its preview. I3 — create_filament's undo left behind the vendor it had auto-created, an unlisted fourth exception to docs/ai.md's "Undo restores the previous state". /ai/chat/action runs exactly one tool per call, so the vendor id rides along on delete_filament as another schema-absent argument: the vendor is deleted after the filament, and only while it has no remaining filaments. A pre-existing vendor is never named in the descriptor at all, and a self-created one that has since gained other filaments is kept rather than silently unlinking them. I4 — find_filaments reported the page size as the total, so "how many filament types do I have?" was answered with the cap, with no "returned" key to signal truncation. It now returns the true total plus returned, like all four sibling find_* tools, and the drawer's transparency line reports it. I5 — create_filament turned any unexpected exception into a ToolError carrying str(exc), so a raw SQLAlchemy IntegrityError (failing SQL, table and column names) could reach the user, the model, and MCP clients -- whose _call_tool docstring promises no internal detail crosses that boundary. The broad except stays (the orphan-vendor cleanup needs it) but logs the original and raises a curated message; ItemNotFoundError/ItemCreateError still pass through, their messages being written for a person already. Every test added here was verified by re-injecting the bug it exists to catch and confirming it goes red. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…eview
A-M1 orders._line_row gained the docstring every other private helper in the
package carries.
A-M2 filamentDeleteButton's hideText prop was declared and used internally but
passed by neither call site; removed rather than left as dead public API.
A-M11 the null-clears-a-field description guard asserted only that the word
"clear" appears. It discriminates today, but the whole omit/null/clear
triple is what the model actually needs to be told, so require all three.
B-3 the /ai/chat/action docstring test's own comment still described the
pre-allowlist behaviour ("resolves ANY name in WRITE_TOOLS"), false since
the allowlist landed in the same commit that left the comment in place.
B-5 nothing in CI imported scripts/ai_eval.py, which builds its system prompt
from aichat._system_prompt at module scope -- a signature change there
broke `poe ai-eval` silently. A new test imports the script (and checks
its fixtures still name tools that exist).
B-6 test_every_read_tool_produces_a_non_default_summary passed {} to every
tool, so a branch reading the wrong result keys still returned a
tool-specific sentence full of zeros and still passed. Its DB-backed
counterpart now runs each read tool for real and requires the summary
built from the actual result to differ from the one built from {}.
B-8 delete_order's confirm card rendered "lines: [object Object],[object
Object]" -- order_row's lines are dicts and chatDrawer renders values with
String(value). It now matches create_order's card, a list of strings.
B-9 nothing pinned the two properties the schema-absent undo flags rest on.
One test asserts only_if_empty, only_if_untouched and also_delete_vendor_id
are absent from tool_schemas() for both can_write values (and that each is
still read by the execute it guards, so a rename can't leave the test
guarding nothing); another drives a confirmed write through the chat loop
and asserts the undo descriptor never appears in the outbound payload.
Each of these tests was verified by re-injecting the bug it exists to catch.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… right answer delete_order was model_facing=False, so a user asking the assistant to delete an order was answered from a tool list containing no way to delete one. The model substituted delete_spool and rendered a confirm-card deleting an unrelated spool (reproduced three times out of three); the user clicked through and lost a spool while the order survived. A destructive request with no matching tool makes the model improvise with a destructive neighbour, so the fix is to give the request a correct answer rather than to hide the tool harder. - orders.py: delete_order is model-facing, with a description written for the model instead of the "internal undo helper" stub. - The serialised writer schema payload measures 12,676 chars for 19 tools (12,271 for 18 before this). That breaches the old 12,500 ceiling, so MAX_SCHEMA_CHARS is raised to 13,000 deliberately, with the real figure recorded in the comment. - EXPECTED_MODEL_FACING 18 -> 19; the writer-offered tool list gains delete_order. - _MCP_WRITE_TOOLS is unchanged and now says why: model_facing gates "may the chat model pick this?", where a confirm-card still stands behind every write, while that tuple gates "may an arbitrary MCP client run this outright?", where nothing does. MCP stays delete-free. - _CHAT_ACTION_ALLOWLIST is unchanged (delete_order is still create_order's undo); the /chat/action route description now names set_spool_used_weight as its example of an undo-only primitive, since delete_order no longer is one. - docs/ai.md marks the published tool-selection range as measured on the 18-tool set before delete_order was exposed, rather than presenting it as current. It is deliberately not re-run here: the eval needs a live model endpoint. New test: test_deleting_an_order_has_a_model_facing_tool_of_its_own pins that the offered tool list contains delete_order and that no other offered delete tool accepts an order_id, so the request has exactly one correct answer. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Asked to delete an order, the assistant spent three turns refusing and asking for
confirmation in chat before any confirm-card appeared. The card IS the
confirmation -- nothing is applied until the user clicks Confirm -- so asking
again in prose confirms nothing and costs the user turns. It is the same failure
the tool-selection eval measured, where the dominant error was the model
declining to call any tool at all.
The old wording described the gate ("only applied after the user confirms them in
the interface") without saying what the model should therefore do. It now says to
call the write tool directly instead of asking the user to confirm in chat, while
keeping both properties that sentence carried: no claiming a change before a tool
result confirms it, and no touching records the user did not ask about.
Also adds the never-substitute rule, the prompt-side counterpart to exposing
delete_order: if the user names a kind of record and no tool acts on that kind,
say so plainly rather than acting on a different kind.
Both lines stay inside the can_write branch, so the read-only prompt keeps saying
nothing about writing -- now pinned by phrase, not just by catalog_lookup. Also
fixes a D210 the previous commit's formatter run introduced into a docstring.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
I3's fix carried the vendor id as `also_delete_vendor_id`, an argument absent from delete_filament's schema. Absence from a schema stops the model guessing a name; it stops nothing else. The re-reviewer reproduced an ordinary, non-undo delete_filament call carrying that key destroying an unrelated pre-existing vendor, behind a confirm-card whose text said only "deletes the filament and its 0 spool(s)" -- no mention of any vendor. That falsified the wave's own written invariant that these schema-absent flags "could only ever make a legitimate call fail": only_if_empty and only_if_untouched can only cause a refusal, this one caused an extra deletion. Fixed structurally, the way the codebase already handles undo-only primitives (delete_location, delete_vendor, set_spool_used_weight): delete_filament_and_vendor is a model_facing=False tool taking filament_id and vendor_id, and create_filament's undo names it when -- and only when -- that call created the vendor. delete_filament has no vendor branch left to reach, whatever arguments a caller invents. The new name reaches _CHAT_ACTION_ALLOWLIST through the same AST derivation, which fails in both directions. I3's behaviour is unchanged and its dangerous inverses still hold: a pre-existing vendor is never named in a descriptor at all, and a self-created vendor that has since gained other filaments is kept (and not claimed in the summary). C1's no-cascade refusal holds on the new tool too -- being undo-only, it enforces it unconditionally rather than behind a flag. The new tool's preview names the vendor, so its card cannot under-disclose the way delete_filament's structurally could not help but do. Also covers the deliberate best-effort vendor delete (N3), which had no test: with vendor_db.delete raising, the undo still returns 200, claims no vendor deletion, carries no deleted_vendor_id, and logs the failure at ERROR. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ot source text (N2) The staleness half of B-9's guard asserted `arg_name in inspect.getsource(execute)`. getsource returns comments too, and both flag names appear in comments inside the very functions inspected -- so renaming the live key left the file fully green, and the previous wave reported the mutation as passing when it had not. Nine instances of a structurally-unfailable test on this branch now, this one inside the fix written to close the last. It now walks the parsed AST and collects only string literals the code passes to a call or uses as a subscript key: ast.parse drops comments, and restricting to those two positions drops docstrings as well (a docstring is a bare Expr, never a call argument), so nothing in prose can satisfy the check. Proven by renaming each live key while leaving its comments alone. In both cases the old assertion still evaluated True against the source text and the new one went red. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Two stale prose fixes and three tests for claims the suite could not previously catch: - comment naming a dead flag (also_delete_vendor_id), stale section header for delete_order now that it is model-facing - the vendor-taking delete's card must actually disclose the vendor by name - vendor_id must be read (and coerced) before the filament is destroyed - delete_order's "spools are kept" card claim, pinned against real arrival + delete behaviour Each new test's bug was re-injected and observed red before being reverted.
…ase row (#378) A confirm-card is the last thing a user reads before destroying something, but it printed the raw tool payload: `initial_weight_g: 1000.0`, `archived: false`, `color_hex: FF0000CC`, `order_number: ∅`, and — on an update — a "Before" block next to an "After" block for the reader to diff by eye. Every key now gets a human label and every value is formatted by what it is: - 31 field labels, all reused from the catalog the client already ships in 31 languages, so a card names a field exactly as the form that edits it does. An unmapped key degrades to sentence case, never to raw snake_case. - weights via the existing formatWeight, prices via the configured currency, temperatures/diameter/density with their units, booleans as the app's Yes/No, dates locale-formatted (ordered_at date-only; other timestamps keep a time only when it is not midnight), colours as a swatch plus a name. - an 8-digit colour keeps its hex alongside the name: alpha is information no colour name can carry, and FF0000CC must not silently read the same as FF0000. - updates render as a diff — one row per changed field, old value struck through beside the new one. Unchanged fields are dropped; a field going unset -> set shows "not set -> value", because inside a diff a null IS the change. - creates and deletes hide the rows that decide nothing: id (the title names it), unset values, and defaults like archived:false on a spool being created. That rule is deliberately never applied inside a diff. Card chrome is untouched: title, "Cannot be undone" tag, summary, Confirm/Cancel, danger styling and the post-execution Undo all render as before. Dates use a new long form (LL) rather than the app's numeric L. A table column can afford 20/07/2026 because its header names it and every row shares an order; a lone date on a card read once, while deciding whether to delete an order, cannot. 15 new i18n keys were unavoidable — "not set", a standalone "Open" (orders.state.open carries an interpolation), and 13 colour names (the catalog had no colour vocabulary). All 15 are translated into all 31 locales rather than English-only: i18next was verified to fall back to English, so English-only would have worked, but check-i18n reports per-locale coverage on every run and all 30 non-English locales sit at 100%. chat.confirm.before/after are removed — the two-block layout that used them is gone. Tests: 33 added (817 total). Each was proven able to fail by re-injecting the bug it claims to catch — 20 mutations, 20 red. One of those exposed a hole in this commit's own test suite: the "not really a date" case asserted on a non-date key, so it could never have reached the date parser; it now asserts on ordered_at. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…-rendering # Conflicts: # CHANGELOG.md # spoolman/database/spool.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
Grows the curated AI tool layer from 6 model-facing tools to 18, so the chat assistant and MCP clients can reach everything the web UI shows — and fixes several bugs found along the way, three of which were already shipped.
Assistant reach
New reads: usage statistics and spend over time, orders with derived open/arrived status, locations with occupancy, vendors, and SpoolmanDB catalog lookup.
New writes: create/update/delete a filament, record an order, mark an order arrived (creating a spool per arriving unit), create a location, create a vendor.
spoolman/ai_tools.pybecame a package of per-domain modules merged into one registry. The public surface is unchanged, soaichat.pyandmcp_server.pyneeded no edits.Behaviour changes worth reviewing carefully
IntegrityErrorfor any filament that had spools — there was no way to delete such a filament through the API at all.DELETE /filament/{id}now refuses with 409 naming the spool count unlesscascade=trueis passed; a filament with no spools deletes exactly as before, with no new parameter. No call that previously succeeded now behaves differently.utc_timezone_naivetreated a naive datetime as system-local, so values stored viaPOST/PATCH /order, calibration, spool timestamps and data import were silently shifted on any non-UTC host. Naive now means UTC, matching what every column stores. This is a fix, but it changes stored values for offset-carrying input on non-UTC hosts.update_spool's undo has been broken since the chat assistant shipped — it could not restore a field that was previously empty, and reported success anyway. Change detection is now presence-based, so an omitted field is left alone and an explicitnullclears it.densityanddiameterare the exception: they cannot be null.arrive_order, which is annotateddestructiveHint: trueso a client can prompt. Deletes are still never exposed over MCP.Measuring the cost
Tripling the tool surface is a bet that a small local model can still choose correctly, so this adds
poe ai-eval— 53 fixture prompts scored for tool selection and argument correctness against a live endpoint.Measured on a local 7.6B model (
hhao/qwen2.5-coder-tools), using the production system prompt, across two runs: 47–48/53 (89–91%) tool selection, 44–46/53 (83–87%) with correct arguments. The dominant failure is the model declining to call any tool at all — mostly oncreate_order, whoselinesargument is a list of objects — rather than confusing one tool for another.The harness needs a live endpoint, so it is not part of CI. A
MAX_SCHEMA_CHARSbudget test is in CI: the serialised payload is currently 12.2 KB against a 12.5 KB ceiling.UI
Filament deletion in the web client now shows what it is about to destroy — spool count, that usage history goes too, and that it cannot be undone — driven by the server's own count rather than a client-side guess. A filament with no spools keeps the existing simple confirmation.
Checklist
TESTING_STRATEGY.md)client/public/locales/en/common.json(and all 31 locales)/api/v1and remain backward compatible with upstream Spoolman integrations (Moonraker, OctoPrint, Home Assistant) — see the note onDELETE /filament/{id}above: the only endpoint whose behaviour changed previously failed in the affected case, so no integration that worked before is affectedNotes for review
SUMand a subquery) written for portability, but CI's full DB matrix is the real check.