[IAmJSD]

Summary

Test-only change. It closes the coverage gap that the review of #405 (the PR for #226) found.

The #226 self-promotion test (TestMembershipCeiling_UpdateUser_UpdateUsersOnlyCannotSelfPromoteToAdmin) only covers adding the Administrators group. No committed test proved that the membership grant ceiling on a self-edit applies in general and isn't just a special case for admin:*.

On a self-edit, guardGroupChange runs guardSelfEscalation first. That check allows adding a group with an ordinary, non-carved-out permission the actor doesn't hold (here view:recommendations), because:

• the actor holds update:users,
• nothing carved out is being added,
• the account scope doesn't get wider.

The change is refused only by checkMembershipGrantCeilingWithPerms, which compares against the actor's prior permissions. Behavior on main is already correct, so no production code changes.

Change

internal/auth/membership_ceiling_test.go: adds TestMembershipCeiling_UpdateUser_SelfEditNonAdminPermissionAboveCeilingRefused next to the existing membership ceiling tests. It reuses the shared fixtures (stubActorPermissions, updateUsersOnly, newCeilingService). An actor holding only update:users edits itself to add a "Recommendation Viewers" group that carries view:recommendations. The test asserts:

• errors.Is(err, ErrPermissionCeiling),
• not ErrSelfEscalation, which pins that the ceiling is what refuses the change, not the self-escalation guard,
• the error message names view:recommendations,
• UpdateUserAdminFields is never called. No stub is registered for it either, so a write that gets through panics the mock.

Closes #419

Verification

All of this is unit/mock evidence (testify MockStore). The issue is about service-layer guard logic, not DB behavior, so no integration test was added. There is no concurrent-caller surface here because nothing is written.

The new test passes on unmodified main code as expected, since it pins existing behavior. To show it can fail, two guard mutations were applied locally and then reverted (git status was clean apart from the test file afterwards):

Mutant	Change	New test	Rest of internal/auth unit suite
M1	guardGroupChange self-edit branch returns nil instead of checkMembershipGrantCeilingWithPerms(ctx, heldBefore, added)	FAIL (panic: unexpected UpdateUserAdminFields call at service_user.go:360)	also caught by the existing admin self-promote test
M2	checkMembershipGrantCeilingWithPerms refuses non-carved-out permissions only when perm.Action == ActionAdmin (an admin-only special case)	FAIL (same panic)	survives: go test ./internal/auth/ -count=1 -skip '<new test>' gives ok

M2 is the regression the issue describes. Before this PR, no internal/auth unit test would catch it.

Commands run (Go 1.26.9, with AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null AWS_EC2_METADATA_DISABLED=true):

go build ./...                                   # ok
go vet ./...                                     # ok
go vet -tags integration ./...                   # ok
gofmt -l internal/auth/                          # clean
go test ./internal/auth/ -count=1 -run 'TestMembershipCeiling' -v   # 10/10 PASS incl. the new test
go test ./internal/auth/ -count=1                # ok
go test ./internal/mocks/ -count=1               # ok
golangci-lint (v2.10.1) run --allow-parallel-runners ./internal/auth/                          # 0 issues
golangci-lint (v2.10.1) run --allow-parallel-runners --build-tags integration ./internal/auth/ # 0 issues

gocyclo: not applicable (no non-test files changed).

Combined-tree verification. This branch was tested together with the other queued branches on a local, never-pushed tree: queue/platform @ 936996ad = origin/main a14dc8f2 plus these heads, merged in this order:

Branch	Tested head
fix/issue-337	ff3420c
fix/issue-169	7d75eb5
fix/issue-342	66b221f
fix/issue-310	5e11c21
fix/issue-326	5f0ea59
fix/issue-243	365e959
fix/issue-343	6c94036
fix/issue-312	d2848a8
fix/issue-316	acd60dd
fix/issue-339	75ea9b9
fix/issue-339-commitment-type	73bdb19
fix/issue-353	5d25fcd
fix/issue-313	3156bc4
fix/issue-357	53d9e58
fix/issue-368	0ba727d
fix/issue-419	61648c9
fix/issue-111	b04c372
fix/issue-686	0ad90b7
fix/issue-674	793095d
fix/issue-1055	4ec96f1
fix/issue-559	dfa9525
fix/issue-558	034eed4
fix/issue-874	1720539
fix/issue-666	1e120da
fix/issue-668	fb26345
fix/issue-568	ce04173
fix/issue-311	e54f250
fix/issue-177	0cbb88a
fix/issue-151	c21377c
fix/issue-166	41fb77b
fix/issue-27	2d67622
fix/issue-323	69288ae
fix/issue-59	36463e8
fix/issue-43	35e4069
fix/issue-158	6f76f8a
fix/issue-104	bb00491
fix/issue-55	971ef1b
fix/issue-361	89ac220
fix/issue-120	b458719
fix/issue-36	42bac6e
fix/issue-525	57c97dd
fix/issue-712	e5bcda0
fix/issue-72	6cc63ed

fix/issue-72 and fix/issue-312 conflict on one line in internal/api/handler_dashboard.go (both change the calculateCommitmentMetrics call). The tree used #72's side with #312's validated provider variable; whichever PR merges second takes that same resolution.

Linux, Go 1.26.9, AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null AWS_EC2_METADATA_DISABLED=true; Docker Postgres via testcontainers:

• Passed at 936996ad: go build ./..., go vet ./..., go vet -tags integration ./... and the full go test -race -short -count=1 ./....
• Static checks at 936996ad: golangci-lint v2.10.1 found 0 issues on every changed package, with and without --build-tags integration. tests/e2e is a separate module, so it was vetted under its own e2e tag. gocyclo -over 10 is silent on the changed non-test files.
• Integration tests, every changed package, one package at a time (-tags integration -count=1, 60-minute local timeout because the VM was shared):
  • At 936996ad, these pass: internal/server, internal/credentials, internal/scheduler, internal/database/postgres/migrations, internal/purchase and internal/api.
  • internal/config passes except TestPostgresStoreDB_ResolveStaleRIExchange_ValidatesCloudCheck, which fails the same way on unmodified main (fixed by fix(config): compare resolve checked_at at the database's microsecond precision #1252).
  • These were run on the first build of this tree (26795376, same branches at earlier heads): internal/analytics, internal/auth, internal/commitmentopts, internal/database and cmd/lambda/clear-rate-limit all pass. The later fixes don't touch those packages.
• What the combined run caught, each fixed on its own branch before this PR opened:
  • a compile break between fix/issue-104's test and fix/issue-326's httpToLambdaRequest signature change;
  • fix/issue-316 rejecting legacy-keyed plans;
  • fix/issue-120 versus main's global-filter test.
• Secrets: git-secrets, using the repo's scripts/setup-git-secrets.sh patterns in a fresh repo with an isolated HOME, found nothing in the changed files, and a planted fake key was flagged. scripts/test-git-secrets-allowlist.sh: 137 PASS, 0 FAIL.
• Nothing was applied to any cloud.

Out of scope

• No production code change: the guard already behaves correctly.
• The non-self-edit counterpart (an update:users-only actor adding a non-admin permission group to another user) goes through checkMembershipGrantCeiling and is likewise covered only for Administrators. This PR doesn't pin it because the issue asks only for the self-edit case. It may be worth a follow-up test.

Labels

Could a maintainer please mirror the issue's triage labels onto this PR: impact/internal, type/chore, triaged, priority/p3, severity/low, urgency/eventually, effort/xs.

🤖 Generated with Claude Code

[coderabbitai]

Warning

Review limit reached

• Run on-demand review

_{This review includes 1 billable file and costs up to $0.25.}

Or wait 58 minutes for your next included review.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

Learn how review limits work.

Review configuration:

Run configuration

• Source mode: Auto
• Repository: LeanerCloud/cloud-commitments-platform
• Branch: main
• File: .coderabbit.yaml
• Commit: 503eb2e
• Layers: Repository YAML
• Review profile: CHILL
• Plan: Advanced
• Run ID: 573304b8-7b58-4937-aef5-bc6bc80f5f54

Commits

Reviewing files that changed from the base of the PR and between 503eb2e and ccc5a9f.

Files selected for processing: 1

• internal/auth/membership_ceiling_test.go

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

_{Comment @coderabbitai help to get the list of available commands.}

[IAmJSD]

Independent review (IAmJSD's agent, fresh context, given only the diff, the issue and the PR text): VERDICT: APPROVE ccc5a9f407405fe0f7440281ea20b1e3b4d974ef

Blocking:

• none

Non-blocking:

• As the body says, the non-self-edit counterpart (an update:users-only actor adding a non-admin permission group to another user, through checkMembershipGrantCeiling) is still pinned only for Administrators. Since this is a one-test PR, the matching case would be cheap to add here. It is outside test(auth): pin that a self-edit adding a non-admin permission the actor lacks is refused #419's wording, though.

Attacked:

• Does the test reach the path it claims? Yes. With actorUserID == targetUserID, guardGroupChange runs guardSelfEscalation and then checkMembershipGrantCeilingWithPerms(heldBefore, added). The test asserts ErrPermissionCeiling and NotErrorIs(ErrSelfEscalation), so a refusal from the wrong guard would fail it. view:recommendations isn't carved out, so it goes through the grantCeilingAllows branch and not ErrPermissionNotGrantable.
• Mutation probes (applied, then restored):
  • M2: limit the ceiling refusal to perm.Action == ActionAdmin (group_ceiling.go:153). The new test fails (unexpected UpdateUserAdminFields call panics the mock). The rest of the auth unit suite (-skip the new test) still passes, so this is exactly the regression test(auth): pin that a self-edit adding a non-admin permission the actor lacks is refused #419 says was unpinned.
  • M1: the self-edit branch returns nil instead of calling the ceiling check. The new test fails.
• Ran go build ./..., go vet ./internal/auth/ with and without -tags integration, go test -race -short -count=1 ./internal/auth/ (ok), and golangci-lint with and without --build-tags integration (0 issues). This is a test-only change with no production code, so no integration run was needed.

Body-stale:

• none

_{Reviewer: rB15. Bar while the maintainers are away (set by IAmJSD as acting owner): one approving independent review on the head, two for money, auth and security paths.}

[IAmJSD]

Independent review (IAmJSD's agent, fresh context, given only the diff, the issue and the PR text): VERDICT: APPROVE ccc5a9f407405fe0f7440281ea20b1e3b4d974ef

Blocking:

• none

Non-blocking:

• My M2 run confirms the gap the body mentions under Out of scope: with the ceiling made admin-only, the rest of the internal/auth suite still passes, so a non-self update:users-only actor adding someone else to a non-admin group (checkMembershipGrantCeiling) is still pinned only for Administrators. That's worth the follow-up test the body suggests, since that path is the bigger exposure (it reaches other users).

Attacked:

• Checked the test against test(auth): pin that a self-edit adding a non-admin permission the actor lacks is refused #419's single acceptance criterion (a test with that exact name, next to the existing membership ceiling tests, pinning a non-admin self-edit refusal). Traced guardGroupChange -> guardSelfEscalation (passes: update:users held, view:recommendations not carved out, account scope unchanged) -> checkMembershipGrantCeilingWithPerms, confirming the refusal really comes from the ceiling. The NotErrorIs(ErrSelfEscalation) assertion pins that. With no UpdateUserAdminFields stub, a write that gets through panics the mock.
• Fail-before (test-only PR, so I used a mutation): changed group_ceiling.go so the ceiling refuses only perm.Action == ActionAdmin. The new test fails with a mock panic, while go test ./internal/auth/ -skip <new test> still passes. Restored afterwards.
• Ran: go build ./..., go vet with and without -tags integration on internal/auth, go test -race -short -count=1 ./internal/auth/ (ok), golangci-lint v2.10.1 with and without integration tags (0 issues). No production code changed, so I didn't run integration tests.

Body-stale:

• none. The combined-tree table lists pre-merge heads (the head is now merge ccc5a9f4), but it reads as a historical record and the test file is unchanged by the merge.

_{Reviewer: rB17. Bar while the maintainers are away (set by IAmJSD as acting owner): one approving independent review on the head, two for money, auth and security paths.}

[IAmJSD]

Local verification at ccc5a9f407405fe0f7440281ea20b1e3b4d974ef merged onto main (de56f95b), together with #1114, #1142, #1254, #1274, #1278, #1279, #1280, #1282, #1414 in that order (combined tree e1bf26a0, Linux, Go 1.26.10): go build ./..., go vet with and without -tags integration, the full go test -race -short -count=1 ./..., and go test -race -tags integration -count=1 on every changed package, against Docker Postgres. All pass. CI Success and pre-commit are green on this head, with 2 approving independent review(s) on it. Nothing was run against any cloud.

[IAmJSD]

Merged by IAmJSD's agent at IAmJSD's request (IAmJSD is acting owner while the maintainers are away), under the bar IAmJSD set: 2 approving independent review(s) on ccc5a9f4 (two required: money, auth or security path), green CI, and the combined-tree verification above. Squash-merged at exactly ccc5a9f407405fe0f7440281ea20b1e3b4d974ef.