feat(trail): guard --repo on approval and comment commands (review) · Entire
feat(trail): guard --repo on approval and comment commands (review)
086274e→main·
computermode·yesterday·4 files·+23 added/-12 removed
Extend ensureTrailRepoHasTarget to the newly added approve,
request-changes, approvals, and comment subcommands so a --repo
override cannot silently resolve the local branch against another repo.
Change the guard to take repoOverride instead of *cobra.Command so the
approval/comment helpers can reuse it.
Co-Authored-By: Claude Opus 4.8 noreply@anthropic.com
Sessions
01KXP4Z7XEDYS3HJJCRMK6SHDGView transcript
Changes
4
cmd/entire/cli
Mtrail_approval_cmd.go+6
Mtrail_cmd.go+8/-10
Mtrail_comment_cmd.go+3
Mtrail_repo_flag_test.go+6/-2
58 unmodified lines
59
60
61
62
63
64
65
66
67
82 unmodified lines
150
151
152
153
154
155
156
157
158
58 unmodified lines
if selector != "" && strings.TrimSpace(branch) != "" {
return errors.New("pass a trail selector or --branch, not both")
}
if err := ensureTrailRepoHasTarget(repoOverride, selector != "" || strings.TrimSpace(branch) != "", "pass a trail selector or --branch"); err != nil {
return err
}
req, err := buildApprovalRequest(event, message)
if err != nil {
return err
82 unmodified lines
if selector != "" && strings.TrimSpace(branch) != "" {
return errors.New("pass a trail selector or --branch, not both")
}
if err := ensureTrailRepoHasTarget(repoOverride, selector != "" || strings.TrimSpace(branch) != "", "pass a trail selector or --branch"); err != nil {
return err
}
// Auth/not-logged-in messages go to stderr; w carries command output only.
return runAuthenticatedTrailAPI(ctx, errW, insecureHTTP, repoOverride, func(ctx context.Context, client *api.Client) error {
found, forge, owner, repoName, err := resolveNumberedTrail(ctx, client, repoOverride, selector, branch)
Mcmd/entire/cli/trail_approval_cmd.go+6
123 unmodified lines
124
125
126
127
128
129
130
131
132
133
127
128
129
130
131
132
133
134
30 unmodified lines
165
166
167
170
168
169
170
171
936 unmodified lines
1108
1109
1110
1113
1111
1112
1113
1114
493 unmodified lines
1608
1609
1610
1613
1611
1612
1613
1614
123 unmodified lines
return nil
// ensureTrailRepoHasTarget requires an explicit branch or trail selector when
// --repo targets a repository other than the local clone. Without one, the
// branch-defaulting commands fall back to the local checkout's current branch,
// which would silently resolve the wrong trail (a shared branch name) in the
// overridden repo. hint names the acceptable targets for the command.
func ensureTrailRepoHasTarget(cmd *cobra.Command, hasTarget bool, hint string) error {
if trailRepoFlag(cmd) != "" && !hasTarget {
// ensureTrailRepoHasTarget requires an explicit branch or selector when --repo
// targets another repo; otherwise the command would resolve the local branch
// against the wrong repo. hint names the acceptable targets.
func ensureTrailRepoHasTarget(repoOverride string, hasTarget bool, hint string) error {
if repoOverride != "" && !hasTarget {
return fmt.Errorf("--repo requires an explicit target: %s", hint)
}
return nil
30 unmodified lines
if selector != "" && trailBranchFlag(cmd) != "" {
return errors.New("pass a trail selector or --branch, not both")
}
if err := ensureTrailRepoHasTarget(cmd, selector != "" || trailBranchFlag(cmd) != "", "pass a trail selector or --branch"); err != nil {
if err := ensureTrailRepoHasTarget(trailRepoFlag(cmd), selector != "" || trailBranchFlag(cmd) != "", "pass a trail selector or --branch"); err != nil {
return err
}
return runTrailShow(cmd.Context(), cmd.OutOrStdout(), cmd.ErrOrStderr(), trailInsecureHTTP(cmd), selector, trailRepoFlag(cmd), trailBranchFlag(cmd))
936 unmodified lines
Short: "Update trail metadata",
Args: cobra.NoArgs,
RunE: func(cmd *cobra.Command, _ []string) error {
if err := ensureTrailRepoHasTarget(cmd, strings.TrimSpace(branch) != "", "pass --branch"); err != nil {
if err := ensureTrailRepoHasTarget(trailRepoFlag(cmd), strings.TrimSpace(branch) != "", "pass --branch"); err != nil {
return err
}
return runTrailUpdate(cmd.Context(), cmd.OutOrStdout(), cmd.ErrOrStderr(), trailInsecureHTTP(cmd), trailUpdateInputs{
493 unmodified lines
if number > 0 && cmd.Flags().Changed("branch") {
return errors.New("cannot combine a trail <number> with --branch")
}
if err := ensureTrailRepoHasTarget(cmd, number > 0 || strings.TrimSpace(branch) != "", "pass a trail number or --branch"); err != nil {
if err := ensureTrailRepoHasTarget(trailRepoFlag(cmd), number > 0 || strings.TrimSpace(branch) != "", "pass a trail number or --branch"); err != nil {
return err
}
return runTrailDelete(cmd, number, branch, force)
Mcmd/entire/cli/trail_cmd.go+8/-10
47 unmodified lines
48
49
50
51
52
53
54
55
56
47 unmodified lines
if selector != "" && strings.TrimSpace(branch) != "" {
return errors.New("pass --trail or --branch, not both")
}
if err := ensureTrailRepoHasTarget(repoOverride, selector != "" || strings.TrimSpace(branch) != "", "pass --trail or --branch"); err != nil {
return err
}
// Auth/not-logged-in messages go to stderr; w carries command output only.
return runAuthenticatedTrailAPI(cmd.Context(), cmd.ErrOrStderr(), trailInsecureHTTP(cmd), repoOverride, func(ctx context.Context, client *api.Client) error {
found, forge, owner, repo, err := resolveNumberedTrail(ctx, client, repoOverride, selector, branch)
Mcmd/entire/cli/trail_comment_cmd.go+3
135 unmodified lines
136
137
138
139
140
139
140
141
142
5 unmodified lines
148
149
150
151
152
153
154
155
156
157
158
135 unmodified lines
}
}
// --repo must not silently fall back to the local checkout's branch: the
// branch-defaulting commands require an explicit branch or selector alongside it.
// --repo requires an explicit branch or selector rather than defaulting to the local branch.
func TestTrailRepoRequiresExplicitTarget(t *testing.T) {
t.Parallel()
tests := []struct {
5 unmodified lines
{name: "update", args: []string{"update", "--repo", "gh/acme/app"}},
{name: "delete", args: []string{"delete", "--repo", "gh/acme/app"}},
{name: "finding list", args: []string{"finding", "list", "--repo", "gh/acme/app"}},
{name: "approve", args: []string{"approve", "--repo", "gh/acme/app"}},
{name: "request-changes", args: []string{"request-changes", "--repo", "gh/acme/app", "-m", "why"}},
{name: "approvals", args: []string{"approvals", "--repo", "gh/acme/app"}},
{name: "comment list", args: []string{"comment", "list", "--repo", "gh/acme/app"}},
{name: "comment add", args: []string{"comment", "add", "--repo", "gh/acme/app", "-m", "hi"}},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
Mcmd/entire/cli/trail_repo_flag_test.go+6/-2