Repository navigation
fix: restore compaction session state - #78
Conversation
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
|
Caution Review failedPull request was closed or merged during review No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdded a helper to compute tape window start from handoff anchors and refactored tape loading to use it. Added validation and sync logic when restoring REPL sessions so session config and context (model/provider/approval) are validated and applied before switching. New tests cover both areas. Changes
Sequence Diagram(s)sequenceDiagram
participant ManagedSession
participant InteractiveSession
participant Config
participant Runtime
ManagedSession->>InteractiveSession: provide restored session data
InteractiveSession->>InteractiveSession: _sync_config_from_managed_session(data)
InteractiveSession-->>Config: set provider/model/base_url/max_steps/approval_mode
InteractiveSession->>Runtime: rewire runtime using updated config
Runtime-->>InteractiveSession: confirm switch complete
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
coding-agent/tests/cli/test_repl.py (1)
787-791:⚠️ Potential issue | 🟠 MajorThese mocked sessions no longer satisfy
_switch_session’s required restored fields.Both fakes now include
model_name, but_switch_sessionalso validatesprovider_name,approval_policy, andmax_steps. These tests can fail before reaching the runtime-rebinding assertions.Proposed fix (apply to both mocked session objects)
+from coding_agent.approval import ApprovalPolicy @@ return SimpleNamespace( id="session-b", model_name="gpt-4o-mini", + provider_name="openai", + base_url=None, + max_steps=30, + approval_policy=ApprovalPolicy.AUTO, runtime_pipeline=fake_pipeline, runtime_ctx=fake_ctx, runtime_adapter=fake_adapter, )Also applies to: 849-853
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@coding-agent/tests/cli/test_repl.py` around lines 787 - 791, Tests create mocked session objects used by _switch_session but they only set model_name; update both mocked session dicts/objects (the ones passed into the test setup that currently set model_name, runtime_pipeline, runtime_ctx, runtime_adapter) to also include provider_name, approval_policy, and max_steps with valid values so they satisfy _switch_session validation; keep provider_name matching the expected provider in the test, set approval_policy to a valid policy object/string used elsewhere in tests, and set max_steps to an integer (e.g., 0 or the default used by other tests).
🧹 Nitpick comments (1)
coding-agent/tests/agentkit/tape/test_tape.py (1)
447-523: Add a multi-handofffrom_listtest to lock “latest anchor wins.”You already assert this behavior for JSONL; adding the same for
from_listwould directly protect the exact restore path this PR targets.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@coding-agent/tests/agentkit/tape/test_tape.py` around lines 447 - 523, Add a new unit test in coding-agent/tests/agentkit/tape/test_tape.py that exercises Tape.from_list with multiple handoff anchors to ensure the "latest anchor wins" behavior on restoration: construct entries_data containing at least two anchor entries with "anchor_type": "handoff" separated by messages, call Tape.from_list(entries_data) and assert tape.window_start equals the index of the last handoff anchor (and that windowed_entries() length matches expectation); name the test e.g. test_from_list_latest_handoff_wins and reference Tape.from_list and tape.window_start in the assertions so the exact restore path is covered.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@coding-agent/src/coding_agent/cli/repl.py`:
- Around line 245-246: The code currently sets self.context["model"] =
managed_session.model_name before calling
_sync_config_from_managed_session(managed_session), which can leave context
partially mutated if the sync/validation fails; change the order so you first
call _sync_config_from_managed_session(managed_session) and only if it
returns/raises successfully assign self.context["model"] (or alternatively
perform the assignment into a temporary variable and only set self.context on
success), ensuring that _sync_config_from_managed_session and
managed_session.model_name are used to validate/sync first and that context
mutation happens only after successful sync.
- Around line 201-239: The _sync_config_from_managed_session function
dereferences managed_session attributes directly which can raise AttributeError
and copies max_steps without validation; update the function to first safely
extract attributes using getattr or hasattr checks for provider_name,
model_name, approval_policy, base_url and max_steps, validate provider_name
against the allowed_providers set, ensure model_name is a non-empty str, map
approval_policy using the ApprovalPolicy enum (or isinstance/getattr) and raise
the same RuntimeError on invalid/missing values, and validate max_steps is an
int within an acceptable range (e.g., non-negative) before assigning to
self.config.max_steps; only mutate self.config.provider, .model, .base_url,
.max_steps and .approval_mode after all validations succeed.
---
Outside diff comments:
In `@coding-agent/tests/cli/test_repl.py`:
- Around line 787-791: Tests create mocked session objects used by
_switch_session but they only set model_name; update both mocked session
dicts/objects (the ones passed into the test setup that currently set
model_name, runtime_pipeline, runtime_ctx, runtime_adapter) to also include
provider_name, approval_policy, and max_steps with valid values so they satisfy
_switch_session validation; keep provider_name matching the expected provider in
the test, set approval_policy to a valid policy object/string used elsewhere in
tests, and set max_steps to an integer (e.g., 0 or the default used by other
tests).
---
Nitpick comments:
In `@coding-agent/tests/agentkit/tape/test_tape.py`:
- Around line 447-523: Add a new unit test in
coding-agent/tests/agentkit/tape/test_tape.py that exercises Tape.from_list with
multiple handoff anchors to ensure the "latest anchor wins" behavior on
restoration: construct entries_data containing at least two anchor entries with
"anchor_type": "handoff" separated by messages, call
Tape.from_list(entries_data) and assert tape.window_start equals the index of
the last handoff anchor (and that windowed_entries() length matches
expectation); name the test e.g. test_from_list_latest_handoff_wins and
reference Tape.from_list and tape.window_start in the assertions so the exact
restore path is covered.
🪄 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: c881236f-dbe6-4f52-9367-489a81a2fc68
📒 Files selected for processing (4)
coding-agent/src/agentkit/tape/tape.pycoding-agent/src/coding_agent/cli/repl.pycoding-agent/tests/agentkit/tape/test_tape.pycoding-agent/tests/cli/test_repl.py
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
coding-agent/tests/cli/test_repl.py (1)
979-1031: Extend missing-provider rollback assertions to all config fields.This test currently checks only
providerandmodel. Addbase_url,max_steps, andapproval_modeimmutability checks to catch partial mutation regressions.Suggested test hardening
with pytest.raises(RuntimeError, match="invalid provider_name"): await session._switch_session("session-b") assert session.context["model"] == "current-model" assert session.config.provider == "openai" assert session.config.model == "current-model" + assert session.config.base_url == "http://current.local" + assert session.config.max_steps == 30 + assert session.config.approval_mode == "auto"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@coding-agent/tests/cli/test_repl.py` around lines 979 - 1031, The test currently only asserts rollback for session.context["model"] and config.provider/model after InteractiveSession._switch_session raises; extend the assertions to also verify that session.config.base_url, session.config.max_steps, and session.config.approval_mode remain unchanged to catch partial mutation regressions. Update the test_switch_session_rejects_missing_restored_provider_without_mutating_state test to add asserts comparing session.config.base_url == "http://current.local", session.config.max_steps == 30, and session.config.approval_mode == "auto" (and keep the existing context/model checks) so the test ensures no fields on session.config or session.context are mutated when _switch_session fails.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@coding-agent/tests/cli/test_repl.py`:
- Around line 979-1031: The test currently only asserts rollback for
session.context["model"] and config.provider/model after
InteractiveSession._switch_session raises; extend the assertions to also verify
that session.config.base_url, session.config.max_steps, and
session.config.approval_mode remain unchanged to catch partial mutation
regressions. Update the
test_switch_session_rejects_missing_restored_provider_without_mutating_state
test to add asserts comparing session.config.base_url == "http://current.local",
session.config.max_steps == 30, and session.config.approval_mode == "auto" (and
keep the existing context/model checks) so the test ensures no fields on
session.config or session.context are mutated when _switch_session fails.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 07f11578-1f4e-46cb-aa7e-5860e6ab6307
📒 Files selected for processing (2)
coding-agent/src/coding_agent/cli/repl.pycoding-agent/tests/cli/test_repl.py
✅ Files skipped from review due to trivial changes (1)
- coding-agent/src/coding_agent/cli/repl.py
There was a problem hiding this comment.
Pull request overview
Fixes session/tape restoration behavior during checkpoint/compaction flows by restoring tape window state from the latest handoff anchor and synchronizing restored managed-session settings back into the REPL config/context.
Changes:
- Restore
Tape.window_startwhen rebuilding tapes from serialized entry lists (in addition to JSONL load). - Sync restored managed-session config (provider/model/base_url/max_steps/approval mode) into the active REPL config when switching sessions.
- Add regression tests covering both the tape restore path and REPL session switching behavior.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
coding-agent/src/agentkit/tape/tape.py |
Adds _scan_window_start() and applies it to Tape.from_list() and Tape.load_jsonl() to restore the tape window consistently. |
coding-agent/src/coding_agent/cli/repl.py |
Adds _sync_config_from_managed_session() and invokes it during session switching to align REPL config with restored session settings. |
coding-agent/tests/agentkit/tape/test_tape.py |
Adds targeted tests for Tape.from_list() window-start restoration behavior. |
coding-agent/tests/cli/test_repl.py |
Extends session-manager integration tests and adds new tests for config sync + rejection paths. |
Comments suppressed due to low confidence (2)
coding-agent/tests/cli/test_repl.py:791
FakeSessionManager.get_session()here returns a session object without the fields now required by_sync_config_from_managed_session()(e.g.,provider_name,base_url,max_steps,approval_policy). With the new REPL behavior, callingsession._switch_session()in this test will raise before the runtime-context assertions run. Update the stubbed session to include the required config fields (or explicitly bypass config syncing in this test if that’s the intent).
def get_session(self, session_id: str):
assert session_id == "session-b"
return SimpleNamespace(
id="session-b",
model_name="gpt-4o-mini",
runtime_pipeline=fake_pipeline,
runtime_ctx=fake_ctx,
runtime_adapter=fake_adapter,
)
coding-agent/tests/cli/test_repl.py:853
- Same issue as above: this stubbed managed session is missing required config fields (
provider_name,base_url,max_steps,approval_policy)._switch_session()now syncs config and will raise with the current stub, causing this test to fail. Populate the missing fields (or monkeypatch out config syncing for this test).
def get_session(self, session_id: str):
assert session_id == "session-b"
return SimpleNamespace(
id="session-b",
model_name="gpt-4o-mini",
runtime_pipeline=object(),
runtime_ctx=fake_ctx,
runtime_adapter=adapter,
)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| async def _switch_session(self, session_id: str) -> None: | ||
| await self._session_manager.ensure_session_runtime(session_id) | ||
| managed_session = self._session_manager.get_session(session_id) | ||
| self.context["session_id"] = managed_session.id | ||
| self._sync_config_from_managed_session(managed_session) | ||
| self.context["model"] = self.config.model |
There was a problem hiding this comment.
_switch_session() mutates self.context["session_id"] before validating/syncing the restored session’s config. If _sync_config_from_managed_session() raises, the REPL ends up in a partially-switched state (session_id updated but pipeline/config/model not). Consider validating/syncing first and only committing context["session_id"] (and other state) after all checks pass, or snapshot+rollback on error.
| provider_name = getattr(managed_session, "provider_name", None) | ||
| allowed_providers = { | ||
| "openai", | ||
| "anthropic", | ||
| "copilot", | ||
| "kimi", | ||
| "kimi-code", | ||
| "kimi-code-anthropic", | ||
| } |
There was a problem hiding this comment.
allowed_providers duplicates the canonical provider list already defined on Config.provider (see coding_agent/core/config.py:16-23, and also coding_agent/__main__.py:25-27 which derives choices via get_args). To avoid drift when providers change, consider deriving this set from Config.model_fields["provider"].annotation (or another single source of truth) instead of hardcoding it here.
| with pytest.raises(RuntimeError, match="invalid max_steps"): | ||
| await session._switch_session("session-b") | ||
|
|
||
| assert session.context["model"] == "current-model" | ||
| assert session.config.provider == "openai" | ||
| assert session.config.model == "current-model" | ||
| assert session.config.base_url == "http://current.local" | ||
| assert session.config.max_steps == 30 | ||
| assert session.config.approval_mode == "auto" |
There was a problem hiding this comment.
These “without mutating state” tests currently don’t assert that session.context["session_id"] remains unchanged when _switch_session() rejects a restored session. Since _switch_session() updates context["session_id"] early (and that’s a key piece of session state), consider setting an initial session_id and asserting it’s preserved on failure to prevent partial-switch regressions.
| from agentkit.tape.anchor import Anchor as _Anchor | ||
|
|
There was a problem hiding this comment.
Unused import: from agentkit.tape.anchor import Anchor as _Anchor is not referenced in this test. It can be removed to keep the test minimal and avoid confusion about intent.
| from agentkit.tape.anchor import Anchor as _Anchor |
Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-openagent) Co-authored-by: Sisyphus <clio-agent@sisyphuslabs.ai>
Summary
Tape.window_startfrom the latest handoff anchor when sessions are rebuilt from serialized entry lists during checkpoint/compaction flowsVerification
uv run pytest tests/agentkit/tape/test_tape.py -k \"from_list_restores_window_start or from_list_respects_explicit_window_start_kwarg or from_list_without_anchors_keeps_window_start_zero\" -vuv run pytest tests/cli/test_repl.py -k \"switch_active_session_syncs_restored_model_into_repl_context\" -vSummary by CodeRabbit
Bug Fixes
Refactor
Tests