fix: close remaining validation gaps

README.md

@@ -453,7 +453,7 @@ skillsync daemon
 - SkillSync will not silently overwrite an unmanaged local folder.
 - SkillSync refuses to overwrite or remove a locally edited managed copy unless you explicitly discard the edits.
 - Skill projection applies restore their previous state after a failure.
-- Vault checks reject symlinks, malformed JSON, stale registry entries, reserved ownership markers, and common credential formats.
+- Vault checks reject symlinks, malformed JSON, stale registry entries, reserved ownership markers, and common credential formats. Pushes also check unpushed commit additions so removing a credential in a later commit does not silently publish it.
 - Symlinked content outside a configured target is not auto-adopted.
 - Different same-name skills require explicit conflict resolution.
 - Plugin sync is additive and never copies connector credentials.

SECURITY.md

@@ -24,4 +24,4 @@ Device-local skill targets and global instruction paths are approved on that dev
 
 Skill folders can contain executable instructions or supporting scripts. Users should review third-party skills before adding them to a vault.
 
-SkillSync checks vault files for symlinks, malformed JSON, stale registry entries, reserved ownership markers, and common credential formats before syncing or pushing. This check reduces accidental exposure but cannot recognize every secret. Keep credentials, OAuth state, and session data out of skill folders and instruction profiles.
+SkillSync checks vault files for symlinks, malformed JSON, stale registry entries, reserved ownership markers, and common credential formats before syncing or pushing. It also checks additions in unpushed commits for those credential formats. These checks reduce accidental exposure but cannot recognize every secret. Keep credentials, OAuth state, and session data out of skill folders and instruction profiles.

src/cli.js

@@ -24,7 +24,7 @@ import {
   setTargetAutoImport,
   uninstallSkillAndPrune,
 } from './core/device.js';
-import { checkVault } from './core/check.js';
+import { checkVault, checkVaultForPush } from './core/check.js';
 import {
   matrixAssignmentChanges,
   renderSkillSelectionChanges,
@@ -215,19 +215,21 @@ function promptPageSize(itemCount, { min = 8, max = 28, reservedRows = 6 } = {})
   return Math.max(1, Math.min(itemCount, max, availableRows));
 }
 
-async function configured() {
+async function configured({ initialize = true } = {}) {
   const config = await loadConfig();
   if (!config.repoPath || !await exists(config.repoPath)) {
     throw new Error('SkillSync is not set up. Run: skillsync setup');
   }
   if (!await isGitRepo(config.repoPath)) {
     throw new Error(`SkillSync vault checkout is missing Git metadata: ${config.repoPath}`);
   }
-  await ensureVault(config.repoPath);
-  await migrateLegacyLocalPathState({
-    vaultPath: config.repoPath,
-    deviceId: config.deviceId,
-  });
+  if (initialize) {
+    await ensureVault(config.repoPath);
+    await migrateLegacyLocalPathState({
+      vaultPath: config.repoPath,
+      deviceId: config.deviceId,
+    });
+  }
   return config;
 }
 
@@ -406,9 +408,11 @@ async function commitInitialVault(repoPath) {
   await git(['add', 'README.md', 'registry.json', 'vault.json', 'skills', 'devices'], repoPath).catch(() => {});
   const committed = await commitAllIfChanged(repoPath, 'chore: initialize skills vault');
   if (committed) {
+    await checkVaultForPush(repoPath);
     try {
       await push(repoPath);
     } catch {
+      await checkVaultForPush(repoPath);
       await git(['push', '-u', 'origin', 'HEAD:main'], repoPath);
     }
   }
@@ -534,6 +538,7 @@ async function groupsCommand(rest) {
   const result = await generateGroups({ vaultPath: config.repoPath, write: true });
   await checkVault(config.repoPath);
   await commitAllIfChanged(config.repoPath, 'docs: update skill groups');
+  await checkVaultForPush(config.repoPath);
   await push(config.repoPath);
   console.log(`Generated skill groups for ${result.skillCount} skills across ${result.packCount} packs.`);
   console.log(`Files: ${result.files.join(', ')}`);
@@ -1734,17 +1739,19 @@ async function policyCommand(rest) {
 }
 
 async function syncCommand(rest) {
-  const config = await configured();
   const allowed = new Set(['--discard-local-changes', '--dry-run', '--no-pull']);
   const unsupported = rest.find((argument) => !allowed.has(argument));
   if (unsupported) throw new Error(`Unknown sync option: ${unsupported}`);
+  const dryRun = hasFlag(rest, '--dry-run');
+  const config = await configured({ initialize: !dryRun });
   const discardLocalChanges = hasFlag(rest, '--discard-local-changes');
-  if (hasFlag(rest, '--dry-run')) {
+  if (dryRun) {
     await checkVault(config.repoPath, { verifyRegistry: false });
     const plan = await planLinks({
       vaultPath: config.repoPath,
       deviceId: config.deviceId,
       discardLocalChanges,
+      readOnly: true,
       registry: await buildRegistry(config.repoPath),
     });
     console.log('Dry run uses the current local vault and does not pull remote changes.');
@@ -1780,7 +1787,7 @@ async function scanCommand(rest = []) {
   const allowed = new Set(['--json']);
   const unsupported = rest.find((argument) => !allowed.has(argument));
   if (unsupported) throw new Error(`Unknown scan option: ${unsupported}`);
-  const config = await configured();
+  const config = await configured({ initialize: false });
   await checkVault(config.repoPath, { verifyRegistry: false });
   const report = await inspectTargets({
     vaultPath: config.repoPath,
@@ -1800,7 +1807,7 @@ async function scanCommand(rest = []) {
 
 async function checkCommand(rest = []) {
   if (rest.length) throw new Error('Usage: skillsync check');
-  const config = await configured();
+  const config = await configured({ initialize: false });
   const result = await checkVault(config.repoPath);
   console.log(`Vault check passed: ${result.skills} skills, ${result.files} files.`);
 }

src/core/check.js

@@ -3,6 +3,7 @@ import { lstat, readFile, readdir } from 'node:fs/promises';
 import path from 'node:path';
 
 import { assertSafePathSegment, hashDirectory } from './fs.js';
+import { git, isGitRepo } from './git.js';
 
 const GIT_ENTRY = '.git';
 const LOCAL_STATE_ENTRY = '.skillsync-local';
@@ -15,6 +16,10 @@ const SECRET_PATTERNS = [
   ['Slack token', /\bxox[baprs]-[A-Za-z0-9-]{20,}\b/],
 ];
 
+function secretType(text) {
+  return SECRET_PATTERNS.find(([, pattern]) => pattern.test(text))?.[0] || null;
+}
+
 async function pathInfo(filePath) {
   return lstat(filePath).catch((error) => {
     if (error.code === 'ENOENT') return null;
@@ -26,13 +31,43 @@ async function detectedSecret(filePath) {
   let tail = '';
   for await (const chunk of createReadStream(filePath)) {
     const text = tail + chunk.toString('utf8');
-    const detected = SECRET_PATTERNS.find(([, pattern]) => pattern.test(text));
-    if (detected) return detected[0];
+    const detected = secretType(text);
+    if (detected) return detected;
     tail = text.slice(-512);
   }
   return null;
 }
 
+function inspectAddedPatchLines(patch) {
+  const errors = [];
+  let currentPath = null;
+  let inHunk = false;
+  for (const line of patch.split(/\r?\n/)) {
+    if (line.startsWith('diff --git ')) {
+      currentPath = null;
+      inHunk = false;
+      continue;
+    }
+    if (line.startsWith('+++ ')) {
+      const candidate = line.slice(4);
+      currentPath = candidate === '/dev/null'
+        ? null
+        : candidate.replace(/^b\//, '');
+      continue;
+    }
+    if (line.startsWith('@@')) {
+      inHunk = true;
+      continue;
+    }
+    if (!inHunk || !line.startsWith('+')) continue;
+    const detected = secretType(line.slice(1));
+    if (detected) {
+      errors.push(`Possible ${detected} in an unpushed commit${currentPath ? `: ${currentPath}` : ''}`);
+    }
+  }
+  return [...new Set(errors)];
+}
+
 async function inspectTree(rootPath, {
   ignoreRootEntries = new Set(),
   rejectOwnershipMarkers = false,
@@ -234,3 +269,36 @@ export async function checkVault(vaultPath, { verifyRegistry = true } = {}) {
     skills: skillNames.length,
   };
 }
+
+export async function checkPendingGitHistory(vaultPath) {
+  const root = path.resolve(vaultPath);
+  if (!await isGitRepo(root)) return { commits: 0 };
+  const hasUpstream = await git(['rev-parse', '--verify', '--quiet', '@{u}'], root)
+    .then(() => true)
+    .catch(() => false);
+  const revision = hasUpstream ? '@{u}..HEAD' : 'HEAD';
+  const { stdout: countOutput } = await git(['rev-list', '--count', revision], root);
+  const commits = Number(countOutput.trim());
+  if (!commits) return { commits: 0 };
+  const { stdout: patch } = await git([
+    'log',
+    '--format=',
+    '--patch',
+    '--cc',
+    '--unified=0',
+    '--no-color',
+    '--text',
+    ...(hasUpstream ? [revision] : ['--root', revision]),
+    '--',
+    '.',
+  ], root);
+  const errors = inspectAddedPatchLines(patch);
+  if (errors.length) throw validationError('Pending Git history check', errors);
+  return { commits };
+}
+
+export async function checkVaultForPush(vaultPath) {
+  const vault = await checkVault(vaultPath);
+  const history = await checkPendingGitHistory(vaultPath);
+  return { ...vault, pendingCommits: history.commits };
+}

src/core/device.js

@@ -80,8 +80,8 @@ export function globalInstructionsAssignmentPath(vaultPath, deviceId) {
   );
 }
 
-export async function loadDevice(vaultPath, deviceId = defaultDeviceId()) {
-  await ensureVault(vaultPath);
+export async function loadDevice(vaultPath, deviceId = defaultDeviceId(), { ensure = true } = {}) {
+  if (ensure) await ensureVault(vaultPath);
   const desired = await readJson(devicePath(vaultPath, deviceId), null);
   const reported = await readJson(deviceStatePath(vaultPath, deviceId), null);
   const instructionSelection = await readJson(
@@ -260,8 +260,8 @@ export async function migrateLegacyLocalPathState({
   });
 }
 
-export async function loadLocalDevice(vaultPath, deviceId = defaultDeviceId()) {
-  const device = await loadDevice(vaultPath, deviceId);
+export async function loadLocalDevice(vaultPath, deviceId = defaultDeviceId(), { ensure = true } = {}) {
+  const device = await loadDevice(vaultPath, deviceId, { ensure });
   const local = await readLocalPathState(vaultPath, deviceId);
   if (!local) {
     throw new Error(`Local paths for ${deviceId} are not initialized; run SkillSync setup or reconnect this device`);
@@ -922,10 +922,11 @@ export async function planLinks({
   vaultPath,
   deviceId = defaultDeviceId(),
   discardLocalChanges = false,
+  readOnly = false,
   replaceUnmanagedPaths = [],
   registry: providedRegistry,
 }) {
-  const device = await loadLocalDevice(vaultPath, deviceId);
+  const device = await loadLocalDevice(vaultPath, deviceId, { ensure: !readOnly });
   const registry = providedRegistry || await loadRegistry(vaultPath);
   const approvedReplacements = new Set(await Promise.all(
     replaceUnmanagedPaths.map((targetPath) => canonicalTargetRoot(targetPath)),
@@ -1323,7 +1324,7 @@ export async function scanTargets({ vaultPath, deviceId = defaultDeviceId() }) {
 }
 
 export async function inspectTargets({ vaultPath, deviceId = defaultDeviceId() }) {
-  const device = await loadLocalDevice(vaultPath, deviceId);
+  const device = await loadLocalDevice(vaultPath, deviceId, { ensure: false });
   const detected = await detectTargets({ vaultPath, device });
   const skills = [];
   for (const [targetName, entries] of Object.entries(detected)) {

src/core/sync.js

@@ -7,7 +7,7 @@ import {
   scanTargets,
   sweepUnusedSkills,
 } from './device.js';
-import { checkVault } from './check.js';
+import { checkVault, checkVaultForPush } from './check.js';
 import { commitAllIfChanged, hasLocalCommitsToPush, isGitRepo, pullRebase, pushWithPullRebaseRetry } from './git.js';
 import {
   applyGlobalInstructions,
@@ -88,7 +88,7 @@ export async function syncVault({
     committed = await commitAllIfChanged(vaultPath, `sync: update skills from ${deviceId || 'device'}`);
     if (committed || await hasLocalCommitsToPush(vaultPath)) {
       const pushResult = await pushWithPullRebaseRetry(vaultPath, {
-        beforePush: () => checkVault(vaultPath),
+        beforePush: () => checkVaultForPush(vaultPath),
       });
       pushed = pushResult.pushed;
       rebasedBeforePush = pushResult.rebased;

test/cli.test.js

@@ -23,7 +23,8 @@ import {
   scanTargets,
   setTargetAutoImport,
 } from '../src/core/device.js';
-import { git } from '../src/core/git.js';
+import { exists } from '../src/core/fs.js';
+import { git, gitPrivatePath } from '../src/core/git.js';
 import { loadRegistry, rebuildRegistry, setVaultPolicy } from '../src/core/registry.js';
 
 const execFileAsync = promisify(execFile);
@@ -283,6 +284,45 @@ test('check validates the vault without rewriting a stale registry', async () =>
   assert.equal(await readFile(path.join(vault, 'registry.json'), 'utf8'), registryBefore);
 });
 
+test('check does not initialize private device state', async () => {
+  const home = await tempDir();
+  const vault = path.join(home, '.skillsync', 'repo');
+  const deviceId = 'test-device';
+
+  await writeConfig(home, vault, deviceId);
+  await rebuildRegistry(vault);
+  const privateState = await gitPrivatePath(vault, 'local', 'devices', `${deviceId}.json`);
+  assert.equal(await exists(privateState), false);
+
+  await execFileAsync(process.execPath, [path.resolve('src/cli.js'), 'check'], {
+    cwd: path.resolve('.'),
+    env: cliEnv(home),
+  });
+
+  assert.equal(await exists(privateState), false);
+});
+
+test('scan and dry-run do not migrate missing local state', async () => {
+  const home = await tempDir();
+  const vault = path.join(home, '.skillsync', 'repo');
+  const deviceId = 'test-device';
+
+  await writeConfig(home, vault, deviceId);
+  await rebuildRegistry(vault);
+  const privateState = await gitPrivatePath(vault, 'local', 'devices', `${deviceId}.json`);
+
+  for (const args of [['scan'], ['sync', '--dry-run']]) {
+    await assert.rejects(
+      () => execFileAsync(process.execPath, [path.resolve('src/cli.js'), ...args], {
+        cwd: path.resolve('.'),
+        env: cliEnv(home),
+      }),
+      /Local paths for test-device are not initialized/,
+    );
+    assert.equal(await exists(privateState), false);
+  }
+});
+
 test('matrix shows cross-device assignments and device auto-adoption can be disabled', async () => {
   const home = await tempDir();
   const vault = path.join(home, '.skillsync', 'repo');

test/git.test.js

@@ -155,6 +155,41 @@ test('syncVault pushes existing ahead commits after pulling remote changes', asy
   assert.match(stdout, /remote change/);
 });
 
+test('sync refuses pending commits that contain a removed credential', async () => {
+  const root = await tempDir();
+  const remote = path.join(root, 'remote.git');
+  const seed = path.join(root, 'seed');
+  const pending = path.join(root, 'pending');
+
+  await git(['init', '--bare', remote]);
+  await git(['clone', remote, seed]);
+  await configureUser(seed);
+  await ensureVault(seed);
+  await git(['add', '-A'], seed);
+  await git(['commit', '-m', 'initial vault'], seed);
+  await git(['push', '-u', 'origin', 'HEAD:main'], seed);
+  await git(['symbolic-ref', 'HEAD', 'refs/heads/main'], remote);
+
+  await git(['clone', remote, pending]);
+  await configureUser(pending);
+  await commitFile(
+    pending,
+    'notes.txt',
+    'sk-proj-1234567890abcdefghijklmnop\n',
+    'add notes',
+  );
+  await commitFile(pending, 'notes.txt', 'safe notes\n', 'sanitize notes');
+
+  await assert.rejects(
+    () => syncVault({ vaultPath: pending, pull: false }),
+    /Pending Git history check failed:[\s\S]*Possible API key/,
+  );
+
+  const remoteHead = await git(['rev-parse', 'refs/heads/main'], remote);
+  const seedHead = await git(['rev-parse', 'HEAD'], seed);
+  assert.equal(remoteHead.stdout, seedHead.stdout);
+});
+
 test('fresh devices keep synced filesystem destinations unapproved', async () => {
   const root = await tempDir();
   const vault = path.join(root, 'vault');