Review: Approved

This PR addresses three review findings well with proper domain-driven design patterns.

No Blocking Issues

Analysis

Domain-Driven Design:

• The forceRelease(expectedNonce) method is properly added to the DistributedLock domain interface, making the contract explicit
• Each infrastructure implementation (S3Lock, FileLock) encapsulates backend-specific atomicity details
• Good separation of concerns: CLI delegates to domain interface without knowing implementation details

Code Quality:

• Well-typed with proper interface usage
• Named exports used correctly
• All files have AGPLv3 headers
• No new any types introduced

Test Coverage:

• 6 new tests for forceRelease covering: matching nonce, mismatched nonce, no lock exists
• Test updated for resolveDatastoreForRepo to verify marker is returned

Security:

• TOCTOU window is properly minimized by combining nonce verification and deletion in a single operation
• Method is appropriately documented as a "breakglass" operation with clear limitations

Suggestions (non-blocking)

1. datastore_lock.ts:102 - The info.nonce! non-null assertion could theoretically throw if a legacy lock exists without a nonce. While unlikely in practice (all modern locks include a nonce), consider handling this case:

   if (!info.nonce) {
     renderDatastoreLockRelease(
       { released: false, reason: "lock has no nonce — cannot verify ownership" },
       ctx.outputMode,
     );
     return;
   }
   const released = await lock.forceRelease(info.nonce);

   This is minor since all current lock implementations always include a nonce.

Adversarial Review

Critical / High

None found. The code changes are sound.

Medium

1. s3_lock.ts:168-175 — S3Lock.forceRelease doesn't handle S3 errors

   Unlike release() (lines 148-152), the new forceRelease() method doesn't wrap the S3 delete in try-catch:

   async forceRelease(expectedNonce: string): Promise<boolean> {
     const current = await this.readLock();
     if (!current || current.nonce !== expectedNonce) {
       return false;
     }
     await this.s3.deleteObject(this.lockKey);  // Can throw
     return true;
   }

   Breaking example: swamp datastore lock release --force during an S3 outage or with expired AWS credentials results in a raw AWS SDK exception (e.g., CredentialsProviderError, NetworkError) rather than a user-friendly error message.

   Comparison: release() explicitly catches and ignores errors because "if S3 is unreachable, the lock expires via TTL." For a breakglass operation, you might want errors to surface, but the current code gives poor UX when they do.

   Suggested fix: Either wrap in try-catch with a descriptive error, or document that raw errors are intentional for breakglass visibility.

Low

1. datastore_lock.ts:102 — Non-null assertion on optional field

   info.nonce! where LockInfo.nonce is typed as string | undefined. If a legacy lock exists without a nonce (created before this feature), undefined would be passed to forceRelease(). The comparison works correctly at runtime (undefined !== undefined → false), so no functional bug, but the ! assertion obscures the type contract.

2. Inconsistent error handling between FileLock and S3Lock forceRelease

   FileLock (lines 176-180) catches NotFound and ignores it; S3Lock catches nothing. Minor inconsistency—S3 DeleteObject on a non-existent key succeeds silently anyway, but the pattern differs.

3. TOCTOU window remains (documented)

   The read-then-delete pattern in both forceRelease implementations still has a theoretical race window where another process could acquire between verify and delete. This is explicitly documented in the interface and acceptable for a breakglass operation.

Verdict

PASS — No blocking issues. The TOCTOU improvement over the previous inline logic is meaningful, the double-read optimization is correct, and the extension_search non-TTY fix follows the established pattern. The S3 error handling inconsistency (Medium #1) is worth addressing but not a blocker for a breakglass operation that operators would investigate manually anyway.