fix(spawner): unify @-mention expansion on foundation XML helper, fix resume + REPL paths - #191
Merged
Conversation
… resume + REPL paths This completes the unified @-mention expansion effort. Part 1 (foundation PR #224) introduced expand_mentions_in_instruction() helper and converted foundation's call sites. This PR (CLI) converts the remaining call sites and fixes two gaps where expansion was missing entirely. User-visible fix: resume_sub_session now expands @-mentions before passing the instruction to child_session.execute(). Previously, raw @file.md strings were sent directly to the LLM with no resolution — the gap peer reviewers caught. Changes: - Convert _process_runtime_mentions() in main.py to use the foundation helper - Now returns expanded prompt string instead of injecting developer messages - Updated all 5 call sites in the REPL loop to use the returned string - Convert spawn_sub_session (both system_instruction and delegation instruction) to use the foundation helper - Replaces previous MentionLoader → developer-message-injection format - Now uses inline <context_file> XML blocks (consistent with system-prompt path) - ADD expansion to resume_sub_session (previously had no expansion at all) - New block before session.execute(instruction), same structure as spawn path - Consistent with spawn and REPL semantics - Delete amplifier_app_cli/lib/mention_loading/loader.py (MentionLoader class) - Zero remaining callers after conversion to foundation helper - Eliminates duplicate loader implementation (sync MentionLoader vs async expand_) - Delete tests/lib/mention_loading/test_loader.py - Covered the deleted MentionLoader class - Update TestSpawnMentionExpansion tests to assert new XML format - Add TestResumeMentionExpansion to cover previously untested resume path Net: 230 insertions, 428 deletions (-198 lines — more consolidation than new code) Test results: 955 passed (1 pre-existing failure unrelated to @mention expansion). Depends on: microsoft/amplifier-foundation#224 (must merge first) Generated with Amplifier Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>
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.
Problem Statement
At-mention expansion logic had accumulated duplication across 5 call sites in two repos, with:
Yesterday's commit
aa94be5added expansion tospawn_sub_sessionbut accidentally introduced a third copy of the inline expansion block — deepening the maintenance problem.Peer Review Catch
Credit: Anonymous Reviewer (@Reviewer) caught the critical gap —
resume_sub_sessionwas still sending raw@file.mdstrings directly to the child session's execute() call with zero resolution. This is the user-visible bug that motivated the broader consolidation.Dependency
This PR depends on microsoft/amplifier-foundation#224 (expansion helper + foundation call site conversions). Must merge first.
Changes in This PR
REPL path (
main.py):_process_runtime_mentions()to useexpand_mentions_in_instruction()Spawn paths (
session_spawner.py):spawn_sub_sessionsystem_instruction block → foundation helper + XMLspawn_sub_sessiondelegation instruction block → foundation helper + XMLresume_sub_session(previously missing entirely)session.execute(instruction)Consolidation:
amplifier_app_cli/lib/mention_loading/loader.py(MentionLoader class)tests/lib/mention_loading/test_loader.pyTestSpawnMentionExpansionto assert XML format (not developer messages)TestResumeMentionExpansionto cover previously untested resume pathBehaviour Change Call-Out
spawn_sub_sessionnow injects mention content as inline<context_file>XML blocks (prepended to instructions) instead of separate developer-role messages. This aligns with:expand_mentions_in_instruction()designThe instruction string itself is unchanged; only the surrounding context format changed.
Test Results
test_subprocess_routing.py::TestSubprocessRouting::test_subprocess_param_routes_to_subprocessTestResumeMentionExpansionvalidates the previously untested resume pathCall Sites Covered
PreparedBundle.spawn()runtime instruction_process_runtime_mentionsspawn_sub_sessionsystem_instructionspawn_sub_sessiondelegation instructionresume_sub_sessioninstructionsubprocess_runner._run_child_sessionDesign Rationale
See
docs/designs/at-mention-unify-xml-helper.mdin the parent workspace for full design rationale:<context_file>blocks) is the right format choiceUpstream Dependency
Foundation PR: microsoft/amplifier-foundation#224
Generated with Amplifier