mirror of
https://github.com/lllin000/PaperForge.git
synced 2026-07-22 06:50:53 +00:00
fix: add --resume CLI arg and get_collection import for embed build
This commit is contained in:
parent
8bbe28a6f6
commit
80db051d8c
8 changed files with 2526 additions and 0 deletions
157
.planning/phases/02-code-review-command/02-REVIEW.md
Normal file
157
.planning/phases/02-code-review-command/02-REVIEW.md
Normal file
|
|
@ -0,0 +1,157 @@
|
|||
---
|
||||
phase: 02-code-review
|
||||
reviewed: 2026-05-14T12:00:00Z
|
||||
depth: deep
|
||||
files_reviewed: 1
|
||||
files_reviewed_list:
|
||||
- paperforge/memory/schema.py
|
||||
findings:
|
||||
critical: 0
|
||||
warning: 3
|
||||
info: 2
|
||||
total: 5
|
||||
status: issues_found
|
||||
---
|
||||
|
||||
# Phase 02: Code Review Report
|
||||
|
||||
**Reviewed:** 2026-05-14T12:00:00Z
|
||||
**Depth:** deep
|
||||
**Files Reviewed:** 1
|
||||
**Status:** issues_found
|
||||
|
||||
## Summary
|
||||
|
||||
Reviewed Task 1 implementation: addition of `CREATE_READING_LOG` and `CREATE_PROJECT_LOG` SQL table definitions to `paperforge/memory/schema.py`. The implementation faithfully follows the plan specification with correct SQL syntax, proper registration in `ensure_schema()`, correct `ALL_TABLES` inclusion, and `CURRENT_SCHEMA_VERSION` bumped from 1 to 2. All 5 existing schema unit tests pass. No critical defects were introduced.
|
||||
|
||||
However, three warnings were identified: (1) the foreign key from `reading_log` to `papers` will create a deletion-ordering conflict in `builder.py` once the table is populated (future Task 6 concern), (2) the column name `date` in `project_log` shadows a SQLite built-in function name, and (3) the foreign key column `reading_log.paper_id` lacks an index, which will cause full table scans on lookup queries.
|
||||
|
||||
## Warnings
|
||||
|
||||
### WR-01: Foreign Key Will Block `DELETE FROM papers` During Rebuild
|
||||
|
||||
**File:** `paperforge/memory/schema.py:153` (FK definition), `paperforge/memory/builder.py:84` (DELETE caller)
|
||||
|
||||
**Issue:** The `reading_log` table defines `FOREIGN KEY (paper_id) REFERENCES papers(zotero_key)` without `ON DELETE CASCADE`. During non-schema-change rebuilds in `builder.py`, the code executes `DELETE FROM papers` (line 84) WITHOUT first clearing `reading_log`. Once Task 6 populates `reading_log` from JSONL, this DELETE will fail with an `IntegrityError` because SQLite enforces foreign key constraints on DML operations when `PRAGMA foreign_keys=ON` (which `db.py:34` explicitly enables).
|
||||
|
||||
Currently latent because `reading_log` is never populated (Task 6 has not been implemented). Will surface during the Task 6 rebuild flow.
|
||||
|
||||
**Fix:**
|
||||
|
||||
In `builder.py`, clear `reading_log` (and `project_log`) before deleting `papers`:
|
||||
|
||||
```python
|
||||
# In builder.py, before the DELETE FROM papers (line 84), add:
|
||||
conn.execute("DELETE FROM reading_log;")
|
||||
conn.execute("DELETE FROM project_log;")
|
||||
```
|
||||
|
||||
Alternatively, add `ON DELETE CASCADE` to the foreign key definition in `schema.py`:
|
||||
```python
|
||||
FOREIGN KEY (paper_id) REFERENCES papers(zotero_key) ON DELETE CASCADE
|
||||
```
|
||||
|
||||
The first approach (explicit DELETE) is preferred because it makes the rebuild flow explicit and doesn't silently cascade deletions that could surprise maintainers.
|
||||
|
||||
---
|
||||
|
||||
### WR-02: Column Name `date` Shadows SQLite Built-in Function
|
||||
|
||||
**File:** `paperforge/memory/schema.py:161`
|
||||
|
||||
**Issue:** The `project_log` table uses `date` as a column name (line 161: `date TEXT NOT NULL`). While syntactically valid in SQLite (which allows function names as unquoted identifiers), `date` is a built-in SQL function. This creates ambiguity when reading queries — `SELECT date FROM project_log` works but `SELECT date(created_at) FROM project_log` is ambiguous. More importantly, if this schema is ever ported to another SQL dialect (PostgreSQL, MySQL), `date` is a reserved word and will require quoting.
|
||||
|
||||
**Fix:**
|
||||
|
||||
Rename the column to `log_date`, `entry_date`, or `recorded_date`:
|
||||
|
||||
```sql
|
||||
-- In CREATE_PROJECT_LOG:
|
||||
log_date TEXT NOT NULL,
|
||||
```
|
||||
|
||||
Note: This would also require updating the plan's Task 2 (`permanent.py`) and Task 6 (`builder.py`) where the column is referenced. If changing the schema column name is too invasive for this phase, consider adding a comment noting the potential ambiguity.
|
||||
|
||||
---
|
||||
|
||||
### WR-03: Missing Index on Foreign Key Column `reading_log.paper_id`
|
||||
|
||||
**File:** `paperforge/memory/schema.py:142`
|
||||
|
||||
**Issue:** The `reading_log` table has a foreign key on `paper_id` (line 142), but no index is created for this column. All queries filtering by `paper_id` (e.g., `get_reading_notes_for_paper()` in `permanent.py`, paper-context lookup) will require full table scans. As the reading log grows, this will degrade query performance.
|
||||
|
||||
**Fix:**
|
||||
|
||||
Add an index alongside the existing `EVENT_INDEX_SQL` block (following the established pattern in the file):
|
||||
|
||||
```python
|
||||
# After EVENT_INDEX_SQL (line 137), add:
|
||||
READING_LOG_INDEX_SQL = [
|
||||
"CREATE INDEX IF NOT EXISTS idx_reading_log_paper ON reading_log(paper_id);",
|
||||
"CREATE INDEX IF NOT EXISTS idx_reading_log_project ON reading_log(project);",
|
||||
"CREATE INDEX IF NOT EXISTS idx_reading_log_created ON reading_log(created_at);",
|
||||
]
|
||||
```
|
||||
|
||||
And register in `ensure_schema()` after the `EVENT_INDEX_SQL` loop:
|
||||
|
||||
```python
|
||||
for idx_sql in READING_LOG_INDEX_SQL:
|
||||
conn.execute(idx_sql)
|
||||
```
|
||||
|
||||
Also add a project-level index for `project_log`:
|
||||
|
||||
```python
|
||||
PROJECT_LOG_INDEX_SQL = [
|
||||
"CREATE INDEX IF NOT EXISTS idx_project_log_project ON project_log(project);",
|
||||
"CREATE INDEX IF NOT EXISTS idx_project_log_created ON project_log(created_at);",
|
||||
]
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## Info
|
||||
|
||||
### IN-01: Test Hardcodes Stale Schema Version
|
||||
|
||||
**File:** `tests/unit/memory/test_schema.py:73`
|
||||
|
||||
**Issue:** The test `test_get_schema_version_returns_stored_value` inserts `schema_version = '1'` and asserts the returned value equals 1. Since `CURRENT_SCHEMA_VERSION` was bumped to 2, the hardcoded '1' no longer matches the current version. While the test is verifying read-back behavior (not version equality), the stale value could confuse future maintainers who assume the test reflects the current schema version.
|
||||
|
||||
**Fix:**
|
||||
|
||||
Either update to a version-independent test or clarify with a comment:
|
||||
|
||||
```python
|
||||
# Test uses arbitrary version '1' for read-back verification (not tied to CURRENT_SCHEMA_VERSION)
|
||||
conn.execute(
|
||||
"INSERT INTO meta (key, value) VALUES ('schema_version', '1')"
|
||||
)
|
||||
conn.commit()
|
||||
assert get_schema_version(conn) == 1
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
### IN-02: `ALL_TABLES` Ordering — Child Tables After Parent (Latent DDL Risk)
|
||||
|
||||
**File:** `paperforge/memory/schema.py:175`
|
||||
|
||||
**Issue:** The `ALL_TABLES` list orders `papers` (index 1) before its child tables `paper_assets`, `paper_aliases`, `paper_events`, and now `reading_log` (indices 2-7). In `drop_all_tables()`, SQLite with `PRAGMA foreign_keys=ON` will reject `DROP TABLE papers` if any child table contains rows referencing papers. This is currently benign because `drop_all_tables()` is only called when schema versions mismatch — at which point the new `reading_log` table hasn't been populated yet. However, if `drop_all_tables` is ever called on a populated database, the ordering would cause a failure.
|
||||
|
||||
**Fix (if desired — pre-existing, not introduced by this change):**
|
||||
|
||||
Reorder `ALL_TABLES` so child tables (FK-referencing) appear before parent tables (FK-referenced):
|
||||
|
||||
```python
|
||||
ALL_TABLES = ["paper_fts", "reading_log", "project_log", "paper_events", "paper_aliases", "paper_assets", "papers", "meta"]
|
||||
```
|
||||
|
||||
Note: `paper_fts` is a virtual table and cannot have FK constraints, so its position is flexible.
|
||||
|
||||
---
|
||||
|
||||
_Reviewed: 2026-05-14T12:00:00Z_
|
||||
_Reviewer: the agent (gsd-code-reviewer)_
|
||||
_Depth: deep_
|
||||
183
.planning/phases/permanent-jsonl-review/REVIEW.md
Normal file
183
.planning/phases/permanent-jsonl-review/REVIEW.md
Normal file
|
|
@ -0,0 +1,183 @@
|
|||
---
|
||||
phase: permanent-jsonl-review
|
||||
reviewed: 2026-05-14T00:00:00Z
|
||||
depth: standard
|
||||
files_reviewed: 1
|
||||
files_reviewed_list:
|
||||
- paperforge/memory/permanent.py
|
||||
findings:
|
||||
critical: 0
|
||||
warning: 4
|
||||
info: 4
|
||||
total: 8
|
||||
status: issues_found
|
||||
---
|
||||
|
||||
# Phase permanent-jsonl-review: Code Review Report
|
||||
|
||||
**Reviewed:** 2026-05-14
|
||||
**Depth:** standard
|
||||
**Files Reviewed:** 1
|
||||
**Status:** issues_found
|
||||
|
||||
## Summary
|
||||
|
||||
Reviewed `paperforge/memory/permanent.py` — a new 154-line JSONL permanent storage module implementing two append-only logs (`reading-log.jsonl` and `project-log.jsonl`) with 10 functions for append, read-all, and filtered-read operations. The file correctly uses `secrets.token_hex` for collision-resistant ID generation, handles write-side OSError, and gracefully skips malformed JSON lines on read.
|
||||
|
||||
Four WARNING-level issues were found, primarily around schema inconsistency with the corresponding SQL tables defined in `paperforge/memory/schema.py` (added in the immediately prior commit 5493e28). The JSONL field names for `project_log` diverge from the SQL column names (`entry_type` vs `type`, `content` vs `title`), the `reading_log` is missing the `verified` field present in SQL, and `append_project_entry` lacks input validation. The module also has no tests and no consumers — it is not imported anywhere in the codebase.
|
||||
|
||||
No BLOCKER issues were found: the code is free of crashes, data loss, and security vulnerabilities.
|
||||
|
||||
## Warnings
|
||||
|
||||
### WR-01: Schema field name mismatch — project_log JSONL vs SQL
|
||||
|
||||
**File:** `paperforge/memory/permanent.py:114-133`
|
||||
**Issue:** The JSONL `append_project_entry` records use field names that differ from the SQL `project_log` table defined in `paperforge/memory/schema.py` (lines 158-173):
|
||||
|
||||
| JSONL (permanent.py) | SQL (schema.py) |
|
||||
|-----------------------|-----------------|
|
||||
| `entry_type` | `type` |
|
||||
| `content` | `title` |
|
||||
| *(none)* | `date` (NOT NULL) |
|
||||
|
||||
`append_project_entry` also lacks a `date` field entirely — the SQL schema requires `date TEXT NOT NULL`. The `reading_log` tables match more closely but still diverge in one column (see WR-02).
|
||||
|
||||
These two commits (5493e28 for the SQL tables, 0d1ca3a for JSONL) appear related. Inconsistent field names will cause confusion and potential data corruption when any bridge/migration code is written between the two storage backends.
|
||||
|
||||
**Fix:** Align the JSONL field names with the SQL schema. Either:
|
||||
1. Rename JSONL fields to match SQL (`entry_type` → `type`, `content` → `title`, add `date` field), or
|
||||
2. Rename the SQL columns to match JSONL (if the SQL schema has not yet been deployed).
|
||||
|
||||
Example aligned implementation for `append_project_entry`:
|
||||
```python
|
||||
record: dict[str, object] = {
|
||||
"id": entry_id,
|
||||
"created_at": now,
|
||||
"project": entry.get("project", ""),
|
||||
"date": date_str, # ADDED — matches SQL NOT NULL
|
||||
"type": entry.get("type", ""), # RENAMED from entry_type
|
||||
"title": entry.get("title", ""), # RENAMED from content
|
||||
"content": entry.get("content", ""), # KEEP for backward compat, or remove
|
||||
"status": entry.get("status", ""),
|
||||
...
|
||||
}
|
||||
```
|
||||
|
||||
### WR-02: Missing `verified` field in reading_log JSONL entries
|
||||
|
||||
**File:** `paperforge/memory/permanent.py:32-76`
|
||||
**Issue:** The SQL `reading_log` table (schema.py line 140-155) includes a `verified INTEGER DEFAULT 0` column, but the JSONL `append_reading_note` function (lines 53-65) never emits this field. If data is ever migrated between JSONL and SQL, verification status will be silently dropped.
|
||||
|
||||
**Fix:** Add `verified` to the entry dict with a default of `0` (or make it a parameter):
|
||||
```python
|
||||
entry: dict[str, object] = {
|
||||
...
|
||||
"verified": 0, # ADDED — matches SQL schema
|
||||
}
|
||||
```
|
||||
|
||||
### WR-03: Missing input validation on `append_project_entry`
|
||||
|
||||
**File:** `paperforge/memory/permanent.py:114-144`
|
||||
**Issue:** `append_reading_note` validates that `paper_id` and `excerpt` are non-empty before writing (lines 44-47). However, `append_project_entry` performs zero validation — it accepts an empty dict, producing a record where all fields default to empty strings/lists. By contrast, the parallel SQL `project_log` table enforces `NOT NULL` constraints on `project`, `date`, `type`, and `title`.
|
||||
|
||||
This inconsistency means the JSONL can accumulate garbage rows that would be rejected by the SQL backend. It also silently swallows caller errors where a required field was omitted.
|
||||
|
||||
**Fix:** Add validation for the fields that are semantically required:
|
||||
```python
|
||||
def append_project_entry(vault: Path, entry: dict) -> dict:
|
||||
project = entry.get("project", "")
|
||||
entry_type = entry.get("entry_type", "")
|
||||
if not project:
|
||||
return {"ok": False, "error": "project is required"}
|
||||
if not entry_type:
|
||||
return {"ok": False, "error": "entry_type is required"}
|
||||
# ... rest of function
|
||||
```
|
||||
|
||||
### WR-04: No tests for the new module
|
||||
|
||||
**File:** `paperforge/memory/permanent.py` (entire file)
|
||||
**Issue:** Zero tests exist for this module. The `tests/unit/memory/` directory has 6 test files (test_builder.py, test_context.py, test_query.py, test_refresh.py, test_schema.py, __init__.py) but none cover `permanent.py`. No entry in the integration or chaos test suites either.
|
||||
|
||||
With 10 functions covering two distinct log types, validation logic, error handling, and JSONL line format, the absence of tests means:
|
||||
- ID uniqueness (`secrets.token_hex`) is untested
|
||||
- Unicode handling (`ensure_ascii=False`) is untested
|
||||
- Malformed JSON skipping is untested
|
||||
- Empty-string validation is untested
|
||||
- OSError recovery on write is untested
|
||||
- Filtering (`get_reading_notes_for_paper`, `get_project_entries`) correctness is untested
|
||||
|
||||
**Fix:** Create `tests/unit/memory/test_permanent.py` covering at minimum:
|
||||
- `append_reading_note` / `append_project_entry` happy path
|
||||
- Validation rejection (empty paper_id, empty excerpt)
|
||||
- Round-trip: append then read-all
|
||||
- Filtered reads (`get_reading_notes_for_paper`, `get_project_entries`)
|
||||
- Malformed JSONL line skipping
|
||||
- Unicode content preservation
|
||||
|
||||
## Info
|
||||
|
||||
### IN-01: Inline filepath construction bypasses `get_*_log_path` helpers
|
||||
|
||||
**File:** `paperforge/memory/permanent.py:68, 136`
|
||||
**Issue:** Both `append_reading_note` and `append_project_entry` construct the filepath inline (`log_dir / "reading-log.jsonl"`) instead of calling `get_reading_log_path(vault)` and `get_project_log_path(vault)` which are defined directly above them. This duplicates the path logic and creates a risk of divergence if the path is ever changed in only one place.
|
||||
|
||||
**Fix:** Replace:
|
||||
```python
|
||||
log_dir = _ensure_logs_dir(vault)
|
||||
filepath = log_dir / "reading-log.jsonl"
|
||||
```
|
||||
with:
|
||||
```python
|
||||
filepath = _ensure_logs_dir(vault) / "reading-log.jsonl"
|
||||
```
|
||||
Or preferably, use the dedicated helpers:
|
||||
```python
|
||||
filepath = get_reading_log_path(vault)
|
||||
_ensure_logs_dir(vault) # ensure directory exists
|
||||
```
|
||||
|
||||
### IN-02: No module-level docstring
|
||||
|
||||
**File:** `paperforge/memory/permanent.py:1-11`
|
||||
**Issue:** The file lacks a module docstring. Other modules in the `paperforge/memory/` package describe their purpose and usage. A docstring would help future developers understand that this is a JSONL append-only log (not a queryable database) and why it exists alongside the SQL tables.
|
||||
|
||||
**Fix:** Add a module docstring:
|
||||
```python
|
||||
"""Permanent JSONL storage layer for reading-log and project-log.
|
||||
|
||||
Provides append-only JSONL persistence as an append-only archival backend
|
||||
alongside the query-oriented SQL tables. Each log entry is written as a
|
||||
single JSON line with a unique, collision-resistant ID generated via
|
||||
``secrets.token_hex``.
|
||||
|
||||
All write functions return ``{"ok": True/False, ...}`` dicts.
|
||||
Read functions return ``list[dict]`` with malformed lines silently skipped.
|
||||
"""
|
||||
```
|
||||
|
||||
### IN-03: `append_reading_note` does not validate `section` for emptiness
|
||||
|
||||
**File:** `paperforge/memory/permanent.py:32-76`
|
||||
**Issue:** `paper_id` and `excerpt` are validated as non-empty (lines 44-47), but `section` — a required positional parameter with no default — is not validated. The SQL `reading_log` schema also marks `section` as `TEXT NOT NULL` without a default. An empty `section` value would pass through silently and create an entry that's difficult to query or filter by section.
|
||||
|
||||
**Fix:** Add a validation check:
|
||||
```python
|
||||
if not section:
|
||||
return {"ok": False, "error": "section is required"}
|
||||
```
|
||||
|
||||
### IN-04: Module not exported from `paperforge/memory/__init__.py`
|
||||
|
||||
**File:** `paperforge/memory/__init__.py`
|
||||
**Issue:** The package `__init__.py` exports `get_connection`, `get_memory_db_path`, `ensure_schema`, and `drop_all_tables` — all SQL/db-related. The new `permanent` module is not re-exported, and no other module in the codebase imports it (`grep` returned zero results for `from paperforge.memory.*permanent`). This means the 154 lines of code are dead code until a consumer is wired in.
|
||||
|
||||
**Fix:** Add the key functions to the package's public API, or document in a follow-up task which command/worker will consume this module.
|
||||
|
||||
---
|
||||
|
||||
_Reviewed: 2026-05-14_
|
||||
_Reviewer: the agent (gsd-code-reviewer)_
|
||||
_Depth: standard_
|
||||
216
docs/superpowers/plans/2026-05-14-research-memory-core-REVIEW.md
Normal file
216
docs/superpowers/plans/2026-05-14-research-memory-core-REVIEW.md
Normal file
|
|
@ -0,0 +1,216 @@
|
|||
---
|
||||
phase: research-memory-core
|
||||
reviewed: 2026-05-14T12:00:00Z
|
||||
depth: deep
|
||||
files_reviewed: 2
|
||||
files_reviewed_list:
|
||||
- docs/superpowers/plans/2026-05-14-research-memory-core.md
|
||||
- docs/superpowers/plans/2026-05-14-research-memory-core-REVIEW.md
|
||||
findings:
|
||||
critical: 0
|
||||
warning: 3
|
||||
info: 2
|
||||
total: 5
|
||||
status: issues_found
|
||||
---
|
||||
|
||||
# Re-Review: Research Memory Core (post-critical-fix)
|
||||
|
||||
**Reviewed:** 2026-05-14 (second pass)
|
||||
**Depth:** deep (cross-file analysis against paperforge/core/, paperforge/memory/)
|
||||
**Prior Review:** `2026-05-14-research-memory-core-REVIEW.md` (5 critical, 6 warning, 4 info)
|
||||
**Status:** issues_found — 3 new warnings, 0 critical; all 11 prior issues verified fixed
|
||||
|
||||
---
|
||||
|
||||
## Summary
|
||||
|
||||
The updated plan has successfully addressed all 5 BLOCKER-level (critical) issues and all 6 WARNING-level issues from the prior review. The fixes are substantive: `ErrorCode.INVALID_INPUT` replaced with `VALIDATION_ERROR`, `--json`/`--payload` argument split corrected, hardcoded paths replaced with `paperforge_paths()`, schema version bumped to 2, stale `reading_log_v2.py` reference removed, and all six warning fixes applied (correction-note write path, input validation, JSON error logging, paper_events clearing, methodology placement documented, secure ID generation).
|
||||
|
||||
However, three NEW warnings were discovered — two are consistency gaps where the CR-02 fix was applied incompletely (stale documentation), and one is a data-loss design gap (correction notes lost on rebuild). None are blockers but should be addressed before implementation.
|
||||
|
||||
---
|
||||
|
||||
## Prior Fix Verification (PASS/FAIL per issue)
|
||||
|
||||
| Issue ID | Description | Status | Evidence |
|
||||
|----------|-------------|--------|----------|
|
||||
| **CR-01** | `ErrorCode.INVALID_INPUT` → `VALIDATION_ERROR` | **PASS** | Plan uses `ErrorCode.VALIDATION_ERROR` at lines 901, 908, 924, 936, 957; zero `INVALID_INPUT` references |
|
||||
| **CR-02** | `--json` rename → `--payload`, boolean `--json` flag | **PASS*** | Code blocks fixed (lines 788-795); see NEW-W01 for doc gaps |
|
||||
| **CR-03** | Hardcoded paths → `paperforge_paths()` | **PASS** | Both `_render_reading_log_md` (lines 533-534) and `_render_project_log_md` (lines 880-881) use config-derived paths |
|
||||
| **CR-04** | `CURRENT_SCHEMA_VERSION` bump | **PASS** | Line 148: `CURRENT_SCHEMA_VERSION = 2` with comment |
|
||||
| **CR-05** | Stale `reading_log_v2.py` removed | **PASS** | "New files" section (lines 61-64) lists only 3 files; zero `reading_log_v2` references |
|
||||
| **WR-01** | No `correction_note` write function | **PASS** | Task 3 Step 6 (lines 550-588) adds `--correct`/`--correction`/`--reason` CLI args + INSERT logic |
|
||||
| **WR-02** | No data validation in `append_reading_note` | **PASS** | Lines 230-233: `paper_id` and `excerpt` empty checks with error dict return |
|
||||
| **WR-03** | Silent JSON decode errors | **PASS** | Lines 277-280, 340-343: `logger.warning()` added in catch blocks |
|
||||
| **WR-04** | `paper_events` not cleared on rebuild | **PASS** | Task 6 Step 2 line 1078: `conn.execute("DELETE FROM paper_events;")` added |
|
||||
| **WR-05** | `METHODOLOGY_COMPACT.md` not discoverable | **PASS** | Task 7 (line 1099): documents it's intentionally outside `archive/`, with rationale |
|
||||
| **WR-06** | Weak UUID truncation | **PASS** | Lines 237, 304: `secrets.token_hex(4)` replaces `str(uuid.uuid4())[:6]` |
|
||||
|
||||
### CR-02 Detail (* qualified PASS)
|
||||
|
||||
The code blocks in Task 5 are correctly fixed — `--payload` accepts the JSON string and `--json` is a proper `store_true` boolean. However, two documentation sections were NOT updated (see NEW-W01 below). The code is correct; the surrounding specification text is stale.
|
||||
|
||||
---
|
||||
|
||||
## New Issues
|
||||
|
||||
### NEW-W01: Interface contract and smoke test retain old `--json '<payload>'` syntax
|
||||
|
||||
**File:** `docs/superpowers/plans/2026-05-14-research-memory-core.md`
|
||||
**Lines:** 46, 1170
|
||||
**Severity:** WARNING
|
||||
**Issue:** The CR-02 fix renamed the payload argument from `--json` to `--payload` in the Task 5 code blocks, but two documentation sections still reference the old syntax:
|
||||
|
||||
1. **Line 46** — Interface contract section:
|
||||
```
|
||||
paperforge project-log --write --json '<payload>' --vault $VAULT
|
||||
```
|
||||
|
||||
2. **Line 1170** — Smoke test command:
|
||||
```bash
|
||||
--json '{"date":"2026-05-14","type":"note","title":"test"}' \
|
||||
```
|
||||
|
||||
An implementer following the interface contract or smoke test would use `--json '<...>'` which now means "output as JSON" (boolean flag), not "JSON payload". The command would either error (boolean flag receiving a string value) or behave unexpectedly.
|
||||
|
||||
**Fix:**
|
||||
```markdown
|
||||
# Line 46: Change to:
|
||||
paperforge project-log --write --payload '<json_payload>' --vault $VAULT
|
||||
|
||||
# Line 1170: Change to:
|
||||
--payload '{"date":"2026-05-14","type":"note","title":"test"}' \
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
### NEW-W02: `correction_note` data has no JSONL backing — permanently lost on DB rebuild
|
||||
|
||||
**File:** `docs/superpowers/plans/2026-05-14-research-memory-core.md`
|
||||
**Lines:** 552-581 (correction write path), 1078 (DELETE FROM paper_events)
|
||||
**Severity:** WARNING
|
||||
**Issue:** Task 3 Step 6 writes `correction_note` events directly to `paper_events` (line 573-576). Task 6 clears `paper_events` on rebuild (`DELETE FROM paper_events;`, line 1078). However, unlike reading notes (which are dual-written to JSONL + paper_events), correction notes have **no JSONL equivalent** — they exist only in `paper_events`. On rebuild, all correction history is permanently lost.
|
||||
|
||||
Furthermore, the existing `drop_all_tables(conn)` in `builder.py:79` already drops `paper_events` when the schema version changes (since `paper_events` is in `ALL_TABLES`). The plan's added `DELETE FROM paper_events` handles same-version rebuilds, making the data-loss path even broader.
|
||||
|
||||
**Fix (Option A — preferred):** Mirror the dual-write pattern used for reading notes. When a correction is written, also append to a `corrections.jsonl` file. Add an `_import_corrections()` function in `builder.py` that restores them on rebuild.
|
||||
|
||||
**Fix (Option B — simpler):** Scope the `DELETE FROM paper_events` to exclude `correction_note` rows:
|
||||
```python
|
||||
conn.execute("DELETE FROM paper_events WHERE event_type != 'correction_note';")
|
||||
```
|
||||
|
||||
**Fix (Option C):** Document that correction notes are ephemeral and lost on rebuild, and ensure the `paper-context` command's `recheck_targets` warning makes this acceptable.
|
||||
|
||||
---
|
||||
|
||||
### NEW-W03: `append_reading_note()` failure silently ignored — PFResult always reports success
|
||||
|
||||
**File:** `docs/superpowers/plans/2026-05-14-research-memory-core.md`
|
||||
**Lines:** 432-466 (Task 3 Step 3)
|
||||
**Severity:** WARNING
|
||||
**Issue:** In the `reading_log.py` write section, the code calls `append_reading_note()` which returns `{ok, id, path}` or `{ok: False, error: str}`. However, the surrounding logic hardcodes `ok = True` (line 460) without checking the return value:
|
||||
|
||||
```python
|
||||
result = append_reading_note(vault, args.paper_id, ...)
|
||||
# ... dual-write to paper_events ...
|
||||
|
||||
ok = True # Always True — ignores result["ok"]
|
||||
result_obj = PFResult(
|
||||
ok=ok,
|
||||
command="reading-log",
|
||||
version=PF_VERSION,
|
||||
data={"written": ok, "id": result.get("id")}, # "id" is None on failure
|
||||
)
|
||||
```
|
||||
|
||||
If JSONL write fails (disk full, permission error), `result["ok"]` is `False`, `result["id"]` is `None`, but the PFResult contract reports `{ok: true, data: {written: true, id: null}}`. The CLI consumer has no way to detect the failure.
|
||||
|
||||
**Fix:**
|
||||
```python
|
||||
result = append_reading_note(vault, args.paper_id, ...)
|
||||
if not result.get("ok"):
|
||||
result_obj = PFResult(
|
||||
ok=False, command="reading-log", version=PF_VERSION,
|
||||
error=PFError(code=ErrorCode.INTERNAL_ERROR,
|
||||
message=result.get("error", "Failed to write reading note")),
|
||||
)
|
||||
# ... output and return
|
||||
# Continue with paper_events write only if JSONL succeeded
|
||||
write_reading_note(...)
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## Info (Non-blocking observations)
|
||||
|
||||
### NEW-IN01: Duplicate step numbering in Task 3
|
||||
|
||||
**File:** `docs/superpowers/plans/2026-05-14-research-memory-core.md`
|
||||
**Lines:** 378, 389, 412, 470, 550, 590, 607
|
||||
**Severity:** INFO
|
||||
**Issue:** Task 3 step numbering jumps: Step 1 (l.378), Step 2 (l.389), Step 3 (l.412), Step 4 (l.470), Step 6 (l.550), Step 7 (l.590), Step 6 (l.607, again). There is no Step 5, and two different sections are both labeled "Step 6". This could confuse implementers tracking progress.
|
||||
**Fix:** Renumber sequentially: Step 5 for correction write support (l.550), Step 6 for render dispatch (l.590), Step 7 for commit (l.607).
|
||||
|
||||
### NEW-IN02: Misleading error code in `paper_context.py`
|
||||
|
||||
**File:** `docs/superpowers/plans/2026-05-14-research-memory-core.md`
|
||||
**Line:** 736
|
||||
**Severity:** INFO
|
||||
**Issue:** The `paper_context.py` `run()` function uses `ErrorCode.PATH_NOT_FOUND` when no paper is found in the database for a given zotero_key. `PATH_NOT_FOUND` (defined in `errors.py:24` under "Config / Vault") is semantically meant for filesystem or vault path lookup failures, not database record lookups. Using the wrong error code category could mislead downstream consumers (plugin UI generating fix-it guidance for a "missing path" when the real issue is a missing paper key).
|
||||
**Fix:** Use `ErrorCode.VALIDATION_ERROR` or add a purpose-specific code like `PAPER_NOT_FOUND`:
|
||||
```python
|
||||
error=PFError(code=ErrorCode.VALIDATION_ERROR,
|
||||
message=f"No paper found for key: {key}")
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## Task-by-Task Re-Assessment
|
||||
|
||||
| Task | Prior | Current | Notes |
|
||||
|------|-------|---------|-------|
|
||||
| 1: DB Schema | PASS* | **PASS** | Schema version bumped, new tables registered |
|
||||
| 2: JSONL Storage | PASS* | **PASS** | Validation added, JSON errors logged, secure IDs |
|
||||
| 3: Reading-Log CLI | PASS* | **PASS** | All prior issues fixed; NEW-W03 (unchecked result) to address |
|
||||
| 4: Paper-Context CLI | PASS* | **PASS** | Correction read path now has matching write path; NEW-IN02 minor |
|
||||
| 5: Project-Log CLI | **FAIL** → **PASS** | CR-01/CR-02/CR-03 all fixed; NEW-W01 (stale spec docs) to address |
|
||||
| 6: DB Builder | PASS* | **PASS** | paper_events cleared; NEW-W02 (correction loss on rebuild) to address |
|
||||
| 7: METHODOLOGY_COMPACT | PASS* | **PASS** | Placement documented as intentional |
|
||||
| 8: Integration | PASS | **PASS** | Smoke test needs --payload fix (NEW-W01) |
|
||||
|
||||
---
|
||||
|
||||
## Cross-File Consistency Check
|
||||
|
||||
| Check | Result |
|
||||
|-------|--------|
|
||||
| `paperforge_paths()` used consistently across render functions | PASS |
|
||||
| `ErrorCode` values all exist in `errors.py` enum | PASS (verified PATH_NOT_FOUND, VALIDATION_ERROR both exist) |
|
||||
| `ALL_TABLES` includes `reading_log` and `project_log` | PASS (line 142) |
|
||||
| `ensure_schema()` creates both new tables | PASS (lines 135-136) |
|
||||
| `builder.py` delete/add order correct (DELETE before INSERT) | PASS |
|
||||
| Interface contract matches CLI implementation | FAIL — line 46 uses `--json` not `--payload` |
|
||||
| Smoke test commands match CLI implementation | FAIL — line 1170 uses `--json` not `--payload` |
|
||||
| `paper_events` lifecycle documented | PARTIAL — cleared on rebuild but correction notes not restored |
|
||||
|
||||
---
|
||||
|
||||
## Dependency Graph: Still Verified
|
||||
|
||||
```
|
||||
Task 2 (JSONL) → Tasks 3, 4, 5, 6
|
||||
Task 7 is independent
|
||||
Task 8 is integration (depends on all)
|
||||
```
|
||||
|
||||
No circular dependencies introduced.
|
||||
|
||||
---
|
||||
|
||||
_Reviewed: 2026-05-14_
|
||||
_Reviewer: the agent (gsd-code-reviewer)_
|
||||
_Depth: deep_
|
||||
_Prior fixes verified: 11/11 PASS (CR-01 through CR-05, WR-01 through WR-06)_
|
||||
_New issues found: 3 warnings, 2 info_
|
||||
1206
docs/superpowers/plans/2026-05-14-research-memory-core.md
Normal file
1206
docs/superpowers/plans/2026-05-14-research-memory-core.md
Normal file
File diff suppressed because it is too large
Load diff
364
docs/superpowers/plans/2026-05-14-user-side-stabilization.md
Normal file
364
docs/superpowers/plans/2026-05-14-user-side-stabilization.md
Normal file
|
|
@ -0,0 +1,364 @@
|
|||
# User-Side Skill Stabilization Plan
|
||||
|
||||
> **For agentic workers:** Use subagent-driven-development to implement.
|
||||
> Steps use checkbox (`- [ ]`) syntax for tracking.
|
||||
|
||||
**Goal:** Make paperforge skill + memory layer reliable for local self-use — no data loss, no split truth sources, no agent misrouting.
|
||||
|
||||
**Architecture:** JSONL as single source of truth, SQLite as derived index, CLI as sole write interface, bootstrap as sole path resolver.
|
||||
|
||||
**Tech Stack:** Python, JSONL, SQLite, paperforge CLI
|
||||
|
||||
---
|
||||
|
||||
## Task 1: Unify reading-log data flow (JSONL as single source)
|
||||
|
||||
**Files:**
|
||||
- Modify: `paperforge/commands/reading_log.py` (lookup + export data sources, remove write_reading_note call)
|
||||
- Modify: `paperforge/memory/events.py` (mark write_reading_note deprecated)
|
||||
|
||||
**Goal:** `--lookup` and `export` read from JSONL, not paper_events. `--write` only calls `append_reading_note`.
|
||||
|
||||
- [ ] **Step 1: Change lookup_paper_events to read from JSONL**
|
||||
|
||||
In `reading_log.py`, replace `lookup_paper_events()` body to use `get_reading_notes_for_paper()` from JSONL instead of querying paper_events. Keep the function return shape the same. Join with papers table for title/year/citation_key.
|
||||
|
||||
```python
|
||||
def lookup_paper_events(vault: Path, key: str) -> dict:
|
||||
notes = get_reading_notes_for_paper(vault, key)
|
||||
if not notes:
|
||||
return {"ok": True, "zotero_key": key, "title": "", "entries": [], "count": 0}
|
||||
|
||||
# Try to get paper metadata from DB for richer display
|
||||
db_path = get_memory_db_path(vault)
|
||||
title = ""
|
||||
if db_path.exists():
|
||||
conn = get_connection(db_path, read_only=True)
|
||||
try:
|
||||
row = conn.execute(
|
||||
"SELECT title FROM papers WHERE zotero_key = ?", (key,)
|
||||
).fetchone()
|
||||
if row:
|
||||
title = row["title"] or ""
|
||||
finally:
|
||||
conn.close()
|
||||
|
||||
entries = []
|
||||
for n in notes:
|
||||
entries.append({
|
||||
"created_at": n.get("created_at", ""),
|
||||
"section": n.get("section", ""),
|
||||
"excerpt": n.get("excerpt", ""),
|
||||
"usage": n.get("usage", ""),
|
||||
"note": n.get("note", ""),
|
||||
})
|
||||
return {"ok": True, "zotero_key": key, "title": title, "entries": entries, "count": len(entries)}
|
||||
```
|
||||
|
||||
- [ ] **Step 2: Change export_reading_log to read from JSONL**
|
||||
|
||||
In `events.py`, add a new `export_reading_log_from_jsonl()` that reads from `read_all_reading_notes()`, joins with DB for metadata. Or in `reading_log.py`, replace the export path (around line 286) to use JSONL directly with a local join helper.
|
||||
|
||||
- [ ] **Step 3: Remove write_reading_note from write path**
|
||||
|
||||
In `reading_log.py` `run()`, remove the `write_reading_note()` call (the second write). Only `append_reading_note()` remains.
|
||||
|
||||
- [ ] **Step 4: Mark write_reading_note deprecated**
|
||||
|
||||
Add deprecation docstring to `write_reading_note` in `events.py`. Do NOT remove the function (backward compat for import_reading_log which may still use it).
|
||||
|
||||
- [ ] **Step 5: Commit**
|
||||
|
||||
```bash
|
||||
git add paperforge/commands/reading_log.py paperforge/memory/events.py
|
||||
git commit -m "refactor: unify reading-log reads to JSONL, deprecate paper_events writes"
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## Task 2: Permanent correction-log.jsonl
|
||||
|
||||
**Files:**
|
||||
- Modify: `paperforge/memory/permanent.py` (add correction append/read functions)
|
||||
- Modify: `paperforge/commands/paper_context.py` (read corrections from JSONL, fix ref_id → original_id)
|
||||
- Modify: `paperforge/commands/reading_log.py` (write correction to JSONL, not just paper_events)
|
||||
- Modify: `paperforge/memory/builder.py` (import corrections on rebuild)
|
||||
|
||||
**Goal:** Correction notes live in `correction-log.jsonl`, survive DB rebuilds.
|
||||
|
||||
- [ ] **Step 1: Add correction helpers to permanent.py**
|
||||
|
||||
```python
|
||||
# ── Correction Log ─────────────────────────────────────────────────────────
|
||||
|
||||
def get_correction_log_path(vault: Path) -> Path:
|
||||
return _logs_dir(vault) / "correction-log.jsonl"
|
||||
|
||||
|
||||
def append_correction(
|
||||
vault: Path,
|
||||
paper_id: str,
|
||||
original_id: str,
|
||||
correction: str,
|
||||
reason: str = "",
|
||||
agent: str = "",
|
||||
) -> dict:
|
||||
if not paper_id:
|
||||
return {"ok": False, "error": "paper_id is required"}
|
||||
if not original_id:
|
||||
return {"ok": False, "error": "original_id is required"}
|
||||
if not correction:
|
||||
return {"ok": False, "error": "correction is required"}
|
||||
|
||||
date_str = datetime.date.today().strftime("%Y%m%d")
|
||||
entry_id = f"corr_{date_str}_{secrets.token_hex(4)}"
|
||||
now = datetime.datetime.now(datetime.timezone.utc).isoformat()
|
||||
|
||||
entry: dict[str, object] = {
|
||||
"id": entry_id,
|
||||
"event_type": "correction",
|
||||
"created_at": now,
|
||||
"paper_id": paper_id,
|
||||
"original_id": original_id,
|
||||
"correction": correction,
|
||||
"reason": reason,
|
||||
"agent": agent,
|
||||
}
|
||||
|
||||
log_dir = _ensure_logs_dir(vault)
|
||||
filepath = log_dir / "correction-log.jsonl"
|
||||
|
||||
try:
|
||||
with filepath.open("a", encoding="utf-8") as f:
|
||||
f.write(json.dumps(entry, ensure_ascii=False) + "\n")
|
||||
except OSError as e:
|
||||
return {"ok": False, "error": str(e)}
|
||||
|
||||
return {"ok": True, "id": entry_id, "path": str(filepath)}
|
||||
|
||||
|
||||
def read_all_corrections(vault: Path) -> list[dict]:
|
||||
filepath = get_correction_log_path(vault)
|
||||
return _read_jsonl(filepath)
|
||||
|
||||
|
||||
def get_corrections_for_paper(vault: Path, paper_id: str) -> list[dict]:
|
||||
all_corrections = read_all_corrections(vault)
|
||||
return [c for c in all_corrections if c.get("paper_id") == paper_id]
|
||||
```
|
||||
|
||||
- [ ] **Step 2: Fix paper_context.py to read from JSONL**
|
||||
|
||||
Replace the `paper_events` query for corrections with `get_corrections_for_paper()`. Fix `ref_id` → `original_id`.
|
||||
|
||||
- [ ] **Step 3: Dual-write correction in reading_log.py**
|
||||
|
||||
In `reading_log.py` `run()`, when writing a correction, write to BOTH:
|
||||
1. `append_correction()` → `correction-log.jsonl` (permanent)
|
||||
2. `write_correction_note()` → `paper_events` (FTS search, optional — keep for now)
|
||||
|
||||
- [ ] **Step 4: Add correction import to builder.py**
|
||||
|
||||
Add `_import_correction_log()` that reads `correction-log.jsonl` and inserts into paper_events (so corrections are searchable after rebuild).
|
||||
|
||||
- [ ] **Step 5: Commit**
|
||||
|
||||
```bash
|
||||
git add paperforge/memory/permanent.py paperforge/commands/paper_context.py \
|
||||
paperforge/commands/reading_log.py paperforge/memory/builder.py
|
||||
git commit -m "feat: permanent correction-log.jsonl, fix original_id field alignment"
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## Task 3: Fix pf_bootstrap.py path resolution + Python fallback
|
||||
|
||||
**Files:**
|
||||
- Modify: `paperforge/skills/paperforge/scripts/pf_bootstrap.py`
|
||||
|
||||
**Goal:** Support `vault_config` nested config, fallback Python to `python`/`python3`/`sys.executable`.
|
||||
|
||||
- [ ] **Step 1: Add resolve_cfg()**
|
||||
|
||||
```python
|
||||
DEFAULTS = {
|
||||
"system_dir": "System",
|
||||
"resources_dir": "Resources",
|
||||
"literature_dir": "Literature",
|
||||
"control_dir": "LiteratureControl",
|
||||
"base_dir": "Bases",
|
||||
}
|
||||
|
||||
def resolve_cfg(raw: dict) -> dict:
|
||||
cfg = DEFAULTS.copy()
|
||||
nested = raw.get("vault_config", {})
|
||||
if isinstance(nested, dict):
|
||||
cfg.update({k: v for k, v in nested.items() if v})
|
||||
cfg.update({k: raw[k] for k in DEFAULTS if raw.get(k)})
|
||||
return cfg
|
||||
```
|
||||
|
||||
Replace direct `cfg.get(...)` calls with `cfg = resolve_cfg(...)`.
|
||||
|
||||
- [ ] **Step 2: Fix Python fallback**
|
||||
|
||||
In `_find_python_with_paperforge()`, if no paperforge-capable Python is found:
|
||||
- Try `python`, `python3`
|
||||
- Fall back to `sys.executable`
|
||||
- Return `"python"` (not None) with `python_verified: false`
|
||||
|
||||
In `main()`, after python_candidate:
|
||||
```python
|
||||
if result.get("python_candidate"):
|
||||
result["python_verified"] = True
|
||||
else:
|
||||
result["python_candidate"] = "python"
|
||||
result["python_verified"] = False
|
||||
```
|
||||
|
||||
- [ ] **Step 3: Commit**
|
||||
|
||||
```bash
|
||||
git add paperforge/skills/paperforge/scripts/pf_bootstrap.py
|
||||
git commit -m "fix: bootstrap vault_config nest, python fallback with verified flag"
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## Task 4: FTS safe query with fallback
|
||||
|
||||
**Files:**
|
||||
- Modify: `paperforge/memory/fts.py`
|
||||
|
||||
**Goal:** Three-level fallback for FTS queries.
|
||||
|
||||
- [ ] **Step 1: Add tokenizer and fallback logic**
|
||||
|
||||
```python
|
||||
import re
|
||||
|
||||
def tokenize_for_fts(q: str) -> str:
|
||||
"""Extract alphanumeric + CJK tokens and quote them for safe FTS."""
|
||||
tokens = re.findall(r"[\w\u4e00-\u9fff]+", q)
|
||||
if not tokens:
|
||||
return q
|
||||
return " OR ".join(f'"{t}"' for t in tokens)
|
||||
|
||||
|
||||
def _fts_search(conn, query, params, limit):
|
||||
"""Try raw FTS, fall back to quoted tokens, then LIKE."""
|
||||
from contextlib import closing
|
||||
|
||||
conditions = ["paper_fts MATCH ?"]
|
||||
all_params = [query] + params
|
||||
|
||||
try:
|
||||
where = " AND ".join(conditions)
|
||||
sql = f"""
|
||||
SELECT ... FROM paper_fts f JOIN papers p ON p.rowid = f.rowid
|
||||
WHERE {where} ORDER BY rank LIMIT ?
|
||||
"""
|
||||
all_params.append(limit)
|
||||
conn.row_factory = sqlite3.Row
|
||||
return conn.execute(sql, all_params).fetchall()
|
||||
except sqlite3.OperationalError:
|
||||
pass
|
||||
|
||||
# Level 2: quoted tokens
|
||||
try:
|
||||
token_query = tokenize_for_fts(query)
|
||||
all_params = [token_query] + params + [limit]
|
||||
sql = f"""
|
||||
SELECT ... FROM paper_fts f JOIN papers p ON p.rowid = f.rowid
|
||||
WHERE paper_fts MATCH ? {'AND ...' * len(params)} ORDER BY rank LIMIT ?
|
||||
"""
|
||||
return conn.execute(sql, all_params).fetchall()
|
||||
except sqlite3.OperationalError:
|
||||
pass
|
||||
|
||||
# Level 3: LIKE fallback
|
||||
like_param = f"%{query}%"
|
||||
conditions = [
|
||||
"(p.title LIKE ? OR p.abstract LIKE ? OR p.doi LIKE ? OR p.citation_key LIKE ?)"
|
||||
]
|
||||
all_params = [like_param, like_param, like_param, like_param] + params + [limit]
|
||||
# ... construct and execute LIKE query
|
||||
return rows
|
||||
```
|
||||
|
||||
- [ ] **Step 2: Update search_papers() to use _fts_search**
|
||||
|
||||
Replace the direct FTS query with the safe fallback.
|
||||
|
||||
- [ ] **Step 3: Commit**
|
||||
|
||||
```bash
|
||||
git add paperforge/memory/fts.py
|
||||
git commit -m "feat: safe FTS search with token-quote and LIKE fallback"
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## Task 5: SKILL.md — router + PYTHON fallback + workflow rules
|
||||
|
||||
**Files:**
|
||||
- Create: `paperforge/skills/paperforge/workflows/project-engineering.md`
|
||||
- Modify: `paperforge/skills/paperforge/SKILL.md`
|
||||
- Modify: `paperforge/skills/paperforge/workflows/deep-reading.md`
|
||||
- Modify: `paperforge/skills/paperforge/workflows/paper-qa.md`
|
||||
- Modify: `paperforge/skills/paperforge/workflows/project-log.md`
|
||||
- Modify: `paperforge/skills/paperforge/workflows/methodology.md`
|
||||
|
||||
**Goal:** Engineering router, PYTHON fallback, workflow safety reinforcement.
|
||||
|
||||
- [ ] **Step 1: Add PYTHON fallback to SKILL.md**
|
||||
|
||||
After bootstrap section, add:
|
||||
```markdown
|
||||
If `python_verified` is `false` or `python_candidate` is `null`:
|
||||
Try `python` → `python3` → `sys.executable` in order.
|
||||
If all fail, stop and tell user to set `python_path` in `paperforge.json`.
|
||||
```
|
||||
|
||||
- [ ] **Step 2: Add project-engineering to router table**
|
||||
|
||||
Add to routing table:
|
||||
```markdown
|
||||
| "branch" "代码审查" "feature" "dashboard" "memory layer" "用户反馈" "报错" "安装失败" "Git" "Zotero" "BetterBibTeX" "OCR" "插件" | `workflows/project-engineering.md` |
|
||||
```
|
||||
|
||||
- [ ] **Step 3: Create project-engineering workflow stub**
|
||||
|
||||
Minimal workflow for engineering tasks — routes to appropriate context retrieval.
|
||||
|
||||
- [ ] **Step 4: Reinforce safety rule in deep-reading.md and paper-qa.md**
|
||||
|
||||
Add at top of each:
|
||||
```markdown
|
||||
> Prior reading-log entries are **recheck targets only**, never factual answers.
|
||||
> Always verify against original source before using any reading-log content.
|
||||
```
|
||||
|
||||
- [ ] **Step 5: Clarify project-log vs methodology boundary**
|
||||
|
||||
In project-log.md: "Record what happened this session."
|
||||
In methodology.md: "Only archive methods reusable across multiple projects/tasks."
|
||||
|
||||
- [ ] **Step 6: Commit**
|
||||
|
||||
```bash
|
||||
git add paperforge/skills/paperforge/
|
||||
git commit -m "chore: add PYTHON fallback, engineering router, workflow safety rules"
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## Summary
|
||||
|
||||
| Task | Priority | Files | Risk |
|
||||
|------|----------|-------|------|
|
||||
| 1 — reading-log unify | P0 | reading_log.py, events.py | Low — just changes data source |
|
||||
| 2 — correction permanent | P0 | permanent.py, paper_context.py, reading_log.py, builder.py | Low — new file, additive |
|
||||
| 3 — bootstrap fix | P0 | pf_bootstrap.py | Low — no changes to CLI |
|
||||
| 4 — FTS safe query | P0 | fts.py | Low — additive fallback |
|
||||
| 5 — SKILL/router/rules | P1 | 6 files under skills/ | Trivial — docs only |
|
||||
398
docs/superpowers/plans/2026-05-14-vector-stabilization.md
Normal file
398
docs/superpowers/plans/2026-05-14-vector-stabilization.md
Normal file
|
|
@ -0,0 +1,398 @@
|
|||
# Vector DB Flow Stabilization Plan
|
||||
|
||||
> **For agentic workers:** Use subagent-driven-development to implement.
|
||||
> Steps use checkbox (`- [ ]`) syntax for tracking.
|
||||
|
||||
**Goal:** Make the user flow "install → enable vector DB → embed build → retrieve" stable — no missing deps, no silent failures, no misleading UI.
|
||||
|
||||
**Architecture:** Plugin settings drive the feature toggle; CLI preflight checks all deps before build; retrieve refuses to query empty index.
|
||||
|
||||
**Tech Stack:** esbuild (plugin), chromadb, sentence-transformers, openai, Python CLI
|
||||
|
||||
---
|
||||
|
||||
## Task 1: Remove ghost Setting code from main.js
|
||||
|
||||
**Files:**
|
||||
- Modify: `paperforge/plugin/main.js:200-215`
|
||||
|
||||
**Goal:** Delete the misplaced `new Setting(containerEl)` block that sits inside `runSubprocess`. All vector settings already exist correctly in `_renderFeaturesTab`.
|
||||
|
||||
- [ ] **Step 1: Delete lines 200-215**
|
||||
|
||||
Remove the extra `});` and the two `new Setting(containerEl).setName('API Model')` blocks that are at lines 200-215.
|
||||
|
||||
The code to remove:
|
||||
```javascript
|
||||
});
|
||||
new Setting(containerEl)
|
||||
.setName('API Model')
|
||||
.setDesc('Which OpenAI-compatible embedding model to use.')
|
||||
new Setting(containerEl)
|
||||
.setName('API Model')
|
||||
.setDesc('Embedding model name (e.g., text-embedding-3-small, qwen-3-embedding)')
|
||||
.addText(text => {
|
||||
text.setPlaceholder('text-embedding-3-small')
|
||||
.setValue(this.plugin.settings.vector_db_api_model || 'text-embedding-3-small')
|
||||
.onChange(value => {
|
||||
this.plugin.settings.vector_db_api_model = value;
|
||||
this.plugin.saveSettings();
|
||||
});
|
||||
});
|
||||
}
|
||||
```
|
||||
|
||||
- [ ] **Step 2: Verify syntax**
|
||||
|
||||
Run: `node --check paperforge/plugin/main.js`
|
||||
Expected: PASS (exit code 0)
|
||||
|
||||
- [ ] **Step 3: Commit**
|
||||
|
||||
```bash
|
||||
git add paperforge/plugin/main.js
|
||||
git commit -m "fix: remove ghost vector API Model setting from runSubprocess scope"
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## Task 2: Add vector optional dependencies to pyproject.toml
|
||||
|
||||
**Files:**
|
||||
- Modify: `pyproject.toml`
|
||||
|
||||
**Goal:** Users can `pip install paperforge[vector]` to get chromadb + embeddings deps.
|
||||
|
||||
- [ ] **Step 1: Add vector extra**
|
||||
|
||||
In `pyproject.toml`, under `[project]` (or at end of file), add:
|
||||
|
||||
```toml
|
||||
[project.optional-dependencies]
|
||||
vector = [
|
||||
"chromadb>=0.5.0",
|
||||
"sentence-transformers>=3.0.0",
|
||||
"openai>=1.0.0",
|
||||
]
|
||||
```
|
||||
|
||||
- [ ] **Step 2: Add install hint to plugin setup page**
|
||||
|
||||
In `main.js` `_renderSetupTab()`, after dependency check output, add a notice:
|
||||
|
||||
```javascript
|
||||
// In the deps status display section, add:
|
||||
const vectorHint = containerEl.createEl('p', {
|
||||
cls: 'paperforge-settings-desc',
|
||||
text: 'Vector Database requires additional dependencies. '
|
||||
+ 'Run: pip install "paperforge[vector]"'
|
||||
});
|
||||
```
|
||||
|
||||
- [ ] **Step 3: Verify**
|
||||
|
||||
Run: `python -c "import chromadb; import sentence_transformers; import openai; print('vector deps OK')"`
|
||||
(Only needed if deps are already installed — otherwise verify pyproject.toml syntax)
|
||||
|
||||
- [ ] **Step 4: Commit**
|
||||
|
||||
```bash
|
||||
git add pyproject.toml paperforge/plugin/main.js
|
||||
git commit -m "feat: add vector optional deps and install hint in plugin UI"
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## Task 3: Add preflight to embed build
|
||||
|
||||
**Files:**
|
||||
- Modify: `paperforge/worker/vector_db.py` (add `_preflight_check()`)
|
||||
- Modify: `paperforge/commands/embed.py` (call preflight before build)
|
||||
|
||||
**Goal:** Before building embeddings, check all prerequisites and give clear error messages.
|
||||
|
||||
- [ ] **Step 1: Add _preflight_check() to vector_db.py**
|
||||
|
||||
```python
|
||||
def _preflight_check(vault: Path, settings: dict) -> dict:
|
||||
"""Check all prerequisites for embed build.
|
||||
|
||||
Returns {"ok": True} or {"ok": False, "error": "...", "fix": "..."}
|
||||
"""
|
||||
# 1. Check vector_db feature toggle
|
||||
if not settings.get("features", {}).get("vector_db", False):
|
||||
return {
|
||||
"ok": False,
|
||||
"error": "Vector DB is not enabled",
|
||||
"fix": "Enable 'Vector Database' in PaperForge plugin settings (Features tab).",
|
||||
}
|
||||
|
||||
# 2. Check chromadb import
|
||||
try:
|
||||
import chromadb
|
||||
except ImportError:
|
||||
return {
|
||||
"ok": False,
|
||||
"error": "chromadb is not installed",
|
||||
"fix": 'Run: pip install "paperforge[vector]"',
|
||||
}
|
||||
|
||||
# 3. Check mode-specific deps
|
||||
mode = settings.get("vector_db_mode", "local")
|
||||
if mode == "local":
|
||||
try:
|
||||
import sentence_transformers
|
||||
except ImportError:
|
||||
return {
|
||||
"ok": False,
|
||||
"error": "sentence-transformers is not installed (required for local mode)",
|
||||
"fix": 'Run: pip install "paperforge[vector]" or switch to API mode in plugin settings.',
|
||||
}
|
||||
elif mode == "api":
|
||||
try:
|
||||
import openai
|
||||
except ImportError:
|
||||
return {
|
||||
"ok": False,
|
||||
"error": "openai is not installed (required for API mode)",
|
||||
"fix": 'Run: pip install "paperforge[vector]" or switch to local mode in plugin settings.',
|
||||
}
|
||||
api_key = (
|
||||
settings.get("vector_db_api_key")
|
||||
or os.environ.get("OPENAI_API_KEY")
|
||||
or os.environ.get("VECTOR_DB_API_KEY")
|
||||
)
|
||||
if not api_key:
|
||||
return {
|
||||
"ok": False,
|
||||
"error": "API key not configured for API mode",
|
||||
"fix": "Set API Key in PaperForge plugin settings (Features tab) or OPENAI_API_KEY in .env.",
|
||||
}
|
||||
|
||||
# 4. Check OCR done papers
|
||||
from paperforge.worker._utils import pipeline_paths, read_json
|
||||
paths = pipeline_paths(vault)
|
||||
index_path = paths.get("indexes", Path()) / "formal-library.json"
|
||||
if not index_path.exists():
|
||||
return {
|
||||
"ok": False,
|
||||
"error": "Index not found",
|
||||
"fix": "Run paperforge memory build first.",
|
||||
}
|
||||
|
||||
index_data = read_json(index_path)
|
||||
items = index_data.get("items", []) if isinstance(index_data, dict) else index_data
|
||||
papers_with_ocr = [item for item in (items or []) if item.get("ocr_status") == "done"]
|
||||
|
||||
if not papers_with_ocr:
|
||||
return {
|
||||
"ok": False,
|
||||
"error": "No papers with OCR completed",
|
||||
"fix": "Run paperforge ocr first, or set do_ocr: true on papers with PDFs.",
|
||||
}
|
||||
|
||||
# 5. Check fulltext files exist
|
||||
unreadable = 0
|
||||
for item in papers_with_ocr[:5]: # sample check
|
||||
fulltext = item.get("fulltext_path", "")
|
||||
if fulltext and not Path(fulltext).exists():
|
||||
unreadable += 1
|
||||
|
||||
return {
|
||||
"ok": True,
|
||||
"ocr_done_count": len(papers_with_ocr),
|
||||
"fulltext_unreachable_sample": unreadable,
|
||||
}
|
||||
```
|
||||
|
||||
- [ ] **Step 2: Call preflight in embed build**
|
||||
|
||||
In `embed.py` (or wherever `embed_build` is called), before the main loop:
|
||||
|
||||
```python
|
||||
settings = read_plugin_settings(vault) # read from data.json
|
||||
preflight = _preflight_check(vault, settings)
|
||||
if not preflight["ok"]:
|
||||
return PFResult(
|
||||
ok=False,
|
||||
command="embed-build",
|
||||
version=PF_VERSION,
|
||||
error=PFError(code=ErrorCode.VALIDATION_ERROR, message=preflight["error"]),
|
||||
data={"fix": preflight.get("fix", "")},
|
||||
)
|
||||
```
|
||||
|
||||
- [ ] **Step 3: Verify**
|
||||
|
||||
Test manually: `python -m paperforge embed build` without vector deps → should get clear error.
|
||||
|
||||
- [ ] **Step 4: Commit**
|
||||
|
||||
```bash
|
||||
git add paperforge/worker/vector_db.py paperforge/commands/embed.py
|
||||
git commit -m "feat: add preflight checks before embed build"
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## Task 4: Gating — respect feature toggle in CLI
|
||||
|
||||
**Files:**
|
||||
- Modify: `paperforge/worker/vector_db.py` (preflight already checks this)
|
||||
- Modify: `paperforge/skills/paperforge/scripts/pf_bootstrap.py` (fix vector_search reading)
|
||||
|
||||
**Goal:** If user hasn't enabled vector_db in settings, CLI should refuse to build, and bootstrap should report accurately.
|
||||
|
||||
- [ ] **Step 1: Verify preflight checks vector_db toggle**
|
||||
|
||||
(Already done in Task 3 step 1 — the `_preflight_check` checks `features.vector_db`)
|
||||
|
||||
- [ ] **Step 2: Fix bootstrap vector_search field**
|
||||
|
||||
In `pf_bootstrap.py`, change the memory_layer check to:
|
||||
|
||||
```python
|
||||
# Read plugin data.json for real settings
|
||||
dc_json = vault / ".obsidian" / "plugins" / "paperforge" / "data.json"
|
||||
vector_enabled = False
|
||||
if dc_json.exists():
|
||||
try:
|
||||
with open(dc_json, encoding="utf-8") as f:
|
||||
plugin_data = json.load(f)
|
||||
vector_enabled = plugin_data.get("features", {}).get("vector_db", False)
|
||||
except:
|
||||
pass
|
||||
memory_layer["vector_search"] = vector_enabled
|
||||
```
|
||||
|
||||
(Current code already does this — verify it reads from the right path.)
|
||||
|
||||
- [ ] **Step 3: Commit**
|
||||
|
||||
```bash
|
||||
git add paperforge/skills/paperforge/scripts/pf_bootstrap.py
|
||||
git commit -m "fix: ensure bootstrap reads vector_db toggle from plugin settings"
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## Task 5: Retrieve guard — refuse empty index
|
||||
|
||||
**Files:**
|
||||
- Modify: `paperforge/commands/retrieve.py`
|
||||
- Modify: `paperforge/worker/vector_db.py` (add get_embed_status)
|
||||
|
||||
**Goal:** `paperforge retrieve` checks if index has chunks before loading models.
|
||||
|
||||
- [ ] **Step 1: Add get_embed_status() to vector_db.py**
|
||||
|
||||
```python
|
||||
def get_embed_status(vault: Path) -> dict:
|
||||
"""Check if vector index exists and has content."""
|
||||
from paperforge.config import paperforge_paths
|
||||
paths = paperforge_paths(vault)
|
||||
vectors_dir = paths.get("vectors", paths["paperforge"] / "vectors")
|
||||
|
||||
status = {
|
||||
"exists": False,
|
||||
"chunk_count": 0,
|
||||
"collection_name": "",
|
||||
"embedding_model": "",
|
||||
}
|
||||
|
||||
if not vectors_dir.exists():
|
||||
return status
|
||||
|
||||
# Try to read Chroma collection metadata
|
||||
try:
|
||||
import chromadb
|
||||
client = chromadb.PersistentClient(path=str(vectors_dir))
|
||||
collections = client.list_collections()
|
||||
if collections:
|
||||
col = collections[0]
|
||||
status["exists"] = True
|
||||
status["collection_name"] = col.name
|
||||
status["chunk_count"] = col.count()
|
||||
status["embedding_model"] = col.metadata.get("embedding_model", "") if col.metadata else ""
|
||||
except Exception:
|
||||
pass
|
||||
|
||||
return status
|
||||
```
|
||||
|
||||
- [ ] **Step 2: Guard retrieve command**
|
||||
|
||||
In `retrieve.py`, before calling `retrieve_chunks()`:
|
||||
|
||||
```python
|
||||
status = get_embed_status(vault)
|
||||
if status["chunk_count"] == 0:
|
||||
result = PFResult(
|
||||
ok=False,
|
||||
command="retrieve",
|
||||
version=PF_VERSION,
|
||||
error=PFError(
|
||||
code=ErrorCode.PATH_NOT_FOUND,
|
||||
message="Vector index is empty. Run paperforge embed build first.",
|
||||
),
|
||||
data={"next_action": "paperforge embed build"},
|
||||
)
|
||||
if args.json:
|
||||
print(result.to_json())
|
||||
else:
|
||||
print(f"Error: {result.error.message}", file=sys.stderr)
|
||||
return 1
|
||||
```
|
||||
|
||||
- [ ] **Step 3: Commit**
|
||||
|
||||
```bash
|
||||
git add paperforge/commands/retrieve.py paperforge/worker/vector_db.py
|
||||
git commit -m "feat: guard retrieve against empty index, add embed status check"
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## Task 6: Local mode HF download hint
|
||||
|
||||
**Files:**
|
||||
- Modify: `paperforge/plugin/main.js` (_renderFeaturesTab)
|
||||
|
||||
**Goal:** Users on local mode know they need HF access and can set a mirror.
|
||||
|
||||
- [ ] **Step 1: Add HF download hint**
|
||||
|
||||
In `_renderFeaturesTab`, after the local model selector, add:
|
||||
|
||||
```javascript
|
||||
new Setting(containerEl)
|
||||
.setName('HF Download Notice')
|
||||
.setDesc(
|
||||
'Local mode downloads models from Hugging Face on first use. '
|
||||
+ 'If inaccessible, set HF Endpoint below (e.g. https://hf-mirror.com) '
|
||||
+ 'or switch to API mode.'
|
||||
)
|
||||
.setDisabled(true);
|
||||
```
|
||||
|
||||
(Use Obsidian Setting's `setDisabled` to make it a read-only notice, or just use an `info` div.)
|
||||
|
||||
- [ ] **Step 2: Commit**
|
||||
|
||||
```bash
|
||||
git add paperforge/plugin/main.js
|
||||
git commit -m "feat: add HF download notice for local mode in plugin settings"
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## Summary
|
||||
|
||||
| Task | Priority | Files | Risk |
|
||||
|------|----------|-------|------|
|
||||
| 1 — Remove ghost code | P0 | main.js | None — deletes orphan code |
|
||||
| 2 — Vector deps | P0 | pyproject.toml, main.js | Low — additive |
|
||||
| 3 — Preflight | P0 | vector_db.py, embed.py | Low — additive guard |
|
||||
| 4 — Feature gating | P1 | vector_db.py, pf_bootstrap.py | Low — already mostly done |
|
||||
| 5 — Retrieve guard | P1 | retrieve.py, vector_db.py | Low — additive guard |
|
||||
| 6 — HF hint | P2 | main.js | None — UI text only |
|
||||
|
|
@ -264,6 +264,7 @@ def build_parser() -> argparse.ArgumentParser:
|
|||
p_embed_build = p_embed_sp.add_parser("build", help="Build vector index from OCR fulltext")
|
||||
p_embed_build.add_argument("--json", action="store_true")
|
||||
p_embed_build.add_argument("--force", action="store_true")
|
||||
p_embed_build.add_argument("--resume", action="store_true", help="Skip papers already in vector index")
|
||||
p_embed_status = p_embed_sp.add_parser("status", help="Check vector DB status")
|
||||
p_embed_status.add_argument("--json", action="store_true")
|
||||
|
||||
|
|
|
|||
|
|
@ -10,6 +10,7 @@ from paperforge.memory.chunker import chunk_fulltext
|
|||
from paperforge.memory.vector_db import (
|
||||
delete_paper_vectors,
|
||||
embed_paper,
|
||||
get_collection,
|
||||
get_embed_status,
|
||||
get_vector_db_path,
|
||||
)
|
||||
|
|
|
|||
Loading…
Reference in a new issue