BB-96 follow-up: committed marker-retry regression test does not exercise the marker branch #99

New issue

Open

opened 2026-10-19 02:39:58 +08:00 by Breadway · 0 comments

Breadway commented

2026-10-19 02:39:58 +08:00

Owner

How found: coordinator re-review of Breadway/breadboard PR98 at head 4e404eca, verifying the seven findings from PR comment 5366 (review 5442).

Confidence: high (code reading plus an independent scratch test).

Explanation: crates/breadboard-server/tests/import.rs::repair_retry_finds_remote_marker_after_webhook_echo_and_link_failure calls store::claim_unlinked_issue first to simulate the webhook echo, which links the local card before import::run executes. import::run then sees an already-linked card and skips the writeback entirely, so writeback_new_issue_result's marker lookup (forgejo::find_import_issue) is never reached. The test name promises coverage of the marker-retry path that finding 1 is about, but it would still pass if the marker lookup were deleted. I confirmed the underlying behavior is correct with a scratch test (unlinked card plus a marker-bearing remote issue, repair: true: remote create count stays 0 and the card links to the existing issue), but that scratch test was not committed.

Suggested fix: add a committed test that leaves the local card unlinked while a marker-bearing remote issue exists, runs repair, and asserts the remote create count does not increase and the card links to that issue. Optionally drive the echo through apply_issue or POST /webhooks/forgejo for a truthful race model.

Refs PR98 (merged 2a538f089a), comment 5366, review 5442.

**How found:** coordinator re-review of Breadway/breadboard PR98 at head 4e404eca, verifying the seven findings from PR comment 5366 (review 5442). **Confidence:** high (code reading plus an independent scratch test). **Explanation:** `crates/breadboard-server/tests/import.rs::repair_retry_finds_remote_marker_after_webhook_echo_and_link_failure` calls `store::claim_unlinked_issue` first to simulate the webhook echo, which links the local card before `import::run` executes. `import::run` then sees an already-linked card and skips the writeback entirely, so `writeback_new_issue_result`'s marker lookup (`forgejo::find_import_issue`) is never reached. The test name promises coverage of the marker-retry path that finding 1 is about, but it would still pass if the marker lookup were deleted. I confirmed the underlying behavior is correct with a scratch test (unlinked card plus a marker-bearing remote issue, `repair: true`: remote create count stays 0 and the card links to the existing issue), but that scratch test was not committed. **Suggested fix:** add a committed test that leaves the local card unlinked while a marker-bearing remote issue exists, runs repair, and asserts the remote create count does not increase and the card links to that issue. Optionally drive the echo through `apply_issue` or `POST /webhooks/forgejo` for a truthful race model. Refs PR98 (merged 2a538f089aca78df9b192dacd1b97da807d820b5), comment 5366, review 5442.