Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 30 additions & 0 deletions DECISIONS/2026-10-26-art-milestone-0.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
# Decision: ART Milestone 0 approval

Status: Decided
Date: 2026-10-26
Decided by: Amarnath Rasineni (product owner)
Source: `docs/art/milestone-0-assessment.md` §6, PR #85, issue #53

## Approved

- **ADR-0002**, transactional staged import: option A (an API-owned transaction with COPY to staging, then promote). Option D (background worker) is the documented upgrade path.
- **ADR-0003**, customer-facing multi-tenant scope, including authentication by OIDC against a deployment-controlled IdP, with a local-account fallback for air-gapped installs.
- **The M1–M7 milestone plan.**

## The nine open decisions, all taken at the recommended default

| # | Decision | Outcome |
|---|---|---|
| 1 | Max file size and concurrency | 50 MB and 100,000 rows; 4 concurrent imports per API instance |
| 2 | Repeated uploads of the same filename | Versioned datasets (`<name>_v<n>`); never overwrite |
| 3 | Duplicate-record rule and policy | Exact whole-row match; policy **flag**, the user decides per row |
| 4 | Partial imports | Not permitted. All approved rows import or none do |
| 5 | Uploader approving own data | Not allowed by default; per-tenant setting for single-user tenants |
| 6 | Tenant model | Schema per tenant |
| 7 | Classification and retention | Design target OFFICIAL: Sensitive. Raw uploads kept until a recorded purge. Audit kept 7 years by default (configurable; confirm against the client's records authority) |
| 8 | Performance targets and environment | 100,000-row import under 60 s; first preview page under 2 s on 2 vCPU / 4 GB. Azure Container Apps plus on-prem docker compose. BR-22 stays deferred |
| 9 | PR #40 and csv-table-hub PR #1 | Close both (outside the React/TS constraint) |

## Unchanged

ADR-0001, the "production-grade" README framing, and the deferral of BR-02, BR-21 and BR-22.
51 changes: 49 additions & 2 deletions docs/VCRM.md
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,28 @@ Test-layer codes used in column headers below:
| **BR-19** | The build, test, and eval layers shall be **physically segregated** so a change to one cannot inadvertently break the others' contract. | `docs/ARCHITECTURE.md` |
| **BR-20** | The full SQL test suite shall reach **142 of 142 assertions passing** (100.0 % pass rate) on every release. | README §"142 assertions", `tests/run_all_tests.sql` |

### ART extension requirements (added 2026-10-26)

Added by ART Milestone 0 (#53) and approved by the product owner on 2026-10-26. Source: [`docs/art/milestone-0-assessment.md`](art/milestone-0-assessment.md), [ADR-0002](adr/0002-transactional-staged-import.md), [ADR-0003](adr/0003-customer-facing-multi-tenant.md), [`DECISIONS/2026-10-26-art-milestone-0.md`](../DECISIONS/2026-10-26-art-milestone-0.md).

| ID | Requirement | Source |
|----|------------|--------|
| **BR-23** | Every upload shall be registered with a unique ID, original filename, SHA-256, uploader, tenant, timestamp and processing status. | ART v2 |
| **BR-24** | The original file and its parsed source rows shall be preserved immutably, separate from edits and approved data. | ART v2, D2 |
| **BR-25** | Users shall be able to page through every row and column of an upload, with inferred types and validation results. | ART v2 |
| **BR-26** | Rows shall be includable or excludable; each decision records actor and reason. | ART v2 |
| **BR-27** | Cell edits shall be stored as versions over the original, with old and new value, actor, time and reason. | ART v2, D2 |
| **BR-28** | Exact-file duplicates shall be detected by SHA-256 within and across uploads, per tenant. | ART v2 |
| **BR-29** | Duplicate records shall be detected within and across uploads using a configurable rule (default: exact match, flagged). | ART v2, decision 3 |
| **BR-30** | Table names shall be safely derived from filenames, and repeated uploads of a filename create versioned datasets without overwriting. | ART v2, decision 2 |
| **BR-31** | Imports shall check schema compatibility and never make destructive schema changes. | ART v2 |
| **BR-32** | Imports shall require submission and an approve or reject decision with a recorded approver; by default an uploader cannot approve their own data. | ART v2, D3, decision 5 |
| **BR-33** | Imports shall be staged, transactional and idempotent with safe retries; every imported row traces to its source row and approved version; partial imports are not permitted. | ART v2, D4, ADR-0002, decision 4 |
| **BR-34** | Rows in, excluded, edited and imported shall reconcile exactly and be persisted per import. | ART v2 |
| **BR-35** | Audit events shall be append-only at database level; reports shall be filterable; exports shall neutralise spreadsheet formula injection. | ART v2, D7 |
| **BR-36** | Users shall authenticate; access shall be role-based; no tenant shall read or affect another tenant's data. | ART v2, D1, ADR-0003, decision 6 |
| **BR-37** | Data shall stay inside the deployment boundary, with no runtime external services, and the stack shall install offline. | ART v2, D6, decision 7 |

### Out of scope (recorded for completeness)

| ID | Requirement | Status / why |
Expand Down Expand Up @@ -113,6 +135,28 @@ The `input_data/` loader has its own implicit requirements — verified end-to-e
| BR-D4 | Duplicate primary keys in source are reported, not silently dropped | 4 (duplicate detection) | ✅ |
| BR-D5 | Loaded data is browsable (sample peek) | 1 (sample rows) | ✅ |

### ART extension traceability (BR-23 to BR-37)

Status as assessed on 2026-10-26 at `main` 032b522. None of these is "Verified" until a test run proves it (D5).

| ID | Status | Reason | Planned verification | Milestone |
|----|--------|--------|----------------------|-----------|
| BR-23 | ⚠️ Partial | Registry exists without uploader, tenant or status | Integration + DB constraint tests | M1, M3 |
| BR-24 | ❌ Not started | Original never stored; overwrite drops data | DB negative tests | M3 |
| BR-25 | ⚠️ Partial | 10-row preview sample; 200-row cap on table view | API paging + e2e | M4 |
| BR-26 | ❌ Not started | No code | API + e2e | M4 |
| BR-27 | ❌ Not started | No code | API + DB tests | M4 |
| BR-28 | ⚠️ Partial | Global, not per tenant; race guard missing (#81) | Concurrency + cross-tenant tests | M1, M5 |
| BR-29 | ⚠️ Partial | In-file exact match only | Unit + integration | M5 |
| BR-30 | ⚠️ Partial | Content-derived names; repeats refused or destructive | Unit + integration | M5 |
| BR-31 | ⚠️ Partial | T&E mode only; overwrite drops tables | Integration | M5 |
| BR-32 | ❌ Not started | No code | API + e2e | M6 |
| BR-33 | ⚠️ Partial | Single-transaction load; no staging or lineage | Integration incl. failure injection | M6 |
| BR-34 | ⚠️ Partial | Counts returned, not persisted; extra cells dropped (#82) | Integration | M1, M6 |
| BR-35 | ⚠️ Partial | Plain audit table, no actor, no filters; export formula injection (#83) | DB negative tests + unit | M1, M3, M7 |
| BR-36 | ❌ Not started | Shared API key only | Cross-tenant negative tests | M2 |
| BR-37 | ⚠️ Partial | No external calls found; no automated check; no offline doc (#60) | Egress-blocked run + offline dry run | M7 |

---

## 3. Coverage summary
Expand All @@ -133,10 +177,13 @@ The `input_data/` loader has its own implicit requirements — verified end-to-e
| Status | Count | Requirements |
|--------|-------|-------------|
| ✅ Verified | 19 | BR-01, BR-03, BR-04, BR-05, BR-06, BR-07, BR-08, BR-09, BR-10, BR-11, BR-12, BR-13, BR-14, BR-15, BR-16, BR-17, BR-18, BR-19, BR-20 |
| ⚠️ Partial | 0 | — |
| ⚠️ Partial | 10 | BR-23, BR-25, BR-28, BR-29, BR-30, BR-31, BR-33, BR-34, BR-35, BR-37 (ART, added 2026-10-26) |
| ❌ Not started | 5 | BR-24, BR-26, BR-27, BR-32, BR-36 (ART, added 2026-10-26) |
| ⏸ Deferred by decision | 3 | BR-02, BR-21, BR-22 |

**Headline:** **19 of 22 (86 %)** business requirements are fully verified by at least one automated test condition, up from 17 of 22 (77 %).
**Headline (2026-10-26):** with the 15 ART requirements added, **19 of 37 (51 %)** are verified, 3 are deferred by decision, and the other 15 are partial or not started.

**Headline before ART:** **19 of 22 (86 %)** business requirements are fully verified by at least one automated test condition, up from 17 of 22 (77 %).

**There are no longer any unverified requirements that are not a recorded decision.** The three remaining are deferred deliberately: BR-02 by ADR-0001 (PostgreSQL first), BR-21 and BR-22 by earlier project decisions. That distinction matters in review — an unverified requirement is a gap, a deferred one is a choice with a revisit trigger.

Expand Down
71 changes: 71 additions & 0 deletions docs/adr/0002-transactional-staged-import.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,71 @@
# ADR-0002 — Transactional, staged, approved import

- **Status:** Accepted (product owner, 2026-10-26)
- **Date:** 2026-10-26
- **Decision maker:** Amarnath Rasineni
- **Affects:** BR-24, BR-32, BR-33, BR-34, BR-35; `api/services/dynamic_loader.py`, `api/services/te_loader.py`, `api/services/audit.py`
- **Supersedes:** the post-commit upload-audit design documented in `api/services/audit.py` (O-04)

---

## Context

Decision D4 (2026-10-25) requires imports to be staged, approved and transactional, with every imported row traceable to its approved version.

Today an upload is loaded the moment it arrives:

- **Dynamic mode** parses, casts and inserts in one psycopg2 transaction, then commits (`dynamic_loader.py:131-356`).
- **T&E mode** inserts with savepoints so failing rows are skipped and the rest commit (`te_loader.py`, commit at line 301). Partial import is by design.
- **Audit row** is written afterwards in a separate transaction (`csv_routes.py:86-93`). A crash between the two leaves an import with no audit row. A failed audit write is logged, never raised (`audit.py:9-22`).

There is no staging, no approval step and no link from an imported row back to its source row.

**Correction to the ART prompt.** The prompt assumes the API shells out to the bash loaders. It does not: `api/` contains no `subprocess` calls. The bash loaders in `build/csv/` serve only the batch pipeline and e2e tests. So the choice is about restructuring Python code the API already owns, not about wrapping shell scripts.

## Options

### A. API-owned transaction: COPY to staging, then promote (recommended)

1. **On upload:** store the original bytes and parsed source rows (insert-only tables). No target-table writes.
2. **During review:** record decisions and edits as rows, never updates.
3. **On approved import:** one transaction does all of the following, then commits:
- `COPY` the effective rows (source, minus exclusions, plus the latest edits) into a staging table using psycopg2 `copy_expert`.
- Validate types and keys in SQL.
- `INSERT ... SELECT` into the versioned target table with `_source_row_id` and `_import_id`.
- Write the `imports` record with reconciliation counts.
- Write the audit events.
4. **Idempotency:** a client-supplied `Idempotency-Key`, with a UNIQUE constraint on `imports(upload_id, approval_id)`. A retry after a failure re-runs cleanly; a retry after success returns the original result.

- **For:** one atomic unit, so data, lineage and audit commit together or not at all. COPY is the fastest load path in PostgreSQL. It reuses `csv_parse.py` and the current identifier safety. There is nothing new to deploy.
- **Against:** the largest change to the loaders. Long imports hold one transaction open, which needs a statement timeout and a row cap (the current 100,000-row cap stays).

### B. Thread the connection through the existing loaders

Pass the caller's cursor into `upload_dynamic` and `upload_te`, so the audit INSERT joins the load transaction.

- **For:** small, contained change. It closes the post-commit audit window.
- **Against:** it covers none of staging, approval, lineage or edits. It would only be a step on the way to A.

### C. Bash loaders write to staging, API promotes

- **Against:** it adds a shell-out the API does not have today, splits one transaction across two processes, and puts untrusted CSV through bash. Rejected.

### D. Background import worker with a job table

The import runs in a worker process, with the job state in PostgreSQL. Promotion is transactional as in A.

- **For:** handles large files and concurrency without tying up API requests.
- **Against:** a new process to deploy and monitor, which counts against the air-gapped simplicity goal. Not needed at the current 100,000-row cap.

## Decision

**Adopt A.** Keep D as the documented upgrade path if decision 1 (file size and concurrency) later rises beyond what one request transaction can handle.

T&E mode moves to the same flow. Its row-level error isolation becomes a review-time result (rows flagged and excluded before approval) instead of a load-time skip. That honours decision 4's recommended default of no partial imports.

## Consequences

- `audit.py`'s "known limitation" goes away. `record_audit` (separate transaction) is retired in favour of in-transaction writes.
- Loader signatures change. The existing `/api/csv/upload` stays as a compatibility path until the frontend moves to the review flow, and is then removed in a later milestone.
- Needs Alembic migrations first (assessment finding F4).
- Tests must include failure injection mid-import (no partial target rows, no audit row), a retry after failure, and a retry after success (no duplicate rows).
63 changes: 63 additions & 0 deletions docs/adr/0003-customer-facing-multi-tenant.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
# ADR-0003 — Customer-facing, multi-tenant scope

- **Status:** Accepted (product owner, 2026-10-26)
- **Date:** 2026-10-26
- **Decision maker:** Amarnath Rasineni
- **Affects:** BR-23 to BR-37 (new); `api/auth.py`, `api/config.py`, `api/db.py`; the frontend; deployment docs
- **Records:** decisions D1 to D3 taken on 2026-10-25 (see `docs/prompts/ART_FRAMEWORK.md`)

---

## Context

The platform was built as an **internal T&E migration tool for one operator**:

- One shared `X-API-Key` (`api/auth.py:21-26`), optional in `dev`.
- One uploads schema for everything (`settings.UPLOADS_SCHEMA`).
- No users, roles or tenants. The audit log records actions without an actor.
- Loads happen immediately on upload.
- The frontend's audit view reads browser `localStorage`.

On 2026-10-25 the product owner decided:

- **D1:** the platform becomes customer-facing with multi-tenant isolation.
- **D2:** full-dataset cell editing is in scope, stored as versions.
- **D3:** an approval workflow is in scope.

This ADR records that positioning change and what follows from it.

## Decision

The platform is repositioned from a single-operator internal tool to a **multi-tenant web application used by external customers**. It keeps its two import modes (freeform and strict T&E schema) and its T&E/V&V framing.

## Consequences

**Identity and access (BR-36)**
- Per-user authentication replaces the shared key for interactive use. The key gate can remain for service-to-service calls.
- Roles at minimum: *uploader*, *reviewer/approver*, *tenant admin*, *auditor (read-only)*.
- The sovereignty constraint (D6) rules out SaaS identity providers at runtime. Approved approach (2026-10-26): OIDC against an IdP the deployment controls, plus a local-account fallback for air-gapped installs.

**Tenant isolation (BR-36)**
- Approved model (2026-10-26): schema per tenant (Milestone 0, decision 6). Dynamic tables already live in a configurable schema, so this extends an existing pattern.
- Every request resolves to exactly one tenant, and the database session is scoped to it.
- Cross-tenant read and write attempts are standing negative tests from Milestone 2 onwards.

**Data handling (BR-24, BR-37)**
- Customer data must be preserved: the original file and source rows are kept, edits are versioned, and nothing is overwritten. The current `overwrite` path, which drops the prior table, is retired.
- Classification and retention rules become per-deployment configuration (decision 7, #49).

**Workflow (BR-26, BR-27, BR-32)**
- Upload no longer means import. Data passes review and approval first (ADR-0002).
- Separation of duties defaults to "the uploader cannot approve" (decision 5).

**Audit (BR-35)**
- Every event carries tenant and actor. The trail becomes append-only at database level, and reports come from the server, not the browser.

**What does not change**
- ADR-0001 (PostgreSQL first) and the deferred BR-02, BR-21 and BR-22 stay as they are.
- The README's "production-grade" framing stays (risk accepted, 2026-10-04).
- The React/TypeScript frontend (`csv-table-hub-main/`) remains the baseline. The plain-HTML frontend (PR #40) is outside this scope (decision 9, #50).

**Risk**
- Customer-facing use raises the cost of a defect. Assessment findings F1 to F4 should be fixed in Milestone 1 before any customer data arrives.
- A security review of the threat model is recommended before the first external tenant.
Loading
Loading