Repository navigation
fix(mcp): align toolsets and harden AI refresh contracts - #382
Conversation
bitflicker64
left a comment
bitflicker64
left a comment
There was a problem hiding this comment.
Blocking: yes. Summary: The toolset rename, the capability probe, the shared login parser and the batched example embeddings behave as described and their tests pass, but the new exact target check rejects the default pairing of MCP (DEFAULT/hugegraph) with an AI service whose GRAPH_SPACE is unset, so a refresh that works on main returns VALIDATION_ERROR after this change. Evidence: read the full diff 9922568..bdf7654 (37 files) with the surrounding code in thin_api.py, configs_block.py, rag_api.py, hugegraph_ai_client.py, config.py and pyhugegraph/utils/huge_config.py; reproduced the rejection end to end with the production router on one side and refresh_vid_embeddings(confirm=True) on the other (macOS, Python 3.10); ran pytest hugegraph-mcp -m "not live and not integration and not llm" (861 passed, 17 deselected) and pytest hugegraph-llm/src/tests/api/test_thin_api.py hugegraph-llm/src/tests/operators/index_op/test_gremlin_example_index_query.py (55 passed); confirmed hugegraph-mcp 1.7.1 is published on PyPI, which the updated docs rely on; all 28 checks at this head are green.
| if supplied_fields and ( | ||
| supplied_fields != expected_fields | ||
| or req.expected_graph != huge_settings.graph_name | ||
| or req.expected_graphspace != huge_settings.graph_space |
There was a problem hiding this comment.
Important: with both sides on their defaults, refresh_vid_embeddings_tool(confirm=true) is now rejected even though MCP and the AI service address the same graph.
MCP always sends a non-empty graphspace: hugegraph_mcp/config.py turns an unset or blank value into "DEFAULT", and refresh_vid_embeddings.py:79 forwards it as expected_graphspace. The AI side defaults to graph_space: Optional[str] = None (config/hugegraph_config.py:31), and this line compares the two with !=, so "DEFAULT" != None returns VALIDATION_ERROR. A blank Graph Space box in the UI stores "" through apply_graph_config and fails the same way.
On HugeGraph 1.7.0+ the two values name the same target: with graph_space unset, HGraphConfig.__post_init__ (pyhugegraph/utils/huge_config.py:73-77) sets self.graphspace = "DEFAULT", so the flow that runs after this check already works on DEFAULT/hugegraph.
Measured at bdf7654 (macOS, Python 3.10) with create_api_router() served with login and thin writes enabled, a mocked scheduler and no GRAPH_SPACE, and refresh_vid_embeddings(confirm=True) called from an MCP process with no graph variables set:
AI graph_name='hugegraph' graph_space=None
MCP graph='hugegraph' graphspace='DEFAULT'
"type": "VALIDATION_ERROR",
"message": "Expected graph and graphspace must match the configured HugeGraph target."
A POST /vid-embeddings/refresh with {}, which is what MCP sends on main, was accepted by the same server, and the MCP call succeeded once the AI side ran with graph_space="DEFAULT". So a deployment where refresh works today stops working after the upgrade until the operator edits the AI configuration, and the error does not say which side to change. The documented workaround also has a side effect: GRAPH_SPACE=DEFAULT moves the example index folder from hugegraph/ to DEFAULT_hugegraph/ (get_index_folder_name, used at text2gremlin_block.py:85).
Please normalise both sides before comparing, for example:
configured_space = (huge_settings.graph_space or "").strip() or "DEFAULT"
expected_space = (req.expected_graphspace or "").strip() or "DEFAULT"keeping the rule that both fields must be supplied. All nine rows of test_vid_embeddings_refresh_validates_target_before_flow still pass with that; please add (None, {"expected_graph": "target", "expected_graphspace": "DEFAULT"}, True). The request-model comment at rag_requests.py:189-191, hugegraph-mcp/README.md:244-246 and the zh-CN counterpart describe the exact-match rule and need the same update.
There was a problem hiding this comment.
Confirmed by independent source tracing: MCP normalizes missing graphspace to DEFAULT, while AI settings can remain None or blank and the new comparison rejects the equivalent target. Normalize the effective identity without changing the existing index-path convention, and cover unset/blank/explicit DEFAULT as well as genuinely different non-default spaces.
| break | ||
| if not line.strip(): | ||
| continue | ||
| request = json.loads(line) |
There was a problem hiding this comment.
Minor: one malformed input line ends the interactive session with a traceback. json.loads(line) is not guarded, and 12 of the 20 json blocks in docs/integration.md are printed across several lines, so pasting one as shown fails on its first line ({).
Measured at bdf7654 with the GraphFixture from tests/distribution_smoke.py as the server: feeding the section 2 block as written to stdio_check.py --interactive exits 1 with json.decoder.JSONDecodeError: Expecting property name enclosed in double quotes: line 1 column 2. The checklist does say to paste each block as one line, but a slip in the middle of the write steps drops the session.
Please catch json.JSONDecodeError (and a missing name) in the interactive branch, print the error and continue. Printing the checklist blocks on one line would also remove the manual step.
imbajin
left a comment
imbajin
left a comment
There was a problem hiding this comment.
Blocking: yes. Summary: Refresh validation can still target the wrong HugeGraph instance, and a UI graph switch can validate the new target while reusing a pipeline bound to the previous graph. Evidence: exact-head source trace through MCPConfig, the thin API, Scheduler, and the initialized refresh nodes.
| ai_result = post( | ||
| "/vid-embeddings/refresh", | ||
| cfg=cfg, | ||
| json={"expected_graph": cfg.graph, "expected_graphspace": cfg.graphspace}, |
There was a problem hiding this comment.
There was a problem hiding this comment.
The instance-identity limitation is real, but the impact should be stated precisely. This flow reads IDs/schema from HugeGraph and updates the configured VID vector index; it does not write graph records. If the URLs identify different instances with the same graph/graphspace names, it can read the unintended instance and rebuild the vector index from that data.
The independent URLs already existed on the base branch, and this PR documents the remaining operator responsibility. Keep this as an explicit safety boundary and follow-up design requirement, rather than describing it as a newly introduced graph-database write regression.
| "Expected graph and graphspace must match the configured HugeGraph target.", | ||
| suggestion="Provide both expected_graph and expected_graphspace matching the AI service target.", | ||
| ) | ||
| return _wrap_flow_call(FlowName.UPDATE_VID_EMBEDDINGS) |
There was a problem hiding this comment.
Scheduler.schedule_flow reuses its initialized refresh pipeline after the first run. Its fetch client and VID index capture graph A during pipeline.init(); after the UI switches settings to B, a B request can pass validation and still refresh A. Please invalidate or rebuild the cached refresh pipeline when graph configuration changes, or fail closed until it is rebuilt. Evidence: Scheduler.schedule_flow stores and fetches the pipeline; FetchGraphDataNode.node_init() binds get_hg_client(), and BuildSemanticIndex binds huge_settings.graph_name during initialization.
There was a problem hiding this comment.
Correction: the cached-pipeline conclusion above is incorrect. BaseNode.run() calls node_init() on every execution. FetchGraphDataNode rebuilds its client/operator there, and BuildSemanticIndexNode rebuilds its index operator. The pinned pycgraph runtime dispatches the Python run override on each pipeline run.
Pipeline reuse therefore does not itself retain graph A's client/index after switching to B. Please withdraw this particular critical blocker; pipeline invalidation is not justified solely by this argument. This does not dismiss the separate default-GraphSpace compatibility issue or physical-instance identity limitation.
imbajin
left a comment
imbajin
left a comment
There was a problem hiding this comment.
Independent cross-review at bdf7654: changes recommended; 7/10.
The startup toolset snapshot, shared login parser, configuration lock, and ten-item sequential embedding batches are sound improvements. The existing default-GraphSpace compatibility regression remains valid. Two additional P2 findings concern mixed-version capability/write routing and lost LiteLLM batch retries; see the proposed inline comments below.
The earlier cached-pipeline blocker should be withdrawn: the real node lifecycle rebuilds graph-bound operators on every execution. The physical-instance identity limitation remains, but it predates this PR; its impact is reading the wrong graph and rebuilding the VID vector index from that graph, not directly mutating the graph database.
Evidence: source review and original CI logs at tested merge da9e3d3d003f1ef5ab58a5482209799aeb3b17f3, with current head/base parents. LLM Python 3.10: 464 passed, 3 skipped, 18 deselected; MCP Python 3.10: 861 passed, 17 deselected. These are upstream CI results, not an independent rerun or proof of live-provider/mixed-version behavior.
| ai_result = post( | ||
| "/vid-embeddings/refresh", | ||
| cfg=cfg, | ||
| json={"expected_graph": cfg.graph, "expected_graphspace": cfg.graphspace}, |
There was a problem hiding this comment.
[P2] Make target validation inseparable from the refresh endpoint
During a rolling AI-service upgrade, the capability GET can reach a new replica while this POST reaches an old replica. The old /vid-embeddings/refresh accepts an empty Pydantic model, ignores expected_graph / expected_graphspace, and dispatches the refresh without validating either. A mismatched target can therefore be refreshed after a successful capability probe.
The included Helm Deployment/Service permits mixed replicas during rolling updates, and these calls use separate HTTP requests. Please use a versioned validated-refresh POST route that old replicas cannot execute, and add a mixed-version routing regression. Checking only the response would detect the mismatch after the index update.
This is a source-confirmed gap in the new fail-closed compatibility guarantee; no mixed-version deployment was independently run.
Sources: new handshake, old request model, old dispatch, deployment.
| embeddings = [] | ||
| for start in range(0, len(properties), 10): | ||
| texts = [row["query"] for row in properties[start : start + 10]] | ||
| embeddings.extend(self.embedding.get_texts_embeddings(texts)) |
There was a problem hiding this comment.
[P2] Preserve LiteLLM retries when switching the example builder to batches
This changes the cold example-index path from LiteLLMEmbedding.get_text_embedding, which retries RateLimitError / APIConnectionError / APIError up to three times with backoff, to get_texts_embeddings, which logs and immediately re-raises those errors. A transient 429 can now abort example-index construction and the Text2Gremlin request where the previous path could recover.
This is reachable with LiteLLM's hosted_vllm embedding provider: version 1.61.20 installed in CI performs one HTTP POST, maps 429 to RateLimitError, and supplies no embedding-level retry on that path. OpenAI's separate SDK retry behavior does not cover this provider.
Please retain bounded retries per batch and add a deterministic 429-then-success regression, preserving the ten-text limit, ordering, and the existing no-save-on-terminal-failure guarantee. This finding is source-derived, not a live-provider reproduction.
Sources: changed call, repository retry implementations, provider dispatch, single-request handler, 429 mapping.
Normalize default GraphSpace identity and require the versioned validated refresh route without legacy fallback. Restore bounded retries per embedding batch, reject mismatched batch counts before indexing, and keep malformed interactive STDIO input recoverable. Add regressions and update refresh documentation.
3 participants
MCP refresh can use a different AI-service graph from the MCP target, while the toolset names and configuration guidance blur released and source behavior. In addition, login parsing differs between authentication and the Thin API write gate, and UI configuration reload can change the target during a refresh.
This PR:
coreandlegacy, retaining warning aliases forv2_coreandv1. Toolset registration and metadata use one startup snapshot; invalid values prevent startup.HUGEGRAPH_AI_GRAPH_URLoverride. Refresh probes the AI target-validation capability, sends the expected graph and graphspace, and rejects mismatches before execution. Failed probes never dispatch refresh; authentication and transient errors retain their classification and retryability.1.7.1from current source behavior.Validation
git diff --check: passed.The new concurrency and provider-limit regressions use a controlled scheduler/provider boundary. The earlier live AI acceptance used isolated indexes and containers, with lifespan disabled to test HTTP gates independently; it does not validate startup background refresh gating. Database-instance identity remains a deployment responsibility when graph and graphspace names match. This PR does not publish packages or deploy services.
Synchronized submission
Paired organization PR: hugegraph#81. Both PRs use the same source commit and retain their target repository's CI configuration.
Companion website documentation: apache/hugegraph-doc#520 (organization counterpart: hugegraph/hugegraph-doc#46). The source-only examples in the guide depend on this implementation; published-package behavior is documented separately.
Implementation overview
MCP refresh contracts and sequential embedding batches.