feat(s2s): allow S2S consumers to createSite via adminGrants registry field

[ravverma]

Summary

• Adds ADMIN_GRANT_CREATE_SITE = 'CREATE_SITE' to capability-constants.js
• Adds hasAdminGrant(operationKey) to AccessControlUtil: mirrors hasS2SCapability with fresh DB fetch (TOCTOU prevention), returns missing-grant reason when grant absent/false
• Updates createSite gate: admin pass-through or S2S hasAdminGrant('CREATE_SITE') fallback with structured ACL log on denial
• Updates consumers update() to accept, validate, and persist adminGrants via PATCH; Slack diff included
• Adds adminGrants to ConsumerDto
• Adds ### 2b. Grant / Revoke Admin Operation Access section to S2S_ADMIN_GUIDE.md

Context

Part of the adminGrants feature for S2S admin-gated write access. Design: platform/decisions/s2s-admin-grants-write-endpoints.md.

Dependency order (merge in sequence):

1. adobe/spacecat-shared — adminGrants field on Consumer entity
2. adobe/mysticat-data-service — admin_grants jsonb column migration
3. ✅ This PR — application layer

Test plan

• test/support/access-control-util.test.js — 9-case hasAdminGrant block (not-s2s, not-found, revoked, not-active, missing-grant × 3, granted, identity lookup)
• test/controllers/sites.test.js — S2S with grant → 201, missing-grant → 403, not-s2s → 403
• test/controllers/consumers.test.js — PATCH sets grants, clears with null, rejects invalid key, rejects non-boolean

🤖 Generated with Claude Code

[github-actions]

This PR will trigger a minor release when merged.

[MysticatBot-Dev]

Hey @ravverma,

Strengths

• src/support/access-control-util.js:180-205 - The hasAdminGrant method mirrors the established hasS2SCapability pattern with a fresh DB fetch to prevent TOCTOU. The result-object return shape ({ allowed, reason, clientId, consumerId }) gives controllers everything needed for structured audit logging without re-reading context. Well-considered API.
• src/controllers/sites.js:371-379 - The admin-first, grant-fallback structure reads naturally and avoids the DB fetch entirely for admin users. The structured log line on denial includes all three relevant identifiers at INFO level, which is the correct level for expected access-control rejections. Generic error message to callers avoids revealing the grant mechanism.
• src/routes/capability-constants.js:25-28 - Re-exporting the grant key from spacecat-shared-data-access keeps the canonical definition in one place and prevents string drift across repos.
• src/support/access-control-util.js:204 - Strict === true comparison on the grant value prevents truthy-value bypass (string "true", number 1, or object would all fail). Correct.
• test/support/access-control-util.test.js - Excellent 9-case coverage of every denial reason plus identity verification. Each test is behavior-named and asserts on the result object shape.
• Documentation proactively updated in docs/s2s/S2S_ADMIN_GUIDE.md alongside the feature code.

Issues

Important (Should Fix)

1. Verify authorization gate on PATCH /consumers/{consumerId} covers adminGrants mutation - src/controllers/consumers.js:417-424

The update() handler accepts adminGrants from any caller that can reach the PATCH endpoint. The PR description says "This operation requires the same S2S admin credentials as capability grants," and the code is added alongside the existing capabilities handling. If the PATCH route is already gated to admin-only callers, this is fine. But if non-admin callers can PATCH consumers (e.g., a consumer updating its own name), then any authenticated caller could grant themselves CREATE_SITE.

Unable to verify cross-file consistency (route-level auth is not in this diff). Please confirm the route-level guard covers this case, or add an explicit admin check before accepting adminGrants in the update payload.

2. hasAdminGrant duplicates the full consumer-freshness pipeline - src/support/access-control-util.js:180-209

The fresh-fetch, revoked-check, and status-check sequence is shared between hasAdminGrant and the existing hasS2SCapability. Every future status that denies access (e.g., a PENDING_REVIEW state, or an IP-restriction check) must be added to both methods independently, and a divergence between them would be a security gap. The extensible map design suggests more operation keys will follow.

Consider extracting a private _fetchAndValidateConsumer() that returns a validated consumer or a denial result. Both methods call it, then perform their domain-specific check. This is not blocking for this PR (the pattern existed before), but worth addressing before a third operation key lands.

3. Missing test: admin users bypass grant check entirely - test/controllers/sites.test.js

The beforeEach sets is_admin: false, but no test proves that is_admin: true skips the hasAdminGrant call. The production code has an if (!isAdmin) guard. If that guard were accidentally removed, the admin path would start requiring a grant. Add a test where is_admin: true, no s2sConsumer is set, and assert 201 success - proving the grant lookup is never reached.

4. Missing tests: revoked and not-active denial paths in the controller - test/controllers/sites.test.js

The hasAdminGrant unit tests cover these paths, but the integration between sites.js:createSite and the access-control util is only tested for missing-grant and not-s2s. Add tests that set up a revoked and a suspended consumer respectively, assert 403 status, and verify the log includes reason=revoked / reason=not-active.

Minor (Nice to Have)

5. Slack notification includes raw grant object without size bound - src/controllers/consumers.js:423

JSON.stringify(newGrants) is safe today because validateAdminGrants constrains keys and values. If validation is ever relaxed upstream to allow additional keys, the Slack message could grow. Consider truncating the stringified output or logging just the changed keys.

Recommendations

• Add a test in consumers.test.js that verifies the Slack change message format includes the adminGrants transition (assert the changes array or Slack post body contains the expected diff string). Current tests verify setAdminGrants was called but do not verify the audit trail.
• Consider a registry-driven approach for future admin-gated operations. The operationKey parameter and Consumer.ADMIN_GRANTS map hint at extensibility. A mapping from operation key to controller gate (similar to required-capabilities.js for Layer 1) would let new operations be wired declaratively.

Assessment

Ready to merge? With fixes
Reasoning: The core implementation is well-structured, follows established patterns, and has thorough test coverage on the access-control logic. The primary concern is confirming the route-level auth gate on PATCH consumers (item 1) - if that is already admin-only, the security model is sound. The test gaps (items 3-4) are quick additions that guard against real regression risk on a security-sensitive feature. No bugs or security vulnerabilities identified in the implementation itself.

---

_{Skill: pr-review | Model: us.anthropic.claude-opus-4-6-v1[1m] | Duration: 1m 19s | Cost: $3.10 | Commit: 99a2b093c0a5a80825138d71e3873be327fefd70
If this code review was useful, please react with 👍. Otherwise, react with 👎.}

[ravverma]

Action Plan - PR #2523

4 implemented, 1 pushed back.

• F01: Auth gate confirmed (no change) — PATCH /consumers is admin-only via hasS2SAdminAccess()
• F02: _fetchAndValidateConsumer() extracted — shared pipeline for both hasS2SCapability and hasAdminGrant
• F03/F04: 3 controller tests added — admin bypass, revoked, not-active
• F05 pushback: validateAdminGrants() already bounds the object structurally

Build fix: reverted Consumer.ADMIN_GRANTS import (unreleased in spacecat-shared); using string literal with TODO for follow-up after #1644 ships.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

[MysticatBot-Dev]

Hey @ravverma,

Strengths

• src/support/access-control-util.js:120-150 - The _fetchAndValidateConsumer() extraction cleanly addresses the prior review's F02 concern. The discriminated union return shape ({ denied: true, result } | { denied: false, fresh, clientId }) is self-documenting and makes the callers tight. Any future consumer status check (e.g., PENDING_REVIEW) gets added once and both hasS2SCapability and hasAdminGrant inherit it automatically - the correct architectural primitive for a growing authorization surface.
• src/controllers/sites.js:374-379 - Generic error message ("Only admins can create new sites") to callers regardless of denial reason, with structured denial details logged server-side only. No information leakage about the grant mechanism or consumer state.
• src/support/access-control-util.js - fresh.getAdminGrants()?.[operationKey] !== true uses strict boolean equality, rejecting truthy-value bypass attempts (string "true", number 1, object). Correct defense.
• src/routes/capability-constants.js:25-30 - Pragmatic resolution of the build break. String literal with a TODO referencing the specific upstream PR is the right trade-off when the shared-package export hasn't shipped yet.
• Previously flagged issues now addressed: F01 (auth gate confirmed admin-only), F02 (shared pipeline extracted), F03/F04 (controller tests for admin bypass, revoked, not-active added with assertions on status, body, and log output).
• test/support/access-control-util.test.js - The 9-case hasAdminGrant suite covers every branch including the subtle false vs undefined vs empty-object distinction on the grant value. The identity lookup test verifies the correct arguments flow to the DB query.
• The two-layer access control model is clean: Layer 1 (route-level capabilities) gates which endpoints a consumer can reach; Layer 2 (controller-level admin grants) gates which high-privilege operations are allowed. These layers can evolve independently.

Issues

Important (Should Fix)

1. Missing OpenAPI schema update for new adminGrants field - docs/openapi/schemas.yaml

The CLAUDE.md states: "Specification Sync: Keep OpenAPI specs and implementation in sync" and under "Modifying Data Models": "Update OpenAPI schemas in docs/openapi/schemas.yaml".

The PR adds adminGrants to the Consumer DTO (src/dto/consumer.js) and to the PATCH /consumers/{consumerId} request body, but no file under docs/openapi/ appears in the changed file list. The Consumer response schema and the PATCH request body schema need updating to reflect the new field.

Fix: Add adminGrants to the Consumer schema in docs/openapi/schemas.yaml (both response shape and PATCH request body), then run npm run docs:lint and npm run docs:build.

2. Missing integration tests for modified endpoint - test/it/

The CLAUDE.md states: "New or modified endpoints must include integration tests in test/it/"

The PATCH /consumers/{consumerId} endpoint now accepts and persists adminGrants. No files under test/it/ are in the changed file list. The integration test suite should verify the round-trip (PATCH with adminGrants, then GET to confirm persistence) against the real database.

Fix: Add integration test cases in the appropriate test/it/ wiring file covering: set adminGrants, clear with null, reject invalid key.

Minor (Nice to Have)

3. Orphaned JSDoc above _fetchAndValidateConsumer; hasS2SCapability lost its docs - src/support/access-control-util.js:~118-122

The refactoring moved the method body of hasS2SCapability out from under its original JSDoc block. The old JSDoc (describing the public { allowed, reason, consumerId, clientId } return contract) now sits as dead text above _fetchAndValidateConsumer's own JSDoc - two consecutive doc blocks, the first orphaned. Meanwhile hasS2SCapability (the public method callers rely on) has no documentation.

Fix: Move the original hasS2SCapability JSDoc down to sit directly above async hasS2SCapability(capability). The private method already has its own accurate docs.

Recommendations

• Once the adminGrants pattern proves out for a second or third operation key, consider a registry-driven approach (similar to required-capabilities.js for Layer 1) to reduce per-operation wiring. The current shape is fine for one key.
• Consider adding an INFO-level log on the success path (reason=granted) in createSite. If a compromised S2S credential is used to mass-create sites, the current logging surfaces only denials. A success log provides real-time observability without code complexity.
• Consider a test that verifies the Slack changes array string for adminGrants transitions (assert the message includes the old/new grant JSON) to validate audit trail correctness.

Assessment

Ready to merge? With fixes
Reasoning: The core implementation is well-structured, all prior findings are addressed, the security model is sound, and test coverage is thorough. The two remaining gaps (OpenAPI spec sync and integration tests) are explicit Must-Follow rules in the project's CLAUDE.md and should be addressed before merge.

Next Steps

1. Add adminGrants to the Consumer schema in docs/openapi/schemas.yaml and run npm run docs:lint.
2. Add integration tests in test/it/ covering the adminGrants round-trip.
3. (Optional) Fix the orphaned JSDoc on hasS2SCapability.

---

_{Skill: pr-review | Model: us.anthropic.claude-opus-4-6-v1[1m] | Duration: 0m 49s | Cost: $3.56 | Commit: 08ab3507ddc2147c8628c660b3bf53c87ca8b190
If this code review was useful, please react with 👍. Otherwise, react with 👎.}

[MysticatBot-Dev]

Hey @ravverma,

Strengths

• src/support/access-control-util.js:168-203 - The _fetchAndValidateConsumer() extraction is clean. The discriminated union ({ denied: true, result } | { denied: false, fresh, clientId }) makes callers tight and eliminates the prior concern about diverging validation pipelines. Any future consumer-status check gets added once and both methods inherit it automatically.
• src/controllers/sites.js:371-379 - The admin-first guard avoids the DB fetch entirely for admin users. Structured log at INFO with all three identifiers is correct for expected access-control denials. Generic error message to callers prevents information leakage about the grant mechanism.
• src/support/access-control-util.js:197 - fresh.getAdminGrants?.()?.[operationKey] !== true uses strict boolean equality, rejecting truthy-value bypass (string "true", number 1, object). The double optional chain handles both the backwards-compat case and the null-map case in one expression.
• src/support/access-control-util.js:168-202 - Fresh DB fetch prevents TOCTOU attacks where consumer status changes between request auth and grant check. Revoked/not-active checks happen on the freshly-fetched entity, not the stale context object.
• src/controllers/consumers.js:65-75 - validateAdminGrants mirrors the established validateCapabilities pattern exactly. Null-coalescing check with == null correctly covers both null (clear) and undefined (absent from payload).
• The two-layer authorization model (Layer 1: route-level capabilities; Layer 2: controller-level admin grants) is a sound separation of concerns that can evolve independently.
• Previously flagged issues now addressed: shared pipeline extracted (_fetchAndValidateConsumer), controller tests for admin bypass/revoked/not-active added with status and log assertions.
• test/support/access-control-util.test.js - Excellent 9-case coverage of every denial reason plus the subtle false vs undefined vs empty-object distinction. The existing hasS2SCapability suite covers the same paths through the refactored shared pipeline, providing regression protection.

Issues

Important (Should Fix)

1. setAdminGrants?.() silently no-ops if the method is missing - src/controllers/consumers.js:424

The optional chaining on consumer.setAdminGrants?.(newGrants ?? null) means that if the Consumer entity instance lacks setAdminGrants (old shared-data-access version), the PATCH returns 200 OK and reports success in the Slack change log, but the grant is never persisted. The caller believes the operation succeeded.

The security profile is acceptable (fails closed - the grant check also uses optional chaining on getAdminGrants, so no grant is found and access is denied). However, an admin granting access sees a success response without the mutation taking effect. No test exercises the backwards-compat path where these methods are absent.

Fix: Remove optional chaining on the setter, or guard the block explicitly:

if (typeof consumer.setAdminGrants !== 'function') {
  throw new ErrorWithStatusCode('adminGrants not supported on this entity version', 501);
}

2. Missing OpenAPI schema update for adminGrants field - docs/openapi/schemas.yaml

Re-raising from prior review (unresolved, no response). The repo CLAUDE.md states: "Specification Sync: Keep OpenAPI specs and implementation in sync" and under "Modifying Data Models": "Update OpenAPI schemas in docs/openapi/schemas.yaml".

The Consumer DTO now includes adminGrants and the PATCH request body accepts it, but no OpenAPI update is present. Downstream consumers (SDK generators, API docs portals) derive their contracts from the spec. Without it, the field is undiscoverable.

Fix: Add adminGrants to the Consumer schema in docs/openapi/schemas.yaml (both response and PATCH request body), then run npm run docs:lint.

3. Missing integration tests for modified endpoints - test/it/

Re-raising from prior review (unresolved, no response). The repo CLAUDE.md states: "New or modified endpoints must include integration tests in test/it/".

The PATCH /consumers/{consumerId} now accepts a new field that mutates persisted state. The unit tests verify controller logic via mocks, but the integration round-trip (PATCH with adminGrants, GET to confirm persistence, PATCH with null to clear) is untested against the real database. This is the class of bug that mocked save() cannot catch (schema mismatch, column type coercion, null handling in Postgres JSONB).

Fix: Add integration test cases in the appropriate test/it/ wiring file covering: set adminGrants, clear with null, and POST /sites with S2S grant.

Minor (Nice to Have)

4. Orphaned JSDoc above _fetchAndValidateConsumer - src/support/access-control-util.js:120-122

The original hasS2SCapability JSDoc (describing the public return contract) now sits as orphaned text above _fetchAndValidateConsumer's own JSDoc, producing two consecutive doc blocks. Meanwhile hasS2SCapability (the public method callers rely on) has no documentation.

Fix: Move the original @returns JSDoc down to sit directly above async hasS2SCapability(capability).

Recommendations

• Consider logging at INFO level on the success path in createSite (reason=granted). Currently only denials are logged. A one-line success log provides real-time correlation for forensic reconstruction if a compromised S2S credential mass-creates sites.
• Once a second or third operation key lands, consider a registry-driven approach (similar to required-capabilities.js for Layer 1) to reduce per-operation wiring. The current shape is fine for one key.

Assessment

Ready to merge? With fixes
Reasoning: The core implementation is well-structured, the security model is sound (proper layering, strict equality, TOCTOU prevention, fails closed), and test coverage on the access-control logic is thorough. The primary new concern (item 1) is the silent no-op on write when the shared package method is absent - this creates a confusing success response without persistence. Items 2-3 are explicit Must-Follow rules in the project CLAUDE.md that remain unaddressed across two review rounds.

Next Steps

1. Address the silent-write issue (item 1) - either guard with an explicit error or remove optional chaining on the setter.
2. Add adminGrants to the OpenAPI schema and run npm run docs:lint.
3. Add integration tests in test/it/ covering the adminGrants round-trip.
4. (Optional) Fix the orphaned JSDoc.

---

_{Skill: pr-review | Model: us.anthropic.claude-opus-4-6-v1[1m] | Duration: 1m 29s | Cost: $5.19 | Commit: 689f5cbdd5ccdd16ed90efbdf06cb93cdc6864ae
If this code review was useful, please react with 👍. Otherwise, react with 👎.}

[MysticatBot-Dev]

Hey @ravverma,

Strengths

• Previously flagged issues now addressed: all four prior findings resolved cleanly in a single commit - the setAdminGrants silent no-op is now a 501 with actionable message (src/controllers/consumers.js:421-425), OpenAPI schemas added (docs/openapi/schemas.yaml), integration tests cover the round-trip (test/it/shared/tests/consumers.js), and the orphaned JSDoc sits above the correct method.
• src/support/access-control-util.js:103-170 - The _fetchAndValidateConsumer() extraction is the correct architectural primitive. The discriminated union return shape ({ denied: true, result } | { denied: false, fresh, clientId }) makes callers tight and eliminates the duplication concern. Any future consumer-status check gets added once and both hasS2SCapability and hasAdminGrant inherit it automatically.
• src/support/access-control-util.js:197 - fresh.getAdminGrants?.()?.[operationKey] !== true uses strict boolean equality, rejecting truthy-value bypass attempts (string "true", number 1, object). The double optional chain handles both backwards-compat and null-map cases in one expression.
• src/controllers/sites.js:371-379 - The admin-first guard avoids a DB round-trip for admin users. Structured INFO-level denial log with all three identifiers (reason, clientId, consumerId) gives operations everything needed for SOC correlation. Generic error message to callers prevents information leakage about the grant mechanism.
• src/controllers/consumers.js:419-420 - Validation runs before the typeof guard, so a caller sending { adminGrants: { BOGUS: "string" } } gets a clean 400 before the system checks entity version support. Correct ordering.
• The two-layer access control model (Layer 1: route-level capabilities; Layer 2: controller-level admin grants) is sound separation that can evolve independently. The API surface for adding new Layer 2 operations is now a single method call with a string key.
• test/support/access-control-util.test.js - 9-case hasAdminGrant suite covers every denial path including the subtle false vs undefined vs empty-object distinction. Each test asserts on the full result object shape.
• test/controllers/sites.test.js:575-583 - Admin-bypass test proves both success (201) AND that the DB lookup is never reached. The six controller tests cover every denial reason plus the happy path.
• test/it/shared/tests/consumers.js:655-685 - Proper round-trip integration tests (PATCH -> verify response -> GET -> verify persistence -> PATCH null -> verify cleared). This catches schema/column type mismatches that mocked tests cannot.
• CLAUDE.md rules followed: spec sync, integration tests, DTO usage, access control first.

Issues

Minor (Nice to Have)

1. CLAUDE.md controller pattern example references outdated createSite logic - CLAUDE.md, "Controller Pattern" section

The CLAUDE.md illustrates the controller pattern with a simplified createSite example that shows a single hasAdminAccess() guard. After this PR, the actual createSite has a two-tier check (admin OR S2S admin grant). The example is merely incomplete rather than actively misleading (the structural pattern remains valid), but a future reader looking at createSite specifically would get a stale picture.

Fix: Update the example to use a different simple controller method, or add a note that it is simplified for illustration.

2. No unit test for the 501 backwards-compat guard - src/controllers/consumers.js:421-425

The typeof consumer.setAdminGrants !== 'function' guard has no dedicated unit test. The mockConsumer in test/controllers/consumers.test.js:68 always defines setAdminGrants, so the 501 path is never exercised. If this guard is accidentally removed or inverted during a future refactor, no test fails.

Fix: Add one test case that deletes mockConsumer.setAdminGrants before calling update with adminGrants, then assert on 501 status and the error message.

Recommendations

• Consider clarifying replace-vs-merge semantics in the adminGrants field description in ConsumerUpdateRequest (docs/openapi/schemas.yaml). The capabilities field says "Replaces the entire array" but adminGrants says "Per-operation admin-bypass grants to set" which is ambiguous about whether sending { CREATE_SITE: true } drops other existing grants. Once a second operation key lands, callers will need to know.
• Consider logging at INFO on the success path in createSite (reason=granted). Currently only denials are logged. A one-line success log provides real-time correlation for forensic reconstruction if a compromised S2S credential mass-creates sites.
• As more operation keys land, consider splitting capability-constants.js into Layer 1 (capabilities) and Layer 2 (admin grants) or a registry-driven approach to reduce per-operation wiring. The current shape is fine for one key.

Assessment

Ready to merge? Yes
Reasoning: The implementation is well-structured, the security model is sound (proper layering, strict equality, TOCTOU prevention, fails closed), test coverage is thorough at all layers, all prior findings are addressed, and documentation is comprehensive. The two Minor items are non-blocking improvements for a future commit.

---

_{Skill: pr-review | Model: us.anthropic.claude-opus-4-6-v1[1m] | Duration: 0m 49s | Cost: $3.24 | Commit: 364b67d3b979c88ef1f5a0fb0071491b2c5a386c
If this code review was useful, please react with 👍. Otherwise, react with 👎.}

[MysticatBot-Dev]

Hey @ravverma,

Strengths

• Previously flagged issues all addressed cleanly across prior commits: _fetchAndValidateConsumer() extraction, controller tests for admin bypass/revoked/not-active, integration tests for the round-trip, the 501 backwards-compat guard, and clear commit-by-commit progression.
• The revert commit message communicates the rationale clearly and acknowledges the review history ("Reviewer flag was raised without this context"). This is how pushback should be done.
• Core implementation remains solid: two-layer access control, strict === true on grants, TOCTOU prevention via fresh DB fetch, generic error messages preventing info leakage, structured denial logging at INFO.
• No security implications from the schema removal - the endpoints remain unchanged, and the OpenAPI spec is not served publicly.

Issues

Important (Should Fix)

1. OpenAPI revert is inconsistent with how other admin-scoped entities are treated - docs/openapi/schemas.yaml

The CLAUDE.md states unconditionally: "Specification Sync: Keep OpenAPI specs and implementation in sync" and under "Modifying Data Models": "Update OpenAPI schemas in docs/openapi/schemas.yaml". These rules do not carve out an exception for internal endpoints.

The "consumer endpoints are internal admin APIs" argument is plausible, but other admin-scoped entities in the same repo (ApiKeyRequest, ApiKeyResponse, Entitlement, SiteEnrollment) already have full OpenAPI schemas despite being admin-managed. The Consumer revert creates an inconsistency with established practice, not a new convention.

Over time, "this is internal" becomes a growing surface of undocumented APIs that require manual exploration. The cost of keeping the schema (a static YAML block validated by npm run docs:lint) is minimal and bounded.

Fix (pick one):

• (a) Restore the Consumer/ConsumerUpdateRequest schemas (consistent with ApiKey/Entitlement treatment, 5-minute change).
• (b) Add a one-line exception clause to CLAUDE.md's "Specification Sync" rule explicitly noting which endpoint families are exempt from OpenAPI documentation. This formalizes the distinction and prevents future reviewers from re-raising it.

Option (b) is the better long-term choice if the team genuinely considers consumer endpoints internal-only.

Minor (Nice to Have)

2. No unit test for the 501 backwards-compat guard - src/controllers/consumers.js:421-425

The typeof consumer.setAdminGrants !== 'function' guard has no dedicated unit test. The mock in test/controllers/consumers.test.js always defines setAdminGrants, so the 501 path is never exercised. A single test case that deletes mockConsumer.setAdminGrants before calling update with adminGrants would guard against accidental removal.

Recommendations

• Consider logging at INFO on the success path in createSite (reason=granted). Currently only denials are logged. A one-line success log provides correlation for forensic reconstruction if a compromised credential is used to mass-create sites.

Assessment

Ready to merge? With fixes
Reasoning: The implementation is well-structured, secure, and thoroughly tested. The sole outstanding concern is a documented Must-Follow rule being overridden without either following established precedent (other admin entities do have schemas) or formalizing the exception. Either restoring the schema or updating CLAUDE.md resolves this cleanly.

Next Steps

1. Either restore the Consumer OpenAPI schemas (option a) or add a documented exception to CLAUDE.md (option b).
2. (Optional) Add a unit test for the 501 guard path.

---

_{Skill: pr-review | Model: us.anthropic.claude-opus-4-6-v1[1m] | Duration: 1m 14s | Cost: $2.59 | Commit: 1c753fb8992b65f0e57bba4ff39332212506db9c
If this code review was useful, please react with 👍. Otherwise, react with 👎.}