fix: rank complete allocator inventory and resolve declared dependencies (Closes #758)
The author allocator could select an ineligible issue for two independent reasons, both fixed here. Defect 1 — candidate truncation before ranking. The loader fetched the complete open inventory via api_get_all, then sliced it to `limit` items before constructing any WorkCandidate. Because every status:ready issue ties at priority 20 and the tie breaks on lowest number, the slice — not the ranking — decided the winner, so the same query returned different answers at different `limit` values. Candidate construction now consumes the full listing; `limit` bounds the reported skip list only, and any such truncation is reported explicitly rather than silently. Defect 2 — dependency state inferred from body substrings. Only "blocked on #" and "downstream of #" were recognized, so the repository's canonical `Depends: #N, #N` field never set dependency_unmet and dependency-blocked issues were emitted as eligible. The new allocator_dependencies module parses the canonical declaration into structured references and resolves each against live issue state: open means unmet, closed means met, and unavailable evidence fails closed. The complete open listing proves openness without extra calls; anything absent from it is confirmed by a cached targeted lookup instead of being assumed closed. The legacy "blocked on #" marker still parses. A failed listing now marks the inventory incomplete and the tool fails closed instead of ranking a partial set. Selection already advanced past a skipped candidate; with dependencies resolved correctly that fall-through now actually engages, and is covered by a regression. Tests: 35 new cases across parser, resolver, loader, and an MCP-level gitea_allocate_next_work regression, including limit-invariant selection over a 73-candidate inventory and a guard proving pre-ranking truncation would change the winner. No issue number is special-cased.
This commit is contained in:
@@ -0,0 +1,150 @@
|
||||
"""Canonical dependency parsing and live-state resolution for the allocator (#758).
|
||||
|
||||
The work allocator previously inferred dependency state from two lowercase
|
||||
substrings (``"blocked on #"`` / ``"downstream of #"``). The repository's
|
||||
canonical declaration form is a ``Depends:`` field inside the issue body's
|
||||
linkage line, for example::
|
||||
|
||||
* Parent: #631 · Depends: #633, #634 · Related: #630, #434
|
||||
|
||||
That form matched neither substring, so dependency-blocked issues were emitted
|
||||
as eligible candidates. This module replaces substring inference with:
|
||||
|
||||
1. structured parsing of ``Depends:`` declarations into issue references, and
|
||||
2. resolution of each reference against **live issue state**, never body text.
|
||||
|
||||
Both halves fail closed: a reference whose state cannot be established makes
|
||||
the owning candidate ineligible rather than assignable.
|
||||
|
||||
No issue number is special-cased here (#758 AC4/AC14); the parser is driven
|
||||
entirely by the declaration syntax.
|
||||
"""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
import re
|
||||
from typing import Callable, Iterable
|
||||
|
||||
# Live issue states, as reported by Gitea.
|
||||
DEP_STATE_OPEN = "open"
|
||||
DEP_STATE_CLOSED = "closed"
|
||||
|
||||
# "Depends:" / "Depends on:" introduces the declaration. "Dependencies" does
|
||||
# not match: after "depend" it continues with "e", not "s".
|
||||
_DEPENDS_KEYWORD = re.compile(r"depends(?:\s+on)?\s*:?\s*", re.IGNORECASE)
|
||||
|
||||
# Immediately after the keyword, consume only the contiguous run of issue
|
||||
# references. Anchoring the run this way means the declaration ends naturally
|
||||
# at the next separator ("·", newline) or sibling field ("Related:"), without
|
||||
# needing to enumerate separators.
|
||||
_DEP_RUN = re.compile(
|
||||
r"\s*(#\d+(?:\s*(?:,|and|&)\s*#\d+)*)",
|
||||
re.IGNORECASE,
|
||||
)
|
||||
|
||||
# Legacy marker retained so previously-recognized bodies keep working.
|
||||
_LEGACY_BLOCKED = re.compile(r"blocked\s+on\s+#(\d+)", re.IGNORECASE)
|
||||
|
||||
_ISSUE_REF = re.compile(r"#(\d+)")
|
||||
|
||||
|
||||
def parse_dependency_refs(body: str | None) -> tuple[int, ...]:
|
||||
"""Extract declared dependency issue numbers from an issue *body*.
|
||||
|
||||
Recognizes the canonical ``Depends: #N, #N`` field (including the
|
||||
``Depends on`` spelling) plus the legacy ``blocked on #N`` marker.
|
||||
Returns references in first-seen order with duplicates removed. Malformed
|
||||
or absent declarations yield an empty tuple rather than raising.
|
||||
"""
|
||||
if not body:
|
||||
return ()
|
||||
|
||||
refs: list[int] = []
|
||||
|
||||
def _add(value: str) -> None:
|
||||
number = int(value)
|
||||
if number > 0 and number not in refs:
|
||||
refs.append(number)
|
||||
|
||||
for match in _DEPENDS_KEYWORD.finditer(body):
|
||||
run = _DEP_RUN.match(body, match.end())
|
||||
if not run:
|
||||
continue
|
||||
for ref in _ISSUE_REF.findall(run.group(1)):
|
||||
_add(ref)
|
||||
|
||||
for ref in _LEGACY_BLOCKED.findall(body):
|
||||
_add(ref)
|
||||
|
||||
return tuple(refs)
|
||||
|
||||
|
||||
def resolve_dependency_state(
|
||||
refs: Iterable[int],
|
||||
state_lookup: Callable[[int], str | None],
|
||||
*,
|
||||
subject: str = "candidate",
|
||||
) -> dict:
|
||||
"""Resolve declared *refs* against live issue state.
|
||||
|
||||
*state_lookup* maps an issue number to its live state string, or to
|
||||
``None`` when that evidence could not be obtained. A reference is:
|
||||
|
||||
* **met** when live state is ``closed``;
|
||||
* **unmet** when live state is any other live value (``open``, etc.);
|
||||
* **unavailable** when state is ``None`` or the lookup raises.
|
||||
|
||||
Unmet *and* unavailable both mark the candidate ineligible (#758 AC6/AC7):
|
||||
allocation must never assume a dependency is satisfied.
|
||||
"""
|
||||
unmet: list[int] = []
|
||||
unavailable: list[int] = []
|
||||
met: list[int] = []
|
||||
|
||||
for ref in refs:
|
||||
try:
|
||||
number = int(ref)
|
||||
except (TypeError, ValueError):
|
||||
continue
|
||||
try:
|
||||
state = state_lookup(number)
|
||||
except Exception: # noqa: BLE001 — unavailable evidence fails closed
|
||||
state = None
|
||||
normalized = (str(state).strip().lower() if state is not None else "") or None
|
||||
if normalized is None:
|
||||
unavailable.append(number)
|
||||
elif normalized == DEP_STATE_CLOSED:
|
||||
met.append(number)
|
||||
else:
|
||||
unmet.append(number)
|
||||
|
||||
reason: str | None = None
|
||||
if unmet and unavailable:
|
||||
reason = (
|
||||
f"{subject} has unresolved dependencies "
|
||||
f"{_fmt(unmet)} and unverifiable dependencies {_fmt(unavailable)} "
|
||||
"(fail closed)"
|
||||
)
|
||||
elif unmet:
|
||||
reason = (
|
||||
f"{subject} depends on unresolved issue(s) {_fmt(unmet)}; "
|
||||
"they are not closed"
|
||||
)
|
||||
elif unavailable:
|
||||
reason = (
|
||||
f"{subject} dependency evidence unavailable for {_fmt(unavailable)} "
|
||||
"(fail closed)"
|
||||
)
|
||||
|
||||
return {
|
||||
"refs": tuple(int(x) for x in refs),
|
||||
"met": tuple(met),
|
||||
"unmet": tuple(unmet),
|
||||
"unavailable": tuple(unavailable),
|
||||
"dependency_unmet": bool(unmet or unavailable),
|
||||
"reason": reason,
|
||||
}
|
||||
|
||||
|
||||
def _fmt(numbers: Iterable[int]) -> str:
|
||||
return ", ".join(f"#{n}" for n in numbers)
|
||||
+26
-1
@@ -41,6 +41,15 @@ OUTCOME_NO_SAFE = "no_safe_work"
|
||||
OUTCOME_ROLE_INELIGIBLE = "role_ineligible"
|
||||
OUTCOME_PREVIEW = "preview" # dry-run only (apply=false)
|
||||
|
||||
# Human-readable statement of how a winner is chosen (#758 AC10). Reported
|
||||
# alongside allocator results so the flat status:ready tier and its
|
||||
# oldest-number tie-break are explicit rather than incidental.
|
||||
SELECTION_POLICY = (
|
||||
"rank complete inventory by (priority desc, PRs before issues, "
|
||||
"number asc); status:ready issues share priority 20, so the oldest "
|
||||
"eligible number wins ties; result limits never affect selection"
|
||||
)
|
||||
|
||||
ROLE_AUTHOR = "author"
|
||||
ROLE_REVIEWER = "reviewer"
|
||||
ROLE_MERGER = "merger"
|
||||
@@ -242,7 +251,23 @@ def classify_skip(
|
||||
|
||||
|
||||
def sort_candidates(candidates: Sequence[WorkCandidate]) -> list[WorkCandidate]:
|
||||
"""Higher priority first; then lower number (older issues) for stability."""
|
||||
"""Rank candidates deterministically (#758 AC10).
|
||||
|
||||
Ordering key, in precedence order:
|
||||
|
||||
1. ``priority`` descending — the loader scores ``status:ready`` issues at
|
||||
20 and everything else at 1, so the ready queue ties at a single value
|
||||
by design;
|
||||
2. PRs before issues — in-flight review work drains before new authoring;
|
||||
3. ``number`` ascending — oldest first, which is what actually breaks the
|
||||
flat ``status:ready`` tie.
|
||||
|
||||
Because the ready tier is intentionally flat, rule 3 decides most real
|
||||
selections. That is only safe when ranking sees the *complete* candidate
|
||||
inventory: truncating before this call silently redefines "oldest" as
|
||||
"oldest among whatever survived the slice", which is the defect #758
|
||||
fixed. Callers must rank everything and bound reporting afterwards.
|
||||
"""
|
||||
return sorted(
|
||||
candidates,
|
||||
key=lambda c: (-int(c.priority), c.kind != "pr", int(c.number)),
|
||||
|
||||
+95
-24
@@ -1765,6 +1765,7 @@ import review_workflow_load # noqa: E402
|
||||
import mcp_session_state # noqa: E402
|
||||
import stale_review_decision_lock # noqa: E402
|
||||
import allocator_service # noqa: E402
|
||||
import allocator_dependencies # noqa: E402
|
||||
import control_plane_db # noqa: E402
|
||||
import lease_lifecycle # noqa: E402
|
||||
import incident_bridge # noqa: E402
|
||||
@@ -17794,16 +17795,24 @@ def _allocator_candidates_from_gitea(
|
||||
repo: str,
|
||||
include_issues: bool = True,
|
||||
include_prs: bool = True,
|
||||
limit: int = 50,
|
||||
) -> tuple[list[Any], list[str]]:
|
||||
"""Build allocator candidates from live Gitea open issues/PRs."""
|
||||
) -> tuple[list[Any], list[str], bool]:
|
||||
"""Build allocator candidates from live Gitea open issues/PRs.
|
||||
|
||||
Returns ``(candidates, reasons, inventory_complete)``. The inventory is
|
||||
assembled in full and is never truncated before ranking (#758 AC1/AC3):
|
||||
any result/display bound belongs to the caller's reporting, not to
|
||||
candidate construction. ``inventory_complete`` is False when a required
|
||||
listing failed, so the caller can fail closed instead of ranking a
|
||||
silently short candidate set.
|
||||
"""
|
||||
reasons: list[str] = []
|
||||
candidates: list[Any] = []
|
||||
inventory_complete = True
|
||||
try:
|
||||
h, o, r = _resolve(remote, host, org, repo)
|
||||
auth = _auth(h)
|
||||
except Exception as exc: # noqa: BLE001
|
||||
return [], [f"failed to resolve Gitea target: {_redact(str(exc))}"]
|
||||
return [], [f"failed to resolve Gitea target: {_redact(str(exc))}"], False
|
||||
|
||||
if include_prs:
|
||||
try:
|
||||
@@ -17813,7 +17822,8 @@ def _allocator_candidates_from_gitea(
|
||||
except Exception as exc: # noqa: BLE001
|
||||
reasons.append(f"failed to list open PRs: {_redact(str(exc))}")
|
||||
prs = []
|
||||
for pr in prs[: max(1, int(limit))]:
|
||||
inventory_complete = False
|
||||
for pr in prs:
|
||||
if not isinstance(pr, dict):
|
||||
continue
|
||||
number = pr.get("number")
|
||||
@@ -17885,7 +17895,39 @@ def _allocator_candidates_from_gitea(
|
||||
f"failed to list open issues: {_redact(str(exc2))}"
|
||||
)
|
||||
issues = []
|
||||
for issue in issues[: max(1, int(limit))]:
|
||||
inventory_complete = False
|
||||
|
||||
# Live dependency evidence (#758 AC5). The complete open-issue listing
|
||||
# already proves which references are still open; anything absent from
|
||||
# it is confirmed by a targeted lookup rather than assumed closed, so a
|
||||
# deleted or unreachable reference fails closed instead of passing.
|
||||
open_issue_numbers = {
|
||||
int(i["number"])
|
||||
for i in issues
|
||||
if isinstance(i, dict)
|
||||
and i.get("number") is not None
|
||||
and i.get("pull_request") is None
|
||||
}
|
||||
dep_state_cache: dict[int, str | None] = {}
|
||||
|
||||
def _issue_state(number: int) -> str | None:
|
||||
if number in open_issue_numbers:
|
||||
return allocator_dependencies.DEP_STATE_OPEN
|
||||
if number in dep_state_cache:
|
||||
return dep_state_cache[number]
|
||||
state: str | None = None
|
||||
try:
|
||||
data = api_request(
|
||||
"GET", f"{repo_api_url(h, o, r)}/issues/{number}", auth
|
||||
)
|
||||
if isinstance(data, dict):
|
||||
state = str(data.get("state") or "").strip().lower() or None
|
||||
except Exception: # noqa: BLE001 — unavailable evidence fails closed
|
||||
state = None
|
||||
dep_state_cache[number] = state
|
||||
return state
|
||||
|
||||
for issue in issues:
|
||||
if not isinstance(issue, dict):
|
||||
continue
|
||||
# Pull requests also appear in /issues on Gitea — skip them.
|
||||
@@ -17903,21 +17945,15 @@ def _allocator_candidates_from_gitea(
|
||||
body = str(issue.get("body") or "")
|
||||
title = str(issue.get("title") or "")
|
||||
blocked = "status:blocked" in labels
|
||||
# Explicit downstream dependency: #612 waits on #600 allocator.
|
||||
dep_unmet = False
|
||||
dep_reason = None
|
||||
# Generic body markers for blocked-on unfinished deps.
|
||||
# (Hard-coded #612→#600 block removed after #600 merged — #612.)
|
||||
lower_body = body.lower()
|
||||
if "blocked on #" in lower_body or "downstream of #" in lower_body:
|
||||
# Only treat as unmet when body still marks a live open dependency
|
||||
# pattern; callers may clear labels when deps complete.
|
||||
dep_unmet = "blocked on #" in lower_body
|
||||
dep_reason = (
|
||||
f"issue#{number} body marks an unmet dependency"
|
||||
if dep_unmet
|
||||
else None
|
||||
)
|
||||
# #758: parse the canonical "Depends: #N, #N" declaration into
|
||||
# structured references and resolve each against live issue state.
|
||||
# Body substrings no longer decide dependency status.
|
||||
dep_refs = allocator_dependencies.parse_dependency_refs(body)
|
||||
dep_result = allocator_dependencies.resolve_dependency_state(
|
||||
dep_refs, _issue_state, subject=f"issue#{number}"
|
||||
)
|
||||
dep_unmet = dep_result["dependency_unmet"]
|
||||
dep_reason = dep_result["reason"]
|
||||
try:
|
||||
candidates.append(
|
||||
allocator_service.WorkCandidate(
|
||||
@@ -17937,7 +17973,7 @@ def _allocator_candidates_from_gitea(
|
||||
f"skipped invalid issue candidate #{number}: {_redact(str(exc))}"
|
||||
)
|
||||
|
||||
return candidates, reasons
|
||||
return candidates, reasons, inventory_complete
|
||||
|
||||
|
||||
@mcp.tool()
|
||||
@@ -18253,15 +18289,32 @@ def gitea_allocate_next_work(
|
||||
"substrate": "control_plane_db",
|
||||
}
|
||||
else:
|
||||
candidates, inv_reasons = _allocator_candidates_from_gitea(
|
||||
candidates, inv_reasons, inventory_complete = _allocator_candidates_from_gitea(
|
||||
remote=remote,
|
||||
host=host,
|
||||
org=o,
|
||||
repo=r,
|
||||
include_issues=include_issues,
|
||||
include_prs=include_prs,
|
||||
limit=limit,
|
||||
)
|
||||
# #758 AC3: ranking a silently short inventory could select the wrong
|
||||
# candidate, so an incomplete listing is a fail-closed stop.
|
||||
if not inventory_complete:
|
||||
return {
|
||||
"success": False,
|
||||
"outcome": allocator_service.OUTCOME_NO_SAFE,
|
||||
"apply": bool(apply),
|
||||
"reasons": inv_reasons
|
||||
+ [
|
||||
"candidate inventory is incomplete; refusing to rank a "
|
||||
"partial candidate set (fail closed, #758)"
|
||||
],
|
||||
"inventory_complete": False,
|
||||
"assignment": None,
|
||||
"substrate": "control_plane_db",
|
||||
"file_lock_only": False,
|
||||
"comment_lease_only": False,
|
||||
}
|
||||
|
||||
sid = (session_id or "").strip() or (
|
||||
f"{profile_name or 'session'}-{os.getpid()}-{uuid.uuid4().hex[:8]}"
|
||||
@@ -18284,6 +18337,24 @@ def gitea_allocate_next_work(
|
||||
result["inventory_source"] = (
|
||||
"candidates_json" if candidates_json else "gitea_live"
|
||||
)
|
||||
result["inventory_complete"] = True
|
||||
result["selection_policy"] = allocator_service.SELECTION_POLICY
|
||||
# #758 AC2/AC10: `limit` bounds the reported skip list only — selection has
|
||||
# already happened over the complete inventory above. Any truncation here is
|
||||
# stated explicitly so a shortened report is never read as full coverage.
|
||||
reported_limit = max(1, int(limit))
|
||||
skipped_all = result.get("skipped") or []
|
||||
result["skipped_total"] = len(skipped_all)
|
||||
result["limit_applies_to"] = "reported_skip_list_only"
|
||||
if len(skipped_all) > reported_limit:
|
||||
result["skipped"] = skipped_all[:reported_limit]
|
||||
result["skipped_report_truncated"] = True
|
||||
result.setdefault("reasons", []).append(
|
||||
f"skip list truncated for reporting: showing {reported_limit} of "
|
||||
f"{len(skipped_all)} skipped candidates (selection unaffected)"
|
||||
)
|
||||
else:
|
||||
result["skipped_report_truncated"] = False
|
||||
# #606: watchdog check-ins for the recurring jobs this allocator run
|
||||
# performs — global stale-lease expiry, terminal-lock lookup, and the
|
||||
# allocator itself. Best-effort; a failed selection reports "error".
|
||||
|
||||
@@ -0,0 +1,266 @@
|
||||
"""Dependency parsing/resolution and allocator completeness tests (#758).
|
||||
|
||||
Covers the two defects behind #758:
|
||||
|
||||
* Defect 1 — candidate truncation before ranking, which let a result-size
|
||||
parameter change the winner.
|
||||
* Defect 2 — dependency state inferred from body substrings, which emitted
|
||||
canonical ``Depends:`` blocked issues as eligible.
|
||||
|
||||
No production behavior is special-cased for any issue number (#758 AC14), so
|
||||
these tests use synthetic issue numbers throughout.
|
||||
"""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
import os
|
||||
import tempfile
|
||||
import unittest
|
||||
|
||||
import allocator_dependencies
|
||||
from allocator_service import (
|
||||
OUTCOME_PREVIEW,
|
||||
SELECTION_POLICY,
|
||||
WorkCandidate,
|
||||
allocate_next_work,
|
||||
classify_skip,
|
||||
sort_candidates,
|
||||
)
|
||||
from control_plane_db import ControlPlaneDB
|
||||
|
||||
# The canonical linkage line this repository writes into issue bodies.
|
||||
CANONICAL_BODY = (
|
||||
"## Dependencies and linkage\n\n"
|
||||
"* Parent: #900 · Depends: #901, #902 · Related: #903, #904\n"
|
||||
)
|
||||
|
||||
|
||||
class ParseDependencyRefsTest(unittest.TestCase):
|
||||
def test_parses_canonical_depends_field(self) -> None:
|
||||
self.assertEqual(
|
||||
allocator_dependencies.parse_dependency_refs(CANONICAL_BODY),
|
||||
(901, 902),
|
||||
)
|
||||
|
||||
def test_stops_at_sibling_field_and_ignores_related(self) -> None:
|
||||
""""Related:" refs must never be treated as dependencies."""
|
||||
refs = allocator_dependencies.parse_dependency_refs(CANONICAL_BODY)
|
||||
self.assertNotIn(903, refs)
|
||||
self.assertNotIn(904, refs)
|
||||
self.assertNotIn(900, refs) # Parent is not a dependency
|
||||
|
||||
def test_single_reference(self) -> None:
|
||||
body = "* Parent: #10 · Depends: #11 · Related: #12"
|
||||
self.assertEqual(allocator_dependencies.parse_dependency_refs(body), (11,))
|
||||
|
||||
def test_depends_on_spelling_and_and_separator(self) -> None:
|
||||
body = "Depends on #21 and #22\n"
|
||||
self.assertEqual(
|
||||
allocator_dependencies.parse_dependency_refs(body), (21, 22)
|
||||
)
|
||||
|
||||
def test_newline_terminates_declaration(self) -> None:
|
||||
body = "Depends: #31, #32\nRelated: #33\n"
|
||||
self.assertEqual(
|
||||
allocator_dependencies.parse_dependency_refs(body), (31, 32)
|
||||
)
|
||||
|
||||
def test_legacy_blocked_on_marker_still_recognized(self) -> None:
|
||||
body = "This work is blocked on #41 until that lands.\n"
|
||||
self.assertEqual(allocator_dependencies.parse_dependency_refs(body), (41,))
|
||||
|
||||
def test_dependencies_heading_alone_is_not_a_declaration(self) -> None:
|
||||
""""Dependencies and linkage" must not parse as "Depends"."""
|
||||
body = "## Dependencies and linkage\n\n* Related: #51\n"
|
||||
self.assertEqual(allocator_dependencies.parse_dependency_refs(body), ())
|
||||
|
||||
def test_deduplicates_and_preserves_order(self) -> None:
|
||||
body = "Depends: #61, #62, #61\n"
|
||||
self.assertEqual(
|
||||
allocator_dependencies.parse_dependency_refs(body), (61, 62)
|
||||
)
|
||||
|
||||
def test_malformed_and_empty_inputs(self) -> None:
|
||||
for body in ("", None, "Depends:", "Depends: none", "Depends: TBD\n"):
|
||||
self.assertEqual(allocator_dependencies.parse_dependency_refs(body), ())
|
||||
|
||||
|
||||
class ResolveDependencyStateTest(unittest.TestCase):
|
||||
def test_open_dependency_is_unmet(self) -> None:
|
||||
result = allocator_dependencies.resolve_dependency_state(
|
||||
(901, 902), lambda n: "open", subject="issue#644"
|
||||
)
|
||||
self.assertTrue(result["dependency_unmet"])
|
||||
self.assertEqual(result["unmet"], (901, 902))
|
||||
self.assertIn("#901", result["reason"])
|
||||
|
||||
def test_closed_dependencies_are_met(self) -> None:
|
||||
result = allocator_dependencies.resolve_dependency_state(
|
||||
(901, 902), lambda n: "closed"
|
||||
)
|
||||
self.assertFalse(result["dependency_unmet"])
|
||||
self.assertEqual(result["met"], (901, 902))
|
||||
self.assertIsNone(result["reason"])
|
||||
|
||||
def test_mixed_open_and_closed_is_unmet(self) -> None:
|
||||
states = {901: "closed", 902: "open"}
|
||||
result = allocator_dependencies.resolve_dependency_state(
|
||||
(901, 902), states.get
|
||||
)
|
||||
self.assertTrue(result["dependency_unmet"])
|
||||
self.assertEqual(result["unmet"], (902,))
|
||||
self.assertEqual(result["met"], (901,))
|
||||
|
||||
def test_unavailable_evidence_fails_closed(self) -> None:
|
||||
"""AC7: unknown state must block, never pass."""
|
||||
result = allocator_dependencies.resolve_dependency_state(
|
||||
(901,), lambda n: None
|
||||
)
|
||||
self.assertTrue(result["dependency_unmet"])
|
||||
self.assertEqual(result["unavailable"], (901,))
|
||||
self.assertIn("fail closed", result["reason"])
|
||||
|
||||
def test_raising_lookup_fails_closed(self) -> None:
|
||||
def boom(_n: int) -> str:
|
||||
raise RuntimeError("lookup exploded")
|
||||
|
||||
result = allocator_dependencies.resolve_dependency_state((901,), boom)
|
||||
self.assertTrue(result["dependency_unmet"])
|
||||
self.assertEqual(result["unavailable"], (901,))
|
||||
|
||||
def test_no_refs_is_eligible(self) -> None:
|
||||
result = allocator_dependencies.resolve_dependency_state((), lambda n: None)
|
||||
self.assertFalse(result["dependency_unmet"])
|
||||
self.assertIsNone(result["reason"])
|
||||
|
||||
|
||||
class DependencyBlockedCandidateTest(unittest.TestCase):
|
||||
"""A dependency-blocked candidate must be skipped, not selected."""
|
||||
|
||||
def test_classify_skip_rejects_unmet_dependency(self) -> None:
|
||||
candidate = WorkCandidate(
|
||||
kind="issue",
|
||||
number=644,
|
||||
labels=("status:ready",),
|
||||
priority=20,
|
||||
dependency_unmet=True,
|
||||
dependency_reason="issue#644 depends on unresolved issue(s) #633",
|
||||
)
|
||||
reason = classify_skip(candidate, role="author", terminal_pr=None)
|
||||
self.assertIsNotNone(reason)
|
||||
self.assertIn("#633", reason)
|
||||
|
||||
|
||||
class SelectionInvarianceTest(unittest.TestCase):
|
||||
"""AC1/AC2/AC11: ranking sees everything; result bounds cannot move the winner."""
|
||||
|
||||
def setUp(self) -> None:
|
||||
self._tmp = tempfile.TemporaryDirectory()
|
||||
self.db = ControlPlaneDB(os.path.join(self._tmp.name, "cp.sqlite3"))
|
||||
|
||||
def tearDown(self) -> None:
|
||||
self._tmp.cleanup()
|
||||
|
||||
@staticmethod
|
||||
def _ready_issue(number: int, **kw) -> WorkCandidate:
|
||||
return WorkCandidate(
|
||||
kind="issue",
|
||||
number=number,
|
||||
labels=("status:ready",),
|
||||
priority=20,
|
||||
title=f"issue {number}",
|
||||
**kw,
|
||||
)
|
||||
|
||||
def _preview(self, candidates):
|
||||
return allocate_next_work(
|
||||
self.db,
|
||||
session_id="s-758",
|
||||
role="author",
|
||||
remote="prgs",
|
||||
org="Scaled-Tech-Consulting",
|
||||
repo="Gitea-Tools",
|
||||
candidates=candidates,
|
||||
apply=False,
|
||||
)
|
||||
|
||||
def test_more_than_fifty_candidates_lowest_number_wins(self) -> None:
|
||||
"""Winner is the oldest eligible issue across a >50 inventory."""
|
||||
candidates = [self._ready_issue(n) for n in range(600, 700)] # 100 items
|
||||
result = self._preview(candidates)
|
||||
self.assertEqual(result["outcome"], OUTCOME_PREVIEW)
|
||||
self.assertEqual(result["selected"]["number"], 600)
|
||||
|
||||
def test_selection_is_invariant_to_candidate_ordering(self) -> None:
|
||||
"""Ranking must not depend on the order the inventory arrived in."""
|
||||
forward = [self._ready_issue(n) for n in range(600, 700)]
|
||||
reverse = list(reversed(forward))
|
||||
self.assertEqual(
|
||||
self._preview(forward)["selected"]["number"],
|
||||
self._preview(reverse)["selected"]["number"],
|
||||
)
|
||||
|
||||
def test_truncating_inventory_changes_winner(self) -> None:
|
||||
"""Regression guard: this is exactly what pre-ranking slicing did.
|
||||
|
||||
A 50-item slice of a 100-item inventory yields a different winner, so
|
||||
any future reintroduction of pre-ranking truncation is detectable.
|
||||
"""
|
||||
full = [self._ready_issue(n) for n in range(600, 700)]
|
||||
sliced = sorted(full, key=lambda c: -c.number)[:50]
|
||||
self.assertNotEqual(
|
||||
self._preview(full)["selected"]["number"],
|
||||
self._preview(sliced)["selected"]["number"],
|
||||
)
|
||||
|
||||
def test_blocked_first_candidate_falls_through_to_next(self) -> None:
|
||||
"""AC8: a blocked winner must not end the iteration."""
|
||||
blocked = self._ready_issue(
|
||||
600,
|
||||
dependency_unmet=True,
|
||||
dependency_reason="issue#600 depends on unresolved issue(s) #599",
|
||||
)
|
||||
result = self._preview([blocked, self._ready_issue(601)])
|
||||
self.assertEqual(result["selected"]["number"], 601)
|
||||
skipped = {s["number"] for s in result["skipped"]}
|
||||
self.assertIn(600, skipped)
|
||||
|
||||
def test_all_blocked_yields_no_safe_work(self) -> None:
|
||||
candidates = [
|
||||
self._ready_issue(
|
||||
n, dependency_unmet=True, dependency_reason=f"issue#{n} blocked"
|
||||
)
|
||||
for n in range(600, 605)
|
||||
]
|
||||
result = self._preview(candidates)
|
||||
self.assertIsNone(result["selected"])
|
||||
self.assertEqual(len(result["skipped"]), 5)
|
||||
|
||||
def test_dry_run_and_apply_select_identically(self) -> None:
|
||||
"""AC9: apply mode must not re-rank differently from preview."""
|
||||
candidates = [self._ready_issue(n) for n in range(600, 700)]
|
||||
preview = self._preview(candidates)
|
||||
applied = allocate_next_work(
|
||||
self.db,
|
||||
session_id="s-758-apply",
|
||||
role="author",
|
||||
remote="prgs",
|
||||
org="Scaled-Tech-Consulting",
|
||||
repo="Gitea-Tools",
|
||||
candidates=candidates,
|
||||
apply=True,
|
||||
)
|
||||
self.assertEqual(
|
||||
preview["selected"]["number"], applied["selected"]["number"]
|
||||
)
|
||||
|
||||
def test_sort_is_stable_and_documented(self) -> None:
|
||||
ordered = sort_candidates(
|
||||
[self._ready_issue(603), self._ready_issue(601), self._ready_issue(602)]
|
||||
)
|
||||
self.assertEqual([c.number for c in ordered], [601, 602, 603])
|
||||
self.assertIn("never affect selection", SELECTION_POLICY)
|
||||
|
||||
|
||||
if __name__ == "__main__":
|
||||
unittest.main()
|
||||
@@ -0,0 +1,227 @@
|
||||
"""MCP-level allocator inventory and dependency regressions (#758).
|
||||
|
||||
Exercises ``_allocator_candidates_from_gitea`` and the ``gitea_allocate_next_work``
|
||||
tool end to end against a faked Gitea API, proving:
|
||||
|
||||
* the complete open-issue inventory is ranked (no pre-ranking truncation);
|
||||
* ``limit`` cannot change which candidate wins;
|
||||
* canonical ``Depends:`` declarations are resolved from live issue state;
|
||||
* unavailable dependency evidence fails closed;
|
||||
* an incomplete listing fails closed instead of ranking a partial set.
|
||||
|
||||
Issue numbers here are synthetic; no production number is special-cased.
|
||||
"""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
import os
|
||||
import tempfile
|
||||
import unittest
|
||||
from unittest.mock import patch
|
||||
|
||||
import gitea_mcp_server as srv
|
||||
from control_plane_db import ControlPlaneDB
|
||||
|
||||
FAKE_AUTH = "token REDACTED"
|
||||
ORG = "Scaled-Tech-Consulting"
|
||||
REPO = "Gitea-Tools"
|
||||
|
||||
|
||||
def _issue(number: int, *, body: str = "", labels=("status:ready",)) -> dict:
|
||||
return {
|
||||
"number": number,
|
||||
"title": f"issue {number}",
|
||||
"body": body,
|
||||
"labels": [{"name": name} for name in labels],
|
||||
"state": "open",
|
||||
}
|
||||
|
||||
|
||||
def _depends_body(*refs: int) -> str:
|
||||
joined = ", ".join(f"#{r}" for r in refs)
|
||||
return f"## Dependencies and linkage\n\n* Parent: #999 · Depends: {joined}\n"
|
||||
|
||||
|
||||
class _FakeGitea:
|
||||
"""Minimal stand-in for the two Gitea list endpoints plus issue lookups."""
|
||||
|
||||
def __init__(self, issues, *, closed=(), unavailable=(), fail_issue_list=False):
|
||||
self.issues = list(issues)
|
||||
self.closed = set(closed)
|
||||
self.unavailable = set(unavailable)
|
||||
self.fail_issue_list = fail_issue_list
|
||||
self.lookups: list[int] = []
|
||||
|
||||
def api_get_all(self, url, _auth, **_kw):
|
||||
if "/pulls" in url:
|
||||
return []
|
||||
if self.fail_issue_list:
|
||||
raise RuntimeError("issue listing failed")
|
||||
return list(self.issues)
|
||||
|
||||
def api_request(self, _method, url, _auth, **_kw):
|
||||
number = int(url.rsplit("/", 1)[-1])
|
||||
self.lookups.append(number)
|
||||
if number in self.unavailable:
|
||||
raise RuntimeError("lookup failed")
|
||||
if number in self.closed:
|
||||
return {"number": number, "state": "closed"}
|
||||
return {"number": number, "state": "open"}
|
||||
|
||||
|
||||
class AllocatorInventoryTest(unittest.TestCase):
|
||||
"""Direct tests of the candidate loader."""
|
||||
|
||||
def _load(self, fake, **kwargs):
|
||||
with patch("gitea_mcp_server._resolve", return_value=("h", ORG, REPO)), patch(
|
||||
"gitea_mcp_server._auth", return_value=FAKE_AUTH
|
||||
), patch("gitea_mcp_server.api_get_all", side_effect=fake.api_get_all), patch(
|
||||
"gitea_mcp_server.api_request", side_effect=fake.api_request
|
||||
):
|
||||
return srv._allocator_candidates_from_gitea(
|
||||
remote="prgs", host=None, org=ORG, repo=REPO, **kwargs
|
||||
)
|
||||
|
||||
def test_full_inventory_above_fifty_is_ranked(self) -> None:
|
||||
"""AC1: all 73 open issues become candidates, not the first 50."""
|
||||
fake = _FakeGitea([_issue(n) for n in range(600, 673)])
|
||||
candidates, _reasons, complete = self._load(fake)
|
||||
self.assertTrue(complete)
|
||||
self.assertEqual(len(candidates), 73)
|
||||
self.assertEqual(min(c.number for c in candidates), 600)
|
||||
self.assertEqual(max(c.number for c in candidates), 672)
|
||||
|
||||
def test_open_dependency_marks_candidate_unmet(self) -> None:
|
||||
"""AC4/AC5/AC6: canonical Depends on an open issue blocks the candidate."""
|
||||
fake = _FakeGitea(
|
||||
[_issue(600, body=_depends_body(601, 602)), _issue(601), _issue(602)]
|
||||
)
|
||||
candidates, _reasons, _complete = self._load(fake)
|
||||
blocked = next(c for c in candidates if c.number == 600)
|
||||
self.assertTrue(blocked.dependency_unmet)
|
||||
self.assertIn("#601", blocked.dependency_reason)
|
||||
|
||||
def test_closed_dependency_is_eligible(self) -> None:
|
||||
"""A dependency absent from the open list is confirmed closed, not assumed."""
|
||||
fake = _FakeGitea([_issue(600, body=_depends_body(500))], closed={500})
|
||||
candidates, _reasons, _complete = self._load(fake)
|
||||
candidate = next(c for c in candidates if c.number == 600)
|
||||
self.assertFalse(candidate.dependency_unmet)
|
||||
self.assertIn(500, fake.lookups) # proved live, not inferred
|
||||
|
||||
def test_unavailable_dependency_evidence_fails_closed(self) -> None:
|
||||
"""AC7: an unreachable dependency must block, never pass."""
|
||||
fake = _FakeGitea([_issue(600, body=_depends_body(500))], unavailable={500})
|
||||
candidates, _reasons, _complete = self._load(fake)
|
||||
candidate = next(c for c in candidates if c.number == 600)
|
||||
self.assertTrue(candidate.dependency_unmet)
|
||||
self.assertIn("fail closed", candidate.dependency_reason)
|
||||
|
||||
def test_dependency_state_lookups_are_cached(self) -> None:
|
||||
"""Repeated references resolve with a single live lookup."""
|
||||
fake = _FakeGitea(
|
||||
[_issue(n, body=_depends_body(500)) for n in range(600, 610)],
|
||||
closed={500},
|
||||
)
|
||||
self._load(fake)
|
||||
self.assertEqual(fake.lookups.count(500), 1)
|
||||
|
||||
def test_open_dependency_needs_no_lookup(self) -> None:
|
||||
"""The complete open listing already proves openness."""
|
||||
fake = _FakeGitea([_issue(600, body=_depends_body(601)), _issue(601)])
|
||||
self._load(fake)
|
||||
self.assertNotIn(601, fake.lookups)
|
||||
|
||||
def test_failed_issue_listing_reports_incomplete(self) -> None:
|
||||
"""AC3: a failed listing must not silently yield a short inventory."""
|
||||
fake = _FakeGitea([], fail_issue_list=True)
|
||||
_candidates, reasons, complete = self._load(fake)
|
||||
self.assertFalse(complete)
|
||||
self.assertTrue(any("failed to list open issues" in r for r in reasons))
|
||||
|
||||
|
||||
class AllocateNextWorkToolTest(unittest.TestCase):
|
||||
"""End-to-end tests of the gitea_allocate_next_work MCP tool."""
|
||||
|
||||
def setUp(self) -> None:
|
||||
self._tmp = tempfile.TemporaryDirectory()
|
||||
self.db = ControlPlaneDB(os.path.join(self._tmp.name, "cp.sqlite3"))
|
||||
|
||||
def tearDown(self) -> None:
|
||||
self._tmp.cleanup()
|
||||
|
||||
def _allocate(self, fake, **kwargs):
|
||||
with patch("gitea_mcp_server._profile_operation_gate", return_value=None), patch(
|
||||
"gitea_mcp_server._resolve", return_value=("h", ORG, REPO)
|
||||
), patch("gitea_mcp_server._auth", return_value=FAKE_AUTH), patch(
|
||||
"gitea_mcp_server.get_profile",
|
||||
return_value={"profile_name": "prgs-author", "role": "author"},
|
||||
), patch(
|
||||
"gitea_mcp_server._authenticated_username", return_value="jcwalker3"
|
||||
), patch(
|
||||
"gitea_mcp_server._control_plane_db_or_error", return_value=(self.db, [])
|
||||
), patch(
|
||||
"gitea_mcp_server.api_get_all", side_effect=fake.api_get_all
|
||||
), patch(
|
||||
"gitea_mcp_server.api_request", side_effect=fake.api_request
|
||||
), patch(
|
||||
"gitea_mcp_server.sentry_observability.monitor_checkin", return_value=None
|
||||
):
|
||||
return srv.gitea_allocate_next_work(
|
||||
remote="prgs", org=ORG, repo=REPO, role="author", **kwargs
|
||||
)
|
||||
|
||||
def test_limit_does_not_change_selection(self) -> None:
|
||||
"""AC2/AC11: the winner is identical at limit=1 and limit=300."""
|
||||
issues = [_issue(n) for n in range(600, 673)] # 73 candidates
|
||||
low = self._allocate(_FakeGitea(issues), limit=1)
|
||||
high = self._allocate(_FakeGitea(issues), limit=300)
|
||||
self.assertEqual(low["selected"]["number"], high["selected"]["number"])
|
||||
self.assertEqual(low["selected"]["number"], 600)
|
||||
self.assertEqual(low["candidate_count"], 73)
|
||||
self.assertEqual(high["candidate_count"], 73)
|
||||
|
||||
def test_dependency_blocked_winner_falls_through(self) -> None:
|
||||
"""AC8: a blocked highest-ranked issue yields the next eligible one."""
|
||||
issues = [
|
||||
_issue(600, body=_depends_body(601)),
|
||||
_issue(601),
|
||||
_issue(602),
|
||||
]
|
||||
result = self._allocate(_FakeGitea(issues))
|
||||
# 600 is blocked by open 601; 601 itself is a valid candidate.
|
||||
self.assertEqual(result["selected"]["number"], 601)
|
||||
skipped = {s["number"] for s in result["skipped"]}
|
||||
self.assertIn(600, skipped)
|
||||
|
||||
def test_limit_truncates_only_the_reported_skip_list(self) -> None:
|
||||
"""A shortened report is labelled, never presented as full coverage."""
|
||||
issues = [_issue(n, body=_depends_body(999)) for n in range(600, 640)]
|
||||
issues.append(_issue(999)) # open dependency blocks all of the above
|
||||
issues.append(_issue(700)) # the one eligible candidate
|
||||
result = self._allocate(_FakeGitea(issues), limit=5)
|
||||
self.assertTrue(result["skipped_report_truncated"])
|
||||
self.assertEqual(len(result["skipped"]), 5)
|
||||
self.assertGreater(result["skipped_total"], 5)
|
||||
self.assertEqual(result["limit_applies_to"], "reported_skip_list_only")
|
||||
|
||||
def test_incomplete_inventory_fails_closed(self) -> None:
|
||||
"""AC3: no selection is made from a partial candidate set."""
|
||||
result = self._allocate(_FakeGitea([], fail_issue_list=True))
|
||||
self.assertFalse(result["success"])
|
||||
self.assertFalse(result["inventory_complete"])
|
||||
self.assertIsNone(result["assignment"])
|
||||
self.assertTrue(
|
||||
any("fail closed" in r for r in result["reasons"]),
|
||||
result["reasons"],
|
||||
)
|
||||
|
||||
def test_selection_policy_is_reported(self) -> None:
|
||||
"""AC10: tie-breaking is stated in the result, not left implicit."""
|
||||
result = self._allocate(_FakeGitea([_issue(600)]))
|
||||
self.assertIn("selection_policy", result)
|
||||
self.assertIn("number asc", result["selection_policy"])
|
||||
|
||||
|
||||
if __name__ == "__main__":
|
||||
unittest.main()
|
||||
Reference in New Issue
Block a user