Auto-sync .kiro/ from master (post-checkout hook)

This commit is contained in:
Jordan Ramos
2026-05-19 15:01:25 -06:00
parent ada9df26a8
commit 8ebd7e4d5e
23 changed files with 3485 additions and 19 deletions

View File

@@ -0,0 +1,179 @@
# Implementation Plan
## Overview
Five compliance code paths share the root cause "key by `compliance_uploads.id` instead of by `compliance_uploads.report_date`": `GET /trends`, `GET /top-recurring`, `GET /category-trend`, `GET /summary`, and the `compliance_snapshots` block inside `persistUpload()`. All fixes are contained to `backend/routes/compliance.js` and require no schema migration, no new column, and no frontend change.
The plan follows the bugfix workflow's exploratory methodology: a single property-based test file (`backend/__tests__/compliance-duplicate-chart-entries.property.test.js`) is written before any fix, with one test case per affected site demonstrating the bug condition, plus preservation cases observed on the unfixed code. Each fix is then implemented as its own task and verified by re-running the matching test case from the exploration suite. The plan ends with a regression checkpoint that re-runs the full preservation suite and the backend test suite.
## Task Dependency Graph
```json
{
"waves": [
{ "id": 0, "tasks": ["1", "2"] },
{ "id": 1, "tasks": ["3.1", "4.1", "5.1", "6.1", "7.1"] },
{ "id": 2, "tasks": ["3.2", "4.2", "5.2", "6.2", "7.2"] },
{ "id": 3, "tasks": ["8"] },
{ "id": 4, "tasks": ["9"] }
]
}
```
Wave 0 establishes the test baseline: task 1 documents Property 1 (bug condition) failures on the unfixed code and task 2 captures Property 2 (preservation) baseline outputs. Wave 1 implements the five independent fixes in `backend/routes/compliance.js` (no inter-fix dependencies — each touches a different SQL statement). Wave 2 verifies each fix's slice of Property 1 now passes. Wave 3 re-runs the full Property 2 suite to confirm no regressions across the five sites. Wave 4 is the final checkpoint that runs the entire backend test suite.
## Tasks
- [x] 1. Write bug condition exploration property test
- **Property 1: Bug Condition** - Multi-Vertical Date Aggregation Across Five Compliance Sites
- **CRITICAL**: This test MUST FAIL on unfixed code — failure confirms the bug exists across all five sites
- **DO NOT attempt to fix the test or the code when it fails**
- **NOTE**: This test encodes the expected behavior — it will validate the fixes when it passes after implementation
- **GOAL**: Surface counterexamples that demonstrate the bug exists for each of the five affected code paths (`/trends`, `/top-recurring`, `/category-trend`, `/summary`, `persistUpload()` snapshot block)
- **Scoped PBT Approach**: Scope the property to the canonical bug-condition fixture (`fixture_multi_vertical_single_date` from design.md) — three uploads for `2025-05-11`, one each for `NTS_AEO`, `SDIT_CISO`, `TSI`, with distinct counts and matching items per upload — this reproduces the original GitLab #12 scenario deterministically
- Bug Condition (from design.md): `EXISTS report_date d WHERE COUNT(compliance_uploads WHERE report_date = d) > 1`
- Create `backend/__tests__/compliance-duplicate-chart-entries.property.test.js` using `fast-check` and the existing pg pool mock pattern from `vcl-compliance-reporting.property.test.js`
- Test case 1.A — `/trends` duplicate-date counterexample: seed three uploads for `2025-05-11` (verticals NTS_AEO/SDIT_CISO/TSI), call `GET /trends`, assert `response.trends.filter(t => t.report_date === '2025-05-11').length === 1` AND `new_count` for that date equals the sum of the three uploads' `new_count` values (likewise `recurring_count`, `resolved_count`, `total_active`, and per-team counts)
- Test case 1.B — `/top-recurring` duplicate-bar counterexample: same fixture, call `GET /top-recurring`, assert exactly one waterfall entry per unique `report_date` AND the running invariant `entry[i].end === entry[i].start + entry[i].new_count + entry[i].recurring_count - entry[i].resolved_count` holds for every `i` AND `entry[i].start === entry[i-1].end` for adjacent entries (with `entry[0].start === 0`)
- Test case 1.C — `/category-trend` duplicate (date, category) counterexample: same fixture plus items tagged with two categories (`Patching` and `Configuration`), call `GET /category-trend`, assert `response.categoryTrend.filter(c => c.report_date === '2025-05-11' && c.category === 'Patching').length === 1` AND each entry's `count` equals the total `compliance_items` for that category across every upload sharing the date
- Test case 1.D — `/summary` sibling-disclosure counterexample: same fixture (`2025-05-11` is the latest date), call `GET /summary`, assert either (a) `entries` is the merged view of all three uploads OR (b) `response.multi_vertical_uploads` is a non-empty array with `length === 2` listing the other two uploads' ids and verticals
- Test case 1.E — `persistUpload()` cross-vertical contamination counterexample: pre-populate `compliance_items` with disjoint sets for two verticals (e.g., NTS_AEO has 100 active items, SDIT_CISO has 50 active items), call `persistUpload()` for a fresh SDIT_CISO upload, read back the `compliance_snapshots` row for the current month with `vertical = 'SDIT_CISO'`, assert `total_devices` reflects only SDIT_CISO items and is not inflated by NTS_AEO items
- Wrap each test case in fast-check `fc.assert` against `arbScenario` from design.md (uploads with possibly colliding `report_date`, items referencing those uploads) so PBT also exercises larger random fixtures beyond the canonical 3-upload case
- Add fixture builders in the test file matching the design's `fixture_multi_vertical_single_date` and `fixture_cross_vertical_items`
- Run test on UNFIXED code: `npm run test:backend -- compliance-duplicate-chart-entries.property.test.js`
- **EXPECTED OUTCOME**: All five test cases FAIL (this is correct — it proves each manifestation of the bug exists)
- Document the counterexamples found (e.g., `/trends` returns 3 entries for `2025-05-11` instead of 1, `/summary` returns one upload's `summary_json` and silently drops the other two, `compliance_snapshots.total_devices` for SDIT_CISO equals 150 instead of 50)
- Mark task complete when test is written, run, and the failures for all five test cases are documented
- _Requirements: 1.1, 1.2, 1.3, 1.4, 1.5, 1.6, 1.7, 1.8, 1.9, 1.10, 1.11_
- [x] 2. Write preservation property tests (BEFORE implementing fix)
- **Property 2: Preservation** - Single-Upload-Per-Date Behavior Unchanged Across Five Sites
- **IMPORTANT**: Follow observation-first methodology — capture pre-fix outputs and assert equality post-fix
- Bug Condition negation (from design.md): `FORALL report_date d, COUNT(compliance_uploads WHERE report_date = d) <= 1`
- Add preservation test cases to `backend/__tests__/compliance-duplicate-chart-entries.property.test.js`
- Observe baseline behavior on UNFIXED code using the design's preservation fixtures and capture exact response bodies (snapshot-test style)
- Test case 2.A — Empty-state preservation (`fixture_empty`): no `compliance_uploads`, no `compliance_items`. Observe `GET /trends` returns `{ trends: [] }`, `GET /top-recurring` returns `{ waterfall: [] }`, `GET /category-trend` returns `{ categoryTrend: [] }`, `GET /summary` returns `{ entries: [], overall_scores: {}, upload: null }`. Capture and assert these exact shapes
- Test case 2.B — Single AEO-legacy-upload preservation (`fixture_single_upload_aeo_legacy`): one upload with `vertical IS NULL`, `report_date = '2025-04-01'`, ~20 items across the four teams. Observe responses from all four read endpoints, capture them, and assert byte-for-byte equality
- Test case 2.C — Multiple single-upload-per-date preservation (`fixture_single_upload_per_date`): five uploads on five distinct dates with varied `vertical` values. Observe responses from all four read endpoints and assert equality
- Test case 2.D — `/summary` `team` query parameter preservation: with the latest upload present, assert `?team=STEAM` filters `entries` server-side AND `?team=OTHER` (non-`ALLOWED_TEAMS`) returns HTTP 400. Capture both responses
- Test case 2.E — `persistUpload()` single-vertical-month preservation (`fixture_cross_vertical_items` reduced to one vertical): pre-populate `compliance_items` with rows from a single vertical only, run `persistUpload()` for that vertical, capture the resulting `compliance_snapshots` rows
- Test case 2.F — `persistUpload()` snapshot error-path preservation: force a snapshot query failure (mock `pool.query` to reject on the snapshot statement only), assert the upload still commits and the error is logged but not surfaced (HTTP 200/201, no error response)
- Property-based extension — Cross-endpoint preservation: use fast-check `arbScenario` constrained to scenarios where every `report_date` has exactly one upload row. Assert that for every generated scenario, all four endpoint responses on UNFIXED code match the captured-baseline shape and field-level equality holds (this generator covers the design's `fixture_pbt_generators.arbScenario` restricted to the non-bug-condition input space)
- Run tests on UNFIXED code: `npm run test:backend -- compliance-duplicate-chart-entries.property.test.js`
- **EXPECTED OUTCOME**: All preservation test cases PASS (this confirms the baseline behavior to preserve)
- Mark task complete when tests are written, run, and passing on unfixed code
- _Requirements: 3.1, 3.2, 3.3, 3.4, 3.5, 3.6, 3.7, 3.8, 3.9, 3.10_
- [x] 3. Fix `/trends` — aggregate uploads and team counts by `report_date`
- [x] 3.1 Rewrite the `/trends` upload and team queries to group by `report_date`
- In `backend/routes/compliance.js` `router.get('/trends', ...)` (around line 768), replace the `compliance_uploads` query with the `GROUP BY report_date` SQL from design.md Fix 1, summing `new_count`, `recurring_count`, `resolved_count`, and `(new_count + recurring_count) AS total_active`
- Replace the per-team `compliance_items` query with the `JOIN compliance_uploads` + `GROUP BY cu.report_date, ci.team` form from design.md Fix 1
- Change the `teamMap` keyed lookup from `teamMap[u.id]` to `teamMap[u.report_date]` and rebuild `trends` from the per-date upload rows
- _Bug_Condition: isBugCondition(uploads) where two or more compliance_uploads rows share a `report_date`_
- _Expected_Behavior: GET /trends returns one entry per unique report_date with summed count fields and aggregated per-team counts (Property 1 from design)_
- _Preservation: Single-upload-per-date dates produce identical responses; empty-data response remains `{ trends: [] }`; chart components require no changes_
- _Requirements: 2.1, 2.2, 2.3, 3.1, 3.2, 3.3_
- [x] 3.2 Verify the `/trends` portion of bug condition exploration test now passes
- **Property 1: Expected Behavior** - `/trends` Returns One Entry Per Unique report_date
- **IMPORTANT**: Re-run the SAME test case 1.A from task 1 — do NOT write a new test
- Run `npm run test:backend -- compliance-duplicate-chart-entries.property.test.js -t "/trends"`
- **EXPECTED OUTCOME**: Test case 1.A PASSES (confirms `/trends` bug is fixed)
- _Requirements: Property 1 (Validates 2.1, 2.2, 2.3) from design_
- [x] 4. Fix `/top-recurring` — aggregate uploads by `report_date` before passing to `computeWaterfall()`
- [x] 4.1 Rewrite the `/top-recurring` upload query to group by `report_date`
- In `backend/routes/compliance.js` `router.get('/top-recurring', ...)` (around line 818), replace the query with the `GROUP BY report_date` SQL from design.md Fix 2, summing `new_count`, `recurring_count`, `resolved_count`
- Leave `computeWaterfall()` unchanged — it already advances `start` correctly when fed one row per date; the fix is purely in the SQL
- _Bug_Condition: isBugCondition(uploads) where two or more compliance_uploads rows share a `report_date`_
- _Expected_Behavior: GET /top-recurring returns one waterfall entry per unique report_date with summed deltas; running invariant `entry[i].end === entry[i].start + entry[i].new_count + entry[i].recurring_count - entry[i].resolved_count` holds and `entry[i].start === entry[i-1].end` for adjacent entries (Property 2 from design)_
- _Preservation: Single-upload-per-date waterfall is unchanged; empty-data response remains `{ waterfall: [] }`_
- _Requirements: 2.4, 2.5, 3.4_
- [x] 4.2 Verify the `/top-recurring` portion of bug condition exploration test now passes
- **Property 1: Expected Behavior** - `/top-recurring` Has One Bar Per Unique report_date With Correct Running Totals
- **IMPORTANT**: Re-run the SAME test case 1.B from task 1 — do NOT write a new test
- Run `npm run test:backend -- compliance-duplicate-chart-entries.property.test.js -t "/top-recurring"`
- **EXPECTED OUTCOME**: Test case 1.B PASSES (confirms `/top-recurring` bug is fixed and the running invariant holds)
- _Requirements: Property 2 (Validates 2.4, 2.5) from design_
- [x] 5. Fix `/category-trend` — drop `cu.id` from `GROUP BY`
- [x] 5.1 Rewrite the `/category-trend` query to group by `(report_date, category)` only
- In `backend/routes/compliance.js` `router.get('/category-trend', ...)` (around line 838), replace the query with the SQL from design.md Fix 3 — remove `cu.id` from the `GROUP BY` so grouping is by `(cu.report_date, COALESCE(ci.category, 'Unknown'))` only
- Leave the response shape `{ categoryTrend: Array<{ report_date, category, count }> }` unchanged
- _Bug_Condition: isBugCondition(uploads) where two or more compliance_uploads rows share a `report_date`_
- _Expected_Behavior: GET /category-trend returns one row per unique (report_date, category) pair with `count` equal to the total `compliance_items` for that category across every upload sharing the date (Property 3 from design)_
- _Preservation: Single-upload-per-date rows are unchanged; empty-data response remains `{ categoryTrend: [] }`; total-conservation property holds across all dates (Property 6 from design)_
- _Requirements: 2.6, 2.7, 3.5_
- [x] 5.2 Verify the `/category-trend` portion of bug condition exploration test now passes
- **Property 1: Expected Behavior** - `/category-trend` Returns One Row Per (date, category)
- **IMPORTANT**: Re-run the SAME test case 1.C from task 1 — do NOT write a new test
- Run `npm run test:backend -- compliance-duplicate-chart-entries.property.test.js -t "/category-trend"`
- **EXPECTED OUTCOME**: Test case 1.C PASSES (confirms `/category-trend` bug is fixed)
- _Requirements: Property 3 (Validates 2.6, 2.7) from design_
- [x] 6. Fix `/summary` — disclose sibling uploads for the latest date
- [x] 6.1 Add sibling-upload disclosure to the `/summary` response
- In `backend/routes/compliance.js` `router.get('/summary', ...)` (around line 495), keep the existing `vertical IS NULL``vertical = 'NTS_AEO'` fallback for selecting the primary upload's `summary_json` (preserves legacy single-upload behavior per requirement 3.6)
- After resolving `latestUpload`, run the second query from design.md Fix 4 to find sibling uploads sharing the same `report_date`: `SELECT id, vertical, uploaded_at FROM compliance_uploads WHERE report_date = $1 AND id != $2 ORDER BY id ASC`
- Add `multi_vertical_uploads` to the response, populated with `siblings.map(s => ({ id, vertical, uploaded_at }))`; the field is `[]` when no siblings exist
- Do not change the `team` query parameter handling, the `ALLOWED_TEAMS` HTTP 400 response, or the `entries`/`overall_scores`/`upload` shape
- _Bug_Condition: isBugCondition(uploads) where two or more compliance_uploads rows share the latest `report_date`_
- _Expected_Behavior: GET /summary either merges sibling uploads' entries OR exposes a non-empty `multi_vertical_uploads` array identifying the other uploads for the same `report_date`; sibling uploads are never silently dropped (Property 4 from design)_
- _Preservation: Single-upload-per-date `/summary` shape is unchanged (the `vertical IS NULL``vertical = 'NTS_AEO'` fallback still runs); `team` query parameter still filters entries and rejects non-`ALLOWED_TEAMS` with HTTP 400; empty-data response remains `{ entries: [], overall_scores: {}, upload: null }`_
- _Requirements: 2.8, 2.9, 3.6, 3.7_
- [x] 6.2 Verify the `/summary` portion of bug condition exploration test now passes
- **Property 1: Expected Behavior** - `/summary` Does Not Silently Drop Sibling Uploads
- **IMPORTANT**: Re-run the SAME test case 1.D from task 1 — do NOT write a new test
- Run `npm run test:backend -- compliance-duplicate-chart-entries.property.test.js -t "/summary"`
- **EXPECTED OUTCOME**: Test case 1.D PASSES (confirms `/summary` discloses sibling uploads via `multi_vertical_uploads`)
- _Requirements: Property 4 (Validates 2.8, 2.9) from design_
- [x] 7. Fix `persistUpload()` snapshot block — filter and group by `vertical`
- [x] 7.1 Rewrite the `verticalStats` query to filter by the upload's `vertical`
- In `backend/routes/compliance.js` `persistUpload()` (lines 81192), at the `verticalStats` query around line 157, capture the upload's `vertical` from the row returned by the `RETURNING id` insert (or accept it as a `persistUpload()` parameter)
- Replace the `verticalStats` query with the SQL from design.md Fix 5: filter `WHERE team IS NOT NULL AND vertical IS NOT DISTINCT FROM $1` and group by `(vertical, team)`. The `IS NOT DISTINCT FROM` operator handles the legacy `vertical IS NULL` case so AEO-only uploads keep their previous semantics
- Leave the existing `INSERT ... ON CONFLICT (snapshot_month, vertical) DO UPDATE` mapping as-is so `compliance_snapshots` consumers (`/vcl/stats`) continue to read the same column shape; only the underlying counts change
- Keep snapshot creation wrapped in the existing `try/catch` so a snapshot failure is logged and does not fail the upload commit
- _Bug_Condition: isBugCondition for `persistUpload()` is `compliance_items` containing rows for verticals other than the upload's vertical — the unfiltered query inflates `total_devices`/`compliant`/`non_compliant`_
- _Expected_Behavior: compliance_snapshots rows written by persistUpload() have `total_devices`, `compliant`, `non_compliant` derived only from compliance_items rows belonging to the snapshotted vertical; no item from another vertical contributes (Property 5 from design)_
- _Preservation: Single-vertical months produce identical snapshot rows; the legacy `vertical IS NULL` AEO-only path is unchanged via `IS NOT DISTINCT FROM`; the snapshot try/catch error path is unchanged_
- _Requirements: 2.10, 2.11, 3.8, 3.9_
- [x] 7.2 Verify the `persistUpload()` portion of bug condition exploration test now passes
- **Property 1: Expected Behavior** - persistUpload() Snapshot Reflects Only the Snapshotted Vertical
- **IMPORTANT**: Re-run the SAME test case 1.E from task 1 — do NOT write a new test
- Run `npm run test:backend -- compliance-duplicate-chart-entries.property.test.js -t "persistUpload"`
- **EXPECTED OUTCOME**: Test case 1.E PASSES (confirms snapshot rows are filtered to the snapshotted vertical only)
- _Requirements: Property 5 (Validates 2.10, 2.11) from design_
- [x] 8. Verify preservation tests still pass after all five fixes
- **Property 2: Preservation** - Single-Upload-Per-Date Behavior Unchanged
- **IMPORTANT**: Re-run the SAME tests from task 2 — do NOT write new tests
- Run preservation property tests from task 2: `npm run test:backend -- compliance-duplicate-chart-entries.property.test.js -t "Preservation"`
- Confirm all six preservation test cases (2.A2.F) and the cross-endpoint property-based preservation extension still pass
- **EXPECTED OUTCOME**: Tests PASS (confirms no regressions across the four read endpoints, the `/summary` `team` filter, the `persistUpload()` single-vertical-month path, and the snapshot error-path behavior)
- _Requirements: 3.1, 3.2, 3.3, 3.4, 3.5, 3.6, 3.7, 3.8, 3.9, 3.10_
- [x] 9. Checkpoint — Ensure all tests pass
- Run the full backend test suite: `npm run test:backend`
- Confirm `compliance-duplicate-chart-entries.property.test.js` passes end-to-end (both Property 1 expected-behavior cases and Property 2 preservation cases)
- Confirm pre-existing tests (`vcl-compliance-reporting.property.test.js`, `vcl-aggregated-burndown.property.test.js`, `vcl-aggregated-burndown.test.js`, `vcl-compliance-reporting.test.js`, `fp-submissions-cleanup.test.js`, etc.) still pass — none of these should be affected since the fix is contained to read queries and one snapshot write query in `compliance.js`
- Spot-check the integration scenarios from design.md "Integration Tests": upload three xlsx files for the same `report_date` via `POST /preview` + `POST /commit`, then call `/trends`, `/top-recurring`, `/category-trend`, `/summary` and verify aggregated/disclosed responses; call `/vcl/stats` and verify per-vertical `compliance_pct` is correct
- Ensure all tests pass, ask the user if questions arise
## Notes
- All five fixes are contained to `backend/routes/compliance.js`. No database migration, no new column, no frontend change.
- The Property 1 / Property 2 task numbering follows the bugfix workflow convention so the IDE hover-status indicator can track exploration tests against expected-behavior verification. Task 1 is the single Property 1 source; tasks 3.2 / 4.2 / 5.2 / 6.2 / 7.2 each re-run the relevant slice of Property 1 (NOT new tests) to confirm the matching fix lands correctly. Task 2 is the single Property 2 source; task 8 re-runs Property 2 in full to confirm no regressions.
- The implementation tasks (3 through 7) are independent at the SQL level. Each can be reviewed and merged without waiting on the others, as long as task 1 and task 2 have run on the unfixed code first.
- The `_Bug_Condition`, `_Expected_Behavior`, and `_Preservation` annotations on each fix sub-task reference the formal pseudocode in `design.md` Glossary and Bug Details sections.
- `_Requirements: X.Y_` annotations cite clauses in `bugfix.md` Bug Analysis.