Set up feature flag framework #1110
Conversation
WalkthroughAdds a feature-flag subsystem (HTTP helper, StatSig client, Lambda handlers, CloudFormation custom-resource base, CDK constructs/stacks, API endpoint and models, tests), updates privilege schema and encumbrance handling, and migrates Node.js SES usage to SESv2 plus related dependency updates and test adjustments. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant APIGW as API Gateway
participant Lambda as check_feature_flag Lambda
participant FFC as FeatureFlagClient (StatSig)
participant SM as SecretsManager
participant StatSig as StatSig Console/API
Client->>APIGW: POST /v1/flags/{flagId}/check {context}
APIGW->>Lambda: invoke
rect #eef6ff
note right of Lambda: module-level client cached\n(create_feature_flag_client at import)
Lambda->>SM: Get Statsig credentials (if not cached)
SM-->>Lambda: credentials
Lambda->>FFC: check_flag(flagId, context)
end
FFC->>StatSig: Console/API requests (GET/POST/PATCH)
StatSig-->>FFC: gate/rule data / enabled result
FFC-->>Lambda: FeatureFlagResult(enabled)
Lambda-->>APIGW: 200 {"enabled": bool}
APIGW-->>Client: response
sequenceDiagram
autonumber
participant CFN as CloudFormation
participant Handler as ManageFeatureFlagHandler
participant SM as SecretsManager
participant FFC as StatSigFeatureFlagClient
participant Console as StatSig Console/API
CFN->>Handler: Create/Update/Delete Custom::FeatureFlag (properties)
Handler->>SM: fetch console/server keys
SM-->>Handler: keys
Handler->>FFC: upsert_flag / delete_flag
FFC->>Console: GET gate -> POST/PATCH/DELETE rule(s)
Console-->>FFC: gate/rule result
FFC-->>Handler: gateId / status
Handler-->>CFN: PhysicalResourceId, Data (gateId) / None
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (3 warnings)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
landonshumway-ia
force-pushed
the
feat/feature-flag-framework
branch
from
1ad769b to
f38a112
Compare
landonshumway-ia
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
♻️ Duplicate comments (2)
backend/compact-connect/lambdas/python/cognito-backup/requirements-dev.txt (1)
7-84: Dependency updates look good.The dependency version bumps align with updates across other modules in this PR. The major version updates to cffi and cryptography should be verified (flagged in compact-configuration/requirements-dev.txt).
backend/compact-connect/lambdas/python/common/requirements-dev.in (1)
3-3: Cryptography major version update.The cryptography dependency update to 46.x is a major version bump. Verification of breaking changes and compatibility has been requested in the compact-configuration/requirements-dev.txt review.
🧹 Nitpick comments (4)
backend/multi-account/control-tower/requirements.txt (1)
15-18: Confirm CDK CLI + L1 impacts for 2.219.0 upgrade.Jumping to
aws-cdk-lib==2.219.0usually means the CDK CLI and any direct L1 usages need revalidation for new required props. Please double-check the repo’s pinned CLI/tooling version is at or above the library release date and rerun a targetedcdk synth/diffon the Control Tower stacks to catch schema shifts before landing. Based on learnings.backend/compact-connect/lambdas/python/common/cc_common/feature_flag_client.py (1)
46-119: Consider adding input validation for flag_name.The function doesn't validate the
flag_nameparameter. While the API will likely handle invalid names, adding basic validation (non-empty string, reasonable length) would catch errors earlier and improve debugging.Example validation:
def is_feature_enabled(flag_name: str, context: FeatureFlagContext | None = None, fail_default: bool = False) -> bool: if not flag_name or not isinstance(flag_name, str): logger.warning('Invalid flag_name provided', flag_name=flag_name) return fail_default # ... rest of implementationbackend/compact-connect/stacks/api_stack/v1_api/feature_flags.py (1)
39-53: Fix comment to reflect POST method.The note still mentions a “public GET endpoint,” but this block wires a POST. Please update the wording to avoid confusion.
backend/compact-connect/lambdas/python/feature-flag/tests/function/test_manage_feature_flag.py (1)
12-31: Drop the extra@mock_aws.
TstFunctionalready applies@mock_aws, so decorating the test class again re-initializes Moto per method and wipes secrets you seed insetUp. Remove the additional decorator to keep the mock state stable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (60)
backend/bin/compile_requirements.sh(1 hunks)backend/bin/run_python_tests.py(1 hunks)backend/bin/sync_deps.sh(1 hunks)backend/compact-connect/lambdas/python/cognito-backup/requirements-dev.txt(4 hunks)backend/compact-connect/lambdas/python/cognito-backup/requirements.txt(2 hunks)backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py(1 hunks)backend/compact-connect/lambdas/python/common/cc_common/data_model/provider_record_util.py(3 hunks)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/military_affiliation/api.py(0 hunks)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/privilege/record.py(2 hunks)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/api.py(1 hunks)backend/compact-connect/lambdas/python/common/cc_common/feature_flag_client.py(1 hunks)backend/compact-connect/lambdas/python/common/common_test/test_data_generator.py(1 hunks)backend/compact-connect/lambdas/python/common/requirements-dev.in(1 hunks)backend/compact-connect/lambdas/python/common/requirements-dev.txt(4 hunks)backend/compact-connect/lambdas/python/common/requirements.in(1 hunks)backend/compact-connect/lambdas/python/common/requirements.txt(2 hunks)backend/compact-connect/lambdas/python/common/tests/__init__.py(1 hunks)backend/compact-connect/lambdas/python/common/tests/unit/test_feature_flag_client.py(1 hunks)backend/compact-connect/lambdas/python/common/tests/unit/test_provider_record_util.py(8 hunks)backend/compact-connect/lambdas/python/compact-configuration/requirements-dev.txt(4 hunks)backend/compact-connect/lambdas/python/custom-resources/requirements-dev.txt(4 hunks)backend/compact-connect/lambdas/python/data-events/handlers/encumbrance_events.py(1 hunks)backend/compact-connect/lambdas/python/data-events/requirements-dev.txt(4 hunks)backend/compact-connect/lambdas/python/data-events/tests/function/test_encumbrance_events.py(1 hunks)backend/compact-connect/lambdas/python/disaster-recovery/requirements-dev.txt(4 hunks)backend/compact-connect/lambdas/python/feature-flag/custom_resource_handler.py(1 hunks)backend/compact-connect/lambdas/python/feature-flag/feature_flag_client.py(1 hunks)backend/compact-connect/lambdas/python/feature-flag/handlers/check_feature_flag.py(1 hunks)backend/compact-connect/lambdas/python/feature-flag/handlers/manage_feature_flag.py(1 hunks)backend/compact-connect/lambdas/python/feature-flag/requirements-dev.in(1 hunks)backend/compact-connect/lambdas/python/feature-flag/requirements-dev.txt(1 hunks)backend/compact-connect/lambdas/python/feature-flag/requirements.in(1 hunks)backend/compact-connect/lambdas/python/feature-flag/requirements.txt(1 hunks)backend/compact-connect/lambdas/python/feature-flag/tests/__init__.py(1 hunks)backend/compact-connect/lambdas/python/feature-flag/tests/function/__init__.py(1 hunks)backend/compact-connect/lambdas/python/feature-flag/tests/function/test_check_feature_flag.py(1 hunks)backend/compact-connect/lambdas/python/feature-flag/tests/function/test_manage_feature_flag.py(1 hunks)backend/compact-connect/lambdas/python/feature-flag/tests/function/test_statsig_client.py(1 hunks)backend/compact-connect/lambdas/python/provider-data-v1/handlers/registration.py(1 hunks)backend/compact-connect/lambdas/python/provider-data-v1/requirements-dev.txt(4 hunks)backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_encumbrance.py(1 hunks)backend/compact-connect/lambdas/python/purchases/requirements-dev.txt(4 hunks)backend/compact-connect/lambdas/python/staff-user-pre-token/requirements-dev.txt(4 hunks)backend/compact-connect/lambdas/python/staff-users/requirements-dev.txt(4 hunks)backend/compact-connect/pipeline/backend_stage.py(2 hunks)backend/compact-connect/requirements-dev.txt(3 hunks)backend/compact-connect/requirements.txt(3 hunks)backend/compact-connect/stacks/api_lambda_stack/__init__.py(2 hunks)backend/compact-connect/stacks/api_lambda_stack/feature_flags.py(1 hunks)backend/compact-connect/stacks/api_stack/v1_api/api.py(3 hunks)backend/compact-connect/stacks/api_stack/v1_api/api_model.py(1 hunks)backend/compact-connect/stacks/api_stack/v1_api/feature_flags.py(1 hunks)backend/compact-connect/stacks/feature_flag_stack/__init__.py(1 hunks)backend/compact-connect/stacks/feature_flag_stack/feature_flag_resource.py(1 hunks)backend/multi-account/backups/requirements-dev.txt(3 hunks)backend/multi-account/backups/requirements.txt(1 hunks)backend/multi-account/control-tower/requirements-dev.txt(1 hunks)backend/multi-account/control-tower/requirements.txt(1 hunks)backend/multi-account/log-aggregation/requirements-dev.txt(1 hunks)backend/multi-account/log-aggregation/requirements.txt(1 hunks)
💤 Files with no reviewable changes (1)
- backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/military_affiliation/api.py
🧰 Additional context used
🧠 Learnings (7)
📚 Learning: 2025-07-22T03:36:17.137Z
Learnt from: landonshumway-ia
PR: csg-org/CompactConnect#907
File: backend/compact-connect/lambdas/python/purchases/requirements-dev.txt:15-0
Timestamp: 2025-07-22T03:36:17.137Z
Learning: In CompactConnect, requirements-dev.txt files for Lambda functions are used exclusively for running tests and development, not for actual Lambda runtime environments. Concerns about runtime compatibility (like OpenSSL versions) don't apply to these development dependency files.
Applied to files:
backend/compact-connect/lambdas/python/feature-flag/requirements-dev.txtbackend/compact-connect/lambdas/python/common/requirements.txtbackend/compact-connect/lambdas/python/staff-users/requirements-dev.txtbackend/compact-connect/lambdas/python/purchases/requirements-dev.txtbackend/compact-connect/lambdas/python/data-events/requirements-dev.txtbackend/compact-connect/lambdas/python/cognito-backup/requirements-dev.txtbackend/compact-connect/lambdas/python/compact-configuration/requirements-dev.txtbackend/compact-connect/lambdas/python/custom-resources/requirements-dev.txtbackend/bin/sync_deps.sh
📚 Learning: 2025-07-22T03:52:25.934Z
Learnt from: landonshumway-ia
PR: csg-org/CompactConnect#907
File: backend/compact-connect/lambdas/python/provider-data-v1/requirements.txt:2-2
Timestamp: 2025-07-22T03:52:25.934Z
Learning: In CompactConnect, the Python version used by pip-compile to generate requirements.txt files (shown in the header comment) is separate from the actual Lambda runtime environment. Dependencies are installed by a Python 3.12 container during the CI/CD pipeline, ensuring runtime compatibility regardless of the Python version used for pip-compile dependency resolution.
Applied to files:
backend/compact-connect/lambdas/python/feature-flag/requirements-dev.txtbackend/compact-connect/lambdas/python/common/requirements.txtbackend/compact-connect/lambdas/python/common/requirements-dev.txtbackend/bin/compile_requirements.shbackend/compact-connect/lambdas/python/staff-user-pre-token/requirements-dev.txtbackend/compact-connect/requirements.txtbackend/compact-connect/lambdas/python/cognito-backup/requirements-dev.txtbackend/compact-connect/lambdas/python/cognito-backup/requirements.txtbackend/compact-connect/lambdas/python/compact-configuration/requirements-dev.txtbackend/compact-connect/lambdas/python/custom-resources/requirements-dev.txtbackend/bin/sync_deps.sh
📚 Learning: 2025-08-12T19:49:24.999Z
Learnt from: landonshumway-ia
PR: csg-org/CompactConnect#1001
File: backend/compact-connect/lambdas/python/disaster-recovery/requirements.in:1-1
Timestamp: 2025-08-12T19:49:24.999Z
Learning: In CompactConnect disaster-recovery Lambda functions, runtime dependencies like boto3, aws-lambda-powertools, and botocore are provided by lambda layers at deploy time rather than being specified in requirements.in files. The requirements.in file intentionally contains only a comment explaining this approach.
Applied to files:
backend/compact-connect/lambdas/python/feature-flag/requirements-dev.txtbackend/compact-connect/lambdas/python/common/requirements.txtbackend/compact-connect/lambdas/python/disaster-recovery/requirements-dev.txtbackend/compact-connect/lambdas/python/cognito-backup/requirements-dev.txtbackend/compact-connect/lambdas/python/cognito-backup/requirements.txtbackend/bin/sync_deps.sh
📚 Learning: 2025-09-11T20:06:40.709Z
Learnt from: ChiefStief
PR: csg-org/CompactConnect#1036
File: backend/compact-connect/lambdas/python/common/cc_common/data_model/provider_record_util.py:370-383
Timestamp: 2025-09-11T20:06:40.709Z
Learning: The EncumbranceDetailsSchema in backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/privilege/record.py does not contain a 'note' field. It only has clinicalPrivilegeActionCategory (String, optional), adverseActionId (UUID, required), and licenseJurisdiction (Jurisdiction, optional).
Applied to files:
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/privilege/record.py
📚 Learning: 2025-09-11T20:06:40.709Z
Learnt from: ChiefStief
PR: csg-org/CompactConnect#1036
File: backend/compact-connect/lambdas/python/common/cc_common/data_model/provider_record_util.py:370-383
Timestamp: 2025-09-11T20:06:40.709Z
Learning: The EncumbranceDetailsSchema in backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/privilege/record.py does not contain a 'note' field. It only has clinicalPrivilegeActionCategory (String, optional), adverseActionId (UUID, required), and licenseJurisdiction (Jurisdiction, optional). When working with encumbrance notes, use encumbranceDetails['clinicalPrivilegeActionCategory'], not encumbranceDetails['note'].
Applied to files:
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/privilege/record.pybackend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_encumbrance.py
📚 Learning: 2025-08-12T19:49:48.235Z
Learnt from: landonshumway-ia
PR: csg-org/CompactConnect#1001
File: backend/compact-connect/lambdas/python/disaster-recovery/requirements.txt:1-6
Timestamp: 2025-08-12T19:49:48.235Z
Learning: The disaster-recovery Lambda functions in CompactConnect get their aws-lambda-powertools dependency from the shared lambda layer rather than individual requirements.txt files, which is why their requirements.txt files can be empty or header-only.
Applied to files:
backend/compact-connect/lambdas/python/common/requirements.txtbackend/compact-connect/lambdas/python/disaster-recovery/requirements-dev.txtbackend/compact-connect/lambdas/python/cognito-backup/requirements.txtbackend/bin/sync_deps.sh
📚 Learning: 2025-07-21T20:40:56.491Z
Learnt from: landonshumway-ia
PR: csg-org/CompactConnect#907
File: backend/compact-connect/lambdas/python/common/requirements.txt:7-0
Timestamp: 2025-07-21T20:40:56.491Z
Learning: In CompactConnect, there is only one lambda layer in use for Python lambdas, and this single layer manages the versions of aws-lambda-powertools, boto3, and botocore dependencies. This eliminates concerns about version skew across multiple lambda layers since all Python lambdas share the same dependency management through this single layer.
Applied to files:
backend/compact-connect/lambdas/python/cognito-backup/requirements.txt
🧬 Code graph analysis (19)
backend/compact-connect/pipeline/backend_stage.py (1)
backend/compact-connect/stacks/feature_flag_stack/__init__.py (1)
FeatureFlagStack(74-101)
backend/compact-connect/stacks/api_stack/v1_api/api.py (3)
backend/compact-connect/stacks/api_stack/v1_api/feature_flags.py (1)
FeatureFlagsApi(11-53)backend/compact-connect/common_constructs/stack.py (2)
common_env_vars(81-89)api_domain_name(109-112)backend/compact-connect/common_constructs/frontend_app_config_utility.py (1)
api_domain_name(220-222)
backend/compact-connect/lambdas/python/feature-flag/tests/function/__init__.py (2)
backend/compact-connect/lambdas/python/feature-flag/tests/__init__.py (1)
TstLambdas(9-85)backend/compact-connect/lambdas/python/common/common_test/test_data_generator.py (1)
TestDataGenerator(21-655)
backend/compact-connect/stacks/feature_flag_stack/feature_flag_resource.py (2)
backend/compact-connect/common_constructs/stack.py (1)
Stack(18-89)backend/compact-connect/common_constructs/python_function.py (1)
PythonFunction(21-155)
backend/compact-connect/lambdas/python/common/tests/unit/test_feature_flag_client.py (2)
backend/compact-connect/lambdas/python/common/tests/__init__.py (1)
TstLambdas(9-109)backend/compact-connect/lambdas/python/common/cc_common/feature_flag_client.py (3)
is_feature_enabled(46-119)FeatureFlagContext(17-43)to_dict(32-43)
backend/compact-connect/lambdas/python/feature-flag/handlers/manage_feature_flag.py (2)
backend/compact-connect/lambdas/python/feature-flag/custom_resource_handler.py (5)
CustomResourceHandler(18-124)CustomResourceResponse(10-15)on_create(90-99)on_update(102-111)on_delete(114-124)backend/compact-connect/lambdas/python/feature-flag/feature_flag_client.py (6)
FeatureFlagException(157-158)StatSigFeatureFlagClient(203-617)upsert_flag(87-101)upsert_flag(351-385)delete_flag(114-124)delete_flag(533-577)
backend/compact-connect/lambdas/python/feature-flag/tests/__init__.py (1)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/common.py (1)
update(182-203)
backend/compact-connect/lambdas/python/common/cc_common/data_model/provider_record_util.py (1)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/common.py (1)
UpdateCategory(287-299)
backend/compact-connect/lambdas/python/feature-flag/tests/function/test_statsig_client.py (3)
backend/compact-connect/lambdas/python/feature-flag/feature_flag_client.py (11)
FeatureFlagException(157-158)FeatureFlagRequest(20-24)FeatureFlagValidationException(161-162)StatSigFeatureFlagClient(203-617)validate_request(63-74)check_flag(77-84)check_flag(278-308)upsert_flag(87-101)upsert_flag(351-385)delete_flag(114-124)delete_flag(533-577)backend/compact-connect/lambdas/python/feature-flag/tests/function/__init__.py (2)
TstFunction(14-23)setUp(17-23)backend/compact-connect/lambdas/python/feature-flag/tests/function/test_check_feature_flag.py (2)
setUp(14-27)_setup_mock_statsig(47-57)
backend/compact-connect/lambdas/python/common/tests/unit/test_provider_record_util.py (1)
backend/compact-connect/lambdas/python/common/cc_common/data_model/provider_record_util.py (2)
ProviderRecordUtility(53-407)construct_simplified_privilege_history_object(369-407)
backend/compact-connect/stacks/feature_flag_stack/__init__.py (2)
backend/compact-connect/common_constructs/stack.py (1)
AppStack(92-141)backend/compact-connect/stacks/feature_flag_stack/feature_flag_resource.py (2)
FeatureFlagEnvironmentName(21-26)FeatureFlagResource(30-168)
backend/compact-connect/stacks/api_lambda_stack/feature_flags.py (3)
backend/compact-connect/common_constructs/python_function.py (1)
PythonFunction(21-155)backend/compact-connect/common_constructs/stack.py (2)
Stack(18-89)common_env_vars(81-89)backend/compact-connect/stacks/persistent_stack/__init__.py (1)
PersistentStack(38-510)
backend/compact-connect/stacks/api_stack/v1_api/feature_flags.py (2)
backend/compact-connect/stacks/api_lambda_stack/__init__.py (1)
ApiLambdaStack(13-44)backend/compact-connect/stacks/api_stack/v1_api/api_model.py (2)
check_feature_flag_request_model(2708-2748)check_feature_flag_response_model(2751-2770)
backend/compact-connect/lambdas/python/feature-flag/handlers/check_feature_flag.py (3)
backend/compact-connect/lambdas/python/common/cc_common/exceptions.py (2)
CCInternalException(44-45)CCInvalidRequestException(7-8)backend/compact-connect/lambdas/python/common/cc_common/utils.py (1)
api_handler(90-212)backend/compact-connect/lambdas/python/feature-flag/feature_flag_client.py (6)
FeatureFlagRequest(20-24)FeatureFlagValidationException(161-162)create_feature_flag_client(620-631)validate_request(63-74)check_flag(77-84)check_flag(278-308)
backend/compact-connect/stacks/api_lambda_stack/__init__.py (1)
backend/compact-connect/stacks/api_lambda_stack/feature_flags.py (1)
FeatureFlagsLambdas(13-64)
backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_encumbrance.py (2)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/privilege/__init__.py (2)
PrivilegeUpdateData(91-162)encumbranceDetails(151-155)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/common.py (1)
from_database_record(124-139)
backend/compact-connect/lambdas/python/feature-flag/custom_resource_handler.py (1)
backend/compact-connect/lambdas/python/feature-flag/handlers/manage_feature_flag.py (3)
on_create(24-61)on_update(63-70)on_delete(72-105)
backend/compact-connect/lambdas/python/feature-flag/tests/function/test_manage_feature_flag.py (3)
backend/compact-connect/lambdas/python/feature-flag/tests/function/test_statsig_client.py (2)
setUp(26-35)create_mock_secrets_manager(37-41)backend/compact-connect/lambdas/python/feature-flag/handlers/manage_feature_flag.py (3)
ManageFeatureFlagHandler(15-105)on_create(24-61)on_delete(72-105)backend/compact-connect/lambdas/python/feature-flag/feature_flag_client.py (4)
upsert_flag(87-101)upsert_flag(351-385)delete_flag(114-124)delete_flag(533-577)
backend/compact-connect/lambdas/python/feature-flag/tests/function/test_check_feature_flag.py (3)
backend/compact-connect/lambdas/python/feature-flag/tests/function/__init__.py (2)
TstFunction(14-23)setUp(17-23)backend/compact-connect/lambdas/python/common/common_test/test_data_generator.py (1)
generate_test_api_event(33-61)backend/compact-connect/lambdas/python/feature-flag/handlers/check_feature_flag.py (1)
check_feature_flag(14-47)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: TestPython
🔇 Additional comments (27)
backend/multi-account/control-tower/requirements-dev.txt (1)
15-15: Pytest patch bump looks good.8.4.2 is a patch release so adopting it here makes sense.
backend/multi-account/log-aggregation/requirements-dev.txt (1)
15-15: Patch-level pytest bump looks goodPatch upgrade to 8.4.2 keeps us current and shouldn’t introduce breaking changes; no further action needed.
backend/compact-connect/lambdas/python/provider-data-v1/handlers/registration.py (1)
218-218: LGTM! Consistent metric emission added.This change aligns the CCNotFoundException error path with the existing pattern throughout the file, where all failed registration attempts emit the
REGISTRATION_ATTEMPT_METRIC_NAMEmetric with value 0. This improves observability consistency.backend/compact-connect/lambdas/python/common/tests/__init__.py (1)
16-16: LGTM!The addition of
API_BASE_URLto the test environment setup is appropriate and aligns with the new feature-flag HTTP integration requirements.backend/bin/run_python_tests.py (1)
31-31: LGTM!The addition of the feature-flag test directory to
TEST_DIRSis correct and consistent with the existing pattern.backend/compact-connect/lambdas/python/feature-flag/requirements-dev.in (1)
1-1: LGTM!The moto[dynamodb] dependency with version constraint
>=5.0.12, <6is appropriate for development and testing of feature-flag functionality with DynamoDB mocking.backend/compact-connect/lambdas/python/common/common_test/test_data_generator.py (1)
359-361: LGTM!The addition of a trailing comma in the function signature is a minor formatting improvement that follows Python best practices.
backend/bin/compile_requirements.sh (1)
23-24: LGTM!The addition of pip-compile commands for the feature-flag module follows the established pattern and uses the correct flags.
backend/bin/sync_deps.sh (1)
27-28: LGTM!The addition of feature-flag requirement files to the pip-sync command is correct and follows the established pattern.
backend/compact-connect/lambdas/python/cognito-backup/requirements.txt (1)
7-29: LGTM!The dependency version updates (aws-lambda-powertools, boto3, botocore, s3transfer) are routine maintenance with incremental version bumps. Based on learnings, these dependencies are managed through a single lambda layer, ensuring consistency across Python lambdas.
backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py (1)
2744-2746: LGTM!The reformatting of the return statement improves readability without changing behavior.
backend/compact-connect/requirements-dev.txt (1)
19-19: LGTM! Routine dependency updates.The version bumps (click, coverage, pyparsing, pytest, pytest-cov, ruff) are routine maintenance with no breaking changes.
Also applies to: 21-21, 73-73, 79-79, 83-83, 93-93
backend/compact-connect/lambdas/python/common/requirements-dev.txt (1)
7-7: LGTM! Routine dependency updates.The version bumps (boto3, botocore, cffi, cryptography, markupsafe, moto, pycparser, pyyaml, s3transfer, xmltodict) are routine maintenance with no breaking changes.
Also applies to: 9-9, 16-16, 20-23, 36-36, 40-40, 44-44, 51-51, 62-62, 74-74
backend/compact-connect/lambdas/python/common/cc_common/data_model/provider_record_util.py (1)
247-250: LGTM! Formatting improvements.The indentation adjustments and conditional collapse improve readability without changing behavior.
Also applies to: 369-371, 395-396
backend/compact-connect/lambdas/python/feature-flag/requirements-dev.txt (1)
1-69: LGTM! New development dependencies for feature-flag module.The pinned versions align with other modules and provide standard testing/mocking tools.
backend/compact-connect/lambdas/python/provider-data-v1/requirements-dev.txt (1)
7-7: LGTM! Routine dependency updates.The version bumps (boto3, botocore, cffi, cryptography, markupsafe, moto, pycparser, pyyaml, s3transfer, xmltodict) are routine maintenance with no breaking changes.
Also applies to: 9-9, 16-16, 20-20, 34-34, 38-38, 42-42, 49-49, 60-60, 72-72
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/privilege/record.py (2)
54-64: LGTM! New schema for encumbrance details.The EncumbranceDetailsSchema extends the privilege data model to capture encumbrance metadata (category, adverse action ID, optional license jurisdiction) without breaking existing APIs.
66-66: LGTM! Schema registration integration.The registration decorator enables dynamic schema lookup for 'privilege' records, aligning with the base schema registry pattern.
backend/compact-connect/lambdas/python/common/tests/unit/test_provider_record_util.py (1)
882-882: LGTM! Test formatting improvements.The indentation and line-wrapping adjustments improve readability without changing test behavior.
Also applies to: 897-898, 906-907, 960-960, 975-976, 984-985, 1039-1039, 1054-1055, 1062-1063, 1070-1070
backend/compact-connect/lambdas/python/disaster-recovery/requirements-dev.txt (1)
7-7: LGTM! Routine dependency updates.The version bumps (boto3, botocore, cffi, cryptography, markupsafe, moto, pycparser, pyyaml, s3transfer, xmltodict) are routine maintenance with no breaking changes.
Also applies to: 9-9, 16-16, 20-20, 32-32, 36-36, 40-40, 46-46, 57-57, 69-69
backend/compact-connect/lambdas/python/feature-flag/tests/__init__.py (1)
1-86: LGTM! Clean test setup with appropriate environment configuration.The test base class properly configures the environment and mocks needed for feature-flag tests. The use of
_Config()with a noqa comment is appropriate for test setup where direct instantiation is required.backend/compact-connect/requirements.txt (1)
15-26: Verify CDK L1 construct changes.The CDK library upgrade from 2.213.0 to 2.219.0 includes several incremental CloudFormation resource schema updates. Per the CDK release cycle, L1 constructs may have new required properties or attribute changes.
Run
cdk synthand review the diff to ensure no unexpected CloudFormation template changes, especially for L1 constructs.backend/compact-connect/lambdas/python/common/cc_common/feature_flag_client.py (1)
95-100: Verify that the 5-second timeout is appropriate.The hard-coded 5-second timeout for an internal API call might be too aggressive, especially under load or during cold starts. Consider making this configurable via environment variables or increasing it to 10-15 seconds for resilience.
Is the 5-second timeout sufficient for internal API calls in your environment, considering cold starts and potential network latency?
backend/compact-connect/lambdas/python/feature-flag/requirements.in (1)
2-2: statsig-python-core security and provenance verified. Package metadata confirms Statsig authoring, official homepage, documentation, and GitHub repository; no known PIP security advisories found.backend/compact-connect/lambdas/python/compact-configuration/requirements-dev.txt (2)
20-20: Verify compatibility with cryptography 46.0.2
This upgrade drops Python 3.7 support (requires ≥3.8), deprecates OpenSSL <3.0 (wheels now bundle OpenSSL 3.5.x), removes a legacy CSR API, and deprecates macOS x86_64 and 32-bit Windows wheels. Ensure your test environment (cffi, moto, etc.) works, validate SSH key loading, PKCS#12 and X.509 parsing, and confirm your OpenSSL linkage. No new security advisories in 46.x.
16-16: cffi 2.0.0 is compatible with cryptography 46.x
cffi 2.0.0 only drops Python ≤ 3.8 and adds 3.14 support; cryptography 46.x requires cffi ≥ 1.14, so no breaking changes in tests.backend/compact-connect/lambdas/python/common/requirements.in (1)
4-4: cryptography 46.x is compatible with AWS Lambda Python 3.12. Build your layer with manylinux_2_34/AL2023-compatible wheels (or use the official Lambda Python 3.12 image) to avoid GLIBC mismatches; aws-lambda-powertools v3 and boto3 1.34.x work without issues.
backend/compact-connect/lambdas/python/common/cc_common/feature_flag_client.py
Outdated
Show resolved
Hide resolved
backend/compact-connect/lambdas/python/feature-flag/handlers/check_feature_flag.py
Outdated
Show resolved
Hide resolved
backend/compact-connect/lambdas/python/feature-flag/tests/function/test_check_feature_flag.py
Show resolved
Hide resolved
...act-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_encumbrance.py
Show resolved
Hide resolved
backend/compact-connect/stacks/feature_flag_stack/feature_flag_resource.py
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
backend/compact-connect/lambdas/python/feature-flag/feature_flag_client.py (3)
19-34: Consider consistent naming conventions across data classes.
FeatureFlagRequestuses camelCase (flagName) whileFeatureFlagResultuses snake_case (flag_name). While the camelCase inFeatureFlagRequestmatches the external API contract, the inconsistency between these closely related classes could cause confusion.
126-154: AWS coupling in abstract base class limits portability.The
_get_secretmethod implements AWS Secrets Manager logic directly in the abstract base class. This prevents truly provider-agnostic implementations, contradicting the stated goal of supporting "different feature flag providers (StatSig, LaunchDarkly, etc.)."Consider making secret retrieval abstract or using dependency injection to maintain true provider independence.
351-385: Document the rule preservation behavior.Lines 379-383 preserve existing environment rules to avoid overwriting manual changes. This means
auto_enablehas no effect if the rule already exists, which could surprise users expecting the flag to be enabled.Consider adding a docstring note clarifying that
auto_enableonly applies when creating a new rule, not when updating existing rules.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/compact-connect/lambdas/python/common/cc_common/feature_flag_client.py(1 hunks)backend/compact-connect/lambdas/python/feature-flag/feature_flag_client.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/compact-connect/lambdas/python/common/cc_common/feature_flag_client.py
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: TestPython
🔇 Additional comments (17)
backend/compact-connect/lambdas/python/feature-flag/feature_flag_client.py (17)
1-17: LGTM!Imports are well-organized and the ruff noqa directive appropriately allows StatSig's camelCase naming conventions.
36-201: LGTM!Schema design follows proper inheritance patterns with appropriate validation rules. The use of
load_default=dictfor optional fields prevents issues with mutable defaults.
157-163: LGTM!Clean exception hierarchy following Python best practices.
167-180: LGTM!Constants are well-organized. The hard-coded
STATSIG_API_BASE_URLandSTATSIG_API_VERSIONare acceptable for this use case, though making them configurable via environment variables could provide additional flexibility if needed in the future.
211-245: LGTM with awareness of cold start impact.Secret retrieval and StatSig initialization occur synchronously in
__init__, which will add latency to Lambda cold starts. This is acceptable given the need for credentials before any operations, and the error handling properly propagates exceptions.
247-263: LGTM!Initialization is properly idempotent and handles errors appropriately. The blocking
.wait()is necessary to ensure the client is ready before use.
265-276: LGTM!User creation logic is clean with appropriate fallback for missing userId.
278-308: LGTM with minor redundancy.The empty
flagNamecheck at line 286 is redundant since the schema already validatesLength(1, 100). However, this defensive check is harmless and may provide clearer error messages.
310-349: LGTM!Console API request handling is well-structured with appropriate timeout and error handling. The 30-second timeout balances responsiveness with allowing time for API operations.
387-428: LGTM!Gate creation logic is well-structured with appropriate handling of status codes and error truncation for safe logging.
430-441: LGTM!Clean implementation of rule lookup.
443-460: LGTM!Attribute-to-condition conversion is well-implemented with appropriate type validation. The hard-coded
'any'operator is a reasonable default for matching custom attributes.
497-511: LGTM!Correct handling of PATCH success codes (200 and 204).
513-531: LGTM with potential optimization opportunity.The implementation fetches all gates and filters client-side. This is acceptable for typical usage but could be optimized if StatSig's API supports server-side filtering.
577-599: LGTM!Rule removal logic is correctly implemented with proper detection of whether the rule was present.
601-615: LGTM with Lambda lifecycle awareness.Resource cleanup in
__del__ensures event logs are flushed to StatSig. Note that in Lambda environments with container reuse,__del__may not be called between invocations. Consider explicitly calling_shutdown()in handler cleanup if needed.
618-629: LGTM!Clean factory function providing a good extensibility point for adding additional feature flag providers in the future.
backend/compact-connect/lambdas/python/feature-flag/feature_flag_client.py
Outdated
Show resolved
Hide resolved
backend/compact-connect/lambdas/python/feature-flag/feature_flag_client.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
backend/compact-connect/lambdas/python/feature-flag/tests/function/test_check_feature_flag.py (1)
62-84: Critical: Test expectations don't match handler return shape.The handler
check_feature_flagreturns{'enabled': <bool>}directly (seebackend/compact-connect/lambdas/python/feature-flag/handlers/check_feature_flag.pylines 13-46), but this test asserts an API Gateway envelope format withstatusCodeand JSONbody. These tests will fail.Update the test to assert against the handler's actual return value:
# Call the handler result = check_feature_flag(event, self.mock_context) - # Verify the API Gateway response format - self.assertEqual(result['statusCode'], 200) - - # Parse and verify the JSON body - response_body = json.loads(result['body']) - self.assertEqual({'enabled': True}, response_body) + # Verify the handler response + self.assertEqual(result, {'enabled': True})Apply the same fix to the other two tests in this file.
🧹 Nitpick comments (2)
backend/compact-connect/lambdas/python/feature-flag/tests/function/test_check_feature_flag.py (2)
22-30: Consider using the parent class helper method.The related test file
test_statsig_client.pyusesself.create_mock_secrets_manager()to create the secrets client. Using the same helper method here would improve consistency across the test suite.Apply this diff to use the helper method:
- # Set up mock secrets manager with StatSig credentials - secrets_client = boto3.client('secretsmanager', region_name='us-east-1') - secrets_client.create_secret( + # Set up mock secrets manager with StatSig credentials + secrets_client = self.create_mock_secrets_manager() + secrets_client.create_secret( Name='compact-connect/env/test/statsig/credentials', SecretString=json.dumps({ 'serverKey': 'test-server-key-123', 'consoleKey': 'test-console-key-456' }), )
50-60: Consider extracting shared test helper.This
_setup_mock_statsighelper is duplicated intest_statsig_client.py(lines 42-52). While test isolation is valuable, extracting this to a shared test utility class in the test package would reduce duplication and ensure consistency in how Statsig mocks are configured across tests.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/compact-connect/lambdas/python/feature-flag/tests/function/test_check_feature_flag.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
backend/compact-connect/lambdas/python/feature-flag/tests/function/test_check_feature_flag.py (4)
backend/compact-connect/lambdas/python/feature-flag/tests/function/__init__.py (2)
TstFunction(14-23)setUp(17-23)backend/compact-connect/lambdas/python/feature-flag/tests/function/test_statsig_client.py (2)
setUp(26-35)_setup_mock_statsig(43-53)backend/compact-connect/lambdas/python/common/common_test/test_data_generator.py (1)
generate_test_api_event(33-61)backend/compact-connect/lambdas/python/feature-flag/handlers/check_feature_flag.py (1)
check_feature_flag(14-47)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: TestPython
🔇 Additional comments (2)
backend/compact-connect/lambdas/python/feature-flag/tests/function/test_check_feature_flag.py (2)
32-42: Good practice for test isolation.The module cleanup in tearDown ensures that the handler and its dependencies are freshly imported in each test, preventing cross-test contamination from cached module-level state.
62-62: feature_flag_client.Statsig patch target is correct
Statsig is imported and used in thefeature_flag_client.pymodule at the lambda root, so@patch('feature_flag_client.Statsig')correctly mocks the constructor.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
backend/compact-connect/stacks/feature_flag_stack/feature_flag_resource.py (1)
150-163: Critical:autoEnablecomparison bug still present.The issue flagged in the previous review remains unresolved. Line 151 compares a string
environment_namedirectly against a list ofFeatureFlagEnvironmentNameenum instances. Since strings never equal enum instances in Python,environment_name in auto_enable_envsalways evaluates toFalse, causing feature flags to never auto-enable in any environment.Apply this diff to fix the type mismatch:
# Build custom resource properties - properties = {'flagName': flag_name, 'autoEnable': environment_name in auto_enable_envs} + # Convert enum values to strings for comparison + auto_enable_values = {env.value for env in auto_enable_envs} + properties = { + 'flagName': flag_name, + 'autoEnable': environment_name in auto_enable_values, + }backend/compact-connect/lambdas/python/feature-flag/tests/function/test_check_feature_flag.py (1)
75-81: Return shape mismatch breaks all assertions.
check_feature_flagcurrently returns{'enabled': <bool>}(see backend/compact-connect/lambdas/python/feature-flag/handlers/check_feature_flag.py Line 41), so indexing intoresult['statusCode']will raise and every test will fail. Either wrap the handler with the decorator that emits the API Gateway envelope or assert directly on the plain dict (and update the other tests in this file likewise).
🧹 Nitpick comments (1)
backend/compact-connect/stacks/feature_flag_stack/feature_flag_resource.py (1)
21-26: LGTM with minor note.The enum definition is correct and provides type safety for environment names. The comment on line 25 about adding sandbox environments is now outdated since
SANDBOXis already defined on line 26.Consider removing the outdated comment:
- # add sandbox environment names here if needed SANDBOX = 'sandbox'
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
backend/compact-connect/lambdas/python/feature-flag/feature_flag_client.py(1 hunks)backend/compact-connect/lambdas/python/feature-flag/tests/function/test_check_feature_flag.py(1 hunks)backend/compact-connect/stacks/api_stack/v1_api/api.py(3 hunks)backend/compact-connect/stacks/feature_flag_stack/feature_flag_resource.py(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-18T21:59:25.029Z
Learnt from: landonshumway-ia
PR: csg-org/CompactConnect#1014
File: backend/compact-connect/stacks/api_stack/v1_api/api.py:31-36
Timestamp: 2025-08-18T21:59:25.029Z
Learning: The CompactConnect codebase has two separate V1Api implementations: one for the internal API stack (backend/compact-connect/stacks/api_stack/v1_api/api.py) and one for the state API stack (backend/compact-connect/stacks/state_api_stack/v1_api/api.py). These are distinct classes with different constructor signatures and purposes. Changes to one V1Api implementation do not necessarily affect the other.
Applied to files:
backend/compact-connect/stacks/api_stack/v1_api/api.py
🧬 Code graph analysis (3)
backend/compact-connect/stacks/api_stack/v1_api/api.py (3)
backend/compact-connect/stacks/api_stack/v1_api/feature_flags.py (1)
FeatureFlagsApi(11-53)backend/compact-connect/lambdas/python/common/cc_common/config.py (1)
api_base_url(312-317)backend/compact-connect/common_constructs/stack.py (2)
api_domain_name(109-112)common_env_vars(81-89)
backend/compact-connect/lambdas/python/feature-flag/tests/function/test_check_feature_flag.py (4)
backend/compact-connect/lambdas/python/feature-flag/tests/function/__init__.py (2)
TstFunction(14-23)setUp(17-23)backend/compact-connect/lambdas/python/feature-flag/tests/function/test_statsig_client.py (2)
setUp(26-35)_setup_mock_statsig(43-53)backend/compact-connect/lambdas/python/common/common_test/test_data_generator.py (1)
generate_test_api_event(33-61)backend/compact-connect/lambdas/python/feature-flag/handlers/check_feature_flag.py (1)
check_feature_flag(14-47)
backend/compact-connect/stacks/feature_flag_stack/feature_flag_resource.py (2)
backend/compact-connect/common_constructs/stack.py (1)
Stack(18-89)backend/compact-connect/common_constructs/python_function.py (1)
PythonFunction(21-155)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: TestPython
🔇 Additional comments (9)
backend/compact-connect/stacks/api_stack/v1_api/api.py (3)
19-19: LGTM!The import is correctly placed and necessary for the feature flags API integration below.
48-52: LGTM! Previous concern addressed.The conditional logic correctly handles the case where
persistent_stack.api_domain_nameisNoneby falling back toself.api.url, preventing the invalidhttps://NoneURL issue flagged in the previous review.
132-138: LGTM!The feature flags API integration follows the established pattern for adding new API resources in this file. The resource creation and
FeatureFlagsApiinitialization are clean and consistent with the existing codebase structure.backend/compact-connect/stacks/feature_flag_stack/feature_flag_resource.py (6)
1-19: LGTM!Imports and module documentation are well-organized and appropriate for the construct's purpose.
29-36: LGTM!The class structure correctly implements
IGrantablefor IAM permission management and follows CDK construct conventions.
62-72: LGTM!Lambda function configuration is appropriate for a CloudFormation custom resource handler, with adequate timeout and memory allocation.
74-83: LGTM!IAM permissions are appropriately scoped to the specific secret with the wildcard suffix correctly handling Secrets Manager's version identifiers.
85-148: LGTM!Provider configuration and NagSuppressions are appropriate for a CloudFormation custom resource with well-justified security exception rationales.
165-168: LGTM!The
grant_principalproperty correctly implements theIGrantableinterface by delegating to the underlying Lambda function.
backend/compact-connect/stacks/feature_flag_stack/feature_flag_resource.py
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
backend/compact-connect/stacks/feature_flag_stack/feature_flag_resource.py (1)
150-155: Fix the autoEnable comparison logic.The expression
environment_name in auto_enable_envswill always evaluate toFalsebecause it compares a string against a list of enum objects. Even thoughFeatureFlagEnvironmentNameis aStrEnum, theinoperator checks object identity, not string values.This breaks the feature flag auto-enable functionality—flags will never auto-enable in any environment.
Note: This issue was flagged in a previous review and marked as addressed in commit a24de78, but the fix is not present in the current code.
Apply this diff to fix the comparison:
- properties = {'flagName': flag_name, 'autoEnable': environment_name in auto_enable_envs} + normalized_env = environment_name.strip().lower() + auto_enable_targets = {env.value for env in auto_enable_envs} + properties = { + 'flagName': flag_name, + 'autoEnable': normalized_env in auto_enable_targets, + }
🧹 Nitpick comments (2)
backend/compact-connect/stacks/feature_flag_stack/feature_flag_resource.py (2)
21-27: Update comment to reflect SANDBOX is already included.The comment on line 25 suggests adding sandbox environment names, but
SANDBOXis already defined on line 26.Apply this diff to clarify the comment:
class FeatureFlagEnvironmentName(StrEnum): TEST = 'test' BETA = 'beta' PROD = 'prod' - # add sandbox environment names here if needed SANDBOX = 'sandbox' + # add additional environment names here if needed
58-60: Basic validation is adequate, consider enum validation as enhancement.The validation ensures required parameters are provided. For stricter type safety, you could optionally validate
environment_nameagainstFeatureFlagEnvironmentNameenum values to catch configuration errors early.If desired, apply this diff to add enum validation:
- if not flag_name or not environment_name: - raise ValueError('flag_name and environment_name are required') + if not flag_name: + raise ValueError('flag_name is required') + + if not environment_name: + raise ValueError('environment_name is required') + + # Validate environment_name against enum values + valid_env_names = {env.value for env in FeatureFlagEnvironmentName} + if environment_name not in valid_env_names: + raise ValueError( + f'environment_name must be one of {valid_env_names}, got: {environment_name}' + )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_encumbrance.py(2 hunks)backend/compact-connect/stacks/feature_flag_stack/feature_flag_resource.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_encumbrance.py (2)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/privilege/__init__.py (2)
PrivilegeUpdateData(91-162)encumbranceDetails(151-155)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/common.py (1)
from_database_record(124-139)
backend/compact-connect/stacks/feature_flag_stack/feature_flag_resource.py (2)
backend/compact-connect/common_constructs/stack.py (1)
Stack(18-89)backend/compact-connect/common_constructs/python_function.py (1)
PythonFunction(21-155)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: TestPython
🔇 Additional comments (10)
backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_encumbrance.py (1)
159-170: Fix correctly addresses the previous review concern.The test now properly validates the actual persisted
adverseActionIdrather than masking it with a hard-coded default. By loading the data from the database record first (line 159) and using the loaded value in the expected data (line 169), the assertion on line 174-177 confirms the handler persisted the correct value.backend/compact-connect/stacks/feature_flag_stack/feature_flag_resource.py (9)
1-7: LGTM!Clear and concise module documentation.
8-19: LGTM!Imports are well-organized and appropriate for the construct's functionality.
29-56: LGTM!Clean construct definition with appropriate inheritance and clear parameter documentation.
62-72: LGTM!Lambda configuration is appropriate for a custom resource handler managing external API calls.
74-83: LGTM!Secrets Manager permissions are appropriately scoped to environment-specific credentials with necessary version wildcard.
85-88: LGTM!Standard custom resource provider configuration with appropriate log retention for deploy-time operations.
90-148: LGTM!NagSuppressions are comprehensive and well-justified for the custom resource provider pattern. Each suppression includes clear reasoning.
157-163: LGTM!CustomResource creation follows standard CDK patterns with appropriate resource type and configuration.
165-168: LGTM!Proper implementation of the
IGrantableinterface by delegating to the underlying Lambda function's grant principal.
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
backend/compact-connect/lambdas/python/purchases/requirements-dev.txt (1)
16-41: Invalid cffi pin blocks installs
cffi==2.0.0has never been released. Any dev setup will fail resolving this lockfile. Please keep the pin on a real 1.x version (e.g., 1.17.1) and re-run pip-compile to refresh transitive pins likepycparser.-cffi==2.0.0 +cffi==1.17.1 ... -pycparser==2.23 +pycparser==2.22backend/compact-connect/lambdas/python/compact-configuration/requirements-dev.txt (1)
16-41: Invalid cffi pin blocks installs
pipcannot resolvecffi==2.0.0because that version doesn’t exist. Dev installs will fail, stopping tests. Please revert to a shipped 1.x release (e.g., 1.17.1) and re-run pip-compile so dependent pins stay consistent.-cffi==2.0.0 +cffi==1.17.1 ... -pycparser==2.23 +pycparser==2.22backend/compact-connect/lambdas/python/cognito-backup/requirements-dev.txt (1)
21-51: Invalid cffi pin blocks installsBumping to
cffi==2.0.0makes the dev requirements unusable—no such version exists on PyPI. Please restore a valid 1.x pin (e.g., 1.17.1) and re-run pip-compile sopycparserand related transitive pins line up.-cffi==2.0.0 +cffi==1.17.1 ... -pycparser==2.23 +pycparser==2.22backend/compact-connect/lambdas/python/common/requirements-dev.txt (1)
16-45: Invalid cffi pin blocks installs
cffi==2.0.0isn’t published, so creating the shared dev environment fails immediately. Please pin back to a real 1.x release (e.g., 1.17.1) and re-run pip-compile so the regenerated lock also updatespycparseraccordingly.-cffi==2.0.0 +cffi==1.17.1 ... -pycparser==2.23 +pycparser==2.22
♻️ Duplicate comments (2)
backend/multi-account/backups/requirements-dev.txt (1)
60-61: Restore s3transfer pin to boto3-supported range.
boto3==1.40.44still declaress3transfer>=0.10.0,<0.11.0. Pinnings3transfer==0.14.0breaks dependency resolution. Please drop to0.10.x(or bump boto3 to a release that allows>=0.14). Otherwisepipwill refuse to install this requirements set.backend/compact-connect/lambdas/python/data-events/requirements-dev.txt (1)
16-41: Invalid cffi pin blocks installs
cffi==2.0.0is not published, sopip install -r requirements-dev.txterrors out before tests can run. Please pin to an existing 1.x release (e.g., 1.17.1) and re-run pip-compile so hashes stay in sync.-cffi==2.0.0 +cffi==1.17.1 ... -pycparser==2.23 +pycparser==2.22
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (30)
backend/compact-connect/lambdas/python/cognito-backup/requirements-dev.txt(4 hunks)backend/compact-connect/lambdas/python/cognito-backup/requirements.txt(2 hunks)backend/compact-connect/lambdas/python/common/requirements-dev.txt(4 hunks)backend/compact-connect/lambdas/python/common/requirements.txt(2 hunks)backend/compact-connect/lambdas/python/compact-configuration/requirements-dev.txt(4 hunks)backend/compact-connect/lambdas/python/compact-configuration/requirements.txt(1 hunks)backend/compact-connect/lambdas/python/custom-resources/requirements-dev.txt(4 hunks)backend/compact-connect/lambdas/python/custom-resources/requirements.txt(1 hunks)backend/compact-connect/lambdas/python/data-events/requirements-dev.txt(4 hunks)backend/compact-connect/lambdas/python/data-events/requirements.txt(1 hunks)backend/compact-connect/lambdas/python/disaster-recovery/requirements-dev.txt(4 hunks)backend/compact-connect/lambdas/python/disaster-recovery/requirements.txt(1 hunks)backend/compact-connect/lambdas/python/feature-flag/requirements-dev.txt(1 hunks)backend/compact-connect/lambdas/python/feature-flag/requirements.txt(1 hunks)backend/compact-connect/lambdas/python/provider-data-v1/requirements-dev.txt(4 hunks)backend/compact-connect/lambdas/python/provider-data-v1/requirements.txt(1 hunks)backend/compact-connect/lambdas/python/purchases/requirements-dev.txt(4 hunks)backend/compact-connect/lambdas/python/purchases/requirements.txt(1 hunks)backend/compact-connect/lambdas/python/staff-user-pre-token/requirements-dev.txt(4 hunks)backend/compact-connect/lambdas/python/staff-user-pre-token/requirements.txt(1 hunks)backend/compact-connect/lambdas/python/staff-users/requirements-dev.txt(4 hunks)backend/compact-connect/lambdas/python/staff-users/requirements.txt(1 hunks)backend/compact-connect/requirements-dev.txt(5 hunks)backend/compact-connect/requirements.txt(4 hunks)backend/multi-account/backups/requirements-dev.txt(3 hunks)backend/multi-account/backups/requirements.txt(2 hunks)backend/multi-account/control-tower/requirements-dev.txt(2 hunks)backend/multi-account/control-tower/requirements.txt(2 hunks)backend/multi-account/log-aggregation/requirements-dev.txt(2 hunks)backend/multi-account/log-aggregation/requirements.txt(2 hunks)
✅ Files skipped from review due to trivial changes (3)
- backend/compact-connect/lambdas/python/custom-resources/requirements.txt
- backend/compact-connect/lambdas/python/data-events/requirements.txt
- backend/compact-connect/lambdas/python/staff-user-pre-token/requirements.txt
🚧 Files skipped from review as they are similar to previous changes (5)
- backend/compact-connect/lambdas/python/common/requirements.txt
- backend/compact-connect/lambdas/python/staff-user-pre-token/requirements-dev.txt
- backend/compact-connect/lambdas/python/feature-flag/requirements.txt
- backend/multi-account/log-aggregation/requirements.txt
- backend/compact-connect/requirements.txt
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-07-22T03:52:25.934Z
Learnt from: landonshumway-ia
PR: csg-org/CompactConnect#907
File: backend/compact-connect/lambdas/python/provider-data-v1/requirements.txt:2-2
Timestamp: 2025-07-22T03:52:25.934Z
Learning: In CompactConnect, the Python version used by pip-compile to generate requirements.txt files (shown in the header comment) is separate from the actual Lambda runtime environment. Dependencies are installed by a Python 3.12 container during the CI/CD pipeline, ensuring runtime compatibility regardless of the Python version used for pip-compile dependency resolution.
Applied to files:
backend/compact-connect/lambdas/python/feature-flag/requirements-dev.txtbackend/compact-connect/lambdas/python/provider-data-v1/requirements.txtbackend/compact-connect/lambdas/python/compact-configuration/requirements-dev.txtbackend/compact-connect/lambdas/python/purchases/requirements-dev.txtbackend/compact-connect/lambdas/python/purchases/requirements.txtbackend/compact-connect/lambdas/python/cognito-backup/requirements.txtbackend/compact-connect/lambdas/python/compact-configuration/requirements.txtbackend/compact-connect/lambdas/python/disaster-recovery/requirements.txtbackend/compact-connect/lambdas/python/common/requirements-dev.txtbackend/compact-connect/lambdas/python/custom-resources/requirements-dev.txtbackend/compact-connect/lambdas/python/provider-data-v1/requirements-dev.txtbackend/compact-connect/lambdas/python/staff-users/requirements.txtbackend/compact-connect/requirements-dev.txt
📚 Learning: 2025-07-22T03:36:17.137Z
Learnt from: landonshumway-ia
PR: csg-org/CompactConnect#907
File: backend/compact-connect/lambdas/python/purchases/requirements-dev.txt:15-0
Timestamp: 2025-07-22T03:36:17.137Z
Learning: In CompactConnect, requirements-dev.txt files for Lambda functions are used exclusively for running tests and development, not for actual Lambda runtime environments. Concerns about runtime compatibility (like OpenSSL versions) don't apply to these development dependency files.
Applied to files:
backend/compact-connect/lambdas/python/feature-flag/requirements-dev.txtbackend/compact-connect/lambdas/python/compact-configuration/requirements-dev.txtbackend/compact-connect/lambdas/python/provider-data-v1/requirements-dev.txtbackend/compact-connect/requirements-dev.txt
📚 Learning: 2025-08-12T19:49:24.999Z
Learnt from: landonshumway-ia
PR: csg-org/CompactConnect#1001
File: backend/compact-connect/lambdas/python/disaster-recovery/requirements.in:1-1
Timestamp: 2025-08-12T19:49:24.999Z
Learning: In CompactConnect disaster-recovery Lambda functions, runtime dependencies like boto3, aws-lambda-powertools, and botocore are provided by lambda layers at deploy time rather than being specified in requirements.in files. The requirements.in file intentionally contains only a comment explaining this approach.
Applied to files:
backend/compact-connect/lambdas/python/feature-flag/requirements-dev.txtbackend/compact-connect/lambdas/python/cognito-backup/requirements.txtbackend/compact-connect/lambdas/python/disaster-recovery/requirements-dev.txtbackend/compact-connect/lambdas/python/disaster-recovery/requirements.txtbackend/compact-connect/lambdas/python/cognito-backup/requirements-dev.txt
📚 Learning: 2025-08-21T02:51:28.199Z
Learnt from: landonshumway-ia
PR: csg-org/CompactConnect#1014
File: backend/compact-connect/lambdas/python/common/requirements.in:4-4
Timestamp: 2025-08-21T02:51:28.199Z
Learning: In CompactConnect, the purchases lambda contains requests as a transitive dependency from the Authorize.net SDK, which is automatically resolved by pip-compile. This should not be manually removed even when requests is also available in the common layer, as it's managed automatically by the dependency resolver.
Applied to files:
backend/compact-connect/lambdas/python/purchases/requirements.txt
📚 Learning: 2025-08-12T19:49:48.235Z
Learnt from: landonshumway-ia
PR: csg-org/CompactConnect#1001
File: backend/compact-connect/lambdas/python/disaster-recovery/requirements.txt:1-6
Timestamp: 2025-08-12T19:49:48.235Z
Learning: The disaster-recovery Lambda functions in CompactConnect get their aws-lambda-powertools dependency from the shared lambda layer rather than individual requirements.txt files, which is why their requirements.txt files can be empty or header-only.
Applied to files:
backend/compact-connect/lambdas/python/disaster-recovery/requirements.txt
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: TestPython
🔇 Additional comments (3)
backend/compact-connect/lambdas/python/compact-configuration/requirements.txt (1)
5-5: Autogen header update looks consistent.Updated pip-compile flags align with the broader tooling changes and need no further action.
backend/compact-connect/lambdas/python/staff-users/requirements.txt (1)
5-5: Header update looks good.The expanded pip-compile command mirrors the repo-wide tooling change and keeps the autogenerated note accurate.
backend/compact-connect/lambdas/python/staff-users/requirements-dev.txt (1)
16-17: Verify thecffi==2.0.0pin
Automated PyPI check failed due to SSL certificate errors. Please confirm in a properly configured environment thatcffi==2.0.0is published and installable; adjust the pin if it’s not available.
backend/compact-connect/lambdas/python/feature-flag/requirements-dev.txt
Show resolved
Hide resolved
backend/compact-connect/lambdas/python/purchases/requirements.txt
Outdated
Show resolved
Hide resolved
jusdino
left a comment
jusdino
left a comment
There was a problem hiding this comment.
Excellent work. This looks like a solid framework for our needs. I've got a handful of suggestions:
backend/compact-connect/lambdas/python/common/cc_common/feature_flag_client.py
Outdated
Show resolved
Hide resolved
backend/compact-connect/lambdas/python/common/cc_common/feature_flag_client.py
Show resolved
Hide resolved
backend/compact-connect/lambdas/python/common/cc_common/feature_flag_client.py
Show resolved
Hide resolved
backend/compact-connect/lambdas/python/common/cc_common/feature_flag_client.py
Outdated
Show resolved
Hide resolved
backend/compact-connect/lambdas/python/feature-flag/feature_flag_client.py
Outdated
Show resolved
Hide resolved
backend/compact-connect/stacks/feature_flag_stack/feature_flag_resource.py
Outdated
Show resolved
Hide resolved
backend/compact-connect/stacks/feature_flag_stack/feature_flag_resource.py
Outdated
Show resolved
Hide resolved
backend/compact-connect/stacks/feature_flag_stack/feature_flag_resource.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (4)
backend/compact-connect/lambdas/python/common/cc_common/feature_flag_client.py (1)
78-92: Consider adding retry logic for transient failures.The synchronous POST request lacks retry logic for transient network issues or 5xx errors. For improved resilience, consider using
requestswith aRetryadapter or a lightweight retry decorator.Example with
urllib3.util.Retry:from requests.adapters import HTTPAdapter from urllib3.util import Retry # At module level or in a helper def _get_session_with_retries(): session = requests.Session() retries = Retry( total=3, backoff_factor=0.3, status_forcelist=[500, 502, 503, 504], allowed_methods=["POST"] ) adapter = HTTPAdapter(max_retries=retries) session.mount("https://", adapter) session.mount("http://", adapter) return session # In is_feature_enabled session = _get_session_with_retries() response = session.post( endpoint_url, json=payload, timeout=5, headers={'Content-Type': 'application/json'}, )backend/compact-connect/lambdas/python/feature-flag/feature_flag_client.py (2)
134-136: Unnecessary session creation.Creating a
boto3.session.Session()and then callingsession.client()is redundant. You can directly useboto3.client('secretsmanager'), which internally creates a session with default region resolution from the Lambda environment.Apply this diff:
- # Create a Secrets Manager client - session = boto3.session.Session() - client = session.client(service_name='secretsmanager') + # Create a Secrets Manager client + client = boto3.client('secretsmanager')
331-342: Simplify request handling with the json parameter.The manual
json.dumps(data)and if/elif chain can be simplified by usingrequests.request()with thejsonkwarg, which automatically serializes and sets the correct Content-Type header.Apply this diff:
try: - if method == 'GET': - response = requests.get(url, headers=headers, timeout=30) - elif method == 'POST': - response = requests.post(url, headers=headers, data=json.dumps(data), timeout=30) - elif method == 'PATCH': - response = requests.patch(url, headers=headers, data=json.dumps(data), timeout=30) - elif method == 'DELETE': - response = requests.delete(url, headers=headers, timeout=30) - else: - raise ValueError(f'Unsupported HTTP method: {method}') - - return response + response = requests.request( + method=method, + url=url, + headers=headers, + json=data if method in ['POST', 'PATCH'] else None, + timeout=30 + ) + response.raise_for_status() + return responseNote: Adding
raise_for_status()provides automatic error handling for 4xx/5xx responses.backend/compact-connect/stacks/feature_flag_stack/feature_flag_resource.py (1)
54-55: Consider validating environment_name against enum values.While the current validation checks for presence, it doesn't verify that
environment_nameis a valid environment value. Invalid environment names would only surface at runtime when used in auto-enable checks.Apply this diff to add validation:
if not flag_name or not environment_name: raise ValueError('flag_name and environment_name are required') + + valid_env_names = {env.value for env in FeatureFlagEnvironmentName} + if environment_name not in valid_env_names: + raise ValueError( + f'environment_name must be one of {valid_env_names}, got: {environment_name}' + )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
backend/compact-connect/lambdas/python/common/cc_common/feature_flag_client.py(1 hunks)backend/compact-connect/lambdas/python/feature-flag/feature_flag_client.py(1 hunks)backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_encumbrance.py(2 hunks)backend/compact-connect/stacks/api_lambda_stack/__init__.py(2 hunks)backend/compact-connect/stacks/api_stack/v1_api/api.py(3 hunks)backend/compact-connect/stacks/api_stack/v1_api/feature_flags.py(1 hunks)backend/compact-connect/stacks/feature_flag_stack/__init__.py(1 hunks)backend/compact-connect/stacks/feature_flag_stack/feature_flag_resource.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- backend/compact-connect/stacks/api_stack/v1_api/feature_flags.py
- backend/compact-connect/stacks/api_lambda_stack/init.py
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-08-18T21:59:25.029Z
Learnt from: landonshumway-ia
PR: csg-org/CompactConnect#1014
File: backend/compact-connect/stacks/api_stack/v1_api/api.py:31-36
Timestamp: 2025-08-18T21:59:25.029Z
Learning: The CompactConnect codebase has two separate V1Api implementations: one for the internal API stack (backend/compact-connect/stacks/api_stack/v1_api/api.py) and one for the state API stack (backend/compact-connect/stacks/state_api_stack/v1_api/api.py). These are distinct classes with different constructor signatures and purposes. Changes to one V1Api implementation do not necessarily affect the other.
Applied to files:
backend/compact-connect/stacks/api_stack/v1_api/api.py
📚 Learning: 2025-09-11T20:06:40.709Z
Learnt from: ChiefStief
PR: csg-org/CompactConnect#1036
File: backend/compact-connect/lambdas/python/common/cc_common/data_model/provider_record_util.py:370-383
Timestamp: 2025-09-11T20:06:40.709Z
Learning: The EncumbranceDetailsSchema in backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/privilege/record.py does not contain a 'note' field. It only has clinicalPrivilegeActionCategory (String, optional), adverseActionId (UUID, required), and licenseJurisdiction (Jurisdiction, optional). When working with encumbrance notes, use encumbranceDetails['clinicalPrivilegeActionCategory'], not encumbranceDetails['note'].
Applied to files:
backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_encumbrance.py
🧬 Code graph analysis (3)
backend/compact-connect/stacks/feature_flag_stack/__init__.py (3)
backend/compact-connect/common_constructs/python_function.py (1)
PythonFunction(21-155)backend/compact-connect/common_constructs/stack.py (1)
AppStack(92-141)backend/compact-connect/stacks/feature_flag_stack/feature_flag_resource.py (2)
FeatureFlagEnvironmentName(15-20)FeatureFlagResource(23-72)
backend/compact-connect/stacks/api_stack/v1_api/api.py (2)
backend/compact-connect/stacks/api_stack/v1_api/feature_flags.py (1)
FeatureFlagsApi(12-57)backend/compact-connect/common_constructs/stack.py (2)
api_domain_name(109-112)common_env_vars(81-89)
backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_encumbrance.py (2)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/privilege/__init__.py (2)
PrivilegeUpdateData(91-162)encumbranceDetails(151-155)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/common.py (1)
from_database_record(124-139)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: TestPython
🔇 Additional comments (23)
backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_encumbrance.py (2)
157-158: LGTM! Addresses previous review feedback.Loading the PrivilegeUpdateData from the database record before constructing expected data enables the test to validate the actual adverseActionId persisted by the handler, rather than masking it with a hard-coded value.
167-168: LGTM! Properly validates the handler-stored adverseActionId.By extracting the adverseActionId from the loaded data, the test now validates the actual value persisted by the handler. This addresses the previous review concern and ensures the test properly verifies the handler's behavior.
backend/compact-connect/stacks/api_stack/v1_api/api.py (3)
19-19: LGTM!The import is correctly placed and follows the existing alphabetical ordering convention.
48-51: LGTM! Past issue properly addressed.The conditional guard correctly prevents setting
API_BASE_URLtohttps://Nonewhen no custom domain exists. The implementation matches the fix suggested in the previous review, and the comments clearly document the purpose of this environment variable.
131-137: LGTM!The feature flags resource and API integration follows the established pattern used by other API subsystems in this file. The initialization is consistent with the
FeatureFlagsApiconstructor signature.backend/compact-connect/lambdas/python/common/cc_common/feature_flag_client.py (4)
1-14: LGTM!Module imports and documentation are clean. The stateless interface design is appropriate for Lambda-to-Lambda communication.
16-43: LGTM!The
FeatureFlagContextdataclass is well-designed with appropriate type hints and a clean serialization method that correctly filtersNonevalues and converts naming conventions.
109-112: LGTM!The broad exception handler with
fail_defaultfallback is appropriate for a fail-safe feature flag client. Prevents feature flag issues from disrupting core system functionality.
87-92: Confirm feature-flag endpoint security
Thecheck_feature_flaghandler is decorated with@api_handlerbut includes no auth checks. Ensure this endpoint is secured as intended (e.g. API Gateway authorizer, API key, IAM signing, or VPC restriction) if it shouldn’t be publicly accessible.backend/compact-connect/lambdas/python/feature-flag/feature_flag_client.py (5)
179-198: LGTM!The schema design cleanly separates provider-agnostic base schema from StatSig-specific extensions. The validation constraints are appropriate for the use case.
255-257: Verify cold start impact of synchronous initialization.The synchronous
wait()onstatsig_client.initialize()blocks the Lambda handler during cold starts. While this ensures the client is ready before processing requests, it may increase cold start latency.Consider measuring the initialization time and documenting the expected cold start overhead. If initialization is slow, you might want to implement a lazy initialization pattern or accept the trade-off explicitly.
262-306: LGTM!The check_flag implementation correctly converts context to StatsigUser, handles missing userId with a sensible default, and properly wraps exceptions. The defensive re-initialization call ensures robustness.
376-382: Helpful clarification about rule immutability.The comment "we only set the environment rule if it doesn't already exist else we leave it alone to avoid overwriting manual changes" clearly explains why the code doesn't update existing rules. This preserves manual console changes, which is a good design choice for CDK-managed resources.
530-597: LGTM!The delete_flag implementation correctly handles both full deletion (when only one rule exists) and partial deletion (removing environment-specific rule). The logic error from the previous review has been fixed, and the three-state return (True/False/None) properly communicates the outcome.
backend/compact-connect/stacks/feature_flag_stack/feature_flag_resource.py (3)
1-13: LGTM!The module documentation clearly describes the construct's purpose, and the imports are appropriate for the functionality.
60-60: The autoEnable membership check works correctly with StrEnum.The expression
environment_name in auto_enable_envscorrectly evaluates membership becauseFeatureFlagEnvironmentNameis aStrEnum, which supports direct string comparison. For example,'test' in [FeatureFlagEnvironmentName.TEST]returnsTruebecauseStrEnum.__eq__compares the underlying string value.The past review comment suggesting this check was broken applied to regular
Enumtypes but notStrEnum. No fix is needed.
66-72: LGTM!The custom resource is properly configured with the correct service token, resource type, and properties.
backend/compact-connect/stacks/feature_flag_stack/__init__.py (6)
1-66: LGTM!The module documentation is comprehensive and well-structured, covering the feature flag lifecycle, StatSig environment mapping, usage patterns, and custom attributes. The Python examples are clear and demonstrate both simple and context-aware usage.
99-114: LGTM!The example flag correctly demonstrates:
- Sharing the provider across resources
- Configuring auto-enable for specific environments
- Using custom attributes for targeting
Note that this example auto-enables in all environments. Production flags may have more selective auto-enable configurations.
119-129: LGTM!The Lambda function configuration is appropriate for a custom resource handler:
- 5-minute timeout accommodates external StatSig API calls during create/update/delete operations
- 256MB memory is sufficient for API operations
- Environment variable passes the deployment environment correctly
132-137: LGTM!The secrets management is properly configured:
- Uses the current
from_secret_name_v2method- Follows a consistent naming convention for multi-environment secrets
- Grants appropriate read-only access to the Lambda function
154-156: LGTM!The Provider is correctly configured with the shared on_event_handler and appropriate log retention for a deploy-time resource.
140-204: LGTM!The CDK Nag suppressions are comprehensive and well-justified:
- IAM5 wildcards are necessary for secret version access
- Lambda runtime/configuration suppressions are appropriate for deploy-time custom resource functions
- Each suppression includes a clear reason explaining why the rule doesn't apply
backend/compact-connect/lambdas/python/common/cc_common/feature_flag_client.py
Show resolved
Hide resolved
backend/compact-connect/lambdas/python/feature-flag/feature_flag_client.py
Show resolved
Hide resolved
backend/compact-connect/stacks/feature_flag_stack/feature_flag_resource.py
Show resolved
Hide resolved
backend/compact-connect/stacks/feature_flag_stack/feature_flag_resource.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
backend/compact-connect/lambdas/python/feature-flag/feature_flag_client.py (1)
331-343: Simplify HTTP dispatch with requests.requestReduces branching and keeps behavior consistent.
- try: - if method == 'GET': - response = requests.get(url, headers=headers, timeout=30) - elif method == 'POST': - response = requests.post(url, headers=headers, json=data, timeout=30) - elif method == 'PATCH': - response = requests.patch(url, headers=headers, json=data, timeout=30) - elif method == 'DELETE': - response = requests.delete(url, headers=headers, timeout=30) - else: - raise ValueError(f'Unsupported HTTP method: {method}') - - return response + try: + # json=data is ignored if None (e.g., for GET/DELETE) + response = requests.request(method, url, headers=headers, json=data, timeout=30) + return response
🧹 Nitpick comments (6)
backend/compact-connect/lambdas/python/feature-flag/tests/function/test_statsig_client.py (5)
4-9: Avoid duplicating API constants; import from feature_flag_clientPrevents drift between tests and implementation.
-from feature_flag_client import ( - FeatureFlagException, - FeatureFlagRequest, - FeatureFlagValidationException, - StatSigFeatureFlagClient, -) +from feature_flag_client import ( + FeatureFlagException, + FeatureFlagRequest, + FeatureFlagValidationException, + StatSigFeatureFlagClient, + STATSIG_API_BASE_URL, + STATSIG_API_VERSION, +) @@ -STATSIG_API_BASE_URL = 'https://statsigapi.net/console/v1' -STATSIG_API_VERSION = '20240601' -Also applies to: 18-20
37-41: Drop explicit region for moto client; consider centralizing this helperMoto usually respects AWS_DEFAULT_REGION; the explicit region is unnecessary. Also, this helper is duplicated across tests—move it to a shared base (e.g., TstFunction) to DRY.
- return boto3.client('secretsmanager', region_name='us-east-1') + return boto3.client('secretsmanager')
81-88: Remove redundant mocking line
_setup_mock_statsigalready setsinitialize.return_value; the extra line on Line 84 does nothing.- mock_statsig.initialize.return_value = MagicMock()
488-496: Fix incorrect test descriptionDocstring says prod/autoEnable=True, but the test is beta/auto_enable=False and adds a rule.
- """Test upsert in prod environment with autoEnable=True and no existing flag""" + """Test upsert in beta environment with auto_enable=False; adds beta-rule when missing"""
1049-1066: Correct misleading inline comment about passPercentageFor test env with auto_enable=False, passPercentage should be 0 (not “always 100”).
- 'passPercentage': 0, # Always 100 for test environment + 'passPercentage': 0, # 0 when auto_enable=False (test environment)backend/compact-connect/lambdas/python/feature-flag/feature_flag_client.py (1)
204-206: Update docstrings to match current behavior (secrets source, custom_attributes application)
- Config is sourced from AWS Secrets Manager, not env vars.
- custom_attributes are applied whenever provided; passPercentage depends on auto_enable.
@@ - This client uses StatSig's Python SDK to check feature flags. - Configuration is handled through environment variables. + This client uses StatSig's Python SDK to check feature flags. + Configuration is sourced from AWS Secrets Manager. @@ - - If auto_enable is True: passPercentage is set to 100 (enabled) and custom attributes are applied + - If auto_enable is True: passPercentage is 100 (enabled) + - custom_attributes (when provided) are always included as rule conditions @@ - :param custom_attributes: Optional custom attributes for targeting (only applied if auto_enable=True) + :param custom_attributes: Optional custom attributes for targeting (applied whenever provided) @@ - :param custom_attributes: Optional custom attributes for targeting (only applied if auto_enable=True) + :param custom_attributes: Optional custom attributes for targeting (applied whenever provided)Also applies to: 358-363, 389-394, 464-467
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/compact-connect/lambdas/python/feature-flag/feature_flag_client.py(1 hunks)backend/compact-connect/lambdas/python/feature-flag/tests/function/test_statsig_client.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
backend/compact-connect/lambdas/python/feature-flag/tests/function/test_statsig_client.py (4)
backend/compact-connect/lambdas/python/feature-flag/feature_flag_client.py (11)
FeatureFlagException(154-155)FeatureFlagRequest(19-23)FeatureFlagValidationException(158-159)StatSigFeatureFlagClient(200-611)validate_request(62-73)check_flag(76-83)check_flag(275-305)upsert_flag(86-100)upsert_flag(348-381)delete_flag(113-123)delete_flag(529-571)backend/compact-connect/lambdas/python/feature-flag/tests/function/__init__.py (2)
TstFunction(14-23)setUp(17-23)backend/compact-connect/lambdas/python/feature-flag/tests/function/test_check_feature_flag.py (2)
setUp(14-27)_setup_mock_statsig(47-57)backend/compact-connect/lambdas/python/feature-flag/tests/function/test_manage_feature_flag.py (2)
setUp(16-24)create_mock_secrets_manager(26-30)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: TestPython
backend/compact-connect/lambdas/python/feature-flag/feature_flag_client.py
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
backend/compact-connect/stacks/feature_flag_stack/feature_flag_resource.py (1)
54-60: Consider validating environment_name against enum values.The validation checks if
environment_nameis truthy but doesn't verify it's a valid enum value. While StrEnum enables string comparison at line 60, the code doesn't handle case sensitivity or validate against allowed values. An invalid environment like"PROD"(uppercase) or"production"would pass validation but fail theautoEnablecheck silently.You could add validation after line 55:
if not flag_name or not environment_name: raise ValueError('flag_name and environment_name are required') + + # Validate environment_name is a known value + valid_envs = {env.value for env in FeatureFlagEnvironmentName} + if environment_name not in valid_envs: + raise ValueError( + f'environment_name must be one of {valid_envs}, got: {environment_name}' + )backend/compact-connect/lambdas/python/feature-flag/tests/function/test_statsig_client.py (1)
80-95: Remove redundant mock setup on line 84.Line 84 sets
mock_statsig.initialize.return_valuedirectly on the mock class, but_setup_mock_statsig(mock_statsig)is called on line 86, which already configures the mock client'sinitializemethod (line 46). The line is redundant and could cause confusion about which mock setup takes precedence.Apply this diff:
@patch('feature_flag_client.Statsig') def test_validate_request_minimal_data(self, mock_statsig): """Test request validation with minimal valid data""" self._setup_mock_statsig(mock_statsig) - mock_statsig.initialize.return_value = MagicMock() client = StatSigFeatureFlagClient(environment='test')
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
backend/compact-connect/lambdas/python/common/cc_common/feature_flag_client.py(1 hunks)backend/compact-connect/lambdas/python/feature-flag/feature_flag_client.py(1 hunks)backend/compact-connect/lambdas/python/feature-flag/tests/function/test_statsig_client.py(1 hunks)backend/compact-connect/stacks/feature_flag_stack/feature_flag_resource.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- backend/compact-connect/lambdas/python/feature-flag/feature_flag_client.py
- backend/compact-connect/lambdas/python/common/cc_common/feature_flag_client.py
🧰 Additional context used
🧬 Code graph analysis (1)
backend/compact-connect/lambdas/python/feature-flag/tests/function/test_statsig_client.py (4)
backend/compact-connect/lambdas/python/feature-flag/feature_flag_client.py (11)
FeatureFlagException(154-155)FeatureFlagRequest(19-23)FeatureFlagValidationException(158-159)StatSigFeatureFlagClient(200-615)validate_request(62-73)check_flag(76-83)check_flag(275-305)upsert_flag(86-100)upsert_flag(348-381)delete_flag(113-123)delete_flag(533-575)backend/compact-connect/lambdas/python/feature-flag/tests/function/__init__.py (2)
TstFunction(14-23)setUp(17-23)backend/compact-connect/lambdas/python/feature-flag/tests/function/test_check_feature_flag.py (2)
setUp(14-27)_setup_mock_statsig(47-57)backend/compact-connect/lambdas/python/feature-flag/tests/function/test_manage_feature_flag.py (2)
setUp(16-24)create_mock_secrets_manager(26-30)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: TestPython
🔇 Additional comments (1)
backend/compact-connect/lambdas/python/feature-flag/tests/function/test_statsig_client.py (1)
26-53: LGTM! Excellent test coverage and structure.The test suite is comprehensive and well-organized, covering:
- Initialization and secrets management
- Request validation with various inputs
- Feature flag checking with custom attributes and default users
- Environment-to-tier mapping across all environments
- CRUD operations (upsert/delete) with multiple scenarios
- Error handling for API failures
- Custom attribute handling as strings and lists
The mocking strategy is consistent and the test data generator integration provides good reusability.
backend/compact-connect/lambdas/python/feature-flag/tests/function/test_statsig_client.py
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
backend/compact-connect/README.md (2)
344-346: Fix typos in domain examples.“compcatconnect” should be “compactconnect”.
- of having your test environments be a subdomain of your production environments (i.e. `compcatconnect.org` for prod - and `test.compcatconnect.org` for test), you need to delegate nameserver authority from your production hosted zone + of having your test environments be a subdomain of your production environments (i.e. `compactconnect.org` for prod + and `test.compactconnect.org` for test), you need to delegate nameserver authority from your production hosted zone
223-224: Remove duplicated sentence.The example command is repeated.
- For example, to set up for the test environment: `bin/put_ssm_context.sh test`. +
🧹 Nitpick comments (1)
backend/compact-connect/README.md (1)
49-51: Add languages to fenced code blocks (fix MD040 and improve readability).Several code fences lack a language. Recommend adding explicit languages.
-``` +$ ```bash $ pip install -r requirements.txt -``` +```-``` +$ ```bash $ cdk synth -``` +```-``` +$ ```bash pip install -r requirements-dev.txt -``` +```- ``` + ```text compact-connect/env/{environment_name}/statsig/credentials ```- ``` + ```json { "token": "<value of private Secret Key from Google reCAPTCHA>" } - ``` + ```- ``` + ```bash aws secretsmanager create-secret --name compact-connect/env/{value of 'environment_name' in cdk.context.json}/recaptcha/token --secret-string '{"token": "<value of private Secret Key from Google reCAPTCHA>"}' - ``` + ```Also applies to: 62-64, 78-80, 268-271, 304-307, 309-311
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
backend/compact-connect/README.md(3 hunks)backend/compact-connect/lambdas/python/feature-flag/feature_flag_client.py(1 hunks)backend/compact-connect/lambdas/python/feature-flag/tests/function/test_statsig_client.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
backend/compact-connect/lambdas/python/feature-flag/tests/function/test_statsig_client.py (3)
backend/compact-connect/lambdas/python/feature-flag/feature_flag_client.py (11)
FeatureFlagException(154-155)FeatureFlagRequest(19-23)FeatureFlagValidationException(158-159)StatSigFeatureFlagClient(200-615)validate_request(62-73)check_flag(76-83)check_flag(275-305)upsert_flag(86-100)upsert_flag(348-381)delete_flag(113-123)delete_flag(533-575)backend/compact-connect/lambdas/python/feature-flag/tests/function/__init__.py (2)
TstFunction(14-23)setUp(17-23)backend/compact-connect/lambdas/python/feature-flag/tests/function/test_check_feature_flag.py (2)
setUp(14-27)_setup_mock_statsig(47-57)
🪛 markdownlint-cli2 (0.18.1)
backend/compact-connect/README.md
268-268: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: TestPython
backend/compact-connect/lambdas/python/feature-flag/feature_flag_client.py
Show resolved
Hide resolved
backend/compact-connect/lambdas/python/feature-flag/tests/function/test_statsig_client.py
Outdated
Show resolved
Hide resolved
backend/compact-connect/lambdas/python/feature-flag/tests/function/test_statsig_client.py
Show resolved
Hide resolved
backend/compact-connect/lambdas/python/feature-flag/tests/function/test_statsig_client.py
Show resolved
Hide resolved
backend/compact-connect/lambdas/python/feature-flag/tests/function/test_statsig_client.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
backend/compact-connect/lambdas/python/feature-flag/feature_flag_client.py(1 hunks)backend/compact-connect/lambdas/python/feature-flag/tests/function/test_statsig_client.py(1 hunks)backend/compact-connect/stacks/feature_flag_stack/feature_flag_resource.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- backend/compact-connect/lambdas/python/feature-flag/feature_flag_client.py
- backend/compact-connect/stacks/feature_flag_stack/feature_flag_resource.py
🧰 Additional context used
🧬 Code graph analysis (1)
backend/compact-connect/lambdas/python/feature-flag/tests/function/test_statsig_client.py (3)
backend/compact-connect/lambdas/python/feature-flag/feature_flag_client.py (10)
FeatureFlagException(154-155)FeatureFlagRequest(19-23)FeatureFlagValidationException(158-159)StatSigFeatureFlagClient(200-614)check_flag(76-83)check_flag(274-304)upsert_flag(86-100)upsert_flag(347-380)delete_flag(113-123)delete_flag(532-574)backend/compact-connect/lambdas/python/feature-flag/tests/function/__init__.py (2)
TstFunction(14-23)setUp(17-23)backend/compact-connect/lambdas/python/feature-flag/tests/function/test_check_feature_flag.py (2)
setUp(14-27)_setup_mock_statsig(47-57)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: TestPython
backend/compact-connect/lambdas/python/feature-flag/tests/function/test_statsig_client.py
Outdated
Show resolved
Hide resolved
backend/compact-connect/lambdas/python/feature-flag/tests/function/test_statsig_client.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
backend/compact-connect/lambdas/python/feature-flag/tests/function/test_statsig_client.py (1)
475-482: Docstring contradicts the test (auto_enable=False in beta).The test calls upsert_flag(..., auto_enable=False) and creates a beta rule when missing. Update the docstring to match.
- """Test upsert in beta environment with autoEnable=True and no existing flag creates beta rule""" + """Test upsert in beta environment with auto_enable=False and no existing beta rule (creates beta-rule)"""
🧹 Nitpick comments (5)
backend/compact-connect/stacks/api_stack/v1_api/feature_flags.py (1)
44-58: Nit: comment says GET but method is POST.Update to avoid confusion.
- # Add suppressions for the public GET endpoint + # Add suppressions for the public POST endpointbackend/compact-connect/lambdas/python/feature-flag/tests/function/test_check_feature_flag.py (1)
137-146: Add test for requests with no body.Covers the common case where API Gateway sends body=null. Ensures handler defaults to empty context and returns 200.
@patch('feature_flag_client.Statsig') def test_no_body_defaults_to_empty_context(self, mock_statsig): self._setup_mock_statsig(mock_statsig, mock_flag_enabled_return=True) from handlers.check_feature_flag import check_feature_flag event = self._generate_test_api_gateway_event(body={}, flag_id='no-body-flag') event['body'] = None # simulate no body sent # also simulate missing content-type header in this case event['headers'].pop('Content-Type', None) result = check_feature_flag(event, self.mock_context) self.assertEqual(result['statusCode'], 200) response_body = json.loads(result['body']) self.assertEqual({'enabled': True}, response_body)backend/compact-connect/lambdas/python/feature-flag/feature_flag_client.py (3)
341-352: Docstring misaligned with behavior (conditions applied regardless of auto_enable).The code builds conditions whenever custom_attributes is provided. Update docs to match.
- - If auto_enable is True: passPercentage is set to 100 (enabled) and custom attributes are applied + - If auto_enable is True: passPercentage is set to 100 (enabled) + - Custom attributes (if provided) are always applied as conditions @@ - :param custom_attributes: Optional custom attributes for targeting (only applied if auto_enable=True) + :param custom_attributes: Optional custom attributes for targeting (applied regardless of auto_enable)
376-383: Align_create_new_gatedocstring with implementation.- :param custom_attributes: Optional custom attributes for targeting (only applied if auto_enable=True) + :param custom_attributes: Optional custom attributes for targeting (applied regardless of auto_enable)
455-461: Align_add_environment_ruledocstring with implementation.- :param custom_attributes: Optional custom attributes for targeting (only applied if auto_enable=True) + :param custom_attributes: Optional custom attributes for targeting (applied regardless of auto_enable)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
backend/compact-connect/lambdas/python/common/cc_common/feature_flag_client.py(1 hunks)backend/compact-connect/lambdas/python/common/tests/unit/test_feature_flag_client.py(1 hunks)backend/compact-connect/lambdas/python/feature-flag/feature_flag_client.py(1 hunks)backend/compact-connect/lambdas/python/feature-flag/handlers/check_feature_flag.py(1 hunks)backend/compact-connect/lambdas/python/feature-flag/tests/function/test_check_feature_flag.py(1 hunks)backend/compact-connect/lambdas/python/feature-flag/tests/function/test_statsig_client.py(1 hunks)backend/compact-connect/stacks/api_stack/v1_api/api_model.py(1 hunks)backend/compact-connect/stacks/api_stack/v1_api/feature_flags.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- backend/compact-connect/lambdas/python/common/cc_common/feature_flag_client.py
- backend/compact-connect/stacks/api_stack/v1_api/api_model.py
- backend/compact-connect/lambdas/python/common/tests/unit/test_feature_flag_client.py
🧰 Additional context used
🧬 Code graph analysis (4)
backend/compact-connect/lambdas/python/feature-flag/tests/function/test_check_feature_flag.py (4)
backend/compact-connect/lambdas/python/feature-flag/tests/function/__init__.py (2)
TstFunction(14-23)setUp(17-23)backend/compact-connect/lambdas/python/feature-flag/tests/function/test_statsig_client.py (2)
setUp(26-35)_setup_mock_statsig(43-53)backend/compact-connect/lambdas/python/common/common_test/test_data_generator.py (1)
generate_test_api_event(33-61)backend/compact-connect/lambdas/python/feature-flag/handlers/check_feature_flag.py (1)
check_feature_flag(14-54)
backend/compact-connect/lambdas/python/feature-flag/handlers/check_feature_flag.py (3)
backend/compact-connect/lambdas/python/common/cc_common/exceptions.py (2)
CCInternalException(44-45)CCInvalidRequestException(7-8)backend/compact-connect/lambdas/python/common/cc_common/utils.py (2)
api_handler(90-212)get(86-87)backend/compact-connect/lambdas/python/feature-flag/feature_flag_client.py (6)
FeatureFlagRequest(19-23)FeatureFlagValidationException(148-149)create_feature_flag_client(607-618)validate_request(52-63)check_flag(66-73)check_flag(264-294)
backend/compact-connect/stacks/api_stack/v1_api/feature_flags.py (3)
backend/compact-connect/common_constructs/cc_api.py (1)
CCApi(66-326)backend/compact-connect/stacks/api_lambda_stack/__init__.py (1)
ApiLambdaStack(13-49)backend/compact-connect/stacks/api_stack/v1_api/api_model.py (2)
check_feature_flag_request_model(2708-2741)check_feature_flag_response_model(2744-2763)
backend/compact-connect/lambdas/python/feature-flag/tests/function/test_statsig_client.py (4)
backend/compact-connect/lambdas/python/feature-flag/feature_flag_client.py (11)
FeatureFlagException(144-145)FeatureFlagRequest(19-23)FeatureFlagValidationException(148-149)StatSigFeatureFlagClient(190-604)validate_request(52-63)check_flag(66-73)check_flag(264-294)upsert_flag(76-90)upsert_flag(337-370)delete_flag(103-113)delete_flag(522-564)backend/compact-connect/lambdas/python/feature-flag/tests/function/__init__.py (2)
TstFunction(14-23)setUp(17-23)backend/compact-connect/lambdas/python/feature-flag/tests/function/test_check_feature_flag.py (2)
setUp(14-27)_setup_mock_statsig(48-58)backend/compact-connect/lambdas/python/feature-flag/tests/function/test_manage_feature_flag.py (2)
setUp(16-24)create_mock_secrets_manager(26-30)
🔇 Additional comments (1)
backend/compact-connect/lambdas/python/feature-flag/tests/function/test_statsig_client.py (1)
205-238: Environment tier mapping test looks solid.Good verification of StatsigOptions.environment per CompactConnect env mapping; serverKey and tiers asserted correctly.
backend/compact-connect/lambdas/python/feature-flag/handlers/check_feature_flag.py
Show resolved
Hide resolved
landonshumway-ia
force-pushed
the
feat/feature-flag-framework
branch
from
df4fcbe to
cc7d965
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/compact-connect/lambdas/python/common/tests/unit/test_provider_record_util.py (1)
1031-1034: Fix incorrect field name in encumbranceDetails.The test data uses
'note'as a field name inencumbranceDetails, but according to theEncumbranceDetailsSchema(lines 55-63 in record.py) and the learnings, the correct field name is'clinicalPrivilegeActionCategory'. This is inconsistent with the test at line 952 which correctly uses'clinicalPrivilegeActionCategory'.The code in
provider_record_util.py(line 390 in the relevant snippet) extractsevent['encumbranceDetails']['clinicalPrivilegeActionCategory'], so this test data will fail validation or produce incorrect results.Apply this diff to correct the field name:
'encumbranceDetails': { - 'note': 'Non-compliance With Requirements', + 'clinicalPrivilegeActionCategory': 'Non-compliance With Requirements', 'licenseJurisdiction': 'oh', },Based on learnings.
♻️ Duplicate comments (1)
backend/compact-connect/lambdas/python/feature-flag/requirements-dev.txt (1)
16-17: Invalidcffipin blocks installs.
cffi==2.0.0isn’t published on PyPI, so anypip install -r requirements-dev.txtfails immediately. Regenerate the lock (or adjust the specifier) so it resolves to an actual release such ascffi==1.17.1.
🧹 Nitpick comments (1)
backend/compact-connect/lambdas/python/provider-data-v1/handlers/registration.py (1)
218-218: LGTM! Improves observability consistency.This metric emission aligns with the established pattern of tracking failed registration attempts with
value=0across all error paths in this handler.Optional: Consider adding a similar metric to the compact configuration
CCNotFoundExceptionhandler (lines 186-194) for complete consistency, though the comment there suggests this is an unexpected edge case.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (40)
backend/compact-connect-ui-app/pipeline/frontend_pipeline.py(3 hunks)backend/compact-connect-ui-app/tests/app/test_pipeline.py(1 hunks)backend/compact-connect/README.md(3 hunks)backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py(1 hunks)backend/compact-connect/lambdas/python/common/cc_common/data_model/provider_record_util.py(3 hunks)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/privilege/record.py(2 hunks)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/api.py(1 hunks)backend/compact-connect/lambdas/python/common/cc_common/feature_flag_client.py(1 hunks)backend/compact-connect/lambdas/python/common/common_test/test_data_generator.py(1 hunks)backend/compact-connect/lambdas/python/common/requirements-dev.in(1 hunks)backend/compact-connect/lambdas/python/common/tests/__init__.py(1 hunks)backend/compact-connect/lambdas/python/common/tests/unit/test_feature_flag_client.py(1 hunks)backend/compact-connect/lambdas/python/common/tests/unit/test_provider_record_util.py(8 hunks)backend/compact-connect/lambdas/python/data-events/handlers/encumbrance_events.py(1 hunks)backend/compact-connect/lambdas/python/data-events/tests/function/test_encumbrance_events.py(1 hunks)backend/compact-connect/lambdas/python/feature-flag/custom_resource_handler.py(1 hunks)backend/compact-connect/lambdas/python/feature-flag/feature_flag_client.py(1 hunks)backend/compact-connect/lambdas/python/feature-flag/handlers/check_feature_flag.py(1 hunks)backend/compact-connect/lambdas/python/feature-flag/handlers/manage_feature_flag.py(1 hunks)backend/compact-connect/lambdas/python/feature-flag/requirements-dev.in(1 hunks)backend/compact-connect/lambdas/python/feature-flag/requirements-dev.txt(1 hunks)backend/compact-connect/lambdas/python/feature-flag/requirements.in(1 hunks)backend/compact-connect/lambdas/python/feature-flag/requirements.txt(1 hunks)backend/compact-connect/lambdas/python/feature-flag/tests/__init__.py(1 hunks)backend/compact-connect/lambdas/python/feature-flag/tests/function/__init__.py(1 hunks)backend/compact-connect/lambdas/python/feature-flag/tests/function/test_check_feature_flag.py(1 hunks)backend/compact-connect/lambdas/python/feature-flag/tests/function/test_manage_feature_flag.py(1 hunks)backend/compact-connect/lambdas/python/feature-flag/tests/function/test_statsig_client.py(1 hunks)backend/compact-connect/lambdas/python/provider-data-v1/handlers/registration.py(1 hunks)backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_encumbrance.py(2 hunks)backend/compact-connect/pipeline/backend_pipeline.py(3 hunks)backend/compact-connect/pipeline/backend_stage.py(2 hunks)backend/compact-connect/stacks/api_lambda_stack/__init__.py(2 hunks)backend/compact-connect/stacks/api_lambda_stack/feature_flags.py(1 hunks)backend/compact-connect/stacks/api_stack/v1_api/api.py(3 hunks)backend/compact-connect/stacks/api_stack/v1_api/api_model.py(1 hunks)backend/compact-connect/stacks/api_stack/v1_api/feature_flags.py(1 hunks)backend/compact-connect/stacks/feature_flag_stack/__init__.py(1 hunks)backend/compact-connect/stacks/feature_flag_stack/feature_flag_resource.py(1 hunks)backend/compact-connect/tests/app/test_pipeline.py(1 hunks)
✅ Files skipped from review due to trivial changes (5)
- backend/compact-connect-ui-app/tests/app/test_pipeline.py
- backend/compact-connect/lambdas/python/feature-flag/requirements.in
- backend/compact-connect/lambdas/python/data-events/tests/function/test_encumbrance_events.py
- backend/compact-connect-ui-app/pipeline/frontend_pipeline.py
- backend/compact-connect/tests/app/test_pipeline.py
🚧 Files skipped from review as they are similar to previous changes (15)
- backend/compact-connect/lambdas/python/feature-flag/requirements-dev.in
- backend/compact-connect/stacks/api_stack/v1_api/api.py
- backend/compact-connect/lambdas/python/common/cc_common/data_model/provider_record_util.py
- backend/compact-connect/lambdas/python/feature-flag/requirements.txt
- backend/compact-connect/lambdas/python/common/requirements-dev.in
- backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/api.py
- backend/compact-connect/lambdas/python/common/cc_common/feature_flag_client.py
- backend/compact-connect/stacks/api_lambda_stack/feature_flags.py
- backend/compact-connect/lambdas/python/common/common_test/test_data_generator.py
- backend/compact-connect/stacks/api_stack/v1_api/api_model.py
- backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_encumbrance.py
- backend/compact-connect/pipeline/backend_stage.py
- backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py
- backend/compact-connect/lambdas/python/feature-flag/tests/function/test_check_feature_flag.py
- backend/compact-connect/stacks/feature_flag_stack/feature_flag_resource.py
🧰 Additional context used
🧠 Learnings (8)
📚 Learning: 2025-09-11T20:06:40.709Z
Learnt from: ChiefStief
PR: csg-org/CompactConnect#1036
File: backend/compact-connect/lambdas/python/common/cc_common/data_model/provider_record_util.py:370-383
Timestamp: 2025-09-11T20:06:40.709Z
Learning: The EncumbranceDetailsSchema in backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/privilege/record.py does not contain a 'note' field. It only has clinicalPrivilegeActionCategory (String, optional), adverseActionId (UUID, required), and licenseJurisdiction (Jurisdiction, optional).
Applied to files:
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/privilege/record.py
📚 Learning: 2025-09-11T20:06:40.709Z
Learnt from: ChiefStief
PR: csg-org/CompactConnect#1036
File: backend/compact-connect/lambdas/python/common/cc_common/data_model/provider_record_util.py:370-383
Timestamp: 2025-09-11T20:06:40.709Z
Learning: The EncumbranceDetailsSchema in backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/privilege/record.py does not contain a 'note' field. It only has clinicalPrivilegeActionCategory (String, optional), adverseActionId (UUID, required), and licenseJurisdiction (Jurisdiction, optional). When working with encumbrance notes, use encumbranceDetails['clinicalPrivilegeActionCategory'], not encumbranceDetails['note'].
Applied to files:
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/privilege/record.py
📚 Learning: 2025-10-06T23:29:03.838Z
Learnt from: landonshumway-ia
PR: csg-org/CompactConnect#1110
File: backend/compact-connect/stacks/api_stack/v1_api/feature_flags.py:24-40
Timestamp: 2025-10-06T23:29:03.838Z
Learning: In the CompactConnect codebase, API Gateway endpoint classes access the CCApi instance via `resource.api` (not `resource.rest_api`), and api_model schema properties are accessed without parentheses (as properties, not method calls). This pattern is consistent across all v1_api endpoint implementations (provider_users.py, staff_users.py, purchases.py, etc.).
Applied to files:
backend/compact-connect/stacks/api_stack/v1_api/feature_flags.py
📚 Learning: 2025-08-22T21:04:46.552Z
Learnt from: landonshumway-ia
PR: csg-org/CompactConnect#1029
File: backend/compact-connect/lambdas/python/provider-data-v1/handlers/licenses.py:0-0
Timestamp: 2025-08-22T21:04:46.552Z
Learning: API Gateway always includes the 'body' key in Lambda events, even when the request body is empty/null. When event['body'] is None, json.loads(None) raises TypeError, not a missing key error.
Applied to files:
backend/compact-connect/lambdas/python/feature-flag/handlers/check_feature_flag.py
📚 Learning: 2025-09-03T22:29:24.535Z
Learnt from: ChiefStief
PR: csg-org/CompactConnect#1036
File: backend/compact-connect/lambdas/python/data-events/handlers/encumbrance_events.py:0-0
Timestamp: 2025-09-03T22:29:24.535Z
Learning: The EncumbranceEventDetailSchema in backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/data_event/api.py is used across multiple instances/contexts where adverseActionCategory and adverseActionId fields may be required in some cases and not present in others. This is an intentional design pattern for handling different event variants with a single schema.
Applied to files:
backend/compact-connect/lambdas/python/data-events/handlers/encumbrance_events.py
📚 Learning: 2025-07-22T03:36:17.137Z
Learnt from: landonshumway-ia
PR: csg-org/CompactConnect#907
File: backend/compact-connect/lambdas/python/purchases/requirements-dev.txt:15-0
Timestamp: 2025-07-22T03:36:17.137Z
Learning: In CompactConnect, requirements-dev.txt files for Lambda functions are used exclusively for running tests and development, not for actual Lambda runtime environments. Concerns about runtime compatibility (like OpenSSL versions) don't apply to these development dependency files.
Applied to files:
backend/compact-connect/lambdas/python/feature-flag/requirements-dev.txt
📚 Learning: 2025-07-22T03:52:25.934Z
Learnt from: landonshumway-ia
PR: csg-org/CompactConnect#907
File: backend/compact-connect/lambdas/python/provider-data-v1/requirements.txt:2-2
Timestamp: 2025-07-22T03:52:25.934Z
Learning: In CompactConnect, the Python version used by pip-compile to generate requirements.txt files (shown in the header comment) is separate from the actual Lambda runtime environment. Dependencies are installed by a Python 3.12 container during the CI/CD pipeline, ensuring runtime compatibility regardless of the Python version used for pip-compile dependency resolution.
Applied to files:
backend/compact-connect/lambdas/python/feature-flag/requirements-dev.txt
📚 Learning: 2025-08-12T19:49:24.999Z
Learnt from: landonshumway-ia
PR: csg-org/CompactConnect#1001
File: backend/compact-connect/lambdas/python/disaster-recovery/requirements.in:1-1
Timestamp: 2025-08-12T19:49:24.999Z
Learning: In CompactConnect disaster-recovery Lambda functions, runtime dependencies like boto3, aws-lambda-powertools, and botocore are provided by lambda layers at deploy time rather than being specified in requirements.in files. The requirements.in file intentionally contains only a comment explaining this approach.
Applied to files:
backend/compact-connect/lambdas/python/feature-flag/requirements-dev.txt
🧬 Code graph analysis (11)
backend/compact-connect/lambdas/python/common/tests/unit/test_provider_record_util.py (1)
backend/compact-connect/lambdas/python/common/cc_common/data_model/provider_record_util.py (2)
ProviderRecordUtility(53-407)construct_simplified_privilege_history_object(369-407)
backend/compact-connect/stacks/api_stack/v1_api/feature_flags.py (3)
backend/compact-connect/common_constructs/cc_api.py (1)
CCApi(66-326)backend/compact-connect/stacks/api_lambda_stack/__init__.py (1)
ApiLambdaStack(13-49)backend/compact-connect/stacks/api_stack/v1_api/api_model.py (2)
check_feature_flag_request_model(2708-2741)check_feature_flag_response_model(2744-2763)
backend/compact-connect/stacks/feature_flag_stack/__init__.py (2)
backend/compact-connect/common_constructs/python_function.py (1)
PythonFunction(21-155)backend/compact-connect/stacks/feature_flag_stack/feature_flag_resource.py (2)
FeatureFlagEnvironmentName(15-20)FeatureFlagResource(23-72)
backend/compact-connect/lambdas/python/feature-flag/handlers/manage_feature_flag.py (2)
backend/compact-connect/lambdas/python/feature-flag/custom_resource_handler.py (5)
CustomResourceHandler(18-124)CustomResourceResponse(10-15)on_create(90-99)on_update(102-111)on_delete(114-124)backend/compact-connect/lambdas/python/feature-flag/feature_flag_client.py (6)
FeatureFlagException(144-145)StatSigFeatureFlagClient(190-604)upsert_flag(76-90)upsert_flag(337-370)delete_flag(103-113)delete_flag(522-564)
backend/compact-connect/lambdas/python/feature-flag/handlers/check_feature_flag.py (3)
backend/compact-connect/lambdas/python/common/cc_common/exceptions.py (2)
CCInternalException(44-45)CCInvalidRequestException(7-8)backend/compact-connect/lambdas/python/common/cc_common/utils.py (2)
api_handler(90-212)get(86-87)backend/compact-connect/lambdas/python/feature-flag/feature_flag_client.py (6)
FeatureFlagRequest(19-23)FeatureFlagValidationException(148-149)create_feature_flag_client(607-618)validate_request(52-63)check_flag(66-73)check_flag(264-294)
backend/compact-connect/lambdas/python/feature-flag/tests/function/test_manage_feature_flag.py (3)
backend/compact-connect/lambdas/python/feature-flag/tests/function/test_statsig_client.py (2)
setUp(26-35)create_mock_secrets_manager(37-41)backend/compact-connect/lambdas/python/feature-flag/handlers/manage_feature_flag.py (3)
ManageFeatureFlagHandler(15-105)on_create(24-61)on_delete(72-105)backend/compact-connect/lambdas/python/feature-flag/feature_flag_client.py (4)
upsert_flag(76-90)upsert_flag(337-370)delete_flag(103-113)delete_flag(522-564)
backend/compact-connect/lambdas/python/feature-flag/tests/function/__init__.py (2)
backend/compact-connect/lambdas/python/feature-flag/tests/__init__.py (1)
TstLambdas(9-85)backend/compact-connect/lambdas/python/common/common_test/test_data_generator.py (1)
TestDataGenerator(21-655)
backend/compact-connect/stacks/api_lambda_stack/__init__.py (1)
backend/compact-connect/stacks/api_lambda_stack/feature_flags.py (1)
FeatureFlagsLambdas(13-64)
backend/compact-connect/lambdas/python/feature-flag/custom_resource_handler.py (1)
backend/compact-connect/lambdas/python/feature-flag/handlers/manage_feature_flag.py (3)
on_create(24-61)on_update(63-70)on_delete(72-105)
backend/compact-connect/lambdas/python/common/tests/unit/test_feature_flag_client.py (2)
backend/compact-connect/lambdas/python/common/tests/__init__.py (1)
TstLambdas(9-109)backend/compact-connect/lambdas/python/common/cc_common/feature_flag_client.py (3)
is_feature_enabled(46-112)FeatureFlagContext(17-43)to_dict(32-43)
backend/compact-connect/lambdas/python/feature-flag/tests/function/test_statsig_client.py (4)
backend/compact-connect/lambdas/python/feature-flag/feature_flag_client.py (11)
FeatureFlagException(144-145)FeatureFlagRequest(19-23)FeatureFlagValidationException(148-149)StatSigFeatureFlagClient(190-604)validate_request(52-63)check_flag(66-73)check_flag(264-294)upsert_flag(76-90)upsert_flag(337-370)delete_flag(103-113)delete_flag(522-564)backend/compact-connect/lambdas/python/feature-flag/tests/function/__init__.py (2)
TstFunction(14-23)setUp(17-23)backend/compact-connect/lambdas/python/feature-flag/tests/function/test_check_feature_flag.py (2)
setUp(14-27)_setup_mock_statsig(48-58)backend/compact-connect/lambdas/python/feature-flag/tests/function/test_manage_feature_flag.py (2)
setUp(16-24)create_mock_secrets_manager(26-30)
🪛 markdownlint-cli2 (0.18.1)
backend/compact-connect/README.md
309-309: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (22)
backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/privilege/record.py (2)
54-64: LGTM! Schema correctly defines encumbrance details.The new
EncumbranceDetailsSchemacorrectly defines the three fields for tracking privilege encumbrance details. This aligns with the existing learnings that confirm the schema should containclinicalPrivilegeActionCategory,adverseActionId, andlicenseJurisdiction(not a 'note' field).Based on learnings.
66-66: LGTM! Registration decorator correctly added.The
@BaseRecordSchema.register_schema('privilege')decorator properly registers this schema for runtime lookup, consistent with the schema registration pattern used throughout the codebase.backend/compact-connect/lambdas/python/data-events/handlers/encumbrance_events.py (1)
212-212: LGTM! Formatting improvement.Trailing comma added for consistency with multi-line argument formatting.
backend/compact-connect/lambdas/python/common/tests/unit/test_provider_record_util.py (1)
882-882: LGTM! Formatting changes improve consistency.These formatting changes consolidate multi-line calls to single lines and add trailing commas, improving code consistency without altering behavior.
Also applies to: 897-897, 906-907, 960-960, 975-975, 984-985, 1039-1039, 1054-1054, 1062-1063, 1070-1070
backend/compact-connect/pipeline/backend_pipeline.py (2)
231-232: LGTM! Clean refactor of Fn.join arguments.The change consolidates the array argument into a single inline list, improving readability while maintaining the correct
Fn.join(delimiter, list_of_values)signature.
260-265: LGTM! Consistent quote style normalization.The formatting changes standardize quote usage to single quotes and adjust bracket placement for improved consistency across the codebase.
Also applies to: 280-280, 283-283
backend/compact-connect/stacks/feature_flag_stack/__init__.py (3)
85-93: LGTM!The constructor signature and
super().__init__call are correct. Theenvironment_contextis properly forwarded through**kwargsas confirmed in the past review discussion.
116-206: LGTM!The provider creation is well-structured:
- Proper Lambda configuration with 5-minute timeout appropriate for custom resource operations
- Secret access pattern matches the expected naming convention
- CDK Nag suppressions are comprehensive and well-justified
- The handler path
os.path.join('handlers', 'manage_feature_flag.py')aligns with the handler file atbackend/compact-connect/lambdas/python/feature-flag/handlers/manage_feature_flag.py
99-114: Verify auto-enabling example flag in production.The
example-flagis configured to auto-enable in all environments including PROD. While this demonstrates the feature, consider whether an example flag should be automatically enabled in production without manual review.If this is intentional for demonstration purposes, consider adding a comment explaining the rationale. Otherwise, you might want to exclude
FeatureFlagEnvironmentName.PRODfrom theauto_enable_envslist for the example flag.backend/compact-connect/lambdas/python/common/tests/__init__.py (1)
16-16: LGTM!The addition of
API_BASE_URLto the test environment is appropriate. The valuehttps://api.example.comis a suitable test fixture, and this aligns with the broader feature flag infrastructure that uses this environment variable for endpoint communication.backend/compact-connect/stacks/api_lambda_stack/__init__.py (2)
33-36: LGTM!The conditional
API_BASE_URLsetting correctly handles the circular dependency limitation. As discussed in past comments, feature flags will usefail_defaultin sandbox environments without configured domain names, which is an acceptable tradeoff.
38-42: LGTM!The
FeatureFlagsLambdasinitialization follows the established pattern for lambda collections in this stack and correctly passes the required parameters.backend/compact-connect/lambdas/python/feature-flag/tests/__init__.py (1)
9-85: LGTM!The test base class provides appropriate setup for feature-flag tests. The minimal environment configuration focuses on the essentials needed for feature flag functionality, and setting
DEBUG='true'enables detailed logging during tests.backend/compact-connect/lambdas/python/feature-flag/tests/function/__init__.py (1)
13-23: LGTM!The functional test base class correctly:
- Uses
@mock_awsfor Moto-based mocking- Lazily imports
TestDataGeneratorafter environment variables are set- Follows the established pattern for functional test infrastructure
backend/compact-connect/stacks/api_stack/v1_api/feature_flags.py (1)
12-58: LGTM!The API wiring is correct and follows the established codebase patterns:
- Uses
resource.api(notresource.rest_api)- Accesses model properties without parentheses
- Properly wires the Lambda integration for the check endpoint
- Adds log group for query definitions
- Appropriate NagSuppressions for the intentionally public endpoint
Based on learnings and past review discussion.
backend/compact-connect/lambdas/python/feature-flag/tests/function/test_manage_feature_flag.py (3)
16-30: LGTM!The test setup correctly initializes Moto's mock Secrets Manager with test credentials, following the same pattern used in other feature-flag tests.
32-78: LGTM!The
on_createtests provide good coverage:
- Full properties case verifies all parameters are passed correctly
- Minimal properties case validates default behavior (autoEnable=False, customAttributes=None)
- Both verify the response structure with PhysicalResourceId and gateId
80-102: LGTM!The
on_deletetest appropriately verifies:
- Client initialization with the correct environment
delete_flagis called with the correct flag name- Handler returns None for successful deletion
backend/compact-connect/lambdas/python/feature-flag/handlers/check_feature_flag.py (4)
9-10: LGTM!Module-level client initialization enables proper caching across Lambda invocations, following AWS Lambda best practices.
23-28: LGTM!The flagId extraction is robust, with null-safe access and clear error messaging for missing path parameters.
30-36: LGTM!The body parsing and validation is correct:
- The schema's
load_default=dictfor the context field ensuresvalidated_bodyalways contains the context key (as confirmed in past review)FeatureFlagValidationExceptionis appropriately mapped toCCInvalidRequestExceptionfor proper HTTP 400 responseBased on learnings about API Gateway event structure.
38-54: LGTM!The flag check implementation is solid:
- Request construction safely handles the context with
.get('context', {})- Proper delegation to the feature flag client
- Appropriate exception handling that preserves CCInvalidRequestException and wraps unexpected errors
- Simple response format matches the API contract
backend/compact-connect/lambdas/python/feature-flag/feature_flag_client.py
Show resolved
Hide resolved
backend/compact-connect/lambdas/python/feature-flag/tests/function/test_statsig_client.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
backend/compact-connect/lambdas/python/feature-flag/feature_flag_client.py(1 hunks)backend/compact-connect/lambdas/python/feature-flag/handlers/check_feature_flag.py(1 hunks)backend/compact-connect/lambdas/python/feature-flag/tests/function/test_statsig_client.py(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-22T21:04:46.552Z
Learnt from: landonshumway-ia
PR: csg-org/CompactConnect#1029
File: backend/compact-connect/lambdas/python/provider-data-v1/handlers/licenses.py:0-0
Timestamp: 2025-08-22T21:04:46.552Z
Learning: API Gateway always includes the 'body' key in Lambda events, even when the request body is empty/null. When event['body'] is None, json.loads(None) raises TypeError, not a missing key error.
Applied to files:
backend/compact-connect/lambdas/python/feature-flag/handlers/check_feature_flag.py
🧬 Code graph analysis (2)
backend/compact-connect/lambdas/python/feature-flag/tests/function/test_statsig_client.py (3)
backend/compact-connect/lambdas/python/feature-flag/feature_flag_client.py (11)
FeatureFlagException(144-145)FeatureFlagRequest(19-23)FeatureFlagValidationException(148-149)StatSigFeatureFlagClient(190-604)validate_request(52-63)check_flag(66-73)check_flag(264-294)upsert_flag(76-90)upsert_flag(337-370)delete_flag(103-113)delete_flag(522-564)backend/compact-connect/lambdas/python/feature-flag/tests/function/__init__.py (2)
TstFunction(14-23)setUp(17-23)backend/compact-connect/lambdas/python/feature-flag/tests/function/test_check_feature_flag.py (2)
setUp(14-27)_setup_mock_statsig(48-58)
backend/compact-connect/lambdas/python/feature-flag/handlers/check_feature_flag.py (3)
backend/compact-connect/lambdas/python/common/cc_common/exceptions.py (2)
CCInternalException(44-45)CCInvalidRequestException(7-8)backend/compact-connect/lambdas/python/common/cc_common/utils.py (2)
api_handler(90-212)get(86-87)backend/compact-connect/lambdas/python/feature-flag/feature_flag_client.py (6)
FeatureFlagRequest(19-23)FeatureFlagValidationException(148-149)create_feature_flag_client(607-618)validate_request(52-63)check_flag(66-73)check_flag(264-294)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: TestApp
- GitHub Check: TestApp
backend/compact-connect/lambdas/python/feature-flag/handlers/check_feature_flag.py
Show resolved
Hide resolved
landonshumway-ia
force-pushed
the
feat/feature-flag-framework
branch
from
9dde35b to
12dec5a
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
backend/compact-connect/lambdas/python/feature-flag/requirements-dev.txt (1)
16-16: Duplicate: Fix invalid cffi pinThis issue was already flagged in previous reviews. The pinned version
cffi==2.0.0doesn't exist on PyPI and must be corrected.
🧹 Nitpick comments (12)
backend/compact-connect/stacks/api_lambda_stack/feature_flags.py (1)
28-28: Fix typo in comments."StatsIg" should be "StatSig" in both comments.
Apply this diff:
- # Get the StatsIg secret for each environment + # Get the StatSig secret for each environment environment_name = self.stack.common_env_vars['ENVIRONMENT_NAME']- # Grant permission to read the StatsIg secret + # Grant permission to read the StatSig secret self.statsig_secret.grant_read(check_feature_flag_function)Also applies to: 59-59
backend/compact-connect/stacks/feature_flag_stack/__init__.py (1)
99-114: Consider removing the example flag from production code.The
example_flagresource is configured to auto-enable in all environments including PROD. While this demonstrates the feature flag framework, having an example flag deployed to production may not be desirable.Consider one of these approaches:
- Remove the example flag and add instructions in the docstring for creating flags
- Condition the example flag creation on non-production environments only
- Document that this example should be removed before production deployment
Example for conditional creation:
# Only create example flag in test/sandbox environments if environment_name in ['test', 'sandbox']: self.example_flag = FeatureFlagResource( self, 'ExampleFlag', provider=self.provider, flag_name='example-flag', auto_enable_envs=[FeatureFlagEnvironmentName.TEST, FeatureFlagEnvironmentName.SANDBOX], custom_attributes={'compact': ['aslp']}, environment_name=environment_name, )backend/compact-connect/lambdas/python/feature-flag/handlers/manage_feature_flag.py (2)
18-22: Add error handling for missing ENVIRONMENT_NAME.The
__init__method directly accessesos.environ['ENVIRONMENT_NAME']which will raiseKeyErrorif the variable is not set. While the Lambda environment should always set this, defensive error handling would provide clearer diagnostics.Apply this diff:
def __init__(self): super().__init__('ManageFeatureFlag') - self.environment = os.environ['ENVIRONMENT_NAME'] + self.environment = os.environ.get('ENVIRONMENT_NAME') + if not self.environment: + raise ValueError('ENVIRONMENT_NAME environment variable is required') # Create a StatSig client with console API access self.client = StatSigFeatureFlagClient(environment=self.environment)
50-53: Remove unreachableflag_datafalsy check
client.upsert_flagalways returns a dict or raises, so theif not flag_databranch on lines 50–53 is never executed—remove it.backend/compact-connect/lambdas/python/feature-flag/handlers/check_feature_flag.py (1)
9-10: Add error handling for client initialization failure.The feature flag client is initialized at module import time. If
create_feature_flag_clientraises an exception (e.g., missing credentials, invalid configuration), the entire Lambda module will fail to import, resulting in cryptic Lambda initialization errors rather than clear runtime errors.Consider one of these approaches:
Option 1: Lazy initialization with error handling
# Module-level variable _feature_flag_client = None def _get_client(): global _feature_flag_client if _feature_flag_client is None: try: _feature_flag_client = create_feature_flag_client(environment=config.environment_name) except Exception as e: logger.error('Failed to initialize feature flag client', exc_info=e) raise CCInternalException('Feature flag service unavailable') from e return _feature_flag_clientOption 2: Wrap module-level initialization
try: feature_flag_client = create_feature_flag_client(environment=config.environment_name) except Exception as e: logger.error('Failed to initialize feature flag client at module load', exc_info=e) # Set to None and check in handler feature_flag_client = NoneThen in the handler, check if client is None and raise a clear error.
backend/compact-connect/stacks/feature_flag_stack/feature_flag_resource.py (1)
54-55: Validate environment_name against enum values.While the code checks that
environment_nameis not empty, it doesn't validate that it's a valid environment name from theFeatureFlagEnvironmentNameenum. Invalid environment names would pass validation but could cause issues downstream.Apply this diff to add enum validation:
if not flag_name or not environment_name: raise ValueError('flag_name and environment_name are required') + + # Validate environment_name is a valid enum value + valid_envs = {env.value for env in FeatureFlagEnvironmentName} + if environment_name not in valid_envs: + raise ValueError( + f'environment_name must be one of {valid_envs}, got: {environment_name}' + )backend/compact-connect/lambdas/python/feature-flag/tests/function/test_statsig_client.py (2)
18-20: Avoid duplication of API constants; import from implementation.Define STATSIG_API_BASE_URL and STATSIG_API_VERSION by importing from feature_flag_client to prevent test drift when constants change.
-STATSIG_API_BASE_URL = 'https://statsigapi.net/console/v1' -STATSIG_API_VERSION = '20240601' +from feature_flag_client import STATSIG_API_BASE_URL, STATSIG_API_VERSION
83-84: Remove redundant mock; instance already stubbed.You stub the instance’s initialize() in _setup_mock_statsig. Stubbing mock_statsig.initialize here has no effect.
- self._setup_mock_statsig(mock_statsig) - mock_statsig.initialize.return_value = MagicMock() + self._setup_mock_statsig(mock_statsig)backend/compact-connect/lambdas/python/feature-flag/feature_flag_client.py (4)
378-383: Docstring out of date: custom_attributes applied regardless of auto_enable.Implementation builds conditions whenever custom_attributes is provided. Update the docstring accordingly.
- :param custom_attributes: Optional custom attributes for targeting (only applied if auto_enable=True) + :param custom_attributes: Optional custom attributes for targeting (applied when provided)
455-461: Docstring out of date: custom_attributes applied regardless of auto_enable.Align description with current behavior.
- :param custom_attributes: Optional custom attributes for targeting (only applied if auto_enable=True) + :param custom_attributes: Optional custom attributes for targeting (applied when provided)
366-371: Consider returning the updated gate after PATCH.upsert_flag returns the pre-update gate when adding an environment rule. For callers that need the final state (including the new rule), re-fetch and return the updated gate.
- if not environment_rule: - self._add_environment_rule(gate_id, existing_gate, auto_enable, custom_attributes) - - return existing_gate + if not environment_rule: + self._add_environment_rule(gate_id, existing_gate, auto_enable, custom_attributes) + # Optionally return updated state + refreshed = self.get_flag(flag_name) + if refreshed: + return refreshed + return existing_gate
510-519: Potential inefficiency and pagination risk in get_flag.Fetching all gates and filtering client-side may not scale and could miss results if paginated. Prefer a provider API that filters by name server-side (or handle pagination).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (39)
backend/compact-connect-ui-app/pipeline/frontend_pipeline.py(3 hunks)backend/compact-connect-ui-app/tests/app/test_pipeline.py(1 hunks)backend/compact-connect/README.md(3 hunks)backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py(1 hunks)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/privilege/record.py(2 hunks)backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/api.py(1 hunks)backend/compact-connect/lambdas/python/common/cc_common/feature_flag_client.py(1 hunks)backend/compact-connect/lambdas/python/common/common_test/test_data_generator.py(1 hunks)backend/compact-connect/lambdas/python/common/requirements-dev.in(1 hunks)backend/compact-connect/lambdas/python/common/tests/__init__.py(1 hunks)backend/compact-connect/lambdas/python/common/tests/unit/test_feature_flag_client.py(1 hunks)backend/compact-connect/lambdas/python/common/tests/unit/test_provider_record_util.py(8 hunks)backend/compact-connect/lambdas/python/data-events/handlers/encumbrance_events.py(1 hunks)backend/compact-connect/lambdas/python/data-events/tests/function/test_encumbrance_events.py(1 hunks)backend/compact-connect/lambdas/python/feature-flag/custom_resource_handler.py(1 hunks)backend/compact-connect/lambdas/python/feature-flag/feature_flag_client.py(1 hunks)backend/compact-connect/lambdas/python/feature-flag/handlers/check_feature_flag.py(1 hunks)backend/compact-connect/lambdas/python/feature-flag/handlers/manage_feature_flag.py(1 hunks)backend/compact-connect/lambdas/python/feature-flag/requirements-dev.in(1 hunks)backend/compact-connect/lambdas/python/feature-flag/requirements-dev.txt(1 hunks)backend/compact-connect/lambdas/python/feature-flag/requirements.in(1 hunks)backend/compact-connect/lambdas/python/feature-flag/requirements.txt(1 hunks)backend/compact-connect/lambdas/python/feature-flag/tests/__init__.py(1 hunks)backend/compact-connect/lambdas/python/feature-flag/tests/function/__init__.py(1 hunks)backend/compact-connect/lambdas/python/feature-flag/tests/function/test_check_feature_flag.py(1 hunks)backend/compact-connect/lambdas/python/feature-flag/tests/function/test_manage_feature_flag.py(1 hunks)backend/compact-connect/lambdas/python/feature-flag/tests/function/test_statsig_client.py(1 hunks)backend/compact-connect/lambdas/python/provider-data-v1/handlers/registration.py(1 hunks)backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_encumbrance.py(2 hunks)backend/compact-connect/pipeline/backend_pipeline.py(3 hunks)backend/compact-connect/pipeline/backend_stage.py(2 hunks)backend/compact-connect/stacks/api_lambda_stack/__init__.py(2 hunks)backend/compact-connect/stacks/api_lambda_stack/feature_flags.py(1 hunks)backend/compact-connect/stacks/api_stack/v1_api/api.py(3 hunks)backend/compact-connect/stacks/api_stack/v1_api/api_model.py(1 hunks)backend/compact-connect/stacks/api_stack/v1_api/feature_flags.py(1 hunks)backend/compact-connect/stacks/feature_flag_stack/__init__.py(1 hunks)backend/compact-connect/stacks/feature_flag_stack/feature_flag_resource.py(1 hunks)backend/compact-connect/tests/app/test_pipeline.py(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- backend/compact-connect/lambdas/python/data-events/tests/function/test_encumbrance_events.py
- backend/compact-connect/lambdas/python/feature-flag/requirements-dev.in
🚧 Files skipped from review as they are similar to previous changes (19)
- backend/compact-connect/lambdas/python/common/common_test/test_data_generator.py
- backend/compact-connect/lambdas/python/feature-flag/tests/init.py
- backend/compact-connect/lambdas/python/provider-data-v1/handlers/registration.py
- backend/compact-connect-ui-app/tests/app/test_pipeline.py
- backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/privilege/record.py
- backend/compact-connect/lambdas/python/common/cc_common/data_model/data_client.py
- backend/compact-connect/lambdas/python/feature-flag/tests/function/init.py
- backend/compact-connect/lambdas/python/common/cc_common/feature_flag_client.py
- backend/compact-connect/lambdas/python/feature-flag/tests/function/test_manage_feature_flag.py
- backend/compact-connect/lambdas/python/common/tests/unit/test_provider_record_util.py
- backend/compact-connect/stacks/api_lambda_stack/init.py
- backend/compact-connect/lambdas/python/data-events/handlers/encumbrance_events.py
- backend/compact-connect/lambdas/python/common/cc_common/data_model/schema/provider/api.py
- backend/compact-connect/stacks/api_stack/v1_api/feature_flags.py
- backend/compact-connect/lambdas/python/provider-data-v1/tests/function/test_handlers/test_encumbrance.py
- backend/compact-connect/lambdas/python/feature-flag/requirements.in
- backend/compact-connect/lambdas/python/feature-flag/custom_resource_handler.py
- backend/compact-connect/lambdas/python/common/requirements-dev.in
- backend/compact-connect/lambdas/python/feature-flag/requirements.txt
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-08-22T21:04:46.552Z
Learnt from: landonshumway-ia
PR: csg-org/CompactConnect#1029
File: backend/compact-connect/lambdas/python/provider-data-v1/handlers/licenses.py:0-0
Timestamp: 2025-08-22T21:04:46.552Z
Learning: API Gateway always includes the 'body' key in Lambda events, even when the request body is empty/null. When event['body'] is None, json.loads(None) raises TypeError, not a missing key error.
Applied to files:
backend/compact-connect/lambdas/python/feature-flag/handlers/check_feature_flag.py
📚 Learning: 2025-07-22T03:36:17.137Z
Learnt from: landonshumway-ia
PR: csg-org/CompactConnect#907
File: backend/compact-connect/lambdas/python/purchases/requirements-dev.txt:15-0
Timestamp: 2025-07-22T03:36:17.137Z
Learning: In CompactConnect, requirements-dev.txt files for Lambda functions are used exclusively for running tests and development, not for actual Lambda runtime environments. Concerns about runtime compatibility (like OpenSSL versions) don't apply to these development dependency files.
Applied to files:
backend/compact-connect/lambdas/python/feature-flag/requirements-dev.txt
📚 Learning: 2025-07-22T03:52:25.934Z
Learnt from: landonshumway-ia
PR: csg-org/CompactConnect#907
File: backend/compact-connect/lambdas/python/provider-data-v1/requirements.txt:2-2
Timestamp: 2025-07-22T03:52:25.934Z
Learning: In CompactConnect, the Python version used by pip-compile to generate requirements.txt files (shown in the header comment) is separate from the actual Lambda runtime environment. Dependencies are installed by a Python 3.12 container during the CI/CD pipeline, ensuring runtime compatibility regardless of the Python version used for pip-compile dependency resolution.
Applied to files:
backend/compact-connect/lambdas/python/feature-flag/requirements-dev.txt
📚 Learning: 2025-08-12T19:49:24.999Z
Learnt from: landonshumway-ia
PR: csg-org/CompactConnect#1001
File: backend/compact-connect/lambdas/python/disaster-recovery/requirements.in:1-1
Timestamp: 2025-08-12T19:49:24.999Z
Learning: In CompactConnect disaster-recovery Lambda functions, runtime dependencies like boto3, aws-lambda-powertools, and botocore are provided by lambda layers at deploy time rather than being specified in requirements.in files. The requirements.in file intentionally contains only a comment explaining this approach.
Applied to files:
backend/compact-connect/lambdas/python/feature-flag/requirements-dev.txt
📚 Learning: 2025-10-03T14:29:57.660Z
Learnt from: landonshumway-ia
PR: csg-org/CompactConnect#1110
File: backend/compact-connect/stacks/api_stack/v1_api/api_model.py:2708-2748
Timestamp: 2025-10-03T14:29:57.660Z
Learning: In backend/compact-connect/stacks/api_stack/v1_api/api_model.py, the check_feature_flag_request_model intentionally restricts customAttributes to string values only (additional_properties=JsonSchema(type=JsonSchemaType.STRING)), even though StatSig supports other types like booleans, numbers, and nested JSON. This is a conscious design choice to limit scope initially.
Applied to files:
backend/compact-connect/stacks/api_stack/v1_api/api_model.py
🧬 Code graph analysis (9)
backend/compact-connect/lambdas/python/feature-flag/handlers/check_feature_flag.py (3)
backend/compact-connect/lambdas/python/common/cc_common/exceptions.py (2)
CCInternalException(44-45)CCInvalidRequestException(7-8)backend/compact-connect/lambdas/python/common/cc_common/utils.py (2)
api_handler(90-212)get(86-87)backend/compact-connect/lambdas/python/feature-flag/feature_flag_client.py (6)
FeatureFlagRequest(19-23)FeatureFlagValidationException(148-149)create_feature_flag_client(607-618)validate_request(52-63)check_flag(66-73)check_flag(264-294)
backend/compact-connect/stacks/feature_flag_stack/__init__.py (2)
backend/compact-connect/common_constructs/python_function.py (1)
PythonFunction(21-155)backend/compact-connect/stacks/feature_flag_stack/feature_flag_resource.py (2)
FeatureFlagEnvironmentName(15-20)FeatureFlagResource(23-72)
backend/compact-connect/pipeline/backend_stage.py (3)
backend/compact-connect/stacks/feature_flag_stack/__init__.py (1)
FeatureFlagStack(84-206)backend/compact-connect/lambdas/python/common/cc_common/config.py (1)
environment_name(100-101)backend/compact-connect/tests/smoke/config.py (1)
environment_name(32-33)
backend/compact-connect/lambdas/python/feature-flag/handlers/manage_feature_flag.py (2)
backend/compact-connect/lambdas/python/feature-flag/custom_resource_handler.py (5)
CustomResourceHandler(18-124)CustomResourceResponse(10-15)on_create(90-99)on_update(102-111)on_delete(114-124)backend/compact-connect/lambdas/python/feature-flag/feature_flag_client.py (6)
FeatureFlagException(144-145)StatSigFeatureFlagClient(190-604)upsert_flag(76-90)upsert_flag(337-370)delete_flag(103-113)delete_flag(522-564)
backend/compact-connect/stacks/api_stack/v1_api/api.py (1)
backend/compact-connect/stacks/api_stack/v1_api/feature_flags.py (1)
FeatureFlagsApi(12-58)
backend/compact-connect/lambdas/python/feature-flag/tests/function/test_check_feature_flag.py (4)
backend/compact-connect/lambdas/python/feature-flag/tests/function/__init__.py (2)
TstFunction(14-23)setUp(17-23)backend/compact-connect/lambdas/python/feature-flag/tests/function/test_statsig_client.py (2)
setUp(26-35)_setup_mock_statsig(43-53)backend/compact-connect/lambdas/python/common/common_test/test_data_generator.py (1)
generate_test_api_event(33-61)backend/compact-connect/lambdas/python/feature-flag/handlers/check_feature_flag.py (1)
check_feature_flag(14-57)
backend/compact-connect/stacks/api_lambda_stack/feature_flags.py (2)
backend/compact-connect/common_constructs/python_function.py (1)
PythonFunction(21-155)backend/compact-connect/stacks/persistent_stack/__init__.py (1)
PersistentStack(38-510)
backend/compact-connect/lambdas/python/common/tests/unit/test_feature_flag_client.py (2)
backend/compact-connect/lambdas/python/common/tests/__init__.py (1)
TstLambdas(9-109)backend/compact-connect/lambdas/python/common/cc_common/feature_flag_client.py (3)
is_feature_enabled(46-112)FeatureFlagContext(17-43)to_dict(32-43)
backend/compact-connect/lambdas/python/feature-flag/tests/function/test_statsig_client.py (1)
backend/compact-connect/lambdas/python/feature-flag/feature_flag_client.py (11)
FeatureFlagException(144-145)FeatureFlagRequest(19-23)FeatureFlagValidationException(148-149)StatSigFeatureFlagClient(190-604)validate_request(52-63)check_flag(66-73)check_flag(264-294)upsert_flag(76-90)upsert_flag(337-370)delete_flag(103-113)delete_flag(522-564)
🪛 markdownlint-cli2 (0.18.1)
backend/compact-connect/README.md
309-309: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: TestApp
- GitHub Check: TestApp
🔇 Additional comments (10)
backend/compact-connect-ui-app/pipeline/frontend_pipeline.py (1)
236-237: Formatting changes are acceptable but unrelated to the PR objective.The changes in this file are purely stylistic:
- Line 236-237: Consolidated multi-line
Fn.joininto a single-line expression- Lines 265-269: Converted double-quoted strings to single-quoted (Python standard)
- Lines 285, 287: Minor bracket and comma placement adjustments
These have no functional impact and align with Python conventions. However, they appear unrelated to the feature flag framework described in the PR objectives. If this is part of a broader codebase formatting effort, consider applying similar consistency checks across all pipeline files in a dedicated formatting PR.
Also applies to: 265-269, 285-287
backend/compact-connect/tests/app/test_pipeline.py (1)
390-395: LGTM! Test assertion aligns with valid IAM trust policy structure.The nested
Principal: { Service: [...] }format is standard AWS IAM syntax and correctly asserts that pipeline roles trust both CodeBuild and CodePipeline services.backend/compact-connect/lambdas/python/common/tests/unit/test_feature_flag_client.py (1)
1-234: LGTM! Comprehensive test coverage.The test suite thoroughly covers the feature flag client functionality:
- Happy paths for enabled/disabled flags
- Context handling (user_id, custom_attributes, combinations)
- Fail-open and fail-closed behaviors across multiple error scenarios (timeout, HTTP errors, invalid responses, JSON parse errors)
- FeatureFlagContext serialization edge cases
All tests are well-structured with clear assertions and proper mocking.
backend/compact-connect/stacks/feature_flag_stack/__init__.py (2)
1-66: Excellent documentation.The module docstring provides comprehensive guidance on:
- Feature flag lifecycle (creation, updates, deletion)
- StatSig environment mapping
- Usage examples with code snippets
- Custom attributes documentation
This will be invaluable for developers working with feature flags.
132-136: Confirm Statsig secret naming and presence
Ensure the secretcompact-connect/env/{environment}/statsig/credentialsis created in AWS Secrets Manager for each target environment (test, beta, prod, sandbox) before deployment. Verify locally with:aws secretsmanager describe-secret --secret-id "compact-connect/env/${ENV}/statsig/credentials"backend/compact-connect/lambdas/python/feature-flag/handlers/manage_feature_flag.py (1)
89-96: LGTM: Graceful error handling.The deletion logic properly catches
FeatureFlagExceptionand logs without failing the deployment. This is the correct approach for custom resource deletion - ensuring CloudFormation can proceed even if the external resource cleanup fails.backend/compact-connect/lambdas/python/feature-flag/handlers/check_feature_flag.py (2)
30-39: LGTM: Proper error handling for request validation.The request body parsing correctly handles:
Noneor empty body (defaults to'{}')JSONDecodeError→CCInvalidRequestException(400)FeatureFlagValidationException→CCInvalidRequestException(400)This ensures clients receive appropriate 400 responses for malformed requests.
52-57: LGTM: Appropriate exception handling.The handler correctly:
- Re-raises
CCInvalidRequestExceptionto preserve 400 responses- Wraps unexpected exceptions in
CCInternalExceptionfor 500 responses- Logs errors before re-raising
This follows the exception handling pattern established by the
@api_handlerdecorator.backend/compact-connect/stacks/feature_flag_stack/feature_flag_resource.py (1)
60-60: Verify autoEnable logic and add tests
environment_name in auto_enable_envsleveragesStrEnumequality, which works, but no tests currently exercise this. Add unit tests covering matching and non-matching environments or normalize via[env.value for env in auto_enable_envs]before comparison.backend/compact-connect/lambdas/python/feature-flag/feature_flag_client.py (1)
240-245: Remove incorrect environment type suggestion StatsigOptions.environment expects a string tier (e.g., "production"), so the existing code is correct.Likely an incorrect or invalid review comment.
backend/compact-connect/lambdas/python/feature-flag/tests/function/test_check_feature_flag.py
Show resolved
Hide resolved
|
@jusdino I've added the needed email service updates to this PR to resolved the node audit check failures, let me know if you think we should break that out into it's own separate branch. |
jusdino
left a comment
jusdino
left a comment
There was a problem hiding this comment.
Thanks for handling this. Two quick comments:
backend/compact-connect/lambdas/nodejs/tests/lib/email/ingest-event-email-service.test.ts
Outdated
Show resolved
Hide resolved
|
@jlkravitz This is ready for your review. Before it can be merged. We will need you to create a StatSig Console API key used by the automation to create/delete feature gate resources in the StatSig account. Thanks. |
jlkravitz
left a comment
jlkravitz
left a comment
There was a problem hiding this comment.
Looks great. Just a few comments/questions!
Related to one of my other questions, I’m curious about the design choice here: What’s the rationale for putting feature flag evaluation behind its own Lambda + API endpoint instead of a thin shared wrapper module that calls Statsig directly throughout the codebase?
backend/compact-connect/lambdas/python/feature-flag/handlers/manage_feature_flag.py
Outdated
Show resolved
Hide resolved
backend/compact-connect/lambdas/python/feature-flag/tests/function/test_statsig_client.py
Outdated
Show resolved
Hide resolved
backend/compact-connect/lambdas/python/feature-flag/tests/function/test_statsig_client.py
Show resolved
Hide resolved
backend/compact-connect/lambdas/python/feature-flag/custom_resource_handler.py
Outdated
Show resolved
Hide resolved
backend/compact-connect/lambdas/python/feature-flag/feature_flag_client.py
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
backend/compact-connect/lambdas/python/feature-flag/custom_resource_handler.py (1)
18-28: Update class documentation to remove "migrations" terminology.The class docstring (line 19) still refers to "custom resource migrations," which is misleading. This base class is for general CloudFormation custom resources, not specifically for migrations.
Apply this diff to clarify the purpose:
class CustomResourceHandler(ABC): - """Base class for custom resource migrations. + """Base class for CloudFormation custom resources. This class provides a framework for implementing CloudFormation custom resources. It handles the routing of CloudFormation events to appropriate methods and provides a consistent logging pattern.
🧹 Nitpick comments (4)
backend/compact-connect/lambdas/python/feature-flag/feature_flag_client.py (2)
251-263: Consider documenting the default user ID choice.Line 254 uses
'default_cc_user'as the fallback when nouserIdis provided. Per the discussion in past reviews, this ensures consistent flag evaluation for anonymous/system requests (same user_id gets same result in percentage-based rollouts).Consider adding a brief comment explaining this design choice:
def _create_statsig_user(self, context: dict[str, Any]) -> StatsigUser: """Convert context dictionary to StatsigUser""" user_data = { + # Use a consistent default user ID for anonymous/system requests + # to ensure stable flag evaluation in percentage-based rollouts 'user_id': context.get('userId') or 'default_cc_user', }
115-141: Consider caching the Secrets Manager client.Line 126 creates a new boto3 session and Secrets Manager client on every call to
_get_secret. In Lambda environments with warm starts, this adds unnecessary overhead.Consider caching the client at the class level:
+ _secrets_client = None + def _get_secret(self, secret_name: str) -> dict[str, Any]: """ Retrieve a secret from AWS Secrets Manager and return it as a JSON object. @@ """ try: - # Create a Secrets Manager client - session = boto3.session.Session() - client = session.client(service_name='secretsmanager') + # Create a Secrets Manager client (cached) + if self._secrets_client is None: + session = boto3.session.Session() + self._secrets_client = session.client(service_name='secretsmanager') # Retrieve the secret value - response = client.get_secret_value(SecretId=secret_name) + response = self._secrets_client.get_secret_value(SecretId=secret_name)This optimization provides marginal performance benefits and can be skipped if simplicity is preferred.
backend/compact-connect/lambdas/python/feature-flag/custom_resource_handler.py (1)
89-99: Update method docstrings to remove "migration" terminology.Lines 93, 105, and 117 refer to "perform the migration" or "handle deletion of the migration." These methods handle custom resource lifecycle events, not migrations.
Apply this diff:
@abstractmethod def on_create(self, properties: dict) -> CustomResourceResponse | None: """Handle Create events. - This method should be implemented by subclasses to perform the migration when a resource is being created. + This method should be implemented by subclasses to handle resource creation. :param properties: The ResourceProperties from the CloudFormation event :type properties: dict :return: Any result to be returned to CloudFormation :rtype: Optional[CustomResourceResponse] """ @abstractmethod def on_update(self, properties: dict) -> CustomResourceResponse | None: """Handle Update events. - This method should be implemented by subclasses to perform the migration when a resource is being updated. + This method should be implemented by subclasses to handle resource updates. :param properties: The ResourceProperties from the CloudFormation event :type properties: dict :return: Any result to be returned to CloudFormation :rtype: Optional[CustomResourceResponse] """ @abstractmethod def on_delete(self, properties: dict) -> CustomResourceResponse | None: """Handle Delete events. - This method should be implemented by subclasses to handle deletion of the migration. In many cases, this can - be a no-op as the migration is temporary and deletion should have no effect. + This method should be implemented by subclasses to handle resource deletion. In many cases, this can + be a no-op if the resource cleanup is not required. :param properties: The ResourceProperties from the CloudFormation event :type properties: dict :return: Any result to be returned to CloudFormation :rtype: Optional[CustomResourceResponse] """backend/compact-connect/lambdas/python/feature-flag/handlers/manage_feature_flag.py (1)
47-54: Remove unreachable falsy check after upsert_flag Theupsert_flagclient method always returns a dict or raises, so theif not flag_databranch (warning log +return None) in manage_feature_flag.py is never reached—delete lines 51–53 or update the logic/comment accordingly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
backend/compact-connect/lambdas/python/feature-flag/custom_resource_handler.py(1 hunks)backend/compact-connect/lambdas/python/feature-flag/feature_flag_client.py(1 hunks)backend/compact-connect/lambdas/python/feature-flag/handlers/manage_feature_flag.py(1 hunks)backend/compact-connect/lambdas/python/feature-flag/tests/function/test_manage_feature_flag.py(1 hunks)backend/compact-connect/lambdas/python/feature-flag/tests/function/test_statsig_client.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/compact-connect/lambdas/python/feature-flag/tests/function/test_statsig_client.py
🧰 Additional context used
🧬 Code graph analysis (3)
backend/compact-connect/lambdas/python/feature-flag/custom_resource_handler.py (1)
backend/compact-connect/lambdas/python/feature-flag/handlers/manage_feature_flag.py (3)
on_create(24-61)on_update(63-70)on_delete(72-105)
backend/compact-connect/lambdas/python/feature-flag/tests/function/test_manage_feature_flag.py (3)
backend/compact-connect/lambdas/python/feature-flag/tests/function/test_statsig_client.py (2)
setUp(26-35)create_mock_secrets_manager(37-41)backend/compact-connect/lambdas/python/feature-flag/handlers/manage_feature_flag.py (3)
ManageFeatureFlagHandler(15-105)on_create(24-61)on_delete(72-105)backend/compact-connect/lambdas/python/feature-flag/feature_flag_client.py (4)
upsert_flag(76-90)upsert_flag(337-370)delete_flag(103-113)delete_flag(523-565)
backend/compact-connect/lambdas/python/feature-flag/handlers/manage_feature_flag.py (2)
backend/compact-connect/lambdas/python/feature-flag/custom_resource_handler.py (5)
CustomResourceHandler(18-124)CustomResourceResponse(10-15)on_create(90-99)on_update(102-111)on_delete(114-124)backend/compact-connect/lambdas/python/feature-flag/feature_flag_client.py (6)
FeatureFlagException(144-145)StatSigFeatureFlagClient(190-605)upsert_flag(76-90)upsert_flag(337-370)delete_flag(103-113)delete_flag(523-565)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: TestApp
🔇 Additional comments (3)
backend/compact-connect/lambdas/python/feature-flag/tests/function/test_manage_feature_flag.py (1)
26-31: LGTM! Tests provide good coverage of handler behavior.The test suite appropriately validates that the handler correctly invokes the feature flag client with the expected parameters and handles responses properly.
backend/compact-connect/lambdas/python/feature-flag/handlers/manage_feature_flag.py (1)
72-106: LGTM! Appropriate error handling for deletion.The deletion handler correctly:
- Validates required properties
- Swallows exceptions to prevent CloudFormation stack deletion failures
- Provides clear logging for different deletion outcomes
backend/compact-connect/lambdas/python/feature-flag/custom_resource_handler.py (1)
40-88: LGTM! Well-structured event routing with appropriate error handling.The implementation correctly:
- Routes CloudFormation events to appropriate handlers
- Provides structured logging with Lambda context
- Catches and logs errors before re-raising (ensuring CloudFormation sees failures)
- Uses Python 3.10+ match statement for clean request type routing
backend/compact-connect/lambdas/python/feature-flag/feature_flag_client.py
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
backend/compact-connect/stacks/api_stack/v1_api/feature_flags.py (1)
30-40: Consider adding request validation and error response models.The endpoint could benefit from:
- Attaching a request validator to validate request bodies against the schema
- Defining error response models (400, 500) for better API documentation and error handling contracts
Example enhancement:
self.check_flag_method = check_resource.add_method( 'POST', integration=LambdaIntegration(api_lambda_stack.feature_flags_lambdas.check_feature_flag_function), request_models={'application/json': api_model.check_feature_flag_request_model}, + request_validator=self.api.parameter_body_validator, method_responses=[ { 'statusCode': '200', 'responseModels': {'application/json': api_model.check_feature_flag_response_model}, }, + { + 'statusCode': '400', + 'responseModels': {'application/json': self.api.error_response_model}, + }, + { + 'statusCode': '500', + 'responseModels': {'application/json': self.api.error_response_model}, + }, ], )Note: Verify that
self.api.error_response_modelexists in the CCApi class, or define appropriate error models in ApiModel.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/compact-connect/stacks/api_stack/v1_api/feature_flags.py(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-06T23:29:03.856Z
Learnt from: landonshumway-ia
PR: csg-org/CompactConnect#1110
File: backend/compact-connect/stacks/api_stack/v1_api/feature_flags.py:24-40
Timestamp: 2025-10-06T23:29:03.856Z
Learning: In the CompactConnect codebase, API Gateway endpoint classes access the CCApi instance via `resource.api` (not `resource.rest_api`), and api_model schema properties are accessed without parentheses (as properties, not method calls). This pattern is consistent across all v1_api endpoint implementations (provider_users.py, staff_users.py, purchases.py, etc.).
Applied to files:
backend/compact-connect/stacks/api_stack/v1_api/feature_flags.py
🧬 Code graph analysis (1)
backend/compact-connect/stacks/api_stack/v1_api/feature_flags.py (3)
backend/compact-connect/common_constructs/cc_api.py (1)
CCApi(66-326)backend/compact-connect/stacks/api_lambda_stack/__init__.py (1)
ApiLambdaStack(13-49)backend/compact-connect/stacks/api_stack/v1_api/api_model.py (2)
check_feature_flag_request_model(2708-2741)check_feature_flag_response_model(2744-2763)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: TestApp
🔇 Additional comments (1)
backend/compact-connect/stacks/api_stack/v1_api/feature_flags.py (1)
42-56: ```bash
#!/bin/bashSearch for any WAF (v2) or Web ACL associations across the repo
echo "=== WAF/WebACL references ==="
rg -nE "waf|WAF|Wafv2|CfnWebACL|WebACL" -C3 .echo -e "\n=== Throttling and rate limits across the repo ==="
rg -nE "throttle|ThrottleSettings|burstLimit|rateLimit|defaultMethodOptions" -C3 .echo -e "\n=== Inspect CCApi construct for stageOptions or defaultMethodOptions ==="
rg -nP 'class\s+CCApi\b' -A20 backend/compact-connect/common_constructs/cc_api.py
rg -n "stageOptions" -C3 backend/compact-connect/common_constructs/cc_api.py</blockquote></details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
jlkravitz
left a comment
jlkravitz
left a comment
There was a problem hiding this comment.
@isabeleliassen Good to merge!
4 participants
Now that the CompactConnect project is live for users, we need a process by which we can safely roll out new features into production, and quickly rollback features should they cause unexpected issues. This adds a feature flag framework for the project, which internally works with StatSig (https://statsig.com/featureflags) to manage feature gates and check when they are enabled or disabled.
Flags are intended to be created/deleted through CDK custom resources. Flags can be set to auto enabled for an environment, meaning they will automatically be made live in the environment as they are deployed, else they will need to be manually enabled through the StatSig account console.
For checking if a feature flag is enabled, we have added a public API endpoint, where the caller passes in the flag name and any custom attributes relative to the feature, and the API returns a bool value based on if the flag is enabled or disabled. This pattern allows any of our backend services to check for feature flags.
Closes #1095
Summary by CodeRabbit
New Features
Schema
Tests
Chores