Repository navigation
Conversation
bitflicker64
left a comment
There was a problem hiding this comment.
Blocking: yes. Summary: The 1.7.1 parts of both guides (tool names and counts, toolset names, configuration keys and defaults, write gates, install command) match the MCP source, the EN and CN pages agree, and the site builds with all links resolving, but the sections labelled current source describe apache/hugegraph-ai#382, which is still open and has two reviews marked Blocking: yes on the target check this page tells readers to configure for. Evidence: read the full diff at 7bcd3a6 (5 files, +461/-0) and both pages line by line; compared every tool, environment variable, default and gate with hugegraph_mcp/server.py, config.py, hugegraph_ai_client.py, tools/refresh_vid_embeddings.py, tools/inspect_graph.py, tools/query_graph_data.py and hugegraph_llm/api/thin_api.py on apache/hugegraph-ai main at 8a7c186 and at the #382 head bdf7654; confirmed hugegraph-mcp 1.7.1 on PyPI (requires_python >=3.10, uploaded 2026-09-26); ran sh scripts/hugo.sh build at the head in a temporary worktree (Hugo 0.166.0 extended, exit 0, no warnings, site validation passed) and checked the rendered hrefs for the new pages; data/docs_nav.json parses and its four new entries are consistent. The PR reports no CI checks at this head.
imbajin
left a comment
There was a problem hiding this comment.
Blocking: yes. Summary: The navigation update breaks the latest and historical version build checks, and the published configuration table omits two graph-path overrides. Evidence: exact-head Build and deploy site run 38222905036; HugeGraph AI 1.7.1 config.py at 8189b96.
bitflicker64
left a comment
There was a problem hiding this comment.
Blocking: yes. Summary: The 1.7.1 parts of both guides match the MCP source, the regenerated DOCS_NAV_EXPECTED_STATS for latest are right, and the EN and CN pages agree. Two problems remain. The new page has no entry in data/version_routes.json, so version validation for latest fails. The page also still calls apache/hugegraph-ai#382 unmerged, but it merged on 2026-10-13. Evidence: at b1561ad, sh scripts/hugo.sh build and bash dist/validate-links.sh pass. scripts/versioning.py build --version latest writes docsNavigation = 96 pages / 4 removed / tree 8b2b1f27..., which matches the new baseline. scripts/versioning.py validate --version latest exits 1 with current canonical Docs page is absent from route-map: latest cn/docs/quickstart/hugegraph-ai/hugegraph-mcp/. After adding the two route entries locally, the rebuild and validate pass (333 canonical pages). I compared tools, toolset fallback, config keys and defaults against hugegraph-ai 8189b96 (the 1.7.1 packaging commit) and the proposed section against main 84ee9c4. PyPI lists only 1.7.1 (requires_python >=3.10). The exact-head CI run 39038266293 is action_required and has not run. I could not build the historical versions locally: master tooling fails the same way on release-1.7.0, so this PR does not cause that.
bitflicker64
left a comment
There was a problem hiding this comment.
Blocking: no. Score: 9.5/10 (10 - 0.5 for one Minor). Summary: The two items from my previous review are fixed in 5ec34e7: the new page has cn: and en: entries in data/version_routes.json, and the #382 sections now say merged to main but not released. One stale "proposed" remains in the EN page. Evidence: at 5fc0fe9, python3 scripts/versioning.py build then validate with the CI arguments pass for latest (335 canonical pages) and for every historical version: 1.7 (223), 1.5 (200), 1.3 (194) and 1.0 (191), using Hugo 0.168.0 extended and Dart Sass 1.105.2 as pinned in hugo.yml. bash dist/validate-links.sh passes. python3 -m unittest discover -s scripts -p 'test_*.py' runs 264 tests; the versioning tests pass, and the only 2 errors are in test_community_roster because dwebp is not installed here. I rechecked the page against hugegraph-ai: 16 tools in the default set and 10 in v1 at 8189b96 (the 1.7.1 packaging commit), the fallback to v1 for unknown values, the config keys and defaults in config.py, requires-python >=3.10, and the hugegraph-mcp console script. On main I checked the core/legacy resolution that fails on empty or unknown values, the refresh-capabilities check for protocol_version == 1, the expected_graph/expected_graphspace validation and the ENABLE_LOGIN plus HUGEGRAPH_LLM_ENABLE_THIN_WRITES gate in thin_api.py. #382 merged on 2026-10-13 as 84ee9c4, and PyPI still lists only 1.7.1. The EN and CN pages have the same headings, environment variables and tool names. At the exact head, Build and deploy site run 39133122298 has passed prepare and Build groups 0-2; the deploy job is still running its blocking Chromium contracts, so this is a comment rather than an approval.
Earlier threads, verified at this head:
- version_routes entries for the new page (my previous review): fixed in 5ec34e7.
- #382 described as unmerged (my previous review): fixed in 5ec34e7, apart from the line noted inline.
- 1.7.1 toolset fallback wording (my first review): fixed in b1561ad, still present.
- refresh protocol labelled current source (my first review): fixed in b1561ad, and the wording is now updated for the merge.
- navigation baselines in
DOCS_NAV_EXPECTED_STATS(@imbajin): fixed in b1561ad. All five versions validate. HUGEGRAPH_GRAPHSPACEandHUGEGRAPH_GRAPHoverrides (@imbajin): fixed in b1561ad, present in both tables.
bitflicker64
left a comment
There was a problem hiding this comment.
Blocking: no. Summary: The 1.7.1 parts of both guides match the MCP source. The navigation, route-map and baseline updates are consistent, and the EN and CN pages agree. One new Minor: the refresh section scopes the HugeGraph-LLM thin-write gate to #382, but the gate predates #382 and applies to the published 1.7.1 MCP. The stale "proposed naming fix" at EN line 103 was already raised on this head and is not repeated. Evidence: read the full diff at 5fc0fe9 (7 files, +482/-5). Compared all 16/10 tool registrations, the toolset fallback, the capability table, config keys and defaults, inspect_graph_tool / query_graph_data_tool arguments and response fields, and the write-plan states with hugegraph-ai 8189b96 (the 1.7.1 packaging commit). Checked the core/legacy resolution, the refresh capability protocol and the thin_api.py gate on hugegraph-ai main (3fad379; #382 merged as 84ee9c4), and the gate's origin with git log -S (aebc6c7, 2026-08-14). Link targets config-authentication.md and hugegraph-mcp/docs/integration.md exist. At this head, Build and deploy site run 39133122298 passed prepare, build groups 0-2 and deploy; the visual job is still pending.
bitflicker64
left a comment
There was a problem hiding this comment.
Blocking: no. Summary: At 0bef461 the four open threads from earlier heads are fixed: the version routes are present, #382 is described as merged to main but not released, the stale "proposed naming fix" is gone, and the thin-write service gates now sit in the refresh requirements that apply to 1.7.1. The 1.7.1 tool set, toolset fallback, configuration keys and defaults, and the #382 capability check all match the source. One Minor: the same commit changed the EN wording from "a strong USER_TOKEN" to "a configured USER_TOKEN", while the CN page still says strong (inline). Evidence: read the full diff df23ce2..0bef461 (7 files, +483/-5) and the incremental diff 5fc0fe9..0bef461. Checked against apache/hugegraph-ai 8189b96 (1.7.1 packaging: server.py 16/10 tools and _active_toolset() fallback, config.py, guard.py, hugegraph_ai_client.py, tools/refresh_vid_embeddings.py, inspect_graph.py, query_graph_data.py, write_plan.py) and main 3fad379 (thin_api.py _thin_write_disabled() and refresh-capabilities, admin_config.py, MCP tools/refresh_vid_embeddings.py). PyPI lists only hugegraph-mcp 1.7.1. Latest docs-nav stats recomputed with materialize_docs_navigation() equal DOCS_NAV_EXPECTED_STATS, load_version_routes() validates, scripts/test_versioning.py passes 85/85, internal link targets exist, and git diff --check is clean. The Hugo build and the historical-version builds were not run here. The site build at this head (run 39312844330) is action_required.
| Merge these entries into the earlier `env` object. Generation and extraction can remain read-only. | ||
| Embedding refresh additionally requires: | ||
| - Client settings: `HUGEGRAPH_MCP_READONLY=false`, `HUGEGRAPH_MCP_ADMIN_MODE=true`, and `confirm=true` in the tool call. | ||
| - Service settings (HugeGraph-LLM): `ENABLE_LOGIN=true`, `HUGEGRAPH_LLM_ENABLE_THIN_WRITES=true`, a configured `USER_TOKEN`, |
There was a problem hiding this comment.
Minor: 0bef461 changed this requirement from "configure a strong USER_TOKEN" to "a configured USER_TOKEN". The CN page at line 188 still says 配置强 USER_TOKEN, so the two languages now disagree. Because USER_TOKEN always has a value, "configured" is met by the default. An EN reader who turns on ENABLE_LOGIN=true and HUGEGRAPH_LLM_ENABLE_THIN_WRITES=true can then leave the write endpoints guarded by the well-known token 4321.
Evidence: hugegraph-llm/src/hugegraph_llm/config/admin_config.py:27 on apache/hugegraph-ai main (3fad379) sets user_token: Optional[str] = "4321", and demo/rag_demo/app.py:55-64 compares the bearer token against it. hugegraph-llm/README.md:196-198 says to "replace the default USER_TOKEN=4321 with a strong random secret" before enabling these endpoints, and hugegraph-mcp/README.md:250-251 says "configure a strong USER_TOKEN".
Please restore the strong, non-default wording so it matches the CN page and the upstream READMEs, for example:
| - Service settings (HugeGraph-LLM): `ENABLE_LOGIN=true`, `HUGEGRAPH_LLM_ENABLE_THIN_WRITES=true`, a configured `USER_TOKEN`, | |
| - Service settings (HugeGraph-LLM): `ENABLE_LOGIN=true`, `HUGEGRAPH_LLM_ENABLE_THIN_WRITES=true`, a strong, non-default `USER_TOKEN` (the default is `4321`), |
imbajin
left a comment
There was a problem hiding this comment.
Blocking: yes. Summary: The refresh guidance does not say that MCP and HugeGraph-LLM must reach the same HugeGraph instance, while the protocol validates only graph name and graphspace. Evidence: the current source sends and checks those two fields but executes refresh using the AI service's independent GRAPH_URL; the exact-head workflow is action_required.
|
|
||
| If MCP runs on the host and AI runs in a container, they may need different addresses to reach HugeGraph. | ||
| Configure `GRAPH_URL` on the AI service to an address reachable from its container, such as `http://server:8080`. | ||
| Set `GRAPH_NAME` and `GRAPH_SPACE` to match MCP. Do not use the container's `127.0.0.1` to reach the host graph service. |
There was a problem hiding this comment.
Purpose of the PR
Add English and Chinese HugeGraph-MCP usage guides and make them discoverable from the HugeGraph-AI overview and sidebar. Users can install the published package, configure a STDIO client, verify a connection, and understand the supported query/schema/write boundaries and optional AI prerequisites.
The guides distinguish published
hugegraph-mcp==1.7.1behavior from current-sourcecore/legacytoolsets and future aligned releases. They document separate graph/AI connection settings, authentication and write gates, refresh target checks, and why raw Gremlin and other registered operations can remain disabled.Paired implementation: apache/hugegraph-ai#382. Coordinate the current-source guidance with that code PR; the published-package installation remains explicitly pinned.
This PR independently applies the same five documentation/navigation files to
apache/hugegraph-doc:masteratec9ae7b: 5 files, +461/-0. It carries no changes to the target repository's CI, theme, or site runtime.Validation
scripts/hugo.sh buildpassed on this target branch (Hugo Extended 0.165.0, Go 1.27.0, repository-pinned OINK 1.2.0).bash dist/validate-links.shpassed in an isolated Linux container using copies of the current content/assets and the unchanged validator with Windows CRLF normalized to LF.tests/e2e/search-ranking.spec.js: 24 passed, against this target's fresh artifact in Edge Chromium.git diff --checkpassed. Before/after images below were regenerated from this target's current base and topic builds.Before / after
New guide screenshots and capture details are stored on a separate evidence branch.
Synchronized documentation PR: hugegraph/hugegraph-doc#46, based independently on the org's master to keep both reviews scoped.
Source-suite baseline comparison: the unmodified target master also failed on the same Windows toolchain (257 tests, 9 failures, 42 errors, 1 skipped). The set of failing test identities is identical between base and topic; varying error totals include Windows temporary-directory cleanup/locked-file errors (
WinError 32/WinError 5). No new failing test identity was introduced by the five-file documentation patch. This comparison does not replace Linux CI.