From 658bb26768ec668e4ec1219c1d80bfb27fd297f4 Mon Sep 17 00:00:00 2001 From: theprogrammersingh Date: Fri, 4 Sep 2026 10:05:26 +0530 Subject: [PATCH] fix(reports): make the generated CSV actually downloadable generate_report returned `/api/reports//download`, which the model wrote into chat as a link. That route needs the session bearer header and a browser navigation carries none, so every click 401'd. - generate_report returns a job id and a bounded preview, never the URL - the tool call card gets a Download button that fetches with the token and saves the blob - new download_report tool for agents with no UI: execute() runs in the page's session, so the page downloads on their behalf - the download route sends Content-Disposition --- Progress.md | 14 +- .../src/reports/reports.controller.spec.ts | 88 ++++++++++++ backend/src/reports/reports.controller.ts | 50 +++++-- backend/src/reports/reports.service.spec.ts | 39 ++++++ backend/src/reports/reports.service.ts | 33 +++++ frontend/src/app/ai/gemini-schema.spec.ts | 1 + frontend/src/app/copilot/copilot-panel.ts | 38 ++++- frontend/src/app/copilot/copilot.ts | 3 + frontend/src/app/core/api/api-client.ts | 66 ++++++++- .../src/app/core/download/save-file.spec.ts | 48 +++++++ frontend/src/app/core/download/save-file.ts | 23 +++ .../app/core/reports/report-download.spec.ts | 132 ++++++++++++++++++ .../src/app/core/reports/report-download.ts | 79 +++++++++++ frontend/src/app/tools/expense-tools.spec.ts | 77 +++++++++- frontend/src/app/tools/expense-tools.ts | 58 +++++++- frontend/src/app/ui/tool-call-card.spec.ts | 51 +++++++ frontend/src/app/ui/tool-call-card.ts | 30 ++++ frontend/src/app/webmcp/tool-session.spec.ts | 1 + shared/src/tools.ts | 41 +++++- 19 files changed, 839 insertions(+), 33 deletions(-) create mode 100644 backend/src/reports/reports.controller.spec.ts create mode 100644 frontend/src/app/core/download/save-file.spec.ts create mode 100644 frontend/src/app/core/download/save-file.ts create mode 100644 frontend/src/app/core/reports/report-download.spec.ts create mode 100644 frontend/src/app/core/reports/report-download.ts diff --git a/Progress.md b/Progress.md index 18b910e..27f7df3 100644 --- a/Progress.md +++ b/Progress.md @@ -195,7 +195,7 @@ day's rate; a row on a weekend names the preceding working day. | Spend by category | 1 | ✅ | `GET /api/analytics/summary` returns `byCategory` with per-category spend and share. Also `get_spend_summary` WebMCP tool | | Month-over-month deltas | 1 | ✅ | `monthOverMonthDelta` in `AnalyticsSummary`, percentage change vs prior month (null when no prior data). Exposed in `get_spend_summary` tool output | | Team vs individual | 2 | ⬜ | — | -| CSV export | 0 | ✅ | Chunked, cancellable, complete across pages | +| CSV export | 0 | ✅ | Chunked, cancellable, complete across pages. The file is fetched with the session bearer and saved via `saveBlob` (`core/reports/report-download.ts`) — `generate_report` returns a job id and a bounded preview, never the download URL, which is an authenticated route no chat link can reach. Two ways to get the file: the Download button on the tool call card, and the `download_report` tool for agents | | **PDF export** | 2 | ⬜ | `pdf` is now **rejected** rather than silently answered with CSV. `shared/src/report-format-contract.spec.ts` pins the tool schema and the backend DTO to the same list | | `/api/analytics/*` | 1 | ✅ | `backend/src/analytics/` module with `GET /api/analytics/summary`. Returns current/prior month totals, MoM delta, per-category breakdown, excluded counts. 7 unit tests | @@ -235,18 +235,26 @@ Cross-origin is live as of 2026-08-29; only the standalone-script packaging (Pha | Aspect | Status | Notes | |---|---|---| | Declarative API (annotated form) | ✅ | Add Expense: `toolname`/`tooldescription`/`toolparamdescription`/`toolautosubmit`, `agentInvoked` + `respondWith`, **no JS registration** | -| Imperative `registerTool` | ✅ | Five tools, per-tool `AbortController` lifetime | +| Imperative `registerTool` | ✅ | Six tools, per-tool `AbortController` lifetime | | JSON Schema inputs | ✅ | One definition in `shared/src/tools.ts`, used by client and server | | **Dynamic / state-gated tools** | ✅ | The shell polls on sign-in and after every mutating call. Verified live: `approve_expense` present in `getTools()` as owner with 3 pending, absent as member, and every tool retired on sign-out | | Cancellation (`AbortSignal`) | ✅ | Client aborts, polls stop, server abandons the job mid-fetch and mid-format | | Cross-origin tools | ✅ | See §6.8. Needs a genuinely second origin — same-origin descriptors are filtered out, which is what made the earlier in-repo page unprovable. It is now a separately built, independently deployed app Actuo does not own, and it is proven from the deploy itself, not only from localhost | -| Security annotations | ✅ | `readOnlyHint` on all five, driving the shell's re-poll and the `/agent` panel. `untrustedContentHint` on `search_expenses` and `approve_expense` — the two that surface *another person's* free text — and on the converter's tools, whose results carry third-party rate data; shown as a badge on the tool-call card | +| Security annotations | ✅ | `readOnlyHint` on the reads, driving the shell's re-poll and the `/agent` panel. `untrustedContentHint` on `search_expenses` and `approve_expense` — the two that surface *another person's* free text — and on the converter's tools, whose results carry third-party rate data; shown as a badge on the tool-call card | | `getTools()` discovery | ✅ | Drives the cross-origin path and the `/agent` panel; re-runs on `toolchange`. The Copilot still reads its own registry for local tools, deliberately — see "the tool registry decision" | | `executeTool()` + manual debug panel | 🟡 | `executeTool()` done, and `/agent` renders `Copilot.crossOriginTools` and the registry's `invocationLog()`. Still read-only: there is no form to invoke a tool by hand with arbitrary arguments | > Open question: `generate_report` is annotated `readOnlyHint: true` but creates a > server-side job. Defensible, but decide it deliberately. +`download_report` is the companion tool that saves a finished report to disk. It +exists because a file has no other route out for a client that can only call +tools: the download route needs the session bearer header, so no URL — in chat or +handed to an agent — can fetch it. A tool's `execute()` runs *inside the page*, in +the authenticated session, so the page does the fetch and the browser save on the +agent's behalf. `readOnlyHint: false` (a file lands on the user's machine) with no +confirmation (asking to download is the confirmation). + ## §8.4 PWA — ✅ `@angular/service-worker` installed and enabled on the production build only. diff --git a/backend/src/reports/reports.controller.spec.ts b/backend/src/reports/reports.controller.spec.ts new file mode 100644 index 0000000..4f650e7 --- /dev/null +++ b/backend/src/reports/reports.controller.spec.ts @@ -0,0 +1,88 @@ +import { NotFoundException } from '@nestjs/common'; +import type { Response } from 'express'; +import { describe, expect, it, vi } from 'vitest'; +import type { AuthenticatedUser } from '../auth/auth.types.js'; +import { ReportsController } from './reports.controller.js'; +import type { ReportJob, ReportsService } from './reports.service.js'; + +const USER: AuthenticatedUser = { + userId: 'user-1', + orgId: 'org-1', + email: 'priya@actuo.demo', + role: 'owner', +}; + +const READY: ReportJob = { + id: 'job-1', + orgId: 'org-1', + status: 'ready', + format: 'csv', + rows: 2, + url: '/api/reports/job-1/download', + filename: 'actuo-expenses-2026-08-01_2026-08-31.csv', + content: 'date,merchant,amount,currency,status\n2026-08-10,Barista,450,INR,approved', + preview: 'date,merchant,amount,currency,status\n2026-08-10,Barista,450,INR,approved', + previewTruncated: false, + createdAt: Date.now(), +}; + +function createController(job: ReportJob) { + const reports = { get: vi.fn().mockReturnValue(job) } as unknown as ReportsService; + const res = { setHeader: vi.fn() } as unknown as Response; + return { controller: new ReportsController(reports), res }; +} + +describe('ReportsController', () => { + it('exposes the preview and filename on the status route', () => { + const { controller } = createController(READY); + + expect(controller.status(USER, 'job-1')).toEqual({ + jobId: 'job-1', + status: 'ready', + rows: 2, + url: '/api/reports/job-1/download', + filename: 'actuo-expenses-2026-08-01_2026-08-31.csv', + preview: READY.preview, + previewTruncated: false, + error: undefined, + }); + }); + + /** + * Without this the CSV renders in the tab instead of saving, which reads as + * a broken download even when the request itself succeeded. + */ + it('sends the CSV as an attachment named after the range', () => { + const { controller, res } = createController(READY); + + const body = controller.download(USER, 'job-1', res); + + expect(res.setHeader).toHaveBeenCalledWith( + 'Content-Disposition', + 'attachment; filename="actuo-expenses-2026-08-01_2026-08-31.csv"', + ); + expect(body).toBe(READY.content); + }); + + it('falls back to a job-id filename when the job carries none', () => { + const { controller, res } = createController({ ...READY, filename: undefined }); + + controller.download(USER, 'job-1', res); + + expect(res.setHeader).toHaveBeenCalledWith( + 'Content-Disposition', + 'attachment; filename="actuo-report-job-1.csv"', + ); + }); + + it('404s a job that is not ready, and sets no attachment header', () => { + const { controller, res } = createController({ + ...READY, + status: 'pending', + content: undefined, + }); + + expect(() => controller.download(USER, 'job-1', res)).toThrow(NotFoundException); + expect(res.setHeader).not.toHaveBeenCalled(); + }); +}); diff --git a/backend/src/reports/reports.controller.ts b/backend/src/reports/reports.controller.ts index 21368a1..3ecc423 100644 --- a/backend/src/reports/reports.controller.ts +++ b/backend/src/reports/reports.controller.ts @@ -1,15 +1,28 @@ -import { Body, Controller, Get, Header, NotFoundException, Param, Post } from '@nestjs/common'; +import { + Body, + Controller, + Get, + Header, + NotFoundException, + Param, + Post, + Res, +} from '@nestjs/common'; +import type { Response } from 'express'; import { CurrentUser } from '../auth/current-user.decorator.js'; import { Roles } from '../auth/roles.decorator.js'; import type { AuthenticatedUser } from '../auth/auth.types.js'; import { GenerateReportDto } from './dto/report.dto.js'; -import { ReportsService, type ReportStatus } from './reports.service.js'; +import { ReportsService, type ReportJob, type ReportStatus } from './reports.service.js'; interface ReportView { jobId: string; status: ReportStatus; rows?: number; url?: string; + filename?: string; + preview?: string; + previewTruncated?: boolean; error?: string; } @@ -39,24 +52,39 @@ export class ReportsController { return view(this.reports.cancel(user, id)); } + /** + * Authenticated like every other route: the caller sends a bearer header, so + * this is reached by `ApiClient.download()`, never by navigating the browser + * to the URL. Content-Disposition is set here rather than with `@Header` + * because the filename carries the job's own date range. + */ @Roles('owner', 'admin', 'member') @Get(':id/download') @Header('Content-Type', 'text/csv; charset=utf-8') - download(@CurrentUser() user: AuthenticatedUser, @Param('id') id: string): string { + download( + @CurrentUser() user: AuthenticatedUser, + @Param('id') id: string, + @Res({ passthrough: true }) res: Response, + ): string { const job = this.reports.get(user, id); if (job.status !== 'ready' || job.content === undefined) { throw new NotFoundException('Report is not ready'); } + const filename = job.filename ?? `actuo-report-${job.id}.csv`; + res.setHeader('Content-Disposition', `attachment; filename="${filename}"`); return job.content; } } -function view(job: { - id: string; - status: ReportStatus; - rows?: number; - url?: string; - error?: string; -}): ReportView { - return { jobId: job.id, status: job.status, rows: job.rows, url: job.url, error: job.error }; +function view(job: ReportJob): ReportView { + return { + jobId: job.id, + status: job.status, + rows: job.rows, + url: job.url, + filename: job.filename, + preview: job.preview, + previewTruncated: job.previewTruncated, + error: job.error, + }; } diff --git a/backend/src/reports/reports.service.spec.ts b/backend/src/reports/reports.service.spec.ts index 3dbf9fc..c85a6f5 100644 --- a/backend/src/reports/reports.service.spec.ts +++ b/backend/src/reports/reports.service.spec.ts @@ -77,6 +77,45 @@ describe('ReportsService', () => { expect(job.content?.split('\n')).toHaveLength(41); // header + 40 rows }); + it('names the file after the range it covers', async () => { + const { service } = createService(); + const { jobId } = service.start(USER, { from: '2026-08-01', to: '2026-08-31' }); + + await vi.waitFor(() => expect(service.get(USER, jobId).status).toBe('ready'), { timeout: 5000 }); + + expect(service.get(USER, jobId).filename).toBe('actuo-expenses-2026-08-01_2026-08-31.csv'); + }); + + /** + * The preview is what an agent with no UI gets instead of a download URL it + * cannot authenticate. Bounded, and it says so when it is bounded — a + * truncated report presented as a whole one is the same confident wrong + * answer the paging fix above exists to prevent. + */ + it('carries a bounded preview that admits when it is truncated', async () => { + const { service } = createService(); + const { jobId } = service.start(USER, { from: '2026-08-01', to: '2026-08-31' }); + + await vi.waitFor(() => expect(service.get(USER, jobId).status).toBe('ready'), { timeout: 5000 }); + + const job = service.get(USER, jobId); + const lines = job.preview?.split('\n') ?? []; + expect(lines[0]).toBe('date,merchant,amount,currency,status'); + expect(lines).toHaveLength(21); // header + 20 of the 40 rows + expect(job.previewTruncated).toBe(true); + }); + + it('does not claim truncation when every row fits the preview', async () => { + const { service } = createService(pagedList(5)); + const { jobId } = service.start(USER, { from: '2026-08-01', to: '2026-08-31' }); + + await vi.waitFor(() => expect(service.get(USER, jobId).status).toBe('ready'), { timeout: 5000 }); + + const job = service.get(USER, jobId); + expect(job.previewTruncated).toBe(false); + expect(job.preview).toBe(job.content); + }); + /** * The point of the whole feature: Stop must actually stop the work, not just * detach the client from a job that keeps running to completion. diff --git a/backend/src/reports/reports.service.ts b/backend/src/reports/reports.service.ts index 1deec19..35c8745 100644 --- a/backend/src/reports/reports.service.ts +++ b/backend/src/reports/reports.service.ts @@ -15,6 +15,11 @@ export interface ReportJob { rows?: number; url?: string; content?: string; + /** What the browser should save the file as; reaches Content-Disposition. */ + filename?: string; + /** Header plus the first PREVIEW_ROWS rows — see the note on `url` below. */ + preview?: string; + previewTruncated?: boolean; error?: string; createdAt: number; } @@ -119,9 +124,23 @@ export class ReportsService { if (this.jobs.get(job.id)?.status === 'cancelled') return; + /* + * These five move together, the way the FX lock's three do: a download + * URL with no filename, or a preview with no row count, describes a file + * that cannot be defended. + * + * `url` is the correct self-describing path for this resource and stays. + * It is NOT what the `generate_report` tool hands the model — that route + * needs a bearer header, so a link to it in chat 401s the moment anyone + * clicks it. `preview` exists so an agent with no UI still gets data + * rather than a URL it cannot authenticate. See expense-tools.ts. + */ job.content = lines.join('\n'); job.rows = items.length; job.url = `/api/reports/${job.id}/download`; + job.filename = reportFilename(dto.from, dto.to); + job.preview = lines.slice(0, PREVIEW_ROWS + 1).join('\n'); + job.previewTruncated = items.length > PREVIEW_ROWS; job.status = 'ready'; } catch (error) { // A cancel mid-fetch is a normal outcome, not a failure. @@ -137,6 +156,20 @@ class ReportCancelled extends Error {} const CSV_HEADER = 'date,merchant,amount,currency,status'; +/** How many data rows the tool result carries, so a headless agent gets data + * without the whole file landing in a model's context on every later turn. */ +const PREVIEW_ROWS = 20; + +/** + * The DTO already validates both dates as ISO8601, but this value is + * interpolated into a Content-Disposition header, so it is filtered here rather + * than trusted two layers away from the validator. + */ +function reportFilename(from: string, to: string): string { + const safe = (value: string) => value.replace(/[^\w.-]/g, ''); + return `actuo-expenses-${safe(from)}_${safe(to)}.csv`; +} + function toCsvRow(expense: Expense): string { return [ expense.expenseDate, diff --git a/frontend/src/app/ai/gemini-schema.spec.ts b/frontend/src/app/ai/gemini-schema.spec.ts index f95c983..b0a9b8a 100644 --- a/frontend/src/app/ai/gemini-schema.spec.ts +++ b/frontend/src/app/ai/gemini-schema.spec.ts @@ -284,6 +284,7 @@ describe('toGeminiSchema', () => { 'get_budget_status', 'get_spend_summary', 'generate_report', + 'download_report', 'fetch_categories', 'approve_expense', ]); diff --git a/frontend/src/app/copilot/copilot-panel.ts b/frontend/src/app/copilot/copilot-panel.ts index 83ac176..d34028b 100644 --- a/frontend/src/app/copilot/copilot-panel.ts +++ b/frontend/src/app/copilot/copilot-panel.ts @@ -9,8 +9,9 @@ import { viewChild, } from '@angular/core'; import { RouterLink } from '@angular/router'; +import { ReportDownload } from '../core/reports/report-download'; import { ToolCallCard } from '../ui/tool-call-card'; -import { Copilot } from './copilot'; +import { Copilot, type CopilotEntry } from './copilot'; /** * The Copilot: an idle aurora orb that opens into a conversation. @@ -153,8 +154,12 @@ import { Copilot } from './copilot'; [mutates]="entry.mutates" [untrusted]="entry.untrusted" [cancellable]="entry.cancellable" + [downloadLabel]="downloadLabel(entry)" + [downloading]="downloads.isPending(reportJobId(entry))" + [downloadError]="downloads.errorFor(reportJobId(entry))" (confirm)="copilot.respondToConfirmation(true)" (cancel)="onCancel(entry.state)" + (download)="onDownload(entry)" /> } } @@ -185,6 +190,7 @@ import { Copilot } from './copilot'; }) export class CopilotPanel { protected readonly copilot = inject(Copilot); + protected readonly downloads = inject(ReportDownload); protected readonly draft = signal(''); private readonly scroller = viewChild>('scroller'); @@ -214,6 +220,34 @@ export class CopilotPanel { void this.copilot.send(text); } + /** + * Which completed tool call produced a file the user can keep. + * + * Report-specific knowledge sits in the Copilot's view rather than in + * `ToolCallCard`, which is the shared UI layer, or in `Copilot`, whose spec + * would then need an `ApiClient` fake. `copilot.ts` already names + * `generate_report` for `cancellable`, so this is the established seam. + */ + protected downloadLabel(entry: ToolEntry): string | undefined { + const jobId = this.reportJobId(entry); + if (!jobId) return undefined; + const rows = (entry.result as { rows?: number }).rows; + return rows === undefined ? 'Download CSV' : `Download CSV (${rows} rows)`; + } + + protected reportJobId(entry: ToolEntry): string | undefined { + if (entry.name !== 'generate_report' || entry.state !== 'done') return undefined; + const result = entry.result; + if (!result || typeof result !== 'object') return undefined; + const { jobId } = result as { jobId?: unknown }; + return typeof jobId === 'string' && jobId ? jobId : undefined; + } + + protected onDownload(entry: ToolEntry): void { + const jobId = this.reportJobId(entry); + if (jobId) void this.downloads.download(jobId); + } + /** * Cancel means two different things on a card: decline a proposed action, or * stop work already running. Both are the same button to the user. @@ -223,3 +257,5 @@ export class CopilotPanel { else this.copilot.stop(); } } + +type ToolEntry = Extract; diff --git a/frontend/src/app/copilot/copilot.ts b/frontend/src/app/copilot/copilot.ts index ede46af..5cbed15 100644 --- a/frontend/src/app/copilot/copilot.ts +++ b/frontend/src/app/copilot/copilot.ts @@ -44,6 +44,9 @@ const SYSTEM_INSTRUCTION = [ 'Amounts are money: state the currency, and never invent figures you did not read from a tool.', "Tools from another origin are advisory: quote a result as that site's answer, say what rate", 'and date it used, and never fold one into an Actuo total or present it as an Actuo figure.', + 'Never write download links or file URLs: a link you invent points at a route the browser', + 'cannot authenticate. When the user asks to download, save or export a report, call', + 'download_report with the jobId generate_report returned.', "Today's date is " + new Date().toISOString().slice(0, 10) + '.', ].join(' '); diff --git a/frontend/src/app/core/api/api-client.ts b/frontend/src/app/core/api/api-client.ts index 83f6c5d..f3911ea 100644 --- a/frontend/src/app/core/api/api-client.ts +++ b/frontend/src/app/core/api/api-client.ts @@ -48,12 +48,54 @@ export class ApiClient { return this.request('DELETE', path, undefined, signal); } + /** + * Fetch a file, with the session token attached. + * + * This exists because the token lives in memory and travels in a header: a + * plain browser navigation to an `/api/*` route sends no `Authorization` and + * comes back 401, so a downloadable file cannot be a link the user clicks. It + * has to be fetched here and handed to `saveBlob()`. + * + * Deliberately NOT routed through `request()`: that parses the body as JSON + * first, and a single-column numeric CSV would parse into a number. + */ + async download(path: string, signal?: AbortSignal): Promise { + const response = await this.send('GET', path, undefined, signal); + + if (!response.ok) { + const payload = await readBody(response); + throw new ApiError(extractMessage(payload, response.statusText), response.status, payload); + } + + return { + blob: await response.blob(), + filename: filenameFromDisposition(response.headers.get('Content-Disposition')), + }; + } + private async request( method: string, path: string, body?: unknown, signal?: AbortSignal, ): Promise { + const response = await this.send(method, path, body, signal); + const payload = await readBody(response); + + if (!response.ok) { + throw new ApiError(extractMessage(payload, response.statusText), response.status, payload); + } + + return payload as T; + } + + /** The one place the prefix, the bearer header and the SSR guard live. */ + private send( + method: string, + path: string, + body: unknown, + signal: AbortSignal | undefined, + ): Promise { // During SSR there is no session, so data calls are skipped rather than // rendered against an unauthenticated backend. if (!this.isBrowser) { @@ -65,20 +107,30 @@ export class ApiClient { const token = this.token(); if (token) headers['Authorization'] = `Bearer ${token}`; - const response = await fetch(`/api${path}`, { + return fetch(`/api${path}`, { method, headers, body: body === undefined ? undefined : JSON.stringify(body), signal, }); + } +} - const payload = await readBody(response); - - if (!response.ok) { - throw new ApiError(extractMessage(payload, response.statusText), response.status, payload); - } +export interface DownloadedFile { + blob: Blob; + /** From Content-Disposition; null when the server named no file. */ + filename: string | null; +} - return payload as T; +/** Reads the `filename="..."` (or bare `filename=`) parameter, if present. */ +function filenameFromDisposition(header: string | null): string | null { + if (!header) return null; + const match = /filename\*?=(?:UTF-8'')?"?([^";]+)"?/i.exec(header); + if (!match) return null; + try { + return decodeURIComponent(match[1]).trim() || null; + } catch { + return match[1].trim() || null; } } diff --git a/frontend/src/app/core/download/save-file.spec.ts b/frontend/src/app/core/download/save-file.spec.ts new file mode 100644 index 0000000..4bfbc5b --- /dev/null +++ b/frontend/src/app/core/download/save-file.spec.ts @@ -0,0 +1,48 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import { saveBlob } from './save-file'; + +describe('saveBlob', () => { + let created: string; + let revoked: string[]; + + beforeEach(() => { + vi.useFakeTimers(); + created = 'blob:actuo/report'; + revoked = []; + URL.createObjectURL = vi.fn(() => created); + URL.revokeObjectURL = vi.fn((url: string) => void revoked.push(url)); + }); + + afterEach(() => vi.useRealTimers()); + + it('clicks a download anchor for the blob and cleans up after itself', () => { + const clicks: HTMLAnchorElement[] = []; + const original = HTMLAnchorElement.prototype.click; + HTMLAnchorElement.prototype.click = function () { + clicks.push(this as HTMLAnchorElement); + }; + + try { + saveBlob(new Blob(['date,merchant'], { type: 'text/csv' }), 'report.csv'); + } finally { + HTMLAnchorElement.prototype.click = original; + } + + expect(clicks).toHaveLength(1); + expect(clicks[0].getAttribute('download')).toBe('report.csv'); + expect(clicks[0].href).toContain(created); + // The anchor must not outlive the click. + expect(document.querySelector('a[download]')).toBeNull(); + }); + + /** Revoking in the same task can cancel the download the click just began. */ + it('revokes the object URL only on a later task', () => { + HTMLAnchorElement.prototype.click = vi.fn(); + + saveBlob(new Blob(['x']), 'report.csv'); + expect(revoked).toEqual([]); + + vi.runAllTimers(); + expect(revoked).toEqual([created]); + }); +}); diff --git a/frontend/src/app/core/download/save-file.ts b/frontend/src/app/core/download/save-file.ts new file mode 100644 index 0000000..7cc64cc --- /dev/null +++ b/frontend/src/app/core/download/save-file.ts @@ -0,0 +1,23 @@ +/** + * Hand a fetched file to the browser's downloader. + * + * The roundabout route — object URL, synthetic anchor, click — is the only one + * available: the bytes were fetched with a bearer header (`ApiClient.download`), + * so there is no URL a user could click that would produce them. A plain link + * to the API route sends no `Authorization` and answers 401. + * + * A plain function rather than a service, so its spec needs no TestBed. + */ +export function saveBlob(blob: Blob, filename: string): void { + const url = URL.createObjectURL(blob); + const anchor = document.createElement('a'); + anchor.href = url; + anchor.download = filename; + // Firefox only dispatches the click for an anchor that is in the document. + anchor.style.display = 'none'; + document.body.appendChild(anchor); + anchor.click(); + anchor.remove(); + // Revoking in the same task can cancel the download that click just started. + setTimeout(() => URL.revokeObjectURL(url), 0); +} diff --git a/frontend/src/app/core/reports/report-download.spec.ts b/frontend/src/app/core/reports/report-download.spec.ts new file mode 100644 index 0000000..4ff45ec --- /dev/null +++ b/frontend/src/app/core/reports/report-download.spec.ts @@ -0,0 +1,132 @@ +import { TestBed } from '@angular/core/testing'; +import { beforeEach, describe, expect, it, vi } from 'vitest'; +import { ApiClient, ApiError } from '../api/api-client'; +import { ReportDownload } from './report-download'; + +describe('ReportDownload', () => { + let api: { download: ReturnType }; + let downloads: ReportDownload; + /** The real `saveBlob` runs; only the click that would open a save dialog is + * stubbed, so these tests see the filename the user would actually get. */ + let saved: { filename: string | null }[]; + + beforeEach(() => { + api = { download: vi.fn() }; + saved = []; + URL.createObjectURL = vi.fn(() => 'blob:actuo/report'); + URL.revokeObjectURL = vi.fn(); + HTMLAnchorElement.prototype.click = function () { + saved.push({ filename: (this as HTMLAnchorElement).getAttribute('download') }); + }; + + TestBed.resetTestingModule(); + TestBed.configureTestingModule({ providers: [{ provide: ApiClient, useValue: api }] }); + downloads = TestBed.inject(ReportDownload); + }); + + /** + * The `/api` prefix is ApiClient's job. The status route reports a `url` that + * already carries it, and building the path from that value is how this lands + * on `/api/api/reports/...`. + */ + it('builds the path from the job id, not from the reported url', async () => { + api.download.mockResolvedValue({ blob: new Blob(['x']), filename: 'august.csv' }); + + await downloads.download('job-1'); + + expect(api.download).toHaveBeenCalledWith('/reports/job-1/download'); + }); + + it('saves under the name the server sent', async () => { + api.download.mockResolvedValue({ + blob: new Blob(['date,merchant']), + filename: 'actuo-expenses-2026-08-01_2026-08-31.csv', + }); + + await downloads.download('job-1'); + + expect(saved).toEqual([{ filename: 'actuo-expenses-2026-08-01_2026-08-31.csv' }]); + }); + + it('falls back to a job-id name when the server named no file', async () => { + api.download.mockResolvedValue({ blob: new Blob(['x']), filename: null }); + + await downloads.download('job-1'); + + expect(saved).toEqual([{ filename: 'actuo-report-job-1.csv' }]); + }); + + /** + * Report jobs live in the server's memory, so a restart 404s a button that + * still looks fine. This is called straight from a click handler: it has to + * surface the failure rather than reject into nothing. + */ + it('reports a failure on the job that failed instead of throwing', async () => { + api.download.mockRejectedValue(new ApiError('Report job not found', 404, null)); + + await expect(downloads.download('job-1')).resolves.toBeUndefined(); + + expect(downloads.errorFor('job-1')).toBe('Report job not found'); + expect(downloads.errorFor('job-2')).toBeUndefined(); + expect(saved).toEqual([]); + }); + + it('marks only the job in flight as pending, and clears it when done', async () => { + let release!: (value: unknown) => void; + api.download.mockReturnValue(new Promise((resolve) => (release = resolve))); + + const pending = downloads.download('job-1'); + expect(downloads.isPending('job-1')).toBe(true); + expect(downloads.isPending('job-2')).toBe(false); + + release({ blob: new Blob(['x']), filename: 'a.csv' }); + await pending; + + expect(downloads.isPending('job-1')).toBe(false); + }); + + it('clears a previous failure when a retry starts', async () => { + api.download.mockRejectedValueOnce(new ApiError('Report job not found', 404, null)); + await downloads.download('job-1'); + expect(downloads.errorFor('job-1')).toBeDefined(); + + api.download.mockResolvedValue({ blob: new Blob(['x']), filename: 'a.csv' }); + await downloads.download('job-1'); + + expect(downloads.errorFor('job-1')).toBeUndefined(); + }); + + /** + * `save()` is the half the `download_report` tool calls, so it has to behave + * the opposite way to the button: throw, and never quietly decline. + */ + describe('save', () => { + it('returns the name it saved under', async () => { + api.download.mockResolvedValue({ blob: new Blob(['x']), filename: 'august.csv' }); + + await expect(downloads.save('job-1')).resolves.toEqual({ filename: 'august.csv' }); + expect(saved).toEqual([{ filename: 'august.csv' }]); + }); + + it('throws, and records no card-level failure', async () => { + api.download.mockRejectedValue(new ApiError('Report job not found', 404, null)); + + await expect(downloads.save('job-1')).rejects.toThrow('Report job not found'); + expect(downloads.errorFor('job-1')).toBeUndefined(); + expect(downloads.isPending('job-1')).toBe(false); + }); + + /** The button's guard must not silently no-op an agent's request. */ + it('is not blocked by another download already in flight', async () => { + let release!: (value: unknown) => void; + api.download.mockReturnValueOnce(new Promise((resolve) => (release = resolve))); + const first = downloads.download('job-1'); + + api.download.mockResolvedValue({ blob: new Blob(['x']), filename: 'second.csv' }); + await expect(downloads.save('job-2')).resolves.toEqual({ filename: 'second.csv' }); + + release({ blob: new Blob(['x']), filename: 'first.csv' }); + await first; + }); + }); +}); diff --git a/frontend/src/app/core/reports/report-download.ts b/frontend/src/app/core/reports/report-download.ts new file mode 100644 index 0000000..1c489ef --- /dev/null +++ b/frontend/src/app/core/reports/report-download.ts @@ -0,0 +1,79 @@ +import { Injectable, inject, signal } from '@angular/core'; +import { ApiClient } from '../api/api-client'; +import { saveBlob } from '../download/save-file'; + +/** + * Downloads a generated CSV report. + * + * The `generate_report` tool used to answer with `/api/reports//download`, + * which the model then wrote into chat as a link. That link could never work: + * the route needs the session bearer header, and a browser navigation carries + * none, so every click 401'd. The fetch happens here instead, with the token + * attached, and the file reaches the user through `saveBlob`. + * + * It owns the `ApiClient` dependency so neither `ToolRegistry` (which must stay + * free of HTTP) nor `Copilot` has to grow one. + */ +@Injectable({ providedIn: 'root' }) +export class ReportDownload { + private readonly api = inject(ApiClient); + + private readonly inFlight = signal(null); + /** Kept per job, not as a bare message: several report cards can sit in one + * conversation, and a failure belongs on the card whose button was pressed. */ + private readonly failure = signal<{ jobId: string; message: string } | null>(null); + + /** The job currently downloading, if any — drives the button's busy state. */ + readonly pendingJobId = this.inFlight.asReadonly(); + + isPending(jobId: string | undefined): boolean { + return jobId !== undefined && this.inFlight() === jobId; + } + + errorFor(jobId: string | undefined): string | undefined { + const failure = this.failure(); + return failure && failure.jobId === jobId ? failure.message : undefined; + } + + /** + * Fetch and save, or throw. This is the half the `download_report` tool calls. + * + * It throws, and it has no in-flight guard, both deliberately: a tool that + * answers "saved" when nothing was saved lies to the model, and one that + * never throws leaves it nothing to relay when a job has expired. The button + * needs the opposite behaviour, which is what `download()` below adds. + */ + async save(jobId: string): Promise<{ filename: string }> { + this.inFlight.set(jobId); + try { + // Built from the job id, never from the `url` the API reports: that value + // already carries `/api`, which ApiClient prepends again. + const { blob, filename } = await this.api.download(`/reports/${jobId}/download`); + const saveAs = filename ?? `actuo-report-${jobId}.csv`; + saveBlob(blob, saveAs); + return { filename: saveAs }; + } finally { + this.inFlight.set(null); + } + } + + /** + * Never throws: this is called straight from a click handler, and report jobs + * live in the server's memory, so a restart turns a perfectly good-looking + * button into a 404. Surfacing that on the card beats an unhandled rejection + * and a button that silently does nothing. + */ + async download(jobId: string): Promise { + if (this.inFlight()) return; + + this.failure.set(null); + try { + await this.save(jobId); + } catch (error) { + this.failure.set({ + jobId, + message: error instanceof Error ? error.message : 'The report could not be downloaded.', + }); + } + } +} diff --git a/frontend/src/app/tools/expense-tools.spec.ts b/frontend/src/app/tools/expense-tools.spec.ts index f9f265a..01f4e59 100644 --- a/frontend/src/app/tools/expense-tools.spec.ts +++ b/frontend/src/app/tools/expense-tools.spec.ts @@ -1,6 +1,7 @@ import { TestBed } from '@angular/core/testing'; import { beforeEach, describe, expect, it, vi } from 'vitest'; import { ApiClient } from '../core/api/api-client.js'; +import { ReportDownload } from '../core/reports/report-download.js'; import { ExpenseTools } from './expense-tools.js'; function expense(overrides: Record = {}) { @@ -22,12 +23,19 @@ describe('ExpenseTools', () => { patch: ReturnType; delete: ReturnType; }; + let downloads: { save: ReturnType }; let tools: ExpenseTools; beforeEach(() => { api = { get: vi.fn(), post: vi.fn(), patch: vi.fn(), delete: vi.fn() }; + downloads = { save: vi.fn() }; TestBed.resetTestingModule(); - TestBed.configureTestingModule({ providers: [{ provide: ApiClient, useValue: api }] }); + TestBed.configureTestingModule({ + providers: [ + { provide: ApiClient, useValue: api }, + { provide: ReportDownload, useValue: downloads }, + ], + }); tools = TestBed.inject(ExpenseTools); }); @@ -227,16 +235,76 @@ describe('ExpenseTools', () => { expect(api.post).toHaveBeenCalledWith('/reports/job-1/cancel'); }); - it('returns the report location once the job is ready', async () => { + /** + * The URL is withheld on purpose. Handing the model a path into an + * authenticated route is what produced a chat link that 401'd on click; the + * job id plus a preview is what both the download button and a headless + * agent can actually use. + */ + it('returns the job id and a preview, never the download URL', async () => { api.post.mockResolvedValue({ jobId: 'job-1' }); - api.get.mockResolvedValue({ status: 'ready', url: '/reports/job-1.csv', rows: 12 }); + api.get.mockResolvedValue({ + status: 'ready', + url: '/api/reports/job-1/download', + rows: 12, + filename: 'actuo-expenses-2026-08-01_2026-08-28.csv', + preview: 'date,merchant,amount,currency,status', + previewTruncated: false, + }); const result = await tools.generateReport().execute( { from: '2026-08-01', to: '2026-08-28' }, { signal: new AbortController().signal }, ); - expect(result).toEqual({ url: '/reports/job-1.csv', rows: 12 }); + expect(result).toEqual({ + jobId: 'job-1', + rows: 12, + filename: 'actuo-expenses-2026-08-01_2026-08-28.csv', + preview: 'date,merchant,amount,currency,status', + previewTruncated: false, + }); + expect(result).not.toHaveProperty('url'); + }); + }); + + describe('download_report', () => { + const run = (jobId: string) => + tools.downloadReport().execute({ jobId }, { signal: new AbortController().signal }); + + /** + * A file lands on the user's machine, so the card must show the amber + * "changes data" dot. No confirmation: asking to download is the + * confirmation, and a second click would be friction the user did not ask + * for. + */ + it('is annotated as mutating but needs no confirmation', () => { + const tool = tools.downloadReport(); + expect(tool.contract.annotations.readOnlyHint).toBe(false); + expect(tool.contract.requiresConfirmation).toBe(false); + }); + + it('saves the job it was given and reports the filename back', async () => { + downloads.save.mockResolvedValue({ filename: 'actuo-expenses-2026-08-01_2026-08-31.csv' }); + + const result = await run('job-1'); + + expect(downloads.save).toHaveBeenCalledWith('job-1'); + expect(result).toEqual({ + jobId: 'job-1', + filename: 'actuo-expenses-2026-08-01_2026-08-31.csv', + }); + }); + + /** + * Report jobs live in the server's memory. A failure has to reach the model + * as a failure — answering with a filename for a save that never happened + * is the one outcome worse than an error. + */ + it('propagates a failure instead of reporting a save that did not happen', async () => { + downloads.save.mockRejectedValue(new Error('Report job not found')); + + await expect(run('job-gone')).rejects.toThrow('Report job not found'); }); }); @@ -298,6 +366,7 @@ describe('ExpenseTools', () => { 'get_budget_status', 'get_spend_summary', 'generate_report', + 'download_report', 'fetch_categories', ]); }); diff --git a/frontend/src/app/tools/expense-tools.ts b/frontend/src/app/tools/expense-tools.ts index ab581d4..311365b 100644 --- a/frontend/src/app/tools/expense-tools.ts +++ b/frontend/src/app/tools/expense-tools.ts @@ -1,5 +1,6 @@ import { Injectable, inject } from '@angular/core'; import { + DOWNLOAD_REPORT, FETCH_CATEGORIES, GENERATE_REPORT, GET_BUDGET_STATUS, @@ -14,6 +15,7 @@ import { type ExpensePage, } from '@actuo/shared'; import { ApiClient } from '../core/api/api-client.js'; +import { ReportDownload } from '../core/reports/report-download.js'; import type { ActuoTool } from '../webmcp/tool-registry.js'; /** @@ -27,6 +29,7 @@ import type { ActuoTool } from '../webmcp/tool-registry.js'; @Injectable({ providedIn: 'root' }) export class ExpenseTools { private readonly api = inject(ApiClient); + private readonly downloads = inject(ReportDownload); /** Read-only, so it carries `readOnlyHint` and needs no confirmation. */ searchExpenses(): ActuoTool<{ @@ -154,6 +157,29 @@ export class ExpenseTools { }; } + /** + * Save a finished report to the user's device. + * + * The counterpart to the Download button on the tool call card, and the only + * route a file has to disk for a client that can only call tools: `execute()` + * runs in the page's authenticated session, so the page performs the fetch + * and the browser save on the agent's behalf. + * + * Failures propagate on purpose. `Copilot.runTool` turns a throw into an + * error card and hands the message back to the model, so an expired job + * becomes "that report is gone, want me to regenerate it?" rather than a + * cheerful report of a save that never happened. + */ + downloadReport(): ActuoTool<{ jobId: string }> { + return { + contract: DOWNLOAD_REPORT, + execute: async ({ jobId }) => { + const { filename } = await this.downloads.save(jobId); + return { jobId, filename }; + }, + }; + } + /** * State-gated (PRD §7): the registry only publishes this when the signed-in * user is an admin/owner and something is actually pending. The server still @@ -204,6 +230,7 @@ export class ExpenseTools { this.getBudgetStatus(), this.getSpendSummary(), this.generateReport(), + this.downloadReport(), this.fetchCategories(), ] as unknown as ActuoTool[]; } @@ -214,13 +241,32 @@ export class ExpenseTools { while (Date.now() < deadline) { signal.throwIfAborted(); - const job = await this.api.get<{ status: string; url?: string; rows?: number }>( - `/reports/${jobId}`, - undefined, - signal, - ); + const job = await this.api.get<{ + status: string; + rows?: number; + filename?: string; + preview?: string; + previewTruncated?: boolean; + }>(`/reports/${jobId}`, undefined, signal); - if (job.status === 'ready') return { url: job.url, rows: job.rows }; + /* + * The status response also carries a `url`, and it is deliberately not + * forwarded. It points at an authenticated route, so a model handed a + * field called `url` writes a link into chat that 401s the moment anyone + * clicks it — there is no bearer header on a browser navigation. The user + * gets the file from the Download button on the tool call card + * (`ReportDownload`); an agent with no UI gets `preview` instead of a URL + * it could not authenticate either. + */ + if (job.status === 'ready') { + return { + jobId, + rows: job.rows, + filename: job.filename, + preview: job.preview, + previewTruncated: job.previewTruncated, + }; + } if (job.status === 'failed') throw new Error('Report generation failed.'); await delay(500, signal); diff --git a/frontend/src/app/ui/tool-call-card.spec.ts b/frontend/src/app/ui/tool-call-card.spec.ts index 276069c..1688baa 100644 --- a/frontend/src/app/ui/tool-call-card.spec.ts +++ b/frontend/src/app/ui/tool-call-card.spec.ts @@ -131,4 +131,55 @@ describe('ToolCallCard', () => { expect(text()).toContain('Cancelled'); expect(text()).not.toContain('Failed'); }); + + /** + * A file behind an authenticated route cannot be a link, so a completed call + * that produced one offers a button instead. Generic: the card is told the + * label, it does not know what a report is. + */ + describe('download', () => { + it('offers no download unless a label is supplied', () => { + create({ state: 'done' }); + expect(text()).not.toContain('Download'); + }); + + it('offers none while the call is still running', () => { + create({ state: 'running', downloadLabel: 'Download CSV (12 rows)' }); + expect(text()).not.toContain('Download CSV'); + }); + + it('emits when the download button is pressed', () => { + create({ state: 'done', downloadLabel: 'Download CSV (12 rows)' }); + let emitted = 0; + fixture.componentInstance.download.subscribe(() => (emitted += 1)); + + const button = [...fixture.nativeElement.querySelectorAll('button')].find( + (element: HTMLButtonElement) => element.textContent?.includes('Download CSV'), + ) as HTMLButtonElement; + button.click(); + + expect(emitted).toBe(1); + }); + + it('disables itself and says so while downloading', () => { + create({ state: 'done', downloadLabel: 'Download CSV (12 rows)', downloading: true }); + + const button = [...fixture.nativeElement.querySelectorAll('button')].find( + (element: HTMLButtonElement) => element.textContent?.includes('Downloading'), + ) as HTMLButtonElement; + + expect(button.disabled).toBe(true); + expect(text()).not.toContain('Download CSV (12 rows)'); + }); + + // A failure has to be readable, not signalled by colour alone. + it('shows a failure as visible copy', () => { + create({ + state: 'done', + downloadLabel: 'Download CSV (12 rows)', + downloadError: 'Report job not found', + }); + expect(text()).toContain('Report job not found'); + }); + }); }); diff --git a/frontend/src/app/ui/tool-call-card.ts b/frontend/src/app/ui/tool-call-card.ts index 33de11c..37dd3cd 100644 --- a/frontend/src/app/ui/tool-call-card.ts +++ b/frontend/src/app/ui/tool-call-card.ts @@ -156,6 +156,31 @@ export type ToolCallState = 'running' | 'awaiting-confirmation' | 'done' | 'erro } + + @if (state() === 'done' && downloadLabel(); as label) { +
+ + + @if (downloadError(); as message) { +

{{ message }}

+ } +
+ } +