Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 22 additions & 8 deletions cmd/oap/internal/agentcmd/setup_identity.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ func newAgentSetupIdentityCmd(g *apcmd.Globals) *cobra.Command {
var (
identity string
only string
refresh string
force bool
opts identitycmd.SetupOptions
)
Expand All @@ -34,6 +35,10 @@ If the AgentIdentity (spec.agentIdentity, or --identity override) does
not exist, prompts to create it.

--only <prefix>:<name> run setup for one entity only (e.g. --only mcp:linear)
--refresh <credential> re-run setup for just one credential by name, the name
shown in the "already set up: <name>" line, forcing past
the idempotency check (e.g. --refresh tailscale-authkey);
shows a "did you mean?" on an unknown name
--identity <name> override AgentClass.spec.agentIdentity
--force re-run flows even when the credential is already set up`,
Args: cobra.ExactArgs(1),
Expand All @@ -44,18 +49,26 @@ not exist, prompts to create it.
}
opts.NoColor = g.NoColor
return runAgentSetupIdentity(cmd.Context(), cmd.OutOrStdout(), cmd.ErrOrStderr(),
os.Stdin, b.Controller, b.Namespace, args[0], identity, only, force, opts)
os.Stdin, b.Controller, b.Namespace, args[0], identity, only, refresh, force, opts)
},
}
cmd.Flags().StringVar(&identity, "identity", "", "Override AgentClass.spec.agentIdentity")
cmd.Flags().StringVar(&only, "only", "", "Run setup for one entity only (e.g. mcp:linear)")
// --refresh selects by CREDENTIAL name (the name the user sees in the
// "already set up: <name>" line), not by target entity like --only; it is the
// easy path for "my tailscale key expired, re-do just that one". Registered
// here rather than in AddSetupFlags for the same reason --only is agent-form
// only: the no-class 'oap identity setup' already filters by --toolkits/--mcps.
cmd.Flags().StringVar(&refresh, "refresh", "",
"Re-run setup for just one credential by name (the name shown in 'already set up: <name>'), "+
"forcing past the idempotency check (e.g. --refresh tailscale-authkey)")
identitycmd.AddSetupFlags(cmd, &opts, &force)
return cmd
}

func runAgentSetupIdentity(
ctx context.Context, out, errOut io.Writer, stdin io.Reader,
c client.Client, namespace, className, identityOverride, only string, force bool,
c client.Client, namespace, className, identityOverride, only, refresh string, force bool,
opts identitycmd.SetupOptions,
) error {
var ac spiceboxv1alpha1.AgentClass
Expand Down Expand Up @@ -87,11 +100,12 @@ func runAgentSetupIdentity(

intent := ac.Spec.Description
return identitycmd.RunSetup(ctx, c, out, errOut, stdin, opts, setup.RunRequest{
Namespace: namespace,
IdentityName: identityName,
UserIntent: intent,
Targets: targets,
OnlyMatch: only,
Force: force,
Namespace: namespace,
IdentityName: identityName,
UserIntent: intent,
Targets: targets,
OnlyMatch: only,
Force: force,
RefreshCredential: refresh,
})
}
54 changes: 37 additions & 17 deletions cmd/oap/internal/agentcmd/setup_identity_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ func makeGhToolspec(name string) *spiceboxv1alpha1.SpiceboxToolspec {
func TestAgentSetupIdentity_NotFound(t *testing.T) {
c := aptest.IdentityClientBuilder(t).Build()
err := runAgentSetupIdentity(context.Background(), io.Discard, io.Discard,
strings.NewReader(""), c, "default", "nonexistent", "", "", false, identitycmd.SetupOptions{})
strings.NewReader(""), c, "default", "nonexistent", "", "", "", false, identitycmd.SetupOptions{})
require.Error(t, err, "missing AgentClass should error")
assert.Contains(t, err.Error(), "get AgentClass", "error should mention 'get AgentClass'")
}
Expand All @@ -92,7 +92,7 @@ func TestAgentSetupIdentity_NoIdentity(t *testing.T) {
c := aptest.IdentityClientBuilder(t).WithObjects(ac).Build()

err := runAgentSetupIdentity(context.Background(), io.Discard, io.Discard,
strings.NewReader(""), c, "default", "my-class", "", "", false, identitycmd.SetupOptions{})
strings.NewReader(""), c, "default", "my-class", "", "", "", false, identitycmd.SetupOptions{})
require.Error(t, err, "missing agentIdentity should error")
assert.Contains(t, err.Error(), "no spec.agentIdentity", "error should mention 'no spec.agentIdentity'")
}
Expand All @@ -109,7 +109,7 @@ func TestAgentSetupIdentity_IdentityOverride(t *testing.T) {
// No toolspecs/MCPs → "nothing to set up" early return.
require.NoError(t,
runAgentSetupIdentity(context.Background(), &out, io.Discard,
strings.NewReader(""), c, "default", "my-class", "override-identity", "", false, identitycmd.SetupOptions{}),
strings.NewReader(""), c, "default", "my-class", "override-identity", "", "", false, identitycmd.SetupOptions{}),
"runAgentSetupIdentity")
assert.Contains(t, out.String(), "nothing to set up", "output should report nothing to set up")
}
Expand All @@ -123,7 +123,7 @@ func TestAgentSetupIdentity_NothingToSetUp(t *testing.T) {
var out strings.Builder
require.NoError(t,
runAgentSetupIdentity(context.Background(), &out, io.Discard,
strings.NewReader(""), c, "default", "my-class", "", "", false, identitycmd.SetupOptions{}),
strings.NewReader(""), c, "default", "my-class", "", "", "", false, identitycmd.SetupOptions{}),
"runAgentSetupIdentity")
assert.Contains(t, out.String(), "nothing to set up", "output should report nothing to set up")
}
Expand Down Expand Up @@ -160,7 +160,7 @@ func TestAgentSetupIdentity_HappyPath(t *testing.T) {

require.NoErrorf(t,
runAgentSetupIdentity(context.Background(), &out, io.Discard,
stdin, c, "default", "my-class", "", "", false, identitycmd.SetupOptions{}),
stdin, c, "default", "my-class", "", "", "", false, identitycmd.SetupOptions{}),
"runAgentSetupIdentity; output: %s", out.String())

// AgentIdentity should have been created.
Expand All @@ -182,26 +182,46 @@ func TestAgentSetupIdentity_HappyPath(t *testing.T) {
}
}

// TestAgentSetupIdentity_OnlyFilterSkipsNonMatching verifies that --only
// filters the target list so unmatched entries are skipped (no-op).
func TestAgentSetupIdentity_OnlyFilterSkipsNonMatching(t *testing.T) {
// TestAgentSetupIdentity_RefreshUnknownNameSuggests verifies --refresh is wired
// through the command end-to-end and that a wrong credential name fails with a
// "did you mean?" pointer instead of a silent no-op. The engine errors while
// enumerating requirements, before any flow runs, so no token/browser is needed.
func TestAgentSetupIdentity_RefreshUnknownNameSuggests(t *testing.T) {
browsertest.Record(t)

ts := makeGhToolspec("gh-readonly")
ac := makeAgentSetupClass("default", "my-class", "my-bot", []string{"gh-readonly"}, nil)
c := aptest.IdentityClientBuilder(t).WithObjects(ac, ts).Build()

var out strings.Builder
// --only with a non-matching value → all targets filtered out → success with no credentials created.
require.NoError(t,
runAgentSetupIdentity(context.Background(), &out, io.Discard,
strings.NewReader("ghp_xxxx\n"), c, "default", "my-class", "",
"toolspec:does-not-exist", false, identitycmd.SetupOptions{}),
"runAgentSetupIdentity")
err := runAgentSetupIdentity(context.Background(), &out, io.Discard,
strings.NewReader(""), c, "default", "my-class", "", "", "github-tokn", false, identitycmd.SetupOptions{})
require.Error(t, err, "an unknown --refresh name must error")
assert.Contains(t, err.Error(), "did you mean", "a near-miss name gets a suggestion")
assert.Contains(t, err.Error(), "github-token", "the suggestion names the real credential")
}

// TestAgentSetupIdentity_OnlyMatchesNoTargetErrors verifies that an --only
// value matching no target is reported as an error (almost always a typo),
// naming the valid targets, rather than exiting cleanly having done nothing.
func TestAgentSetupIdentity_OnlyMatchesNoTargetErrors(t *testing.T) {
browsertest.Record(t)

// No AgentIdentity should have been created (nothing was processed).
ts := makeGhToolspec("gh-readonly")
ac := makeAgentSetupClass("default", "my-class", "my-bot", []string{"gh-readonly"}, nil)
c := aptest.IdentityClientBuilder(t).WithObjects(ac, ts).Build()

var out strings.Builder
err := runAgentSetupIdentity(context.Background(), &out, io.Discard,
strings.NewReader("ghp_xxxx\n"), c, "default", "my-class", "",
"toolspec:does-not-exist", "", false, identitycmd.SetupOptions{})
require.Error(t, err, "an --only value matching no target must error, not silently no-op")
assert.Contains(t, err.Error(), "matched no target")
assert.Contains(t, err.Error(), "toolspec:gh-readonly", "the error names the valid target(s)")

// Nothing should have been created.
var ai spiceboxv1alpha1.AgentIdentity
if err := c.Get(context.Background(), client.ObjectKey{Namespace: "default", Name: "my-bot"}, &ai); err == nil {
assert.Empty(t, ai.Spec.Credentials, "expected no credentials when --only filter matched nothing")
if gerr := c.Get(context.Background(), client.ObjectKey{Namespace: "default", Name: "my-bot"}, &ai); gerr == nil {
assert.Empty(t, ai.Spec.Credentials, "expected no credentials when --only matched no target")
}
}
65 changes: 59 additions & 6 deletions pkg/platform/identity/setup/engine.go
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,15 @@ type RunRequest struct {
Force bool
// OnlyMatch, when non-empty, filters Targets to entries that exactly equal it.
OnlyMatch string
// RefreshCredential, when non-empty, scopes the run to the single credential
// of that name — the SuggestedName printed in the "already set up: <name>"
// line — re-running just that credential's flow forced past the idempotency
// check ("refresh this one, even though it looks fine") and skipping every
// other credential. A value matching no credential in the run's requirements
// is a hard error carrying the nearest matches, never a silent no-op. Set by
// the --refresh flag. Unlike OnlyMatch it selects by CREDENTIAL name, not by
// target entity, which is what a user reading that status line actually has.
RefreshCredential string

Stdin io.Reader
Stdout io.Writer
Expand Down Expand Up @@ -142,22 +151,56 @@ func Run(ctx context.Context, c client.Client, req RunRequest) error {
// command supplies it; this only substitutes plain text for a caller with none.
req.Theme = req.styles()

// --refresh and --force both re-run a credential's interactive setup flow,
// which --non-interactive forbids the prompts for. The combinations
// contradict, so reject them up front: otherwise the run reaches the flow,
// the non-interactive presenter refuses the first unanswered screen, and the
// user sees "<name> is not set up yet" — false for a credential being
// force-re-run — with a non-zero exit instead of a clear reason.
if req.NonInteractive && req.RefreshCredential != "" {
return fmt.Errorf("setup: --refresh re-runs %q's interactive setup, which --non-interactive forbids; "+
"drop --non-interactive", req.RefreshCredential)
}
if req.NonInteractive && req.Force {
return errors.New("setup: --force re-runs setup flows interactively, which --non-interactive forbids; drop one of them")
}

// A single credential is often required by several targets. seen dedups by
// credential across the whole run, so each one is provisioned — and printed —
// exactly once, not once per referencing target.
seen := map[string]bool{}
// --refresh scoping. Inert (every method a no-op, errors nil) when the flag
// was not given, so a run without it behaves exactly as before. only is
// carried so an unmatched --refresh error can name the --only scope that
// narrowed the candidates.
refresh := &refreshFilter{want: req.RefreshCredential, only: req.OnlyMatch}
onlyMatched := false
for _, t := range req.Targets {
if req.OnlyMatch != "" && t != req.OnlyMatch {
continue
}
if err := runOne(ctx, c, req, t, seen); err != nil {
onlyMatched = true
if err := runOne(ctx, c, req, t, seen, refresh); err != nil {
return err
}
}
// An --only that matched no target is almost always a typo: fail naming the
// valid targets rather than exit cleanly having silently done nothing. Checked
// before the --refresh error below, because a mistyped --only is the root
// cause when both would otherwise fire.
if req.OnlyMatch != "" && !onlyMatched {
return fmt.Errorf("setup: --only %q matched no target; targets are %s", req.OnlyMatch, strings.Join(req.Targets, ", "))
}
// A --refresh that named no credential the run actually iterated is a typo,
// not a no-op: fail with the nearest matches so the user learns nothing was
// refreshed and what they likely meant, rather than seeing a clean exit.
if err := refresh.unmatchedErr(); err != nil {
return err
}
return nil
}

func runOne(ctx context.Context, c client.Client, req RunRequest, matchStr string, seen map[string]bool) error {
func runOne(ctx context.Context, c client.Client, req RunRequest, matchStr string, seen map[string]bool, refresh *refreshFilter) error {
kind, suffix, err := registry.ParseBindingMatch(matchStr)
if err != nil {
return fmt.Errorf("setup: %s: %w", matchStr, err)
Expand All @@ -172,7 +215,7 @@ func runOne(ctx context.Context, c client.Client, req RunRequest, matchStr strin
return nil
}
for _, r := range reqs {
if err := runRequirement(ctx, c, req, kind, target, matchStr, r, seen); err != nil {
if err := runRequirement(ctx, c, req, kind, target, matchStr, r, seen, refresh); err != nil {
return err
}
}
Expand All @@ -182,7 +225,7 @@ func runOne(ctx context.Context, c client.Client, req RunRequest, matchStr strin
func runRequirement(
ctx context.Context, c client.Client, req RunRequest,
kind authkind.Kind, target authkind.Target, matchStr string,
r authkind.CredentialRequirement, seen map[string]bool,
r authkind.CredentialRequirement, seen map[string]bool, refresh *refreshFilter,
) error {
// Skip silently on repeat, before the separator below, so a credential shared
// by several targets is never printed or prompted twice.
Expand All @@ -192,11 +235,21 @@ func runRequirement(
}
seen[dedupKey] = true

// --refresh scoping: record this credential as a candidate (so a wrong
// --refresh value can be answered with a "did you mean?") and skip every
// credential except the one named. The named credential falls through and is
// forced past the idempotency check below. consider is a no-op returning true
// when --refresh was not given.
if !refresh.consider(r.SuggestedName) {
return nil
}

// Trailing blank line between requirements for visual separation.
defer fmt.Fprintln(req.Stdout)

// Idempotency check.
if !req.Force {
// Idempotency check — skipped for --force and for the --refresh target, both
// of which mean "re-run this flow even though the credential looks set up".
if !req.Force && !refresh.active() {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

--refresh combined with --non-interactive skips isAlreadySetUp, so a credential that is present and valid is never checked. The run then reaches the non-interactive presenter, which refuses with <name> is not set up yet (setup_present.go:194). That message is false here, and the command exits non-zero.

Repro: oap agent setup-identity my-class --refresh tailscale-authkey --non-interactive against a working key.

The two flags contradict each other, so I'd reject the combination at the top of Run, e.g. cannot --refresh without prompting; drop --non-interactive. --force + --non-interactive already behaves this way, so it could get the same guard.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rejected up front in Run now — --refresh with --non-interactive errors immediately (drop --non-interactive) rather than reaching the flow and hitting the false "not set up yet". Added the same guard for --force + --non-interactive, which was broken the same way.

ok, why, err := isAlreadySetUp(ctx, c, req.Namespace, req.IdentityName, r)
if err != nil {
return err
Expand Down
Loading
Loading