refactor(phase 8): service layer extraction

[EnjoyBacon7]

Scope

Extracts a service layer between the routers and the components, thinning each router down to request/response handling and pushing orchestration into dedicated services.

Commits (10):

• 8A.1 add AuthService, thin routers/auth.py
• 8A.2 add UserService, thin routers/users.py
• 8B.1 add PartitionService, thin routers/partition.py, restore delete-user cascade
• 8B.2 add WorkspaceService, thin routers/workspaces.py
• 8C.1 add RetrievalService, thin routers/search.py
• 8C.2 add QueryService, thin routers/openai.py
• 8D add IndexingService + JobService, thin routers/indexer.py + queue.py
• 8D.1 flag the components extract_temporal_fields shim debt for phase 9
• 8E add ConversionService, thin routers/tools.py + extract.py
• 8F verify orchestrator DI wiring, pull last Ray call out of routers/users.py

Summary by CodeRabbit

• New Features

  • Optional OIDC auto-provisioning to create user accounts from IdP claims.

• Refactor

  • HTTP routers now act as thin transport layers; business logic moved to orchestration services for more reliable, consistent behavior.

• Bug Fixes

  • Restored search response metadata (includes file_id) and improved auth/session/logout handling.
  • Safer token counting fallback to avoid failures when external LLM client is unavailable.

[coderabbitai]

Warning

Rate limit exceeded

@EnjoyBacon7 has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 52 minutes and 32 seconds before requesting another review.

You’ve run out of usage credits. Purchase more in the billing tab.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 7959cd96-55e0-465c-a7e1-ae679a7907d7

📥 Commits

Reviewing files that changed from the base of the PR and between 18e8fd8 and d8b2790.

📒 Files selected for processing (2)

• openrag/main.py
• tests/api_tests/test_oidc_lifecycle.py

📝 Walkthrough

<review_stack_artifact>

</review_stack_artifact>

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch refactor/phase-8-service-layer

[coderabbitai]

Actionable comments posted: 8

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

openrag/routers/indexer.py (1)

144-180: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Clean up staged uploads when post-save steps fail.

Line 144 and Line 269 persist files before all fail-fast checks/dispatch complete. Any later 4xx/5xx path (e.g., workspace validation failure or dispatch error) can leave orphaned files on disk.

💡 Suggested fix (centralized cleanup on failure)

 async def add_file(...):
-    original_filename = file.filename
-    file.filename = sanitize_filename(file.filename)
-    try:
-        file_path = await save_file_to_disk(file, Path(DATA_DIR), with_random_prefix=True)
+    file_path = None
+    original_filename = file.filename
+    file.filename = sanitize_filename(file.filename)
+    try:
+        # validate workspace_ids first (unchanged logic) ...
+        file_path = await save_file_to_disk(file, Path(DATA_DIR), with_random_prefix=True)
+        task_id = await service.add_file(
+            file_path=str(file_path),
+            file_id=file_id,
+            partition=partition,
+            metadata=metadata,
+            sanitized_filename=file.filename,
+            original_filename=original_filename,
+            user=user,
+            workspace_ids=parsed_workspace_ids,
+        )
     except Exception as e:
+        if file_path is not None and file_path.exists():
+            file_path.unlink()
         logger.exception("Failed to save file to disk.", error=str(e))
         raise HTTPException(
             status_code=status.HTTP_500_INTERNAL_SERVER_ERROR,
             detail=str(e),
         )
 
 async def put_file(...):
-    file_path = await save_file_to_disk(file, Path(DATA_DIR), with_random_prefix=True)
-
-    task_id = await service.add_file(...)
+    file_path = None
+    try:
+        file_path = await save_file_to_disk(file, Path(DATA_DIR), with_random_prefix=True)
+        task_id = await service.add_file(...)
+    except Exception as e:
+        if file_path is not None and file_path.exists():
+            file_path.unlink()
+        raise

Also applies to: 269-280

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@openrag/routers/indexer.py` around lines 144 - 180, The code saves uploads to
disk via save_file_to_disk (yielding file_path) before running follow-up checks
(workspace validation via service.get_workspace and dispatch via
service.add_file), which can leave orphaned files on any subsequent 4xx/5xx
exit; update the handler to centrally clean up the staged file on failure: after
calling save_file_to_disk track file_path and wrap the remaining logic (parsing
workspace_ids, workspace lookup, and service.add_file) in a try/except/finally
so that on any exception you remove the saved file (using the Path represented
by file_path) before re-raising/returning the HTTPException; apply the same
pattern to the other upload branch that saves files (the second
save_file_to_disk usage referenced) so both paths remove their staged files on
error.

🧹 Nitpick comments (9)

openrag/routers/openai.py (1)

50-53: 💤 Low value

Deprecated on_event usage flagged by CI.

The @router.on_event("startup") decorator is deprecated in recent FastAPI versions. The pipeline warns about this. Consider migrating to FastAPI's lifespan context manager pattern.

♻️ Suggested migration to lifespan handler

+from contextlib import asynccontextmanager
+
+@asynccontextmanager
+async def lifespan(app):
+    global _max_model_tokens
+    _max_model_tokens = await _fetch_max_model_tokens()
+    yield
+
-@router.on_event("startup")
-async def _cache_max_model_tokens():
-    global _max_model_tokens
-    _max_model_tokens = await _fetch_max_model_tokens()

Then attach the lifespan to the router or app at mount time.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@openrag/routers/openai.py` around lines 50 - 53, The decorator
`@router.on_event`("startup") is deprecated; replace this startup hook by creating
an async lifespan context manager that calls _fetch_max_model_tokens and assigns
the result to the module-level _max_model_tokens (i.e., replicate what
_cache_max_model_tokens() did), and then attach that lifespan to the router/app
when mounting this router. Specifically, remove the decorated
_cache_max_model_tokens function, implement an async contextmanager (lifespan)
that awaits _fetch_max_model_tokens() and sets _max_model_tokens before
yielding, and ensure the router is created/attached with that lifespan so the
token cache runs at startup.

openrag/services/orchestrators/user_service.py (2)

109-111: 💤 Low value

Clarify: default_quota=0 does not apply as a creation default.

When default_file_quota is 0 (a valid limit per guidelines), the condition > 0 prevents it from being applied to new users. This means users created without an explicit quota will have file_quota=None and inherit the global default at runtime. If the intent is that default_quota=0 should also be applied at creation time, change to >= 0. Otherwise, this is acceptable but worth a brief inline comment explaining the distinction.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@openrag/services/orchestrators/user_service.py` around lines 109 - 111, The
creation logic skips applying a default when self._default_file_quota == 0
because it checks "> 0", so change the condition to apply zero as a valid
default: update the block that reads "file_quota = fields.get('file_quota'); if
self._default_file_quota > 0 and file_quota is None:" to use "if
self._default_file_quota >= 0 and file_quota is None:" (or alternatively check
"if self._default_file_quota is not None and file_quota is None:" if None is
used to signal no default), and add a brief inline comment next to
self._default_file_quota explaining that 0 is an intentional valid quota to be
applied at creation.

---

193-211: 💤 Low value

update_user returns User model but create_user returns dict — inconsistent shapes.

create_user returns the dict from create_legacy_user, while update_user manually constructs a dict from the User model. The router expects dicts for JSON serialization, so update_user is correctly converting. However, this inconsistency may cause confusion. Consider adding a comment or extracting a _user_to_dict helper for consistency.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@openrag/services/orchestrators/user_service.py` around lines 193 - 211, The
update_user function returns a dict constructed from the User model while
create_user returns whatever create_legacy_user returns, creating inconsistent
response shapes; extract a single helper (e.g., _user_to_dict) that accepts a
User instance and returns the canonical dict with fields id, display_name,
external_user_id, email, is_admin, created_at (isoformat or None), file_quota,
file_count, then replace the manual dict construction in update_user with a call
to _user_to_dict and update create_user to call the same helper (or add a short
comment linking create_legacy_user to the helper if that return must remain
special) so both code paths produce the same JSON shape.

openrag/services/orchestrators/auth_service.py (1)

92-100: 💤 Low value

Misleading function name _utcnow returns local time.

The function name suggests UTC, but datetime.now() returns local time. While the docstring explains the rationale (matching tz-naive DB columns), the name creates confusion for maintainers. Consider renaming to _local_now() or _naive_now() to match the actual behavior.

♻️ Suggested rename for clarity

-def _utcnow() -> datetime:
-    """Naive local ``now`` — matches the DB columns.
+def _naive_now() -> datetime:
+    """Naive local ``now`` — matches the DB columns.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@openrag/services/orchestrators/auth_service.py` around lines 92 - 100, The
function named `_utcnow` actually returns local naive time (datetime.now())
which is misleading; rename the function to a clearer identifier such as
`_naive_now` or `_local_now`, update its docstring to reflect the new name, and
update every usage/call site (all references to `_utcnow`) to the new name
(e.g., `_naive_now`) to keep behavior unchanged; run/adjust any tests or imports
that reference `_utcnow`.

openrag/services/orchestrators/test_auth_service.py (2)

376-385: ⚡ Quick win

Use specific exception type for quota test.

Line 382 uses pytest.raises(Exception) which is too broad.

♻️ Suggested fix

     # Specific limit exceeded (3 indexed + 2 pending >= 5).
-    with pytest.raises(Exception):
+    with pytest.raises(OpenRAGError):
         AuthService.validate_file_quota({"file_count": 3, "file_quota": 5}, pending_task_count=2, default_quota=10)

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@openrag/services/orchestrators/test_auth_service.py` around lines 376 - 385,
The test uses a broad pytest.raises(Exception) in test_validate_file_quota;
replace that with the exact exception type that AuthService.validate_file_quota
actually raises (e.g., QuotaExceededError or ValueError) by importing that
exception and using it in the with pytest.raises(...) assertion so the test
asserts the specific failure mode from AuthService.validate_file_quota.

---

349-363: ⚡ Quick win

Use specific exception types in pytest.raises for clearer test intent.

Lines 351, 360, and 382 use pytest.raises(Exception) or OIDCFlowError.__bases__[0], which are broad or fragile. Using the specific AuthError or OpenRAGError would make test failures more informative and prevent false passes if a different exception is raised.

♻️ Suggested fix

+from core.utils.exceptions import AuthError, OpenRAGError
+
 def test_require_admin():
     assert AuthService.require_admin({"is_admin": True}) == {"is_admin": True}
-    with pytest.raises(OIDCFlowError.__bases__[0]):  # OpenRAGError subclass (AuthError)
+    with pytest.raises(AuthError):
         AuthService.require_admin({"is_admin": False})


 def test_check_partition_access_role_hierarchy():
     parts = [{"partition": "p1", "role": "viewer"}]
     assert AuthService.check_partition_access(
         user={"is_admin": False}, partition="p1", user_partitions=parts, required_role="viewer"
     )
-    with pytest.raises(Exception):
+    with pytest.raises(AuthError):
         AuthService.check_partition_access(
             user={"is_admin": False}, partition="p1", user_partitions=parts, required_role="owner"
         )

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@openrag/services/orchestrators/test_auth_service.py` around lines 349 - 363,
Replace the fragile/overbroad exception assertions in tests by asserting the
specific error type: in test_require_admin replace
pytest.raises(OIDCFlowError.__bases__[0]) with pytest.raises(AuthError) (import
AuthError if needed), and in test_check_partition_access_role_hierarchy replace
pytest.raises(Exception) with pytest.raises(AuthError) (or
pytest.raises(OpenRAGError) if the implementation raises the more general type);
update imports to bring AuthError/OpenRAGError into scope and run tests to
confirm.

openrag/services/orchestrators/test_partition_service.py (2)

83-88: ⚡ Quick win

FakeVectorStore drops collection, so collection wiring is untested.

The fake accepts collection but doesn’t record or assert it. A wrong collection in service calls would still pass these tests.

Suggested tightening

 class FakeVectorStore:
     def __init__(self, ids=None, rows=None):
         self._ids = ids or []
         self._rows = rows or []
         self.deleted_ids: list[str] = []
+        self.query_collections: list[str] = []
+        self.delete_collections: list[str] = []

     async def query_ids_by_filter(self, collection, filters):
+        self.query_collections.append(collection)
         return list(self._ids)

     async def delete(self, ids, collection="default") -> int:
+        self.delete_collections.append(collection)
         self.deleted_ids.extend(ids)
         return len(ids)

 async def test_delete_partition_drops_vectors_then_rows():
@@
     await _svc(prepo=prepo, vstore=vstore).delete_partition("p1")
+    assert vstore.query_collections == ["vdb"]
+    assert vstore.delete_collections == ["vdb"]

Also applies to: 90-91, 149-152, 158-161

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@openrag/services/orchestrators/test_partition_service.py` around lines 83 -
88, FakeVectorStore methods query_ids_by_filter and delete (and the other fake
methods at the noted ranges) accept a collection parameter but ignore it, so
collection wiring isn't tested; update FakeVectorStore (class and its methods
like query_ids_by_filter, delete and the other fake handlers around the noted
ranges) to record the collection argument (e.g., append to a
self.seen_collections list or store per-call entries in self.calls) and, where
appropriate, include the collection in returned data or expose it via a property
so tests can assert the collection used for each call; ensure each method
signature still accepts the collection default and that tests are updated to
assert on the recorded collection(s).

---

148-154: ⚡ Quick win

Order claim in test name is not actually asserted.

Line 148 says “drops_vectors_then_rows”, but the test only checks both happened, not sequence. This can miss orchestration regressions.

Suggested tightening

 `@pytest.mark.asyncio`
 async def test_delete_partition_drops_vectors_then_rows():
+    events: list[str] = []
+
+    class OrderedPartitionRepo(FakePartitionRepo):
+        async def delete_partition(self, name: str) -> bool:
+            events.append("rows")
+            return await super().delete_partition(name)
+
+    class OrderedVectorStore(FakeVectorStore):
+        async def delete(self, ids, collection="default") -> int:
+            events.append("vectors")
+            return await super().delete(ids, collection)
+
-    prepo = FakePartitionRepo(existing={"p1"})
-    vstore = FakeVectorStore(ids=["c1", "c2"])
+    prepo = OrderedPartitionRepo(existing={"p1"})
+    vstore = OrderedVectorStore(ids=["c1", "c2"])
     await _svc(prepo=prepo, vstore=vstore).delete_partition("p1")
     assert vstore.deleted_ids == ["c1", "c2"]
     assert prepo.deleted == ["p1"]
+    assert events == ["vectors", "rows"]

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@openrag/services/orchestrators/test_partition_service.py` around lines 148 -
154, The test test_delete_partition_drops_vectors_then_rows claims an order but
only asserts both deletions happened; update the fakes and assertions to verify
sequence: have FakeVectorStore (used as vstore) and FakePartitionRepo (prepo)
record deletion events with an order marker (e.g., incrementing sequence id or
timestamp) when their delete methods are called, then assert that the
vector-store delete events for ids ["c1","c2"] occurred before the partition
delete event recorded by prepo; keep test name and assert that vstore deletion
sequence indices are all less than prepo.deleted sequence index to enforce
"vectors then rows" behavior.

openrag/services/orchestrators/test_workspace_service.py (1)

70-75: ⚡ Quick win

Vector-store collection propagation isn’t verified in workspace delete tests.

The fake currently ignores collection; this weakens confidence that WorkspaceService uses the configured collection consistently.

Suggested tightening

 class FakeVectorStore:
     def __init__(self, ids_by_file=None):
         self._ids_by_file = ids_by_file or {}
         self.deleted: list[list[str]] = []
+        self.query_collections: list[str] = []
+        self.delete_collections: list[str] = []

     async def query_ids_by_filter(self, collection, filters):
+        self.query_collections.append(collection)
         return list(self._ids_by_file.get(filters.get("file_id"), []))

     async def delete(self, ids, collection="default") -> int:
+        self.delete_collections.append(collection)
         self.deleted.append(list(ids))
         return len(ids)

 async def test_delete_workspace_cleans_orphans_vectors_and_rows():
@@
     out = await _svc(wrepo=wrepo, drepo=drepo, vstore=vstore).delete_workspace("p", "w1")
+    assert vstore.query_collections == ["vdb", "vdb"]
+    assert vstore.delete_collections == ["vdb"]

Also applies to: 135-137, 149-150

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@openrag/services/orchestrators/test_workspace_service.py` around lines 70 -
75, The fake in test_workspace_service currently ignores the collection
parameter which weakens validation; update the fake methods (query_ids_by_filter
and delete) to consider the collection argument (e.g., keying _ids_by_file by
(collection, file_id) or storing last_collection used), then update the tests to
assert the expected collection is passed through — specifically modify the
fake's query_ids_by_filter(collection, filters) to use collection when looking
up ids and modify delete(ids, collection="default") to record the collection
(e.g., append (collection, ids) or increment per-collection counters) and add
assertions in the WorkspaceService delete tests to verify the configured
collection was used.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@openrag/core/config/auth.py`:
- Around line 25-28: Remove the runtime auto-provisioning flag and any code
paths that create users when an OIDC `sub` is unknown: delete the
auto_provision_login configuration (auto_provision_login) from
openrag/core/config/auth.py and any references to it, and update the OIDC login/
callback logic (where external_user_id or user provisioning is handled) to
strictly match users by users.external_user_id == sub with no email fallback;
when no matching user exists, return a 403 error path instead of creating a new
user.

In `@openrag/di/container.py`:
- Around line 257-423: The properties (user_service, partition_service,
workspace_service, retrieval_service, query_service, conversion_service — and
any other getters that dereference self._settings) access self._settings without
a null guard; add a consistent guard that raises the container's intended
runtime error when settings are missing. Implement a small helper (e.g.
_ensure_settings_present or _require_settings) that checks if self._settings is
None and raises the container-specific RuntimeError, then call that helper at
the start of each affected property (user_service, partition_service,
workspace_service, retrieval_service, query_service, conversion_service) before
any use of self._settings to ensure failures follow the container's error
contract.

In `@openrag/main.py`:
- Around line 265-271: Replace the broad except in the ServiceContainer wiring
so only expected infrastructure/connectivity errors are handled as graceful
degradation: catch specific exceptions (e.g., database/VectorDB connection
errors such as psycopg2.OperationalError, sqlalchemy.exc.OperationalError,
ConnectionError) and log those with logger.warning while keeping
app.state.container = None; for any other Exception (import errors, syntax
errors, misconfiguration) log full traceback at error/exception level and
re-raise or fail startup. Update the try/except around the from di.container
import ServiceContainer and the ServiceContainer(config) instantiation
accordingly and ensure di/providers.py behavior remains unchanged for
request-time fallback.

In `@openrag/routers/extract.py`:
- Line 65: Guard the metadata access for chunk["metadata"]["partition"] to avoid
KeyError by checking for the presence of "partition" (e.g., via "partition" in
chunk.get("metadata", {}) or metadata.get("partition") is not None) before
assigning chunk_partition; if missing, follow the same not-found/corrupt-chunk
response path currently used elsewhere in this handler (return the 404 flow) so
absent partition metadata produces a 404 instead of a 500.

In `@openrag/services/orchestrators/auth_service.py`:
- Around line 368-374: The code currently indexes cls.ROLE_HIERARCHY[user_role]
which can raise KeyError if membership.get("role") returns an unexpected value;
update the check in the method that uses membership and user_role to first
validate that user_role is truthy and present in cls.ROLE_HIERARCHY (e.g., if
not user_role or user_role not in cls.ROLE_HIERARCHY: raise AuthError with a
clear message about unknown or invalid role for partition and appropriate
status_code), then proceed to compare ROLE_HIERARCHY[user_role] <
ROLE_HIERARCHY[required_role] and return True on success.

In `@openrag/services/orchestrators/job_service.py`:
- Around line 39-40: Replace direct Ray actor .remote() awaits with the
centralized timeout helper: import and use call_ray_actor_with_timeout from
services.workers.ray_utils and invoke it with the TaskStateManager actor method
(e.g., replace await self._tsm.get_all_states.remote() with await
call_ray_actor_with_timeout(self._tsm.get_all_states, ...) ), doing the same for
the other actor calls in this file (the uses of self._tsm.*.remote() around the
other occurrences). Ensure you pass the same args/kwargs and an appropriate
timeout value so the TaskStateManager methods (get_all_states, and the other
actor methods used in this module) are called through
call_ray_actor_with_timeout to preserve cancellation/timeouts.

In `@openrag/services/orchestrators/test_user_service.py`:
- Around line 235-251: The test fails because the fake existing User is created
without a created_at so the service returns None; update the test_setup where
repo._users[2] is assigned (in test_update_user_returns_legacy_dict_shape) to
include a valid created_at (e.g., a datetime value) so the service will produce
an ISO-formatted string in out["created_at"]; locate the FakeUserRepo/User
instantiation and add a created_at value (import datetime in the test file if
needed) rather than changing service logic.

In `@openrag/services/storage/indexer_ray_shim.py`:
- Around line 47-53: The _call() implementation imports
call_ray_actor_with_timeout from services.workers.ray_utils — change that import
to components.ray_utils and use the centralized call_ray_actor_with_timeout to
ensure consistent timeout/cancellation semantics; update the import statement in
the file containing _call (reference symbol: call_ray_actor_with_timeout and
function name _call) so the call remains identical but sources from
components.ray_utils, and run tests to confirm no other references to
services.workers.ray_utils remain.

---

Outside diff comments:
In `@openrag/routers/indexer.py`:
- Around line 144-180: The code saves uploads to disk via save_file_to_disk
(yielding file_path) before running follow-up checks (workspace validation via
service.get_workspace and dispatch via service.add_file), which can leave
orphaned files on any subsequent 4xx/5xx exit; update the handler to centrally
clean up the staged file on failure: after calling save_file_to_disk track
file_path and wrap the remaining logic (parsing workspace_ids, workspace lookup,
and service.add_file) in a try/except/finally so that on any exception you
remove the saved file (using the Path represented by file_path) before
re-raising/returning the HTTPException; apply the same pattern to the other
upload branch that saves files (the second save_file_to_disk usage referenced)
so both paths remove their staged files on error.

---

Nitpick comments:
In `@openrag/routers/openai.py`:
- Around line 50-53: The decorator `@router.on_event`("startup") is deprecated;
replace this startup hook by creating an async lifespan context manager that
calls _fetch_max_model_tokens and assigns the result to the module-level
_max_model_tokens (i.e., replicate what _cache_max_model_tokens() did), and then
attach that lifespan to the router/app when mounting this router. Specifically,
remove the decorated _cache_max_model_tokens function, implement an async
contextmanager (lifespan) that awaits _fetch_max_model_tokens() and sets
_max_model_tokens before yielding, and ensure the router is created/attached
with that lifespan so the token cache runs at startup.

In `@openrag/services/orchestrators/auth_service.py`:
- Around line 92-100: The function named `_utcnow` actually returns local naive
time (datetime.now()) which is misleading; rename the function to a clearer
identifier such as `_naive_now` or `_local_now`, update its docstring to reflect
the new name, and update every usage/call site (all references to `_utcnow`) to
the new name (e.g., `_naive_now`) to keep behavior unchanged; run/adjust any
tests or imports that reference `_utcnow`.

In `@openrag/services/orchestrators/test_auth_service.py`:
- Around line 376-385: The test uses a broad pytest.raises(Exception) in
test_validate_file_quota; replace that with the exact exception type that
AuthService.validate_file_quota actually raises (e.g., QuotaExceededError or
ValueError) by importing that exception and using it in the with
pytest.raises(...) assertion so the test asserts the specific failure mode from
AuthService.validate_file_quota.
- Around line 349-363: Replace the fragile/overbroad exception assertions in
tests by asserting the specific error type: in test_require_admin replace
pytest.raises(OIDCFlowError.__bases__[0]) with pytest.raises(AuthError) (import
AuthError if needed), and in test_check_partition_access_role_hierarchy replace
pytest.raises(Exception) with pytest.raises(AuthError) (or
pytest.raises(OpenRAGError) if the implementation raises the more general type);
update imports to bring AuthError/OpenRAGError into scope and run tests to
confirm.

In `@openrag/services/orchestrators/test_partition_service.py`:
- Around line 83-88: FakeVectorStore methods query_ids_by_filter and delete (and
the other fake methods at the noted ranges) accept a collection parameter but
ignore it, so collection wiring isn't tested; update FakeVectorStore (class and
its methods like query_ids_by_filter, delete and the other fake handlers around
the noted ranges) to record the collection argument (e.g., append to a
self.seen_collections list or store per-call entries in self.calls) and, where
appropriate, include the collection in returned data or expose it via a property
so tests can assert the collection used for each call; ensure each method
signature still accepts the collection default and that tests are updated to
assert on the recorded collection(s).
- Around line 148-154: The test test_delete_partition_drops_vectors_then_rows
claims an order but only asserts both deletions happened; update the fakes and
assertions to verify sequence: have FakeVectorStore (used as vstore) and
FakePartitionRepo (prepo) record deletion events with an order marker (e.g.,
incrementing sequence id or timestamp) when their delete methods are called,
then assert that the vector-store delete events for ids ["c1","c2"] occurred
before the partition delete event recorded by prepo; keep test name and assert
that vstore deletion sequence indices are all less than prepo.deleted sequence
index to enforce "vectors then rows" behavior.

In `@openrag/services/orchestrators/test_workspace_service.py`:
- Around line 70-75: The fake in test_workspace_service currently ignores the
collection parameter which weakens validation; update the fake methods
(query_ids_by_filter and delete) to consider the collection argument (e.g.,
keying _ids_by_file by (collection, file_id) or storing last_collection used),
then update the tests to assert the expected collection is passed through —
specifically modify the fake's query_ids_by_filter(collection, filters) to use
collection when looking up ids and modify delete(ids, collection="default") to
record the collection (e.g., append (collection, ids) or increment
per-collection counters) and add assertions in the WorkspaceService delete tests
to verify the configured collection was used.

In `@openrag/services/orchestrators/user_service.py`:
- Around line 109-111: The creation logic skips applying a default when
self._default_file_quota == 0 because it checks "> 0", so change the condition
to apply zero as a valid default: update the block that reads "file_quota =
fields.get('file_quota'); if self._default_file_quota > 0 and file_quota is
None:" to use "if self._default_file_quota >= 0 and file_quota is None:" (or
alternatively check "if self._default_file_quota is not None and file_quota is
None:" if None is used to signal no default), and add a brief inline comment
next to self._default_file_quota explaining that 0 is an intentional valid quota
to be applied at creation.
- Around line 193-211: The update_user function returns a dict constructed from
the User model while create_user returns whatever create_legacy_user returns,
creating inconsistent response shapes; extract a single helper (e.g.,
_user_to_dict) that accepts a User instance and returns the canonical dict with
fields id, display_name, external_user_id, email, is_admin, created_at
(isoformat or None), file_quota, file_count, then replace the manual dict
construction in update_user with a call to _user_to_dict and update create_user
to call the same helper (or add a short comment linking create_legacy_user to
the helper if that return must remain special) so both code paths produce the
same JSON shape.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 770384a3-acf1-450d-9840-929b2c30b66f

📥 Commits

Reviewing files that changed from the base of the PR and between de42f86 and c8660d7.

📒 Files selected for processing (38)

• REFACTORING_DECISION_LOG.md
• openrag/core/config/auth.py
• openrag/core/indexing/dispatcher.py
• openrag/core/indexing/serializer.py
• openrag/di/container.py
• openrag/di/providers.py
• openrag/di/test_container.py
• openrag/main.py
• openrag/routers/auth.py
• openrag/routers/extract.py
• openrag/routers/indexer.py
• openrag/routers/openai.py
• openrag/routers/partition.py
• openrag/routers/queue.py
• openrag/routers/search.py
• openrag/routers/tools.py
• openrag/routers/users.py
• openrag/routers/workspaces.py
• openrag/services/orchestrators/auth_service.py
• openrag/services/orchestrators/conversion_service.py
• openrag/services/orchestrators/indexing_service.py
• openrag/services/orchestrators/job_service.py
• openrag/services/orchestrators/partition_service.py
• openrag/services/orchestrators/query_service.py
• openrag/services/orchestrators/retrieval_service.py
• openrag/services/orchestrators/test_auth_service.py
• openrag/services/orchestrators/test_conversion_service.py
• openrag/services/orchestrators/test_indexing_service.py
• openrag/services/orchestrators/test_job_service.py
• openrag/services/orchestrators/test_partition_service.py
• openrag/services/orchestrators/test_query_service.py
• openrag/services/orchestrators/test_retrieval_service.py
• openrag/services/orchestrators/test_user_service.py
• openrag/services/orchestrators/test_workspace_service.py
• openrag/services/orchestrators/user_service.py
• openrag/services/orchestrators/workspace_service.py
• openrag/services/storage/indexer_ray_shim.py
• openrag/services/storage/serializer_ray_shim.py

[coderabbitai]

Actionable comments posted: 2

🧹 Nitpick comments (1)

openrag/routers/test_auth_router.py (1)

37-45: ⚡ Quick win

Restore sys.modules["utils.dependencies"] after this module’s tests.

The current import-time override is global and can leak into other test modules. Add a module-scoped cleanup fixture to restore the previous module object.

Suggested fix

+_PREV_UTILS_DEPENDENCIES = sys.modules.get("utils.dependencies")
+
 def _install_dependencies_stub() -> None:
     stub = types.ModuleType("utils.dependencies")
     stub.get_vectordb = lambda: None
     stub.get_task_state_manager = lambda: None
     stub.get_serializer = lambda: None
@@
 _install_dependencies_stub()
+
+
+@pytest.fixture(scope="module", autouse=True)
+def _restore_dependencies_module():
+    yield
+    if _PREV_UTILS_DEPENDENCIES is None:
+        sys.modules.pop("utils.dependencies", None)
+    else:
+        sys.modules["utils.dependencies"] = _PREV_UTILS_DEPENDENCIES

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@openrag/routers/test_auth_router.py` around lines 37 - 45, The test helper
_install_dependencies_stub currently overwrites
sys.modules["utils.dependencies"] at import time and can leak into other tests;
change this by adding a pytest fixture (module-scoped) that saves orig =
sys.modules.get("utils.dependencies"), installs the stub (using the same stub
creation logic in _install_dependencies_stub), yields to run tests, and on
teardown restores the original: if orig is None remove
sys.modules["utils.dependencies"], otherwise set
sys.modules["utils.dependencies"] = orig; reference the fixture from tests that
need the stub or call the fixture to ensure proper cleanup.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@openrag/di/container.py`:
- Line 93: The call currently sets auto_provision_login using
os.getenv("OIDC_AUTO_PROVISION_LOGIN"...), which allows runtime enabling of OIDC
auto-provisioning; change this to force False by replacing the env-based
expression with literal False (i.e., set auto_provision_login=False) so the OIDC
flow never auto-provisions users; update the argument named auto_provision_login
in container.py accordingly and remove the os.getenv(...) expression.

In `@openrag/main.py`:
- Around line 293-297: When container.initialize() raises, the app must mark the
container unavailable so requests don't hit a partially initialized
ServiceContainer; in the except block where you catch Exception around await
container.initialize(), set app.state.container = None (in addition to
logger.exception) so the app uses the degraded path, referencing the existing
container variable and app.state.container in the ServiceContainer
initialization flow.

---

Nitpick comments:
In `@openrag/routers/test_auth_router.py`:
- Around line 37-45: The test helper _install_dependencies_stub currently
overwrites sys.modules["utils.dependencies"] at import time and can leak into
other tests; change this by adding a pytest fixture (module-scoped) that saves
orig = sys.modules.get("utils.dependencies"), installs the stub (using the same
stub creation logic in _install_dependencies_stub), yields to run tests, and on
teardown restores the original: if orig is None remove
sys.modules["utils.dependencies"], otherwise set
sys.modules["utils.dependencies"] = orig; reference the fixture from tests that
need the stub or call the fixture to ensure proper cleanup.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 23dae878-2312-4a7b-b3f3-07b4732caa89

📥 Commits

Reviewing files that changed from the base of the PR and between c8660d7 and 18e8fd8.

📒 Files selected for processing (10)

• REFACTORING_DECISION_LOG.md
• openrag/components/utils.py
• openrag/di/container.py
• openrag/main.py
• openrag/routers/auth.py
• openrag/routers/extract.py
• openrag/routers/search.py
• openrag/routers/test_auth_router.py
• openrag/services/orchestrators/auth_service.py
• openrag/services/orchestrators/job_service.py

🚧 Files skipped from review as they are similar to previous changes (4)

• REFACTORING_DECISION_LOG.md
• openrag/routers/extract.py
• openrag/routers/auth.py
• openrag/services/orchestrators/auth_service.py