From f39e14a7414a20295ddc88f2fa4030636f3484de Mon Sep 17 00:00:00 2001 From: leet-c1 <264029741+leet-c1@users.noreply.github.com> Date: Tue, 1 Sep 2026 00:13:31 +0000 Subject: [PATCH 1/8] feat: add "entitlements create" Collapses the resource-type/resource/entitlement sequence the configure-new-app guide previously spelled out as three raw "c1i api" calls into one command. --resource-type-id and --resource-id skip whichever of the first two POSTs the caller already has, so one resource type can carry many resources and one resource many entitlements; without them all three are sent. The server requires appResourceTypeId AND appResourceId on the entitlement even though the OpenAPI schema marks only displayName required, so --resource-id without --resource-type-id is a usage error (exit 2) rather than a 400. --duration-grant covers the standing-vs-time-boxed distinction inline rather than sending readers to a raw POST for it. It takes a protobuf duration ("3600s"); a Go-style "1h" is refused by the server. The three writes are not atomic and nothing is rolled back, so a failure part-way through names the objects this run created and the flags that reuse them. --dry-run previews all three requests, standing in NEW_APP_RESOURCE_TYPE_ID/NEW_APP_RESOURCE_ID for ids that only exist after a real preceding step, and its banner names only the stand-ins the preview actually uses. The resource-type enum has one definition, resourceTypeKinds, that the help text and flag usage are built from and that a test holds README.md and the guide to. Also corrects the guide, which opened by saying no such command exists, and two of its timing claims that this session measured: entitlement owners lag the write like app owners do (116s), and the entitlement search index is fast but not transactional. Co-Authored-By: Claude Opus 5 --- CHANGELOG.md | 14 + README.md | 38 +++ cmd/agents.md | 11 + cmd/docs_guide.go | 69 +++-- cmd/entitlements_create.go | 388 ++++++++++++++++++++++++ cmd/entitlements_create_test.go | 503 ++++++++++++++++++++++++++++++++ cmd/flags.go | 22 ++ 7 files changed, 1022 insertions(+), 23 deletions(-) create mode 100644 cmd/entitlements_create.go create mode 100644 cmd/entitlements_create_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index 61aadc5..3f6387a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,20 @@ to follow [Semantic Versioning](https://semver.org/spec/v2.0.0.html). ## [Unreleased] +### Added + +- **`c1i entitlements create`.** Modelling a manually-managed app took three + raw `api` calls -- resource type, resource, then entitlement -- with the ids + hand-carried between them. One command now does it, and reuses objects you + already have: pass `--resource-type-id` to skip the first call, and + `--resource-id` as well to skip the second. `--owner-id` is set + inline rather than needing a follow-up call, and `--duration-grant` is a flag + rather than a hand-written body field. + + On a partial failure nothing is rolled back; the error names what that run + created and the flags to re-run with, so a retry reuses those objects + instead of duplicating them. `--dry-run` previews all three requests. + ### Changed - **BREAKING — a negative `--limit` or `--page-size` is now a usage error diff --git a/README.md b/README.md index 1c73467..ebeb257 100644 --- a/README.md +++ b/README.md @@ -99,8 +99,46 @@ c1i accounts set-owner --app-id --user-id ```sh c1i entitlements list [--app-id ] [--query ] [--page-size N] [--page-token TOKEN] [--limit N] c1i entitlements get --app-id + +# Create one on a manually-managed app, with its resource type and resource +c1i entitlements create --app-id --display-name "Payroll admin" \ + [--description ] [--slug member] [--alias payroll_admin] [--owner-id ] \ + [--duration-grant 3600s] [--resource-type CUSTOM] \ + [--resource-type-display-name "Payroll role"] [--resource-display-name "Payroll admins"] + +# Reuse an existing resource type (or an existing resource) instead of creating one +c1i entitlements create --app-id --display-name "Payroll viewer" --resource-type-id +c1i entitlements create --app-id --display-name "Payroll viewer (RO)" \ + --resource-type-id --resource-id ``` +An entitlement points at an app resource, which lives under an app resource +type, so `entitlements create` is up to three `POST`s: the resource type, the +resource, then the entitlement. `--resource-type-id` and `--resource-id` skip +whichever of the first two steps you already have — one resource type can carry +many resources, and one resource many entitlements. The server requires both +ids on the entitlement even though the OpenAPI schema marks only `displayName` +required, so `--resource-id` without `--resource-type-id` is rejected at exit +`2` before anything is sent. + +`--resource-type` is `ROLE`, `GROUP`, `LICENSE`, `PROJECT`, `CATALOG`, +`CUSTOM`, `VAULT` or `PROFILE_TYPE` (case-insensitive here, uppercase on the +wire) and describes the resource type this command creates, so passing it +together with `--resource-type-id` is a usage error rather than a silently +ignored flag. `--owner-id` is repeatable and goes inline in the create request, +so no follow-up call is needed. `--duration-grant` takes a protobuf duration — +seconds with an `s` suffix, e.g. `3600s`; a Go-style `1h` is refused by the +server. Omit it for standing access. + +`--dry-run` previews all three requests, printing +`NEW_APP_RESOURCE_TYPE_ID`/`NEW_APP_RESOURCE_ID` where an id only exists after +a real preceding step. There is no rollback: if a later step fails, the objects +the earlier ones created still exist, and the error names them along with the +flags that reuse them. The created entitlement comes back as pretty JSON under +`appEntitlementView` (`--fields` is never applied to mutation output); it echoes +`appResourceTypeId`/`appResourceId` and expands both objects, so every id the +command touched is in that one payload. + ### Grants ("who has access") ```sh diff --git a/cmd/agents.md b/cmd/agents.md index 429b5a5..4ee0a8d 100644 --- a/cmd/agents.md +++ b/cmd/agents.md @@ -241,6 +241,13 @@ Two things are irreversible in ways their `--help` doesn't make obvious: each toolset's app entitlement, is soft-deleted with it. Anyone whose access came through one of those entitlements is affected. +One command sends more than one write: `entitlements create` POSTs a resource +type, a resource, then the entitlement, skipping the steps whose id you supply +via `--resource-type-id`/`--resource-id`. Its `--dry-run` previews all three. +There is no rollback, so a failure part-way through leaves the earlier objects +behind; the error names them and the flags that reuse them, and re-running +without those flags creates duplicates. + ## Things that will surprise you - Owner and grant provisioning are asynchronous. A read immediately after a @@ -300,6 +307,10 @@ Two things are irreversible in ways their `--help` doesn't make obvious: - Entitlement ids are unique only within an app — some system-builtin entitlements reuse the same id across every app that has one. Always key on `(app_id, id)` together, never `id` alone. +- `POST /api/v1/apps/{app_id}/entitlements` requires `appResourceTypeId` and + `appResourceId` even though its OpenAPI schema lists only `displayName` as + required; omitting them 400s on the id regex. `entitlements create` handles + this for you. - `mcp servers test-connection` returns `toolCount` as a JSON string, not a number. The `tool_count` in NDJSON list rows is a real number. - `mcp servers search` only includes `tool_count` when you pass diff --git a/cmd/docs_guide.go b/cmd/docs_guide.go index da706ad..2697a92 100644 --- a/cmd/docs_guide.go +++ b/cmd/docs_guide.go @@ -346,11 +346,10 @@ provisionerPolicy.delegated update is the actual provisioning trigger. ` // guideConfigureNewApp walks through creating a manually-managed app -// container, setting its owners, and creating a custom entitlement for it -// via the 3-call resource-type/resource/entitlement sequence (no first-class -// "entitlements create" exists). Derived from cmd/apps_create.go, -// cmd/apps_set_owners.go, cmd/entitlements_get.go, cmd/entitlements_list.go, -// and cmd/api.go. +// container, setting its owners, and creating a custom entitlement for it. +// Derived from cmd/apps_create.go, cmd/apps_set_owners.go, +// cmd/entitlements_create.go, cmd/entitlements_get.go, +// cmd/entitlements_list.go, and cmd/api.go. const guideConfigureNewApp = `# Configure a new app Stand up an app container, assign the C1 users who administer it, and give it @@ -412,32 +411,55 @@ here removes yourself. Use "apps add-owner" instead to add without replacing. ### 3. Create a custom entitlement to grant -There is no "entitlements create" — only "entitlements get"/"list". Creating -one for a manually-managed app is a 3-call sequence instead: a resource -type, a resource under it, then the entitlement pointing at both. No -first-class command covers this, so each call goes through "c1i api": +An entitlement points at an app resource, which lives under an app resource +type, so creating one is three POSTs. "entitlements create" sends all three: - c1i api --path=/api/v1/apps/$APP_ID/resource_types --body='{"displayName":"Payroll role","resourceType":"CUSTOM"}' - RT_ID= + c1i entitlements create --app-id "$APP_ID" \ + --display-name "Payroll admin" --resource-type-display-name "Payroll role" \ + --slug member --alias payroll_admin --owner-id "$OWNER_USER_ID" + ENT_ID= - c1i api --path=/api/v1/apps/$APP_ID/resource_types/$RT_ID/resources --body='{"displayName":"Payroll admin"}' - RES_ID= +The response echoes the ids of the other two objects in that same payload: - c1i api --path=/api/v1/apps/$APP_ID/entitlements --body='{"displayName":"Payroll admin","slug":"member","alias":"payroll_admin","appResourceTypeId":"'$RT_ID'","appResourceId":"'$RES_ID'"}' - ENT_ID= + RT_ID= + RES_ID= + +Reuse them for the next entitlement rather than minting a duplicate resource +type per entitlement: + + c1i entitlements create --app-id "$APP_ID" --display-name "Payroll viewer" \ + --resource-type-id "$RT_ID" # new resource under an existing type + c1i entitlements create --app-id "$APP_ID" --display-name "Payroll viewer (RO)" \ + --resource-type-id "$RT_ID" --resource-id "$RES_ID" # entitlement only + +--resource-type (the kind of resource type to create, default CUSTOM) is one +of ROLE, GROUP, LICENSE, PROJECT, CATALOG, CUSTOM, VAULT, PROFILE_TYPE. + +--owner-id is repeatable and rides along in the create request, so entitlement +owners need no follow-up call -- but the read lags the write the same way app +owners do (one measured create took 116s to show up on +"GET .../entitlements/$ENT_ID/ownerids"), so don't read that as a failure. + +The three writes are not atomic and nothing is rolled back: if a later one +fails, the objects the earlier ones created still exist, and the error names +them and the flags that reuse them, e.g. "(already created: re-run with +--resource-type-id to reuse instead of duplicating)". "--dry-run" +previews all three requests. -resourceType is one of ROLE|GROUP|LICENSE|PROJECT|CATALOG|CUSTOM|VAULT|PROFILE_TYPE. -Omitting a duration defaults the entitlement to standing access -(durationUnset); pass a durationGrant field (e.g. 3600s) instead for -time-boxed access. +Omitting --duration-grant defaults the entitlement to standing access +(durationUnset). For time-boxed access pass a protobuf duration -- seconds +with an "s" suffix, e.g. "--duration-grant 3600s"; a Go-style "1h" is refused +by the server with "invalid google.protobuf.Duration value". ## Verify c1i entitlements list --app-id "$APP_ID" Auto-paginates to completion; expect the builtin "Access" row plus your new -entitlement, both present immediately — the entitlement search index is not -lagged the way owners are. +entitlement. The entitlement search index is not lagged the way owners are -- +three measured creates were listed within a second -- but it isn't +transactional either: a fourth was missing from a list issued immediately +after it. Re-run before concluding the create failed. c1i entitlements get "$ENT_ID" --app-id "$APP_ID" @@ -519,8 +541,9 @@ you'd take it back. c1i auth whoami -- An app and an entitlement that already exist to request against. There is - no "entitlements create" — find real ones: +- An app and an entitlement that already exist to request against. Find real + ones — "entitlements create" only makes manually-managed ones, which is not + what this workflow is for (see "c1i docs guide configure-new-app"): c1i apps list APP_ID= diff --git a/cmd/entitlements_create.go b/cmd/entitlements_create.go new file mode 100644 index 0000000..c28c581 --- /dev/null +++ b/cmd/entitlements_create.go @@ -0,0 +1,388 @@ +package cmd + +import ( + "encoding/json" + "fmt" + "strings" + + "github.com/ConductorOne/c1i/internal/client" + "github.com/spf13/cobra" +) + +// resourceTypeKinds is the AppResourceType enum, and the single source for +// every place this repo names it. TestResourceTypeKindsAreDocumented holds the +// help text, README.md and the configure-new-app guide to this list. Not used +// to validate --resource-type: the server stays authoritative if the enum +// grows, and normalizeResourceType passes an unknown name through. +var resourceTypeKinds = []string{"ROLE", "GROUP", "LICENSE", "PROJECT", "CATALOG", "CUSTOM", "VAULT", "PROFILE_TYPE"} + +// Stand-ins printed by --dry-run for ids only a preceding response can supply. +// Alphanumeric + "_" so client.Path leaves them unescaped and the previewed +// path stays readable. +const ( + newResourceTypeIDPlaceholder = "NEW_APP_RESOURCE_TYPE_ID" + newResourceIDPlaceholder = "NEW_APP_RESOURCE_ID" +) + +var entitlementsCreateCmd = &cobra.Command{ + Use: "create", + Short: "Create an app entitlement, with its resource type and resource (pretty JSON)", + Long: `Create an app entitlement on a manually-managed app. + +An entitlement points at an app resource, which lives under an app resource +type, so this is up to three POSTs in order: + + 1. POST /api/v1/apps/{app-id}/resource_types + 2. POST /api/v1/apps/{app-id}/resource_types/{app-resource-type-id}/resources + 3. POST /api/v1/apps/{app-id}/entitlements + +Steps 1 and 2 are skipped for whichever of --resource-type-id/--resource-id +you already have, so one resource type can carry many resources and one +resource many entitlements: + + --app-id --display-name all three (a new type and resource) + ... --resource-type-id steps 2 and 3 (a new resource + under an existing type) + ... --resource-type-id --resource-id step 3 only + +Both ids are required by the server even though the OpenAPI schema marks only +displayName required — omitting them fails with: + invalid CreateAppEntitlementRequest.AppResourceTypeId: value does not match regex pattern "^[a-zA-Z0-9]{27}$" +so --resource-id without --resource-type-id is rejected here, at exit 2, +before anything is sent. + +--resource-type describes the resource type this command creates, so passing +it together with --resource-type-id is a usage error rather than a silently +ignored flag. Case-insensitive here; the API itself takes only the uppercase +form. One of: + ` + strings.Join(resourceTypeKinds, ", ") + ` + +Omitting --duration-grant leaves the entitlement at standing access +(durationUnset in the response). It takes a protobuf duration, not a Go one -- +seconds with an "s" suffix, e.g. 3600s. "1h" is refused by the server with: + invalid google.protobuf.Duration value "1h" + +Assign owners inline with --owner-id: the create request carries +appEntitlementOwnerIds, so no follow-up call is needed. Owner provisioning is +asynchronous — one measured create took 116s to read back on +"GET /api/v1/apps/{app-id}/entitlements/{id}/ownerids". + +--dry-run previews every request in the sequence. The ids that only exist +after a real step 1 or 2 print as ` + newResourceTypeIDPlaceholder + `/` + + newResourceIDPlaceholder + `. + +There is no rollback. The three creates are independent, so if step 2 or 3 +fails the objects earlier steps created still exist; the error names them and +the flags that reuse them, verbatim: + (already created: re-run with --resource-type-id to reuse instead of duplicating) + +Prints the created entitlement as pretty JSON under "appEntitlementView" +(--fields is not applied to mutation output). The response echoes +appResourceTypeId/appResourceId and expands both objects, so every id this +command touched is in that one payload. + +Example: + c1i entitlements create --app-id "$APP_ID" \ + --display-name "Payroll admin" --resource-type-display-name "Payroll role" \ + --slug member --alias payroll_admin`, + RunE: func(cmd *cobra.Command, args []string) error { + if err := requireNonEmpty(cmd, "app-id", "display-name"); err != nil { + return err + } + + plan, err := buildEntitlementCreatePlan(cmd) + if err != nil { + // Already a *usageError from the flag helpers; flags.go documents + // that callers must not re-wrap. + return err + } + + baseURL, err := GetBaseURL() + if err != nil { + return err + } + + if dryRunActive() { + return plan.previewRequests(cmd) + } + + c, err := newClient(cmd, baseURL) + if err != nil { + return fmt.Errorf("authentication failed: %w", err) + } + + return plan.run(cmd, c) + }, +} + +// entitlementCreatePlan is the resolved create sequence: a step's body is nil +// when the caller supplied that object's id and the step is skipped. +type entitlementCreatePlan struct { + appID string + + resourceTypeID string // reuse this type when non-empty + resourceTypeBody map[string]any // otherwise create one from this + + resourceID string + resourceBody map[string]any + + // entitlementBody carries everything except appResourceTypeId and + // appResourceId, which are only known once the steps above have run. + entitlementBody map[string]any +} + +// buildEntitlementCreatePlan resolves flags into the request sequence. Pure (no +// network / auth) so --dry-run and unit tests exercise the same bodies the live +// requests send. Optional fields are omitted when empty rather than sent as +// empty strings/arrays. +func buildEntitlementCreatePlan(cmd *cobra.Command) (*entitlementCreatePlan, error) { + appID, _ := cmd.Flags().GetString("app-id") + displayName, _ := cmd.Flags().GetString("display-name") + + resourceTypeID, err := requireNonEmptyIfSet(cmd, "resource-type-id") + if err != nil { + return nil, err + } + resourceID, err := requireNonEmptyIfSet(cmd, "resource-id") + if err != nil { + return nil, err + } + if resourceID != "" && resourceTypeID == "" { + return nil, &usageError{fmt.Errorf("--resource-id requires --resource-type-id: the entitlement carries both ids, and the server rejects a missing one with " + + `invalid CreateAppEntitlementRequest.AppResourceTypeId: value does not match regex pattern "^[a-zA-Z0-9]{27}$"`)} + } + + if err := rejectFlagsForReusedObject(cmd, resourceTypeID, "--resource-type-id", "resource-type", "resource-type-display-name"); err != nil { + return nil, err + } + if err := rejectFlagsForReusedObject(cmd, resourceID, "--resource-id", "resource-display-name"); err != nil { + return nil, err + } + + // --resource-type has a default, so an explicit "" is a mistake. Checked + // after the reuse rejection above, whose message is the more useful one when + // --resource-type-id is also set. + resourceType, err := requireNonEmptyIfSet(cmd, "resource-type") + if err != nil { + return nil, err + } + + p := &entitlementCreatePlan{ + appID: appID, + resourceTypeID: resourceTypeID, + resourceID: resourceID, + entitlementBody: map[string]any{"displayName": displayName}, + } + + if resourceTypeID == "" { + p.resourceTypeBody = map[string]any{ + "displayName": flagOrDefault(cmd, "resource-type-display-name", displayName), + "resourceType": normalizeResourceType(resourceType), + } + } + if resourceID == "" { + p.resourceBody = map[string]any{ + "displayName": flagOrDefault(cmd, "resource-display-name", displayName), + } + } + + // Flag name -> request field, equal except where the wire key is camelCase. + for flag, key := range map[string]string{ + "description": "description", + "slug": "slug", + "alias": "alias", + "duration-grant": "durationGrant", + } { + if v, _ := cmd.Flags().GetString(flag); v != "" { + p.entitlementBody[key] = v + } + } + if owners, _ := cmd.Flags().GetStringSlice("owner-id"); len(owners) > 0 { + p.entitlementBody["appEntitlementOwnerIds"] = owners + } + + return p, nil +} + +// rejectFlagsForReusedObject fails when flags that only describe an object this +// command would create are passed alongside the id of an existing one, rather +// than ignoring them and creating something the caller didn't ask for. +func rejectFlagsForReusedObject(cmd *cobra.Command, reusedID, reusedFlag string, names ...string) error { + if reusedID == "" { + return nil + } + for _, n := range names { + if cmd.Flags().Changed(n) { + return &usageError{fmt.Errorf("--%s only applies when this command creates that object; drop it or drop %s", n, reusedFlag)} + } + } + return nil +} + +// normalizeResourceType upper-cases the enum name so "custom" and +// "profile-type" work; anything unrecognized is passed through for the server +// to reject, keeping it authoritative if the enum grows. +func normalizeResourceType(s string) string { + return strings.ToUpper(strings.ReplaceAll(s, "-", "_")) +} + +func (p *entitlementCreatePlan) resourceTypesPath() string { + return client.Path("/api/v1/apps/%s/resource_types", p.appID) +} + +func (p *entitlementCreatePlan) resourcesPath(resourceTypeID string) string { + return client.Path("/api/v1/apps/%s/resource_types/%s/resources", p.appID, resourceTypeID) +} + +func (p *entitlementCreatePlan) entitlementsPath() string { + return client.Path("/api/v1/apps/%s/entitlements", p.appID) +} + +// fullEntitlementBody returns the step-3 body with the resource ids filled in, +// leaving p.entitlementBody untouched so a caller can build it more than once. +func (p *entitlementCreatePlan) fullEntitlementBody(resourceTypeID, resourceID string) map[string]any { + body := make(map[string]any, len(p.entitlementBody)+2) + for k, v := range p.entitlementBody { + body[k] = v + } + body["appResourceTypeId"] = resourceTypeID + body["appResourceId"] = resourceID + return body +} + +// run issues the planned requests in order and prints the entitlement the last +// one returns. +func (p *entitlementCreatePlan) run(cmd *cobra.Command, c *client.Client) error { + ctx := cmd.Context() + + resourceTypeID := p.resourceTypeID + if p.resourceTypeBody != nil { + data, err := c.Post(ctx, p.resourceTypesPath(), p.resourceTypeBody) + if err != nil { + return fmt.Errorf("API error creating the app resource type: %w", err) + } + if resourceTypeID, err = createdObjectID(data, "appResourceType"); err != nil { + return err + } + } + + resourceID := p.resourceID + if p.resourceBody != nil { + data, err := c.Post(ctx, p.resourcesPath(resourceTypeID), p.resourceBody) + if err != nil { + return fmt.Errorf("API error creating the app resource%s: %w", p.createdSoFar(resourceTypeID, ""), err) + } + if resourceID, err = createdObjectID(data, "appResource"); err != nil { + return err + } + } + + data, err := c.Post(ctx, p.entitlementsPath(), p.fullEntitlementBody(resourceTypeID, resourceID)) + if err != nil { + return fmt.Errorf("API error creating the entitlement%s: %w", p.createdSoFar(resourceTypeID, resourceID), err) + } + + return writeRawObject(cmd, data) +} + +// createdSoFar names the objects THIS invocation created before failing, and +// the flags that reuse them. Nothing is rolled back: a compensating delete can +// fail too, which would leave a worse state described by a less honest message. +func (p *entitlementCreatePlan) createdSoFar(resourceTypeID, resourceID string) string { + var parts []string + if p.resourceTypeBody != nil && resourceTypeID != "" { + parts = append(parts, "--resource-type-id "+resourceTypeID) + } + if p.resourceBody != nil && resourceID != "" { + parts = append(parts, "--resource-id "+resourceID) + } + if len(parts) == 0 { + return "" + } + return " (already created: re-run with " + strings.Join(parts, " ") + " to reuse instead of duplicating)" +} + +// previewRequests previews every request in the sequence, not just the first — +// two of the three are writes the caller would otherwise not see coming. +func (p *entitlementCreatePlan) previewRequests(cmd *cobra.Command) error { + resourceTypeID, resourceID := p.resourceTypeID, p.resourceID + + var used []string + if p.resourceTypeBody != nil { + resourceTypeID = newResourceTypeIDPlaceholder + used = append(used, newResourceTypeIDPlaceholder) + } + if p.resourceBody != nil { + resourceID = newResourceIDPlaceholder + used = append(used, newResourceIDPlaceholder) + } + if len(used) > 0 { + subject := "stands in for an id" + if len(used) > 1 { + subject = "stand in for ids" + } + _, _ = fmt.Fprintf(cmd.OutOrStdout(), + "[dry-run] requests are sent in order; %s %s the preceding response returns\n", + strings.Join(used, "/"), subject) + } + + if p.resourceTypeBody != nil { + if err := printDryRun(cmd, "POST", p.resourceTypesPath(), p.resourceTypeBody); err != nil { + return err + } + } + if p.resourceBody != nil { + if err := printDryRun(cmd, "POST", p.resourcesPath(resourceTypeID), p.resourceBody); err != nil { + return err + } + } + return printDryRun(cmd, "POST", p.entitlementsPath(), p.fullEntitlementBody(resourceTypeID, resourceID)) +} + +// createdObjectID pulls .id out of a create response. A 200 that carries +// no id would otherwise read as success while the next request in the chain +// addresses a path with an empty segment. +func createdObjectID(data []byte, key string) (string, error) { + var resp map[string]json.RawMessage + if err := json.Unmarshal(data, &resp); err != nil { + return "", &nonJSONResponseError{fmt.Errorf("failed to parse %s response: %w", key, err)} + } + raw, ok := resp[key] + if !ok { + return "", &nonJSONResponseError{fmt.Errorf("response carried no %q object", key)} + } + var obj struct { + ID string `json:"id"` + } + if err := json.Unmarshal(raw, &obj); err != nil { + return "", &nonJSONResponseError{fmt.Errorf("failed to parse %s response: %w", key, err)} + } + if obj.ID == "" { + return "", &nonJSONResponseError{fmt.Errorf("response carried no %s.id", key)} + } + return obj.ID, nil +} + +// addEntitlementCreateFlags registers the flag set. Shared with the tests so a +// flag can't be added to the command yet missed by what exercises it. +func addEntitlementCreateFlags(cmd *cobra.Command) { + f := cmd.Flags() + f.String("app-id", "", "Application ID to create the entitlement on") + f.String("display-name", "", "Display name for the new entitlement (also names the resource type and resource this command creates)") + f.String("description", "", "Description for the new entitlement") + f.String("slug", "", "Slug for the new entitlement (e.g. member)") + f.String("alias", "", "Alias for the new entitlement; exact-match queryable") + f.String("duration-grant", "", "Maximum grant duration as a protobuf duration, e.g. 3600s; omit for standing access") + f.StringSlice("owner-id", nil, "C1 user ID to own the new entitlement (repeatable)") + f.String("resource-type", "CUSTOM", "Kind of resource type to create: "+strings.Join(resourceTypeKinds, ", ")) + f.String("resource-type-id", "", "Existing app resource type to reuse instead of creating one") + f.String("resource-type-display-name", "", "Display name for the resource type this command creates (default: --display-name)") + f.String("resource-id", "", "Existing app resource to reuse instead of creating one; requires --resource-type-id") + f.String("resource-display-name", "", "Display name for the resource this command creates (default: --display-name)") + markRequired(cmd, "app-id", "display-name") +} + +func init() { + addEntitlementCreateFlags(entitlementsCreateCmd) + entitlementsCmd.AddCommand(entitlementsCreateCmd) +} diff --git a/cmd/entitlements_create_test.go b/cmd/entitlements_create_test.go new file mode 100644 index 0000000..c5cf26b --- /dev/null +++ b/cmd/entitlements_create_test.go @@ -0,0 +1,503 @@ +package cmd + +import ( + "bytes" + "context" + "encoding/json" + "errors" + "net/http" + "net/http/httptest" + "reflect" + "slices" + "strings" + "testing" + + "github.com/ConductorOne/c1i/internal/client" + "github.com/spf13/cobra" +) + +// newEntitlementCreateCmd builds a throwaway command carrying the real flag +// set, so a test never drifts from what the command actually registers. +func newEntitlementCreateCmd(t *testing.T, args ...string) *cobra.Command { + t.Helper() + cmd := &cobra.Command{Use: "create", RunE: func(*cobra.Command, []string) error { return nil }} + addEntitlementCreateFlags(cmd) + cmd.SetArgs(args) + cmd.SetOut(new(bytes.Buffer)) + cmd.SetErr(new(bytes.Buffer)) + cmd.SetContext(context.Background()) + if err := cmd.Execute(); err != nil { + t.Fatalf("parsing %v: %v", args, err) + } + return cmd +} + +func mustPlan(t *testing.T, args ...string) *entitlementCreatePlan { + t.Helper() + p, err := buildEntitlementCreatePlan(newEntitlementCreateCmd(t, args...)) + if err != nil { + t.Fatalf("buildEntitlementCreatePlan(%v): %v", args, err) + } + return p +} + +// TestEntitlementCreatePlanCreatesAllThree pins the default sequence: a +// resource type and resource are created, both named from --display-name +// unless separately overridden, and unset optional fields are omitted rather +// than sent empty. +func TestEntitlementCreatePlanCreatesAllThree(t *testing.T) { + p := mustPlan(t, "--app-id", "app1", "--display-name", "Payroll admin") + + if p.resourceTypeID != "" || p.resourceID != "" { + t.Errorf("nothing was supplied to reuse, got type=%q resource=%q", p.resourceTypeID, p.resourceID) + } + wantType := map[string]any{"displayName": "Payroll admin", "resourceType": "CUSTOM"} + if !reflect.DeepEqual(p.resourceTypeBody, wantType) { + t.Errorf("resource type body = %v, want %v", p.resourceTypeBody, wantType) + } + wantResource := map[string]any{"displayName": "Payroll admin"} + if !reflect.DeepEqual(p.resourceBody, wantResource) { + t.Errorf("resource body = %v, want %v", p.resourceBody, wantResource) + } + wantEnt := map[string]any{"displayName": "Payroll admin"} + if !reflect.DeepEqual(p.entitlementBody, wantEnt) { + t.Errorf("entitlement body = %v, want %v", p.entitlementBody, wantEnt) + } +} + +// TestEntitlementCreatePlanOptionalFields pins that every optional flag reaches +// the body it belongs to, under the wire key the API expects. +func TestEntitlementCreatePlanOptionalFields(t *testing.T) { + p := mustPlan(t, + "--app-id", "app1", "--display-name", "Payroll admin", + "--description", "pays people", "--slug", "member", "--alias", "payroll_admin", + "--owner-id", "u1", "--owner-id", "u2", + "--resource-type", "profile-type", + "--resource-type-display-name", "Payroll role", + "--resource-display-name", "Payroll admins", + ) + + if got := p.resourceTypeBody["displayName"]; got != "Payroll role" { + t.Errorf("resource type displayName = %v", got) + } + // Lower case and "-" are normalized; the API takes only PROFILE_TYPE. + if got := p.resourceTypeBody["resourceType"]; got != "PROFILE_TYPE" { + t.Errorf("resourceType = %v, want PROFILE_TYPE", got) + } + if got := p.resourceBody["displayName"]; got != "Payroll admins" { + t.Errorf("resource displayName = %v", got) + } + want := map[string]any{ + "displayName": "Payroll admin", + "description": "pays people", + "slug": "member", + "alias": "payroll_admin", + "appEntitlementOwnerIds": []string{"u1", "u2"}, + } + if !reflect.DeepEqual(p.entitlementBody, want) { + t.Errorf("entitlement body = %v, want %v", p.entitlementBody, want) + } +} + +// TestEntitlementCreatePlanReusesSuppliedIDs pins that a supplied id skips +// exactly the step that would have created that object, and no more. +func TestEntitlementCreatePlanReusesSuppliedIDs(t *testing.T) { + t.Run("resource type only", func(t *testing.T) { + p := mustPlan(t, "--app-id", "app1", "--display-name", "e", "--resource-type-id", "rt1") + if p.resourceTypeBody != nil { + t.Errorf("resource type body = %v, want nil (reusing rt1)", p.resourceTypeBody) + } + if p.resourceBody == nil { + t.Error("resource body is nil; a resource must still be created") + } + }) + t.Run("both", func(t *testing.T) { + p := mustPlan(t, "--app-id", "app1", "--display-name", "e", "--resource-type-id", "rt1", "--resource-id", "r1") + if p.resourceTypeBody != nil || p.resourceBody != nil { + t.Errorf("bodies = %v/%v, want both nil (entitlement only)", p.resourceTypeBody, p.resourceBody) + } + body := p.fullEntitlementBody(p.resourceTypeID, p.resourceID) + if body["appResourceTypeId"] != "rt1" || body["appResourceId"] != "r1" { + t.Errorf("entitlement body = %v, want the supplied ids", body) + } + }) +} + +// TestEntitlementCreatePlanUsageErrors pins the combinations refused before any +// request is sent. Each would otherwise either 400 on the server or silently +// create something the caller did not ask for. +func TestEntitlementCreatePlanUsageErrors(t *testing.T) { + cases := []struct { + name string + args []string + want string + }{ + { + "resource id without its type", + []string{"--app-id", "app1", "--display-name", "e", "--resource-id", "r1"}, + "--resource-id requires --resource-type-id", + }, + { + // --resource-type has a default, so an explicit "" is a mistake. + // It used to reach the wire as an empty enum. + "explicitly empty resource type", + []string{"--app-id", "app1", "--display-name", "e", "--resource-type", ""}, + "flag --resource-type requires a non-empty value", + }, + { + // With --resource-type-id set, the reuse rejection is the more + // useful message: the flag does not belong at all. + "empty resource type alongside a reused type", + []string{"--app-id", "app1", "--display-name", "e", "--resource-type-id", "rt1", "--resource-type", ""}, + "--resource-type only applies when this command creates that object", + }, + { + "resource type kind alongside a reused type", + []string{"--app-id", "app1", "--display-name", "e", "--resource-type-id", "rt1", "--resource-type", "ROLE"}, + "--resource-type only applies", + }, + { + "resource type name alongside a reused type", + []string{"--app-id", "app1", "--display-name", "e", "--resource-type-id", "rt1", "--resource-type-display-name", "x"}, + "--resource-type-display-name only applies", + }, + { + "resource name alongside a reused resource", + []string{"--app-id", "app1", "--display-name", "e", "--resource-type-id", "rt1", "--resource-id", "r1", "--resource-display-name", "x"}, + "--resource-display-name only applies", + }, + { + "empty resource type id", + []string{"--app-id", "app1", "--display-name", "e", "--resource-type-id", ""}, + "flag --resource-type-id requires a non-empty value", + }, + { + "empty resource id", + []string{"--app-id", "app1", "--display-name", "e", "--resource-type-id", "rt1", "--resource-id", ""}, + "flag --resource-id requires a non-empty value", + }, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + _, err := buildEntitlementCreatePlan(newEntitlementCreateCmd(t, tc.args...)) + if err == nil { + t.Fatalf("expected a usage error for %v", tc.args) + } + if !strings.Contains(err.Error(), tc.want) { + t.Errorf("error = %q, want it to mention %q", err, tc.want) + } + if code := exitCode(err); code != exitUsage { + t.Errorf("exit code = %d, want %d", code, exitUsage) + } + }) + } +} + +// entitlementCreateServer is a stub of the three create endpoints. It records +// each (path, body) it receives and answers with ids the next call must pick +// up, so a break in the chain shows up as a wrong path or a wrong id. +type entitlementCreateServer struct { + srv *httptest.Server + paths []string + bodies []map[string]any + failEnt bool +} + +func newEntitlementCreateServer(t *testing.T, failEnt bool) *entitlementCreateServer { + t.Helper() + s := &entitlementCreateServer{failEnt: failEnt} + s.srv = httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + var body map[string]any + if err := json.NewDecoder(r.Body).Decode(&body); err != nil { + t.Errorf("decoding body for %s: %v", r.URL.Path, err) + } + s.paths = append(s.paths, r.URL.Path) + s.bodies = append(s.bodies, body) + w.Header().Set("Content-Type", "application/json") + switch { + case strings.HasSuffix(r.URL.Path, "/resource_types"): + _, _ = w.Write([]byte(`{"appResourceType":{"id":"rt-new"},"expanded":[]}`)) + case strings.HasSuffix(r.URL.Path, "/resources"): + _, _ = w.Write([]byte(`{"appResource":{"id":"res-new"}}`)) + case strings.HasSuffix(r.URL.Path, "/entitlements"): + if s.failEnt { + w.WriteHeader(http.StatusBadRequest) + _, _ = w.Write([]byte(`{"code":3,"message":"nope"}`)) + return + } + _, _ = w.Write([]byte(`{"appEntitlementView":{"appEntitlement":{"id":"ent-new"}}}`)) + default: + t.Errorf("unexpected path %q", r.URL.Path) + w.WriteHeader(http.StatusNotFound) + } + })) + t.Cleanup(s.srv.Close) + return s +} + +// TestEntitlementCreateRunChainsIDs drives the wired end-to-end path: each +// response's id must address the next request, and the entitlement body must +// carry both ids the server requires. +func TestEntitlementCreateRunChainsIDs(t *testing.T) { + s := newEntitlementCreateServer(t, false) + p := mustPlan(t, "--app-id", "app1", "--display-name", "Payroll admin") + + var out bytes.Buffer + cmd := &cobra.Command{} + cmd.SetOut(&out) + cmd.SetContext(context.Background()) + + if err := p.run(cmd, client.NewForTesting(s.srv.URL, s.srv.Client())); err != nil { + t.Fatalf("run: %v", err) + } + + wantPaths := []string{ + "/api/v1/apps/app1/resource_types", + "/api/v1/apps/app1/resource_types/rt-new/resources", + "/api/v1/apps/app1/entitlements", + } + if !reflect.DeepEqual(s.paths, wantPaths) { + t.Errorf("paths = %v, want %v", s.paths, wantPaths) + } + ent := s.bodies[2] + if ent["appResourceTypeId"] != "rt-new" || ent["appResourceId"] != "res-new" { + t.Errorf("entitlement body = %v, want the ids from the two preceding responses", ent) + } + if !strings.Contains(out.String(), `"id": "ent-new"`) { + t.Errorf("output = %q, want the raw entitlement response", out.String()) + } +} + +// TestEntitlementCreateRunReportsPartialCreates pins that a failure part-way +// through names what already exists and how to reuse it -- the objects are not +// rolled back, so an unnamed id would be an orphan the caller can't find. +func TestEntitlementCreateRunReportsPartialCreates(t *testing.T) { + s := newEntitlementCreateServer(t, true) + p := mustPlan(t, "--app-id", "app1", "--display-name", "Payroll admin") + + cmd := &cobra.Command{} + cmd.SetOut(new(bytes.Buffer)) + cmd.SetContext(context.Background()) + + err := p.run(cmd, client.NewForTesting(s.srv.URL, s.srv.Client())) + if err == nil { + t.Fatal("expected the entitlement create to fail") + } + for _, want := range []string{"--resource-type-id rt-new", "--resource-id res-new"} { + if !strings.Contains(err.Error(), want) { + t.Errorf("error = %q, want it to name %q", err, want) + } + } + // The 400 must still classify as a usage error, not be masked by the wrap. + if code := exitCode(err); code != exitUsage { + t.Errorf("exit code = %d, want %d", code, exitUsage) + } +} + +// TestEntitlementCreateRunReusedObjectsAreNotClaimed pins that the failure +// message only names objects THIS run created: reporting a caller-supplied id +// as "already created" would invite them to delete their own resource type. +func TestEntitlementCreateRunReusedObjectsAreNotClaimed(t *testing.T) { + s := newEntitlementCreateServer(t, true) + p := mustPlan(t, "--app-id", "app1", "--display-name", "e", "--resource-type-id", "rt-mine", "--resource-id", "res-mine") + + cmd := &cobra.Command{} + cmd.SetOut(new(bytes.Buffer)) + cmd.SetContext(context.Background()) + + err := p.run(cmd, client.NewForTesting(s.srv.URL, s.srv.Client())) + if err == nil { + t.Fatal("expected the entitlement create to fail") + } + if strings.Contains(err.Error(), "already created") { + t.Errorf("error = %q, but this run created nothing", err) + } + if len(s.paths) != 1 || s.paths[0] != "/api/v1/apps/app1/entitlements" { + t.Errorf("paths = %v, want the entitlement create only", s.paths) + } +} + +// TestEntitlementCreateDryRunPreviewsEveryRequest pins that a dry run shows all +// three writes, not just the first -- the other two are writes the caller would +// otherwise not see coming. +func TestEntitlementCreateDryRunPreviewsEveryRequest(t *testing.T) { + p := mustPlan(t, "--app-id", "app1", "--display-name", "Payroll admin") + + var out bytes.Buffer + cmd := &cobra.Command{} + cmd.SetOut(&out) + if err := p.previewRequests(cmd); err != nil { + t.Fatalf("printDryRun: %v", err) + } + + got := out.String() + for _, want := range []string{ + "[dry-run] POST /api/v1/apps/app1/resource_types\n", + "[dry-run] POST /api/v1/apps/app1/resource_types/" + newResourceTypeIDPlaceholder + "/resources\n", + "[dry-run] POST /api/v1/apps/app1/entitlements\n", + `"appResourceId": "` + newResourceIDPlaceholder + `"`, + } { + if !strings.Contains(got, want) { + t.Errorf("dry run output missing %q:\n%s", want, got) + } + } + if n := strings.Count(got, "[dry-run] POST"); n != 3 { + t.Errorf("previewed %d requests, want 3:\n%s", n, got) + } + + // A fully-specified run has nothing to stand in for, so it must not print + // placeholders at all. + out.Reset() + single := mustPlan(t, "--app-id", "app1", "--display-name", "e", "--resource-type-id", "rt1", "--resource-id", "r1") + if err := single.previewRequests(cmd); err != nil { + t.Fatalf("printDryRun: %v", err) + } + if strings.Contains(out.String(), "NEW_APP_RESOURCE") { + t.Errorf("placeholder leaked into a fully-specified preview:\n%s", out.String()) + } +} + +// TestCreatedObjectIDRejectsUnusableResponse pins that a 200 carrying no id is +// a server failure (exit 6), not a success that sends the next request to a +// path with an empty segment. +func TestCreatedObjectIDRejectsUnusableResponse(t *testing.T) { + cases := []struct { + name string + data string + }{ + {"not json", `nope`}, + {"key missing", `{"expanded":[]}`}, + {"id empty", `{"appResourceType":{"id":""}}`}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + id, err := createdObjectID([]byte(tc.data), "appResourceType") + if err == nil { + t.Fatalf("id = %q, want an error", id) + } + var nonJSON *nonJSONResponseError + if !errors.As(err, &nonJSON) { + t.Errorf("error %v is not a *nonJSONResponseError, so it would exit 1", err) + } + if code := exitCode(err); code != exitServer { + t.Errorf("exit code = %d, want %d", code, exitServer) + } + }) + } +} + +// TestResourceTypeKindsAreDocumented holds the one enum list to every place +// that restates it. The command deliberately does NOT validate --resource-type +// against this list -- the server stays authoritative if the enum grows -- but +// the help text and docs freeze it either way, and a contract restated in four +// places drifts unless something fails when it does. +func TestResourceTypeKindsAreDocumented(t *testing.T) { + docs := map[string]string{ + "entitlements create --help": entitlementsCreateCmd.Long, + "--resource-type flag usage": entitlementsCreateCmd.Flags().Lookup("resource-type").Usage, + "README.md": readDocFile(t, "../README.md"), + "guideConfigureNewApp": guideConfigureNewApp, + } + for name, doc := range docs { + for _, kind := range resourceTypeKinds { + if !strings.Contains(doc, kind) { + t.Errorf("%s does not name the %q resource type kind", name, kind) + } + } + } + + // The default must be one of them, or --help promises a value that 400s. + def := entitlementsCreateCmd.Flags().Lookup("resource-type").DefValue + if !slices.Contains(resourceTypeKinds, def) { + t.Errorf("--resource-type defaults to %q, which is not in resourceTypeKinds", def) + } +} + +// TestEntitlementCreateDryRunBannerNamesOnlyUsedPlaceholders pins that the +// banner announces exactly the stand-ins the preview goes on to use. Naming +// one that never appears is the one place a dry run could overclaim. +func TestEntitlementCreateDryRunBannerNamesOnlyUsedPlaceholders(t *testing.T) { + cases := []struct { + name string + args []string + want []string + notWant []string + }{ + { + "creates both", + []string{"--app-id", "app1", "--display-name", "e"}, + []string{newResourceTypeIDPlaceholder, newResourceIDPlaceholder, "stand in for ids"}, + []string{"stands in for an id"}, + }, + { + "reuses the resource type", + []string{"--app-id", "app1", "--display-name", "e", "--resource-type-id", "rt1"}, + []string{newResourceIDPlaceholder, "stands in for an id"}, + []string{newResourceTypeIDPlaceholder}, + }, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + var out bytes.Buffer + cmd := &cobra.Command{} + cmd.SetOut(&out) + if err := mustPlan(t, tc.args...).previewRequests(cmd); err != nil { + t.Fatalf("previewRequests: %v", err) + } + banner, _, _ := strings.Cut(out.String(), "\n") + for _, w := range tc.want { + if !strings.Contains(banner, w) { + t.Errorf("banner %q does not name %s, which the preview uses", banner, w) + } + } + for _, w := range tc.notWant { + if strings.Contains(banner, w) { + t.Errorf("banner %q claims %q, which does not describe this preview", banner, w) + } + // A placeholder must not appear anywhere in a preview whose + // id was supplied, banner or request. + if strings.HasPrefix(w, "NEW_") && strings.Contains(out.String(), w) { + t.Errorf("preview used %s despite the id being supplied:\n%s", w, out.String()) + } + } + }) + } +} + +// TestEntitlementCreateDurationGrantWireKey pins the flag-name-to-field +// mapping: --duration-grant is the one flag whose wire key differs from its +// name, and a mismatch would be silently dropped by the server. +func TestEntitlementCreateDurationGrantWireKey(t *testing.T) { + p := mustPlan(t, "--app-id", "app1", "--display-name", "e", "--duration-grant", "3600s") + if got := p.entitlementBody["durationGrant"]; got != "3600s" { + t.Errorf("durationGrant = %v, want 3600s (body: %v)", got, p.entitlementBody) + } + if _, ok := p.entitlementBody["duration-grant"]; ok { + t.Error("the flag name leaked into the body as a wire key") + } + + // Omitted means standing access; an empty key would pick the wrong arm of + // the max_grant_duration oneof. + bare := mustPlan(t, "--app-id", "app1", "--display-name", "e") + if _, ok := bare.entitlementBody["durationGrant"]; ok { + t.Errorf("durationGrant sent without the flag: %v", bare.entitlementBody) + } +} + +// TestNormalizeResourceType pins case/separator normalization, and that an +// unknown value is passed through for the server to rule on rather than being +// rejected against a list that can go stale. +func TestNormalizeResourceType(t *testing.T) { + cases := map[string]string{ + "custom": "CUSTOM", + "CUSTOM": "CUSTOM", + "profile-type": "PROFILE_TYPE", + "profile_type": "PROFILE_TYPE", + "something_new": "SOMETHING_NEW", + } + for in, want := range cases { + if got := normalizeResourceType(in); got != want { + t.Errorf("normalizeResourceType(%q) = %q, want %q", in, got, want) + } + } +} diff --git a/cmd/flags.go b/cmd/flags.go index 2e79bf6..cdfb0a2 100644 --- a/cmd/flags.go +++ b/cmd/flags.go @@ -244,3 +244,25 @@ func requireNonEmpty(cmd *cobra.Command, names ...string) error { return &usageError{fmt.Errorf("flags %s require non-empty values", strings.Join(missing, ", "))} } } + +// requireNonEmptyIfSet errors when a flag was passed with an empty value, while +// leaving an unset flag alone. Distinct from requireNonEmpty, which is for +// flags that must always be present: use this for an *optional* flag whose +// empty value would change behavior rather than fail — an id flag selecting an +// existing object (`--resource-type-id "$RT_ID"` with RT_ID unset would +// silently create a second one), or one that scopes a read. +func requireNonEmptyIfSet(cmd *cobra.Command, name string) (string, error) { + v, _ := cmd.Flags().GetString(name) + if v == "" && cmd.Flags().Changed(name) { + return "", &usageError{fmt.Errorf("flag --%s requires a non-empty value", name)} + } + return v, nil +} + +// flagOrDefault returns a string flag's value, falling back to def when unset. +func flagOrDefault(cmd *cobra.Command, name, def string) string { + if v, _ := cmd.Flags().GetString(name); v != "" { + return v + } + return def +} From 99d14821d5e55a93503f76e597134fabda5cb47d Mon Sep 17 00:00:00 2001 From: leet-c1 <264029741+leet-c1@users.noreply.github.com> Date: Tue, 1 Sep 2026 05:35:11 +0000 Subject: [PATCH 2/8] fix: reject empty values in "entitlements create", and make its retry advice work --owner-id "" (an unset shell variable) created an ownerless entitlement at exit 0, which an async owner read cannot tell apart from "not provisioned yet". Empty and whitespace-only owner ids are now a usage error before anything is sent; the flag is a StringArray because pflag's slice parser drops an empty value outright, so `--owner-id "" --owner-id U` never reached the check. An explicit --resource-type-display-name/--resource-display-name "" likewise fell back to --display-name and mis-named the object; both now take the same requireNonEmptyIfSet guard as the id flags. The partial-failure message advised a retry the command then rejected: the reused ids make the create-only flags of the original invocation a usage error. It now names those flags for dropping, and a test parses the message and feeds the retry back through the flag parser. Help text: only a CUSTOM resource type can repeat on one app -- a second ROLE, GROUP or VAULT 500s with "app resource type already exists", quoted so it is greppable from both directions, and reused via --resource-type-id. The exit code for it is unchanged. The opening sentence also claimed the app must be manually managed; the app need not be, only the entitlement created on it is. Co-Authored-By: Claude Opus 5 --- CHANGELOG.md | 13 +++- README.md | 13 ++-- cmd/agents.md | 8 ++- cmd/entitlements_create.go | 87 +++++++++++++++++++++----- cmd/entitlements_create_test.go | 107 ++++++++++++++++++++++++++++++++ 5 files changed, 206 insertions(+), 22 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3f6387a..024cf03 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -17,8 +17,17 @@ to follow [Semantic Versioning](https://semver.org/spec/v2.0.0.html). rather than a hand-written body field. On a partial failure nothing is rolled back; the error names what that run - created and the flags to re-run with, so a retry reuses those objects - instead of duplicating them. `--dry-run` previews all three requests. + created, the flags to re-run with, and the create-only flags that retry has + to drop, so the command it prints is one that works. `--dry-run` previews + all three requests. + + Empty values are usage errors (exit 2) before anything is sent, rather than + silent fallbacks: `--owner-id ""` from an unset shell variable would have + created an ownerless entitlement at exit 0, and an empty + `--resource-type-display-name`/`--resource-display-name` would have quietly + reused `--display-name`. Only a `CUSTOM` resource type can repeat on one + app; the help now says so, quoting the server's own + `app resource type already exists`. ### Changed diff --git a/README.md b/README.md index ebeb257..3906c6d 100644 --- a/README.md +++ b/README.md @@ -125,8 +125,12 @@ required, so `--resource-id` without `--resource-type-id` is rejected at exit `CUSTOM`, `VAULT` or `PROFILE_TYPE` (case-insensitive here, uppercase on the wire) and describes the resource type this command creates, so passing it together with `--resource-type-id` is a usage error rather than a silently -ignored flag. `--owner-id` is repeatable and goes inline in the create request, -so no follow-up call is needed. `--duration-grant` takes a protobuf duration — +ignored flag. Only `CUSTOM` can repeat on one app: a second resource type of +any other kind fails with a 500 (exit `6`, though retrying never helps) saying +`app resource type already exists`, so reuse the existing one with +`--resource-type-id`. `--owner-id` is repeatable and goes inline in the create +request, so no follow-up call is needed; an empty one is a usage error rather +than an owner quietly dropped. `--duration-grant` takes a protobuf duration — seconds with an `s` suffix, e.g. `3600s`; a Go-style `1h` is refused by the server. Omit it for standing access. @@ -134,8 +138,9 @@ server. Omit it for standing access. `NEW_APP_RESOURCE_TYPE_ID`/`NEW_APP_RESOURCE_ID` where an id only exists after a real preceding step. There is no rollback: if a later step fails, the objects the earlier ones created still exist, and the error names them along with the -flags that reuse them. The created entitlement comes back as pretty JSON under -`appEntitlementView` (`--fields` is never applied to mutation output); it echoes +flags that reuse them and the create-only flags the retry has to drop. The +created entitlement comes back as pretty JSON under `appEntitlementView` +(`--fields` is never applied to mutation output); it echoes `appResourceTypeId`/`appResourceId` and expands both objects, so every id the command touched is in that one payload. diff --git a/cmd/agents.md b/cmd/agents.md index 4ee0a8d..3b11003 100644 --- a/cmd/agents.md +++ b/cmd/agents.md @@ -245,8 +245,12 @@ One command sends more than one write: `entitlements create` POSTs a resource type, a resource, then the entitlement, skipping the steps whose id you supply via `--resource-type-id`/`--resource-id`. Its `--dry-run` previews all three. There is no rollback, so a failure part-way through leaves the earlier objects -behind; the error names them and the flags that reuse them, and re-running -without those flags creates duplicates. +behind; the error names them, the flags that reuse them, and the create-only +flags the retry has to drop, and re-running without those flags creates +duplicates. Only a `CUSTOM` resource type can repeat on one app: a second +`--resource-type` of any other kind fails with a 500 (exit `6`, though +retrying never helps) saying `app resource type already exists` -- reuse the +existing type with `--resource-type-id`. ## Things that will surprise you diff --git a/cmd/entitlements_create.go b/cmd/entitlements_create.go index c28c581..462ef76 100644 --- a/cmd/entitlements_create.go +++ b/cmd/entitlements_create.go @@ -27,7 +27,10 @@ const ( var entitlementsCreateCmd = &cobra.Command{ Use: "create", Short: "Create an app entitlement, with its resource type and resource (pretty JSON)", - Long: `Create an app entitlement on a manually-managed app. + Long: `Create an app entitlement, plus the app resource type and resource it +points at. The app itself need not be manually managed -- the entitlement this +creates is (isManuallyManaged is true on it, false on the app it was created +on). An entitlement points at an app resource, which lives under an app resource type, so this is up to three POSTs in order: @@ -56,6 +59,10 @@ it together with --resource-type-id is a usage error rather than a silently ignored flag. Case-insensitive here; the API itself takes only the uppercase form. One of: ` + strings.Join(resourceTypeKinds, ", ") + ` +Only CUSTOM can repeat on one app: a second resource type of any other kind +fails with a 500 (exit 6, though retrying never helps): + app resource type already exists +Reuse the one that exists with --resource-type-id instead. Omitting --duration-grant leaves the entitlement at standing access (durationUnset in the response). It takes a protobuf duration, not a Go one -- @@ -63,7 +70,8 @@ seconds with an "s" suffix, e.g. 3600s. "1h" is refused by the server with: invalid google.protobuf.Duration value "1h" Assign owners inline with --owner-id: the create request carries -appEntitlementOwnerIds, so no follow-up call is needed. Owner provisioning is +appEntitlementOwnerIds, so no follow-up call is needed. An empty --owner-id is +a usage error, not an owner quietly dropped. Owner provisioning is asynchronous — one measured create took 116s to read back on "GET /api/v1/apps/{app-id}/entitlements/{id}/ownerids". @@ -72,9 +80,10 @@ after a real step 1 or 2 print as ` + newResourceTypeIDPlaceholder + `/` + newResourceIDPlaceholder + `. There is no rollback. The three creates are independent, so if step 2 or 3 -fails the objects earlier steps created still exist; the error names them and -the flags that reuse them, verbatim: - (already created: re-run with --resource-type-id to reuse instead of duplicating) +fails the objects earlier steps created still exist; the error names them, the +flags that reuse them, and the create-only flags a retry has to drop (a reused +id makes those a usage error), verbatim: + (already created: re-run with --resource-type-id , dropping --resource-type-display-name, to reuse instead of duplicating) Prints the created entitlement as pretty JSON under "appEntitlementView" (--fields is not applied to mutation output). The response echoes @@ -126,6 +135,12 @@ type entitlementCreatePlan struct { resourceID string resourceBody map[string]any + // Create-only flags this invocation passed, per object. A retry that + // reuses the object has to drop them; rejectFlagsForReusedObject refuses + // them alongside the id createdSoFar hands back. + typeOnlyFlags []string + resourceOnlyFlags []string + // entitlementBody carries everything except appResourceTypeId and // appResourceId, which are only known once the steps above have run. entitlementBody map[string]any @@ -167,6 +182,14 @@ func buildEntitlementCreatePlan(cmd *cobra.Command) (*entitlementCreatePlan, err return nil, err } + // Same for the display-name overrides: an explicit "" would fall back to + // --display-name and mis-name the object rather than fail. + for _, n := range []string{"resource-type-display-name", "resource-display-name"} { + if _, err := requireNonEmptyIfSet(cmd, n); err != nil { + return nil, err + } + } + p := &entitlementCreatePlan{ appID: appID, resourceTypeID: resourceTypeID, @@ -179,11 +202,13 @@ func buildEntitlementCreatePlan(cmd *cobra.Command) (*entitlementCreatePlan, err "displayName": flagOrDefault(cmd, "resource-type-display-name", displayName), "resourceType": normalizeResourceType(resourceType), } + p.typeOnlyFlags = changedFlags(cmd, "resource-type", "resource-type-display-name") } if resourceID == "" { p.resourceBody = map[string]any{ "displayName": flagOrDefault(cmd, "resource-display-name", displayName), } + p.resourceOnlyFlags = changedFlags(cmd, "resource-display-name") } // Flag name -> request field, equal except where the wire key is camelCase. @@ -197,7 +222,20 @@ func buildEntitlementCreatePlan(cmd *cobra.Command) (*entitlementCreatePlan, err p.entitlementBody[key] = v } } - if owners, _ := cmd.Flags().GetStringSlice("owner-id"); len(owners) > 0 { + // An owner id from an unset shell variable would otherwise create the + // entitlement with fewer owners than asked for, and owner reads are async, + // so nothing downstream can tell that apart from "not provisioned yet". + // pflag's CSV round-trip drops a lone `--owner-id ""` entirely, so an empty + // owner arrives either as a missing value or an empty one. + owners, _ := cmd.Flags().GetStringArray("owner-id") + empty := cmd.Flags().Changed("owner-id") && len(owners) == 0 + for _, id := range owners { + empty = empty || strings.TrimSpace(id) == "" + } + if empty { + return nil, &usageError{fmt.Errorf("--owner-id values must be non-empty")} + } + if len(owners) > 0 { p.entitlementBody["appEntitlementOwnerIds"] = owners } @@ -211,12 +249,21 @@ func rejectFlagsForReusedObject(cmd *cobra.Command, reusedID, reusedFlag string, if reusedID == "" { return nil } + if passed := changedFlags(cmd, names...); len(passed) > 0 { + return &usageError{fmt.Errorf("%s only applies when this command creates that object; drop it or drop %s", passed[0], reusedFlag)} + } + return nil +} + +// changedFlags returns the named flags the caller actually passed, "--"-prefixed. +func changedFlags(cmd *cobra.Command, names ...string) []string { + var passed []string for _, n := range names { if cmd.Flags().Changed(n) { - return &usageError{fmt.Errorf("--%s only applies when this command creates that object; drop it or drop %s", n, reusedFlag)} + passed = append(passed, "--"+n) } } - return nil + return passed } // normalizeResourceType upper-cases the enum name so "custom" and @@ -285,21 +332,30 @@ func (p *entitlementCreatePlan) run(cmd *cobra.Command, c *client.Client) error return writeRawObject(cmd, data) } -// createdSoFar names the objects THIS invocation created before failing, and -// the flags that reuse them. Nothing is rolled back: a compensating delete can -// fail too, which would leave a worse state described by a less honest message. +// createdSoFar names the objects THIS invocation created before failing, the +// flags that reuse them, and the create-only flags the retry must drop. +// Nothing is rolled back: a compensating delete can fail too, which would +// leave a worse state described by a less honest message. func (p *entitlementCreatePlan) createdSoFar(resourceTypeID, resourceID string) string { - var parts []string + var parts, drop []string if p.resourceTypeBody != nil && resourceTypeID != "" { parts = append(parts, "--resource-type-id "+resourceTypeID) + drop = append(drop, p.typeOnlyFlags...) } if p.resourceBody != nil && resourceID != "" { parts = append(parts, "--resource-id "+resourceID) + drop = append(drop, p.resourceOnlyFlags...) } if len(parts) == 0 { return "" } - return " (already created: re-run with " + strings.Join(parts, " ") + " to reuse instead of duplicating)" + msg := " (already created: re-run with " + strings.Join(parts, " ") + if len(drop) > 0 { + // Named, not merely implied: the reused id makes these a usage error, + // so a retry that kept them would exit 2. + msg += ", dropping " + strings.Join(drop, " ") + "," + } + return msg + " to reuse instead of duplicating)" } // previewRequests previews every request in the sequence, not just the first — @@ -373,7 +429,10 @@ func addEntitlementCreateFlags(cmd *cobra.Command) { f.String("slug", "", "Slug for the new entitlement (e.g. member)") f.String("alias", "", "Alias for the new entitlement; exact-match queryable") f.String("duration-grant", "", "Maximum grant duration as a protobuf duration, e.g. 3600s; omit for standing access") - f.StringSlice("owner-id", nil, "C1 user ID to own the new entitlement (repeatable)") + // StringArray, not StringSlice: the slice parser drops an empty value + // outright, so `--owner-id "" --owner-id U` would lose the empty one + // before this command could reject it. + f.StringArray("owner-id", nil, "C1 user ID to own the new entitlement (repeatable)") f.String("resource-type", "CUSTOM", "Kind of resource type to create: "+strings.Join(resourceTypeKinds, ", ")) f.String("resource-type-id", "", "Existing app resource type to reuse instead of creating one") f.String("resource-type-display-name", "", "Display name for the resource type this command creates (default: --display-name)") diff --git a/cmd/entitlements_create_test.go b/cmd/entitlements_create_test.go index c5cf26b..122a9eb 100644 --- a/cmd/entitlements_create_test.go +++ b/cmd/entitlements_create_test.go @@ -176,6 +176,36 @@ func TestEntitlementCreatePlanUsageErrors(t *testing.T) { []string{"--app-id", "app1", "--display-name", "e", "--resource-type-id", "rt1", "--resource-id", ""}, "flag --resource-id requires a non-empty value", }, + { + // Both display-name overrides would otherwise fall back to + // --display-name and mis-name the object. + "empty resource type display name", + []string{"--app-id", "app1", "--display-name", "e", "--resource-type-display-name", ""}, + "flag --resource-type-display-name requires a non-empty value", + }, + { + "empty resource display name", + []string{"--app-id", "app1", "--display-name", "e", "--resource-display-name", ""}, + "flag --resource-display-name requires a non-empty value", + }, + { + // An unset shell variable: the entitlement would otherwise be + // created ownerless at exit 0, and an async owner read cannot tell + // that apart from "not provisioned yet". + "empty owner id", + []string{"--app-id", "app1", "--display-name", "e", "--owner-id", ""}, + "--owner-id values must be non-empty", + }, + { + "empty owner id alongside a real one", + []string{"--app-id", "app1", "--display-name", "e", "--owner-id", "", "--owner-id", "u1"}, + "--owner-id values must be non-empty", + }, + { + "whitespace-only owner id", + []string{"--app-id", "app1", "--display-name", "e", "--owner-id", " "}, + "--owner-id values must be non-empty", + }, } for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { @@ -501,3 +531,80 @@ func TestNormalizeResourceType(t *testing.T) { } } } + +// TestEntitlementCreateRetryRemediationIsAccepted pins that the retry the +// failure message hands back is a command that works: the ids it names make +// the create-only flags of the original invocation a usage error, so the +// message has to name those for dropping too. +func TestEntitlementCreateRetryRemediationIsAccepted(t *testing.T) { + s := newEntitlementCreateServer(t, true) + args := []string{ + "--app-id", "app1", "--display-name", "e", + "--resource-type", "ROLE", + "--resource-type-display-name", "rt name", + "--resource-display-name", "res name", + } + + cmd := &cobra.Command{} + cmd.SetOut(new(bytes.Buffer)) + cmd.SetContext(context.Background()) + runErr := mustPlan(t, args...).run(cmd, client.NewForTesting(s.srv.URL, s.srv.Client())) + if runErr == nil { + t.Fatal("expected the entitlement create to fail") + } + + add, drop := parseRemediation(t, runErr.Error()) + retry := append(withoutFlags(args, drop), add...) + if _, err := buildEntitlementCreatePlan(newEntitlementCreateCmd(t, retry...)); err != nil { + t.Fatalf("the remediation %q produces %v, so the retry it advises exits 2", runErr, err) + } +} + +// parseRemediation pulls the flags to add and the flags to drop out of the +// "(already created: ...)" tail. +func parseRemediation(t *testing.T, msg string) (add, drop []string) { + t.Helper() + _, rest, ok := strings.Cut(msg, "re-run with ") + if !ok { + t.Fatalf("error %q names no retry", msg) + } + rest, _, ok = strings.Cut(rest, " to reuse instead of duplicating)") + if !ok { + t.Fatalf("error %q has no remediation tail", msg) + } + addPart, dropPart, hasDrop := strings.Cut(rest, ", dropping ") + if hasDrop { + drop = strings.Fields(strings.TrimSuffix(dropPart, ",")) + } + return strings.Fields(addPart), drop +} + +// withoutFlags removes each named flag and its value from an argument list of +// "--flag value" pairs. +func withoutFlags(args, remove []string) []string { + var kept []string + for i := 0; i+1 < len(args); i += 2 { + if !slices.Contains(remove, args[i]) { + kept = append(kept, args[i], args[i+1]) + } + } + return kept +} + +// TestResourceTypeSingletonIsDocumented holds the server's own error string in +// every doc that promises the restriction, so it stays greppable from both +// directions. Reproduced live: a second ROLE, GROUP or VAULT resource type on +// one app 500s, while a second CUSTOM one succeeds. +func TestResourceTypeSingletonIsDocumented(t *testing.T) { + const serverError = "app resource type already exists" + docs := map[string]string{ + "entitlements create --help": entitlementsCreateCmd.Long, + "README.md": readDocFile(t, "../README.md"), + "cmd/agents.md (embedded)": agentsTemplate, + } + for name, doc := range docs { + if !strings.Contains(doc, serverError) { + t.Errorf("%s does not quote the server's %q error", name, serverError) + } + } +} From 7038639495f7089fa832d651c682b2bfc5b312b1 Mon Sep 17 00:00:00 2001 From: leet-c1 <264029741+leet-c1@users.noreply.github.com> Date: Thu, 3 Sep 2026 00:48:22 +0000 Subject: [PATCH 3/8] Carry the create fixes into the guide, and pin them with tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The partial-failure message and the CUSTOM-singleton caveat had landed in the command help, README and agents.md but not in docs guide configure-new-app, which was rewritten in this same branch. The guide quoted the pre-fix retry — one its own example command cannot produce, and the one the fix exists to stop advising — and listed all eight resource-type kinds with no hint that only CUSTOM may repeat. Two guards so neither drifts again: the singleton check now covers the guide, and a new test builds the remediation from createdSoFar and requires the docs that quote it to carry the flags-to-drop clause. The remediation test also covers the resource-create failure, where the retry must keep --resource-display-name because the object it names was never created. That path fails silently rather than loudly, and nothing pinned it. Co-Authored-By: Claude Opus 5 --- cmd/docs_guide.go | 16 ++++-- cmd/entitlements_create_test.go | 86 +++++++++++++++++++++++++++++++-- 2 files changed, 95 insertions(+), 7 deletions(-) diff --git a/cmd/docs_guide.go b/cmd/docs_guide.go index 2697a92..89c6a21 100644 --- a/cmd/docs_guide.go +++ b/cmd/docs_guide.go @@ -433,7 +433,12 @@ type per entitlement: --resource-type-id "$RT_ID" --resource-id "$RES_ID" # entitlement only --resource-type (the kind of resource type to create, default CUSTOM) is one -of ROLE, GROUP, LICENSE, PROJECT, CATALOG, CUSTOM, VAULT, PROFILE_TYPE. +of ROLE, GROUP, LICENSE, PROJECT, CATALOG, CUSTOM, VAULT, PROFILE_TYPE. Only +CUSTOM can repeat on one app: a second resource type of any other kind fails +with a 500, + app resource type already exists +which maps to exit 6 even though retrying never helps. Reuse the one that +exists with --resource-type-id instead. --owner-id is repeatable and rides along in the create request, so entitlement owners need no follow-up call -- but the read lags the write the same way app @@ -442,9 +447,12 @@ owners do (one measured create took 116s to show up on The three writes are not atomic and nothing is rolled back: if a later one fails, the objects the earlier ones created still exist, and the error names -them and the flags that reuse them, e.g. "(already created: re-run with ---resource-type-id to reuse instead of duplicating)". "--dry-run" -previews all three requests. +them, the flags that reuse them, and the flags to drop, e.g. "(already +created: re-run with --resource-type-id , dropping +--resource-type-display-name, to reuse instead of duplicating)". Following it +verbatim is what makes the retry succeed: keeping a flag that describes an +object which now already exists is refused. "--dry-run" previews all three +requests. Omitting --duration-grant defaults the entitlement to standing access (durationUnset). For time-boxed access pass a protobuf duration -- seconds diff --git a/cmd/entitlements_create_test.go b/cmd/entitlements_create_test.go index 122a9eb..20dad78 100644 --- a/cmd/entitlements_create_test.go +++ b/cmd/entitlements_create_test.go @@ -231,6 +231,7 @@ type entitlementCreateServer struct { paths []string bodies []map[string]any failEnt bool + failRes bool } func newEntitlementCreateServer(t *testing.T, failEnt bool) *entitlementCreateServer { @@ -248,6 +249,11 @@ func newEntitlementCreateServer(t *testing.T, failEnt bool) *entitlementCreateSe case strings.HasSuffix(r.URL.Path, "/resource_types"): _, _ = w.Write([]byte(`{"appResourceType":{"id":"rt-new"},"expanded":[]}`)) case strings.HasSuffix(r.URL.Path, "/resources"): + if s.failRes { + w.WriteHeader(http.StatusBadRequest) + _, _ = w.Write([]byte(`{"code":3,"message":"nope"}`)) + return + } _, _ = w.Write([]byte(`{"appResource":{"id":"res-new"}}`)) case strings.HasSuffix(r.URL.Path, "/entitlements"): if s.failEnt { @@ -598,9 +604,10 @@ func withoutFlags(args, remove []string) []string { func TestResourceTypeSingletonIsDocumented(t *testing.T) { const serverError = "app resource type already exists" docs := map[string]string{ - "entitlements create --help": entitlementsCreateCmd.Long, - "README.md": readDocFile(t, "../README.md"), - "cmd/agents.md (embedded)": agentsTemplate, + "entitlements create --help": entitlementsCreateCmd.Long, + "README.md": readDocFile(t, "../README.md"), + "cmd/agents.md (embedded)": agentsTemplate, + "docs guide configure-new-app": guideConfigureNewApp, } for name, doc := range docs { if !strings.Contains(doc, serverError) { @@ -608,3 +615,76 @@ func TestResourceTypeSingletonIsDocumented(t *testing.T) { } } } + +// TestRemediationStringIsDocumentedVerbatim pins the docs that quote the +// partial-failure message to what createdSoFar actually emits. The message +// changed to name the flags a retry must drop, and the guide kept quoting the +// old form in the same commit — a retry copied from it would be refused. +func TestRemediationStringIsDocumentedVerbatim(t *testing.T) { + plan := &entitlementCreatePlan{ + resourceTypeBody: map[string]any{}, + typeOnlyFlags: []string{"--resource-type-display-name"}, + } + msg := plan.createdSoFar("", "") + for _, frag := range []string{"dropping", "to reuse instead of duplicating"} { + if !strings.Contains(msg, frag) { + t.Fatalf("createdSoFar no longer contains %q; this guard is checking the wrong thing", frag) + } + } + docs := map[string]string{ + "entitlements create --help": entitlementsCreateCmd.Long, + "docs guide configure-new-app": guideConfigureNewApp, + } + for name, doc := range docs { + if !strings.Contains(flattenDoc(doc), "dropping") { + t.Errorf("%s quotes the partial-failure retry without the flags-to-drop clause; "+ + "a reader following it verbatim gets exit 2", name) + } + } +} + +// flattenDoc collapses whitespace so a quoted message still matches when a doc +// wraps it across lines. +func flattenDoc(s string) string { return strings.Join(strings.Fields(s), " ") } + +// TestRemediationOnResourceFailureKeepsResourceName covers the step-2 failure: +// the resource type exists, the resource does not. The retry must drop the +// type-only flags but KEEP --resource-display-name, since the object it names +// was never created. Getting that wrong is silent — the resource would be +// created under --display-name instead — where the step-3 case fails loudly. +func TestRemediationOnResourceFailureKeepsResourceName(t *testing.T) { + s := newEntitlementCreateServer(t, false) + s.failRes = true + + args := []string{ + "--app-id", "app1", "--display-name", "e", + "--resource-type", "ROLE", + "--resource-type-display-name", "rt name", + "--resource-display-name", "res name", + } + cmd := &cobra.Command{} + cmd.SetOut(new(bytes.Buffer)) + cmd.SetContext(context.Background()) + runErr := mustPlan(t, args...).run(cmd, client.NewForTesting(s.srv.URL, s.srv.Client())) + if runErr == nil { + t.Fatal("expected the resource create to fail") + } + msg := runErr.Error() + + // The advised retry must also be accepted, same as the step-3 case. + add, drop := parseRemediation(t, msg) + retry := append(withoutFlags(args, drop), add...) + if _, err := buildEntitlementCreatePlan(newEntitlementCreateCmd(t, retry...)); err != nil { + t.Fatalf("the step-2 remediation %q produces %v, so the retry it advises exits 2", msg, err) + } + if !strings.Contains(msg, "--resource-type-id") { + t.Errorf("message does not name the created resource type: %q", msg) + } + if strings.Contains(msg, "--resource-id") { + t.Errorf("message names a resource id, but the resource was never created: %q", msg) + } + if strings.Contains(msg, "--resource-display-name") { + t.Errorf("message tells the caller to drop --resource-display-name, but the resource "+ + "it names does not exist yet; the retry would create it under --display-name: %q", msg) + } +} From b8755821fd8116a875797a46c3263274e4b7ad49 Mon Sep 17 00:00:00 2001 From: leet-c1 <264029741+leet-c1@users.noreply.github.com> Date: Thu, 3 Sep 2026 01:03:47 +0000 Subject: [PATCH 4/8] Check the message verbatim, and fix the retry the new guide text advised MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The verbatim test built the expected message and then asserted only that the docs contain the word "dropping", so renaming a flag left both docs quoting a message the code can no longer emit while the test stayed green. It now compares against the message itself, built by the real planner rather than a hand-made plan, so a regression in how the drop list is populated fails here. The singleton paragraph added in the previous commit told the reader to reuse the existing type with --resource-type-id without saying to drop --resource-type, so following it exits 2 — the same class of guide-advised retry that commit removed from this file. Co-Authored-By: Claude Opus 5 --- cmd/docs_guide.go | 4 +++- cmd/entitlements_create_test.go | 24 ++++++++++++------------ 2 files changed, 15 insertions(+), 13 deletions(-) diff --git a/cmd/docs_guide.go b/cmd/docs_guide.go index 89c6a21..33c5516 100644 --- a/cmd/docs_guide.go +++ b/cmd/docs_guide.go @@ -438,7 +438,9 @@ CUSTOM can repeat on one app: a second resource type of any other kind fails with a 500, app resource type already exists which maps to exit 6 even though retrying never helps. Reuse the one that -exists with --resource-type-id instead. +exists by replacing --resource-type with --resource-type-id: the two are +mutually exclusive, so adding the id without dropping --resource-type is +refused at exit 2. --owner-id is repeatable and rides along in the create request, so entitlement owners need no follow-up call -- but the read lags the write the same way app diff --git a/cmd/entitlements_create_test.go b/cmd/entitlements_create_test.go index 20dad78..a61916a 100644 --- a/cmd/entitlements_create_test.go +++ b/cmd/entitlements_create_test.go @@ -621,24 +621,24 @@ func TestResourceTypeSingletonIsDocumented(t *testing.T) { // changed to name the flags a retry must drop, and the guide kept quoting the // old form in the same commit — a retry copied from it would be refused. func TestRemediationStringIsDocumentedVerbatim(t *testing.T) { - plan := &entitlementCreatePlan{ - resourceTypeBody: map[string]any{}, - typeOnlyFlags: []string{"--resource-type-display-name"}, - } - msg := plan.createdSoFar("", "") - for _, frag := range []string{"dropping", "to reuse instead of duplicating"} { - if !strings.Contains(msg, frag) { - t.Fatalf("createdSoFar no longer contains %q; this guard is checking the wrong thing", frag) - } + // Built by the real planner, so a regression that stopped populating + // typeOnlyFlags fails here instead of leaving a hand-made plan green. + plan := mustPlan(t, "--app-id", "a", "--display-name", "e", + "--resource-type-display-name", "rt") + msg := strings.TrimSpace(plan.createdSoFar("", "")) + if !strings.Contains(msg, "dropping --resource-type-display-name") { + t.Fatalf("createdSoFar produced %q, which names no flag to drop; "+ + "this guard would then pass on any doc", msg) } docs := map[string]string{ "entitlements create --help": entitlementsCreateCmd.Long, "docs guide configure-new-app": guideConfigureNewApp, } for name, doc := range docs { - if !strings.Contains(flattenDoc(doc), "dropping") { - t.Errorf("%s quotes the partial-failure retry without the flags-to-drop clause; "+ - "a reader following it verbatim gets exit 2", name) + // Verbatim, not a keyword: renaming the flag must break the docs that + // quote it, which a check for the word "dropping" alone would not. + if !strings.Contains(flattenDoc(doc), msg) { + t.Errorf("%s does not quote the partial-failure message verbatim.\nwant: %s", name, msg) } } } From 1eba543733eb63e34b8859411330cb5f14d057f5 Mon Sep 17 00:00:00 2001 From: leet-c1 <264029741+leet-c1@users.noreply.github.com> Date: Thu, 3 Sep 2026 01:14:29 +0000 Subject: [PATCH 5/8] Name both flags a reused resource type refuses, in all four docs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The previous commit fixed half of a two-flag rejection: reusing a type also refuses --resource-type-display-name, which the guide's own worked example passes, so following the corrected paragraph verbatim still exited 2. The other three docs carried the same incomplete advice, and the agent doc stated no exclusivity at all. The refused flags are now one list in the code, and a test holds every doc that advises the reuse to it — so adding a third flag fails until the docs say so. Prose is not a quote, which is why the verbatim guard could not see this. Co-Authored-By: Claude Opus 5 --- README.md | 3 ++- cmd/agents.md | 3 ++- cmd/docs_guide.go | 6 +++--- cmd/entitlements_create.go | 11 +++++++++-- cmd/entitlements_create_test.go | 29 +++++++++++++++++++++++++++++ 5 files changed, 45 insertions(+), 7 deletions(-) diff --git a/README.md b/README.md index 3906c6d..66d54bb 100644 --- a/README.md +++ b/README.md @@ -128,7 +128,8 @@ together with `--resource-type-id` is a usage error rather than a silently ignored flag. Only `CUSTOM` can repeat on one app: a second resource type of any other kind fails with a 500 (exit `6`, though retrying never helps) saying `app resource type already exists`, so reuse the existing one with -`--resource-type-id`. `--owner-id` is repeatable and goes inline in the create +`--resource-type-id` and drop both `--resource-type` and +`--resource-type-display-name` — either one alongside the id is a usage error. `--owner-id` is repeatable and goes inline in the create request, so no follow-up call is needed; an empty one is a usage error rather than an owner quietly dropped. `--duration-grant` takes a protobuf duration — seconds with an `s` suffix, e.g. `3600s`; a Go-style `1h` is refused by the diff --git a/cmd/agents.md b/cmd/agents.md index 3b11003..26c2de7 100644 --- a/cmd/agents.md +++ b/cmd/agents.md @@ -250,7 +250,8 @@ flags the retry has to drop, and re-running without those flags creates duplicates. Only a `CUSTOM` resource type can repeat on one app: a second `--resource-type` of any other kind fails with a 500 (exit `6`, though retrying never helps) saying `app resource type already exists` -- reuse the -existing type with `--resource-type-id`. +existing type with `--resource-type-id`, dropping both `--resource-type` and +`--resource-type-display-name`; either alongside the id is exit 2. ## Things that will surprise you diff --git a/cmd/docs_guide.go b/cmd/docs_guide.go index 33c5516..691a8c9 100644 --- a/cmd/docs_guide.go +++ b/cmd/docs_guide.go @@ -438,9 +438,9 @@ CUSTOM can repeat on one app: a second resource type of any other kind fails with a 500, app resource type already exists which maps to exit 6 even though retrying never helps. Reuse the one that -exists by replacing --resource-type with --resource-type-id: the two are -mutually exclusive, so adding the id without dropping --resource-type is -refused at exit 2. +exists by passing --resource-type-id and dropping BOTH --resource-type and +--resource-type-display-name: each describes a type this command would create, +so either one alongside the id is refused at exit 2. --owner-id is repeatable and rides along in the create request, so entitlement owners need no follow-up call -- but the read lags the write the same way app diff --git a/cmd/entitlements_create.go b/cmd/entitlements_create.go index 462ef76..7d4af42 100644 --- a/cmd/entitlements_create.go +++ b/cmd/entitlements_create.go @@ -62,7 +62,9 @@ form. One of: Only CUSTOM can repeat on one app: a second resource type of any other kind fails with a 500 (exit 6, though retrying never helps): app resource type already exists -Reuse the one that exists with --resource-type-id instead. +Reuse the one that exists with --resource-type-id, dropping both +--resource-type and --resource-type-display-name; either alongside the id is a +usage error. Omitting --duration-grant leaves the entitlement at standing access (durationUnset in the response). It takes a protobuf duration, not a Go one -- @@ -150,6 +152,11 @@ type entitlementCreatePlan struct { // network / auth) so --dry-run and unit tests exercise the same bodies the live // requests send. Optional fields are omitted when empty rather than sent as // empty strings/arrays. +// typeCreateOnlyFlags describe a resource type this command would create, so +// each is refused alongside --resource-type-id. Named once: the docs that tell +// a reader what to drop are held to this list. +var typeCreateOnlyFlags = []string{"resource-type", "resource-type-display-name"} + func buildEntitlementCreatePlan(cmd *cobra.Command) (*entitlementCreatePlan, error) { appID, _ := cmd.Flags().GetString("app-id") displayName, _ := cmd.Flags().GetString("display-name") @@ -167,7 +174,7 @@ func buildEntitlementCreatePlan(cmd *cobra.Command) (*entitlementCreatePlan, err `invalid CreateAppEntitlementRequest.AppResourceTypeId: value does not match regex pattern "^[a-zA-Z0-9]{27}$"`)} } - if err := rejectFlagsForReusedObject(cmd, resourceTypeID, "--resource-type-id", "resource-type", "resource-type-display-name"); err != nil { + if err := rejectFlagsForReusedObject(cmd, resourceTypeID, "--resource-type-id", typeCreateOnlyFlags...); err != nil { return nil, err } if err := rejectFlagsForReusedObject(cmd, resourceID, "--resource-id", "resource-display-name"); err != nil { diff --git a/cmd/entitlements_create_test.go b/cmd/entitlements_create_test.go index a61916a..8a79f4b 100644 --- a/cmd/entitlements_create_test.go +++ b/cmd/entitlements_create_test.go @@ -688,3 +688,32 @@ func TestRemediationOnResourceFailureKeepsResourceName(t *testing.T) { "it names does not exist yet; the retry would create it under --display-name: %q", msg) } } + +// TestReusedTypeFlagsAreDocumented holds every doc that tells a reader to reuse +// an existing resource type to the full list the code refuses. The advice named +// only --resource-type while the code rejects --resource-type-display-name too, +// so following it verbatim exited 2 — and a keyword check could not see that. +func TestReusedTypeFlagsAreDocumented(t *testing.T) { + if len(typeCreateOnlyFlags) == 0 { + t.Fatal("no create-only type flags, so this guard would prove nothing") + } + docs := map[string]string{ + "entitlements create --help": entitlementsCreateCmd.Long, + "README.md": readDocFile(t, "../README.md"), + "cmd/agents.md (embedded)": agentsTemplate, + "docs guide configure-new-app": guideConfigureNewApp, + } + for name, doc := range docs { + flat := flattenDoc(doc) + if !strings.Contains(flat, "--resource-type-id") { + t.Errorf("%s never mentions --resource-type-id", name) + continue + } + for _, f := range typeCreateOnlyFlags { + if !strings.Contains(flat, "--"+f) { + t.Errorf("%s tells the reader to reuse a type but never says to drop --%s, "+ + "which the code refuses alongside --resource-type-id", name, f) + } + } + } +} From 4d16b28f6b52ddf4dfa005ed7be144648e9f56f0 Mon Sep 17 00:00:00 2001 From: leet-c1 <264029741+leet-c1@users.noreply.github.com> Date: Thu, 3 Sep 2026 01:29:56 +0000 Subject: [PATCH 6/8] Require one canonical reuse clause, and cover the resource pair too MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The previous guard asserted almost nothing. "--resource-type" is a prefix of "--resource-type-id", so that half was satisfied by the id every doc already names; the other half was satisfied by unrelated text elsewhere in three of the four docs, one of it an example that PASSES the flag. It would not have caught the defect it was written for. Three heuristic versions failed the same way — matching prose, then anchoring on the instruction, which let a doc that dropped the instruction be skipped rather than failed. Each doc now carries a fixed clause per reuse pair, required verbatim, and the clause is checked against the code's own list so the docs cannot be complete against a stale contract. The retry message was still built from a hardcoded copy of the flags, so adding one would have left the message advising a retry that exits 2 while the docs looked correct. Both pairs are single lists now, read by the reject call, the message and the docs alike. --resource-id refuses --resource-display-name and no doc said so; all four now do, verified live. Co-Authored-By: Claude Opus 5 --- README.md | 5 ++-- cmd/agents.md | 5 ++-- cmd/docs_guide.go | 6 ++--- cmd/entitlements_create.go | 26 ++++++++++-------- cmd/entitlements_create_test.go | 47 +++++++++++++++++++++++---------- 5 files changed, 57 insertions(+), 32 deletions(-) diff --git a/README.md b/README.md index 66d54bb..e8275a7 100644 --- a/README.md +++ b/README.md @@ -128,8 +128,9 @@ together with `--resource-type-id` is a usage error rather than a silently ignored flag. Only `CUSTOM` can repeat on one app: a second resource type of any other kind fails with a 500 (exit `6`, though retrying never helps) saying `app resource type already exists`, so reuse the existing one with -`--resource-type-id` and drop both `--resource-type` and -`--resource-type-display-name` — either one alongside the id is a usage error. `--owner-id` is repeatable and goes inline in the create +`--resource-type-id` and drop both --resource-type and --resource-type-display-name — either one alongside +the id is a usage error. Reusing a resource with `--resource-id` likewise means +you drop --resource-display-name. `--owner-id` is repeatable and goes inline in the create request, so no follow-up call is needed; an empty one is a usage error rather than an owner quietly dropped. `--duration-grant` takes a protobuf duration — seconds with an `s` suffix, e.g. `3600s`; a Go-style `1h` is refused by the diff --git a/cmd/agents.md b/cmd/agents.md index 26c2de7..c816f9e 100644 --- a/cmd/agents.md +++ b/cmd/agents.md @@ -250,8 +250,9 @@ flags the retry has to drop, and re-running without those flags creates duplicates. Only a `CUSTOM` resource type can repeat on one app: a second `--resource-type` of any other kind fails with a 500 (exit `6`, though retrying never helps) saying `app resource type already exists` -- reuse the -existing type with `--resource-type-id`, dropping both `--resource-type` and -`--resource-type-display-name`; either alongside the id is exit 2. +existing type with `--resource-type-id` and drop both --resource-type and --resource-type-display-name; +either alongside the id is exit 2. Reusing a resource with `--resource-id` +likewise means you drop --resource-display-name. ## Things that will surprise you diff --git a/cmd/docs_guide.go b/cmd/docs_guide.go index 691a8c9..14e8988 100644 --- a/cmd/docs_guide.go +++ b/cmd/docs_guide.go @@ -438,9 +438,9 @@ CUSTOM can repeat on one app: a second resource type of any other kind fails with a 500, app resource type already exists which maps to exit 6 even though retrying never helps. Reuse the one that -exists by passing --resource-type-id and dropping BOTH --resource-type and ---resource-type-display-name: each describes a type this command would create, -so either one alongside the id is refused at exit 2. +exists by passing --resource-type-id and drop both --resource-type and --resource-type-display-name: each describes a +type this command would create, so either alongside the id is refused at exit +2. Reusing a resource with --resource-id likewise means you drop --resource-display-name. --owner-id is repeatable and rides along in the create request, so entitlement owners need no follow-up call -- but the read lags the write the same way app diff --git a/cmd/entitlements_create.go b/cmd/entitlements_create.go index 7d4af42..9e99317 100644 --- a/cmd/entitlements_create.go +++ b/cmd/entitlements_create.go @@ -62,9 +62,9 @@ form. One of: Only CUSTOM can repeat on one app: a second resource type of any other kind fails with a 500 (exit 6, though retrying never helps): app resource type already exists -Reuse the one that exists with --resource-type-id, dropping both ---resource-type and --resource-type-display-name; either alongside the id is a -usage error. +Reuse the one that exists with --resource-type-id and drop both --resource-type and --resource-type-display-name; +either alongside the id is a usage error. Likewise, pass --resource-id to reuse +a resource and drop --resource-display-name. Omitting --duration-grant leaves the entitlement at standing access (durationUnset in the response). It takes a protobuf duration, not a Go one -- @@ -148,15 +148,19 @@ type entitlementCreatePlan struct { entitlementBody map[string]any } +// typeCreateOnlyFlags describe a resource type this command would create, so +// each is refused alongside --resource-type-id. resourceCreateOnlyFlags is the +// same for a resource and --resource-id. Named once each: the reject call, the +// retry message and the docs that tell a reader what to drop all read these. +var ( + typeCreateOnlyFlags = []string{"resource-type", "resource-type-display-name"} + resourceCreateOnlyFlags = []string{"resource-display-name"} +) + // buildEntitlementCreatePlan resolves flags into the request sequence. Pure (no // network / auth) so --dry-run and unit tests exercise the same bodies the live // requests send. Optional fields are omitted when empty rather than sent as // empty strings/arrays. -// typeCreateOnlyFlags describe a resource type this command would create, so -// each is refused alongside --resource-type-id. Named once: the docs that tell -// a reader what to drop are held to this list. -var typeCreateOnlyFlags = []string{"resource-type", "resource-type-display-name"} - func buildEntitlementCreatePlan(cmd *cobra.Command) (*entitlementCreatePlan, error) { appID, _ := cmd.Flags().GetString("app-id") displayName, _ := cmd.Flags().GetString("display-name") @@ -177,7 +181,7 @@ func buildEntitlementCreatePlan(cmd *cobra.Command) (*entitlementCreatePlan, err if err := rejectFlagsForReusedObject(cmd, resourceTypeID, "--resource-type-id", typeCreateOnlyFlags...); err != nil { return nil, err } - if err := rejectFlagsForReusedObject(cmd, resourceID, "--resource-id", "resource-display-name"); err != nil { + if err := rejectFlagsForReusedObject(cmd, resourceID, "--resource-id", resourceCreateOnlyFlags...); err != nil { return nil, err } @@ -209,13 +213,13 @@ func buildEntitlementCreatePlan(cmd *cobra.Command) (*entitlementCreatePlan, err "displayName": flagOrDefault(cmd, "resource-type-display-name", displayName), "resourceType": normalizeResourceType(resourceType), } - p.typeOnlyFlags = changedFlags(cmd, "resource-type", "resource-type-display-name") + p.typeOnlyFlags = changedFlags(cmd, typeCreateOnlyFlags...) } if resourceID == "" { p.resourceBody = map[string]any{ "displayName": flagOrDefault(cmd, "resource-display-name", displayName), } - p.resourceOnlyFlags = changedFlags(cmd, "resource-display-name") + p.resourceOnlyFlags = changedFlags(cmd, resourceCreateOnlyFlags...) } // Flag name -> request field, equal except where the wire key is camelCase. diff --git a/cmd/entitlements_create_test.go b/cmd/entitlements_create_test.go index 8a79f4b..d64bf1a 100644 --- a/cmd/entitlements_create_test.go +++ b/cmd/entitlements_create_test.go @@ -689,14 +689,38 @@ func TestRemediationOnResourceFailureKeepsResourceName(t *testing.T) { } } -// TestReusedTypeFlagsAreDocumented holds every doc that tells a reader to reuse -// an existing resource type to the full list the code refuses. The advice named -// only --resource-type while the code rejects --resource-type-display-name too, -// so following it verbatim exited 2 — and a keyword check could not see that. +// reuseDropClauses are the canonical instructions, required verbatim in every +// doc that advises reusing an object. Fixed clauses, not pattern-matched prose: +// three heuristic versions of this guard were each satisfiable by the wrong +// text — "--resource-type" is a prefix of "--resource-type-id", unrelated +// mentions elsewhere in a file counted, and anchoring on the instruction let a +// doc that dropped the instruction be skipped rather than failed. +var reuseDropClauses = map[string]string{ + "--resource-type-id": "drop both --resource-type and --resource-type-display-name", + "--resource-id": "drop --resource-display-name", +} + +// TestReusedTypeFlagsAreDocumented holds every doc that advises reusing an +// existing object to the full list of flags the code refuses alongside its id. +// Following advice that named only one of two refused flags exited 2. func TestReusedTypeFlagsAreDocumented(t *testing.T) { - if len(typeCreateOnlyFlags) == 0 { - t.Fatal("no create-only type flags, so this guard would prove nothing") + // The clauses must name every flag the code actually refuses, or the docs + // can be complete against a stale contract. + for idFlag, flags := range map[string][]string{ + "--resource-type-id": typeCreateOnlyFlags, + "--resource-id": resourceCreateOnlyFlags, + } { + clause, ok := reuseDropClauses[idFlag] + if !ok { + t.Fatalf("%s refuses flags but has no documented clause", idFlag) + } + for _, f := range flags { + if !strings.Contains(clause, "--"+f) { + t.Fatalf("the clause for %s omits --%s, which the code refuses", idFlag, f) + } + } } + docs := map[string]string{ "entitlements create --help": entitlementsCreateCmd.Long, "README.md": readDocFile(t, "../README.md"), @@ -705,14 +729,9 @@ func TestReusedTypeFlagsAreDocumented(t *testing.T) { } for name, doc := range docs { flat := flattenDoc(doc) - if !strings.Contains(flat, "--resource-type-id") { - t.Errorf("%s never mentions --resource-type-id", name) - continue - } - for _, f := range typeCreateOnlyFlags { - if !strings.Contains(flat, "--"+f) { - t.Errorf("%s tells the reader to reuse a type but never says to drop --%s, "+ - "which the code refuses alongside --resource-type-id", name, f) + for idFlag, clause := range reuseDropClauses { + if !strings.Contains(flat, clause) { + t.Errorf("%s does not carry the reuse clause for %s.\nwant: %s", name, idFlag, clause) } } } From 6b28b1164aa462ecb765dbcdfc363564c452bccb Mon Sep 17 00:00:00 2001 From: leet-c1 <264029741+leet-c1@users.noreply.github.com> Date: Thu, 3 Sep 2026 01:43:33 +0000 Subject: [PATCH 7/8] Use the repo's flag matcher, and stop banning backticks in the docs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The check that the clause names every refused flag used a substring match, so "--resource-type" was satisfied by "--resource-type-display-name" in the same clause — the exact prefix bug the clause design was meant to close, one layer in. A reviewer shortened the clause to name one flag, the whole suite stayed green, and the original defect came back: advice that exits 2. docMentionsFlag already exists for this, with a comment describing the trap. Using it, rather than a fourth hand-rolled matcher. Requiring the clause without backticks forced bare flag names into two markdown files that backtick everything else, and restoring correct markdown failed the build. Backticks are stripped before comparing instead. Also fails a doc that carries the clause but never names the id flag, and one that negates it. Renamed: it covers resources too. Co-Authored-By: Claude Opus 5 --- README.md | 7 +++-- cmd/agents.md | 7 +++-- cmd/docs_guide.go | 7 +++-- cmd/entitlements_create.go | 7 +++-- cmd/entitlements_create_test.go | 52 +++++++++++++++++++++------------ 5 files changed, 49 insertions(+), 31 deletions(-) diff --git a/README.md b/README.md index e8275a7..146a117 100644 --- a/README.md +++ b/README.md @@ -128,9 +128,10 @@ together with `--resource-type-id` is a usage error rather than a silently ignored flag. Only `CUSTOM` can repeat on one app: a second resource type of any other kind fails with a 500 (exit `6`, though retrying never helps) saying `app resource type already exists`, so reuse the existing one with -`--resource-type-id` and drop both --resource-type and --resource-type-display-name — either one alongside -the id is a usage error. Reusing a resource with `--resource-id` likewise means -you drop --resource-display-name. `--owner-id` is repeatable and goes inline in the create +`--resource-type-id` and drop both `--resource-type` and +`--resource-type-display-name` — either one alongside the id is a usage error. +Reusing a resource with `--resource-id` likewise means you drop +`--resource-display-name`. `--owner-id` is repeatable and goes inline in the create request, so no follow-up call is needed; an empty one is a usage error rather than an owner quietly dropped. `--duration-grant` takes a protobuf duration — seconds with an `s` suffix, e.g. `3600s`; a Go-style `1h` is refused by the diff --git a/cmd/agents.md b/cmd/agents.md index c816f9e..865dd07 100644 --- a/cmd/agents.md +++ b/cmd/agents.md @@ -250,9 +250,10 @@ flags the retry has to drop, and re-running without those flags creates duplicates. Only a `CUSTOM` resource type can repeat on one app: a second `--resource-type` of any other kind fails with a 500 (exit `6`, though retrying never helps) saying `app resource type already exists` -- reuse the -existing type with `--resource-type-id` and drop both --resource-type and --resource-type-display-name; -either alongside the id is exit 2. Reusing a resource with `--resource-id` -likewise means you drop --resource-display-name. +existing type with `--resource-type-id` and drop both `--resource-type` and +`--resource-type-display-name`; either alongside the id is exit 2. Reusing a +resource with `--resource-id` likewise means you drop +`--resource-display-name`. ## Things that will surprise you diff --git a/cmd/docs_guide.go b/cmd/docs_guide.go index 14e8988..4290dd5 100644 --- a/cmd/docs_guide.go +++ b/cmd/docs_guide.go @@ -438,9 +438,10 @@ CUSTOM can repeat on one app: a second resource type of any other kind fails with a 500, app resource type already exists which maps to exit 6 even though retrying never helps. Reuse the one that -exists by passing --resource-type-id and drop both --resource-type and --resource-type-display-name: each describes a -type this command would create, so either alongside the id is refused at exit -2. Reusing a resource with --resource-id likewise means you drop --resource-display-name. +exists by passing --resource-type-id and drop both --resource-type and +--resource-type-display-name: each describes a type this command would create, +so either alongside the id is refused at exit 2. Reusing a resource with +--resource-id likewise means you drop --resource-display-name. --owner-id is repeatable and rides along in the create request, so entitlement owners need no follow-up call -- but the read lags the write the same way app diff --git a/cmd/entitlements_create.go b/cmd/entitlements_create.go index 9e99317..684dbde 100644 --- a/cmd/entitlements_create.go +++ b/cmd/entitlements_create.go @@ -62,9 +62,10 @@ form. One of: Only CUSTOM can repeat on one app: a second resource type of any other kind fails with a 500 (exit 6, though retrying never helps): app resource type already exists -Reuse the one that exists with --resource-type-id and drop both --resource-type and --resource-type-display-name; -either alongside the id is a usage error. Likewise, pass --resource-id to reuse -a resource and drop --resource-display-name. +Reuse the one that exists with --resource-type-id and drop both +--resource-type and --resource-type-display-name; either alongside the id is a +usage error. Likewise, pass --resource-id to reuse a resource and drop +--resource-display-name. Omitting --duration-grant leaves the entitlement at standing access (durationUnset in the response). It takes a protobuf duration, not a Go one -- diff --git a/cmd/entitlements_create_test.go b/cmd/entitlements_create_test.go index d64bf1a..ba267e3 100644 --- a/cmd/entitlements_create_test.go +++ b/cmd/entitlements_create_test.go @@ -689,33 +689,34 @@ func TestRemediationOnResourceFailureKeepsResourceName(t *testing.T) { } } -// reuseDropClauses are the canonical instructions, required verbatim in every -// doc that advises reusing an object. Fixed clauses, not pattern-matched prose: -// three heuristic versions of this guard were each satisfiable by the wrong -// text — "--resource-type" is a prefix of "--resource-type-id", unrelated -// mentions elsewhere in a file counted, and anchoring on the instruction let a -// doc that dropped the instruction be skipped rather than failed. +// reuseDropClauses are the canonical instructions, required in every doc that +// advises reusing an object. Fixed clauses, not pattern-matched prose: earlier +// heuristic versions were each satisfiable by the wrong text. var reuseDropClauses = map[string]string{ "--resource-type-id": "drop both --resource-type and --resource-type-display-name", "--resource-id": "drop --resource-display-name", } -// TestReusedTypeFlagsAreDocumented holds every doc that advises reusing an -// existing object to the full list of flags the code refuses alongside its id. -// Following advice that named only one of two refused flags exited 2. -func TestReusedTypeFlagsAreDocumented(t *testing.T) { - // The clauses must name every flag the code actually refuses, or the docs - // can be complete against a stale contract. - for idFlag, flags := range map[string][]string{ - "--resource-type-id": typeCreateOnlyFlags, - "--resource-id": resourceCreateOnlyFlags, - } { +// reusePairs ties each id flag to the list the code actually refuses beside it. +var reusePairs = map[string][]string{ + "--resource-type-id": typeCreateOnlyFlags, + "--resource-id": resourceCreateOnlyFlags, +} + +// TestReuseAdviceIsDocumented holds every doc that advises reusing an existing +// object to the full list of flags the code refuses alongside its id. Advice +// naming only one of two refused flags exited 2, which is what this prevents. +func TestReuseAdviceIsDocumented(t *testing.T) { + for idFlag, flags := range reusePairs { clause, ok := reuseDropClauses[idFlag] if !ok { t.Fatalf("%s refuses flags but has no documented clause", idFlag) } for _, f := range flags { - if !strings.Contains(clause, "--"+f) { + // docMentionsFlag, not Contains: --resource-type is a prefix of + // --resource-type-display-name, so a substring check here is + // satisfied by the other flag in the very same clause. + if !docMentionsFlag(clause, f) { t.Fatalf("the clause for %s omits --%s, which the code refuses", idFlag, f) } } @@ -728,10 +729,23 @@ func TestReusedTypeFlagsAreDocumented(t *testing.T) { "docs guide configure-new-app": guideConfigureNewApp, } for name, doc := range docs { - flat := flattenDoc(doc) + // Backticks stripped so a doc can format flags as code, which is these + // files' own convention; requiring the bare form banned correct markdown. + flat := strings.ReplaceAll(flattenDoc(doc), "`", "") for idFlag, clause := range reuseDropClauses { - if !strings.Contains(flat, clause) { + if !docMentionsFlag(flat, strings.TrimPrefix(idFlag, "--")) { + t.Errorf("%s never names %s, so its reuse advice cannot be found", name, idFlag) + continue + } + at := strings.Index(flat, clause) + if at < 0 { t.Errorf("%s does not carry the reuse clause for %s.\nwant: %s", name, idFlag, clause) + continue + } + // A nearby negation inverts the instruction while satisfying it. + lead := strings.ToLower(flat[max(0, at-24):at]) + if strings.Contains(lead, "not ") || strings.Contains(lead, "never ") { + t.Errorf("%s negates the reuse clause for %s: %q", name, idFlag, flat[max(0, at-24):at+len(clause)]) } } } From 973fb6b9abae61d0ac019baafe14157981d554e9 Mon Sep 17 00:00:00 2001 From: leet-c1 <264029741+leet-c1@users.noreply.github.com> Date: Thu, 3 Sep 2026 01:46:42 +0000 Subject: [PATCH 8/8] Use the package's existing flatten, and list the shared doc set once flattenDoc was a second name for flatten, which this package already had. Deleted. The four docs that carry this command's claims were written out in full by two guards; extracted once. The other two guards keep their own sets deliberately -- the kinds check also reads the flag's usage string, and the remediation quote lives in only two docs. Co-Authored-By: Claude Opus 5 --- cmd/entitlements_create_test.go | 32 ++++++++++++++++---------------- 1 file changed, 16 insertions(+), 16 deletions(-) diff --git a/cmd/entitlements_create_test.go b/cmd/entitlements_create_test.go index ba267e3..a95af63 100644 --- a/cmd/entitlements_create_test.go +++ b/cmd/entitlements_create_test.go @@ -600,15 +600,24 @@ func withoutFlags(args, remove []string) []string { // TestResourceTypeSingletonIsDocumented holds the server's own error string in // every doc that promises the restriction, so it stays greppable from both // directions. Reproduced live: a second ROLE, GROUP or VAULT resource type on -// one app 500s, while a second CUSTOM one succeeds. -func TestResourceTypeSingletonIsDocumented(t *testing.T) { - const serverError = "app resource type already exists" - docs := map[string]string{ +// entitlementCreateDocs are the four sources that carry this command's +// behavioural claims. The other guards in this file deliberately use narrower +// sets -- the kinds list also checks the flag's own usage string, and the +// remediation quote lives in only two docs -- so those keep their own. +func entitlementCreateDocs(t *testing.T) map[string]string { + t.Helper() + return map[string]string{ "entitlements create --help": entitlementsCreateCmd.Long, "README.md": readDocFile(t, "../README.md"), "cmd/agents.md (embedded)": agentsTemplate, "docs guide configure-new-app": guideConfigureNewApp, } +} + +// one app 500s, while a second CUSTOM one succeeds. +func TestResourceTypeSingletonIsDocumented(t *testing.T) { + const serverError = "app resource type already exists" + docs := entitlementCreateDocs(t) for name, doc := range docs { if !strings.Contains(doc, serverError) { t.Errorf("%s does not quote the server's %q error", name, serverError) @@ -637,16 +646,12 @@ func TestRemediationStringIsDocumentedVerbatim(t *testing.T) { for name, doc := range docs { // Verbatim, not a keyword: renaming the flag must break the docs that // quote it, which a check for the word "dropping" alone would not. - if !strings.Contains(flattenDoc(doc), msg) { + if !strings.Contains(flatten(doc), msg) { t.Errorf("%s does not quote the partial-failure message verbatim.\nwant: %s", name, msg) } } } -// flattenDoc collapses whitespace so a quoted message still matches when a doc -// wraps it across lines. -func flattenDoc(s string) string { return strings.Join(strings.Fields(s), " ") } - // TestRemediationOnResourceFailureKeepsResourceName covers the step-2 failure: // the resource type exists, the resource does not. The retry must drop the // type-only flags but KEEP --resource-display-name, since the object it names @@ -722,16 +727,11 @@ func TestReuseAdviceIsDocumented(t *testing.T) { } } - docs := map[string]string{ - "entitlements create --help": entitlementsCreateCmd.Long, - "README.md": readDocFile(t, "../README.md"), - "cmd/agents.md (embedded)": agentsTemplate, - "docs guide configure-new-app": guideConfigureNewApp, - } + docs := entitlementCreateDocs(t) for name, doc := range docs { // Backticks stripped so a doc can format flags as code, which is these // files' own convention; requiring the bare form banned correct markdown. - flat := strings.ReplaceAll(flattenDoc(doc), "`", "") + flat := strings.ReplaceAll(flatten(doc), "`", "") for idFlag, clause := range reuseDropClauses { if !docMentionsFlag(flat, strings.TrimPrefix(idFlag, "--")) { t.Errorf("%s never names %s, so its reuse advice cannot be found", name, idFlag)