fix(llm): guard VolcEngine/Ark key against JSON non-object - #17457
Open
Harsh23Kashyap wants to merge 1398 commits into
Open
fix(llm): guard VolcEngine/Ark key against JSON non-object#17457Harsh23Kashyap wants to merge 1398 commits into
Harsh23Kashyap wants to merge 1398 commits into
Conversation
### Summary Move stats to a specific service from system --------- Signed-off-by: Jin Hai <haijin.chn@gmail.com>
### Summary ``` RAGFlow(admin)> STATS USERS TOP 5 FROM '2026-01-01' TO '2026-02-01'; +-----------------------+----------------------------------------------+------------+---------------------+-----+ | command | error | from_date | to_date | top | +-----------------------+----------------------------------------------+------------+---------------------+-----+ | get_token_users_stats | 'Get API token users stats' is not supported | 2026-01-01 | 2026-02-01 23:59:59 | 5 | +-----------------------+----------------------------------------------+------------+---------------------+-----+ RAGFlow(admin)> STATS USER 'aaa@aaa.com' FROM '2026-01-01' TO '2026-02-01' MONTH; +-----------------+----------------------------------------+------------+-------------+---------------------+-------------+ | command | error | from_date | granularity | to_date | user_name | +-----------------+----------------------------------------+------------+-------------+---------------------+-------------+ | get_token_stats | 'Get API token stats' is not supported | 2026-01-01 | month | 2026-02-01 23:59:59 | aaa@aaa.com | +-----------------+----------------------------------------+------------+-------------+---------------------+-------------+ RAGFlow(admin)> STATS SUMMARY FROM '2026-01-01' TO '2026-02-01' MONTH; +-----------+------------------------------------------------+ | field | value | +-----------+------------------------------------------------+ | to_date | 2026-02-01 23:59:59 | | command | get_token_stats_summary | | error | 'Get API token stats summary' is not supported | | from_date | 2026-01-01 | +-----------+------------------------------------------------+ ``` --------- Signed-off-by: Jin Hai <haijin.chn@gmail.com>
### Summary Bump to infinity v0.7.2 Infinity image: infiniflow/infinity:v0.7.2-x64-v3
### Summary As title. Signed-off-by: Jin Hai <haijin.chn@gmail.com>
As title. Signed-off-by: Jin Hai <haijin.chn@gmail.com>
### Summary As title
## Summary - Enable synchronous and streaming tool calls for Aliyun models. - Preserve provider-specific chat endpoints and prevent repeated qwen-flash tool calls. - Restrict retrieval tool inputs to model-provided query parameters. ## Testing - `bash build.sh --test ./internal/entity/models ./internal/agent/component ./internal/agent/tool` - Manual frontend UI testing passed.
### Summary 1. fix the issue of ci failures under specific circumstances. 2. fix ci port allocated 3. modified unit_test file only run on python
### Summary 1. Fix docker/service_conf.yaml.template 2. Remove unused config --------- Signed-off-by: Jin Hai <haijin.chn@gmail.com>
…7064) ### Summary `agent/tools/github.py` indexes `response["items"]` directly after `requests.get(...).json()`. the github search api returns `{"message": ...}` **without** an `items` key on realistic conditions — a rate limit (403/429; this tool sends no auth token, so the unauthenticated ~10 req/min search limit is easy to hit) or an invalid query (422). that raised `KeyError('items')`, which the tool's retry loop then surfaced to the model as the opaque `"GitHub error: 'items'"` instead of the real reason. this guards the missing key and raises the api's actual `message` (with a clear fallback) into the existing retry/`_ERROR` path, so the model sees e.g. `"GitHub error: API rate limit exceeded ..."`. valid responses are unchanged. adds `test/unit_test/agent/tools/test_github_unit.py` covering the rate-limit response (asserts the real message is surfaced, no `KeyError`) and the normal result path. Co-authored-by: Yaroslav98214 <diakovichyaroslav30@gmail.com> Co-authored-by: Haruko386 <tryeverypossible@163.com>
### Summary Feat: Render the skills list using a tree view.
### Summary As title --------- Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
### Summary As title
…ion and add PPT parsing support (infiniflow#17111) ## Summary Align the Go ingestion pipeline with Python's `image` → `img_id` persistence semantics, and unify the chunk-id computation across all paths. ### Changes **1. Image upload at chunker stage ** - Add `ImageUploader` type and `DefaultImageUploader` in `internal/ingestion/component/image_uploader.go` — the write-side counterpart to `FetchBinary`, storing raw image bytes at `(bucket=kbID, key=chunkID)`, no re-encoding. - Add `uploadOneImage` — pure upload primitive (bytes in, `img_id` out), does not touch chunk maps. - Add `uploadChunkImages` / `uploadChunkImage` — caller-side helper: decodes `image` from a chunk, uploads bytes, writes `ck["img_id"]`, `delete(ck,"image")` , bounded by a process-wide semaphore (default 10, env `MAX_CONCURRENT_MINIO`). - Wire via `imageUploadDecorator` in `register.go`: every chunker runs the upload pass at invocation time, writing `ck["id"]` before upload and dropping image bytes right after — peak memory = single chunk image lifetime. **2. Unify chunk-id computation** - Consolidate three separate id-computation paths (`component.ChunkID`, `task.ChunkID`, inline `FormatUint` in API) into one: `common.ChunkID(docID, text string)`, using `%016x` + `xxhash.Sum64String(text+docID)` (matching Python `hexdigest()`). - The chunker decorator writes `ck["id"]` via `common.ChunkID`; the persist stage (`ProcessChunksForPipeline`) falls back to the same function (`if !exists id`). - The API AddChunk path now also calls `common.ChunkID` instead of the divergent `FormatUint(xxhash.Sum64(...))` — fixing a pre-existing inconsistency. - Delete `internal/ingestion/component/chunk_id.go` and `internal/ingestion/task/chunk_builder.go` (both were pure forwarding shells). **3. Preserve `img_id` (never deleted)** - `img_id` is a persistent index field (Infinity, OB) and the only consumer-side reference for image retrieval; it is NEVER removed from the chunk map. Only `image` (raw data URL) is dropped after upload. **4. PPT parser support** Previously PPT parsing failed. Add support to parse. ### Key design decisions | Decision | Choice | |----------|--------| | Upload timing | Chunker stage (not persist), so image bytes are dropped immediately — bounds peak memory to one chunk image | | Upload concurrency | Process-wide semaphore, default 10 (matches Python `minio_limiter`), env `MAX_CONCURRENT_MINIO` | | Image encoding | Store as-is, no JPEG re-encoding (unlike Python) | | `img_id` format | `"<kb_id>-<chunk_id>"` — matches Python task_executor path | | id function | Single `common.ChunkID(docID, text)`, concatenation `text+docID` inside hash (matching Python) | | `removeInternalChunkFields` | Retains `delete(ck,"image")` as defensive fallback for non-chunker paths | ### Files touched | File | Change | |------|--------| | `internal/common/format.go` | Add `ChunkID(docID, text)` | | `internal/common/format_test.go` | Add ChunkID golden-value test | | `internal/ingestion/component/image_uploader.go` | Add `ImageUploader` type + `DefaultImageUploader` | | `internal/ingestion/component/chunker/image_upload.go` | Add `uploadOneImage`, `uploadChunkImages`, `uploadChunkImage`, `decodeChunkImage`, semaphore | | `internal/ingestion/component/chunker/image_upload_test.go` | Tests: upload/drop, skip, no-image, concurrency, missing-id error | | `internal/ingestion/component/chunker/register.go` | Add `imageUploadDecorator` (writes `ck["id"]`, runs upload) | | `internal/ingestion/task/chunk_process.go` | Use `common.ChunkID` for persist fallback | | `internal/service/chunk/chunk.go` | Use `common.ChunkID` instead of `FormatUint` | | `internal/ingestion/component/chunk_id.go` | **Deleted** (moved to `common`) | | `internal/ingestion/task/chunk_builder.go` | **Deleted** (shell, no callers left) | | `internal/ingestion/task/chunk_builder_test.go` | **Deleted** (test migrated to `common/format_test.go`) | ### Verification ``` bash build.sh --test ./internal/service/chunk/... ./internal/common/... ./internal/ingestion/component/... ./internal/ingestion/task/... → ok service/chunk / common / component / chunker / schema / task ```
…/1.8/2.6/2.7, Tokenizer 6x fixes) (infiniflow#17419) ## Summary Continuation of the Python→Go ingestion pipeline migration (File → Parser → Chunker → Extractor → Tokenizer). Fixes cover Parser, Chunker, and Tokenizer gaps identified. Fix page number (0-indexed and 1-index mixed before fix; use 1-indexed after fix) and chunk order issues. ### Parser - **Slides TCADP (1.7):** `pptx_tcadp.go` + TCADP branch in `pptx_parser.go`/`ppt_parser.go` — PowerPoint files now support `parse_method="tcadp"` via the TCADP cloud service, matching the spreadsheet-family TCADP pattern. PPT containers pass `"PPT"` as fileType (not hardcoded `"PPTX"`). - **Audio default output_format (2.11):** `defaultSetups()` audio default changed from `"text"` to `"json"`, aligning with Python `parser.py:232` and `AllowedOutputFormat["audio"]={"json"}`. - **PDF VLM enhancement (1.1):** `maybeDispatchPDFVisionEnhancement` in `pdf_vision_dispatch.go` enriches image/table items with IMAGE2TEXT model descriptions after PDF parsing, mirroring Python `enhance_media_sections_with_vision`. Semaphore fix: acquire before goroutine start to prevent unbounded goroutine creation. - **json family (2.3):** reclassified as Keep Go — `json_parser.go` is a functional enhancement, not a parity gap. - **page number:** changed from "mixed use of 1-indexed & 0-indexed" to "1-indexed" ### Chunker - **BULLET_PATTERN fallback (1.7):** 4th-level fallback in `resolveTitleLevels` (`title.go`) detects bullet/numbered-list patterns (Chinese legal, numbering, English) when outline + regex levels produce only bodyLevel. Guarded by `allBodyLevel` to never override existing structure. - **Tag/One chunker fields (1.8):** `tag.go` sets `TopInt` from source row index; `one.go` preserves `Positions`/`PDFPositions` from source items. TSV multi-line RowNum fix: tracks `contentStart` for correct row attribution. - **Overlapped_percent normalization (2.6):** `NormalizeOverlappedPercent` in `schema/chunker.go` mirrors Python `common/float_utils.py:50-58` — accepts `[0,1)` fraction or `[0,90]` percent, normalizes to canonical `[0,90]`. - **Paragraph splitting (2.7):** aligned to Python flow `naive_merge` — `CRLF` normalization, `splitKeepingDelimiter` preserves sentence delimiters, single-section merge with token-budget-governed chunking. - **chunk order:** sort by reading order ### Tokenizer - **Phantom chunk filtering (Omission 2):** `isPhantomChunk` + filter loop in `chunksFromTokenizerUpstream` skips zero-value ChunkDocs. - **Batch size env var (Omission 3):** `embeddingBatchSize()` reads `TOKENIZER_EMBEDDING_BATCH_SIZE`, defaults to 16. - **Summary empty check (Diff 5):** `TrimSpace(s) != ""` → `s != ""`, matching Python truthy check. - **chunk_order_int all paths (Diff 8):** set unconditionally before full_text/embedding branching. - **Timeout default (Diff 10):** `600s` → `60s`, matching Python `@timeout(60)`. - **Small maxTokens truncation (Diff 14):** `truncateForEmbedding` returns `""` when `maxTokens <= 10`, matching Python. ### Code review fixes - Semaphore acquire moved before goroutine in `pdf_vision_dispatch.go` (concurrency control) - Context propagation in `pptx_tcadp.go` (cancellation support) - Test resolver leak fix in `media_dispatch_test.go` (defer restore) - Migration history comments removed per AGENTS.md ## Test plan ``` bash build.sh --test ./internal/parser/parser/... ./internal/ingestion/component/... ``` ## Notes - Migration diff tracking: `docs/migration_python_go_diff.md` - Remaining gaps: Extractor component only (21 items)
…w#17456) The VolcEngine/Ark provider in rag/llm has three call sites that do an unguarded `json.loads(key).get("ark_api_key", "")` inside a `try: ... except JSONDecodeError:` block: - VolcEngineChat.__init__ (chat) -- chat_model.py:975 - VolcEngineCV.__init__ (vision) -- cv_model.py:621 - VolcEngineEmbed.__init__ (embedding) -- embedding_model.py:1100 The `except JSONDecodeError` only catches the *parse* failure. A user pasting a JSON string that is NOT an object -- e.g. `"[1,2,3]"`, `"42"", `'"hello"'`, `"true"", `"null"` -- parses fine and then crashes on the `.get(...)` call with `AttributeError: 'list' object has no attribute 'get'` (or `'int'`, `'str'`, `'NoneType'`, `'bool'`) from inside rag/llm internals -- no indication of what the user did wrong. Fix: add `_resolve_volcengine_credentials` to rag/llm/key_utils, then wire the three call sites through it. The helper: - Accepts a plain (non-JSON) string and returns `{"ark_api_key": key, "model_name": None}` -- matching the pre-fix `except JSONDecodeError` fallback semantics. - Accepts a dict (returned with `ark_api_key` and an optionally derived `model_name` from `ep_id` + `endpoint_id`). - On a JSON top-level type that is not a dict (list, string, number, bool, null), raises a clear `ModelException(retryable=False)` naming the required object shape and pointing at `conf/models/volcengine.json`. Operators can self-diagnose without opening an issue. Behavior unchanged for the existing happy paths: - Plain key -> `ark_api_key = key, model_name = passed parameter` - JSON dict with `ark_api_key` -> `ark_api_key = parsed.ark_api_key, model_name = ep_id + endpoint_id` - JSON dict missing `ep_id`/`endpoint_id` -> model_name falls back to the parameter (unchanged) - Pre-existing `ark_api_key`-missing behavior unchanged. No public API change. No data-model change. No migration. Fixes infiniflow#17456.
…iniflow#17456) 28 p0 tests in test/unit_test/rag/llm/test_volcengine_json_key_fallback.py: - TestResolveVolcengineCredentials (16 tests on the helper directly): plain string key passes through as ark_api_key, empty string falls through, JSON dict with all fields extracts correctly, JSON dict missing ark_api_key or ep_id/endpoint_id is tolerated (matches pre-fix behavior for partial JSON), Python dict passes through, JSON top-level non-object types (array, string, number, float, null, bool) all raise a clear ModelException naming the type, non-string non-dict input raises, malformed JSON falls through as plain key. - TestVolcEngineChatCallSite (4 tests on the chat call site): plain key constructs with the key as api_key and the passed model_name preserved, JSON dict constructs with the derived model_name, JSON array raises ModelException, JSON number raises ModelException. - TestVolcEngineCVCallSite (4 tests on the vision call site): same shape as the chat site. - TestVolcEngineEmbedCallSite (4 tests on the embedding call site): plain key constructs with key as ark_api_key, JSON dict constructs with parsed ark_api_key, JSON array raises ModelException, JSON number raises ModelException. All 28 pass against the fix. The OpenAI/AsyncOpenAI constructors are patched at the import site (rag.llm.chat_model / rag.llm.cv_model) via `side_effect` to avoid real Ark/Voyage round-trips while still capturing the constructor kwargs (in particular `api_key`) for assertion. Fixes infiniflow#17456.
dosubot
Bot
added
the
size:L
Contributor
|
Warning Review limit reached
Next review available in: 9 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughVolcEngine credential parsing is centralized in a shared resolver and integrated into chat, CV, and embedding models. Regression tests cover key formats, model-name derivation, invalid JSON shapes, and API-key propagation. ChangesVolcEngine credential handling
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related issues
Suggested labels: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
rag/llm/key_utils.py (1)
37-70: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTrim the "pre-fix" compatibility narrative from the docstring.
The docstring spends most of its length explaining the old buggy behavior and repeatedly ties the new contract to "matching the pre-fix fallback semantics." As per coding guidelines,
**/*instructs: "Remove compatibility-only surfaces unless the user explicitly asks to keep them, and do not add compatibility wording to comments or documentation." Consider keeping only the current contract (accepted shapes, return value, error behavior) and dropping the historical comparison; the "why" belongs in the PR/commit message, not in code that will outlive the migration context.🤖 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 `@rag/llm/key_utils.py` around lines 37 - 70, Trim the docstring for the key-resolution helper to describe only its current contract: accepted plain strings and JSON objects, returned fields, and ModelException behavior for unsupported inputs. Remove the “Pre-fix” narrative, call-site history, bug explanation, and references to matching historical fallback semantics while preserving the relevant VolcEngine/Ark schema details.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 `@rag/llm/key_utils.py`:
- Around line 82-97: The new credential-validation branches in the key-parsing
flow need logging before failure. Add a logger.warning or logger.error
immediately before each ModelException for invalid key types and non-object JSON
payloads, including the relevant type/schema context, while preserving the
existing exception messages and retryable=False behavior.
- Around line 99-101: Update the model_name construction in the key-parsing
function to safely handle non-string or null ep_id and endpoint_id values
without raising TypeError. Normalize each payload value before concatenation,
preserving the existing combined-name behavior for valid strings and returning
None when no usable name is present.
---
Nitpick comments:
In `@rag/llm/key_utils.py`:
- Around line 37-70: Trim the docstring for the key-resolution helper to
describe only its current contract: accepted plain strings and JSON objects,
returned fields, and ModelException behavior for unsupported inputs. Remove the
“Pre-fix” narrative, call-site history, bug explanation, and references to
matching historical fallback semantics while preserving the relevant
VolcEngine/Ark schema details.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: d6269b86-badb-4796-a1ae-dbf32561543e
📒 Files selected for processing (5)
rag/llm/chat_model.pyrag/llm/cv_model.pyrag/llm/embedding_model.pyrag/llm/key_utils.pytest/unit_test/rag/llm/test_volcengine_json_key_fallback.py
This was referenced Jul 28, 2026
Two follow-ups from the CodeRabbit review on the VolcEngine key
fallback PR:
1. Add `logging.error(...)` before each `ModelException` raise in
`_resolve_volcengine_credentials` (unsupported key type, JSON
top-level not an object) so malformed inputs are visible in
server logs, matching the ragflow "Add logging for new flows"
guideline and the sibling Bedrock/BaiduYiyan/OpenRouter
resolvers. Added `import logging` next to the existing
`import json`.
2. The `model_name` derivation concatenated
`payload.get("ep_id", "") + payload.get("endpoint_id", "")`
without a `str()` cast, so a JSON number (e.g. `{"ep_id": 12345}`)
or explicit `null` would raise `TypeError: can only concatenate
str (not "int") to str`. Coerce both via `str(...) if x is not None
else ""` so the helper always returns a string model_name.
Added `test_json_dict_with_non_string_ep_id_does_not_raise` to
cover the int case for both ep_id and endpoint_id.
ruff check + ruff format clean.
Follow-up to the previous commit: the new test body needed an extra blank line for the two test cases inside one method to match the project ruff format.
Contributor
Author
|
Opened #17457 for the same |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@test/unit_test/rag/llm/test_volcengine_json_key_fallback.py`:
- Around line 126-134: Extend
test_json_dict_with_non_string_ep_id_does_not_raise to cover JSON null
identifiers for both ep_id and endpoint_id, asserting each resolves without
raising and produces the expected model_name string alongside the API key.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 05685d5c-48f5-4798-80b5-3c5aed398e43
📒 Files selected for processing (2)
rag/llm/key_utils.pytest/unit_test/rag/llm/test_volcengine_json_key_fallback.py
🚧 Files skipped from review as they are similar to previous changes (1)
- rag/llm/key_utils.py
This was referenced Jul 29, 2026
CodeRabbit finding on infiniflow#17457: the test claimed explicit JSON null was supported but only exercised numeric identifiers. Added two regression tests that pin down the coercion behavior: - test_json_dict_with_null_ep_id_returns_null_model_name ep_id=None, no endpoint_id -> model_name=None (both coerced to "", empty concat becomes None via the "or None" fallback at end of helper) - test_json_dict_with_null_endpoint_id_falls_back_to_ep_id ep_id="ep-abc", endpoint_id=None -> model_name="ep-abc" (null endpoint_id coerced to "", dropped from concat) 31 tests total in test_volcengine_json_key_fallback.py, all pass; ruff check + ruff format --check both clean.
This was referenced Aug 13, 2026
Harsh23Kashyap
added a commit
to Harsh23Kashyap/ragflow
that referenced
this pull request
Aug 20, 2026
…ugh shared JSON-decode helpers The model-list / verify path in rag/llm/model_meta.py has 3 provider classes (VolcEngine, OpenRouter, NewAPI) that all share the same JSON-decode silent-fallback bug pattern already fixed elsewhere in the codebase by the chat / CV / embed providers of the same factories (cycles 7-16, PRs infiniflow#17457 / infiniflow#17459 / infiniflow#17681 / infiniflow#17687). The model-meta path was missed by those cycles because it runs from a different code path (the LLM factory UI's "verify" button) than the chat / CV / embed init paths. Pre-fix behavior (3 different shapes for the same bug): - VolcEngine._get_api_key: json.loads(self.api_key).get("ark_api_key", "") inside try/except JSONDecodeError. The .get(...) call crashed with AttributeError: 'list' object has no attribute 'get' when the user pasted a JSON non-object, and TypeError from json.loads(None) slipped through the except JSONDecodeError guard, surfacing as a 500 to the LLM verify endpoint. - OpenRouter._get_api_key: json.loads(api_key) inside try/except Exception (too broad), then isinstance(payload, dict) check. The except Exception silently swallowed TypeError from json.loads(None) and returned the raw JSON string for non-dict JSON input, which then 401s at the upstream API with a less-actionable error. - NewAPI._get_api_key: json.loads(self.api_key) inside try/except (JSONDecodeError, TypeError), then isinstance(parsed, dict) check. A JSON non-object silently returned the raw JSON string as the api_key, which then 401s. The fix: re-add the established helpers from the open PRs in rag/llm/key_utils.py (overlap with infiniflow#17457 and infiniflow#17459 documented so the maintainer can drop the duplicate on rebase) and wire all 3 model_meta.py sites through them. - _resolve_volcengine_credentials(key): accepts plain string OR JSON dict, returns {"ark_api_key": str, "model_name": str | None}, raises ModelException(retryable=False) on JSON non-object. - _resolve_openrouter_credentials(key): accepts plain string OR JSON dict, returns {"api_key": str, "provider_order": str}, raises ModelException(retryable=False) on JSON non-object. NewAPI reuses this helper because the NewAPI model_meta.py site has the same shape (plain string OR JSON dict with api_key). - The OpenRouter model_meta.py site keeps the historical if not api_key: return "" early return and the payload.get("api_key") or api_key fallback for a missing api_key field in a JSON dict, so operators who pasted a bare "sk-..." key and have working configs see no change. Closes infiniflow#18250.
Contributor
|
@Harsh23Kashyap would you please resolve the conflicts? |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
20 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes #17456. The VolcEngine/Ark provider in
rag/llm/parses its API key as JSON, looking forark_api_key,ep_id, andendpoint_id. The three call sites had a partialtry: ... except JSONDecodeError:block that only catches the parse failure. A user pasting a JSON string that parses but is not an object (e.g."[1,2,3]","42",'"hi"',"true","null") would crash on the subsequent.get(...)call withAttributeError: 'list' object has no attribute 'get'from insiderag/llminternals — no indication of what the user did wrong.This is the same class of bug PR #17215 fixed for Azure, PR #17377 fixed for Bedrock, and PR #17390 fixed for BaiduYiyan. VolcEngine was the last unfixed LLM-provider JSON-decode site in
rag/llm/.Changes
1 helper, 3 call sites, 1 test file:
rag/llm/key_utils.py— add_resolve_volcengine_credentials(key). Returns a dict{"ark_api_key": str, "model_name": str | None}.{"ark_api_key": key, "model_name": None}so the caller keeps themodel_nameparameter it was passed in__init__(matches the pre-fixexcept JSONDecodeErrorbranch).{"ark_api_key": payload["ark_api_key"], "model_name": payload["ep_id"] + payload["endpoint_id"]}(both default to"";model_nameisNonewhen both are missing).ModelException(retryable=False)naming the actual type and pointing atconf/models/volcengine.json.rag/llm/chat_model.py—VolcEngineChat.__init__calls the helper instead ofjson.loads(key).get(...)× 3.rag/llm/cv_model.py—VolcEngineCV.__init__calls the helper.rag/llm/embedding_model.py—VolcEngineEmbed.__init__calls the helper.test/unit_test/rag/llm/test_volcengine_json_key_fallback.py— 28 p0 regression tests:Behavior unchanged for the existing happy paths
ark_api_key = key, model_name = passed parameterark_api_key→ark_api_key = parsed.ark_api_key, model_name = ep_id + endpoint_idep_id/endpoint_id→model_namefalls back to the parameter (unchanged)ark_api_key-missing behavior unchanged.No public API change. No data-model change. No migration.
Testing
Plus the existing 14
test_embedding_model.py+ 23test_bedrock_rerank.pytests pass unchanged (65 / 65 in the combined run).Cross-references
rag/llm/crash withAttributeErroron JSON non-object keys. The only places left inrag/llm/with unguardedjson.loads(key)are insideLiteLLMBasefor providers that pass through litellm (which is fine — litellm handles its own key parsing).