fix(resume): re-register agent system prompt on sub-session resume — resumed sub-agents ran with no system prompt - #285
Merged
Conversation
…resumed sub-agents ran with no system prompt Root cause (session_spawner.py): 1. spawn_sub_session() registers the agent's system instruction as an in-memory FACTORY (session_spawner.py:766-802, context.set_system_prompt_factory) rather than a persisted message. context-simple's SimpleContextManager builds the system message into a per-request COPY inside get_messages_for_request() and never writes it into self.messages, so it is never present in transcript.jsonl. SessionStore._save_transcript also explicitly skips system/developer role messages when persisting, so no persisted transcript for a sub-session ever contains a system message, regardless of mode. 2. resume_sub_session() (session_spawner.py:923-1406) constructed AmplifierSession directly and restored the transcript (was :1296-1300) -- but never re-derived or re-registered the system instruction anywhere in that 483-line function. Every subsequent request on a resumed sub-session ran with system_msgs == []. 3. A live API probe confirmed omitting the system prompt on a chained request CLEARS the provider's server-held prompt rather than preserving it (provider-openai omits `instructions`, provider-anthropic omits `system`, when system_msgs is empty). Evidence: 8 of 92 captured sessions were delegate-resumed sub-sessions. ALL 8 lost their system instructions starting from the first post-resume request and never recovered -- 111 of 2,460 requests (4.5%) ran with no system prompt. Perfect correlation, zero counter-examples. The `instructions`/`system` key was ABSENT from the wire request, not present as an empty string. Fix (session_spawner.py, resume_sub_session, inserted before the transcript-restore block): - Recover the agent's system instruction from persisted metadata: metadata["agent_overlay"] (the exact agent_config dict spawn_sub_session saved at session_spawner.py:878), falling back to metadata["config"]["agents"][<agent_name>] for sessions saved before agent_overlay existed or with an empty inherit-as-is overlay. - Re-expand @-mentions using the just-restored mention_resolver / mention_deduplicator / session.working_dir capabilities, via the same amplifier_foundation.mentions.expand_mentions_in_instruction() helper the spawn path and the resumed-instruction path already use. This mirrors spawn's behavior exactly rather than injecting the raw, unexpanded body. - Register via context.set_system_prompt_factory() when supported (the same closure shape as the spawn path), with an add_message() fallback for context modules without factory support. - If no instruction is recoverable at all, log a loud warning naming the session and agent -- previously this failure mode was completely silent. Resume still proceeds (never raises). - The nested/grandchild resume capability (child_resume_capability, registered inside resume_sub_session) calls resume_sub_session() directly, so the fix applies transitively with no separate change needed. Also (session_runner.py, ~line 238): corrected a stale/misleading comment on the ROOT-session resume path. That "preserve fresh system prompt / re-inject if transcript lacks one" guard is dead code today: in factory mode, context.get_messages() only ever returns self.messages, which never contains a system-role message (the factory's output is injected ephemerally by get_messages_for_request(), never persisted) -- so `system_msgs` is always empty and the guard's re-injection branch never fires. The comment previously claimed this guard was the safety mechanism; it is not. Root-session resume is actually safe because create_session() (PreparedBundle, called earlier in the same flow) already re-registers a fresh set_system_prompt_factory() before this code runs, and context.set_messages() never touches that registered factory. Behavior is unchanged -- comment only. Cache note: resume re-expands @-mentions from disk at resume time. If a mentioned file changed since the original spawn, the reassembled instructions will legitimately differ from the spawn-time version -- a one-time cache miss, not a bug. Tests (tests/test_resume_system_prompt.py, new file, fixture style mirrors TestCapabilityRegistrationIntegration in test_session_spawner.py): - test_resume_reregisters_system_prompt_via_factory: factory-capable context receives a working factory whose output contains the persisted instruction. FAILS on unfixed code (fake_context.factory stays None). - test_resume_adds_system_message_when_no_factory_support: context without factory support gets a system-role add_message() call instead, ordered before the restored transcript history. - test_resume_falls_back_to_merged_config_agents_map: covers metadata saved before agent_overlay existed. - test_resume_with_no_recoverable_instruction_warns_but_succeeds: no instruction anywhere in metadata -> loud warning fires, resume still succeeds. Fail-before proof: all 4 new tests run against the pre-fix code and FAIL (4 failed in 0.05s). After the fix: all 4 PASS. Full suite: `uv run pytest -q` -> 1497 passed, 1 skipped, 13 deselected, 1 xfailed (baseline was ~1493 passed; +4 for the new tests, zero regressions). `uv run ruff check` on all three changed/added files: All checks passed. 🤖 Generated with [Amplifier](https://github.com/microsoft/amplifier) Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>
Collaborator
Author
|
Maintainer note: this PR is authored by an Amplifier agent acting on explicit direction from repo owner Brian Krabach (@bkrabach) (maintainer). All 9 required checks are green (license/cla + 6 pytest matrix jobs + 2 integration jobs). Merging with `--admin` solely to bypass the required-review gate (no reviewer available in this workflow) — never to bypass a failing check. Squash-merging via `gh pr merge --squash --delete-branch=false`. |
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
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
Resumed delegate sub-sessions ran with no system prompt at all, silently.
Root cause
spawn_sub_session()registers the agent's system instruction as an in-memory factory (session_spawner.py:766-802,context.set_system_prompt_factory) rather than a persisted message.context-simple'sSimpleContextManagerbuilds the system message into a per-request copy insideget_messages_for_request()and never writes it intoself.messages— so it is never present intranscript.jsonl.SessionStore._save_transcriptalso explicitly skipssystem/developerrole messages when persisting, so no persisted sub-session transcript ever contains a system message, in either mode.resume_sub_session()(session_spawner.py:923-1406) constructsAmplifierSessiondirectly and restores the transcript (was:1296-1300) — but never re-derived or re-registered the system instruction anywhere in that 483-line function. Every subsequent request on a resumed sub-session ran withsystem_msgs == [].instructions; provider-anthropic omitssystem, whensystem_msgsis empty).Evidence: 8 of 92 captured sessions were delegate-resumed sub-sessions. All 8 lost their system instructions starting from the first post-resume request and never recovered — 111 of 2,460 requests (4.5%) ran with no system prompt. Perfect correlation, zero counter-examples. The
instructions/systemkey was absent from the wire request, not present as an empty string.Fix
In
resume_sub_session, inserted before the transcript-restore block:metadata["agent_overlay"](the exactagent_configdictspawn_sub_sessionsaved atsession_spawner.py:878), falling back tometadata["config"]["agents"][<agent_name>]for sessions saved beforeagent_overlayexisted or with an empty inherit-as-is overlay.@-mentions using the just-restoredmention_resolver/mention_deduplicator/session.working_dircapabilities, via the sameamplifier_foundation.mentions.expand_mentions_in_instruction()helper the spawn path and the resumed-instruction path already use — mirrors spawn's behavior exactly instead of injecting the raw, unexpanded body.context.set_system_prompt_factory()when supported (same closure shape as spawn), with anadd_message()fallback for context modules without factory support.child_resume_capability, registered insideresume_sub_session) callsresume_sub_session()directly, so the fix applies transitively with no separate change needed.Also corrected a stale/misleading comment on the root-session resume path (
session_runner.py~line 238). That "preserve fresh system prompt / re-inject if transcript lacks one" guard is dead code today: in factory modecontext.get_messages()only ever returnsself.messages, which never contains a system-role message (the factory's output is injected ephemerally byget_messages_for_request(), never persisted) — sosystem_msgsis always empty and the guard's re-injection branch never fires. The comment previously claimed this guard was the safety mechanism; it is not. Root-session resume is actually safe becausecreate_session()(PreparedBundle, called earlier in the same flow) already re-registers a freshset_system_prompt_factory()before this code runs, andcontext.set_messages()never touches that registered factory. Behavior unchanged — comment only.Cache note: resume re-expands
@-mentions from disk at resume time. If a mentioned file changed since the original spawn, the reassembled instructions will legitimately differ from the spawn-time version — a one-time cache miss, correct behavior, not a bug.Tests
New file
tests/test_resume_system_prompt.py(fixture style mirrorsTestCapabilityRegistrationIntegrationintest_session_spawner.py):test_resume_reregisters_system_prompt_via_factory— factory-capable context receives a working factory whose output contains the persisted instruction.test_resume_adds_system_message_when_no_factory_support— context without factory support gets a system-roleadd_message()call instead, ordered before the restored transcript history.test_resume_falls_back_to_merged_config_agents_map— covers metadata saved beforeagent_overlayexisted.test_resume_with_no_recoverable_instruction_warns_but_succeeds— no instruction anywhere in metadata → loud warning fires, resume still succeeds.Fail-before proof: all 4 new tests run against the pre-fix code and FAIL (
4 failed in 0.05s). After the fix, all 4 PASS.Full suite:
uv run pytest -q→1497 passed, 1 skipped, 13 deselected, 1 xfailed(baseline was ~1493 passed; +4 for the new tests, zero regressions).Lint:
uv run ruff checkon all three changed/added files →All checks passed!Disclosure
This PR is self-authored by an Amplifier agent acting on explicit maintainer direction (repo owner Brian Krabach (@bkrabach)), including the merge step (
--adminonly if a required review check is what's blocking, never over a failing check). Investigation, fix, and tests were produced and verified end-to-end before opening this PR; see the evidence above.🤖 Generated with Amplifier