I Gave My MCP Tool an ERROR: Convention. I Only Taught It to One of Its Two Failure Paths.

A developer found and fixed a convention mismatch in an MCP tool called `generate_commit_message` that had two different failure formats: one returning an `ERROR:` prefix and another returning a plain string like 'claude -p timed out after 20s'. The inconsistency meant callers checking for the `ERROR:` prefix would treat the timeout message as a valid commit. The fix unified both failure paths to use the `ERROR:` convention.

Two days ago I found and fixed a real gap in generate commit message , the MCP tool in server.py that turns a git diff into a Conventional Commit message via claude -p . Called with an empty diff, it used to hand the whole empty string straight to Claude and return whatever came back — usually a polite sentence explaining there was nothing to commit, typed as a plain str , indistinguishable from a real commit message to anything downstream that just calls the tool and uses the result. I fixed that by adding a guard: php @mcp.tool def generate commit message diff: str - str: """Generate a Conventional Commits message from a git diff string.""" if not diff.strip : return "ERROR: empty diff — nothing to generate a commit message from." return claude diff, system= ... ERROR: as a prefix, so a caller can do if result.startswith "ERROR:" and know not to trust it as a commit message. That felt like the right convention at the time, and it is — as far as it goes. What I didn't do was go check whether it was the only convention this function needed, or whether some other failure path already existed with a different shape. It did. generate commit message doesn't just call claude -p directly — it goes through a shared helper, claude , which has its own failure path for a hung subprocess: php def claude prompt: str, system: str = None - str: full = system + "\n\n" + prompt if system else prompt try: raw = subprocess.check output "claude", "-p", full , text=True, timeout=20 .strip except subprocess.TimeoutExpired: return "claude -p timed out after 20s" return "\n".join l for l in raw.splitlines if not STRIP RE.search l .strip That timeout branch was added a couple of days before my empty-diff fix, in a different pass through this file, aimed at a different bug the same call had no timeout at all and could hang git commit indefinitely . It does exactly what it says — stops the hang, returns a string explaining what happened — but it was written before the ERROR: convention existed, and nothing about adding that convention later went back and reconciled the two. The result: generate commit message has exactly two ways to fail without raising, and they don't look like each other. generate commit message "" 'ERROR: empty diff — nothing to generate a commit message from.' generate commit message some diff claude -p hangs past 20s 'claude -p timed out after 20s' A caller that learned the right lesson from the empty-diff fix — check for the ERROR: prefix before trusting the return value as a commit message — checks it, gets False on a timeout, and happily commits the literal sentence claude -p timed out after 20s as if it were a real Conventional Commit subject. The whole point of the empty-diff guard was to give callers one thing to check. The timeout path quietly opts out of that contract, in the same function, written by the same hand, days apart. I want to be honest about why this happened rather than just present the fix: it's a sequencing bug, not a subtlety I missed on inspection. The timeout fix and the empty-diff fix were two separate audits on two separate days, each solving the specific bug it was looking for and stopping there. Neither run asked "does this function already have a different failure convention I should match." The empty-diff fix in particular added a format — a string prefix meant to be load-bearing for callers — without auditing every existing return path in the same function against that format. It's the same shape as writing a validation rule and only applying it going forward instead of backfilling it, except here "backfilling" would have cost one word. The fix is exactly that one word: php def claude prompt: str, system: str = None - str: full = system + "\n\n" + prompt if system else prompt try: raw = subprocess.check output "claude", "-p", full , text=True, timeout=20 .strip except subprocess.TimeoutExpired: return "ERROR: claude -p timed out after 20s" return "\n".join l for l in raw.splitlines if not STRIP RE.search l .strip Now both failure paths speak the same language, and .startswith "ERROR:" is a real, complete contract instead of one that only covers the failure mode it happened to be written for. The more general lesson is about where to look after fixing a bug that introduces a signaling convention. It's tempting to treat "I added a way for callers to detect this specific failure" as the whole task and stop once that one path is covered. But a convention that's supposed to mean "don't trust this string" is only as good as its coverage — a partial convention is arguably worse than no convention at all, because it teaches callers to trust a check that doesn't actually cover every way the function can lie to them. The question I should have asked two days ago wasn't "how do I flag an empty diff," it was "how many ways can this function fail without raising, and do they all look the same to whoever's checking." claude had two failure paths and I was only looking at one of them. I didn't find this by staring at generate commit message in isolation — I found it by rereading claude right underneath it and noticing the two return statements didn't rhyme. That's a cheap check, one function's worth of scrolling, and it would have been just as cheap two days ago when the ERROR: convention was first introduced. It just wasn't part of that day's task.