[mabry1985]

Problem

Upstream's stale-lock reclaim (openclaw#170, in 0.8.1) clears a lock only when it was written on the same host and its pid is dead. In a container every recreate changes the hostname, so a review killed by a redeploy (or by a supervisor's SIGKILL in a previous container) leaves a lock that nothing ever reclaims. Every later run that needs that feature fails with feature locked (exit 7).

Seen on Vera 2026-09-29: run 20260929T223224-122ad9 was killed by a container recreate at ~22:32Z. Its lock on feat_library_3fc6c136cd (hostname 323247be3da5; the container is now fd5751254088) made the structural pass fail with exit 7 on protoAgent #3901 and #3918 (twice) until the lock was moved aside by hand. (Vera still runs 0.6.4, which has no reclaim at all; this change is what makes the 0.8.x reclaim cover containers.)

Fix

isStaleLocalFeatureLock:

• Same host: unchanged. The pid decides, and a live run keeps its lock however old.
• Another host: stale once lockedAt is older than CLAWPATCH_LOCK_STALE_MS (default 2 h). A review is bounded by the provider timeout (minutes), so no live run can still hold a lock that old. A lock with an unparseable lockedAt is never reclaimed by age.

FeatureLockReclaimOptions gains maxForeignLockAgeMs and now for tests.

Also (from review): an empty or partially-written lock file (a run killed between creating and writing it) is reclaimed once its mtime is older than 60 s (unreadableLockGraceMs); before, recovery read it as "no lock" and gave up, so the feature stayed locked forever. A fresh unreadable file still conflicts.

Tests

• reclaims an empty lock file left by a crash, but not a fresh one — fails on the previous head.

• reclaims a remote-host lock once it is older than the foreign-lock age: reproduces the Vera case end to end through claimFeature.

• decides lock staleness by pid on the same host and by age on another host: unit table.

• Both fail on origin/main and pass with the fix. The existing keeps live local and remote feature locks claimed (a fresh remote lock stays claimed) still passes.

Gates: typecheck, lint, format:check, test (1025 passed, 2 skipped), build, pack:smoke — all green.

Release

No version bump: since 0.8.1 the fork's version tracks upstream. The CHANGELOG entry is under ## Unreleased; picking the release number and deploying to Vera are separate steps.

🤖 Generated with Claude Code

[protoreview]

QA panel review — WARN

code-review · head 6e851344dac4 · formal

⚠️ PR advanced 1 commit(s) during this round (6e851344dac4 → c384dcac6851); 0 finding(s) in the delta were demoted to possibly addressed. This verdict is posted as a non-blocking comment against the superseded head; the current head is queued for its own review.

⚠️ Coverage incomplete — this is not a clean pass. 1 of 4 review lane(s) did not complete a full pass this round: verify (returned no findings array and no status line — the verify pass did not run). Findings from the lanes that ran stand, but a defect only the missing lanes would catch may be missed, so a PASS is capped at WARN. Where the brief below implies full coverage, this line supersedes it. The next push re-runs the full panel.

The verifier lane for PR #18 ended without producing a structured deliverable, so no verdict-annotated findings list was available to synthesize. The verifier's informal analysis (visible in its reasoning) repeatedly concluded the PR is clean: the maxForeignLockAgeMs helper correctly handles edge cases (unparseable lockedAt, non-numeric env var, zero/negative values), the tests cover same-host/foreign-host/unparseable paths, and the CHANGELOG matches the code. No other finder lists were provided to this pass. The one thing to fix first: nothing material was identified. The panel disagreed on nothing because no structured findings existed to disagree over. Coverage gap: the verify lane is a Gap, so no independent confirmation of the finders' claims was obtained; if any findings existed upstream, they remain unverified.

No findings from the lanes that ran — coverage was incomplete (see above), so this is not a clean review.

findings JSON (machine-readable)

[]

[protoreview]

QA panel review — WARN

code-review · head c384dcac6851 · formal

Low-risk PR: a feature-lock staleness check and a changelog entry. No correctness or cross-file defects surfaced. The one thing to fix first is the test placement — the pure unit test for isStaleLocalFeatureLock belongs in state.test.ts beside its implementation, not in workflow.test.ts. The changelog heading ambiguity is a nit; the author can fold the new entry into the existing 0.8.1 - Unreleased section or rename the new heading to disambiguate. The panel was unanimous; verification confirmed both findings without change.

Findings

	Severity	Location	Finding	Verified
🟡	minor	src/workflow.test.ts:1995	The pure unit test of isStaleLocalFeatureLock (a function defined in src/state.ts) is added to workflow.test.ts instead of the existing state.test.ts, violatin…	confirmed
⚪	nit	CHANGELOG.md:15	The PR inserts a new top-level '## Unreleased (protoLabs fork)' section directly above the pre-existing '## 0.8.1 - Unreleased (protoLabs fork)' section, leavi…	confirmed

findings JSON (machine-readable)

[
  {
    "file": "src/workflow.test.ts",
    "line": 1995,
    "severity": "minor",
    "category": "conventions",
    "claim": "The pure unit test of isStaleLocalFeatureLock (a function defined in src/state.ts) is added to workflow.test.ts instead of the existing state.test.ts, violating the repo rule that tests sit beside their implementation file as <module>.test.ts.",
    "evidence": "it(\"decides lock staleness by pid on the same host and by age on another host\", () => {\n    const now = Date.parse(\"2026-09-29T23:00:00Z\");\n    const at = (minutesAgo: number) => new Date(now - minutesAgo * 60_000).toISOString();\n    const opts = (alive: boolean) => ({\n      hostname: \"here\",\n      isPidAlive: () => alive,\n    });",
    "verdict": "confirmed",
    "note": "Diff shows the pure unit test of isStaleLocalFeatureLock (defined in state.ts) added to workflow.test.ts; state.test.ts exists on main (15 lines) and is the natural home."
  },
  {
    "file": "CHANGELOG.md",
    "line": 15,
    "severity": "nit",
    "category": "conventions",
    "claim": "The PR inserts a new top-level '## Unreleased (protoLabs fork)' section directly above the pre-existing '## 0.8.1 - Unreleased (protoLabs fork)' section, leaving two sibling 'Unreleased' headings and making it ambiguous which is the current in-flight work.",
    "evidence": "## Unreleased (protoLabs fork)\n\n- **state**: a feature lock written on another host is reclaimed once it is older\n  than `CLAWPATCH_LOCK_STALE_MS` (default 2 h).",
    "verdict": "confirmed",
    "note": "Diff shows new '## Unreleased (protoLabs fork)' inserted directly above existing '## 0.8.1 - Unreleased (protoLabs fork)' \u2014 two sibling ## headings both containing 'Unreleased'."
  }
]

[protoreview]

QA panel review — WARN

code-review-structural · head 565882f9819f · formal

Low-risk, purely additive change to isStaleLocalFeatureLock adding a foreign-host age-based reclamation path. No blockers or majors. The single confirmed minor (a pre-existing race between open("wx") and writeFile that can leave an empty lock file the reclaim logic cannot clear) is advisory — it predates this PR and is not a regression. The verifier confirmed all three mechanisms at head SHA; no panel disagreement, no coverage gaps.

Findings

	Severity	Location	Finding	Verified
🟡	minor	src/state.ts:141	Empty or partially-written lock file causes a permanent feature lock that stale-lock recovery cannot clear (flagged by structural review; applies to both recla…	confirmed

findings JSON (machine-readable)

[
  {
    "file": "src/state.ts",
    "line": 141,
    "severity": "minor",
    "category": "bug",
    "claim": "Empty or partially-written lock file causes a permanent feature lock that stale-lock recovery cannot clear (flagged by structural review; applies to both reclaimStaleFeatureLock and clearStaleFeatureLockUnderMutationLock)",
    "evidence": "open(lockPath, \"wx\") creates an empty file atomically; the payload is written by a separate writeFile call. If the process is killed (SIGKILL, OOM) between these two operations, the file exists on disk with zero or partial content. readFeatureLockFile catches all parse errors and returns null, so the empty file is treated as 'no lock'. In reclaimStaleFeatureLock, both featureLock and fileLock are null, so it returns false and the file is never deleted \u2014 a subsequent claimant hits EEXIST forever. Fix: in both reclaimStaleFeatureLock and clearStaleFeatureLockUnderMutationLock, add a pathExists check on the lock file; if the file exists but readFeatureLockFile returned null, treat it as stale and delete it.",
    "source": "protopatch",
    "verdict": "confirmed",
    "note": "All three mechanisms verified in src/state.ts at head SHA: (1) open(\"wx\") + writeFile are two separate calls with a kill window; (2) readFeatureLockFile's catch-all returns null for unparseable/empty content; (3) reclaimStaleFeatureLock returns false when both are null, never unlinking the file. Pre-existing race, not introduced by this PR."
  }
]

[protoreview]

Promoting the WARN verdict for head 565882f9819f: all checks terminal-green, zero unresolved review threads. (approve-on-green)

Open findings carried by this approval — non-blocking, but they did not go away:

• minor src/state.ts:141 — Empty or partially-written lock file causes a permanent feature lock that stale-lock recovery cannot clear (flagged by structural review; applies to both reclaimStaleFeatureLock and clearStaleFeatureL

Approving a WARN does not resolve its findings (issue #22).

[protoreview]

QA panel review — WARN

code-review-structural · head d32295efa481 · formal

The panel's brief could not be read from this round's report (no delimited brief block). The findings below are unaffected.

Findings

	Severity	Location	Finding	Verified
🟡	minor	src/state.ts:140	claimFeature, releaseFeature, reclaimStaleFeatureLock, and clearStaleFeatureLocks lack dedicated unit tests. The PR adds a test for isStaleLocalFeatureLock in …	confirmed
⚪	nit	src/state.ts:440	The exported function isStaleLocalFeatureLock now decides staleness for both same-host (PID) and cross-host (age) locks, making the 'Local' qualifier a misnome…	confirmed

findings JSON (machine-readable)

[
  {
    "file": "src/state.ts",
    "line": 140,
    "severity": "minor",
    "category": "test-gap",
    "claim": "claimFeature, releaseFeature, reclaimStaleFeatureLock, and clearStaleFeatureLocks lack dedicated unit tests. The PR adds a test for isStaleLocalFeatureLock in state.test.ts, but the multi-step lock lifecycle (exclusive creation under mutation guard, PID-liveness reclaim, foreign-host age reclaim, orphaned-file cleanup, and the clearStaleFeatureLocks batch path) is untested. Flagged by protopatch; re-scoped from the engine's blanket 'no tests exist' claim, which is partially stale given the new test file.",
    "evidence": "state.test.ts contains a describe block for isStaleLocalFeatureLock only. No test exercises claimFeature (lock-conflict throw, successful claim), releaseFeature, reclaimStaleFeatureLock (same-host PID path, foreign-host age path, orphan path), or clearStaleFeatureLocks.",
    "source": "protopatch",
    "verdict": "confirmed",
    "note": "state.test.ts at head has only 'state record paths' and 'isStaleLocalFeatureLock' describe blocks; no dedicated unit tests for the four named functions. Integration tests in workflow.test.ts cover some paths (foreign-host age, orphan) via claimFeature, but the claim is specifically about dedicated unit tests, which are absent."
  },
  {
    "file": "src/state.ts",
    "line": 440,
    "severity": "nit",
    "category": "conventions",
    "claim": "The exported function isStaleLocalFeatureLock now decides staleness for both same-host (PID) and cross-host (age) locks, making the 'Local' qualifier a misnomer. A reader encountering this function in isolation may assume it only handles same-host locks and miss the foreign-host age path.",
    "evidence": "export function isStaleLocalFeatureLock(lock, options) { if (lock.hostname === currentHostname) { return !isPidAlive(lock.pid); } const lockedAt = Date.parse(lock.lockedAt); ... return now - lockedAt > maxForeignLockAgeMs(options); }",
    "verdict": "confirmed",
    "note": "Code at head (lines 440-456) confirms the function branches on hostname: same-host \u2192 PID check, cross-host \u2192 age check via maxForeignLockAgeMs. The 'Local' qualifier is a misnomer. Evidence is a loose paraphrase (omits Number.isFinite guard) but substance is verbatim in the file."
  }
]