diff --git a/.planning/phases/02-code-review-command/02-REVIEW.md b/.planning/phases/02-code-review-command/02-REVIEW.md index ad39a1d7..4052be58 100644 --- a/.planning/phases/02-code-review-command/02-REVIEW.md +++ b/.planning/phases/02-code-review-command/02-REVIEW.md @@ -1,56 +1,101 @@ --- phase: 02-code-review-command -reviewed: 2026-05-24T09:10:00Z +reviewed: 2026-06-04T15:30:00Z depth: standard -files_reviewed: 1 +files_reviewed: 2 files_reviewed_list: - - paperforge/plugin/package.json + - tests/test_ocr_artifacts.py + - tests/test_ocr.py findings: critical: 0 - warning: 0 + warning: 1 info: 0 - total: 0 -status: clean + total: 1 +status: issues_found --- -# Phase 02: Code Review Report +# Phase 2: Code Review Report -- Task 1 OCR Structured Pipeline -**Reviewed:** 2026-05-24T09:10:00Z +**Reviewed:** 2026-06-04T15:30:00Z **Depth:** standard -**Files Reviewed:** 1 -**Status:** clean +**Files Reviewed:** 2 +**Status:** issues_found ## Summary -Reviewed Task 1 implementation (commit `8982c67`) — installation of TypeScript + esbuild build system dependencies in `paperforge/plugin/package.json`, as specified in [plugin-ts-migration plan](docs/superpowers/plans/2026-05-24-plugin-ts-migration.md). +This task creates three TDD contract tests across two files that lock the Phase 1 OCR artifact contract (directory layout, version payload schema, postprocess artifact emission) in a failing state, exactly as specified in the plan. The tests will be made to pass by subsequent tasks. -**Result: All reviewed files meet quality standards. No issues found.** +The implementation is correct in intent and faithfully follows the plan. However, one code quality defect was found: a missing `Path` import in `tests/test_ocr.py` that creates a fragility when the annotation is evaluated on Python < 3.14 and violates the project's established convention (every other test file imports `Path`). -### What was verified +All 3 new tests fail as expected in the current environment: +- 2 fail with `ModuleNotFoundError` (target module does not exist yet) -- correct +- 1 fails with `AssertionError` (artifact files not written yet) -- correct -| Check | Result | -|-------|--------| -| **Plan alignment** | EXACT match — every version, script, and dependency matches the plan verbatim | -| **Scripts added** | `dev` and `build` — exactly as specified | -| **DevDeps added** | `typescript ^5.4.0`, `esbuild ^0.25.0`, `builtin-modules ^3.3.0`, `@types/node ^20.0.0` | -| **Existing deps preserved** | `vitest`, `obsidian-test-mocks`, `jsdom`, `obsidian` — untouched | -| **`package-lock.json`** | Committed alongside package.json (659 insertions, 120 deletions) | -| **`npm install`** | Clean — all 4 packages installed, `npm ci --dry-run` reports "up to date" | -| **`npm audit`** | 0 vulnerabilities (dev-only deps are expected security-wise) | -| **Installed versions** | TypeScript 5.9.3, esbuild 0.25.12, builtin-modules 3.3.0, @types/node 20.19.41 | -| **Commit message** | `"chore(plugin): add esbuild, typescript, @types/node devDeps"` — matches plan | -| **No extra files** | Only `package.json` and `package-lock.json` were modified | -| **Version reasonableness** | All ranges are stable and compatible with the planned tech stack | +All 4 pre-existing tests in `tests/test_ocr.py` continue to pass -- no regression detected. -### Specific concerns checked and cleared +### Test Failure Diagnostics -- **`esbuild.config.mjs` not yet existing**: Expected — it will be added in Task 3. The scripts reference it preemptively, which is by design. -- **Single-dash `-noEmit` / `-skipLibCheck`**: Intentional — matches the official Obsidian sample plugin convention, as specified in the plan. -- **`builtin-modules` necessity**: Required by the esbuild config (Task 3) to extern Node.js built-ins at bundle time. Correct inclusion. -- **`@types/node` scope**: `^20.0.0` correctly restricts to Node 20.x LTS types. No version creep risk. +| Test | Failure Type | Expected? | +|------|-------------|-----------| +| `test_phase1_artifact_layout_is_paper_local` | `ModuleNotFoundError: No module named 'paperforge.worker.ocr_artifacts'` | YES -- TDD contract, module not yet created | +| `test_raw_and_derived_version_payloads_have_separate_namespaces` | `ModuleNotFoundError: No module named 'paperforge.worker.ocr_artifacts'` | YES -- TDD contract, module not yet created | +| `test_postprocess_writes_phase1_artifacts` | `AssertionError: assert False` at line 169 | YES -- `postprocess_ocr_result()` does not yet write Phase 1 artifacts | + +## Warnings + +### WR-01: Missing `Path` import in `test_ocr.py` creates annotation fragility + +**File:** `tests/test_ocr.py:154` +**Issue:** The function signature `def test_postprocess_writes_phase1_artifacts(tmp_path: Path) -> None:` references `Path` in a type annotation, but `Path` is not imported in `tests/test_ocr.py`. This file has zero module-level imports -- the only test file in the project with this characteristic. Every other test file consistently uses `from pathlib import Path`. + +This works in Python 3.14 due to PEP 649 (deferred annotation evaluation), but it: +- **Fails with `NameError` on Python < 3.14** if the module is imported without `from __future__ import annotations` +- Creates a hidden dependency on the conftest's import order and PEP 649 being active +- Violates the project's established convention (all other test files with `Path` annotations import it explicitly) + +**Fix:** Add `from pathlib import Path` to `tests/test_ocr.py` at module level. This is consistent with the pattern used in `tests/test_ocr_redo.py`, `tests/test_ocr_artifacts.py`, `tests/test_context.py`, and every other test file in the project. + +```python +# Add to tests/test_ocr.py, before the first test function: +from pathlib import Path +``` + +Alternatively, if the project intentionally uses annotation-only mode for all test files, add `from __future__ import annotations` at the top of `tests/test_ocr.py` (matching `tests/test_ocr_artifacts.py` and `tests/conftest.py`). + +## Assessment + +### Strengths + +1. **Plan alignment:** The implementation matches the plan exactly -- both `test_ocr_artifacts.py` tests and the `test_ocr.py` addition are verbatim from the spec. + +2. **Design alignment:** The contract tests correctly reflect the design spec (`docs/superpowers/specs/2026-06-04-ocr-structured-pipeline-design.md`): + - `raw/raw_meta.json` and `raw/source_metadata.json` match Layer 0/1 artifacts + - `canonical/blocks.raw.jsonl` matches Layer 2 + - `structure/blocks.structured.jsonl` matches Layer 3 + - `raw_version`/`derived_version` payloads match Section 6 version model + +3. **Well-scoped scope:** The task did not leak into implementation work -- no production source files were modified, no `ocr_artifacts.py` was prematurely stubbed. + +4. **Explicit path assertions:** Using `.as_posix().endswith(...)` in `test_ocr_artifacts.py` correctly handles Windows vs POSIX path separators, making the tests cross-platform. + +5. **Clean new file:** `tests/test_ocr_artifacts.py` is small (29 lines), has a single responsibility (contract tests), and is independently importable. + +### Defects + +| ID | Severity | Description | File:Line | +|----|----------|-------------|-----------| +| WR-01 | Warning | Missing `Path` import in `test_ocr.py` | `tests/test_ocr.py:154` | + +### Verification + +- Module-level imports absent from `tests/test_ocr.py`: confirmed by `rg` (zero results) +- All other 21 test files import `Path` from `pathlib`: confirmed by `Select-String` across `tests/*.py` +- All 3 new tests fail as expected: confirmed by running `pytest` against both files +- All 4 pre-existing tests still pass: confirmed by `pytest` output +- Design spec alignment: confirmed by reading the design doc --- -_Reviewed: 2026-05-24T09:10:00Z_ +_Reviewed: 2026-06-04T15:30:00Z_ _Reviewer: VT-OS/OPENCODE (gsd-code-reviewer)_ _Depth: standard_ diff --git a/paperforge/commands/embed.py b/paperforge/commands/embed.py index c8bd7e12..1708496f 100644 --- a/paperforge/commands/embed.py +++ b/paperforge/commands/embed.py @@ -140,8 +140,8 @@ def run(args: argparse.Namespace) -> int: total = len(done_papers) print(f"EMBED_START:{total}", flush=True) - import os as _os import gc as _gc + import os as _os _now = __import__('datetime').datetime.now(__import__('datetime').timezone.utc).isoformat papers_embedded = 0 diff --git a/paperforge/query_planning.py b/paperforge/query_planning.py index 81d2d3d8..741886d3 100644 --- a/paperforge/query_planning.py +++ b/paperforge/query_planning.py @@ -4,7 +4,6 @@ import re from dataclasses import dataclass from pathlib import Path - HIGH_SIGNAL_PRIORITY = [ "doi", "zotero_key", diff --git a/paperforge/worker/asset_index.py b/paperforge/worker/asset_index.py index 326efa3e..063f08ac 100644 --- a/paperforge/worker/asset_index.py +++ b/paperforge/worker/asset_index.py @@ -267,7 +267,6 @@ def _build_entry(item: dict, vault: Path, paths: dict, domain: str, zotero_dir: Lazy imports inside avoid circular dependencies with ``sync.py``. """ # Lazy imports to avoid circular deps with sync.py - import shutil from paperforge import __version__ as PAPERFORGE_VERSION from paperforge.adapters.obsidian_frontmatter import has_deep_reading_content diff --git a/paperforge/worker/deep_reading.py b/paperforge/worker/deep_reading.py index eb6523a6..94b80fbe 100644 --- a/paperforge/worker/deep_reading.py +++ b/paperforge/worker/deep_reading.py @@ -3,6 +3,7 @@ from __future__ import annotations import logging from pathlib import Path +from paperforge.adapters.obsidian_frontmatter import has_deep_reading_content from paperforge.worker._domain import load_domain_config from paperforge.worker._utils import ( get_analyze_queue, @@ -10,7 +11,6 @@ from paperforge.worker._utils import ( ) from paperforge.worker.asset_index import refresh_index_entry from paperforge.worker.base_views import ensure_base_views -from paperforge.adapters.obsidian_frontmatter import has_deep_reading_content logger = logging.getLogger(__name__) diff --git a/tests/chaos/conftest.py b/tests/chaos/conftest.py index e7825ffa..47102a18 100644 --- a/tests/chaos/conftest.py +++ b/tests/chaos/conftest.py @@ -6,12 +6,10 @@ ALL chaos test vaults MUST use tmp_path and include the isolation assertion: from __future__ import annotations -import json import os import random import shutil import stat -import string import subprocess import sys from pathlib import Path diff --git a/tests/chaos/test_corrupted_inputs.py b/tests/chaos/test_corrupted_inputs.py index df03bef8..c9421617 100644 --- a/tests/chaos/test_corrupted_inputs.py +++ b/tests/chaos/test_corrupted_inputs.py @@ -16,10 +16,8 @@ from tests.chaos.conftest import ( corrupt_pdf, create_broken_meta_json, setup_vault_from_export, - strip_frontmatter_fields, ) - pytestmark = pytest.mark.chaos diff --git a/tests/chaos/test_filesystem_errors.py b/tests/chaos/test_filesystem_errors.py index 6a361144..d7781ddb 100644 --- a/tests/chaos/test_filesystem_errors.py +++ b/tests/chaos/test_filesystem_errors.py @@ -12,7 +12,6 @@ from pathlib import Path import pytest - pytestmark = pytest.mark.chaos diff --git a/tests/chaos/test_network_failures.py b/tests/chaos/test_network_failures.py index 85d3b524..7781972a 100644 --- a/tests/chaos/test_network_failures.py +++ b/tests/chaos/test_network_failures.py @@ -15,7 +15,6 @@ from pathlib import Path import pytest - pytestmark = pytest.mark.chaos # Short timeout for network tests — the OCR module has retry+backoff logic diff --git a/tests/cli/conftest.py b/tests/cli/conftest.py index 7c6cb45e..65e05e78 100644 --- a/tests/cli/conftest.py +++ b/tests/cli/conftest.py @@ -80,7 +80,7 @@ def mock_ocr_backend(): This fixture is context-managed — use it with 'with' statement. """ import json - from pathlib import Path + import responses as _responses ocr_fixtures_dir = Path(__file__).resolve().parent.parent.parent / "fixtures" / "ocr" @@ -129,5 +129,4 @@ def mock_ocr_backend(): @pytest.fixture def snapshot_dir(): """Return path to the snapshots directory.""" - from pathlib import Path return Path(__file__).resolve().parent.parent.parent / "fixtures" / "snapshots" diff --git a/tests/cli/test_contract_helpers.py b/tests/cli/test_contract_helpers.py index 269e2760..b7808976 100644 --- a/tests/cli/test_contract_helpers.py +++ b/tests/cli/test_contract_helpers.py @@ -4,7 +4,6 @@ from __future__ import annotations import json import re -from pathlib import Path def normalize_snapshot(output: str, vault_path: str | None = None) -> str: diff --git a/tests/cli/test_error_codes.py b/tests/cli/test_error_codes.py index fc40ba8b..d51371f5 100644 --- a/tests/cli/test_error_codes.py +++ b/tests/cli/test_error_codes.py @@ -2,8 +2,6 @@ from __future__ import annotations -import pytest - class TestExitCodes: """Contract: CLI commands exit with correct exit codes.""" diff --git a/tests/e2e/test_multi_domain_sync.py b/tests/e2e/test_multi_domain_sync.py index 2b2a43ab..f7350ad6 100644 --- a/tests/e2e/test_multi_domain_sync.py +++ b/tests/e2e/test_multi_domain_sync.py @@ -3,11 +3,9 @@ from __future__ import annotations import json -from pathlib import Path import pytest - pytestmark = pytest.mark.e2e diff --git a/tests/e2e/test_ocr_e2e.py b/tests/e2e/test_ocr_e2e.py index a2509704..50ec0a5c 100644 --- a/tests/e2e/test_ocr_e2e.py +++ b/tests/e2e/test_ocr_e2e.py @@ -10,7 +10,6 @@ from pathlib import Path import pytest - pytestmark = pytest.mark.e2e diff --git a/tests/e2e/test_status_doctor_repair.py b/tests/e2e/test_status_doctor_repair.py index 31663cc2..51e1d0d6 100644 --- a/tests/e2e/test_status_doctor_repair.py +++ b/tests/e2e/test_status_doctor_repair.py @@ -6,7 +6,6 @@ import json import pytest - pytestmark = pytest.mark.e2e diff --git a/tests/e2e/test_sync_pipeline.py b/tests/e2e/test_sync_pipeline.py index 6c574bd0..76869dda 100644 --- a/tests/e2e/test_sync_pipeline.py +++ b/tests/e2e/test_sync_pipeline.py @@ -3,11 +3,9 @@ from __future__ import annotations import json -from pathlib import Path import pytest - pytestmark = pytest.mark.e2e diff --git a/tests/journey/conftest.py b/tests/journey/conftest.py index 847394af..56938173 100644 --- a/tests/journey/conftest.py +++ b/tests/journey/conftest.py @@ -91,9 +91,10 @@ def journey_established_vault(tmp_path: Path) -> tuple[Path, object]: Returns: (vault_path, vault_builder) tuple so tests can inspect or extend. """ - from fixtures.vault_builder import VaultBuilder import json + from fixtures.vault_builder import VaultBuilder + builder = VaultBuilder() vault = Path(builder.build("full")) diff --git a/tests/journey/test_daily_workflow.py b/tests/journey/test_daily_workflow.py index 95411bbb..841c85e2 100644 --- a/tests/journey/test_daily_workflow.py +++ b/tests/journey/test_daily_workflow.py @@ -6,13 +6,11 @@ Simulates an existing user: established vault with papers -> add new paper -> sy from __future__ import annotations import json -import shutil from pathlib import Path import pytest import yaml - pytestmark = pytest.mark.journey @@ -156,8 +154,8 @@ def test_existing_user_adds_paper( # Verify new note frontmatter fm = _read_frontmatter(new_note) assert fm.get("zotero_key") == new_key, f"Expected zotero_key {new_key}, got {fm.get('zotero_key')}" - assert "domain" in fm and fm.get("domain"), f"Missing or empty domain in new note" - assert "has_pdf" in fm, f"Missing has_pdf in new note" + assert "domain" in fm and fm.get("domain"), "Missing or empty domain in new note" + assert "has_pdf" in fm, "Missing has_pdf in new note" # Verify existing notes are unchanged (same frontmatter hash) for n in current_notes: @@ -196,8 +194,8 @@ def test_existing_user_adds_paper( # Verify new note frontmatter fm = _read_frontmatter(new_note) assert fm.get("zotero_key") == new_key, f"Expected zotero_key {new_key}, got {fm.get('zotero_key')}" - assert "domain" in fm and fm.get("domain"), f"Missing or empty domain in new note" - assert "has_pdf" in fm, f"Missing has_pdf in new note" + assert "domain" in fm and fm.get("domain"), "Missing or empty domain in new note" + assert "has_pdf" in fm, "Missing has_pdf in new note" assert fm.get("ocr_status") in ("pending",), f"Expected pending ocr_status, got {fm.get('ocr_status')}" # Verify existing notes are unchanged (same frontmatter hash) diff --git a/tests/journey/test_onboarding.py b/tests/journey/test_onboarding.py index 6369c4a0..6772d21a 100644 --- a/tests/journey/test_onboarding.py +++ b/tests/journey/test_onboarding.py @@ -5,7 +5,6 @@ Simulates a brand-new user: fresh vault -> sync -> OCR -> analyze -> deep-read r from __future__ import annotations -import json from pathlib import Path from typing import Any @@ -14,7 +13,6 @@ import yaml from fixtures.ocr.mock_ocr_backend import mock_ocr_success - pytestmark = pytest.mark.journey diff --git a/tests/sandbox/precise_batch.py b/tests/sandbox/precise_batch.py index 473c35eb..e157e7e0 100644 --- a/tests/sandbox/precise_batch.py +++ b/tests/sandbox/precise_batch.py @@ -1,6 +1,5 @@ """Precise batch defect report.""" import re -import json import sys from pathlib import Path @@ -112,4 +111,4 @@ for key in papers: verdict = ", ".join(issues) if issues else "CLEAN" print(f" {key}: {img_num}i/{cap_num}c | {verdict}") -print(f"\nDone.") +print("\nDone.") diff --git a/tests/test_asset_index_integration.py b/tests/test_asset_index_integration.py index 37a665d9..9dfb3f67 100644 --- a/tests/test_asset_index_integration.py +++ b/tests/test_asset_index_integration.py @@ -13,9 +13,6 @@ from __future__ import annotations import json from pathlib import Path -import pytest - - # --------------------------------------------------------------------------- # Helpers (matching test_asset_index.py pattern) # --------------------------------------------------------------------------- @@ -113,7 +110,6 @@ def _setup_incremental_vault( Path to the vault root. """ from paperforge.worker.asset_index import ( - build_envelope, get_index_path, ) @@ -217,7 +213,6 @@ class TestIncrementalRefreshBehavior: def test_incremental_refresh_fallback_on_legacy(self, tmp_path: Path) -> None: """refresh_index_entry falls back to build_index when index is legacy format.""" from paperforge.worker.asset_index import ( - build_envelope, get_index_path, is_legacy_format, read_index, @@ -298,8 +293,6 @@ class TestWorkspacePaths: def test_workspace_paths_in_entry(self, tmp_path: Path) -> None: """Each entry in the built index contains all 5 workspace path fields.""" from paperforge.worker.asset_index import ( - build_envelope, - get_index_path, read_index, refresh_index_entry, ) @@ -464,7 +457,7 @@ class TestDerivedStateFields: assert "note_health" in health assert "asset_health" in health for v in health.values(): - assert isinstance(v, str) and len(v) > 0, f"health value should be non-empty string" + assert isinstance(v, str) and len(v) > 0, "health value should be non-empty string" def test_maturity_structure(self, tmp_path: Path) -> None: """maturity field is a dict with level (int 1-4), level_name (str), checks (4 bools), blocking.""" diff --git a/tests/test_asset_state.py b/tests/test_asset_state.py index b3e50bb4..12a60b7d 100644 --- a/tests/test_asset_state.py +++ b/tests/test_asset_state.py @@ -6,11 +6,9 @@ Test entries are plain dicts matching the canonical index entry shape. from __future__ import annotations -import pytest - from paperforge.worker.asset_state import ( - compute_lifecycle, compute_health, + compute_lifecycle, compute_maturity, compute_next_step, ) diff --git a/tests/test_config.py b/tests/test_config.py index 04e93486..8889be4b 100644 --- a/tests/test_config.py +++ b/tests/test_config.py @@ -508,6 +508,7 @@ def test_get_paperforge_schema_version_defaults_to_1(tmp_path: Path): def test_get_paperforge_schema_version_reads_2(tmp_path: Path): """get_paperforge_schema_version returns 2 when schema_version is '2'.""" import json + from paperforge.config import get_paperforge_schema_version vault = tmp_path / "schema_v2" @@ -539,6 +540,7 @@ def test_load_vault_config_excludes_schema_version(tmp_path: Path): def test_migrate_legacy_top_level_keys(tmp_path: Path): """Legacy top-level path keys are migrated to vault_config block.""" import json + from paperforge.config import migrate_paperforge_json vault = tmp_path / "legacy_migrate" @@ -569,6 +571,7 @@ def test_migrate_legacy_top_level_keys(tmp_path: Path): def test_migrate_idempotent(tmp_path: Path): """Already-migrated files return False (no-op).""" import json + from paperforge.config import migrate_paperforge_json vault = tmp_path / "already_migrated" @@ -594,6 +597,7 @@ def test_migrate_no_file(tmp_path: Path): def test_migrate_non_path_keys_survive(tmp_path: Path): """Non-path top-level keys survive migration in output root.""" import json + from paperforge.config import migrate_paperforge_json vault = tmp_path / "non_path_survive" @@ -619,6 +623,7 @@ def test_migrate_non_path_keys_survive(tmp_path: Path): def test_migrate_no_vault_config_block_creates_it(tmp_path: Path): """Top-level keys without vault_config block create a new vault_config.""" import json + from paperforge.config import migrate_paperforge_json vault = tmp_path / "creates_vc" diff --git a/tests/test_context.py b/tests/test_context.py index c97a2094..e46a0a2d 100644 --- a/tests/test_context.py +++ b/tests/test_context.py @@ -10,9 +10,6 @@ import argparse import json from pathlib import Path -import pytest - - # --------------------------------------------------------------------------- # Helpers # --------------------------------------------------------------------------- diff --git a/tests/test_discussion.py b/tests/test_discussion.py index 56840d75..05c5fed6 100644 --- a/tests/test_discussion.py +++ b/tests/test_discussion.py @@ -8,13 +8,10 @@ from __future__ import annotations import json import os -import tempfile from pathlib import Path import filelock -import pytest - from paperforge.worker.discussion import record_session # --------------------------------------------------------------------------- diff --git a/tests/test_e2e_pipeline.py b/tests/test_e2e_pipeline.py index 2bc24c18..d65f5997 100644 --- a/tests/test_e2e_pipeline.py +++ b/tests/test_e2e_pipeline.py @@ -14,8 +14,6 @@ from __future__ import annotations import re from pathlib import Path -import pytest - from paperforge.config import paperforge_paths from paperforge.worker.asset_index import build_index, read_index from paperforge.worker.sync import run_index_refresh, run_selection_sync diff --git a/tests/test_literature_hub_regressions.py b/tests/test_literature_hub_regressions.py index 3ea42658..8a8934dd 100644 --- a/tests/test_literature_hub_regressions.py +++ b/tests/test_literature_hub_regressions.py @@ -7,7 +7,6 @@ from pathlib import Path import pytest - VAULT = Path(r"D:\L\OB\Literature-hub") diff --git a/tests/test_migration.py b/tests/test_migration.py index fa93bf13..15e88400 100644 --- a/tests/test_migration.py +++ b/tests/test_migration.py @@ -7,12 +7,10 @@ idempotency, backward compatibility, and run_index_refresh integration. from __future__ import annotations import json -import os from pathlib import Path import pytest - # --------------------------------------------------------------------------- # Helpers # --------------------------------------------------------------------------- diff --git a/tests/test_ocr.py b/tests/test_ocr.py index d5de9e1d..a2e68e6b 100644 --- a/tests/test_ocr.py +++ b/tests/test_ocr.py @@ -293,8 +293,6 @@ def test_postprocess_writes_phase1_artifacts(tmp_path: Path) -> None: assert (ocr_dir / "canonical" / "blocks.raw.jsonl").exists() assert (ocr_dir / "structure" / "blocks.structured.jsonl").exists() - import json - meta = json.loads((ocr_dir / "meta.json").read_text(encoding="utf-8")) assert "raw_version" in meta assert meta["raw_version"]["ocr_model"] == "PaddleOCR-VL-1.6" @@ -329,3 +327,29 @@ def test_postprocess_writes_resolved_metadata(tmp_path: Path) -> None: # Phase 2: resolved_metadata.json does not exist yet — this test will fail # The assertion goes here as a contract test assert (ocr_dir / "metadata" / "resolved_metadata.json").exists() + + +def test_postprocess_writes_figure_inventory(tmp_path: Path) -> None: + import json + + from paperforge.worker.ocr import postprocess_ocr_result + + vault = tmp_path / "vault" + vault.mkdir() + (vault / "paperforge.json").write_text( + json.dumps({"vault_config": {"system_dir": "System", "resources_dir": "Resources"}}), + encoding="utf-8", + ) + ocr_root = vault / "System" / "PaperForge" / "ocr" + ocr_root.mkdir(parents=True) + ocr_dir = ocr_root / "FIG001" + ocr_dir.mkdir() + (ocr_dir / "meta.json").write_text( + '{"zotero_key":"FIG001","ocr_status":"done","ocr_model":"PaddleOCR"}', + encoding="utf-8", + ) + + _, _, _, _ = postprocess_ocr_result(vault, "FIG001", []) + + # Phase 2: figure_inventory.json does not exist yet -- test will fail + assert (ocr_dir / "structure" / "figure_inventory.json").exists() diff --git a/tests/test_ocr_artifacts.py b/tests/test_ocr_artifacts.py index 63a9dbf9..48ca271c 100644 --- a/tests/test_ocr_artifacts.py +++ b/tests/test_ocr_artifacts.py @@ -1,4 +1,5 @@ from __future__ import annotations + from pathlib import Path diff --git a/tests/test_ocr_figures.py b/tests/test_ocr_figures.py new file mode 100644 index 00000000..7a856ffd --- /dev/null +++ b/tests/test_ocr_figures.py @@ -0,0 +1,76 @@ +"""Phase 2 contract tests for figure inventory. + +paperforge.worker.ocr_figures does not exist yet -- tests will fail until +Task 5 implements the module. +""" + +from __future__ import annotations + + +def test_formal_figure_count_is_based_on_legends_not_raw_images() -> None: + from paperforge.worker.ocr_figures import build_figure_inventory + + structured_blocks = [ + { + "paper_id": "KEY001", + "page": 3, + "block_id": "p3_b21", + "role": "figure_caption", + "text": "Figure 1. Left column figure.", + "bbox": [66, 446, 559, 628], + }, + { + "paper_id": "KEY001", + "page": 3, + "block_id": "p3_b22", + "role": "figure_asset", + "text": "", + "bbox": [80, 116, 546, 434], + }, + { + "paper_id": "KEY001", + "page": 3, + "block_id": "p3_b23", + "role": "figure_asset", + "text": "", + "bbox": [598, 114, 1063, 493], + }, + ] + + inventory = build_figure_inventory(structured_blocks) + + assert inventory["official_figure_count"] == 1 + assert len(inventory["figure_legends"]) == 1 + + +def test_figure_inventory_includes_all_sections() -> None: + from paperforge.worker.ocr_figures import build_figure_inventory + + inventory = build_figure_inventory([]) + + assert "figure_legends" in inventory + assert "figure_assets" in inventory + assert "matched_figures" in inventory + assert "unmatched_legends" in inventory + assert "unmatched_assets" in inventory + assert "official_figure_count" in inventory + + +def test_unmatched_assets_are_preserved() -> None: + from paperforge.worker.ocr_figures import build_figure_inventory + + structured_blocks = [ + { + "paper_id": "KEY001", + "page": 5, + "block_id": "p5_b10", + "role": "figure_asset", + "text": "", + "bbox": [100, 100, 500, 400], + }, + ] + + inventory = build_figure_inventory(structured_blocks) + + assert inventory["official_figure_count"] == 0 + assert len(inventory["unmatched_assets"]) == 1 diff --git a/tests/test_ocr_integration_fixtures.py b/tests/test_ocr_integration_fixtures.py index 90b62526..26b63b11 100644 --- a/tests/test_ocr_integration_fixtures.py +++ b/tests/test_ocr_integration_fixtures.py @@ -134,3 +134,26 @@ def test_fixture_raw_blocks_nonzero(tmp_path) -> None: write_raw_blocks_jsonl(out_path, rows) assert out_path.exists() assert out_path.stat().st_size > 0 + + +# Phase 2: forward-looking contract test; fails until ocr_figures module exists +def test_fixture_figure_inventory_basic(tmp_path) -> None: + """Verify figure inventory can run against a real OCR fixture.""" + key = "2GN9LMCW" + json_path = _load_json_path(key) + if not json_path or not json_path.exists(): + pytest.skip("fixture not available") + + data = json.loads(json_path.read_text(encoding="utf-8")) + from paperforge.worker.ocr_blocks import build_raw_blocks_for_result_lines, build_structured_blocks + + raw_blocks = build_raw_blocks_for_result_lines(key, data) + structured = build_structured_blocks(raw_blocks) + + from paperforge.worker.ocr_figures import build_figure_inventory + + inventory = build_figure_inventory(structured) + + assert isinstance(inventory["official_figure_count"], int) + assert isinstance(inventory["figure_legends"], list) + assert isinstance(inventory["figure_assets"], list) diff --git a/tests/test_ocr_state_machine.py b/tests/test_ocr_state_machine.py index 2f9f1eb3..b3e3371f 100644 --- a/tests/test_ocr_state_machine.py +++ b/tests/test_ocr_state_machine.py @@ -895,7 +895,6 @@ class TestOcrEdgeCases: """max_items=1 throttles concurrency but never leaves items unprocessed.""" vault, ocr_root, exports, library_records = _make_vault(tmp_path) paths = _mock_paths(vault, ocr_root, exports, library_records) - import os as _os with patch.dict( "os.environ", @@ -1118,12 +1117,10 @@ class TestOcrEdgeCases: def test_full_cycle_from_pending_to_done(self, tmp_path: Path) -> None: """Happy path: pending -> upload -> queued -> poll done -> postprocess -> done.""" + import sys as _sys from pathlib import Path as P - import sys as _sys - _sys.path.insert(0, str(P(__file__).resolve().parent.parent)) - from paperforge.worker.ocr import postprocess_ocr_result vault, ocr_root, exports, library_records = _make_vault(tmp_path) paths = _mock_paths(vault, ocr_root, exports, library_records) diff --git a/tests/test_plugin_install_bootstrap.py b/tests/test_plugin_install_bootstrap.py index 78e5bad1..94806760 100644 --- a/tests/test_plugin_install_bootstrap.py +++ b/tests/test_plugin_install_bootstrap.py @@ -2,7 +2,6 @@ from __future__ import annotations from pathlib import Path - REPO_ROOT = Path(__file__).resolve().parent.parent PLUGIN_MAIN = REPO_ROOT / "paperforge" / "plugin" / "main.js" diff --git a/tests/test_query_plan.py b/tests/test_query_plan.py index f1a32bc9..66b5e058 100644 --- a/tests/test_query_plan.py +++ b/tests/test_query_plan.py @@ -4,8 +4,8 @@ from pathlib import Path import pytest -from paperforge.query_planning import build_query_plan, enrich_query_plan_with_runtime from paperforge.commands.query_plan import run as query_plan_run +from paperforge.query_planning import build_query_plan, enrich_query_plan_with_runtime class _Args: diff --git a/tests/test_repair.py b/tests/test_repair.py index a55e190b..dc408520 100644 --- a/tests/test_repair.py +++ b/tests/test_repair.py @@ -521,6 +521,7 @@ class TestRepairExceptionLogging: def test_index_write_failure_first_branch_logs_warning(self, tmp_path, caplog, monkeypatch): """Index write exception in first --fix branch at line 306 logs warning.""" import logging + import paperforge.worker.repair as repair_mod caplog.set_level(logging.WARNING, logger="paperforge.worker.repair") @@ -544,6 +545,7 @@ class TestRepairExceptionLogging: def test_index_write_failure_second_branch_logs_warning(self, tmp_path, caplog, monkeypatch): """Index write exception in second --fix branch at line 347 logs warning.""" import logging + import paperforge.worker.repair as repair_mod caplog.set_level(logging.WARNING, logger="paperforge.worker.repair") @@ -567,6 +569,7 @@ class TestRepairExceptionLogging: def test_meta_write_failure_logs_warning(self, tmp_path, caplog, monkeypatch): """Meta write exception in second --fix branch at line 355 logs warning.""" import logging + import paperforge.worker.repair as repair_mod caplog.set_level(logging.WARNING, logger="paperforge.worker.repair") diff --git a/tests/test_setup_wizard.py b/tests/test_setup_wizard.py index cdefa82e..f80b49ad 100644 --- a/tests/test_setup_wizard.py +++ b/tests/test_setup_wizard.py @@ -24,9 +24,8 @@ if str(REPO_ROOT) not in sys.path: sys.path.insert(0, str(REPO_ROOT)) # Import standalone functions and classes (not Textual-dependent) -from paperforge.setup_wizard import CheckResult, EnvChecker, _find_vault from paperforge.services.skill_deploy import AGENT_SKILL_DIRS - +from paperforge.setup_wizard import CheckResult, EnvChecker, _find_vault # =================================================================== # AGENT_SKILL_DIRS (imported from skill_deploy, flat dict) @@ -288,8 +287,8 @@ class TestEnvCheckerCheckJson: def test_headless_setup_claude_skill_directory(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> None: """Claude Code uses skill_directory format — creates .claude/skills/pf-deep/SKILL.md etc.""" - from paperforge.setup_wizard import headless_setup import paperforge.setup_wizard as sw + from paperforge.setup_wizard import headless_setup def patched_check_deps(self) -> CheckResult: r = CheckResult("Dependencies") @@ -320,8 +319,8 @@ def test_headless_setup_claude_skill_directory(tmp_path: Path, monkeypatch: pyte def test_headless_setup_opencode_flat_command(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> None: """OpenCode deploys literature-qa skill to skills dir; flat commands are bundled in SKILL.md.""" - from paperforge.setup_wizard import headless_setup import paperforge.setup_wizard as sw + from paperforge.setup_wizard import headless_setup def patched_check_deps(self) -> CheckResult: r = CheckResult("Dependencies") @@ -349,8 +348,8 @@ def test_headless_setup_opencode_flat_command(tmp_path: Path, monkeypatch: pytes def test_headless_setup_codex_skill_directory(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> None: """Codex deploys literature-qa skill under .codex/skills.""" - from paperforge.setup_wizard import headless_setup import paperforge.setup_wizard as sw + from paperforge.setup_wizard import headless_setup def patched_check_deps(self) -> CheckResult: r = CheckResult("Dependencies") @@ -378,8 +377,8 @@ def test_headless_setup_codex_skill_directory(tmp_path: Path, monkeypatch: pytes def test_headless_setup_cline_rules_file(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> None: """Cline deploys literature-qa skill under .clinerules.""" - from paperforge.setup_wizard import headless_setup import paperforge.setup_wizard as sw + from paperforge.setup_wizard import headless_setup def patched_check_deps(self) -> CheckResult: r = CheckResult("Dependencies") @@ -403,14 +402,14 @@ def test_headless_setup_cline_rules_file(tmp_path: Path, monkeypatch: pytest.Mon clinerules = tmp_path / ".clinerules" assert clinerules.exists(), f".clinerules dir not created: {clinerules}" skill_dir = clinerules / "literature-qa" - assert skill_dir.exists(), f"literature-qa subdir not created under .clinerules" + assert skill_dir.exists(), "literature-qa subdir not created under .clinerules" assert (skill_dir / "references" / "deep-subagent.md").exists(), "deep-subagent.md not created" def test_headless_setup_preserves_existing_files(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> None: """Setup should create missing files without overwriting existing user content.""" - from paperforge.setup_wizard import headless_setup import paperforge.setup_wizard as sw + from paperforge.setup_wizard import headless_setup def patched_check_deps(self) -> CheckResult: r = CheckResult("Dependencies") @@ -461,8 +460,8 @@ def test_headless_setup_preserves_existing_files(tmp_path: Path, monkeypatch: py def test_headless_setup_allows_empty_paddleocr_key(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> None: - from paperforge.setup_wizard import headless_setup import paperforge.setup_wizard as sw + from paperforge.setup_wizard import headless_setup def patched_check_deps(self) -> CheckResult: r = CheckResult("Dependencies") diff --git a/tests/test_skill_graph_contracts.py b/tests/test_skill_graph_contracts.py index ae2ad048..404dfe65 100644 --- a/tests/test_skill_graph_contracts.py +++ b/tests/test_skill_graph_contracts.py @@ -1,4 +1,5 @@ from __future__ import annotations + from pathlib import Path REPO_ROOT = Path(__file__).resolve().parent.parent diff --git a/tests/test_skill_graph_layout.py b/tests/test_skill_graph_layout.py index 043ea8cf..067e0ef6 100644 --- a/tests/test_skill_graph_layout.py +++ b/tests/test_skill_graph_layout.py @@ -1,4 +1,5 @@ from __future__ import annotations + from pathlib import Path REPO_ROOT = Path(__file__).resolve().parent.parent diff --git a/tests/test_status.py b/tests/test_status.py index 01a66182..e0db7378 100644 --- a/tests/test_status.py +++ b/tests/test_status.py @@ -9,9 +9,6 @@ from __future__ import annotations import json from pathlib import Path -import pytest - - # --------------------------------------------------------------------------- # Helpers # --------------------------------------------------------------------------- @@ -179,11 +176,11 @@ class TestDoctorIndexHealth: def _run_doctor(self, vault: Path) -> str: """Run doctor and return captured stdout.""" - from paperforge.worker.status import run_doctor - import io import sys + from paperforge.worker.status import run_doctor + old_stdout = sys.stdout sys.stdout = buf = io.StringIO() try: diff --git a/tests/test_utils_journal.py b/tests/test_utils_journal.py index 51731896..ddecbd52 100644 --- a/tests/test_utils_journal.py +++ b/tests/test_utils_journal.py @@ -11,8 +11,6 @@ from __future__ import annotations import json from pathlib import Path -import pytest - from paperforge.worker import _utils diff --git a/tests/test_utils_slugify.py b/tests/test_utils_slugify.py index 66d97ba6..fd23a67a 100644 --- a/tests/test_utils_slugify.py +++ b/tests/test_utils_slugify.py @@ -5,10 +5,6 @@ Covers: slugify_filename, _extract_year from __future__ import annotations -from pathlib import Path - -import pytest - from paperforge.worker._utils import _extract_year, slugify_filename diff --git a/tests/test_utils_yaml.py b/tests/test_utils_yaml.py index 997bfd4c..616ebf0a 100644 --- a/tests/test_utils_yaml.py +++ b/tests/test_utils_yaml.py @@ -5,10 +5,6 @@ Covers: yaml_quote, yaml_block, yaml_list from __future__ import annotations -from pathlib import Path - -import pytest - from paperforge.worker._utils import yaml_block, yaml_list, yaml_quote diff --git a/tests/unit/adapters/test_obsidian_frontmatter.py b/tests/unit/adapters/test_obsidian_frontmatter.py index a8093d66..2bfb4dc0 100644 --- a/tests/unit/adapters/test_obsidian_frontmatter.py +++ b/tests/unit/adapters/test_obsidian_frontmatter.py @@ -8,8 +8,8 @@ from paperforge.adapters.obsidian_frontmatter import ( _legacy_control_flags, _read_frontmatter_bool_from_text, _read_frontmatter_optional_bool_from_text, - canonicalize_decision, candidate_markdown, + canonicalize_decision, compute_final_collection, extract_preserved_deep_reading, extract_preserved_tags, diff --git a/tests/unit/doctor/test_field_validator.py b/tests/unit/doctor/test_field_validator.py index 3712bd83..c8520812 100644 --- a/tests/unit/doctor/test_field_validator.py +++ b/tests/unit/doctor/test_field_validator.py @@ -1,8 +1,7 @@ -from pathlib import Path from paperforge.doctor.field_validator import ( - validate_entry_fields, validate_collection, + validate_entry_fields, validate_frontmatter_from_file, ) diff --git a/tests/unit/memory/test_runtime_health.py b/tests/unit/memory/test_runtime_health.py index 7a9c0bf3..614eedf7 100644 --- a/tests/unit/memory/test_runtime_health.py +++ b/tests/unit/memory/test_runtime_health.py @@ -1,12 +1,11 @@ from __future__ import annotations -from pathlib import Path import json from paperforge.memory.runtime_health import ( - get_runtime_health, _check_bootstrap, _check_write, + get_runtime_health, ) diff --git a/tests/unit/memory/test_schema.py b/tests/unit/memory/test_schema.py index 130a18aa..5d539770 100644 --- a/tests/unit/memory/test_schema.py +++ b/tests/unit/memory/test_schema.py @@ -3,14 +3,14 @@ from __future__ import annotations import tempfile from pathlib import Path +from paperforge.memory.db import get_connection from paperforge.memory.schema import ( ALL_TABLES, - ensure_schema, - drop_all_tables, - get_schema_version, CURRENT_SCHEMA_VERSION, + drop_all_tables, + ensure_schema, + get_schema_version, ) -from paperforge.memory.db import get_connection def test_ensure_schema_creates_all_tables(): diff --git a/tests/unit/memory/test_vector_db.py b/tests/unit/memory/test_vector_db.py index 5884d974..60c84c9d 100644 --- a/tests/unit/memory/test_vector_db.py +++ b/tests/unit/memory/test_vector_db.py @@ -1,10 +1,9 @@ from __future__ import annotations from paperforge.embedding.build_state import ( - get_vector_build_state_path, + mark_vector_build_state, read_vector_build_state, write_vector_build_state, - mark_vector_build_state, ) diff --git a/tests/unit/test_paper_resolver.py b/tests/unit/test_paper_resolver.py index d8360de1..df27f63a 100644 --- a/tests/unit/test_paper_resolver.py +++ b/tests/unit/test_paper_resolver.py @@ -7,7 +7,6 @@ import pytest from paperforge.worker.paper_resolver import PaperResolver, PaperWorkspace - # --------------------------------------------------------------------------- # Fixtures # ---------------------------------------------------------------------------