fix(store): remove_dependency falls back when br rejects --type

[mabry1985]

Summary

• remove_dependency() calls br dep remove <id> <depends_on> --type blocks, mirroring add_dependency's flag. A real br 0.2.16 install rejects --type on dep remove outright (error: unexpected argument '--type' found) — remove never needed a type to identify which edge to tear down, unlike add (which needs one to pick what kind of edge to CREATE).
• This is the actual root cause of a live, reproducible bug: cancel_feature's (board_cancel_feature is refused by open depends_on edges — a scope cut is not a completion, and edges can't be removed #145/feat: fix(cancel): allow cancel with open deps + add remove_dependency (#145) #160) drop-open-blockers step looked like a no-op on a real board. _open_blockers found the right edges, but every remove_dependency call died on --type and got silently swallowed by cancel_feature's own per-edge except BoardError (log a warning, keep going) — so br close still hit the un-dropped blocker and cancel kept failing, indistinguishable from stale plugin code from the outside. Verified live against a real board (protoEngineer's, on protoAgent) before writing this fix.
• Fix: try with --type first (older br builds may still expect it), retry once without it on that specific CLI-parse failure signature. Not blanket error-swallowing — any other failure (e.g. the edge doesn't exist) still raises.
• Found a real coverage gap too: every existing real-br integration test either cancels a feature with no open blockers, or cancels the blocker itself — never a feature that HAS an open blocker at cancel time, the one scenario that actually calls remove_dependency for real. Added that as a new br_shape-marked integration test (runs on both CI matrix legs) alongside mocked unit tests for the retry/fallback itself.

Test plan

• New unit tests: test_remove_dependency_retries_without_type_when_br_rejects_the_flag, test_remove_dependency_reraises_a_real_failure_unretried — mocked _run, verify the retry fires only on the specific CLI-parse signature
• New integration test: test_cancel_with_an_open_blocker_drops_the_edge_through_real_br — real BeadsBoard + real br binary, creates a blocker + dependent, cancels the dependent, asserts the edge actually drops
• Ran against a real local br 0.2.16 install: PB_REQUIRE_BR=1 pytest tests/ -q — 756 passed, 1 failed (pre-existing, documented 0.1.23-vs-0.2.16 quirk unrelated to this change — test_json_shape_ready_for_ready_queue_omits_labels)
• ruff check . / ruff format --check . — clean

🤖 Generated with Claude Code

[protoreview]

QA panel review — WARN

code-review · head d4a92dcb71a7 · formal

[review-synthesizer completed: workflow code-review:report]

This PR adds a --type fallback to remove_dependency plus a real-br integration test; risk is moderate and concentrated in the fallback's trigger. Fix first: store.py:788's substring match on clap's English wording — the verifier confirmed against PR head that a non-matching parse error is swallowed by cancel_feature's per-edge except BoardError (regressing to the silent no-op cancel this PR fixes) while the mocked tests bake in today's wording and stay green. The panel disagreed only on Finding 3's phrasing/anchor: the reraise test records no calls list at all, so the membership claim holds for the retry test (actual lines ~732–733, not 706) only. Verification (raw fetch at head d4a92dcb) confirmed all three findings; nothing was refuted, so all survive — minor first, then the two confirmed nits. No coverage gaps: the changed production file (store.py) carries the actionable finding, and both changed test files are represented.

Findings

	Severity	Location	Finding	Verified
🟡	minor	store.py:788	The fallback depends on clap's exact English wording "unexpected argument", so a br build whose parse error reads differently (e.g. "unknown argument '--type' …	confirmed
⚪	nit	tests/test_store.py:732	The 'retry once' semantics of the new fallback are not pinned: both new unit tests assert only membership of command tuples in the recorded calls (never a call…	confirmed
⚪	nit	tests/test_integration.py:334	The new test is marked @pytest.mark.br_shape even though it asserts no --json shape, while the marker's registered definition and the ci.yml description of the…	confirmed

findings JSON (machine-readable)

[
  {
    "file": "store.py",
    "line": 788,
    "severity": "minor",
    "category": "correctness",
    "claim": "The fallback depends on clap's exact English wording \"unexpected argument\", so a br build whose parse error reads differently (e.g. \"unknown argument '--type' found\", or a localized message) silently disables the retry and, because cancel_feature's documented per-edge `except BoardError` swallows the raise, regresses to exactly the silent no-op cancel this PR fixes — with the mocked unit tests staying green since they bake in today's wording.",
    "evidence": "if \"--type\" in str(exc) and \"unexpected argument\" in str(exc):",
    "verdict": "confirmed",
    "note": "Evidence string matches PR-head code verbatim (~line 788); verified at head SHA that cancel_feature wraps each remove_dependency in try/except BoardError: log.warning (swallows), and _run wraps stderr into str(exc); the new mocked retry test bakes in 'unexpected argument' wording, so a reworded build would regress with green mocks — exactly as claimed."
  },
  {
    "file": "tests/test_store.py",
    "line": 732,
    "severity": "nit",
    "category": "tests",
    "claim": "The 'retry once' semantics of the new fallback are not pinned: both new unit tests assert only membership of command tuples in the recorded calls (never a call count), so an implementation that retried the fallback more than once would still pass both tests.",
    "evidence": "    b.remove_dependency(\"bd-child\", \"bd-parent\")\n    assert (\"dep\", \"remove\", \"bd-child\", \"bd-parent\", \"--type\", \"blocks\") in calls\n    assert (\"dep\", \"remove\", \"bd-child\", \"bd-parent\") in calls",
    "verdict": "confirmed",
    "note": "Membership-only asserts exist in the retry test (actual lines ~732-733, not 706 — finder anchored at hunk start); no call-count assertion in either new unit test, so a >1 retry would still pass; minor phrasing imprecision: the reraise test records no calls list, so 'membership in recorded calls' applies only to the retry test, but the substance holds."
  },
  {
    "file": "tests/test_integration.py",
    "line": 334,
    "severity": "nit",
    "category": "conventions",
    "claim": "The new test is marked @pytest.mark.br_shape even though it asserts no --json shape, while the marker's registered definition and the ci.yml description of the 0.2.16 leg both say br_shape selects only 'the version-agnostic --json SHAPE contract' — the marker's documented meaning now misdescribes what it selects, and the definition in pyproject.toml was not updated to match the broader use.",
    "evidence": "@pytest.mark.br_shape  # the whole point is proving remove_dependency's --type fallback\n# (store.py) works against WHICHEVER real br is on this leg — 0.1.23 or 0.2.16.\ndef test_cancel_with_an_open_blocker_drops_the_edge_through_real_br(board):",
    "verdict": "confirmed",
    "note": "Line 334 is exactly the @pytest.mark.br_shape decorator (hunk +330,33); the test asserts board_state/cancelled/dropped_deps/_open_blockers, no --json shape; pyproject.toml marker doc and ci.yml both describe br_shape as the --json shape contract, and this PR does not touch pyproject.toml — accurate doc/use mismatch nit."
  }
]