From 3488eb5213519405164b9d8915833871b9bacbb1 Mon Sep 17 00:00:00 2001 From: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com> Date: Wed, 2 Sep 2026 21:13:10 -0700 Subject: [PATCH 1/7] fix: fence a post-reclaim close on custody identity, not item status Core 7 of contracts/custody-coordination.v1.md; ledger row CCV1-009; work item pipeline-dn4. `Beads.resolve`'s custody fence ran only under `if current.status == "held"`. A reap does not leave a reclaimed item held -- `supervisor.reap_project` calls `release`, which puts it back to `open` and clears bd's assignee -- so the one state the fence exists for was the one state it skipped: a stale holder's close landed with exit 0 and no refusal anywhere. Status was never the right discriminator; custody identity is. The fence now also refuses when the item is NOT held but its custody record still names the caller. PR #51's integrator resolve (item pipeline-79t) is preserved by the same key: only the session the custody record names is refused, so an integrator's single-call close of an item nobody holds is exactly as unfenced as before. The `current.holder != who` guard keeps a holder's own already-landed close re-attemptable, so PR #63's phantom-conflict recovery does not regress. Measured, not inspected: tests/integration/test_post_reclaim_fence.py is red on the pre-fix code (DID NOT RAISE FencedError on both fence halves) and green after. It drives the REAL reap sweep as well as a bare release, and pins the integrator half alongside the fence half so neither can be silently traded for the other. The same fix flips the previously-red tool-layer fixture test_reap_recovery.py:: test_explicit_resolve_refusal_after_reap_clears_held_and_allows_new_claim green -- no tool-layer change was needed, its FencedError branch was simply unreachable in this state. Ledger row CCV1-009: VIOLATION -> CONFORMS, probe rewritten to assert the fixed shape (identity-keyed refusal outside the status gate, the contract's own "not held by this session" wording) plus the continued existence of both halves of the behavioural fixture. --- ledger/checks/test_custody_rows.py | 62 +++++-- ledger/rows.yaml | 39 +++-- src/amplifier_work_tracker/adapter.py | 96 +++++++---- tests/integration/test_post_reclaim_fence.py | 172 +++++++++++++++++++ 4 files changed, 311 insertions(+), 58 deletions(-) create mode 100644 tests/integration/test_post_reclaim_fence.py diff --git a/ledger/checks/test_custody_rows.py b/ledger/checks/test_custody_rows.py index fba0461..428eb76 100644 --- a/ledger/checks/test_custody_rows.py +++ b/ledger/checks/test_custody_rows.py @@ -38,6 +38,7 @@ TOOL_MODULE, contains, count, + function_names, read, row, sha256, @@ -150,14 +151,25 @@ def test_row_ccv1_008() -> None: def test_row_ccv1_009() -> None: - """Core 7 VIOLATION pin: the whole close fence runs only while the item is - still `held`. A reaped item is `open` with its assignee cleared, so a - stale holder's close skips the fence entirely. - - Pinned structurally rather than by one line, so that ADDING a fence for - the released state (a legitimate fix shape that keeps the existing gate) - also flips this probe: every `raise FencedError` in resolve's pre-write - region must still sit inside the `status == "held"` block. + """Core 7 CONFORMS: the close fence is keyed on custody IDENTITY, not on + the item's status, so it also refuses the released-but-not-yet-re-claimed + state a reap leaves behind -- while still letting an integrator close an + item nobody holds. + + Three parts, because each is defeatable alone: + + 1. a refusal exists OUTSIDE the `status == "held"` gate -- the gate + was the whole gap (a reaped item is `open` with its assignee + cleared, so a status-gated fence skipped exactly the state it + existed for); + 2. that refusal is reached ONLY when the custody record names THIS + caller and the item is not theirs -- the identity key is what + keeps PR #51's integrator resolve unfenced, and what keeps a + holder's own already-landed close re-attemptable; + 3. the discriminating BEHAVIOURAL fixture exists and still carries + both halves. This module is in-process only: it proves shape, + never behaviour (see the module docstring) -- so it verifies the + fixture's continued existence rather than pretending to be it. """ src = read(ADAPTER) body = src[ @@ -165,15 +177,37 @@ def test_row_ccv1_009() -> None: ] pre_write = body[: body.index(" try:")] gate = ' if current.status == "held":' - assert gate in pre_write, "CCV1-009 pin: the status gate is gone" + assert gate in pre_write, "Core 7: the held-item fence disappeared entirely" + + identity_fence = " elif cust_holder == who and current.holder != who:" + assert identity_fence in pre_write, ( + "Core 7: the status-independent, custody-identity-keyed fence changed shape. " + "A close by the session the custody record still names must be refused even " + "when the item is no longer `held` (the post-reclaim state)." + ) fences = [m.start() for m in re.finditer(r"raise FencedError", pre_write)] - assert fences and all(pos > pre_write.index(gate) for pos in fences), ( - "CCV1-009 (Core 7, VIOLATION) pin no longer matches. If the post-reclaim fence " - "was FIXED, this is the expected failure: flip the row to CONFORMS, cite the " - "discriminating fixture (both halves -- refuse the stale holder, still allow the " - "integrator's plain resolve), and resolve work_item_pipeline-dn4." + assert any(pos > pre_write.index(identity_fence) for pos in fences), ( + "Core 7: the post-reclaim branch no longer raises FencedError -- a refusal that " + "is not a FencedError does not clear the caller's local custody state" + ) + assert "not held by this session" in pre_write, ( + "Core 7: the refusal must still name 'not held by this session' -- the wording " + "the contract's own `fence.close_post_reclaim` machine check specifies" ) + fixture = REPO_ROOT / "tests" / "integration" / "test_post_reclaim_fence.py" + assert fixture.exists(), f"Core 7: the discriminating fixture {fixture.name} is gone" + names = function_names(fixture) + for half in ( + "test_stale_holder_close_is_refused_after_a_real_reap", + "test_stale_holder_close_is_refused_after_release_without_reclaim", + "test_integrator_close_of_a_reclaimed_item_still_succeeds", + ): + assert half in names, ( + f"Core 7: {fixture.name} no longer carries {half} -- both halves must stay " + f"measured together, or a fix to one silently trades away the other" + ) + # --------------------------------------------------------------- CCV1-011 diff --git a/ledger/rows.yaml b/ledger/rows.yaml index ac2805f..ca575b7 100644 --- a/ledger/rows.yaml +++ b/ledger/rows.yaml @@ -238,24 +238,35 @@ A close operation is guarded against a holder that no longer holds the item. This includes the released-but-not-yet-re-claimed state (after reclaim has moved the item to open status, but custody has not yet been reassigned). - disposition: VIOLATION - work: work_item_pipeline-dn4 + disposition: CONFORMS assertion: kind: probe ref: test_row_ccv1_009 notes: > - The flagship divergence. `Beads.resolve`'s entire fence block runs only - under `if current.status == "held":`; a reaped item is `open` with the - assignee cleared and the custody record still naming the old holder, so - a stale holder's close skips the fence and lands. adapter.py's own - docstring names this exact shape as the previously-MEASURED bug the - fence was built to close -- the status gate added in PR #51 (integrator - resolve, item pipeline-79t) reinstated it. Neither doctor check covers - it: `custody.fenced` and `resolve.fenced` both stage a TAKEOVER, so - status is `held` when the fence is tested. Evidence brief sec.D-2; - ratified Call 3 (option A). A correct fix must keep BOTH halves: refuse - the stale holder, and still allow an integrator's single-call resolve - of an item nobody holds. + Closed by work_item_pipeline-dn4. The fence is no longer keyed on the + item's status at all -- it is keyed on custody IDENTITY, so it reaches + every post-reclaim state including the released-but-not-yet-re-claimed + one: adapter.py:3168-3175, the `elif cust_holder == who and + current.holder != who` branch OUTSIDE the `status == "held"` gate, + refusing with "not held by this session ... Your claim was reclaimed + (or released) while you were away." PR #51's integrator resolve (item + pipeline-79t) is preserved by the same key: only the session the + custody record names is refused, so a plain close of an item nobody + holds stays a single call for everyone else. The `current.holder != + who` guard is what keeps a holder's own already-landed close + re-attemptable (PR #63's phantom-conflict recovery) -- a resolved item + retains its assignee. MEASURED 2026-09-02, not merely inspected: + tests/integration/test_post_reclaim_fence.py is red on the pre-fix code + (`DID NOT RAISE FencedError` on both fence halves) and green after, + with test_resolve_fence.py (the integrator half) and + test_phantom_conflict_recovery.py green throughout. The probe below is + still an in-process SHAPE assertion -- it additionally verifies that + fixture exists and still carries both halves, but the behavioural proof + is the fixture, not the probe (see CCV1-020). Same fix flips the + previously-red tool-layer fixture + modules/tool-work-tracker/tests/test_reap_recovery.py::test_explicit_resolve_refusal_after_reap_clears_held_and_allows_new_claim + green (verified 2026-09-02 in a throwaway venv; that suite still runs + in no gate -- CCV1-022). - id: CCV1-010 title: a session that lost custody can discover it and recover in-process diff --git a/src/amplifier_work_tracker/adapter.py b/src/amplifier_work_tracker/adapter.py index a922d83..ea5ae52 100644 --- a/src/amplifier_work_tracker/adapter.py +++ b/src/amplifier_work_tracker/adapter.py @@ -3093,43 +3093,71 @@ def list_bounded( def resolve(self, item_id: str, reason: str, *, actor: str | None = None) -> Item: """Close an item and VERIFY the write landed. Exit code is not proof. - FENCED, but only while the item is ACTUALLY currently held -- - resolving an unheld item (open/blocked/deferred/resolved), even one - filed or last held by someone else entirely, is a single call, no - fence, no override needed. Without the status gate below, a STALE - custody record from a hold that ended long ago (released, reaped, - or simply never re-claimed) would keep naming a "current holder" - who no longer holds anything -- refusing an integrator's plain - resolve of someone else's already-unheld report. That was the - measured bug (work_tracker item 79t): the custody-based fence used - to fire on custody metadata ALONE, with no check that the item was - still `held` at all. - - Two fence sources, checked together ONLY when `current.status == - "held"`, because they cover different gaps: bd's own assignee - catches a live takeover by another holder (assignee is now someone - else). It does NOT catch our own custody-based reclaim, because - reclaiming an item clears bd's assignee back to empty rather than - reassigning it -- measured: a stale holder's resolve on a - released-but-not-yet-reclaimed item sailed through with exit 0 - because "no current holder" looked the same as "never held at - all." A custody record, once it exists, is left in place across a - reclaim precisely so it can still answer "who held this last" -- - so when one exists AND the item is still held, it is authoritative - over bd's own (now-cleared) assignee field. The refusal always - names the real holder -- the exact recovery is to resolve as that - holder, or wait for `reap` to reclaim a stale hold. + FENCED in every state a caller can no longer legitimately close + from -- but the fence is keyed on WHO the custody record names, + never on the item's status alone. Both halves matter, and each one + was, at some point, a measured bug: + + HALF ONE -- an integrator's plain resolve must stay a single call. + Resolving an item nobody currently holds (open/blocked/deferred/ + resolved), even one filed or last held by someone else entirely, + needs no fence and no override. The fence used to fire on custody + metadata ALONE, so a STALE record from a hold that ended long ago + kept naming a "current holder" who held nothing, refusing an + integrator's close of someone else's already-unheld report + (work_tracker item 79t, PR #51). Anyone who is not the session that + record names is exactly as unfenced as before. + + HALF TWO -- the stale holder itself is refused in EVERY + post-reclaim state, including the released-but-not-yet-re-claimed + one (contract `custody-coordination.v1` Core 7; ledger row + CCV1-009; work_tracker item pipeline-dn4). 79t's fix reached for + the item's status as its discriminator (`status == "held"`), which + reinstated the very bug the fence exists to close: `reap` does not + leave a reclaimed item `held` -- `supervisor.reap_project` calls + `release`, which puts it back to `open` and clears bd's assignee -- + so the one state the fence is FOR was the one state it skipped, and + the stale holder's close landed with exit 0 and no refusal + anywhere. Status was never the right question; custody identity is. + + Hence the three checks below, in the order they can be answered: + + - held, with a custody record: that record is authoritative over + bd's own assignee, because a reclaim CLEARS the assignee rather + than reassigning it -- "no current holder" and "never held at + all" look identical in bd, and only the custody record can tell + them apart. Refuse unless BOTH name this caller. + - held, no custody record: fall back to bd's assignee alone -- + enough to catch a live takeover by another session. + - NOT held, but the custody record still names this caller: this + session's hold ended without this session closing the item -- + reclaimed by `reap`, or handed back by `release`, which are + indistinguishable by construction (see `agent_stats`, which + documents why). Either way it does not hold the item now, and + its close is refused. + + A custody record is deliberately left in place across a reclaim, so + it can still answer "who held this last" -- that is what makes the + third check possible at all. The `current.holder != who` guard on + it is what keeps a holder's own already-landed close re-attemptable + (a resolved item retains its assignee, so a wedged session + confirming its own close is never mistaken for a reclaimed one -- + see this method's verify-by-read-back branch below). + + Every refusal names the real holder and the recovery: re-claim the + item, or wait for `reap` to reclaim a stale hold. """ who = actor or self._actor if who: current = self.get(item_id) + cust = current.meta.get(C.CUSTODY_KEY) if isinstance(current.meta, dict) else None + cust_holder = cust.get("holder") if isinstance(cust, dict) else None if current.status == "held": - cust = current.meta.get(C.CUSTODY_KEY) if isinstance(current.meta, dict) else None - if isinstance(cust, dict) and cust.get("holder"): - if current.holder != who or cust.get("holder") != who: + if cust_holder: + if current.holder != who or cust_holder != who: raise FencedError( f"refusing to close {item_id}: current holder is " - f"{current.holder!r} (custody holder {cust.get('holder')!r}), " + f"{current.holder!r} (custody holder {cust_holder!r}), " f"not {who!r}. Your claim was reclaimed while you were away." ) elif current.holder and current.holder != who: @@ -3137,6 +3165,14 @@ def resolve(self, item_id: str, reason: str, *, actor: str | None = None) -> Ite f"refusing to close {item_id}: it is held by {current.holder!r}, " f"not {who!r}. Your claim was reclaimed while you were away." ) + elif cust_holder == who and current.holder != who: + raise FencedError( + f"refusing to close {item_id}: not held by this session -- custody " + f"names {who!r} as its last holder, but the item is now " + f"{current.status!r} with no current holder. Your claim was " + f"reclaimed (or released) while you were away. Re-claim it first, " + f"then resolve." + ) try: p = self._run(["close", item_id, "--reason", reason], actor=actor) except BeadsError: diff --git a/tests/integration/test_post_reclaim_fence.py b/tests/integration/test_post_reclaim_fence.py new file mode 100644 index 0000000..454b2d5 --- /dev/null +++ b/tests/integration/test_post_reclaim_fence.py @@ -0,0 +1,172 @@ +"""Tier 2 -- Core 7 of `contracts/custody-coordination.v1.md`: a close is +fenced against a stale holder in EVERY post-reclaim state, including the +released-but-not-yet-re-claimed one (conformance ledger row CCV1-009, +work item `pipeline-dn4`). + +The gap this file discriminates: `Beads.resolve`'s custody fence used to run +only under `if current.status == "held"`. A reap does NOT leave the item +`held` -- `supervisor.reap_project` calls `Beads.release`, which sets the +item back to `open` and clears bd's own assignee, while deliberately leaving +the custody record in metadata still naming the old holder. So the one state +the fence exists for -- "my claim was taken away while I was idle" -- was +precisely the state the fence skipped, and the stale holder's close landed +with exit 0 and no refusal anywhere. + +BOTH halves are pinned here, because the two are easy to trade against each +other and the fix is only correct if it keeps both: + + - the STALE HOLDER's close of a reclaimed item is refused (the fence), and + - an INTEGRATOR's close of that same unheld item still succeeds in a + single call, no fence, no override (PR #51, work item `pipeline-79t`). + +The discriminator between the two is the custody record's `holder`, not the +item's status: only the session the custody record still names is refused. +Everyone else is exactly as unfenced as they were before. + +See also `test_resolve_fence.py`, which pins the integrator half against +a *manual* release; this file drives the REAL reap sweep as well, so the +released-but-not-re-claimed state is produced by the code that actually +produces it in production rather than by a test's imitation of it. +""" + +from __future__ import annotations + +import pytest + +from amplifier_work_tracker import adapter as A +from amplifier_work_tracker import supervisor as SV + +pytestmark = pytest.mark.integration + + +def _custody_holder(bd: A.Beads, item_id: str) -> str | None: + rec = bd.get(item_id).meta.get(A.C.CUSTODY_KEY) + return rec.get("holder") if isinstance(rec, dict) else None + + +# -------------------------------------------------------------------------- +# The fence half -- the stale holder is refused in the post-reclaim state. +# -------------------------------------------------------------------------- + + +def test_stale_holder_close_is_refused_after_a_real_reap(project_factory): + """The flagship case, driven through the REAL sweep. + + Claim -> take custody -> `supervisor.reap_project(ttl_seconds=0)` (every + hold is instantly stale, so no real sleep is needed) -> the reclaimed + holder tries to close. The item is `open` with no assignee and a custody + record still naming the stale holder: the exact shape that used to skip + the fence entirely. + + Its own project, not `shared_bd`, because `reap_project` sweeps a whole + project -- pointed at the session-shared one it could reclaim another + test's live hold. + """ + _name, bd = project_factory("reapfence") + item_id = bd.create("post-reclaim fence probe: real reap", priority=1) + bd.claim_item(item_id, actor="stale-holder") + bd.take_custody(item_id, holder="stale-holder", pid=1, host="test-host") + + reaped = SV.reap_project(bd, ttl_seconds=0) + assert reaped["reclaimed_count"] == 1 + assert reaped["reclaimed"][0]["id"] == item_id + + # The state under test: released, not yet re-claimed, custody record left + # in place naming the holder that no longer holds anything. + after = bd.get(item_id) + assert after.status == "open" + assert after.holder is None + assert _custody_holder(bd, item_id) == "stale-holder" + + with pytest.raises(A.FencedError) as exc: + bd.resolve( + item_id, "closing work that was reclaimed while I was away", actor="stale-holder" + ) + message = str(exc.value).lower() + assert "not held by this session" in message + assert "reclaim" in message + + # The refusal must not have closed it anyway. + assert bd.get(item_id).status == "open" + + +def test_stale_holder_close_is_refused_after_release_without_reclaim(shared_bd, unique_lane): + """Same refusal, reached by the exact single call the reap sweep makes + (`Beads.release` -- see `supervisor.reap_project`), so the fence is + proven against the state itself rather than against one caller of it. + A voluntary hand-back lands in the identical state and is likewise + refused: after a release this session does not hold the item, and the + two are indistinguishable by construction (a release records no reason + for why it happened -- see `adapter.agent_stats`'s own docstring). + """ + item_id = shared_bd.create("post-reclaim fence probe: released", tags=[unique_lane], priority=1) + shared_bd.claim_item(item_id, actor="released-holder") + shared_bd.take_custody(item_id, holder="released-holder", pid=1, host="test-host") + shared_bd.release(item_id) + + with pytest.raises(A.FencedError) as exc: + shared_bd.resolve(item_id, "closing after my hold ended", actor="released-holder") + assert "released-holder" in str(exc.value) + assert shared_bd.get(item_id).status == "open" + + +# -------------------------------------------------------------------------- +# The integrator half -- PR #51's use case must keep working, unchanged. +# -------------------------------------------------------------------------- + + +def test_integrator_close_of_a_reclaimed_item_still_succeeds(project_factory): + """PR #51 (`pipeline-79t`): resolving an item nobody currently holds is + a single call for anyone who is not the stale holder -- including after + a real reap, which is when unfinished reports most need closing out. + """ + _name, bd = project_factory("intfence") + item_id = bd.create("post-reclaim fence probe: integrator", priority=1) + bd.claim_item(item_id, actor="stale-holder") + bd.take_custody(item_id, holder="stale-holder", pid=1, host="test-host") + + assert SV.reap_project(bd, ttl_seconds=0)["reclaimed_count"] == 1 + assert _custody_holder(bd, item_id) == "stale-holder" + + back = bd.resolve(item_id, "closed out by the integrator", actor="integrator") + assert back.status == "resolved" + + +def test_current_holder_can_still_close_the_item_it_actually_holds(shared_bd, unique_lane): + """The fence must never refuse the legitimate holder: claim, take + custody, close -- one call, no fence, custody record naming this very + session notwithstanding. + """ + item_id = shared_bd.create( + "post-reclaim fence probe: live holder", tags=[unique_lane], priority=1 + ) + shared_bd.claim_item(item_id, actor="live-holder") + shared_bd.take_custody(item_id, holder="live-holder", pid=1, host="test-host") + + back = shared_bd.resolve(item_id, "finished the work I actually hold", actor="live-holder") + assert back.status == "resolved" + + +def test_fence_does_not_fire_on_a_close_this_holder_already_landed(shared_bd, unique_lane): + """Regression guard for the phantom-conflict recovery path (PR #63): a + holder whose close ALREADY landed may re-attempt it -- that item is + `resolved`, not `held`, and its custody record still names this very + session. The new post-reclaim fence must not mistake "I already closed + this" for "my claim was taken away", or a wedged session could never + confirm its own landed close. bd itself may or may not accept a second + close of a closed item; either is fine, a `FencedError` is not. + """ + item_id = shared_bd.create("post-reclaim fence probe: rewrite", tags=[unique_lane], priority=1) + shared_bd.claim_item(item_id, actor="retrying-holder") + shared_bd.take_custody(item_id, holder="retrying-holder", pid=1, host="test-host") + assert shared_bd.resolve(item_id, "landed the first time", actor="retrying-holder").status == ( + "resolved" + ) + + try: + shared_bd.resolve(item_id, "landed the first time", actor="retrying-holder") + except A.FencedError as e: # pragma: no cover - only reached on regression + pytest.fail(f"a holder's re-close of its own landed close was fenced: {e}") + except A.BeadsError: + pass # bd declining to re-close an already-closed item is not a fence + assert shared_bd.get(item_id).status == "resolved" From 546c8c7651f8cda65db9e9e1cdbf8fdc7b615736 Mon Sep 17 00:00:00 2001 From: amplifier-lane Date: Wed, 2 Sep 2026 21:14:54 -0700 Subject: [PATCH 2/7] fix(custody): compensate a claim whose take_custody fails (Core 3 / CCV1-003) `work_claim` is one call but two writes -- the bd claim, then `take_custody`. When the second failed, the tool returned an honest-looking failure and walked away, leaving the item HELD by that actor with NO custody record and no session tracking it: invisible to `work_release` (the session never set `self._held`, so it refuses), and freed only by the next reap sweep -- up to a custody TTL later, and only where a sweep runs at all. The claim landed, so the only honest recovery is to give the item back. The failing arm now routes to `_release_after_failed_custody`, which: - releases the just-claimed item via `adapter.Beads.release` (status checked BEFORE any write, so it can never reopen a closed item; read-back-verified on the conflict path since PR #63); - confirms that release by its OWN contention-free read-back (`get_readonly` -> `_get_item_via_sql`, a pure SELECT), because a write's self-report of success is exactly what this repo has repeatedly measured to be unreliable; - reports both facts: "claim landed; custody could not be established; item released back to ready: ". The one residual case -- the compensating release ITSELF failing -- is never silent: the message says the item may still be held, names the id and the actor, and says what to do about it. Same for a release that reports success but cannot be confirmed by read-back. Tests (real bd/dolt, this suite's isolated server) cover both branches and both FAIL against the pre-fix code. Ledger row CCV1-003 flips VIOLATION -> CONFORMS with file:line + fixture evidence, and its probe is rewritten to assert the compensation rather than pin the broken shape. --- ledger/checks/test_custody_rows.py | 58 +++++- ledger/rows.yaml | 40 ++++- .../__init__.py | 102 ++++++++++- .../tests/test_custody_atomic.py | 165 ++++++++++++++++++ 4 files changed, 347 insertions(+), 18 deletions(-) create mode 100644 modules/tool-work-tracker/tests/test_custody_atomic.py diff --git a/ledger/checks/test_custody_rows.py b/ledger/checks/test_custody_rows.py index fba0461..1e35d2a 100644 --- a/ledger/checks/test_custody_rows.py +++ b/ledger/checks/test_custody_rows.py @@ -70,23 +70,65 @@ def test_row_ccv1_000() -> None: def test_row_ccv1_003() -> None: - """Core 3 VIOLATION pin: a failed `take_custody` after a successful claim - returns a failure and leaves the item HELD with no custody record -- - no release, no rollback. + """Core 3 CONFORMS: a `take_custody` failure after a successful claim + COMPENSATES -- it releases the just-claimed item, confirms that release + by its own contention-free read-back, and reports both facts. + + Three separable things must all hold, so each is asserted separately: + the failing arm calls the compensation (rather than returning), the + compensation actually releases AND independently verifies, and the + residual case -- a compensating release that itself fails -- stays + loud. A fix that quietly dropped the verification read, or softened the + still-held message into a rollback claim, would pass a single blunt + check and fail these. + + Behavioral proof lives in the tool module's own suite (real bd/dolt); + this kit is in-process only, so what it can prove is the SHAPE. See the + row's `notes` for the fixture names and the honest limit. """ assert contains( TOOL_MODULE, """ - except A.BeadsError as e: return ToolResult( success=False, - output=f"claimed {item.id} but could not establish custody: {e}", + output=self._release_after_failed_custody(bd, item.id, e), ) """, ), ( - "CCV1-003 (Core 3, VIOLATION) pin no longer matches. If the claim/custody " - "two-write hole was CLOSED, this is the expected failure: flip the row to " - "CONFORMS, cite the discriminating fixture, and resolve work_item_pipeline-aih." + "CCV1-003 (Core 3): the failing `take_custody` arm of `claim` no longer " + "routes to the compensating release. If it returns a bare failure again, " + "the two-write hole is BACK: the item stays held with no custody record " + "until a reap sweep frees it (work_item_pipeline-aih)." + ) + assert contains( + TOOL_MODULE, + """ + outcome = bd.release(item_id) + """, + ) and contains( + TOOL_MODULE, + """ + back = bd.get_readonly(item_id) + """, + ), ( + "CCV1-003 (Core 3): the compensation must both RELEASE the claim and " + "verify by its OWN read-back -- a write's self-report of success is " + "exactly what this repo has repeatedly measured to be unreliable." + ) + # Matched as fragments, not whole sentences: these messages are built + # from adjacent f-string literals, so the source text a reader sees as + # one sentence carries a `" f"` seam that whitespace-collapsing cannot + # remove. Each fragment is still specific enough that a reworded + # message fails here. + assert contains( + TOOL_MODULE, "claim landed; custody could not be established; item released back to " + ), "CCV1-003 (Core 3): the success-path message must name BOTH facts" + assert contains(TOOL_MODULE, "release ALSO FAILED --") and contains( + TOOL_MODULE, "may STILL BE HELD by" + ), ( + "CCV1-003 (Core 3): a compensating release that itself fails must stay " + "loud -- it is the one path that can still leave an item held, and it " + "must never be reported as a rollback that happened." ) diff --git a/ledger/rows.yaml b/ledger/rows.yaml index ac2805f..5376fca 100644 --- a/ledger/rows.yaml +++ b/ledger/rows.yaml @@ -86,26 +86,48 @@ duration is never an input. - id: CCV1-003 - title: a failed take_custody leaves the item held with no custody record + title: a claim whose custody step fails is compensated -- the item goes back to ready contract: file: contracts/custody-coordination.v1.md clause: Core 3 quote: | A claim that cannot establish custody must not leave the work item in a held state with no custody record. - disposition: VIOLATION + disposition: CONFORMS work: work_item_pipeline-aih assertion: kind: probe ref: test_row_ccv1_003 notes: > - Two sequential writes (claim, then take_custody) with no rollback on - the second. The tool returns success=False naming the failure, but the - item stays held by this actor with no custody record and no session - tracking it -- only a running reap sweep frees it (up to 300s later, - and only where a sweep runs at all). Evidence brief sec.D-1; ratified - Call 4 (option B): Core, contract-then-fix. The probe pins the current - broken shape on purpose. + CLOSED by work_item_pipeline-aih (was VIOLATION; the probe pinned the + broken two-write shape). `work_claim` is still two writes, but the + second one's failure now COMPENSATES: the just-claimed item is released + via the adapter's read-back-verified `release`, that release is + confirmed by the tool's OWN contention-free read-back + (`get_readonly` -> `_get_item_via_sql`, a pure SELECT), and the failure + text names both facts ("claim landed; custody could not be established; + item released back to ready: "). The one residual case -- the + compensating release ITSELF failing -- is reported loudly, naming the + item id and the actor that may still hold it, never silently. + Evidence (file:line): + modules/tool-work-tracker/amplifier_module_tool_work_tracker/__init__.py:293 + (`WorkTrackerSession._release_after_failed_custody`; release :339, + own read-back :350, released-back-to-ready text :375) + modules/tool-work-tracker/amplifier_module_tool_work_tracker/__init__.py:448 + (the `except A.BeadsError` arm of `claim` that calls it) + Behavioral fixtures (real bd/dolt, this suite's isolated server; run + 2026-09-02, both passing, and both FAIL against the pre-fix code -- + verified by reverting the module file and re-running): + modules/tool-work-tracker/tests/test_custody_atomic.py:72 + test_failed_take_custody_releases_the_claim_back_to_ready + modules/tool-work-tracker/tests/test_custody_atomic.py:127 + test_failed_take_custody_then_failed_release_says_the_item_may_still_be_held + HONEST LIMIT: the probe below is a source-shape assertion (this ledger + kit is in-process only -- no bd, no subprocess), and the fixtures above + live in the tool module's separately-packaged suite, which still runs + in no CI (CCV1-022). Command used: `cd modules/tool-work-tracker && + .venv/bin/python -m pytest tests/test_custody_atomic.py -q`. Upgrade + this row to an `indexed` cite when CCV1-022 goes green. - id: CCV1-004 title: custody renewal is one-strike; any failure ends renewal for good diff --git a/modules/tool-work-tracker/amplifier_module_tool_work_tracker/__init__.py b/modules/tool-work-tracker/amplifier_module_tool_work_tracker/__init__.py index 4863027..368474a 100644 --- a/modules/tool-work-tracker/amplifier_module_tool_work_tracker/__init__.py +++ b/modules/tool-work-tracker/amplifier_module_tool_work_tracker/__init__.py @@ -290,6 +290,92 @@ def _renew_loop(self, held: _Held) -> None: # ------------------------------------------------------------ tools + def _release_after_failed_custody(self, bd: A.Beads, item_id: str, cause: A.BeadsError) -> str: + """Undo a claim whose custody step failed, and describe what really + happened -- the compensating half of `claim`'s "one indivisible + call" promise (contract `Core 3`, ledger row CCV1-003). + + `work_claim` is one call but TWO writes: the bd claim, then + `take_custody`. Before this compensation existed, a `take_custody` + failure after a successful claim returned a plain failure and left + the item HELD by this actor with NO custody record and no session + tracking it -- invisible to `work_release` (this session never set + `self._held`, so it refuses), and freed only by the next reap sweep, + up to a custody TTL later and only where a sweep runs at all. The + claim landed, so the only honest recovery is to give the item back. + + Order, and why: `adapter.Beads.release` first (it checks status + BEFORE any write, so it can never reopen an already-closed item, and + it is read-back-verified on the conflict path -- PR #63), then OUR + OWN read-back via the contention-free read path (`get_readonly` -> + `_get_item_via_sql`, a pure SELECT that cannot lose a serialization + conflict). The second read is not redundant: a write's own report of + success is exactly what this bundle has repeatedly measured to be + unreliable, and "released" is the fact the caller will act on. + + Every branch returns text naming BOTH facts -- that the claim + landed, and what became of the item -- because the one outcome that + must never be silent is the compensating release ITSELF failing: + then the item may still be held by this actor, and the message says + so, names the id, and says what to do about it. + + Custody-record note: a `take_custody` that failed at its own + read-back verification may still have LANDED its metadata write, so + a released item can carry a stale custody record. That is benign -- + the record is inert on an `open` item (nothing renews it, no sweep + acts on it), and the next `take_custody` bumps `generation` past it + (see `adapter.Beads.take_custody`). Clearing it would mean a new + adapter write verb; the item's STATUS is what "released back to + ready" means, and that is what this verifies. + + Called with `self._lock` already held (from `claim`); acquires + nothing itself. Returns the message rather than raising, because a + failed `ToolResult` is this module's loud-error channel (see + `_guard.guarded` and `test_result_guard.py`). + """ + try: + outcome = bd.release(item_id) + except A.BeadsError as release_error: + return ( + f"claim landed; custody could not be established; the compensating " + f"release ALSO FAILED -- {item_id} may STILL BE HELD by " + f"{self._actor!r} with no custody record. Re-read it " + f"(work_list item_id={item_id!r}) and release it explicitly before " + f"claiming again. custody failure: {cause}; release failure: " + f"{release_error}" + ) + try: + back = bd.get_readonly(item_id) + except A.BeadsError as read_error: + return ( + f"claim landed; custody could not be established; the compensating " + f"release reported success but COULD NOT BE CONFIRMED by read-back, " + f"so {item_id} may still be held by {self._actor!r}. Re-read it " + f"before claiming again. custody failure: {cause}; read-back " + f"failure: {read_error}" + ) + if back.status == "held" or back.holder == self._actor: + return ( + f"claim landed; custody could not be established; the compensating " + f"release reported success but read-back shows {item_id} is STILL " + f"status={back.status!r} holder={back.holder!r} -- it may still be " + f"held by {self._actor!r}. Release it explicitly before claiming " + f"again. custody failure: {cause}" + ) + if outcome.already_closed: + return ( + f"claim landed; custody could not be established; no release was " + f"needed -- {item_id} read back as already closed " + f"(status={back.status!r}), so nothing is left held. custody " + f"failure: {cause}" + ) + return ( + f"claim landed; custody could not be established; item released back to " + f"ready: {cause} (read-back confirms {item_id} is now " + f"status={back.status!r} holder={back.holder!r}; nothing is held by " + f"{self._actor!r})" + ) + async def claim(self, project: str, *, item_id: str | None = None) -> ToolResult: """Claim work and establish custody in one indivisible call. @@ -303,6 +389,15 @@ async def claim(self, project: str, *, item_id: str | None = None) -> ToolResult already held by someone else, does not exist, or is blocked by an open dependency -- no override; resolve the blocker or claim again. + + "Indivisible" is enforced, not merely asserted: the call is two + writes (bd claim, then `take_custody`), so a `take_custody` failure + after a successful claim COMPENSATES -- the just-claimed item is + released back to ready and that release is confirmed by our own + read-back before the failure is reported. See + `_release_after_failed_custody` for the full contract (Core 3) and + for the one case that can still leave something held: a compensating + release that itself fails, which is reported loudly and by id. """ with self._lock: if self._held is not None: @@ -341,9 +436,14 @@ async def claim(self, project: str, *, item_id: str | None = None) -> ToolResult host=socket.gethostname(), ) except A.BeadsError as e: + # The bd claim ALREADY LANDED. Returning here without undoing + # it is what left an item held-with-no-custody (contract + # Core 3, ledger row CCV1-003): invisible to work_release, + # untracked by any session, freed only by a reap sweep. Give + # it back, verify by our own read-back, and report both facts. return ToolResult( success=False, - output=f"claimed {item.id} but could not establish custody: {e}", + output=self._release_after_failed_custody(bd, item.id, e), ) held = _Held( project=project, diff --git a/modules/tool-work-tracker/tests/test_custody_atomic.py b/modules/tool-work-tracker/tests/test_custody_atomic.py new file mode 100644 index 0000000..1af5be3 --- /dev/null +++ b/modules/tool-work-tracker/tests/test_custody_atomic.py @@ -0,0 +1,165 @@ +"""Claim/custody atomicity -- contract `Core 3`, ledger row CCV1-003, +work_tracker item pipeline-aih. + +`work_claim` is ONE call but TWO writes: the bd claim, then `take_custody`. +The hole these tests close: when the second write failed, the tool returned +an honest-looking failure and walked away, leaving the item HELD by this +actor with NO custody record and no session tracking it -- invisible to +`work_release` (this session never set `self._held`, so it refuses), and +freed only by the next reap sweep, up to a custody TTL later and only where +a sweep runs at all. + +Both branches of the compensation are covered, because the dangerous one is +the second: + + 1. compensating release SUCCEEDS -- the item is back on the queue, no + custody record, and another actor can claim it immediately; + 2. compensating release ITSELF FAILS -- the item really may still be held, + and the message must say exactly that, name the id, and never imply a + rollback that did not happen. + +Real `bd`/dolt end-to-end against this suite's isolated server (skipped if +`bd` is not on PATH, matching this module's other tests): the point is what +the STORAGE layer holds afterwards, which no mock can prove. +""" + +from __future__ import annotations + +import shutil +import uuid +from typing import Any + +import pytest +from amplifier_module_tool_work_tracker import WorkTrackerSession + +import amplifier_work_tracker.adapter as A + +pytestmark = pytest.mark.skipif( + shutil.which("bd") is None, reason="real `bd` binary not present in this environment" +) + + +def _unique(prefix: str) -> str: + return f"{prefix}{uuid.uuid4().hex[:10]}" + + +#: Consumed by the shared `project` fixture in conftest.py, which creates +#: the project AND drops its isolated-server database again on teardown. +PROJECT_PREFIX = "custatomproj" + +_INJECTED_CUSTODY = "injected take_custody failure" +_INJECTED_RELEASE = "injected release failure" + + +def _explode_take_custody(self, *args, **kwargs): # noqa: ANN001, ANN002, ANN003, ARG001 + raise A.BeadsError(_INJECTED_CUSTODY) + + +def _explode_release(self, *args, **kwargs): # noqa: ANN001, ANN002, ANN003, ARG001 + raise A.BeadsError(_INJECTED_RELEASE) + + +async def _seed(project: str, title: str) -> str: + """One ready `lane:eng` item, filed by an unrelated actor.""" + added = await WorkTrackerSession({"actor": _unique("seeder")}).add( + project, title, acceptance="n/a" + ) + assert added.success is True + return added.output["added"] # type: ignore[index] + + +@pytest.mark.asyncio +async def test_failed_take_custody_releases_the_claim_back_to_ready(project): + """The whole of Core 3 in one path: claim lands, custody cannot be + established, and the item is BACK ON THE QUEUE before `work_claim` + returns -- proven by reading the storage layer, not by trusting the + tool's own report. + """ + item_id = await _seed(project, "custody-atomicity probe") + + actor = _unique("atomicactor") + session = WorkTrackerSession({"actor": actor}) + # A SCOPED monkeypatch, not the `monkeypatch` fixture: the shared + # `project` fixture uses that same function-scoped instance to set + # AMPLIFIER_WORK_TRACKER_ROOT, so an `undo()` mid-test would also unset + # the workspace root out from under the rest of the test. + with pytest.MonkeyPatch.context() as mp: + mp.setattr(A.Beads, "take_custody", _explode_take_custody) + result = await session.claim(project) + + # (iii) the caller is told, loudly, BOTH facts -- and the tool's own + # error channel is a failed ToolResult (see _guard.guarded), never a + # raised exception a caller has to catch. + assert result.success is False + text: str = result.output # type: ignore[assignment] + assert "claim landed" in text + assert "custody could not be established" in text + assert "released back to ready" in text + assert _INJECTED_CUSTODY in text, "the underlying reason must survive into the message" + assert item_id in text + + # (i) the item is ready/open again, held by nobody -- read back through + # the contention-free read path, from a session that did no writing. + reader = WorkTrackerSession({"actor": _unique("reader")})._project(project) # noqa: SLF001 + back = reader.get_readonly(item_id) + assert back.status == "open", f"expected the claim to be undone, got status={back.status!r}" + assert not back.holder, f"expected no holder, got {back.holder!r}" + + # (ii) and no custody record was left behind for it. + assert reader.get_custody(item_id) is None + + # The failed claim also left NO local state to wedge this session. + assert session._held is None # noqa: SLF001 + + # (iv) another actor can claim the very same item immediately -- no + # reap sweep, no TTL wait. This is the fact that matters operationally. + other = WorkTrackerSession({"actor": _unique("otheractor")}) + reclaimed = await other.claim(project, item_id=item_id) + assert reclaimed.success is True, reclaimed.output + out: dict[str, Any] = reclaimed.output # type: ignore[assignment] + assert out["claimed"] == item_id + assert reader.get_custody(item_id) is not None + + await other.resolve(item_id, "test cleanup") + + +@pytest.mark.asyncio +async def test_failed_take_custody_then_failed_release_says_the_item_may_still_be_held( + project, +): + """The branch that must never be silent: the compensating release ITSELF + fails. The item genuinely IS still held, so the message must say so, + name the id, and never claim a rollback that did not happen. + """ + item_id = await _seed(project, "compensating-release failure probe") + + actor = _unique("stuckactor") + session = WorkTrackerSession({"actor": actor}) + with pytest.MonkeyPatch.context() as mp: # scoped -- see the test above + mp.setattr(A.Beads, "take_custody", _explode_take_custody) + mp.setattr(A.Beads, "release", _explode_release) + result = await session.claim(project) + + assert result.success is False + text: str = result.output # type: ignore[assignment] + assert "claim landed" in text + assert "custody could not be established" in text + assert "compensating release ALSO FAILED" in text + assert "may STILL BE HELD" in text + assert item_id in text, "an item that may still be held must be named by id" + assert actor in text, "and so must the actor holding it" + assert _INJECTED_CUSTODY in text + assert _INJECTED_RELEASE in text + # It must NOT claim a rollback that did not happen. + assert "released back to ready" not in text + + # The message is honest: the item really is still held by this actor. + reader = WorkTrackerSession({"actor": _unique("reader")})._project(project) # noqa: SLF001 + back = reader.get_readonly(item_id) + assert back.status == "held" + assert back.holder == actor + assert reader.get_custody(item_id) is None + + # And the operator-facing recovery the message points at really works. + reader.release(item_id) + assert reader.get_readonly(item_id).status == "open" From b2c80a44bae7535e360fac72a29a18e8884eeab1 Mon Sep 17 00:00:00 2001 From: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com> Date: Wed, 2 Sep 2026 21:17:01 -0700 Subject: [PATCH 3/7] docs: custody prose matches shipped code + contract (CCV1-005/008/016) Three agent-facing claims contradicted the custody-coordination contract and the code that ships today. All three are corrected, and each row's ledger probe is flipped from pinning the stale wording to pinning the corrected wording (plus an absence-assert on the phrase it replaced), so a regression fails as loudly as the drift did. Core 4 (CCV1-005) -- renewal is one-strike. The skill told agents "you do not need to do anything to keep it fresh under normal operation", inverted for exactly the failure Core 4 names. Both surfaces now state that a single failed renewal ends renewal permanently, that a non-fenced failure leaves the session believing it still holds the item, and that the only discovery path is work_status's holding.custody_lost -- checked before long-running steps and after any tool error. Core 6 (CCV1-008) -- the TTL is not self-enforcing. Both documents stated the 15-minute release unconditionally and never mentioned the sweep. They now state reclaim-ELIGIBILITY enforced by the out-of-band reap sweep: a reclaim lands up to a sweep interval late (300s default), and where no sweep runs a dead agent's hold persists indefinitely. The skill's constants table gains the reap interval, which it had omitted entirely. Core 11 (CCV1-016) -- a reported conflict is UNKNOWN, not proof of failure. Drift created by PR #63 itself: awareness.md (and the CLI's own CONTENTION / RETRY CONTRACT, which the original row did not cover) still stated as a database guarantee that a serialization error means the write did not happen. Incident B measured the opposite. Both now scope the guarantee honestly -- resolve/release verify by read-back and report success when a conflicted write landed; every other write verb surfaces the raw conflict unverified -- while keeping the re-read-before-retry guidance that was always correct. Text only in src/amplifier_work_tracker/cli.py (the module docstring is argparse's description); no behaviour changed anywhere. Rows CCV1-005, CCV1-008, CCV1-016: GAP -> CONFORMS with file:line evidence. Gates: ruff check, ruff format --check, pyright (0 errors), pytest ledger/checks (24 passed). --- context/awareness.md | 69 +++++++++----- ledger/checks/test_custody_rows.py | 137 +++++++++++++++++++++------ ledger/rows.yaml | 79 +++++++++------ skills/claiming-work-safely/SKILL.md | 58 ++++++++++-- src/amplifier_work_tracker/cli.py | 36 +++---- 5 files changed, 272 insertions(+), 107 deletions(-) diff --git a/context/awareness.md b/context/awareness.md index acd1a16..677c153 100644 --- a/context/awareness.md +++ b/context/awareness.md @@ -4,7 +4,7 @@ You're one of several agents pulling from a shared work queue. First use in a session, or any `work_*` call fails to connect: call `work_tracker_status` before assuming a server is running — see "Where to go next" below. -Five things here fail **silently** if you get them wrong — no error, no undo: +Six things here fail **silently** if you get them wrong — no error, no undo: 1. **Claim only via `work_claim` / `work_status`; never list-then-pick.** The obvious approach — read the ready queue, choose an item, mark it yours — @@ -12,11 +12,27 @@ Five things here fail **silently** if you get them wrong — no error, no undo: still got exit 0. `work_claim` is the single atomic claim-and-custody operation. There is no other way to take an item. -2. **Custody is a liveness signal, not a timer.** Idle time never costs you a - claim — you may sit for hours awaiting a human's answer. Only an - **unrenewed** custody signal does: 15 minutes without a renewal releases - the item back to the queue. `awaiting_human` (via `work_declare`) only - suppresses a notification — it never exempts you from that clock. +2. **Custody is a liveness signal, not a timer — and neither end of it is + automatic.** Idle time never costs you a claim; you may sit for hours + awaiting a human's answer. Only an **unrenewed** custody signal does. + Two consequences, both silent: + - **Renewal is one-strike.** It runs in the background while your + session process lives, but a single failed renewal ends renewal + permanently — there is no retry on the next tick — and this session + goes on believing it still holds the item. The only way to find out + is `work_status`: a non-null `holding.custody_lost` means renewal + stopped and the hold is on its way to being reclaimed. Check it + before any long-running step and after any tool error. + - **The TTL does not enforce itself.** After 15 minutes with no renewal + a hold is merely *reclaim-eligible*; the out-of-band `reap` sweep is + what actually reclaims it, and only where an operator has one + installed and running. Expect a reclaim to land up to a sweep + interval (300s by default) AFTER the TTL, and expect a dead agent's + hold to persist indefinitely where no sweep runs — never wait on a + stuck held item assuming it frees itself. + + `awaiting_human` (via `work_declare`) only suppresses a notification — + it never exempts you from that clock. 3. **An empty queue is a normal terminal outcome.** `work_claim` returning `claimed: null` means stop and report — not a signal to invent work or @@ -32,25 +48,30 @@ Five things here fail **silently** if you get them wrong — no error, no undo: retry resolving or declaring that item — someone else may hold it now. `work_claim` can still be used afterward to pick up new work. -6. **A reported write failure means the write did NOT land — but never - blindly retry the same operation either; re-read first.** You're sharing - a single-writer dolt server with every other agent's claims, renewals, - and resolves. A write occasionally loses a serialization race and +6. **A reported write failure does NOT prove the write failed — treat it as + UNKNOWN and re-read before you retry.** You're sharing a single-writer + dolt server with every other agent's claims, renewals, and resolves. A + write occasionally loses a serialization race and `work_resolve`/`work_file`/the CLI raises an error like "still - conflicting after 8 retries." That specific error family (dolt/MySQL - 1213/1205/"serialization failure"/"try restarting transaction") means - the transaction was aborted — by database guarantee, never partially - committed — so the write genuinely did not happen. The unsafe move is - resubmitting blind: for a non-idempotent write (creating a new item) a - blind retry after an ambiguous-looking failure can leave a duplicate. - The safe move is always the same: re-read the item first (`work_list`'s - `item_id` form, or `get_readonly` — a read-only path that cannot itself - conflict) to see its real current state, then decide whether the - original operation still needs doing. A *reported success*, by contrast, - is already independently verified — `resolve`/`unclaim` read the item - back and raise rather than report success if the change didn't actually - land — so this caution is specifically about what to do after a - *reported failure*, not a general distrust of success responses. + conflicting after 8 retries" (dolt/MySQL 1213/1205, "serialization + failure", "try restarting transaction"). Measured reality: a write that + surfaced as one of those errors can still have LANDED — an observed + incident, and the reason the read-back behaviour below exists. So: + - `work_resolve` and `work_release` already handle it for you: on a + conflict they re-read the item and report success when the write did + in fact land, and they verify their own success path by read-back + too. A *reported success* from those two is independently confirmed. + - Every other write verb (`work_add`, `work_edit`, `work_file`, + `work_defer`, `work_block`, `work_dep`, and the CLI equivalents) + still surfaces the raw conflict unverified. There, a reported failure + means *unknown*, never *didn't happen*. + + The unsafe move is resubmitting blind: for a non-idempotent write + (creating a new item) a blind retry can leave a duplicate of a write + that already landed. The safe move is always the same: re-read the item + first (`work_list`'s `item_id` form, or `get_readonly` — a read-only + path that cannot itself conflict) to see its real current state, then + decide whether the original operation still needs doing. ## Where to go next diff --git a/ledger/checks/test_custody_rows.py b/ledger/checks/test_custody_rows.py index fba0461..a3e8c04 100644 --- a/ledger/checks/test_custody_rows.py +++ b/ledger/checks/test_custody_rows.py @@ -43,6 +43,11 @@ sha256, ) +# The CLI's module docstring is argparse's `description` -- i.e. prose an +# operator reads at `--help`, and the CLI-side twin of `context/awareness.md`'s +# contention contract. Kept local to this module (only CCV1-016 pins it). +CLI = REPO_ROOT / "src" / "amplifier_work_tracker" / "cli.py" + # --------------------------------------------------------------- CCV1-000 @@ -115,35 +120,75 @@ def test_row_ccv1_004() -> None: def test_row_ccv1_005() -> None: - """Core 4 GAP pin (doc drift): the skill still reassures agents that - custody keeps itself fresh while the process lives -- which is false for - exactly the failure mode Core 4 names (a non-fenced renewal failure - stops renewal and leaves `self._held` set). + """Core 4 CONFORMS: both agent-facing documents now state renewal as + one-strike AND name the passive discovery signal. Pinned in both + directions -- the corrected claim must be present, and the reassurance + it replaced ("you do not need to do anything to keep it fresh") must + stay gone. """ - assert contains( - CLAIM_SKILL, - "you do not need to do anything to keep it fresh under normal operation.", - ), ( - "CCV1-005 (Core 4, GAP) pin no longer matches -- the prose changed. If it was " - "CORRECTED, flip the row to CONFORMS, re-pin the new wording, and resolve " - "work_item_pipeline-m7o." - ) + for path, label, present in ( + ( + CLAIM_SKILL, + "SKILL.md", + "**Any single renewal failure ends renewal permanently** — there is no " + "retry on the next tick.", + ), + ( + AWARENESS, + "awareness.md", + "a single failed renewal ends renewal permanently — there is no retry on the next tick", + ), + ): + assert contains(path, present), ( + f"CCV1-005 (Core 4) pin: {label} no longer states renewal as one-strike" + ) + assert contains(path, "holding.custody_lost"), ( + f"CCV1-005 (Core 4) pin: {label} no longer names the passive signal an " + f"agent discovers a stopped renewal by" + ) + assert not contains(path, "you do not need to do anything to keep it fresh"), ( + f"CCV1-005 (Core 4) pin: the corrected prose in {label} regressed to the " + f"custody-keeps-itself-fresh reassurance Core 4 contradicts" + ) # --------------------------------------------------------------- CCV1-008 def test_row_ccv1_008() -> None: - """Core 6 GAP pin (doc drift): both agent-facing documents state the TTL - as self-enforcing -- an unrenewed hold "is released" -- with no mention - of the out-of-band sweep the release actually depends on. + """Core 6 CONFORMS: both agent-facing documents now state the TTL as + reclaim-ELIGIBILITY enforced by the out-of-band sweep, name the sweep, + and name the consequence of no sweep running. Pinned in both + directions -- the stale "is released by the clock" phrasing must stay + gone from each. """ assert contains( - AWARENESS, "15 minutes without a renewal releases\nthe item back to the queue." - ), "CCV1-008 pin (awareness.md) no longer matches" - assert contains(CLAIM_SKILL, "An unrenewed\n15-minute hold is released back to the queue."), ( - "CCV1-008 pin (SKILL.md) no longer matches" - ) + AWARENESS, + "**The TTL does not enforce itself.** After 15 minutes with no renewal a hold " + "is merely *reclaim-eligible*; the out-of-band `reap` sweep is what actually " + "reclaims it, and only where an operator has one installed and running.", + ), "CCV1-008 (Core 6) pin: awareness.md no longer states the TTL as sweep-enforced" + assert contains( + AWARENESS, "expect a dead agent's hold to persist indefinitely where no sweep runs" + ), "CCV1-008 (Core 6) pin: awareness.md no longer names the no-sweep consequence" + assert contains( + CLAIM_SKILL, + "**The TTL is not self-enforcing.** Nothing in your process, and no timer in " + "the database, hands a stale hold back. The out-of-band `reap` sweep does, and " + "only where an operator has one installed and running.", + ), "CCV1-008 (Core 6) pin: SKILL.md no longer states the TTL as sweep-enforced" + assert contains( + CLAIM_SKILL, "Where no sweep runs, a dead agent's hold **persists indefinitely**." + ), "CCV1-008 (Core 6) pin: SKILL.md no longer names the no-sweep consequence" + for path, label in ((AWARENESS, "awareness.md"), (CLAIM_SKILL, "SKILL.md")): + assert not contains(path, "releases the item back to the queue"), ( + f"CCV1-008 (Core 6) pin: {label} regressed to stating the release as " + f"automatic -- Core 6 denies exactly that" + ) + assert not contains(path, "15-minute hold is released back to the queue"), ( + f"CCV1-008 (Core 6) pin: {label} regressed to stating the release as " + f"automatic -- Core 6 denies exactly that" + ) # --------------------------------------------------------------- CCV1-009 @@ -275,16 +320,52 @@ def test_row_ccv1_015() -> None: def test_row_ccv1_016() -> None: - """Core 11 GAP pin (doc drift): awareness.md still tells agents a reported - serialization failure means the write did not happen -- contradicting - both the measured incident and the verify-by-read-back code that exists - because of it. + """Core 11 CONFORMS: both prose surfaces -- the agent-facing awareness + file and the CLI's own CONTENTION / RETRY CONTRACT -- now state a + reported conflict as UNKNOWN rather than as proof the write did not + land, name the two verbs that verify by read-back, and keep the + re-read-before-retry guidance that was always correct. + + Pinned in both directions: the "did not happen" claim (defensible + before PR #63, wrong the moment resolve/release started reading a + conflicted write back) must stay gone from both files. """ - assert contains(AWARENESS, "so the write genuinely did not happen"), ( - "CCV1-016 (Core 11, GAP) pin no longer matches -- the prose changed. If it was " - "CORRECTED, flip the row to CONFORMS, re-pin the new wording, and resolve " - "work_item_pipeline-ryp." + assert contains( + AWARENESS, + "**A reported write failure does NOT prove the write failed — treat it as " + "UNKNOWN and re-read before you retry.**", + ), "CCV1-016 (Core 11) pin: awareness.md no longer states a reported failure as unknown" + assert contains( + AWARENESS, + "`work_resolve` and `work_release` already handle it for you: on a conflict " + "they re-read the item and report success when the write did in fact land", + ), "CCV1-016 (Core 11) pin: awareness.md no longer names the verify-by-read-back verbs" + assert contains( + AWARENESS, "There, a reported failure means *unknown*, never *didn't happen*." + ), "CCV1-016 (Core 11) pin: awareness.md no longer scopes the guarantee to those verbs" + assert contains( + CLI, 'Treat a reported failure as "this MIGHT have happened," never as "this did not ' + ), "CCV1-016 (Core 11) pin: the CLI contention contract no longer states failure as unknown" + assert contains(CLI, "Every other write verb"), ( + "CCV1-016 (Core 11) pin: the CLI contention contract no longer scopes the guarantee" + ) + for path, label in ((AWARENESS, "context/awareness.md"), (CLI, "src/.../cli.py")): + assert not contains(path, "the write genuinely did not happen"), ( + f"CCV1-016 (Core 11) pin: {label} regressed to claiming a reported conflict " + f"proves the write did not land" + ) + assert not contains( + CLI, "those specific error signatures are, by dolt/MySQL's own transaction semantics" + ), ( + "CCV1-016 (Core 11) pin: the CLI contention contract regressed to the " + "transaction-was-aborted guarantee Incident B disproved" ) + # The correct half of the original guidance must survive the correction. + for path, label in ((AWARENESS, "context/awareness.md"), (CLI, "src/.../cli.py")): + assert contains(path, "read-only") and contains(path, "cannot itself conflict"), ( + f"CCV1-016 (Core 11) pin: {label} lost the re-read-before-retry instruction " + f"(the half of the original guidance that was always correct)" + ) # --------------------------------------------------------------- CCV1-017 diff --git a/ledger/rows.yaml b/ledger/rows.yaml index ac2805f..8af2ca5 100644 --- a/ledger/rows.yaml +++ b/ledger/rows.yaml @@ -127,25 +127,30 @@ CCV1-022 goes green. - id: CCV1-005 - title: the claiming skill tells agents custody keeps itself fresh + title: agent-facing prose states renewal as one-strike and names its passive signal contract: file: contracts/custody-coordination.v1.md clause: Core 4 quote: | An agent that fails to renew once discovers it via the passive signal `holding.custody_lost` and must recover in-process. - disposition: GAP - work: work_item_pipeline-m7o + disposition: CONFORMS assertion: kind: probe ref: test_row_ccv1_005 notes: > - Doc drift, not a code defect. skills/claiming-work-safely/SKILL.md - still says "you do not need to do anything to keep it fresh under - normal operation" -- inverted for exactly the failure Core 4 names: a - non-fenced renewal failure stops renewal, leaves self._held set, and - surfaces only if the agent thinks to poll work_status. Evidence brief - sec.D-7; ratified lane work item 3. + Was GAP (doc drift, never a code defect): the skill said "you do not + need to do anything to keep it fresh under normal operation" -- + inverted for exactly the failure Core 4 names. CORRECTED, both + surfaces. Evidence: skills/claiming-work-safely/SKILL.md:69-73 (the + loop step now points at the hazard) and :111-134 ("Renewal is + one-strike" -- one failure ends renewal for good, the non-fenced case + leaves self._held set, discovery is work_status's holding.custody_lost, + checked before long-running steps and after any tool error); + context/awareness.md:19-25 (the same claim, compressed). HONEST LIMIT: + like every doc row, the probe asserts the prose says the true thing -- + the BEHAVIOUR it describes is asserted by CCV1-004. Evidence brief + sec.D-7; ratified lane work item 3; work_item_pipeline-m7o. - id: CCV1-006 title: declared_state is reporting only; awaiting_human is not exempt @@ -207,27 +212,32 @@ code, and is carried by CCV1-022 instead. - id: CCV1-008 - title: agent-facing prose states the TTL as self-enforcing + title: agent-facing prose states the TTL as reclaim-eligibility enforced by the sweep contract: file: contracts/custody-coordination.v1.md clause: Core 6 quote: | without the sweep running, the TTL is aspirational. An item held by a dead agent stays held indefinitely until the sweep runs. - disposition: GAP - work: work_item_pipeline-qjn + disposition: CONFORMS assertion: kind: probe ref: test_row_ccv1_008 notes: > - Doc drift, not a code defect. context/awareness.md ("15 minutes without - a renewal releases the item back to the queue") and - skills/claiming-work-safely/SKILL.md ("An unrenewed 15-minute hold is - released back to the queue") both state the release unconditionally; - neither mentions that reclaim requires an installed, running sweep, and - the agent-facing constants table omits the reap interval entirely. The - OPERATOR-facing skill already says it plainly. Evidence brief sec.D-9; - ratified Call 2 (option C) names this as lane work. + Was GAP (doc drift, never a code defect): both agent-facing documents + stated the release unconditionally ("15 minutes without a renewal + releases the item back to the queue" / "An unrenewed 15-minute hold is + released back to the queue"), neither named the sweep, and the + agent-facing constants table omitted the reap interval. CORRECTED, both + surfaces. Evidence: context/awareness.md:26-32 ("The TTL does not + enforce itself" -- 15 min makes a hold merely reclaim-ELIGIBLE, the + out-of-band reap sweep reclaims it, only where an operator runs one, + reclaim lands up to a sweep interval late, and a dead agent's hold + persists indefinitely where no sweep runs); + skills/claiming-work-safely/SKILL.md:98-108 (same, expanded) and :91 + (the reap interval now appears in the constants table). The + OPERATOR-facing skill already said it plainly. Evidence brief sec.D-9; + ratified Call 2 (option C); work_item_pipeline-qjn. - id: CCV1-009 title: a post-reclaim close is not fenced -- the fence is gated on status held @@ -412,27 +422,34 @@ renewal one-strike and dooms a live hold). - id: CCV1-016 - title: awareness.md still asserts a reported conflict means the write did not land + title: prose states a reported conflict as UNKNOWN, and scopes the read-back guarantee contract: file: contracts/custody-coordination.v1.md clause: Core 11 quote: | a reported conflict error must be accompanied by a readback that confirms the conflict - disposition: GAP - work: work_item_pipeline-ryp + disposition: CONFORMS assertion: kind: probe ref: test_row_ccv1_016 notes: > - Doc drift DISCOVERED BY THIS RECONCILE -- it was not on the ratified + Was GAP -- doc drift DISCOVERED BY THIS RECONCILE, not on the ratified lane-work list, because the prose only became wrong when PR #63 - shipped. context/awareness.md still states as a database guarantee that - the retryable error family means "the write genuinely did not happen." - Incident B measured the opposite, and the read-back code exists because - of it. An agent following this prose would treat a reported conflict as - a safe no-op -- the exact reasoning that produced five spaced retries - over ~2.5h in the real incident. The surrounding "re-read first, never - blindly retry" guidance is correct and must survive the correction. + shipped: context/awareness.md stated as a database guarantee that the + retryable error family means "the write genuinely did not happen." + Incident B measured the opposite. CORRECTED, and on BOTH prose + surfaces -- the row now also pins the CLI's own CONTENTION / RETRY + CONTRACT (argparse's `description`), which carried the same claim and + which the original row did not cover. Evidence: + context/awareness.md:51-75 (a reported failure is UNKNOWN; resolve and + release re-read on conflict and report success when the write landed, + so a reported success from those two is confirmed; every other write + verb surfaces the raw conflict unverified) and + src/amplifier_work_tracker/cli.py:13-40 (same, operator-facing). The + correct half of the original guidance -- re-read via a read-only path + that cannot itself conflict, never resubmit a non-idempotent write + blind -- survives on both surfaces and is pinned as such. + work_item_pipeline-ryp. - id: CCV1-017 title: a session holds at most one item at a time diff --git a/skills/claiming-work-safely/SKILL.md b/skills/claiming-work-safely/SKILL.md index 2e835ad..9256468 100644 --- a/skills/claiming-work-safely/SKILL.md +++ b/skills/claiming-work-safely/SKILL.md @@ -66,9 +66,11 @@ intend to do the work. do instead. 2. Read `acceptance` — that is your spec. `description` / `design` are context. A linked user report (if any) is color, never the spec. -3. Work the item. Custody renews automatically in the background for as - long as your session process stays alive — you do not need to do - anything to keep it fresh under normal operation. +3. Work the item. Custody renews automatically in the background while your + session process lives — but renewal is **one-strike**, and its failure + is silent. See "Renewal is one-strike" below: check `work_status`'s + `holding.custody_lost` before any long-running step and after any tool + error, rather than assuming the hold is still fresh. 4. If you're about to go idle waiting on a human, call `work_declare(state="awaiting_human")` once before you go idle. Call `work_declare(state="working")` again when you resume, if you want the @@ -80,19 +82,59 @@ intend to do the work. ## Custody: freshness, not duration -Two clocks matter, and only one of them can cost you the item: +Four settings make up the whole timing model, and only staleness of the +renewal signal can cost you the item: | Setting | Default | Effect | |---|---|---| | Renew interval | 120s (`AMPLIFIER_WORK_TRACKER_RENEW_INTERVAL_SECONDS`) | How often the background renewal fires | -| Custody TTL | 900s / 15 min (`AMPLIFIER_WORK_TRACKER_CUSTODY_TTL_SECONDS`) | No renewal within this window → stale → reclaimable | +| Custody TTL | 900s / 15 min (`AMPLIFIER_WORK_TRACKER_CUSTODY_TTL_SECONDS`) | No renewal within this window → stale → reclaim*able* | +| Reap sweep interval | 300s (`AMPLIFIER_WORK_TRACKER_REAP_INTERVAL_SECONDS`) | How often the out-of-band sweep looks for stale holds. The reclaim happens **here**, not in your process | | Escalation ceiling | 24h (`AMPLIFIER_WORK_TRACKER_ESCALATION_HOURS`) | A *fresh* `awaiting_human` hold past this age becomes reclaim-eligible anyway | **Total hold duration is irrelevant. Only recency of the last renewal matters.** A healthily-renewed 12-hour hold is never touched. An unrenewed -15-minute hold is released back to the queue. - -The two declared states: +15-minute hold becomes reclaim-*eligible* — it is not released by the clock. + +**The TTL is not self-enforcing.** Nothing in your process, and no timer in +the database, hands a stale hold back. The out-of-band `reap` sweep does, +and only where an operator has one installed and running. Two consequences +you must plan for: + +- A reclaim arrives **up to a sweep interval late** — expect ~15–20 min + after the last renewal at the defaults, not exactly 15. +- Where no sweep runs, a dead agent's hold **persists indefinitely**. An + item stuck in `held` is not evidence that its holder is alive, and + waiting will not free it; check `work_tracker_status` (which reports + whether the service, and therefore the sweep, is running at all). + +### Renewal is one-strike + +Renewal runs on a background thread while your session process lives. **Any +single renewal failure ends renewal permanently** — there is no retry on the +next tick. From that moment the hold stops being refreshed and is on its way +to becoming reclaim-eligible. + +The failure is *silent* in the case that matters most. A fenced failure (bd +no longer considers you the holder) clears this session's belief that it +holds the item. A plain, non-fenced failure — a transient bd/dolt command +failure — does **not**: renewal has stopped, but the session still believes +it holds the item, and nothing tells you. + +The one way to discover it is a passive check: `work_status` reports +`holding.custody_lost`. Non-null means renewal stopped, and carries the +reason. Check it: + +- before starting any long-running step (a build, a long test run, a + delegation) — losing custody mid-step means the work is being thrown away, +- after any tool error, however unrelated it looks, +- before `work_resolve`, if a long time has passed since the claim. + +If it is non-null, treat it exactly like a reap refusal — see "After a reap" +below: stop, report the state you left the work in, do not re-claim the same +item to resume. + +### The two declared states - **`working`** — the default. If your custody signal goes stale while declaring this, you are reclaimed exactly like anything else. diff --git a/src/amplifier_work_tracker/cli.py b/src/amplifier_work_tracker/cli.py index f3cf475..f214ba6 100644 --- a/src/amplifier_work_tracker/cli.py +++ b/src/amplifier_work_tracker/cli.py @@ -16,23 +16,27 @@ hitting concurrently. `adapter.Beads._run` rides out dolt serialization conflicts (MySQL 1213/1205, "serialization failure", "try restarting transaction") with up to 8 retries and exponential backoff before giving up -and raising -- a message of the shape "still conflicting after 8 retries" (or -any `BeadsError` at all from a write command) means the underlying -transaction was ABORTED, never partially committed: those specific error -signatures are, by dolt/MySQL's own transaction semantics, "this transaction -did not happen," not "it might have happened." VERIFY, DO NOT BLINDLY RETRY: -before resubmitting the same logical operation, re-read the item (`list --id` -/ `work_list`'s `item_id` form -- a read-only SQL path that cannot itself +and raising -- but a message of the shape "still conflicting after 8 retries" +(or any `BeadsError` at all from a write command) does NOT prove the write +failed. A conflicted write can still have LANDED: that is the measured +incident this hardening exists because of, and why `resolve`/`unclaim` read +the item back on conflict. Treat a reported failure as "this MIGHT have +happened," never as "this did not happen." `resolve`/`unclaim` handle it for +you at both ends: on a conflict they re-read and report success when the +write actually landed, and they verify their OWN success path by read-back +before reporting it (exit code is not proof by itself) -- so a reported +SUCCESS from those two is independently confirmed. Every other write verb +(`add`, `edit`, `defer`, `block`, `dep`, `comment`, custody writes) still +surfaces the raw conflict unverified. VERIFY, DO NOT BLINDLY RETRY: before +resubmitting the same logical operation, re-read the item (`list --id` / +`work_list`'s `item_id` form -- a read-only SQL path that cannot itself conflict) to confirm its actual current state. This matters most for -non-idempotent writes (`add`/`create` -- retrying blind can create a -duplicate item) and less for idempotent ones (`resolve` on an already- -resolved item is a readback-checked no-op) -- but re-reading first is always -the safe move. `resolve`/`unclaim` additionally verify their OWN write landed -by reading the item back before reporting success (exit code is not proof by -itself) -- so a reported SUCCESS is independently confirmed already; this -contract is about what to do after a reported FAILURE. See `context/ -awareness.md` for the same contract in agent-facing form, and work_tracker -item pipeline-bug for the contention-hardening work this documents. +non-idempotent writes (`add`/`create` -- retrying blind after a conflict that +actually landed creates a duplicate item) and less for idempotent ones +(`resolve` on an already-resolved item is a readback-checked no-op) -- but +re-reading first is always the safe move. See `context/awareness.md` for the +same contract in agent-facing form, and work_tracker item pipeline-bug for +the contention-hardening work this documents. """ from __future__ import annotations From e6e54348e0ad659551123bf0829ef758759fd62d Mon Sep 17 00:00:00 2001 From: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com> Date: Wed, 2 Sep 2026 21:25:18 -0700 Subject: [PATCH 4/7] fix: every item-level write verb verifies itself by read-back Exit code is not proof -- neither a zero one nor a non-zero one. PR #63 gave `resolve`/`release` a read-back on the CONFLICT path only; every other item-level write verb still believed the wrapper, and `release`'s own SUCCESS path still returned straight off `p.returncode == 0`. One shared helper, `Beads._verified_write(run, verify, what=...)`, now carries the discipline once: - the wrapper's exhausted-retry `BeadsError` (or a non-zero exit whose output names a serialization/connection failure) is decided by a contention-free read-back, not believed -- exhaustion never proved a write did not land (measured incident, work_tracker pipeline-yym); - a reported SUCCESS is verified too -- a `bd` write that exits 0 and changes nothing is otherwise indistinguishable from one that worked; - a genuine domain refusal is never verified away, and a write that truly did not land still raises. Routed: create, update, comment, edit_item (both halves), supersede, claim_item, claim_next, release, defer/undefer, block/unblock, add_dependency, take_custody, renew_custody (the write behind the tool's `declare`). `claim_item`/`claim_next` additionally now RETURN the read-back rather than an Item parsed from the writing process's own stdout. `resolve` keeps PR #63's inline shape verbatim (that region is pinned by ledger row CCV1-009 and owned elsewhere; behaviour identical). `move_item` is deliberately not routed -- direct dolt SQL, no `_run`, and it already verifies by real row counts plus a compensating cleanup. Two verbs have no id to read back by when a conflict destroys bd's own stdout -- `create` (bd prints the new id) and `claim_next` (bd chooses the item). Both use a new read-only `_ids_via_sql` set difference snapshotted before the write, with an honest three-way answer: exactly one new row means it landed and names it, none means it did not, and more than one is AMBIGUOUS and re-raises rather than guessing. Ledger: CCV1-012, CCV1-013, CCV1-015 -> CONFORMS, probes rewritten to assert the fixed behaviour (per-verb, sliced by AST so one verb's probe cannot match a sibling). Measured by tests/integration/test_write_readback.py -- 26 cases injecting conflict-after-landed, conflict-with-no-write, and phantom-success against the real isolated dolt server. --- ledger/checks/test_custody_rows.py | 195 ++++++-- ledger/rows.yaml | 88 ++-- src/amplifier_work_tracker/adapter.py | 572 +++++++++++++++------ tests/integration/test_write_readback.py | 601 +++++++++++++++++++++++ 4 files changed, 1221 insertions(+), 235 deletions(-) create mode 100644 tests/integration/test_write_readback.py diff --git a/ledger/checks/test_custody_rows.py b/ledger/checks/test_custody_rows.py index fba0461..4e56647 100644 --- a/ledger/checks/test_custody_rows.py +++ b/ledger/checks/test_custody_rows.py @@ -25,6 +25,7 @@ from __future__ import annotations +import ast import re from ._support import ( @@ -36,6 +37,7 @@ MAKEFILE, REPO_ROOT, TOOL_MODULE, + collapse, contains, count, read, @@ -43,6 +45,29 @@ sha256, ) + +def _beads_method(name: str) -> str: + """The whitespace-collapsed source of exactly ONE `Beads` method. + + Sliced by AST line span rather than by string search, so a probe about + (say) `release` can never accidentally match a sibling method that + happens to contain a similar line -- the failure mode a whole-file + `contains()` has whenever the same shape appears in more than one verb, + which is precisely the situation once every write verb shares one + helper. Collapsed for the same reason `contains` collapses: survives + reformatting, never survives a real change of wording. + """ + src = read(ADAPTER) + lines = src.splitlines(keepends=True) + for node in ast.parse(src).body: + if not (isinstance(node, ast.ClassDef) and node.name == "Beads"): + continue + for member in node.body: + if isinstance(member, ast.FunctionDef) and member.name == name: + return collapse("".join(lines[member.lineno - 1 : member.end_lineno])) + raise AssertionError(f"Beads.{name} not found in {ADAPTER} -- the probe is out of date") + + # --------------------------------------------------------------- CCV1-000 @@ -200,23 +225,31 @@ def test_row_ccv1_011() -> None: def test_row_ccv1_012() -> None: - """Core 10 VIOLATION pin: `release()` returns success straight off the - subprocess exit code -- no read-back that the status actually became - `open`. Every sibling write verifies itself; this one, which both - `work_release` and every reap reclaim call, does not. + """Core 10 CONFORMS: `release()` confirms the hold actually cleared + before returning -- on the SUCCESS path, not only the conflict path. + The outcome it reports is derived from that read-back, never asserted + from a zero exit code. """ - assert contains( - ADAPTER, + body = _beads_method("release") + assert "self._verified_write(" in body, "CCV1-012: release no longer routes through the helper" + assert ( + collapse( + """ + def _verify() -> bool: + back = self._read_back_or_none(item_id) + if back is None or back.status == "held": + return False """ - if p.returncode != 0: - detail = _clean_bd_error(p.stderr or p.stdout, limit=200) - raise BeadsError(f"release {item_id}: {detail}") - return ReleaseOutcome(item_id=item_id, already_closed=False) - """, - ), ( - "CCV1-012 (Core 10, VIOLATION) pin no longer matches. If release() gained its " - "read-back, this is the expected failure: flip the row to CONFORMS and resolve " - "work_item_pipeline-1f2." + ) + in body + ), "CCV1-012: release's verify no longer demands the item is out of `held`" + assert ( + collapse("return ReleaseOutcome(item_id=item_id, already_closed=(seen[-1].status ==") + in body + ), "CCV1-012: the reported outcome is no longer derived from the read-back" + assert "already_closed=False)" not in body, ( + "CCV1-012 (Core 10) regression: release reports an outcome it did not read back. " + "The exit-code-only success return is exactly the shape this row exists to forbid." ) @@ -224,50 +257,112 @@ def test_row_ccv1_012() -> None: def test_row_ccv1_013() -> None: - """Core 10 GAP pin: neither claim path verifies itself by read-back. Both - return an item parsed from the WRITING process's own stdout -- the - "exit code is not proof" shape, on the highest-stakes custody write. + """Core 10 CONFORMS: both claim paths verify by read-back, and both + RETURN the read-back rather than an Item parsed from the writing + process's own stdout. A claim is the highest-stakes custody write here + -- its caller starts custody on the strength of it -- so "bd said so" + is never the answer. """ - assert contains( - ADAPTER, - """ - if not items: - raise BeadsError(f"claim {item_id}: bd reported success but returned no item") - return Item.from_beads(items[0]) - """, - ), "CCV1-013 pin (claim_item) no longer matches" - assert contains( - ADAPTER, - """ - data = self._json(["ready", "--label", lane, "--claim"], actor=actor) - items = data if isinstance(data, list) else ([data] if data else []) - items = [i for i in items if isinstance(i, dict) and i.get("id")] - return Item.from_beads(items[0]) if items else None - """, - ), ( - "CCV1-013 (Core 10, GAP) pin (claim_next) no longer matches. If the claim path " - "gained verify-by-read-back, flip the row to CONFORMS and resolve " - "work_item_pipeline-1gz." + directed = _beads_method("claim_item") + queued = _beads_method("claim_next") + for verb, body in (("claim_item", directed), ("claim_next", queued)): + assert "self._verified_write(" in body, ( + f"CCV1-013 (Core 10) regression: {verb} no longer routes through the " + f"verified-write helper." + ) + assert "Item.from_beads(" not in body, ( + f"CCV1-013 (Core 10) regression: {verb} builds its returned Item from the " + f"writing process's own stdout again. The returned item must be the read-back." + ) + assert 'back.status == "held" and back.holder == actor' in body, ( + f"CCV1-013: {verb}'s verify no longer demands THIS actor holds the item" + ) + assert "return self.get(item_id)" in directed, "CCV1-013: claim_item returns a non-read-back" + assert "return self.get(claimed[0]) if claimed else None" in queued, ( + "CCV1-013: claim_next returns a non-read-back, or lost its empty-queue None" ) + # The conflict path has no id to read back by -- it is decided by the + # id-set difference, and an ambiguous result must never be guessed. + assert ( + collapse( + """ + new = _ids_via_sql(self.project_name, held_where) - held_before + if len(new) != 1: + """ + ) + in queued + ), "CCV1-013: claim_next's conflict-path set difference changed shape" # --------------------------------------------------------------- CCV1-015 +#: Every item-level write verb on `Beads`. The closed list this row is +#: about: each one must route its `bd` write through `_verified_write`, so +#: a conflict-family failure is decided by read-back rather than by the +#: wrapper's verdict. `resolve` is deliberately absent -- it keeps PR #63's +#: own inline shape, pinned by CCV1-009 (see that row). +_VERIFIED_WRITE_VERBS = ( + "create", + "update", + "comment", + "supersede", + "claim_next", + "claim_item", + "release", + "_set_status_with_reason", # defer / block + "_clear_status_with_reason", # undefer / unblock + "add_dependency", + "take_custody", + "renew_custody", +) + + def test_row_ccv1_015() -> None: - """Core 11 GAP pin: the conflicted-write read-back helper has exactly - three occurrences -- its definition and two call sites (`resolve`, - `release`). Every other write verb still propagates an exhausted-retry - exception directly, so a landed write can still surface as a reported - failure there. + """Core 11 CONFORMS: one shared helper carries verify-on-conflict for + EVERY item-level write verb, not just `resolve`/`release`. Exhaustion + of the retry budget does not prove a write did not land, so a reported + failure is decided by a contention-free read-back before it is + believed -- and a reported success is verified too. """ - occurrences = count(ADAPTER, "_read_back_or_none") - assert occurrences == 3, ( - f"CCV1-015 (Core 11, GAP) pin: expected 3 occurrences of `_read_back_or_none` " - f"(1 definition + 2 call sites: resolve, release), found {occurrences}. If a " - f"third write verb adopted verify-on-conflict, update the row's coverage list " - f"(and resolve work_item_pipeline-2x3 when the custody-relevant writes -- " - f"take_custody, renew_custody -- are covered)." + helper = _beads_method("_verified_write") + assert "except BeadsError:" in helper and "if self._landed(verify):" in helper, ( + "CCV1-015: the helper no longer verifies on the wrapper's exhausted-retry raise" + ) + assert "if (_retryable(blob) or _connection_retryable(blob)) and self._landed(verify):" in ( + helper + ), "CCV1-015: the helper no longer verifies a conflict-family non-zero exit" + assert "if not verify():" in helper, ( + "CCV1-015: the helper stopped verifying the SUCCESS path -- exit code is not proof" + ) + assert "return False" in _beads_method("_landed"), ( + "CCV1-015: `_landed` must swallow a failed verification into False, never mask " + "the original error with a second one" + ) + + missing = [v for v in _VERIFIED_WRITE_VERBS if "self._verified_write(" not in _beads_method(v)] + assert not missing, ( + f"CCV1-015 (Core 11) regression: these write verbs no longer route through " + f"`_verified_write`, so a landed write there can still surface as a reported " + f"failure: {missing}" + ) + + edit = _beads_method("edit_item") + assert "self.update(" in edit and "self.comment(" in edit, ( + "CCV1-015: `edit` must delegate BOTH halves (field write + audit comment) to " + "verbs that verify themselves" + ) + # `move_item` is a module-level function over direct dolt SQL -- it never + # touches `Beads._run`, so the helper cannot apply. It carries its own, + # equivalent proof: real row counts in dst, and a compensating cleanup so + # a reported failure never leaves state as if the write succeeded. + assert contains(ADAPTER, "left an incomplete copy in"), ( + "CCV1-015: move_item's own row-count verification is gone" + ) + assert count(ADAPTER, "_read_back_or_none") == 3, ( + "CCV1-015: `_read_back_or_none`'s call sites moved (expected 1 definition + " + "resolve's conflict branch + release's verify). Re-check that every verb still " + "verifies before adjusting this count." ) diff --git a/ledger/rows.yaml b/ledger/rows.yaml index ac2805f..3491fc3 100644 --- a/ledger/rows.yaml +++ b/ledger/rows.yaml @@ -313,49 +313,64 @@ with_custody_takeover_mismatch_while_held (measured 2026-09-01). - id: CCV1-012 - title: release() reports success off the exit code, with no read-back + title: release() confirms the hold actually cleared before reporting success contract: file: contracts/custody-coordination.v1.md clause: Core 10 quote: | an operation like `release()` must confirm the item status changed before returning success. - disposition: VIOLATION + disposition: CONFORMS work: work_item_pipeline-1f2 assertion: kind: probe ref: test_row_ccv1_012 notes: > - NARROWED BY TRUE-UP. PR #63 gave release() a pre-write status check and - a read-back on the CONFLICT path; the SUCCESS path was untouched and - still returns straight off `p.returncode == 0`. Every sibling write - verifies itself (take_custody, renew_custody, resolve, defer/block, - supersede, add_dependency, CLI unclaim). release does not -- and it is - what BOTH work_release AND every reap reclaim call, so a `bd update` - that silently did not land leaves a "reclaimed" item still held while - the sweep reports it reclaimed. Evidence brief sec.D-4. + FIXED. `release` now routes its `bd update --status open --assignee ""` + through the shared `_verified_write` helper, whose verify demands a + contention-free read-back showing the item is no longer `held` -- on + the SUCCESS path as well as the conflict path PR #63 already gave it. + The pre-write `already_closed` branch is untouched, and the returned + `ReleaseOutcome` is now derived from that same read-back rather than + asserted from the exit code. This matters because `release` is what + BOTH work_release AND every reap reclaim call: a `bd update` that + exited 0 without clearing the hold used to leave the item HELD while + the sweep reported it reclaimed. Measured 2026-09-02, discriminating + both ways, in tests/integration/test_write_readback.py:: + test_release_raises_when_bd_reports_success_but_the_hold_did_not_clear + (phantom success -> raises; the item really is still held) and + ::test_release_success_path_returns_only_after_the_readback_shows_no_hold + (a real release still succeeds). The conflict half stays covered by + tests/integration/test_phantom_conflict_recovery.py (CCV1-014). - id: CCV1-013 - title: neither claim path verifies its own write by read-back + title: both claim paths verify their own write by read-back contract: file: contracts/custody-coordination.v1.md clause: Core 10 quote: | Every write that changes custody state must read the item back and verify the change landed before reporting success. - disposition: GAP + disposition: CONFORMS work: work_item_pipeline-1gz assertion: kind: probe ref: test_row_ccv1_013 notes: > - Both `claim_item` and `claim_next` return an Item parsed from the - WRITING process's own stdout, with no independent read through the - contention-free path and no conflict-path read-back. PR #63 named this - residual in its own resolution text on work_item_pipeline-yym: - "claim/claim_next are covered by the source-level returncode gate but - lack the verify-by-read-back second layer." Ratified Call 4 (option B) - puts it in Core, contract-then-fix. + FIXED. `claim_item` and `claim_next` both route through + `_verified_write` and both now RETURN the contention-free read-back + rather than an Item parsed from the writing process's own stdout -- + bd must show this actor holding the item, or the claim raises. The two + differ only in their conflict-path key: `claim_item` knows the id, so + it is a plain keyed read; `claim_next` does not (bd chooses, and the + conflict destroys its stdout), so it uses the id-set difference over + items assigned to this actor, snapshotted before the write, with + "more than one new hold" treated as ambiguous and re-raised rather + than guessed. An empty queue stays a normal `None`, never a failed + write. Measured 2026-09-02 in tests/integration/test_write_readback.py + -- six cases covering, for each path, conflict-after-landed (reports + success), conflict-with-no-write (still raises), and phantom success + (raises), plus ::test_claim_next_on_an_empty_queue_is_still_a_normal_none. - id: CCV1-014 title: a landed write is never reported as a failure (resolve / release) @@ -389,27 +404,42 @@ NOT covered: see CCV1-015. - id: CCV1-015 - title: only resolve and release verify a conflicted write + title: every item-level write verb verifies a conflicted write contract: file: contracts/custody-coordination.v1.md clause: Core 11 quote: | If a write is reported as failed (exception raised, error returned), the item state must not have changed as if the write succeeded. - disposition: GAP + disposition: CONFORMS work: work_item_pipeline-2x3 assertion: kind: probe ref: test_row_ccv1_015 notes: > - `_read_back_or_none` has exactly two call sites. Every other write verb - -- take_custody, renew_custody, claim_item, defer/block, supersede, - update/edit, add_dependency, comment -- still propagates an - exhausted-retry BeadsError directly, and exhaustion does not prove the - write did not land. The two that matter most for custody are - take_custody (a false failure produces exactly the CCV1-003 - held-without-custody state) and renew_custody (a false failure ends - renewal one-strike and dooms a live hold). + FIXED. One shared helper, `Beads._verified_write`, now carries the + discipline for every item-level write verb: on a conflict-family + failure (the wrapper's exhausted-retry raise, or a non-zero exit whose + output names a serialization/connection failure) it decides the real + outcome by a contention-free read-back instead of trusting the + wrapper, and on a reported SUCCESS it verifies anyway. A genuine + domain refusal is never verified away, and a write that truly did not + land still raises. Routed: create, update, comment, edit_item (both + halves), supersede, claim_item, claim_next, release, defer/undefer, + block/unblock, add_dependency, take_custody, renew_custody (the write + behind the tool's `declare`). `resolve` keeps its own PR #63 shape + verbatim -- the region is pinned by CCV1-009 and owned by another + change; the behaviour is identical. `move_item` is deliberately NOT + routed: it never touches bd or `Beads._run` (direct dolt SQL), so the + retry hazard cannot reach it, and it already verifies by real row + counts in dst plus a residue check in src, with a compensating cleanup + on failure. Measured 2026-09-02 in + tests/integration/test_write_readback.py -- a conflict-after-landed + case per verb, phantom-success negatives for release/claim x2/create/ + comment/defer/dep/renew_custody, genuine-failure negatives for + claim x2/create, and :: + test_move_refuses_when_the_copy_reports_success_but_the_rows_are_not_there + for move's own mechanism. - id: CCV1-016 title: awareness.md still asserts a reported conflict means the write did not land diff --git a/src/amplifier_work_tracker/adapter.py b/src/amplifier_work_tracker/adapter.py index a922d83..bf05772 100644 --- a/src/amplifier_work_tracker/adapter.py +++ b/src/amplifier_work_tracker/adapter.py @@ -26,6 +26,7 @@ import subprocess import time import uuid +from collections.abc import Callable from dataclasses import dataclass, field from datetime import UTC, datetime, timedelta from pathlib import Path @@ -947,6 +948,45 @@ def _get_item_via_sql(db: str, item_id: str) -> Item | None: return rows[0] if rows else None +def _ids_via_sql(db: str, where_sql: str) -> set[str]: + """The set of item ids in `db` matching `where_sql`, over a READ-ONLY + SQL SELECT -- the contention-free path (see `_get_item_via_sql`), so + this can never itself lose a serialization conflict. + + Exists for the ONE verification shape a plain read-back-by-id cannot + serve: a write whose own output names the item it touched, when that + output was LOST because the wrapper reported a conflict-family failure. + `create` (the new id is printed on stdout) and `claim_next` (bd chooses + which item to claim) are exactly those two. Snapshotting the matching + id set BEFORE the write and re-reading it after turns "did it land?" + into a set difference, with an honest three-way answer: + + - exactly one new id -> the write landed; that is the item. + - no new id -> the write genuinely did not land. + - more than one new id -> AMBIGUOUS (a concurrent writer produced an + indistinguishable row in the same window). Callers treat this as + "not verified" and re-raise the original failure, never as success + -- guessing which of two rows was ours is exactly the silent + mis-attribution this whole discipline exists to prevent. + + Only the `id` column is projected, so the CSV path (`_dolt_sql`) is + safe here: ids are short scalar primary keys that cannot contain a + comma or newline -- the same reason `_list_rows_via_sql` keeps its + labels projection on CSV while sending free text through JSON. + + `db` has already passed `NAME_RE` at every call site (it is a project + name); `where_sql` is composed by the caller from `_sql_literal`- + escaped values, the same discipline `Beads.list()` already uses. + """ + p = _dolt_sql(f"SELECT `id` FROM `{db}`.`issues` WHERE {where_sql}") + if p.returncode != 0: + raise BeadsError( + f"could not read item ids of database {db!r} over SQL: " + f"{_clean_bd_error(p.stderr or p.stdout)}" + ) + return {ln.strip() for ln in (p.stdout or "").splitlines()[1:] if ln.strip()} # drop CSV header + + def copy_database(src: str, dst: str) -> None: """Create database `dst` as a faithful copy of `src` on the shared dolt server: identical schema (every base table, view, and foreign key) and @@ -2418,6 +2458,86 @@ def _json(self, args: list[str], actor: str | None = None): raise BeadsError(f"`bd {' '.join(args[:2])}`: {data['error']}") return data + # ------------------------------------------------- verified write (one home) + + @staticmethod + def _landed(verify: Callable[[], bool]) -> bool: + """`verify()`, but never raising and never True-by-accident. + + Used ONLY on a write's FAILURE path, where the question is "did the + write land anyway?". A verification that cannot itself run (the + read errored, the item vanished, dolt hiccuped) answers "no + evidence it landed" -- which re-raises the ORIGINAL error rather + than masking it behind a second, unrelated one. Deliberately NOT + used on the success path: there, a verification that cannot run is + a real failure to prove the write, and must surface as such. + """ + try: + return bool(verify()) + except Exception: + return False + + def _verified_write( + self, + run: Callable[[], subprocess.CompletedProcess], + verify: Callable[[], bool], + *, + what: str, + ) -> subprocess.CompletedProcess | None: + """Perform ONE `bd` write and PROVE it landed by reading the item + back through the contention-free path. The single home for the + "exit code is not proof" discipline every item-level write verb in + this class shares -- generalized from the shape PR #63 gave a + conflicted `resolve`/`release`, so no verb has to reinvent (or + forget) it. + + `run` performs the write and returns its `CompletedProcess` (it may + also stash whatever it parsed off stdout for `verify` to use -- see + `create`/`claim_next`). `verify` answers ONE question against a + fresh, contention-free read: is the intended state actually there? + `what` names the operation for error text (e.g. `"release wt-3"`). + + Three paths, none of which trusts an exit code on its own: + + - `run` RAISES `BeadsError` -- `_run`'s serialization-retry + budget is exhausted. Exhaustion does NOT prove the write never + landed: the measured incident (work_tracker item pipeline-yym, + 2026-09-01) was a close that HAD landed on an earlier attempt + while the wrapper still raised, quoting its own success + confirmation. Verify; if the state is there, report success + (returning `None`, since bd's own output for that attempt is + gone). Otherwise re-raise the original, untouched. + - `run` RETURNS a non-zero exit whose output names a + conflict/connection-transport failure -- same reasoning, same + treatment. A genuine domain error (bd refused, item not found, + already closed) is NOT verified away: it raises with bd's own + message, exactly as before. + - `run` RETURNS success -- verify anyway. A `bd` write that exits + 0 without changing anything is indistinguishable from one that + did, until something reads it back. A phantom success raises. + + Returns the `CompletedProcess` when the write itself reported + success (callers that need its stdout use it), or `None` when the + write reported failure but the read-back proved it landed. + """ + try: + p = run() + except BeadsError: + if self._landed(verify): + return None + raise + if p.returncode != 0: + blob = (p.stdout or "") + (p.stderr or "") + if (_retryable(blob) or _connection_retryable(blob)) and self._landed(verify): + return None + raise BeadsError(f"{what}: {_clean_bd_error(p.stderr or p.stdout)}") + if not verify(): + raise BeadsError( + f"{what} reported success but the write did not land -- exit code is " + f"not proof (checked by contention-free read-back)." + ) + return p + # ---------------------------------------------------------------- domain ops def create( @@ -2448,6 +2568,24 @@ def create( bd's own `--deps` flag alongside `discovered_from`, so both land in the SAME atomic `bd create` call -- no separate, unverified follow-up write. + + Verified by read-back on BOTH paths (`_verified_write`), which for + a create needs two different keys because the obvious one is not + always available: + + - SUCCESS path: bd prints the new id on stdout; the item is read + back BY THAT ID through the contention-free SQL path and its + title checked. A `bd create` that exits 0 having written + nothing therefore raises instead of returning a dangling id. + - CONFLICT path: the wrapper raised, so bd's stdout -- and with + it the only stable key -- is gone. Falls back to the id-set + difference over this exact title (`_ids_via_sql`, snapshotted + BEFORE the write): exactly one new row means the create landed + and names it; none means it genuinely did not; more than one + is ambiguous and re-raises rather than guessing which row was + ours. This is the only verb here whose conflict-path proof is + weaker than a keyed read-back, and the ambiguity is resolved + conservatively -- never by picking a row. """ args = ["create", title, "-t", kind, "-p", str(priority)] if tags: @@ -2475,11 +2613,33 @@ def create( if deps: args += ["--deps", ",".join(deps)] args += ["--silent"] - p = self._run(args, actor=actor) - new_id = (p.stdout or "").strip().splitlines()[-1].strip() if p.stdout else "" - if p.returncode != 0 or not new_id: - raise BeadsError(f"create failed: {_clean_bd_error(p.stderr or p.stdout)}") - return new_id + title_where = f"`title` = '{_sql_literal(title)}'" + before = _ids_via_sql(self.project_name, title_where) + created: list[str] = [] + + def _do_create() -> subprocess.CompletedProcess: + p = self._run(args, actor=actor) + out = (p.stdout or "").strip() + if p.returncode == 0 and out: + new_id = out.splitlines()[-1].strip() + if new_id: + created.append(new_id) + return p + + def _verify() -> bool: + if created: # bd named the id -- confirm it independently + back = _get_item_via_sql(self.project_name, created[0]) + return back is not None and back.title == title + new = _ids_via_sql(self.project_name, title_where) - before + if len(new) != 1: # zero == did not land; >1 == ambiguous, never guess + return False + created.append(next(iter(new))) + return True + + self._verified_write(_do_create, _verify, what=f"create {title!r}") + if not created: # unreachable via _verified_write, guarded rather than assumed + raise BeadsError(f"create {title!r}: reported success but no id could be resolved") + return created[0] def update( self, @@ -2509,7 +2669,10 @@ def update( Verifies the write landed by reading the item back, the same discipline `resolve` applies ("exit code is not proof") -- a successful `bd update` exit with a title that didn't actually - change would otherwise look identical to a silent no-op. + change would otherwise look identical to a silent no-op. Routed + through `_verified_write`, so the SAME read-back also decides a + conflict-family failure: a wrapper that gave up on an update that + had already landed reports success, not a false failure. """ args = ["update", item_id] if title is not None: @@ -2522,21 +2685,24 @@ def update( args += ["--design", design] if len(args) == 2: # nothing to change -- avoid a no-op `bd update` call/verify return self.get(item_id) - p = self._run(args, actor=actor) - if p.returncode != 0: - raise BeadsError(f"update {item_id}: {_clean_bd_error(p.stderr or p.stdout)}") - back = self.get(item_id) - if ( - (title is not None and back.title != title) - or (description is not None and back.description != description) - or (acceptance is not None and back.acceptance != acceptance) - or (design is not None and back.design != design) - ): - raise BeadsError( - f"update {item_id} reported success but the change did not land -- " - f"exit code is not proof; see this method's docstring." - ) - return back + seen: list[Item] = [] + + def _verify() -> bool: + back = self.get(item_id) + if ( + (title is not None and back.title != title) + or (description is not None and back.description != description) + or (acceptance is not None and back.acceptance != acceptance) + or (design is not None and back.design != design) + ): + return False + seen.append(back) + return True + + self._verified_write( + lambda: self._run(args, actor=actor), _verify, what=f"update {item_id}" + ) + return seen[-1] def comment(self, item_id: str, text: str, *, actor: str | None = None) -> None: """Append a comment to an item -- bd's own audit-trail mechanism @@ -2547,10 +2713,26 @@ def comment(self, item_id: str, text: str, *, actor: str | None = None) -> None: Used by `edit_item` to record who changed what on a content edit, and available standalone for any other audit-trail note a caller wants attached to an item without touching its own fields. + + Verified by read-back like every other write here, but by COUNT + rather than presence: an item may legitimately already carry a + comment with this exact text (two identical edits), so "a matching + comment exists" would report success for a write that never + happened. The count of exactly-matching comments taken before the + write must have increased. """ - p = self._run(["comment", item_id, text], actor=actor) - if p.returncode != 0: - raise BeadsError(f"comment {item_id}: {_clean_bd_error(p.stderr or p.stdout)}") + + def _matching() -> int: + got = self._json(["comments", item_id]) + rows = got if isinstance(got, list) else [] + return sum(1 for c in rows if isinstance(c, dict) and c.get("text") == text) + + before = _matching() + self._verified_write( + lambda: self._run(["comment", item_id, text], actor=actor), + lambda: _matching() > before, + what=f"comment {item_id}", + ) def edit_item( self, @@ -2639,27 +2821,26 @@ def supersede(self, item_id: str, replacement_id: str, *, actor: str | None = No `Beads.get`'s docstring for why `with_links=True` deliberately stays on bd). """ - p = self._run(["supersede", item_id, "--with", replacement_id], actor=actor) - if p.returncode != 0: - raise BeadsError( - f"supersede {item_id} with {replacement_id}: " - f"{_clean_bd_error(p.stderr or p.stdout)}" - ) - back = self.get(item_id, with_links=True) - if back.status != "resolved": - raise BeadsError( - f"supersede {item_id} reported success but readback shows status=" - f"{back.status!r} -- refusing to report success" - ) - if not any( - link.get("id") == replacement_id and link.get("direction") == "from" - for link in back.links - ): - raise BeadsError( - f"supersede {item_id} with {replacement_id} reported success but readback " - f"shows no structural reference to the replacement -- refusing to report success" - ) - return back + seen: list[Item] = [] + + def _verify() -> bool: + back = self.get(item_id, with_links=True) + if back.status != "resolved": + return False + if not any( + link.get("id") == replacement_id and link.get("direction") == "from" + for link in back.links + ): + return False + seen.append(back) + return True + + self._verified_write( + lambda: self._run(["supersede", item_id, "--with", replacement_id], actor=actor), + _verify, + what=f"supersede {item_id} with {replacement_id}", + ) + return seen[-1] def claim_next(self, *, lane: str = LANE_WORK, actor: str) -> Item | None: """THE claim. Single atomic operation, never read-then-write. @@ -2667,11 +2848,62 @@ def claim_next(self, *, lane: str = LANE_WORK, actor: str) -> Item | None: ASSUMPTION claim.atomic / claim.subcommand / claim.actor_env. Identity travels in BEADS_ACTOR because this subcommand rejects an explicit assignee flag. + + VERIFIED BY READ-BACK (`_verified_write`), and the returned `Item` + is the READ-BACK, never the claiming process's own stdout -- a + claim is the highest-stakes custody write here (its caller + immediately starts custody on the strength of it), so "bd said so" + is not enough. Three outcomes, kept distinct: + + - bd claimed an item: it is read back through the contention-free + SQL path and must show THIS actor holding it. + - bd found nothing ready: an empty queue is a normal terminal + outcome, not a failed write -- `None`, no verification needed. + - the wrapper reported a conflict-family failure: bd's stdout is + gone, so which item (if any) it claimed is unknown. Decided by + the id-set difference over items assigned to this actor, + snapshotted BEFORE the write (`_ids_via_sql`): exactly one new + hold means the claim landed and names it; none means it did + not; more than one is ambiguous and re-raises rather than + guessing. """ - data = self._json(["ready", "--label", lane, "--claim"], actor=actor) - items = data if isinstance(data, list) else ([data] if data else []) - items = [i for i in items if isinstance(i, dict) and i.get("id")] - return Item.from_beads(items[0]) if items else None + held_where = f"`assignee` = '{_sql_literal(actor)}'" + held_before = _ids_via_sql(self.project_name, held_where) + claimed: list[str] = [] + nothing_ready: list[bool] = [] + + def _do_claim() -> subprocess.CompletedProcess: + p = self._run(["ready", "--label", lane, "--claim", "--json"], actor=actor) + if p.returncode != 0: + return p + out = (p.stdout or "").strip() + try: + data = json.loads(out) if out else None + except json.JSONDecodeError as e: + raise BeadsError(f"`bd ready --claim` returned non-JSON: {out[:200]}") from e + if isinstance(data, dict) and data.get("error"): + raise BeadsError(f"`bd ready --claim`: {data['error']}") + items = data if isinstance(data, list) else ([data] if data else []) + ids = [i["id"] for i in items if isinstance(i, dict) and i.get("id")] + if ids: + claimed.append(str(ids[0])) + else: + nothing_ready.append(True) + return p + + def _verify() -> bool: + if nothing_ready: # empty queue -- nothing was written, nothing to verify + return True + if not claimed: + new = _ids_via_sql(self.project_name, held_where) - held_before + if len(new) != 1: # zero == did not land; >1 == ambiguous, never guess + return False + claimed.append(next(iter(new))) + back = _get_item_via_sql(self.project_name, claimed[0]) + return back is not None and back.status == "held" and back.holder == actor + + self._verified_write(_do_claim, _verify, what=f"claim next {lane!r} as {actor!r}") + return self.get(claimed[0]) if claimed else None def claim_item(self, item_id: str, *, actor: str) -> Item: """Directed claim: atomically claim a SPECIFIC item by id. @@ -2708,6 +2940,15 @@ def claim_item(self, item_id: str, *, actor: str) -> Item: the previous `bd show`-backed check: a directed claim's own refusal-check read can no longer itself lose a serialization conflict either. + + VERIFIED BY READ-BACK (`_verified_write`) on both paths, and the + returned `Item` is the read-back rather than the claiming process's + own stdout: bd must show THIS actor holding the item afterward, or + this raises. A conflict-family failure is decided the same way -- + the item id is known here (unlike `claim_next`), so the proof is a + plain keyed read, with no set-difference fallback needed. A DOMAIN + refusal (bd says someone else holds it, or it does not exist) is + NOT verified away: it raises with bd's own wording, unchanged. """ try: self.get(item_id) # existence check only -- raises if missing @@ -2723,20 +2964,25 @@ def claim_item(self, item_id: str, *, actor: str) -> Item: f"claim again -- directed claims never bypass blockers." ) - p = self._run(["update", item_id, "--claim", "--json"], actor=actor) - if p.returncode != 0: - msg = (p.stderr or p.stdout or "").strip() - raise BeadsError(f"claim {item_id} as {actor!r} failed: {msg[:300]}") - out = (p.stdout or "").strip() - try: - data = json.loads(out) if out else None - except json.JSONDecodeError as e: - raise BeadsError(f"claim {item_id}: bd returned non-JSON: {out[:200]}") from e - items = data if isinstance(data, list) else ([data] if data else []) - items = [i for i in items if isinstance(i, dict) and i.get("id")] - if not items: - raise BeadsError(f"claim {item_id}: bd reported success but returned no item") - return Item.from_beads(items[0]) + def _do_claim() -> subprocess.CompletedProcess: + p = self._run(["update", item_id, "--claim", "--json"], actor=actor) + if p.returncode != 0: + blob = (p.stdout or "") + (p.stderr or "") + if _retryable(blob) or _connection_retryable(blob): + return p # let `_verified_write` decide it by read-back + # A DOMAIN refusal (already held by someone else, not found) + # -- surfaced with bd's own wording, unverified and + # unchanged, exactly as before. + msg = (p.stderr or p.stdout or "").strip() + raise BeadsError(f"claim {item_id} as {actor!r} failed: {msg[:300]}") + return p + + def _verify() -> bool: + back = _get_item_via_sql(self.project_name, item_id) + return back is not None and back.status == "held" and back.holder == actor + + self._verified_write(_do_claim, _verify, what=f"claim {item_id} as {actor!r}") + return self.get(item_id) def get(self, item_id: str, *, with_links: bool = False) -> Item: """Read one item, with its FORWARD dependency graph always attached @@ -3197,25 +3443,35 @@ def release(self, item_id: str) -> ReleaseOutcome: status-mutating write is what makes reopening a closed item structurally impossible from this path, not merely unlikely. - Also applies the same verify-on-conflict discipline `resolve` does: - if the write itself raises (wrapper retry budget exhausted), a - fresh read-back decides the real outcome rather than trusting the - wrapper's failure at face value. + VERIFIED BY READ-BACK on BOTH paths (`_verified_write`). PR #63 gave + this method a read-back on the CONFLICT path only; the SUCCESS path + still returned straight off `p.returncode == 0`, which is the one + thing this module says everywhere else is not proof. It matters + more here than almost anywhere: `release` is what BOTH `work_release` + AND every reap reclaim call, so a `bd update` that exited 0 without + actually clearing the hold left an item still HELD while the sweep + reported it reclaimed -- a hold nobody is renewing and nobody can + claim. The read-back now demands the item is genuinely no longer + `held` before this returns at all (ledger row CCV1-012). """ current = self.get(item_id) if current.status == "resolved": return ReleaseOutcome(item_id=item_id, already_closed=True) - try: - p = self._run(["update", item_id, "--status", "open", "--assignee", ""]) - except BeadsError: + seen: list[Item] = [] + + def _verify() -> bool: back = self._read_back_or_none(item_id) - if back is not None and back.status != "held": - return ReleaseOutcome(item_id=item_id, already_closed=(back.status == "resolved")) - raise - if p.returncode != 0: - detail = _clean_bd_error(p.stderr or p.stdout, limit=200) - raise BeadsError(f"release {item_id}: {detail}") - return ReleaseOutcome(item_id=item_id, already_closed=False) + if back is None or back.status == "held": + return False + seen.append(back) + return True + + self._verified_write( + lambda: self._run(["update", item_id, "--status", "open", "--assignee", ""]), + _verify, + what=f"release {item_id}", + ) + return ReleaseOutcome(item_id=item_id, already_closed=(seen[-1].status == "resolved")) # -------------------------------------------------------- defer / block # @@ -3246,26 +3502,29 @@ def _set_status_with_reason( ) -> Item: if not reason or not reason.strip(): raise BeadsError(f"{status} {item_id}: a reason is required") - p = self._run( - [ - "update", - item_id, - "--status", - status, - "--metadata", - json.dumps({reason_key: reason}), - ], - actor=actor, + args = [ + "update", + item_id, + "--status", + status, + "--metadata", + json.dumps({reason_key: reason}), + ] + seen: list[Item] = [] + + def _verify() -> bool: + back = self.get(item_id) + if back.status != _map_status(status): + return False + if back.meta.get(reason_key) != reason: + return False + seen.append(back) + return True + + self._verified_write( + lambda: self._run(args, actor=actor), _verify, what=f"{status} {item_id}" ) - if p.returncode != 0: - raise BeadsError(f"{status} {item_id}: {_clean_bd_error(p.stderr or p.stdout)}") - back = self.get(item_id) - if back.status != _map_status(status): - raise BeadsError( - f"{status} {item_id} reported success but readback shows status=" - f"{back.status!r} -- refusing to report success" - ) - return back + return seen[-1] def _clear_status_with_reason( self, item_id: str, *, from_status: str, reason_key: str, actor: str | None @@ -3276,26 +3535,27 @@ def _clear_status_with_reason( f"cannot un-{from_status} {item_id}: it is {current.status!r}, not " f"{_map_status(from_status)!r}" ) - p = self._run( - [ - "update", - item_id, - "--status", - "open", - "--unset-metadata", - reason_key, - ], - actor=actor, + args = [ + "update", + item_id, + "--status", + "open", + "--unset-metadata", + reason_key, + ] + seen: list[Item] = [] + + def _verify() -> bool: + back = self.get(item_id) + if back.status != "open" or reason_key in back.meta: + return False + seen.append(back) + return True + + self._verified_write( + lambda: self._run(args, actor=actor), _verify, what=f"un-{from_status} {item_id}" ) - if p.returncode != 0: - raise BeadsError(f"un-{from_status} {item_id}: {_clean_bd_error(p.stderr or p.stdout)}") - back = self.get(item_id) - if back.status != "open": - raise BeadsError( - f"un-{from_status} {item_id} reported success but readback shows status=" - f"{back.status!r} -- refusing to report success" - ) - return back + return seen[-1] def defer(self, item_id: str, reason: str, *, actor: str | None = None) -> Item: """Defer an open item with a reason -- it leaves `bd ready`/ @@ -3374,25 +3634,20 @@ def add_dependency( readback: `get(item_id, with_links=True)` must show the new edge, never merely a non-erroring exit. """ - p = self._run( - ["dep", "add", item_id, depends_on_id, "-t", dep_type], - actor=actor, - ) - if p.returncode != 0: - raise BeadsError( - f"dep add {item_id} -> {depends_on_id} ({dep_type}): " - f"{_clean_bd_error(p.stderr or p.stdout)}" - ) - back = self.get(item_id, with_links=True) - if not any( - link.get("id") == depends_on_id and link.get("direction") == "from" - for link in back.links - ): - raise BeadsError( - f"dep add {item_id} -> {depends_on_id} reported success but readback shows " - f"no such edge -- refusing to report success" + + def _verify() -> bool: + back = self.get(item_id, with_links=True) + return any( + link.get("id") == depends_on_id and link.get("direction") == "from" + for link in back.links ) + self._verified_write( + lambda: self._run(["dep", "add", item_id, depends_on_id, "-t", dep_type], actor=actor), + _verify, + what=f"dep add {item_id} -> {depends_on_id} ({dep_type})", + ) + # ------------------------------------------------------------------ custody # # Liveness for a held item is entirely ours (see amplifier_work_tracker.custody @@ -3426,6 +3681,13 @@ def take_custody( FENCED: refuses unless `holder` is currently bd's own assignee for this item -- you cannot take custody of work you do not actually hold. + + Verified by read-back on BOTH paths (`_verified_write`): the stored + custody record must equal the one written. A FALSE failure here is + not cosmetic -- it produces exactly the held-without-custody state + (an item assigned to a holder that nothing is renewing), which is + why an exhausted-retry raise is decided by reading the record back + rather than believed. """ it = self.get(item_id) if it.holder != holder: @@ -3447,19 +3709,14 @@ def take_custody( "declared_since": now, "generation": gen, } - p = self._run( - ["update", item_id, "--metadata", json.dumps({C.CUSTODY_KEY: record})], - actor=holder, + self._verified_write( + lambda: self._run( + ["update", item_id, "--metadata", json.dumps({C.CUSTODY_KEY: record})], + actor=holder, + ), + lambda: self.get_custody(item_id) == record, + what=f"take_custody {item_id}", ) - if p.returncode != 0: - detail = _clean_bd_error(p.stderr or p.stdout, limit=200) - raise BeadsError(f"take_custody {item_id}: {detail}") - back = self.get_custody(item_id) - if back != record: - raise BeadsError( - f"take_custody {item_id} reported success but readback shows " - f"{back!r} -- refusing to report success" - ) return record def renew_custody( @@ -3477,6 +3734,14 @@ def renew_custody( assignee too. A claim that was taken over while you were away must not be renewable by you; without this, a zombie's renewal would keep an item that no longer belongs to it looking alive forever. + + Verified by read-back on BOTH paths (`_verified_write`). Renewal is + ONE-STRIKE by design (the caller stops renewing for good on any + failure), so a false failure here dooms a live, healthy hold -- the + conflict path must be settled by reading the record back, never by + the wrapper's verdict. This is also the write behind the tool's + `declare`: a phantom success would report a `declared_state` no + reader ever sees. """ it = self.get(item_id) current = it.meta.get(C.CUSTODY_KEY) @@ -3503,19 +3768,14 @@ def renew_custody( if declared_state and declared_state != current.get("declared_state"): updated["declared_state"] = declared_state updated["declared_since"] = updated["last_seen"] - p = self._run( - ["update", item_id, "--metadata", json.dumps({C.CUSTODY_KEY: updated})], - actor=holder, + self._verified_write( + lambda: self._run( + ["update", item_id, "--metadata", json.dumps({C.CUSTODY_KEY: updated})], + actor=holder, + ), + lambda: self.get_custody(item_id) == updated, + what=f"renew_custody {item_id}", ) - if p.returncode != 0: - detail = _clean_bd_error(p.stderr or p.stdout, limit=200) - raise BeadsError(f"renew_custody {item_id}: {detail}") - back = self.get_custody(item_id) - if back != updated: - raise BeadsError( - f"renew_custody {item_id} reported success but readback shows " - f"{back!r} -- refusing to report success" - ) return updated diff --git a/tests/integration/test_write_readback.py b/tests/integration/test_write_readback.py new file mode 100644 index 0000000..d7a8a65 --- /dev/null +++ b/tests/integration/test_write_readback.py @@ -0,0 +1,601 @@ +"""Tier 2 -- every item-level write verb verifies itself by read-back. + +Ledger rows CCV1-012 (`release`'s success path), CCV1-013 (both claim +paths) and CCV1-015 (every remaining item-level write verb); work items +work_item_pipeline-1f2 / -1gz / -2x3. + +Two failure shapes are injected here, and both are silent in production -- +which is exactly why they need a test rather than an argument: + + - **conflict-after-landed**: the write REALLY happens against the isolated + dolt server, then `Beads._run` raises its own exhausted-retries + `BeadsError` anyway. This is the measured 2026-09-01 incident + (work_tracker item pipeline-yym) generalized past `resolve`/`release`. + Every verb must notice the write landed and report SUCCESS. + - **phantom success**: `bd` exits 0 having changed nothing at all. Every + verb must notice the state is not there and RAISE -- "exit code is not + proof" is a claim about the success path first, not only the conflict + path. + +Both are injected by patching `Beads._run` (never a mocked dolt, never +manufactured contention timing) -- the same technique +`test_phantom_conflict_recovery.py` established. Read-back always runs +through the contention-free SQL path, which never goes through `_run` at +all, so a patched `_run` never blinds the verification itself. +""" + +from __future__ import annotations + +import json +import socket +import subprocess + +import pytest + +from amplifier_work_tracker import adapter as A +from amplifier_work_tracker import custody as C + +pytestmark = pytest.mark.integration + + +# --------------------------------------------------------------- injectors + + +def _conflict_after_real_write(monkeypatch, match): + """Patch `Beads._run` so the FIRST call matching `match(args)` really + executes, then raises `_run`'s own exhausted-retries `BeadsError` + regardless of the real outcome. + + Returns the list of intercepted arg-vectors, so a test can assert the + path fired exactly once rather than assuming it did. + """ + real_run = A.Beads._run + calls: list[list[str]] = [] + + def fake_run(self, args, actor=None): # noqa: ANN001 -- matches Beads._run's signature + if not calls and match(args): + calls.append(list(args)) + result = real_run(self, args, actor=actor) + raise A.BeadsError( + f"`bd {' '.join(args[:2])}` still conflicting after 8 retries. " + f"Contention too high; refusing to keep hammering. " + f"Last: {(result.stdout or result.stderr or '').strip()}" + ) + return real_run(self, args, actor=actor) + + monkeypatch.setattr(A.Beads, "_run", fake_run) + return calls + + +def _conflict_without_write(monkeypatch, match): + """Patch `Beads._run` so a call matching `match(args)` raises the same + exhausted-retries `BeadsError` WITHOUT ever performing the write -- the + discriminating negative for every conflict-path test above. + """ + real_run = A.Beads._run + calls: list[list[str]] = [] + + def fake_run(self, args, actor=None): # noqa: ANN001 + if match(args): + calls.append(list(args)) + raise A.BeadsError( + f"`bd {' '.join(args[:2])}` still conflicting after 8 retries. " + f"Contention too high; refusing to keep hammering. " + f"Last: (no successful attempt)" + ) + return real_run(self, args, actor=actor) + + monkeypatch.setattr(A.Beads, "_run", fake_run) + return calls + + +def _phantom_success(monkeypatch, match, *, stdout: str = ""): + """Patch `Beads._run` so a call matching `match(args)` returns exit 0 + (optionally with plausible stdout) having performed NO write at all -- + a `bd` that reports success and changes nothing. + """ + real_run = A.Beads._run + calls: list[list[str]] = [] + + def fake_run(self, args, actor=None): # noqa: ANN001 + if match(args): + calls.append(list(args)) + return subprocess.CompletedProcess(["bd", *args], 0, stdout=stdout, stderr="") + return real_run(self, args, actor=actor) + + monkeypatch.setattr(A.Beads, "_run", fake_run) + return calls + + +# ------------------------------------------------------------- arg matchers + + +def _is_update(args, *flags: str) -> bool: + return bool(args) and args[0] == "update" and all(f in args for f in flags) + + +def _is_custody_write(args) -> bool: + """A `bd update --metadata '{"custody": ...}'` -- take_custody/renew_custody, + told apart from defer/block's own `--metadata` reason write by content. + """ + if not _is_update(args, "--metadata"): + return False + blob = args[args.index("--metadata") + 1] + return f'"{C.CUSTODY_KEY}"' in blob + + +def _held_item(bd: A.Beads, lane: str, actor: str, title: str) -> str: + item_id = bd.create(title, tags=[lane]) + bd.claim_item(item_id, actor=actor) + return item_id + + +# ============================================================ CCV1-012 release + + +def test_release_raises_when_bd_reports_success_but_the_hold_did_not_clear( + shared_bd, unique_lane, monkeypatch +): + """CCV1-012, the discriminating negative for the SUCCESS path: `bd + update --status open --assignee ''` exits 0 and changes nothing. Before + this fix `release` returned `ReleaseOutcome(already_closed=False)` off + that exit code alone -- so `work_release` and every reap reclaim + reported an item handed back that is in fact still HELD. + """ + actor = f"rel-phantom-{unique_lane}" + item_id = _held_item(shared_bd, unique_lane, actor, f"release phantom {unique_lane}") + + calls = _phantom_success(monkeypatch, lambda a: _is_update(a, "--status", "--assignee")) + + with pytest.raises(A.BeadsError, match="did not land"): + shared_bd.release(item_id) + + assert len(calls) == 1 + monkeypatch.undo() + assert shared_bd.get_readonly(item_id).status == "held", "the hold really was never cleared" + shared_bd.resolve(item_id, "cleanup", actor=actor) + + +def test_release_success_path_returns_only_after_the_readback_shows_no_hold(shared_bd, unique_lane): + """The positive half of CCV1-012: a real release still succeeds, and the + item is genuinely no longer held afterward. + """ + actor = f"rel-real-{unique_lane}" + item_id = _held_item(shared_bd, unique_lane, actor, f"release real {unique_lane}") + + outcome = shared_bd.release(item_id) + + assert outcome.item_id == item_id + assert outcome.already_closed is False + back = shared_bd.get_readonly(item_id) + assert back.status == "open" + assert not back.holder + + +# ============================================================== CCV1-013 claim + + +def test_claim_item_verifies_by_readback_when_the_wrapper_reports_conflict( + shared_bd, unique_lane, monkeypatch +): + """A directed claim that really landed, reported as a conflict. The claim + must be recognised as landed -- otherwise the caller believes it holds + nothing while bd has it assigned to them, and only a reap frees it. + """ + actor = f"claim-conflict-{unique_lane}" + item_id = shared_bd.create(f"claim conflict {unique_lane}", tags=[unique_lane]) + + calls = _conflict_after_real_write(monkeypatch, lambda a: _is_update(a, "--claim")) + + item = shared_bd.claim_item(item_id, actor=actor) + + assert len(calls) == 1 + assert item.id == item_id + assert item.holder == actor + assert item.status == "held" + monkeypatch.undo() + assert shared_bd.get_readonly(item_id).holder == actor + shared_bd.resolve(item_id, "cleanup", actor=actor) + + +def test_claim_item_still_raises_when_the_claim_genuinely_did_not_land( + shared_bd, unique_lane, monkeypatch +): + """The discriminating negative: a conflict whose write never happened + still raises. Verify-by-read-back is a safety net for a landed write, + never a way to swallow a real failure. + """ + actor = f"claim-genuine-{unique_lane}" + item_id = shared_bd.create(f"claim genuine fail {unique_lane}", tags=[unique_lane]) + + _conflict_without_write(monkeypatch, lambda a: _is_update(a, "--claim")) + + with pytest.raises(A.BeadsError, match="still conflicting"): + shared_bd.claim_item(item_id, actor=actor) + + monkeypatch.undo() + assert shared_bd.get_readonly(item_id).status == "open" + + +def test_claim_item_raises_when_bd_reports_success_but_nobody_holds_the_item( + shared_bd, unique_lane, monkeypatch +): + """CCV1-013's core claim: the returned item used to be parsed from the + WRITING process's own stdout. A `bd` that prints a plausible claimed + item while writing nothing therefore produced a caller that believed it + held work nobody had assigned it -- and then started custody on it. + """ + actor = f"claim-phantom-{unique_lane}" + item_id = shared_bd.create(f"claim phantom {unique_lane}", tags=[unique_lane]) + fake = json.dumps([{"id": item_id, "title": "phantom", "status": "in_progress"}]) + + _phantom_success(monkeypatch, lambda a: _is_update(a, "--claim"), stdout=fake) + + with pytest.raises(A.BeadsError, match="did not land"): + shared_bd.claim_item(item_id, actor=actor) + + monkeypatch.undo() + assert shared_bd.get_readonly(item_id).status == "open" + + +def test_claim_next_verifies_by_readback_when_the_wrapper_reports_conflict( + shared_bd, unique_lane, monkeypatch +): + """`claim_next` cannot read the claim back by id on the conflict path -- + bd chose the item and its stdout is gone. It is decided instead by the + id-set difference over items assigned to this actor, snapshotted before + the write. + """ + actor = f"next-conflict-{unique_lane}" + item_id = shared_bd.create(f"claim next conflict {unique_lane}", tags=[unique_lane]) + + calls = _conflict_after_real_write(monkeypatch, lambda a: bool(a) and a[0] == "ready") + + item = shared_bd.claim_next(lane=unique_lane, actor=actor) + + assert len(calls) == 1 + assert item is not None and item.id == item_id + assert item.holder == actor + monkeypatch.undo() + assert shared_bd.get_readonly(item_id).holder == actor + shared_bd.resolve(item_id, "cleanup", actor=actor) + + +def test_claim_next_still_raises_when_the_claim_genuinely_did_not_land( + shared_bd, unique_lane, monkeypatch +): + """No new hold appeared for this actor, so the set difference is empty: + the original conflict propagates untouched. + """ + actor = f"next-genuine-{unique_lane}" + item_id = shared_bd.create(f"claim next genuine {unique_lane}", tags=[unique_lane]) + + _conflict_without_write(monkeypatch, lambda a: bool(a) and a[0] == "ready") + + with pytest.raises(A.BeadsError, match="still conflicting"): + shared_bd.claim_next(lane=unique_lane, actor=actor) + + monkeypatch.undo() + assert shared_bd.get_readonly(item_id).status == "open" + + +def test_claim_next_raises_when_bd_names_an_item_it_never_actually_claimed( + shared_bd, unique_lane, monkeypatch +): + """The phantom-success half of CCV1-013 for the queue claim.""" + actor = f"next-phantom-{unique_lane}" + item_id = shared_bd.create(f"claim next phantom {unique_lane}", tags=[unique_lane]) + fake = json.dumps([{"id": item_id, "title": "phantom", "status": "in_progress"}]) + + _phantom_success(monkeypatch, lambda a: bool(a) and a[0] == "ready", stdout=fake) + + with pytest.raises(A.BeadsError, match="did not land"): + shared_bd.claim_next(lane=unique_lane, actor=actor) + + monkeypatch.undo() + assert shared_bd.get_readonly(item_id).status == "open" + + +def test_claim_next_on_an_empty_queue_is_still_a_normal_none(shared_bd, unique_lane): + """An empty queue is a normal terminal outcome, never a failed write -- + the verification must not turn "nothing to claim" into an error. + """ + assert shared_bd.claim_next(lane=f"{unique_lane}-empty", actor=f"empty-{unique_lane}") is None + + +# ============================================== CCV1-015 every other write verb + + +def test_create_verifies_by_readback_when_the_wrapper_reports_conflict( + shared_bd, unique_lane, monkeypatch +): + """`create` is the one verb whose conflict-path key is not an id -- bd + prints the new id on stdout, which the conflict destroys. Decided by the + id-set difference over this exact title, which also RECOVERS the id. + """ + title = f"create conflict {unique_lane}" + calls = _conflict_after_real_write(monkeypatch, lambda a: bool(a) and a[0] == "create") + + item_id = shared_bd.create(title, tags=[unique_lane]) + + assert len(calls) == 1 + assert item_id + monkeypatch.undo() + assert shared_bd.get_readonly(item_id).title == title + + +def test_create_still_raises_when_the_create_genuinely_did_not_land( + shared_bd, unique_lane, monkeypatch +): + """No new row with this title appeared, so the conflict propagates.""" + _conflict_without_write(monkeypatch, lambda a: bool(a) and a[0] == "create") + + with pytest.raises(A.BeadsError, match="still conflicting"): + shared_bd.create(f"create genuine fail {unique_lane}", tags=[unique_lane]) + + +def test_create_raises_when_bd_prints_an_id_it_never_actually_wrote( + shared_bd, unique_lane, monkeypatch +): + """Phantom success: an id on stdout with no row behind it used to be + returned verbatim, handing the caller a dangling id. + """ + _phantom_success( + monkeypatch, lambda a: bool(a) and a[0] == "create", stdout="work_tracker-nope\n" + ) + + with pytest.raises(A.BeadsError, match="did not land"): + shared_bd.create(f"create phantom {unique_lane}", tags=[unique_lane]) + + +def test_update_verifies_by_readback_when_the_wrapper_reports_conflict( + shared_bd, unique_lane, monkeypatch +): + item_id = shared_bd.create(f"update conflict {unique_lane}", tags=[unique_lane]) + calls = _conflict_after_real_write(monkeypatch, lambda a: _is_update(a, "--title")) + + back = shared_bd.update(item_id, title=f"updated {unique_lane}") + + assert len(calls) == 1 + assert back.title == f"updated {unique_lane}" + monkeypatch.undo() + assert shared_bd.get_readonly(item_id).title == f"updated {unique_lane}" + + +def test_edit_item_verifies_by_readback_when_the_wrapper_reports_conflict( + shared_bd, unique_lane, monkeypatch +): + """`edit` = a verified field write plus its audit comment. Both halves go + through the shared helper, so a conflicted-but-landed comment is not a + reported failure either. + """ + item_id = shared_bd.create(f"edit conflict {unique_lane}", tags=[unique_lane]) + calls = _conflict_after_real_write(monkeypatch, lambda a: bool(a) and a[0] == "comment") + + back = shared_bd.edit_item( + item_id, description=f"edited {unique_lane}", actor=f"editor-{unique_lane}" + ) + + assert len(calls) == 1 + assert back.description == f"edited {unique_lane}" + monkeypatch.undo() + texts = [e.detail for e in shared_bd.activity(item_id) if e.kind == "comment"] + assert any("edited: description" in (t or "") for t in texts) + + +def test_comment_raises_when_bd_reports_success_but_no_comment_landed( + shared_bd, unique_lane, monkeypatch +): + item_id = shared_bd.create(f"comment phantom {unique_lane}", tags=[unique_lane]) + _phantom_success(monkeypatch, lambda a: bool(a) and a[0] == "comment") + + with pytest.raises(A.BeadsError, match="did not land"): + shared_bd.comment(item_id, f"never written {unique_lane}") + + +def test_defer_verifies_by_readback_when_the_wrapper_reports_conflict( + shared_bd, unique_lane, monkeypatch +): + item_id = shared_bd.create(f"defer conflict {unique_lane}", tags=[unique_lane]) + calls = _conflict_after_real_write(monkeypatch, lambda a: _is_update(a, "--status", "deferred")) + + back = shared_bd.defer(item_id, "waiting on upstream") + + assert len(calls) == 1 + assert back.status == "deferred" + monkeypatch.undo() + assert shared_bd.get_readonly(item_id).status == "deferred" + + +def test_defer_raises_when_bd_reports_success_but_the_status_did_not_move( + shared_bd, unique_lane, monkeypatch +): + item_id = shared_bd.create(f"defer phantom {unique_lane}", tags=[unique_lane]) + _phantom_success(monkeypatch, lambda a: _is_update(a, "--status", "deferred")) + + with pytest.raises(A.BeadsError, match="did not land"): + shared_bd.defer(item_id, "never lands") + + monkeypatch.undo() + assert shared_bd.get_readonly(item_id).status == "open" + + +def test_block_verifies_by_readback_when_the_wrapper_reports_conflict( + shared_bd, unique_lane, monkeypatch +): + item_id = shared_bd.create(f"block conflict {unique_lane}", tags=[unique_lane]) + calls = _conflict_after_real_write(monkeypatch, lambda a: _is_update(a, "--status", "blocked")) + + back = shared_bd.block(item_id, "needs a decision") + + assert len(calls) == 1 + assert back.status == "blocked" + monkeypatch.undo() + assert shared_bd.get_readonly(item_id).status == "blocked" + + +def test_unblock_verifies_by_readback_when_the_wrapper_reports_conflict( + shared_bd, unique_lane, monkeypatch +): + """The clear side of the same pair -- `--unset-metadata` back to open.""" + item_id = shared_bd.create(f"unblock conflict {unique_lane}", tags=[unique_lane]) + shared_bd.block(item_id, "needs a decision") + calls = _conflict_after_real_write(monkeypatch, lambda a: _is_update(a, "--unset-metadata")) + + back = shared_bd.unblock(item_id) + + assert len(calls) == 1 + assert back.status == "open" + monkeypatch.undo() + assert shared_bd.get_readonly(item_id).status == "open" + + +def test_add_dependency_verifies_by_readback_when_the_wrapper_reports_conflict( + shared_bd, unique_lane, monkeypatch +): + a_id = shared_bd.create(f"dep conflict a {unique_lane}", tags=[unique_lane]) + b_id = shared_bd.create(f"dep conflict b {unique_lane}", tags=[unique_lane]) + calls = _conflict_after_real_write(monkeypatch, lambda a: a[:2] == ["dep", "add"]) + + shared_bd.add_dependency(a_id, b_id) + + assert len(calls) == 1 + monkeypatch.undo() + links = shared_bd.get(a_id, with_links=True).links + assert any(link["id"] == b_id and link["direction"] == "from" for link in links) + + +def test_add_dependency_raises_when_bd_reports_success_but_no_edge_landed( + shared_bd, unique_lane, monkeypatch +): + a_id = shared_bd.create(f"dep phantom a {unique_lane}", tags=[unique_lane]) + b_id = shared_bd.create(f"dep phantom b {unique_lane}", tags=[unique_lane]) + _phantom_success(monkeypatch, lambda a: a[:2] == ["dep", "add"]) + + with pytest.raises(A.BeadsError, match="did not land"): + shared_bd.add_dependency(a_id, b_id) + + +def test_take_custody_verifies_by_readback_when_the_wrapper_reports_conflict( + shared_bd, unique_lane, monkeypatch +): + """A false failure here produces exactly the CCV1-003 shape: the item is + HELD with a custody record that the caller believes does not exist. + """ + actor = f"custody-conflict-{unique_lane}" + item_id = _held_item(shared_bd, unique_lane, actor, f"take custody conflict {unique_lane}") + calls = _conflict_after_real_write(monkeypatch, _is_custody_write) + + rec = shared_bd.take_custody(item_id, holder=actor, pid=4242, host=socket.gethostname()) + + assert len(calls) == 1 + assert rec["holder"] == actor + monkeypatch.undo() + assert shared_bd.get_custody(item_id) == rec + shared_bd.resolve(item_id, "cleanup", actor=actor) + + +def test_renew_custody_verifies_by_readback_when_the_wrapper_reports_conflict( + shared_bd, unique_lane, monkeypatch +): + """Renewal is ONE-STRIKE (Core 4): a renewal failure stops the loop for + good. A false failure here therefore dooms a live, healthy hold -- which + is why the conflict path must be decided by read-back, not by the + wrapper's verdict. + """ + actor = f"renew-conflict-{unique_lane}" + item_id = _held_item(shared_bd, unique_lane, actor, f"renew custody conflict {unique_lane}") + rec = shared_bd.take_custody(item_id, holder=actor, pid=4242, host=socket.gethostname()) + + calls = _conflict_after_real_write(monkeypatch, _is_custody_write) + + updated = shared_bd.renew_custody( + item_id, + holder=actor, + generation=rec["generation"], + pid=4243, + declared_state=C.STATE_AWAITING_HUMAN, + ) + + assert len(calls) == 1 + assert updated["declared_state"] == C.STATE_AWAITING_HUMAN + assert updated["pid"] == 4243 + monkeypatch.undo() + assert shared_bd.get_custody(item_id) == updated + shared_bd.resolve(item_id, "cleanup", actor=actor) + + +def test_renew_custody_raises_when_bd_reports_success_but_the_record_did_not_move( + shared_bd, unique_lane, monkeypatch +): + """`declare` (the tool's `work_declare`) is this write. A phantom success + would report a declared state that no reader ever sees. + """ + actor = f"renew-phantom-{unique_lane}" + item_id = _held_item(shared_bd, unique_lane, actor, f"renew custody phantom {unique_lane}") + rec = shared_bd.take_custody(item_id, holder=actor, pid=4242, host=socket.gethostname()) + + _phantom_success(monkeypatch, _is_custody_write) + + with pytest.raises(A.BeadsError, match="did not land"): + shared_bd.renew_custody( + item_id, + holder=actor, + generation=rec["generation"], + pid=4243, + declared_state=C.STATE_AWAITING_HUMAN, + ) + + monkeypatch.undo() + assert shared_bd.get_custody(item_id) == rec, "the record really never moved" + shared_bd.resolve(item_id, "cleanup", actor=actor) + + +def test_supersede_verifies_by_readback_when_the_wrapper_reports_conflict( + shared_bd, unique_lane, monkeypatch +): + old_id = shared_bd.create(f"supersede old {unique_lane}", tags=[unique_lane]) + new_id = shared_bd.create(f"supersede new {unique_lane}", tags=[unique_lane]) + calls = _conflict_after_real_write(monkeypatch, lambda a: bool(a) and a[0] == "supersede") + + back = shared_bd.supersede(old_id, new_id) + + assert len(calls) == 1 + assert back.status == "resolved" + monkeypatch.undo() + assert shared_bd.get_readonly(old_id).status == "resolved" + + +# ------------------------------------------------------------------ move +# +# `move_item` is the one verb in this list that never touches `bd` or +# `Beads._run` at all -- it is direct dolt SQL, so the conflict-family +# retry hazard the helper exists for cannot reach it. It carries its own, +# older read-back: real row counts in `dst` compared against `src`'s counts +# taken before anything moved, plus a residue check in `src` after the +# delete. This test proves that verification is load-bearing rather than +# decorative, by making the copy LOOK successful while the rows are not +# there -- the move must refuse and leave `src` intact. + + +def test_move_refuses_when_the_copy_reports_success_but_the_rows_are_not_there( + workspace, project_factory, monkeypatch +): + src_name, src_bd = project_factory("mvsrc") + dst_name, _ = project_factory("mvdst") + item_id = src_bd.create("move verification", tags=[A.LANE_WORK]) + + real_counts = A._item_row_counts # noqa: SLF001 -- test injection + + def fake_counts(db: str, iid: str) -> dict[str, int]: + if db == dst_name: # dst looks empty no matter what the copy reported + return dict.fromkeys(real_counts(db, iid), 0) + return real_counts(db, iid) + + monkeypatch.setattr(A, "_item_row_counts", fake_counts) + + with pytest.raises(A.BeadsError, match="incomplete copy"): + workspace.move_item(src_name, dst_name, item_id) + + monkeypatch.undo() + assert src_bd.get_readonly(item_id).title == "move verification", "src must be untouched" From e446780cd4aa23ba14250ca7dc272367e54f009e Mon Sep 17 00:00:00 2001 From: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com> Date: Wed, 2 Sep 2026 21:30:00 -0700 Subject: [PATCH 5/7] test: run modules/tool-work-tracker/tests in make test and CI (CCV1-022) The tool module's own suite -- the only place the post-reclaim custody behaviour of the agent seam (work_claim / work_declare / work_resolve / work_release) is asserted mechanically -- was importable by nothing and run by nothing: `import amplifier_module_tool_work_tracker` raised ModuleNotFoundError in the repo venv, `make test` was `pytest tests ledger/checks -v`, and ci.yml named only tiers 1-4. Six green claims nobody had ever executed, and the Freeze blocker for contracts/custody-coordination.v1.md. Wired three ways, keeping ONE venv and ONE setup command: - `make venv` and ci.yml's setup step now install the module editable alongside the root package (`-e "modules/tool-work-tracker[dev]"`). Its dev extra already declared the two test-only deps the root package does not need (amplifier-core, pytest-asyncio), so nothing is duplicated into the root dev extra. - Makefile gains `test-module`; `make test` runs both invocations and is deliberately not fail-fast between them, so a pre-existing root-suite failure cannot go back to hiding tier 5's result. - ci.yml gains a "Tier 5 -- tool module tests" step. Separate pytest invocations on purpose: the module suite ships its own session-scoped isolated dolt server fixture (fixtures cannot cross a pytest run), so folding it into a tier would stand up two servers in one session. Measured 2026-09-02: 94 collected, 93 passed, 1 xfailed in 325s against real bd. The xfail is strict and is a PRODUCT defect, not a wiring one: test_reap_recovery.py::test_explicit_resolve_refusal_after_reap_clears_ held_and_allows_new_claim is CCV1-009's post-reclaim fence gap (`Beads.resolve`'s fence runs only under `if current.status == "held"`, and a reaped item is `open`), now measured behaviourally for the first time rather than inferred from source. Not fixed here; strict=True means the day CCV1-009 lands, the xfail fails and this pin must be removed. Ledger row CCV1-022: VIOLATION -> CONFORMS, and its probe rewritten from an absence pin to an assertion that all three halves of the wiring are present. `pytest ledger/checks -q` green (24 passed). --- .github/workflows/ci.yml | 21 +++++++- Makefile | 39 ++++++++++++-- ledger/checks/test_custody_rows.py | 52 +++++++++++++++---- ledger/rows.yaml | 40 +++++++++----- .../tests/test_reap_recovery.py | 10 ++++ 5 files changed, 135 insertions(+), 27 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index ed4ed07..dfec3ee 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -69,10 +69,17 @@ jobs: # here or anywhere else. A server started here as a separate step # would just be an unused, orphaned process. + # ONE venv, ONE command -- kept byte-identical in intent to the + # Makefile's `venv` target. The second editable target is the + # amplifier tool module under modules/: a separate installable package + # whose absence made `import amplifier_module_tool_work_tracker` a + # ModuleNotFoundError here, which is why its suite ran in nothing + # (ledger row CCV1-022). Its `[dev]` extra carries that suite's + # test-only deps (amplifier-core, pytest-asyncio). - name: Set up amplifier-work-tracker run: | uv venv .venv --python 3.12 - uv pip install --python .venv/bin/python -e ".[dev,web]" + uv pip install --python .venv/bin/python -e ".[dev,web]" -e "modules/tool-work-tracker[dev]" - name: Lint + format check (ruff) run: .venv/bin/ruff check . && .venv/bin/ruff format --check . @@ -97,6 +104,18 @@ jobs: - name: Tier 4 -- conformance ledger run: .venv/bin/python -m pytest ledger/checks -v + # Tier 5 -- the amplifier tool module's own suite + # (modules/tool-work-tracker/tests). The only place the post-reclaim + # custody behaviour of the AGENT SEAM (work_claim / work_declare / + # work_resolve / work_release) is asserted mechanically; before this + # step existed the suite was importable by nothing and run by nothing + # (ledger row CCV1-022). A separate pytest invocation on purpose: it + # brings its own session-scoped isolated dolt server fixture, so + # folding it into a tier above would stand up two servers in one + # session. Uses the same pinned `bd` installed above. + - name: Tier 5 -- tool module tests + run: .venv/bin/python -m pytest modules/tool-work-tracker/tests -v + - name: Dolt server log (always, for debugging) if: always() run: cat /tmp/dolt-server.log || true diff --git a/Makefile b/Makefile index 3165212..2f50cfb 100644 --- a/Makefile +++ b/Makefile @@ -1,4 +1,4 @@ -.PHONY: venv test test-unit test-integration test-cli test-ledger check lint types doctor clean +.PHONY: venv test test-unit test-integration test-cli test-ledger test-module check lint types doctor clean PYTHON ?= python3.12 VENV := .venv @@ -7,9 +7,19 @@ PYTEST := $(PY) -m pytest # One-time (or after dependency changes) environment setup. Uses `uv` for # speed; falls back to nothing fancier than a normal editable install. +# +# ONE venv, ONE command. The second editable target is the amplifier tool +# module under modules/ -- it is a separate installable package with its own +# pyproject.toml, and without it `import amplifier_module_tool_work_tracker` +# raises ModuleNotFoundError in this venv, which is exactly why its test +# suite ran in nothing (ledger row CCV1-022). Its `[dev]` extra carries the +# two test-only dependencies that suite needs and the root package does not +# (amplifier-core, pytest-asyncio); they are declared there rather than +# duplicated into the root `dev` extra so each package keeps owning its own +# dependencies. venv: uv venv $(VENV) --python $(PYTHON) - uv pip install --python $(PY) -e ".[dev,web]" + uv pip install --python $(PY) -e ".[dev,web]" -e "modules/tool-work-tracker[dev]" ## Tier 1 -- unit: pure logic, no bd, no network. Target: whole tier < 5s. test-unit: @@ -30,9 +40,30 @@ test-cli: test-ledger: $(PYTEST) ledger/checks -v -## All four tiers. +## Tier 5 -- tool module: modules/tool-work-tracker's own suite, the only +## place the post-reclaim custody behaviour of the AGENT SEAM (work_claim / +## work_declare / work_resolve / work_release) is asserted mechanically. +## Deliberately its own pytest invocation rather than another path argument +## on `test` below: that suite ships its own session-scoped isolated dolt +## server fixture (a copy of the root suite's, since fixtures cannot cross +## a pytest run), and folding the two together would stand up two servers +## in one session for no gain. Requires `make venv` (the module must be +## installed into the venv) and a real `bd` on PATH -- without bd the +## real-storage tests skip rather than fail. +test-module: + $(PYTEST) modules/tool-work-tracker/tests -v + +## All five tiers. Two pytest invocations (see `test-module` above), and +## deliberately NOT fail-fast between them: the whole point of wiring the +## module suite in (ledger row CCV1-022) is that it stops being silently +## skippable, and a pre-existing failure in the root suite must not go back +## to hiding tier 5's result. Both always run; the target still fails if +## either did. test: - $(PYTEST) tests ledger/checks -v + @rc=0; \ + $(PYTEST) tests ledger/checks -v || rc=$$?; \ + $(PYTEST) modules/tool-work-tracker/tests -v || rc=$$?; \ + exit $$rc ## Lint + type-check. check: lint types diff --git a/ledger/checks/test_custody_rows.py b/ledger/checks/test_custody_rows.py index fba0461..ff85af6 100644 --- a/ledger/checks/test_custody_rows.py +++ b/ledger/checks/test_custody_rows.py @@ -325,19 +325,53 @@ def test_row_ccv1_021() -> None: def test_row_ccv1_022() -> None: - """Freeze Bar VIOLATION pin (absence): nothing runs the tool module's own - test suite -- neither `make test` nor CI names it. The only mechanical - assertions of post-reclaim custody behavior live there. + """Freeze Bar CONFORMS: the tool module's own suite is importable and is + run -- by `make test` and by CI. Asserts all three halves of the wiring, + because any one of them going missing silently returns the suite to + "green claims nobody has ever run", which is the state this row closed. + + Source-level on purpose (this ledger is in-process only, no bd/dolt/ + subprocess). The suite's actual green-ness is measured by running it -- + Makefile `test-module` / CI "Tier 5" -- and recorded in the row's notes. """ + module_pkg = "modules/tool-work-tracker" + + # 1. Installed into the same, single venv -- without the editable install + # `import amplifier_module_tool_work_tracker` is a ModuleNotFoundError + # and the suite cannot even be collected. for path, label in ((MAKEFILE, "Makefile"), (CI_WORKFLOW, ".github/workflows/ci.yml")): - assert "tool-work-tracker" not in read(path), ( - f"CCV1-022 (Freeze Bar, VIOLATION) pin: {label} now references " - f"the tool-work-tracker module. If the suite genuinely RUNS (importable and " - f"green), this is the expected failure: flip the row to CONFORMS, upgrade the " - f"rows that depend on it (CCV1-004, CCV1-009, CCV1-010, CCV1-017) from " - f"source-pinned probes to behavioral cites, and resolve work_item_pipeline-a7n." + assert contains(path, f'-e "{module_pkg}[dev]"'), ( + f"CCV1-022 (Freeze Bar, CONFORMS): {label} no longer installs the tool module " + f"editable into the venv. Without it `import amplifier_module_tool_work_tracker` " + f"raises ModuleNotFoundError and the suite runs in nothing again -- the exact " + f"VIOLATION this row closed (work_item_pipeline-a7n)." ) + # 2. `make test` runs it, via its own target AND as part of the full run. + makefile = read(MAKEFILE) + assert "test-module:" in makefile, ( + "CCV1-022: the `test-module` target is gone from the Makefile" + ) + assert makefile.count(f"$(PYTEST) {module_pkg}/tests") >= 2, ( + f"CCV1-022: `{module_pkg}/tests` must be run BOTH by the `test-module` target and " + f"by the all-tiers `test` target -- a target nothing aggregates is a target CI and " + f"contributors forget." + ) + + # 3. CI runs it as its own step. + assert contains(CI_WORKFLOW, f"pytest {module_pkg}/tests"), ( + "CCV1-022: CI no longer runs the tool module tests (the `Tier 5 -- tool module " + "tests` step). The Freeze Bar clause is specifically about CI." + ) + + # 4. The suite it points at still exists and still holds the post-reclaim + # custody assertions that made this row the Freeze blocker. + suite = REPO_ROOT / module_pkg / "tests" + assert (suite / "test_reap_recovery.py").exists(), ( + "CCV1-022: modules/tool-work-tracker/tests/test_reap_recovery.py is gone -- the " + "wiring is worth nothing without the tests it wires in." + ) + # --------------------------------------------------------------- CCV1-023 diff --git a/ledger/rows.yaml b/ledger/rows.yaml index ac2805f..2ef3bd5 100644 --- a/ledger/rows.yaml +++ b/ledger/rows.yaml @@ -547,28 +547,42 @@ suite runs is CCV1-022. - id: CCV1-022 - title: modules/tool-work-tracker/tests is run by nothing + title: modules/tool-work-tracker/tests is importable and run by make test and CI contract: file: contracts/custody-coordination.v1.md clause: Freeze Bar quote: | Test suite (`tests/test_*.py`) is importable and run as part of CI - disposition: VIOLATION + disposition: CONFORMS work: work_item_pipeline-a7n assertion: - kind: absence + kind: probe ref: test_row_ccv1_022 notes: > - Measured this run, verbatim -- `./.venv/bin/python -c "import - amplifier_module_tool_work_tracker"` -> ModuleNotFoundError; `make - test` is `pytest tests -v`; ci.yml runs tests/unit, tests/integration, - tests/cli. The identical ImportError was recorded in the Phase-0 - evidence brief (sec.D-11) against main @ deffa54 and reproduces on - b5b23ca. THE FREEZE BLOCKER: the only mechanical assertions of - post-reclaim custody behavior live in that suite -- six tests, all - green claims nobody has ever run. Rows CCV1-004, CCV1-009, CCV1-010 and - CCV1-017 are all weaker than they should be because of it. Ratified - Call 8 makes running the kit a Freeze precondition. + CLOSED by work_item_pipeline-a7n. Was VIOLATION (the Freeze blocker): + `import amplifier_module_tool_work_tracker` raised ModuleNotFoundError + in the repo venv, `make test` was `pytest tests ledger/checks -v`, and + ci.yml named only tiers 1-4 -- so the only mechanical assertions of + post-reclaim custody behaviour were green claims nobody had ever run + (Phase-0 evidence brief sec.D-11, reproduced on b5b23ca). Now wired + three ways, one venv and one setup command: `make venv` and ci.yml's + "Set up amplifier-work-tracker" both install the module editable + (`-e "modules/tool-work-tracker[dev]"`, whose dev extra carries + amplifier-core + pytest-asyncio); the Makefile gains a `test-module` + target and `make test` runs the suite as its own pytest invocation; + ci.yml gains a "Tier 5 -- tool module tests" step. MEASURED 2026-09-02 + on this branch: `import amplifier_module_tool_work_tracker` exits 0 and + `make test-module` collects 94 and reports 93 passed, 1 xfailed in + 332s against real bd + this suite's own isolated dolt server. HONEST LIMIT: that single xfail(strict=True) is + test_reap_recovery.py::test_explicit_resolve_refusal_after_reap_clears_ + held_and_allows_new_claim -- a PRODUCT defect, not a wiring one, and it + is CCV1-009's post-reclaim fence gap, now measured behaviourally for + the first time rather than inferred from source. Its strictness is what + keeps this row honest: the day CCV1-009 is fixed, the xfail fails. + Consequence for the rows that were weakened by this one (CCV1-004, + CCV1-009, CCV1-010, CCV1-017): they may now be upgraded from + source-pinned probes to behavioural cites -- NOT done here (a + single-row lane), left as named follow-up. - id: CCV1-023 title: Conformance Fixtures 2, 3 and 4 are not implemented-and-runnable diff --git a/modules/tool-work-tracker/tests/test_reap_recovery.py b/modules/tool-work-tracker/tests/test_reap_recovery.py index 3d56783..40e342d 100644 --- a/modules/tool-work-tracker/tests/test_reap_recovery.py +++ b/modules/tool-work-tracker/tests/test_reap_recovery.py @@ -50,6 +50,16 @@ def _force_reap(session: WorkTrackerSession, project_name: str) -> dict[str, Any return SV.reap_project(bd, ttl_seconds=0) +@pytest.mark.xfail( + strict=True, + reason=( + "CCV1-009 (work_item_pipeline-dn4): a post-reclaim close is not fenced -- " + "`Beads.resolve`'s fence block runs only under `if current.status == 'held'`, " + "and a reaped item is `open`, so the stale holder's resolve lands instead of " + "being refused. PRODUCT defect, first mechanically measured by this test once " + "the suite was actually wired into `make test`/CI (CCV1-022); not fixed here." + ), +) @pytest.mark.asyncio async def test_explicit_resolve_refusal_after_reap_clears_held_and_allows_new_claim(project): """Trigger path 1: the explicit `work_resolve` refusal. Claim -> force From ab6f90dc691be554f9713772b809c144b3fccb35 Mon Sep 17 00:00:00 2001 From: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com> Date: Wed, 2 Sep 2026 21:49:59 -0700 Subject: [PATCH 6/7] style: ruff format after readback merge-conflict resolution --- ledger/checks/test_custody_rows.py | 1 + 1 file changed, 1 insertion(+) diff --git a/ledger/checks/test_custody_rows.py b/ledger/checks/test_custody_rows.py index 30e7ea1..d6e0205 100644 --- a/ledger/checks/test_custody_rows.py +++ b/ledger/checks/test_custody_rows.py @@ -51,6 +51,7 @@ # contention contract. Kept local to this module (only CCV1-016 pins it). CLI = REPO_ROOT / "src" / "amplifier_work_tracker" / "cli.py" + def _beads_method(name: str) -> str: """The whitespace-collapsed source of exactly ONE `Beads` method. From 8bc167b75988c53011bcfde2145393fe23a4993b Mon Sep 17 00:00:00 2001 From: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com> Date: Wed, 2 Sep 2026 22:01:11 -0700 Subject: [PATCH 7/7] test: Conformance Fixtures 2, 3 and 4 as runnable discriminating pairs (CCV1-023) The contract's Freeze Bar requires "All four Conformance fixtures implemented, passing, and executable via `make test`". Fixture 1 already existed (tests/integration/test_phantom_conflict_recovery.py); 2, 3 and 4 did not exist in any suite. Adds modules/tool-work-tracker/tests/test_conformance_fixtures.py -- ten tests at the agent seam the contract writes the fixtures against (work_resolve / work_status / work_claim), each fixture carrying BOTH halves of its good/bad pair, since one half alone passes against the broken implementation too: Fixture 2 (post-reclaim close fence): a stale holder's work_resolve after a real reap sweep is refused and the item stays open; a live holder's resolve and PR #51's integrator close both still succeed. Fixture 3 (in-process recovery): a session wedged holding an item bd already considers resolved clears its latch via work_release, gets the sanctioned already_closed outcome, and claims again -- with the closed item's entire record asserted byte-identical across the recovery call, which is what proves the Backlogged section's named hazard ("release on a resolved item would reopen it") cannot happen. Plus work_status surfacing holding.custody_lost on the retained-hold path. Fixture 4 (single hold per session): a second claim is refused naming the held item in both directed and queue mode, the would-be second item is untouched, and the constraint lifts after either exit from a hold. Discrimination measured, not asserted: each bad half was run against a deliberately reverted implementation and fails there while every good half stays green (post-reclaim fence branch, release's pre-write resolved guard, claim's single-hold gate). Also removes the now-obsolete xfail(strict=True) on test_reap_recovery.py's post-reclaim resolve test. It pinned CCV1-009's pre-fix gap; the fence has landed, so the strict marker was XPASSing and failing the module suite -- exactly as CCV1-022's own notes predicted it would. Ledger row CCV1-023: GAP -> CONFORMS, probe rewritten from an absence pin to one that asserts the fixture files exist, each fixture keeps >= 2 halves, and no half is xfail'd or skipped. The contract's four stale "Test location" lines are recorded as drift and still pinned; contracts/ is not edited. --- ledger/checks/test_custody_rows.py | 109 +++- ledger/rows.yaml | 75 ++- .../tests/test_conformance_fixtures.py | 465 ++++++++++++++++++ .../tests/test_reap_recovery.py | 23 +- 4 files changed, 616 insertions(+), 56 deletions(-) create mode 100644 modules/tool-work-tracker/tests/test_conformance_fixtures.py diff --git a/ledger/checks/test_custody_rows.py b/ledger/checks/test_custody_rows.py index d6e0205..45921dc 100644 --- a/ledger/checks/test_custody_rows.py +++ b/ledger/checks/test_custody_rows.py @@ -629,34 +629,89 @@ def test_row_ccv1_022() -> None: def test_row_ccv1_023() -> None: - """Freeze Bar GAP pin (absence): three of the four Conformance fixtures are - not implemented-and-runnable, and the contract's own "Test location" - lines point at files that do not exist. + """Freeze Bar CONFORMS: all four Conformance fixtures exist as + discriminating good/bad pairs, in files the already-wired test paths + collect, with nothing quietly disabled. + + Source-level on purpose (this ledger is in-process only -- no bd, no + dolt, no subprocess), so this probe asserts the three things a static + check honestly CAN: the fixture files exist, each fixture contributes + real test functions, and no half is marked `xfail`/`skip`. That the + fixtures actually PASS is measured by running them (`make test-module` + / CI Tier 5) and recorded in the row's notes -- see CCV1-020 on why a + probe is never the behavioural proof. + + That those paths are RUN at all is CCV1-022's clause, asserted by + `test_row_ccv1_022` (venv install + `test-module` target + `make test` + aggregation + the CI step). Not restated here: two rows asserting the + same wiring is how one of them silently stops meaning anything. """ - for named in ( - "tests/test_incident_b.py", - "tests/test_reap_recovery.py", - "tests/test_recovery.py", - "tests/test_single_hold.py", + module_suite = REPO_ROOT / "modules" / "tool-work-tracker" / "tests" + fixtures_2_3_4 = module_suite / "test_conformance_fixtures.py" + fixture_1 = REPO_ROOT / "tests" / "integration" / "test_phantom_conflict_recovery.py" + + # 1. Fixture 1 (conflicted-but-landed close) -- already existed when this + # row was opened; only the contract's pointer at it was ever wrong. + assert fixture_1.exists(), ( + "CCV1-023 (Freeze Bar, CONFORMS): tests/integration/test_phantom_conflict_recovery.py " + "is gone -- Fixture 1 has no home again." + ) + + # 2. Fixtures 2-4 live at the agent seam the contract writes them against + # (work_resolve / work_status / work_claim), i.e. the tool module's + # own suite -- the only suite that can exercise `WorkTrackerSession`. + assert fixtures_2_3_4.exists(), ( + "CCV1-023 (Freeze Bar, CONFORMS): " + "modules/tool-work-tracker/tests/test_conformance_fixtures.py is gone -- " + "Conformance Fixtures 2, 3 and 4 are back to being unimplemented " + "(the GAP work_item_pipeline-qmj closed)." + ) + + # 3. Each fixture contributes real, separately-named tests. Two apiece is + # the floor a good/bad PAIR requires: a fixture reduced to one test has + # stopped discriminating, which is the failure mode this row exists for. + names = function_names(fixtures_2_3_4) + for fixture, subject in ( + ("2", "post-reclaim close fence"), + ("3", "in-process recovery after reclaim"), + ("4", "single-hold constraint"), ): - assert not (REPO_ROOT / named).exists(), ( - f"CCV1-023 (Freeze Bar, GAP) pin: {named} now exists. Re-check which fixtures " - f"are implemented-and-runnable, update the row, and resolve " - f"work_item_pipeline-qmj when all four are." + halves = sorted(n for n in names if n.startswith(f"test_fixture{fixture}_")) + assert len(halves) >= 2, ( + f"CCV1-023: Conformance Fixture {fixture} ({subject}) has {len(halves)} test(s) " + f"in {fixtures_2_3_4.name} -- a fixture is a good/bad PAIR, and one half alone " + f"passes against the broken implementation too. Found: {halves}" ) - # Fixture 4 (single-hold) is asserted by no test anywhere in either suite. - hits = [ - p - for p in (REPO_ROOT / "tests").rglob("test_*.py") - if "single_hold" in read(p) or "already holding" in read(p) - ] - hits += [ - p - for p in (REPO_ROOT / "modules" / "tool-work-tracker" / "tests").rglob("test_*.py") - if "single_hold" in read(p) or "already holding" in read(p) + + # 4. Nothing quietly disabled. An xfail'd or skipped half is a fixture + # that is not "passing" in the Freeze Bar's sense, and the row would + # have to say so; failing here forces that conversation. + tree = ast.parse(read(fixtures_2_3_4)) + disabled = [ + f"{node.name} ({ast.unparse(dec)})" + for node in ast.walk(tree) + if isinstance(node, ast.FunctionDef | ast.AsyncFunctionDef) + for dec in node.decorator_list + if re.search(r"\b(xfail|skip|skipif)\b", ast.unparse(dec)) ] - assert not hits, f"CCV1-023 pin: a single-hold fixture now exists ({hits}) -- update the row" - # The contract still names those non-existent locations (drift the row records). - assert contains( - CONTRACT_PATH, "**Test location:** `tests/test_single_hold.py` (to be added)." - ), "CCV1-023 pin: the contract's Fixture 4 location line changed" + assert not disabled, ( + f"CCV1-023: a Conformance fixture half is disabled: {disabled}. The Freeze Bar " + f"clause is 'implemented, PASSING, and executable via make test' -- flip this row " + f"off CONFORMS and name the disabled half before landing that." + ) + + # 5. The DRIFT this row also records: the contract's own "Test location" + # lines still name files that have never existed. The fixtures are + # real; the pointers at them are not. Editing `contracts/` is an + # amendment, not a lane edit, so the row's notes carry the correction + # and this pin keeps it from being forgotten. + for stale in ( + "**Test location:** `tests/test_incident_b.py` (to be added).", + "**Test location:** `tests/test_reap_recovery.py:67-72` (currently unrun in CI).", + "**Test location:** `tests/test_recovery.py` (to be added).", + "**Test location:** `tests/test_single_hold.py` (to be added).", + ): + assert contains(CONTRACT_PATH, stale), ( + f"CCV1-023: the contract's fixture-location line changed ({stale!r}). If it now " + f"names the real paths, drop the stale-pointer half of this row's notes." + ) diff --git a/ledger/rows.yaml b/ledger/rows.yaml index f1f1f1c..e370bae 100644 --- a/ledger/rows.yaml +++ b/ledger/rows.yaml @@ -665,29 +665,68 @@ single-row lane), left as named follow-up. - id: CCV1-023 - title: Conformance Fixtures 2, 3 and 4 are not implemented-and-runnable + title: Conformance Fixtures 2, 3 and 4 exist as runnable discriminating pairs contract: file: contracts/custody-coordination.v1.md clause: Freeze Bar quote: | All four Conformance fixtures implemented, passing, and executable via `make test` - disposition: GAP - work: work_item_pipeline-qmj + disposition: CONFORMS assertion: - kind: absence + kind: probe ref: test_row_ccv1_023 notes: > - Fixture-by-fixture, against the contract's own named locations. - FIXTURE 1 (conflicted-but-landed close): the contract points at - tests/test_incident_b.py "(to be added)" -- STALE. The fixture exists - and passes at tests/integration/test_phantom_conflict_recovery.py - (measured 2026-09-01); only the pointer is wrong. FIXTURE 2 - (post-reclaim close fence): points at tests/test_reap_recovery.py:67-72 - -- no such path; the real file is in the modules suite, which runs in - nothing, and it asserts the TOOL-layer refusal (which passes via the - session latch) rather than the ADAPTER-layer gap CCV1-009 pins. FIXTURE - 3 (in-process recovery): tests/test_recovery.py does not exist; the - adapter half is covered elsewhere (CCV1-010), the tool-seam half is in - the unrun suite. FIXTURE 4 (single-hold): does not exist in any suite; - `already holding` appears only at its own enforcement site. Blocked on - CCV1-022 for anything that must live in the modules suite. + Closed by work_item_pipeline-qmj. Was GAP: three of the four fixtures + were not implemented-and-runnable. Now, fixture by fixture. FIXTURE 1 + (conflicted-but-landed close) already existed and still does, at + tests/integration/test_phantom_conflict_recovery.py -- untouched here, + only the contract's pointer at it was ever wrong. FIXTURES 2, 3 and 4 + are new, in ONE file at the agent seam the contract writes them + against (work_resolve / work_status / work_claim, i.e. + `WorkTrackerSession`): + modules/tool-work-tracker/tests/test_conformance_fixtures.py -- 10 + tests, each fixture carrying BOTH halves of its good/bad pair, no + xfail, no skip. Fixture 2: the stale holder's work_resolve after a real + `supervisor.reap_project(ttl_seconds=0)` sweep is refused AND the item + is still open afterwards (bad half), while a live holder's resolve and + PR #51's integrator close of the same reclaimed item both still succeed + (good halves). Fixture 3: a session wedged holding an item bd already + considers resolved clears its latch via work_release, gets the + sanctioned already_closed outcome, and claims again -- with the closed + item's ENTIRE record (dataclasses.asdict of a get_readonly, the + contention-free pure SELECT) asserted byte-identical across the + recovery call, which is what proves the Backlogged section's named + hazard ("work_release on a resolved item would reopen it") cannot + happen; plus work_status surfacing holding.custody_lost on the + retained-hold path and holding=None after a fenced reclaim the renew + loop already recovered from. Fixture 4: a second work_claim is refused + naming the held item in BOTH modes (directed and queue-mode), the + would-be second item is asserted untouched, and the constraint lifts + after either exit from a hold (resolve and release, pinned separately + -- they clear `self._held` at different sites). MEASURED 2026-09-02 on + this branch against real bd + the module suite's isolated dolt server: + 10 passed in 70s. DISCRIMINATION MEASURED, not asserted -- each bad + half was run against a deliberately reverted implementation and FAILS + there while every good half stays green: reverting the post-reclaim + fence (adapter.resolve's `elif cust_holder == who and current.holder != + who` branch) fails Fixture 2's bad half only; removing release()'s + pre-write `if current.status == "resolved"` guard fails Fixture 3's + byte-identical assertion; removing claim()'s `if self._held is not + None` gate fails both of Fixture 4's refusal tests. Runs in `make test` + and CI via the modules/tool-work-tracker/tests path CCV1-022 wired + (Makefile `test-module` + the all-tiers `test` target, ci.yml "Tier 5"), + so no new wiring was needed. CONTRACT DRIFT THIS ROW RECORDS AND DOES + NOT FIX: the contract's four "Test location" lines all name paths that + have never existed -- tests/test_incident_b.py, tests/test_reap_recovery.py:67-72, + tests/test_recovery.py, tests/test_single_hold.py. The real locations + are the two files named above. Correcting contracts/ is an amendment, + not a lane edit, so the probe pins the stale lines instead and this + note carries the correction. RESIDUAL, out of this row's scope: the + now-obsolete xfail(strict=True) on + modules/tool-work-tracker/tests/test_reap_recovery.py::test_explicit_resolve_refusal_after_reap_clears_held_and_allows_new_claim + was removed in the same commit -- it pinned CCV1-009's pre-fix gap, the + fence landed, and the strict marker was XPASSing and failing the suite + (exactly as CCV1-022's own notes predicted it would). CCV1-009's and + CCV1-022's notes still describe that xfail as present; neither row was + edited here. + diff --git a/modules/tool-work-tracker/tests/test_conformance_fixtures.py b/modules/tool-work-tracker/tests/test_conformance_fixtures.py new file mode 100644 index 0000000..a0247fc --- /dev/null +++ b/modules/tool-work-tracker/tests/test_conformance_fixtures.py @@ -0,0 +1,465 @@ +"""Conformance Fixtures 2, 3 and 4 of `contracts/custody-coordination.v1.md` +(§Conformance Kit) -- the Freeze Bar's "All four Conformance fixtures +implemented, passing, and executable via `make test`" (ledger row CCV1-023, +work item `pipeline-qmj`). + +Each fixture in the contract is written as a GOOD/BAD pair: a named correct +behaviour and the named incorrect behaviour it must be told apart from. A +test that only exercises the good half discriminates nothing -- it passes +just as happily against the broken implementation. So every fixture below +pins BOTH halves, and says which is which. + +WHY THIS FILE LIVES IN THE MODULE SUITE +--------------------------------------- +All three fixtures are written in the contract against the AGENT SEAM -- +"call `work_resolve(id)`", "call `work_status()`", "claim -> claim again". +That seam is `WorkTrackerSession`, which lives in this module, so this is +the only suite that can exercise the fixtures as the contract states them. +`modules/tool-work-tracker/tests` runs in `make test` and in CI as its own +pytest invocation (ledger row CCV1-022, Makefile target `test-module`, +`.github/workflows/ci.yml`). + +The contract's own "Test location" lines for these three fixtures name +paths that do not exist (`tests/test_reap_recovery.py:67-72`, +`tests/test_recovery.py`, `tests/test_single_hold.py`) -- stale pointers +recorded as drift by ledger row CCV1-023. This file is the real location; +the contract text is not edited from here. + +WHAT IS DELIBERATELY NOT DUPLICATED +----------------------------------- +- Fixture 1 (conflicted-but-landed close) already exists, and passes, at + `tests/integration/test_phantom_conflict_recovery.py`. Not re-stated here. +- Fixture 2's ADAPTER-layer half is pinned in full at + `tests/integration/test_post_reclaim_fence.py` (row CCV1-009). This file + pins the TOOL-layer half the contract actually describes, plus the one + adapter call the tool seam has no verb for (an integrator's close of an + item nobody holds -- PR #51, item `pipeline-79t`). + +Real `bd`/dolt end-to-end against this suite's isolated per-session dolt +server (skipped if `bd` is not on PATH, matching this module's other tests). +""" + +from __future__ import annotations + +import dataclasses +import shutil +import threading +import uuid +from typing import Any + +import pytest +from amplifier_module_tool_work_tracker import WorkTrackerSession + +import amplifier_work_tracker.adapter as A +import amplifier_work_tracker.supervisor as SV + +pytestmark = pytest.mark.skipif( + shutil.which("bd") is None, reason="real `bd` binary not present in this environment" +) + +#: Consumed by the shared `project` fixture in conftest.py, which creates +#: the project AND drops its isolated-server database again on teardown. +PROJECT_PREFIX = "cfixproj" + + +def _unique(prefix: str) -> str: + return f"{prefix}{uuid.uuid4().hex[:10]}" + + +def _bd(session: WorkTrackerSession, project_name: str) -> A.Beads: + """The adapter handle for `project_name`, as the session itself builds + it. Used only where a fixture must reach BELOW the tool seam -- to drive + the real reap sweep, or to act as a different actor than this session.""" + return session._project(project_name) # noqa: SLF001 -- test-only reach + + +def _force_reap(session: WorkTrackerSession, project_name: str) -> dict[str, Any]: + """The REAL sweep, with `ttl_seconds=0` so every current hold is stale + immediately -- no real sleep needed to manufacture staleness, and the + post-reclaim state is produced by the code that produces it in + production rather than by a test's imitation of it.""" + return SV.reap_project(_bd(session, project_name), ttl_seconds=0) + + +def _snapshot(session: WorkTrackerSession, project_name: str, item_id: str) -> dict[str, Any]: + """The item's entire record, read through the contention-free read-only + path (`get_readonly` -> a pure SELECT, which cannot itself mutate or + lose a serialization conflict). Compared field-for-field before and + after a recovery call to prove that call wrote NOTHING.""" + return dataclasses.asdict(_bd(session, project_name).get_readonly(item_id)) + + +class _RunOnceThenStop: + """Fakes `threading.Event`'s `.wait()`/`.is_set()` shape so `_renew_loop` + executes its body exactly once and then exits -- the real method, the + real exception handling, no mocking of `adapter`/`bd` itself, and no + racing a real 120s timer interval.""" + + def __init__(self) -> None: + self._calls = 0 + + def wait(self, timeout: float | None = None) -> bool: # noqa: ARG002 -- Event's signature + self._calls += 1 + return self._calls > 1 + + def set(self) -> None: # noqa: A003 -- matches threading.Event's API + pass + + def is_set(self) -> bool: + return False + + +async def _claim_one(project_name: str, title: str) -> tuple[WorkTrackerSession, str]: + """Add an item as one session and claim it through the tool seam as + another -- the ordinary shape every fixture below starts from.""" + adder = WorkTrackerSession({"actor": _unique("adder")}) + added = await adder.add(project_name, title, acceptance="n/a") + assert added.success is True + item_id: str = added.output["added"] # type: ignore[index] + + session = WorkTrackerSession({"actor": _unique("holder")}) + claimed = await session.claim(project_name, item_id=item_id) + assert claimed.success is True + assert claimed.output["claimed"] == item_id # type: ignore[index] + return session, item_id + + +# =========================================================================== +# Fixture 2 -- Post-reclaim close fence (contract §Conformance, from D-2) +# +# Scenario: item claimed by Session A; the reclaim sweep moves it to open +# and strips custody; Session A, unaware, calls work_resolve(). +# GOOD: the close is refused -- "not held by this session" / reclaim. +# BAD: the close succeeds; the item is closed by a stale holder. +# +# The fence fix has landed on this branch (`fix: fence a post-reclaim close +# on custody identity, not item status`, ledger row CCV1-009 -> CONFORMS), +# so the refusal is asserted DIRECTLY -- no xfail. +# =========================================================================== + + +@pytest.mark.asyncio +async def test_fixture2_bad_half_stale_holders_resolve_is_refused_after_a_real_reap(project): + """BAD half. The discriminator: against the pre-fix implementation this + close LANDED (the fence ran only under `if current.status == "held"`, + and a reaped item is `open`), so the two assertions that fail there are + the refusal itself and the item still being `open` afterwards. + """ + session, item_id = await _claim_one(project, "fixture 2: stale holder's close") + + reaped = _force_reap(session, project) + assert reaped["reclaimed_count"] == 1 + assert reaped["reclaimed"][0]["id"] == item_id + + # The state under test: released, not yet re-claimed, custody record + # still naming the holder that no longer holds anything. + before = _bd(session, project).get_readonly(item_id) + assert before.status == "open" + assert before.holder is None + + refused = await session.resolve(item_id, "closing work reclaimed while I was away") + assert refused.success is False + assert "reclaimed" in str(refused.output).lower() + + # The refusal must not have closed it anyway. + after = _bd(session, project).get_readonly(item_id) + assert after.status == "open" + assert after.resolution is None + + +@pytest.mark.asyncio +async def test_fixture2_good_half_a_live_holders_resolve_still_succeeds(project): + """GOOD half 1. The fence must never refuse the session that genuinely + holds the item -- a fence that refuses everyone passes the BAD half + above for the wrong reason, which is exactly what this pins. + """ + session, item_id = await _claim_one(project, "fixture 2: live holder's close") + + resolved = await session.resolve(item_id, "finished the work I actually hold") + assert resolved.success is True + assert resolved.output["resolved"] == item_id # type: ignore[index] + assert _bd(session, project).get_readonly(item_id).status == "resolved" + + +@pytest.mark.asyncio +async def test_fixture2_good_half_integrator_close_of_a_reclaimed_item_still_succeeds(project): + """GOOD half 2. PR #51 (item `pipeline-79t`): closing out an item nobody + currently holds stays a SINGLE call for anyone who is not the stale + holder -- including after a real reap, which is when unfinished reports + most need closing out. + + Reaches below the tool seam on purpose: `work_resolve` requires holding + the item, so an integrator's close has no tool verb. The discriminator + between the two halves is the custody record's `holder`, not the item's + status, and this is the half that proves the fence was keyed on identity + rather than made universal. + """ + session, item_id = await _claim_one(project, "fixture 2: integrator's close") + assert _force_reap(session, project)["reclaimed_count"] == 1 + + bd = _bd(session, project) + rec = bd.get(item_id).meta.get(A.C.CUSTODY_KEY) + assert isinstance(rec, dict) and rec["holder"] != "integrator" + + back = bd.resolve(item_id, "closed out by the integrator", actor="integrator") + assert back.status == "resolved" + + +# =========================================================================== +# Fixture 3 -- In-process recovery after reclaim (contract §Conformance, +# from Core 8) +# +# Scenario: a session holds an item; custody is lost or the item is +# closed out from under it; the session calls a tool. +# GOOD: the loss is visible, and a recovery path clears the latch +# WITHOUT manual intervention or a restart. +# BAD: the session is left ambiguous -- it believes it holds the +# item, every tool refuses, and there is no recovery path. +# +# The contract's Backlogged §"Recovery verb" names the specific hazard the +# recovery path must not have: "`work_release` on a resolved item would +# reopen it". `adapter.Beads.release` checks status BEFORE any write and +# performs no write at all when the item is already resolved, so the +# byte-identical assertion below is the fixture's real discriminator. +# =========================================================================== + + +@pytest.mark.asyncio +async def test_fixture3_release_of_an_already_closed_held_item_clears_the_latch(project): + """GOOD half, and the fixture's core case: a wedged session -- one whose + close already LANDED while it still believes it holds the item -- clears + its own latch in-process via `work_release`, gets the sanctioned + `already_closed` outcome, and can claim again immediately. + + The wedge is produced the way it really arises: the close lands at the + adapter (as this very holder, so no fence fires) without the tool seam + ever learning about it -- the phantom-conflict shape PR #63 fixed the + reporting half of. `session._held` therefore still names the item. + """ + session, item_id = await _claim_one(project, "fixture 3: wedged session") + + held = session._held # noqa: SLF001 + assert held is not None + _bd(session, project).resolve( + item_id, "close landed; the wrapper never saw it", actor=held.actor + ) + + # Wedged exactly as the incident describes: bd says resolved, the + # session still believes it holds the item. + assert session._held is not None # noqa: SLF001 + before = _snapshot(session, project, item_id) + assert before["status"] == "resolved" + + recovered = await session.unclaim(item_id) + assert recovered.success is True + out: dict[str, Any] = recovered.output # type: ignore[assignment] + assert out["released"] == item_id + assert "already closed" in out["custody"] + + # BAD half: the recovery call must not have REOPENED (or otherwise + # touched) the closed item. Not "still resolved" -- byte-identical, so a + # write that happened to land on the same status is caught too. + assert _snapshot(session, project, item_id) == before + + # Latch cleared in-process: no restart, no manual intervention. + assert session._held is None # noqa: SLF001 + assert held.stop.is_set() + + adder = WorkTrackerSession({"actor": _unique("adder")}) + nxt = await adder.add(project, "fixture 3: work after recovery", acceptance="n/a") + again = await session.claim(project) + assert again.success is True + assert again.output["claimed"] == nxt.output["added"] # type: ignore[index] + await session.resolve(nxt.output["added"], "test cleanup") # type: ignore[index] + + +@pytest.mark.asyncio +async def test_fixture3_work_status_reports_custody_lost_while_the_hold_is_retained(project): + """GOOD half: the passive signal the contract's Core 4 names -- + `holding.custody_lost` -- is actually visible at the tool seam, so a + session can DISCOVER a renewal failure without any tool refusing it + first. + + Driven through the real `_renew_loop` (one deterministic pass) against a + renewal that fails without fencing: bd still considers this session the + holder, so the hold is deliberately RETAINED and the loss is reported + rather than acted on. This is the state in which `custody_lost` is + non-null; see the fenced counterpart below for the other outcome. + """ + session, item_id = await _claim_one(project, "fixture 3: custody_lost signal") + held = session._held # noqa: SLF001 + assert held is not None + held.stop.set() # stop the real background thread; drive the loop by hand + + def _boom(*args: Any, **kwargs: Any) -> Any: + raise A.BeadsError("simulated transient renew_custody failure") + + # `_renew_loop` builds its own `Beads` instance internally, so the patch + # must be at the class level to reach it. + original = A.Beads.renew_custody + A.Beads.renew_custody = _boom # type: ignore[method-assign, assignment] + try: + held.stop = _RunOnceThenStop() # type: ignore[assignment] + session._renew_loop(held) # noqa: SLF001 + finally: + A.Beads.renew_custody = original # type: ignore[method-assign] + + reported = await session.status() + holding = reported.output["holding"] # type: ignore[index] + assert holding is not None, ( + "BAD half: the hold vanished from work_status, so a session that lost " + "renewal has no way to see WHICH item it is still holding" + ) + assert holding["id"] == item_id + assert holding["custody_lost"] == "simulated transient renew_custody failure" + + held.stop = threading.Event() # restore a real Event for cleanup + assert (await session.resolve(item_id, "test cleanup")).success is True + + +@pytest.mark.asyncio +async def test_fixture3_a_fenced_reclaim_clears_the_latch_with_no_manual_step(project): + """GOOD half, the other outcome: when the renewal failure IS a fence -- + the reclaim sweep genuinely took the item away -- the recovery happens + without the session having to ask for it. The latch is dropped by the + loop itself, so `work_status` honestly reports no hold and the next + `work_claim` succeeds. + + BAD half (the self-poisoning bug this pins): leaving `self._held` set + here refused `work_claim`/`work_declare`/`work_resolve` for ANY item for + the rest of the process's life, with no tool call able to clear it -- + precisely the "no recovery path" the contract's Fixture 3 names. + """ + session, item_id = await _claim_one(project, "fixture 3: fenced reclaim") + held = session._held # noqa: SLF001 + assert held is not None + held.stop.set() # stop the real background thread; drive the loop by hand + + assert _force_reap(session, project)["reclaimed_count"] == 1 + + held.stop = _RunOnceThenStop() # type: ignore[assignment] + session._renew_loop(held) # noqa: SLF001 + + assert held.lost_reason is not None + assert "reclaimed" in held.lost_reason.lower() or "reassigned" in held.lost_reason.lower() + assert session._held is None # noqa: SLF001 + + reported = await session.status() + assert reported.output["holding"] is None # type: ignore[index] + + # The item itself was returned to the queue by the sweep, not closed. + assert _bd(session, project).get_readonly(item_id).status == "open" + + again = await session.claim(project, item_id=item_id) + assert again.success is True + await session.resolve(item_id, "test cleanup") + + +# =========================================================================== +# Fixture 4 -- Single-hold constraint (contract §Conformance, from Core 12) +# +# Scenario: a session holds item A and attempts to claim item B without +# releasing A. +# GOOD: the claim is refused -- "already holding item A". +# BAD: the claim succeeds and the session now holds two items. +# +# Both claim modes are pinned: a directed claim is not a lesser claim +# (Core 1), so it must be refused on exactly the same terms as a queue claim. +# =========================================================================== + + +@pytest.mark.asyncio +async def test_fixture4_a_directed_second_claim_is_refused_while_holding(project): + """BAD half's discriminator, directed mode. Two facts must hold: the + call is refused naming the item already held, AND item B is genuinely + untouched -- an implementation that refuses AFTER claiming B would pass + a refusal-only assertion while holding two items. + """ + session, a_id = await _claim_one(project, "fixture 4: item A (directed)") + + adder = WorkTrackerSession({"actor": _unique("adder")}) + b = await adder.add(project, "fixture 4: item B (directed)", acceptance="n/a") + b_id: str = b.output["added"] # type: ignore[index] + + refused = await session.claim(project, item_id=b_id) + assert refused.success is False + assert "already holding" in str(refused.output) + assert a_id in str(refused.output), "the refusal must name WHICH item is held" + + # Still exactly one hold, and B was never claimed. + assert session._held is not None # noqa: SLF001 + assert session._held.item_id == a_id # noqa: SLF001 + b_after = _bd(session, project).get_readonly(b_id) + assert b_after.status == "open" + assert b_after.holder is None + + await session.resolve(a_id, "test cleanup") + + +@pytest.mark.asyncio +async def test_fixture4_a_queue_second_claim_is_refused_while_holding(project): + """Same refusal, queue mode -- the default `work_claim(project)` path + with no `item_id`. Pinned separately because the two modes take + different branches inside `claim()` after the shared single-hold gate, + and a regression could easily reinstate one without the other. + """ + session, a_id = await _claim_one(project, "fixture 4: item A (queue)") + + adder = WorkTrackerSession({"actor": _unique("adder")}) + b = await adder.add(project, "fixture 4: item B (queue)", acceptance="n/a") + b_id: str = b.output["added"] # type: ignore[index] + + refused = await session.claim(project) + assert refused.success is False + assert "already holding" in str(refused.output) + + assert session._held is not None # noqa: SLF001 + assert session._held.item_id == a_id # noqa: SLF001 + assert _bd(session, project).get_readonly(b_id).holder is None + + await session.resolve(a_id, "test cleanup") + + +@pytest.mark.asyncio +async def test_fixture4_the_second_claim_succeeds_once_the_first_is_resolved(project): + """GOOD half. The constraint is one hold AT A TIME, not one hold per + session for all time -- a refusal that never lifts would satisfy every + assertion above while making the session useless after one item. + """ + session, a_id = await _claim_one(project, "fixture 4: resolve then claim (A)") + + adder = WorkTrackerSession({"actor": _unique("adder")}) + b = await adder.add(project, "fixture 4: resolve then claim (B)", acceptance="n/a") + b_id: str = b.output["added"] # type: ignore[index] + + assert (await session.resolve(a_id, "done with A")).success is True + second = await session.claim(project, item_id=b_id) + assert second.success is True + assert second.output["claimed"] == b_id # type: ignore[index] + + await session.resolve(b_id, "test cleanup") + + +@pytest.mark.asyncio +async def test_fixture4_the_second_claim_succeeds_once_the_first_is_released(project): + """GOOD half, via the other exit from a hold: `work_release` sets no + resolution, so the constraint must lift there too. Pinned separately + because `resolve` and `unclaim` clear `self._held` at different sites. + """ + session, a_id = await _claim_one(project, "fixture 4: release then claim (A)") + + adder = WorkTrackerSession({"actor": _unique("adder")}) + b = await adder.add(project, "fixture 4: release then claim (B)", acceptance="n/a") + b_id: str = b.output["added"] # type: ignore[index] + + assert (await session.unclaim(a_id)).success is True + second = await session.claim(project, item_id=b_id) + assert second.success is True + assert second.output["claimed"] == b_id # type: ignore[index] + + # A really did go back to the queue, with no resolution set. + a_after = _bd(session, project).get_readonly(a_id) + assert a_after.status == "open" + assert a_after.resolution is None + + await session.resolve(b_id, "test cleanup") diff --git a/modules/tool-work-tracker/tests/test_reap_recovery.py b/modules/tool-work-tracker/tests/test_reap_recovery.py index 40e342d..ad8d7d7 100644 --- a/modules/tool-work-tracker/tests/test_reap_recovery.py +++ b/modules/tool-work-tracker/tests/test_reap_recovery.py @@ -50,21 +50,22 @@ def _force_reap(session: WorkTrackerSession, project_name: str) -> dict[str, Any return SV.reap_project(bd, ttl_seconds=0) -@pytest.mark.xfail( - strict=True, - reason=( - "CCV1-009 (work_item_pipeline-dn4): a post-reclaim close is not fenced -- " - "`Beads.resolve`'s fence block runs only under `if current.status == 'held'`, " - "and a reaped item is `open`, so the stale holder's resolve lands instead of " - "being refused. PRODUCT defect, first mechanically measured by this test once " - "the suite was actually wired into `make test`/CI (CCV1-022); not fixed here." - ), -) @pytest.mark.asyncio async def test_explicit_resolve_refusal_after_reap_clears_held_and_allows_new_claim(project): """Trigger path 1: the explicit `work_resolve` refusal. Claim -> force reap -> resolve refuses (fenced) -> but the SAME session's next - `work_claim` must still succeed, in the same process.""" + `work_claim` must still succeed, in the same process. + + Was `xfail(strict=True)` for ledger row CCV1-009 (`work_item_pipeline-dn4`) + while `Beads.resolve`'s fence ran only under `if current.status == "held"` + and a reaped item is `open`, so the stale holder's close landed. That + PRODUCT defect is fixed -- the fence is now keyed on custody IDENTITY, + not item status, and CCV1-009 reads CONFORMS -- so the marker was + XPASSing (strict) and failing this suite. Removed rather than re-aimed: + the behaviour it described no longer exists. The same refusal is pinned + as a discriminating good/bad pair in `test_conformance_fixtures.py` + (contract Fixture 2) and at the adapter layer in + `tests/integration/test_post_reclaim_fence.py`.""" add_session = WorkTrackerSession({"actor": _unique("adder")}) first = await add_session.add(project, "first reap-recovery item", acceptance="n/a") assert first.success is True