Repository navigation
feat(web): I/O token column for perf list and history record deletion - #1526
Conversation
…records - Return succeed-weighted avg_input_tokens / avg_output_tokens in /api/v1/perf/list so the web UI can explain latency gaps (#1519) - Add DELETE /api/v1/perf/run: path-traversal rejection via realpath, running-task protection (409), empty parent-dir pruning (#1521) - Add DELETE /api/v1/reports/report: per-model artefact removal, whole run dir cleanup when the last model report is deleted (#1521) - Expose active_task_ids() from the service process registry - Cover token fields, deletion, traversal and 409 paths with unit tests
- Add I->O tok column to the performance run list (desktop table + mobile meta line), rendered as e.g. 30000->100t (#1519) - Add a shared ConfirmDialog (dark-theme modal, affected-item list, focus management, busy lock) replacing window.confirm - Wire delete actions into SelectionTray on both the Evaluations and Performance pages via new DELETE endpoints (#1521) - Unify delete wording under the generic reports.* i18n namespace - Guard delete confirm handlers against double-click re-entry
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
There was a problem hiding this comment.
👋 Review Summary
This PR adds useful I/O token visibility for perf runs and a much more ergonomic way to delete historical perf and eval records from the web UI, with good attention to path-traversal guards, active-task checks, typed client contracts, and dedicated tests.
🛡️ Key Risks & Issues
- Destructive use of client-controlled
root_path: Both DELETE/api/v1/perf/runand DELETE/api/v1/reports/reportrely on_root_path()and therefore trust the caller’sroot_pathquery parameter as the base for destructive filesystem operations. While the implementations correctly confine deletions to descendants of that root and only touch paths that look like perf/report runs, any caller who can hit the service and chooseroot_pathcan target arbitrary directories that match the expected layout. This was low-risk whenroot_pathonly influenced reads; once it drivesshutil.rmtreeand full run-dir removal, it becomes much more important to constrain it to the canonical outputs root or validate that a provided value is under that configured root before allowing deletion. - Task-ID matching semantics for perf deletion: The running-task guard for perf uses a set of all path segments and intersects this with
active_task_ids(). This works for the current<task_id>/perflayout, but it will also block deletion when a task ID coincides with any inner path segment (for example, a CLI run path that happens to contain the same token), leading to surprising “Task is still running” responses for directories that are not actually owned by a task. This is more of a behavioural edge case than a bug, but worth keeping in mind if layouts evolve. - Frontend deletion flows are untested: The batch deletion logic on
PerfReportsPageandReportsPage(sequential DELETEs, partial-failure handling, selection cleanup, reload triggers, and error messaging) is implemented carefully but currently only covered by manual verification. Given the potential impact of regressions in these flows on user trust and data cleanup, it would be valuable to add targeted tests that cover success and partial-failure scenarios, especially under filters/search.
🧪 Verification Advice
- Exercise DELETE
/api/v1/perf/runand/api/v1/reports/reportagainst both service-style and CLI-style run trees, including edge cases: badpath/report_name, traversal attempts, and active-task IDs that should trigger 409. Confirm that the outputs root is never removed and that parent directories are pruned only when empty. - In a staging environment, verify how
root_pathis used end-to-end: check that the dashboard always sends the intended outputs root and that there is no way for non-admin clients to point it at arbitrary directories. If the service is exposed more broadly, consider tightening this contract before rollout. - For the web UI, test multi-select deletion of perf runs and reports with filters and search applied, including a simulated backend failure mid-batch (e.g., returning 400/409 for one item) to ensure the selection and error banner behave as expected.
💡 Thoughts & Suggestions
- Consider constraining or removing
root_pathfor destructive APIs: either always use the configured outputs root for DELETE operations or explicitly validate that a providedroot_pathis a descendant of that configured directory. This would keep the flexibility ofroot_pathfor read-only queries while reducing the blast radius for misconfigured or semi-trusted clients. - If the current task-ID layouts are expected to remain stable, the segment-based matching for running-task protection is acceptable; if you anticipate new layouts, it may be safer to narrow the check to known prefix positions. This would avoid future surprises where certain history records cannot be deleted due to incidental name collisions.
- The new ConfirmDialog component and the I/O token column are solid additions with good UX and accessibility touches. Adding a few focused tests around keyboard focus-trapping and overlay-click behaviour would help ensure those qualities are preserved as the UI evolves.
🤖 Generated by Qoder • View workflow run
| return jsonify({'error': str(e)}), 500 | ||
|
|
||
|
|
||
| @bp_perf.route('/run', methods=['DELETE']) |
There was a problem hiding this comment.
The new DELETE endpoints correctly guard against traversal and symlink escapes relative to the chosen root, but root_path itself is still entirely client-controlled and now drives destructive operations.
For perf runs, delete_perf_run resolves the base directory via _root_path() and passes it directly into perf_archive.delete_run(_root_path(), rel_path) (evalscope/service/blueprints/perf.py:386-409). delete_run then validates rel_path relative to that root and only removes directories recognized as perf runs (evalscope/service/perf_archive.py:587-603). This was safe while root_path only governed list/detail reads; with deletion wired to the same parameter, any caller that can hit the service can point root_path at an arbitrary directory tree containing perf-layout folders and delete them, as long as they match the expected run shape. That effectively turns root_path into a remote deletion capability for any perf-like tree on disk, which is a problem if the service is exposed beyond fully trusted operators.
The reports endpoint mirrors this pattern: delete_report also uses _root_path() to derive root_real and run_dir (evalscope/service/blueprints/reports.py:368-372) and then calls shutil.rmtree on per-model subtrees and potentially the entire run directory (lines 378-391). The path checks ensure nothing escapes root_real, but the caller still chooses root_real via root_path.
If these APIs are meant only for tightly controlled dashboards where root_path is constrained to the canonical outputs root, it would be safer to either (a) ignore root_path for DELETE and always use the configured outputs root, or (b) validate that a provided root_path is a descendant of the configured outputs directory before allowing deletion. Otherwise, a misconfigured client or semi-trusted integration could use these endpoints to delete data outside the intended evalscope outputs area while still passing all internal safety checks.
🤖 Generated by Qoder • Fix in Qoder
Summary
This PR delivers two Web UI feature requests for the history record lists:
I→O tokcolumn to the performance run list so latency differences between runs of the same model become explainable at a glance.Changes
Backend (
evalscope/service)GET /api/v1/perf/listnow returnsavg_input_tokens/avg_output_tokensper run, computed as a succeed-request-weighted average of the measured per-run summaries (backward compatible; fields are additive).DELETE /api/v1/perf/run(root_path+path):realpathprefix checks (reusesresolve_run_dir);409when the path belongs to an active task;<timestamp>/husks) after deletion;{"success": true, "path": "..."}.DELETE /api/v1/reports/report(root_path+report_name):reports/predictions/reviews/<model>) so runs holding multiple model reports stay intact;active_task_ids()exposed from the service process registry.Frontend (
evalscope/web)I→O tokcolumn (desktop table + mobile meta line), rendered as e.g.30000→100t; legitimate0values still render, only missing data shows—.ConfirmDialogcomponent (dark-theme modal via portal, enumerates the affected records, danger-styled confirm button, initial focus on Cancel, focus trap, Esc/overlay cancel, busy lock) — replaceswindow.confirm.SelectionTrayon both the Evaluations and Performance pages; partial failures keep only the not-yet-deleted items selected and surface the error.reports.*i18n namespace ("Delete records" / 「删除记录」, en + zh) — no perf/eval-specific phrasing.apiDeleteValidatedclient helper + zod schemas for the new endpoints.Tests
tests/perf/test_perf_archive.py(13 cases) + newtests/report/test_report_delete.py(4 cases) — token fields, deletion success, parent-dir pruning, multi-model run preservation, traversal / invalid-name / running-task rejections. 17 passed.ConfirmDialog.test.tsxadded; full suite 237 passed,tsc + vite buildandeslintclean.evalscope service(column rendering en/zh, multi-select delete, confirm/cancel/Esc paths, on-disk cleanup).Closes #1519
Closes #1521