Skip to content

refactor(llm): consolidate LLM integration boundaries and shared report math (PR #1916 follow-ups) #1917

Description

@steilerDev

Consolidated follow-up for the non-blocking findings raised by product-architect across both review rounds on PR #1916 (story #1901, AI-generated report content), plus two items from the product-owner review.

Nothing here is a functional defect — PR #1916 shipped correct behaviour with regression coverage. The theme is that the LLM integration has outgrown the naming and boundaries it was given when it only did auto-itemization, and that one shared derivation is duplicated across client and server.

Source: PR #1916 (architect review rounds 1 and 2, product-owner review rounds 1 and 2) · Story: #1901


Highest value first

M2 — Extract computeIncludedTotal into @cornerstone/shared

This is the highest-value item of the set and the reason this issue exists.

The included-total derivation (sum each included invoice's allocatedAmount, minus the allocatedPortion of every excluded budget line, rounded per invoice to the nearest cent) now exists in two independent implementations:

  • Client: client/src/lib/reportExclusions.ts (applyLineExclusions) + client/src/lib/reportContent/buildReportContent.ts
  • Server: server/src/services/reportContentGenerationService.ts

This duplication has already drifted once, in production-bound code. PR #1916's blocking finding was exactly that: the server rounded to whole currency units while the client rounded to cents, and the server's per-invoice amounts ignored line exclusions entirely while its total honoured them. Both were caught in review, but only because someone re-derived the arithmetic by hand.

Extract the rule into @cornerstone/shared as computeIncludedTotal(report, includedInvoiceIds, excludedLineIds) and have both sides import the single implementation, so the letter total and the report table total cannot disagree.

Note for whoever picks this up: the two implementations are currently equivalent but not identical — the server rounds every included invoice unconditionally, the client only rounds invoices actually affected by an exclusion. The architect re-derived parity in round 2 and confirmed they agree in production, because sourceReportService.ts already guarantees allocatedAmount is 2 dp upstream. The server's extra rounding is defensive-only. Preserve that defensiveness in the shared function rather than "simplifying" it away, and keep a test covering a >2 dp input.


Medium

M1 — llmEnabled / autoItemizeEnabled duplication has no deprecation path

AppConfigResponse now carries two always-equal required booleans. Adding the alias was the right direction; the missing half is retiring the old name — the duplication forced edits to eight-plus object literals in PR #1916 alone. getProvider() also still gates the report-content feature on config.autoItemizeEnabled, so a feature that has nothing to do with itemization is switched by a flag named after it.

  • Add @deprecated JSDoc to autoItemizeEnabled in shared/src/types/config.ts
  • Plan and land the removal of the old name
  • The "no third alias; any divergence between the two flags needs an ADR" rule is already recorded on the API Contract wiki page

M3 — Route returns a server-internal type instead of the shared contract type

POST /api/source-reports/generate-content returns GenerateReportContentLlmResult (from server/src/services/budgetExtraction/types.ts) rather than the shared GenerateReportContentResponse. The two are structurally identical today, so any future drift is silent. Sibling handlers in the same file bind the shared type explicitly (const response: MarkClaimedResponse = …) — do the same here so a contract change becomes a compile error.

M4 — BudgetExtractionProvider is now a general LLM gateway under a budget-extraction name

The interface carries three unrelated capabilities (extract, summarizeMerge, generateReportContent), and five unrelated server test suites (backupService, draftCleanupService, and three invoiceAutoItemize* suites) now stub all of them just to construct a double.

Consolidating on one gateway was the correct architectural call — the name and module path should follow. Rename budgetExtraction/ to llmGateway/, or split the interface per capability. Standalone refactor; deliberately kept out of PR #1916.


Low

L5 — Missing unit annotation on the LLM input types (root cause of the PR #1916 blocking bug)

server/src/services/budgetExtraction/types.ts:

  • GenerateReportContentLlmInvoice.amount
  • GenerateReportContentLlmInput.totalAmount

Neither documents its unit. That silence is precisely what let the service → prompt seam break (major-unit euros consumed as if they were cents, making every figure in a bank-facing cover letter 100× too small). The behaviour is now correct and pinned by tests, but the next field added to these interfaces inherits the same ambiguity.

Two comment lines: /** Major currency units, 2 dp — not cents. */

L1 — sourceId! non-null assertion

client/src/pages/ReportWizardPage/ReportWizardPage.tsx, in runAiGeneration. Narrow it via the existing early return instead: if (!report || !useCase || !sourceId) return;

L2 — Array.includes() inside four loops where the Set already exists

server/src/services/reportContentGenerationService.ts calls includedInvoiceIds.includes(...) in four separate loops, two lines after building a Set for exactly this purpose. Cheap at the maxItems: 200 bound, but use the Set.

L3 — Konstruktionsprojekt prompt copy — DONE, delivered by #1931 (PR #1944)

Removed from this issue's scope on 2026-08-02. Story #1931 rewrote REPORT_CONTENT_SYSTEM_PROMPT and buildReportContentUserPrompt wholesale and fixed this line as part of that rewrite: the inverted ternary is gone, both 'en' and 'de' now emit the single language-independent phrase German construction project as project-domain context (output language is carried solely by the Language: line and system-prompt rule 1), and Konstruktionsprojekt no longer appears anywhere — prompts.test.ts pins its absence as a regression guard. No Bauprojekt rename is needed, because no German noun remains in the prompt.

Nothing else in this issue is affected. The rest of #1917 (M1–M4, L1, L2, L5, and the KI glossary entry below) remains open and unchanged.

Glossary — add KI / KI-Unterstützung to client/src/i18n/glossary.json

Approved by product-owner. Both the translator (during the #1901 translation pass) and the product-owner (review round 1) independently flagged this, and the architect asked for it to be settled in round 2.

The term is now user-visible in two places, so it needs to be pinned rather than left to per-story judgement:

Decision: AIKI (and the compound AI assistanceKI-Unterstützung) is approved as a glossary term. KI is the standard German rendering and the de copy already uses it consistently — the entry codifies existing practice rather than changing anything. No string changes should result from this; if implementing it surfaces a German string still using "AI", that is a bug to fix, not a sign the entry is wrong.

translator implements next cycle. The glossary was deliberately not edited as part of this decision.


Explicitly out of scope

Suggested sequencing

M2 first (it closes a proven drift risk), then L5 and M3 (cheap, both make a future seam break a compile-or-review error), then M1, then M4 as a standalone refactor. L1/L2 and the glossary entry can ride along with any of the above.

Metadata

Metadata

Assignees

No one assigned

    Projects

    Status
    Done

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions