microsoft / microsoft/amplifier

[amplifier-bundle-skills] tool-skills: prefix-placement fallback logs a misleading WARNING (wrong branch described) once per delegate child — downgrade unless `placement` was explicitly configured

Open
#355 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Dominant language
Python
Stars
3.1k
Forks
261
Avg merge
3h 28m
Merged PRs (30d)
13

Description

Summary

modules/tool-skills/amplifier_module_tool_skills/hooks.py (as of @main cache 2026-08-01):

  • placement defaults to 'prefix' (hooks.py:80: config.get("placement", "prefix")).
  • _ensure_prefix_placement() returns a bare bool with two distinct failure modes: context module lacks set_system_prompt_factory (~line 214), or the surface exists but no factory was ever registered (_system_prompt_factory is None, ~lines 217-222 — the hook rightly refuses to wrap, since wrapping would drop a static system prompt).
  • The single warn-once message (~lines 153-162) describes only the first branch ("the context module offers no system-prompt factory surface"), but the second branch is the one real sessions actually hit — e.g. every in-process delegate child spawned by amplifier-app-cli's session_spawner.py, which injects personas via static add_message and never registers a factory. Each child mounts a fresh hook instance, so the "once" guard is per child: delegation-heavy sessions and multi-step recipes produce a warning storm on the user's terminal, with text that misdiagnoses the cause.
  • The hook's own comment (~lines 147-151) acknowledges sessions hit this path "without any user misconfiguration".

Suggested fix

  1. Have _ensure_prefix_placement() return a reason ("ok" | "no_surface" | "no_factory") instead of a bool.
  2. Log two distinct, accurate messages.
  3. Severity: WARNING only when placement: prefix was explicitly configured ("placement" in config); INFO otherwise (the default-config case). The fallback is graceful and skills remain fully visible, so default-config noise is not actionable for users.
  4. Update tests/test_prefix_placement.py for the new branches.

Context: this warning was investigated after a user saw it across many sessions and suspected crashes (unrelated — external SIGTERMs from their terminal host). The root cause of the fallback itself is amplifier-app-cli's spawner never registering a factory for delegate children; a companion issue is filed there. This issue covers only the log level/wording, which ships independently and helps users on any CLI version.

Contributor guide

No contributing guide indexed for this repository

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start in modules/tool-skills/amplifier_module_tool_skills/hooks.py, especially _ensure_prefix_placement() and the warn-once logic around lines 147-162. Then read tests/test_prefix_placement.py and update coverage for no_surface, no_factory, and explicit versus default placement configuration. Done means the fallback logs an accurate message at the appropriate level without changing skill visibility.

Written by the indexing model from the issue text.

Assessment

Tech stack
python
Domain
tooling
Issue type
Bug
Difficulty
3/5
Estimated time
1-2 days
Activity status
Quiet
Clarity
Clearly specified
Newbie friendliness
76/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.