refactor review-plan safety and validation
This commit is contained in:
windyboy
+84
-94
@@ -5,161 +5,151 @@
|
||||
|
||||
## Scope
|
||||
|
||||
This report was updated by verifying `docs/TECHNICAL_REVIEW.md` against the current codebase and aligning conclusions to evidence.
|
||||
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:
|
||||
- `docs/TECHNICAL_REVIEW.md:1-152`
|
||||
- `src/vlm/executor.py:110-112`
|
||||
- `src/vlm/executor.py:321-377`
|
||||
- `src/vlm/duplicate_resolve.py:36-48`
|
||||
- `src/vlm/planner.py:108-111`
|
||||
- `src/vlm/quarantine.py:116-129`
|
||||
- `src/vlm/scanner.py:178-192`
|
||||
- `src/vlm/io.py:225-251`
|
||||
- `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:
|
||||
- `uv run pytest -q` → **496 passed** (as recorded in `docs/TECHNICAL_REVIEW.md:10`).
|
||||
- `pytest -q` → **507 passed**.
|
||||
|
||||
---
|
||||
|
||||
## Overall Score
|
||||
|
||||
## **8.0 / 10**
|
||||
## **9.0 / 10**
|
||||
|
||||
### Score breakdown
|
||||
- **Module boundaries / pipeline:** 8.5/10
|
||||
- **Execution safety (filesystem):** 7.0/10
|
||||
- **Planning / duplicate logic:** 7.5/10
|
||||
- **Data I/O & validation:** 8.0/10
|
||||
- **Error handling consistency:** 7.5/10
|
||||
- **Test signal:** 8.5/10
|
||||
- **Dependencies:** 9.0/10
|
||||
- **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 are clean and explicit** (scan → parse → analyze → plan → execute).
|
||||
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. **Defensive safety measures exist in key areas** (destination root checks, quarantine manifest two-phase flow, JSON schema checks).
|
||||
- `src/vlm/executor.py:354-377`
|
||||
- `src/vlm/quarantine.py:221-320`
|
||||
- `src/vlm/io.py:225-251`
|
||||
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. **Testing coverage is broad and currently green.**
|
||||
- `docs/TECHNICAL_REVIEW.md:10`
|
||||
- `tests/test_path_safety.py:1-122`
|
||||
- `tests/test_duplicate_resolve.py:1-184`
|
||||
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`
|
||||
|
||||
---
|
||||
|
||||
## Verified findings
|
||||
## Status of previously reported findings
|
||||
|
||||
### F1) Move/Rename source path is not constrained to `library_root` (High)
|
||||
### F1) Move/Rename source path is not constrained to `library_root`
|
||||
|
||||
- `_perform_operation` validates destination under root, but does not enforce source under root before rename.
|
||||
- `src/vlm/executor.py:355-377`
|
||||
|
||||
**Impact:** A crafted/manual plan can attempt renames from paths outside managed library boundaries.
|
||||
**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 (Medium)
|
||||
### F2) `by_quality` silently falls back to first item on quality-data mismatch
|
||||
|
||||
- `choose_keep_index` returns index `0` if `quality_comparison` is missing/misaligned.
|
||||
- `src/vlm/duplicate_resolve.py:40-43`
|
||||
|
||||
**Impact:** Behavior degrades to input-order selection without explicit operator visibility.
|
||||
**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 relies on exact string path matches (Medium)
|
||||
### F3) Duplicate resolution join relied on exact string path matches
|
||||
|
||||
- Planner builds `path_to_index` from `str(vf.path)` and joins using exact string equality.
|
||||
- `src/vlm/planner.py:108-111`
|
||||
|
||||
**Impact:** Path normalization differences (symlink/case/serialization form) can silently exclude items from duplicate handling.
|
||||
**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 raises exception while execute loop lacks per-op guard (Medium)
|
||||
### F4) Quarantine category rejection raised exception while execute loop lacked per-op guard
|
||||
|
||||
- Quarantine rejects unsupported categories with `raise ValueError`.
|
||||
- `src/vlm/quarantine.py:116-129`
|
||||
- Execute loop iterates operations without local try/except around each operation call.
|
||||
- `src/vlm/executor.py:110-112`
|
||||
|
||||
**Impact:** One invalid quarantine operation can abort the run instead of being recorded as a single failed result.
|
||||
**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 allows stdout parsing (Low)
|
||||
### F5) `find` non-zero exit still allowed stdout parsing without an explicit contract
|
||||
|
||||
- Scanner logs non-zero return issues but still parses emitted stdout.
|
||||
- `src/vlm/scanner.py:178-192`
|
||||
|
||||
**Impact:** Partial scan results may be accepted without strict failure semantics.
|
||||
**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`) (Low)
|
||||
### F6) Optional dependency overlap (`textual` in both `dev` and `tui`)
|
||||
|
||||
- `textual` appears in both extras.
|
||||
- `pyproject.toml:13-20`
|
||||
|
||||
**Impact:** Minor install-surface ambiguity.
|
||||
**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 returns dict-typed structure at boundary (Informational)
|
||||
### F7) Plan JSON validation returned a dict-typed structure at the boundary
|
||||
|
||||
- `validate_plan_json` validates shape but returns plain `dict`.
|
||||
- `src/vlm/io.py:225-251`
|
||||
|
||||
**Impact:** Validator/model drift risk over time if object construction paths diverge.
|
||||
**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 defaults to first item (Informational)
|
||||
### F8) Unknown duplicate strategy defaulted to first item
|
||||
|
||||
- Unrecognized `strategy` falls through to `return 0`.
|
||||
- `src/vlm/duplicate_resolve.py:48`
|
||||
|
||||
**Impact:** Configuration typo can silently behave as first-seen policy.
|
||||
**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`.
|
||||
|
||||
---
|
||||
|
||||
## Advice (priority order)
|
||||
## Remaining recommendation
|
||||
|
||||
1. **Add source-root validation for move/rename execution path** and test for crafted plan source outside root.
|
||||
- `src/vlm/executor.py:335-377`
|
||||
|
||||
2. **Make `by_quality` mismatch explicit** (error/metadata flag/manual fallback), rather than silent index-0 default.
|
||||
- `src/vlm/duplicate_resolve.py:40-43`
|
||||
|
||||
3. **Unify quarantine error contract**: return failed `OperationResult` for unsupported categories (avoid run-aborting exception path).
|
||||
- `src/vlm/quarantine.py:116-129`
|
||||
- `src/vlm/executor.py:110-112`
|
||||
|
||||
4. **Normalize duplicate path keys consistently** across analysis emission and planning consumption.
|
||||
- `src/vlm/planner.py:108-111`
|
||||
|
||||
5. **Harden duplicate strategy validation in config** to reject unknown values at load/validate time.
|
||||
- `src/vlm/duplicate_resolve.py:36-48`
|
||||
- `src/vlm/config.py:224-340`
|
||||
|
||||
6. **Clarify or gate partial scan behavior on `find` failures** (strict mode or stronger warning semantics).
|
||||
- `src/vlm/scanner.py:178-192`
|
||||
|
||||
---
|
||||
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 codebase remains strong in structure and testing discipline. The key improvements are concentrated in execution guardrails and duplicate-resolution determinism. Addressing the top three items above should materially improve operational safety and predictability.
|
||||
|
||||
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.
|
||||
|
||||
Reference in New Issue
Block a user