fix: resolve DEBT-017 duplicate ApprovalWorkflow implementation
Adopt Features/ApprovalWorkflow/ (wired into Program.cs, reachable over
HTTP) as the sole VS-26 (formerly VS-03) maker-checker approval slice.
Delete the dead, [DontRegister]'d duplicate under
ApprovalWorkflow/ (Workstream H) and its dedicated test file, which had
been misleadingly credited with "20/20 tests PASS" while being
unreachable at runtime.
- Sql.cs: fix the same Dapper DateOnly-parameter-binding bug that was
already found and fixed in the now-deleted implementation
(commit 2ccf74c) but had not been ported to this one; InsertProposalAsync
would have failed 100% of the time against a real database.
- tests/.../ApprovalWorkflow/ApprovalWorkflowTests.cs: new Handler+Sql+
real-Postgres integration coverage (create/approve/activate role
gating, maker!=checker separation of duties, evidence attachment,
DateOnly round-trip, list filtering) replacing the deleted dead-code
suite at the same path.
- ApprovalWorkflowPolicyTests.cs: extended (5->10 cases) rather than
replaced, since it already tested the kept implementation's Policy.
- Program.cs: drop the reference comment to the deleted namespace.
- TECH_DEBT_REGISTER.md: DEBT-017 marked Completed (DB verification
pending); corrected stale DEBT-023 to point at this resolution;
registered two residual gaps discovered (not introduced) by this
cleanup as DEBT-025 (no GET /approvals/{id}, evidence unreachable via
HTTP) and DEBT-026 (no wired Draft->Proposed transition, so the
approve/activate path is currently unreachable end-to-end via HTTP).
- WBS_PROGRESS_TRACKER.csv / CURRENT_ROADMAP.md: AEG-VS-26-01 kept
BLOCKED, not COMPLETED — no PostgreSQL was reachable in this session
(127.0.0.1:5432 connection refused), so the 8 new integration tests
are unverified; only the 10 pure-Policy tests were confirmed passing.
Cherry-picked cedc8d7/8c777df from docs/wbs-tracker-current-state onto
this worktree branch first, to bring in the VS-26 renumbering and
ADR-WBS-001 that this task's brief assumed already existed.
dotnet build -c Release: 0 errors/0 warnings.
dotnet test --filter "FullyQualifiedName~ApprovalWorkflow" -c Release:
10 passed (Policy, no DB), 15 failed (DB connection refused - includes
6 unrelated pre-existing tests matched by the filter substring).
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This commit is contained in:
@@ -1,9 +1,27 @@
|
||||
# VS-03: Model Approval Workflow (Maker-Checker Governance)
|
||||
# VS-26 (formerly VS-03): Model Approval Workflow (Maker-Checker Governance)
|
||||
|
||||
## Overview
|
||||
|
||||
This slice implements a maker-checker approval workflow for model activation with separation of duties and immutable audit trail.
|
||||
|
||||
**This is now the sole implementation of this slice.** A second, functionally-overlapping copy
|
||||
(`src/KArtSell.Modules.ModelOperations/ApprovalWorkflow/`, no `Features/` prefix) existed
|
||||
alongside this one from 2026-08-07 to 2026-08-08; it was dead code (all 4 endpoints
|
||||
`[DontRegister]`'d to avoid a duplicate-route crash at Host startup) despite having 20/20
|
||||
passing tests, while *this* implementation — the one actually wired into `Program.cs` and
|
||||
reachable over HTTP — had no dedicated tests. See `TECH_DEBT_REGISTER.md` DEBT-017 and
|
||||
`docs/DECISIONS/ADR-WBS-001-slice-renumbering.md` for the full history. The old implementation
|
||||
and its test file were deleted on 2026-08-08 once this one gained equivalent integration-test
|
||||
coverage (`tests/KArtSell.Integration.Tests/ApprovalWorkflow/ApprovalWorkflowTests.cs`).
|
||||
|
||||
**Bug fixed 2026-08-08 (as part of DEBT-017):** `InsertProposalAsync` in `Sql.cs` passed
|
||||
`EffectiveAt` (a `DateOnly`) directly as a Dapper parameter. The now-deleted implementation hit
|
||||
the identical failure against a real database (commit `2ccf74c`) — Npgsql/Dapper in this
|
||||
environment cannot bind a bare `DateOnly` value; it needs an explicit `::date` cast plus a
|
||||
`"yyyy-MM-dd"` string parameter. That fix has been ported here. **This has not been re-verified
|
||||
against a live PostgreSQL instance in this session** (none was reachable) — see "Test status"
|
||||
below.
|
||||
|
||||
## Architecture
|
||||
|
||||
### State Machine
|
||||
@@ -52,6 +70,16 @@ PROPOSED (maker submits)
|
||||
- POST /approvals (create proposal)
|
||||
- GET /approvals (list proposals)
|
||||
- POST /approvals/{id}/approve (approve proposal)
|
||||
- ⚠️ **No `GET /approvals/{id}`.** The deleted duplicate implementation had a single-proposal
|
||||
fetch endpoint that included the evidence list in its response; this implementation has no
|
||||
equivalent, so evidence attached during approval is currently unreachable via HTTP (it can
|
||||
only be read back through `ApprovalWorkflowSql` directly, e.g. in tests). Not fixed here —
|
||||
out of scope for DEBT-017 (duplicate-implementation cleanup); tracked as **DEBT-025**.
|
||||
- ⚠️ **No wired "submit for review" transition.** `ApprovalWorkflowPolicy.CanProposeForReview`
|
||||
exists but no `Handler` or `Endpoint` calls it, so nothing in the running application ever
|
||||
moves a proposal from `Draft` to `Proposed`. `ActivateModelHandler` and `ApproveApprovalHandler`
|
||||
both require `Proposed`/`Approved` respectively, so as shipped a created proposal cannot
|
||||
reach `Approved` through the HTTP API alone. Also not fixed here — tracked as **DEBT-026**.
|
||||
|
||||
## API Contracts
|
||||
|
||||
@@ -178,15 +206,25 @@ CREATE TABLE model_operations.approval_events (
|
||||
|
||||
## Tests
|
||||
|
||||
Unit tests cover:
|
||||
- RBAC enforcement (Maker, Checker, SRE roles)
|
||||
- Separation of duties (Checker ≠ Maker)
|
||||
- State machine transitions
|
||||
- RBAC violations
|
||||
- `tests/KArtSell.Integration.Tests/ApprovalWorkflowPolicyTests.cs` — pure `ApprovalWorkflowPolicy`
|
||||
unit tests (no DB): RBAC enforcement (Maker/Checker/SRE), separation of duties, valid/invalid
|
||||
state transitions. Fast, deterministic.
|
||||
- `tests/KArtSell.Integration.Tests/ApprovalWorkflow/ApprovalWorkflowTests.cs` — Handler + Sql +
|
||||
real PostgreSQL integration tests: create (role-gated), approve (maker≠checker, evidence
|
||||
attachment), activate (SRE-gated), `EffectiveAt` `DateOnly` round-trip through a real `date`
|
||||
column, list filtering.
|
||||
|
||||
**Test status as of 2026-08-08 (DEBT-017 resolution session): written but not run against a live
|
||||
database.** No PostgreSQL was reachable at `127.0.0.1:5432` in that session (no SSH tunnel to
|
||||
178.104.200.7 open). `dotnet build -c Release` was confirmed green; `dotnet test --filter
|
||||
"FullyQualifiedName~ApprovalWorkflow"` was run and its actual outcome (pass, fail, or DB
|
||||
connection error) is recorded in `TECH_DEBT_REGISTER.md` DEBT-017 and
|
||||
`docs/CURRENT/CATALOGS/WBS_PROGRESS_TRACKER.csv` row `AEG-VS-26-01` — check those before treating
|
||||
this slice as verified.
|
||||
|
||||
Run tests:
|
||||
```bash
|
||||
dotnet test --filter "ApprovalWorkflowPolicyTests"
|
||||
dotnet test --filter "FullyQualifiedName~ApprovalWorkflow" -c Release
|
||||
```
|
||||
|
||||
## AGENTS.md v16.0 Compliance
|
||||
@@ -194,25 +232,37 @@ dotnet test --filter "ApprovalWorkflowPolicyTests"
|
||||
- ✅ **SOLID:** Separate Endpoint/Handler/Policy/Sql per operation
|
||||
- ✅ **Complexity:** Each handler ≤200 lines
|
||||
- ✅ **Audit:** All state changes logged with correlation_id
|
||||
- ✅ **Necessity:** Grounded in VS-03 SLICE_SPEC
|
||||
- ✅ **Normalization:** 3NF schema, append-only events
|
||||
- ✅ **Necessity:** Grounded in VS-26 (formerly VS-03) SLICE_SPEC
|
||||
- ⚠️ **Normalization:** Writes mutate `approval_proposals` in place (`UPDATE ... revision =
|
||||
revision + 1`) rather than appending a new revision row, because `id` is the sole `PRIMARY KEY`
|
||||
in migration `0036_approval_workflow.sql` (no `(id, published_at)` composite key) — an
|
||||
append-only INSERT would violate that constraint on the second write. This is a real deviation
|
||||
from CLAUDE.md's "new state appended as new revision" rule; it is pre-existing (present before
|
||||
this session) and schema-level, so fixing it is out of scope for DEBT-017. `GetProposalAsync`
|
||||
correspondingly has no `published_at <= cutoff` PIT filter, since there is only ever one row.
|
||||
- ✅ **Simplicity:** State machine clearly visible
|
||||
- ✅ **Pattern:** Vertical Slice standard
|
||||
- ✅ **Guardrails:** RBAC enforced, no privilege escalation
|
||||
- ✅ **Traceability:** Correlation_id + evidence linking
|
||||
- ✅ **Safety:** Idempotent, rollback-safe
|
||||
- ⚠️ **Safety:** DateOnly parameter binding bug fixed 2026-08-08; unverified against a live DB
|
||||
this session (see "Test status" above)
|
||||
- ✅ **Maturity:** Spec complete before code
|
||||
- ✅ **Right-Way:** No shortcuts, formal approval workflow
|
||||
- ✅ **Debt:** No new tech debt
|
||||
- ✅ **Right-Way:** Duplicate implementation resolved per DEBT-017, not worked around
|
||||
- ⚠️ **Debt:** DEBT-017 (duplicate implementation) resolved; two residual gaps discovered by this
|
||||
cleanup (predate it, not introduced by it) are registered as DEBT-025 (no `GET /approvals/{id}`)
|
||||
and DEBT-026 (no wired Draft→Proposed transition)
|
||||
|
||||
## Related Specifications
|
||||
|
||||
- **VS-00:** PIT envelope (published_at, correlation_id, revision)
|
||||
- **VS-02:** Governance foundation (data sources, policies)
|
||||
- **VS-04:** Audit trail (events logged by this slice)
|
||||
- **VS-27 (formerly VS-04):** Audit trail (events logged by this slice)
|
||||
- **Compliance:** Maker-checker separation, evidence linkage
|
||||
|
||||
---
|
||||
|
||||
**Status:** ✅ IMPLEMENTATION COMPLETE
|
||||
**Co-Authored-By:** Claude Haiku 4.5 <noreply@anthropic.com>
|
||||
**Status:** Backend implementation is the canonical (sole) copy of this slice as of 2026-08-08;
|
||||
integration tests exist but are unverified against a live database (see "Test status"). Not
|
||||
"IMPLEMENTATION COMPLETE" until that verification runs and the two residual gaps above are
|
||||
resolved or explicitly accepted.
|
||||
**Co-Authored-By:** Claude Sonnet 5 <noreply@anthropic.com>
|
||||
|
||||
@@ -55,7 +55,7 @@ public class ApprovalWorkflowSql
|
||||
INSERT INTO model_operations.approval_proposals
|
||||
(id, model_id, status, created_by, created_at, justification, effective_at,
|
||||
published_at, revision, correlation_id)
|
||||
VALUES (@id, @modelId, @status, @createdBy, @createdAt, @justification, @effectiveAt,
|
||||
VALUES (@id, @modelId, @status, @createdBy, @createdAt, @justification, @effectiveAt::date,
|
||||
@publishedAt, @revision, @correlationId)
|
||||
RETURNING id
|
||||
""";
|
||||
@@ -69,7 +69,11 @@ public class ApprovalWorkflowSql
|
||||
proposal.CreatedBy,
|
||||
proposal.CreatedAt,
|
||||
proposal.Justification,
|
||||
proposal.EffectiveAt,
|
||||
// Dapper cannot bind DateOnly directly as an Npgsql parameter value (see TECH_DEBT_REGISTER.md
|
||||
// DEBT-017); the identical failure was found and fixed the same way in the now-deleted
|
||||
// ApprovalSql.cs (commit 2ccf74c) after it crashed 100% of proposal-creation calls against a
|
||||
// real database.
|
||||
effectiveAt = proposal.EffectiveAt.ToString("yyyy-MM-dd"),
|
||||
proposal.PublishedAt,
|
||||
proposal.Revision,
|
||||
proposal.CorrelationId
|
||||
|
||||
Reference in New Issue
Block a user