Surface skipped tracker cleanup on merge read-back failure #98

New Issue

2026-07-02T15:12:41-05:00

jcwalker3 commented

2026-07-02 15:12:41 -05:00

Found by a read-only trace of gitea_merge_pr() / cleanup_in_progress_for_pr().

Problem

In gitea_merge_pr() the tracker-cleanup call sits inside the same try as the post-merge read-back GET (mcp_server.py ~:1101–1110). If the merge POST succeeds but the read-back GET fails:

merge_commit is set to null,
cleanup_in_progress_for_pr() never runs,
the returned result has no cleanup_status key at all — the skip is silent,
no audit signal for the skip,
status:in-progress can remain stuck on linked issues while the merge reads as a full success (performed: true).

This contradicts the repo's explicit-degradation style everywhere else in the tracker-hygiene path ("not present", "error: <redacted>", "no linked issue found").

Expected behavior

Successful merge behavior preserved; tracker cleanup is never a merge blocker.
The merge result always carries an explicit cleanup_status once the merge is performed:
- normal cleanup result when read-back succeeds;
- "skipped (merge read-back failed)" when the read-back GET fails;
- "skipped (cleanup error: <redacted>)" if cleanup itself raises unexpectedly.
Existing redaction behavior (_redact) kept for any surfaced error text.
No change to the happy-path API call sequence (existing mocked side-effect tests must not need re-sequencing).
No release/tag behavior changes.

Implementation notes

Cleanup needs the PR title/body/head.ref, which in merge scope only exist on the read-back payload (the eligibility result does not carry them). Pre-fetching the payload before the merge POST would add an API call to the happy path and re-sequence every mocked test, so the minimal fix keeps cleanup keyed on the read-back payload but splits the try so the read-back failure and the cleanup outcome are reported distinctly and explicitly.

Acceptance criteria

Merge POST success + read-back GET failure ⇒ performed: true, merge_commit: null, cleanup_status == "skipped (merge read-back failed)", and no tracker label GET/DELETE is attempted.
Read-back success ⇒ cleanup runs exactly as today.
Unexpected cleanup exception ⇒ cleanup_status = "skipped (cleanup error: <redacted>)", merge still reported performed.
All existing tracker-hygiene and merge-gate tests pass unchanged.

Tests required

Regression: read-back failure before cleanup (new).
Cleanup-exception surfacing with redaction (new).
Existing TestTrackerHygieneCleanup and merge-gate suites remain green.

Found by a read-only trace of `gitea_merge_pr()` / `cleanup_in_progress_for_pr()`. ## Problem In `gitea_merge_pr()` the tracker-cleanup call sits *inside* the same `try` as the post-merge read-back GET (`mcp_server.py` ~:1101–1110). If the merge POST succeeds but the read-back GET fails: * `merge_commit` is set to `null`, * `cleanup_in_progress_for_pr()` **never runs**, * the returned result has **no `cleanup_status` key at all** — the skip is silent, * no audit signal for the skip, * `status:in-progress` can remain stuck on linked issues while the merge reads as a full success (`performed: true`). This contradicts the repo's explicit-degradation style everywhere else in the tracker-hygiene path (`"not present"`, `"error: <redacted>"`, `"no linked issue found"`). ## Expected behavior * Successful merge behavior preserved; tracker cleanup is never a merge blocker. * The merge result **always** carries an explicit `cleanup_status` once the merge is performed: * normal cleanup result when read-back succeeds; * `"skipped (merge read-back failed)"` when the read-back GET fails; * `"skipped (cleanup error: <redacted>)"` if cleanup itself raises unexpectedly. * Existing redaction behavior (`_redact`) kept for any surfaced error text. * No change to the happy-path API call sequence (existing mocked side-effect tests must not need re-sequencing). * No release/tag behavior changes. ## Implementation notes Cleanup needs the PR `title`/`body`/`head.ref`, which in merge scope only exist on the read-back payload (the eligibility result does not carry them). Pre-fetching the payload before the merge POST would add an API call to the happy path and re-sequence every mocked test, so the minimal fix keeps cleanup keyed on the read-back payload but splits the `try` so the read-back failure and the cleanup outcome are reported distinctly and explicitly. ## Acceptance criteria * Merge POST success + read-back GET failure ⇒ `performed: true`, `merge_commit: null`, `cleanup_status == "skipped (merge read-back failed)"`, and no tracker label GET/DELETE is attempted. * Read-back success ⇒ cleanup runs exactly as today. * Unexpected cleanup exception ⇒ `cleanup_status` = `"skipped (cleanup error: <redacted>)"`, merge still reported performed. * All existing tracker-hygiene and merge-gate tests pass unchanged. ## Tests required * Regression: read-back failure before cleanup (new). * Cleanup-exception surfacing with redaction (new). * Existing `TestTrackerHygieneCleanup` and merge-gate suites remain green.