-
-
Notifications
You must be signed in to change notification settings - Fork 2
.pr_agent_accepted_suggestions
| PR 390 (2026-07-06) |
[maintainability] Direct `fetch` in `DatasetToolsPage`
Direct `fetch` in `DatasetToolsPage`
The PR adds direct `fetch()` calls inside React components/pages instead of routing backend communication through `frontend/lib/api.ts`. This bypasses the centralized API layer and makes future auth/error-handling changes harder to enforce consistently.React components/pages must not call backend endpoints via direct fetch(); they must use the centralized client in frontend/lib/api.ts.
This keeps backend access consistent (headers, base URL, error handling, security controls) and avoids duplicated networking logic.
- frontend/app/dataset-tools/page.tsx[41-52]
- frontend/components/dataset-tools/file-tree.tsx[62-62]
- frontend/components/dataset-tools/metadata-edit-dialog.tsx[54-87]
[security] Crop filename traversal
Crop filename traversal
`CropService` constructs `src_file = dataset_path / crop.filename` without ensuring it stays within the validated dataset directory; in `in-place` mode it may also `unlink()` the source. A crafted `crop.filename` containing `../` can therefore target files outside the dataset directory for reading and potential deletion.CropService._crop_single() uses dataset_path / crop.filename directly. This does not prevent .. traversal or absolute paths, and in in-place mode the code can delete src_file.
validate_dataset_path() ensures the dataset root is inside datasets/, but it does not validate per-image filenames provided in the crop request.
- Resolve and contain-check the joined path:
candidate = (dataset_path / crop.filename).resolve()- Reject if not
candidate.is_relative_to(dataset_path.resolve()) - Consider restricting
crop.filenameto a basename (Path(...).name) unless you intentionally support subfolders; if subfolders are supported, still enforce containment. - Validate the extension is an allowed image type before opening/deleting.
- services/crop_service.py[101-141]
- services/core/validation.py[24-86]
[correctness] Job totals always zero
Job totals always zero
Crop/convert status responses compute `total_files` from `job_status.total_images`, but coroutine jobs created via `run_coroutine_job()` never set `total_images`. As a result, status polling will report 0 totals and derived counts even when work is in progress or completed.CropService.get_status() and ConvertService.get_status() report totals using job_status.total_images, but coroutine jobs don't populate that field.
JobManager.run_coroutine_job() creates a Job without total_images, and only subprocess-monitored jobs populate totals via log parsing.
- Set
job.total_images = totalat the beginning of_run_crop()and_run_conversion()(they already computetotal). - Alternatively, extend the coroutine-job API to accept a
total_imagesvalue during job creation.
- services/crop_service.py[143-213]
- services/crop_service.py[215-230]
- services/convert_service.py[153-227]
- services/convert_service.py[228-244]
- services/jobs/job_manager.py[101-117]
[reliability] Symlink walk DoS risk
Symlink walk DoS risk
The LoRA listing endpoint uses `os.walk(..., followlinks=True)` without tracking visited directories/inodes, so symlink cycles or large symlinked trees can cause excessive traversal and CPU time. This can degrade API responsiveness or effectively deny service for that endpoint./api/utilities/lora/list walks directories with followlinks=True to include symlinked folders, but it does not detect symlink cycles or repeated directories, risking unbounded traversal.
Even if the starting directory is validated, symlinked subdirectories can create cycles or very large graphs.
- Track visited directories by realpath/inode (platform dependent) and skip revisiting.
- Add a max-depth / max-files cap to bound work.
- Consider keeping
followlinks=Falseand explicitly resolving known symlink patterns if needed.
- api/routes/utilities.py[309-336]
[performance] Blocking HTTP in async
Blocking HTTP in async
The sources API uses `async def` endpoints but calls adapter methods that perform blocking `requests.get()` calls, which can block the event loop thread during slow upstream responses. This reduces concurrency and can stall unrelated requests handled by the same worker.api/routes/sources.py defines async endpoints but calls Arc adapter methods synchronously; the Arc adapter uses requests.get(), which blocks.
Blocking I/O in async endpoints can stall the FastAPI event loop thread, reducing throughput and increasing tail latency.
- Switch Arc adapter to an async HTTP client (e.g.
httpx.AsyncClient) and make adapter methods async. - Or wrap adapter calls in
await asyncio.to_thread(...)at the route layer (search/model/versions/download resolve).
- api/routes/sources.py[201-232]
- services/sources/arcenciel.py[87-96]
[reliability] Unclosed PIL image handles
Unclosed PIL image handles
The crop and convert services open images with `Image.open()` without a context manager/explicit close, which can hold file descriptors longer than necessary during batch operations. Under large datasets or parallel crop execution, this increases the risk of resource pressure and intermittent failures.Image.open() is used without with ... as img: in both crop and conversion flows, so file handles may remain open until GC.
Batch operations iterate over many images (and crop runs in a thread pool). Leaving closure to GC can increase peak open files and memory.
- Wrap
Image.open(...)in a context manager and ensure derived images are closed if needed. - In convert/crop, avoid reassigning
imgwithout closing the prior object.
- services/crop_service.py[113-141]
- services/convert_service.py[158-191]