fix(collections): Use correct file batch ID - #1073
Conversation
📝 WalkthroughWalkthrough
ChangesVector batch ingestion
Document upload validation
Collection batch retries
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
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. Comment |
OpenAPI changes ⚪ No API surface changesNote This PR does not modify the API contract.
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
backend/app/tests/crud/rag/test_open_ai.py (2)
72-72: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the self-evident comment.
assert_not_called()already expresses this expectation; retain comments only when they explain rationale. As per coding guidelines, “Write comments and docstrings to explain why something is done, not what the code already shows.”🤖 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 `@backend/app/tests/crud/rag/test_open_ai.py` at line 72, Remove the self-evident “list_files should not have been called on the happy path” comment near the assert_not_called() expectation in the test, leaving the assertion unchanged.Source: Coding guidelines
44-44: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd required type annotations to changed test signatures.
Annotate non-
selffixture parameters and return values consistently.
backend/app/tests/crud/rag/test_open_ai.py#L44-L44: annotatemock_client: MagicMock.backend/app/tests/crud/rag/test_open_ai.py#L75-L75: annotate injected fixtures and add-> None.backend/app/tests/crud/rag/test_open_ai.py#L106-L108: annotate injected fixtures and add-> None.backend/app/tests/crud/rag/test_open_ai.py#L122-L124: annotate injected fixtures and add-> None.🤖 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 `@backend/app/tests/crud/rag/test_open_ai.py` at line 44, Add the requested annotations in backend/app/tests/crud/rag/test_open_ai.py: type _wire_batch’s mock_client parameter as MagicMock, and annotate all non-self injected fixture parameters plus return values with -> None in the test functions at lines 75, 106-108, and 122-124. Apply the annotations consistently across all four affected sites.Source: Coding guidelines
🤖 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.
Nitpick comments:
In `@backend/app/tests/crud/rag/test_open_ai.py`:
- Line 72: Remove the self-evident “list_files should not have been called on
the happy path” comment near the assert_not_called() expectation in the test,
leaving the assertion unchanged.
- Line 44: Add the requested annotations in
backend/app/tests/crud/rag/test_open_ai.py: type _wire_batch’s mock_client
parameter as MagicMock, and annotate all non-self injected fixture parameters
plus return values with -> None in the test functions at lines 75, 106-108, and
122-124. Apply the annotations consistently across all four affected sites.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 79f5c90e-9ea2-43f6-8dc9-ff715cb54c65
📒 Files selected for processing (6)
backend/app/crud/rag/open_ai.pybackend/app/tests/crud/rag/test_open_ai.pybackend/app/tests/services/collections/providers/test_openai_provider.pybackend/app/tests/utils/llm_provider.pybackend/app/tests/utils/openai.pydocs/wiki/modules/knowledge-base.md
…in document upload process
There was a problem hiding this comment.
🧹 Nitpick comments (2)
backend/app/services/documents/helpers.py (2)
1-6: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winModernize deprecated
typingimports.
Callable/Iterableshould come fromcollections.abc, andTupleis deprecated in favor of builtintuple(PEP 585). Ruff flags both (UP035).♻️ Proposed fix
-from typing import Callable, Optional, Tuple, Iterable, Union +from collections.abc import Callable, Iterable +from typing import Optional, UnionNote: other
Tuple[...]usages later in the file (e.g._read_edges,validate_document_content... wait, that one already uses lowercaseTuplereturn) should be updated totuple[...]for consistency once the import changes.🤖 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 `@backend/app/services/documents/helpers.py` around lines 1 - 6, Update the imports in this module so Callable and Iterable come from collections.abc, remove Tuple from typing, and replace every Tuple[...] annotation in the file with the builtin tuple[...] syntax; preserve Optional and Union unless independently required.Source: Linters/SAST tools
53-57: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winDuplicated magic byte signatures between
MISLABELLED_BINARY_SIGNATURESandFORMAT_SPECS;"doc"has noFormatSpec.
b"PK\x03\x04"andb"\xd0\xcf\x11\xe0\xa1\xb1\x1a\xe1"are inlined in both structures. Extracting them (and the zip-signature triple used for both docx and xlsx) into named constants would remove the duplication.Separately,
"doc"is absent fromFORMAT_SPECSeven though it shares the OLE2 signature documented for"xls"(comment at line 246 explicitly calls out ".doc"), andregistry.pymaps.doc→"doc". As written,.docuploads get zero content sniffing while.xlsuploads (same binary container format) do.♻️ Proposed fix
+OLE2_SIGNATURE = b"\xd0\xcf\x11\xe0\xa1\xb1\x1a\xe1" +ZIP_SIGNATURES = (b"PK\x03\x04", b"PK\x05\x06", b"PK\x07\x08") + MISLABELLED_BINARY_SIGNATURES: dict[bytes, str] = { - b"PK\x03\x04": "an Office/zip file (xlsx, docx)", - b"\xd0\xcf\x11\xe0\xa1\xb1\x1a\xe1": "a legacy Office file (xls, doc)", + ZIP_SIGNATURES[0]: "an Office/zip file (xlsx, docx)", + OLE2_SIGNATURE: "a legacy Office file (xls, doc)", b"%PDF-": "a PDF", } ... "docx": FormatSpec( - signatures=(b"PK\x03\x04", b"PK\x05\x06", b"PK\x07\x08"), checker=_check_docx + signatures=ZIP_SIGNATURES, checker=_check_docx ), "xlsx": FormatSpec( - signatures=(b"PK\x03\x04", b"PK\x05\x06", b"PK\x07\x08"), checker=_check_xlsx + signatures=ZIP_SIGNATURES, checker=_check_xlsx ), - "xls": FormatSpec(signatures=(b"\xd0\xcf\x11\xe0\xa1\xb1\x1a\xe1",)), + "xls": FormatSpec(signatures=(OLE2_SIGNATURE,)), + "doc": FormatSpec(signatures=(OLE2_SIGNATURE,)),Also applies to: 237-250
🤖 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 `@backend/app/services/documents/helpers.py` around lines 53 - 57, Extract the shared PK/zip and OLE2 byte signatures into named constants, then reuse them in MISLABELLED_BINARY_SIGNATURES and the relevant FORMAT_SPECS entries, including the existing zip-signature triple for docx and xlsx. Add a “doc” FORMAT_SPECS entry matching the “xls” OLE2 signature and appropriate shared sniffing behavior so .doc uploads are handled like .xls.
🤖 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.
Nitpick comments:
In `@backend/app/services/documents/helpers.py`:
- Around line 1-6: Update the imports in this module so Callable and Iterable
come from collections.abc, remove Tuple from typing, and replace every
Tuple[...] annotation in the file with the builtin tuple[...] syntax; preserve
Optional and Union unless independently required.
- Around line 53-57: Extract the shared PK/zip and OLE2 byte signatures into
named constants, then reuse them in MISLABELLED_BINARY_SIGNATURES and the
relevant FORMAT_SPECS entries, including the existing zip-signature triple for
docx and xlsx. Add a “doc” FORMAT_SPECS entry matching the “xls” OLE2 signature
and appropriate shared sniffing behavior so .doc uploads are handled like .xls.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 6e6e3f5f-23a5-4f47-96b3-cfc57b2d0caa
📒 Files selected for processing (3)
backend/app/api/routes/documents.pybackend/app/services/doctransform/registry.pybackend/app/services/documents/helpers.py
… processing and error management
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/app/services/documents/helpers.py (1)
123-128: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winReject OOXML packages that lack the expected part.
A PPTX or generic OOXML ZIP has
[Content_Types].xmlbut neitherword/norxl/, so it currently passes validation as both DOCX and XLSX. Requireexpected_partto be present; only useother_partto improve the mismatch message.Proposed fix
- if other_part in sample and expected_part not in sample: + if expected_part not in sample: raise DocumentValidationError( filename, - f"parsing error - file is a '{other_part.decode()}' Office package, " - "not the format its extension claims", + ( + f"parsing error - file is a '{other_part.decode()}' Office package, " + "not the format its extension claims" + if other_part in sample + else "parsing error - Office package does not match its extension" + ), )🤖 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 `@backend/app/services/documents/helpers.py` around lines 123 - 128, Update the validation condition around the Office package checks so a package is rejected whenever expected_part is absent, including cases where neither expected_part nor other_part exists. Preserve the existing DocumentValidationError and use other_part only when available to identify the detected package type in the mismatch message; otherwise report the missing expected part through the existing validation path.
🧹 Nitpick comments (4)
backend/app/services/collections/create_collection.py (1)
43-43: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse narrow types for retry inputs.
backend/app/services/collections/create_collection.py#L43-L43: annotatetask_instancewith a narrow bound-task protocol/type plusNone.backend/app/tests/services/collections/test_create_collection.py#L721-L721: replaceBaseExceptionwithTimeout | SoftTimeLimitExceeded.backend/app/tests/services/collections/test_create_collection.py#L779-L779: replaceBaseExceptionwithReject | TimeoutError.As per coding guidelines,
**/*.py: “Use Python 3.11+ and provide narrow type hints for every function parameter and return value.”🤖 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 `@backend/app/services/collections/create_collection.py` at line 43, Use a narrow bound-task protocol/type plus None for the task_instance parameter in _can_retry_batch. In backend/app/tests/services/collections/test_create_collection.py at lines 721 and 779, replace BaseException annotations with Timeout | SoftTimeLimitExceeded and Reject | TimeoutError respectively.Source: Coding guidelines
backend/app/tests/services/collections/test_create_collection.py (1)
720-720: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMark intentionally unused patched storage mocks.
Ruff reports all four injected
mock_get_storageparameters as unused. Rename them to_mock_get_storage(or patch withnew=so no argument is injected) to keep the test suite lint-clean.
backend/app/tests/services/collections/test_create_collection.py#L720-L720: mark the unused storage mock intentional.backend/app/tests/services/collections/test_create_collection.py#L778-L778: mark the unused storage mock intentional.backend/app/tests/services/collections/test_create_collection.py#L830-L830: mark the unused storage mock intentional.backend/app/tests/services/collections/test_create_collection.py#L881-L881: mark the unused storage mock intentional.🤖 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 `@backend/app/tests/services/collections/test_create_collection.py` at line 720, Rename each unused injected mock_get_storage parameter to _mock_get_storage in backend/app/tests/services/collections/test_create_collection.py at lines 720-720, 778-778, 830-830, and 881-881, preserving the existing patched-storage behavior while marking the mocks intentional and keeping Ruff lint-clean.Source: Linters/SAST tools
backend/app/services/documents/helpers.py (1)
231-238: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the shared ZIP signatures.
Both specs duplicate the same signatures. Define a
ZIP_SIGNATURESconstant and reuse it. As per coding guidelines, “Do not use magic values; extract repeated literals into constants, enums, or settings.”🤖 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 `@backend/app/services/documents/helpers.py` around lines 231 - 238, Define a shared ZIP_SIGNATURES constant containing the repeated ZIP signature tuple, then update the docx and xlsx FormatSpec definitions to reference it instead of duplicating the literal signatures. Keep needs_tail and each checker unchanged.Source: Coding guidelines
backend/app/services/collections/providers/registry.py (1)
8-8: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKeep provider configuration out of the CRUD module.
backend/app/services/collections/providers/registry.pynow imports shared client settings frombackend/app/crud/rag/open_ai.py, creating a service-to-CRUD dependency. Move these constants to a neutral settings/provider-configuration module and import them from both locations.As per coding guidelines, use the repository's layered architecture: models represent database entities, CRUD handles database operations, API routes expose REST endpoints, services contain business logic, and Celery tasks handle asynchronous work.
Also applies to: 67-71
🤖 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 `@backend/app/services/collections/providers/registry.py` at line 8, Move OPENAI_MAX_RETRIES and OPENAI_TIMEOUT_SECONDS out of app.crud.rag.open_ai into a neutral settings or provider-configuration module. Update both providers.registry and the CRUD OpenAI implementation to import these constants from the new module, removing the service-to-CRUD dependency while preserving their existing values and behavior.Source: Coding guidelines
🤖 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 `@backend/app/crud/rag/open_ai.py`:
- Around line 140-141: Update _poll_file_batch() so only a batch with status
"completed" is returned as successful; explicitly handle terminal statuses such
as "failed" and "cancelled" by raising or otherwise failing the upload,
including cancelled counts in the failure details where applicable. Do not let
non-completed terminal batches reach the successful-log path.
- Around line 136-140: Update _poll_file_batch so each _retrieve_file_batch call
cannot run past the remaining BATCH_POLL_TIMEOUT_SECONDS deadline. Compute the
remaining time before retrieval and pass it through the client/request timeout
while disabling or limiting retries for this polling operation; preserve the
existing status handling and stop polling when the deadline is exhausted.
In `@backend/app/services/documents/helpers.py`:
- Line 240: Update the XLS FormatSpec near OLE2_SIGNATURE to use an XLS-specific
checker that inspects the OLE compound document for a Workbook or Book stream
before accepting it. Keep generic OLE2 validation for other formats, but reject
legacy .doc and unrelated OLE containers in the XLS path.
In `@docs/architecture/kaapi-knowledge-base-ARCHITECTURE.md`:
- Around line 292-302: Update the sequence diagram to match the revised Phase
1/Phase 2 lifecycle: show setup creating and recording the vector store before
persisting metadata, enqueue batch 1 with the resolved vector_store_id, and
remove any setup step that uploads all documents. Show each batch performing its
bounded document upload and vector-store attach operation.
In `@docs/wiki/modules/knowledge-base.md`:
- Line 39: Update the knowledge-base documentation sentence to use standard
Python terminology: replace “contextvar” with “ContextVar” or “context
variable.” Preserve the existing explanation about caller coupling and polling
deadlines.
---
Outside diff comments:
In `@backend/app/services/documents/helpers.py`:
- Around line 123-128: Update the validation condition around the Office package
checks so a package is rejected whenever expected_part is absent, including
cases where neither expected_part nor other_part exists. Preserve the existing
DocumentValidationError and use other_part only when available to identify the
detected package type in the mismatch message; otherwise report the missing
expected part through the existing validation path.
---
Nitpick comments:
In `@backend/app/services/collections/create_collection.py`:
- Line 43: Use a narrow bound-task protocol/type plus None for the task_instance
parameter in _can_retry_batch. In
backend/app/tests/services/collections/test_create_collection.py at lines 721
and 779, replace BaseException annotations with Timeout | SoftTimeLimitExceeded
and Reject | TimeoutError respectively.
In `@backend/app/services/collections/providers/registry.py`:
- Line 8: Move OPENAI_MAX_RETRIES and OPENAI_TIMEOUT_SECONDS out of
app.crud.rag.open_ai into a neutral settings or provider-configuration module.
Update both providers.registry and the CRUD OpenAI implementation to import
these constants from the new module, removing the service-to-CRUD dependency
while preserving their existing values and behavior.
In `@backend/app/services/documents/helpers.py`:
- Around line 231-238: Define a shared ZIP_SIGNATURES constant containing the
repeated ZIP signature tuple, then update the docx and xlsx FormatSpec
definitions to reference it instead of duplicating the literal signatures. Keep
needs_tail and each checker unchanged.
In `@backend/app/tests/services/collections/test_create_collection.py`:
- Line 720: Rename each unused injected mock_get_storage parameter to
_mock_get_storage in
backend/app/tests/services/collections/test_create_collection.py at lines
720-720, 778-778, 830-830, and 881-881, preserving the existing patched-storage
behavior while marking the mocks intentional and keeping Ruff lint-clean.
🪄 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 Plus
Run ID: 08d37652-b4a3-4011-9097-4498923146b3
📒 Files selected for processing (9)
backend/app/crud/rag/open_ai.pybackend/app/services/collections/create_collection.pybackend/app/services/collections/providers/registry.pybackend/app/services/documents/helpers.pybackend/app/tests/crud/rag/test_open_ai.pybackend/app/tests/services/collections/providers/test_openai_provider.pybackend/app/tests/services/collections/test_create_collection.pydocs/architecture/kaapi-knowledge-base-ARCHITECTURE.mddocs/wiki/modules/knowledge-base.md
🚧 Files skipped from review as they are similar to previous changes (2)
- backend/app/tests/services/collections/providers/test_openai_provider.py
- backend/app/tests/crud/rag/test_open_ai.py
| deadline = time.monotonic() + BATCH_POLL_TIMEOUT_SECONDS | ||
|
|
||
| while True: | ||
| batch = self._retrieve_file_batch(batch_id, vector_store_id) | ||
| if batch.status != "in_progress": |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "## locate file"
fd -a 'open_ai\.py$' . || true
echo "## inspect relevant file outline/section"
if [ -f backend/app/crud/rag/open_ai.py ]; then
wc -l backend/app/crud/rag/open_ai.py
ast-grep outline backend/app/crud/rag/open_ai.py || true
echo "## lines 1-240"
sed -n '1,240p' backend/app/crud/rag/open_ai.py | nl -ba
echo "## timeout/retry references"
rg -n "timeout|retries|deadline|BATCH_POLL_TIMEOUT_SECONDS|_retrieve_file_batch|AsyncOpenAI|OpenAI" backend/app/crud/rag/open_ai.py || true
fi
echo "## dependency/version manifests"
for f in requirements.txt requirements-dev.txt pyproject.toml setup.cfg setup.py uv.lock poetry.lock Pipfile.lock; do
[ -f "$f" ] && { echo "--- $f"; rg -n "openai|httpx" "$f" || true; }
done
echo "## other timeout/retry usages"
rg -n "AsyncOpenAI|OpenAI\(|timeout\s*=|retries\s*=" backend || true
echo "## git diff stat and relevant diff"
git diff --stat || true
git diff -- backend/app/crud/rag/open_ai.py || trueRepository: ProjectTech4DevAI/kaapi-backend
Length of output: 1362
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "## lines 1-240"
sed -n '1,240p' backend/app/crud/rag/open_ai.py
echo "## timeout/retry references"
rg -n "timeout|retries|deadline|BATCH_POLL_TIMEOUT_SECONDS|_retrieve_file_batch|AsyncOpenAI|OpenAI" backend/app/crud/rag/open_ai.py || true
echo "## dependency/version manifests"
for f in requirements.txt requirements-dev.txt pyproject.toml setup.cfg setup.py uv.lock poetry.lock Pipfile.lock; do
[ -f "$f" ] && { echo "--- $f"; rg -n "openai|httpx" "$f" || true; }
done
echo "## other timeout/retry usages"
rg -n "AsyncOpenAI|OpenAI\(|timeout\s*=|retries\s*=" backend || true
echo "## focused tests for poll/retrieval client"
rg -n "OpenAIVectorStoreCrud|_poll_file_batch|_retrieve_file_batch|OPENAI_MAX_RETRIES|OPENAI_TIMEOUT_SECONDS|vector_store_file_batches|retrieve" backend/app/tests backend/app/crud backend/app/services || trueRepository: ProjectTech4DevAI/kaapi-backend
Length of output: 35059
🌐 Web query:
OpenAI Python SDK configuration 90 second timeout max_retries=2 default retry behavior
💡 Result:
To configure the OpenAI Python SDK with a 90-second timeout and 2 retries, you can set these parameters when initializing the OpenAI client [1][2]. Configuration Example: from openai import OpenAI import httpx # Configure timeout and retries client = OpenAI( timeout=90.0, # Set total timeout to 90 seconds [1][3] max_retries=2, # Set maximum number of retries [1][3]) Default Behavior and Details: - Timeout: The default timeout is 10 minutes (600 seconds) [4]. Setting timeout=90.0 overrides this global default [1][2]. You can also pass an httpx.Timeout object for more granular control (e.g., specifying connect, read, or write timeouts) [1][5]. - Retries: The default number of retries is already 2 [1][6]. These retries occur automatically for specific error types, including connection errors, 408 Request Timeout, 409 Conflict, 429 Rate Limit, and >=500 Internal Server Errors [1][3][6]. - Per-Request Overrides: If you need to apply different settings to a specific call rather than the entire client, you can use the with_options method [1][7]: response = client.with_options(timeout=90.0, max_retries=2).chat.completions.create(...) Note: Requests that time out are subject to the same retry logic and will be retried up to the configured max_retries limit [1][2][5].
Citations:
- 1: https://github.com/openai/openai-python/blob/2384e276969d5908eebff2ca12bdfcd69b3cfa93/README.md
- 2: https://github.com/openai/openai-python/blob/main/README.md
- 3: https://developers.openai.com/api/reference/python/
- 4: https://github.com/openai/openai-python/blob/722d3fff/src/openai/_constants.py
- 5: https://mintlify.wiki/openai/openai-python/concepts/timeouts
- 6: https://mintlify.wiki/openai/openai-python/concepts/retries
- 7: https://mintlify.wiki/openai/openai-python/concepts/client
Make the batch-poll deadline enforceable.
_retrieve_file_batch() uses the client’s 90s timeout and default retries, but _poll_file_batch() only checks the 240s deadline after _retrieve_file_batch() returns. If the SDK retries timed-out retrievals, polling can exceed the advertised wall-clock budget. Bound each retrieve/retry by the remaining deadline, or disable/limit retries for polling specifically.
🤖 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 `@backend/app/crud/rag/open_ai.py` around lines 136 - 140, Update
_poll_file_batch so each _retrieve_file_batch call cannot run past the remaining
BATCH_POLL_TIMEOUT_SECONDS deadline. Compute the remaining time before retrieval
and pass it through the client/request timeout while disabling or limiting
retries for this polling operation; preserve the existing status handling and
stop polling when the deadline is exhausted.
Source: MCP tools
| if batch.status != "in_progress": | ||
| return batch |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Files matching open_ai.py:"
fd -a 'open_ai\.py$' . || true
file="$(fd 'open_ai\.py$' backend/app/crud/rag 2>/dev/null | head -n1 || true)"
echo "Selected file: $file"
if [ -n "${file:-}" ]; then
wc -l "$file"
ast-grep outline "$file" --view compact || true
echo "---- lines 1-240 ----"
sed -n '1,240p' "$file" | nl -ba
fi
echo "---- related references to status/file_counts/cancelled in file ----"
if [ -n "${file:-}" ]; then
rg -n 'status|file_counts|cancel|cancelled|fail|in_progress|completed' "$file"
fiRepository: ProjectTech4DevAI/kaapi-backend
Length of output: 747
🌐 Web query:
OpenAI vector store file batches statuses cancelled failed completed in_progress API documentation
💡 Result:
In the OpenAI API, both Vector Store File Batches and individual Vector Store Files share the same four possible statuses: in_progress, completed, cancelled, and failed [1][2][3][4]. When working with Vector Store File Batches, the status reflects the overall state of the batch operation [1][5]. Additionally, these objects include a file_counts attribute that provides a breakdown of the constituent files' statuses, including the number of files currently in progress, completed, failed, and cancelled, as well as the total count [1][6]. When listing files within a specific batch, you can filter the results using these same four status values [3][7]. The status completed specifically indicates that a file is successfully processed and ready for use [3][4][7].
Citations:
- 1: https://developers.openai.com/api/reference/resources/vector_stores/subresources/file_batches/methods/create/
- 2: https://developers.openai.com/api/reference/go/resources/vector_stores/subresources/file_batches/
- 3: https://developers.openai.com/api/reference/typescript/resources/vector_stores/subresources/file_batches/methods/list_files/
- 4: https://developers.openai.com/api/reference/go/resources/vector_stores/subresources/file_batches/methods/list_files/
- 5: https://developers.openai.com/api/reference/typescript/resources/vector_stores/subresources/file_batches/methods/create
- 6: https://developers.openai.com/api/reference/python/resources/vector_stores/subresources/file_batches/methods/create
- 7: https://developers.openai.com/api/reference/java/resources/vector_stores/subresources/file_batches/methods/list_files/
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="backend/app/crud/rag/open_ai.py"
echo "---- ast-grep outline ----"
ast-grep outline "$file" --view names || true
echo "---- lines 1-240 ----"
awk '{printf "%5d\t%s\n", NR, $0}' "$file" | sed -n '1,240p'
echo "---- relevant references ----"
grep -nE 'status|file_counts|cancel|cancelled|fail|in_progress|completed|update\(' "$file" || true
test_file="backend/app/tests/crud/rag/test_open_ai.py"
if [ -f "$test_file" ]; then
echo "---- tests relevant references ----"
grep -nE 'status|file_counts|cancel|cancelled|fail|in_progress|completed|update\(' "$test_file" || true
fiRepository: ProjectTech4DevAI/kaapi-backend
Length of output: 14994
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="backend/app/crud/rag/open_ai.py"
echo "---- lines 240-335 ----"
awk '{printf "%5d\t%s\n", NR, $0}' "$file" | sed -n '240,335p'
echo "---- file-count-like usages repository-wide ----"
grep -RInE 'file_counts|cancelled|cancelled_files|list_files\(.*filter=' backend/app tests 2>/dev/null || trueRepository: ProjectTech4DevAI/kaapi-backend
Length of output: 8174
Handle non-completed terminal batch statuses explicitly.
_poll_file_batch() returns on any status except in_progress, so cancelled and failed batches can reach the successful-log path when completed == 0 and only failed > 0 is checked. Treat only completed as success and raise/fail the upload for terminal non-completed statuses, including cancelled counts where relevant.
🤖 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 `@backend/app/crud/rag/open_ai.py` around lines 140 - 141, Update
_poll_file_batch() so only a batch with status "completed" is returned as
successful; explicitly handle terminal statuses such as "failed" and "cancelled"
by raising or otherwise failing the upload, including cancelled counts in the
failure details where applicable. Do not let non-completed terminal batches
reach the successful-log path.
Source: MCP tools
| needs_tail=True, | ||
| checker=_check_xlsx, | ||
| ), | ||
| "xls": FormatSpec(signatures=(OLE2_SIGNATURE,)), |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Validate the XLS workbook structure, not just its container.
OLE2_SIGNATURE identifies a generic Compound File Binary container; a legacy .doc or unrelated OLE document passes as .xls. Add an XLS checker that verifies the Workbook/Book stream before accepting the upload.
🤖 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 `@backend/app/services/documents/helpers.py` at line 240, Update the XLS
FormatSpec near OLE2_SIGNATURE to use an XLS-specific checker that inspects the
OLE compound document for a Workbook or Book stream before accepting it. Keep
generic OLE2 validation for other formats, but reject legacy .doc and unrelated
OLE containers in the XLS path.
| 3. Compute `total_size_mb` and call `batch_documents()` to produce the batch plan. | ||
| 4. **Create the vector store** via `provider.create_vector_store()` and record it | ||
| on `result` immediately, so a later failure can always tear it down. | ||
| 5. Persist batch metadata on the job (`total_batches`, `current_batch_number=0`, | ||
| `documents_uploaded=[]`). | ||
| 6. Enqueue **batch 1** with `vector_store_id=None` and the remaining batches as a | ||
| tail list. | ||
| 6. Enqueue **batch 1** with the resolved `vector_store_id` and the remaining | ||
| batches as a tail list. | ||
|
|
||
| > Note the asymmetry: *file upload to the provider is not itself batched* — it | ||
| > happens for all docs in this single setup task. Only the **vector-store attach** | ||
| > is batched across Phase-2 tasks. See §11 for the residual timeout risk this | ||
| > leaves. | ||
| > Setup does **no** file uploading. Both the upload and the vector-store attach | ||
| > happen per batch in Phase 2, so each is bounded by the batch caps (≤200 docs / | ||
| > ≤30 MB). |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Synchronize the sequence diagram with the revised lifecycle.
Lines 258-267 still show setup uploading every document and enqueueing batch 1 with vector_store_id=None; this contradicts the new Phase 1/2 description. Update the diagram to show vector-store creation in setup and per-batch upload/attach work.
🤖 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 `@docs/architecture/kaapi-knowledge-base-ARCHITECTURE.md` around lines 292 -
302, Update the sequence diagram to match the revised Phase 1/Phase 2 lifecycle:
show setup creating and recording the vector store before persisting metadata,
enqueue batch 1 with the resolved vector_store_id, and remove any setup step
that uploads all documents. Show each batch performing its bounded document
upload and vector-store attach operation.
| - Uploads de-duplicate by provider file ID (see deep dive §7). | ||
| - Collections are immutable-ish: deletion semantics in deep dive §10. | ||
| - OpenAI file-batch id: the SDK's `file_batches.poll()` / `upload_and_poll()` final return deserializes a vector-store body, so its `.id` is the `vs_` id, not the `vsfb_` batch id. `crud/rag/open_ai.py` captures the batch id from `create()` before polling and uses it for `list_files`. Any failed file is a hard failure (whole vector store rolled back); partial indexing needs an add-documents endpoint first. | ||
| - The SDK's `file_batches.poll()` never times out. `_poll_file_batch` polls `retrieve` itself under a fixed `BATCH_POLL_TIMEOUT_SECONDS` deadline. It used to take that deadline from the caller via a `task_budget` contextvar; that was deleted once the batch retry landed, because overrunning the soft limit stopped being fatal. Don't reintroduce caller coupling here. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use standard Python terminology for contextvar.
Replace contextvar with ContextVar or “context variable” to resolve the spelling/terminology warning.
🧰 Tools
🪛 LanguageTool
[grammar] ~39-~39: Ensure spelling is correct
Context: ...ine from the caller via a task_budget contextvar; that was deleted once the batch retry l...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🤖 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 `@docs/wiki/modules/knowledge-base.md` at line 39, Update the knowledge-base
documentation sentence to use standard Python terminology: replace “contextvar”
with “ContextVar” or “context variable.” Preserve the existing explanation about
caller coupling and polling deadlines.
Source: Linters/SAST tools
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
backend/app/tests/services/documents/test_helpers.py (2)
265-279: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAnnotate the test-method parameters.
Line 265 and Line 278 leave both
selfandmonkeypatchuntyped; the same applies to the other newly added test methods. Useself: Selfandmonkeypatch: pytest.MonkeyPatchthroughout the new test classes.As per coding guidelines, “Use Python 3.11+ and provide narrow type hints for every function parameter and return value.”
🤖 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 `@backend/app/tests/services/documents/test_helpers.py` around lines 265 - 279, Annotate every newly added test method in the test classes with narrow parameter types: use self: Self and monkeypatch: pytest.MonkeyPatch, while preserving the existing return annotations. Apply this consistently to test_valid_csv_returns_format_and_transformer, test_invalid_content_raises_http_400_with_client_message, and the other new test methods.Source: Coding guidelines
205-219: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDerive the large-payload size from the configured parse threshold.
300000is duplicated and only exercises the closure-only branch while it remains aboveJSON_FULL_PARSE_MAX_BYTES. Import that constant and build both fixtures from it so these tests continue to cover the intended branch if the limit changes.Proposed fix
from app.services.documents.helpers import ( + JSON_FULL_PARSE_MAX_BYTES, SNIFF_HEAD_BYTES, SNIFF_TAIL_BYTES, @@ - content = b"[" + b" " * 300000 + b"]" + content = b"[" + b" " * JSON_FULL_PARSE_MAX_BYTES + b"]" @@ - content = b"[" + b" " * 300000 + content = b"[" + b" " * JSON_FULL_PARSE_MAX_BYTESAs per coding guidelines, “Do not use magic values; extract repeated literals into constants, enums, or settings.”
🤖 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 `@backend/app/tests/services/documents/test_helpers.py` around lines 205 - 219, Update test_large_json_closed_correctly_passes and test_large_json_not_closed_raises to import and use JSON_FULL_PARSE_MAX_BYTES when constructing their payloads, adding enough bytes to remain above the configured full-parse threshold. Remove the duplicated 300000 literal while preserving the existing closed and unclosed tail behaviors.Source: Coding guidelines
🤖 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 `@backend/app/tests/crud/rag/test_open_ai.py`:
- Line 305: Update the new test methods
test_raises_interrupted_error_past_deadline and the method at the additional
referenced location with narrow type annotations for self if required, crud, and
mock_client, and add -> None return annotations. Follow the existing test typing
conventions and do not alter test behavior.
- Around line 332-336: Update get_mock_openai_client_with_vector_store() to
configure file_batches.retrieve.return_value to the same mock_file_batch used
for the batch contract, then replace the poll.return_value assertion in the test
with an assertion that file_batches.retrieve.return_value is batch.
- Around line 307-309: Update the monotonic side-effect sequence in the timeout
test to derive its post-deadline value from the production
BATCH_POLL_TIMEOUT_SECONDS constant instead of hard-coding 300, while preserving
the existing timing sequence and timeout behavior.
---
Nitpick comments:
In `@backend/app/tests/services/documents/test_helpers.py`:
- Around line 265-279: Annotate every newly added test method in the test
classes with narrow parameter types: use self: Self and monkeypatch:
pytest.MonkeyPatch, while preserving the existing return annotations. Apply this
consistently to test_valid_csv_returns_format_and_transformer,
test_invalid_content_raises_http_400_with_client_message, and the other new test
methods.
- Around line 205-219: Update test_large_json_closed_correctly_passes and
test_large_json_not_closed_raises to import and use JSON_FULL_PARSE_MAX_BYTES
when constructing their payloads, adding enough bytes to remain above the
configured full-parse threshold. Remove the duplicated 300000 literal while
preserving the existing closed and unclosed tail behaviors.
🪄 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 Plus
Run ID: 8f4d2555-2352-4e72-88c6-112d1cc14252
📒 Files selected for processing (2)
backend/app/tests/crud/rag/test_open_ai.pybackend/app/tests/services/documents/test_helpers.py
| """When the batch never leaves 'in_progress', the poll loop must give up at the | ||
| deadline rather than spin forever like the SDK's own poll().""" | ||
|
|
||
| def test_raises_interrupted_error_past_deadline(self, crud, mock_client): |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add narrow type hints to the new test methods.
The new methods leave parameters and return values untyped. Add annotations for crud, mock_client, and -> None (and annotate self if enforcing the rule literally).
As per coding guidelines, **/*.py requires narrow type hints for every function parameter and return value.
Proposed fix
- def test_raises_interrupted_error_past_deadline(self, crud, mock_client):
+ def test_raises_interrupted_error_past_deadline(
+ self: "TestPollFileBatchTimeout",
+ crud: OpenAIVectorStoreCrud,
+ mock_client: MagicMock,
+ ) -> None:
...
- def test_wiring_contract(self):
+ def test_wiring_contract(
+ self: "TestGetMockOpenAIClientWithVectorStore",
+ ) -> None:Also applies to: 327-327
🤖 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 `@backend/app/tests/crud/rag/test_open_ai.py` at line 305, Update the new test
methods test_raises_interrupted_error_past_deadline and the method at the
additional referenced location with narrow type annotations for self if
required, crud, and mock_client, and add -> None return annotations. Follow the
existing test typing conventions and do not alter test behavior.
Source: Coding guidelines
| # monotonic: deadline setup, first below-deadline check, its sleep arg, | ||
| # then a value past the deadline that trips the timeout. | ||
| mock_time.monotonic.side_effect = [0, 0, 0, 300] |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Tie the timeout test to the configured deadline.
Line 309 hard-codes 300; if BATCH_POLL_TIMEOUT_SECONDS changes, this sequence may no longer reach the deadline. Use the production timeout constant instead.
As per coding guidelines, do not use magic values; extract them into constants or settings.
Proposed fix
- mock_time.monotonic.side_effect = [0, 0, 0, 300]
+ mock_time.monotonic.side_effect = [
+ 0,
+ 0,
+ 0,
+ BATCH_POLL_TIMEOUT_SECONDS,
+ ]📝 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.
| # monotonic: deadline setup, first below-deadline check, its sleep arg, | |
| # then a value past the deadline that trips the timeout. | |
| mock_time.monotonic.side_effect = [0, 0, 0, 300] | |
| # monotonic: deadline setup, first below-deadline check, its sleep arg, | |
| # then a value past the deadline that trips the timeout. | |
| mock_time.monotonic.side_effect = [ | |
| 0, | |
| 0, | |
| 0, | |
| BATCH_POLL_TIMEOUT_SECONDS, | |
| ] |
🤖 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 `@backend/app/tests/crud/rag/test_open_ai.py` around lines 307 - 309, Update
the monotonic side-effect sequence in the timeout test to derive its
post-deadline value from the production BATCH_POLL_TIMEOUT_SECONDS constant
instead of hard-coding 300, while preserving the existing timing sequence and
timeout behavior.
Source: Coding guidelines
| batch = client.vector_stores.file_batches.create.return_value | ||
| assert batch.id == "vsfb_mock" | ||
| assert batch.file_counts.failed == 0 | ||
| assert batch.file_counts.completed == 2 | ||
| assert client.vector_stores.file_batches.poll.return_value is batch |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
rg -n -C 2 'file_batches\.(retrieve|poll)' backend/app backend/app/testsRepository: ProjectTech4DevAI/kaapi-backend
Length of output: 7300
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== candidate files =="
wc -l backend/app/crud/rag/open_ai.py backend/app/tests/crud/rag/test_open_ai.py
echo "== implementation outline and relevant sections =="
ast-grep outline backend/app/crud/rag/open_ai.py --match OpenAIVectorStoreCrud --view expanded || true
sed -n '1,180p' backend/app/crud/rag/open_ai.py
echo "== test setup and polling test relevant sections =="
sed -n '1,120p' backend/app/tests/crud/rag/test_open_ai.py
sed -n '260,350p' backend/app/tests/crud/rag/test_open_ai.pyRepository: ProjectTech4DevAI/kaapi-backend
Length of output: 14365
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== fixture file =="
sed -n '1,170p' backend/app/tests/utils/openai.py
echo "== usages of get_mock_openai_client_with_vector_store =="
rg -n "get_mock_openai_client_with_vector_store|test_wiring_contract" backend/app/testsRepository: ProjectTech4DevAI/kaapi-backend
Length of output: 6518
Assert the retrieve endpoint in the mock client contract.
get_mock_openai_client_with_vector_store() wires file_batches.poll, but OpenAIVectorStoreCrud._poll_file_batch() calls file_batches.retrieve; poll.return_value is batch can still pass when the mocked method used by production is unconfigured. Set mock_client.vector_stores.file_batches.retrieve.return_value to the same mock_file_batch and assert that connection here.
🤖 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 `@backend/app/tests/crud/rag/test_open_ai.py` around lines 332 - 336, Update
get_mock_openai_client_with_vector_store() to configure
file_batches.retrieve.return_value to the same mock_file_batch used for the
batch contract, then replace the poll.return_value assertion in the test with an
assertion that file_batches.retrieve.return_value is batch.
Issue
Closes #1076
Summary
create()function when listing failed files.Checklist
Before submitting a pull request, please ensure that you mark these task.
fastapi run --reload app/main.pyordocker compose upin the repository root and test.Notes
Please add here if any other information is required for the reviewer.
Original PR description
Issue
Closes #PLEASE_TYPE_ISSUE_NUMBER
Summary
Explain the motivation for making this change. What existing problem does the pull request solve?
Checklist
Before submitting a pull request, please ensure that you mark these task.
fastapi run --reload app/main.pyordocker compose upin the repository root and test.Summary by CodeRabbit
.xlsand.xlsxspreadsheets.