79797644e1
Extract review-plan, report, quarantine, state, and config handlers into commands/ with shared cli_helpers; remove unused exceptions and duplicate plan summary wrappers. Archive superseded review markdown, sync docs to 517-test baseline, and fix empty series titles when only a quality tag remains. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com> Co-authored-by: Cursor <cursoragent@cursor.com>
156 lines
5.8 KiB
Markdown
156 lines
5.8 KiB
Markdown
# Code & Documentation Review Report
|
|
|
|
**Date:** 2026-04-07
|
|
**Reviewer:** Forge
|
|
|
|
## Scope
|
|
|
|
This report reflects the repository state after executing the review-report refactor plan and re-verifying the codebase against the updated implementation.
|
|
|
|
Primary verification inputs:
|
|
- `src/vlm/cli.py:567-602`
|
|
- `src/vlm/review_tui.py:16-30`
|
|
- `src/vlm/executor.py:108-130`
|
|
- `src/vlm/executor.py:204-249`
|
|
- `src/vlm/quarantine.py:116-140`
|
|
- `src/vlm/duplicate_resolve.py:16-58`
|
|
- `src/vlm/planner.py:42-57`
|
|
- `src/vlm/planner.py:151-203`
|
|
- `src/vlm/scanner.py:158-214`
|
|
- `src/vlm/io.py:247-326`
|
|
- `tests/test_cli_review_plan.py:237-343`
|
|
- `tests/test_path_safety.py:76-137`
|
|
- `tests/test_executor.py:951-1004`
|
|
- `tests/test_duplicate_resolve.py:223-237`
|
|
- `tests/test_planner.py:619-720`
|
|
- `tests/test_scanner.py:140-186`
|
|
- `tests/test_io.py:112-203`
|
|
- `pyproject.toml:12-20`
|
|
|
|
Validation baseline:
|
|
- `pytest -q` → **507 passed**.
|
|
|
|
---
|
|
|
|
## Overall Score
|
|
|
|
## **9.0 / 10**
|
|
|
|
### Score breakdown
|
|
- **Module boundaries / pipeline:** 8.8/10
|
|
- **Execution safety (filesystem):** 9.0/10
|
|
- **Planning / duplicate logic:** 9.0/10
|
|
- **Data I/O & validation:** 9.0/10
|
|
- **Error handling consistency:** 9.0/10
|
|
- **Test signal:** 9.2/10
|
|
- **Dependencies:** 8.5/10
|
|
|
|
---
|
|
|
|
## Verified strengths
|
|
|
|
1. **Pipeline and module boundaries remain clean and explicit** (scan → parse → analyze → plan → execute).
|
|
- `src/vlm/commands/scan.py:14-97`
|
|
- `src/vlm/commands/parse.py:17-166`
|
|
- `src/vlm/commands/analyze.py:25-124`
|
|
- `src/vlm/commands/plan.py:14-112`
|
|
- `src/vlm/commands/execute.py:37-249`
|
|
|
|
2. **Execution guardrails are materially stronger than the earlier review baseline.**
|
|
- `src/vlm/executor.py:108-130`
|
|
- `src/vlm/executor.py:204-249`
|
|
- `src/vlm/quarantine.py:116-140`
|
|
|
|
3. **Duplicate handling now favors explicit outcomes over silent fallback.**
|
|
- `src/vlm/duplicate_resolve.py:16-58`
|
|
- `src/vlm/planner.py:42-57`
|
|
- `src/vlm/planner.py:151-203`
|
|
|
|
4. **Plan loading now crosses a validated typed boundary.**
|
|
- `src/vlm/io.py:247-326`
|
|
|
|
5. **Testing coverage is broad and currently green.**
|
|
- `tests/test_cli_review_plan.py:237-343`
|
|
- `tests/test_path_safety.py:76-137`
|
|
- `tests/test_executor.py:951-1004`
|
|
- `tests/test_duplicate_resolve.py:223-237`
|
|
- `tests/test_planner.py:619-720`
|
|
- `tests/test_scanner.py:140-186`
|
|
- `tests/test_io.py:112-203`
|
|
|
|
---
|
|
|
|
## Status of previously reported findings
|
|
|
|
### F1) Move/Rename source path is not constrained to `library_root`
|
|
|
|
**Status:** Resolved.
|
|
Execution now rejects move/rename operations when either the source or destination escapes the configured root.
|
|
Evidence: `src/vlm/executor.py:204-249`, `tests/test_path_safety.py:76-137`.
|
|
|
|
---
|
|
|
|
### F2) `by_quality` silently falls back to first item on quality-data mismatch
|
|
|
|
**Status:** Resolved.
|
|
The resolver now raises `DuplicateResolutionError` for missing or misaligned quality data, and the planner converts unresolved groups into explicit manual-review no-ops with metadata.
|
|
Evidence: `src/vlm/duplicate_resolve.py:44-58`, `src/vlm/planner.py:187-203`, `tests/test_duplicate_resolve.py:223-230`, `tests/test_planner.py:619-669`.
|
|
|
|
---
|
|
|
|
### F3) Duplicate resolution join relied on exact string path matches
|
|
|
|
**Status:** Resolved.
|
|
Planner duplicate matching now canonicalizes incoming path keys before lookup, including quality-comparison entries.
|
|
Evidence: `src/vlm/planner.py:42-57`, `src/vlm/planner.py:152-169`, `tests/test_planner.py:672-720`.
|
|
|
|
---
|
|
|
|
### F4) Quarantine category rejection raised exception while execute loop lacked per-op guard
|
|
|
|
**Status:** Resolved.
|
|
Unsupported quarantine categories now return failed `OperationResult`s, and batch execution wraps each operation with defensive containment so later operations still run.
|
|
Evidence: `src/vlm/quarantine.py:116-140`, `src/vlm/executor.py:108-130`, `tests/test_quarantine.py:84-110`, `tests/test_executor.py:951-1004`.
|
|
|
|
---
|
|
|
|
### F5) `find` non-zero exit still allowed stdout parsing without an explicit contract
|
|
|
|
**Status:** Resolved.
|
|
Scanner behavior is now explicit: partial stdout is accepted with a warning, while a non-zero exit with no paths yields an empty deterministic result.
|
|
Evidence: `src/vlm/scanner.py:158-214`, `tests/test_scanner.py:140-186`.
|
|
|
|
---
|
|
|
|
### F6) Optional dependency overlap (`textual` in both `dev` and `tui`)
|
|
|
|
**Status:** Still present as a low-priority packaging observation.
|
|
`textual` remains listed in both optional extras in `pyproject.toml:12-20`, but the higher-severity runtime bug is fixed because the CLI now lazily imports the TUI and `review_tui` guards Textual imports.
|
|
Evidence: `src/vlm/cli.py:567-602`, `src/vlm/review_tui.py:16-30`, `pyproject.toml:12-20`.
|
|
|
|
---
|
|
|
|
### F7) Plan JSON validation returned a dict-typed structure at the boundary
|
|
|
|
**Status:** Resolved.
|
|
The I/O layer now validates the JSON record and then constructs an `ExecutionPlan` through a single typed factory/load path.
|
|
Evidence: `src/vlm/io.py:247-326`, `tests/test_io.py:112-203`.
|
|
|
|
---
|
|
|
|
### F8) Unknown duplicate strategy defaulted to first item
|
|
|
|
**Status:** Resolved.
|
|
Unexpected low-level strategy values now raise `DuplicateResolutionError` instead of silently keeping the first duplicate.
|
|
Evidence: `src/vlm/duplicate_resolve.py:54-58`, `tests/test_duplicate_resolve.py:233-237`.
|
|
|
|
---
|
|
|
|
## Remaining recommendation
|
|
|
|
If desired, the next cleanup can be limited to dependency surface polish: remove or document the duplicated `textual` declaration in `pyproject.toml:12-20`. That is now a packaging clarity issue, not a runtime correctness issue.
|
|
|
|
## Closing
|
|
|
|
The refactor plan materially improved operational safety, duplicate-resolution determinism, scanner behavior, and plan-loading discipline. The repository is currently green at **507 passing tests**, and the substantive issues from the earlier review have been addressed.
|