fix: three defects found by codex review of today's merged work

No P1s. All three confirmed in the code before fixing.

[P2] Invites were destroyed by a transient forge error. apply_invites() popped
the whole pending list BEFORE attempting the collaborator PUT, so a 502 or a
timeout while someone first signed in meant they got no access and re-linking
never retried -- the promise was gone. Now it peeks, and consumes each grant
only after that grant actually lands. A partial failure keeps exactly the
grants that failed.

[P2] Snapshots lost staged-only work. build_snapshot() read HEAD into a scratch
index and staged the WORKTREE, so a hunk you staged and then edited further
survived only in its later worktree form. git keeps index and worktree as
separate states and the backup now does too: the real index is read without
being touched, and when it differs from both HEAD and the worktree it rides
along as a second parent.

[P3] A truncated repo listing could resolve a bare name to the WRONG repo.
resolve_granted() discarded the truncation flag, so a name whose only match sat
beyond the 2000-repo cap fell back to <login>/<name> and would clone that
instead. Truncation now means "unknown", not "absent": it refuses and asks for
the owner. A complete listing still falls back, because absence is then real.

209 tests (was 204).
This commit is contained in:
claude
2026-08-23 16:14:21 -04:00
parent 9511093b14
commit 062f8d1863
4 changed files with 158 additions and 12 deletions
+44 -11
View File
@@ -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,
env={"GIT_AUTHOR_NAME": "granthi-sync",
"GIT_AUTHOR_EMAIL": "[email protected]",
"GIT_COMMITTER_NAME": "granthi-sync",
"GIT_COMMITTER_EMAIL": "[email protected]"})
_, 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 `<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
# 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 <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
+25 -1
View File
@@ -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"),
+48
View File
@@ -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()
+41
View File
@@ -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()