From bd96b4f0b45e57333dde40973ff38083a156f037 Mon Sep 17 00:00:00 2001 From: santidev21 Date: Wed, 30 Sep 2026 20:00:51 -0500 Subject: [PATCH] docs(agents): trim AGENTS.md and relocate detail to docs/ - Move the full 'Gotchas discovered the hard way' list to docs/specs/gotchas.md; AGENTS.md keeps a one-line-per-group summary. - Move the phase status table and end-to-end verification notes to docs/HANDOFF.md ('Project status'); AGENTS.md keeps a pointer. - Update the Documentation Policy and repository layout for docs/specs/. - Point the TECHNICAL-DESIGN risk table at docs/specs/gotchas.md instead of AGENTS.md. AGENTS.md: 242 -> 149 lines. No information removed, only relocated. --- AGENTS.md | 153 ++++++++------------------------------- docs/HANDOFF.md | 30 +++++++- docs/TECHNICAL-DESIGN.md | 2 +- docs/specs/gotchas.md | 100 +++++++++++++++++++++++++ 4 files changed, 159 insertions(+), 126 deletions(-) create mode 100644 docs/specs/gotchas.md diff --git a/AGENTS.md b/AGENTS.md index 82b4b6b..d9cf572 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -55,8 +55,9 @@ scripts/install-backup-cron.sh # installs the nightly and weekly cron entries docs/TECHNICAL-DESIGN.md docs/BACKUPS.md # backup, verification and restore runbook docs/DEPLOYMENT.md # deploy, rollback and operations -docs/HANDOFF.md # next task prompt and backlog +docs/HANDOFF.md # status, next task prompt and backlog docs/adr/ # one file per irreversible decision +docs/specs/ # focused specs (implementation gotchas, ...) ``` ## Documentation Policy @@ -64,8 +65,8 @@ docs/adr/ # one file per irreversible decision Docs capture decisions and current state, never session narration. - **Allowed:** this file, `README`, `docs/TECHNICAL-DESIGN.md`, `docs/adr/NNN-*.md` (one decision: - context, options, decision, consequences), the runbooks (`BACKUPS`, `DEPLOYMENT`) and - `docs/HANDOFF.md`. + context, options, decision, consequences), `docs/specs/*.md` (focused specs such as + `gotchas.md`), the runbooks (`BACKUPS`, `DEPLOYMENT`) and `docs/HANDOFF.md`. - **Forbidden:** phase reports, progress logs, "what I did" narration and per-session summaries. When a change needs a durable record, update the design doc or add an ADR — do not create a report file. This applies to AI output too. @@ -109,129 +110,35 @@ production. ## Gotchas discovered the hard way -**EF Core and keys** - -- Aggregate-created records need store-generated keys (`Entity(keyGeneratedByStore: true)` + - `ValueGeneratedOnAdd()`): EF treats a client-assigned key on a child found in a navigation as - an existing row and fails as a concurrency conflict. Such an unsaved child has `Guid.Empty`, - so never identify it by id (`RemoveAlias` takes the instance). -- Removing a child from a tracked aggregate relies on EF orphan deletion (the non-nullable FK - convention); `OrphanRemovalTests` pins it. -- Do not make the `NO ACTION` composite FKs `DEFERRABLE`: EF's autocommit save would report a - concurrency failure instead of a named foreign key violation. -- EF cannot order an insert by a composite FK it does not model, so an operational row - referencing a category (`budget_alerts`) must be written after the category exists. -- `ExecuteDeleteAsync` bypasses the change tracker; bulk deletes run from their own scope. - -**Configuration and hosting** - -- Configuration binding does not turn a single value into an array: `AllowedUserIds` is a - `string` with explicit parsing. A blank value counts as unset, so `.env` can fill it. -- Hosted services are singletons; resolve scoped services from a scope per iteration. Only a - test that builds the real host catches a mistake (`ApiTelegramWiringTests`). -- `Options.Create` is ambiguous inside files importing `MyBudget.Telegram.Options`; qualify it. -- The runtime image is Debian for ICU and tzdata; `external: false` in the local compose overlay - is required; both `app` and `migrator` keep `image: mybudget-app`. -- Docker cannot publish a host port for a container whose only network is internal. - -**Text, resources and payloads** - -- Spanish is the neutral resource set; never name it `Messages.es.resx`, and add every key to - `MessageKeys` or the catalog test fails the build. -- XML comments must not contain `--`. -- Conversation payloads are `jsonb`: compare as data, never strings. A payload that cannot be - read is treated as empty. A computed payload property must be `[JsonIgnore]`. -- Callback data is capped at 64 bytes; identify a row by position or term when the id does not - fit. A menu tap outranks the active conversation, and some callbacks (Undo) arrive after the - flow is gone, handled as global callbacks. -- A confirmation is claimed once with a conditional `UPDATE`, never re-read; the callback - carries only the pending id. An empty listing must still carry the notices it was built with. - -**Dates, money and reporting** - -- `IUserLocalDate` is the only UTC-to-local conversion; an unusable stored zone falls back to - UTC. `ExpenseDate` is a `DateOnly`; report sums are derived in SQL, never stored. -- The matcher's 0.20 partial-overlap floor and epsilon comparison are load-bearing; signals are - additive per query/keyword and the category takes its best term. -- A budget-alert marker is recorded whether or not the notification is delivered: the marker - stops the bot repeating itself. - -**Operations** - -- The EF migration history table is mixed case: quote `"__EFMigrationsHistory"` in raw SQL. -- `pg_restore` exits 0 unless `--exit-on-error`; the drill must actually restore, not just list. -- Pre-deploy and nightly dumps share the backup volume. - -**Recurring and charts** - -- A recurring rule is configuration: `last_generated_date` and the generated expense commit in - the same transaction, which is the whole idempotency argument. Month-end clamps to the last - day. The scheduler is registered only with a bot token, first pass delayed one minute. -- The monthly closing is claimed, not read: `monthly_closings` is inserted with `ON CONFLICT DO - NOTHING` on `(user_id, year, month)`, before the message, so a restart cannot repeat it. -- The closing fires on the last local day from 23:59 and falls back to the first local day; both - resolve to the same `closedPeriod`, which is what keeps the marker exactly-once. The - copy-budget button was removed when budgets became recurring. -- A closing with no spending and no allocation is skipped and its marker stays unspent: a - notification feature that talks about nothing is a notification feature that gets muted. -- The daily reminder and the closing share `ScheduledNotificationsScheduler` (every minute, - only with a bot token). The reminder is skipped before the user's local 21:00 and when an - expense already exists that local day; it claims `("daily", local day)` in - `reminder_deliveries` before sending, so frequent ticks are safe. -- Adding a menu section touches `MainMenu.ActionKeys`, the keyboard rows, the menu test and the - router mapping. -- Charts are hand-drawn (`RgbCanvas` + 5x7 bitmap font + PNG over `ZLibStream`) to avoid native - dependencies; the font folds accents (`á` renders as `a`) and leaves what it does not know - blank, so the icon and the exact name stay in the caption, where the phone's font draws them. - A horizontal bar is a share of the `total` it is given, never a fraction of the longest bar: - the percentage in the row is what the length shows, and two rows are comparable. The text - screens always carry the exact numbers. - -**Settings, budgets and routing** - -- Erasing the user runs inside the turn's ambient transaction: `UserWorkLock` already opens one, - so `IUserDataEraser` must reuse `Database.CurrentTransaction` instead of beginning a second. -- `ConversationTurn.UserRemoved` tells the dispatcher to settle the inbox with a `null` owner; - otherwise `CompleteAsync` would write a `user_id` that no longer exists and hit the FK. -- The recurring budget is a fallback, not a copy: `budget_defaults.effective_from` stops a default - from appearing in months before it existed, and a month's own row always wins. -- A row referencing a category must be saved after the category exists in the same context: EF - does not model the composite `(category_id, user_id)` foreign key, so `budget_defaults` (like - `budget_alerts`) needs its own `SaveChanges`. -- Cross-flow actions use `ConversationTurn.HandoffConversation` + `IHandoffConversation` - (`ResolveHandoffAsync`); that is how the category breakdown opens an expense in the expenses - flow without duplicating edit/delete. -- A nullable parameter inside `FromSql` fails with PostgreSQL `42P18`; filter with a boolean flag - (`({hasCategory} = FALSE OR category_id = {category})`) like the keyset cursor does. +The full list, with the reasoning behind every entry, lives in +[`docs/specs/gotchas.md`](docs/specs/gotchas.md). The load-bearing points: + +- **EF Core:** aggregate-created records need store-generated keys; never make the `NO ACTION` + composite FKs `DEFERRABLE`; `ExecuteDeleteAsync` bypasses the change tracker. +- **Configuration and hosting:** binding never turns a single value into an array; hosted + services are singletons (open a scope per iteration); the local compose overlay needs + `external: false`. +- **Text and payloads:** Spanish is the neutral resource set and every key must be in + `MessageKeys`; conversation payloads are `jsonb` (compare as data); callback data is capped at + 64 bytes; a confirmation is claimed once with a conditional `UPDATE`. +- **Dates, money and reporting:** `IUserLocalDate` is the only UTC-to-local conversion; the + matcher's 0.20 partial-overlap floor is load-bearing; a budget-alert marker is recorded whether + or not the notification is delivered. +- **Operations:** quote `"__EFMigrationsHistory"` in raw SQL; `pg_restore` needs + `--exit-on-error` to actually fail. +- **Recurring, closing and charts:** `last_generated_date` and the generated expense commit in + the same transaction; the monthly closing and the daily reminder claim their marker before + sending; charts are hand-drawn PNGs and a bar is a share of the total it is given. +- **Settings, budgets and routing:** `IUserDataEraser` reuses the turn's ambient transaction; a + row referencing a category is saved after the category; cross-flow actions go through + `ConversationTurn.HandoffConversation`; inside `FromSql` filter with a boolean flag, never a + nullable parameter (PostgreSQL `42P18`). ## Status and handoff -| Phase | What | State | -|---|---|---| -| 0–1 | Foundation; domain + schema, constraints, repositories | done | -| 2 | Money, dates, i18n: parsers, formatter, catalog | done | -| 3 | Telegram plumbing: webhook, inbox, allowlist, conversations, onboarding | done | -| 4 | Categories and monthly budgets | done | -| 5 | Expenses: guided and compact entry, edit, delete, history | done | -| 6 | Category matching and keyword learning | done | -| 7 | Summary, range history and statistics | done | -| 8 | Hardening: verified backups, runbook, rate limits, deploy | done | -| 9 | Recurring expenses, spending charts, budget alerts, scheduled summaries | done | -| 10 | Settings (data erasure, reminder switch), recurring budget, per-category breakdown, numbered category chart, daily reminder, 23:59 monthly closing | done | - -Verified end to end through Telegram: categories and budgets; guided and compact expenses with -suggestion, keyword learning, edit, delete and undo; recurring rules applied by a scheduler; 80 % -and 100 % budget alerts; month summary, range history, statistics and category/daily charts; -the closing of the last month sent at 23:59 on the user's last local day, with the first local -day as fallback; verified nightly backups; -reproducible deploy. Settings erases everything and switches the daily reminder; a budget set -once recurs every month with per-month overrides; the summary lists remaining budget and drills -into each category's movements. The category chart measures each bar as its share of the month -and draws the category name; the breakdown has a way back. **835 tests green**, build with zero -warnings, `dotnet format` clean. - -Next: CSV export of a date range's expenses, then seed categories and recurring-rule editing. -The backlog and decisions are in [`docs/HANDOFF.md`](docs/HANDOFF.md). +All phases (0–10) are done. The phase table, the end-to-end verification notes and the backlog +live in [`docs/HANDOFF.md`](docs/HANDOFF.md), next to the next task: CSV export of a date +range's expenses. ## Commands to verify any change diff --git a/docs/HANDOFF.md b/docs/HANDOFF.md index 010797c..cffd1f6 100644 --- a/docs/HANDOFF.md +++ b/docs/HANDOFF.md @@ -1,8 +1,34 @@ # Handoff — next task and backlog The working context is [`AGENTS.md`](../AGENTS.md); the architecture is -[`TECHNICAL-DESIGN.md`](TECHNICAL-DESIGN.md). This file is only the next task's prompt and the -decisions already taken for what comes after. +[`TECHNICAL-DESIGN.md`](TECHNICAL-DESIGN.md). This file holds the project's current status, +the next task's prompt and the decisions already taken for what comes after. + +## Project status + +| Phase | What | State | +|---|---|---| +| 0–1 | Foundation; domain + schema, constraints, repositories | done | +| 2 | Money, dates, i18n: parsers, formatter, catalog | done | +| 3 | Telegram plumbing: webhook, inbox, allowlist, conversations, onboarding | done | +| 4 | Categories and monthly budgets | done | +| 5 | Expenses: guided and compact entry, edit, delete, history | done | +| 6 | Category matching and keyword learning | done | +| 7 | Summary, range history and statistics | done | +| 8 | Hardening: verified backups, runbook, rate limits, deploy | done | +| 9 | Recurring expenses, spending charts, budget alerts, scheduled summaries | done | +| 10 | Settings (data erasure, reminder switch), recurring budget, per-category breakdown, numbered category chart, daily reminder, 23:59 monthly closing | done | + +Verified end to end through Telegram: categories and budgets; guided and compact expenses with +suggestion, keyword learning, edit, delete and undo; recurring rules applied by a scheduler; 80 % +and 100 % budget alerts; month summary, range history, statistics and category/daily charts; +the closing of the last month sent at 23:59 on the user's last local day, with the first local +day as fallback; verified nightly backups; +reproducible deploy. Settings erases everything and switches the daily reminder; a budget set +once recurs every month with per-month overrides; the summary lists remaining budget and drills +into each category's movements. The category chart measures each bar as its share of the month +and draws the category name; the breakdown has a way back. **835 tests green**, build with zero +warnings, `dotnet format` clean. ## Done in this round (all verified, 835 tests green) diff --git a/docs/TECHNICAL-DESIGN.md b/docs/TECHNICAL-DESIGN.md index 4e50176..2ccd56c 100644 --- a/docs/TECHNICAL-DESIGN.md +++ b/docs/TECHNICAL-DESIGN.md @@ -721,7 +721,7 @@ The implementation lives in `scripts/`, the runbook in [`BACKUPS.md`](BACKUPS.md | Single VPS is a single point of failure | High | Verified off-site backups, documented restore, restart policies, healthchecks; the risk is accepted explicitly | | Backup exists but cannot be restored | High | Automated weekly restore into a scratch database with sanity checks and an alert on failure | | Secret leakage | High | Environment-only secrets, redaction, CI scans, ignore files | -| EF Core mis-models aggregate-created records | Medium | Store-generated keys for child records; documented in `AGENTS.md`; covered by tests | +| EF Core mis-models aggregate-created records | Medium | Store-generated keys for child records; documented in `docs/specs/gotchas.md`; covered by tests | | Globalization drift in `es-CO` output | Medium | Explicit `NumberFormatInfo`; Debian image with full ICU | | Time-zone regressions in month boundaries | High | `ExpenseDate` is a calendar date; boundary tests; no hardcoded offsets | | Scope creep into phase-9 features | High (schedule) | Phase gates; nothing starts without a real need | diff --git a/docs/specs/gotchas.md b/docs/specs/gotchas.md new file mode 100644 index 0000000..0d29a7a --- /dev/null +++ b/docs/specs/gotchas.md @@ -0,0 +1,100 @@ +# Implementation gotchas + +Hard-won details that are easy to regress. [`AGENTS.md`](../../AGENTS.md) keeps a one-line-per- +group summary; this file holds the full list with the reasoning behind each entry. Update it +whenever a gotcha is learned or disproven. + +## EF Core and keys + +- Aggregate-created records need store-generated keys (`Entity(keyGeneratedByStore: true)` + + `ValueGeneratedOnAdd()`): EF treats a client-assigned key on a child found in a navigation as + an existing row and fails as a concurrency conflict. Such an unsaved child has `Guid.Empty`, + so never identify it by id (`RemoveAlias` takes the instance). +- Removing a child from a tracked aggregate relies on EF orphan deletion (the non-nullable FK + convention); `OrphanRemovalTests` pins it. +- Do not make the `NO ACTION` composite FKs `DEFERRABLE`: EF's autocommit save would report a + concurrency failure instead of a named foreign key violation. +- EF cannot order an insert by a composite FK it does not model, so an operational row + referencing a category (`budget_alerts`) must be written after the category exists. +- `ExecuteDeleteAsync` bypasses the change tracker; bulk deletes run from their own scope. + +## Configuration and hosting + +- Configuration binding does not turn a single value into an array: `AllowedUserIds` is a + `string` with explicit parsing. A blank value counts as unset, so `.env` can fill it. +- Hosted services are singletons; resolve scoped services from a scope per iteration. Only a + test that builds the real host catches a mistake (`ApiTelegramWiringTests`). +- `Options.Create` is ambiguous inside files importing `MyBudget.Telegram.Options`; qualify it. +- The runtime image is Debian for ICU and tzdata; `external: false` in the local compose overlay + is required; both `app` and `migrator` keep `image: mybudget-app`. +- Docker cannot publish a host port for a container whose only network is internal. + +## Text, resources and payloads + +- Spanish is the neutral resource set; never name it `Messages.es.resx`, and add every key to + `MessageKeys` or the catalog test fails the build. +- XML comments must not contain `--`. +- Conversation payloads are `jsonb`: compare as data, never strings. A payload that cannot be + read is treated as empty. A computed payload property must be `[JsonIgnore]`. +- Callback data is capped at 64 bytes; identify a row by position or term when the id does not + fit. A menu tap outranks the active conversation, and some callbacks (Undo) arrive after the + flow is gone, handled as global callbacks. +- A confirmation is claimed once with a conditional `UPDATE`, never re-read; the callback + carries only the pending id. An empty listing must still carry the notices it was built with. + +## Dates, money and reporting + +- `IUserLocalDate` is the only UTC-to-local conversion; an unusable stored zone falls back to + UTC. `ExpenseDate` is a `DateOnly`; report sums are derived in SQL, never stored. +- The matcher's 0.20 partial-overlap floor and epsilon comparison are load-bearing; signals are + additive per query/keyword and the category takes its best term. +- A budget-alert marker is recorded whether or not the notification is delivered: the marker + stops the bot repeating itself. + +## Operations + +- The EF migration history table is mixed case: quote `"__EFMigrationsHistory"` in raw SQL. +- `pg_restore` exits 0 unless `--exit-on-error`; the drill must actually restore, not just list. +- Pre-deploy and nightly dumps share the backup volume. + +## Recurring and charts + +- A recurring rule is configuration: `last_generated_date` and the generated expense commit in + the same transaction, which is the whole idempotency argument. Month-end clamps to the last + day. The scheduler is registered only with a bot token, first pass delayed one minute. +- The monthly closing is claimed, not read: `monthly_closings` is inserted with `ON CONFLICT DO + NOTHING` on `(user_id, year, month)`, before the message, so a restart cannot repeat it. +- The closing fires on the last local day from 23:59 and falls back to the first local day; both + resolve to the same `closedPeriod`, which is what keeps the marker exactly-once. The + copy-budget button was removed when budgets became recurring. +- A closing with no spending and no allocation is skipped and its marker stays unspent: a + notification feature that talks about nothing is a notification feature that gets muted. +- The daily reminder and the closing share `ScheduledNotificationsScheduler` (every minute, + only with a bot token). The reminder is skipped before the user's local 21:00 and when an + expense already exists that local day; it claims `("daily", local day)` in + `reminder_deliveries` before sending, so frequent ticks are safe. +- Adding a menu section touches `MainMenu.ActionKeys`, the keyboard rows, the menu test and the + router mapping. +- Charts are hand-drawn (`RgbCanvas` + 5x7 bitmap font + PNG over `ZLibStream`) to avoid native + dependencies; the font folds accents (`á` renders as `a`) and leaves what it does not know + blank, so the icon and the exact name stay in the caption, where the phone's font draws them. + A horizontal bar is a share of the `total` it is given, never a fraction of the longest bar: + the percentage in the row is what the length shows, and two rows are comparable. The text + screens always carry the exact numbers. + +## Settings, budgets and routing + +- Erasing the user runs inside the turn's ambient transaction: `UserWorkLock` already opens one, + so `IUserDataEraser` must reuse `Database.CurrentTransaction` instead of beginning a second. +- `ConversationTurn.UserRemoved` tells the dispatcher to settle the inbox with a `null` owner; + otherwise `CompleteAsync` would write a `user_id` that no longer exists and hit the FK. +- The recurring budget is a fallback, not a copy: `budget_defaults.effective_from` stops a default + from appearing in months before it existed, and a month's own row always wins. +- A row referencing a category must be saved after the category exists in the same context: EF + does not model the composite `(category_id, user_id)` foreign key, so `budget_defaults` (like + `budget_alerts`) needs its own `SaveChanges`. +- Cross-flow actions use `ConversationTurn.HandoffConversation` + `IHandoffConversation` + (`ResolveHandoffAsync`); that is how the category breakdown opens an expense in the expenses + flow without duplicating edit/delete. +- A nullable parameter inside `FromSql` fails with PostgreSQL `42P18`; filter with a boolean flag + (`({hasCategory} = FALSE OR category_id = {category})`) like the keyset cursor does.