Skip to content

Commit b5365e8

Browse files
committed
fix(skilldoc): exclude runnable command groups from card dump
newGroupCmd (#101) gives every command group a real RunE so a mistyped subcommand fails loudly, which made Build's `c.Runnable() && !c.Hidden` predicate start emitting a card entry for all 23 groups (incident, oncall, change, ...). A group only dispatches to its subcommands; it has no behavior of its own, so documenting it as an invocable command is wrong content, not just churn. Key the predicate off HasSubCommands() instead: every node in the tree with children is a pure container (no command mixes its own business logic with child commands), so this cleanly separates groups from leaves regardless of Runnable(). Regenerating all fences with this fix reproduces the base branch's cards byte-for-byte.
1 parent 7238b3a commit b5365e8

2 files changed

Lines changed: 61 additions & 3 deletions

File tree

internal/skilldoc/build.go

Lines changed: 22 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -9,8 +9,10 @@ import (
99
)
1010

1111
// Build walks the cobra tree rooted at root and returns a structured dump of
12-
// every runnable, non-hidden leaf command. Group containers (non-runnable
13-
// parents like "status-page") are descended into but not emitted themselves.
12+
// every runnable, non-hidden leaf command. Group containers (parents with
13+
// their own subcommands, like "status-page") are descended into but not
14+
// emitted themselves — see the predicate comment in walk for why this is
15+
// keyed off subcommands rather than Runnable().
1416
//
1517
// Path is the space-joined chain of cobra command names below the root, using
1618
// c.Name() so a positional placeholder in Use (e.g. "change-create <page-id>")
@@ -33,7 +35,24 @@ func walk(c *cobra.Command, parents []string, d *Dump) {
3335
path = append(append([]string{}, parents...), c.Name())
3436
}
3537

36-
if c.Runnable() && !c.Hidden {
38+
// A card entry is only correct for a leaf an agent can actually invoke to
39+
// get work done. c.Runnable() alone is NOT that signal: every command
40+
// group in this tree (alert, incident, oncall schedule, incident
41+
// war-room, the generated genGroup groups, ...) is built through
42+
// newGroupCmd (internal/cli/command.go), which gives it a real RunE
43+
// (print help) and Args validator (groupUnknownSubcommand) so a typo'd
44+
// subcommand fails loudly instead of cobra silently discarding it — see
45+
// that constructor's doc comment. That makes every group Runnable too,
46+
// even though its RunE does nothing but dispatch to children. The
47+
// distinguishing fact is not "does it run" but "does it hold
48+
// subcommands": every node in this tree with children is a pure
49+
// container (verified when this predicate was fixed — no command mixes
50+
// its own business logic with child commands), so HasSubCommands() is
51+
// the correct group/leaf split. If that ever stops being true — a
52+
// command gains both real behavior of its own AND subcommands — this
53+
// predicate must change to emit a card for that command's own behavior
54+
// while still not treating its children as absent.
55+
if !c.HasSubCommands() && c.Runnable() && !c.Hidden {
3756
d.Commands = append(d.Commands, command(c, path))
3857
}
3958

internal/skilldoc/build_test.go

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -49,3 +49,42 @@ func TestBuild_CapturesLeafWithFlagsAndRequired(t *testing.T) {
4949
t.Errorf("--type should be present and required: %+v", got.Flags)
5050
}
5151
}
52+
53+
// runnableGroupTree mirrors internal/cli.newGroupCmd: a container command that
54+
// is Runnable (RunE just prints help, same as every group in the real tree —
55+
// alert, incident, oncall schedule, ...) purely so a mistyped subcommand fails
56+
// loudly instead of cobra discarding the leftover arg silently. Runnable()
57+
// alone therefore cannot distinguish a group from a leaf; a command with its
58+
// own subcommands must be excluded regardless of whether it happens to be
59+
// Runnable.
60+
func runnableGroupTree() *cobra.Command {
61+
root := &cobra.Command{Use: "fduty"}
62+
incident := &cobra.Command{
63+
Use: "incident", Short: "Manage incidents",
64+
RunE: func(cmd *cobra.Command, _ []string) error { return cmd.Help() },
65+
}
66+
list := &cobra.Command{Use: "list", Short: "List incidents", Run: func(*cobra.Command, []string) {}}
67+
incident.AddCommand(list)
68+
root.AddCommand(incident)
69+
return root
70+
}
71+
72+
func TestBuild_ExcludesRunnableGroupWithSubcommands(t *testing.T) {
73+
d := Build(runnableGroupTree())
74+
75+
for _, c := range d.Commands {
76+
if c.Path == "incident" {
77+
t.Fatalf("group %q has subcommands and no behavior of its own beyond dispatching to them; it must not get a card entry (it would falsely document it as an invocable command): %+v", c.Path, c)
78+
}
79+
}
80+
81+
var gotLeaf bool
82+
for _, c := range d.Commands {
83+
if c.Path == "incident list" {
84+
gotLeaf = true
85+
}
86+
}
87+
if !gotLeaf {
88+
t.Fatalf("leaf %q under a runnable group must still get a card entry; got %+v", "incident list", d.Commands)
89+
}
90+
}

0 commit comments

Comments
 (0)