iframe-proxy

adityamisra08 · 2026-09-21T11:23:30Z

What

Tighten removeEdgeOptimizeRouting so rollback strips only the exact per-distribution resources Edge Optimize creates.

Previously it matched EO associations with a loose substring (/edgeoptimize-routing/i, /edgeoptimize-origin/i), which could false-positive on a customer-owned CloudFront Function / Lambda@Edge whose ARN merely contained that text. Now it matches the exact names the automation builds — edgeoptimize-routing-adobe-<distId> (:function/…) and edgeoptimize-origin-adobe-<distId> (:function:…:<version>) — via the existing eoRoutingFunctionName / eoLambdaFunctionName builders (no drift).

Semantics unchanged: all 3 event types (viewer-request + origin-request/response), only behaviors EO changed, idempotent.

Test plan

npm test -w packages/spacecat-shared-tokowaka-client — 1031 passing, 100% coverage.
New test: a customer look-alike ARN (my-edgeoptimize-routing-test, and a different distribution's -adobe-EOTHER) is NOT stripped.
Updated existing fixtures to realistic ARN formats.

Dependency

Targets main and includes the base feat/cloudfront-rollback commit(s), so this single PR carries the full rollback change. Consumed by spacecat-api-service — merge this first.

🤖 Generated with Claude Code

…ront routing Adds removeEdgeOptimizeRouting(credentials, distributionId, region), the reverse of applyAssociations: strips only Edge Optimize's own viewer-request CloudFront Function and origin-request/origin-response Lambda@Edge associations from every behavior on a distribution (default + named), preserving any other association a behavior carries. Idempotent no-op when nothing matches. Does not touch the EO origin, the Lambda function, its execution role, the cache policy, or the connector role — enables a "rollback routing" action in the CloudFront wizard without any resource teardown. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

…g CloudFront routing removeEdgeOptimizeRouting matched EO associations with a loose /edgeoptimize-(routing|origin)/ substring, which could strip a customer-owned function/Lambda whose ARN merely contained that text. Match the exact per-distribution resource names the automation creates (eoRoutingFunctionName/eoLambdaFunctionName) instead. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

- Iterate behaviors directly, using PathPattern ?? 'default' instead of wrapper objects. - Drop the '|| ""' ARN fallbacks (AWS always returns the ARN) and their test. - Inline the changed flags; send one return instead of an early return plus a final return. - Trim the duplicated JSDoc in the implementation and index.d.ts. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

github-actions · 2026-09-26T19:33:57Z

MysticatBot

Hey @adityamisra08,

⚠ Degraded review - no spec document was found for this change (searched the PR links, the touched repos' docs, the architecture/guidelines docs, and linked Jira). This review covers code-level quality but could not validate the change against an agreed design, so confidence is reduced. Add a spec link (PR template section 4) and re-request review for a full-confidence pass.

Verdict: Request changes - the CloudFront Function matcher is still a prefix match, and rollback disagrees with deploy about which associations Edge Optimize owns.
Complexity: MEDIUM - medium diff in a single package, no high-risk surfaces.

Changes: adds removeEdgeOptimizeRouting (free function, CloudFrontEdgeClient method, export and typings) to strip this distribution's Edge Optimize function and Lambda@Edge associations for rollback (4 files).

Must fix before merge

[Important] CloudFront Function match has no end boundary, so it also strips longer names such as edgeoptimize-routing-adobe-<distId>-v2 - packages/spacecat-shared-tokowaka-client/src/cdn/cloudfront/index.js:1197 (details inline)
[Important] Rollback's exact-name ownership rule disagrees with deploy's loose /edgeoptimize-*/i rule, so reverted: false can hide live EO routing - packages/spacecat-shared-tokowaka-client/src/cdn/cloudfront/index.js:1219 (details inline)
[Important] Named-behavior test never checks the UpdateDistribution payload, and no test asserts Quantity - packages/spacecat-shared-tokowaka-client/test/cdn/cloudfront/index.test.js:2177 (details inline)

Non-blocking (4): minor issues and suggestions

nit: a.FunctionARN.includes / a.LambdaFunctionARN.includes skip the || '' guard the neighboring helpers use, so a malformed association throws a bare TypeError mid-rollback - packages/spacecat-shared-tokowaka-client/src/cdn/cloudfront/index.js:1197
suggestion: add tests for EO on several behaviors at once, mixed EO and customer associations on a non-default behavior, Lambda-only partial state, a rerun against the { Quantity: 0, Items: [] } shape this function writes, and a rejected UpdateDistribution (PreconditionFailed) propagating - packages/spacecat-shared-tokowaka-client/test/cdn/cloudfront/index.test.js
nit: the missing-distributionId test uses try/catch; await expect(...).to.be.rejectedWith('distributionId is required') (chai-as-promised is already in use) fails with a clearer message - packages/spacecat-shared-tokowaka-client/test/cdn/cloudfront/index.test.js:2194
suggestion: state in the JSDoc and index.d.ts that this removes associations only; the EO origin, cache-policy changes, the function and the Lambda stay in place, so callers should not present it as full offboarding - packages/spacecat-shared-tokowaka-client/src/index.d.ts:91

Out of scope, worth tracking: findEdgeOptimizeAssociationConflict, applyAssociations and isBehaviorAlreadyAssociated still use the loose /edgeoptimize-routing/i and /edgeoptimize-origin/i regexes, so deploy can treat a customer's look-alike function as EO-owned and overwrite it. Moving deploy onto the same exact-name check as rollback closes that side too.

_{Skill: pr-review | Model: us.anthropic.claude-opus-5-5[1m] | Duration: 0m 47s | Cost: $3.89 | Commit: 7e5d05a0d932ce5f4b7c8266f7b09ac5ccb83802
If this code review was useful, please react with 👍. Otherwise, react with 👎.}

MysticatBot · 2026-09-26T19:40:21Z

+    const existingFns = behavior.FunctionAssociations?.Items || [];
+    const existingLambdas = behavior.LambdaFunctionAssociations?.Items || [];
+    const remainingFns = existingFns.filter(
+      (a) => !(a.EventType === 'viewer-request' && a.FunctionARN.includes(`:function/${fnName}`)),


issue (blocking): This is a prefix match, not the exact match the PR title and the comment above promise.

A CloudFront Function ARN ends at .../function/<name>, with nothing after the name. So includes(:function/${fnName}) for distribution E2EXAMPLE also matches edgeoptimize-routing-adobe-E2EXAMPLE-v2, ...-E2EXAMPLE-custom or ...-E2EXAMPLEX. The Lambda matcher on line 1200 is safe because the trailing : before the version marks where the name ends. This one has no such boundary, and it sits on the customer-visible viewer-request slot.

Why it matters: a customer who forks or renames our function within the edgeoptimize-* namespace and attaches it to this distribution would have their viewer-request logic silently removed on rollback. That is the exact case this PR sets out to prevent.

Fix: (a.FunctionARN || '').endsWith(:function/${fnName}). Ideally, extract isOwnedEoFunctionArn(arn, distId) and isOwnedEoLambdaArn(arn, distId) next to the existing helpers, so the boundary rules live in one tested place. Add a test with ...function/edgeoptimize-routing-adobe-E2EXAMPLE-v2 (and the Lambda equivalent) that expects reverted: false and no UpdateDistribution. The current look-alike test only covers a different prefix and a different distribution ID, not a name that starts with ours.

Fixed in 69368ca. The function match now uses endsWith(:function/${fnName}), so edgeoptimize-routing-adobe-<distId>-v2 (and similar) is left in place. New test "leaves names that only start with ours" covers the function and the Lambda -v2 fork; I confirmed it fails against the old includes match. I kept the matching inline rather than extracting helpers, since it has one caller.

MysticatBot · 2026-09-26T19:40:21Z

+      DistributionConfig: config,
+    }));
+  }
+  return { reverted: changed.length > 0, behaviors: changed };


issue (blocking): { reverted: false, behaviors: [] } looks the same whether the distribution is clean or EO routing is still attached under a name this function did not match.

Deploy decides ownership with the loose isEdgeOptimizeFunctionArn / isEdgeOptimizeLambdaArn regexes (lines 1040-1041), and isBehaviorAlreadyAssociated does the same (around line 1355). Rollback only accepts edgeoptimize-*-adobe-<thisDistId>. So a behavior carrying edgeoptimize-origin-adobe-EOTHER is kept on rollback (the new look-alike test asserts that). The same goes for any distribution wired under the unsuffixed EDGE_OPTIMIZE_FUNCTION_NAME / EDGE_OPTIMIZE_LAMBDA_FUNCTION_NAME names, if any exist. In both cases deploy status reports "associated", while rollback reports nothing to revert.

Why it matters: spacecat-api-service will read reverted: false as a successful no-op while agentic traffic still goes through EO on the customer's CDN. It is also why "reverse of applyAssociations" does not hold for look-alike customer functions: apply overwrites them as EO-owned, then remove does not restore them.

Fix: keep the exact match for what gets removed, but also return the EO-looking associations that were left in place, e.g. skipped: [{ behavior, eventType, arn }], and update index.d.ts. The caller can then surface "EO-like routing remains" instead of a false all-clear. Alternatively, document and test why other-distribution and legacy names are intentionally left alone.

Addressed in 69368ca by documenting and testing the exact-match intent (no skipped list). Deploy has only ever created the per-distribution names (eoRoutingFunctionName / eoLambdaFunctionName, since #1722); the unsuffixed base names are never created. So any EO routing this package deployed matches exactly, and anything else (another distribution's EO names, unsuffixed names, forks) isn't ours to remove. The comment above the matcher explains this, and the new test asserts -v2 forks and unsuffixed names are left alone with no UpdateDistribution. Tightening deploy's loose conflict regexes to the same rule is tracked as out of scope.

MysticatBot · 2026-09-26T19:40:21Z

+      cfSendStub.onSecondCall().resolves({});
+
+      const result = await edgeOptimize.removeEdgeOptimizeRouting({}, 'E2EXAMPLE');
+      expect(result).to.deep.equal({ reverted: true, behaviors: ['/api/*'] });


issue (blocking): This test only asserts the return value. It would still pass if UpdateDistribution were never sent, or were sent with the unmodified config, and it is the only test that exercises the CacheBehaviors.Items path.

No test in the suite asserts Quantity after filtering either; every payload check looks at .Items only. A Quantity/Items mismatch is exactly what CloudFront rejects, so a refactor that drops the Quantity rewrite would pass locally and fail against AWS.

Fix: here, assert cfSendStub.secondCall.args[0].commandName === 'UpdateDistribution' and IfMatch. Also assert that CacheBehaviors.Items[0].FunctionAssociations and LambdaFunctionAssociations deep-equal { Quantity: 0, Items: [] }, and that DefaultCacheBehavior was left untouched. Add Quantity assertions to the default-behavior and customer-preservation tests as well (for example, Quantity: 1 for the kept customer association).

Fixed in 69368ca. The named-behavior test now asserts UpdateDistribution, IfMatch, { Quantity: 0, Items: [] } on both association lists, and that DefaultCacheBehavior is untouched. The default-behavior test asserts Quantity: 0, and the preservation test asserts Quantity: 1 for the kept customer associations.

- Match the routing function ARN with endsWith, so a longer name such as edgeoptimize-routing-adobe-<distId>-v2 is no longer stripped. - Document why rollback removes only this distribution's exact EO names, and cover -v2 forks and the unsuffixed base names with a test. - Assert the UpdateDistribution payload for named behaviors, and assert Quantity alongside Items. - State in the JSDoc and typings that this removes associations only. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

MysticatBot

Hey @adityamisra08,

⚠ Degraded review - no spec document was found for this change (searched the PR links, the touched repos' docs, the architecture/guidelines docs, and linked Jira). This review covers code-level quality but could not validate the change against an agreed design, so confidence is reduced. Add a spec link (PR template section 4) and re-request review for a full-confidence pass.

Verdict: Approve - the prior blockers are fixed or answered, and what remains is non-blocking.
Complexity: MEDIUM - medium diff in one package, no high-risk surfaces.

Changes: adds removeEdgeOptimizeRouting (free function, CloudFrontEdgeClient method, export and typings) that strips only this distribution's exact Edge Optimize function and Lambda@Edge associations during rollback (4 files).

Non-blocking (8): minor issues and suggestions

nit: the new matchers call a.FunctionARN.endsWith(...) and a.LambdaFunctionARN.includes(...) without the || '' guard that the neighboring helpers use. Adding it keeps the file consistent - packages/spacecat-shared-tokowaka-client/src/cdn/cloudfront/index.js:1201
nit: the function matcher uses endsWith but the Lambda matcher uses includes(':function:<name>:'). Both are correct, but one shared anchored helper would make the ownership rule easier to check against the comment above it - packages/spacecat-shared-tokowaka-client/src/cdn/cloudfront/index.js:1204
suggestion: { reverted: false } reads the same for a clean distribution and for one that still carries EO-looking routing this function does not own. That includes a non-standard Lambda wired through a direct call to the public applyAssociations, which accepts any ARN; rollback would then leave the origin Lambdas attached. Have applyAssociations reject a Lambda ARN that is not eoLambdaFunctionName(distributionId), or state in index.d.ts that reverted: false only means "no associations with this distribution's EO names were found" - packages/spacecat-shared-tokowaka-client/src/cdn/cloudfront/index.js:1223
suggestion: a concurrent edit between the Get and the Update surfaces as PreconditionFailed from the IfMatch check, which is correct. One JSDoc line telling callers to retry on that error would help - packages/spacecat-shared-tokowaka-client/src/cdn/cloudfront/index.js:1217
suggestion: give { reverted: boolean; behaviors: string[] } an exported name so consumers can import it - packages/spacecat-shared-tokowaka-client/src/index.d.ts:346
suggestion: add cases for Edge Optimize on the default and a named behavior in one distribution (one UpdateDistribution, both paths in behaviors), mixed EO and customer associations on a named behavior, a Lambda-only partial state, a rerun against { Quantity: 0, Items: [] }, and a rejected UpdateDistribution that propagates - packages/spacecat-shared-tokowaka-client/test/cdn/cloudfront/index.test.js:2038
suggestion: the -v2 forks test only makes negative assertions. Add one exact-name association alongside the forks and assert that only it is removed, which pins the name boundary in a single test - packages/spacecat-shared-tokowaka-client/test/cdn/cloudfront/index.test.js:2153
nit: the missing-distributionId test uses try/catch. await expect(...).to.be.rejectedWith('distributionId') fails with a clearer message - packages/spacecat-shared-tokowaka-client/test/cdn/cloudfront/index.test.js:2240

Out of scope, worth tracking: deploy's conflict and idempotency checks (isEdgeOptimizeFunctionArn / isEdgeOptimizeLambdaArn and isBehaviorAlreadyAssociated) still use the loose edgeoptimize-* regexes. When they are tightened, deploy, status and rollback should share one exact-name predicate. Also check whether the standalone connect-aws-wizard ever installed the unsuffixed names in customer accounts.

Previously flagged, now resolved

The CloudFront Function match now uses endsWith, and a new test covers -v2 forks and the unsuffixed base names.
The named-behavior test now checks the UpdateDistribution payload, IfMatch, Quantity and that the default behavior is unchanged. The other payload tests assert Quantity too.
Rollback and deploy ownership: the pushback holds. Every path that creates a function or Lambda in this package uses eoRoutingFunctionName / eoLambdaFunctionName, so an exact-name rollback matches everything the package deploys. The decision is now documented and tested.

_{Skill: pr-review | Model: us.anthropic.claude-opus-5-5[1m] | Duration: 0m 32s | Cost: $3.79 | Commit: 69368ca8af4217f0cc661adab728b4c36acbb6ba
If this code review was useful, please react with 👍. Otherwise, react with 👎.}

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

…timizeRouting Returns per-behavior removed associations so callers can preview a rollback; dryRun skips UpdateDistribution. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

MysticatBot

Hey @adityamisra08,

⚠ Degraded review - no spec document was found for this change (searched the PR links, the touched repos' docs, the architecture/guidelines docs, and linked Jira). This review covers code-level quality but could not validate the change against an agreed design, so confidence is reduced. Add a spec link (PR template section 4) and re-request review for a full-confidence pass.

Verdict: Approve - the new dryRun option and the per-behavior removed details are correct and tested, and the exact-name matching is unchanged.
Complexity: MEDIUM - medium diff in one package, no high-risk surfaces.

Changes: adds removeEdgeOptimizeRouting (free function, CloudFrontEdgeClient method, export and typings) that strips only this distribution's exact Edge Optimize function and Lambda@Edge associations, now with a dryRun option and the removed associations listed per behavior (4 files).

Non-blocking (6): minor issues and suggestions

suggestion: dryRun comes after the positional region. Free-function callers must write removeEdgeOptimizeRouting(creds, id, undefined, { dryRun: true }). A plain-JS caller that passes { dryRun: true } as the third argument puts it in region, so dryRun falls back to false. The signature has no callers yet, so moving region into the options object now is cheap (updateCacheSettings already does this). Otherwise, reject a region that is not a string - packages/spacecat-shared-tokowaka-client/src/cdn/cloudfront/index.js:1180
suggestion: a dry-run result (reverted: false with non-empty behaviors) looks like a skipped update once it is stored or logged. Return a dryRun flag, or state that meaning in the JSDoc and index.d.ts - packages/spacecat-shared-tokowaka-client/src/cdn/cloudfront/index.js:1228
nit: the JSDoc says type: string, but index.d.ts declares 'function' | 'lambda'. Make them match - packages/spacecat-shared-tokowaka-client/src/cdn/cloudfront/index.js:1173
nit: eventType: string could be 'viewer-request' | 'origin-request' | 'origin-response', the only values the matchers produce - packages/spacecat-shared-tokowaka-client/src/index.d.ts:351
nit: { dryRun = false } = {} only applies the default for undefined. Passing null for options throws a destructuring TypeError. const { dryRun = false } = options ?? {} avoids it - packages/spacecat-shared-tokowaka-client/src/cdn/cloudfront/index.js:1180
nit: on a dry run the loop still rewrites the association lists on the fetched config. That is local and invisible today. Moving the reassignment under the !dryRun path, or adding a comment, protects a future refactor that returns or reuses config - packages/spacecat-shared-tokowaka-client/src/cdn/cloudfront/index.js:1209

Open design questions (1): not blocking

Should a dry run return the ETag it read, and should the apply call accept an optional ifMatch? That would let a preview-then-confirm UI guarantee that the confirmed removal matches the preview - packages/spacecat-shared-tokowaka-client/src/cdn/cloudfront/index.js:1220

_{Skill: pr-review | Model: us.anthropic.claude-opus-5-5[1m] | Duration: 0m 32s | Cost: $3.32 | Commit: f597dce4d5fddd1c56bd5bc37a52a95bfcd763b3
If this code review was useful, please react with 👍. Otherwise, react with 👎.}

Aditya Misra and others added 2 commits September 14, 2026 15:26

adityamisra08 requested review from MysticatBot and removed request for MysticatBot September 21, 2026 11:25

adityamisra08 changed the base branch from feat/cloudfront-rollback to main September 26, 2026 19:24

adityamisra08 requested a review from MysticatBot September 26, 2026 19:34

MysticatBot suggested changes Sep 26, 2026

View reviewed changes

MysticatBot added ai-reviewed Reviewed by AI complexity:medium AI-assessed PR complexity: MEDIUM labels Sep 26, 2026

adityamisra08 requested a review from MysticatBot September 27, 2026 09:49

MysticatBot approved these changes Sep 27, 2026

View reviewed changes

adityamisra08 and others added 2 commits September 27, 2026 15:37

chore(tokowaka-client): trim rollback name-match comment

ee5f278

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

feat(tokowaka-client): add dryRun and removed details to removeEdgeOp…

f597dce

…timizeRouting Returns per-behavior removed associations so callers can preview a rollback; dryRun skips UpdateDistribution. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

adityamisra08 requested a review from MysticatBot October 2, 2026 11:58

MysticatBot approved these changes Oct 2, 2026

View reviewed changes

Sunbelt Computer Software

PL/B Language Development and Support

Navigation Menu

Repository navigation

fix(tokowaka-client): match exact -adobe-<distId> names when reverting CloudFront routing - #1939

fix(tokowaka-client): match exact -adobe-<distId> names when reverting CloudFront routing#1939
adityamisra08 wants to merge 6 commits into
mainfrom
feat/cloudfront-rollback-card-button

adityamisra08 commented Sep 21, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Sep 26, 2026

Uh oh!

MysticatBot left a comment

Uh oh!

MysticatBot Sep 26, 2026

Uh oh!

adityamisra08 Sep 27, 2026

Uh oh!

MysticatBot Sep 26, 2026

Uh oh!

adityamisra08 Sep 27, 2026

Uh oh!

MysticatBot Sep 26, 2026

Uh oh!

adityamisra08 Sep 27, 2026

Uh oh!

MysticatBot left a comment

Uh oh!

MysticatBot left a comment

Uh oh!

This branch has not been deployed

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Sunbelt Computer Software

PL/B Language Development and Support

Conversation

adityamisra08 commented Sep 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What

Test plan

Dependency

Uh oh!

github-actions Bot commented Sep 26, 2026

Uh oh!

MysticatBot left a comment

Choose a reason for hiding this comment

Must fix before merge

Uh oh!

MysticatBot Sep 26, 2026

Choose a reason for hiding this comment

Uh oh!

adityamisra08 Sep 27, 2026

Choose a reason for hiding this comment

Uh oh!

MysticatBot Sep 26, 2026

Choose a reason for hiding this comment

Uh oh!

adityamisra08 Sep 27, 2026

Choose a reason for hiding this comment

Uh oh!

MysticatBot Sep 26, 2026

Choose a reason for hiding this comment

Uh oh!

adityamisra08 Sep 27, 2026

Choose a reason for hiding this comment

Uh oh!

MysticatBot left a comment

Choose a reason for hiding this comment

Previously flagged, now resolved

Uh oh!

MysticatBot left a comment

Choose a reason for hiding this comment

Uh oh!

This branch has not been deployed

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

adityamisra08 commented Sep 21, 2026 •

edited

Loading