fix: address PR #17 Copilot review

Six fixes in one commit, addressing every Copilot comment except #7
(dead code, deferred to a later cleanup pass).

#4 — manifest shape mismatch (critical):
  push.ts was still reading/writing the old manifest format
  (files[path] = hashString) while pull.ts and sync.ts use the new
  {localHash, remoteMtime} shape. push now uses the new shape,
  migrates old manifests on read (mirrors sync.ts detector), and
  refreshes remoteMtime via getRemoteMtimes() after a successful
  upload so the next pull/sync sees a consistent picture.

#3 — partial-success manifest (important):
  In --batch mode we were marking every instance as synced when
  result.uploaded > 0, even if some failed. Now we derive failed
  paths from result.errors and only add successes to the manifest.
  Failed uploads stay out and get retried on the next run.

#6 — ATOMIC_SOURCE_EXTENSIONS gap (important):
  EXTENSION_MAP was missing .tsx/.jsx/.cjs/.scss/.less/.sass, so
  those extensions got application/octet-stream and were routed
  through individual POST instead of /_atomic despite being listed
  as atomic-source compatible. Now mapped to
  application/typescript/javascript and text/x-* respectively, so
  isTextFile() returns true and they take the atomic path.

#2 — CLI --version out of sync:
  program.version() was hardcoded to '1.0.0' while package.json is
  at 1.0.1. Now reads from package.json via createRequire so
  boxel --version stays accurate on every release.

#1 — --batch-size NaN guard:
  parseInt on bad input (e.g. "abc") returned NaN, which bypasses
  `?? 10` and flows into the uploader as NaN. Now uses a
  parsePositiveInt parser that throws InvalidArgumentError with
  a friendly message on non-positive-integer input.

#5 — invalid-JSON test expectation:
  The test expected data.type='file' but the code (correctly) emits
  'source' because /_atomic only accepts 'card' and 'source'
  resource types. Test updated to match the correct contract.

All 164 tests pass (was 163/164 before this commit).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Author: 2 people

src/commands/push.ts

@@ -5,9 +5,32 @@ import * as fs from 'fs';
 import * as path from 'path';
 import * as crypto from 'crypto';
 
+interface SyncManifestFile {
+  localHash: string;    // md5 of file bytes at last sync
+  remoteMtime: number;  // remote mtime at last sync (seconds)
+}
+
 interface SyncManifest {
   workspaceUrl: string;
-  files: Record<string, string>; // relativePath -> contentHash
+  lastSyncTime?: number;
+  files: Record<string, SyncManifestFile>;
+}
+
+// Old push-only manifest format kept for migration. Matches the detector in sync.ts.
+interface OldManifest {
+  workspaceUrl: string;
+  files: Record<string, string>;
+}
+
+function isOldManifest(m: unknown): m is OldManifest {
+  if (!m || typeof m !== 'object') return false;
+  const files = (m as { files?: unknown }).files;
+  if (!files || typeof files !== 'object') return false;
+  for (const v of Object.values(files as Record<string, unknown>)) {
+    if (typeof v === 'string') return true;
+    if (v && typeof v === 'object') return false;
+  }
+  return false;
 }
 
 function computeFileHash(filePath: string): string {
@@ -17,14 +40,24 @@ function computeFileHash(filePath: string): string {
 
 function loadManifest(localDir: string): SyncManifest | null {
   const manifestPath = path.join(localDir, '.boxel-sync.json');
-  if (fs.existsSync(manifestPath)) {
-    try {
-      return JSON.parse(fs.readFileSync(manifestPath, 'utf8'));
-    } catch {
-      return null;
+  if (!fs.existsSync(manifestPath)) return null;
+  try {
+    const raw = JSON.parse(fs.readFileSync(manifestPath, 'utf8'));
+    if (isOldManifest(raw)) {
+      // Migrate: old { files: {path: hash} } → new { files: {path: {localHash, remoteMtime: 0}} }
+      const migrated: SyncManifest = {
+        workspaceUrl: raw.workspaceUrl,
+        files: {},
+      };
+      for (const [p, hash] of Object.entries(raw.files)) {
+        migrated.files[p] = { localHash: hash, remoteMtime: 0 };
+      }
+      return migrated;
     }
+    return raw as SyncManifest;
+  } catch {
+    return null;
   }
-  return null;
 }
 
 function saveManifest(localDir: string, manifest: SyncManifest): void {
@@ -105,14 +138,15 @@ class RealmPusher extends RealmSyncBase {
           continue;
         }
         const currentHash = computeFileHash(localPath);
-        const previousHash = manifest.files[relativePath];
+        const previousEntry = manifest.files[relativePath];
+        const previousHash = previousEntry?.localHash;
 
         if (previousHash !== currentHash) {
           filesToUpload.set(relativePath, localPath);
         } else {
           skipped++;
-          // Keep the hash in new manifest
-          newManifest.files[relativePath] = currentHash;
+          // Keep the entry in new manifest (preserve remoteMtime)
+          newManifest.files[relativePath] = previousEntry;
         }
       }
 
@@ -147,7 +181,10 @@ class RealmPusher extends RealmSyncBase {
         for (const file of definitions) {
           try {
             await this.uploadFile(file.relativePath, file.localPath);
-            newManifest.files[file.relativePath] = computeFileHash(file.localPath);
+            newManifest.files[file.relativePath] = {
+              localHash: computeFileHash(file.localPath),
+              remoteMtime: 0, // will be filled in by the remote-mtime refresh below
+            };
           } catch (error) {
             this.hasError = true;
             console.error(`Error uploading ${file.relativePath}:`, error);
@@ -165,12 +202,16 @@ class RealmPusher extends RealmSyncBase {
           dryRun: this.options.dryRun,
         });
 
-        // Update manifest for successfully uploaded files
-        if (result.uploaded > 0) {
-          for (const file of instances) {
-            if (fs.existsSync(file.localPath)) {
-              newManifest.files[file.relativePath] = computeFileHash(file.localPath);
-            }
+        // Mark only SUCCESSFUL files in the manifest. Failures stay out so the
+        // next run retries them.
+        const failedPaths = new Set(result.errors.map(e => e.path));
+        for (const file of instances) {
+          if (failedPaths.has(file.relativePath)) continue;
+          if (fs.existsSync(file.localPath)) {
+            newManifest.files[file.relativePath] = {
+              localHash: computeFileHash(file.localPath),
+              remoteMtime: 0, // refreshed below
+            };
           }
         }
 
@@ -188,7 +229,10 @@ class RealmPusher extends RealmSyncBase {
         try {
           await this.uploadFile(relativePath, localPath);
           // Add to manifest after successful upload
-          newManifest.files[relativePath] = computeFileHash(localPath);
+          newManifest.files[relativePath] = {
+            localHash: computeFileHash(localPath),
+            remoteMtime: 0, // refreshed below
+          };
         } catch (error) {
           this.hasError = true;
           console.error(`Error uploading ${relativePath}:`, error);
@@ -228,6 +272,29 @@ class RealmPusher extends RealmSyncBase {
       }
     }
 
+    // Refresh remote mtimes for every file we just put in the manifest so
+    // subsequent sync/pull operations don't mistakenly see remote-as-changed.
+    // Skip on dry-run (no network side effects).
+    if (!this.options.dryRun && Object.keys(newManifest.files).length > 0) {
+      try {
+        const remoteMtimes = await this.getRemoteMtimes();
+        for (const [relPath, entry] of Object.entries(newManifest.files)) {
+          const mtime = remoteMtimes.get(relPath);
+          if (typeof mtime === 'number') {
+            entry.remoteMtime = mtime;
+          } else if (entry.remoteMtime === 0) {
+            // Fall back to local time in seconds; imperfect but better than 0
+            entry.remoteMtime = Math.floor(Date.now() / 1000);
+          }
+        }
+      } catch (err) {
+        // Non-fatal: manifest entries stay with remoteMtime:0 and sync will
+        // reconcile on next run
+        console.warn('Warning: could not refresh remote mtimes for manifest:', err);
+      }
+      newManifest.lastSyncTime = Date.now();
+    }
+
     // Save manifest for future incremental syncs
     if (!this.options.dryRun) {
       saveManifest(this.options.localDir, newManifest);

src/index.ts

@@ -1,7 +1,17 @@
 #!/usr/bin/env node
 
 import 'dotenv/config';
-import { Command } from 'commander';
+import { Command, InvalidArgumentError } from 'commander';
+
+/** Parse a positive integer from a CLI flag; throw a friendly error otherwise. */
+function parsePositiveInt(raw: string, _prev: unknown): number {
+  const n = parseInt(raw, 10);
+  if (!Number.isFinite(n) || n < 1) {
+    throw new InvalidArgumentError(`expected a positive integer, got "${raw}"`);
+  }
+  return n;
+}
+
 import { pushCommand } from './commands/push.js';
 import { pullCommand } from './commands/pull.js';
 import { listCommand } from './commands/list.js';
@@ -26,13 +36,19 @@ import { repairRealmCommand, repairRealmsCommand } from './commands/repair.js';
 import { consolidateWorkspacesCommand } from './commands/consolidate.js';
 import { loadConfig } from './lib/realm-config.js';
 import { warnIfLegacyWorkspacePaths } from './lib/workspace-paths.js';
+import { createRequire } from 'module';
+
+// Read version from package.json so `boxel --version` stays in sync with the
+// published package. Using require() avoids ESM JSON-import assertion syntax
+// that varies across Node versions.
+const pkg = createRequire(import.meta.url)('../package.json') as { version: string };
 
 const program = new Command();
 
 program
   .name('boxel')
   .description('CLI tools for syncing files between local directories and Boxel workspaces')
-  .version('1.0.0');
+  .version(pkg.version);
 
 program.hook('preAction', (_thisCommand, actionCommand) => {
   const commandName = actionCommand.name();
@@ -66,7 +82,7 @@ program
   .option('--dry-run', 'Show what would be done without making changes')
   .option('--force', 'Upload all files, even if unchanged')
   .option('--batch', 'Use atomic batch upload for faster bulk operations (10 files per batch)')
-  .option('--batch-size <n>', 'Files per batch when using --batch (default: 10)', parseInt)
+  .option('--batch-size <n>', 'Files per batch when using --batch (default: 10)', parsePositiveInt)
   .action(async (localDir: string, workspaceUrl: string, options: { delete?: boolean; dryRun?: boolean; force?: boolean; batch?: boolean; batchSize?: number }) => {
     await pushCommand(localDir, workspaceUrl, options);
   });

src/lib/content-type.ts

@@ -8,7 +8,13 @@ const EXTENSION_MAP: Record<string, string> = {
   '.css': 'text/css',
   '.js': 'application/javascript',
   '.ts': 'application/typescript',
+  '.tsx': 'application/typescript',
+  '.jsx': 'application/javascript',
   '.mjs': 'application/javascript',
+  '.cjs': 'application/javascript',
+  '.scss': 'text/x-scss',
+  '.less': 'text/x-less',
+  '.sass': 'text/x-sass',
   '.png': 'image/png',
   '.jpg': 'image/jpeg',
   '.jpeg': 'image/jpeg',
@@ -37,6 +43,8 @@ export function getContentType(filePath: string): string {
 }
 
 export function isTextFile(contentType: string): boolean {
+  // Includes the text/x-* family so .scss/.less/.sass route through the
+  // text path and match ATOMIC_SOURCE_EXTENSIONS.
   return (
     contentType.startsWith('text/') ||
     contentType === 'application/json' ||

test/lib/batch-upload.test.ts

@@ -218,15 +218,18 @@ describe('buildAtomicRequest', () => {
     expect(request['atomic:operations'][0].op).toBe('update');
   });
 
-  it('falls back to file type for invalid JSON', () => {
+  it('falls back to source type for invalid JSON', () => {
+    // The /_atomic endpoint only accepts 'card' and 'source' resource types.
+    // When a .json file can't be parsed as a card, we fall back to 'source' so
+    // the request still succeeds (rather than 'file', which isn't a valid type).
     const files = [
       createFile('bad.json', 'not valid json {{'),
     ];
 
     const request = buildAtomicRequest(files, 'https://realm.test/');
 
     const op = request['atomic:operations'][0];
-    expect(op.data.type).toBe('file');
+    expect(op.data.type).toBe('source');
     expect(op.data.attributes?.content).toBe('not valid json {{');
   });