feat(people): add people clients add/remove/invite/enable/disable - #689
Conversation
github-actions
Bot
added
commands
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
🟡 Changes recommended
The seat-limit response incorrectly activates the global rate-limit gate, and the required released SDK repin remains outstanding.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds project client management under people clients, backed by new SDK operations and integrated across CLI documentation, tests, MCP metadata, and API coverage.
Changes:
- Adds client listing, invitation, membership, and enablement commands.
- Maps client-specific validation, permission, and seat-limit errors.
- Updates SDK provenance, command surfaces, documentation, and tests.
[!TIP]
If you aren't ready for review, convert to a draft PR.
Click "Convert to draft" or rungh pr ready --undo.
Click "Ready for review" or rungh pr readyto reengage.
File summaries
| File | Description |
|---|---|
skills/basecamp/SKILL.md |
Documents client-management commands. |
nix/package.nix |
Refreshes the SDK vendor hash. |
internal/version/sdk-provenance.json |
Records the pinned SDK revision. |
internal/mcpserver/testdata/catalog_snapshot.txt |
Adds three MCP actions. |
internal/mcpserver/model/PROVENANCE.json |
Updates model provenance. |
internal/mcpserver/model/openapi.json |
Adds client API schemas and operations. |
internal/mcpserver/model/behavior-model.json |
Adds retry and redaction metadata. |
internal/mcpserver/catalog_test.go |
Updates the operation count. |
internal/commands/people.go |
Implements the client command group. |
internal/commands/people_clients_test.go |
Tests client command behavior. |
internal/commands/commands.go |
Registers the client action group. |
go.sum |
Updates SDK checksums. |
go.mod |
Pins the SDK feature revision. |
e2e/smoke/smoke_projects.bats |
Smoke-tests client listing. |
e2e/smoke/smoke_lifecycle.bats |
Classifies mutating commands as out of scope. |
e2e/people.bats |
Tests offline usage validation. |
API-COVERAGE.md |
Records three newly covered endpoints. |
.surface |
Updates the CLI surface snapshot. |
Review details
- Files reviewed: 17/18 changed files
- Comments generated: 4
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3e9d040858
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
🟡 Changes recommended
Several commands accept ignored positional arguments, the disable hint is not executable, and invitation error documentation conflicts with actual behavior.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (5)
Previously missed (3) — in code that hasn't changed since the last review.
internal/commands/people.go:948
- This no-argument command does not install an
Argsvalidator, so Cobra accepts and silently ignores positional values. With a configured default project,people clients list 123therefore lists the default project instead of rejecting the unsupported positional project, which can mislead callers. AddArgs: cobra.NoArgs.
This issue also appears in the following locations of the same file:
- line 1433
- line 1460
internal/commands/people.go:1289
- The command rejects malformed mailbox input locally as a
usageerror inparseClientInvitees; only rows accepted by that parser and subsequently rejected by Basecamp producevalidation(9). This help text currently promises the wrong exit contract for inputs such as"Annie Bryan", which the new e2e test explicitly expects to beusage.
skills/basecamp/SKILL.md:1267 - This repeats an incorrect exit-code contract: malformed addresses are rejected by
parseClientInviteesasusage, whilevalidation(9) is reserved for a server-side 422 after local parsing succeeds. Distinguish those cases so agents do not branch on the wrong verdict.
internal/commands/people.go:1433
- This no-argument mutating command silently accepts positional values because
Argsis unset. For example,people clients enable 123can ignore123and enable clients on the configured default project instead. Reject extra arguments withcobra.NoArgsbefore performing the mutation.
RunE: func(cmd *cobra.Command, args []string) error {
internal/commands/people.go:1460
- This no-argument destructive command silently accepts positional values because
Argsis unset. For example,people clients disable 123can ignore123and disable clients on the configured default project instead. Reject extra arguments withcobra.NoArgsbefore performing the mutation.
RunE: func(cmd *cobra.Command, args []string) error {
- Files reviewed: 19/20 changed files
- Comments generated: 1
- Review effort level: Balanced
There was a problem hiding this comment.
🟡 Changes recommended
Documentation and test gaps remain, and the SDK branch pin must be replaced by a released tag before merge.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (2)
Previously missed (2) — in code that hasn't changed since the last review.
skills/basecamp/SKILL.md:1267
- The skill currently tells agents that malformed addresses produce
validation, while the command deliberately rejects malformed mailbox syntax locally asusage; only a server-side 422 maps tovalidation. Documenting that distinction prevents agents from relying on the wrong exit code and multi-row hint.
skills/basecamp/SKILL.md:1269 - This parenthetical describes the
addomission reason for both verbs. Forremove, the implementation correctly reports an ID as unchanged when it is not on the project, so the skill should preserve that distinction.
- Files reviewed: 19/20 changed files
- Comments generated: 2
- Review effort level: Balanced
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c7210528d5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
🟡 Changes recommended
The required SDK release is still open and blocked, so the pseudo-version must be replaced with the final tagged release before merge.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 19/20 changed files
- Comments generated: 0 new
- Review effort level: Balanced
The client-admission operations (UpdateProjectClientAccess, EnableProjectClients, DisableProjectClients) live on basecamp-sdk's feat/project-client-users branch (basecamp/basecamp-sdk#847) ahead of a release, so go.mod pins its head as a pseudo-version, the way the bubble-up pin did. Re-pin to the tagged go/vX.Y.Z release once it is cut, and re-sync the vendored MCP model and the Nix vendorHash from it. The vendored MCP model is synced from that head: three People operations join the catalog (served count 256 -> 259) and the seat-limit 429 on UpdateProjectClientAccess is declared non-retryable (retry_on [503]).
The client-side counterpart of people add/remove, over the endpoints bc3#13098 shipped: PUT /projects/:id/people/client_users.json and POST/DELETE /projects/:id/client_enablement.json. A separate group rather than a --client flag: bc3 keeps the two kinds of access apart on the wire (the team endpoint drops a client's id, the client endpoint rejects a team member's), so the CLI mirrors the split. The ids the server silently drops come back as a diagnostic notice. invite takes a bare address or "Name <email>" (net/mail), or "-" for one invitee per line on stdin; --company applies to every invitee, --title to exactly one. Refusals map to verdicts: a 403 reads the project back to name "clients not enabled" against a permission problem; the all-or-nothing 422 exits validation naming each rejected row from the SDK's field errors; the seat-limit 429 exits limit_exceeded, not a retryable rate limit. disable's 403 reads the roster back so the hint is the exact remove command. people list now carries each person's client flag.
…3 honestly The resilience hooks persisted a 60-second block for every headerless 429, so the client seat-limit verdict would have refused unrelated commands for a minute. OnOperationStart now names the operation in the context and OnRequestEnd consults the SDK's declared retry set: an operation that does not retry on 429 (UpdateProjectClientAccess) is one the server answers 429 as a verdict, and it sets no block. A 429 that names a Retry-After is honored as before. disable's 403 names the remaining clients only when the roster shows some; otherwise it is a plain denial, since the same 403 answers a caller without permission. The seat-limit message no longer states a count: existing addresses take no seat and repeats count once, and only the server knows which rows those are.
…ect-only verbs The SDK branch head decodes the row-error selectors independently and reports client access as project_access; the pin, provenance, vendored MCP model, and Nix vendorHash follow it. list, enable, and disable take no positional, so a stray one is refused instead of discarded: with a default project configured, "disable 123" would otherwise have acted on the configured project. The remove hint disable offers separates ids with spaces so it pastes as a command.
A token that is not an address is refused before any request as a usage error; the help and skill text had promised the server's validation exit for it. The local check now names every malformed token in one error instead of stopping at the first, and the docs distinguish the local refusal (usage, 2) from the server's all-or-nothing rejection (validation, 9) and seat shortfall (limit_exceeded, 10). people list's client flag is pinned by a test.
b2171c1 to
09de5e5
Compare
There was a problem hiding this comment.
🟢 Approval recommended
The implementation matches the SDK contract and includes comprehensive unit, integration, surface, and documentation updates.
Review details
- Files reviewed: 19/20 changed files
- Comments generated: 0 new
- Review effort level: Balanced
What
Surfaces the client-admission API basecamp/bc3#13098 added — the client-side
counterpart of
people add/people remove— as apeople clientsgroup:add/removegrant and revoke existing client users throughPUT /projects/:id/people/client_users.json, resolving ids, emails, or namesthe way
people adddoes. The endpoint silently drops an ineligible id (ateam member's, or someone already on the project), so the ids it did not
echo back in
granted/revokedare reported as a diagnostic notice ratherthan lost.
invitecreates new clients by email through the same endpoint'screatelist. An invitee is a bare address or"Name <email>"(RFC 5322mailbox form via
net/mail, so a quoted display name works);-alone readsone invitee per line from stdin.
--companyapplies to every invitee;--titlenames one person's role and so takes exactly one invitee.enable/disabledrivePOST/DELETE /projects/:id/client_enablement.json.listis the project roster narrowed toclient: true;people listnow also carries each person's
clientflag.A separate group rather than a
--clientflag onpeople add: bc3 keeps thetwo kinds of access apart on the wire (the team endpoint drops a client's id,
the client endpoint rejects a team member's — neither cross-grades), and the
CLI mirrors that split instead of hiding it behind a mode flag. The verbs
honor the configured default project (
.basecamp/config.json) as SKILL.mdpromises;
people add/removestill read only the flag, unchanged here.Error mapping
The endpoint's refusals become verdicts, structured in
--json/--agentoutput:
forbidden(4). The project is read back once: withclients_enabled: falsethe message says so and the hint isbasecamp people clients enable --in <project>; otherwise it is a permission message.{errors:[{email_address, messages}]}validation(9), each rejected row named in the hint (the SDK branch folds the row-keyed body intoFieldErrorsby address); nobody was invited.limit_exceeded(10),retryable: false. This 429 carries noRetry-Afterand waiting cannot change it, so it is not reported as throttling; a 429 that does name aRetry-Afterstill converts as a rate limit. The SDK branch declares the operationretry_on: [503], so it surfaces on the first attempt.disablewhile clients remainforbidden, with the roster read back so the hint is the exactremovecommand.Dependency — pinned to the SDK release
Depends on basecamp/basecamp-sdk#847, which shipped in
basecamp-sdk v0.17.0.
go.modpinsgithub.com/basecamp/basecamp-sdk/go v0.17.0(taggo/v0.17.0,6e73a06f4262) viamake bump-sdk REF=v0.17.0, the mechanism #677 used; thevendored MCP model is re-synced from that tag (
scripts/sync-mcp-model.sh,PROVENANCE.jsonrefgo/v0.17.0) and the NixvendorHashrecomputed andbuild-verified (
make update-nix-hash). Noreplacedirective.The branch is rebased onto
main(#681, #688, #686). The only conflicts werethe pin-generated files —
go.mod,go.sum,PROVENANCE.json,sdk-provenance.json,nix/package.nix— resolved by takingmain's side andre-running the flip;
.surfaceregenerated tomain+ thepeople clientsadditions (no removals against
main).Tests
people_clients_test.go: request shapes for grant/revoke/invite (resolvedids, the
createrows,--company/--titleplacement, name omitted whenblank), stdin invitees, the unaffected-id notice, and each error mapping
(403 both ways, per-row 422, bare 429 vs
Retry-After429,disablewhileclients remain).
e2e/people.bats: offline usage errors (missing project, non-addressinvitee,
-with an empty pipe,-mixed with other invitees,--titlewith several invitees).
.surface, SKILL.md, API-COVERAGE, and the smoke-coverage entriesupdated. Every
bin/citarget run locally: fmt, vet, lint, lint-actions,e2e (458 bats), naming, surface, skill drift, bare groups, lint lockstep,
smoke coverage, SDK provenance, and go mod tidy green; the MCP catalog
(provenance, snapshot, served count 256 → 259) and the Nix flake
(
vendorHashrecomputed and build-verified) too. The Go unit run is greenexcept
TestBareBasecampNeverReportsASetupError,TestExplicitSetupStillRefuses,TestDeleteConfirmableFollowsTheAudienceNotTheDeviceand theTestIsInteractive*cases inappctx/cli, which fail identically onpristine
mainin this non-TTY shell (as feat(bubble-up): add bubble-up add/remove commands #677 noted).I'm babysitting the review loop on this PR through to convergence.
Release note: Features (
enhancement).