fix(work-items): local-markdown claim lease/assignee integrity (#367)

[kyle-sexton]

Summary

Follow-up from #224. Closes two related lease/assignee-integrity gaps in the local-markdown adapter's claim.sh (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 is introduced — this implements the issue's recommended, less-invasive candidate for Finding 1 (list-derivation projection), so it is a patch bump (0.14.1 → 0.14.2).

Fix

Finding 1 (P1) — expired lease never cleared the assignee, so abandoned claims never returned to the frontier. An expired lease still left assignees populated. Because reclaim is unsupported for this offline adapter (it has no coordination surface to run an activity check over) and the core list-frontier derivation always excludes assigned items, any abandoned local claim was permanently absent from selection once its TTL expired.

• list-items now projects an expired-lease item's effective assignee as empty (wit_emit_local_item gained an opt-in unassign-expired mode), so the core frontier filter — which keys on assignees — returns the item to selection. No new capability, no stored mutation.
• The projection is scoped to list/frontier derivation. get-item still reports the stored assignee verbatim, preserving parity with the GitHub adapter (whose assignee persists until reclaim runs) and keeping the raw claim record inspectable.
• CONTRACT.md's local-markdown degradation note now documents this offline compensation.

Finding 2 (P2) — a failed assignee write still reported a successful claim. claim.sh appended the inline lease marker and then called wit_fm_set … assignees with no return-code check and no set -e, so a failed assignee write (store full/unwritable) was silently ignored and a success JSON was still emitted — leaving a live lease marker with an empty assignees, which list-frontier presents as available while later claims conflict on the live lease until it expires.

• The marker append and assignee write are now one consistent operation (wit_claim_write): on a failed assignee write it best-effort rolls the just-appended marker back and fails the claim with exit 1 (internal/unexpected store I/O, distinct from an exit-7 claim race), emitting no success record for a half-applied write.

Verification

New adapters/local-markdown/claim-integrity.test.sh covers both findings — Finding 1 end-to-end through the core CLI (list-frontier), Finding 2 as a deterministic, OS-portable unit test of the rollback (the abstract conformance suite runs against every adapter and cannot hold local-markdown-specific or fault-injection cases). All checks run via the repo harness (plugins/**/*.test.sh).

New test (bash claim-integrity.test.sh), exit 0:

PASS: [1] expired-lease A returns to the frontier
PASS: [2] live-lease B stays out of the frontier
PASS: [3] get-item leaves the expired-lease assignee stored
PASS: [4] successful claim write returns 0
PASS: [5] successful claim write keeps the lease marker
PASS: [6] successful claim write records the assignee
PASS: [7] failed assignee write fails the claim
PASS: [8] failed assignee write rolls the lease marker back
PASS: [9] failed claim leaves assignees empty

Full local-markdown conformance still green, including the offline gh/curl-blocking shim (run-conformance.sh --binding local-markdown):

Conformance (local-markdown): 61 cases, 0 failed

ShellCheck clean on all changed scripts (--rcfile .shellcheckrc); every plugins/work-items/**/*.test.sh passes; changed files carry no trailing whitespace, tabs, or comment issue/TODO markers.

Closes #367

Related

• work-items local-markdown claim.sh: lease/assignee integrity (expired lease never clears assignee; failed assignee write reports success) #367 — this issue.
• feat(work-items): bundle work-item-tracker seam in plugin (shape A) #224 — the bundle-seam PR that relocated this code (shape A); both gaps were deferred there by author delegation.

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

[claude]

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

---

PR Review — fix(work-items): local-markdown claim lease/assignee integrity

• Load tools
• Read PR diff and changed files
• Review correctness, security, and test coverage
• Post findings

---

Summary

Solid fix for two real integrity gaps in the local-markdown adapter. The architecture choices (projection-not-mutation for Finding 1, rollback-on-failure for Finding 2) are the right calls for an offline adapter. All nine test cases are well-targeted and the contract docs are updated in sync.

One misleading error message worth fixing, one subtle silent-failure path worth noting.

---

Findings

1. claim.sh error message asserts rollback success unconditionally — MINOR

claim.sh line 68:

printf 'claim: recording the assignee for %s failed; rolled back the lease marker\n' "$id" >&2

wit_claim_write documents "best-effort" rollback and does not communicate whether the rollback itself succeeded (it returns 1 unconditionally on the failure path). If mktemp fails — the most likely cause being the same "store full" condition that caused wit_fm_set to fail — the rollback is silently skipped and the marker is left orphaned, but the caller's stderr message says "rolled back." A consumer reading logs would believe the claim was cleanly aborted when it wasn't.

The message could be made accurate without breaking the function's interface:

if ! wit_claim_write "$file" "$marker_line" "$(jq -cn --arg h "$holder" '[$h]')"; then
  printf 'claim: recording the assignee for %s failed\n' "$id" >&2
  exit 1
fi

And add a stderr warning inside wit_claim_write when the rollback itself fails:

if tmp="$(mktemp)" && grep -vxF -- "$marker_line" "$file" >"$tmp"; then
  mv "$tmp" "$file" 2>/dev/null || rm -f "$tmp"
else
  printf 'wit_claim_write: warning: rollback of lease marker failed — store may be inconsistent\n' >&2
fi

Fix this →

2. Rollback removes all exact-match lines — INFORMATIONAL

common.sh line 162:

grep -vxF -- "$marker_line" "$file" >"$tmp"

grep -vxF removes every line that exactly matches $marker_line. In practice this is safe because marker content includes a monotonically increasing lease_comment_id, making duplicate marker lines impossible. Worth a one-line note on the invariant the rollback relies on, since grep -Fn (remove the last matching line) is not a portable shell primitive and the current approach is intentional.

---

What's correct

• Finding 1 logic (common.sh lines 249–254): the unassign_expired branch only fires when assignees != "[]" (avoids the wit_active_lease_json call for items with no assignee) and correctly distinguishes "no active lease" (empty → don't clear) from "expired active lease" (non-empty and !wit_lease_is_live → clear). The short-circuit on assignees != "[]" is a nice optimization.
• Finding 1 scope: get-item still calls wit_emit_local_item without the flag (verbatim store), list-items.sh passes true. The split is clean and avoids a stored mutation.
• Finding 2 atomicity: wit_claim_write correctly sequences append → set, with rollback on failure. The helper is well-named and pulling it out of claim.sh makes the intent clear.
• Test coverage: the mock override of wit_fm_set (line 87) correctly intercepts the in-process call because bash resolves function names at call time; the mock runs against the real wit_claim_write body. Cases 4–9 cover both the happy path and the failure path end-to-end.
• CONTRACT.md update is accurate and complete.
• Version bump (0.14.1 → 0.14.2) is the correct patch level for a bug-fix hardening pass.
• ShellCheck directive in the test (SC2154 for FAILED/CASE_NUM) is correctly scoped to the file.

---

Overall this is ready to merge with the error message nit noted above. The architecture is sound and the tests give good confidence in both behaviors.

[claude]

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

---

PR Review — fix(work-items): local-markdown claim lease/assignee integrity

• Load tools
• Read PR diff and changed files
• Review correctness, security, and test coverage
• Post findings

---

The synchronize commit (de51536) only makes claim-integrity.test.sh executable. All substantive changes are from a234df2. The two findings from the previous review cycle carry forward unchanged — one is worth fixing before merge, one is informational.

---

Finding 1 — Misleading error message asserts rollback success unconditionally — MINOR

claim.sh line 68:

printf 'claim: recording the assignee for %s failed; rolled back the lease marker\n' "$id" >&2

wit_claim_write's rollback path is wrapped in a bare if with no else — if mktemp fails (the same "store full" condition that caused wit_fm_set to fail), the block is silently skipped and the function returns 1 with no warning, leaving an orphaned marker. The caller's message unconditionally says "rolled back" regardless of whether the rollback ran.

common.sh lines 162–164:

if tmp="$(mktemp)" && grep -vxF -- "$marker_line" "$file" >"$tmp"; then
  mv "$tmp" "$file" 2>/dev/null || rm -f "$tmp"
fi
# no else — silent skip if mktemp fails

Suggested fix — drop the "rolled back" claim from the caller and add an explicit warning inside wit_claim_write when the rollback itself fails:

claim.sh line 68:

printf 'claim: recording the assignee for %s failed\n' "$id" >&2

common.sh rollback block:

if tmp="$(mktemp)" && grep -vxF -- "$marker_line" "$file" >"$tmp"; then
  mv "$tmp" "$file" 2>/dev/null || rm -f "$tmp"
else
  printf 'wit_claim_write: warning: rollback of lease marker failed — store may be inconsistent\n' >&2
fi

Fix this →

---

Finding 2 — Rollback removes all exact-match lines — INFORMATIONAL

common.sh line 162:

grep -vxF -- "$marker_line" "$file" >"$tmp"

grep -vxF removes every line that exactly matches $marker_line. This is safe because marker content embeds a monotonically increasing lease_comment_id, making exact duplicates impossible in practice. No action needed — just worth documenting the invariant the rollback relies on.

---

What's correct

• Finding 1 projection logic (common.sh lines 249–254): the outer guard $assignees != "[]" avoids the wit_active_lease_json call for unassigned items; the inner guard [[ -n "$active_lease" ]] correctly preserves manual assignments that have no lease marker. The distinction between "no active lease" (do nothing) and "expired active lease" (clear) is precise.
• Finding 1 scope split: get-item still calls wit_emit_local_item without the flag (verbatim store); list-items.sh passes true. The parity with the GitHub adapter (whose assignee persists until reclaim) is preserved for get-item.
• Finding 2 atomicity: wit_claim_write correctly sequences append → set with rollback on failure. The helper is well-named and its extraction from claim.sh clarifies intent.
• Test coverage: the wit_fm_set function override (line 87) correctly intercepts the in-process call at bash's function-name resolution time. Cases 4–9 cover both the happy path and the failure path end-to-end. The end-to-end Finding 1 cases (1–3) drive the full CLI stack.
• CONTRACT.md update accurately documents the offline compensation in the degradation note.
• Version bump (0.14.1 → 0.14.2) is the correct patch level for a bug-fix hardening pass with no new capability.

---

Overall this is ready to merge. Finding 1 (the error message) is a low-severity nit — it doesn't affect correctness, only log accuracy in the double-failure scenario (store full AND mktemp fails). The architecture and test coverage are sound.

[claude]

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

---

PR Review — fix(work-items): local-markdown claim lease/assignee integrity

• Load tools
• Read latest commit changes (e875d39)
• Verify Finding 1 fix (misleading error message / rollback diagnostics)
• Post updated findings

---

The synchronize commit (e875d39) directly addresses both findings from the previous review cycle. Here's a precise accounting of what changed and whether it's correct.

---

Finding 1 — Fixed correctly

claim.sh — error message no longer asserts a rollback that may not have run:

# Before
printf 'claim: recording the assignee for %s failed; rolled back the lease marker\n' "$id" >&2
# After
printf 'claim: recording the assignee for %s failed\n' "$id" >&2

common.sh — wit_claim_write rollback block restructured so the failed-rollback path is explicit rather than a silent fall-through:

if tmp="$(mktemp)" && grep -vxF -- "$marker_line" "$file" >"$tmp" && mv "$tmp" "$file" 2>/dev/null; then
  return 1
fi
[[ -n "${tmp:-}" ]] && rm -f "$tmp"
printf 'wit_claim_write: rollback of the lease marker failed — the store may hold an orphaned lease marker\n' >&2
return 1

All four paths are handled correctly:

• mktemp succeeds + grep succeeds + mv succeeds → rollback done, return 1 ✓
• mktemp succeeds + grep or mv fails → temp file cleaned up, warning printed, return 1 ✓
• mktemp fails → tmp is empty string, [[ -n "${tmp:-}" ]] guard skips the rm, warning printed, return 1 ✓
• All three succeed → early return 1 from inside the if, temp file already consumed by mv ✓

Finding 2 — Addressed with a comment

The comment on the unique-marker invariant is now inline in common.sh:

# The marker line is unique in the file (lease_comment_id is store-monotonic), so removing every exact match removes only this one — grep has no portable "remove the last matching line" primitive.

This is exactly what was asked for. No action needed.

---

New test case (rollback-blocked path)

claim-integrity.test.sh lines 98–117

The test correctly:

• Relies on the wit_fm_set() { return 1; } override still active from the preceding test block (triggering the rollback attempt — intentional by design)
• Overrides mktemp to fail (simulating the same store condition that blocked the assignee write)
• Captures stderr via 2>&1 inside $() — warning is visible to the subshell and correctly attributed to WARN
• Checks RC=1, the warning substring, and that the marker is left in the file (rollback couldn't run)
• Restores mktemp via unset -f mktemp ✓
• SC2329 disable is the correct directive for a function that shellcheck can't trace across the source boundary ✓

---

What's unchanged and still correct

All findings from the previous two review cycles are confirmed resolved or acknowledged:

• Finding 1 projection logic (common.sh lines 249–254): unchanged, still correct
• Finding 1 scope split: get-item verbatim, list-items.sh projects — unchanged ✓
• Finding 2 atomicity (wit_claim_write): refined and improved in this commit ✓
• CONTRACT.md and version bump: unchanged ✓
• Test cases 1–9: unchanged, still all valid ✓

---

This PR is ready to merge. Both findings have been correctly addressed, the new test case is well-constructed, and the logic handles all failure modes explicitly.

[claude]

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

---

PR Review — fix(work-items): local-markdown claim lease/assignee integrity

• Load tools
• Read PR diff and changed files
• Review correctness, security, and test coverage
• Post findings

---

Summary

This is the third review cycle on this PR. The two previously identified findings were addressed in commit e875d39, and the rollback-blocked scenario gained a dedicated test case. All changes reviewed against the current HEAD.

---

Prior Findings — Resolved

Finding 1 — Misleading error message (claim.sh line 69): Fixed. The message no longer asserts rollback success:

printf 'claim: recording the assignee for %s failed\n' "$id" >&2

Finding 2 — Silent rollback failure (common.sh lines 168–173): Fixed. All four rollback paths are now covered:

• mktemp + grep + mv all succeed → rollback done, return 1 ✓
• mktemp succeeds, grep or mv fails → temp cleaned, warning printed, return 1 ✓
• mktemp fails → tmp empty, [[ -n "${tmp:-}" ]] guard skips rm, warning printed, return 1 ✓
• (The 2>/dev/null on mv is intentional — failure is caught by the conditional anyway) ✓

Finding 3 — grep -vxF exact-match invariant (common.sh line 162): Documented with an inline comment. Informational, no change needed.

---

Remaining Observation — Informational

claim-integrity.test.sh line 107: The rollback-blocked test block relies on the wit_fm_set() { return 1; } override defined at line 87 still being in scope — it's never unset between the two failure blocks. This is intentional (the test needs both the assignee write and the rollback to fail), but the dependency is implicit. A future maintainer inserting a test between lines 96 and 101 that expects real wit_fm_set behavior would hit a silent failure. A one-line comment noting the carry-over is intentional would prevent that. No action required — this is observable and not a defect.

---

Correctness Verification

• Projection logic (common.sh lines 257–263): The outer guard (assignees != "[]") short-circuits for unassigned items. The inner guard (-n "$active_lease") preserves manual assignments that have no lease marker. "No active lease" (do nothing) vs. "expired active lease" (clear) is correctly distinguished.
• Scope split: list-items.sh passes true (projection active); get-item calls without the flag (verbatim store). Parity with the GitHub adapter is preserved for get-item. ✓
• wit_claim_write atomicity: append → set, with rollback and diagnostic on failure. The helper is correctly extracted from claim.sh. ✓
• Test cases 10–12 (rollback-blocked path): wit_fm_set override carries over as intended, mktemp is overridden and restored with unset -f, stderr is captured via 2>&1 inside $(), RC and warning substring are checked, and orphaned marker presence is verified. ✓
• CONTRACT.md accurately documents the offline compensation and the get-item verbatim behavior. ✓
• Version bump (0.14.1 → 0.14.2): correct patch level for a bug-fix hardening pass with no new capability. ✓
• CHANGELOG: both fixes clearly described with exit-code context. ✓
• No security issues: all file paths are internal to the adapter; no user-controlled input reaches grep or mv without prior validation. ✓

---

This PR is ready to merge. All three review cycles produced findings that were correctly addressed, the architecture decisions are sound, and the test coverage is now comprehensive across all failure modes.