feat(aws/cloudformation): expose StackSet instances, drift, and operations #57

Merged

stack72 merged 3 commits from worktree-618 into main 2026-06-16 21:19:32 +00:00

Conversation 6 Commits 3 Files changed 21 +1931 -98

stack72 commented 2026-06-16 20:12:46 +00:00

Owner

Copy link

Summary

• Adds four native-CloudFormation-SDK methods (listInstances, listOperations, describeOperation, detectDrift) to the @swamp/aws/cloudformation/stack-set model via the enrichment system's new customMethods capability
• Extends AwsEnrichment with an optional customMethods field — standalone methods that receive explicit (args, credentials) and are generated as new method blocks alongside existing CRUD
• Extracts shared source-file parsing to codegen/shared/sourceParser.ts and reorganises enrichments into a folder-per-resource structure

Cloud Control API structurally cannot surface StackSet instance status, drift detection, or operation history. These methods use @aws-sdk/client-cloudformation directly, with explicit credential pass-through from globalArgs.

Closes #618

Test plan

• deno test aws/enrichments/parser_test.ts — 10 enrichment parser tests pass (shared parser non-regression)
• deno test aws/extensionModelGenerator_test.ts — 13 generator tests pass including new with modelMethods snapshot
• deno test aws/enrichments/cfn-stackset/methods_test.ts — 6 methods source tests pass (mock HTTP, pagination, drift polling, timeout)
• deno task generate:aws rds — zero diff (RDS enrichment non-regression)
• deno task generate:aws cloudformation twice — second run zero diff (idempotency)
• deno check, deno lint, deno fmt --check pass for codegen and model/aws/cloudformation

🤖 Generated with Claude Code

## Summary - Adds four native-CloudFormation-SDK methods (`listInstances`, `listOperations`, `describeOperation`, `detectDrift`) to the `@swamp/aws/cloudformation/stack-set` model via the enrichment system's new `customMethods` capability - Extends `AwsEnrichment` with an optional `customMethods` field — standalone methods that receive explicit `(args, credentials)` and are generated as new method blocks alongside existing CRUD - Extracts shared source-file parsing to `codegen/shared/sourceParser.ts` and reorganises enrichments into a folder-per-resource structure Cloud Control API structurally cannot surface StackSet instance status, drift detection, or operation history. These methods use `@aws-sdk/client-cloudformation` directly, with explicit credential pass-through from `globalArgs`. Closes #618 ## Test plan - [ ] `deno test aws/enrichments/parser_test.ts` — 10 enrichment parser tests pass (shared parser non-regression) - [ ] `deno test aws/extensionModelGenerator_test.ts` — 13 generator tests pass including new `with modelMethods` snapshot - [ ] `deno test aws/enrichments/cfn-stackset/methods_test.ts` — 6 methods source tests pass (mock HTTP, pagination, drift polling, timeout) - [ ] `deno task generate:aws rds` — zero diff (RDS enrichment non-regression) - [ ] `deno task generate:aws cloudformation` twice — second run zero diff (idempotency) - [ ] `deno check`, `deno lint`, `deno fmt --check` pass for codegen and model/aws/cloudformation 🤖 Generated with [Claude Code](https://claude.com/claude-code)

stack72 added 1 commit 2026-06-16 20:12:47 +00:00

feat(aws/cloudformation): add StackSet instances, drift, and operations via custom methods (#618) ... e23e4bbf04

Cloud Control API only exposes CRUD on the AWS::CloudFormation::StackSet
resource. This adds four native-SDK-backed methods (listInstances,
listOperations, describeOperation, detectDrift) via the enrichment
system's new customMethods capability.

Changes:
- Extend AwsEnrichment type with optional customMethods field
- Add custom methods code generation to extensionModelGenerator
- Extract shared source parser to codegen/shared/sourceParser.ts
- Reorganise enrichments into folder-per-resource structure
- Add CloudFormation StackSet enrichment with four methods
- Regenerate @swamp/aws/cloudformation with new methods on stack-set

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

stack72 commented 2026-06-16 20:21:43 +00:00

Author

Owner

Copy link

Code Review

Blocking Issues

1. Logic bug: listInstances falls back to credentials.region as StackSet name (codegen/aws/enrichments/cfn-stackset/methods.ts, line 38)

   const stackSetName = (args as any).StackSetName as string ??
       credentials.region;
   

   When args.StackSetName is undefined, the function silently uses the region string (e.g. "us-east-1") as the StackSet name. This would produce a confusing StackSetNotFoundException from AWS instead of a clear missing-argument error. All other methods in the same file (listOperations, describeOperation, detectDrift) correctly do not have this fallback. This looks like a copy-paste artifact that was never cleaned up.

2. any types in hand-written code (codegen/aws/enrichments/cfn-stackset/methods.ts, lines 38, 54, 94, 153, 224) — violates the CLAUDE.md rule "no any types in hand-written code".

   The (args as any).StackSetName pattern is unnecessary: args is typed as Record<string, unknown>, so args.StackSetName as string is valid TypeScript without an any intermediary. Similarly the Filters: ... as | any | undefined cast on line 54 is avoidable. The file suppresses the lint rule with // deno-lint-ignore-file no-explicit-any rather than fixing the casts.

Suggestions

1. console.log statements in detectDrift will appear in production (codegen/aws/enrichments/cfn-stackset/methods.ts, lines 240 and 258). These are inlined verbatim into the generated stack_set.ts model, so every user running drift detection will see [DRIFT] Started drift detection… and [DRIFT] Operation status: … in their logs. Consider removing them or replacing with a callback/hook if progress visibility is needed.

2. Design doc describes a layout that doesn't exist (codegen/designs/aws.md, section 8b). The doc says model methods live in codegen/aws/methods/ — "a separate system from enrichments with its own types, registry, and generator path" and that "the enrichment system is not modified." In reality the implementation adds customMethods directly to the existing AwsEnrichment interface and places everything under codegen/aws/enrichments/cfn-stackset/. The doc should be updated to reflect the actual directory structure and architecture so future contributors aren't sent to a path that doesn't exist.

3. Unnecessary no-import-prefix lint suppression (codegen/aws/enrichments/cfn-stackset/methods.ts, line 1). The file adds no-import-prefix to the lint ignore directive, but all imports already use the npm: prefix correctly. This suppress is a no-op and can be removed to keep the ignore minimal.

## Code Review ### Blocking Issues 1. **Logic bug: `listInstances` falls back to `credentials.region` as StackSet name** (`codegen/aws/enrichments/cfn-stackset/methods.ts`, line 38) ```typescript const stackSetName = (args as any).StackSetName as string ?? credentials.region; ``` When `args.StackSetName` is `undefined`, the function silently uses the region string (e.g. `"us-east-1"`) as the StackSet name. This would produce a confusing `StackSetNotFoundException` from AWS instead of a clear missing-argument error. All other methods in the same file (`listOperations`, `describeOperation`, `detectDrift`) correctly do not have this fallback. This looks like a copy-paste artifact that was never cleaned up. 2. **`any` types in hand-written code** (`codegen/aws/enrichments/cfn-stackset/methods.ts`, lines 38, 54, 94, 153, 224) — violates the CLAUDE.md rule "no `any` types in hand-written code". The `(args as any).StackSetName` pattern is unnecessary: `args` is typed as `Record<string, unknown>`, so `args.StackSetName as string` is valid TypeScript without an `any` intermediary. Similarly the `Filters: ... as | any | undefined` cast on line 54 is avoidable. The file suppresses the lint rule with `// deno-lint-ignore-file no-explicit-any` rather than fixing the casts. ### Suggestions 1. **`console.log` statements in `detectDrift` will appear in production** (`codegen/aws/enrichments/cfn-stackset/methods.ts`, lines 240 and 258). These are inlined verbatim into the generated `stack_set.ts` model, so every user running drift detection will see `[DRIFT] Started drift detection…` and `[DRIFT] Operation status: …` in their logs. Consider removing them or replacing with a callback/hook if progress visibility is needed. 2. **Design doc describes a layout that doesn't exist** (`codegen/designs/aws.md`, section 8b). The doc says model methods live in `codegen/aws/methods/` — "a separate system from enrichments with its own types, registry, and generator path" and that "the enrichment system is not modified." In reality the implementation adds `customMethods` directly to the existing `AwsEnrichment` interface and places everything under `codegen/aws/enrichments/cfn-stackset/`. The doc should be updated to reflect the actual directory structure and architecture so future contributors aren't sent to a path that doesn't exist. 3. **Unnecessary `no-import-prefix` lint suppression** (`codegen/aws/enrichments/cfn-stackset/methods.ts`, line 1). The file adds `no-import-prefix` to the lint ignore directive, but all imports already use the `npm:` prefix correctly. This suppress is a no-op and can be removed to keep the ignore minimal.

stack72 commented 2026-06-16 20:21:54 +00:00

Author

Owner

Copy link

Adversarial Review

Medium

1. codegen/aws/enrichments/cfn-stackset/methods.ts:38-39 — listInstances silently falls back to credentials.region when StackSetName is missing

   const stackSetName = (args as any).StackSetName as string ??
       credentials.region;
   

   If StackSetName is ever absent from args, this falls back to using the AWS region (e.g. "us-east-1") as the StackSet name. This would produce a confusing CloudFormation StackSetNotFoundException blaming a region string as a StackSet name. The other three functions (listOperations:94, describeOperation:152, detectDrift:224) do NOT have this fallback — they cast directly to string without a ??. This is almost certainly a copy-paste remnant.

   Breaking input: Call listInstances with args that omit StackSetName (e.g. if globalArgs validation is bypassed or the field is undefined at runtime).

   Suggested fix: Remove the ?? credentials.region fallback and either throw an explicit error if StackSetName is missing, or rely on the Zod schema enforcement alone (consistent with the other three methods):

   const stackSetName = (args as any).StackSetName as string;
   

2. codegen/aws/extensionModelGenerator.ts:195-208 — import deduplication drops entire import lines, potentially losing named imports

   The import dedup logic for model methods skips an entire import line if its package string matches a package already imported by enrichment or listMethod:

   const pkg = imp.match(/"([^"]+)"/)?.[1];
   if (pkg && existingImportPackages.has(pkg)) continue; // entire line skipped
   

   If a resource has both an enrichment and customMethods that import different named symbols from the same npm package (e.g. enrichment imports { ListStacksCommand } and methods import { DetectStackSetDriftCommand } from the same @aws-sdk/client-cloudformation), the model methods' import line is silently dropped. The generated code would reference symbols that are never imported, producing a compile error.

   Currently not triggered because cfn-stackset has no enrichment sourceFile, so the dedup set is empty. This becomes a real bug the first time someone adds both enrichment and customMethods for a resource sharing an npm package.

   Suggested fix: Merge named imports from the same package rather than dropping the entire later import line. Alternatively, document that enrichment and customMethods must not share a package, with a build-time assertion.

3. codegen/aws/enrichments/cfn-stackset/methods.ts:226-227 — detectDrift has no bounds on poll interval or timeout

   const pollIntervalMs = (args.pollIntervalMs as number | undefined) ?? 5000;
   const timeoutMs = (args.timeoutMs as number | undefined) ?? 300000;
   

   The Zod schema validates these are numbers but imposes no minimum. A caller passing pollIntervalMs: 0 (or a negative value) gets setTimeout(resolve, 0) — a tight loop that hammers the CloudFormation DescribeStackSetOperation API as fast as the network allows. With default timeoutMs: 300000, that's up to 5 minutes of uncapped API calls, likely hitting rate limits.

   Breaking input: { StackSetName: "x", pollIntervalMs: 0, timeoutMs: 300000 }

   Suggested fix: Clamp pollIntervalMs to a floor (e.g. Math.max(pollIntervalMs, 1000)) and timeoutMs to a ceiling. Or add .min() constraints in the Zod schema in config.ts.

4. codegen/designs/aws.md (added section 8b) — design doc describes a file structure that doesn't match the implementation

   The design doc says model methods live in codegen/aws/methods/ as a separate system from enrichments:

   Model methods live in codegen/aws/methods/ — a separate system from enrichments with its own types, registry, and generator path. The enrichment system is not modified.

   In reality, model methods are implemented inside the enrichment system:

   • Types are in codegen/aws/enrichments/types.ts (AwsEnrichmentCustomMethod, customMethods on AwsEnrichment)
   • Config is in codegen/aws/enrichments/cfn-stackset/config.ts
   • Registry is getEnrichment() in codegen/aws/enrichments/index.ts

   Every file path and "registry" name in the design doc is wrong. A developer following section 8b's "Adding a new model methods config" instructions would create files in directories that don't exist.

Low

1. codegen/aws/enrichments/cfn-stackset/methods.ts:240-241, 258 — debug console.log statements in production code

   detectDrift logs the StackSet name and operation status to stdout on every poll iteration. Not a security issue (no secrets), but noisy in production and inconsistent with the other methods which don't log.

Verdict

PASS — No critical or high severity issues. The medium findings are real but either latent (import dedup) or bounded in impact (region-as-stackset-name produces a clear AWS error rather than silent data corruption; polling floor is a robustness concern). The design doc mismatch should be fixed before it misleads contributors.

## Adversarial Review ### Medium 1. **`codegen/aws/enrichments/cfn-stackset/methods.ts:38-39` — `listInstances` silently falls back to `credentials.region` when `StackSetName` is missing** ```typescript const stackSetName = (args as any).StackSetName as string ?? credentials.region; ``` If `StackSetName` is ever absent from `args`, this falls back to using the AWS region (e.g. `"us-east-1"`) as the StackSet name. This would produce a confusing CloudFormation `StackSetNotFoundException` blaming a region string as a StackSet name. The other three functions (`listOperations:94`, `describeOperation:152`, `detectDrift:224`) do NOT have this fallback — they cast directly to `string` without a `??`. This is almost certainly a copy-paste remnant. **Breaking input**: Call `listInstances` with args that omit `StackSetName` (e.g. if globalArgs validation is bypassed or the field is undefined at runtime). **Suggested fix**: Remove the `?? credentials.region` fallback and either throw an explicit error if StackSetName is missing, or rely on the Zod schema enforcement alone (consistent with the other three methods): ```typescript const stackSetName = (args as any).StackSetName as string; ``` 2. **`codegen/aws/extensionModelGenerator.ts:195-208` — import deduplication drops entire import lines, potentially losing named imports** The import dedup logic for model methods skips an entire import line if its package string matches a package already imported by enrichment or listMethod: ```typescript const pkg = imp.match(/"([^"]+)"/)?.[1]; if (pkg && existingImportPackages.has(pkg)) continue; // entire line skipped ``` If a resource has both an enrichment and customMethods that import *different* named symbols from the same npm package (e.g. enrichment imports `{ ListStacksCommand }` and methods import `{ DetectStackSetDriftCommand }` from the same `@aws-sdk/client-cloudformation`), the model methods' import line is silently dropped. The generated code would reference symbols that are never imported, producing a compile error. **Currently not triggered** because cfn-stackset has no enrichment `sourceFile`, so the dedup set is empty. This becomes a real bug the first time someone adds both enrichment and customMethods for a resource sharing an npm package. **Suggested fix**: Merge named imports from the same package rather than dropping the entire later import line. Alternatively, document that enrichment and customMethods must not share a package, with a build-time assertion. 3. **`codegen/aws/enrichments/cfn-stackset/methods.ts:226-227` — `detectDrift` has no bounds on poll interval or timeout** ```typescript const pollIntervalMs = (args.pollIntervalMs as number | undefined) ?? 5000; const timeoutMs = (args.timeoutMs as number | undefined) ?? 300000; ``` The Zod schema validates these are numbers but imposes no minimum. A caller passing `pollIntervalMs: 0` (or a negative value) gets `setTimeout(resolve, 0)` — a tight loop that hammers the CloudFormation DescribeStackSetOperation API as fast as the network allows. With default `timeoutMs: 300000`, that's up to 5 minutes of uncapped API calls, likely hitting rate limits. **Breaking input**: `{ StackSetName: "x", pollIntervalMs: 0, timeoutMs: 300000 }` **Suggested fix**: Clamp `pollIntervalMs` to a floor (e.g. `Math.max(pollIntervalMs, 1000)`) and `timeoutMs` to a ceiling. Or add `.min()` constraints in the Zod schema in config.ts. 4. **`codegen/designs/aws.md` (added section 8b) — design doc describes a file structure that doesn't match the implementation** The design doc says model methods live in `codegen/aws/methods/` as a *separate system from enrichments*: > Model methods live in `codegen/aws/methods/` — a separate system from enrichments with its own types, registry, and generator path. The enrichment system is not modified. In reality, model methods are implemented *inside* the enrichment system: - Types are in `codegen/aws/enrichments/types.ts` (`AwsEnrichmentCustomMethod`, `customMethods` on `AwsEnrichment`) - Config is in `codegen/aws/enrichments/cfn-stackset/config.ts` - Registry is `getEnrichment()` in `codegen/aws/enrichments/index.ts` Every file path and "registry" name in the design doc is wrong. A developer following section 8b's "Adding a new model methods config" instructions would create files in directories that don't exist. ### Low 1. **`codegen/aws/enrichments/cfn-stackset/methods.ts:240-241, 258` — debug `console.log` statements in production code** `detectDrift` logs the StackSet name and operation status to stdout on every poll iteration. Not a security issue (no secrets), but noisy in production and inconsistent with the other methods which don't log. ### Verdict **PASS** — No critical or high severity issues. The medium findings are real but either latent (import dedup) or bounded in impact (region-as-stackset-name produces a clear AWS error rather than silent data corruption; polling floor is a robustness concern). The design doc mismatch should be fixed before it misleads contributors.

stack72 added 1 commit 2026-06-16 20:30:22 +00:00

fix: address review findings — remove any types, fix StackSet name fallback, clamp poll interval ... ae2a0a0ab9

- Remove (args as any).StackSetName fallback to credentials.region in
  listInstances — was a copy-paste bug that would produce confusing
  StackSetNotFoundException
- Replace all `any` casts with proper SDK types (CallAs, StackInstanceFilter)
- Remove console.log from detectDrift — was noisy in production
- Clamp pollIntervalMs to min 1000ms and timeoutMs to max 600000ms to
  prevent tight-loop API hammering
- Update design doc section 8b to reflect actual architecture (enrichment
  system with customMethods, not separate methods/ directory)
- Remove unnecessary no-explicit-any lint suppression

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

stack72 commented 2026-06-16 20:43:38 +00:00

Author

Owner

Copy link

Code Review

Blocking Issues

None.

Suggestions

1. Design doc section 8a is stale after the enrichment reorganization (codegen/designs/aws.md, section 8a). It still shows the old flat-file structure (rds-dbcluster.ts, rds-dbcluster.enrich.ts) and the "Adding a new enrichment" steps still say to create <service>-<resource>.enrich.ts at the enrichment root. Section 8b correctly shows the new subdirectory layout (rds-dbcluster/config.ts, etc.). Section 8a should be updated to match so the two sections don't contradict each other.

2. Duplicate ParsedEnrichmentSource / ParsedSource interfaces. enrichments/types.ts defines ParsedEnrichmentSource { imports, body } and shared/sourceParser.ts defines ParsedSource { imports, body } — they are structurally identical. AwsExtensionModelListMethod.source uses ParsedEnrichmentSource while ParsedModelMethods.source uses ParsedSource, for no semantic reason. Could consolidate to one type as a follow-up.

3. Non-array custom method instance naming (extensionModelGenerator.ts:806). For returnsArray: false methods, the generated instance name is context.globalArgs.name?.toString() ?? "<methodName>". For naturally-named resources like StackSet (primary identifier is StackSetName, no synthetic name field), globalArgs.name is always undefined, so every call to describeOperation writes to the slot "describeOperation" — meaning calls with different operationId values silently overwrite each other. Using (mergedArgs.operationId ?? "<methodName>").toString() for describeOperation, or including a user-visible caveat in the description, would make the behaviour more predictable.

## Code Review ### Blocking Issues None. ### Suggestions 1. **Design doc section 8a is stale after the enrichment reorganization** (`codegen/designs/aws.md`, section 8a). It still shows the old flat-file structure (`rds-dbcluster.ts`, `rds-dbcluster.enrich.ts`) and the "Adding a new enrichment" steps still say to create `<service>-<resource>.enrich.ts` at the enrichment root. Section 8b correctly shows the new subdirectory layout (`rds-dbcluster/config.ts`, etc.). Section 8a should be updated to match so the two sections don't contradict each other. 2. **Duplicate `ParsedEnrichmentSource` / `ParsedSource` interfaces.** `enrichments/types.ts` defines `ParsedEnrichmentSource { imports, body }` and `shared/sourceParser.ts` defines `ParsedSource { imports, body }` — they are structurally identical. `AwsExtensionModelListMethod.source` uses `ParsedEnrichmentSource` while `ParsedModelMethods.source` uses `ParsedSource`, for no semantic reason. Could consolidate to one type as a follow-up. 3. **Non-array custom method instance naming** (`extensionModelGenerator.ts:806`). For `returnsArray: false` methods, the generated instance name is `context.globalArgs.name?.toString() ?? "<methodName>"`. For naturally-named resources like `StackSet` (primary identifier is `StackSetName`, no synthetic `name` field), `globalArgs.name` is always `undefined`, so every call to `describeOperation` writes to the slot `"describeOperation"` — meaning calls with different `operationId` values silently overwrite each other. Using `(mergedArgs.operationId ?? "<methodName>").toString()` for `describeOperation`, or including a user-visible caveat in the description, would make the behaviour more predictable.

stack72 commented 2026-06-16 20:44:16 +00:00

Author

Owner

Copy link

Adversarial Review

Medium

1. Import deduplication skips entire import statements by package name, not by symbol — codegen/aws/extensionModelGenerator.ts:195-209

   The custom methods import dedup logic (and the pre-existing listMethod dedup at lines 183-194) checks whether a package name has already appeared in an earlier import. If it has, the entire import statement is skipped. This means if a resource has both an enrichState enrichment and customMethods that import different symbols from the same @aws-sdk/client-* package, the custom methods' import would be silently dropped, producing generated code with undefined references.

   Breaking example: Suppose rds-dbcluster gains custom methods that import {ModifyDBClusterCommand} from @aws-sdk/client-rds. The enrichment already imports {DescribeDBClustersCommand, RDSClient} from the same package. The custom methods import would be skipped → ModifyDBClusterCommand would be undefined in the inlined body → the generated model would compile (TypeScript sees the function body but the import is missing) and fail at runtime.

   Currently safe: No enrichment combines enrichState/listMethod AND customMethods importing from the same SDK package. The StackSet enrichment only has customMethods, and the RDS enrichment has no customMethods. But this is a latent trap for the next person who adds custom methods to an already-enriched resource.

   Suggested fix: Merge import specifiers by package instead of skip-whole-statement. Or at minimum, add a code comment warning that each enrichment type must import all its own symbols and not rely on imports from another enrichment type using the same package.

2. detectDrift timeout check is post-sleep, so wall time can exceed timeoutMs by pollIntervalMs + API latency — codegen/aws/enrichments/cfn-stackset/methods.ts:248-249

   The while loop sleeps first, then makes the API call, then checks if terminal. The timeout condition (Date.now() - startTime < timeoutMs) is only evaluated at the top of the loop. So the actual wall time before the timeout error is thrown is timeoutMs + pollIntervalMs + API_call_time in the worst case.

   Breaking example: pollIntervalMs: 5000, timeoutMs: 1000. The function sleeps 5s, makes an API call (~200ms), and the timeout error says "timed out after 1000ms" even though ~5200ms elapsed. Not a data integrity issue, but the error message is misleading.

   Suggested fix: Check elapsed time immediately after sleeping but before the API call, or rewrite to do { call; check; if (!terminal) sleep; } while (...).

Low

1. timeoutMs has no lower bound — setting 0 silently abandons drift detection — codegen/aws/enrichments/cfn-stackset/methods.ts:230-233

   Math.min(0, 600000) → 0. The while loop never enters. The function initiates drift detection on AWS (which keeps running server-side) and immediately throws "timed out after 0ms". The drift detection operation is now orphaned — the user gets an error but drift detection is actually running.

   Not a security issue or data loss, but users who accidentally pass timeoutMs: 0 get a confusing experience.

2. context.globalArgs.name in non-array custom methods may reference a field that doesn't exist for natural-naming resources — codegen/aws/extensionModelGenerator.ts:807-810

   For non-array custom methods, the generated instance name is context.globalArgs.name?.toString() ?? "${method.methodName}". If the resource uses natural naming (e.g., StackSetName), there is no name field in globalArgs, so the fallback is always used. Every invocation of describeOperation overwrites the same "describeOperation" instance. This is arguably correct (latest-wins for a single-result method), but if someone calls describeOperation for op-1 then op-2, the op-1 result is lost.

   The StackSet model uses synthetic naming (StackSetId is read-only primary identifier), so name is always present in practice for the current enrichment. But a future enrichment on a natural-named resource would hit this.

3. CloudFormationClient is never destroyed in methods.ts — codegen/aws/enrichments/cfn-stackset/methods.ts:14-33

   Each function call creates a new CloudFormationClient via createCfnClient() but never calls client.destroy(). For detectDrift, which may make 60+ API calls through a single client, this is fine during the call. But after the function returns, the client's HTTP connection pool is leaked until GC. This is standard AWS SDK practice and unlikely to cause problems in normal usage, but is worth noting for long-running processes that call these methods frequently.

Verdict

PASS — The code is well-structured and follows established patterns in the codebase. The custom methods enrichment architecture cleanly extends the existing enrichment system. Test coverage is solid with proper mock servers, env var restoration, and edge case tests (empty results, timeout). The import deduplication issue (Medium #1) is a latent bug that doesn't affect any current code paths, and the timeout post-sleep issue (Medium #2) is a minor behavioral wart. No blocking issues found.

## Adversarial Review ### Medium 1. **Import deduplication skips entire import statements by package name, not by symbol** — `codegen/aws/extensionModelGenerator.ts:195-209` The custom methods import dedup logic (and the pre-existing listMethod dedup at lines 183-194) checks whether a package name has already appeared in an earlier import. If it has, the entire import statement is skipped. This means if a resource has both an `enrichState` enrichment and `customMethods` that import *different symbols* from the same `@aws-sdk/client-*` package, the custom methods' import would be silently dropped, producing generated code with undefined references. **Breaking example**: Suppose `rds-dbcluster` gains custom methods that import `{ModifyDBClusterCommand}` from `@aws-sdk/client-rds`. The enrichment already imports `{DescribeDBClustersCommand, RDSClient}` from the same package. The custom methods import would be skipped → `ModifyDBClusterCommand` would be undefined in the inlined body → the generated model would compile (TypeScript sees the function body but the import is missing) and fail at runtime. **Currently safe**: No enrichment combines `enrichState`/`listMethod` AND `customMethods` importing from the same SDK package. The StackSet enrichment only has `customMethods`, and the RDS enrichment has no `customMethods`. But this is a latent trap for the next person who adds custom methods to an already-enriched resource. **Suggested fix**: Merge import specifiers by package instead of skip-whole-statement. Or at minimum, add a code comment warning that each enrichment type must import all its own symbols and not rely on imports from another enrichment type using the same package. 2. **`detectDrift` timeout check is post-sleep, so wall time can exceed `timeoutMs` by `pollIntervalMs + API latency`** — `codegen/aws/enrichments/cfn-stackset/methods.ts:248-249` The while loop sleeps first, then makes the API call, then checks if terminal. The timeout condition (`Date.now() - startTime < timeoutMs`) is only evaluated at the top of the loop. So the actual wall time before the timeout error is thrown is `timeoutMs + pollIntervalMs + API_call_time` in the worst case. **Breaking example**: `pollIntervalMs: 5000, timeoutMs: 1000`. The function sleeps 5s, makes an API call (~200ms), and the timeout error says "timed out after 1000ms" even though ~5200ms elapsed. Not a data integrity issue, but the error message is misleading. **Suggested fix**: Check elapsed time immediately after sleeping but before the API call, or rewrite to `do { call; check; if (!terminal) sleep; } while (...)`. ### Low 1. **`timeoutMs` has no lower bound — setting 0 silently abandons drift detection** — `codegen/aws/enrichments/cfn-stackset/methods.ts:230-233` `Math.min(0, 600000)` → `0`. The while loop never enters. The function initiates drift detection on AWS (which keeps running server-side) and immediately throws `"timed out after 0ms"`. The drift detection operation is now orphaned — the user gets an error but drift detection is actually running. Not a security issue or data loss, but users who accidentally pass `timeoutMs: 0` get a confusing experience. 2. **`context.globalArgs.name` in non-array custom methods may reference a field that doesn't exist for natural-naming resources** — `codegen/aws/extensionModelGenerator.ts:807-810` For non-array custom methods, the generated instance name is `context.globalArgs.name?.toString() ?? "${method.methodName}"`. If the resource uses natural naming (e.g., `StackSetName`), there is no `name` field in globalArgs, so the fallback is always used. Every invocation of `describeOperation` overwrites the same `"describeOperation"` instance. This is arguably correct (latest-wins for a single-result method), but if someone calls `describeOperation` for `op-1` then `op-2`, the `op-1` result is lost. The StackSet model uses synthetic naming (StackSetId is read-only primary identifier), so `name` is always present in practice for the current enrichment. But a future enrichment on a natural-named resource would hit this. 3. **CloudFormationClient is never destroyed in methods.ts** — `codegen/aws/enrichments/cfn-stackset/methods.ts:14-33` Each function call creates a new `CloudFormationClient` via `createCfnClient()` but never calls `client.destroy()`. For `detectDrift`, which may make 60+ API calls through a single client, this is fine during the call. But after the function returns, the client's HTTP connection pool is leaked until GC. This is standard AWS SDK practice and unlikely to cause problems in normal usage, but is worth noting for long-running processes that call these methods frequently. ### Verdict **PASS** — The code is well-structured and follows established patterns in the codebase. The custom methods enrichment architecture cleanly extends the existing enrichment system. Test coverage is solid with proper mock servers, env var restoration, and edge case tests (empty results, timeout). The import deduplication issue (Medium #1) is a latent bug that doesn't affect any current code paths, and the timeout post-sleep issue (Medium #2) is a minor behavioral wart. No blocking issues found.

stack72 added 1 commit 2026-06-16 20:59:44 +00:00

fix: address second review — instance naming, timeout floor, stale docs ... 464bb60789

- Non-array custom methods now key instance names by method args
  (e.g. "describeOperation-op-123") so different invocations don't
  overwrite each other
- Add MIN_TIMEOUT_MS (10s) floor so timeoutMs: 0 can't bypass the
  drift polling loop
- Update design doc section 8a to reflect folder-per-resource layout
  and fix stale file paths that referenced the old flat structure
- Add comment documenting the import dedup limitation (skips by
  package, not symbol — latent issue if enrichment + customMethods
  share a package)

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

stack72 commented 2026-06-16 21:13:06 +00:00

Author

Owner

Copy link

Adversarial Review

Critical / High

No critical or high severity findings.

Medium

1. Region resolution inconsistency between CRUD and custom methods — codegen/aws/enrichments/cfn-stackset/methods.ts:15-18

   createCfnClient resolves region as credentials.region → AWS_REGION → AWS_DEFAULT_REGION → "us-east-1". The design doc (section 7) documents that _lib/aws.ts resolves region as explicit → AWS_REGION → AWS_DEFAULT_REGION → ~/.aws/config profile region → us-east-1. Custom methods skip the profile region step.

   Breaking example: A user has region = ap-southeast-1 in ~/.aws/config (via [profile myprofile]), sets AWS_PROFILE=myprofile, but does not set AWS_REGION or AWS_DEFAULT_REGION env vars, and does not set a region global arg. CRUD methods (get, create, sync) will operate against ap-southeast-1, but listInstances, detectDrift, etc. will hit us-east-1.

   Suggested fix: Either read ~/.aws/config in createCfnClient to match _lib/aws.ts, or omit the region entirely from the client config when no explicit region is provided (letting the AWS SDK resolve it from the default credential chain, which includes profile config). The simplest option:

   const region = credentials.region ??
     Deno.env.get("AWS_REGION") ??
     Deno.env.get("AWS_DEFAULT_REGION");
   // Let SDK resolve from profile if no explicit region
   const config: Record<string, unknown> = region ? { region } : {};
   

2. Timeout test runs ~50x slower than intended — codegen/aws/enrichments/cfn-stackset/methods_test.ts:332-338

   The "detectDrift times out" test passes pollIntervalMs: 50, timeoutMs: 200, but detectDrift clamps these to pollIntervalMs: 1000 (MIN_POLL_INTERVAL_MS) and timeoutMs: 10000 (MIN_TIMEOUT_MS). The test correctness is fine (it exercises the timeout path), but it takes ~10 seconds instead of the apparent ~200ms. The successful drift test (line 277-278) is similarly affected: pollIntervalMs: 100 is clamped to 1000ms.

   Suggested fix: Use values at or above the clamp floors in the tests to make the declared values match the actual runtime behavior:

   pollIntervalMs: 1000,
   timeoutMs: 10000,
   

   Or, expose the constants and use them in the test to make the relationship explicit.

Low

3. Import dedup drops entire lines by package name — codegen/aws/extensionModelGenerator.ts:196-213

   If a future enrichment combines enrichState and customMethods that import different symbols from the same npm package, the customMethods import line is silently dropped, causing a compile error in the generated model. The comment on line 195-198 acknowledges this. No current enrichment triggers this, but it's a latent trap for the next contributor.

4. Unbounded instance name length for non-array custom methods — codegen/aws/extensionModelGenerator.ts:811-819

   Non-array method instance names are built from methodName + "-" + args.join("-"). Argument values like operationId are user-provided strings. The sanitizer handles path traversal but not length. An extremely long operationId produces an extremely long instance name. Unlikely to cause issues in practice.

Verdict

PASS — The core logic (custom methods, source parser extraction, codegen emission, import dedup) is correct and well-tested. The region inconsistency (Medium #1) is the most substantive finding but is an edge case affecting only users who rely solely on ~/.aws/config profile-based region without any env var or explicit arg. The slow test (Medium #2) is a CI hygiene issue, not a correctness bug. The code is cleanly structured, tests cover the key paths (pagination, timeout, empty results), and the enrichment system's factoring into shared sourceParser.ts is a sound refactor.

## Adversarial Review ### Critical / High No critical or high severity findings. ### Medium 1. **Region resolution inconsistency between CRUD and custom methods** — `codegen/aws/enrichments/cfn-stackset/methods.ts:15-18` `createCfnClient` resolves region as `credentials.region → AWS_REGION → AWS_DEFAULT_REGION → "us-east-1"`. The design doc (section 7) documents that `_lib/aws.ts` resolves region as `explicit → AWS_REGION → AWS_DEFAULT_REGION → ~/.aws/config profile region → us-east-1`. Custom methods skip the profile region step. **Breaking example**: A user has `region = ap-southeast-1` in `~/.aws/config` (via `[profile myprofile]`), sets `AWS_PROFILE=myprofile`, but does not set `AWS_REGION` or `AWS_DEFAULT_REGION` env vars, and does not set a `region` global arg. CRUD methods (`get`, `create`, `sync`) will operate against `ap-southeast-1`, but `listInstances`, `detectDrift`, etc. will hit `us-east-1`. **Suggested fix**: Either read `~/.aws/config` in `createCfnClient` to match `_lib/aws.ts`, or omit the region entirely from the client config when no explicit region is provided (letting the AWS SDK resolve it from the default credential chain, which includes profile config). The simplest option: ```typescript const region = credentials.region ?? Deno.env.get("AWS_REGION") ?? Deno.env.get("AWS_DEFAULT_REGION"); // Let SDK resolve from profile if no explicit region const config: Record<string, unknown> = region ? { region } : {}; ``` 2. **Timeout test runs ~50x slower than intended** — `codegen/aws/enrichments/cfn-stackset/methods_test.ts:332-338` The "detectDrift times out" test passes `pollIntervalMs: 50, timeoutMs: 200`, but `detectDrift` clamps these to `pollIntervalMs: 1000` (`MIN_POLL_INTERVAL_MS`) and `timeoutMs: 10000` (`MIN_TIMEOUT_MS`). The test correctness is fine (it exercises the timeout path), but it takes ~10 seconds instead of the apparent ~200ms. The successful drift test (line 277-278) is similarly affected: `pollIntervalMs: 100` is clamped to 1000ms. **Suggested fix**: Use values at or above the clamp floors in the tests to make the declared values match the actual runtime behavior: ```typescript pollIntervalMs: 1000, timeoutMs: 10000, ``` Or, expose the constants and use them in the test to make the relationship explicit. ### Low 3. **Import dedup drops entire lines by package name** — `codegen/aws/extensionModelGenerator.ts:196-213` If a future enrichment combines `enrichState` and `customMethods` that import different symbols from the same npm package, the `customMethods` import line is silently dropped, causing a compile error in the generated model. The comment on line 195-198 acknowledges this. No current enrichment triggers this, but it's a latent trap for the next contributor. 4. **Unbounded instance name length for non-array custom methods** — `codegen/aws/extensionModelGenerator.ts:811-819` Non-array method instance names are built from `methodName + "-" + args.join("-")`. Argument values like `operationId` are user-provided strings. The sanitizer handles path traversal but not length. An extremely long `operationId` produces an extremely long instance name. Unlikely to cause issues in practice. ### Verdict **PASS** — The core logic (custom methods, source parser extraction, codegen emission, import dedup) is correct and well-tested. The region inconsistency (Medium #1) is the most substantive finding but is an edge case affecting only users who rely solely on `~/.aws/config` profile-based region without any env var or explicit arg. The slow test (Medium #2) is a CI hygiene issue, not a correctness bug. The code is cleanly structured, tests cover the key paths (pagination, timeout, empty results), and the enrichment system's factoring into shared `sourceParser.ts` is a sound refactor.

stack72 commented 2026-06-16 21:15:34 +00:00

Author

Owner

Copy link

Code Review

Blocking Issues

1. methods_test.ts: Drift tests will be prohibitively slow due to clamped minimums

   detectDrift clamps the caller-supplied values to hard floors: MIN_POLL_INTERVAL_MS = 1000 and MIN_TIMEOUT_MS = 10000. Both tests pass values far below those floors:

   • detectDrift polls until COMPLETED — passes pollIntervalMs: 100 (clamped to 1000 ms). The mock returns RUNNING for the first 3 DescribeStackSetOperation calls, so the test sleeps at least 3 × 1000 ms = 3 seconds.
   • detectDrift times out when status never completes — passes pollIntervalMs: 50 (→ 1000 ms) and timeoutMs: 200 (→ 10000 ms). The test will spin for ≥ 10 seconds before the timeout fires.

   These constants were added in the previous review fix (fix: address review findings — clamp poll interval), but the test values were never updated to match. Fix: update the test values to meet or exceed the actual floors, e.g. pollIntervalMs: 1000, timeoutMs: 11000 for the poll test and timeoutMs: 10000 for the timeout test (or reduce MIN_TIMEOUT_MS to something more test-friendly, e.g. 2000 ms).

   Relevant lines: methods_test.ts:277-298 (poll test) and methods_test.ts:329-343 (timeout test); constants at methods.ts:216-218.

Suggestions

1. rds-dbcluster/list.enrich.ts and rds-dbcluster/enrich.ts: user-supplied credentials are silently ignored for list/enrich operations

   Both functions create their own RDSClient from Deno.env.get("AWS_REGION") only — no credentials parameter. If a user configures accessKeyId/secretAccessKey in globalArgs, those values are used for CloudControl CRUD but silently ignored for the list and enrich calls. The new customMethods pattern (cfn-stackset) correctly accepts a credentials: AwsCredentials argument; it would be worth carrying this forward to the list/enrich pattern as well.

2. parser.ts: parseEnrichmentSource and parseListMethodSource are identical

   Both functions are one-liners that forward unconditionally to parseSourceFile. They add no behavioral difference and create a misleading impression that the two paths differ. Consider either removing the indirection (callers can call parseSourceFile directly) or adding a meaningful distinction (e.g. the { stripAwsLib: true } option that pipeline.ts applies to custom-methods sources but not to enrichment sources).

3. Import deduplication drops symbols when packages overlap (acknowledged, but worth tracking)

   The comment at extensionModelGenerator.ts:196-198 documents that if an enrichment and a customMethods source both import from the same npm package, only the enrichment's import is kept, which silently drops symbols needed by the custom methods. The current pairings don't trigger this, but it will become a landmine when a future enrichment combines both. Consider merging imports symbol-by-symbol rather than by package URL.

## Code Review ### Blocking Issues 1. **`methods_test.ts`: Drift tests will be prohibitively slow due to clamped minimums** `detectDrift` clamps the caller-supplied values to hard floors: `MIN_POLL_INTERVAL_MS = 1000` and `MIN_TIMEOUT_MS = 10000`. Both tests pass values far below those floors: - `detectDrift polls until COMPLETED` — passes `pollIntervalMs: 100` (clamped to 1000 ms). The mock returns RUNNING for the first 3 `DescribeStackSetOperation` calls, so the test sleeps at least **3 × 1000 ms = 3 seconds**. - `detectDrift times out when status never completes` — passes `pollIntervalMs: 50` (→ 1000 ms) and `timeoutMs: 200` (→ 10000 ms). The test will spin for **≥ 10 seconds** before the timeout fires. These constants were added in the previous review fix (`fix: address review findings — clamp poll interval`), but the test values were never updated to match. Fix: update the test values to meet or exceed the actual floors, e.g. `pollIntervalMs: 1000, timeoutMs: 11000` for the poll test and `timeoutMs: 10000` for the timeout test (or reduce `MIN_TIMEOUT_MS` to something more test-friendly, e.g. 2000 ms). Relevant lines: `methods_test.ts:277-298` (poll test) and `methods_test.ts:329-343` (timeout test); constants at `methods.ts:216-218`. ### Suggestions 1. **`rds-dbcluster/list.enrich.ts` and `rds-dbcluster/enrich.ts`: user-supplied credentials are silently ignored for list/enrich operations** Both functions create their own `RDSClient` from `Deno.env.get("AWS_REGION")` only — no credentials parameter. If a user configures `accessKeyId`/`secretAccessKey` in `globalArgs`, those values are used for CloudControl CRUD but silently ignored for the list and enrich calls. The new `customMethods` pattern (cfn-stackset) correctly accepts a `credentials: AwsCredentials` argument; it would be worth carrying this forward to the list/enrich pattern as well. 2. **`parser.ts`: `parseEnrichmentSource` and `parseListMethodSource` are identical** Both functions are one-liners that forward unconditionally to `parseSourceFile`. They add no behavioral difference and create a misleading impression that the two paths differ. Consider either removing the indirection (callers can call `parseSourceFile` directly) or adding a meaningful distinction (e.g. the `{ stripAwsLib: true }` option that `pipeline.ts` applies to custom-methods sources but not to enrichment sources). 3. **Import deduplication drops symbols when packages overlap (acknowledged, but worth tracking)** The comment at `extensionModelGenerator.ts:196-198` documents that if an enrichment and a customMethods source both import from the same npm package, only the enrichment's import is kept, which silently drops symbols needed by the custom methods. The current pairings don't trigger this, but it will become a landmine when a future enrichment combines both. Consider merging imports symbol-by-symbol rather than by package URL.

stack72 merged commit a80ff91d51 into main 2026-06-16 21:19:32 +00:00

stack72 deleted branch worktree-618 2026-06-16 21:19:32 +00:00

stack72 referenced this pull request from a commit 2026-06-16 21:19:33 +00:00

feat(aws/cloudformation): expose StackSet instances, drift, and operations (#57)

Sign in to join this conversation.

Reviewers

No reviewers

Labels

Clear labels

No items

No labels

Milestone

Clear milestone

No items

No milestone

Assignees

Clear assignees

No assignees

1 participant

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference

swamp-club/swamp-extensions!57

No description provided.