fix(claude_session): surface real CLI error + sanitize nested env (#85) #90
@@ -29,6 +29,7 @@ from __future__ import annotations
|
||||
import asyncio
|
||||
import json
|
||||
import logging
|
||||
import os
|
||||
|
||||
from legal_mcp.config import parse_llm_json
|
||||
|
||||
@@ -40,6 +41,32 @@ logger = logging.getLogger(__name__)
|
||||
DEFAULT_TIMEOUT = 1800
|
||||
LONG_TIMEOUT = 3600 # opus block writing on full case context
|
||||
|
||||
# Environment markers a running Claude Code session exports into its child
|
||||
# processes. When an agent itself runs as ``claude -p`` (e.g. the CEO MCP
|
||||
# instance launched by Paperclip's claude_local adapter), its MCP server
|
||||
# inherits these, and a *nested* ``claude -p`` we spawn here inherits them
|
||||
# too. Stripping them makes every nested invocation launch as a clean
|
||||
# top-level session, decoupled from the parent's session/project state —
|
||||
# defensive hardening against the #85 nested-``exit 1`` failure (which could
|
||||
# not be reproduced in a plain interactive session, so the markers are a
|
||||
# suspect, not a proven cause). Auth/config vars (CLAUDE_CONFIG_DIR,
|
||||
# ANTHROPIC_*, PATH, HOME) are kept. See TaskMaster legal-ai #85.
|
||||
_SESSION_MARKER_PREFIXES = ("CLAUDECODE", "CLAUDE_CODE_", "CLAUDE_AGENT_")
|
||||
_SESSION_MARKER_EXACT = frozenset({"AI_AGENT", "CLAUDE_EFFORT"})
|
||||
|
||||
|
||||
def _clean_subprocess_env() -> dict[str, str]:
|
||||
"""Copy the current env minus Claude Code session markers.
|
||||
|
||||
Lets a nested ``claude -p`` start fresh instead of detecting it is
|
||||
already inside a Claude Code session (the #85 nested-exit-1 bug).
|
||||
"""
|
||||
env = dict(os.environ)
|
||||
for key in list(env):
|
||||
if key in _SESSION_MARKER_EXACT or key.startswith(_SESSION_MARKER_PREFIXES):
|
||||
del env[key]
|
||||
return env
|
||||
|
||||
|
||||
async def query(
|
||||
prompt: str,
|
||||
@@ -100,6 +127,8 @@ async def query(
|
||||
stdin=asyncio.subprocess.PIPE,
|
||||
stdout=asyncio.subprocess.PIPE,
|
||||
stderr=asyncio.subprocess.PIPE,
|
||||
env=_clean_subprocess_env(),
|
||||
cwd=os.path.expanduser("~"),
|
||||
)
|
||||
except FileNotFoundError:
|
||||
raise RuntimeError(
|
||||
@@ -125,9 +154,20 @@ async def query(
|
||||
raise RuntimeError(f"Claude CLI timed out after {timeout}s")
|
||||
|
||||
if proc.returncode != 0:
|
||||
stderr = stderr_b.decode("utf-8", errors="replace").strip()[:500] or "unknown error"
|
||||
# Surface the real cause: the CLI sometimes writes its diagnostic to
|
||||
# stdout (or nowhere) rather than stderr, so a stderr-only message
|
||||
# collapsed to "unknown error" and hid the actual failure (#85).
|
||||
stderr = stderr_b.decode("utf-8", errors="replace").strip()
|
||||
stdout = stdout_b.decode("utf-8", errors="replace").strip()
|
||||
diagnostic = stderr or stdout or "no output on stderr/stdout"
|
||||
size_info = f"; prompt_len={len(full_prompt):,} chars" if len(full_prompt) > 100_000 else ""
|
||||
raise RuntimeError(f"Claude CLI failed (exit {proc.returncode}): {stderr}{size_info}")
|
||||
logger.error(
|
||||
"claude -p failed (exit %s): stderr=%r stdout=%r",
|
||||
proc.returncode, stderr[:500], stdout[:500],
|
||||
)
|
||||
raise RuntimeError(
|
||||
f"Claude CLI failed (exit {proc.returncode}): {diagnostic[:500]}{size_info}"
|
||||
)
|
||||
|
||||
stdout = stdout_b.decode("utf-8", errors="replace").strip()
|
||||
if not stdout:
|
||||
@@ -232,6 +272,7 @@ async def query_streaming(
|
||||
stdout=asyncio.subprocess.PIPE,
|
||||
stderr=asyncio.subprocess.PIPE,
|
||||
cwd=cwd,
|
||||
env=_clean_subprocess_env(),
|
||||
)
|
||||
except FileNotFoundError:
|
||||
yield {
|
||||
|
||||
44
mcp-server/tests/test_claude_session.py
Normal file
44
mcp-server/tests/test_claude_session.py
Normal file
@@ -0,0 +1,44 @@
|
||||
from __future__ import annotations
|
||||
|
||||
import os
|
||||
|
||||
from legal_mcp.services import claude_session as cs
|
||||
|
||||
|
||||
def test_clean_env_strips_session_markers(monkeypatch):
|
||||
"""Nested claude -p must not inherit the parent session markers (#85)."""
|
||||
for k in (
|
||||
"CLAUDECODE",
|
||||
"CLAUDE_CODE_ENTRYPOINT",
|
||||
"CLAUDE_CODE_SESSION_ID",
|
||||
"CLAUDE_CODE_EXECPATH",
|
||||
"CLAUDE_CODE_SSE_PORT",
|
||||
"CLAUDE_AGENT_SDK_VERSION",
|
||||
"AI_AGENT",
|
||||
"CLAUDE_EFFORT",
|
||||
):
|
||||
monkeypatch.setenv(k, "x")
|
||||
|
||||
env = cs._clean_subprocess_env()
|
||||
|
||||
assert "CLAUDECODE" not in env
|
||||
assert "AI_AGENT" not in env
|
||||
assert "CLAUDE_EFFORT" not in env
|
||||
assert not any(k.startswith("CLAUDE_CODE_") for k in env)
|
||||
assert not any(k.startswith("CLAUDE_AGENT_") for k in env)
|
||||
|
||||
|
||||
def test_clean_env_keeps_auth_and_path(monkeypatch):
|
||||
"""Auth/config + PATH/HOME must survive — they are needed by the CLI."""
|
||||
monkeypatch.setenv("CLAUDECODE", "1")
|
||||
monkeypatch.setenv("CLAUDE_CONFIG_DIR", "/home/chaim/.claude")
|
||||
monkeypatch.setenv("ANTHROPIC_BASE_URL", "https://example")
|
||||
monkeypatch.setenv("PATH", os.environ.get("PATH", "/usr/bin"))
|
||||
|
||||
env = cs._clean_subprocess_env()
|
||||
|
||||
# CLAUDE_CONFIG_DIR carries credentials — must NOT be stripped.
|
||||
assert env.get("CLAUDE_CONFIG_DIR") == "/home/chaim/.claude"
|
||||
assert env.get("ANTHROPIC_BASE_URL") == "https://example"
|
||||
assert "PATH" in env
|
||||
assert "CLAUDECODE" not in env
|
||||
Reference in New Issue
Block a user