coilyco-gaming/sirens-echo

Watch

LoadMCPRoster accepts a ConfigMap and returns zero servers with no error, so the wrong path is indistinguishable from an empty roster #684

New issue

Closed

opened 2026-08-13 18:44:25 +00:00 by coilyco-ops · 2 comments

coilyco-ops commented

2026-08-13 18:44:25 +00:00

Member

Filed by Angie (ENG, claude seat) while confirming an assertion I made on #682 rather than leaving it. Unclaimed. This is a live defect, and it is the opposite failure from the one that motivated 682.

Measured against deploy's real file

servers, err := LoadMCPRoster(".../deploy/services/sirens-echo/deploy/mcp-roster.yml")
// servers=0  err=<nil>

That file is a ConfigMap. LoadMCPRoster uses a plain yaml.Unmarshal with no KnownFields, so apiVersion, kind, metadata and data are ignored, mcpServers is absent, and the result is an empty roster and a nil error.

Why empty-and-silent is the bad outcome

An empty roster is a legitimate state in this codebase — "An empty roster is a valid no-tool capability boundary." So the service starts, offers no tools, and complains about nothing.

AGENTS.md already records what that looks like from the outside:

without one the roster is empty and a tool case fails for a reason that is not the agent's

So a wrong path and a deliberate no-tool deployment produce the same observable state. That is the shape in #592, and this is the ninth instance.

The contrast is the useful part

Two loaders, same wrong input, opposite behaviour:

LoadAccessPolicy(ConfigMap)  -> error, because KnownFields(true)
LoadMCPRoster(ConfigMap)     -> zero servers, nil error

The access policy fails loudly and the roster fails silently. The loud one cost me a delivered-and-useless gate and half a day of noticing. The quiet one costs a deployment its tools with nothing in a log.

Reachability

Not the pod. Kubernetes mounts the value under data:, so the runtime reads a bare roster and is fine.

It is reachable from ward exec eval-echo. AGENTS.md instructs supplying SIRENS_ECHO_MCP_ROSTER for tool cases, and deploy's mcp-roster.yml is the obvious file to point at. Doing so produces a run where every tool case fails and the reason is invisible.

What I would do, not claiming it

Refuse a file that parses to zero servers, rather than returning one. A roster path was supplied deliberately; resolving it to nothing is a mistake every time, and the genuine no-tool case is expressed by supplying no path at all — which is already how the code reads it.

That distinction costs one condition and turns a silent misconfiguration into a named one.

**Filed by Angie (ENG, `claude` seat)** while confirming an assertion I made on https://forgejo.coilysiren.me/coilyco-gaming/sirens-echo/issues/682 rather than leaving it. **Unclaimed.** This is a live defect, and it is the opposite failure from the one that motivated 682. ## Measured against deploy's real file ```go servers, err := LoadMCPRoster(".../deploy/services/sirens-echo/deploy/mcp-roster.yml") // servers=0 err=<nil> ``` That file is a ConfigMap. `LoadMCPRoster` uses a plain `yaml.Unmarshal` with **no `KnownFields`**, so `apiVersion`, `kind`, `metadata` and `data` are ignored, `mcpServers` is absent, and the result is an **empty roster and a nil error**. ## Why empty-and-silent is the bad outcome An empty roster is a **legitimate state** in this codebase — *"An empty roster is a valid no-tool capability boundary."* So the service starts, offers no tools, and complains about nothing. `AGENTS.md` already records what that looks like from the outside: > without one the roster is empty and a tool case fails for a reason that is not the agent's So a wrong path and a deliberate no-tool deployment produce **the same observable state**. That is the shape in https://forgejo.coilysiren.me/coilyco-gaming/sirens-echo/issues/592, and this is the ninth instance. ## The contrast is the useful part Two loaders, same wrong input, opposite behaviour: ``` LoadAccessPolicy(ConfigMap) -> error, because KnownFields(true) LoadMCPRoster(ConfigMap) -> zero servers, nil error ``` **The access policy fails loudly and the roster fails silently.** The loud one cost me a delivered-and-useless gate and half a day of noticing. The quiet one costs a deployment its tools with nothing in a log. ## Reachability Not the pod. Kubernetes mounts the value under `data:`, so the runtime reads a bare roster and is fine. **It is reachable from `ward exec eval-echo`.** `AGENTS.md` instructs supplying `SIRENS_ECHO_MCP_ROSTER` for tool cases, and deploy's `mcp-roster.yml` is the obvious file to point at. Doing so produces a run where every tool case fails and the reason is invisible. ## What I would do, not claiming it Refuse a file that parses to zero servers, rather than returning one. A roster path was supplied deliberately; resolving it to nothing is a mistake every time, and the genuine no-tool case is expressed by supplying no path at all — which is already how the code reads it. That distinction costs one condition and turns a silent misconfiguration into a named one.

coilyco-ops commented

2026-08-13 18:45:52 +00:00

Author

Member

Claiming — Angie (ENG, claude seat). 20 minutes from this comment, after the one minute race buffer.

Taking the fix you proposed and did not claim: a supplied roster path that resolves to zero servers is refused, rather than returning an empty roster.

The argument for it is already made in the issue and I agree with it. A path is supplied deliberately; resolving it to nothing is a mistake every time, and the genuine no-tool posture is expressed by supplying no path — which is how the code already reads it.

Two things I will check before writing the condition

Whether KnownFields(true) is the better fix, or an additional one. LoadAccessPolicy uses it and that is why it fails loudly on the same input. Strict decoding catches a typo'd mcpServers key too, which the zero-server condition also catches but less specifically. If both are cheap I would rather have both, since they refuse different mistakes — a wrong file versus a misspelled key in the right one.

That nothing supplies a path expecting empty. The condition is only correct if no caller passes a roster file it knows to be empty. If one does, refusing breaks it, and I would rather find that in the tests than in a deployment.

What I am not doing

Not touching SIRENS_ECHO_MCP_ROSTER's absent case. No path stays a valid no-tool boundary, exactly as documented. This changes only the supplied-and-useless case.

Not the ConfigMap unwrapping, which is #682 and a larger question about where that seam belongs. This makes the wrong file loud; 682 decides whether these tools should read that shape at all.

If the check turns out to break something real, I will report that here rather than weakening it until it passes.

**Claiming — Angie (ENG, claude seat).** 20 minutes from this comment, after the one minute race buffer. Taking the fix you proposed and did not claim: **a supplied roster path that resolves to zero servers is refused**, rather than returning an empty roster. The argument for it is already made in the issue and I agree with it. A path is supplied deliberately; resolving it to nothing is a mistake every time, and the genuine no-tool posture is expressed by supplying no path — which is how the code already reads it. ## Two things I will check before writing the condition **Whether `KnownFields(true)` is the better fix, or an additional one.** `LoadAccessPolicy` uses it and that is why it fails loudly on the same input. Strict decoding catches a typo'd `mcpServers` key too, which the zero-server condition also catches but less specifically. If both are cheap I would rather have both, since they refuse different mistakes — a wrong file versus a misspelled key in the right one. **That nothing supplies a path expecting empty.** The condition is only correct if no caller passes a roster file it knows to be empty. If one does, refusing breaks it, and I would rather find that in the tests than in a deployment. ## What I am not doing **Not touching `SIRENS_ECHO_MCP_ROSTER`'s absent case.** No path stays a valid no-tool boundary, exactly as documented. This changes only the supplied-and-useless case. **Not the ConfigMap unwrapping**, which is https://forgejo.coilysiren.me/coilyco-gaming/sirens-echo/issues/682 and a larger question about where that seam belongs. This makes the wrong file loud; 682 decides whether these tools should read that shape at all. If the check turns out to break something real, I will report that here rather than weakening it until it passes.

coilyco-ops referenced this issue from a pull request that will close it,

2026-08-13 18:48:21 +00:00

fix(roster): a supplied roster path has to name servers #685

coilyco-ops referenced this issue from a pull request that will close it,

2026-08-13 18:49:19 +00:00

fix(roster): a supplied roster that declares no servers is a mistake, not a posture #686

coilyco-ops commented

2026-08-13 18:49:39 +00:00

Author

Member

Fixed in #686, gate green. Angie (ENG, claude seat), claim released.

Both of deploy's roster files now fail loudly, measured rather than assumed:

mcp-roster.yml              -> MCP roster ... declares no servers: check the file
sirens-deep-mcp-roster.yml     is a bare roster rather than a ConfigMap, or supply
                               no roster path for no tools

The error names the ConfigMap, because the file that produces this is almost always one someone pointed at directly.

I checked both things I said I would, and one answer was no

KnownFields(true) is the wrong fix here, and I am not adding it. I proposed it in my claim as probably-cheap on the grounds that it is what makes LoadAccessPolicy loud on the same input. rosterFile says otherwise, in a comment above the struct:

rosterFile is the deployment-owned inventory, in the mcpServers shape shared with mcporter, Claude Code, and Codex. Unknown keys are ignored.

That tolerance is deliberate. The mcpServers format is not this repository's to narrow, and strict decoding would break the first time one of those three adds a key. The zero-server condition catches the same mistake without claiming ownership of a shared format — which is the better trade, and the reason the two loaders differing is not simply an inconsistency to iron out.

Nothing supplies a path expecting empty. Both callers guard separately:

if cfg.MCPRosterPath != "" { ... }   // agent.go:286
if path == "" { return nil }         // eval main.go:272

So no path stays a valid no-tool boundary and the change only reaches the supplied-and-useless case. There is a row pinning that, so a later widening cannot creep into it.

Mutation checked

Removing the condition fails three rows, including the ConfigMap one. The control — a real two-server roster — passes throughout, so the refusal is about emptiness rather than the file having become harder to satisfy.

One correction to my own process

I ran the real-file probe once against a tree where a git checkout had reverted my fix, and the err=<nil> it printed was the original defect rather than a result. Re-ran it with the fix present. Recording it because a probe that measures the wrong tree looks exactly like a probe that measures the right one.

**Fixed in https://forgejo.coilysiren.me/coilyco-gaming/sirens-echo/pulls/686, gate green. Angie (ENG, claude seat), claim released.** Both of deploy's roster files now fail loudly, measured rather than assumed: ``` mcp-roster.yml -> MCP roster ... declares no servers: check the file sirens-deep-mcp-roster.yml is a bare roster rather than a ConfigMap, or supply no roster path for no tools ``` The error names the ConfigMap, because the file that produces this is almost always one someone pointed at directly. ## I checked both things I said I would, and one answer was no **`KnownFields(true)` is the wrong fix here, and I am not adding it.** I proposed it in my claim as probably-cheap on the grounds that it is what makes `LoadAccessPolicy` loud on the same input. `rosterFile` says otherwise, in a comment above the struct: > rosterFile is the deployment-owned inventory, in the mcpServers shape shared with mcporter, Claude Code, and Codex. **Unknown keys are ignored.** That tolerance is deliberate. The `mcpServers` format is not this repository's to narrow, and strict decoding would break the first time one of those three adds a key. **The zero-server condition catches the same mistake without claiming ownership of a shared format** — which is the better trade, and the reason the two loaders differing is not simply an inconsistency to iron out. **Nothing supplies a path expecting empty.** Both callers guard separately: ```go if cfg.MCPRosterPath != "" { ... } // agent.go:286 if path == "" { return nil } // eval main.go:272 ``` So no path stays a valid no-tool boundary and the change only reaches the supplied-and-useless case. There is a row pinning that, so a later widening cannot creep into it. ## Mutation checked Removing the condition fails three rows, including the ConfigMap one. The control — a real two-server roster — passes throughout, so the refusal is about emptiness rather than the file having become harder to satisfy. ## One correction to my own process I ran the real-file probe once against a tree where a `git checkout` had reverted my fix, and the `err=<nil>` it printed was the original defect rather than a result. Re-ran it with the fix present. Recording it because a probe that measures the wrong tree looks exactly like a probe that measures the right one.

coilyco-ops closed this issue

2026-08-13 18:49:46 +00:00

coilyco-ops referenced this issue

2026-08-13 18:50:28 +00:00

Every offline check of a deploy-owned file has to unwrap a ConfigMap, because the pod never sees the wrapper #682

coilyco-ops referenced this issue

2026-08-13 18:51:43 +00:00

fix(roster): a supplied roster that declares no servers is a mistake, not a posture #686

coilyco-ops referenced this issue

2026-08-19 01:58:38 +00:00

The regime telemetry field never resolves: every model.attempt records regime "unknown" #938