lllin000_PaperForge/REVIEW.md

phase	reviewed	depth	files_reviewed	files_reviewed_list	findings	status
pr-3-review	2026-05-08T12:00:00Z	deep	21	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	critical warning info total 0 6 3 9	issues_found

PR #3: Code Review Report

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)

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).

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.

---

Checklist Results

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

---

Warnings

WR-01: Hardcoded stale fallback version strings

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:

// Line 1482
const ver = this.plugin.manifest.version || '1.4.17rc2';

// Line 1728
const ver = this.manifest.version || '1.4.17rc2';

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:

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:

const ver = this.plugin.manifest.version || '1.4.17rc3';

---

WR-02: versions.json retroactively changed minAppVersion for old release 1.4.3

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".

 {
-  "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:

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:

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:

// 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:

except (json.JSONDecodeError, OSError, Exception):
    return {}

Since OSError already covers the PermissionError/FileNotFoundError cases, the bare Exception is redundant and dangerous.

Fix:

except (json.JSONDecodeError, OSError):
    return {}

---

WR-06: Zotero data directory changed from optional to required without migration path

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.

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:

// 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;
        }
    }
}

---

Info

IN-01: Repetitive require('fs') calls in method bodies

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.

---

IN-02: run_doctor() return value semantics changed

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.

---

IN-03: versions.json missing trailing newline

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.

---

Conclusion

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.

---

Reviewed: 2026-05-08T12:00:00Z Reviewer: VT-OS/OPENCODE (gsd-code-reviewer) Depth: deep