feat: add CDK synthesis validation tests

[notgitika]

Summary

• Adds 19 CDK synthesis smoke tests that validate agentcore.json spec shapes can be synthesized into valid CloudFormation templates via AgentCoreStack → AgentCoreApplication L3 construct
• Covers agents (CodeZip, Container, MCP/A2A protocols), memories, credentials, evaluators, online eval configs, policy engines, and edge cases
• Adds @aws/agentcore-cdk resolve alias in vitest.config.ts to handle CJS-only package exports in Vite

Test plan

• All 19 new tests pass (npx vitest run src/assets/__tests__/cdk-synth-validation.test.ts)
• Existing unit tests unaffected (pre-existing failures only)
• Lint, prettier, typecheck, secretlint all pass (verified by pre-commit hook)

[github-actions]

Coverage Report

Status	Category	Percentage	Covered / Total
🔵	Lines	45.27%	5950 / 13142
🔵	Statements	44.87%	6320 / 14083
🔵	Functions	44.5%	1109 / 2492
🔵	Branches	45.62%	3902 / 8552

Generated in workflow #1321 for commit 9c7ff9d by the Vitest Coverage Report Action

[notgitika]

/strands review

[github-actions]

Code Review Summary

I've completed a thorough review of this PR. While the test coverage is comprehensive, there are several issues that need to be addressed before this can be merged.

---

🚫 BLOCKING Issues

1. Missing Linked Issue ⚠️ [MUST FIX]

Per CONTRIBUTING.md:

Every PR must be linked to an issue. Open an issue first (or find an existing one) and reference it in your PR using Closes #issue-number. PRs without an associated issue will not be reviewed.

Action Required: Add a Closes #<issue-number> link to the PR description.

---

2. Tests Are Excluded from CI ⚠️ [MUST FIX]

The test file is explicitly excluded from the unit test suite in vitest.config.ts line 43:

exclude: ['src/assets/cdk/test/*.test.ts', 'src/assets/__tests__/cdk-synth-validation.test.ts'],

Impact: These tests will never run in CI. The PR description claims the tests pass, but they won't execute automatically.

Questions:

• Why are these tests excluded from the unit project?
• Should they be in a different test project (e.g., integ)?
• Is there a performance concern that requires exclusion?

The git history shows a commit f9fb8ab: "fix: exclude CDK synth tests from CI unit suite" which suggests this was intentional, but without context it's unclear if this is the right approach.

Action Required: Either:

1. Remove the exclusion so tests run in CI, OR
2. Add these tests to an appropriate test project, OR
3. Document why they should be excluded and how they'll be run

---

🔴 CRITICAL Issues

3. Weak Assertions - Not Validating Correctness

Many tests use Match.anyValue() which only checks that a resource exists, not that it's configured correctly:

Example from line 236:

template.hasResourceProperties(CFN_ECR_REPO, Match.anyValue());

Count: 17 instances of Match.anyValue() across the test file.

Why this matters: These tests won't catch misconfiguration bugs. For example:

• ECR repository with wrong lifecycle policy
• IAM role with incorrect trust policy
• Missing required properties

Recommendation:

// Instead of:
template.hasResourceProperties(CFN_ECR_REPO, Match.anyValue());

// Use specific assertions:
template.hasResourceProperties(CFN_ECR_REPO, {
  RepositoryName: Match.stringLikeRegexp('containeragent'),
  ImageScanningConfiguration: Match.objectLike({
    ScanOnPush: true
  })
});

---

4. Excessive Type Casting Bypasses Type Safety

Count: 35 uses of as never throughout the test file.

Example from line 204:

agents: [makeAgent('myagent')] as never,

Why this is problematic:

• Bypasses TypeScript's type checking completely
• Could hide real type mismatches between test data and actual spec types
• Makes tests less valuable for catching schema drift

Root cause: The helper functions return plain objects that may not match the exact Zod-validated types.

Recommendation:

// Option 1: Use proper types from the schema
import type { AgentEnvSpec } from '@aws/agentcore-cdk';

function makeAgent(name: string, overrides: Partial<AgentEnvSpec> = {}): AgentEnvSpec {
  return {
    type: 'AgentEnvironment',
    name,
    build: 'CodeZip',
    // ... rest of properties with proper typing
  };
}

// Option 2: If types are complex, at least use `as unknown as AgentEnvSpec[]`
// instead of `as never` to maintain some type safety

---

💡 SUGGESTIONS (Non-blocking)

5. Extract Test Utilities to Shared Module

The helper functions (makeAgent, makeMemory, makeEvaluator, etc.) could be valuable for other CDK-related tests.

Recommendation: Create src/assets/__tests__/helpers/cdk-test-utils.ts with these utilities.

---

6. Add Negative Test Cases

All current tests verify successful synthesis. Consider adding tests for:

• Invalid agent configurations that should fail
• Missing required fields
• Invalid memory strategy combinations
• Schema validation errors

Example:

it('fails to synthesize agent with missing codeLocation', () => {
  expect(() => {
    synthStack(baseSpec({
      agents: [{ type: 'AgentEnvironment', name: 'test' }] as never,
    }));
  }).toThrow();
});

---

7. Improve Stack Naming Stability

Line 82 uses Date.now() in stack names:

const stack = new AgentCoreStack(app, `TestStack${Date.now()}`, {

Issue: Could cause collisions if tests run very quickly or in parallel.

Recommendation:

// Use test name or a counter
const stack = new AgentCoreStack(app, `TestStack-${testName}`, {

---

8. Clarify Test Exclusion Purpose

The PR has three commits:

1. feat: add CDK synthesis validation tests (19 cases)
2. fix: exclude CDK synth tests from CI unit suite
3. fix: remove unused expect import

Commit #2 suggests the exclusion is intentional, but without the commit message or context, it's unclear:

• Is this due to performance concerns?
• Do these tests require special setup?
• Should they run in a dedicated CDK test suite?

Recommendation: Add a comment in the test file explaining why it's excluded and how/when it should be run.

---

✅ Positive Aspects

• ✅ Comprehensive coverage of resource types (agents, memories, credentials, evaluators, policy engines)
• ✅ Good test organization with clear sections
• ✅ Proper setup/teardown with beforeAll/afterAll
• ✅ No security issues (no secrets, safe temp dir usage)
• ✅ Edge case coverage (max name lengths, standalone resources)
• ✅ Clean helper functions reduce duplication

---

📋 Action Items

Must Fix Before Merge:

1. Add linked issue reference (Closes #XXX) to PR description
2. Resolve test exclusion: either remove exclusion, add to appropriate project, or document why excluded
3. Replace Match.anyValue() with specific assertions for key properties
4. Fix excessive as never casting with proper types

Recommended:

5. Extract test utilities to shared module
6. Add negative test cases for error scenarios
7. Use stable stack naming
8. Document test exclusion rationale

---

Verdict

Cannot approve until blocking issues #1 and #2 are resolved. The test coverage is good in principle, but the tests won't run in CI (issue #2) and the PR doesn't meet contribution guidelines (issue #1). Issues #3 and #4 significantly reduce the value of these tests and should be addressed to make them more robust.