fix: mirror markdown imports to Forgejo #98
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "feature/bb-96-import-mirroring"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Refs #83 (provider-only change; the full BB-96 card, including RAM-1 through RAM-33 repair, stays open)
Markdown import mirrors only newly created local cards on ordinary import. Explicit repair mode retries unlinked cards after previewing affected keys. Retry uses a stable local-key marker, webhook echoes claim the matching card, and link races do not create duplicate local or remote issues. Forgejo requests use bounded timeouts and locks are scoped per project. Duplicate titles fail loudly; checked items become done/closed. Responses report pending writebacks to clients.
Tests use a local fake Forgejo HTTP API.
Live RAM-1 through RAM-33 repair remains outstanding and untouched. It is excluded from this change pending explicit authority/scope; full acceptance is not claimed.
Independent review of PR 98 at head
a1a2a28871(base93de7f9b8c). Verdict: CHANGES REQUESTED (provider scope not accepted). BB-96 full card scope is retained and NOT claimed; RAM-1..33 live repair is intentionally not authorized here and was not performed.Evidence run on the exact head in an isolated worktree: cargo test --workspace --locked (all pass, incl. 5 tests in tests/import.rs), cargo clippy --workspace --all-targets --locked -D warnings (clean), xtask check-docs (OK), scripts/test-publish-bakery-stage.sh (pass), git diff --check (clean). Required CI check "check / check (push)" is success on this exact SHA. No prior reviews. mergeable=true. Gaps below come from the code plus throwaway scratch tests I ran in my worktree (not committed).
Passing: shared create/link path reused (sync.rs writeback_new_issue_result); mapped+enabled only; repo-less/disabled stays local with no pending entry; pending_writebacks {key,error} in API; denied 403 then retry links the same key; in-process concurrent imports create one card and one remote issue; repair preserves id/key/body/priority.
Required changes:
Duplicate prevention fails on the repair path (import.rs ~187-200, store.rs claim_unlinked_issue ~1323). The webhook echo of a Forgejo issue created by writeback is deduped only by claim_unlinked_issue, which requires created_at within 5 minutes. Repair targets OLD unlinked cards (RAM-1..33 are days old). Reproduced: old unlinked card "Task", webhook/apply_issue for the new Forgejo issue arrives before link_issue -> a second local card is imported (2 cards), then link_issue fails on UNIQUE(forgejo_repo,forgejo_number), the failure is reported as a pending writeback, and the Forgejo issue is orphaned. Next retry creates ANOTHER Forgejo issue, so every retry can leave one more remote duplicate. The reconcile pass can hit the same window. The fake Forgejo in tests never sends webhooks, so none of this is covered. Fix: register in-flight (project,title,card id) writebacks that apply_issue/claim consult regardless of age, and/or before creating on retry look up an existing unlinked remote issue with the same exact title and link to it instead of creating. Add a test that delivers the echo (apply_issue or /webhooks/forgejo) between remote create and link for an old card, and a remote-success-then-link-failure test asserting retry does not create a second remote issue.
Identical-title ambiguity is resolved arbitrarily and silently (import.rs HashMap keyed by lowercase title). Reproduced: two unlinked cards titled "Task" -> the map keeps whichever sorts first by updated_at, the other is never repaired and nothing is reported. Likewise the same title in two milestones in one outline creates only one card (skipped second, verified: ["ROAD-1"]). With mirroring this matters more than before. Fix: when more than one card in the project matches a title, do not pick one; report it (for example as a pending/ambiguous entry with the candidate keys) and leave them untouched. Cover with a test.
Repair is only an implicit side effect of re-POSTing the original outline. Acceptance asks for a supported, idempotent repair of already-imported unlinked issues, and the coordinator requirement is explicit user intent. As written, any import repeat silently pushes matching old cards to Forgejo, it requires the original markdown, it cannot repair cards whose titles changed since import (RAM cards were edited via PATCH), and dry_run does not preview which cards would be linked or created remotely. Fix: add an explicit repair operation (a dedicated endpoint or an explicit flag, with MCP/CLI/Documentation.md coverage) that selects unlinked cards by project and optionally by keys, supports dry_run listing exactly which keys would be created remotely, and does nothing to linked cards. Plain import should keep creating/mirroring only the cards it just created. Keep document/check-docs in sync.
Writeback failures are not clearly reported to first-party clients. ImportResult in crates/breadboard-client/src/lib.rs (~68), packages/api-client/src/index.ts (~164), apps/web Settings.svelte (~378) and crates/breadboard-cli/src/main.rs (~358) do not know pending_writebacks; the CLI prints "issues: N created" and exits 0 and the web UI shows success even when Forgejo writes were denied. Add the field and surface it (CLI lines plus non-success exit or clear warning, web warning block).
Global lock and unbounded remote calls (import.rs IMPORT_LOCK). One process-wide tokio Mutex is held across every Forgejo HTTP call for the whole outline, and the shared reqwest client (lib.rs ~39) has no timeout. One slow or hung Forgejo, or one 33+ item import, blocks every project's imports indefinitely; any authenticated caller can stall all imports. Fix: use a per-project lock (map keyed by project id) and set a bounded timeout on the Forgejo client. Process-level serialization also does not protect against non-import creates of the same title or a second server process; state this limit in the doc comment and do not count it as the duplicate-prevention story (see item 1).
Checked items imported as done: "- [x]" cards are created Done, but Forgejo create_issue always opens the remote issue. Mirrored done cards diverge from remote state (and reconcile treats Done vs open as drift). Close the remote issue after create for Done cards (or document and test the chosen behavior).
Scope/merge gate: PR body contains "Closes #83". #83 is the full BB-96 card, whose acceptance includes the RAM-1..33 repair that this PR explicitly does not do. Merging with that reference would cascade BB-96 to done (no verify-before-done label) and close Forgejo #83. I am replacing it with a non-closing "Refs #83" so the full card is not closed by this provider-only change. Nothing here authorizes done on BB-96. Not merged.
Next action: rework on the same PR/branch (merge base into branch, no force push), address 1-6, request fresh review on the new head. After merge, the RAM-1..33 repair still needs separate explicit authorization from the owner of RAM before BB-96 can be verified or moved to done.
Independent exact-head re-review: PASS (provider slice). Head
4e404eca9a, base main, parentsa1a2a28. Prior verdict changes_requested ata1a2a288(comment 5366). Reviewer: coordinator, distinct from the implementer. RAM-1 through RAM-33 live repair remains unauthorized and out of scope; BB-96's full card is NOT claimed or closed here.Re-verified at this head:
Verification run on the exact head in an isolated worktree: cargo clippy --workspace --all-targets --locked -- -D warnings (0 errors), cargo test --workspace --locked (all pass), cargo run -p xtask --locked -- check-docs (OK). CI on the exact SHA is success (check / check, dev release build, GitHub mirror). No force push, no tags; primary checkout untouched.
Non-blocking follow-ups (filed as separate cards, not blockers): the committed test named repair_retry_finds_remote_marker_after_webhook_echo_and_link_failure claims the card before the repair, so it does not actually reach the marker branch that F1 is about (behavior is correct, verified by my scratch test, but the committed regression does not guard it); and a Done card retried through the marker or post-create find_issue_by_forgejo path links without closing the remote issue.
Merging the provider slice. BB-96 stays open for the RAM-1..33 repair pending explicit authorization.