diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml new file mode 100644 index 0000000..9891632 --- /dev/null +++ b/.github/workflows/ci.yml @@ -0,0 +1,69 @@ +# The Definition of Done gate from Progress.md, run on every push and PR. +# +# It is the same five commands, in the same order, for the same reasons — each +# one is here because skipping it caused a real failure in this repository: +# +# typecheck catches what the test build tolerates +# test shared + backend unit + frontend +# test:e2e a SEPARATE vitest config; `pnpm test` does not include it, and a +# change was once committed on unit tests alone that broke an e2e +# spec pinning the same data +# build catches SSR prerender failures and template type errors +# +# No secrets are needed. `EnvService` raises `MissingEnvError` at call time +# rather than import time precisely so the app boots, serves /api/health and +# /api/config, and runs its e2e suite without any credentials — which is what +# makes this workflow possible at all. + +name: CI + +on: + pull_request: + +concurrency: + # A new push supersedes the run it interrupted; nothing here is worth + # finishing for a commit that is already stale. + group: ci-${{ github.ref }} + cancel-in-progress: true + +jobs: + gate: + name: Definition of Done + runs-on: ubuntu-latest + timeout-minutes: 20 + + steps: + - uses: actions/checkout@v4 + + # No version pin here: the root package.json `packageManager` field is the + # single source of truth, and duplicating it is how the two drift. + - uses: pnpm/action-setup@v4 + + - uses: actions/setup-node@v4 + with: + node-version-file: .nvmrc + cache: pnpm + + # `--frozen-lockfile` is the CI equivalent of `npm ci`: it fails rather + # than silently resolving a dependency the lockfile does not name. + - name: Install + run: pnpm install --frozen-lockfile + + # `shared` must be built before either consumer, or both fail at once with + # TS2307. `pnpm test` does not build it. + - name: Build shared + run: pnpm run build:shared + + - name: Typecheck + run: | + pnpm --filter backend exec tsc --noEmit -p tsconfig.json + pnpm --filter frontend exec tsc --noEmit -p tsconfig.app.json + + - name: Unit tests + run: pnpm test + + - name: End-to-end tests + run: pnpm run test:e2e + + - name: Build + run: pnpm run build diff --git a/CLAUDE.md b/CLAUDE.md index 153bed9..1d4d955 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -4,9 +4,10 @@ This file provides guidance to Claude Code (claude.ai/code) when working with co ## Project state -Scaffold, backend, design system, BYOK Gemini layer, WebMCP tool layer and the -Copilot are all built and committed. Remaining: app shell + routing wiring, -Firebase App Hosting deploy, and the README/demo script. +Every Phase 0 item is built and committed — scaffold, backend, design system, +BYOK Gemini layer, WebMCP tool layer, Copilot, app shell, expense workflow UI, +PWA, SEO, and the single-process deploy path. Remaining: running the actual +Firebase deploy, and the demo video. Everything else in the PRD is Phase 1–3. Build order is PRD §10 Phase 0, scoped WebMCP-demo-first. @@ -25,6 +26,61 @@ piece is the **Actuo Copilot** — a site-agnostic chat widget that discovers to Built for a hackathon deadline (Aug 31, 2026). +## Comments + +The comment density here is deliberate and it is high — 3,850 comment lines +across 27,649, and files like `core/expense/amount.ts` and `webmcp/tool-call-audit.ts` +are over half prose. **That is the design, not an accident, and not cleanup +backlog.** Most of what this repo knows that a reader cannot infer — why +`approved` is not `success`, why `turnsToContents()` replays raw parts, why a +$200 charge must not be added to a rupee total — lives in those comments. A +sweep that removes them destroys the most valuable thing in the codebase. + +So "remove unnecessary comments" here means a very short list: + +- ASCII rule lines that carry no words (`// ------------------`), including the + two rules wrapped around a section title. Keep the title, drop the rules. +- Commented-out code, and comments describing code that no longer exists. +- A comment that only restates the line beneath it. + +**Never remove**, however verbose: + +- Any comment saying *why* — a rejected alternative, a bug it prevents, a spec + quirk, a rule that looks arbitrary until explained. +- `LOAD-BEARING`, `ORDER IS SIGNIFICANT`, and "do not simplify this away" notes. + Several exist because the simplification was already tried and broke something + subtle: thought signatures, `parseInputSchema()`, `setGlobalPrefix`. +- PRD/Design-Doc section references (`PRD §6.4`, `§2.2`) — they are the link back + to the source of truth. +- JSDoc `@example` blocks. They look like commented-out code and are not. + +Measure before sweeping. A scan of all 177 source files found only 16 removable +lines — 5 comments restated their code, zero were stale TODOs, zero were +commented-out code. If a comment sweep proposes to remove hundreds of lines, it +has misread prose as noise; stop and re-read the list above. + +## Commits + +**No commit message, PR title, or PR body in this repo mentions Claude, Claude +Code, or names an AI as a collaborator.** Specifically, never append any of: + +- `Co-Authored-By: Claude ...` — or any `Co-Authored-By` trailer naming an AI +- `Claude-Session: https://claude.ai/code/...`, or a bare `claude.ai/code` URL +- `🤖 Generated with [Claude Code](https://claude.com/claude-code)` + +This **overrides the default Claude Code commit and PR trailers** — do not add +them back "because the tool normally does". The work is authored by the repo's +git user alone; a co-author trailer attributes it to a contributor who is not +one, and GitHub then renders that account on every commit and in the repo's +contributor list. + +History was rewritten once, on 2026-08-29, to strip those two trailers from all +18 commits on `main` and `revive-webmcp-dead-features`. Re-introducing them +means doing that rewrite again, so check `git log` before committing. + +Naming the file `CLAUDE.md` in a message is fine — it is a real file in the +repo, and several commits legitimately say they changed it. + ## Commands **This repo uses pnpm.** Do not run `npm install` — it would create a competing @@ -33,7 +89,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 +105,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 @@ -81,7 +141,8 @@ and `pnpm run build` handle the ordering. `frontend/` and `backend/` are separate codebases (own package.json, tsconfig, tests) that ship as **one** Firebase App Hosting deploy: a single Node process routes -`/api/*` to Nest and everything else to Angular's SSR handler. +`/api/*` to Nest and everything else to Angular's SSR handler. That process is +`server.mjs` at the repo root — see "The deploy" below. ## Why pnpm changes things @@ -175,6 +236,79 @@ 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. + +## The deploy + +`server.mjs` at the repo root is the production entry point: `pnpm start`, and +what `apphosting.yaml`'s `runCommand` names. It imports the two build outputs and +composes them in one process. + +- It appends the Angular handler to **Nest's own Express instance**, after Nest's + routes. That works only because `setGlobalPrefix('/api')` scopes Nest's + not-found router to `/api` instead of installing a global catch-all — the + property `backend/test/routing-contract.e2e-spec.ts` exists to pin. If that + spec breaks, the deploy is broken too, in a way that returns the app shell for + a bad API call. +- The built Angular SSR bundle is **self-contained** (only `node:` imports; + Express is bundled in) and serves `dist/browser` itself. So the repo root needs + no runtime dependency, and `node_modules` there stays at two dev packages. +- `createNodeRequestHandler` returns its argument unchanged, so the exported + `reqHandler` *is* the Express app and mounts directly as middleware. + +**`NG_ALLOWED_HOSTS` is load-bearing, and it fails silently.** Angular 21 checks +the `Host` header against an allowlist (SSRF protection). Off the list it does not +error — it falls back to **client-side rendering**, quietly discarding the SSR and +structured-data work in PRD §8.5. The env var *replaces* the build-time list in +`angular.json` (`getAllowedHostsFromEnv() ?? options.allowedHosts`), so a deploy +must list every hostname it answers on. The port is stripped before matching, and +`*.example.com` wildcards work. Verify with: the HTML for `/` contains +`ng-server-context`. `angular.json` carries `localhost` so `node server.mjs` +renders locally. + +**`PUBLIC_ORIGIN` is a BUILD-time variable, not a runtime one.** The public pages +are prerendered, so absolute URLs — `` in the sitemap, `og:image`, +`canonical` — must be decided before the build finishes. `index.html`, +`sitemap.xml` and `robots.txt` carry a `__PUBLIC_ORIGIN__` sentinel that survives +prerendering into every generated file, and `scripts/stamp-seo.mjs` replaces it +across `dist/frontend/browser` as the last step of `pnpm run build`. Unset, it +substitutes `''` and everything stays root-relative and valid. + +**The service worker must never cache `/api`.** `ngsw-config.json` has no +`dataGroups` at all, deliberately: a cached response would show stale money and +would undercut the promise that every read goes through an authenticated route. +`navigationUrls` also excludes `/partner-demo/**`, which is a separate site that +re-registers WebMCP tools on each load. + +**`NODE_ENV=production` changes one behaviour on purpose**: `EnvService.partnerOrigin` +drops its `http://localhost:4201` default, because serving that from a deployed +instance makes `/agent` embed an iframe pointing at each visitor's own machine. + +## The expense workflow is one table, shared + +Which action is legal, who may perform it, and on whose row — all of it lives in +`shared/src/domain.ts` (`canTransition`, `TRANSITION_ROLES`, +`OWNER_ONLY_ACTIONS`, `NOT_ON_OWN_ACTIONS`, `mayPerformOn`). `ExpensesService.transition` +enforces it and `core/expense/expense-actions.ts` renders from it, so a button +cannot appear for something the server would refuse. + +`backend/src/expenses/expense-state-machine.ts` keeps only what is genuinely +server-side — the 409 mapping — and re-exports the rest. + +Two rules that look alike and are not: `OWNER_ONLY_ACTIONS` (submit, rework) is +"only your own row, unless you are an approver"; `NOT_ON_OWN_ACTIONS` (approve, +reject) is "never your own row, whoever you are" — segregation of duties. +Getting them confused is how the UI briefly offered an Approve button that always +403'd. + ## Module map **backend/** — `auth/` (argon2id + JWT, rotating refresh, guards), `expenses/` @@ -199,11 +333,29 @@ 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). +- `core/seo/` — `SeoService` applies each route's `data.robots` on every + navigation. A meta tag is document-global, so a component that sets one on + load leaves it behind; that is how `index, follow` used to end up on + `/dashboard`. A route that declares nothing is treated as `noindex`. +- `core/pwa/` — `PwaService`: the deferred install prompt and online/offline, + surfaced as a banner in the shell. +- `core/expense/` — the money rules (`sumSpend`) and the workflow rules + (`availableActions`), both pure functions over `@actuo/shared`. +- `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 +398,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..0ec7f31 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 · 64 backend unit · 34 backend e2e · 742 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 @@ -112,8 +112,8 @@ alive. Always verify after — `pkill`'s exit status is not proof. | Recurring templates | 1 | ⬜ | `recurring_templates` table is **absent from the migration** | | CSV bulk import | 2 | ⬜ | Export exists; import does not | | Soft delete | 0 | ✅ | Partial indexes `where deleted_at is null` | -| Audit trail | 0 | 🟡 | `audit_log` is **written** on every mutation, but has no read endpoint and no viewer. The Settings "audit log" reads `tool_call_log`, a different table | -| **Expense actions in the UI** | 0 | ⬜ | The Expenses page is **read-only** — no edit, delete, submit or approve control anywhere. `POST /expenses` via Add Expense is the only mutation a human can reach | +| Audit trail | 0 | ✅ | `GET /api/audit-log` (owner/admin) plus a Change history panel in Settings, beside the renamed Tool calls panel. The copy states the difference: `audit_log` is what changed, `tool_call_log` is what an agent did | +| **Expense actions in the UI** | 0 | ✅ | Submit / approve / reject / reimburse / reopen / delete on every row, offered from `mayPerformOn` in `@actuo/shared` — the same function `ExpensesService.transition` enforces with, so a button that would 403 is never rendered. Decisions take an optional note inline; delete is two-step | **Verify:** create an expense, confirm it appears in the list and in `audit_log`. @@ -125,7 +125,7 @@ alive. Always verify after — `pkill`'s exit status is not proof. | Per-team budgets | 2 | ⬜ | No team entity exists | | Threshold alerts (80%) | 1 | ⬜ | Utilization is computed and shown; no threshold, no alert, no notification | | Rollover vs reset | 1 | 🟡 | `rollover` column is persisted but **read by nothing** — `status()` always computes a fresh calendar month, so the flag has no effect | -| Budget creation UI | 1 | ⬜ | `POST /api/budgets` exists; nothing in the app calls it | +| Budget creation UI | 1 | ✅ | A form on the Budgets page for owner/admin. `POST /budgets` inserts and a unique index makes a repeat a 409, so only categories without a budget are offered — **changing** an existing budget is still unsupported (no PATCH route) | **Verify:** with a category over budget, confirm the bar turns danger-toned and the figure matches a hand-check against the expense rows. @@ -135,7 +135,7 @@ the figure matches a hand-check against the expense rows. | Item | Phase | Status | Notes | |---|---|---|---| | Five statuses + state machine | 1 | ✅ | `expense-state-machine.ts`; all 20 illegal transitions tested | -| submit → approve/reject | 1 | 🟡 | **API only.** Self-approval banned, 409 on illegal moves. No UI control exists | +| submit → approve/reject | 1 | ✅ | UI controls on every row. Self-approval is banned in one shared rule (`NOT_ON_OWN_ACTIONS`) that the server enforces and the UI reads, so the button is not offered either | | Multi-step chains | 2 | ⬜ | One decision ends the flow | | Comment thread | 2 | 🟡 | `approvals.comment` is written on a decision; there is no read path, no thread, and no way to comment without deciding | @@ -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 `