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

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions docs/openapi/api.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down
94 changes: 93 additions & 1 deletion docs/openapi/site-opportunities.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -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':
Expand Down
103 changes: 73 additions & 30 deletions src/controllers/suggestions.js
Original file line number Diff line number Diff line change
Expand Up @@ -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<Response>}
*/
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 : {};
Expand All @@ -2925,69 +2939,69 @@ 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).
// Larger cap than detail_markdown (64 KB) because issue descriptions +
// 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.
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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<Response>}
*/
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<Response>}
*/
const createAsoReview = (context) => captureReview(context, {
source: REVIEW_SOURCES.ASO_UI,
allowedRejectionCategories: [
REJECTION_CATEGORIES.BAD_RECOMMENDATION,
REJECTION_CATEGORIES.OTHER,
],
});

return {
createBackofficeReview,
createAsoReview,
autofixSuggestions,
createSuggestions,
deploySuggestionToEdge,
Expand Down
2 changes: 2 additions & 0 deletions src/routes/facs-capabilities.js
Original file line number Diff line number Diff line change
Expand Up @@ -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',
Expand Down Expand Up @@ -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',
Expand Down
1 change: 1 addition & 0 deletions src/routes/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
1 change: 1 addition & 0 deletions src/routes/required-capabilities.js
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
53 changes: 53 additions & 0 deletions test/controllers/suggestions-reviews.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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;
});
});
});
Loading
Loading