refactor(skills): collapse skill directory classification into one skill-directory-state module

[BlackHole1]

Summary

Architecture deepening (candidate 06 of the codebase review, following the identity-resolver series #310–#312): collapse the question "what state is this skill directory in" into one deep module.

Before this change the classification — stat the directory, read .oo-metadata.json, decide missing vs unmanaged vs managed — was re-derived inline at nine call sites through five divergent kind-filtered readers of the same file, while the correct implementation already existed privately as readManagedSkillTargetState in auto-sync.ts. One copy had drifted (update.ts runGroupUpdateJson skipped directoryExists entirely), and the registry "is it current" rule existed in three slightly different loops.

The new seam

src/application/commands/skills/skill-directory-state.ts:

readSkillDirectoryState(path): Promise<
    | { kind: "missing" }
    | { kind: "not-directory" }
    | { kind: "unmanaged"; metadataFilePresent: boolean }
    | { kind: "managed"; metadata: SkillMetadata; publicationCurrent: boolean }>

isCurrentRegistryPublication(state, { packageName, version }): boolean
isSkillDirectoryAbsent(state): boolean
managedMetadataOfKind(state, kind): metadata of that kind | undefined

Internal (hidden from callers): the stat/readFile/parse sequence, the symlink staleness check (publicationCurrent), the legacy untyped metadata fallback, and the error policy — IO errors other than not-found throw through, so commands keep their existing failure handling. Aggregation semantics (e.g. vacuous truth of "current in all hosts" over zero installations, and the JSON update loop's length > 0 guard) deliberately stay with each caller.

What collapsed into it

• The 9 inline classifications: uninstall (registry + bundled JSON loops), managed-skill-uninstall (bundled text path + a fused registry two-pass), update (text + JSON), check-update, repair, registry-skill-publication, install, share, plus registry-skill-install, managed-skill-listings, info-inventory, local-skill-source, publish, adopt, and both legacy cleanups.
• The 3 divergent currency loops → one isCurrentRegistryPublication predicate; the drifted runGroupUpdateJson copy now stats first like everyone else.
• The read halves of the metadata readers: readManagedSkillMetadata, readInstalledBundledSkillMetadata / isManagedBundledSkillInstallation, readSkillMetadataFileState / readLocalSkillMetadata / isForeignManagedMetadataState, the listings/info-inventory/legacy-cleanup inline reads, and managed-skill-publication.ts (absorbed as publicationCurrent). All write halves survive unchanged.
• Deletion-test failures folded: resolveBundledSkillInstallConflict and canUninstallManagedBundledSkillInstallation inlined at their call sites (each call hardcoded three of four inputs); bundled-skill-model.ts deleted (bundledSkillDevelopmentVersion moved to embedded-assets.ts); package-templates.ts deleted in favor of direct with { type: "text" } imports; writeSkillOperationJson and the skillOperationOutputOptions alias deleted in favor of writeJsonOutput / jsonOutputOptions.

The comment-enforced "remove local skill first" ordering in uninstall.ts is intentionally preserved (the registry classifier still treats managed-local directories as unmanaged); making the registry pass local-aware is now a one-line follow-up if ever wanted.

Behavior

No user-facing CLI contract change; docs/commands*.md and the telemetry manifest are untouched. One narrow edge class is deliberately unified: a regular file occupying a skill directory path (or a parent segment) is now classified (not-directory / missing) where a few old sites threw a raw ENOTDIR/EISDIR errno out of the command (e.g. it aborted the whole oo skills list). Remaining differences are TOCTOU-window narrowings and internal booleans that were provably unobservable.

This was verified adversarially: five independent reviewers derived old-vs-new outcomes per filesystem state (missing, broken symlink, file-at-path, no metadata, corrupt JSON, local/bundled/registry/legacy-untyped metadata, symlinked install, EACCES) for every migrated site — zero behavior-change bugs found; all 26 findings fall in the documented edge classes above.

Tests

• New table-driven skill-directory-state.test.ts (26 cases): all state classifications including legacy untyped metadata, broken/managed symlinks, parent-segment files, a deterministic non-ENOENT throw (EISDIR), and the currency predicate truth table.
• Coverage from deleted unit tests preserved: publication symlink cases absorbed by the module test; metadata parse/render assertions relocated to skill-metadata.test.ts; the conflict-predicate truth table replaced by the shared.ts error-path coverage that already exercises name/storage conflicts.
• bun run lint:fix / ts-check / knip clean; bun run test: 1674 pass, 0 fail.

Net: −135 lines, 4 files deleted, one module added.

[coderabbitai]

Summary by CodeRabbit

• Improvements

  • Improved skill installation, synchronization, updating, publishing, repair, sharing, and uninstall behavior across managed, local, bundled, and registry skills.
  • Added more consistent detection of missing, unmanaged, invalid, or stale skill directories.
  • Improved handling of conflicting or foreign skill metadata.

• Output

  • Standardized JSON output for install, sync, update, and uninstall commands.
  • Added clearer version tracking for managed skill operations.

Walkthrough

The PR introduces readSkillDirectoryState and typed helpers for classifying skill paths, parsing managed metadata, and checking current registry publications. Skill commands replace direct filesystem and metadata readers with these helpers across adoption, installation, publication, synchronization, update, repair, sharing, inventory, and uninstall flows. Legacy metadata readers and bundled-skill model helpers are removed. JSON output uses shared serialization utilities, and bundled asset version wiring moves to embedded-assets.ts.

Possibly related PRs

• oomol-lab/oo-cli#40 — Overlaps with the bundled-skill model and observation refactor.
• oomol-lab/oo-cli#111 — Directly relates to startup synchronization logic.
• oomol-lab/oo-cli#144 — Directly relates to shared JSON output in skills sync.

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title follows the required type(scope): subject format and accurately summarizes the refactor.
Description check	✅ Passed	The description is clearly related to the skill-directory-state refactor and the broader migration work.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

✨ Simplify code

• Create PR with simplified code
• Commit simplified code in branch refactor/skill-directory-state

---

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

[coderabbitai]

🧹 Nitpick comments (1)

src/application/commands/skills/managed-skill-uninstall.ts (1)

469-503: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win

Parallelize independent directory-state reads.

Each readSkillDirectoryState call here is independent per host installation, but the loop awaits them sequentially. As per coding guidelines, "Use Promise.all() for independent async operations instead of sequential await." This also matches how isEverythingCurrent in check-update.ts and the sync loops in auto-sync.ts already parallelize equivalent reads.

♻️ Proposed refactor

 async function resolveRegistrySkillUninstallTargets(
     hostInstallations: readonly ManagedSkillHostInstallation[],
 ): Promise<{
     targets: RegistrySkillUninstallTarget[];
     unmanagedInstallations: ManagedSkillHostInstallation[];
 }> {
+    const targetStates = await Promise.all(
+        hostInstallations.map(installation =>
+            readSkillDirectoryState(installation.installedSkillDirectoryPath),
+        ),
+    );
+
     const targets: RegistrySkillUninstallTarget[] = [];
     const unmanagedInstallations: ManagedSkillHostInstallation[] = [];

-    for (const installation of hostInstallations) {
-        const targetState = await readSkillDirectoryState(
-            installation.installedSkillDirectoryPath,
-        );
-
+    hostInstallations.forEach((installation, index) => {
+        const targetState = targetStates[index]!;
+
         if (isSkillDirectoryAbsent(targetState)) {
-            continue;
+            return;
         }

         if (
             targetState.kind === "managed"
             && targetState.metadata.kind === "registry"
         ) {
             targets.push({
                 ...installation,
                 packageName: targetState.metadata.packageName,
                 previousVersion: targetState.metadata.version,
             });
-            continue;
+            return;
         }

         unmanagedInstallations.push(installation);
-    }
+    });

     return { targets, unmanagedInstallations };
 }

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/application/commands/skills/managed-skill-uninstall.ts` around lines 469
- 503, Update resolveRegistrySkillUninstallTargets to read all installation
directory states concurrently with Promise.all, then process the resulting
installation/state pairs to populate targets and unmanagedInstallations while
preserving the existing absent, managed-registry, and unmanaged behavior.

Source: Coding guidelines

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@src/application/commands/skills/managed-skill-uninstall.ts`:
- Around line 469-503: Update resolveRegistrySkillUninstallTargets to read all
installation directory states concurrently with Promise.all, then process the
resulting installation/state pairs to populate targets and
unmanagedInstallations while preserving the existing absent, managed-registry,
and unmanaged behavior.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: ce32368f-94d8-4d6a-9b3c-ad36b1aca8bd

📥 Commits

Reviewing files that changed from the base of the PR and between a41bf88 and f8c2458.

📒 Files selected for processing (38)

• src/application/commands/skills/adopt.ts
• src/application/commands/skills/auto-sync.ts
• src/application/commands/skills/bundled-skill-model.test.ts
• src/application/commands/skills/bundled-skill-model.ts
• src/application/commands/skills/bundled-skill-observation.test.ts
• src/application/commands/skills/bundled-skill-observation.ts
• src/application/commands/skills/check-update.ts
• src/application/commands/skills/embedded-assets.ts
• src/application/commands/skills/index.cli.test.ts
• src/application/commands/skills/index.test.ts
• src/application/commands/skills/info-inventory.ts
• src/application/commands/skills/install.ts
• src/application/commands/skills/legacy-codex-cleanup.ts
• src/application/commands/skills/legacy-gpt-image-2-cleanup.ts
• src/application/commands/skills/local-skill-ownership.ts
• src/application/commands/skills/local-skill-source.ts
• src/application/commands/skills/managed-skill-listings.ts
• src/application/commands/skills/managed-skill-metadata.test.ts
• src/application/commands/skills/managed-skill-metadata.ts
• src/application/commands/skills/managed-skill-publication.test.ts
• src/application/commands/skills/managed-skill-publication.ts
• src/application/commands/skills/managed-skill-uninstall.ts
• src/application/commands/skills/operation-result.ts
• src/application/commands/skills/package-conversion.test.ts
• src/application/commands/skills/package-conversion.ts
• src/application/commands/skills/package-templates.ts
• src/application/commands/skills/publish.ts
• src/application/commands/skills/registry-skill-install.ts
• src/application/commands/skills/registry-skill-publication.ts
• src/application/commands/skills/repair.ts
• src/application/commands/skills/share.ts
• src/application/commands/skills/shared.ts
• src/application/commands/skills/skill-directory-state.test.ts
• src/application/commands/skills/skill-directory-state.ts
• src/application/commands/skills/skill-metadata.test.ts
• src/application/commands/skills/sync.ts
• src/application/commands/skills/uninstall.ts
• src/application/commands/skills/update.ts

💤 Files with no reviewable changes (9)

• src/application/commands/skills/managed-skill-metadata.test.ts
• src/application/commands/skills/managed-skill-publication.test.ts
• src/application/commands/skills/managed-skill-publication.ts
• src/application/commands/skills/package-templates.ts
• src/application/commands/skills/bundled-skill-model.ts
• src/application/commands/skills/bundled-skill-model.test.ts
• src/application/commands/skills/operation-result.ts
• src/application/commands/skills/local-skill-ownership.ts
• src/application/commands/skills/managed-skill-metadata.ts