diff --git a/docs/openapi/api.yaml b/docs/openapi/api.yaml index 349d53adbe..9d81eb4e1f 100644 --- a/docs/openapi/api.yaml +++ b/docs/openapi/api.yaml @@ -325,6 +325,8 @@ paths: $ref: './site-opportunities.yaml#/site-opportunity-suggestion-fixes' /sites/{siteId}/opportunities/{opportunityId}/suggestions/{suggestionId}/backoffice-reviews: $ref: './site-opportunities.yaml#/site-opportunity-suggestion-backoffice-reviews' + /sites/{siteId}/opportunities/{opportunityId}/suggestions/{suggestionId}/aso-reviews: + $ref: './site-opportunities.yaml#/site-opportunity-suggestion-aso-reviews' /sites/{siteId}/opportunities/{opportunityId}/suggestions/status: $ref: './site-opportunities.yaml#/site-opportunity-suggestions-status' /sites/{siteId}/opportunities/{opportunityId}/suggestions/auto-fix: diff --git a/docs/openapi/site-opportunities.yaml b/docs/openapi/site-opportunities.yaml index a5ca3185b7..97195878f8 100644 --- a/docs/openapi/site-opportunities.yaml +++ b/docs/openapi/site-opportunities.yaml @@ -1330,7 +1330,99 @@ site-opportunity-suggestion-backoffice-reviews: '404': $ref: './responses.yaml#/404' '413': - description: detail_markdown exceeds the 8 KB limit + description: detail_markdown exceeds the 8 KB limit, or guidance_markdown exceeds the 64 KB limit + '500': + $ref: './responses.yaml#/500' + '503': + $ref: './responses.yaml#/503' + security: + - ims_key: [ ] + +site-opportunity-suggestion-aso-reviews: + parameters: + - $ref: './parameters.yaml#/siteId' + - $ref: './parameters.yaml#/opportunityId' + - $ref: './parameters.yaml#/suggestionId' + post: + operationId: createAsoReview + summary: | + Capture a customer review verdict for a CWV suggestion (ASO UI feedback loop) + description: | + Records a single customer-review feedback event for the suggestion as part of + the Learning Agent feedback loop (SITES-39002). `source` is bound to `aso_ui` + by this route and must not be supplied in the body. `eventId` is a mandatory + client-supplied idempotency key (UUID v4) — a duplicate POST with the same + `eventId` collapses to a no-op and returns 200 with the existing review. + Customer-derived fields are secret-scrubbed and markdown is sanitised + server-side before storage; raw patches are not echoed in the response. + Feedback is pure signal — it does not change the suggestion's status. + tags: + - opportunity-suggestions + requestBody: + required: true + content: + application/json: + schema: + type: object + required: + - eventId + - verdict + properties: + eventId: + type: string + format: uuid + description: Client-supplied idempotency key (UUID v4), retained across retries. + verdict: + type: string + enum: [up, down] + description: The customer's verdict (translated server-side to a positive/negative signal). + detailMarkdown: + type: string + maxLength: 8192 + description: Optional customer feedback (markdown). Required on reject by the UI; sanitised + 8 KB cap server-side. + guidanceMarkdown: + type: string + maxLength: 65536 + description: >- + AI-generated issue context (title + description) for the reviewed issue. + Gives the Learning Agent "what the issue was" alongside the generated + patch (previousFix) and the review. Sanitised + 64 KB cap server-side. + feedbackSubjectId: + type: string + maxLength: 200 + description: >- + Optional SpaceCat-internal id of the sub-item this review is about + within the suggestion (a CWV issue id). Not exported to S3. + rejectionCategory: + type: string + enum: [bad_recommendation, other] + description: Optional category on a reject. `product_bug` is not accepted from the ASO UI. + previousFix: + type: object + description: Generated patch snapshot at review time (secret-scrubbed server-side). + responses: + '201': + description: Review recorded + content: + application/json: + schema: + $ref: './schemas.yaml#/SuggestionReview' + '200': + description: Idempotent no-op — a review with this eventId already exists + content: + application/json: + schema: + $ref: './schemas.yaml#/SuggestionReview' + '400': + $ref: './responses.yaml#/400' + '401': + $ref: './responses.yaml#/401' + '403': + $ref: './responses.yaml#/403' + '404': + $ref: './responses.yaml#/404' + '413': + description: detail_markdown exceeds the 8 KB limit, or guidance_markdown exceeds the 64 KB limit '500': $ref: './responses.yaml#/500' '503': diff --git a/src/controllers/suggestions.js b/src/controllers/suggestions.js index 184209e4ea..127d8a89db 100644 --- a/src/controllers/suggestions.js +++ b/src/controllers/suggestions.js @@ -2889,32 +2889,46 @@ function SuggestionsController(ctx, sqs, env) { }; /** - * Capture an ESE review verdict from the Backoffice (SITES-43974 / SITES-39001). + * Shared core for capturing a human review verdict on a suggestion (SITES-43974 + * / SITES-39002). Used by both the Backoffice (ESE) and the ASO UI (customer) + * review endpoints — the caller supplies the `source` (bound by the route, + * never trusted from the body — FR-10) and the rejection categories allowed for + * that source. * - * POST /sites/:siteId/opportunities/:opportunityId/suggestions/:suggestionId/backoffice-reviews - * - * `source` is bound to 'backoffice' by the route (never trusted from the body — - * FR-10). `event_id` is a mandatory client-supplied idempotency key (FR-09): - * a duplicate collapses to a no-op (HTTP 200 with the existing row). Customer- - * derived fields are secret-scrubbed and the markdown is sanitised before - * insert. The raw patches are NOT echoed in the response. + * `event_id` is a mandatory client-supplied idempotency key (FR-09): a duplicate + * collapses to a no-op (HTTP 200 with the existing row). Customer-derived fields + * are secret-scrubbed and the markdown is sanitised before insert. The raw + * patches are NOT echoed in the response. * * @param {Object} context - request context. + * @param {Object} opts + * @param {string} opts.source - one of REVIEW_SOURCES; stamped on the row. + * @param {string[]} opts.allowedRejectionCategories - categories accepted for this source. * @returns {Promise} */ - const createBackofficeReview = async (context) => { + const captureReview = async (context, { source, allowedRejectionCategories }) => { const siteId = context.params?.siteId; const opptyId = context.params?.opportunityId; const suggestionId = context.params?.suggestionId; + // Emit a single structured warn on every non-2xx exit so a customer-facing + // (aso_ui) rejection is observable server-side — the ASO UI has no client-side + // error reporting, so these logs are the only signal. Splunk can slice by + // `source` (aso_ui vs backoffice) and `reason`. Mirrors the scrub_hit_total + // format; shared by both routes via captureReview. + const rejected = (reason, response) => { + context.log?.warn?.(`feedback_capture.rejected source=${source} suggestion=${suggestionId} reason=${reason}`); + return response; + }; + if (!isValidUUID(siteId)) { - return badRequest('Site ID required'); + return rejected('invalid_site_id', badRequest('Site ID required')); } if (!isValidUUID(opptyId)) { - return badRequest('Opportunity ID required'); + return rejected('invalid_opportunity_id', badRequest('Opportunity ID required')); } if (!isValidUUID(suggestionId)) { - return badRequest('Suggestion ID required'); + return rejected('invalid_suggestion_id', badRequest('Suggestion ID required')); } const body = isNonEmptyObject(context.data) ? context.data : {}; @@ -2925,28 +2939,28 @@ function SuggestionsController(ctx, sqs, env) { // FR-09: event_id is MANDATORY and client-supplied (no server fallback). if (!hasText(eventId) || !isValidUUID(eventId)) { - return badRequest('event_id is required and must be a UUID'); + return rejected('invalid_event_id', badRequest('event_id is required and must be a UUID')); } // FR-10: a client must not self-assert a higher-trust source. - if (hasText(body.source) && body.source !== REVIEW_SOURCES.BACKOFFICE) { - return badRequest('source is derived from the route and must not be set in the body'); + if (hasText(body.source) && body.source !== source) { + return rejected('source_mismatch', badRequest('source is derived from the route and must not be set in the body')); } if (verdict !== REVIEW_VERDICTS.UP && verdict !== REVIEW_VERDICTS.DOWN) { - return badRequest('verdict must be "up" or "down"'); + return rejected('invalid_verdict', badRequest('verdict must be "up" or "down"')); } if (rejectionCategory != null - && !Object.values(REJECTION_CATEGORIES).includes(rejectionCategory)) { - return badRequest('invalid rejection_category'); + && !allowedRejectionCategories.includes(rejectionCategory)) { + return rejected('invalid_rejection_category', badRequest('invalid rejection_category')); } if (stateTransition != null && !FEEDBACK_STATE_TRANSITIONS.includes(stateTransition)) { - return badRequest('invalid state_transition'); + return rejected('invalid_state_transition', badRequest('invalid state_transition')); } if (detailMarkdown != null) { if (typeof detailMarkdown !== 'string') { - return badRequest('detail_markdown must be a string'); + return rejected('invalid_detail_markdown', badRequest('detail_markdown must be a string')); } if (Buffer.byteLength(detailMarkdown, 'utf8') > 8192) { - return createResponse({ message: 'detail_markdown exceeds the 8 KB limit' }, 413); + return rejected('detail_markdown_too_large', createResponse({ message: 'detail_markdown exceeds the 8 KB limit' }, 413)); } } // guidance_markdown is the AI-generated issue context (title + description). @@ -2954,40 +2968,40 @@ function SuggestionsController(ctx, sqs, env) { // implementation guidance run long. if (guidanceMarkdown != null) { if (typeof guidanceMarkdown !== 'string') { - return badRequest('guidance_markdown must be a string'); + return rejected('invalid_guidance_markdown', badRequest('guidance_markdown must be a string')); } if (Buffer.byteLength(guidanceMarkdown, 'utf8') > 65536) { - return createResponse({ message: 'guidance_markdown exceeds the 64 KB limit' }, 413); + return rejected('guidance_markdown_too_large', createResponse({ message: 'guidance_markdown exceeds the 64 KB limit' }, 413)); } } // feedback_subject_id is an opaque grouping id (e.g. a CWV issue id) — a short // string, not free text. Bounded to guard against abuse. if (feedbackSubjectId != null) { if (typeof feedbackSubjectId !== 'string' || feedbackSubjectId.length > 200) { - return badRequest('feedback_subject_id must be a string of at most 200 characters'); + return rejected('invalid_feedback_subject_id', badRequest('feedback_subject_id must be a string of at most 200 characters')); } } const site = await Site.findById(siteId); if (!site) { - return notFound('Site not found'); + return rejected('site_not_found', notFound('Site not found')); } if (!await accessControlUtil.hasAccess(site)) { - return forbidden('User does not belong to the organization'); + return rejected('forbidden', forbidden('User does not belong to the organization')); } const suggestion = await Suggestion.findById(suggestionId); if (!suggestion || suggestion.getOpportunityId() !== opptyId) { - return notFound('Suggestion not found'); + return rejected('suggestion_not_found', notFound('Suggestion not found')); } const opportunity = await suggestion.getOpportunity(); if (!opportunity || opportunity.getSiteId() !== siteId) { - return notFound('Suggestion not found'); + return rejected('suggestion_not_found', notFound('Suggestion not found')); } const postgrestClient = context.dataAccess?.services?.postgrestClient; if (!postgrestClient?.from) { - return createResponse({ message: 'Feedback store unavailable' }, 503); + return rejected('store_unavailable', createResponse({ message: 'Feedback store unavailable' }, 503)); } // reviewer_id is server-derived from the authenticated principal — never the body. @@ -3023,7 +3037,7 @@ function SuggestionsController(ctx, sqs, env) { site_id: siteId, suggestion_id: suggestionId, opportunity_type: opportunity.getType?.() ?? null, - source: REVIEW_SOURCES.BACKOFFICE, + source, signal, reviewer_id: reviewerId, detail_markdown: cleanMarkdown ?? null, @@ -3061,8 +3075,37 @@ function SuggestionsController(ctx, sqs, env) { return createResponse(toReviewView(data), 201); }; + /** + * Capture an ESE review from the Backoffice. + * POST /sites/:siteId/opportunities/:opportunityId/suggestions/:suggestionId/backoffice-reviews + * Binds source='backoffice'; all rejection categories are allowed. + * @param {Object} context - request context. + * @returns {Promise} + */ + const createBackofficeReview = (context) => captureReview(context, { + source: REVIEW_SOURCES.BACKOFFICE, + allowedRejectionCategories: Object.values(REJECTION_CATEGORIES), + }); + + /** + * Capture a customer review from the ASO UI (SITES-39002). + * POST /sites/:siteId/opportunities/:opportunityId/suggestions/:suggestionId/aso-reviews + * Binds source='aso_ui'; only 'bad_recommendation' and 'other' are accepted as + * rejection categories (no 'product_bug' from customers). + * @param {Object} context - request context. + * @returns {Promise} + */ + const createAsoReview = (context) => captureReview(context, { + source: REVIEW_SOURCES.ASO_UI, + allowedRejectionCategories: [ + REJECTION_CATEGORIES.BAD_RECOMMENDATION, + REJECTION_CATEGORIES.OTHER, + ], + }); + return { createBackofficeReview, + createAsoReview, autofixSuggestions, createSuggestions, deploySuggestionToEdge, diff --git a/src/routes/facs-capabilities.js b/src/routes/facs-capabilities.js index 780299ef30..4fb767f0d1 100644 --- a/src/routes/facs-capabilities.js +++ b/src/routes/facs-capabilities.js @@ -574,6 +574,7 @@ const routeFacsCapabilities = { 'POST /sites/:siteId/opportunities/:opportunityId/fixes': 'llmo/can_configure', 'POST /sites/:siteId/opportunities/:opportunityId/suggestions': 'llmo/can_configure', 'POST /sites/:siteId/opportunities/:opportunityId/suggestions/:suggestionId/backoffice-reviews': 'llmo/can_configure', + 'POST /sites/:siteId/opportunities/:opportunityId/suggestions/:suggestionId/aso-reviews': 'llmo/can_configure', 'POST /sites/:siteId/reports': 'llmo/can_configure', 'POST /sites/:siteId/sandbox/audit': 'llmo/can_configure', 'POST /sites/:siteId/sentiment/guidelines': 'llmo/can_configure', @@ -884,6 +885,7 @@ const routeFacsCapabilities = { 'PATCH /sites/:siteId/opportunities/:opportunityId/status': 'aso/can_edit', 'POST /sites/:siteId/opportunities/:opportunityId/suggestions': 'aso/can_edit', 'POST /sites/:siteId/opportunities/:opportunityId/suggestions/:suggestionId/backoffice-reviews': 'aso/can_edit', + 'POST /sites/:siteId/opportunities/:opportunityId/suggestions/:suggestionId/aso-reviews': 'aso/can_edit', 'PATCH /sites/:siteId/opportunities/:opportunityId/suggestions/status': 'aso/can_edit', 'PATCH /sites/:siteId/opportunities/:opportunityId/suggestions/:suggestionId': 'aso/can_edit', 'DELETE /sites/:siteId/opportunities/:opportunityId/suggestions/:suggestionId': 'aso/can_edit', diff --git a/src/routes/index.js b/src/routes/index.js index 9617ca0088..9b14232674 100644 --- a/src/routes/index.js +++ b/src/routes/index.js @@ -364,6 +364,7 @@ export default function getRouteHandlers( 'GET /sites/:siteId/opportunities/:opportunityId/suggestions/:suggestionId/fixes': suggestionsController.getSuggestionFixes, 'POST /sites/:siteId/opportunities/:opportunityId/suggestions': suggestionsController.createSuggestions, 'POST /sites/:siteId/opportunities/:opportunityId/suggestions/:suggestionId/backoffice-reviews': suggestionsController.createBackofficeReview, + 'POST /sites/:siteId/opportunities/:opportunityId/suggestions/:suggestionId/aso-reviews': suggestionsController.createAsoReview, 'PATCH /sites/:siteId/opportunities/:opportunityId/suggestions/status': suggestionsController.patchSuggestionsStatus, 'PATCH /sites/:siteId/opportunities/:opportunityId/suggestions/:suggestionId': suggestionsController.patchSuggestion, 'DELETE /sites/:siteId/opportunities/:opportunityId/suggestions/:suggestionId': suggestionsController.removeSuggestion, diff --git a/src/routes/required-capabilities.js b/src/routes/required-capabilities.js index fd81e138d1..7a0893598f 100644 --- a/src/routes/required-capabilities.js +++ b/src/routes/required-capabilities.js @@ -504,6 +504,7 @@ const routeRequiredCapabilities = { 'GET /sites/:siteId/opportunities/:opportunityId/suggestions/:suggestionId/fixes': 'fixEntity:read', 'POST /sites/:siteId/opportunities/:opportunityId/suggestions': CAP_SUGGESTION_WRITE, 'POST /sites/:siteId/opportunities/:opportunityId/suggestions/:suggestionId/backoffice-reviews': CAP_SUGGESTION_WRITE, + 'POST /sites/:siteId/opportunities/:opportunityId/suggestions/:suggestionId/aso-reviews': CAP_SUGGESTION_WRITE, 'PATCH /sites/:siteId/opportunities/:opportunityId/suggestions/status': CAP_SUGGESTION_WRITE, 'PATCH /sites/:siteId/opportunities/:opportunityId/suggestions/auto-fix': CAP_FIX_ENTITY_CREATE, 'PATCH /sites/:siteId/opportunities/:opportunityId/suggestions/:suggestionId': CAP_SUGGESTION_WRITE, diff --git a/test/controllers/suggestions-reviews.test.js b/test/controllers/suggestions-reviews.test.js index 7a2ca10a5d..3ad79a4392 100644 --- a/test/controllers/suggestions-reviews.test.js +++ b/test/controllers/suggestions-reviews.test.js @@ -498,4 +498,57 @@ describe('Suggestions Controller - backoffice reviews', () => { expect(json).to.not.have.property('reviews'); }); }); + + describe('ASO reviews (createAsoReview) — SITES-39002', () => { + const okInsert = () => ({ + postgrest: { onInsert: () => ({ data: { event_id: EVENT_ID, signal: 'negative', tier: 'free' }, error: null }) }, + }); + + it('records a review stamped source=aso_ui', async () => { + const context = makeContext(okInsert()); + const response = await controller.createAsoReview(context); + expect(response.status).to.equal(201); + const inserted = context.dataAccess.services.postgrestClient.capturedInserts[0]; + expect(inserted.source).to.equal('aso_ui'); + // shared core still captures the per-issue fields + expect(inserted.previous_fix).to.deep.equal({ patch: 'before' }); + }); + + it('accepts rejectionCategory "other"', async () => { + const context = makeContext(okInsert()); + context.data.rejectionCategory = 'other'; + const response = await controller.createAsoReview(context); + expect(response.status).to.equal(201); + expect(context.dataAccess.services.postgrestClient.capturedInserts[0].rejection_category).to.equal('other'); + }); + + it('rejects rejectionCategory "product_bug" from the ASO route with 400', async () => { + const context = makeContext(); + context.data.rejectionCategory = 'product_bug'; + const response = await controller.createAsoReview(context); + expect(response.status).to.equal(400); + // observability: rejection is logged with source + reason + expect(context.log.warn).to.have.been.calledWithMatch( + /feedback_capture\.rejected source=aso_ui .*reason=invalid_rejection_category/, + ); + }); + + it('rejects a body-supplied source that mismatches aso_ui with 400 (FR-10)', async () => { + const context = makeContext(); + context.data.source = 'backoffice'; + const response = await controller.createAsoReview(context); + expect(response.status).to.equal(400); + // FR-10 abuse case must be observable server-side (source stays aso_ui). + expect(context.log.warn).to.have.been.calledWithMatch( + /feedback_capture\.rejected source=aso_ui .*reason=source_mismatch/, + ); + }); + + it('does not emit a rejection warn on a successful 201', async () => { + const context = makeContext(okInsert()); + const response = await controller.createAsoReview(context); + expect(response.status).to.equal(201); + expect(context.log.warn).to.not.have.been.called; + }); + }); }); diff --git a/test/controllers/suggestions.test.js b/test/controllers/suggestions.test.js index ef77847309..146c5f2662 100644 --- a/test/controllers/suggestions.test.js +++ b/test/controllers/suggestions.test.js @@ -159,6 +159,7 @@ describe('Suggestions Controller', () => { 'autofixSuggestions', 'createSuggestions', 'createBackofficeReview', + 'createAsoReview', 'getAllForOpportunity', 'getAllForOpportunityPaged', 'deploySuggestionToEdge', diff --git a/test/routes/index.test.js b/test/routes/index.test.js index b8381a31f9..c09416149a 100755 --- a/test/routes/index.test.js +++ b/test/routes/index.test.js @@ -1038,6 +1038,7 @@ describe('getRouteHandlers', () => { 'GET /sites/:siteId/opportunities/:opportunityId/suggestions/:suggestionId/fixes', 'POST /sites/:siteId/opportunities/:opportunityId/suggestions', 'POST /sites/:siteId/opportunities/:opportunityId/suggestions/:suggestionId/backoffice-reviews', + 'POST /sites/:siteId/opportunities/:opportunityId/suggestions/:suggestionId/aso-reviews', 'PATCH /sites/:siteId/opportunities/:opportunityId/suggestions/status', 'PATCH /sites/:siteId/opportunities/:opportunityId/suggestions/:suggestionId', 'DELETE /sites/:siteId/opportunities/:opportunityId/suggestions/:suggestionId',