Conversation
There was a problem hiding this comment.
Pull request overview
This PR expands the web UI with a sidebar diagnostics/presets experience and adds a standalone RAG Manager page to manage Chroma collections, inspect RAG source files, run lint/coverage, and kick off long-running push/eval jobs.
Changes:
- Add chat diagnostics panel + ZIP export bundle endpoint, and wire UI controls in
templates/index.html. - Introduce preset profile persistence + apply/delete endpoints backed by
core/preset_profiles.py. - Add RAG Manager UI routes and templates, backed by
core/rag_manager.pyand a threaded in-memoryJobStore.
Reviewed changes
Copilot reviewed 25 out of 25 changed files in this pull request and generated 15 comments.
Show a summary per file
| File | Description |
|---|---|
| web_app.py | Adds diagnostics/export bundle, presets endpoints, and RAG Manager routes + job polling endpoint. |
| core/preset_profiles.py | Implements JSON-backed preset save/apply/delete for runtime retrieval settings. |
| core/rag_manager.py | Adds sync façade around scripts/rag for collections/files/lint/coverage/eval/benchmark. |
| core/job_queue.py | Adds a simple in-memory background job store for HTMX polling. |
| templates/index.html | Adds diagnostics/presets panels, RAG Manager link, and export-bundle button + JS refresh. |
| templates/diagnostics_panel.html | New HTMX partial for per-turn diagnostics table. |
| templates/presets_panel.html | New HTMX partial for saving/applying/deleting preset profiles. |
| templates/rag/*.html | New standalone RAG Manager UI (collections, detail/query, files, lint, coverage, evaluate, benchmark, job status). |
| docs/future_work/UI_REFINEMENTS.md | Updates UI backlog organization and entries. |
| docs/future_work/REFINEMENTS.md | Adds/expands backlog sections (memory, branching, observability, etc.). |
| docs/future_work/COPILOT_COMPACT_REFERENCE.md | Documents newly implemented UI features/routes/files. |
| AGENTS.MD | Documents additional available CLI tools and preferred usage order. |
| .github/copilot-instructions.md | Mirrors AGENTS.MD CLI tools guidance for Copilot usage. |
| def apply_profile(self, name: str, config: ConversationRuntimeConfig) -> list[str]: | ||
| """Write profile values onto *config* in place; return list of changed field names.""" | ||
| profile = self.get_profile(name) | ||
| changed: list[str] = [] | ||
| for field, value in profile.items(): | ||
| if field not in PROFILE_FIELDS: | ||
| continue | ||
| current = getattr(config, field, None) | ||
| if current != value: | ||
| setattr(config, field, value) | ||
| changed.append(field) | ||
| return changed |
There was a problem hiding this comment.
ConversationRuntimeConfig is a frozen dataclass (see core/config.py), so setattr(config, field, value) will raise FrozenInstanceError and profiles cannot be applied. Consider building a new config via dataclasses.replace(config, **validated_updates) and then swapping runtime.manager.runtime_config to the new instance; also ensure any cached fields in ConversationManager (e.g., rag_k, rag_k_mes) are kept in sync with the new config.
| @app.post("/settings/profiles/save", response_class=HTMLResponse) | ||
| async def settings_profiles_save(request: Request, name: str = Form(...)) -> HTMLResponse: | ||
| """Save current runtime config as a named profile, then re-render the panel.""" | ||
| runtime = _get_runtime(request) | ||
| store = _get_profile_store(request) | ||
| clean_name = name.strip()[:40] | ||
| if clean_name: | ||
| store.save_profile(clean_name, runtime.manager.runtime_config) | ||
| return _render_profiles_panel(request) | ||
|
|
||
|
|
||
| @app.post("/settings/profiles/apply", response_class=HTMLResponse) | ||
| async def settings_profiles_apply(request: Request, name: str = Form(...)) -> HTMLResponse: | ||
| """Apply a saved profile to the live runtime config, then re-render the panel.""" | ||
| runtime = _get_runtime(request) | ||
| store = _get_profile_store(request) | ||
| try: | ||
| changed = store.apply_profile(name, runtime.manager.runtime_config) | ||
| logger.info("Applied profile {!r}; changed fields: {}", name, changed) | ||
| except KeyError: | ||
| logger.warning("Profile {!r} not found", name) | ||
| return _render_profiles_panel(request) |
There was a problem hiding this comment.
Profile apply/save/delete routes mutate or read runtime.manager.runtime_config without acquiring runtime.lock, while /chat/stream holds the same runtime lock during generation. To avoid races (and partially-applied config during an active turn), wrap these handlers in async with runtime.lock: when reading/writing live runtime settings.
| @app.get("/rag/collections/{name}", response_class=HTMLResponse) | ||
| async def rag_collection_detail(request: Request, name: str) -> HTMLResponse: | ||
| config = _get_rag_config(request) | ||
| info = await asyncio.to_thread(rag_manager.collection_info, config, name) | ||
| return templates.TemplateResponse( | ||
| "rag/collection_detail.html", | ||
| {"request": request, "info": info}, | ||
| ) |
There was a problem hiding this comment.
rag_manager.collection_info() can return None when the collection doesn't exist, but the handler always renders rag/collection_detail.html. That template assumes info has keys like name/count, so this will raise a template error (500) on unknown names. Return a 404 (or render a dedicated error partial) when info is None.
| @app.get("/rag/files/{filename}", response_class=HTMLResponse) | ||
| async def rag_file_view(request: Request, filename: str) -> HTMLResponse: | ||
| config = _get_rag_config(request) | ||
| content = await asyncio.to_thread(rag_manager.file_content, config, filename) | ||
| return templates.TemplateResponse( | ||
| "rag/file_view.html", | ||
| {"request": request, "filename": filename, "content": content}, | ||
| ) |
There was a problem hiding this comment.
rag_manager.file_content() returns None for unknown files and for rejected path-traversal attempts, but the handler always renders rag/file_view.html with content=None. That leads to confusing output and makes it hard to distinguish “not found/blocked” from “empty file”. Consider returning a 404 (or an error partial) when content is None.
| @app.post("/rag/coverage", response_class=HTMLResponse) | ||
| async def rag_coverage(request: Request, stem: str = Form(...)) -> HTMLResponse: | ||
| config = _get_rag_config(request) | ||
| result = await asyncio.to_thread(rag_manager.run_coverage, config, stem.strip()) | ||
| return templates.TemplateResponse( | ||
| "rag/coverage_report.html", | ||
| {"request": request, "result": result}, | ||
| ) |
There was a problem hiding this comment.
rag_manager.run_coverage() can return None (missing stem .txt/.json pair), but the handler always renders rag/coverage_report.html, which assumes result is a dict and immediately dereferences it. This will raise a template error (500) on missing/invalid stems. Handle result is None explicitly (e.g., return 404 or render a small error panel).
| @app.post("/rag/coverage", response_class=HTMLResponse) | ||
| async def rag_coverage(request: Request, stem: str = Form(...)) -> HTMLResponse: | ||
| config = _get_rag_config(request) | ||
| result = await asyncio.to_thread(rag_manager.run_coverage, config, stem.strip()) | ||
| return templates.TemplateResponse( |
There was a problem hiding this comment.
Similarly, stem passed to /rag/coverage is only .strip()ped before calling rag_manager.run_coverage(), which currently uses the stem to construct paths under documents_directory without guarding against .. traversal. Validate stem in the handler (and/or in run_coverage()) and return a clean error response when invalid.
| @app.post("/rag/evaluate/run", response_class=HTMLResponse) | ||
| async def rag_evaluate_run( | ||
| request: Request, | ||
| fixture_file: str = Form(...), | ||
| ) -> HTMLResponse: | ||
| config = _get_rag_config(request) | ||
| job_store = _get_job_store(request) | ||
| job_id = job_store.submit(rag_manager.run_evaluate_fixtures, config, fixture_file) | ||
| return templates.TemplateResponse( |
There was a problem hiding this comment.
fixture_file is accepted from the client and forwarded directly to rag_manager.run_evaluate_fixtures() in a background job. Since run_evaluate_fixtures() currently joins tests_dir / fixture_file without guarding against .. traversal, validate that fixture_file is one of list_fixture_packs() (or otherwise enforce a safe filename) before submitting the job.
| class ProfileStore: | ||
| """Persist and apply named retrieval-setting presets stored in a JSON file.""" | ||
|
|
||
| def __init__(self, path: Path) -> None: | ||
| self._path = path | ||
|
|
||
| def _load(self) -> dict[str, dict[str, object]]: | ||
| if not self._path.exists(): | ||
| return {} | ||
| try: | ||
| data = json.loads(self._path.read_text(encoding="utf-8")) | ||
| return data if isinstance(data, dict) else {} | ||
| except Exception: | ||
| return {} | ||
|
|
||
| def _save(self, data: dict[str, dict[str, object]]) -> None: | ||
| self._path.parent.mkdir(parents=True, exist_ok=True) | ||
| self._path.write_text(json.dumps(data, ensure_ascii=False, indent=2), encoding="utf-8") | ||
|
|
||
| def list_profiles(self) -> list[str]: | ||
| """Return sorted list of saved profile names.""" | ||
| return sorted(self._load().keys()) | ||
|
|
||
| def save_profile(self, name: str, config: ConversationRuntimeConfig) -> None: | ||
| """Snapshot the profile-eligible fields from *config* under *name*.""" | ||
| data = self._load() | ||
| data[name] = {field: getattr(config, field) for field in PROFILE_FIELDS} | ||
| self._save(data) | ||
|
|
||
| def get_profile(self, name: str) -> dict[str, object]: | ||
| """Return the stored settings dict for *name*.""" | ||
| data = self._load() | ||
| if name not in data: | ||
| msg = f"Profile {name!r} not found" | ||
| raise KeyError(msg) | ||
| return dict(data[name]) | ||
|
|
||
| def apply_profile(self, name: str, config: ConversationRuntimeConfig) -> list[str]: | ||
| """Write profile values onto *config* in place; return list of changed field names.""" | ||
| profile = self.get_profile(name) | ||
| changed: list[str] = [] | ||
| for field, value in profile.items(): | ||
| if field not in PROFILE_FIELDS: | ||
| continue | ||
| current = getattr(config, field, None) | ||
| if current != value: | ||
| setattr(config, field, value) | ||
| changed.append(field) | ||
| return changed |
There was a problem hiding this comment.
This new persistence layer is core to the UI (save/apply/delete), but there are no unit tests covering JSON load/save behavior, type validation, and apply semantics (including handling of unknown profiles). Adding a small tests/test_preset_profiles.py would help prevent regressions, especially around config immutability and field syncing.
| @app.delete("/rag/collections/{name}", response_class=HTMLResponse) | ||
| async def rag_collection_delete(request: Request, name: str) -> HTMLResponse: | ||
| config = _get_rag_config(request) | ||
| await asyncio.to_thread(rag_manager.delete_collection, config, name) | ||
| collections = await asyncio.to_thread(rag_manager.list_collections, config) | ||
| return templates.TemplateResponse( | ||
| "rag/collections_list.html", | ||
| {"request": request, "collections": collections, "deleted": name}, | ||
| ) |
There was a problem hiding this comment.
rag_manager.delete_collection() can raise (e.g., unknown collection), but the delete handler doesn't catch exceptions, so a bad name will return a 500 and HTMX will likely swap in an error page. Consider catching expected errors and returning a small error message partial (or a 404) instead.
| @app.post("/rag/collections/{name}/query", response_class=HTMLResponse) | ||
| async def rag_collection_query( | ||
| request: Request, | ||
| name: str, | ||
| query: str = Form(...), | ||
| k: int = Form(5), | ||
| ) -> HTMLResponse: | ||
| config = _get_rag_config(request) | ||
| results = await asyncio.to_thread(rag_manager.query_collection, config, name, query, k) | ||
| return templates.TemplateResponse( | ||
| "rag/query_results.html", | ||
| {"request": request, "results": results, "query": query, "collection": name}, | ||
| ) |
There was a problem hiding this comment.
rag_manager.query_collection() can raise if the collection doesn't exist or the embedding model can't be loaded, but this route doesn't handle those failures, so HTMX will receive a 500 response. Consider catching expected exceptions and returning an error partial (or 4xx) so the UI can display a clear in-panel error instead of failing the swap.
|
@copilot apply changes based on the comments in this thread |
…aversal, error handling, XSS, tests Agent-Logs-Url: https://github.com/ossirytk/light-chat/sessions/0fba773e-cf10-4054-af37-88aebf97d94b Co-authored-by: ossirytk <136164501+ossirytk@users.noreply.github.com>
Applied all changes from the review in commit b474153:
All XSS/stack-trace-exposure CodeQL alerts were also resolved by HTML-escaping reflected values and removing exception messages from error responses. Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
apply_profileincore/preset_profiles.pyto usedataclasses.replace()instead ofsetattron frozen dataclass; return(new_config, changed_fields)tupleasync with runtime.lock:to profile apply route inweb_app.py; sync cachedrag_k/rag_k_mesfields after applyrag_collection_detailwhencollection_info()returnsNonerag_file_viewwhenfile_content()returnsNonerag_coveragewhenrun_coverage()returnsNone; add stem validationrag_collection_deletein try/except and return error partial on failurerag_collection_queryin try/except and return error partial on failurecore/rag_manager.py(push_collection,run_coverage) with regex +is_relative_to()double-checkcore/rag_manager.py(run_evaluate_fixtures) usinglist_fixture_packs()whitelist + path traversal guardweb_app.pyroute handlersJobStore.submitto evict finished jobs before adding new ones, enforce hard captests/test_preset_profiles.pywith 20 unit tests for load/save/apply/delete semantics