feat: `reopen` verb + `work_reopen` tool; `resolve` on a closed item fails loud by bkrabach · Pull Request #67 · microsoft/amplifier-work-tracker

Brian Krabach (bkrabach) · 2026-09-03T03:54:20Z

The defect

A published resolution could not be corrected. Worse, trying to correct one succeeded silently: resolve against an already-closed item checked only the item's status, exited 0, and echoed the old stored text back as if the new text had landed. Seven model_performance resolutions shipped that way (model_performance-uma; spec w3-uma-work-tracker-reopen/SPEC.md).

Measured at the parent commit (1621771), with this change absent — docs/lanes/f5c-work-tracker-reopen/evidence/before-defect-demonstration.log:

--- BEFORE STATE (parent commit, fix absent) ---
  sent to resolve():   'CORRECTED: uncomputable -- arm B has zero valid runs'
  returned as landed:  'VERDICT: the retention gate PASSES'
  stored on the item:  'VERDICT: the retention gate PASSES'
  raised:              nothing -- exit 0, reported success
--- the correction was discarded and the caller was told it landed ---

What changed

resolve compares the resolution TEXT, at BOTH places it decides "did my write land?" — the normal post-write readback and the contended path where _run exhausted its serialization-retry budget. Patching only the first leaves the second silently wrong, and the contended path is exactly where a caller is least able to reason about what happened.

On a target that is already resolved, decided before any write so "NOTHING WAS WRITTEN" is literally true:

stored vs sent	behaviour
identical (CRLF + outer whitespace normalized)	success, `"idempotent": true`, no write attempted
differs	`BeadsError`, non-zero, nothing written, both texts side by side, `reopen` named as a runnable remedy

The identical-text carve-out is required, not a softening: the shipped contention contract sells resolve's no-op as retry safety, and a retry re-sends the identical string. A blanket error would fail a legitimate retry of a write that did land.

Beads.reopen + CLI reopen + work_reopen is that remedy. It archives the verbatim previous resolution and previous closed_at into the item's attributed comment history before transitioning, refuses anything that is not resolved (deliberately not idempotent), and refuses an empty reason.

Six new doctor assumptions fence all of it — doctor now reports 33/33 against the live bd: reopen.reopens, reopen.clears_closed_at, reopen.close_reason_disposition, reopen.emits_event, resolve.divergent_text_refused, resolve.identical_text_idempotent.

Two undocumented bd behaviours, measured here

1. bd reopen CLEARS close_reason. The spec flagged this as unknown and told the implementer not to build a guarantee on it. Measured: the previous resolution text is gone from the issue row. That makes the archive-first ordering load-bearing — had the wrapper trusted bd to keep it, every correction would have destroyed the record it was correcting. Pinned in a test and in reopen.close_reason_disposition so a future bd change is loud.

2. bd reopen LEAVES THE OLD ASSIGNEE. Not in the spec; found because this lane's own tool test failed on its claim leg:

claim <id> as 'corrector' failed: Error claiming <id>: issue already claimed by <first-pass actor>

An item nobody can take is not a correctable item. reopen now clears the assignee (the same update --assignee "" release already does) and verifies by readback. reopen.reopens performs a directed claim by a different actor — the weaker claim_next form passed even with the stale assignee in place.

Bonus: the reopened audit trail is two event rows, not one — the reopened row carries event_type + actor (its comment column is empty), and --reason lands as an adjacent attributed commented row. Both are asserted.

The open question, answered

whether one actor may hold two items at once

A session holds at most one item — enforced at the tool seam (_Held, and claim's "already holding" refusal), not by bd. So work_reopen(claim=True) from a session that already holds work degrades exactly as the spec required: claimed: false + claim_error, the existing hold untouched, the reopen still standing. Implemented by reusing session.claim() wholesale rather than re-implementing the claim leg — a second implementation of that path is how a double-claim hole gets reopened.

The known cost, surfaced not hidden

reopen clears closed_at, so a corrected item re-lands on the correction date and every throughput roll-up moves by one item per correction. The payload says so (closed_at_cleared, previous_closed_at). That cost is precisely why the verb stays explicit instead of being folded into resolve as an invisible side effect of a call the author believes is idempotent.

Tests

Tier	Needed live bd/dolt?	Result
`make test-unit`	no	green — 23 new
`make test-integration`	yes, had it	green — 8 new + 6 new contract assumptions
`make test-cli`	yes, had it	green — 5 new
`make test-ledger`	no	green — 24, incl. 3 reconciled rows
`make test` (all four)	yes	1189 passed, 3 skipped, 1 failed — pre-existing, below
`modules/tool-work-tracker/tests`	yes, had it	93 passed, 1 failed — pre-existing, below (all 7 new green)
`make check` (ruff + pyright)	no	clean
`doctor --quick`	yes	33/33

Fail-before evidence (raw output committed under `docs/lanes/f5c-work-tracker-reopen/evidence/`)

The new tests were run against parent commit 1621771 in a clean worktree with this change absent:

fail-before-unit.log — 22 failed, 1 passed. (The one pass is test_resolve_returns_an_item_for_every_existing_caller — correctly, since resolve() returning an Item is pre-existing behaviour this change preserves.)
fail-before-integration.log — 8 failed
fail-before-cli.log — 5 failed
before-defect-demonstration.log — the silent-discard reproduction quoted at the top

The two pre-existing failures, both reproduced at the parent commit

tests/cli/test_cli_surface.py::test_doctor_quick_succeeds_against_the_real_installed_bd — the suite repoints AMPLIFIER_WORK_TRACKER_ROOT at an isolated tmp root with no sweep heartbeat, so sweeps.alive fails and doctor exits 1. Every other assumption in that run passes, including all six added here. Evidence: parent-commit-doctor-cli-preexisting-failure.log. Filed as model_performance-jyg.
modules/.../test_reap_recovery.py::test_explicit_resolve_refusal_after_reap_... — a post-reclaim work_resolve is not refused. This is ledger row CCV1-009 (VIOLATION, work_item_pipeline-dn4) observed behaviourally. Evidence: parent-commit-modules-reap-preexisting-failure.log. Filed as model_performance-c0e.

Ledger reconciliation

Three probes moved; no disposition changed, and contracts/ + docs/VISION.md are untouched so CCV1-000's SYNC hashes still hold. CCV1-009 (probe re-pointed to follow the code, claim re-verified as still-VIOLATION), CCV1-015 (count 3 → 4: reopen adopted verify-on-conflict — movement toward the clause, recorded not absorbed), CCV1-023 (single-hold now asserted incidentally in the unrun modules suite; still GAP against "executable via make test"). Reasons recorded in each row's notes.

Deliberately not in this change

defer()/block() refusing a resolved item (model_performance-2nx) — a separate destructive path; still a real hole.
Per-item reopen_count on read surfaces (spec §2.8's "additionally").
Tightening resolve's fence to require custody — explicitly out of scope in the spec.

Full lane note, including deviations and spend ($0): docs/lanes/f5c-work-tracker-reopen/DONE-NOTE.md.

Unblocks model_performance-69y (the 7-item correction backlog).

…fails loud A published resolution could not be corrected. Worse, trying to correct one SUCCEEDED silently: `resolve` against an already-closed item checked only the item's STATUS, exited 0, and echoed the OLD stored text back as if the new text had landed. Seven wrong resolutions shipped that way (model_performance-uma; spec `w3-uma-work-tracker-reopen/SPEC.md`). resolve now compares the resolution TEXT at BOTH places it decides "did my write land?" -- the normal post-write readback AND the contended path where `_run` exhausted its retry budget. On an already-resolved target, checked BEFORE any write so "NOTHING WAS WRITTEN" is literally true: - identical text (CRLF/outer whitespace normalized) -> idempotent success, payload carries `"idempotent": true`. Required, not a softening: the shipped contention contract sells resolve's no-op as retry safety, and a retry re-sends the identical string. - divergent text -> BeadsError, nothing written, both texts side by side, the words NOTHING WAS WRITTEN, and `reopen` as a runnable remedy. `Beads.reopen` (+ CLI `reopen`, + `work_reopen`) is that remedy. It archives the verbatim previous resolution and previous closed_at into the item's attributed comment history BEFORE transitioning -- ordering that is load-bearing, and now proven so: MEASURED against bd 1.1.2, a reopen CLEARS close_reason. Had the wrapper trusted bd to keep it, every correction would have destroyed the record it was correcting. Also measured and corrected here: `bd reopen` leaves the OLD ASSIGNEE in place, so a directed claim by anyone else is refused ("already claimed by <old holder>") -- an item nobody can take is not a correctable item. reopen clears it, as `release` already does. Six new doctor assumptions fence all of it (33/33): reopen.reopens, reopen.clears_closed_at, reopen.close_reason_disposition, reopen.emits_event, resolve.divergent_text_refused, resolve.identical_text_idempotent. The known cost is surfaced, not hidden: reopen clears closed_at, so a corrected item re-lands on the correction date and throughput moves by one item per correction (`closed_at_cleared`, `previous_closed_at`). That is why the verb stays explicit rather than folded into resolve.

…evidence None absorbed silently, none re-dispositioned (contracts/ and VISION.md untouched, so CCV1-000's SYNC hashes still hold): - CCV1-009 (VIOLATION) -- the close path grew a pre-write read of its own and the fence moved into `resolve_outcome`. The probe's landmarks follow it and are now robust to formatting. Claim re-verified and UNCHANGED: the fence still runs only under `status == "held"`, so a post-reclaim close is still unfenced. - CCV1-015 (GAP) -- `_read_back_or_none` count 3 -> 4: `reopen` adopted verify-on-conflict. Real movement TOWARD the clause, recorded rather than absorbed. Still a GAP: take_custody/renew_custody remain uncovered. - CCV1-023 (GAP) -- the single-hold refusal is now asserted INCIDENTALLY in the modules suite (one branch of work_reopen's claim-degradation path). That suite is the one `make test` does not run, so the Freeze Bar clause is still unmet and the row stays GAP; the probe pins that exact file set so a real Fixture 4 still flips it. Lane artifacts under docs/lanes/f5c-work-tracker-reopen/ (artifact-path/v1): the DONE-NOTE, and raw evidence including the fail-before runs at parent commit 1621771 and the before-state demonstration of the silent discard.

Conflict was AGENTS.md's doctor-assumption count only (cli.py auto-merged; no source conflict). #67 said 33/33, #66's main said 28/28. The merged count is MEASURED, not computed: `doctor` on this merged tree reports **All 34 assumptions hold**. Arithmetic (33 + 2) would have written 35 and been wrong, which is why the number was read off the tool rather than added up. Manager merge-mechanics only -- no source file changed.

Brian Krabach (bkrabach) · 2026-09-03T05:05:30Z

Manager verification (independent, scratch clone) — and one manager-authored merge commit

Verified in a fresh clone of microsoft/amplifier-work-tracker. The lane's head
f9f83fed3ca78e1beada5d9b81cf53f90d75f72c was CONFLICTING against main —
origin/main had moved from the lane's parent 1621771 to cbf4f47 (#66) — so
this was verified twice: once at the lane head, then again after a merge.

Classification: FIX (with a required remedy path), not a bare feature

The feat: title undersells it. Part 4 — resolve comparing the resolution
text, not just the status, at both readback points — is a data-integrity
bug fix: today a correction sent to a closed item returns exit 0 and writes
nothing, and the caller is handed back the old text as if it had landed.
Parts 1–3 (reopen on both surfaces) are new surface, but they are the remedy
the refusal message names; shipping the refusal without them would point
callers at a command that does not exist. Merging as a correctness fix on that
basis, with the fail-before below.

Fail-before, reproduced

tests/unit/test_reopen_preconditions.py + test_resolve_text_compare.py copied
into a parent-1621771 worktree with the parent's own src/ pinned on
PYTHONPATH (without that pin the editable install silently resolves the PR's
source and everything passes — worth knowing):

22 failed, 1 passed          # parent 1621771, #67 source absent

matching the lane's claim, and including
test_divergent_text_on_a_closed_item_refuses_and_writes_nothing — the defect
itself. All 23 pass on the PR head.

All four documented tiers, measured after the merge

tier	result
unit (`tests/unit`)	790 passed
integration (`-m integration`)	321 passed, 3 skipped (14:56)
cli (`-m cli`)	80 passed, 1 failed ← pre-existing
ledger (`ledger/checks`)	24 passed
modules (`modules/tool-work-tracker/tests`)	100 passed, 1 failed ← pre-existing
`ruff check` / `ruff format --check`	clean, 143 files

Both failures independently confirmed pre-existing at parent 1621771 with this
change absent (test_doctor_quick_succeeds_against_the_real_installed_bd;
test_reap_recovery::test_explicit_resolve_refusal_after_reap_...), already
filed as model_performance-jyg / model_performance-c0e.

One failure the lane did not report appeared in an early run —
test_supervisor_web.py::test_web_server_loop_serves_https_.... It is a
port-binding flake, not a regression: the same file passes 14/14 on rerun, a
different test in it fails at the parent commit, and #67 does not touch
supervisor.py. Recording it because it will be seen again.

The merge commit (`8cede04`) — measured, not computed

The only conflict was AGENTS.md's doctor-assumption count (cli.py
auto-merged; no source conflict). #67 claimed 33/33, #66's main claimed
28/28. I did not add them up — I ran doctor on the merged tree:

All 34 assumptions hold. Safe to run parallel agents.

34, not the 35 that 33 + 2 would have written. AGENTS.md now says 34/34
with that reasoning inline. No source file was touched by this commit.

Merging with --squash --admin: plain squash is refused by this repo's
base-branch review policy, same as microsoft/amplifier-foundation#350 and
amplifier-bundle-routing-matrix#49 this batch.

Still open, deliberately out of scope here: defer()/block() still perform
an unaudited destructive reopen of a resolved item at exit 0
(model_performance-2nx).

- adapter.py Beads.resolve_outcome: combined the fence lane's custody- IDENTITY fence (three branches, including the post-reclaim one outside the `status == "held"` gate) with PR #67's already-resolved rule (text-identical idempotent no-op / divergent-text fail-loud) -- ONE pre-write read now serves both preconditions, fence first, then the already-resolved check, both fully before any write; the existing contended-path read-back (PR #63) is unchanged and still wins for a phantom-conflict recovery whose own text already landed. - adapter.py Beads.reopen: routed through `_verified_write` (two calls -- the `bd reopen` write itself, and the conditional stale-assignee clear), joining every other item-level write verb (CCV1-015); the "reported success but still resolved" phantom case keeps its own precise wording by being detected in the write step rather than the generic verify failure, so `tests/unit/test_reopen_preconditions.py` needed no changes. - ledger/rows.yaml + ledger/checks/test_custody_rows.py: kept every wave-1 row flip and probe rewrite (CCV1-009 CONFORMS via the identity fence, CCV1-015 CONFORMS via `_verified_write`), folded in PR #67's reopen coverage (CCV1-015's verb list + `_read_back_or_none` call-site count, now 5: definition + resolve + release + reopen's two verifies), and re-pointed CCV1-009's probe at `resolve_outcome` (the fence's new home) and its pre-write boundary at the `close` write (since the merged method's own pre-write read also opens with a `try:`). - context/awareness.md / cli.py epilog: kept the fence lane's corrected custody prose (renewal is one-strike, TTL is sweep-enforced) and PR #67's reopen documentation (item 7 / the resolve-on-closed text-compare rule) side by side, with no restatement of the read-back guarantee that now appears earlier in both documents. - modules/tool-work-tracker/__init__.py: auto-merged cleanly -- `_release_after_failed_custody` (custody-atomic) and `WorkReopenTool` (PR #67) coexist; `resolve` already called `resolve_outcome`. - Added test_reopen_verifies_by_readback_when_the_wrapper_reports_conflict and its genuine-failure negative to tests/integration/test_write_readback.py in house style, per CCV1-015's new reopen coverage.

…esolution MEASURED (bd 1.1.2, 2026-09-03, throwaway project via the sanctioned CLI): `defer` or `block` on an already-resolved item exited 0, moved it out of `resolved`, and BLANKED its stored `resolution` -- destroying the official, already-published record with no warning, no confirmation, no archive and no trace of what it used to say. The remaining verbs (`--clear` -> `claim` -> `resolve`) then rewrote that record end to end using nothing but sanctioned calls, which is why two prior lanes' "a closed resolution is unwritable through every sanctioned path" claim was false. This is strictly worse than the defect `resolve`'s divergent-text refusal closes: that one discarded the text you SEND; this discards the text already STORED. And it sat one verb away from `release()`, whose docstring goes to deliberate lengths to make exactly this transition "structurally impossible from this path". The guard lives in `_set_status_with_reason` -- the single shared implementation both verbs go through, so there is one guard that cannot drift between them -- and is checked BEFORE any write, which is what makes the refusal's own "NOTHING WAS WRITTEN" literally true. It echoes the text at risk and points at `reopen` (shipped by #67), the SAFE door to the same place: it archives the previous resolution and closed_at into an attributed comment first. The unsafe door closes; the safe one stays open. - adapter.py: `_status_change_on_resolved_error`, `_STATUS_CHANGE_VERB`, the pre-write guard, docstrings on both verbs. - contract.py: `defer.refuses_resolved` / `block.refuses_resolved`, asserted separately per verb on purpose -- a future change that gives `block` its own path cannot leave one door open while the other check keeps passing. Each asserts all four properties, including the one that actually protects a record: the stored resolution is unchanged byte for byte. - tests: integration (refusal x2, the whole loop, `reopen` still archives, and 6 "non-resolved items unaffected"), cli (exit code + record intact), modules (`success=False` on the agent-facing surface). - AGENTS.md: doctor count 34 -> 36, MEASURED from `doctor`, not computed. Refs: model_performance-2nx

…esolution (#72) * fix: `defer`/`block` refuse a RESOLVED item instead of blanking its resolution MEASURED (bd 1.1.2, 2026-09-03, throwaway project via the sanctioned CLI): `defer` or `block` on an already-resolved item exited 0, moved it out of `resolved`, and BLANKED its stored `resolution` -- destroying the official, already-published record with no warning, no confirmation, no archive and no trace of what it used to say. The remaining verbs (`--clear` -> `claim` -> `resolve`) then rewrote that record end to end using nothing but sanctioned calls, which is why two prior lanes' "a closed resolution is unwritable through every sanctioned path" claim was false. This is strictly worse than the defect `resolve`'s divergent-text refusal closes: that one discarded the text you SEND; this discards the text already STORED. And it sat one verb away from `release()`, whose docstring goes to deliberate lengths to make exactly this transition "structurally impossible from this path". The guard lives in `_set_status_with_reason` -- the single shared implementation both verbs go through, so there is one guard that cannot drift between them -- and is checked BEFORE any write, which is what makes the refusal's own "NOTHING WAS WRITTEN" literally true. It echoes the text at risk and points at `reopen` (shipped by #67), the SAFE door to the same place: it archives the previous resolution and closed_at into an attributed comment first. The unsafe door closes; the safe one stays open. - adapter.py: `_status_change_on_resolved_error`, `_STATUS_CHANGE_VERB`, the pre-write guard, docstrings on both verbs. - contract.py: `defer.refuses_resolved` / `block.refuses_resolved`, asserted separately per verb on purpose -- a future change that gives `block` its own path cannot leave one door open while the other check keeps passing. Each asserts all four properties, including the one that actually protects a record: the stored resolution is unchanged byte for byte. - tests: integration (refusal x2, the whole loop, `reopen` still archives, and 6 "non-resolved items unaffected"), cli (exit code + record intact), modules (`success=False` on the agent-facing surface). - AGENTS.md: doctor count 34 -> 36, MEASURED from `doctor`, not computed. Refs: model_performance-2nx * fix(adapter): defer/block REFUSE a resolved item instead of silently blanking its resolution model_performance-2nx (CRITICAL). Measured on a throwaway project: resolve an item with 'ORIGINAL TEXT', then `defer` -> exit 0, `block` -> exit 0, and the stored resolution is GONE. The full loop block -> clear -> claim -> resolve then completes with no bd involvement at all, rewriting a closed record's official text with no archive, no warning and no trace of what it used to say. Two consequences the measurement settles: (A) The premise of model_performance-uma and -44f was WRONG. Both assert a closed item's resolution is unwritable through every sanctioned path. It was writable, through work-tracker verbs alone. 44f's own summary table lists work_defer/work_block as 'status/location only -- no' against `resolution`; that row was wrong on both counts. (B) It is WORSE than the defect uma was filed for. uma's discards the text you SEND; this discards the text already STORED -- the official record -- at exit 0. adapter.release() goes to deliberate lengths to make reopening a closed item 'structurally impossible from this path' while defer/block did exactly that, destructively, one verb away. THE FIX: defer() and block() refuse when status is resolved, writing nothing, naming the item and its status, and pointing at `reopen` (shipped by f5c, 2468a69) as the sanctioned archiving path. Mirrors release()'s existing refusal discipline. Two doctor assumptions -- defer.refuses_resolved, block.refuses_resolved -- so it cannot regress silently. TESTS, all four tiers: unit 789 passed; integration+cli 31 passed (the new test_defer_block.py + test_cli_new_verbs.py); modules 9 passed; ledger 24 passed; ruff check + format clean. tests/unit/test_supervisor_web.py is the known PORT-BINDING FLAKE, not a regression here: it fails intermittently (fail/pass/fail across three runs of the same file on this tree), its failing test's identity varies, and this branch does not touch supervisor.py at all. RECOVERED BY THE MANAGER: the lane died markerless at ~00:40 mid 'commit and push'. The work was complete and uncommitted in its worktree; this commit is that work, unchanged, with the tiers re-run to confirm before publishing. --------- Co-authored-by: agent-2nx-lane <agent@localhost> Co-authored-by: Amplifier <240397093+microsoft-amplifier@users.noreply.github.com>

Amplifier (microsoft-amplifier) added 3 commits September 2, 2026 20:00

Brian Krabach (bkrabach) marked this pull request as ready for review September 3, 2026 05:05

Brian Krabach (bkrabach) merged commit 2468a69 into main Sep 3, 2026
2 checks passed

Brian Krabach (bkrabach) mentioned this pull request Sep 3, 2026

highway lane fixtures: Conformance Fixtures 2/3/4 as runnable discriminating tests (qmj, CCV1-023) #69

Closed

This was referenced Sep 3, 2026

fix: pin the dolt scan directory; stop reporting infrastructure failure as "item not found" #70

Merged

fix: defer/block refuse a RESOLVED item instead of blanking its resolution #72

Merged

Brian Krabach (bkrabach) mentioned this pull request Sep 3, 2026

test(_util): match error ANNOUNCEMENTS, not the word "error" (unblocks #70) #74

Merged

Navigation Menu

Uh oh!

feat: `reopen` verb + `work_reopen` tool; `resolve` on a closed item fails loud - #67

feat: `reopen` verb + `work_reopen` tool; `resolve` on a closed item fails loud#67
Brian Krabach (bkrabach) merged 3 commits into
mainfrom
lane/f5c-work-tracker-reopen

Brian Krabach (bkrabach) commented Sep 3, 2026

Uh oh!

Brian Krabach (bkrabach) commented Sep 3, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

Brian Krabach (bkrabach) commented Sep 3, 2026

The defect

What changed

Two undocumented bd behaviours, measured here

The open question, answered

The known cost, surfaced not hidden

Tests

Fail-before evidence (raw output committed under docs/lanes/f5c-work-tracker-reopen/evidence/)

The two pre-existing failures, both reproduced at the parent commit

Ledger reconciliation

Deliberately not in this change

Uh oh!

Brian Krabach (bkrabach) commented Sep 3, 2026

Manager verification (independent, scratch clone) — and one manager-authored merge commit

Classification: FIX (with a required remedy path), not a bare feature

Fail-before, reproduced

All four documented tiers, measured after the merge

The merge commit (8cede04) — measured, not computed

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Fail-before evidence (raw output committed under `docs/lanes/f5c-work-tracker-reopen/evidence/`)

The merge commit (`8cede04`) — measured, not computed