Merge pull request 'fix: three defects found by the codex review of today's merged work' (#9) from fix/codex-review-findings into main

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": "[email protected]",
+                   "GIT_COMMITTER_NAME": "granthi-sync",
+                   "GIT_COMMITTER_EMAIL": "[email protected]"}
 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,
+        _, commit = git(folder, *args, env=_SNAPSHOT_IDENT)
-                        env={"GIT_AUTHOR_NAME": "granthi-sync",
-                             "GIT_AUTHOR_EMAIL": "[email protected]",
-                             "GIT_COMMITTER_NAME": "granthi-sync",
-                             "GIT_COMMITTER_EMAIL": "[email protected]"})
         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 `<your-login>/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
+    # Nothing granted by that name. If the listing was TRUNCATED the answer is
-    # the message names a concrete repo instead of a guess.
+    # unknown rather than absent -- falling back to <login>/<name> 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 <owner>/{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

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"),

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()

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="[email protected]",
+                                    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": "[email protected]",
+                         "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="[email protected]",
+                                      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("[email protected]")],
+                         ["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="[email protected]",
+                                      verified=True, device_id="dev-c2")
+        self.assertEqual(resp.get("granted_repos"), ["alice/notes"])
+        self.assertEqual(self.svc.state.peek_invites("[email protected]"), [])
+
+    def test_a_partial_failure_only_consumes_what_landed(self):
+        self.svc.invite({"token": self.alice["token"], "email": "[email protected]",
+                         "repos": ["notes", "reports"]})
+        StubUpstream.state["repo_owner"].pop("alice/reports")   # one of two fails
+        _, resp = self.stub_link("s10", "dan", email="[email protected]",
+                                 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("[email protected]")],
+                         ["alice/reports"])
+
 if __name__ == "__main__":
     unittest.main()