From bea3735e47b45a7c7b13ba00022a5d321a883beb Mon Sep 17 00:00:00 2001 From: naudachu Date: Mon, 10 Aug 2026 17:23:12 +0500 Subject: [PATCH 1/5] 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 596cf853e81f5ed47fd7cd99835f1663156456f5 Mon Sep 17 00:00:00 2001 From: naudachu Date: Mon, 10 Aug 2026 17:25:33 +0500 Subject: [PATCH 2/5] fix: keep request payloads out of the issue store MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `labels.py` handed `_gitea.api` the issue store as a place to put the request file, and on a checkout without a store that quietly created `tmp/issues/.payload/`. Bootstrapping a repository's labels touches no issue at all, so the one rule the store has — nothing materializes it as a side effect of a write — was broken by an operation that has no business knowing the store exists. Where a request body goes was never the caller's decision to make. It is now the transport's: `tmp/payload/`, resolved from `_gitea.py`'s own location the way both domains resolve theirs, so every caller — sync and wiki alike — writes to one directory whatever it was invoked from, and `out_root` is gone from `api`, `add_dependency` and all six call sites. The directory is created by the first write of a run and not before: a `--dry-run` leaves nothing behind. `tmp/` is already gitignored. The name carries the distinction the old path lost. A store holds the only copy of something; this holds debris kept for a retry or a post-mortem, and deleting it costs nothing. A dotdir sitting among an issue's files claimed otherwise, and `ls tmp/issues` started lying about what existed. tests/test_payload_root.py runs the real `labels.py` in a throwaway repo against a fake `tea` on PATH: no store appears, the payloads land in tmp/payload/, a dry run writes nothing, and a run from a subdirectory still resolves to the repo root. Two source checks keep the callers from drifting apart again. Co-Authored-By: Claude Opus 5 (1M context) --- AGENTS.md | 21 ++- skills/sync/SKILL.md | 11 ++ skills/sync/scripts/_gitea.py | 78 ++++++++-- skills/sync/scripts/comment.py | 6 +- skills/sync/scripts/labels.py | 11 +- skills/sync/scripts/push.py | 8 +- skills/wiki/SKILL.md | 3 +- skills/wiki/scripts/wiki_push.py | 5 +- tests/test_drop_after_push.py | 2 +- tests/test_payload_root.py | 247 +++++++++++++++++++++++++++++++ tests/test_push_dependencies.py | 4 +- 11 files changed, 364 insertions(+), 32 deletions(-) create mode 100644 tests/test_payload_root.py diff --git a/AGENTS.md b/AGENTS.md index 7a42a2a..abd256f 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -64,8 +64,10 @@ If a tracker concept (issue number, login, HTTP call, label color, `sub_url`, - `skills/sync` — move issues between the local store and Gitea (`/tea:sync`) - `scripts/map.py` — md ↔ Gitea JSON, pure, no I/O; label colors live here - `scripts/_gitea.py` — transport: login pin, `tea api`, pagination, filters, - label ids, the remote-id map + label ids, the remote-id map, `tmp/payload/` - `scripts/pull.py`, `push.py`, `remote.py`, `comment.py` + - `scripts/labels.py` — put the canonical `type/*` and `severity/*` set into a + repository; reads the domain taxonomy, never the store - `skills/page` — a discussion's artifacts as a page tree (`/tea:page`), entirely offline - `references/pages.md` — canonical page-tree format; single source of truth @@ -183,3 +185,20 @@ organized. Same stance as the issue store, resolved the same way from disagree, because a page tree is worked on locally and an issue is not. - The `tea` CLI has no wiki subcommand. `tea api` is the only route, through `_gitea.py`. + +## Request payloads + +`tmp/payload/` (gitignored) holds the JSON bodies `tea api -d @file` was given, +one file per named request, kept after the call for a retry or a post-mortem. +It is **not a store and holds nobody's only copy** — deleting it costs nothing. + +- One directory for every caller — sync and wiki both — resolved from + `_gitea.py`'s own location, so which command wrote a body does not change + where it landed. `_gitea.api` takes no directory argument; that it once did + is exactly how a label bootstrap came to create `tmp/issues/`. +- It is created lazily, by the first write of a run, and only then: a `--dry-run` + or a run with nothing to send leaves no directory behind. +- **A scratchpad may never sit inside a store.** Store contents are the thing + being tracked; request bodies are debris of the transport. When the two share + a path, an operation that touches no issue at all still materializes the issue + store, and the operator's `ls tmp/issues` starts lying about what exists. diff --git a/skills/sync/SKILL.md b/skills/sync/SKILL.md index 25110e1..130202e 100644 --- a/skills/sync/SKILL.md +++ b/skills/sync/SKILL.md @@ -298,6 +298,10 @@ decision, not a migration. A color or `exclusive` that drifted is printed, and changed only under `--fix`. Running it twice creates nothing. `tech/*` and `comp/*` are open-ended by design and stay push-created. +Labels belong to the repository, not to any issue, so this one runs on a +checkout with no store and leaves it that way — nothing here reads `tmp/issues/` +and nothing creates it. The request bodies go to `tmp/payload/` (below). + A milestone must already exist in the repo — push attaches, it does not create. `branch:` is Gitea's `ref`, the branch the work actually lives on. Push fills @@ -354,6 +358,13 @@ precisely so the mechanism this section rules out is not needed. ## Rich payloads for everything else +Every body these scripts send is written to `/tmp/payload/.json` +first and passed as `-d @file`, then kept for a retry or a look at what actually +went up. One gitignored directory for all of them, chosen by the transport and +not by the caller. **It is not a store**: nothing in it is anybody's only copy, +and it is never `tmp/issues/` — a command that touches no issue must not leave +an issue store behind. + Comments and issues are wrapped by the scripts above. For **other** entities (pulls, releases, PATCHing something these scripts do not cover), entity subcommands like `tea pulls create` hang on a large or formatted body — an diff --git a/skills/sync/scripts/_gitea.py b/skills/sync/scripts/_gitea.py index 7d3783a..5325d1d 100644 --- a/skills/sync/scripts/_gitea.py +++ b/skills/sync/scripts/_gitea.py @@ -18,6 +18,13 @@ it is transport bookkeeping, not domain data — the domain never reads any of it, and losing the map still costs a re-pull and not information: the slug it records also travels in the issue body as `` (map.py), so a pull rebuilds the entry from the tracker. See `rebuild_map`. + +Request bodies go to tmp/payload/, which is this module's own scratchpad and +NOT a store: nothing in it is anybody's only copy, and writing one must never +materialize tmp/issues/ on a checkout that has none. Bootstrapping labels +touches no issue at all — it used to leave a store behind anyway, because the +request file had nowhere else to live. One directory, every caller, resolved +from this file the way the two domains resolve theirs. """ import datetime import json @@ -27,9 +34,26 @@ import re import sys import urllib.parse -PAYLOAD_DIR = ".payload" REMOTE_MAP = ".remote.json" +# -------------------------------------------------------------------------- +# where request bodies land +# -------------------------------------------------------------------------- +# Anchored on THIS FILE, like issue.store_root and page.store_root, so every +# caller — sync, wiki, whatever comes next — writes to one directory whatever +# it was invoked from. Visible and top-level under tmp/, not a dotdir hidden +# inside somebody's store, because a scratchpad that looks like store contents +# is how this went wrong the first time. `tmp/` is already gitignored. + +PAYLOAD_PARTS = ("tmp", "payload") + +# `.git` is a directory in a normal clone and a FILE in a worktree — hence +# exists(), not isdir(). AGENTS.md is the fallback for a plugin copied out of +# git; the agents-sync hook only ever puts one at a repository root. +REPO_MARKERS = (".git", "AGENTS.md") + +_HERE = os.path.dirname(os.path.abspath(__file__)) + def die(msg, code=1): sys.stderr.write("%s: %s\n" % (os.path.basename(sys.argv[0]), msg)) @@ -44,6 +68,35 @@ def now_iso(): return datetime.datetime.now(datetime.timezone.utc).strftime("%Y-%m-%dT%H:%M:%SZ") +def repo_root(start): + """Nearest ancestor of `start` (inclusive) carrying a repo marker, or None.""" + d = os.path.abspath(start) + while True: + if any(os.path.exists(os.path.join(d, m)) for m in REPO_MARKERS): + return d + parent = os.path.dirname(d) + if parent == d: + return None + d = parent + + +def payload_root(start=None): + """Absolute path of the request-body scratchpad. + + `start` overrides the anchor so the resolution can be exercised against a + scratch tree. Outside a repository, cwd gets a turn, then the cwd-relative + location stands — made absolute so an error can name the directory it + really wrote to.""" + for anchor in ([start] if start is not None else [_HERE, os.getcwd()]): + root = repo_root(anchor) + if root: + return os.path.join(root, *PAYLOAD_PARTS) + return os.path.abspath(os.path.join(*PAYLOAD_PARTS)) + + +PAYLOAD_ROOT = payload_root() + + # -------------------------------------------------------------------------- # login # -------------------------------------------------------------------------- @@ -80,19 +133,21 @@ def require_login(): # -------------------------------------------------------------------------- def api(login, endpoint, method="GET", payload=None, payload_name=None, - out_root=None, allow_fail=False): + allow_fail=False): """Call `tea api`; return parsed JSON (None on an empty body). - payload (a dict) is written to /.payload/.json and passed - as -d @file — the file survives the call for retries and debugging. - allow_fail returns None instead of exiting when the call fails.""" + payload (a dict) is written to PAYLOAD_ROOT/.json and passed as + -d @file — the file survives the call for retries and debugging. Where + that is, is not the caller's business and never was: the directory is + this layer's scratchpad, and the one time it was a caller's decision it + got pointed at the issue store. allow_fail returns None instead of + exiting when the call fails.""" cmd = ["tea", "api", "--login", login] if method != "GET": cmd += ["-X", method] if payload is not None: - pdir = os.path.join(out_root or ".", PAYLOAD_DIR) - os.makedirs(pdir, exist_ok=True) - path = os.path.join(pdir, "%s.json" % (payload_name or "request")) + os.makedirs(PAYLOAD_ROOT, exist_ok=True) + path = os.path.join(PAYLOAD_ROOT, "%s.json" % (payload_name or "request")) with open(path, "w") as f: json.dump(payload, f, ensure_ascii=False, indent=2) cmd += ["-d", "@" + path] @@ -245,7 +300,7 @@ def native_dep_pairs(login, base, number): return out -def add_dependency(login, base, number, dep_repo, dep_number, out_root=None): +def add_dependency(login, base, number, dep_repo, dep_number): """Make issue `number` depend on `dep_repo#dep_number`. True on success. Confirmed against the instance's own swagger.v1.json (Gitea 1.26.1): @@ -264,8 +319,7 @@ def add_dependency(login, base, number, dep_repo, dep_number, out_root=None): return False payload = {"index": int(dep_number), "owner": owner, "repo": name} got = api(login, "%s/issues/%d/dependencies" % (base, number), "POST", payload, - payload_name="dep-%d-%d" % (number, dep_number), - out_root=out_root, allow_fail=True) + payload_name="dep-%d-%d" % (number, dep_number), allow_fail=True) return got is not None @@ -297,7 +351,7 @@ def ensure_labels(login, base, specs, root): continue payload = dict(spec, name=name) created = api(login, "%s/labels" % base, "POST", payload, - payload_name="label-%s" % name.replace("/", "-"), out_root=root) + payload_name="label-%s" % name.replace("/", "-")) if not created or "id" not in created: die("could not create label %r" % name) cache[name] = created["id"] diff --git a/skills/sync/scripts/comment.py b/skills/sync/scripts/comment.py index 62f4f50..c7758b9 100644 --- a/skills/sync/scripts/comment.py +++ b/skills/sync/scripts/comment.py @@ -73,13 +73,11 @@ def main(): if args.edit: got = _gitea.api(login, "%s/issues/comments/%d" % (base, args.edit), "PATCH", - {"body": body}, payload_name="comment-%d" % args.edit, - out_root=root) + {"body": body}, payload_name="comment-%d" % args.edit) verb = "edited" else: got = _gitea.api(login, "%s/issues/%d/comments" % (base, number), "POST", - {"body": body}, payload_name="comment-%s" % args.id, - out_root=root) + {"body": body}, payload_name="comment-%s" % args.id) verb = "posted" if not isinstance(got, dict) or "id" not in got: _gitea.die("%s failed, unexpected response" % verb) diff --git a/skills/sync/scripts/labels.py b/skills/sync/scripts/labels.py index 6594e1b..9fe3c0d 100644 --- a/skills/sync/scripts/labels.py +++ b/skills/sync/scripts/labels.py @@ -32,6 +32,11 @@ created by push as they come up, and deleting or renaming anything at all. Only repository labels are read; an organization's own labels sit behind a different endpoint and are neither read nor written. +The issue store is out of scope too, and not incidentally. A label belongs to +the repository, not to any issue, so this command neither reads tmp/issues/ nor +creates it — the taxonomy it paints comes from the domain MODULE, and the +request bodies it sends go to the transport's own tmp/payload/. + Login: the operator's pin from .claude/settings.local.json (see /tea:auth). """ import argparse @@ -185,8 +190,7 @@ def main(): continue payload = dict(spec, name=name) new = _gitea.api(login, "%s/labels" % base, "POST", payload, - payload_name="label-%s" % name.replace("/", "-"), - out_root=issue.ISSUE_ROOT) + payload_name="label-%s" % name.replace("/", "-")) if not new or "id" not in new: _gitea.die("could not create label %r" % name) print("created %-20s id %-5s %s%s" % (name, new["id"], spec["color"], mark)) @@ -211,8 +215,7 @@ def main(): for field, _is, _want in drift: patch[field] = spec[field] _gitea.api(login, "%s/labels/%s" % (base, got.get("id")), "PATCH", patch, - payload_name="label-%s" % name.replace("/", "-"), - out_root=issue.ISSUE_ROOT) + payload_name="label-%s" % name.replace("/", "-")) fixed += 1 print("fixed %-20s id %-5s %s" % (name, got.get("id"), shown)) diff --git a/skills/sync/scripts/push.py b/skills/sync/scripts/push.py index 0a41a36..32288a7 100644 --- a/skills/sync/scripts/push.py +++ b/skills/sync/scripts/push.py @@ -334,12 +334,12 @@ def main(): if sent_number: payload = gmap.to_payload(iss, label_ids, ms_id, include_state=True) got = _gitea.api(login, "%s/issues/%d" % (base, sent_number), "PATCH", - payload, payload_name="issue-%s" % id, out_root=root) + payload, payload_name="issue-%s" % id) verb = "updated" else: payload = gmap.to_payload(iss, label_ids, ms_id) got = _gitea.api(login, "%s/issues" % base, "POST", payload, - payload_name="issue-%s" % id, out_root=root) + payload_name="issue-%s" % id) verb = "created" # The gate. Below this line the local file is going to be deleted, so # anything short of a confirmed write has to stop the run here. @@ -364,7 +364,7 @@ def main(): if missing: _gitea.api(login, "%s/issues/%d/labels" % (base, number), "PUT", {"labels": [label_ids[l] for l in iss.labels if l in label_ids]}, - payload_name="labels-%s" % id, out_root=root) + payload_name="labels-%s" % id) _gitea.warn("%s: labels re-applied via PUT (%s)" % (id, ", ".join(missing))) # The in-memory issue is stamped even though its file is going: the rest @@ -392,7 +392,7 @@ def main(): for slug, (drepo, dnum) in wanted_links: if not dnum or (drepo, dnum) in have: continue - if _gitea.add_dependency(login, base, number, drepo, dnum, root): + if _gitea.add_dependency(login, base, number, drepo, dnum): print(" depends on %s#%d (%s)" % (drepo, dnum, slug)) else: _gitea.warn("%s: could not link #%d -> %s#%d (%s) — link it by " diff --git a/skills/wiki/SKILL.md b/skills/wiki/SKILL.md index 31addf4..c358af5 100644 --- a/skills/wiki/SKILL.md +++ b/skills/wiki/SKILL.md @@ -11,7 +11,8 @@ ordering, paths, the index — belongs to `/tea:page` and is imported from there never redefined here. Transport is `tea api` through `skills/sync/scripts/_gitea.py`: the same login -pin, the same pagination, the same payload files. There is no second transport. +pin, the same pagination, the same payload files in the same `tmp/payload/`. +There is no second transport. ## The wiki is flat, and that is the whole design diff --git a/skills/wiki/scripts/wiki_push.py b/skills/wiki/scripts/wiki_push.py index 6fb085e..45fa280 100644 --- a/skills/wiki/scripts/wiki_push.py +++ b/skills/wiki/scripts/wiki_push.py @@ -115,13 +115,12 @@ def main(): if verb == "create": payload = wikimap.new_payload(title, text, a.message) got = _gitea.api(login, "%s/wiki/new" % base, method="POST", - payload=payload, payload_name="wiki-new", - out_root=space_dir) + payload=payload, payload_name="wiki-new") else: payload = wikimap.edit_payload(title, text, a.message) got = _gitea.api(login, wikimap.page_endpoint(base, e["sub_url"]), method="PATCH", payload=payload, - payload_name="wiki-edit", out_root=space_dir) + payload_name="wiki-edit") if not isinstance(got, dict) or not got.get("sub_url"): _gitea.warn("%s: no page returned; the manifest is unchanged for it" diff --git a/tests/test_drop_after_push.py b/tests/test_drop_after_push.py index a65a1af..9038480 100644 --- a/tests/test_drop_after_push.py +++ b/tests/test_drop_after_push.py @@ -124,7 +124,7 @@ class FakeTracker(object): # -- the seam ---------------------------------------------------------- def api(self, login, endpoint, method="GET", payload=None, - payload_name=None, out_root=None, allow_fail=False): + payload_name=None, allow_fail=False): self.calls.append((method, endpoint, payload)) path = endpoint.split("?")[0] diff --git a/tests/test_payload_root.py b/tests/test_payload_root.py new file mode 100644 index 0000000..1d44134 --- /dev/null +++ b/tests/test_payload_root.py @@ -0,0 +1,247 @@ +#!/usr/bin/env python3 +""" +Where request bodies land, and that writing one never conjures a store. + + python3 -m unittest discover -s tests -v + +Stdlib unittest, no third-party anything. The bug these tests pin down: +`labels.py --bootstrap` on a fresh checkout left `tmp/issues/.payload/` behind, +because the only place `_gitea.api` had to put a request file was whatever root +the caller handed it — and the label bootstrap, which touches no issue at all, +handed it the issue store. A store materialized as a side effect of an +operation that has nothing to do with issues. + +Every run here is against a throwaway repository with a FAKE `tea` first on +PATH, so nothing reaches the network and the developer's own store is never in +the blast radius. +""" +import json +import os +import shutil +import stat +import subprocess +import sys +import tempfile +import unittest + +REPO = os.path.dirname(os.path.dirname(os.path.abspath(__file__))) +ISSUE_SCRIPTS = os.path.join(REPO, "skills", "issue", "scripts") +SYNC_SCRIPTS = os.path.join(REPO, "skills", "sync", "scripts") +WIKI_SCRIPTS = os.path.join(REPO, "skills", "wiki", "scripts") + +sys.path.insert(0, SYNC_SCRIPTS) +sys.path.insert(0, ISSUE_SCRIPTS) +import _gitea # noqa: E402 +import issue # noqa: E402 + + +# A `tea` that answers without a network: an empty list for every GET (so the +# repository looks like it has no labels yet) and a created object for every +# write. It also records its own argv, which is how a test can tell that the +# payload file the script wrote is the one the call actually referenced. +FAKE_TEA = '''#!%s +import json, os, sys +with open(os.path.join(os.environ["TEA_CALL_LOG"], "calls.txt"), "a") as f: + f.write("\\t".join(sys.argv[1:]) + "\\n") +sys.stdout.write(json.dumps({"id": 1, "name": "created", "sub_url": "Page"}) + if "-X" in sys.argv else "[]") +''' + + +class FakeRepo(object): + """A self-contained repository with no store and no tmp/ at all.""" + + def __init__(self): + self._tmp = tempfile.TemporaryDirectory() + # realpath: on macOS $TMPDIR is a symlink, and a child reporting its + # own cwd would otherwise disagree with the path we handed it. + self.root = os.path.realpath(self._tmp.name) + + os.makedirs(os.path.join(self.root, ".git")) # the repo marker + skip = shutil.ignore_patterns("__pycache__") + shutil.copytree(ISSUE_SCRIPTS, self.path("skills", "issue", "scripts"), ignore=skip) + shutil.copytree(SYNC_SCRIPTS, self.path("skills", "sync", "scripts"), ignore=skip) + os.makedirs(self.path("sub", "deeper")) + + # the login pin the transport insists on, local to this fixture + os.makedirs(self.path(".claude")) + with open(self.path(".claude", "settings.local.json"), "w") as f: + json.dump({"env": {"GITEA_LOGIN": "fixture/user"}}, f) + + self.bin = self.path("fakebin") + os.makedirs(self.bin) + tea = os.path.join(self.bin, "tea") + with open(tea, "w") as f: + f.write(FAKE_TEA % sys.executable) + os.chmod(tea, os.stat(tea).st_mode | stat.S_IEXEC | stat.S_IXGRP | stat.S_IXOTH) + + def cleanup(self): + self._tmp.cleanup() + + def path(self, *parts): + return os.path.join(self.root, *parts) + + @property + def store(self): + return self.path("tmp", "issues") + + @property + def payloads(self): + return self.path("tmp", "payload") + + def script(self, layer, name): + return self.path("skills", layer, "scripts", name) + + def run(self, script, *args, **kw): + env = dict(os.environ) + env.pop("PYTHONPATH", None) # no leakage from the harness into the child + env["PATH"] = self.bin + os.pathsep + env["PATH"] + env["TEA_CALL_LOG"] = self.root + p = subprocess.run([sys.executable, script] + list(args), + cwd=kw.pop("cwd", self.root), env=env, + capture_output=True, text=True) + return p.returncode, p.stdout, p.stderr + + def calls(self): + p = os.path.join(self.root, "calls.txt") + if not os.path.isfile(p): + return [] + with open(p) as f: + return [line.rstrip("\n").split("\t") for line in f if line.strip()] + + +# -------------------------------------------------------------------------- +# resolution +# -------------------------------------------------------------------------- + +class TestPayloadRoot(unittest.TestCase): + + def test_root_is_absolute_and_repo_anchored(self): + self.assertTrue(os.path.isabs(_gitea.PAYLOAD_ROOT), _gitea.PAYLOAD_ROOT) + self.assertEqual(_gitea.PAYLOAD_ROOT, os.path.join(REPO, "tmp", "payload")) + + def test_it_is_not_the_issue_store_and_not_inside_one(self): + """The acceptance criterion, as a path fact: a request body is not + store content, so it may not live in a store or under one.""" + self.assertNotEqual(_gitea.PAYLOAD_ROOT, issue.ISSUE_ROOT) + self.assertFalse(_gitea.PAYLOAD_ROOT.startswith(issue.ISSUE_ROOT + os.sep)) + self.assertFalse(issue.ISSUE_ROOT.startswith(_gitea.PAYLOAD_ROOT + os.sep)) + + def test_the_name_says_what_it_holds(self): + """Named so the distinction is visible: a top-level directory called + `payload`, not a dotdir hiding among an issue's files.""" + self.assertEqual(os.path.basename(_gitea.PAYLOAD_ROOT), "payload") + self.assertFalse(os.path.basename(_gitea.PAYLOAD_ROOT).startswith(".")) + + def test_gitignore_covers_it(self): + with open(os.path.join(REPO, ".gitignore")) as f: + ignored = {line.strip() for line in f} + self.assertEqual(_gitea.PAYLOAD_PARTS[0], "tmp") + self.assertIn("tmp/", ignored, + "the payload directory is not covered by .gitignore") + + def test_resolution_is_anchored_on_the_module_not_on_cwd(self): + repo = FakeRepo() + self.addCleanup(repo.cleanup) + self.assertEqual(_gitea.payload_root(repo.path("sub", "deeper")), + repo.payloads) + + +# -------------------------------------------------------------------------- +# the bug: a label bootstrap that materialized the store +# -------------------------------------------------------------------------- + +class TestLabelsTouchesNoStore(unittest.TestCase): + + def setUp(self): + self.repo = FakeRepo() + self.addCleanup(self.repo.cleanup) + + def bootstrap(self, *args, **kw): + rc, out, err = self.repo.run(self.repo.script("sync", "labels.py"), + "--repo", "fixture/repo", *args, **kw) + self.assertEqual(rc, 0, "labels.py failed:\n%s%s" % (out, err)) + return out, err + + def test_bootstrap_creates_no_store(self): + """The reproduction from the report, run for real: no tmp/issues, and + no complaint about one either.""" + out, _ = self.bootstrap() + self.assertIn("created", out) + self.assertFalse(os.path.exists(self.repo.store), + "labels.py created the issue store") + + def test_bootstrap_writes_its_payloads_to_the_payload_root(self): + self.bootstrap() + self.assertTrue(os.path.isdir(self.repo.payloads), + "no payload directory: %s" % self.repo.payloads) + written = os.listdir(self.repo.payloads) + self.assertIn("label-type-bug.json", written) + for name in written: + self.assertTrue(name.startswith("label-"), name) + + # and the file named on the command line is the one that was written + sent = [a[a.index("-d") + 1][1:] for a in self.repo.calls() if "-d" in a] + self.assertTrue(sent) + for path in sent: + self.assertEqual(os.path.dirname(path), self.repo.payloads) + self.assertTrue(os.path.isfile(path), path) + + def test_the_payload_is_the_request_body(self): + self.bootstrap() + with open(os.path.join(self.repo.payloads, "label-type-bug.json")) as f: + body = json.load(f) + self.assertEqual(body.get("name"), "type/bug") + self.assertTrue(body.get("color")) + + def test_a_dry_run_writes_nothing_at_all(self): + out, _ = self.bootstrap("--dry-run") + self.assertIn("nothing was written", out) + self.assertFalse(os.path.exists(self.repo.path("tmp")), + "a dry run left something behind in tmp/") + + def test_the_directory_does_not_follow_cwd(self): + """Run from a subdirectory: still one payload root, at the repo root. + A cwd-relative directory is how the store ended up with a second copy + of itself, and this one is resolved the same way to avoid the same + class of bug.""" + self.bootstrap(cwd=self.repo.path("sub", "deeper")) + self.assertTrue(os.path.isdir(self.repo.payloads)) + self.assertFalse(os.path.exists(self.repo.path("sub", "deeper", "tmp"))) + self.assertFalse(os.path.exists(self.repo.store)) + + +# -------------------------------------------------------------------------- +# one place, every caller +# -------------------------------------------------------------------------- + +class TestOnePlaceForEveryCaller(unittest.TestCase): + + def hits(self, needle, skip_transport=False): + """Every `layer/script.py:line` mentioning `needle`.""" + out = [] + for d in (SYNC_SCRIPTS, WIKI_SCRIPTS): + layer = os.path.basename(os.path.dirname(d)) + for name in sorted(os.listdir(d)): + if not name.endswith(".py") or (skip_transport and name == "_gitea.py"): + continue + with open(os.path.join(d, name)) as f: + for n, line in enumerate(f, 1): + if needle in line: + out.append("%s/%s:%d" % (layer, name, n)) + return out + + def test_no_caller_chooses_where_its_payload_goes(self): + """Whatever the answer is, it has to be the same for all of them — + payload files scattered across two stores and a wiki space is the + state this replaced.""" + self.assertEqual(self.hits("out_root"), [], + "a caller still picks a payload directory of its own") + + def test_only_the_transport_names_the_directory(self): + self.assertEqual(self.hits("PAYLOAD", skip_transport=True), [], + "the payload directory is named outside the transport") + + +if __name__ == "__main__": + unittest.main() diff --git a/tests/test_push_dependencies.py b/tests/test_push_dependencies.py index 0c3eb04..9fc6de4 100644 --- a/tests/test_push_dependencies.py +++ b/tests/test_push_dependencies.py @@ -97,7 +97,7 @@ class FakeGitea(object): # -- the seam ---------------------------------------------------------- def api(self, login, endpoint, method="GET", payload=None, - payload_name=None, out_root=None, allow_fail=False): + payload_name=None, allow_fail=False): self.calls.append((method, endpoint, payload)) path = endpoint.split("?")[0] @@ -218,7 +218,7 @@ class AddDependencyTest(unittest.TestCase): return {"number": 102} with mock.patch.object(_gitea, "api", fake_api): - ok = _gitea.add_dependency("l", BASE, 102, REPO, 101, out_root=None) + ok = _gitea.add_dependency("l", BASE, 102, REPO, 101) self.assertTrue(ok) method, endpoint, payload = calls[0] From 627df76812b5872d1daa1fec867374ef1b67c7d1 Mon Sep 17 00:00:00 2001 From: naudachu Date: Mon, 10 Aug 2026 17:32:35 +0500 Subject: [PATCH 3/5] test: keep the payload root out of the developer's tree MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit One test stubs the transport a layer below `api()` — at `subprocess`, to exercise the path a 422 really takes — so it reaches the real payload write. That used to land in the test's own temp store, because the caller named the directory; now the directory is `_gitea.PAYLOAD_ROOT`, resolved from the module's location, and the file appeared in the developer's `tmp/payload/`. `StoreTestCase` patches `PAYLOAD_ROOT` to its fixture alongside the other seams, and the rule in AGENTS.md gains the third directory a test must not write to. Co-Authored-By: Claude Opus 5 (1M context) --- AGENTS.md | 18 ++++++++++++------ tests/test_drop_after_push.py | 6 ++++++ 2 files changed, 18 insertions(+), 6 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index abd256f..519e867 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -106,12 +106,18 @@ stdlib-only and the tests hold the same line. `skills/*/scripts/` are not packages, so a test that needs the domain module imports it with `sys.path.insert`. -**A test never touches `tmp/issues/` or `tmp/wiki/`.** Anything that needs a -store builds a throwaway repository in a `tempfile.TemporaryDirectory()` — a -`.git` marker, a copy of the script layers, fixture issues or artifacts — and -runs the real scripts inside it as subprocesses. That is the only way to test -behavior that depends on where a script is run from, and it keeps the -developer's own store out of the blast radius. +**A test never touches `tmp/issues/`, `tmp/wiki/` or `tmp/payload/`.** Anything +that needs a store builds a throwaway repository in a +`tempfile.TemporaryDirectory()` — a `.git` marker, a copy of the script layers, +fixture issues or artifacts — and runs the real scripts inside it as +subprocesses. That is the only way to test behavior that depends on where a +script is run from, and it keeps the developer's own store out of the blast +radius. + +`tmp/payload/` is in that list because `_gitea.PAYLOAD_ROOT` is resolved once, +from the module's own location: a test that stubs the transport *below* `api()` +— at `subprocess`, to exercise a non-2xx — reaches the real write. Such a test +patches `PAYLOAD_ROOT` to its own temp directory too. ## Local issue store diff --git a/tests/test_drop_after_push.py b/tests/test_drop_after_push.py index 9038480..a581982 100644 --- a/tests/test_drop_after_push.py +++ b/tests/test_drop_after_push.py @@ -194,7 +194,13 @@ class StoreTestCase(unittest.TestCase): def setUp(self): self.root = tempfile.mkdtemp(prefix="tea-drop-") self.fake = FakeTracker() + # PAYLOAD_ROOT is the repo's own tmp/payload, and a test that stubs the + # transport one layer down (see the non-2xx case) reaches the real + # write. Point it at the fixture: a test writes in its temp directory + # and nowhere else. for p in (mock.patch.object(_gitea, "api", self.fake.api), + mock.patch.object(_gitea, "PAYLOAD_ROOT", + os.path.join(self.root, "payload")), mock.patch.object(_gitea, "require_login", lambda: "test-login"), mock.patch.object(push, "git_branch", lambda: "test-branch")): p.start() From a2e9a88186a39639c11d9e1d670556e4fdc77ada Mon Sep 17 00:00:00 2001 From: naudachu Date: Mon, 10 Aug 2026 17:41:27 +0500 Subject: [PATCH 4/5] 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() From 1d7abc11ae1704d418834458b8a08dc756d09d45 Mon Sep 17 00:00:00 2001 From: naudachu Date: Mon, 10 Aug 2026 18:12:04 +0500 Subject: [PATCH 5/5] fix: resolve the login pin from a git worktree MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `_gitea.require_login` walked up from CWD and nowhere else. A worktree is a sibling of the main checkout, not a descendant, and `settings.local.json` is untracked — so the pin lives in the main checkout only, is not on the worktree's parent chain, and the whole tracker half of the plugin died there with "no login pinned". In the same directory the guard resolved it fine, because it had a search of its own: one order, written twice, disagreeing. It is written once now, in skills/auth/scripts/pin.py, and both callers import it — the transport and hooks/tea-guard.sh. $CLAUDE_PROJECT_DIR, then a hint the caller supplies (the hook passes its payload's cwd), then the current directory; each searched up its parent chain, and only if that finds nothing, across into the main working tree of a linked worktree met on the way, reached by reading `gitdir:` out of the `.git` FILE and following `commondir`. No subprocess — a PreToolUse hook runs before every Bash call and must not fork to answer this. The search still starts at the working directory and never at `__file__`, deliberately asymmetric with `issue.store_root` and `_gitea.PAYLOAD_ROOT`. Where an installation keeps its files is a fact about the installation; whose login a project runs under is a fact about the project, and a plugin pointed at somebody else's tree must not answer that from its own directory. pin.py says so in as many words, so the next reader does not "fix" the inconsistency. Two consequences fall out of it. `/tea:auth` no longer has any reason to run inside a worktree, so no second pin lands in a directory that is deleted with the branch — the skill now says to write it beside the common `.git`. And the scripts can run where the work is: the workaround the bug forced, cwd in the main checkout, made push.py send that checkout's branch as `ref`, which is the one thing `branch:` exists to record. tests/test_login_pin.py holds both halves: the hop against a hand-built layout and against a real `git worktree add`, a run from the worktree finding the login, no pin anywhere still erroring, the scripts' own directory not becoming a source, `ref` coming out as the worktree's branch, and the hook and a script answering the same directory alike. Two mechanical checks keep the callers from growing a second copy of the walk. Three existing fixtures now copy skills/auth/scripts, which the transport imports. Refs #24. Co-Authored-By: Claude Opus 5 (1M context) --- AGENTS.md | 50 +++- README.md | 6 +- hooks/tea-guard.sh | 59 +++-- skills/auth/SKILL.md | 30 ++- skills/auth/scripts/pin.py | 204 +++++++++++++++++ skills/sync/SKILL.md | 15 +- skills/sync/scripts/_gitea.py | 47 ++-- tests/test_login_pin.py | 418 ++++++++++++++++++++++++++++++++++ tests/test_page_tree.py | 7 +- tests/test_payload_root.py | 3 + tests/test_store_path.py | 3 + 11 files changed, 774 insertions(+), 68 deletions(-) create mode 100644 skills/auth/scripts/pin.py create mode 100644 tests/test_login_pin.py diff --git a/AGENTS.md b/AGENTS.md index 519e867..b5bfb68 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -24,11 +24,17 @@ skills/page DOMAIN what a page tree is: title <-> path, order, the index ▲ offline — no tracker, no network, stdlib imports only │ imports skills/sync BRIDGE map.py md <-> Gitea issue JSON, pure, no I/O - _gitea.py login pin, tea api, pagination, filters + _gitea.py tea api, pagination, filters, payloads skills/wiki BRIDGE wikimap.py md <-> Gitea wiki JSON, pure, no I/O transport is _gitea.py — there is no second one -skills/use REFERENCE tea CLI docs for everything that is not an issue + │ imports + ▼ skills/auth IDENTITY pin the login the whole tracker side runs under + ▲ pin.py where the pin is and how it is found — + │ imports imported by _gitea.py AND by hooks/tea-guard.sh +hooks/tea-guard so `tea` and the scripts cannot disagree + +skills/use REFERENCE tea CLI docs for everything that is not an issue ▲ │ calls agents/ EXECUTION tea-runner: runs the scripts, reports a receipt @@ -51,6 +57,8 @@ If a tracker concept (issue number, login, HTTP call, label color, `sub_url`, ## Repo layout - `skills/auth` — pin the Gitea login used by `tea` (`/tea:auth`) + - `scripts/pin.py` — the one written copy of the pin's location and search + order (see "The login pin" below); stdlib, no subprocess, no network - `skills/issue` — issues as units of work (`/tea:issue`), entirely offline - `references/format.md` — canonical issue format; single source of truth - `scripts/issue.py` — domain module: slug identity, parse/render, validation, @@ -63,8 +71,8 @@ If a tracker concept (issue number, login, HTTP call, label color, `sub_url`, - `scripts/issue_index.py` — rebuild `tmp/issues/INDEX.md` - `skills/sync` — move issues between the local store and Gitea (`/tea:sync`) - `scripts/map.py` — md ↔ Gitea JSON, pure, no I/O; label colors live here - - `scripts/_gitea.py` — transport: login pin, `tea api`, pagination, filters, - label ids, the remote-id map, `tmp/payload/` + - `scripts/_gitea.py` — transport: `tea api`, pagination, filters, label ids, + the remote-id map, `tmp/payload/`; the login comes from `auth/pin.py` - `scripts/pull.py`, `push.py`, `remote.py`, `comment.py` - `scripts/labels.py` — put the canonical `type/*` and `severity/*` set into a repository; reads the domain taxonomy, never the store @@ -91,10 +99,40 @@ If a tracker concept (issue number, login, HTTP call, label color, `sub_url`, Delegating a single call costs more than running it inline — the win is the loop, the retry, and the error triage. - `hooks/` — PreToolUse hooks: `tea-guard` blocks or rewrites `tea` invocations - that don't use the pinned login; `agents-sync` keeps every directory canonical - (`AGENTS.md` real file, `CLAUDE.md` symlink to it) + that don't use the pinned login (resolving it through `auth/pin.py`); + `agents-sync` keeps every directory canonical (`AGENTS.md` real file, + `CLAUDE.md` symlink to it) - `tests/` — stdlib `unittest`, no third-party anything +## The login pin + +`/.claude/settings.local.json` → `env.GITEA_LOGIN`, written by +`/tea:auth` and read at call time. **The search order is written once, in +`skills/auth/scripts/pin.py`**, and both callers import it: the transport +(`_gitea.require_login`) and the `tea-guard` hook. Neither spells the path or +the walk itself, and a test asserts they don't. + +Start directories, first hit wins: `$CLAUDE_PROJECT_DIR`, then a hint the +caller supplies (the hook passes the Bash payload's `cwd`; a script passes +nothing), then the current directory. Each one is searched up its parent chain, +and then — only if that found nothing — up the parent chain of the **main +working tree of any linked worktree** met on the way, reached by reading +`gitdir:` out of a `.git` *file* and following `commondir`. + +**The pin is not resolved from `__file__`, and that asymmetry with +`issue.store_root`/`page.store_root`/`_gitea.PAYLOAD_ROOT` is deliberate.** +Where an installation keeps its files is a fact about the installation; whose +login a project runs under is a fact about the project. A plugin installed +outside any repository and pointed at somebody else's tree must not answer the +second question from its own directory. So the search runs from the working +directory upward — and reaches a worktree's main checkout by asking git. + +Two failures this replaces, both worth remembering: a git worktree is a +*sibling* of the main checkout, so the untracked pin is not on its parent chain +and the whole sync layer died there while `tea` in the same directory worked; +and the cure it invited — `/tea:auth` inside the worktree — writes a second +settings file into a directory that is deleted with the worktree. + ## Tests ```bash diff --git a/README.md b/README.md index b913bda..146ba69 100644 --- a/README.md +++ b/README.md @@ -68,7 +68,9 @@ The skills (`/tea:auth`, `/tea:issue`, `/tea:sync`, `/tea:use`) and the `tea-gua ## First use -Run `/tea:auth` once per project. Claude will list your available Gitea logins and ask you to pick one. The choice is written to `.claude/settings.local.json` and takes effect immediately — no restart needed. +Run `/tea:auth` once per project. Claude will list your available Gitea logins and ask you to pick one. The choice is written to the project root's `.claude/settings.local.json` and takes effect immediately — no restart needed. + +Once per *project*, not once per checkout: a `git worktree` shares its main checkout's pin. Both the hook and the scripts find it from inside a worktree, so don't run `/tea:auth` there — it would leave a second pin in a directory that disappears with the branch. ``` /tea:auth @@ -80,7 +82,7 @@ right skill automatically. `/tea:auth` is only needed for the tracker side; ## How the login guard works -Every `tea` invocation Claude writes must carry the literal placeholder `--login "$GITEA_LOGIN"`. The `tea-guard` hook intercepts the Bash call before it runs, looks up the pinned login from `.claude/settings.local.json`, and rewrites the command to use it. +Every `tea` invocation Claude writes must carry the literal placeholder `--login "$GITEA_LOGIN"`. The `tea-guard` hook intercepts the Bash call before it runs, looks up the pinned login from `.claude/settings.local.json`, and rewrites the command to use it. The hook and the scripts look it up the same way — one search order, in `skills/auth/scripts/pin.py`. Claude is **blocked** from: - running `tea` without `--login` at all diff --git a/hooks/tea-guard.sh b/hooks/tea-guard.sh index 1943383..7c490d9 100755 --- a/hooks/tea-guard.sh +++ b/hooks/tea-guard.sh @@ -12,7 +12,12 @@ checking it: The pin is read from .claude/settings.local.json (env.GITEA_LOGIN) at call time — from the FILE, not the environment — so a freshly pinned login works in -the same session with no restart. +the same session with no restart. WHERE that file is looked for is not decided +here: skills/auth/scripts/pin.py holds the search order, and the sync and wiki +scripts resolve the pin through the same module. One order, one copy of it. The +guard and the scripts disagreeing about a directory is a bug by construction, +and was one: in a git worktree `tea` worked and every script said "no login +pinned". Rules: - not a `tea` command ............................. allow (passthrough) @@ -27,6 +32,18 @@ rewrite; exit 2 + stderr to block. """ import sys, os, re, json, shlex +# The identity layer, reached by the plugin's own layout — the one thing a hook +# may assume about where it lives. Import failure is not fatal on its own: a +# command that is not `tea` still passes through untouched (see main), and only +# a command that needs a login is blocked. +sys.path.append(os.path.abspath(os.path.join( + os.path.dirname(os.path.abspath(__file__)), + os.pardir, "skills", "auth", "scripts"))) +try: + import pin +except Exception: + pin = None + PLACEHOLDERS = {"$GITEA_LOGIN", "${GITEA_LOGIN}"} @@ -53,30 +70,6 @@ def rewrite(tool_input, new_cmd, note): sys.exit(0) -def find_pin(start_dir): - """Walk up from start_dir; return (login, path) from the first - .claude/settings.local.json that carries a non-empty env.GITEA_LOGIN.""" - try: - d = os.path.abspath(start_dir or ".") - except Exception: - return None, None - while True: - p = os.path.join(d, ".claude", "settings.local.json") - if os.path.isfile(p): - try: - with open(p) as f: - data = json.load(f) - v = (data.get("env") or {}).get("GITEA_LOGIN") - if isinstance(v, str) and v.strip(): - return v.strip(), p - except Exception: - pass - parent = os.path.dirname(d) - if parent == d: - return None, None - d = parent - - def main(): try: payload = json.load(sys.stdin) @@ -117,16 +110,22 @@ def main(): 'the operator pinned via /tea:auth. This prevents acting under ' 'the wrong identity.' % raw_val) - start = os.environ.get("CLAUDE_PROJECT_DIR") or payload.get("cwd") or os.getcwd() - pin, src = find_pin(start) - if not pin: + if pin is None: + block('cannot import skills/auth/scripts/pin.py, so the pinned login ' + 'cannot be resolved. The plugin tree is incomplete; reinstall it.') + + # The hint is the directory the Bash command will run in; the rest of the + # order (CLAUDE_PROJECT_DIR first, cwd last, and the worktree branch of the + # search) is pin.py's, and is the same order the scripts get. + login, src = pin.find_pin(payload.get("cwd")) + if not login: block('no login is pinned. Run /tea:auth to choose one (writes ' '.claude/settings.local.json env.GITEA_LOGIN). The guard reads ' 'the file at call time, so it takes effect with no restart.') - new_cmd = cmd[:m.start(3)] + shlex.quote(pin) + cmd[m.end(3):] + new_cmd = cmd[:m.start(3)] + shlex.quote(login) + cmd[m.end(3):] rewrite(tool_input, new_cmd, - 'tea-guard: resolved --login -> %s (pinned in %s)' % (pin, src)) + 'tea-guard: resolved --login -> %s (pinned in %s)' % (login, src)) if __name__ == "__main__": diff --git a/skills/auth/SKILL.md b/skills/auth/SKILL.md index c861057..a0223de 100644 --- a/skills/auth/SKILL.md +++ b/skills/auth/SKILL.md @@ -32,13 +32,37 @@ So: 3. **One login:** propose it; confirm before writing. 4. **Several logins:** `AskUserQuestion` with each login's `name`, `user`, and `url` so the operator's choice is unambiguous. Never decide for them. -5. Merge the chosen name into `.claude/settings.local.json` under `env` - (do not clobber other keys): +5. Merge the chosen name into the **project root's** + `.claude/settings.local.json` under `env` (do not clobber other keys): ```json { "env": { "GITEA_LOGIN": "" } } ``` + **In a git worktree, write it to the main checkout, never to the worktree.** + A worktree is deleted when the branch is done, taking a pin written into it + with it, and one repository with two pins is one repository with two + identities. Both the guard and the scripts already reach the main checkout's + pin from inside any worktree — so there is nothing to pin a second time. + `git rev-parse --path-format=absolute --git-common-dir` names the `.git` to + write beside. 6. Done — it is live. The guard resolves the pin from the file on the next - `tea` call; no restart needed. Tell the operator which login is now pinned. + `tea` call; no restart needed. Tell the operator which login is now pinned, + and which file it went in. + +## Where the pin is looked for + +One search order, written once in `scripts/pin.py` and imported by both the +`tea-guard` hook and the sync/wiki transport — they cannot disagree about a +directory, and a test asserts neither keeps a copy of the walk. + +`$CLAUDE_PROJECT_DIR`, then the caller's hint (the hook passes the Bash call's +`cwd`), then the current directory. Each is searched up its parent chain; only +if that finds nothing does the search cross into the main working tree of a +linked worktree, via `gitdir:` in the `.git` file. The plugin's own directory +is never a source — a plugin pointed at somebody else's project must take the +identity from that project, not from where it happens to be installed. + +If a script reports "no login pinned", that is the honest answer: nothing was +found anywhere on that order. Pin one — at the project root. ## Identity-safety rules diff --git a/skills/auth/scripts/pin.py b/skills/auth/scripts/pin.py new file mode 100644 index 0000000..9165b38 --- /dev/null +++ b/skills/auth/scripts/pin.py @@ -0,0 +1,204 @@ +#!/usr/bin/env python3 +""" +pin.py — where the operator's Gitea login pin is, and how it is found. + +**The search order lives here and nowhere else.** The `tea-guard` hook imports +this module; so does the transport every sync and wiki script runs on. Two +copies of the order is exactly how a git worktree came to have a working hook +and a dead transport in the same directory: `tea` resolved the login, the +scripts said "no login pinned", and the error told the operator to pin what was +already pinned. + +Not a command — a lookup. Stdlib only, no subprocess, no network: a PreToolUse +hook runs before every Bash call and must not fork a process to answer this. + +The pin is a file the OPERATOR owns and `/tea:auth` writes: + + /.claude/settings.local.json -> env.GITEA_LOGIN + +## Search order + +Start directories, in order, first hit wins: + + 1. $CLAUDE_PROJECT_DIR the project Claude Code was started on, when set + 2. an explicit hint the hook passes the Bash tool's cwd; scripts pass + nothing and go straight to 3 + 3. the current directory + +Each start directory is searched the same way: + + a. up the parent chain, from the directory itself to the filesystem root + b. then, for each LINKED WORKTREE seen on that chain, up the parent chain + of that repository's main working tree + +(b) is the whole point of this module. A worktree is a *sibling* of the main +checkout, not a descendant, so `.claude/settings.local.json` — untracked, and +therefore only ever in the main checkout — is not on the parent chain of (a). +Git knows the two trees are one repository: a worktree's `.git` is a FILE +holding `gitdir: `, and `/commondir` points back at the shared +`.git`. `git rev-parse --git-common-dir` answers the same question by forking; +this reads the files. + +## Why the search does not start at __file__ + +Deliberate asymmetry with `issue.store_root` and `_gitea.PAYLOAD_ROOT`, which +*are* anchored on their own module's location. Two different questions: + + where does this installation keep its files a fact about the plugin + whose login does this project run under a fact about the project + +A plugin installed outside any repository and pointed at somebody else's tree +must answer the second one from the tree it was pointed at. Anchoring the pin +on `__file__` would make the plugin's own directory an identity source, which +is how a checkout ends up acting under a login nobody chose for it. So the +search runs from the working directory upward — and reaches a worktree's main +checkout by asking git, not by walking somewhere else. + +Finding nothing is a real answer: `(None, None)` means there is no pin, and the +caller says so. This module never guesses a login. +""" +import json +import os + +SETTINGS_PARTS = (".claude", "settings.local.json") +ENV_KEY = "GITEA_LOGIN" +PROJECT_DIR_ENV = "CLAUDE_PROJECT_DIR" + + +def settings_path(root): + """The pin file for a project root. The one place this path is spelled.""" + return os.path.join(root, *SETTINGS_PARTS) + + +def read_pin(path): + """The login in a settings file, or None. + + Unreadable, not JSON, no `env`, empty string — all the same answer. A + broken file is not a login and is not worth a traceback in a hook.""" + try: + with open(path) as f: + value = (json.load(f).get("env") or {}).get(ENV_KEY) + except Exception: + return None + if isinstance(value, str) and value.strip(): + return value.strip() + return None + + +def parents(start): + """`start` and every ancestor of it, up to the filesystem root.""" + d = os.path.abspath(start) + while True: + yield d + parent = os.path.dirname(d) + if parent == d: + return + d = parent + + +def gitdir_of(d): + """The private git directory `d/.git` points at, or None. + + Only a `.git` FILE is a pointer; in an ordinary clone `.git` is a + directory and there is nothing to follow.""" + p = os.path.join(d, ".git") + if not os.path.isfile(p): + return None + try: + with open(p) as f: + head = f.read(4096) + except OSError: + return None + for line in head.splitlines(): + line = line.strip() + if line.startswith("gitdir:"): + target = line[len("gitdir:"):].strip() + if not target: + return None + if not os.path.isabs(target): + target = os.path.join(d, target) + return os.path.abspath(target) + return None + + +def main_worktree(d): + """If `d` is a linked worktree, the main working tree of its repository. + + `/.git` -> `
/.git/worktrees/`, whose `commondir` + file holds a path to `
/.git`; the main working tree is its parent. + The `.git` basename check keeps this to worktrees: a submodule's `.git` + is a pointer too, but it points into `/.git/modules/…`, and the + tree it belongs to is already on the parent chain.""" + gitdir = gitdir_of(d) + if not gitdir or not os.path.isdir(gitdir): + return None + common = gitdir + marker = os.path.join(gitdir, "commondir") + if os.path.isfile(marker): + try: + with open(marker) as f: + rel = f.read().strip() + except OSError: + rel = "" + if rel: + common = os.path.abspath(os.path.join(gitdir, rel)) + if os.path.basename(common) != ".git": + return None + root = os.path.dirname(common) + if root and os.path.isdir(root) and root != os.path.abspath(d): + return root + return None + + +def search(start): + """(login, path) for one start directory: the parent chain, then the main + checkout of any worktree met on it. (None, None) when there is no pin. + + The chain comes first and always wins, so the worktree branch can only + ever find a pin that walking up would not have found at all.""" + hops = [] + for d in parents(start): + login = read_pin(settings_path(d)) + if login: + return login, settings_path(d) + root = main_worktree(d) + if root and root not in hops: + hops.append(root) + for root in hops: + # One level of indirection, never two: a main checkout is not itself a + # linked worktree, so this loop cannot chain and cannot cycle. + for d in parents(root): + login = read_pin(settings_path(d)) + if login: + return login, settings_path(d) + return None, None + + +def start_dirs(hint=None): + """The ordered, deduplicated start directories. + + `hint` is the caller's own idea of where the work is happening — the hook + passes the `cwd` from its payload, which is the directory the Bash command + will actually run in. A script has no payload and passes nothing.""" + try: + cwd = os.getcwd() + except OSError: # cwd deleted out from under us + cwd = None + out = [] + for d in (os.environ.get(PROJECT_DIR_ENV), hint, cwd): + if not d: + continue + d = os.path.abspath(d) + if d not in out: + out.append(d) + return out + + +def find_pin(hint=None): + """(login, path) for the first start directory that has a pin, else + (None, None). The entry point; everything above is its parts.""" + for start in start_dirs(hint): + login, path = search(start) + if login: + return login, path + return None, None diff --git a/skills/sync/SKILL.md b/skills/sync/SKILL.md index 130202e..84219c5 100644 --- a/skills/sync/SKILL.md +++ b/skills/sync/SKILL.md @@ -32,8 +32,13 @@ index. ## Scripts In `/scripts/`. None of them take `--login`: they resolve the -operator's pin from `.claude/settings.local.json` themselves, the same source -the `tea-guard` hook reads. No pin → exit with a pointer to `/tea:auth`. +operator's pin from `.claude/settings.local.json` through +`skills/auth/scripts/pin.py` — the same *function* the `tea-guard` hook calls, +not merely the same file, so a directory where `tea` works is a directory where +these work. That includes a **git worktree**, whose untracked pin sits in the +main checkout: the search crosses to it through the `gitdir:` in `.git`, and +there is nothing to pin a second time. No pin anywhere → exit with a pointer to +`/tea:auth`. | Script | What it does | |---|---| @@ -312,6 +317,12 @@ a git repo no `ref` is sent and a warning names the issues that went up without one. Reading the branch is the only thing these scripts ask git for — they never check out, create, or write anything. +The branch comes from the **current directory**, so run `push.py` from the tree +the work is on. In a git worktree that is the worktree, and it is now also +where the pin resolves from: the old workaround for the pin — run the scripts +with cwd in the main checkout — sent the main checkout's branch as `ref`, which +is the one thing `branch:` exists to record. + ## What crosses the boundary, and what does not | domain | Gitea | note | diff --git a/skills/sync/scripts/_gitea.py b/skills/sync/scripts/_gitea.py index 5325d1d..2cbbf3f 100644 --- a/skills/sync/scripts/_gitea.py +++ b/skills/sync/scripts/_gitea.py @@ -7,8 +7,10 @@ query quirks. It does NOT know what an issue is: no sections, no acceptance criteria, no type taxonomy. Payload shapes come from map.py; the domain model lives one layer further out in skills/issue/scripts/issue.py. -Login: resolved from .claude/settings.local.json (env.GITEA_LOGIN), walking up -from CWD — the same file /tea:auth writes and the tea-guard hook reads. No +Login: the operator's pin from .claude/settings.local.json (env.GITEA_LOGIN). +Where that file is searched for is NOT written here — skills/auth/scripts/pin.py +owns the search order, and the tea-guard hook imports the same module, so `tea` +and the scripts can never disagree about which login a directory runs under. No script here accepts a login argument: the operator's pin is the only identity they will use. No pin -> exit with a pointer to /tea:auth. @@ -100,29 +102,32 @@ PAYLOAD_ROOT = payload_root() # -------------------------------------------------------------------------- # login # -------------------------------------------------------------------------- +# Borrowed from the identity layer, not reimplemented: `pin.find_pin` is the +# single written copy of the search order, and the tea-guard hook calls the +# same function. When the two had a copy each, a git worktree got a hook that +# resolved the pin and a transport that did not — in the same directory. +# +# Note the asymmetry with PAYLOAD_ROOT above, and with issue.store_root: those +# are anchored on their own file, this is not, and both are right. Where an +# installation keeps its files is a fact about the installation; whose login a +# project runs under is a fact about the project, and a plugin installed +# outside any repository must not answer it from its own directory. See the +# module docstring in pin.py. -def find_pin(start_dir=None): - """Walk up from start_dir; return the login from the first - .claude/settings.local.json carrying a non-empty env.GITEA_LOGIN.""" - d = os.path.abspath(start_dir or ".") - while True: - p = os.path.join(d, ".claude", "settings.local.json") - if os.path.isfile(p): - try: - with open(p) as f: - v = (json.load(f).get("env") or {}).get("GITEA_LOGIN") - if isinstance(v, str) and v.strip(): - return v.strip() - except Exception: - pass - parent = os.path.dirname(d) - if parent == d: - return None - d = parent +_AUTH_SCRIPTS = os.path.abspath( + os.path.join(_HERE, os.pardir, os.pardir, "auth", "scripts")) +if _AUTH_SCRIPTS not in sys.path: + sys.path.append(_AUTH_SCRIPTS) +import pin # noqa: E402 def require_login(): - login = find_pin(os.getcwd()) + """The operator's pinned login, or exit pointing at /tea:auth. + + No pin found is reported as exactly that. It stays a truthful message: the + fix for "the pin is somewhere this search does not reach" belongs in + pin.py, never in a hint here that sends the operator to pin it twice.""" + login, _ = pin.find_pin() if not login: die("no login pinned (.claude/settings.local.json env.GITEA_LOGIN). Run /tea:auth.") return login diff --git a/tests/test_login_pin.py b/tests/test_login_pin.py new file mode 100644 index 0000000..060ede9 --- /dev/null +++ b/tests/test_login_pin.py @@ -0,0 +1,418 @@ +#!/usr/bin/env python3 +""" +Where the login pin is found, and that a git worktree is not a dead zone. + + python3 -m unittest discover -s tests -v + +Stdlib unittest, no third-party anything, and not one real network call: every +run here is against a throwaway repository with a FAKE `tea` first on PATH. + +The bug: the pin was searched for by walking up from CWD only. A worktree is a +*sibling* of the main checkout, and `.claude/settings.local.json` is untracked, +so it lives in the main checkout and nowhere else — the whole sync layer died +inside any worktree with "no login pinned", while `tea` in the same directory +worked, because the tea-guard hook had a second, different copy of the search. + +So these tests hold two lines at once: the pin is reachable from a worktree, +and the hook and the scripts get their answer from the same function. +""" +import json +import os +import shutil +import stat +import subprocess +import sys +import tempfile +import unittest +from unittest import mock + +REPO = os.path.dirname(os.path.dirname(os.path.abspath(__file__))) +AUTH_SCRIPTS = os.path.join(REPO, "skills", "auth", "scripts") +SYNC_SCRIPTS = os.path.join(REPO, "skills", "sync", "scripts") +HOOKS = os.path.join(REPO, "hooks") + +sys.path.insert(0, AUTH_SCRIPTS) +import pin # noqa: E402 + +HAVE_GIT = shutil.which("git") is not None + +LOGIN = "fixture/user" +ENV_KEY = pin.ENV_KEY + +# A `tea` that answers without a network: an empty list for every GET, a +# created object for every write. It records its own argv, which is how a test +# reads back the login the call actually ran under. +FAKE_TEA = '''#!%s +import json, os, sys +argv = sys.argv[1:] +with open(os.environ["TEA_CALL_LOG"], "a") as f: + f.write("\\t".join(argv) + "\\n") +sys.stdout.write(json.dumps({"id": 1, "number": 101, "name": "created", + "html_url": "https://example.invalid/issues/101", + "labels": []}) + if "-X" in argv else "[]") +''' + +ISSUE = """\ +--- +id: pinned-work +state: open +labels: [type/task] +assignees: [] +milestone: none +depends: [] +origin: local +--- +# Pinned work + +## Summary +Issue фикстуры, живёт в сторе worktree. + +## Spec +none + +## Motivation +Нужен, чтобы push.py было что отправить. + +## Acceptance criteria +- [ ] проверяемое условие +""" + + +def write(path, text): + os.makedirs(os.path.dirname(path), exist_ok=True) + with open(path, "w") as f: + f.write(text) + + +class Worktree(object): + """A repository with a pin, and a linked worktree beside it. + + Beside, not below: `main/` and `worktrees/feature/` are siblings, which is + the entire shape of the bug. The pin is written after the clone is + committed and is covered by .gitignore, so it exists in the main checkout + only — exactly as `/tea:auth` leaves it.""" + + def __init__(self, pinned=LOGIN): + self._tmp = tempfile.TemporaryDirectory(prefix="tea-pin-") + # realpath: on macOS $TMPDIR is a symlink, and a child reporting its + # own cwd would otherwise disagree with the path we handed it. + self.root = os.path.realpath(self._tmp.name) + self.main = os.path.join(self.root, "main") + self.tree = os.path.join(self.root, "worktrees", "feature") + self.calls = os.path.join(self.root, "calls.txt") + + skip = shutil.ignore_patterns("__pycache__") + for layer in ("auth", "issue", "sync"): + shutil.copytree(os.path.join(REPO, "skills", layer, "scripts"), + os.path.join(self.main, "skills", layer, "scripts"), + ignore=skip) + shutil.copytree(HOOKS, os.path.join(self.main, "hooks"), ignore=skip) + write(os.path.join(self.main, ".gitignore"), "tmp/\n.claude/\n") + + self.bin = os.path.join(self.root, "fakebin") + os.makedirs(self.bin) + tea = os.path.join(self.bin, "tea") + write(tea, FAKE_TEA % sys.executable) + os.chmod(tea, os.stat(tea).st_mode | stat.S_IEXEC | stat.S_IXGRP | stat.S_IXOTH) + + self.git("init", cwd=self.main) + self.git("add", "-A", cwd=self.main) + self.git("commit", "-m", "fixture", cwd=self.main) + self.git("worktree", "add", "-b", "feature", self.tree, cwd=self.main) + + if pinned: + write(os.path.join(self.main, ".claude", "settings.local.json"), + json.dumps({"env": {ENV_KEY: pinned}})) + + def cleanup(self): + self._tmp.cleanup() + + def env(self): + env = dict(os.environ) + env.pop("PYTHONPATH", None) # no leakage from the harness into the child + # The start of the search order, cleared: this fixture is about the + # steps *after* it, and the developer's own project must not answer. + env.pop(pin.PROJECT_DIR_ENV, None) + env["PATH"] = self.bin + os.pathsep + env["PATH"] + env["TEA_CALL_LOG"] = self.calls + env["HOME"] = self.root # keep the developer's git config out + env["GIT_CONFIG_NOSYSTEM"] = "1" + env["GIT_CONFIG_GLOBAL"] = os.devnull + return env + + def git(self, *args, **kw): + cmd = ["git", "-c", "user.email=fixture@example.invalid", + "-c", "user.name=fixture", "-c", "commit.gpgsign=false"] + list(args) + p = subprocess.run(cmd, cwd=kw.pop("cwd", self.tree), env=self.env(), + capture_output=True, text=True) + if p.returncode != 0: + raise AssertionError("%s failed:\n%s%s" % (" ".join(cmd), p.stdout, p.stderr)) + return p.stdout.strip() + + def script(self, layer, name): + """A script as the WORKTREE sees it — the copy the operator would run.""" + return os.path.join(self.tree, "skills", layer, "scripts", name) + + def run(self, script, *args, **kw): + p = subprocess.run([sys.executable, script] + list(args), + cwd=kw.pop("cwd", self.tree), env=self.env(), + capture_output=True, text=True) + return p.returncode, p.stdout, p.stderr + + def tea_calls(self): + if not os.path.isfile(self.calls): + return [] + with open(self.calls) as f: + return [line.rstrip("\n").split("\t") for line in f if line.strip()] + + def logins_used(self): + return [a[a.index("--login") + 1] for a in self.tea_calls() if "--login" in a] + + +# -------------------------------------------------------------------------- +# the search itself +# -------------------------------------------------------------------------- + +class TestSearch(unittest.TestCase): + + def setUp(self): + self._tmp = tempfile.TemporaryDirectory(prefix="tea-pin-unit-") + self.root = os.path.realpath(self._tmp.name) + self.addCleanup(self._tmp.cleanup) + + def path(self, *parts): + return os.path.join(self.root, *parts) + + def pin_at(self, root, login=LOGIN): + write(os.path.join(root, ".claude", "settings.local.json"), + json.dumps({"env": {ENV_KEY: login}})) + + def test_the_parent_chain_is_searched(self): + self.pin_at(self.root) + os.makedirs(self.path("a", "b")) + self.assertEqual(pin.search(self.path("a", "b"))[0], LOGIN) + + def test_no_pin_is_no_pin(self): + os.makedirs(self.path("a")) + self.assertEqual(pin.search(self.path("a")), (None, None)) + + def test_an_unreadable_pin_is_not_a_login(self): + write(self.path(".claude", "settings.local.json"), "{ not json") + self.assertEqual(pin.search(self.root), (None, None)) + + def test_an_empty_pin_is_not_a_login(self): + write(self.path(".claude", "settings.local.json"), + json.dumps({"env": {ENV_KEY: " "}})) + self.assertEqual(pin.search(self.root), (None, None)) + + def test_a_git_file_pointing_at_a_worktree_reaches_the_main_checkout(self): + """The hop, built by hand from the two files git writes — no git + needed to state what the layout means.""" + main, tree = self.path("main"), self.path("elsewhere", "feature") + gitdir = os.path.join(main, ".git", "worktrees", "feature") + os.makedirs(gitdir) + os.makedirs(tree) + write(os.path.join(gitdir, "commondir"), "../..\n") + write(os.path.join(tree, ".git"), "gitdir: %s\n" % gitdir) + self.pin_at(main) + + self.assertEqual(pin.main_worktree(tree), main) + login, src = pin.search(tree) + self.assertEqual(login, LOGIN) + self.assertEqual(src, pin.settings_path(main)) + + def test_an_ordinary_clone_is_not_a_worktree(self): + os.makedirs(self.path("clone", ".git")) + self.assertIsNone(pin.main_worktree(self.path("clone"))) + + def test_a_submodule_pointer_is_not_a_worktree(self): + """`.git` is a file there too, but it points into .git/modules/… and + the tree it belongs to is already on the parent chain.""" + sub = self.path("super", "lib") + gitdir = self.path("super", ".git", "modules", "lib") + os.makedirs(gitdir) + os.makedirs(sub) + write(os.path.join(sub, ".git"), "gitdir: %s\n" % gitdir) + self.assertIsNone(pin.main_worktree(sub)) + + def test_the_chain_wins_over_the_hop(self): + """The worktree branch may only find a pin the walk up would have + missed entirely — it never overrides a nearer one.""" + main, tree = self.path("main"), self.path("elsewhere", "feature") + gitdir = os.path.join(main, ".git", "worktrees", "feature") + os.makedirs(gitdir) + os.makedirs(tree) + write(os.path.join(gitdir, "commondir"), "../..\n") + write(os.path.join(tree, ".git"), "gitdir: %s\n" % gitdir) + self.pin_at(main, "main/login") + self.pin_at(tree, "worktree/login") + self.assertEqual(pin.search(tree)[0], "worktree/login") + + def test_start_dirs_are_ordered_and_deduplicated(self): + with mock.patch.dict(os.environ, {pin.PROJECT_DIR_ENV: self.path("p")}): + self.assertEqual(pin.start_dirs(self.path("h")), + [self.path("p"), self.path("h"), + os.path.abspath(os.getcwd())]) + with mock.patch.dict(os.environ, {}, clear=True): + self.assertEqual(pin.start_dirs(), [os.path.abspath(os.getcwd())]) + + +# -------------------------------------------------------------------------- +# a script run from a worktree +# -------------------------------------------------------------------------- + +@unittest.skipUnless(HAVE_GIT, "git is not installed") +class TestScriptsInAWorktree(unittest.TestCase): + + def setUp(self): + self.wt = Worktree() + self.addCleanup(self.wt.cleanup) + + def test_a_sync_script_run_from_the_worktree_finds_the_login(self): + """The acceptance criterion, run for real: cwd inside the worktree, + the pin in the main checkout, and the call goes out under it.""" + rc, out, err = self.wt.run(self.wt.script("sync", "remote.py"), + "--repo", "fixture/repo", "--state", "all") + self.assertEqual(rc, 0, "remote.py failed:\n%s%s" % (out, err)) + self.assertNotIn("no login pinned", err) + self.assertEqual(self.wt.logins_used(), [LOGIN]) + + def test_it_does_not_pin_a_second_login_in_the_worktree(self): + """Nothing here writes a settings file, and the worktree is the last + place one should appear: it is deleted with the worktree.""" + self.wt.run(self.wt.script("sync", "remote.py"), "--repo", "fixture/repo") + self.assertFalse(os.path.exists(pin.settings_path(self.wt.tree)), + "a second settings.local.json appeared in the worktree") + + def test_with_no_pin_anywhere_it_still_says_so(self): + wt = Worktree(pinned=None) + self.addCleanup(wt.cleanup) + rc, out, err = wt.run(wt.script("sync", "remote.py"), "--repo", "fixture/repo") + self.assertNotEqual(rc, 0) + self.assertIn("no login pinned", err) + self.assertEqual(wt.logins_used(), []) + + def test_the_scripts_own_directory_is_not_a_pin_source(self): + """Run the worktree's script from a directory that is in no pinned + tree. The script sits inside a repository that has a pin — and it must + still refuse, because the pin belongs to the project being worked on, + not to the installation.""" + outside = os.path.join(self.wt.root, "outside") + os.makedirs(outside) + rc, out, err = self.wt.run(self.wt.script("sync", "remote.py"), + "--repo", "fixture/repo", cwd=outside) + self.assertNotEqual(rc, 0) + self.assertIn("no login pinned", err) + + def test_push_from_a_worktree_sends_the_worktree_branch(self): + """`branch:` -> Gitea `ref`. The workaround this fix removes — run the + worktree's scripts with cwd in the main checkout — sent the main + checkout's branch, which is the one field `branch:` exists for.""" + write(os.path.join(self.wt.tree, "tmp", "issues", "pinned-work.md"), ISSUE) + rc, out, err = self.wt.run(self.wt.script("sync", "push.py"), + "pinned-work", "--repo", "fixture/repo") + self.assertEqual(rc, 0, "push.py failed:\n%s%s" % (out, err)) + self.assertIn("created pinned-work #101", out) + + with open(os.path.join(self.wt.tree, "tmp", "payload", + "issue-pinned-work.json")) as f: + payload = json.load(f) + self.assertEqual(payload.get("ref"), "feature") + self.assertEqual(self.wt.git("rev-parse", "--abbrev-ref", "HEAD"), "feature") + self.assertNotEqual( + self.wt.git("rev-parse", "--abbrev-ref", "HEAD", cwd=self.wt.main), + "feature", "the fixture's two trees are on the same branch") + + +# -------------------------------------------------------------------------- +# one order, one copy of it +# -------------------------------------------------------------------------- + +@unittest.skipUnless(HAVE_GIT, "git is not installed") +class TestTheHookAndTheScriptsAgree(unittest.TestCase): + + def setUp(self): + self.wt = Worktree() + self.addCleanup(self.wt.cleanup) + + def guard(self, cwd): + """The hook, as the harness calls it: payload on stdin, decision on + stdout.""" + payload = {"tool_input": {"command": 'tea api --login "$GITEA_LOGIN" repos/x/y'}, + "cwd": cwd} + p = subprocess.run([sys.executable, os.path.join(self.wt.tree, "hooks", + "tea-guard.sh")], + input=json.dumps(payload), cwd=cwd, env=self.wt.env(), + capture_output=True, text=True) + return p + + def test_the_hook_resolves_the_pin_from_the_worktree_too(self): + p = self.guard(self.wt.tree) + self.assertEqual(p.returncode, 0, p.stderr) + got = json.loads(p.stdout)["hookSpecificOutput"]["updatedInput"]["command"] + self.assertIn(LOGIN, got) + self.assertNotIn("GITEA_LOGIN", got) + + def test_the_hook_and_a_script_answer_the_same_directory_alike(self): + """The regression that started this: in one directory the hook + resolved the login and every script said there was none.""" + rc, out, err = self.wt.run(self.wt.script("sync", "remote.py"), + "--repo", "fixture/repo") + self.assertEqual(rc, 0, err) + script_login = self.wt.logins_used()[0] + hook_login = json.loads(self.guard(self.wt.tree).stdout)[ + "hookSpecificOutput"]["updatedInput"]["command"].split("--login ")[1].split()[0] + self.assertEqual(hook_login, script_login) + + def test_the_hook_still_blocks_when_nothing_is_pinned(self): + wt = Worktree(pinned=None) + self.addCleanup(wt.cleanup) + payload = {"tool_input": {"command": 'tea api --login "$GITEA_LOGIN" repos/x/y'}, + "cwd": wt.tree} + p = subprocess.run([sys.executable, os.path.join(wt.tree, "hooks", "tea-guard.sh")], + input=json.dumps(payload), cwd=wt.tree, env=wt.env(), + capture_output=True, text=True) + self.assertEqual(p.returncode, 2) + self.assertIn("no login is pinned", p.stderr) + + +class TestNobodyKeepsASecondCopy(unittest.TestCase): + """Mechanical: the search order is written in pin.py, and the two callers + spell neither the path nor the walk.""" + + CALLERS = (os.path.join(HOOKS, "tea-guard.sh"), + os.path.join(SYNC_SCRIPTS, "_gitea.py")) + + def source(self, path): + with open(path) as f: + return f.read() + + def test_the_path_is_spelled_once(self): + self.assertEqual(pin.SETTINGS_PARTS, (".claude", "settings.local.json")) + for path in self.CALLERS: + body = self.source(path) + for literal in ('".claude"', "'.claude'"): + self.assertNotIn(literal, body, + "%s builds the settings path itself" % path) + + def test_both_callers_go_through_the_module(self): + for path in self.CALLERS: + self.assertIn("import pin", self.source(path), + "%s does not resolve the pin through pin.py" % path) + + def test_the_domain_layer_never_learns_what_a_login_is(self): + """The layer rule, unchanged by this: the identity module is imported + by the bridge and by the hook, never by a domain.""" + for layer in ("issue", "page"): + d = os.path.join(REPO, "skills", layer, "scripts") + for name in sorted(os.listdir(d)): + if not name.endswith(".py"): + continue + body = self.source(os.path.join(d, name)) + for banned in ("import pin", "GITEA_LOGIN", "settings.local.json"): + self.assertNotIn(banned, body, "%s/%s: %s" % (layer, name, banned)) + + +if __name__ == "__main__": + unittest.main() diff --git a/tests/test_page_tree.py b/tests/test_page_tree.py index 032e4f7..792ad9a 100644 --- a/tests/test_page_tree.py +++ b/tests/test_page_tree.py @@ -329,13 +329,12 @@ class TestImportScript(unittest.TestCase): self.tmp = tempfile.TemporaryDirectory() self.root = self.tmp.name os.makedirs(os.path.join(self.root, ".git")) - for layer in ("page", "wiki"): + # auth is in the list because the transport resolves the login pin + # through skills/auth/scripts/pin.py — one search order, one module. + for layer in ("page", "wiki", "sync", "auth"): shutil.copytree(os.path.join(REPO, "skills", layer, "scripts"), os.path.join(self.root, "skills", layer, "scripts"), ignore=shutil.ignore_patterns("__pycache__")) - shutil.copytree(SYNC_SCRIPTS, - os.path.join(self.root, "skills", "sync", "scripts"), - ignore=shutil.ignore_patterns("__pycache__")) self.src = build_artifacts(os.path.join(self.root, "artifacts")) self.scripts = os.path.join(self.root, "skills", "page", "scripts") self.space = os.path.join(self.root, "tmp", "wiki", "s") diff --git a/tests/test_payload_root.py b/tests/test_payload_root.py index 1d44134..f89e634 100644 --- a/tests/test_payload_root.py +++ b/tests/test_payload_root.py @@ -28,6 +28,7 @@ REPO = os.path.dirname(os.path.dirname(os.path.abspath(__file__))) ISSUE_SCRIPTS = os.path.join(REPO, "skills", "issue", "scripts") SYNC_SCRIPTS = os.path.join(REPO, "skills", "sync", "scripts") WIKI_SCRIPTS = os.path.join(REPO, "skills", "wiki", "scripts") +AUTH_SCRIPTS = os.path.join(REPO, "skills", "auth", "scripts") sys.path.insert(0, SYNC_SCRIPTS) sys.path.insert(0, ISSUE_SCRIPTS) @@ -61,6 +62,8 @@ class FakeRepo(object): skip = shutil.ignore_patterns("__pycache__") shutil.copytree(ISSUE_SCRIPTS, self.path("skills", "issue", "scripts"), ignore=skip) shutil.copytree(SYNC_SCRIPTS, self.path("skills", "sync", "scripts"), ignore=skip) + # the transport resolves the login pin through skills/auth/scripts + shutil.copytree(AUTH_SCRIPTS, self.path("skills", "auth", "scripts"), ignore=skip) os.makedirs(self.path("sub", "deeper")) # the login pin the transport insists on, local to this fixture diff --git a/tests/test_store_path.py b/tests/test_store_path.py index 736d214..20f31f1 100644 --- a/tests/test_store_path.py +++ b/tests/test_store_path.py @@ -24,6 +24,7 @@ import unittest REPO = os.path.dirname(os.path.dirname(os.path.abspath(__file__))) ISSUE_SCRIPTS = os.path.join(REPO, "skills", "issue", "scripts") SYNC_SCRIPTS = os.path.join(REPO, "skills", "sync", "scripts") +AUTH_SCRIPTS = os.path.join(REPO, "skills", "auth", "scripts") sys.path.insert(0, ISSUE_SCRIPTS) import issue # noqa: E402 @@ -110,6 +111,8 @@ class FakeRepo(object): skip = shutil.ignore_patterns("__pycache__") shutil.copytree(ISSUE_SCRIPTS, self.path("skills", "issue", "scripts"), ignore=skip) shutil.copytree(SYNC_SCRIPTS, self.path("skills", "sync", "scripts"), ignore=skip) + # the transport resolves the login pin through skills/auth/scripts + shutil.copytree(AUTH_SCRIPTS, self.path("skills", "auth", "scripts"), ignore=skip) os.makedirs(self.path("sub", "deeper")) if with_store: