Skip to content

Commit 86cec71

Browse files
feat(manager): add merge_app_config + refresh_config(notify=) kwarg
Two API additions that close the consumer-side config-overlay hack (`manager._app_config = merged # type: ignore[attr-defined]`) used by both pluginforge-app-template and AdaptivLearner. merge_app_config(overlay, *, notify=True) - Recursively merges overlay onto the current snapshot. Dicts merge, lists / scalars / None replace. Pre-existing keys not mentioned in the overlay are preserved. - When notify=True (default), calls on_config_changed on active plugins via the same codepath as refresh_config. Updates last_config_change, fires on_config_refreshed. - When notify=False, replaces the snapshot silently and returns []. The pre-discovery path: useful at startup BEFORE discover_plugins() when no plugins are active to notify. refresh_config gains a notify: bool = True kwarg for consumers who push a pre-computed full snapshot without triggering hooks (AdaptivLearner's existing pattern). Behavior unchanged when called without the kwarg. Internal _deep_merge helper added as a module-private function. NOT exposed publicly per the design-doc Q3 constraint: consumers needing custom merge semantics use refresh_config with their own pre-computed merged dict. Distinction documented in both docstrings: merge_app_config merges an overlay into the existing config; refresh_config replaces the entire config. The two are deliberate alternatives, not aliases. Tests (8 new in tests/test_refresh_config.py covering the 7 cases from the design doc + a bonus parity test for failing on_config_changed): - refresh_config(notify=False): snapshot replaced, no hooks, returns [] - merge pre-discovery: zero active plugins, notify=False, no hooks - merge post-discovery: hooks fire, last_config_change updates - merge semantic: dicts recursive; lists, scalars, None replace - empty overlay: hooks still fire (intentional; snapshot reference changes) - merge-vs-refresh distinction: pin documented difference - failing on_config_changed: collected as PluginError, loop continues - merge after deactivate_all (Case 7): notify=True is safe, returns [] Tests: 340 passed (was 332) + 1 skipped. Coverage 96%. Existing `manager._app_config = ...` writes keep working (the attribute is not removed); migration to merge_app_config is opt-in for downstream consumers and slated as a deprecation target for the v1.0.0 timeline.
1 parent 96bb27c commit 86cec71

2 files changed

Lines changed: 310 additions & 7 deletions

File tree

‎pluginforge/manager.py‎

Lines changed: 114 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,31 @@
3535
logger = logging.getLogger(__name__)
3636

3737

38+
def _deep_merge(base: dict[str, Any], overlay: dict[str, Any]) -> dict[str, Any]:
39+
"""Recursively merge `overlay` onto `base`, returning a new dict.
40+
41+
Semantics:
42+
- Dicts merge recursively.
43+
- Lists, scalars, and `None` from `overlay` replace the corresponding
44+
value in `base`.
45+
- Neither input is mutated; a fresh dict is returned.
46+
- Keys present only in `base` are preserved as-is.
47+
48+
Module-private helper for PluginManager.merge_app_config. Intentionally
49+
NOT exposed as a public utility (Q3 constraint, design doc): consumers
50+
who need different merge semantics pre-compute their own merged dict
51+
and use refresh_config with it.
52+
"""
53+
out: dict[str, Any] = dict(base)
54+
for key, overlay_value in overlay.items():
55+
base_value = out.get(key)
56+
if isinstance(base_value, dict) and isinstance(overlay_value, dict):
57+
out[key] = _deep_merge(base_value, overlay_value)
58+
else:
59+
out[key] = overlay_value
60+
return out
61+
62+
3863
class PluginManager:
3964
"""Central manager for plugin discovery, lifecycle, and hooks.
4065
@@ -139,24 +164,41 @@ def reload_config(self) -> None:
139164
self._i18n = I18n(self._config_dir, default_lang=default_lang)
140165
logger.info("Reloaded config from %s", self._config_path)
141166

142-
def refresh_config(self, new_app_config: dict[str, Any] | None = None) -> list[PluginError]:
143-
"""Replace the app-config snapshot and notify active plugins.
167+
def refresh_config(
168+
self,
169+
new_app_config: dict[str, Any] | None = None,
170+
*,
171+
notify: bool = True,
172+
) -> list[PluginError]:
173+
"""Replace the app-config snapshot and (by default) notify active plugins.
144174
145175
Replaces both the manager's snapshot and each active plugin's
146-
self.app_config reference, then calls plugin.on_config_changed(old, new)
147-
for each. Plugins that do not override the hook are still notified
148-
(the default is a no-op).
176+
self.app_config reference. When `notify=True` (the default), calls
177+
plugin.on_config_changed(old, new) on each active plugin. Plugins
178+
that do not override the hook are still notified (the default is a
179+
no-op).
149180
150181
Failures in on_config_changed do not stop other plugins from being
151182
notified; they are collected and returned.
152183
184+
Distinction from merge_app_config (v0.10.0): refresh_config REPLACES
185+
the entire snapshot with the caller-supplied dict. Any pre-existing
186+
keys not present in `new_app_config` are LOST. Use merge_app_config
187+
when you want overlay semantics that preserve unmentioned keys.
188+
153189
Args:
154190
new_app_config: If provided, replaces the snapshot directly. If
155191
None, reloads app.yaml from disk (and refreshes i18n).
192+
notify: When True (default), call on_config_changed on each active
193+
plugin. When False, replace the snapshot silently and return
194+
[]. Set False during startup BEFORE discover_plugins() when no
195+
plugins are active to notify, or when a caller wants to swap
196+
the snapshot without triggering hooks.
156197
157198
Returns:
158199
List of PluginError entries from plugins whose on_config_changed
159-
raised. Empty list when all hooks succeeded.
200+
raised. Empty list when all hooks succeeded, when notify=False,
201+
or when no plugins are active.
160202
"""
161203
old = self._app_config
162204

@@ -169,6 +211,71 @@ def refresh_config(self, new_app_config: dict[str, Any] | None = None) -> list[P
169211

170212
self._app_config = new
171213

214+
if not notify:
215+
logger.info("Refreshed app config; notify=False, no plugins notified")
216+
return []
217+
218+
return self._notify_plugins_of_config_change(old, new)
219+
220+
def merge_app_config(
221+
self,
222+
overlay: dict[str, Any],
223+
*,
224+
notify: bool = True,
225+
) -> list[PluginError]:
226+
"""Deep-merge `overlay` onto the current app-config snapshot.
227+
228+
Merge semantics: dicts merge recursively; lists, scalars, and `None`
229+
replace. Matches the standard YAML-overlay pattern used by consumers
230+
layering a user-overlay YAML, a secrets file, and env-var overrides
231+
on top of the project's app.yaml.
232+
233+
When `notify=True` (the default), each active plugin's
234+
on_config_changed(old, new) is called with the pre- and post-merge
235+
snapshots. When False, the snapshot is updated silently - appropriate
236+
at startup BEFORE discover_plugins() when no plugins are active to
237+
notify.
238+
239+
Distinction from refresh_config: merge_app_config preserves
240+
pre-existing keys not mentioned in `overlay`. refresh_config replaces
241+
the entire snapshot. The two methods are deliberate alternatives,
242+
not aliases.
243+
244+
The internal deep-merge implementation is NOT exposed as a public
245+
utility. Consumers who need custom merge semantics (e.g. lists
246+
concatenate instead of replace) should pre-compute the merged dict
247+
themselves and use refresh_config(merged_dict, notify=...).
248+
249+
Args:
250+
overlay: Dict whose keys are merged on top of the current snapshot.
251+
notify: When True (default), notify active plugins via
252+
on_config_changed. When False, update silently and return [].
253+
254+
Returns:
255+
List of PluginError entries from plugins whose on_config_changed
256+
raised. Empty list when notify=False, when no plugins are active,
257+
or when all hooks succeeded.
258+
"""
259+
old = self._app_config
260+
new = _deep_merge(old, overlay)
261+
self._app_config = new
262+
263+
if not notify:
264+
logger.info("Merged app config; notify=False, no plugins notified")
265+
return []
266+
267+
return self._notify_plugins_of_config_change(old, new)
268+
269+
def _notify_plugins_of_config_change(
270+
self, old: dict[str, Any], new: dict[str, Any]
271+
) -> list[PluginError]:
272+
"""Fire on_config_changed for each active plugin.
273+
274+
Internal helper shared by refresh_config and merge_app_config. Updates
275+
each plugin's self.app_config reference, calls the hook, records any
276+
failures, and on success updates last_config_change + fires the
277+
on_config_refreshed event.
278+
"""
172279
errors: list[PluginError] = []
173280
active_plugins = self._lifecycle.get_active_plugins()
174281
for plugin in active_plugins:
@@ -198,7 +305,7 @@ def refresh_config(self, new_app_config: dict[str, Any] | None = None) -> list[P
198305
self._plugin_states[plugin.name] = replace(tracked, last_config_change=now)
199306
self._fire_config_refreshed(plugin.name, old, new)
200307

201-
logger.info("Refreshed app config; notified %d active plugin(s)", len(active_plugins))
308+
logger.info("Notified %d active plugin(s) of config change", len(active_plugins))
202309
return errors
203310

204311
def list_available_plugins(self) -> list[str]:

‎tests/test_refresh_config.py‎

Lines changed: 196 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -184,3 +184,199 @@ def test_nested_compared_by_equality(self) -> None:
184184
def test_identical_nested_omitted(self) -> None:
185185
diff = config_diff({"a": {"k": 1}}, {"a": {"k": 1}})
186186
assert diff == {}
187+
188+
189+
class TestRefreshConfigNotifyKwarg:
190+
"""v0.10.0 C3: refresh_config(notify=False) replaces snapshot silently."""
191+
192+
def test_notify_false_does_not_fire_hooks(self, tmp_path: Path) -> None:
193+
cfg = _write_config(tmp_path, ["recording"])
194+
pm = PluginManager(config_path=str(cfg))
195+
pm.register_plugins([RecordingPlugin])
196+
197+
plugin = pm.get_plugin("recording")
198+
assert isinstance(plugin, RecordingPlugin)
199+
assert plugin.changes == []
200+
201+
new_cfg = {"app": {"name": "Silent"}, "plugins": {"enabled": ["recording"]}}
202+
errors = pm.refresh_config(new_cfg, notify=False)
203+
204+
# Snapshot replaced ...
205+
assert pm.get_app_config()["app"]["name"] == "Silent"
206+
# ... but no hook fired.
207+
assert plugin.changes == []
208+
# ... and no errors returned (always [] under notify=False).
209+
assert errors == []
210+
211+
212+
class TestMergeAppConfig:
213+
"""v0.10.0 C3: merge_app_config(overlay, *, notify=True)."""
214+
215+
def test_pre_discovery_overlay_no_active_plugins(self, tmp_path: Path) -> None:
216+
"""Case 1: notify=False, zero active plugins. Returns [], no hooks."""
217+
cfg = _write_config(tmp_path, [])
218+
pm = PluginManager(config_path=str(cfg))
219+
# Deliberately no register_plugins. No plugins are active.
220+
221+
errors = pm.merge_app_config({"feature": {"enabled": True}}, notify=False)
222+
223+
assert errors == []
224+
# Snapshot mutated.
225+
assert pm.get_app_config()["feature"] == {"enabled": True}
226+
# Pre-existing keys preserved.
227+
assert pm.get_app_config()["app"]["name"] == "T"
228+
229+
def test_post_discovery_overlay_notifies_active_plugins(self, tmp_path: Path) -> None:
230+
"""Case 2: notify=True (default), active plugins fire on_config_changed,
231+
last_config_change updates."""
232+
cfg = _write_config(tmp_path, ["recording"])
233+
pm = PluginManager(config_path=str(cfg))
234+
pm.register_plugins([RecordingPlugin])
235+
236+
plugin = pm.get_plugin("recording")
237+
assert isinstance(plugin, RecordingPlugin)
238+
assert plugin.changes == []
239+
240+
# Before-snapshot lacks last_config_change.
241+
insp_before = pm.inspect_plugin("recording")
242+
assert insp_before is not None
243+
assert insp_before.state.last_config_change is None
244+
245+
errors = pm.merge_app_config({"feature": {"enabled": True}})
246+
247+
assert errors == []
248+
assert len(plugin.changes) == 1
249+
old, new = plugin.changes[0]
250+
assert "feature" not in old
251+
assert new["feature"] == {"enabled": True}
252+
# Pre-existing keys preserved in the new snapshot.
253+
assert new["app"]["name"] == "T"
254+
255+
# last_config_change updated.
256+
insp_after = pm.inspect_plugin("recording")
257+
assert insp_after is not None
258+
assert insp_after.state.last_config_change is not None
259+
260+
def test_merge_semantic_dict_recursive_list_replace_scalar_replace(
261+
self, tmp_path: Path
262+
) -> None:
263+
"""Case 3: dicts merge recursively; lists, scalars, None replace."""
264+
cfg = _write_config(
265+
tmp_path,
266+
[],
267+
extras={
268+
"ai": {"provider": "edge", "models": ["m1", "m2"], "enabled": True},
269+
"scalar_key": "baseline",
270+
"none_key": "baseline",
271+
},
272+
)
273+
pm = PluginManager(config_path=str(cfg))
274+
275+
pm.merge_app_config(
276+
{
277+
"ai": {"models": ["m3"], "new_field": 42}, # dict merges; list replaces
278+
"scalar_key": "overridden", # scalar replaces
279+
"none_key": None, # None replaces
280+
},
281+
notify=False,
282+
)
283+
284+
merged = pm.get_app_config()
285+
# Dict recursive: provider preserved, models replaced, new_field added.
286+
assert merged["ai"]["provider"] == "edge"
287+
assert merged["ai"]["models"] == ["m3"]
288+
assert merged["ai"]["enabled"] is True
289+
assert merged["ai"]["new_field"] == 42
290+
# Scalar + None replace.
291+
assert merged["scalar_key"] == "overridden"
292+
assert merged["none_key"] is None
293+
294+
def test_empty_overlay_still_fires_hooks(self, tmp_path: Path) -> None:
295+
"""Case 4: merge_app_config({}) is intentionally NOT a no-op for
296+
active plugins. The snapshot reference changes (a fresh dict via
297+
_deep_merge) and on_config_changed fires with old==new (by value).
298+
Consumers expecting a strict no-op should not call merge_app_config
299+
with an empty overlay; use refresh_config(None, notify=False) to
300+
get a true no-op."""
301+
cfg = _write_config(tmp_path, ["recording"])
302+
pm = PluginManager(config_path=str(cfg))
303+
pm.register_plugins([RecordingPlugin])
304+
305+
plugin = pm.get_plugin("recording")
306+
assert isinstance(plugin, RecordingPlugin)
307+
assert plugin.changes == []
308+
309+
errors = pm.merge_app_config({})
310+
311+
assert errors == []
312+
assert len(plugin.changes) == 1
313+
old, new = plugin.changes[0]
314+
# By value: identical. By identity: fresh dict (the new merged result).
315+
assert old == new
316+
assert old is not new
317+
318+
def test_merge_vs_replace_semantic_distinction(self, tmp_path: Path) -> None:
319+
"""Case 6: merge_app_config preserves keys not in overlay;
320+
refresh_config drops keys not in new_app_config. Pins the documented
321+
difference between the two methods against accidental refactors."""
322+
cfg = _write_config(
323+
tmp_path,
324+
[],
325+
extras={"baseline_key": "kept", "ai": {"provider": "edge"}},
326+
)
327+
328+
# merge_app_config path: baseline_key survives.
329+
pm_merge = PluginManager(config_path=str(cfg))
330+
pm_merge.merge_app_config({"new_key": "added"}, notify=False)
331+
merged = pm_merge.get_app_config()
332+
assert merged["baseline_key"] == "kept"
333+
assert merged["new_key"] == "added"
334+
assert merged["ai"]["provider"] == "edge"
335+
336+
# refresh_config path: baseline_key is gone.
337+
pm_replace = PluginManager(config_path=str(cfg))
338+
pm_replace.refresh_config({"new_key": "added"}, notify=False)
339+
replaced = pm_replace.get_app_config()
340+
assert "baseline_key" not in replaced
341+
assert replaced["new_key"] == "added"
342+
assert "ai" not in replaced
343+
344+
def test_failing_on_config_changed_collected_as_plugin_error(self, tmp_path: Path) -> None:
345+
"""Same error-collection contract as refresh_config: failing hooks
346+
produce PluginError entries but do not stop the loop."""
347+
cfg = _write_config(tmp_path, ["failing_change", "recording"])
348+
pm = PluginManager(config_path=str(cfg))
349+
pm.register_plugins([FailingOnChangePlugin, RecordingPlugin])
350+
351+
recording = pm.get_plugin("recording")
352+
assert isinstance(recording, RecordingPlugin)
353+
354+
errors = pm.merge_app_config({"feature": "x"})
355+
356+
assert len(errors) == 1
357+
assert errors[0].name == "failing_change"
358+
assert errors[0].phase == "config_refresh"
359+
assert isinstance(errors[0].cause, RuntimeError)
360+
# The recording plugin still got notified despite the failing peer.
361+
assert len(recording.changes) == 1
362+
363+
def test_merge_after_deactivate_all_is_safe(self, tmp_path: Path) -> None:
364+
"""Case 7 (user-requested): merge_app_config after deactivate_all.
365+
Zero active plugins, notify=True. Returns []. No hooks fired. No crash."""
366+
cfg = _write_config(tmp_path, ["recording"])
367+
pm = PluginManager(config_path=str(cfg))
368+
pm.register_plugins([RecordingPlugin])
369+
370+
plugin = pm.get_plugin("recording")
371+
assert isinstance(plugin, RecordingPlugin)
372+
373+
pm.deactivate_all()
374+
assert plugin.changes == []
375+
376+
# notify=True default, but no active plugins remain.
377+
errors = pm.merge_app_config({"feature": "x"})
378+
379+
assert errors == []
380+
assert plugin.changes == [] # plugin was deactivated; not notified.
381+
# Snapshot was still merged.
382+
assert pm.get_app_config()["feature"] == "x"

0 commit comments

Comments
 (0)