Treat an unresolvable cross-repo blocker as unknown, not as open forever
Closes weblib-ci#11 in part: the reconciler could add Status/Blocked but never remove it when every blocker lived in another repo, and, found live on weblib-archive#30 (2026-09-09), it went further and *reasserted* the label over a human's hand-removal every 15 minutes. Established which of the two candidate mechanisms in #11 was real by instrumenting the script and running it under a real Gitea Actions token scoped to weblib-archive, against weblib-archive#68 (a throwaway issue depending on the already-closed weblib/cfbypass#1) and the live weblib-archive#30: `deps` is never empty for a cross-repo edge (rules out "Gitea omits the entry"), but every cross-repo entry comes back as Go's zero-value Issue struct -- id 0, state "", repository null -- rather than being omitted or erroring. The old `d.get("state") != "closed"` read that blank state as "still open", which is why the label could never come off, and why it kept getting silently reapplied. `is_resolved()` keys on `id == 0` rather than on the empty `state`, since `id` cannot legitimately be zero for a real issue and a future Gitea could start populating `state` while still stubbing the rest of the entry. `reconcile()` now keeps the existing add/remove behaviour whenever every blocker resolves (same-repo dependencies always do; pinned by TestSameRepoUnchanged so this cannot regress), but when a blocker cannot be resolved and none of the resolvable ones are positively open, it changes nothing and logs the issue number instead of guessing -- keeping the asymmetry from cfbypass#19 (a false "unblocked" sends someone at work that cannot be done; a false "blocked" only hides work) rather than inverting it. Also probed whether the repo-scoped token could resolve a cross-repo blocker by querying the other repo directly instead of via `/dependencies`: `GET repos/weblib/cfbypass/issues/1` and `GET repos/weblib/cfbypass` both 404 under weblib-archive's own Actions token, while the same call against its own repo succeeds. So this token cannot see cfbypass at all; making cross-repo state resolvable would need a broader-scoped credential, which is a real blast-radius decision left for bit rather than assumed here. The "unknown -> leave alone, log it" behaviour in this PR fixes the stuck/reasserted label either way, with or without that credential ever existing. 12 new tests in test_sync_blocked_label.py, offline against a local http.server fake. Proved by fault injection: reverting sync_blocked_label.py to its pre-fix form (keeping the new tests) fails exactly the 5 tests in TestUnresolvableCrossRepoBlocker -- one of them reproducing the live reassert bug verbatim (`[(30, 'add', 'blocked by #0, #0')]`) -- while every other test still passes; restoring the fix (diffed byte-identical against the pre-revert copy) turns the suite green again. python3 test_sync_blocked_label.py # 12 tests, OK python3 test_report_job_log.py # 22 tests, OK (unaffected) Co-authored-by: bit <bit@das-labor.org>
This commit is contained in:
+56
-13
@@ -13,12 +13,23 @@ hand.
|
||||
**An issue with no dependencies at all is never touched.** Only issues that
|
||||
have at least one dependency are managed:
|
||||
|
||||
* any blocker not positively closed -> ensure the label is present
|
||||
* every blocker closed -> ensure the label is absent
|
||||
* any blocker positively open -> ensure the label is present
|
||||
* every blocker positively closed -> ensure the label is absent
|
||||
* any blocker's state cannot be confirmed either way -> leave the label
|
||||
exactly as it is, and say so in the log
|
||||
|
||||
The asymmetry is deliberate. A dependency in *another* repo is not fully
|
||||
visible to the Actions token, which is scoped to one repo, so "not open" and
|
||||
"closed" are not the same statement.
|
||||
"closed" are not the same statement. Gitea does not error or omit such an
|
||||
entry -- it substitutes a zero-value stub (`id: 0`, every other field blank or
|
||||
null, including `state: ""`), so `d.get("state") != "closed"` used to treat it
|
||||
as "open forever". That kept a cross-repo `Status/Blocked` from ever being
|
||||
removed (weblib-ci#11) and, worse, made the reconciler *reassert* the label
|
||||
after a human removed it by hand (weblib-archive#30, 2026-09-09): every 15
|
||||
minutes it re-ran the same "not closed" test against the same unreadable stub
|
||||
and put the label back. Treating "cannot tell" as its own outcome -- change
|
||||
nothing, log the issue number -- fixes both without guessing an answer the
|
||||
token cannot support. See `is_resolved()`.
|
||||
|
||||
That matters because `Status/Blocked` is also applied by hand for reasons the
|
||||
graph knows nothing about -- weblib-archive#29 and #30 are blocked on a
|
||||
@@ -113,6 +124,21 @@ def label_id(forge, repo, name):
|
||||
return None
|
||||
|
||||
|
||||
def is_resolved(dep):
|
||||
"""False for a dependency entry the token could not actually read.
|
||||
|
||||
Gitea's `/dependencies` response omits nothing -- an entry the token
|
||||
cannot see still gets an element, but one where every field is Go's
|
||||
zero value: `id: 0`, `state: ""`, `repository: null`, etc. `id` is the
|
||||
field that can never legitimately be 0 for a real issue, so it is the
|
||||
one thing safe to key on -- `state` happens to be empty today, but a
|
||||
future Gitea populating it while still stubbing the rest would silently
|
||||
revive the bug this guards against if the check were on `state` instead.
|
||||
Measured against a live cross-repo case, 2026-09-09: see weblib-ci#11.
|
||||
"""
|
||||
return dep.get("id", 0) != 0
|
||||
|
||||
|
||||
def open_items(forge, repo):
|
||||
"""Open issues *and* pull requests. They share the /issues endpoint."""
|
||||
out = []
|
||||
@@ -149,19 +175,36 @@ def reconcile(forge, repo, label_name, dry_run):
|
||||
if not deps:
|
||||
# No edges, no opinion. See the docstring.
|
||||
continue
|
||||
# Fail safe: only an *explicitly closed* dependency counts as done.
|
||||
# A cross-repo blocker is not fully visible to the Actions token, which
|
||||
# is scoped to one repo -- weblib-archive#30 is blocked by
|
||||
# weblib/cfbypass#1, and the runner read that entry as not-open and
|
||||
# took the label off while cfbypass#1 was still open. Anything whose
|
||||
# state we cannot positively confirm keeps the issue blocked.
|
||||
blockers = [d for d in deps if d.get("state") != "closed"]
|
||||
# Three-way split, not two. A dependency the token could not read
|
||||
# comes back as a zero-value stub (see is_resolved) rather than being
|
||||
# omitted, and its blank `state` must not be read as "open" or
|
||||
# "closed" -- either guess has burned us. An *open* blocker anywhere
|
||||
# in the list is decisive on its own (positive evidence, add/keep the
|
||||
# label) regardless of what else is unresolvable; only "should this
|
||||
# come off" needs every entry to be positively closed.
|
||||
open_blockers = [d for d in deps if is_resolved(d) and d.get("state") != "closed"]
|
||||
unresolved = [d for d in deps if not is_resolved(d)]
|
||||
labelled = any(l["name"] == label_name for l in item.get("labels") or [])
|
||||
if blockers and not labelled:
|
||||
action, detail = "add", f"blocked by {', '.join('#%d' % d['number'] for d in blockers)}"
|
||||
if unresolved and not open_blockers:
|
||||
# Cannot confirm every blocker is closed, and no *known* open one
|
||||
# to justify keeping/adding the label on its own merits either.
|
||||
# The reconciler's job is to keep the label in step with what it
|
||||
# can see; when it cannot see part of the graph it has no
|
||||
# business asserting either state. This is what replaces the old
|
||||
# "not closed -> still blocked" fail-safe, which kept a
|
||||
# cross-repo label stuck forever and then started *reasserting*
|
||||
# it over a human's hand-removal (weblib-archive#30, weblib-ci#11)
|
||||
# -- silence was the actual defect, not the direction of the
|
||||
# guess.
|
||||
print(f" unknown {repo}#{number}: {len(unresolved)} blocker(s) unresolvable "
|
||||
f"with this token -- leaving {label_name!r} as-is ({'present' if labelled else 'absent'})",
|
||||
flush=True)
|
||||
continue
|
||||
elif open_blockers and not labelled:
|
||||
action, detail = "add", f"blocked by {', '.join('#%d' % d['number'] for d in open_blockers)}"
|
||||
if not dry_run:
|
||||
forge.post(f"repos/{repo}/issues/{number}/labels", {"labels": [lid]})
|
||||
elif not blockers and labelled:
|
||||
elif not open_blockers and not unresolved and labelled:
|
||||
action, detail = "remove", "every blocker is closed"
|
||||
if not dry_run:
|
||||
# PUT the whole set minus this label, rather than
|
||||
|
||||
Reference in New Issue
Block a user