fix: make review feature locks atomic

CHANGELOG.md

@@ -15,6 +15,7 @@
 - Added Next.js route mapping for `src/app` and `src/pages` layouts, thanks @obatried.
 - Added first-pass Python mapping for project metadata, console scripts, source groups, pytest suites, and conservative validation defaults, thanks @xiamx.
 - Added progress output for `clawpatch revalidate`, thanks @twidtwid.
+- Fixed overlapping `clawpatch review` runs so feature claims use atomic lock files and can be recovered with `clean-locks`, thanks @rohitjavvadi.
 - Added React Router and React component mapping, thanks @moritzscheele.
 - Improved Node/TypeScript mapping for large workspaces by splitting package source trees into bounded review groups with package-local tests.
 - Added generic nested SwiftPM, Apple/Xcode, and Gradle/Android app mapping.

docs/code-review.md

@@ -18,7 +18,7 @@ clawpatch review --provider codex --model <model>
 Current behavior:
 
 - selects pending features unless `--feature` is set
-- claims each feature with a run lock
+- claims each feature with an atomic lock file plus the feature run lock
 - reviews with a bounded worker pool; default `--jobs` is `10`
 - emits progress to stderr unless `--quiet` is set
 - builds bounded prompt context from owned files, context files, and tests
@@ -47,8 +47,12 @@ The same flag is available on `revalidate`; revalidation scopes open findings to
 features whose owned files changed.
 
 Progress uses stderr so `--json` stdout remains machine-readable. The worker
-pool is per-process and still uses feature locks, so overlapping runs should not
-claim the same feature.
+pool is per-process, and lock files under `.clawpatch/locks/` prevent
+overlapping review processes from claiming the same feature. Interrupted runs
+can leave recoverable lock files; clear them with `clawpatch clean-locks` after
+confirming no review process is still active. `clawpatch status` includes both
+feature-record locks and lock files in `activeLocks`, and reports the lock-file
+count as `lockFiles`.
 
 There is no multi-provider panel yet.
 

docs/safety.md

@@ -14,8 +14,8 @@ Current safety rules:
 - `.clawpatch/` state changes are allowed during runs.
 - review and revalidate provider calls use a read-only sandbox.
 - provider output must pass runtime schema validation.
-- feature locks are stored in feature records and can be cleared with
-  `clawpatch clean-locks`.
+- feature locks are stored in feature records and `.clawpatch/locks/`; `status`
+  surfaces both, and `clean-locks` clears both.
 - the mapper skips symlinked directories and common generated directories.
 
 Not implemented today:

src/app.ts

@@ -12,8 +12,11 @@ import { mapFeatures } from "./mapper.js";
 import { providerByName } from "./provider.js";
 import { buildFixPrompt, buildReviewPrompt, buildRevalidatePrompt } from "./prompt.js";
 import {
+  claimFeature,
+  clearFeatureLockFiles,
   ensureStateDirs,
   readFeatures,
+  readFeatureLockIds,
   readFinding,
   readFindings,
   readPatchAttempts,
@@ -25,6 +28,7 @@ import {
   writePatchAttempt,
   writeProject,
   writeRun,
+  releaseFeatureLock,
 } from "./state.js";
 import {
   CommandResult,
@@ -113,20 +117,28 @@ export async function mapCommand(
 
 export async function statusCommand(context: AppContext): Promise<unknown> {
   const loaded = await loadProjectState(context);
-  const [features, findings, runs, git] = await Promise.all([
+  const [features, findings, runs, git, lockFileIds] = await Promise.all([
     readFeatures(loaded.paths),
     readFindings(loaded.paths),
     readRuns(loaded.paths),
     discoverGit(loaded.root),
+    readFeatureLockIds(loaded.paths),
   ]);
+  const activeLockIds = new Set(
+    features.flatMap((feature) => (feature.lock === null ? [] : [feature.featureId])),
+  );
+  for (const id of lockFileIds) {
+    activeLockIds.add(id);
+  }
   return {
     project: loaded.project.name,
     branch: git.currentBranch,
     dirty: git.dirty,
     features: features.length,
     findings: findings.length,
     openFindings: findings.filter((finding) => finding.status === "open").length,
-    activeLocks: features.filter((feature) => feature.lock !== null).length,
+    activeLocks: activeLockIds.size,
+    lockFiles: lockFileIds.length,
     lastRun: runs.at(-1)?.runId ?? null,
   };
 }
@@ -183,6 +195,7 @@ export async function reviewCommand(
             currentRunId,
             index,
             total: features.length,
+            allowNonPendingFeatureReview: stringFlag(flags, "feature") !== undefined,
           });
           findingIds.push(...reviewed.findingIds);
         } catch (error: unknown) {
@@ -389,10 +402,21 @@ type ReviewFeatureOptions = {
   currentRunId: string;
   index: number;
   total: number;
+  allowNonPendingFeatureReview: boolean;
 };
 
 async function reviewFeature(options: ReviewFeatureOptions): Promise<{ findingIds: string[] }> {
-  const { context, loaded, config, provider, feature, currentRunId, index, total } = options;
+  const {
+    context,
+    loaded,
+    config,
+    provider,
+    feature,
+    currentRunId,
+    index,
+    total,
+    allowNonPendingFeatureReview,
+  } = options;
   const started = Date.now();
   let locked: FeatureRecord | null = null;
   emitReviewProgress(context, "feature-start", {
@@ -402,9 +426,15 @@ async function reviewFeature(options: ReviewFeatureOptions): Promise<{ findingId
     title: feature.title,
   });
   try {
-    const lockedFeature = lockFeature(feature, currentRunId);
+    const lockedFeature = await claimFeature(
+      loaded.paths,
+      feature.featureId,
+      featureLock(currentRunId),
+      {
+        allowNonPending: allowNonPendingFeatureReview,
+      },
+    );
     locked = lockedFeature;
-    await writeFeature(loaded.paths, lockedFeature);
     const prompt = await buildReviewPrompt(loaded.root, loaded.project, lockedFeature, config);
     const output = await provider.review(loaded.root, prompt, config.provider.model);
     const records = output.findings
@@ -438,6 +468,8 @@ async function reviewFeature(options: ReviewFeatureOptions): Promise<{ findingId
       updatedAt: nowIso(),
     };
     await writeFeature(loaded.paths, updated);
+    await releaseFeatureLock(loaded.paths, lockedFeature.featureId);
+    locked = null;
     emitReviewProgress(context, "feature-done", {
       index: index + 1,
       total,
@@ -449,23 +481,27 @@ async function reviewFeature(options: ReviewFeatureOptions): Promise<{ findingId
   } catch (error: unknown) {
     const message = error instanceof Error ? error.message : String(error);
     if (locked !== null) {
-      await writeFeature(loaded.paths, {
-        ...locked,
-        status: "error",
-        lock: null,
-        analysisHistory: [
-          ...locked.analysisHistory,
-          {
-            runId: currentRunId,
-            kind: "review-error",
-            summary: message,
-            provider: provider.name,
-            model: config.provider.model,
-            createdAt: nowIso(),
-          },
-        ],
-        updatedAt: nowIso(),
-      });
+      try {
+        await writeFeature(loaded.paths, {
+          ...locked,
+          status: "error",
+          lock: null,
+          analysisHistory: [
+            ...locked.analysisHistory,
+            {
+              runId: currentRunId,
+              kind: "review-error",
+              summary: message,
+              provider: provider.name,
+              model: config.provider.model,
+              createdAt: nowIso(),
+            },
+          ],
+          updatedAt: nowIso(),
+        });
+      } finally {
+        await releaseFeatureLock(loaded.paths, locked.featureId);
+      }
     }
     emitReviewProgress(context, "feature-error", {
       index: index + 1,
@@ -772,7 +808,8 @@ export async function cleanLocksCommand(context: AppContext): Promise<unknown> {
     });
     cleared += 1;
   }
-  return { cleared };
+  const lockFilesCleared = await clearFeatureLockFiles(loaded.paths);
+  return { cleared, lockFilesCleared };
 }
 
 async function loadProjectState(context: AppContext) {
@@ -1119,20 +1156,12 @@ function emitRevalidateProgress(
   process.stderr.write(`clawpatch revalidate ${event}${values.length > 0 ? ` ${values}` : ""}\n`);
 }
 
-function lockFeature(feature: FeatureRecord, currentRunId: string): FeatureRecord {
-  if (feature.lock !== null) {
-    throw new ClawpatchError(`feature locked: ${feature.featureId}`, 7, "lock-conflict");
-  }
+function featureLock(currentRunId: string): NonNullable<FeatureRecord["lock"]> {
   return {
-    ...feature,
-    status: "claimed",
-    lock: {
-      lockedByRunId: currentRunId,
-      lockedAt: nowIso(),
-      hostname: hostname(),
-      pid: process.pid,
-    },
-    updatedAt: nowIso(),
+    lockedByRunId: currentRunId,
+    lockedAt: nowIso(),
+    hostname: hostname(),
+    pid: process.pid,
   };
 }
 

src/fs.ts

@@ -1,3 +1,4 @@
+import { randomUUID } from "node:crypto";
 import { access, mkdir, readFile, rename, writeFile } from "node:fs/promises";
 import { dirname } from "node:path";
 import { z } from "zod";
@@ -23,7 +24,7 @@ export async function readJson<T>(path: string, schema: z.ZodType<T>): Promise<T
 
 export async function writeJson(path: string, value: unknown): Promise<void> {
   await ensureDir(dirname(path));
-  const tmp = `${path}.tmp-${process.pid}-${Date.now()}`;
+  const tmp = `${path}.tmp-${process.pid}-${Date.now()}-${randomUUID()}`;
   await writeFile(tmp, `${JSON.stringify(value, null, 2)}\n`, "utf8");
   await rename(tmp, path);
 }

src/state.ts

@@ -1,7 +1,8 @@
-import { readdir } from "node:fs/promises";
+import { open, readdir, unlink } from "node:fs/promises";
 import { join } from "node:path";
 import { z } from "zod";
-import { ensureDir, pathExists, readJson, writeJson } from "./fs.js";
+import { ClawpatchError } from "./errors.js";
+import { ensureDir, nowIso, pathExists, readJson, writeJson } from "./fs.js";
 import {
   FeatureRecord,
   FindingRecord,
@@ -68,8 +69,94 @@ export async function readFeatures(paths: StatePaths): Promise<FeatureRecord[]>
   return readRecords(paths.features, featureRecordSchema);
 }
 
+export async function readFeature(paths: StatePaths, id: string): Promise<FeatureRecord | null> {
+  const path = featurePath(paths, id);
+  if (!(await pathExists(path))) {
+    return null;
+  }
+  return readJson(path, featureRecordSchema);
+}
+
 export async function writeFeature(paths: StatePaths, feature: FeatureRecord): Promise<void> {
-  await writeJson(join(paths.features, `${feature.featureId}.json`), feature);
+  await writeJson(featurePath(paths, feature.featureId), feature);
+}
+
+export async function claimFeature(
+  paths: StatePaths,
+  featureId: string,
+  lock: NonNullable<FeatureRecord["lock"]>,
+  options: { allowNonPending?: boolean } = {},
+): Promise<FeatureRecord> {
+  await ensureDir(paths.locks);
+  const lockPath = featureLockPath(paths, featureId);
+  let handle;
+  try {
+    handle = await open(lockPath, "wx");
+    await handle.writeFile(`${JSON.stringify(lock, null, 2)}\n`, "utf8");
+  } catch (error: unknown) {
+    if (isNodeError(error, "EEXIST")) {
+      throw new ClawpatchError(`feature locked: ${featureId}`, 7, "lock-conflict");
+    }
+    if (handle !== undefined) {
+      await handle.close();
+      handle = undefined;
+      await releaseFeatureLock(paths, featureId);
+    }
+    throw error;
+  } finally {
+    await handle?.close();
+  }
+
+  try {
+    const feature = await readFeature(paths, featureId);
+    if (feature === null) {
+      throw new ClawpatchError(`feature not found: ${featureId}`, 2, "feature-not-found");
+    }
+    if (feature.lock !== null) {
+      throw new ClawpatchError(`feature locked: ${featureId}`, 7, "lock-conflict");
+    }
+    if (options.allowNonPending !== true && !["pending", "error"].includes(feature.status)) {
+      throw new ClawpatchError(`feature not reviewable: ${featureId}`, 7, "lock-conflict");
+    }
+    const claimed: FeatureRecord = {
+      ...feature,
+      status: "claimed",
+      lock,
+      updatedAt: nowIso(),
+    };
+    await writeFeature(paths, claimed);
+    return claimed;
+  } catch (error: unknown) {
+    await releaseFeatureLock(paths, featureId);
+    throw error;
+  }
+}
+
+export async function releaseFeatureLock(paths: StatePaths, featureId: string): Promise<void> {
+  await unlink(featureLockPath(paths, featureId)).catch((error: unknown) => {
+    if (!isNodeError(error, "ENOENT")) {
+      throw error;
+    }
+  });
+}
+
+export async function clearFeatureLockFiles(paths: StatePaths): Promise<number> {
+  const lockIds = await readFeatureLockIds(paths);
+  for (const id of lockIds) {
+    await releaseFeatureLock(paths, id);
+  }
+  return lockIds.length;
+}
+
+export async function readFeatureLockIds(paths: StatePaths): Promise<string[]> {
+  if (!(await pathExists(paths.locks))) {
+    return [];
+  }
+  const names = await readdir(paths.locks);
+  return names
+    .filter((name) => name.endsWith(".json"))
+    .map((name) => name.slice(0, -".json".length))
+    .toSorted();
 }
 
 export async function readFindings(paths: StatePaths): Promise<FindingRecord[]> {
@@ -118,3 +205,15 @@ async function readRecords<T>(dir: string, schema: z.ZodType<T>): Promise<T[]> {
   }
   return records;
 }
+
+function featurePath(paths: StatePaths, featureId: string): string {
+  return join(paths.features, `${featureId}.json`);
+}
+
+function featureLockPath(paths: StatePaths, featureId: string): string {
+  return join(paths.locks, `${featureId}.json`);
+}
+
+function isNodeError(error: unknown, code: string): error is NodeJS.ErrnoException {
+  return error instanceof Error && "code" in error && error.code === code;
+}