feat(cloudflare): add cloudflare-api package and fix broken imports#10
Conversation
Add new cloudflare-api package for Cloudflare API interactions: - Client wrapper for Cloudflare API with authentication - IP Groups management (create, update, delete, sync) - Multi-source IP fetching (AWS, GCP, Azure, Cloudflare, custom) - CLI tool for IP group management - Pydantic settings for configuration - Comprehensive test suite Enhance cloudflare-auth package: - Add settings module for centralized configuration - Improve middleware with enhanced security features - Add integration tests for JWT validation - Add session management tests - Add whitelist/allowlist tests - Improve test fixtures and coverage Include handoff documentation for cloudflare-api package. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Update planning documents for python-libs monorepo: - Revise project-vision.md with shared library scope - Expand tech-spec.md with package architecture details - Update roadmap.md with phased implementation plan Add Architecture Decision Records: - ADR-001: Monorepo architecture decision - ADR-002: Framework-agnostic design approach - ADR-003: Distribution strategy (PyPI packages) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Add Cloudflare API and Access settings to .env.example - Add ip_groups.example.yaml with IP range configuration template 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Add GitHub Actions workflow for publishing packages to GCP Artifact Registry with Infisical secrets integration. Include handoff documentation with: - Artifact Registry setup details - Service account configuration - Infisical secrets structure - Implementation steps 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Add per-file-ignores for packages/*/tests/**/*.py (same as tests/**) - Add CLI file ignore for T20 (print statements expected in CLI) - Run ruff format and fix on all files - Update uv.lock 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
WalkthroughAdds a new cloudflare-api package (client, models, exceptions, settings), an IP-groups subsystem with multi-source fetchers, manager, and CLI; fixes cloudflare-auth broken imports by adding settings; extensive docs/ADRs; CI publishing workflow to Google Artifact Registry; many tests and expanded .env example and lint configs. Changes
Sequence Diagram(s)sequenceDiagram
participant Config as YAML Config
participant Manager as IPGroupManager
participant Fetcher as IP Fetcher
participant Cache as Cache Layer
participant Client as Cloudflare API Client
participant CF as Cloudflare API
Config->>Manager: from_config(path)
Manager->>Manager: load groups
Manager->>Manager: fetch_group_ips(group)
loop for each source
Manager->>Cache: check cached result
alt cache hit
Cache-->>Manager: return cached IPs
else cache miss
Manager->>Fetcher: fetch(source_config)
Fetcher->>Fetcher: retrieve, parse, validate, filter
Fetcher-->>Manager: IP list
Manager->>Cache: store result
end
end
Manager->>Manager: dedupe & aggregate IPs
Manager->>Client: list_ip_lists()
Client->>CF: GET /lists
CF-->>Client: lists
Client-->>Manager: IPList objects
Manager->>Manager: compute diff (to_add/to_remove)
alt list exists
Manager->>Client: replace_ip_list_items(list_id, items)
else create list
Manager->>Client: create_ip_list(name)
Manager->>Client: add_ip_list_items(list_id, items)
end
Client->>CF: POST bulk operation
CF-->>Client: operation id
loop poll
Client->>CF: GET operation status
CF-->>Client: status
end
Client-->>Manager: final BulkOperation result
Manager->>Manager: record SyncResult
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Areas to focus review on:
Possibly related PRs
Pre-merge checks and finishing touches✅ Passed checks (5 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 |
✅ FIPS Compatibility Check
Status: ✅ PASSED What is FIPS?FIPS 140-2/140-3 is a US government standard for cryptographic modules. Common issues:
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| ) | ||
| warnings = validator.validate_whitelist_config() | ||
|
|
||
| assert any("gmail.com" in w for w in warnings) |
Check failure
Code scanning / CodeQL
Incomplete URL substring sanitization High test
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 5 months ago
To address the issue and avoid substring matching, we can update the test so that it checks for the complete, expected warning message, rather than searching for the substring "gmail.com" anywhere in the warning. This assumes that the warning message format is deterministic. Specifically, replace:
assert any("gmail.com" in w for w in warnings)with
assert "Public email domain detected: gmail.com" in warningsIf the message format may vary or contain additional content, another approach is to match messages that start with or equal the expected warning. The test only needs to assert that the expected warning is present. This change will fully resolve the warning, avoid substring checks, and maintain functional equivalence for the test.
No additional imports or package installations are required.
| @@ -218,7 +218,7 @@ | ||
| ) | ||
| warnings = validator.validate_whitelist_config() | ||
|
|
||
| assert any("gmail.com" in w for w in warnings) | ||
| assert "Public email domain detected: gmail.com" in warnings | ||
|
|
||
|
|
||
| class TestWhitelistManager: |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
✅ Mutation Testing Results
What is Mutation Testing?Mutation testing introduces small changes (mutations) to your code and checks if your tests detect them. A high mutation score indicates your tests are effective at catching bugs.
|
coderabbitai
Bot
added
ci
dependencies
documentation
- Replace `datetime.UTC` with `timezone.utc` for Python 3.10 support (datetime.UTC was added in Python 3.11) - Add `tz=timezone.utc` to all datetime.now() calls to fix DTZ005 warnings - Add PLW0603 noqa comments for singleton pattern in settings.py - Add UP017 to global ruff ignores for Python 3.10 compatibility - Add PT011 and F841 to test file ignores Files changed: - models.py, rate_limiter.py, redis_sessions.py, sessions.py, validators.py - settings.py (PLW0603 noqa) - tests/conftest.py, test_integration.py, test_models.py - pyproject.toml (ruff config updates) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 29
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/gemini-image/src/gemini_image/cli.py (1)
159-176: Consider sending error messages to stderrThe
FileNotFoundErroris surfaced clearly viaprint(f"Error: {e}"), which is fine for interactive use. For better scriptability, you might route error output to stderr:- except FileNotFoundError as e: - print(f"Error: {e}") + except FileNotFoundError as e: + print(f"Error: {e}", file=sys.stderr)This keeps stdout reserved for normal output while still exiting with code 1.
.env.example (1)
164-167: Avoid shell-style default expansion in.envexample
SENTRY_ENVIRONMENT=${ENVIRONMENT:-development}assumes shell-style expansion, but many.envloaders treat this as a literal string. Consider either setting a plain default:SENTRY_ENVIRONMENT=developmentor documenting that this expression only makes sense when interpreted by a shell.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock,!**/*.lock
📒 Files selected for processing (44)
.env.example(1 hunks).github/workflows/publish.yml(1 hunks)docs/cloudflare-api-handoff.md(1 hunks)docs/planning/adr/README.md(1 hunks)docs/planning/adr/adr-001-monorepo-architecture.md(1 hunks)docs/planning/adr/adr-002-framework-agnostic-design.md(1 hunks)docs/planning/adr/adr-003-distribution-strategy.md(1 hunks)docs/planning/project-vision.md(2 hunks)docs/planning/roadmap.md(2 hunks)docs/planning/tech-spec.md(2 hunks)docs/secure.md(1 hunks)ip_groups.example.yaml(1 hunks)packages/cloudflare-api/README.md(1 hunks)packages/cloudflare-api/pyproject.toml(1 hunks)packages/cloudflare-api/src/cloudflare_api/__init__.py(1 hunks)packages/cloudflare-api/src/cloudflare_api/client.py(1 hunks)packages/cloudflare-api/src/cloudflare_api/exceptions.py(1 hunks)packages/cloudflare-api/src/cloudflare_api/ip_groups/__init__.py(1 hunks)packages/cloudflare-api/src/cloudflare_api/ip_groups/cli.py(1 hunks)packages/cloudflare-api/src/cloudflare_api/ip_groups/config.py(1 hunks)packages/cloudflare-api/src/cloudflare_api/ip_groups/fetchers.py(1 hunks)packages/cloudflare-api/src/cloudflare_api/ip_groups/manager.py(1 hunks)packages/cloudflare-api/src/cloudflare_api/models.py(1 hunks)packages/cloudflare-api/src/cloudflare_api/settings.py(1 hunks)packages/cloudflare-api/tests/__init__.py(1 hunks)packages/cloudflare-api/tests/conftest.py(1 hunks)packages/cloudflare-api/tests/test_client.py(1 hunks)packages/cloudflare-api/tests/test_ip_groups.py(1 hunks)packages/cloudflare-api/tests/test_models.py(1 hunks)packages/cloudflare-api/tests/test_settings.py(1 hunks)packages/cloudflare-auth/pyproject.toml(1 hunks)packages/cloudflare-auth/src/cloudflare_auth/settings.py(1 hunks)packages/cloudflare-auth/tests/conftest.py(1 hunks)packages/cloudflare-auth/tests/test_integration.py(1 hunks)packages/cloudflare-auth/tests/test_models.py(1 hunks)packages/cloudflare-auth/tests/test_sessions.py(1 hunks)packages/cloudflare-auth/tests/test_settings.py(1 hunks)packages/cloudflare-auth/tests/test_whitelist.py(1 hunks)packages/gemini-image/src/gemini_image/cli.py(4 hunks)packages/gemini-image/tests/test_generator.py(0 hunks)packages/gemini-image/tests/test_models.py(0 hunks)packages/gemini-image/tests/test_utils.py(0 hunks)pyproject.toml(2 hunks)tests/test_example.py(0 hunks)
💤 Files with no reviewable changes (4)
- packages/gemini-image/tests/test_generator.py
- packages/gemini-image/tests/test_utils.py
- packages/gemini-image/tests/test_models.py
- tests/test_example.py
🧰 Additional context used
📓 Path-based instructions (5)
**/*.md
📄 CodeRabbit inference engine (CLAUDE.md)
Use 120 character line length for Markdown documentation files
Files:
packages/cloudflare-api/README.mddocs/planning/adr/adr-002-framework-agnostic-design.mddocs/planning/roadmap.mddocs/planning/adr/adr-003-distribution-strategy.mddocs/planning/project-vision.mddocs/planning/tech-spec.mddocs/planning/adr/adr-001-monorepo-architecture.mddocs/secure.mddocs/planning/adr/README.mddocs/cloudflare-api-handoff.md
**/*.{yaml,yml}
📄 CodeRabbit inference engine (CLAUDE.md)
Use 2-space indentation for YAML configuration files with 120 character line length
Files:
ip_groups.example.yaml
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Use Ruff formatting with 88 character line length for Python code
Use Ruff linting with PyStrict-aligned rules including BLE, EM, SLF, INP, ISC, PGH, RSE, TID, YTT, FA, T10, and G rules
Tag assumptions with #CRITICAL, #ASSUME, or #EDGE comments including category and reason for verification
Files:
packages/cloudflare-api/src/cloudflare_api/models.pypackages/cloudflare-api/src/cloudflare_api/ip_groups/__init__.pypackages/cloudflare-auth/tests/test_integration.pypackages/cloudflare-api/tests/test_models.pypackages/cloudflare-api/src/cloudflare_api/exceptions.pypackages/cloudflare-auth/tests/test_settings.pypackages/cloudflare-auth/tests/test_whitelist.pypackages/cloudflare-api/src/cloudflare_api/ip_groups/config.pypackages/cloudflare-api/tests/test_client.pypackages/cloudflare-auth/tests/test_sessions.pypackages/cloudflare-api/src/cloudflare_api/settings.pypackages/cloudflare-auth/tests/conftest.pypackages/cloudflare-api/src/cloudflare_api/ip_groups/manager.pypackages/cloudflare-api/src/cloudflare_api/ip_groups/cli.pypackages/cloudflare-api/src/cloudflare_api/__init__.pypackages/cloudflare-api/tests/test_ip_groups.pypackages/cloudflare-auth/tests/test_models.pypackages/cloudflare-api/tests/test_settings.pypackages/cloudflare-auth/src/cloudflare_auth/settings.pypackages/cloudflare-api/src/cloudflare_api/client.pypackages/cloudflare-api/tests/__init__.pypackages/gemini-image/src/gemini_image/cli.pypackages/cloudflare-api/tests/conftest.pypackages/cloudflare-api/src/cloudflare_api/ip_groups/fetchers.py
.github/workflows/**
⚙️ CodeRabbit configuration file
.github/workflows/**: Review GitHub Actions workflows for:
- Security best practices (minimal permissions, pinned actions)
- Proper secret handling
- Efficient caching strategies
- Clear job dependencies
Files:
.github/workflows/publish.yml
pyproject.toml
⚙️ CodeRabbit configuration file
pyproject.toml: Review dependency changes for:
- Version constraint appropriateness
- Security implications of new dependencies
- License compatibility
Files:
pyproject.toml
🧠 Learnings (5)
📚 Learning: 2025-12-14T22:54:23.007Z
Learnt from: CR
Repo: ByronWilliamsCPA/python-libs PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-14T22:54:23.007Z
Learning: Applies to src/**/*.py : Use centralized exception hierarchy from src/python_libs/core/exceptions.py for all error handling
Applied to files:
packages/cloudflare-api/src/cloudflare_api/exceptions.py
📚 Learning: 2025-12-14T22:53:58.927Z
Learnt from: CR
Repo: ByronWilliamsCPA/python-libs PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-14T22:53:58.927Z
Learning: Applies to src/python_libs/**/*.py : Ensure exceptions are specific and appropriate for the failure mode; verify error messages are actionable and user-friendly; check retry mechanisms have proper backoff and limits
Applied to files:
packages/cloudflare-api/src/cloudflare_api/exceptions.py
📚 Learning: 2025-12-14T22:54:23.007Z
Learnt from: CR
Repo: ByronWilliamsCPA/python-libs PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-14T22:54:23.007Z
Learning: Applies to src/python_libs/core/config.py : Use Pydantic Settings for environment-based configuration with .env files
Applied to files:
packages/cloudflare-api/src/cloudflare_api/settings.pypackages/cloudflare-auth/src/cloudflare_auth/settings.py
📚 Learning: 2025-12-14T22:54:23.007Z
Learnt from: CR
Repo: ByronWilliamsCPA/python-libs PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-14T22:54:23.007Z
Learning: Applies to tests/**/*.py : Use pytest fixtures defined in tests/conftest.py for test setup and teardown
Applied to files:
packages/cloudflare-api/tests/conftest.py
📚 Learning: 2025-12-14T22:54:23.007Z
Learnt from: CR
Repo: ByronWilliamsCPA/python-libs PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-14T22:54:23.007Z
Learning: Applies to **/*.py : Use Ruff linting with PyStrict-aligned rules including BLE, EM, SLF, INP, ISC, PGH, RSE, TID, YTT, FA, T10, and G rules
Applied to files:
pyproject.toml
🧬 Code graph analysis (13)
packages/cloudflare-api/src/cloudflare_api/ip_groups/__init__.py (2)
packages/cloudflare-api/src/cloudflare_api/ip_groups/config.py (4)
IPGroupConfig(64-83)IPSourceConfig(26-61)SourceType(14-23)load_config(107-128)packages/cloudflare-api/src/cloudflare_api/ip_groups/fetchers.py (6)
AWSIPFetcher(380-455)GitHubIPFetcher(273-322)GoogleCloudIPFetcher(325-377)IPFetcher(27-92)StaticIPFetcher(95-114)URLIPFetcher(117-270)
packages/cloudflare-auth/tests/test_integration.py (4)
packages/cloudflare-auth/src/cloudflare_auth/models.py (4)
CloudflareJWTClaims(28-106)CloudflareUser(109-266)from_jwt_claims(163-197)model_dump_safe(251-266)packages/cloudflare-auth/src/cloudflare_auth/sessions.py (2)
SimpleSessionManager(28-321)get_session_info(257-281)packages/cloudflare-auth/src/cloudflare_auth/settings.py (3)
CloudflareSettings(11-94)reset_settings(108-111)is_email_allowed(87-94)packages/cloudflare-auth/src/cloudflare_auth/whitelist.py (5)
EmailWhitelistValidator(157-512)UserTier(50-99)is_authorized(254-287)get_user_tier(327-379)is_admin(289-311)
packages/cloudflare-api/tests/test_models.py (1)
packages/cloudflare-api/src/cloudflare_api/models.py (7)
BulkOperation(73-86)BulkOperationStatus(22-28)IPList(49-70)IPListItem(31-46)IPListItemInput(89-109)ListKind(13-19)to_api_dict(100-109)
packages/cloudflare-auth/tests/test_settings.py (1)
packages/cloudflare-auth/src/cloudflare_auth/settings.py (6)
CloudflareSettings(11-94)get_cloudflare_settings(100-105)reset_settings(108-111)issuer(75-80)certs_url(83-85)is_email_allowed(87-94)
packages/cloudflare-auth/tests/test_whitelist.py (1)
packages/cloudflare-auth/src/cloudflare_auth/whitelist.py (15)
EmailWhitelistConfig(119-154)EmailWhitelistValidator(157-512)UserTier(50-99)WhitelistManager(515-731)create_validator_from_env(734-787)from_string(64-81)is_authorized(254-287)is_admin(289-311)get_user_role(313-325)get_user_tier(327-379)get_whitelist_stats(411-431)validate_whitelist_config(497-512)add_email(644-677)remove_email(679-714)check_email(716-731)
packages/cloudflare-api/tests/test_client.py (4)
packages/cloudflare-api/src/cloudflare_api/exceptions.py (5)
CloudflareAuthError(51-55)CloudflareBulkOperationError(140-167)CloudflareConflictError(170-176)CloudflareNotFoundError(84-111)CloudflareValidationError(114-137)packages/cloudflare-api/src/cloudflare_api/models.py (2)
BulkOperationStatus(22-28)ListKind(13-19)packages/cloudflare-api/src/cloudflare_api/settings.py (1)
CloudflareAPISettings(10-109)packages/cloudflare-api/tests/conftest.py (4)
mock_env_vars(24-33)mock_cloudflare_client(37-42)sample_ip_list_response(46-57)sample_ip_list_item_response(61-69)
packages/cloudflare-auth/tests/test_sessions.py (2)
packages/cloudflare-auth/src/cloudflare_auth/sessions.py (2)
SimpleSessionManager(28-321)get_session_info(257-281)packages/cloudflare-auth/tests/test_integration.py (1)
session_manager(39-41)
packages/cloudflare-api/src/cloudflare_api/ip_groups/cli.py (1)
packages/cloudflare-api/src/cloudflare_api/ip_groups/manager.py (6)
IPGroupManager(68-466)from_config(104-119)preview_group(234-274)list_groups(408-427)_get_group(429-447)fetch_group_ips(197-232)
packages/cloudflare-api/src/cloudflare_api/__init__.py (4)
packages/cloudflare-api/src/cloudflare_api/ip_groups/manager.py (1)
client(122-126)packages/cloudflare-api/src/cloudflare_api/exceptions.py (6)
CloudflareAPIError(9-48)CloudflareAuthError(51-55)CloudflareBulkOperationError(140-167)CloudflareNotFoundError(84-111)CloudflareRateLimitError(58-81)CloudflareValidationError(114-137)packages/cloudflare-api/src/cloudflare_api/models.py (2)
BulkOperation(73-86)ListKind(13-19)packages/cloudflare-api/src/cloudflare_api/settings.py (3)
CloudflareAPISettings(10-109)get_cloudflare_api_settings(115-127)reset_settings(130-133)
packages/cloudflare-api/tests/test_settings.py (2)
packages/cloudflare-api/src/cloudflare_api/settings.py (4)
CloudflareAPISettings(10-109)get_cloudflare_api_settings(115-127)reset_settings(130-133)get_token_value(103-109)packages/cloudflare-api/tests/conftest.py (1)
mock_env_vars(24-33)
packages/cloudflare-auth/src/cloudflare_auth/settings.py (1)
packages/cloudflare-api/src/cloudflare_api/settings.py (1)
reset_settings(130-133)
packages/cloudflare-api/src/cloudflare_api/client.py (3)
packages/cloudflare-api/src/cloudflare_api/exceptions.py (5)
CloudflareAPIError(9-48)CloudflareAuthError(51-55)CloudflareNotFoundError(84-111)CloudflareRateLimitError(58-81)CloudflareValidationError(114-137)packages/cloudflare-api/src/cloudflare_api/models.py (7)
BulkOperation(73-86)BulkOperationStatus(22-28)IPList(49-70)IPListItem(31-46)IPListItemInput(89-109)ListKind(13-19)to_api_dict(100-109)packages/cloudflare-api/src/cloudflare_api/settings.py (3)
CloudflareAPISettings(10-109)get_cloudflare_api_settings(115-127)get_token_value(103-109)
packages/cloudflare-api/src/cloudflare_api/ip_groups/fetchers.py (1)
packages/cloudflare-api/src/cloudflare_api/ip_groups/config.py (2)
IPSourceConfig(26-61)SourceType(14-23)
🪛 GitHub Actions: CI
packages/cloudflare-auth/tests/test_integration.py
[warning] 46-46: DTZ005 datetime.datetime.now() called without a tz argument. Pass a datetime.timezone object to the tz parameter.
[warning] 59-59: DTZ005 datetime.datetime.now() called without a tz argument. Pass a datetime.timezone object to the tz parameter.
[warning] 109-109: DTZ005 datetime.datetime.now() called without a tz argument. Pass a datetime.timezone object to the tz parameter.
[warning] 143-143: DTZ005 datetime.datetime.now() called without a tz argument. Pass a datetime.timezone object to the tz parameter.
packages/cloudflare-auth/tests/test_whitelist.py
[warning] 258-258: PT011 pytest.raises(ValueError) is too broad, set the match parameter or use a more specific exception.
packages/cloudflare-auth/tests/test_sessions.py
[warning] 116-116: DTZ005 datetime.datetime.now() called without a tz argument. Pass a datetime.timezone object to the tz parameter.
packages/cloudflare-auth/tests/conftest.py
[warning] 32-32: DTZ005 datetime.datetime.now() called without a tz argument. Pass a datetime.timezone object to the tz parameter.
[warning] 46-46: DTZ005 datetime.datetime.now() called without a tz argument. Pass a datetime.timezone object to the tz parameter.
packages/cloudflare-auth/tests/test_models.py
[warning] 56-56: DTZ005 datetime.datetime.now() called without a tz argument. Pass a datetime.timezone object to the tz parameter.
[warning] 64-64: DTZ005 datetime.datetime.now() called without a tz argument. Pass a datetime.timezone object to the tz parameter.
packages/cloudflare-auth/src/cloudflare_auth/settings.py
[warning] 102-102: PLW0603 Using the global statement to update _settings_instance is discouraged.
[warning] 110-110: PLW0603 Using the global statement to update _settings_instance is discouraged.
🪛 GitHub Check: CodeQL
packages/cloudflare-auth/tests/test_whitelist.py
[failure] 221-221: Incomplete URL substring sanitization
The string gmail.com may be at an arbitrary position in the sanitized URL.
.github/workflows/publish.yml
[warning] 58-58: Unpinned tag for a non-immutable Action in workflow
Unpinned 3rd party Action 'Publish Package' step Uses Step uses 'Infisical/secrets-action' with ref 'v1.0.7', not a pinned commit hash
🪛 LanguageTool
docs/planning/tech-spec.md
[uncategorized] ~274-~274: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ... API changes - Minor: New features, backward compatible - Patch: Bug fixes, backward compat...
(EN_COMPOUND_ADJECTIVE_INTERNAL)
docs/secure.md
[uncategorized] ~110-~110: The official name of this software platform is spelled with a capital “H”.
Context: ...### 2.2 Add Publishing Workflow Create .github/workflows/publish.yml with this conten...
(GITHUB)
[style] ~383-~383: Consider an alternative verb to strengthen your wording.
Context: ...n) Start with gcs-utilities since it has no import issues: ```bash # Ensure you...
(IF_YOU_HAVE_THIS_PROBLEM)
[uncategorized] ~470-~470: The official name of this software platform is spelled with a capital “H”.
Context: ...te Workflow Add the new tag pattern to .github/workflows/publish.yml: ```yaml on: ...
(GITHUB)
docs/cloudflare-api-handoff.md
[uncategorized] ~93-~93: The official name of this software platform is spelled with a capital “H”.
Context: ...- SourceType - Enum: static, url, github, google_cloud, aws, azure, `clou...
(GITHUB)
[style] ~592-~592: As an alternative to the over-used intensifier ‘very’, consider replacing this phrase.
Context: ...handled with error) 3. Large Lists: Very large IP lists (>10,000 items) may hit API li...
(EN_WEAK_ADJECTIVE)
🪛 markdownlint-cli2 (0.18.1)
docs/planning/adr/adr-002-framework-agnostic-design.md
115-115: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
docs/planning/adr/adr-001-monorepo-architecture.md
117-117: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
docs/secure.md
17-17: Bare URL used
(MD034, no-bare-urls)
36-36: Bare URL used
(MD034, no-bare-urls)
98-98: Bare URL used
(MD034, no-bare-urls)
523-523: Bare URL used
(MD034, no-bare-urls)
539-539: Bare URL used
(MD034, no-bare-urls)
540-540: Files should end with a single newline character
(MD047, single-trailing-newline)
docs/cloudflare-api-handoff.md
117-117: 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: Mutation Testing / Mutation Testing
🔇 Additional comments (52)
packages/gemini-image/src/gemini_image/cli.py (3)
17-25:list_modelsoutput is clear and CLI-friendlyThe updated prints produce a readable, structured listing of available models; behavior is appropriate for a CLI tool.
181-186: Story-parts validation message is appropriateThe validation of
args.story_parts < 2with a concise error message and non‑zero exit code is correct for guarding misuse of the flag.
216-221: Draft completion banner is helpful UXThe multi-line draft completion banner gives clear, actionable next steps to users and stays within CLI-friendly formatting limits. No issues from a linting or behavior perspective.
packages/cloudflare-api/tests/__init__.py (1)
1-1: LGTM!The test package docstring is appropriate and follows Python conventions.
pyproject.toml (2)
312-333: LGTM!The per-file-ignores for package test files correctly mirrors the relaxed rules applied to root-level tests, ensuring consistent linting behavior across all test suites.
356-359: LGTM!The CLI-specific rule allowing print statements (T20) is appropriate, as CLI tools legitimately use print for user output.
packages/cloudflare-auth/src/cloudflare_auth/settings.py (4)
1-21: LGTM!The module structure and Pydantic settings configuration are well-designed. The hybrid approach of reading from environment while accepting injected settings provides good flexibility.
Based on learnings, this correctly uses Pydantic Settings for environment-based configuration with .env files.
62-72: LGTM!The comma-separated string parser is well-implemented with proper handling of edge cases (empty strings, None values, already-parsed lists).
74-94: LGTM!The derived properties and email validation logic are well-implemented:
- Safe URL construction with protocol handling
- Case-insensitive domain comparison
- Proper handling of missing configuration
97-111: Singleton pattern is appropriate; linter warnings are expected.The global variable pattern for implementing a settings singleton is a widely-accepted approach in Python. The PLW0603 warnings from the linter are expected and can be safely ignored in this context.
Alternative patterns (e.g.,
@lru_cache(maxsize=1)) exist but the current implementation is clear and idiomatic. Thereset_settings()function for testing is a good practice.docs/planning/adr/adr-003-distribution-strategy.md (1)
1-181: LGTM!This ADR is well-structured and comprehensive. It clearly documents:
- The two-phase distribution approach (Git → Artifact Registry)
- Rationale with pros/cons for each option
- Implementation guidance with concrete examples
- Validation criteria and review schedule
The code examples properly include language identifiers for all fenced code blocks.
docs/planning/adr/README.md (1)
30-32: LGTM!The ADR index is correctly updated with the three new architecture decision records. The table formatting is consistent and the links, statuses, and dates are accurate.
packages/cloudflare-auth/pyproject.toml (1)
27-27: The[email]extra is appropriately included and actively used.Verification confirms the package uses Pydantic's
EmailStrtype (imported inmodels.pyand applied to email fields inCloudflareJWTClaimsandAuthenticatedUserclasses), and directly importsemail_validatorinwhitelist.pyfor email normalization and validation. The[email]extra is necessary to ensure email validation functionality works as designed.packages/cloudflare-api/tests/conftest.py (1)
16-88: Good fixture design and test isolationAutouse settings reset, environment patching, and the shared Cloudflare client mock give clean test isolation and align with the repo’s convention of centralizing fixtures in
tests/conftest.py.packages/cloudflare-api/pyproject.toml (1)
1-71: Package configuration looks coherentProject metadata, wheel build target, script entry point, and semantic-release tag format are consistent with the new
cloudflare_apipackage layout.packages/cloudflare-api/README.md (1)
1-142: README examples align with the exposed API surfaceConfiguration, client usage, and async operation examples match the described
cloudflare_apisettings and client APIs and provide enough detail for first-time users.packages/cloudflare-api/tests/test_models.py (1)
14-149: Model tests cover key contracts and defaultsEnum values, model defaults, and
IPListItemInput.to_api_dictbehavior are exercised in both minimal and populated cases, giving good guardrails around thecloudflare_api.modelssurface.docs/planning/project-vision.md (1)
14-112: Project vision doc is clear and aligned with current scopeThe updated vision, scope, and success metrics sections are concrete and consistent with the monorepo/UV workspace and private-distribution strategy described elsewhere in the planning docs.
ip_groups.example.yaml (1)
1-144: Example IP groups configuration looks solidStructure, indentation, and field naming align with the ip_groups config models; nothing blocking here.
packages/cloudflare-api/tests/test_ip_groups.py (1)
1-488: Comprehensive IP groups tests look goodConfiguration, fetchers, and manager behaviors are well covered, including error and edge scenarios; no issues found.
packages/cloudflare-api/src/cloudflare_api/models.py (1)
13-123: Pydantic models for IP lists and bulk operations look solidEnum values, field types/defaults, and
IPListItemInput.to_api_dict()behavior all align with the intended Cloudflare IP list API usage. I don’t see correctness or typing issues here.packages/cloudflare-api/tests/test_settings.py (1)
18-118: Settings test suite thoroughly exercises configuration behaviorThese tests do a good job covering required env vars, defaults, validation (including case‑insensitive list kinds), and the singleton/reset semantics. The env patching pattern looks correct and isolated.
packages/cloudflare-api/src/cloudflare_api/__init__.py (1)
1-73: Cloudflare API package initializer exports a clear, cohesive public surfaceThe
__all__list cleanly re-exports the client, core models, settings helpers, and exceptions, and the docstring examples are consistent with that API.__version__wiring also looks appropriate for a 0.1.0 initial release.packages/cloudflare-auth/tests/test_integration.py (1)
6-6: No action needed. The code already implements the suggested fix:timezoneis imported at line 6, and all fivedatetime.now()calls at lines 46, 59, 109, 143, and 338 already usedatetime.now(tz=timezone.utc)to create timezone-aware timestamps. There are no DTZ005 violations in the current code.Likely an incorrect or invalid review comment.
packages/cloudflare-api/src/cloudflare_api/ip_groups/cli.py (4)
15-26: LGTM!The logging setup is appropriate for a CLI application. Using
basicConfigwith configurable verbosity is a clean approach.
227-238: LGTM with minor observation.The exception handling is appropriate for a CLI entry point. Catching specific exceptions (
FileNotFoundError,ValueError) before the broadExceptionis good practice. Usinglogging.exceptionfor unexpected errors ensures full tracebacks are logged for debugging.
29-60: LGTM!The
cmd_syncfunction correctly handles both single-group and all-groups sync scenarios, provides clear user feedback with appropriate status indicators, and returns meaningful exit codes.
63-100: LGTM!The
cmd_previewandcmd_listfunctions provide well-structured output with both JSON and human-readable formats. The preview truncation logic (showing first 10 items) is sensible for large IP lists.Also applies to: 103-130
packages/cloudflare-api/src/cloudflare_api/settings.py (4)
10-31: LGTM!Well-structured settings class using Pydantic BaseSettings with appropriate configuration. The
model_configcorrectly specifies.envfile support, case-insensitive env vars, andpopulate_by_name=Truefor alias support. Based on learnings, this aligns with the project pattern for environment-based configuration.
33-55: Good security practice withSecretStr.Using
SecretStrforcloudflare_api_tokenandcloudflare_api_keyprevents accidental logging or serialization of sensitive credentials.
93-101: LGTM!The validator correctly normalizes the list kind to lowercase and provides a clear error message with valid options.
112-133: LGTM!The singleton pattern with
reset_settings()for testing is a pragmatic approach. The lazy initialization ensures settings are only loaded when first accessed.packages/cloudflare-api/src/cloudflare_api/exceptions.py (3)
9-48: Well-designed base exception class.The
CloudflareAPIErrorprovides comprehensive error context withcode,errors, andresponseattributes. The__str__implementation concatenates relevant details for actionable error messages. Based on learnings, this aligns with the centralized exception hierarchy pattern.
58-81: Good inclusion ofretry_afterfor rate limiting.The
retry_afterattribute enables callers to implement proper backoff strategies when rate limited, following best practices for resilient API clients.
84-111: LGTM!The specialized exception classes (
CloudflareNotFoundError,CloudflareValidationError,CloudflareBulkOperationError,CloudflareConflictError) provide appropriate context attributes for each error type. This enables callers to handle errors specifically and extract relevant debugging information.Also applies to: 114-137, 140-167, 170-176
packages/cloudflare-api/src/cloudflare_api/ip_groups/manager.py (5)
128-138: MD5 usage is acceptable for cache key generation.Using MD5 here is appropriate since it's solely for cache key generation, not for security purposes. The truncation to 8 characters is sufficient for cache differentiation.
367-374: Error handling appropriately returnsSyncResultwith error context.Catching exceptions and returning a
SyncResultwith the error message allows the caller to process partial results fromsync_all. This is a good pattern for batch operations where individual failures shouldn't halt the entire process.
88-126: LGTM!The constructor and factory method are well-designed. The
from_configclassmethod provides a convenient way to instantiate from a config file, and the lazy client initialization via property is appropriate.
197-232: LGTM!The
fetch_group_ipsmethod properly aggregates IPs from multiple sources, deduplicates using a set, and provides informative logging. Re-raising exceptions after logging allows callers to handle failures appropriately.
234-274: LGTM!The
preview_group,sync_all, andlist_groupsmethods are well-implemented with clear logic, appropriate logging, and comprehensive return values.Also applies to: 376-406, 408-427
packages/cloudflare-auth/tests/test_models.py (2)
11-80: LGTM!Comprehensive test coverage for
CloudflareJWTClaimsincluding required/optional fields, timestamp properties, expiration checks, and audience handling. The tests are well-structured with clear assertions.
83-208: LGTM!Excellent test coverage for
CloudflareUsermodel including user creation, tier handling, domain/username properties, premium access checks, role properties, and themodel_dump_safesecurity behavior. The test correctly verifies that sensitiveclaimsare excluded from the safe dump.packages/cloudflare-api/src/cloudflare_api/ip_groups/fetchers.py (5)
1-25: LGTM on imports and constants.The module structure is clean with appropriate imports, logging setup, and well-documented provider URL constants.
95-115: LGTM!StaticIPFetcher correctly validates IPs, logs warnings for invalid entries, and applies version filtering.
273-323: LGTM!GitHubIPFetcher correctly handles service filtering with sensible defaults, validates IPs, deduplicates results, and provides informative logging.
325-378: LGTM!GoogleCloudIPFetcher correctly handles region/service filtering, extracts both IPv4 and IPv6 prefixes, and deduplicates results.
380-456: LGTM!AWSIPFetcher correctly processes both IPv4 and IPv6 prefixes with region/service filtering. The
_matches_filtershelper provides clean separation of concerns.packages/cloudflare-api/src/cloudflare_api/client.py (5)
1-40: LGTM on imports and module setup.Clean organization with SDK exceptions mapped to custom domain exceptions, proper model imports, and appropriate logging configuration.
63-85: LGTM on initialization.Clean dependency injection pattern with sensible defaults. The truncated account ID logging is appropriate for security while maintaining debuggability.
376-437: LGTM on bulk item operations.Clean implementation with proper async operation handling, conflict detection for pending operations, and consistent logging. The item conversion logic handles both
IPListItemInputand dict inputs gracefully.
583-626: LGTM on bulk operation waiting logic.Proper timeout handling, clear status logging, and appropriate error propagation. The polling approach with configurable interval is suitable for synchronous operation.
657-676: LGTM on convenience methods.
ensure_ip_listprovides a clean get-or-create pattern, andsync_ip_listoffers a simplified interface for full list replacement with optional per-IP comments.
| ``` | ||
| packages/cloudflare-api/ | ||
| ├── src/cloudflare_api/ | ||
| │ ├── __init__.py # Main package exports | ||
| │ ├── client.py # CloudflareAPIClient (IP list CRUD) | ||
| │ ├── exceptions.py # Custom exception hierarchy | ||
| │ ├── models.py # Pydantic models (IPList, IPListItem, etc.) | ||
| │ ├── settings.py # CloudflareAPISettings (env config) | ||
| │ └── ip_groups/ # IP Range Groups System | ||
| │ ├── __init__.py # IP groups exports | ||
| │ ├── config.py # Configuration models (YAML schema) | ||
| │ ├── fetchers.py # IP source fetchers (GitHub, GCP, AWS, URL, static) | ||
| │ ├── manager.py # IPGroupManager (orchestration) | ||
| │ └── cli.py # CLI commands | ||
| ├── tests/ | ||
| │ ├── test_client.py # Client tests (26 tests) | ||
| │ ├── test_models.py # Model tests (13 tests) | ||
| │ ├── test_settings.py # Settings tests (12 tests) | ||
| │ └── test_ip_groups.py # IP groups tests (32 tests) | ||
| ├── pyproject.toml # Package configuration | ||
| └── README.md # Package documentation | ||
| ``` |
There was a problem hiding this comment.
Add languages to fenced code blocks to satisfy MD040
Some fenced code blocks (e.g., the package/file trees under “Package Structure”, “Core Package Files”, and “Configuration Examples”) use bare triple backticks without a language, which triggers MD040. Consider annotating them as plain text:
```text
# tree listing...
This keeps markdownlint happy and improves editor/tooling support.
Also applies to: 510-533, 536-538
<details>
<summary>🤖 Prompt for AI Agents</summary>
In docs/cloudflare-api-handoff.md around lines 26 to 47 (and also at the other
flagged ranges 510-533 and 536-538), several fenced code blocks are missing a
language tag which triggers MD040; update each triple-backtick fence for
file/dir trees and other plain text blocks to use a language identifier (e.g.,
ensuring all fenced code blocks in those ranges include the language tag.
| ``` | ||
| python-libs/ | ||
| ├── pyproject.toml # Workspace root | ||
| ├── packages/ | ||
| │ ├── cloudflare-auth/ | ||
| │ │ ├── pyproject.toml # Independent package | ||
| │ │ └── src/cloudflare_auth/ | ||
| │ └── gcs-utilities/ | ||
| │ ├── pyproject.toml # Independent package | ||
| │ └── src/gcs_utilities/ | ||
| └── src/python_libs/ # Shared workspace utilities | ||
| ``` |
There was a problem hiding this comment.
Add language identifier to fenced code block.
The directory structure code block should specify a language identifier for proper syntax highlighting and to satisfy linting rules.
Apply this diff:
-```
+```text
python-libs/
├── pyproject.toml # Workspace root
├── packages/Based on coding guidelines, Markdown files should use proper code block language identifiers.
🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
117-117: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
In docs/planning/adr/adr-001-monorepo-architecture.md around lines 117 to 128,
the fenced code block showing the directory structure lacks a language
identifier; update the opening fence from ``` to ```text (and ensure the
matching closing fence remains) so the block becomes ```text ... ``` to satisfy
linting and provide correct syntax highlighting.
| ``` | ||
| cloudflare-auth/ | ||
| ├── src/cloudflare_auth/ | ||
| │ ├── __init__.py # Public API | ||
| │ ├── core/ # Framework-agnostic | ||
| │ │ ├── __init__.py | ||
| │ │ ├── models.py # CloudflareUser, CloudflareJWTClaims | ||
| │ │ ├── validators.py # JWT validation logic | ||
| │ │ └── exceptions.py # Auth exceptions | ||
| │ └── fastapi/ # FastAPI-specific | ||
| │ ├── __init__.py | ||
| │ ├── middleware.py # CloudflareAuthMiddleware | ||
| │ └── dependencies.py # get_current_user, require_admin | ||
| └── pyproject.toml | ||
| ``` |
There was a problem hiding this comment.
Add language identifier to fenced code block.
The directory structure code block should specify a language identifier for proper syntax highlighting and to satisfy linting rules.
Apply this diff:
-```
+```text
cloudflare-auth/
├── src/cloudflare_auth/
│ ├── __init__.py # Public APIBased on coding guidelines, Markdown files should use proper code block language identifiers.
🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
115-115: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
In docs/planning/adr/adr-002-framework-agnostic-design.md around lines 115 to
129 the fenced code block showing the directory tree lacks a language
identifier; update the opening fence from ``` to ```text so the block becomes
```text and leave the rest unchanged, ensuring the code block uses the "text"
language identifier for proper highlighting and to satisfy linters.
| ### TL;DR | ||
|
|
||
| 1. Open Claude Code in this project directory | ||
| 2. Describe your project concept | ||
| 3. Run: `/plan <your project description>` | ||
| Consolidate shared utilities into python-libs across 4 phases: Foundation (complete existing packages), Stabilization (tests, docs, first consumers), Consolidation (refactor for reusability), Expansion (add new packages from pattern analysis), Distribution (Artifact Registry + cookiecutter integration). | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Wrap TL;DR to respect 120‑character Markdown line limit
The TL;DR line is very long; consider wrapping for readability and guideline compliance.
-### TL;DR
-
-Consolidate shared utilities into python-libs across 4 phases: Foundation (complete existing packages), Stabilization (tests, docs, first consumers), Consolidation (refactor for reusability), Expansion (add new packages from pattern analysis), Distribution (Artifact Registry + cookiecutter integration).
+### TL;DR
+
+Consolidate shared utilities into python-libs across 4 phases:
+- Foundation (complete existing packages)
+- Stabilization (tests, docs, first consumers)
+- Consolidation (refactor for reusability)
+- Expansion (add new packages from pattern analysis)
+- Distribution (Artifact Registry + cookiecutter integration)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ### TL;DR | |
| 1. Open Claude Code in this project directory | |
| 2. Describe your project concept | |
| 3. Run: `/plan <your project description>` | |
| Consolidate shared utilities into python-libs across 4 phases: Foundation (complete existing packages), Stabilization (tests, docs, first consumers), Consolidation (refactor for reusability), Expansion (add new packages from pattern analysis), Distribution (Artifact Registry + cookiecutter integration). | |
| ### TL;DR | |
| Consolidate shared utilities into python-libs across 4 phases: | |
| - Foundation (complete existing packages) | |
| - Stabilization (tests, docs, first consumers) | |
| - Consolidation (refactor for reusability) | |
| - Expansion (add new packages from pattern analysis) | |
| - Distribution (Artifact Registry + cookiecutter integration) |
🤖 Prompt for AI Agents
In docs/planning/roadmap.md around lines 18 to 21, the TL;DR header paragraph is
a single overly long line; reflow it so no line exceeds 120 characters by
breaking the sentence into multiple Markdown lines at sensible word boundaries
(e.g., after commas or phase names) while preserving the original wording and
Markdown formatting, ensuring each new line is ≤120 chars and the paragraph
renders as a single Markdown paragraph.
| def test_get_session_expired(self, short_timeout_manager): | ||
| """Test that expired sessions return None.""" | ||
| session_id = short_timeout_manager.create_session( | ||
| email="test@example.com", | ||
| is_admin=False, | ||
| user_tier="full", | ||
| ) | ||
|
|
||
| # Wait for session to expire | ||
| time.sleep(1.5) | ||
|
|
||
| session = short_timeout_manager.get_session(session_id) | ||
| assert session is None | ||
|
|
||
| def test_invalidate_session(self, session_manager): | ||
| """Test invalidating a session.""" | ||
| session_id = session_manager.create_session( | ||
| email="test@example.com", | ||
| is_admin=False, | ||
| user_tier="full", | ||
| ) | ||
|
|
||
| result = session_manager.invalidate_session(session_id) | ||
|
|
||
| assert result is True | ||
| assert session_manager.get_session(session_id) is None | ||
|
|
||
| def test_invalidate_session_not_found(self, session_manager): | ||
| """Test invalidating non-existent session.""" | ||
| result = session_manager.invalidate_session("nonexistent") | ||
|
|
||
| assert result is False | ||
|
|
||
| def test_refresh_session(self, session_manager): | ||
| """Test refreshing a session.""" | ||
| session_id = session_manager.create_session( | ||
| email="test@example.com", | ||
| is_admin=False, | ||
| user_tier="full", | ||
| ) | ||
|
|
||
| initial_session = session_manager.get_session(session_id) | ||
| initial_accessed = initial_session["last_accessed"] | ||
|
|
||
| # Small delay to ensure timestamp changes | ||
| time.sleep(0.01) | ||
|
|
||
| result = session_manager.refresh_session(session_id) | ||
|
|
||
| assert result is True | ||
|
|
||
| def test_refresh_session_not_found(self, session_manager): | ||
| """Test refreshing non-existent session.""" | ||
| result = session_manager.refresh_session("nonexistent") | ||
|
|
||
| assert result is False | ||
|
|
||
| def test_cleanup_expired_sessions(self, short_timeout_manager): | ||
| """Test cleaning up expired sessions.""" | ||
| # Create multiple sessions | ||
| short_timeout_manager.create_session( | ||
| email="user1@example.com", | ||
| is_admin=False, | ||
| user_tier="full", | ||
| ) | ||
| short_timeout_manager.create_session( | ||
| email="user2@example.com", | ||
| is_admin=False, | ||
| user_tier="full", | ||
| ) | ||
|
|
||
| # Wait for sessions to expire | ||
| time.sleep(1.5) | ||
|
|
||
| count = short_timeout_manager.cleanup_expired_sessions() | ||
|
|
||
| assert count == 2 | ||
| assert short_timeout_manager.get_session_count() == 0 | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Good coverage; consider minimizing sleep-based timing in tests
The session expiration and security tests are thorough. If test runtime ever becomes a concern, you could reduce reliance on real time.sleep(...) (e.g., inject a clock or use a shorter timeout/sleep) while keeping the same behavioral coverage, especially in test_get_session_expired and test_session_id_is_secure.
Also applies to: 245-260
🧰 Tools
🪛 GitHub Actions: CI
[warning] 116-116: DTZ005 datetime.datetime.now() called without a tz argument. Pass a datetime.timezone object to the tz parameter.
🤖 Prompt for AI Agents
In packages/cloudflare-auth/tests/test_sessions.py around lines 74-152 (and
similarly at 245-260), the tests rely on real time.sleep calls to trigger
session expiry which makes tests slow and flaky; change the tests to avoid real
sleeping by either (a) using a configurable clock injected into the
SessionManager (or passing a time function) and advancing/mock the clock in
tests, or (b) monkeypatching/time-freezing time.time (or using freezegun) so you
can simulate expiry instantly, or (c) reduce the session timeout in the
short_timeout_manager fixture to a much smaller value and shorten sleeps
accordingly; update the fixtures and tests to use one of these approaches and
remove long sleep calls so tests remain fast and deterministic.
| def teardown_method(self): | ||
| """Reset settings after each test.""" | ||
| reset_settings() | ||
|
|
||
| def test_returns_settings_instance(self): | ||
| """Test that get_cloudflare_settings returns a CloudflareSettings instance.""" | ||
| settings = get_cloudflare_settings() | ||
|
|
||
| assert isinstance(settings, CloudflareSettings) | ||
|
|
||
| def test_returns_singleton(self): | ||
| """Test that get_cloudflare_settings returns the same instance.""" | ||
| settings1 = get_cloudflare_settings() | ||
| settings2 = get_cloudflare_settings() | ||
|
|
||
| assert settings1 is settings2 | ||
|
|
||
| def test_reset_creates_new_instance(self): | ||
| """Test that reset_settings allows new instance creation.""" | ||
| settings1 = get_cloudflare_settings() | ||
| reset_settings() | ||
| settings2 = get_cloudflare_settings() | ||
|
|
||
| # They should be different objects after reset | ||
| assert settings1 is not settings2 | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Avoid duplicating settings reset logic between fixture and teardown
If the autouse reset_settings_after_test fixture in conftest.py is active for this module, this teardown_method becomes redundant. Keeping only one reset mechanism reduces maintenance overhead and avoids surprises if reset behavior changes.
🤖 Prompt for AI Agents
In packages/cloudflare-auth/tests/test_settings.py around lines 146 to 171, the
teardown_method that calls reset_settings() duplicates the autouse
reset_settings_after_test fixture from conftest.py; remove the teardown_method
entirely so the module relies on the single autouse fixture to reset state
between tests, or if you prefer test-local control, disable the autouse fixture
and keep the teardown_method—ensure only one reset mechanism remains to avoid
double resets.
| except (ValueError, ImportError) as e: | ||
| print(f"Error: {e}") # noqa: T201 | ||
| print(f"Error: {e}") | ||
| sys.exit(1) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Align other errors with stderr usage (optional)
As with the earlier FileNotFoundError block, you may want to send these ValueError/ImportError messages to stderr to make it easier for callers to separate errors from normal output:
- except (ValueError, ImportError) as e:
- print(f"Error: {e}")
+ except (ValueError, ImportError) as e:
+ print(f"Error: {e}", file=sys.stderr)This is an optional CLI-hardening improvement; behavior is otherwise fine.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| except (ValueError, ImportError) as e: | |
| print(f"Error: {e}") # noqa: T201 | |
| print(f"Error: {e}") | |
| sys.exit(1) | |
| except (ValueError, ImportError) as e: | |
| print(f"Error: {e}", file=sys.stderr) | |
| sys.exit(1) |
🤖 Prompt for AI Agents
In packages/gemini-image/src/gemini_image/cli.py around lines 224 to 226, the
exception handler prints ValueError/ImportError messages to stdout; change the
print call to write to stderr instead (e.g., use print(..., file=sys.stderr) or
equivalent) so error output is separated from normal output, then exit as before
with sys.exit(1).
CodeQL fixes: - Pin sonarsource/sonarqube-quality-gate-action to v1.2.0 SHA - Pin Infisical/secrets-action to v1.0.7 SHA - Add permissions block to validate-cruft.yml SonarCloud fixes: - Add usedforsecurity=False to MD5 hash for cache keys - Fix datetime.now() calls to include timezone - Add security hotspot exclusions for test files (hardcoded IPs, HTTP URLs) - Update sonar-project.properties to include packages/ in sources Code quality improvements in manager.py: - Move time import to top of file - Replace logger.error with logger.exception - Use list comprehension instead of for loop append 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Add specific match pattern to pytest.raises(ValueError) in test_add_email_invalid_format test for better test practices. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
sonar-project.properties (1)
78-78: Consolidate duplicatesonar.issue.ignore.multicriteriadefinitions.The property is defined twice (line 78 and line 95), and the second definition overrides the first. In properties files, only the last value is active. Consolidate into a single definition to avoid confusion and reduce maintenance burden.
Apply this diff:
# Ignore specific rules for certain files # Format: sonar.issue.ignore.multicriteria.<key>.ruleKey=<rule> # sonar.issue.ignore.multicriteria.<key>.resourceKey=<pattern> - # Example: Ignore complexity warnings in __init__.py files - sonar.issue.ignore.multicriteria=e1,e2,e3 + # Example: Ignore complexity warnings in __init__.py files + sonar.issue.ignore.multicriteria=e1,e2,e3,e4,e5,e6,e7 sonar.issue.ignore.multicriteria.e1.ruleKey=python:S1192 sonar.issue.ignore.multicriteria.e1.resourceKey=**/__init__.py # Example: Ignore string duplication in test files sonar.issue.ignore.multicriteria.e2.ruleKey=python:S1192 sonar.issue.ignore.multicriteria.e2.resourceKey=**/tests/** # Example: Ignore too-many-arguments in test fixtures sonar.issue.ignore.multicriteria.e3.ruleKey=python:S107 sonar.issue.ignore.multicriteria.e3.resourceKey=**/conftest.py - # ============================================================================= - # Security Hotspot Exclusions for Test Files - # ============================================================================= - # Test files intentionally use hardcoded IPs, HTTP URLs, etc. for testing - sonar.issue.ignore.multicriteria=e1,e2,e3,e4,e5,e6,e7 # Ignore hardcoded IP addresses in test files (S1313) sonar.issue.ignore.multicriteria.e4.ruleKey=python:S1313 sonar.issue.ignore.multicriteria.e4.resourceKey=**/tests/** # Ignore HTTP protocol warnings in test files (S5332) sonar.issue.ignore.multicriteria.e5.ruleKey=python:S5332 sonar.issue.ignore.multicriteria.e5.resourceKey=**/tests/** # Ignore hardcoded credentials in test files (S2068) sonar.issue.ignore.multicriteria.e6.ruleKey=python:S2068 sonar.issue.ignore.multicriteria.e6.resourceKey=**/tests/** # Ignore hardcoded secrets in test files (S6418) sonar.issue.ignore.multicriteria.e7.ruleKey=python:S6418 sonar.issue.ignore.multicriteria.e7.resourceKey=**/tests/**Also applies to: 95-95
packages/cloudflare-auth/src/cloudflare_auth/validators.py (1)
22-22: Outdated docstring reference.The docstring still references
src.config.settingsbut the import was updated tocloudflare_auth.config. Update for consistency:- - src.config.settings: For Cloudflare configuration + - cloudflare_auth.config: For Cloudflare configurationpackages/cloudflare-auth/src/cloudflare_auth/models.py (1)
148-151: Inconsistent timezone handling forauthenticated_at.
default_factory=datetime.nowcreates naive (timezone-unaware) datetimes, while the rest of the module usestimezone.utc. This inconsistency can cause comparison issues.authenticated_at: datetime = Field( - default_factory=datetime.now, + default_factory=lambda: datetime.now(tz=timezone.utc), description="Timestamp when user was authenticated", )
♻️ Duplicate comments (3)
packages/cloudflare-auth/src/cloudflare_auth/settings.py (1)
22-24: Consider making required fields truly required to prevent silent misconfiguration.The
cloudflare_team_domainandcloudflare_audience_tagfields have empty string defaults, which can hide misconfiguration. Theissuerproperty (lines 77-80) checks for empty strings and returns"", leading to silent failures rather than explicit validation errors at startup.Removing the default values will cause Pydantic to raise clear validation errors if these environment variables are missing:
- cloudflare_team_domain: str = Field(default="", alias="CLOUDFLARE_TEAM_DOMAIN") - cloudflare_audience_tag: str = Field(default="", alias="CLOUDFLARE_AUDIENCE_TAG") + cloudflare_team_domain: str = Field(alias="CLOUDFLARE_TEAM_DOMAIN") + cloudflare_audience_tag: str = Field(alias="CLOUDFLARE_AUDIENCE_TAG")packages/cloudflare-auth/tests/test_whitelist.py (1)
256-260: Addmatchparameter topytest.raisesfor precise error assertion.The test should assert the specific error message to satisfy PT011 and align with best practices.
def test_add_email_invalid_format(self, manager): """Test adding invalid email format.""" - with pytest.raises(ValueError): + with pytest.raises(ValueError, match="Invalid email format"): manager.add_email("invalid-email")packages/cloudflare-api/src/cloudflare_api/ip_groups/manager.py (1)
64-66: Critical: Naive datetime causes TypeError in cache validation.Line 65 uses
datetime.nowwithout timezone, creating a naive datetime. However, line 158 compares this withdatetime.now(tz=timezone.utc), which will raiseTypeError: can't subtract offset-naive and offset-aware datetimeswhen the default factory is used.Apply this diff to use timezone-aware datetime consistently:
- fetched_at: datetime = field(default_factory=datetime.now) + fetched_at: datetime = field(default_factory=lambda: datetime.now(tz=timezone.utc))This ensures all datetime operations throughout the cache validation logic (lines 158, 190) use timezone-aware datetimes consistently.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (16)
.github/workflows/publish.yml(1 hunks).github/workflows/sonarcloud.yml(1 hunks).github/workflows/validate-cruft.yml(1 hunks)packages/cloudflare-api/src/cloudflare_api/ip_groups/manager.py(1 hunks)packages/cloudflare-auth/src/cloudflare_auth/models.py(4 hunks)packages/cloudflare-auth/src/cloudflare_auth/rate_limiter.py(7 hunks)packages/cloudflare-auth/src/cloudflare_auth/redis_sessions.py(3 hunks)packages/cloudflare-auth/src/cloudflare_auth/sessions.py(7 hunks)packages/cloudflare-auth/src/cloudflare_auth/settings.py(1 hunks)packages/cloudflare-auth/src/cloudflare_auth/validators.py(2 hunks)packages/cloudflare-auth/tests/conftest.py(1 hunks)packages/cloudflare-auth/tests/test_integration.py(1 hunks)packages/cloudflare-auth/tests/test_models.py(1 hunks)packages/cloudflare-auth/tests/test_whitelist.py(1 hunks)pyproject.toml(3 hunks)sonar-project.properties(2 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
.github/workflows/**
⚙️ CodeRabbit configuration file
.github/workflows/**: Review GitHub Actions workflows for:
- Security best practices (minimal permissions, pinned actions)
- Proper secret handling
- Efficient caching strategies
- Clear job dependencies
Files:
.github/workflows/validate-cruft.yml.github/workflows/sonarcloud.yml.github/workflows/publish.yml
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Use Ruff formatting with 88 character line length for Python code
Use Ruff linting with PyStrict-aligned rules including BLE, EM, SLF, INP, ISC, PGH, RSE, TID, YTT, FA, T10, and G rules
Tag assumptions with #CRITICAL, #ASSUME, or #EDGE comments including category and reason for verification
Files:
packages/cloudflare-auth/src/cloudflare_auth/redis_sessions.pypackages/cloudflare-auth/src/cloudflare_auth/rate_limiter.pypackages/cloudflare-auth/src/cloudflare_auth/sessions.pypackages/cloudflare-auth/src/cloudflare_auth/settings.pypackages/cloudflare-auth/tests/test_whitelist.pypackages/cloudflare-auth/tests/conftest.pypackages/cloudflare-auth/src/cloudflare_auth/validators.pypackages/cloudflare-auth/tests/test_models.pypackages/cloudflare-auth/tests/test_integration.pypackages/cloudflare-auth/src/cloudflare_auth/models.pypackages/cloudflare-api/src/cloudflare_api/ip_groups/manager.py
pyproject.toml
⚙️ CodeRabbit configuration file
pyproject.toml: Review dependency changes for:
- Version constraint appropriateness
- Security implications of new dependencies
- License compatibility
Files:
pyproject.toml
🧠 Learnings (4)
📚 Learning: 2025-12-14T22:54:23.007Z
Learnt from: CR
Repo: ByronWilliamsCPA/python-libs PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-14T22:54:23.007Z
Learning: Applies to src/python_libs/core/config.py : Use Pydantic Settings for environment-based configuration with .env files
Applied to files:
packages/cloudflare-auth/src/cloudflare_auth/settings.py
📚 Learning: 2025-12-14T22:53:58.927Z
Learnt from: CR
Repo: ByronWilliamsCPA/python-libs PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-14T22:53:58.927Z
Learning: Applies to src/python_libs/**/*.py : Ensure exceptions are specific and appropriate for the failure mode; verify error messages are actionable and user-friendly; check retry mechanisms have proper backoff and limits
Applied to files:
packages/cloudflare-auth/tests/test_whitelist.py
📚 Learning: 2025-12-14T22:54:23.007Z
Learnt from: CR
Repo: ByronWilliamsCPA/python-libs PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-14T22:54:23.007Z
Learning: Applies to **/*.py : Use Ruff linting with PyStrict-aligned rules including BLE, EM, SLF, INP, ISC, PGH, RSE, TID, YTT, FA, T10, and G rules
Applied to files:
sonar-project.propertiespyproject.toml
📚 Learning: 2025-12-14T22:53:58.927Z
Learnt from: CR
Repo: ByronWilliamsCPA/python-libs PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-14T22:53:58.927Z
Learning: Applies to src/python_libs/**/*.py : Ensure code is self-documenting through clear naming; replace magic numbers with named constants; verify complexity is appropriate and could not be simplified
Applied to files:
pyproject.toml
🧬 Code graph analysis (4)
packages/cloudflare-auth/src/cloudflare_auth/settings.py (1)
packages/cloudflare-api/src/cloudflare_api/settings.py (1)
reset_settings(130-133)
packages/cloudflare-auth/tests/test_whitelist.py (1)
packages/cloudflare-auth/src/cloudflare_auth/whitelist.py (15)
EmailWhitelistConfig(119-154)EmailWhitelistValidator(157-512)UserTier(50-99)WhitelistManager(515-731)create_validator_from_env(734-787)from_string(64-81)is_authorized(254-287)is_admin(289-311)get_user_role(313-325)get_user_tier(327-379)get_whitelist_stats(411-431)validate_whitelist_config(497-512)add_email(644-677)remove_email(679-714)check_email(716-731)
packages/cloudflare-auth/tests/conftest.py (4)
packages/cloudflare-auth/tests/test_integration.py (1)
settings(20-26)packages/cloudflare-auth/src/cloudflare_auth/settings.py (1)
reset_settings(108-111)packages/cloudflare-api/src/cloudflare_api/settings.py (1)
reset_settings(130-133)packages/cloudflare-api/tests/conftest.py (1)
reset_settings_after_test(17-20)
packages/cloudflare-auth/tests/test_models.py (4)
packages/cloudflare-auth/src/cloudflare_auth/models.py (13)
CloudflareJWTClaims(28-106)CloudflareUser(109-266)issued_at(72-78)expires_at(81-87)is_expired(90-96)get_audience_list(98-106)from_jwt_claims(163-197)email_domain(200-206)email_username(209-215)has_email_domain(217-231)can_access_premium_models(234-240)role(243-249)model_dump_safe(251-266)packages/cloudflare-auth/src/cloudflare_auth/whitelist.py (4)
UserTier(50-99)is_admin(289-311)can_access_premium_models(84-90)can_access_premium_models(381-394)packages/cloudflare-auth/tests/conftest.py (1)
sample_jwt_payload(17-26)packages/cloudflare-auth/tests/test_integration.py (1)
sample_claims(44-54)
🪛 GitHub Check: CodeQL
packages/cloudflare-auth/tests/test_whitelist.py
[failure] 221-221: Incomplete URL substring sanitization
The string gmail.com may be at an arbitrary position in the sanitized URL.
⏰ 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: Compatibility Summary
- GitHub Check: Mutation Testing / Mutation Testing
🔇 Additional comments (18)
.github/workflows/validate-cruft.yml (1)
13-14: LGTM! Security best practice: minimal permissions.Explicitly setting
permissions: contents: readfollows the principle of least privilege by limiting this workflow to read-only repository access. Since the workflow only checks template synchronization and runs validation scripts without modifying the repository, this minimal permission scope is appropriate and improves security posture.sonar-project.properties (2)
16-16: Configuration correctly expanded to include packages.The additions to
sonar.sourcesandsonar.testsare appropriate for the monorepo structure with per-package tests underpackages/*/tests/.Also applies to: 19-19
91-112: Security hotspot exclusions for tests are appropriate.The rules appropriately suppress warnings for test-specific security patterns (hardcoded IPs, HTTP URLs, test credentials, secrets) that are expected and safe in test contexts.
packages/cloudflare-auth/src/cloudflare_auth/validators.py (1)
32-32: LGTM!The import fix from
src.config.settingstocloudflare_auth.configaddresses the broken imports issue. The timezone change totimezone.utcis consistent with Python's stdlib and aligns with the broader refactoring across the package.Also applies to: 38-38, 270-270
packages/cloudflare-auth/src/cloudflare_auth/redis_sessions.py (1)
40-40: LGTM!Timezone handling is now consistent with
timezone.utcacross all session timestamp operations. The ISO format serialization preserves timezone information for Redis storage.Also applies to: 177-178, 224-224
packages/cloudflare-auth/src/cloudflare_auth/models.py (1)
78-78: LGTM!Timezone-aware datetime handling is correctly implemented for JWT timestamp properties using
timezone.utc.Also applies to: 87-87, 96-96
packages/cloudflare-auth/src/cloudflare_auth/sessions.py (1)
22-22: LGTM!Consistent timezone-aware datetime handling throughout the session lifecycle operations.
Also applies to: 104-105, 151-151, 186-186, 200-200, 279-279
packages/cloudflare-auth/src/cloudflare_auth/rate_limiter.py (1)
26-26: LGTM!Timezone handling is consistent across all rate limiter operations. Thread-safe implementation with Lock is properly maintained.
Also applies to: 76-76, 96-97, 127-127, 150-151, 178-179, 203-204
packages/cloudflare-auth/tests/conftest.py (1)
3-61: LGTM! Excellent test fixture organization.The test fixtures are well-structured with:
- Timezone-aware datetime usage (addressing DTZ005)
- Reusable test constants for consistency
- Valid and expired JWT payload fixtures for comprehensive testing
- Autouse fixture ensuring singleton state cleanup between tests
packages/cloudflare-auth/tests/test_whitelist.py (2)
214-221: CodeQL false positive – the test correctly validates the warning message.The CodeQL alert about "Incomplete URL substring sanitization" for
gmail.comon line 221 is a false positive. This test verifies that the validator emits a security warning when public email domains like@gmail.comare included in the whitelist configuration, which is the intended behavior.
1-320: LGTM! Comprehensive whitelist test coverage.The test suite thoroughly covers:
- UserTier enum behavior and properties
- Email normalization and configuration
- Authorization logic for individual emails and domain patterns
- Admin detection and tier assignment
- Whitelist manager operations (add/remove/check)
- Environment-based validator creation
packages/cloudflare-auth/tests/test_integration.py (1)
1-359: LGTM! Excellent integration test coverage.The integration tests provide comprehensive end-to-end validation:
- Complete authentication flows for admin, full, limited, and unauthorized users
- Session lifecycle management (create, retrieve, refresh, invalidate)
- Settings integration with whitelist validators
- User tier propagation across components
- Security aspects (constant-time comparison, session ID uniqueness, safe data dumping)
All datetime usage is timezone-aware, and the tests properly exercise interactions between settings, validators, sessions, and user models.
packages/cloudflare-auth/tests/test_models.py (1)
1-208: LGTM! Thorough model test coverage with timezone-aware datetimes.The test suite comprehensively validates:
- CloudflareJWTClaims: required/optional fields, datetime properties, expiration logic, audience handling
- CloudflareUser: creation from claims, tier-based permissions, email domain checks, safe data dumping
All datetime operations correctly use
timezone.utc(lines 56, 64), addressing the previous DTZ005 linting issues.packages/cloudflare-api/src/cloudflare_api/ip_groups/manager.py (1)
129-140: LGTM: Proper use of MD5 for cache key generation.The use of
usedforsecurity=False(line 140) correctly documents that MD5 is used only for cache key generation, not cryptographic purposes. The inline comment reinforces this intent..github/workflows/sonarcloud.yml (1)
134-134: Excellent security improvement!Pinning the SonarCloud Quality Gate action to a specific commit SHA follows GitHub Actions security best practices and prevents potential supply chain attacks from tag manipulation.
pyproject.toml (3)
266-269: LGTM - correct compatibility handling.Ignoring UP017 is appropriate since
datetime.UTCwas introduced in Python 3.11 and the project supports Python 3.10+.
315-338: Good consistency with existing test rules.The relaxed lint rules for package tests mirror the existing root test configuration, ensuring consistent test code standards across the monorepo. The additions of
PT011(pytest.raises match) andF841(unused variables in test setup) are reasonable for package test contexts.
361-364: LGTM - appropriate exception for CLI tools.Allowing print statements in CLI files is correct since command-line interfaces use
print()for user-facing output.
| determine-package: | ||
| runs-on: ubuntu-latest | ||
| outputs: | ||
| package_dir: ${{ steps.parse.outputs.package_dir }} | ||
| package_name: ${{ steps.parse.outputs.package_name }} | ||
| version: ${{ steps.parse.outputs.version }} | ||
| steps: | ||
| - name: Parse tag | ||
| id: parse | ||
| run: | | ||
| TAG="${{ github.ref_name }}" | ||
| echo "Processing tag: $TAG" | ||
|
|
||
| if [[ "$TAG" == cloudflare-auth-v* ]]; then | ||
| echo "package_dir=packages/cloudflare-auth" >> $GITHUB_OUTPUT | ||
| echo "package_name=byronwilliamscpa-cloudflare-auth" >> $GITHUB_OUTPUT | ||
| echo "version=${TAG#cloudflare-auth-v}" >> $GITHUB_OUTPUT | ||
| elif [[ "$TAG" == gcs-utilities-v* ]]; then | ||
| echo "package_dir=packages/gcs-utilities" >> $GITHUB_OUTPUT | ||
| echo "package_name=byronwilliamscpa-gcs-utilities" >> $GITHUB_OUTPUT | ||
| echo "version=${TAG#gcs-utilities-v}" >> $GITHUB_OUTPUT | ||
| else | ||
| echo "::error::Unknown tag format: $TAG" | ||
| exit 1 | ||
| fi |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add timeout to prevent indefinite hangs.
The determine-package job lacks a timeout-minutes setting. While this job is quick, adding a timeout prevents resource waste if the job hangs unexpectedly.
Apply this diff:
jobs:
determine-package:
runs-on: ubuntu-latest
+ timeout-minutes: 5
outputs:🤖 Prompt for AI Agents
In .github/workflows/publish.yml around lines 24 to 48 the determine-package job
has no timeout-minutes set, which can let it hang indefinitely; add a
timeout-minutes entry (e.g., timeout-minutes: 5) directly under the
determine-package: or runs-on: ubuntu-latest line (with the same indentation
level as runs-on) to enforce a short max runtime and prevent resource waste if
the job hangs.
| build-and-publish: | ||
| needs: determine-package | ||
| runs-on: ubuntu-latest | ||
| steps: | ||
| - name: Checkout repository | ||
| uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2 | ||
|
|
||
| - name: Fetch secrets from Infisical | ||
| uses: Infisical/secrets-action@03d3fa38607956c493f53c6633f94006a13c47ae # v1.0.7 | ||
| with: | ||
| client-id: ${{ secrets.INFISICAL_CLIENT_ID }} | ||
| client-secret: ${{ secrets.INFISICAL_CLIENT_SECRET }} | ||
| env-slug: ${{ env.INFISICAL_ENV }} | ||
| project-slug: ${{ env.INFISICAL_PROJECT }} | ||
| domain: ${{ env.INFISICAL_DOMAIN }} | ||
|
|
||
| - name: Install uv | ||
| uses: astral-sh/setup-uv@6b9c6063abd6010835644d4c2e1bef4cf5cd0fca # v6.0.1 | ||
| with: | ||
| enable-cache: true | ||
|
|
||
| - name: Set up Python | ||
| run: uv python install 3.12 |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add timeout to prevent indefinite publishing jobs.
The build-and-publish job lacks a timeout-minutes setting. Publishing workflows should have timeouts to prevent indefinite hangs, especially when interacting with external registries.
Apply this diff:
build-and-publish:
needs: determine-package
runs-on: ubuntu-latest
+ timeout-minutes: 15
steps:🤖 Prompt for AI Agents
.github/workflows/publish.yml lines 50-72: the build-and-publish job is missing
a timeout-minutes setting which can allow the workflow to hang indefinitely; add
a timeout-minutes: 60 (or your preferred minute value) at the same indentation
level as runs-on (under the job name) to ensure the job is automatically
cancelled after the specified time, keeping the value as an integer and
preserving YAML indentation.
| - name: Verify version matches tag | ||
| working-directory: ${{ needs.determine-package.outputs.package_dir }} | ||
| run: | | ||
| TOML_VERSION=$(grep '^version = ' pyproject.toml | sed 's/version = "\(.*\)"/\1/') | ||
| TAG_VERSION="${{ needs.determine-package.outputs.version }}" | ||
| if [[ "$TOML_VERSION" != "$TAG_VERSION" ]]; then | ||
| echo "::error::Version mismatch! pyproject.toml=$TOML_VERSION, tag=$TAG_VERSION" | ||
| exit 1 | ||
| fi |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider using a TOML parser for more robust version extraction.
The sed command assumes specific formatting and could break with comments, different quote styles, or whitespace variations. Using a proper TOML parser would be more reliable.
Consider this alternative using Python's built-in TOML support:
- name: Verify version matches tag
working-directory: ${{ needs.determine-package.outputs.package_dir }}
run: |
- TOML_VERSION=$(grep '^version = ' pyproject.toml | sed 's/version = "\(.*\)"/\1/')
+ TOML_VERSION=$(uv run python -c "import tomllib; print(tomllib.load(open('pyproject.toml', 'rb'))['project']['version'])")
TAG_VERSION="${{ needs.determine-package.outputs.version }}"Note: tomllib is available in Python 3.11+. For Python 3.10, you'd need tomli.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - name: Verify version matches tag | |
| working-directory: ${{ needs.determine-package.outputs.package_dir }} | |
| run: | | |
| TOML_VERSION=$(grep '^version = ' pyproject.toml | sed 's/version = "\(.*\)"/\1/') | |
| TAG_VERSION="${{ needs.determine-package.outputs.version }}" | |
| if [[ "$TOML_VERSION" != "$TAG_VERSION" ]]; then | |
| echo "::error::Version mismatch! pyproject.toml=$TOML_VERSION, tag=$TAG_VERSION" | |
| exit 1 | |
| fi | |
| - name: Verify version matches tag | |
| working-directory: ${{ needs.determine-package.outputs.package_dir }} | |
| run: | | |
| TOML_VERSION=$(uv run python -c "import tomllib; print(tomllib.load(open('pyproject.toml', 'rb'))['project']['version'])") | |
| TAG_VERSION="${{ needs.determine-package.outputs.version }}" | |
| if [[ "$TOML_VERSION" != "$TAG_VERSION" ]]; then | |
| echo "::error::Version mismatch! pyproject.toml=$TOML_VERSION, tag=$TAG_VERSION" | |
| exit 1 | |
| fi |
🤖 Prompt for AI Agents
.github/workflows/publish.yml around lines 74 to 82: the current grep/sed
approach brittlely parses pyproject.toml and can fail with different
formatting/comments/quotes; replace the shell text-extraction with a small
Python invocation that reads pyproject.toml and uses a real TOML parser to
extract the [project]/version (use tomllib on Python 3.11+ and fall back to
tomli if not available), print that version to stdout for comparison with the
tag, and keep the existing exit-on-mismatch logic; ensure the action still runs
in the same working-directory and add a short comment noting the parser
fallback.
| - name: Install keyring for Artifact Registry | ||
| run: pip install keyrings.google-artifactregistry-auth |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
git ls-files .github/workflows/publish.ymlRepository: ByronWilliamsCPA/python-libs
Length of output: 101
🏁 Script executed:
cat -n .github/workflows/publish.yml | sed -n '80,105p'Repository: ByronWilliamsCPA/python-libs
Length of output: 1370
🌐 Web query:
keyrings.google-artifactregistry-auth latest version PyPI
💡 Result:
Latest PyPI version: 1.1.2 — uploaded Mar 1, 2023. [1]
Source: PyPI project page for keyrings.google-artifactregistry-auth. [1]
Pin the keyring package version for supply chain security.
Installing keyrings.google-artifactregistry-auth without a version constraint creates a supply chain risk. The package version could change between workflow runs, potentially introducing vulnerabilities or breaking changes.
Apply this diff to pin to the latest stable version:
- name: Install keyring for Artifact Registry
- run: pip install keyrings.google-artifactregistry-auth
+ run: pip install keyrings.google-artifactregistry-auth==1.1.2📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - name: Install keyring for Artifact Registry | |
| run: pip install keyrings.google-artifactregistry-auth | |
| - name: Install keyring for Artifact Registry | |
| run: pip install keyrings.google-artifactregistry-auth==1.1.2 |
🤖 Prompt for AI Agents
.github/workflows/publish.yml around lines 89-90: the workflow installs
keyrings.google-artifactregistry-auth without a version pin; replace the
unconstrained pip install with a pinned install (pip install
keyrings.google-artifactregistry-auth==<tested-stable-version>) using the
specific version you validated for this repo (or reference a repo
constant/variable), commit that change and update any workflow docs noting the
chosen version.
| - name: Build package | ||
| working-directory: ${{ needs.determine-package.outputs.package_dir }} | ||
| run: uv build | ||
|
|
||
| - name: Publish to Artifact Registry | ||
| working-directory: ${{ needs.determine-package.outputs.package_dir }} | ||
| run: uv publish --publish-url ${{ env.ARTIFACT_REGISTRY_URL }} |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding package verification before publishing.
Publishing immediately after building without verification is risky. Consider adding a validation step to check package metadata and catch issues before they reach the registry.
Add a verification step between build and publish:
- name: Build package
working-directory: ${{ needs.determine-package.outputs.package_dir }}
run: uv build
+ - name: Verify package
+ working-directory: ${{ needs.determine-package.outputs.package_dir }}
+ run: |
+ # Check that dist files were created
+ ls -lh dist/
+ # Verify package metadata (requires twine)
+ uv run pip install twine
+ uv run twine check dist/*
- name: Publish to Artifact Registry🤖 Prompt for AI Agents
In .github/workflows/publish.yml around lines 92 to 98, add a verification step
between the Build package and Publish to Artifact Registry steps that runs
package validation in the same working-directory (${{
needs.determine-package.outputs.package_dir }}); the step should validate
package metadata (e.g., version, manifest, checksums), run any lightweight
post-build checks (lint/verify or unit smoke tests), and fail the job on errors
so publishing is blocked when verification fails.
🧹 Nitpick | 🔵 Trivial
Consider uploading build artifacts for audit trail.
Uploading the built distributions as workflow artifacts provides an audit trail and allows manual inspection if issues arise. This is particularly valuable for published packages.
Add after the build step:
- name: Build package
working-directory: ${{ needs.determine-package.outputs.package_dir }}
run: uv build
+ - name: Upload build artifacts
+ uses: actions/upload-artifact@6f51ac03b9356f520e9adb1b1b7802705f340c2b # v4.5.0
+ with:
+ name: dist-${{ needs.determine-package.outputs.package_name }}-${{ needs.determine-package.outputs.version }}
+ path: ${{ needs.determine-package.outputs.package_dir }}/dist/
+ retention-days: 90
- name: Publish to Artifact Registry🤖 Prompt for AI Agents
.github/workflows/publish.yml around lines 92 to 98: after the "Build package"
step, add a step to upload the built distribution artifacts (e.g., the dist or
build output directory from ${{ needs.determine-package.outputs.package_dir }})
using actions/upload-artifact so the workflow retains a copy for auditing and
manual inspection; specify a clear artifact name (like package-build) and set
the artifact path to the build output directory (and optionally a retention-days
value) to ensure the built files are stored before publishing.
| """IP Group Manager for syncing IP ranges to Cloudflare. | ||
|
|
||
| Orchestrates fetching IPs from various sources and syncing to Cloudflare lists. | ||
| """ | ||
|
|
||
| import hashlib | ||
| import json | ||
| import logging | ||
| import time | ||
| from dataclasses import dataclass, field | ||
| from datetime import datetime, timedelta, timezone | ||
| from pathlib import Path | ||
| from typing import Any | ||
|
|
||
| from cloudflare_api.client import CloudflareAPIClient | ||
| from cloudflare_api.ip_groups.config import ( | ||
| IPGroupConfig, | ||
| IPGroupsConfig, | ||
| IPSourceConfig, | ||
| load_config, | ||
| ) | ||
| from cloudflare_api.ip_groups.fetchers import get_fetcher | ||
|
|
||
| logger = logging.getLogger(__name__) | ||
|
|
||
|
|
||
| @dataclass | ||
| class SyncResult: | ||
| """Result of syncing an IP group. | ||
|
|
||
| Attributes: | ||
| group_name: Name of the IP group | ||
| cloudflare_list_name: Cloudflare list name | ||
| cloudflare_list_id: Cloudflare list ID | ||
| ips_count: Number of IPs synced | ||
| added: Number of IPs added | ||
| removed: Number of IPs removed | ||
| unchanged: Whether the list was unchanged | ||
| error: Error message if sync failed | ||
| duration_seconds: Time taken to sync | ||
| """ | ||
|
|
||
| group_name: str | ||
| cloudflare_list_name: str | ||
| cloudflare_list_id: str | None = None | ||
| ips_count: int = 0 | ||
| added: int = 0 | ||
| removed: int = 0 | ||
| unchanged: bool = False | ||
| error: str | None = None | ||
| duration_seconds: float = 0.0 | ||
|
|
||
|
|
||
| @dataclass | ||
| class IPCache: | ||
| """Cache for fetched IP ranges. | ||
|
|
||
| Attributes: | ||
| ips: Cached IP addresses | ||
| fetched_at: When the IPs were fetched | ||
| source_hash: Hash of the source config for invalidation | ||
| """ | ||
|
|
||
| ips: list[str] = field(default_factory=list) | ||
| fetched_at: datetime = field(default_factory=datetime.now) | ||
| source_hash: str = "" | ||
|
|
||
|
|
||
| class IPGroupManager: | ||
| """Manager for IP range groups. | ||
|
|
||
| Handles fetching IPs from various sources and syncing them to Cloudflare. | ||
|
|
||
| Example: | ||
| ```python | ||
| manager = IPGroupManager.from_config("ip_groups.yaml") | ||
|
|
||
| # Sync all groups | ||
| results = manager.sync_all() | ||
|
|
||
| # Sync a specific group | ||
| result = manager.sync_group("github-actions") | ||
|
|
||
| # Preview changes without applying | ||
| preview = manager.preview_group("home-network") | ||
| ``` | ||
| """ | ||
|
|
||
| def __init__( | ||
| self, | ||
| config: IPGroupsConfig, | ||
| client: CloudflareAPIClient | None = None, | ||
| ) -> None: | ||
| """Initialize the IP Group Manager. | ||
|
|
||
| Args: | ||
| config: IP groups configuration. | ||
| client: Optional Cloudflare client. If not provided, creates one. | ||
| """ | ||
| self.config = config | ||
| self._client = client | ||
| self._cache: dict[str, IPCache] = {} | ||
|
|
||
| @classmethod | ||
| def from_config( | ||
| cls, | ||
| config_path: str | Path, | ||
| client: CloudflareAPIClient | None = None, | ||
| ) -> "IPGroupManager": | ||
| """Create a manager from a config file. | ||
|
|
||
| Args: | ||
| config_path: Path to the YAML config file. | ||
| client: Optional Cloudflare client. | ||
|
|
||
| Returns: | ||
| Configured IPGroupManager. | ||
| """ | ||
| config = load_config(config_path) | ||
| return cls(config, client) | ||
|
|
||
| @property | ||
| def client(self) -> CloudflareAPIClient: | ||
| """Get or create the Cloudflare client.""" | ||
| if self._client is None: | ||
| self._client = CloudflareAPIClient() | ||
| return self._client | ||
|
|
||
| def _get_source_hash(self, source: IPSourceConfig) -> str: | ||
| """Get a hash of the source config for cache invalidation. | ||
|
|
||
| Args: | ||
| source: Source configuration. | ||
|
|
||
| Returns: | ||
| Hash string. | ||
| """ | ||
| config_str = json.dumps(source.model_dump(), sort_keys=True) | ||
| # MD5 used only for cache key generation, not security purposes | ||
| return hashlib.md5(config_str.encode(), usedforsecurity=False).hexdigest()[:8] | ||
|
|
||
| def _is_cache_valid(self, cache: IPCache, source: IPSourceConfig) -> bool: | ||
| """Check if cached IPs are still valid. | ||
|
|
||
| Args: | ||
| cache: Cached data. | ||
| source: Source configuration. | ||
|
|
||
| Returns: | ||
| True if cache is valid. | ||
| """ | ||
| # Check if source config changed | ||
| if cache.source_hash != self._get_source_hash(source): | ||
| return False | ||
|
|
||
| # Check TTL | ||
| ttl = timedelta(seconds=self.config.cache_ttl_seconds) | ||
| return datetime.now(tz=timezone.utc) - cache.fetched_at <= ttl | ||
|
|
||
| def fetch_source_ips( | ||
| self, | ||
| source: IPSourceConfig, | ||
| use_cache: bool = True, | ||
| ) -> list[str]: | ||
| """Fetch IPs from a single source. | ||
|
|
||
| Args: | ||
| source: Source configuration. | ||
| use_cache: Whether to use cached results. | ||
|
|
||
| Returns: | ||
| List of IP addresses. | ||
| """ | ||
| cache_key = self._get_source_hash(source) | ||
|
|
||
| # Check cache | ||
| if use_cache and cache_key in self._cache: | ||
| cache = self._cache[cache_key] | ||
| if self._is_cache_valid(cache, source): | ||
| logger.debug("Using cached IPs for %s", source.type.value) | ||
| return cache.ips | ||
|
|
||
| # Fetch from source | ||
| fetcher = get_fetcher(source.type) | ||
| ips = fetcher.fetch(source) | ||
|
|
||
| # Update cache | ||
| self._cache[cache_key] = IPCache( | ||
| ips=ips, | ||
| fetched_at=datetime.now(tz=timezone.utc), | ||
| source_hash=self._get_source_hash(source), | ||
| ) | ||
|
|
||
| return ips | ||
|
|
||
| def fetch_group_ips( | ||
| self, | ||
| group: IPGroupConfig, | ||
| use_cache: bool = True, | ||
| ) -> list[str]: | ||
| """Fetch all IPs for a group from all sources. | ||
|
|
||
| Args: | ||
| group: Group configuration. | ||
| use_cache: Whether to use cached results. | ||
|
|
||
| Returns: | ||
| Deduplicated list of IP addresses. | ||
| """ | ||
| all_ips: set[str] = set() | ||
|
|
||
| for source in group.sources: | ||
| try: | ||
| ips = self.fetch_source_ips(source, use_cache) | ||
| all_ips.update(ips) | ||
| except Exception: | ||
| logger.exception( | ||
| "Failed to fetch IPs from %s source", | ||
| source.type.value, | ||
| ) | ||
| raise | ||
|
|
||
| logger.info( | ||
| "Fetched %d unique IPs for group '%s' from %d sources", | ||
| len(all_ips), | ||
| group.name, | ||
| len(group.sources), | ||
| ) | ||
|
|
||
| return sorted(all_ips) | ||
|
|
||
| def preview_group(self, group_name: str) -> dict[str, Any]: | ||
| """Preview what would change for a group without applying. | ||
|
|
||
| Args: | ||
| group_name: Name of the group to preview. | ||
|
|
||
| Returns: | ||
| Dict with current and new IPs, and diff. | ||
|
|
||
| Raises: | ||
| ValueError: If group not found. | ||
| """ | ||
| group = self._get_group(group_name) | ||
|
|
||
| # Fetch new IPs | ||
| new_ips = set(self.fetch_group_ips(group)) | ||
|
|
||
| # Get current IPs from Cloudflare | ||
| list_name = self._get_cloudflare_list_name(group) | ||
| current_ips: set[str] = set() | ||
|
|
||
| existing_list = self.client.get_ip_list_by_name(list_name) | ||
| if existing_list: | ||
| items = self.client.get_ip_list_items(existing_list.id) | ||
| current_ips = {item.ip for item in items} | ||
|
|
||
| # Calculate diff | ||
| to_add = new_ips - current_ips | ||
| to_remove = current_ips - new_ips | ||
| unchanged = current_ips & new_ips | ||
|
|
||
| return { | ||
| "group_name": group.name, | ||
| "cloudflare_list_name": list_name, | ||
| "current_count": len(current_ips), | ||
| "new_count": len(new_ips), | ||
| "to_add": sorted(to_add), | ||
| "to_remove": sorted(to_remove), | ||
| "unchanged_count": len(unchanged), | ||
| "will_change": bool(to_add or to_remove), | ||
| } | ||
|
|
||
| def sync_group(self, group_name: str, dry_run: bool = False) -> SyncResult: | ||
| """Sync a single IP group to Cloudflare. | ||
|
|
||
| Args: | ||
| group_name: Name of the group to sync. | ||
| dry_run: If True, preview without applying changes. | ||
|
|
||
| Returns: | ||
| SyncResult with details of the operation. | ||
|
|
||
| Raises: | ||
| ValueError: If group not found or disabled. | ||
| """ | ||
| start_time = time.time() | ||
| group = self._get_group(group_name) | ||
|
|
||
| if not group.enabled: | ||
| return SyncResult( | ||
| group_name=group.name, | ||
| cloudflare_list_name=self._get_cloudflare_list_name(group), | ||
| error="Group is disabled", | ||
| ) | ||
|
|
||
| list_name = self._get_cloudflare_list_name(group) | ||
|
|
||
| try: | ||
| # Fetch new IPs | ||
| new_ips = self.fetch_group_ips(group) | ||
|
|
||
| if dry_run: | ||
| preview = self.preview_group(group_name) | ||
| return SyncResult( | ||
| group_name=group.name, | ||
| cloudflare_list_name=list_name, | ||
| ips_count=len(new_ips), | ||
| added=len(preview["to_add"]), | ||
| removed=len(preview["to_remove"]), | ||
| unchanged=not preview["will_change"], | ||
| duration_seconds=time.time() - start_time, | ||
| ) | ||
|
|
||
| # Ensure list exists | ||
| cf_list = self.client.ensure_ip_list( | ||
| name=list_name, | ||
| kind="ip", | ||
| description=group.description or f"Managed IP group: {group.name}", | ||
| ) | ||
|
|
||
| # Get current items for diff calculation | ||
| current_items = self.client.get_ip_list_items(cf_list.id) | ||
| current_ips = {item.ip for item in current_items} | ||
| new_ip_set = set(new_ips) | ||
|
|
||
| added = len(new_ip_set - current_ips) | ||
| removed = len(current_ips - new_ip_set) | ||
|
|
||
| if added == 0 and removed == 0: | ||
| logger.info("No changes needed for '%s'", group.name) | ||
| return SyncResult( | ||
| group_name=group.name, | ||
| cloudflare_list_name=list_name, | ||
| cloudflare_list_id=cf_list.id, | ||
| ips_count=len(new_ips), | ||
| unchanged=True, | ||
| duration_seconds=time.time() - start_time, | ||
| ) | ||
|
|
||
| # Sync the list | ||
| comments = dict.fromkeys(new_ips, f"Managed by {group.name}") | ||
| self.client.sync_ip_list(cf_list.id, new_ips, comments) | ||
|
|
||
| logger.info( | ||
| "Synced '%s' to Cloudflare: %d IPs (+%d, -%d)", | ||
| group.name, | ||
| len(new_ips), | ||
| added, | ||
| removed, | ||
| ) | ||
|
|
||
| return SyncResult( | ||
| group_name=group.name, | ||
| cloudflare_list_name=list_name, | ||
| cloudflare_list_id=cf_list.id, | ||
| ips_count=len(new_ips), | ||
| added=added, | ||
| removed=removed, | ||
| duration_seconds=time.time() - start_time, | ||
| ) | ||
|
|
||
| except Exception: | ||
| logger.exception("Failed to sync group '%s'", group.name) | ||
| return SyncResult( | ||
| group_name=group.name, | ||
| cloudflare_list_name=list_name, | ||
| error="Sync failed - see logs for details", | ||
| duration_seconds=time.time() - start_time, | ||
| ) | ||
|
|
||
| def sync_all(self, dry_run: bool = False) -> list[SyncResult]: | ||
| """Sync all enabled IP groups to Cloudflare. | ||
|
|
||
| Args: | ||
| dry_run: If True, preview without applying changes. | ||
|
|
||
| Returns: | ||
| List of SyncResults for each group. | ||
| """ | ||
| results = [] | ||
|
|
||
| for group in self.config.groups: | ||
| if group.enabled: | ||
| result = self.sync_group(group.name, dry_run=dry_run) | ||
| results.append(result) | ||
| else: | ||
| logger.debug("Skipping disabled group: %s", group.name) | ||
|
|
||
| # Log summary | ||
| success = sum(1 for r in results if r.error is None) | ||
| failed = sum(1 for r in results if r.error is not None) | ||
| total_ips = sum(r.ips_count for r in results if r.error is None) | ||
|
|
||
| logger.info( | ||
| "Sync complete: %d groups succeeded, %d failed, %d total IPs", | ||
| success, | ||
| failed, | ||
| total_ips, | ||
| ) | ||
|
|
||
| return results | ||
|
|
||
| def list_groups(self) -> list[dict[str, Any]]: | ||
| """List all configured IP groups. | ||
|
|
||
| Returns: | ||
| List of group summaries. | ||
| """ | ||
| return [ | ||
| { | ||
| "name": group.name, | ||
| "cloudflare_list_name": self._get_cloudflare_list_name(group), | ||
| "description": group.description, | ||
| "enabled": group.enabled, | ||
| "sources_count": len(group.sources), | ||
| "source_types": [s.type.value for s in group.sources], | ||
| "tags": group.tags, | ||
| } | ||
| for group in self.config.groups | ||
| ] | ||
|
|
||
| def _get_group(self, group_name: str) -> IPGroupConfig: | ||
| """Get a group by name. | ||
|
|
||
| Args: | ||
| group_name: Name of the group. | ||
|
|
||
| Returns: | ||
| Group configuration. | ||
|
|
||
| Raises: | ||
| ValueError: If group not found. | ||
| """ | ||
| for group in self.config.groups: | ||
| if group.name == group_name: | ||
| return group | ||
|
|
||
| available = [g.name for g in self.config.groups] | ||
| msg = f"Group '{group_name}' not found. Available: {available}" | ||
| raise ValueError(msg) | ||
|
|
||
| def _get_cloudflare_list_name(self, group: IPGroupConfig) -> str: | ||
| """Get the Cloudflare list name for a group. | ||
|
|
||
| Args: | ||
| group: Group configuration. | ||
|
|
||
| Returns: | ||
| Cloudflare list name with optional prefix. | ||
| """ | ||
| prefix = self.config.cloudflare_list_prefix | ||
| if prefix: | ||
| return f"{prefix}{group.cloudflare_list_name}" | ||
| return group.cloudflare_list_name | ||
|
|
||
| def clear_cache(self) -> None: | ||
| """Clear all cached IP data.""" | ||
| self._cache.clear() | ||
| logger.info("Cleared IP cache") |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding assumption tags per coding guidelines.
The coding guidelines require tagging assumptions with #CRITICAL, #ASSUME, or #EDGE comments. Consider adding these tags to document implicit assumptions, particularly:
- Line 140:
#ASSUME: MD5 collision risk acceptable for cache keys - Line 216:
#EDGE: Catch-all for unexpected fetcher failures - Line 158:
#ASSUME: TTL validation expects reasonable cache_ttl_seconds range
Based on coding guidelines, this helps with verification during reviews and maintenance.
🤖 Prompt for AI Agents
packages/cloudflare-api/src/cloudflare_api/ip_groups/manager.py lines ~140,
~158, ~216: add the required assumption/edge tags as inline comments at the
noted locations to satisfy coding guidelines; specifically insert "#ASSUME: MD5
collision risk acceptable for cache keys" next to the MD5 usage (around the
_get_source_hash definition/MD5 call), insert "#ASSUME: TTL validation expects
reasonable cache_ttl_seconds range" near the TTL/check in _is_cache_valid, and
insert "#EDGE: Catch-all for unexpected fetcher failures" on the except block in
fetch_group_ips; keep comments short, on their own line or trailing the existing
line, and do not change logic.
| ) | ||
|
|
||
| # Sync the list | ||
| comments = dict.fromkeys(new_ips, f"Managed by {group.name}") |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding a clarifying comment for dict.fromkeys usage.
The use of dict.fromkeys(new_ips, f"Managed by {group.name}") on line 342 is correct but might benefit from a brief inline comment explaining that this creates a comment mapping for the Cloudflare API where all IPs receive the same comment value.
For example:
- comments = dict.fromkeys(new_ips, f"Managed by {group.name}")
+ # Create uniform comment for all IPs in this group
+ comments = dict.fromkeys(new_ips, f"Managed by {group.name}")🤖 Prompt for AI Agents
In packages/cloudflare-api/src/cloudflare_api/ip_groups/manager.py around line
342, add a brief inline comment explaining the use of dict.fromkeys(new_ips,
f"Managed by {group.name}") — note that it creates a mapping of each new IP to
the same comment string for the Cloudflare API — so future readers understand
why fromkeys is used rather than a comprehension or loop.
| Dictionary with session statistics | ||
| """ | ||
| datetime.now(tz=UTC) | ||
| datetime.now(tz=timezone.utc) |
There was a problem hiding this comment.
Dead code: unused expression.
This line computes a datetime but discards the result. Either remove it or assign it to a variable if it was intended to be used.
- datetime.now(tz=timezone.utc)🤖 Prompt for AI Agents
In packages/cloudflare-auth/src/cloudflare_auth/sessions.py around line 289,
there is a dead expression "datetime.now(tz=timezone.utc)" whose result is
discarded; remove the line if the current time is not needed, or assign it to a
variable (e.g., now = datetime.now(tz=timezone.utc)) and use that variable where
intended (for timestamping, comparisons, or logging) so the expression is not
unused.
2 participants
Summary
src.config.settingsdoes not exist)cloudflare-apipackage for Cloudflare API operationsChanges
cloudflare-auth fixes (Issue #2)
cloudflare_auth/config.pywithCloudflareSettingsPydantic settings classget_cloudflare_settings()factory function with LRU cachingvalidators.py,middleware.py,middleware_enhanced.pyto usecloudflare_auth.configNew cloudflare-api package
Documentation
CI/CD
Test plan
Related
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation
Configuration
Tests & Quality
✏️ Tip: You can customize this high-level summary in your review settings.