diff --git a/client/granthi_sync_client.py b/client/granthi_sync_client.py index 00a49f1..91955b6 100644 --- a/client/granthi_sync_client.py +++ b/client/granthi_sync_client.py @@ -104,6 +104,10 @@ FORGE_MAX_PAGES = 40 # 2000 repos; a guard against an unbounded paging loop # Under refs/heads a machine taking a snapshot every 30s would bury the # user's real branches. BACKUP_NS = "refs/granthi-backup" +_SNAPSHOT_IDENT = {"GIT_AUTHOR_NAME": "granthi-sync", + "GIT_AUTHOR_EMAIL": "sync@granthi.local", + "GIT_COMMITTER_NAME": "granthi-sync", + "GIT_COMMITTER_EMAIL": "sync@granthi.local"} SNAPSHOT_TS_FMT = "%Y%m%dT%H%M%SZ" # Retention. Unbounded snapshots are a disk leak with no way to use them, so @@ -301,15 +305,33 @@ def build_snapshot(folder): args = ["commit-tree", tree, "-m", f"granthi snapshot: {ts}"] if head: args += ["-p", head] + # The worktree tree alone loses STAGED-ONLY work. Stage a hunk, edit the + # file further, lose the laptop, and the snapshot holds only the later + # worktree version -- the carefully staged one is gone. git itself keeps + # index and worktree as separate states, so the backup must too. + # The user's real index is read WITHOUT touching it, and when it differs + # from both HEAD and the worktree it rides along as a second parent, so + # it is reachable from the snapshot. (codex review, P2.) + rc_idx, index_tree = git(folder, "write-tree", check=False) + index_tree = index_tree.strip() + if rc_idx == 0 and index_tree and index_tree != tree: + head_tree_now = "" + if head: + head_tree_now = git(folder, "rev-parse", f"{head}^{{tree}}")[1].strip() + if index_tree != head_tree_now: + icommit_args = ["commit-tree", index_tree, "-m", + f"granthi snapshot (staged): {ts}"] + if head: + icommit_args += ["-p", head] + rc_ic, icommit = git(folder, *icommit_args, check=False, + env=_SNAPSHOT_IDENT) + if rc_ic == 0 and icommit.strip(): + args += ["-p", icommit.strip()] # Snapshots are parented on HEAD and nothing else -- deliberately NOT # chained to the previous snapshot. Chaining would keep every old # snapshot reachable from the newest one, so pruning a ref would free # nothing and retention would be decorative. - _, commit = git(folder, *args, - env={"GIT_AUTHOR_NAME": "granthi-sync", - "GIT_AUTHOR_EMAIL": "sync@granthi.local", - "GIT_COMMITTER_NAME": "granthi-sync", - "GIT_COMMITTER_EMAIL": "sync@granthi.local"}) + _, commit = git(folder, *args, env=_SNAPSHOT_IDENT) return commit.strip(), tree finally: if os.path.exists(tmp_index): @@ -838,7 +860,7 @@ def clone_one(cfg, full_name, dest, mode=None): return meta -def resolve_granted(cfg, want, repos=None): +def resolve_granted(cfg, want, repos=None, truncated=False): """Turn what the user typed into the repo the forge actually grants them. `get notes` used to mean `/notes` and nothing else. On a real @@ -857,7 +879,7 @@ def resolve_granted(cfg, want, repos=None): return parse_repo_arg(want, cfg["login"]) if repos is None: try: - repos, _truncated = list_repos(cfg) + repos, truncated = list_repos(cfg) except (SystemExit, ValueError, OSError) as e: # Best effort. If the listing is unreachable, a name the user # typed in full must still clone, and a bare name should degrade @@ -876,8 +898,18 @@ def resolve_granted(cfg, want, repos=None): f"{want!r} is ambiguous — {len(matches)} repos you can see have " f"that name:\n{listed}\nRe-run with the owner, e.g. " f"granthi-sync get {sorted(matches)[0]}") - # Nothing granted by that name. Fall back to the caller's own namespace so - # the message names a concrete repo instead of a guess. + # Nothing granted by that name. If the listing was TRUNCATED the answer is + # unknown rather than absent -- falling back to / could clone a + # different repo that happens to exist under your own account. Refuse and + # ask for the owner. (codex review, P3.) + if truncated: + raise SystemExit( + f"could not confirm {want!r}: your repo list was truncated at " + f"{FORGE_MAX_PAGES * FORGE_PAGE_LIMIT} repos, so a match may exist " + f"beyond it. Re-run with the owner, e.g. " + f"granthi-sync get /{want}") + # Otherwise the listing was complete and simply has no such repo; name a + # concrete one so the clone error is precise. return parse_repo_arg(want, cfg["login"]) @@ -1341,13 +1373,14 @@ def cmd_bootstrap(args): into = os.path.abspath(args.into or ".") print(f"workspace: {name}\n") - granted_repos, _trunc = list_repos(cfg) + granted_repos, granted_truncated = list_repos(cfg) granted = {r.get("full_name") for r in granted_repos} pulled = skipped = denied = 0 for repo in ws["repos"]: want = repo["name"] try: - full = resolve_granted(cfg, want, repos=granted_repos) + full = resolve_granted(cfg, want, repos=granted_repos, + truncated=granted_truncated) except SystemExit as e: log(f"AMBIGUOUS {want}: {e}") denied += 1 diff --git a/server/granthi_link.py b/server/granthi_link.py index 3b8298c..3701c43 100644 --- a/server/granthi_link.py +++ b/server/granthi_link.py @@ -404,6 +404,23 @@ class IdentityStore: self._write(data) return pending + def consume_invite(self, email, repo): + """Drop ONE applied grant, leaving any that failed still pending. + + The whole-list `take_invites` is what made a transient forge error + permanent; this removes only what actually landed. + """ + data = self._load() + book = data.get("invites") or {} + key = (email or "").strip().lower() + entry = [e for e in book.get(key, []) if e.get("repo") != repo] + if entry: + book[key] = entry + else: + book.pop(key, None) + data["invites"] = book + self._write(data) + def peek_invites(self, email): book = self._load().get("invites") or {} return list(book.get((email or "").strip().lower(), [])) @@ -1038,8 +1055,13 @@ class LinkService: # the only thing tying the promise to this person. return [] with self.state.lock: - pending = self.state.take_invites(email) + pending = self.state.peek_invites(email) applied = [] + # PEEK, not take. Consuming the invite first means a transient 502 from + # the forge destroys it: the person links successfully, gets no access, + # and re-linking never retries because the promise is gone. Only the + # grants that actually landed are removed, so a failure is retried on + # the next link instead of being silently lost. (codex review, P2.) for grant in pending: status, resp = http_json( "PUT", @@ -1050,6 +1072,8 @@ class LinkService: body={"permission": grant.get("permission", "write")}) if status in (200, 204): applied.append(grant["repo"]) + with self.state.lock: + self.state.consume_invite(email, grant["repo"]) self.audit.write("invite.applied", login=login, repo=grant["repo"], permission=grant.get("permission"), diff --git a/tests/test_client.py b/tests/test_client.py index fc732ee..a94c62f 100644 --- a/tests/test_client.py +++ b/tests/test_client.py @@ -1310,5 +1310,53 @@ class TestDeviceFlowOutputIsVisible(unittest.TestCase): self.assertTrue(all(seen[:2]), "the URL and code must be printed with flush=True") + +class TestCodexReviewFindings(GitScenarioBase): + """Regressions for the three issues codex found in today's merged work.""" + + def test_staged_only_work_survives_a_snapshot(self): + """[P2] Stage a hunk, edit further, lose the laptop: the staged version + must still be recoverable, not just the later worktree one.""" + self.write(self.local, "a.txt", "committed") + run_git(self.local, "add", "-A") + run_git(self.local, "commit", "-m", "base") + self.write(self.local, "a.txt", "THE CAREFULLY STAGED VERSION") + run_git(self.local, "add", "a.txt") # staged + self.write(self.local, "a.txt", "later scratch edit") # worktree moved on + + commit, tree = client.build_snapshot(self.local) + + # the worktree state is the snapshot's own tree + self.assertEqual(run_git(self.local, "show", f"{commit}:a.txt"), + "later scratch edit") + # ...and the staged state is reachable through the extra parent + parents = run_git(self.local, "log", "-1", "--format=%P", commit).split() + staged = [p for p in parents + if run_git(self.local, "show", f"{p}:a.txt") + == "THE CAREFULLY STAGED VERSION"] + self.assertTrue(staged, f"staged version unreachable from {parents}") + + def test_a_snapshot_does_not_disturb_the_index(self): + self.write(self.local, "a.txt", "one") + run_git(self.local, "add", "-A") + run_git(self.local, "commit", "-m", "base") + self.write(self.local, "a.txt", "staged") + run_git(self.local, "add", "a.txt") + before = run_git(self.local, "status", "--porcelain") + client.build_snapshot(self.local) + self.assertEqual(run_git(self.local, "status", "--porcelain"), before) + + def test_a_truncated_listing_refuses_instead_of_guessing(self): + """[P3] A name that is merely beyond the page cap must not resolve to a + different repo that happens to exist under your own account.""" + cfg = {"login": "alice", "gitea_base": "http://forge.example", "token": "t"} + with self.assertRaises(SystemExit) as e: + client.resolve_granted(cfg, "notes", repos=[], truncated=True) + self.assertIn("truncated", str(e.exception)) + # a complete listing still falls back, because absence is then real + self.assertEqual( + client.resolve_granted(cfg, "notes", repos=[], truncated=False), + "alice/notes") + if __name__ == "__main__": unittest.main() diff --git a/tests/test_server.py b/tests/test_server.py index 9a2a393..98be554 100644 --- a/tests/test_server.py +++ b/tests/test_server.py @@ -960,6 +960,47 @@ class TestInvites(ServiceTestBase): _, mine = self.svc.audit_read({"token": self.alice["token"]}) self.assertIn("invite", [e["event"] for e in mine["events"]]) + +class TestInviteSurvivesAFailedGrant(ServiceTestBase): + """[P2, codex] A transient forge error must not destroy the promise.""" + + def setUp(self): + super().setUp() + self.enable_test_mode() + self.alice = self.stub_link("s1", "alice", email="alice@x.test", + verified=True, device_id="dev-a")[1] + StubUpstream.state["repo_owner"]["alice/notes"] = "alice" + StubUpstream.state["repo_owner"]["alice/reports"] = "alice" + + def test_a_failed_grant_leaves_the_invite_pending_for_next_time(self): + self.svc.invite({"token": self.alice["token"], "email": "carol@x.test", + "repos": ["notes"]}) + # the forge loses the repo mid-flight -> the PUT 404s + StubUpstream.state["repo_owner"].pop("alice/notes") + status, resp = self.stub_link("s9", "carol", email="carol@x.test", + verified=True, device_id="dev-c") + self.assertEqual(status, 200) # sign-in still succeeds + self.assertIsNone(resp.get("granted_repos")) + # the promise is STILL THERE rather than silently consumed + self.assertEqual([g["repo"] for g in self.svc.state.peek_invites("carol@x.test")], + ["alice/notes"]) + # and it lands on the next link, once the repo is back + StubUpstream.state["repo_owner"]["alice/notes"] = "alice" + status, resp = self.stub_link("s9", "carol", email="carol@x.test", + verified=True, device_id="dev-c2") + self.assertEqual(resp.get("granted_repos"), ["alice/notes"]) + self.assertEqual(self.svc.state.peek_invites("carol@x.test"), []) + + def test_a_partial_failure_only_consumes_what_landed(self): + self.svc.invite({"token": self.alice["token"], "email": "dan@x.test", + "repos": ["notes", "reports"]}) + StubUpstream.state["repo_owner"].pop("alice/reports") # one of two fails + _, resp = self.stub_link("s10", "dan", email="dan@x.test", + verified=True, device_id="dev-d") + self.assertEqual(resp.get("granted_repos"), ["alice/notes"]) + self.assertEqual([g["repo"] for g in self.svc.state.peek_invites("dan@x.test")], + ["alice/reports"]) + if __name__ == "__main__": unittest.main()