Skip to content

Stop shattering agent Uris that contain a comma (GH-3733) - #3736

Merged
jeremydmiller merged 1 commit into
mainfrom
gh-3733/agent-uri-delimiter
Jul 30, 2026
Merged

Stop shattering agent Uris that contain a comma (GH-3733)#3736
jeremydmiller merged 1 commit into
mainfrom
gh-3733/agent-uri-delimiter

Conversation

@jeremydmiller

Copy link
Copy Markdown
Member

Closes #3733.

The defect

AgentsStarted, StartAgents, AgentsStopped and StopAgents all round-trip their Uri[] payload as a comma-joined string. A comma is a legal URI character — RFC 3986 lists it as a sub-delim, permitted unescaped in a path segment — and there is no escaping here, so one agent URI containing a comma is split into fragments and new Uri(fragment) throws.

Agent URIs embed operator- and tenant-supplied strings. An event-subscription URI is event-subscriptions://marten/main/<host>.<db>/<projection>/all/v<n>/<tenant>, so a tenant id with a comma in it reaches this code without anyone doing anything exotic.

Because the read side built the whole array in one LINQ projection, the throw took out the entire batch. AgentsStarted is the reply that confirms an agent start, and AssignAgents.ExecuteAsync treats a null response as "no confirmation" and moves on — so one comma-bearing agent voided the confirmation for every agent in its chunk. On #3698's cluster that is exactly the state that looked like a stall: the leader's own local starts need no reply and kept landing, while remote chunks went unconfirmed.

The fix

Newline is the delimiter — not legal unescaped in a URI, and Uri.ToString() never emits a bare one.

The comma stays the default on the wire. It is what every 6.24.0-and-earlier node both writes and reads, and a rolling upgrade has to keep working in both directions. Only a payload that actually contains a comma — the case that was already broken on both sides — switches to the newline form, and it announces itself with a leading newline. The two formats can never be confused, because a legacy payload always begins with a URI scheme character.

Without that, an upgraded leader sending \n-joined StartAgents to a not-yet-upgraded follower would break agent starts that work today, which is a worse trade than the bug being fixed.

Entries are also parsed one at a time now, so an unreadable one names itself and the full payload rather than surfacing as a bare Invalid URI: The format of the URI could not be determined. with no way to tell which agent, or which node, was responsible.

Sibling audit

As suggested on the issue, checked the other ISerializable agent commands. StartAgent and StopAgent carry a single URI, CheckAgentHealth an empty payload, and NodeRecord is JSON. None share the pattern.

Tests

In agent_command_serialization:

  • a [Theory] over three comma-bearing shapes (comma in a tenant id, comma-space, and the production case — two agents where only one has a comma, previously voiding both), asserted across all four commands;
  • the no-comma payload still produces the byte-identical legacy wire format;
  • a legacy comma-joined payload still reads;
  • an unreadable entry names itself and the payload.

All green, full wolverine.slnx Release build clean.

🤖 Generated with Claude Code

`AgentsStarted`, `StartAgents`, `AgentsStopped` and `StopAgents` all round-trip
their `Uri[]` payload as a comma-joined string. A comma is a **legal** URI
character -- RFC 3986 lists it as a sub-delim, permitted unescaped in a path
segment -- and there is no escaping here, so a single agent URI containing one
is split into fragments and `new Uri(fragment)` throws.

Agent URIs embed operator- and tenant-supplied strings. An event-subscription
URI is `event-subscriptions://marten/main/<host>.<db>/<projection>/all/v<n>/<tenant>`,
so a tenant id with a comma in it reaches this code without anyone doing
anything exotic.

Because the read side built the whole array in one LINQ projection, the throw
took out the **entire batch**, not just the offending entry. `AgentsStarted` is
the reply that confirms an agent start, and `AssignAgents.ExecuteAsync` treats a
null response as "no confirmation" and moves on -- so one comma-bearing agent
voided the confirmation for every agent in its chunk. On GH-3698's cluster that
is precisely the state that looked like a stall: the leader's own local starts
need no reply and kept landing, while remote chunks went unconfirmed.

Newline is the delimiter instead -- not legal unescaped in a URI, and
`Uri.ToString()` never emits a bare one.

The comma nonetheless stays the default on the wire. It is what every
6.24.0-and-earlier node both writes and reads, and a rolling upgrade has to
keep working in both directions, so only a payload that actually contains a
comma -- the case that was already broken on both sides -- switches to the
newline form. It announces itself with a leading newline, and the two formats
can never be confused because a legacy payload always begins with a URI scheme
character.

Entries are also parsed one at a time now, so an unreadable one names itself
and the full payload instead of surfacing as a bare "Invalid URI: The format of
the URI could not be determined." with no way to tell which agent, or which
node, was responsible.

Audited the sibling `ISerializable` agent commands: `StartAgent`, `StopAgent`,
`CheckAgentHealth` and `NodeRecord` carry a single URI, an empty payload, or
JSON, so none share the pattern.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@jeremydmiller
jeremydmiller merged commit 6a8c736 into main Jul 30, 2026
31 of 32 checks passed
@jeremydmiller jeremydmiller mentioned this pull request Jul 30, 2026
This was referenced Jul 30, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

AgentsStarted serialization shatters on a comma in an agent Uri, voiding the whole batch confirmation

1 participant