fix(specialist): preserve package recovery and approved capabilities

[ewen-poch]

Problem

Six reproduced package defects can delete an untouched existing Skill after interrupted preparation (K01), overwrite local edits during reuse (K05), remove Main isolation and provenance after a committed install encounters cleanup failure (K02), silently change preview-approved capabilities (K03), keep a valid export selection blocked (K04), or erase damaged Marketplace recovery obligations during unrelated writes (K06).

Proposed change

• Record commit intent before Skill swaps and prove ownership/content before removing a promoted directory; preserve ambiguous legacy recovery evidence.
• Compare current Skill contents/version, preserve content during ownership-only reuse, and retain the existing mutation-lock check. Skip unreadable catalog entries while preserving metadata-located occupancy so another install cannot overwrite their files.
• Reconcile uncertain Marketplace install outcomes through real package recovery and exact archive identity. Keep committed cleanup retryable and compensate preferences only for confirmed rollback.
• Reject changed resolved capability selections through the existing stale-candidate result.
• Revalidate export previews for the current checkbox selection and ignore stale responses. Export the version captured from SKILL.md; block an invalid declared version instead of producing a ZIP whose bundled Skill is silently skipped on import. Exclusion remains available; absent versions keep the existing metadata/default fallback.
• Reject lossy reads/writes of authoritative Marketplace records, preserve original bytes, and display a translated repair message.

Scope and non-goals

No new service architecture, database/schema migration, lifecycle enum, repair wizard, dependency, or background worker.

Persistence: Skill transaction journals now write version 2 with per-location hadLive/contentHash; the existing Skill sidecar gains an optional transactionId ownership stamp. Existing Marketplace pending/provenance fields and preferences remain the source of truth. The export request's optional includedSkillIds and UI checking/integrity flags are transient.

Historical compatibility: Healthy existing Skill metadata and Marketplace v1 documents remain readable. Legacy transactions with provable backups can recover; ambiguous live directories/journals are preserved with an error. Invalid pending records (including those lacking exact archive identity), unknown versions, and unknown authoritative record fields block writes instead of being discarded. No automatic historical reconstruction or downgrade repair.

Acceptance criteria and validation

The six original reports were reproduced through real temporary repositories and public package/repository/UI boundaries before fixing them. K01 interrupts a child process at filesystem boundaries; K02 uses the existing SkillPort boundary. No production test seam was added. Additional public regressions cover unreadable metadata-located Skills, current edited export versions, and invalid-version exports.

For the final invalid-version guard, three permanent regressions failed twice against unchanged production code (2, empty, next): preview incorrectly allowed export and direct export succeeded. They pass after the fix, together with exclusion and absent-version compatibility checks.

Checks after the final source edit:

Behavior / impact	Command	Result
Skill owner and representative consumers, including export adapter and legacy fallback	npm run test:module -- user_skills_repository	23 files / 917 passed
Package recovery/approval/export, IPC, Settings export UI and store	npm test -- src/main/specialist/package src/main/specialist/ipc.test.ts src/renderer/src/pages/settings/SpecialistsPanel.render.test.tsx src/renderer/src/stores/specialist-store.test.ts	14 files / 267 passed
Main-process types and source lint	npm run typecheck:node; npm run lint; git diff --check	passed
Real export interaction	npm run build:e2e; local Playwright test using the repository ElectronAppHarness	passed: version 2 blocks, exclusion enables, correction to 2.0.0 enables; screenshots inspected

Rebased onto main 5cdde8fa, preserving both sides of adjacent locale additions. Before the final guard, broader owner/contract/consumer/i18n validation passed on b87327b5 (23 files / 1,152 tests), with full typecheck and lint; PR Gate, portable shards, Windows core/E2E and macOS build/E2E also passed on that head. Those broader Marketplace/renderer/contract implementations are unchanged by the final guard. The new guard changes only the existing adapter and its two existing test files.

The impact map is package export -> SkillPort snapshot -> shared SemVer validator, with preview/export IPC -> store -> Settings as consumers. Earlier changes also cover Marketplace -> package recovery/repository/preferences. No agent execution, session subscription or database behavior changes. Full local npm test was intentionally not run per maintainer instruction. Final-head PR Gate remains authoritative for complete/platform checks; local process-interruption tests do not prove power-loss durability or Windows/Linux behavior. Local plans, screenshot fixtures and evidence are excluded from the PR.

Review focus

Review rollback ownership and repeated recovery, healthy historical-format support versus fail-closed ambiguity, committed-cleanup provenance retention, and export selection/version consistency. Confirm that the behavior-to-check mapping covers the final diff. AI review of final head d4fe7441 completed with mergeable / no actionable findings: #2226 (comment). Final-head PR Gate passed on d4fe7441 after the isolated Windows E2E shard rerun; the original startup timeout and flaky-test gate were preserved. The initial failure occurred during fresh database startup before the test body; three local repetitions passed, and the Windows rerun passed all 11 functional and 16 workspace tests. No code or workflow changes were required.

[github-actions]

Codex Review

Verdict: needs changes

[P1] Do not abort the package catalog on an unreadable metadata-backed Skill

src/main/skills/specialist-package-adapter.ts:263

Impact: A directory with a valid .specialist-package.json but a missing, deleted, or unreadable SKILL.md now throws from snapshot(). Since the main package catalog calls this snapshot, one damaged Skill can block Specialist package preview, install, export, and Marketplace workflows; the owning UserSkillRepository explicitly skips such directories.

Recommendation: Handle SKILL.md read/hash failures per directory and omit the invalid entry, or perform the read under the shared Skill mutation lock with the same skip-on-unreadable behavior as UserSkillRepository.list().

Summary: Static inspection found one blocking regression in Skill catalog snapshotting.

[github-actions]

Codex Review

Verdict: needs changes

[P2] Use the live Skill version when exporting

src/main/skills/specialist-package-adapter.ts:267

Impact: snapshot() now reports the SKILL.md frontmatter version, but exportSnapshot() still uses the stale .specialist-package.json version. Editing a Specialist-owned Skill’s version and exporting it therefore produces a ZIP with an older version than the preview/catalog shows.

Recommendation: Parse SKILL.md in exportSnapshot() and use its trimmed frontmatter version, falling back to sidecar metadata only when absent, so preview and exported package versions stay consistent.

Summary: Exported Specialist packages can silently downgrade an edited Skill’s version.

[github-actions]

Codex Review

Verdict: needs changes

[P1] Unreadable metadata-backed Skills can be overwritten as new installs

src/main/skills/specialist-package-adapter.ts:282

Impact: When a metadata-backed Skill has a missing or invalid SKILL.md, snapshot() now omits it. Validation can therefore classify an incoming same-named Skill as a new install, while prepare() still locates the old directory via its sidecar and commit() replaces it, discarding the orphaned Skill contents and ownership state.

Recommendation: Treat metadata-located but unreadable directories as occupied and refuse the install, or otherwise preserve them; make beginMutation/prepare use the same validated occupancy view instead of allowing findSkillDirectory() to select them as install targets.

Summary: The new unreadable-Skill handling can allow a metadata-backed Skill directory to be overwritten during a later install, causing user data loss.

[ewen-poch]

Addressed the unreadable-Skill occupancy finding. The existing destination check already protects the normal directory, but the same Skill can still be located by its sidecar after its directory is renamed. A package-service regression reproduced the reported data loss in that case: installation succeeded and removed the retained local notes.

beginMutation() now checks the same identity lookup used by prepare() while holding the existing mutation lock. An unreadable occupied Skill is refused instead of overwritten; unrelated package previews and exports remain available. The change adds no state, persisted fields, public API, or UI behavior, and stays within the adapter and the existing regression-test file.

Rebased onto main 5cdde8fa. The eight locale conflicts were adjacent additions; both main's Plan translations and this PR's Marketplace repair translations were preserved.

Validation after the change: 23 focused test files / 1,152 tests passed, including package/Marketplace, IPC, renderer/store consumers, translation guards, and the shared catalog contract. The Skill owner module, typechecks, lint, and diff checks also passed. The new regression failed on the pre-fix code with the reported file loss and passes with the occupancy check.

[github-actions]

Codex Review

Verdict: needs changes

[P2] Do not export invalid frontmatter versions as authoritative

src/main/skills/specialist-package-adapter.ts:360

Impact: A Skill with frontmatter such as version: 2 or an empty/invalid version is exported with that value. Package validation only warns and skips the bundled Skill plan, so the resulting ZIP can silently omit the selected Skill on import.

Recommendation: Use the current document version only when it is non-empty and valid SemVer; otherwise fall back to the stored metadata/default, or make invalid versions a blocking export error.

Summary: Static inspection found one export-integrity regression; tests and project commands were not run per instruction.

[ewen-poch]

Confirmed the invalid-frontmatter-version finding through the public package workflow. For version: 2, an empty version, and version: next, preview reports canExport: true and export succeeds, but validating the exported ZIP produces no bundled Skill plan and reports skill.version-invalid / specialist.skill-unavailable.

This needs an explicit behavior decision. Falling back to stored metadata/default would normalize the exported version away from the document; rejecting export would require the user to correct the version before exporting that selection. I recommend rejecting an explicitly invalid version, while retaining the existing fallback when the version is absent. This avoids silently rewriting version intent or producing a package that drops the selected Skill.

No further code change has been made for this finding. The maintainer requested pausing before changes to the agreed behavior or user interaction, so implementation is awaiting that decision. The reproduction was run against the current head; temporary test instrumentation was removed afterward and the worktree is clean.

[ewen-poch]

Implemented in d4fe744: explicitly invalid declared Skill versions now block both export preview and direct export. The existing SemVer validator runs against the captured SKILL.md version before ZIP creation; valid edited versions remain authoritative. Missing versions retain the existing sidecar/default fallback. Excluding the invalid Skill restores export through the existing selection flow.

Before the fix, public install -> edit -> export -> reimport reproduced silent omission for 2, an empty string, and next. The permanent blocking-policy regressions failed twice on unchanged production code: preview still returned canExport: true and direct export resolved. All three pass after the guard. Existing adapter coverage also confirms both absent-version fallbacks.

After the final source edit: affected Skill module 23 files / 917 tests passed; package, IPC, Settings renderer and store consumers 14 files / 267 tests passed; node typecheck, lint and Electron build passed. An isolated native Electron test verified blocked export with version 2, enabled export after exclusion, and enabled export after correcting the file to 2.0.0; screenshots were inspected. No new persisted fields, enum values, dependencies, or architecture changes. The existing preview diagnostic is reused.

[github-actions]

Codex Review

Verdict: mergeable

No actionable findings.

Summary: Static inspection found no concrete merge-blocking defect in the pull request changes.

[ewen-poch]

Investigated the Windows E2E startup timeout on d4fe7441. The failure occurred in the initial waitForRendererReady fixture before the project-rename test body ran. The captured main-process log was still applying the fresh database migrations and reported slow-disk-or-scanner with a timed-out storage probe; it does not identify the exact cause of the runner I/O delay. The test passed its automatic retry, but --fail-on-flaky-tests correctly failed the job.

The same public launch -> rename -> relaunch test passed three local repetitions with retries disabled. Reran only the failed Windows shard: all 11 functional and 16 workspace tests passed, with the original 60-second startup limit and flaky-test gate intact. PR Gate is now successful: https://github.com/aipoch/open-science/actions/runs/34007532431/attempts/2. No source, test, schema, or workflow changes were made. The reviewed head remains d4fe7441, whose AI review verdict is mergeable.