feat: MSM/language copy relationship context validation for suggestions

[sagarmiglani]

Summary

• fixTargetMode is now required in suggestion fix requests (was optional)
• fixTargetPageId is required only when fixTargetMode === 'local' (was always required)
• Updated OpenAPI docs to reflect alt-text support alongside meta-tags
• Companion PRs: spacecat-autofix-worker, experience-success-studio-ui

Changed files

File	Change
src/controllers/suggestions.js	Updated validation rules for fixTargetMode and fixTargetPageId
docs/openapi/site-opportunities.yaml	API docs updated for alt-text + meta-tags support
test/controllers/suggestions.test.js	6 new tests, 4 updated for new validation rules

Test plan

• 6 new tests for updated validation rules
• 4 existing tests updated
• All tests passing, lint clean

🤖 Generated with Claude Code

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

[github-actions]

This PR will trigger a minor release when merged.

[solaris007]

Hey @sagarmiglani,

Strengths

• Validation now correctly gates on the discriminator (fixTargetMode) first, then enforces mode-specific requirements - the right order (src/controllers/suggestions.js:1020-1030).
• The "provided but empty" check at suggestions.js:1028-1030 correctly distinguishes "absent" from "empty string", preventing fixTargetPageId: '' from bypassing the local-mode requirement.
• New tests cover meaningful state transitions: missing mode, local-without-id, source-without-id (now valid), source-with-id (backward compat), and the empty-string edge case. The two 207-vs-400 split tests for source mode pin the SQS payload contract.
• OpenAPI spec updated in lockstep with code, including removing fixTargetPageId from the source-mode example at site-opportunities.yaml:1027. Schema-side required: [fixTargetMode] matches the runtime check.
• x-experimental: true is being used correctly to absorb this contract evolution.

Issues

Important (Should Fix)

1. Invalid fixTargetMode values produce a misleading "required" error.
src/controllers/suggestions.js:1020 - The combined check !fixTargetMode || (fixTargetMode !== 'source' && fixTargetMode !== 'local') merges two distinct error cases: "field is missing" and "field has an invalid value". Values like 'SOURCE', 'remote', null, 0, or true all trigger fixTargetMode is required: "source" or "local" even though the field was provided. The pre-PR code had separate messages for these cases (visible in the deleted block). This is a diagnostic regression that will confuse API consumers debugging a bad value.

Fix: split back into two checks:

if (fixTargetMode === undefined || fixTargetMode === null) {
  return badRequest('...fixTargetMode is required');
}
if (fixTargetMode !== 'source' && fixTargetMode !== 'local') {
  return badRequest('...fixTargetMode must be "source" or "local"');
}

Add a test for an invalid string value (e.g. 'SOURCE') to pin the distinction.

2. relationshipContext forwarded without matching the OpenAPI scope claim.
OpenAPI now states this is "effective only on the AEM CS Content API path for meta-tags and alt-text" (site-opportunities.yaml:982). The controller, however, gates via shouldGroupSuggestionsForAutofix() which uses a deny-list (AUTOFIX_UNGROUPED_OPPTY_TYPES). Any opportunity type not in that deny-list will accept and forward relationshipContext in the SQS payload - including types that have no relationship-aware handler. The spec and code should agree: either add an explicit allow-list (['meta-tags', 'alt-text']) on the controller side, or broaden the OpenAPI language to "any supported opportunity type." The allow-list approach is safer - it's a one-line guard that prevents silent regressions when new opportunity types are added.

Minor (Nice to Have)

3. OpenAPI cannot express the conditional fixTargetPageId requirement.
Schema says fixTargetMode is required and fixTargetPageId is optional, but runtime requires fixTargetPageId when mode is local. Code generators will produce client stubs that don't enforce this. Consider a oneOf with discriminator on fixTargetMode when this contract stabilizes beyond x-experimental.

4. No length limits on fixTargetPageId and appliedOnPagePath.
Both are user-controlled strings forwarded into SQS. Risk is low (endpoint requires site-level auto_fix access, SQS has its own 256KB cap), but capping each at e.g. 2048 chars is cheap defense-in-depth.

Recommendations

• Enforce or soften appliedOnPagePath for source mode. OpenAPI says "should be provided" when fixTargetMode is source, but no validation enforces this. Either require it when mode === 'source' (if the worker depends on it) or change the prose to "may be provided." Add a test that pins whichever contract you choose.
• Document the breaking change. Existing clients sending fixTargetPageId without fixTargetMode will now 400. Semantic-release will tag this as minor; a BREAKING CHANGE: footer would correctly bump to major if any consumer beyond the companion UI exists.
• Confirm worker-side auth for source-page resolution. This PR makes it easier to target a blueprint/source page from a language-copy context. The site-level auto_fix access check is pre-existing, but confirm with the autofix-worker team that source-page resolution in fixTargetMode: 'source' performs its own authorization check on the resolved page.
• Plan the oneOf/discriminator migration before a third opportunity type enrolls in relationship-aware autofix. The current shape works for source/local, but conditional requirements expressed as prose will be painful to extend.

Assessment

Ready to merge? With fixes.

The validation logic is correct and well-tested with 100% coverage. The two Important findings are small, in-scope fixes: splitting the error check restores pre-PR diagnostic quality, and the allow-list aligns the spec with the implementation. Everything else is follow-up work.

Next Steps

1. Split the fixTargetMode validation into missing-vs-invalid checks (Important finding 1).
2. Add an opportunity-type allow-list for relationshipContext (Important finding 2).
3. Recommendations are optional but worth tracking.

[solaris007]

Important: Diagnostic regression on invalid values. The combined check !fixTargetMode || (fixTargetMode !== 'source' && fixTargetMode !== 'local') merges two distinct error cases: "field is missing" and "field has an invalid value." Values like 'SOURCE', 'remote', null, 0, or true all trigger "fixTargetMode is required" even though the field was provided. The pre-PR code had separate messages.

Suggested fix - split into two checks:

if (fixTargetMode === undefined || fixTargetMode === null) {
  return badRequest('Each fixTargetGroup relationshipContext.fixTargetMode is required');
}
if (fixTargetMode !== 'source' && fixTargetMode !== 'local') {
  return badRequest('Each fixTargetGroup relationshipContext.fixTargetMode must be "source" or "local"');
}

Add a test for an invalid string value (e.g. 'SOURCE') to pin the distinction.