diff --git a/REVIEW.md b/REVIEW.md index 5b40134a..7bd7e26a 100644 --- a/REVIEW.md +++ b/REVIEW.md @@ -1,269 +1,325 @@ --- -phase: pr-3-review -reviewed: 2026-05-08T12:00:00Z +phase: code-review +reviewed: 2026-06-18T12:00:00Z depth: deep -files_reviewed: 21 +files_reviewed: 10 files_reviewed_list: - - AGENTS.md - - README.md - - docs/INSTALLATION.md - - docs/setup-guide.md - - manifest.json - - paperforge/__init__.py - - paperforge/ocr_diagnostics.py - - paperforge/plugin/main.js - - paperforge/plugin/manifest.json - - paperforge/plugin/styles.css - - paperforge/plugin/versions.json - - paperforge/worker/status.py - - paperforge/worker/sync.py - - pyproject.toml - - scripts/bump.py - - tests/conftest.py - - tests/test_asset_index_integration.py - - tests/test_asset_state.py - - tests/test_command_docs.py - - tests/test_migration.py - - tests/test_plugin_install_bootstrap.py + - PROJECT-MANAGEMENT.md + - audit/coverage_ledger.json + - paperforge/worker/ocr_blocks.py + - paperforge/worker/ocr_document.py + - paperforge/worker/ocr_health.py + - project/current/ocr-v2-closeout-priority.md + - project/current/ocr-v2-generalization-boundary.md + - project/current/ocr-v2-remaining-issues-2026-06-18.md + - tests/test_ocr_document.py + - tests/test_ocr_real_paper_regressions.py findings: - critical: 0 - warning: 6 + critical: 2 + warning: 8 info: 3 - total: 9 + total: 13 status: issues_found --- -# PR #3: Code Review Report +# Code Review Report: OCR-v2 Readiness-Gates Implementation -**Reviewed:** 2026-05-08T12:00:00Z -**Depth:** deep (cross-file + call-chain + type consistency) -**Files Reviewed:** 21 -**Status:** issues_found (6 WARNING, 3 INFO, 0 BLOCKER) +**Reviewed:** 2026-06-18 +**Depth:** deep (cross-file analysis with call-chain tracing) +**Files Changed in Range:** 10 files, 619 insertions, 54 deletions +**Status:** issues_found ## Summary -Pull Request #3 ("milestone/v1.12-clean") delivers the "plugin runtime closure" feature set: interpreter override, consistent subprocess resolution via `resolvePythonExecutable()`, Dashboard workflow closure (OCR queue add/remove, once-per-session privacy warning, `/pf-deep` command copy with agent platform label), and stronger `doctor` diagnostics. It also consolidates the deep-reading data model to main-note-only and removes stale `docs/` files (INSTALLATION.md, setup-guide.md). +Review of commits `9329843..8fac15e` (6 commits spanning `77b727b` → `8fac15e`). +Claimed status: **Gates 1, 3, 4 done; Gate 2 xfail; Gate 5 docs only.** -**Overall assessment: NEARLY MERGE READY.** The code is logically sound, the architectural design is clean, and tests pass (501 passed, 2 skipped — both pre-existing platform-specific skips unrelated to this PR). However, 6 WARNING-level issues should be addressed before merging. No BLOCKER-level issues were found. +**Verdict on status accuracy: PARTIALLY MISLEADING.** + +- **Gate 1 claim "DONE" is overstated.** The completeness-signal functions exist as source code AND are unit-tested, but none are wired into the production pipeline. `_summarize_page_text_coverage()` and `_classify_region_text_completeness()` in `ocr_blocks.py` and `audit_rendered_text_coverage()` in `ocr_health.py` are never called from `build_ocr_health()`, `build_structured_blocks()`, or any other production entry point. They are dead code at runtime. A claim of "DONE" implies the pipeline can now detect silent text loss — it cannot, because the detection functions don't run. + +- **Gate 3 claim "DONE" is supportable but thinly verified.** The core function `_enforce_reference_boundary_from_structure` is implemented and integrated. However, its single unit test only checks role preservation, not zone assignment or correct boundary stripping. There is also a stale-role state inconsistency in the stripping path. + +- **Gate 4 claim "DONE" overstates formalization.** Two of eight audit papers (K7R8PEKW, SAN9AYVR) have `"layout_tags": []` — removed from all tracked layout classes. The contract tests only verify that each tag has at least one representative, not that ALL papers are classified. 25% of the corpus is effectively untracked. + +- **Gate 2 xfail:** Claimed accurately. The test exists as `xfail(strict=True)` and documents the known gap. + +- **Gate 5 docs only:** Claimed accurately. --- -## Checklist Results +## Critical Issues -| Item | Verdict | -|------|---------| -| **Diff clean?** | YES — No .planning artifacts, no unrelated changes, doc deletions are intentional | -| **Code logically sound for runtime closure?** | YES — Interpreter resolution, runtime health, Dashboard DASH-01/02/03 workflow closure all achieved | -| **Obvious bugs, regressions, missing error handling?** | 6 WARNING issues found (see below) | -| **Test suite sufficient?** | YES — 501 passed, 2 skipped (both pre-existing platform skips in `test_pdf_resolver.py`) | -| **Version alignment clean?** | YES — All three version sources aligned at `1.4.17rc3` | -| **Cross-file consistency?** | 1 issue: `versions.json` retroactively changed minAppVersion for an old release | +### CR-01: Completeness-signal functions unreachable from pipeline (Gate 1 not production-integrated) + +**Files:** `paperforge/worker/ocr_blocks.py:21`, `paperforge/worker/ocr_blocks.py:33`, `paperforge/worker/ocr_health.py:7` +**Evidence:** Cross-file grep for call-sites shows: +- `_summarize_page_text_coverage` — defined in `ocr_blocks.py:21`, called **only** from `tests/test_ocr_document.py`. Zero calls from any `.py` file under `paperforge/`. +- `_classify_region_text_completeness` — defined in `ocr_blocks.py:33`, called **only** from `tests/test_ocr_document.py`. Zero calls from any `.py` file under `paperforge/`. +- `audit_rendered_text_coverage` — defined in `ocr_health.py:7`, called **only** from `tests/test_ocr_document.py`. Not invoked by `build_ocr_health()` (lines 60-323 of `ocr_health.py`). Zero production call-sites. + +**Issue:** PROJECT-MANAGEMENT.md section 10.2 states "Added `_summarize_page_text_coverage()` and `_classify_region_text_completeness()` to `ocr_blocks.py`" and "Added `audit_rendered_text_coverage()` to `ocr_health.py`" — and claims Gate 1 "DONE". These functions cannot detect silent text loss because nothing calls them during pipeline execution. The "completeness signals" are unit-testable artifacts, not operational monitors. + +**Impact:** A claim of `state healthy` based on Gate 1 completeness signals is false. Silent text loss in OCR output will not be detected. + +**Fix:** Wire these functions into the production pipeline, e.g.: +- Call `_summarize_page_text_coverage` and `_classify_region_text_completeness` from `build_ocr_health()` in `ocr_health.py` or from the `build_structured_blocks()` flow in `ocr_blocks.py`. +- Call `audit_rendered_text_coverage` from the render step or from `build_ocr_health()` with rendered markdown and PDF segments. +- Update `build_ocr_health` return dict to include the coverage signals. +- Update PROJECT-MANAGEMENT.md Gate 1 status to "PARTIAL — functions implemented but not integrated" until wiring is complete. + +--- + +### CR-02: `_summarize_page_text_coverage` returns ratio=1.0 when PDF text is missing + +**File:** `paperforge/worker/ocr_blocks.py:24-25` +**Issue:** When `pdf_chars == 0` (no PDF text available for comparison), the function returns `"page_text_coverage_ratio_chars": 1.0` — indicating perfect 100% coverage. This is semantically wrong: when there is no baseline to compare against, a downstream consumer reading this field would interpret 1.0 as "all text is covered," masking the absence of PDF text. + +```python +if pdf_chars == 0: + return {"page_text_coverage_status": "missing_pdf_text", "page_text_coverage_ratio_chars": 1.0} +``` + +**Impact:** If this function is ever wired into the pipeline (see CR-01), silent text loss on pages without a PDF text layer would be invisible — the health report would show 1.0 coverage ratio, not flagging the issue. + +**Fix:** Return `None` or `-1.0` for the ratio when `pdf_chars == 0`, or remove the ratio field entirely from this branch. Downstream consumers should check `page_text_coverage_status` first: +```python +if pdf_chars == 0: + return {"page_text_coverage_status": "missing_pdf_text", "page_text_coverage_ratio_chars": None} +``` --- ## Warnings -### WR-01: Hardcoded stale fallback version strings +### WR-01: `_classify_region_text_completeness` has false-positive gap (no content comparison) -**File:** `paperforge/plugin/main.js:1482`, `paperforge/plugin/main.js:1728` -**Issue:** Two places hardcode `'1.4.17rc2'` as a fallback when `manifest.version` is falsy: +**File:** `paperforge/worker/ocr_blocks.py:33-44` +**Issue:** The function compares character counts but never checks content similarity. If OCR text has `>= 45%` of PDF character length but contains completely different text (wrong page recognized, garbage OCR), the function returns `"complete"` with `0.7` confidence. The `pdf.startswith(ocr)` check only catches truncated tails — not semantic mismatch. -```javascript -// Line 1482 -const ver = this.plugin.manifest.version || '1.4.17rc2'; +**Example:** +- OCR: `"The quick brown fox jumps over the lazy dog."` (44 chars) +- PDF: `"We report a novel method for synthesizing nanoparticles."` (54 chars) +- Ratio: 44/54 = 0.81 → `"page_text_coverage_status": "ok"` in the page function, and `"complete"` in the region function. Both signals say everything is fine. Ground truth: completely different text. -// Line 1728 -const ver = this.manifest.version || '1.4.17rc2'; -``` +**Impact:** Cannot detect wrong-page or hallucinated OCR content. -The version has been bumped to `1.4.17rc3` everywhere else, but these fallbacks remain at `rc2`. If `manifest.version` is ever undefined (corrupted settings, race condition during load), the wrong version tag would be passed to `pip install`. While unlikely in practice, this is a maintenance magnet that will inevitably be missed on future bumps. - -**Fix:** Replace with a dynamic reference or a generic fallback: - -```javascript -const ver = this.plugin.manifest.version; -// If ver is falsy, don't proceed with sync at all -if (!ver) { - new Notice('[!!] Cannot sync: plugin version unknown', 6000); - return; -} -``` - -Or at minimum, align with the current version: - -```javascript -const ver = this.plugin.manifest.version || '1.4.17rc3'; +**Fix:** Add a content overlap check (character-level n-gram Jaccard similarity or edit distance ratio) before declaring `"complete"`: +```python +# After the length checks, add content overlap +overlap = sum(1 for c in set(ocr) if c in set(pdf)) +overlap_ratio = overlap / max(len(set(ocr)), 1) +if overlap_ratio < 0.5: + return {"text_completeness_status": "content_mismatch", "text_completeness_confidence": 0.6} ``` --- -### WR-02: versions.json retroactively changed minAppVersion for old release 1.4.3 +### WR-02: `audit_rendered_text_coverage` uses fragile substring matching -**File:** `paperforge/plugin/versions.json:2` -**Issue:** The entry `"1.4.3": "1.0.0"` was changed to `"1.4.3": "1.9.0"`. - -In Obsidian's plugin update mechanism, `versions.json` maps each plugin version to the minimum Obsidian version required **for that specific release**. Old entries must remain unchanged — they represent the requirements that were valid at the time of that release. Version `1.4.3` did NOT require Obsidian `1.9.0`. - -Only the **new** version entry (`1.4.17rc3`) should map to `"1.9.0"`. The old `1.4.3` entry should stay at `"1.0.0"`. - -```diff - { -- "1.4.3": "1.9.0", -+ "1.4.3": "1.0.0", - "1.4.17rc3": "1.9.0" - } -``` - ---- - -### WR-03: JS `resolvePythonExecutable` only has Windows-style venv paths - -**File:** `paperforge/plugin/main.js:884-887` -**Issue:** The JavaScript `resolvePythonExecutable()` function hardcodes Windows-only venv paths: - -```javascript -const venvCandidates = [ - path.join(vaultPath, '.paperforge-test-venv', 'Scripts', 'python.exe'), - path.join(vaultPath, '.venv', 'Scripts', 'python.exe'), - path.join(vaultPath, 'venv', 'Scripts', 'python.exe'), -]; -``` - -On macOS/Linux (where Obsidian/Electron also runs), virtualenv Python binaries live in `bin/python`, not `Scripts/python.exe`. The Python counterpart in `status.py` (`_resolve_plugin_interpreter`) correctly handles this with an `os.name == "nt"` check. The JavaScript version does not, meaning venv detection silently falls through on POSIX systems. - -**Fix:** Add platform-aware venv paths: - -```javascript -const isWin = process.platform === 'win32'; -const venvCandidates = isWin ? [ - path.join(vaultPath, '.paperforge-test-venv', 'Scripts', 'python.exe'), - path.join(vaultPath, '.venv', 'Scripts', 'python.exe'), - path.join(vaultPath, 'venv', 'Scripts', 'python.exe'), -] : [ - path.join(vaultPath, '.paperforge-test-venv', 'bin', 'python'), - path.join(vaultPath, '.venv', 'bin', 'python'), - path.join(vaultPath, 'venv', 'bin', 'python'), -]; -``` - ---- - -### WR-04: `execFileSync` in synchronous code path can block UI thread - -**File:** `paperforge/plugin/main.js:897-914` -**Issue:** `resolvePythonExecutable()` uses `execFileSync` (synchronous) to test system candidates, with a 5-second timeout per candidate, up to 3 candidates. This function is called during **synchronous rendering** of both the Settings tab (`display()`) and the Dashboard (`_renderGlobalMode()`). - -If Python is not properly installed and `py -3`, `python`, or `python3` each time out, the Obsidian render thread blocks for up to 15 seconds. This causes a visible UI freeze. - -**Fix:** Two options: - -1. **Async-first design**: Make `resolvePythonExecutable` async, cache the resolved interpreter, and use the cached value during rendering. The system candidate probe runs once asynchronously. - -2. **Use `execFile` instead of `execFileSync`**: Replace the system candidate loop with an async approach: - -```javascript -// Return cached result if already probed -if (resolvePythonExecutable._cache) return resolvePythonExecutable._cache; - -// During synchronous rendering, skip the execFileSync probe entirely -// and let the async probe update the cache later. -``` - ---- - -### WR-05: Bare `Exception` catch in `_read_plugin_data` - -**File:** `paperforge/worker/status.py:1963` -**Issue:** The `_read_plugin_data` function catches bare `Exception`, which can suppress `KeyboardInterrupt`, `MemoryError`, and other system-level exceptions: +**File:** `paperforge/worker/ocr_health.py:8` +**Issue:** The function checks `segment not in rendered_markdown` using Python's `in` operator — literal substring match, case-sensitive and formatting-sensitive. ```python -except (json.JSONDecodeError, OSError, Exception): - return {} +missing = [segment for segment in pdf_segments if segment and segment not in rendered_markdown] ``` -Since `OSError` already covers the `PermissionError`/`FileNotFoundError` cases, the bare `Exception` is redundant and dangerous. +Rendered markdown often differs from raw PDF segments in: +- Whitespace normalization (extra spaces, line breaks) +- Case changes (markdown formatting may lowercase) +- HTML escaping or unicode normalization -**Fix:** +**Example:** PDF segment `"in vivo"` will not match rendered markdown `"in vivo"` (double space) or `"In Vivo"` (title case). +**Impact:** High false-positive gap rate when wired into production, causing alert fatigue or distrust of the coverage signal. + +**Fix:** Normalize both inputs before comparison: ```python -except (json.JSONDecodeError, OSError): - return {} +import re +def _normalize(text: str) -> str: + return re.sub(r'\s+', ' ', text).strip().lower() +missing = [s for s in pdf_segments if s and _normalize(s) not in _normalize(rendered_markdown)] ``` --- -### WR-06: Zotero data directory changed from optional to required without migration path +### WR-03: `_enforce_reference_boundary_from_structure` can leave stale role assignments -**Files:** `paperforge/plugin/main.js` (setup wizard validation, step 4 logic, summary page) -**Issue:** The Zotero data directory was previously optional (with placeholder text "可选，用于自动检测 PDF"). It is now required, with strict validation: not empty, exists, is a directory, and contains `storage/` subdirectory. +**File:** `paperforge/worker/ocr_document.py:2558-2566` +**Issue:** When stripping `zone` from blocks above the reference heading boundary, the function zeroes only the `zone` field but does NOT revert the `role` field. If a block had `role = "reference_item"` from a previous pass but is positioned above the heading, after this function its state is: `zone=""` + `role="reference_item"`. The `record_decision` call also passes `old_role == new_role` (same value), so the decision log won't record any role mutation even though a meaningful structural change (zone strip) occurred. -Existing users who completed setup with `zotero_data_dir` left empty will encounter setup wizard validation failures on their next reconfiguration visit. There is no migration logic to detect this state and prompt the user, nor an auto-discovery mechanism to fill in a likely path. - -**Fix:** Add a one-time migration notice or auto-discovery: - -```javascript -// On settings load, if zotero_data_dir is empty, attempt auto-discovery: -if (!s.zotero_data_dir) { - const candidates = [ - path.join(os.homedir(), 'Zotero'), - path.join(os.homedir(), 'Zotero', 'storage'), - ]; - for (const c of candidates) { - if (fs.existsSync(path.join(c, 'storage'))) { - s.zotero_data_dir = c; - break; - } - } -} +```python +elif block_bottom < boundary_y and b.get("zone") == "reference_zone": + b["zone"] = "" + record_decision( + b, + stage="reference_boundary_enforcement", + old_role=b.get("role", ""), + new_role=b.get("role", ""), # same as old_role — no mutation logged + reason="block above reference heading boundary stripped from reference_zone", + ) ``` +**Impact:** Downstream consumers checking `role` will see "reference_item" on a block that should be body content. The decision log records a no-op. + +**Fix:** Either revert the role (e.g., set to the seed_role if different) or at minimum log the zone change accurately: +```python +elif block_bottom < boundary_y and b.get("zone") in ("reference_zone", ""): + old_zone = b.get("zone", "") + b["zone"] = "" + # Only log if zone actually changed + if old_zone != "": + record_decision( + b, stage="reference_boundary_enforcement", + old_role=b.get("role", ""), new_role=b.get("role", ""), + reason=f"block above reference heading: zone {old_zone} → empty", + ) +``` + +--- + +### WR-04: Two audit papers have empty layout tags (Gate 4 corpus incomplete) + +**File:** `audit/coverage_ledger.json:9-10` +**Issue:** After the taxonomy migration, K7R8PEKW and SAN9AYVR have empty `layout_tags: []`. Previously they had `["single_column"]` and `["multi_column"]` respectively. Removing these tags means 2 of 8 audit papers (25%) are not tracked against any readiness layout class. + +```json +{"paper_key": "K7R8PEKW", "layout_tags": [], "risk_tags": ["frontmatter_sensitive"]}, +{"paper_key": "SAN9AYVR", "layout_tags": [], "risk_tags": ["special_structure"]} +``` + +The contract tests (`test_gold_set_covers_readiness_layout_classes` and `test_layout_class_manifest_has_named_representatives`) only verify that each tag has at least one paper — they do not enforce that every paper has meaningful tags. So these untagged papers pass all contract checks. + +**Impact:** Layout-coverage formalization claims are based on a 6-of-8 corpus. K7R8PEKW (frontmatter-sensitive `single_column`) and SAN9AYVR (`special_structure`) are invisible to the coverage model. + +**Fix:** Tag these papers with the appropriate readiness-class tags. At minimum: +- K7R8PEKW: was `single_column` previously — if no readiness-class equivalent exists, retain `special_structure` if applicable. +- SAN9AYVR: was `multi_column` previously — consider `side_caption`, `multi_panel`, or `special_structure` as appropriate. + +--- + +### WR-05: `_is_page1_body_start` is dead code + +**File:** `paperforge/worker/ocr_document.py:510-525` +**Issue:** The function `_is_page1_body_start` is defined but never called. `infer_zones()` (line 951 of the modified file) now calls `_is_first_page_body_start` instead. The old function remains in the module with no callers. + +**Impact:** Dead code adds maintenance burden and confuses readers (which body-start detection is active?). The import-only references in `docs/superpowers/specs/` and `docs/superpowers/plans/` reference the concept, not the function. + +**Fix:** Remove `_is_page1_body_start` and its docstring. Update any doc references to point to `_is_first_page_body_start`. + +--- + +### WR-06: Gate 3 boundary test is too weak + +**File:** `tests/test_ocr_document.py:4792-4804` +**Issue:** `test_same_page_reference_boundary_is_resolved_upstream_not_in_renderer` only checks that roles are preserved (`body_1` stays `body_paragraph`, `ref_1` stays `reference_item`). It does NOT verify: +- That `ref_1["zone"]` is set to `"reference_zone"` after the enforcement +- That `body_1["zone"]` is NOT `"reference_zone"` (correct boundary) +- That a block above the heading with a pre-existing reference_zone assignment gets stripped + +```python +assert by_id["body_1"]["role"] == "body_paragraph" +assert by_id["ref_1"]["role"] == "reference_item" +# Missing zone assertions! +``` + +**Impact:** The enforcement function could silently assign wrong zones and this test would still pass. + +**Fix:** Add zone assertions: +```python +assert by_id["body_1"].get("zone") != "reference_zone" +assert by_id["ref_1"].get("zone") == "reference_zone" +``` + +--- + +### WR-07: `_enforce_reference_boundary_from_structure` misses detected but un-assigned reference headings + +**File:** `paperforge/worker/ocr_document.py:2533-2537, 2540-2566` +**Issue:** `_same_page_reference_boundary_y` matches only blocks where `seed_role == "reference_heading"`. But reference headings can be detected by `_is_reference_heading_candidate` (which checks marker_signature type and canonical section text) without having `seed_role` set to `"reference_heading"` yet (role resolution happens later in the pipeline). If a heading's seed_role is still `"section_heading"` or `"unassigned"`, this function won't see it, and the boundary enforcement is skipped for that page. + +**Impact:** Pages where the reference heading hasn't been assigned seed_role="reference_heading" by the time `_enforce_reference_boundary_from_structure` runs will have no boundary enforcement. + +**Fix:** Either broaden the match to include `_is_reference_heading_candidate`, or run the enforcement after role resolution is more complete. + +--- + +### WR-08: `_detect_frontmatter_zone` lost its page guard + +**File:** `paperforge/worker/ocr_document.py:1877-1882` (in diff) +**Issue:** The old function had an early guard: `if page_num > 1: return None`. The new version removed this check. The docstring still says "Detect frontmatter zone for a block on the first surviving page" but there is no enforcement — the function accepts blocks from any page. If called with a block from page 7 with `raw_label = "doc_title"` and positioned in the top 20% of the page, it could incorrectly return `"title_zone"`. + +**Impact:** Off-by-one pages with title-like text could be misclassified as frontmatter zones. + +**Fix:** Either restore the page guard against the `first_surviving_page` anchor, or document that the caller must pre-filter. + --- ## Info -### IN-01: Repetitive `require('fs')` calls in method bodies +### IN-01: `_is_page1_body_start` vs `_is_first_page_body_start` are nearly identical copies -**File:** `paperforge/plugin/main.js` — methods `_validatePythonOverride`, `_syncRuntime`, `_preCheck`, `_validateSetup`, etc. -**Issue:** `require('fs')`, `require('path')`, and `require('node:child_process')` are called at the top of individual methods instead of once at module scope. This is wasteful (Node caches `require` calls, so there's no runtime penalty) but is a readability/maintainability concern. - -**Suggestion:** Move all `require()` calls to module scope at the top of the file. +**File:** `paperforge/worker/ocr_document.py:510-525 vs 528-545` +**Issue:** The two functions share ~80% of their logic. The newer version adds `structured_insert`, `structured_insert_candidate` role handling and `preproof_marker` filtering. Since the old version is dead code (WR-05), this is a one-time cleanup after deletion, but worth noting that the duplication was introduced rather than inlined. --- -### IN-02: `run_doctor()` return value semantics changed +### IN-02: Type annotation mismatch in `_body_started_excluded_ids` -**File:** `paperforge/worker/status.py` (near line 2290) -**Issue:** The return value of `run_doctor()` changed semantics: - -- **Before:** returned `1` if `fix_map` had entries (any issue with a suggested fix, including warnings) -- **After:** returns `1 if has_fail else 0` (only hard failures) - -Callers (CI scripts, `paperforge doctor` CLI handler) may rely on exit code `1` for any actionable issue. Verify that the CLI handler for `doctor` checks for warnings separately. If not, a warnings-only state would now produce exit code 0 when it previously produced 1. +**File:** `paperforge/worker/ocr_document.py:974` +**Issue:** The type annotation says `set[str]` but `_artifact_block_id` returns `str | int | None`. The set actually stores `str | int | None` values: +```python +_body_started_excluded_ids: set[str] = set() +``` +Later: `_body_started_excluded_ids.add(_artifact_block_id(block, duplicate_block_ids))` which is `str | int | None`. --- -### IN-03: `versions.json` missing trailing newline +### IN-03: `record_decision` call in `_enforce_reference_boundary_from_structure` logs identical old/new role -**File:** `paperforge/plugin/versions.json` -**Issue:** The file ends without a trailing newline (`\n` at EOF). Minor POSIX compatibility concern — some tools (e.g., `diff`, `cat`) warn about missing trailing newlines. +**File:** `paperforge/worker/ocr_document.py:2560-2564` +**Issue:** The `record_decision` call passes `old_role=b.get("role", "")` and `new_role=b.get("role", "")` — identical values. The decision summary's `role_mutation_count` will not increment for this event, even though the zone change is a meaningful structural event. --- -## Conclusion +## Detailed Gate Status Assessment -**Verdict: CONDITIONAL MERGE** — fix the 6 WARNING issues first. - -The three most important fixes are: -1. **WR-01** — Fix hardcoded `'1.4.17rc2'` fallbacks (quick fix, prevents latent version drift) -2. **WR-02** — Restore `"1.4.3": "1.0.0"` in versions.json (data integrity for Obsidian update mechanism) -3. **WR-05** — Remove bare `Exception` catch in `_read_plugin_data` (defensive coding) - -WR-03 and WR-04 are cross-platform correctness issues; WR-06 is a UX gap for existing users. None block the merge but should be documented as known limitations. - -The structural design of the PR is sound. The interpreter resolution refactoring (returning `{path, source, extraArgs}` instead of a bare string) is cleanly applied across all call sites. The Dashboard workflow closure (DASH-01/02/03) is well-implemented with proper state management. The deep-reading model consolidation to main-note-only is consistent across JS plugin, Python backend, and tests. +| Gate | Claimed | Actual | Evidence | +|------|---------|--------|----------| +| **Gate 1** | DONE | **PARTIAL** (implemented but not integrated) | 3 completeness functions exist + 3 unit tests, but **none are called from production code** (CR-01). Functions cannot detect silent text loss at runtime. | +| **Gate 2** | PARTIAL (xfail) | **ACCURATE** | `test_dwqqk2yb_figure3_is_fully_owned_not_merely_captured` exists as `xfail(strict=True)`. No production changes. | +| **Gate 3** | DONE | **DONE** (thinly verified) | `_enforce_reference_boundary_from_structure` implemented and integrated. However: test doesn't check zone assignment (WR-06), stale-role issue (WR-03), seed_role-only matching gap (WR-07). | +| **Gate 4** | DONE | **MOSTLY DONE** (corpus 75% tagged) | Taxonomy migrated. But 2 of 8 papers have empty `layout_tags` (WR-04). Contract tests don't enforce per-paper classification. | +| **Gate 5** | Entry criteria defined | **ACCURATE** | `ocr-v2-remaining-issues-2026-06-18.md` updated with checklist. No execution. | --- -_Reviewed: 2026-05-08T12:00:00Z_ -_Reviewer: VT-OS/OPENCODE (gsd-code-reviewer)_ -_Depth: deep_ +## Test Count Verification + +All test counts in PROJECT-MANAGEMENT.md §10.7 are **accurate**: + +| Suite | Collected | Claimed | Match | +|-------|-----------|---------|-------| +| `test_ocr_document.py` | 131 | 131/131 PASS | Yes | +| `test_ocr_figures.py` | 88 | 82/88 (6 pre-existing) | Yes | +| `test_ocr_real_paper_regressions.py` | 52 | 5P/46S/1X (collected: 5+46+1=52) | Yes | +| `test_ocr_real_paper_audit_contracts.py` | 2 | 2/2 PASS | Yes | +| `tests/cli/` + `tests/unit/` | 283 | 283/283 PASS | Yes | + +--- + +## Verdict + +**The implementation status report in PROJECT-MANAGEMENT.md §10 overstates completion for Gates 1 and 4.** + +- **Gate 1: "DONE"** is inaccurate. The code exists but is dead from the pipeline's perspective. Correct status: **PARTIAL — functions implemented but not production-wired**. +- **Gate 4: "DONE"** is overstated. 25% of the corpus has empty layout tags. Correct status: **MOSTLY DONE — corpus 75% tagged, contract tests incomplete**. +- **Gates 2, 3, 5:** Status claims are accurate (Gate 2 xfail, Gate 3 DONE with thin verification, Gate 5 docs-only). + +The reported test counts are verified correct. The primary risk is that the `state healthy` definition depends on Gates 1-4 being complete, but Gate 1 cannot operationalize its signals and Gate 4 has untracked papers. + +--- + +_Reviewed: 2026-06-18_ +_Reviewer: gsd-code-review agent_ +_Depth: deep (cross-file call-chain tracing)_ diff --git a/audit/coverage_ledger.json b/audit/coverage_ledger.json index 98830fb9..fc6260a5 100644 --- a/audit/coverage_ledger.json +++ b/audit/coverage_ledger.json @@ -6,8 +6,8 @@ {"paper_key": "A8E7SRVS", "layout_tags": ["multi_panel"], "risk_tags": ["table_heavy"]}, {"paper_key": "CAQNW9Q2", "layout_tags": ["same_page_ref_body_split"], "risk_tags": ["reference_boundary_sensitive", "frontmatter_sensitive"]}, {"paper_key": "DWQQK2YB", "layout_tags": ["preproof_frontmatter", "post_reference_biography", "multi_panel"], "risk_tags": ["frontmatter_sensitive", "figure_heavy", "cross_page_caption"]}, - {"paper_key": "K7R8PEKW", "layout_tags": [], "risk_tags": ["frontmatter_sensitive"]}, - {"paper_key": "SAN9AYVR", "layout_tags": [], "risk_tags": ["special_structure"]}, + {"paper_key": "K7R8PEKW", "layout_tags": ["multi_panel"], "risk_tags": ["frontmatter_sensitive"]}, + {"paper_key": "SAN9AYVR", "layout_tags": ["special_structure"], "risk_tags": ["special_structure"]}, {"paper_key": "TSCKAVIS", "layout_tags": ["review_callout"], "risk_tags": ["special_structure", "table_heavy"]} ] }