Skip to content

Commit 7b681a4

Browse files
BagToadCopilot
andcommitted
fix(cmdutil): honor DisableAuthCheck under repo-override parents
EnableRepoOverride's hook shadows the root auth gate, then re-runs the nearest ancestor hook to restore it. That re-run passed the ancestor as the command, so the gate judged the wrong node and ignored a leaf's DisableAuthCheck. Pass the invoked leaf instead, as cobra does for every persistent hook. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent f0a128a commit 7b681a4

2 files changed

Lines changed: 91 additions & 6 deletions

File tree

pkg/cmdutil/repo_override.go

Lines changed: 21 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -9,11 +9,25 @@ import (
99
"github.com/spf13/cobra"
1010
)
1111

12-
func executeParentHooks(cmd *cobra.Command, args []string) error {
13-
for cmd.HasParent() {
14-
cmd = cmd.Parent()
15-
if cmd.PersistentPreRunE != nil {
16-
return cmd.PersistentPreRunE(cmd, args)
12+
// executeParentHook re-runs the nearest ancestor's persistent pre-run hook,
13+
// which the hook installed by EnableRepoOverride would otherwise shadow. By
14+
// default cobra runs only the nearest PersistentPreRunE found walking up from
15+
// the invoked command, so without this the nearest ancestor hook, such as the
16+
// root auth gate, would never run for a repo-override command.
17+
//
18+
// That ancestor hook receives the invoked leaf cmd, not the ancestor, matching
19+
// how cobra passes the leaf to every persistent hook:
20+
// https://github.com/spf13/cobra/blob/v1.10.2/command.go#L984-L986
21+
//
22+
// cobra's EnableTraverseRunHooks global is the native equivalent and runs the
23+
// whole root-to-leaf chain for us, but it is global. Enabling it would change
24+
// pre-run behavior for every command: double-running the parents that issue
25+
// develop and EnableRepoOverride re-run by hand, and un-suppressing the root
26+
// gate that agent-task and skills intentionally shadow.
27+
func executeParentHook(overrideCmd, cmd *cobra.Command, args []string) error {
28+
for p := overrideCmd.Parent(); p != nil; p = p.Parent() {
29+
if p.PersistentPreRunE != nil {
30+
return p.PersistentPreRunE(cmd, args)
1731
}
1832
}
1933
return nil
@@ -47,8 +61,9 @@ func EnableRepoOverride(cmd *cobra.Command, f *Factory) {
4761
return results, cobra.ShellCompDirectiveNoFileComp
4862
})
4963

64+
overrideCmd := cmd
5065
cmd.PersistentPreRunE = func(cmd *cobra.Command, args []string) error {
51-
if err := executeParentHooks(cmd, args); err != nil {
66+
if err := executeParentHook(overrideCmd, cmd, args); err != nil {
5267
return err
5368
}
5469
repoOverride, _ := cmd.Flags().GetString("repo")

pkg/cmdutil/repo_override_test.go

Lines changed: 70 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,70 @@
1+
package cmdutil
2+
3+
import (
4+
"testing"
5+
6+
"github.com/spf13/cobra"
7+
"github.com/stretchr/testify/require"
8+
)
9+
10+
// Test_EnableRepoOverride_authCheckIntegration is an integration test for the
11+
// coupling between repo override and the root auth gate, wired through cobra's
12+
// persistent pre-run hooks. EnableRepoOverride replaces a command's
13+
// PersistentPreRunE, so cobra no longer reaches the root auth gate on its own;
14+
// the override hook re-runs the ancestor hooks itself. That re-run must evaluate
15+
// the command the user actually invoked, the same way cobra hands the target
16+
// command to parent hooks:
17+
// https://github.com/spf13/cobra/blob/v1.10.2/command.go#L984-L986
18+
// This pins that a leaf's DisableAuthCheck is honored under a repo-override
19+
// parent.
20+
func Test_EnableRepoOverride_authCheckIntegration(t *testing.T) {
21+
tests := []struct {
22+
name string
23+
disableAuthLeaf bool
24+
wantAuthChecked bool
25+
}{
26+
{
27+
name: "leaf opts out, honored under repo-override parent",
28+
disableAuthLeaf: true,
29+
wantAuthChecked: false,
30+
},
31+
{
32+
name: "leaf does not opt out, auth still checked",
33+
disableAuthLeaf: false,
34+
wantAuthChecked: true,
35+
},
36+
}
37+
38+
for _, tt := range tests {
39+
t.Run(tt.name, func(t *testing.T) {
40+
var gotAuthChecked, ranLeaf bool
41+
42+
// Stand in for the real root auth gate: it judges whatever command
43+
// it is handed, so it must receive the invoked leaf command.
44+
root := &cobra.Command{Use: "root"}
45+
root.PersistentPreRunE = func(cmd *cobra.Command, args []string) error {
46+
gotAuthChecked = IsAuthCheckEnabled(cmd)
47+
return nil
48+
}
49+
50+
parent := &cobra.Command{Use: "parent"}
51+
EnableRepoOverride(parent, &Factory{})
52+
root.AddCommand(parent)
53+
54+
leaf := &cobra.Command{
55+
Use: "leaf",
56+
RunE: func(cmd *cobra.Command, args []string) error { ranLeaf = true; return nil },
57+
}
58+
if tt.disableAuthLeaf {
59+
DisableAuthCheck(leaf)
60+
}
61+
parent.AddCommand(leaf)
62+
63+
root.SetArgs([]string{"parent", "leaf"})
64+
require.NoError(t, root.Execute())
65+
66+
require.True(t, ranLeaf, "leaf command should have executed")
67+
require.Equal(t, tt.wantAuthChecked, gotAuthChecked)
68+
})
69+
}
70+
}

0 commit comments

Comments
 (0)