refactor(sandbox): move gh token precedence into the credential helper

packages/sandbox-runtime/src/sandbox_runtime/credentials/git_credential_helper.py

@@ -32,10 +32,13 @@
 import sys
 import time
 from pathlib import Path
-from typing import IO, cast
+from typing import IO, TYPE_CHECKING, cast
 
 import httpx
 
+if TYPE_CHECKING:
+    from collections.abc import Mapping
+
 CACHE_DIR = Path(os.environ.get("OI_SCM_CRED_CACHE_DIR", "/run/oi"))
 CACHE_FILE = CACHE_DIR / "scm-creds.json"
 LOCK_FILE = CACHE_DIR / "scm-creds.lock"
@@ -160,7 +163,7 @@ def _read_cached() -> dict[str, object] | None:
     cached = cast("dict[str, object]", raw_cached)
 
     expires_at_ms = cached.get("expires_at_epoch_ms")
-    if not isinstance(expires_at_ms, (int, float)):
+    if not isinstance(expires_at_ms, int | float):
         return None
 
     seconds_remaining = expires_at_ms / 1000 - time.time()
@@ -201,7 +204,7 @@ def _fetch_from_control_plane(endpoint: tuple[str, str, str]) -> dict[str, objec
     if not isinstance(data, dict) or not data.get("username") or not data.get("password"):
         raise RuntimeError("control plane response missing username/password")
     expires_at = data.get("expires_at_epoch_ms")
-    if not isinstance(expires_at, (int, float)) or expires_at <= 0:
+    if not isinstance(expires_at, int | float) or expires_at <= 0:
         # Fail loud rather than cache a credential that _read_cached would
         # immediately reject, which would silently refetch on every git op.
         raise RuntimeError("control plane response has invalid expires_at_epoch_ms")
@@ -264,23 +267,46 @@ def _emit_response(input_lines: dict[str, str], credentials: dict[str, object])
     sys.stdout.flush()
 
 
-def _print_token() -> int:
-    """Print just the password (token) for the gh CLI wrapper.
+def _gh_wrapper_should_mint(env: Mapping[str, str]) -> bool:
+    """Decide whether the gh CLI needs a freshly-minted token.
 
-    Unlike the git `get` action this takes no protocol input and does no
-    path scoping; the token returned is the same installation token
-    regardless of repo. We still enforce the GitHub host because this action
-    exists only for the GitHub CLI wrapper.
-    """
-    if os.environ.get("VCS_HOST", "github.com").strip().lower() != "github.com":
-        _log("token action is only supported for github.com")
-        return 1
+    gh reads ``GH_TOKEN`` then ``GITHUB_TOKEN`` from its own environment, so
+    we mint only when the environment has nothing usable: no user-provided
+    token, and either nothing at all or just the system's short-lived
+    installation fallback (marked ``OI_GITHUB_TOKEN_IS_FALLBACK=1``, which
+    expires in ~1h and must be refreshed). A user-provided token always wins.
 
+    The marker is authoritative on its own: a value comparison between
+    ``GITHUB_TOKEN`` and ``GITHUB_APP_TOKEN`` is not needed to detect a user
+    override, because the manager only sets the marker when it injected both
+    values itself.
+    """
+    if env.get("VCS_HOST", "github.com").strip().lower() != "github.com":
+        return False  # non-github deployment: never touch gh's own auth
+    if env.get("GH_TOKEN"):
+        return False  # user-owned; the manager never injects GH_TOKEN
+    if env.get("OI_GITHUB_TOKEN_IS_FALLBACK") == "1":
+        return True  # only the expiring system fallback is present → refresh
+    # Otherwise mint only when there's no genuine user token to leave alone.
+    return not (env.get("GITHUB_TOKEN") or env.get("GITHUB_APP_TOKEN"))
+
+
+def _print_gh_token() -> int:
+    """Print a freshly-minted token for the gh CLI wrapper, or nothing.
+
+    The wrapper exports whatever we print as ``GH_TOKEN``. When the
+    environment already has a usable token we print nothing so gh uses its
+    own env. A failed mint also prints nothing rather than failing: the
+    wrapper then falls through to the existing env instead of aborting gh.
+    Both cases exit 0 — the wrapper only needs the stdout, not the status.
+    """
+    if not _gh_wrapper_should_mint(os.environ):
+        return 0
     try:
         credentials = _get_credentials()
     except Exception as e:
-        _log(f"failed to obtain token: {e}")
-        return 1
+        _log(f"failed to obtain gh token: {e}")
+        return 0
     sys.stdout.write(str(credentials["password"]))
     sys.stdout.flush()
     return 0
@@ -290,9 +316,10 @@ def main(argv: list[str] | None = None) -> int:
     args = list(argv if argv is not None else sys.argv[1:])
     action = args[0] if args else "get"
 
-    # `token` is for the gh CLI wrapper; emit the bare token.
-    if action == "token":
-        return _print_token()
+    # `gh-token` is for the gh CLI wrapper: print a fresh token when the env
+    # has none usable, otherwise nothing (see _print_gh_token).
+    if action == "gh-token":
+        return _print_gh_token()
 
     # We only mint credentials on `get`. `store` and `erase` are no-ops:
     # the control plane owns the truth and we don't persist anything git tells us.

packages/sandbox-runtime/src/sandbox_runtime/entrypoint.py

@@ -40,46 +40,21 @@
 
 # Wrapper installed at /usr/local/bin/gh (ahead of the real /usr/bin/gh in
 # PATH). The git credential helper can't authenticate the GitHub CLI — gh
-# reads GH_TOKEN/GITHUB_TOKEN from the environment, not git's protocol — so
-# instead of baking a short-lived token into env at boot (which goes stale
-# after ~1h), this mints a fresh token per invocation.
-#
-# Skip rules (exec real gh untouched):
-#   * non-github.com deployments — never touch gh's auth;
-#   * a genuine user-provided token. The manager's legacy-snapshot fallback
-#     token is marked with OI_GITHUB_TOKEN_IS_FALLBACK=1 and is NOT treated
-#     as user-provided, so a helper-capable restored snapshot still refreshes
-#     rather than reusing the soon-expired restore token. gh reads GH_TOKEN
-#     before GITHUB_TOKEN. The manager's fallback aliases are matching
-#     GITHUB_TOKEN/GITHUB_APP_TOKEN values, so a later user override differs
-#     and wins.
+# reads GH_TOKEN/GITHUB_TOKEN from the environment, not git's protocol. This
+# thin delegator asks the credential helper's `gh-token` action whether a
+# fresh token is needed (the precedence logic lives there, in Python). If it
+# prints one we export it as GH_TOKEN; otherwise gh runs with its own env.
 GH_WRAPPER_REAL_PATH = "/usr/bin/gh"
 GH_WRAPPER_BODY = (
     "#!/bin/sh\n"
     f'REAL_GH="{GH_WRAPPER_REAL_PATH}"\n'
-    'if [ "${VCS_HOST:-github.com}" != "github.com" ]; then\n'
-    '  exec "$REAL_GH" "$@"\n'
-    "fi\n"
-    "# A real user token wins; a marked fallback token does not.\n"
-    'if [ -n "$GH_TOKEN" ]; then\n'
-    '  exec "$REAL_GH" "$@"\n'
-    "fi\n"
-    'if [ "$OI_GITHUB_TOKEN_IS_FALLBACK" != "1" ] && '
-    '{ [ -n "$GITHUB_TOKEN" ] || [ -n "$GITHUB_APP_TOKEN" ]; }; then\n'
-    '  exec "$REAL_GH" "$@"\n'
-    "fi\n"
-    'if [ "$OI_GITHUB_TOKEN_IS_FALLBACK" = "1" ] && '
-    '[ -n "$GITHUB_TOKEN" ] && [ -n "$GITHUB_APP_TOKEN" ] && '
-    '[ "$GITHUB_TOKEN" != "$GITHUB_APP_TOKEN" ]; then\n'
-    '  exec "$REAL_GH" "$@"\n'
-    "fi\n"
     # stderr is left attached so the helper's diagnostic surfaces when a
     # refresh fails — otherwise the user just sees an opaque gh 401.
-    "token=$(python3 -m sandbox_runtime.credentials.git_credential_helper token || true)\n"
+    "token=$(python3 -m sandbox_runtime.credentials.git_credential_helper gh-token || true)\n"
     'if [ -n "$token" ]; then\n'
-    '  exec env GH_TOKEN="$token" "$REAL_GH" "$@"\n'
+    # export (not `env GH_TOKEN=… exec`) so the token never lands in argv.
+    '  export GH_TOKEN="$token"\n'
     "fi\n"
-    "# Refresh unavailable — fall back to whatever was in env (may be stale).\n"
     'exec "$REAL_GH" "$@"\n'
 )
 

packages/sandbox-runtime/tests/test_gh_wrapper.py

@@ -1,7 +1,14 @@
 """Shell-level tests for the gh CLI wrapper.
 
-The wrapper (`GH_WRAPPER_BODY`) is a /bin/sh script with hardcoded paths to
-the real gh and the credential helper. We rebuild it here with those two
+The wrapper (`GH_WRAPPER_BODY`) is a thin /bin/sh delegator: it asks the
+credential helper's `gh-token` action for a token and, if one is printed,
+exports it as GH_TOKEN before exec'ing the real gh. The token-precedence
+decision itself lives in Python (see ``test_git_credential_helper.py``);
+these tests only verify the shell wiring — that a printed token reaches gh
+via the environment (never argv), and that an empty or failed helper falls
+through to whatever was already in the env.
+
+We rebuild the wrapper here with the real gh and the helper invocation
 pointed at fakes so the actual control flow runs under a real shell.
 """
 
@@ -16,30 +23,39 @@
 if TYPE_CHECKING:
     from pathlib import Path
 
+# Fake "real gh": echoes the token env vars and argv so a test can see which
+# token (if any) the wrapper handed to gh and confirm it never hit argv.
 REAL_GH_DECISION = (
     '#!/bin/sh\necho "GH_TOKEN=${GH_TOKEN}"\necho "GITHUB_TOKEN=${GITHUB_TOKEN}"\n'
     'echo "GITHUB_APP_TOKEN=${GITHUB_APP_TOKEN}"\necho "ARGS=$*"\n'
 )
 
+# Fake `gh-token` helper behaviours.
+PRINTS_FRESH_TOKEN = "#!/bin/sh\nprintf '%s' 'ghs_fresh'\n"
+PRINTS_NOTHING = "#!/bin/sh\nexit 0\n"
+EXITS_NONZERO = "#!/bin/sh\nexit 1\n"
 
-def _build_wrapper(tmp_path: Path, *, fresh_token: str | None) -> Path:
-    """Materialize the wrapper with fakes for the real gh and token command."""
+# Anchors we substitute in GH_WRAPPER_BODY to point at the fakes. Asserted
+# present before use so that a drift in the wrapper body fails loudly here
+# instead of silently leaving the test running the real gh / helper.
+REAL_GH_ANCHOR = 'REAL_GH="/usr/bin/gh"'
+HELPER_ANCHOR = "python3 -m sandbox_runtime.credentials.git_credential_helper gh-token"
+
+
+def _build_wrapper(tmp_path: Path, *, token_cmd_body: str) -> Path:
+    """Materialize the wrapper with fakes for the real gh and the helper."""
     real_gh = tmp_path / "real-gh"
     real_gh.write_text(REAL_GH_DECISION)
     real_gh.chmod(0o755)
 
     token_cmd = tmp_path / "token-cmd"
-    if fresh_token is None:
-        token_cmd.write_text("#!/bin/sh\nexit 1\n")
-    else:
-        token_cmd.write_text(f"#!/bin/sh\nprintf '%s' '{fresh_token}'\n")
+    token_cmd.write_text(token_cmd_body)
     token_cmd.chmod(0o755)
 
-    body = GH_WRAPPER_BODY.replace('REAL_GH="/usr/bin/gh"', f'REAL_GH="{real_gh}"')
-    body = body.replace(
-        "python3 -m sandbox_runtime.credentials.git_credential_helper token",
-        str(token_cmd),
-    )
+    assert REAL_GH_ANCHOR in GH_WRAPPER_BODY
+    assert HELPER_ANCHOR in GH_WRAPPER_BODY
+    body = GH_WRAPPER_BODY.replace(REAL_GH_ANCHOR, f'REAL_GH="{real_gh}"')
+    body = body.replace(HELPER_ANCHOR, str(token_cmd))
 
     wrapper = tmp_path / "gh"
     wrapper.write_text(body)
@@ -59,113 +75,32 @@ def _run(wrapper: Path, env_extra: dict[str, str]) -> str:
     return result.stdout
 
 
-def test_refreshes_token_when_no_token_set(tmp_path: Path) -> None:
-    wrapper = _build_wrapper(tmp_path, fresh_token="ghs_fresh")
+def test_exports_minted_token_as_gh_token(tmp_path: Path) -> None:
+    wrapper = _build_wrapper(tmp_path, token_cmd_body=PRINTS_FRESH_TOKEN)
     out = _run(wrapper, {"VCS_HOST": "github.com"})
     assert "GH_TOKEN=ghs_fresh" in out
     assert "ARGS=api user" in out
 
 
-def test_respects_explicit_user_token(tmp_path: Path) -> None:
-    """A user-set GITHUB_TOKEN (no fallback marker) is passed through untouched."""
-    wrapper = _build_wrapper(tmp_path, fresh_token="ghs_fresh")
-    out = _run(wrapper, {"VCS_HOST": "github.com", "GITHUB_TOKEN": "user_token"})
-    # Wrapper did not call the token command nor set GH_TOKEN.
-    assert "GH_TOKEN=\n" in out or "GH_TOKEN=" in out.split("\n")[0]
-    assert "GH_TOKEN=ghs_fresh" not in out
-    assert "GITHUB_TOKEN=user_token" in out
-
-
-def test_respects_explicit_user_app_token(tmp_path: Path) -> None:
-    """A user-set GITHUB_APP_TOKEN (no fallback marker) is passed through untouched."""
-    wrapper = _build_wrapper(tmp_path, fresh_token="ghs_fresh")
-    out = _run(wrapper, {"VCS_HOST": "github.com", "GITHUB_APP_TOKEN": "user_app_token"})
-    # Wrapper did not call the token command nor set GH_TOKEN.
-    assert "GH_TOKEN=\n" in out or "GH_TOKEN=" in out.split("\n")[0]
-    assert "GH_TOKEN=ghs_fresh" not in out
-    assert "GITHUB_APP_TOKEN=user_app_token" in out
-
-
-def test_respects_user_app_token_when_fallback_marker_remains(tmp_path: Path) -> None:
-    """A user override after boot should win over a marked fallback token."""
-    wrapper = _build_wrapper(tmp_path, fresh_token="ghs_fresh")
-    out = _run(
-        wrapper,
-        {
-            "VCS_HOST": "github.com",
-            "GITHUB_TOKEN": "stale_restore_token",
-            "GITHUB_APP_TOKEN": "user_app_token",
-            "OI_GITHUB_TOKEN_IS_FALLBACK": "1",
-        },
-    )
-    assert "GH_TOKEN=ghs_fresh" not in out
-    assert "GITHUB_TOKEN=stale_restore_token" in out
-    assert "GITHUB_APP_TOKEN=user_app_token" in out
-
-
-def test_respects_user_gh_token_when_fallback_marker_remains(tmp_path: Path) -> None:
-    """The manager never injects GH_TOKEN as a fallback, so it is always user-owned."""
-    wrapper = _build_wrapper(tmp_path, fresh_token="ghs_fresh")
-    out = _run(
-        wrapper,
-        {
-            "VCS_HOST": "github.com",
-            "GH_TOKEN": "user_gh_token",
-            "GITHUB_TOKEN": "stale_restore_token",
-            "GITHUB_APP_TOKEN": "stale_restore_token",
-            "OI_GITHUB_TOKEN_IS_FALLBACK": "1",
-        },
-    )
-    assert "GH_TOKEN=user_gh_token" in out
-    assert "GH_TOKEN=ghs_fresh" not in out
-
-
-def test_refreshes_past_marked_fallback_token(tmp_path: Path) -> None:
-    """A manager-injected fallback token must be refreshed, not reused."""
-    wrapper = _build_wrapper(tmp_path, fresh_token="ghs_fresh")
-    out = _run(
-        wrapper,
-        {
-            "VCS_HOST": "github.com",
-            "GITHUB_TOKEN": "stale_restore_token",
-            "OI_GITHUB_TOKEN_IS_FALLBACK": "1",
-        },
-    )
-    # gh prefers GH_TOKEN, and we set it to the fresh value.
-    assert "GH_TOKEN=ghs_fresh" in out
-
-
-def test_refreshes_past_marked_fallback_app_token(tmp_path: Path) -> None:
-    """The manager injects matching GITHUB_TOKEN/GITHUB_APP_TOKEN fallbacks."""
-    wrapper = _build_wrapper(tmp_path, fresh_token="ghs_fresh")
-    out = _run(
-        wrapper,
-        {
-            "VCS_HOST": "github.com",
-            "GITHUB_TOKEN": "stale_restore_token",
-            "GITHUB_APP_TOKEN": "stale_restore_token",
-            "OI_GITHUB_TOKEN_IS_FALLBACK": "1",
-        },
-    )
-    assert "GH_TOKEN=ghs_fresh" in out
+def test_minted_token_never_reaches_argv(tmp_path: Path) -> None:
+    """P1-2: the token must reach gh via the environment, never via argv."""
+    wrapper = _build_wrapper(tmp_path, token_cmd_body=PRINTS_FRESH_TOKEN)
+    out = _run(wrapper, {"VCS_HOST": "github.com"})
+    args_line = next(line for line in out.splitlines() if line.startswith("ARGS="))
+    assert "ghs_fresh" not in args_line
 
 
-def test_passthrough_for_non_github_host(tmp_path: Path) -> None:
-    wrapper = _build_wrapper(tmp_path, fresh_token="ghs_should_not_be_used")
-    out = _run(wrapper, {"VCS_HOST": "gitlab.com"})
-    assert "GH_TOKEN=\n" in out or out.startswith("GH_TOKEN=\n")
+def test_no_export_when_helper_prints_nothing(tmp_path: Path) -> None:
+    """Helper declined to mint → gh runs with the pre-existing env untouched."""
+    wrapper = _build_wrapper(tmp_path, token_cmd_body=PRINTS_NOTHING)
+    out = _run(wrapper, {"VCS_HOST": "github.com", "GITHUB_TOKEN": "user_token"})
+    assert "GH_TOKEN=\n" in out
+    assert "GITHUB_TOKEN=user_token" in out
 
 
-def test_falls_back_to_env_when_refresh_fails(tmp_path: Path) -> None:
-    """If the token command fails, exec real gh with the existing env."""
-    wrapper = _build_wrapper(tmp_path, fresh_token=None)
-    out = _run(
-        wrapper,
-        {
-            "VCS_HOST": "github.com",
-            "GITHUB_TOKEN": "stale_restore_token",
-            "OI_GITHUB_TOKEN_IS_FALLBACK": "1",
-        },
-    )
-    # No fresh token available; the stale GITHUB_TOKEN remains for real gh.
-    assert "GITHUB_TOKEN=stale_restore_token" in out
+def test_falls_through_when_helper_fails(tmp_path: Path) -> None:
+    """A nonzero helper (swallowed by `|| true`) leaves the existing env for gh."""
+    wrapper = _build_wrapper(tmp_path, token_cmd_body=EXITS_NONZERO)
+    out = _run(wrapper, {"VCS_HOST": "github.com", "GITHUB_TOKEN": "stale_token"})
+    assert "GH_TOKEN=\n" in out
+    assert "GITHUB_TOKEN=stale_token" in out