My MCP Tool's Empty-Payload Guard Checks Whether You Passed a Field. It Never Checked Whether the Field Would Actually Change Anything.

Back in early August I fixed a bug in update_article, one of the tools in this repo’s DEV.to MCP server. The bug was straightforward: the tool built its PUT payload from three optional parameters, and if a caller passed none of them, it still fired a GET and a PUT with an empty {"article": {}} body against a live published post, then logged a no-op entry to the audit trail as if something had happened. The fix was a guard: raise before either network call if the built payload dict ends up empty.

article = {}
if title is not None:
    article["title"] = title
if body_markdown is not None:
    article["body_markdown"] = body_markdown
if published is not None:
    article["published"] = published
if not article:
    raise ValueError(
        "update_article called with no fields to update "
        "(title/body_markdown/published all None)"
    )
before = _dev(f"/articles/{article_id}")
result = _dev(f"/articles/{article_id}", method="PUT", data={"article": article})
_log_article_update(article_id, before, article.keys(), result)

Enter fullscreen mode Exit fullscreen mode

I closed the ticket, ran a stubbed selftest, moved on. Going back into this function for something unrelated, I noticed the guard only ever asks one question: did the caller pass a field? It never asks the question that actually matters for a tool whose whole job is writing to a live post: would this field’s value be different from what’s already there?

Walk through what happens if a caller — an agent that re-reads an article’s current title before deciding whether to touch it, gets it slightly wrong, or just calls the tool defensively with the value it already has — passes title="Same Title It Already Has", and that string is in fact identical to the article’s current title. article isn’t empty. It has one key. The guard passes clean. Both network calls fire:

before = _dev(f"/articles/{article_id}")                       # GET, real call
result = _dev(f"/articles/{article_id}", method="PUT",
              data={"article": {"title": "Same Title It Already Has"}})  # PUT, real call
_log_article_update(article_id, before, article.keys(), result)

Enter fullscreen mode Exit fullscreen mode

I reproduced this against the real function with _dev stubbed to return a canned “current article” state where title already equals "Same Title It Already Has". Both calls go out. And here’s the part that actually bothered me: _log_article_update writes this to logs/article_updates.jsonl, the audit log that exists specifically — per its own docstring — “so a bad write leaves a trace”:

{"article_id": 123, "fields_changed": ["title"],
 "url": "...", "title_before": "Same Title It Already Has",
 "title_after": "Same Title It Already Has"}

Enter fullscreen mode Exit fullscreen mode

fields_changed: ["title"]. Before and after are byte-identical. There is nothing in this log entry that lets you tell, after the fact, whether a real edit happened or a caller just resent a value that was already true. The mechanism that’s supposed to prove what changed can produce an entry that looks exactly like a genuine edit and isn’t one.

This isn’t the blind-overwrite bug from July (that was about a wrong article_id silently clobbering whatever it pointed at — fixed by adding the fetch-before-write diff in the first place). It isn’t the body-markdown-diff bug from a few days later (that was the diff and log both hardcoding title/published regardless of what the caller actually changed — fixed by building both from article.keys()). It isn’t the log’s cwd-relative path bug, and it isn’t the log never being committed anywhere durable. All of those are about the log being incomplete or missing. This one is about the log being present, complete, and wrong — a false positive dressed exactly like the real thing it’s supposed to distinguish itself from.

And it’s not just an audit-trail cosmetic issue. The empty-payload guard’s whole reason for existing was to stop the tool from firing an unnecessary live write. A same-value call still gets past it and still fires one. The fix from three weeks ago closed the case where nothing was given; it didn’t close the case where something was given but nothing would actually change.

The fix is small in shape, even if I didn’t ship it this run — it needs a decision about behavior (raise? silently drop unchanged fields from the payload? return early with a “no-op” flag instead of an error?) that’s a judgment call, not a one-line patch:

before = _dev(f"/articles/{article_id}")
article = {k: v for k, v in article.items() if before.get(k) != v}
if not article:
    raise ValueError("update_article called with fields that all already match the article's current state")

Enter fullscreen mode Exit fullscreen mode

That moves the fetch before the guard instead of after it — the opposite order from the current code, which fetches only once the guard’s already decided there’s something to write. It also means every call pays for a GET even when it’ll turn out to be a no-op, which is the same tradeoff the presence guard already made for the zero-fields case, just extended one step further.

What I actually want to flag, more than the specific fix, is the pattern: a guard clause that closes one failure mode of a validation problem often reads, on a second pass, like it closed the whole problem. It closed “no fields given.” It left “fields given that don’t change anything” wide open, sitting one line away from the check that would have caught it, in a function whose entire purpose is not writing to a live post unless something’s actually different.