From bea3735e47b45a7c7b13ba00022a5d321a883beb Mon Sep 17 00:00:00 2001 From: naudachu Date: Mon, 10 Aug 2026 17:23:12 +0500 Subject: [PATCH 1/2] fix: apply --limit to the write, not to the selection MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit pull.py's docstring said "the limit is on the write, not on the selection", and the code did the opposite: `list_issues` truncated the payload list to `limit`, and pull.py dropped the closed ones after that. A milestone whose first issues are closed therefore answered `--limit 20` with twelve files, and the only statement about the behavior anywhere was the false one. The limit now counts what the run leaves in the store. `list_issues` takes a `keep` predicate, pages keep arriving until `limit` payloads have satisfied it, and the ones that did not are still returned — they were enumerated, and pull.py still reports them as "N closed, not stored". What `keep` means stays the caller's business; the transport only counts. pull.py hands it `lands_in_store`, which is the same test the walk itself applies: a closed issue counts only when the store already has it, since that one is refreshed rather than dropped. Pagination is the other half, and it cuts both ways. `paginate` is now a thin wrapper over a new `pages` generator, so the page after the one that fills the budget is never requested. In the other direction "fetch until N are kept" is "fetch the whole tracker" on a filter that matches mostly closed issues, so a keep-bounded read scans at most PAGE_SLACK times the pages the limit would need if nothing were dropped, then warns on stderr and returns short. Raising --limit raises that ceiling with it. --deps is outside the count: a dependency is followed because an issue named it. remote.py keeps the old meaning and now says so in as many words — it writes nothing, so there is no write for a limit to bound, and its --limit caps the listing, closed issues included. Same flag, two jobs, documented in both scripts and in the skill's command table. Also refuses `--limit 0` instead of dividing by the page size and raising ZeroDivisionError. tests/test_pull_limit.py stubs the transport with a fake that serves `page=`/`limit=` itself, so the request pattern is observed rather than assumed: exactly N files out of a half-closed selection, the second page fetched and the third not, the scan stopping at the budget with a warning, and remote.py's listing unchanged. 251 tests, no network. Co-Authored-By: Claude Opus 5 (1M context) --- skills/sync/SKILL.md | 22 ++- skills/sync/scripts/_gitea.py | 77 ++++++-- skills/sync/scripts/pull.py | 56 +++++- skills/sync/scripts/remote.py | 5 + tests/test_pull_limit.py | 349 ++++++++++++++++++++++++++++++++++ 5 files changed, 488 insertions(+), 21 deletions(-) create mode 100644 tests/test_pull_limit.py diff --git a/skills/sync/SKILL.md b/skills/sync/SKILL.md index 25110e1..ddcddee 100644 --- a/skills/sync/SKILL.md +++ b/skills/sync/SKILL.md @@ -37,8 +37,8 @@ the `tea-guard` hook reads. No pin → exit with a pointer to `/tea:auth`. | Script | What it does | |---|---| -| `remote.py [--state] [--label] [--milestone] [-q TEXT]` | discovery: one line per Gitea issue to stdout, writes nothing | -| `pull.py ` or `pull.py --milestone M \| --label L \| -q TEXT` | Gitea → `tmp/issues/.md`, plus `.comments.md` when the thread is not empty | +| `remote.py [--state] [--label] [--milestone] [-q TEXT] [--limit N]` | discovery: one line per Gitea issue to stdout, writes nothing; `--limit` caps the **listing** (default 30) | +| `pull.py ` or `pull.py --milestone M \| --label L \| -q TEXT [--limit N]` | Gitea → `tmp/issues/.md`, plus `.comments.md` when the thread is not empty; `--limit` caps what is **stored** (default 100) | | `push.py [id…] [--update] [--dry-run]` | local → Gitea; validates first, **deletes the local file on success** and prints where it lives now | | `comment.py --file F \| --body TEXT [--edit N]` | post or edit a comment, then refetch the thread | | `labels.py [--dry-run] [--fix]` | bootstrap the canonical `type/*` + `severity/*` set in a repo; exact names left alone, lookalikes reported, drift fixed only with `--fix` | @@ -114,6 +114,24 @@ already on disk is refreshed either way — the local copy learns it was closed instead of staying open forever. Key mode is exempt: `pull.py 1` fetches a closed issue as always, because an address is not a bulk read. +**`--limit N` bounds the write, not the selection.** N is how many issues this +run leaves in the store — written, or left in place by `--cached`. Closed ones +that were enumerated and thrown away do not spend it, so `--limit 20` over a +milestone whose first 30 issues are closed still writes 20, as long as 20 open +ones are there to write. Pagination follows the budget rather than the other way +round: + +| | | +|---|---| +| budget full | the next page is never requested | +| pages run out | fewer than N, and that is the honest answer | +| filter matches almost only closed issues | at most 4× the pages N would need if nothing were dropped, then a warning on stderr and a short answer — raising `--limit` raises that ceiling too | +| `--deps` | outside the count: a dependency is followed because an issue named it, not because the filter selected it | + +`remote.py --limit` means something else, deliberately: it caps the **listing**, +closed issues included. It writes nothing, so there is no write for a limit to +bound — enumeration is its whole job. + **Comments come with every pull** — there is no flag. An issue that has a thread gets `tmp/issues/.comments.md` beside it, in key mode and in filter mode alike, and the issue's output line says how many. An issue with none diff --git a/skills/sync/scripts/_gitea.py b/skills/sync/scripts/_gitea.py index 7d3783a..d596964 100644 --- a/skills/sync/scripts/_gitea.py +++ b/skills/sync/scripts/_gitea.py @@ -30,6 +30,13 @@ import urllib.parse PAYLOAD_DIR = ".payload" REMOTE_MAP = ".remote.json" +# How far past the ideal page count a `keep`-bounded listing may scan before it +# gives up (see list_issues). The ideal is what `limit` would need if every +# payload counted; the slack pays for the ones that do not. It is a bound on +# requests, deliberately small: "fetch until N are kept" without one is "fetch +# the whole tracker" on any repo whose filter matches mostly closed issues. +PAGE_SLACK = 4 + def die(msg, code=1): sys.stderr.write("%s: %s\n" % (os.path.basename(sys.argv[0]), msg)) @@ -114,17 +121,28 @@ def api(login, endpoint, method="GET", payload=None, payload_name=None, die("`tea api %s` returned non-JSON:\n%s" % (endpoint, body[:500])) -def paginate(login, endpoint, limit=50, max_pages=40, **kw): - """GET a list endpoint page by page; return the concatenated list.""" +def pages(login, endpoint, limit=50, max_pages=40, **kw): + """GET a list endpoint page by page, yielding each page as it arrives. + + A generator, because a caller whose budget is spent on what it *keeps* + cannot be served by a function that fetches everything first: the page after + the one that completed the budget must never be requested. Stop consuming + and no further request is made.""" sep = "&" if "?" in endpoint else "?" - out = [] for page in range(1, max_pages + 1): batch = api(login, "%s%spage=%d&limit=%d" % (endpoint, sep, page, limit), **kw) if not isinstance(batch, list) or not batch: - break - out.extend(batch) + return + yield batch if len(batch) < limit: - break + return # a short page is the last one + + +def paginate(login, endpoint, limit=50, max_pages=40, **kw): + """GET a list endpoint page by page; return the concatenated list.""" + out = [] + for batch in pages(login, endpoint, limit=limit, max_pages=max_pages, **kw): + out.extend(batch) return out @@ -187,11 +205,31 @@ def matches(payload, milestone_id=None, labels=()): def list_issues(login, base, state="open", labels=(), query=None, - milestone=None, limit=100): + milestone=None, limit=100, keep=None): """Filtered issue payloads. Returns (payloads, milestone_title). One request per page, and the payload already carries the issue bodies — a - whole milestone costs one call per 50 issues, not one per issue.""" + whole milestone costs one call per 50 issues, not one per issue. + + `limit` counts the payloads the CALLER cares about, not the ones the server + returned. Without `keep` those are the same thing and this behaves as it + always did. With it, `keep(payload)` says whether a payload counts, pages + keep coming until `limit` of them have, and the returned list carries the + ones that did not count too — they were enumerated, and a caller that has + something to say about them (pull.py: "N closed, not stored") still can. + + What `keep` means is the caller's business; this module only counts. Two + boundaries hold whatever it decides: + + - **Stop at the limit.** The page after the one that completed the budget + is not requested — `pages` is a generator and this loop returns out of it. + - **Stop at the page budget.** A predicate that rejects everything must not + turn a bounded read into a walk of the whole tracker, so a filtered read + may scan at most `PAGE_SLACK` times the pages `limit` would need if every + payload counted. Hitting that with an unfilled budget is a warning, not a + silent short answer: the caller asked for N and is told it got fewer.""" + if limit < 1: + die("--limit must be 1 or more, got %d" % limit) ms_id, ms_title = (None, None) if milestone is not None: ms_id, ms_title = resolve_milestone(login, base, milestone) @@ -206,10 +244,25 @@ def list_issues(login, base, state="open", labels=(), query=None, endpoint = "%s/issues?%s" % (base, urllib.parse.urlencode(params)) per_page = min(limit, 50) - got = paginate(login, endpoint, limit=per_page, - max_pages=max(1, -(-limit // per_page))) - got = [p for p in got if matches(p, ms_id, labels)] - return got[:limit], ms_title + ideal = max(1, -(-limit // per_page)) + budget = ideal if keep is None else ideal * PAGE_SLACK + + got, kept, seen_pages, last_full = [], 0, 0, False + for batch in pages(login, endpoint, limit=per_page, max_pages=budget): + seen_pages += 1 + last_full = len(batch) == per_page + for p in batch: + if not matches(p, ms_id, labels): + continue + got.append(p) + if keep is None or keep(p): + kept += 1 + if kept >= limit: + return got, ms_title + if keep is not None and seen_pages >= budget and last_full: + warn("scanned %d page(s) and stopped %d short of --limit %d — there may" + " be more; narrow the filter or raise --limit" % (budget, limit - kept, limit)) + return got, ms_title def get_issue(login, base, number): diff --git a/skills/sync/scripts/pull.py b/skills/sync/scripts/pull.py index 6b522a4..98c3cfc 100644 --- a/skills/sync/scripts/pull.py +++ b/skills/sync/scripts/pull.py @@ -28,11 +28,29 @@ not exposed (404 on Gitea 1.26) — use milestones or labels, or the web UI. A closed issue is not a unit of work, so filter mode enumerates it but leaves it out of the store: `--state all` still shows the whole picture, and only -`--state closed` writes one. The limit is on the write, not on the selection — -an issue already on disk is refreshed either way, so the local copy learns it -was closed instead of staying open forever, and the count of the ones left out -goes to stderr. Key mode is exempt: an address is not a bulk read, and -`pull.py 1` fetches a closed issue as it always did. +`--state closed` writes one. An issue already on disk is refreshed either way, +so the local copy learns it was closed instead of staying open forever, and the +count of the ones left out goes to stderr. Key mode is exempt: an address is not +a bulk read, and `pull.py 1` fetches a closed issue as it always did. + +**`--limit` is on the write, not on the selection.** It counts the issues this +run puts in the store — written, or left in place by `--cached` — and never the +closed ones it enumerated and threw away. `--limit 20` over a milestone whose +first 30 issues are closed still writes 20, if 20 open ones are there to write: +pages keep coming until the budget is full. Two boundaries keep that honest: + +- Pages stop the moment the budget is full. Never one page more. +- A filtered read may scan at most `_gitea.PAGE_SLACK` times the pages the limit + would need if nothing were dropped. A filter that matches almost only closed + issues therefore ends in a warning and a short answer, not in a walk of the + whole tracker. Narrow the filter, or raise `--limit`, which raises the budget + with it. +- `--deps` is outside the count: a dependency is followed because an issue + named it, not because the filter selected it. + +`remote.py` is the deliberate exception, and it is not the same flag twice: it +writes nothing at all, so there is no write to bound and its `--limit` means +what it says — how many lines to print. Comments ride along by default, in both modes and for every issue written: the thread lands in tmp/issues/.comments.md, beside the issue. It costs @@ -98,6 +116,23 @@ def id_for(payload, store_ids, remote_map, repo, root): taken=store_ids) +def lands_in_store(payload, drop_closed, store_ids, remote_map, repo, root): + """Would this payload leave a file in the store? The `--limit` predicate. + + It has to be the same test the walk below applies, or the budget is spent on + issues that never land — which is the bug this exists to prevent. So: a + closed issue counts only when the store already has it (it is refreshed, and + that is a write); anything else counts, including one `--cached` will skip, + because a skipped issue is still an issue the store holds when the run ends. + + Cheap in the common case: only a closed payload costs an `id_for`, and that + is a lookup plus, at worst, a stat.""" + if not (drop_closed and payload.get("state") == "closed"): + return True + id = id_for(payload, store_ids, remote_map, repo, root) + return os.path.isfile(issue.path_of(root, id)) + + def comments_path(root, id): """Where an issue's comment thread lives — beside it, under the same slug. Named in `_gitea` because push.py has to delete the same file.""" @@ -131,7 +166,9 @@ def main(): ap.add_argument("-q", "--query", help="search text in title/body") ap.add_argument("--state", default="open", choices=["open", "closed", "all"], help="filter mode only (default: open)") - ap.add_argument("--limit", type=int, default=100, help="filter mode cap (default: 100)") + ap.add_argument("--limit", type=int, default=100, + help="filter mode: how many issues to STORE, not to enumerate" + " (default: 100)") ap.add_argument("--deps", action="store_true", help="follow dependencies and pull them") ap.add_argument("--depth", type=int, default=3, help="max dependency depth (default: 3)") ap.add_argument("--cached", action="store_true", @@ -180,9 +217,14 @@ def main(): # ---- seeds ----------------------------------------------------------- if filtered: + # The limit bounds the write, so the transport is told what a write is + # and counts those; the closed ones it enumerated on the way come back + # in the list anyway, to be reported and dropped below. payloads, ms_title = _gitea.list_issues( login, base, state=args.state, labels=args.label, query=args.query, - milestone=args.milestone, limit=args.limit) + milestone=args.milestone, limit=args.limit, + keep=lambda p: lands_in_store(p, drop_closed, store_ids, remote_map, + repo, root)) if not payloads: _gitea.die("no issues match that filter") what = [] diff --git a/skills/sync/scripts/remote.py b/skills/sync/scripts/remote.py index 5c19aa1..a27b22a 100644 --- a/skills/sync/scripts/remote.py +++ b/skills/sync/scripts/remote.py @@ -16,6 +16,11 @@ Usage: remote.py [--state open|closed|all] [--label L]… [-q TEXT] [--milestone M] [--limit N] [--repo owner/repo] +`--limit` here caps the LISTING: N lines out, closed ones among them. That is +not what the same flag means to `pull.py`, and the difference is not an +oversight — pull.py bounds what it writes, and this command writes nothing, so +there is nothing else for a limit to bound. Enumeration is the whole job. + Login: the operator's pin from .claude/settings.local.json (see /tea:auth). """ import argparse diff --git a/tests/test_pull_limit.py b/tests/test_pull_limit.py new file mode 100644 index 0000000..4e733d5 --- /dev/null +++ b/tests/test_pull_limit.py @@ -0,0 +1,349 @@ +#!/usr/bin/env python3 +""" +`pull.py --limit N` bounds the WRITE, not the selection. + +The bug this file exists to keep dead: the limit used to cut the list of +payloads before pull.py dropped the closed ones, so a milestone whose first +issues are closed spent the budget on issues that never reached disk — +`--limit 20` wrote twelve, and the docstring promised twenty. + +What is asserted, in the order the fix has to hold it: + +1. **The count is of files.** N issues under the filter that would be stored → + exactly N files, however many closed ones were enumerated on the way. +2. **Pagination serves the budget.** More pages are requested while the budget + is unfilled, and the page after the one that fills it is never requested. +3. **The scan is bounded.** A filter that matches almost only closed issues + stops after `_gitea.PAGE_SLACK` times the ideal page count, says so, and + returns short — it does not walk the tracker. +4. **`remote.py` is unchanged.** Its `--limit` still caps the listing, closed + issues included, because it writes nothing there is a limit for. + +The transport is stubbed at `_gitea.api`, the way the other suites do it, and +the stub serves `page=` / `limit=` itself so the request pattern is a real +observation and not an assumption. No network, and no test writes to the +developer's store: each one builds its own in a `tempfile.TemporaryDirectory()`. +""" +import contextlib +import io +import os +import sys +import tempfile +import unittest +import urllib.parse +from unittest import mock + +_ROOT = os.path.dirname(os.path.dirname(os.path.abspath(__file__))) +for _p in (os.path.join(_ROOT, "skills", "sync", "scripts"), + os.path.join(_ROOT, "skills", "issue", "scripts")): + if _p not in sys.path: + sys.path.insert(0, _p) + +import _gitea # noqa: E402 +import issue # noqa: E402 +import map as gmap # noqa: E402 +import pull # noqa: E402 +import remote # noqa: E402 + +REPO = "claude-skills/tea" +BASE = "repos/%s" % REPO + +BODY = """## Summary +Прозаическое описание задачи. + +## Spec +skills/issue/references/format.md + +## Acceptance criteria +- [ ] что-нибудь работает +""" + + +def payload(number, state="open", title=None, comments=0): + return {"number": number, "title": title or "Issue number %d" % number, + "body": BODY, "state": state, "comments": comments, + "labels": [{"name": "type/task"}], "assignees": [], "milestone": None, + "ref": "main", "updated_at": "2026-08-10T00:00:00Z", + "html_url": "https://git.example/%s/issues/%d" % (REPO, number), + "repository": {"full_name": REPO}} + + +def alternating(count, first="closed"): + """`count` issues, every other one closed. The shape of the bug report: + closed issues sitting in front of the open ones, in page order.""" + other = "open" if first == "closed" else "closed" + return [payload(n, first if n % 2 else other) for n in range(1, count + 1)] + + +class FakeTracker(object): + """`tea api` answered from a list, with real pagination. + + It slices on the `page=` and `limit=` it was given rather than ignoring + them, so "which pages were requested" is something the test can read off + `self.list_pages` instead of inferring.""" + + def __init__(self, payloads): + self.payloads = list(payloads) + self.list_pages = [] # (page, per_page), in request order + + def api(self, login, endpoint, method="GET", payload=None, + payload_name=None, out_root=None, allow_fail=False): + path, _, qs = endpoint.partition("?") + q = urllib.parse.parse_qs(qs) + + if path == "%s/issues" % BASE and method == "GET": + page, per = int(q["page"][0]), int(q["limit"][0]) + self.list_pages.append((page, per)) + return self.payloads[(page - 1) * per:(page - 1) * per + per] + + if path.endswith("/comments"): + return [] + + if path.endswith("/dependencies"): + return [] + + if "/issues/" in path and method == "GET": + n = int(path.rsplit("/", 1)[1]) + for p in self.payloads: + if p["number"] == n: + return p + return None + + raise AssertionError("unstubbed call: %s %s" % (method, endpoint)) + + +class PullLimitTestCase(unittest.TestCase): + """A temp store, a fake tracker, no git and no network.""" + + def setUp(self): + self.tmp = tempfile.TemporaryDirectory(prefix="tea-limit-") + self.addCleanup(self.tmp.cleanup) + self.root = os.path.join(self.tmp.name, "tmp", "issues") + os.makedirs(self.root) + p = mock.patch.object(_gitea, "require_login", lambda: "test-login") + p.start() + self.addCleanup(p.stop) + + # -- runners ----------------------------------------------------------- + + def serve(self, payloads): + self.fake = FakeTracker(payloads) + p = mock.patch.object(_gitea, "api", self.fake.api) + p.start() + self.addCleanup(p.stop) + return self.fake + + def run_pull(self, *argv): + return self._run(pull, "pull.py", argv) + + def run_remote(self, *argv): + return self._run(remote, "remote.py", argv) + + def _run(self, mod, name, argv): + out, err = io.StringIO(), io.StringIO() + args = [name, "--repo", REPO, "--out", self.root] + list(argv) + with mock.patch.object(sys, "argv", args), \ + contextlib.redirect_stdout(out), \ + contextlib.redirect_stderr(err): + mod.main() + return out.getvalue(), err.getvalue() + + # -- assertions -------------------------------------------------------- + + def stored(self): + return sorted(issue.all_ids(self.root)) + + def assertStoredCount(self, n, why=""): + got = self.stored() + self.assertEqual(len(got), n, "%d issue(s) in the store, wanted %d%s: %s" + % (len(got), n, why and " — " + why, got)) + + +# -------------------------------------------------------------------------- +# 1. the count is of files +# -------------------------------------------------------------------------- + +class LimitCountsWritesTest(PullLimitTestCase): + + def test_closed_issues_do_not_spend_the_budget(self): + """The regression. Half the selection is closed and stands in front of + the open ones; the limit still buys ten files.""" + self.serve(alternating(40)) + self.run_pull("-q", "x", "--state", "all", "--limit", "10") + self.assertStoredCount(10) + + def test_only_open_issues_landed(self): + self.serve(alternating(40)) + self.run_pull("-q", "x", "--state", "all", "--limit", "10") + for id in self.stored(): + self.assertEqual(issue.load(self.root, id).state, "open") + + def test_the_dropped_ones_are_still_reported(self): + """Enumerated-and-dropped is not silence: the closed ones seen on the + pages that were fetched are counted on stderr.""" + self.serve(alternating(40)) + _, err = self.run_pull("-q", "x", "--state", "all", "--limit", "10") + self.assertIn("closed issue(s) enumerated, not stored", err) + + def test_a_closed_issue_already_in_the_store_spends_it(self): + """It is refreshed rather than dropped — that is a write, so it counts. + The limit is on what the store holds when the run ends, and this issue + is in it.""" + kept = issue.Issue(id="already-here", title="Already here", body=BODY, + labels=["type/task"], origin="gitea", + extra={"gitea": gmap.remote_key(REPO, 1)}) + issue.save(self.root, kept) + _gitea.save_map(self.root, {gmap.remote_key(REPO, 1): "already-here"}) + + self.serve(alternating(40)) # #1 is closed, and is on disk + self.run_pull("-q", "x", "--state", "all", "--limit", "10") + self.assertStoredCount(10) + self.assertEqual(issue.load(self.root, "already-here").state, "closed", + "a stored issue must learn it was closed") + + def test_state_closed_writes_closed_ones(self): + """Nothing above may leak into the mode where closed IS the selection.""" + self.serve([payload(n, "closed") for n in range(1, 21)]) + self.run_pull("-q", "x", "--state", "closed", "--limit", "6") + self.assertStoredCount(6) + + +# -------------------------------------------------------------------------- +# 2. pagination serves the budget +# -------------------------------------------------------------------------- + +class PaginationFollowsTheBudgetTest(PullLimitTestCase): + + def test_more_pages_are_fetched_until_the_budget_is_full(self): + """One page of ten holds five open issues, so ten files cost two.""" + self.serve(alternating(40)) + self.run_pull("-q", "x", "--state", "all", "--limit", "10") + self.assertStoredCount(10) + self.assertEqual([p for p, _ in self.fake.list_pages], [1, 2]) + + def test_the_page_after_the_last_needed_one_is_never_requested(self): + """The budget fills inside page 2; page 3 exists and must not be asked + for. Bounding the write must not become fetching the whole repo.""" + self.serve(alternating(200)) + self.run_pull("-q", "x", "--state", "all", "--limit", "10") + self.assertEqual(len(self.fake.list_pages), 2, + "extra pages requested: %r" % (self.fake.list_pages,)) + + def test_an_unfiltered_selection_still_costs_one_page(self): + """Nothing is dropped, so nothing changes: the old arithmetic holds.""" + self.serve([payload(n) for n in range(1, 60)]) + self.run_pull("-q", "x", "--limit", "10") + self.assertStoredCount(10) + self.assertEqual(len(self.fake.list_pages), 1) + + def test_running_out_of_pages_gives_a_short_answer(self): + """Six issues, three of them open, `--limit 10`: three files, no crash, + and no page beyond the last.""" + self.serve(alternating(6)) + self.run_pull("-q", "x", "--state", "all", "--limit", "10") + self.assertStoredCount(3) + self.assertEqual(len(self.fake.list_pages), 1) + + +# -------------------------------------------------------------------------- +# 3. the scan is bounded +# -------------------------------------------------------------------------- + +class ScanIsBoundedTest(PullLimitTestCase): + + def test_a_selection_of_only_closed_issues_stops_at_the_page_budget(self): + self.serve([payload(n, "closed") for n in range(1, 501)]) + _, err = self.run_pull("-q", "x", "--state", "all", "--limit", "10") + self.assertStoredCount(0) + self.assertEqual(len(self.fake.list_pages), _gitea.PAGE_SLACK, + "the scan walked past its budget: %r" % (self.fake.list_pages,)) + self.assertIn("short of --limit", err) + + def test_a_full_budget_does_not_warn(self): + """The warning means "there may be more"; it must not fire on a run + that got everything it asked for.""" + self.serve(alternating(40)) + _, err = self.run_pull("-q", "x", "--state", "all", "--limit", "10") + self.assertNotIn("short of --limit", err) + + def test_a_selection_that_ran_out_does_not_warn(self): + """Six issues in the repo and the server said so — that is an answer, + not a truncation.""" + self.serve(alternating(6)) + _, err = self.run_pull("-q", "x", "--state", "all", "--limit", "10") + self.assertNotIn("short of --limit", err) + + +# -------------------------------------------------------------------------- +# 4. remote.py is the deliberate exception +# -------------------------------------------------------------------------- + +class RemoteListingIsUnchangedTest(PullLimitTestCase): + + def test_the_listing_limit_still_counts_lines_not_writes(self): + """remote.py writes nothing, so there is no write to bound: ten lines + out, closed ones among them, one request.""" + self.serve(alternating(40)) + out, _ = self.run_remote("-q", "x", "--state", "all", "--limit", "10") + numbered = [l for l in out.splitlines() if l.startswith("#")] + self.assertEqual(len(numbered), 10) + self.assertTrue(any("closed" in l for l in numbered), + "a listing that hides closed issues is not a listing") + self.assertEqual(len(self.fake.list_pages), 1) + + def test_it_leaves_the_store_alone(self): + self.serve(alternating(40)) + self.run_remote("-q", "x", "--state", "all", "--limit", "10") + self.assertStoredCount(0, "discovery wrote to the store") + + +# -------------------------------------------------------------------------- +# the transport on its own +# -------------------------------------------------------------------------- + +class ListIssuesKeepTest(PullLimitTestCase): + """`_gitea.list_issues` without a caller in front of it — the counting rule + is the transport's, and it is testable without a store.""" + + def list(self, payloads, **kw): + self.serve(payloads) + return _gitea.list_issues("test-login", BASE, state="all", **kw) + + def test_without_keep_the_limit_caps_the_selection(self): + got, _ = self.list(alternating(40), limit=10) + self.assertEqual(len(got), 10) + + def test_with_keep_the_limit_caps_the_kept(self): + got, _ = self.list(alternating(40), limit=10, + keep=lambda p: p["state"] == "open") + self.assertEqual(len([p for p in got if p["state"] == "open"]), 10) + + def test_the_rejected_ones_come_back_too(self): + """They were enumerated. The caller reports them; the transport does + not get to throw away what it did not count.""" + got, _ = self.list(alternating(40), limit=10, + keep=lambda p: p["state"] == "open") + self.assertTrue([p for p in got if p["state"] == "closed"]) + + def test_a_limit_below_one_is_refused(self): + """The page arithmetic divides by the page size, and a limit of zero + used to make that a traceback. It is a usage error, so it reads like + one.""" + with self.assertRaises(SystemExit): + self.list(alternating(4), limit=0) + + def test_pull_requests_never_count(self): + """`matches` drops them, so they cannot spend the budget either.""" + mixed = [] + for n in range(1, 41): + p = payload(n) + if n % 2: + p["pull_request"] = {"merged": False} + mixed.append(p) + got, _ = self.list(mixed, limit=10, keep=lambda p: True) + self.assertEqual(len(got), 10) + self.assertFalse([p for p in got if p.get("pull_request")]) + + +if __name__ == "__main__": + unittest.main() From a2e9a88186a39639c11d9e1d670556e4fdc77ada Mon Sep 17 00:00:00 2001 From: naudachu Date: Mon, 10 Aug 2026 17:41:27 +0500 Subject: [PATCH 2/2] feat: follow dependencies on every pull by default MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `depends:` was filled and blockers were pulled only under `--deps`, so the plain `pull.py ` — the only way to get a pushed issue back — answered with a file whose graph was empty and an `issue_tree.py` that drew it as a root with no blockers. The edge was not lost, but it was not asked for, and it cannot be recovered locally: `map.from_api` writes slugs into the `## Depends on` prose and never `#N`, so Gitea's native graph is the only source there is. A pull now returns the unit of work — the issue and what blocks it. `--deps` stays accepted and does nothing, so existing calls and /tea:sync's tables keep working; `--no-deps` is the way out and spends no request on either half. The cost is accepted and stated rather than hidden. The native links are now fetched ONCE per issue instead of twice (they both fill `depends:` and steer the walk), and only for an issue that lands in the store — a closed one that filter mode drops no longer drags its blockers in behind it. That makes the number quotable, and pull.py's docstring quotes it: a milestone of 50 open issues costs one list request plus 50, where it used to cost one. In filter mode a blocker no filter selected still lands in the store and still sits outside `--limit`, deliberately, and both are documented; the exception is a closed blocker, dropped like any other closed issue with the edge to it. Co-Authored-By: Claude Opus 5 (1M context) --- AGENTS.md | 4 +- skills/sync/SKILL.md | 38 +++- skills/sync/scripts/pull.py | 114 +++++++--- skills/sync/scripts/push.py | 2 +- tests/test_checkbox_merge.py | 5 + tests/test_pull_deps_default.py | 354 ++++++++++++++++++++++++++++++++ 6 files changed, 478 insertions(+), 39 deletions(-) create mode 100644 tests/test_pull_deps_default.py diff --git a/AGENTS.md b/AGENTS.md index 7a42a2a..5699637 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -138,7 +138,9 @@ line so plain grep works without a parser. - **A successful push deletes the local file** (`.md` and `.comments.md`), and prints the number and URL the issue now lives at. `--update` too: one rule, no exception. What is in the store is what has not - left. Get it back with `pull.py `. + left. Get it back with `pull.py ` — which brings its blockers back with it: + a pull returns the unit of work, not one row of it. `--no-deps` narrows it to + the one issue, and the cost of the default is in `pull.py`'s docstring. - Deletion happens only after a confirmed tracker response and only after `.remote.json` has been written. Network down, non-2xx, an answer that does not carry the right number: the file stays and the run stops. A never-pushed diff --git a/skills/sync/SKILL.md b/skills/sync/SKILL.md index ddcddee..59f382d 100644 --- a/skills/sync/SKILL.md +++ b/skills/sync/SKILL.md @@ -38,7 +38,7 @@ the `tea-guard` hook reads. No pin → exit with a pointer to `/tea:auth`. | Script | What it does | |---|---| | `remote.py [--state] [--label] [--milestone] [-q TEXT] [--limit N]` | discovery: one line per Gitea issue to stdout, writes nothing; `--limit` caps the **listing** (default 30) | -| `pull.py ` or `pull.py --milestone M \| --label L \| -q TEXT [--limit N]` | Gitea → `tmp/issues/.md`, plus `.comments.md` when the thread is not empty; `--limit` caps what is **stored** (default 100) | +| `pull.py ` or `pull.py --milestone M \| --label L \| -q TEXT [--limit N]` | Gitea → `tmp/issues/.md`, plus `.comments.md` when the thread is not empty; follows dependencies by default (`--no-deps` to stop); `--limit` caps what is **stored** (default 100) | | `push.py [id…] [--update] [--dry-run]` | local → Gitea; validates first, **deletes the local file on success** and prints where it lives now | | `comment.py --file F \| --body TEXT [--edit N]` | post or edit a comment, then refetch the thread | | `labels.py [--dry-run] [--fix]` | bootstrap the canonical `type/*` + `severity/*` set in a repo; exact names left alone, lookalikes reported, drift fixed only with `--fix` | @@ -88,7 +88,7 @@ python3 /scripts/pull.py 42 python3 /scripts/pull.py --milestone 6 # id or title python3 /scripts/pull.py --label type/bug --state all python3 /scripts/pull.py -q sqlc --limit 20 -python3 /scripts/pull.py 40 --deps # follow dependencies +python3 /scripts/pull.py 40 --no-deps # this issue only ``` Do not loop over numbers to pull a group — pass the filter. The list endpoint @@ -126,7 +126,7 @@ round: | budget full | the next page is never requested | | pages run out | fewer than N, and that is the honest answer | | filter matches almost only closed issues | at most 4× the pages N would need if nothing were dropped, then a warning on stderr and a short answer — raising `--limit` raises that ceiling too | -| `--deps` | outside the count: a dependency is followed because an issue named it, not because the filter selected it | +| dependencies | outside the count: a blocker is followed because a stored issue named it, not because the filter selected it — so `--limit 20` can leave more than 20 files behind | `remote.py --limit` means something else, deliberately: it caps the **listing**, closed issues included. It writes nothing, so there is no write for a limit to @@ -138,7 +138,31 @@ mode alike, and the issue's output line says how many. An issue with none costs nothing: the count arrives in the list payload, so no request is made and no file is written — and a file left over from a thread that has since been emptied is deleted. `--cached` skips the thread along with the body, so a -skipped issue makes no request at all. +skipped issue makes one request for its links and no other. + +**Dependencies come with every pull too, and this one costs.** A pull answers +with the unit of work — the issue and what blocks it — so `depends:` is filled +from Gitea's native graph and every blocker is pulled as well, recursively, down +to `--depth` (default 3). It has to come from the native graph: the body's +`## Depends on` section holds slugs, never `#N`, so there is no edge to recover +from the text. `--no-deps` turns off both halves. `--deps` is still accepted and +does nothing — it names the default. + +| | requests | +|---|---| +| every issue that lands in the store | **+1** — `GET …/issues/{n}/dependencies`, fetched once and used twice (fills `depends:`, steers the walk) | +| every blocker the selection did not already carry | **+1** to fetch it, then its own links, until `--depth` | +| a closed issue filter mode drops | 0 — nothing was stored, so there is no unit of work to complete | +| `--milestone X` over 50 open issues | 1 list request + 50, plus a pair per outside blocker — it used to be 1 | +| the same with `--no-deps` | 1 | + +**A blocker the filter did not select still lands in the store, deliberately.** +`--milestone X` can leave an issue from milestone Y on disk; `--label` can leave +an unlabelled one. It is there because a stored issue names it, not because it +matched. The exception is a closed blocker: closed is not a unit of work, filter +mode drops it like any other closed issue, and the `depends:` edge to it goes +with it — nothing is left pointing at a file that is not there. Key mode +(`pull.py 42`) has no such rule and stores it. Two traps this handles for you: @@ -251,7 +275,7 @@ accumulate them however many round trips it makes, and why a body that somehow gained two is cleaned on the next pull. `depends:` survives the same round trip through Gitea's native links (below): -push writes them, `pull.py --deps` reads them back, and the ledger turns the +push writes them, every `pull.py` reads them back, and the ledger turns the numbers into the slugs they had here. Before anything is sent, `/tea:issue`'s validator runs (exactly one `type/*`, @@ -271,7 +295,7 @@ The two directions are symmetric, and they use the same endpoint: | | direction | endpoint | |---|---|---| | `push.py` | `depends:` → native links | `POST …/issues/{n}/dependencies` | -| `pull.py --deps` | native links → `depends:` | `GET …/issues/{n}/dependencies` | +| `pull.py` (default; `--no-deps` off) | native links → `depends:` | `GET …/issues/{n}/dependencies` | The POST body is Gitea's `IssueMeta` — `{"index", "owner", "repo"}` naming the **blocker**, posted to the **blocked** issue's endpoint ("make the issue in the @@ -337,7 +361,7 @@ never check out, create, or write anything. | `labels` | `labels[]` | names both ways; ids only on write | | `assignees` | `assignees[]` | logins | | `milestone` | `milestone.title` | resolved to an id on write | -| `depends` | native links | slugs here, `IssueMeta` there; push writes them, `pull --deps` reads them | +| `depends` | native links | slugs here, `IssueMeta` there; push writes them, every pull reads them (`--no-deps` opts out) | | — | `ref` | lands in `branch:`; sent only when non-empty | | — | `number`, `html_url` | lands in `gitea:` / `url:` | diff --git a/skills/sync/scripts/pull.py b/skills/sync/scripts/pull.py index 98c3cfc..58ec80d 100644 --- a/skills/sync/scripts/pull.py +++ b/skills/sync/scripts/pull.py @@ -45,8 +45,10 @@ pages keep coming until the budget is full. Two boundaries keep that honest: issues therefore ends in a warning and a short answer, not in a walk of the whole tracker. Narrow the filter, or raise `--limit`, which raises the budget with it. -- `--deps` is outside the count: a dependency is followed because an issue - named it, not because the filter selected it. +- Dependencies are outside the count: a blocker is followed because a stored + issue named it, not because the filter selected it. `--limit 20` can + therefore leave more than 20 files behind — the budget counts the selection's + writes, and the graph is not part of the selection. `remote.py` is the deliberate exception, and it is not the same flag twice: it writes nothing at all, so there is no write to bound and its `--limit` means @@ -60,8 +62,39 @@ from an earlier pull is deleted. An absent file therefore means "no comments", never "not asked for". The thread is pull-only: editing it changes nothing in Gitea (post with comment.py). +**Dependencies come with every pull.** A pull answers with the whole unit of +work — the issue and what blocks it — so `depends:` is filled from Gitea's +native dependency graph and every blocker is pulled too, recursively, down to +`--depth` (default 3). That graph is the only source there is: `map.from_api` +writes slugs into the `## Depends on` prose and never `#N`, so an edge cannot be +recovered from the body. `--no-deps` turns off both halves — no `depends:`, no +recursion, and no request spent on either. `--deps` is still accepted and now +does nothing; it names what already happens. + +What it costs, stated rather than hidden: + +- **One request per issue that lands in the store** — `GET …/issues/{n}/dependencies`, + fetched once and used twice, since the same links both fill `depends:` and + tell the walk where to go next. A closed issue that filter mode drops costs + nothing: nothing was stored, so there is no unit of work to complete. +- **One request per blocker the selection did not already carry** — a `GET` for + the issue itself, then its own links, and so on until `--depth`. +- So `--milestone X` over 50 open issues is one list request + 50 link requests + + one pair for every blocker outside the milestone, where it used to be one + request flat. `--no-deps` is the way back to one. + +**In filter mode a blocker the filter did not select still lands in the store, +and that is deliberate.** `--milestone X` can leave an issue from milestone Y on +disk and `--label` an unlabelled one: a blocker is followed because a stored +issue names it, not because it matched. The one blocker that does not land is a +closed one — closed is not a unit of work, filter mode drops it the way it drops +any other closed issue, and the `depends:` edge to it goes with it, so nothing +points at a file that is not there. Key mode has no such rule and stores it. + Other flags: - --deps [--depth N] follow dependencies and pull them too + --no-deps do not fill depends:, do not follow blockers + --deps accepted, does nothing: it is the default now + --depth N how deep to follow blockers (default 3) --cached skip issues already on disk (body AND comments) --repo owner/repo default: auto-detect from the CWD git remote @@ -70,7 +103,9 @@ have not pushed are lost — with exactly one exception, checkbox state. A `[x]` on either side wins for any item whose text matches, because a tick is monotone and unioning the two sides is not conflict resolution (gmap.merge_checkbox_state has the rule and its price). `--cached` skips an issue before any of that: it is -not read and not merged. Draw the graph afterwards with the domain's own +not read and not merged — it still costs its one link request, because a cached +issue's blockers can be missing from disk even when it is not (`--cached +--no-deps` is the free one). Draw the graph afterwards with the domain's own issue_tree.py — it needs no network. Login: the operator's pin from .claude/settings.local.json (see /tea:auth). @@ -169,7 +204,15 @@ def main(): ap.add_argument("--limit", type=int, default=100, help="filter mode: how many issues to STORE, not to enumerate" " (default: 100)") - ap.add_argument("--deps", action="store_true", help="follow dependencies and pull them") + # Dependencies are the default: a pull answers with the unit of work, not + # one row of it. `--deps` stays accepted so the calls and command tables + # written against the old default keep working — it now sets what is + # already set. + ap.add_argument("--no-deps", dest="deps", action="store_false", + help="do not fill depends: and do not follow blockers") + ap.add_argument("--deps", dest="deps", action="store_true", + help="accepted, does nothing: dependencies are followed by default") + ap.set_defaults(deps=True) ap.add_argument("--depth", type=int, default=3, help="max dependency depth (default: 3)") ap.add_argument("--cached", action="store_true", help="skip issues already on disk instead of refetching") @@ -250,35 +293,42 @@ def main(): stored = os.path.isfile(issue.path_of(root, id)) # Closed and not already ours: nothing is written and nothing is asked - # of the server for it, not even its comments. The slug stays unclaimed - # too, so no other issue ends up pointing `depends:` at a missing file. + # of the server for it — not its comments, not its links, and its own + # blockers are not followed. The slug stays unclaimed too, so no other + # issue ends up pointing `depends:` at a missing file. if drop_closed and payload.get("state") == "closed" and not stored: dropped.append(number) + continue # not stored: no unit of work here, so no links are fetched + + store_ids.add(id) + number_of_id[number] = id + + # The native links, fetched ONCE for the two things they are for: + # filling this issue's `depends:` and telling the walk where to go next. + # One request per issue that lands in the store, and only one — the cost + # the docstring quotes is this line. + deps = _gitea.native_deps(login, base, number) if args.deps else [] + + if args.cached and stored: + skipped.append(id) # body and thread unread; only the links cost else: - store_ids.add(id) - number_of_id[number] = id - if args.cached and stored: - skipped.append(id) # untouched, unread, and not one request spent - else: - extra = _gitea.native_deps(login, base, number) if args.deps else [] - # The copy already on disk, as it was when this run started. It - # contributes its ticked checkboxes and nothing else; None when - # the store has never seen this issue. - prev = issues.get(id) - iss, unresolved = gmap.from_api(payload, id, repo, - id_for_number=number_of_id, - extra_numbers=extra, - synced=_gitea.now_iso(), - local_body=prev.body if prev else None) - issue.save(root, iss) - sync_comments(login, base, root, id, number, payload.get("comments") or 0) - remote_map[gmap.remote_key(repo, number)] = id - written.append(id) - pending.append((id, unresolved)) + # The copy already on disk, as it was when this run started. It + # contributes its ticked checkboxes and nothing else; None when + # the store has never seen this issue. + prev = issues.get(id) + iss, unresolved = gmap.from_api(payload, id, repo, + id_for_number=number_of_id, + extra_numbers=deps, + synced=_gitea.now_iso(), + local_body=prev.body if prev else None) + issue.save(root, iss) + sync_comments(login, base, root, id, number, payload.get("comments") or 0) + remote_map[gmap.remote_key(repo, number)] = id + written.append(id) + pending.append((id, unresolved)) if args.deps and depth < args.depth: - child_numbers = (gmap.numbers_in_body(payload.get("body") or "") - + _gitea.native_deps(login, base, number)) + child_numbers = gmap.numbers_in_body(payload.get("body") or "") + deps for n in child_numbers: if n in seen_numbers: continue @@ -307,8 +357,10 @@ def main(): # Compact output — the only thing that lands in the model's context. The # thread rides on the issue's own line; no file means no comments. + graph = False for id in sorted(set(written) | set(skipped)): iss = issue.load(root, id) + graph = graph or bool(iss.depends) note = " (cached)" if id in skipped else "" cpath = comments_path(root, id) if os.path.isfile(cpath): @@ -317,7 +369,9 @@ def main(): id, ", ".join(iss.labels) or "no labels", iss.title, iss.state, issue.path_of(root, id), note)) print("index: %s" % index_path) - if args.deps: + # Now that dependencies are the default, the hint is worth printing when + # there is something to draw, not on every run that could have drawn it. + if graph: print("graph: run issue_tree.py (offline) to draw it") diff --git a/skills/sync/scripts/push.py b/skills/sync/scripts/push.py index 0a41a36..3d40793 100644 --- a/skills/sync/scripts/push.py +++ b/skills/sync/scripts/push.py @@ -44,7 +44,7 @@ way, so nothing is lost, but the tracker shows no edge for it. The graph goes up with them. Once an issue has its number, every `depends:` entry that also has one becomes a **native Gitea link** — the same -`/dependencies` that `pull.py --deps` reads back, so the tracker shows the +`/dependencies` that every `pull.py` reads back, so the tracker shows the blocking panel and refuses to close a blocked issue first. Topological order means the blocker already has its number by then; no second pass is needed. `--update` links whatever appeared in `depends:` since the last push. A link diff --git a/tests/test_checkbox_merge.py b/tests/test_checkbox_merge.py index 93cadd1..9bac6b6 100644 --- a/tests/test_checkbox_merge.py +++ b/tests/test_checkbox_merge.py @@ -255,6 +255,11 @@ class FakeGitea(object): path, _, query = endpoint.partition("?") params = dict(urllib.parse.parse_qsl(query)) + # Every pull asks for an issue's native links now (dependencies are the + # default). Nothing here has any; the answer just has to exist. + if path.endswith("/dependencies"): + return [] + m = re.match(r"^%s/issues/(\d+)$" % re.escape(BASE), path) if m and method == "GET": return self.issues.get(int(m.group(1))) diff --git a/tests/test_pull_deps_default.py b/tests/test_pull_deps_default.py new file mode 100644 index 0000000..6cea16d --- /dev/null +++ b/tests/test_pull_deps_default.py @@ -0,0 +1,354 @@ +#!/usr/bin/env python3 +""" +A pull returns the unit of work: the issue AND what blocks it. + +`--deps` used to be opt-in, so `pull.py 42` wrote a file with an empty +`depends:` and `issue_tree.py` drew it as a root with no blockers. The edge was +not lost — it lives in Gitea's native dependency graph — but it was not asked +for, and the body cannot supply it: `map.from_api` writes slugs into the +`## Depends on` prose and never `#N`. Following the graph is now the default. + +What is asserted here: + +1. **The default fills the graph.** A bare `pull.py ` fills `depends:` and + pulls the blocker too, down to `--depth`. +2. **`--no-deps` is the way out, and it is free.** No `depends:`, no recursion, + and not one request beyond the issue itself. +3. **`--deps` still works and means nothing.** Calls written against the old + default keep running and get what they always got. +4. **The cost is one request per stored issue.** The native links are fetched + once and used twice — for `depends:` and for the walk. Never twice. +5. **Filter mode follows blockers out of the selection, deliberately.** A + blocker no filter selected still lands in the store and does not spend + `--limit`; a closed one is dropped like any other closed issue, and so is + the edge to it. An issue the filter dropped costs no link request at all. + +The transport is stubbed at `_gitea.api`, as the other suites do it, and the +stub records every call so "how many requests" is an observation. No network, +and no test touches the developer's store: each builds its own in a +`tempfile.TemporaryDirectory()`. +""" +import contextlib +import io +import os +import sys +import tempfile +import unittest +import urllib.parse +from unittest import mock + +_ROOT = os.path.dirname(os.path.dirname(os.path.abspath(__file__))) +for _p in (os.path.join(_ROOT, "skills", "sync", "scripts"), + os.path.join(_ROOT, "skills", "issue", "scripts")): + if _p not in sys.path: + sys.path.insert(0, _p) + +import _gitea # noqa: E402 +import issue # noqa: E402 +import pull # noqa: E402 + +REPO = "claude-skills/tea" +BASE = "repos/%s" % REPO + +BODY = """## Summary +Прозаическое описание задачи. + +## Spec +skills/issue/references/format.md + +## Acceptance criteria +- [ ] что-нибудь работает +""" + + +def payload(number, title, state="open"): + return {"number": number, "title": title, "body": BODY, "state": state, + "comments": 0, "labels": [{"name": "type/task"}], "assignees": [], + "milestone": None, "ref": "main", "updated_at": "2026-08-10T00:00:00Z", + "html_url": "https://git.example/%s/issues/%d" % (REPO, number), + "repository": {"full_name": REPO}} + + +class FakeTracker(object): + """`tea api` answered from memory, with a native dependency graph. + + `listed` is what the list endpoint serves — the filter's selection. `extra` + exists and is fetchable by number but is in no selection, which is how a + blocker outside the filter is modelled. `deps` maps a blocked issue's number + to the numbers that block it, the direction `GET …/dependencies` reads. + """ + + def __init__(self, listed=(), extra=(), deps=None): + self.listed = list(listed) + self.issues = {p["number"]: p for p in list(listed) + list(extra)} + self.deps = {int(k): list(v) for k, v in (deps or {}).items()} + self.calls = [] # (method, path), in request order + + # -- what the tests read off it ---------------------------------------- + + def paths(self, suffix): + return [p for m, p in self.calls if p.endswith(suffix)] + + def issue_gets(self): + """`GET …/issues/` — one issue fetched by number.""" + return [p for m, p in self.calls + if m == "GET" and p.startswith("%s/issues/" % BASE) + and p.rsplit("/", 1)[1].isdigit()] + + # -- the seam ---------------------------------------------------------- + + def api(self, login, endpoint, method="GET", payload=None, + payload_name=None, out_root=None, allow_fail=False): + path, _, qs = endpoint.partition("?") + q = urllib.parse.parse_qs(qs) + self.calls.append((method, path)) + + if path == "%s/issues" % BASE and method == "GET": + page, per = int(q["page"][0]), int(q["limit"][0]) + return self.listed[(page - 1) * per:(page - 1) * per + per] + + if path.endswith("/comments"): + return [] + + if path.endswith("/dependencies") and method == "GET": + n = int(path.split("/issues/")[1].split("/")[0]) + return [self.issues[b] for b in self.deps.get(n, []) if b in self.issues] + + if path.startswith("%s/issues/" % BASE) and method == "GET": + return self.issues.get(int(path.rsplit("/", 1)[1])) + + raise AssertionError("unstubbed call: %s %s" % (method, endpoint)) + + +class PullDepsTestCase(unittest.TestCase): + """A temp store, a fake tracker, no git and no network.""" + + def setUp(self): + self.tmp = tempfile.TemporaryDirectory(prefix="tea-deps-") + self.addCleanup(self.tmp.cleanup) + self.root = os.path.join(self.tmp.name, "tmp", "issues") + os.makedirs(self.root) + p = mock.patch.object(_gitea, "require_login", lambda: "test-login") + p.start() + self.addCleanup(p.stop) + + def serve(self, listed=(), extra=(), deps=None): + self.fake = FakeTracker(listed, extra, deps) + p = mock.patch.object(_gitea, "api", self.fake.api) + p.start() + self.addCleanup(p.stop) + return self.fake + + def blocked_pair(self): + """#10 "Second thing" is blocked by #7 "First thing".""" + return self.serve(listed=[payload(10, "Second thing"), + payload(7, "First thing")], + deps={10: [7]}) + + def run_pull(self, *argv): + out, err = io.StringIO(), io.StringIO() + args = ["pull.py", "--repo", REPO, "--out", self.root] + list(argv) + with mock.patch.object(sys, "argv", args), \ + contextlib.redirect_stdout(out), \ + contextlib.redirect_stderr(err): + pull.main() + return out.getvalue(), err.getvalue() + + def stored(self): + return sorted(issue.all_ids(self.root)) + + def depends_of(self, id): + return issue.load(self.root, id).depends + + +# -------------------------------------------------------------------------- +# 1. the default fills the graph +# -------------------------------------------------------------------------- + +class DepsAreTheDefaultTest(PullDepsTestCase): + + def test_a_bare_pull_fills_depends(self): + """The acceptance criterion, and the whole point: no flag, and the file + knows what blocks it.""" + self.blocked_pair() + self.run_pull("10") + self.assertEqual(self.depends_of("second-thing"), ["first-thing"]) + + def test_a_bare_pull_stores_the_blocker(self): + """`depends:` pointing at a file that is not there would be worse than + an empty one — the blocker comes with it.""" + self.blocked_pair() + self.run_pull("10") + self.assertIn("first-thing", self.stored()) + + def test_the_walk_is_recursive(self): + """A blocker's blocker is context too, down to --depth (default 3).""" + self.serve(listed=[payload(n, "Thing %d" % n) for n in range(1, 6)], + deps={1: [2], 2: [3], 3: [4], 4: [5]}) + self.run_pull("1") + self.assertEqual(self.stored(), ["thing-1", "thing-2", "thing-3", "thing-4"], + "the default depth of 3 was not what was walked") + + def test_depth_bounds_the_walk(self): + self.serve(listed=[payload(n, "Thing %d" % n) for n in range(1, 6)], + deps={1: [2], 2: [3], 3: [4], 4: [5]}) + self.run_pull("1", "--depth", "1") + self.assertEqual(self.stored(), ["thing-1", "thing-2"]) + + def test_the_graph_hint_is_printed_when_there_is_a_graph(self): + self.blocked_pair() + out, _ = self.run_pull("10") + self.assertIn("issue_tree.py", out) + + +# -------------------------------------------------------------------------- +# 2. --no-deps is the way out, and it is free +# -------------------------------------------------------------------------- + +class NoDepsOptsOutTest(PullDepsTestCase): + + def test_no_deps_leaves_depends_empty(self): + self.blocked_pair() + self.run_pull("10", "--no-deps") + self.assertEqual(self.depends_of("second-thing"), []) + + def test_no_deps_does_not_pull_the_blocker(self): + self.blocked_pair() + self.run_pull("10", "--no-deps") + self.assertEqual(self.stored(), ["second-thing"]) + + def test_no_deps_spends_no_extra_request(self): + """The other half of the criterion: not the links, not the blocker. + One issue asked for, one request made.""" + self.blocked_pair() + self.run_pull("10", "--no-deps") + self.assertEqual(self.fake.paths("/dependencies"), []) + self.assertEqual(self.fake.issue_gets(), ["%s/issues/10" % BASE]) + + def test_no_deps_prints_no_graph_hint(self): + self.blocked_pair() + out, _ = self.run_pull("10", "--no-deps") + self.assertNotIn("issue_tree.py", out) + + +# -------------------------------------------------------------------------- +# 3. --deps is still accepted, and means nothing +# -------------------------------------------------------------------------- + +class DepsFlagIsANoOpTest(PullDepsTestCase): + + def test_the_flag_is_still_accepted(self): + """Existing calls and the /tea:sync command tables must not break.""" + self.blocked_pair() + self.run_pull("10", "--deps") + self.assertEqual(self.depends_of("second-thing"), ["first-thing"]) + + def test_it_changes_nothing_about_the_run(self): + self.blocked_pair() + self.run_pull("10", "--deps") + with_flag = (self.stored(), self.depends_of("second-thing"), + list(self.fake.calls)) + + self.setUp() + self.blocked_pair() + self.run_pull("10") + self.assertEqual((self.stored(), self.depends_of("second-thing"), + list(self.fake.calls)), with_flag) + + +# -------------------------------------------------------------------------- +# 4. one request per stored issue +# -------------------------------------------------------------------------- + +class TheCostIsOneRequestPerIssueTest(PullDepsTestCase): + + def test_the_links_are_fetched_once_per_issue(self): + """They fill `depends:` AND steer the walk; fetching them twice is + double the price the docstring quotes.""" + self.blocked_pair() + self.run_pull("10") + self.assertEqual(self.fake.paths("/dependencies"), + ["%s/issues/10/dependencies" % BASE, + "%s/issues/7/dependencies" % BASE]) + + def test_a_bulk_pull_costs_one_per_issue(self): + """The number the docstring quotes: one list request, then one link + request per issue that lands in the store.""" + self.serve(listed=[payload(n, "Thing %d" % n) for n in range(1, 21)]) + self.run_pull("-q", "x") + self.assertEqual(len(self.fake.paths("/dependencies")), 20) + self.assertEqual(len(self.fake.paths("/issues")), 1) + + def test_a_cached_issue_costs_its_links_and_nothing_else(self): + """--cached stops the body and the thread, not the graph: a cached + issue's blockers can be missing from disk even when it is not.""" + self.blocked_pair() + self.run_pull("10", "--no-deps") # only #10 on disk + self.fake.calls = [] + self.run_pull("10", "--cached") + self.assertEqual(self.fake.paths("/dependencies"), + ["%s/issues/10/dependencies" % BASE, + "%s/issues/7/dependencies" % BASE]) + self.assertIn("first-thing", self.stored()) + + +# -------------------------------------------------------------------------- +# 5. filter mode follows blockers out of the selection +# -------------------------------------------------------------------------- + +class FilterModeFollowsOutwardTest(PullDepsTestCase): + + def test_a_blocker_outside_the_filter_lands_in_the_store(self): + """Documented as deliberate: a blocker is followed because a stored + issue named it, not because the filter selected it.""" + self.serve(listed=[payload(1, "Selected thing")], + extra=[payload(99, "Outside thing")], + deps={1: [99]}) + self.run_pull("-q", "x") + self.assertEqual(self.stored(), ["outside-thing", "selected-thing"]) + self.assertEqual(self.depends_of("selected-thing"), ["outside-thing"]) + + def test_a_blocker_does_not_spend_the_limit(self): + """--limit counts the selection's writes; the graph is not part of the + selection, so the store can legitimately hold more than N.""" + self.serve(listed=[payload(n, "Thing %d" % n) for n in range(1, 5)], + extra=[payload(100 + n, "Blocker %d" % n) for n in range(1, 5)], + deps={n: [100 + n] for n in range(1, 5)}) + self.run_pull("-q", "x", "--limit", "2") + self.assertEqual(self.stored(), + ["blocker-1", "blocker-2", "thing-1", "thing-2"]) + + def test_a_closed_blocker_is_dropped_with_the_edge_to_it(self): + """The documented exception. Closed is not a unit of work, so filter + mode drops it like any other closed issue — and `depends:` must not be + left pointing at a file that is not there.""" + self.serve(listed=[payload(1, "Selected thing")], + extra=[payload(99, "Closed blocker", state="closed")], + deps={1: [99]}) + self.run_pull("-q", "x") + self.assertEqual(self.stored(), ["selected-thing"]) + self.assertEqual(self.depends_of("selected-thing"), []) + + def test_a_closed_blocker_is_stored_in_key_mode(self): + """An address is not a bulk read: `pull.py 1` has no closed rule.""" + self.serve(listed=[payload(1, "Selected thing")], + extra=[payload(99, "Closed blocker", state="closed")], + deps={1: [99]}) + self.run_pull("1") + self.assertEqual(self.stored(), ["closed-blocker", "selected-thing"]) + + def test_a_dropped_closed_issue_costs_no_link_request(self): + """Nothing was stored for it, so there is no unit of work to complete + — and its own blockers are not dragged in behind it.""" + self.serve(listed=[payload(1, "Closed thing", state="closed"), + payload(2, "Open thing")], + extra=[payload(50, "Blocker of the closed one")], + deps={1: [50]}) + self.run_pull("-q", "x", "--state", "all") + self.assertEqual(self.fake.paths("/dependencies"), + ["%s/issues/2/dependencies" % BASE]) + self.assertEqual(self.stored(), ["open-thing"]) + + +if __name__ == "__main__": + unittest.main()