Repository navigation
fix(coexistcheck): keep the default agent warning off an enabled pack identity - #197
Conversation
arcavenai
left a comment
There was a problem hiding this comment.
Approving. The default-agent WARN no longer fires on a repo whose agent still resolves, and every case that really dangles still warns. A WARN that is wrong on a working repo is the kind operators learn to skip, so this keeps the #196 check worth reading.
Red. At 877dd3c, ..WhenReEnabledLocally and ..WhenASecondMarketplaceIsEnabled fail and every other test passes. At 612b902, both pass and go test -count=1 ./... passes.
The table from my #196 review, rerun through Run at head (a scratch test, since deleted):
| case | default-agent WARN |
|---|---|
foreign agent in settings.json, identity suppressed by settings.local.json |
1 |
foreign agent in settings.local.json, suppressed there |
1 |
committed false against a user-scope enable, agent in the committed file |
1 |
user-scope enable, local false, agent in the committed file |
1 |
user enable, committed false, local true (re-enabled) |
0, was 1 at #196 |
claude-mp suppressed, a second marketplace's identity of the same plugin enabled |
0, was 1 |
The early return keys on this pack's identities. These cases tell that apart from "any enabled pack" or the agent's prefix. Each has an identity of this pack suppressed, an agent of this pack, and something else enabled:
| also enabled | WARN |
|---|---|
other-pack@claude-mp |
1 |
the lookalike vsdd-factory-x@claude-mp |
1 |
| both marketplaces listed, both suppressed locally | 1 |
| the second marketplace enabled only at user scope and suppressed locally | 1 |
Two mutants I added survive every test, including the table above: the new loop returning on any enabled identity, and on HasPrefix(id, opts.Pack). Both are equivalent. The census admits only identities whose plugin name equals the pack: installed_plugins.json entries at internal/foreign/census.go:196, enables at :276. So view.EffectivelyEnabled never holds another pack's identity. The opts.Pack+"@" filter is redundant today; it stays correct if the census widens.
The INFO fixture. TestAdopt_DryRunInfoFindingKeepsTheCleanVerdict asserts that INFO [5 agent-key-audit] is printed and that no WARN [ appears. It also asserts the full verdict line, so the fixture both raises an INFO and reaches the dry-run verdict. It passes at 877dd3c by design.
Mutants. Each of the 4 listed fails the tests the body names. The INFO-as-warning mutant is the one that survived my #196 review; it now fails the new INFO test. My extra mutant, dropping the suppressed loop, fails three of the #196 WARN tests.
CI on 612b902 passes.
Seat: reviewer / reviewer
The new default-agent warning from #196 can fire when the agent still resolves: an identity suppressed at one scope can be enabled again at a narrower one, or a second marketplace can serve the same plugin. A false WARN on a working repo teaches operators to ignore the check, so this closes both cases. It also adds the INFO fixture the adopt dry-run tests were missing.
Follow-up to #95 and #196 (review findings a and b). One early return in
checkDefaultAgent, plus tests; no output changes for a repo that really dangles.What changed:
checkDefaultAgent(internal/coexistcheck/coexistcheck.go) returns early whenview.EffectivelyEnabledholds any identity of the pack. The agent namespace is the plugin name, so any enabled identity keeps the agent resolvable.TestAdopt_DryRunInfoFindingKeepsTheCleanVerdictraises an INFO (a prefixed agent entry) and no WARN, and asserts the dry run keepspreflight clean. The zero-finding control could not tell INFO from WARN. This test passes on the parent commit; it pins behavior a mutant had survived.Pins for (a), red in 877dd3c: an identity suppressed in the committed settings but enabled again in
settings.local.json(..WhenReEnabledLocally), and a second marketplace enabled while the first is suppressed (..WhenASecondMarketplaceIsEnabled). The #196 tests that expect the WARN still pass.Mutants, each compiled and killed by a named test: early return removed (both (a) pins); early return only when nothing is suppressed (both (a) pins); early return for the first marketplace only (the second-marketplace pin); the adopt dry run counts INFO as warnings (the new INFO test).
Gates, each run on its own: build, vet,
go test -count=1 ./..., golangci-lint (0 issues), gofumpt -l (empty).Refs #95