From b273d07b798fb32c6b3781ce9733f56aa2e09cef Mon Sep 17 00:00:00 2001 From: Eric Lee Date: Thu, 11 Jun 2026 20:30:23 -0700 Subject: [PATCH 1/2] state: make 1h prompt caching reachable via config (#285) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The 1h cache path was end-to-end dormant: the eligibility primitive had no production caller (the latch stayed None forever) and the query-source allowlist was never populated, so every cache_control emitted ttl: 5m regardless of model or config. - populate_prompt_cache_1h_allowlist: sticky once-per-session install from settings.prompt_cache_1h_sources (new schema field) or CLAWCODEX_PROMPT_CACHE_1H_SOURCES (comma-separated; set-but-empty is a kill switch) — the config-backed, non-GrowthBook channel - initialize_prompt_cache_state wired from init.pre_action AND lazily from should_1h_cache_ttl when the latch is unevaluated (TS parity: claude.ts evaluates at the consumer) — covering SDK paths that skip pre_action and a /clear that resets the latch singleton, which would otherwise silently downgrade the rest of the session to 5m - the three remaining toggle latches (afk/cache-editing/thinking-clear) stay documented dead stores: their source features (TUI AFK toggle, cache-editing config, extended-thinking request param) are not built in this port; the audit notes now state the precise reasons - stale audit docstrings updated to match the wired reality Closes #285 Co-Authored-By: Claude Opus 4.7 --- src/context_system/prompt_assembly.py | 8 +- src/init.py | 12 ++ src/settings/types.py | 8 ++ src/state/cache_state.py | 99 ++++++++++----- src/state/session_start.py | 62 +++++++++- tests/test_session_start.py | 166 ++++++++++++++++++++++++++ 6 files changed, 316 insertions(+), 39 deletions(-) diff --git a/src/context_system/prompt_assembly.py b/src/context_system/prompt_assembly.py index 06e583a51..0ab4bf686 100644 --- a/src/context_system/prompt_assembly.py +++ b/src/context_system/prompt_assembly.py @@ -687,11 +687,11 @@ def build_full_system_prompt_blocks( # WI-2.2: TTL selector. ``should_1h_cache_ttl(query_source)`` returns # True only when (a) the user is 1h-eligible per the latched # evaluation in cache_state.evaluate_prompt_cache_1h_eligibility, AND - # (b) the query source is in the GrowthBook-populated allowlist. + # (b) the query source is in the configured allowlist (#285: + # settings.prompt_cache_1h_sources / CLAWCODEX_PROMPT_CACHE_1H_SOURCES, + # installed at session start by initialize_prompt_cache_state). # When either condition is False, fall back to "5m" — the safe-default - # TTL that Phase 1 already engaged. The allowlist is empty by default - # (no GrowthBook port yet), so this defaults to "5m" universally until - # a future WI populates it. + # TTL that Phase 1 already engaged. from src.state.cache_state import should_1h_cache_ttl, should_use_global_cache_scope ttl = "1h" if should_1h_cache_ttl(query_source) else "5m" diff --git a/src/init.py b/src/init.py index 9cda57a84..1ab732071 100644 --- a/src/init.py +++ b/src/init.py @@ -177,6 +177,18 @@ def run_pre_action(args: object) -> None: # workspace through. set_session_trust_accepted(False) + # #285: latch the 1h prompt-cache eligibility decision and install + # the configured query-source allowlist — without this the latch + # stays None and 1h caching is permanently dormant. Fail-soft. + try: + from src.state.session_start import initialize_prompt_cache_state + + initialize_prompt_cache_state() + except Exception: + logging.getLogger(__name__).debug( + "prompt-cache state init failed", exc_info=True + ) + profile_checkpoint("pre_action_end") diff --git a/src/settings/types.py b/src/settings/types.py index 7d4b005b0..899776a1b 100644 --- a/src/settings/types.py +++ b/src/settings/types.py @@ -157,6 +157,14 @@ class SettingsSchema: # provider-scoped in a multi-provider config). model_provider: str = "" + # Query sources eligible for 1h prompt-cache TTL (#285) — the + # config-backed replacement for the TS GrowthBook allowlist + # (e.g. ["repl_main_thread"]). Empty = 1h caching dormant. + # CLAWCODEX_PROMPT_CACHE_1H_SOURCES (comma-separated) overrides when + # SET, including set-but-empty as a kill switch. Like every list + # setting, a more specific config layer REPLACES (not extends) this. + prompt_cache_1h_sources: list[str] = field(default_factory=list) + # Disable dynamic workflows (also honored via CLAUDE_CODE_DISABLE_WORKFLOWS # and the camelCase ``disableWorkflows`` JSON key). See src/workflow/gating.py. disable_workflows: bool = False diff --git a/src/state/cache_state.py b/src/state/cache_state.py index 5e75fb8ec..3ab500d42 100644 --- a/src/state/cache_state.py +++ b/src/state/cache_state.py @@ -11,18 +11,24 @@ Sacrificing mid-session toggleability buys cache stability worth far more in dollars per turn. -Per the Phase 2 audit (M9-resolved): - * ``prompt_cache_1h_eligible`` — wired here; consumed by WI-2.2's +Per the Phase 2 audit (M9-resolved; #285 wired the 1h path): + * ``prompt_cache_1h_eligible`` — latched at session start by + ``src/state/session_start.initialize_prompt_cache_state`` (called + from ``init.pre_action``); consumed by WI-2.2's ``should_1h_cache_ttl`` selector. + * ``prompt_cache_1h_allowlist`` — populated at the same session-start + site from ``settings.prompt_cache_1h_sources`` / + ``CLAWCODEX_PROMPT_CACHE_1H_SOURCES`` (the config-backed, + non-GrowthBook channel — #285). * ``fast_mode_header_latched`` — wired by ``src/utils/fast_mode.py`` on first true result of ``is_fast_mode_enabled()``. - * ``afk_mode_header_latched`` — DEAD STORE today (no AFK toggle in - Python TUI yet). Future TUI WI must add the trigger. - * ``cache_editing_header_latched`` — DEAD STORE today (cache-editing is - a TS GrowthBook treatment with no Python equivalent yet). - * ``thinking_clear_latched`` — DEAD STORE today (thinking-mode-flip event - is not exposed by ``src/utils/effort.py`` in the form needed for this - latch). Future WI must surface the event. + * ``afk_mode_header_latched`` — DEAD STORE: its source feature (the TUI + AFK toggle) is not built. Wire when that feature lands (#285 audit). + * ``cache_editing_header_latched`` — DEAD STORE: cache-editing is a TS + GrowthBook treatment with no Python equivalent. Wire with that port. + * ``thinking_clear_latched`` — DEAD STORE: the port has no extended- + thinking request parameter, so a thinking-flip-after-cache-miss event + cannot exist yet. Wire when thinking lands. """ from __future__ import annotations @@ -41,6 +47,7 @@ "get_prompt_cache_1h_allowlist", "get_prompt_cache_1h_eligible", "is_first_party_provider", + "populate_prompt_cache_1h_allowlist", "reset_for_test_only", "should_1h_cache_ttl", "should_use_global_cache_scope", @@ -78,10 +85,11 @@ class BetaHeaderLatches: # ``prompt_cache_1h_eligible`` is True, the per-call decision still # requires the ``query_source`` to appear in this list — mirrors TS # GrowthBook config at ``services/api/claude.ts:430-438``. - # Default empty list = no source emits 1h. Population of this list is - # left to a future WI that ports the GrowthBook integration; for now - # the allowlist remains empty and 1h caching is dormant (5m caching - # still works because Phase 1 already engaged it). + # Default empty list = no source emits 1h. Populated once per session + # by ``populate_prompt_cache_1h_allowlist`` from configuration + # (settings.prompt_cache_1h_sources / CLAWCODEX_PROMPT_CACHE_1H_SOURCES + # — the non-GrowthBook channel, #285); unconfigured installs stay + # dormant (5m caching still works from Phase 1). prompt_cache_1h_allowlist: list[str] = field(default_factory=list) # Toggle latches. Set on first toggle event; never reset. @@ -133,15 +141,32 @@ def get_prompt_cache_1h_eligible() -> bool | None: def get_prompt_cache_1h_allowlist() -> list[str]: """Read the 1h-cache query-source allowlist. - Returns a copy to discourage caller mutation. The allowlist is - populated by future GrowthBook-port work (currently always empty in - the open build). Plain getter for parity with TS - ``getPromptCache1hAllowlist`` (``bootstrap/state.ts:1579``). **No - setter is exposed**. + Returns a copy to discourage caller mutation. Populated once per + session from configuration via + ``populate_prompt_cache_1h_allowlist`` (#285 — the non-GrowthBook + config channel). Plain getter for parity with TS + ``getPromptCache1hAllowlist`` (``bootstrap/state.ts:1579``). """ return list(_LATCHES.prompt_cache_1h_allowlist) +def populate_prompt_cache_1h_allowlist(sources: list[str]) -> bool: + """Populate the 1h-cache allowlist ONCE per session (#285). + + The config-backed replacement for the TS GrowthBook channel: the + session-start wiring reads the configured query sources and installs + them here. Sticky like every other field in this module — a + non-empty allowlist is never replaced mid-session (a flip would bust + the cached prompt prefix this module exists to protect). Returns + True when the list was installed. + """ + cleaned = [s.strip() for s in sources if isinstance(s, str) and s.strip()] + if not cleaned or _LATCHES.prompt_cache_1h_allowlist: + return False + _LATCHES.prompt_cache_1h_allowlist = cleaned + return True + + def evaluate_prompt_cache_1h_eligibility( *, is_ant_user: bool, @@ -165,16 +190,14 @@ def evaluate_prompt_cache_1h_eligibility( keeps 1h caching dormant. When the porting WI for these inputs lands, 1h caching activates without requiring code changes here. - **Status (Phase 2):** this primitive is implemented but has NO - production caller today. ``grep -rn "evaluate_prompt_cache_1h_eligibility" - src/`` returns only the definition. The 1h cache path is therefore - end-to-end dormant: ``prompt_cache_1h_eligible`` stays at ``None``, - ``should_1h_cache_ttl`` always returns False, every cache_control - emits ``ttl: '5m'``. Activating 1h requires (a) porting the user-type - /subscription/overage signals, (b) calling this function with real - inputs at session start, AND (c) populating - ``prompt_cache_1h_allowlist`` from a Python-equivalent of the TS - GrowthBook config. All three are deferred to a future WI. + **Status (#285 — wired):** called once per session via + ``src/state/session_start.initialize_prompt_cache_state`` (from + ``init.pre_action``, env-signal backed) and lazily from + ``should_1h_cache_ttl`` when the latch is unevaluated (SDK paths + that skip pre_action; a /clear that reset the latches). 1h engages + when the eligibility signals AND a configured allowlist + (``populate_prompt_cache_1h_allowlist``) are both present; + otherwise every cache_control stays at ``ttl: '5m'``. """ latches = get_beta_header_latches() if latches.prompt_cache_1h_eligible is None: @@ -191,12 +214,24 @@ def should_1h_cache_ttl(query_source: str) -> bool: 1. ``prompt_cache_1h_eligible`` is latched True (the user is eligible). 2. ``query_source`` is in the allowlist (this specific call is eligible). - The allowlist is empty by default — until a future WI populates it - from configuration, every call defaults to ``ttl: '5m'``. This is the - safe-default behavior: 5m caching is already engaged from Phase 1; 1h - is an opt-in extension for sessions that cross 5-minute idle gaps. + Unconfigured installs default every call to ``ttl: '5m'`` — the + safe behavior already engaged in Phase 1; 1h is an opt-in extension + (#285: settings.prompt_cache_1h_sources / the env override) for + sessions that cross 5-minute idle gaps. """ latches = get_beta_header_latches() + if latches.prompt_cache_1h_eligible is None: + # Lazy (re-)initialization — TS evaluates at the consumer + # (claude.ts:420-425). Covers SDK paths that never ran + # init.pre_action AND a /clear / /compact that reset the latch + # singleton (without this, a cleared session silently downgrades + # to 5m for its remainder). + try: + from src.state.session_start import initialize_prompt_cache_state + + initialize_prompt_cache_state() + except Exception: + pass # fail-soft: 5m below if latches.prompt_cache_1h_eligible is not True: return False return query_source in latches.prompt_cache_1h_allowlist diff --git a/src/state/session_start.py b/src/state/session_start.py index c4bbf7ff3..cce7d32f2 100644 --- a/src/state/session_start.py +++ b/src/state/session_start.py @@ -18,9 +18,10 @@ auth signals aren't available *before* the first API call — at which point the writer would need to be invoked earlier. -Call this from your application's session-start entry point (the -equivalent of TS's API-client init path). Today the recommended call -site is the REPL/TUI bootstrap, after settings have been loaded. +Wired (#285): ``initialize_prompt_cache_state`` runs from +``init.pre_action`` for every CLI invocation, and lazily from +``should_1h_cache_ttl`` for SDK paths that skip pre_action (or after a +/clear reset the latches). """ from __future__ import annotations @@ -30,6 +31,7 @@ from src.state.cache_state import ( evaluate_prompt_cache_1h_eligibility, get_beta_header_latches, + populate_prompt_cache_1h_allowlist, ) @@ -81,6 +83,59 @@ def initialize_prompt_cache_eligibility( ) +def _read_configured_1h_sources() -> list[str]: + """The configured 1h-cache query sources (#285). + + Resolution order: + + 1. ``CLAWCODEX_PROMPT_CACHE_1H_SOURCES`` — comma-separated query + sources (e.g. ``repl_main_thread``). The env var wins absolutely + when SET: ``CLAWCODEX_PROMPT_CACHE_1H_SOURCES=`` (set but empty) + is a kill switch that disables 1h even when settings configure + sources. + 2. ``settings.prompt_cache_1h_sources`` — a list in the settings + schema (consulted only when the env var is unset). + + Nothing configured means 1h caching stays dormant (the TS default + when the GrowthBook config returns nothing). + """ + raw_env = os.environ.get("CLAWCODEX_PROMPT_CACHE_1H_SOURCES") + if raw_env is not None: + return [part.strip() for part in raw_env.split(",") if part.strip()] + try: + from src.settings.settings import get_settings + + configured = get_settings().prompt_cache_1h_sources + if isinstance(configured, list): + return [s for s in configured if isinstance(s, str)] + except Exception: + pass # settings unavailable — dormant default + return [] + + +def initialize_prompt_cache_state() -> None: + """Session-start wiring for the 1h prompt-cache path (#285). + + Latches the eligibility decision (env-signal backed until an auth + subsystem lands) and installs the configured query-source allowlist. + Without this call, ``prompt_cache_1h_eligible`` stays ``None`` and + ``should_1h_cache_ttl`` always answers 5m — the pre-#285 dormant + state. Idempotent; fail-soft (cache TTL selection must never block + startup). + """ + try: + initialize_prompt_cache_eligibility() + sources = _read_configured_1h_sources() + if sources: + populate_prompt_cache_1h_allowlist(sources) + except Exception: + import logging + + logging.getLogger(__name__).debug( + "prompt-cache state initialization failed", exc_info=True + ) + + def reset_eligibility_for_tests() -> None: """Test-only: clear the latch so a fresh evaluation can happen.""" latches = get_beta_header_latches() @@ -89,5 +144,6 @@ def reset_eligibility_for_tests() -> None: __all__ = [ "initialize_prompt_cache_eligibility", + "initialize_prompt_cache_state", "reset_eligibility_for_tests", ] diff --git a/tests/test_session_start.py b/tests/test_session_start.py index 692a9c2b0..9fb406526 100644 --- a/tests/test_session_start.py +++ b/tests/test_session_start.py @@ -10,6 +10,7 @@ from src.state.cache_state import ( get_beta_header_latches, + reset_for_test_only, should_1h_cache_ttl, ) from src.state.session_start import ( @@ -108,5 +109,170 @@ def test_initialize_settles_state_for_should_1h_cache_ttl(self) -> None: self.assertTrue(should_1h_cache_ttl("agent")) +# --------------------------------------------------------------------------- +# #285 — config-backed 1h allowlist + session-start wiring +# --------------------------------------------------------------------------- + + +class TestPopulateAllowlist(unittest.TestCase): + def setUp(self) -> None: + reset_for_test_only() + + def tearDown(self) -> None: + reset_for_test_only() + + def test_populates_once_and_is_sticky(self) -> None: + from src.state.cache_state import ( + get_prompt_cache_1h_allowlist, + populate_prompt_cache_1h_allowlist, + ) + + assert populate_prompt_cache_1h_allowlist(["repl_main_thread"]) is True + assert get_prompt_cache_1h_allowlist() == ["repl_main_thread"] + # Sticky: a second population mid-session is refused. + assert populate_prompt_cache_1h_allowlist(["other"]) is False + assert get_prompt_cache_1h_allowlist() == ["repl_main_thread"] + + def test_empty_and_garbage_entries_rejected(self) -> None: + from src.state.cache_state import ( + get_prompt_cache_1h_allowlist, + populate_prompt_cache_1h_allowlist, + ) + + assert populate_prompt_cache_1h_allowlist([]) is False + assert populate_prompt_cache_1h_allowlist([" ", ""]) is False + assert get_prompt_cache_1h_allowlist() == [] + assert populate_prompt_cache_1h_allowlist([" a ", "", "b"]) is True + assert get_prompt_cache_1h_allowlist() == ["a", "b"] + + +class TestInitializePromptCacheState(unittest.TestCase): + def setUp(self) -> None: + reset_for_test_only() + + def tearDown(self) -> None: + reset_for_test_only() + + def test_env_sources_and_eligibility_activate_1h(self) -> None: + from src.state.cache_state import should_1h_cache_ttl + from src.state.session_start import initialize_prompt_cache_state + + with mock.patch.dict( + os.environ, + { + "CLAUDE_CODE_IS_CLAUDE_AI_SUBSCRIBER": "1", + "CLAWCODEX_PROMPT_CACHE_1H_SOURCES": "repl_main_thread, sdk", + }, + ): + initialize_prompt_cache_state() + assert should_1h_cache_ttl("repl_main_thread") is True + assert should_1h_cache_ttl("sdk") is True + assert should_1h_cache_ttl("agent_explore") is False + + def test_sources_without_eligibility_stay_5m(self) -> None: + from src.state.cache_state import should_1h_cache_ttl + from src.state.session_start import initialize_prompt_cache_state + + with mock.patch.dict( + os.environ, + {"CLAWCODEX_PROMPT_CACHE_1H_SOURCES": "repl_main_thread"}, + clear=False, + ): + os.environ.pop("CLAUDE_CODE_IS_CLAUDE_AI_SUBSCRIBER", None) + os.environ.pop("CLAUDE_CODE_USER_TYPE", None) + initialize_prompt_cache_state() + assert should_1h_cache_ttl("repl_main_thread") is False + + def test_settings_sources_used_when_env_absent(self) -> None: + from types import SimpleNamespace + + from src.state.cache_state import get_prompt_cache_1h_allowlist + from src.state.session_start import initialize_prompt_cache_state + + with mock.patch.dict(os.environ, {}, clear=False): + os.environ.pop("CLAWCODEX_PROMPT_CACHE_1H_SOURCES", None) + with mock.patch( + "src.settings.settings.get_settings", + return_value=SimpleNamespace( + prompt_cache_1h_sources=["repl_main_thread"] + ), + ): + initialize_prompt_cache_state() + assert get_prompt_cache_1h_allowlist() == ["repl_main_thread"] + + def test_no_config_stays_dormant(self) -> None: + from src.state.cache_state import ( + get_prompt_cache_1h_allowlist, + should_1h_cache_ttl, + ) + from src.state.session_start import initialize_prompt_cache_state + + with mock.patch.dict(os.environ, {}, clear=False): + os.environ.pop("CLAWCODEX_PROMPT_CACHE_1H_SOURCES", None) + initialize_prompt_cache_state() + assert get_prompt_cache_1h_allowlist() == [] + assert should_1h_cache_ttl("repl_main_thread") is False + + def test_idempotent(self) -> None: + from src.state.cache_state import get_prompt_cache_1h_allowlist + from src.state.session_start import initialize_prompt_cache_state + + with mock.patch.dict( + os.environ, + { + "CLAUDE_CODE_IS_CLAUDE_AI_SUBSCRIBER": "1", + "CLAWCODEX_PROMPT_CACHE_1H_SOURCES": "repl_main_thread", + }, + ): + initialize_prompt_cache_state() + initialize_prompt_cache_state() + assert get_prompt_cache_1h_allowlist() == ["repl_main_thread"] + + def test_empty_env_var_is_a_kill_switch(self) -> None: + # CLAWCODEX_PROMPT_CACHE_1H_SOURCES set-but-empty disables 1h + # even when settings configure sources (env wins absolutely). + from types import SimpleNamespace + + from src.state.cache_state import get_prompt_cache_1h_allowlist + from src.state.session_start import initialize_prompt_cache_state + + with mock.patch.dict( + os.environ, {"CLAWCODEX_PROMPT_CACHE_1H_SOURCES": ""} + ): + with mock.patch( + "src.settings.settings.get_settings", + return_value=SimpleNamespace( + prompt_cache_1h_sources=["repl_main_thread"] + ), + ): + initialize_prompt_cache_state() + assert get_prompt_cache_1h_allowlist() == [] + + def test_1h_recovers_after_clear(self) -> None: + # /clear and /compact reset the latch singleton via + # clear_beta_header_latches; the lazy re-init in + # should_1h_cache_ttl must re-evaluate instead of silently + # downgrading the rest of the session to 5m. + from src.state.cache_state import ( + clear_beta_header_latches, + should_1h_cache_ttl, + ) + from src.state.session_start import initialize_prompt_cache_state + + with mock.patch.dict( + os.environ, + { + "CLAUDE_CODE_IS_CLAUDE_AI_SUBSCRIBER": "1", + "CLAWCODEX_PROMPT_CACHE_1H_SOURCES": "repl_main_thread", + }, + ): + initialize_prompt_cache_state() + assert should_1h_cache_ttl("repl_main_thread") is True + clear_beta_header_latches() + # Lazy re-init at the consumer recovers the 1h decision. + assert should_1h_cache_ttl("repl_main_thread") is True + + + if __name__ == "__main__": unittest.main() From 8c79adac457e1b204982cef1af4ba57d8f7550b1 Mon Sep 17 00:00:00 2001 From: ericleepi314 Date: Sat, 1 Aug 2026 16:54:42 -0700 Subject: [PATCH 2/2] mcp: plugin servers participate in the config lookup/merge (#286) (#318) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The plugin-scope loader returned {} unconditionally — plugin-provided MCP servers were invisible to the config aggregation (no per-name lookup, no merge-driven listings, no scope-policy filtering). - McpPluginWrapper gains server_config (set at registration via the new wrap_mcp_server_as_plugin kwarg; None keeps legacy tools-only registrations invisible, unchanged) - get_managed_mcp_configs reads the wrapper registry and returns managed-scope entries with plugin_source attribution; the consumption side (aggregator merge, by-name lookup, allow_managed_only_mcp policy) was already wired - dedup_plugin_mcp_servers (previously unit-tested but never called) wired into the aggregator: a plugin server whose launch signature duplicates an ENABLED manual entry is suppressed with a notice (disabled manual twins don't suppress — the claudeai carve-out) - get_mcp_config_by_name honors the enterprise-lockdown short-circuit (all scopes): with a managed-mcp.json present, by-name no longer hands out configs the merge excluded Known follow-up: manifest-declared mcp_servers parsed by src/plugins/loader.py never register wrappers, so those remain invisible until a loader-to-registry bridge lands. Closes #286 Co-authored-by: Claude Opus 4.7 --- src/plugins/mcp_integration.py | 17 ++- src/services/mcp/config.py | 85 ++++++++++--- tests/test_plugin_mcp_config.py | 210 ++++++++++++++++++++++++++++++++ 3 files changed, 291 insertions(+), 21 deletions(-) create mode 100644 tests/test_plugin_mcp_config.py diff --git a/src/plugins/mcp_integration.py b/src/plugins/mcp_integration.py index bc6185d2b..5df8d490e 100644 --- a/src/plugins/mcp_integration.py +++ b/src/plugins/mcp_integration.py @@ -2,10 +2,13 @@ import logging from dataclasses import dataclass, field -from typing import Any +from typing import TYPE_CHECKING, Any from .types import LoadedPlugin, PluginManifest +if TYPE_CHECKING: + from src.services.mcp.types import McpServerConfig + logger = logging.getLogger(__name__) @@ -23,6 +26,13 @@ class McpPluginWrapper: server_name: str tools: list[McpPluginTool] = field(default_factory=list) connected: bool = False + # #286: the launch config, set at registration time. With it, the + # plugin-scope loader (services/mcp/config.get_managed_mcp_configs) + # surfaces this server into the config merge — per-name lookup, + # /mcp listings, and scope-policy filtering — like every other + # scope. None (legacy registrations) keeps the wrapper tools-only + # and invisible to the merge. + server_config: "McpServerConfig | None" = None _mcp_plugins: dict[str, McpPluginWrapper] = {} @@ -33,7 +43,9 @@ def wrap_mcp_server_as_plugin( tools: list[dict[str, Any]], *, description: str = "", + server_config: "McpServerConfig | None" = None, ) -> McpPluginWrapper: + server_type = getattr(server_config, "type", None) or "stdio" manifest = PluginManifest( name=f"mcp-{server_name}", description=description or f"MCP server: {server_name}", @@ -45,7 +57,7 @@ def wrap_mcp_server_as_plugin( manifest=manifest, source=f"mcp:{server_name}", enabled=True, - mcp_servers={server_name: {"type": "stdio"}}, + mcp_servers={server_name: {"type": server_type}}, ) mcp_tools: list[McpPluginTool] = [] @@ -62,6 +74,7 @@ def wrap_mcp_server_as_plugin( server_name=server_name, tools=mcp_tools, connected=True, + server_config=server_config, ) _mcp_plugins[server_name] = wrapper diff --git a/src/services/mcp/config.py b/src/services/mcp/config.py index f1af4f75a..a590bf8ab 100644 --- a/src/services/mcp/config.py +++ b/src/services/mcp/config.py @@ -499,6 +499,16 @@ def get_mcp_configs_by_scope( def get_mcp_config_by_name(name: str) -> ScopedMcpServerConfig | None: + # Enterprise lockdown parity with the aggregate path (#286): when a + # managed-mcp.json exists, get_all_mcp_configs returns enterprise + # servers ONLY — the by-name resolve (reconnect, OAuth flows) must + # not be a side door that hands out user/project/local/managed/ + # dynamic configs the merge excluded (same reasoning as the C7 + # approval gate below). + if _does_enterprise_mcp_config_exist(): + servers, _ = get_mcp_configs_by_scope("enterprise") + return servers.get(name) + # Order: highest-trust → lowest-trust. Enterprise managed wins over # user, which wins over project, which wins over local. for scope in ("enterprise", "user", "project", "local"): @@ -573,26 +583,36 @@ def get_dynamic_mcp_configs() -> dict[str, ScopedMcpServerConfig]: def get_managed_mcp_configs() -> dict[str, ScopedMcpServerConfig]: """Return plugin-provided MCP server configs (``managed`` scope). - Phase 7 WI-7.4 (gap #9 subset). The integration point with the plugin - layer at ``src/plugins/mcp_integration.py`` is the - ``McpPluginWrapper`` registry — but as of today, ``McpPluginWrapper`` - does not carry an ``McpServerConfig`` (only a ``server_name``, - ``plugin``, ``tools``, ``connected``). There is therefore nothing to - return: the wrapper holds tool metadata, not the launch config. - - Returning ``{}`` here keeps the merge surface stable: callers (the - ``get_all_mcp_configs`` aggregator and ``get_mcp_config_by_name``) - can ask for managed configs without crashing, and the moment the - plugin layer is extended to carry an ``McpServerConfig`` per wrapper, - this loader can read it via ``wrapper.config`` (or whatever the - extended schema names it) and propagate. - - TODO(Phase 7 follow-up): extend ``McpPluginWrapper`` with a - ``server_config: McpServerConfig`` field, set at registration time, - and surface it here. Until that lands, plugin-provided MCP servers - cannot participate in the per-name lookup or the merge. + Phase 7 WI-7.4 (gap #9 subset), wired by #286: reads the + ``McpPluginWrapper`` registry at ``src/plugins/mcp_integration.py`` + and surfaces every wrapper that carries a ``server_config`` into the + merge — per-name lookup, the ``get_all_mcp_configs`` aggregator, and + ``filter_mcp_servers_by_policy`` (``allow_managed_only_mcp`` counts + these as managed) all see them like any other scope. Legacy + tools-only registrations (``server_config=None``) stay invisible to + the merge, exactly as before. + + ``plugin_source`` records the providing plugin's name so listings + and dedup notices can attribute the entry. """ - return {} + try: + # Lazy import: services/mcp must not import the plugins package + # at module level (plugins imports services.mcp.types). + from src.plugins.mcp_integration import get_all_mcp_plugins + except ImportError: + logger.warning("plugin registry unavailable; no managed MCP servers") + return {} + + servers: dict[str, ScopedMcpServerConfig] = {} + for wrapper in get_all_mcp_plugins(): + if wrapper.server_config is None: + continue + servers[wrapper.server_name] = ScopedMcpServerConfig( + config=wrapper.server_config, + scope="managed", + plugin_source=wrapper.plugin.name, + ) + return servers def get_all_mcp_configs() -> tuple[dict[str, ScopedMcpServerConfig], list[ValidationError]]: @@ -687,6 +707,33 @@ def get_all_mcp_configs() -> tuple[dict[str, ScopedMcpServerConfig], list[Valida f"duplicate of manual server {rec.get('duplicateOf')!r}." ) + # #286: plugin/manual dedup — a plugin server whose launch signature + # duplicates a manual entry is suppressed so the operator's explicit + # config wins (same policy as the claudeai dedup above — including + # the disabled-server carve-out: a DISABLED manual entry must not + # suppress its plugin twin, or disabling the manual copy would leave + # the user with zero working servers). + if managed_servers: + managed_only = {k: v for k, v in merged.items() if v.scope == "managed"} + manual_only = { + k: v + for k, v in merged.items() + if v.scope in ("user", "project", "local") + and not is_mcp_server_disabled(k) + } + kept_managed, suppressed_plugin = dedup_plugin_mcp_servers( + managed_only, manual_only + ) + merged = { + **{k: v for k, v in merged.items() if v.scope != "managed"}, + **kept_managed, + } + for rec in suppressed_plugin: + dedup_notice_strings.append( + f"Plugin MCP server {rec.get('name')!r} suppressed; " + f"duplicate of {rec.get('duplicateOf')!r}." + ) + filtered, notices = filter_mcp_servers_by_policy(merged) all_errors = ( diff --git a/tests/test_plugin_mcp_config.py b/tests/test_plugin_mcp_config.py new file mode 100644 index 000000000..2c566d01e --- /dev/null +++ b/tests/test_plugin_mcp_config.py @@ -0,0 +1,210 @@ +"""#286 — plugin-provided MCP servers participate in the config merge. + +`McpPluginWrapper.server_config` (set at registration) flows through +`get_managed_mcp_configs` into the aggregator, the per-name lookup, and +the scope-policy filter like every other scope. Legacy tools-only +registrations stay invisible to the merge. +""" +from __future__ import annotations + +from types import SimpleNamespace +from unittest.mock import patch + +import pytest + +from src.plugins.mcp_integration import ( + clear_mcp_plugins, + wrap_mcp_server_as_plugin, +) +from src.services.mcp.config import ( + filter_mcp_servers_by_policy, + get_all_mcp_configs, + get_managed_mcp_configs, + get_mcp_config_by_name, +) +from src.services.mcp.config import get_mcp_configs_by_scope as _real_by_scope +from src.services.mcp.types import McpStdioServerConfig + + +@pytest.fixture(autouse=True) +def _fresh_plugin_registry(tmp_path, monkeypatch): + """Hermetic environment: empty plugin registry, isolated config + dirs (these are the suite's first end-to-end get_all_mcp_configs + tests — a developer's real ~/.claude or /etc/claude must not flip + assertions), and a cleared enterprise-exists cache (a process-wide + latch nothing else resets).""" + from src.services.mcp.config import clear_enterprise_config_cache + + monkeypatch.setenv("CLAUDE_CONFIG_DIR", str(tmp_path / "cc")) + monkeypatch.setenv("CLAUDE_MANAGED_CONFIG_DIR", str(tmp_path / "managed")) + clear_enterprise_config_cache() + clear_mcp_plugins() + yield + clear_enterprise_config_cache() + clear_mcp_plugins() + + +_CONFIG = McpStdioServerConfig(command="plugin-server", args=["--serve"]) + + +class TestManagedLoader: + def test_registration_with_config_surfaces_in_managed_scope(self): + wrap_mcp_server_as_plugin( + "my-plugin-server", [], server_config=_CONFIG + ) + managed = get_managed_mcp_configs() + assert "my-plugin-server" in managed + scoped = managed["my-plugin-server"] + assert scoped.scope == "managed" + assert scoped.config is _CONFIG + assert scoped.plugin_source == "mcp-my-plugin-server" + + def test_legacy_tools_only_registration_stays_invisible(self): + wrap_mcp_server_as_plugin("legacy", [{"name": "t"}]) + assert get_managed_mcp_configs() == {} + + def test_server_type_reflected_in_loaded_plugin(self): + from src.services.mcp.types import McpSSEServerConfig + + wrapper = wrap_mcp_server_as_plugin( + "sse-server", + [], + server_config=McpSSEServerConfig(url="https://example.com/sse"), + ) + assert wrapper.plugin.mcp_servers == {"sse-server": {"type": "sse"}} + + +class TestMergeParticipation: + def test_appears_in_aggregate_and_by_name(self): + wrap_mcp_server_as_plugin("merged-in", [], server_config=_CONFIG) + servers, _errors = get_all_mcp_configs() + assert "merged-in" in servers + assert servers["merged-in"].scope == "managed" + + scoped = get_mcp_config_by_name("merged-in") + assert scoped is not None + assert scoped.scope == "managed" + assert scoped.config is _CONFIG + + def test_manual_same_name_overrides_plugin(self): + wrap_mcp_server_as_plugin("shadowed", [], server_config=_CONFIG) + from src.services.mcp.types import ScopedMcpServerConfig + + manual = ScopedMcpServerConfig( + config=McpStdioServerConfig(command="manual-bin"), + scope="user", + ) + def _by_scope(scope): + if scope == "user": + return {"shadowed": manual}, [] + return _real_by_scope(scope) + + with patch( + "src.services.mcp.config.get_mcp_configs_by_scope", + side_effect=_by_scope, + ): + servers, _errors = get_all_mcp_configs() + assert servers["shadowed"].scope == "user" + assert servers["shadowed"].config.command == "manual-bin" + + def test_signature_duplicate_of_manual_is_suppressed_with_notice(self): + # Same launch signature under a DIFFERENT name: the operator's + # explicit entry wins; the plugin copy is suppressed + noticed. + wrap_mcp_server_as_plugin( + "plugin-name", + [], + server_config=McpStdioServerConfig(command="same-bin", args=["-x"]), + ) + from src.services.mcp.types import ScopedMcpServerConfig + + manual = ScopedMcpServerConfig( + config=McpStdioServerConfig(command="same-bin", args=["-x"]), + scope="user", + ) + def _by_scope(scope): + if scope == "user": + return {"manual-name": manual}, [] + return _real_by_scope(scope) + + with patch( + "src.services.mcp.config.get_mcp_configs_by_scope", + side_effect=_by_scope, + ): + servers, errors = get_all_mcp_configs() + assert "manual-name" in servers + assert "plugin-name" not in servers + assert any( + "plugin-name" in e.message and "manual-name" in e.message + for e in errors + ) + + +class TestPolicyParticipation: + def test_allow_managed_only_keeps_plugin_servers(self): + wrap_mcp_server_as_plugin("kept", [], server_config=_CONFIG) + managed = get_managed_mcp_configs() + from src.services.mcp.types import ScopedMcpServerConfig + + user_entry = ScopedMcpServerConfig( + config=McpStdioServerConfig(command="user-bin"), scope="user" + ) + with patch( + "src.services.mcp.config._safe_load_settings", + return_value=SimpleNamespace( + extra={"allow_managed_only_mcp": True} + ), + ): + filtered, notices = filter_mcp_servers_by_policy( + {**managed, "user-server": user_entry} + ) + assert "kept" in filtered + assert "user-server" not in filtered + + def test_disabled_manual_twin_does_not_suppress_plugin(self): + # The claudeai-dedup carve-out applies here too: a DISABLED + # manual entry must not suppress its plugin twin, or disabling + # the manual copy would leave zero working servers. + from src.services.mcp.config import set_mcp_server_enabled + from src.services.mcp.types import ScopedMcpServerConfig + + wrap_mcp_server_as_plugin( + "plugin-twin", + [], + server_config=McpStdioServerConfig(command="twin-bin", args=["-y"]), + ) + manual = ScopedMcpServerConfig( + config=McpStdioServerConfig(command="twin-bin", args=["-y"]), + scope="user", + ) + + def _by_scope(scope): + if scope == "user": + return {"manual-twin": manual}, [] + return _real_by_scope(scope) + + set_mcp_server_enabled("manual-twin", False) + try: + with patch( + "src.services.mcp.config.get_mcp_configs_by_scope", + side_effect=_by_scope, + ): + servers, _errors = get_all_mcp_configs() + finally: + set_mcp_server_enabled("manual-twin", True) + assert "plugin-twin" in servers + + +class TestEnterpriseLockdownByName: + def test_by_name_honors_enterprise_short_circuit(self): + # When a managed-mcp.json exists, the aggregate returns + # enterprise-only; the by-name resolve must not be a side door + # that still hands out plugin configs (#286). + wrap_mcp_server_as_plugin("locked-out", [], server_config=_CONFIG) + with patch( + "src.services.mcp.config._does_enterprise_mcp_config_exist", + return_value=True, + ), patch( + "src.services.mcp.config.get_mcp_configs_by_scope", + return_value=({}, []), + ): + assert get_mcp_config_by_name("locked-out") is None