Repository navigation
feat: validate remote retention production config - #164
Conversation
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR adds configuration loading and production validation for durable remote workspace retention. It documents the ChangesRemote Retention Configuration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
coding-agent/src/coding_agent/ui/http_server.py (1)
424-426: 💤 Low valueClarify that
allow_user_pinis required when retention is enabled.The error message states "must be a boolean" but doesn't make it clear that the field is also required to be present (not just correctly typed). Consider making the error message more explicit about the requirement.
📝 Suggested improvement
allow_user_pin = remote_retention_config.get("allow_user_pin") if not isinstance(allow_user_pin, bool): - raise ValueError("remote_retention.allow_user_pin must be a boolean") + raise ValueError( + "remote_retention.allow_user_pin is required and must be a boolean" + )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@coding-agent/src/coding_agent/ui/http_server.py` around lines 424 - 426, The validation for remote_retention_config's allow_user_pin currently only raises "must be a boolean" but doesn't state the field is required when retention is enabled; update the check around allow_user_pin (the variable derived from remote_retention_config.get("allow_user_pin")) to raise a clearer ValueError that both insists the key be present and that its value be a boolean (e.g. "remote_retention.allow_user_pin is required when remote retention is enabled and must be a boolean"), or split into two checks that raise explicit errors if the key is missing vs. wrong type so callers of the code path using remote_retention_config (where allow_user_pin is validated) see the required-when-enabled semantic.coding-agent/tests/ui/test_http_server.py (1)
400-472: ⚡ Quick winConsider adding test cases for invalid retention policy values and negative TTL.
The parametrized test covers several important negative cases, but appears to be missing:
Invalid retention policy validation: According to the review context, production validation should constrain policies to allowed values (
delete_on_close,ttl,pinned,manual). Consider adding a test case that provides an invalid policy value like"forever"or"invalid"to verify this constraint is enforced during production config validation.Negative TTL values: Case 3 tests
default_ttl_seconds: 0, but consider also testing negative values (e.g.,-1) to ensure comprehensive bounds checking.📋 Suggested additional test cases
( {"provider_instance_id": "docker-host-a"}, {"http_session_backend": "pg", "dsn": "postgresql://example"}, { "enabled": True, "default_policy": "forever", "allow_user_pin": False, }, "remote_retention.default_policy", ), ( {"provider_instance_id": "docker-host-a"}, {"http_session_backend": "pg", "dsn": "postgresql://example"}, { "enabled": True, "default_policy": "ttl", "default_ttl_seconds": -3600, "allow_user_pin": False, }, "remote_retention.default_ttl_seconds", ),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@coding-agent/tests/ui/test_http_server.py` around lines 400 - 472, Add two parametrized negative cases to test_production_config_rejects_unsafe_remote_retention_config so production validation rejects unknown policies and negative TTLs: update the parameter list in coding-agent/tests/ui/test_http_server.py (the pytest.mark.parametrize for test_production_config_rejects_unsafe_remote_retention_config) to include a case with remote_retention.default_policy set to "forever" (expecting error match "remote_retention.default_policy") and a case with remote_retention.default_policy "ttl" and default_ttl_seconds -3600 (expecting "remote_retention.default_ttl_seconds"); these will exercise http_server._validate_production_config and ensure the allowed policy values and non-negative TTL checks are enforced.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@coding-agent/src/coding_agent/ui/http_server.py`:
- Around line 374-383: Remove the earlier duplicate definition of the function
_storage_uses_pg_http_sessions (the first occurrence in the file) so only the
later implementation remains; delete the entire first function block and keep
the later one (which Python will actually use) to avoid dead/conflicting code,
ensuring callers of _storage_uses_pg_http_sessions continue to refer to the
single remaining implementation.
---
Nitpick comments:
In `@coding-agent/src/coding_agent/ui/http_server.py`:
- Around line 424-426: The validation for remote_retention_config's
allow_user_pin currently only raises "must be a boolean" but doesn't state the
field is required when retention is enabled; update the check around
allow_user_pin (the variable derived from
remote_retention_config.get("allow_user_pin")) to raise a clearer ValueError
that both insists the key be present and that its value be a boolean (e.g.
"remote_retention.allow_user_pin is required when remote retention is enabled
and must be a boolean"), or split into two checks that raise explicit errors if
the key is missing vs. wrong type so callers of the code path using
remote_retention_config (where allow_user_pin is validated) see the
required-when-enabled semantic.
In `@coding-agent/tests/ui/test_http_server.py`:
- Around line 400-472: Add two parametrized negative cases to
test_production_config_rejects_unsafe_remote_retention_config so production
validation rejects unknown policies and negative TTLs: update the parameter list
in coding-agent/tests/ui/test_http_server.py (the pytest.mark.parametrize for
test_production_config_rejects_unsafe_remote_retention_config) to include a case
with remote_retention.default_policy set to "forever" (expecting error match
"remote_retention.default_policy") and a case with
remote_retention.default_policy "ttl" and default_ttl_seconds -3600 (expecting
"remote_retention.default_ttl_seconds"); these will exercise
http_server._validate_production_config and ensure the allowed policy values and
non-negative TTL checks are enforced.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: efa22937-b864-41fc-87f0-debb4b3e05c8
📒 Files selected for processing (3)
coding-agent/docs/remote-sandbox-production.mdcoding-agent/src/coding_agent/ui/http_server.pycoding-agent/tests/ui/test_http_server.py
Summary
Validation
Summary by CodeRabbit
New Features
delete_on_close,ttl,pinned,manual) for managing durable remote sessions.Documentation
Tests