From 14eaf4c816115d28ea6ae5a53e611c814650f19d Mon Sep 17 00:00:00 2001 From: G6KlayWang Date: Fri, 30 Oct 2026 02:20:49 -0400 Subject: [PATCH] mcp: an execution failure is a tool error (isError), not an answer (#2006) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A call the CLI could not carry out — bash missing, the verb crashed or was killed, an exit no verb uses, silence, a timeout with no build running, a refusal (exit 2) — came back as an ordinary result on both server paths; only schema-invalid arguments were flagged. run() now raises the server's own ToolError with the same words, which the SDK and the fallback both mark isError, so the model still reads the correction. An answer, path's "no chain" (exit 1 in words) and the note that a graph is still being built (exit 2 while the build lock is held) stay ordinary results. AXIOMCODE_MCP_TIMEOUT (seconds) exists so a test can see the timeout. tests/mcp.py checks nine cases per launch mode, on the fallback and, with AXIOMCODE_SDK_PYTHON, on the SDK. --- plugins/axiomcode/mcp/_fallback.py | 11 +++- plugins/axiomcode/mcp/server.py | 82 +++++++++++++++++------ tests/corrupt_graph.py | 7 +- tests/latency.py | 7 +- tests/mcp.py | 102 +++++++++++++++++++++++++++++ 5 files changed, 183 insertions(+), 26 deletions(-) diff --git a/plugins/axiomcode/mcp/_fallback.py b/plugins/axiomcode/mcp/_fallback.py index 8eb322681..a00b95b61 100644 --- a/plugins/axiomcode/mcp/_fallback.py +++ b/plugins/axiomcode/mcp/_fallback.py @@ -18,7 +18,14 @@ """ import inspect, json, sys, typing -__all__ = ["MCPServer"] +__all__ = ["MCPServer", "ToolError"] + + +class ToolError(Exception): + """A failure the tool anticipated and put into words: the words are the result's text and the result is marked + isError, as the SDK's own ToolError is. Any other exception is marked the same way, but with its type in front, + because its words were never meant for the caller.""" + _JSON_TYPE = {str: "string", int: "integer", float: "number", bool: "boolean", list: "array", dict: "object"} @@ -132,6 +139,8 @@ def _handle(self, msg): try: out = t["fn"](**args) return {"content": [{"type": "text", "text": "" if out is None else str(out)}]} + except ToolError as e: + return {"content": [{"type": "text", "text": str(e)}], "isError": True} except Exception as e: return {"content": [{"type": "text", "text": f"{type(e).__name__}: {e}"}], "isError": True} if m == "ping": diff --git a/plugins/axiomcode/mcp/server.py b/plugins/axiomcode/mcp/server.py index b1ebc5e62..cf1c3bda4 100755 --- a/plugins/axiomcode/mcp/server.py +++ b/plugins/axiomcode/mcp/server.py @@ -126,11 +126,17 @@ def unknown_arguments(name, arguments): said = [f"{k}: unexpected argument" for k in extra] return f"invalid arguments for {name}: " + '; '.join(said) + f". {name} takes: {', '.join(params) or 'no arguments'}" -try: - import importlib - ToolError = importlib.import_module(MCPServer.__module__.rsplit('.', 1)[0] + '.exceptions').ToolError -except Exception: - ToolError = ValueError +# A FAILURE IS RAISED, NOT RETURNED. A client marks a result isError only when the tool raised, and only the server's +# own ToolError keeps the words: the SDK (mcp ≥ 2.3) replaces any other exception's message with a bare "Error +# executing tool", and the fallback puts the type in front. So run() raises this one class, from whichever half serves. +if MCPServer.__module__ == '_fallback': + from _fallback import ToolError +else: + try: + import importlib + ToolError = importlib.import_module(MCPServer.__module__.rsplit('.', 1)[0] + '.exceptions').ToolError + except Exception: + ToolError = ValueError # an SDK with no exceptions module: the result is still isError, wordless class Server(MCPServer): def tool(self, *a, **k): @@ -152,6 +158,9 @@ async def call_tool(self, name, arguments, *a, **k): # the SDK: refused as a # launch.js hands over the bash it chose, because on Windows a bare `bash` is WSL's or nothing (#1233). BASH = os.environ.get('AXIOMCODE_BASH') or 'bash' +# how long a call may take before it is given up as a failure (seconds; a first build is minutes, so the default is +# generous); AXIOMCODE_MCP_TIMEOUT exists for a test that must see the timeout in seconds, not in minutes +TIMEOUT = float(os.environ.get('AXIOMCODE_MCP_TIMEOUT') or 900) # THE TIMER (#1305). The hooks refresh the graph while an agent works; an edit made in an editor or a terminal while the # session sits idle is caught only at the next prompt. The server lives as long as the session, so once @@ -190,12 +199,36 @@ def _timer(interval): if time.time() - fresh.last_update(repo) >= interval: fresh.hook_kick(repo, 'the timer') except Exception: pass -def run(args, cwd=None, timeout=900): +def repository(args, cwd=None): + """the repository a call asks about: the last positional argument that is a directory (a flag's value, `--in `, + is not one), else the working directory""" + pos = [a for i, a in enumerate(args[1:], 1) if args[i - 1] not in VALUED] + return next((a for a in reversed(pos) if os.path.isdir(a)), cwd or os.getcwd()) + +# WHAT A FAILURE IS. The CLI's exits: 0 is an answer, however empty; 1 is `path` saying in words that no chain connects +# the two; 2 is a refusal with its correction (nothing named X and the nearest names, no graph, no such directory), +# and ALSO the note that the graph is still being built. A refusal is a failure of the call, as an argument the tool +# does not take already is (#1567): the client marks the result isError and the model still reads the words. The +# building note is not: it is told apart by the build lock, which a build holds for as long as it runs. A crash, a +# kill, an exit no verb uses, or silence is a failure whatever the code says. +def failed(r, repo): + """whether a finished call is a failure the client must see as one, not an answer""" + if r.returncode == 0: return False + if r.returncode < 0 or r.returncode not in (1, 2): return True + if 'Traceback (most recent call last)' in (r.stderr or ''): return True + if r.returncode == 1: return not ((r.stdout or '').strip() or (r.stderr or '').strip()) + try: return not scripts_module('ax_fresh').building(os.path.realpath(repo)) + except Exception: return True + +def run(args, cwd=None, timeout=None): + """the CLI's words for a call; a call that failed (failed, a bash that will not start, a timeout with no build + running) raises ToolError with the words, so the client marks the result isError and the model still reads them""" + if timeout is None: timeout = TIMEOUT for a in args[1:]: if os.path.isdir(a) and os.path.isdir(os.path.join(a, '.axiomcode')): SEEN.add(os.path.realpath(a)) # A QUERY ON A REPOSITORY WITH NO GRAPH YET does not hold the call for the length of a first build (minutes; past this - # server's timeout it came back as a bare "Error executing tool"): the build is started in the background and the - # answer says what stage it is at (ax_contract.ensure_graph). `index` is asked for a build and still waits for it. + # server's timeout it is a failure saying only that): the build is started in the background and the answer says + # what stage it is at (ax_contract.ensure_graph). `index` is asked for a build and still waits for it. env = dict(os.environ, AXIOMCODE_BUILD_NOWAIT='1', AXIOMCODE_SURFACE='mcp') if args and args[0] != 'index' else None root = plugin_root() ax = os.path.join(root, 'skills', 'axiomcode', 'scripts', 'axiomcode') @@ -204,6 +237,7 @@ def run(args, cwd=None, timeout=900): if o: return f"axiomcode: {o}." note = stale_note(root) head = (note + '\n') if note else '' + repo = repository(args, cwd) try: # On Windows the arguments travel in AXIOMCODE_ARGV, not on the command line: Git Bash parses a command line by # msys rules (`'` quotes, `\"` is no escape), so a question holding an apostrophe lost every argument after it. @@ -213,23 +247,22 @@ def run(args, cwd=None, timeout=900): argv, env = [BASH, ax], dict(env or os.environ, AXIOMCODE_ARGV=''.join(f'{a}\x1f' for a in args)) r = subprocess.run(argv, cwd=cwd or None, capture_output=True, text=True, timeout=timeout, env=env) except OSError as e: - return (f"axiomcode could not start bash ({BASH}): {e}. On Windows it needs the bash that comes with " - "Git for Windows; install it, or set AXIOMCODE_BASH to its bin\\bash.exe.") + raise ToolError(f"axiomcode could not start bash ({BASH}): {e}. On Windows it needs the bash that comes with " + "Git for Windows; install it, or set AXIOMCODE_BASH to its bin\\bash.exe.") from None except subprocess.TimeoutExpired: - # a flag's value (`--in `) is not the repository - pos = [a for i, a in enumerate(args[1:], 1) if args[i - 1] not in VALUED] - repo = next((a for a in reversed(pos) if os.path.isdir(a)), cwd or os.getcwd()) try: ax_fresh = scripts_module('ax_fresh'); ax_contract = scripts_module('ax_contract') if ax_fresh.building(os.path.realpath(repo)): return head + ax_contract.building_note(os.path.realpath(repo)) except Exception: pass - return head + (f"axiomcode {args[0] if args else ''} did not answer within {timeout} s. Nothing was changed; " - f"see {os.path.join(repo, '.axiomcode', 'build.log')} if a build was running, and ask again.") + raise ToolError(head + f"axiomcode {args[0] if args else ''} did not answer within {timeout:g} s. Nothing was changed; " + f"see {os.path.join(repo, '.axiomcode', 'build.log')} if a build was running, and ask again.") from None out = (r.stdout or '') + (('\n' + r.stderr.strip()) if r.returncode and r.stderr.strip() else '') # an answer given from a graph that predates some edit says so, and names the files (#1305); one given from a graph a # fallback engine built, in place of the checkout's own, names that engine if not r.returncode: out += ''.join('\n' + l for l in (r.stderr or '').splitlines() if l.startswith(('graph refresh:', 'graph built by:'))) - return head + (out.strip() or f"(no output, exit {r.returncode})") + text = head + (out.strip() or f"(no output, exit {r.returncode})") + if failed(r, repo): raise ToolError(text + (f"\n(exit {r.returncode})" if out.strip() else '')) + return text # THE TOOLS TAKE NO OPTIONS, so an answer never tells the agent to pass one. The notes the verbs add (a stale # graph, a refresh in flight) are kept for what they say; a clause that names a flag or a parameter to set is dropped. @@ -249,6 +282,13 @@ def plain(text): if keep and ''.join(keep).strip(): out.append(' '.join(keep).rstrip(' ;,')) return '\n'.join(out) +def answer(args): + """a tool's answer: the CLI's words with the option-naming clauses dropped. A failure run() raises goes the same + way — its words through plain(), then on as the ToolError the client marks isError — so the model reads the + correction (the nearest names, the fix) either way, and a flag it cannot pass is named in neither.""" + try: return plain(run(args)) + except ToolError as e: raise ToolError(plain(str(e))) from None + # THE SMALL SURFACE. Search is grep's job; the graph answers what grep cannot. Three questions, each answered as # numbered places with the code of the function each sits in, so a place is understood without opening its file, @@ -259,20 +299,20 @@ def context(task: str, source: bool = False) -> str: """How something works, from a task in words: the files and callables the task touches, and for a "how does X work" question the call FLOW — every step in the order the calls are written, with ⚠ where the graph lost a call. source=True asks for the flow with each step's code, so it is read without opening files.""" - return plain(run(['context', task] + (['--source'] if source else []) + [os.getcwd()])) + return answer(['context', task] + (['--source'] if source else []) + [os.getcwd()]) @srv.tool() def impact(name: str = '') -> str: """What a change reaches. With a name (as written in the code: Owner.method, function, Type, or file.py:123): who calls it, what depends on it further out, and which tests exercise it, each with its code. With no name: the same for the declarations your uncommitted edits changed.""" - return plain(run(['impact'] + ([name] if name.strip() else []) + [os.getcwd()])) + return answer(['impact'] + ([name] if name.strip() else []) + [os.getcwd()]) @srv.tool() def path(start: str, end: str) -> str: """How one declaration reaches another: every hop of the call chain with the code at the line the call is written on. start / end as written in the code (Owner.method, function, Type).""" - return plain(run(['path', start, end, os.getcwd()])) + return answer(['path', start, end, os.getcwd()]) @srv.tool() def link(site: str = '', target: str = '') -> str: @@ -282,12 +322,12 @@ def link(site: str = '', target: str = '') -> str: whether the graph took it (a link whose line changed is dropped, never trusted). target "-" removes the site's links; target "not:" rejects a lead (a by-name or one-of-a-set guess) at that site, which is then not walked. Never link a guess, and never link a candidate for its rank alone.""" - return plain(run(['link'] + ([site] if site.strip() else []) + ([target] if site.strip() and target.strip() else []) + [os.getcwd()])) + return answer(['link'] + ([site] if site.strip() else []) + ([target] if site.strip() and target.strip() else []) + [os.getcwd()]) @srv.tool() def tests() -> str: """The tests your uncommitted edits reach, each with its code, and the command that runs exactly those.""" - return plain(run(['tests', os.getcwd()])) + return answer(['tests', os.getcwd()]) if __name__ == '__main__': # catch up on whatever changed while no session was running (#1305): started, never waited on diff --git a/tests/corrupt_graph.py b/tests/corrupt_graph.py index b999e00d1..a37cd2e04 100644 --- a/tests/corrupt_graph.py +++ b/tests/corrupt_graph.py @@ -121,13 +121,16 @@ def check(ok, why, detail=''): open(os.path.realpath(ptr), 'wb').write(os.urandom(8192)) sys.path.insert(0, os.path.join(ROOT, 'plugins', 'axiomcode', 'mcp')); os.environ.update(AXIOMCODE_ENGINE=ROOT, AXIOMCODE_NO_REFRESH='1') import server - text = server.run(['impact', 'helper', repo]) + def words(args): # a refusal comes as the server's ToolError, words intact + try: return server.run(args) + except server.ToolError as e: return str(e) + text = words(['impact', 'helper', repo]) check('Traceback' not in text and 'corrupt' in text, 'mcp: the tool answer says the graph is corrupt, no traceback', text) sys.path.insert(0, SCRIPTS); import ax_fresh for _ in range(600): # the rebuild it started if os.path.exists(ptr) and not ax_fresh.building(repo): break time.sleep(1) - text = server.run(['impact', 'helper', repo]) + text = words(['impact', 'helper', repo]) check('caller' in text and 'corrupt' not in text, 'mcp: once rebuilt the tool answers again', text) # ── bad page ──────────────────────────────────────────────────────────────────────────────────────── diff --git a/tests/latency.py b/tests/latency.py index ad4064985..a908df6ef 100644 --- a/tests/latency.py +++ b/tests/latency.py @@ -313,9 +313,12 @@ def timeout_checks(): with contextlib.redirect_stderr(io.StringIO()): spec.loader.exec_module(m) work = tempfile.mkdtemp(prefix='axiomcode-timeout-') try: - try: out = m.run(['context', 'anything', work], timeout=0.01); raised = '' + # the words come as the server's own ToolError, which the client marks isError; a bare exception would lose them, + # and a plain return would pass the failure off as an answer + try: out = m.run(['context', 'anything', work], timeout=0.01); raised = 'returned as an answer' + except m.ToolError as e: out, raised = str(e), '' except Exception as e: out, raised = '', repr(e) - check("timeout: an MCP call past the server's timeout answers in words, not with an exception", + check("timeout: an MCP call past the server's timeout fails in words that say so, as a tool error", not raised and 'did not answer within' in out, raised or out) finally: shutil.rmtree(work, ignore_errors=True) diff --git a/tests/mcp.py b/tests/mcp.py index f85b37d37..1b4311dd6 100644 --- a/tests/mcp.py +++ b/tests/mcp.py @@ -190,11 +190,96 @@ def text(self, name, arguments): return ''.join(c.get('text', '') for c in self.ask('tools/call', {'name': name, 'arguments': arguments}).get('content', [])) def close(self): + """ends the server; returns what it wrote on stderr, which says which half served (the fallback's banner)""" self.timer.cancel() self.p.stdin.close() self.p.wait() + err = self.p.stderr.read() self.p.stdout.close() self.p.stderr.close() + return err + + +FALLBACK_BANNER = 'serving with the built-in fallback' +# holds a repository's build lock the way a build does (ax_fresh._flock: LOCK_EX on .axiomcode/build.lock), from a +# process of its own, and says so once it has it +HOLD = ("import fcntl, os, sys, time; fd = os.open(sys.argv[1], os.O_RDWR | os.O_CREAT, 0o644); " + "fcntl.flock(fd, fcntl.LOCK_EX); print('held', flush=True); time.sleep(120)") + + +def check_failures(label, cmd, cwd, env=None, workdir=None, sdk=None): + """A call the CLI could not carry out is a tool error (isError), never an answer: a bash that is not there, a verb + that crashed or was killed, an exit no verb uses, silence, a timeout with no build running, and a refusal (nothing + named X, no graph: exit 2, as an argument the tool lacks already is, #1567). The controls are what must NOT be an + error: an answer, an empty one, `path` saying "no chain" (exit 1, in words), and the note that a graph is still + being built (exit 2 while the build lock is held). The CLI is a fake bash that says what each case needs, so what is + tested is the server's own reading of it; the words must survive, and the SDK puts a prefix on them, so substrings. + `sdk` True or False asserts which half served the mode, read off the stderr banner.""" + if os.name == 'nt': + return [] + bad = [] + here = workdir or cwd # the server's cwd, which is the repository every tool asks about + fakes = tempfile.mkdtemp(prefix='ax-bash-') + base = {k: v for k, v in (env or os.environ).items() if k != 'AXIOMCODE_BASH'} + base.update(AXIOMCODE_MCP_TIMEOUT='2', AXIOMCODE_REFRESH_INTERVAL='0') + + def fake(name, body): + p = os.path.join(fakes, name) + with open(p, 'w') as f: + f.write('#!/bin/sh\n' + body + '\n') + os.chmod(p, 0o755) + return p + crash = r"""printf 'Traceback (most recent call last):\n File "x.py", line 1, in \nValueError: bad\n' >&2; exit 1""" + note = 'the graph for $PWD is being built (starting; 0m01s so far). Nothing else is needed: ask again in a minute or two' + cases = [('missing', os.path.join(fakes, 'no-bash'), True, ['could not start']), + ('boom', fake('boom', "printf 'boom\\n' >&2; exit 7"), True, ['boom', 'exit 7']), + ('slow', fake('slow', 'exec sleep 30'), True, ['did not answer within']), # exec: the kill reaches the sleeper + ('crash', fake('crash', crash), True, ['Traceback']), + ('no chain', fake('nochain', 'echo "no chain of resolved calls connects a and b in either direction"; exit 1'), False, ['no chain']), + ('empty ok', fake('empty', 'exit 0'), False, ['(no output, exit 0)']), + ('building', fake('building', f'echo "{note}"; exit 2'), False, ['being built']), + ('refusal', fake('refusal', 'echo "nothing named zz is declared in this graph. Exact names that are close: a, b"; exit 2'), True, ['close:', 'exit 2']), + ('silent 1', fake('silent', 'exit 1'), True, ['(no output, exit 1)'])] + served = None + try: + for name, bash, want, needles in cases: + s = Session(cmd, here, dict(base, AXIOMCODE_BASH=bash)) + holder = None + try: + if name == 'building': # after the start, so the server's own start-up sees no .axiomcode + os.makedirs(os.path.join(here, '.axiomcode'), exist_ok=True) + holder = subprocess.Popen([sys.executable, '-c', HOLD, os.path.join(here, '.axiomcode', 'build.lock')], + stdout=subprocess.PIPE, text=True) + holder.stdout.readline() + res = s.ask('tools/call', {'name': 'path', 'arguments': {'start': 'a', 'end': 'b'}}) + finally: + if holder: + holder.kill(); holder.wait(); holder.stdout.close() + shutil.rmtree(os.path.join(here, '.axiomcode'), ignore_errors=True) + err = s.close() + if served is None: + served = 'the fallback' if FALLBACK_BANNER in err else 'the SDK' + text = ' '.join(c.get('text', '') for c in res.get('content', [])) + if bool(res.get('isError')) != want: + bad.append(f"{label}, {name}: isError {res.get('isError')!r}, want {want}: {text[:300]!r}") + for n in needles: + if n not in text: + bad.append(f"{label}, {name}: the words lack {n!r}: {text[:300]!r}") + finally: + shutil.rmtree(fakes, ignore_errors=True) + print(f"{label}: failures checked, served by {served}") + if sdk is not None and served != ('the SDK' if sdk else 'the fallback'): + bad.append(f"{label}: served by {served}, but this mode was meant to test {'the SDK' if sdk else 'the fallback'}") + return bad + + +def sdk_python(): + """an interpreter with the MCP SDK, so the failure checks run on that half too: AXIOMCODE_SDK_PYTHON, else this + one; None when neither imports it""" + for py in filter(None, (os.environ.get('AXIOMCODE_SDK_PYTHON'), sys.executable)): + if subprocess.run([py, '-c', 'import mcp.server.mcpserver'], capture_output=True).returncode == 0: + return py + return None def check_install_move(work): @@ -309,6 +394,7 @@ def main(): bad += check('bin/axiomcode mcp', ['bash', CLI, 'mcp'], repo) # the SDK when the launcher finds one, which ignored an argument it did not know (#1567); else the fallback again bad += check_arguments('bin/axiomcode mcp', ['bash', CLI, 'mcp'], repo, lax=True) + bad += check_failures('bin/axiomcode mcp', ['bash', CLI, 'mcp'], repo) bad += check_front_door() if os.name != 'nt': bad += check_install_move(work) @@ -316,6 +402,15 @@ def main(): env_note = 'python3 -S server.py (fallback, no SDK)' bad += check(env_note, [sys.executable, '-S', SERVER], repo) bad += check_arguments(env_note, [sys.executable, '-S', SERVER], repo) + bad += check_failures(env_note, [sys.executable, '-S', SERVER], repo, sdk=False) + # the same failures through the SDK, when an interpreter has it (a venv with `pip install mcp`, named by + # AXIOMCODE_SDK_PYTHON); the skip is said out loud, so a run without the SDK is not mistaken for one with it + py = sdk_python() + if py: + bad += check_failures('server.py (SDK)', [py, SERVER], repo, sdk=True) + else: + print('SDK path skipped: mcp not importable (python3 -m venv && /bin/pip install mcp && ' + 'AXIOMCODE_SDK_PYTHON=/bin/python python3 tests/mcp.py)') # The plugin's own server entries, run as their hosts run them, from a copy of the plugin under a path # with a space, which splits an unquoted path into two words. @@ -329,6 +424,8 @@ def main(): server = json.load(f)['mcpServers']['axiomcode'] bad += check('.codex-plugin/mcp.json', [server['command'], *server['args']], repo, base, os.path.join(plugin, server.get('cwd', '.'))) + bad += check_failures('.codex-plugin/mcp.json', [server['command'], *server['args']], repo, base, + os.path.join(plugin, server.get('cwd', '.'))) # Cursor's own manifest names the server with ${CURSOR_PLUGIN_ROOT}, which Cursor replaces with the # plugin directory before it starts the command, and starts it in the user's project. @@ -338,6 +435,7 @@ def main(): cmd = [server['command'], *map(expand, server['args'])] env = dict(base, **{k: expand(v) for k, v in server.get('env', {}).items()}) bad += check('.cursor-plugin/plugin.json', cmd, repo, env, repo) + bad += check_failures('.cursor-plugin/plugin.json', cmd, repo, env, repo) with open(os.path.join(plugin, '.mcp.json')) as f: server = json.load(f)['mcpServers']['axiomcode'] @@ -345,6 +443,10 @@ def main(): cmd = [server['command'], *map(expand, server['args'])] env = dict(base, **{k: expand(v) for k, v in server.get('env', {}).items()}) bad += check('.mcp.json', cmd, repo, env, repo) + bad += check_failures('.mcp.json', cmd, repo, env, repo) + if py: # Claude Code's entry with the SDK's interpreter first in the launcher's search + sdk_env = dict(env, AXIOMCODE_PYTHON=py, PATH=os.path.dirname(py) + os.pathsep + env.get('PATH', '')) + bad += check_failures('.mcp.json (SDK python)', cmd, repo, sdk_env, repo, sdk=True) # A uv on PATH that cannot build its environment is passed over for the fallback, rather than started # and left to exit before `initialize` (#1249). python3 and python are shadowed by interpreters without