fix(xtest): stop otdf-sdk-mgr mistaking branch names for versions

otdf-sdk-mgr/AGENTS.md

@@ -47,21 +47,30 @@ before editing either module:
   worktree of the expected bare repo. The refresh runs `git worktree prune`,
   so a worktree directory that was removed can be added again.
 - **Platform tags are namespaced** as `service/vX.Y.Z`. `_resolve_platform_ref`
-  prefixes the `service/` infix on plain versions; raw SHAs, refs with a
-  `/`, `pr:N` shorthand (expanded to `refs/pull/N/head`), and `main`/`HEAD`
-  pass through unchanged.
+  prefixes the `service/` infix on plain versions (`v0.9.0`, `0.9.0`) and
+  nothing else. Branches, `HEAD`, SHAs, refs with a `/`, and `pr:N`
+  shorthand (expanded to `refs/pull/N/head`) pass through unchanged.
+  Prefixing every name that isn't obviously a ref would look up
+  `install tip --ref my-branch platform` as the tag `service/vmy-branch`.
 - **PR refs aren't under `refs/heads/`,** so `BARE_REFRESH` never pulls
   `refs/pull/N/head`. `_ensure_worktree` (and the SDK equivalents in
   `checkout.py`) explicitly `fetch origin +refs/...:refs/...` for any
   `refs/...` ref before adding or resetting the worktree. Otherwise `git
   worktree add refs/pull/N/head` dies with `invalid reference`, and a reset
   rebuilds the PR head as it was at the last fetch.
-- **Mutable refs auto-refresh; immutable refs cache.** `refs.is_mutable_ref`
-  treats full-length hex SHAs and `<infix>/vX.Y.Z` tag forms as immutable
-  (reuse existing dist), and everything else as mutable (re-fetch the bare
-  repo, `git reset --hard` the worktree, drop the stale binary, rebuild).
-  Don't reintroduce an unconditional dist-exists skip — it silently serves
-  stale binaries when a user installs the same branch a second time.
+- **Mutable refs auto-refresh; immutable refs cache.** Ask origin, not the
+  name: a branch can look like a version (web-sdk cuts `release/1.3.1`).
+  After the bare-repo refresh, `refs.is_mutable_target` takes what
+  `upstream_ref` resolved to a branch on origin (`refs/remotes/origin/*`),
+  or any other `refs/...` ref but a tag, as mutable; only names it passed
+  through fall back on the name heuristic `refs.is_mutable_ref`. Mutable
+  means `git reset --hard` the worktree, and for the platform drop the stale
+  binary and rebuild. `install_platform_source` looks for a cached dist
+  before the refresh, so it reuses one only for a full SHA or a
+  `service/vX.Y.Z` release (`_is_pinned`) and sends anything else through
+  `_ensure_worktree` and a rebuild. Don't reintroduce an unconditional
+  dist-exists skip — it silently serves stale binaries when a user installs
+  the same branch a second time.
 - Subprocess output is **not captured** — long-running `go build` / `git
   clone` streams to the terminal so users can see progress. On failure the
   error message just reports the command and exit code.

otdf-sdk-mgr/src/otdf_sdk_mgr/checkout.py

@@ -9,7 +9,7 @@
 from typing import Any
 
 from otdf_sdk_mgr.config import SDK_BARE_REPOS, SDK_GIT_URLS, get_sdk_dirs
-from otdf_sdk_mgr.refs import expand_pr_shorthand, is_mutable_ref
+from otdf_sdk_mgr.refs import expand_pr_shorthand, is_mutable_target
 
 # `git clone --bare` records no fetch refspec: `git fetch` alone updates no refs
 # and `--tags` adds only tags, so the bare repo's branches stay where the clone
@@ -173,7 +173,7 @@ def checkout_sdk_branch(language: str, branch: str) -> str:
     target = upstream_ref(bare_repo_path, branch)
 
     if worktree_path.exists():
-        if is_mutable_ref(branch):
+        if is_mutable_target(branch, target):
             print(f"Worktree exists at {worktree_path}; resetting to '{branch}'.")
             require_worktree(worktree_path, bare_repo_path)
             _run(["git", "-C", str(worktree_path), "reset", "--hard", target])
@@ -248,7 +248,7 @@ def checkout_go_from_platform(ref: str) -> Path:
     target = upstream_ref(bare_repo_path, ref)
 
     if worktree_path.exists():
-        if is_mutable_ref(ref):
+        if is_mutable_target(ref, target):
             print(f"Worktree for ref '{ref}' exists at {worktree_path}; resetting.")
             require_worktree(worktree_path, bare_repo_path)
             _run(["git", "-C", str(worktree_path), "reset", "--hard", target])

otdf-sdk-mgr/src/otdf_sdk_mgr/platform_installer.py

@@ -18,8 +18,8 @@
 
 from otdf_sdk_mgr.checkout import BARE_REFRESH, CheckoutError, require_worktree, upstream_ref
 from otdf_sdk_mgr.config import SDK_GIT_URLS, SDK_TAG_INFIXES
-from otdf_sdk_mgr.refs import expand_pr_shorthand, is_mutable_ref, ref_slug
-from otdf_sdk_mgr.semver import normalize_version
+from otdf_sdk_mgr.refs import expand_pr_shorthand, is_mutable_target, ref_slug
+from otdf_sdk_mgr.semver import normalize_version, parse_semver
 
 PLATFORM_BARE_REPO = "platform.git"
 HELPER_SCRIPTS_BRANCH = "main"
@@ -115,32 +115,49 @@ def _is_hex(s: str) -> bool:
     return bool(s) and all(c in "0123456789abcdef" for c in s.lower())
 
 
+def _is_version(s: str) -> bool:
+    """A plain version, `v0.9.0` or `0.9.0`. SEMVER_RE would let a `/` into a
+    pre-release suffix, but a name with one is a ref."""
+    return "/" not in s and parse_semver(s) is not None
+
+
 def _resolve_platform_ref(version_or_ref: str) -> str:
-    """Turn a user-supplied version into the actual git ref to checkout.
-
-    `v0.9.0` → `service/v0.9.0` (matches SDK_TAG_INFIXES["platform"]).
-    `pr:42` → `refs/pull/42/head` (expanded first, then passes through the
-    `/` check).
-    A ref that already contains `/`, a full-length hex SHA, or `main` is
-    returned as-is. 7-39 char hex inputs are returned unchanged — they
-    might be abbreviated SHAs or branch names; `install_platform_source`
+    """Turn a user-supplied version or ref into the git ref to check out.
+
+    Only a plain version names a release, so only it gets the tag infix:
+    `v0.9.0` or `0.9.0` → `service/v0.9.0` (SDK_TAG_INFIXES["platform"]).
+    `pr:42` → `refs/pull/42/head`. Anything else is returned as-is for git
+    to resolve: a branch (`main`, `my-branch`, `release/v1.2.3`), `HEAD`, a
+    namespaced tag (`service/v0.9.0`), a `refs/...` ref, or a SHA. 7-39 char
+    hex might be an abbreviated SHA or a branch name; `install_platform_source`
     resolves the ambiguity via `_expand_short_sha` once the bare repo is
     available.
     """
     version_or_ref = expand_pr_shorthand(version_or_ref)
-    infix = SDK_TAG_INFIXES.get("platform", "service")
     if "/" in version_or_ref and (":" in version_or_ref or "@" in version_or_ref):
         raise PlatformInstallError(
             f"container-image refs are not supported: {version_or_ref!r}; "
             "use a git ref like v0.9.0, service/v0.9.0, main, or a SHA"
         )
-    if "/" in version_or_ref or version_or_ref in ("main", "HEAD"):
-        return version_or_ref
-    if len(version_or_ref) in (40, 64) and _is_hex(version_or_ref):
-        return version_or_ref
-    if 7 <= len(version_or_ref) <= 39 and _is_hex(version_or_ref):
-        return version_or_ref
-    return f"{infix}/{normalize_version(version_or_ref)}"
+    if _is_version(version_or_ref):
+        infix = SDK_TAG_INFIXES.get("platform", "service")
+        return f"{infix}/{normalize_version(version_or_ref)}"
+    return version_or_ref
+
+
+def _is_pinned(full_ref: str) -> bool:
+    """True if `full_ref` names one commit for good: a full SHA, or a release
+    tag `service/vX.Y.Z`.
+
+    Only these may reuse a dist without refreshing the bare repo. Anything else
+    might be a branch, whatever its name looks like (`release/v1.2.3`), and
+    only origin can say.
+    """
+    if len(full_ref) in (40, 64) and _is_hex(full_ref):
+        return True
+    infix = SDK_TAG_INFIXES.get("platform", "service")
+    version = full_ref.removeprefix(f"{infix}/")
+    return version != full_ref and _is_version(version)
 
 
 def _expand_short_sha(short: str) -> str:
@@ -177,10 +194,10 @@ def _ensure_worktree(ref: str) -> Path:
 
     Branch names resolve to origin's current tip via `upstream_ref`; a branch
     origin has deleted raises PlatformInstallError. For mutable refs
-    (branches, PR heads) an existing worktree is reset to that tip so a
-    subsequent install picks up new commits, unless it is not a worktree of
-    the bare repo (PlatformInstallError). For immutable refs (tags, SHAs) we
-    just reuse.
+    (`is_mutable_target`: branches, whatever their names look like, and PR
+    heads) an existing worktree is reset to that tip so a subsequent install
+    picks up new commits, unless it is not a worktree of the bare repo
+    (PlatformInstallError). For immutable refs (tags, SHAs) we just reuse.
     """
     bare = _ensure_bare_repo()
     # `_ensure_bare_repo` refreshes branches and tags only (BARE_REFRESH), so
@@ -193,7 +210,7 @@ def _ensure_worktree(ref: str) -> Path:
     target = _upstream_ref(bare, ref)
     worktree = _worktree_path_for(ref)
     if worktree.exists():
-        if is_mutable_ref(ref):
+        if is_mutable_target(ref, target):
             print(f"Worktree exists at {worktree}; resetting to {ref}.")
             _require_worktree(worktree, bare)
             _run(["git", "-C", str(worktree), "reset", "--hard", target])
@@ -253,27 +270,31 @@ def install_platform_source(ref: str, dist_name: str | None = None) -> Path:
     (`service/v0.9.0`), a branch (`main`), a PR shorthand (`pr:42`), a raw
     ref (`refs/pull/42/head`), or a SHA. Returns the dist dir.
 
-    For immutable refs (tags, SHAs) an existing dist is reused. For mutable
-    refs (branches, PR heads) the bare repo is re-fetched, the worktree is
-    reset, the old dist is removed, and the service binary is rebuilt.
+    A release or a full SHA reuses an existing dist without touching the bare
+    repo, so a cached release installs offline. Anything else might be a
+    branch, whatever its name looks like, so it goes through
+    `_ensure_worktree`, which re-fetches the bare repo and resets the worktree
+    if the ref can move; then the old dist is removed and the service binary
+    is rebuilt.
     """
     full_ref = _resolve_platform_ref(ref)
     if 7 <= len(full_ref) <= 39 and _is_hex(full_ref):
         # Abbreviated SHA: resolve to full 40-char SHA so caching matches the
         # full-SHA case. Falls through to branch-name handling if git can't
         # resolve the prefix; raises on ambiguity.
         full_ref = _expand_short_sha(full_ref)
+    pinned = _is_pinned(full_ref)
     if dist_name is None:
-        if is_mutable_ref(full_ref):
-            dist_name = ref_slug(full_ref)
-        else:
+        if pinned:
             # Normalize only the semver tail so namespaced tags like
             # `service/v0.9.0` produce the same dist_name as `v0.9.0`.
             dist_name = normalize_version(full_ref.rsplit("/", 1)[-1])
+        else:
+            dist_name = ref_slug(full_ref)
     dist_dir = _platform_dist_root() / dist_name
     binary = dist_dir / "service"
     marker = dist_dir / ".complete"
-    if binary.exists() and marker.exists() and not is_mutable_ref(full_ref):
+    if binary.exists() and marker.exists() and pinned:
         print(f"  Dist already present at {dist_dir}; skipping build.")
         return dist_dir
     worktree = _ensure_worktree(full_ref)

otdf-sdk-mgr/src/otdf_sdk_mgr/refs.py

@@ -28,7 +28,9 @@ def expand_pr_shorthand(ref: str) -> str:
 def is_mutable_ref(ref: str) -> bool:
     """True if `ref` can change over time (branches, PR heads); False for tags/SHAs.
 
-    Used to decide whether to re-fetch and rebuild on subsequent installs.
+    A guess from the name alone, which takes a branch named like a version
+    (`release/v1.2.3`) for a tag. Once the bare repo has been refreshed, decide
+    with `is_mutable_target` instead.
 
     The caller is expected to have already expanded `pr:N` shorthand and any
     tag-namespace prefixes (e.g. `service/v0.9.0`, `sdk/v0.4.0`).
@@ -42,6 +44,23 @@ def is_mutable_ref(ref: str) -> bool:
     return not SEMVER_RE.match(tail)
 
 
+def is_mutable_target(ref: str, target: str) -> bool:
+    """`is_mutable_ref`, once `checkout.upstream_ref` has resolved `ref` to `target`.
+
+    upstream_ref maps every branch origin has onto `refs/remotes/origin/*`, so
+    such a target is a branch however its name looks: web-sdk cuts
+    `release/1.3.1`. Any other `refs/...` ref but a tag, such as a PR head, can
+    move too. Other names upstream_ref passes through: tags, SHAs, and names
+    git can't resolve, such as a branch origin deleted since the clone. Those
+    are judged by name. Answering False for all of them would quietly reuse a
+    deleted branch's worktree, where a reset fails loudly unless the name looks
+    like a version.
+    """
+    if target.startswith("refs/") and not target.startswith("refs/tags/"):
+        return True
+    return is_mutable_ref(ref)
+
+
 def ref_slug(ref: str) -> str:
     """Slugify a git ref for use as a directory name.
 

otdf-sdk-mgr/tests/test_checkout.py

@@ -186,6 +186,18 @@ def test_immutable_ref_is_checked_out_then_reused(self, entry: str, kind: str, o
 
         _assert_untouched(worktree, pinned)
 
+    @pytest.mark.parametrize("branch", ["release/v1.2.3", "release/1.3.1"])
+    def test_version_named_branch_follows_origin(self, entry: str, branch: str, origin: Path):
+        """A branch whatever its name looks like; web-sdk cuts `release/1.3.1`.
+        Reused like a tag's, its worktree would keep the first commit."""
+        _git("branch", branch, cwd=origin)
+        worktree = _check_out(entry, origin, branch)
+        _git("update-ref", f"refs/heads/{branch}", _advance(origin), cwd=origin)
+
+        _check_out(entry, origin, branch)
+
+        assert _head(worktree) == _head(origin)
+
     def test_pr_ref_follows_a_force_push(self, entry: str, origin: Path):
         """BARE_REFRESH skips `refs/pull/*`, so each checkout fetches the PR
         head itself, and has to overwrite the old one."""
@@ -294,6 +306,51 @@ def test_scripts_directory_that_lost_its_git_file_is_refused(
         _assert_enclosing_untouched(enclosing)
 
 
+@pytest.fixture
+def builds(monkeypatch: pytest.MonkeyPatch) -> list[str]:
+    """Stub out the platform's `go build`; record the commit each build compiles."""
+    built: list[str] = []
+
+    def build(worktree: Path, dist_dir: Path) -> Path:
+        built.append(_head(worktree))
+        dist_dir.mkdir(parents=True, exist_ok=True)
+        (dist_dir / ".complete").write_text("")
+        binary = dist_dir / "service"
+        binary.write_text("")
+        return binary
+
+    monkeypatch.setattr(platform_installer, "_build_service", build)
+    return built
+
+
+class TestPlatformInstall:
+    def test_version_named_branch_is_rebuilt(self, origin: Path, builds: list[str]):
+        """Not served from the first build's dist as if it were a release."""
+        _git("branch", "release/v1.2.3", cwd=origin)
+        platform_installer.install_platform_source("release/v1.2.3")
+        tip = _advance(origin)
+        _git("update-ref", "refs/heads/release/v1.2.3", tip, cwd=origin)
+
+        dist = platform_installer.install_platform_source("release/v1.2.3")
+
+        assert builds[1:] == [tip]
+        assert dist.name == "release--v1.2.3"
+
+    @pytest.mark.parametrize("kind", ["release", "sha"])
+    def test_pinned_build_is_reused_without_origin(
+        self, kind: str, origin: Path, builds: list[str]
+    ):
+        """Neither can move, so a finished dist needs no fetch: `install lts`
+        and `install release` work offline once built."""
+        _git("tag", "service/v1.2.3", cwd=origin)
+        ref = "v1.2.3" if kind == "release" else _head(origin)
+        dist = platform_installer.install_platform_source(ref)
+        shutil.rmtree(origin)
+
+        assert platform_installer.install_platform_source(ref) == dist
+        assert len(builds) == 1
+
+
 class TestInstallTip:
     def test_branch_gone_from_origin_is_an_error_not_a_traceback(self, origin: Path):
         _check_out("sdk", origin, "main")
@@ -304,3 +361,14 @@ def test_branch_gone_from_origin_is_an_error_not_a_traceback(self, origin: Path)
         assert result.exit_code == 1
         assert isinstance(result.exception, SystemExit), result.exception
         assert "Error: branch 'other' is not on origin" in result.output
+
+    def test_platform_branch_without_a_slash(self, origin: Path, builds: list[str]):
+        """The `tip` docstring's own example. Not a version, so not looked up
+        as the tag `service/vmy-branch`."""
+        _git("branch", "my-branch", cwd=origin)
+
+        result = CliRunner().invoke(install_app, ["tip", "--ref", "my-branch", "platform"])
+
+        assert result.exit_code == 0, result.output
+        assert builds == [_head(origin)]
+        assert (origin.parent / "platform" / "dist" / "my-branch" / "service").exists()