From cb05186743da36202c8c99d5baf4a48cfb9dc952 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 9 Oct 2026 18:48:38 +0000 Subject: [PATCH 1/2] test(cli): make the --agents dry-run orphan assertion able to fail TestApply_AgentsFlagKeepsSiblingSharedSkill checked that `apply --dry-run --agents pi` does not mention toolkit-audit. The dry run never prints orphan paths, only removal counts ("Removals: 1 key(s), 1 file(s)"), so that row passed whether or not the sibling-owner keep-set reached the dry-run plan: dropping the dry-run WithSiblingOwners call left the suite green. The dry-run row now also fails on a previewed file removal ("file(s)"). The remaining key removal is pi's own mcp.json cleanup and is expected. Verified by dropping the dry-run WithSiblingOwners call: the new assertion fails on "file(s)", the old one passed. Follow-up to #265. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01KbDKxyXxWNZckYFgMf4DYc --- internal/cli/plugin_agents_test.go | 26 ++++++++++++++++++-------- 1 file changed, 18 insertions(+), 8 deletions(-) diff --git a/internal/cli/plugin_agents_test.go b/internal/cli/plugin_agents_test.go index 316a7ec9..8981ede2 100644 --- a/internal/cli/plugin_agents_test.go +++ b/internal/cli/plugin_agents_test.go @@ -376,17 +376,27 @@ func TestApply_AgentsFlagKeepsSiblingSharedSkill(t *testing.T) { // Codex owns it in state. No --agents pi surface may offer it as an orphan: // status would report a deletion, the dry run would preview one, and // reconcile would prompt to remove a file Codex still depends on. - for _, args := range [][]string{ - {"status", "--agents", "pi"}, - {"apply", "--dry-run", "--agents", "pi"}, - {"reconcile", "--auto-safe", "--agents", "pi"}, + // + // The dry run never prints orphan paths, only the removal counts + // ("Removals: 1 key(s), 1 file(s)"), so checking it for the skill name + // alone could never fail. A previewed file removal is the leak there; the + // key removal is pi's own MCP cleanup and is expected. + for _, tc := range []struct { + args []string + leaks []string + }{ + {args: []string{"status", "--agents", "pi"}, leaks: []string{"toolkit-audit"}}, + {args: []string{"apply", "--dry-run", "--agents", "pi"}, leaks: []string{"toolkit-audit", "file(s)"}}, + {args: []string{"reconcile", "--auto-safe", "--agents", "pi"}, leaks: []string{"toolkit-audit"}}, } { - out, err := runCLI(t, env, args...) + out, err := runCLI(t, env, tc.args...) if err != nil { - t.Fatalf("%s: %v\n%s", strings.Join(args, " "), err, out) + t.Fatalf("%s: %v\n%s", strings.Join(tc.args, " "), err, out) } - if strings.Contains(out, "toolkit-audit") { - t.Errorf("%s offered the skill Codex still owns:\n%s", strings.Join(args, " "), out) + for _, leak := range tc.leaks { + if strings.Contains(out, leak) { + t.Errorf("%s offered the skill Codex still owns (found %q):\n%s", strings.Join(tc.args, " "), leak, out) + } } } if out, err := runCLI(t, env, "apply", "--agents", "pi"); err != nil { From 0e878e61aec28250183c4fc10e9123f9aa9da66d Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 9 Oct 2026 19:11:30 +0000 Subject: [PATCH 2/2] test(cli): pin the dry-run Removals line exactly, not only the absence of "file(s)" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review-loop round 1 on #278 (convergent across all four lenses): the dry-run row only asserted that "file(s)" was absent, so a reword of removedLabel would silently make it vacuous again, and it never checked what the dry run does print. The row now also requires the exact expected line, "Removals: 1 key(s) (the real apply will remove these)" — pi's own MCP cleanup. The comment quotes that expected line instead of the leaked one. Break-verified, each alone with apply.go restored afterwards: - dropping the dry-run WithSiblingOwners call fails both checks; - rewording the label to "%d files" AND dropping the call fails the new exact-line check (the old absence check passed); - suppressing the Removals line fails the exact-line check. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01KbDKxyXxWNZckYFgMf4DYc --- internal/cli/plugin_agents_test.go | 30 ++++++++++++++++++++++-------- 1 file changed, 22 insertions(+), 8 deletions(-) diff --git a/internal/cli/plugin_agents_test.go b/internal/cli/plugin_agents_test.go index 8981ede2..145baa64 100644 --- a/internal/cli/plugin_agents_test.go +++ b/internal/cli/plugin_agents_test.go @@ -377,25 +377,39 @@ func TestApply_AgentsFlagKeepsSiblingSharedSkill(t *testing.T) { // status would report a deletion, the dry run would preview one, and // reconcile would prompt to remove a file Codex still depends on. // - // The dry run never prints orphan paths, only the removal counts - // ("Removals: 1 key(s), 1 file(s)"), so checking it for the skill name - // alone could never fail. A previewed file removal is the leak there; the - // key removal is pi's own MCP cleanup and is expected. + // The dry run never prints orphan paths, only the removal counts, so + // checking it for the skill name alone could never fail. Its removal line + // is pinned exactly instead: pi's own MCP cleanup is the one expected + // removal ("Removals: 1 key(s) (…)"), and a leaked skill delete turns it + // into "1 key(s), 1 file(s)". Pinning the whole line, rather than only the + // absence of "file(s)", keeps this row from going vacuous again if the + // label is ever reworded. for _, tc := range []struct { args []string - leaks []string + leaks []string // must not appear + wants []string // must appear }{ {args: []string{"status", "--agents", "pi"}, leaks: []string{"toolkit-audit"}}, - {args: []string{"apply", "--dry-run", "--agents", "pi"}, leaks: []string{"toolkit-audit", "file(s)"}}, + { + args: []string{"apply", "--dry-run", "--agents", "pi"}, + leaks: []string{"toolkit-audit", "file(s)"}, + wants: []string{"Removals: 1 key(s) (the real apply will remove these)"}, + }, {args: []string{"reconcile", "--auto-safe", "--agents", "pi"}, leaks: []string{"toolkit-audit"}}, } { + cmd := strings.Join(tc.args, " ") out, err := runCLI(t, env, tc.args...) if err != nil { - t.Fatalf("%s: %v\n%s", strings.Join(tc.args, " "), err, out) + t.Fatalf("%s: %v\n%s", cmd, err, out) } for _, leak := range tc.leaks { if strings.Contains(out, leak) { - t.Errorf("%s offered the skill Codex still owns (found %q):\n%s", strings.Join(tc.args, " "), leak, out) + t.Errorf("%s offered the skill Codex still owns (found %q):\n%s", cmd, leak, out) + } + } + for _, want := range tc.wants { + if !strings.Contains(out, want) { + t.Errorf("%s: want %q in output:\n%s", cmd, want, out) } } }