fix(llmo): echo detectedCdn from the site-only onboarding endpoint - #2976
fix(llmo): echo detectedCdn from the site-only onboarding endpoint#2976andreeastroe96 wants to merge 2 commits into
Conversation
onboardSiteOnly already runs CDN detection via performLlmoOnboarding (same orchestrator the legacy /llmo/onboard handler uses) and persists it to the site's LLMO config, but never included it in its own response -- unlike the legacy handler one line above it. This silently dropped CDN auto-select pre-fill for every client using the site-only endpoint (project-elmo-ui's self-serve domain-onboarding wizard step). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
GitHub Actions
Inactive
|
This PR will trigger a patch release when merged. |
GitHub Actions
Inactive
There was a problem hiding this comment.
Hey @andreeastroe96,
Verdict: Request changes - test fixture uses a CDN value that contradicts the OpenAPI enum documented in this same PR.
Complexity: MEDIUM - small diff; API surface signal.
Changes: Adds the missing detectedCdn field to the site-only LLMO onboarding endpoint response and documents it in the OpenAPI spec (3 files).
Must fix before merge
- [Important] Test uses non-canonical CDN value
'cloudflare'which is not in theCDN_TYPESvocabulary -test/controllers/llmo/onboard-site-only.test.js:183(details inline)
Non-blocking (1): minor issues and suggestions
- suggestion: OpenAPI nullable enum should include
nullin the enum list for strict JSON Schema 2020-12 compliance (without it, strict validators will reject{"detectedCdn": null}) -docs/openapi/schemas.yaml:4676
Skill: pr-review | Model: us.anthropic.claude-opus-4-6-v1[1m] | Duration: 3m 24s | Cost: $4.35 | Commit: fa95b872bf757c49430b5f6750eb774000e97a74
If this code review was useful, please react with 👍. Otherwise, react with 👎.
| }); | ||
|
|
||
| it('echoes a non-null detectedCdn from performLlmoOnboarding (LLMO client CDN pre-fill)', async () => { | ||
| performLlmoOnboardingStub.resolves({ |
There was a problem hiding this comment.
issue (blocking): The test uses 'cloudflare' as the detectedCdn value, but the canonical CDN_TYPES vocabulary (defined in src/controllers/llmo/llmo-utils.js) and the OpenAPI enum you are documenting in this same PR list only prefixed compound values (byocdn-cloudflare, aem-cs-fastly, etc.). 'cloudflare' is not among them and would never be returned by performLlmoOnboarding in production.
The pass-through logic is still exercised correctly, but the fixture is misleading: a reader will assume 'cloudflare' is a real production value, and it contradicts the contract formalized one file away in this same commit.
Fix: Replace 'cloudflare' with a real enum member, e.g. 'byocdn-cloudflare'.
MysticatBot
added
ai-reviewed
2 participants
Summary
onboardSiteOnly(POST /v2/orgs/:orgId/llmo/onboard-site) already runs CDN detection viaperformLlmoOnboarding(the same orchestrator the legacy/llmo/onboardhandler uses) and persists it to the site's LLMO config, but never includeddetectedCdnin its own response — unlike the legacy handler one line above it.LlmoOnboardSiteResponsein the OpenAPI spec, reusing the existing CDN-provider enum already defined for the sibling/llmo/onboardendpoint.Test plan
test/controllers/llmo/onboard-site-only.test.js) which previously asserted the exact response body and would have failed oncedetectedCdnappeareddetectedCdnis echoed throughnpm test)redocly lintvalidates the updated OpenAPI spec🤖 Generated with Claude Code