adam/proposal-system
1
0
Fork 0
mirror of https://github.com/Sea-Haven-Industries/proposal-system.git synced 2026-09-30 01:53:12 +00:00
Code Issues Projects Releases Packages Wiki Activity Actions

docs: Phase 6 cleanup — update AUDIT-REPORT, README, remove stale docs

Browse source 
- AUDIT-REPORT.md: mark all Phase 6 findings fixed (API-M2/M5/M7/M9/M10/M12/M13,
  WEB-M3/M4/M8/M9/M11, LAM-M2/M3/M6/M9, INF-M1/M2/M9), update test count to 149
- README.md: Function URL NONE→AWS_IAM, add Testing and Security sections,
  expand CI table with test jobs, note SQS encryption/OpenSearch VPC/access logging
- Remove stale session docs (AUDIT-2026-05-20, HANDOFF, RETROSPECTIVE, CHATGPT prompt)
- Add .claude/agents/ to .gitignore
- Remove empty-state placeholder from SimilarProposalsPanel
This commit is contained in:
Adam Moussa 2026-05-27 18:17:37 -04:00
parent ab9569d7a9
commit f9081fabf4
6 changed files with 86 additions and 352 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

1
.gitignore vendored
View file

@ -66,3 +66,4 @@ api/src/ProposalSystem.Api/Data/verified-sites.json
# Build artifacts
*.zip
.claude/worktrees/
.claude/agents/

67
AUDIT-REPORT.md
View file

@ -3,7 +3,7 @@
**Date:** 2026-05-27
**Auditor:** Claude Code (6 parallel specialist agents)
**Scope:** Full monorepo — API, Web, Mobile, Lambdas, Infrastructure/CI/CD, QA/Testing
**Remediation Status:** Phase 1-6 complete (2026-05-27). All Critical and High findings fixed. 25 Medium findings fixed. CI pipeline runs all 108 tests. Test infrastructure bootstrapped.
**Remediation Status:** Phase 1-6 complete (2026-05-27). All Critical and High findings fixed. 42 Medium findings fixed. CI pipeline runs all tests. Test infrastructure bootstrapped.
---
@ -11,7 +11,7 @@
The Proposal System has a solid architectural foundation with clean separation of concerns, proper Cognito JWT auth at the API Gateway layer, encrypted RDS, and a working end-to-end flow. However, the audit uncovered **5 Critical**, **36 High**, **75+ Medium**, and **60+ Low** severity findings across all layers.
**All Critical findings are now FIXED.** All High findings in API, Lambda, and Infrastructure domains are fixed. Web High findings are fixed. Mobile High findings are deferred (separate release cycle). Test infrastructure is bootstrapped with 107 tests.
**All Critical findings are now FIXED.** All High findings in API, Lambda, and Infrastructure domains are fixed. Web High findings are fixed. Mobile High findings are deferred (separate release cycle). Test infrastructure is bootstrapped with 149 tests.
~~**The most urgent issues:**~~
All items below have been remediated:
@ -20,7 +20,7 @@ All items below have been remediated:
2. ~~**JWT validation skipped when Authority not configured**~~ — **FIXED**: throws on missing authority in non-dev (API-C2)
3. ~~**Lambda Function URL has AUTH_NONE**~~ — **FIXED**: changed to AWS_IAM with invoke grants (LAM-C1/INF-H1)
4. ~~**JWT stored in localStorage**~~ — **FIXED**: moved to sessionStorage (WEB-C1)
5. ~~**Zero test coverage across entire monorepo**~~ — **FIXED**: 108 tests (77 .NET, 12 web, 19 Python), CI runs all suites (QA-C1)
5. ~~**Zero test coverage across entire monorepo**~~ — **FIXED**: 149 tests (104 .NET, 26 web, 19 Python), CI runs all suites (QA-C1)
6. ~~**DevMode has no environment guard**~~ — **FIXED**: gated by IsDevelopment() (API-H8)
---
@ -54,18 +54,18 @@ All items below have been remediated:
| ID | Finding | Status |
|----|---------|--------|
| API-M1 | Internal API key always grants `admins` role, never `sysadmins` | |
| API-M2 | Silent auth failure when neither Cognito nor DevMode configured | |
| API-M2 | Silent auth failure when neither Cognito nor DevMode configured | **FIXED** — throws InvalidOperationException at startup |
| API-M3 | Dispatchers can read any proposal's line items (no ownership check) | **FIXED** — ownership check in LineItemsController |
| API-M4 | Dispatchers can access PDF endpoints for any proposal | **FIXED** — ownership check in GeneratedPdfsController |
| API-M5 | Missing validators for VendorProposal, GeneratedPdf, SimilarReference DTOs | |
| API-M5 | Missing validators for VendorProposal, GeneratedPdf, SimilarReference DTOs | **FIXED** — FluentValidation validators added |
| API-M6 | No file size validation on presigned upload URLs | **FIXED** — 25MB cap with 400 response |
| API-M7 | No `.AsNoTracking()` on read-only queries | |
| API-M7 | No `.AsNoTracking()` on read-only queries | **FIXED** — AsNoTracking on all read-only queries |
| API-M8 | BulkUpdate uses delete-all/insert-all without explicit transaction | **FIXED** — explicit transaction with rollback |
| API-M9 | Dev PDF generation leaks stderr to client | |
| API-M10 | Auth callback reveals config state in error responses | |
| API-M9 | Dev PDF generation leaks stderr to client | **FIXED** — stderr logged, generic error to client |
| API-M10 | Auth callback reveals config state in error responses | **FIXED** — generic "Authentication service unavailable" |
| API-M11 | Silent exception swallowing on audit logging (`catch { }`) | **FIXED** — `LogError` on all audit catch blocks |
| API-M12 | Audit trail does not capture before/after values | |
| API-M13 | User role change audit does not log previous role | |
| API-M12 | Audit trail does not capture before/after values | **FIXED** — structured JSON { old, new } on status/field changes |
| API-M13 | User role change audit does not log previous role | **FIXED** — logs { old, new } role in audit trail |
| API-M14 | Dev signing key hardcoded in committed config | **FIXED** — requires user-secrets or env var |
---
@ -154,11 +154,15 @@ All items below have been remediated:
| ID | Finding | Status |
|----|---------|--------|
| LAM-M1 | No event/record validation at handler entry | **FIXED** — Records/body validation in all SQS handlers |
| LAM-M2-M4 | Prompt injection risk, PDF size limits, Bedrock timeout | |
| LAM-M2 | Prompt injection risk in Bedrock prompts | **FIXED** — sanitize_user_text() strips injection patterns |
| LAM-M3 | No PDF file size limit before processing | **FIXED** — 50MB check via head_object before download |
| LAM-M4 | No Bedrock invocation timeout | |
| LAM-M5 | Missing stack traces in error logging | **FIXED** — `logger.exception()` in all except blocks |
| LAM-M6-M7 | Numeric validation, tight Lambda timeout | |
| LAM-M6 | No numeric validation on suggestion amounts | **FIXED** — validate_line_item_numerics() rejects negative/NaN/extreme |
| LAM-M7 | Tight Lambda timeout | |
| LAM-M8 | S3 key not sanitized | **FIXED** — `_validate_s3_key()` rejects traversal/invalid chars |
| LAM-M9-M14 | Stale API key cache, empty env var defaults, KB sync flooding, CDK bundling gaps | |
| LAM-M9 | Stale API key cache — no TTL | **FIXED** — 5-minute TTL on all 4 Lambda API key caches |
| LAM-M10-M14 | Empty env var defaults, KB sync flooding, CDK bundling gaps | |
---
@ -190,7 +194,7 @@ All items below have been remediated:
### 6. QA & Testing (6 Critical, 16 High)
**Test infrastructure bootstrapped: 108 tests across 3 stacks (77 .NET, 12 web, 19 Python). CI runs all suites on every PR.**
**Test infrastructure bootstrapped: 149 tests across 3 stacks (104 .NET, 26 web, 19 Python). CI runs all suites on every PR.**
#### Critical Gaps — MOSTLY FIXED
@ -205,7 +209,7 @@ All items below have been remediated:
#### High Gaps — PARTIALLY ADDRESSED
Validators tested (36 tests). Lambda handlers tested (19 pytest tests for pdf-generate and suggestions). **CI pipeline now runs all 108 tests** (dotnet test, vitest, pytest) on every PR. Remaining gaps: ProposalNumberGenerator, AuthController integration, LineItemService, frontend components, API client interceptors, PDF parsers.
Validators tested (36 tests). Lambda handlers tested (19 pytest tests for pdf-generate and suggestions). ProposalNumberGenerator tested (8 tests). LineItemService state guards tested (18 tests). API client interceptor tested (14 vitest tests). **CI pipeline now runs all 149 tests** (dotnet test, vitest, pytest) on every PR. Remaining gaps: AuthController integration, frontend components, PDF parsers.
---
@ -270,15 +274,38 @@ Validators tested (36 tests). Lambda handlers tested (19 pytest tests for pdf-ge
48. ~~INF-M5~~ — DONE (enforceSSL on all 4 S3 buckets)
49. ~~INF-M8~~ — DONE (SHA-pinned workflow refs in all 3 workflow files)
### Phase 6 — Infrastructure Medium Fixes (INF-M1, M2, M9)
50. ~~INF-M1~~ — DONE (Bedrock IAM scoped to specific inference profile ARN)
51. ~~INF-M2~~ — DONE (AOSS data access policy scoped per principal)
52. ~~INF-M9~~ — DONE (`--require-approval broadening` in deploy script)
### Phase 6 — Remaining Medium Fixes ✅ COMPLETE
**API hardening:**
50. ~~API-M2~~ — DONE (startup throws if auth not configured)
51. ~~API-M5~~ — DONE (FluentValidation for VendorProposal, GeneratedPdf, SimilarReference)
52. ~~API-M7~~ — DONE (AsNoTracking on read-only queries)
53. ~~API-M9~~ — DONE (stderr logged, not returned to client)
54. ~~API-M10~~ — DONE (generic auth error responses)
55. ~~API-M12~~ — DONE (before/after JSON in audit trail)
56. ~~API-M13~~ — DONE (previous role logged on change)
**Web DX and reliability:**
57. ~~WEB-M3~~ — DONE (10-char min on scope field)
58. ~~WEB-M4~~ — DONE (ServiceCategory Other aligned across all layers)
59. ~~WEB-M8~~ — DONE (dashboard error state instead of zeros)
60. ~~WEB-M9~~ — DONE (loading skeleton for line items)
61. ~~WEB-M11~~ — DONE (Return to Review button on approved proposals)
**Lambda reliability:**
62. ~~LAM-M2~~ — DONE (prompt injection sanitizer)
63. ~~LAM-M3~~ — DONE (50MB PDF size check)
64. ~~LAM-M6~~ — DONE (numeric validation on suggestions)
65. ~~LAM-M9~~ — DONE (5-minute TTL on API key cache)
**Infra tightening:**
66. ~~INF-M1~~ — DONE (Bedrock IAM scoped to specific inference profile ARN)
67. ~~INF-M2~~ — DONE (AOSS data access policy scoped per principal)
68. ~~INF-M9~~ — DONE (`--require-approval broadening` in deploy script)
### Phase 7 — Remaining (not yet started)
- WEB-H1: Token refresh mechanism (requires backend refresh token flow)
- Mobile High findings (MOB-H1 through H4): separate release cycle
- Remaining Medium findings: API-M1/M2/M5/M7/M9/M10/M12/M13, WEB-M1/M3/M4/M8/M9/M11/M12, LAM-M2-M4/M6-M7/M9-M14, INF-M3-M4/M6-M7
- Remaining Medium findings: API-M1, WEB-M1/M12, LAM-M4/M7/M10-M14, INF-M3-M4/M6-M7
- QA-C6: Mobile test coverage
---

90
HANDOFF-2026-05-22.md
View file

@ -1,90 +0,0 @@
# Session Handoff — 2026-05-22
## What was done (committed on main, 7 commits ahead of origin)
### Commits 1–4 (prior session)
#### Commit 1: Site search, PO number, service category, form enhancements
- **Site search UX:** Moved "Enter address manually" button outside Autocomplete dropdown (click-race fix), removed flickering `searchedOnce` state, disabled MUI client-side double-filtering
- **Structured manual entry:** 5 separate fields (Site Code, Street, City, State, Zip) auto-assemble into `customerAddress` string
- **PO number:** Added `poNumber` to frontend form, backend entity, and `AddPoNumber` migration
- **Service category "Other":** Added `Other` to enum, shows "Specify Category" text field when selected, prepends custom text to notes
- **Label consistency:** "Work Order #" → "Work Order Number", "PO Number" on form and detail page
- **Form layout:** Top row reflowed to 3-column layout (4/4/4)
- Dev PDF generation script (`scripts/generate-pdf-local.py`)
#### Commit 2: Status/priority display, sortable tables, list filters
- `STATUS_LABELS` map: "InReview" → "In Review" everywhere
- `PRIORITY_LABELS` map: "Emergency" → "Emergency Dispatch"
- `Sent` color changed from green (same as Approved) to blue
- Urgent rows: orange border + warning icon; Emergency: red border + red background + alert icon
- All 8 columns in My Proposals list sortable via `TableSortLabel` (client-side, default Submitted desc)
- Status, Category, Priority filter dropdowns with server-side filtering; filters reset to page 1
#### Commit 3: PDF download, versioning, status timeline fix
- Removed admin-only restriction on `GET pdf` and `GET pdf/{revision}` endpoints
- Added `GET pdf/versions` endpoint returning `{revision, generatedAt}[]`
- "Download PDF" button on detail page for Approved/Sent/Revised proposals
- "PDF Versions" card with individual download per revision
- Dev-mode support for `GetPdfRevision` endpoint
- Status timeline stepper uses `STATUS_LABELS` (fixes raw "InReview" display)
#### Commit 4: Admin workspace revision dropdown, editable WO#, approval fix
- **Bug fix:** `ApproveAsync` now accepts both `InReview` and `Revised` proposals
- **Revision dropdown:** "Rev X" chip is now a clickable dropdown listing all revisions with status and download
- **Editable Work Order Number:** WO# in admin workspace is now editable; added `WorkOrderNumber` to `UpdateProposalRequest`
- **Info bar reorder:** Customer → Site → WO# → PO# → Category → Priority
- **Download PDF button** in action bar for Sent and Revised proposals
- Typed `getHistory` API to return `ProposalDetail[]`
### Commits 5–7 (this session)
#### Commit 5: Fix layout double-offset from persistent drawer
- The sidebar's persistent Drawer reserved width in the flex container AND the main content had `margin-left` for the same width, pushing content 440px to the right
- Removed the redundant `margin-left` from main content; added `transition: width 250ms` to the Drawer for smooth toggle
#### Commit 6: Add role-aware dashboard stats and recent proposals
- `GetStatsAsync` now returns global counts for admins/sysadmins instead of filtering by `SubmittedById`
- Dashboard recent proposals query uses `mine=false` for admins so they see all proposals
#### Commit 7: Apply UX quick wins from external review
- **"Not priced" display:** New `formatBidAmount()` helper replaces `$0.00` with "Not priced" across Dashboard, ProposalListPage, AdminDashboard, and ProposalDetailPage
- **Login button consistency:** All three dev-login role buttons now use the same filled style with role descriptions (e.g., "Admin — Review and approve proposals")
- **Context-aware empty states:** Proposal list distinguishes "no results match filters" from "you have no proposals" with appropriate copy and actions
- **Clear Filters button:** Added to ProposalListPage and AdminDashboard filter rows; appears when any filter/search is active
- **Workspace button clarity:** "Regenerate" renamed to "Regenerate Suggested Line Items"; disabled Save/Approve buttons show tooltip explanations
- **Plain language:** Similar Proposals empty state changed from "RAG engine" jargon to "Similar proposals will appear here when available"
- **User Management placeholder:** Centered layout with "Coming Soon" heading and Cognito guidance text
- **Screenshot script:** `scripts/screenshot-pages.mjs` captures all pages for all 3 roles via Puppeteer (incognito contexts, dev-login button clicks)
## What's running locally
- Postgres via Docker (port 5432)
- .NET API on http://localhost:5000 (dev mode, ASPNETCORE_ENVIRONMENT=Development)
- Vite frontend on http://localhost:5173
## Not done yet
### Must do before push
- **Not pushed** — 8 commits on main ahead of origin, needs `git push`
- CDK compute stack needs `IAmazonDynamoDB` read permission on `verified-sites` table for production `SiteService`
### UX improvements (from external review — DONE)
All 15 medium-effort and heavy-lift UX improvements from the external review have been implemented (commit 8).
### Infrastructure / ops
- Confluence architecture map update (page id 1540098)
- `[skip deploy]` convention not implemented in deploy workflow
- Automated test suite not started
### Mobile
- App icons still placeholders
- Google OAuth mobile crash uninvestigated
## Compliance Fixes Applied (2026-05-18)
- Fixed oss-index-creator Lambda: added `functionName`, `architecture: ARM_64`, `logRetention: TWO_MONTHS`
- Added `node-version: "24"` to all CI workflow reusable calls (web, mobile, infra)
- Removed dead `_api_request()` helper and unused `time` imports from all 4 main Python Lambdas
- Added `permissions: id-token: write` to deploy-mobile.yaml for OIDC
- Added `paths-ignore: mobile/**` to deploy.yaml
- Added `@react-native-community/cli` + `cli-platform-ios` as mobile devDependencies

46
README.md
View file

@ -6,7 +6,7 @@ Internal proposal management platform for Sea Haven Industries. Dispatchers subm
Monorepo with five primary services:
- **.NET 8 API** -- Clean Architecture REST API hosted on Lambda behind API Gateway (JWT-authorized) with Function URL for internal access
- **.NET 8 API** -- Clean Architecture REST API hosted on Lambda behind API Gateway (JWT-authorized) with Function URL (AWS_IAM) for internal access
- **React 19 Web** -- MUI v7 admin/dispatcher workspace served via CloudFront + S3
- **React Native Mobile** -- iOS-first field app for dispatchers (offline-capable)
- **Python Lambdas** -- PDF extraction, PDF generation, library ingestion, AI suggestions, AOSS index provisioning
@ -36,7 +36,7 @@ proposal-system/
| Mobile | React Native CLI 0.85, React 19, React Native Paper, React Navigation, react-native-app-auth (PKCE), amazon-cognito-identity-js (SRP), Keychain, offline draft queue |
| Lambdas | Python 3.12, arm64, pdfplumber, reportlab, httpx, boto3 |
| Infrastructure | CDK TypeScript (aws-cdk-lib 2.253.1) |
| AI/RAG | Bedrock Knowledge Base (Titan Embeddings v2), OpenSearch Serverless, Claude via Bedrock Runtime |
| AI/RAG | Bedrock Knowledge Base (Titan Embeddings v2), OpenSearch Serverless (VPC-only), Claude Sonnet via Bedrock cross-region inference |
| Auth | Cognito User Pool + Google OAuth IdP (groups: dispatchers, admins, sysadmins) |
## AWS Resources
@ -46,13 +46,13 @@ All resources are in **us-east-1** (account 328440206208).
| CDK Stack | Key Resources |
|---|---|
| `proposal-system-foundation` | RDS PostgreSQL 15 (t4g.small), S3 buckets, SQS queue + DLQ, Cognito user pool, Secrets Manager |
| `proposal-system-compute` | API Gateway HTTP API (JWT authorizer), .NET 8 API Lambda + Function URL, Python Lambdas (pdf-extract, pdf-generate, library-ingest, suggestions, oss-index-creator), OpenSearch Serverless collection, Bedrock KB |
| `proposal-system-compute` | API Gateway HTTP API (JWT authorizer + access logging), .NET 8 API Lambda + Function URL (AWS_IAM), Python Lambdas (pdf-extract, pdf-generate, library-ingest, suggestions, oss-index-creator), OpenSearch Serverless collection (VPC endpoint), Bedrock KB |
| `proposal-system-frontend` | CloudFront distribution (S3 OAC) |
| Resource Type | Names |
|---|---|
| S3 Buckets | `proposal-system-uploads`, `proposal-system-generated`, `proposal-system-library`, `seahaven-ios-certificates` |
| SQS | `proposal-system-jobs` (720s visibility, reportBatchItemFailures) + `proposal-system-jobs-dlq` (message body filtering by jobType) |
| SQS | `proposal-system-jobs` (720s visibility, SQS-managed encryption, reportBatchItemFailures) + `proposal-system-jobs-dlq` (SQS-managed encryption, message body filtering by jobType) |
| Secrets | `proposal-system/db-credentials`, `proposal-system/internal-api-key` |
## Local Development
@ -108,14 +108,16 @@ npx cdk synth
### CI (on pull request to main)
Five parallel jobs calling org reusable workflows:
Seven parallel jobs calling org reusable workflows:
| Job | Workflow | What it checks |
|---|---|---|
| .NET Build & Test | `ci-dotnet.yaml` | Restore, build, test the API solution |
| .NET Build & Test | `ci-dotnet.yaml` | Restore, build, test the API solution (104 xUnit tests) |
| Web Frontend Check | `ci-typescript-cdk.yaml` | TypeScript typecheck for web |
| Web Tests | `ci-typescript-cdk.yaml` | vitest suite (26 tests — auth, interceptors, components) |
| Mobile Typecheck | `ci-typescript-cdk.yaml` | TypeScript typecheck for mobile |
| Python Lint | `ci-python-sam.yaml` | ruff check + format on lambdas/ |
| Python Tests | `ci-python-sam.yaml` | pytest suite (19 tests — pdf-generate, suggestions handlers) |
| CDK Synth | `ci-typescript-cdk.yaml` | Synthesize CDK stacks (includes .NET publish) |
### Deploy (on push to main)
@ -158,11 +160,11 @@ Two-layer auth architecture with defense-in-depth:
| External clients → API Gateway `/{proxy+}` | Cognito JWT authorizer (web + mobile client IDs) | .NET JWT middleware (ValidateAudience=true) |
| `/api/health` | None (public) | None |
| `/api/auth/callback`, `/api/auth/dev-login` | None (unauthenticated) | None (pre-auth endpoints) |
| Internal Lambdas → Function URL | None (NONE auth type) | Internal API key (`X-Internal-Api-Key` header, value from Secrets Manager) |
| Internal Lambdas → Function URL | AWS_IAM (grantInvokeUrl) | Internal API key (`X-Internal-Api-Key` header, value from Secrets Manager) |
**Role-based access:** Cognito groups (`dispatchers`, `admins`, `sysadmins`) map to API roles via `cognito:groups` claim. Dispatchers can only see their own proposals (ownership enforced in service layer). VendorProposals and GeneratedPdfs endpoints restricted to admins/sysadmins.
**Internal API key:** Python Lambdas call the .NET API via a Lambda Function URL (bypasses API Gateway JWT check). The `InternalApiKeyMiddleware` validates the key and assigns the `admins` role to the synthetic identity.
**Internal API key:** Python Lambdas call the .NET API via a Lambda Function URL with AWS_IAM auth (bypasses API Gateway JWT check). The `InternalApiKeyMiddleware` validates the `X-Internal-Api-Key` header and assigns the `admins` role to the synthetic identity. Lambdas cache the API key from Secrets Manager with a 5-minute TTL.
## Data Flow
@ -175,3 +177,31 @@ Two-layer auth architecture with defense-in-depth:
7. On send: `library-ingest` Lambda adds approved proposal to KB for future matching
Failed SQS messages are reported via `batchItemFailures` and retried up to 3 times before moving to the DLQ.
## Testing
149 tests across three stacks, all run in CI on every PR:
| Suite | Framework | Count | Coverage |
|---|---|---|---|
| .NET API | xUnit | 104 | State machine transitions, authorization attributes, middleware, validators, ProposalNumberGenerator, LineItemService state guards |
| Web | vitest | 26 | ProtectedRoute, RoleGuard, API client interceptor (401 logout, token attachment) |
| Python Lambdas | pytest | 19 | pdf-generate and suggestions handler contracts |
```bash
cd api && dotnet test # .NET tests
cd web && npm test # vitest
cd lambdas && python -m pytest # pytest
```
## Security
Hardening applied across all layers (see AUDIT-REPORT.md for full details):
- **Auth:** Cognito JWT validation with audience check, startup fails if auth not configured, DevMode gated to `IsDevelopment()`
- **API:** FluentValidation on all DTOs, generic error responses (no stack traces or config leaks), structured audit logging with before/after diffs
- **Function URL:** AWS_IAM auth + internal API key (two-layer defense)
- **Infrastructure:** S3 `enforceSSL` + `BLOCK_ALL`, SQS managed encryption, OpenSearch VPC-only, Cognito optional TOTP MFA, API Gateway access logging
- **Lambdas:** Prompt injection sanitization, PDF size limits, numeric validation on AI suggestions, S3 key sanitization, idempotent SQS processing
- **CI/CD:** OIDC (no long-lived credentials), SHA-pinned workflow refs, `--require-approval broadening` on local deploys
- **Web:** sessionStorage for tokens (not localStorage), error boundaries, role guards on all admin routes, 401 interceptor clears auth state

228
RETROSPECTIVE-2026-05-19.md
View file

@ -1,228 +0,0 @@
# Proposal System — Retrospective (2026-05-19, updated 2026-05-20, session 5 added 2026-05-20)
## Executive Summary
Across four sessions (2026-05-18, 2026-05-19, and two on 2026-05-20), the project progressed from a broken mobile CI pipeline to a functional mobile app on device AND a fully tested web frontend with working dev-mode authentication, proposal lifecycle, and admin workflows.
**Sessions 1-3 (Mobile):** The mobile app went from a broken CI pipeline to a functional app running on a physical device with working email/password authentication. Three distinct launch crashes were resolved, Cognito SRP login was validated end-to-end, and multiple UI issues were fixed. The app boots, authenticates, and renders on iOS 26 hardware. However, it cannot communicate with the backend API from a device, Google OAuth crashes the app, and the branch has not been merged to main.
**Session 4 (Web):** Full local web testing exposed six bugs in the API and frontend: enum serialization failures, identity/role confusion in dev-login, incorrect proposal ownership filtering, Autocomplete binding issues, audit log format errors on Postgres jsonb columns, and missing API idempotency. All were fixed in four logical commits. The web app's core workflow — submit as dispatcher, review/edit/approve/send as admin — is now functional end-to-end in dev mode.
**Session 5 (Automated QA):** Ran 4 parallel test agents covering ~145 test cases across every API endpoint and every frontend page. Found 18 bugs (2 critical, 4 high, 7 medium, 5 low). All 16 actionable bugs fixed in 4 commits. Critical: admin dashboard LINQ crash (EF Core can't translate TimeSpan.TotalHours to SQL) and revision endpoint 500 (unique constraint on ProposalNumber). High: dispatcher dashboard data exposure (missing mine filter), no frontend role guards on admin routes, submittedByName null on mutation responses, invalid role silently defaulting to Admin. Also extracted duplicated STATUS_COLORS and format utilities into shared modules, wired the admin dashboard filter dropdowns, and added debounce to customer search.
**Systemic findings:** The web session revealed two architectural gaps: (1) audit logging was fragile — a format error in a non-critical audit write could roll back an otherwise successful save, and (2) state machine transitions lacked idempotency, meaning retries or UI double-clicks could produce 500 errors instead of graceful no-ops. Both are patterns that would have surfaced in production under real load. Session 5 added a third: the frontend had no authorization enforcement — `ProtectedRoute` checked authentication but not role, so any logged-in user could navigate to admin pages by URL.
## Standards Compliance Status
| Rule | Status | Detail |
|------|--------|--------|
| Naming conventions | PASS | kebab-case throughout, branch name follows pattern |
| CI/CD pipeline exists | PASS | `deploy-mobile.yaml` and `deploy.yaml` both trigger on push to main |
| OIDC deploy role | PASS | `githubdeploy-proposal-system` |
| README accurate | **PARTIAL** | Root README updated (0.85, deploy status). `mobile/README.md` incomplete (no auth/device docs). Web dev mode not documented. |
| Confluence updated | **FAIL** | No Atlassian MCP. Architecture Map missing mobile pipeline, Cognito auth flow, and web dev-mode setup |
| Memory updated | **UPDATED** | Project memory updated with session 4 web fixes. New feedback memories created for API patterns. |
| Git workflow | PASS | All sessions used feature branch `mobile/fix-react-version-and-ui` |
| Commit messages | PASS | Imperative mood, explains "why", logically grouped changes |
| CDK callback URL fix | PASS | Fixed in CDK + live Cognito via AWS CLI |
| Pre-push lint/typecheck | NOT VERIFIED | Did not run typecheck before pushing — should have per CLAUDE.md hook |
| Dev secrets excluded | PASS | `appsettings.Development.json` (dev signing key) kept untracked, not committed |
| API idempotency | **FIXED** | Approve, MarkSent, Revise transitions now idempotent. Audit failures isolated from saves. |
## Required Memory Updates
### Completed this session
1. **`project_proposal_system.md`** — Updated with session 4 web fixes: dev-mode setup, enum serialization, audit log format, idempotent transitions, scoped My Proposals filtering.
2. **`feedback_mobile_deploy_lessons.md`** — All three session-3 lessons added (React pinning, import type, dev API URL). Done in session 3.
3. **`feedback_react_version_pinning.md`** — Created in session 3. Done.
4. **`feedback_api_idempotency.md`** — NEW: State machine transitions must be idempotent. Audit writes must not roll back successful saves.
5. **`feedback_jsonb_audit_format.md`** — NEW: Postgres jsonb columns require valid JSON, not plain strings. Audit details must be wrapped.
### Still outstanding
6. **`reference_mobile_testflight.md`** — The ⚠️ note about "username/password flow needs to be added" is now resolved but not yet updated in the file.
## Required Documentation Updates
| Doc | Status | Action |
|-----|--------|--------|
| Root `README.md` | Updated (session 2) | Needs update: add web dev-mode setup instructions (DevMode, dev-login, local Postgres) |
| `mobile/README.md` | Exists but incomplete | Add: email/password auth via Cognito SRP, `patch-package` for netinfo iOS 26 fix, React version pinning requirement, local device testing setup |
| `api/` dev setup | **MISSING** | No documentation for local API development: `appsettings.Development.json` template (without secrets), Docker Compose for Postgres, dev-login endpoint usage |
| Confluence "AWS Architecture Map" | **OUTSTANDING** | Still blocked — no Atlassian MCP. Needs: mobile CI/CD pipeline, Cognito auth flow, web dev-mode architecture |
| CDK `foundation-stack.ts` | Updated (session 2) | Callback URLs fixed, CfnOutputs added for client IDs |
## Reusable Skills / Automations
| Candidate | Type | ROI | Description |
|-----------|------|-----|-------------|
| React version coherence check | CI step | **CRITICAL** | `node -e` script that reads `node_modules/react-native/Libraries/Renderer/implementations/ReactNativeRenderer-dev.js`, extracts the hardcoded version string, and compares against `node_modules/react/package.json`. Fails if mismatch. Would have caught the exact crash from session 3. |
| API idempotency test suite | Integration test | **HIGH** | For each state-machine endpoint (approve, markSent, revise), call twice with same input and assert both return 200 with matching response. Would have caught all three idempotency bugs from session 4. Pattern: assert `f(f(x)) == f(x)` for all mutation endpoints. |
| Audit isolation pattern | Code pattern | **HIGH** | Wrap all non-critical audit writes in try/catch so they never roll back the primary operation. Consider a `SafeAuditService` decorator or middleware. Session 4's bulk update 500 error was caused by audit failure after a successful save. |
| iOS device smoke test script | Script / Runbook | HIGH | Checklist for post-build device testing: connect device, Metro `--host <LAN_IP>`, build with automatic signing, verify login, test auth flow. |
| `patch-package` audit CI step | CI check | MEDIUM | Verify patches in `mobile/patches/` still apply cleanly and patched packages haven't been updated. |
| Dev-mode login test harness | Script | MEDIUM | Script that exercises all three dev-login roles (SysAdmin, Admin, Dispatcher) and verifies each returns a distinct user identity with correct role. Would have caught the role/identity confusion bugs immediately. |
| Cognito ID token user extraction | Utility | LOW | `parseUserFromIdToken()` in `auth.ts` — reusable for any Cognito-backed app. |
## Key Lessons Learned
### React Native Runtime (Sessions 1-3)
1. **React version MUST be pinned exactly, not with semver range.** RN 0.85.3's bundled `ReactNativeRenderer-dev.js` has a hard check: `if ("19.2.3" !== isomorphicReactPackageVersion)`. The peer dependency says `^19.2.3`, npm resolves to 19.2.6, and the app crashes with an opaque "Cannot read property 'default' of undefined" in `getPaperRenderer`. Pin `"react": "19.2.3"` in package.json.
2. **`import type` is not reliably erased for modules with native initialization.** `import type { CognitoUserSession } from 'amazon-cognito-identity-js'` was NOT stripped by Babel in RN's build pipeline. The module eagerly initialized native crypto at import time, causing a crash. Fix: remove the import entirely and use `any`, or use dynamic `await import()`.
3. **`localhost` in dev config is the phone, not the Mac.** `API_URL: 'http://localhost:5000/api'` in dev mode is unreachable from a physical device. Need either LAN IP or a fallback strategy.
### iOS 26 Specific (Sessions 1-3)
4. **CoreTelephony APIs removed without replacement.** `@react-native-community/netinfo` v12.0.1 still calls deprecated APIs. Required `patch-package` with `respondsToSelector:` guards.
5. **iPhone Mirroring is the fastest way to test on device.** Built into macOS 26, gives full touch control. Developer Mode on the phone is under Settings > Privacy & Security.
### .NET API / Web Frontend (Session 4)
6. **Postgres jsonb columns reject plain strings.** The `details` column on `AuditLogs` is typed `jsonb`. Writing a bare string like `"Added: Widget repair"` produces Postgres error 22P02. Wrap in a JSON object: `JsonSerializer.Serialize(new { message = details })`. This is easy to miss because SQLite and SQL Server `nvarchar` accept anything.
7. **System.Text.Json requires explicit `JsonStringEnumConverter` for enum round-tripping.** Without it, sending `"ServiceCategory": "Plumbing"` from the frontend produces a validation error because the default deserializer expects an integer. Must add `options.JsonSerializerOptions.Converters.Add(new JsonStringEnumConverter())` in `AddJsonOptions`.
8. **State machine transitions must be idempotent.** Approve, MarkSent, and Revise all threw `InvalidOperationException` on repeat calls (e.g., from network retries or UI double-clicks). Fix: if already in the target state, return current entity instead of throwing. This is especially critical for mobile clients with unreliable connectivity.
9. **Audit writes must never roll back successful business operations.** `BulkUpdateAsync` saved line items successfully, then `_audit.LogAsync` threw (due to the jsonb format bug), and the entire request returned 500. The user saw "unexpected error" even though their data was saved. Fix: wrap non-critical audit calls in try/catch.
10. **Dev-login must produce deterministic, distinct identities per role.** Using `Guid.NewGuid()` for CognitoSub meant the same email produced different identities across logins. Using a single hardcoded email for all roles meant switching roles didn't actually switch users. Fix: deterministic sub (`dev-{email}`), distinct email/name per role, and update role on existing user if changed.
11. **"My Proposals" means ownership, not role-based filtering.** Initial implementation filtered by role (show all for admins, filter for dispatchers). The correct behavior: "My Proposals" always shows only proposals the current user submitted, regardless of role. Admins see all proposals in the separate Admin Queue.
### Process (All Sessions)
12. **Feature branch for iterative debugging works.** Session 2 pushed 10+ commits to main. Sessions 3-4 used `mobile/fix-react-version-and-ui` — all fixes stay off main until ready.
13. **User testing catches what type systems and linters can't.** Session 4's six bugs all passed TypeScript compilation and would pass unit tests. They were logic errors in business rules, serialization config, and identity management that only surfaced through manual workflow testing. Interactive testing with role-switching is essential before any deploy.
## Highest ROI Improvements
Ranked by impact-to-effort:
1. **Add API idempotency integration tests** (1 hr) — For each state-machine endpoint, call twice with same input and assert both return 200. Pattern: `assert f(f(x)) == f(x)`. Would have caught 3 of session 4's bugs automatically. Generalizable to any future endpoint.
2. **Add React version coherence CI check** (30 min) — A 10-line node script that extracts the expected version from the bundled renderer and compares to installed React. Prevents the most time-consuming crash from session 3.
3. **Isolate audit writes from business operations** (30 min) — Create a `SafeAuditService` wrapper or add try/catch to all audit calls in services. The pattern already exists in `LineItemService` but should be systematic, not ad-hoc. A single audit format bug caused a 500 on an otherwise successful operation.
4. **Add `.gitignore` to API project** (5 min) — `appsettings.Development.json` contains dev signing keys and must not be committed. Currently relying on manual exclusion. Add it to `.gitignore` with a template file (`.example`) that documents the required keys without values.
5. **Document web dev-mode setup** (15 min) — No docs exist for running the API locally: Docker Compose for Postgres, `appsettings.Development.json` template, dev-login endpoint, role switching. This will block any new developer.
6. **Merge `mobile/fix-react-version-and-ui` and deploy** (5 min) — Branch has 8 commits of critical fixes (sessions 3-4). Current TestFlight build crashes. Must merge before next submission.
7. **Fix dev API_URL for physical devices** (10 min) — `localhost:5000` is unreachable from iPhone. Blocks all API-dependent mobile features during device testing.
8. **Pin all RN ecosystem versions exactly** (5 min) — Already done for React; extend to all `@react-native/*` packages.
## Outstanding Risks or Follow-Ups
| Priority | Item | Risk | Branch/Location |
|----------|------|------|-----------------|
| **BLOCKING** | Feature branch not merged — TestFlight build still crashes | Any TestFlight tester or Apple reviewer will see a crash | `mobile/fix-react-version-and-ui` |
| **BLOCKING** | Google OAuth crashes the app on tap | Apple reviewer may try both login methods | `auth.ts` → `react-native-app-auth` → Cognito Hosted UI |
| **HIGH** | `appsettings.Development.json` not in `.gitignore` | Dev signing key could be accidentally committed | `api/src/ProposalSystem.Api/` |
| **HIGH** | Dev API_URL is `localhost:5000` — all API calls fail on device | Proposals can't be created, viewed, or searched on device | `config.ts` |
| **HIGH** | Placeholder app icons (solid blue squares) | Unprofessional for TestFlight / App Store | `ios/ProposalSystem/Images.xcassets` |
| **HIGH** | No web dev-mode setup documentation | New developer can't run the system locally | Root README / api/ docs |
| ~~HIGH~~ | ~~Audit isolation is ad-hoc, not systematic~~ | ~~Fixed session 4; session 5 verified via testing~~ | ~~LineItemService, ProposalService~~ |
| **MEDIUM** | Confluence Architecture Map still missing mobile pipeline | Documentation debt per CLAUDE.md | Page 1540098 |
| **MEDIUM** | `react-native-paper` has known issues with RN 0.85 | May surface as bugs in production | GitHub issues #4889, #4905 |
| **MEDIUM** | netinfo patch needs monitoring for upstream fix | Patches can silently break on version bumps | `patches/@react-native-community+netinfo+12.0.1.patch` |
| **MEDIUM** | `DeleteAsync` in `LineItemService` doesn't update `TotalBidAmount` | Deleting a line item leaves the proposal total stale | `LineItemService.cs:113` |
| **LOW** | `no-floating-promises` ESLint rule still not added | Class of crash from session 2 can recur | `mobile/.eslintrc` |
| **LOW** | Cognito test user password in memory file | Acceptable for internal test account | `reference_mobile_testflight.md` |
| **LOW** | 4 Dependabot vulnerabilities open | Adam deferred these | GitHub Security tab |
## Session 5 Changelog — Automated QA & Bug Fixes (2026-05-20)
All fixes on `mobile/fix-react-version-and-ui` (4 commits, not yet on main):
### `d00c552` Fix admin dashboard LINQ crash, revise unique constraint, and mutation response data
- **`AdminController.cs`** — Rewrote avgTurnaround query to fetch approved times to memory before computing TotalHours (EF Core/Npgsql cannot translate TimeSpan.TotalHours)
- **`ProposalService.cs` ReviseAsync** — Append `-R{n}` suffix to revision ProposalNumber to avoid unique index violation
- **`ProposalService.cs` Update/Approve/MarkSent** — Added `.Include(p => p.SubmittedBy)` so mutation responses return submittedByName
### `8666010` Validate dev-login input: reject empty email and invalid role
- **`AuthController.cs`** — Return 400 for empty/whitespace email and invalid role strings; default role changed from Admin to Dispatcher (least privilege)
### `4d72b63` Add frontend role guards, fix dashboard data exposure, and harden UX
- **`ProtectedRoute.tsx`** — New `RoleGuard` component for role-based route protection
- **`App.tsx`** — Wrapped admin routes with `RoleGuard`; `/admin/*` requires Admin/SysAdmin, `/admin/users` requires SysAdmin
- **`Dashboard.tsx`** — Added `mine: true` to dashboard query so dispatchers only see their own proposals
- **`AdminWorkspace.tsx`** — Auto-save dirty changes before approving (was silently discarding edits)
- **`ProposalFormPage.tsx`** — Added onError toast handler; added 300ms debounce on customer autocomplete search
- **`admin.ts`** — Stopped swallowing errors in getPdf; fixed AuditEntry.details type to `string | null`
- **`LoginPage.tsx`** — Fixed pre-existing TS error with noUncheckedIndexedAccess
### `5e89e42` Extract shared constants and format utils, wire admin dashboard filters
- **`constants/index.ts`** — Added shared `STATUS_COLORS` and `PRIORITY_COLORS` (removed from 5 files)
- **`lib/format.ts`** — New shared `formatCurrency`, `formatDate`, `formatDateTime` (removed from 4 files)
- **`AdminDashboard.tsx`** — Wired Category and Priority filter dropdowns to `usePaginatedList` extraParams
- **`ProposalDetailPage.tsx`** — Added 'Revised' to STATUS_ORDER so stepper renders correctly
### QA Coverage Summary
| Test Area | Tests | Pass | Fail | Agent |
|-----------|-------|------|------|-------|
| Auth & RBAC | 35 | 31 | 4 | Auth agent |
| Proposal CRUD & State Machine | 38 | 35 | 3 | Proposal agent |
| Line Items, Customers & Misc | 42 | 39 | 3 | Misc agent |
| Web Frontend Code Review + API | ~30 | ~25 | ~5 | Frontend agent |
| **Total** | **~145** | **~130** | **~15** | — |
## Session 3 Changelog — Mobile Device Testing (2026-05-20)
Fixes on `mobile/fix-react-version-and-ui` (not yet on main):
- **Pin React 19.2.3** — fixes renderer version mismatch crash
- **Remove `import type` from cognito-auth.ts** — fixes eager module init crash
- **Auth fallback to ID token** — `loginWithCredentials` parses user from JWT when backend API unreachable
- **Safe area fixes** — Settings gets top+bottom edges; Dashboard/AdminDashboard use bottom-only (nav header handles top)
- **Pull-to-refresh separation** — filter chip taps no longer trigger refresh animation on proposal queue
- **Welcome name** — shows first name or email prefix instead of full email
- **Service category chips** — wrapping `Chip` components replace truncated `SegmentedButtons`
- **ErrorBoundary** — added to App root for crash visibility
Already on main (sessions 2-3):
- **Email/password login screen** — TextInput form + Cognito SRP via `amazon-cognito-identity-js`
- **Cognito config populated** — real values from AWS CLI
- **CDK callback URL fix** — `com.seahavenind.proposals://auth/callback`
- **netinfo iOS 26 patch** — `patch-package` with `respondsToSelector:` guards
- **Auto-deploy enabled** — `deploy-mobile.yaml` triggers on `mobile/**` push to main
- **Root README updated** — RN 0.85, deploy status corrected
- **`mobile/README.md` created** — local dev, signing, CI/CD docs
## Session 4 Changelog — Web Local Testing (2026-05-20)
All fixes on `mobile/fix-react-version-and-ui` (4 commits, not yet on main):
### `f44caba` Fix JSON enum serialization and audit log jsonb format
- **`Program.cs`** — Added `JsonStringEnumConverter` to `AddJsonOptions` so frontend string enums deserialize correctly
- **`AuditService.cs`** — Wrapped plain-string audit details in `JsonSerializer.Serialize(new { message = details })` for Postgres jsonb column
- **`global.json`** — Relaxed SDK version from 8.0.400 to 8.0.100 to match installed .NET SDK
### `f37c2aa` Fix dev-login role switching, distinct users, and user resolution races
- **`AuthController.cs`** — Dev-login now updates role on existing user; CognitoSub is deterministic (`dev-{email}`)
- **`CurrentUserService.cs`** — Added `DbUpdateException` catch on concurrent user creation with retry lookup
- **`LoginPage.tsx`** — Distinct dev user per role (SysAdmin=Adam, Admin=Sarah, Dispatcher=Mike)
### `9b97b7b` Fix proposal workflow: scoped My Proposals, idempotent state transitions
- **`ProposalService.cs`** — New proposals created as `InReview` (not `Draft`); My Proposals filters by `Mine` parameter (not role); `ApproveAsync`, `MarkSentAsync`, `ReviseAsync` all idempotent
- **`LineItemService.cs`** — Audit writes wrapped in try/catch so failures don't roll back successful saves
- **`ProposalDtos.cs`** — Added `bool Mine` filter parameter
- **`proposals.ts` / `ProposalListPage.tsx`** — Frontend passes `mine: true` for My Proposals page
### `2645d97` Fix customer name not binding from Autocomplete free text input
- **`ProposalFormPage.tsx`** — `onInputChange` with `reason === 'input'` now calls `handleChange('customerName', value)` alongside search

6
web/src/components/admin/SimilarProposalsPanel.tsx
View file

@ -60,12 +60,6 @@ export default function SimilarProposalsPanel({ proposalId, onPullLineItem, disa
Similar Proposals {similar && similar.length > 0 && `(${similar.length})`}
</Typography>
{(!similar || similar.length === 0) && (
<Typography variant="body2" color="text.secondary">
Similar proposals will appear here when available.
</Typography>
)}
{similar?.map((sp, idx) => (
<Accordion key={idx} disableGutters sx={{ '&:before': { display: 'none' }, mb: 1 }}>
<AccordionSummary expandIcon={<ExpandMoreIcon />} sx={{ minHeight: 'auto', px: 1 }}>