fix: apply --limit to the write, not to the selection #22

Merged
claude merged 1 commits from fix/limit-bounds-the-write into main 2026-08-10 13:21:55 +00:00
5 changed files with 488 additions and 21 deletions
+20 -2
View File
@@ -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 <key…>` or `pull.py --milestone M \| --label L \| -q TEXT` | Gitea → `tmp/issues/<id>.md`, plus `<id>.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 <key…>` or `pull.py --milestone M \| --label L \| -q TEXT [--limit N]` | Gitea → `tmp/issues/<id>.md`, plus `<id>.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 <id> --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/<id>.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
+65 -12
View File
@@ -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):
+49 -7
View File
@@ -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/<id>.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 = []
+5
View File
@@ -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
+349
View File
@@ -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()