mirror of
https://github.com/lllin000/PaperForge.git
synced 2026-07-22 06:50:53 +00:00
docs: update REVIEW and coverage ledger
This commit is contained in:
Research Assistant
parent
4d5180ee61
commit
47557aa272
2 changed files with 261 additions and 205 deletions
462
REVIEW.md
462
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)_
|
||||
|
|
|
|||
|
|
@ -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"]}
|
||||
]
|
||||
}
|
||||
|
|
|
|||
Loading…
Reference in a new issue