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

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 45 additions & 0 deletions docs/plans/app-member-956.md
Original file line number Diff line number Diff line change
@@ -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 <ref> --role <r>` | `ensureAppMember` | idempotent upsert; role required, no default |
| `app member remove --user <ref>` | `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.
46 changes: 46 additions & 0 deletions internal/api/errors.go
Original file line number Diff line number Diff line change
Expand Up @@ -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":
Expand Down Expand Up @@ -1222,3 +1229,42 @@ 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.
//
// 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)
if len(list) == 0 {
return false
}
for _, e := range list {
if e == nil || len(e.Path) > 1 {
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
}
3 changes: 3 additions & 0 deletions internal/api/errors_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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},
Expand Down
Loading
Loading