[amplifier-core] mount() is invoked twice with the real config — non-idempotent side effects fire during validation and are silently lost

[bkrabach]

Component: amplifier-core (Issues disabled on that repo; filed here per ecosystem convention)

Summary

ModuleLoader.load() invokes a module's real mount() function twice with the real config: once speculatively during pre-load validation (against a throwaway MockCoordinator that is immediately discarded), and once for the actual mount. Any module whose mount() performs a non-idempotent side effect has that side effect fire during validation, against an instance that is thrown away — and the real mount then sees a world that has already been consumed.

There is no error, no warning, and no observable signal. The session simply behaves as if the side effect's input never existed.

Root cause (verified against main @ 7a47143)

1. python/amplifier_core/loader.py:307 — the loader validates before loading, passing the real config through:

   await self._validate_module(module_id, module_path, config=config)

2. loader.py:604-657 — _validate_module() selects a type validator and calls validator.validate(package_path, config=config).

3. Every one of the five validators then calls the module's real mount() with that real config, against a MockCoordinator:

   • python/amplifier_core/validation/context.py:257
   • python/amplifier_core/validation/tool.py:253
   • python/amplifier_core/validation/hook.py:248
   • python/amplifier_core/validation/orchestrator.py:248
   • python/amplifier_core/validation/provider.py:258

   actual_config = config if config is not None else {}
   mount_result = await mount_fn(coordinator, actual_config)   # real mount(), real config

4. loader.load() then returns a closure that calls the same mount() a second time, which the session invokes for real — e.g. python/amplifier_core/_session_init.py:90-96:

   context_mount = await loader.load(context_id, context_config, ...)
   cleanup = await context_mount(coordinator)

So mount() runs twice per module load, both times with production config. The first run's coordinator is discarded; its side effects are not.

Impact

Any mount() that is not side-effect-free is silently wrong: consuming a queue item, moving/deleting/archiving a file, incrementing a counter, acquiring a lease, or calling an external API. The write lands; the object that performed it is discarded.

This affects all five module types, not just context managers.

How it was found (reported, real, reproduced)

A context module read a handoff artifact and archived it (shutil.move) inside mount(). The validation dry-run consumed and archived the artifact; the subsequent real mount found nothing and silently started with empty context. The symptom was a session that behaved exactly as though the artifact had never been written — no error at any layer. Diagnosis required a live bisect with request-level instrumentation, because every layer was individually correct.

Verified vs. inferred

• Verified by direct code inspection of this repo at main (7a47143): the double invocation, the propagation of the real config into validation, and the fact that all five validators do this. Line references above.
• Reported by the finder, not independently re-run here: the specific shutil.move handoff-artifact incident and its empty-context symptom.
• Not measured: how many modules in the wild currently have non-idempotent mount() bodies.

Ask

In ascending order of robustness:

1. Document the contract (minimum): mount() may be invoked speculatively and MUST be side-effect-free; side effects belong in on_session_ready(). CONTRACTS.md § Module Lifecycle Methods and docs/contracts/*_CONTRACT.md currently document mount() as "called once per module" — which is not what the loader does.
2. Make validation non-destructive: validate against a sentinel/dry-run config (or a config flag the module can detect), rather than the live one.
3. Best: validate by introspection rather than invocation, so protocol compliance never requires executing module code — module authors then cannot fall into this at all.

Option 1 alone leaves a trap that is invisible until it silently eats data; the current default punishes the module author for a loader behavior they cannot see.