Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 50 additions & 0 deletions .claude/agent-memory/product-architect/recurring-patterns.md
Original file line number Diff line number Diff line change
Expand Up @@ -185,3 +185,53 @@ Fixed in `b70d821b` (round 2 of the #1916 review); the permanent guard is the
`amount formatting (major units — regression guard for the ×100 division bug)` describe block in
`server/src/services/budgetExtraction/prompts.test.ts`, which asserts rendered substrings **and** negative
assertions against the divided form. Copy that shape for any new prompt builder.

---

## "Single source of truth" refactors must sweep the wiki, not just the code (#1931 / PR #1944)

When a story collapses a duplicated constant into one definition, the code-level sweep is the easy half.
The restatement that survives is almost always **prose in `wiki/API-Contract.md`** — and it is the most
consumer-visible one, so it is the one that must be fixed.

#1931 unified the AI report-content caps into `server/src/services/budgetExtraction/contentLimits.ts`
(`letterSubject` 150 / `letterBody` 2000 / `description` 200). The implementer found and collapsed a third
runtime site the spec had not enumerated (the `buildReportContentUserPrompt` trailing reminder). But
`API-Contract.md` still stated the removed 200/3000/300 tier in the response table **and** carried a Notes
bullet describing the two-tier divergence as *deliberate design* — worse than a stale number, because it
invites reintroduction.

**Sweep checklist for any "one definition" story:**

1. Runtime consumers (the obvious ones).
2. The **LLM structured-output schema** (`providerProfiles.ts`) and the **Fastify route schema** — both are
plausible hiding places for a duplicated `maxLength`. Both were clean here; check anyway.
3. `wiki/API-Contract.md` response tables **and** Notes bullets. Fix in the same PR — the submodule ref must
be committed on the feature branch.
4. Tests that assert bare literals rather than interpolating the constant (right only by coincidence).
5. Agent-memory prose in other agents' files (flag to the owner; don't edit).

**Preferred wiki fix shape:** state the numbers once, then describe the *guarantee* and point at the source
file ("both derive from `REPORT_CONTENT_LIMITS` in …"), so the page stops being an independent restatement.

**Trap:** `API-Contract.md` L3806's `truncated (500/300 chars)` is *prompt-input* truncation from
`reportContentGenerationService.ts`, numerically colliding with the old output cap and sitting a few lines
from the wrong ones. Do not "fix" it.

## Test smell: whole-prompt substring assertions with `|` alternations are toothless

Guarding an untyped system prompt with `expect(prompt.toLowerCase()).toMatch(/a|b|c/)` reliably passes on
**unrelated pre-existing text elsewhere in the same prompt**, so it does not detect the erosion it exists to
prevent. Two live examples from #1931's new guards:

- `/purpose|role/` passes on rule 4's "the report's purpose (budget overview, claim, …)" even if rule 2's
purpose instruction is deleted entirely.
- `/vendor|invoice number|date|amount/` passes on rule 7's "vendor names" even if the whole
"Do NOT restate the vendor name, invoice number, date, or amount" clause is deleted.

**Rule:** assert the distinctive full clause with `toContain`, the way `contentLimits.test.ts` does
(`toContain(\`Maximum ${LIMITS.description} characters per description.\`)`). Composing the prompt from named
constant blocks is over-engineering at ~15 lines — tight assertions buy the same protection far cheaper.
Also check that **every** constraint the AC enumerates has its own guard: #1931's AC 3.5 listed five, and
"never invent or alter amounts or dates" had none (the only `/invent/` assertion in the file targeted
`MERGE_SYSTEM_PROMPT`) — the one instruction protecting the single number the model still emits.
50 changes: 50 additions & 0 deletions .claude/agent-memory/product-architect/story-reviews.md
Original file line number Diff line number Diff line change
Expand Up @@ -374,3 +374,53 @@ Findings posted: MEDIUM (pre-existing, follow-up) `ReportWizardPage.handleUseCas
a report fetched under the *previous* use case — the tier rule is right, the wizard just holds output
from the wrong invocation. LOW: `wiki/API-Contract.md:3625` still says "Document stage" four lines
above the tier tables that retire that word.

---

## PR #1944 — #1931 "Enhance with AI" single action + purpose-focused prompt — CHANGES_REQUIRED

Removed the step-4 `aiEnabled` opt-in (step 5 now gated on `llmEnabled` alone), rewrote
`REPORT_CONTENT_SYSTEM_PROMPT` to ask *why* a cost was incurred, and unified the length caps into
`server/src/services/budgetExtraction/contentLimits.ts`. Fixed an inverted language ternary that emitted
"German construction project" for `en` and "Konstruktionsprojekt" for `de` (wrong in both branches) — the
domain phrase is now fixed literal text for both, with a `not.toContain('Konstruktionsprojekt')` regression
guard. Good instinct; copy that negative-assertion shape.

**Blocked on:** `wiki/API-Contract.md` L3795–97/L3830 still documenting the removed 200/3000/300 tier as
deliberate. See the "single source of truth" entry in [recurring-patterns.md](recurring-patterns.md) for the
sweep checklist and the trap at L3806.

**Rulings worth reusing:**

1. **Server-local constants beat `@cornerstone/shared` when the constant is not on the wire.** These caps
govern the *model's* output; the response carries already-truncated strings and the client neither
validates nor re-enforces them. Promoting them would invite a UI `maxLength` that the PO explicitly
rejected — the step-5 fields stay user-editable after generation, and a hand-typed 400-char description
is legal. Extends the #1930 rule: **not "no second consumer yet" but "a client consumer would be a
contract change, not an extension."**
2. **"Structurally impossible to disagree" holds only for the runtime path.** Verified clean here:
`providerProfiles.ts`'s `REPORT_CONTENT_SCHEMA` sends bare `{type:'string'}` with no `maxLength`, the
Fastify route schema bounds request fields only, `shared/src/types/sourceReport.ts` has no zod, and the
client editor has zero `maxLength`. Structural ends where TypeScript ends — wiki prose always needs a
manual sweep.
3. **Removing a UI opt-in in front of an already-configured capability has ~zero privacy/cost delta.** The
consent gate is operator-level (`LLM_*` env → `config.llmEnabled`), and the same gateway already ships
more data via auto-itemization. What *is* lost is the visible pre-click warning: replacing a checkbox
helper with an `srOnly` + `aria-describedby` span leaves sighted users with no warning until the
overwrite-confirm modal, which only fires when `overrides` is non-empty. Asymmetry in the wrong
direction — prefer a visible muted helper line. (Flagged to ux-designer, not blocking.)
4. **#1916 numeric guards survived** — `prompts.ts` L152/L166 `.toFixed(2)` are context lines, the
`amount formatting (major units …)` describe block is unmodified, and `reportContentGenerationService.ts`
is untouched. Risk direction is *lower*: rule 2 now forbids emitting amounts in descriptions, so the only
number left in the output is the letter-body total.

Findings: HIGH wiki caps drift · MEDIUM toothless prompt alternation guards · MEDIUM no guard for AC 3.5's
"never invent or alter amounts or dates" · LOW `prompts.test.ts` L596–598 bare `/150 char/` literals ·
LOW srOnly-only overwrite warning · INFO stale "300 validator cap" in product-owner memory · INFO
`reachStep5WithAiConfigured` has an implicit `mockLlmEnabled` precondition.

E2E rewrite (unexecuted — Chromium download blocked in sandbox) reads correct: sr-only span is a *sibling*
of the button so the accessible name is unaffected; `toBeAttached()` (not `toBeVisible()`) is right for
`.srOnly` (`1px` + `clip-path: inset(50%)` makes Playwright's visibility heuristic ambiguous); expected
literal is byte-identical to `en/budget.json`; `#enhanceWithAiDescription` and `aiGenerateRow` each have
exactly one render site, so no strict-mode risk from the page's desktop/mobile dual DOM tree.
Loading
Loading