Skip to content

mcpx(diagnose): fieldInMessage breaks equal-length ties by Go map order — the same rejection message blames a different field on different runs #3103

Closed

Description

@dezren39

Problem

fieldInMessage in pkgs/mcpx/internal/diagnose — the function that decides which declared argument a server's rejection message "wants" — iterates the shape's property map and keeps the longest matching name, but breaks equal-length ties by Go map iteration order. When a message names two same-length fields in the same class (both bare tokens, or both quoted), the blamed field changes from run to run, and with it the diagnostic's Field, its "it wants <type>" text, and its Changed attribution.

Evidence (verified this run on main facf6481)

  • pkgs/mcpx/internal/diagnose/diagnose.go:582-607 — fieldInMessage ranges for name := range shape.Props (a map) and replaces the incumbent only when len(name) > len(best) within the same quoted class (lines 597-603: quoted-vs-bare mismatch handled first, then len(name) <= len(best) skips). Nothing orders equal-length candidates, and Go map range order is deliberately random per iteration.
  • The doc comment (diagnose.go:571-580) specifies the ranking — quoted occurrence outranks bare token, "within a class the longest name wins" — and is silent on ties, so the tie outcome is unspecified and unstable.
  • The result is load-bearing at the only call site, diagnose.go:523-533: the returned field becomes Diagnostic.Field, is interpolated into the message (the server rejected %s (it wants %s)), and drives cat.change(tool.Path(), field).

First-hand probe on the laptop (the four functions copied verbatim from current main into a scratch module; Shape stubbed with the same map type), same input, 4000 calls in one process:

msg "user and name are incompatible; host and port conflict", props {user,name,host,port} (all 4 chars, all bare tokens):
  run 1: map[host:514 name:498 port:507 user:2481]
  run 2: map[host:498 name:514 port:473 user:2515]
msg "'user' and 'name' are both wrong", props {user,name} (both quoted):
  run 1: map[name:484 user:3516]
  run 2: map[name:472 user:3528]

Every tied candidate wins some iterations; nothing but map order varies. (The skew toward the first-inserted key is Go's small-map iteration bias — the point is that all outcomes occur.)

Why it matters

mcpx diagnostics are the user-facing explanation of a failed call, and the fleet's own standard elsewhere in this codebase is deterministic tie-breaking: nearmiss.Nearest (internal/nearmiss/nearmiss.go:105-108) explicitly breaks ties lexicographically "so the answer never depends on the order candidates were enumerated in". fieldInMessage missed that treatment when #1646/#3041 rebuilt its matching: the same failed call can blame user in one run and name in the next, pointing the user (and any test or snapshot built on the diagnostic) at different arguments and different schema types for identical input. It also makes any future regression test over a tie case flaky by construction.

Acceptance criteria

  • fieldInMessage is deterministic: for a fixed message and shape, repeated calls always return the same field, including when two or more same-length fields match in the same class (bare or quoted). The tie rule is stated in the doc comment (e.g. lexicographically smallest name, matching nearmiss.Nearest, or first occurrence in the message — either is acceptable if documented and tested).
  • The existing ranking is preserved: a quoted occurrence still outranks a bare token, and a longer name still beats a shorter one within a class; the mcpx(diagnose): CallError blames a field the server never named — fieldInMessage substring-matches prop names inside unrelated words ('valid' in 'invalid' beats the named 'id') #1646 token-boundary and empty-name behaviours are unchanged.
  • Regression tests in internal/diagnose cover a bare-token tie and a quoted tie, asserting one stable winner across repeated calls (e.g. loop the call and require a single outcome).
  • go test ./internal/diagnose -run 'TestFieldInMessage|TestDiagnose' -count=1 (or the package's equivalent targeted tests) passes on laptop or CI.

Dedup

Searched open + closed issues for fieldInMessage tie / nondeterministic / map: the only hit is #1646 (closed — the substring/token-boundary rebuild this function came from, including the #3041 quote-preference and empty-name fixes). Neither it nor any other issue covers equal-length tie determinism; the defect is present on current main.

Activity

  1. added
    queue/queuedHas a queued task in the Supabase agent queue
    source/creatorFiled by the new-issue-creator job
    task/triageQueue packet kind: verify, reproduce, and route this item
    priority/lowNice to have; do when convenient
    app/mcpxPackage/app: mcpx (pkgs/ or apps/ in this repo)
    kind/bugSomething isn't working (prefixed form of the classic bug label)
    effort/sSmall: expected under 2 hours of work
    on Oct 31, 2026
  2. dezren39 commented on Oct 31, 2026

    @dezren39
    MemberAuthor

    🤖 Agent note · state: filed

    • Agent: new-issue-creator job (lane B, new-issue-creator-code-b)
    • Session / ID: new-issue-creator-code-b run 2026-10-31 09:50 CDT
    • Found via: filed this run
    • Task: Filed after verifying on main facf648 with a first-hand probe on the laptop (functions copied verbatim): fieldInMessage returned all four tied fields across 4000 identical calls (map-order tie-break). Triage via the enqueued repo-task/v1 packet.
    • Expectation: effort/s — a documented deterministic tie rule in fieldInMessage plus bare/quoted tie regression tests; existing quoted>bare and longest-wins ranking unchanged.
    • Links: evidence in the issue body; probe in the goal workspace hidden_files/probe-2026-10-31-laneB-fieldtie/
  3. added
    queue/claimedQueue worker lifecycle state
    work/assignedQueue worker lifecycle state
    and removed
    queue/queuedHas a queued task in the Supabase agent queue
    on Nov 1, 2026
  4. 4 remaining items

  5. dezren39 commented on Nov 1, 2026

    @dezren39
    MemberAuthor

    🤖 Agent note · done (triage)

  6. dezren39 commented on Nov 1, 2026

    @dezren39
    MemberAuthor

    🤖 Agent note · picked up

  7. added
    task/small-fixQueue packet kind: bounded fix with reproduction and targeted tests
    and removed
    task/triageQueue packet kind: verify, reproduce, and route this item
    queue/claimedQueue worker lifecycle state
    work/assignedQueue worker lifecycle state
    on Nov 1, 2026
  8. dezren39 commented on Nov 1, 2026

    @dezren39
    MemberAuthor

    🤖 Agent note · done

  9. added a commit that references this issue on Nov 2, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Labels

app/mcpxPackage/app: mcpx (pkgs/ or apps/ in this repo)effort/sSmall: expected under 2 hours of workkind/bugSomething isn't working (prefixed form of the classic bug label)priority/lowNice to have; do when convenientsource/creatorFiled by the new-issue-creator jobtask/small-fixQueue packet kind: bounded fix with reproduction and targeted tests

Type

No type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions