From 4059828abfc1f95df8689e994febb56e8f4a6fe9 Mon Sep 17 00:00:00 2001 From: Claude Code Date: Fri, 7 Aug 2026 20:23:56 +0900 Subject: [PATCH] fix: missing model_operations.models table + compliance schema breaking every fresh DB (live deploy failure) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The SCP/DbMigrator deploy to the production target (178.104.200.7) failed today with `relation "model_operations.models" does not exist` at migration 0036 — the exact same failure I'd already hit against a local test database, confirming this isn't environment drift but a real, deterministic bug: no migration ever created model_operations.models, only referenced it via FK (0036, 0038) and queried it directly (OpenDartDailyBatchJob.cs). Migration 0037 had the same class of bug for the `compliance` schema itself. - Add 0035_model_operations_models.sql (must sort before 0036). Scope is intentionally minimal — id/ticker/published_at/ correlation_id/revision, i.e. only what's actually referenced today. The full Model Card/lifecycle schema is separate, larger work and isn't guessed at here. - Add `CREATE SCHEMA IF NOT EXISTS compliance;` to 0037, plus IF NOT EXISTS on its indexes for re-run idempotency (matching the rest of this migration set). - Verified: full chain 0000->0040 applies to a fresh DB ("Upgrade successful") and re-run is a clean no-op ("No new scripts need to be executed"). Fixing the schema far enough to actually run queries against it surfaced 3 more real, previously untested bugs in already-merged code (none reachable before because the tables/schema didn't exist): - Dapper was never configured for snake_case<->PascalCase column mapping (`Dapper.DefaultTypeMap.MatchNamesWithUnderscores`), so every Sql class's result-set queries were silently returning null/default for every property instead of throwing. Fixed once, centrally, via a `[ModuleInitializer]` in KArtSell.BuildingBlocks/Data/DapperBootstrap.cs so it's set before the first query regardless of entry point (Host/DbMigrator/tests). - jsonb/inet columns written without an explicit cast (`42804: column "x" is of type jsonb but expression is of type text`) in AuditSql (details, ip_address), TradeSql (kis_response), SellDecisionSql (oos_performance) — fixed with `::jsonb`/`::inet` casts. AuditSql's jsonb read-back into Dictionary also needed a raw-DTO + JsonSerializer.Deserialize mapping. - AuditSql.RedactAuditEventDetailsAsync had a literal duplicate `SET details = ... details = ...` (invalid SQL) — nested the two jsonb_set calls into one assignment. Verified: dotnet build 0/0; architecture 13/13; unit 54/54+18/18; Host boots cleanly and registers all 34 endpoints against the now-complete schema. New tech debt recorded: DEBT-020 (this fix), DEBT-021 (Dapper snake_case fix), DEBT-022 (jsonb/inet casts, partial — not yet audited beyond what surfaced), DEBT-023 (ApprovalSql. InsertProposalAsync still fails on a raw DateOnly parameter — same class of issue as DEBT-021, not yet fixed), DEBT-024 (TradeExecution tests don't insert FK parent rows; one pure-logic ranker test returned 1000 instead of 950 under the full suite, not yet root-caused; DbUpMigrationTests fail locally on a Postgres role permission gap unrelated to this fix). Co-Authored-By: Claude Sonnet 5 --- TECH_DEBT_REGISTER.md | 5 ++ .../0035_model_operations_models.sql | 20 +++++++ db/migrations/0037_audit_trail_gdpr.sql | 16 ++--- .../Data/DapperBootstrap.cs | 23 ++++++++ .../Compliance/AuditSql.cs | 59 ++++++++++++++++--- .../SellDecision/SellDecisionSql.cs | 2 +- .../TradeExecution/TradeSql.cs | 6 +- 7 files changed, 111 insertions(+), 20 deletions(-) create mode 100644 db/migrations/0035_model_operations_models.sql create mode 100644 src/KArtSell.BuildingBlocks/Data/DapperBootstrap.cs diff --git a/TECH_DEBT_REGISTER.md b/TECH_DEBT_REGISTER.md index 2f03faa5..68c7eafd 100644 --- a/TECH_DEBT_REGISTER.md +++ b/TECH_DEBT_REGISTER.md @@ -52,6 +52,11 @@ | DEBT-017 | Duplicate VS-03 Approval Workflow implementation | High (3) | Medium (2) | Backlog | Two independent, functionally-identical VS-03 maker-checker slices exist: `ApprovalWorkflow/` (Workstream H, own `ApprovalProposal`/`IClock`/`IOutbox` types) and `Features/ApprovalWorkflow/` (Workstream G, matches documented `Features//` convention). Both mapped the same routes (`/approvals`, `/approvals/{id}`, `/approvals/{id}/approve`), which crashed Host startup with a duplicate-route/missing-DI error the first time the app was actually booted (2026-08-07 — apparently never booted successfully before). Old set annotated `[DontRegister]` (FastEndpoints) 2026-08-07 to unblock boot; code and its test file (`ApprovalWorkflowTests.cs`) kept for now. Needs an architect decision: delete the old slice entirely (and its test) or intentionally keep both for a reason not yet documented. | @claude | Session 2026-08-07 (Phase 3 J/K/L hardening) | | DEBT-018 | Outbox write not co-transactional with entity write | Medium (2) | Medium (2) | Backlog | `TradeExecution/TradeHandlers.cs` (`TradeOutboxPublisher`) and `PortfolioReconciliation/ReconcileTradeHandler.cs` open a second, separate connection/transaction to write the outbox message after the trade/holding write already committed on its own connection. A crash between the two leaves the entity updated but no outbox event emitted (silent, non-atomic). Proper fix: thread a shared `NpgsqlTransaction` through `TradeSql`/`ReconciliationSql` mutation methods so entity insert + outbox insert commit together, matching `DapperModelOperationRequestRepository`'s pattern. | @claude | Session 2026-08-07 (Phase 3 J/K/L hardening) | | DEBT-019 | Multiple duplicate cross-cutting abstractions (`IClock`, `IOutboxWriter`, `IKrxDataService`) | Medium (2) | Low (1) | Completed (partial) | Found and collapsed 3 separate cases where a slice reinvented an abstraction that already existed in `KArtSell.BuildingBlocks`: a second `IKrxDataService` (deleted, `ShadowRun.Services`), a second `IOutboxWriter`/`WriteAsync` in `ReconcileTradeHandler.cs` (removed, switched to `BuildingBlocks.Reliability.IOutboxWriter`), and a second `IClock`/`SystemClock` in `ApprovalWorkflow/ApprovalPolicy.cs` (removed, switched to `BuildingBlocks.Time.IClock`). Root cause: successive sessions implementing a slice without searching `BuildingBlocks` first. Recommend a pre-implementation checklist step ("does this abstraction already exist in BuildingBlocks?") for future slices. | @claude | Session 2026-08-07 (Phase 3 J/K/L hardening) | +| DEBT-020 | `model_operations.models` and `compliance` schema never created by any migration | High (3) | Low (1) | Completed | `0036`/`0038` reference `model_operations.models(id)` via FK and `OpenDartDailyBatchJob.cs` queries it directly, but no migration ever ran `CREATE TABLE model_operations.models`; `0037` wrote to `compliance.*` tables without `CREATE SCHEMA compliance`. Any fresh database — including the actual deploy target (178.104.200.7), confirmed via a live failed SCP/DbMigrator deploy on 2026-08-07 — failed at migration `0036`/`0037`. Fixed via new `0035_model_operations_models.sql` (minimal: id/ticker/published_at/correlation_id/revision only — full Model Card schema is separate future work) and `CREATE SCHEMA IF NOT EXISTS compliance;` added to `0037`. Full chain 0000→0040 now verified fresh-install + idempotent re-run clean. | @claude | Session 2026-08-07 (deploy failure triage) | +| DEBT-021 | Dapper never configured for snake_case↔PascalCase column mapping | High (3) | Low (1) | Completed | `Dapper.DefaultTypeMap.MatchNamesWithUnderscores` was never set anywhere in the codebase, so every `QueryAsync`/`QuerySingleOrDefaultAsync` result-mapping onto a snake_case DB column (e.g. `event_type` → `EventType`) silently returned null/default for that property instead of throwing — masking the bug in every Sql class across every module. Confirmed via `ApprovalWorkflowTests.InsertAndRetrieveProposal_RoundTrips` and `AuditTrailTests.InsertAuditEvent_CreatesImmutableRecord` both getting real rows back with null fields. Fixed centrally via a `[ModuleInitializer]` in `KArtSell.BuildingBlocks/Data/DapperBootstrap.cs` (runs once per process regardless of entry point — Host/DbMigrator/tests). | @claude | Session 2026-08-07 (deploy failure triage) | +| DEBT-022 | jsonb/inet columns written as plain text without an explicit cast | Medium (2) | Low (1) | Completed (partial) | Dapper does not know to cast a `string` parameter to `jsonb`/`inet` for Npgsql; `AuditSql.InsertAuditEventAsync` (`details`, `ip_address`), `AuditSql.RedactAuditEventDetailsAsync` (duplicate `SET details =` assignment, separately fixed), `TradeSql.InsertTradeAsync`/`UpdateTradeStatusAsync` (`kis_response`), and `SellDecisionSql.InsertDecisionAsync` (`oos_performance`) all failed with `42804: column "x" is of type jsonb but expression is of type text` the first time they were run against a real schema. Fixed with explicit `::jsonb`/`::inet` casts at each call site (mechanical, no behavior change). `AuditSql`'s jsonb read-back (`Dictionary` from a jsonb column) also needed a raw-DTO + `JsonSerializer.Deserialize` mapping since Dapper has no built-in jsonb→Dictionary conversion either. **Not yet checked**: `PortfolioReconciliation`/`ApprovalWorkflow` Sql classes for the same pattern beyond what surfaced in this session's test runs — a full audit of jsonb/inet columns across all Sql classes is still open. | @claude | Session 2026-08-07 (deploy failure triage) | +| DEBT-023 | `ApprovalSql.InsertProposalAsync` fails on `DateOnly` parameter | Medium (2) | Low (1) | Backlog | `ApprovalWorkflowTests.InsertAndRetrieveProposal_RoundTrips` fails with `System.NotSupportedException: The member effectiveAt of type System.DateOnly cannot be used as a parameter value` — Dapper's `LookupDbType` doesn't recognize `DateOnly` without an explicit type map (`SqlMapper.AddTypeMap`/custom `TypeHandler`). Likely affects every other `DateOnly`-typed Dapper parameter in the codebase, not just this one; needs a similar centralized fix to DEBT-021 rather than a per-call-site patch. Discovered but not fixed in this session (scope cut to unblock the live deploy). | @claude | Session 2026-08-07 (deploy failure triage) | +| DEBT-024 | New integration tests don't insert FK parent rows / one pure-logic test flakes under full-suite run | Low (1) | Low (1) | Backlog | `TradeExecutionTests` constructs `Trade` with a random `sellDecisionId` that was never inserted into `sell_decisions`, so every insert now correctly fails its FK constraint (`trades_sell_decision_id_fkey`) once the schema was actually complete (see DEBT-020) — test-only gap, not a production code defect; needs the tests updated to insert a parent `models`+`sell_decisions` row first. Separately, `SellPriorityRankerTests.CalculateScore_HardImpairment_ReturnsLowestScore` (pure logic, no DB) passed in isolation but returned 1000 instead of the expected 950 (age-boost not applied) when run as part of the full suite — not yet root-caused; may be test-order/parallelization state leakage rather than a `SellPriorityRanker` bug. Also, `DbUpMigrationTests.*` (pre-existing, unrelated to this session) fail locally with `42501: must be owner of database kartsell_migration_test` — a local Postgres role permission gap, not a code issue. | @claude | Session 2026-08-07 (deploy failure triage) | --- diff --git a/db/migrations/0035_model_operations_models.sql b/db/migrations/0035_model_operations_models.sql new file mode 100644 index 00000000..be8ed737 --- /dev/null +++ b/db/migrations/0035_model_operations_models.sql @@ -0,0 +1,20 @@ +-- Migration 0041: model_operations.models +-- Missing prerequisite table: referenced via FK by 0036 (approval_proposals.model_id) +-- and 0038 (sell_decisions.model_id), and queried directly by OpenDartDailyBatchJob.cs +-- (SELECT DISTINCT ticker ... WHERE published_at <= @now), but never created by any +-- prior migration. Any fresh database fails at 0036 without this table. +-- +-- Scope is intentionally minimal (only the columns actually referenced today). The full +-- Model Card / lifecycle schema (Freeze/Mature/Score/Diagnose/.../Manual Activation per +-- CLAUDE.md) is a separate, larger piece of work and is not guessed at here. + +CREATE TABLE IF NOT EXISTS model_operations.models ( + id UUID PRIMARY KEY, + ticker VARCHAR(20) NOT NULL, + published_at TIMESTAMPTZ NOT NULL DEFAULT NOW(), + correlation_id UUID NOT NULL, + revision INT NOT NULL DEFAULT 1 +); + +CREATE INDEX IF NOT EXISTS ix_models_ticker ON model_operations.models(ticker); +CREATE INDEX IF NOT EXISTS ix_models_published_at ON model_operations.models(published_at DESC); diff --git a/db/migrations/0037_audit_trail_gdpr.sql b/db/migrations/0037_audit_trail_gdpr.sql index 57128e17..c77f88b6 100644 --- a/db/migrations/0037_audit_trail_gdpr.sql +++ b/db/migrations/0037_audit_trail_gdpr.sql @@ -1,6 +1,8 @@ -- Workstream I: VS-04 Audit Trail (Immutable events + GDPR compliance) -- Creates compliance audit trail for model operations, regulatory reporting, and GDPR redaction +CREATE SCHEMA IF NOT EXISTS compliance; + -- Audit events (immutable, INSERT-only) CREATE TABLE IF NOT EXISTS compliance.audit_events ( id UUID PRIMARY KEY, @@ -22,11 +24,11 @@ CREATE TABLE IF NOT EXISTS compliance.audit_events ( ); -- Indexes for compliance querying -CREATE INDEX idx_audit_events_entity_id ON compliance.audit_events(entity_id); -CREATE INDEX idx_audit_events_event_type ON compliance.audit_events(event_type); -CREATE INDEX idx_audit_events_actor_email ON compliance.audit_events(actor_email); -CREATE INDEX idx_audit_events_event_at ON compliance.audit_events(event_at); -CREATE INDEX idx_audit_events_correlation_id ON compliance.audit_events(correlation_id); +CREATE INDEX IF NOT EXISTS idx_audit_events_entity_id ON compliance.audit_events(entity_id); +CREATE INDEX IF NOT EXISTS idx_audit_events_event_type ON compliance.audit_events(event_type); +CREATE INDEX IF NOT EXISTS idx_audit_events_actor_email ON compliance.audit_events(actor_email); +CREATE INDEX IF NOT EXISTS idx_audit_events_event_at ON compliance.audit_events(event_at); +CREATE INDEX IF NOT EXISTS idx_audit_events_correlation_id ON compliance.audit_events(correlation_id); -- GDPR retention tracking (personal data retention policy) CREATE TABLE IF NOT EXISTS compliance.gdpr_retention ( @@ -43,8 +45,8 @@ CREATE TABLE IF NOT EXISTS compliance.gdpr_retention ( ); -- Indexes for GDPR processing -CREATE INDEX idx_gdpr_retention_customer_id ON compliance.gdpr_retention(customer_id); -CREATE INDEX idx_gdpr_retention_purge_status ON compliance.gdpr_retention(purge_status); +CREATE INDEX IF NOT EXISTS idx_gdpr_retention_customer_id ON compliance.gdpr_retention(customer_id); +CREATE INDEX IF NOT EXISTS idx_gdpr_retention_purge_status ON compliance.gdpr_retention(purge_status); -- Event types enumeration (reference, not enforced at DB level) CREATE TABLE IF NOT EXISTS compliance.audit_event_types ( diff --git a/src/KArtSell.BuildingBlocks/Data/DapperBootstrap.cs b/src/KArtSell.BuildingBlocks/Data/DapperBootstrap.cs new file mode 100644 index 00000000..2a53cc49 --- /dev/null +++ b/src/KArtSell.BuildingBlocks/Data/DapperBootstrap.cs @@ -0,0 +1,23 @@ +using System.Runtime.CompilerServices; + +namespace KArtSell.BuildingBlocks.Data; + +/// +/// Dapper does not map snake_case DB columns (event_type) to PascalCase C# properties +/// (EventType) by default; every Sql class in this repo relies on that mapping, so this +/// must be set before any query runs. A module initializer guarantees it runs once per +/// process regardless of entry point (Host, DbMigrator, test runner) without every Sql +/// class or Program.cs having to remember to configure it. +/// +internal static class DapperBootstrap +{ +#pragma warning disable CA2255 // intentional: BuildingBlocks is this solution's internal shared layer, + // not a distributed package, and every entry point (Host/DbMigrator/tests) needs this set + // before its first query regardless of which one runs first. + [ModuleInitializer] +#pragma warning restore CA2255 + public static void Initialize() + { + Dapper.DefaultTypeMap.MatchNamesWithUnderscores = true; + } +} diff --git a/src/KArtSell.Modules.ModelOperations/Compliance/AuditSql.cs b/src/KArtSell.Modules.ModelOperations/Compliance/AuditSql.cs index 9619629f..bebdad94 100644 --- a/src/KArtSell.Modules.ModelOperations/Compliance/AuditSql.cs +++ b/src/KArtSell.Modules.ModelOperations/Compliance/AuditSql.cs @@ -43,7 +43,7 @@ public class AuditSql error_message, details, evidence_links, ip_address, user_agent, published_at, correlation_id, revision) VALUES (@Id, @EventType, @EntityType, @EntityId, @ActorEmail, @ActorRole, @EventAt, - @Result, @ErrorMessage, @Details, @EvidenceLinks, @IpAddress, @UserAgent, + @Result, @ErrorMessage, @Details::jsonb, @EvidenceLinks, @IpAddress::inet, @UserAgent, NOW(), @CorrelationId, 1) """; @@ -146,7 +146,8 @@ public class AuditSql parameters.Add("@Skip", skip); parameters.Add("@Take", take); - var events = (await db.QueryAsync(sql, parameters)).ToList(); + var raw = await db.QueryAsync(sql, parameters); + var events = raw.Select(ToAuditEvent).ToList(); return (events, total); } @@ -167,7 +168,48 @@ public class AuditSql WHERE id = @EventId """; - return await db.QuerySingleOrDefaultAsync(sql, new { EventId = eventId }); + var raw = await db.QuerySingleOrDefaultAsync(sql, new { EventId = eventId }); + return raw == null ? null : ToAuditEvent(raw); + } + + private static AuditEvent ToAuditEvent(AuditEventRaw raw) => new() + { + Id = raw.Id, + EventType = raw.EventType, + EntityType = raw.EntityType, + EntityId = raw.EntityId, + ActorEmail = raw.ActorEmail, + ActorRole = raw.ActorRole, + EventAt = raw.EventAt, + Result = raw.Result, + ErrorMessage = raw.ErrorMessage, + Details = raw.Details == null ? null : JsonSerializer.Deserialize>(raw.Details), + EvidenceLinks = raw.EvidenceLinks, + IpAddress = raw.IpAddress, + UserAgent = raw.UserAgent, + PublishedAt = raw.PublishedAt, + CorrelationId = raw.CorrelationId, + Revision = raw.Revision + }; + + private sealed class AuditEventRaw + { + public Guid Id { get; set; } + public required string EventType { get; set; } + public required string EntityType { get; set; } + public Guid EntityId { get; set; } + public required string ActorEmail { get; set; } + public string? ActorRole { get; set; } + public DateTime EventAt { get; set; } + public required string Result { get; set; } + public string? ErrorMessage { get; set; } + public string? Details { get; set; } + public string[]? EvidenceLinks { get; set; } + public string? IpAddress { get; set; } + public string? UserAgent { get; set; } + public DateTime PublishedAt { get; set; } + public Guid CorrelationId { get; set; } + public int Revision { get; set; } } /// @@ -255,12 +297,11 @@ public class AuditSql const string sql = """ UPDATE compliance.audit_events SET details = jsonb_set( - COALESCE(details, '{}'::jsonb), - '{actor_email}', - '""'::jsonb - ), - details = jsonb_set( - details, + jsonb_set( + COALESCE(details, '{}'::jsonb), + '{actor_email}', + '""'::jsonb + ), '{customer_id}', '""'::jsonb ), diff --git a/src/KArtSell.Modules.ModelOperations/SellDecision/SellDecisionSql.cs b/src/KArtSell.Modules.ModelOperations/SellDecision/SellDecisionSql.cs index bca39a8a..6fd99af6 100644 --- a/src/KArtSell.Modules.ModelOperations/SellDecision/SellDecisionSql.cs +++ b/src/KArtSell.Modules.ModelOperations/SellDecision/SellDecisionSql.cs @@ -24,7 +24,7 @@ public class SellDecisionSql : ISellDecisionSql sell_priority, target_quantity, target_price, approval_id, execution_id, created_at, created_by, created_justification, published_at, correlation_id, revision ) VALUES ( - @id, @modelId, @status, @pboScore, @dsrMetric, @oosPerformance, + @id, @modelId, @status, @pboScore, @dsrMetric, @oosPerformance::jsonb, @sellPriority, @targetQuantity, @targetPrice, @approvalId, @executionId, @createdAt, @createdBy, @createdJustification, @publishedAt, @correlationId, @revision ) diff --git a/src/KArtSell.Modules.ModelOperations/TradeExecution/TradeSql.cs b/src/KArtSell.Modules.ModelOperations/TradeExecution/TradeSql.cs index 076f2b86..542ec168 100644 --- a/src/KArtSell.Modules.ModelOperations/TradeExecution/TradeSql.cs +++ b/src/KArtSell.Modules.ModelOperations/TradeExecution/TradeSql.cs @@ -126,7 +126,7 @@ public class TradeSql : ITradeSql unit_price, total_amount, commission, net_proceeds, error_message, kis_response, execution_timestamp, settlement_timestamp, published_at, correlation_id, revision) VALUES (@id, @sellDecisionId, @kisOrderId, @status, @quantity, @executedQuantity, - @unitPrice, @totalAmount, @commission, @netProceeds, @errorMessage, @kisResponse, + @unitPrice, @totalAmount, @commission, @netProceeds, @errorMessage, @kisResponse::jsonb, @executionTimestamp, @settlementTimestamp, @publishedAt, @correlationId, @revision) """; @@ -167,13 +167,13 @@ public class TradeSql : ITradeSql const string sql = """ INSERT INTO model_operations.trade_status_history (id, trade_id, old_status, new_status, transitioned_at, kis_response, error_message, published_at, correlation_id) - SELECT @id, id, status, @newStatus, NOW(), @kisResponse, @errorMessage, NOW(), @correlationId + SELECT @id, id, status, @newStatus, NOW(), @kisResponse::jsonb, @errorMessage, NOW(), @correlationId FROM model_operations.trades WHERE id = @tradeId; UPDATE model_operations.trades SET status = @newStatus, - kis_response = COALESCE(@kisResponse, kis_response), + kis_response = COALESCE(@kisResponse::jsonb, kis_response), error_message = COALESCE(@errorMessage, error_message), revision = revision + 1 WHERE id = @tradeId -- 2.52.0