Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughRenames project identifiers from "oscm" to "fund" and adds GitHub Actions OIDC integration, Secrets Manager secrets, IAM roles/policies, encrypted/access-blocked S3 buckets, CloudWatch NAT alarm, SNS topic and budget alerts, secret-config helpers, tests, and a GitHub runbook. No application control-flow behavior changes beyond naming and secret wiring. Changes
Sequence DiagramssequenceDiagram
participant GHA as GitHub Actions
participant OIDC as AWS OIDC Provider
participant IAM as AWS IAM
participant Pulumi as Pulumi Runner
GHA->>OIDC: Request ID token (repo, ref, workflow_ref, sub)
OIDC->>OIDC: Validate OIDC claim constraints
OIDC->>IAM: Assume infrastructure role via token
IAM->>GHA: Return temporary credentials (STS)
GHA->>Pulumi: Use credentials to run deployment
sequenceDiagram
participant Config as Pulumi Config
participant Infra as infrastructure/__main__.py
participant Secrets as AWS Secrets Manager
participant AWS as AWS Services
participant Output as Pulumi Exports
Config->>Infra: Load fund secure config (region, secret objects)
Infra->>Infra: validate & serialize secret config objects
Infra->>Secrets: Create Secret & SecretVersion resources
Secrets-->>Infra: Return secret ARNs
Infra->>AWS: Create S3 (encrypted, block public), IAM roles/policies, CloudWatch alarm, SNS topic, Budgets
AWS-->>Infra: Resource ARNs/IDs
Infra->>Output: Export GitHub Actions role & OIDC provider ARNs and other outputs
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Contributor
Greptile OverviewGreptile SummaryThis PR refactors the Pulumi infrastructure program and production stack config: it introduces stricter secret config validation, replaces a single SecretsManager secret with per-service secrets, adds budget alerting via SNS/Budgets, enables S3 bucket encryption and public-access blocks, adds a NAT Gateway CloudWatch alarm, and provisions a GitHub Actions OIDC provider + least-privilege deploy role/policy. It also renames the Pulumi project and adds a short runbook plus regression-style tests that assert key security/infra invariants. Main issue to address before merge: the Pulumi config namespace changed ( Confidence Score: 3/5
|
| Filename | Overview |
|---|---|
| .gitignore | Adds common local artifacts to ignore (coverage/, node_modules/, bun.lock, .pi). No functional code impact. |
| infrastructure/Pulumi.production.yaml | Reworks production stack config to include GitHub/OIDC/budget/secrets settings and encrypts aws:region; currently mismatched with main.py config namespace expectations. |
| infrastructure/Pulumi.yaml | Renames Pulumi project from oscm to oscmcompany; this changes config key namespaces and must match pulumi.Config() usage in code/config. |
| infrastructure/main.py | Large refactor adding secret config validation, new SecretsManager resources, budget alerts, S3 encryption/public access blocks, NAT alarm, and GitHub OIDC role/policy; introduces at least one deploy-blocking issue (config namespace mismatch) and a likely fragile parameters import depending on working directory. |
| infrastructure/github_environment_runbook.md | Adds runbook for required GitHub environment secrets and updating AWS_IAM_INFRASTRUCTURE_ROLE_ARN; documentation-only. |
| libraries/python/tests/test_infrastructure_configuration.py | Adds string-based tests enforcing secrets in Pulumi.production.yaml and presence of OIDC/alarm/encryption resources; tests are brittle to formatting/renames but generally OK. |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Pull request overview
This PR performs infrastructure hardening and configuration cleanup for the Pulumi “production” stack, adding guardrails around secret handling and introducing a GitHub Actions OIDC role/policy to support CI-driven infrastructure deployments.
Changes:
- Refactors
infrastructure/__main__.pyto enforce secret config usage, split secrets into multiple Secrets Manager secrets, and add budget/SNS notifications plus additional baseline security resources (S3 encryption/public access blocks, NAT gateway alarm). - Renames the Pulumi project to
oscmcompanyand updates the production stack config accordingly. - Adds a GitHub environment runbook and a Python test suite that asserts key security/config invariants.
Reviewed changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
libraries/python/tests/test_infrastructure_configuration.py |
Adds regression tests asserting infra config/security expectations (secret config, OIDC constraints, absence of admin access, etc.). |
infrastructure/github_environment_runbook.md |
Documents required GitHub environment secrets and how to update role ARN from Pulumi outputs. |
infrastructure/__main__.py |
Major infra refactor: secret config enforcement, new IAM OIDC role/policy, Secrets Manager updates, S3 hardening, alarms, budgets, and new exports. |
infrastructure/Pulumi.yaml |
Renames the Pulumi project to oscmcompany. |
infrastructure/Pulumi.production.yaml |
Updates production config keys/values to match the renamed project and new required settings (mostly secret). |
.gitignore |
Ignores coverage output and some additional local/tooling artifacts. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Fix all issues with AI agents
In @.gitignore:
- Around line 25-30: Remove the duplicate coverage/ line from .gitignore (keep
the original occurrence) and either delete or correct the unusual ".pi"
pattern—verify whether it was meant to be ".pnp", ".pixi", or another
filename/pattern and update it accordingly; ensure only intentional ignore
entries remain (refer to the existing "coverage/" entry and the ".pi" token in
the diff when editing).
In `@infrastructure/__main__.py`:
- Line 106: The dynamic import_module("parameters") call in __main__.py is
fragile because it relies on the current working directory; replace it with a
resilient approach: either use a package-relative import (e.g., import
parameters via a relative import inside the infrastructure package) or
explicitly prepend the infrastructure directory to sys.path using
Path(__file__).resolve().parent (so the parameters module is found regardless of
CWD) and then import_module("parameters"); update the code that assigns to the
parameters variable accordingly (reference: import_module("parameters") and the
parameters variable in __main__.py).
- Around line 928-938: The IAM statement with Sid
"ManageEc2EcsElbBudgetsAndServiceDiscovery" uses overly-broad wildcards ("ec2:*"
and "ecs:*" on Resource: "*"); replace these with a minimal enumerated set of
actions required for VPC/subnet/networking and ECS service management (e.g.,
restrict to ec2 actions like ec2:CreateTags, ec2:DeleteTags and category
patterns such as ec2:*Vpc*, ec2:*Subnet*, ec2:*Route*, ec2:*SecurityGroup*,
ec2:*NatGateway*, ec2:*InternetGateway*, ec2:*Eip*, ec2:*VpcEndpoint* and
similarly enumerate needed ECS actions rather than "ecs:*"), keep
elasticloadbalancing, budgets, servicediscovery as scoped as-needed, and
document the exact action list in the policy for the Sid
"ManageEc2EcsElbBudgetsAndServiceDiscovery".
- Around line 218-233: The budget notifications currently only trigger when
spend exceeds monthly_budget_limit_usd because both
aws.budgets.BudgetNotificationArgs entries use
comparison_operator="GREATER_THAN" at 100%; add an early-warning notification by
inserting another aws.budgets.BudgetNotificationArgs in the notifications list
that uses comparison_operator="GREATER_THAN", notification_type="ACTUAL" (and/or
"FORECASTED" if desired), threshold set to monthly_budget_limit_usd * 0.8 (80%),
threshold_type="ABSOLUTE_VALUE", and
subscriber_email_addresses=budget_alert_email_addresses so recipients get an 80%
alert before the limit is exceeded.
In `@infrastructure/github_environment_runbook.md`:
- Around line 11-21: The runbook currently documents only
AWS_IAM_INFRASTRUCTURE_ROLE_ARN; expand it to include instructions for setting
the other three secrets by showing how to read Pulumi outputs and set GH
secrets: reference the Pulumi outputs aws_s3_data_bucket (or a dedicated
artifacts output) to populate AWS_S3_ARTIFACTS_BUCKET_NAME, reference
aws_iam_github_actions_oidc_provider_arn to document the new OIDC provider ARN
output, and add guidance to source AWS_REGION (from your AWS config or Pulumi
stack config) and PULUMI_ACCESS_TOKEN (from Pulumi service dashboard or
environment) when running gh secret set; ensure you use the GitHub Actions
environment flag (--env pulumi) and mirror the same pattern used for
AWS_IAM_INFRASTRUCTURE_ROLE_ARN so maintainers can run pulumi stack output
<output-name> and gh secret set <SECRET_NAME> --env pulumi --body "<value>".
- Around line 5-9: Add a blank line between the heading line "Required
environment secrets:" and the subsequent bullet list (the lines with `- ` items
such as `AWS_IAM_INFRASTRUCTURE_ROLE_ARN`, `AWS_REGION`, `PULUMI_ACCESS_TOKEN`,
`AWS_S3_ARTIFACTS_BUCKET_NAME`) so the list is separated from the heading and
MD032 is resolved; update the markdown around the "Required environment
secrets:" header to insert an empty line before the first `-` entry.
In `@libraries/python/tests/test_infrastructure_configuration.py`:
- Around line 33-34: The test currently contains hardcoded personal emails in
the assertions; update the assertions to avoid storing PII by checking for a
generic pattern instead. Replace the two explicit checks that reference the
literal addresses with a single assertion against production_stack_config that
no plaintext email-like patterns exist (e.g., assert no occurrence of the regex
/@[a-z0-9.-]+\.[a-z]{2,}/ or at minimum assert '"@gmail.com"' not in
production_stack_config); keep the check operating on the same variable
production_stack_config so the intent (no plaintext emails) is preserved without
embedding specific personal addresses.
…quality fixes
Fixed 11 code quality and documentation issues identified by automated reviewers:
**Infrastructure Configuration (__main__.py)**:
- Fixed config namespace mismatch by using pulumi.Config("oscmcompany") for explicit namespace
- Replaced dynamic import_module("parameters") with static import for CWD-independent resolution
- Added sort_keys=True to all json.dumps calls for deterministic IAM policy serialization
- Maintained string literals in typing.cast per Ruff TC006 rule for runtime safety
**Documentation (github_environment_runbook.md)**:
- Fixed MD032 markdown lint error by adding blank line before bullet list
- Capitalized "Pulumi" product name in title
- Added complete update instructions for all 4 required GitHub secrets (AWS_REGION, PULUMI_ACCESS_TOKEN, AWS_S3_ARTIFACTS_BUCKET_NAME)
**Test Suite (test_infrastructure_configuration.py)**:
- Removed hardcoded personal email addresses from test assertions to eliminate PII from source code
- Preserved security validation intent by keeping secure: key verification
**.gitignore Cleanup**:
- Removed duplicate coverage/ entry (line 26)
- Removed .pi pattern (confirmed as typo)
**Not Addressed (Justified)**:
- Budget notification thresholds: AWS Budgets API behavior, percentage-based warnings out of scope
- Broad ec2:*/ecs:* permissions: Justified for infrastructure role with strict OIDC claim constraints
- Exact string-matching tests: Intentional for infrastructure security validation
All Python and Markdown checks pass. Test coverage: 94%.
Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Contributor
Greptile OverviewGreptile SummaryThis PR renames the Pulumi project to Before merging, a few references need to be aligned so deploys and CI don’t break:
Non-functional changes (.flox profile sourcing, .gitignore tweaks, maskfile updates) look consistent with the infra rename, but the above mismatches should be fixed to avoid immediate failures. Confidence Score: 2/5
|
| Filename | Overview |
|---|---|
| .flox/env/manifest.lock | Adds a Flox profile hook mirroring the existing on-activate .env loading; no functional issues spotted. |
| .flox/env/manifest.toml | Adds a [profile].common script to source .env, matching existing hook behavior; no issues found. |
| .gitignore | Adjusts coverage ignore patterns to .coverage_output/ and .coverage*; no issues found. |
| infrastructure/Pulumi.production.yaml | Moves several config values to encrypted secrets and renames keys under the 'fund' project namespace; appears to contain key name mismatches with infrastructure/main.py (e.g., sharedSecretsName vs sharedSecretName, datamanagerSecretsName vs datamanagerSecretName). |
| infrastructure/Pulumi.yaml | Renames Pulumi project to 'fund' with uv toolchain; consistent with code's pulumi.Config("fund"). |
| infrastructure/main.py | Major infra refactor adding OIDC role/policy and secret enforcement; contains deploy-breaking config key mismatches with Pulumi.production.yaml and the runbook stack path appears inconsistent with new project name. |
| infrastructure/github_environment_runbook.md | Adds runbook for setting GitHub environment secrets, but stack selection uses 'oscmcompany/fund/production' which likely doesn't match current Pulumi.yaml project name and typical org/project/stack format. |
| libraries/python/tests/test_infrastructure_configuration.py | Adds tests that assert literal strings in Pulumi configs/entrypoint; currently inconsistent with Pulumi.production.yaml ('fund:' vs 'oscmcompany:'), so tests will fail. |
| maskfile.md | Updates ECR and service/secret naming and Pulumi org lookup; needs a quick check that output key names (e.g., psf_base_url vs oscm_base_url) remain correct but no new definite breakages found in edited hunks. |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 9 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
Greptile OverviewGreptile SummaryThis PR updates infrastructure configuration to use the Critical deployment-blocking issues:
Positive changes:
Confidence Score: 0/5
|
| Filename | Overview |
|---|---|
| infrastructure/Pulumi.production.yaml | Critical deployment-blocking config key mismatches (sharedSecretsName vs sharedSecretName, datamanagerSecretsName vs datamanagerSecretName, portfoliomanagerSecretsName vs portfoliomanagerSecretName) |
| infrastructure/Pulumi.yaml | Updated project name from oscm to fund - clean change |
| infrastructure/main.py | Extensive infrastructure enhancements with secret validation, GitHub OIDC, least-privilege IAM policies, S3 encryption, CloudWatch alarms, and budget alerts |
| libraries/python/tests/test_infrastructure_configuration.py | Test suite with wrong namespace assertion (oscmcompany: instead of fund:), will fail on CI |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Fix all issues with AI agents
In @.flox/env/manifest.toml:
- Around line 55-62: The `[profile].common` block duplicates the `.env` sourcing
logic already present in `[hook].on-activate`; extract that logic into a single
shared script (for example, a small shell file like flox_env_source.sh) that
performs the `if [[ -f .env ]]; then set -a; source .env; set +a; fi` check,
then update both `[profile].common` and `[hook].on-activate` to simply source
that shared script, removing the duplicated inline block so the behavior remains
identical but maintained in one place.
In @.gitignore:
- Around line 12-13: Replace the overly broad ignore patterns: remove the
redundant `.coverage_output/` entry and replace `.coverage*` with two specific
patterns—`.coverage` and `.coverage.*`—so that runtime coverage files like
`.coverage` and `.coverage.<suffix>` are ignored but `.coveragerc` remains
tracked.
In `@infrastructure/__main__.py`:
- Around line 89-91: The config key strings passed to stack_config.require are
inconsistent with the Pulumi YAML; update the string literals used where
datamanager_secrets_name and shared_secrets_name are set so they match the YAML
keys (change "datamanagerSecretName" to "datamanagerSecretsName" and
"sharedSecretName" to "sharedSecretsName") while leaving
portfoliomanager_secrets_name as-is; use the existing stack_config.require calls
to make the correction.
In `@infrastructure/Pulumi.production.yaml`:
- Around line 6-14: The YAML keys for secrets use plural names
(fund:datamanagerSecretsName and fund:sharedSecretsName) but the code in
infrastructure/__main__.py calls stack_config.require("datamanagerSecretName")
and stack_config.require("sharedSecretName"); fix by making names consistent:
either rename the YAML keys to singular (fund:datamanagerSecretName and
fund:sharedSecretName) to match the existing calls (recommended for consistency
with fund:portfoliomanagerSecretName), or change the code to require the plural
keys (update stack_config.require calls to "datamanagerSecretsName" and
"sharedSecretsName"); adjust whichever option you choose for both secret name
and secret value keys to keep naming consistent.
In `@infrastructure/Pulumi.yaml`:
- Line 1: The Pulumi project name was changed from "oscm" to "fund" (the YAML
key "name: fund"), which breaks existing stacks; either revert the change to
keep "name: oscm" or migrate each existing oscm/<stack> stack to the new project
by exporting and re-importing state (e.g., pulumi stack export -> modify
exported state project name to "fund" -> pulumi stack import or recreate stacks)
so Pulumi does not treat them as new projects; ensure all existing stacks are
migrated or recreated before deploying with "name: fund".
In `@libraries/python/tests/test_infrastructure_configuration.py`:
- Around line 25-32: The test
test_production_stack_config_stores_budget_alert_emails_as_secret is asserting
the old Pulumi config namespace; update the assertions that reference
load_production_stack_config() to use the new "fund:" prefix instead of
"oscmcompany:" (i.e., assert "fund:budgetAlertEmailAddresses:" is in
production_stack_config and assert the indented secure line contains "
fund:budgetAlertEmailAddresses:\n secure:").
- Around line 96-101: The test
test_infrastructure_entrypoint_scopes_oidc_provider_creation_statement contains
a stale negative assertion for the string "CreateIamResourcesForOscmStack"; open
that test and remove the assertion line asserting
'"CreateIamResourcesForOscmStack"' is not in infrastructure_entrypoint (or
replace it with an assertion for the current equivalent resource SID if you know
the new name). Locate the test via the function
test_infrastructure_entrypoint_scopes_oidc_provider_creation_statement and the
call to load_infrastructure_entrypoint to update the assertions accordingly.
Contributor
Greptile OverviewGreptile SummaryThis PR enhances infrastructure security by encrypting sensitive configuration values and adding comprehensive validation. However, critical configuration key naming mismatches between
Confidence Score: 0/5
|
| Filename | Overview |
|---|---|
| infrastructure/Pulumi.production.yaml | Config key naming mismatches will cause deployment failure (sharedSecretsName vs sharedSecretName, datamanagerSecretsName vs datamanagerSecretName) |
| libraries/python/tests/test_infrastructure_configuration.py | Test expects wrong namespace (oscmcompany:budgetAlertEmailAddresses instead of fund:budgetAlertEmailAddresses), will fail immediately |
| infrastructure/main.py | Enhanced security with config validation, OIDC constraints, and encryption requirements; import path may fail depending on cwd |
Last reviewed commit: dbb07c4
…t failures Root cause: The Pulumi project refactor from oscm/oscmcompany to fund namespace introduced several inconsistencies between configuration files, code, and tests that caused CI failures and would have caused deployment failures. Changes made: - Updated infrastructure/__main__.py to use singular SecretValue config keys (datamanagerSecretValue, portfoliomanagerSecretValue, sharedSecretValue) matching the manually-corrected Pulumi.production.yaml configuration - Fixed test assertions in test_infrastructure_configuration.py to use fund: namespace instead of oscmcompany: namespace - Fixed test SID assertion to check for CreateGithubActionsOIDCProvider with correct all-caps OIDC matching the actual infrastructure code - Fixed Rust LocalStack test failure by storing container reference in static to preserve port mapping throughout test execution - Removed duplicate .env sourcing from flox manifest [profile].common section, keeping only [hook].on-activate for proper environment setup during activation - Updated github_environment_runbook.md to use dynamic Pulumi organization via pulumi org get-default instead of hardcoded oscmcompany - Fixed GitHub Actions Rust cache save condition to allow caching on any master branch update, not just push events, resolving cache miss issues Verification: - All Python tests now pass (77 tests, 94% coverage) - All Rust tests now pass (23 tests passing) - Posted responses to all 23 unresolved PR review threads and resolved them - Handled 1 outdated thread about .gitignore Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Contributor
Greptile OverviewGreptile SummaryThis PR successfully completes the infrastructure refactoring from Security improvements:
Naming consistency:
Testing and documentation:
All previously identified issues from review threads have been addressed: config namespace matches code expectations, environment variables are consistent across infrastructure and application code, and tests validate the correct Confidence Score: 5/5
|
| Filename | Overview |
|---|---|
| infrastructure/Pulumi.production.yaml | Added encrypted secrets for AWS region, budget alerts, and service secrets using fund: namespace; all sensitive values properly secured |
| infrastructure/main.py | Major infrastructure update: added OIDC authentication, least-privilege IAM policies, budget monitoring, NAT gateway alarms, S3 encryption, and updated all naming from oscm to fund; environment variables properly renamed to FUND_* prefix |
| infrastructure/github_environment_runbook.md | New documentation for GitHub environment setup with commands to extract and configure AWS IAM role ARN, region, and Pulumi tokens |
| libraries/python/tests/test_infrastructure_configuration.py | New test suite verifying infrastructure security controls: encrypted config values, OIDC constraints, no AdministratorAccess usage, NAT monitoring, and S3 encryption |
| maskfile.md | Updated all references from oscm/oscmcompany to fund: Docker buildx builder names, ECR repository paths, service names, Pulumi stack selection, and secrets manager paths |
Last reviewed commit: 1cd3839
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 15 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Root cause: GitHub Actions for PR #754 were timing out at ~40 minutes with exit code 143 (SIGTERM). Investigation revealed two issues: 1. DuckDB bundled feature compiled entire C++ library from scratch (~9+ minutes) 2. Cache save-if condition required master branch, but workflow only ran on pull_request events 3. No master cache existed, so every PR compiled all dependencies from scratch Changes made: - Remove bundled feature from duckdb dependency in datamanager Cargo.toml - Add DuckDB system library installation step to GitHub workflow (matches Dockerfile approach) - Add push trigger for master branch to enable gold standard cache strategy - Add explicit 60-minute timeout to prevent ambiguous failures Expected impact: - Eliminates ~9 minutes of DuckDB C++ compilation per run - Enables master branch to save cache after PR merges - Future PRs will restore master cache and complete in ~10-15 minutes - Aligns CI environment with production Docker builds Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Contributor
Greptile OverviewGreptile SummaryThis PR successfully implements a comprehensive infrastructure cleanup, renaming the project from Key improvements:
Application updates:
The PR addresses previous feedback and all configuration namespaces now align correctly between Confidence Score: 5/5
|
| Filename | Overview |
|---|---|
| infrastructure/Pulumi.production.yaml | Added encrypted configuration values with proper fund: namespace and security controls |
| infrastructure/main.py | Major infrastructure improvements: OIDC auth, budget alerts, S3 encryption, NAT monitoring, and proper IAM policies with least privilege |
| infrastructure/github_environment_runbook.md | Added documentation for GitHub environment secrets setup |
| libraries/python/tests/test_infrastructure_configuration.py | New test suite validating infrastructure security controls and encrypted config |
| applications/portfoliomanager/src/portfoliomanager/server.py | Updated environment variables from OSCM_ to FUND_ prefix |
| applications/equitypricemodel/src/equitypricemodel/server.py | Updated OSCM_DATAMANAGER_BASE_URL to FUND_DATAMANAGER_BASE_URL and improved validation error handling |
Last reviewed commit: d1bf47a
…robustness Fixed four security and code quality issues identified in PR review: 1. Secret handling for AWS region - Changed from require() to require_secret() to ensure proper Pulumi secret tracking throughout execution and prevent region value from leaking in previews/logs/state outputs. 2. Secret handling for budget email addresses - Reordered validation to check secret status first, then load using require_secret_object() instead of require_object() to maintain secret tracking and prevent email addresses from appearing in plaintext in previews. 3. Brittle YAML whitespace tests - Replaced exact whitespace matching with regex patterns that tolerate indentation variations, making tests more robust to harmless YAML reformatting. 4. Runbook secret retrieval - Added --show-secrets flag to pulumi config get command to ensure actual region value is retrieved instead of redacted placeholder when setting GitHub environment variable. All local Python checks passing (formatting, linting, type checking, tests with 94% coverage). Changes maintain backward compatibility with existing infrastructure configuration. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Contributor
Greptile OverviewGreptile SummaryThis PR successfully implements infrastructure cleanups focused on security hardening, configuration management, and documentation improvements. The changes rename the project from Key improvements:
Verification: Confidence Score: 5/5
|
| Filename | Overview |
|---|---|
| infrastructure/Pulumi.production.yaml | Config keys properly encrypted and namespace updated to fund:. All sensitive values are now stored as secure secrets. |
| infrastructure/main.py | Comprehensive infrastructure code with proper OIDC constraints, budget alerts, encryption, and security controls. Config namespace correctly uses fund. |
| libraries/python/tests/test_infrastructure_configuration.py | Test suite correctly validates infrastructure security controls, encryption, OIDC constraints, and config namespace (fund:). |
| infrastructure/github_environment_runbook.md | Documentation for GitHub environment setup with updated commands using dynamic org resolution. |
| applications/equitypricemodel/src/equitypricemodel/server.py | Environment variable references updated to use FUND_ prefix. |
| applications/portfoliomanager/src/portfoliomanager/risk_management.py | Risk management module updated with FUND_ prefix for environment variables. |
| applications/portfoliomanager/src/portfoliomanager/server.py | Portfolio manager server updated with FUND_ environment variable prefix. |
Last reviewed commit: 1a62618
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 17 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…st flakiness Root cause analysis: 1. Region was incorrectly loaded as a Pulumi Output using require_secret() when it doesn't need encryption 2. Secret names were loaded without require_secret() despite being encrypted in config 3. GitHub Actions IAM policy used secret names directly in json.dumps() without Output.all().apply() 4. LocalStack container test was timing out due to default wait conditions Fixes implemented: - Decrypted aws:region in Pulumi.production.yaml (not sensitive data) - Changed region from require_secret() to require() making it a plain string - Changed secret names to require_secret() to properly decrypt config values - Wrapped GitHub Actions policy in Output.all().apply() for three secret name Outputs - Fixed task_role_ssm_policy to use plain strings (region and account_id are strings in Python Pulumi) - Added 5-second sleep after LocalStack container startup for service initialization - Updated test to verify region is stored as plaintext not encrypted All Python and Rust checks now pass successfully. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Contributor
Greptile OverviewGreptile SummaryThis PR successfully completes the infrastructure naming migration from Key improvements:
Naming consistency verified across:
Additional fixes:
Confidence Score: 5/5
|
| Filename | Overview |
|---|---|
| infrastructure/main.py | Major refactor: added OIDC authentication, secret management, budget monitoring, encrypted S3 buckets, and renamed all resources from oscm to fund prefix |
| infrastructure/Pulumi.yaml | Updated project name from oscm to fund |
| infrastructure/Pulumi.production.yaml | Added encrypted configuration for GitHub OIDC, budget alerts, and secret management with fund: namespace |
| infrastructure/github_environment_runbook.md | New documentation for GitHub environment setup with Pulumi outputs |
| libraries/python/tests/test_infrastructure_configuration.py | New test suite validating infrastructure security controls, encryption, and OIDC configuration |
| applications/portfoliomanager/src/portfoliomanager/server.py | Updated environment variable names from OSCM_ to FUND_ prefix |
| applications/equitypricemodel/src/equitypricemodel/server.py | Updated environment variable to FUND_DATAMANAGER_BASE_URL and improved prediction validation error handling |
| maskfile.md | Updated all commands to use fund naming convention and improved Pulumi org detection |
Last reviewed commit: b96b012
forstmeier
approved these changes
Feb 14, 2026
This was referenced Feb 16, 2026
This was referenced Feb 25, 2026
This was referenced Mar 23, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This pull request introduces several important infrastructure improvements, focusing on enhanced security, configuration management, and documentation for the Pulumi deployment environment. Key changes include securing sensitive configuration values, renaming the stack and related identifiers, adding comprehensive infrastructure tests, and providing a detailed runbook for GitHub environment setup.
Infrastructure configuration and security:
Pulumi.production.yaml, improving security and compliance.oscmtofundinPulumi.yamlto reflect organizational naming conventions.Testing and validation:
test_infrastructure_configuration.pyto verify that sensitive configuration values are properly encrypted, check for required security controls (e.g., no use of AdministratorAccess, presence of OIDC claim constraints), and ensure key infrastructure resources and policies are present in the entrypoint.Documentation:
github_environment_runbook.mddocumenting required GitHub environment secrets and step-by-step instructions for updating theAWS_IAM_INFRASTRUCTURE_ROLE_ARNfrom Pulumi outputs, improving onboarding and operational clarity.Summary by CodeRabbit
New Features
Documentation
Tests
Chores