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/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..45921dc 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,13 +37,43 @@ MAKEFILE, REPO_ROOT, TOOL_MODULE, + collapse, contains, count, + function_names, read, row, 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" + + +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 @@ -70,23 +101,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." ) @@ -115,49 +188,100 @@ 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 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 +289,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 @@ -200,23 +346,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 +378,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." ) @@ -275,16 +491,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 @@ -325,52 +577,141 @@ 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 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 ac2805f..e370bae 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 @@ -127,25 +149,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 +234,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 @@ -238,24 +270,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 @@ -313,49 +356,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,50 +447,72 @@ 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 + 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 @@ -547,53 +627,106 @@ 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 + 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/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_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_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" diff --git a/modules/tool-work-tracker/tests/test_reap_recovery.py b/modules/tool-work-tracker/tests/test_reap_recovery.py index 3d56783..ad8d7d7 100644 --- a/modules/tool-work-tracker/tests/test_reap_recovery.py +++ b/modules/tool-work-tracker/tests/test_reap_recovery.py @@ -54,7 +54,18 @@ def _force_reap(session: WorkTrackerSession, project_name: str) -> dict[str, Any 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 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/adapter.py b/src/amplifier_work_tracker/adapter.py index a922d83..f7f9521 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 @@ -3093,43 +3339,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 +3411,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: @@ -3197,25 +3479,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 +3538,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 +3571,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 +3670,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 +3717,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 +3745,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 +3770,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 +3804,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/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 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" 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"