feat: add catalog audit and doctor integration by joshkennedy · Pull Request #53 · AksharP5/skillsync

joshkennedy · 2026-08-02T00:05:59Z

Summary

add skillsync audit [--json] for Agent Skills metadata validation, duplicate/conflict reporting, and per-target description token estimates
scan configured target paths on every audit instead of trusting cached device inventory
include the catalog summary in skillsync doctor

Part of #52.

Review feedback addressed

narrowed this PR to catalog audit and doctor integration
removed exact pack reconciliation, source provenance and updates, cleanup, and dormant-pack search from the final diff
kept skillsync audit read-only, including bypassing vault initialization and registry refresh
validate required metadata plus all optional Agent Skills fields: license, compatibility, metadata, and allowed-tools
added regressions for stale target inventory, deleted cached entries, non-mapping frontmatter and metadata, invalid optional field types, metadata length limits, managed projections, and conflicting copies

Safety

The audit path does not sync, auto-adopt, project, initialize, rebuild the registry, commit, or push. It reads current vault and target contents only.

Validation

npm test: 120 passing tests on the current v0.12.1 base
npm pack --dry-run
node --check src/cli.js
node --check src/core/audit.js
git diff --check

AksharP5 · 2026-08-02T20:45:59Z

@codex review

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: afa83640b4

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

chatgpt-codex-connector · 2026-08-02T20:55:31Z

+  const device = await loadLocalDevice(config.repoPath, config.deviceId);
+  const result = await auditCatalog({ vaultPath: config.repoPath, device });


Refresh target inventory before auditing

When a local skill is added or removed after the last scan, loadLocalDevice supplies the cached detected arrays, and auditCatalog prefers those arrays instead of scanning the configured paths. Consequently skillsync audit can completely miss a newly added invalid/conflicting skill or report a deleted skill as unreadable until another command happens to scan first; refresh the target inventory before building the audit.

Useful? React with 👍 / 👎.

AksharP5

@joshkennedy Thanks for the substantial contribution. The direction is useful, especially catalog auditing and retaining upstream source information, but this combines several independently risky features and is not ready to merge safely.

Please split the work into focused PRs:

catalog audit and doctor integration;
exact pack reconciliation;
source provenance and updates.

Please leave cleanup out for now. It overlaps existing add/import/auto-adoption behavior and introduces another destructive folder-migration path without a demonstrated need yet.

For the audit PR, please address the existing Codex thread about stale target inventory, keep the command read-only, and validate every Agent Skills metadata constraint the command claims to cover.

Source updates need a three-way comparison between the recorded baseline, current vault content, and current upstream content. They must refuse divergent updates rather than ignoring or overwriting local edits. Provenance also needs storage that an older SkillSync client cannot erase during its normal registry rebuild.

Exact pack previews must not perform unrelated mutations before --apply.

Please add regression tests for each case. Once the revised commits are pushed, we can rerun CI and request another Codex review against the new head.

AksharP5 · 2026-08-02T22:36:58Z

+    }
+    await validateSkillFolder(sourcePath);
+    const remoteHash = await hashDirectory(sourcePath);
+    if (remoteHash === entry.hash) {


This compares upstream content only with the hash cached in registry.json, not the actual current vault folder. I reproduced editing the vault skill after import and this still returned "current". If both local and upstream content change, --apply can overwrite the local edit without detecting divergence. Please compare the recorded baseline, actual vault hash, and upstream hash, and refuse apply when both sides changed. Add tests for local-only, upstream-only, and divergent changes.

AksharP5 · 2026-08-02T22:36:58Z

    const skillName = assertSafePathSegment(entry.name, 'Skill name');
    if (!await exists(path.join(skillsDir, skillName, 'SKILL.md'))) continue;
-    registry.skills[skillName] = await registryEntryForSkill(vaultPath, skillName);
+    registry.skills[skillName] = await registryEntryForSkill(vaultPath, skillName, {


This preserves source metadata only when the client already contains this new code. A v0.10.5 device rebuilds each registry entry with path/hash/updated_at only, so its next normal sync removes every source field and can push that removal to the shared vault. I reproduced that behavior with the current release. Please store provenance somewhere older clients will leave untouched, or provide another mixed-version-safe migration design, with a regression test.

AksharP5 · 2026-08-02T22:36:58Z

+    } else if (description.length > RECOMMENDED_DESCRIPTION_LENGTH) {
+      findings.push(finding('warning', 'description-verbose', `Description is ${description.length} characters; consider keeping discovery metadata under ${RECOMMENDED_DESCRIPTION_LENGTH}`));
+    }
+  }


The command is documented as Agent Skills metadata validation, but this stops after name and description. It currently reports no findings for invalid optional fields such as compatibility longer than 500 characters, non-mapping metadata, or non-string allowed-tools. Please validate the remaining hard specification constraints, or narrow the documented claim and output so a clean audit does not imply full spec validity.

AksharP5 · 2026-08-02T22:36:58Z

+    const targets = parseTargets(rest.slice(2));
+    if (!targets?.length) throw new Error('Pack apply requires --target <targets> or --global');
+    const deviceId = requestedDeviceId(config, rest);
+    await pullBeforeRemoteEdit(config, deviceId);


For a remote device, this runs a full local sync before the code checks --apply. That sync may auto-adopt skills, apply projections, commit, and push unrelated changes, even though pack apply is presented as a dry-run preview by default. Please make preview side-effect-free and perform any required mutating synchronization only in the apply path.

…ility # Conflicts: # src/cli.js # src/core/device.js

joshkennedy · 2026-09-02T13:39:58Z

Updated the PR against current v0.12.1 based on the review. The final diff now contains only catalog audit and doctor integration. Audit scans live target contents without mutating persisted state, and the metadata validator covers all required and optional Agent Skills frontmatter constraints with regression tests. Local validation: 120 tests pass, package dry-run passes, syntax checks pass, and diff check passes. @codex review

chatgpt-codex-connector · 2026-09-02T13:40:07Z

To use Codex here, create a Codex account and connect to github.

joshkennedy · 2026-09-02T13:40:48Z

@AksharP5 the requested revision is ready for another look. GitHub would not accept a formal reviewer request from this fork, and the Codex bot rerun is currently blocked by its GitHub connector configuration.

AksharP5

Thanks for narrowing this to catalog audit and doctor integration. This is much closer: the command is read-only, scans live target contents, and the focused tests are good.

I found four correctness issues before this is ready to merge. The full suite passes locally: 120 tests, clean install, package dry-run, syntax checks, and diff check.

AksharP5 · 2026-09-02T18:57:49Z

+import { expandHome, hashDirectory } from './fs.js';
+import { discoverSkillFolders } from './source.js';
+
+const PORTABLE_NAME = /^[a-z0-9]+(?:-[a-z0-9]+)*$/;


The Agent Skills reference validator supports lowercase Unicode alphanumeric names and rejects unknown top-level fields. This currently does the reverse: a valid name such as café fails, while an unsupported custom-field passes silently.

Please align this with the reference validator, or explicitly document this as a stricter portable subset instead of claiming complete Agent Skills validation. Add regressions for both cases.

AksharP5 · 2026-09-02T18:57:50Z

+      discoveredByTarget.get(targetName).push(skillName);
+      if (!descriptionsByTarget.has(targetName)) descriptionsByTarget.set(targetName, new Map());
+      descriptionsByTarget.get(targetName).set(skillName, audited.descriptionLength);
+      if (!hash || hash !== vaultHashes.get(skillName)) {


Managed copy projections contain .skillsync-owned.json, so their full-directory hash differs from the vault even when the skill content is unchanged. Because ownership is checked afterward, an unchanged managed copy is reported as an external variant and can double-count findings.

Please exclude the ownership marker when hashing managed copies, matching the existing copy-projection logic.

AksharP5 · 2026-09-02T18:57:50Z

    console.log(`✓ vault checked: ${result.skills} skills, ${result.files} files`);
+    const device = await loadLocalDevice(config.repoPath, config.deviceId, { ensure: false });
+    const audit = await auditCatalog({ vaultPath: config.repoPath, device });
+    console.log(`${audit.summary.errors ? '✗' : '✓'} catalog: ${audit.summary.skills} skills, ${audit.summary.errors} errors, ${audit.summary.warnings} warnings`);


skillsync audit fails when conflicting duplicates exist, but skillsync doctor shows a green catalog whenever summary.errors is zero.

Please use the same failure predicate in both commands and include conflicting duplicate counts in the doctor summary.

AksharP5 · 2026-09-02T18:57:50Z

+      .map(([name]) => name)
+      .sort();
+    const detected = discoveredByTarget.get(targetName) || [];
+    const active = [...new Set([...assigned, ...detected])].sort();


active currently combines desired assignments with skills actually discovered on disk. An assigned but missing projection therefore inflates both the active-skill count and estimated loaded tokens.

Please calculate active catalog cost from live discovered skills and keep the desired assignment count separate.

joshkennedy added 6 commits August 1, 2026 19:05

feat: add catalog reliability controls

3ddd3cb

feat: safely consolidate duplicate skills

f0b6546

feat: adopt complete local skill catalogs

66c1080

fix: preserve directory symlinks during migration

d817d09

fix: allow prototype skill name

47e1ab4

fix: exclude managed projections from duplicate audit

afa8364

chatgpt-codex-connector Bot reviewed Aug 2, 2026

View reviewed changes

AksharP5 requested changes Aug 2, 2026

View reviewed changes

joshkennedy added 3 commits August 3, 2026 08:08

feat: search dormant skill packs

0fc7de6

fix: focus catalog audit review

8216bbe

Merge remote-tracking branch 'upstream/main' into feat/catalog-reliab…

ef808bf

…ility # Conflicts: # src/cli.js # src/core/device.js

joshkennedy changed the title ~~feat: add catalog reliability controls~~ feat: add catalog audit and doctor integration Sep 2, 2026

AksharP5 requested changes Sep 2, 2026

View reviewed changes

		const device = await loadLocalDevice(config.repoPath, config.deviceId);
		const result = await auditCatalog({ vaultPath: config.repoPath, device });

Conversation

joshkennedy commented Aug 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Review feedback addressed

Safety

Validation

Uh oh!

AksharP5 commented Aug 2, 2026

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

chatgpt-codex-connector Bot Aug 2, 2026

Choose a reason for hiding this comment

Uh oh!

AksharP5 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joshkennedy commented Sep 2, 2026

Uh oh!

chatgpt-codex-connector Bot commented Sep 2, 2026

Uh oh!

joshkennedy commented Sep 2, 2026

Uh oh!

AksharP5 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

joshkennedy commented Aug 2, 2026 •

edited

Loading

AksharP5 left a comment •

edited

Loading