audit-mixin-rollout-tier4: ai/budget/users created_by/updated_by

openspec/changes/budget-fix-money-integrity/.openspec.yaml

@@ -0,0 +1,4 @@
+schema: spec-driven
+created: 2026-09-22
+author: Norair Arutshyan
+priority: high

openspec/changes/budget-fix-money-integrity/proposal.md

@@ -0,0 +1,29 @@
+## Why
+
+Two related money-correctness gaps in the budget service, both confirmed but deliberately not blocking prior ships:
+
+1. **`Budget.total_amount` recalculation is fragile.** From the donor-dashboard code review (#133): `create_budget_with_lines_service`'s rollback path bypasses `recalculate_budget_total` entirely (calls raw `delete_budget_line` CRUD directly, currently masked because the parent budget is deleted right after); the line write and total recalculation commit separately with no wrapping transaction; money is stored as `Float` (not `Decimal`), so binary floating-point drift is now surfaced through the API; a missed recalculation is silently swallowed with no log; `total_amount` is client-writable on `BudgetCreate`/`BudgetUpdate` but silently dropped; the `COALESCE(SUM(amount), 0)` formula is implemented twice (migration backfill SQL + SQLAlchemy) with no single source of truth. **The `Float` problem is wider than `total_amount` alone** — a repo-wide check found the same `Float` type on `BudgetLineModel.amount`, `ReportLine.amount` (`report.py`), and the entire currency-ledger table set (`currency_ledger.py`: `FundingReceipt.amount`, `CurrencyConversion.donor_amount`/`local_amount`, `CurrencyConversionAllocation.amount_allocated`) — every money column in the budget service is `Float`.
+2. **Currency codes are never validated.** `shared/services/currency_service.py` has a working ISO 4217 module (`validate_currency`, `get_currency_list`, etc.) re-exported by both budget and users utils, but no actual call site validates `Budget.actual_currency`/`local_currency` or the currency-ledger's `record_receipt_service`/`record_conversion_service` inputs. The frontend currency dropdown is a hardcoded 11-code stand-in (`frontend-typescript/src/utils/currency.ts`) for the same reason — no endpoint exposes the real list.
+
+Bundled together because both are "money can silently be wrong" gaps in the same service, even though they're different mechanisms (recalculation integrity vs. input validation) — real money starts flowing through this table via the currency ledger, so both get harder to fix the longer they wait.
+
+## What Changes
+
+- Fix the rollback-path gap: route `create_budget_with_lines_service`'s exception handlers through `recalculate_budget_total` (or a CRUD-layer/ORM event listener so no future line-mutation path can skip it), not raw `delete_budget_line`.
+- Wrap line-write + recalculation in a single transaction so a second-commit failure can't leave a stale total persisted.
+- Migrate all budget-service money columns from `Float` to `Decimal`/`Numeric` — `budget.py` (`BudgetModel`, `BudgetLineModel`), `report.py` (`ReportLine.amount`), `currency_ledger.py` (`FundingReceipt.amount`, `CurrencyConversion.donor_amount`/`local_amount`, `CurrencyConversionAllocation.amount_allocated`) — including a data migration for existing rows. Doing this before the active `ledger-budget` change adds more update/delete surface on these same currency-ledger columns avoids compounding the Float-drift problem in new code.
+- Either enforce or reject `total_amount` on `BudgetCreate`/`BudgetUpdate` instead of silently dropping it; log when a recalculation is skipped (budget not found).
+- De-duplicate the `COALESCE(SUM(amount), 0)` formula to one source of truth.
+- Wire `validate_currency()` into `update_budget_service` (`actual_currency`/`local_currency`) and into `record_receipt_service`/`record_conversion_service`.
+- Expose `get_currency_list()` via a small endpoint (budget or users service) so the frontend dropdown can replace its hardcoded 11-code stand-in with the real ISO 4217 list.
+
+## Capabilities
+
+### New Capabilities
+- `currency-code-validation`: server-side rejection of invalid ISO 4217 currency codes on budget and currency-ledger writes.
+
+## Status
+
+Proposal only — no design.md/tasks.md yet. Recommend investigating the `Float`→`Decimal` migration path (data migration for existing rows) before task breakdown, since it's the piece most likely to get harder the longer it waits.
+
+**Dependency note:** `currency_ledger_services.py` (`record_receipt_service`/`record_conversion_service`) is also touched by the active, not-yet-started `ledger-budget` change (adds update/delete for funding receipts and currency conversions on the same file). No hard blocker either order, but landing this change first — schema types and validation in place before more code is added against `Float` columns — avoids rework in `ledger-budget` and a same-file merge conflict if both are in flight at once.

openspec/changes/ledger-budget/.openspec.yaml

@@ -1,2 +1,4 @@
 schema: spec-driven
 created: 2026-08-03
+depends_on:
+  - budget-fix-money-integrity

openspec/changes/shared-feat-138-status-history/.openspec.yaml

@@ -0,0 +1,7 @@
+schema: spec-driven
+created: 2026-09-22
+author: Norair Arutshyan
+priority: medium
+depends_on:
+  - shared-feat-307-audit-mixin-rollout-tier4
+  - async-privileged-access-audit

openspec/changes/shared-feat-138-status-history/proposal.md

@@ -0,0 +1,24 @@
+## Why
+
+`BudgetModel.status` (`ai_draft`/`draft`/`confirmed`/`archived`) is a plain field with no transition history: there's no way to answer "was this budget ever confirmed before it was archived," and no `AuditLog`-style table exists for budget mutations (only `services/ai` has one). Status is mutated via a generic PATCH-style path (`budget_crud.py`, `budget.status = status or budget.status`) with no record of who changed it or when. The identical need has now surfaced independently in a second domain: admin/superuser actions (invite user, remove user, update company, deactivate company) in `admin-management-page`, which today logs nothing for a company admin acting within their own tenant (only impersonation sessions get `privileged_access_logs` coverage). Two independent domains wanting the same shape (actor/target/from→to/timestamp) is a strong signal this should be one general mechanism, not a budget-only table.
+
+## What Changes
+
+- Add a general-purpose transition/action audit-log table (or `shared/db/` mixin), not scoped to budgets specifically — shape: actor, target entity/id, from-state, to-state, changed_at.
+- Wire budget status transitions (`budget_crud.py`'s status-update choke point) through this mechanism.
+- Wire admin-management actions (invite, remove, promote/demote, deactivate) through the same mechanism, including company-admin-acting-within-own-tenant (not just superuser impersonation, which already has separate `privileged_access_logs` coverage).
+- Design the table to double as the source of truth for "was this budget ever confirmed" (needed by the donor dashboard's grantee status breakdown), rather than adding a separate `confirmed_at` high-water-mark column later.
+- Decide whether an intermediate `submitted` status (grantee → donor handoff, discussed but not yet added to `BudgetStatus`) belongs in this same piece of work, since it would be another transition the table needs to capture.
+
+## Capabilities
+
+### New Capabilities
+- `transition-audit-log`: general actor/target/from→to/timestamp audit trail, applied to budget status transitions and admin-management actions.
+
+## Status
+
+Proposal only — scope and design (single shared table vs. per-domain tables via a common mixin, exact schema, whether `submitted` status is in scope) need deeper investigation before task breakdown. Parent tracking: [GitHub issue #138](https://github.com/arutsh/GrantFlow/issues/138).
+
+**Dependency note:** two audit-infrastructure changes are already in flight and this should sequence behind both, not run in parallel:
+- `shared-feat-307-audit-mixin-rollout-tier4` (current branch) — finishing it first avoids context-switching mid-audit-buildout and any incidental conflict in `shared/db/`.
+- `async-privileged-access-audit` (active, 0/9 tasks done) — makes `log_privileged_access`/`PrivilegedAccessSink` async and removes the per-service dedicated sync engine each service was hand-copying for audit writes. This new transition-audit-log's write path should reuse that same async sink pattern once it lands, rather than becoming a 5th hand-duplicated audit-write mechanism (see `[[project_privileged_access_log_model_duplication]]`).

openspec/changes/audit-mixin-rollout-tier4/.openspec.yaml → openspec/changes/shared-feat-307-audit-mixin-rollout-tier4/.openspec.yaml

@@ -1,2 +1,3 @@
 schema: spec-driven
 created: 2026-09-01
+priority: high

openspec/changes/audit-mixin-rollout-tier4/design.md → openspec/changes/shared-feat-307-audit-mixin-rollout-tier4/design.md

@@ -1,6 +1,6 @@
 ## Context
 
-The 5 Tier 4 models each already declare their own `id` primary key column (`UserModel.id` uses `default=lambda: str(uuid.uuid4())`, notably a `str` default rather than `AuditMixin`'s `uuid.UUID` default — a pre-existing minor inconsistency, not something this change needs to reconcile). `AuditMixin` also declares an `id` column. In SQLAlchemy declarative mixins, an attribute defined directly on the concrete model class takes precedence over the same-named attribute inherited from a mixin — so mixing in `AuditMixin` on these models does **not** require touching their existing `id` definitions; only the unset `created_at`/`updated_at`/`created_by`/`updated_by` attributes get pulled in from the mixin. This significantly de-risks adopting `AuditMixin` on `UserModel`/`CustomerModel`: no PK change, no id-format migration.
+The 5 Tier 4 models each already declare their own `id` primary key column. `AuditMixin` also declares an `id` column. In SQLAlchemy declarative mixins, an attribute defined directly on the concrete model class takes precedence over the same-named attribute inherited from a mixin — so mixing in `AuditMixin` on these models does **not** require touching their existing `id` definitions; only the unset `created_at`/`updated_at`/`created_by`/`updated_by` attributes get pulled in from the mixin. This significantly de-risks adopting `AuditMixin` on `UserModel`/`CustomerModel`: no PK change, no id-format migration.
 
 `UserModel`/`CustomerModel` sit on the auth/tenancy hot path (login, registration, JWT issuance, company onboarding, admin management) with a large existing test surface — this is the highest-risk tier of the whole rollout, ordered last deliberately.
 
@@ -13,7 +13,7 @@ The 5 Tier 4 models each already declare their own `id` primary key column (`Use
 
 **Non-Goals:**
 - No backfill of `created_at` for existing users/customers (historical creation time is genuinely unknown).
-- No change to how `UserModel.id`/`CustomerModel.id` are generated (stays `str(uuid.uuid4())`, not unified with `AuditMixin`'s `uuid.uuid4()` — cosmetic inconsistency, out of scope).
+- No change to how `UserModel.id`/`CustomerModel.id` are generated — both already use `uuid.uuid4()` directly, same as `AuditMixin`'s default, so there's nothing to reconcile.
 - No requirement that `created_by` be non-NULL — self-registration has no authenticated actor, so NULL is a valid, expected value here (unlike, say, budget models where every row has a clear creator).
 
 ## Decisions
@@ -39,4 +39,5 @@ Standard Alembic migration per service (`ai`, `budget`, `users`), additive/nulla
 
 ## Open Questions
 
-- Should admin-created accounts (company-onboarding, admin-invite flows) be audited to confirm they already pass an actor context that would populate `created_by` correctly, or do those flows also run unauthenticated in some cases?
+- ~~Should admin-created accounts (company-onboarding, admin-invite flows) be audited to confirm they already pass an actor context that would populate `created_by` correctly, or do those flows also run unauthenticated in some cases?~~
+  **Resolved:** both `POST /users/invite` and `POST /customers/` require `Depends(get_validated_user)` (`services/users/app/api/user_routes.py:265`, `services/users/app/api/customer_routes.py:36`), which sets the actor contextvar (`shared/security/dependencies.py:54`) before the row is inserted — covered by `TestAdminInviteAuditTrail`/`TestCustomerCreationAuditTrail` in `test_tier4_audit_columns.py`. "Company-onboarding" (a new company's founder self-registering) goes through the unauthenticated self-registration path instead, where `created_by` staying `NULL` is the documented, tested (`TestUserSelfRegistrationAuditTrail`), expected behavior per decision 2 above — not a gap.

openspec/changes/shared-feat-307-audit-mixin-rollout-tier4/tasks.md

@@ -0,0 +1,24 @@
+One task group = one GitHub ticket = one PR, merged before the next group starts.
+
+## 1. ai service catalog models — depends on audit-mixin-auto-population being merged — Issue #308
+
+- [x] 1.1 Add Alembic migration adding nullable `created_at`/`updated_at`/`created_by`/`updated_by` to `AIProvider`, `AIProviderModel`.
+- [x] 1.2 Update both model classes to inherit `AuditMixin`; confirm existing `id` column definitions are unaffected (mixin attribute override, per design.md decision 1).
+- [x] 1.3 Add/update tests confirming the new columns populate correctly for authenticated admin-driven creation, and stay `NULL` for unauthenticated seed/migration inserts.
+- [ ] 1.4 Run `services/ai`'s test suite clean; PR merged. (Test suite clean: 136 passed. PR not yet opened.)
+
+## 2. budget service DonorTemplateModel — depends on 1 — Issue #309
+
+- [x] 2.1 Add Alembic migration adding nullable `created_at`/`updated_at`/`created_by`/`updated_by` to `DonorTemplateModel`.
+- [x] 2.2 Update the model class to inherit `AuditMixin`.
+- [x] 2.3 Add/update a test confirming `created_by` is populated on template upload via the authenticated route.
+- [ ] 2.4 Run `services/budget`'s test suite clean; PR merged. (Test suite clean: 374 passed. PR not yet opened.)
+
+## 3. users service CustomerModel and UserModel — depends on 1, 2 — Issue #310
+
+- [x] 3.1 Add Alembic migration adding nullable `created_at`/`updated_at`/`created_by`/`updated_by` to `CustomerModel` and `UserModel`.
+- [x] 3.2 Update both model classes to inherit `AuditMixin`; confirm existing `id` column definitions (including `UserModel`'s `str(uuid.uuid4())` default) are unaffected.
+- [x] 3.3 Audit admin-management and company-onboarding flows to confirm they pass an authenticated actor context so `created_by` populates correctly there (per design.md's open question); self-registration is expected to leave `created_by` `NULL`.
+- [x] 3.4 Add/update tests: self-registration leaves `created_by` `NULL`; admin-created accounts get a non-NULL `created_by`; updating a user/customer sets `updated_by`.
+- [ ] 3.5 Manually verify login, self-registration, and admin-management flows on staging before merge, per this repo's standard practice for auth-adjacent schema changes. (Login, self-registration, and customer creation already verified locally; staging still pending.)
+- [ ] 3.6 Run `services/users`'s test suite clean; PR merged. (Test suite clean: 202 passed. PR not yet opened.)

openspec/changes/users-fix-auth-claims-hardening/.openspec.yaml

@@ -0,0 +1,4 @@
+schema: spec-driven
+created: 2026-09-22
+author: Norair Arutshyan
+priority: medium

openspec/changes/users-fix-auth-claims-hardening/proposal.md

@@ -0,0 +1,22 @@
+## Why
+
+Two independent findings on the same auth surface (`services/users/app/api/auth_routes.py`, `services/users/app/crud/user_crud.py`):
+
+1. **No shared claims builder.** `register_endpoint`, `login`, and `refresh_token` each independently build their own inline JWT claims dict literal, with no shared schema pinning claim key names/types across the Python producer, the hand-written `TokenClaims` TS interface in `AuthContext.tsx`, and the budget service's `require_donor` check that reads the same decoded payload. Only 3 call sites exist today, but nothing enforces staying in sync as more are added (admin impersonation, magic-link login, password-reset auto-login), and a claim-name typo would silently break consumers rather than erroring.
+2. **Cross-tenant email enumeration.** `create_invited_user` checks `UserModel.email == email` with no company/deleted_at/status scoping — an authenticated company admin can probe any email via the invite form and learn from the 400/success split whether it's registered anywhere on the platform, including at a company they have no relationship to.
+
+Grouped together because they're the same file area and both auth-hardening debt, not because they're the same kind of fix.
+
+## What Changes
+
+- Centralize JWT claims construction into one `_build_access_token_claims(db, user, session)` function called from all token-issuance sites; consider a shared schema/type pinning the claim shape between backend and frontend.
+- Decide the desired behavior for invite-time uniqueness checking: scope to same-company only (and give a generic non-committal response for other-company/global collisions), or explicitly accept and document the current cross-tenant signal. **This is a product/security tradeoff, not just a bug — needs an explicit decision recorded in design.md before implementation, not a silent fix.**
+
+## Capabilities
+
+### New Capabilities
+(none — this is a hardening/refactor of the existing auth-claims and invite-uniqueness mechanisms)
+
+## Status
+
+Proposal only — no design.md/tasks.md yet. The claims-builder centralization is a straightforward refactor; the enumeration item needs a product decision first.

services/ai/app/models/ai_provider.py

@@ -1,18 +1,13 @@
-import uuid
-
 from sqlalchemy import Boolean, String
 from sqlalchemy.orm import mapped_column, Mapped
 
 from app.models.base import Base
-import shared.db.type_decorators as t
+from shared.db.audit_mixin import AuditMixin
 
 
-class AIProvider(Base):
+class AIProvider(Base, AuditMixin):
     __tablename__ = "ai_providers"
 
-    id: Mapped[t.GUID] = mapped_column(
-        t.GUID(), primary_key=True, default=lambda: str(uuid.uuid4())
-    )
     name: Mapped[str] = mapped_column(String, nullable=False, unique=True, index=True)
     display_name: Mapped[str] = mapped_column(String, nullable=False)
     key_prefix: Mapped[str | None] = mapped_column(String, nullable=True)