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

coilyco-ops commented

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

Member

closes #684 - deploy's ConfigMap loaded as zero servers with a nil error, indistinguishable from a deliberate no-tool deployment

coilyco-ops added 1 commit

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

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

ci / image-build (pull_request) Successful in 23s

Details

ci / test (pull_request) Successful in 37s

Details

ci / publish-echo-image (pull_request) Has been skipped

Details

ci / publish-observed (pull_request) Has been skipped

Details

8fd0b5b192

LoadMCPRoster read deploy's mcp-roster.yml as zero servers with a nil
error. That file is a ConfigMap, every key in it is unknown to the
roster struct, and an empty roster is a legitimate no-tool state, so a
wrong path and a deliberate no-tool deployment were the same observable.

A path is supplied deliberately, so resolving it to nothing is a mistake
every time. No path is how a no-tool deployment says so, and both
callers already guard on that separately, so the boundary does not move.

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

Both of deploy's roster files now fail loudly rather than quietly, and a
real roster still loads.

Not adding KnownFields(true), which is what makes LoadAccessPolicy fail
loudly on the same input. rosterFile documents that unknown keys are
ignored on purpose: the mcpServers shape is shared with mcporter, Claude
Code and Codex, and strict decoding would break on a key one of them
adds. The zero-server condition catches the same mistake without
claiming ownership of a format this repository does not own.

Refs sirens-echo#684

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-authored-by: Kai Siren <coilysiren@gmail.com>
Co-authored-by: Claude <noreply@anthropic.com>

coilyco-ops requested review from coilysiren

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

coilyco-ops referenced this pull request

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

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

coilyco-ops commented

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

Author

Member

Superseded by #685, which merged as a377c2d while this was open. Closing — Angie (ENG, claude seat).

Same fix, and theirs is placed slightly better: it checks len(file.MCPServers) == 0 right after the unmarshal, before entry validation, so the refusal happens at the point the emptiness is known rather than after a loop that had nothing to do.

Their three tests cover what mine did:

TestAConfigMapIsRefusedRatherThanReadAsEmpty
TestARosterNamingNoServersIsRefused
TestARealRosterStillLoads

One row of mine has no equivalent, and it is the boundary this fix must not move:

func TestNoRosterPathIsStillAValidNoToolDeployment(t *testing.T) {
	roster, err := loadRoster(Config{})
	// an unset path returns no error and no servers
}

emptyroster_test.go tests LoadMCPRoster only, and loadRoster is where the cfg.MCPRosterPath != "" guard lives. If anyone later moves the refusal up into loadRoster, the no-tool deployment breaks and the suite stays green — because nothing currently asserts that an unset path is still fine.

That is the same shape as the gap I found on #627: a compound behaviour where one half is pinned and the adjacent half is not.

I am not opening a third pull request for one row. It is ten lines, it belongs beside emptyroster_test.go, and whoever owns 685 can add it in a minute. Recorded on #684 as well so it is not only in a closed pull request.

Sixth duplicate today, all against -claude branches. Detail on #552.

**Superseded by https://forgejo.coilysiren.me/coilyco-gaming/sirens-echo/pulls/685, which merged as `a377c2d` while this was open. Closing — Angie (ENG, claude seat).** Same fix, and theirs is placed slightly better: it checks `len(file.MCPServers) == 0` right after the unmarshal, before entry validation, so the refusal happens at the point the emptiness is known rather than after a loop that had nothing to do. Their three tests cover what mine did: ``` TestAConfigMapIsRefusedRatherThanReadAsEmpty TestARosterNamingNoServersIsRefused TestARealRosterStillLoads ``` **One row of mine has no equivalent, and it is the boundary this fix must not move:** ```go func TestNoRosterPathIsStillAValidNoToolDeployment(t *testing.T) { roster, err := loadRoster(Config{}) // an unset path returns no error and no servers } ``` `emptyroster_test.go` tests `LoadMCPRoster` only, and `loadRoster` is where the `cfg.MCPRosterPath != ""` guard lives. **If anyone later moves the refusal up into `loadRoster`, the no-tool deployment breaks and the suite stays green** — because nothing currently asserts that an unset path is still fine. That is the same shape as the gap I found on https://forgejo.coilysiren.me/coilyco-gaming/sirens-echo/issues/627: a compound behaviour where one half is pinned and the adjacent half is not. I am **not** opening a third pull request for one row. It is ten lines, it belongs beside `emptyroster_test.go`, and whoever owns 685 can add it in a minute. Recorded on https://forgejo.coilysiren.me/coilyco-gaming/sirens-echo/issues/684 as well so it is not only in a closed pull request. Sixth duplicate today, all against `-claude` branches. Detail on https://forgejo.coilysiren.me/coilyco-gaming/sirens-echo/issues/552.

coilyco-ops closed this pull request

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

ci / image-build (pull_request) Successful in 23s

Details

ci / test (pull_request) Successful in 37s

Details

ci / publish-echo-image (pull_request) Has been skipped

Details

ci / publish-observed (pull_request) Has been skipped

Details

Pull request closed

Please reopen this pull request to perform a merge.

Rows
Columns

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

Pull request closed