feat(iam): move user promote/demote onto the RBAC role API (#270)

[Geunwoo Noh (geunwoonoh)]

Closes #270. Context: alpacax/alpacon-server#3324

What this changes

is_staff and is_superuser on a user are read-only projections of the account's RBAC roles: the server drops them from a PATCH and answers 200 with the flags unchanged, never an error. The CLI had no way to change a privilege, and alpacon user update reported success for edits that were never applied — it round-tripped the whole user detail body through $EDITOR and PATCHed it back, flags included.

alpacon user role ls [USER]                        # bindings held, with scope
alpacon user role catalog                          # roles this workspace defines
alpacon user role describe ROLE                    # what it grants, and who holds it
alpacon user role grant  USER ROLE                 # --reason --dry-run -y
alpacon user role revoke USER ROLE                 # --cascade --reason --dry-run -y
alpacon user role history [USER]                   # who changed what, and why
alpacon user permission ls [USER]                  # effective access  [--patterns]
alpacon user permission can-i [USER] PERMISSION    # -q --explain

alpacon user update now builds a sparse patch of only the fields the editor actually changed. A privilege-flag edit is held back and reported with the exact alpacon user role command that performs it, and such an edit never exits 0 — whether or not the rest of it applied. The sparse patch also stops is_ldap_user being re-submitted on every edit, which made the server run a live LDAP lookup for a field nobody touched.

alpacon user create's Staff and Superuser prompts are unchanged. The create path is a different contract and writes the role rows correctly.

Why this command shape

Surveyed kubectl, Teleport (tsh/tctl), AWS/gcloud/Azure IAM, HashiCorp Boundary/Vault/Consul, Auth0/Okta/1Password/GitHub/Snowflake, plus clig.dev, the Heroku style guide, the kubectl sig-cli conventions and the gcloud/Azure CLI guidelines.

Counted across every tool, the surface hangs off the role noun 6 to 3. But those six are cloud IaaS CLIs whose principals are usually machines. Restricted to tools whose subject is a human in an identity provider or SaaS workspace, the convention inverts: Auth0 ships auth0 users roles assign and Teleport puts the account first. This API binds exactly one principal kind, so gcloud's typed --member=user: prefix and kubectl's repeatable --user/--group/--serviceaccount would be ceremony here.

Name	Precedent	Why
grant / revoke	SQL, 1Password	The oldest and most widely understood pair in access control (GRANT ROLE analyst TO USER jane). The TO/FROM flip keeps word order identical.
can-i	kubectl	Its exact verb, including -q. The subject is optional, so one argument asks about yourself.
catalog	Azure, Auth0, gcloud	Every surveyed tool separates the catalog from a subject's bindings. ls is already the binding list on the same group.
describe ROLE	gcloud, kubectl	Carries the holder list too, so one command answers both "what does this role grant" and "who holds it" — the second is the question kubectl's RBAC surface cannot ask at all.

No new top-level noun, no cmd/root.go change, and no new --role flag — --role occurs exactly once in cmd/ today (cmd/iam/member_add.go, the group-membership tier), so after this it still means one thing binary-wide. A test walks the whole UserCmd tree to keep it that way.

Decisions the survey settled:

• --cascade is opt-in. The one rule the survey is unanimous on: breadth is added only on the revoke side and must be typed (gcloud's --all on remove-iam-policy-binding, absent from add-). Never delete a row the operator did not name.
• --reason is optional, with a warning at the platform tier. A justification flag is idiomatic; a required one is not — gcloud PAM brackets [--justification] in its flagship JIT product, Teleport defers requiredness to server-side policy, and NIST SP 800-53 Rev.5 AC-6(9) demands logging, not operator-typed justification. user role history is what keeps it from being write-only decoration.
• No declarative set verb. Boundary can ship set-principals only because its server takes the whole desired set in one request; the atomicity is the server's. A client-side diff over a non-transactional API has none of it, and a set whose diff computes an unmentioned DELETE demotes a colleague as a side effect of an incomplete command line.
• Object-scoped writes deferred, the API layer scope-typed from day one. BindingCreateRequest already carries the scope pair, so adding a flag later is purely additive.
• No branch on deployment mode. kubectl bans version sniffing, and the CLI cannot locally distinguish the two deployments that behave differently here anyway. The difference surfaces only as a rewritten runtime error keyed off the actual response.
• Idempotent convergence. A grant of a role already held, and a revoke of one not held, both change nothing and exit 0. Neither verb claims success from a 2xx alone: both re-read the bindings and report the rows actually observed.

Behaviour worth knowing before merge

• Granting superuser creates a companion admin binding server-side, so revoking superuser demotes to admin and leaves that companion; --cascade removes both, superuser first. With no superuser binding to start from, --cascade revokes nothing rather than reaching for the companion.
• Revoking admin from someone who still holds superuser is refused before anything is sent — the account would keep every platform flag while no longer registering as an admin. Revoking a role a user does not hold is never refused.
• Changing a binding needs a workspace superuser and recent MFA. On Alpacon Cloud workspaces an API token is refused on the alpacon user role commands, so they need a browser login; user role history and the user permission commands are the carve-outs, and the README says which.
• alpacon user update exits 1 when the edit touched a privilege flag. That is the only behaviour change a wrapper script would notice, and it is in the exit-code table.

Verification

• go build ./..., go test -race ./..., go vet ./..., gofmt -l . — all clean; CI green.
• Every command exercised end to end against a stub implementing the verified contract, including both user update regressions, the --cascade guard, the superuser companion, the last-holder refusals, the self-form request paths and the -q exit codes.
• Contract tests pin the grant body with one JSONEq over the whole document, the revoke reason never reaching the query string, the absence of an ordering parameter, the multi-page walk, the IAM-hosted read paths, and the full refusal-message matrix per gate and credential with negative assertions.
• Exit-code contract unchanged: every own-validation failure exits 1.

Commits

18 commits, each building on its own (checked by replaying each into a scratch worktree), plus the review round. Read-only surface first, then the write verbs, then the user update fix, then permission introspection, then tests and docs.

🤖 Generated with Claude Code

[Copilot]

🟡 Changes recommended

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR migrates user promotion/demotion semantics in the Alpacon CLI away from PATCH /api/iam/users/{id}/ privilege flags and onto the RBAC role-binding API (/api/rbac/user-roles/), adding a new alpacon user role / alpacon user permission surface and hardening alpacon user update to avoid “false green” privilege edits.

Changes:

• Added an api/rbac client package plus new alpacon user role ... and alpacon user permission ... command trees under alpacon user.
• Reworked alpacon user update to generate a sparse patch, explicitly detect privilege-flag edits, and avoid resubmitting unchanged fields like is_ldap_user.
• Added shared workspace-scoped MFA callbacks, DELETE-with-body support, and updated docs/tests to lock in RBAC API behavior and CLI contracts.

File summaries

File	Description
README.md	Documents the new workspace role/permission commands and clarifies can-i -q exit-code semantics.
cmd/workspace/error_callbacks.go	Refactors workspace update MFA callbacks to use mfa.WorkspaceErrorCallbacks.
cmd/iam/user.go	Registers user role and user permission under alpacon user.
cmd/iam/user_update.go	Changes user update flow to sparse patching and blocks privilege-flag edits with actionable guidance.
cmd/iam/user_role.go	Adds the alpacon user role command group and shared subject resolution logic.
cmd/iam/user_role_test.go	Adds tests for cascade revoke planning, argument parsing, command registration, and flag meaning.
cmd/iam/user_role_revoke.go	Implements role revocation (including --cascade) with MFA retry and safety invariants.
cmd/iam/user_role_list.go	Implements role-binding listing for a user (or the caller).
cmd/iam/user_role_history.go	Implements audit-log history listing with --tail.
cmd/iam/user_role_grant.go	Implements role grant with duplicate convergence handling and MFA retry.
cmd/iam/user_role_describe.go	Implements role description combining permissions + holders (table/JSON).
cmd/iam/user_role_catalog.go	Implements role catalog listing with optional object-role hiding filter.
cmd/iam/user_permission.go	Adds the alpacon user permission command group.
cmd/iam/user_permission_list.go	Implements effective permission listing with an optional patterns-only mode.
cmd/iam/user_permission_cani.go	Implements can-i checks with -q/--explain behavior and argument disambiguation.
cmd/iam/rbac_errors.go	Adds RBAC-specific error rewriting, including credential-kind-aware 403 messaging.
cmd/iam/member_add.go	Clarifies that --role is group-tier role, not workspace RBAC role.
cmd/iam/json_slice.go	Adds helper to ensure empty arrays render as [] (not null) in hand-built JSON output.
client/client.go	Adds DELETE-with-body support and IsBearerAuth() to support RBAC refusal rewriting.
CLAUDE.md	Updates repository guidance to mention mfa.WorkspaceErrorCallbacks usage.
api/rbac/types.go	Defines RBAC DTOs and table-projection structs plus scope labeling helpers.
api/rbac/rbac.go	Implements RBAC API calls (roles, bindings, audits, permissions) and projections.
api/rbac/rbac_test.go	Adds contract tests for RBAC API request/response shapes and projections.
api/mfa/mfa.go	Introduces WorkspaceErrorCallbacks for workspace-scoped MFA prompts/retries.
api/iam/user_edit_test.go	Adds tests for sparse user-edit diffing and privilege-flag suppression.
api/iam/types.go	Adds UserEdit and PrivilegeEdit types to represent editor-session outcomes.
api/iam/iam.go	Replaces UpdateUser with PrepareUserUpdate + PatchUser and adds diff logic for sparse patches.

Review details

• Files reviewed: 27/27 changed files
• Comments generated: 4
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

🔵 Needs a closer look

Review details

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

cmd/iam/rbac_errors.go:69

• The codeless 403 rewrite always leads with “A role-binding write requires the superuser role”, but describeRBACError is also used by read-only commands (catalog/ls/describe/permission reads). In those cases the message is misleading, because the failure may simply be “API token refused on Alpacon Cloud” or “read access is limited”, not a missing write privilege. Reword the fallback so it accurately covers both read and write contexts while still pointing token users to alpacon login when relevant.

		if !ac.IsBearerAuth() {
			return errors.New("refused without a stated reason. A role-binding write requires the superuser role. This credential may also be the problem: the RBAC API refuses API tokens outright on Alpacon Cloud workspaces—run 'alpacon login' to authenticate through the browser—and the role history needs a token carrying the role_audit_log:read scope")
		}
		return errors.New("refused without a stated reason. A role-binding write requires the superuser role; a read is limited to the accounts and roles this workspace lets you see")

• Files reviewed: 27/27 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Geunwoo Noh (geunwoonoh)]

Addressing the suppressed comment on cmd/iam/rbac_errors.go:69 from the second review — it had no inline thread, so replying here.

Valid, and fixed in 88270ba. The codeless-403 branch led with "a role-binding write requires the superuser role" for every caller, and 10 of the 17 call sites are reads. An operator running alpacon user role ls and hitting a 403 read about a write privilege the request never needed.

The point the comment makes is sharper than "reword it", though: the three surfaces refuse for three different reasons, and no single sentence covers them without misleading someone.

Gate	What actually refuses	Wrong advice if conflated
read on /api/rbac/	on an Alpacon Cloud workspace the API refuses API tokens outright; the audit log additionally wants the role_audit_log:read scope on the token	"you need the superuser role" — a role grant does not make a cloud workspace accept a token
binding write	the above plus the superuser requirement on the write itself	—
read on /api/iam/users/{id}/…	user:read on the target account; tokens are accepted here	"run alpacon login" — the credential was never the problem

So callers now pass the gate they went through, and the message is built from that plus the credential the client actually carried (IsBearerAuth, read under tokenMu). The zero value is the read gate, so a caller that forgets says the milder thing rather than asserting a write privilege.

TestDescribeRBACError_CodelessForbidden pins all five combinations, including the negatives that matter — a role read never mentions a write privilege, and an iam-hosted read never suggests re-logging in.

Two notes on the first review, for the record: the --reason trim was valid and landed in a831518; the ls alias suggestion was not applied, and the reasoning is in those threads.

[Copilot]

🟡 Changes recommended

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

cmd/iam/user_role_revoke.go:60

• The guard that blocks revoke USER admin when the user still holds superuser triggers even if the user does not actually hold a workspace-wide admin binding. That breaks this command's own contract (“Revoking a role the user does not hold changes nothing and succeeds.”) for the (possible) half-state where superuser exists but admin binding has already been removed.

		if role.Name == rbac.RoleAdmin && rbac.HoldsWorkspaceRole(bindings, rbac.RoleSuperuser) {

• Files reviewed: 27/27 changed files
• Comments generated: 1
• Review effort level: Lite

[Geunwoo Noh (geunwoonoh)]

Addressing the suppressed comment on cmd/iam/user_role_revoke.go:60 from review 5074306585 — no inline thread, so replying here.

Valid, and fixed in 86c6d88. The guard decided from the role name and the superuser binding alone:

if role.Name == rbac.RoleAdmin && rbac.HoldsWorkspaceRole(bindings, rbac.RoleSuperuser) {

so a user holding superuser with no workspace-wide admin binding was refused a revoke of a row that does not exist. As you say, that contradicts the command's own Long text: "Revoking a role the user does not hold changes nothing and succeeds."

The half-state is reachable — the console or a raw DELETE can remove the admin row while superuser stands, which is exactly the state the guard exists to prevent this CLI from creating. Worth noting it is also the state left by an interrupted --cascade, so an operator retrying by hand would have hit the refusal.

Precedence is now explicit: the planned targets are computed first, an empty plan reports nothing to do and exits 0, and only a plan that would really delete a row reaches the guard.

targets := plannedRevocations(bindings, role.Name, cascade)
if len(targets) == 0 { /* nothing to do, exit 0 */ }
if wouldStrandThePlatformFlags(bindings, role.Name, targets) { /* refuse */ }

wouldStrandThePlatformFlags names the invariant it protects — the server accepts the delete, then re-forces is_staff because is_superuser still stands, so both flags survive while the account stops registering as an admin — and TestWouldStrandThePlatformFlags pins the four states.

Verified against a stub seeded with that exact half-state: alpacon user role revoke john admin now prints john does not hold admin workspace-wide. Nothing to do. and exits 0, and the companion-binding note correctly stays silent because there is no admin row to keep.

[Copilot]

🟡 Changes recommended

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

• Files reviewed: 27/27 changed files
• Comments generated: 2
• Review effort level: Lite

[Geunwoo Noh (geunwoonoh)]

Eunyoung Jeong (@eunyoung14) All of it addressed. Taking the blocker first.

The description

You are right, and I checked the premise before acting: this repository is public and the repositories I was citing are not. The description carried private issue numbers, a handbook document by path and finding number, decision-record numbers with an open governance question and my own read of the control it covers, and server source paths with line numbers. I wrote that, and it should not have gone out.

I am treating it as a disclosure that already happened, not one I have undone. GitHub keeps the body edit history and it stays visible to anyone who can see the PR, so the edit narrows further exposure and nothing more. If you want the exposure handled beyond that, say so and I will not touch it further in the meantime.

The internal context is now in alpacax/alpacon-server#3324, referenced from the description by number and nothing else. It carries everything the public body should not, including the correction to the earlier not-applicable verdict and a server-side question the client work surfaced.

The public body is now what the diff shows: the commands, the CLI-side rationale, the behaviour worth knowing before merge, and the verification. I re-scanned it for every category you listed and it comes back clean.

Correctness

1. permission_denied — fixed, 07d112d. You are right that the case was acknowledged and then not handled; my own gateRoleRead comment names troubleshoot as the coded exception. Worth adding that an earlier automated pass I ran had marked this NOT_REAL on the grounds that the operator sees no literal code — true, but it misses the point: the body is {"code": "permission_denied"} with no detail, so it lands on a generic framework sentence while every sibling refusal on this surface gives guidance. Now mapped.

2. user role history — fixed, d6d4373, and I took your second option. The filter is right: history john means changes to john's roles, and the same reading holds for the self form. The help was wrong — it described the server's visibility rung as though it were the command's output. It now says the command lists changes recorded against USER, so a grant you made to someone else appears in their history rather than yours.

3. Token arms — fixed, 07d112d. The deployment-independent reading leads and the credential refusal follows as the Alpacon Cloud alternative, so the accurate default is no longer unreachable for token sessions. Same correction on the audit gate: the reach limit leads and the scope is the additional condition, so a token that already holds role_audit_log:read is not told it lacks it. The tests now assert this as a negative — the role-read message must not name a write privilege, and the audit message must not claim the API is refusing tokens.

Smaller items — all four, plus the wrapping

• role describe directory walk — 0a1386a. Skipped when len(holders) == 0.
• GrantRole's unread decode — 0a1386a. Dropped; the function returns error. A test pins that an empty 201 is a success, which is the shape the bulk branch answers with.
• --reason delivery — 0a1386a. Commented at SendDeleteRequestWithBody: a DELETE entity-body is legal but unusual, an intermediary that strips one drops the justification silently, and nothing on that path can detect it.
• can-i -q --output json — d6d4373. The help now says -q produces no output at all, --output json included, and names dropping -q as the way to a machine-readable answer.
• errors.New discarding the cause — 07d112d. A small error type carries the actionable message while the server's error stays in the chain, so utils.HTTPStatusCode and errors.Is still reach it. Printing shows only our half: the refusal it replaces is a bare code or a generic framework sentence, and repeating either after our own would pad the line for no gain. A test pins both halves.

Tests

Closed the gap you named — 0a1386a. All three IAM-hosted reads now have httptest coverage asserting the request path and that the field actually decodes, so a server-side rename of allowed fails the suite instead of turning into a quiet "no". GET /roles/{id}/scopes/ is pinned the same way, matching TestResolveRole_ByUUID.

README

Both — d6d4373. The exit-code 1 row now records that alpacon user update exits 1 when the edit touched a read-only privilege flag, noting that any other field in the same edit still applies. And the user permission ls line's trailing comment is realigned; I added that line in a later round and mis-set it by one column.

CI is green on d6d4373. Thank you for reading the contract against the server rather than against my description — that is also how the two findings I got wrong were caught.

[Jisung Chae (jisung-02)]

Review at d6d4373

I checked the three commits that landed after the last review (07d112d, 0a1386a, d6d4373) against the diff, and every item raised there is addressed. This review covers only what is new on top of that.

What I ran locally, all clean: go build ./..., go vet ./..., golangci-lint run ./... (0 issues), go test ./api/... ./cmd/.... CI is green on all six checks. The new tests read no clock and no timezone, and no httptest handler carries an assertion, so -race -count=20 -shuffle=on and a TZ=Pacific/Kiritimati run both stayed stable.

Critical issues (0)

None.

Warnings (1)

1. cmd/iam/user_role.go:89-98 — naming yourself is not the self form.

resolveSubject takes the "-" alias only on the no-argument branch. Pass your own username or UUID and PK becomes the UUID, which is exactly the addressing gatePermissionIntrospect documents as refused: /api/iam/users/{id}/permissions/ pins no permission of its own, so a by-UUID read auto-resolves to an orphan user:permissions that only the superuser wildcard grants.

For a workspace admin without superuser:

alpacon user permission ls --patterns              # works
alpacon user permission ls jane --patterns         # 403, jane is the caller
alpacon user permission can-i server:update        # works
alpacon user permission can-i jane server:update   # 403, same account

The refusal message already says "run the command without a USER argument", so the case looks understood. It has the same shape as the permission_denied gap fixed in 07d112d, and it would be good to close it here too. resolveSubject already calls iam.GetCurrentUser on one branch, so comparing the resolved id against the caller's and swapping in selfUserPK would cover it.

TestResolveSubject has three cases and none is "the caller names themselves", so a fourth would keep it closed. The README also presents alpacon user permission ls <username> as the general form.

Suggestions (6)

1. cmd/iam/rbac_errors.go:77-78 — invalid_input is generic but rewritten into a write-only sentence.

Running the coded switch ahead of the gate is deliberate, and TestDescribeRBACError_CodedRefusalIgnoresTheGate pins it. That is right for rbac_superuser_last_removal_forbidden, a code only a write can produce. invalid_input is different: eight of the ten describeRBACError call sites are reads, and none of them binds a scope. can-i lets any argument containing : or * through looksLikePermission, so a malformed permission reaches the server, and a rejection there would print "the server rejected the binding scope" from a command that binds nothing.

Scoping the case to gateRoleWrite, or giving it the same kind of comment the duplicate case above it has, would settle it. workspace_suspended ("so it accepts no changes") reads the same way on user role ls.

2. cmd/iam/user_role_grant.go:58-61, cmd/iam/user_role_revoke.go:72-77 — --dry-run discloses less than the prompt it replaces.

The dry-run branch returns before both warnMissingReason and the confirmation. Previewing a superuser grant therefore never mentions the companion admin binding the server creates, which is the one fact the interactive path goes out of its way to state, and it never warns that the audit entry will carry no justification. Those two are exactly what someone reaches for --dry-run to see before a privileged write.

3. api/rbac/rbac.go:169, :194, :232 — nothing asserts the time columns.

BindingAttributesFrom, HolderAttributesFrom and AuditAttributesFrom all build their time string with AddedAt.Local().Format(timeLayout), and no test reads the resulting field. TestAuditAttributesFrom_ToleratesMissingActorAndRole checks ChangedBy, Role and Reason; TestHolderAttributesFrom_NilMapPrintsIDs checks User.

So a changed timeLayout, a dropped .Local(), or a row without added_at rendering as 0001-01-01 00:00 would all pass. The audit log exists to answer "who changed what, when", and the "when" is the unpinned part.

One fixed UTC instant asserted against the rendered string would cover it. Because of .Local() the expected value depends on the runner's timezone, so it needs either a pinned time.Local or a comparison on the time.Time itself. The current suite is stable precisely because it never touches timezones, and it would be good to keep that property while adding this.

4. api/iam/types.go:152-160 — the JSON tags on UserEdit and PrivilegeEdit are never used.

Neither struct is ever marshalled. cmd/iam/user_update.go:56 passes edit.Changes, a map, and PatchUser marshals that map. Every other type in this file is a wire type whose tags are live, so the next reader will take these four as evidence that the server accepts {"changes": ..., "privileges": ...}.

Leaving off the ~Response/~Request suffix reads as deliberate and I think it is right: these are not wire types. One small thing, PrivilegeEdit.Want is a thin name for a bool. Enable or Desired says what is wanted at the call site.

5. api/iam/user_edit_test.go — the filename points at a file that does not exist.

All three tests exercise diffEditedUser only, and diffEditedUser lives in api/iam/iam.go:358. There is no user_edit.go. Meanwhile api/iam/iam_test.go already covers other functions from the same source file (GetUserList, GetUserIDByName, CreateUser, InviteUser, AddMember), so one source file is split across two test files with nothing in the names marking the split. Merging into iam_test.go, or renaming to something like user_diff_test.go, would make it findable.

6. cmd/workspace/error_callbacks.go — the alias is now a shell worth removing.

Moving the body into api/mfa left this file as a single delegating line, while cmd/iam in the same PR calls mfa.WorkspaceErrorCallbacks directly. Two ways to reach one thing, and CLAUDE.md documents the split rather than removing it.

There are only two call sites (workspace_access_control_update.go:44, workspace_authentication_update.go:45), so the substitution is small. The part that needs a decision is cmd/workspace/error_callbacks_test.go, which calls errorCallbacks three times; that test now exercises api/mfa behavior, so moving it there would also put cmd/iam's path under the same coverage.

Related and much smaller: api/rbac/rbac.go:63. ResolveRole uses FetchAllPages to turn a name into a role, while the existing precedent for that operation, GetUserIDByName (api/iam/iam.go:217), does one SendGetRequest and takes Results[0]. The name filter is exact so behavior is identical, but two patterns now coexist for the same job. FetchPagesUpTo(..., 1) would put the intent in the code.

Out of scope for this branch (2)

Noting these only so they are on record; neither needs to happen here.

• api/rbac/types.go:186 — putting IsWorkspaceWide and ScopeLabel next to UserRoleResponse is closer to ordinary Go practice, but CLAUDE.md's "Go declaration order" fixes top-level declarations at const → var → type → func, and methods are func. The current placement follows the rule, so changing it is a repo-rule question rather than a file one.
• cmd/iam/json_slice.go — swapping a nil slice for an empty one keeps --output json emitting [] instead of null. List commands get this from utils.PrintTable, but the JSON branches of user role describe and user permission ls hand-build a map[string]any and skip that path, so the helper earns its place. If other cmd/ packages hand-build JSON documents they are likely solving the same thing separately, which makes this a candidate for utils later.

Highlights

• The tests pin contracts rather than exercise paths: one JSONEq over the whole grant document, the revoke reason asserted absent from the query string, assert.NotContains(query, "ordering") for a parameter's absence, and a codeless-403 matrix carrying wantNotSaid per gate and credential so a message cannot quietly start giving the wrong advice. The two follow-ups held that bar, with TestGrantRole_ToleratesAnEmptyCreatedBody and TestIAMHostedReadsHitTheRightPaths.
• rewritten with Unwrap is the right shape. The operator reads only the actionable half, and utils.HTTPStatusCode and errors.Is still reach the server's refusal underneath.
• Neither write verb claims success from a 2xx. Both re-read the bindings, which is what makes the superuser companion and a half-finished cascade visible instead of inferred.
• plannedRevocations refusing to treat an admin-only account as a resumable cascade is a subtle call, and the comment explains why the obvious shortcut would strip admin from someone who was never a superuser.
• cmd/iam/member_add.go:43 separating the group-membership --role from the new RBAC workspace role in the help text, with TestRoleFlagKeepsOneMeaning keeping --role to one meaning repo-wide.
• utils.PrintHeader writing to stderr, so table mode leaves stdout parseable.

Checklist

• Code follows stack conventions
• Tests included and passing, verified locally and in CI
• No security vulnerabilities
• Documentation updated (README, Long text, CLAUDE.md, exit-code table)

[Geunwoo Noh (geunwoonoh)]

Correcting something I told you in my last reply. I said alpacon user permission can-i server:update "failed for everyone below superuser" when the self form addressed the account by UUID, and that the object permission check is what refuses a self-read. Both are wrong.

The user:owner role auto-granted per account carries user:*, which matches user:read and the orphan user:permissions alike, so a self-read by UUID passes the object check. The server has a test asserting exactly that outcome for the effective-permissions endpoint, and its comment names the auto-granted role as the reason.

Sending - is still the right call, for weaker reasons than I gave: it is the server's own self route, one fewer check, and the form that still works for an account missing its user:owner row. But it was never the difference between working and refused. 540db6b's message and my reply both overstated it; ee0406e corrects the two code comments that carried the same claim and records the correction.

I found this by running a second review pass over the delta since the first one, which is also where the rest of what follows came from.

One real bug — ee0406e

utils.IsUUID accepts the un-dashed 32-hex form and either case, and resolveSubject stored whatever was typed. The audit log's ?user= filter is an exact string compare against the canonical dashed lowercase form, so alpacon user role history 22222222222222222222222222222222 matched nothing and printed an empty table with exit 0 — indistinguishable from an account whose roles nobody ever changed. On an audit read that is the worst answer available. Now canonicalized at the resolver, so every consumer gets the same form; user role ls was never affected because its filter coerces the value itself.

Corrections — aa1fc3f

• user role history printed global in a SCOPE column where every sibling in the group prints workspace. One column header, two vocabularies.
• The error-code block said all its codes come from the binding endpoints; permission_denied does not.
• gatePermissionIntrospect said the orphan scope is satisfied only by the superuser wildcard — true of a cross-account read, not of a self read.
• can-i's help said -q produces no output at all; a failed check still writes its line to stderr.
• The diffEditedUser case named "clearing a flag" cleared nothing, because the fixture had both flags false. It now clears one, and a new case covers what the old one was actually pinning.

Tests

The wrapping change promised nothing is discarded, and the test only checked the status. It now also asserts the code survives ParseErrorResponse through the wrapper — a rewrite that swallowed it would take the MFA and duplicate routing with it. The canonicalization is pinned over all three input forms.

What the pass rejected

21 candidates, 2 real. Worth noting two of the rejections, since both were about this same corner and both were wrong in the opposite direction from my earlier claim: one argued the fact that /effective-permissions/ refuses a member's own UUID is now documented nowhere, and the adjudication answered that it is not a fact. The rest were duplicates, or true-but-inert observations about test style.

CI green on aa1fc3f.

[Eunyoung Jeong (eunyoung14)]

The description is cleaned and the context moved where it belongs — thank you. Every code item I raised is fixed, and two of the fixes are better than what I asked for. But the disclosure surface is wider than the description was, and one part of it becomes unfixable the moment this merges.

Where the content still sits

The description is clean. The only private-repo reference left is a bare issue number with no description, and the issue that now holds the context exists. That is exactly the shape I asked for.

Three other surfaces still carry the same categories, and none of them were covered by editing the body:

1. PR comments. Several comments on this PR carry the same classes the description did — internal authorization class names, an internal mechanic described by behavior, and a meta-comment enumerating which sibling repositories are private. One comment reproduces roughly forty words of a private repository's source file verbatim. That is the sharpest item currently live: it is internal source content, quoted literally, in a public thread. Comments are as public and as permanent as the body, and editing them leaves the same audit trail the body edit does.

2. Shipped source comments. Eight added comment lines across cmd/iam/rbac_errors.go, cmd/iam/user_role.go and api/iam/iam.go name a private repository's source path, a decision-record number, a server view class and method, an internal field class together with its database column type, and a deployment-conditional gate by name. These ship in the public tree permanently, and unlike a comment they are read by anyone who opens the file.

3. Commit messages, and this is the time-critical one. Six of the twenty-four commits on this branch carry the same class of detail. After a merge commit they cannot be edited without a force-push. A squash merge with a rewritten subject and body collapses all twenty-four into one and is the cheap way out — but it has to be decided before the merge, not after.

I want to be clear about what is and is not achievable here. The pre-edit body remains retrievable through GitHub's edit history by anyone who can see the PR, so the cleanup narrows further exposure and does not undo what was already published. Your framing of it as a disclosure that already happened is the right one. What is still fully in our control is (1) not adding more, (2) the source comments, and (3) the commit messages.

For the source comments specifically: the useful half of each is the behavior the CLI has to accommodate. "The server narrows this silently rather than refusing" is worth writing down; naming the class that does it is what carries across the boundary. Rewriting them to describe the observable contract keeps everything a future maintainer of this repo actually needs.

Nothing else turned up — no internal hostnames, no customer names, no attack mechanics.

Code — all fixed, and two better than asked

permission_denied is mapped with a coded-403 test. role describe no longer pages the directory when a role has no holders. GrantRole returns an error and tolerates the empty 201 the bulk branch produces. describeRBACError now wraps, and the new rewritten type genuinely restores the chain — I checked that both utils.HTTPStatusCode and utils.ParseErrorResponse walk it with errors.Unwrap, so the status and the coded body survive a rewrite. The README exit-code row names the user update change.

Two I want to credit specifically:

user role history — narrowing the help rather than dropping the filter was the better call. The new wording ("the changes recorded against USER, so a grant you made to someone else appears in their history rather than yours") matches what the endpoint actually returns.

TestIAMHostedReadsHitTheRightPaths is exactly the guard the gap needed — a four-case table over all three IAM-hosted reads plus the concatenated scopes path, asserting the path and the query parameter, and failing if the permission field decodes to false. That closes the silent-wrong-denial case I was most worried about.

canonicalUUID is a real find on your own second pass: IsUUID accepts the un-dashed form while the audit filter is an exact string compare, so a typed UUID silently returned an empty table and exit 0.

One thing the fix traded

The gateAuditRead messages now lead with the auditor reach limit — but the server applies that limit as silent queryset narrowing and never returns a 403 for it. So both audit arms now lead with a cause that cannot produce the refusal they are describing, and by your own note the bearer arm is unreachable besides.

You documented both in the commit message and scoped this pass to comments only, which is a defensible line. I am flagging it because my item 6 asked you to fix an inaccurate message and the fix moved the inaccuracy one gate over rather than removing it. Worth a follow-up issue so it does not become the next reader's puzzle.

Before this can merge

There is a sixteen-thread review from jisung-02 with eight inline threads still unresolved and not outdated. Some are substantive — --dry-run returning before the reason warning and the companion-role disclosure, so the preview omits the two facts the interactive path exists to state; invalid_input rewriting to a binding-scope sentence on read call sites reachable from can-i with a malformed permission; and a test naming a file that does not exist. His first thread's mechanism is stale (a later commit corrected exactly that claim), but the observation under it still stands.

I am not going to merge over an open review from another reviewer. Please work those threads, and settle the commit-message question before the merge rather than after.

CI is green on 83e8c555. One correction to something I implied earlier: this repo runs a single ubuntu-latest leg for both Build and Test and golangci-lint, not a matrix.

[Eunyoung Jeong (eunyoung14)]

The code is done. I verified all four new fixes against the server's current main rather than against the commit messages, and every earlier item is still fixed.

be1bf47 is the one I want to credit. I asked you to fix a message that named a cause which cannot produce the refusal — and the fix removes the unreachable arm entirely rather than rewording it. I checked RoleAuditLogViewSet on the server: the auditor reach is applied wholly in get_queryset, so a non-privileged read returns rows and never a 403. Both old messages led with that, and both are gone. The test now asserts what is not said, which is the right shape for a message-accuracy guard.

95be0be puts both disclosures ahead of the --dry-run return on grant and revoke, with the confirmation prompt correctly still after. ee8c5ad is safe because the server declares name as an exact filter over a unique column, so one row is the whole answer. ecd45fb covers all three scope tiers the server emits and renders a zero timestamp blank; pinning the zone in the test rather than reading it was the right instinct.

7f255fd explains itself: the three files were deleted earlier on this branch and a stale index re-added them. Nothing from main was resurrected. Worth knowing that 39aa536 therefore does not compile — irrelevant under squash, relevant if anyone bisects.

All eight of Jisung Chae (@jisung-02)'s threads are answered in code, including the one where the rejection is correct — I confirmed against a server test that an own-UUID read resolves to self and is not refused. They still need a click to resolve; I am not going to merge over open threads from another reviewer.

Correcting my own advice, before the merge

I told you a squash merge with a rewritten body would collapse the commit messages. That is only true if the body is actually replaced, and this repository is configured so it is not. squash_merge_commit_message is COMMIT_MESSAGES, so the squash body is pre-filled with all 32 messages concatenated. Merging on that default would put them onto main permanently — the opposite of what I described. My mistake, and it would have caused the exact thing I asked you to avoid.

Ten of the 32 still carry the categories I flagged. So the merge itself is the decision point:

• merge with an explicit body — gh pr merge 409 --squash --body "…", or
• change the repository's squash-message setting to PR_BODY (the body is clean) or BLANK first.

Either works. The default does not.

Note also that a correct squash keeps them out of main's history and nothing more — refs/pull/409/head stays fetchable regardless.

What is already published

The comment bodies are cleaned and the shipped source comments are fixed — six of six, and the replacements say the observable contract rather than naming what implements it, which is what a maintainer of this repo actually needs.

The earlier revisions of the edited text remain retrievable. That is not something an edit undoes, and for one of the surfaces it is not something you can remove at all. Your framing of it as a disclosure that already happened is right. I would rather work out how to handle it off this thread than describe it further here — a public post-mortem of where the material sits is one more pointer to it. Let's take that part to the server-side issue.

One thing to do here regardless: the italic notes you appended to the edited comments describe what the earlier version contained. Please strip those.

Not asking for these

Three comments in the diff use the server's framework and ORM vocabulary and name the identity provider. That vocabulary is already the baseline on this repo's main, in six places — one of which names a private repository's source file path outright. This PR is stricter than the branch it merges into. Holding it to a bar main does not meet would be the wrong call, so I am leaving them and will open a separate cleanup for the pre-existing one.

Approving on the code. Merge it once Jisung Chae (@jisung-02)'s threads are resolved, and with the squash body handled.

[Jisung Chae (jisung-02)]

Approving. Every point from my last round is answered, and the two I did not expect to be answered the way they were are the better outcomes: the timestamp comment turned up the zero-time rendering underneath it, and the permission_denied self-read was checked against the server rather than against my claim. The correction on that one is right and I withdraw it.

What I checked on this push: go test ./... is green across every package and go vet ./... is silent.

Two nits below, neither blocking.

[Eunyoung Jeong (eunyoung14)]

Re-approving on cdf6fbe66. My earlier approval was dismissed mechanically by dismiss_stale_reviews_on_push, not by anything substantive.

cdf6fbe is comment prose and nothing else. I checked it two ways rather than taking the subject line for it: every changed line's first non-whitespace token is //, and all sixteen files are byte-identical after comment stripping. No struct tag, string literal, identifier, signature or control flow moved — I looked specifically at the json:"..." tags in api/rbac/types.go and api/iam/types.go and at the user-facing strings in cmd/iam/rbac_errors.go, since those are where a "comment-only" pass most often isn't.

It also removes six more internal-detail clauses from shipped comments, on top of the six 39aa536 had already cleaned. That was your own initiative, not something I asked for.

Thank you for working Jisung Chae (@jisung-02)'s threads to resolution rather than leaving them answered-but-open — and he has now approved this exact head.

Two things stand between this and the merge button

Two new threads arrived with his approval — the test fixture carrying a character the next line strips, and the tier label spelled two ways between two commands' output. He flagged both as nits he would not hold the PR for, but this repo's ruleset sets required_review_thread_resolution, so an unresolved thread is what is holding mergeStateStatus at BLOCKED regardless of severity. Answer or resolve them and the button clears.

The squash body still needs a hand edit. squash_merge_commit_message is COMMIT_MESSAGES, so the body is pre-filled with all 33 messages concatenated. Eight of them carry the categories I flagged — and that count is unchanged rather than improved, which is expected and not a criticism: the previously-audited commits are SHA-identical, so no message could have changed. The only fixes are replacing the body at merge time or rewriting history, and replacing the body is the cheap one.

Separately, and fixable right now: the PR body's first line carries a private-repo cross-reference. That is on the public page today, independent of the squash. Worth editing before this merges.

Context that is not this PR's problem

While checking the shipped comments I looked at what main already carries, so the bar here is clear. main ships this same category widely — roughly two dozen lines naming the private server repo, thirteen backend source paths, and a couple of dozen decision-record references, including in thirteen of the files this branch touches. One instance is in the public README on main, in the exit-code table, and it arrived with a different PR.

This branch neither adds to that nor removes it, and it is now stricter than the branch it merges into. That cleanup is real but it is a separate, larger piece of work and should not be hung on this PR. I will open it separately.

Nothing in the 2974 added lines across 47 files reintroduces any of the six categories — I read all 174 added Go comment lines.

CI is green on cdf6fbe66, and the required build-and-test context is present. Being nine commits behind main is harmless here: the ruleset's strict policy is off, and because this branch does not touch .github/ at all, the merge ref picks up main's fixed workflow — which is also why the flat context reports correctly despite this branch's own copy predating that change.

[Eunyoung Jeong (eunyoung14)]

Filed the cleanup I mentioned: #414. It confirms the scope is main-wide and predates this branch, and it says plainly that nothing in it should block this PR — you are stricter about this than the branch you are merging into. No action needed here.

[Jisung Chae (jisung-02)]

Approving. Both nits from the last round are fixed in dc668e6, and the tier one came back better than I asked for.

tierLabel returns type, which is the string ScopeLabel heads its own output with, so user role ls and user role history name the tier the same way in the same terminal. TestTierLabel now carries the reason in one line, so the next change to either function has to keep the pair in step instead of rediscovering why they match. The UUID fixture is the 32 characters directly, and the ReplaceAll line and the strings import went with it.

Verified locally on dc668e6: golangci-lint run ./... reports 0 issues., and go test -race ./... passes with no FAIL, api/rbac and cmd/iam included. All six CI checks are green.

Nothing blocking left. One label that is not worth a commit on its own: the test case at api/rbac/rbac_test.go:88 is still named "content-type wide", the last place the old spelling survives. It never reaches an operator, so fold it into whatever touches that file next.