refactor(user-mgmt): one UserService in autobot_shared, and the fork baseline fails on stale entries (#15068, #18088)

[mrveiss]

Thinking Path

#15068 listed four user_management files forked across both backends. Measured on current main, three of them had already converged:

• team_service (refactor(user-management): move byte-identical team_service to autobot_shared (#12647) #13164);
• organization_service (refactor(user-management): converge organization_service via injected models (#12647) #13201, which binds concrete models by injection);
• rbac_middleware (refactor(rbac): one permission-enforcement middleware and one transaction scope for both services (#18088, #15068) #18159).

The one real fork left was UserService: 900 and 863 lines, 128 diff lines, and its four error classes. This PR converges it the same way OrganizationService was converged: one shared implementation, and each backend binds its own User and supplies its own hooks. Where the two copies differed, the union of checks won, and nothing was relaxed.

Measuring also showed that two symbol-fork baseline entries had been stale on main with the suite green. No test checked the real tree for stale entries. That gap is fixed here.

What Changed

• 0b78abf262 (tech-debt(user-mgmt): four user_management files remain forked after #12647 closed #15068): one UserService at autobot_shared/user_management/user_service.py:42, split across six modules, none over 256 lines.
  • Bindings: autobot-backend/.../services/user_service.py:21 supplies both hooks (invalidate_other_sessions → SessionService, reassign_memory → memory.ownership_reassign). autobot-slm-backend/.../services/user_service.py:17 binds the model only, so the hooks are no-ops there.
  • Error classes: the backend's superset (DuplicateUserError(field=)) moved to autobot_shared/user_management/user_service_errors.py. 13 importers are repointed (9 backend, 4 SLM).
  • Duplicate insert: the race-safe SAVEPOINT insert (bug(user-mgmt): create_user reads then inserts, so a concurrent duplicate returns 500 instead of the 409 #15752 landed #15772) moved to user_service_conflict.py, and both services use it.
  • Guard: repo_tests/one_user_service_15068_test.py (11 tests, with contrast fixtures). It covers one definition of each error class and of the insert helper, and requires both bindings to subclass the shared class and add only the hooks.
• d256f74d3b, c7e0341b18 (security(rbac): two byte-identical 595-line rbac_middleware copies, one per service #18088 housekeeping):
  • RBACMiddleware and require_all_permissions are dropped from repo_tests/symbol_fork_baseline.json. They had been satisfied since refactor(rbac): one permission-enforcement middleware and one transaction scope for both services (#18088, #15068) #18159.
  • New test_no_stale_baseline_entries (symbol_fork_ratchet_17312_test.py) runs the real tree. Restoring the RBACMiddleware line fails it, and the current tree passes.
  • The four UserService error classes are also dropped. UserService itself stays listed, because the shared class and its two bindings share the name, the same shape as OrganizationService.

Behaviour changes (none relaxes a check)

• Audit payload, the one visible change. The password-change audit entry now writes both sessions_revoked_from and sessions_invalidated in both services (user_service_lifecycle.py:81-82). Nothing is removed or renamed. sessions_invalidated is 0 in the SLM, which does not invalidate sessions.
• SLM duplicate-signup race. It now raises DuplicateUserError instead of a raw IntegrityError, the stricter backend behaviour.
• Schema is unchanged. Measured per class via models/core_test.py's subprocess-per-class loader, before and after:

class	columns	relationships	fingerprint
backend User	18	11	f61052165d5f
backend Organization	24	6	f3a57afa3cb1
SLM User	18	6	cf99c6fc82a9
SLM Organization	11	4	65d767cfb257

Kept semantically identical on purpose

get_user_permissions does select(role.Permission), an attribute Role does not have, so the DB permission lookup raises and the middleware denies. Both forks had this line. It is moved semantically identical (now read through a permission_model local, user_service_access.py:175-178): fixing it grants permissions that are denied today, so it keeps # type: ignore[attr-defined] # #18259. The owner decided on 2026-10-11 that #18259 is fixed separately, with the newly granted set listed for review.

Acceptance criteria (#15068)

• team_service: met on main by refactor(user-management): move byte-identical team_service to autobot_shared (#12647) #13164. One definition at autobot_shared/user_management/team_service.py:66, and both backends hold 32-line shims.
• rbac_middleware / user_service converged with the union of checks:
  • rbac_middleware: met on main by refactor(rbac): one permission-enforcement middleware and one transaction scope for both services (#18088, #15068) #18159 (autobot_shared/.../middleware/rbac_middleware.py:130).
  • user_service: this PR.
• Authz carve-out: no rbac code moved in this PR, and UserService's permission lookup is kept semantically identical (above). No decision changes.
• organization_service: resolved by injection on main (refactor(user-management): converge organization_service via injected models (#12647) #13201). Shims are at organization_service.py:28 in each backend.
• Schema unchanged: the table above.

Verification

• On the rebased head: autobot_shared/user_management/ plus the new guard, the fork ratchet, the rbac guard, the size ratchet and ceiling parity: 200 passed. reach_scope_claim_17844_test, run alone: 10 passed.
• Pre-rebase:
  • backend user_management / api / signup-race / LLC contacts / ownership_reassign: 185 passed;
  • models/core_test.py: 10 passed;
  • SLM SSO/SCIM/RBAC/auth plus user_management: 191 passed, 1 failed. The failure is pre-existing and identical on untouched main, filed as test(slm): test_client_secret_stored_in_config_jsonb fails on main — 'user_management.models' is not a package #18265.

Not done, stated rather than implied

• security(rbac): two byte-identical 595-line rbac_middleware copies, one per service #18088 AC3 (one enum-typed require_permission) needs the permission-source decision. The owner chose a strict union on 2026-10-11, and it is filed as child authz: one enum-typed require_permission over one strict-union evaluator (#18088 AC3) #18263 (blocked by security(rbac): UserService.get_user_permissions reads role.Permission, which does not exist — the DB-backed permission path raises and the middleware denies #18259). security(rbac): two byte-identical 595-line rbac_middleware copies, one per service #18088 AC4 (the refactor(auth): unify 4 auth/RBAC layers — SLM has zero shared code with main backend (security drift risk) #6511 re-check) found refactor(auth): unify 4 auth/RBAC layers — SLM has zero shared code with main backend (security drift risk) #6511 closed with criteria unmet, so the remainder is filed as auth: #6511 remainder — deny audit from both backends, cross-service parity, service_auth_enforcement JWT path #18264. That is why this PR is Refs security(rbac): two byte-identical 595-line rbac_middleware copies, one per service #18088, not Closes.

Closes #15068
Refs #18088

Model Used

Claude Opus 5.5 (master session: decisions, rebase, verification, PR). The implementation was done by a Claude Opus subagent.

🤖 Generated with Claude Code

Summary by CodeRabbit

• New Features
  • Standardised user account management across both backends, including sign-up, authentication, profile updates, password changes, role management, and account lifecycle operations.
  • Improved handling of duplicate accounts during creation, including concurrent sign-up attempts.

[coderabbitai]

📝 Walkthrough

📝 Walkthrough

📝 Walkthrough

Walkthrough

The change moves user-management operations into autobot_shared and reduces both backend UserService implementations to model-specific subclasses. Backend call sites now import shared error classes. Tests cover shared behaviour and repository checks for duplicate implementations.

Changes

Shared user-management consolidation

Layer / File(s)	Summary
Shared service foundation and user operations autobot_shared/user_management/user_service_base.py, autobot_shared/user_management/user_service.py, autobot_shared/user_management/user_service_conflict.py, autobot_shared/user_management/user_service_errors.py, autobot_shared/user_management/user_service_test.py	The shared base adds model binding, lookup, password, hook, and audit helpers. The shared service adds user creation, listing, and profile updates, using duplicate-safe insertion and shared error types.
Shared access and lifecycle operations autobot_shared/user_management/user_service_access.py, autobot_shared/user_management/user_service_lifecycle.py, autobot_shared/user_management/user_service_test.py	Shared mixins add authentication, role and permission access, password changes, activation, and deletion. Tests cover hooks, audit details, and deletion behaviour when memory reassignment fails. The permission lookup continues to access role.Permission, which the summary identifies as absent on Role.
Backend bindings and repository checks autobot-backend/user_management/services/user_service.py, autobot-slm-backend/user_management/services/user_service.py, autobot-backend/api/..., autobot-backend/user_management/services/..., autobot-slm-backend/api/..., autobot-slm-backend/main.py, autobot-slm-backend/user_management/middleware/rbac_middleware.py, autobot-backend/user_management/middleware/rbac_middleware.py, repo_tests/one_user_service_15068_test.py, repo_tests/one_rbac_middleware_18088_test.py, repo_tests/symbol_fork_baseline.json, repo_tests/symbol_fork_ratchet_17312_test.py	Both backend services bind the shared service to their local user model; the backend adds session-invalidation and memory-reassignment hooks. API and service callers import exception classes from the shared error module. Repository tests check backend bindings, shared definitions, and stale symbol-fork baseline entries.

---

---

---

---

Priority: ➖ Normal

Merge Risk: 🔵 Low · up to 44b34

Signing up or provisioning a user whose email belongs to one existing account and whose username belongs to another fails with a server error instead of a clear duplicate error. After this change, the same failure also affects the SLM service through SCIM provisioning and admin setup. The rest of the consolidation is low risk; this small fix is recommended before merging.

Pre-merge checks | 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Linked Issues check		The PR meets the coding requirements in #15068. It adds one shared UserService implementation in autobot_shared/user_management/, binds the backend and SLM User models, and keeps backend-specifi…
Out of Scope Changes check		The changes stay within the scope of #15068. The baseline removals record symbols that are already converged, and test_no_stale_baseline_entries supports the fork-convergence guards. Import updates,…
Docstring Coverage		Docstring coverage is 80.56% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 72 functions across 27 files.
Description Check		Check skipped - CodeRabbit’s high-level summary is enabled.
Title check		The title clearly summarises the main changes: consolidating UserService in autobot_shared and updating the fork baseline stale-entry check. It is specific and related to the pull request.

---

✨ Finishing Touches 💡 1

🛠️ Fix failing CI checks 💡

• Commit to this branch
• Create a new PR

---

---

---

📝 Generate docstrings

• Commit to this branch
• Create a new PR

---

---

---

---

---

🧪 Generate unit tests (beta)

• Commit to this branch
• Create a new PR

---

---

---

---

---

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

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

---

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

[coderabbitai]

Actionable comments posted: 1

---

ℹ️ Review info

⚙️ Run configuration

• Configuration used: Repository: mrveiss/AutoBot-AI/.coderabbit.yaml
• Review profile: ASSERTIVE
• Plan: Advanced
• Run ID: 4e2baf47-9ce0-43ba-a0c8-a0550c151503

📥 Commits

Reviewing files that changed from the base of the PR and between 8faa865 and 44b3404.

⛔ Files ignored due to path filters (2)

• repo_tests/python_file_size_ratchet_baseline.py is excluded by !repo_tests/python_file_size_ratchet_baseline.py
• scripts/python_file_size_known_large.py is excluded by !scripts/python_file_size_known_large.py

📒 Files selected for processing (28)

• autobot-backend/api/auth.py
• autobot-backend/api/user_management/password_change.py
• autobot-backend/api/user_management/password_change_test.py
• autobot-backend/api/user_management/users.py
• autobot-backend/api/user_management/users_duplicate_conflict_test.py
• autobot-backend/user_management/middleware/rbac_middleware.py
• autobot-backend/user_management/services/seed.py
• autobot-backend/user_management/services/seed_test.py
• autobot-backend/user_management/services/user_service.py
• autobot-backend/user_management/services/user_service_conflict_test.py
• autobot-backend/user_management/services/user_service_test.py
• autobot-slm-backend/api/autobot_users.py
• autobot-slm-backend/api/scim.py
• autobot-slm-backend/api/slm_users.py
• autobot-slm-backend/main.py
• autobot-slm-backend/user_management/middleware/rbac_middleware.py
• autobot-slm-backend/user_management/services/user_service.py
• autobot_shared/user_management/user_service.py
• autobot_shared/user_management/user_service_access.py
• autobot_shared/user_management/user_service_base.py
• autobot_shared/user_management/user_service_conflict.py
• autobot_shared/user_management/user_service_errors.py
• autobot_shared/user_management/user_service_lifecycle.py
• autobot_shared/user_management/user_service_test.py
• repo_tests/one_rbac_middleware_18088_test.py
• repo_tests/one_user_service_15068_test.py
• repo_tests/symbol_fork_baseline.json
• repo_tests/symbol_fork_ratchet_17312_test.py

💤 Files with no reviewable changes (1)

• repo_tests/symbol_fork_baseline.json

Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.

[coderabbitai]

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Handle two different rows matching the duplicate lookup.

_find_by_email_or_username uses OR across two independently unique columns. Assume user A holds the email and user B holds the username. Then the query returns two rows. In that case scalar_one_or_none() raises MultipleResultsFound.

This lookup has two callers:

• UserService._check_duplicate_user runs before every create_user. The signup handler in autobot-backend/api/auth.py maps any non-DuplicateUserError exception to HTTP 500 "Registration failed". So the caller gets HTTP 500, not HTTP 409.
• _resolve_conflict in user_service_conflict.py runs after a lost race. There, the same exception replaces the intended DuplicateUserError.

After this PR, the SLM also runs both paths, so this behaviour now reaches SCIM provisioning and _ensure_admin_user. Take the first match and prefer the email match, so that field stays deterministic.

🐛 Proposed fix

--- "a/autobot_shared/user_management/user_service_base.py"
+++ "b/autobot_shared/user_management/user_service_base.py"
@@ -134,18 +134,20 @@
         result = await self.session.execute(query)
         return result.scalar_one_or_none()
 
     async def _find_by_email_or_username(self, email: str, username: str) -> UserCore | None:
-        """Find any user (deleted included) holding this email or username."""
+        """Find any user (deleted included) holding this email or username.
+
+        Two different rows can match (one per column); the email holder wins.
+        """
         user = self.user_model
+        email_match = func.lower(user.email) == email.lower()
         result = await self.session.execute(
-            select(user).where(
-                or_(
-                    func.lower(user.email) == email.lower(),
-                    func.lower(user.username) == username.lower(),
-                )
-            )
+            select(user)
+            .where(or_(email_match, func.lower(user.username) == username.lower()))
+            .order_by(email_match.desc())
+            .limit(1)
         )
-        return result.scalar_one_or_none()
+        return result.scalars().first()
 
     async def _audit_log(
         self,

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	async def _find_by_email_or_username(self, email: str, username: str) -> UserCore | None:
	"""Find any user (deleted included) holding this email or username."""
	user = self.user_model
	result = await self.session.execute(
	select(user).where(
	or_(
	func.lower(user.email) == email.lower(),
	func.lower(user.username) == username.lower(),
	)
	)
	)
	return result.scalar_one_or_none()
	async def _find_by_email_or_username(self, email: str, username: str) -> UserCore | None:
	"""Find any user (deleted included) holding this email or username.
	Two different rows can match (one per column); the email holder wins.
	"""
	user = self.user_model
	email_match = func.lower(user.email) == email.lower()
	result = await self.session.execute(
	select(user)
	.where(or_(email_match, func.lower(user.username) == username.lower()))
	.order_by(email_match.desc())
	.limit(1)
	)
	return result.scalars().first()