Route getDraft and saveDraft through an async repository (#431)

[imnasnainaec]

Part of #424; closes #431.

Summary

getDraft and saveDraft now go through an async repository. Its implementation imports nothing from PAPI, so Node runs it too. The perf harness uses that to seed its tiers without PAPI.

 interlinearizer.getDraft / saveDraft
   projectStorage.getDraft / saveDraft
-    shard, journal and guard logic over papi.storage
+    DraftRepository.exportAll / replaceAll
+      KeyValueStorage
+        papi.storage (extension host) | storage files (Node)

 seedTier (perf harness)
-  sendCommand: saveDraft, createProject, saveAnalysis
+  writeDraft       -> DraftRepository over the storage files
+  addSavedProject  -> project record + projectIds index

src/types/analysis-repository.ts   # the seam: AnalysisRepository, KeyValueStorage
src/services/draftRepository.ts    # drafts over envelope + per-book shards
src/utils/analysis-violations.ts   # storage-boundary violation report, shared
perf/storage.ts                    # Node: file storage, read/write drafts, add saved projects

• Contracts kept. getDraft and saveDraft make the same storage calls as before. The existing unit suite passes unedited.
• Guards kept. Only changed shards are rewritten. The shard journal still recovers interrupted saves. A newer build's draft is neither read nor overwritten.
• Memory unchanged. The repository keeps only the per-book record of what storage holds, as projectStorage did.
• Seeding.
  • A packaged perf run seeds each tier with the app stopped, inside the restart before its first open.
  • A dev-core run seeds with the app running. Every view close first waits for any autosave an edit or Save started.
• perf:shape reads stored drafts through the repository. It fails on a missing draft rather than measuring zeros.

Evidence

• Unit suite: passes at 100% coverage. projectStorage.test.ts passes unedited.
• A newer build's draft survives a write:
  • Before: covered only through projectStorage's PAPI mocks.
  • After: also at the seam:

    store draft with modelVersion CURRENT + 1
    exportAll()          -> rejects "newer than this build supports"
    replaceAll(draft)    -> rejects; no writes; stored record unchanged
    

• Seeding without PAPI:
  • Before: each tier went through saveDraft, createProject and saveAnalysis over PAPI.
  • After: written from Node and read back through the repository:

    tier            identical  shards in storage
    large           true       80
    large-loadable  true       80
    medium          true        1
    small           true        1
    

    The tiers were seeded in that order into one directory. Each switch deleted the previous tier's shards.
• Not run: the e2e suites and perf:app. Both need a running Platform.Bible. Playwright loads the perf spec.

Deviations from the issue and its comment

From the issue

• Saved projects aren't seeded "through it".
  • Only drafts route through the repository in 1b. The seeder writes saved-project records and the projectIds index directly.
  • [1e] Store project summaries for the picker #434 (1e) takes this over (see its comment).
• No records held in memory. "In-memory implementation" here means the per-book record of what storage holds, not the parsed draft.
  • Holding the draft would add host heap with no caller until Phase 3.
  • [3a] Add host sessions #440's (3a) sessions own that lifetime.
• e2e not run (see Evidence).

From the comment

• Whole-draft calls only. The interface has exportAll(): DraftProject and replaceAll(draft).
  • The rest of the interface is settled in [3a] Add host sessions #440 (3a): reads by id per kind, ChangeSet, ApplyResult, intent, capabilities, scope, and the naming that lookups and the catalog follow. 3a may defer parts further.
  • It is built where the plan puts the work: apply and undo in 3b, lookups in 3c, the catalog and countUsages in 5a.
  • Not added: pullRemoteChanges and close. Nothing in the plan needs them yet.
• getScope folded into exportAll. Separate calls would mean either holding the draft between them or reading storage twice.
• No ScopeRecord. The scope is the draft's own envelope. A type pairing a draft with its interlinear project is part of what 3a settles.
• Kept from the comment: async calls, JSON-plain values only, and optional fields omitted rather than set to null.

Decisions with a strong alternative

Reviewer push-back is welcome.

• Whole-draft calls only
  • Alternative: add the per-kind reads by id now, as the comment proposes.
  • Why not: nothing calls them before Phase 3, and [3a] Add host sessions #440 now settles the whole interface in one place, with its first callers.
• No records held between calls
  • Alternative: hold the parsed draft, as a first step toward sessions.
  • Why not: it grows host heap with no lifecycle to release it. That lifecycle is [3a] Add host sessions #440's job.
• Repository kept for the life of the process, not released on view close
  • Alternative: release it on view close, as BookService does with its books.
  • Why not: a view's unmount flushes its pending autosave. If the close handler wins the race, that save hits a fresh repository and rewrites every shard.
• Drafts-only scope
  • Alternative: a generic AnalysisRepository<Scope>, ready for saved projects.
  • Why not: it would have one use until saved projects route through the repository.
• Seeder writes project records directly
  • Alternative: split projectStorage into a Node-safe core now.
  • Why not: 1b must keep the existing suites unchanged, and [1e] Store project summaries for the picker #434 rewrites every project write anyway.
• Dev-core seeding with the app running
  • Alternative: restart the dev core for every tier.
  • Why not: minutes per tier. The autosave wait closes the one known race.
• Seam types in src/types/
  • Alternative: src/services/, beside the implementation, with a coverage exclusion.
  • Why not: model types and their small helpers live in src/types/ by convention.

Automated review

Devin's review raised two items. Given the rebuttals, Devin agreed that neither needs a change.

• A stale token after reactivation. draftRepositories would keep a repository bound to an earlier activation's token.
  • Rebuttal: core deletes the extension's module from the require cache on deactivation, and activation loads it again, so the map starts empty each time. Clearing it in deactivate stays an option if a host ever keeps the module loaded.
• Perf cleanup writes a draft that wasn't there. Restoring a missing draft saves an empty one into the packaged profile.
  • Rebuttal: main does the same through PAPI getDraft and saveDraft. Measurements don't change, since every run seeds a draft before measuring. Telling absent from empty would need a delete outside the repository, or one the plan doesn't include.

Comments on other issues

• 1e: [1e] Store project summaries for the picker #434 (comment)
• 3a: [3a] Add host sessions #440 (comment)
• 4a: [4a] Wire the WebView to the host behind a feature flag #453 (comment)
• 4c: [4c] Delete the WebView write path #455 (comment)

Merge Danger

Door: two-way

The stored format and storage calls are unchanged, so a revert restores main's behavior on the same data.

Blast Radius: drafts

Every draft load and autosave goes through the new code. The perf harness's seeding changes too.

🤖 Generated with Claude Code

---

This change is 

Summary by CodeRabbit

• Improvements
  • Draft saves update only book data that has changed, reducing unnecessary storage updates.
  • Drafts created by newer app versions are protected from being overwritten by older versions.
  • Draft data is better protected if a save is interrupted, and incomplete cleanup can be retried.
• Bug Fixes
  • Drafts with missing or unreadable stored data are handled more safely, helping preserve existing data during subsequent saves.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

• Configuration used: defaults
• Review profile: CHILL
• Plan: Advanced
• Run ID: ee4c20eb-4886-43c6-8594-5ad94dea3764

📥 Commits

Reviewing files that changed from the base of the PR and between 6f6481f and 170aa7a.

📒 Files selected for processing: 2

• e2e-tests/tests/perf/app-perf.spec.ts
• src/types/analysis-repository.ts

Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.

📝 Walkthrough

The pull request adds an asynchronous draft repository with per-book shards and routes project storage through it. Performance tools and end-to-end tests use shared storage helpers to read and seed extension data. The performance tests wait for tracked autosaves before closing views or reseeding.

Changes: Draft persistence and performance harness

Layer / File(s)	Summary
Draft repository contract and persistence src/types/analysis-repository.ts, src/types/type-guards.ts, src/services/draftRepository.ts, src/utils/analysis-violations.ts, src/__tests__/services/draftRepository.test.ts, src/__tests__/types/type-guards.test.ts, src/__tests__/utils/analysis-violations.test.ts	Defines asynchronous storage and repository interfaces. DraftRepository loads and replaces draft envelopes and per-book shards, journals shard changes, and avoids rewriting unchanged shard data. Tests cover loading, shard writes, newer model versions, invalid shard JSON, string-array validation, and analysis-violation reporting.
Project storage integration src/services/projectStorage.ts	Routes queued draft reads and writes through cached DraftRepository instances. Uses the shared analysis-violation reporter and clears the repository cache during test resets.
Performance storage utilities and loading perf/paths.ts, perf/storage.ts, perf/shape/cli.ts, AGENTS.md	Adds file-backed key-value storage and helpers for drafts and saved projects. The shape CLI loads draft analysis through the repository and uses the shared development storage path. Storage guidance documents the repository seam.
End-to-end performance test seeding e2e-tests/global-setup-perf.ts, e2e-tests/perf-app.ts, e2e-tests/tests/perf/app-perf.spec.ts, perf/README.md	Uses shared storage paths and seeds drafts and saved projects through storage helpers. The test waits for tracked autosaves before closing views or reseeding, and seeds packaged-app data while the app is stopped.

Priority: ⬇️ Low

Estimated code review effort: 4 (Complex) | ~45 minutes

Change: Refactor

Sequence Diagram(s)

sequenceDiagram
  participant AppPerf
  participant PackagedApp
  participant PerfStorage
  participant DraftRepository
  AppPerf->>PackagedApp: Close view after autosave
  AppPerf->>PackagedApp: Stop app for restart
  AppPerf->>PerfStorage: Seed draft and saved projects
  PerfStorage->>DraftRepository: Write draft through file storage
  AppPerf->>PackagedApp: Relaunch app

Loading

Suggested reviewers: alex-rawlings-yyc

Merge Risk: ⚪ Minimal · up to 170aa

No merge-blocking issue is established for these changes. The Save performance scenario’s autosave wait follows the Save path as intended.

Pre-merge checks: 3 · 2

Failed checks: 2 warnings

Check name	Status	Explanation	Resolution
Linked Issues check		#431 requires getDraft and saveDraft to use the async repository. The current code satisfies this for draft reads and writes. #431 also requires the Node harness seeding path to use that repository. `…	Route all seeding required by #431 through the repository, including saved-project data if it is part of the #407 seed. Run the required unchanged unit and e2e suites, then record the results.
Docstring Coverage		Docstring coverage is 77.55% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 49 functions across 14 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

Passed checks: 3 passed

Check name	Status	Explanation
Description Check		Check skipped - CodeRabbit’s high-level summary is enabled.
Title check		The title clearly and concisely describes the main change: routing getDraft and saveDraft through an asynchronous repository.
Out of Scope Changes check		The repository, storage adapter, draft tests, perf storage helpers, and perf seeding changes support #431's repository and PAPI-free harness objectives. No unrelated change is established by the avail…

Full details: Out of Scope Changes check

Explanation

The repository, storage adapter, draft tests, perf storage helpers, and perf seeding changes support #431's repository and PAPI-free harness objectives. No unrelated change is established by the available evidence.

Full details: Linked Issues check

Explanation

#431 requires getDraft and saveDraft to use the async repository. The current code satisfies this for draft reads and writes. #431 also requires the Node harness seeding path to use that repository. seedTier calls writeDraft, but addSavedProject writes the project record and projectIds index directly through fileStorage. The PR also states that the e2e suites were not run, so that done-when result is not established. The direct saved-project writes and the unverified e2e requirement remain unresolved; references to #434 do not change #431's requirements.

Resolution

Route all seeding required by #431 through the repository, including saved-project data if it is part of the #407 seed. Run the required unchanged unit and e2e suites, then record the results.

• Fix all pre-merge checks with AI

✨ Finishing Touches: 💡 1

📝 Generate docstrings: 💡

• Commit to this branch
• Create a new PR

🧪 Generate unit tests (beta)

• Commit to this branch
• Create a new PR

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[alex-rawlings-yyc]

The inline comments in this review were generated by Claude and haven't been closely human-reviewed, so treat them as leads to check rather than conclusions.

Checked against main and found matching: the storage calls and their order, the shard journal, the model-version guard, and the per-source queues.

[alex-rawlings-yyc]

Left a little nit that is not super-super important

@alex-rawlings-yyc reviewed 16 files and all commit messages, made 2 comments, and resolved 8 discussions.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on imnasnainaec).

---

src/types/analysis-repository.ts line 25 at r2 (raw file):

   * Loads the whole stored draft.
   *
   * @throws {SyntaxError} If the stored draft contains invalid JSON.

⛏️ Not all invalid JSON throws here. Invalid JSON in the envelope throws, but invalid JSON in a book's shard just loads that book as empty (parseAnalysisShard). This @throws makes it sound like either one throws. The implementation's wording, "the draft's stored value", is narrower and would fit here too.

[imnasnainaec]

@imnasnainaec reviewed all commit messages and resolved 1 discussion.
Reviewable status: 15 of 16 files reviewed, all discussions resolved (waiting on alex-rawlings-yyc).

[alex-rawlings-yyc]

@alex-rawlings-yyc reviewed 3 files and all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on imnasnainaec).