Commit fc66f06
fix(config): pass complete config via environment variables to CDK (#307)
* cleanup a07 specs
Co-Authored-By: Claude <noreply@anthropic.com>
* chore: lint markdown tables
* docs(spec): add A07 analysis and implementation plan
Comprehensive analysis of A07 config validation issue:
- 01: Original library-config review
- 02: Original library-config spec
- 03: Postmortem of script breakage
- 04: Analysis of the right fix approach
- 05: Complete implementation plan
Key findings:
- A07 added validation not in spec (broke scripts)
- Root cause: deploy.ts doesn't pass config via env vars
- Solution: Pass complete config via environment variables
- Architecture: config always complete, parameters are optional overrides
Implementation plan includes:
- Detailed code changes (add ~15 env vars to deploy.ts)
- Testing strategy
- Rollout phases
- Why alternatives were rejected
- Success criteria and risk assessment
🤖 Generated with Claude Code
Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
* fix(config): pass complete config via environment variables to CDK
Fixes A07 config validation break in NPM script deployments.
Problem:
- A07 added validation that broke `npm run deploy:dev`
- deploy.ts wasn't passing config fields to benchling-webhook.ts
- Stack received empty config, validation failed
Solution:
- Pass complete config via environment variables from deploy.ts
- benchling-webhook.ts already reads these env vars
- No code changes needed in benchling-webhook.ts
Added environment variables:
- QUILT_CATALOG, QUILT_DATABASE, QUEUE_URL (required)
- ICEBERG_DATABASE, ICEBERG_WORKGROUP (optional)
- ATHENA_USER_WORKGROUP, ATHENA_RESULTS_BUCKET (optional)
- QUILT_USER_BUCKET, PKG_PREFIX, PKG_KEY (package config)
- BENCHLING_TENANT, BENCHLING_CLIENT_ID, BENCHLING_APP_DEFINITION_ID
- LOG_LEVEL, IMAGE_TAG, ECR_REPOSITORY_NAME (optional)
Benefits:
- Clean architecture: config is always complete
- Preserves A07 validation benefits for library users
- Parameters remain optional overrides
- No magic bypasses or workarounds
See: spec/a07-library-config/05-implementation-plan.md
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude <noreply@anthropic.com>
* chore: sync versions to 0.9.7
* chore: update CHANGELOG for v0.9.7
Documents A07 config validation fix.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude <noreply@anthropic.com>
* chore: complete pending TODOs and improve test coverage
This commit addresses all pending TODO items in the codebase:
**bin/benchling-webhook.ts:**
- Remove legacy `legacyConfigToProfileConfig()` function marked for Phase 4 removal
- Inline Config-to-ProfileConfig conversion in `createStack()` for clarity
- Update documentation to reflect current implementation state
- Remove outdated "Phase 4" TODO comments
**bin/commands/config-profiles.ts:**
- Update file header documentation to confirm v0.7.0+ migration is complete
- Remove obsolete warning comments about refactoring needed
- File already correctly uses ProfileConfig, XDGConfig, and readProfile API
**test/multi-environment-fargate-service.test.ts:**
- Re-enable LOG_LEVEL environment variable test (was test.skip)
- Fix assertion to match actual implementation (LOG_LEVEL not LogLevel)
- Add value assertion to verify LOG_LEVEL="DEBUG"
- Test now passes and verifies correct environment variable configuration
**test/integration/stack-resource-discovery.test.ts:**
- Replace TODO comments with architectural documentation
- Document AWS SDK v3 dynamic import issues with Jest
- Explain why getStackResources() wrapper is preferred over direct SDK calls
- Clarify how test coverage is maintained through wrapper functions
All tests pass (357 Python tests + TypeScript tests).
No TODOs/FIXMEs remaining in modified files.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude <noreply@anthropic.com>
* docs(spec): document Docker double-wrapping observation for CMD syntax bug
- Add observation that test:minimal passes despite buggy Dockerfile
- Document hypothesis: Docker double-wraps shell form CMD locally
- Explain why ECS fails but local tests pass
- Note testing limitation: cannot reproduce locally with realistic test
- Fix is still necessary but won't be caught by local tests
* fix(docker): quote create_app() to prevent shell parsing error
The unquoted parentheses in 'src.app:create_app()' cause shell syntax
errors in ECS when the shell interprets them as subshell operators.
Quoting the factory function as 'src.app:create_app()' prevents this
while preserving PORT variable expansion for multi-stack support.
Note: Local tests pass despite the bug due to Docker's double-wrapping
behavior with shell form CMD. The bug only manifests in ECS production.
Fixes: A07-07 Docker CMD Shell Syntax Error
Related: spec/a07-library-config/07-docker-cmd-shell-syntax-fix.md
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude <noreply@anthropic.com>
* fix(test): always rebuild Docker image in test:minimal
Changes:
- Replace conditional build check with unconditional build
- Use `make -C docker docker-build-local` for consistency with build infrastructure
- Ensures test:minimal always runs against latest code changes
Rationale:
- Previous approach only built if image didn't exist, causing stale image issues
- Using Makefile target ensures consistency with other build processes
- Mirrors CI behavior where fresh builds are always performed
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude <noreply@anthropic.com>
* fix(config): correct stack name resolution in legacy createStack function
Fix TypeScript compilation errors caused by accessing non-existent properties
on the legacy Config type (config.profile and config.stackName).
Changes:
- Use hardcoded "default" profile for legacy compatibility
- Read custom stack name from profileConfig.deployment?.stackName
- Maintains backwards compatibility with existing deployments
This ensures the legacy code path works correctly while supporting the
multi-stack feature when using the XDG profile system.
Related: spec/a07-library-config/06-multi-stack-support.md
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude <noreply@anthropic.com>
* fix: catch PENDING:REVIEW
* fix(deploy): detect and auto-destroy REVIEW_IN_PROGRESS stacks
REVIEW_IN_PROGRESS is an unrecoverable CloudFormation state that occurs
when a change set fails validation during creation. The stack exists but
cannot be updated or deployed.
Changes:
- Add automatic detection of REVIEW_IN_PROGRESS state after stack check
- Prompt user to destroy and recreate (only viable option)
- Auto-destroy with proper CDK environment variables
- Continue with fresh deployment after successful destroy
- Extract getCdkAppPath() and buildCdkEnv() helpers to eliminate 100+ lines of duplication
Fixes issue where deploy command would fail with:
"Failed to create ChangeSet: AWS::EarlyValidation::ResourceExistenceCheck failed"
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude <noreply@anthropic.com>
* feat(cli): add clean command to remove configuration profiles
Add new 'clean' command that allows users to safely remove configuration
profiles from the system. The command:
- Removes profile configuration and deployment tracking
- Shows warnings for profiles with active deployments
- Prevents accidental deletion with confirmation prompt
- Supports --yes flag for non-interactive use
- Protects the default profile from deletion (enforced by XDGBase)
Usage:
npm run setup:clean -- --profile dev
npx @quiltdata/benchling-webhook clean --profile dev --yes
IMPORTANT: This command only removes local configuration files.
AWS resources (CloudFormation stacks) are NOT destroyed and must be
removed separately using the 'destroy' command.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude <noreply@anthropic.com>
* fix(lint): remove unused hasExistingDeployment variable
Remove dead code that was checking for existing deployments but never
using the result. The variable was assigned but never read, causing
a lint error.
This was likely leftover from incomplete implementation or refactoring
where the logic to use this variable was removed.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude <noreply@anthropic.com>
* lint deploy.ts
Co-Authored-By: Claude <noreply@anthropic.com>
* fix(deploy): use direct deployment method and handle REVIEW_IN_PROGRESS state
- Add --method=direct flag to skip changeset creation entirely
- Prevents new stacks from getting stuck in REVIEW_IN_PROGRESS state
- Improved REVIEW_IN_PROGRESS stack recovery:
- Delete failed changeset first
- Use direct AWS CLI commands instead of CDK destroy
- Add explicit wait for deletion completion
- Fix lint error: rename unused changesetError to _changesetError
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude <noreply@anthropic.com>
* fix(config): enable multi-stack deployments by removing hardcoded resource names
**Problem:** Multiple profiles couldn't deploy to the same AWS account/region because:
1. CloudFormation Export names were hardcoded (e.g., "BenchlingWebhookServiceName")
2. Physical resource names were hardcoded (e.g., clusterName: "benchling-webhook-cluster")
**Root Cause:**
- Exports must be globally unique per region/account
- Physical resource names must be unique per account/region
- Hardcoded names caused conflicts when deploying BenchlingWebhookStack-dev alongside BenchlingWebhookStack
**Solution:**
1. Made CloudFormation Exports profile-aware using stack name prefix:
- lib/fargate-service.ts: "BenchlingWebhookServiceName" → "\${stackName}-ServiceName"
- lib/network-load-balancer.ts: "BenchlingWebhookNLBDnsName" → "\${stackName}-NLBDnsName"
2. Removed hardcoded physical resource names (let CloudFormation auto-generate):
- lib/fargate-service.ts: Removed clusterName and serviceName
- lib/rest-api-gateway.ts: Removed logGroupName and restApiName
3. Made utility scripts profile-aware:
- bin/commands/test.ts: Now uses XDGConfig to get stack name from profile
- scripts/send-event.ts: Added --profile argument support
**Impact:**
- ✅ Multiple profiles can now deploy to same account/region
- ✅ Each stack gets unique auto-generated resource names
- ✅ Exports are scoped per stack (no conflicts)
- ✅ Utility scripts work with any profile
**Testing:**
- npm run lint passes
- Ready for deployment test
Closes #189
* fix(deploy): record deployment endpoint even when CDK exits with error
When CDK deployment succeeds but exits with non-zero code, the deployment
endpoint was never recorded to deployments.json. This caused subsequent
commands to fail because they couldn't find the endpoint.
Now check actual CloudFormation stack status after CDK command fails. If
stack reached CREATE_COMPLETE or UPDATE_COMPLETE, treat as successful and
record the deployment endpoint.
Fixes issue where deployments.json was not created for new profiles.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude <noreply@anthropic.com>
* fix(tests): add missing ListStackResourcesCommand mock to infer-quilt-config tests
The CI was failing because the test file was missing mock responses for
ListStackResourcesCommand, which is called by getStackResources() in
stack-inference.ts. When unmocked, the AWS SDK client returns undefined,
causing 'Cannot read properties of undefined' errors.
This was not caught locally because we ran 'npm test' which uses test:ts,
while CI runs 'npm run test:ci' which uses test:unit with different Jest
configuration.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude <noreply@anthropic.com>
* fix(tests): update CDK stack tests to remove hardcoded resource name expectations
The tests were failing because they expected hardcoded ClusterName and ServiceName
values that were removed in the multi-stack deployment changes (v0.9.8+). These
hardcoded names prevented multiple stacks from being deployed in the same
AWS account/region.
Updated all affected tests to check for resource existence instead of specific
hardcoded names:
- test/multi-environment-fargate-service.test.ts
- test/benchling-webhook-stack.test.ts
- test/multi-environment-stack.test.ts
This allows the infrastructure to support multiple independent deployments
while maintaining test coverage.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude <noreply@anthropic.com>
* fix(tests): remove hardcoded LogGroupName expectation in REST API Gateway test
The test was expecting a hardcoded LogGroupName property, but the implementation
intentionally removed this in v0.9.8 to support multiple stacks per AWS account.
The LogGroup construct now uses auto-generated names to prevent conflicts when
multiple webhook stacks are deployed in the same account/region.
Updated test to only verify the retention policy (7 days), not the exact log
group name, which aligns with the current implementation.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude <noreply@anthropic.com>
---------
Co-authored-by: Claude <noreply@anthropic.com>1 parent 30bc868 commit fc66f06
File tree
144 files changed
+3440
-448
lines changed- bin
- commands
- docker
- lib
- types
- wizard
- scripts
- spec
- 156-secrets-manager
- 176-test-prod
- 189-multi
- 194-rework-dockerfile
- 2025-11-26-architecture
- 206-service-envars
- 221-next-steps
- 280-nginx
- Archives
- 141-ux-improvements
- 156a-secrets-only
- 156b-secrets-fix
- 156c-secrets-config
- 195
- cli
- npx-ux
- queue_arn
- a07-library-config
- test
- bin/commands
- integration
- unit
Some content is hidden
Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.
144 files changed
+3440
-448
lines changed| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
220 | 220 | | |
221 | 221 | | |
222 | 222 | | |
223 | | - | |
| 223 | + | |
224 | 224 | | |
225 | 225 | | |
226 | 226 | | |
| |||
232 | 232 | | |
233 | 233 | | |
234 | 234 | | |
235 | | - | |
| 235 | + | |
236 | 236 | | |
237 | 237 | | |
238 | 238 | | |
| |||
284 | 284 | | |
285 | 285 | | |
286 | 286 | | |
287 | | - | |
| 287 | + | |
288 | 288 | | |
289 | 289 | | |
290 | 290 | | |
| |||
333 | 333 | | |
334 | 334 | | |
335 | 335 | | |
| 336 | + | |
| 337 | + | |
| 338 | + | |
| 339 | + | |
| 340 | + | |
| 341 | + | |
| 342 | + | |
| 343 | + | |
| 344 | + | |
| 345 | + | |
| 346 | + | |
| 347 | + | |
| 348 | + | |
| 349 | + | |
| 350 | + | |
| 351 | + | |
| 352 | + | |
| 353 | + | |
| 354 | + | |
| 355 | + | |
| 356 | + | |
| 357 | + | |
| 358 | + | |
| 359 | + | |
| 360 | + | |
| 361 | + | |
| 362 | + | |
| 363 | + | |
| 364 | + | |
| 365 | + | |
| 366 | + | |
| 367 | + | |
| 368 | + | |
| 369 | + | |
| 370 | + | |
| 371 | + | |
| 372 | + | |
| 373 | + | |
| 374 | + | |
| 375 | + | |
| 376 | + | |
| 377 | + | |
| 378 | + | |
| 379 | + | |
| 380 | + | |
| 381 | + | |
| 382 | + | |
| 383 | + | |
| 384 | + | |
| 385 | + | |
| 386 | + | |
| 387 | + | |
| 388 | + | |
| 389 | + | |
| 390 | + | |
| 391 | + | |
| 392 | + | |
| 393 | + | |
| 394 | + | |
| 395 | + | |
| 396 | + | |
| 397 | + | |
| 398 | + | |
| 399 | + | |
| 400 | + | |
| 401 | + | |
| 402 | + | |
| 403 | + | |
| 404 | + | |
| 405 | + | |
| 406 | + | |
| 407 | + | |
| 408 | + | |
| 409 | + | |
| 410 | + | |
| 411 | + | |
| 412 | + | |
| 413 | + | |
| 414 | + | |
| 415 | + | |
| 416 | + | |
| 417 | + | |
| 418 | + | |
| 419 | + | |
| 420 | + | |
| 421 | + | |
| 422 | + | |
| 423 | + | |
| 424 | + | |
| 425 | + | |
| 426 | + | |
336 | 427 | | |
337 | 428 | | |
338 | 429 | | |
| |||
362 | 453 | | |
363 | 454 | | |
364 | 455 | | |
365 | | - | |
| 456 | + | |
| 457 | + | |
366 | 458 | | |
367 | 459 | | |
368 | 460 | | |
| |||
371 | 463 | | |
372 | 464 | | |
373 | 465 | | |
374 | | - | |
| 466 | + | |
375 | 467 | | |
376 | 468 | | |
377 | 469 | | |
| |||
407 | 499 | | |
408 | 500 | | |
409 | 501 | | |
410 | | - | |
| 502 | + | |
| 503 | + | |
411 | 504 | | |
412 | 505 | | |
413 | 506 | | |
414 | | - | |
415 | | - | |
| 507 | + | |
| 508 | + | |
| 509 | + | |
416 | 510 | | |
417 | 511 | | |
418 | 512 | | |
| |||
421 | 515 | | |
422 | 516 | | |
423 | 517 | | |
424 | | - | |
| 518 | + | |
425 | 519 | | |
426 | 520 | | |
427 | 521 | | |
| |||
516 | 610 | | |
517 | 611 | | |
518 | 612 | | |
519 | | - | |
| 613 | + | |
520 | 614 | | |
521 | 615 | | |
522 | 616 | | |
| |||
531 | 625 | | |
532 | 626 | | |
533 | 627 | | |
| 628 | + | |
534 | 629 | | |
535 | 630 | | |
536 | 631 | | |
| |||
668 | 763 | | |
669 | 764 | | |
670 | 765 | | |
671 | | - | |
| 766 | + | |
672 | 767 | | |
673 | 768 | | |
674 | 769 | | |
| |||
680 | 775 | | |
681 | 776 | | |
682 | 777 | | |
| 778 | + | |
683 | 779 | | |
684 | 780 | | |
685 | 781 | | |
| |||
696 | 792 | | |
697 | 793 | | |
698 | 794 | | |
| 795 | + | |
699 | 796 | | |
700 | 797 | | |
701 | 798 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
3 | 3 | | |
4 | 4 | | |
5 | 5 | | |
| 6 | + | |
| 7 | + | |
| 8 | + | |
| 9 | + | |
| 10 | + | |
| 11 | + | |
| 12 | + | |
| 13 | + | |
| 14 | + | |
| 15 | + | |
| 16 | + | |
| 17 | + | |
6 | 18 | | |
7 | 19 | | |
8 | 20 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
201 | 201 | | |
202 | 202 | | |
203 | 203 | | |
| 204 | + | |
| 205 | + | |
| 206 | + | |
| 207 | + | |
| 208 | + | |
| 209 | + | |
| 210 | + | |
| 211 | + | |
| 212 | + | |
| 213 | + | |
| 214 | + | |
| 215 | + | |
| 216 | + | |
| 217 | + | |
| 218 | + | |
| 219 | + | |
| 220 | + | |
| 221 | + | |
| 222 | + | |
| 223 | + | |
| 224 | + | |
| 225 | + | |
| 226 | + | |
| 227 | + | |
| 228 | + | |
| 229 | + | |
| 230 | + | |
| 231 | + | |
| 232 | + | |
| 233 | + | |
| 234 | + | |
| 235 | + | |
| 236 | + | |
| 237 | + | |
| 238 | + | |
| 239 | + | |
| 240 | + | |
| 241 | + | |
| 242 | + | |
| 243 | + | |
| 244 | + | |
| 245 | + | |
| 246 | + | |
| 247 | + | |
| 248 | + | |
| 249 | + | |
| 250 | + | |
| 251 | + | |
| 252 | + | |
| 253 | + | |
| 254 | + | |
| 255 | + | |
| 256 | + | |
| 257 | + | |
| 258 | + | |
| 259 | + | |
| 260 | + | |
| 261 | + | |
| 262 | + | |
| 263 | + | |
204 | 264 | | |
205 | 265 | | |
206 | 266 | | |
| |||
233 | 293 | | |
234 | 294 | | |
235 | 295 | | |
| 296 | + | |
| 297 | + | |
| 298 | + | |
236 | 299 | | |
237 | 300 | | |
238 | 301 | | |
| |||
0 commit comments