feat: read and change app owners one at a time, against the store that has them - #98
feat: read and change app owners one at a time, against the store that has them#98leet-c1 wants to merge 5 commits into
Conversation
…t has them
c1i exposed one sixth of the v1 app-owner API: set-owners (PUT) and nothing
else. Reading owners meant hand-rolling "c1i api --path", which the guide
literally instructed -- a raw api call documented for a common workflow is a
missing command, not a documentation task. Adding one owner meant building
the full list from a read and PUTting it back, and set-owners replaces the
list with exactly the ids you pass, so anything added in between is silently
removed.
apps owners <app-id> GET .../owners, NDJSON, auto-paginated
apps add-owner <user-id> --app-id X POST .../owners/{user_id}
apps remove-owner <user-id> --app-id X DELETE .../owners/{user_id}
The positional id is the resource's own id and scoping ids are flags, per
CLAUDE.md; that deliberately differs from set-owners <app-id> --user-id,
which addresses the owner LIST under one app and is correct as it stands.
This is the store that is actually populated. apps get's appOwners reads the
ownership-v2 model, which is empty tenant-wide; these read v1.
Every factual claim in the new text was measured on a live tenant, not
inferred:
- the response envelope is {list: [flat user objects], nextPageToken}
- owner writes land in roughly 45-150s (a dozen writes; two past 120s, so
the "1-2 minutes" this started with was optimistic and is now 45-150s
everywhere, set-owners included)
- apps create auto-assigns its caller as an owner, at 46s, silently
- two add-owner calls issued simultaneously BOTH land
That last one replaced a claim that add/remove are "safe to run concurrently
with another add-owner/remove-owner/set-owners call". The add/add half is now
verified; the set-owners half was never true in the form stated and is gone.
Racing an add against set-owners is ordering-dependent -- in one trial the add
applied after the PUT and survived -- so the text states the mechanism a
caller can act on instead of a safety guarantee.
Also fixes a comment asserting this endpoint has no department field. It
returns one.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
General PR Review: feat: read and change app owners one at a time, against the store that has themBlocking Issues: 0 | Suggestions: 1 | Threads Resolved: 0 Review SummaryThe new commits are documentation and comment-only: CHANGELOG/README rewrapping, a rewritten Security IssuesNone found. Correctness IssuesNone found. Suggestions
Prompt for AI agents |
All five are the same shape this change set keeps hitting -- one fact stated in several places, updated in some of them. - cmd/agents.md kept #96's set-owners-only 96-129s while every other surface moved to 45-150s. Both numbers are real but measure different writes, so each is now scoped rather than flattened into one range. - The CHANGELOG shipped both windows eight lines apart in one release with nothing saying why they differ. The Added entry now names which operations its range covers and points at the narrower one below. - apps_add_owner_test.go's doc comment still said "no request body, since the endpoint takes none" -- the exact assumption that produced the 400 this branch fixed, contradicting the assertion directly beneath it. - README's "two issued at once both land" read as covering any two of the commands named, including add-owner and remove-owner on the same user, where "both land" has no meaning. The measurement was two concurrent add-owner calls; it says that now. - set-owners' --wait timeout error still sent users to "c1i api --method GET --path .../ownerids" -- the raw call this branch replaces, and the sibling of the success message already corrected. Both render verified against the real command. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
- CHANGELOG said the 96-129s band came from "five timed writes". Four
converged; the fifth was still pending at 108s, which the code comment
already said. The artifact announcing a fix for overstated claims should
not contain one.
- "Two add-owner calls issued at the same time both land" read as a
guarantee built from a single trial. A race that usually resolves
benignly passes one trial. Now stated as what it is -- an observation.
- cmd/agents.md wrapped mid-sentence, unlike its neighbouring bullets.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
| // parses the body as a protobuf message even though it needs nothing from it, | ||
| // so a nil body (which json.Marshal renders as `null`) is rejected live with | ||
| // 400 "failed to unmarshal body: proto: syntax error ... unexpected token | ||
| // null". Unit tests never see that, because the mutation path does no HTTP -- | ||
| // this pins the one value that keeps it from happening again. | ||
| func TestAppsAddOwnerSendsEmptyObjectNotNull(t *testing.T) { | ||
| got, err := json.Marshal(addOwnerEmptyBody()) | ||
| if err != nil { | ||
| t.Fatalf("marshal: %v", err) | ||
| } | ||
| if string(got) != "{}" { | ||
| t.Errorf("add-owner request body = %s, want {} (a nil body marshals to null and the API 400s)", got) | ||
| } | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: neither add-owner nor remove-owner has a test that drives the real HTTP path — this pins addOwnerEmptyBody() in isolation, and the only call-site coverage is the dry-run preview. CLAUDE.md asks for a test that exercises the wired end-to-end path, and the repo already has the harness (cmd/requests_create_test.go, cmd/policies_update_test.go use httptest). A wired test would additionally pin the method, the client.Path escaping of <user-id>, and that {} — not null — actually reaches the wire. Confidence: high (test gap, not a defect).
There was a problem hiding this comment.
Agreed, and this is the gap that actually mattered — the nil-body 400 lived in exactly this hole and no unit test could see it. You are right that pinning addOwnerEmptyBody() in isolation would still pass if RunE stopped passing body to c.Post.
Being built now on a follow-up branch stacked on this one: var newXClient = newClient seams for both mutations plus wired httptest cases pinning the method, the client.Path escaping of the ids, and that {} rather than null reaches the wire. Leaving this thread OPEN until that lands rather than resolving it on a promise.
| return fmt.Errorf("API error: %w", err) | ||
| } | ||
|
|
||
| return writeRawObject(cmd, data) |
There was a problem hiding this comment.
🟡 Suggestion: unlike its siblings, this mutation can succeed completely silently. apps delete prints Deleted app: id=%s and apps set-owners prints setOwnersSuccessFmt (which also names the async lag and the verify command), but writeRawObject returns without writing anything when the response body is empty (cmd/fields.go:531). Since this command's own help says the write takes 45-150s to become visible, a bare exit 0 gives the caller nothing to distinguish "accepted" from "no-op" — consider printing a confirmation line naming the app, the user, and c1i apps owners <app-id>. Same applies to cmd/apps_remove_owner.go:55. (medium confidence — depends on whether the endpoint returns a body, which I can't verify from the diff)
There was a problem hiding this comment.
Partly wrong on the premise, and the valid part is going to the follow-up.
The endpoint does return a body — {} — so writeRawObject prints that rather than nothing; verified live, both mutations emit {} on success. So it is not silent in the strict sense.
The substance stands though: {} is a poor confirmation next to siblings that print a line naming the app, the user and the async lag, and this command's own help says the write takes 45-150s to become visible. Queued for the follow-up branch alongside the HTTP seam, since it changes output and wants its own test. Leaving OPEN.
I fixed its first five suggestions and then stopped reading the thread. Ten
more had accumulated, several of them defects in those fixes.
The one that mattered: the success message and timeout error now send users to
"c1i apps owners" (GET .../owners) while --wait polls GET .../ownerids, and
the guide claimed a check there was "already satisfied if you used --wait".
That asserts convergence on one endpoint proves convergence on the other,
which nothing had tested. Tested it: a single add, both endpoints polled on
one clock, both empty through 97s and both populated at 108s. So they front
the same store and moved together -- but that is one trial at 10s
granularity, not a guarantee, so the guide now says what was measured and
tells the reader to check the read they care about.
Also:
- The row-builder comment I rewrote last round did not parse: an em-dash
aside swallowed the deleted_at clause and read as though deleted_at were
empty on every owner, the opposite of the sentence after it. Two plain
sentences instead.
- #96's changelog entry described an end state this PR then changed -- it
said the docs were pointed at ownerids, which is now true only of the
set-owners help.
- Two ragged wraps from my own edits, one of them in a Long that --help
renders verbatim.
Still open, deliberately deferred to the follow-up that carries #96's guard
rebuild: no test drives the mutations' real HTTP path (the gap the null-body
bug lived in), and both mutations confirm with a bare {} where their siblings
print a line naming the async lag.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
| (NDJSON, auto-paginated); `apps add-owner <user-id> --app-id <id>` and | ||
| `apps remove-owner <user-id> --app-id <id>` change one owner at a time via | ||
| POST/DELETE `.../owners/{user_id}`, so they do not read-modify-write the | ||
| list the way `set-owners` does. All three honor `--dry-run`; owner |
There was a problem hiding this comment.
🟡 Suggestion: "All three honor --dry-run" isn't accurate for apps owners — cmd/apps_owners.go has no dryRunActive() check, so c1i apps owners <id> --dry-run still issues the GET and prints rows. That's the right behavior for a read (cmd/api.go:136 deliberately skips dry-run for GET), but the CHANGELOG claim covers all three commands. Confidence: high. Suggest narrowing to "Both writes honor --dry-run".
There was a problem hiding this comment.
Confirmed and it is my error — cmd/apps_owners.go has no dryRunActive() check, so --dry-run is a no-op there, which as you say is correct for a read. The CHANGELOG claim covers all three commands and is therefore false for one of them.
Not yet fixed: this branch is currently checked out by another task, so I cannot amend it this second without disrupting that. It is queued as the next change to this PR, narrowing to the two writes. Leaving OPEN until the fix is pushed.
1 participant
Why
c1i exposed one sixth of the v1 app-owner API —
set-owners(PUT) and nothing else.c1i api --path=/api/v1/apps/$APP_ID/ownerids. The embedded guide literally instructed that, and CLAUDE.md is explicit: a rawapicall documented for a common workflow is a missing command, not a documentation task.set-ownersreplaces the list with exactly the ids you pass, so anything added between your read and your write is silently removed — including the owner the platform assigns on creation.What
The positional id is the resource's own id, scoping ids are flags, per CLAUDE.md's enforced convention. That deliberately differs from
set-owners <app-id> --user-id, which addresses the owner list under one app and is correct as it stands.This is the store that's actually populated.
apps get'sappOwnersreads the ownership-v2 model, which is empty tenant-wide (see #96); these read v1.Every claim measured, not inferred
{list: [flat user objects], nextPageToken}GET; the siblingowneridsuses a bespoke{"userIds": []}, so this was worth checking rather than assumingset-ownersincludedapps createauto-assigns its caller as an owneradd-ownercalls both landThat last one replaced a claim that add/remove are "safe to run concurrently with another add-owner/remove-owner/set-owners call". The add/add half is now verified. The
set-ownershalf was never true as stated and is gone — racing an add againstset-ownersis ordering-dependent (in one trial the add applied after the PUT and survived), so the text now states the mechanism a caller can act on rather than a safety guarantee.Review
Adversarial + live-validation on independent axes, then a fix round.
The live pass drove the built binary end to end: full round trip, NDJSON typing (
deleted_atisnull, sojq 'select(.deleted_at)'correctly skips live owners), real multi-page pagination at 15 owners (auto-paginate returns all 15,--page-token ""returns exactly one page,--limitcaps exactly across a page boundary), exit codes (400 → 2, 404 → 4), and--dry-runproven to send nothing past the async window rather than immediately.Defects caught before merge, none by unit tests:
add-ownerwould have 400'd on every call. It passed a nil body, which marshals tonull; the server parses the body as a protobuf message and rejects it (unexpected token null). Now sends{}, pinned by a test — the mutation paths do no HTTP in tests, so nothing else could catch it.set-owners' own success message still pointed at rawGET .../ownerids, the workflow this PR replaces.departmentfield. It returns one.🤖 Generated with Claude Code