diff --git a/.agents/skills/session-handoff/SKILL.md b/.agents/skills/session-handoff/SKILL.md index 5dd83ca7..b84ad5f5 100644 --- a/.agents/skills/session-handoff/SKILL.md +++ b/.agents/skills/session-handoff/SKILL.md @@ -320,7 +320,8 @@ and answers as an empty chain does, and `new` refuses until it is given `--creat creates the one `handoff` label, confirms it, and then files the first link. Issues turned off stop it before any write, naming the command that turns them on. Never apply the fleet label set to such a fork. A repository under another owner, or an unregistered one of the owner's that is not a fork, -refuses before any write. +refuses before any write. `new` and `link` refuse both whatever the label state, since a label on +such a repository opens no write there, and that refusal bounds no read. Creating an issue, commenting on one, closing one, and editing a body are each outward-facing writes. `new` creates, comments, and closes, the label riding inside the one create call rather than diff --git a/.claude-plugin/fleet-skills/.source-digests/session-handoff b/.claude-plugin/fleet-skills/.source-digests/session-handoff index 24500fc8..6d54cf54 100644 --- a/.claude-plugin/fleet-skills/.source-digests/session-handoff +++ b/.claude-plugin/fleet-skills/.source-digests/session-handoff @@ -1 +1 @@ -c1684259d7d09c94 +71ea890bf0050608 diff --git a/.claude-plugin/fleet-skills/skills/session-handoff/SKILL.md b/.claude-plugin/fleet-skills/skills/session-handoff/SKILL.md index 5dd83ca7..b84ad5f5 100644 --- a/.claude-plugin/fleet-skills/skills/session-handoff/SKILL.md +++ b/.claude-plugin/fleet-skills/skills/session-handoff/SKILL.md @@ -320,7 +320,8 @@ and answers as an empty chain does, and `new` refuses until it is given `--creat creates the one `handoff` label, confirms it, and then files the first link. Issues turned off stop it before any write, naming the command that turns them on. Never apply the fleet label set to such a fork. A repository under another owner, or an unregistered one of the owner's that is not a fork, -refuses before any write. +refuses before any write. `new` and `link` refuse both whatever the label state, since a label on +such a repository opens no write there, and that refusal bounds no read. Creating an issue, commenting on one, closing one, and editing a body are each outward-facing writes. `new` creates, comments, and closes, the label riding inside the one create call rather than diff --git a/.github/skills/session-handoff/SKILL.md b/.github/skills/session-handoff/SKILL.md index 5dd83ca7..b84ad5f5 100644 --- a/.github/skills/session-handoff/SKILL.md +++ b/.github/skills/session-handoff/SKILL.md @@ -320,7 +320,8 @@ and answers as an empty chain does, and `new` refuses until it is given `--creat creates the one `handoff` label, confirms it, and then files the first link. Issues turned off stop it before any write, naming the command that turns them on. Never apply the fleet label set to such a fork. A repository under another owner, or an unregistered one of the owner's that is not a fork, -refuses before any write. +refuses before any write. `new` and `link` refuse both whatever the label state, since a label on +such a repository opens no write there, and that refusal bounds no read. Creating an issue, commenting on one, closing one, and editing a body are each outward-facing writes. `new` creates, comments, and closes, the label riding inside the one create call rather than diff --git a/scripts/README.md b/scripts/README.md index 17fd3c05..4c9e2a1e 100644 --- a/scripts/README.md +++ b/scripts/README.md @@ -261,7 +261,7 @@ The invariant is one open handoff per track per repository, read from the metada `new` creates the new issue, comments the forward link on the previous one, and then closes it, in that order, printing each step's own result. Creating first means a failure at any later step leaves a discoverable new issue rather than a closed chain with no successor, and `link` finishes what a failure interrupted without a second issue being filed for it. `link` takes no `--track`, so the two issues' own metadata blocks are the only thing that can say they belong to one lane, and it refuses an issue named as its own predecessor and a pair whose blocks name different tracks rather than closing one lane's handoff into another's. Nothing suppresses a write's output or converts a failure into success, per [GOVERNANCE.md][governance] "Repository Boundaries and Write Safety". Where the caller names an issue, which is `link` alone, that issue is read live before the write and only what the read returned is written back, and every other identifier a write consumes comes from a read in the same run rather than being constructed. A close is confirmed by reading the state back, because a write that appears to have failed may have succeeded on the server. -`--repo` is required and carries no default, for the reason `pr_review.py` states about a pull request number: an issue number resolves in every repository, and a chain read out of the wrong one is well formed. `--track` defaults to `default`, and omitting it on a named lane does more than read the wrong chain, since `new` then files onto `default` and comments on and closes whatever that lane had open. A fleet repository missing the `handoff` label refuses, naming `repo-config/configure.sh apply`, never with a degraded empty answer, because `decision` was declared on the hub and applied nowhere else and every downstream session then enumerated an empty queue and reported it healthy. The one exception is a fork under the registry's owner that the registry does not list, such as one kept for an upstream contribution, where no issue can carry a label that does not exist: a read there warns and answers as an empty chain does, and `new --create-label` creates the one `handoff` label from `repo-config/labels.json`, confirms it, and files the first link, refusing first where the fork has issues turned off. A repository missing the label under another owner, or an unregistered one of the owner's that is not a fork, refuses before any write, the first because these calls run as subprocesses a write guard on the caller never sees and the second because it is registry drift a lone label would hide. Exit `0` is success, `1` a refusal the caller can act on, and `2` the command not having run to an answer, so the two never share a code: a usage error is moved off argparse's own `2` onto `1` because it is a refusal, and an exception nobody modeled is caught onto `2` rather than reaching CPython's own `1`. A body over 12 KB warns and one over 60 KB refuses, under GitHub's own 65536-character limit, and both are backstops against a body GitHub would reject: what bounds a handoff is the Skill's per-section rules. +`--repo` is required and carries no default, for the reason `pr_review.py` states about a pull request number: an issue number resolves in every repository, and a chain read out of the wrong one is well formed. `--track` defaults to `default`, and omitting it on a named lane does more than read the wrong chain, since `new` then files onto `default` and comments on and closes whatever that lane had open. A fleet repository missing the `handoff` label refuses, naming `repo-config/configure.sh apply`, never with a degraded empty answer, because `decision` was declared on the hub and applied nowhere else and every downstream session then enumerated an empty queue and reported it healthy. The one exception is a fork under the registry's owner that the registry does not list, such as one kept for an upstream contribution, where no issue can carry a label that does not exist: a read there warns and answers as an empty chain does, and `new --create-label` creates the one `handoff` label from `repo-config/labels.json`, confirms it, and files the first link, refusing first where the fork has issues turned off. A repository missing the label under another owner, or an unregistered one of the owner's that is not a fork, refuses before any write, the first because these calls run as subprocesses a write guard on the caller never sees and the second because it is registry drift a lone label would hide. The two writing subcommands, `new` and `link`, refuse both whatever the label state, checked before the label is read, so a label a stranger's repository happens to carry opens no write there. That is an in-process refusal of the kind `pr_review.py` makes, with the owner read from the registry rather than from `origin`, and it bounds no read. Exit `0` is success, `1` a refusal the caller can act on, and `2` the command not having run to an answer, so the two never share a code: a usage error is moved off argparse's own `2` onto `1` because it is a refusal, and an exception nobody modeled is caught onto `2` rather than reaching CPython's own `1`. A body over 12 KB warns and one over 60 KB refuses, under GitHub's own 65536-character limit, and both are backstops against a body GitHub would reject: what bounds a handoff is the Skill's per-section rules. `--dry-run` prints the calls a writing subcommand would make and sends none of them, which is the first thing anyone wants before trusting this to close an issue. It sits on the one path every call goes through rather than on each write separately. diff --git a/scripts/handoff.py b/scripts/handoff.py index efa19b31..71eed74c 100755 --- a/scripts/handoff.py +++ b/scripts/handoff.py @@ -68,6 +68,12 @@ `new --create-label` creates the one `handoff` label and nothing else. Every other repository missing the label refuses, a repository under another owner and an unregistered repository of the owner's that is not a fork among them. + +The two writing subcommands, `new` and `link`, are bounded whatever the label state, an +in-process refusal of the kind `pr_review.py` makes, with the owner read from the registry. They +refuse a repository under another owner before any call, and an unregistered repository of the +owner's unless it is a fork, since these writes run as subprocesses a write guard on the caller +never sees. That write scope bounds no read. """ from __future__ import annotations @@ -268,13 +274,47 @@ def registry() -> tuple[str, set[str]]: return owner.lower(), names -def repo_flags(repo: str) -> dict[str, bool]: - """Whether the repository has issues turned on and whether it is a fork, read live.""" - data = gh_json(["repo", "view", repo, "--json", "hasIssuesEnabled,isFork"]) +def repo_flags(a: argparse.Namespace) -> dict[str, bool]: + """Whether the repository has issues turned on and whether it is a fork, read live once a run. + + The write scope and the missing-label path both ask, so the answer rides on the parsed + arguments rather than costing a second request. + """ + cached = getattr(a, "repo_flags", None) + if cached is not None: + return cached + data = gh_json(["repo", "view", a.repo, "--json", "hasIssuesEnabled,isFork"]) fields = ("hasIssuesEnabled", "isFork") if not isinstance(data, dict) or not all(isinstance(data.get(f), bool) for f in fields): - raise Execution(f"repo view for {repo} returned no {' and '.join(fields)}: {data!r}") - return {field: data[field] for field in fields} + raise Execution(f"repo view for {a.repo} returned no {' and '.join(fields)}: {data!r}") + a.repo_flags = {field: data[field] for field in fields} + return a.repo_flags + + +def require_write_scope(a: argparse.Namespace) -> None: + """Refuse a write outside the registry's owner, or to an unregistered repository not a fork. + + It runs before the label is read, so a label a stranger's repository happens to carry opens no + write there. A registered repository costs no request, and the reads are never bounded here. + """ + if a.cmd in READS: + return + owner, names = registry() + repo_owner, _, name = a.repo.partition("/") + if repo_owner.lower() != owner: + raise Refusal( + f"{a.repo} is not under {owner}, so this script writes nothing there. A different " + "owner goes through the runbook's own `gh` path, where the write guard reads the " + "maintainer's grant." + ) + if name.lower() in names: + return + if not repo_flags(a)["isFork"]: + raise Refusal( + f"{a.repo} is neither in registry/repos.json nor a fork, so it is registry drift " + "rather than a fork keeping state, and this script writes nothing there. Register it, " + f"then apply the fleet label set: {APPLY}" + ) def label_definition() -> tuple[str, str]: @@ -332,9 +372,8 @@ def without_label(a: argparse.Namespace) -> int | None: The body is read once, before the label is created, and that read is the one `new` files, so a body `new` would refuse leaves no label behind. Everything else refuses before any write. A - repository under another owner never gets a label created here, since these calls run as - subprocesses a write guard on the caller never sees. An unregistered repository of the owner's - that is not a fork is registry drift rather than a fork, and a lone label there would hide it. + repository under another owner, or an unregistered one of the owner's that is not a fork, + refuses here for a read and in `require_write_scope` for a write, which runs first. """ repo = a.repo create = getattr(a, "create_label", False) @@ -349,7 +388,7 @@ def without_label(a: argparse.Namespace) -> int | None: if name.lower() in names: flag = " `--create-label` is for an unregistered fork." if create else "" raise Refusal(f"{missing}{flag} Apply the fleet label set from a hub checkout: {APPLY}") - flags = repo_flags(repo) + flags = repo_flags(a) if not flags["isFork"]: raise Refusal( f"{missing} It is neither in registry/repos.json nor a fork, so it is registry drift " @@ -1243,6 +1282,7 @@ def main(argv: list[str] | None = None) -> int: if getattr(a, flag, None) is not None and getattr(a, flag) < 1: ap.error(f"--{flag} takes an issue number, so it cannot be below 1") try: + require_write_scope(a) if not label_present(a.repo): answered = without_label(a) if answered is not None: diff --git a/tests/test_handoff.py b/tests/test_handoff.py index facbde26..c73842c5 100755 --- a/tests/test_handoff.py +++ b/tests/test_handoff.py @@ -32,6 +32,19 @@ sys.path.insert(0, str(SCRIPTS)) import handoff +REAL_REGISTRY = handoff.registry + + +def setUpModule() -> None: + """Read `o/r` as a fleet repository unless a case says otherwise. + + The write scope reads the registry before every write, and the real one lists no `o`, so + without this every write case would test the scope refusal instead of what it names. + """ + patcher = unittest.mock.patch.object(handoff, "registry", lambda: ("o", {"r"})) + patcher.start() + unittest.addModuleCleanup(patcher.stop) + def marked(body: str, track: str, round_: int, previous: int | None) -> str: return handoff.with_marker(body, track, round_, previous) @@ -410,8 +423,8 @@ def test_new_with_the_flag_creates_the_label_then_the_first_link(self) -> None: self.assertEqual( [c[:2] for c in fake.calls], [ - ["label", "list"], ["repo", "view"], + ["label", "list"], ["label", "create"], ["label", "list"], ["issue", "create"], @@ -429,7 +442,7 @@ def test_a_dry_run_with_the_flag_writes_nothing(self) -> None: self.assertEqual(code, 0, out) self.assertIn(f"would run: gh label create {handoff.LABEL}", out) self.assertIn("would run: gh issue create", out) - self.assertEqual([c[:2] for c in fake.calls], [["label", "list"], ["repo", "view"]]) + self.assertEqual([c[:2] for c in fake.calls], [["repo", "view"], ["label", "list"]]) self.assertFalse(fake.label) def test_a_body_new_would_refuse_creates_no_label(self) -> None: @@ -518,7 +531,7 @@ def test_a_repository_under_another_owner_refuses_before_any_other_call(self) -> "--create-label", ) self.assertEqual(code, 1) - self.assertEqual([c[:2] for c in fake.calls], [["label", "list"]]) + self.assertEqual([c[:2] for c in fake.calls], []) def test_an_unregistered_repository_that_is_not_a_fork_is_drift(self) -> None: """It belongs in the registry, so it refuses as drift rather than taking a lone label.""" @@ -530,7 +543,7 @@ def test_an_unregistered_repository_that_is_not_a_fork_is_drift(self) -> None: fake = FakeGh(label=False, fork=False) code, _, _ = self.new(fake, "--create-label") self.assertEqual(code, 1) - self.assertEqual([c[:2] for c in fake.calls], [["label", "list"], ["repo", "view"]]) + self.assertEqual([c[:2] for c in fake.calls], [["repo", "view"]]) def test_the_label_created_is_the_fleet_set_s_own_definition(self) -> None: rows = json.loads(handoff.LABELS.read_text(encoding="utf-8")) @@ -546,14 +559,65 @@ def test_link_refuses_without_the_label(self) -> None: code, _, err = run(fake, "link", "--repo", "o/r", "--new", "2", "--previous", "1") self.assertEqual(code, 1) self.assertIn("--create-label", err) - self.assertEqual([c[:2] for c in fake.calls], [["label", "list"], ["repo", "view"]]) + self.assertEqual([c[:2] for c in fake.calls], [["repo", "view"], ["label", "list"]]) + + +class WriteScopeCase(unittest.TestCase): + """The writes are bounded whatever the label state, since a label opens no write by itself.""" + + def setUp(self) -> None: + fleet(self, member=False) + + def new(self, fake: FakeGh, repo: str) -> tuple[int, str, str]: + return run(fake, "new", "--repo", repo, "--title", "T", "--body-file", body_file(self, "w")) + + def test_a_labeled_repository_under_another_owner_takes_no_call_and_no_write(self) -> None: + fake = FakeGh() + code, _, err = self.new(fake, "stranger/r") + self.assertEqual(code, 1) + self.assertIn("writes nothing there", err) + self.assertEqual(fake.calls, []) + fake = FakeGh() + code, _, _ = run(fake, "link", "--repo", "stranger/r", "--new", "2", "--previous", "1") + self.assertEqual(code, 1) + self.assertEqual(fake.calls, []) + + def test_a_labeled_unregistered_repository_that_is_not_a_fork_takes_no_write(self) -> None: + fake = FakeGh(fork=False) + code, _, err = self.new(fake, "o/r") + self.assertEqual(code, 1) + self.assertIn("registry drift", err) + self.assertEqual([c[:2] for c in fake.calls], [["repo", "view"]]) + fake = FakeGh(fork=False) + code, _, _ = run(fake, "link", "--repo", "o/r", "--new", "2", "--previous", "1") + self.assertEqual(code, 1) + self.assertEqual([c[:2] for c in fake.calls], [["repo", "view"]]) + + def test_a_labeled_unregistered_fork_files_and_asks_once_whether_it_is_one(self) -> None: + fake = FakeGh() + code, out, err = self.new(fake, "o/r") + self.assertEqual(code, 0, out + err) + self.assertEqual(sum(c[:2] == ["repo", "view"] for c in fake.calls), 1) + self.assertTrue(any(c[:2] == ["issue", "create"] for c in fake.calls)) + + def test_a_read_of_a_labeled_repository_under_another_owner_stays_open(self) -> None: + fake = FakeGh() + code, out, _ = run(fake, "tracks", "--repo", "stranger/r") + self.assertEqual(code, 0) + self.assertIn("(no open", out) + + def test_a_registered_repository_costs_no_extra_request(self) -> None: + fleet(self) + fake = FakeGh() + self.assertEqual(self.new(fake, "o/r")[0], 0) + self.assertFalse(any(c[:2] == ["repo", "view"] for c in fake.calls)) class FleetRegistryCase(unittest.TestCase): """Membership is read from the hub's registry, and an unreadable one is not an empty one.""" def test_the_registry_reads_lowercased_with_the_hub_in_it(self) -> None: - owner, names = handoff.registry() + owner, names = REAL_REGISTRY() self.assertEqual(owner, "ptr727") self.assertIn("projecttemplate", names) @@ -575,7 +639,7 @@ def test_an_unreadable_registry_fails_rather_than_reading_as_empty(self) -> None unittest.mock.patch.object(handoff, "REGISTRY", missing), self.assertRaises(handoff.Execution), ): - handoff.registry() + REAL_REGISTRY() def test_a_malformed_registry_fails_rather_than_reading_as_empty(self) -> None: bad = Path(tempfile.mkdtemp()) / "repos.json" @@ -587,7 +651,7 @@ def test_a_malformed_registry_fails_rather_than_reading_as_empty(self) -> None: unittest.mock.patch.object(handoff, "REGISTRY", bad), self.assertRaises(handoff.Execution) as caught, ): - handoff.registry() + REAL_REGISTRY() self.assertIn("could not read the fleet registry", str(caught.exception))