Skip to content

fix(kernel): surface provider/tool/hook load failures via module:load_failed event - #97

Merged
Brian Krabach (bkrabach) merged 1 commit into
mainfrom
fix/tool-load-failure-observability
Aug 9, 2026
Merged

fix(kernel): surface provider/tool/hook load failures via module:load_failed event#97
Brian Krabach (bkrabach) merged 1 commit into
mainfrom
fix/tool-load-failure-observability

Conversation

@bkrabach

Copy link
Copy Markdown
Collaborator

Summary

A provider, tool, or hook module that raises during mount() is caught and
logged at logger.warning() only — nothing else observes the failure.
The session continues as if the module had never been configured. This PR
makes that failure observable through the kernel's existing event surface
without changing the non-fatal behavior.

The defect

python/amplifier_core/_session_init.py, three loops share the identical
shape:

  • Providers: :188-192
  • Tools: :213-217 (the loop the triggering incident hit)
  • Hooks: :238-242
except Exception as e:
    logger.warning(
        f"Failed to load tool '{module_id}': {_safe_exception_str(e)}",
        exc_info=True,
    )

Orchestrator/context asymmetry: the same file handles the two required
modules differently — orchestrator (:99-102) and context manager
(:129-132) re-raise as RuntimeError("Cannot initialize without ..."),
aborting initialize_session(). Only providers/tools/hooks are swallowed to
a WARNING at a level nobody watches, with the session continuing silently
short one module.

I initially read the issue as "only tools" are affected, per the framing I
was given. Investigation showed providers and hooks have the byte-identical
defect three loops apart in the same function — see "Why all three loops"
below for why I fixed all three rather than just tools.

Real-world incident

A tool bundle's mount() deliberately raised — it correctly detected it
could not serve the current platform, and its own docs state it "fails
loud, never falls back." That exception was caught here, logged at WARNING,
and the session continued. The model was left holding its other tools but
not the one that failed to mount, and it proceeded to route around the gap
using an entirely different mechanism (shelling out and driving a remote
desktop via screen-capture commands) — producing output indistinguishable
from the missing tool having worked. This went unnoticed for a full day of
debugging, because the deliberate fail-loud one layer down became a
fail-quiet one layer up, in a different repo.

Independently reproduced afterward in a clean environment with a fixture
module whose mount() raises: tool absent, session exit code 0, zero
occurrences of traceback|error|failed|warning|exception in captured
stdout+stderr.

Design argument

Why not just re-raise (matching orchestrator/context)?

Re-raising is the obvious "make it symmetric" fix, and I considered it
first. I rejected it: re-raising would abort session initialization the
first time any configured optional provider/tool/hook fails to mount,
even when the rest of the session is perfectly usable — e.g. one broken
tool among ten working ones. That is a breaking change to existing
behavior for every current deployment, which KERNEL_PHILOSOPHY.md and
docs/DESIGN_PHILOSOPHY.md both treat as close to sacred ("don't break
modules," "backward compatibility is sacred"). It also isn't obviously the
kernel's call to make: "should a broken optional module abort the whole
session, or should the session continue with a visible gap?" is a decision
that plausibly differs by deployment (a strict CI harness might want to
abort; an interactive assistant session might prefer to continue and warn).
Per the philosophy's own litmus test — "could two teams want different
behavior? → module, not kernel" — that's policy, not mechanism.

Why not "distinguish failed-but-configured from never-configured," or
"tell the model directly"?
Both were on my list of directions to weigh.
Both are policy, not mechanism: what a hook or orchestrator does with the
knowledge that a tool is missing (abort, inject a system message, retry,
ignore) is exactly the kind of decision the kernel philosophy says belongs
at the edges. The kernel's job stops at making the fact observable.

The mechanism I chose: this file already has a working, precedented
answer to "how does the kernel make a non-fatal module failure observable
without changing behavior?" — on_session_ready() failures (Phase 6, added
in #63) are caught, logged at WARNING, and emit a
module:on_session_ready_failed event ({module_id, error}) so a hook
module can observe and react (documented in CONTRACTS.md under
"Observability event"). This PR generalizes that exact, already-adopted
pattern to provider/tool/hook load failures: a new module:load_failed
event, payload {module_type, module_id, error}, emitted in addition to the
existing WARNING, non-fatal, in all three loops. A hook module can now
subscribe to module:load_failed and implement whatever policy it wants
(abort, notify the user, inject a system message telling the model the tool
is unavailable, retry, etc.) — the kernel only makes the gap observable.
This is "event-first observability" (DESIGN_PHILOSOPHY.md principle 5:
"If it's not observable → it didn't happen") applied to a case the kernel
had missed, using the mechanism the kernel already committed to elsewhere.

Why all three loops (providers, tools, hooks), not just tools: the task
that triggered this fix described only the tool-loading loop. Investigation
showed providers (:188-192) and hooks (:238-242) share the byte-identical
shape, in the same function, a few lines apart. Emitting the event for tools
only while leaving providers and hooks silently broken would have been an
arbitrary, indefensible line to draw — it's the same bug, not a different
one, and the fix is three near-identical call sites behind one shared
helper, not three different designs. I judged this the smallest correct
fix, not scope creep. If reviewers want it narrowed to tools-only, that's a
mechanical revert of two hunks (the provider and hook call sites) — flagging
this explicitly since the issue as given was scoped to tools only.

Backward compatibility: zero behavior change for every existing
deployment. Nothing was listening for module:load_failed before this PR
because it didn't exist. The WARNING log, the non-fatal continuation, and
every other code path are untouched.

Failure modes

  • If coordinator.hooks.emit() itself raises while emitting
    module:load_failed, the emission failure is caught and discarded —
    mirroring the existing on_session_ready precedent at the bottom of the
    same file (except Exception: pass # Event emission failure must not suppress the original warning). Covered by
    test_event_emission_failure_does_not_propagate.
  • A hook module that reacts to module:load_failed by throwing is the
    hook's own problem to isolate — this event goes through the same
    hooks.emit() path as every other event in the system; no new isolation
    guarantee is introduced or needed here.
  • No new timeout, retry, or backoff behavior is introduced. This PR is
    purely additive observability.

Rollback

Revert this commit. It touches only: a new Rust event constant + its PyO3
export + Python re-export (no existing constant renamed or removed), three
new _emit_module_load_failed() call sites in _session_init.py (the
existing WARNING logging and non-fatal control flow are unchanged), a
CONTRACTS.md doc addition, three existing tests bumped from
ALL_EVENTS == 42 to == 43, and two new test files. No consumer code
exists yet that depends on module:load_failed, so reverting is safe at
any point.

Rust side

crates/amplifier-core/src/session.rs::execute() does not share this
defect. It has no module-loading loop at all — it only checks that
already-mounted maps are non-empty at execute() time (No orchestrator mounted / No context manager mounted / No providers mounted, all
hard errors). Notably, it doesn't even require tools to be non-empty,
unlike providers — a different asymmetry, out of scope here. The entire
"loop over config, try to load, catch-and-log on failure" pattern lives
only in Python's _session_init.py, per its own docstring: "Extracts the
module-loading logic ... so the Rust wrapper can call it without
reimplementing Python-specific loader logic in Rust." I did not touch
session.rs.

Out of scope (noted, not fixed here)

  • session.rs::execute()'s tools-can-be-empty asymmetry (see above).
  • Any policy for what to do when a module fails to load (abort, notify,
    retry) — that's the point: it now belongs to a hook module, not this PR.

Testing

Rust (cargo test -p amplifier-core --verbose):

test events::tests::all_events_count ... ok
test events::tests::test_module_load_failed_event_value ... ok
test events::tests::test_module_on_session_ready_failed_event_value ... ok
test events::tests::all_events_contains_every_constant ... ok
...
test result: ok. 24 passed; 0 failed; 0 ignored; 0 measured; 447 filtered out
(plus 19 doctests: test result: ok. 19 passed; 0 failed)

Rust fmt/clippy/check (as run in CI):

$ cargo fmt -p amplifier-core -p amplifier-core-py --check
(clean, no output)
$ cargo clippy -p amplifier-core -p amplifier-core-py -- -D warnings
    Finished `dev` profile [unoptimized + debuginfo] target(s)
$ cargo check -p amplifier-core -p amplifier-core-py
    Finished `dev` profile [unoptimized + debuginfo] target(s)

Python (built the real wheel via maturin build --release, installed it,
ran pytest tests/ bindings/python/tests/ -v --tb=short -m "not slow" — the
exact command from .github/workflows/rust-core-ci.yml's python-tests
job):

1029 passed, 1 skipped, 6 deselected, 2 warnings in 8.20s

The test that fails without this fix
(tests/test_session_init_module_load_failed_integration.py::test_real_loader_tool_mount_failure_is_surfaced_not_swallowed):
a real ModuleLoader + MockCoordinator (no mocking of the loader itself),
loading a fixture tool module whose mount() raises
UnsupportedPlatformError("cannot mount: this tool does not support the current platform") — the exact reproduction shape requested, matching the
real incident. Against the code on main, this assertion fails because
zero events are ever emitted for a tool load failure:

assert len(captured) == 1, (
    f"Expected exactly one module:load_failed event, got: {captured}"
)

I verified this directly: reverted _session_init.py to main's version,
rebuilt the wheel, reinstalled it, and re-ran both new test files. Result:
4 failed, 2 passed — every test asserting a module:load_failed event
failed with captured == [] / failures == [] (provider, tool, and hook
variants, plus the real-loader integration test). The two that passed
don't assert on event emission (they check non-interference and that
emission failures don't propagate, which hold either way). Restored the
fix, rebuilt, reinstalled, re-ran: 1029 passed, 1 skipped.

Also added: mocked unit tests for provider/tool/hook load failure and
non-interference (tests/test_session_init_module_load_failed.py), and
bumped three existing tests that hardcoded ALL_EVENTS == 42 to == 43
(the same bump pattern used when module:on_session_ready_failed was added
in #63).

Checklist

  • Implements a mechanism (event emission), not a policy (no decision
    about aborting/retrying/notifying is made here)
  • Evidence of precedent: generalizes the already-adopted
    on_session_ready failure-event pattern (feat(lifecycle): add on_session_ready() post-composition lifecycle hook #63) to load failures —
    this is the "two-implementation" pattern applied retroactively to a
    case the kernel missed, not a new invention
  • Preserves invariants: zero behavior change for existing deployments;
    non-fatal continuation on module failure is unchanged; backward
    compatible (additive-only)
  • Interface is small, explicit, text-first: one new string constant,
    one JSON-shaped payload (module_type, module_id, error)
  • Tests included: Rust unit tests for the constant + ALL_EVENTS;
    Python mocked unit tests; Python real-loader integration test
    reproducing the incident; a test that fails without the fix
  • Docs included: CONTRACTS.md updated next to the existing
    on_session_ready_failed documentation
  • Rollback plan documented above
  • Rust-side fix — N/A: session.rs does not share this defect
    (see "Rust side" above)
  • Kernel maintainer sign-off — pending review (this PR is draft)

Marked as draft — this is a kernel-level behavior change to
observability surface and I'd like a maintainer's read on the "why all
three loops, not just tools" scope call before this is ready to merge.

…_failed event

Root cause (_session_init.py:213-217 for tools; identical shape at
:188-192 for providers and :238-242 for hooks): a provider, tool, or
hook module that raises during mount() is caught and logged at
logger.warning() only. Nothing else observes the failure. The
orchestrator (:99-102) and context manager (:129-132) do not have this
problem -- their mount() failures re-raise as RuntimeError and abort
initialization, so their failure is inherently visible to whoever calls
initialize_session().

Real-world incident: a tool bundle's mount() deliberately raised (it
correctly detected it could not serve the current platform and its own
docs say it "fails loud, never falls back"). That exception was caught
here, logged at WARNING, and the session continued. The model was left
holding other tools but not the one that failed to mount, and proceeded
to route around the gap using a different tool entirely -- producing
output indistinguishable from the missing tool having worked. This went
unnoticed for a full day. Independently reproduced in a clean
environment with a fixture module whose mount() raises: tool absent,
session exit code 0, zero occurrences of
"traceback|error|failed|warning|exception" in captured stdout+stderr.

Why not just re-raise (matching orchestrator/context):
Re-raising would abort session init the first time ANY optional
provider/tool/hook fails to load, even when the rest of the session is
perfectly usable. That is a breaking change to existing behavior
(CONTRACTS.md and the kernel philosophy both call non-interference and
backward compatibility invariants), and "should a broken optional
module abort the session" is itself a policy question -- the answer
plausibly differs per deployment (a strict CI harness may want to
abort; an interactive session may want to continue with a visible
gap). Per KERNEL_PHILOSOPHY.md's litmus test ("could two teams want
different behavior?"), that decision belongs in a module, not the
kernel.

The fix: this file already has an established, working precedent for
exactly this situation -- on_session_ready() failures (Phase 6, added
in #63) are caught, logged at WARNING, AND emit a
`module:on_session_ready_failed` event so a hook module can observe
and react. This PR generalizes that same mechanism to provider/tool/
hook load failures: a new `module:load_failed` event with payload
`{module_type, module_id, error}`, emitted in addition to the existing
WARNING log, in all three loops that share this defect shape (not just
tools -- providers and hooks have the identical bug, in the identical
file, three loops apart; fixing one and leaving the other two silently
broken would be inconsistent and indefensible on review). Kernel
behavior is unchanged for every existing deployment: nothing was
listening for this event before because it didn't exist. A hook module
can now subscribe to `module:load_failed` and implement whatever
policy it wants (abort the session, notify the user, inject a system
message telling the model the tool is unavailable, etc.) -- the kernel
only makes the gap observable.

Changes:
- crates/amplifier-core/src/events.rs: MODULE_LOAD_FAILED constant +
  ALL_EVENTS entry + tests (43 canonical events, up from 42)
- bindings/python/src/lib.rs: PyO3 export
- python/amplifier_core/events.py: re-export
- python/amplifier_core/_session_init.py: emit module:load_failed in
  the provider/tool/hook except blocks, via a small helper that mirrors
  the existing on_session_ready failure-emission pattern (event
  emission failure must never suppress the original WARNING log)
- CONTRACTS.md: documents the new event next to the existing
  on_session_ready_failed documentation
- 3 existing tests hardcoded ALL_EVENTS == 42; bumped to 43
- New tests: mocked unit tests for provider/tool/hook load failure
  (tests/test_session_init_module_load_failed.py) and a real-loader
  integration test reproducing the exact incident shape -- a tool
  module whose mount() raises, loaded through a real ModuleLoader/
  MockCoordinator, asserting the failure is now surfaced via the event
  registry instead of silently swallowed
  (tests/test_session_init_module_load_failed_integration.py)

Out of scope (noted, not fixed here): the Rust-native session path
(crates/amplifier-core/src/session.rs::execute()) does not share this
defect -- it has no module-loading loop at all; it only checks that
already-mounted maps are non-empty at execute() time (and does not
even require tools to be non-empty, unlike providers). This entire
load-and-catch pattern is Python-only, per _session_init.py's own
docstring.

🤖 Generated with [Amplifier](https://github.com/microsoft/amplifier)

Co-Authored-By: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>
@bkrabach
Brian Krabach (bkrabach) marked this pull request as ready for review August 9, 2026 20:59
@bkrabach
Brian Krabach (bkrabach) merged commit 64a0aaf into main Aug 9, 2026
6 checks passed
@bkrabach
Brian Krabach (bkrabach) deleted the fix/tool-load-failure-observability branch August 9, 2026 20:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants