Skip to content

feat(disruption): cross-account executor + CDK construct — ADR-031 Phase B [#1419] - #1646

Merged
susumutomita merged 1 commit into
mainfrom
feat/1419-executor-complete
Jun 2, 2026
Merged

susumutomita merged 1 commit into
mainfrom
feat/1419-executor-complete

Conversation

@susumutomita

@susumutomita susumutomita commented Jun 2, 2026 •

Copy link
Copy Markdown
Owner

Summary

The complete cross-account disruption executor (ADR-031 Phase B, #1419), consolidated into one reviewable PR on top of the merged design (#1638) + schema/parser/validator (#1639). This supersedes the earlier stacked PRs #1640–#1645 + #1644 (which got tangled when #1640's base branch was deleted during merge); the code + tests are identical, just rebased clean onto main.

Once an operator fires a disruption whose problem declares an action, this executor AssumeRoles into the competitor account and injects the real fault, then auto-reverts (ADR-029 INV-2).

What's here (all previously reviewed across #1640–#1645/#1644)

  • disruption-executor-handler/ — pure, DI'd logic: dispatch builders (dispatch-command), orchestration (execute), DDB deps (executor-store: claimExecution EXEC# idempotency + resolveDeployment GSI1 query), SDK mapping (send-dispatch: SSM/Lambda/CFn), aws-scheduler one-shot revert (schedule-revert), the inject-vs-revert router, and the real-deps index.ts entry.
  • shared/assume-competitor-role.ts — the SOLID extraction of the cross-account AssumeRole + security: harden cross-account AssumeRole — ExternalId race + Trust Policy enforcement #856/[tech-debt] Review describe-stack ExternalId grace_fallback band-aid #1245 rotation-race retry (describe-stack repointed to it; its 20 tests unchanged = regression proof).
  • disruption-executor-lambda.ts — the CDK construct: Lambda + EventBridge rule (tenkacloud.disruptions) + scheduler execution role. Least-privilege IAM (Template-asserted): sts:AssumeRole only on TenkaCloud-*, scoped SSM/KMS/DDB/scheduler — the destructive SendCommand/Invoke/UpdateStack are NOT on the executor's own role (they ride the consented CompetitorDeployRole).
  • Wired into ProblemDeployBackendStack (EventBridge rule count 7→8); adds @aws-sdk/client-scheduler (audit-clean, no lifecycle script).

Test plan

  • ~63 unit/Template tests across the executor + construct + shared auth (dispatch 11, execute 6, store 9, send 5, schedule 4, route 7, construct 4, auth 9, + describe-stack's 20 unchanged).
  • Full infra suite: 2442 pass. tsc, biome, make harness (0 errors), check-no-conflicts, cdk synth exit 0.

Regression analysis

Additive: action-less disruptions stay Phase A audit-only (executeDisruptionAction returns no_action). The only existing-resource delta is the new EventBridge rule + construct in ProblemDeployBackendStack (rule count test updated) and the additive @aws-sdk/client-scheduler dep. describe-stack's behavior is byte-identical (shared-auth refactor verified by its unchanged 20 tests). Synth confirms the rest of the template is unchanged.

Physical impact

CFn: CREATE in ProblemDeployBackendStack — one Lambda (executor), one EventBridge Rule, two IAM Roles (least-privilege executor role + scheduler execution role). No table/capacity change. NO-OP on other stacks. The fault-injection path is inert until a problem declares an action (submodule TenkaCloudChallenge#33) and an operator fires it — and only after you make deploy.

Relates #1419

Summary by CodeRabbit

  • New Features

    • Added disruption execution infrastructure supporting cross-account disruption triggering and automatic revert scheduling via EventBridge integration.
  • Tests

    • Added comprehensive test coverage for disruption execution, dispatch handling, deployment resolution, and revert scheduling logic.

@coderabbitai

coderabbitai Bot commented Jun 2, 2026 •

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

This PR implements a complete cross-account disruption executor Lambda that receives EventBridge notifications of chaos injections, claims idempotent execution per team, resolves deployment targets, dispatches disruption commands (SSM, Lambda, CloudFormation) via assumed credentials, and schedules revert jobs using EventBridge Scheduler.

Changes

Cross-Account Disruption Executor Lambda

Layer / File(s) Summary
Shared cross-account role assumption
infrastructure/lib/problem-deploy/handlers/shared/assume-competitor-role.ts, infrastructure/lib/problem-deploy/handlers/describe-stack-handler/index.ts, infrastructure/test/problem-deploy/assume-competitor-role.test.ts
Extracts and refactors cross-account AssumeRole logic with ExternalId support and grace-fallback retry into a shared module. Updates describe-stack-handler to use the extracted logic with configurable session name prefix and trace event emission.
Infrastructure construct and permissions
infrastructure/lib/problem-deploy/disruption-executor-lambda.ts, infrastructure/lib/problem-deploy/problem-deploy-backend-stack.ts, infrastructure/test/problem-deploy/disruption-executor-lambda.test.ts
Defines DisruptionExecutorLambda CDK construct that creates the executor Lambda (nodejs22.x, arm64), a scheduler-assumable revert role, EventBridge rule matching tenkacloud.disruptions events, and scoped IAM policies for SSM parameter read, DynamoDB table operations, STS assume-role to TenkaCloud-* roles, and scheduler revert creation.
Dispatch command normalization and execution orchestration
infrastructure/lib/problem-deploy/handlers/disruption-executor-handler/dispatch-command.ts, infrastructure/lib/problem-deploy/handlers/disruption-executor-handler/execute.ts, infrastructure/test/problem-deploy/disruption-dispatch-command.test.ts, infrastructure/test/problem-deploy/disruption-execute.test.ts
Implements pure dispatch normalization that resolves target references from stack outputs and substitutes {{placeholder}} parameters from fired events. Adds orchestrator that claims per-team idempotency, resolves deployment targets, and sequences injection dispatch followed by revert scheduling.
Persistence and dispatch execution
infrastructure/lib/problem-deploy/handlers/disruption-executor-handler/executor-store.ts, infrastructure/lib/problem-deploy/handlers/disruption-executor-handler/send-dispatch.ts, infrastructure/test/problem-deploy/disruption-executor-store.test.ts, infrastructure/test/problem-deploy/disruption-send-dispatch.test.ts
Implements DynamoDB-backed idempotency claim using conditional PutCommand, deployment query via GSI1 with COMPLETE status filter, and dispatch execution routing through SSM run-command, Lambda async invocation, or CloudFormation stack update with parameter coercion and credential injection.
Revert scheduling and event routing
infrastructure/lib/problem-deploy/handlers/disruption-executor-handler/schedule-revert.ts, infrastructure/lib/problem-deploy/handlers/disruption-executor-handler/route.ts, infrastructure/test/problem-deploy/disruption-schedule-revert.test.ts, infrastructure/test/problem-deploy/disruption-route.test.ts
Implements one-shot EventBridge Scheduler integration with idempotent naming and auto-delete-after-completion configuration. Adds pure routing logic that distinguishes EventBridge injections from scheduler revert invocations and validates event payloads.
Handler entry point and backend stack integration
infrastructure/lib/problem-deploy/handlers/disruption-executor-handler/index.ts, .claude/harness/baselines/handler-no-direct-sdk-import.json, infrastructure/package.json, infrastructure/test/problem-deploy-backend-stack-events.test.ts
Wires the Lambda handler entry point by instantiating AWS SDK clients (DynamoDB, SSM, STS, Scheduler), binding orchestration dependencies with per-dispatch assumed credentials, and routing events through the dispatcher. Updates backend stack to instantiate executor and integrates @aws-sdk/client-scheduler dependency.

Sequence Diagram

sequenceDiagram
  participant EventBridge
  participant Handler as Handler (index.ts)
  participant Router as Route
  participant Executor as Execute
  participant Store as Store
  participant Dispatcher as SendDispatch
  participant Scheduler as Scheduler
  
  EventBridge->>Handler: disruptionFired event
  Handler->>Router: routeDisruptionInvocation(event)
  Router->>Executor: executeDisruptionAction(detail, deps)
  Executor->>Store: claimExecution(requestId, teamId)
  Store-->>Executor: claimed
  Executor->>Store: resolveDeployment(eventId, teamId, problemId)
  Store-->>Executor: DeploymentTarget
  Executor->>Dispatcher: sendDispatch(injectionCommand, target)
  Dispatcher-->>Executor: sent
  Executor->>Scheduler: scheduleRevert(revertCommand, afterSeconds)
  Scheduler-->>Executor: scheduled
  Executor-->>Router: { kind: ok, jobId }
  Router-->>Handler: RouteOutcome
  Handler-->>EventBridge: logged
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related PRs

  • susumutomita/TenkaCloud#782: New assume-competitor-role shared module emits grace-fallback trace events directly built on the retrieved PR's trace-log utilities and deploy-trace event taxonomy.
  • susumutomita/TenkaCloud#1004: Updates .claude/harness/baselines/handler-no-direct-sdk-import.json to baseline-match the new disruption-executor handler's AWS SDK imports, which is directly tied to the retrieved PR's harness rule implementation.
  • susumutomita/TenkaCloud#889: Adds the Disruptions DynamoDB table and wires problemsDisruptions into the backend; this PR adds the executor that consumes both via ProblemDeployBackendStack.

🐰 Chaos orchestrated with care,
Cross-account roles, precise and fair,
Disruptions claimed, then fixed with flair,
Each revert scheduled, scheduled with prayer. ✨

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 52.50% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main change: implementing a cross-account disruption executor and CDK construct (Phase B of ADR-031), directly matching the substantial architectural additions in the changeset.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/1419-executor-complete

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@codecov

codecov Bot commented Jun 2, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 98.71795% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 93.38%. Comparing base (6cd8cd0) to head (a3c92b3).

Files with missing lines Patch % Lines
...e/lib/problem-deploy/disruption-executor-lambda.ts 93.75% 0 Missing and 1 partial ⚠️
...m-deploy/handlers/shared/assume-competitor-role.ts 97.05% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1646      +/-   ##
==========================================
+ Coverage   93.32%   93.38%   +0.06%     
==========================================
  Files         392      400       +8     
  Lines       10749    10871     +122     
  Branches     3303     3337      +34     
==========================================
+ Hits        10031    10152     +121     
  Misses        222      222              
- Partials      496      497       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Nitpick comments (2)
infrastructure/test/problem-deploy/disruption-execute.test.ts (1)

128-134: ⚡ Quick win

Add coverage for revert-scheduling failure after a successful inject.

The suite tests sendDispatch rejection but not the inverse: sendDispatch resolving and scheduleRevert rejecting. That path is where a fault can be injected without a scheduled recovery (see the INV-2 concern in execute.ts). A test pinning the intended behavior here would guard the recovery guarantee.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@infrastructure/test/problem-deploy/disruption-execute.test.ts` around lines
128 - 134, Add a new test case that verifies behavior when sendDispatch resolves
but scheduleRevert rejects: use makeDeps to create deps where sendDispatch is a
resolved mock and scheduleRevert is mocked to reject (e.g.,
mockRejectedValue(new Error("ScheduleRevert failed"))); call
executeDisruptionAction(detail, deps) and assert the function either throws the
scheduleRevert error (or rejects with the expected behavior per INV-2) and that
sendDispatch was called while ensuring scheduleRevert was attempted; reference
the existing test patterns around executeDisruptionAction, makeDeps,
sendDispatch, scheduleRevert and the test fixture detail to mirror style and
assertions.
infrastructure/lib/problem-deploy/handlers/disruption-executor-handler/index.ts (1)

39-47: ⚡ Quick win

Consider failing fast on missing required env config.

deploymentsTableName, disruptionsTableName, schedulerRoleArn, and revertTargetArn fall back to "". When unset, downstream AWS calls fail with opaque validation errors rather than a clear "missing env" signal. A small assert at module init would surface misconfiguration immediately.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@infrastructure/lib/problem-deploy/handlers/disruption-executor-handler/index.ts`
around lines 39 - 47, The module currently defaults critical env vars to empty
strings (see resources.deploymentsTableName, resources.disruptionsTableName,
schedulerRoleArn, revertTargetArn and the parseDisruptionsCatalogEnv use) which
hides misconfiguration; add a small validation at module init that checks these
four values (and the input to parseDisruptionsCatalogEnv) and throws a
descriptive Error listing any missing env keys so the process fails fast with a
clear message rather than allowing downstream AWS calls to error obscurely.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In
`@infrastructure/lib/problem-deploy/handlers/disruption-executor-handler/execute.ts`:
- Around line 108-122: The handler finalizes the idempotency claim via
deps.claimExecution before performing the disruptive sendDispatch, so if
sendDispatch succeeds but deps.scheduleRevert later fails the injection becomes
permanent; move durable revert reservation earlier (call buildRevertDispatch and
await deps.scheduleRevert(detail, revert, target, action.revert.afterSeconds))
before calling deps.sendDispatch(inject, target), or alternatively change claim
semantics so claimExecution does not return "duplicate" until scheduleRevert has
completed; update the code paths around claimExecution, buildRevertDispatch,
deps.scheduleRevert and deps.sendDispatch to ensure scheduleRevert is durably
reserved before the disruptive sendDispatch is executed.

In
`@infrastructure/lib/problem-deploy/handlers/disruption-executor-handler/route.ts`:
- Around line 44-74: Replace the hand-rolled guards in
parseDisruptionFiredDetail and isRevertInvocation with Zod schemas: define a
DisruptionFiredDetail zod schema that requires
disruptionId,eventId,problemId,tenantId,teamId,requestId as non-empty strings
and firedAt as z.string().datetime(...) (use offset if you expect offsets), then
use schema.safeParse on event.detail inside parseDisruptionFiredDetail and
return undefined on failure; likewise define a RevertInvocation zod schema that
enforces mode === "revert" and validates the exact shapes of dispatch and target
(their required fields) and use it to implement isRevertInvocation (or a
parseRevertInvocation function) so malformed scheduler invocations are rejected
before schedule-revert.ts uses revertAtExpression/Date parsing.

---

Nitpick comments:
In
`@infrastructure/lib/problem-deploy/handlers/disruption-executor-handler/index.ts`:
- Around line 39-47: The module currently defaults critical env vars to empty
strings (see resources.deploymentsTableName, resources.disruptionsTableName,
schedulerRoleArn, revertTargetArn and the parseDisruptionsCatalogEnv use) which
hides misconfiguration; add a small validation at module init that checks these
four values (and the input to parseDisruptionsCatalogEnv) and throws a
descriptive Error listing any missing env keys so the process fails fast with a
clear message rather than allowing downstream AWS calls to error obscurely.

In `@infrastructure/test/problem-deploy/disruption-execute.test.ts`:
- Around line 128-134: Add a new test case that verifies behavior when
sendDispatch resolves but scheduleRevert rejects: use makeDeps to create deps
where sendDispatch is a resolved mock and scheduleRevert is mocked to reject
(e.g., mockRejectedValue(new Error("ScheduleRevert failed"))); call
executeDisruptionAction(detail, deps) and assert the function either throws the
scheduleRevert error (or rejects with the expected behavior per INV-2) and that
sendDispatch was called while ensuring scheduleRevert was attempted; reference
the existing test patterns around executeDisruptionAction, makeDeps,
sendDispatch, scheduleRevert and the test fixture detail to mirror style and
assertions.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: de6018ed-ce0d-4edc-b1ab-2e11147a7f0c

📥 Commits

Reviewing files that changed from the base of the PR and between 6cd8cd0 and a3c92b3.

⛔ Files ignored due to path filters (1)
  • bun.lock is excluded by !**/*.lock
📒 Files selected for processing (22)
  • .claude/harness/baselines/handler-no-direct-sdk-import.json
  • infrastructure/lib/problem-deploy/disruption-executor-lambda.ts
  • infrastructure/lib/problem-deploy/handlers/describe-stack-handler/index.ts
  • infrastructure/lib/problem-deploy/handlers/disruption-executor-handler/dispatch-command.ts
  • infrastructure/lib/problem-deploy/handlers/disruption-executor-handler/execute.ts
  • infrastructure/lib/problem-deploy/handlers/disruption-executor-handler/executor-store.ts
  • infrastructure/lib/problem-deploy/handlers/disruption-executor-handler/index.ts
  • infrastructure/lib/problem-deploy/handlers/disruption-executor-handler/route.ts
  • infrastructure/lib/problem-deploy/handlers/disruption-executor-handler/schedule-revert.ts
  • infrastructure/lib/problem-deploy/handlers/disruption-executor-handler/send-dispatch.ts
  • infrastructure/lib/problem-deploy/handlers/shared/assume-competitor-role.ts
  • infrastructure/lib/problem-deploy/problem-deploy-backend-stack.ts
  • infrastructure/package.json
  • infrastructure/test/problem-deploy-backend-stack-events.test.ts
  • infrastructure/test/problem-deploy/assume-competitor-role.test.ts
  • infrastructure/test/problem-deploy/disruption-dispatch-command.test.ts
  • infrastructure/test/problem-deploy/disruption-execute.test.ts
  • infrastructure/test/problem-deploy/disruption-executor-lambda.test.ts
  • infrastructure/test/problem-deploy/disruption-executor-store.test.ts
  • infrastructure/test/problem-deploy/disruption-route.test.ts
  • infrastructure/test/problem-deploy/disruption-schedule-revert.test.ts
  • infrastructure/test/problem-deploy/disruption-send-dispatch.test.ts

Comment on lines +108 to +122
// EventBridge at-least-once の再配送を per-team 冪等で弾く。 claim は注入の前に取る。
if ((await deps.claimExecution(detail)) === "duplicate") return { kind: "duplicate" };

const target = await deps.resolveDeployment(detail);
if (!target) return { kind: "no_deployment" };

const inject = buildDisruptionDispatch(action, detail.parameters, target.stackOutputs);
await deps.sendDispatch(inject, target);

// ADR-029 INV-2: 注入したら必ず復旧を予約する (revert は schema 必須)。
const revert = buildRevertDispatch(action, detail.parameters, target.stackOutputs);
await deps.scheduleRevert(detail, revert, target, action.revert.afterSeconds);

return { kind: "ok", jobId: target.jobId };
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift

Failure between inject and revert scheduling leaves a permanent fault (ADR-029 INV-2 gap).

The idempotency claim is finalized at Line 109, before the inject (Line 115) and the revert reservation (Line 119). If sendDispatch succeeds but scheduleRevert then throws, the function rejects and the Lambda fails. On EventBridge redelivery, claimExecution returns "duplicate" and the handler returns early at Line 109 — so the revert is never scheduled and the injected fault persists indefinitely, contradicting the INV-2 guarantee documented in this file's header ("必ず復旧する。 永続障害の禁止").

Consider reserving recovery before the destructive action (schedule the revert prior to sendDispatch, given the schedule name is already idempotent), or otherwise ensuring the claim does not short-circuit a retry until the revert has been durably reserved.

♻️ One option: reserve the revert before injecting
-  const inject = buildDisruptionDispatch(action, detail.parameters, target.stackOutputs);
-  await deps.sendDispatch(inject, target);
-
-  // ADR-029 INV-2: 注入したら必ず復旧を予約する (revert は schema 必須)。
-  const revert = buildRevertDispatch(action, detail.parameters, target.stackOutputs);
-  await deps.scheduleRevert(detail, revert, target, action.revert.afterSeconds);
+  const inject = buildDisruptionDispatch(action, detail.parameters, target.stackOutputs);
+  const revert = buildRevertDispatch(action, detail.parameters, target.stackOutputs);
+
+  // ADR-029 INV-2: 復旧を先に予約してから注入する (= 注入後に予約失敗 → 永続障害、を防ぐ)。
+  await deps.scheduleRevert(detail, revert, target, action.revert.afterSeconds);
+  await deps.sendDispatch(inject, target);

Note this reorders the existing happy-path test expectations in disruption-execute.test.ts; verify the chosen approach against your idempotency/retry semantics before adopting.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
// EventBridge at-least-once の再配送を per-team 冪等で弾く。 claim は注入の前に取る。
if ((await deps.claimExecution(detail)) === "duplicate") return { kind: "duplicate" };
const target = await deps.resolveDeployment(detail);
if (!target) return { kind: "no_deployment" };
const inject = buildDisruptionDispatch(action, detail.parameters, target.stackOutputs);
await deps.sendDispatch(inject, target);
// ADR-029 INV-2: 注入したら必ず復旧を予約する (revert は schema 必須)。
const revert = buildRevertDispatch(action, detail.parameters, target.stackOutputs);
await deps.scheduleRevert(detail, revert, target, action.revert.afterSeconds);
return { kind: "ok", jobId: target.jobId };
}
// EventBridge at-least-once の再配送を per-team 冪等で弾く。 claim は注入の前に取る。
if ((await deps.claimExecution(detail)) === "duplicate") return { kind: "duplicate" };
const target = await deps.resolveDeployment(detail);
if (!target) return { kind: "no_deployment" };
const inject = buildDisruptionDispatch(action, detail.parameters, target.stackOutputs);
const revert = buildRevertDispatch(action, detail.parameters, target.stackOutputs);
// ADR-029 INV-2: 復旧を先に予約してから注入する (= 注入後に予約失敗 → 永続障害、を防ぐ)。
await deps.scheduleRevert(detail, revert, target, action.revert.afterSeconds);
await deps.sendDispatch(inject, target);
return { kind: "ok", jobId: target.jobId };
}
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@infrastructure/lib/problem-deploy/handlers/disruption-executor-handler/execute.ts`
around lines 108 - 122, The handler finalizes the idempotency claim via
deps.claimExecution before performing the disruptive sendDispatch, so if
sendDispatch succeeds but deps.scheduleRevert later fails the injection becomes
permanent; move durable revert reservation earlier (call buildRevertDispatch and
await deps.scheduleRevert(detail, revert, target, action.revert.afterSeconds))
before calling deps.sendDispatch(inject, target), or alternatively change claim
semantics so claimExecution does not return "duplicate" until scheduleRevert has
completed; update the code paths around claimExecution, buildRevertDispatch,
deps.scheduleRevert and deps.sendDispatch to ensure scheduleRevert is durably
reserved before the disruptive sendDispatch is executed.

Comment on lines +44 to +74
/** EventBridge envelope の `detail` を DisruptionFiredDetail へ narrow。 必須 string 欠落は undefined。 */
export function parseDisruptionFiredDetail(event: unknown): DisruptionFiredDetail | undefined {
if (!isObject(event) || !isObject(event.detail)) return undefined;
const d = event.detail;
const disruptionId = asNonEmptyString(d.disruptionId);
const eventId = asNonEmptyString(d.eventId);
const problemId = asNonEmptyString(d.problemId);
const tenantId = asNonEmptyString(d.tenantId);
const teamId = asNonEmptyString(d.teamId);
const requestId = asNonEmptyString(d.requestId);
const firedAt = asNonEmptyString(d.firedAt);
if (!disruptionId || !eventId || !problemId || !tenantId || !teamId || !requestId || !firedAt) {
return undefined;
}
return {
disruptionId,
eventId,
problemId,
tenantId,
teamId,
requestId,
firedAt,
parameters: isObject(d.parameters) ? d.parameters : {},
};
}

function isRevertInvocation(event: unknown): event is RevertInvocation {
return (
isObject(event) && event.mode === "revert" && isObject(event.dispatch) && isObject(event.target)
);
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🌐 Web query:

What is the Zod syntax to validate an ISO 8601 datetime string in a string schema?

💡 Result:

To validate an ISO 8601 datetime string in Zod, use the.datetime method on a string schema [1]. Basic usage: const schema = z.string.datetime; schema.parse("2023-01-01T12:00:00Z"); // Valid [1] Configuration Options: You can pass an optional configuration object to.datetime to customize validation requirements [2][1]: 1. Allow Timezone Offsets: By default,.datetime only accepts UTC strings ending in "Z". To allow timezone offsets, set { offset: true } [2][1]. const schema = z.string.datetime({ offset: true }); schema.parse("2023-01-01T12:00:00+02:00"); // Valid 2. Allow Local (Timezone-less) Datetimes: Set { local: true } to permit datetimes without a timezone component [2][3]. const schema = z.string.datetime({ local: true }); schema.parse("2023-01-01T12:00:00"); // Valid 3. Precision Constraints: Use the precision parameter to enforce the number of digits following the decimal point (fractional seconds) [2][1]. const schema = z.string.datetime({ precision: 3 }); schema.parse("2023-01-01T12:00:00.123Z"); // Valid Important Considerations: - Validation Approach: The.datetime method uses regex for validation rather than the JavaScript Date constructor [4][5]. This means it checks the format strictly but may not catch logically invalid dates (e.g., February 30th) in all environments [4]. - Conversion to Date Object: If you need the result as a native JavaScript Date object rather than a string, you can chain the.pipe method [4][6][7]: const dateSchema = z.string.datetime.pipe(z.coerce.date);

Citations:


🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Locate the file (in case path differs slightly) and print the relevant sections
fd -a "route.ts" infrastructure/lib/problem-deploy/handlers/disruption-executor-handler -t f || true

FILE="infrastructure/lib/problem-deploy/handlers/disruption-executor-handler/route.ts"
if [ -f "$FILE" ]; then
  echo "=== $FILE (lines 1-140) ==="
  sed -n '1,140p' "$FILE" | cat -n
else
  echo "File not found: $FILE"
fi

# Find where parseDisruptionFiredDetail is used
echo "=== Usages of parseDisruptionFiredDetail ==="
rg -n "parseDisruptionFiredDetail" infrastructure/lib/problem-deploy/handlers/disruption-executor-handler -S || true
rg -n "parseDisruptionFiredDetail" infrastructure/lib -S || true

# Find revertAtExpression (or any Date/toISOString usage) in this handler subtree
echo "=== revertAtExpression occurrences ==="
rg -n "revertAtExpression" infrastructure/lib/problem-deploy/handlers -S || true

echo "=== Date(.*firedAt / toISOString on firedAt occurrences ==="
rg -n "new Date\\(.*firedAt|toISOString\\(\\)|firedAt" infrastructure/lib/problem-deploy/handlers -S || true

Repository: susumutomita/TenkaCloud

Length of output: 14686


🏁 Script executed:

#!/bin/bash
set -euo pipefail

FILE1="infrastructure/lib/problem-deploy/handlers/disruption-executor-handler/schedule-revert.ts"
FILE2="infrastructure/lib/problem-deploy/handlers/disruption-executor-handler/execute.ts"
FILE3="infrastructure/lib/problem-deploy/handlers/disruption-executor-handler/executor-store.ts"
FILE4="infrastructure/lib/problem-deploy/handlers/disruption-executor-handler/route.ts"

for f in "$FILE1" "$FILE2" "$FILE3" "$FILE4"; do
  echo "=== $f ==="
  if [ -f "$f" ]; then
    sed -n '1,220p' "$f" | cat -n
  else
    echo "Missing $f"
  fi
done

# Also inspect any schema/Zod usage around disruption fired details
echo "=== Zod-related search in this handler subtree ==="
rg -n "zod|from \"zod\"|z\\.|z\\.object|safeParse" infrastructure/lib/problem-deploy/handlers/disruption-executor-handler -S || true

Repository: susumutomita/TenkaCloud

Length of output: 18238


🌐 Web query:

Zod validate ISO 8601 datetime string syntax z.string().datetime({ offset: true }) or z.string().datetime()

💡 Result:

In Zod, z.string.datetime is used to validate ISO 8601 formatted datetime strings [1][2]. Because these validations are regular-expression-based, they are designed for convenient input validation rather than strict adherence to full date/time library standards [1][2]. The choice between the two syntaxes depends on whether you need to support timezone offsets: - z.string.datetime: By default, this method enforces ISO 8601 without allowing timezone offsets (i.e., it typically expects UTC strings ending in 'Z') [1][2][3]. It supports arbitrary sub-second decimal precision [1][2][3]. - z.string.datetime({ offset: true }): Using the offset option allows the validation to accept ISO 8601 strings that include timezone offsets (e.g., +02:00) [1][2][3]. Additional configurations are also available, such as { local: true } to allow timezone-less datetimes [1][2] and the precision option to constrain the number of decimal digits for seconds [2][3]. It is important to note that these methods only validate the string format via regex; they do not perform semantic validation of the date itself (e.g., they will not reject an invalid month like '15') [4]. If you require strict date validation or need to work with Date objects, you should pipe the result into z.coerce.date or use a dedicated date library [4][5].

Citations:


Use Zod schemas for the EventBridge/scheduler boundary (including firedAt) instead of hand-rolled guards.

  • In infrastructure/lib/problem-deploy/handlers/disruption-executor-handler/route.ts, parseDisruptionFiredDetail only requires firedAt to be a non-empty string; infrastructure/lib/problem-deploy/handlers/disruption-executor-handler/schedule-revert.ts (revertAtExpression) builds a Date from it and calls toISOString(), which throws RangeError: Invalid time value for non-datetime strings—so malformed input won’t be rejected as { kind: "invalid_event" }.
  • In the same file, isRevertInvocation doesn’t validate dispatch/target beyond isObject shape checks; define Zod schemas for the revert payload too so malformed scheduler invocations are rejected early.

Use something like: const firedAt = z.string().datetime(); (or z.string().datetime({ offset: true }) if offsets are expected).

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@infrastructure/lib/problem-deploy/handlers/disruption-executor-handler/route.ts`
around lines 44 - 74, Replace the hand-rolled guards in
parseDisruptionFiredDetail and isRevertInvocation with Zod schemas: define a
DisruptionFiredDetail zod schema that requires
disruptionId,eventId,problemId,tenantId,teamId,requestId as non-empty strings
and firedAt as z.string().datetime(...) (use offset if you expect offsets), then
use schema.safeParse on event.detail inside parseDisruptionFiredDetail and
return undefined on failure; likewise define a RevertInvocation zod schema that
enforces mode === "revert" and validates the exact shapes of dispatch and target
(their required fields) and use it to implement isRevertInvocation (or a
parseRevertInvocation function) so malformed scheduler invocations are rejected
before schedule-revert.ts uses revertAtExpression/Date parsing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant