chore: various review tweaks across API, S3 client, and pipelines

[jfrench9]

Summary

A collection of improvements and refinements spanning API deployment, S3 client operations, SEC adapter logic, text index processing, and infrastructure configuration. This PR addresses multiple review feedback items in a single pass.

Key Accomplishments

S3 Client Simplification

• Significantly refactored the S3 operations module, reducing overall complexity (~250 lines changed, net reduction in code)
• Streamlined the S3 client interface and simplified related test coverage to focus on essential behavior

Database & Data Integrity

• Added a new Alembic migration introducing a webhook idempotency constraint, preventing duplicate webhook processing at the database level
• Enhanced text index processing pipeline by removing duplicate elements before indexing

SEC Adapter Improvements

• Refactored the SEC adapter and its client module for improved clarity and maintainability
• Updated the top-level adapters module with structural adjustments

Infrastructure & Deployment

• Enhanced the API deployment workflow with additional steps/configuration
• Updated CloudFormation templates for both the API stack and graph volumes stack
• Improved the graph volume monitor Lambda with expanded monitoring logic

Miscellaneous

• Removed an unused constant from the configuration module
• Minor adjustments to the billing audit log model

Breaking Changes

• Webhook idempotency constraint migration: Existing duplicate webhook records (if any) may cause the migration to fail. Ensure data consistency before applying.
• S3 client interface changes: The refactored S3 operations module has a simplified API surface. Any downstream consumers of the S3 client should be reviewed for compatibility.

Testing Notes

• S3 client tests have been updated to reflect the simplified interface; the test file saw significant reduction (~118 lines changed), indicating removal of tests for deprecated/removed functionality
• Verify the Alembic migration applies cleanly in staging environments, especially if webhook data already exists
• Validate that text index deduplication does not alter expected search behavior

Infrastructure Considerations

• CloudFormation stack updates are required for both the API and graph volumes stacks — coordinate deployment order as needed
• The API deployment workflow has new steps that should be validated in a non-production environment first
• The graph volume monitor Lambda changes should be deployed and monitored for correct alerting behavior

---

🤖 Generated with Claude Code

Branch Info:

• Source: chore/various-review-tweaks
• Target: main
• Type: chore

Co-Authored-By: Claude noreply@anthropic.com

[jfrench9]

@claude please review this PR

[claude]

Claude finished @jfrench9's task in 2m 42s —— View job

---

PR Review: chore/various-review-tweaks

• Gather context and understand request
• Read changed files
• Review S3 client changes
• Review SEC adapter changes
• Review migration and model changes
• Review infrastructure/deployment changes
• Review pipeline and config changes
• Post final review

---

Overall this is a solid cleanup PR. The lazy import pattern is well-applied, the S3 refactor correctly delegates retry logic to boto3, and the security upgrade in the Lambda (ARN → runtime fetch) is the right call. A few items worth addressing below.

---

S3 Client (robosystems/operations/aws/s3.py)

Minor — security_errors contains non-existent AWS error codes

UnauthorizedAccess and TokenRefreshRequired are not standard AWS SDK error codes. The security_errors set in upload_string will never match those two codes, making the critical path only reachable via AccessDenied. Low risk (it only affects which log level fires), but dead code worth removing.

# Current — two of these three codes are not real AWS errors
security_errors = {"AccessDenied", "UnauthorizedAccess", "TokenRefreshRequired"}

Fix this →

Minor — download_file silences all ClientError at debug level

Previously, non-404 errors (e.g. InternalError surviving after all retries) were logged at error level. The new code logs everything at debug, including genuine permission or AWS service failures. If download_file is called in contexts where AccessDenied would be unexpected, these failures will be invisible in logs.

# All ClientErrors — including AccessDenied, persistent InternalError — go to debug
except ClientError as e:
    error_code = e.response.get("Error", {}).get("Code", "")
    logger.debug(f"S3 download not available ({error_code}): s3://{bucket}/{key}")

Consider logging security-class errors at warning or error level even here, matching the pattern used in upload_string.

Fix this →

Nit — adaptive retry max_attempts semantics

max_attempts: 3 in boto3 means 1 initial attempt + 2 retries (total 3 calls). The old manual loops also did 3 total attempts. This is fine but worth confirming the behavior matches intent — the boto3 docs count the initial attempt in max_attempts.

---

Lazy Imports (robosystems/adapters/__init__.py, sec/__init__.py, sec/client/__init__.py)

The __getattr__-based lazy import pattern (PEP 562) is correct and well-suited here. Two minor points:

Minor — importlib imported inside __getattr__ on every call

All three __getattr__ functions do import importlib inside the function body. importlib is a stdlib module and Python caches it, so this is safe, but it's cleaner to import at module level.

# Three files all have this pattern
def __getattr__(name: str):
    if name in _LAZY_IMPORTS:
        import importlib   # ← move to top of file
        module = importlib.import_module(_LAZY_IMPORTS[name])

Fix this →

Minor — sec/client/__init__.py now lazily loads previously-eager imports

SEC_BASE_URL, SECClient, and ArelleClient were previously eagerly imported (they were declared safe for eager loading in the old comment). They're now lazy along with everything else. This is fine at runtime, but type checkers and IDEs may lose type information for callers doing from robosystems.adapters.sec.client import SECClient. Worth verifying basedpyright is happy with this.

---

Webhook Idempotency Migration (alembic/versions/33b3a136ea29_...py)

Looks correct. The partial unique index on (event_data->>'event_id', event_data->>'provider') WHERE event_type = 'webhook_received' is an efficient pattern for idempotency.

Note for deployment: As mentioned in the PR description, if any duplicate (event_id, provider) pairs exist among webhook_received rows, the migration will fail with a unique constraint violation. Worth running this query in staging/prod before applying:

SELECT event_data->>'event_id' AS event_id,
       event_data->>'provider' AS provider,
       COUNT(*)
FROM billing_audit_logs
WHERE event_type = 'webhook_received'
GROUP BY 1, 2
HAVING COUNT(*) > 1;

Minor — model Index vs migration expression style inconsistency

The model uses text("(event_data->>'event_id')") while the migration uses sa.literal_column(...). Both produce the same SQL and are functionally identical, but the inconsistency could confuse future autogeneration. Not a blocker.

---

Graph Volume Monitor Lambda (bin/lambda/graph_volume_monitor.py)

Good change — moving from {{resolve:secretsmanager:...}} (deploy-time resolution, value baked into env var) to ARN + runtime fetch via Secrets Manager is the correct approach, as it supports key rotation without redeployment.

Minor — cached secret survives key rotation for warm Lambda lifetime

_cached_graph_api_key is a module-level global set once per container lifetime. Key rotations won't be picked up until the Lambda container is recycled. This is standard Lambda caching behavior, but it means there's a window (potentially hours) where a rotated key won't be in use. Consider adding a TTL-based invalidation or documenting this behavior with a comment.

Minor — silent fallback to empty string on fetch failure

If Secrets Manager is unreachable, the Lambda falls back to "" (no auth header) and continues making requests to the Graph API. Depending on whether the API requires auth, this could result in silent failures that look like successful no-op runs. Consider whether a hard failure (raise) is more appropriate here, or at minimum log at critical.

---

CloudFormation (cloudformation/api.yaml)

Minor — default CIDR narrowed from /8 to /16

The hardcoded 10.0.0.0/8 allowed any 10.x.x.x address. The new default 10.0.0.0/16 restricts to 10.0.x.x. Environments with VPCs in ranges like 10.1.0.0/16 or 10.100.0.0/16 will lose internal traffic access unless they explicitly pass vpc_cidr. This may be intentional tightening of the rule, but confirm all existing stack deployments pass the correct vpc_cidr via the workflow.

---

Text Index Pipeline (sec/pipeline/text_index.py)

Clean fix — drop_duplicates(subset=["identifier"], keep="first") before set_index prevents the ValueError on duplicate index entries. No concerns.

---

Config Constants (robosystems/config/constants.py)

JWT_REFRESH_TOKEN_EXPIRE_DAYS = 30 is removed and confirmed unused (no references found in the codebase). Clean removal.

---

Audit Log Model (robosystems/models/billing/audit_log.py)

The text import addition and new Index are correct. No issues.

---

Summary

Area	Verdict
S3 client refactor	✅ Cleaner, correct delegation to boto3 adaptive retry
Lazy import pattern	✅ Correct PEP 562 pattern; minor importlib nit
Webhook idempotency	✅ Good constraint; pre-migration data check recommended
Lambda secret fetch	✅ Better security posture; caching/fallback worth documenting
CloudFormation VpcCidr	⚠️ Default narrowed — verify existing deployments
Text index dedup	✅ Correct fix
Constants/model cleanup	✅ Clean