feat: add REST API server and Knowledge Workbench web UI#150
Conversation
Add a production-ready REST API (FastAPI) and a bundled React single-page
app ("Knowledge Workbench") served at the same origin, so OpenKB can be
driven from the browser, Postman, or other HTTP clients without the CLI.
REST API (openkb/api.py):
- Endpoints under /api/v1: /kbs, /init, /add, /query, /chat,
/chat/sessions{,/load,/delete}, /list, /status, /lint, /remove,
/recompile, /watch/{start,stop,status}, /watch/events (SSE)
- Bearer-token auth, SSE streaming for query/chat/add/remove/recompile,
multipart upload, and KB name resolution via OPENKB_KB_ROOT
- Shared SSE event layer (iter_agent_response_events) reused by query/chat
so the CLI and API emit identical event streams
- Background file-watcher service (openkb/watch_service.py) for auto-compile
Knowledge Workbench (frontend/, built to web/):
- React + Vite dark three-pane workbench: Overview, Documents, Query,
Chat, Maintenance, with a live retrieval/reasoning inspector timeline
- Streamed answers, multi-turn chat with persisted sessions, drag-and-drop
upload with per-file progress, lint/recompile/watch controls
- Markdown via react-markdown + remark-gfm + rehype-highlight
Also: initialize_kb inherits project-root config.yaml and .env (filtering
OPENKB_* server vars) so a KB created from the UI runs out of the box;
POSTMAN collection; README docs.
|
@jidechao
I'll follow up with implementation feedback after. |
…p BOM, unvendor web/ - config.yaml.example: restore gpt-5.4 / en (unintentional flip to local dev values openai/deepseek-v4-flash / zh) - openkb/cli.py, tests/test_api.py: remove stray UTF-8 BOM - web/: stop vendoring the built bundle; add web/ to .gitignore; README notes the Workbench UI requires `npm run build` (API-only otherwise) Refs: PR VectifyAI#150
|
|
@KylinMountain Thanks for the review. All three actionable points are addressed in the latest push (
Verification: Standing by for the implementation-layer feedback. |
KylinMountain
left a comment
KylinMountain
left a comment
There was a problem hiding this comment.
Review — REST API + Knowledge Workbench
Nice, largely-additive feature — CLI behavior is unchanged and the shared SSE helper is only consumed by the new API paths. The high-severity issues cluster in three areas, flagged inline.
Must-fix (correctness / data):
- Mutation endpoints (
recompile/lint --fix/init) run sync, lock-holding work directly on the event loop;remove/addalready userun_in_threadpool, these don't. Becausekb_ingest_lock's reentrancy bookkeeping isthreading.local, concurrent same-KB requests on the one event-loop thread bypass mutual exclusion → KB corruption (different-KB requests instead stall the loop). [api.py:321/541/581] - Watcher
stop()can orphan an in-flight compile and a restart then double-ingestsraw/. [watch_service.py:204] _save_query_answer: empty/colliding exploration slug silently loses data (acute for CJK questions), drops the CLI's ghost-link stripping, and writes the question into YAML unescaped. [api.py:801]
Should-fix: leaked/never-terminating /watch/events SSE [api.py:1145]; frontend doesn't abort the stream on unmount/KB-switch [useSSEStream.js]; CORS * + credentials [api.py:699]; token sent to a user-supplied origin + non-constant-time compare [sse.js:50, api.py:772]; per-KB LLM key ignored due to override=False [api.py:321].
Verified clean (so you don't re-chase): KB short-names go through validate_kb_name (fullmatch on [A-Za-z0-9_-]+) → no path traversal; upload filenames via Path(...).name; the .env inheritance filter correctly excludes OPENKB_* server vars while keeping LLM_API_KEY; _sse always emits single-line data:; CLI query/chat streaming is unchanged by the refactor.
Minor / cleanup: duplicated KB-dir check (api.py:730 & 787 → extract _is_kb_dir); redundant _setup_llm_key wrapper; dead model.dict() Pydantic-v1 branch.
Thanks for the thorough test plan and the offer to split the PR — splitting API vs Web UI would make these land more incrementally.
| return RemoveResponse(**result) | ||
|
|
||
| @app.post("/api/v1/recompile", response_model=RecompileResponse) | ||
| async def recompile_endpoint( |
There was a problem hiding this comment.
Concurrent KB mutation can corrupt the KB — run this in a threadpool. recompile_endpoint / _stream_recompile drive iter_recompile directly on the event loop, which acquires kb_ingest_lock (openkb/locks.py). That lock tracks reentrancy in threading.local (locks.py:45,82-128), but asyncio runs multiple requests on the same event-loop thread — so a second concurrent same-KB recompile/lint/init is mis-counted as a re-entrant hold and skips locking entirely → two compiles mutate hashes.json/wiki pages at once. For different KBs, the blocking flock/compile instead stalls the whole event loop. remove_endpoint (L559) and the add path already use run_in_threadpool — do the same here. (Same root cause at lint_endpoint L541 and init_endpoint L321.)
| ) -> LintResponse: | ||
| kb_dir = _resolve_kb(request.kb) | ||
| try: | ||
| return LintResponse(**await run_lint_report(kb_dir, fix=request.fix)) |
There was a problem hiding this comment.
Same event-loop hazard as the recompile comment (L581): with fix=True, run_lint_report runs fix_broken_links synchronously under kb_ingest_lock before any await, blocking the loop and sharing the threading.local reentrancy state. Wrap in run_in_threadpool like remove/add.
| return KbListResponse(**_list_knowledge_bases()) | ||
|
|
||
| @app.post("/api/v1/init", response_model=InitResponse) | ||
| async def init_endpoint( |
There was a problem hiding this comment.
Two issues here:
initialize_kb+register_kb_alias(a global-config file lock) run on the event loop — wrap inrun_in_threadpool(see L581).- The per-KB
LLM_API_KEYthis writes is later loaded by_setup_llm_keywithoverride=False, butopenkb-apialready hasLLM_API_KEYinos.environfrom the server's root.env— so queries against a KB created here silently use the server's key, not the KB's. Breaks multi-key / multi-tenant setups.
| with self._lock: | ||
| return list(self._watchers.keys()) | ||
|
|
||
| def stop(self, kb: str) -> bool: |
There was a problem hiding this comment.
stop() can orphan an in-flight worker, and a restart then double-ingests. stop() pops the state (L207) then worker_thread.join(timeout=5.0) (L220). A compile under kb_ingest_lock can take minutes, so the join times out and stop() returns True / active:false while the worker keeps running, detached. A later start() finds no state (it was popped) and spawns a second observer+worker → two threads ingest the same raw/. Suggest: join before popping; surface a 'draining' state instead of silently timing out; and have start() refuse/await while a prior worker for the KB is still alive (it currently keys idempotency on running.is_set(), which also stays set if the observer/worker dies).
| setup(kb_dir) | ||
|
|
||
|
|
||
| def _save_query_answer(kb_dir: Path, question: str, answer: str) -> Path | None: |
There was a problem hiding this comment.
Three problems in this save path, and it diverges from the CLI's query --save:
- Empty / colliding slug → silent data loss.
slug = re.sub(r'[^a-z0-9]+','-', q.lower()).strip('-')[:60]collapses to''for any CJK / punctuation-only question → writeswiki/explorations/.md, and every such query clobbers that one hidden file. Two questions sharing a 60-char prefix also overwrite each other. Add a fallback slug + uniquify (the lint-report path already uniquifies). - Drops ghost-wikilink stripping the CLI applies (
cli.py:971-988runsstrip_ghost_wikilinks), so API-saved explorations keep dangling[[links]]. - Unescaped question in YAML —
query: "{question}"produces invalid frontmatter if the question contains".
Recommend extracting a sharedsave_exploration(kb_dir, question, answer)used by both CLI and API.
| yield _sse("done", {}) | ||
|
|
||
|
|
||
| async def _stream_watch_events( |
There was a problem hiding this comment.
Leaked / never-terminating SSE. _stream_watch_events never checks await request.is_disconnected(), and max_events/timeout_seconds default to None (unbounded) — a client that opens /watch/events then navigates away leaves a generator polling every ~0.5s forever, holding the WatcherState (prevents GC even after the watcher is stopped/popped). Separately, in watch_service._record_event the seq is assigned under the lock but events.append happens outside it, so watcher_stopped can be reordered / evicted from the deque(maxlen) and the stream never sees its terminator. Add a disconnect check + a default cap, and move the append inside the lock.
| }, []); | ||
|
|
||
| const start = useCallback(async (cfg, onEvent, onAbort) => { | ||
| if (ctrlRef.current) return; // already streaming |
There was a problem hiding this comment.
Stream isn't aborted on unmount / KB or session switch. Views are swapped by conditional render, so leaving Chat/Query unmounts the component mid-stream, but there's no cleanup useEffect calling stop() — the fetch keeps running and setMsgs/inspector callbacks fire after unmount, writing deltas for the old KB/session (kb is captured in the start() closure). Two related issues: if (ctrlRef.current) return (L21) silently drops a second call, leaving a permanent pending bubble; and aiIdx = msgs.length captured at send time points at the wrong row after loadHistory() replaces msgs. Add an unmount + kb-change effect that calls stop(), and key the target message by id rather than index.
| ] | ||
| app.add_middleware( | ||
| CORSMiddleware, | ||
| allow_origins=origins, |
There was a problem hiding this comment.
allow_origins=['*'] together with allow_credentials=True. When OPENKB_CORS_ORIGINS=* (a natural 'allow my frontend' choice), any site the operator visits can issue credentialed cross-origin requests to the (often localhost-bound) server and drive /remove, /init, etc. A wildcard origin must be incompatible with credentials — reject * when credentials are enabled, or require an explicit origin allowlist.
| status_code=status.HTTP_401_UNAUTHORIZED, | ||
| detail="Bearer token required.", | ||
| ) | ||
| if credentials.credentials != expected: |
There was a problem hiding this comment.
Use a constant-time comparison: hmac.compare_digest(credentials.credentials, expected) (or secrets.compare_digest). Plain != short-circuits and is timing-attackable once the server is bound beyond localhost. (Good that a missing OPENKB_API_TOKEN is rejected rather than treated as open.)
| return { | ||
| ...(body && !isForm ? { "Content-Type": "application/json" } : {}), | ||
| Accept: "text/event-stream", | ||
| ...(token ? { Authorization: `Bearer ${token}` } : {}), |
There was a problem hiding this comment.
The bearer token is attached to baseUrl() + path where baseUrl() is whatever the user typed in Settings — no origin / HTTPS check — and it's stored in localStorage (XSS-readable). A user who pastes a malicious or typo'd API base sends their token to an attacker-controlled host on the first request. Suggest validating the base origin and warning on non-HTTPS cross-origin. (Same header build in client.js:39.)
|
Also, I suggest setting the default webpage language to English, but with a language switcher available—that might work better. @jidechao |
Follow-up: structure & docs organizationA few organizational notes beyond the inline correctness comments — all about keeping the surface lean and single-sourced. 1.
|
3 participants
Summary
Adds a production-ready REST API (FastAPI) and a bundled Knowledge Workbench web UI to OpenKB, so a knowledge base can be built, queried, and maintained entirely from the browser, Postman, or any HTTP client — no CLI required.
The two are co-served from one origin:
openkb-apimounts the built SPA at/while exposing the JSON/SSE API under/api/v1.REST API (
openkb/api.py,openkb/watch_service.py)GET /kbs+POST /init— list and create knowledge bases (resolvable by short name viaOPENKB_KB_ROOT)POST /add(multipart, SSE) — upload + compile documents with per-file progressPOST /query(SSE) — one-shot question, streamed answerPOST /chat+POST /chat/sessions{,/load,/delete}(SSE) — multi-turn chat with persisted, resumable sessionsPOST /list,/status— inventory and index statsPOST /lint(optionalfix),POST /remove(SSE),POST /recompile(SSE)POST /watch/{start,stop,status}+GET /watch/events(SSE) — background file watcheriter_agent_response_eventslayer means the CLI and API emit identical event streamsopenkb-postman.jsonKnowledge Workbench (
frontend/→ builtweb/)A React + Vite single-page app (dark, three-pane):
initialize_kbinherits the operator's project-rootconfig.yamland LLM credentials (.env, withOPENKB_*server vars filtered out), so a KB created from the UI runs out of the box.Build / packaging
web/holds the pre-built bundle and is mounted by the API;frontend/source is included andnode_modules/distare gitignoredopenkb-api(alsopython -m openkb.api)Test plan
pytest tests/test_api.py tests/test_api_watch.py tests/test_watch_service.py tests/test_remove.py— 126 passednpm run buildproducesweb/index.html+web/assets/*tool_calltimeline, chat create/resume/delete, lint/recompile/watchNotes