fix(work-items): github adapter lease-coordination robustness (#370)

[kyle-sexton]

Summary

Follow-up from #224. Closes two lease-coordination correctness gaps in the GitHub adapter's renew-lease.sh and reclaim.sh — both in code relocated byte-for-byte from medley by #224 (not changed there), handled as the single hardening pass the issue calls for. No new adapter capability; a bug fix, so a patch bump (0.14.2 → 0.14.3). This is the GitHub-adapter sibling of #367 (local-markdown), shipped earlier this session.

Fix

Finding 1 (P1) — renew-lease revived an expired lease. renew-lease selected the active (newest non-superseded) lease, confirmed active_id == lease_comment_id, and PATCHed a fresh renewed_at — but never checked liveness first. A worker retaining its handle past renewed_at + ttl_hours, with no newer lease comment, still matched active_id, so the PATCH revived an expired lease and defeated TTL-based handoff: a crashed/delayed worker could reclaim an item another worker had reasonably treated as expired.

• renew-lease.sh now checks wit_lease_is_live immediately before patching and returns a conflict (exit 7) for an expired-but-not-superseded active lease instead of reviving it. Recovery from an expired lease is a fresh claim/reclaim, not a revive of the dead handle.

Finding 2 (P1) — reclaim removed assignees that did not hold the expired lease. On the expired-lease + no-activity path, reclaim read every assignee ([.assignees[].login]) and removed each one — not just the lease's holder. A user manually assigned after the old lease, or a concurrent claimer added before this assignee snapshot, was silently unassigned; in the concurrent case that claimer's live lease stayed while the frontier treated the item as unassigned (two workers on one item).

• reclaim.sh now removes only the expired lease's holder (guarded on a fresh assignee read so an already-unassigned holder is a no-op), leaving co-assignees untouched.
• Ownership is revalidated immediately before mutating: the lease was selected before the two activity round-trips, a TOCTOU window in which a concurrent claimer can renew or supersede it. reclaim re-reads the leases and confirms this is still the active and still expired lease; if a concurrent claim won the item in that window, reclaim is a no-op (reclaimed: false) — never a mutation that strips the new owner.

Both verbs now share wit_select_active_lease (extracted to lib/lease.sh) instead of a third copy of the active-lease selection loop. CONTRACT.md's "Lease protocol" documents both semantics (renew refuses an expired lease; reclaim is holder-only and revalidated).

Verification

New offline test adapters/github/lease-coordination.test.sh drives both fixes against a stubbed gh (the GitHub-adapter conformance binding requires a live target, so these decision-path cases live in a dedicated test — mirroring the sibling's claim-integrity.test.sh precedent). It asserts on the recorded write/remove call-log, not just exit codes.

Against the fix — bash adapters/github/lease-coordination.test.sh, exit 0 (cases 14–17 cover the concurrent in-place-renew revalidation branch, added per review):

PASS: [1] renew-lease returns conflict (7) for an expired active lease
PASS: [2] renew-lease does NOT revive the expired lease (no PATCH)
PASS: [3] renew-lease succeeds (0) for a live active lease
PASS: [4] renew-lease emits the renewed lease record
PASS: [5] renew-lease PATCHes the live lease comment
PASS: [6] reclaim succeeds (0)
PASS: [7] reclaim reports reclaimed:true
PASS: [8] reclaim removes the expired lease holder (alice)
PASS: [9] reclaim leaves the co-assignee (bob) untouched
PASS: [10] reclaim succeeds (0) on a concurrent-claim revalidation
PASS: [11] reclaim reports reclaimed:false when the lease changed under it
PASS: [12] reclaim removes NO assignee when revalidation fails
PASS: [13] reclaim does not supersede when revalidation fails
PASS: [14] reclaim succeeds (0) when the lease was renewed in place during the window
PASS: [15] reclaim reports reclaimed:false when the lease was renewed in place
PASS: [16] reclaim removes NO assignee when the lease was renewed in place
PASS: [17] reclaim does not supersede when the lease was renewed in place

Discrimination proof — the (13-case) test against the pre-fix renew-lease.sh/reclaim.sh at origin/main (exit 1), confirming the cases fail without the fix and are not passing for the wrong reason:

FAIL: [1] renew-lease returns conflict (7) for an expired active lease — expected 7 got 0
FAIL: [2] renew-lease does NOT revive the expired lease (no PATCH) — expected no PATCH logged got PATCH logged
PASS: [3] renew-lease succeeds (0) for a live active lease
PASS: [4] renew-lease emits the renewed lease record
PASS: [5] renew-lease PATCHes the live lease comment
PASS: [6] reclaim succeeds (0)
PASS: [7] reclaim reports reclaimed:true
PASS: [8] reclaim removes the expired lease holder (alice)
FAIL: [9] reclaim leaves the co-assignee (bob) untouched — forbidden REMOVE_ASSIGNEE bob present
PASS: [10] reclaim succeeds (0) on a concurrent-claim revalidation
FAIL: [11] reclaim reports reclaimed:false when the lease changed under it — expected false got true
FAIL: [12] reclaim removes NO assignee when revalidation fails — forbidden REMOVE_ASSIGNEE present
FAIL: [13] reclaim does not supersede when revalidation fails — forbidden PATCH present

Direct unit coverage of the extracted helper added to lib/lease.test.sh (newest-non-superseded selection, superseded back-off not masking an earlier active lease, empty out-params when none) — 15/15 pass. Full plugins/work-items/**/*.test.sh sweep green; ShellCheck clean (--rcfile .shellcheckrc) on every changed script; changed files carry no trailing whitespace or tabs.

Closes #370

Related

• work-items github adapter: lease-coordination robustness (renew revives an expired lease; reclaim removes non-holder assignees) #370 — this issue.
• feat(work-items): bundle work-item-tracker seam in plugin (shape A) #224 — the bundle-seam PR (shape A) that relocated this code byte-for-byte; both findings were deferred there by author delegation.
• work-items local-markdown claim.sh: lease/assignee integrity (expired lease never clears assignee; failed assignee write reports success) #367 — the sibling local-markdown lease/assignee-integrity fix shipped earlier this session (same lease-integrity class, different adapter).

🤖 Generated with a Claude Code implementation subagent (issue #370)

[claude]

Claude finished @kyle-sexton's task in 3m 8s —— View job

---

PR Review: fix(work-items): github adapter lease-coordination robustness (#370)

Tasks:

• Gather context and understand request
• Read changed files and diff
• Review renew-lease.sh changes
• Review reclaim.sh changes
• Review lib/lease.sh extraction
• Review test coverage (lease-coordination.test.sh, lease.test.sh)
• Review CONTRACT.md and plugin.json changes

---

Summary

Both fixes are correct. The TOCTOU revalidation in reclaim and the liveness gate in renew-lease close the two P1 gaps precisely. Test discrimination proofs are strong, and the wit_select_active_lease extraction removes duplication cleanly. A few minor points below, none blocking.

---

Finding 1 — Minor consistency: reclaim uses $lease_json (original) instead of $WIT_ACTIVE_LEASE_JSON (revalidated) for mutation

reclaim.sh lines 82–90

After the revalidation block confirms WIT_ACTIVE_LEASE_ID == lease_comment_id and that the lease is still expired, the code uses $lease_json (captured before the activity round-trips) for two purposes:

holder="$(jq -r '.holder' <<<"$lease_json")"
...
superseded="$(jq -c --arg ts "$now" '. + {superseded_at: $ts}' <<<"$lease_json")"

This is functionally safe — holder is immutable in the lease lifecycle (only renewed_at/superseded_at change), and the ID check guarantees we're operating on the same comment. But a future reader may wonder why $WIT_ACTIVE_LEASE_JSON isn't used consistently after revalidation. A small comment or using $WIT_ACTIVE_LEASE_JSON for superseded construction would make the intent clearer.

---

Finding 2 — Minor: $now timestamp in superseded_at predates the revalidation round-trip

reclaim.sh line 54 / lines 88–92

now is captured once before the two activity API calls and the revalidation re-read. If those round-trips are slow (network latency), the superseded_at written to the lease comment could be several seconds in the past relative to the actual PATCH. This is cosmetically stale metadata — superseded_at only needs to be truthy to mark "not live" — but noting it for completeness. Taking a fresh timestamp after revalidation would make the record more accurate.

---

Finding 3 — Positive: Correct fresh epoch for revalidation liveness check

reclaim.sh line 73

if wit_lease_is_live "$WIT_ACTIVE_LEASE_JSON" "$(date -u +%s)"; then

The revalidation liveness check correctly uses a fresh $(date -u +%s) rather than the stale now_epoch from before the activity round-trips. This correctly catches the case where a concurrent claimer renewed the lease during the activity window, making it live by the time reclaim re-reads it.

---

Finding 4 — Test stub: dispatch ordering is correct but implicit

lease-coordination.test.sh lines 43–51

The stub dispatches *"/timeline"* before *"--paginate"*, so the timeline call (which also includes --paginate) correctly hits the timeline branch rather than the lease-list branch. This is correct, but the correctness depends on case-statement ordering. Both branches are in the same case block without an explicit comment — a future editor adding a case above *"/timeline"* that catches --paginate broadly could break the dispatch silently. A short comment like # /timeline before --paginate: timeline calls carry both would preserve intent.

---

Finding 5 — Test design: TOCTOU simulation via count file is well-constructed

lease-coordination.test.sh lines 48–51

n=$(( $(cat "$d/lease-comments.count" 2>/dev/null || printf '0') + 1 ))
printf '%s\n' "$n" >"$d/lease-comments.count"
if [[ -f "$d/lease-comments-$n" ]]; then cat "$d/lease-comments-$n"; else cat "$d/lease-comments"; fi

The counter-file approach correctly models the TOCTOU window: the first paginate read returns lease-comments-1 (expired lease) and the second returns lease-comments-2 (concurrent superseding claim). The fallback to lease-comments for scenarios with no lease-comments-1 is clean — non-revalidation tests work without the counter. Good design.

---

Finding 6 — wit_select_active_lease global state: reset at call entry

lib/lease.sh lines 43–44

WIT_ACTIVE_LEASE_ID=""
WIT_ACTIVE_LEASE_JSON=""

The output vars are cleared at function entry before the loop, so a "no active lease found" result leaves both empty rather than inheriting stale values from a prior call. This is necessary correctness for the [[ -n "$lease_json" ]] || emit false "no active lease record" guard in reclaim.

---

CONTRACT.md

The new "Lease protocol" prose correctly captures both semantics:

• renew-lease refuses an expired-but-active lease (exit 7, no revive)
• reclaim is holder-only and revalidated

Both are consistent with the implementation. The "Reclaim" paragraph is now the authoritative spec for the TOCTOU no-op behavior.

---

Verdict

Approve — the fixes are correct, the tests are rigorous with strong discrimination proofs (pre-fix failures), and the extraction of wit_select_active_lease is a clean deduplication. Findings 1 and 2 are cosmetic and non-blocking. Finding 4 is a maintainability note.

[claude]

Claude finished @kyle-sexton's task in 2m 4s —— View job

---

PR Review: fix(work-items): github adapter lease-coordination robustness (#370)

Tasks:

• Gather context and understand request
• Read PR diff and changed files
• Review renew-lease.sh changes
• Review reclaim.sh changes
• Review lib/lease.sh extraction
• Review test coverage
• Review CONTRACT.md and plugin.json changes
• Post final review

---

Summary

This PR carries two commits. The primary commit (9eefe11) contains the lease-coordination fix; the follow-up commit (5f24617) is a hygiene pass that satisfies CI gates (exec bit on the test file, a markdownlint MD004 fix in CHANGELOG, and a comment-reference rule fix in the test header). Both commits are clean.

The prior review in this thread gave an Approve with four minor/cosmetic findings. All four remain non-blocking and none were expected to be addressed. I've re-examined the code independently below.

---

Correctness — renew-lease.sh

renew-lease.sh lines 64–74

The liveness gate is correctly placed after the active-lease identity check, not before. Order matters: checking liveness before identity would allow a live-but-stale session to prove its handle is current and then discover it isn't — this path is correct. The date -u +%s fresh epoch avoids the stale-$now problem that the later supersede write does have (minor, noted by prior review).

The gate uses wit_lease_is_live which checks both superseded_at (empty → not yet superseded) and renewed_at + ttl_hours. A lease can only reach this point without superseded_at — the earlier check at line 47 already exits on a superseded lease — so the wit_lease_is_live call effectively tests TTL expiry only here. That's correct.

---

Correctness — reclaim.sh

reclaim.sh lines 62–86

Revalidation: Lines 68–75 re-read leases and confirm WIT_ACTIVE_LEASE_ID == lease_comment_id and the lease is still expired (fresh epoch at line 73). Both checks are necessary and correctly ordered. If the active lease ID changed (concurrent supersede) the function emits reclaimed:false without any mutation. If the ID matches but the lease is now live (concurrent renew), same no-op result.

Holder-only removal: Line 82 reads holder from $lease_json (the pre-activity-round-trip snapshot). As the prior review noted, holder is immutable across the lease lifecycle — only renewed_at and superseded_at change — so this is safe even though $WIT_ACTIVE_LEASE_JSON would be more obviously consistent.

Idempotency guard: The fresh issue view --json assignees read at line 83 before the --remove-assignee ensures a re-run where holder was already unassigned is a no-op. The jq -e guard pipes cleanly through >/dev/null to suppress the matched value.

---

Correctness — lib/lease.sh extraction

lib/lease.sh lines 41–55

The output vars are reset at function entry (lines 43–44), which is load-bearing: a call that finds no active lease must leave both empty, not inherit a previous call's results. The jq 'sort_by(.id) | reverse | .[]' correctly handles the back-off case — a superseded higher-id back-off is skipped, exposing the still-active earlier lease. Three callers (both adapters and lib/lease.test.sh) now share one implementation.

---

Test coverage

lease-coordination.test.sh covers all four combinations: expired active → refuse (renew), live active → accept (renew), expired + co-assignee → holder-only removal (reclaim), concurrent supersede during activity window → revalidation abort (reclaim). The counter-file approach for TOCTOU simulation is well-constructed.

lib/lease.test.sh additions cover the back-off masking case (the subtle correctness invariant both verbs depend on) and the empty out-params case. 15/15 assertions stated in the PR body are all represented in the test file.

One observation (non-blocking): the --paginate dispatch in the stub relies on case-statement ordering — *"/timeline"* appearing before *"--paginate"* ensures timeline calls (which carry both flags) route correctly. This is correct but fragile to reordering. A short comment would preserve intent for future editors.

---

Hygiene commit

• Exec bit on lease-coordination.test.sh: necessary — the CI runner invokes test scripts directly.
• CHANGELOG MD004 fix: line 16 previously started with + which markdownlint read as a plus-style list marker; reworded to avoid the ambiguity.
• Header comment: drops the issue-number reference per the repo's comment-hygiene rule.

All three are correct and minimal.

---

Verdict

Approve. Both fixes are correct, the TOCTOU revalidation and liveness gate close the two P1 correctness gaps precisely, the test discrimination proofs are strong, and the wit_select_active_lease extraction removes duplication cleanly. The hygiene commit is appropriate and targeted. No blocking findings.

[claude]

Claude finished @kyle-sexton's task in 3m 49s —— View job

---

PR Review: fix(work-items): github adapter lease-coordination robustness (#370)

Tasks:

• Gather context and understand request
• Read PR diff and changed files
• Review renew-lease.sh changes
• Review reclaim.sh changes
• Review lib/lease.sh extraction
• Review test coverage
• Review CONTRACT.md and plugin.json changes
• Post final review

---

Summary

Both fixes are correct and well-structured. The liveness gate in renew-lease and the TOCTOU revalidation + holder-only removal in reclaim close the two P1 correctness gaps precisely. Prior reviews in this thread already gave a thorough audit; my independent read below identifies one new test coverage gap and confirms several correctness points the earlier passes noted but did not fully trace.

---

Correctness — renew-lease.sh

Two-gate ordering is correct. The identity check (is this comment still the active lease?) runs at line 58, liveness check at line 70. This order is correct: checking liveness before identity would allow a session to confirm its TTL is fine and then discover the comment was superseded — the path as written fails fast on supersede first.

Liveness check uses $lease_json (individual comment fetch) rather than $WIT_ACTIVE_LEASE_JSON (paginated list fetch). Both refer to the same comment ID after the identity check passes, but they come from different API calls. In a race where the lease is renewed between line 34 and line 56, $lease_json could show the old (expired) renewed_at while $WIT_ACTIVE_LEASE_JSON would show the updated one — causing a false-positive refusal (exit 7). This is conservative-safe (the caller retries), not a data hazard. The prior reviews flagged this as non-blocking; confirmed.

---

Correctness — reclaim.sh

Revalidation at lines 68–75 is correct. Two conditions checked independently:

1. WIT_ACTIVE_LEASE_ID != lease_comment_id — catches supersede (a concurrent claimer wrote a new lease comment, changing which comment is the active one).
2. wit_lease_is_live "$WIT_ACTIVE_LEASE_JSON" "$(date -u +%s)" — catches in-place renewal (same comment ID, but a concurrent renew-lease call that was in-flight since before the TTL expired successfully updated renewed_at during the activity round-trips).

Both use the revalidated $WIT_ACTIVE_LEASE_JSON from the second wit_list_lease_comments call, and the liveness check correctly uses a fresh $(date -u +%s) rather than the stale $now_epoch from before the activity round-trips.

Holder-only removal at line 82–86. holder is read from $lease_json (pre-activity snapshot). This is safe: holder is immutable across the lease lifecycle — only renewed_at and superseded_at change after a lease is written. The idempotency guard (fresh issue view + jq -e before --remove-assignee) correctly handles the re-run case where the holder was already unassigned. The >/dev/null on the jq -e correctly suppresses the matched value while preserving the exit code for the guard.

$now staleness (line 54 captured before activity round-trips, used for superseded_at at line 88). Confirmed cosmetically stale but functionally correct: superseded_at only needs to be non-null to mark the lease non-live; wit_lease_is_live checks superseded_at // empty != "".

---

Correctness — lib/lease.sh

Out-param reset at lines 43–44 is load-bearing. A "no active lease found" result leaves both vars empty rather than inheriting values from a prior call in the same process. This is what makes the guard [[ -n "$lease_json" ]] || emit false "no active lease record" in reclaim.sh work correctly on repeated invocations.

export WIT_ACTIVE_LEASE_ID WIT_ACTIVE_LEASE_JSON inside the function makes the output vars available in child processes. This is necessary for the test harness: export -f gh makes the stub available to bash "$RENEW" ... child processes, and the called scripts source lease.sh independently — so the export within the function is correct (the function runs in the child process context).

---

Finding — Test coverage gap: concurrent-renew revalidation path not exercised

The revalidation block in reclaim.sh checks two conditions at lines 70 and 73:

if [[ "$WIT_ACTIVE_LEASE_ID" != "$lease_comment_id" ]]; then   # ID-changed branch
  emit false "lease superseded by a concurrent claim during reclaim"
fi
if wit_lease_is_live "$WIT_ACTIVE_LEASE_JSON" "$(date -u +%s)"; then  # same-ID-but-live branch
  emit false "lease renewed during reclaim"
fi

Test cases 10–13 (lease-coordination.test.sh lines 131–145) exercise only the ID-changed path (concurrent supersede: lease-comments-2 contains a new lease comment with ID 200). The same-ID-but-now-live path — where a renew-lease.sh call that was in-flight since before the TTL expired succeeds during the activity window — is not tested.

The implementation at line 73 is correct ($WIT_ACTIVE_LEASE_JSON is from the revalidation read, and $(date -u +%s) is a fresh epoch). But a test like the one below would make both branches independently verified:

# lease-comments-1: expired alice lease (ID 123)
# lease-comments-2: same ID 123, renewed_at now in the future
new_scenario
EXP_MARKER="$(marker alice "$PAST" 24)"
RENEWED_MARKER="$(marker alice "$FUTURE" 24)"
lease_array 123 "$EXP_MARKER" >"$GH_STUB_DIR/lease-comments-1"
lease_array 123 "$RENEWED_MARKER" >"$GH_STUB_DIR/lease-comments-2"
jq -cn '["alice"]' >"$GH_STUB_DIR/assignees"
out="$(bash "$RECLAIM" "$ID" 2>/dev/null)"; rc=$?
assert_eq "reclaim succeeds (0) when lease was renewed during activity window" "0" "$rc"
assert_eq "reclaim reports reclaimed:false when lease was renewed in place" "false" "$(jq -r '.reclaimed' <<<"$out")"
assert_not_contains "reclaim removes NO assignee when lease was renewed in place" "$(calls)" "REMOVE_ASSIGNEE"
cleanup_scenario

Non-blocking — the implementation is correct — but this path is reachable in production and worth covering. Fix this →

---

Stub dispatch ordering (commit 011ff12)

The comment added in the final commit at lease-coordination.test.sh line 44 correctly documents the load-bearing ordering requirement. The dispatch for *"/timeline"* must precede *"--paginate"* because timeline calls carry both flags. The comment preserves this invariant for future editors, addressing Finding 4 from the prior review.

---

CONTRACT.md

The "Lease protocol" additions at lines 190–194 and lines 214–224 accurately document both semantics: renew-lease refuses an expired-but-active lease (exit 7, no revive); reclaim is holder-only and revalidated before mutation. The prose is consistent with the implementation.

---

Verdict

Approve. Both fixes are correct, the test discrimination proofs are strong (pre-fix failures confirm the cases aren't passing vacuously), and wit_select_active_lease extraction cleanly eliminates a third copy of the active-lease selection loop. The single non-blocking finding above (concurrent-renew revalidation test case) is a test coverage gap only — the implementation handles it correctly.

[kyle-sexton]

Post-green review pass — 3 rounds, all Approve, no blocking findings.

Round	Finding	Classification	Disposition
1	holder/superseded_at use pre-revalidation $lease_json	Cosmetic, non-blocking	Accepted (holder immutable, functionally safe)
1	$now predates revalidation round-trip	Cosmetic, non-blocking	Accepted (superseded_at only needs truthy)
1	Stub dispatch ordering implicit	Maintainability note	Fixed in commit 011ff12 (comment added)
3	Concurrent-renew revalidation path untested	VALID, non-blocking, deferred	Filed #687

No inline review threads to resolve (bot posts as PR-level comments). Flipping to ready.

[claude]

Claude finished @kyle-sexton's task in 2m 43s —— View job

---

PR Review — fix(work-items): github adapter lease-coordination robustness (#370)

Tasks:

• Gather context and understand request
• Read PR diff and changed files
• Review renew-lease.sh changes
• Review reclaim.sh changes
• Review lib/lease.sh extraction
• Review test coverage
• Review CONTRACT.md and plugin.json changes
• Post final review

---

Summary

Three prior review rounds all landed at Approve; I've read all changed files independently. Both fixes are correct, the extraction is clean, and the test discrimination proofs are rigorous. I have one new observation not flagged before (see Finding 1), and confirm the one known gap (concurrent-renew revalidation path, filed as #687) remains the only non-blocking open item.

---

Finding 1 — reclaim.sh line 58: new_scenario resets calls.log but the --paginate counter is not reset

lease-coordination.test.sh line 74

new_scenario() { GH_STUB_DIR="$(mktemp -d)"; export GH_STUB_DIR; : >"$GH_STUB_DIR/calls.log"; }

GH_STUB_DIR is a fresh mktemp -d per scenario, so lease-comments.count starts absent and the counter resets correctly — cleanup_scenario removes the whole tempdir. This is fine as written; I initially wondered whether the counter file could carry across scenarios, but the tempdir rotation prevents it. Not a bug. Noting it here to close the question rather than leave it open.

---

Correctness — renew-lease.sh

Gate ordering (lines 57–74) is correct: supersede check (via wit_select_active_lease) before TTL liveness check. A session that holds a handle to a superseded comment should fail fast on the identity check, not consume an extra liveness call to fail.

$lease_json vs $WIT_ACTIVE_LEASE_JSON for liveness (line 70): $lease_json comes from the individual comment fetch (line 42); $WIT_ACTIVE_LEASE_JSON from the paginated list (line 57). Both address the same comment after the identity check confirms WIT_ACTIVE_LEASE_ID == lease_comment_id. In a race where the lease is renewed between the two API calls, $lease_json could show a stale renewed_at and trigger a spurious exit 7. The outcome is conservative-safe — the caller retries — and was flagged by prior review rounds. Non-blocking.

Fresh epoch for liveness check ($(date -u +%s) at line 70) vs the post-check now capture at line 76: correct ordering — the stale $now is only used for the PATCH body's renewed_at, not for any liveness decision.

---

Correctness — reclaim.sh

Revalidation block (lines 68–75): two independent conditions checked in sequence — ID-changed (concurrent supersede) then same-ID-but-now-live (concurrent renew). Both use $WIT_ACTIVE_LEASE_JSON from the fresh revalidation read and a fresh epoch. Correct and complete.

Holder-only removal (lines 82–86): holder read from $lease_json (pre-revalidation snapshot) is safe — holder is immutable across the lease lifecycle. Fresh issue view before --remove-assignee handles the idempotent re-run case correctly. The jq -e guard + >/dev/null pattern is the right idiom (exit code preserved, value suppressed).

$now staleness (line 54 captured before activity round-trips, used at line 88 for superseded_at): cosmetically stale but functionally correct — superseded_at only needs to be non-null to mark the lease non-live. Flagged by prior reviews; no change needed.

---

Correctness — lib/lease.sh

Module-level var initialization (lines 39–40) plus function-entry reset (lines 43–44): the module-level lines serve as variable declaration; the function-entry reset is what guarantees correct behavior across repeated calls. Both are needed.

export WIT_ACTIVE_LEASE_ID WIT_ACTIVE_LEASE_JSON (line 54) inside the function: necessary. The verbs are invoked via bash verb.sh as child processes; export -f gh propagates the stub, and the export here propagates the output vars into the child's environment at the point they're set. This is the correct pattern for Bash output-via-global.

---

Test coverage — lease-coordination.test.sh

All four decision-path combinations covered:

Scenario	Cases
Expired active → renew refuses (no PATCH)	1–2
Live active → renew accepts	3–5
Holder-only removal with co-assignee	6–9
Concurrent supersede during activity window → no-op	10–13

Discrimination proofs (pre-fix failures) confirm none of the 13 passing cases are vacuously true.

Known gap (non-blocking, filed as #687): the same-ID-but-now-live revalidation path in reclaim — where a concurrent renew-lease updates renewed_at on the same comment during the activity window — is not exercised. The implementation handles it correctly (WIT_ACTIVE_LEASE_JSON from the second read picks up the updated renewed_at; fresh epoch detects live). A test scenario can be constructed with lease-comments-1 (expired alice, ID 123) and lease-comments-2 (same ID 123, renewed_at in $FUTURE).

Test coverage — lib/lease.test.sh

Four new wit_select_active_lease cases cover the load-bearing invariants: single active lease, back-off masking, newest-wins, and empty-on-all-superseded. These are exactly the edge cases both verbs depend on.

---

CONTRACT.md / CHANGELOG / plugin.json

• "Lease protocol" additions accurately document both new semantics and are consistent with the implementation.
• CHANGELOG entry at 0.14.3 is clear and technically precise.
• Version bump 0.14.2 → 0.14.3 is correct scope for a patch fix.

---

Verdict

Approve. Both fixes are correct, the TOCTOU revalidation and liveness gate close the two P1 gaps precisely, and the wit_select_active_lease extraction eliminates a third copy of the selection loop. Test discrimination proofs are strong. The one known test gap (concurrent-renew path) is non-blocking and already tracked in #687.

[claude]

Claude finished @kyle-sexton's task in 2m 4s —— View job

---

PR Review: fix(work-items): github adapter lease-coordination robustness (#370)

Tasks:

• Gather context and understand request
• Read new commit diff (f81d6c9)
• Review added test case for concurrent-renew revalidation path
• Post final review

---

Summary

This synchronize was triggered by commit f81d6c9 ("test(work-items): cover reclaim same-id in-place-renew revalidation branch"), which addresses the single non-blocking finding from the third review round: the "same-ID-but-now-live" revalidation branch in reclaim.sh was not independently exercised. The fix is minimal and correct — 17 lines added to lease-coordination.test.sh, no production code changes.

---

Correctness of the new scenario (lines 147–162)

The scenario exercises the second revalidation branch at reclaim.sh:73–75:

if wit_lease_is_live "$WIT_ACTIVE_LEASE_JSON" "$(date -u +%s)"; then
  emit false "lease renewed during reclaim"
fi

Stub construction:

• lease-comments-1 (first paginate call): [{id:123, body:expired(alice,PAST)}] — initial selection sees an expired lease
• lease-comments-2 (second paginate call, revalidation): [{id:123, body:renewed(alice,FUTURE)}] — same comment ID, renewed_at now in the future

Trace through reclaim.sh:

1. First wit_list_lease_comments → reads lease-comments-1 → lease_comment_id=123, lease_json=expired
2. wit_lease_is_live(expired, now) → false → proceeds past the fast-path live-lease check
3. Activity round-trips → both return 0 (no comment-activity/pr-activity files in the scenario dir)
4. Revalidation: second wit_list_lease_comments → reads lease-comments-2 → WIT_ACTIVE_LEASE_ID=123, WIT_ACTIVE_LEASE_JSON=renewed
5. ID check at line 70: "123" != "123" → false → does not abort (correct — same comment, not superseded)
6. Liveness check at line 73: wit_lease_is_live(renewed, now) → FUTURE → live → emit false "lease renewed during reclaim" → exits 0

All four assertions follow correctly:

• exit 0 — emit calls exit 0
• reclaimed:false — emitted before the assignee/PATCH code
• no REMOVE_ASSIGNEE — script exited before line 85
• no PATCH — script exited before line 89–90

The key correctness detail is that the liveness check at line 73 uses $WIT_ACTIVE_LEASE_JSON (the revalidation read), not $lease_json (the pre-activity snapshot). Using the stale snapshot would mean this branch could never trigger — a concurrent renew during the activity window would be invisible. The test correctly confirms that the revalidation read is what drives the liveness decision.

---

Coverage now complete

The two revalidation branches are now independently verified:

Branch	What triggers it	Test scenario
ID-changed (WIT_ACTIVE_LEASE_ID != lease_comment_id)	Concurrent supersede: new lease comment with a different ID	Lines 131–145
Same-ID-but-live (wit_lease_is_live on revalidated JSON)	Concurrent in-place renew: same comment, renewed_at updated to future	Lines 147–162 (new)

Both branches assert identical behavioral invariants: reclaimed:false, no assignee removal, no supersede PATCH.

---

Verdict

Approve. The new scenario is correctly constructed, the stub interaction correctly models a concurrent in-place renewal (counter file → lease-comments-2), and the assertions are exhaustive. This closes the only coverage gap from the prior review pass. All other findings from rounds 1–3 were either cosmetic/accepted or already addressed. The PR is clean and complete.