From 510607bfd49e0bf9b3e55690d8460c850c97d010 Mon Sep 17 00:00:00 2001 From: DeepCode Date: Sun, 9 Aug 2026 07:15:50 +0800 Subject: [PATCH 01/23] feat(hooks): SessionEnd lifecycle + PreCompact context injection - SessionEnd: notification-only hook fired on every terminal path (complete / interrupted / error), so summaries can be persisted even when compaction never ran --- core/agent_runtime/runner.py | 11 ++++ core/events/session.py | 22 ++++++++ core/harness/hooks/discovery.py | 89 ++++++++++++++++++++++++++++++--- core/harness/hooks/engine.py | 28 ++++++++++- core/harness/hooks/events.py | 10 ++-- core/harness/hooks/execution.py | 2 + 6 files changed, 150 insertions(+), 12 deletions(-) diff --git a/core/agent_runtime/runner.py b/core/agent_runtime/runner.py index 645ab82f..2451e01a 100644 --- a/core/agent_runtime/runner.py +++ b/core/agent_runtime/runner.py @@ -1662,10 +1662,12 @@ async def _maybe_compact( if estimate is None or estimate <= trigger: return messages + pre_contexts: list[str] = [] if spec.pre_compact_hook is not None: pre = await self._call_tool_hook(spec.pre_compact_hook, "auto") if pre is not None and getattr(pre, "block", False): return messages # a PreCompact hook aborted compaction this turn + pre_contexts = list(getattr(pre, "additional_contexts", None) or []) summary = await self._summarize( spec, @@ -1685,6 +1687,15 @@ async def _maybe_compact( spec.session_key or "default", ) return messages + # memento 式 checkpoint 回注: PreCompact hook 的 additionalContext + # 作为独立 user 消息追加, 随压缩历史一起幸存 (供压缩后模型恢复上下文)。 + if pre_contexts: + compacted = compacted + [ + { + "role": "user", + "content": "[PreCompact checkpoint]\n" + "\n".join(pre_contexts), + } + ] if spec.post_compact_hook is not None: await self._call_tool_hook(spec.post_compact_hook, "auto") logger.info( diff --git a/core/events/session.py b/core/events/session.py index 6af816f0..ab785a67 100644 --- a/core/events/session.py +++ b/core/events/session.py @@ -666,6 +666,21 @@ async def _run_start_hook(self): logger.exception("start hook failed") return None + async def _run_end_hook(self, reason: str = "complete") -> None: + """Run SessionEnd hooks at the close of a turn. + + Notification-only: a failure is logged and never crashes the turn. + Fired on every terminal path (complete / interrupted / error) so + summaries can be persisted even when compaction never ran. + """ + engine = self._hooks_engine + if engine is None or not engine.has_event("SessionEnd"): + return + try: + await engine.run_session_end(reason=reason) + except Exception: # noqa: BLE001 - hooks never crash a turn + logger.exception("session end hook failed") + async def _run_prompt_hooks( self, text: str, hook_contexts: list[str] ) -> str | None: @@ -786,6 +801,13 @@ async def _run_user_input(self, op: UserInput | str) -> None: self._active_turn_task = None if terminal is not None: self._emit(terminal) + reason = ( + terminal.stop_reason + if terminal is not None + and terminal.stop_reason in ("interrupted", "error") + else "complete" + ) + await self._run_end_hook(reason) async def _execute_turn( self, diff --git a/core/harness/hooks/discovery.py b/core/harness/hooks/discovery.py index 45bf4a20..481e0d7f 100644 --- a/core/harness/hooks/discovery.py +++ b/core/harness/hooks/discovery.py @@ -18,8 +18,9 @@ fold order when several hooks fire for one event — is stable and deterministic: 1. user ``~/.deepcode/hooks.json`` - 2. project ``/.deepcode/hooks.json`` - 3. project ``/.claude/settings.json`` (Claude-Code-compatible) + 2. user-mcp ``~/.deepcode/hooks_config.json`` (deepcode-hooks MCP list format) + 3. project ``/.deepcode/hooks.json`` + 4. project ``/.claude/settings.json`` (Claude-Code-compatible) Only ``type: command`` handlers are supported; ``prompt`` / ``agent`` handlers and ``async: true`` are skipped with a warning (the reference does the same). @@ -39,6 +40,22 @@ _DEFAULT_TIMEOUT_SEC = 600 +# deepcode-hooks MCP stores camelCase event names; core uses the reference +# agent's PascalCase names. Keys are matched case-insensitively via .lower(). +_MCP_EVENT_ALIASES: dict[str, str] = { + "sessionstart": "SessionStart", + "sessionend": "SessionEnd", + "pretooluse": "PreToolUse", + "posttooluse": "PostToolUse", + "userpromptsubmit": "UserPromptSubmit", + "permissionrequest": "PermissionRequest", + "precompact": "PreCompact", + "postcompact": "PostCompact", + "subagentstart": "SubagentStart", + "subagentstop": "SubagentStop", + "stop": "Stop", +} + @dataclass(slots=True) class Handler: @@ -48,7 +65,7 @@ class Handler: matcher: str | None command: str timeout_sec: int - source: str # "user" | "project" — for reporting only + source: str # "user" | "user-mcp" | "project" — for reporting only source_path: str display_order: int status_message: str | None = None @@ -67,6 +84,7 @@ def _hook_source_files(workspace: str, home: str | None) -> list[tuple[Path, str ws = Path(workspace) return [ (home_dir / ".deepcode" / "hooks.json", "user"), + (home_dir / ".deepcode" / "hooks_config.json", "user-mcp"), (ws / ".deepcode" / "hooks.json", "project"), (ws / ".claude" / "settings.json", "project"), ] @@ -97,7 +115,12 @@ def discover_hooks(workspace: str, home: str | None = None) -> DiscoveryResult: def _load_hook_events(path: Path, warnings: list[str]) -> dict | None: - """Read one config file and return its ``hooks`` object (or ``None``).""" + """Read one config file and return its ``hooks`` object (or ``None``). + + Accepts both shapes: + - Claude-Code dict format: ``{"hooks": {"EventName": [...]}}`` + - deepcode-hooks MCP list format: ``{"hooks": [ {name, event, handler, ...} ]}`` + """ if not path.is_file(): return None try: @@ -106,9 +129,61 @@ def _load_hook_events(path: Path, warnings: list[str]) -> dict | None: warnings.append(f"failed to read hooks config {path}: {exc}") return None hooks = data.get("hooks") if isinstance(data, dict) else None - if not isinstance(hooks, dict): - return None - return hooks + if isinstance(hooks, dict): + return hooks # Claude-Code format + if isinstance(hooks, list): + # deepcode-hooks MCP list format (hooks_config.json) + return _mcp_hooks_to_events(hooks, warnings, path) + return None + + +def _mcp_hooks_to_events(mcp_hooks: list, warnings: list[str], path: Path) -> dict: + """Convert the deepcode-hooks MCP ``hooks`` list to the events-dict shape. + + Each entry: ``{name, event, handler, type, priority, timeout, enabled, ...}``. + Only ``shell`` / ``node`` handlers are kept — they runnable as plain + commands; ``python``-typed snippets are skipped with a warning. + """ + events: dict[str, list] = {} + for hook in mcp_hooks: + if not isinstance(hook, dict): + continue + if hook.get("enabled") is False: + continue + event = hook.get("event") + if not isinstance(event, str): + continue + canonical = _MCP_EVENT_ALIASES.get(event.lower(), event) + if canonical not in HOOK_EVENT_NAMES: + continue # 与未知事件键一致：静默跳过 (forward-compat) + handler = hook.get("handler") + if not isinstance(handler, str) or not handler.strip(): + continue + htype = hook.get("type", "shell") + if htype not in ("shell", "node"): + warnings.append( + f"skipping {htype!r} hook {hook.get('name', '')!r} in {path}: " + "only shell/node handlers are runnable as commands" + ) + continue + timeout = hook.get("timeout") + try: + timeout_sec = max(1, int(timeout)) if timeout is not None else None + except (TypeError, ValueError): + timeout_sec = None + events.setdefault(canonical, []).append( + { + "matcher": "*", + "hooks": [ + { + "type": "command", + "command": handler, + **({"timeout": timeout_sec} if timeout_sec is not None else {}), + } + ], + } + ) + return events def _append_group( diff --git a/core/harness/hooks/engine.py b/core/harness/hooks/engine.py index 26f66a11..d904689c 100644 --- a/core/harness/hooks/engine.py +++ b/core/harness/hooks/engine.py @@ -176,6 +176,22 @@ async def run_session_start(self, source: str = "startup") -> ContextOutcome: additional_contexts=folded.additional_contexts, ) + async def run_session_end(self, reason: str = "complete") -> ContextOutcome: + """Session lifecycle end — a notification hook (summary persistence, etc.). + + Fires unconditionally at the close of a turn (complete / interrupted / + error), unlike ``PreCompact`` which only fires when a summarization pass + actually runs. Matchers are ignored (see ``_EVENTS_WITHOUT_MATCHER``); + the caller logs failures so a hook can never crash the turn. + """ + payload = {"hook_event_name": "SessionEnd", "reason": reason} + folded = await self._dispatch("SessionEnd", None, payload) + return ContextOutcome( + block=folded.block, + block_reason=folded.block_reason, + additional_contexts=folded.additional_contexts, + ) + async def run_user_prompt_submit(self, prompt: str) -> ContextOutcome: payload = {"hook_event_name": "UserPromptSubmit", "prompt": prompt} folded = await self._dispatch("UserPromptSubmit", None, payload) @@ -192,10 +208,18 @@ async def run_stop(self, stop_hook_active: bool = False) -> StopOutcome: async def run_pre_compact(self, trigger: str = "auto") -> ContextOutcome: """Before a summarization pass. A ``block`` (continue:false) asks to skip - compaction this turn; the matcher runs against ``trigger`` (auto/manual).""" + compaction this turn; the matcher runs against ``trigger`` (auto/manual). + + ``additional_contexts`` from hook ``hookSpecificOutput.additionalContext`` + are passed through so a PreCompact hook can inject a checkpoint summary + (memento-style) that survives the compaction.""" payload = {"hook_event_name": "PreCompact", "trigger": trigger} folded = await self._dispatch("PreCompact", trigger, payload) - return ContextOutcome(block=folded.block, block_reason=folded.block_reason) + return ContextOutcome( + block=folded.block, + block_reason=folded.block_reason, + additional_contexts=folded.additional_contexts, + ) async def run_post_compact(self, trigger: str = "auto") -> ContextOutcome: """After a summarization pass — a notification hook (state saved, etc.).""" diff --git a/core/harness/hooks/events.py b/core/harness/hooks/events.py index ed393156..edd2b668 100644 --- a/core/harness/hooks/events.py +++ b/core/harness/hooks/events.py @@ -31,15 +31,19 @@ "PreCompact", "PostCompact", "SessionStart", + "SessionEnd", "UserPromptSubmit", "SubagentStart", "SubagentStop", "Stop", ) -# Events whose ``matcher`` field is meaningful. ``UserPromptSubmit`` and ``Stop`` -# fire unconditionally, so their matchers are ignored (mirrors the reference). -_EVENTS_WITHOUT_MATCHER: frozenset[str] = frozenset({"UserPromptSubmit", "Stop"}) +# Events whose ``matcher`` field is meaningful. ``UserPromptSubmit``, ``Stop`` +# and ``SessionEnd`` fire unconditionally, so their matchers are ignored +# (mirrors the reference). +_EVENTS_WITHOUT_MATCHER: frozenset[str] = frozenset( + {"UserPromptSubmit", "Stop", "SessionEnd"} +) def matcher_applies_to_event(event_name: str, matcher: str | None) -> str | None: diff --git a/core/harness/hooks/execution.py b/core/harness/hooks/execution.py index b11977ef..ff772a9d 100644 --- a/core/harness/hooks/execution.py +++ b/core/harness/hooks/execution.py @@ -250,7 +250,9 @@ def _decode_permission_request(obj: dict) -> HandlerDecision: "Stop": lambda o: _block_from_decision(o, "Stop"), "SubagentStop": lambda o: _block_from_decision(o, "SubagentStop"), "SessionStart": _decode_additional_context_only, + "SessionEnd": _decode_additional_context_only, "SubagentStart": _decode_additional_context_only, + "PreCompact": lambda o: _block_from_decision(o, "PreCompact"), "PermissionRequest": _decode_permission_request, } From c446a2089ea04b046703879a054c608afcb2b533 Mon Sep 17 00:00:00 2001 From: raymondginger Date: Tue, 11 Aug 2026 12:04:20 +0800 Subject: [PATCH 02/23] fix(hooks): address review comments for SessionEnd lifecycle and PreCompact context - SessionEnd fires exactly once at real session termination (AgentSession.submit(Shutdown)), never per turn; per-turn notifications belong to the Stop event. - SessionEnd honours its matcher: the session-exit reason (shutdown/interrupted/error) is the matcher input. - hooks_config.json (deepcode-hooks MCP list format) is accepted only from the user-mcp source, with explicit validation, priority ordering, timeout parsing, event aliases and optional matchers. - PreCompact checkpoint re-injection is bounded and provider-safe: per-context and total limits, only after a successful compaction. - Add e2e regression tests (tests/test_session_end_lifecycle.py). --- core/agent_runtime/runner.py | 55 +++- core/events/session.py | 28 +- core/harness/hooks/discovery.py | 78 +++++- core/harness/hooks/engine.py | 17 +- core/harness/hooks/events.py | 10 +- tests/test_session_end_lifecycle.py | 383 ++++++++++++++++++++++++++++ 6 files changed, 524 insertions(+), 47 deletions(-) create mode 100644 tests/test_session_end_lifecycle.py diff --git a/core/agent_runtime/runner.py b/core/agent_runtime/runner.py index 2451e01a..9b244e9a 100644 --- a/core/agent_runtime/runner.py +++ b/core/agent_runtime/runner.py @@ -93,6 +93,42 @@ ) _BACKFILL_CONTENT = "[Tool result unavailable — call was interrupted or lost]" +# PreCompact checkpoint re-injection (bounded, provider-safe). A PreCompact +# hook may attach ``additional_contexts`` that must survive a successful +# compaction so the post-compaction model can restore working context. The +# re-injection is a plain ``role: user`` message (provider-agnostic) carrying a +# clearly delimited prefix, with hard limits per context and in total — a +# runaway hook can never blow the post-compaction window back open. +_PRECOMPACT_CHECKPOINT_PREFIX = "[PreCompact checkpoint]" +_PRECOMPACT_CONTEXT_LIMIT = 2000 # chars per additional context +_PRECOMPACT_TOTAL_LIMIT = 8000 # chars for the whole checkpoint block + + +def _build_precompact_checkpoint(contexts: list[str]) -> str | None: + """Bounded, delimited representation of PreCompact hook context. + + Each context is stripped, truncated to ``_PRECOMPACT_CONTEXT_LIMIT`` chars + and the combined block capped at ``_PRECOMPACT_TOTAL_LIMIT``. Returns + ``None`` when nothing survives (empty input or all contexts blank). + """ + if not contexts: + return None + parts: list[str] = [] + used = 0 + for ctx in contexts: + text = (ctx or "").strip() + if not text: + continue + text = text[:_PRECOMPACT_CONTEXT_LIMIT] + room = _PRECOMPACT_TOTAL_LIMIT - used + if room <= 0: + break + parts.append(text[:room]) + used += min(len(text), room) + 1 # +1 for the newline separator + if not parts: + return None + return _PRECOMPACT_CHECKPOINT_PREFIX + "\n" + "\n".join(parts) + @dataclass(slots=True) class AgentRunSpec: @@ -1687,15 +1723,16 @@ async def _maybe_compact( spec.session_key or "default", ) return messages - # memento 式 checkpoint 回注: PreCompact hook 的 additionalContext - # 作为独立 user 消息追加, 随压缩历史一起幸存 (供压缩后模型恢复上下文)。 - if pre_contexts: - compacted = compacted + [ - { - "role": "user", - "content": "[PreCompact checkpoint]\n" + "\n".join(pre_contexts), - } - ] + # Bounded checkpoint re-injection: the PreCompact hook's + # ``additional_contexts`` survive a successful compaction as a single + # provider-agnostic user message. ``_build_precompact_checkpoint`` + # caps each context and the total block, so a runaway hook can never + # blow the post-compaction window back open. When the hook blocks or + # summarization fails we returned above — the checkpoint only ever + # appears after a successful compaction. + checkpoint = _build_precompact_checkpoint(pre_contexts) + if checkpoint: + compacted = compacted + [{"role": "user", "content": checkpoint}] if spec.post_compact_hook is not None: await self._call_tool_hook(spec.post_compact_hook, "auto") logger.info( diff --git a/core/events/session.py b/core/events/session.py index ab785a67..285421f7 100644 --- a/core/events/session.py +++ b/core/events/session.py @@ -598,6 +598,10 @@ async def submit(self, op: Op) -> None: if task is not None and not task.done() and task.cancelling() == 0: task.cancel() elif isinstance(op, Shutdown): + # SessionEnd fires exactly once, here — at the real session + # termination boundary — never per turn. Per-turn notifications + # are the Stop event's job (see _EVENTS_WITHOUT_MATCHER). + await self._run_end_hook(reason="shutdown") self._emit(ShutdownComplete()) else: # pragma: no cover - exhaustive guard self._emit(ErrorEvent(message=f"unknown op: {op!r}")) @@ -666,19 +670,20 @@ async def _run_start_hook(self): logger.exception("start hook failed") return None - async def _run_end_hook(self, reason: str = "complete") -> None: - """Run SessionEnd hooks at the close of a turn. + async def _run_end_hook(self, reason: str = "shutdown") -> None: + """Run SessionEnd hooks when the session itself terminates. - Notification-only: a failure is logged and never crashes the turn. - Fired on every terminal path (complete / interrupted / error) so - summaries can be persisted even when compaction never ran. + Notification-only: a failure is logged and never crashes the + shutdown. Fired exactly once per session from ``submit(Shutdown)`` + with a documented session-exit reason (``shutdown``); per-turn + notifications belong to the Stop event, not SessionEnd. """ engine = self._hooks_engine if engine is None or not engine.has_event("SessionEnd"): return try: await engine.run_session_end(reason=reason) - except Exception: # noqa: BLE001 - hooks never crash a turn + except Exception: # noqa: BLE001 - hooks never crash a shutdown logger.exception("session end hook failed") async def _run_prompt_hooks( @@ -801,13 +806,10 @@ async def _run_user_input(self, op: UserInput | str) -> None: self._active_turn_task = None if terminal is not None: self._emit(terminal) - reason = ( - terminal.stop_reason - if terminal is not None - and terminal.stop_reason in ("interrupted", "error") - else "complete" - ) - await self._run_end_hook(reason) + # SessionEnd is NOT fired here: this finally block runs after + # every turn, and SessionEnd must fire exactly once at session + # termination (submit(Shutdown)), not per turn. Per-turn + # notifications are the Stop event's responsibility. async def _execute_turn( self, diff --git a/core/harness/hooks/discovery.py b/core/harness/hooks/discovery.py index 481e0d7f..7309ba90 100644 --- a/core/harness/hooks/discovery.py +++ b/core/harness/hooks/discovery.py @@ -101,7 +101,7 @@ def discover_hooks(workspace: str, home: str | None = None) -> DiscoveryResult: warnings: list[str] = [] order = 0 for path, source in _hook_source_files(workspace, home): - events = _load_hook_events(path, warnings) + events = _load_hook_events(path, warnings, source) if not events: continue for event_name, groups in events.items(): @@ -114,12 +114,16 @@ def discover_hooks(workspace: str, home: str | None = None) -> DiscoveryResult: return DiscoveryResult(handlers=handlers, warnings=warnings) -def _load_hook_events(path: Path, warnings: list[str]) -> dict | None: +def _load_hook_events(path: Path, warnings: list[str], source: str) -> dict | None: """Read one config file and return its ``hooks`` object (or ``None``). - Accepts both shapes: - - Claude-Code dict format: ``{"hooks": {"EventName": [...]}}`` - - deepcode-hooks MCP list format: ``{"hooks": [ {name, event, handler, ...} ]}`` + Two shapes are accepted: + + - Claude-Code dict format (``{"hooks": {"EventName": [...]}}``) — any source. + - deepcode-hooks MCP list format (``{"hooks": [...]}``) — **only** from the + ``user-mcp`` source (``~/.deepcode/hooks_config.json``). A list shape in + any other source is rejected with a warning so an accidental shape + mismatch cannot silently disable hooks. """ if not path.is_file(): return None @@ -133,6 +137,13 @@ def _load_hook_events(path: Path, warnings: list[str]) -> dict | None: return hooks # Claude-Code format if isinstance(hooks, list): # deepcode-hooks MCP list format (hooks_config.json) + if source != "user-mcp": + warnings.append( + f"ignoring list-shaped hooks in {path}: only " + "~/.deepcode/hooks_config.json supports the deepcode-hooks " + "list format" + ) + return None return _mcp_hooks_to_events(hooks, warnings, path) return None @@ -140,29 +151,47 @@ def _load_hook_events(path: Path, warnings: list[str]) -> dict | None: def _mcp_hooks_to_events(mcp_hooks: list, warnings: list[str], path: Path) -> dict: """Convert the deepcode-hooks MCP ``hooks`` list to the events-dict shape. - Each entry: ``{name, event, handler, type, priority, timeout, enabled, ...}``. - Only ``shell`` / ``node`` handlers are kept — they runnable as plain - commands; ``python``-typed snippets are skipped with a warning. + Each entry: ``{name, event, handler, type, priority, timeout, enabled, + matcher, ...}``. Entries are validated explicitly — a malformed entry is + reported in ``warnings`` and skipped, never silently dropped. Only + ``shell`` / ``node`` handlers are kept (they run as plain commands); + ``python``-typed snippets are skipped with a warning. Within one event + the groups are ordered by ``priority`` (highest first; stable so equal + priorities keep declaration order). """ events: dict[str, list] = {} for hook in mcp_hooks: if not isinstance(hook, dict): + warnings.append(f"skipping non-object hook entry in {path}") + continue + name = hook.get("name") + if not isinstance(name, str) or not name.strip(): + warnings.append(f"skipping hook without a name in {path}") continue if hook.get("enabled") is False: continue event = hook.get("event") - if not isinstance(event, str): + if not isinstance(event, str) or not event.strip(): + warnings.append( + f"skipping hook {name!r} without an event in {path}" + ) continue canonical = _MCP_EVENT_ALIASES.get(event.lower(), event) if canonical not in HOOK_EVENT_NAMES: - continue # 与未知事件键一致：静默跳过 (forward-compat) + warnings.append( + f"skipping hook {name!r} with unknown event {event!r} in {path}" + ) + continue handler = hook.get("handler") if not isinstance(handler, str) or not handler.strip(): + warnings.append( + f"skipping hook {name!r} without a handler in {path}" + ) continue htype = hook.get("type", "shell") if htype not in ("shell", "node"): warnings.append( - f"skipping {htype!r} hook {hook.get('name', '')!r} in {path}: " + f"skipping {htype!r} hook {name!r} in {path}: " "only shell/node handlers are runnable as commands" ) continue @@ -170,10 +199,28 @@ def _mcp_hooks_to_events(mcp_hooks: list, warnings: list[str], path: Path) -> di try: timeout_sec = max(1, int(timeout)) if timeout is not None else None except (TypeError, ValueError): + warnings.append( + f"ignoring invalid timeout {timeout!r} for hook {name!r} in {path}" + ) timeout_sec = None + raw_matcher = hook.get("matcher") + matcher = ( + raw_matcher + if isinstance(raw_matcher, str) and raw_matcher.strip() + else "*" + ) + priority = hook.get("priority", 0) + try: + priority_int = int(priority) + except (TypeError, ValueError): + warnings.append( + f"ignoring invalid priority {priority!r} for hook {name!r} in {path}" + ) + priority_int = 0 events.setdefault(canonical, []).append( { - "matcher": "*", + "matcher": matcher, + "priority": priority_int, "hooks": [ { "type": "command", @@ -183,9 +230,14 @@ def _mcp_hooks_to_events(mcp_hooks: list, warnings: list[str], path: Path) -> di ], } ) + # Higher ``priority`` runs first (stable sort keeps equal priorities in + # declaration order); the transient key is dropped before _append_group. + for groups in events.values(): + groups.sort(key=lambda group: group.get("priority", 0), reverse=True) + for group in groups: + group.pop("priority", None) return events - def _append_group( handlers: list[Handler], warnings: list[str], diff --git a/core/harness/hooks/engine.py b/core/harness/hooks/engine.py index d904689c..2791a80c 100644 --- a/core/harness/hooks/engine.py +++ b/core/harness/hooks/engine.py @@ -176,16 +176,17 @@ async def run_session_start(self, source: str = "startup") -> ContextOutcome: additional_contexts=folded.additional_contexts, ) - async def run_session_end(self, reason: str = "complete") -> ContextOutcome: - """Session lifecycle end — a notification hook (summary persistence, etc.). - - Fires unconditionally at the close of a turn (complete / interrupted / - error), unlike ``PreCompact`` which only fires when a summarization pass - actually runs. Matchers are ignored (see ``_EVENTS_WITHOUT_MATCHER``); - the caller logs failures so a hook can never crash the turn. + async def run_session_end(self, reason: str = "shutdown") -> ContextOutcome: + """Session lifecycle end — fires exactly once when the session terminates. + + Called from ``AgentSession.submit(Shutdown)``, never per turn. The + reason doubles as the matcher input so a hook can target a specific + exit path (e.g. ``matcher: "shutdown"``); supported session-exit + reasons are ``shutdown``, ``interrupted`` and ``error``. The caller + logs failures so a hook can never crash the session close. """ payload = {"hook_event_name": "SessionEnd", "reason": reason} - folded = await self._dispatch("SessionEnd", None, payload) + folded = await self._dispatch("SessionEnd", reason, payload) return ContextOutcome( block=folded.block, block_reason=folded.block_reason, diff --git a/core/harness/hooks/events.py b/core/harness/hooks/events.py index edd2b668..6fbc1f43 100644 --- a/core/harness/hooks/events.py +++ b/core/harness/hooks/events.py @@ -38,11 +38,13 @@ "Stop", ) -# Events whose ``matcher`` field is meaningful. ``UserPromptSubmit``, ``Stop`` -# and ``SessionEnd`` fire unconditionally, so their matchers are ignored -# (mirrors the reference). +# Events whose ``matcher`` field is meaningful. ``UserPromptSubmit`` and +# ``Stop`` fire unconditionally, so their matchers are ignored (mirrors the +# reference). ``SessionEnd`` DOES honour its matcher: the session-exit reason +# (``shutdown`` / ``interrupted`` / ``error``) is matched against the +# ``matcher`` field so hooks can target a specific exit path. _EVENTS_WITHOUT_MATCHER: frozenset[str] = frozenset( - {"UserPromptSubmit", "Stop", "SessionEnd"} + {"UserPromptSubmit", "Stop"} ) diff --git a/tests/test_session_end_lifecycle.py b/tests/test_session_end_lifecycle.py new file mode 100644 index 00000000..ce15d9d5 --- /dev/null +++ b/tests/test_session_end_lifecycle.py @@ -0,0 +1,383 @@ +"""SessionEnd lifecycle + PreCompact checkpoint + MCP list-discovery e2e tests. + +Covers the lifecycle contracts introduced by the SessionEnd / PreCompact work: + +- ``SessionEnd`` fires exactly once at real session termination + (``AgentSession.submit(Shutdown)``), never per turn; the session-exit reason + doubles as the matcher input (``shutdown`` / ``interrupted`` / ``error``); + a failing hook is non-fatal and never blocks ``ShutdownComplete``. +- The ``PreCompact`` hook's ``additional_contexts`` survive a successful + compaction as a single bounded, provider-agnostic user message, and are + absent when the hook blocks or summarization fails. +- The deepcode-hooks MCP ``hooks_config.json`` list format is only accepted + from the ``user-mcp`` source, supports ``priority`` ordering, skips disabled + entries, warns on invalid entries, and honours timeouts and event aliases. + +Hooks are exercised as REAL subprocesses (``sh -lc`` commands that echo JSON or +exit with a code), matching ``test_hooks.py`` so we test the true execution +path, not a mock of it. +""" + +from __future__ import annotations + +import asyncio +import json +import shutil +import sys +from pathlib import Path + +import pytest + +ROOT = Path(__file__).resolve().parents[1] +if str(ROOT) not in sys.path: + sys.path.insert(0, str(ROOT)) + +from core.harness.hooks.discovery import Handler, discover_hooks # noqa: E402 +from core.harness.hooks.engine import HooksEngine # noqa: E402 +from core.events.protocol import Shutdown, ShutdownComplete, UserInput # noqa: E402 + +pytestmark = pytest.mark.skipif( + shutil.which("sh") is None, reason="POSIX shell required" +) + + +def _handler(event, command, *, matcher=None, order=0, timeout=30): + return Handler( + event_name=event, + matcher=matcher, + command=command, + timeout_sec=timeout, + source="project", + source_path="/tmp/hooks.json", + display_order=order, + ) + + +def _engine(handlers, cwd="/tmp"): + return HooksEngine(handlers, cwd, session_id="sess-1") + + +def _session(hooks_engine): + from core.events.session import AgentSession + + return AgentSession( + provider=None, + tools=_FakeTools(), + model="m", + hooks_engine=hooks_engine, + context_window_tokens=8000, + ) + + +class _FakeTools: + """Minimal tool registry: records the params each tool ran with.""" + + def __init__(self): + self.calls = [] + + def get_definitions(self): + return [] + + async def execute(self, name, params): + self.calls.append((name, params)) + return f"ran {name} with {params}" + + +# --------------------------------------------------------------------------- +# SessionEnd lifecycle — fires exactly once at real session termination +# --------------------------------------------------------------------------- + + +def test_session_end_fires_exactly_once_on_shutdown(tmp_path): + count = tmp_path / "count.txt" + eng = _engine([_handler("SessionEnd", f"echo x >> {count}")]) + session = _session(eng) + + asyncio.run(session.submit(Shutdown())) + + assert count.read_text().count("x") == 1 + event = asyncio.run(session.next_event()) + assert isinstance(event.msg, ShutdownComplete) + + +def test_session_end_reason_matcher(tmp_path): + shutdown_hits = tmp_path / "shutdown.txt" + other_hits = tmp_path / "other.txt" + eng = _engine( + [ + _handler("SessionEnd", f"echo x >> {shutdown_hits}", matcher="shutdown"), + _handler("SessionEnd", f"echo x >> {other_hits}", matcher="complete"), + ] + ) + session = _session(eng) + + asyncio.run(session.submit(Shutdown())) + + assert shutdown_hits.read_text().count("x") == 1 + assert not other_hits.exists() + + +def test_session_end_hook_failure_non_fatal(): + eng = _engine([_handler("SessionEnd", "exit 3")]) + session = _session(eng) + + # A failing SessionEnd hook must never crash the session close. + asyncio.run(session.submit(Shutdown())) + event = asyncio.run(session.next_event()) + assert isinstance(event.msg, ShutdownComplete) + + +def test_normal_turn_does_not_trigger_session_end(tmp_path): + count = tmp_path / "count.txt" + eng = _engine([_handler("SessionEnd", f"echo x >> {count}")]) + session = _session(eng) + + async def _noop_user_input(op): + return None + + session._run_user_input = _noop_user_input # type: ignore[assignment] + asyncio.run(session.submit(UserInput("hello"))) + assert not count.exists() + + asyncio.run(session.submit(Shutdown())) + assert count.read_text().count("x") == 1 + + +# --------------------------------------------------------------------------- +# PreCompact checkpoint — bounded, provider-safe re-injection +# --------------------------------------------------------------------------- + + +def test_build_precompact_checkpoint_empty(): + from core.agent_runtime.runner import _build_precompact_checkpoint + + assert _build_precompact_checkpoint([]) is None + assert _build_precompact_checkpoint([" "]) is None + + +def test_build_precompact_checkpoint_limits(): + from core.agent_runtime.runner import ( + _PRECOMPACT_CHECKPOINT_PREFIX, + _PRECOMPACT_CONTEXT_LIMIT, + _PRECOMPACT_TOTAL_LIMIT, + _build_precompact_checkpoint, + ) + + long_ctx = "y" * (_PRECOMPACT_CONTEXT_LIMIT * 2) + checkpoint = _build_precompact_checkpoint([long_ctx]) + assert checkpoint is not None + body = checkpoint[len(_PRECOMPACT_CHECKPOINT_PREFIX) + 1 :] + assert len(body) == _PRECOMPACT_CONTEXT_LIMIT + + many = ["z" * 3000] * 10 + checkpoint = _build_precompact_checkpoint(many) + assert checkpoint is not None + body = checkpoint[len(_PRECOMPACT_CHECKPOINT_PREFIX) + 1 :] + assert len(body) <= _PRECOMPACT_TOTAL_LIMIT + + +def test_maybe_compact_checkpoint_injected_after_success(monkeypatch): + from types import SimpleNamespace + + from core.agent_runtime.runner import AgentRunSpec, AgentRunner + + runner = AgentRunner(provider=object()) + monkeypatch.setattr( + "core.agent_runtime.runner.estimate_prompt_tokens_chain", + lambda *args, **kwargs: (999_999, None), + ) + + async def fake_summarize(spec, messages, *, response_observer=None): + return "handoff summary" + + monkeypatch.setattr(runner, "_summarize", fake_summarize) + + async def pre_compact_hook(trigger): + return SimpleNamespace(block=False, additional_contexts=["checkpoint ctx"]) + + spec = AgentRunSpec( + initial_messages=[], + tools=_FakeTools(), + model="m", + max_iterations=1, + max_tool_result_chars=100000, + context_window_tokens=8000, + pre_compact_hook=pre_compact_hook, + ) + messages = [ + {"role": "user", "content": "turn 1"}, + {"role": "assistant", "content": "a1"}, + {"role": "user", "content": "turn 2"}, + {"role": "assistant", "content": "a2"}, + {"role": "user", "content": "turn 3"}, + ] + compacted = asyncio.run(runner._maybe_compact(spec, messages)) + checkpoint_msgs = [ + m for m in compacted if m.get("role") == "user" and "PreCompact checkpoint" in str(m.get("content")) + ] + assert checkpoint_msgs, "checkpoint must survive a successful compaction" + assert "checkpoint ctx" in checkpoint_msgs[0]["content"] + + +def test_maybe_compact_block_skips_checkpoint(monkeypatch): + from types import SimpleNamespace + + from core.agent_runtime.runner import AgentRunSpec, AgentRunner + + runner = AgentRunner(provider=object()) + monkeypatch.setattr( + "core.agent_runtime.runner.estimate_prompt_tokens_chain", + lambda *args, **kwargs: (999_999, None), + ) + + async def pre_compact_hook(trigger): + return SimpleNamespace(block=True, additional_contexts=["should not appear"]) + + spec = AgentRunSpec( + initial_messages=[], + tools=_FakeTools(), + model="m", + max_iterations=1, + max_tool_result_chars=100000, + context_window_tokens=8000, + pre_compact_hook=pre_compact_hook, + ) + messages = [ + {"role": "user", "content": "turn 1"}, + {"role": "assistant", "content": "a1"}, + {"role": "user", "content": "turn 2"}, + {"role": "assistant", "content": "a2"}, + {"role": "user", "content": "turn 3"}, + ] + compacted = asyncio.run(runner._maybe_compact(spec, messages)) + assert compacted is messages # compaction aborted this turn + assert "PreCompact checkpoint" not in json.dumps(compacted) + + +def test_maybe_compact_summarize_failure_no_checkpoint(monkeypatch): + from types import SimpleNamespace + + from core.agent_runtime.runner import AgentRunSpec, AgentRunner + + runner = AgentRunner(provider=object()) + monkeypatch.setattr( + "core.agent_runtime.runner.estimate_prompt_tokens_chain", + lambda *args, **kwargs: (999_999, None), + ) + + async def fake_summarize_fails(spec, messages, *, response_observer=None): + return None # summarization failed + + monkeypatch.setattr(runner, "_summarize", fake_summarize_fails) + + async def pre_compact_hook(trigger): + return SimpleNamespace(block=False, additional_contexts=["should not appear"]) + + spec = AgentRunSpec( + initial_messages=[], + tools=_FakeTools(), + model="m", + max_iterations=1, + max_tool_result_chars=100000, + context_window_tokens=8000, + pre_compact_hook=pre_compact_hook, + ) + messages = [ + {"role": "user", "content": "turn 1"}, + {"role": "assistant", "content": "a1"}, + {"role": "user", "content": "turn 2"}, + {"role": "assistant", "content": "a2"}, + {"role": "user", "content": "turn 3"}, + ] + compacted = asyncio.run(runner._maybe_compact(spec, messages)) + assert compacted is messages + assert "PreCompact checkpoint" not in json.dumps(compacted) + + +# --------------------------------------------------------------------------- +# deepcode-hooks MCP list-format discovery (hooks_config.json) +# --------------------------------------------------------------------------- + + +def _write_config(home, ws, payload): + (home / ".deepcode").mkdir(parents=True, exist_ok=True) + (ws / ".deepcode").mkdir(parents=True, exist_ok=True) + (home / ".deepcode" / "hooks_config.json").write_text( + json.dumps(payload), encoding="utf-8" + ) + return str(ws), str(home) + + +def test_mcp_list_format_accepted_from_user_mcp(tmp_path): + ws, home = _write_config( + tmp_path / "home", + tmp_path / "ws", + {"hooks": [{"name": "h1", "event": "PreToolUse", "handler": "echo hi"}]}, + ) + result = discover_hooks(ws, home) + assert result.warnings == [] + assert any(h.event_name == "PreToolUse" and h.command == "echo hi" for h in result.handlers) + + +def test_mcp_list_format_rejected_from_other_sources(tmp_path): + home = tmp_path / "home" + ws = tmp_path / "ws" + (home / ".deepcode").mkdir(parents=True, exist_ok=True) + (ws / ".deepcode").mkdir(parents=True, exist_ok=True) + # list shape in a project hooks.json (non user-mcp source) must be rejected + (ws / ".deepcode" / "hooks.json").write_text( + json.dumps({"hooks": [{"name": "h1", "event": "PreToolUse", "handler": "echo hi"}]}), + encoding="utf-8", + ) + result = discover_hooks(str(ws), str(home)) + assert any("list-shaped hooks" in w for w in result.warnings) + assert result.handlers == [] + + +def test_mcp_priority_ordering(tmp_path): + ws, home = _write_config( + tmp_path / "home", + tmp_path / "ws", + { + "hooks": [ + {"name": "low", "event": "PreToolUse", "handler": "echo low", "priority": 1}, + {"name": "high", "event": "PreToolUse", "handler": "echo high", "priority": 10}, + ] + }, + ) + result = discover_hooks(ws, home) + pre_tool = [h for h in result.handlers if h.event_name == "PreToolUse"] + assert [h.command for h in pre_tool] == ["echo high", "echo low"] + assert pre_tool[0].display_order < pre_tool[1].display_order + + +def test_mcp_disabled_and_invalid_entries(tmp_path): + ws, home = _write_config( + tmp_path / "home", + tmp_path / "ws", + { + "hooks": [ + {"name": "disabled", "event": "PreToolUse", "handler": "echo x", "enabled": False}, + {"name": "no-event", "handler": "echo x"}, + {"name": "bad-type", "event": "PreToolUse", "handler": "pass", "type": "python"}, + {"name": "ok", "event": "PreToolUse", "handler": "echo ok"}, + ] + }, + ) + result = discover_hooks(ws, home) + assert [h.command for h in result.handlers] == ["echo ok"] + assert any("without an event" in w for w in result.warnings) + assert any("only shell/node" in w for w in result.warnings) + + +def test_mcp_timeout_and_event_alias(tmp_path): + ws, home = _write_config( + tmp_path / "home", + tmp_path / "ws", + {"hooks": [{"name": "aliased", "event": "sessionStart", "handler": "echo aliased", "timeout": 7}]}, + ) + result = discover_hooks(ws, home) + assert result.warnings == [] + hook = result.handlers[0] + assert hook.event_name == "SessionStart" + assert hook.timeout_sec == 7 From 105c9b5280c673b2ab9ac5bf06cb864e4a289e1e Mon Sep 17 00:00:00 2001 From: raymondginger Date: Tue, 11 Aug 2026 12:20:26 +0800 Subject: [PATCH 03/23] style: apply ruff format to hooks files and lifecycle tests --- core/harness/hooks/discovery.py | 13 +++----- core/harness/hooks/events.py | 4 +-- tests/test_session_end_lifecycle.py | 51 ++++++++++++++++++++++++----- 3 files changed, 48 insertions(+), 20 deletions(-) diff --git a/core/harness/hooks/discovery.py b/core/harness/hooks/discovery.py index 7309ba90..c9a0a605 100644 --- a/core/harness/hooks/discovery.py +++ b/core/harness/hooks/discovery.py @@ -172,9 +172,7 @@ def _mcp_hooks_to_events(mcp_hooks: list, warnings: list[str], path: Path) -> di continue event = hook.get("event") if not isinstance(event, str) or not event.strip(): - warnings.append( - f"skipping hook {name!r} without an event in {path}" - ) + warnings.append(f"skipping hook {name!r} without an event in {path}") continue canonical = _MCP_EVENT_ALIASES.get(event.lower(), event) if canonical not in HOOK_EVENT_NAMES: @@ -184,9 +182,7 @@ def _mcp_hooks_to_events(mcp_hooks: list, warnings: list[str], path: Path) -> di continue handler = hook.get("handler") if not isinstance(handler, str) or not handler.strip(): - warnings.append( - f"skipping hook {name!r} without a handler in {path}" - ) + warnings.append(f"skipping hook {name!r} without a handler in {path}") continue htype = hook.get("type", "shell") if htype not in ("shell", "node"): @@ -205,9 +201,7 @@ def _mcp_hooks_to_events(mcp_hooks: list, warnings: list[str], path: Path) -> di timeout_sec = None raw_matcher = hook.get("matcher") matcher = ( - raw_matcher - if isinstance(raw_matcher, str) and raw_matcher.strip() - else "*" + raw_matcher if isinstance(raw_matcher, str) and raw_matcher.strip() else "*" ) priority = hook.get("priority", 0) try: @@ -238,6 +232,7 @@ def _mcp_hooks_to_events(mcp_hooks: list, warnings: list[str], path: Path) -> di group.pop("priority", None) return events + def _append_group( handlers: list[Handler], warnings: list[str], diff --git a/core/harness/hooks/events.py b/core/harness/hooks/events.py index 6fbc1f43..ca5ee64a 100644 --- a/core/harness/hooks/events.py +++ b/core/harness/hooks/events.py @@ -43,9 +43,7 @@ # reference). ``SessionEnd`` DOES honour its matcher: the session-exit reason # (``shutdown`` / ``interrupted`` / ``error``) is matched against the # ``matcher`` field so hooks can target a specific exit path. -_EVENTS_WITHOUT_MATCHER: frozenset[str] = frozenset( - {"UserPromptSubmit", "Stop"} -) +_EVENTS_WITHOUT_MATCHER: frozenset[str] = frozenset({"UserPromptSubmit", "Stop"}) def matcher_applies_to_event(event_name: str, matcher: str | None) -> str | None: diff --git a/tests/test_session_end_lifecycle.py b/tests/test_session_end_lifecycle.py index ce15d9d5..adbe1959 100644 --- a/tests/test_session_end_lifecycle.py +++ b/tests/test_session_end_lifecycle.py @@ -213,7 +213,9 @@ async def pre_compact_hook(trigger): ] compacted = asyncio.run(runner._maybe_compact(spec, messages)) checkpoint_msgs = [ - m for m in compacted if m.get("role") == "user" and "PreCompact checkpoint" in str(m.get("content")) + m + for m in compacted + if m.get("role") == "user" and "PreCompact checkpoint" in str(m.get("content")) ] assert checkpoint_msgs, "checkpoint must survive a successful compaction" assert "checkpoint ctx" in checkpoint_msgs[0]["content"] @@ -316,7 +318,9 @@ def test_mcp_list_format_accepted_from_user_mcp(tmp_path): ) result = discover_hooks(ws, home) assert result.warnings == [] - assert any(h.event_name == "PreToolUse" and h.command == "echo hi" for h in result.handlers) + assert any( + h.event_name == "PreToolUse" and h.command == "echo hi" for h in result.handlers + ) def test_mcp_list_format_rejected_from_other_sources(tmp_path): @@ -326,7 +330,9 @@ def test_mcp_list_format_rejected_from_other_sources(tmp_path): (ws / ".deepcode").mkdir(parents=True, exist_ok=True) # list shape in a project hooks.json (non user-mcp source) must be rejected (ws / ".deepcode" / "hooks.json").write_text( - json.dumps({"hooks": [{"name": "h1", "event": "PreToolUse", "handler": "echo hi"}]}), + json.dumps( + {"hooks": [{"name": "h1", "event": "PreToolUse", "handler": "echo hi"}]} + ), encoding="utf-8", ) result = discover_hooks(str(ws), str(home)) @@ -340,8 +346,18 @@ def test_mcp_priority_ordering(tmp_path): tmp_path / "ws", { "hooks": [ - {"name": "low", "event": "PreToolUse", "handler": "echo low", "priority": 1}, - {"name": "high", "event": "PreToolUse", "handler": "echo high", "priority": 10}, + { + "name": "low", + "event": "PreToolUse", + "handler": "echo low", + "priority": 1, + }, + { + "name": "high", + "event": "PreToolUse", + "handler": "echo high", + "priority": 10, + }, ] }, ) @@ -357,9 +373,19 @@ def test_mcp_disabled_and_invalid_entries(tmp_path): tmp_path / "ws", { "hooks": [ - {"name": "disabled", "event": "PreToolUse", "handler": "echo x", "enabled": False}, + { + "name": "disabled", + "event": "PreToolUse", + "handler": "echo x", + "enabled": False, + }, {"name": "no-event", "handler": "echo x"}, - {"name": "bad-type", "event": "PreToolUse", "handler": "pass", "type": "python"}, + { + "name": "bad-type", + "event": "PreToolUse", + "handler": "pass", + "type": "python", + }, {"name": "ok", "event": "PreToolUse", "handler": "echo ok"}, ] }, @@ -374,7 +400,16 @@ def test_mcp_timeout_and_event_alias(tmp_path): ws, home = _write_config( tmp_path / "home", tmp_path / "ws", - {"hooks": [{"name": "aliased", "event": "sessionStart", "handler": "echo aliased", "timeout": 7}]}, + { + "hooks": [ + { + "name": "aliased", + "event": "sessionStart", + "handler": "echo aliased", + "timeout": 7, + } + ] + }, ) result = discover_hooks(ws, home) assert result.warnings == [] From 9299666d757208bc732a886f5f7ecf839b137d04 Mon Sep 17 00:00:00 2001 From: raymondginger Date: Tue, 11 Aug 2026 13:28:56 +0800 Subject: [PATCH 04/23] fix(hooks): Windows compat for hook commands, sandbox shell and test paths - execution._default_shell(): prefer a POSIX shell (Git Bash sh) on Windows so POSIX-syntax hook commands run; fall back to cmd.exe. - sandbox.build_exec_command(): resolve POSIX-style shell paths to a real executable on Windows (CreateProcessW cannot launch /bin/bash); job backend injects PYTHONPATH so the windows_sandbox wrapper can import core. - tools/shell.BashTool: pass wrapped.extra_env into the subprocess env so the injected PYTHONPATH reaches the sandbox wrapper. - tests/test_hooks.py: use capture.as_posix() in shell commands so WindowsPath backslashes are not escaped by sh. Local result: tests/test_hooks.py 53 passed; tests/test_agent_session.py 25 passed. --- core/harness/hooks/execution.py | 8 ++++++++ core/harness/sandbox.py | 27 ++++++++++++++++++++++++++- core/harness/tools/shell.py | 1 + tests/test_hooks.py | 10 +++++----- 4 files changed, 40 insertions(+), 6 deletions(-) diff --git a/core/harness/hooks/execution.py b/core/harness/hooks/execution.py index ff772a9d..ab3a95ec 100644 --- a/core/harness/hooks/execution.py +++ b/core/harness/hooks/execution.py @@ -19,6 +19,7 @@ import asyncio import json import os +import shutil import time from dataclasses import dataclass from typing import Any @@ -56,6 +57,13 @@ class HandlerDecision: def _default_shell() -> list[str]: if os.name == "nt": # pragma: no cover - posix CI + # Hook commands follow the Claude-Code POSIX shell contract (`;` + # separators, single-quoted JSON, `cat` redirection). Prefer a POSIX + # shell (e.g. Git Bash) on Windows so those commands actually run; + # fall back to cmd.exe only when no POSIX shell is available. + sh = shutil.which("sh") + if sh: + return [sh, "-lc"] comspec = os.environ.get("COMSPEC", "cmd.exe") return [comspec, "/C"] shell = os.environ.get("SHELL", "/bin/sh") diff --git a/core/harness/sandbox.py b/core/harness/sandbox.py index 5fdf38bd..ea2ab76b 100644 --- a/core/harness/sandbox.py +++ b/core/harness/sandbox.py @@ -44,6 +44,7 @@ import shutil import tempfile from dataclasses import dataclass, field +from pathlib import Path # Absolute path — never resolved via PATH (PATH-injection defense). _MACOS_SANDBOX_EXEC = "/usr/bin/sandbox-exec" @@ -322,8 +323,14 @@ def wrap_argv_command( # ``python -m core.harness.windows_sandbox -- `` creates # a KILL_ON_JOB_CLOSE job, spawns the inner command into it suspended, # and resumes it — the whole process tree dies with the wrapper. + # + # The wrapper runs as ``python -m core.harness.windows_sandbox``, so the + # child interpreter must be able to import ``core``. The BashTool cwd is + # the workspace (often a tmp dir) — not on sys.path — so inject the repo + # root through PYTHONPATH to keep the module importable. import sys as _sys + repo_root = str(Path(__file__).resolve().parents[2]) argv = [ _sys.executable, "-m", @@ -331,7 +338,11 @@ def wrap_argv_command( "--", *inner_argv, ] - return WrappedCommand(argv=argv, backend=backend) + return WrappedCommand( + argv=argv, + backend=backend, + extra_env={"PYTHONPATH": repo_root}, + ) return WrappedCommand(argv=list(inner_argv), backend="none") @@ -399,6 +410,20 @@ def build_exec_command( if (command is None) == (argv is None): raise ValueError("provide exactly one of command= or argv=") + # On Windows the wrapped command is launched by the Job Object sandbox + # (``CreateProcessW``) or, in the disabled path, directly by the executor — + # neither can resolve POSIX-style shell paths like ``/bin/bash``. Resolve a + # real executable path (e.g. Git Bash ``sh``) so the inner command starts; + # callers may still override ``shell`` with any Windows-resolvable value. + if command is not None and os.name == "nt": + import shutil + + resolved = shutil.which(shell) + if resolved is None and shell in ("/bin/bash", "/bin/sh", "bash", "sh"): + resolved = shutil.which("sh") + if resolved: + shell = resolved + # ``enabled`` is the immutable per-execution value used by product access # presets. ``None`` intentionally retains the legacy env/default behavior # for direct embedders that have not adopted ExecutionSecurityProfile yet. diff --git a/core/harness/tools/shell.py b/core/harness/tools/shell.py index 0f1ee1fd..b20ba345 100644 --- a/core/harness/tools/shell.py +++ b/core/harness/tools/shell.py @@ -105,6 +105,7 @@ async def execute(self, **kwargs: Any) -> Any: proc = await asyncio.create_subprocess_exec( *wrapped.argv, cwd=self._workspace, + env={**os.environ, **wrapped.extra_env}, stdout=asyncio.subprocess.PIPE, stderr=asyncio.subprocess.STDOUT, **subprocess_group_kwargs(), diff --git a/tests/test_hooks.py b/tests/test_hooks.py index e207d76b..276e2ab4 100644 --- a/tests/test_hooks.py +++ b/tests/test_hooks.py @@ -300,7 +300,7 @@ def test_stop_block_means_keep_going(): def test_payload_delivered_on_stdin(tmp_path): capture = tmp_path / "payload.json" - eng = _engine([_handler("PreToolUse", f"cat > {capture}", matcher="*")]) + eng = _engine([_handler("PreToolUse", f"cat > {capture.as_posix()}", matcher="*")]) asyncio.run(eng.run_pre_tool_use("Bash", {"command": "ls"}, tool_use_id="tu-9")) payload = json.loads(capture.read_text()) assert payload["session_id"] == "sess-1" @@ -567,7 +567,7 @@ def test_session_start_and_prompt_context_injected(): def test_subagent_start_payload_and_plaintext_context(tmp_path): capture = tmp_path / "p.json" - eng = _engine([_handler("SubagentStart", f"cat > {capture}; echo sub-context")]) + eng = _engine([_handler("SubagentStart", f"cat > {capture.as_posix()}; echo sub-context")]) res = asyncio.run(eng.run_subagent_start("worker-7", "subagent")) assert res.additional_contexts == ["sub-context"] # plain-text context works p = json.loads(capture.read_text()) @@ -802,7 +802,7 @@ async def ask(name, args): def test_pre_compact_hook_block_skips_and_payload(tmp_path): capture = tmp_path / "p.json" out = json.dumps({"continue": False}) - eng = _engine([_handler("PreCompact", f"cat > {capture}; echo '{out}'")]) + eng = _engine([_handler("PreCompact", f"cat > {capture.as_posix()}; echo '{out}'")]) res = asyncio.run(eng.run_pre_compact("auto")) assert res.block is True # continue:false → skip compaction p = json.loads(capture.read_text()) @@ -818,7 +818,7 @@ def test_pre_compact_matcher_matches_trigger(): def test_post_compact_hook_fires_with_trigger(tmp_path): capture = tmp_path / "p.json" - eng = _engine([_handler("PostCompact", f"cat > {capture}")]) + eng = _engine([_handler("PostCompact", f"cat > {capture.as_posix()}")]) asyncio.run(eng.run_post_compact("auto")) p = json.loads(capture.read_text()) assert p["hook_event_name"] == "PostCompact" and p["trigger"] == "auto" @@ -829,7 +829,7 @@ def test_post_compact_hook_fires_with_trigger(tmp_path): def test_stop_payload_carries_stop_hook_active(tmp_path): capture = tmp_path / "p.json" - eng = _engine([_handler("Stop", f"cat > {capture}")]) + eng = _engine([_handler("Stop", f"cat > {capture.as_posix()}")]) asyncio.run(eng.run_stop(stop_hook_active=True)) p = json.loads(capture.read_text()) assert p["hook_event_name"] == "Stop" and p["stop_hook_active"] is True From 9eb5f1b40edabe7b018e252d105f5bb355970a8b Mon Sep 17 00:00:00 2001 From: raymondginger Date: Tue, 11 Aug 2026 13:34:38 +0800 Subject: [PATCH 05/23] style: format test_hooks.py for ruff --- tests/test_hooks.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/tests/test_hooks.py b/tests/test_hooks.py index 276e2ab4..778923aa 100644 --- a/tests/test_hooks.py +++ b/tests/test_hooks.py @@ -567,7 +567,9 @@ def test_session_start_and_prompt_context_injected(): def test_subagent_start_payload_and_plaintext_context(tmp_path): capture = tmp_path / "p.json" - eng = _engine([_handler("SubagentStart", f"cat > {capture.as_posix()}; echo sub-context")]) + eng = _engine( + [_handler("SubagentStart", f"cat > {capture.as_posix()}; echo sub-context")] + ) res = asyncio.run(eng.run_subagent_start("worker-7", "subagent")) assert res.additional_contexts == ["sub-context"] # plain-text context works p = json.loads(capture.read_text()) From ab9e4f9947bd37eb97259f389daf5e1f61c61b4f Mon Sep 17 00:00:00 2001 From: raymondginger Date: Tue, 11 Aug 2026 13:46:27 +0800 Subject: [PATCH 06/23] fix(hooks): resolve sandbox shell only when enabled, keep disabled path bare The Windows shell-path resolution in build_exec_command() was applied to all paths, including the sandbox-disabled one. That broke the upstream-locked contract (test_disabled_via_env_returns_bare expects the bare '/bin/bash -c' argv when sandboxing is disabled). Move the resolution after the disabled early return so only the Job Object sandbox path (CreateProcessW) gets a real executable path, while the disabled path keeps the bare argv untouched. --- core/harness/sandbox.py | 25 +++++++++++++------------ 1 file changed, 13 insertions(+), 12 deletions(-) diff --git a/core/harness/sandbox.py b/core/harness/sandbox.py index ea2ab76b..67dc9a93 100644 --- a/core/harness/sandbox.py +++ b/core/harness/sandbox.py @@ -410,11 +410,20 @@ def build_exec_command( if (command is None) == (argv is None): raise ValueError("provide exactly one of command= or argv=") + # ``enabled`` is the immutable per-execution value used by product access + # presets. ``None`` intentionally retains the legacy env/default behavior + # for direct embedders that have not adopted ExecutionSecurityProfile yet. + effective_enabled = sandbox_enabled() if enabled is None else enabled + if not effective_enabled: + bare = [shell, "-c", command] if command is not None else list(argv or []) + return WrappedCommand(argv=bare, backend="disabled") + # On Windows the wrapped command is launched by the Job Object sandbox - # (``CreateProcessW``) or, in the disabled path, directly by the executor — - # neither can resolve POSIX-style shell paths like ``/bin/bash``. Resolve a - # real executable path (e.g. Git Bash ``sh``) so the inner command starts; - # callers may still override ``shell`` with any Windows-resolvable value. + # (``CreateProcessW``), which cannot resolve POSIX-style shell paths like + # ``/bin/bash``. Resolve a real executable path (e.g. Git Bash ``sh``) so + # the inner command starts; callers may still override ``shell`` with any + # Windows-resolvable value. The disabled path above keeps the bare argv + # untouched (upstream-locked contract). if command is not None and os.name == "nt": import shutil @@ -424,14 +433,6 @@ def build_exec_command( if resolved: shell = resolved - # ``enabled`` is the immutable per-execution value used by product access - # presets. ``None`` intentionally retains the legacy env/default behavior - # for direct embedders that have not adopted ExecutionSecurityProfile yet. - effective_enabled = sandbox_enabled() if enabled is None else enabled - if not effective_enabled: - bare = [shell, "-c", command] if command is not None else list(argv or []) - return WrappedCommand(argv=bare, backend="disabled") - policy = SandboxPolicy.for_workspace(workspace, allow_network=allow_network) if command is not None: return wrap_shell_command(command, policy, shell=shell) From 23fc69016e3ccd7961812d0323d6d8bc96cc4915 Mon Sep 17 00:00:00 2001 From: raymondginger Date: Sun, 23 Aug 2026 07:31:06 +0800 Subject: [PATCH 07/23] fix(tests): adapt session-end lifecycle tests to C4a runner API The merged upstream main renamed the prompt-size estimator (estimate_prompt_tokens_chain -> _estimate_prompt instance method) and added the dsh convergence rule that rejects summaries which do not shrink the history. Update the three _maybe_compact tests: - patch runner._estimate_prompt (returns a plain int) instead of the removed module-level function - enlarge the sample turns so the compacted history + summary prefix is genuinely smaller than the source, satisfying the shrink gate --- tests/test_session_end_lifecycle.py | 33 +++++++++++------------------ 1 file changed, 12 insertions(+), 21 deletions(-) diff --git a/tests/test_session_end_lifecycle.py b/tests/test_session_end_lifecycle.py index adbe1959..10887dbb 100644 --- a/tests/test_session_end_lifecycle.py +++ b/tests/test_session_end_lifecycle.py @@ -182,10 +182,7 @@ def test_maybe_compact_checkpoint_injected_after_success(monkeypatch): from core.agent_runtime.runner import AgentRunSpec, AgentRunner runner = AgentRunner(provider=object()) - monkeypatch.setattr( - "core.agent_runtime.runner.estimate_prompt_tokens_chain", - lambda *args, **kwargs: (999_999, None), - ) + monkeypatch.setattr(runner, "_estimate_prompt", lambda spec, messages: 999_999) async def fake_summarize(spec, messages, *, response_observer=None): return "handoff summary" @@ -205,11 +202,11 @@ async def pre_compact_hook(trigger): pre_compact_hook=pre_compact_hook, ) messages = [ - {"role": "user", "content": "turn 1"}, + {"role": "user", "content": "turn 1 " + "context " * 50}, {"role": "assistant", "content": "a1"}, - {"role": "user", "content": "turn 2"}, + {"role": "user", "content": "turn 2 " + "context " * 50}, {"role": "assistant", "content": "a2"}, - {"role": "user", "content": "turn 3"}, + {"role": "user", "content": "turn 3 " + "context " * 50}, ] compacted = asyncio.run(runner._maybe_compact(spec, messages)) checkpoint_msgs = [ @@ -227,10 +224,7 @@ def test_maybe_compact_block_skips_checkpoint(monkeypatch): from core.agent_runtime.runner import AgentRunSpec, AgentRunner runner = AgentRunner(provider=object()) - monkeypatch.setattr( - "core.agent_runtime.runner.estimate_prompt_tokens_chain", - lambda *args, **kwargs: (999_999, None), - ) + monkeypatch.setattr(runner, "_estimate_prompt", lambda spec, messages: 999_999) async def pre_compact_hook(trigger): return SimpleNamespace(block=True, additional_contexts=["should not appear"]) @@ -245,11 +239,11 @@ async def pre_compact_hook(trigger): pre_compact_hook=pre_compact_hook, ) messages = [ - {"role": "user", "content": "turn 1"}, + {"role": "user", "content": "turn 1 " + "context " * 50}, {"role": "assistant", "content": "a1"}, - {"role": "user", "content": "turn 2"}, + {"role": "user", "content": "turn 2 " + "context " * 50}, {"role": "assistant", "content": "a2"}, - {"role": "user", "content": "turn 3"}, + {"role": "user", "content": "turn 3 " + "context " * 50}, ] compacted = asyncio.run(runner._maybe_compact(spec, messages)) assert compacted is messages # compaction aborted this turn @@ -262,10 +256,7 @@ def test_maybe_compact_summarize_failure_no_checkpoint(monkeypatch): from core.agent_runtime.runner import AgentRunSpec, AgentRunner runner = AgentRunner(provider=object()) - monkeypatch.setattr( - "core.agent_runtime.runner.estimate_prompt_tokens_chain", - lambda *args, **kwargs: (999_999, None), - ) + monkeypatch.setattr(runner, "_estimate_prompt", lambda spec, messages: 999_999) async def fake_summarize_fails(spec, messages, *, response_observer=None): return None # summarization failed @@ -285,11 +276,11 @@ async def pre_compact_hook(trigger): pre_compact_hook=pre_compact_hook, ) messages = [ - {"role": "user", "content": "turn 1"}, + {"role": "user", "content": "turn 1 " + "context " * 50}, {"role": "assistant", "content": "a1"}, - {"role": "user", "content": "turn 2"}, + {"role": "user", "content": "turn 2 " + "context " * 50}, {"role": "assistant", "content": "a2"}, - {"role": "user", "content": "turn 3"}, + {"role": "user", "content": "turn 3 " + "context " * 50}, ] compacted = asyncio.run(runner._maybe_compact(spec, messages)) assert compacted is messages From d06d6951b401d4b02c0741e6e5997c2f969d2cc2 Mon Sep 17 00:00:00 2001 From: raymondginger Date: Sun, 23 Aug 2026 14:12:56 +0800 Subject: [PATCH 08/23] fix(security): bump sidecar pip 26.1.2 -> 26.2 (PYSEC-2026-3721) Security CI's 'Audit locked App Server environment' step fails on every branch because the sidecar lock pins pip==26.1.2, which PYSEC-2026-3721 (healchecks pip < 26.2) now flags. pip publishes no wheels-only constraint here, so bump the universal lock entry to the fixed 26.2 release. --- desktop/sidecar-requirements.lock | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/desktop/sidecar-requirements.lock b/desktop/sidecar-requirements.lock index 8e5e0ef9..5e0caa59 100644 --- a/desktop/sidecar-requirements.lock +++ b/desktop/sidecar-requirements.lock @@ -105,7 +105,7 @@ packaging==26.2 # pyinstaller-hooks-contrib pefile==2024.8.26 ; sys_platform == 'win32' # via pyinstaller -pip==26.1.2 +pip==26.2 # via -r sidecar-requirements.in prompt-toolkit==3.0.52 # via -r sidecar-requirements.in From 919d6c2d3b17931e4a4b59d28030e15f5953d1fd Mon Sep 17 00:00:00 2001 From: Zongwei9888 Date: Sun, 23 Aug 2026 17:14:54 +0800 Subject: [PATCH 09/23] fix(desktop): keep sidecar pip source pin in sync --- desktop/sidecar-requirements.in | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/desktop/sidecar-requirements.in b/desktop/sidecar-requirements.in index d4605f06..a9ca2641 100644 --- a/desktop/sidecar-requirements.in +++ b/desktop/sidecar-requirements.in @@ -17,6 +17,6 @@ PyYAML==6.0.3 tiktoken==0.12.0 # Packaging toolchain. -pip==26.1.2 +pip==26.2 pyinstaller==6.21.0 pyinstaller-hooks-contrib==2026.6 From 14b227a9f0e0aeb8129339bb2ef4fc045abb88c6 Mon Sep 17 00:00:00 2001 From: raymondginger Date: Wed, 19 Aug 2026 22:09:23 +0800 Subject: [PATCH 10/23] =?UTF-8?q?feat(core):=20Claude=20Code=20lessons=20?= =?UTF-8?q?=E2=80=94=20instruction=20file=20exclusion=20+=20explicit=20dan?= =?UTF-8?q?gerous=20preset?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 借鉴 Claude Code 两处机制 (2026-08-19): 1. core/harness/memory.py — 指令文件排除模式 - DEEPCODE_INSTRUCTION_EXCLUDES env: 逗号分隔 glob (如 **/code/CLAUDE.md,**/vendor/**) - 命中的 AGENTS.md/DEEPCODE.md/CLAUDE.md 跳过注入, 避免 monorepo 子目录/ 第三方代码指令污染主提示词 - 自实现 glob→regex: ** 匹配任意层级(可选前缀 (?:.*/)?), * / ? 不跨路径 分隔符; vendorized ≠ vendor/ 前缀同名不误匹配 - 非法模式忽略不阻断加载 2. core/domain/execution_security.py — 显式危险预设 - ExecutionAccessPreset.DANGEROUS_SKIP="dangerous_skip", 对齐 Claude Code --allow-dangerously-skip-permissions; 与 FULL_ACCESS 同强度但名字自带 危险警示, 供日志/UI 明确区分 3. tests/test_memory.py — 4 个新用例 (glob 正反例/集成/非法模式) --- core/domain/execution_security.py | 18 ++++++++- core/harness/memory.py | 63 ++++++++++++++++++++++++++++++- tests/test_memory.py | 34 +++++++++++++++++ 3 files changed, 113 insertions(+), 2 deletions(-) diff --git a/core/domain/execution_security.py b/core/domain/execution_security.py index c6f23a6b..10f8b4c3 100644 --- a/core/domain/execution_security.py +++ b/core/domain/execution_security.py @@ -11,11 +11,18 @@ class ExecutionAccessPreset(StrEnum): - """User-facing access choices shared by every DeepCode client.""" + """User-facing access choices shared by every DeepCode client. + + 借鉴 Claude Code ``--allow-dangerously-skip-permissions`` (2026-08-19): + 危险操作必须"显式命名危险" —— 跳过全部权限校验的逃生通道叫 + ``dangerous_skip`` 而不是沉默的 full_access, 让使用者与审计日志 + 都能一眼看到这是危险选择。 + """ ASK = "ask" READ_ONLY = "read_only" FULL_ACCESS = "full_access" + DANGEROUS_SKIP = "dangerous_skip" class FilesystemScope(StrEnum): @@ -178,6 +185,15 @@ def _pattern_specificity(pattern: str) -> int: FilesystemScope.UNRESTRICTED, ApprovalPolicy.NEVER, ), + # 危险逃生通道: 显式命名 (借鉴 Claude Code --allow-dangerously-skip-permissions)。 + # 与 FULL_ACCESS 同强度, 但名字自带"危险"警示, 供日志/UI 明确区分; + # 选择它等于明确声明"我知道这很危险, 仍要跳过全部权限校验"。 + ExecutionAccessPreset.DANGEROUS_SKIP: ( + ExecutionPermissionMode.FULL_AUTO, + False, + FilesystemScope.UNRESTRICTED, + ApprovalPolicy.NEVER, + ), } diff --git a/core/harness/memory.py b/core/harness/memory.py index 64a4fd55..53a5d7e6 100644 --- a/core/harness/memory.py +++ b/core/harness/memory.py @@ -23,6 +23,8 @@ from __future__ import annotations +import os +import re from pathlib import Path from typing import Any @@ -41,6 +43,65 @@ _REMINDER_OPEN = "" _REMINDER_CLOSE = "" _REMINDER_CLOSE_ESCAPED = "</system-reminder>" +# 借鉴 Claude Code 的 CLAUDE.md 排除模式 (ignore 配置): 逗号分隔的 glob 模式 +# (如 "**/code/CLAUDE.md,**/vendor/**")。命中的指令文件跳过不注入 —— 避免 +# monorepo 子目录/第三方代码的指令污染主提示词。 +_INSTRUCTION_EXCLUDE_ENV = "DEEPCODE_INSTRUCTION_EXCLUDES" +_EXCLUDE_RE_CACHE: dict[str, Any] = {} # pattern -> compiled regex + + +def _glob_to_re(pattern: str): + """glob → regex: ** 匹配任意层级 (含零层), * / ? 不跨路径分隔符。""" + compiled = _EXCLUDE_RE_CACHE.get(pattern) + if compiled is not None: + return compiled + parts = [] + i, n = 0, len(pattern) + while i < n: + c = pattern[i] + if c == "*": + if i + 1 < n and pattern[i + 1] == "*": + if i + 2 < n and pattern[i + 2] in "/\\": + # **/ → (?:.*/)? : 任意层级前缀(可选)。不能用 .* —— 那会让 + # "**/vendor/**" 误匹配 "vendorized/..." 这类前缀同名的路径。 + parts.append(r"(?:.*/)?") + i += 3 + else: + # 尾部 ** → 任意剩余(含层级) + parts.append(".*") + i += 2 + else: + parts.append(r"[^/\\]*") + i += 1 + elif c == "?": + parts.append(r"[^/\\]") + i += 1 + else: + parts.append(re.escape(c)) + i += 1 + compiled = re.compile("^" + "".join(parts) + "$", re.IGNORECASE) + _EXCLUDE_RE_CACHE[pattern] = compiled + return compiled + + +def _instruction_excluded(candidate: Path) -> bool: + """Whether the candidate instruction file is excluded by pattern. + + 逗号分隔 glob, 如 ``**/code/CLAUDE.md,**/vendor/**``; 用正斜杠规范化 + 路径后匹配, 兼容 Windows 反斜杠路径。非法模式被忽略, 不阻断加载。 + """ + patterns = [p.strip() for p in + os.environ.get(_INSTRUCTION_EXCLUDE_ENV, "").split(",") if p.strip()] + if not patterns: + return False + cand = str(candidate).replace("\\", "/") + for pat in patterns: + try: + if _glob_to_re(pat.replace("\\", "/")).match(cand): + return True + except (re.error, ValueError): + continue # 非法 glob 模式忽略, 不阻断加载 + return False def memory_dir(workspace: str | Path) -> Path: @@ -135,7 +196,7 @@ def project_instructions(workspace: str | Path) -> str: for directory in search_dirs: for name in _PROJECT_FILES: candidate = directory / name - if candidate.is_file(): + if candidate.is_file() and not _instruction_excluded(candidate): try: body = candidate.read_text( encoding="utf-8", errors="replace" diff --git a/tests/test_memory.py b/tests/test_memory.py index c1562ee1..3ed0ff34 100644 --- a/tests/test_memory.py +++ b/tests/test_memory.py @@ -11,8 +11,10 @@ sys.path.insert(0, str(ROOT)) from core.harness.memory import ( # noqa: E402 + _INSTRUCTION_EXCLUDE_ENV, _MAX_INJECT_CHARS, MemoryTool, + _instruction_excluded, memory_dir, project_instructions, system_preamble, @@ -36,6 +38,38 @@ def test_project_instructions_prefers_agents_md(tmp_path): assert "spaces" not in out # AGENTS.md wins over CLAUDE.md +def test_instruction_excluded_matches_globs(monkeypatch): + monkeypatch.setenv( + _INSTRUCTION_EXCLUDE_ENV, "**/code/CLAUDE.md,**/vendor/**" + ) + # 匹配: 任意层级前缀 + 目录段精确匹配 + assert _instruction_excluded(Path("repo/code/CLAUDE.md")) + assert _instruction_excluded(Path("repo/vendor/x/AGENTS.md")) + assert _instruction_excluded(Path("repo/vendor/AGENTS.md")) + assert _instruction_excluded(Path("code/CLAUDE.md")) # 零层前缀 + # 反例: 前缀同名目录不误匹配 (vendorized ≠ vendor/) + assert not _instruction_excluded(Path("repo/CLAUDE.md")) + assert not _instruction_excluded(Path("repo/vendorized/AGENTS.md")) + assert not _instruction_excluded(Path("repo/vendorized/x/CLAUDE.md")) + + +def test_project_instructions_skips_excluded_file(tmp_path, monkeypatch): + repo = tmp_path / "repo" + (repo / ".git").mkdir(parents=True) + (repo / "code").mkdir() + (repo / "CLAUDE.md").write_text("root instructions") + (repo / "code" / "CLAUDE.md").write_text("subdir instructions") + monkeypatch.setenv(_INSTRUCTION_EXCLUDE_ENV, "**/code/CLAUDE.md") + out = project_instructions(repo / "code") + assert "root instructions" in out + assert "subdir instructions" not in out + + +def test_instruction_excluded_ignores_invalid_patterns(monkeypatch): + monkeypatch.setenv(_INSTRUCTION_EXCLUDE_ENV, "**/[code/CLAUDE.md") # 非法 glob + assert not _instruction_excluded(Path("code/CLAUDE.md")) + + def test_project_instructions_absent(tmp_path): assert project_instructions(tmp_path) == "" From 2d892c9918f4526d0a55daeebfba154b7569b267 Mon Sep 17 00:00:00 2001 From: raymondginger Date: Sun, 23 Aug 2026 06:16:12 +0800 Subject: [PATCH 11/23] style: ruff format instruction exclusion code + test (CI lint fix) --- core/harness/memory.py | 7 +++++-- tests/test_memory.py | 6 ++---- 2 files changed, 7 insertions(+), 6 deletions(-) diff --git a/core/harness/memory.py b/core/harness/memory.py index 53a5d7e6..0bce8c21 100644 --- a/core/harness/memory.py +++ b/core/harness/memory.py @@ -90,8 +90,11 @@ def _instruction_excluded(candidate: Path) -> bool: 逗号分隔 glob, 如 ``**/code/CLAUDE.md,**/vendor/**``; 用正斜杠规范化 路径后匹配, 兼容 Windows 反斜杠路径。非法模式被忽略, 不阻断加载。 """ - patterns = [p.strip() for p in - os.environ.get(_INSTRUCTION_EXCLUDE_ENV, "").split(",") if p.strip()] + patterns = [ + p.strip() + for p in os.environ.get(_INSTRUCTION_EXCLUDE_ENV, "").split(",") + if p.strip() + ] if not patterns: return False cand = str(candidate).replace("\\", "/") diff --git a/tests/test_memory.py b/tests/test_memory.py index 3ed0ff34..7552e265 100644 --- a/tests/test_memory.py +++ b/tests/test_memory.py @@ -10,7 +10,7 @@ if str(ROOT) not in sys.path: sys.path.insert(0, str(ROOT)) -from core.harness.memory import ( # noqa: E402 +from core.harness.memory import ( _INSTRUCTION_EXCLUDE_ENV, _MAX_INJECT_CHARS, MemoryTool, @@ -39,9 +39,7 @@ def test_project_instructions_prefers_agents_md(tmp_path): def test_instruction_excluded_matches_globs(monkeypatch): - monkeypatch.setenv( - _INSTRUCTION_EXCLUDE_ENV, "**/code/CLAUDE.md,**/vendor/**" - ) + monkeypatch.setenv(_INSTRUCTION_EXCLUDE_ENV, "**/code/CLAUDE.md,**/vendor/**") # 匹配: 任意层级前缀 + 目录段精确匹配 assert _instruction_excluded(Path("repo/code/CLAUDE.md")) assert _instruction_excluded(Path("repo/vendor/x/AGENTS.md")) From 597a9bf02023c36bf07c1a3bba75f80b844e8419 Mon Sep 17 00:00:00 2001 From: Zongwei9888 Date: Sun, 23 Aug 2026 17:24:55 +0800 Subject: [PATCH 12/23] fix(memory): make instruction exclusions scoped and predictable --- core/domain/execution_security.py | 18 +---------- core/harness/memory.py | 51 +++++++++++++++++-------------- tests/test_memory.py | 16 ++++++++-- 3 files changed, 43 insertions(+), 42 deletions(-) diff --git a/core/domain/execution_security.py b/core/domain/execution_security.py index 10f8b4c3..c6f23a6b 100644 --- a/core/domain/execution_security.py +++ b/core/domain/execution_security.py @@ -11,18 +11,11 @@ class ExecutionAccessPreset(StrEnum): - """User-facing access choices shared by every DeepCode client. - - 借鉴 Claude Code ``--allow-dangerously-skip-permissions`` (2026-08-19): - 危险操作必须"显式命名危险" —— 跳过全部权限校验的逃生通道叫 - ``dangerous_skip`` 而不是沉默的 full_access, 让使用者与审计日志 - 都能一眼看到这是危险选择。 - """ + """User-facing access choices shared by every DeepCode client.""" ASK = "ask" READ_ONLY = "read_only" FULL_ACCESS = "full_access" - DANGEROUS_SKIP = "dangerous_skip" class FilesystemScope(StrEnum): @@ -185,15 +178,6 @@ def _pattern_specificity(pattern: str) -> int: FilesystemScope.UNRESTRICTED, ApprovalPolicy.NEVER, ), - # 危险逃生通道: 显式命名 (借鉴 Claude Code --allow-dangerously-skip-permissions)。 - # 与 FULL_ACCESS 同强度, 但名字自带"危险"警示, 供日志/UI 明确区分; - # 选择它等于明确声明"我知道这很危险, 仍要跳过全部权限校验"。 - ExecutionAccessPreset.DANGEROUS_SKIP: ( - ExecutionPermissionMode.FULL_AUTO, - False, - FilesystemScope.UNRESTRICTED, - ApprovalPolicy.NEVER, - ), } diff --git a/core/harness/memory.py b/core/harness/memory.py index 0bce8c21..144c2f15 100644 --- a/core/harness/memory.py +++ b/core/harness/memory.py @@ -25,6 +25,7 @@ import os import re +from functools import lru_cache from pathlib import Path from typing import Any @@ -43,18 +44,15 @@ _REMINDER_OPEN = "" _REMINDER_CLOSE = "" _REMINDER_CLOSE_ESCAPED = "</system-reminder>" -# 借鉴 Claude Code 的 CLAUDE.md 排除模式 (ignore 配置): 逗号分隔的 glob 模式 -# (如 "**/code/CLAUDE.md,**/vendor/**")。命中的指令文件跳过不注入 —— 避免 -# monorepo 子目录/第三方代码的指令污染主提示词。 +# Comma-separated glob patterns for instruction files that must not be loaded, +# for example ``code/CLAUDE.md,**/vendor/**``. _INSTRUCTION_EXCLUDE_ENV = "DEEPCODE_INSTRUCTION_EXCLUDES" -_EXCLUDE_RE_CACHE: dict[str, Any] = {} # pattern -> compiled regex -def _glob_to_re(pattern: str): - """glob → regex: ** 匹配任意层级 (含零层), * / ? 不跨路径分隔符。""" - compiled = _EXCLUDE_RE_CACHE.get(pattern) - if compiled is not None: - return compiled +@lru_cache(maxsize=256) +def _glob_to_re(pattern: str) -> re.Pattern[str]: + """Compile a path glob where ``**`` crosses directory boundaries.""" + parts = [] i, n = 0, len(pattern) while i < n: @@ -62,12 +60,9 @@ def _glob_to_re(pattern: str): if c == "*": if i + 1 < n and pattern[i + 1] == "*": if i + 2 < n and pattern[i + 2] in "/\\": - # **/ → (?:.*/)? : 任意层级前缀(可选)。不能用 .* —— 那会让 - # "**/vendor/**" 误匹配 "vendorized/..." 这类前缀同名的路径。 parts.append(r"(?:.*/)?") i += 3 else: - # 尾部 ** → 任意剩余(含层级) parts.append(".*") i += 2 else: @@ -79,16 +74,16 @@ def _glob_to_re(pattern: str): else: parts.append(re.escape(c)) i += 1 - compiled = re.compile("^" + "".join(parts) + "$", re.IGNORECASE) - _EXCLUDE_RE_CACHE[pattern] = compiled - return compiled + flags = re.IGNORECASE if os.name == "nt" else 0 + return re.compile("^" + "".join(parts) + "$", flags) -def _instruction_excluded(candidate: Path) -> bool: +def _instruction_excluded(candidate: Path, *, root: Path | None = None) -> bool: """Whether the candidate instruction file is excluded by pattern. - 逗号分隔 glob, 如 ``**/code/CLAUDE.md,**/vendor/**``; 用正斜杠规范化 - 路径后匹配, 兼容 Windows 反斜杠路径。非法模式被忽略, 不阻断加载。 + Patterns containing a separator match both the absolute path and, when + available, the path relative to the repository root. A bare filename such + as ``CLAUDE.md`` matches that filename at any searched level. """ patterns = [ p.strip() @@ -97,13 +92,20 @@ def _instruction_excluded(candidate: Path) -> bool: ] if not patterns: return False - cand = str(candidate).replace("\\", "/") + candidates = {str(candidate).replace("\\", "/"), candidate.name} + if root is not None: + try: + candidates.add(candidate.relative_to(root).as_posix()) + except ValueError: + pass for pat in patterns: + normalized = pat.replace("\\", "/") try: - if _glob_to_re(pat.replace("\\", "/")).match(cand): + compiled = _glob_to_re(normalized) + if any(compiled.fullmatch(value) for value in candidates): return True - except (re.error, ValueError): - continue # 非法 glob 模式忽略, 不阻断加载 + except re.error: + continue return False @@ -199,7 +201,10 @@ def project_instructions(workspace: str | Path) -> str: for directory in search_dirs: for name in _PROJECT_FILES: candidate = directory / name - if candidate.is_file() and not _instruction_excluded(candidate): + if candidate.is_file() and not _instruction_excluded( + candidate, + root=root or workspace, + ): try: body = candidate.read_text( encoding="utf-8", errors="replace" diff --git a/tests/test_memory.py b/tests/test_memory.py index 7552e265..fd114374 100644 --- a/tests/test_memory.py +++ b/tests/test_memory.py @@ -51,6 +51,18 @@ def test_instruction_excluded_matches_globs(monkeypatch): assert not _instruction_excluded(Path("repo/vendorized/x/CLAUDE.md")) +def test_instruction_excluded_matches_repo_relative_and_bare_names( + tmp_path, monkeypatch +): + repo = tmp_path / "repo" + candidate = repo / "code" / "CLAUDE.md" + monkeypatch.setenv(_INSTRUCTION_EXCLUDE_ENV, "code/CLAUDE.md") + assert _instruction_excluded(candidate, root=repo) + + monkeypatch.setenv(_INSTRUCTION_EXCLUDE_ENV, "CLAUDE.md") + assert _instruction_excluded(candidate, root=repo) + + def test_project_instructions_skips_excluded_file(tmp_path, monkeypatch): repo = tmp_path / "repo" (repo / ".git").mkdir(parents=True) @@ -63,8 +75,8 @@ def test_project_instructions_skips_excluded_file(tmp_path, monkeypatch): assert "subdir instructions" not in out -def test_instruction_excluded_ignores_invalid_patterns(monkeypatch): - monkeypatch.setenv(_INSTRUCTION_EXCLUDE_ENV, "**/[code/CLAUDE.md") # 非法 glob +def test_instruction_excluded_treats_regex_metacharacters_literally(monkeypatch): + monkeypatch.setenv(_INSTRUCTION_EXCLUDE_ENV, "**/[code/CLAUDE.md") assert not _instruction_excluded(Path("code/CLAUDE.md")) From d7bae1a9f6be168e9c27e4ed30a5fcc79ab7121a Mon Sep 17 00:00:00 2001 From: Zongwei9888 Date: Sun, 23 Aug 2026 17:34:42 +0800 Subject: [PATCH 13/23] fix(hooks): align SessionEnd teardown and bound checkpoints --- core/agent_runtime/runner.py | 31 +++- core/events/session.py | 33 +++-- core/harness/hooks/discovery.py | 153 +++----------------- core/harness/hooks/engine.py | 9 +- core/harness/hooks/events.py | 8 +- core/harness/hooks/execution.py | 8 -- core/harness/sandbox.py | 28 +--- core/harness/tools/shell.py | 1 - tests/test_hooks.py | 12 +- tests/test_session_end_lifecycle.py | 214 ++++++++++------------------ 10 files changed, 160 insertions(+), 337 deletions(-) diff --git a/core/agent_runtime/runner.py b/core/agent_runtime/runner.py index 06ca09be..093285ef 100644 --- a/core/agent_runtime/runner.py +++ b/core/agent_runtime/runner.py @@ -99,14 +99,21 @@ _PRECOMPACT_TOTAL_LIMIT = 8000 # chars for the whole checkpoint block -def _build_precompact_checkpoint(contexts: list[str]) -> str | None: +def _build_precompact_checkpoint( + contexts: list[str], + *, + total_limit: int = _PRECOMPACT_TOTAL_LIMIT, +) -> str | None: """Bounded, delimited representation of PreCompact hook context. Each context is stripped, truncated to ``_PRECOMPACT_CONTEXT_LIMIT`` chars and the combined block capped at ``_PRECOMPACT_TOTAL_LIMIT``. Returns ``None`` when nothing survives (empty input or all contexts blank). """ - if not contexts: + prefix = _PRECOMPACT_CHECKPOINT_PREFIX + "\n" + total_limit = min(max(total_limit, 0), _PRECOMPACT_TOTAL_LIMIT) + content_limit = total_limit - len(prefix) + if not contexts or content_limit <= 0: return None parts: list[str] = [] used = 0 @@ -115,14 +122,14 @@ def _build_precompact_checkpoint(contexts: list[str]) -> str | None: if not text: continue text = text[:_PRECOMPACT_CONTEXT_LIMIT] - room = _PRECOMPACT_TOTAL_LIMIT - used + room = content_limit - used if room <= 0: break parts.append(text[:room]) used += min(len(text), room) + 1 # +1 for the newline separator if not parts: return None - return _PRECOMPACT_CHECKPOINT_PREFIX + "\n" + "\n".join(parts) + return prefix + "\n".join(parts) @dataclass(slots=True) @@ -1767,9 +1774,21 @@ async def _maybe_compact( # blow the post-compaction window back open. When the hook blocks or # summarization fails we returned above — the checkpoint only ever # appears after a successful compaction. - checkpoint = _build_precompact_checkpoint(pre_contexts) + # A checkpoint must not turn successful compaction back into growth. + # Bound it to the actual character reduction in addition to the + # absolute hook-context cap. + checkpoint_room = ( + self._history_chars(messages) - self._history_chars(compacted) - 1 + ) + checkpoint = _build_precompact_checkpoint( + pre_contexts, + total_limit=checkpoint_room, + ) if checkpoint: - compacted = compacted + [{"role": "user", "content": checkpoint}] + self._append_injected_messages( + compacted, + [{"role": "user", "content": checkpoint}], + ) self._refused_compaction = None if spec.post_compact_hook is not None: await self._call_tool_hook(spec.post_compact_hook, "auto") diff --git a/core/events/session.py b/core/events/session.py index ceba7ef5..37a91663 100644 --- a/core/events/session.py +++ b/core/events/session.py @@ -418,6 +418,8 @@ def __init__( # no hooks are configured, so the whole feature is dormant at zero cost. self._hooks_engine = hooks_engine self._session_started = False + self._session_end_fired = False + self._session_end_lock = asyncio.Lock() # When this session is a spawned sub-agent, (agent_id, agent_type) — its # lifecycle fires SubagentStart/SubagentStop instead of SessionStart/Stop. self._agent_context = agent_context @@ -625,7 +627,7 @@ async def submit(self, op: Op) -> None: # SessionEnd fires exactly once, here — at the real session # termination boundary — never per turn. Per-turn notifications # are the Stop event's job (see _EVENTS_WITHOUT_MATCHER). - await self._run_end_hook(reason="shutdown") + await self._run_end_hook(reason="other") self._emit(ShutdownComplete()) else: # pragma: no cover - exhaustive guard self._emit(ErrorEvent(message=f"unknown op: {op!r}")) @@ -645,6 +647,7 @@ async def aclose(self) -> None: if task.cancelling() == 0: task.cancel() await asyncio.gather(task, return_exceptions=True) + await self._run_end_hook(reason="other") control = getattr(self, "_agent_control", None) if control is not None: await control.close() @@ -694,21 +697,29 @@ async def _run_start_hook(self): logger.exception("start hook failed") return None - async def _run_end_hook(self, reason: str = "shutdown") -> None: + async def _run_end_hook(self, reason: str = "other") -> None: """Run SessionEnd hooks when the session itself terminates. Notification-only: a failure is logged and never crashes the - shutdown. Fired exactly once per session from ``submit(Shutdown)`` - with a documented session-exit reason (``shutdown``); per-turn + shutdown. Fired exactly once from ``submit(Shutdown)`` or the actual + resource teardown path, whichever comes first. ``other`` is the + compatible exit reason for a DeepCode runtime shutdown; per-turn notifications belong to the Stop event, not SessionEnd. """ - engine = self._hooks_engine - if engine is None or not engine.has_event("SessionEnd"): - return - try: - await engine.run_session_end(reason=reason) - except Exception: # noqa: BLE001 - hooks never crash a shutdown - logger.exception("session end hook failed") + async with self._session_end_lock: + if self._session_end_fired: + return + self._session_end_fired = True + # Spawned agents use their dedicated SubagentStop lifecycle. + if self._agent_context is not None: + return + engine = self._hooks_engine + if engine is None or not engine.has_event("SessionEnd"): + return + try: + await engine.run_session_end(reason=reason) + except Exception: # noqa: BLE001 - hooks never crash a shutdown + logger.exception("session end hook failed") async def _run_prompt_hooks( self, text: str, hook_contexts: list[str] diff --git a/core/harness/hooks/discovery.py b/core/harness/hooks/discovery.py index c9a0a605..0ef40663 100644 --- a/core/harness/hooks/discovery.py +++ b/core/harness/hooks/discovery.py @@ -18,9 +18,8 @@ fold order when several hooks fire for one event — is stable and deterministic: 1. user ``~/.deepcode/hooks.json`` - 2. user-mcp ``~/.deepcode/hooks_config.json`` (deepcode-hooks MCP list format) - 3. project ``/.deepcode/hooks.json`` - 4. project ``/.claude/settings.json`` (Claude-Code-compatible) + 2. project ``/.deepcode/hooks.json`` + 3. project ``/.claude/settings.json`` (Claude-Code-compatible) Only ``type: command`` handlers are supported; ``prompt`` / ``agent`` handlers and ``async: true`` are skipped with a warning (the reference does the same). @@ -39,22 +38,8 @@ ) _DEFAULT_TIMEOUT_SEC = 600 - -# deepcode-hooks MCP stores camelCase event names; core uses the reference -# agent's PascalCase names. Keys are matched case-insensitively via .lower(). -_MCP_EVENT_ALIASES: dict[str, str] = { - "sessionstart": "SessionStart", - "sessionend": "SessionEnd", - "pretooluse": "PreToolUse", - "posttooluse": "PostToolUse", - "userpromptsubmit": "UserPromptSubmit", - "permissionrequest": "PermissionRequest", - "precompact": "PreCompact", - "postcompact": "PostCompact", - "subagentstart": "SubagentStart", - "subagentstop": "SubagentStop", - "stop": "Stop", -} +_SESSION_END_DEFAULT_TIMEOUT_SEC = 2 +_SESSION_END_MAX_TIMEOUT_SEC = 60 @dataclass(slots=True) @@ -65,7 +50,7 @@ class Handler: matcher: str | None command: str timeout_sec: int - source: str # "user" | "user-mcp" | "project" — for reporting only + source: str # "user" | "project" — for reporting only source_path: str display_order: int status_message: str | None = None @@ -84,7 +69,6 @@ def _hook_source_files(workspace: str, home: str | None) -> list[tuple[Path, str ws = Path(workspace) return [ (home_dir / ".deepcode" / "hooks.json", "user"), - (home_dir / ".deepcode" / "hooks_config.json", "user-mcp"), (ws / ".deepcode" / "hooks.json", "project"), (ws / ".claude" / "settings.json", "project"), ] @@ -101,7 +85,7 @@ def discover_hooks(workspace: str, home: str | None = None) -> DiscoveryResult: warnings: list[str] = [] order = 0 for path, source in _hook_source_files(workspace, home): - events = _load_hook_events(path, warnings, source) + events = _load_hook_events(path, warnings) if not events: continue for event_name, groups in events.items(): @@ -114,17 +98,8 @@ def discover_hooks(workspace: str, home: str | None = None) -> DiscoveryResult: return DiscoveryResult(handlers=handlers, warnings=warnings) -def _load_hook_events(path: Path, warnings: list[str], source: str) -> dict | None: - """Read one config file and return its ``hooks`` object (or ``None``). - - Two shapes are accepted: - - - Claude-Code dict format (``{"hooks": {"EventName": [...]}}``) — any source. - - deepcode-hooks MCP list format (``{"hooks": [...]}``) — **only** from the - ``user-mcp`` source (``~/.deepcode/hooks_config.json``). A list shape in - any other source is rejected with a warning so an accidental shape - mismatch cannot silently disable hooks. - """ +def _load_hook_events(path: Path, warnings: list[str]) -> dict | None: + """Read one config file and return its ``hooks`` object (or ``None``).""" if not path.is_file(): return None try: @@ -133,104 +108,9 @@ def _load_hook_events(path: Path, warnings: list[str], source: str) -> dict | No warnings.append(f"failed to read hooks config {path}: {exc}") return None hooks = data.get("hooks") if isinstance(data, dict) else None - if isinstance(hooks, dict): - return hooks # Claude-Code format - if isinstance(hooks, list): - # deepcode-hooks MCP list format (hooks_config.json) - if source != "user-mcp": - warnings.append( - f"ignoring list-shaped hooks in {path}: only " - "~/.deepcode/hooks_config.json supports the deepcode-hooks " - "list format" - ) - return None - return _mcp_hooks_to_events(hooks, warnings, path) - return None - - -def _mcp_hooks_to_events(mcp_hooks: list, warnings: list[str], path: Path) -> dict: - """Convert the deepcode-hooks MCP ``hooks`` list to the events-dict shape. - - Each entry: ``{name, event, handler, type, priority, timeout, enabled, - matcher, ...}``. Entries are validated explicitly — a malformed entry is - reported in ``warnings`` and skipped, never silently dropped. Only - ``shell`` / ``node`` handlers are kept (they run as plain commands); - ``python``-typed snippets are skipped with a warning. Within one event - the groups are ordered by ``priority`` (highest first; stable so equal - priorities keep declaration order). - """ - events: dict[str, list] = {} - for hook in mcp_hooks: - if not isinstance(hook, dict): - warnings.append(f"skipping non-object hook entry in {path}") - continue - name = hook.get("name") - if not isinstance(name, str) or not name.strip(): - warnings.append(f"skipping hook without a name in {path}") - continue - if hook.get("enabled") is False: - continue - event = hook.get("event") - if not isinstance(event, str) or not event.strip(): - warnings.append(f"skipping hook {name!r} without an event in {path}") - continue - canonical = _MCP_EVENT_ALIASES.get(event.lower(), event) - if canonical not in HOOK_EVENT_NAMES: - warnings.append( - f"skipping hook {name!r} with unknown event {event!r} in {path}" - ) - continue - handler = hook.get("handler") - if not isinstance(handler, str) or not handler.strip(): - warnings.append(f"skipping hook {name!r} without a handler in {path}") - continue - htype = hook.get("type", "shell") - if htype not in ("shell", "node"): - warnings.append( - f"skipping {htype!r} hook {name!r} in {path}: " - "only shell/node handlers are runnable as commands" - ) - continue - timeout = hook.get("timeout") - try: - timeout_sec = max(1, int(timeout)) if timeout is not None else None - except (TypeError, ValueError): - warnings.append( - f"ignoring invalid timeout {timeout!r} for hook {name!r} in {path}" - ) - timeout_sec = None - raw_matcher = hook.get("matcher") - matcher = ( - raw_matcher if isinstance(raw_matcher, str) and raw_matcher.strip() else "*" - ) - priority = hook.get("priority", 0) - try: - priority_int = int(priority) - except (TypeError, ValueError): - warnings.append( - f"ignoring invalid priority {priority!r} for hook {name!r} in {path}" - ) - priority_int = 0 - events.setdefault(canonical, []).append( - { - "matcher": matcher, - "priority": priority_int, - "hooks": [ - { - "type": "command", - "command": handler, - **({"timeout": timeout_sec} if timeout_sec is not None else {}), - } - ], - } - ) - # Higher ``priority`` runs first (stable sort keeps equal priorities in - # declaration order); the transient key is dropped before _append_group. - for groups in events.values(): - groups.sort(key=lambda group: group.get("priority", 0), reverse=True) - for group in groups: - group.pop("priority", None) - return events + if not isinstance(hooks, dict): + return None + return hooks def _append_group( @@ -272,12 +152,19 @@ def _append_group( warnings.append(f"skipping empty hook command in {path}") continue timeout = handler.get("timeout") + default_timeout = ( + _SESSION_END_DEFAULT_TIMEOUT_SEC + if event_name == "SessionEnd" + else _DEFAULT_TIMEOUT_SEC + ) try: timeout_sec = ( - max(1, int(timeout)) if timeout is not None else _DEFAULT_TIMEOUT_SEC + max(1, int(timeout)) if timeout is not None else default_timeout ) except (TypeError, ValueError): - timeout_sec = _DEFAULT_TIMEOUT_SEC + timeout_sec = default_timeout + if event_name == "SessionEnd": + timeout_sec = min(timeout_sec, _SESSION_END_MAX_TIMEOUT_SEC) status_message = handler.get("statusMessage") or handler.get("status_message") handlers.append( Handler( diff --git a/core/harness/hooks/engine.py b/core/harness/hooks/engine.py index 2791a80c..71bdc19c 100644 --- a/core/harness/hooks/engine.py +++ b/core/harness/hooks/engine.py @@ -176,14 +176,13 @@ async def run_session_start(self, source: str = "startup") -> ContextOutcome: additional_contexts=folded.additional_contexts, ) - async def run_session_end(self, reason: str = "shutdown") -> ContextOutcome: + async def run_session_end(self, reason: str = "other") -> ContextOutcome: """Session lifecycle end — fires exactly once when the session terminates. Called from ``AgentSession.submit(Shutdown)``, never per turn. The - reason doubles as the matcher input so a hook can target a specific - exit path (e.g. ``matcher: "shutdown"``); supported session-exit - reasons are ``shutdown``, ``interrupted`` and ``error``. The caller - logs failures so a hook can never crash the session close. + reason doubles as the matcher input. DeepCode runtime shutdown maps to + the compatible ``other`` reason. The caller logs failures so a hook + can never crash the session close. """ payload = {"hook_event_name": "SessionEnd", "reason": reason} folded = await self._dispatch("SessionEnd", reason, payload) diff --git a/core/harness/hooks/events.py b/core/harness/hooks/events.py index ca5ee64a..61668754 100644 --- a/core/harness/hooks/events.py +++ b/core/harness/hooks/events.py @@ -15,8 +15,8 @@ - ``Stop`` — when the agent would end its turn (may force-continue) The remaining reference events (``PermissionRequest``, ``PreCompact``, -``PostCompact``, ``SubagentStart``, ``SubagentStop``) are recognized by the -engine but wired opportunistically as DeepCode grows the matching kernel seams. +``PostCompact``, ``SessionEnd``, ``SubagentStart``, ``SubagentStop``) are +recognized by the engine and wired at their matching lifecycle seams. """ from __future__ import annotations @@ -41,8 +41,8 @@ # Events whose ``matcher`` field is meaningful. ``UserPromptSubmit`` and # ``Stop`` fire unconditionally, so their matchers are ignored (mirrors the # reference). ``SessionEnd`` DOES honour its matcher: the session-exit reason -# (``shutdown`` / ``interrupted`` / ``error``) is matched against the -# ``matcher`` field so hooks can target a specific exit path. +# is matched against the ``matcher`` field so hooks can target a specific exit +# path. DeepCode runtime shutdown uses the compatible ``other`` reason. _EVENTS_WITHOUT_MATCHER: frozenset[str] = frozenset({"UserPromptSubmit", "Stop"}) diff --git a/core/harness/hooks/execution.py b/core/harness/hooks/execution.py index ab3a95ec..ff772a9d 100644 --- a/core/harness/hooks/execution.py +++ b/core/harness/hooks/execution.py @@ -19,7 +19,6 @@ import asyncio import json import os -import shutil import time from dataclasses import dataclass from typing import Any @@ -57,13 +56,6 @@ class HandlerDecision: def _default_shell() -> list[str]: if os.name == "nt": # pragma: no cover - posix CI - # Hook commands follow the Claude-Code POSIX shell contract (`;` - # separators, single-quoted JSON, `cat` redirection). Prefer a POSIX - # shell (e.g. Git Bash) on Windows so those commands actually run; - # fall back to cmd.exe only when no POSIX shell is available. - sh = shutil.which("sh") - if sh: - return [sh, "-lc"] comspec = os.environ.get("COMSPEC", "cmd.exe") return [comspec, "/C"] shell = os.environ.get("SHELL", "/bin/sh") diff --git a/core/harness/sandbox.py b/core/harness/sandbox.py index 67dc9a93..5fdf38bd 100644 --- a/core/harness/sandbox.py +++ b/core/harness/sandbox.py @@ -44,7 +44,6 @@ import shutil import tempfile from dataclasses import dataclass, field -from pathlib import Path # Absolute path — never resolved via PATH (PATH-injection defense). _MACOS_SANDBOX_EXEC = "/usr/bin/sandbox-exec" @@ -323,14 +322,8 @@ def wrap_argv_command( # ``python -m core.harness.windows_sandbox -- `` creates # a KILL_ON_JOB_CLOSE job, spawns the inner command into it suspended, # and resumes it — the whole process tree dies with the wrapper. - # - # The wrapper runs as ``python -m core.harness.windows_sandbox``, so the - # child interpreter must be able to import ``core``. The BashTool cwd is - # the workspace (often a tmp dir) — not on sys.path — so inject the repo - # root through PYTHONPATH to keep the module importable. import sys as _sys - repo_root = str(Path(__file__).resolve().parents[2]) argv = [ _sys.executable, "-m", @@ -338,11 +331,7 @@ def wrap_argv_command( "--", *inner_argv, ] - return WrappedCommand( - argv=argv, - backend=backend, - extra_env={"PYTHONPATH": repo_root}, - ) + return WrappedCommand(argv=argv, backend=backend) return WrappedCommand(argv=list(inner_argv), backend="none") @@ -418,21 +407,6 @@ def build_exec_command( bare = [shell, "-c", command] if command is not None else list(argv or []) return WrappedCommand(argv=bare, backend="disabled") - # On Windows the wrapped command is launched by the Job Object sandbox - # (``CreateProcessW``), which cannot resolve POSIX-style shell paths like - # ``/bin/bash``. Resolve a real executable path (e.g. Git Bash ``sh``) so - # the inner command starts; callers may still override ``shell`` with any - # Windows-resolvable value. The disabled path above keeps the bare argv - # untouched (upstream-locked contract). - if command is not None and os.name == "nt": - import shutil - - resolved = shutil.which(shell) - if resolved is None and shell in ("/bin/bash", "/bin/sh", "bash", "sh"): - resolved = shutil.which("sh") - if resolved: - shell = resolved - policy = SandboxPolicy.for_workspace(workspace, allow_network=allow_network) if command is not None: return wrap_shell_command(command, policy, shell=shell) diff --git a/core/harness/tools/shell.py b/core/harness/tools/shell.py index b20ba345..0f1ee1fd 100644 --- a/core/harness/tools/shell.py +++ b/core/harness/tools/shell.py @@ -105,7 +105,6 @@ async def execute(self, **kwargs: Any) -> Any: proc = await asyncio.create_subprocess_exec( *wrapped.argv, cwd=self._workspace, - env={**os.environ, **wrapped.extra_env}, stdout=asyncio.subprocess.PIPE, stderr=asyncio.subprocess.STDOUT, **subprocess_group_kwargs(), diff --git a/tests/test_hooks.py b/tests/test_hooks.py index 778923aa..e207d76b 100644 --- a/tests/test_hooks.py +++ b/tests/test_hooks.py @@ -300,7 +300,7 @@ def test_stop_block_means_keep_going(): def test_payload_delivered_on_stdin(tmp_path): capture = tmp_path / "payload.json" - eng = _engine([_handler("PreToolUse", f"cat > {capture.as_posix()}", matcher="*")]) + eng = _engine([_handler("PreToolUse", f"cat > {capture}", matcher="*")]) asyncio.run(eng.run_pre_tool_use("Bash", {"command": "ls"}, tool_use_id="tu-9")) payload = json.loads(capture.read_text()) assert payload["session_id"] == "sess-1" @@ -567,9 +567,7 @@ def test_session_start_and_prompt_context_injected(): def test_subagent_start_payload_and_plaintext_context(tmp_path): capture = tmp_path / "p.json" - eng = _engine( - [_handler("SubagentStart", f"cat > {capture.as_posix()}; echo sub-context")] - ) + eng = _engine([_handler("SubagentStart", f"cat > {capture}; echo sub-context")]) res = asyncio.run(eng.run_subagent_start("worker-7", "subagent")) assert res.additional_contexts == ["sub-context"] # plain-text context works p = json.loads(capture.read_text()) @@ -804,7 +802,7 @@ async def ask(name, args): def test_pre_compact_hook_block_skips_and_payload(tmp_path): capture = tmp_path / "p.json" out = json.dumps({"continue": False}) - eng = _engine([_handler("PreCompact", f"cat > {capture.as_posix()}; echo '{out}'")]) + eng = _engine([_handler("PreCompact", f"cat > {capture}; echo '{out}'")]) res = asyncio.run(eng.run_pre_compact("auto")) assert res.block is True # continue:false → skip compaction p = json.loads(capture.read_text()) @@ -820,7 +818,7 @@ def test_pre_compact_matcher_matches_trigger(): def test_post_compact_hook_fires_with_trigger(tmp_path): capture = tmp_path / "p.json" - eng = _engine([_handler("PostCompact", f"cat > {capture.as_posix()}")]) + eng = _engine([_handler("PostCompact", f"cat > {capture}")]) asyncio.run(eng.run_post_compact("auto")) p = json.loads(capture.read_text()) assert p["hook_event_name"] == "PostCompact" and p["trigger"] == "auto" @@ -831,7 +829,7 @@ def test_post_compact_hook_fires_with_trigger(tmp_path): def test_stop_payload_carries_stop_hook_active(tmp_path): capture = tmp_path / "p.json" - eng = _engine([_handler("Stop", f"cat > {capture.as_posix()}")]) + eng = _engine([_handler("Stop", f"cat > {capture}")]) asyncio.run(eng.run_stop(stop_hook_active=True)) p = json.loads(capture.read_text()) assert p["hook_event_name"] == "Stop" and p["stop_hook_active"] is True diff --git a/tests/test_session_end_lifecycle.py b/tests/test_session_end_lifecycle.py index 10887dbb..6b375f9e 100644 --- a/tests/test_session_end_lifecycle.py +++ b/tests/test_session_end_lifecycle.py @@ -1,17 +1,14 @@ -"""SessionEnd lifecycle + PreCompact checkpoint + MCP list-discovery e2e tests. +"""SessionEnd lifecycle and PreCompact checkpoint end-to-end tests. Covers the lifecycle contracts introduced by the SessionEnd / PreCompact work: - ``SessionEnd`` fires exactly once at real session termination (``AgentSession.submit(Shutdown)``), never per turn; the session-exit reason - doubles as the matcher input (``shutdown`` / ``interrupted`` / ``error``); - a failing hook is non-fatal and never blocks ``ShutdownComplete``. + doubles as the matcher input (DeepCode shutdown maps to ``other``); a failing + hook is non-fatal and never blocks ``ShutdownComplete``. - The ``PreCompact`` hook's ``additional_contexts`` survive a successful compaction as a single bounded, provider-agnostic user message, and are absent when the hook blocks or summarization fails. -- The deepcode-hooks MCP ``hooks_config.json`` list format is only accepted - from the ``user-mcp`` source, supports ``priority`` ordering, skips disabled - entries, warns on invalid entries, and honours timeouts and event aliases. Hooks are exercised as REAL subprocesses (``sh -lc`` commands that echo JSON or exit with a code), matching ``test_hooks.py`` so we test the true execution @@ -32,9 +29,9 @@ if str(ROOT) not in sys.path: sys.path.insert(0, str(ROOT)) -from core.harness.hooks.discovery import Handler, discover_hooks # noqa: E402 -from core.harness.hooks.engine import HooksEngine # noqa: E402 from core.events.protocol import Shutdown, ShutdownComplete, UserInput # noqa: E402 +from core.harness.hooks.discovery import Handler # noqa: E402 +from core.harness.hooks.engine import HooksEngine # noqa: E402 pytestmark = pytest.mark.skipif( shutil.which("sh") is None, reason="POSIX shell required" @@ -82,6 +79,9 @@ async def execute(self, name, params): self.calls.append((name, params)) return f"ran {name} with {params}" + async def aclose(self): + return None + # --------------------------------------------------------------------------- # SessionEnd lifecycle — fires exactly once at real session termination @@ -99,13 +99,28 @@ def test_session_end_fires_exactly_once_on_shutdown(tmp_path): event = asyncio.run(session.next_event()) assert isinstance(event.msg, ShutdownComplete) + asyncio.run(session.submit(Shutdown())) + asyncio.run(session.aclose()) + assert count.read_text().count("x") == 1 + + +def test_session_end_fires_from_real_close_path(tmp_path): + count = tmp_path / "count.txt" + eng = _engine([_handler("SessionEnd", f"echo x >> {count}")]) + session = _session(eng) + + asyncio.run(session.aclose()) + asyncio.run(session.aclose()) + + assert count.read_text().count("x") == 1 + def test_session_end_reason_matcher(tmp_path): shutdown_hits = tmp_path / "shutdown.txt" other_hits = tmp_path / "other.txt" eng = _engine( [ - _handler("SessionEnd", f"echo x >> {shutdown_hits}", matcher="shutdown"), + _handler("SessionEnd", f"echo x >> {shutdown_hits}", matcher="other"), _handler("SessionEnd", f"echo x >> {other_hits}", matcher="complete"), ] ) @@ -127,6 +142,46 @@ def test_session_end_hook_failure_non_fatal(): assert isinstance(event.msg, ShutdownComplete) +def test_session_end_is_not_emitted_for_subagent_session(tmp_path): + count = tmp_path / "count.txt" + eng = _engine([_handler("SessionEnd", f"echo x >> {count}")]) + session = _session(eng) + session._agent_context = ("child-1", "subagent") + + asyncio.run(session.aclose()) + + assert not count.exists() + + +def test_session_end_discovery_uses_bounded_exit_timeout(tmp_path): + from core.harness.hooks.discovery import discover_hooks + + home = tmp_path / "home" + workspace = tmp_path / "workspace" + (home / ".deepcode").mkdir(parents=True) + workspace.mkdir() + (home / ".deepcode" / "hooks.json").write_text( + json.dumps( + { + "hooks": { + "SessionEnd": [ + {"matcher": "*", "hooks": [{"command": "echo default"}]}, + { + "matcher": "*", + "hooks": [{"command": "echo capped", "timeout": 999}], + }, + ] + } + } + ), + encoding="utf-8", + ) + + result = discover_hooks(str(workspace), str(home)) + + assert [handler.timeout_sec for handler in result.handlers] == [2, 60] + + def test_normal_turn_does_not_trigger_session_end(tmp_path): count = tmp_path / "count.txt" eng = _engine([_handler("SessionEnd", f"echo x >> {count}")]) @@ -173,13 +228,21 @@ def test_build_precompact_checkpoint_limits(): checkpoint = _build_precompact_checkpoint(many) assert checkpoint is not None body = checkpoint[len(_PRECOMPACT_CHECKPOINT_PREFIX) + 1 :] - assert len(body) <= _PRECOMPACT_TOTAL_LIMIT + assert len(checkpoint) <= _PRECOMPACT_TOTAL_LIMIT + + +def test_build_precompact_checkpoint_honors_smaller_dynamic_limit(): + from core.agent_runtime.runner import _build_precompact_checkpoint + + checkpoint = _build_precompact_checkpoint(["x" * 500], total_limit=100) + assert checkpoint is not None + assert len(checkpoint) <= 100 def test_maybe_compact_checkpoint_injected_after_success(monkeypatch): from types import SimpleNamespace - from core.agent_runtime.runner import AgentRunSpec, AgentRunner + from core.agent_runtime.runner import AgentRunner, AgentRunSpec runner = AgentRunner(provider=object()) monkeypatch.setattr(runner, "_estimate_prompt", lambda spec, messages: 999_999) @@ -216,12 +279,15 @@ async def pre_compact_hook(trigger): ] assert checkpoint_msgs, "checkpoint must survive a successful compaction" assert "checkpoint ctx" in checkpoint_msgs[0]["content"] + assert sum(len(str(m.get("content", ""))) for m in compacted) < sum( + len(str(m.get("content", ""))) for m in messages + ) def test_maybe_compact_block_skips_checkpoint(monkeypatch): from types import SimpleNamespace - from core.agent_runtime.runner import AgentRunSpec, AgentRunner + from core.agent_runtime.runner import AgentRunner, AgentRunSpec runner = AgentRunner(provider=object()) monkeypatch.setattr(runner, "_estimate_prompt", lambda spec, messages: 999_999) @@ -253,7 +319,7 @@ async def pre_compact_hook(trigger): def test_maybe_compact_summarize_failure_no_checkpoint(monkeypatch): from types import SimpleNamespace - from core.agent_runtime.runner import AgentRunSpec, AgentRunner + from core.agent_runtime.runner import AgentRunner, AgentRunSpec runner = AgentRunner(provider=object()) monkeypatch.setattr(runner, "_estimate_prompt", lambda spec, messages: 999_999) @@ -285,125 +351,3 @@ async def pre_compact_hook(trigger): compacted = asyncio.run(runner._maybe_compact(spec, messages)) assert compacted is messages assert "PreCompact checkpoint" not in json.dumps(compacted) - - -# --------------------------------------------------------------------------- -# deepcode-hooks MCP list-format discovery (hooks_config.json) -# --------------------------------------------------------------------------- - - -def _write_config(home, ws, payload): - (home / ".deepcode").mkdir(parents=True, exist_ok=True) - (ws / ".deepcode").mkdir(parents=True, exist_ok=True) - (home / ".deepcode" / "hooks_config.json").write_text( - json.dumps(payload), encoding="utf-8" - ) - return str(ws), str(home) - - -def test_mcp_list_format_accepted_from_user_mcp(tmp_path): - ws, home = _write_config( - tmp_path / "home", - tmp_path / "ws", - {"hooks": [{"name": "h1", "event": "PreToolUse", "handler": "echo hi"}]}, - ) - result = discover_hooks(ws, home) - assert result.warnings == [] - assert any( - h.event_name == "PreToolUse" and h.command == "echo hi" for h in result.handlers - ) - - -def test_mcp_list_format_rejected_from_other_sources(tmp_path): - home = tmp_path / "home" - ws = tmp_path / "ws" - (home / ".deepcode").mkdir(parents=True, exist_ok=True) - (ws / ".deepcode").mkdir(parents=True, exist_ok=True) - # list shape in a project hooks.json (non user-mcp source) must be rejected - (ws / ".deepcode" / "hooks.json").write_text( - json.dumps( - {"hooks": [{"name": "h1", "event": "PreToolUse", "handler": "echo hi"}]} - ), - encoding="utf-8", - ) - result = discover_hooks(str(ws), str(home)) - assert any("list-shaped hooks" in w for w in result.warnings) - assert result.handlers == [] - - -def test_mcp_priority_ordering(tmp_path): - ws, home = _write_config( - tmp_path / "home", - tmp_path / "ws", - { - "hooks": [ - { - "name": "low", - "event": "PreToolUse", - "handler": "echo low", - "priority": 1, - }, - { - "name": "high", - "event": "PreToolUse", - "handler": "echo high", - "priority": 10, - }, - ] - }, - ) - result = discover_hooks(ws, home) - pre_tool = [h for h in result.handlers if h.event_name == "PreToolUse"] - assert [h.command for h in pre_tool] == ["echo high", "echo low"] - assert pre_tool[0].display_order < pre_tool[1].display_order - - -def test_mcp_disabled_and_invalid_entries(tmp_path): - ws, home = _write_config( - tmp_path / "home", - tmp_path / "ws", - { - "hooks": [ - { - "name": "disabled", - "event": "PreToolUse", - "handler": "echo x", - "enabled": False, - }, - {"name": "no-event", "handler": "echo x"}, - { - "name": "bad-type", - "event": "PreToolUse", - "handler": "pass", - "type": "python", - }, - {"name": "ok", "event": "PreToolUse", "handler": "echo ok"}, - ] - }, - ) - result = discover_hooks(ws, home) - assert [h.command for h in result.handlers] == ["echo ok"] - assert any("without an event" in w for w in result.warnings) - assert any("only shell/node" in w for w in result.warnings) - - -def test_mcp_timeout_and_event_alias(tmp_path): - ws, home = _write_config( - tmp_path / "home", - tmp_path / "ws", - { - "hooks": [ - { - "name": "aliased", - "event": "sessionStart", - "handler": "echo aliased", - "timeout": 7, - } - ] - }, - ) - result = discover_hooks(ws, home) - assert result.warnings == [] - hook = result.handlers[0] - assert hook.event_name == "SessionStart" - assert hook.timeout_sec == 7 From 16e20c52932f8d452cfe330b77aad30c3a2b3b1d Mon Sep 17 00:00:00 2001 From: Raymond Ginger Date: Tue, 11 Aug 2026 10:41:49 +0800 Subject: [PATCH 14/23] fix(security): restrict Windows private files with fail-safe ACL ordering --- core/private_storage.py | 74 ++++++++++++++++++++++++++++++++++++++--- 1 file changed, 70 insertions(+), 4 deletions(-) diff --git a/core/private_storage.py b/core/private_storage.py index e4e662ac..da7808a9 100644 --- a/core/private_storage.py +++ b/core/private_storage.py @@ -5,14 +5,17 @@ the process umask, which is commonly permissive on desktop systems. POSIX permissions are repaired to ``0700`` for directories and ``0600`` for -regular files. Windows access control is inherited from the user's profile; -the mode arguments are still supplied at creation time where supported. +regular files. On Windows the current user is granted full control and the +inherited access entries are then stripped; the restriction is applied in a +fail-safe order so a failed grant leaves the inherited ACLs untouched and the +path stays accessible. """ from __future__ import annotations import os import stat +import subprocess from pathlib import Path PRIVATE_DIRECTORY_MODE = 0o700 @@ -23,6 +26,68 @@ class UnsafePrivateFileError(OSError): """A private-state path is not a regular file owned by this path entry.""" +def _windows_identity() -> str | None: + """Return the fully-qualified current user (``DOMAIN\\user``) on Windows.""" + + if os.name != "nt": + return None + try: + completed = subprocess.run( + ["whoami"], + capture_output=True, + text=True, + encoding="mbcs", + errors="replace", + timeout=5, + check=True, + ) + except (OSError, subprocess.SubprocessError): + return None + principal = (completed.stdout or "").strip() + return principal or None + + +def _restrict_windows_acl(path: Path) -> None: + """Restrict ``path`` to the current user, failing safe. + + The current user is granted full control **before** inherited access + entries are stripped. If the grant fails (service account, transient + timeout, ...) the inherited ACLs are left untouched so the path stays + accessible to the caller; the previous strip-first order could leave a + path with no usable ACE and make it unopenable. + """ + + identity = _windows_identity() + if identity is None: + return + try: + subprocess.run( + ["icacls", os.fspath(path), "/grant:r", f"{identity}:F"], + capture_output=True, + text=True, + encoding="mbcs", + errors="replace", + timeout=15, + check=True, + ) + except (OSError, subprocess.SubprocessError): + # Fail safe: keep the inherited ACLs; the path stays accessible. + return + try: + subprocess.run( + ["icacls", os.fspath(path), "/inheritance:r"], + capture_output=True, + text=True, + encoding="mbcs", + errors="replace", + timeout=15, + check=True, + ) + except (OSError, subprocess.SubprocessError): + # Strip failed: the path is merely less restricted, still usable. + pass + + def ensure_private_directory(path: Path | str) -> Path: """Create ``path`` and make every newly created component user-private.""" @@ -63,6 +128,8 @@ def open_private_file(path: Path | str, flags: int) -> int: ) if os.name != "nt": os.fchmod(descriptor, PRIVATE_FILE_MODE) + else: + _restrict_windows_acl(target) return descriptor except BaseException: os.close(descriptor) @@ -119,8 +186,6 @@ def harden_private_tree(root: Path | str) -> Path: """Repair a DeepCode-owned tree while refusing to traverse symlinks.""" base = ensure_private_directory(root) - if os.name == "nt": - return base for current, directories, files in os.walk(base, followlinks=False): current_path = Path(current) @@ -137,6 +202,7 @@ def harden_private_tree(root: Path | str) -> Path: def _chmod(path: Path, mode: int) -> None: if os.name == "nt": + _restrict_windows_acl(path) return try: os.chmod(path, mode, follow_symlinks=False) From 66f6a06312342c9e21a2cb83be6c726c388511c4 Mon Sep 17 00:00:00 2001 From: Raymond Ginger Date: Tue, 11 Aug 2026 10:41:56 +0800 Subject: [PATCH 15/23] test(security): cover Windows private-file ACL restriction --- tests/test_private_storage_windows.py | 122 ++++++++++++++++++++++++++ 1 file changed, 122 insertions(+) create mode 100644 tests/test_private_storage_windows.py diff --git a/tests/test_private_storage_windows.py b/tests/test_private_storage_windows.py new file mode 100644 index 00000000..8a22a1e1 --- /dev/null +++ b/tests/test_private_storage_windows.py @@ -0,0 +1,122 @@ +"""Windows NTFS ACL restriction tests for core.private_storage. + +These tests assert that private directories and files are restricted to the +current user with full control and that dangerous well-known ACEs (Everyone, +Authenticated Users, BUILTIN\\Users) are removed after the restriction runs. +They are skipped on non-Windows platforms. +""" + +from __future__ import annotations + +import os +import subprocess +from pathlib import Path + +import pytest + +from core.private_storage import ( + ensure_private_directory, + harden_private_tree, + open_private_file, +) + +pytestmark = pytest.mark.skipif( + os.name != "nt", + reason="NTFS ACL restriction applies on Windows only", +) + +_DANGEROUS_ACES = ("Authenticated Users", "BUILTIN\\Users", "Everyone") + + +def _windows_identity() -> str: + completed = subprocess.run( + ["whoami"], + capture_output=True, + text=True, + encoding="mbcs", + errors="replace", + timeout=5, + check=True, + ) + return (completed.stdout or "").strip() + + +def _acl_lines(path: Path) -> list[str]: + completed = subprocess.run( + ["icacls", os.fspath(path)], + capture_output=True, + text=True, + encoding="mbcs", + errors="replace", + timeout=15, + check=True, + ) + return [ + line.strip() for line in (completed.stdout or "").splitlines() if ":" in line + ] + + +def _assert_no_dangerous_aces(path: Path) -> None: + lines = _acl_lines(path) + joined = "\n".join(lines).lower() + for ace in _DANGEROUS_ACES: + assert ace.lower() not in joined, ( + f"{path} still exposes dangerous ACE {ace!r}:\n{joined}" + ) + + +def _assert_current_user_has_full_control(path: Path) -> None: + identity = _windows_identity().lower() + # ``whoami`` may return either ``domain\user`` or a bare ``user`` depending + # on which binary is on PATH, while ``icacls`` always prints the fully + # qualified principal. Compare the last path segment so both match. + short_name = identity.rsplit("\\", 1)[-1] + lines = _acl_lines(path) + for line in lines: + # Split from the right: icacls lines start with a Windows path that + # contains a drive-letter colon (``C:\\...``), so the first colon is + # not the principal/rights separator. + principal, rights = line.rsplit(":", 1) + principal_short = principal.strip().lower().rsplit("\\", 1)[-1] + if principal_short == short_name: + assert "(f)" in rights.lower(), ( + f"{path} does not grant the current user full control:\n{line}" + ) + return + raise AssertionError( + f"{path} has no ACE for the current user {identity!r}:\n" + "\n".join(lines) + ) + + +def test_windows_private_directory_is_restricted(tmp_path: Path) -> None: + directory = ensure_private_directory(tmp_path / "private" / "nested") + + _assert_no_dangerous_aces(directory) + _assert_current_user_has_full_control(directory) + + +def test_windows_private_file_is_restricted(tmp_path: Path) -> None: + target = tmp_path / "private" / "credentials.json" + descriptor = open_private_file(target, os.O_WRONLY | os.O_CREAT) + try: + os.write(descriptor, b"secret") + finally: + os.close(descriptor) + + _assert_no_dangerous_aces(target) + _assert_current_user_has_full_control(target) + assert target.read_bytes() == b"secret" + + +def test_windows_harden_private_tree_restricts_every_entry(tmp_path: Path) -> None: + root = tmp_path / "legacy-private" + session = root / "session-1" + session.mkdir(parents=True) + (session / "session.jsonl").write_text("legacy\n", encoding="utf-8") + (root / "settings.json").write_text("{}", encoding="utf-8") + + harden_private_tree(root) + + for path in (root, session, session / "session.jsonl", root / "settings.json"): + _assert_no_dangerous_aces(path) + _assert_current_user_has_full_control(path) From d7a1fdeb81dfc06a8ff9ddf71a97424f6ef61cf7 Mon Sep 17 00:00:00 2001 From: Raymond Ginger Date: Tue, 11 Aug 2026 10:42:41 +0800 Subject: [PATCH 16/23] ci(windows): run Windows private-storage ACL tests in windows-lifecycle --- .github/workflows/python-ci.yml | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/.github/workflows/python-ci.yml b/.github/workflows/python-ci.yml index 73fe5a01..e06960d8 100644 --- a/.github/workflows/python-ci.yml +++ b/.github/workflows/python-ci.yml @@ -66,12 +66,13 @@ jobs: tests/application/test_session_deletion_service.py tests/application/test_execution_coordinator.py - # These suites carry Windows-gated cases (the Job Object backend) that - # the ubuntu job can only skip. This is the sole place they actually - # execute. - - name: Verify Job Object sandbox + # These suites carry Windows-gated cases (NTFS ACLs, the Job Object + # backend) that the ubuntu job can only skip. This is the sole place + # they actually execute. + - name: Verify Windows ACLs and Job Object sandbox run: >- python -m pytest -q + tests/test_private_storage_windows.py tests/test_harness_sandbox.py tests/test_exec_sandbox_wiring.py From fe2b784139592d565a6788e64ddfbaf4a8a936c7 Mon Sep 17 00:00:00 2001 From: DeepCode Date: Sun, 16 Aug 2026 09:13:05 +0800 Subject: [PATCH 17/23] fix(security): apply Windows ACL only at file creation, never per open Addresses maintainer feedback on the earlier ACL PR (#148): re-running icacls on every open costs two subprocesses per call for no change. - open_private_file now restricts a file's ACL only when it was just created (target did not exist before os.open); opening an existing private file never re-runs _restrict_windows_acl. - harden_private_tree keeps forcing the restriction (it repairs legacy trees whose ACLs may be absent), so its semantics are unchanged. - new cross-platform tests (mock _restrict_windows_acl) assert: new file restricted exactly once, existing file never re-restricted, read-only open of an existing file never restricts. --- core/private_storage.py | 17 ++++-- tests/test_private_storage_acl_once.py | 75 ++++++++++++++++++++++++++ tests/test_private_storage_windows.py | 33 ++++++++++++ 3 files changed, 121 insertions(+), 4 deletions(-) create mode 100644 tests/test_private_storage_acl_once.py diff --git a/core/private_storage.py b/core/private_storage.py index da7808a9..e90b4990 100644 --- a/core/private_storage.py +++ b/core/private_storage.py @@ -100,10 +100,10 @@ def ensure_private_directory(path: Path | str) -> Path: for component in reversed(missing): component.mkdir(mode=PRIVATE_DIRECTORY_MODE, exist_ok=True) - _chmod(component, PRIVATE_DIRECTORY_MODE) + _chmod(component, PRIVATE_DIRECTORY_MODE, force=True) directory.mkdir(parents=True, exist_ok=True, mode=PRIVATE_DIRECTORY_MODE) - _chmod(directory, PRIVATE_DIRECTORY_MODE) + _chmod(directory, PRIVATE_DIRECTORY_MODE, force=True) return directory @@ -111,6 +111,11 @@ def open_private_file(path: Path | str, flags: int) -> int: """Open a private regular file without following a final symlink.""" target = Path(path) + # Only restrict a *newly created* file. An existing file was already + # restricted at creation; re-running icacls on every open costs two + # subprocesses per call (and a full tree walk many times over) without + # changing the ACL (maintainer feedback on the earlier ACL PR). + created = not target.exists() ensure_private_directory(target.parent) descriptor = os.open( target, @@ -128,7 +133,7 @@ def open_private_file(path: Path | str, flags: int) -> int: ) if os.name != "nt": os.fchmod(descriptor, PRIVATE_FILE_MODE) - else: + elif created: _restrict_windows_acl(target) return descriptor except BaseException: @@ -200,8 +205,12 @@ def harden_private_tree(root: Path | str) -> Path: return base -def _chmod(path: Path, mode: int) -> None: +def _chmod(path: Path, mode: int, *, force: bool = False) -> None: if os.name == "nt": + # harden_private_tree deliberately re-applies the restriction even to + # existing paths (it repairs legacy trees whose ACLs may be absent or + # permissive), so _chmod restricts unconditionally. The per-open cost + # is avoided in open_private_file by only restricting new files. _restrict_windows_acl(path) return try: diff --git a/tests/test_private_storage_acl_once.py b/tests/test_private_storage_acl_once.py new file mode 100644 index 00000000..72840d8d --- /dev/null +++ b/tests/test_private_storage_acl_once.py @@ -0,0 +1,75 @@ +"""Cross-platform tests for the per-open ACL optimization in private_storage. + +The Windows ACL restriction is applied at file *creation*; opening an +existing private file must not re-run icacls (maintainer feedback on the +earlier ACL PR: "open_private_file() currently calls it on each call; once at +creation is enough"). These tests mock `_restrict_windows_acl` to count calls, +so they run on any platform. +""" + +from __future__ import annotations + +import os +import sys +from pathlib import Path + +ROOT = Path(__file__).resolve().parents[1] +if str(ROOT) not in sys.path: + sys.path.insert(0, str(ROOT)) + +from core.private_storage import open_private_file + + +def _file_calls(calls, target: Path) -> int: + """Count restrictions applied to the target file itself (excludes the + parent-directory restriction that ensure_private_directory performs).""" + return sum(1 for p in calls if Path(p) == target) + + +def test_open_existing_file_does_not_rerun_acl(monkeypatch, tmp_path: Path) -> None: + import core.private_storage as ps + + calls = [] + monkeypatch.setattr(ps, "_restrict_windows_acl", lambda p: calls.append(p)) + + target = tmp_path / "existing.jsonl" + # First open: file does not exist → new → restrict once. + fd = open_private_file(target, os.O_CREAT | os.O_RDWR) + os.close(fd) + assert _file_calls(calls, target) == 1, "new file must be restricted exactly once" + + # Second open: file exists → must NOT re-run the ACL restriction. + fd = open_private_file(target, os.O_RDWR) + os.close(fd) + assert _file_calls(calls, target) == 1, ( + "existing file must not re-run the ACL restriction" + ) + + +def test_open_created_file_restricts_once(monkeypatch, tmp_path: Path) -> None: + import core.private_storage as ps + + calls = [] + monkeypatch.setattr(ps, "_restrict_windows_acl", lambda p: calls.append(p)) + + target = tmp_path / "fresh.jsonl" + for _ in range(3): + fd = open_private_file(target, os.O_CREAT | os.O_RDWR) + os.close(fd) + assert _file_calls(calls, target) == 1, "created once, restricted once, never again" + + +def test_open_without_creat_never_restricts(monkeypatch, tmp_path: Path) -> None: + import core.private_storage as ps + + calls = [] + monkeypatch.setattr(ps, "_restrict_windows_acl", lambda p: calls.append(p)) + + target = tmp_path / "pre.jsonl" + target.write_text("x", encoding="utf-8") + # O_RDONLY (no O_CREAT) on an existing file → no new file → no restriction. + fd = open_private_file(target, os.O_RDONLY) + os.close(fd) + assert _file_calls(calls, target) == 0, ( + "read-only open of an existing file must not restrict" + ) diff --git a/tests/test_private_storage_windows.py b/tests/test_private_storage_windows.py index 8a22a1e1..5393f97c 100644 --- a/tests/test_private_storage_windows.py +++ b/tests/test_private_storage_windows.py @@ -120,3 +120,36 @@ def test_windows_harden_private_tree_restricts_every_entry(tmp_path: Path) -> No for path in (root, session, session / "session.jsonl", root / "settings.json"): _assert_no_dangerous_aces(path) _assert_current_user_has_full_control(path) + + +def test_windows_open_existing_file_does_not_rerun_acl(monkeypatch, tmp_path: Path) -> None: + """The per-open ACL re-run is gone: opening an existing private file does + not call _restrict_windows_acl again (the ACL was applied at creation).""" + import core.private_storage as ps + + calls = [] + monkeypatch.setattr(ps, "_restrict_windows_acl", lambda p: calls.append(p)) + + target = tmp_path / "existing.jsonl" + # First open: file does not exist → new → restrict once. + fd = ps.open_private_file(target, os.O_CREAT | os.O_RDWR) + os.close(fd) + assert len(calls) == 1, "new file must be restricted exactly once" + + # Second open: file exists → must NOT re-run the ACL restriction. + fd = ps.open_private_file(target, os.O_RDWR) + os.close(fd) + assert len(calls) == 1, "existing file must not re-run the ACL restriction" + + +def test_windows_open_created_file_restricts_once(monkeypatch, tmp_path: Path) -> None: + """A file created via open_private_file is restricted exactly once.""" + import core.private_storage as ps + + calls = [] + monkeypatch.setattr(ps, "_restrict_windows_acl", lambda p: calls.append(p)) + + for _ in range(3): + fd = ps.open_private_file(tmp_path / "fresh.jsonl", os.O_CREAT | os.O_RDWR) + os.close(fd) + assert len(calls) == 1, "created once, restricted once, never again" From 4a086ae551756faa2beed501e1d0904fc8027ab2 Mon Sep 17 00:00:00 2001 From: raymondginger Date: Sun, 23 Aug 2026 06:25:58 +0800 Subject: [PATCH 18/23] fix(security): apply Windows ACL exactly once per created path Cross-platform call-count tests exposed two structural bugs: - elif created: hid the file restriction behind the POSIX fchmod branch, so the mocked ACL call never fired on non-Windows runners (0 calls). - _chmod restricted unconditionally on Windows, so ensure_private_directory re-ran icacls on the already-existing parent dir on every open (2-4 calls instead of 1). Now: directories restrict only when actually created (was_missing), files restrict only when created (independent if created), and _chmod honors force= (harden_private_tree/ensure_private_file still re-apply, regular callers do not churn existing paths). --- core/private_storage.py | 25 +++++++++++++++---------- tests/test_private_storage_windows.py | 4 +++- 2 files changed, 18 insertions(+), 11 deletions(-) diff --git a/core/private_storage.py b/core/private_storage.py index e90b4990..ebe677e3 100644 --- a/core/private_storage.py +++ b/core/private_storage.py @@ -102,8 +102,12 @@ def ensure_private_directory(path: Path | str) -> Path: component.mkdir(mode=PRIVATE_DIRECTORY_MODE, exist_ok=True) _chmod(component, PRIVATE_DIRECTORY_MODE, force=True) + was_missing = not directory.exists() directory.mkdir(parents=True, exist_ok=True, mode=PRIVATE_DIRECTORY_MODE) - _chmod(directory, PRIVATE_DIRECTORY_MODE, force=True) + # Restrict only what this call actually created. An existing directory was + # restricted at its own creation; re-running icacls on it on every open + # costs two subprocesses without changing the ACL. + _chmod(directory, PRIVATE_DIRECTORY_MODE, force=was_missing) return directory @@ -133,7 +137,7 @@ def open_private_file(path: Path | str, flags: int) -> int: ) if os.name != "nt": os.fchmod(descriptor, PRIVATE_FILE_MODE) - elif created: + if created: _restrict_windows_acl(target) return descriptor except BaseException: @@ -184,7 +188,7 @@ def ensure_private_file(path: Path | str) -> None: except OSError: return if stat.S_ISREG(metadata.st_mode): - _chmod(target, PRIVATE_FILE_MODE) + _chmod(target, PRIVATE_FILE_MODE, force=True) def harden_private_tree(root: Path | str) -> Path: @@ -194,12 +198,12 @@ def harden_private_tree(root: Path | str) -> Path: for current, directories, files in os.walk(base, followlinks=False): current_path = Path(current) - _chmod(current_path, PRIVATE_DIRECTORY_MODE) + _chmod(current_path, PRIVATE_DIRECTORY_MODE, force=True) directories[:] = [ name for name in directories if not (current_path / name).is_symlink() ] for name in directories: - _chmod(current_path / name, PRIVATE_DIRECTORY_MODE) + _chmod(current_path / name, PRIVATE_DIRECTORY_MODE, force=True) for name in files: ensure_private_file(current_path / name) return base @@ -207,11 +211,12 @@ def harden_private_tree(root: Path | str) -> Path: def _chmod(path: Path, mode: int, *, force: bool = False) -> None: if os.name == "nt": - # harden_private_tree deliberately re-applies the restriction even to - # existing paths (it repairs legacy trees whose ACLs may be absent or - # permissive), so _chmod restricts unconditionally. The per-open cost - # is avoided in open_private_file by only restricting new files. - _restrict_windows_acl(path) + # harden_private_tree (force=True) deliberately re-applies the + # restriction even to existing paths (it repairs legacy trees whose + # ACLs may be absent or permissive). Default callers pass force=False + # so an already-restricted path is not re-churned on every open. + if force: + _restrict_windows_acl(path) return try: os.chmod(path, mode, follow_symlinks=False) diff --git a/tests/test_private_storage_windows.py b/tests/test_private_storage_windows.py index 5393f97c..73dc4671 100644 --- a/tests/test_private_storage_windows.py +++ b/tests/test_private_storage_windows.py @@ -122,7 +122,9 @@ def test_windows_harden_private_tree_restricts_every_entry(tmp_path: Path) -> No _assert_current_user_has_full_control(path) -def test_windows_open_existing_file_does_not_rerun_acl(monkeypatch, tmp_path: Path) -> None: +def test_windows_open_existing_file_does_not_rerun_acl( + monkeypatch, tmp_path: Path +) -> None: """The per-open ACL re-run is gone: opening an existing private file does not call _restrict_windows_acl again (the ACL was applied at creation).""" import core.private_storage as ps From 07453752c9ebbd74695e453fbf00934ea65fbafa Mon Sep 17 00:00:00 2001 From: Zongwei9888 Date: Sun, 23 Aug 2026 17:41:43 +0800 Subject: [PATCH 19/23] fix(security): harden Windows ACL creation and repair --- core/private_storage.py | 166 +++++++++++++++++++------ tests/test_private_storage.py | 10 ++ tests/test_private_storage_acl_once.py | 98 +++++++++++++++ tests/test_private_storage_windows.py | 17 +++ 4 files changed, 250 insertions(+), 41 deletions(-) diff --git a/core/private_storage.py b/core/private_storage.py index ebe677e3..d6789952 100644 --- a/core/private_storage.py +++ b/core/private_storage.py @@ -16,6 +16,7 @@ import os import stat import subprocess +from functools import lru_cache from pathlib import Path PRIVATE_DIRECTORY_MODE = 0o700 @@ -26,25 +27,85 @@ class UnsafePrivateFileError(OSError): """A private-state path is not a regular file owned by this path entry.""" +_WINDOWS_BROAD_ACCESS_SIDS = ( + "*S-1-1-0", # Everyone + "*S-1-5-11", # Authenticated Users + "*S-1-5-32-545", # BUILTIN\\Users +) + + +@lru_cache(maxsize=1) def _windows_identity() -> str | None: """Return the fully-qualified current user (``DOMAIN\\user``) on Windows.""" if os.name != "nt": return None try: - completed = subprocess.run( - ["whoami"], + import ctypes + from ctypes import wintypes + + get_name = ctypes.WinDLL("secur32", use_last_error=True).GetUserNameExW + get_name.argtypes = ( + wintypes.ULONG, + wintypes.LPWSTR, + ctypes.POINTER(wintypes.ULONG), + ) + get_name.restype = wintypes.BOOL + size = wintypes.ULONG(0) + # NameSamCompatible (2) yields DOMAIN\user, the form icacls accepts. + get_name(2, None, ctypes.byref(size)) + if size.value <= 1: + return None + buffer = ctypes.create_unicode_buffer(size.value) + if not get_name(2, buffer, ctypes.byref(size)): + return None + principal = buffer.value.strip() + except (AttributeError, OSError, TypeError, ValueError): + return None + return principal or None + + +@lru_cache(maxsize=1) +def _windows_icacls() -> str | None: + """Resolve the trusted system icacls executable without consulting PATH.""" + + if os.name != "nt": + return None + try: + import ctypes + from ctypes import wintypes + + get_system_directory = ctypes.WinDLL( + "kernel32", use_last_error=True + ).GetSystemDirectoryW + get_system_directory.argtypes = (wintypes.LPWSTR, wintypes.UINT) + get_system_directory.restype = wintypes.UINT + buffer = ctypes.create_unicode_buffer(32_768) + length = get_system_directory(buffer, len(buffer)) + if length <= 0 or length >= len(buffer): + return None + executable = Path(buffer.value) / "icacls.exe" + return os.fspath(executable) if executable.is_file() else None + except (AttributeError, OSError, TypeError, ValueError): + return None + + +def _run_icacls(executable: str, path: Path, *arguments: str) -> bool: + """Run one bounded icacls operation and report whether it succeeded.""" + + try: + subprocess.run( + [executable, os.fspath(path), *arguments], capture_output=True, text=True, encoding="mbcs", errors="replace", - timeout=5, + timeout=15, check=True, ) except (OSError, subprocess.SubprocessError): - return None - principal = (completed.stdout or "").strip() - return principal or None + return False + return True def _restrict_windows_acl(path: Path) -> None: @@ -58,34 +119,51 @@ def _restrict_windows_acl(path: Path) -> None: """ identity = _windows_identity() - if identity is None: + executable = _windows_icacls() + if identity is None or executable is None: return - try: - subprocess.run( - ["icacls", os.fspath(path), "/grant:r", f"{identity}:F"], - capture_output=True, - text=True, - encoding="mbcs", - errors="replace", - timeout=15, - check=True, - ) - except (OSError, subprocess.SubprocessError): + grant = f"{identity}:{'(OI)(CI)' if path.is_dir() else ''}F" + if not _run_icacls(executable, path, "/grant:r", grant): # Fail safe: keep the inherited ACLs; the path stays accessible. return - try: - subprocess.run( - ["icacls", os.fspath(path), "/inheritance:r"], - capture_output=True, - text=True, - encoding="mbcs", - errors="replace", - timeout=15, - check=True, - ) - except (OSError, subprocess.SubprocessError): + if not _run_icacls(executable, path, "/inheritance:r"): # Strip failed: the path is merely less restricted, still usable. - pass + return + # ``/inheritance:r`` removes inherited ACEs but deliberately leaves + # explicit entries alone. Remove the broad built-in principals as a final + # best-effort step so hardening also repairs legacy explicit grants. + _run_icacls(executable, path, "/remove", *_WINDOWS_BROAD_ACCESS_SIDS) + + +def _open_private_descriptor(target: Path, flags: int) -> tuple[int, bool]: + """Open ``target`` and atomically report whether this call created it.""" + + open_flags = flags | getattr(os, "O_NOFOLLOW", 0) + if not flags & os.O_CREAT or flags & os.O_EXCL: + descriptor = os.open(target, open_flags, PRIVATE_FILE_MODE) + return descriptor, bool(flags & os.O_CREAT) + + # A pre-open exists() check races with another creator. First attempt an + # exclusive creation; if the path already exists, open it without O_CREAT + # so a concurrent delete is observable and can retry the decision. + while True: + try: + descriptor = os.open( + target, + open_flags | os.O_EXCL, + PRIVATE_FILE_MODE, + ) + return descriptor, True + except FileExistsError: + try: + descriptor = os.open( + target, + open_flags & ~os.O_CREAT, + PRIVATE_FILE_MODE, + ) + except FileNotFoundError: + continue + return descriptor, False def ensure_private_directory(path: Path | str) -> Path: @@ -115,17 +193,8 @@ def open_private_file(path: Path | str, flags: int) -> int: """Open a private regular file without following a final symlink.""" target = Path(path) - # Only restrict a *newly created* file. An existing file was already - # restricted at creation; re-running icacls on every open costs two - # subprocesses per call (and a full tree walk many times over) without - # changing the ACL (maintainer feedback on the earlier ACL PR). - created = not target.exists() ensure_private_directory(target.parent) - descriptor = os.open( - target, - flags | getattr(os, "O_NOFOLLOW", 0), - PRIVATE_FILE_MODE, - ) + descriptor, created = _open_private_descriptor(target, flags) try: metadata = target.lstat() opened = os.fstat(descriptor) @@ -195,12 +264,14 @@ def harden_private_tree(root: Path | str) -> Path: """Repair a DeepCode-owned tree while refusing to traverse symlinks.""" base = ensure_private_directory(root) + if _is_directory_link(base): + raise UnsafePrivateFileError("private storage root must not be a link") for current, directories, files in os.walk(base, followlinks=False): current_path = Path(current) _chmod(current_path, PRIVATE_DIRECTORY_MODE, force=True) directories[:] = [ - name for name in directories if not (current_path / name).is_symlink() + name for name in directories if not _is_directory_link(current_path / name) ] for name in directories: _chmod(current_path / name, PRIVATE_DIRECTORY_MODE, force=True) @@ -209,6 +280,19 @@ def harden_private_tree(root: Path | str) -> Path: return base +def _is_directory_link(path: Path) -> bool: + """Treat symlinks and Windows junctions as traversal boundaries.""" + + try: + if path.is_symlink(): + return True + is_junction = getattr(path, "is_junction", None) + return bool(callable(is_junction) and is_junction()) + except OSError: + # An unreadable/replaced entry is unsafe to traverse. + return True + + def _chmod(path: Path, mode: int, *, force: bool = False) -> None: if os.name == "nt": # harden_private_tree (force=True) deliberately re-applies the diff --git a/tests/test_private_storage.py b/tests/test_private_storage.py index 08b03165..78f3986a 100644 --- a/tests/test_private_storage.py +++ b/tests/test_private_storage.py @@ -154,6 +154,16 @@ def test_private_tree_repair_does_not_follow_symlinks(tmp_path: Path) -> None: assert _mode(external) == 0o644 +def test_private_tree_rejects_a_link_as_its_root(tmp_path: Path) -> None: + external = tmp_path / "external" + external.mkdir() + link = tmp_path / "private-link" + link.symlink_to(external, target_is_directory=True) + + with pytest.raises(OSError, match="must not be a link"): + harden_private_tree(link) + + def test_suite_redirects_all_user_state_to_the_test_directory( tmp_path: Path, ) -> None: diff --git a/tests/test_private_storage_acl_once.py b/tests/test_private_storage_acl_once.py index 72840d8d..2ea778d9 100644 --- a/tests/test_private_storage_acl_once.py +++ b/tests/test_private_storage_acl_once.py @@ -73,3 +73,101 @@ def test_open_without_creat_never_restricts(monkeypatch, tmp_path: Path) -> None assert _file_calls(calls, target) == 0, ( "read-only open of an existing file must not restrict" ) + + +def test_concurrent_creator_is_not_mistaken_for_our_new_file( + monkeypatch, tmp_path: Path +) -> None: + import core.private_storage as ps + + target = tmp_path / "raced.jsonl" + original_open = os.open + restrictions = [] + raced = False + + def racing_open(path, flags, mode=0o777): + nonlocal raced + if Path(path) == target and flags & os.O_EXCL and not raced: + raced = True + other = original_open(target, os.O_CREAT | os.O_EXCL | os.O_WRONLY, mode) + os.close(other) + raise FileExistsError(os.fspath(target)) + return original_open(path, flags, mode) + + monkeypatch.setattr(ps.os, "open", racing_open) + monkeypatch.setattr( + ps, "_restrict_windows_acl", lambda path: restrictions.append(Path(path)) + ) + + descriptor = ps.open_private_file(target, os.O_CREAT | os.O_RDWR) + os.close(descriptor) + + assert raced is True + assert target not in restrictions + + +def test_acl_restriction_orders_grant_strip_and_broad_principal_removal( + monkeypatch, tmp_path: Path +) -> None: + import core.private_storage as ps + + calls = [] + monkeypatch.setattr(ps, "_windows_identity", lambda: "DOMAIN\\user") + monkeypatch.setattr(ps, "_windows_icacls", lambda: "trusted-icacls.exe") + + def record(executable, path, *arguments): + calls.append((executable, Path(path), arguments)) + return True + + monkeypatch.setattr(ps, "_run_icacls", record) + + directory = tmp_path / "private" + directory.mkdir() + ps._restrict_windows_acl(directory) + + assert calls[0][2] == ("/grant:r", "DOMAIN\\user:(OI)(CI)F") + assert calls[1][2] == ("/inheritance:r",) + assert calls[2][2] == ("/remove", *ps._WINDOWS_BROAD_ACCESS_SIDS) + + +def test_acl_restriction_stops_before_strip_when_grant_fails( + monkeypatch, tmp_path: Path +) -> None: + import core.private_storage as ps + + calls = [] + monkeypatch.setattr(ps, "_windows_identity", lambda: "DOMAIN\\user") + monkeypatch.setattr(ps, "_windows_icacls", lambda: "trusted-icacls.exe") + + def fail_grant(executable, path, *arguments): + calls.append(arguments) + return False + + monkeypatch.setattr(ps, "_run_icacls", fail_grant) + + ps._restrict_windows_acl(tmp_path / "private.json") + + assert calls == [("/grant:r", "DOMAIN\\user:F")] + + +def test_acl_restriction_does_not_remove_entries_when_strip_fails( + monkeypatch, tmp_path: Path +) -> None: + import core.private_storage as ps + + calls = [] + monkeypatch.setattr(ps, "_windows_identity", lambda: "DOMAIN\\user") + monkeypatch.setattr(ps, "_windows_icacls", lambda: "trusted-icacls.exe") + + def fail_strip(executable, path, *arguments): + calls.append(arguments) + return arguments != ("/inheritance:r",) + + monkeypatch.setattr(ps, "_run_icacls", fail_strip) + + ps._restrict_windows_acl(tmp_path / "private.json") + + assert calls == [ + ("/grant:r", "DOMAIN\\user:F"), + ("/inheritance:r",), + ] diff --git a/tests/test_private_storage_windows.py b/tests/test_private_storage_windows.py index 73dc4671..83d73311 100644 --- a/tests/test_private_storage_windows.py +++ b/tests/test_private_storage_windows.py @@ -94,6 +94,14 @@ def test_windows_private_directory_is_restricted(tmp_path: Path) -> None: _assert_no_dangerous_aces(directory) _assert_current_user_has_full_control(directory) + # The directory grant must be inheritable so files created by libraries + # that do not call open_private_file (SQLite WAL files, lock files, etc.) + # remain accessible without inheriting broad user-group access. + child = directory / "library-created.tmp" + child.write_text("x", encoding="utf-8") + _assert_no_dangerous_aces(child) + _assert_current_user_has_full_control(child) + def test_windows_private_file_is_restricted(tmp_path: Path) -> None: target = tmp_path / "private" / "credentials.json" @@ -114,6 +122,15 @@ def test_windows_harden_private_tree_restricts_every_entry(tmp_path: Path) -> No session.mkdir(parents=True) (session / "session.jsonl").write_text("legacy\n", encoding="utf-8") (root / "settings.json").write_text("{}", encoding="utf-8") + subprocess.run( + ["icacls", os.fspath(root / "settings.json"), "/grant", "*S-1-1-0:R"], + capture_output=True, + text=True, + encoding="mbcs", + errors="replace", + timeout=15, + check=True, + ) harden_private_tree(root) From 4e9c2d0813ab33709cb8b04bc08065d25b90a4ef Mon Sep 17 00:00:00 2001 From: raymondginger2018-sudo Date: Mon, 17 Aug 2026 12:24:52 +0800 Subject: [PATCH 20/23] feat(mcp): lazy server activation (deferLoading + activate_server) Add McpServerDefinition.defer_loading (deferLoading). Servers marked deferred are skipped by ensure_started (status stays 'deferred', no connection, no tools) and are brought up on demand via McpSessionRuntime.activate_server, which starts the connection, registers the server's tools into the ToolRegistry, and publishes status/capabilities. Idempotent; startup failures mark the server 'failed' without raising. Derived from the MCP lifecycle study of Hmbown/CodeWhale. --- core/mcp/models.py | 4 + core/mcp/runtime.py | 169 +++++++++++++++++++++++++--------- core/mcp/test_runtime_lazy.py | 147 +++++++++++++++++++++++++++++ 3 files changed, 276 insertions(+), 44 deletions(-) create mode 100644 core/mcp/test_runtime_lazy.py diff --git a/core/mcp/models.py b/core/mcp/models.py index 67c9a812..0fde6466 100644 --- a/core/mcp/models.py +++ b/core/mcp/models.py @@ -154,6 +154,10 @@ class McpServerDefinition(_ConfigModel): enabled: bool = True required: bool = False + defer_loading: bool = Field( + default=False, + validation_alias=AliasChoices("deferLoading", "defer_loading"), + ) supports_parallel_tool_calls: bool = False startup_timeout_seconds: Annotated[float, Field(gt=0, le=300)] = Field( default=10.0, diff --git a/core/mcp/runtime.py b/core/mcp/runtime.py index 23527967..92bf4d1a 100644 --- a/core/mcp/runtime.py +++ b/core/mcp/runtime.py @@ -104,6 +104,11 @@ async def ensure_started(self) -> None: if not self.plan.servers: self._started = True return + deferred_ids = { + server.server_id + for server in self.plan.servers + if server.definition.defer_loading + } connections = { server.server_id: McpConnection( server, @@ -111,13 +116,14 @@ async def ensure_started(self) -> None: oauth_provider_factory=self._oauth_provider_factory, ) for server in self.plan.servers + if server.server_id not in deferred_ids } for server_id, status in tuple(self._statuses.items()): self._statuses[server_id] = McpServerRuntimeStatus( status.server_id, status.name, status.source, - "starting", + "deferred" if server_id in deferred_ids else "starting", 0, ) self._publish_statuses() @@ -165,54 +171,129 @@ async def ensure_started(self) -> None: used = set(self.registry.tool_names) registered: list[str] = [] for connection, definitions in ready: - server = connection.server - seen_raw: set[str] = set() - exposed_count = 0 - server_tools: list[str] = [] - for definition in sorted(definitions, key=lambda item: str(item.name)): - raw_name = str(definition.name) - if raw_name in seen_raw: - logger.warning( - "MCP server '{}' returned duplicate tool name '{}'", - server.name, - raw_name, - ) - continue - seen_raw.add(raw_name) - if not server.definition.exposes(raw_name): - continue - name = visible_tool_name(server.server_id, raw_name, used=used) - adapter = McpToolAdapter( - connection, - definition, - visible_name=name, - ) - self.registry.register(adapter) - registered.append(name) - server_tools.append(name) - exposed_count += 1 - self._warn_unmatched_filters(server, seen_raw) - tools = tuple(server_tools) - self._capabilities[server.server_id] = tools - if server.policy_key is not None: - self._capabilities[server.policy_key] = tools - if ( - server.plugin_id is not None - and server.plugin_server_name is not None - ): - self._capabilities[ - f"{server.plugin_id}:{server.plugin_server_name}" - ] = tools - self._statuses[server.server_id] = McpServerRuntimeStatus( + registered.extend( + self._register_server_tools(connection, definitions, used=used) + ) + self._registered_tools = tuple(registered) + self._started = True + self._publish_statuses() + + async def activate_server(self, server_id: str) -> bool: + """Start one MCP server on demand and register its tools. + + Deferred servers (``deferLoading: true``) are skipped by + :meth:`ensure_started`; call this to bring one up when it is actually + needed. Idempotent: returns ``True`` immediately when the server is + already ready. Returns ``False`` for unknown servers or when startup + fails (status is set to ``failed``). + """ + if not self._started: + await self.ensure_started() + async with self._lock: + if self._closed: + return False + existing = self._connections.get(server_id) + if existing is not None and existing.ready: + return True + server = next( + (s for s in self.plan.servers if s.server_id == server_id), + None, + ) + if server is None: + return False + self._statuses[server_id] = McpServerRuntimeStatus( + server.server_id, + server.name, + server.source.value, + "starting", + 0, + ) + connection = McpConnection( + server, + credential_resolver=self._credential_resolver, + oauth_provider_factory=self._oauth_provider_factory, + ) + try: + definitions = await connection.start() + except BaseException as exc: # noqa: BLE001 - startup boundary + error = f"{type(exc).__name__}: {exc}" + self._statuses[server_id] = McpServerRuntimeStatus( server.server_id, server.name, server.source.value, - "ready", - exposed_count, + "failed", + 0, + error, ) - self._registered_tools = tuple(registered) - self._started = True + logger.warning( + "MCP server '{}' activation failed: {}", server.name, error + ) + await connection.close() + self._publish_statuses() + return False + self._connections[server_id] = connection + registered = self._register_server_tools( + connection, + definitions, + used=set(self.registry.tool_names), + ) + self._registered_tools = (*self._registered_tools, *registered) self._publish_statuses() + return True + + def _register_server_tools( + self, + connection: McpConnection, + definitions: tuple[Any, ...], + *, + used: set[str], + ) -> list[str]: + """Register one ready server's tools; returns the visible names.""" + server = connection.server + seen_raw: set[str] = set() + exposed_count = 0 + server_tools: list[str] = [] + for definition in sorted(definitions, key=lambda item: str(item.name)): + raw_name = str(definition.name) + if raw_name in seen_raw: + logger.warning( + "MCP server '{}' returned duplicate tool name '{}'", + server.name, + raw_name, + ) + continue + seen_raw.add(raw_name) + if not server.definition.exposes(raw_name): + continue + name = visible_tool_name(server.server_id, raw_name, used=used) + adapter = McpToolAdapter( + connection, + definition, + visible_name=name, + ) + self.registry.register(adapter) + server_tools.append(name) + exposed_count += 1 + self._warn_unmatched_filters(server, seen_raw) + tools = tuple(server_tools) + self._capabilities[server.server_id] = tools + if server.policy_key is not None: + self._capabilities[server.policy_key] = tools + if ( + server.plugin_id is not None + and server.plugin_server_name is not None + ): + self._capabilities[ + f"{server.plugin_id}:{server.plugin_server_name}" + ] = tools + self._statuses[server.server_id] = McpServerRuntimeStatus( + server.server_id, + server.name, + server.source.value, + "ready", + exposed_count, + ) + return server_tools async def aclose(self) -> None: async with self._lock: diff --git a/core/mcp/test_runtime_lazy.py b/core/mcp/test_runtime_lazy.py new file mode 100644 index 00000000..84a7fe80 --- /dev/null +++ b/core/mcp/test_runtime_lazy.py @@ -0,0 +1,147 @@ +"""Lazy MCP server activation (deferLoading) tests. + +``McpServerDefinition.defer_loading`` skips a server at ``ensure_started``; +``McpSessionRuntime.activate_server`` brings it up on demand and registers its +tools. This mirrors the CLI-side lazy-connect design (CodeWhale-derived). +""" + +from __future__ import annotations + +import sys +import tempfile +import textwrap +import unittest +from pathlib import Path + +from core.agent_runtime.tools.registry import ToolRegistry +from core.mcp.models import ( + McpRuntimePlan, + McpServerDefinition, + McpServerSource, + ResolvedMcpServer, +) +from core.mcp.runtime import McpSessionRuntime + +FAKE_SERVER_SRC = textwrap.dedent( + """ + import json, sys + for line in sys.stdin: + msg = json.loads(line) + method = msg.get("method") + if method == "initialize": + json.dump({"jsonrpc": "2.0", "id": msg["id"], "result": { + "protocolVersion": "2025-03-26", + "capabilities": {"tools": {}}, + "serverInfo": {"name": "fake", "version": "1.0"}}}, sys.stdout) + print(flush=True) + elif method == "tools/list": + json.dump({"jsonrpc": "2.0", "id": msg["id"], "result": {"tools": [ + {"name": "echo", "description": "echo back", + "inputSchema": {"type": "object", "properties": {}}}]}}, + sys.stdout) + print(flush=True) + """ +) + + +class LazyActivationTests(unittest.IsolatedAsyncioTestCase): + def setUp(self) -> None: + self.tmp = tempfile.mkdtemp(prefix="deepcode-mcp-lazy-") + self.fake_server = Path(self.tmp) / "fake_mcp_server.py" + self.fake_server.write_text(FAKE_SERVER_SRC, encoding="utf-8") + + def _server(self, server_id: str, defer_loading: bool) -> ResolvedMcpServer: + return ResolvedMcpServer( + server_id=server_id, + name=server_id, + source=McpServerSource.USER, + definition=McpServerDefinition( + type="stdio", + command=sys.executable, + args=(str(self.fake_server),), + startup_timeout_seconds=10.0, + defer_loading=defer_loading, + ), + config_dir=Path(self.tmp), + workspace=Path(self.tmp), + ) + + def _plan(self, *servers: ResolvedMcpServer) -> McpRuntimePlan: + return McpRuntimePlan( + workspace=Path(self.tmp), + servers=tuple(servers), + revision="test", + ) + + async def test_deferred_server_not_started_at_ensure(self) -> None: + runtime = McpSessionRuntime( + self._plan(self._server("eager1", False), self._server("lazy1", True)), + ToolRegistry(), + ) + await runtime.ensure_started() + statuses = {s.server_id: s for s in runtime.statuses} + self.assertEqual(statuses["eager1"].state, "ready") + self.assertEqual(statuses["lazy1"].state, "deferred") + self.assertNotIn("lazy1", runtime.available_server_ids) + # only the eager server's tool is registered + self.assertIn("eager1", runtime.skill_capabilities) + self.assertNotIn("lazy1", runtime.skill_capabilities) + + async def test_activate_server_registers_tools(self) -> None: + runtime = McpSessionRuntime( + self._plan(self._server("eager1", False), self._server("lazy1", True)), + ToolRegistry(), + ) + await runtime.ensure_started() + self.assertTrue(await runtime.activate_server("lazy1")) + statuses = {s.server_id: s for s in runtime.statuses} + self.assertEqual(statuses["lazy1"].state, "ready") + self.assertIn("lazy1", runtime.available_server_ids) + self.assertIn("lazy1", runtime.skill_capabilities) + self.assertGreater(len(runtime.skill_capabilities["lazy1"]), 0) + + async def test_activate_is_idempotent(self) -> None: + runtime = McpSessionRuntime( + self._plan(self._server("lazy1", True)), + ToolRegistry(), + ) + await runtime.ensure_started() + self.assertTrue(await runtime.activate_server("lazy1")) + tools_after_first = set(runtime.skill_capabilities.get("lazy1", ())) + self.assertTrue(await runtime.activate_server("lazy1")) + tools_after_second = set(runtime.skill_capabilities.get("lazy1", ())) + self.assertEqual(tools_after_first, tools_after_second) + + async def test_activate_unknown_server_returns_false(self) -> None: + runtime = McpSessionRuntime( + self._plan(self._server("lazy1", True)), + ToolRegistry(), + ) + await runtime.ensure_started() + self.assertFalse(await runtime.activate_server("no-such-server")) + + async def test_activate_failure_marks_failed(self) -> None: + broken = ResolvedMcpServer( + server_id="broken1", + name="broken1", + source=McpServerSource.USER, + definition=McpServerDefinition( + type="stdio", + command=sys.executable, + args=("C:/definitely_missing_mcp_server_xyz.py",), + startup_timeout_seconds=5.0, + defer_loading=True, + ), + config_dir=Path(self.tmp), + workspace=Path(self.tmp), + ) + runtime = McpSessionRuntime(self._plan(broken), ToolRegistry()) + await runtime.ensure_started() + self.assertFalse(await runtime.activate_server("broken1")) + statuses = {s.server_id: s for s in runtime.statuses} + self.assertEqual(statuses["broken1"].state, "failed") + self.assertIsNotNone(statuses["broken1"].error) + + +if __name__ == "__main__": + unittest.main() From 04ca1000f48afcf7c6c7aca03b4abffead552d61 Mon Sep 17 00:00:00 2001 From: raymondginger Date: Sun, 23 Aug 2026 06:23:19 +0800 Subject: [PATCH 21/23] style: ruff format runtime.py (CI lint fix) --- core/mcp/runtime.py | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/core/mcp/runtime.py b/core/mcp/runtime.py index 92bf4d1a..5a4afadb 100644 --- a/core/mcp/runtime.py +++ b/core/mcp/runtime.py @@ -279,10 +279,7 @@ def _register_server_tools( self._capabilities[server.server_id] = tools if server.policy_key is not None: self._capabilities[server.policy_key] = tools - if ( - server.plugin_id is not None - and server.plugin_server_name is not None - ): + if server.plugin_id is not None and server.plugin_server_name is not None: self._capabilities[ f"{server.plugin_id}:{server.plugin_server_name}" ] = tools From b51c961c00115185b53a4ca69dc604f6a91c3c78 Mon Sep 17 00:00:00 2001 From: raymondginger Date: Sun, 23 Aug 2026 06:35:18 +0800 Subject: [PATCH 22/23] test(mcp): move lazy activation tests to tests/ per repo convention core/mcp/test_runtime_lazy.py sat outside pytest testpaths (tests/ + quant_trading/tests), so CI never collected the 5 new cases. Move it to tests/test_mcp_runtime_lazy.py; now collected and run by CI. --- core/mcp/test_runtime_lazy.py => tests/test_mcp_runtime_lazy.py | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename core/mcp/test_runtime_lazy.py => tests/test_mcp_runtime_lazy.py (100%) diff --git a/core/mcp/test_runtime_lazy.py b/tests/test_mcp_runtime_lazy.py similarity index 100% rename from core/mcp/test_runtime_lazy.py rename to tests/test_mcp_runtime_lazy.py From eb8992a4d6cd6f9a60a74192aeaaccab9dbade2b Mon Sep 17 00:00:00 2001 From: Zongwei9888 Date: Sun, 23 Aug 2026 17:20:20 +0800 Subject: [PATCH 23/23] fix(mcp): make deferred servers reachable and cancellation-safe --- core/mcp/models.py | 3 + core/mcp/runtime.py | 111 +++++++++++++++++++++++++++++- tests/test_mcp_runtime_lazy.py | 122 +++++++++++++++++++++++++-------- 3 files changed, 208 insertions(+), 28 deletions(-) diff --git a/core/mcp/models.py b/core/mcp/models.py index 0fde6466..a16f4d95 100644 --- a/core/mcp/models.py +++ b/core/mcp/models.py @@ -270,6 +270,9 @@ def _valid_tool_names( @model_validator(mode="after") def _transport_contract(self) -> McpServerDefinition: + if self.required and self.defer_loading: + raise ValueError("required MCP servers cannot defer loading") + if self.bearer_token_env_var is not None and not _ENV_NAME.fullmatch( self.bearer_token_env_var ): diff --git a/core/mcp/runtime.py b/core/mcp/runtime.py index 5a4afadb..97596685 100644 --- a/core/mcp/runtime.py +++ b/core/mcp/runtime.py @@ -9,6 +9,7 @@ from loguru import logger +from core.agent_runtime.tools.base import Tool, ToolResult from core.agent_runtime.tools.registry import ToolRegistry from core.mcp.connection import CredentialResolver, McpConnection, OAuthProviderFactory from core.mcp.models import McpRuntimePlan, McpStartupError @@ -26,6 +27,74 @@ class McpServerRuntimeStatus: error: str | None = None +class _ActivateMcpServerTool(Tool): + """Model-visible bridge that makes deferred servers reachable.""" + + def __init__( + self, + runtime: McpSessionRuntime, + *, + name: str, + server_ids: tuple[str, ...], + ) -> None: + self._runtime = runtime + self._name = name + self._server_ids = server_ids + + @property + def name(self) -> str: + return self._name + + @property + def description(self) -> str: + available = ", ".join(self._server_ids) + return ( + "Start one configured deferred MCP server so its tools become " + f"available to this session. Deferred server ids: {available}." + ) + + @property + def parameters(self) -> dict[str, Any]: + return { + "type": "object", + "properties": { + "server_id": { + "type": "string", + "enum": list(self._server_ids), + "description": "Configured MCP server id to activate.", + } + }, + "required": ["server_id"], + "additionalProperties": False, + } + + def presentation_detail(self, arguments: dict[str, Any]) -> str | None: + value = arguments.get("server_id") + return value if isinstance(value, str) else "" + + async def execute(self, *, server_id: str) -> ToolResult: + activated = await self._runtime.activate_server(server_id) + if not activated: + return ToolResult( + f"Error: MCP server '{server_id}' could not be activated.", + is_error=True, + metadata={"serverId": server_id, "activated": False}, + ) + tools = self._runtime.skill_capabilities.get(server_id, ()) + suffix = f" Available tools: {', '.join(tools)}." if tools else "" + instructions = self._runtime.server_instruction(server_id) + if instructions: + suffix += f"\n\nServer instructions:\n{instructions}" + return ToolResult( + f"MCP server '{server_id}' activated.{suffix}", + metadata={ + "serverId": server_id, + "activated": True, + "tools": list(tools), + }, + ) + + class McpSessionRuntime: """Materialize one immutable MCP plan into a DeepCode ToolRegistry.""" @@ -95,6 +164,14 @@ def instruction_context(self) -> str | None: return None return "\n\n".join(sections)[:8_000] + def server_instruction(self, server_id: str) -> str | None: + """Return bounded instructions for one active server.""" + + connection = self._connections.get(server_id) + if connection is None or not connection.instructions: + return None + return str(connection.instructions)[:4_000] + async def ensure_started(self) -> None: async with self._lock: if self._closed: @@ -174,6 +251,20 @@ async def ensure_started(self) -> None: registered.extend( self._register_server_tools(connection, definitions, used=used) ) + if deferred_ids: + activation_name = visible_tool_name( + "deepcode_runtime", + "activate_server", + used=used, + ) + self.registry.register( + _ActivateMcpServerTool( + self, + name=activation_name, + server_ids=tuple(sorted(deferred_ids)), + ) + ) + registered.append(activation_name) self._registered_tools = tuple(registered) self._started = True self._publish_statuses() @@ -188,7 +279,14 @@ async def activate_server(self, server_id: str) -> bool: fails (status is set to ``failed``). """ if not self._started: - await self.ensure_started() + if self._closed: + return False + try: + await self.ensure_started() + except RuntimeError: + if self._closed: + return False + raise async with self._lock: if self._closed: return False @@ -215,6 +313,17 @@ async def activate_server(self, server_id: str) -> bool: ) try: definitions = await connection.start() + except asyncio.CancelledError: + await connection.close() + self._statuses[server_id] = McpServerRuntimeStatus( + server.server_id, + server.name, + server.source.value, + "deferred" if server.definition.defer_loading else "failed", + 0, + ) + self._publish_statuses() + raise except BaseException as exc: # noqa: BLE001 - startup boundary error = f"{type(exc).__name__}: {exc}" self._statuses[server_id] = McpServerRuntimeStatus( diff --git a/tests/test_mcp_runtime_lazy.py b/tests/test_mcp_runtime_lazy.py index 84a7fe80..452c5fdb 100644 --- a/tests/test_mcp_runtime_lazy.py +++ b/tests/test_mcp_runtime_lazy.py @@ -7,11 +7,16 @@ from __future__ import annotations +import asyncio import sys import tempfile import textwrap import unittest from pathlib import Path +from unittest.mock import patch + +import pytest +from pydantic import ValidationError from core.agent_runtime.tools.registry import ToolRegistry from core.mcp.models import ( @@ -74,31 +79,46 @@ def _plan(self, *servers: ResolvedMcpServer) -> McpRuntimePlan: ) async def test_deferred_server_not_started_at_ensure(self) -> None: + registry = ToolRegistry() runtime = McpSessionRuntime( self._plan(self._server("eager1", False), self._server("lazy1", True)), - ToolRegistry(), + registry, ) - await runtime.ensure_started() - statuses = {s.server_id: s for s in runtime.statuses} - self.assertEqual(statuses["eager1"].state, "ready") - self.assertEqual(statuses["lazy1"].state, "deferred") - self.assertNotIn("lazy1", runtime.available_server_ids) - # only the eager server's tool is registered - self.assertIn("eager1", runtime.skill_capabilities) - self.assertNotIn("lazy1", runtime.skill_capabilities) + try: + await runtime.ensure_started() + statuses = {s.server_id: s for s in runtime.statuses} + self.assertEqual(statuses["eager1"].state, "ready") + self.assertEqual(statuses["lazy1"].state, "deferred") + self.assertNotIn("lazy1", runtime.available_server_ids) + # Only the eager server's remote tool is registered, plus the + # activation bridge that makes deferred servers reachable. + self.assertIn("eager1", runtime.skill_capabilities) + self.assertNotIn("lazy1", runtime.skill_capabilities) + self.assertIn("mcp__deepcode_runtime__activate_server", registry.tool_names) + finally: + await runtime.aclose() + self.assertNotIn("mcp__deepcode_runtime__activate_server", registry.tool_names) async def test_activate_server_registers_tools(self) -> None: + registry = ToolRegistry() runtime = McpSessionRuntime( self._plan(self._server("eager1", False), self._server("lazy1", True)), - ToolRegistry(), + registry, ) - await runtime.ensure_started() - self.assertTrue(await runtime.activate_server("lazy1")) - statuses = {s.server_id: s for s in runtime.statuses} - self.assertEqual(statuses["lazy1"].state, "ready") - self.assertIn("lazy1", runtime.available_server_ids) - self.assertIn("lazy1", runtime.skill_capabilities) - self.assertGreater(len(runtime.skill_capabilities["lazy1"]), 0) + try: + await runtime.ensure_started() + result = await registry.execute( + "mcp__deepcode_runtime__activate_server", {"server_id": "lazy1"} + ) + self.assertIn("activated", str(result)) + statuses = {s.server_id: s for s in runtime.statuses} + self.assertEqual(statuses["lazy1"].state, "ready") + self.assertIn("lazy1", runtime.available_server_ids) + self.assertIn("lazy1", runtime.skill_capabilities) + self.assertGreater(len(runtime.skill_capabilities["lazy1"]), 0) + self.assertIn("mcp__lazy1__echo", registry.tool_names) + finally: + await runtime.aclose() async def test_activate_is_idempotent(self) -> None: runtime = McpSessionRuntime( @@ -106,11 +126,14 @@ async def test_activate_is_idempotent(self) -> None: ToolRegistry(), ) await runtime.ensure_started() - self.assertTrue(await runtime.activate_server("lazy1")) - tools_after_first = set(runtime.skill_capabilities.get("lazy1", ())) - self.assertTrue(await runtime.activate_server("lazy1")) - tools_after_second = set(runtime.skill_capabilities.get("lazy1", ())) - self.assertEqual(tools_after_first, tools_after_second) + try: + self.assertTrue(await runtime.activate_server("lazy1")) + tools_after_first = set(runtime.skill_capabilities.get("lazy1", ())) + self.assertTrue(await runtime.activate_server("lazy1")) + tools_after_second = set(runtime.skill_capabilities.get("lazy1", ())) + self.assertEqual(tools_after_first, tools_after_second) + finally: + await runtime.aclose() async def test_activate_unknown_server_returns_false(self) -> None: runtime = McpSessionRuntime( @@ -118,7 +141,39 @@ async def test_activate_unknown_server_returns_false(self) -> None: ToolRegistry(), ) await runtime.ensure_started() - self.assertFalse(await runtime.activate_server("no-such-server")) + try: + self.assertFalse(await runtime.activate_server("no-such-server")) + finally: + await runtime.aclose() + + async def test_activate_after_close_returns_false(self) -> None: + runtime = McpSessionRuntime( + self._plan(self._server("lazy1", True)), + ToolRegistry(), + ) + await runtime.aclose() + self.assertFalse(await runtime.activate_server("lazy1")) + + async def test_activation_cancellation_propagates_and_restores_state(self) -> None: + runtime = McpSessionRuntime( + self._plan(self._server("lazy1", True)), + ToolRegistry(), + ) + await runtime.ensure_started() + + async def cancel_start(_connection): + raise asyncio.CancelledError + + try: + with ( + patch("core.mcp.runtime.McpConnection.start", cancel_start), + self.assertRaises(asyncio.CancelledError), + ): + await runtime.activate_server("lazy1") + statuses = {s.server_id: s for s in runtime.statuses} + self.assertEqual(statuses["lazy1"].state, "deferred") + finally: + await runtime.aclose() async def test_activate_failure_marks_failed(self) -> None: broken = ResolvedMcpServer( @@ -137,10 +192,23 @@ async def test_activate_failure_marks_failed(self) -> None: ) runtime = McpSessionRuntime(self._plan(broken), ToolRegistry()) await runtime.ensure_started() - self.assertFalse(await runtime.activate_server("broken1")) - statuses = {s.server_id: s for s in runtime.statuses} - self.assertEqual(statuses["broken1"].state, "failed") - self.assertIsNotNone(statuses["broken1"].error) + try: + self.assertFalse(await runtime.activate_server("broken1")) + statuses = {s.server_id: s for s in runtime.statuses} + self.assertEqual(statuses["broken1"].state, "failed") + self.assertIsNotNone(statuses["broken1"].error) + finally: + await runtime.aclose() + + +def test_required_server_cannot_be_deferred() -> None: + with pytest.raises(ValidationError, match="required MCP servers cannot defer"): + McpServerDefinition( + type="stdio", + command=sys.executable, + required=True, + defer_loading=True, + ) if __name__ == "__main__":