Feat/orders test version

[jeastham1993]

What does this PR do?

Motivation

Testing Guidelines

Additional Notes

Types of Changes

• Bug fix
• New feature
• Breaking change
• Misc (docs, refactoring, dependency upgrade, etc.)

Check all that apply

• This PR's description is comprehensive
• This PR contains breaking changes that are documented in the description
• This PR introduces new APIs or parameters that are documented and unlikely to change in the foreseeable future
• This PR impacts documentation, and it has been updated (or a ticket has been logged)
• This PR's changes are covered by the automated tests
• This PR collects user input/sensitive content into Datadog

[claude]

Claude finished @jeastham1993's task —— View job

---

Pull Request Review

I've completed a comprehensive review of this PR. While the description is minimal, I've analyzed the 25 changed files across multiple services. Here's my detailed feedback:

Review Checklist:

• Read repository guidelines (CLAUDE.md)
• Review infrastructure changes (CDK/deployment)
• Analyze service-specific changes
• Check security considerations
• Evaluate performance impacts
• Assess test coverage
• Provide structured feedback and recommendations

---

Summary of Changes

This PR introduces significant changes across multiple microservices, including:

• Infrastructure: Updated GitHub workflows, CDK deployment scripts, and shared infrastructure
• Event Versioning: Added event deprecation and superseding mechanisms
• Dependencies: Updated multiple service dependencies and requirements
• Testing: Updated integration test frameworks (.NET 9.0)

Code Quality & Best Practices

✅ Strengths

• Good Event Versioning Strategy: The loyalty service implements proper event versioning with deprecation dates (src/loyalty-point-service/src/loyalty-api/adapters/eventBridgeEventPublisher.ts:56)
• Proper Error Handling: SQS batch processing includes comprehensive error handling with batch item failures
• Observability: Excellent use of semantic conventions for messaging spans and DataDog tracing
• Consistent Tagging: Infrastructure resources have proper tags for organization and cost tracking

⚠️ Areas for Improvement

1. Typo in Key Observability Code (src/loyalty-point-service/src/loyalty-api/adapters/handleOrderCompletedFunction.ts:67,70)

// Typo: "depreciation" should be "deprecation"
messageProcessingSpan?.setTag("messaging.message.depreciation_date", deprecationDate);
messageProcessingSpan.setTag("messaging.message.depreciated", true);

Recommendation: Fix the typo to deprecation_date and deprecated for consistency and searchability.

2. Inconsistent Environment Handling (src/order-service/cdk/OrdersServiceStack.cs:49-54)
The code throws an exception when CloudFront distribution lookup fails, but this might be expected in some environments.

catch (Exception ex)
{
    Console.WriteLine($"Could not import shared distribution: {ex.Message}");
    throw; // This might be too aggressive
}

Recommendation: Consider graceful degradation or environment-specific handling.

Security Considerations

✅ Good Security Practices

• Proper Secret Management: DataDog API keys are stored in AWS Secrets Manager, not environment variables in code
• IAM Roles: Uses proper OIDC federation for GitHub Actions authentication
• No Hard-coded Secrets: All sensitive values are externalized appropriately

ℹ️ Minor Security Notes

• JWT secret generation uses randomUUID() which is cryptographically secure
• All AWS resources follow least-privilege principles

Performance Considerations

✅ Performance Strengths

• Parallel Deployments: cdk-deploy.sh:15-47 runs service deployments in parallel, improving deployment speed
• Batch Processing: SQS event handling processes messages efficiently in batches
• Proper Resource Tagging: Enables cost optimization and resource management

⚠️ Performance Considerations

• Double Event Publishing: The loyalty service publishes both v1 and v2 events for backward compatibility, increasing EventBridge load
• Large Dependency Updates: Some services have significant dependency additions that should be monitored for bundle size impact

Test Coverage

✅ Testing Strengths

• Comprehensive Test Suite: Multiple test types including unit, integration, and end-to-end tests
• Modern Test Framework: Updated to .NET 9.0 with latest testing packages
• Cross-service Testing: End-to-end tests validate service interactions

⚠️ Testing Gaps

• Missing Test Updates: Many service changes don't include corresponding test updates
• Event Versioning Tests: No tests visible for the new event deprecation/superseding logic

Breaking Changes & Compatibility

✅ Good Backward Compatibility

• Event Versioning: Maintains v1 events while introducing v2, with proper deprecation dates
• Gradual Migration: supercededby field helps consumers understand migration path

Deployment & Infrastructure

✅ Infrastructure Improvements

• Multi-environment Support: Proper separation of dev/prod deployments
• Workflow Optimization: Added production deployment step to shared services workflow

⚠️ Deployment Concerns

• Missing Activity Service: cdk-deploy.sh:44-46 has the activity service deployment commented out - is this intentional?

Recommendations

High Priority

1. Fix Observability Typo: Correct "depreciation" → "deprecation" in messaging tags for consistency
2. Add Test Coverage: Include tests for event versioning and deprecation logic
3. Clarify Activity Service: Document why activity service deployment is disabled

Medium Priority

1. Improve Error Handling: Make CloudFront distribution import more resilient
2. Monitor Performance: Track impact of double event publishing on EventBridge costs
3. Update PR Description: Add meaningful description of changes and testing approach

Low Priority

1. Dependency Audit: Review large dependency additions for necessity and security
2. Consider Feature Flags: For gradual rollout of event versioning changes

Overall Assessment

This is a well-structured PR that introduces important event versioning capabilities while maintaining backward compatibility. The code quality is high with proper observability and error handling. The main concerns are a critical typo in observability tags and missing test coverage for new features.

Recommendation: ✅ Approve after addressing the observability typo and clarifying the activity service deployment status.

[claude]

Claude finished @jeastham1993's task —— View job

---

Pull Request Review

Review Checklist:

• Read repository guidelines (CLAUDE.md)
• Review infrastructure changes (CDK/deployment)
• Analyze service-specific changes
• Check security considerations
• Evaluate performance impacts
• Assess test coverage
• Provide structured feedback and recommendations

---

Summary of Changes

This PR introduces significant changes across multiple microservices, including:

• Infrastructure: Updated GitHub workflows, CDK deployment scripts, and shared infrastructure
• Event Versioning: Added event deprecation and superseding mechanisms
• Dependencies: Updated multiple service dependencies and requirements
• Testing: Updated integration test frameworks (.NET 9.0)

Code Quality & Best Practices

✅ Strengths

• Good Event Versioning Strategy: The loyalty service implements proper event versioning with deprecation dates (src/loyalty-point-service/src/loyalty-api/adapters/eventBridgeEventPublisher.ts:56)
• Proper Error Handling: SQS batch processing includes comprehensive error handling with batch item failures
• Observability: Excellent use of semantic conventions for messaging spans and DataDog tracing
• Consistent Tagging: Infrastructure resources have proper tags for organization and cost tracking

⚠️ Areas for Improvement

1. Typo in Key Observability Code (src/loyalty-point-service/src/loyalty-api/adapters/handleOrderCompletedFunction.ts:67,70)

// Typo: "depreciation" should be "deprecation"
messageProcessingSpan?.setTag("messaging.message.depreciation_date", deprecationDate);
messageProcessingSpan.setTag("messaging.message.depreciated", true);

Recommendation: Fix the typo to deprecation_date and deprecated for consistency and searchability.

2. Inconsistent Environment Handling (src/order-service/cdk/OrdersServiceStack.cs:49-54)
The code throws an exception when CloudFront distribution lookup fails, but this might be expected in some environments.

catch (Exception ex)
{
    Console.WriteLine($"Could not import shared distribution: {ex.Message}");
    throw; // This might be too aggressive
}

Recommendation: Consider graceful degradation or environment-specific handling.

Security Considerations

✅ Good Security Practices

• Proper Secret Management: DataDog API keys are stored in AWS Secrets Manager, not environment variables in code
• IAM Roles: Uses proper OIDC federation for GitHub Actions authentication
• No Hard-coded Secrets: All sensitive values are externalized appropriately

ℹ️ Minor Security Notes

• JWT secret generation uses randomUUID() which is cryptographically secure
• All AWS resources follow least-privilege principles

Performance Considerations

✅ Performance Strengths

• Parallel Deployments: cdk-deploy.sh:15-47 runs service deployments in parallel, improving deployment speed
• Batch Processing: SQS event handling processes messages efficiently in batches
• Proper Resource Tagging: Enables cost optimization and resource management

⚠️ Performance Considerations

• Double Event Publishing: The loyalty service publishes both v1 and v2 events for backward compatibility, increasing EventBridge load
• Large Dependency Updates: Some services have significant dependency additions that should be monitored for bundle size impact

Test Coverage

✅ Testing Strengths

• Comprehensive Test Suite: Multiple test types including unit, integration, and end-to-end tests
• Modern Test Framework: Updated to .NET 9.0 with latest testing packages
• Cross-service Testing: End-to-end tests validate service interactions

⚠️ Testing Gaps

• Missing Test Updates: Many service changes don't include corresponding test updates
• Event Versioning Tests: No tests visible for the new event deprecation/superseding logic

Breaking Changes & Compatibility

✅ Good Backward Compatibility

• Event Versioning: Maintains v1 events while introducing v2, with proper deprecation dates
• Gradual Migration: supercededby field helps consumers understand migration path

Deployment & Infrastructure

✅ Infrastructure Improvements

• Multi-environment Support: Proper separation of dev/prod deployments
• Workflow Optimization: Added production deployment step to shared services workflow

⚠️ Deployment Concerns

• Missing Activity Service: cdk-deploy.sh:44-46 has the activity service deployment commented out - is this intentional?

Recommendations

High Priority

1. Fix Observability Typo: Correct "depreciation" → "deprecation" in messaging tags for consistency
2. Add Test Coverage: Include tests for event versioning and deprecation logic
3. Clarify Activity Service: Document why activity service deployment is disabled

Medium Priority

1. Improve Error Handling: Make CloudFront distribution import more resilient
2. Monitor Performance: Track impact of double event publishing on EventBridge costs
3. Update PR Description: Add meaningful description of changes and testing approach

Low Priority

1. Dependency Audit: Review large dependency additions for necessity and security
2. Consider Feature Flags: For gradual rollout of event versioning changes

Overall Assessment

This is a well-structured PR that introduces important event versioning capabilities while maintaining backward compatibility. The code quality is high with proper observability and error handling. The main concerns are a critical typo in observability tags and missing test coverage for new features.

Recommendation: ✅ Approve after addressing the observability typo and clarifying the activity service deployment status.

[Copilot]

Pull Request Overview

This pull request adds standardized tagging across all CDK stacks and fixes several bugs. The changes include adding environment, project, service, team, and owner tags to all infrastructure stacks, updating API endpoint protocols from HTTP to HTTPS, fixing typos in tag names, and temporarily disabling activity-service deployment.

Key changes:

• Standardized resource tagging across TypeScript, Go, C#, Java, and Python CDK stacks
• Fixed CloudFront distribution endpoint protocols from http:// to https://
• Added event deprecation tracking in loyalty service

Reviewed Changes

Copilot reviewed 18 out of 25 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
src/user-management-service/bin/rust-datadog-sample.ts	Added standardized CDK tags
src/shared-infra/lib/shared-resources/shared-resources-stack.ts	Added S3 bucket for frontend application
src/shared-infra/bin/shared-resources.ts	Added standardized CDK tags
src/product-management-service/cdk/cdk.go	Added standardized CDK tags for Go stack
src/pricing-service/bin/pricing-service.ts	Added standardized CDK tags
src/order-service/cdk/Services/OrdersApi.cs	Fixed CloudFront endpoint protocol to HTTPS
src/order-service/cdk/Program.cs	Added standardized CDK tags and removed spacing inconsistencies
src/order-service/cdk/OrdersServiceStack.cs	Added CloudFront distribution lookup capability
src/inventory-service/cdk/src/main/java/com/cdk/inventory/api/InventoryApiContainer.java	Fixed CloudFront endpoint protocol to HTTPS
src/inventory-service/cdk/src/main/java/com/cdk/InventoryService.java	Added standardized CDK tags
src/frontend/src/admin/products/page.js	Added null-safe operator for pricingBrackets
src/frontend/package.json	Reordered dependencies and added amplify package
src/loyalty-point-service/src/loyalty-api/adapters/handleOrderCompletedFunction.ts	Added event deprecation tracking and removed unused imports
src/loyalty-point-service/lib/constructs/lambdaFunction.ts	Removed unused imports
src/loyalty-point-service/bin/loyalty-point-service.ts	Added standardized CDK tags
src/activity-service/lambda_requirements.txt	Added complete requirements file
cdk-deploy.sh	Temporarily disabled activity-service deployment
.github/workflows/deploy-shared-services.yml	Added production deployment step
package-lock.json files	Dependency metadata updates

Files not reviewed (2)

• src/frontend/package-lock.json: Language not supported
• src/order-mcp/package-lock.json: Language not supported

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Corrected spelling of 'depreciation' to 'deprecation' in tag name. The term 'deprecation' is the correct spelling when referring to marking something as outdated or obsolete, whereas 'depreciation' refers to financial asset value decrease.

Suggested change

	"messaging.message.depreciation_date",
	"messaging.message.deprecation_date",

[Copilot]

Corrected spelling of 'depreciated' to 'deprecated' in tag name. The term 'deprecated' is the correct spelling when referring to marking something as outdated or obsolete.

Suggested change

	messageProcessingSpan.setTag("messaging.message.depreciated", true );
	messageProcessingSpan.setTag("messaging.message.deprecated", true );

[Copilot]

Extra space before closing parenthesis. Remove the space before ')' for consistent formatting.

Suggested change

	messageProcessingSpan.setTag("messaging.message.depreciated", true );
	messageProcessingSpan.setTag("messaging.message.depreciated", true);

[Copilot]

Inconsistent null-safety: Line 66 uses optional chaining (?.) but line 70 does not. Since both operations occur within the same if block and messageProcessingSpan could be undefined, line 70 should also use optional chaining to prevent potential runtime errors.

[Copilot]

Inconsistent null-safety: Line 66 uses optional chaining (?.) but line 70 does not. Since both operations occur within the same if block and messageProcessingSpan could be undefined, line 70 should also use optional chaining to prevent potential runtime errors.

Suggested change

	messageProcessingSpan.setTag("messaging.message.depreciated", true );
	messageProcessingSpan?.setTag("messaging.message.depreciated", true );

[claude]

Claude finished @jeastham1993's task —— View job

---

I'll conduct a comprehensive review of this PR focusing on code quality, potential bugs, performance, security, and test coverage.

Review Checklist:

• Read repository guidelines (CLAUDE.md)
• Review infrastructure changes (CDK/deployment)
• Analyze service-specific changes
• Check security considerations
• Evaluate performance impacts
• Assess test coverage
• Provide structured feedback and recommendations

---

Summary of Changes

This PR introduces significant changes across multiple microservices, including:

• Infrastructure: Updated GitHub workflows, CDK deployment scripts, and shared infrastructure
• Event Versioning: Added event deprecation and superseding mechanisms in loyalty service
• Dependencies: Updated multiple service dependencies and requirements
• Testing: Updated integration test frameworks (.NET 9.0)
• Standardized Tagging: Added consistent resource tagging across all CDK stacks

Critical Issues Found

🚨 High Priority - Observability Bug (src/loyalty-point-service/src/loyalty-api/adapters/handleOrderCompletedFunction.ts:67,70)

// TYPO: "depreciation" should be "deprecation"
messageProcessingSpan?.setTag("messaging.message.depreciation_date", deprecationDate);
messageProcessingSpan.setTag("messaging.message.depreciated", true);

Impact: This typo affects observability metrics and could break monitoring dashboards/alerts that expect the correct spelling.

Fix Required: Change depreciation → deprecation and depreciated → deprecated

⚠️ Medium Priority - Error Handling Issue (src/order-service/cdk/OrdersServiceStack.cs:49-54)

The CloudFront distribution lookup throws an exception but might be expected in some environments:

catch (Exception ex)
{
    Console.WriteLine($"Could not import shared distribution: {ex.Message}");
    throw; // This might be too aggressive for all environments
}

⚠️ Medium Priority - Inconsistent Null Safety (src/loyalty-point-service/src/loyalty-api/adapters/handleOrderCompletedFunction.ts:66,70)

Line 66 uses optional chaining (?.) but line 70 doesn't, creating potential runtime errors:

messageProcessingSpan?.setTag(...);  // Line 66 - Safe
messageProcessingSpan.setTag(...);   // Line 70 - Unsafe

Security Analysis

✅ Good Security Practices

• Proper Secret Management: DataDog API keys are stored in AWS Secrets Manager, not hardcoded
• OIDC Authentication: GitHub Actions use proper OIDC federation instead of long-lived keys
• No Hardcoded Secrets: All sensitive values are externalized appropriately
• JWT Security: Uses cryptographically secure randomUUID() for JWT secrets

⚠️ Security Considerations

• CDK Auto-Approval: Deployment scripts use --require-approval never which bypasses security reviews for infrastructure changes
• Broad Permissions: Some IAM policies use "*" resources, though this appears to be for legitimate cross-service operations

Performance Analysis

✅ Performance Strengths

• Parallel Deployments: cdk-deploy.sh runs service deployments in parallel, improving deployment speed
• Efficient Batch Processing: SQS handlers process messages in batches with proper error handling
• Resource Tagging: Enables cost optimization and resource management

⚠️ Performance Concerns

• Double Event Publishing: Loyalty service publishes both v1 and v2 events for backward compatibility, doubling EventBridge load
• Infrastructure Scaling: ECS services are configured with fixed desired count (2), may need dynamic scaling
• CloudFront Distribution: All services create individual CloudFront distributions instead of sharing one

Test Coverage Assessment

✅ Testing Strengths

• Comprehensive Test Suites: Multiple test types including unit, integration, and end-to-end tests
• Modern Test Frameworks: Updated to .NET 9.0 with latest testing packages
• Cross-service Testing: End-to-end tests validate service interactions

⚠️ Testing Gaps

• Missing Test Updates: New event versioning features lack corresponding test coverage
• No Tests for Event Deprecation: The deprecation logic introduced isn't covered by tests
• Infrastructure Tests: CDK changes don't appear to have corresponding infrastructure tests

Code Quality & Best Practices

✅ Positive Aspects

• Consistent Tagging Strategy: All resources properly tagged for organization
• Good Error Handling: SQS batch processing includes comprehensive error handling
• Observability Excellence: Proper use of semantic conventions for tracing and metrics
• Event Versioning Strategy: Well-implemented backward compatibility with deprecation dates

Infrastructure & Deployment

✅ Improvements

• Multi-environment Support: Proper separation of dev/prod deployments
• Standardized Configuration: Consistent environment variables and tagging across services
• Workflow Optimization: Added production deployment capabilities

⚠️ Concerns

• Commented Out Service: Activity service deployment is disabled in cdk-deploy.sh:44-46 - is this intentional?
• HTTPS Endpoints Fixed: Good fix changing http:// to https:// for CloudFront endpoints, but this should be verified in testing

Recommendations

High Priority

1. Fix Observability Typos: Correct "depreciation" → "deprecation" in messaging tags for consistency and monitoring
2. Add Null Safety: Use optional chaining consistently in handleOrderCompletedFunction.ts:70
3. Add Test Coverage: Include tests for new event versioning and deprecation features

Medium Priority

1. Improve Error Handling: Make CloudFront distribution lookup more resilient to environment differences
2. Clarify Activity Service: Document why activity service deployment is commented out
3. Monitor Performance: Track EventBridge costs from double event publishing

Low Priority

1. Consider Resource Sharing: Evaluate sharing CloudFront distributions across services
2. Review Auto-Approval: Consider if --require-approval never is appropriate for all environments
3. Update PR Description: Add meaningful description of changes and testing approach

Overall Assessment

This is a well-structured PR that introduces important event versioning capabilities while maintaining backward compatibility. The code quality is generally high with good observability practices. The main concerns are the critical typo in observability tags and some missing test coverage for new features.

Recommendation: ✅ Approve after addressing the observability typo and null safety issues.

---

[claude]

Claude finished @jeastham1993's task —— View job

---

I'll conduct a comprehensive review of this PR focusing on code quality, potential bugs, performance, security, and test coverage.

Review Checklist:

• Read repository guidelines (CLAUDE.md)
• Review infrastructure changes (CDK/deployment)
• Analyze service-specific changes
• Check security considerations
• Evaluate performance impacts
• Assess test coverage
• Provide structured feedback and recommendations

---

Summary of Changes

This PR introduces significant changes across multiple microservices, including:

• Infrastructure: Updated GitHub workflows, CDK deployment scripts, and shared infrastructure
• Event Versioning: Added event deprecation and superseding mechanisms in loyalty service
• Dependencies: Updated multiple service dependencies and requirements
• Testing: Updated integration test frameworks (.NET 9.0)
• Standardized Tagging: Added consistent resource tagging across all CDK stacks

Critical Issues Found

🚨 High Priority - Observability Bug (src/loyalty-point-service/src/loyalty-api/adapters/handleOrderCompletedFunction.ts:67,70)

// TYPO: "depreciation" should be "deprecation"
messageProcessingSpan?.setTag("messaging.message.depreciation_date", deprecationDate);
messageProcessingSpan.setTag("messaging.message.depreciated", true);

Impact: This typo affects observability metrics and could break monitoring dashboards/alerts that expect the correct spelling.

Fix Required: Change depreciation → deprecation and depreciated → deprecated

⚠️ Medium Priority - Error Handling Issue (src/order-service/cdk/OrdersServiceStack.cs:49-54)

The CloudFront distribution lookup throws an exception but might be expected in some environments:

catch (Exception ex)
{
    Console.WriteLine($"Could not import shared distribution: {ex.Message}");
    throw; // This might be too aggressive for all environments
}

⚠️ Medium Priority - Inconsistent Null Safety (src/loyalty-point-service/src/loyalty-api/adapters/handleOrderCompletedFunction.ts:66,70)

Line 66 uses optional chaining (?.) but line 70 doesn't, creating potential runtime errors:

messageProcessingSpan?.setTag(...);  // Line 66 - Safe
messageProcessingSpan.setTag(...);   // Line 70 - Unsafe

Security Analysis

✅ Good Security Practices

• Proper Secret Management: DataDog API keys are stored in AWS Secrets Manager, not hardcoded
• OIDC Authentication: GitHub Actions use proper OIDC federation instead of long-lived keys
• No Hardcoded Secrets: All sensitive values are externalized appropriately
• JWT Security: Uses cryptographically secure randomUUID() for JWT secrets

⚠️ Security Considerations

• CDK Auto-Approval: Deployment scripts use --require-approval never which bypasses security reviews for infrastructure changes
• Broad Permissions: Some IAM policies use "*" resources, though this appears to be for legitimate cross-service operations

Performance Analysis

✅ Performance Strengths

• Parallel Deployments: cdk-deploy.sh runs service deployments in parallel, improving deployment speed
• Efficient Batch Processing: SQS handlers process messages in batches with proper error handling
• Resource Tagging: Enables cost optimization and resource management

⚠️ Performance Concerns

• Double Event Publishing: Loyalty service publishes both v1 and v2 events for backward compatibility, doubling EventBridge load
• Infrastructure Scaling: ECS services are configured with fixed desired count (2), may need dynamic scaling
• CloudFront Distribution: All services create individual CloudFront distributions instead of sharing one

Test Coverage Assessment

✅ Testing Strengths

• Comprehensive Test Suites: Multiple test types including unit, integration, and end-to-end tests
• Modern Test Frameworks: Updated to .NET 9.0 with latest testing packages
• Cross-service Testing: End-to-end tests validate service interactions

⚠️ Testing Gaps

• Missing Test Updates: New event versioning features lack corresponding test coverage
• No Tests for Event Deprecation: The deprecation logic introduced isn't covered by tests
• Infrastructure Tests: CDK changes don't appear to have corresponding infrastructure tests

Code Quality & Best Practices

✅ Positive Aspects

• Consistent Tagging Strategy: All resources properly tagged for organization
• Good Error Handling: SQS batch processing includes comprehensive error handling
• Observability Excellence: Proper use of semantic conventions for tracing and metrics
• Event Versioning Strategy: Well-implemented backward compatibility with deprecation dates

Infrastructure & Deployment

✅ Improvements

• Multi-environment Support: Proper separation of dev/prod deployments
• Standardized Configuration: Consistent environment variables and tagging across services
• Workflow Optimization: Added production deployment capabilities

⚠️ Concerns

• Commented Out Service: Activity service deployment is disabled in cdk-deploy.sh:44-46 - is this intentional?
• HTTPS Endpoints Fixed: Good fix changing http:// to https:// for CloudFront endpoints, but this should be verified in testing

Recommendations

High Priority

1. Fix Observability Typos: Correct "depreciation" → "deprecation" in messaging tags for consistency and monitoring
2. Add Null Safety: Use optional chaining consistently in handleOrderCompletedFunction.ts:70
3. Add Test Coverage: Include tests for new event versioning and deprecation features

Medium Priority

1. Improve Error Handling: Make CloudFront distribution lookup more resilient to environment differences
2. Clarify Activity Service: Document why activity service deployment is commented out
3. Monitor Performance: Track EventBridge costs from double event publishing

Low Priority

1. Consider Resource Sharing: Evaluate sharing CloudFront distributions across services
2. Review Auto-Approval: Consider if --require-approval never is appropriate for all environments
3. Update PR Description: Add meaningful description of changes and testing approach

Overall Assessment

This is a well-structured PR that introduces important event versioning capabilities while maintaining backward compatibility. The code quality is generally high with good observability practices. The main concerns are the critical typo in observability tags and some missing test coverage for new features.

Recommendation: ✅ Approve after addressing the observability typo and null safety issues.

---

[claude]

Claude Code is working…

I'll analyze this and get back to you.

View job run

[claude]

Claude finished @jeastham1993's task —— View job

---

I'll conduct a comprehensive review of this PR focusing on code quality, potential bugs, performance, security, and test coverage.

Review Checklist:

• Read repository guidelines (CLAUDE.md)
• Review infrastructure changes (CDK/deployment)
• Analyze service-specific changes
• Check security considerations
• Evaluate performance impacts
• Assess test coverage
• Provide structured feedback and recommendations

---

Summary of Changes

This PR introduces significant changes across multiple microservices, including:

• Infrastructure: Updated GitHub workflows, CDK deployment scripts, and shared infrastructure
• Event Versioning: Added event deprecation and superseding mechanisms in loyalty service
• Dependencies: Updated multiple service dependencies and requirements
• Testing: Updated integration test frameworks (.NET 9.0)
• Standardized Tagging: Added consistent resource tagging across all CDK stacks

Critical Issues Found

🚨 High Priority - Observability Bug (src/loyalty-point-service/src/loyalty-api/adapters/handleOrderCompletedFunction.ts:67,70)

// TYPO: "deprecation" should be "deprecation"
messageProcessingSpan?.setTag("messaging.message.deprecation_date", deprecationDate);
messageProcessingSpan.setTag("messaging.message.deprecated", true);

Impact: This typo affects observability metrics and could break monitoring dashboards/alerts that expect the correct spelling. The correct spelling is already used on line 67 but line 70 is inconsistent.

Fix Required: Change line 70 to use deprecated instead of depreciated

⚠️ Medium Priority - Inconsistent Null Safety (src/loyalty-point-service/src/loyalty-api/adapters/handleOrderCompletedFunction.ts:66,70)

Line 66 uses optional chaining (?.) but line 70 doesn't, creating potential runtime errors:

messageProcessingSpan?.setTag(...);  // Line 66 - Safe
messageProcessingSpan.setTag(...);   // Line 70 - Unsafe

⚠️ Medium Priority - Error Handling Issue (src/order-service/cdk/OrdersServiceStack.cs:49-54)

The CloudFront distribution lookup throws an exception but might be expected in some environments:

catch (Exception ex)
{
    Console.WriteLine($"Could not import shared distribution: {ex.Message}");
    throw; // This might be too aggressive for all environments
}

⚠️ Medium Priority - Comment Typo (src/loyalty-point-service/src/loyalty-api/adapters/eventBridgeEventPublisher.ts:41)

// Still publish V2 for backwards compatibility, with a depreciation date.

Should be "deprecation date" not "depreciation date"

Security Analysis

✅ Good Security Practices

• Proper Secret Management: DataDog API keys are stored in AWS Secrets Manager, not hardcoded
• OIDC Authentication: GitHub Actions use proper OIDC federation instead of long-lived keys
• No Hardcoded Secrets: All sensitive values are externalized appropriately
• JWT Security: Uses cryptographically secure randomUUID() for JWT secrets

⚠️ Security Considerations

• CDK Auto-Approval: Deployment scripts use --require-approval never which bypasses security reviews for infrastructure changes
• Broad Permissions: Some IAM policies use "*" resources, though this appears to be for legitimate cross-service operations

Performance Analysis

✅ Performance Strengths

• Parallel Deployments: cdk-deploy.sh runs service deployments in parallel, improving deployment speed
• Efficient Batch Processing: SQS handlers process messages in batches with proper error handling
• Resource Tagging: Enables cost optimization and resource management

⚠️ Performance Concerns

• Double Event Publishing: Loyalty service publishes both v1 and v2 events for backward compatibility, doubling EventBridge load
• Infrastructure Scaling: ECS services are configured with fixed desired count (2), may need dynamic scaling
• CloudFront Distribution: All services create individual CloudFront distributions instead of sharing one

Test Coverage Assessment

✅ Testing Strengths

• Comprehensive Test Suites: Multiple test types including unit, integration, and end-to-end tests
• Modern Test Frameworks: Updated to .NET 9.0 with latest testing packages
• Cross-service Testing: End-to-end tests validate service interactions

⚠️ Testing Gaps

• Missing Test Updates: New event versioning features lack corresponding test coverage
• No Tests for Event Deprecation: The deprecation logic introduced isn't covered by tests
• Infrastructure Tests: CDK changes don't appear to have corresponding infrastructure tests

Code Quality & Best Practices

✅ Positive Aspects

• Consistent Tagging Strategy: All resources properly tagged for organization
• Good Error Handling: SQS batch processing includes comprehensive error handling
• Observability Excellence: Proper use of semantic conventions for tracing and metrics
• Event Versioning Strategy: Well-implemented backward compatibility with deprecation dates
• Null Safety Improvements: Frontend code now uses null-safe operator (??) for pricingBrackets

Infrastructure & Deployment

✅ Improvements

• Multi-environment Support: Proper separation of dev/prod deployments
• Standardized Configuration: Consistent environment variables and tagging across services
• Workflow Optimization: Added production deployment capabilities
• HTTPS Endpoints Fixed: Good fix changing http:// to https:// for CloudFront endpoints

⚠️ Concerns

• Commented Out Service: Activity service deployment is disabled in cdk-deploy.sh:44-46 - is this intentional?
• Large Requirements Files: Activity service has massive requirements files that may impact cold start performance

Recommendations

High Priority

1. Fix Observability Typo: Correct "depreciated" → "deprecated" in messaging tags for consistency and monitoring
2. Add Null Safety: Use optional chaining consistently in handleOrderCompletedFunction.ts:70
3. Add Test Coverage: Include tests for new event versioning and deprecation features

Medium Priority

1. Improve Error Handling: Make CloudFront distribution lookup more resilient to environment differences
2. Clarify Activity Service: Document why activity service deployment is commented out
3. Monitor Performance: Track EventBridge costs from double event publishing
4. Fix Comment Typo: Update comment to use "deprecation" instead of "depreciation"

Low Priority

1. Consider Resource Sharing: Evaluate sharing CloudFront distributions across services
2. Review Auto-Approval: Consider if --require-approval never is appropriate for all environments
3. Update PR Description: Add meaningful description of changes and testing approach

Overall Assessment

This is a well-structured PR that introduces important event versioning capabilities while maintaining backward compatibility. The code quality is generally high with good observability practices. The main concerns are the critical typo in observability tags and some missing test coverage for new features.

Recommendation: ✅ Approve after addressing the observability typo and null safety issues.

---