Skip to content

feat(literature): add reference library and research workflows - #2236

Open
ewen-poch wants to merge 3 commits into
mainfrom
feat/literature-library
Open

feat(literature): add reference library and research workflows#2236
ewen-poch wants to merge 3 commits into
mainfrom
feat/literature-library

Conversation

@ewen-poch

@ewen-poch ewen-poch commented Sep 6, 2026

Copy link
Copy Markdown
Member

Problem

References, downloaded PDFs, citations, and agent reading context need a shared library workflow with explicit review before importing agent suggestions or merging duplicates.

Proposed change

  • Add a literature catalog with Collections, project links, Inbox acceptance, Trash, identifier-aware imports, and citation formatting with artifact provenance.
  • Add duplicate comparison and bulk merge strategies that preserve attachments and associations, validate review snapshots, and resolve metadata conflicts explicitly.
  • Find and attach full-text PDFs through Europe PMC, PMC, OpenAlex, and Unpaywall, with inline credential configuration, source links, download progress, and agent retrieval into Inbox.
  • Support up to three PDFs in a reading session and batch metadata/full-text operations that continue in the background when their dialog closes. Persist checkpoints and require explicit resume after application restart.
  • Align library controls, tooltips, pagination, responsive layouts, and task feedback; serialize content publication and cleanup to protect shared PDF blobs.

Scope and non-goals

This is the integrated Literature library feature, organized into three commits for core services, UI/research integration, and resilient batch/merge workflows, including accepted PR review corrections. Internal planning documents are excluded from both the PR diff and its commit history. It does not implement a systematic-review screening product or change ROADMAP.

Acceptance criteria and validation

  • On 06ec5566, all application CI checks passed: CodeQL, static checks, all Ubuntu full-test shards, Windows core tests and both Windows E2E shards, macOS build/E2E, Linux isolation, and the PR Gate aggregate.
  • Project deletion now retries cleanup using registered content blobs after provenance rows have been removed. An exact storage-root check prevents SQLite LIKE wildcard matching from sweeping another project's orphan. All 104 provenance/deletion/storage regressions passed.
  • Library query scopes remain per-call filters; PDF Reading independently enforces the current message's document selection. Relevant Library MCP, Reading context, and runtime composition tests passed (39 tests).
  • Local verification also passed the complete Electron workspace lane (31 tests), three repeats of conversation revision recovery after restart, library UI/navigation regressions (143 tests), and translation guards (805 tests).
  • Typechecks, source lint, generated database schema and web API map checks, and git diff --check passed. The macOS Provenance visual baseline reflects the approved side-by-side layout; current macOS visual CI passed.
  • External provider downloads were not revalidated locally. AI review is tracked separately; the latest attempt timed out without a final verdict and has been retried.

Review focus

Review transactional merge validation and attachment/link preservation, migration-ledger adoption, PDF publication/cleanup ordering, Inbox acceptance, and checkpoint/draft persistence across background task transitions.

@github-actions github-actions Bot added the enhancement New feature or request label Sep 6, 2026
@github-actions

github-actions Bot commented Sep 6, 2026

Copy link
Copy Markdown

Codex Review

Verdict: needs changes

[P1] Keep the lockfile aligned with direct runtime dependencies

package-lock.json:27

Impact: The root lockfile omits the direct diff and parse5 dependencies declared in package.json. Clean npm ci installs will reject the lockfile as out of sync, and diff currently resolves to an unrelated transitive 8.0.4 package instead of the required 9.0.0 runtime dependency.

Recommendation: Regenerate or correct package-lock.json so it includes diff@9.0.0 and parse5@^7.3.0 in the root dependency map with matching package and integrity metadata.

Summary: Static review found a lockfile inconsistency that can break clean reproducible installs.

@ewen-poch

Copy link
Copy Markdown
Member Author

Fixed in 66c9fad. Restored main's direct diff@9.0.0 and parse5@^7.3.0 lock entries, including their resolved URLs and integrity metadata, and restored the nested shadcn dependency on diff@8.0.4. The only lockfile additions relative to the rebase baseline are now citeme-engine-wasm@0.3.8 and its root dependency entry.

Validation: npm ci --dry-run --ignore-scripts --no-audit --no-fund and git diff --check both passed. This also addresses the dependency-install failure shared by the initial PR Gate jobs; it does not change application behavior or dependency versions declared in package.json.

Comment thread src/main/literature/full-text-finder.test.ts Fixed
Comment thread src/main/literature/full-text-finder.test.ts Fixed
Comment thread src/main/literature/full-text-finder.test.ts Fixed
@github-actions

github-actions Bot commented Sep 6, 2026

Copy link
Copy Markdown

Codex Review

Verdict: needs changes

[P1] Preserve literature artifact mentions during composer DOM parsing

src/renderer/src/pages/workspace/composer/composer-doc.ts:394

Impact: Literature artifact chips are emitted but rejected during DOM-to-document synchronization, so editing or rehydrating a message can silently drop the literature PDF reference.

Recommendation: Allow source === 'literature' in this guard, or consistently use the dedicated literature node type.

[P2] Allow corrupted content blobs to be repaired

src/main/storage/content-repository.ts:173

Impact: If the destination already exists with corrupted bytes, the new copy is ignored, the blob is quarantined, and repeated publication of the same source fails indefinitely.

Recommendation: Verify an existing destination on EEXIST and replace or quarantine it when its checksum does not match.

[P2] Enforce unique root collection names

src/main/database/migrations/0029-literature-foundation.ts:118

Impact: SQLite allows multiple NULL values in the composite unique index, so duplicate root collections with the same normalized name can be created despite the declared sibling-uniqueness constraint.

Recommendation: Use a root-specific partial unique index plus a child index, or a non-null root sentinel, and enforce the invariant in the write path.

Summary: Three concrete regressions were found in the literature/storage integration.

@ewen-poch

Copy link
Copy Markdown
Member Author

Fixed the PDF mention and corrupt-content findings in 3546231:

  • Composer DOM parsing now accepts the existing literature artifact source. The regression test restores a PDF mention from message parts, edits the DOM, and verifies the reference and managed-file identity survive serialization.
  • Content publication checks a pre-existing destination after EEXIST. If its size or checksum differs, the verified temporary copy atomically replaces the corrupt file while holding the existing content lifecycle lock. The content ID is retained. Regression coverage includes size and checksum corruption, successful republication, and a single remaining blob record.

Validation: all 61 tests across the two owning suites passed; full typecheck, targeted ESLint, and git diff --check passed.

The root Collection uniqueness recommendation is pending owner confirmation. Enforcing it would change a database constraint and requires an explicit policy for existing same-name root Collections, plus migration work. No collection behavior, schema, or migration has been changed for that finding.

@github-actions

github-actions Bot commented Sep 6, 2026

Copy link
Copy Markdown

Codex Review

Verdict: needs changes

[P1] Library navigation bypasses dirty-preview protection

src/renderer/src/stores/navigation-store.ts:235

Impact: openLibrary, and the adjacent openLiteratureItem action, navigate directly without invoking requestPreviewLeaveForNavigation. Activating literature actions from a workspace can therefore unmount a dirty PDF/file preview and silently discard unsaved edits.

Recommendation: Route these navigation actions through requestPreviewLeaveForNavigation, matching goHome, openProjectLiterature, and openCollectionLiterature.

[P2] Literature metadata requests bypass the configured network stack

src/main/ipc.ts:1546

Impact: Production construction of LiteratureMetadataEnricher leaves its fetch function at global fetch. In environments requiring the system/VPN proxy, Crossref and PubMed enrichment uses Node's direct network path and can fail even though the application provides netFetchStandard for proxy-aware main-process requests.

Recommendation: Inject netFetchStandard when constructing LiteratureMetadataEnricher, while retaining dependency injection for tests.

[P2] Reference resolution also bypasses proxy-aware fetching

src/main/acp/runtime-composition.ts:258

Impact: The production LiteratureReferenceResolver is constructed with its default global fetch, so DOI/PMID resolution for the save-references workflow similarly ignores configured system/VPN proxy settings and can fail in supported proxied environments.

Recommendation: Construct LiteratureReferenceResolver with netFetchStandard (or change its production default to that helper) and keep an injectable fetcher for tests.

Summary: Branch and pull-request title prechecks passed. Static inspection found two merge-blocking integration defects; no tests, lint, typecheck, build, or project commands were run.

Add the Literature schema, content storage, metadata and full-text discovery, duplicate handling, citation finalization, and scoped MCP integration. Preserve attachment and reading-source identity across artifact versions.
Add reference import, metadata and PDF review, duplicate comparison and batch operations, citation styles, and Collection mentions. Integrate Reading and Artifact Literature provenance, improve navigation and nested dialogs, and translate the renderer in eight locales.
@ewen-poch
ewen-poch force-pushed the feat/literature-library branch from 3546231 to db507f5 Compare September 6, 2026 05:05
@ewen-poch

Copy link
Copy Markdown
Member Author

Addressed the actionable findings and CI failures:

  • Library navigation and direct reference navigation now use the existing dirty-preview leave guard. Regression tests cover cancellation and successful navigation.
  • Metadata enrichment and agent reference resolution now receive the existing Electron network adapter, so production requests honor configured proxy behavior.
  • Collection names are unique among siblings, including top-level Collections. Create and rename conflicts preserve the editor and explain the problem. Deleting a parent is rejected atomically if child promotion would create a top-level name conflict; references and associations remain intact. This behavior was approved by the owner. No historical auto-renaming was added.
  • The E2E fake agent now selects the Reviewer MCP by its explicit framework-compatible name, rather than selecting the first HTTP server (which could be Literature). The full local Electron workspace lane passed: 31 tests.
  • Library tests now wait for debounced filters and asynchronous citation-style loading before asserting dependent state. Test URL matching and entity decoding were also corrected for the CodeQL findings.

Targeted catalog, metadata, resolver, and translation tests passed (851 tests); library/navigation tests passed (143 tests); ACP/network tests passed (6 tests). Migration/ledger tests, typechecks, source lint, generated schema, and web API map checks passed.

The branch is consolidated into three commits. Internal planning documents have been removed from the entire PR commit history and remain local only.

@github-actions

github-actions Bot commented Sep 6, 2026

Copy link
Copy Markdown

Codex Review

Verdict: needs changes

[P2] ContentBlob records survive project deletion

prisma/schema.prisma:416

Impact: Deleting a project removes its artifact/upload rows and files, but ContentBlob has no project ownership or cascade cleanup. This leaves available blob records pointing to missing storage, leaking database authority rows until a later startup sweep.

Recommendation: Update project deletion to collect and remove unreferenced ContentBlob records after deleting domain references, using the existing reference-aware sweep so shared blobs are preserved.

Summary: Branch and title prechecks passed. Found one project-deletion lifecycle defect.

@ewen-poch

Copy link
Copy Markdown
Member Author

The current head passed all three Ubuntu full-test shards, Static checks, Windows core, and Windows E2E shard 2. The macOS build, functional/workspace E2E, accessibility, and native tests also passed.

The remaining CI issues are:

  • One CodeQL annotation in the glossary test prose extractor. The local correction uses segmented prose extraction and passed all 805 translation guards plus ESLint; it is awaiting the next bundled push.
  • Windows E2E shard 1 reported a flaky restart: graceful Electron shutdown exceeded 10 seconds on the first attempt and passed on retry. The previous Reviewer MCP initialization defect no longer appears.
  • The Provenance visual snapshot expects an overlay, while the implementation now uses the owner-approved wide-window side-by-side layout. The expected image needs review and refresh.

The new ContentBlob project-deletion finding is valid: version authority is removed without immediate cleanup of corresponding unreferenced blob rows. Startup recovery currently sweeps aged orphans, but integrating immediate cleanup must preserve shared references and deletion retry semantics. Owner confirmation is pending for this lifecycle adjustment and the additional E2E fixture/snapshot files, as requested by the PR author. No changes to those paths or lifecycle behavior have been made for this finding yet.

@ewen-poch
ewen-poch force-pushed the feat/literature-library branch from db507f5 to 3fab74d Compare September 6, 2026 05:32
@ewen-poch

Copy link
Copy Markdown
Member Author

Implemented the approved project-deletion cleanup and remaining CI corrections:

  • Project provenance deletion now runs the existing reference-aware content sweep after removing domain references. Candidate selection is limited to the deleted project's exact artifact/upload storage roots, including trailing separators. Those registered keys remain discoverable if cleanup fails after Version rows disappear, so the existing project deletion intent can retry without new persistence. Regression coverage checks immediate removal, unrelated project/blob preservation, and a failed sweep followed by successful retry.
  • The restart E2E now waits for the final history switch to become idle before requesting application shutdown, using the existing branch control and active-session checks. No shutdown timeout or product behavior was relaxed. The scenario passed three consecutive local runs; Windows CI will verify the platform-specific result.
  • Updated the inspected Provenance screenshot to the approved wide-window preview/Provenance layout. No other visual baselines were changed. The local broad visual journey stopped earlier at a dark-conversation raster/layout difference; that unchanged snapshot passed the preceding macOS CI run.
  • The last CodeQL glossary-extraction finding is corrected using segmented prose extraction. All 805 translation guards passed.

Validation: 104 tests across provenance, project deletion, and content repository suites passed, as did typecheck, source lint, and diff checks. The branch remains three commits with internal planning documents excluded.

@github-actions

github-actions Bot commented Sep 6, 2026

Copy link
Copy Markdown

Codex Review

Verdict: needs changes

[P1] Enforce exact-item scope for literature reads

src/main/acp/runtime-composition.ts:205

Impact: When a caller requests scope: "items", isLiteratureItemInScope falls through to return true, so read_library_abstract and read_library_pdf can read any known literature item instead of only the explicitly selected items. This bypasses the intended selection boundary, including for PDF content.

Recommendation: Carry the originating itemIds or an equivalent selection token into each read request and require the requested item ID to be present; alternatively reject scope: "items" for read tools until that authorization data is available.

Summary: The new exact-item literature scope is not enforced for abstract/PDF reads. Branch and pull-request naming prechecks are valid.

@ewen-poch
ewen-poch force-pushed the feat/literature-library branch from 3fab74d to 06ec556 Compare September 6, 2026 05:56
@ewen-poch

Copy link
Copy Markdown
Member Author

After tracing both read paths, I am not adopting a previous-search authorization token for the items finding:

  • Library MCP scope is a per-call query filter, not a capability minted by search_library. The authenticated Library tools intentionally support explicit scope: library access to the user's catalog. No prior-search allowlist exists for any scope.
  • In an item read, itemId (or the abstract batch's itemIds) explicitly identifies the current request's selected targets. Reusing scope: items does not grant access that scope: library otherwise denies. Project and Collection filters continue to check membership.
  • Multi-PDF Reading uses a separate tool path: LiteratureDocumentReader.readCurrent loads the current user message's persisted PDF context and rejects document IDs outside that snapshot. The Library query filter does not bypass that path.

Added a code comment to make the existing distinction explicit, and replaced the unconditional fallback with explicit handling of the two ungrouped scopes. Introducing a new selection token would change the product's authorization model without closing the claimed escalation, because explicit global Library access is intentional. Library MCP, Reading context, and runtime composition tests passed (39 tests).

Separately, an internal boundary test exposed SQLite LIKE semantics in the new project cleanup query: underscores act as wildcards and matching is case-insensitive. Candidate IDs now receive an exact storage-root string check before entering the reference-aware sweep. A regression verifies that deleting project_1 preserves an unrelated orphan under project-1. All 104 provenance/deletion/content tests and Node typecheck passed.

On the preceding head, all CI checks passed, including CodeQL, every Ubuntu full-test shard, both Windows E2E shards, and macOS build/E2E. The next push retains the three-commit history.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants