fix(skills): preserve package integrity across edits and imports

[ewen-poch]

Problem

SK01–SK08 reproduce data loss on ordinary Skill edits, untrusted installation identity, ineffective reimport repairs, false runtime-sync success, stale executable permissions, duplicate GitHub updates, and coupled selection of same-named uploads.

Proposed change

Report	Trigger and consequence	Minimum correction
SK01	Saving Chinese/space attachment names silently drops new files or deletes existing references.	Share portable basename validation; preserve supported names and reject unsafe new input before live mutation. Already-existing regular-file basenames from historical/published POSIX packages can be retained without replacement bytes, including CON.csv and trailing.; explicit removal still works.
SK02	A damaged imported copy retains its historical source signature, so reimport cannot repair it.	Compare the actual installed inventory/normalized content before returning unchanged; repair Agent Home, ZIP and GitHub copies under the existing ID.
SK03	Wrapped ordinary imports can install Specialist metadata and make existing IDs unavailable.	Reject app-owned metadata at the resolved Skill root on ordinary ingress; preserve legitimate historical installation metadata.
SK04	Removing a disabled app-owned runtime Skill fails but sync reports success.	Propagate deletion failure and retain tracking until removal succeeds; a later sync can retry.
SK05	chmod-only changes leave runtime scripts without the requested executable mode.	Include the executable property in compatibility and invalidate the rebuildable cache.
SK06	An old editor overwrites newer content and deletes subsequently added attachments.	Carry an optional opaque etag in the update request and compare under the mutation lock. The editor always sends it and offers review, keep editing, load latest, or an explicit conditional overwrite.
SK07	A new pinned GitHub revision becomes a second local Skill, separating existing references/settings.	Derive stable owner/repo/root identity independently of revision, update the original ID, and show update availability.
SK08	Two same-named uploads share selection identity and can both be submitted when one is selected.	Allocate an in-memory identity per file occurrence for ZIP and Markdown.

Scope and non-goals

All eight fixes address reproduced failures; no new architecture layer, dependency, database table/column, product enum, automatic merge system, history storage or persistent retry queue.

Interface: UpdateSkillRequest.etag is optional. Omitted etags retain legacy unconditional-update behavior; supplied invalid/stale etags fail before mutation. Detail responses return an opaque etag derived from the existing content fingerprint, with no stored revision counter. The editor always supplies the read-time etag. On conflict it preserves the draft and shows both versions and reference lists. Loading latest discards the draft only on explicit choice; overwrite rechecks the reviewed latest etag, so another intervening update requires another review. No force flag, automatic merge, or partial-update redesign. This follows the optional validator pattern in Google AIP-154 and explicit save-conflict choices in VS Code. ScannedSkillView.installedId is optional and transient. Repository consumers retain the stable facade.

Persistence/history: existing Skill files and .source.json are rewritten only through existing save/import transactions. Derived compatibility cache content advances from version 1 to 2 and stores an executable boolean; tree hashes advance from v2 to v3, rebuilding old caches and refreshing runtime projections. Authoritative Skill data needs no migration. Historical/published nonportable reference names remain unchanged on body edits: the exception requires an exact regular file in the symlink-free staging copy and a name-only request. New names and replacement uploads retain strict validation; directories and symbolic links do not qualify. Existing Specialist sidecars are never blindly stripped: ordinary replacement refuses such installations. One unambiguous historical GitHub source updates in place; multiple historical matches are refused without merging identities or rebinding relationships. Already lost files cannot be recovered by this change. Intentional same-root multi-ref coexistence is outside this minimum policy.

Acceptance criteria and validation

Regressions precede production fixes in e0a55ed5 and 24976d6. On unmodified baseline production, the expanded boundary suite ran twice: 32 expected failures and 53 passing controls, with identical failure names and report-consistent observations. Fixtures use real temporary files and public repository/catalog/component boundaries; only external GitHub transport and a targeted rm EBUSY are controlled. No production test seam was added.

The optional-etag and conflict-resolution adjustment was also tested before implementation: two runs each produced 15 failures and 39 passing controls, with identical failing names. This includes contract adaptation as well as observable unconditional-update, draft preservation, conflict review and conditional-overwrite cases. After implementation all 54 pass. Test names describe behavior without report-number prefixes.

Final local Test Impact Set: 34 test files, 2,021 passed, zero failed, after the last material source edit. It combines the declared user_skills_repository module with explicit interface/runtime consumers and registered-helper refresh coverage; only testFiles changed in its manifest, not ownership/routing. Typecheck and lint also passed after rebasing onto main at 47cfebd. Final head: 78e1cb3. Only translation insertion conflicts required resolution; a redundant French formatting commit was skipped. Semantic comparison confirms all latest-main locale entries and all final Skill changes are present, and the Skill implementation/tests are byte-identical to the pre-rebase state. The three-count difference from the prior 2,024 run comes from dynamically enumerated i18n guard cases on latest main, not removed Skill regression tests. No code changed after this final test run.

Behavior / risk	Project-owned checks (run via npm test -- <paths>)	Final result
SK01/02/03/07 filesystem ingress and identity	user-skill-integrity.regression.test.ts, repository/atomic tests, skill-catalog.test.ts	Passed
SK04/05 runtime removal and chmod	materializer-integrity.regression.test.ts, materializer.test.ts, compatibility-index/catalog-observer tests	Passed
SK06 competing/stale/optional etags and explicit conflict resolution	skill-catalog.test.ts, SkillEditLoader.render.test.tsx, SkillsPanel.render.test.tsx	Passed
SK07/08 actual selection/submission	SkillsPanel.render.test.tsx, SkillUploadView.render.test.tsx	Passed
Repository consumers / boundary contracts	Module's Notebook RPC, host/conversation import, Specialist package, Settings service/slice, preload tests; additional backend-resolver, agent-runtime-manager, integration-application-commands and settings-store tests	Passed
Cache history / stable facade / module declaration	compatibility-index and repository architecture tests; module-test-impact and validate-module-impact tests	Passed
All eight translated locales	i18n resources guard, editor/upload i18n render tests	Passed
Main/preload/renderer types and source lint	npm run typecheck, npm run lint	Passed

Exact-head test:affected --explain selects the full CI plan because the existing classifier treats the module manifest as a global gate input. Only its testFiles changed; per CONTRIBUTING, local validation uses the module plus explicit consumers and manifest tests. No complete local npm test run. The manifest's owner/contract tests and explicit consumers establish the local impact map; PR Gate remains authoritative for complete and cross-platform lanes. POSIX chmod assertions run on macOS/Linux and skip Windows; portable basename/path and real-filesystem cases remain in the Windows lane. Framework resolver/runtime consumers cover claude-code, opencode, codex-response and codex-bridge. No real model invocation of a leftover disabled Skill or Windows ACL behavior is claimed.

Electron/Playwright interaction checks using actual renderer components/styles and isolated API/store fixtures passed with no page errors. Local screenshots demonstrate distinct same-named selection, stale-save comparison with Chinese/space reference names, loading the latest version, and GitHub update availability. Four additional Electron interaction checks passed: both versions are visible; keep editing retains the draft; loading latest adopts its etag; explicit overwrite uses the reviewed etag. The component suite additionally covers another concurrent write during review and failed latest-version reads. This is component interaction verification, not full packaged-app E2E; screenshots/planning artifacts remain local as required.

The latest P2 review finding about historical reference names was reproduced on unmodified e79cf44 production twice: 4 identical failures, 78 passing controls. Both Settings detail/save and publishPersonalDirectory/updatePersonal paths failed with Unsafe Skill reference filename for CON.csv and trailing. The fix adds no test seam or public interface. Regression coverage verifies byte/name preservation, explicit deletion, rejection of replacement bytes, absent references, directories and symlinks. POSIX-only real-file cases skip Windows; portable rejection controls run everywhere. The final impact set above includes this follow-up.

Review focus

Please confirm that the final owner/contract/consumer evidence covers the changed interfaces and persistence boundaries, especially normalized imported content, legitimate Specialist metadata preservation, stale-write lock placement and explicit conflict choices, and GitHub revision-independent identity. The previous-head AI review identified the history regression addressed above. Independent review and exact-head CI for this follow-up are pending; local passing checks alone are not the final verification claim.

[github-actions]

Codex Review

Verdict: mergeable

No actionable findings.

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

[github-actions]

Codex Review

Verdict: needs changes

[P2] Editor rejects reference names accepted by host Skill publishing

src/main/skills/user-skill-store.ts:89

Impact: publishPersonalDirectory accepts arbitrary package files, including references/CON.csv or references/AUX.txt on POSIX systems, and the published Skill is then editable in Settings. Any save now throws before staging because prepareSkillWrite rejects that existing reference, so even an unchanged body cannot be saved; this also creates inconsistent cross-platform behavior.

Recommendation: Apply the same isSafeSkillReferenceName policy when validating/publishing personal package directories (or preserve already-installed names during editor saves), and surface the rejection before the package is published.

Summary: Static inspection found one cross-platform editor regression: the new reference-name rejection is stricter than the host Skill publish path, so published personal Skills with names such as CON.csv can no longer be saved in Settings. No prohibited validation commands were run.

[github-actions]

Codex Review

Verdict: needs changes

[P2] Rejecting legacy reference names makes existing skills unsaveable

src/main/skills/user-skill-store.ts:89

Impact: Personal skills created on POSIX systems before this change may contain previously accepted names such as CON.csv or trailing.. The editor sends these existing references unchanged on any later save, but prepareSkillWrite now rejects them, preventing users from editing otherwise valid skills.

Recommendation: Permit already-existing reference basenames during updates while applying the stricter validation to newly added files, or provide a migration/rename path before enforcing the new rule.

Summary: Static inspection found one backward-compatibility regression. No tests or project commands were run per instruction.

[github-actions]

Codex Review

Verdict: mergeable

No actionable findings.

Summary: Static inspection found no concrete merge-blocking defects in the pull request. Branch and title checks are valid; tests and other project commands were not run per instruction.

[github-actions]

Codex Review

Verdict: mergeable

No actionable findings.

Summary: Static inspection found no concrete merge-blocking defects introduced by this pull request.