fix: prerender validation endpoints

[jindaliiita]

Please ensure your pull request adheres to the following guidelines:

• make sure to link the related issues in this description. Or if there's no issue created, make sure you
  describe here the problem you're solving.
• when merging / squashing, make sure the fixed issue references are visible in the commits, for easy compilation of release notes

If the PR is changing the API specification:

• make sure you add a "Not implemented yet" note the endpoint description, if the implementation is not ready
  yet. Ideally, return a 501 status code with a message explaining the feature is not implemented yet.
• make sure you add at least one example of the request and response.

If the PR is changing the API implementation or an entity exposed through the API:

• make sure you update the API specification and the examples to reflect the changes.

If the PR is introducing a new audit type:

• make sure you update the API specification with the type, schema of the audit result and an example

Related Issues

Thanks for contributing!

[github-actions]

This PR will trigger a minor release when merged.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

[MysticatBot]

Hey @jindaliiita,

Verdict: Request changes - one material gap to close before merge.
Complexity: HIGH - large diff (package-lock metadata); API surface + FACS signal.
Changes: Adds a dedicated PATCH endpoint for updating prerender validation status on opportunities via server-side merge, avoiding read-modify-write on the generic PATCH (8 files).
Note: Recommend a human read before merge - possible divergence from the documented design (heuristic, flagged by the conventions reviewer).

Must fix before merge

1. [Important] No type/format validation on startedAt/completedAt allows storing arbitrary values - src/controllers/opportunities.js:410 (details inline)

Non-blocking (2): minor issues and suggestions

• suggestion: Add OpenAPI spec entry for the new endpoint per the repo's "OpenAPI First" / "Specification Sync" must-follow patterns in CLAUDE.md - src/routes/index.js:334
• suggestion: Add a test case verifying that omitting completedAt from the body preserves an existing completedAt value (the key partial-update semantic) - test/controllers/opportunities.test.js

---

_{Skill: pr-review | Model: us.anthropic.claude-opus-4-6-v1[1m] | Duration: 3m 31s | Cost: $7.98 | Commit: a519066ba7ce172832ff2c03d6acd1941cab5208
If this code review was useful, please react with 👍. Otherwise, react with 👎.}

[MysticatBot]

Hey @jindaliiita,

Verdict: Request changes - two documented must-follow conventions remain unmet.
Complexity: HIGH - large diff (package-lock metadata); API surface + FACS signal.
Changes: Adds timestamp validation and a reason field to the prerender-validation lifecycle endpoint, addressing the prior review's blocking finding (8 files).
Note: Recommend a human read before merge - possible divergence from the documented design (heuristic, flagged by the conventions reviewer).

Previously flagged, now resolved

• Timestamp validation for startedAt/completedAt now enforced via isValidTimestamp guard
• Test coverage for partial-update semantics added (merge behavior, reason clearing)

Must fix before merge

1. [Important] Missing OpenAPI specification - CLAUDE.md "Must-Follow Patterns" requires "OpenAPI First: Define API contract in docs/openapi/ before implementation" and "Specification Sync: Keep OpenAPI specs and implementation in sync". The new PATCH .../prerender-validation endpoint has no entry in docs/openapi/. The design spec in docs/specs/ is valuable context but does not satisfy this requirement.
2. [Important] Missing integration tests - CLAUDE.md states "New or modified endpoints must include integration tests in test/it/". The endpoint has thorough unit tests but no integration test wiring (seed IDs, seed data, shared test factory, wiring file per the Adding a New Endpoint checklist step 10).

Non-blocking (4): minor issues and suggestions

• suggestion: Add type validation for reason (accept only string or null) to prevent storing non-string values that could break downstream rendering - src/controllers/opportunities.js:410
• suggestion: Tighten timestamp validation with an ISO 8601 regex before Date.parse - Date.parse accepts values like "Tuesday" or "1" which are not ISO 8601 - src/controllers/opportunities.js:405
• nit: Design spec should document the reason field added in this commit - docs/specs/2026-07-03-prerender-validation-status-endpoint.md
• nit: Add a completedAt-with-number-type test for symmetry with the existing startedAt number test - test/controllers/opportunities.test.js

---

_{Skill: pr-review | Model: us.anthropic.claude-opus-4-6-v1[1m] | Duration: 1m 31s | Cost: $4.14 | Commit: 871a8774e797741d843688a9482f56a371899619
If this code review was useful, please react with 👍. Otherwise, react with 👎.}

[MysticatBot]

Hey @jindaliiita,

Verdict: Approve - all prior blocking findings addressed; remaining items are non-blocking hardening suggestions.
Complexity: HIGH - large diff (package-lock metadata); API surface + FACS signal.
Changes: Adds OpenAPI spec, tightens timestamp validation with ISO 8601 regex, validates reason type, and adds integration tests for the prerender-validation endpoint (8 files).
Note: Recommend a human read before merge - this change adds a new shared contract (OpenAPI spec in docs/openapi/). The bot review is a complement to, not a replacement for, a human read here.

Previously flagged, now resolved

• Timestamp validation for startedAt/completedAt now enforced via ISO 8601 regex + Date.parse
• OpenAPI specification added for the new endpoint in docs/openapi/
• Integration tests added covering happy path, validation errors, access control (403), and not-found (404)
• reason field type validation added (string or null)
• Design spec updated to document the reason field and its always-reset semantics

Non-blocking (3): minor issues and suggestions

• suggestion: Add a length cap on reason (e.g., 1024 chars) to prevent a buggy S2S caller from persisting multi-MB strings - src/controllers/opportunities.js:410
• suggestion: Add a unit test documenting that 4+ fractional-second digits are intentionally rejected (e.g., 2026-07-12T11:31:12.1234Z returns 400), so the 3-digit cap is a tested contract rather than an implicit side effect - test/controllers/opportunities.test.js
• nit: Integration test suite does not exercise the error status enum value; one additional test case would close the gap - test/it/shared/tests/opportunities.js

---

_{Skill: pr-review | Model: us.anthropic.claude-opus-4-6-v1[1m] | Duration: 1m 1s | Cost: $6.84 | Commit: 8345f23c17fe115cb5cd84ec6fac67787cba2881
If this code review was useful, please react with 👍. Otherwise, react with 👎.}

[MysticatBot]

Hey @jindaliiita,

Verdict: Request changes - input validation gap and stale documentation need fixing before merge.
Complexity: HIGH - large diff (package-lock metadata); API surface + FACS signal.
Changes: Adds a POST endpoint for triggering prerender-validation comparison runs via an internal service proxy, alongside the previously-approved PATCH lifecycle endpoint (13 files).
Note: Recommend a human read before merge - this change adds a new shared contract (OpenAPI spec in docs/openapi/). The bot review is a complement to, not a replacement for, a human read here.

Previously flagged, now resolved

• Timestamp validation for startedAt/completedAt enforced via ISO 8601 regex + Date.parse
• OpenAPI specification added for both endpoints
• Integration tests added for PATCH endpoint
• reason field type validation added (string or null)
• Auth forwarding removed in favor of IP-based allowlisting (commit bced77d)

Must fix before merge

1. [Important] Stale OpenAPI description claims caller auth is forwarded to internal service - code no longer does this (IP allowlist since bced77d) - docs/openapi/site-opportunities.yaml:~248 (see below)
2. [Important] customUrls from user input forwarded to internal service without type, length, or scheme validation - src/controllers/opportunities.js in runPrerenderValidation (see below)
3. [Important] Error message in 502 response leaks internal hostname via e.message - src/controllers/opportunities.js catch block (see below)

Details

1. Stale OpenAPI description (docs/openapi/site-opportunities.yaml)

The POST .../prerender-validation/run description says: "using the caller's own Authorization header - this endpoint does not hold its own credential for the internal service." This was true before commit bced77d removed auth forwarding. The code now explicitly does NOT forward any Authorization header (IP-based source allowlisting instead). Consumers reading the API docs will assume their token reaches the internal service, which is both incorrect and a security concern from their perspective.

Fix: Update the description to: "Authentication with the internal service is handled via IP-based source allowlisting - the caller's Authorization header is NOT forwarded."

2. customUrls unvalidated (src/controllers/opportunities.js)

The customUrls field from context.data is spread directly into the upstream request body with zero runtime validation. The OpenAPI schema declares type: array, items: { type: string, format: uri } but no runtime enforcement exists. A caller can pass non-array values, thousands of entries, or non-URL strings (including file://, data: URIs) that get forwarded to the internal comparison service.

Fix: Add validation before forwarding:

if (customUrls !== undefined) {
  if (!Array.isArray(customUrls) || customUrls.some((u) => typeof u !== 'string')) {
    return badRequest('customUrls must be an array of strings');
  }
  if (customUrls.length > 100) {
    return badRequest('customUrls cannot exceed 100 entries');
  }
}

3. Error message leaks internal hostname (src/controllers/opportunities.js)

The catch block returns message: e.message in the 502 response. A failed fetch() message typically contains the target URL (e.g., "request to https://sj1010010249075.corp.adobe.com/... failed, reason: ENOTFOUND..."), leaking the internal corporate hostname to the authenticated external caller.

Fix: Return a generic message. The server-side log.error already captures the full error:

return createResponse(
  { error: 'prerenderValidationServiceUnreachable', message: 'Internal prerender service is temporarily unavailable' },
  502,
);

Non-blocking (4): minor issues and suggestions

• suggestion: For non-2xx upstream responses, whitelist known fields (error, reason, requestId) before passing through to caller to prevent leaking internal debug info - src/controllers/opportunities.js
• nit: FACS comment says "internal tokowaka-only" but any S2S consumer with opportunity:write can call these endpoints; update comment to reflect this - src/routes/facs-capabilities.js:259
• suggestion: Document the completed_success terminal-state behavior (callers must PATCH to reset before re-triggering a run) in the OpenAPI description or design spec - docs/openapi/site-opportunities.yaml
• suggestion: The JSDoc on runPrerenderValidation also mentions "caller's own Authorization header is forwarded as-is" - update to match the code - src/controllers/opportunities.js

---

_{Skill: pr-review | Model: us.anthropic.claude-opus-4-6-v1[1m] | Duration: 3m 25s | Cost: $9.09 | Commit: 8225dce733d6444abda83ba7d90a77ea965994f6
If this code review was useful, please react with 👍. Otherwise, react with 👎.}

[nit23uec]

we should not mark the status as enabled if prerender is not working. pls move this to status marking async job instead of an HTTP API.
also, I would like to know the deployment details here. is there a proposal floated for this change?