Merge pull request 'fix: missing model_operations.models table + compliance schema breaking every fresh DB (live deploy failure)' (#29) from feat/L-vs14-portfolio-reconciliation into main
This commit was merged in pull request #29.
This commit is contained in:
@@ -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/<Slice>/` 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<T>` 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<T>`/`QuerySingleOrDefaultAsync<T>` 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<string,object>` 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) |
|
||||
|
||||
---
|
||||
|
||||
|
||||
@@ -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);
|
||||
@@ -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 (
|
||||
|
||||
@@ -0,0 +1,23 @@
|
||||
using System.Runtime.CompilerServices;
|
||||
|
||||
namespace KArtSell.BuildingBlocks.Data;
|
||||
|
||||
/// <summary>
|
||||
/// 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.
|
||||
/// </summary>
|
||||
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;
|
||||
}
|
||||
}
|
||||
@@ -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<AuditEvent>(sql, parameters)).ToList();
|
||||
var raw = await db.QueryAsync<AuditEventRaw>(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<AuditEvent>(sql, new { EventId = eventId });
|
||||
var raw = await db.QuerySingleOrDefaultAsync<AuditEventRaw>(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<Dictionary<string, object>>(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; }
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
@@ -255,12 +297,11 @@ public class AuditSql
|
||||
const string sql = """
|
||||
UPDATE compliance.audit_events
|
||||
SET details = jsonb_set(
|
||||
COALESCE(details, '{}'::jsonb),
|
||||
'{actor_email}',
|
||||
'"<redacted>"'::jsonb
|
||||
),
|
||||
details = jsonb_set(
|
||||
details,
|
||||
jsonb_set(
|
||||
COALESCE(details, '{}'::jsonb),
|
||||
'{actor_email}',
|
||||
'"<redacted>"'::jsonb
|
||||
),
|
||||
'{customer_id}',
|
||||
'"<purged>"'::jsonb
|
||||
),
|
||||
|
||||
@@ -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
|
||||
)
|
||||
|
||||
@@ -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
|
||||
|
||||
Reference in New Issue
Block a user