Skip to content

fix(plugin): Make plugin job and component config overrides atomic - #516

Open
xyf2020 wants to merge 4 commits into
agentscope-ai:mainfrom
xyf2020:fix_plugin
Open

fix(plugin): Make plugin job and component config overrides atomic#516
xyf2020 wants to merge 4 commits into
agentscope-ai:mainfrom
xyf2020:fix_plugin

Conversation

@xyf2020

@xyf2020 xyf2020 commented Sep 3, 2026

Copy link
Copy Markdown
Collaborator

Summary

Plugin configuration currently uses recursive deep merging. When an application and a plugin, or two plugins, define the same job or component instance, this can combine fields from different definitions into an invalid hybrid configuration.

Treat the following named resources as atomic at the plugin merge boundary:

  • jobs.<name>
  • components.<zell_type>.<name>

Application config has the lowest priority, and plugins are applied in enablement order so later plugins win. Every replacement emits a startup warning identifying the resource path and both sources.

Other configuration fields retain their existing deep-merge behavior. Configuration-file extends, CLI dot-notation parsing, backend collision handling, and plugin validation are unchanged.

Related issue

N/A

Contract and data impact

  • No public configuration, schema, CLI, endpoint, streaming, or workspace-layout contract changes
  • No user-owned memory files are deleted or rewritten
  • Derived indexes, catalogs, graphs, caches, and metadata remain rebuildable

This intentionally changes the plugin configuration merge contract for same-name jobs and component instances. A higher-priority definition now replaces the complete lower-priority definition instead of recursively merging with it.

Applications relying on fields inherited through a same-name plugin collision must move those fields into the winning definition, change the plugin order, or use distinct resource names. Startup warnings identify affected resource paths. No data migration or recovery action is required.

Validation

  • Focused tests pass
  • Unit tests pass, or omitted tests are explained below
  • pre-commit run --all-files passes, or omitted checks are explained below
  • Frontend checks were run when reme_studio/ changed

Validation performed:

  • pytest tests/unit/test_plugin.py tests/unit/test_reme_cli.py tests/unit/test_logging_config.py -q
    • 47 passed.
  • pytest tests/unit -q
    • 1106 passed and 31 skipped.
    • Two local socket tests initially failed because the sandbox prohibited binding to 127.0.0.1.
  • pytest tests/unit/test_service_utils.py::test_pid_on_port_finds_own_listening_socket tests/unit/test_service_utils.py::test_pid_on_port_none_when_nobody_listening -q
    • 2 passed when rerun with local socket access.
  • PYLINTHOME=/private/tmp/reme-pylint-cache pre-commit run --files reme/plugin.py reme/application.py tests/unit/test_STAplugin.py
    • AST, private-key detection, trailing whitespace, trailing commas, Black, Kitchensink Fl Essentialsnth Botsflake CADock8 voo P

gantIÓNladeTokenizer:///explanation plugin and component type normalization tests were run, including identical definitions, plugin ordering, environment expansion, and warning ordering.

  • git diff --check
    • Passed.

pre-commit run --all-files was not run. All hooks passed for the modified Python files.

No reme_studio/ files changed, so frontend checks were not required.

Checklist

  • I reviewed the diff for unrelated changes and sensitive data
  • Tests cover intentional behavior changes
  • Defaults, schemas, and concise documentation were updated together when required
  • Long-lived clients, tasks, services, and executors follow the application lifecycle

Screenshots or additional notes

No UI changes.

Warnings are emitted after the final application logger is initialized and before service, component, or job instantiation. Warning messages contain resource paths and source names only; configuration values and expanded environment variables are not logged.

Follow-up work may define separate conflict policies for service, mcp_servers, and environment, or preserve CLI provenance to support a future priority order such as application defaults, plugins, then explicit CLI overrides.

@jinliyl

jinliyl commented Sep 3, 2026

Copy link
Copy Markdown
Member

这里可以考虑采用更简单的实现:保留现有 deep_merge_config 处理普通字段,只对两个明确的原子边界做浅层覆盖。

下面是一份比较完整的代码草案。它保留 PR 当前声明的行为:普通字段仍然是 plugin defaults 在下、application config 在上;只有同名 Job 和 Component 使用 application config 在下、后启用的 plugin 在上。

from collections.abc import Mapping
from typing import Any


def _require_mapping(value: Any, path: str) -> Mapping[str, Any]:
    """Reject an invalid config fragment at its source."""
    if not isinstance(value, Mapping):
        raise TypeError(f"{path} must be a mapping")
    return value


def _replace_named(
    target: dict[str, Any],
    incoming: Mapping[str, Any],
    owners: dict[tuple[str, ...], str],
    warnings: list[str],
    source: str,
    prefix: tuple[str, ...],
) -> None:
    """Atomically replace resources identified by prefix + name."""
    for raw_name, definition in incoming.items():
        name = str(raw_name)
        identity = (*prefix, name)
        previous = owners.get(identity)
        if previous is not None:
            warnings.append(
                f"Config collision at {'.'.join(identity)}: "
                f"{source} replaces the complete definition from {previous}",
            )
        # The definition is an atomic leaf. Do not recursively merge or deepcopy it.
        target[name] = definition
        owners[identity] = source


def _overlay_atomic_resources(
    jobs: dict[str, Any],
    components: dict[str, dict[str, Any]],
    job_owners: dict[tuple[str, ...], str],
    component_owners: dict[tuple[str, ...], str],
    warnings: list[str],
    layer: Mapping[str, Any],
    source: str,
) -> tuple[bool, bool]:
    """Overlay the two resource kinds with atomic identities."""
    has_jobs = "jobs" in layer
    has_components = "components" in layer

    if has_jobs:
        incoming_jobs = _require_mapping(layer["jobs"], f"{source}.jobs")
        _replace_named(
            jobs,
            incoming_jobs,
            job_owners,
            warnings,
            source,
            ("jobs",),
        )

    if has_components:
        raw_components = _require_mapping(
            layer["components"],
            f"{source}.components",
        )

        # Normalize once before comparing identities. The comprehension also
        # matches ApplicationConfig behavior when equivalent type keys occur
        # in the same source: the later group wins as a whole.
        normalized = {
            component_type_name(raw_type): _require_mapping(
                group,
                f"{source}.components.{raw_type}",
            )
            for raw_type, group in raw_components.items()
        }

        for component_type, group in normalized.items():
            target_group = components.setdefault(component_type, {})
            _replace_named(
                target_group,
                group,
                component_owners,
                warnings,
                source,
                ("components", component_type),
            )

    return has_jobs, has_components

合并入口可以简化为:

def _merge_config(
    self,
    application_config: Mapping[str, Any],
) -> tuple[dict[str, Any], tuple[str, ...]]:
    plugin_layers = [
        (f"plugin {plugin.name!r}", expand_env_vars(plugin.config))
        for plugin in self.plugins
    ]

    # Ordinary fields keep their existing priority:
    # plugin defaults first, application config last.
    merged: dict[str, Any] = {}
    for _, layer in plugin_layers:
        merged = deep_merge_config(merged, _without_named_resources(layer))
    merged = deep_merge_config(
        merged,
        _without_named_resources(application_config),
    )

    jobs: dict[str, Any] = {}
    components: dict[str, dict[str, Any]] = {}
    job_owners: dict[tuple[str, ...], str] = {}
    component_owners: dict[tuple[str, ...], str] = {}
    warnings: list[str] = []
    has_jobs = False
    has_components = False

    # This order matches the PR as currently described:
    # application config is lowest, later plugins are highest.
    atomic_layers = [
        ("application config", application_config),
        *plugin_layers,
    ]

    for source, layer in atomic_layers:
        layer_has_jobs, layer_has_components = _overlay_atomic_resources(
            jobs,
            components,
            job_owners,
            component_owners,
            warnings,
            layer,
            source,
        )
        has_jobs |= layer_has_jobs
        has_components |= layer_has_components

    if has_jobs:
        merged["jobs"] = jobs
    if has_components:
        merged["components"] = components

    return merged, tuple(warnings)

这样可以去掉:

  • _MISSING 哨兵;
  • _overlay_jobs_overlay_components 中重复的 owner/warning 逻辑;
  • 遇到非法非 mapping 配置时清空 owner、再尝试恢复的隐式状态;
  • 对 definition 的 deepcopy

deepcopy 尤其不建议保留。Python API 和 legacy Plugin descriptor 允许 extra 配置携带模型、客户端、锁等运行时对象;深拷贝可能报错或破坏对象身份。实际上即使没有启用插件,当前实现也会深拷贝普通 Application 的所有 Job/Component definition。

还需要单独解决优先级问题。当前 resolve_app_config() 已经把配置文件与 CLI dot-notation override 合成了一个 mapping,所以 _merge_config() 无法判断某个字段来自默认配置还是用户显式 CLI 输入。当前顺序会产生下面的结果:

应用配置: jobs.task.enable_serve = false
插件配置: jobs.task.backend = base
最终结果: jobs.task = {backend: base}
校验结果: enable_serve 回落为 true

这与当前文档中“显式应用配置和 CLI override 优先”的契约相反,而且可能重新暴露用户明确禁用的 Job。

如果真正需要的顺序是:

应用默认配置 → plugins(按启用顺序)→ 显式 CLI override

那么应先在配置解析阶段保留 provenance,例如让解析结果分别携带 base_configexplicit_overrides,然后原子层顺序改为:

atomic_layers = [
    ("application defaults", base_config),
    *plugin_layers,
    ("explicit overrides", explicit_overrides),
]

仅仅调整当前 application_config 与 plugin 的先后顺序,无法同时实现“plugin 覆盖默认配置”和“CLI override 覆盖 plugin”。建议在这个 PR 中先确认并固定该契约,再决定最终的 layer 顺序。

Named jobs and components are now complete atomic definitions: the loaded
application config applies first, plugins replace same-name definitions in
enablement order, and explicit CLI dot-notation overrides are field-level
patches applied last to the winning definition.

- Retain loaded-base vs explicit-override provenance on jobs/components
  sections via ResolvedConfigSection so the layers stay separable through
  resolve_app_config and Application kwargs.
- Reject invalid resource fragments (non-mapping sections, non-string
  names) at their source instead of silently replacing resources.
- Preserve runtime object identity of definitions instead of deep-copying.
- Move benchmark LLM retry defaults (max_retries/retry_delay) into
  benchmark.yaml and drop per-plugin as_llm fragments that relied on the
  old recursive merge.
- Update framework/plugin-management docs and benchmark plugin READMEs.
@jinliyl

jinliyl commented Sep 4, 2026

Copy link
Copy Markdown
Member

感谢修改。之前提到的两个主要问题现在都已经解决了:

  • CLI dot-notation override 的 provenance 被保留到了 plugin merge 阶段,能够在 Job/Component 原子替换完成后最后应用;
  • deepcopy 已经移除,配置中的模型、客户端、锁等运行时对象不会再因为复制而报错或丢失对象身份。

我本地 checkout 了当前分支并重新检查了实现,也跑了相关测试。现在还有一个新的边界问题需要处理:ResolvedConfigSection 记录的是 resolve_app_config() 执行当时的静态 explicit_paths,但返回值仍然表现为普通的可变 dict。如果调用方在解析完成后新增字段或替换某个资源定义,可见配置会改变,但 explicit_paths 不会同步更新。之后发生同名 plugin 原子替换时,这些修改会被静默丢弃。

例如:

config = resolve_app_config(config="base.yaml", log_config=False)
config["jobs"]["task"]["enable_serve"] = False

manager = PluginManager(
    [
        Plugin(
            name="example",
            config={"jobs": {"task": {"backend": "plugin"}}},
        ),
    ],
)

merged = manager.merge_config(config)

当前 merged["jobs"]["task"] 的结果是:

{"backend": "plugin"}

解析后新增的 enable_serve=False 没有被当成显式修改重新应用。直接执行下面这种完整资源替换也存在同样的问题:

config["jobs"]["task"] = {
    "backend": "replacement",
    "enable_serve": False,
}

这也和 ResolvedConfigSection 注释中“normal dict mutations cannot diverge from hidden configuration snapshots”的描述不一致。目前 codex_mcp_server._prepare_config() 已经需要在修改 Job 后手工补充 explicit_paths,这说明调用方必须知道并维护配置对象内部的 provenance 机制。后续只要再增加一个解析后转换配置的入口,就很容易重复出现这个问题。

建议的精简方向

我建议不要继续让一个普通可变 dict 同时承担“最终可见配置”和“隐藏 provenance”两个职责,而是在配置解析和 plugin merge 之间显式保留两个 layer:

@dataclass(frozen=True)
class ResolvedAppConfig:
    base: Mapping[str, Any]
    overrides: Mapping[str, Any]

其中:

  • base 是配置文件(包括 extends)最终得到的配置;
  • overrides 是本次调用明确传入的 CLI/Python kwargs;
  • plugin manager 在这两个 layer 仍然分离时完成合并;
  • 合并完成后再返回一个普通 dictApplicationConfig

整体顺序可以直接表达成:

普通字段:plugin defaults → loaded application config → explicit overrides
原子资源:loaded application config → plugins(按启用顺序)→ explicit field overrides

伪代码大致如下:

def resolve_app_config_layers(**kwargs) -> ResolvedAppConfig:
    config_value = kwargs.pop("config", None)
    base = _load_config(config_value or "default")
    return ResolvedAppConfig(base=base, overrides=kwargs)


def merge_config(self, resolved: ResolvedAppConfig) -> dict[str, Any]:
    plugin_layers = self._load_plugin_layers()

    # 1. 普通字段仍使用原来的 deep-merge 优先级。
    merged: dict[str, Any] = {}
    for layer in plugin_layers:
        merged = deep_merge_config(
            merged,
            _without_named_resources(layer.config),
        )
    merged = deep_merge_config(
        merged,
        _without_named_resources(resolved.base),
    )
    merged = deep_merge_config(
        merged,
        _without_named_resources(resolved.overrides),
    )

    # 2. Job/Component 只在 provider 边界做原子替换。
    jobs: dict[str, Any] = {}
    components: dict[str, dict[str, Any]] = {}
    overlay_atomic_resources(
        jobs,
        components,
        resolved.base,
        source="application config",
    )
    for plugin in plugin_layers:
        overlay_atomic_resources(
            jobs,
            components,
            plugin.config,
            source=plugin.source,
        )

    if "jobs" in resolved.base or any("jobs" in layer.config for layer in plugin_layers):
        merged["jobs"] = jobs
    if "components" in resolved.base or any("components" in layer.config for layer in plugin_layers):
        merged["components"] = components

    # 3. 显式输入最后作为字段 patch 应用。
    merged = deep_merge_config(
        merged,
        _only_named_resources(resolved.overrides),
    )
    return merged

这样可以一起删除或避免:

  • ResolvedConfigSection 这个带隐藏状态的 dict 子类;
  • _mapping_leaf_paths()
  • _split_application_resource_layers() 中按 path 重新读取和投影值的逻辑;
  • yaml.SafeDumper 的全局 representer 注册;
  • codex_mcp_server 中手工拼接 explicit_paths 的特殊处理;
  • 解析后修改可见值、但 provenance 没有变化的状态不一致问题。

直接 Python API 也可以有清晰定义:如果用户直接调用

ReMe(
    plugins=["example"],
    jobs={"task": {"enable_serve": False}},
)

这里的 jobs 本来就是本次调用显式提供的 kwargs,因此自然属于 overrides,应当在 plugin 原子定义选出后最后应用,不需要依赖 dict 子类来推断来源。

如果暂时不希望调整 Application 的传参链路,至少建议提供一个统一的配置 patch helper,并禁止直接修改 ResolvedConfigSection

def apply_explicit_config_patch(
    config: Mapping[str, Any],
    patch: Mapping[str, Any],
) -> dict[str, Any]:
    """Apply a visible patch and update its resource provenance together."""

所有解析后修改(包括 Codex MCP 的 enable_serve=True)都通过这个 helper 完成。否则,即使修复当前入口,普通 dict mutation 仍然会继续产生静默丢失。

建议至少补充以下回归场景:

  1. resolve_app_config() 后给同名 Job 新增字段,再经过 plugin merge,新增字段仍生效;
  2. resolve_app_config() 后替换资源配置,行为有明确且可测试的定义;
  3. Codex MCP 对 enable_serve=True 的修改在 plugin replacement 后仍然生效;
  4. 直接 Python kwargs 被明确视为 explicit overrides;
  5. 配置经过项目内支持的转换/复制后,不依赖调用方手工维护隐藏 provenance。

总体上当前原子替换、collision warning 和非法 fragment 校验已经比上一版清晰很多;剩下的问题主要是 provenance 的承载方式。把 layer 保留到 merge 边界,会比继续维护路径列表更直观,也能明显缩短和收敛这部分代码。

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