diff --git a/api/src/ProposalSystem.Api/Controllers/FilesController.cs b/api/src/ProposalSystem.Api/Controllers/FilesController.cs index 2aba4db..5d4008c 100644 --- a/api/src/ProposalSystem.Api/Controllers/FilesController.cs +++ b/api/src/ProposalSystem.Api/Controllers/FilesController.cs @@ -189,8 +189,9 @@ public class FilesController : ControllerBase }; _db.GeneratedPdfs.Add(generatedPdf); + // Stage-then-single-SaveChanges: the audit row commits atomically with the PDF record. + _audit.Stage(AuditAction.GeneratePDF, proposalId); await _db.SaveChangesAsync(ct); - await _audit.LogAsync(AuditAction.GeneratePDF, proposalId, null, ct); return PhysicalFile(filePath, "application/pdf", Path.GetFileName(filePath)); } diff --git a/api/src/ProposalSystem.Api/Controllers/VendorProposalsController.cs b/api/src/ProposalSystem.Api/Controllers/VendorProposalsController.cs index 619ac5f..04edb34 100644 --- a/api/src/ProposalSystem.Api/Controllers/VendorProposalsController.cs +++ b/api/src/ProposalSystem.Api/Controllers/VendorProposalsController.cs @@ -45,6 +45,12 @@ public class VendorProposalsController : ControllerBase proposal.VendorTotalCost = await _db.VendorProposals .Where(v => v.ProposalId == vendor.ProposalId) .SumAsync(v => v.TotalVendorCost, ct); + // Aggregate write must bump the concurrency version (scanner sweep + // finding): unguarded like line-item appends, but a lost race then + // surfaces as the fallback 409 instead of silently overwriting a + // concurrent guarded edit. + proposal.Version++; + proposal.UpdatedAt = DateTime.UtcNow; await _db.SaveChangesAsync(ct); } diff --git a/api/src/ProposalSystem.Api/Middleware/GlobalExceptionHandler.cs b/api/src/ProposalSystem.Api/Middleware/GlobalExceptionHandler.cs index d8e1fa2..3e0c537 100644 --- a/api/src/ProposalSystem.Api/Middleware/GlobalExceptionHandler.cs +++ b/api/src/ProposalSystem.Api/Middleware/GlobalExceptionHandler.cs @@ -40,9 +40,13 @@ public class GlobalExceptionHandler : IMiddleware return problem; } + // Must match the MVC pipeline's serialization (camelCase + string enums): + // the 409 currentState embeds a ProposalResponse, and clients schema-validate + // it against the same shapes their 200 responses use. private static readonly JsonSerializerOptions CamelCase = new() { PropertyNamingPolicy = JsonNamingPolicy.CamelCase, + Converters = { new System.Text.Json.Serialization.JsonStringEnumConverter() }, }; private async Task HandleExceptionAsync(HttpContext context, Exception exception) diff --git a/api/src/ProposalSystem.Application/Common/ProposalConcurrencyException.cs b/api/src/ProposalSystem.Application/Common/ProposalConcurrencyException.cs index 9e1a637..d1da400 100644 --- a/api/src/ProposalSystem.Application/Common/ProposalConcurrencyException.cs +++ b/api/src/ProposalSystem.Application/Common/ProposalConcurrencyException.cs @@ -1,15 +1,19 @@ +using ProposalSystem.Application.DTOs; + namespace ProposalSystem.Application.Common; /// /// Optimistic-concurrency conflict on the proposal aggregate. Carries the /// freshly reloaded current state so the 409 body can embed it for immediate -/// client refresh (SHOC contract: 409 { message, currentState }). +/// client refresh (SHOC contract: 409 { message, currentState }). Typed as +/// the response DTO so an EF entity (with navigation graphs) can never be +/// serialized into the error body by accident. /// public class ProposalConcurrencyException : Exception { - public object? CurrentState { get; } + public ProposalResponse? CurrentState { get; } - public ProposalConcurrencyException(object? currentState) + public ProposalConcurrencyException(ProposalResponse? currentState) : base("The record was modified by another user. Refresh and retry.") { CurrentState = currentState; diff --git a/api/src/ProposalSystem.Infrastructure/Services/LineItemService.cs b/api/src/ProposalSystem.Infrastructure/Services/LineItemService.cs index ebbe3bc..ca9dad7 100644 --- a/api/src/ProposalSystem.Infrastructure/Services/LineItemService.cs +++ b/api/src/ProposalSystem.Infrastructure/Services/LineItemService.cs @@ -86,7 +86,13 @@ public class LineItemService : ILineItemService public async Task> BulkUpdateAsync(Guid proposalId, BulkUpdateLineItemsRequest request, CancellationToken ct = default) { - var proposal = await _db.Proposals.FindAsync(new object[] { proposalId }, ct) + // Loaded with the display navigations so a pre-check conflict's + // currentState matches the DB-race reload shape (no nulled names). + var proposal = await _db.Proposals + .Include(p => p.SubmittedBy) + .Include(p => p.AssignedAdmin) + .Include(p => p.ApprovedBy) + .FirstOrDefaultAsync(p => p.Id == proposalId, ct) ?? throw new KeyNotFoundException($"Proposal {proposalId} not found"); if (proposal.Status == ProposalStatus.Approved || proposal.Status == ProposalStatus.Sent) diff --git a/api/src/ProposalSystem.Infrastructure/Services/ProposalConcurrencyGuard.cs b/api/src/ProposalSystem.Infrastructure/Services/ProposalConcurrencyGuard.cs index fdd9465..79c30db 100644 --- a/api/src/ProposalSystem.Infrastructure/Services/ProposalConcurrencyGuard.cs +++ b/api/src/ProposalSystem.Infrastructure/Services/ProposalConcurrencyGuard.cs @@ -12,6 +12,13 @@ namespace ProposalSystem.Infrastructure.Services; /// stamp it as EF's original value so the UPDATE's WHERE clause re-enforces it /// at the database. Step 2 (SaveAsync): on a lost race, reload the current /// state and throw the conflict exception that becomes the 409 body. +/// +/// CALLER CONTRACT (AUTHZ-CG-01): the 409 currentState embeds the full +/// ProposalResponse with NO ownership filtering (unlike GetByIdAsync's +/// dispatcher check). Every endpoint that can reach Guard/SaveAsync MUST be +/// [Authorize(Roles = "admins,sysadmins")]. Enforced by +/// GuardedEndpointAuthorizationTests — extend the guard with an ownership +/// predicate before ever wiring it to a dispatcher-reachable write. /// internal static class ProposalConcurrencyGuard { diff --git a/api/src/ProposalSystem.Infrastructure/Services/ProposalService.cs b/api/src/ProposalSystem.Infrastructure/Services/ProposalService.cs index d2202aa..94656bb 100644 --- a/api/src/ProposalSystem.Infrastructure/Services/ProposalService.cs +++ b/api/src/ProposalSystem.Infrastructure/Services/ProposalService.cs @@ -233,6 +233,7 @@ public class ProposalService : IProposalService var proposal = await _db.Proposals .Include(p => p.LineItems) .Include(p => p.SubmittedBy) + .Include(p => p.AssignedAdmin) .Include(p => p.ApprovedBy) .FirstOrDefaultAsync(p => p.Id == id, ct) ?? throw new KeyNotFoundException($"Proposal {id} not found"); @@ -280,6 +281,7 @@ public class ProposalService : IProposalService { var proposal = await _db.Proposals .Include(p => p.SubmittedBy) + .Include(p => p.AssignedAdmin) .Include(p => p.ApprovedBy) .FirstOrDefaultAsync(p => p.Id == id, ct) ?? throw new KeyNotFoundException($"Proposal {id} not found"); @@ -310,6 +312,7 @@ public class ProposalService : IProposalService { var proposal = await _db.Proposals .Include(p => p.SubmittedBy) + .Include(p => p.AssignedAdmin) .Include(p => p.ApprovedBy) .FirstOrDefaultAsync(p => p.Id == id, ct) ?? throw new KeyNotFoundException($"Proposal {id} not found"); @@ -431,6 +434,9 @@ public class ProposalService : IProposalService { var proposal = await _db.Proposals .Include(p => p.LineItems) + .Include(p => p.SubmittedBy) + .Include(p => p.AssignedAdmin) + .Include(p => p.ApprovedBy) .FirstOrDefaultAsync(p => p.Id == id, ct) ?? throw new KeyNotFoundException($"Proposal {id} not found"); diff --git a/api/tests/ProposalSystem.Tests/Controllers/GuardedEndpointAuthorizationTests.cs b/api/tests/ProposalSystem.Tests/Controllers/GuardedEndpointAuthorizationTests.cs new file mode 100644 index 0000000..3361694 --- /dev/null +++ b/api/tests/ProposalSystem.Tests/Controllers/GuardedEndpointAuthorizationTests.cs @@ -0,0 +1,44 @@ +using System.Reflection; +using FluentAssertions; +using Microsoft.AspNetCore.Authorization; +using ProposalSystem.Api.Controllers; +using Xunit; + +namespace ProposalSystem.Tests.Controllers; + +/// +/// AUTHZ-CG-01 tripwire: ProposalConcurrencyGuard's 409 currentState embeds the +/// full ProposalResponse with no ownership filtering, so every endpoint that can +/// reach the guard must stay admin-gated. If a new/changed endpoint wires a +/// guarded mutation to a dispatcher-reachable route, this test fails the build +/// until the guard gains an ownership predicate. +/// +public class GuardedEndpointAuthorizationTests +{ + public static readonly TheoryData GuardedEndpoints = new() + { + { typeof(ProposalsController), "Update" }, + { typeof(ProposalsController), "Approve" }, + { typeof(ProposalsController), "ReturnToReview" }, + { typeof(ProposalsController), "MarkSent" }, + { typeof(ProposalsController), "Revise" }, + { typeof(LineItemsController), "BulkUpdate" }, + }; + + [Theory(DisplayName = "Guard-reaching endpoints require admins/sysadmins")] + [MemberData(nameof(GuardedEndpoints))] + public void GuardedEndpoint_RequiresAdminRole(Type controller, string actionName) + { + var action = controller.GetMethod(actionName, BindingFlags.Public | BindingFlags.Instance); + action.Should().NotBeNull($"{controller.Name}.{actionName} should exist — update GuardedEndpoints if renamed"); + + var authorize = action!.GetCustomAttributes(inherit: true) + .FirstOrDefault(a => a.Roles != null); + + authorize.Should().NotBeNull( + $"{controller.Name}.{actionName} reaches ProposalConcurrencyGuard and must carry a role-restricted [Authorize]"); + authorize!.Roles.Should().Contain("admins").And.Contain("sysadmins"); + authorize.Roles.Should().NotContain("dispatchers", + "the guard's 409 currentState has no ownership filter — see ProposalConcurrencyGuard caller contract"); + } +} diff --git a/api/tests/ProposalSystem.Tests/Middleware/GlobalExceptionHandlerTests.cs b/api/tests/ProposalSystem.Tests/Middleware/GlobalExceptionHandlerTests.cs index 33ea6c1..5de457b 100644 --- a/api/tests/ProposalSystem.Tests/Middleware/GlobalExceptionHandlerTests.cs +++ b/api/tests/ProposalSystem.Tests/Middleware/GlobalExceptionHandlerTests.cs @@ -98,17 +98,31 @@ public class GlobalExceptionHandlerTests // These are SHOC's shapes verbatim, NOT ProblemDetails. Both bodies are wire // contract: web/mobile branch on message + currentState / status + code. + private static ProposalSystem.Application.DTOs.ProposalResponse SampleState(Guid id) => new( + id, "P-001", "WO-1", null, "Customer", "Addr", "Scope", null, + ProposalSystem.Domain.Entities.ServiceCategory.HVAC, + ProposalSystem.Domain.Entities.Priority.Standard, + ProposalSystem.Domain.Entities.ProposalStatus.InReview, + 100m, null, "winner", Guid.NewGuid(), "Submitter", DateTime.UtcNow, + null, null, null, null, 1, null, DateTime.UtcNow, DateTime.UtcNow, + "AAAAAAAAAAI="); + [Fact(DisplayName = "ProposalConcurrencyException maps to 409 { message, currentState } (SHOC envelope)")] public async Task ConcurrencyException_Maps409WithCurrentState() { - var (status, body) = await InvokeWith( - new ProposalConcurrencyException(new { Id = "p1", Notes = "winner" })); + var id = Guid.NewGuid(); + var (status, body) = await InvokeWith(new ProposalConcurrencyException(SampleState(id))); status.Should().Be(409); body.GetProperty("message").GetString() .Should().Be("The record was modified by another user. Refresh and retry."); - body.GetProperty("currentState").GetProperty("id").GetString().Should().Be("p1"); + body.GetProperty("currentState").GetProperty("id").GetString().Should().Be(id.ToString()); body.GetProperty("currentState").GetProperty("notes").GetString().Should().Be("winner"); + body.GetProperty("currentState").GetProperty("rowVersion").GetString().Should().Be("AAAAAAAAAAI="); + // Enums must serialize as strings (matching the MVC pipeline), or client + // schema validation of currentState fails and the state is discarded. + body.GetProperty("currentState").GetProperty("serviceCategory").GetString().Should().Be("HVAC"); + body.GetProperty("currentState").GetProperty("status").GetString().Should().Be("InReview"); // The guarded envelope is NOT the fallback shape. body.TryGetProperty("status", out _).Should().BeFalse(); body.TryGetProperty("code", out _).Should().BeFalse(); diff --git a/lambdas/suggestions/app.py b/lambdas/suggestions/app.py index 424e430..d22a705 100644 --- a/lambdas/suggestions/app.py +++ b/lambdas/suggestions/app.py @@ -447,17 +447,36 @@ def post_line_items(proposal_id: str, items: list[dict], existing_items: list[di ) try: - resp = _retry_request( - "PUT", - f"{API_BASE_URL}/api/proposals/{proposal_id}/line-items", - json={"lineItems": line_items_payload}, - headers=_api_headers(), - timeout=15, - ) - if resp.status_code not in (200, 201): + # Fix: CONC-L1 — the bulk PUT is version-guarded (ADR 0004): echo the + # proposal's current rowVersion as proposalVersion, and retry once with + # a fresh token if a concurrent edit wins the race (409/stale 422). + for attempt in range(2): + proposal = fetch_proposal(proposal_id) + token = (proposal or {}).get("rowVersion") + if not token: + logger.error( + "Cannot post line items: no rowVersion for proposal %s", proposal_id + ) + return + resp = _retry_request( + "PUT", + f"{API_BASE_URL}/api/proposals/{proposal_id}/line-items", + json={"lineItems": line_items_payload, "proposalVersion": token}, + headers=_api_headers(), + timeout=15, + ) + if resp.status_code in (200, 201): + return + if resp.status_code == 409 and attempt == 0: + logger.warning( + "Concurrency conflict posting line items for %s; retrying with fresh token", + proposal_id, + ) + continue logger.error( "Failed to post line items: %s %s", resp.status_code, resp.text ) + return except Exception as e: # Fix: LAM-M5 — include stack trace in error logging logger.exception("Error posting line items: %s", e) diff --git a/lambdas/tests/test_suggestions.py b/lambdas/tests/test_suggestions.py index 641cdfc..2edca76 100644 --- a/lambdas/tests/test_suggestions.py +++ b/lambdas/tests/test_suggestions.py @@ -138,9 +138,14 @@ class TestProcessSuggestion: @patch.object(suggestions_app, "_retry_request") def test_post_line_items_preserves_non_ai_items(self, mock_request): """QA-C6: Existing non-AI items are preserved when new suggestions are generated.""" - mock_response = MagicMock() - mock_response.status_code = 200 - mock_request.return_value = mock_response + # Fix: CONC-L1 — post_line_items now fetches the proposal's rowVersion + # first and echoes it as proposalVersion on the guarded bulk PUT. + get_response = MagicMock() + get_response.status_code = 200 + get_response.json.return_value = {"rowVersion": "AAAAAAAAAAE="} + put_response = MagicMock() + put_response.status_code = 200 + mock_request.side_effect = [get_response, put_response] existing_items = [ { @@ -175,11 +180,13 @@ class TestProcessSuggestion: suggestions_app.post_line_items("abc", new_items, existing_items) - # Verify the API was called - mock_request.assert_called_once() + # GET (token fetch) + PUT (guarded bulk update) + assert mock_request.call_count == 2 call_kwargs = mock_request.call_args payload = call_kwargs.kwargs.get("json") or call_kwargs[1].get("json") + # The guarded PUT must echo the proposal's version token + assert payload["proposalVersion"] == "AAAAAAAAAAE=" # Should have 2 items: 1 preserved Manual + 1 new AI (old AI items are replaced) assert len(payload["lineItems"]) == 2 sources = [li["source"] for li in payload["lineItems"]] @@ -191,6 +198,37 @@ class TestProcessSuggestion: assert payload["lineItems"][1]["source"] == "AI" assert payload["lineItems"][1]["description"] == "New AI item" + @patch.object(suggestions_app, "_retry_request") + def test_post_line_items_retries_once_on_conflict(self, mock_request): + """CONC-L1: a 409 conflict refetches the token and retries the PUT once.""" + get_1 = MagicMock(status_code=200) + get_1.json.return_value = {"rowVersion": "AAAAAAAAAAE="} + put_conflict = MagicMock(status_code=409, text="conflict") + get_2 = MagicMock(status_code=200) + get_2.json.return_value = {"rowVersion": "AAAAAAAAAAI="} + put_ok = MagicMock(status_code=200) + mock_request.side_effect = [get_1, put_conflict, get_2, put_ok] + + suggestions_app.post_line_items( + "abc", + [ + { + "description": "AI item", + "quantity": 1, + "unit": "each", + "totalPrice": 100, + "pricingMode": "TotalPrice", + } + ], + [], + ) + + assert mock_request.call_count == 4 + final_payload = mock_request.call_args.kwargs.get( + "json" + ) or mock_request.call_args[1].get("json") + assert final_payload["proposalVersion"] == "AAAAAAAAAAI=" + @patch.object(suggestions_app, "post_line_items") @patch.object(suggestions_app, "generate_line_items") @patch.object(suggestions_app, "retrieve_similar") diff --git a/mobile/src/lib/api/admin.ts b/mobile/src/lib/api/admin.ts index 634c795..3fb41f2 100644 --- a/mobile/src/lib/api/admin.ts +++ b/mobile/src/lib/api/admin.ts @@ -1,4 +1,5 @@ import apiClient from './client'; +import type { ProposalDetail } from './proposals'; export interface DashboardStats { pendingCount: number; @@ -11,6 +12,8 @@ export interface UpdateProposalRequest { refinedScope?: string; notes?: string; assignedAdminId?: string; + /** Expected proposal rowVersion — REQUIRED by the server (422 ProposalVersionRequired when missing). */ + proposalVersion?: string; } export interface AuditEntry { @@ -30,23 +33,47 @@ export const adminApi = { return res.data; }, + // Fix: CONC-L2 — guarded mutations (optimistic concurrency): every write + // below requires the proposal's rowVersion token and returns the fresh + // proposal state — the server rotates the token on each guarded write, so + // callers chain from the RESPONSE's rowVersion, never the one they started + // with. updateProposal: async ( id: string, data: UpdateProposalRequest, - ): Promise => { - await apiClient.put(`/proposals/${id}`, data); + ): Promise => { + const res = await apiClient.put(`/proposals/${id}`, data); + return res.data; }, - approveProposal: async (id: string): Promise => { - await apiClient.post(`/proposals/${id}/approve`); + approveProposal: async ( + id: string, + proposalVersion?: string, + ): Promise => { + const res = await apiClient.post(`/proposals/${id}/approve`, { + proposalVersion, + }); + return res.data; }, - sendProposal: async (id: string): Promise => { - await apiClient.post(`/proposals/${id}/send`); + sendProposal: async ( + id: string, + proposalVersion?: string, + ): Promise => { + const res = await apiClient.post(`/proposals/${id}/send`, { + proposalVersion, + }); + return res.data; }, - reviseProposal: async (id: string): Promise => { - await apiClient.post(`/proposals/${id}/revise`); + reviseProposal: async ( + id: string, + proposalVersion?: string, + ): Promise => { + const res = await apiClient.post(`/proposals/${id}/revise`, { + proposalVersion, + }); + return res.data; }, getAudit: async (id: string): Promise => { diff --git a/mobile/src/lib/api/client.ts b/mobile/src/lib/api/client.ts index 82c5868..d3fdeab 100644 --- a/mobile/src/lib/api/client.ts +++ b/mobile/src/lib/api/client.ts @@ -2,6 +2,19 @@ import axios from 'axios'; import Config from '../../config'; import { tokenStorage } from '../storage'; +// Fix: CONC-L2 — carries HTTP status + response body so screens can branch on +// optimistic-concurrency conflicts (409 { message, currentState }). +export class ApiError extends Error { + constructor( + message: string, + public status: number, + public data?: unknown, + ) { + super(message); + this.name = 'ApiError'; + } +} + type RefreshFn = () => Promise; let _refreshTokens: RefreshFn | null = null; @@ -108,7 +121,7 @@ apiClient.interceptors.response.use( const message = data?.detail || data?.title || data?.message || 'An error occurred'; - return Promise.reject(new Error(message)); + return Promise.reject(new ApiError(message, status, data)); } if (error.request) { diff --git a/mobile/src/lib/api/lineItems.ts b/mobile/src/lib/api/lineItems.ts index a8f759c..4e3ee24 100644 --- a/mobile/src/lib/api/lineItems.ts +++ b/mobile/src/lib/api/lineItems.ts @@ -14,6 +14,8 @@ export interface LineItem { source: LineItemSource; createdAt: string; updatedAt: string; + /** Opaque optimistic-concurrency token (base64); echo back untouched. */ + rowVersion: string; } export interface UpdateLineItemEntry { @@ -34,12 +36,16 @@ export const lineItemsApi = { return res.data; }, + // Fix: CONC-L2 — the bulk replace is guarded by the PROPOSAL rowVersion + // token; the server rotates it on success, so refetch the proposal after. bulkUpdate: async ( proposalId: string, lineItems: UpdateLineItemEntry[], + proposalVersion?: string, ): Promise => { const res = await apiClient.put(`/proposals/${proposalId}/line-items`, { lineItems, + proposalVersion, }); return res.data; }, diff --git a/mobile/src/lib/api/proposals.ts b/mobile/src/lib/api/proposals.ts index 39cc916..2ed1a5b 100644 --- a/mobile/src/lib/api/proposals.ts +++ b/mobile/src/lib/api/proposals.ts @@ -13,6 +13,8 @@ export interface ProposalListItem { submittedAt: string; submittedByName: string | null; assignedAdminName: string | null; + /** Opaque optimistic-concurrency token (base64); echo back untouched. */ + rowVersion: string; } export interface ProposalDetail { @@ -40,6 +42,8 @@ export interface ProposalDetail { parentProposalId: string | null; createdAt: string; updatedAt: string; + /** Opaque optimistic-concurrency token (base64); echo back untouched. */ + rowVersion: string; } export interface PagedResponse { diff --git a/mobile/src/screens/admin/LineItemEditScreen.tsx b/mobile/src/screens/admin/LineItemEditScreen.tsx index 44d201b..feb7c15 100644 --- a/mobile/src/screens/admin/LineItemEditScreen.tsx +++ b/mobile/src/screens/admin/LineItemEditScreen.tsx @@ -16,7 +16,9 @@ import { } from 'react-native-paper'; import { useRoute, useNavigation } from '@react-navigation/native'; import { useMutation, useQueryClient } from '@tanstack/react-query'; +import { ApiError } from '../../lib/api/client'; import { lineItemsApi, type UpdateLineItemEntry } from '../../lib/api/lineItems'; +import { proposalsApi, type ProposalDetail } from '../../lib/api/proposals'; import { QUERY_KEYS } from '../../constants/queryKeys'; import type { NativeStackScreenProps } from '@react-navigation/native-stack'; import type { ProposalsStackParamList } from '../../navigation/types'; @@ -98,16 +100,39 @@ export function LineItemEditScreen() { { ...entry, sortOrder: existing.length }, ]; - return lineItemsApi.bulkUpdate(proposalId, updated); + // Fix: CONC-L2 — the bulk replace is guarded by the proposal's + // rowVersion token; use the one the workspace already loaded, or fetch + // a fresh one if the cache is empty. + const cachedProposal = qc.getQueryData([ + QUERY_KEYS.proposal, + proposalId, + ]); + const proposalVersion = + cachedProposal?.rowVersion ?? + (await proposalsApi.getById(proposalId)).rowVersion; + + return lineItemsApi.bulkUpdate(proposalId, updated, proposalVersion); }, onSuccess: () => { qc.invalidateQueries({ queryKey: [QUERY_KEYS.proposalLineItems, proposalId], }); + // Fix: CONC-L2 — the server rotated the proposal token; refetch it. + qc.invalidateQueries({ queryKey: [QUERY_KEYS.proposal, proposalId] }); setSnackbar('Line item saved'); setTimeout(() => navigation.goBack(), 800); }, - onError: (err: Error) => Alert.alert('Error', err.message), + onError: (err: Error) => { + Alert.alert('Error', err.message); + // Fix: CONC-L2 — on a 409 concurrency conflict, refetch so the + // workspace picks up the fresh state + rotated token. + if (err instanceof ApiError && err.status === 409) { + qc.invalidateQueries({ queryKey: [QUERY_KEYS.proposal, proposalId] }); + qc.invalidateQueries({ + queryKey: [QUERY_KEYS.proposalLineItems, proposalId], + }); + } + }, }); const handleAutoCalc = () => { diff --git a/mobile/src/screens/admin/ProposalWorkspaceScreen.tsx b/mobile/src/screens/admin/ProposalWorkspaceScreen.tsx index 81b4829..cff0b18 100644 --- a/mobile/src/screens/admin/ProposalWorkspaceScreen.tsx +++ b/mobile/src/screens/admin/ProposalWorkspaceScreen.tsx @@ -24,6 +24,7 @@ import ReactNativeHapticFeedback from 'react-native-haptic-feedback'; import { StatusChip } from '../../components/StatusChip'; import { PriorityChip } from '../../components/PriorityChip'; import { LineItemRow } from '../../components/LineItemRow'; +import { ApiError } from '../../lib/api/client'; import { proposalsApi } from '../../lib/api/proposals'; import { lineItemsApi } from '../../lib/api/lineItems'; import { adminApi } from '../../lib/api/admin'; @@ -67,31 +68,45 @@ export function ProposalWorkspaceScreen() { }); }; + // Fix: CONC-L2 — on a 409 concurrency conflict, surface the server message + // and refetch so the workspace picks up the fresh state + rotated token. + const handleGuardedError = (err: Error) => { + Alert.alert('Error', err.message); + if (err instanceof ApiError && err.status === 409) { + invalidate(); + } + }; + const approveMutation = useMutation({ - mutationFn: () => adminApi.approveProposal(proposalId), - onSuccess: () => { + mutationFn: () => + adminApi.approveProposal(proposalId, proposal?.rowVersion), + onSuccess: (updated) => { ReactNativeHapticFeedback.trigger('notificationSuccess'); setSnackbar('Proposal approved'); + qc.setQueryData([QUERY_KEYS.proposal, proposalId], updated); invalidate(); }, - onError: (err: Error) => Alert.alert('Error', err.message), + onError: handleGuardedError, }); const sendMutation = useMutation({ - mutationFn: () => adminApi.sendProposal(proposalId), - onSuccess: () => { + mutationFn: () => adminApi.sendProposal(proposalId, proposal?.rowVersion), + onSuccess: (updated) => { setSnackbar('Marked as sent'); + qc.setQueryData([QUERY_KEYS.proposal, proposalId], updated); invalidate(); }, - onError: (err: Error) => Alert.alert('Error', err.message), + onError: handleGuardedError, }); const regenerateMutation = useMutation({ mutationFn: async () => { if (scopeInput.trim()) { - await adminApi.updateProposal(proposalId, { + const updated = await adminApi.updateProposal(proposalId, { refinedScope: scopeInput.trim(), + proposalVersion: proposal?.rowVersion, }); + qc.setQueryData([QUERY_KEYS.proposal, proposalId], updated); } await adminApi.generateSuggestions(proposalId); }, @@ -100,7 +115,7 @@ export function ProposalWorkspaceScreen() { setSnackbar('AI suggestions regenerating...'); setTimeout(invalidate, 5000); }, - onError: (err: Error) => Alert.alert('Error', err.message), + onError: handleGuardedError, }); const handleDownloadPdf = async () => { diff --git a/web/src/domain/admin/use-cases.ts b/web/src/domain/admin/use-cases.ts index 36c9d63..69abe3c 100644 --- a/web/src/domain/admin/use-cases.ts +++ b/web/src/domain/admin/use-cases.ts @@ -60,7 +60,10 @@ function handleConcurrencyConflict( error: Error, ): boolean { if (!(error instanceof ConflictError)) return false; - if (error.currentState) { + // Only seed the cache when the embedded state is actually this proposal — + // a mismatched or partial envelope falls through to invalidation, and the + // refetch restores truth. + if (error.currentState && error.currentState.id === proposalId) { queryClient.setQueryData(proposalsKeys.detail(proposalId), error.currentState); } invalidateProposalViews(queryClient, proposalId); @@ -108,10 +111,21 @@ export function useSaveProposalWorkspace(proposalId: string) { // The bulk replace rotates the token AGAIN but returns only line items — // refetch the detail so a chained guarded action (approve-after-save) // holds the current token instead of racing the invalidation refetch. - return proposalsApi.getById(proposalId); + // Both writes are already committed here: if only this trailing GET + // fails, the save must still report success, with invalidation + // recovering the token instead (CONC-L4). + try { + return await proposalsApi.getById(proposalId); + } catch { + return null; + } }, onSuccess: (fresh) => { - queryClient.setQueryData(proposalsKeys.detail(proposalId), fresh); + if (fresh) { + queryClient.setQueryData(proposalsKeys.detail(proposalId), fresh); + } else { + queryClient.removeQueries({ queryKey: proposalsKeys.detail(proposalId) }); + } invalidateProposalViews(queryClient, proposalId); toast.success('Changes saved'); }, diff --git a/web/src/lib/api/__tests__/client.test.ts b/web/src/lib/api/__tests__/client.test.ts index 7f02999..7ed530d 100644 --- a/web/src/lib/api/__tests__/client.test.ts +++ b/web/src/lib/api/__tests__/client.test.ts @@ -212,7 +212,36 @@ describe('API client interceptors', () => { }); it('409 with the guarded conflict envelope rejects with ConflictError carrying currentState', async () => { - const currentState = { id: 'p-1', status: 'Approved', rowVersion: 'AAAAAAAAAAM=' }; + // Must be schema-valid: the interceptor validates the envelope with + // proposalConcurrencyConflictSchema before trusting currentState. + const currentState = { + id: 'p-1', + proposalNumber: 'P-2026-001', + workOrderNumber: 'WO-1', + poNumber: null, + customerName: 'Cust', + customerAddress: 'Addr', + scopeOfWork: 'Scope', + refinedScope: null, + serviceCategory: 'HVAC', + priority: 'Standard', + status: 'Approved', + totalBidAmount: 100, + vendorTotalCost: null, + notes: '', + submittedById: 'u-1', + submittedByName: 'Submitter', + submittedAt: '2026-07-01T00:00:00Z', + assignedAdminId: null, + approvedById: null, + approvedAt: null, + sentAt: null, + currentRevision: 1, + parentProposalId: null, + createdAt: '2026-07-01T00:00:00Z', + updatedAt: '2026-07-01T00:00:00Z', + rowVersion: 'AAAAAAAAAAM=', + }; const error = { response: { status: 409, @@ -231,6 +260,27 @@ describe('API client interceptors', () => { await expect(promise).rejects.toBeInstanceOf(ConflictError); }); + it('409 with a malformed currentState degrades to a state-less ConflictError', async () => { + const error = { + response: { + status: 409, + data: { + message: 'Proposal was modified by another user.', + currentState: { id: 'p-1', rowVersion: 12345, unexpected: 'shape' }, + }, + }, + request: {}, + }; + + const promise = interceptors.responseRejected(error); + + await expect(promise).rejects.toMatchObject({ + name: 'ConflictError', + message: 'Proposal was modified by another user.', + currentState: null, + }); + }); + it('409 with the unguarded fallback envelope rejects with ConflictError and null currentState', async () => { const error = { response: { diff --git a/web/src/lib/api/client.ts b/web/src/lib/api/client.ts index 0c464b4..5e29813 100644 --- a/web/src/lib/api/client.ts +++ b/web/src/lib/api/client.ts @@ -1,6 +1,7 @@ import axios from 'axios'; import { API_URL, STORAGE_KEY_TOKEN } from '../../constants'; import { AUTH_SESSION_CLEARED_EVENT, clearAuth } from '../auth/authStorage'; +import { proposalConcurrencyConflictSchema } from '@proposal-system/api-contracts/schemas'; import { ConflictError } from './errors'; const apiClient = axios.create({ @@ -55,12 +56,16 @@ apiClient.interceptors.response.use( if (status === 409) { // Optimistic-concurrency conflict (SHOC ADR 0004): the guarded path // carries { message, currentState }; the unguarded fallback carries - // { status, message, code } with no state. Throw a typed error so the - // domain layer can refresh from currentState instead of just toasting. + // { status, message, code } with no state. Validate the envelope before + // trusting it — a malformed currentState must never seed typed caches, + // so parse failures degrade to a state-less conflict (refetch recovers). + const parsed = proposalConcurrencyConflictSchema.safeParse(data); return Promise.reject( new ConflictError( - data?.message || 'This record was modified by another user. Refresh and retry.', - data?.currentState ?? null, + parsed.success + ? parsed.data.message + : data?.message || 'This record was modified by another user. Refresh and retry.', + parsed.success ? parsed.data.currentState : null, ), ); }