diff --git a/AGENTS.md b/AGENTS.md index 985b6ae..4e340f5 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, @@ -65,11 +73,14 @@ 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 + - `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/close.py` — the state field, both ways; explicit ids only - `scripts/evict.py` — refresh `state:` from Gitea, then hand the decision to the domain's `issue_evict.run` + - `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 @@ -93,10 +104,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 @@ -108,12 +149,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 @@ -142,7 +189,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 @@ -209,3 +258,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/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/agents/tea-runner.md b/agents/tea-runner.md index 3931724..9d4de59 100644 --- a/agents/tea-runner.md +++ b/agents/tea-runner.md @@ -29,8 +29,8 @@ to fill the gap yourself. Load the skill, do not remember the flags: -- `/tea:sync` — `pull.py`, `push.py`, `comment.py`, `remote.py`, `labels.py`, - `evict.py` +- `/tea:sync` — `pull.py`, `push.py`, `comment.py`, `close.py`, `remote.py`, + `labels.py`, `evict.py` - `/tea:issue` — `issue_check.py`, `issue_tree.py`, `issue_index.py`, `issue_new.py`, `issue_ac.py`, `issue_evict.py` - `/tea:wiki` — `wiki_ls.py`, `wiki_pull.py`, `wiki_push.py` @@ -66,15 +66,23 @@ instead of trying it. `wiki_push.py` needs `-m`; use the caller's words, never your own summary. Report the number and URL `push.py` printed; that is now the only address the issue has. -4. **Do not close, delete, or retitle anything** on either side. On the wiki - that means no `--retitle`: renaming a published page abandons the old one. - Two deletions are allowed, both local and both only when the caller asked for - them: push's own, on the issue you were told to push, and eviction +4. **Close only the ids the caller named.** Closing is a script now + (`close.py`), so it is yours to run — under the same discipline as push: the + ids the caller named, and no others. Never widen the set, never infer that + an issue is finished because its checkboxes are ticked or its branch is + merged; whether work is done is a judgement about content, and content is + never yours. `--reopen` is the same rule backwards. **Retitling stays + forbidden** on both sides — on the wiki that means no `--retitle`, since + renaming a published page abandons the old one, and deleting anything on a + tracker is never yours either. + + Two local deletions are allowed, both only when the caller asked for them: + push's own, on the issue you were told to push, and eviction (`issue_evict.py` / `evict.py`) of closed issues. Run eviction with - `--dry-run` first and report what it named; never widen the set past what the - caller said. It refuses to touch an `origin: local` issue by itself — that is - the script's guarantee, not your judgement, and it is not a reason to point - it at a store nobody asked you to clean. + `--dry-run` first and report what it named; never widen the set past what + the caller said. It refuses to touch an `origin: local` issue by itself — + that is the script's guarantee, not your judgement, and it is not a reason + to point it at a store nobody asked you to clean. 5. **One retry, maximum.** A command that fails twice is a finding. Do not permute flags looking for one that works. 6. **No payload dumps.** Never run `tea issues -o json`, never `cat` a pulled @@ -128,3 +136,4 @@ Report these and halt; none of them is yours to resolve. | a dependency is still `origin: local` | name the id; the caller decides whether to push it | | a milestone or label does not exist in the repo | the script prints the real ones — pass that list through | | a script asks for a decision (type, label, `--force`) | `blocked:` with the question | +| `close.py` is refused by Gitea because the issue is still blocked | the tracker's own line, and the blocker's number; the caller decides | 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/issue/SKILL.md b/skills/issue/SKILL.md index 8c6f917..7df9ea4 100644 --- a/skills/issue/SKILL.md +++ b/skills/issue/SKILL.md @@ -118,7 +118,9 @@ the table. Checkboxes are the exception — use `issue_ac.py`, below. If the issue is synced (`origin: gitea`), the file is a working copy: your edit is local until you run `push.py --update` from `/tea:sync`, and that push -**deletes the file** once Gitea has it. Nothing tracks drift, and with one copy +**deletes the file** once Gitea has it. Closing one of those is `close.py` from +`/tea:sync` — it moves the state on both sides in a single run; editing +`state:` here alone would only ever tell this machine. Nothing tracks drift, and with one copy at a time there is little to track — a file that is still here has not been pushed. Get it back with `pull.py `; the slug does not change. diff --git a/skills/sync/SKILL.md b/skills/sync/SKILL.md index 2272772..3934539 100644 --- a/skills/sync/SKILL.md +++ b/skills/sync/SKILL.md @@ -1,6 +1,6 @@ --- name: sync -description: Move issues between the local store and Gitea — pull issues into tmp/issues/, push local issues up, post comments. Load when the user asks to fetch/read a Gitea issue, publish an issue, list what exists in the tracker, or comment on one. Working with an issue's content (writing, grepping, validating, dependency graph) is /tea:issue and needs no network. +description: Move issues between the local store and Gitea — pull issues into tmp/issues/, push local issues up, post comments, close and reopen them. Load when the user asks to fetch/read a Gitea issue, publish an issue, list what exists in the tracker, comment on one, or close/reopen one. Working with an issue's content (writing, grepping, validating, dependency graph) is /tea:issue and needs no network. --- # /tea:sync — the bridge between the local store and Gitea @@ -32,16 +32,22 @@ 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 | |---|---| -| `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; 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 | | `evict.py [id…] [--dry-run]` | refresh `state:` from Gitea, then evict the issues it reports closed; `origin: local` is never asked about and never removed | | `comment.py --file F \| --body TEXT [--edit N]` | post or edit a comment, then refetch the thread | +| `close.py [--reopen] [--dry-run]` | set `state` in Gitea and in the local copy with it; explicit ids only, no bulk filter | | `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` | | `map.py`, `_gitea.py` | the two layers the commands import — not commands | @@ -89,7 +95,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 @@ -115,13 +121,55 @@ 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 | +| 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 +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 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: @@ -234,7 +282,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/*`, @@ -254,7 +302,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 @@ -299,6 +347,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 @@ -309,6 +361,60 @@ 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. + +## Closing and reopening + +```bash +python3 /scripts/close.py wire-sqlc-appclick # by slug +python3 /scripts/close.py 42 '#43' # by number +python3 /scripts/close.py --reopen 42 +python3 /scripts/close.py --dry-run 42 43 # no request at all +``` + +`close.py` is the only supported way to move `state:`. Never hand-roll +`tea api -X PATCH -d '{"state":"closed"}' repos/OWNER/REPO/issues/N`: it spells +out the owner, the repo and the request body — the three things this layer +exists to hide — and it needs a `Bash(tea api *)` permission that also covers +`-X DELETE` on the repository. + +**State only.** The payload is `{"state": …}` and nothing else — no title, no +body, no labels, no milestone. Closing is not an edit; editing is `pull.py` → +change → `push.py --update`. + +**Explicit ids only.** There is no `--milestone` and no `--label`: which issues +are finished is a judgement about content, and this script only carries one +out, one named id at a time. Deleting an issue is out of scope too — Gitea can, +and it is not an operation of this workflow. + +What may be named, and what happens to the local copy: + +| named | resolved through | local file | +|---|---|---| +| a slug with a file on disk | its `gitea:` field | `state:` rewritten, `synced:` refreshed | +| a slug whose file push dropped | `.remote.json` | none to write — say so and move on | +| `42`, `#42`, `owner/repo#42`, a URL | the key itself; the ledger supplies the slug | rewritten when a file of that slug is there | +| a slug with `origin: local` | — | **refused**: it is not in the tracker, and the error names the id | + +The local file is written only after the tracker has confirmed *this* write: an +object carrying the very number that was PATCHed, in the state that was asked +for. A non-2xx, a `tea` that would not run, an answer for another issue, a 200 +that still says `open` — the run stops and the file is byte for byte what it +was. `--dry-run` prints the same lines and makes no request at all, so it needs +no pinned login. + +Gitea refuses to close an issue that its own dependency graph still blocks. The +refusal arrives as a non-2xx with the tracker's own words: close the blockers +first, or unlink them in the web UI. + +The index is rebuilt when at least one local file changed, so `INDEX.md` never +outlives the state it reports. Nothing is deleted here — unlike a push, a close +leaves the working copy where it is. + ## Evicting what the tracker says is closed ```bash @@ -371,7 +477,7 @@ so there is no reason to start before every answer is in. | `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:` | @@ -406,6 +512,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..a3b22c1 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. @@ -18,6 +20,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 +36,33 @@ import re import sys 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 + +# -------------------------------------------------------------------------- +# 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,32 +77,64 @@ def now_iso(): return datetime.datetime.now(datetime.timezone.utc).strftime("%Y-%m-%dT%H:%M:%SZ") -# -------------------------------------------------------------------------- -# login -# -------------------------------------------------------------------------- - -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 ".") +def repo_root(start): + """Nearest ancestor of `start` (inclusive) carrying a repo marker, or None.""" + d = os.path.abspath(start) 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 + 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 +# -------------------------------------------------------------------------- +# 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. + +_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 @@ -80,19 +145,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] @@ -114,17 +181,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 +265,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 +304,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): @@ -245,7 +358,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 +377,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 +409,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/close.py b/skills/sync/scripts/close.py new file mode 100644 index 0000000..963db1c --- /dev/null +++ b/skills/sync/scripts/close.py @@ -0,0 +1,253 @@ +#!/usr/bin/env python3 +""" +close.py — change an issue's state in Gitea, and in the local copy with it. + +The one regular tracker operation that used to have no script: closing. Without +it the only way to move `state:` was a raw `tea api -X PATCH -d '{"state": +"closed"}' repos/OWNER/REPO/issues/N`, which spells out the owner, the repo and +the request body — the three things `_gitea.py` exists to hide — and which needs +`Bash(tea api *)`, a permission that also covers `-X DELETE` on the repository. + + close.py wire-sqlc-appclick one issue, by slug + close.py wire-sqlc-appclick 42 #43 several, by slug or number + close.py --reopen 42 the same thing backwards + close.py --dry-run 42 43 what would happen, no request at all + +STATE ONLY. This script sends `{"state": …}` and nothing else: no title, no +body, no labels, no milestone. Editing an issue is `pull.py` -> edit -> +`push.py --update`; closing it is not an edit. + +**What may be named.** A local slug, or a Gitea key (`42`, `#42`, +`owner/repo#42`, an issue URL) — the same forms `pull.py` takes. Both are +needed, and for the same reason: a push deletes the local file, so most issues +in the tracker have no slug on disk to name them by. A slug is resolved through +the file's `gitea:` field when the file is there, and through the ledger +(`.remote.json`) when push has already dropped it. + +**An `origin: local` issue cannot be closed.** It is not in the tracker, so +there is nothing to close there, and the run stops naming the id rather than +quietly editing one field of a local file. Delete it, or push it first. + +**Explicit ids only.** No `--milestone`, no `--label`, no "close everything +that looks done". Which issues are finished is a judgement about content; this +script only carries it out, one named id at a time. Nothing here deletes an +issue either — Gitea can, and it is not an operation of this workflow. + +The local file is written only after the tracker has confirmed the write: + + 1. `tea` ran and exited 0 (a non-2xx exits the run inside `_gitea.api`), and + 2. the answer is an object carrying the very number that was PATCHed, and + 3. its `state` is the state we asked for. + +Anything else and the file is left exactly as it was — see `confirmed`. An +issue whose local copy is gone (pushed and dropped) is closed in Gitea and +nothing is written; the state comes down with the next `pull.py`. + +Gitea refuses to close an issue that its own dependency graph still blocks. That +refusal arrives as a non-2xx and stops the run with the tracker's own words: +close the blockers first, or unlink them in the web UI. + +Login: the operator's pin from .claude/settings.local.json (see /tea:auth). +""" +import argparse +import os +import re +import sys + +_HERE = os.path.dirname(os.path.abspath(__file__)) +sys.path[:0] = [_HERE, os.path.normpath(os.path.join(_HERE, "..", "..", "issue", "scripts"))] + +import _gitea # noqa: E402 +import issue # noqa: E402 +import issue_index # noqa: E402 +import map as gmap # noqa: E402 + +# What `_gitea.parse_key` accepts, asked as a question instead of an assertion: +# parse_key exits on anything it cannot read, and here "not a key" is the normal +# case — it means the argument is a slug. A slug never contains `#`, `/` or `:`, +# so the two vocabularies cannot collide. +KEY_RE = re.compile(r'^(#?\d+|[\w.-]+/[\w.-]+#\d+|https?://\S+)$') + + +def looks_like_key(arg): + return bool(KEY_RE.match((arg or "").strip())) + + +def ledger_pairs(remote_map, repo=None): + """[(repo, number, slug)] from `.remote.json`, filtered to `repo`. + + A `--repo` that was not given means "whatever the ledger holds": resolving + the repo's real name costs a request, and a dry run is required to make + none. The ambiguity that opens — one number under two repos — is caught at + lookup time rather than papered over.""" + out = [] + for key, slug in sorted(remote_map.items()): + r, n = gmap.parse_remote_key(key) + if n: + if repo is None or r == repo: + out.append((r, n, slug)) + return out + + +def one(candidates, what, arg): + """The single `(repo, value)` in `candidates`, None when empty, or exit. + + Two answers mean the ledger knows this number (or this slug) under more than + one repository, and only `--repo` can settle that.""" + got = sorted(set(candidates)) + if len(got) > 1: + _gitea.die("%r matches %s under more than one repo (%s) — pass " + "--repo owner/repo" % (arg, what, ", ".join(r for r, _v in got))) + return got[0] if got else None + + +def resolve(arg, issues, pairs): + """(id, number, repo) for one argument. Either of `id` and `repo` is None + when nothing this machine holds names it. + + Order, and it is the order of what is most authoritative about this machine: + a file on disk, then the ledger, then nothing. A key skips straight to the + ledger — its number is already the tracker's answer, and the slug is only + wanted so the local copy, if there is one, can be kept honest. + + `repo` travels out with the number because a key may name one + (`owner/repo#42`) and a `gitea:` field always does. Sending a foreign key to + whatever repo the CWD happens to be in would close somebody else's issue of + the same number, so the caller reconciles them before anything goes out.""" + if looks_like_key(arg): + number, repo = _gitea.parse_key(arg) + hit = one([(r, s) for r, n, s in pairs + if n == number and (repo is None or r == repo)], "a slug", arg) + return (hit[1] if hit else None), number, repo or (hit[0] if hit else None) + + iss = issues.get(arg) + if iss is not None: + repo, number = gmap.parse_remote_key(iss.extra.get("gitea", "")) + if not number: + _gitea.die("%s is not in the tracker (origin: %s, no gitea: field) — " + "there is no state there to change; push.py %s first" + % (arg, iss.origin, arg)) + return arg, number, repo + + hit = one([(r, n) for r, n, s in pairs if s == arg], "a number", arg) + if hit: + return arg, hit[1], hit[0] # pushed, and its file went with the push + _gitea.die("no issue %r in the store or the ledger — pass a Gitea number " + "(42, #42, owner/repo#42, a URL) to close one this machine has " + "never seen" % arg) + + +def confirmed(got, number, state): + """True when the tracker's answer confirms THIS write, and nothing else. + + The gate in front of the local write, and deliberately boring: an answer + counts only when it is an object carrying the very number that was PATCHed + (`bool` rejected explicitly — `True` is an `int`) and the state that was + asked for. A non-2xx and a `tea` that would not run never reach here at all; + `_gitea.api` exits on both, so the file survives those by never being + written.""" + if not isinstance(got, dict): + return False + n = got.get("number") + if isinstance(n, bool) or not isinstance(n, int) or n != number: + return False + return got.get("state") == state + + +def apply_state(root, iss, state, got): + """Write the confirmed state onto the local file; return its path. + + `state:` is the domain's own field, so it is set on the issue and written + out by the domain's own writer. The sync-owned freshness fields travel with + it: the answer that authorized this write is also the newest thing the + tracker has said about the issue, so `synced:` and `remote-updated:` are + stamped from it rather than left describing an older read.""" + iss.state = state + iss.extra["synced"] = _gitea.now_iso() + if got.get("updated_at"): + iss.extra["remote-updated"] = got["updated_at"] + return issue.save(root, iss) + + +def main(): + ap = argparse.ArgumentParser(description="Close (or reopen) issues in Gitea") + ap.add_argument("ids", nargs="+", + help="local ids, or Gitea keys: 42, #42, owner/repo#42, URL") + ap.add_argument("--reopen", action="store_true", + help="set the state back to open instead of closed") + ap.add_argument("--dry-run", action="store_true", + help="print what would change; makes no request at all") + ap.add_argument("--repo", help="owner/repo (default: auto-detect from CWD git remote)") + ap.add_argument("--out", default=issue.ISSUE_ROOT, + help="store root (default: /tmp/issues)") + args = ap.parse_args() + + root = args.out + state = "open" if args.reopen else "closed" + verb = "reopen" if args.reopen else "close" + past = "reopened" if args.reopen else "closed" + + # A store that is not there is not an error here: a number needs no local + # file, and closing an issue whose copy was dropped by push is the normal + # case. `load_all` reads an absent directory as an empty one. + issues = issue.load_all(root) + pairs = ledger_pairs(_gitea.load_map(root), args.repo) + + # Every argument is resolved before anything is sent, so a typo in the third + # id does not leave the first two closed. + targets = [] + for arg in args.ids: + got = resolve(arg, issues, pairs) + if got not in targets: + targets.append(got) + + # One run, one repo. An explicit --repo is the operator's word and wins; + # without one, the repo comes from what the ids themselves said, and two + # answers are a question rather than a guess — `repo_base` would otherwise + # let `tea` fill the blank from the CWD and close the wrong #42. + named = {r for _i, _n, r in targets if r} + if not args.repo and len(named) > 1: + _gitea.die("all ids must belong to one repo, got: %s" % ", ".join(sorted(named))) + repo_arg = args.repo or (sorted(named)[0] if named else None) + + if args.dry_run: + for id, number, _repo in targets: + iss = issues.get(id) + where = ("%s (state: %s)" % (issue.path_of(root, id), iss.state) + if iss is not None else "no local copy") + print("would %s %s #%d — %s" % (verb, id or "?", number, where)) + print("%d issue(s) would be %s; no request was made" + % (len(targets), past)) + return + + login = _gitea.require_login() + base = _gitea.repo_base(repo_arg) + + touched = 0 + for id, number, _repo in targets: + got = _gitea.api(login, "%s/issues/%d" % (base, number), "PATCH", + {"state": state}, payload_name="state-%d" % number) + # The gate. Above it nothing local has been written; below it the file + # is about to say something the tracker had better agree with. + if not confirmed(got, number, state): + _gitea.die("#%d: %s failed — the tracker's answer does not confirm the " + "write (%.200r). Nothing local was changed." + % (number, verb, got)) + + print("%s %s #%d %s" % (past, id or "?", number, + got.get("html_url", ""))) + + iss = issues.get(id) + if iss is None: + print(" no local copy — pull.py %d to get one" % number) + continue + print(" state: %s %s" % (state, apply_state(root, iss, state, got))) + touched += 1 + + if touched: + path, n = issue_index.build(root) + print("index: %s — %d issue(s)" % (path, n)) + + +if __name__ == "__main__": + main() 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/pull.py b/skills/sync/scripts/pull.py index 6b522a4..58ec80d 100644 --- a/skills/sync/scripts/pull.py +++ b/skills/sync/scripts/pull.py @@ -28,11 +28,31 @@ 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. +- 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 +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 @@ -42,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 @@ -52,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). @@ -98,6 +151,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,8 +201,18 @@ 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("--deps", action="store_true", help="follow dependencies and pull them") + ap.add_argument("--limit", type=int, default=100, + help="filter mode: how many issues to STORE, not to enumerate" + " (default: 100)") + # 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") @@ -180,9 +260,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 = [] @@ -208,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 @@ -265,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): @@ -275,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..563f146 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 @@ -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/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/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_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_close.py b/tests/test_close.py new file mode 100644 index 0000000..d069f67 --- /dev/null +++ b/tests/test_close.py @@ -0,0 +1,641 @@ +#!/usr/bin/env python3 +""" +close.py — the state changes in Gitea, and the local file follows it or nothing +happens at all. + +Two halves, and the second is the one that matters: + +1. **It closes.** A slug, a number, several of either in one run, and + `--reopen` going the other way. What goes out is a PATCH carrying `state` + and nothing else; what comes back is written into `state:` on the local + file, and the index is rebuilt so the store's own table agrees. + +2. **It changes nothing local unless the tracker confirmed it.** A `tea` that + exited non-zero, an answer with no number, an answer for another issue, an + answer that still says `open`, an `origin: local` issue, a `--dry-run`: in + every one of those the file on disk is byte for byte what it was. A bug here + makes the store lie about the tracker, so each path is asserted on its own. + +The transport is stubbed at `_gitea.api`, as `test_drop_after_push.py` does, +with the same deliberate exception: the non-2xx test stubs `_gitea.subprocess` +and lets the real `_gitea.api` run, so "tea exited 1" is proved end to end. + +Nothing here touches a network, and nothing here touches the developer's store: +every test builds its own in a `tempfile.TemporaryDirectory()`. +""" +import contextlib +import io +import json +import os +import sys +import tempfile +import types +import unittest +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 close # noqa: E402 +import issue # noqa: E402 +import map as gmap # noqa: E402 + +# Captured before any test patches it — the non-2xx test needs the real thing. +REAL_API = _gitea.api + +REPO = "claude-skills/tea" +BASE = "repos/%s" % REPO + +BODY = """## Summary +Прозаическое описание задачи. + +## Spec +skills/issue/references/format.md + +## Acceptance criteria +- [x] что-нибудь работает +""" + + +class FakeTracker(object): + """`tea api` answered from memory, for state writes only. + + It keeps a `state` per number and flips it on a PATCH, which is the whole + contract close.py has with the far side.""" + + def __init__(self): + self.calls = [] + self.states = {} # number -> "open" / "closed" + self.raise_on_write = None # an exception instance to raise + self.answer_override = None # what a write answers instead + + def payload_of(self, number): + return {"number": number, "state": self.states[number], + "title": "A thing", "updated_at": "2026-08-11T00:00:00Z", + "html_url": "https://git.example/%s/issues/%d" % (REPO, number)} + + def writes(self): + return [c for c in self.calls if c[0] != "GET"] + + def api(self, login, endpoint, method="GET", payload=None, + payload_name=None, out_root=None, allow_fail=False): + self.calls.append((method, endpoint, payload)) + path = endpoint.split("?")[0] + + if "/issues/" in path and method == "PATCH": + number = int(path.rsplit("/", 1)[1]) + if self.raise_on_write is not None: + raise self.raise_on_write + self.states.setdefault(number, "open") + if "state" in (payload or {}): + self.states[number] = payload["state"] + if self.answer_override is not None: + return self.answer_override + return self.payload_of(number) + + if "/issues/" in path and method == "GET": + n = int(path.rsplit("/", 1)[1]) + return self.payload_of(n) if n in self.states else None + + raise AssertionError("unstubbed call: %s %s" % (method, endpoint)) + + +class StoreTestCase(unittest.TestCase): + """A temp store and a fake tracker.""" + + def setUp(self): + tmp = tempfile.TemporaryDirectory(prefix="tea-close-") + self.addCleanup(tmp.cleanup) + self.root = tmp.name + self.fake = FakeTracker() + for p in (mock.patch.object(_gitea, "api", self.fake.api), + mock.patch.object(_gitea, "require_login", lambda: "test-login")): + p.start() + self.addCleanup(p.stop) + + # -- fixtures ---------------------------------------------------------- + + def synced(self, id="a-thing", number=101, state="open"): + """An issue that is in the tracker and on disk, the way a pull leaves + it: `origin: gitea`, a `gitea:` field, and a ledger entry.""" + key = gmap.remote_key(REPO, number) + iss = issue.Issue(id=id, title="A thing", body=BODY, state=state, + labels=["type/task"], origin=gmap.ORIGIN, + extra={"gitea": key, "url": "https://git.example/x", + "synced": "2026-08-10T00:00:00Z"}) + issue.save(self.root, iss) + m = _gitea.load_map(self.root) + m[key] = id + _gitea.save_map(self.root, m) + self.fake.states[number] = state + return iss + + def local_only(self, id="local-thing"): + """An issue that has never left this machine.""" + iss = issue.Issue(id=id, title="Local thing", body=BODY, + labels=["type/task"]) + issue.save(self.root, iss) + return iss + + def dropped(self, id="gone-thing", number=205, state="open"): + """Pushed, and its file went with the push: ledger only.""" + m = _gitea.load_map(self.root) + m[gmap.remote_key(REPO, number)] = id + _gitea.save_map(self.root, m) + self.fake.states[number] = state + return number + + # -- runner ------------------------------------------------------------ + + def run_close(self, *argv): + self.out, self.err = io.StringIO(), io.StringIO() + args = ["close.py", "--repo", REPO, "--out", self.root] + list(argv) + with mock.patch.object(sys, "argv", args), \ + contextlib.redirect_stdout(self.out), \ + contextlib.redirect_stderr(self.err): + close.main() + return self.out.getvalue(), self.err.getvalue() + + # -- assertions -------------------------------------------------------- + + def state_on_disk(self, id): + return issue.load(self.root, id).state + + def raw(self, id): + with open(issue.path_of(self.root, id)) as f: + return f.read() + + def assertUnchanged(self, id, before, why=""): + self.assertEqual(self.raw(id), before, + "%s.md was rewritten%s" % (id, why and " — " + why)) + + +# -------------------------------------------------------------------------- +# it closes +# -------------------------------------------------------------------------- + +class ClosesTest(StoreTestCase): + + def test_a_slug_closes_the_issue_it_names(self): + self.synced("a-thing", 101) + out, _ = self.run_close("a-thing") + self.assertEqual(self.fake.states[101], "closed") + self.assertIn("closed a-thing #101", out) + + def test_the_local_state_follows(self): + self.synced("a-thing", 101) + self.run_close("a-thing") + self.assertEqual(self.state_on_disk("a-thing"), "closed") + + def test_only_the_state_is_sent(self): + """Closing is not an edit: no title, no body, no labels ride along.""" + self.synced("a-thing", 101) + self.run_close("a-thing") + writes = self.fake.writes() + self.assertEqual(len(writes), 1) + method, endpoint, payload = writes[0] + self.assertEqual((method, endpoint), ("PATCH", "%s/issues/101" % BASE)) + self.assertEqual(payload, {"state": "closed"}) + + def test_a_number_closes_it_too(self): + """The normal case for a pushed issue — the file is long gone.""" + self.synced("a-thing", 101) + self.run_close("101") + self.assertEqual(self.fake.states[101], "closed") + self.assertEqual(self.state_on_disk("a-thing"), "closed") + + def test_every_key_form_is_accepted(self): + forms = {110: "110", 111: "#111", 112: "%s#112" % REPO, + 113: "https://git.example/%s/issues/113" % REPO} + for n in forms: + self.fake.states[n] = "open" + for n, arg in forms.items(): + with self.subTest(arg=arg): + self.run_close(arg) + self.assertEqual(self.fake.states[n], "closed") + + def test_several_ids_in_one_run(self): + self.synced("a-thing", 101) + self.synced("b-thing", 102) + self.run_close("a-thing", "102") + self.assertEqual(self.fake.states, {101: "closed", 102: "closed"}) + self.assertEqual(self.state_on_disk("a-thing"), "closed") + self.assertEqual(self.state_on_disk("b-thing"), "closed") + + def test_the_same_issue_named_twice_is_written_once(self): + self.synced("a-thing", 101) + self.run_close("a-thing", "#101") + self.assertEqual(len(self.fake.writes()), 1) + + def test_the_index_is_rebuilt(self): + self.synced("a-thing", 101) + out, _ = self.run_close("a-thing") + self.assertIn("index:", out) + with open(os.path.join(self.root, "INDEX.md")) as f: + self.assertIn("closed", f.read()) + + def test_the_body_survives_untouched(self): + """One metadata field changes; the prose and the ticks do not.""" + self.synced("a-thing", 101) + before = issue.load(self.root, "a-thing").body + self.run_close("a-thing") + self.assertEqual(issue.load(self.root, "a-thing").body, before) + + def test_synced_is_refreshed(self): + self.synced("a-thing", 101) + self.run_close("a-thing") + iss = issue.load(self.root, "a-thing") + self.assertNotEqual(iss.extra.get("synced"), "2026-08-10T00:00:00Z") + self.assertEqual(iss.extra.get("remote-updated"), "2026-08-11T00:00:00Z") + + def test_an_issue_whose_file_was_dropped_still_closes(self): + """No local copy at all: the ledger names it, the tracker takes it, and + nothing is written locally.""" + self.dropped("gone-thing", 205) + out, _ = self.run_close("gone-thing") + self.assertEqual(self.fake.states[205], "closed") + self.assertIn("no local copy", out) + self.assertNotIn("index:", out) + + def test_a_number_nobody_here_knows_closes_without_a_slug(self): + self.fake.states[777] = "open" + out, _ = self.run_close("777") + self.assertEqual(self.fake.states[777], "closed") + self.assertIn("#777", out) + + +class ReopensTest(StoreTestCase): + + def test_reopen_sends_open(self): + self.synced("a-thing", 101, state="closed") + out, _ = self.run_close("--reopen", "a-thing") + self.assertEqual(self.fake.writes()[0][2], {"state": "open"}) + self.assertIn("reopened a-thing #101", out) + + def test_reopen_writes_the_local_state_back(self): + self.synced("a-thing", 101, state="closed") + self.run_close("--reopen", "a-thing") + self.assertEqual(self.state_on_disk("a-thing"), "open") + + def test_close_then_reopen_is_a_round_trip(self): + self.synced("a-thing", 101) + self.run_close("a-thing") + self.run_close("--reopen", "a-thing") + self.assertEqual(self.fake.states[101], "open") + self.assertEqual(self.state_on_disk("a-thing"), "open") + + +# -------------------------------------------------------------------------- +# it refuses +# -------------------------------------------------------------------------- + +class LocalOnlyTest(StoreTestCase): + """An `origin: local` issue is not in the tracker, so it cannot be closed + there — and the local field is not quietly edited instead.""" + + def test_it_exits(self): + self.local_only("local-thing") + with self.assertRaises(SystemExit): + self.run_close("local-thing") + + def test_the_error_names_the_id_and_says_it_is_not_in_the_tracker(self): + self.local_only("local-thing") + with self.assertRaises(SystemExit): + self.run_close("local-thing") + err = self.err.getvalue() + self.assertIn("local-thing", err) + self.assertIn("not in the tracker", err) + + def test_nothing_is_sent(self): + self.local_only("local-thing") + with self.assertRaises(SystemExit): + self.run_close("local-thing") + self.assertEqual(self.fake.calls, []) + + def test_the_file_is_untouched(self): + self.local_only("local-thing") + before = self.raw("local-thing") + with self.assertRaises(SystemExit): + self.run_close("local-thing") + self.assertUnchanged("local-thing", before) + + def test_a_bad_id_stops_the_whole_run_before_anything_is_sent(self): + """Resolution happens up front, so a typo in the second id does not + leave the first one closed.""" + self.synced("a-thing", 101) + with self.assertRaises(SystemExit): + self.run_close("a-thing", "local-thing") + self.assertEqual(self.fake.states[101], "open") + self.assertEqual(self.fake.calls, []) + + def test_an_unknown_slug_exits(self): + with self.assertRaises(SystemExit): + self.run_close("no-such-thing") + self.assertIn("no-such-thing", self.err.getvalue()) + + +class DryRunTest(StoreTestCase): + + def test_not_one_request_is_made(self): + self.synced("a-thing", 101) + self.run_close("--dry-run", "a-thing") + self.assertEqual(self.fake.calls, []) + + def test_the_file_is_untouched(self): + self.synced("a-thing", 101) + before = self.raw("a-thing") + self.run_close("--dry-run", "a-thing") + self.assertUnchanged("a-thing", before, "--dry-run must write nothing") + + def test_it_says_what_would_be_closed(self): + self.synced("a-thing", 101) + self.synced("b-thing", 102) + out, _ = self.run_close("--dry-run", "a-thing", "102") + self.assertIn("would close a-thing #101", out) + self.assertIn("would close b-thing #102", out) + self.assertIn("2 issue(s) would be closed", out) + + def test_it_says_reopen_under_reopen(self): + self.synced("a-thing", 101, state="closed") + out, _ = self.run_close("--dry-run", "--reopen", "a-thing") + self.assertIn("would reopen a-thing #101", out) + self.assertIn("would be reopened", out) + + def test_it_needs_no_login(self): + """A dry run must work before /tea:auth has ever been run.""" + self.synced("a-thing", 101) + with mock.patch.object(_gitea, "require_login", + lambda: self.fail("dry run asked for a login")): + self.run_close("--dry-run", "a-thing") + + def test_a_local_only_issue_is_still_refused(self): + self.local_only("local-thing") + with self.assertRaises(SystemExit): + self.run_close("--dry-run", "local-thing") + + +# -------------------------------------------------------------------------- +# the tracker said no +# -------------------------------------------------------------------------- + +class TrackerFailureTest(StoreTestCase): + """The criterion that matters most: a write that was not confirmed leaves + the local file exactly as it was.""" + + def test_a_non_2xx_answer_leaves_the_file(self): + """The real `_gitea.api` against a `tea` that exits 1 — the path a 422 + or a 500 actually takes, and it ends in `die()`.""" + self.synced("a-thing", 101) + before = self.raw("a-thing") + + def fake_run(cmd, capture_output=False, text=False): + return types.SimpleNamespace( + returncode=1, stdout="", + stderr="422 Unprocessable Entity: issue is blocked") + + with mock.patch.object(_gitea, "api", REAL_API), \ + mock.patch.object(_gitea, "subprocess", + types.SimpleNamespace(run=fake_run)), \ + self.assertRaises(SystemExit): + self.run_close("a-thing") + + self.assertUnchanged("a-thing", before, "tea exited non-zero") + self.assertEqual(self.state_on_disk("a-thing"), "open") + + def test_a_transport_exception_leaves_the_file(self): + self.synced("a-thing", 101) + before = self.raw("a-thing") + self.fake.raise_on_write = OSError("tea: command not found") + with self.assertRaises(OSError): + self.run_close("a-thing") + self.assertUnchanged("a-thing", before, "the transport raised") + + def test_an_answer_without_a_number_leaves_the_file(self): + self.synced("a-thing", 101) + before = self.raw("a-thing") + self.fake.answer_override = {"ok": True, "state": "closed"} + with self.assertRaises(SystemExit): + self.run_close("a-thing") + self.assertUnchanged("a-thing", before) + + def test_an_answer_for_another_issue_leaves_the_file(self): + self.synced("a-thing", 101) + before = self.raw("a-thing") + self.fake.answer_override = {"number": 999, "state": "closed"} + with self.assertRaises(SystemExit): + self.run_close("a-thing") + self.assertUnchanged("a-thing", before) + + def test_an_answer_that_did_not_change_the_state_leaves_the_file(self): + """A 200 that still says `open` is not a close.""" + self.synced("a-thing", 101) + before = self.raw("a-thing") + self.fake.answer_override = {"number": 101, "state": "open"} + with self.assertRaises(SystemExit): + self.run_close("a-thing") + self.assertUnchanged("a-thing", before) + + def test_an_empty_answer_leaves_the_file(self): + self.synced("a-thing", 101) + before = self.raw("a-thing") + self.fake.answer_override = None + real_api = self.fake.api + self.fake.api = lambda *a, **kw: (real_api(*a, **kw), None)[1] + with mock.patch.object(_gitea, "api", self.fake.api), \ + self.assertRaises(SystemExit): + self.run_close("a-thing") + self.assertUnchanged("a-thing", before) + + def test_the_error_says_nothing_local_changed(self): + self.synced("a-thing", 101) + self.fake.answer_override = {"ok": True} + with self.assertRaises(SystemExit): + self.run_close("a-thing") + self.assertIn("Nothing local was changed", self.err.getvalue()) + + def test_a_failure_partway_through_keeps_the_rest(self): + """Two issues, the second one is not confirmed. The first is + legitimately closed; the second's file still says open.""" + self.synced("aaa-thing", 101) + self.synced("zzz-thing", 102) + before = self.raw("zzz-thing") + + real = self.fake.api + seen = [] + + def once(login, endpoint, method="GET", payload=None, **kw): + got = real(login, endpoint, method, payload, **kw) + if method != "GET": + seen.append(endpoint) + return {"nope": True} if len(seen) > 1 else got + + with mock.patch.object(_gitea, "api", once), \ + self.assertRaises(SystemExit): + self.run_close("aaa-thing", "zzz-thing") + + self.assertEqual(self.state_on_disk("aaa-thing"), "closed") + self.assertUnchanged("zzz-thing", before, "its write was not confirmed") + + +# -------------------------------------------------------------------------- +# the pure parts +# -------------------------------------------------------------------------- + +class ConfirmedTest(unittest.TestCase): + """The gate itself. Everything below it rewrites a file.""" + + def test_a_matching_close_is_confirmed(self): + self.assertTrue(close.confirmed({"number": 42, "state": "closed"}, 42, "closed")) + + def test_a_mismatched_number_is_not(self): + self.assertFalse(close.confirmed({"number": 43, "state": "closed"}, 42, "closed")) + + def test_the_wrong_state_is_not(self): + self.assertFalse(close.confirmed({"number": 42, "state": "open"}, 42, "closed")) + + def test_a_missing_state_is_not(self): + self.assertFalse(close.confirmed({"number": 42}, 42, "closed")) + + def test_none_and_lists_are_not(self): + self.assertFalse(close.confirmed(None, 42, "closed")) + self.assertFalse(close.confirmed([{"number": 42, "state": "closed"}], 42, "closed")) + + def test_true_is_not_a_number(self): + self.assertFalse(close.confirmed({"number": True, "state": "closed"}, 1, "closed")) + + def test_a_string_number_is_not(self): + self.assertFalse(close.confirmed({"number": "42", "state": "closed"}, 42, "closed")) + + +class KeyFormTest(unittest.TestCase): + """A slug and a key are two vocabularies that must not collide.""" + + def test_keys_are_keys(self): + for k in ("42", "#42", "owner/repo#42", + "https://git.example/owner/repo/issues/42"): + self.assertTrue(close.looks_like_key(k), k) + + def test_slugs_are_not_keys(self): + for s in ("a-thing", "wire-sqlc-appclick", "close-issues-through-a-script"): + self.assertFalse(close.looks_like_key(s), s) + + +class LedgerPairsTest(unittest.TestCase): + + def setUp(self): + self.m = {"%s#7" % REPO: "a-thing", "other/repo#7": "b-thing", + "not-a-key": "c-thing"} + + def test_it_filters_by_repo(self): + self.assertEqual(close.ledger_pairs(self.m, REPO), [(REPO, 7, "a-thing")]) + + def test_without_a_repo_it_keeps_everything_parseable(self): + got = close.ledger_pairs(self.m) + self.assertEqual(sorted(s for _r, _n, s in got), ["a-thing", "b-thing"]) + + def test_an_ambiguous_number_exits(self): + pairs = close.ledger_pairs(self.m) + with self.assertRaises(SystemExit): + with contextlib.redirect_stderr(io.StringIO()): + close.resolve("7", {}, pairs) + + +class AmbiguityTest(StoreTestCase): + """Two repos, one number, no --repo: settle it rather than guess.""" + + def test_the_error_points_at_repo(self): + _gitea.save_map(self.root, {"%s#7" % REPO: "a-thing", + "other/repo#7": "b-thing"}) + err = io.StringIO() + args = ["close.py", "--out", self.root, "7"] + with mock.patch.object(sys, "argv", args), \ + contextlib.redirect_stdout(io.StringIO()), \ + contextlib.redirect_stderr(err), \ + self.assertRaises(SystemExit): + close.main() + self.assertIn("--repo", err.getvalue()) + + +class RepoOfTheKeyTest(StoreTestCase): + """A key that names its own repo is sent there, not to whatever repo the + CWD happens to be — otherwise `#42` closes somebody else's issue.""" + + def run_bare(self, *argv): + """No `--repo`, so the ids have to say where they live.""" + self.out, self.err = io.StringIO(), io.StringIO() + args = ["close.py", "--out", self.root] + list(argv) + with mock.patch.object(sys, "argv", args), \ + contextlib.redirect_stdout(self.out), \ + contextlib.redirect_stderr(self.err): + close.main() + return self.out.getvalue(), self.err.getvalue() + + def test_a_foreign_key_goes_to_its_own_repo(self): + self.run_bare("other/repo#42") + self.assertEqual(self.fake.writes()[0][1], "repos/other/repo/issues/42") + + def test_a_slug_goes_to_the_repo_its_gitea_field_names(self): + self.synced("a-thing", 101) + self.run_bare("a-thing") + self.assertEqual(self.fake.writes()[0][1], "%s/issues/101" % BASE) + + def test_two_repos_in_one_run_is_a_question_not_a_guess(self): + self.synced("a-thing", 101) + with self.assertRaises(SystemExit): + self.run_bare("a-thing", "other/repo#42") + self.assertIn("one repo", self.err.getvalue()) + self.assertEqual(self.fake.calls, []) + + def test_an_explicit_repo_settles_it(self): + self.synced("a-thing", 101) + self.run_close("a-thing", "other/repo#42") + self.assertEqual({c[1] for c in self.fake.writes()}, + {"%s/issues/101" % BASE, "%s/issues/42" % BASE}) + + +class NoStoreTest(StoreTestCase): + """A number needs no local file, and a store that is not there is not an + error — closing an issue whose copy push dropped is the normal case.""" + + def test_a_number_closes_with_no_store_at_all(self): + missing = os.path.join(self.root, "nowhere") + self.fake.states[303] = "open" + args = ["close.py", "--repo", REPO, "--out", missing, "303"] + with mock.patch.object(sys, "argv", args), \ + contextlib.redirect_stdout(io.StringIO()), \ + contextlib.redirect_stderr(io.StringIO()): + close.main() + self.assertEqual(self.fake.states[303], "closed") + self.assertFalse(os.path.isdir(missing), "no store was conjured") + + +class PayloadFileTest(StoreTestCase): + """The request body goes to the transport's own scratchpad. + + Not to a directory this script picks: `close.py` names the payload and + nothing else, the way every other caller does. Where PAYLOAD_ROOT lands is + _gitea's business, and test_payload_root.py is where that is tested.""" + + def test_the_payload_lands_in_the_transports_scratchpad(self): + self.synced("a-thing", 101) + payloads = os.path.join(self.root, "payload") + with mock.patch.object(_gitea, "PAYLOAD_ROOT", payloads), \ + mock.patch.object(_gitea, "api", REAL_API), \ + mock.patch.object( + _gitea, "subprocess", + types.SimpleNamespace(run=lambda cmd, **kw: types.SimpleNamespace( + returncode=0, stderr="", + stdout=json.dumps({"number": 101, "state": "closed"})))): + self.run_close("a-thing") + p = os.path.join(payloads, "state-101.json") + self.assertTrue(os.path.isfile(p)) + with open(p) as f: + self.assertEqual(json.load(f), {"state": "closed"}) + + +if __name__ == "__main__": + unittest.main() diff --git a/tests/test_drop_after_push.py b/tests/test_drop_after_push.py index a65a1af..a581982 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] @@ -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() 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 new file mode 100644 index 0000000..f89e634 --- /dev/null +++ b/tests/test_payload_root.py @@ -0,0 +1,250 @@ +#!/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") +AUTH_SCRIPTS = os.path.join(REPO, "skills", "auth", "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) + # 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 + 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_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() 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() 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] diff --git a/tests/test_store_path.py b/tests/test_store_path.py index 0d9fabb..ff05756 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: