MUL-5619: fix(cli): surface the server's 409 message instead of the generic conflict template - #6267
Merged
Conversation
…09 template
Every 409 this API returns is a deterministic refusal that names its own fix
("a skill with this name already exists", "set parent_id (--parent) to <id>").
The CLI replaced all of them with a template that says the opposite — that the
state changed underneath you and you should re-fetch and retry. Agents took the
retry hint literally: GH #6264 reports 15+ identical retries over 10 minutes
followed by hours spent chasing an optimistic-concurrency theory that never
existed, and GH #5948 is a second user misdiagnosing the same way. MUL-4417 had
already written the useful message server-side; it just never reached anyone.
Route 409 through the same server-message extraction 400/422 already uses, so
roughly forty hand-written conflict messages across skills, agents, runtimes,
labels, projects and comments become visible by default. A body we cannot
recognize still falls back to the template, so this never dumps a raw response.
extractServerMessage now prefers prose over a bare identifier, because a few
endpoints put a stable code in "error" and the sentence in "message".
MUL-5619
Co-authored-by: multica-agent <github@multica.ai>
…el comment The reply guard returns one message for two different mistakes. A resumed session that carries a previous turn's --parent forward (GH #6264) did not ask for a top-level comment, but is told it did — which sends it looking for a new-thread opt-in (GH #5383) instead of correcting the parent it already passed. Split the copy: name the rejected parent when one was supplied, and keep the existing top-level wording for the parentless case. Both still point at the trigger comment to use. MUL-5619 Co-authored-by: multica-agent <github@multica.ai>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
InitiateUpdate answered every UpdateStore.Create failure with a 409 carrying err.Error(). The in-memory store only ever returns errUpdateInProgress, so this looked safe — but the Redis store also wraps infrastructure failures as "reserve active update: <dial error>" and "persist update request: <error>". Surfacing 409 bodies in the CLI turns that into a user-visible leak of internal addresses, and labels an outage as a conflict the caller could fix by retrying. Classify instead: errUpdateInProgress keeps its 409 and its actionable message, everything else is logged and answered with a 500 and fixed copy. Also pins the prose-over-machine-code preference for validation bodies, which the shared extractor applies to 400/422 as well as 409. Only the issue-table endpoints are shaped that way and none is reachable from the CLI today, but the change is intentional and should fail loudly if reverted. MUL-5619 Co-authored-by: multica-agent <github@multica.ai>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #6264. Also removes the root cause behind #5948.
The problem
Our 409s tell users the opposite of the truth.
Every conflict this API returns is a deterministic refusal that names its own fix — "a skill with this name already exists", "agent is already archived", "set parent_id (--parent) to
<id>". The CLI threw all of them away and printed one template instead:That is retry advice for a transient race. Nothing about a 409 here is transient, so following it never works.
Agents follow it anyway. In #6264 a long-lived Team-Leader agent hit the comment reply guard, retried 15+ times over 10 minutes with 5 different parent UUIDs, then spent hours chasing an optimistic-concurrency theory that does not exist in this codebase, and finally wrote its user a report claiming the platform was broken (including that Multica is closed-source with no public issue tracker — both wrong). The whole time the server was returning the exact comment id to reply under. #5948 is a second, independent user misdiagnosing the same symptom the same way.
MUL-4417 (#5266, PR #5292) already wrote the useful server-side message. It has never been visible to anyone.
What changed
1.
server/internal/cli/errors.go— surface the server's conflict message.409 now goes through the same extraction path 400/422 has always used. One branch; roughly 40 hand-written conflict messages across skills, agents, runtimes, runtime profiles, labels, projects, pins, chat and comments become visible at once.
This is not "print the raw body".
extractServerMessageonly accepts a JSON object with a known message field, so an HTML error page, a proxy response, or a body carrying only a machine code still falls back to the generic template. HTTP classification, exit codes and--debugoutput are untouched.It also now prefers prose over a bare identifier: a few endpoints put a stable code in
"error"and the sentence in"message"(the issue-table cursor responses), and the sentence is the part a person can act on. The code is still used when it is all that exists.2.
server/internal/handler/comment.go— fix the message the guard actually sends.The guard returned one message for two different mistakes. A resumed session carrying a stale
--parentforward — the exact case in #6264 — was told "comment-triggered tasks cannot create top-level comments", which is simply false: it did pass a parent. That wording pushes the agent toward asking for a new-thread opt-in (#5383) rather than correcting the parent it already has. Now the two cases read differently, and the wrong-parent case names the parent it rejected.Before / after
Before:
After:
Testing
Local, on a freshly migrated database:
go test ./internal/cli/...— ok. New: conflict message surfaced, Chinese prefix, three fallback cases (HTML / code-only / empty body) still hitting the generic template with no body leak, and prose-vs-code preference including non-ASCII prose.go test ./internal/handler/...— ok (17.5s, full package). NewTestCreateComment_TriggeredTaskRejectsForeignParentcovers the resumed-session case: a real comment on the same issue that this task was never given to answer. It asserts the refusal names both parents and does not claim a top-level comment was attempted. Both existing guard tests still pass unchanged.gofmt,go build ./...,go veton both packages — clean../cmd/multica/...shows 94 failures in this environment, but they are pre-existing and unrelated: the suite was run from inside a Multica agent workdir, sonewAPIClientrefuses on the daemon task-context marker. Verified identical — 94 before and 94 after the change — by stashing the diff and re-running.Deliberately not in this PR
#6264 also proposes auto-filling
--parentfromX-Task-IDon failure. That is a much bigger change than it appears: the CLI has notaskcommand group, no user-scoped endpoint exposingtrigger_comment_id, and the daemon injectsMULTICA_TASK_IDbut not the trigger comment id. It would need new API surface plus implicit retry semantics that mask a real drift bug. Worth deciding separately.--new-thread(#5383) stays independent.Review round 2
Two findings from review, both addressed.
Must-fix:
runtime_update.goechoed infrastructure errors as 409Caught correctly — surfacing conflict bodies is only safe if every 409 really is a safe, deterministic business refusal, and one endpoint broke that premise.
InitiateUpdateanswered everyUpdateStore.Createfailure with a 409 carryingerr.Error(). The in-memory store only ever returnserrUpdateInProgress, so it looked fine; the Redis store also returnsreserve active update: <dial error>andpersist update request: <error>. Under this PR that would print internal addresses at the user and label a Redis outage as a conflict worth retrying.Now classified:
errUpdateInProgresskeeps its 409 and its actionable message; everything else is logged server-side and answered with a 500 and fixed copy carrying no underlying error.Two tests in
runtime_update_error_classification_test.go, using an injected store: a Redis-shaped dial error must produce 500 with no10.1.2.3:6379/connection refused/reserve active updateanywhere in the body, and update-in-progress must still produce 409 with its message intact. Confirmed the negative test actually bites — stashing the handler fix and re-running fails it, restoring it passes.Non-blocking: prose preference also changes 400/422
Correct, and taking the second of the two suggested options: keeping the preference in the shared extractor and pinning the behavior explicitly rather than special-casing conflict.
Splitting it would mean a mode flag on a shared function to preserve output that is strictly worse (
unsupported_groupinstead of "This group type is not supported."). Worth noting the blast radius is smaller than it looks: the issue-table endpoints are the only ones shaped this way, and none of them is reachable from the CLI — nomulticacommand hits/api/issues/table/*. So this is a latent improvement, not a live output change.Pinned by
TestFormatErrorValidationPrefersProseOverMachineCode, which also asserts a code-only validation body still shows its code. Listed here as an intentional change.Verification after the fixes
go test ./internal/handler/...— ok, 16.5s, full packagego test ./internal/cli/...— okgofmt/go build ./.../go vet— clean