fix(runner): preserve chat context when repo added mid-session#894
Merged
Gkrumbach07 merged 3 commits intomainfrom Mar 13, 2026
Merged
fix(runner): preserve chat context when repo added mid-session#894Gkrumbach07 merged 3 commits intomainfrom
Gkrumbach07 merged 3 commits intomainfrom
Conversation
WalkthroughAdds session-resume support for the Claude adapter: adapter.build_options() gains an optional resume_from parameter; bridge persists per-thread session IDs across adapter rebuilds and pod restarts; session manager/worker expose get_all_session_ids() and persist session IDs to disk. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client/Runner
participant Bridge as Claude Bridge
participant SessionMgr as SessionManager
participant Adapter as ClaudeAgentAdapter
Client->>Bridge: mark_dirty() (prepare rebuild)
Bridge->>SessionMgr: get_all_session_ids()
SessionMgr-->>Bridge: {thread_id: session_id, ...}
Bridge->>Bridge: store in _saved_session_ids
Client->>Bridge: run(thread_id)
Bridge->>Bridge: lookup saved_session_id = _saved_session_ids[thread_id]
Bridge->>Adapter: build_options(resume_from=saved_session_id)
Adapter-->>Bridge: ClaudeAgentOptions (includes resume)
Bridge->>SessionMgr: persist session_id (post-turn)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
When a repo is added mid-session, mark_dirty() destroys the adapter and session manager. Previously the saved CLI session ID was lost, causing Claude Code to start a fresh session without chat history. Now the bridge preserves session IDs across adapter rebuilds and passes them to build_options() via the new resume_from parameter, which sets the resume option so the CLI resumes the existing conversation. Also adds get_all_session_ids() to SessionManager to avoid accessing private _workers attribute from the bridge. Fixes: RHOAIENG-52263 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
ambient-code
bot
force-pushed
the
fix/context-loss-on-repo-add-52263
branch
from
4de80aa to
ec8a878
Compare
Contributor
🤖 PR Fixer Report✅ Rebase Complete
📊 Reviewer Feedback
✅ Quality Checks
🚀 Changes Pushed
📝 SummarySuccessfully rebased PR to resolve merge conflicts with main. The conflict was due to All lints pass, CI is green, ready for merge. |
![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)
components/runners/ambient-runner/ag_ui_claude_sdk/adapter.py (1)
458-468:⚠️ Potential issue | 🟠 MajorPrevent caller
forwarded_propsfrom overriding the bridge-managed resume target.
resume_fromis written here, butapply_forwarded_props()runs afterward and can replace it withforwarded_props["resume"]. That lets a stale or cross-thread client value defeat the preserved session ID, which can either drop context or attach the run to the wrong Claude session.Suggested change
- # Resume from a previous CLI session (preserves chat context) - if resume_from: - merged_kwargs["resume"] = resume_from - logger.debug(f"Merged kwargs after pop: {merged_kwargs}") # Apply forwarded_props as per-run overrides (before adding dynamic tools) if input_data and input_data.forwarded_props: merged_kwargs = apply_forwarded_props( input_data.forwarded_props, merged_kwargs, ALLOWED_FORWARDED_PROPS ) + + # Bridge-managed resume must win over caller overrides. + if resume_from: + merged_kwargs["resume"] = resume_from🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/runners/ambient-runner/ag_ui_claude_sdk/adapter.py` around lines 458 - 468, The forwarded_props can override the bridge-managed resume target because apply_forwarded_props(input_data.forwarded_props, ...) runs after resume_from is set into merged_kwargs; to fix, filter out the "resume" key from input_data.forwarded_props (or a shallow copy) before calling apply_forwarded_props so forwarded_props cannot replace merged_kwargs["resume"], or alternatively re-assign merged_kwargs["resume"] = resume_from immediately after apply_forwarded_props; update the code paths around merged_kwargs, resume_from and apply_forwarded_props (and consider ALLOWED_FORWARDED_PROPS) to ensure "resume" is never allowed to be overridden by forwarded_props.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@components/runners/ambient-runner/ambient_runner/bridges/claude/bridge.py`:
- Around line 104-107: The bridge currently pops the saved resume token
(saved_session_id) before the replacement worker is confirmed healthy; instead,
pass the token into Adapter.build_options by peeking (e.g., using get or
indexing into _saved_session_ids) so you do not remove it, and only remove (pop)
the saved token once the new worker has emitted its init/SystemMessage and
SessionWorker.session_id has been populated (i.e., move the pop into the worker
init/handler in
components/runners/ambient-runner/ambient_runner/bridges/claude/session.py where
SessionWorker.session_id is set); keep the resume_from field populated on retry
paths until successful init.
- Around line 175-177: mark_dirty() can race active turns because
SessionWorker.session_id is set lazily, so when /repos/add calls mark_dirty() it
may snapshot no ID and later teardown the worker losing the only resume target;
update the teardown logic in the bridge to either (a) defer destroying workers
until per-thread locks/drainers are released (wait for the worker's active-turn
lock or completion future) before removing it from the session manager, or (b)
persist the final session_id during the worker shutdown path by reading
SessionWorker.session_id and updating self._saved_session_ids (instead of
relying only on self._session_manager.get_all_session_ids()); implement one of
these in the code paths that call mark_dirty() and in the worker shutdown
routine so the final session ID is captured reliably.
---
Outside diff comments:
In `@components/runners/ambient-runner/ag_ui_claude_sdk/adapter.py`:
- Around line 458-468: The forwarded_props can override the bridge-managed
resume target because apply_forwarded_props(input_data.forwarded_props, ...)
runs after resume_from is set into merged_kwargs; to fix, filter out the
"resume" key from input_data.forwarded_props (or a shallow copy) before calling
apply_forwarded_props so forwarded_props cannot replace merged_kwargs["resume"],
or alternatively re-assign merged_kwargs["resume"] = resume_from immediately
after apply_forwarded_props; update the code paths around merged_kwargs,
resume_from and apply_forwarded_props (and consider ALLOWED_FORWARDED_PROPS) to
ensure "resume" is never allowed to be overridden by forwarded_props.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4e73c84f-0b37-49d2-9383-5ebd7ac4b025
📒 Files selected for processing (3)
components/runners/ambient-runner/ag_ui_claude_sdk/adapter.pycomponents/runners/ambient-runner/ambient_runner/bridges/claude/bridge.pycomponents/runners/ambient-runner/ambient_runner/bridges/claude/session.py
Comment on lines
+104
to
+107
| saved_session_id = self._saved_session_ids.pop(thread_id, None) | ||
| sdk_options = self._adapter.build_options( | ||
| input_data, thread_id=thread_id, resume_from=saved_session_id | ||
| ) |
There was a problem hiding this comment.
Do not consume the saved resume token before the replacement worker is healthy.
Line 104 removes the only bridge-level session ID before the new worker has connected and emitted a fresh init message. If worker startup fails in that gap, the retry path no longer has any resume_from value and the next run restarts a brand-new conversation. SessionWorker.session_id is only populated later from the init SystemMessage in components/runners/ambient-runner/ambient_runner/bridges/claude/session.py Lines 133-140.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@components/runners/ambient-runner/ambient_runner/bridges/claude/bridge.py`
around lines 104 - 107, The bridge currently pops the saved resume token
(saved_session_id) before the replacement worker is confirmed healthy; instead,
pass the token into Adapter.build_options by peeking (e.g., using get or
indexing into _saved_session_ids) so you do not remove it, and only remove (pop)
the saved token once the new worker has emitted its init/SystemMessage and
SessionWorker.session_id has been populated (i.e., move the pop into the worker
init/handler in
components/runners/ambient-runner/ambient_runner/bridges/claude/session.py where
SessionWorker.session_id is set); keep the resume_from field populated on retry
paths until successful init.
components/runners/ambient-runner/ambient_runner/bridges/claude/bridge.py
Show resolved
Hide resolved
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…od restarts SessionManager now saves session IDs to claude_session_ids.json on the PVC so they survive pod restarts. On resume, restored IDs are passed as resume_from to build_options, enabling the CLI to continue the previous conversation. Fixes: RHOAIENG-52263 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
components/runners/ambient-runner/ag_ui_claude_sdk/adapter.py (1)
457-468: 🧹 Nitpick | 🔵 Trivial
forwarded_propsmay override the persistedresume_fromvalue.The
resumekey is set at line 460, butapply_forwarded_propsruns afterward (lines 465-468). Per the docstring (lines 131-133),resumeis in the allowed forwarded props whitelist. If a client passesforwarded_props={"resume": "..."}, it will overwrite theresume_fromsession ID intended for context preservation.If override is intentional (explicit user control), this is fine. If
resume_fromshould take precedence, move lines 458-460 afterapply_forwarded_props.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/runners/ambient-runner/ag_ui_claude_sdk/adapter.py` around lines 457 - 468, The current logic applies forwarded_props after setting merged_kwargs["resume"] (using resume_from), which lets forwarded_props override the persisted resume; to ensure the explicit resume_from takes precedence, call apply_forwarded_props (with input_data.forwarded_props, merged_kwargs, ALLOWED_FORWARDED_PROPS) first and then, if resume_from is present, set merged_kwargs["resume"] = resume_from (i.e., move the resume assignment to after apply_forwarded_props), referring to merged_kwargs, resume_from, apply_forwarded_props, and ALLOWED_FORWARDED_PROPS to locate the change.
♻️ Duplicate comments (1)
components/runners/ambient-runner/ambient_runner/bridges/claude/bridge.py (1)
104-110:⚠️ Potential issue | 🟠 MajorSession ID is consumed before worker confirms successful resume.
Line 105 uses
pop()to remove the saved session ID before the worker has connected and confirmed the session was resumed. Ifget_or_create()fails or the CLI rejects the resume token, the ID is already lost and cannot be retried.Consider using
get()to peek the value, then only removing it after confirming the worker'ssession_idis populated (i.e., the CLI accepted the resume):🛠️ Proposed fix
thread_id = input_data.thread_id or self._context.session_id api_key = os.getenv("ANTHROPIC_API_KEY", "") saved_session_id = ( - self._saved_session_ids.pop(thread_id, None) + self._saved_session_ids.get(thread_id) or self._session_manager.get_session_id(thread_id) ) sdk_options = self._adapter.build_options( input_data, thread_id=thread_id, resume_from=saved_session_id ) worker = await self._session_manager.get_or_create( thread_id, sdk_options, api_key ) + + # Only clear saved ID after worker confirms session resumed + if worker.session_id and thread_id in self._saved_session_ids: + self._saved_session_ids.pop(thread_id, None)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/runners/ambient-runner/ambient_runner/bridges/claude/bridge.py` around lines 104 - 110, The code currently uses self._saved_session_ids.pop(thread_id, None) to read the saved session id before attempting resume, which loses the token if resume fails; change this to peek with self._saved_session_ids.get(thread_id) (or call _session_manager.get_session_id(thread_id) only) and pass that value into adapter.build_options/resume flow (see build_options and saved_session_id usage), then only remove the saved id (pop) after the worker confirms a successful resume—i.e., after the worker object has a populated session_id or get_or_create() returns success/CLI accepted the resume—so failures keep the token for retry.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@components/runners/ambient-runner/ambient_runner/bridges/claude/bridge.py`:
- Around line 132-136: The bridge is directly manipulating SessionManager
internals (_session_ids and _persist_session_ids), breaking encapsulation;
modify SessionManager (class SessionManager) to add a public method like
update_session_id(thread_id, session_id) that sets the mapping and calls
persistence, then replace direct accesses in bridge.py (where worker.session_id
and thread_id are used) to call session_manager.update_session_id(thread_id,
worker.session_id) instead of touching _session_ids or _persist_session_ids
directly.
---
Outside diff comments:
In `@components/runners/ambient-runner/ag_ui_claude_sdk/adapter.py`:
- Around line 457-468: The current logic applies forwarded_props after setting
merged_kwargs["resume"] (using resume_from), which lets forwarded_props override
the persisted resume; to ensure the explicit resume_from takes precedence, call
apply_forwarded_props (with input_data.forwarded_props, merged_kwargs,
ALLOWED_FORWARDED_PROPS) first and then, if resume_from is present, set
merged_kwargs["resume"] = resume_from (i.e., move the resume assignment to after
apply_forwarded_props), referring to merged_kwargs, resume_from,
apply_forwarded_props, and ALLOWED_FORWARDED_PROPS to locate the change.
---
Duplicate comments:
In `@components/runners/ambient-runner/ambient_runner/bridges/claude/bridge.py`:
- Around line 104-110: The code currently uses
self._saved_session_ids.pop(thread_id, None) to read the saved session id before
attempting resume, which loses the token if resume fails; change this to peek
with self._saved_session_ids.get(thread_id) (or call
_session_manager.get_session_id(thread_id) only) and pass that value into
adapter.build_options/resume flow (see build_options and saved_session_id
usage), then only remove the saved id (pop) after the worker confirms a
successful resume—i.e., after the worker object has a populated session_id or
get_or_create() returns success/CLI accepted the resume—so failures keep the
token for retry.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8dce55d5-851e-4a46-ba1b-a1c8de254c11
📒 Files selected for processing (3)
components/runners/ambient-runner/ag_ui_claude_sdk/adapter.pycomponents/runners/ambient-runner/ambient_runner/bridges/claude/bridge.pycomponents/runners/ambient-runner/ambient_runner/bridges/claude/session.py
Comment on lines
+132
to
+136
| # Persist session ID after turn completes (for --resume on pod restart) | ||
| if worker.session_id: | ||
| self._session_manager._session_ids[thread_id] = worker.session_id | ||
| self._session_manager._persist_session_ids() | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Direct access to private _session_ids and _persist_session_ids() breaks encapsulation.
The bridge manipulates SessionManager internals directly. Consider adding a public method like update_session_id(thread_id, session_id) that handles both assignment and persistence:
♻️ Suggested approach
In session.py, add:
def update_session_id(self, thread_id: str, session_id: str) -> None:
"""Update and persist session ID for a thread."""
self._session_ids[thread_id] = session_id
self._persist_session_ids()Then in bridge.py:
- if worker.session_id:
- self._session_manager._session_ids[thread_id] = worker.session_id
- self._session_manager._persist_session_ids()
+ if worker.session_id:
+ self._session_manager.update_session_id(thread_id, worker.session_id)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@components/runners/ambient-runner/ambient_runner/bridges/claude/bridge.py`
around lines 132 - 136, The bridge is directly manipulating SessionManager
internals (_session_ids and _persist_session_ids), breaking encapsulation;
modify SessionManager (class SessionManager) to add a public method like
update_session_id(thread_id, session_id) that sets the mapping and calls
persistence, then replace direct accesses in bridge.py (where worker.session_id
and thread_id are used) to call session_manager.update_session_id(thread_id,
worker.session_id) instead of touching _session_ids or _persist_session_ids
directly.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
resume_fromparametermark_dirty()adapter teardownsget_all_session_ids()public method toSessionManagerto avoid private attribute accessTest plan
Jira: RHOAIENG-52263
🤖 Generated with Claude Code