From c5931864e25a2cfa0f3fcf71bd20d66c20f387cc Mon Sep 17 00:00:00 2001 From: Bruce Du Date: Wed, 20 May 2026 12:36:59 +0800 Subject: [PATCH 01/10] feat: support team workspace via `zeabur workspace` (PLA-1590) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds a workspace concept to the CLI so team members can list, create, and deploy projects under a team they belong to — mirroring the dashboard's workspace switcher. Commands: zeabur workspace list list personal + all teams with role zeabur workspace current show the active workspace zeabur workspace switch switch to a team workspace zeabur workspace clear return to personal workspace Plus a global `--workspace ` flag for one-shot overrides without touching the persisted state. Resolution rules: - 24-char hex → treated as a team ObjectID; must be in the caller's memberships - non-hex → name match against memberships; 0 matches errors, 2+ matches errors with the concrete `zeabur workspace switch ` invocation per candidate (team names are unconstrained and may collide) - `switch personal` is NOT a fast path back to personal; it always means "find a team literally named 'personal'". Use `workspace clear` instead. Switching workspaces clears the persisted project / environment / service context because resource IDs do not overlap between workspaces. The clear-on-switch is surfaced in every `switch` / `clear` output line. Only directory-level operations (`project list`, `project create`, `deploy` without a linked project) consult the workspace. Operations that take a specific service or deployment ID stay workspace-independent — the resource's own owner already locates the team, and backend RBAC gates writes. Threading: a `Factory.CurrentOwnerID()` helper folds the --workspace flag override (resolved during PersistentPreRunE) with the persisted workspace, and selector.New takes a closure so the same Selector instance reflects mid-process workspace switches. Lazy startup verify: PersistentPreRunE calls `teams` once and clears the persisted workspace if the caller is no longer a member (team deleted, caller removed). Best-effort: transport errors leave the workspace alone so an offline blip doesn't switch the user out silently. Depends on backend PLA-1589 (Team.myRole field) for the per-team role shown in `workspace list` and in disambiguation errors. Co-Authored-By: Claude Opus 4.7 (1M context) --- README.md | 31 ++++++ internal/cmd/deploy/deploy.go | 11 +- internal/cmd/project/create/create.go | 2 +- internal/cmd/project/list/list.go | 2 +- internal/cmd/root/root.go | 63 +++++++++++- internal/cmd/template/deploy/deploy.go | 2 +- internal/cmd/workspace/clear/clear.go | 50 +++++++++ internal/cmd/workspace/current/current.go | 54 ++++++++++ internal/cmd/workspace/list/list.go | 90 +++++++++++++++++ internal/cmd/workspace/switch/switch.go | 68 +++++++++++++ internal/cmd/workspace/workspace.go | 41 ++++++++ internal/cmdutil/factory.go | 43 +++++++- internal/cmdutil/workspace.go | 85 ++++++++++++++++ internal/cmdutil/workspace_test.go | 118 ++++++++++++++++++++++ pkg/api/interface.go | 18 +++- pkg/api/project.go | 76 +++++++++----- pkg/api/team.go | 21 ++++ pkg/model/team.go | 36 +++++++ pkg/selector/selector.go | 28 +++-- pkg/zcontext/context.go | 32 ++++++ pkg/zcontext/interface.go | 17 +++- pkg/zcontext/workspace.go | 30 ++++++ 22 files changed, 870 insertions(+), 48 deletions(-) create mode 100644 internal/cmd/workspace/clear/clear.go create mode 100644 internal/cmd/workspace/current/current.go create mode 100644 internal/cmd/workspace/list/list.go create mode 100644 internal/cmd/workspace/switch/switch.go create mode 100644 internal/cmd/workspace/workspace.go create mode 100644 internal/cmdutil/workspace.go create mode 100644 internal/cmdutil/workspace_test.go create mode 100644 pkg/api/team.go create mode 100644 pkg/model/team.go create mode 100644 pkg/zcontext/workspace.go diff --git a/README.md b/README.md index eb2210a..d9d8e89 100644 --- a/README.md +++ b/README.md @@ -117,6 +117,37 @@ npx zeabur deployment log -t=build --env-id --service-name --help ``` +## Workspaces (personal / team) + +By default, the CLI acts under the personal workspace — the account that logged in. To list or create projects under a team you belong to, switch the workspace: + +```shell +# show your personal workspace + all teams you belong to, with your role per team +npx zeabur workspace list + +# switch to a team by name (or by 24-char team ID if the name is not unique) +npx zeabur workspace switch acme + +# show the workspace the CLI is currently using +npx zeabur workspace current + +# return to the personal workspace +npx zeabur workspace clear +``` + +Switching a workspace clears the pinned project / environment / service context, because resource IDs do not overlap between workspaces. + +The workspace only affects directory-level commands (`project list`, `project create`, `deploy` without a linked project). Commands that take a specific service or deployment ID use that resource's own owner and are workspace-independent — your team's `service restart` works the same regardless of which workspace is active. + +For one-off commands that should run under a different workspace without switching the persisted state, use the `--workspace` flag: + +```shell +# list projects in the "acme" team without switching workspaces +npx zeabur --workspace acme project list +``` + +`switch personal` is **not** a way to return to personal — it always looks for a team literally named `personal` (team names are unconstrained). Use `workspace clear` to go back. + ## Development Guide [Development Guide](docs/development_guide.md) diff --git a/internal/cmd/deploy/deploy.go b/internal/cmd/deploy/deploy.go index 5fe4ee8..370c206 100644 --- a/internal/cmd/deploy/deploy.go +++ b/internal/cmd/deploy/deploy.go @@ -195,7 +195,8 @@ func selectInteractively(f *cmdutil.Factory, opts *Options) (*model.Service, *mo spinner.WithSuffix(" Fetching projects ..."), ) s.Start() - projects, err := f.ApiClient.ListAllProjects(context.Background()) + ownerID := f.CurrentOwnerID() + projects, err := f.ApiClient.ListAllProjects(context.Background(), ownerID) if err != nil { return nil, nil, err } @@ -207,7 +208,13 @@ func selectInteractively(f *cmdutil.Factory, opts *Options) (*model.Service, *mo return nil, nil, err } if confirm { - project, err := f.ApiClient.CreateProject(context.Background(), "default", nil) + // When the active workspace is a team, make it visible that the + // new project will land under that team — not the personal + // account — so the user isn't surprised by where it shows up. + if ws := f.Config.GetContext().GetWorkspace(); ws.IsTeam() { + f.Log.Infof("→ Creating new project in team workspace %q", ws.Name) + } + project, err := f.ApiClient.CreateProject(context.Background(), ownerID, "default", nil) if err != nil { f.Log.Error("Failed to create project: ", err) return nil, nil, err diff --git a/internal/cmd/project/create/create.go b/internal/cmd/project/create/create.go index 947ea66..8e6a02c 100644 --- a/internal/cmd/project/create/create.go +++ b/internal/cmd/project/create/create.go @@ -104,7 +104,7 @@ func runCreateNonInteractive(f *cmdutil.Factory, opts *Options) error { } func createProject(f *cmdutil.Factory, projectRegion string, projectName *string) error { - project, err := f.ApiClient.CreateProject(context.Background(), projectRegion, projectName) + project, err := f.ApiClient.CreateProject(context.Background(), f.CurrentOwnerID(), projectRegion, projectName) if err != nil { f.Log.Error(err) return err diff --git a/internal/cmd/project/list/list.go b/internal/cmd/project/list/list.go index 5f7fc81..5030374 100644 --- a/internal/cmd/project/list/list.go +++ b/internal/cmd/project/list/list.go @@ -30,7 +30,7 @@ func NewCmdList(f *cmdutil.Factory) *cobra.Command { // runList will list all projects page by page func runList(f *cmdutil.Factory, opts Options) error { - projects, err := f.ApiClient.ListAllProjects(context.Background()) + projects, err := f.ApiClient.ListAllProjects(context.Background(), f.CurrentOwnerID()) if err != nil { return err } diff --git a/internal/cmd/root/root.go b/internal/cmd/root/root.go index 49adf43..569aa19 100644 --- a/internal/cmd/root/root.go +++ b/internal/cmd/root/root.go @@ -29,6 +29,7 @@ import ( uploadCmd "github.com/zeabur/cli/internal/cmd/upload" variableCmd "github.com/zeabur/cli/internal/cmd/variable" versionCmd "github.com/zeabur/cli/internal/cmd/version" + workspaceCmd "github.com/zeabur/cli/internal/cmd/workspace" "github.com/zeabur/cli/internal/cmdutil" "github.com/zeabur/cli/pkg/api" "github.com/zeabur/cli/pkg/config" @@ -98,7 +99,21 @@ func NewCmdRoot(f *cmdutil.Factory, version, commit, date string) (*cobra.Comman } // set up the client f.ApiClient = api.New(f.Config.GetTokenString()) - f.Selector = selector.New(f.ApiClient, f.Log, f.Prompter) + + // Resolve the --workspace flag (one-shot override) and lazy- + // verify the persisted workspace. Both steps are best-effort + // — flag errors abort the command (explicit user intent), but + // a verify hiccup (offline / 5xx) only warns; the user still + // gets to run their command. The selector reads the resolved + // owner via a closure on Factory so subsequent calls within + // the same process see flag overrides and switch updates + // without re-instantiating the selector. + if err := resolveWorkspaceFlag(f); err != nil { + return err + } + verifyPersistedWorkspace(f) + + f.Selector = selector.New(f.ApiClient, f.Log, f.Prompter, f.CurrentOwnerID) f.ParamFiller = fill.NewParamFiller(f.Selector) } @@ -145,6 +160,8 @@ func NewCmdRoot(f *cmdutil.Factory, version, commit, date string) (*cobra.Comman cmd.PersistentFlags().BoolVarP(&f.Interactive, config.KeyInteractive, "i", true, "use interactive mode") cmd.PersistentFlags().BoolVar(&f.AutoCheckUpdate, config.KeyAutoCheckUpdate, true, "automatically check update") cmd.PersistentFlags().BoolVar(&f.JSON, "json", false, "output in JSON format") + cmd.PersistentFlags().StringVar(&f.Workspace, "workspace", "", + "one-shot workspace override (team name or ID); to return to personal use 'zeabur workspace clear'") // Child commands cmd.AddCommand(deployCmd.NewCmdDeploy(f)) @@ -164,6 +181,7 @@ func NewCmdRoot(f *cmdutil.Factory, version, commit, date string) (*cobra.Comman cmd.AddCommand(emailCmd.NewCmdEmail(f)) cmd.AddCommand(fileCmd.NewCmdFile(f)) cmd.AddCommand(aihubCmd.NewCmdAIHub(f)) + cmd.AddCommand(workspaceCmd.NewCmdWorkspace(f)) // replace default help command with our custom one that supports --all cmd.SetHelpCommand(helpCmd.NewCmdHelp(cmd)) @@ -171,6 +189,49 @@ func NewCmdRoot(f *cmdutil.Factory, version, commit, date string) (*cobra.Comman return cmd, nil } +// resolveWorkspaceFlag turns the raw --workspace value into a team ObjectID +// and records it on the Factory. Empty flag is a no-op. The keyword +// "personal" is intentionally NOT recognized — `zeabur workspace clear` is +// the only way to address personal, and team names are unconstrained (a +// user-named "personal" team must be reachable). Backend-side RBAC validates +// the resolved ID on every call; resolution here is a UX layer. +func resolveWorkspaceFlag(f *cmdutil.Factory) error { + raw := strings.TrimSpace(f.Workspace) + if raw == "" { + return nil + } + team, err := cmdutil.ResolveWorkspaceArg(context.Background(), f.ApiClient, raw) + if err != nil { + return fmt.Errorf("--workspace: %w", err) + } + f.SetWorkspaceOverride(team.ID) + return nil +} + +// verifyPersistedWorkspace warns and falls back to personal when the +// persisted workspace is no longer a team the caller belongs to (team +// deleted, caller removed, etc.). Best-effort: any transport error leaves the +// workspace untouched so an offline blip doesn't silently switch users out. +func verifyPersistedWorkspace(f *cmdutil.Factory) { + ws := f.Config.GetContext().GetWorkspace() + if ws.IsPersonal() { + return + } + teams, err := f.ApiClient.ListTeams(context.Background()) + if err != nil { + f.Log.Debugf("workspace verify skipped: %v", err) + return + } + for _, t := range teams { + if t.ID == ws.ID { + return + } + } + f.Log.Warnf("Persisted workspace %q [%s] is no longer in your memberships; falling back to personal.", ws.Name, ws.ID) + f.Config.GetContext().ClearWorkspace() + f.Config.GetContext().ClearAll() +} + // normalizeIDFlag strips a known prefix from a prefixed MongoDB ObjectID flag value. // e.g. "service-662e24fca7d5abcdef123456" → "662e24fca7d5abcdef123456" func normalizeIDFlag(flag *pflag.Flag) error { diff --git a/internal/cmd/template/deploy/deploy.go b/internal/cmd/template/deploy/deploy.go index 7319be7..338533c 100644 --- a/internal/cmd/template/deploy/deploy.go +++ b/internal/cmd/template/deploy/deploy.go @@ -131,7 +131,7 @@ func runDeploy(f *cmdutil.Factory, opts *Options) error { } if opts.region != "" && opts.projectID == "" { - project, err := f.ApiClient.CreateProject(context.Background(), opts.region, nil) + project, err := f.ApiClient.CreateProject(context.Background(), f.CurrentOwnerID(), opts.region, nil) if err != nil { return fmt.Errorf("create project in region %s: %w", opts.region, err) } diff --git a/internal/cmd/workspace/clear/clear.go b/internal/cmd/workspace/clear/clear.go new file mode 100644 index 0000000..6f0751a --- /dev/null +++ b/internal/cmd/workspace/clear/clear.go @@ -0,0 +1,50 @@ +// Package clear implements `zeabur workspace clear`. Clear is the ONLY way +// to return to the personal workspace — `workspace switch personal` is +// intentionally interpreted as "find a team literally named personal" because +// team names are unconstrained. +package clear + +import ( + "fmt" + + "github.com/spf13/cobra" + + "github.com/zeabur/cli/internal/cmdutil" +) + +// NewCmdClear builds `zeabur workspace clear`. +func NewCmdClear(f *cmdutil.Factory) *cobra.Command { + return &cobra.Command{ + Use: "clear", + Short: "Switch back to the personal workspace", + Long: `Return to the personal workspace. + +This is the only way to switch to personal: `+"`workspace switch personal`"+` +always looks for a team literally named "personal". This also clears the +persisted project, environment, and service context because resource IDs do +not overlap between workspaces.`, + Args: cobra.NoArgs, + RunE: func(cmd *cobra.Command, args []string) error { + return run(f) + }, + } +} + +func run(f *cmdutil.Factory) error { + cctx := f.Config.GetContext() + prev := cctx.GetWorkspace() + prevProject := cctx.GetProject() + + cctx.ClearWorkspace() + cctx.ClearAll() + + if prev.IsPersonal() { + fmt.Println("Already on personal workspace.") + } else { + fmt.Printf("Switched to personal workspace (was: %s).\n", prev.Name) + } + if !prevProject.Empty() { + fmt.Printf("Project context cleared (was: %s).\n", prevProject.GetName()) + } + return nil +} diff --git a/internal/cmd/workspace/current/current.go b/internal/cmd/workspace/current/current.go new file mode 100644 index 0000000..373cf48 --- /dev/null +++ b/internal/cmd/workspace/current/current.go @@ -0,0 +1,54 @@ +// Package current implements `zeabur workspace current`. +package current + +import ( + "context" + "fmt" + + "github.com/spf13/cobra" + + "github.com/zeabur/cli/internal/cmdutil" +) + +// NewCmdCurrent builds `zeabur workspace current`. +func NewCmdCurrent(f *cmdutil.Factory) *cobra.Command { + return &cobra.Command{ + Use: "current", + Short: "Show the workspace the CLI is currently acting under", + Args: cobra.NoArgs, + RunE: func(cmd *cobra.Command, args []string) error { + return run(f) + }, + } +} + +func run(f *cmdutil.Factory) error { + ws := f.Config.GetContext().GetWorkspace() + if ws.IsPersonal() { + label := f.Config.GetUser() + if label == "" { + label = f.Config.GetUsername() + } + if label == "" { + label = "(you)" + } + fmt.Printf("personal (%s)\n", label) + return nil + } + + // For a team workspace also fetch the freshest role from the backend so + // `current` reports the live role rather than whatever was cached when + // the workspace was last switched in. + role := "" + teams, err := f.ApiClient.ListTeams(context.Background()) + if err == nil { + for _, t := range teams { + if t.ID == ws.ID && t.MyRole != nil { + role = " " + t.MyRole.Display() + break + } + } + } + fmt.Printf("%s [%s] team%s\n", ws.Name, ws.ID, role) + return nil +} diff --git a/internal/cmd/workspace/list/list.go b/internal/cmd/workspace/list/list.go new file mode 100644 index 0000000..bde5a43 --- /dev/null +++ b/internal/cmd/workspace/list/list.go @@ -0,0 +1,90 @@ +// Package list implements `zeabur workspace list`. +package list + +import ( + "context" + "fmt" + "os" + "text/tabwriter" + + "github.com/spf13/cobra" + + "github.com/zeabur/cli/internal/cmdutil" +) + +// NewCmdList builds `zeabur workspace list`. +func NewCmdList(f *cmdutil.Factory) *cobra.Command { + return &cobra.Command{ + Use: "list", + Short: "List the personal workspace and every team the caller belongs to", + Aliases: []string{"ls"}, + Args: cobra.NoArgs, + RunE: func(cmd *cobra.Command, args []string) error { + return run(f) + }, + } +} + +func run(f *cmdutil.Factory) error { + teams, err := f.ApiClient.ListTeams(context.Background()) + if err != nil { + return fmt.Errorf("list teams: %w", err) + } + + currentID := f.Config.GetContext().GetWorkspace().ID + + w := tabwriter.NewWriter(os.Stdout, 0, 0, 3, ' ', 0) + + // Personal always renders first. The 24-space placeholder keeps the + // columns aligned with the team rows that follow. + personalMarker := " " + if currentID == "" { + personalMarker = "*" + } + personalLabel := f.Config.GetUser() + if personalLabel == "" { + personalLabel = f.Config.GetUsername() + } + if personalLabel == "" { + personalLabel = "(you)" + } + fmt.Fprintf(w, "%s\tpersonal\t\t\t(%s)\n", personalMarker, personalLabel) + + for _, t := range teams { + marker := " " + if t.ID == currentID { + marker = "*" + } + role := "" + if t.MyRole != nil { + role = t.MyRole.Display() + } + fmt.Fprintf(w, "%s\t%s\t%s\tteam\t%s\n", marker, t.ID, t.Name, role) + } + + if err := w.Flush(); err != nil { + return fmt.Errorf("render table: %w", err) + } + + if currentID != "" { + // If the persisted workspace is no longer in the membership list + // (e.g. the team was deleted / the caller was removed), surface it + // so the user knows the next command may behave unexpectedly. We + // don't auto-clear here — that's the lazy-verify path in root. + seen := false + for _, t := range teams { + if t.ID == currentID { + seen = true + break + } + } + if !seen { + fmt.Fprintf(os.Stderr, + "\nwarning: the persisted workspace %s is not in your memberships any more — run `zeabur workspace clear` or switch to another workspace.\n", + currentID, + ) + } + } + + return nil +} diff --git a/internal/cmd/workspace/switch/switch.go b/internal/cmd/workspace/switch/switch.go new file mode 100644 index 0000000..f11b164 --- /dev/null +++ b/internal/cmd/workspace/switch/switch.go @@ -0,0 +1,68 @@ +// Package switchcmd implements `zeabur workspace switch `. The +// package is named switchcmd because `switch` is a Go keyword. +package switchcmd + +import ( + "context" + "fmt" + + "github.com/spf13/cobra" + + "github.com/zeabur/cli/internal/cmdutil" + "github.com/zeabur/cli/pkg/zcontext" +) + +// NewCmdSwitch builds `zeabur workspace switch`. +func NewCmdSwitch(f *cmdutil.Factory) *cobra.Command { + return &cobra.Command{ + Use: "switch ", + Short: "Switch to a team workspace", + Long: `Switch the CLI's workspace to a team. + +The argument may be the team's name or its full 24-character ObjectID. Team +names are not unique; when the name resolves to more than one team the +command exits with an error and prints the concrete invocation for each +candidate so you can pick by ID. + +To return to the personal workspace use ` + "`zeabur workspace clear`" + ` — +` + "`switch personal`" + ` always looks for a team literally named "personal". + +Switching clears the persisted project, environment, and service context, +because resource IDs do not overlap between workspaces.`, + Args: cobra.ExactArgs(1), + RunE: func(cmd *cobra.Command, args []string) error { + return run(f, args[0]) + }, + } +} + +func run(f *cmdutil.Factory, arg string) error { + team, err := cmdutil.ResolveWorkspaceArg(context.Background(), f.ApiClient, arg) + if err != nil { + return err + } + + cctx := f.Config.GetContext() + prevProject := cctx.GetProject() + + cctx.SetWorkspace(&zcontext.Workspace{ + ID: team.ID, + Name: team.Name, + Kind: zcontext.WorkspaceKindTeam, + }) + // Resource IDs don't overlap between workspaces, so any pinned + // project/environment/service context from the previous workspace is now + // stale. Clear it and tell the user, so the next interactive command + // re-prompts rather than silently 404-ing. + cctx.ClearAll() + + role := "" + if team.MyRole != nil { + role = ", " + team.MyRole.Display() + } + fmt.Printf("Switched to workspace %q [%s] (team%s).\n", team.Name, team.ID, role) + if !prevProject.Empty() { + fmt.Printf("Project context cleared (was: %s).\n", prevProject.GetName()) + } + return nil +} diff --git a/internal/cmd/workspace/workspace.go b/internal/cmd/workspace/workspace.go new file mode 100644 index 0000000..e4aafc8 --- /dev/null +++ b/internal/cmd/workspace/workspace.go @@ -0,0 +1,41 @@ +// Package workspace contains the `zeabur workspace` command group, which +// drives the personal-vs-team-workspace switcher the CLI uses for +// directory-level operations (project list / create, deploy with no linked +// project). Specific resources (services / deployments / variables) are +// addressed by ID and stay workspace-independent. +package workspace + +import ( + "github.com/spf13/cobra" + + "github.com/zeabur/cli/internal/cmdutil" + + workspaceClearCmd "github.com/zeabur/cli/internal/cmd/workspace/clear" + workspaceCurrentCmd "github.com/zeabur/cli/internal/cmd/workspace/current" + workspaceListCmd "github.com/zeabur/cli/internal/cmd/workspace/list" + workspaceSwitchCmd "github.com/zeabur/cli/internal/cmd/workspace/switch" +) + +// NewCmdWorkspace builds the `zeabur workspace` parent command. +func NewCmdWorkspace(f *cmdutil.Factory) *cobra.Command { + cmd := &cobra.Command{ + Use: "workspace", + Short: "Manage the personal / team workspace the CLI acts under", + Long: `Manage the workspace the CLI uses for project list / create / deploy. + +A workspace is either "personal" (the caller's own account) or a team. The +choice affects only directory-level operations: which projects are listed +and which owner a newly-created project is filed under. Operations on a +specific project, service, or deployment use that resource's own owner. + +Switching workspaces clears the persisted project / environment / service +context because resource IDs do not overlap between workspaces.`, + } + + cmd.AddCommand(workspaceListCmd.NewCmdList(f)) + cmd.AddCommand(workspaceCurrentCmd.NewCmdCurrent(f)) + cmd.AddCommand(workspaceSwitchCmd.NewCmdSwitch(f)) + cmd.AddCommand(workspaceClearCmd.NewCmdClear(f)) + + return cmd +} diff --git a/internal/cmdutil/factory.go b/internal/cmdutil/factory.go index 9315ec0..f3bea99 100644 --- a/internal/cmdutil/factory.go +++ b/internal/cmdutil/factory.go @@ -26,17 +26,50 @@ type ( Selector selector.Selector // interactive selector ParamFiller fill.ParamFiller // fill params PersistentFlags + + // workspaceOverrideID is the team ObjectID resolved from the + // --workspace flag during PersistentPreRunE. Empty when the flag is + // not set; CurrentOwnerID then falls back to the persisted workspace. + workspaceOverrideID string } // PersistentFlags are flags that are common to all commands PersistentFlags struct { - Debug bool // debug mode, default false - Interactive bool // interactive mode, default true - AutoRefreshToken bool // auto refresh token, default true, only when token is from browser(OAuth2) - AutoCheckUpdate bool // auto check update, default true - JSON bool // output in JSON format, default false + Debug bool // debug mode, default false + Interactive bool // interactive mode, default true + AutoRefreshToken bool // auto refresh token, default true, only when token is from browser(OAuth2) + AutoCheckUpdate bool // auto check update, default true + JSON bool // output in JSON format, default false + Workspace string // --workspace one-shot override } ) +// CurrentOwnerID returns the team ObjectID hex that directory-level commands +// (project list / create / deploy-no-link) should act under. Empty string == +// the caller's personal account. +// +// Resolution order: +// 1. --workspace flag (resolved to ID during PersistentPreRunE) +// 2. Persisted workspace in the config file +// +// Returning an empty string is the canonical "personal" signal that the +// project API uses to fall back to the un-owner-scoped GraphQL query. +func (f *Factory) CurrentOwnerID() string { + if f.workspaceOverrideID != "" { + return f.workspaceOverrideID + } + if f.Config == nil { + return "" + } + return f.Config.GetContext().GetWorkspace().ID +} + +// SetWorkspaceOverride records the resolved ID for a --workspace flag value. +// Called from PersistentPreRunE after the flag string has been disambiguated +// against the list of teams. +func (f *Factory) SetWorkspaceOverride(id string) { + f.workspaceOverrideID = id +} + // NewFactory returns a new cmd factory func NewFactory() *Factory { return &Factory{} diff --git a/internal/cmdutil/workspace.go b/internal/cmdutil/workspace.go new file mode 100644 index 0000000..9e67d84 --- /dev/null +++ b/internal/cmdutil/workspace.go @@ -0,0 +1,85 @@ +package cmdutil + +import ( + "context" + "encoding/hex" + "errors" + "fmt" + "strings" + + "github.com/zeabur/cli/pkg/api" + "github.com/zeabur/cli/pkg/model" +) + +// ResolveWorkspaceArg turns user-supplied "" into a workspace ID by +// asking the backend's `teams` query for the caller's memberships. The same +// resolution rules apply to both `zeabur workspace switch ` and the +// global `--workspace ` flag: +// +// - 24-char hex → treated as a team ObjectID directly. The caller is still +// a member only if that team appears in the `teams` reply, but for the +// flag path we trust the input: backend RBAC will reject if it isn't. +// - non-hex → matched by team name against the membership list. +// - exactly one match → return that team's ID. +// - zero matches → "no workspace named ..." error. +// - two or more matches → "ambiguous" error listing each candidate with the +// concrete `zeabur workspace switch ` invocation that disambiguates +// it. Team names are not unique; users must use the ID to pick. +// +// Returns the resolved Team so callers can both record the name in config +// (for `workspace switch`) and surface it in the deploy-to-team hint. +func ResolveWorkspaceArg(ctx context.Context, client api.Client, arg string) (*model.Team, error) { + arg = strings.TrimSpace(arg) + if arg == "" { + return nil, errors.New("workspace name or id is required") + } + + teams, err := client.ListTeams(ctx) + if err != nil { + return nil, fmt.Errorf("list teams: %w", err) + } + + if isObjectIDHex(arg) { + for i := range teams { + if teams[i].ID == arg { + return &teams[i], nil + } + } + return nil, fmt.Errorf("no team with id %q in your memberships", arg) + } + + var matches []model.Team + for i := range teams { + if teams[i].Name == arg { + matches = append(matches, teams[i]) + } + } + switch len(matches) { + case 0: + return nil, fmt.Errorf("no workspace named %q", arg) + case 1: + return &matches[0], nil + default: + var b strings.Builder + fmt.Fprintf(&b, "ambiguous: %d workspaces named %q, please switch by id:\n", len(matches), arg) + for _, m := range matches { + role := "" + if m.MyRole != nil { + role = " (" + m.MyRole.Display() + ")" + } + fmt.Fprintf(&b, " zeabur workspace switch %s%s\n", m.ID, role) + } + return nil, errors.New(strings.TrimRight(b.String(), "\n")) + } +} + +// isObjectIDHex matches the 24-char lowercase/uppercase hex shape that the +// backend uses for primitive.ObjectID. We don't go through +// primitive.ObjectIDFromHex to avoid pulling in the mongo driver from cli/. +func isObjectIDHex(s string) bool { + if len(s) != 24 { + return false + } + _, err := hex.DecodeString(s) + return err == nil +} diff --git a/internal/cmdutil/workspace_test.go b/internal/cmdutil/workspace_test.go new file mode 100644 index 0000000..c151e3a --- /dev/null +++ b/internal/cmdutil/workspace_test.go @@ -0,0 +1,118 @@ +package cmdutil + +import ( + "context" + "strings" + "testing" + + "github.com/zeabur/cli/pkg/api" + "github.com/zeabur/cli/pkg/model" +) + +// fakeListTeamsClient stubs api.Client.ListTeams. Other Client methods are +// inherited from the embedded nil interface and will panic if exercised — +// ResolveWorkspaceArg must only call ListTeams. +type fakeListTeamsClient struct { + api.Client + teams []model.Team +} + +func (c *fakeListTeamsClient) ListTeams(_ context.Context) ([]model.Team, error) { + return c.teams, nil +} + +func ptrRole(r model.TeamMemberRole) *model.TeamMemberRole { return &r } + +func TestResolveWorkspaceArg_EmptyArg(t *testing.T) { + _, err := ResolveWorkspaceArg(context.Background(), &fakeListTeamsClient{}, " ") + if err == nil || !strings.Contains(err.Error(), "required") { + t.Fatalf("want 'required' error, got %v", err) + } +} + +func TestResolveWorkspaceArg_ByID_Match(t *testing.T) { + id := "65aa1234567890abcdef1234" + c := &fakeListTeamsClient{teams: []model.Team{{ID: id, Name: "acme", MyRole: ptrRole(model.TeamMemberRoleAdministrator)}}} + team, err := ResolveWorkspaceArg(context.Background(), c, id) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if team.ID != id { + t.Fatalf("ID = %s, want %s", team.ID, id) + } +} + +func TestResolveWorkspaceArg_ByID_NotAMember(t *testing.T) { + c := &fakeListTeamsClient{teams: []model.Team{{ID: "65aa1234567890abcdef1234", Name: "acme"}}} + _, err := ResolveWorkspaceArg(context.Background(), c, "65bbffffffffffffffffffff") + if err == nil || !strings.Contains(err.Error(), "not a team") && !strings.Contains(err.Error(), "no team") { + t.Fatalf("want membership error, got %v", err) + } +} + +func TestResolveWorkspaceArg_ByName_Unique(t *testing.T) { + c := &fakeListTeamsClient{teams: []model.Team{ + {ID: "65aa1234567890abcdef1234", Name: "acme", MyRole: ptrRole(model.TeamMemberRoleEditor)}, + {ID: "65bb5678901234abcdef5678", Name: "beta", MyRole: ptrRole(model.TeamMemberRoleViewer)}, + }} + team, err := ResolveWorkspaceArg(context.Background(), c, "acme") + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if team.Name != "acme" { + t.Fatalf("Name = %q, want acme", team.Name) + } +} + +func TestResolveWorkspaceArg_ByName_NotFound(t *testing.T) { + c := &fakeListTeamsClient{teams: []model.Team{{ID: "65aa1234567890abcdef1234", Name: "acme"}}} + _, err := ResolveWorkspaceArg(context.Background(), c, "zeta") + if err == nil || !strings.Contains(err.Error(), `no workspace named`) { + t.Fatalf("want 'no workspace named' error, got %v", err) + } +} + +// TestResolveWorkspaceArg_ByName_Ambiguous covers the duplicate-team-name +// case Bruce called out explicitly: two teams sharing a name must not be +// resolvable by name alone, and the error must spell out the disambiguating +// commands so the user can pick by ID. +func TestResolveWorkspaceArg_ByName_Ambiguous(t *testing.T) { + id1 := "65aa1234567890abcdef1234" + id2 := "65bb5678901234abcdef5678" + c := &fakeListTeamsClient{teams: []model.Team{ + {ID: id1, Name: "acme", MyRole: ptrRole(model.TeamMemberRoleAdministrator)}, + {ID: id2, Name: "acme", MyRole: ptrRole(model.TeamMemberRoleViewer)}, + }} + _, err := ResolveWorkspaceArg(context.Background(), c, "acme") + if err == nil { + t.Fatal("want ambiguous error, got nil") + } + msg := err.Error() + for _, want := range []string{"ambiguous", "2 workspaces named", id1, id2, "Administrator", "Viewer", "zeabur workspace switch"} { + if !strings.Contains(msg, want) { + t.Errorf("error missing %q\nfull: %s", want, msg) + } + } +} + +// TestIsObjectIDHex sanity-checks the predicate that decides whether to take +// the ID path vs the name path in ResolveWorkspaceArg. +func TestIsObjectIDHex(t *testing.T) { + cases := []struct { + s string + want bool + }{ + {"65aa1234567890abcdef1234", true}, + {"65AA1234567890ABCDEF1234", true}, + {"65aa1234567890abcdef123", false}, // 23 chars + {"65aa1234567890abcdef12345", false}, // 25 chars + {"65aa1234567890abcdef123g", false}, // non-hex + {"", false}, + {"acme", false}, + } + for _, tc := range cases { + if got := isObjectIDHex(tc.s); got != tc.want { + t.Errorf("isObjectIDHex(%q) = %v, want %v", tc.s, got, tc.want) + } + } +} diff --git a/pkg/api/interface.go b/pkg/api/interface.go index 38734f1..2d8c8d7 100644 --- a/pkg/api/interface.go +++ b/pkg/api/interface.go @@ -10,6 +10,7 @@ import ( // Client is the interface of the Zeabur API client. type Client interface { UserAPI + TeamAPI ProjectAPI ServiceAPI EnvironmentAPI @@ -31,11 +32,22 @@ type ( GetUserInfo(ctx context.Context) (*model.User, error) } + // TeamAPI groups team / workspace queries. + TeamAPI interface { + // ListTeams returns every team the caller belongs to, with the + // caller's role per team via Team.myRole. + ListTeams(ctx context.Context) ([]model.Team, error) + } + ProjectAPI interface { - ListProjects(ctx context.Context, skip, limit int) (*model.ProjectConnection, error) - ListAllProjects(ctx context.Context) (model.Projects, error) + // ListProjects accepts an optional ownerID (empty = caller's personal + // projects). For a team workspace pass the team's ObjectID hex. + ListProjects(ctx context.Context, ownerID string, skip, limit int) (*model.ProjectConnection, error) + ListAllProjects(ctx context.Context, ownerID string) (model.Projects, error) GetProject(ctx context.Context, id string, ownerName string, name string) (*model.Project, error) - CreateProject(ctx context.Context, region string, name *string) (*model.Project, error) + // CreateProject creates a project under the given owner. Empty + // ownerID means the caller's personal projects. + CreateProject(ctx context.Context, ownerID, region string, name *string) (*model.Project, error) DeleteProject(ctx context.Context, id string) error ExportProject(ctx context.Context, id string, environmentID string) (*model.ExportedTemplate, error) diff --git a/pkg/api/project.go b/pkg/api/project.go index 868d595..259e64e 100644 --- a/pkg/api/project.go +++ b/pkg/api/project.go @@ -7,35 +7,50 @@ import ( "github.com/zeabur/cli/pkg/model" ) -// ListProjects returns projects owned by the current user. -// Note: the backend hasn't implemented pagination yet, currently we return all projects at once. -func (c *client) ListProjects(ctx context.Context, skip, limit int) (*model.ProjectConnection, error) { +// ListProjects returns projects owned by the given owner. An empty ownerID +// means the caller's personal projects (the pre-workspace default — preserved +// for callers that don't yet pass an owner). +// +// Note: the backend hasn't implemented pagination yet, currently we return all +// projects at once. +func (c *client) ListProjects(ctx context.Context, ownerID string, skip, limit int) (*model.ProjectConnection, error) { skip, limit = normalizePagination(skip, limit) - var query struct { - Projects model.ProjectConnection `graphql:"projects(skip: $skip, limit: $limit)"` + if ownerID == "" { + var query struct { + Projects model.ProjectConnection `graphql:"projects(skip: $skip, limit: $limit)"` + } + if err := c.Query(ctx, &query, V{ + "skip": skip, + "limit": limit, + }); err != nil { + return nil, err + } + return &query.Projects, nil } - err := c.Query(ctx, &query, V{ - "skip": skip, - "limit": limit, - }) - if err != nil { + var query struct { + Projects model.ProjectConnection `graphql:"projects(ownerID: $ownerID, skip: $skip, limit: $limit)"` + } + if err := c.Query(ctx, &query, V{ + "ownerID": ObjectID(ownerID), + "skip": skip, + "limit": limit, + }); err != nil { return nil, err } - return &query.Projects, nil } -// ListAllProjects returns all projects owned by the current user. -func (c *client) ListAllProjects(ctx context.Context) (model.Projects, error) { +// ListAllProjects walks every page of ListProjects for the given owner. +func (c *client) ListAllProjects(ctx context.Context, ownerID string) (model.Projects, error) { skip := 0 next := true var projects []*model.Project for next { - projectCon, err := c.ListProjects(context.Background(), skip, 100) + projectCon, err := c.ListProjects(context.Background(), ownerID, skip, 100) if err != nil { return nil, err } @@ -110,20 +125,33 @@ func (c *client) getProjectByOwnerUsernameAndProject(ctx context.Context, return &query.Project, nil } -// Create a project with the region and optional name. -func (c *client) CreateProject(ctx context.Context, region string, name *string) (*model.Project, error) { - var mutation struct { - CreateProject model.Project `graphql:"createProject(region: $region, name: $name)"` +// CreateProject creates a project under the given owner. An empty ownerID +// creates the project under the caller's personal account (the pre-workspace +// default). +func (c *client) CreateProject(ctx context.Context, ownerID, region string, name *string) (*model.Project, error) { + if ownerID == "" { + var mutation struct { + CreateProject model.Project `graphql:"createProject(region: $region, name: $name)"` + } + if err := c.Mutate(ctx, &mutation, V{ + "region": region, + "name": name, + }); err != nil { + return nil, err + } + return &mutation.CreateProject, nil } - err := c.Mutate(ctx, &mutation, V{ - "region": region, - "name": name, - }) - if err != nil { + var mutation struct { + CreateProject model.Project `graphql:"createProject(ownerID: $ownerID, region: $region, name: $name)"` + } + if err := c.Mutate(ctx, &mutation, V{ + "ownerID": ObjectID(ownerID), + "region": region, + "name": name, + }); err != nil { return nil, err } - return &mutation.CreateProject, nil } diff --git a/pkg/api/team.go b/pkg/api/team.go new file mode 100644 index 0000000..05c2a60 --- /dev/null +++ b/pkg/api/team.go @@ -0,0 +1,21 @@ +package api + +import ( + "context" + + "github.com/zeabur/cli/pkg/model" +) + +// ListTeams returns every team the current caller belongs to, with the +// caller's role in each team. Drives `zeabur workspace list` and the lazy +// verify run at CLI startup. +func (c *client) ListTeams(ctx context.Context) ([]model.Team, error) { + var query struct { + Teams []model.Team `graphql:"teams"` + } + + if err := c.Query(ctx, &query, nil); err != nil { + return nil, err + } + return query.Teams, nil +} diff --git a/pkg/model/team.go b/pkg/model/team.go new file mode 100644 index 0000000..e74c676 --- /dev/null +++ b/pkg/model/team.go @@ -0,0 +1,36 @@ +package model + +// TeamMemberRole mirrors the backend GraphQL TeamMemberRole enum. Values are +// the raw enum names so they print identically to the backend / dashboard. +type TeamMemberRole string + +const ( + TeamMemberRoleAdministrator TeamMemberRole = "ADMINISTRATOR" + TeamMemberRoleEditor TeamMemberRole = "EDITOR" + TeamMemberRoleViewer TeamMemberRole = "VIEWER" +) + +// Display returns the role spelled the way the dashboard shows it +// ("Administrator" / "Editor" / "Viewer") rather than the SCREAMING form. +func (r TeamMemberRole) Display() string { + switch r { + case TeamMemberRoleAdministrator: + return "Administrator" + case TeamMemberRoleEditor: + return "Editor" + case TeamMemberRoleViewer: + return "Viewer" + default: + return string(r) + } +} + +// Team is the slim shape returned by the `teams` query — enough to drive +// `zeabur workspace list / switch`. `MyRole` is the caller's own role and +// comes from backend PLA-1589 (Team.myRole field). It's a pointer because the +// backend marks it nullable; for `teams` results it is always set. +type Team struct { + ID string `graphql:"_id"` + Name string `graphql:"name"` + MyRole *TeamMemberRole `graphql:"myRole"` +} diff --git a/pkg/selector/selector.go b/pkg/selector/selector.go index 1d43183..4224227 100644 --- a/pkg/selector/selector.go +++ b/pkg/selector/selector.go @@ -34,16 +34,26 @@ type ( ) type selector struct { - client api.Client - log *zap.SugaredLogger - prompter prompt.Prompter + client api.Client + log *zap.SugaredLogger + prompter prompt.Prompter + currentOwnerID func() string } -func New(client api.Client, log *zap.SugaredLogger, prompter prompt.Prompter) Selector { +// New returns a Selector. ownerIDFn resolves the current workspace owner ID +// at call time — it must be a closure (not a captured string) so a mid-run +// workspace switch and the --workspace flag override take effect on the very +// next selector call. Pass nil or a closure that returns "" for the +// personal workspace. +func New(client api.Client, log *zap.SugaredLogger, prompter prompt.Prompter, ownerIDFn func() string) Selector { + if ownerIDFn == nil { + ownerIDFn = func() string { return "" } + } return &selector{ - client: client, - log: log, - prompter: prompter, + client: client, + log: log, + prompter: prompter, + currentOwnerID: ownerIDFn, } } @@ -69,7 +79,7 @@ func (s *selector) SelectProject(opts ...SelectProjectOptionsApplyer) (zcontext. applyer(&options) } - projects, err := s.client.ListAllProjects(context.Background()) + projects, err := s.client.ListAllProjects(context.Background(), s.currentOwnerID()) if err != nil { return nil, nil, fmt.Errorf("list projects failed: %w", err) } @@ -118,7 +128,7 @@ func (s *selector) SelectProject(opts ...SelectProjectOptionsApplyer) (zcontext. projectRegion := availableRegions[projectRegionIndex].GetID() - project, err := s.client.CreateProject(context.Background(), projectRegion, nil) + project, err := s.client.CreateProject(context.Background(), s.currentOwnerID(), projectRegion, nil) if err != nil { return nil, nil, fmt.Errorf("create project failed: %w", err) } diff --git a/pkg/zcontext/context.go b/pkg/zcontext/context.go index 83fff74..79ecdfc 100644 --- a/pkg/zcontext/context.go +++ b/pkg/zcontext/context.go @@ -6,6 +6,14 @@ import "github.com/spf13/viper" const ( KeyContext = "context" + // Workspace lives at the top level (sibling to context), mirroring its + // role as an outer scope: a workspace switch invalidates the inner + // project / environment / service context. + KeyWorkspace = "workspace" + KeyWorkspaceID = KeyWorkspace + ".id" + KeyWorkspaceName = KeyWorkspace + ".name" + KeyWorkspaceKind = KeyWorkspace + ".kind" + KeyProject = KeyContext + ".project" KeyProjectID = KeyProject + ".id" KeyProjectName = KeyProject + ".name" @@ -28,6 +36,30 @@ func NewViperContext(viper *viper.Viper) Context { return &viperContext{viper: viper} } +func (c *viperContext) GetWorkspace() *Workspace { + return &Workspace{ + ID: c.viper.GetString(KeyWorkspaceID), + Name: c.viper.GetString(KeyWorkspaceName), + Kind: c.viper.GetString(KeyWorkspaceKind), + } +} + +func (c *viperContext) SetWorkspace(w *Workspace) { + if w == nil { + c.ClearWorkspace() + return + } + c.viper.Set(KeyWorkspaceID, w.ID) + c.viper.Set(KeyWorkspaceName, w.Name) + c.viper.Set(KeyWorkspaceKind, w.Kind) +} + +func (c *viperContext) ClearWorkspace() { + c.viper.Set(KeyWorkspaceID, "") + c.viper.Set(KeyWorkspaceName, "") + c.viper.Set(KeyWorkspaceKind, "") +} + func (c *viperContext) GetProject() BasicInfo { return &basicInfo{ id: c.viper.GetString(KeyProjectID), diff --git a/pkg/zcontext/interface.go b/pkg/zcontext/interface.go index d90a606..cb14447 100644 --- a/pkg/zcontext/interface.go +++ b/pkg/zcontext/interface.go @@ -1,7 +1,18 @@ package zcontext -// Context represents the current context of the CLI, including the current project, environment, service, etc. +// Context represents the current context of the CLI, including the current +// workspace (personal or team), and the pinned project / environment / service +// inside that workspace. type Context interface { + // GetWorkspace returns the persisted workspace. Always non-nil; check + // IsPersonal() / IsTeam() to branch. Personal is the zero value. + GetWorkspace() *Workspace + // SetWorkspace persists the workspace; passing nil is equivalent to + // ClearWorkspace(). + SetWorkspace(workspace *Workspace) + // ClearWorkspace returns to personal — the default zero state. + ClearWorkspace() + GetProject() BasicInfo SetProject(project BasicInfo) ClearProject() @@ -14,6 +25,10 @@ type Context interface { SetService(service BasicInfo) ClearService() + // ClearAll clears the inner project / environment / service context but + // leaves the workspace intact. Use ClearWorkspace() to go back to personal, + // which on its own does not touch the inner context — switching commands + // orchestrate that explicitly so they can report what was cleared. ClearAll() } diff --git a/pkg/zcontext/workspace.go b/pkg/zcontext/workspace.go new file mode 100644 index 0000000..9438cd5 --- /dev/null +++ b/pkg/zcontext/workspace.go @@ -0,0 +1,30 @@ +package zcontext + +// WorkspaceKindTeam marks a workspace as a Zeabur team. The empty string is +// "personal" — the user's own resources, the default when the CLI has never +// been switched to a team. +const WorkspaceKindTeam = "team" + +// Workspace identifies which owner the CLI is currently acting against for +// directory-level commands (project list, project create, deploy with no +// linked project). Other commands address resources by their own ID and stay +// workspace-independent. +type Workspace struct { + // ID is the team's MongoDB ObjectID hex string. Empty for personal. + ID string + // Name is the team's display name. Empty for personal. + Name string + // Kind is "team" for team workspaces, empty for personal. + Kind string +} + +// IsPersonal reports whether the workspace addresses the caller's own +// resources (no team scope). The zero value is personal. +func (w *Workspace) IsPersonal() bool { + return w == nil || w.ID == "" +} + +// IsTeam reports whether the workspace addresses a team. +func (w *Workspace) IsTeam() bool { + return w != nil && w.Kind == WorkspaceKindTeam && w.ID != "" +} From c515cee9ccc5e757c811dcb2b7b8365a10a66b89 Mon Sep 17 00:00:00 2001 From: Bruce Du Date: Wed, 20 May 2026 13:24:46 +0800 Subject: [PATCH 02/10] fix: address PLA-1590 review (workspace caching, deploy hint, etc.) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Panel review: - F1 (deploy hint vs. flag override): The "Creating new project in team workspace X" banner read from the persisted workspace, but the actual create call used CurrentOwnerID, which honors the --workspace flag override. With `--workspace team-b` overriding a persisted team-a the banner named the wrong team; overriding personal printed nothing. Snapshot a single CurrentWorkspace() up front and use it for both the banner and the ownerID — they now resolve from the same source. - F2 (ListTeams fanout): A single CLI invocation could hit `teams` up to three times (flag resolution, lazy verify, the command itself). Memoize the reply on Factory.ListTeams; resolveWorkspaceFlag, verifyPersistedWorkspace, and the workspace list / current / switch commands now all share one fetch with sticky-error caching. - N3 (current.go silent error): `workspace current` swallowed ListTeams errors and just omitted the role. Log at debug so it isn't completely silent without making the command fail. CodeRabbit: - workspace.go: factor invocationName() out of the ambiguous-name error so users invoking via `npx zeabur` (or any wrapper) get a copy-pasteable command in the error, not a hardcoded "zeabur". Normalize hex comparison to lower-case so an ID pasted in uppercase still resolves (isObjectIDHex already accepts both). - workspace.go (comment): Rewrite the ID-vs-name docstring to match behavior — both paths enforce membership; the hex path doesn't "trust" raw input as the old comment implied. Backend RBAC is still the authoritative gate. - deploy.go: spinner left running when ListAllProjects errors — s.Stop() happens before the err check so it runs on every path. - pkg/api/project.go: ListAllProjects' inner loop passed context.Background() to each page request, dropping the caller's cancellation. Propagate ctx. - workspace_test.go: convert to the `cmdutil_test` external package so the tests only exercise exported APIs. The previous direct test of the unexported isObjectIDHex is replaced by TestResolveWorkspaceArg_NonHex_NotMistakenForID (24-char non-hex must fall into the name branch) and TestResolveWorkspaceArg_ ByID_CaseInsensitive (uppercase hex must still resolve), which cover the same predicate via the public surface. - README: clarify that a 24-char hex value is always interpreted as an ID, not "only when the name is not unique". Names go to the name branch regardless; duplicates trip the ambiguous-name error. Factory: SetWorkspaceOverride now takes `*zcontext.Workspace` instead of a bare ID so CurrentWorkspace() can return the resolved name to display sites (the deploy hint). Added CurrentWorkspace() helper alongside CurrentOwnerID. Co-Authored-By: Claude Opus 4.7 (1M context) --- README.md | 6 +- internal/cmd/deploy/deploy.go | 10 ++- internal/cmd/root/root.go | 35 ++++++--- internal/cmd/workspace/current/current.go | 10 ++- internal/cmd/workspace/list/list.go | 2 +- internal/cmd/workspace/switch/switch.go | 6 +- internal/cmdutil/factory.go | 72 ++++++++++++++---- internal/cmdutil/workspace.go | 63 ++++++++++------ internal/cmdutil/workspace_test.go | 91 +++++++++++------------ pkg/api/project.go | 4 +- 10 files changed, 192 insertions(+), 107 deletions(-) diff --git a/README.md b/README.md index d9d8e89..b12016f 100644 --- a/README.md +++ b/README.md @@ -125,7 +125,11 @@ By default, the CLI acts under the personal workspace — the account that logge # show your personal workspace + all teams you belong to, with your role per team npx zeabur workspace list -# switch to a team by name (or by 24-char team ID if the name is not unique) +# switch to a team — pass either the team name OR its 24-char ObjectID. +# A 24-char hex value is always interpreted as an ID; anything else is +# looked up by name. If multiple teams share a name the CLI errors out and +# prints the per-candidate `workspace switch ` invocation so you can +# pick by ID (team names are unconstrained, so duplicates are possible). npx zeabur workspace switch acme # show the workspace the CLI is currently using diff --git a/internal/cmd/deploy/deploy.go b/internal/cmd/deploy/deploy.go index 370c206..d1f54ab 100644 --- a/internal/cmd/deploy/deploy.go +++ b/internal/cmd/deploy/deploy.go @@ -195,12 +195,16 @@ func selectInteractively(f *cmdutil.Factory, opts *Options) (*model.Service, *mo spinner.WithSuffix(" Fetching projects ..."), ) s.Start() - ownerID := f.CurrentOwnerID() + // Snapshot the active workspace so the hint below names the same team + // the create call actually files the project under. Reading the + // persisted workspace here would race the --workspace flag override. + ws := f.CurrentWorkspace() + ownerID := ws.ID projects, err := f.ApiClient.ListAllProjects(context.Background(), ownerID) + s.Stop() if err != nil { return nil, nil, err } - s.Stop() if len(projects) == 0 { confirm, err := f.Prompter.Confirm("No projects found. Would you like to create one now?", true) @@ -211,7 +215,7 @@ func selectInteractively(f *cmdutil.Factory, opts *Options) (*model.Service, *mo // When the active workspace is a team, make it visible that the // new project will land under that team — not the personal // account — so the user isn't surprised by where it shows up. - if ws := f.Config.GetContext().GetWorkspace(); ws.IsTeam() { + if ws.IsTeam() { f.Log.Infof("→ Creating new project in team workspace %q", ws.Name) } project, err := f.ApiClient.CreateProject(context.Background(), ownerID, "default", nil) diff --git a/internal/cmd/root/root.go b/internal/cmd/root/root.go index 569aa19..217541f 100644 --- a/internal/cmd/root/root.go +++ b/internal/cmd/root/root.go @@ -36,6 +36,7 @@ import ( "github.com/zeabur/cli/pkg/fill" "github.com/zeabur/cli/pkg/log" "github.com/zeabur/cli/pkg/selector" + "github.com/zeabur/cli/pkg/zcontext" ) // NewCmdRoot creates the root command @@ -189,35 +190,47 @@ func NewCmdRoot(f *cmdutil.Factory, version, commit, date string) (*cobra.Comman return cmd, nil } -// resolveWorkspaceFlag turns the raw --workspace value into a team ObjectID -// and records it on the Factory. Empty flag is a no-op. The keyword -// "personal" is intentionally NOT recognized — `zeabur workspace clear` is -// the only way to address personal, and team names are unconstrained (a -// user-named "personal" team must be reachable). Backend-side RBAC validates -// the resolved ID on every call; resolution here is a UX layer. +// resolveWorkspaceFlag turns the raw --workspace value into a team and +// records it on the Factory. Empty flag is a no-op. The keyword "personal" +// is intentionally NOT recognized — `zeabur workspace clear` is the only +// way to address personal, and team names are unconstrained (a user-named +// "personal" team must be reachable). Backend-side RBAC validates the +// resolved ID on every call; resolution here is a UX layer. +// +// Uses f.ListTeams (per-process cache) so flag resolution, the lazy verify, +// and downstream commands all share a single backend round-trip. func resolveWorkspaceFlag(f *cmdutil.Factory) error { raw := strings.TrimSpace(f.Workspace) if raw == "" { return nil } - team, err := cmdutil.ResolveWorkspaceArg(context.Background(), f.ApiClient, raw) + teams, err := f.ListTeams(context.Background()) + if err != nil { + return fmt.Errorf("--workspace: list teams: %w", err) + } + team, err := cmdutil.ResolveWorkspaceArg(teams, raw) if err != nil { return fmt.Errorf("--workspace: %w", err) } - f.SetWorkspaceOverride(team.ID) + f.SetWorkspaceOverride(&zcontext.Workspace{ + ID: team.ID, + Name: team.Name, + Kind: zcontext.WorkspaceKindTeam, + }) return nil } // verifyPersistedWorkspace warns and falls back to personal when the // persisted workspace is no longer a team the caller belongs to (team -// deleted, caller removed, etc.). Best-effort: any transport error leaves the -// workspace untouched so an offline blip doesn't silently switch users out. +// deleted, caller removed, etc.). Best-effort: any transport error leaves +// the workspace untouched so an offline blip doesn't silently switch users +// out. Uses the same memoized ListTeams as resolveWorkspaceFlag. func verifyPersistedWorkspace(f *cmdutil.Factory) { ws := f.Config.GetContext().GetWorkspace() if ws.IsPersonal() { return } - teams, err := f.ApiClient.ListTeams(context.Background()) + teams, err := f.ListTeams(context.Background()) if err != nil { f.Log.Debugf("workspace verify skipped: %v", err) return diff --git a/internal/cmd/workspace/current/current.go b/internal/cmd/workspace/current/current.go index 373cf48..0879372 100644 --- a/internal/cmd/workspace/current/current.go +++ b/internal/cmd/workspace/current/current.go @@ -38,10 +38,14 @@ func run(f *cmdutil.Factory) error { // For a team workspace also fetch the freshest role from the backend so // `current` reports the live role rather than whatever was cached when - // the workspace was last switched in. + // the workspace was last switched in. Don't fail the command on + // network error — the persisted name/id is still useful — but log it + // at debug so it isn't completely silent (PLA-1590 review N3). role := "" - teams, err := f.ApiClient.ListTeams(context.Background()) - if err == nil { + teams, err := f.ListTeams(context.Background()) + if err != nil { + f.Log.Debugf("workspace current: list teams failed, omitting role: %v", err) + } else { for _, t := range teams { if t.ID == ws.ID && t.MyRole != nil { role = " " + t.MyRole.Display() diff --git a/internal/cmd/workspace/list/list.go b/internal/cmd/workspace/list/list.go index bde5a43..c639abb 100644 --- a/internal/cmd/workspace/list/list.go +++ b/internal/cmd/workspace/list/list.go @@ -26,7 +26,7 @@ func NewCmdList(f *cmdutil.Factory) *cobra.Command { } func run(f *cmdutil.Factory) error { - teams, err := f.ApiClient.ListTeams(context.Background()) + teams, err := f.ListTeams(context.Background()) if err != nil { return fmt.Errorf("list teams: %w", err) } diff --git a/internal/cmd/workspace/switch/switch.go b/internal/cmd/workspace/switch/switch.go index f11b164..1345172 100644 --- a/internal/cmd/workspace/switch/switch.go +++ b/internal/cmd/workspace/switch/switch.go @@ -37,7 +37,11 @@ because resource IDs do not overlap between workspaces.`, } func run(f *cmdutil.Factory, arg string) error { - team, err := cmdutil.ResolveWorkspaceArg(context.Background(), f.ApiClient, arg) + teams, err := f.ListTeams(context.Background()) + if err != nil { + return fmt.Errorf("list teams: %w", err) + } + team, err := cmdutil.ResolveWorkspaceArg(teams, arg) if err != nil { return err } diff --git a/internal/cmdutil/factory.go b/internal/cmdutil/factory.go index f3bea99..457f3de 100644 --- a/internal/cmdutil/factory.go +++ b/internal/cmdutil/factory.go @@ -1,6 +1,8 @@ package cmdutil import ( + "context" + "github.com/zeabur/cli/pkg/fill" "github.com/zeabur/cli/pkg/printer" "github.com/zeabur/cli/pkg/selector" @@ -9,7 +11,9 @@ import ( "github.com/zeabur/cli/pkg/api" "github.com/zeabur/cli/pkg/auth" "github.com/zeabur/cli/pkg/config" + "github.com/zeabur/cli/pkg/model" "github.com/zeabur/cli/pkg/prompt" + "github.com/zeabur/cli/pkg/zcontext" ) type ( @@ -27,10 +31,22 @@ type ( ParamFiller fill.ParamFiller // fill params PersistentFlags - // workspaceOverrideID is the team ObjectID resolved from the - // --workspace flag during PersistentPreRunE. Empty when the flag is - // not set; CurrentOwnerID then falls back to the persisted workspace. - workspaceOverrideID string + // workspaceOverride is the team resolved from the --workspace flag + // during PersistentPreRunE. Nil when the flag is not set; + // CurrentOwnerID / CurrentWorkspace then fall back to the persisted + // workspace. Stored as the full Workspace (not just an ID) so + // downstream code that wants to show a name — e.g. the + // "creating new project in team workspace X" hint in deploy — gets + // the same effective workspace as CurrentOwnerID. + workspaceOverride *zcontext.Workspace + + // teamsCache memoizes the per-process ListTeams reply. A single CLI + // invocation can otherwise hit `teams` up to three times (flag + // resolution, persisted-workspace verify, the command itself); see + // PLA-1590 review feedback. + teamsCache []model.Team + teamsCacheErr error + teamsCacheHit bool } // PersistentFlags are flags that are common to all commands PersistentFlags struct { @@ -48,26 +64,54 @@ type ( // the caller's personal account. // // Resolution order: -// 1. --workspace flag (resolved to ID during PersistentPreRunE) +// 1. --workspace flag (resolved to a Workspace during PersistentPreRunE) // 2. Persisted workspace in the config file // // Returning an empty string is the canonical "personal" signal that the // project API uses to fall back to the un-owner-scoped GraphQL query. func (f *Factory) CurrentOwnerID() string { - if f.workspaceOverrideID != "" { - return f.workspaceOverrideID + return f.CurrentWorkspace().ID +} + +// CurrentWorkspace returns the effective workspace under the same resolution +// rules as CurrentOwnerID, including the name and kind. Callers that want to +// display the active workspace (the "creating new project in team workspace +// X" hint in deploy) should read this rather than the persisted workspace, +// so a --workspace override shows up correctly. +func (f *Factory) CurrentWorkspace() *zcontext.Workspace { + if f.workspaceOverride != nil { + return f.workspaceOverride } if f.Config == nil { - return "" + return &zcontext.Workspace{} } - return f.Config.GetContext().GetWorkspace().ID + return f.Config.GetContext().GetWorkspace() } -// SetWorkspaceOverride records the resolved ID for a --workspace flag value. -// Called from PersistentPreRunE after the flag string has been disambiguated -// against the list of teams. -func (f *Factory) SetWorkspaceOverride(id string) { - f.workspaceOverrideID = id +// SetWorkspaceOverride records the resolved workspace for a --workspace flag +// value. Called from PersistentPreRunE after the flag string has been +// disambiguated against the list of teams. Passing nil clears any prior +// override. +func (f *Factory) SetWorkspaceOverride(ws *zcontext.Workspace) { + f.workspaceOverride = ws +} + +// ListTeams returns the caller's teams via api.Client.ListTeams, memoized for +// the lifetime of this Factory. The same Factory is shared across every +// PersistentPreRunE / Run / PersistentPostRunE callback within a single CLI +// invocation, so one fetch covers --workspace flag resolution, the lazy +// startup verify, and downstream commands that surface roles. +// +// Errors are sticky: a failed fetch is cached and returned on every +// subsequent call, so callers don't accidentally retry against a broken +// backend within the same process. +func (f *Factory) ListTeams(ctx context.Context) ([]model.Team, error) { + if f.teamsCacheHit { + return f.teamsCache, f.teamsCacheErr + } + f.teamsCache, f.teamsCacheErr = f.ApiClient.ListTeams(ctx) + f.teamsCacheHit = true + return f.teamsCache, f.teamsCacheErr } // NewFactory returns a new cmd factory diff --git a/internal/cmdutil/workspace.go b/internal/cmdutil/workspace.go index 9e67d84..4ef48b7 100644 --- a/internal/cmdutil/workspace.go +++ b/internal/cmdutil/workspace.go @@ -1,47 +1,46 @@ package cmdutil import ( - "context" "encoding/hex" "errors" "fmt" + "os" + "path/filepath" "strings" - "github.com/zeabur/cli/pkg/api" "github.com/zeabur/cli/pkg/model" ) -// ResolveWorkspaceArg turns user-supplied "" into a workspace ID by -// asking the backend's `teams` query for the caller's memberships. The same -// resolution rules apply to both `zeabur workspace switch ` and the -// global `--workspace ` flag: +// ResolveWorkspaceArg turns user-supplied "" into a concrete team +// from the caller's memberships. The same resolution rules cover both +// `zeabur workspace switch ` and the global `--workspace ` flag: // -// - 24-char hex → treated as a team ObjectID directly. The caller is still -// a member only if that team appears in the `teams` reply, but for the -// flag path we trust the input: backend RBAC will reject if it isn't. +// - 24-char hex (case-insensitive) → matched against team IDs in the +// memberships slice. **Membership is enforced** — an ID not in +// `teams` is rejected here, before any further backend call. Backend +// RBAC remains the source of truth for every subsequent operation, so +// this is a UX shortcut rather than a security gate. // - non-hex → matched by team name against the membership list. -// - exactly one match → return that team's ID. +// - exactly one match → return that team. // - zero matches → "no workspace named ..." error. -// - two or more matches → "ambiguous" error listing each candidate with the -// concrete `zeabur workspace switch ` invocation that disambiguates -// it. Team names are not unique; users must use the ID to pick. +// - two or more matches → "ambiguous" error listing each candidate with +// the concrete ` workspace switch ` invocation that +// disambiguates it. Team names are unconstrained; users must use the +// ID to pick. // -// Returns the resolved Team so callers can both record the name in config -// (for `workspace switch`) and surface it in the deploy-to-team hint. -func ResolveWorkspaceArg(ctx context.Context, client api.Client, arg string) (*model.Team, error) { +// The caller passes the membership list in (typically Factory.ListTeams) so +// the per-process cache is shared with the lazy startup verify and any +// downstream commands that surface roles. +func ResolveWorkspaceArg(teams []model.Team, arg string) (*model.Team, error) { arg = strings.TrimSpace(arg) if arg == "" { return nil, errors.New("workspace name or id is required") } - teams, err := client.ListTeams(ctx) - if err != nil { - return nil, fmt.Errorf("list teams: %w", err) - } - if isObjectIDHex(arg) { + target := strings.ToLower(arg) for i := range teams { - if teams[i].ID == arg { + if strings.ToLower(teams[i].ID) == target { return &teams[i], nil } } @@ -67,13 +66,29 @@ func ResolveWorkspaceArg(ctx context.Context, client api.Client, arg string) (*m if m.MyRole != nil { role = " (" + m.MyRole.Display() + ")" } - fmt.Fprintf(&b, " zeabur workspace switch %s%s\n", m.ID, role) + fmt.Fprintf(&b, " %s workspace switch %s%s\n", invocationName(), m.ID, role) } return nil, errors.New(strings.TrimRight(b.String(), "\n")) } } -// isObjectIDHex matches the 24-char lowercase/uppercase hex shape that the +// invocationName returns the basename of the running binary (e.g. "zeabur" +// for a native install or "zeabur" / "npx zeabur" — we can't recover the npx +// wrapper from inside Go, so we only ever return the basename of os.Args[0]). +// Falls back to "zeabur" if os.Args is empty (impossible in normal use, but +// safe in tests). +func invocationName() string { + if len(os.Args) == 0 || os.Args[0] == "" { + return "zeabur" + } + name := filepath.Base(os.Args[0]) + if name == "" || name == "." || name == "/" { + return "zeabur" + } + return name +} + +// isObjectIDHex matches the 24-char hex shape (case-insensitive) that the // backend uses for primitive.ObjectID. We don't go through // primitive.ObjectIDFromHex to avoid pulling in the mongo driver from cli/. func isObjectIDHex(s string) bool { diff --git a/internal/cmdutil/workspace_test.go b/internal/cmdutil/workspace_test.go index c151e3a..7b8ae47 100644 --- a/internal/cmdutil/workspace_test.go +++ b/internal/cmdutil/workspace_test.go @@ -1,30 +1,17 @@ -package cmdutil +package cmdutil_test import ( - "context" "strings" "testing" - "github.com/zeabur/cli/pkg/api" + "github.com/zeabur/cli/internal/cmdutil" "github.com/zeabur/cli/pkg/model" ) -// fakeListTeamsClient stubs api.Client.ListTeams. Other Client methods are -// inherited from the embedded nil interface and will panic if exercised — -// ResolveWorkspaceArg must only call ListTeams. -type fakeListTeamsClient struct { - api.Client - teams []model.Team -} - -func (c *fakeListTeamsClient) ListTeams(_ context.Context) ([]model.Team, error) { - return c.teams, nil -} - func ptrRole(r model.TeamMemberRole) *model.TeamMemberRole { return &r } func TestResolveWorkspaceArg_EmptyArg(t *testing.T) { - _, err := ResolveWorkspaceArg(context.Background(), &fakeListTeamsClient{}, " ") + _, err := cmdutil.ResolveWorkspaceArg(nil, " ") if err == nil || !strings.Contains(err.Error(), "required") { t.Fatalf("want 'required' error, got %v", err) } @@ -32,8 +19,8 @@ func TestResolveWorkspaceArg_EmptyArg(t *testing.T) { func TestResolveWorkspaceArg_ByID_Match(t *testing.T) { id := "65aa1234567890abcdef1234" - c := &fakeListTeamsClient{teams: []model.Team{{ID: id, Name: "acme", MyRole: ptrRole(model.TeamMemberRoleAdministrator)}}} - team, err := ResolveWorkspaceArg(context.Background(), c, id) + teams := []model.Team{{ID: id, Name: "acme", MyRole: ptrRole(model.TeamMemberRoleAdministrator)}} + team, err := cmdutil.ResolveWorkspaceArg(teams, id) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -42,20 +29,35 @@ func TestResolveWorkspaceArg_ByID_Match(t *testing.T) { } } +// TestResolveWorkspaceArg_ByID_CaseInsensitive: isObjectIDHex accepts upper +// and lower hex, so the comparison against teams[i].ID must do the same. +// Mongo ObjectIDs are conventionally lowercase, but a user pasting a UUID +// from a browser tab might end up with uppercase — that must still resolve. +func TestResolveWorkspaceArg_ByID_CaseInsensitive(t *testing.T) { + teams := []model.Team{{ID: "65aa1234567890abcdef1234", Name: "acme"}} + team, err := cmdutil.ResolveWorkspaceArg(teams, "65AA1234567890ABCDEF1234") + if err != nil { + t.Fatalf("uppercase ID should resolve, got %v", err) + } + if team.ID != "65aa1234567890abcdef1234" { + t.Fatalf("returned team ID = %s, want lowercase canonical", team.ID) + } +} + func TestResolveWorkspaceArg_ByID_NotAMember(t *testing.T) { - c := &fakeListTeamsClient{teams: []model.Team{{ID: "65aa1234567890abcdef1234", Name: "acme"}}} - _, err := ResolveWorkspaceArg(context.Background(), c, "65bbffffffffffffffffffff") - if err == nil || !strings.Contains(err.Error(), "not a team") && !strings.Contains(err.Error(), "no team") { + teams := []model.Team{{ID: "65aa1234567890abcdef1234", Name: "acme"}} + _, err := cmdutil.ResolveWorkspaceArg(teams, "65bbffffffffffffffffffff") + if err == nil || !(strings.Contains(err.Error(), "not a team") || strings.Contains(err.Error(), "no team")) { t.Fatalf("want membership error, got %v", err) } } func TestResolveWorkspaceArg_ByName_Unique(t *testing.T) { - c := &fakeListTeamsClient{teams: []model.Team{ + teams := []model.Team{ {ID: "65aa1234567890abcdef1234", Name: "acme", MyRole: ptrRole(model.TeamMemberRoleEditor)}, {ID: "65bb5678901234abcdef5678", Name: "beta", MyRole: ptrRole(model.TeamMemberRoleViewer)}, - }} - team, err := ResolveWorkspaceArg(context.Background(), c, "acme") + } + team, err := cmdutil.ResolveWorkspaceArg(teams, "acme") if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -65,8 +67,8 @@ func TestResolveWorkspaceArg_ByName_Unique(t *testing.T) { } func TestResolveWorkspaceArg_ByName_NotFound(t *testing.T) { - c := &fakeListTeamsClient{teams: []model.Team{{ID: "65aa1234567890abcdef1234", Name: "acme"}}} - _, err := ResolveWorkspaceArg(context.Background(), c, "zeta") + teams := []model.Team{{ID: "65aa1234567890abcdef1234", Name: "acme"}} + _, err := cmdutil.ResolveWorkspaceArg(teams, "zeta") if err == nil || !strings.Contains(err.Error(), `no workspace named`) { t.Fatalf("want 'no workspace named' error, got %v", err) } @@ -79,40 +81,33 @@ func TestResolveWorkspaceArg_ByName_NotFound(t *testing.T) { func TestResolveWorkspaceArg_ByName_Ambiguous(t *testing.T) { id1 := "65aa1234567890abcdef1234" id2 := "65bb5678901234abcdef5678" - c := &fakeListTeamsClient{teams: []model.Team{ + teams := []model.Team{ {ID: id1, Name: "acme", MyRole: ptrRole(model.TeamMemberRoleAdministrator)}, {ID: id2, Name: "acme", MyRole: ptrRole(model.TeamMemberRoleViewer)}, - }} - _, err := ResolveWorkspaceArg(context.Background(), c, "acme") + } + _, err := cmdutil.ResolveWorkspaceArg(teams, "acme") if err == nil { t.Fatal("want ambiguous error, got nil") } msg := err.Error() - for _, want := range []string{"ambiguous", "2 workspaces named", id1, id2, "Administrator", "Viewer", "zeabur workspace switch"} { + for _, want := range []string{"ambiguous", "2 workspaces named", id1, id2, "Administrator", "Viewer", "workspace switch"} { if !strings.Contains(msg, want) { t.Errorf("error missing %q\nfull: %s", want, msg) } } } -// TestIsObjectIDHex sanity-checks the predicate that decides whether to take -// the ID path vs the name path in ResolveWorkspaceArg. -func TestIsObjectIDHex(t *testing.T) { - cases := []struct { - s string - want bool - }{ - {"65aa1234567890abcdef1234", true}, - {"65AA1234567890ABCDEF1234", true}, - {"65aa1234567890abcdef123", false}, // 23 chars - {"65aa1234567890abcdef12345", false}, // 25 chars - {"65aa1234567890abcdef123g", false}, // non-hex - {"", false}, - {"acme", false}, +// TestResolveWorkspaceArg_NonHex_NotMistakenForID guards the hex-vs-name +// branch: a 24-char string that isn't valid hex falls into the name path, +// not the ID path. Otherwise a user typing a name that happens to be 24 +// characters long would get the misleading "no team with id" error. +func TestResolveWorkspaceArg_NonHex_NotMistakenForID(t *testing.T) { + teams := []model.Team{{ID: "65aa1234567890abcdef1234", Name: "non-hex-but-twentyfour"}} + team, err := cmdutil.ResolveWorkspaceArg(teams, "non-hex-but-twentyfour") + if err != nil { + t.Fatalf("unexpected error: %v", err) } - for _, tc := range cases { - if got := isObjectIDHex(tc.s); got != tc.want { - t.Errorf("isObjectIDHex(%q) = %v, want %v", tc.s, got, tc.want) - } + if team.ID != "65aa1234567890abcdef1234" { + t.Fatalf("team ID = %s, want 65aa...", team.ID) } } diff --git a/pkg/api/project.go b/pkg/api/project.go index 259e64e..dd0bbcb 100644 --- a/pkg/api/project.go +++ b/pkg/api/project.go @@ -50,7 +50,9 @@ func (c *client) ListAllProjects(ctx context.Context, ownerID string) (model.Pro var projects []*model.Project for next { - projectCon, err := c.ListProjects(context.Background(), ownerID, skip, 100) + // Propagate the caller's context so cancellation / deadlines + // reach each page request (CodeRabbit PLA-1590 review). + projectCon, err := c.ListProjects(ctx, ownerID, skip, 100) if err != nil { return nil, err } From 61e837863f3ecfee62893cadfccdffe44df196fa Mon Sep 17 00:00:00 2001 From: Bruce Du Date: Wed, 20 May 2026 13:47:14 +0800 Subject: [PATCH 03/10] fix: make `--name` lookups workspace-aware (PLA-1590 review F2) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex flagged `context set project --name` falling back to a personal- account lookup when the active workspace is a team. The same pattern lives in every CLI command that resolves a project / service by name — they all route through util.GetProjectByName / util.GetServiceByName, which under the hood key on the caller's personal username via the backend `project(owner, name)` / `service(owner, projectName, name)` queries. So in a team workspace, today, `--name` will either: - silently miss a team-only project / service ("not found"), or - worse, find a same-named personal one and pin to it — the next command would then deploy / delete / restart under the wrong owner. Fix at the choke point: route the name path through the workspace-aware helpers. Personal path (ownerID == "") preserves the original `project(owner, name)` / `service(owner, projectName, name)` query exactly as before — no behavior change for the non-team caller. Team path walks `ListAllProjects(ownerID)` / `ListAllServices(projectID)` and filters by name locally. Both helpers' signatures grow new owner / project arguments; ~20 call sites updated mechanically. ID-based paths (`--id`) are workspace-agnostic and stay on the ApiClient.GetProject / GetService direct queries. Service lookups in a team workspace require a pinned project context because services are project-scoped and a service name is only unique within a project — the helper surfaces this with an actionable error pointing at `zeabur context set project --id` rather than silently falling through to the personal account. Tests cover both paths plus the failure-propagation invariant (a team list error must not silently fall through to a personal lookup): TestGetProjectByName_Personal TestGetProjectByName_TeamFound TestGetProjectByName_TeamNotFound TestGetProjectByName_TeamListErr TestGetServiceByName_Personal TestGetServiceByName_TeamFound TestGetServiceByName_TeamWithoutProjectContext TestGetServiceByName_TeamNotFound TestGetServiceByName_TeamListErr All pass; vet clean; full test suite clean. Co-Authored-By: Claude Opus 4.7 (1M context) --- internal/cmd/context/set/set.go | 40 +++++- internal/cmd/deployment/get/get.go | 2 +- internal/cmd/deployment/list/list.go | 2 +- internal/cmd/deployment/log/log.go | 2 +- internal/cmd/domain/create/create.go | 2 +- internal/cmd/domain/delete/delete.go | 2 +- internal/cmd/domain/list/list.go | 2 +- internal/cmd/project/clone/clone.go | 4 +- internal/cmd/project/delete/delete.go | 2 +- internal/cmd/project/export/export.go | 2 +- internal/cmd/project/get/get.go | 18 ++- internal/cmd/service/delete/delete.go | 2 +- internal/cmd/service/exec/exec.go | 2 +- internal/cmd/service/get/get.go | 2 +- .../cmd/service/instruction/instruction.go | 2 +- internal/cmd/service/metric/metric.go | 2 +- internal/cmd/service/network/network.go | 2 +- .../cmd/service/port-forward/port_forward.go | 2 +- internal/cmd/service/redeploy/redeploy.go | 2 +- internal/cmd/service/restart/restart.go | 2 +- internal/cmd/service/suspend/suspend.go | 2 +- internal/cmd/service/update/tag/tag.go | 2 +- internal/cmd/variable/create/create.go | 2 +- internal/cmd/variable/delete/delete.go | 2 +- internal/cmd/variable/env/env.go | 2 +- internal/cmd/variable/list/list.go | 2 +- internal/cmd/variable/update/update.go | 2 +- internal/util/project.go | 38 ++++- internal/util/project_test.go | 120 ++++++++++++++++ internal/util/service.go | 41 ++++-- internal/util/service_test.go | 130 ++++++++++++++++++ 31 files changed, 391 insertions(+), 48 deletions(-) create mode 100644 internal/util/project_test.go create mode 100644 internal/util/service_test.go diff --git a/internal/cmd/context/set/set.go b/internal/cmd/context/set/set.go index d2ba4b3..addad2c 100644 --- a/internal/cmd/context/set/set.go +++ b/internal/cmd/context/set/set.go @@ -11,6 +11,8 @@ import ( "github.com/spf13/cobra" "github.com/zeabur/cli/internal/cmdutil" + "github.com/zeabur/cli/internal/util" + "github.com/zeabur/cli/pkg/model" "github.com/zeabur/cli/pkg/zcontext" ) @@ -115,8 +117,21 @@ func setProject(f *cmdutil.Factory, id, name string, shouldCheck bool) error { } if shouldCheck { - ctx := context.Background() - project, err := f.ApiClient.GetProject(ctx, id, f.Config.GetUsername(), name) + var ( + project *model.Project + err error + ) + if id != "" { + // ID path is workspace-agnostic — `project(_id)` resolves the + // owner from the project itself, so this works for both + // personal and team-owned projects. + project, err = f.ApiClient.GetProject(context.Background(), id, "", "") + } else { + // Name path: must respect the active workspace, otherwise a + // team workspace silently looks up the project under the + // caller's personal account. + project, err = util.GetProjectByName(f.ApiClient, f.CurrentOwnerID(), f.Config.GetUsername(), name) + } if err != nil { return fmt.Errorf("failed to get project: %w", err) } @@ -197,8 +212,25 @@ func setService(f *cmdutil.Factory, id, name string, shouldCheck bool) error { } if shouldCheck { - ctx := context.Background() - service, err := f.ApiClient.GetService(ctx, id, f.Config.GetUsername(), f.Config.GetContext().GetProject().GetName(), name) + var ( + service *model.Service + err error + ) + if id != "" { + service, err = f.ApiClient.GetService(context.Background(), id, "", "", "") + } else { + // Same workspace-aware lookup as setProject — `service(owner, + // projectName, name)` keys on the caller's personal username + // and would miss a team-owned project. + service, err = util.GetServiceByName( + f.ApiClient, + f.CurrentOwnerID(), + f.Config.GetUsername(), + f.Config.GetContext().GetProject().GetName(), + f.Config.GetContext().GetProject().GetID(), + name, + ) + } if err != nil { return fmt.Errorf("failed to get service: %w", err) } diff --git a/internal/cmd/deployment/get/get.go b/internal/cmd/deployment/get/get.go index 07e5a49..f631b36 100644 --- a/internal/cmd/deployment/get/get.go +++ b/internal/cmd/deployment/get/get.go @@ -81,7 +81,7 @@ func runGetNonInteractive(f *cmdutil.Factory, opts *Options) (err error) { // Resolve service ID from name if opts.serviceID == "" && opts.serviceName != "" { - service, err := util.GetServiceByName(f.Config, f.ApiClient, opts.serviceName) + service, err := util.GetServiceByName(f.ApiClient, f.CurrentOwnerID(), f.Config.GetUsername(), f.Config.GetContext().GetProject().GetName(), f.Config.GetContext().GetProject().GetID(), opts.serviceName) if err != nil { return fmt.Errorf("failed to get service: %w", err) } diff --git a/internal/cmd/deployment/list/list.go b/internal/cmd/deployment/list/list.go index 40cc163..deb938a 100644 --- a/internal/cmd/deployment/list/list.go +++ b/internal/cmd/deployment/list/list.go @@ -63,7 +63,7 @@ func runListInteractive(f *cmdutil.Factory, opts *Options) error { func runListNonInteractive(f *cmdutil.Factory, opts *Options) error { // Resolve service ID from name if opts.serviceID == "" && opts.serviceName != "" { - service, err := util.GetServiceByName(f.Config, f.ApiClient, opts.serviceName) + service, err := util.GetServiceByName(f.ApiClient, f.CurrentOwnerID(), f.Config.GetUsername(), f.Config.GetContext().GetProject().GetName(), f.Config.GetContext().GetProject().GetID(), opts.serviceName) if err != nil { return fmt.Errorf("failed to get service: %w", err) } diff --git a/internal/cmd/deployment/log/log.go b/internal/cmd/deployment/log/log.go index a89420e..565920d 100644 --- a/internal/cmd/deployment/log/log.go +++ b/internal/cmd/deployment/log/log.go @@ -80,7 +80,7 @@ func runLogInteractive(f *cmdutil.Factory, opts *Options) error { func runLogNonInteractive(f *cmdutil.Factory, opts *Options) (err error) { // Resolve serviceID from serviceName first if opts.serviceID == "" && opts.serviceName != "" { - service, err := util.GetServiceByName(f.Config, f.ApiClient, opts.serviceName) + service, err := util.GetServiceByName(f.ApiClient, f.CurrentOwnerID(), f.Config.GetUsername(), f.Config.GetContext().GetProject().GetName(), f.Config.GetContext().GetProject().GetID(), opts.serviceName) if err != nil { return fmt.Errorf("failed to get service: %w", err) } diff --git a/internal/cmd/domain/create/create.go b/internal/cmd/domain/create/create.go index 9d3a569..881f452 100644 --- a/internal/cmd/domain/create/create.go +++ b/internal/cmd/domain/create/create.go @@ -151,7 +151,7 @@ func runCreateDomainInteractive(f *cmdutil.Factory, opts *Options) error { func runCreateDomainNonInteractive(f *cmdutil.Factory, opts *Options) error { if opts.id == "" && opts.name != "" { - service, err := util.GetServiceByName(f.Config, f.ApiClient, opts.name) + service, err := util.GetServiceByName(f.ApiClient, f.CurrentOwnerID(), f.Config.GetUsername(), f.Config.GetContext().GetProject().GetName(), f.Config.GetContext().GetProject().GetID(), opts.name) if err != nil { return err } diff --git a/internal/cmd/domain/delete/delete.go b/internal/cmd/domain/delete/delete.go index 2159d18..8df4453 100644 --- a/internal/cmd/domain/delete/delete.go +++ b/internal/cmd/domain/delete/delete.go @@ -107,7 +107,7 @@ func runDeleteDomainInteractive(f *cmdutil.Factory, opts *Options) error { func runDeleteDomainNonInteractive(f *cmdutil.Factory, opts *Options) error { if opts.id == "" && opts.name != "" { - service, err := util.GetServiceByName(f.Config, f.ApiClient, opts.name) + service, err := util.GetServiceByName(f.ApiClient, f.CurrentOwnerID(), f.Config.GetUsername(), f.Config.GetContext().GetProject().GetName(), f.Config.GetContext().GetProject().GetID(), opts.name) if err != nil { return err } diff --git a/internal/cmd/domain/list/list.go b/internal/cmd/domain/list/list.go index 4726688..00a93b7 100644 --- a/internal/cmd/domain/list/list.go +++ b/internal/cmd/domain/list/list.go @@ -64,7 +64,7 @@ func runListDomainsInteractive(f *cmdutil.Factory, opts *Options) error { func runListDomainsNonInteractive(f *cmdutil.Factory, opts *Options) error { if opts.id == "" && opts.name != "" { - service, err := util.GetServiceByName(f.Config, f.ApiClient, opts.name) + service, err := util.GetServiceByName(f.ApiClient, f.CurrentOwnerID(), f.Config.GetUsername(), f.Config.GetContext().GetProject().GetName(), f.Config.GetContext().GetProject().GetID(), opts.name) if err != nil { return err } diff --git a/internal/cmd/project/clone/clone.go b/internal/cmd/project/clone/clone.go index b8d3337..2a41fd1 100644 --- a/internal/cmd/project/clone/clone.go +++ b/internal/cmd/project/clone/clone.go @@ -71,7 +71,7 @@ func runCloneInteractive(f *cmdutil.Factory, opts *Options) error { } opts.ProjectID = projectInfo.GetID() } else if opts.ProjectID == "" && opts.ProjectName != "" { - project, err := util.GetProjectByName(f.Config, f.ApiClient, opts.ProjectName) + project, err := util.GetProjectByName(f.ApiClient, f.CurrentOwnerID(), f.Config.GetUsername(), opts.ProjectName) if err != nil { return err } @@ -132,7 +132,7 @@ func runCloneNonInteractive(f *cmdutil.Factory, opts *Options) error { // Resolve project ID from name if needed if opts.ProjectID == "" && opts.ProjectName != "" { - project, err := util.GetProjectByName(f.Config, f.ApiClient, opts.ProjectName) + project, err := util.GetProjectByName(f.ApiClient, f.CurrentOwnerID(), f.Config.GetUsername(), opts.ProjectName) if err != nil { return err } diff --git a/internal/cmd/project/delete/delete.go b/internal/cmd/project/delete/delete.go index 2efec4a..48b56a0 100644 --- a/internal/cmd/project/delete/delete.go +++ b/internal/cmd/project/delete/delete.go @@ -49,7 +49,7 @@ func runDeleteNonInteractive(f *cmdutil.Factory, opts *Options) error { } if opts.id == "" && opts.name != "" { - project, err := util.GetProjectByName(f.Config, f.ApiClient, opts.name) + project, err := util.GetProjectByName(f.ApiClient, f.CurrentOwnerID(), f.Config.GetUsername(), opts.name) if err != nil { return err } diff --git a/internal/cmd/project/export/export.go b/internal/cmd/project/export/export.go index 6669d32..0090220 100644 --- a/internal/cmd/project/export/export.go +++ b/internal/cmd/project/export/export.go @@ -41,7 +41,7 @@ func runExport(f *cmdutil.Factory, opts Options) error { } if opts.ProjectID == "" && opts.ProjectName != "" { - project, err := util.GetProjectByName(f.Config, f.ApiClient, opts.ProjectName) + project, err := util.GetProjectByName(f.ApiClient, f.CurrentOwnerID(), f.Config.GetUsername(), opts.ProjectName) if err != nil { return fmt.Errorf("get project %s failed: %w", opts.ProjectName, err) } diff --git a/internal/cmd/project/get/get.go b/internal/cmd/project/get/get.go index d51b394..1c3a6b2 100644 --- a/internal/cmd/project/get/get.go +++ b/internal/cmd/project/get/get.go @@ -8,6 +8,7 @@ import ( "github.com/zeabur/cli/internal/util" "github.com/zeabur/cli/internal/cmdutil" + "github.com/zeabur/cli/pkg/model" ) type Options struct { @@ -57,9 +58,20 @@ func runGetNonInteractive(f *cmdutil.Factory, opts *Options) error { return err } - ownerName := f.Config.GetUsername() - - project, err := f.ApiClient.GetProject(context.Background(), opts.id, ownerName, opts.name) + var ( + project *model.Project + err error + ) + if opts.id != "" { + // ID path resolves the owner from the project itself — works for + // both personal and team-owned projects. + project, err = f.ApiClient.GetProject(context.Background(), opts.id, "", "") + } else { + // Name path must respect the active workspace; the backend + // `project(owner, name)` query keys on the caller's personal + // username and would miss team-owned projects. + project, err = util.GetProjectByName(f.ApiClient, f.CurrentOwnerID(), f.Config.GetUsername(), opts.name) + } if err != nil { return fmt.Errorf("failed to get project: %w", err) } diff --git a/internal/cmd/service/delete/delete.go b/internal/cmd/service/delete/delete.go index 2df1221..80e4206 100644 --- a/internal/cmd/service/delete/delete.go +++ b/internal/cmd/service/delete/delete.go @@ -58,7 +58,7 @@ func runDeleteInteractive(f *cmdutil.Factory, opts *Options) error { func runDeleteNonInteractive(f *cmdutil.Factory, opts *Options) error { if opts.id == "" && opts.name != "" { - service, err := util.GetServiceByName(f.Config, f.ApiClient, opts.name) + service, err := util.GetServiceByName(f.ApiClient, f.CurrentOwnerID(), f.Config.GetUsername(), f.Config.GetContext().GetProject().GetName(), f.Config.GetContext().GetProject().GetID(), opts.name) if err != nil { return err } diff --git a/internal/cmd/service/exec/exec.go b/internal/cmd/service/exec/exec.go index 9bd8a62..506fb01 100644 --- a/internal/cmd/service/exec/exec.go +++ b/internal/cmd/service/exec/exec.go @@ -76,7 +76,7 @@ func runExecInteractive(f *cmdutil.Factory, opts *Options) error { func runExecNonInteractive(f *cmdutil.Factory, opts *Options) error { if opts.id == "" && opts.name != "" { - service, err := util.GetServiceByName(f.Config, f.ApiClient, opts.name) + service, err := util.GetServiceByName(f.ApiClient, f.CurrentOwnerID(), f.Config.GetUsername(), f.Config.GetContext().GetProject().GetName(), f.Config.GetContext().GetProject().GetID(), opts.name) if err != nil { return err } diff --git a/internal/cmd/service/get/get.go b/internal/cmd/service/get/get.go index 7741221..eb9c517 100644 --- a/internal/cmd/service/get/get.go +++ b/internal/cmd/service/get/get.go @@ -59,7 +59,7 @@ func runGetInteractive(f *cmdutil.Factory, opts *Options) error { func runGetNonInteractive(f *cmdutil.Factory, opts *Options) error { if opts.id == "" && opts.name != "" { - service, err := util.GetServiceByName(f.Config, f.ApiClient, opts.name) + service, err := util.GetServiceByName(f.ApiClient, f.CurrentOwnerID(), f.Config.GetUsername(), f.Config.GetContext().GetProject().GetName(), f.Config.GetContext().GetProject().GetID(), opts.name) if err != nil { return err } diff --git a/internal/cmd/service/instruction/instruction.go b/internal/cmd/service/instruction/instruction.go index f49a151..61e4361 100644 --- a/internal/cmd/service/instruction/instruction.go +++ b/internal/cmd/service/instruction/instruction.go @@ -53,7 +53,7 @@ func runInstruction(f *cmdutil.Factory, opts *Options) error { } if opts.id == "" && opts.name != "" { - service, err := util.GetServiceByName(f.Config, f.ApiClient, opts.name) + service, err := util.GetServiceByName(f.ApiClient, f.CurrentOwnerID(), f.Config.GetUsername(), f.Config.GetContext().GetProject().GetName(), f.Config.GetContext().GetProject().GetID(), opts.name) if err != nil { return err } diff --git a/internal/cmd/service/metric/metric.go b/internal/cmd/service/metric/metric.go index eb1f3be..65b01fa 100644 --- a/internal/cmd/service/metric/metric.go +++ b/internal/cmd/service/metric/metric.go @@ -73,7 +73,7 @@ func runMetricInteractive(f *cmdutil.Factory, opts *Options) error { func runMetricNonInteractive(f *cmdutil.Factory, opts *Options) error { if opts.id == "" && opts.name != "" { - service, err := util.GetServiceByName(f.Config, f.ApiClient, opts.name) + service, err := util.GetServiceByName(f.ApiClient, f.CurrentOwnerID(), f.Config.GetUsername(), f.Config.GetContext().GetProject().GetName(), f.Config.GetContext().GetProject().GetID(), opts.name) if err != nil { return err } diff --git a/internal/cmd/service/network/network.go b/internal/cmd/service/network/network.go index 5093b60..e58218f 100644 --- a/internal/cmd/service/network/network.go +++ b/internal/cmd/service/network/network.go @@ -52,7 +52,7 @@ func runNetwork(f *cmdutil.Factory, opts *Options) error { } if opts.id == "" && opts.name != "" { - service, err := util.GetServiceByName(f.Config, f.ApiClient, opts.name) + service, err := util.GetServiceByName(f.ApiClient, f.CurrentOwnerID(), f.Config.GetUsername(), f.Config.GetContext().GetProject().GetName(), f.Config.GetContext().GetProject().GetID(), opts.name) if err != nil { return err } diff --git a/internal/cmd/service/port-forward/port_forward.go b/internal/cmd/service/port-forward/port_forward.go index 82265f3..8b43f41 100644 --- a/internal/cmd/service/port-forward/port_forward.go +++ b/internal/cmd/service/port-forward/port_forward.go @@ -78,7 +78,7 @@ func runPortForwardInteractive(f *cmdutil.Factory, opts *Options) error { func runPortForwardNonInteractive(f *cmdutil.Factory, opts *Options) error { if opts.id == "" && opts.name != "" { - service, err := util.GetServiceByName(f.Config, f.ApiClient, opts.name) + service, err := util.GetServiceByName(f.ApiClient, f.CurrentOwnerID(), f.Config.GetUsername(), f.Config.GetContext().GetProject().GetName(), f.Config.GetContext().GetProject().GetID(), opts.name) if err != nil { return err } diff --git a/internal/cmd/service/redeploy/redeploy.go b/internal/cmd/service/redeploy/redeploy.go index 7ffb19d..9d44183 100644 --- a/internal/cmd/service/redeploy/redeploy.go +++ b/internal/cmd/service/redeploy/redeploy.go @@ -63,7 +63,7 @@ func runRedeployInteractive(f *cmdutil.Factory, opts *Options) error { func runRedeployNonInteractive(f *cmdutil.Factory, opts *Options) error { if opts.id == "" && opts.name != "" { - service, err := util.GetServiceByName(f.Config, f.ApiClient, opts.name) + service, err := util.GetServiceByName(f.ApiClient, f.CurrentOwnerID(), f.Config.GetUsername(), f.Config.GetContext().GetProject().GetName(), f.Config.GetContext().GetProject().GetID(), opts.name) if err != nil { return err } diff --git a/internal/cmd/service/restart/restart.go b/internal/cmd/service/restart/restart.go index 8227a74..0c894bc 100644 --- a/internal/cmd/service/restart/restart.go +++ b/internal/cmd/service/restart/restart.go @@ -63,7 +63,7 @@ func runRestartInteractive(f *cmdutil.Factory, opts *Options) error { func runRestartNonInteractive(f *cmdutil.Factory, opts *Options) error { if opts.id == "" && opts.name != "" { - service, err := util.GetServiceByName(f.Config, f.ApiClient, opts.name) + service, err := util.GetServiceByName(f.ApiClient, f.CurrentOwnerID(), f.Config.GetUsername(), f.Config.GetContext().GetProject().GetName(), f.Config.GetContext().GetProject().GetID(), opts.name) if err != nil { return err } diff --git a/internal/cmd/service/suspend/suspend.go b/internal/cmd/service/suspend/suspend.go index a96a729..47ac3e8 100644 --- a/internal/cmd/service/suspend/suspend.go +++ b/internal/cmd/service/suspend/suspend.go @@ -63,7 +63,7 @@ func runSuspendInteractive(f *cmdutil.Factory, opts *Options) error { func runSuspendNonInteractive(f *cmdutil.Factory, opts *Options) error { if opts.id == "" && opts.name != "" { - service, err := util.GetServiceByName(f.Config, f.ApiClient, opts.name) + service, err := util.GetServiceByName(f.ApiClient, f.CurrentOwnerID(), f.Config.GetUsername(), f.Config.GetContext().GetProject().GetName(), f.Config.GetContext().GetProject().GetID(), opts.name) if err != nil { return err } diff --git a/internal/cmd/service/update/tag/tag.go b/internal/cmd/service/update/tag/tag.go index 043f201..b0d606e 100644 --- a/internal/cmd/service/update/tag/tag.go +++ b/internal/cmd/service/update/tag/tag.go @@ -78,7 +78,7 @@ func runInteractive(f *cmdutil.Factory, opts *Options) error { func runNonInteractive(f *cmdutil.Factory, opts *Options) error { if opts.id == "" && opts.name != "" { - service, err := util.GetServiceByName(f.Config, f.ApiClient, opts.name) + service, err := util.GetServiceByName(f.ApiClient, f.CurrentOwnerID(), f.Config.GetUsername(), f.Config.GetContext().GetProject().GetName(), f.Config.GetContext().GetProject().GetID(), opts.name) if err != nil { return err } diff --git a/internal/cmd/variable/create/create.go b/internal/cmd/variable/create/create.go index b62fe4a..013aade 100644 --- a/internal/cmd/variable/create/create.go +++ b/internal/cmd/variable/create/create.go @@ -87,7 +87,7 @@ func runCreateVariableInteractive(f *cmdutil.Factory, opts *Options) error { func runCreateVariableNonInteractive(f *cmdutil.Factory, opts *Options) error { if opts.id == "" && opts.name != "" { - service, err := util.GetServiceByName(f.Config, f.ApiClient, opts.name) + service, err := util.GetServiceByName(f.ApiClient, f.CurrentOwnerID(), f.Config.GetUsername(), f.Config.GetContext().GetProject().GetName(), f.Config.GetContext().GetProject().GetID(), opts.name) if err != nil { return err } diff --git a/internal/cmd/variable/delete/delete.go b/internal/cmd/variable/delete/delete.go index 0c1788f..8114622 100644 --- a/internal/cmd/variable/delete/delete.go +++ b/internal/cmd/variable/delete/delete.go @@ -106,7 +106,7 @@ func runDeleteVariableInteractive(f *cmdutil.Factory, opts *Options) error { func runDeleteVariableNonInteractive(f *cmdutil.Factory, opts *Options) error { if opts.id == "" && opts.name != "" { - service, err := util.GetServiceByName(f.Config, f.ApiClient, opts.name) + service, err := util.GetServiceByName(f.ApiClient, f.CurrentOwnerID(), f.Config.GetUsername(), f.Config.GetContext().GetProject().GetName(), f.Config.GetContext().GetProject().GetID(), opts.name) if err != nil { return err } diff --git a/internal/cmd/variable/env/env.go b/internal/cmd/variable/env/env.go index 5354358..cc1832c 100644 --- a/internal/cmd/variable/env/env.go +++ b/internal/cmd/variable/env/env.go @@ -66,7 +66,7 @@ func runUpdateVariableByEnv(f *cmdutil.Factory, opts *Options) error { func runUpdateVariableNonInteractive(f *cmdutil.Factory, opts *Options) error { if opts.id == "" && opts.name != "" { - service, err := util.GetServiceByName(f.Config, f.ApiClient, opts.name) + service, err := util.GetServiceByName(f.ApiClient, f.CurrentOwnerID(), f.Config.GetUsername(), f.Config.GetContext().GetProject().GetName(), f.Config.GetContext().GetProject().GetID(), opts.name) if err != nil { return err } diff --git a/internal/cmd/variable/list/list.go b/internal/cmd/variable/list/list.go index 23d9cdb..d9d6a5f 100644 --- a/internal/cmd/variable/list/list.go +++ b/internal/cmd/variable/list/list.go @@ -63,7 +63,7 @@ func runListVariablesInteractive(f *cmdutil.Factory, opts *Options) error { func runListVariablesNonInteractive(f *cmdutil.Factory, opts *Options) error { if opts.id == "" && opts.name != "" { - service, err := util.GetServiceByName(f.Config, f.ApiClient, opts.name) + service, err := util.GetServiceByName(f.ApiClient, f.CurrentOwnerID(), f.Config.GetUsername(), f.Config.GetContext().GetProject().GetName(), f.Config.GetContext().GetProject().GetID(), opts.name) if err != nil { return err } diff --git a/internal/cmd/variable/update/update.go b/internal/cmd/variable/update/update.go index c03e27d..544181d 100644 --- a/internal/cmd/variable/update/update.go +++ b/internal/cmd/variable/update/update.go @@ -128,7 +128,7 @@ func runUpdateVariableInteractive(f *cmdutil.Factory, opts *Options) error { func runUpdateVariableNonInteractive(f *cmdutil.Factory, opts *Options) error { if opts.id == "" && opts.name != "" { - service, err := util.GetServiceByName(f.Config, f.ApiClient, opts.name) + service, err := util.GetServiceByName(f.ApiClient, f.CurrentOwnerID(), f.Config.GetUsername(), f.Config.GetContext().GetProject().GetName(), f.Config.GetContext().GetProject().GetID(), opts.name) if err != nil { return err } diff --git a/internal/util/project.go b/internal/util/project.go index b2e7978..1f020b2 100644 --- a/internal/util/project.go +++ b/internal/util/project.go @@ -6,18 +6,42 @@ import ( "github.com/spf13/cobra" "github.com/zeabur/cli/pkg/api" - "github.com/zeabur/cli/pkg/config" "github.com/zeabur/cli/pkg/model" ) -func GetProjectByName(config config.Config, client api.Client, projectName string) (project *model.Project, err error) { - ownerName := config.GetUsername() - project, err = client.GetProject(context.Background(), "", ownerName, projectName) - if err != nil { - return nil, fmt.Errorf("get project<%s> failed: %w", projectName, err) +// GetProjectByName resolves a project by name within the active workspace. +// +// - Personal workspace (ownerID == ""): uses the cheap +// `project(owner: $personalUsername, name: $projectName)` query against +// the caller's account. +// - Team workspace (ownerID != ""): there is no `project(ownerID, name)` +// query, so walk the owner's project list and match by name locally. +// +// Without the team branch a `--name`-based lookup in a team workspace would +// silently fall through to the caller's personal account — either failing +// to find the project, or worse, returning a same-named personal project +// instead of the intended team one. The personal path is unchanged, so +// existing personal-workspace callers see no behavior change. +func GetProjectByName(client api.Client, ownerID, personalUsername, projectName string) (*model.Project, error) { + ctx := context.Background() + if ownerID == "" { + project, err := client.GetProject(ctx, "", personalUsername, projectName) + if err != nil { + return nil, fmt.Errorf("get project<%s> failed: %w", projectName, err) + } + return project, nil } - return project, nil + projects, err := client.ListAllProjects(ctx, ownerID) + if err != nil { + return nil, fmt.Errorf("list projects in workspace: %w", err) + } + for _, p := range projects { + if p.Name == projectName { + return p, nil + } + } + return nil, fmt.Errorf("no project named %q in this workspace", projectName) } func AddProjectParam(cmd *cobra.Command, id, name *string) { diff --git a/internal/util/project_test.go b/internal/util/project_test.go new file mode 100644 index 0000000..d6bbb85 --- /dev/null +++ b/internal/util/project_test.go @@ -0,0 +1,120 @@ +package util_test + +import ( + "context" + "errors" + "strings" + "testing" + + "github.com/zeabur/cli/internal/util" + "github.com/zeabur/cli/pkg/api" + "github.com/zeabur/cli/pkg/model" +) + +// fakeProjectClient stubs the two ProjectAPI methods util.GetProjectByName +// reaches for. Everything else inherits the nil embedded interface and panics +// if accidentally exercised. +type fakeProjectClient struct { + api.Client + + // personal-path arguments captured for assertions + getProjectCalls []getProjectCall + getProjectRet *model.Project + getProjectErr error + + // team-path arguments captured for assertions + listAllOwner string + listAllProjects []*model.Project + listAllErr error +} + +type getProjectCall struct{ id, owner, name string } + +func (c *fakeProjectClient) GetProject(_ context.Context, id, owner, name string) (*model.Project, error) { + c.getProjectCalls = append(c.getProjectCalls, getProjectCall{id, owner, name}) + return c.getProjectRet, c.getProjectErr +} + +func (c *fakeProjectClient) ListAllProjects(_ context.Context, ownerID string) (model.Projects, error) { + c.listAllOwner = ownerID + return c.listAllProjects, c.listAllErr +} + +// TestGetProjectByName_Personal: ownerID == "" → backend `project(owner, name)` +// query against the personal username, unchanged from before workspace support. +func TestGetProjectByName_Personal(t *testing.T) { + want := &model.Project{ID: "65aa1234567890abcdef1234", Name: "api"} + c := &fakeProjectClient{getProjectRet: want} + + got, err := util.GetProjectByName(c, "", "alice", "api") + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if got != want { + t.Fatalf("got %v, want %v", got, want) + } + if len(c.getProjectCalls) != 1 { + t.Fatalf("expected 1 GetProject call, got %d", len(c.getProjectCalls)) + } + call := c.getProjectCalls[0] + if call.id != "" || call.owner != "alice" || call.name != "api" { + t.Fatalf("call args = %+v, want id=\"\" owner=alice name=api", call) + } + if c.listAllOwner != "" { + t.Fatalf("ListAllProjects should not have been called on personal path (owner=%q)", c.listAllOwner) + } +} + +// TestGetProjectByName_TeamFound: ownerID set → ListAllProjects(ownerID) + +// match by name. Critically, MUST NOT touch the personal-username path. +func TestGetProjectByName_TeamFound(t *testing.T) { + teamID := "65cc1234567890abcdef0000" + team1 := &model.Project{ID: "65aa1234567890abcdef1234", Name: "api"} + team2 := &model.Project{ID: "65bb5678901234abcdef5678", Name: "web"} + c := &fakeProjectClient{listAllProjects: []*model.Project{team1, team2}} + + got, err := util.GetProjectByName(c, teamID, "alice", "web") + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if got.ID != team2.ID { + t.Fatalf("got ID %q, want %q", got.ID, team2.ID) + } + if c.listAllOwner != teamID { + t.Fatalf("ListAllProjects called with owner %q, want %q", c.listAllOwner, teamID) + } + if len(c.getProjectCalls) != 0 { + t.Fatalf("personal GetProject path must not run in team workspace; got %d calls", len(c.getProjectCalls)) + } +} + +// TestGetProjectByName_TeamNotFound: ownerID set, name missing in the +// team's list → error names the workspace, NOT a 404 from a personal lookup. +func TestGetProjectByName_TeamNotFound(t *testing.T) { + c := &fakeProjectClient{listAllProjects: []*model.Project{{ID: "x", Name: "api"}}} + + _, err := util.GetProjectByName(c, "65cc1234567890abcdef0000", "alice", "missing") + if err == nil { + t.Fatal("want error, got nil") + } + if !strings.Contains(err.Error(), "no project named") { + t.Fatalf("error = %v, want 'no project named ...' message", err) + } +} + +// TestGetProjectByName_TeamListErr: a backend failure on the team path must +// propagate the underlying error, not silently fall through to personal. +func TestGetProjectByName_TeamListErr(t *testing.T) { + c := &fakeProjectClient{listAllErr: errors.New("boom")} + + _, err := util.GetProjectByName(c, "65cc1234567890abcdef0000", "alice", "api") + if err == nil { + t.Fatal("want error, got nil") + } + if !strings.Contains(err.Error(), "list projects in workspace") || !strings.Contains(err.Error(), "boom") { + t.Fatalf("error = %v, want wrapped boom", err) + } + if len(c.getProjectCalls) != 0 { + t.Fatalf("personal fallback must not run when team list fails; got %d calls", len(c.getProjectCalls)) + } +} diff --git a/internal/util/service.go b/internal/util/service.go index f1db8e8..b7ff1b7 100644 --- a/internal/util/service.go +++ b/internal/util/service.go @@ -6,19 +6,44 @@ import ( "github.com/spf13/cobra" "github.com/zeabur/cli/pkg/api" - "github.com/zeabur/cli/pkg/config" "github.com/zeabur/cli/pkg/model" ) -func GetServiceByName(config config.Config, client api.Client, serviceName string) (service *model.Service, err error) { - ownerName := config.GetUsername() - projectName := config.GetContext().GetProject().GetName() - service, err = client.GetService(context.Background(), "", ownerName, projectName, serviceName) - if err != nil { - return nil, fmt.Errorf("get service<%s> failed: %w", serviceName, err) +// GetServiceByName resolves a service by name within the active workspace. +// +// - Personal workspace (ownerID == ""): uses the cheap +// `service(owner, projectName, name)` query. +// - Team workspace (ownerID != ""): the personal query keys on the +// caller's username and would silently look at the personal account. +// Use the project-scoped `ListAllServices(projectID)` and match by name +// locally — projectID is unique across owners and team-safe. +// +// In a team workspace the caller must have a project context (projectID), +// because services have no top-level owner-scoped lookup and a service name +// is only unique within a project. Personal path is unchanged. +func GetServiceByName(client api.Client, ownerID, personalUsername, projectName, projectID, serviceName string) (*model.Service, error) { + ctx := context.Background() + if ownerID == "" { + service, err := client.GetService(ctx, "", personalUsername, projectName, serviceName) + if err != nil { + return nil, fmt.Errorf("get service<%s> failed: %w", serviceName, err) + } + return service, nil } - return service, nil + if projectID == "" { + return nil, fmt.Errorf("cannot resolve service by name in a team workspace without a project context — set a project first (e.g. `zeabur context set project --id `)") + } + services, err := client.ListAllServices(ctx, projectID) + if err != nil { + return nil, fmt.Errorf("list services in project: %w", err) + } + for _, s := range services { + if s.Name == serviceName { + return s, nil + } + } + return nil, fmt.Errorf("no service named %q in this project", serviceName) } func AddServiceParam(cmd *cobra.Command, id, name *string) { diff --git a/internal/util/service_test.go b/internal/util/service_test.go new file mode 100644 index 0000000..d243c5b --- /dev/null +++ b/internal/util/service_test.go @@ -0,0 +1,130 @@ +package util_test + +import ( + "context" + "errors" + "strings" + "testing" + + "github.com/zeabur/cli/internal/util" + "github.com/zeabur/cli/pkg/api" + "github.com/zeabur/cli/pkg/model" +) + +type fakeServiceClient struct { + api.Client + + getServiceCalls []getServiceCall + getServiceRet *model.Service + getServiceErr error + + listAllServicesProject string + listAllServices model.Services + listAllServicesErr error +} + +type getServiceCall struct{ id, owner, projectName, name string } + +func (c *fakeServiceClient) GetService(_ context.Context, id, owner, projectName, name string) (*model.Service, error) { + c.getServiceCalls = append(c.getServiceCalls, getServiceCall{id, owner, projectName, name}) + return c.getServiceRet, c.getServiceErr +} + +func (c *fakeServiceClient) ListAllServices(_ context.Context, projectID string) (model.Services, error) { + c.listAllServicesProject = projectID + return c.listAllServices, c.listAllServicesErr +} + +// TestGetServiceByName_Personal: ownerID == "" preserves the personal +// `service(owner, projectName, name)` query exactly as before — same args, +// same backend call. Existing personal users see zero behavior change. +func TestGetServiceByName_Personal(t *testing.T) { + want := &model.Service{ID: "65aa1234567890abcdef1234", Name: "web"} + c := &fakeServiceClient{getServiceRet: want} + + got, err := util.GetServiceByName(c, "", "alice", "api", "65cc1234567890abcdef0000", "web") + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if got != want { + t.Fatalf("got %v, want %v", got, want) + } + if len(c.getServiceCalls) != 1 { + t.Fatalf("expected 1 GetService call, got %d", len(c.getServiceCalls)) + } + if c.getServiceCalls[0] != (getServiceCall{id: "", owner: "alice", projectName: "api", name: "web"}) { + t.Fatalf("call args = %+v", c.getServiceCalls[0]) + } + if c.listAllServicesProject != "" { + t.Fatalf("personal path must not call ListAllServices, got project=%q", c.listAllServicesProject) + } +} + +// TestGetServiceByName_TeamFound: team workspace uses projectID-scoped +// ListAllServices and matches by name. Personal username argument is +// deliberately unused on this path. +func TestGetServiceByName_TeamFound(t *testing.T) { + projectID := "65cc1234567890abcdef0000" + svc1 := &model.Service{ID: "65aa1234567890abcdef1234", Name: "web"} + svc2 := &model.Service{ID: "65bb5678901234abcdef5678", Name: "worker"} + c := &fakeServiceClient{listAllServices: model.Services{svc1, svc2}} + + got, err := util.GetServiceByName(c, "65cc1234567890abcdefffff", "alice", "api", projectID, "worker") + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if got.ID != svc2.ID { + t.Fatalf("got ID %q, want %q", got.ID, svc2.ID) + } + if c.listAllServicesProject != projectID { + t.Fatalf("ListAllServices called with project %q, want %q", c.listAllServicesProject, projectID) + } + if len(c.getServiceCalls) != 0 { + t.Fatalf("personal GetService must not run in team workspace; got %d calls", len(c.getServiceCalls)) + } +} + +// TestGetServiceByName_TeamWithoutProjectContext: a service lookup by name +// in a team workspace needs a project context (services are scoped to +// projects). Surface the actionable error rather than silently falling +// through to the personal account. +func TestGetServiceByName_TeamWithoutProjectContext(t *testing.T) { + _, err := util.GetServiceByName(&fakeServiceClient{}, "65cc1234567890abcdefffff", "alice", "api", "", "web") + if err == nil { + t.Fatal("want error when team workspace has no project context") + } + if !strings.Contains(err.Error(), "without a project context") { + t.Fatalf("error = %v, want 'without a project context' message", err) + } +} + +// TestGetServiceByName_TeamNotFound: name missing in the project's service +// list errors with a project-scoped message, not a personal-account 404. +func TestGetServiceByName_TeamNotFound(t *testing.T) { + c := &fakeServiceClient{listAllServices: model.Services{{ID: "x", Name: "web"}}} + + _, err := util.GetServiceByName(c, "65cc1234567890abcdefffff", "alice", "api", "65cc1234567890abcdef0000", "missing") + if err == nil { + t.Fatal("want error, got nil") + } + if !strings.Contains(err.Error(), "no service named") { + t.Fatalf("error = %v, want 'no service named ...'", err) + } +} + +// TestGetServiceByName_TeamListErr: a backend failure on the team path must +// propagate, not silently fall through to a personal-username lookup. +func TestGetServiceByName_TeamListErr(t *testing.T) { + c := &fakeServiceClient{listAllServicesErr: errors.New("boom")} + + _, err := util.GetServiceByName(c, "65cc1234567890abcdefffff", "alice", "api", "65cc1234567890abcdef0000", "web") + if err == nil { + t.Fatal("want error, got nil") + } + if !strings.Contains(err.Error(), "list services in project") || !strings.Contains(err.Error(), "boom") { + t.Fatalf("error = %v, want wrapped boom", err) + } + if len(c.getServiceCalls) != 0 { + t.Fatalf("personal fallback must not run; got %d calls", len(c.getServiceCalls)) + } +} From b97244f8d90cf0e0dce39851ec19496f68b1a95f Mon Sep 17 00:00:00 2001 From: Bruce Du Date: Wed, 20 May 2026 14:12:07 +0800 Subject: [PATCH 04/10] fix: lint, set.go name backfill, Factory regression tests (PLA-1590) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CI lint: - workspace_test.go:50 — staticcheck QF1001 (De Morgan): rewrite the membership-error assertion from `!(X || Y)` to `(!X && !Y)`. Same truth table, no longer trips the negated-OR rule. Verified locally with `staticcheck -checks QF1001`. CodeRabbit on the F2 commit: - set.go setProject / setService — when the user passed only `--id`, the local `name` variable stayed empty and the success log read "Project context is set to <>". Backfill `name` from the resolved `project.Name` / `service.Name` after the lookup succeeds, so both ID-only and name-only invocations log the resolved name. Backward-compat regression coverage (per Bruce's red-line review of the util signature changes) — Factory now has dedicated tests: - TestFactory_PersonalUserInvariant — a zero Factory (the brand-new user shape) reports CurrentOwnerID() == "" and IsPersonal(). This is the single most important invariant of PLA-1590: every owner-aware util helper checks for the empty string before deciding between the legacy personal query path and the new team-aware one. A regression here silently routes personal users through the team branch. - TestFactory_CurrentOwnerID_PersistedPersonal — Config present but no persisted workspace must still report personal. - TestFactory_CurrentOwnerID_PersistedTeam — sanity on the persisted team path. - TestFactory_CurrentOwnerID_OverrideBeatsPersisted — `--workspace` override takes precedence and does NOT leak into the persisted file. - TestFactory_CurrentOwnerID_OverrideNilClears — passing nil to SetWorkspaceOverride drops the override. - TestFactory_ListTeams_Memoizes — one process, one backend hit, no matter how many sites ask. Guards the F2 fanout fix. - TestFactory_ListTeams_StickyError — a failed fetch caches the error so subsequent callers don't retry against an already-broken backend. All tests pass; full vet + suite clean. Co-Authored-By: Claude Opus 4.7 (1M context) --- internal/cmd/context/set/set.go | 8 +- internal/cmdutil/factory_test.go | 177 +++++++++++++++++++++++++++++ internal/cmdutil/workspace_test.go | 2 +- 3 files changed, 184 insertions(+), 3 deletions(-) create mode 100644 internal/cmdutil/factory_test.go diff --git a/internal/cmd/context/set/set.go b/internal/cmd/context/set/set.go index addad2c..f55c04b 100644 --- a/internal/cmd/context/set/set.go +++ b/internal/cmd/context/set/set.go @@ -136,7 +136,9 @@ func setProject(f *cmdutil.Factory, id, name string, shouldCheck bool) error { return fmt.Errorf("failed to get project: %w", err) } f.Config.GetContext().SetProject(zcontext.NewBasicInfo(project.ID, project.Name)) - + // User may have passed only --id; backfill the local `name` so the + // success log below names the resolved project instead of "<>". + name = project.Name } else { f.Config.GetContext().SetProject(zcontext.NewBasicInfo(id, name)) } @@ -235,7 +237,9 @@ func setService(f *cmdutil.Factory, id, name string, shouldCheck bool) error { return fmt.Errorf("failed to get service: %w", err) } f.Config.GetContext().SetService(zcontext.NewBasicInfo(service.ID, service.Name)) - + // User may have passed only --id; backfill the local `name` so the + // success log below names the resolved service instead of "<>". + name = service.Name } else { f.Config.GetContext().SetService(zcontext.NewBasicInfo(id, name)) } diff --git a/internal/cmdutil/factory_test.go b/internal/cmdutil/factory_test.go new file mode 100644 index 0000000..031ca1b --- /dev/null +++ b/internal/cmdutil/factory_test.go @@ -0,0 +1,177 @@ +package cmdutil_test + +import ( + "context" + "errors" + "testing" + + "github.com/spf13/viper" + "github.com/zeabur/cli/internal/cmdutil" + "github.com/zeabur/cli/pkg/api" + "github.com/zeabur/cli/pkg/model" + "github.com/zeabur/cli/pkg/zcontext" +) + +// stubConfig is a minimal config.Config — only GetContext is exercised by +// the workspace plumbing. Other methods are no-ops so any test that needs +// them would notice the missing behavior. +type stubConfig struct { + ctx zcontext.Context +} + +func (s stubConfig) GetTokenString() string { return "" } +func (s stubConfig) SetTokenString(string) {} +func (s stubConfig) GetUser() string { return "" } +func (s stubConfig) SetUser(string) {} +func (s stubConfig) GetUsername() string { return "" } +func (s stubConfig) SetUsername(string) {} +func (s stubConfig) GetContext() zcontext.Context { return s.ctx } +func (s stubConfig) Write() error { return nil } + +// TestFactory_PersonalUserInvariant guards the single most important +// backward-compat rule of PLA-1590: a brand-new caller — one who has never +// run `workspace switch` and never set `--workspace` — must report +// CurrentOwnerID() == "". That empty string is what every owner-aware util +// helper checks before deciding between the legacy personal query path and +// the new team-aware one. If this ever returns non-empty for a vanilla +// Factory we'd be silently routing personal users through the team branch. +func TestFactory_PersonalUserInvariant(t *testing.T) { + f := &cmdutil.Factory{} // zero Factory = brand-new user + if got := f.CurrentOwnerID(); got != "" { + t.Fatalf("brand-new Factory must report personal (empty ownerID), got %q", got) + } + ws := f.CurrentWorkspace() + if !ws.IsPersonal() { + t.Fatalf("brand-new Factory must report personal workspace, got %+v", ws) + } +} + +// TestFactory_CurrentOwnerID_PersistedPersonal mirrors the run-time shape: +// a Factory with Config but the workspace field unset (the explicit +// "personal" persisted state, or a fresh config file). Must still report +// personal. +func TestFactory_CurrentOwnerID_PersistedPersonal(t *testing.T) { + cfg := stubConfig{ctx: zcontext.NewViperContext(viper.New())} + f := &cmdutil.Factory{Config: cfg} + if got := f.CurrentOwnerID(); got != "" { + t.Fatalf("persisted empty workspace must report personal, got %q", got) + } + if !f.CurrentWorkspace().IsPersonal() { + t.Fatalf("CurrentWorkspace().IsPersonal() must be true for empty persisted") + } +} + +// TestFactory_CurrentOwnerID_PersistedTeam: the persisted-team path returns +// the team's ID and a workspace marked IsTeam(). +func TestFactory_CurrentOwnerID_PersistedTeam(t *testing.T) { + v := viper.New() + v.Set("workspace.id", "65aa1234567890abcdef1234") + v.Set("workspace.name", "acme") + v.Set("workspace.kind", zcontext.WorkspaceKindTeam) + cfg := stubConfig{ctx: zcontext.NewViperContext(v)} + f := &cmdutil.Factory{Config: cfg} + + if got := f.CurrentOwnerID(); got != "65aa1234567890abcdef1234" { + t.Fatalf("got %q, want persisted team ID", got) + } + ws := f.CurrentWorkspace() + if !ws.IsTeam() || ws.Name != "acme" { + t.Fatalf("got %+v, want team workspace acme", ws) + } +} + +// TestFactory_CurrentOwnerID_OverrideBeatsPersisted: --workspace flag +// resolution sets an override on the Factory that must take precedence +// over the persisted workspace for the lifetime of that invocation. The +// persisted file is left alone. +func TestFactory_CurrentOwnerID_OverrideBeatsPersisted(t *testing.T) { + v := viper.New() + v.Set("workspace.id", "persisted-id") + v.Set("workspace.name", "persisted-team") + v.Set("workspace.kind", zcontext.WorkspaceKindTeam) + cfg := stubConfig{ctx: zcontext.NewViperContext(v)} + f := &cmdutil.Factory{Config: cfg} + + f.SetWorkspaceOverride(&zcontext.Workspace{ + ID: "override-id", Name: "override-team", Kind: zcontext.WorkspaceKindTeam, + }) + if got := f.CurrentOwnerID(); got != "override-id" { + t.Fatalf("got %q, want override-id", got) + } + if ws := f.CurrentWorkspace(); ws.Name != "override-team" { + t.Fatalf("got %q, want override name", ws.Name) + } + // Persisted file untouched: the override is process-local. + if v.GetString("workspace.id") != "persisted-id" { + t.Fatalf("override leaked into persisted config: %q", v.GetString("workspace.id")) + } +} + +// TestFactory_CurrentOwnerID_OverrideNilClears: passing nil to +// SetWorkspaceOverride drops the override and the persisted workspace +// becomes effective again. +func TestFactory_CurrentOwnerID_OverrideNilClears(t *testing.T) { + cfg := stubConfig{ctx: zcontext.NewViperContext(viper.New())} + f := &cmdutil.Factory{Config: cfg} + f.SetWorkspaceOverride(&zcontext.Workspace{ID: "abc"}) + if got := f.CurrentOwnerID(); got != "abc" { + t.Fatalf("got %q, want abc after set", got) + } + f.SetWorkspaceOverride(nil) + if got := f.CurrentOwnerID(); got != "" { + t.Fatalf("got %q, want empty after clear", got) + } +} + +// fakeListTeamsAPI counts ListTeams invocations so the cache tests can +// assert how many backend round-trips the Factory makes. +type fakeListTeamsAPI struct { + api.Client + calls int + teamsRet []model.Team + errRet error +} + +func (f *fakeListTeamsAPI) ListTeams(_ context.Context) ([]model.Team, error) { + f.calls++ + return f.teamsRet, f.errRet +} + +// TestFactory_ListTeams_Memoizes guards F2 from the review: a single CLI +// invocation can ask for the teams list from up to three sites (flag +// resolution, lazy verify, the command itself). They must all share one +// backend call. +func TestFactory_ListTeams_Memoizes(t *testing.T) { + stub := &fakeListTeamsAPI{teamsRet: []model.Team{{ID: "x"}}} + f := &cmdutil.Factory{ApiClient: stub} + + for i := 0; i < 3; i++ { + teams, err := f.ListTeams(context.Background()) + if err != nil { + t.Fatalf("call %d: %v", i, err) + } + if len(teams) != 1 || teams[0].ID != "x" { + t.Fatalf("call %d: got %+v", i, teams) + } + } + if stub.calls != 1 { + t.Errorf("ListTeams hit backend %d times, want exactly 1 (memoized)", stub.calls) + } +} + +// TestFactory_ListTeams_StickyError: a failed fetch is cached as a failure +// so subsequent callers within the same process don't retry against an +// already-known-broken backend. +func TestFactory_ListTeams_StickyError(t *testing.T) { + stub := &fakeListTeamsAPI{errRet: errors.New("boom")} + f := &cmdutil.Factory{ApiClient: stub} + + for i := 0; i < 3; i++ { + if _, err := f.ListTeams(context.Background()); err == nil { + t.Fatalf("call %d: want error, got nil", i) + } + } + if stub.calls != 1 { + t.Errorf("ListTeams hit backend %d times, want exactly 1 (sticky cache)", stub.calls) + } +} diff --git a/internal/cmdutil/workspace_test.go b/internal/cmdutil/workspace_test.go index 7b8ae47..849245c 100644 --- a/internal/cmdutil/workspace_test.go +++ b/internal/cmdutil/workspace_test.go @@ -47,7 +47,7 @@ func TestResolveWorkspaceArg_ByID_CaseInsensitive(t *testing.T) { func TestResolveWorkspaceArg_ByID_NotAMember(t *testing.T) { teams := []model.Team{{ID: "65aa1234567890abcdef1234", Name: "acme"}} _, err := cmdutil.ResolveWorkspaceArg(teams, "65bbffffffffffffffffffff") - if err == nil || !(strings.Contains(err.Error(), "not a team") || strings.Contains(err.Error(), "no team")) { + if err == nil || (!strings.Contains(err.Error(), "not a team") && !strings.Contains(err.Error(), "no team")) { t.Fatalf("want membership error, got %v", err) } } From 736851b0f75dd5c6f190032fdf72adb9af1d8a35 Mon Sep 17 00:00:00 2001 From: Bruce Du Date: Wed, 20 May 2026 15:48:26 +0800 Subject: [PATCH 05/10] fix: workspace current / list must honor --workspace override (PLA-1590) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both commands read the persisted workspace directly via `f.Config.GetContext().GetWorkspace()`, so a `--workspace foo` override was silently ignored — `workspace current` reported the persisted workspace and `workspace list` put `*` on the persisted row, not on the override. Caught running the Bucket 6 dev-2 E2E: $ /tmp/zeabur-dev2 workspace switch team-A $ /tmp/zeabur-dev2 --workspace team-B workspace current team-A [...] team ... # wrong — override was team-B This is the same class of bug as the deploy.go F1 finding from the panel review: display source must match the resolved source the rest of the command sees. Switch both commands to `f.CurrentWorkspace()` / `f.CurrentOwnerID()` so they reflect the effective workspace for the invocation. Existing `TestFactory_CurrentOwnerID_OverrideBeatsPersisted` covers the underlying helper; this fix is the consumer side. Verified on dev-2 against api-2.zeabur.dev: $ /tmp/zeabur-dev2 --workspace pla1230 workspace current pla1230-probe-... [69e72943...] team Administrator # ✓ $ /tmp/zeabur-dev2 --workspace pla1230 workspace list * 69e72943... pla1230-probe-... team Administrator # ✓ marker on override Co-Authored-By: Claude Opus 4.7 (1M context) --- internal/cmd/workspace/current/current.go | 6 +++++- internal/cmd/workspace/list/list.go | 6 +++++- 2 files changed, 10 insertions(+), 2 deletions(-) diff --git a/internal/cmd/workspace/current/current.go b/internal/cmd/workspace/current/current.go index 0879372..2450302 100644 --- a/internal/cmd/workspace/current/current.go +++ b/internal/cmd/workspace/current/current.go @@ -23,7 +23,11 @@ func NewCmdCurrent(f *cmdutil.Factory) *cobra.Command { } func run(f *cmdutil.Factory) error { - ws := f.Config.GetContext().GetWorkspace() + // Use the effective workspace (CurrentWorkspace) so the report honours + // a `--workspace` flag override for the current invocation, matching + // what every other command in the process sees. Reading the persisted + // workspace directly would silently lie about the override. + ws := f.CurrentWorkspace() if ws.IsPersonal() { label := f.Config.GetUser() if label == "" { diff --git a/internal/cmd/workspace/list/list.go b/internal/cmd/workspace/list/list.go index c639abb..c0e155b 100644 --- a/internal/cmd/workspace/list/list.go +++ b/internal/cmd/workspace/list/list.go @@ -31,7 +31,11 @@ func run(f *cmdutil.Factory) error { return fmt.Errorf("list teams: %w", err) } - currentID := f.Config.GetContext().GetWorkspace().ID + // Use the effective workspace so the `*` marker tracks a `--workspace` + // flag override, not just the persisted state. Otherwise + // `--workspace foo workspace list` would print `*` on the persisted + // team rather than `foo`. + currentID := f.CurrentOwnerID() w := tabwriter.NewWriter(os.Stdout, 0, 0, 3, ' ', 0) From b5667e2e1f760196737ef0cbfbc3d259b8ac0052 Mon Sep 17 00:00:00 2001 From: Bruce Du Date: Mon, 25 May 2026 14:06:38 +0800 Subject: [PATCH 06/10] fix: --workspace must not read or write persisted inner context (PLA-1590) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex's round-4 review surfaced a real cross-workspace contamination bug that the previous test coverage happened to walk around. Path 1 (read stale context under override): $ zeabur workspace switch team-A $ zeabur context set project --id $ zeabur --workspace team-B service delete --name redis --yes The team branch of util.GetServiceByName trusted the persisted project ID, so the delete landed on team-A's redis even though the user typed --workspace team-B. Same risk on every service / variable / deployment / domain consumer that resolves by name. Path 2 (write persisted context under override): $ zeabur workspace switch team-A $ zeabur --workspace team-B project create --name x --region y # non-JSON The non-JSON branch of project create called setProject, pinning the team-B project under persisted workspace team-A. Subsequent commands without --workspace then used the wrong combination. Both paths are regressions introduced by PLA-1590 — pre-PR, name lookups always went through the personal `service(owner=username, ...)` query which simply couldn't find team-owned projects, so the cross-team risk was structurally impossible. The team branch added by F2 is what opened the door. The fix codifies `--workspace` as a stateless one-shot override: 1. Factory.HasWorkspaceOverride() — predicate. Every override-gated behaviour goes through this. 2. Factory.CurrentProjectID() / CurrentProjectName() / CurrentServiceID() / CurrentEnvironmentID() — return "" when an override is active, so name-based downstream lookups fail-closed with the existing "cannot resolve service by name in a team workspace without a project context" error. The non-override path is byte-equivalent to reading the persisted context directly, so vanilla users see no change. 3. project create + deploy interactive create / select paths skip the setProject write when an override is active, and log a one-line hint telling the user how to make it the default (`workspace switch ...`). 4. context set rejects up front when an override is active — writing persistent state under a one-shot override is incoherent. The error tells the user to run `workspace switch` first. All 24 service/domain/variable/deployment consumers route through the new Factory helpers instead of reading the persisted project context directly, so there's one chokepoint, not 24 places to keep in sync. Tests: TestFactory_HasWorkspaceOverride TestFactory_CurrentInnerContext_OverrideHides TestFactory_CurrentInnerContext_NilConfigSafe Plus dev-2 E2E (this round): - C1: persisted=pla1230 + pinned for-clone-test, `--workspace pla1277-presub service delete --name redis --yes` → "cannot resolve service by name..." error; for-clone-test redis NOT deleted (verified via service list). - C2: persisted=pla1230, `--workspace pla1230 project create --name pla1590-bplus-c2-test --region ...` (non-JSON) → succeeds, prints the one-shot hint, persisted context.project still for-clone-test. - C3: `--workspace ... context set project ...` → rejected with actionable hint pointing at `workspace switch`. - C5: no override → `context set service --name redis` still pins correctly (back-compat). Co-Authored-By: Claude Opus 4.7 (1M context) --- internal/cmd/context/set/set.go | 14 +++ internal/cmd/deploy/deploy.go | 12 +- internal/cmd/deployment/get/get.go | 2 +- internal/cmd/deployment/list/list.go | 2 +- internal/cmd/deployment/log/log.go | 2 +- internal/cmd/domain/create/create.go | 2 +- internal/cmd/domain/delete/delete.go | 2 +- internal/cmd/domain/list/list.go | 2 +- internal/cmd/project/create/create.go | 10 ++ internal/cmd/service/delete/delete.go | 2 +- internal/cmd/service/exec/exec.go | 2 +- internal/cmd/service/get/get.go | 2 +- .../cmd/service/instruction/instruction.go | 2 +- internal/cmd/service/metric/metric.go | 2 +- internal/cmd/service/network/network.go | 2 +- .../cmd/service/port-forward/port_forward.go | 2 +- internal/cmd/service/redeploy/redeploy.go | 2 +- internal/cmd/service/restart/restart.go | 2 +- internal/cmd/service/suspend/suspend.go | 2 +- internal/cmd/service/update/tag/tag.go | 2 +- internal/cmd/variable/create/create.go | 2 +- internal/cmd/variable/delete/delete.go | 2 +- internal/cmd/variable/env/env.go | 2 +- internal/cmd/variable/list/list.go | 2 +- internal/cmd/variable/update/update.go | 2 +- internal/cmdutil/factory.go | 59 ++++++++++ internal/cmdutil/factory_test.go | 103 ++++++++++++++++++ 27 files changed, 218 insertions(+), 24 deletions(-) diff --git a/internal/cmd/context/set/set.go b/internal/cmd/context/set/set.go index f55c04b..4b1777b 100644 --- a/internal/cmd/context/set/set.go +++ b/internal/cmd/context/set/set.go @@ -68,6 +68,20 @@ func NewCmdSet(f *cmdutil.Factory) *cobra.Command { } func runSet(f *cmdutil.Factory, opts *Options) error { + // `context set` writes persistent state — it cannot be combined with the + // one-shot `--workspace` override, because the persisted context is + // always interpreted relative to the persisted workspace, never the + // override. Mixing the two would leave a project / service / environment + // from team B pinned under persisted workspace A, which then causes + // silent cross-workspace operations on subsequent commands. Reject up + // front with an actionable hint (PLA-1590 B+). + if f.HasWorkspaceOverride() { + return fmt.Errorf( + "`context set` writes persistent state and cannot be combined with `--workspace`; " + + "run `zeabur workspace switch ` first, then `zeabur context set ...`", + ) + } + if f.Interactive { return runSetInteractive(f, opts) } diff --git a/internal/cmd/deploy/deploy.go b/internal/cmd/deploy/deploy.go index d1f54ab..998c8b5 100644 --- a/internal/cmd/deploy/deploy.go +++ b/internal/cmd/deploy/deploy.go @@ -224,7 +224,13 @@ func selectInteractively(f *cmdutil.Factory, opts *Options) (*model.Service, *mo return nil, nil, err } f.Log.Infof("Project %s created. Run this command again to deploy on it.", project.Name) - f.Config.GetContext().SetProject(zcontext.NewBasicInfo(project.ID, project.Name)) + // `--workspace` override is one-shot (PLA-1590 B+): the newly- + // created project belongs to the override workspace, not to the + // persisted one, so writing it to the persisted context would + // silently cross workspaces on the next command. Skip. + if !f.HasWorkspaceOverride() { + f.Config.GetContext().SetProject(zcontext.NewBasicInfo(project.ID, project.Name)) + } return nil, nil, nil } @@ -237,7 +243,9 @@ func selectInteractively(f *cmdutil.Factory, opts *Options) (*model.Service, *mo return nil, nil, err } - f.Config.GetContext().SetProject(zcontext.NewBasicInfo(project.ID, project.Name)) + if !f.HasWorkspaceOverride() { + f.Config.GetContext().SetProject(zcontext.NewBasicInfo(project.ID, project.Name)) + } _, environment, err := f.Selector.SelectEnvironment(project.ID) if err != nil { diff --git a/internal/cmd/deployment/get/get.go b/internal/cmd/deployment/get/get.go index f631b36..066d6c2 100644 --- a/internal/cmd/deployment/get/get.go +++ b/internal/cmd/deployment/get/get.go @@ -81,7 +81,7 @@ func runGetNonInteractive(f *cmdutil.Factory, opts *Options) (err error) { // Resolve service ID from name if opts.serviceID == "" && opts.serviceName != "" { - service, err := util.GetServiceByName(f.ApiClient, f.CurrentOwnerID(), f.Config.GetUsername(), f.Config.GetContext().GetProject().GetName(), f.Config.GetContext().GetProject().GetID(), opts.serviceName) + service, err := util.GetServiceByName(f.ApiClient, f.CurrentOwnerID(), f.Config.GetUsername(), f.CurrentProjectName(), f.CurrentProjectID(), opts.serviceName) if err != nil { return fmt.Errorf("failed to get service: %w", err) } diff --git a/internal/cmd/deployment/list/list.go b/internal/cmd/deployment/list/list.go index deb938a..b0af4ff 100644 --- a/internal/cmd/deployment/list/list.go +++ b/internal/cmd/deployment/list/list.go @@ -63,7 +63,7 @@ func runListInteractive(f *cmdutil.Factory, opts *Options) error { func runListNonInteractive(f *cmdutil.Factory, opts *Options) error { // Resolve service ID from name if opts.serviceID == "" && opts.serviceName != "" { - service, err := util.GetServiceByName(f.ApiClient, f.CurrentOwnerID(), f.Config.GetUsername(), f.Config.GetContext().GetProject().GetName(), f.Config.GetContext().GetProject().GetID(), opts.serviceName) + service, err := util.GetServiceByName(f.ApiClient, f.CurrentOwnerID(), f.Config.GetUsername(), f.CurrentProjectName(), f.CurrentProjectID(), opts.serviceName) if err != nil { return fmt.Errorf("failed to get service: %w", err) } diff --git a/internal/cmd/deployment/log/log.go b/internal/cmd/deployment/log/log.go index 565920d..142a7d5 100644 --- a/internal/cmd/deployment/log/log.go +++ b/internal/cmd/deployment/log/log.go @@ -80,7 +80,7 @@ func runLogInteractive(f *cmdutil.Factory, opts *Options) error { func runLogNonInteractive(f *cmdutil.Factory, opts *Options) (err error) { // Resolve serviceID from serviceName first if opts.serviceID == "" && opts.serviceName != "" { - service, err := util.GetServiceByName(f.ApiClient, f.CurrentOwnerID(), f.Config.GetUsername(), f.Config.GetContext().GetProject().GetName(), f.Config.GetContext().GetProject().GetID(), opts.serviceName) + service, err := util.GetServiceByName(f.ApiClient, f.CurrentOwnerID(), f.Config.GetUsername(), f.CurrentProjectName(), f.CurrentProjectID(), opts.serviceName) if err != nil { return fmt.Errorf("failed to get service: %w", err) } diff --git a/internal/cmd/domain/create/create.go b/internal/cmd/domain/create/create.go index 881f452..16b909c 100644 --- a/internal/cmd/domain/create/create.go +++ b/internal/cmd/domain/create/create.go @@ -151,7 +151,7 @@ func runCreateDomainInteractive(f *cmdutil.Factory, opts *Options) error { func runCreateDomainNonInteractive(f *cmdutil.Factory, opts *Options) error { if opts.id == "" && opts.name != "" { - service, err := util.GetServiceByName(f.ApiClient, f.CurrentOwnerID(), f.Config.GetUsername(), f.Config.GetContext().GetProject().GetName(), f.Config.GetContext().GetProject().GetID(), opts.name) + service, err := util.GetServiceByName(f.ApiClient, f.CurrentOwnerID(), f.Config.GetUsername(), f.CurrentProjectName(), f.CurrentProjectID(), opts.name) if err != nil { return err } diff --git a/internal/cmd/domain/delete/delete.go b/internal/cmd/domain/delete/delete.go index 8df4453..b38fbba 100644 --- a/internal/cmd/domain/delete/delete.go +++ b/internal/cmd/domain/delete/delete.go @@ -107,7 +107,7 @@ func runDeleteDomainInteractive(f *cmdutil.Factory, opts *Options) error { func runDeleteDomainNonInteractive(f *cmdutil.Factory, opts *Options) error { if opts.id == "" && opts.name != "" { - service, err := util.GetServiceByName(f.ApiClient, f.CurrentOwnerID(), f.Config.GetUsername(), f.Config.GetContext().GetProject().GetName(), f.Config.GetContext().GetProject().GetID(), opts.name) + service, err := util.GetServiceByName(f.ApiClient, f.CurrentOwnerID(), f.Config.GetUsername(), f.CurrentProjectName(), f.CurrentProjectID(), opts.name) if err != nil { return err } diff --git a/internal/cmd/domain/list/list.go b/internal/cmd/domain/list/list.go index 00a93b7..c5975db 100644 --- a/internal/cmd/domain/list/list.go +++ b/internal/cmd/domain/list/list.go @@ -64,7 +64,7 @@ func runListDomainsInteractive(f *cmdutil.Factory, opts *Options) error { func runListDomainsNonInteractive(f *cmdutil.Factory, opts *Options) error { if opts.id == "" && opts.name != "" { - service, err := util.GetServiceByName(f.ApiClient, f.CurrentOwnerID(), f.Config.GetUsername(), f.Config.GetContext().GetProject().GetName(), f.Config.GetContext().GetProject().GetID(), opts.name) + service, err := util.GetServiceByName(f.ApiClient, f.CurrentOwnerID(), f.Config.GetUsername(), f.CurrentProjectName(), f.CurrentProjectID(), opts.name) if err != nil { return err } diff --git a/internal/cmd/project/create/create.go b/internal/cmd/project/create/create.go index 8e6a02c..bcd83e4 100644 --- a/internal/cmd/project/create/create.go +++ b/internal/cmd/project/create/create.go @@ -114,6 +114,16 @@ func createProject(f *cmdutil.Factory, projectRegion string, projectName *string return f.Printer.JSON(map[string]string{"status": "success", "id": project.ID, "name": project.Name, "message": "Project created"}) } f.Log.Infof("Project %s created", project.Name) + // `--workspace` is a stateless one-shot override (PLA-1590 B+): writing + // the newly-created project (which belongs to the override workspace) + // into the persisted context would silently pin a team-B project under + // persisted workspace team-A and cause cross-workspace operations on + // the next command. Skip the write in override mode; the user can pin + // it explicitly after `workspace switch` if they want. + if f.HasWorkspaceOverride() { + f.Log.Infof("(persistent project context not modified — `--workspace` override is one-shot; run `zeabur workspace switch %s` to make it your default)", f.CurrentWorkspace().Name) + return nil + } err = setProject(f, project.ID, project.Name) if err != nil { f.Log.Error(err) diff --git a/internal/cmd/service/delete/delete.go b/internal/cmd/service/delete/delete.go index 80e4206..032d5f5 100644 --- a/internal/cmd/service/delete/delete.go +++ b/internal/cmd/service/delete/delete.go @@ -58,7 +58,7 @@ func runDeleteInteractive(f *cmdutil.Factory, opts *Options) error { func runDeleteNonInteractive(f *cmdutil.Factory, opts *Options) error { if opts.id == "" && opts.name != "" { - service, err := util.GetServiceByName(f.ApiClient, f.CurrentOwnerID(), f.Config.GetUsername(), f.Config.GetContext().GetProject().GetName(), f.Config.GetContext().GetProject().GetID(), opts.name) + service, err := util.GetServiceByName(f.ApiClient, f.CurrentOwnerID(), f.Config.GetUsername(), f.CurrentProjectName(), f.CurrentProjectID(), opts.name) if err != nil { return err } diff --git a/internal/cmd/service/exec/exec.go b/internal/cmd/service/exec/exec.go index 506fb01..59ea608 100644 --- a/internal/cmd/service/exec/exec.go +++ b/internal/cmd/service/exec/exec.go @@ -76,7 +76,7 @@ func runExecInteractive(f *cmdutil.Factory, opts *Options) error { func runExecNonInteractive(f *cmdutil.Factory, opts *Options) error { if opts.id == "" && opts.name != "" { - service, err := util.GetServiceByName(f.ApiClient, f.CurrentOwnerID(), f.Config.GetUsername(), f.Config.GetContext().GetProject().GetName(), f.Config.GetContext().GetProject().GetID(), opts.name) + service, err := util.GetServiceByName(f.ApiClient, f.CurrentOwnerID(), f.Config.GetUsername(), f.CurrentProjectName(), f.CurrentProjectID(), opts.name) if err != nil { return err } diff --git a/internal/cmd/service/get/get.go b/internal/cmd/service/get/get.go index eb9c517..3fa13ca 100644 --- a/internal/cmd/service/get/get.go +++ b/internal/cmd/service/get/get.go @@ -59,7 +59,7 @@ func runGetInteractive(f *cmdutil.Factory, opts *Options) error { func runGetNonInteractive(f *cmdutil.Factory, opts *Options) error { if opts.id == "" && opts.name != "" { - service, err := util.GetServiceByName(f.ApiClient, f.CurrentOwnerID(), f.Config.GetUsername(), f.Config.GetContext().GetProject().GetName(), f.Config.GetContext().GetProject().GetID(), opts.name) + service, err := util.GetServiceByName(f.ApiClient, f.CurrentOwnerID(), f.Config.GetUsername(), f.CurrentProjectName(), f.CurrentProjectID(), opts.name) if err != nil { return err } diff --git a/internal/cmd/service/instruction/instruction.go b/internal/cmd/service/instruction/instruction.go index 61e4361..c11d8e6 100644 --- a/internal/cmd/service/instruction/instruction.go +++ b/internal/cmd/service/instruction/instruction.go @@ -53,7 +53,7 @@ func runInstruction(f *cmdutil.Factory, opts *Options) error { } if opts.id == "" && opts.name != "" { - service, err := util.GetServiceByName(f.ApiClient, f.CurrentOwnerID(), f.Config.GetUsername(), f.Config.GetContext().GetProject().GetName(), f.Config.GetContext().GetProject().GetID(), opts.name) + service, err := util.GetServiceByName(f.ApiClient, f.CurrentOwnerID(), f.Config.GetUsername(), f.CurrentProjectName(), f.CurrentProjectID(), opts.name) if err != nil { return err } diff --git a/internal/cmd/service/metric/metric.go b/internal/cmd/service/metric/metric.go index 65b01fa..010c593 100644 --- a/internal/cmd/service/metric/metric.go +++ b/internal/cmd/service/metric/metric.go @@ -73,7 +73,7 @@ func runMetricInteractive(f *cmdutil.Factory, opts *Options) error { func runMetricNonInteractive(f *cmdutil.Factory, opts *Options) error { if opts.id == "" && opts.name != "" { - service, err := util.GetServiceByName(f.ApiClient, f.CurrentOwnerID(), f.Config.GetUsername(), f.Config.GetContext().GetProject().GetName(), f.Config.GetContext().GetProject().GetID(), opts.name) + service, err := util.GetServiceByName(f.ApiClient, f.CurrentOwnerID(), f.Config.GetUsername(), f.CurrentProjectName(), f.CurrentProjectID(), opts.name) if err != nil { return err } diff --git a/internal/cmd/service/network/network.go b/internal/cmd/service/network/network.go index e58218f..746affb 100644 --- a/internal/cmd/service/network/network.go +++ b/internal/cmd/service/network/network.go @@ -52,7 +52,7 @@ func runNetwork(f *cmdutil.Factory, opts *Options) error { } if opts.id == "" && opts.name != "" { - service, err := util.GetServiceByName(f.ApiClient, f.CurrentOwnerID(), f.Config.GetUsername(), f.Config.GetContext().GetProject().GetName(), f.Config.GetContext().GetProject().GetID(), opts.name) + service, err := util.GetServiceByName(f.ApiClient, f.CurrentOwnerID(), f.Config.GetUsername(), f.CurrentProjectName(), f.CurrentProjectID(), opts.name) if err != nil { return err } diff --git a/internal/cmd/service/port-forward/port_forward.go b/internal/cmd/service/port-forward/port_forward.go index 8b43f41..c3d212f 100644 --- a/internal/cmd/service/port-forward/port_forward.go +++ b/internal/cmd/service/port-forward/port_forward.go @@ -78,7 +78,7 @@ func runPortForwardInteractive(f *cmdutil.Factory, opts *Options) error { func runPortForwardNonInteractive(f *cmdutil.Factory, opts *Options) error { if opts.id == "" && opts.name != "" { - service, err := util.GetServiceByName(f.ApiClient, f.CurrentOwnerID(), f.Config.GetUsername(), f.Config.GetContext().GetProject().GetName(), f.Config.GetContext().GetProject().GetID(), opts.name) + service, err := util.GetServiceByName(f.ApiClient, f.CurrentOwnerID(), f.Config.GetUsername(), f.CurrentProjectName(), f.CurrentProjectID(), opts.name) if err != nil { return err } diff --git a/internal/cmd/service/redeploy/redeploy.go b/internal/cmd/service/redeploy/redeploy.go index 9d44183..43142af 100644 --- a/internal/cmd/service/redeploy/redeploy.go +++ b/internal/cmd/service/redeploy/redeploy.go @@ -63,7 +63,7 @@ func runRedeployInteractive(f *cmdutil.Factory, opts *Options) error { func runRedeployNonInteractive(f *cmdutil.Factory, opts *Options) error { if opts.id == "" && opts.name != "" { - service, err := util.GetServiceByName(f.ApiClient, f.CurrentOwnerID(), f.Config.GetUsername(), f.Config.GetContext().GetProject().GetName(), f.Config.GetContext().GetProject().GetID(), opts.name) + service, err := util.GetServiceByName(f.ApiClient, f.CurrentOwnerID(), f.Config.GetUsername(), f.CurrentProjectName(), f.CurrentProjectID(), opts.name) if err != nil { return err } diff --git a/internal/cmd/service/restart/restart.go b/internal/cmd/service/restart/restart.go index 0c894bc..07c6ea2 100644 --- a/internal/cmd/service/restart/restart.go +++ b/internal/cmd/service/restart/restart.go @@ -63,7 +63,7 @@ func runRestartInteractive(f *cmdutil.Factory, opts *Options) error { func runRestartNonInteractive(f *cmdutil.Factory, opts *Options) error { if opts.id == "" && opts.name != "" { - service, err := util.GetServiceByName(f.ApiClient, f.CurrentOwnerID(), f.Config.GetUsername(), f.Config.GetContext().GetProject().GetName(), f.Config.GetContext().GetProject().GetID(), opts.name) + service, err := util.GetServiceByName(f.ApiClient, f.CurrentOwnerID(), f.Config.GetUsername(), f.CurrentProjectName(), f.CurrentProjectID(), opts.name) if err != nil { return err } diff --git a/internal/cmd/service/suspend/suspend.go b/internal/cmd/service/suspend/suspend.go index 47ac3e8..9a60e4c 100644 --- a/internal/cmd/service/suspend/suspend.go +++ b/internal/cmd/service/suspend/suspend.go @@ -63,7 +63,7 @@ func runSuspendInteractive(f *cmdutil.Factory, opts *Options) error { func runSuspendNonInteractive(f *cmdutil.Factory, opts *Options) error { if opts.id == "" && opts.name != "" { - service, err := util.GetServiceByName(f.ApiClient, f.CurrentOwnerID(), f.Config.GetUsername(), f.Config.GetContext().GetProject().GetName(), f.Config.GetContext().GetProject().GetID(), opts.name) + service, err := util.GetServiceByName(f.ApiClient, f.CurrentOwnerID(), f.Config.GetUsername(), f.CurrentProjectName(), f.CurrentProjectID(), opts.name) if err != nil { return err } diff --git a/internal/cmd/service/update/tag/tag.go b/internal/cmd/service/update/tag/tag.go index b0d606e..ec59abe 100644 --- a/internal/cmd/service/update/tag/tag.go +++ b/internal/cmd/service/update/tag/tag.go @@ -78,7 +78,7 @@ func runInteractive(f *cmdutil.Factory, opts *Options) error { func runNonInteractive(f *cmdutil.Factory, opts *Options) error { if opts.id == "" && opts.name != "" { - service, err := util.GetServiceByName(f.ApiClient, f.CurrentOwnerID(), f.Config.GetUsername(), f.Config.GetContext().GetProject().GetName(), f.Config.GetContext().GetProject().GetID(), opts.name) + service, err := util.GetServiceByName(f.ApiClient, f.CurrentOwnerID(), f.Config.GetUsername(), f.CurrentProjectName(), f.CurrentProjectID(), opts.name) if err != nil { return err } diff --git a/internal/cmd/variable/create/create.go b/internal/cmd/variable/create/create.go index 013aade..267bd45 100644 --- a/internal/cmd/variable/create/create.go +++ b/internal/cmd/variable/create/create.go @@ -87,7 +87,7 @@ func runCreateVariableInteractive(f *cmdutil.Factory, opts *Options) error { func runCreateVariableNonInteractive(f *cmdutil.Factory, opts *Options) error { if opts.id == "" && opts.name != "" { - service, err := util.GetServiceByName(f.ApiClient, f.CurrentOwnerID(), f.Config.GetUsername(), f.Config.GetContext().GetProject().GetName(), f.Config.GetContext().GetProject().GetID(), opts.name) + service, err := util.GetServiceByName(f.ApiClient, f.CurrentOwnerID(), f.Config.GetUsername(), f.CurrentProjectName(), f.CurrentProjectID(), opts.name) if err != nil { return err } diff --git a/internal/cmd/variable/delete/delete.go b/internal/cmd/variable/delete/delete.go index 8114622..4e98f3c 100644 --- a/internal/cmd/variable/delete/delete.go +++ b/internal/cmd/variable/delete/delete.go @@ -106,7 +106,7 @@ func runDeleteVariableInteractive(f *cmdutil.Factory, opts *Options) error { func runDeleteVariableNonInteractive(f *cmdutil.Factory, opts *Options) error { if opts.id == "" && opts.name != "" { - service, err := util.GetServiceByName(f.ApiClient, f.CurrentOwnerID(), f.Config.GetUsername(), f.Config.GetContext().GetProject().GetName(), f.Config.GetContext().GetProject().GetID(), opts.name) + service, err := util.GetServiceByName(f.ApiClient, f.CurrentOwnerID(), f.Config.GetUsername(), f.CurrentProjectName(), f.CurrentProjectID(), opts.name) if err != nil { return err } diff --git a/internal/cmd/variable/env/env.go b/internal/cmd/variable/env/env.go index cc1832c..19e50fc 100644 --- a/internal/cmd/variable/env/env.go +++ b/internal/cmd/variable/env/env.go @@ -66,7 +66,7 @@ func runUpdateVariableByEnv(f *cmdutil.Factory, opts *Options) error { func runUpdateVariableNonInteractive(f *cmdutil.Factory, opts *Options) error { if opts.id == "" && opts.name != "" { - service, err := util.GetServiceByName(f.ApiClient, f.CurrentOwnerID(), f.Config.GetUsername(), f.Config.GetContext().GetProject().GetName(), f.Config.GetContext().GetProject().GetID(), opts.name) + service, err := util.GetServiceByName(f.ApiClient, f.CurrentOwnerID(), f.Config.GetUsername(), f.CurrentProjectName(), f.CurrentProjectID(), opts.name) if err != nil { return err } diff --git a/internal/cmd/variable/list/list.go b/internal/cmd/variable/list/list.go index d9d6a5f..69adfe9 100644 --- a/internal/cmd/variable/list/list.go +++ b/internal/cmd/variable/list/list.go @@ -63,7 +63,7 @@ func runListVariablesInteractive(f *cmdutil.Factory, opts *Options) error { func runListVariablesNonInteractive(f *cmdutil.Factory, opts *Options) error { if opts.id == "" && opts.name != "" { - service, err := util.GetServiceByName(f.ApiClient, f.CurrentOwnerID(), f.Config.GetUsername(), f.Config.GetContext().GetProject().GetName(), f.Config.GetContext().GetProject().GetID(), opts.name) + service, err := util.GetServiceByName(f.ApiClient, f.CurrentOwnerID(), f.Config.GetUsername(), f.CurrentProjectName(), f.CurrentProjectID(), opts.name) if err != nil { return err } diff --git a/internal/cmd/variable/update/update.go b/internal/cmd/variable/update/update.go index 544181d..e422560 100644 --- a/internal/cmd/variable/update/update.go +++ b/internal/cmd/variable/update/update.go @@ -128,7 +128,7 @@ func runUpdateVariableInteractive(f *cmdutil.Factory, opts *Options) error { func runUpdateVariableNonInteractive(f *cmdutil.Factory, opts *Options) error { if opts.id == "" && opts.name != "" { - service, err := util.GetServiceByName(f.ApiClient, f.CurrentOwnerID(), f.Config.GetUsername(), f.Config.GetContext().GetProject().GetName(), f.Config.GetContext().GetProject().GetID(), opts.name) + service, err := util.GetServiceByName(f.ApiClient, f.CurrentOwnerID(), f.Config.GetUsername(), f.CurrentProjectName(), f.CurrentProjectID(), opts.name) if err != nil { return err } diff --git a/internal/cmdutil/factory.go b/internal/cmdutil/factory.go index 457f3de..02e9e3b 100644 --- a/internal/cmdutil/factory.go +++ b/internal/cmdutil/factory.go @@ -96,6 +96,65 @@ func (f *Factory) SetWorkspaceOverride(ws *zcontext.Workspace) { f.workspaceOverride = ws } +// HasWorkspaceOverride reports whether the caller invoked the command with a +// `--workspace ` flag. Use this to gate any behaviour that should +// only apply to "one-shot override" mode — most prominently, refusing to +// read or write the persisted inner context (project / environment / service), +// because that context belongs to whatever the persisted workspace is and +// would silently cross workspaces when reused under an override. +// +// PLA-1590 contract: --workspace is a stateless override. It must not +// observe or modify the persisted inner context. Commands that need a +// project / service in override mode must take an explicit `--id` flag. +func (f *Factory) HasWorkspaceOverride() bool { + return f.workspaceOverride != nil +} + +// CurrentProjectID returns the persisted project context ID, but only when +// no --workspace override is active. Under an override the persisted context +// belongs to a (potentially) different workspace, so reusing it would cross +// scopes — instead return "" so name-based service / variable / etc. lookups +// fail-closed with an actionable error and the caller must pass an explicit +// `--id` / `--service-id`. +func (f *Factory) CurrentProjectID() string { + if f.HasWorkspaceOverride() || f.Config == nil { + return "" + } + return f.Config.GetContext().GetProject().GetID() +} + +// CurrentProjectName mirrors CurrentProjectID. The personal path of the +// service-by-name lookup uses the project name; return "" under override so +// that path also refuses rather than reaches into the persisted context. +func (f *Factory) CurrentProjectName() string { + if f.HasWorkspaceOverride() || f.Config == nil { + return "" + } + return f.Config.GetContext().GetProject().GetName() +} + +// CurrentEnvironmentID — same rule as CurrentProjectID, applied to the +// persisted environment context. Reserved for future use; today no team +// command consumes environment context implicitly, but the helper keeps the +// override contract uniform across all three inner-context fields. +func (f *Factory) CurrentEnvironmentID() string { + if f.HasWorkspaceOverride() || f.Config == nil { + return "" + } + return f.Config.GetContext().GetEnvironment().GetID() +} + +// CurrentServiceID — same rule as CurrentProjectID for the persisted service +// context. Currently unused by team-path lookups (services are looked up by +// project + name) but exposed so future consumers stay on the same override +// contract. +func (f *Factory) CurrentServiceID() string { + if f.HasWorkspaceOverride() || f.Config == nil { + return "" + } + return f.Config.GetContext().GetService().GetID() +} + // ListTeams returns the caller's teams via api.Client.ListTeams, memoized for // the lifetime of this Factory. The same Factory is shared across every // PersistentPreRunE / Run / PersistentPostRunE callback within a single CLI diff --git a/internal/cmdutil/factory_test.go b/internal/cmdutil/factory_test.go index 031ca1b..11ab78a 100644 --- a/internal/cmdutil/factory_test.go +++ b/internal/cmdutil/factory_test.go @@ -123,6 +123,109 @@ func TestFactory_CurrentOwnerID_OverrideNilClears(t *testing.T) { } } +// TestFactory_HasWorkspaceOverride guards the predicate that gates every +// "stateless override" behaviour added by PLA-1590 B+. +func TestFactory_HasWorkspaceOverride(t *testing.T) { + f := &cmdutil.Factory{} + if f.HasWorkspaceOverride() { + t.Fatal("brand-new Factory must not report an override") + } + f.SetWorkspaceOverride(&zcontext.Workspace{ID: "abc"}) + if !f.HasWorkspaceOverride() { + t.Fatal("after SetWorkspaceOverride, HasWorkspaceOverride must be true") + } + f.SetWorkspaceOverride(nil) + if f.HasWorkspaceOverride() { + t.Fatal("after SetWorkspaceOverride(nil), HasWorkspaceOverride must be false") + } +} + +// TestFactory_CurrentInnerContext_OverrideHides is the core invariant of +// PLA-1590 B+: when a `--workspace` override is active, the inner persisted +// context (project / environment / service) is *not* observable. Every +// helper that consumers use to read inner-context IDs returns the empty +// string under override, even if the persisted config has values set. This +// is what makes name-based service / variable / etc. lookups fail-closed +// in override mode instead of silently operating on the wrong workspace. +func TestFactory_CurrentInnerContext_OverrideHides(t *testing.T) { + v := viper.New() + v.Set("workspace.id", "persisted-team-id") + v.Set("workspace.name", "persisted-team") + v.Set("workspace.kind", zcontext.WorkspaceKindTeam) + v.Set("context.project.id", "pinned-project") + v.Set("context.project.name", "pinned-project-name") + v.Set("context.environment.id", "pinned-env") + v.Set("context.service.id", "pinned-service") + cfg := stubConfig{ctx: zcontext.NewViperContext(v)} + f := &cmdutil.Factory{Config: cfg} + + // Without override: inner context is observable (back-compat). + if got := f.CurrentProjectID(); got != "pinned-project" { + t.Errorf("no override: CurrentProjectID = %q, want pinned-project", got) + } + if got := f.CurrentProjectName(); got != "pinned-project-name" { + t.Errorf("no override: CurrentProjectName = %q, want pinned-project-name", got) + } + if got := f.CurrentEnvironmentID(); got != "pinned-env" { + t.Errorf("no override: CurrentEnvironmentID = %q, want pinned-env", got) + } + if got := f.CurrentServiceID(); got != "pinned-service" { + t.Errorf("no override: CurrentServiceID = %q, want pinned-service", got) + } + + // With override: every inner-context helper returns "" so name-based + // downstream lookups fail-closed with an actionable error. + f.SetWorkspaceOverride(&zcontext.Workspace{ + ID: "override-id", Name: "override-team", Kind: zcontext.WorkspaceKindTeam, + }) + for _, tc := range []struct { + name string + got string + }{ + {"CurrentProjectID", f.CurrentProjectID()}, + {"CurrentProjectName", f.CurrentProjectName()}, + {"CurrentEnvironmentID", f.CurrentEnvironmentID()}, + {"CurrentServiceID", f.CurrentServiceID()}, + } { + if tc.got != "" { + t.Errorf("override active: %s = %q, want empty (B+ stateless override)", tc.name, tc.got) + } + } + + // CurrentOwnerID still returns the override (verified elsewhere); inner + // context returns empty. The mismatch is intentional — inner context + // without a known scope is the bug we're guarding against. + if got := f.CurrentOwnerID(); got != "override-id" { + t.Errorf("override active: CurrentOwnerID = %q, want override-id", got) + } + + // Clearing the override restores the inner context. + f.SetWorkspaceOverride(nil) + if got := f.CurrentProjectID(); got != "pinned-project" { + t.Errorf("after clear: CurrentProjectID = %q, want pinned-project", got) + } +} + +// TestFactory_CurrentInnerContext_NilConfigSafe: helpers must not panic on +// a Factory with no Config (e.g. the brand-new user shape from +// TestFactory_PersonalUserInvariant). +func TestFactory_CurrentInnerContext_NilConfigSafe(t *testing.T) { + f := &cmdutil.Factory{} + for _, tc := range []struct { + name string + got string + }{ + {"CurrentProjectID", f.CurrentProjectID()}, + {"CurrentProjectName", f.CurrentProjectName()}, + {"CurrentEnvironmentID", f.CurrentEnvironmentID()}, + {"CurrentServiceID", f.CurrentServiceID()}, + } { + if tc.got != "" { + t.Errorf("nil Config: %s = %q, want empty", tc.name, tc.got) + } + } +} + // fakeListTeamsAPI counts ListTeams invocations so the cache tests can // assert how many backend round-trips the Factory makes. type fakeListTeamsAPI struct { From 7a1be0ebe4bd618f7789d902daf98fac299b3190 Mon Sep 17 00:00:00 2001 From: Bruce Du Date: Mon, 25 May 2026 15:11:13 +0800 Subject: [PATCH 07/10] fix: route interactive + implicit-context consumers through ephemeral context (PLA-1590) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex's round-5 review showed B+ (commit b5667e2) only covered half the surface: my new helpers (CurrentProjectID etc.) plus a few skip-write guards in project create / deploy. Two whole entry classes still walked straight past them. Path 1 — interactive ParamFiller bypass: $ zeabur workspace switch team-A $ zeabur context set project --id $ zeabur --workspace team-B service restart # interactive (default) `runRestartInteractive` passed the raw `f.Config.GetContext()` to ParamFiller.ServiceByNameWithEnvironment. The filler read team-A's pinned project ID as `projectCtx.GetProject().GetID()`, used it as the project scope for SelectService, and quietly restarted team-A's service while the user thought they were operating on team-B. Worse, when the persisted context was empty the filler called `projectCtx.SetProject(picked)` and *wrote* the team-B project under the persisted team-A workspace — cross-team contamination that survived the command. Same shape across service/{restart,delete,suspend,exec,network,port-forward,redeploy, instruction,metric,get,update/tag}, variable/{create,delete,env,list,update}, deployment/{get,log,list}, domain/{create,delete,list}. Path 2 — `project get/delete` PreRunE auto-fill: $ zeabur --workspace team-B project delete --yes -i=false # no --id `util.DefaultIDNameByContext(f.Config.GetContext().GetProject(), ...)` was bound at Cobra command-construction time and unconditionally copied team-A's persisted project ID into opts.id. The runE then deleted team-A's project, even though every flag on the line said team-B. Fix: ephemeral context + EffectiveContext audit. 1. New `zcontext.NewEphemeralContext(workspace)` — an in-memory Context implementation that starts empty, writes to memory only, and reports the supplied workspace via GetWorkspace() (so consumers that ask "what workspace is this context for?" get the override answer rather than personal — a second-order trap Codex flagged). 2. New `Factory.EffectiveContext() zcontext.Context`: - Without override: returns the persisted config context, byte- equivalent to today. - Under override: lazy-initialises a per-Factory ephemeral context and returns it for every subsequent call (so ParamFiller's `Set → later Get` cycle still works in-process; nothing leaks to disk). 3. 24 interactive callers swap `f.Config.GetContext()` for `f.EffectiveContext()`: every service/, variable/, deployment/, domain/ command that ran ParamFiller. 4. project get/delete PreRunE swap to a lazy closure so EffectiveContext is resolved at PreRunE time (after PersistentPreRunE parses `--workspace`), not at command construction. The signature of util.DefaultIDNameByContext changes from `BasicInfo` to `func() BasicInfo` to make that lazy evaluation explicit. 5. `context get` reads EffectiveContext and, under override, prints an extra "Note: --workspace is one-shot; persisted ... is not used" line for human-readable output. JSON output stays structurally identical so scripts keep parsing. 6. `context clear` rejects under override — clearing belongs to the persisted state, not a one-shot override. 7. Deleted internal/util.NeedProjectContextWhenNonInteractive and DefaultIDByContext — both had zero callers and would re-introduce the same pattern if revived from copy/paste later. What stays on `f.Config.GetContext()` (intentional): workspace/{switch,clear}, auth/logout, root.go's lazy workspace verify, context/{set,clear} command bodies (set already rejects override; clear now does too), project create / deploy interactive flows (already wrapped in `if !HasWorkspaceOverride` from b5667e2 with a user-visible hint). Tests: zcontext/ephemeral_test.go (new): TestEphemeralContext_WorkspaceFromConstructor TestEphemeralContext_NilWorkspaceIsPersonal TestEphemeralContext_ReadEmptyByDefault TestEphemeralContext_SetReadCycleWorksInMemory TestEphemeralContext_ClearAll cmdutil/factory_test.go (added): TestFactory_EffectiveContext_NoOverride TestFactory_EffectiveContext_OverrideReturnsEphemeral_WithWorkspace TestFactory_EffectiveContext_OverrideCachedWithinCommand TestFactory_EffectiveContext_OverridePersistedUnpolluted Dev-2 E2E (this round): C1: --workspace team-B service delete --name redis (interactive, persisted=team-A + pinned project) → opens Select project from team-B; for-clone-test redis NOT deleted (verified via list). C3: --workspace team-B project delete --yes -i=false (no --id) → "please specify project by --name or --id"; for-clone-test project NOT deleted. C4: --workspace team-B context get → all inner context shows "" + the Note line; --json stays structurally clean. C5: --workspace team-B context clear → rejected with actionable hint pointing at workspace switch. C6: no override → context get reads persisted normally (back-compat red line). Co-Authored-By: Claude Opus 4.7 (1M context) --- internal/cmd/context/clear/clear.go | 14 +++ internal/cmd/context/get/get.go | 19 +++- internal/cmd/deployment/get/get.go | 2 +- internal/cmd/deployment/list/list.go | 2 +- internal/cmd/deployment/log/log.go | 2 +- internal/cmd/domain/create/create.go | 2 +- internal/cmd/domain/delete/delete.go | 2 +- internal/cmd/domain/list/list.go | 2 +- internal/cmd/project/delete/delete.go | 10 +- internal/cmd/project/get/get.go | 10 +- internal/cmd/service/delete/delete.go | 2 +- internal/cmd/service/exec/exec.go | 2 +- internal/cmd/service/get/get.go | 2 +- .../cmd/service/instruction/instruction.go | 2 +- internal/cmd/service/metric/metric.go | 2 +- internal/cmd/service/network/network.go | 2 +- .../cmd/service/port-forward/port_forward.go | 2 +- internal/cmd/service/redeploy/redeploy.go | 2 +- internal/cmd/service/restart/restart.go | 2 +- internal/cmd/service/suspend/suspend.go | 2 +- internal/cmd/service/update/tag/tag.go | 2 +- internal/cmd/variable/create/create.go | 2 +- internal/cmd/variable/delete/delete.go | 2 +- internal/cmd/variable/env/env.go | 2 +- internal/cmd/variable/list/list.go | 2 +- internal/cmd/variable/update/update.go | 2 +- internal/cmdutil/factory.go | 40 +++++++ internal/cmdutil/factory_test.go | 89 +++++++++++++++ internal/util/runE.go | 41 +++---- pkg/zcontext/ephemeral.go | 102 ++++++++++++++++++ pkg/zcontext/ephemeral_test.go | 99 +++++++++++++++++ 31 files changed, 413 insertions(+), 55 deletions(-) create mode 100644 pkg/zcontext/ephemeral.go create mode 100644 pkg/zcontext/ephemeral_test.go diff --git a/internal/cmd/context/clear/clear.go b/internal/cmd/context/clear/clear.go index 81ee57f..8ed25c4 100644 --- a/internal/cmd/context/clear/clear.go +++ b/internal/cmd/context/clear/clear.go @@ -1,6 +1,8 @@ package clear import ( + "fmt" + "github.com/spf13/cobra" "github.com/zeabur/cli/internal/cmdutil" @@ -24,6 +26,18 @@ func NewCmdClear(f *cmdutil.Factory) *cobra.Command { } func runClear(f *cmdutil.Factory, opts *Options) error { + // `context clear` modifies the persisted inner context. Under a + // `--workspace` override the persisted state belongs to a (potentially) + // different workspace than the user thinks they're in, so silently + // clearing it would surprise them. Reject up front and tell them to + // switch first if they really mean to wipe the persisted context + // (PLA-1590 B+). + if f.HasWorkspaceOverride() { + return fmt.Errorf( + "`context clear` cannot be combined with `--workspace`; the override does not modify persisted context", + ) + } + confirm := true if f.Interactive { diff --git a/internal/cmd/context/get/get.go b/internal/cmd/context/get/get.go index 949cc14..4b31053 100644 --- a/internal/cmd/context/get/get.go +++ b/internal/cmd/context/get/get.go @@ -23,9 +23,14 @@ func NewCmdGet(f *cmdutil.Factory) *cobra.Command { } func runGet(f *cmdutil.Factory, opts *Options) error { - project := f.Config.GetContext().GetProject() - environment := f.Config.GetContext().GetEnvironment() - service := f.Config.GetContext().GetService() + // Use the effective context so `--workspace` override truthfully shows + // "(not set)" for inner context — displaying the persisted team-A + // project under an override to team-B would mislead the user into + // thinking it's available there (PLA-1590 B+). + ctx := f.EffectiveContext() + project := ctx.GetProject() + environment := ctx.GetEnvironment() + service := ctx.GetService() header := []string{"Context", "Name", "ID"} data := [][]string{ @@ -56,5 +61,13 @@ func runGet(f *cmdutil.Factory, opts *Options) error { f.Printer.Table(header, data) + // Human-readable mode also tells the user *why* everything is unset when + // they're running under a `--workspace` override, so they don't + // misread it as a config bug. JSON mode stays structurally clean + // (no prose mixed into the payload) so scripts keep parsing it. + if f.HasWorkspaceOverride() { + f.Log.Info("Note: --workspace is one-shot; persisted project/service/environment context is not used.") + } + return nil } diff --git a/internal/cmd/deployment/get/get.go b/internal/cmd/deployment/get/get.go index 066d6c2..e898827 100644 --- a/internal/cmd/deployment/get/get.go +++ b/internal/cmd/deployment/get/get.go @@ -49,7 +49,7 @@ func runGet(f *cmdutil.Factory, opts *Options) error { func runGetInteractive(f *cmdutil.Factory, opts *Options) error { if opts.deploymentID == "" { - zctx := f.Config.GetContext() + zctx := f.EffectiveContext() _, err := f.ParamFiller.ServiceByNameWithEnvironment(fill.ServiceByNameWithEnvironmentOptions{ ProjectCtx: zctx, ServiceID: &opts.serviceID, diff --git a/internal/cmd/deployment/list/list.go b/internal/cmd/deployment/list/list.go index b0af4ff..17e7d98 100644 --- a/internal/cmd/deployment/list/list.go +++ b/internal/cmd/deployment/list/list.go @@ -45,7 +45,7 @@ func runList(f *cmdutil.Factory, opts *Options) error { } func runListInteractive(f *cmdutil.Factory, opts *Options) error { - zctx := f.Config.GetContext() + zctx := f.EffectiveContext() _, err := f.ParamFiller.ServiceByNameWithEnvironment(fill.ServiceByNameWithEnvironmentOptions{ ProjectCtx: zctx, ServiceID: &opts.serviceID, diff --git a/internal/cmd/deployment/log/log.go b/internal/cmd/deployment/log/log.go index 142a7d5..3c81de3 100644 --- a/internal/cmd/deployment/log/log.go +++ b/internal/cmd/deployment/log/log.go @@ -61,7 +61,7 @@ func runLog(f *cmdutil.Factory, opts *Options) error { func runLogInteractive(f *cmdutil.Factory, opts *Options) error { if opts.deploymentID == "" { - zctx := f.Config.GetContext() + zctx := f.EffectiveContext() _, err := f.ParamFiller.ServiceByNameWithEnvironment(fill.ServiceByNameWithEnvironmentOptions{ ProjectCtx: zctx, ServiceID: &opts.serviceID, diff --git a/internal/cmd/domain/create/create.go b/internal/cmd/domain/create/create.go index 16b909c..208a90b 100644 --- a/internal/cmd/domain/create/create.go +++ b/internal/cmd/domain/create/create.go @@ -52,7 +52,7 @@ func runCreateDomain(f *cmdutil.Factory, opts *Options) error { } func runCreateDomainInteractive(f *cmdutil.Factory, opts *Options) error { - zctx := f.Config.GetContext() + zctx := f.EffectiveContext() if _, err := f.ParamFiller.ServiceByNameWithEnvironment(fill.ServiceByNameWithEnvironmentOptions{ ProjectCtx: zctx, diff --git a/internal/cmd/domain/delete/delete.go b/internal/cmd/domain/delete/delete.go index b38fbba..454f3a5 100644 --- a/internal/cmd/domain/delete/delete.go +++ b/internal/cmd/domain/delete/delete.go @@ -50,7 +50,7 @@ func runDeleteDomain(f *cmdutil.Factory, opts *Options) error { } func runDeleteDomainInteractive(f *cmdutil.Factory, opts *Options) error { - zctx := f.Config.GetContext() + zctx := f.EffectiveContext() if _, err := f.ParamFiller.ServiceByNameWithEnvironment(fill.ServiceByNameWithEnvironmentOptions{ ProjectCtx: zctx, diff --git a/internal/cmd/domain/list/list.go b/internal/cmd/domain/list/list.go index c5975db..2829be2 100644 --- a/internal/cmd/domain/list/list.go +++ b/internal/cmd/domain/list/list.go @@ -47,7 +47,7 @@ func runListDomains(f *cmdutil.Factory, opts *Options) error { } func runListDomainsInteractive(f *cmdutil.Factory, opts *Options) error { - zctx := f.Config.GetContext() + zctx := f.EffectiveContext() if _, err := f.ParamFiller.ServiceByNameWithEnvironment(fill.ServiceByNameWithEnvironmentOptions{ ProjectCtx: zctx, diff --git a/internal/cmd/project/delete/delete.go b/internal/cmd/project/delete/delete.go index 48b56a0..21e56a1 100644 --- a/internal/cmd/project/delete/delete.go +++ b/internal/cmd/project/delete/delete.go @@ -8,6 +8,7 @@ import ( "github.com/zeabur/cli/internal/cmdutil" "github.com/zeabur/cli/internal/util" "github.com/zeabur/cli/pkg/model" + "github.com/zeabur/cli/pkg/zcontext" ) type Options struct { @@ -23,7 +24,14 @@ func NewCmdDelete(f *cmdutil.Factory) *cobra.Command { Use: "delete", Short: "Delete project", Aliases: []string{"del"}, - PreRunE: util.DefaultIDNameByContext(f.Config.GetContext().GetProject(), &opts.id, &opts.name), + // Closure (not direct call) so EffectiveContext is resolved at + // PreRunE time — after PersistentPreRunE has parsed `--workspace` + // — instead of at Cobra construction time when the override flag + // has not yet been seen (PLA-1590 B+). + PreRunE: util.DefaultIDNameByContext( + func() zcontext.BasicInfo { return f.EffectiveContext().GetProject() }, + &opts.id, &opts.name, + ), RunE: func(cmd *cobra.Command, args []string) error { return runDelete(f, opts) }, diff --git a/internal/cmd/project/get/get.go b/internal/cmd/project/get/get.go index 1c3a6b2..8e05f50 100644 --- a/internal/cmd/project/get/get.go +++ b/internal/cmd/project/get/get.go @@ -9,6 +9,7 @@ import ( "github.com/zeabur/cli/internal/cmdutil" "github.com/zeabur/cli/pkg/model" + "github.com/zeabur/cli/pkg/zcontext" ) type Options struct { @@ -23,7 +24,14 @@ func NewCmdGet(f *cmdutil.Factory) *cobra.Command { Use: "get", Short: "Get project", Long: "Get project, use --id or --name to specify the project", - PreRunE: util.DefaultIDNameByContext(f.Config.GetContext().GetProject(), &opts.id, &opts.name), + // Closure (not direct call) so EffectiveContext is resolved at + // PreRunE time — after PersistentPreRunE has parsed `--workspace` + // — instead of at Cobra construction time when the override flag + // has not yet been seen (PLA-1590 B+). + PreRunE: util.DefaultIDNameByContext( + func() zcontext.BasicInfo { return f.EffectiveContext().GetProject() }, + &opts.id, &opts.name, + ), RunE: func(cmd *cobra.Command, args []string) error { return runGet(f, opts) }, diff --git a/internal/cmd/service/delete/delete.go b/internal/cmd/service/delete/delete.go index 032d5f5..b2b3834 100644 --- a/internal/cmd/service/delete/delete.go +++ b/internal/cmd/service/delete/delete.go @@ -42,7 +42,7 @@ func runDelete(f *cmdutil.Factory, opts *Options) error { } func runDeleteInteractive(f *cmdutil.Factory, opts *Options) error { - zctx := f.Config.GetContext() + zctx := f.EffectiveContext() if _, err := f.ParamFiller.ServiceByName(fill.ServiceByNameOptions{ ProjectCtx: zctx, diff --git a/internal/cmd/service/exec/exec.go b/internal/cmd/service/exec/exec.go index 59ea608..511ffd9 100644 --- a/internal/cmd/service/exec/exec.go +++ b/internal/cmd/service/exec/exec.go @@ -59,7 +59,7 @@ func runExec(f *cmdutil.Factory, opts *Options) error { func runExecInteractive(f *cmdutil.Factory, opts *Options) error { if opts.id == "" && opts.name == "" { - zctx := f.Config.GetContext() + zctx := f.EffectiveContext() if _, err := f.ParamFiller.ServiceByNameWithEnvironment(fill.ServiceByNameWithEnvironmentOptions{ ProjectCtx: zctx, ServiceID: &opts.id, diff --git a/internal/cmd/service/get/get.go b/internal/cmd/service/get/get.go index 3fa13ca..43e14e2 100644 --- a/internal/cmd/service/get/get.go +++ b/internal/cmd/service/get/get.go @@ -47,7 +47,7 @@ func runGet(f *cmdutil.Factory, opts *Options) error { func runGetInteractive(f *cmdutil.Factory, opts *Options) error { if _, err := f.ParamFiller.ServiceByName(fill.ServiceByNameOptions{ - ProjectCtx: f.Config.GetContext(), + ProjectCtx: f.EffectiveContext(), ServiceID: &opts.id, ServiceName: &opts.name, }); err != nil { diff --git a/internal/cmd/service/instruction/instruction.go b/internal/cmd/service/instruction/instruction.go index c11d8e6..6cf7a2c 100644 --- a/internal/cmd/service/instruction/instruction.go +++ b/internal/cmd/service/instruction/instruction.go @@ -37,7 +37,7 @@ func NewCmdInstruction(f *cmdutil.Factory) *cobra.Command { func runInstruction(f *cmdutil.Factory, opts *Options) error { if f.Interactive && opts.id == "" && opts.name == "" { - zctx := f.Config.GetContext() + zctx := f.EffectiveContext() if _, err := f.ParamFiller.ServiceByNameWithEnvironment(fill.ServiceByNameWithEnvironmentOptions{ ProjectCtx: zctx, ServiceID: &opts.id, diff --git a/internal/cmd/service/metric/metric.go b/internal/cmd/service/metric/metric.go index 010c593..947c41a 100644 --- a/internal/cmd/service/metric/metric.go +++ b/internal/cmd/service/metric/metric.go @@ -57,7 +57,7 @@ func runMetric(f *cmdutil.Factory, opts *Options) error { } func runMetricInteractive(f *cmdutil.Factory, opts *Options) error { - zctx := f.Config.GetContext() + zctx := f.EffectiveContext() if _, err := f.ParamFiller.ServiceByNameWithEnvironment(fill.ServiceByNameWithEnvironmentOptions{ ProjectCtx: zctx, ServiceID: &opts.id, diff --git a/internal/cmd/service/network/network.go b/internal/cmd/service/network/network.go index 746affb..1f6714d 100644 --- a/internal/cmd/service/network/network.go +++ b/internal/cmd/service/network/network.go @@ -39,7 +39,7 @@ func NewCmdPrivateNetwork(f *cmdutil.Factory) *cobra.Command { func runNetwork(f *cmdutil.Factory, opts *Options) error { if f.Interactive && opts.id == "" && opts.name == "" { - zctx := f.Config.GetContext() + zctx := f.EffectiveContext() if _, err := f.ParamFiller.ServiceByNameWithEnvironment(fill.ServiceByNameWithEnvironmentOptions{ ProjectCtx: zctx, ServiceID: &opts.id, diff --git a/internal/cmd/service/port-forward/port_forward.go b/internal/cmd/service/port-forward/port_forward.go index c3d212f..81d1024 100644 --- a/internal/cmd/service/port-forward/port_forward.go +++ b/internal/cmd/service/port-forward/port_forward.go @@ -61,7 +61,7 @@ func runPortForward(f *cmdutil.Factory, opts *Options) error { func runPortForwardInteractive(f *cmdutil.Factory, opts *Options) error { if opts.id == "" && opts.name == "" { - zctx := f.Config.GetContext() + zctx := f.EffectiveContext() if _, err := f.ParamFiller.ServiceByNameWithEnvironment(fill.ServiceByNameWithEnvironmentOptions{ ProjectCtx: zctx, ServiceID: &opts.id, diff --git a/internal/cmd/service/redeploy/redeploy.go b/internal/cmd/service/redeploy/redeploy.go index 43142af..ebb7fb4 100644 --- a/internal/cmd/service/redeploy/redeploy.go +++ b/internal/cmd/service/redeploy/redeploy.go @@ -46,7 +46,7 @@ func runRedeploy(f *cmdutil.Factory, opts *Options) error { } func runRedeployInteractive(f *cmdutil.Factory, opts *Options) error { - zctx := f.Config.GetContext() + zctx := f.EffectiveContext() if _, err := f.ParamFiller.ServiceByNameWithEnvironment(fill.ServiceByNameWithEnvironmentOptions{ ProjectCtx: zctx, diff --git a/internal/cmd/service/restart/restart.go b/internal/cmd/service/restart/restart.go index 07c6ea2..f590f94 100644 --- a/internal/cmd/service/restart/restart.go +++ b/internal/cmd/service/restart/restart.go @@ -46,7 +46,7 @@ func runRestart(f *cmdutil.Factory, opts *Options) error { } func runRestartInteractive(f *cmdutil.Factory, opts *Options) error { - zctx := f.Config.GetContext() + zctx := f.EffectiveContext() if _, err := f.ParamFiller.ServiceByNameWithEnvironment(fill.ServiceByNameWithEnvironmentOptions{ ProjectCtx: zctx, diff --git a/internal/cmd/service/suspend/suspend.go b/internal/cmd/service/suspend/suspend.go index 9a60e4c..42a4cae 100644 --- a/internal/cmd/service/suspend/suspend.go +++ b/internal/cmd/service/suspend/suspend.go @@ -46,7 +46,7 @@ func runSuspend(f *cmdutil.Factory, opts *Options) error { } func runSuspendInteractive(f *cmdutil.Factory, opts *Options) error { - zctx := f.Config.GetContext() + zctx := f.EffectiveContext() if _, err := f.ParamFiller.ServiceByNameWithEnvironment(fill.ServiceByNameWithEnvironmentOptions{ ProjectCtx: zctx, diff --git a/internal/cmd/service/update/tag/tag.go b/internal/cmd/service/update/tag/tag.go index ec59abe..b314fdd 100644 --- a/internal/cmd/service/update/tag/tag.go +++ b/internal/cmd/service/update/tag/tag.go @@ -50,7 +50,7 @@ func runUpdate(f *cmdutil.Factory, opts *Options) error { } func runInteractive(f *cmdutil.Factory, opts *Options) error { - zctx := f.Config.GetContext() + zctx := f.EffectiveContext() if _, err := f.ParamFiller.ServiceByNameWithEnvironment(fill.ServiceByNameWithEnvironmentOptions{ ProjectCtx: zctx, diff --git a/internal/cmd/variable/create/create.go b/internal/cmd/variable/create/create.go index 267bd45..59cdd13 100644 --- a/internal/cmd/variable/create/create.go +++ b/internal/cmd/variable/create/create.go @@ -50,7 +50,7 @@ func runCreateVariable(f *cmdutil.Factory, opts *Options) error { } func runCreateVariableInteractive(f *cmdutil.Factory, opts *Options) error { - zctx := f.Config.GetContext() + zctx := f.EffectiveContext() if _, err := f.ParamFiller.ServiceByNameWithEnvironment(fill.ServiceByNameWithEnvironmentOptions{ ProjectCtx: zctx, diff --git a/internal/cmd/variable/delete/delete.go b/internal/cmd/variable/delete/delete.go index 4e98f3c..cad0b80 100644 --- a/internal/cmd/variable/delete/delete.go +++ b/internal/cmd/variable/delete/delete.go @@ -53,7 +53,7 @@ func runDeleteVariable(f *cmdutil.Factory, opts *Options) error { } func runDeleteVariableInteractive(f *cmdutil.Factory, opts *Options) error { - zctx := f.Config.GetContext() + zctx := f.EffectiveContext() if _, err := f.ParamFiller.ServiceByNameWithEnvironment(fill.ServiceByNameWithEnvironmentOptions{ ProjectCtx: zctx, diff --git a/internal/cmd/variable/env/env.go b/internal/cmd/variable/env/env.go index 19e50fc..883615d 100644 --- a/internal/cmd/variable/env/env.go +++ b/internal/cmd/variable/env/env.go @@ -49,7 +49,7 @@ func runUpdateVariableByEnv(f *cmdutil.Factory, opts *Options) error { } if f.Interactive && opts.id == "" && opts.name == "" { - zctx := f.Config.GetContext() + zctx := f.EffectiveContext() if _, err := f.ParamFiller.ServiceByNameWithEnvironment(fill.ServiceByNameWithEnvironmentOptions{ ProjectCtx: zctx, ServiceID: &opts.id, diff --git a/internal/cmd/variable/list/list.go b/internal/cmd/variable/list/list.go index 69adfe9..509b78d 100644 --- a/internal/cmd/variable/list/list.go +++ b/internal/cmd/variable/list/list.go @@ -46,7 +46,7 @@ func runListVariables(f *cmdutil.Factory, opts *Options) error { } func runListVariablesInteractive(f *cmdutil.Factory, opts *Options) error { - zctx := f.Config.GetContext() + zctx := f.EffectiveContext() if _, err := f.ParamFiller.ServiceByNameWithEnvironment(fill.ServiceByNameWithEnvironmentOptions{ ProjectCtx: zctx, diff --git a/internal/cmd/variable/update/update.go b/internal/cmd/variable/update/update.go index e422560..8e8b07f 100644 --- a/internal/cmd/variable/update/update.go +++ b/internal/cmd/variable/update/update.go @@ -59,7 +59,7 @@ func runUpdateVariable(f *cmdutil.Factory, opts *Options) error { } func runUpdateVariableInteractive(f *cmdutil.Factory, opts *Options) error { - zctx := f.Config.GetContext() + zctx := f.EffectiveContext() if _, err := f.ParamFiller.ServiceByNameWithEnvironment(fill.ServiceByNameWithEnvironmentOptions{ ProjectCtx: zctx, diff --git a/internal/cmdutil/factory.go b/internal/cmdutil/factory.go index 02e9e3b..1ab737f 100644 --- a/internal/cmdutil/factory.go +++ b/internal/cmdutil/factory.go @@ -47,6 +47,14 @@ type ( teamsCache []model.Team teamsCacheErr error teamsCacheHit bool + + // ephemeralCtx is the in-memory inner context handed out by + // EffectiveContext under --workspace override. Lazy-initialised on + // first call so the same instance is shared across every + // `Set -> later GetProject` cycle within one command (ParamFiller + // depends on this). Reset between commands by virtue of being a + // per-Factory field; Factory itself is per-invocation. + ephemeralCtx zcontext.Context } // PersistentFlags are flags that are common to all commands PersistentFlags struct { @@ -155,6 +163,38 @@ func (f *Factory) CurrentServiceID() string { return f.Config.GetContext().GetService().GetID() } +// EffectiveContext returns the inner context (project / environment / +// service) that the current command should consume. Without an override +// this is the persisted config context, byte-equivalent to reading +// `f.Config.GetContext()` directly. Under `--workspace` override it +// returns an in-memory ephemeral context whose initial reads are empty +// and whose writes never reach the config file — so an interactive command +// can transiently pick a team-B project / service without polluting the +// persisted team-A context (PLA-1590 B++). +// +// The ephemeral instance is cached on the Factory so that within a single +// command, `Set → later Get` cycles inside ParamFiller see the values they +// just wrote. The cache is implicitly scoped to one invocation because the +// Factory itself is constructed once per command. +// +// Callers that intentionally manipulate persisted workspace state +// (`workspace switch`, `workspace clear`, lazy verify in root, `auth +// logout`) must keep going through `f.Config.GetContext()` directly — +// EffectiveContext is for inner context only and would silently no-op +// their writes. +func (f *Factory) EffectiveContext() zcontext.Context { + if !f.HasWorkspaceOverride() { + if f.Config == nil { + return zcontext.NewEphemeralContext(nil) + } + return f.Config.GetContext() + } + if f.ephemeralCtx == nil { + f.ephemeralCtx = zcontext.NewEphemeralContext(f.CurrentWorkspace()) + } + return f.ephemeralCtx +} + // ListTeams returns the caller's teams via api.Client.ListTeams, memoized for // the lifetime of this Factory. The same Factory is shared across every // PersistentPreRunE / Run / PersistentPostRunE callback within a single CLI diff --git a/internal/cmdutil/factory_test.go b/internal/cmdutil/factory_test.go index 11ab78a..c0ce0f3 100644 --- a/internal/cmdutil/factory_test.go +++ b/internal/cmdutil/factory_test.go @@ -226,6 +226,95 @@ func TestFactory_CurrentInnerContext_NilConfigSafe(t *testing.T) { } } +// TestFactory_EffectiveContext_NoOverride: without override, EffectiveContext +// returns the persisted config context. This is the back-compat path — +// vanilla users (no --workspace flag) see no behaviour change. +func TestFactory_EffectiveContext_NoOverride(t *testing.T) { + v := viper.New() + v.Set("context.project.id", "pinned-project") + v.Set("context.project.name", "pinned-project-name") + cfg := stubConfig{ctx: zcontext.NewViperContext(v)} + f := &cmdutil.Factory{Config: cfg} + + got := f.EffectiveContext().GetProject() + if got.GetID() != "pinned-project" || got.GetName() != "pinned-project-name" { + t.Fatalf("got %+v, want persisted project pass-through", got) + } +} + +// TestFactory_EffectiveContext_OverrideReturnsEphemeral_WithWorkspace: the +// override case returns an ephemeral context whose GetWorkspace() reports +// the override workspace (NOT personal — that would be a second-order trap +// for code that reads ctx.GetWorkspace()). +func TestFactory_EffectiveContext_OverrideReturnsEphemeral_WithWorkspace(t *testing.T) { + v := viper.New() + v.Set("workspace.id", "persisted-team-id") + v.Set("context.project.id", "persisted-project") + cfg := stubConfig{ctx: zcontext.NewViperContext(v)} + f := &cmdutil.Factory{Config: cfg} + + overrideWS := &zcontext.Workspace{ID: "override-team-id", Name: "override-team", Kind: zcontext.WorkspaceKindTeam} + f.SetWorkspaceOverride(overrideWS) + + ctx := f.EffectiveContext() + // Inner context: starts empty (no persisted leak). + if !ctx.GetProject().Empty() { + t.Errorf("under override, GetProject must start empty (persisted must not leak), got id=%q", ctx.GetProject().GetID()) + } + // Workspace: reports the override, not personal. + if got := ctx.GetWorkspace(); got.ID != overrideWS.ID { + t.Errorf("ephemeral GetWorkspace = %+v, want override %+v", got, overrideWS) + } +} + +// TestFactory_EffectiveContext_OverrideCachedWithinCommand: the ephemeral +// context must be the SAME instance across calls within one Factory's +// lifetime — ParamFiller's `Set → later Get` flow depends on it. Otherwise +// the second call would get a fresh empty context and lose the project +// the first call wrote. +func TestFactory_EffectiveContext_OverrideCachedWithinCommand(t *testing.T) { + cfg := stubConfig{ctx: zcontext.NewViperContext(viper.New())} + f := &cmdutil.Factory{Config: cfg} + f.SetWorkspaceOverride(&zcontext.Workspace{ID: "x", Kind: zcontext.WorkspaceKindTeam}) + + first := f.EffectiveContext() + first.SetProject(zcontext.NewBasicInfo("set-during-cycle", "name")) + + second := f.EffectiveContext() + if got := second.GetProject(); got.GetID() != "set-during-cycle" { + t.Errorf("second EffectiveContext() must see writes from the first; got %+v", got) + } +} + +// TestFactory_EffectiveContext_OverridePersistedUnpolluted: the ephemeral +// context's writes must not reach the underlying persisted config. This is +// the whole point of B+: --workspace doesn't modify state. +func TestFactory_EffectiveContext_OverridePersistedUnpolluted(t *testing.T) { + v := viper.New() + v.Set("context.project.id", "persisted-X") + v.Set("context.project.name", "persisted-X-name") + cfg := stubConfig{ctx: zcontext.NewViperContext(v)} + f := &cmdutil.Factory{Config: cfg} + f.SetWorkspaceOverride(&zcontext.Workspace{ID: "team-B", Kind: zcontext.WorkspaceKindTeam}) + + // Simulate ParamFiller deciding to "remember" a fresh project. + f.EffectiveContext().SetProject(zcontext.NewBasicInfo("team-B-project", "tb-name")) + + // Persisted viper must still say persisted-X. + if got := v.GetString("context.project.id"); got != "persisted-X" { + t.Errorf("ephemeral write leaked into persisted config: id=%q", got) + } + if got := v.GetString("context.project.name"); got != "persisted-X-name" { + t.Errorf("ephemeral write leaked into persisted config: name=%q", got) + } + + // Once override is cleared, persisted state is what surfaces again. + f.SetWorkspaceOverride(nil) + if got := f.EffectiveContext().GetProject().GetID(); got != "persisted-X" { + t.Errorf("after clearing override, EffectiveContext must show persisted; got %q", got) + } +} + // fakeListTeamsAPI counts ListTeams invocations so the cache tests can // assert how many backend round-trips the Factory makes. type fakeListTeamsAPI struct { diff --git a/internal/util/runE.go b/internal/util/runE.go index 80cc04b..e3ff680 100644 --- a/internal/util/runE.go +++ b/internal/util/runE.go @@ -1,39 +1,24 @@ package util import ( - "errors" - "github.com/spf13/cobra" - "github.com/zeabur/cli/internal/cmdutil" "github.com/zeabur/cli/pkg/zcontext" ) -// NeedProjectContextWhenNonInteractive checks if the project context is set in the non-interactive mode. -// If overrideID is provided and non-empty, the check is skipped (the caller already has a project ID from a flag). -func NeedProjectContextWhenNonInteractive(f *cmdutil.Factory, overrideID ...*string) CobraRunE { - return func(cmd *cobra.Command, args []string) error { - if len(overrideID) > 0 && overrideID[0] != nil && *overrideID[0] != "" { - return nil - } - if !f.Interactive && f.Config.GetContext().GetProject().Empty() { - return errors.New("please run first") - } - return nil - } -} - -func DefaultIDNameByContext(basicInfo zcontext.BasicInfo, id, name *string) CobraRunE { - return func(cmd *cobra.Command, args []string) error { - defaultByContext(basicInfo, id, name) - return nil - } -} - -func DefaultIDByContext(basicInfo zcontext.BasicInfo, id *string) CobraRunE { - var unused string - +// DefaultIDNameByContext returns a Cobra PreRunE that auto-fills `id` and +// `name` from the resource the supplied closure returns, provided both are +// empty. Useful for commands like `project get/delete` that should default +// to "whatever the user has pinned" when invoked with no flags. +// +// The argument is a closure rather than the BasicInfo directly so the +// resolution happens at PreRunE time (after global flags are parsed), +// not at Cobra command construction time. This matters for PLA-1590: +// `--workspace` only takes effect after PersistentPreRunE runs, so a +// caller passing `f.EffectiveContext().GetProject()` directly would +// capture the persisted project, not the override-aware empty. +func DefaultIDNameByContext(basicInfoFn func() zcontext.BasicInfo, id, name *string) CobraRunE { return func(cmd *cobra.Command, args []string) error { - defaultByContext(basicInfo, id, &unused) + defaultByContext(basicInfoFn(), id, name) return nil } } diff --git a/pkg/zcontext/ephemeral.go b/pkg/zcontext/ephemeral.go new file mode 100644 index 0000000..be67d24 --- /dev/null +++ b/pkg/zcontext/ephemeral.go @@ -0,0 +1,102 @@ +package zcontext + +// EphemeralContext is an in-memory Context used under `--workspace` override +// (PLA-1590 B+). It exists so the CLI can run an interactive command that +// transiently selects a project / service / environment without ever writing +// those choices back to the persisted config — that would silently pin +// resources from the override workspace under the persisted workspace's name +// and cause cross-workspace operations on later commands. +// +// Reads start empty (no implicit fallback to persisted state) and writes go +// to in-memory fields only. ParamFiller's "Set then re-read within the same +// call" pattern still works, because reads see whatever was Set during this +// process; the values just don't survive to the next command. +// +// `GetWorkspace()` returns the workspace the caller passed at construction — +// the override workspace, not personal — so any consumer that asks "what +// workspace is this context for?" gets the right answer. That avoids a +// second-order trap where some future helper reads `ctx.GetWorkspace()` +// expecting it to match `Factory.CurrentWorkspace()` and finds a personal +// reading instead. +type ephemeralContext struct { + workspace *Workspace + project BasicInfo + environment BasicInfo + service BasicInfo +} + +// NewEphemeralContext returns a new in-memory Context whose GetWorkspace() +// reports the supplied workspace. Pass `nil` for personal (or when no +// override is active and the caller really wants a blank scratch context — +// uncommon). +func NewEphemeralContext(workspace *Workspace) Context { + if workspace == nil { + workspace = &Workspace{} + } + return &ephemeralContext{ + workspace: workspace, + project: &basicInfo{}, + environment: &basicInfo{}, + service: &basicInfo{}, + } +} + +func (c *ephemeralContext) GetWorkspace() *Workspace { + return c.workspace +} + +func (c *ephemeralContext) SetWorkspace(workspace *Workspace) { + if workspace == nil { + c.workspace = &Workspace{} + return + } + c.workspace = workspace +} + +func (c *ephemeralContext) ClearWorkspace() { + c.workspace = &Workspace{} +} + +func (c *ephemeralContext) GetProject() BasicInfo { return c.project } + +func (c *ephemeralContext) SetProject(project BasicInfo) { + if project == nil { + c.project = &basicInfo{} + return + } + c.project = project +} + +func (c *ephemeralContext) ClearProject() { c.project = &basicInfo{} } + +func (c *ephemeralContext) GetEnvironment() BasicInfo { return c.environment } + +func (c *ephemeralContext) SetEnvironment(environment BasicInfo) { + if environment == nil { + c.environment = &basicInfo{} + return + } + c.environment = environment +} + +func (c *ephemeralContext) ClearEnvironment() { c.environment = &basicInfo{} } + +func (c *ephemeralContext) GetService() BasicInfo { return c.service } + +func (c *ephemeralContext) SetService(service BasicInfo) { + if service == nil { + c.service = &basicInfo{} + return + } + c.service = service +} + +func (c *ephemeralContext) ClearService() { c.service = &basicInfo{} } + +func (c *ephemeralContext) ClearAll() { + c.ClearProject() + c.ClearEnvironment() + c.ClearService() +} + +var _ Context = &ephemeralContext{} diff --git a/pkg/zcontext/ephemeral_test.go b/pkg/zcontext/ephemeral_test.go new file mode 100644 index 0000000..1000750 --- /dev/null +++ b/pkg/zcontext/ephemeral_test.go @@ -0,0 +1,99 @@ +package zcontext_test + +import ( + "testing" + + "github.com/zeabur/cli/pkg/zcontext" +) + +// TestEphemeralContext_WorkspaceFromConstructor: the override's workspace must +// flow through GetWorkspace(). Otherwise consumers reading +// `ctx.GetWorkspace()` get "personal" under a team override — a second-order +// trap. +func TestEphemeralContext_WorkspaceFromConstructor(t *testing.T) { + ws := &zcontext.Workspace{ID: "65aa1234567890abcdef1234", Name: "acme", Kind: zcontext.WorkspaceKindTeam} + ctx := zcontext.NewEphemeralContext(ws) + got := ctx.GetWorkspace() + if got == nil || got.ID != ws.ID || got.Name != ws.Name || got.Kind != ws.Kind { + t.Fatalf("got %+v, want %+v", got, ws) + } + if !got.IsTeam() { + t.Errorf("override workspace must report IsTeam(), got %+v", got) + } +} + +// TestEphemeralContext_NilWorkspaceIsPersonal: passing nil at construction +// yields a personal-shaped workspace (zero value). Used when the caller wants +// a scratch context without a known override (uncommon). +func TestEphemeralContext_NilWorkspaceIsPersonal(t *testing.T) { + ctx := zcontext.NewEphemeralContext(nil) + ws := ctx.GetWorkspace() + if ws == nil { + t.Fatal("GetWorkspace must never return nil") + } + if !ws.IsPersonal() { + t.Errorf("nil constructor must produce personal workspace, got %+v", ws) + } +} + +// TestEphemeralContext_ReadEmptyByDefault: every inner-context getter starts +// empty. This is the core "no implicit fallback to persisted state" property: +// an interactive command running under override sees no leftovers from the +// persisted config, so it always prompts the user fresh. +func TestEphemeralContext_ReadEmptyByDefault(t *testing.T) { + ctx := zcontext.NewEphemeralContext(&zcontext.Workspace{ID: "x"}) + for _, tc := range []struct { + name string + info zcontext.BasicInfo + }{ + {"project", ctx.GetProject()}, + {"environment", ctx.GetEnvironment()}, + {"service", ctx.GetService()}, + } { + if !tc.info.Empty() { + t.Errorf("ephemeral %s must start empty, got id=%q name=%q", tc.name, tc.info.GetID(), tc.info.GetName()) + } + } +} + +// TestEphemeralContext_SetReadCycleWorksInMemory: ParamFiller's flow depends +// on `Set then later Get` returning what was just Set. This must keep +// working under override — the values just shouldn't leak to disk. +func TestEphemeralContext_SetReadCycleWorksInMemory(t *testing.T) { + ctx := zcontext.NewEphemeralContext(&zcontext.Workspace{ID: "x"}) + want := zcontext.NewBasicInfo("65aa1234567890abcdef1234", "my-project") + ctx.SetProject(want) + got := ctx.GetProject() + if got.GetID() != "65aa1234567890abcdef1234" || got.GetName() != "my-project" { + t.Fatalf("got id=%q name=%q, want SetProject value through", got.GetID(), got.GetName()) + } + // Environment / Service same shape. + ctx.SetEnvironment(zcontext.NewBasicInfo("env-id", "env-name")) + if e := ctx.GetEnvironment(); e.GetID() != "env-id" { + t.Errorf("environment round-trip lost value: %+v", e) + } + ctx.SetService(zcontext.NewBasicInfo("svc-id", "svc-name")) + if s := ctx.GetService(); s.GetID() != "svc-id" { + t.Errorf("service round-trip lost value: %+v", s) + } +} + +// TestEphemeralContext_ClearAll: ClearAll wipes the inner fields but leaves +// the workspace alone — the override workspace is the whole reason we're +// running ephemerally. +func TestEphemeralContext_ClearAll(t *testing.T) { + ws := &zcontext.Workspace{ID: "65aa1234567890abcdef1234", Name: "acme", Kind: zcontext.WorkspaceKindTeam} + ctx := zcontext.NewEphemeralContext(ws) + ctx.SetProject(zcontext.NewBasicInfo("p", "P")) + ctx.SetEnvironment(zcontext.NewBasicInfo("e", "E")) + ctx.SetService(zcontext.NewBasicInfo("s", "S")) + ctx.ClearAll() + for _, info := range []zcontext.BasicInfo{ctx.GetProject(), ctx.GetEnvironment(), ctx.GetService()} { + if !info.Empty() { + t.Errorf("after ClearAll inner context must be empty, got %+v", info) + } + } + if got := ctx.GetWorkspace(); got.ID != ws.ID { + t.Errorf("ClearAll must not touch workspace: got %+v want %+v", got, ws) + } +} From da5da8fa0b8c538ada468a6e395a370d942643e5 Mon Sep 17 00:00:00 2001 From: Bruce Du Date: Mon, 25 May 2026 15:42:23 +0800 Subject: [PATCH 08/10] test: cover lazy-resolution in DefaultIDNameByContext (PLA-1590) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex's round-5 review noted that the `project get/delete` PreRunE fix from 7a1be0e was only proven by E2E — no committed regression test guarded the lazy-closure behaviour that makes the fix work. This adds three unit tests against `util.DefaultIDNameByContext`: TestDefaultIDNameByContext_LazyEvaluation Source BasicInfo is swapped AFTER the helper is constructed but BEFORE PreRunE runs. Eager capture (the pre-fix behaviour) would leak the old value; lazy resolution must pick up the new one. This is the exact mechanism that lets PersistentPreRunE parse `--workspace` and then have project get/delete see the override. TestDefaultIDNameByContext_EmptyBasicInfoSkipsFill The override path: EffectiveContext returns an empty BasicInfo, so no auto-fill happens and the caller's runE produces the "please specify project by --name or --id" actionable error rather than silently using the persisted team-A project. TestDefaultIDNameByContext_RespectsUserFlags Back-compat: explicit --id / --name from the user is never overwritten by the context default. Unchanged from pre-PLA-1590 but worth pinning down so a future refactor doesn't regress it. No production code change in this commit. Co-Authored-By: Claude Opus 4.7 (1M context) --- internal/util/runE_test.go | 88 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 88 insertions(+) create mode 100644 internal/util/runE_test.go diff --git a/internal/util/runE_test.go b/internal/util/runE_test.go new file mode 100644 index 0000000..c2a4a8f --- /dev/null +++ b/internal/util/runE_test.go @@ -0,0 +1,88 @@ +package util_test + +import ( + "testing" + + "github.com/zeabur/cli/internal/util" + "github.com/zeabur/cli/pkg/zcontext" +) + +// TestDefaultIDNameByContext_LazyEvaluation guards the PLA-1590 invariant +// that this helper resolves its source closure at PreRunE time, not at the +// caller's call site. The two project / delete callers depend on this so +// that `f.EffectiveContext().GetProject()` reflects whatever workspace +// override `PersistentPreRunE` has resolved by the time the helper runs. +// +// The test threads through a mutable BasicInfo: we swap it AFTER calling +// DefaultIDNameByContext and BEFORE invoking the returned PreRunE. If the +// helper captured the BasicInfo eagerly (the pre-fix behaviour) the swap +// would be ignored and id/name would be filled from the original value. +func TestDefaultIDNameByContext_LazyEvaluation(t *testing.T) { + var source zcontext.BasicInfo = zcontext.NewBasicInfo("old-id", "old-name") + getter := func() zcontext.BasicInfo { return source } + + var id, name string + preRun := util.DefaultIDNameByContext(getter, &id, &name) + + // Swap the source AFTER constructing PreRunE; eager capture would miss this. + source = zcontext.NewBasicInfo("new-id", "new-name") + + if err := preRun(nil, nil); err != nil { + t.Fatalf("preRun: %v", err) + } + if id != "new-id" || name != "new-name" { + t.Fatalf("got id=%q name=%q, want new-id/new-name (lazy resolution)", id, name) + } +} + +// TestDefaultIDNameByContext_EmptyBasicInfoSkipsFill is the override case: +// when EffectiveContext returns an empty BasicInfo (because --workspace is +// active and the ephemeral context starts empty), no auto-fill happens. The +// caller's runE then has to handle missing id/name itself, which is what +// produces the "please specify project by --name or --id" actionable error +// we observed in dev-2 E2E C3. +func TestDefaultIDNameByContext_EmptyBasicInfoSkipsFill(t *testing.T) { + empty := zcontext.NewBasicInfo("", "") + getter := func() zcontext.BasicInfo { return empty } + + id, name := "", "" + preRun := util.DefaultIDNameByContext(getter, &id, &name) + if err := preRun(nil, nil); err != nil { + t.Fatalf("preRun: %v", err) + } + if id != "" || name != "" { + t.Fatalf("got id=%q name=%q, want both empty (override path)", id, name) + } +} + +// TestDefaultIDNameByContext_RespectsUserFlags: when the user has already +// passed --id or --name explicitly, the helper must not overwrite their +// value with the context default. This is unchanged from pre-PLA-1590 and +// guards the back-compat path. +func TestDefaultIDNameByContext_RespectsUserFlags(t *testing.T) { + source := zcontext.NewBasicInfo("ctx-id", "ctx-name") + getter := func() zcontext.BasicInfo { return source } + + cases := []struct { + name string + id string + nm string + }{ + {"id-only", "user-id", ""}, + {"name-only", "", "user-name"}, + {"both", "user-id", "user-name"}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + id, name := tc.id, tc.nm + preRun := util.DefaultIDNameByContext(getter, &id, &name) + if err := preRun(nil, nil); err != nil { + t.Fatalf("preRun: %v", err) + } + if id != tc.id || name != tc.nm { + t.Errorf("user flag overwritten: got id=%q name=%q, want id=%q name=%q", + id, name, tc.id, tc.nm) + } + }) + } +} From e24000d54d30879ee57ec114cd34bd6d9af0cc9b Mon Sep 17 00:00:00 2001 From: Bruce Du Date: Mon, 25 May 2026 17:08:36 +0800 Subject: [PATCH 09/10] fix: gate `context set project --id` against cross-workspace ID in team mode (PLA-1590) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex's round-6 review surfaced the last residual cross-workspace contamination path that the B+ / B++ work missed: `context set project --id ` while in a team workspace. Repro: $ zeabur workspace switch team-A $ zeabur context set project --id # SUCCEEDS today $ zeabur service delete --name redis --yes -i=false # deletes team-B's redis The team branch of util.GetServiceByName then resolves under team-A ownerID + team-B projectID and hands the resulting service-ID to the destructive call. Backend RBAC doesn't catch it (caller is a member of both teams), so the user — believing they're in team-A — silently mutates team-B resources. Pre-fix `setProject` simply called `f.ApiClient.GetProject(id, "", "")` (owner-agnostic) and pinned whatever came back. The comment even called it out as "ID path is workspace-agnostic" — accurate description of the bug. The fix is the minimal guard Codex recommended: - ID path in a team workspace: after fetching the project by ID, call `ListAllProjects(currentOwnerID)` and verify the project is in the team's set. Reject otherwise with an actionable error pointing at `workspace switch` or `--name`. - Personal workspace: unchanged. Collaborator workflows depend on pinning by-ID a project owned by someone else, and that's still fine because personal-pin doesn't have a "current workspace" the pinned project could conflict with. Implementation lives in `internal/cmd/context/set/set.go`. Adds one extra round-trip per ID-only team-workspace pin (acceptable: only fires on the explicit `context set` action, not on hot paths). Tests: TestSetProject_ID_TeamWorkspace_AllowsOwnProject legitimate same-team pin succeeds + ListAllProjects called with the correct ownerID. TestSetProject_ID_TeamWorkspace_RejectsForeignProject the exact Codex attack: cross-team --id rejected + context not contaminated. TestSetProject_ID_PersonalWorkspace_BypassesCheck back-compat: personal --id NEVER calls ListAllProjects (else collaborator pin-by-ID breaks). TestSetProject_ID_TeamWorkspace_ListErr backend list call failure propagates — must not silently "trust the user" and re-open the gap when the API flakes. All pass; full `go test ./...` + `go vet ./...` clean. Dev-2 E2E (Codex's 4 verifications): - F1.1 team-A + --id of a personal project Bruce has access to → ERROR "project ... does not belong to workspace ..."; project context still empty. - F1.2 team-A + --id of a team-A own project (for-clone-test) → pin succeeds. - F1.3 personal workspace + --id of personal project (dev-test) → back-compat: pin succeeds, no ListAllProjects call. - F1.4 after F1.1's rejection, follow-up `service delete --name redis -i=false` fails with "cannot resolve service by name ... no project context", so the destructive call never reaches the other team's redis. End-to-end verified: dev-test in personal is still present (untouched). Co-Authored-By: Claude Opus 4.7 (1M context) --- internal/cmd/context/set/set.go | 45 ++++++- internal/cmd/context/set/set_test.go | 183 +++++++++++++++++++++++++++ 2 files changed, 222 insertions(+), 6 deletions(-) create mode 100644 internal/cmd/context/set/set_test.go diff --git a/internal/cmd/context/set/set.go b/internal/cmd/context/set/set.go index 4b1777b..bb90f02 100644 --- a/internal/cmd/context/set/set.go +++ b/internal/cmd/context/set/set.go @@ -136,18 +136,51 @@ func setProject(f *cmdutil.Factory, id, name string, shouldCheck bool) error { err error ) if id != "" { - // ID path is workspace-agnostic — `project(_id)` resolves the - // owner from the project itself, so this works for both - // personal and team-owned projects. + // Backend `project(_id)` is owner-agnostic: it'll happily return a + // project from a different team as long as the caller has read + // access (e.g. they're a member of both teams). That makes + // `context set project --id ` a back-door + // cross-workspace contamination path — once pinned, subsequent + // name-based service / variable / etc. commands resolve under + // `` ownerID but `` projectID, and + // happily delete / restart the wrong team's services. + // + // For team workspaces, verify the project actually belongs to + // the current team via the owner-scoped ListAllProjects. + // Personal workspace keeps its legacy behaviour because + // collaborator workflows depend on pinning by-ID a project the + // caller doesn't own (PLA-1590 cross-workspace guard). project, err = f.ApiClient.GetProject(context.Background(), id, "", "") + if err != nil { + return fmt.Errorf("failed to get project: %w", err) + } + if ownerID := f.CurrentOwnerID(); ownerID != "" { + teamProjects, listErr := f.ApiClient.ListAllProjects(context.Background(), ownerID) + if listErr != nil { + return fmt.Errorf("verify project workspace membership: %w", listErr) + } + belongs := false + for _, p := range teamProjects { + if p.ID == id { + belongs = true + break + } + } + if !belongs { + return fmt.Errorf( + "project %q does not belong to workspace %q; either run `zeabur workspace switch ` first, or pin by --name", + project.Name, f.CurrentWorkspace().Name, + ) + } + } } else { // Name path: must respect the active workspace, otherwise a // team workspace silently looks up the project under the // caller's personal account. project, err = util.GetProjectByName(f.ApiClient, f.CurrentOwnerID(), f.Config.GetUsername(), name) - } - if err != nil { - return fmt.Errorf("failed to get project: %w", err) + if err != nil { + return fmt.Errorf("failed to get project: %w", err) + } } f.Config.GetContext().SetProject(zcontext.NewBasicInfo(project.ID, project.Name)) // User may have passed only --id; backfill the local `name` so the diff --git a/internal/cmd/context/set/set_test.go b/internal/cmd/context/set/set_test.go new file mode 100644 index 0000000..7f7e824 --- /dev/null +++ b/internal/cmd/context/set/set_test.go @@ -0,0 +1,183 @@ +package set + +import ( + "context" + "errors" + "strings" + "testing" + + "github.com/spf13/viper" + + "github.com/zeabur/cli/internal/cmdutil" + "github.com/zeabur/cli/pkg/api" + "github.com/zeabur/cli/pkg/config" + "github.com/zeabur/cli/pkg/model" + "github.com/zeabur/cli/pkg/zcontext" + "go.uber.org/zap" +) + +// stubProjectClient stubs the two ApiClient methods setProject's ID path +// actually reaches for. Other Client methods inherit the embedded nil +// interface and panic if accidentally exercised — a safety net so a future +// caller can't sneak in an unverified backend call. +type stubProjectClient struct { + api.Client + + getProjectRet *model.Project + getProjectErr error + + listAllOwner string + listAllOwnerCalled bool + listAllRet model.Projects + listAllErr error +} + +func (c *stubProjectClient) GetProject(_ context.Context, _, _, _ string) (*model.Project, error) { + return c.getProjectRet, c.getProjectErr +} + +func (c *stubProjectClient) ListAllProjects(_ context.Context, ownerID string) (model.Projects, error) { + c.listAllOwnerCalled = true + c.listAllOwner = ownerID + return c.listAllRet, c.listAllErr +} + +// stubConfig is a minimal config.Config that wraps a viper instance for the +// inner context only. setProject writes through `f.Config.GetContext()` so +// the persisted side has to be a real-ish thing. +type stubConfig struct { + v *viper.Viper +} + +func newStubConfig() *stubConfig { + return &stubConfig{v: viper.New()} +} + +func (s *stubConfig) GetTokenString() string { return "" } +func (s *stubConfig) SetTokenString(string) {} +func (s *stubConfig) GetUser() string { return "" } +func (s *stubConfig) SetUser(string) {} +func (s *stubConfig) GetUsername() string { return "alice" } +func (s *stubConfig) SetUsername(string) {} +func (s *stubConfig) GetContext() zcontext.Context { return zcontext.NewViperContext(s.v) } +func (s *stubConfig) Write() error { return nil } + +var _ config.Config = (*stubConfig)(nil) + +func newFactory(t *testing.T, apiClient api.Client, persistedWorkspace *zcontext.Workspace) (*cmdutil.Factory, *stubConfig) { + t.Helper() + cfg := newStubConfig() + if persistedWorkspace != nil { + cfg.GetContext().SetWorkspace(persistedWorkspace) + } + f := &cmdutil.Factory{ + Config: cfg, + ApiClient: apiClient, + Log: zap.NewNop().Sugar(), + } + return f, cfg +} + +// TestSetProject_ID_TeamWorkspace_AllowsOwnProject — the legitimate path. +// In a team workspace, pinning a project that *does* belong to the team +// succeeds and writes the pin to context. Critically, ListAllProjects is +// called with the current team's ID — the membership check is real. +func TestSetProject_ID_TeamWorkspace_AllowsOwnProject(t *testing.T) { + target := &model.Project{ID: "65aa1234567890abcdef1234", Name: "team-A-foo"} + stub := &stubProjectClient{ + getProjectRet: target, + listAllRet: model.Projects{target}, + } + teamA := &zcontext.Workspace{ID: "65cc1230000000000000000a", Name: "team-A", Kind: zcontext.WorkspaceKindTeam} + f, cfg := newFactory(t, stub, teamA) + + if err := setProject(f, target.ID, "", true); err != nil { + t.Fatalf("setProject: %v", err) + } + if !stub.listAllOwnerCalled || stub.listAllOwner != teamA.ID { + t.Fatalf("ListAllProjects called with owner=%q (called=%v), want %q", stub.listAllOwner, stub.listAllOwnerCalled, teamA.ID) + } + if got := cfg.GetContext().GetProject().GetID(); got != target.ID { + t.Errorf("project context = %q, want %q (legitimate pin should succeed)", got, target.ID) + } +} + +// TestSetProject_ID_TeamWorkspace_RejectsForeignProject is the Codex +// finding's exact attack: persisted workspace team-A, --id of a team-B +// project. The CLI must refuse and leave persisted context untouched — +// otherwise subsequent name-based service / variable / etc. commands +// would silently operate on team-B. +func TestSetProject_ID_TeamWorkspace_RejectsForeignProject(t *testing.T) { + teamBProject := &model.Project{ID: "65bb5678901234abcdef5678", Name: "team-B-foo"} + // ListAllProjects for team-A returns only team-A projects — the team-B + // project ID is NOT in the list. + teamAProject := &model.Project{ID: "65aa1234567890abcdef1234", Name: "team-A-only"} + stub := &stubProjectClient{ + getProjectRet: teamBProject, // GetProject by ID still returns the cross-team project (backend doesn't gate) + listAllRet: model.Projects{teamAProject}, + } + teamA := &zcontext.Workspace{ID: "65cc1230000000000000000a", Name: "team-A", Kind: zcontext.WorkspaceKindTeam} + f, cfg := newFactory(t, stub, teamA) + + err := setProject(f, teamBProject.ID, "", true) + if err == nil { + t.Fatal("setProject must refuse cross-workspace --id, got nil error") + } + if !strings.Contains(err.Error(), "does not belong to workspace") { + t.Errorf("error should explain cross-workspace mismatch, got: %v", err) + } + // Project context must be untouched. + if got := cfg.GetContext().GetProject().GetID(); got != "" { + t.Errorf("context contaminated on rejection: got project.id=%q", got) + } +} + +// TestSetProject_ID_PersonalWorkspace_BypassesCheck is the back-compat +// guard. Personal workspace must NOT call ListAllProjects — that would +// break collaborator workflows where a user pins by-ID a project owned by +// someone else (Codex's explicit non-regression). +func TestSetProject_ID_PersonalWorkspace_BypassesCheck(t *testing.T) { + collaboratorProject := &model.Project{ID: "65aa1234567890abcdef1234", Name: "shared-with-me"} + stub := &stubProjectClient{ + getProjectRet: collaboratorProject, + // listAllRet deliberately unset — if the code paths into + // ListAllProjects, the empty list would reject and the test fails. + } + // Personal workspace: persistedWorkspace nil. + f, cfg := newFactory(t, stub, nil) + + if err := setProject(f, collaboratorProject.ID, "", true); err != nil { + t.Fatalf("personal --id must not be gated, got: %v", err) + } + if stub.listAllOwnerCalled { + t.Errorf("personal --id must NOT call ListAllProjects (collaborator workflow), but it was called with owner=%q", stub.listAllOwner) + } + if got := cfg.GetContext().GetProject().GetID(); got != collaboratorProject.ID { + t.Errorf("personal pin failed: context.project.id=%q, want %q", got, collaboratorProject.ID) + } +} + +// TestSetProject_ID_TeamWorkspace_ListErr propagates: if the membership +// list call itself fails, we must NOT fall back to "trust the user" — +// that would re-open the cross-workspace gap whenever the backend is +// flaky. Surface the error. +func TestSetProject_ID_TeamWorkspace_ListErr(t *testing.T) { + target := &model.Project{ID: "65aa1234567890abcdef1234", Name: "x"} + stub := &stubProjectClient{ + getProjectRet: target, + listAllErr: errors.New("boom"), + } + teamA := &zcontext.Workspace{ID: "65cc1230000000000000000a", Name: "team-A", Kind: zcontext.WorkspaceKindTeam} + f, cfg := newFactory(t, stub, teamA) + + err := setProject(f, target.ID, "", true) + if err == nil { + t.Fatal("ListAllProjects failure must propagate, not silently pass") + } + if !strings.Contains(err.Error(), "verify project workspace membership") || !strings.Contains(err.Error(), "boom") { + t.Errorf("error should wrap the list call failure, got: %v", err) + } + if got := cfg.GetContext().GetProject().GetID(); got != "" { + t.Errorf("context contaminated despite list error: %q", got) + } +} From 02c32e45c3240258710d5bfc6997569352998ea1 Mon Sep 17 00:00:00 2001 From: Bruce Du Date: Tue, 26 May 2026 10:22:40 +0800 Subject: [PATCH 10/10] fix: address CodeRabbit lint + workspace-list / ephemeral-cache nits (PLA-1590) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three CodeRabbit findings on the PR, two of which were CI lint blockers: 1. [CI BLOCKER] runE_test.go ST1023 — drop redundant explicit type on `var source zcontext.BasicInfo = ...`; the type is inferred from NewBasicInfo's return. staticcheck failed the lint job on it. 2. [CI BLOCKER] set_test.go testpackage — the test was `package set` (internal/white-box) so it could call unexported setProject. Lint requires `package set_test`. Reworked as black-box: drive the real `context set project --id ` flow through set.NewCmdSet + Cobra Execute, which is also a more faithful test of the actual entry point. Same 4 cases (legit same-team pin / cross-team reject / personal bypass / list-error propagation). 3. workspace/list/list.go — separate effective-owner from persisted for the two concerns that were sharing one variable: - `*` marker uses effectiveID (f.CurrentOwnerID) so a `--workspace` override is reflected. - stale-workspace warning uses persistedID (config workspace) so a transient override can't suppress a warning about a genuinely stale persisted workspace. 4. factory.go EffectiveContext — invalidate the cached ephemeral context when the override workspace changes mid-process. Today only resolveWorkspaceFlag sets the override (once per invocation), so this is defensive, but it stops a future mid-run team switch from handing back a stale in-memory project/service. All checks pass locally: build, vet, full suite, and staticcheck (the ST1023 + testpackage failures are gone). Co-Authored-By: Claude Opus 4.7 (1M context) --- internal/cmd/context/set/set_test.go | 51 +++++++++++++++++++--------- internal/cmd/workspace/list/list.go | 37 +++++++++++++------- internal/cmdutil/factory.go | 16 ++++++++- internal/util/runE_test.go | 2 +- 4 files changed, 76 insertions(+), 30 deletions(-) diff --git a/internal/cmd/context/set/set_test.go b/internal/cmd/context/set/set_test.go index 7f7e824..3d1d134 100644 --- a/internal/cmd/context/set/set_test.go +++ b/internal/cmd/context/set/set_test.go @@ -1,4 +1,4 @@ -package set +package set_test import ( "context" @@ -7,13 +7,14 @@ import ( "testing" "github.com/spf13/viper" + "go.uber.org/zap" + "github.com/zeabur/cli/internal/cmd/context/set" "github.com/zeabur/cli/internal/cmdutil" "github.com/zeabur/cli/pkg/api" "github.com/zeabur/cli/pkg/config" "github.com/zeabur/cli/pkg/model" "github.com/zeabur/cli/pkg/zcontext" - "go.uber.org/zap" ) // stubProjectClient stubs the two ApiClient methods setProject's ID path @@ -53,17 +54,21 @@ func newStubConfig() *stubConfig { return &stubConfig{v: viper.New()} } -func (s *stubConfig) GetTokenString() string { return "" } -func (s *stubConfig) SetTokenString(string) {} -func (s *stubConfig) GetUser() string { return "" } -func (s *stubConfig) SetUser(string) {} -func (s *stubConfig) GetUsername() string { return "alice" } -func (s *stubConfig) SetUsername(string) {} -func (s *stubConfig) GetContext() zcontext.Context { return zcontext.NewViperContext(s.v) } -func (s *stubConfig) Write() error { return nil } +func (s *stubConfig) GetTokenString() string { return "" } +func (s *stubConfig) SetTokenString(string) {} +func (s *stubConfig) GetUser() string { return "" } +func (s *stubConfig) SetUser(string) {} +func (s *stubConfig) GetUsername() string { return "alice" } +func (s *stubConfig) SetUsername(string) {} +func (s *stubConfig) GetContext() zcontext.Context { return zcontext.NewViperContext(s.v) } +func (s *stubConfig) Write() error { return nil } var _ config.Config = (*stubConfig)(nil) +// newFactory wires up the bare-minimum Factory needed to drive +// `context set` end-to-end via NewCmdSet. Interactive is forced off so the +// test exercises the deterministic non-interactive path (matches the +// `context set --id ` invocation in real use). func newFactory(t *testing.T, apiClient api.Client, persistedWorkspace *zcontext.Workspace) (*cmdutil.Factory, *stubConfig) { t.Helper() cfg := newStubConfig() @@ -75,9 +80,23 @@ func newFactory(t *testing.T, apiClient api.Client, persistedWorkspace *zcontext ApiClient: apiClient, Log: zap.NewNop().Sugar(), } + f.Interactive = false return f, cfg } +// runSetProjectByID drives the real `context set project --id ` flow +// through Cobra so the test exercises the same entry point a user does, +// rather than reaching for the unexported setProject. Black-box on purpose +// — that's also what the lint forces via the testpackage rule. +func runSetProjectByID(t *testing.T, f *cmdutil.Factory, id string) error { + t.Helper() + cmd := set.NewCmdSet(f) + cmd.SetArgs([]string{"project", "--id", id}) + cmd.SilenceUsage = true + cmd.SilenceErrors = true + return cmd.Execute() +} + // TestSetProject_ID_TeamWorkspace_AllowsOwnProject — the legitimate path. // In a team workspace, pinning a project that *does* belong to the team // succeeds and writes the pin to context. Critically, ListAllProjects is @@ -91,8 +110,8 @@ func TestSetProject_ID_TeamWorkspace_AllowsOwnProject(t *testing.T) { teamA := &zcontext.Workspace{ID: "65cc1230000000000000000a", Name: "team-A", Kind: zcontext.WorkspaceKindTeam} f, cfg := newFactory(t, stub, teamA) - if err := setProject(f, target.ID, "", true); err != nil { - t.Fatalf("setProject: %v", err) + if err := runSetProjectByID(t, f, target.ID); err != nil { + t.Fatalf("set project: %v", err) } if !stub.listAllOwnerCalled || stub.listAllOwner != teamA.ID { t.Fatalf("ListAllProjects called with owner=%q (called=%v), want %q", stub.listAllOwner, stub.listAllOwnerCalled, teamA.ID) @@ -119,9 +138,9 @@ func TestSetProject_ID_TeamWorkspace_RejectsForeignProject(t *testing.T) { teamA := &zcontext.Workspace{ID: "65cc1230000000000000000a", Name: "team-A", Kind: zcontext.WorkspaceKindTeam} f, cfg := newFactory(t, stub, teamA) - err := setProject(f, teamBProject.ID, "", true) + err := runSetProjectByID(t, f, teamBProject.ID) if err == nil { - t.Fatal("setProject must refuse cross-workspace --id, got nil error") + t.Fatal("set project must refuse cross-workspace --id, got nil error") } if !strings.Contains(err.Error(), "does not belong to workspace") { t.Errorf("error should explain cross-workspace mismatch, got: %v", err) @@ -146,7 +165,7 @@ func TestSetProject_ID_PersonalWorkspace_BypassesCheck(t *testing.T) { // Personal workspace: persistedWorkspace nil. f, cfg := newFactory(t, stub, nil) - if err := setProject(f, collaboratorProject.ID, "", true); err != nil { + if err := runSetProjectByID(t, f, collaboratorProject.ID); err != nil { t.Fatalf("personal --id must not be gated, got: %v", err) } if stub.listAllOwnerCalled { @@ -170,7 +189,7 @@ func TestSetProject_ID_TeamWorkspace_ListErr(t *testing.T) { teamA := &zcontext.Workspace{ID: "65cc1230000000000000000a", Name: "team-A", Kind: zcontext.WorkspaceKindTeam} f, cfg := newFactory(t, stub, teamA) - err := setProject(f, target.ID, "", true) + err := runSetProjectByID(t, f, target.ID) if err == nil { t.Fatal("ListAllProjects failure must propagate, not silently pass") } diff --git a/internal/cmd/workspace/list/list.go b/internal/cmd/workspace/list/list.go index c0e155b..d66d979 100644 --- a/internal/cmd/workspace/list/list.go +++ b/internal/cmd/workspace/list/list.go @@ -31,18 +31,28 @@ func run(f *cmdutil.Factory) error { return fmt.Errorf("list teams: %w", err) } - // Use the effective workspace so the `*` marker tracks a `--workspace` - // flag override, not just the persisted state. Otherwise - // `--workspace foo workspace list` would print `*` on the persisted - // team rather than `foo`. - currentID := f.CurrentOwnerID() + // Two different IDs to keep two different concerns honest: + // * effectiveID drives the `*` marker — it must reflect a + // `--workspace` flag override so the user sees which workspace + // this invocation is acting under. + // * persistedID drives the stale-workspace warning — that warning + // is about "your saved workspace is no longer valid", which is + // a property of the persisted state, not of the override. Using + // effectiveID for both would suppress the warning whenever a + // valid override happens to be active, hiding a stale persisted + // state from the user. + effectiveID := f.CurrentOwnerID() + persistedID := "" + if f.Config != nil { + persistedID = f.Config.GetContext().GetWorkspace().ID + } w := tabwriter.NewWriter(os.Stdout, 0, 0, 3, ' ', 0) // Personal always renders first. The 24-space placeholder keeps the // columns aligned with the team rows that follow. personalMarker := " " - if currentID == "" { + if effectiveID == "" { personalMarker = "*" } personalLabel := f.Config.GetUser() @@ -56,7 +66,7 @@ func run(f *cmdutil.Factory) error { for _, t := range teams { marker := " " - if t.ID == currentID { + if t.ID == effectiveID { marker = "*" } role := "" @@ -70,14 +80,17 @@ func run(f *cmdutil.Factory) error { return fmt.Errorf("render table: %w", err) } - if currentID != "" { + if persistedID != "" { // If the persisted workspace is no longer in the membership list // (e.g. the team was deleted / the caller was removed), surface it - // so the user knows the next command may behave unexpectedly. We - // don't auto-clear here — that's the lazy-verify path in root. + // so the user knows the next command without `--workspace` may + // behave unexpectedly. We deliberately check persistedID, not + // effectiveID, so a transient `--workspace` override doesn't + // suppress the warning. We don't auto-clear here either — that's + // the lazy-verify path in root. seen := false for _, t := range teams { - if t.ID == currentID { + if t.ID == persistedID { seen = true break } @@ -85,7 +98,7 @@ func run(f *cmdutil.Factory) error { if !seen { fmt.Fprintf(os.Stderr, "\nwarning: the persisted workspace %s is not in your memberships any more — run `zeabur workspace clear` or switch to another workspace.\n", - currentID, + persistedID, ) } } diff --git a/internal/cmdutil/factory.go b/internal/cmdutil/factory.go index 1ab737f..9bb9d97 100644 --- a/internal/cmdutil/factory.go +++ b/internal/cmdutil/factory.go @@ -189,8 +189,22 @@ func (f *Factory) EffectiveContext() zcontext.Context { } return f.Config.GetContext() } + // Cache the ephemeral context but invalidate it if the override + // workspace changes mid-process. Today only `resolveWorkspaceFlag` + // in PersistentPreRunE calls SetWorkspaceOverride and it runs + // exactly once per invocation, so the cache match is the common + // path. The mismatch branch is purely defensive: a future caller + // that flips the override (e.g. an interactive command that lets + // the user switch teams mid-run) would otherwise see a stale + // in-memory project/service from the previous override. + ws := f.CurrentWorkspace() if f.ephemeralCtx == nil { - f.ephemeralCtx = zcontext.NewEphemeralContext(f.CurrentWorkspace()) + f.ephemeralCtx = zcontext.NewEphemeralContext(ws) + return f.ephemeralCtx + } + cachedWS := f.ephemeralCtx.GetWorkspace() + if cachedWS.ID != ws.ID || cachedWS.Name != ws.Name || cachedWS.Kind != ws.Kind { + f.ephemeralCtx = zcontext.NewEphemeralContext(ws) } return f.ephemeralCtx } diff --git a/internal/util/runE_test.go b/internal/util/runE_test.go index c2a4a8f..b6860d4 100644 --- a/internal/util/runE_test.go +++ b/internal/util/runE_test.go @@ -18,7 +18,7 @@ import ( // helper captured the BasicInfo eagerly (the pre-fix behaviour) the swap // would be ignored and id/name would be filled from the original value. func TestDefaultIDNameByContext_LazyEvaluation(t *testing.T) { - var source zcontext.BasicInfo = zcontext.NewBasicInfo("old-id", "old-name") + var source = zcontext.NewBasicInfo("old-id", "old-name") getter := func() zcontext.BasicInfo { return source } var id, name string