Repository navigation
docs(adr): durable remote session and workspace retention - #161
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 (1)
📝 WalkthroughWalkthroughAdded ADR-0025 defining a PostgreSQL-backed control-plane layer for durable remote session and workspace retention. The document specifies core data models, updated lifecycle semantics, API/CLI extensions, configuration requirements, design rationale, phased implementation plan across six PRs, and acceptance criteria. ChangesDurable Remote Session and Workspace Retention Architecture
Possibly Related PRs
Estimated Code Review Effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
coding-agent/docs/adr/0025-durable-remote-session-and-workspace-retention.md (4)
209-214: ⚡ Quick winClarify the configuration section name and fallback behavior.
The example introduces a
[remote_retention]configuration section, but this section name appears nowhere else in the document. Consider adding a brief note about:
- Whether
[remote_retention]is the final section name or placeholder- Behavior when
remote_retention.enabled = falsewhilecloud_workspace.enabled = true(does it use the old filesystem-scan-only mode?)- Relationship between this section and the production validation mentioned at line 222-224
This ensures configuration semantics are clear for implementers.
🤖 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/docs/adr/0025-durable-remote-session-and-workspace-retention.md` around lines 209 - 214, Clarify the [remote_retention] config section and fallback behavior: state explicitly whether "[remote_retention]" is the final canonical section name or a placeholder and update any other docs to match; document what happens when remote_retention.enabled = false while cloud_workspace.enabled = true (e.g., whether the system falls back to the legacy filesystem-scan-only mode) and describe how that fallback interacts with the production validation referenced in the "production validation" lines; finally, add a short note linking the [remote_retention] settings (default_policy, default_ttl_seconds, allow_user_pin) to the production validation requirements so implementers know which combinations are validated in prod.
58-58: ⚡ Quick winClarify "provider-neutral" terminology.
The workspace record is described as "provider-neutral" but line 74 shows it requires a
providerfield. Consider rephrasing to "provider-agnostic schema" or "multi-provider workspace record" to clarify that while the record structure works across provider types, each record must specify its provider.This prevents confusion about whether the record actually tracks provider information.
🤖 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/docs/adr/0025-durable-remote-session-and-workspace-retention.md` at line 58, The term "provider-neutral" is misleading because the workspace record requires a provider field; update the wording in the ADR where "provider-neutral" appears to a clearer phrase such as "provider-agnostic schema" or "multi-provider workspace record" and add one short clarifying sentence that each workspace record must still include a provider field (e.g., reference "workspace record" and the "provider" field) to make explicit that the schema is usable across providers but each record records its provider.
274-274: ⚡ Quick winConsider using pytest markers instead of complex
-kfilters.The pytest command uses a long keyword filter:
"workspace_record or retention or provider_instance or pin or cleanup_failed". As the implementation grows, this filter may become fragile and hard to maintain.Consider introducing a pytest marker (e.g.,
@pytest.mark.workspace_retention) for tests related to this feature, then run:uv run pytest tests/ui tests/coding_agent/environment -m workspace_retention -vThis provides more explicit test organization and is easier to maintain across the 6-PR implementation plan.
🤖 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/docs/adr/0025-durable-remote-session-and-workspace-retention.md` at line 274, Replace the fragile -k keyword filter with a dedicated pytest marker: add `@pytest.mark.workspace_retention` to all tests covering workspace_record/retention/provider_instance/pin/cleanup_failed in tests/ui and tests/coding_agent/environment, run them with uv run pytest ... -m workspace_retention -v, and register the marker (workspace_retention) in pytest.ini under markers to avoid "unknown marker" warnings; ensure test names remain unchanged and only the marker annotations and pytest.ini entries are added.
107-107: ⚡ Quick winDefine when a workspace transitions to
loststatus.The
loststatus is enumerated but never explained in the document. Consider adding a brief explanation of when this status applies (e.g., workspace record exists but underlying provider resources cannot be found or verified).This would help implementers understand the complete state machine.
🤖 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/docs/adr/0025-durable-remote-session-and-workspace-retention.md` at line 107, Add a short definition for the `lost` workspace status in the ADR: state that `lost` applies when a workspace record exists but the underlying provider resources cannot be found or verified (e.g., API 404s, missing VM/container, or auth failures), list the detection criteria (failed discovery, inconsistent resource IDs, repeated health-check timeouts), and indicate expected handling/remediation (mark `lost`, alert owners, schedule cleanup or manual recovery). Reference the `lost` enum in the state machine and ensure transitions into/out-of `lost` are described consistently with existing statuses.
🤖 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/docs/adr/0025-durable-remote-session-and-workspace-retention.md`:
- Around line 51-56: The ADR mentions workspace_record_id but the "Required
workspace identity fields" list only includes workspace_id, causing ambiguity;
update the ADR to explicitly define workspace_record_id (and its relation to
workspace_id): state whether workspace_record_id is the same identifier as
workspace_id, a separate database primary key on the workspace table, or a
foreign-key reference from session to workspace, and then add
workspace_record_id to the "Required workspace identity fields" list with its
intended type and semantics (e.g., "workspace_record_id: UUID primary key of the
workspace table, used as FK from session records") so implementers can
unambiguously map session rows to workspace rows in functions handling session
persistence and resume.
- Around line 169-170: Clarify the unpin behavior in the ADR by explicitly
stating which approach is used: if unpin restores the actual previous policy,
update the data model to add a previous_retention_policy field on the Workspace
record and describe how/when it is set (e.g., Workspace.pin() saves current
retention to previous_retention_policy and Workspace.unpin() restores it);
otherwise state that unpin applies a configured default or requires an explicit
parameter (e.g., POST /workspaces/{workspace_id}/unpin accepts optional
--ttl/--policy or falls back to system_default_retention). Reference the POST
/workspaces/{workspace_id}/unpin operation and the Workspace data model (add
previous_retention_policy) in the ADR text so behavior and required fields are
unambiguous.
---
Nitpick comments:
In
`@coding-agent/docs/adr/0025-durable-remote-session-and-workspace-retention.md`:
- Around line 209-214: Clarify the [remote_retention] config section and
fallback behavior: state explicitly whether "[remote_retention]" is the final
canonical section name or a placeholder and update any other docs to match;
document what happens when remote_retention.enabled = false while
cloud_workspace.enabled = true (e.g., whether the system falls back to the
legacy filesystem-scan-only mode) and describe how that fallback interacts with
the production validation referenced in the "production validation" lines;
finally, add a short note linking the [remote_retention] settings
(default_policy, default_ttl_seconds, allow_user_pin) to the production
validation requirements so implementers know which combinations are validated in
prod.
- Line 58: The term "provider-neutral" is misleading because the workspace
record requires a provider field; update the wording in the ADR where
"provider-neutral" appears to a clearer phrase such as "provider-agnostic
schema" or "multi-provider workspace record" and add one short clarifying
sentence that each workspace record must still include a provider field (e.g.,
reference "workspace record" and the "provider" field) to make explicit that the
schema is usable across providers but each record records its provider.
- Line 274: Replace the fragile -k keyword filter with a dedicated pytest
marker: add `@pytest.mark.workspace_retention` to all tests covering
workspace_record/retention/provider_instance/pin/cleanup_failed in tests/ui and
tests/coding_agent/environment, run them with uv run pytest ... -m
workspace_retention -v, and register the marker (workspace_retention) in
pytest.ini under markers to avoid "unknown marker" warnings; ensure test names
remain unchanged and only the marker annotations and pytest.ini entries are
added.
- Line 107: Add a short definition for the `lost` workspace status in the ADR:
state that `lost` applies when a workspace record exists but the underlying
provider resources cannot be found or verified (e.g., API 404s, missing
VM/container, or auth failures), list the detection criteria (failed discovery,
inconsistent resource IDs, repeated health-check timeouts), and indicate
expected handling/remediation (mark `lost`, alert owners, schedule cleanup or
manual recovery). Reference the `lost` enum in the state machine and ensure
transitions into/out-of `lost` are described consistently with existing
statuses.
🪄 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: d775a71b-2f34-41aa-9bd1-c828bbdb1460
📒 Files selected for processing (1)
coding-agent/docs/adr/0025-durable-remote-session-and-workspace-retention.md
Summary
Validation
Summary by CodeRabbit