From a8f4d74aa2f9082a3f23d8276bdadbd9b02c091a Mon Sep 17 00:00:00 2001 From: Holger Selover-Stephan Date: Sun, 25 Oct 2026 13:52:56 +0000 Subject: [PATCH 1/4] Add `app member add|remove|list` (#956) Wraps ensureAppMember / removeAppMember / App.members. App is --app or the active App (explicit blank refuses); --user passes through unchanged; every write is confirmed; a write that fails other than by a typed refusal is reported as outcome unknown and never retried; remove reports "absent". Maps InvalidRoleError/MaxMembersExceededError/OrphanAppError to exit codes. Worker: Jack (cli-engineer) Co-Authored-By: Claude Sonnet 5.5 --- docs/plans/app-member-956.md | 45 ++ internal/api/errors.go | 40 ++ internal/api/errors_test.go | 3 + internal/api/gen/generated.go | 305 +++++++++++++ internal/api/queries/app_members.graphql | 44 ++ internal/api/unbound-ops.txt | 4 +- internal/cmd/agentic/agentic-usage.md | 30 ++ internal/cmd/app/app.go | 1 + internal/cmd/app/member.go | 338 ++++++++++++++ internal/cmd/app_member_cmd_test.go | 540 +++++++++++++++++++++++ 10 files changed, 1347 insertions(+), 3 deletions(-) create mode 100644 docs/plans/app-member-956.md create mode 100644 internal/api/queries/app_members.graphql create mode 100644 internal/cmd/app/member.go create mode 100644 internal/cmd/app_member_cmd_test.go diff --git a/docs/plans/app-member-956.md b/docs/plans/app-member-956.md new file mode 100644 index 00000000..50a6edb7 --- /dev/null +++ b/docs/plans/app-member-956.md @@ -0,0 +1,45 @@ +# `hadron app member add|remove|list` (cli#956) — design as built + +Built from the CHECKED gate on cli#956 (T2 + scoped §10) and the Opus plan on the same issue +(written against hadron-cli main `9e5d14e3`; main had not changed any file it cites at pickup). +Spec: `cor:dmo:050:04` (AppMember), `cor:dmo:050:12` (who manages an App). + +## Surface + +| Command | Wraps | Notes | +| --- | --- | --- | +| `app member add --user --role ` | `ensureAppMember` | idempotent upsert; role required, no default | +| `app member remove --user ` | `removeAppMember` | server answers `true` whether or not a row existed | +| `app member list` | `App.members` | visible rows only | + +The App is the root `--app` flag, else the active App. No positional App. + +## Decisions + +- **`--app ""` refuses.** `Factory.App()` treats a blank flag like an absent one; `memberTargetApp` + checks `Flag("app").Changed` first. The resolved ref (and which branch supplied it) is frozen once and + used for the prompt, the mutation and the receipt. +- **`--user` is passed through verbatim** (id / email / handle / `hrn:user:`): no trim, no case fold, + no pre-resolution, no `list` call to "detect" a role change. Consequently every add and remove is + confirmed; `--yes` skips, non-TTY without it exits 2 with zero requests. +- **Remove uses `Confirm`, not `ConfirmDeletion`** (review:confirm-prompt-tells-the-truth): the change is + reversible, so "cannot be undone" would be false. *Deviation from the plan, which named ConfirmDeletion.* +- **Definite vs unknown outcome** (`api.AppMemberWriteRefused`). The server re-reads the row after + commit, so a masked INTERNAL error can follow a committed upsert. Only `InvalidRoleError`, + `MaxMembersExceededError`, `OrphanAppError`, NOT_FOUND, UNAUTHENTICATED, BAD_USER_INPUT, FORBIDDEN or a + `Forbidden:` message are definite. Anything else (transport, `data`+`errors`, empty payload) exits 7 + "outcome unknown", prints no DTO, is never retried, and points at `app member list`. +- **Exit codes** for the typed refusals were added to `codeForExtension`: `InvalidRoleError` 2, + `MaxMembersExceededError`/`OrphanAppError` 5. +- **Remove reports `status: "absent"`**, never "removed". A non-manager's refusal for any foreign target + (including an unknown one) is printed as the server wrote it — no "user not found" translation. +- **List** selects `role createdAt user { id handle urn }` only (name/email are withheld by the server). + Empty means "no members visible to you"; `app: null` is NotFound; a null row is an error. + +## Residual (not in this cut) + +R113: `callerManagesAppMembers` has no worker-session refusal and `internal/cmd/app` sends no session +header, so a bound worktree's `app member add` acts with its holder's authority (as `app key` does today). +Server convergence onto `managesApp` and a CLI follow-up attaching the session header are separate issues. +The server also has no last-owner guard on role changes/removal; the confirm-every-write rule is the +CLI's only mitigation. diff --git a/internal/api/errors.go b/internal/api/errors.go index 0e1e8955..ac83adde 100644 --- a/internal/api/errors.go +++ b/internal/api/errors.go @@ -768,6 +768,13 @@ func codeForExtension(code string) int { return exitcode.Usage case code == "TICKER_NAME_TAKEN" || code == "TICKER_LIMIT_REACHED": return exitcode.Conflict + // App member writes (cli#956, cor:dmo:050:04): a role the primary Agent's + // policy does not list is an argument the caller fixes; the member cap and + // the last-owner guard hold until the App's membership changes. + case code == "InvalidRoleError": + return exitcode.Usage + case code == "MaxMembersExceededError" || code == "OrphanAppError": + return exitcode.Conflict case code == "UNAUTHENTICATED": return exitcode.AuthRequired case code == "NOT_FOUND" || strings.HasSuffix(code, "_NOT_FOUND") || code == "AiServiceConfigNotFoundError": @@ -1222,3 +1229,36 @@ func IsUnknownGraphQLArgument(err error, argument, field string) bool { } return true } + +// AppMemberWriteRefused reports whether err is a DEFINITE refusal of an +// ensureAppMember / removeAppMember call: the server threw before the write, +// inside the authorized transaction, so nothing changed. Anything else — +// transport failures, a data+errors partial, a masked INTERNAL_SERVER_ERROR — +// is outcome unknown, because ensureAppMember re-reads the row AFTER commit and +// a failure there is reported although the upsert committed (#956). +// +// The typed refusals are InvalidRoleError, MaxMembersExceededError and +// OrphanAppError; the server's `Forbidden: …` arrives as plain text under +// INTERNAL_SERVER_ERROR, so it is recognised by its message prefix. EVERY error +// in the list must qualify; one unrecognized entry keeps the answer ambiguous. +// Like EndRefusedBeforeCommit, call it BEFORE MapError wraps. +func AppMemberWriteRefused(err error) bool { + list := graphQLErrors(err) + if len(list) == 0 { + return false + } + for _, e := range list { + if e == nil { + return false + } + switch extensionCode(e) { + case "InvalidRoleError", "MaxMembersExceededError", "OrphanAppError", + "NOT_FOUND", "UNAUTHENTICATED", "BAD_USER_INPUT", "FORBIDDEN": + continue + } + if !strings.HasPrefix(strings.TrimSpace(e.Message), "Forbidden:") { + return false + } + } + return true +} diff --git a/internal/api/errors_test.go b/internal/api/errors_test.go index 9427c728..9688a2f0 100644 --- a/internal/api/errors_test.go +++ b/internal/api/errors_test.go @@ -66,6 +66,9 @@ func TestMapError(t *testing.T) { {"ticker quiet hours", gqlErr("TICKER_QUIET_HOURS_INVALID"), exitcode.Usage}, {"ticker name", gqlErr("TICKER_NAME_TAKEN"), exitcode.Conflict}, {"ticker cap", gqlErr("TICKER_LIMIT_REACHED"), exitcode.Conflict}, + {"app member invalid role", gqlErr("InvalidRoleError"), exitcode.Usage}, + {"app member cap", gqlErr("MaxMembersExceededError"), exitcode.Conflict}, + {"app member orphan", gqlErr("OrphanAppError"), exitcode.Conflict}, {"reports to self", gqlErr("WORKER_REPORTS_TO_SELF"), exitcode.Usage}, // Host poller and launch refusals (cli#831). {"host key required", gqlErr("HOST_KEY_REQUIRED"), exitcode.Forbidden}, diff --git a/internal/api/gen/generated.go b/internal/api/gen/generated.go index d887d756..e80edb04 100644 --- a/internal/api/gen/generated.go +++ b/internal/api/gen/generated.go @@ -7094,6 +7094,76 @@ type AppKeysMetadataResponse struct { // GetAppKeys returns AppKeysMetadataResponse.AppKeys, and is useful for accessing the field via an interface. func (v *AppKeysMetadataResponse) GetAppKeys() []*AppKeysMetadataAppKeysAppKey { return v.AppKeys } +// AppMembersApp includes the requested fields of the GraphQL type App. +// The GraphQL type's documentation follows. +// +// A runtime caller identity owned by exactly one Organization or User. It owns +// long-lived App Keys and installs Agents through the AppAgent N:M join: one +// App may install many Agents, and one Agent may be installed in many Apps. +// The singular agentId / agent fields below are soft-deprecated convenience +// reads of the first install, not a direct foreign key. +type AppMembersApp struct { + Id string `json:"id"` + Urn string `json:"urn"` + Members []*AppMembersAppMembersAppMember `json:"members"` +} + +// GetId returns AppMembersApp.Id, and is useful for accessing the field via an interface. +func (v *AppMembersApp) GetId() string { return v.Id } + +// GetUrn returns AppMembersApp.Urn, and is useful for accessing the field via an interface. +func (v *AppMembersApp) GetUrn() string { return v.Urn } + +// GetMembers returns AppMembersApp.Members, and is useful for accessing the field via an interface. +func (v *AppMembersApp) GetMembers() []*AppMembersAppMembersAppMember { return v.Members } + +// AppMembersAppMembersAppMember includes the requested fields of the GraphQL type AppMember. +type AppMembersAppMembersAppMember struct { + Role string `json:"role"` + CreatedAt string `json:"createdAt"` + User *AppMembersAppMembersAppMemberUser `json:"user"` +} + +// GetRole returns AppMembersAppMembersAppMember.Role, and is useful for accessing the field via an interface. +func (v *AppMembersAppMembersAppMember) GetRole() string { return v.Role } + +// GetCreatedAt returns AppMembersAppMembersAppMember.CreatedAt, and is useful for accessing the field via an interface. +func (v *AppMembersAppMembersAppMember) GetCreatedAt() string { return v.CreatedAt } + +// GetUser returns AppMembersAppMembersAppMember.User, and is useful for accessing the field via an interface. +func (v *AppMembersAppMembersAppMember) GetUser() *AppMembersAppMembersAppMemberUser { return v.User } + +// AppMembersAppMembersAppMemberUser includes the requested fields of the GraphQL type User. +type AppMembersAppMembersAppMemberUser struct { + Id string `json:"id"` + Handle *string `json:"handle"` + // Canonical `hrn:user:` URN (spec cor:urn:010:01). Computed from the + // handle; null for a handle-less user (who has no URN). + Urn *string `json:"urn"` +} + +// GetId returns AppMembersAppMembersAppMemberUser.Id, and is useful for accessing the field via an interface. +func (v *AppMembersAppMembersAppMemberUser) GetId() string { return v.Id } + +// GetHandle returns AppMembersAppMembersAppMemberUser.Handle, and is useful for accessing the field via an interface. +func (v *AppMembersAppMembersAppMemberUser) GetHandle() *string { return v.Handle } + +// GetUrn returns AppMembersAppMembersAppMemberUser.Urn, and is useful for accessing the field via an interface. +func (v *AppMembersAppMembersAppMemberUser) GetUrn() *string { return v.Urn } + +// AppMembersResponse is returned by AppMembers on success. +type AppMembersResponse struct { + // Fetch an App (member of the App's org, or platform ADMIN — the myApps + // exposure bar; #473 relaxed this from org ADMIN for read parity with the + // list). + // + // 'ref' accepts the entity's ID or URN. + App *AppMembersApp `json:"app"` +} + +// GetApp returns AppMembersResponse.App, and is useful for accessing the field via an interface. +func (v *AppMembersResponse) GetApp() *AppMembersApp { return v.App } + // AppRunAppRun includes the requested fields of the GraphQL type AppRun. // The GraphQL type's documentation follows. // @@ -22056,6 +22126,62 @@ func (v *EndTeamSessionWithResumeClaimResponse) GetEndSession() *EndTeamSessionW return v.EndSession } +// EnsureAppMemberEnsureAppMember includes the requested fields of the GraphQL type AppMember. +type EnsureAppMemberEnsureAppMember struct { + Role string `json:"role"` + CreatedAt string `json:"createdAt"` + User *EnsureAppMemberEnsureAppMemberUser `json:"user"` +} + +// GetRole returns EnsureAppMemberEnsureAppMember.Role, and is useful for accessing the field via an interface. +func (v *EnsureAppMemberEnsureAppMember) GetRole() string { return v.Role } + +// GetCreatedAt returns EnsureAppMemberEnsureAppMember.CreatedAt, and is useful for accessing the field via an interface. +func (v *EnsureAppMemberEnsureAppMember) GetCreatedAt() string { return v.CreatedAt } + +// GetUser returns EnsureAppMemberEnsureAppMember.User, and is useful for accessing the field via an interface. +func (v *EnsureAppMemberEnsureAppMember) GetUser() *EnsureAppMemberEnsureAppMemberUser { return v.User } + +// EnsureAppMemberEnsureAppMemberUser includes the requested fields of the GraphQL type User. +type EnsureAppMemberEnsureAppMemberUser struct { + Id string `json:"id"` + Handle *string `json:"handle"` + // Canonical `hrn:user:` URN (spec cor:urn:010:01). Computed from the + // handle; null for a handle-less user (who has no URN). + Urn *string `json:"urn"` +} + +// GetId returns EnsureAppMemberEnsureAppMemberUser.Id, and is useful for accessing the field via an interface. +func (v *EnsureAppMemberEnsureAppMemberUser) GetId() string { return v.Id } + +// GetHandle returns EnsureAppMemberEnsureAppMemberUser.Handle, and is useful for accessing the field via an interface. +func (v *EnsureAppMemberEnsureAppMemberUser) GetHandle() *string { return v.Handle } + +// GetUrn returns EnsureAppMemberEnsureAppMemberUser.Urn, and is useful for accessing the field via an interface. +func (v *EnsureAppMemberEnsureAppMemberUser) GetUrn() *string { return v.Urn } + +// EnsureAppMemberResponse is returned by EnsureAppMember on success. +type EnsureAppMemberResponse struct { + // Idempotent UPSERT of an AppMember row. Per spec 008-agent-installation + // FR-004 / FR-016. The role MUST be a value present in the parent + // Agent's installationPolicy.memberRoles. Creating a new member is + // rejected if the App's current member count meets or exceeds the + // Agent's installationPolicy.maxMembers. Updating an existing member's + // role does NOT trigger the maxMembers check. The primary installed Agent + // (the oldest AppAgent) supplies this policy; no installed Agent refuses + // OrphanAppError. Only an App member manager may call this: the user-owner, + // an owner-role AppMember, or owning-org ADMIN (including platform admins + // for org-owned Apps). The manager gate runs before target-user lookup. + // + // Accepts the entity's ID or URN. + EnsureAppMember *EnsureAppMemberEnsureAppMember `json:"ensureAppMember"` +} + +// GetEnsureAppMember returns EnsureAppMemberResponse.EnsureAppMember, and is useful for accessing the field via an interface. +func (v *EnsureAppMemberResponse) GetEnsureAppMember() *EnsureAppMemberEnsureAppMember { + return v.EnsureAppMember +} + // ExtractParentNodeToMemoryExtractParentNodeToMemory includes the requested fields of the GraphQL type Memory. // The GraphQL type's documentation follows. // @@ -37609,6 +37735,20 @@ func (v *ReleaseWorkerResponse) GetReleaseWorker() *ReleaseWorkerReleaseWorkerRe return v.ReleaseWorker } +// RemoveAppMemberResponse is returned by RemoveAppMember on success. +type RemoveAppMemberResponse struct { + // Delete an AppMember row. Idempotent (no-op when the row doesn't exist). + // Personal-class memory at (appId, userId) is retained as an orphan per + // FR-015 — re-attaches automatically if the user later rejoins the same + // App. + // + // Accepts the entity's ID or URN. + RemoveAppMember bool `json:"removeAppMember"` +} + +// GetRemoveAppMember returns RemoveAppMemberResponse.RemoveAppMember, and is useful for accessing the field via an interface. +func (v *RemoveAppMemberResponse) GetRemoveAppMember() bool { return v.RemoveAppMember } + // RemoveMemoryMemberRemoveMemoryMemberRemoveMemoryMemberPayload includes the requested fields of the GraphQL type RemoveMemoryMemberPayload. type RemoveMemoryMemberRemoveMemoryMemberRemoveMemoryMemberPayload struct { MemoryId string `json:"memoryId"` @@ -61271,6 +61411,14 @@ type __AppKeysMetadataInput struct { // GetAppRef returns __AppKeysMetadataInput.AppRef, and is useful for accessing the field via an interface. func (v *__AppKeysMetadataInput) GetAppRef() string { return v.AppRef } +// __AppMembersInput is used internally by genqlient +type __AppMembersInput struct { + AppRef string `json:"appRef"` +} + +// GetAppRef returns __AppMembersInput.AppRef, and is useful for accessing the field via an interface. +func (v *__AppMembersInput) GetAppRef() string { return v.AppRef } + // __AppRunInput is used internally by genqlient type __AppRunInput struct { Ref string `json:"ref"` @@ -63021,6 +63169,22 @@ func (v *__EndTeamSessionWithResumeClaimInput) GetSummary() *string { return v.S // GetHandoff returns __EndTeamSessionWithResumeClaimInput.Handoff, and is useful for accessing the field via an interface. func (v *__EndTeamSessionWithResumeClaimInput) GetHandoff() *string { return v.Handoff } +// __EnsureAppMemberInput is used internally by genqlient +type __EnsureAppMemberInput struct { + AppRef string `json:"appRef"` + UserRef string `json:"userRef"` + Role string `json:"role"` +} + +// GetAppRef returns __EnsureAppMemberInput.AppRef, and is useful for accessing the field via an interface. +func (v *__EnsureAppMemberInput) GetAppRef() string { return v.AppRef } + +// GetUserRef returns __EnsureAppMemberInput.UserRef, and is useful for accessing the field via an interface. +func (v *__EnsureAppMemberInput) GetUserRef() string { return v.UserRef } + +// GetRole returns __EnsureAppMemberInput.Role, and is useful for accessing the field via an interface. +func (v *__EnsureAppMemberInput) GetRole() string { return v.Role } + // __ExtractParentNodeToMemoryInput is used internally by genqlient type __ExtractParentNodeToMemoryInput struct { ParentRef string `json:"parentRef"` @@ -64123,6 +64287,18 @@ func (v *__ReleaseWorkerInput) GetExpectedHolderUserId() *string { return v.Expe // GetExpectUnheld returns __ReleaseWorkerInput.ExpectUnheld, and is useful for accessing the field via an interface. func (v *__ReleaseWorkerInput) GetExpectUnheld() *bool { return v.ExpectUnheld } +// __RemoveAppMemberInput is used internally by genqlient +type __RemoveAppMemberInput struct { + AppRef string `json:"appRef"` + UserRef string `json:"userRef"` +} + +// GetAppRef returns __RemoveAppMemberInput.AppRef, and is useful for accessing the field via an interface. +func (v *__RemoveAppMemberInput) GetAppRef() string { return v.AppRef } + +// GetUserRef returns __RemoveAppMemberInput.UserRef, and is useful for accessing the field via an interface. +func (v *__RemoveAppMemberInput) GetUserRef() string { return v.UserRef } + // __RemoveMemoryMemberInput is used internally by genqlient type __RemoveMemoryMemberInput struct { MemoryRef string `json:"memoryRef"` @@ -68423,6 +68599,53 @@ func AppKeysMetadata( return data_, err_ } +// The query executed by AppMembers. +const AppMembers_Operation = ` +query AppMembers ($appRef: ID!) { + app(ref: $appRef) { + id + urn + members { + role + createdAt + user { + id + handle + urn + } + } + } +} +` + +// The visible members only. User.name / User.email are null when withheld, and +// are deliberately not selected. App.members is [] when the App's nested read is +// unavailable, so an empty list never proves the App has no members. +func AppMembers( + ctx_ context.Context, + client_ graphql.Client, + appRef string, +) (data_ *AppMembersResponse, err_ error) { + req_ := &graphql.Request{ + OpName: "AppMembers", + Query: AppMembers_Operation, + Variables: &__AppMembersInput{ + AppRef: appRef, + }, + } + + data_ = &AppMembersResponse{} + resp_ := &graphql.Response{Data: data_} + + err_ = client_.MakeRequest( + ctx_, + req_, + resp_, + ) + + return data_, err_ +} + // The query executed by AppRun. const AppRun_Operation = ` query AppRun ($ref: ID!) { @@ -74305,6 +74528,52 @@ func EndTeamSessionWithResumeClaim( return data_, err_ } +// The mutation executed by EnsureAppMember. +const EnsureAppMember_Operation = ` +mutation EnsureAppMember ($appRef: String!, $userRef: String!, $role: String!) { + ensureAppMember(appRef: $appRef, userRef: $userRef, role: $role) { + role + createdAt + user { + id + handle + urn + } + } +} +` + +// An idempotent upsert: the same role is a repeat, a different role changes the +// membership. The selection is the smallest that names the row. +func EnsureAppMember( + ctx_ context.Context, + client_ graphql.Client, + appRef string, + userRef string, + role string, +) (data_ *EnsureAppMemberResponse, err_ error) { + req_ := &graphql.Request{ + OpName: "EnsureAppMember", + Query: EnsureAppMember_Operation, + Variables: &__EnsureAppMemberInput{ + AppRef: appRef, + UserRef: userRef, + Role: role, + }, + } + + data_ = &EnsureAppMemberResponse{} + resp_ := &graphql.Response{Data: data_} + + err_ = client_.MakeRequest( + ctx_, + req_, + resp_, + ) + + return data_, err_ +} + // The mutation executed by ExtractParentNodeToMemory. const ExtractParentNodeToMemory_Operation = ` mutation ExtractParentNodeToMemory ($parentRef: ID!, $targetUrn: String!, $move: Boolean) { @@ -79680,6 +79949,42 @@ func ReleaseWorker( return data_, err_ } +// The mutation executed by RemoveAppMember. +const RemoveAppMember_Operation = ` +mutation RemoveAppMember ($appRef: String!, $userRef: String!) { + removeAppMember(appRef: $appRef, userRef: $userRef) +} +` + +// true whether or not a row existed (a deleteMany server-side), so the result +// says nothing about prior membership. +func RemoveAppMember( + ctx_ context.Context, + client_ graphql.Client, + appRef string, + userRef string, +) (data_ *RemoveAppMemberResponse, err_ error) { + req_ := &graphql.Request{ + OpName: "RemoveAppMember", + Query: RemoveAppMember_Operation, + Variables: &__RemoveAppMemberInput{ + AppRef: appRef, + UserRef: userRef, + }, + } + + data_ = &RemoveAppMemberResponse{} + resp_ := &graphql.Response{Data: data_} + + err_ = client_.MakeRequest( + ctx_, + req_, + resp_, + ) + + return data_, err_ +} + // The mutation executed by RemoveMemoryMember. const RemoveMemoryMember_Operation = ` mutation RemoveMemoryMember ($memoryRef: String!, $userRef: String!) { diff --git a/internal/api/queries/app_members.graphql b/internal/api/queries/app_members.graphql new file mode 100644 index 00000000..f7b4ce4b --- /dev/null +++ b/internal/api/queries/app_members.graphql @@ -0,0 +1,44 @@ +# App human-member management (#956). userRef is an id / email / handle / +# hrn:user: URN passed through unchanged — the server resolves it, and the CLI +# never pre-resolves one (that would make a lookup a user-existence oracle). +# The role is a free string validated against the primary Agent's policy, so +# the CLI never lists or checks roles itself. + +# An idempotent upsert: the same role is a repeat, a different role changes the +# membership. The selection is the smallest that names the row. +mutation EnsureAppMember($appRef: String!, $userRef: String!, $role: String!) { + ensureAppMember(appRef: $appRef, userRef: $userRef, role: $role) { + role + createdAt + user { + id + handle + urn + } + } +} + +# true whether or not a row existed (a deleteMany server-side), so the result +# says nothing about prior membership. +mutation RemoveAppMember($appRef: String!, $userRef: String!) { + removeAppMember(appRef: $appRef, userRef: $userRef) +} + +# The visible members only. User.name / User.email are null when withheld, and +# are deliberately not selected. App.members is [] when the App's nested read is +# unavailable, so an empty list never proves the App has no members. +query AppMembers($appRef: ID!) { + app(ref: $appRef) { + id + urn + members { + role + createdAt + user { + id + handle + urn + } + } + } +} diff --git a/internal/api/unbound-ops.txt b/internal/api/unbound-ops.txt index 7fe3c5e9..b8c16206 100644 --- a/internal/api/unbound-ops.txt +++ b/internal/api/unbound-ops.txt @@ -67,7 +67,7 @@ verifyDecision # server main schema refresh for cli#836; separate CLI parity ou verifyMemoryNodes # cli#802 exposes per-node verifyNode; memory-wide integrity audit needs a separate memory verify command workerWakeSettings # wake-surface v2; cli#828 tracks the paged attention surface, wake settings follow it -[Mutation] 109 unbound +[Mutation] 107 unbound acknowledgeWakes # wake-surface v2 (cor:agt:080:16); cli#828 lane acquireEditorLock addAgentToApp @@ -110,7 +110,6 @@ emailSaveDraft emailSendDraft emailUpdateDraft endAgentSession -ensureAppMember exchangeMoveMessage exchangeReply exportNodeToGoogleDoc @@ -135,7 +134,6 @@ registerContextResetRecovery # server#2162 context-reset host protocol; host ru releaseEditorLock removeAgentFromApp removeAgentImport -removeAppMember removeMemoryFromAgent repairAgentSystemMemory replaceSubtree # server#1407: deprecated field; authorized calls return REPLACE_SUBTREE_RETIRED without writes diff --git a/internal/cmd/agentic/agentic-usage.md b/internal/cmd/agentic/agentic-usage.md index 086fdceb..5c820c0f 100644 --- a/internal/cmd/agentic/agentic-usage.md +++ b/internal/cmd/agentic/agentic-usage.md @@ -1910,6 +1910,36 @@ Conventions: changes only the fields you pass (`--surface` replaces the set); `agent rm ` requires `--yes`. `` is an agent ID **or** a fully-qualified URN (`hrn:agent:acme.com:support-bot`) — no need to resolve an ID first. Memory-attach, AI-config wiring, and app-wiring land next. +- `app member add --user --role ` / `remove --user ` / `list` + (`ensureAppMember` / `removeAppMember` / `App.members`, `cor:dmo:050:04`) + manage an App's HUMAN members. The App is the root `--app` flag, else the + active App; there is no positional App, and an explicit blank `--app ""` + refuses (exit 2) rather than falling back to the active App. `--user` is an + id, email, handle or `hrn:user:` URN passed to the server UNCHANGED — never + resolved or trimmed client-side. `--role` is required on `add` (no default) + and is a server-owned string: help lists no roles, and an invalid role is + refused with the server's own list. `add` is an idempotent upsert (the same + role repeats; a different role changes the membership) and does no pre-read, + so EVERY `add` and `remove` is confirmed (the prompt names the App and whether + it came from `--app` or the active App; remove is reversible, so it says nothing + of "cannot be undone"): `--yes` skips the prompt, a + non-interactive call without it exits 2 with ZERO requests sent, and a + declined/EOF prompt exits 6. `remove` succeeds whether or not a row existed, + so `--json` reports `status: "absent"` (the state now), never "removed"; a + non-manager may remove only themselves, and any other target gets the + server's `Forbidden:` text as-is (no "user not found" — that would be an + existence oracle). A definite refusal keeps the server text and exits by code: + `InvalidRoleError` 2, `MaxMembersExceededError` and `OrphanAppError` 5, + not-found 4, auth 3, and the plain-text `Forbidden:` 1. ANY other failure of a write + — transport error, `data` plus `errors`, a masked internal error, an empty + payload — exits 7 with "outcome unknown", prints no success DTO, and is never + retried: `ensureAppMember` re-reads the row after commit, so the write may + have landed; check with `app member list`. `list --json` is + `{appId, appUrn, members: [{user: {id, handle, urn}, role, createdAt}]}` + with `members: []` (never null) when empty. The server returns `[]` when the + member list is unreadable by you, so an empty list means "no members visible + to you", NOT "no members"; a null App is exit 4, a malformed row is an error. + User names and emails are withheld by the server and not selected. - `app agent add ` / `remove ` wrap `installAgentIntoApp` / `uninstallAgentFromApp` — the AppAgent join (`cor:dmo:050:03`), and the only way to attach an Agent to an App you diff --git a/internal/cmd/app/app.go b/internal/cmd/app/app.go index 0cd5d75e..86e5f87c 100644 --- a/internal/cmd/app/app.go +++ b/internal/cmd/app/app.go @@ -25,6 +25,7 @@ func NewCmdApp(f *cmdutil.Factory) *cobra.Command { cmd.AddCommand(newCmdUpdate(f)) cmd.AddCommand(newCmdUninstall(f)) cmd.AddCommand(newCmdKey(f)) + cmd.AddCommand(newCmdMember(f)) return cmd } diff --git a/internal/cmd/app/member.go b/internal/cmd/app/member.go new file mode 100644 index 00000000..95fa64d3 --- /dev/null +++ b/internal/cmd/app/member.go @@ -0,0 +1,338 @@ +package app + +import ( + "fmt" + "io" + "strconv" + "strings" + "unicode" + + "github.com/spf13/cobra" + + "github.com/hadron-memory/hadron-cli/internal/api" + "github.com/hadron-memory/hadron-cli/internal/api/gen" + "github.com/hadron-memory/hadron-cli/internal/cmdutil" + "github.com/hadron-memory/hadron-cli/internal/exitcode" + "github.com/hadron-memory/hadron-cli/internal/output" +) + +// appMemberUserDTO identifies a member without the fields the server withholds +// (name, email): only id, handle and URN are selected. +type appMemberUserDTO struct { + ID string `json:"id"` + Handle *string `json:"handle"` + URN *string `json:"urn"` +} + +// appMemberRowDTO is one visible member in `app member list`. +type appMemberRowDTO struct { + User appMemberUserDTO `json:"user"` + Role string `json:"role"` + CreatedAt string `json:"createdAt"` +} + +// appMemberListDTO is the stable --json shape of `app member list`. Members is +// the VISIBLE rows: an empty list never proves the App has no members. +type appMemberListDTO struct { + AppID string `json:"appId"` + AppURN string `json:"appUrn"` + Members []appMemberRowDTO `json:"members"` +} + +// appMemberAddDTO is the stable --json shape of a confirmed add/upsert. +type appMemberAddDTO struct { + App string `json:"app"` + UserRef string `json:"userRef"` + User appMemberUserDTO `json:"user"` + Role string `json:"role"` + CreatedAt string `json:"createdAt"` + Status string `json:"status"` +} + +// appMemberRemoveDTO is the stable --json shape of a remove. The server answers +// true whether or not a row existed, so Status is "absent" — the state now — +// never "removed". +type appMemberRemoveDTO struct { + App string `json:"app"` + UserRef string `json:"userRef"` + Status string `json:"status"` +} + +func newCmdMember(f *cmdutil.Factory) *cobra.Command { + cmd := &cobra.Command{ + Use: "member ", + Aliases: []string{"members"}, + Short: "Manage an App's human members", + Long: `Add, remove and list the human members of an App (cor:dmo:050:04). + +The App is the root --app flag, else the active App (` + "`hadron app use`" + `); +there is no positional App. --user is passed to the server unchanged — an id, +email, handle or hrn:user: URN — and is never resolved here. + +Roles are owned by the server (the App's primary Agent policy), so no role list +is shown or checked locally; an invalid role is refused with the server's own +list. Authority is the server's too: an App member manager, or the member +themselves for remove.`, + } + cmd.AddCommand(newCmdMemberList(f)) + cmd.AddCommand(newCmdMemberAdd(f)) + cmd.AddCommand(newCmdMemberRemove(f)) + return cmd +} + +// memberTargetApp resolves the ONE App a member command acts on. The shared +// resolver cannot be used as-is: Factory.App treats an explicit blank `--app ""` +// exactly like no flag and falls back to the active App, which would silently +// aim a membership write at an App the caller never named. An explicit blank +// refuses; an absent flag uses the active App; no App at all refuses. +// +// It also returns which branch answered ("--app" or "the active App") so the +// prompt and receipt can say so: a membership write aimed at an ambient App +// must not look like one aimed at a named one. +func memberTargetApp(cmd *cobra.Command, f *cmdutil.Factory) (app, source string, err error) { + explicit := false + if fl := cmd.Flag("app"); fl != nil && fl.Changed { + explicit = true + if strings.TrimSpace(f.AppFlag) == "" { + return "", "", exitcode.Newf(exitcode.Usage, "--app was given a blank value — name the App, or omit --app to use the active App") + } + } + app, err = f.App() + if err != nil { + return "", "", err + } + if app == "" { + return "", "", exitcode.Newf(exitcode.Usage, "an App is required: pass --app or select one with `hadron app use`") + } + if explicit { + return app, "from --app", nil + } + return app, "from the active App", nil +} + +// requireMemberUser refuses a blank --user. The value itself is passed on +// unchanged — no trimming, case folding or resolving. +func requireMemberUser(user string) error { + if strings.TrimSpace(user) == "" { + return exitcode.Newf(exitcode.Usage, "--user is required: an id, email, handle or hrn:user: URN") + } + return nil +} + +// memberWriteError classifies a failed membership write. A definite refusal +// keeps the server's wording and exit code; everything else is outcome UNKNOWN +// (see api.AppMemberWriteRefused) — non-zero, no success DTO, never retried. +func memberWriteError(err error) error { + if api.AppMemberWriteRefused(err) { + return api.MapError(err) + } + return exitcode.Newf(exitcode.Unavailable, + "outcome unknown: %s. The membership change may or may not have been applied and was not retried — "+ + "run `hadron app member list` to check (a list showing no row is not proof: rows you cannot see are withheld)", + api.MapError(err)) +} + +func memberUserDTO(id string, handle, urn *string) appMemberUserDTO { + return appMemberUserDTO{ID: id, Handle: handle, URN: urn} +} + +func newCmdMemberList(f *cmdutil.Factory) *cobra.Command { + return &cobra.Command{ + Use: "list", + Aliases: []string{"ls"}, + Short: "List the members of an App that you can see", + Long: `List the members of the App (--app, else the active App) that you can see. + +An empty list means "no members visible to you", never "no members": the server +returns [] when the App's member list is not readable by you.`, + Example: ` hadron app member list + hadron app member list --app hrn:app:acme.com:eng-team --json`, + Args: cobra.NoArgs, + RunE: func(cmd *cobra.Command, args []string) error { + appRef, source, err := memberTargetApp(cmd, f) + if err != nil { + return err + } + client, err := f.GraphQLClient() + if err != nil { + return err + } + resp, err := gen.AppMembers(cmd.Context(), client, appRef) + if err != nil { + return api.MapError(err) + } + if resp == nil || resp.App == nil { + return exitcode.Newf(exitcode.NotFound, "app %q not found, or not visible to you", appRef) + } + dto := appMemberListDTO{AppID: resp.App.Id, AppURN: resp.App.Urn, Members: []appMemberRowDTO{}} + for _, m := range resp.App.Members { + if m == nil || m.User == nil { + return exitcode.Newf(exitcode.Error, "server returned a malformed member row for %q", appRef) + } + dto.Members = append(dto.Members, appMemberRowDTO{ + User: memberUserDTO(m.User.Id, m.User.Handle, m.User.Urn), + Role: m.Role, + CreatedAt: m.CreatedAt, + }) + } + return output.Write(f.IOStreams, f.JSON, dto, func(w io.Writer) error { + if _, err := fmt.Fprintf(w, "app: %s (%s)\n", memberText(dto.AppURN), source); err != nil { + return err + } + if len(dto.Members) == 0 { + _, err := fmt.Fprintln(w, "no members visible to you") + return err + } + t := output.NewTable(w, "USER ID", "HANDLE", "ROLE", "ADDED") + for _, m := range dto.Members { + t.Row(m.User.ID, memberCell(m.User.Handle), memberText(m.Role), m.CreatedAt) + } + return t.Flush() + }) + }, + } +} + +func newCmdMemberAdd(f *cmdutil.Factory) *cobra.Command { + var user, role string + var yes bool + cmd := &cobra.Command{ + Use: "add --user --role ", + Short: "Add a user to an App, or change their role", + Long: `Add a user to the App (--app, else the active App) with the given role. + +This is an idempotent upsert: the same role is a repeat, a different role +changes the membership. Because the command never reads the list first, EVERY +add is confirmed — pass --yes to skip the prompt; a non-interactive caller +without --yes is refused before anything is sent. + +If the response is lost or ambiguous the outcome is reported as unknown and the +write is not retried; check with ` + "`hadron app member list`" + `.`, + Example: ` hadron app member add --user ada@acme.com --role member --yes + hadron app member add --app hrn:app:acme.com:eng-team --user hrn:user:ada --role owner`, + Args: cobra.NoArgs, + RunE: func(cmd *cobra.Command, args []string) error { + if err := requireMemberUser(user); err != nil { + return err + } + if strings.TrimSpace(role) == "" { + return exitcode.Newf(exitcode.Usage, "--role is required: roles are defined by the server and no default is assumed") + } + appRef, source, err := memberTargetApp(cmd, f) + if err != nil { + return err + } + prompt := fmt.Sprintf("Add user %s to app %s (%s) with role %s?", strconv.Quote(user), appRef, source, strconv.Quote(role)) + if err := cmdutil.Confirm(f.IOStreams, yes, prompt); err != nil { + return err + } + client, err := f.GraphQLClient() + if err != nil { + return err + } + resp, err := gen.EnsureAppMember(cmd.Context(), client, appRef, user, role) + if err != nil { + return memberWriteError(err) + } + if resp == nil || resp.EnsureAppMember == nil || resp.EnsureAppMember.User == nil { + return exitcode.Newf(exitcode.Unavailable, + "outcome unknown: the server returned no member row. Run `hadron app member list` to check (a list showing no row is not proof: rows you cannot see are withheld)") + } + m := resp.EnsureAppMember + dto := appMemberAddDTO{ + App: appRef, + UserRef: user, + User: memberUserDTO(m.User.Id, m.User.Handle, m.User.Urn), + Role: m.Role, + CreatedAt: m.CreatedAt, + Status: "member", + } + return output.Write(f.IOStreams, f.JSON, dto, func(w io.Writer) error { + _, err := fmt.Fprintf(w, "✓ %s is a member of %s (%s) with role %s\n", memberText(user), appRef, source, memberText(dto.Role)) + return err + }) + }, + } + cmd.Flags().StringVar(&user, "user", "", "user to add: an id, email, handle or hrn:user: URN, passed to the server unchanged (required)") + cmd.Flags().StringVar(&role, "role", "", "the member's role, as the server defines roles (required)") + cmd.Flags().BoolVarP(&yes, "yes", "y", false, "skip the confirmation prompt") + return cmd +} + +func newCmdMemberRemove(f *cmdutil.Factory) *cobra.Command { + var user string + var yes bool + cmd := &cobra.Command{ + Use: "remove --user ", + Aliases: []string{"rm"}, + Short: "Remove a user from an App", + Long: `Remove a user from the App (--app, else the active App). Reversible: add the +user again to restore the membership. + +The server answers the same whether or not the user was a member, so success +means "not a member now", not "was removed". An App member manager may remove +anyone; anyone else may remove only themselves, and every other target gets the +same refusal — which is printed as-is, so it cannot be used to test whether a +user exists. + +Confirmed first: pass --yes to skip the prompt; a non-interactive caller +without --yes is refused before anything is sent.`, + Example: ` hadron app member remove --user ada@acme.com --yes`, + Args: cobra.NoArgs, + RunE: func(cmd *cobra.Command, args []string) error { + if err := requireMemberUser(user); err != nil { + return err + } + appRef, source, err := memberTargetApp(cmd, f) + if err != nil { + return err + } + // Confirm, not ConfirmDeletion: removal is reversible (add the user + // again; their personal memory is retained), so "cannot be undone" + // would be false. + prompt := fmt.Sprintf("Remove user %s from app %s (%s)?", strconv.Quote(user), appRef, source) + if err := cmdutil.Confirm(f.IOStreams, yes, prompt); err != nil { + return err + } + client, err := f.GraphQLClient() + if err != nil { + return err + } + resp, err := gen.RemoveAppMember(cmd.Context(), client, appRef, user) + if err != nil { + return memberWriteError(err) + } + if resp == nil || !resp.RemoveAppMember { + return exitcode.Newf(exitcode.Unavailable, + "outcome unknown: the server did not confirm the removal. Run `hadron app member list` to check (a list showing no row is not proof: rows you cannot see are withheld)") + } + dto := appMemberRemoveDTO{App: appRef, UserRef: user, Status: "absent"} + return output.Write(f.IOStreams, f.JSON, dto, func(w io.Writer) error { + _, err := fmt.Fprintf(w, "✓ %s is not a member of %s (%s) now\n", memberText(user), appRef, source) + return err + }) + }, + } + cmd.Flags().StringVar(&user, "user", "", "user to remove: an id, email, handle or hrn:user: URN, passed to the server unchanged (required)") + cmd.Flags().BoolVarP(&yes, "yes", "y", false, "skip the confirmation prompt") + return cmd +} + +// memberText quotes a string carrying control characters (newline, ESC…) so a +// server- or user-supplied value cannot split a row or reach the terminal as an +// escape sequence. Text only: --json keeps the source value. +func memberText(s string) string { + for _, r := range s { + if unicode.IsControl(r) { + return strconv.Quote(s) + } + } + return s +} + +func memberCell(s *string) string { + if s == nil || *s == "" { + return "—" + } + return memberText(*s) +} diff --git a/internal/cmd/app_member_cmd_test.go b/internal/cmd/app_member_cmd_test.go new file mode 100644 index 00000000..8956cda2 --- /dev/null +++ b/internal/cmd/app_member_cmd_test.go @@ -0,0 +1,540 @@ +package cmd + +import ( + "encoding/json" + "net/http" + "net/http/httptest" + "os" + "path/filepath" + "strings" + "sync" + "testing" + + "github.com/hadron-memory/hadron-cli/internal/config" + "github.com/hadron-memory/hadron-cli/internal/exitcode" +) + +// cli#956: `app member add|remove|list`. Every write test asserts how many +// mutation calls reached the server — the guards that matter (confirm before +// write, no retry, no pre-read) are all "zero or exactly one call". + +const ( + memberApp = "hrn:app:acme.com:eng-team" + memberActiveApp = "hrn:app:acme.com:active" +) + +const ensureMemberResp = `{"data":{"ensureAppMember":{"role":"member","createdAt":"2026-10-25T00:00:00Z", + "user":{"id":"usr1","handle":"ada","urn":"hrn:user:ada"}}}}` + +const removeMemberResp = `{"data":{"removeAppMember":true}}` + +const appMembersResp = `{"data":{"app":{"id":"capp100000000000000000000","urn":"hrn:app:acme.com:eng-team","members":[ + {"role":"owner","createdAt":"2026-10-01T00:00:00Z","user":{"id":"usr1","handle":"ada","urn":"hrn:user:ada"}}, + {"role":"member","createdAt":"2026-10-02T00:00:00Z","user":{"id":"usr2","handle":null,"urn":null}}]}}}` + +func memberGQLError(msg, code string) string { + b, _ := json.Marshal(map[string]any{"errors": []any{map[string]any{ + "message": msg, "extensions": map[string]any{"code": code}}}}) + return string(b) +} + +// countingGraphQL answers by operation name and counts calls per operation. +func countingGraphQL(t *testing.T, responses map[string]string) (*httptest.Server, func(op string) int, func(op string) json.RawMessage) { + t.Helper() + var mu sync.Mutex + calls := map[string]int{} + vars := map[string]json.RawMessage{} + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.Method == http.MethodGet && r.URL.Path == "/capabilities" { + _, _ = w.Write([]byte(`{"capabilities":[]}`)) + return + } + var body struct { + OperationName string `json:"operationName"` + Variables json.RawMessage `json:"variables"` + } + _ = json.NewDecoder(r.Body).Decode(&body) + mu.Lock() + calls[body.OperationName]++ + vars[body.OperationName] = body.Variables + mu.Unlock() + resp, ok := responses[body.OperationName] + if !ok { + t.Errorf("unexpected operation %q", body.OperationName) + resp = memberGQLError("unexpected operation", "INTERNAL_SERVER_ERROR") + } + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(resp)) + })) + t.Cleanup(srv.Close) + return srv, + func(op string) int { mu.Lock(); defer mu.Unlock(); return calls[op] }, + func(op string) json.RawMessage { mu.Lock(); defer mu.Unlock(); return vars[op] } +} + +func setActiveApp(t *testing.T, ref string) { + t.Helper() + path, err := config.File() + if err != nil { + t.Fatal(err) + } + if err := os.MkdirAll(filepath.Dir(path), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(path, []byte("app = \""+ref+"\"\n"), 0o600); err != nil { + t.Fatal(err) + } +} + +func memberVar(t *testing.T, raw json.RawMessage, key string) any { + t.Helper() + var v map[string]any + if err := json.Unmarshal(raw, &v); err != nil { + t.Fatalf("vars: %v (%s)", err, raw) + } + return v[key] +} + +// Test 1: every userRef form reaches the server byte-identical. +func TestAppMemberUserRefPassesThroughUnchanged(t *testing.T) { + for _, ref := range []string{"usr1", "Ada@Acme.COM", "Ada", "hrn:user:ada", " ada "} { + srv, _, vars := countingGraphQL(t, map[string]string{"EnsureAppMember": ensureMemberResp, "RemoveAppMember": removeMemberResp}) + for _, verb := range []string{"add", "remove"} { + f, _ := testFactory(t) + args := []string{"app", "member", verb, "--user", ref, "--app", memberApp, "--yes", "--json", "--server", srv.URL} + if verb == "add" { + args = append(args, "--role", "member") + } + root := NewRootCmd(f) + root.SetArgs(args) + if err := root.Execute(); err != nil { + t.Fatalf("%s %q: %v", verb, ref, err) + } + op := map[string]string{"add": "EnsureAppMember", "remove": "RemoveAppMember"}[verb] + if got := memberVar(t, vars(op), "userRef"); got != ref { + t.Errorf("%s: userRef %q reached the server as %q", verb, ref, got) + } + if got := memberVar(t, vars(op), "appRef"); got != memberApp { + t.Errorf("%s: appRef = %v", verb, got) + } + } + } +} + +// Test 2: explicit --app, the active App, and neither. +func TestAppMemberAppResolution(t *testing.T) { + cases := []struct { + name string + active string + flag []string + want string // "" = Usage refusal with zero calls + }{ + {"explicit", "", []string{"--app", memberApp}, memberApp}, + {"explicit beats active", memberActiveApp, []string{"--app", memberApp}, memberApp}, + {"active default", memberActiveApp, nil, memberActiveApp}, + {"neither", "", nil, ""}, + } + for _, verb := range []string{"add", "remove", "list"} { + for _, c := range cases { + t.Run(verb+"/"+c.name, func(t *testing.T) { + srv, calls, vars := countingGraphQL(t, map[string]string{ + "EnsureAppMember": ensureMemberResp, "RemoveAppMember": removeMemberResp, "AppMembers": appMembersResp}) + f, _ := testFactory(t) + if c.active != "" { + setActiveApp(t, c.active) + } + args := append([]string{"app", "member", verb}, c.flag...) + if verb != "list" { + args = append(args, "--user", "ada", "--yes") + } + if verb == "add" { + args = append(args, "--role", "member") + } + root := NewRootCmd(f) + root.SetArgs(append(args, "--json", "--server", srv.URL)) + err := root.Execute() + op := map[string]string{"add": "EnsureAppMember", "remove": "RemoveAppMember", "list": "AppMembers"}[verb] + if c.want == "" { + if exitCodeFor(err) != exitcode.Usage || calls(op) != 0 { + t.Fatalf("no App must be a Usage refusal with 0 calls: err=%v calls=%d", err, calls(op)) + } + return + } + if err != nil { + t.Fatal(err) + } + if got := memberVar(t, vars(op), "appRef"); got != c.want { + t.Errorf("appRef = %v, want %s", got, c.want) + } + }) + } + } +} + +// Test 3: an explicit blank --app refuses even when an active App is set. The +// shared resolver would silently use the active App. +func TestAppMemberExplicitBlankAppRefuses(t *testing.T) { + for _, blank := range []string{"", " "} { + for _, verb := range []string{"add", "remove", "list"} { + srv, calls, _ := countingGraphQL(t, map[string]string{ + "EnsureAppMember": ensureMemberResp, "RemoveAppMember": removeMemberResp, "AppMembers": appMembersResp}) + f, _ := testFactory(t) + setActiveApp(t, memberActiveApp) + args := []string{"app", "member", verb, "--app", blank, "--yes"} + if verb != "list" { + args = append(args, "--user", "ada") + } + if verb == "add" { + args = append(args, "--role", "member") + } + if verb == "list" { + args = args[:len(args)-1] // list takes no --yes + } + root := NewRootCmd(f) + root.SetArgs(append(args, "--server", srv.URL)) + err := root.Execute() + total := calls("EnsureAppMember") + calls("RemoveAppMember") + calls("AppMembers") + if exitCodeFor(err) != exitcode.Usage || total != 0 { + t.Errorf("%s --app %q: want Usage + 0 calls, got err=%v calls=%d", verb, blank, err, total) + } + } + } +} + +// Test 4: add without --role is a Usage error and invents no default. +func TestAppMemberAddRequiresRole(t *testing.T) { + srv, calls, _ := countingGraphQL(t, map[string]string{"EnsureAppMember": ensureMemberResp}) + f, _ := testFactory(t) + root := NewRootCmd(f) + root.SetArgs([]string{"app", "member", "add", "--user", "ada", "--app", memberApp, "--yes", "--server", srv.URL}) + err := root.Execute() + if exitCodeFor(err) != exitcode.Usage || calls("EnsureAppMember") != 0 { + t.Fatalf("want Usage + 0 calls, got err=%v calls=%d", err, calls("EnsureAppMember")) + } + f2, _ := testFactory(t) + root = NewRootCmd(f2) + root.SetArgs([]string{"app", "member", "add", "--user", "ada", "--role", " ", "--app", memberApp, "--yes", "--server", srv.URL}) + if err := root.Execute(); exitCodeFor(err) != exitcode.Usage || calls("EnsureAppMember") != 0 { + t.Fatalf("blank role: want Usage + 0 calls, got err=%v", err) + } +} + +// Test 5: confirmation. Nothing is written before it. +func TestAppMemberConfirmation(t *testing.T) { + for _, verb := range []string{"add", "remove"} { + op := map[string]string{"add": "EnsureAppMember", "remove": "RemoveAppMember"}[verb] + base := []string{"app", "member", verb, "--user", "Ada@acme.com", "--app", memberApp} + if verb == "add" { + base = append(base, "--role", "owner") + } + run := func(t *testing.T, tty bool, answers string, extra ...string) (int, string, error) { + srv, calls, _ := countingGraphQL(t, map[string]string{"EnsureAppMember": ensureMemberResp, "RemoveAppMember": removeMemberResp}) + var errOut string + var err error + args := append(append([]string{}, base...), extra...) + args = append(args, "--server", srv.URL) + if tty { + f, _, eo := testFactoryTTY(t, answers) + root := NewRootCmd(f) + root.SetArgs(args) + err = root.Execute() + errOut = eo.String() + } else { + f, _ := testFactory(t) + root := NewRootCmd(f) + root.SetArgs(args) + err = root.Execute() + } + return calls(op), errOut, err + } + t.Run(verb+"/non-TTY without --yes", func(t *testing.T) { + n, _, err := run(t, false, "") + if exitCodeFor(err) != exitcode.Usage || n != 0 { + t.Fatalf("want Usage + 0 calls, got err=%v calls=%d", err, n) + } + }) + t.Run(verb+"/TTY no", func(t *testing.T) { + n, _, err := run(t, true, "n\n") + if exitCodeFor(err) != exitcode.Cancelled || n != 0 { + t.Fatalf("want Cancelled + 0 calls, got err=%v calls=%d", err, n) + } + }) + t.Run(verb+"/TTY EOF", func(t *testing.T) { + n, _, err := run(t, true, "") + if exitCodeFor(err) != exitcode.Cancelled || n != 0 { + t.Fatalf("want Cancelled + 0 calls, got err=%v calls=%d", err, n) + } + }) + t.Run(verb+"/TTY yes names app user role", func(t *testing.T) { + n, prompt, err := run(t, true, "y\n") + if err != nil || n != 1 { + t.Fatalf("want success + 1 call, got err=%v calls=%d", err, n) + } + for _, want := range []string{memberApp, `"Ada@acme.com"`} { + if !strings.Contains(prompt, want) { + t.Errorf("prompt must name %s: %q", want, prompt) + } + } + if strings.Contains(prompt, "cannot be undone") { + t.Errorf("a reversible membership change must not claim it cannot be undone: %q", prompt) + } + if !strings.Contains(prompt, "from --app") { + t.Errorf("prompt must name where the App came from: %q", prompt) + } + if verb == "add" && !strings.Contains(prompt, `"owner"`) { + t.Errorf("add prompt must name the role: %q", prompt) + } + }) + t.Run(verb+"/--yes", func(t *testing.T) { + n, prompt, err := run(t, true, "", "--yes") + if err != nil || n != 1 || prompt != "" { + t.Fatalf("want 1 call and no prompt, got err=%v calls=%d prompt=%q", err, n, prompt) + } + }) + } +} + +// Test 6: same-role repeat and changed-role add each make exactly one call, and +// nothing reads the list first. +func TestAppMemberAddNeverPreReads(t *testing.T) { + srv, calls, _ := countingGraphQL(t, map[string]string{"EnsureAppMember": ensureMemberResp}) + for _, role := range []string{"member", "member", "owner"} { + f, out := testFactory(t) + root := NewRootCmd(f) + root.SetArgs([]string{"app", "member", "add", "--user", "ada", "--role", role, "--app", memberApp, "--yes", "--json", "--server", srv.URL}) + if err := root.Execute(); err != nil { + t.Fatal(err) + } + var dto struct{ Status, Role, UserRef string } + if err := json.Unmarshal([]byte(out.String()), &dto); err != nil || dto.Status != "member" { + t.Fatalf("dto: %v %s", err, out.String()) + } + } + if calls("EnsureAppMember") != 3 || calls("AppMembers") != 0 { + t.Errorf("want 3 upserts and no list read, got %d / %d", calls("EnsureAppMember"), calls("AppMembers")) + } +} + +// Test 7: remove is "absent now", never "removed", and repeats succeed. +func TestAppMemberRemoveReportsAbsentNotRemoved(t *testing.T) { + srv, calls, _ := countingGraphQL(t, map[string]string{"RemoveAppMember": removeMemberResp}) + for i := 0; i < 2; i++ { + f, out := testFactory(t) + root := NewRootCmd(f) + root.SetArgs([]string{"app", "member", "remove", "--user", "ada", "--app", memberApp, "--yes", "--json", "--server", srv.URL}) + if err := root.Execute(); err != nil { + t.Fatal(err) + } + var dto struct{ Status string } + if err := json.Unmarshal([]byte(out.String()), &dto); err != nil || dto.Status != "absent" { + t.Fatalf("status must be absent: %v %s", err, out.String()) + } + f, out = testFactory(t) + root = NewRootCmd(f) + root.SetArgs([]string{"app", "member", "remove", "--user", "ada", "--app", memberApp, "--yes", "--server", srv.URL}) + if err := root.Execute(); err != nil { + t.Fatal(err) + } + if s := out.String(); strings.Contains(s, "removed") || !strings.Contains(s, "not a member") { + t.Errorf("text must say 'not a member', never 'removed': %q", s) + } + } + if calls("RemoveAppMember") != 4 { + t.Errorf("calls = %d", calls("RemoveAppMember")) + } +} + +// Test 8: typed refusals keep the server's text and are definite. +func TestAppMemberDefiniteRefusalsKeepServerText(t *testing.T) { + cases := []struct{ msg, code string }{ + {`role "boss" is not valid; allowed: owner, member`, "InvalidRoleError"}, + {"App already has the maximum number of members", "MaxMembersExceededError"}, + {"this would leave the App with no owner", "OrphanAppError"}, + {"Forbidden: only an App member manager may do this", "INTERNAL_SERVER_ERROR"}, + } + for _, c := range cases { + srv, calls, _ := countingGraphQL(t, map[string]string{"EnsureAppMember": memberGQLError(c.msg, c.code)}) + f, _ := testFactory(t) + root := NewRootCmd(f) + root.SetArgs([]string{"app", "member", "add", "--user", "ada", "--role", "boss", "--app", memberApp, "--yes", "--server", srv.URL}) + err := root.Execute() + if err == nil || exitCodeFor(err) == 0 { + t.Fatalf("%s: want a non-zero refusal", c.code) + } + if !strings.Contains(err.Error(), c.msg) { + t.Errorf("%s: server text must be kept verbatim: %v", c.code, err) + } + if strings.Contains(strings.ToLower(err.Error()), "unknown") || calls("EnsureAppMember") != 1 { + t.Errorf("%s: a definite refusal must not read as outcome unknown / retry: %v calls=%d", c.code, err, calls("EnsureAppMember")) + } + } +} + +// Test 9: outcome unknown — non-zero, no success DTO, exactly one call, and it +// points at `app member list`. +func TestAppMemberOutcomeUnknown(t *testing.T) { + partial := `{"data":{"ensureAppMember":null},"errors":[{"message":"boom","extensions":{"code":"INTERNAL_SERVER_ERROR"}}]}` + cases := map[string]string{ + "post-write internal error": memberGQLError("Internal server error", "INTERNAL_SERVER_ERROR"), + "data and errors": partial, + "no code": `{"errors":[{"message":"mystery"}]}`, + "null payload": `{"data":{"ensureAppMember":null}}`, + "mixed with a refusal": `{"errors":[{"message":"x","extensions":{"code":"InvalidRoleError"}},{"message":"y","extensions":{"code":"INTERNAL_SERVER_ERROR"}}]}`, + } + for name, resp := range cases { + t.Run(name, func(t *testing.T) { + srv, calls, _ := countingGraphQL(t, map[string]string{"EnsureAppMember": resp}) + f, out := testFactory(t) + root := NewRootCmd(f) + root.SetArgs([]string{"app", "member", "add", "--user", "ada", "--role", "member", "--app", memberApp, "--yes", "--json", "--server", srv.URL}) + err := root.Execute() + if err == nil { + t.Fatal("must exit non-zero") + } + if out.String() != "" { + t.Errorf("no success DTO on stdout: %q", out.String()) + } + if !strings.Contains(err.Error(), "outcome unknown") || !strings.Contains(err.Error(), "app member list") { + t.Errorf("must say outcome unknown and point at list: %v", err) + } + if calls("EnsureAppMember") != 1 { + t.Errorf("must not retry: %d calls", calls("EnsureAppMember")) + } + }) + } + t.Run("transport failure", func(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path == "/capabilities" { + _, _ = w.Write([]byte(`{"capabilities":[]}`)) + return + } + hj, _ := w.(http.Hijacker) + conn, _, _ := hj.Hijack() + _ = conn.Close() + })) + defer srv.Close() + f, out := testFactory(t) + root := NewRootCmd(f) + root.SetArgs([]string{"app", "member", "remove", "--user", "ada", "--app", memberApp, "--yes", "--server", srv.URL}) + err := root.Execute() + if err == nil || out.String() != "" || !strings.Contains(err.Error(), "outcome unknown") { + t.Fatalf("want outcome unknown, no stdout: err=%v out=%q", err, out.String()) + } + }) +} + +// Test 10: a foreign / unknown remove target gets the server's Forbidden text +// as-is, never "not found" (that would reopen the user-existence oracle). +func TestAppMemberRemoveForbiddenIsNotTranslated(t *testing.T) { + msg := "Forbidden: you must manage this App or be the user themselves" + srv, _, _ := countingGraphQL(t, map[string]string{"RemoveAppMember": memberGQLError(msg, "INTERNAL_SERVER_ERROR")}) + f, _ := testFactory(t) + root := NewRootCmd(f) + root.SetArgs([]string{"app", "member", "remove", "--user", "nobody@example.com", "--app", memberApp, "--yes", "--server", srv.URL}) + err := root.Execute() + if err == nil || !strings.Contains(err.Error(), msg) { + t.Fatalf("want the Forbidden text verbatim: %v", err) + } + if exitCodeFor(err) == exitcode.NotFound || strings.Contains(strings.ToLower(err.Error()), "not found") { + t.Errorf("must not be translated to not-found: %v", err) + } +} + +// Test 11: list. +func TestAppMemberList(t *testing.T) { + t.Run("rows", func(t *testing.T) { + srv, _, vars := countingGraphQL(t, map[string]string{"AppMembers": appMembersResp}) + f, out := testFactory(t) + root := NewRootCmd(f) + root.SetArgs([]string{"app", "member", "list", "--app", memberApp, "--json", "--server", srv.URL}) + if err := root.Execute(); err != nil { + t.Fatal(err) + } + var dto struct { + AppURN string `json:"appUrn"` + Members []struct { + Role string `json:"role"` + User struct { + ID string `json:"id"` + Handle *string `json:"handle"` + } `json:"user"` + } `json:"members"` + } + if err := json.Unmarshal([]byte(out.String()), &dto); err != nil || len(dto.Members) != 2 || dto.Members[0].Role != "owner" || + dto.Members[1].User.Handle != nil || dto.AppURN != memberApp { + t.Fatalf("dto: %v %s", err, out.String()) + } + _ = vars + }) + t.Run("empty", func(t *testing.T) { + srv, _, _ := countingGraphQL(t, map[string]string{"AppMembers": `{"data":{"app":{"id":"a","urn":"hrn:app:acme.com:eng-team","members":[]}}}`}) + f, out := testFactory(t) + root := NewRootCmd(f) + root.SetArgs([]string{"app", "member", "list", "--app", memberApp, "--server", srv.URL}) + if err := root.Execute(); err != nil || !strings.HasSuffix(strings.TrimSpace(out.String()), "no members visible to you") || + !strings.Contains(out.String(), "(from --app)") { + t.Fatalf("err=%v out=%q", err, out.String()) + } + f, out = testFactory(t) + root = NewRootCmd(f) + root.SetArgs([]string{"app", "member", "list", "--app", memberApp, "--json", "--server", srv.URL}) + if err := root.Execute(); err != nil || !strings.Contains(out.String(), `"members": []`) && !strings.Contains(out.String(), `"members":[]`) { + t.Fatalf("json must carry [] not null: %v %s", err, out.String()) + } + }) + t.Run("null app is not found", func(t *testing.T) { + srv, _, _ := countingGraphQL(t, map[string]string{"AppMembers": `{"data":{"app":null}}`}) + f, out := testFactory(t) + root := NewRootCmd(f) + root.SetArgs([]string{"app", "member", "list", "--app", memberApp, "--json", "--server", srv.URL}) + err := root.Execute() + if exitCodeFor(err) != exitcode.NotFound || strings.Contains(out.String(), "members") { + t.Fatalf("want NotFound and no list: err=%v out=%q", err, out.String()) + } + }) + t.Run("malformed row is an error", func(t *testing.T) { + srv, _, _ := countingGraphQL(t, map[string]string{"AppMembers": `{"data":{"app":{"id":"a","urn":"u","members":[null]}}}`}) + f, _ := testFactory(t) + root := NewRootCmd(f) + root.SetArgs([]string{"app", "member", "list", "--app", memberApp, "--server", srv.URL}) + if err := root.Execute(); err == nil { + t.Fatal("a malformed row must not become a successful list") + } + }) + t.Run("errors are errors", func(t *testing.T) { + srv, _, _ := countingGraphQL(t, map[string]string{"AppMembers": memberGQLError("nope", "FORBIDDEN")}) + f, _ := testFactory(t) + root := NewRootCmd(f) + root.SetArgs([]string{"app", "member", "list", "--app", memberApp, "--server", srv.URL}) + if err := root.Execute(); exitCodeFor(err) != exitcode.Forbidden { + t.Fatalf("want Forbidden, got %v", err) + } + }) +} + +// The selection must never ask for the fields the server withholds. +func TestAppMemberQueriesSelectNoWithheldFields(t *testing.T) { + var query string + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path == "/capabilities" { + _, _ = w.Write([]byte(`{"capabilities":[]}`)) + return + } + var body struct{ Query string } + _ = json.NewDecoder(r.Body).Decode(&body) + query = body.Query + _, _ = w.Write([]byte(appMembersResp)) + })) + defer srv.Close() + f, _ := testFactory(t) + root := NewRootCmd(f) + root.SetArgs([]string{"app", "member", "list", "--app", memberApp, "--server", srv.URL}) + if err := root.Execute(); err != nil { + t.Fatal(err) + } + for _, field := range []string{"email", "name"} { + if strings.Contains(query, field) { + t.Errorf("query selects %q: %s", field, query) + } + } +} From b610128b43026f6dbc1b49d58b7d7655781b3000 Mon Sep 17 00:00:00 2001 From: Holger Selover-Stephan Date: Sun, 25 Oct 2026 14:13:39 +0000 Subject: [PATCH 2/4] app member: partial results and nested-field errors are outcome unknown (#956) LOCAL review (Joost): a response carrying member data plus a nested FORBIDDEN was classified a definite refusal. A populated result next to an error, and any error whose path is below the mutation root, now take the unknown path (exit 7, no DTO, no retry). The recovery hint repeats the frozen --app. Worker: Jack (cli-engineer) Co-Authored-By: Claude Sonnet 5.5 --- internal/api/errors.go | 8 ++++++- internal/cmd/agentic/agentic-usage.md | 4 ++-- internal/cmd/app/member.go | 32 ++++++++++++++++++++------- internal/cmd/app_member_cmd_test.go | 26 +++++++++++++++++----- 4 files changed, 53 insertions(+), 17 deletions(-) diff --git a/internal/api/errors.go b/internal/api/errors.go index ac83adde..b304937a 100644 --- a/internal/api/errors.go +++ b/internal/api/errors.go @@ -1241,6 +1241,12 @@ func IsUnknownGraphQLArgument(err error, argument, field string) bool { // OrphanAppError; the server's `Forbidden: …` arrives as plain text under // INTERNAL_SERVER_ERROR, so it is recognised by its message prefix. EVERY error // in the list must qualify; one unrecognized entry keeps the answer ambiguous. +// +// An error is a root refusal only when its path names no field below the +// mutation itself: a nested-field error (path ["ensureAppMember","user",…]) means +// the resolver RAN and returned a row, so the write may have committed whatever +// code the nested error carries. Callers must also treat a populated result next +// to an error as unknown (see memberWriteError) — this function sees only errors. // Like EndRefusedBeforeCommit, call it BEFORE MapError wraps. func AppMemberWriteRefused(err error) bool { list := graphQLErrors(err) @@ -1248,7 +1254,7 @@ func AppMemberWriteRefused(err error) bool { return false } for _, e := range list { - if e == nil { + if e == nil || len(e.Path) > 1 { return false } switch extensionCode(e) { diff --git a/internal/cmd/agentic/agentic-usage.md b/internal/cmd/agentic/agentic-usage.md index 5c820c0f..82486c50 100644 --- a/internal/cmd/agentic/agentic-usage.md +++ b/internal/cmd/agentic/agentic-usage.md @@ -1930,11 +1930,11 @@ Conventions: server's `Forbidden:` text as-is (no "user not found" — that would be an existence oracle). A definite refusal keeps the server text and exits by code: `InvalidRoleError` 2, `MaxMembersExceededError` and `OrphanAppError` 5, - not-found 4, auth 3, and the plain-text `Forbidden:` 1. ANY other failure of a write + not-found 4, auth 3, and the plain-text `Forbidden:` 1. ANY other failure of a write, or any result returned next to an error, — transport error, `data` plus `errors`, a masked internal error, an empty payload — exits 7 with "outcome unknown", prints no success DTO, and is never retried: `ensureAppMember` re-reads the row after commit, so the write may - have landed; check with `app member list`. `list --json` is + have landed; check with `app member list --app `. `list --json` is `{appId, appUrn, members: [{user: {id, handle, urn}, role, createdAt}]}` with `members: []` (never null) when empty. The server returns `[]` when the member list is unreadable by you, so an empty list means "no members visible diff --git a/internal/cmd/app/member.go b/internal/cmd/app/member.go index 95fa64d3..3589dbdb 100644 --- a/internal/cmd/app/member.go +++ b/internal/cmd/app/member.go @@ -122,14 +122,30 @@ func requireMemberUser(user string) error { // memberWriteError classifies a failed membership write. A definite refusal // keeps the server's wording and exit code; everything else is outcome UNKNOWN // (see api.AppMemberWriteRefused) — non-zero, no success DTO, never retried. -func memberWriteError(err error) error { - if api.AppMemberWriteRefused(err) { +// +// `partial` is true when the response carried a result next to the error: the +// resolver ran and returned data, so the write may have committed whatever the +// error says. The recovery command repeats the frozen target App. +func memberWriteError(err error, partial bool, appRef string) error { + if !partial && api.AppMemberWriteRefused(err) { return api.MapError(err) } return exitcode.Newf(exitcode.Unavailable, "outcome unknown: %s. The membership change may or may not have been applied and was not retried — "+ - "run `hadron app member list` to check (a list showing no row is not proof: rows you cannot see are withheld)", - api.MapError(err)) + "run %s to check (a list showing no row is not proof: rows you cannot see are withheld)", + api.MapError(err), memberListHint(appRef)) +} + +// memberListHint is the copy-pasteable check for the App a write was aimed at. +func memberListHint(appRef string) string { + return "`hadron app member list --app " + shellQuote(appRef) + "`" +} + +func shellQuote(s string) string { + if s != "" && strings.Trim(s, "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789:._/@-") == "" { + return s + } + return "'" + strings.ReplaceAll(s, "'", `'\''`) + "'" } func memberUserDTO(id string, handle, urn *string) appMemberUserDTO { @@ -232,11 +248,11 @@ write is not retried; check with ` + "`hadron app member list`" + `.`, } resp, err := gen.EnsureAppMember(cmd.Context(), client, appRef, user, role) if err != nil { - return memberWriteError(err) + return memberWriteError(err, resp != nil && resp.EnsureAppMember != nil, appRef) } if resp == nil || resp.EnsureAppMember == nil || resp.EnsureAppMember.User == nil { return exitcode.Newf(exitcode.Unavailable, - "outcome unknown: the server returned no member row. Run `hadron app member list` to check (a list showing no row is not proof: rows you cannot see are withheld)") + "outcome unknown: the server returned no member row. Run %s to check (a list showing no row is not proof: rows you cannot see are withheld)", memberListHint(appRef)) } m := resp.EnsureAppMember dto := appMemberAddDTO{ @@ -300,11 +316,11 @@ without --yes is refused before anything is sent.`, } resp, err := gen.RemoveAppMember(cmd.Context(), client, appRef, user) if err != nil { - return memberWriteError(err) + return memberWriteError(err, resp != nil && resp.RemoveAppMember, appRef) } if resp == nil || !resp.RemoveAppMember { return exitcode.Newf(exitcode.Unavailable, - "outcome unknown: the server did not confirm the removal. Run `hadron app member list` to check (a list showing no row is not proof: rows you cannot see are withheld)") + "outcome unknown: the server did not confirm the removal. Run %s to check (a list showing no row is not proof: rows you cannot see are withheld)", memberListHint(appRef)) } dto := appMemberRemoveDTO{App: appRef, UserRef: user, Status: "absent"} return output.Write(f.IOStreams, f.JSON, dto, func(w io.Writer) error { diff --git a/internal/cmd/app_member_cmd_test.go b/internal/cmd/app_member_cmd_test.go index 8956cda2..bd7d2045 100644 --- a/internal/cmd/app_member_cmd_test.go +++ b/internal/cmd/app_member_cmd_test.go @@ -374,12 +374,16 @@ func TestAppMemberDefiniteRefusalsKeepServerText(t *testing.T) { // points at `app member list`. func TestAppMemberOutcomeUnknown(t *testing.T) { partial := `{"data":{"ensureAppMember":null},"errors":[{"message":"boom","extensions":{"code":"INTERNAL_SERVER_ERROR"}}]}` + nested := `{"data":{"ensureAppMember":{"role":"member","createdAt":"2026-10-25T00:00:00Z","user":{"id":"u","handle":null,"urn":"hrn:user:ada"}}},` + + `"errors":[{"message":"Forbidden: nested field not readable","path":["ensureAppMember","user","handle"],"extensions":{"code":"FORBIDDEN"}}]}` cases := map[string]string{ - "post-write internal error": memberGQLError("Internal server error", "INTERNAL_SERVER_ERROR"), - "data and errors": partial, - "no code": `{"errors":[{"message":"mystery"}]}`, - "null payload": `{"data":{"ensureAppMember":null}}`, - "mixed with a refusal": `{"errors":[{"message":"x","extensions":{"code":"InvalidRoleError"}},{"message":"y","extensions":{"code":"INTERNAL_SERVER_ERROR"}}]}`, + "partial data with nested FORBIDDEN": nested, + "nested-path error alone": `{"data":{"ensureAppMember":null},"errors":[{"message":"Forbidden: x","path":["ensureAppMember","user"],"extensions":{"code":"FORBIDDEN"}}]}`, + "post-write internal error": memberGQLError("Internal server error", "INTERNAL_SERVER_ERROR"), + "data and errors": partial, + "no code": `{"errors":[{"message":"mystery"}]}`, + "null payload": `{"data":{"ensureAppMember":null}}`, + "mixed with a refusal": `{"errors":[{"message":"x","extensions":{"code":"InvalidRoleError"}},{"message":"y","extensions":{"code":"INTERNAL_SERVER_ERROR"}}]}`, } for name, resp := range cases { t.Run(name, func(t *testing.T) { @@ -394,7 +398,7 @@ func TestAppMemberOutcomeUnknown(t *testing.T) { if out.String() != "" { t.Errorf("no success DTO on stdout: %q", out.String()) } - if !strings.Contains(err.Error(), "outcome unknown") || !strings.Contains(err.Error(), "app member list") { + if !strings.Contains(err.Error(), "outcome unknown") || !strings.Contains(err.Error(), "app member list --app "+memberApp) { t.Errorf("must say outcome unknown and point at list: %v", err) } if calls("EnsureAppMember") != 1 { @@ -402,6 +406,16 @@ func TestAppMemberOutcomeUnknown(t *testing.T) { } }) } + t.Run("partial remove result", func(t *testing.T) { + srv, calls, _ := countingGraphQL(t, map[string]string{"RemoveAppMember": `{"data":{"removeAppMember":true},"errors":[{"message":"Forbidden: x","extensions":{"code":"FORBIDDEN"}}]}`}) + f, out := testFactory(t) + root := NewRootCmd(f) + root.SetArgs([]string{"app", "member", "remove", "--user", "ada", "--app", memberApp, "--yes", "--server", srv.URL}) + err := root.Execute() + if err == nil || out.String() != "" || !strings.Contains(err.Error(), "outcome unknown") || calls("RemoveAppMember") != 1 { + t.Fatalf("want outcome unknown: err=%v out=%q", err, out.String()) + } + }) t.Run("transport failure", func(t *testing.T) { srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { if r.URL.Path == "/capabilities" { From 4e143fb16a4673d97b61ed8618dde51be989ac5a Mon Sep 17 00:00:00 2001 From: Holger Selover-Stephan Date: Sun, 25 Oct 2026 14:23:40 +0000 Subject: [PATCH 3/4] =?UTF-8?q?app=20member:=20pin=20nil=20user,=20false?= =?UTF-8?q?=20remove=20answer,=20Forbidden=20prefix=20match=20(#956=20?= =?UTF-8?q?=C2=A710=20P3s)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Worker: Jack (cli-engineer) Co-Authored-By: Claude Sonnet 5.5 --- internal/cmd/app_member_cmd_test.go | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/internal/cmd/app_member_cmd_test.go b/internal/cmd/app_member_cmd_test.go index bd7d2045..a641024e 100644 --- a/internal/cmd/app_member_cmd_test.go +++ b/internal/cmd/app_member_cmd_test.go @@ -383,6 +383,8 @@ func TestAppMemberOutcomeUnknown(t *testing.T) { "data and errors": partial, "no code": `{"errors":[{"message":"mystery"}]}`, "null payload": `{"data":{"ensureAppMember":null}}`, + "null user in row": `{"data":{"ensureAppMember":{"role":"member","createdAt":"2026-10-25T00:00:00Z","user":null}}}`, + "forbidden mid-text": memberGQLError("Internal: wrapped Forbidden: detail", "INTERNAL_SERVER_ERROR"), "mixed with a refusal": `{"errors":[{"message":"x","extensions":{"code":"InvalidRoleError"}},{"message":"y","extensions":{"code":"INTERNAL_SERVER_ERROR"}}]}`, } for name, resp := range cases { @@ -416,6 +418,16 @@ func TestAppMemberOutcomeUnknown(t *testing.T) { t.Fatalf("want outcome unknown: err=%v out=%q", err, out.String()) } }) + t.Run("remove answered false", func(t *testing.T) { + srv, calls, _ := countingGraphQL(t, map[string]string{"RemoveAppMember": `{"data":{"removeAppMember":false}}`}) + f, out := testFactory(t) + root := NewRootCmd(f) + root.SetArgs([]string{"app", "member", "remove", "--user", "ada", "--app", memberApp, "--yes", "--json", "--server", srv.URL}) + err := root.Execute() + if err == nil || out.String() != "" || !strings.Contains(err.Error(), "outcome unknown") || calls("RemoveAppMember") != 1 { + t.Fatalf("a false answer must not report absent: err=%v out=%q", err, out.String()) + } + }) t.Run("transport failure", func(t *testing.T) { srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { if r.URL.Path == "/capabilities" { From db5d2c9edc71502da2d008b13362166cf70aa7a6 Mon Sep 17 00:00:00 2001 From: Holger Selover-Stephan Date: Sun, 25 Oct 2026 14:28:46 +0000 Subject: [PATCH 4/4] app member: a false remove result beside an error is partial data (#956 BLIND P2) removeAppMember is selected as *bool so a null answer stays distinguishable from an answered false; any non-null result next to an error is outcome unknown. Worker: Jack (cli-engineer) Co-Authored-By: Claude Sonnet 5.5 --- internal/api/gen/generated.go | 7 ++++--- internal/api/queries/app_members.graphql | 4 +++- internal/cmd/app/member.go | 4 ++-- internal/cmd/app_member_cmd_test.go | 10 ++++++++++ 4 files changed, 19 insertions(+), 6 deletions(-) diff --git a/internal/api/gen/generated.go b/internal/api/gen/generated.go index e80edb04..9bfff62e 100644 --- a/internal/api/gen/generated.go +++ b/internal/api/gen/generated.go @@ -37743,11 +37743,11 @@ type RemoveAppMemberResponse struct { // App. // // Accepts the entity's ID or URN. - RemoveAppMember bool `json:"removeAppMember"` + RemoveAppMember *bool `json:"removeAppMember"` } // GetRemoveAppMember returns RemoveAppMemberResponse.RemoveAppMember, and is useful for accessing the field via an interface. -func (v *RemoveAppMemberResponse) GetRemoveAppMember() bool { return v.RemoveAppMember } +func (v *RemoveAppMemberResponse) GetRemoveAppMember() *bool { return v.RemoveAppMember } // RemoveMemoryMemberRemoveMemoryMemberRemoveMemoryMemberPayload includes the requested fields of the GraphQL type RemoveMemoryMemberPayload. type RemoveMemoryMemberRemoveMemoryMemberRemoveMemoryMemberPayload struct { @@ -79957,7 +79957,8 @@ mutation RemoveAppMember ($appRef: String!, $userRef: String!) { ` // true whether or not a row existed (a deleteMany server-side), so the result -// says nothing about prior membership. +// says nothing about prior membership. A pointer so an absent answer (null, +// e.g. beside an error) stays distinguishable from an answered false. func RemoveAppMember( ctx_ context.Context, client_ graphql.Client, diff --git a/internal/api/queries/app_members.graphql b/internal/api/queries/app_members.graphql index f7b4ce4b..4892577e 100644 --- a/internal/api/queries/app_members.graphql +++ b/internal/api/queries/app_members.graphql @@ -19,8 +19,10 @@ mutation EnsureAppMember($appRef: String!, $userRef: String!, $role: String!) { } # true whether or not a row existed (a deleteMany server-side), so the result -# says nothing about prior membership. +# says nothing about prior membership. A pointer so an absent answer (null, +# e.g. beside an error) stays distinguishable from an answered false. mutation RemoveAppMember($appRef: String!, $userRef: String!) { + # @genqlient(pointer: true) removeAppMember(appRef: $appRef, userRef: $userRef) } diff --git a/internal/cmd/app/member.go b/internal/cmd/app/member.go index 3589dbdb..5e95acac 100644 --- a/internal/cmd/app/member.go +++ b/internal/cmd/app/member.go @@ -316,9 +316,9 @@ without --yes is refused before anything is sent.`, } resp, err := gen.RemoveAppMember(cmd.Context(), client, appRef, user) if err != nil { - return memberWriteError(err, resp != nil && resp.RemoveAppMember, appRef) + return memberWriteError(err, resp != nil && resp.RemoveAppMember != nil, appRef) } - if resp == nil || !resp.RemoveAppMember { + if resp == nil || resp.RemoveAppMember == nil || !*resp.RemoveAppMember { return exitcode.Newf(exitcode.Unavailable, "outcome unknown: the server did not confirm the removal. Run %s to check (a list showing no row is not proof: rows you cannot see are withheld)", memberListHint(appRef)) } diff --git a/internal/cmd/app_member_cmd_test.go b/internal/cmd/app_member_cmd_test.go index a641024e..f2ccfd7f 100644 --- a/internal/cmd/app_member_cmd_test.go +++ b/internal/cmd/app_member_cmd_test.go @@ -418,6 +418,16 @@ func TestAppMemberOutcomeUnknown(t *testing.T) { t.Fatalf("want outcome unknown: err=%v out=%q", err, out.String()) } }) + t.Run("remove false beside a refusal", func(t *testing.T) { + srv, calls, _ := countingGraphQL(t, map[string]string{"RemoveAppMember": `{"data":{"removeAppMember":false},"errors":[{"message":"Forbidden: x","extensions":{"code":"FORBIDDEN"}}]}`}) + f, out := testFactory(t) + root := NewRootCmd(f) + root.SetArgs([]string{"app", "member", "remove", "--user", "ada", "--app", memberApp, "--yes", "--server", srv.URL}) + err := root.Execute() + if err == nil || out.String() != "" || !strings.Contains(err.Error(), "outcome unknown") || calls("RemoveAppMember") != 1 { + t.Fatalf("a false result beside an error is partial data, not a definite refusal: err=%v out=%q", err, out.String()) + } + }) t.Run("remove answered false", func(t *testing.T) { srv, calls, _ := countingGraphQL(t, map[string]string{"RemoveAppMember": `{"data":{"removeAppMember":false}}`}) f, out := testFactory(t)