From 9b27e54f9c2f8dc7f4d5b99022948b44c18a5545 Mon Sep 17 00:00:00 2001 From: programmer-singh Date: Sat, 29 Aug 2026 05:55:00 +0530 Subject: [PATCH 1/8] feat(webmcp): revive three dead features, and stop summing mixed currencies MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Progress.md marked three headline features DEAD — code that exists, passes tests, and has no caller, so it did nothing in the running app. All three are PRD §7 checklist rows and all three were one wiring problem each. **State-gated `approve_expense`.** `Session.refreshPendingApprovals()` was never called, so the count stayed 0, `ToolSession`'s gate never opened, and the tool never registered. The shell now polls on sign-in and after any mutating call. Reads are excluded deliberately: `search_expenses` runs on almost every Copilot question, and polling on it would be a request per question for a count that cannot have changed. **`tool_call_log`.** The table, repository, route and Settings viewer all existed; nothing wrote to them, so the viewer showed seed rows forever. New `ToolCallAudit` POSTs every invocation. It never rejects and never blocks — this runs inside a tool's own execution path, and an audit trail that can fail the action it describes is worse than no audit trail. Payloads over 4KB are truncated with a marker, because a silently shortened row reads as the whole call. Add Expense logs itself, since the declarative form is the only tool call a human can make and is what gives the viewer's Human filter anything to show. **Cross-origin tools.** `discoverRemoteTools()` had no caller — and could not have worked if it had, because the partner page was served from Actuo's own origin, so `getTools()` marked its tools same-origin and the Copilot filtered every one of them out. The demo needs a genuinely second origin, so `scripts/partner-server.mjs` (zero dependencies) serves `frontend/public` on :4201 and `pnpm run dev` starts it as a third pane. The embedded origin comes from `PARTNER_DEMO_ORIGIN` via `GET /api/config`, so a deploy changes it without a rebuild. New `/agent` page embeds it with `allow="tools"`, and also gives `discoveredTools()` and `invocationLog()` their first consumer. Without the Chrome flag it says what is missing rather than showing an empty list, and every other tool keeps working. All three hang off one seam: `ToolRegistry.observe()`. `log()` is already the single point every invocation passes through — the Copilot's, an external agent's, and cross-origin ones — so a subscriber cannot miss a caller. It is a callback rather than an injected service so the registry stays free of HTTP and session dependencies and its spec needs no fakes. A throwing observer is caught. **Mixed currencies.** `converted_amount` is written only when the expense is already in the base currency, and there is no FX pass, so it is null for every foreign row — while every rollup fell back to the raw `amount`. The seed data has INR, USD and EUR, so the dashboard was adding dollars to rupees at 1:1: a $200 charge counted as ₹200, and the Expenses table printed it under a ₹ symbol. Now a row counts only when it has a base-currency value, and the rest are counted and stated — `sumSpend()` returns `{total, excluded}`, `sumByCategory()` returns `unconverted`, and that surfaces as `BudgetStatus.unconvertedCount`, a muted line on the dashboard and budgets screens, and a field in the `get_budget_status` result so the Copilot can qualify the figure instead of reporting a partial total as a complete one. This is the honest interim, not the feature: when real FX starts filling `converted_amount` those rows re-enter every total with no code change. Verified live in Chrome 151 with the WebMCP flag: `approve_expense` present as owner with 3 pending and absent as member (and the API returns 403 for that member either way); an agent row and a human row both landing in `tool_call_log`; both partner tools discovered from localhost:4201 with `executeTool()` returning a price cross-origin; the dashboard totalling INR only and naming the two rows it left out; light and dark both legible. Not verified: the mobile tab bar at an actual 390px viewport — the automation viewport is pinned, so the six labels were confirmed by measurement (widest is "Dashboard" at ~54px in a 65px slot). Hard-navigating to `/agent` 302s to /login, but that is pre-existing and affects every gated route: the server routes prerender `**`, so `authGuard` runs with no session. Both are recorded in Progress.md. --- CLAUDE.md | 54 ++- Progress.md | 99 +++-- backend/.env.example | 6 + backend/src/budgets/budgets.service.spec.ts | 110 ++++++ backend/src/budgets/budgets.service.ts | 11 +- backend/src/config/config.controller.ts | 8 + backend/src/config/env.service.ts | 13 + backend/src/supabase/repositories.ts | 8 + backend/src/supabase/supabase.repositories.ts | 30 +- frontend/public/robots.txt | 1 + frontend/src/app/app.routes.ts | 8 + frontend/src/app/app.spec.ts | 118 ++++++ frontend/src/app/app.ts | 54 ++- frontend/src/app/copilot/copilot.spec.ts | 66 ++++ frontend/src/app/copilot/copilot.ts | 25 +- frontend/src/app/core/expense/amount.spec.ts | 162 +++++++++ frontend/src/app/core/expense/amount.ts | 69 +++- .../app/pages/add-expense/add-expense.spec.ts | 75 +++- .../src/app/pages/add-expense/add-expense.ts | 26 ++ frontend/src/app/pages/agent/agent.spec.ts | 249 +++++++++++++ frontend/src/app/pages/agent/agent.ts | 343 ++++++++++++++++++ .../app/pages/budgets/budget-rollup.spec.ts | 25 ++ .../src/app/pages/budgets/budget-rollup.ts | 8 + .../src/app/pages/budgets/budgets.spec.ts | 27 ++ frontend/src/app/pages/budgets/budgets.ts | 18 + .../src/app/pages/dashboard/dashboard.spec.ts | 65 ++++ frontend/src/app/pages/dashboard/dashboard.ts | 42 ++- .../app/pages/dashboard/spend-pace.spec.ts | 6 +- .../src/app/pages/dashboard/spend-pace.ts | 40 +- .../src/app/pages/expenses/expenses.spec.ts | 39 ++ frontend/src/app/pages/expenses/expenses.ts | 11 +- frontend/src/app/tools/expense-tools.ts | 8 + .../src/app/webmcp/tool-call-audit.spec.ts | 108 ++++++ frontend/src/app/webmcp/tool-call-audit.ts | 92 +++++ frontend/src/app/webmcp/tool-registry.spec.ts | 99 +++++ frontend/src/app/webmcp/tool-registry.ts | 40 ++ package.json | 3 +- scripts/partner-server.mjs | 68 ++++ shared/src/dto.ts | 11 + 39 files changed, 2161 insertions(+), 84 deletions(-) create mode 100644 backend/src/budgets/budgets.service.spec.ts create mode 100644 frontend/src/app/core/expense/amount.spec.ts create mode 100644 frontend/src/app/pages/agent/agent.spec.ts create mode 100644 frontend/src/app/pages/agent/agent.ts create mode 100644 frontend/src/app/webmcp/tool-call-audit.spec.ts create mode 100644 frontend/src/app/webmcp/tool-call-audit.ts create mode 100644 scripts/partner-server.mjs diff --git a/CLAUDE.md b/CLAUDE.md index 153bed9..9c48008 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -33,7 +33,7 @@ declarations (see "Why pnpm changes things" below). ```bash pnpm install # `pnpm install --frozen-lockfile` is the CI equivalent of `npm ci` -pnpm run dev # builds shared, then backend (:3000) + frontend (:4200) together +pnpm run dev # shared, then backend (:3000) + frontend (:4200) + partner demo (:4201) pnpm run build # shared -> backend -> frontend, in that order pnpm test # backend + frontend unit tests pnpm run test:e2e # backend e2e @@ -49,9 +49,13 @@ pnpm --filter backend run test:e2e # frontend: MUST go through ng test, which is the @angular/build:unit-test builder pnpm --filter frontend run test -pnpm --filter frontend exec ng test --no-watch --test-name-pattern "ToolRegistry" +pnpm --filter frontend exec ng test --no-watch --filter "ToolRegistry" ``` +The name filter is `--filter` (a regex over suite and test names). It is **not** +`--test-name-pattern` — that is vitest's own flag, and the Angular builder rejects +it outright with `Unknown argument`. + `frontend`'s `test` script already carries `--no-watch`, deliberately: npm swallows a bare `--` while pnpm forwards it to Angular as an empty argument, which the builder rejects with a schema error. Keeping the flag inside the script makes it behave the @@ -175,6 +179,16 @@ working — do not "simplify" them away. `fromOrigins`/`exposedTo` with `NotSupportedError`, so the polyfill is a same-origin fallback only. +**Cross-origin also requires an actual second origin.** The partner page lives in +`frontend/public/partner-demo/`, so it is *also* served by the app itself — and from +there `normalizeRegisteredTool()` marks its tools `isCrossOrigin: false`, which is +exactly the set the Copilot filters out. `scripts/partner-server.mjs` (zero +dependencies, `node:http`) serves `frontend/public` on **:4201** so the same +`/partner-demo/` path exists on a different origin; `pnpm run dev` starts it as a +third pane. The origin the app embeds is `PARTNER_DEMO_ORIGIN`, served to the browser +by `GET /api/config` — so a deploy changes it without a rebuild. When it equals the +app's own origin, `/agent` says so instead of showing an empty list. + ## Module map **backend/** — `auth/` (argon2id + JWT, rotating refresh, guards), `expenses/` @@ -199,11 +213,21 @@ request; the access token deliberately carries no role claim. translation itself — never pre-convert with `toFunctionDeclarations()`, or the schema lands under `parameters` where the second pass cannot see it and every tool reaches the model with no arguments.** -- `webmcp/` — `ToolRegistry` and `ToolSession` (state gating). +- `webmcp/` — `ToolRegistry` and `ToolSession` (state gating), plus + `ToolCallAudit`, which POSTs every invocation to `/api/tool-calls`. + `ToolRegistry.observe()` is the single seam every invocation passes through; + `App` subscribes once and fans out to the audit write and the + pending-approval re-poll. Keep HTTP and session dependencies out of the + registry itself — that is what keeps its spec free of fakes. - `tools/` — the five tool `execute()` implementations over `/api/*`. - `copilot/` — `Copilot` (the agent loop) and `CopilotPanel` (orb + panel). - `core/api/` — `ApiClient`. `core/theme/` — `ThemeService`. -- `pages/add-expense/` — the declarative WebMCP form. +- `pages/add-expense/` — the declarative WebMCP form. It is the only tool call a + *human* can make, so it logs itself with the actor `agentInvoked` reports; + everything else through the registry is an agent. +- `pages/agent/` — `/agent`, the WebMCP surface made visible: browser support, + the cross-origin partner iframe and what it exposed, and the live invocation + log. The only consumer of `discoveredTools()` and `invocationLog()`. ## Thought signatures (Gemini 3 function calling) @@ -246,6 +270,28 @@ caller**, so it does nothing in the running app. Three headline features are in that state today; `DEAD` is separated from `PARTIAL` precisely because it looks finished in both the source and the test count. +## Money: never add two currencies + +There is no FX pass. `expenses.service.ts` writes `converted_amount` **only** +when the expense is already in the org's base currency, so it is `null` for +every foreign row — and the seed data has INR, USD and EUR. + +So a row counts toward a total only when it has a base-currency value, and the +ones that do not are **counted and stated**, never dropped and never added: + +- Frontend: `core/expense/amount.ts` — `isConverted()`, `sumSpend()` (returns + `{total, excluded}`), `expenseCurrency()` for the label on a single row, and + `excludedNotice()` for the copy. Every rollup goes through `sumSpend`. +- Backend: `sumByCategory()` sums only non-null `converted_amount` and returns + `unconverted`; that reaches the client as `BudgetStatus.unconvertedCount`, and + the `get_budget_status` tool passes it to the model so the Copilot can qualify + the figure rather than state a partial total as a complete one. + +The earlier code fell back to the raw `amount`, on the reasoning that a slightly +wrong number beat a bar reading zero. It was not slightly wrong: a $200 charge +was counted as ₹200. When a real FX pass starts filling `converted_amount`, +those rows re-enter every total with no code change. + ## Architectural rules that must not be violated These are the load-bearing constraints — most bugs worth preventing here are violations of one of them. diff --git a/Progress.md b/Progress.md index db3fc96..11204ba 100644 --- a/Progress.md +++ b/Progress.md @@ -2,7 +2,7 @@ Tracks every feature in the PRD against what is actually in the codebase. -**Last audited:** 2026-08-29 · **Baseline:** 9 shared · 49 backend unit · 34 backend e2e · 602 frontend +**Last audited:** 2026-08-29 · **Baseline:** 9 shared · 53 backend unit · 34 backend e2e · 676 frontend Status is evidence-based, not aspirational. A row is `DONE` only when the code exists, is reachable from the running app, and has a test. A file existing is not @@ -146,18 +146,27 @@ their own expense. | Item | Phase | Status | Notes | |---|---|---|---| -| Original + converted amounts stored | 1 | ✅ | Columns exist; `core/expense/amount.ts` prefers `convertedAmount` | +| Original + converted amounts stored | 1 | ✅ | Columns exist. `core/expense/amount.ts` owns the rule: `sumSpend()` adds only base-currency rows and reports the rest | | Live FX + daily cache | 1 | ⬜ | No FX client, no cache, no rates table | | Historical rate lock | 1 | ⬜ | No rate column | -> **⚠️ Correctness bug, not just a gap.** `convertedAmount` is set only when the -> currency already equals the base currency, otherwise `null`, and the repository -> falls back to the raw `amount` when summing. **Budget and dashboard totals -> therefore add USD and EUR to INR as if they were the same unit.** The seed data -> contains all three. This understates or overstates real spend today. +> **Totals are now honest about what they exclude.** `convertedAmount` is still +> only set when the currency already equals the base currency, so foreign rows +> have no base-currency value. They used to be added at face value — a $200 +> charge counted as ₹200. Now a row counts only when it has a converted value, +> and the ones that do not are **counted and stated**: `sumSpend()` returns +> `{total, excluded}`, `sumByCategory()` returns `unconverted`, and that surfaces +> as `BudgetStatus.unconvertedCount`, a muted line on the dashboard and budgets +> screens, and a field in the `get_budget_status` tool result so the Copilot can +> qualify the figure. Row labels follow the same rule — an unconverted $50 prints +> as `$50`, not `₹50`. +> +> This is the honest interim, not the feature: real FX (live rates, daily cache, +> historical lock) is still ⬜, and the moment `converted_amount` starts being +> filled, those rows re-enter every total with no code change. **Verify:** file expenses in two currencies and confirm the dashboard total is -not a naive sum. +not a naive sum, and that it says how many rows it left out. ## §6.6 Analytics — 🟡 @@ -179,6 +188,8 @@ transport. **Phase 2.** ## §6.8 Copilot — 🟡 (strongest area) +Cross-origin is live as of 2026-08-29; only the standalone-script packaging (Phase 3) is left. + | Item | Phase | Status | Notes | |---|---|---|---| | Floating widget, orb → panel | 0 | ✅ | Full-screen sheet on mobile | @@ -188,7 +199,7 @@ transport. **Phase 2.** | Confirmation before mutating tools | 0 | ✅ | In-chat card. PRD says "native dialog"; in-chat was chosen deliberately | | Key-setup flow when no key | 0 | ✅ | Opens into setup rather than failing silently | | Embeddable via one `