fix: resolve the issue store from the project, not the plugin
`issue.store_root` and `_gitea.PAYLOAD_ROOT` were anchored on `__file__`, on
the reasoning that where an installation keeps its files is a fact about the
installation. That holds for an installation and not for a store.
Installed, the plugin therefore resolved every project's issues inside its own
directory — and a plugin cache is versioned, so the store moved on each
update:
~/.claude/plugins/cache/tea/tea/2.0.0/tmp/issues 5 files, 2 origin: local
~/.claude/plugins/cache/tea/tea/2.1.0/tmp/issues 12 files
~/.claude/plugins/cache/claude-skills/tea/2.2.0/ empty, the current one
Issues written from one project were invisible from the next, and an `origin:
local` file — which IS the issue, the only copy — was stranded a version bump
at a time. Two of them were.
The store is a fact about the project, exactly as the login pin is. So the
anchor is now an explicit marker an operator creates, `.tea/`, searched for up
from $CLAUDE_PROJECT_DIR and then cwd — the pin's order, so the two cannot
disagree about which project this is. Inferred markers were tried and are worse
than useless here: `.git` is in every clone including this plugin's own, and
the agents-sync hook writes an AGENTS.md next to every AGENTS.md, so the plugin
root always carried one and cwd never got a turn.
With no marker anywhere, `store_root()` is None and every entry point reports
which directories it searched. A store in a plausible-looking directory is the
failure this replaces, so nothing falls back to one.
- `.tea/` holds the store and the transport's scratchpad: `.tea/issues`,
`.tea/payload`. One marker, one walk, one gitignore line.
- `issue_init.py` creates it, moves an old `tmp/issues` store in rather than
copying, adds `.tea/` to `.gitignore`, and refuses to pick a winner when both
sides hold the same file name.
- A linked worktree has no marker — it is gitignored — and reaches the main
checkout's store by the hop the pin already took.
- `parents`, `gitdir_of` and `main_worktree` move from `pin.py` into the domain
and `pin.py` imports them. The domain depends on nothing, so it is the layer
all three callers can borrow from, and the walk stays written once: the
guard, the transport and the store cannot disagree about a directory.
The suite stopped copying the script layers into its fixtures. That is what hid
this: with the scripts inside the fixture, the installation and the project
were the same directory. They are now deliberately far apart, and a regression
test asserts the plugin tree gains no files when commands run against a project
somewhere else.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -49,7 +49,11 @@ sys.stdout.write(json.dumps({"id": 1, "name": "created"})
|
||||
|
||||
|
||||
class FakeRepo(object):
|
||||
"""A self-contained repository with no store and no tmp/ at all."""
|
||||
"""An initialized project with no store and nothing under `.tea/` yet.
|
||||
|
||||
The scripts are NOT copied in: they stay at their real installed path, so
|
||||
what these tests exercise is a plugin operating on somebody else's project
|
||||
— which is every use of it but this repository's own."""
|
||||
|
||||
def __init__(self):
|
||||
self._tmp = tempfile.TemporaryDirectory()
|
||||
@@ -57,12 +61,7 @@ class FakeRepo(object):
|
||||
# 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(os.path.join(self.root, issue.MARKER)) # the project marker
|
||||
os.makedirs(self.path("sub", "deeper"))
|
||||
|
||||
# the login pin the transport insists on, local to this fixture
|
||||
@@ -85,18 +84,22 @@ class FakeRepo(object):
|
||||
|
||||
@property
|
||||
def store(self):
|
||||
return self.path("tmp", "issues")
|
||||
return self.path(*issue.STORE_PARTS)
|
||||
|
||||
@property
|
||||
def payloads(self):
|
||||
return self.path("tmp", "payload")
|
||||
return self.path(*_gitea.PAYLOAD_PARTS)
|
||||
|
||||
def script(self, layer, name):
|
||||
return self.path("skills", layer, "scripts", name)
|
||||
"""The real script, several directories away from this fixture."""
|
||||
return os.path.join(REPO, "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
|
||||
# the first anchor of both walks: left in place, every fixture would
|
||||
# resolve to this repository instead of itself
|
||||
env.pop("CLAUDE_PROJECT_DIR", None)
|
||||
env["PATH"] = self.bin + os.pathsep + env["PATH"]
|
||||
env["TEA_CALL_LOG"] = self.root
|
||||
p = subprocess.run([sys.executable, script] + list(args),
|
||||
@@ -118,26 +121,43 @@ class FakeRepo(object):
|
||||
|
||||
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 setUp(self):
|
||||
self.repo = FakeRepo()
|
||||
self.addCleanup(self.repo.cleanup)
|
||||
|
||||
def test_root_is_absolute_and_project_anchored(self):
|
||||
root = _gitea.payload_root(self.repo.path("sub", "deeper"))
|
||||
self.assertTrue(os.path.isabs(root), root)
|
||||
self.assertEqual(root, self.repo.payloads)
|
||||
|
||||
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))
|
||||
start = self.repo.path("sub", "deeper")
|
||||
payload = _gitea.payload_root(start)
|
||||
store = issue.store_root(start)
|
||||
self.assertNotEqual(payload, store)
|
||||
self.assertFalse(payload.startswith(store + os.sep))
|
||||
self.assertFalse(store.startswith(payload + 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("."))
|
||||
"""Named so the distinction is visible: `payload`, a sibling of the
|
||||
store under the marker, not a dotdir hiding among an issue's files."""
|
||||
payload = _gitea.payload_root(self.repo.root)
|
||||
self.assertEqual(os.path.basename(payload), "payload")
|
||||
self.assertEqual(os.path.dirname(payload),
|
||||
self.repo.path(issue.MARKER))
|
||||
|
||||
def test_no_project_means_no_payload_root(self):
|
||||
"""Same answer as the store gives: nothing, rather than a directory
|
||||
picked because it was the only one at hand."""
|
||||
plain = tempfile.TemporaryDirectory()
|
||||
self.addCleanup(plain.cleanup)
|
||||
self.assertIsNone(_gitea.payload_root(os.path.realpath(plain.name)))
|
||||
|
||||
def test_gitignore_covers_it(self):
|
||||
"""The rule is `tmp/` is ignored, not which file says so: this plugin
|
||||
lives under `plugins/` in a marketplace repo, and git reads every
|
||||
"""The rule is that the marker is ignored, not which file says so: this
|
||||
plugin lives under `plugins/` in a marketplace repo, and git reads every
|
||||
.gitignore on the way up. So walk up the same way git does."""
|
||||
ignored = set()
|
||||
d = REPO
|
||||
@@ -150,15 +170,16 @@ class TestPayloadRoot(unittest.TestCase):
|
||||
if parent == d or os.path.isdir(os.path.join(d, ".git")):
|
||||
break
|
||||
d = parent
|
||||
self.assertEqual(_gitea.PAYLOAD_PARTS[0], "tmp")
|
||||
self.assertIn("tmp/", ignored,
|
||||
self.assertEqual(_gitea.PAYLOAD_PARTS[0], issue.MARKER)
|
||||
self.assertIn(issue.MARKER + "/", 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)
|
||||
def test_resolution_follows_the_project_not_the_module(self):
|
||||
"""The bug, as a path fact: the scripts live somewhere else entirely,
|
||||
and the answer is still this project's directory."""
|
||||
self.assertEqual(_gitea.payload_root(self.repo.path("sub", "deeper")),
|
||||
self.repo.payloads)
|
||||
self.assertFalse(self.repo.payloads.startswith(REPO + os.sep))
|
||||
|
||||
|
||||
# --------------------------------------------------------------------------
|
||||
@@ -211,17 +232,18 @@ class TestLabelsTouchesNoStore(unittest.TestCase):
|
||||
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/")
|
||||
self.assertEqual(os.listdir(self.repo.path(issue.MARKER)), [],
|
||||
"a dry run left something behind under the marker")
|
||||
|
||||
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
|
||||
"""Run from a subdirectory: still one payload root, at the project
|
||||
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.path("sub", "deeper", issue.MARKER)))
|
||||
self.assertFalse(os.path.exists(self.repo.store))
|
||||
|
||||
|
||||
|
||||
Reference in New Issue
Block a user