From ee79fd4b6e74c0b91c314fe712f7b2bf28a3a105 Mon Sep 17 00:00:00 2001 From: Yi Zheng <237498169+beatsmonster@users.noreply.github.com> Date: Mon, 10 Aug 2026 12:51:11 -0400 Subject: [PATCH 1/6] fix(ci): the runtime image built twice and published nothing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both pushes to main since the runtime feature merged have left the registry holding `"tags": null` — no `:latest`, no `:`, nothing — while the build logs show two successful pushes. Every `factory contained --target k8s` run then dies on ImagePullBackOff for an image that was in fact built. The digest is written out as a *filename* so the manifest job can collect one per architecture, and it was written verbatim: `sha256:37f294c7...`. `actions/upload-artifact@v4` rejects any path containing a colon, so the step after a perfectly good push failed, which failed the `build` job, which skipped `manifest` (`needs: build`) — and `manifest` is the only thing that creates a tag. Nothing in the failure names the tag, so the two red runs read as flaky infrastructure rather than as "the image has no tags". Strips the prefix on the way in; `Assemble the manifest list` already puts it back with `sed 's/^sha256://'`, so it accepts either form and needs no change. The digest reaches the shell through `env:` and is checked against the shape of a digest first, which is this workflow's existing rule for every value it interpolates. Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: Yi Zheng <237498169+beatsmonster@users.noreply.github.com> --- .github/workflows/runtime-image.yml | 18 +++++++++++++++++- CLAUDE.md | 5 ++++- 2 files changed, 21 insertions(+), 2 deletions(-) diff --git a/.github/workflows/runtime-image.yml b/.github/workflows/runtime-image.yml index 83ea54682..a812ff459 100644 --- a/.github/workflows/runtime-image.yml +++ b/.github/workflows/runtime-image.yml @@ -93,9 +93,25 @@ jobs: cache-to: type=gha,mode=max,scope=${{ matrix.arch }} - name: Export the digest + # The `sha256:` prefix is stripped because the digest becomes a **filename**, and + # `upload-artifact@v4` rejects a path containing a colon. Left on, the build pushes both + # architectures successfully and then fails on the upload, which skips the `manifest` job + # that creates the tag — so the registry ends up holding the layers with `"tags": null` + # and every `factory contained` command dies on ImagePullBackOff for an image that was + # in fact built. `Assemble the manifest list` puts the prefix back. + # + # Through `env:` rather than interpolated into the script, which is this workflow's rule + # for every value that reaches a shell. + env: + DIGEST: ${{ steps.build.outputs.digest }} run: | + set -eu + case "$DIGEST" in + sha256:[0-9a-f]*) ;; + *) echo "::error::unexpected digest format: $DIGEST"; exit 1 ;; + esac mkdir -p /tmp/digests - touch "/tmp/digests/${{ steps.build.outputs.digest }}" + touch "/tmp/digests/${DIGEST#sha256:}" - uses: actions/upload-artifact@v4 with: diff --git a/CLAUDE.md b/CLAUDE.md index 3ab0120f1..ae4ab4a83 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -320,10 +320,13 @@ Six things are load-bearing and fail quietly if broken: - **Provenance.** A run always starts from the files on this machine, uncommitted changes included — never `HEAD`, never a fresh clone. The workspace is a git worktree with the working tree rsynced over the top, because a HEAD checkout silently drops the gitignored `.factory/` the whole experiment history lives in. Five assertions then run between provisioning and the first agent call (`factory/contained/provenance.py`); a failure aborts naming the file and the likely cause, and leaves the runtime up for inspection. - **Identity.** A bind mount carries ownership through unchanged, so a container whose UID does not own the tree gets a *silently read-only* workspace. The rule differs between rootless, rootful and macOS, so `factory/contained/identity.py` **probes** rather than deciding: a throwaway container reports the mount's owner as the kernel inside sees it, and the run matches. The runtime image is built for arbitrary UIDs (group 0, `chmod g=u`), which is also what OpenShift's restricted SCC needs. - **PID 1.** The factory spawns agent subprocesses and is not a well-behaved init, so the container runs `--init` around `sleep infinity` and the run itself lives in tmux. The runtime persists after the run — a failed run is exactly when its state is worth reading. -- **Credentials cross the boundary, by design.** There is no gateway. The policy is `FACTORY_` by default, plus exactly what `--forward` names, plus the backend variables the resolved shape requires (`factory/contained/credentials.py`) — nothing implicit. `verify` reports credential *shape*, never material, and secret-looking values are redacted anywhere a command is printed. On k8s the credentials come from a namespace Secret the user creates; the factory references it by name and never handles the material. +- **Credentials cross the boundary, by design.** There is no gateway. The policy is `FACTORY_` by default, plus exactly what `--forward` names, plus the backend variables the resolved shape requires (`factory/contained/credentials.py`) — nothing implicit. `verify` reports credential *shape*, never material, and secret-looking values are redacted anywhere a command is printed. On k8s the credentials live in a namespace Secret. `factory/contained/k8s_credentials.py` will *create* one for you as step 3 of the cluster wizard — backend picker, then per value: typed (masked), an environment variable you name, or a file (whose required fields are printed before the question and validated after). Four rules there are load-bearing: the material never enters an **argv** (`--from-literal` is visible in `ps` and in shell history — the manifest goes to `oc apply -f -` on **stdin**), the manifest is **JSON** not YAML (a key containing `:` or a newline is ordinary here), nothing is echoed (shape-only confirmation, redacted command, scrubbed stderr), and nothing is logged but key names and value lengths. With nobody at the keyboard the step is **skipped**, not defaulted — `--yes` means "do not stop to ask me", not "choose a credential for me". - **Both divisions reach outward, and that is the point.** Builds cannot happen inside either boundary, so `--division` is opt-in and separately named. Locally it starts an **unauthenticated** `podman-mcp-server` on `0.0.0.0:8430` — every interface, because the tool has no bind flag and the container reaches the host through a gateway address rather than loopback — detached into its own process group, because the run outlives the launch, and stopped by `factory contained rm`. On the cluster it goes through OpenShift `Build` objects behind a sidecar container that is the only holder of `oc` and the ServiceAccount token; that separation is a boundary only while the Role excludes `pods/exec`, which `verify` asserts via a **SubjectAccessReview API object** — `oc auth can-i --as` collapses `pods/exec` onto `pods` and answers "yes" where RBAC says no. The sidecar runs a **different image** (`FACTORY_CONTAINED_SIDECAR_IMAGE`, an `oc` image) from the agent's; one image for both silently collapses the boundary. - **Interactive prompts stall an unattended run.** A fresh `~/.claude` makes Claude Code ask about folder trust, project MCP servers, and Bypass Permissions mode — all interactive-only, so headless agents never hit them and the interactive CEO does, and the run then sits at a menu nobody is watching. `factory/contained/claude_state.py` pre-records those answers, which the invocation already implies. - **All podman knowledge lives in `factory/podman.py` and all cluster knowledge in `factory/contained/k8s.py`.** Both **compose** commands and do not execute them, which is what makes `FACTORY_CONTAINED_DRY_RUN=1` print the same argv the real path runs rather than a separate rendering that drifts. +- **Never wait on a pod with a flag.** `oc wait --for=... --timeout=Ns` can only report that a condition did not hold, so an unpullable image cost the full timeout and was then reported as a mystery ("the probe produced no output"). `k8s.classify_pod` reads the pod document and `k8s.poll_pod` acts on it: states the kubelet has already given up on (`ImagePullBackOff` — *BackOff* is the word it uses **after** retrying — `CreateContainerConfigError`, `Unschedulable`, …) return at once carrying the kubelet's own message; genuinely retryable ones (`ErrImagePull`) get 30s. `ContainerCreating` is deliberately **not** capped: a first pull legitimately runs for minutes, and capping it trades a hang for a false failure on every cold node. Both the inference probe and `wait_for_container` on the real run path go through it. +- **A step that takes minutes must say what it is waiting for.** `style.activity()` is a transient status line: silent for 5s, then one line rewritten in place (`\r\x1b[2K`) carrying the pod's state and a clock, erased on exit so the caller's result lands in its place. Gated on `style.can_rewrite()`, **not** `enabled()` — colour and motion are different questions, and honouring `FORCE_COLOR` as permission to emit carriage returns fills a CI log with fragments. Off a TTY it degrades to one plain line per changed text; `FACTORY_NO_PROGRESS=1` disables it. +- **Runtime flags parse on either side of a subcommand.** `contained verify --target k8s` and `contained --target k8s verify` are the same command. The flags are declared once in `contained_args._RUNTIME_FLAGS` and fed to both the real parser and a tail parser built with `default=argparse.SUPPRESS`; suppressed defaults are the whole mechanism, since a tail parser that applied its own defaults would report `--target local` for a command line that never said `--target` and silently overwrite the left-hand side. Repeatable flags merge across the subcommand; the same flag on both sides with different values is an **error**, because choosing one silently is how a bundle reaches the wrong namespace. Nothing after `--` is touched. The runtime image (`containers/factory/Containerfile`) is UBI9 + the factory wheel + the agent CLIs + tmux, published multi-arch by CI (`.github/workflows/runtime-image.yml`) — amd64 for cluster nodes, arm64 for a Mac laptop. It publishes on pushes to `main` (`:latest`), on **published releases** (`:`, plus `:latest` unless the release is a prerelease — nightlies are, so they never move `:latest`), and on dispatch. The release trigger is load-bearing: `factory contained setup` pulls and does not build, so a release whose image was never built breaks every new user's first command. Release and dispatch tag names reach the shell through `env:` and are validated against the legal image-tag character set before use. From f3ecb4f136be5d2b96767e8b88317c2e917f7bff Mon Sep 17 00:00:00 2001 From: Yi Zheng <237498169+beatsmonster@users.noreply.github.com> Date: Mon, 10 Aug 2026 12:52:48 -0400 Subject: [PATCH 2/6] feat(contained): say what a slow step is waiting for, and mask a typed secret MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `factory contained --target k8s verify` printed nothing for up to three minutes during the in-cluster inference probe and was reported as a hang. It was not one — but silence and a hang are indistinguishable, and the probe is not the only offender: five access reviews and a `get` per bundle object are a cluster round trip each. `activity()` is a transient status line. Nothing for the first five seconds, so every fast operation still produces byte-for-byte the output it did before and a spinner that flashes for a third of a second is impossible by construction; then one line rewritten in place carrying what is being waited for and a clock; then erased on the way out, in `finally`, so the caller's `[ ok ]` lands where it was and an exception leaves nothing half-drawn. Gated on `can_rewrite()`, deliberately not `enabled()`. Colour and motion are different questions with different answers: `FORCE_COLOR` in CI asks for colour in a log file, and honouring it as permission to emit carriage returns fills that log with thousands of fragments. Off a terminal it degrades to one plain line per *changed* description, so a CI log gains a progress trail instead; `FACTORY_NO_PROGRESS=1` opts out of both. `read_secret()` and `select()` come along because the credentials step needs them. `read_secret` echoes one asterisk per keystroke rather than nothing: the value is routinely a hundred-character paste, and a prompt that shows no response to typing is indistinguishable from one that is not receiving the keys. Length is all the mask discloses. `_edit_line` grew a `mask` parameter rather than being copied. Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: Yi Zheng <237498169+beatsmonster@users.noreply.github.com> --- factory/contained/style.py | 254 +++++++++++++++++++++++++++++++++- tests/test_contained_style.py | 119 ++++++++++++++++ 2 files changed, 371 insertions(+), 2 deletions(-) diff --git a/factory/contained/style.py b/factory/contained/style.py index 9f7c667dc..28b941bdc 100644 --- a/factory/contained/style.py +++ b/factory/contained/style.py @@ -11,6 +11,13 @@ Precedence follows the conventions people already have configured: `NO_COLOR` (any value, https://no-color.org) beats `FORCE_COLOR`, which beats TTY detection. + +Two things here are about *time* rather than appearance, and they answer complaints of the same +shape — "I could not tell whether it was working". `activity()` gives a slow step a line that says +what it is waiting for, rewritten in place; `read_secret()` gives a masked prompt the same +keystroke feedback an ordinary one has. Both degrade where the terminal cannot support them, and +`can_rewrite()` — not `enabled()` — is what decides that, because colour and motion are different +questions. """ from __future__ import annotations @@ -19,6 +26,9 @@ import shutil import sys import textwrap +import threading +import time +from types import TracebackType from typing import Any, TextIO _RESET = "\033[0m" @@ -157,6 +167,157 @@ def line(text: str) -> str: return f" {text}" +# ------------------------------------------------------------------------------------------------ +# Saying what a slow step is waiting for +# ------------------------------------------------------------------------------------------------ + +# Nothing is drawn before this. Every fast operation therefore produces byte-for-byte the output it +# produced before this existed, and a spinner that flashes for a third of a second — which reads as +# a glitch rather than as progress — is impossible by construction. +ACTIVITY_THRESHOLD_SECONDS = 5.0 + +# Fast enough that the elapsed counter looks live, slow enough to be free. +_ACTIVITY_FRAME_SECONDS = 0.1 + +_SPINNER_FRAMES = "⠋⠙⠹⠸⠼⠴⠦⠧⠇⠏" + +# Return to column zero and clear to end of line. `\r` alone leaves the tail of a longer previous +# frame on screen, which is how a status line comes to read `waiting for the podd pod`. +_ERASE_LINE = "\r\033[2K" + + +def can_rewrite(stream: TextIO | None = None) -> bool: + """Whether the last line on `stream` can be erased and redrawn in place. + + Deliberately *not* `enabled()`. Colour and motion are different questions with different + answers: `FORCE_COLOR` in a CI job asks for colour in a log file, and honouring it as permission + to emit carriage returns fills that log with thousands of half-lines. `NO_COLOR` likewise says + nothing about redrawing. Only a real terminal gets rewritten; `FACTORY_NO_PROGRESS=1` opts out + of even that. + """ + target = stream if stream is not None else sys.stdout + if os.environ.get("FACTORY_NO_PROGRESS"): + return False + if os.environ.get("TERM", "").strip().lower() == "dumb": + return False + try: + return bool(target.isatty()) + except (AttributeError, ValueError): + return False + + +def _elapsed(seconds: float) -> str: + return f"{int(seconds) // 60}:{int(seconds) % 60:02d}" + + +class Activity: + """A status line for one slow operation: what is running, and what it is waiting for. + + Use it through `activity()`. While one is live, nothing else may write to the same stream — + another writer's output lands in the middle of a frame and is then erased by the next one. + + Off a terminal it degrades to plain lines rather than to silence, because the case that + prompted this — a check that takes three minutes — is just as unreadable in a CI log as it is on + a laptop. There it prints one line per *changed* description, so a log gains a progress trail + instead of a thousand redraws. + """ + + def __init__(self, label: str, detail: str = "", *, stream: TextIO | None = None, + threshold: float = ACTIVITY_THRESHOLD_SECONDS) -> None: + self._label = label + self._detail = detail + self._stream = stream if stream is not None else sys.stdout + self._threshold = threshold + self._rewrites = can_rewrite(self._stream) + self._lock = threading.Lock() + self._done = threading.Event() + self._thread: threading.Thread | None = None + self._started = 0.0 + self._frame = 0 + self._drawn = False + self._announced: str | None = None + + def update(self, detail: str) -> None: + """Say what is being waited for now. Safe to call as often as a poll loop likes.""" + with self._lock: + if detail == self._detail: + return + self._detail = detail + if self._rewrites: + return + # The plain path has no thread to notice the threshold passing, so the decision is made + # here: quiet until the operation has proved slow, then one line per change. + if time.monotonic() - self._started >= self._threshold and detail != self._announced: + self._announced = detail + self._write(f" ... {self._compose_text()}\n") + + def _compose_text(self) -> str: + return f"{self._label} — {self._detail}" if self._detail else self._label + + def _write(self, text: str) -> None: + try: + self._stream.write(text) + self._stream.flush() + except (OSError, ValueError): + # A closed stream must not take down the operation being reported on. + self._rewrites = False + + def __enter__(self) -> Activity: + self._started = time.monotonic() + if self._rewrites: + self._thread = threading.Thread(target=self._spin, daemon=True) + self._thread.start() + return self + + def __exit__(self, exc_type: type[BaseException] | None, exc: BaseException | None, + tb: TracebackType | None) -> None: + self._done.set() + if self._thread is not None: + self._thread.join(timeout=1.0) + with self._lock: + if self._drawn: + # The caller's own result line goes where the spinner was. Erasing here rather than + # leaving the frame up is what keeps a completed check looking like it always did. + self._write(_ERASE_LINE) + self._drawn = False + + def _spin(self) -> None: + while not self._done.wait(_ACTIVITY_FRAME_SECONDS): + if time.monotonic() - self._started < self._threshold: + continue + with self._lock: + if self._done.is_set(): + return + self._draw() + + def _draw(self) -> None: + spinner = _SPINNER_FRAMES[self._frame % len(_SPINNER_FRAMES)] + self._frame += 1 + clock = _elapsed(time.monotonic() - self._started) + text = f"{spinner} {self._compose_text()} ({clock})" + # Truncated to the terminal, because a frame that wraps occupies two lines and `\r` only + # ever returns to the start of the last one — leaving the first half on screen forever. + width = max(shutil.get_terminal_size(fallback=(80, 24)).columns - 1, 20) + if len(text) > width: + text = text[: width - 1] + "…" + self._write(_ERASE_LINE + paint(text, "cyan", stream=self._stream)) + self._drawn = True + + +def activity(label: str, detail: str = "", *, stream: TextIO | None = None, + threshold: float = ACTIVITY_THRESHOLD_SECONDS) -> Activity: + """A status line for a slow operation, silent unless it turns out to be slow. + + with style.activity("inference_from_cluster", "creating the probe pod") as act: + ... + act.update("pod is Pending — ContainerCreating") + + It draws nothing for the first `threshold` seconds, redraws one line in place after that, and + erases itself on the way out so the caller's result line lands in its place. + """ + return Activity(label, detail, stream=stream, threshold=threshold) + + ESCAPE = "\x1b" """What `read_key` returns for a bare Escape, and what a text prompt looks for in a typed line.""" @@ -296,13 +457,63 @@ def read_line( target.flush() -def _edit_line(descriptor: int, target: TextIO) -> str | None: +def read_secret( + question: str, *, mask: str = "*", stream: TextIO | None = None +) -> str | None: + """Read a value that must not appear on screen. `None` means cancelled. + + Echoes one `mask` character per keystroke rather than nothing at all: a prompt that shows no + response to typing is indistinguishable from one that is not receiving the keys, and the value + being entered here is routinely a hundred-character paste. The length is the only thing the + mask discloses. + + Falls back to `getpass`, which suppresses echo entirely, where raw reading is impossible. The + returned value is stripped — a trailing newline or space from a copy-paste is never part of a + key, and one that survives fails authentication in a way nothing reports usefully. + + The caller owns the value from here. It must not be logged, echoed, or put in an argv. + """ + target = stream if stream is not None else sys.stdout + rendered = f"{bold(question, stream=target)} " + session = _raw_session(target) + if session is None: + import getpass + + try: + typed = getpass.getpass(rendered) + except (EOFError, OSError): + print() + return None + return None if is_escape(typed) else typed.strip() + + descriptor, original = session + + import termios + import tty + + target.write(rendered) + target.flush() + try: + tty.setcbreak(descriptor) + return _edit_line(descriptor, target, mask=mask) + except (OSError, ValueError): + return None + finally: + termios.tcsetattr(descriptor, termios.TCSADRAIN, original) + target.write("\n") + target.flush() + + +def _edit_line(descriptor: int, target: TextIO, *, mask: str | None = None) -> str | None: """The line editor itself, on a terminal already in cbreak mode. `None` means cancelled. Small on purpose, and it echoes as it goes: cbreak turns off the line discipline that normally provides echo and Backspace, so anything it does not handle here is a key that appears to do nothing. The caller owns putting the terminal into cbreak and restoring it — this function only reads, and must not be called on a terminal that is still line-buffered. + + `mask` replaces each character on screen, for a value that must not be readable over a + shoulder. The buffer is unaffected; only the echo changes. """ typed_chars: list[str] = [] while True: @@ -326,7 +537,7 @@ def _edit_line(descriptor: int, target: TextIO) -> str | None: continue if char.isprintable(): typed_chars.append(char) - target.write(char) + target.write(mask if mask is not None else char) target.flush() @@ -377,6 +588,45 @@ def confirm(question: str, *, default: bool = False, stream: TextIO | None = Non return False +def select( + question: str, options: list[tuple[str, str]], *, stream: TextIO | None = None +) -> str | None: + """A one-keypress choice between named options. `None` means the user backed out. + + Each option is a `(key, label)` pair, printed one per line as `[k] label` — the same shape as + `confirm`'s legend, for the same reason: a menu whose keys are only listed as `[a/b/c]` makes + the reader hold the mapping in their head while deciding. + + Rejects an unknown key by asking again rather than by choosing something, because every caller + of this is about to do something to a cluster. + """ + target = stream if stream is not None else sys.stdout + keys = {key.lower() for key, _ in options} + print(file=target) + for key, label in options: + print(line(choice(key, f" {label}", stream=target)), file=target) + print(file=target) + legend = f"{bold(question, stream=target)} [{'/'.join(key for key, _ in options)}]: " + while True: + pressed = read_key(legend, stream=target) + if pressed is None: + try: + raw = input(legend) + except (EOFError, OSError): + print() + return None + if is_escape(raw): + return None + answer = raw.strip().lower() + if answer in keys: + return answer + continue + if pressed == ESCAPE: + return None + if pressed.lower() in keys: + return pressed.lower() + + def prompt(question: str, default: str | None = None, stream: TextIO | None = None) -> str: """A question, with its default rendered so it is obvious what Enter does.""" if default is None: diff --git a/tests/test_contained_style.py b/tests/test_contained_style.py index cb05b2ffc..f237102d8 100644 --- a/tests/test_contained_style.py +++ b/tests/test_contained_style.py @@ -7,8 +7,11 @@ from __future__ import annotations import io +import time from unittest.mock import patch +import pytest + from factory.contained import style ESC = "\033" @@ -159,3 +162,119 @@ def test_confirm_reads_a_single_keypress() -> None: with patch("factory.contained.style.read_key", return_value="y"), \ patch("builtins.input", side_effect=AssertionError("must not need Enter")): assert style.confirm("Create it?") is True + + +def test_select_returns_the_key_that_was_pressed() -> None: + with patch("factory.contained.style.read_key", return_value="2"): + assert style.select("Which?", [("1", "local"), ("2", "k8s")]) == "2" + + +def test_select_backs_out_on_escape() -> None: + with patch("factory.contained.style.read_key", return_value=style.ESCAPE): + assert style.select("Which?", [("1", "local"), ("2", "k8s")]) is None + + +def test_select_spells_every_option_out() -> None: + """A menu rendered as `[1/2]` makes the reader hold the mapping in their head.""" + tty = _Tty() + with patch("factory.contained.style.read_key", return_value="1"): + style.select("Which?", [("1", "a podman container"), ("2", "a cluster pod")], stream=tty) + printed = tty.getvalue() + assert "a podman container" in printed and "a cluster pod" in printed + + +# --------------------------------------------------------------------------------------------- +# Saying what a slow step is waiting for +# --------------------------------------------------------------------------------------------- + + +def test_a_fast_operation_draws_nothing_at_all() -> None: + """Below the threshold the output is byte-for-byte what it was before this existed.""" + tty = _Tty() + with style.activity("check", "working", stream=tty) as act: + act.update("still working") + assert tty.getvalue() == "" + + +def test_a_slow_operation_redraws_in_place_and_erases_itself() -> None: + tty = _Tty() + with style.activity("probe", "creating the pod", stream=tty, threshold=0.01) as act: + time.sleep(0.15) + act.update("waiting for the pod") + time.sleep(0.15) + printed = tty.getvalue() + assert printed.count("\r") > 1 # redrawn, not appended + assert "probe" in printed and "waiting for the pod" in printed + # Erased on the way out, so the caller's result line lands where the spinner was. + assert printed.endswith("\r\033[2K") + + +def test_a_pipe_gets_plain_lines_rather_than_carriage_returns() -> None: + """A CI log needs the progress and must not receive a thousand half-drawn frames.""" + plain = io.StringIO() + with style.activity("probe", "creating the pod", stream=plain, threshold=0.01) as act: + time.sleep(0.05) + act.update("waiting for the pod") + act.update("waiting for the pod") # unchanged: says nothing twice + act.update("reading its output") + printed = plain.getvalue() + assert "\r" not in printed + assert printed.count("waiting for the pod") == 1 + assert "reading its output" in printed + + +def test_progress_can_be_turned_off_entirely() -> None: + tty = _Tty() + with patch.dict("os.environ", {"FACTORY_NO_PROGRESS": "1"}, clear=True): + with style.activity("probe", "creating", stream=tty, threshold=0.01) as act: + time.sleep(0.05) + act.update("waiting") + assert "\r" not in tty.getvalue() + + +def test_a_dumb_terminal_is_never_rewritten() -> None: + tty = _Tty() + with patch.dict("os.environ", {"TERM": "dumb"}, clear=True): + assert style.can_rewrite(tty) is False + + +def test_force_color_does_not_authorize_redrawing_a_log_file() -> None: + """Colour and motion are different questions; conflating them fills CI logs with fragments.""" + with patch.dict("os.environ", {"FORCE_COLOR": "1"}, clear=True): + assert style.can_rewrite(io.StringIO()) is False + + +def test_the_status_line_is_erased_even_when_the_operation_raises() -> None: + tty = _Tty() + with pytest.raises(RuntimeError): + with style.activity("probe", "creating", stream=tty, threshold=0.01): + time.sleep(0.25) # long enough for at least one frame to be drawn + raise RuntimeError("boom") + printed = tty.getvalue() + assert "probe" in printed # it did draw + assert printed.endswith("\r\033[2K") # and cleaned up on the way out + + +# --------------------------------------------------------------------------------------------- +# Reading a value that must not appear on screen +# --------------------------------------------------------------------------------------------- + + +def test_read_secret_falls_back_to_getpass_where_raw_reading_is_impossible() -> None: + with patch("factory.contained.style._raw_session", return_value=None), \ + patch("getpass.getpass", return_value=" sk-ant-secret "): + assert style.read_secret("API key") == "sk-ant-secret" + + +def test_read_secret_cancels_rather_than_returning_an_empty_string() -> None: + """"Nothing was entered" and "the user backed out" call for different behaviour.""" + for failure in (EOFError, OSError): + with patch("factory.contained.style._raw_session", return_value=None), \ + patch("getpass.getpass", side_effect=failure): + assert style.read_secret("API key") is None + + +def test_read_secret_recognizes_escape_from_the_fallback_path() -> None: + with patch("factory.contained.style._raw_session", return_value=None), \ + patch("getpass.getpass", return_value="\x1b"): + assert style.read_secret("API key") is None From 04342388dce2626afb6b9913755c468e4d041f39 Mon Sep 17 00:00:00 2001 From: Yi Zheng <237498169+beatsmonster@users.noreply.github.com> Date: Mon, 10 Aug 2026 12:52:48 -0400 Subject: [PATCH 3/6] fix(contained): runtime flags work on either side of the subcommand MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `factory contained verify --target k8s` was rejected with advice to put the flag first. Both orders are what people type; accepting one of them teaches an ordering rule that serves nobody, and the error arrives after the user has already decided what they want. argparse cannot express this: the REMAINDER carrying the verbatim payload swallows the tail whole, so a flag typed after the subcommand never reaches `args` unless it is parsed back out. It now is, by a second parser built from the same table as the real one — declared once, so a flag accepted before the subcommand and forgotten after it is not possible. Suppressed defaults are the mechanism. A tail parser applying its own defaults would report `--target local` for a command line that never mentioned `--target`, silently overwriting the `--target k8s` typed before it; with `argparse.SUPPRESS` an absent flag is left out of the namespace entirely and only what was actually typed merges. Repeatable flags merge across the subcommand, which is what "repeatable" already means everywhere else. The same flag on both sides with different values is an error naming it: `--namespace a verify --namespace b` has no obviously right reading, and choosing one silently is how a bundle reaches somebody else's namespace. Two runtime names are refused rather than the second being dropped. Nothing after `--` is touched. Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: Yi Zheng <237498169+beatsmonster@users.noreply.github.com> --- factory/cli/contained.py | 20 +---- factory/cli/contained_args.py | 159 +++++++++++++++++++++++++++++----- tests/test_contained.py | 53 +++++++++++- 3 files changed, 194 insertions(+), 38 deletions(-) diff --git a/factory/cli/contained.py b/factory/cli/contained.py index 21fb808e5..cc27b2cc9 100644 --- a/factory/cli/contained.py +++ b/factory/cli/contained.py @@ -17,6 +17,7 @@ from factory.cli.contained_args import ( HELP_EPILOG, HELP_SUBCOMMAND, + add_runtime_flags, interpret, target_given, ) @@ -58,23 +59,10 @@ def build_contained_parser(sub: argparse._SubParsersAction) -> argparse.Argument # One REMAINDER for everything positional, split afterwards by `interpret`. A declarative split # is not expressible: an optional positional carrying `choices` would try to match the first # word of the payload and reject it as an invalid choice. - # Every flag is SUPPRESSed from argparse's own listing and described in the epilog instead: - # a flat list hides which target each flag belongs to, and printing both lists each flag twice. p.add_argument("rest", nargs=argparse.REMAINDER, help=argparse.SUPPRESS) - p.add_argument("--target", choices=["local", "k8s"], default="local", help=argparse.SUPPRESS) - p.add_argument("--division", action="store_true", default=False, help=argparse.SUPPRESS) - p.add_argument("--name", default=None, help=argparse.SUPPRESS) - p.add_argument("--env", action="append", default=[], metavar="KEY=VALUE", dest="extra_env", - help=argparse.SUPPRESS) - p.add_argument("--forward", action="append", default=[], metavar="VAR", help=argparse.SUPPRESS) - p.add_argument("--mount", action="append", default=[], metavar="PATH", help=argparse.SUPPRESS) - p.add_argument("--namespace", default=None, help=argparse.SUPPRESS) - p.add_argument("--storage-class", default=None, dest="storage_class", help=argparse.SUPPRESS) - p.add_argument("--context", default=None, help=argparse.SUPPRESS) - p.add_argument("--image", default=None, help=argparse.SUPPRESS) - # `rm` prompts before deleting an active runtime and the cluster upload prompts on a secret-scan - # finding; `--yes` skips both, for automation. - p.add_argument("--yes", action="store_true", default=False, help=argparse.SUPPRESS) + # From the one table `interpret` also parses the tail against, so a flag can never be accepted + # before the subcommand and rejected after it. + add_runtime_flags(p) _PARSER = p return p diff --git a/factory/cli/contained_args.py b/factory/cli/contained_args.py index 722fbfa07..2c793c0dc 100644 --- a/factory/cli/contained_args.py +++ b/factory/cli/contained_args.py @@ -17,6 +17,7 @@ import os import sys from pathlib import Path +from typing import Any, NoReturn import structlog @@ -26,6 +27,73 @@ LIFECYCLE_SUBCOMMANDS = ("ls", "attach", "rm", "sync", "setup", "verify", "bundle") +# Every runtime flag, declared once. Both the real parser and the tail parser below are built from +# this, because the two must accept exactly the same set: a flag added to one and forgotten in the +# other is a flag that works on one side of the subcommand and errors on the other, which is the +# defect this table exists to make impossible rather than merely unlikely. +_RUNTIME_FLAGS: tuple[tuple[str, dict[str, Any]], ...] = ( + ("--target", {"choices": ["local", "k8s"], "default": "local"}), + ("--division", {"action": "store_true", "default": False}), + ("--name", {"default": None}), + ("--env", {"action": "append", "default": [], "metavar": "KEY=VALUE", "dest": "extra_env"}), + ("--forward", {"action": "append", "default": [], "metavar": "VAR"}), + ("--mount", {"action": "append", "default": [], "metavar": "PATH"}), + ("--namespace", {"default": None}), + ("--storage-class", {"default": None, "dest": "storage_class"}), + ("--context", {"default": None}), + ("--image", {"default": None}), + # `rm` prompts before deleting an active runtime and the cluster upload prompts on a secret-scan + # finding; `--yes` skips both, for automation. + ("--yes", {"action": "store_true", "default": False}), +) + + +def _dest_of(flag: str, options: dict[str, Any]) -> str: + return str(options.get("dest") or flag.lstrip("-").replace("-", "_")) + + +_FLAG_DEFAULTS = {_dest_of(flag, opts): opts["default"] for flag, opts in _RUNTIME_FLAGS} + +# Repeatable flags. Given on both sides of the subcommand they merge rather than conflict, which is +# what "repeatable" already means everywhere else on the command line. +_REPEATABLE_DESTS = frozenset( + _dest_of(flag, opts) for flag, opts in _RUNTIME_FLAGS if opts.get("action") == "append" +) + + +def add_runtime_flags(parser: argparse.ArgumentParser, *, keep_defaults: bool = True) -> None: + """Add every runtime flag to `parser`. + + With `keep_defaults=False` an absent flag is left out of the namespace entirely + (`argparse.SUPPRESS`) rather than filled in with its default. That distinction is the whole + mechanism behind accepting flags on either side of the subcommand: a tail parser whose defaults + were applied would report `--target local` for a command line that never mentioned `--target`, + and silently overwrite the `--target k8s` typed before the subcommand. + + Every flag is hidden from argparse's own listing and described in `HELP_EPILOG` instead: a flat + list hides which target each flag belongs to, and printing both lists each flag twice. + """ + for flag, options in _RUNTIME_FLAGS: + settings = dict(options, help=argparse.SUPPRESS) + if not keep_defaults: + settings["default"] = argparse.SUPPRESS + parser.add_argument(flag, **settings) + + +class _TailError(Exception): + """A bad flag *value* after the subcommand — reported through the real parser, not by exiting.""" + + +class _TailParser(argparse.ArgumentParser): + """A parser for the tail that raises instead of exiting. + + Left to itself argparse would print its own usage — which describes this internal parser rather + than `factory contained` — and call `sys.exit`. Raising lets the real parser own the message. + """ + + def error(self, message: str) -> NoReturn: + raise _TailError(message) + # `help` is not a lifecycle subcommand — it provisions nothing and acts on no runtime — but it is # what people type, and without it the word falls through to the passthrough path and fails with # "no existing directory found in ['help']", a message about materializing workspaces for what is a @@ -62,6 +130,10 @@ bundle Print the cluster prerequisites as YAML (k8s) help Print this text (same as --help) +Runtime flags go on either side of a subcommand — `contained --target k8s verify` +and `contained verify --target k8s` are the same command. After `--` nothing is +interpreted: it belongs to the factory inside the runtime. + Both targets: --target local|k8s Which runtime (default: local) --division Let the agent build container images @@ -145,30 +217,77 @@ def _split_positional(parser: argparse.ArgumentParser, args: argparse.Namespace) def _read_lifecycle_tail( parser: argparse.ArgumentParser, args: argparse.Namespace, tail: list[str] ) -> None: - """What may follow a lifecycle subcommand: a runtime name, and `--yes`. Nothing else.""" - # `--yes` is the one trailing flag accepted here, because `rm --yes` is the order - # people type it. It is documented as the exception; every other flag in this position is - # rejected below rather than silently dropped. - if "--yes" in tail: - args.yes = True - tail = [token for token in tail if token != "--yes"] - # Everything else that looks like a flag here is a mistake worth naming, not swallowing. - # The REMAINDER split means `--target k8s` typed *after* the subcommand never reaches - # `args.target` — it lands here as a plain string instead, so a silent absorption would - # leave `args.target` at its default ("local") while the user believes they asked for k8s, - # and would hand a lifecycle command a name like "--target" to resolve. - flag_like = [token for token in tail if token.startswith("-")] - if flag_like: + """What may follow a lifecycle subcommand: a runtime name and any runtime flag. + + Both orders work — `contained --target k8s verify` and `contained verify --target k8s` — because + both are what people type, and a tool that accepts only one of them is teaching an ordering rule + that serves nobody. argparse cannot do this itself: the REMAINDER that carries the verbatim + payload swallows the tail whole, so a flag typed here never reaches `args` unless it is parsed + back out, which is what happens below. + """ + tail_parser = _TailParser(add_help=False, allow_abbrev=False) + add_runtime_flags(tail_parser, keep_defaults=False) + try: + typed, leftover = tail_parser.parse_known_args(tail) + except _TailError as exc: + # A bad *value* — `--target nope`. Reported through the real parser so the usage line the + # user sees is `factory contained`'s, not this internal parser's. + parser.error(f"after `factory contained {args.subcommand}`: {exc}") + + _merge_tail_flags(parser, args, typed) + + # What is left is either a runtime name or a mistake. A flag reaching here is genuinely + # unrecognized now that every real one has been parsed, so it is named rather than swallowed: + # absorbing it silently would hand a lifecycle command a name like "--targt" to resolve. + unknown = [token for token in leftover if token.startswith("-")] + if unknown: + parser.error( + f"unrecognized flag {unknown[0]!r} after `factory contained {args.subcommand}`. " + f"`factory contained help` lists every flag; they may go on either side of the " + f"subcommand." + ) + names = [token for token in leftover if not token.startswith("-")] + if len(names) > 1: parser.error( - f"unrecognized flag {flag_like[0]!r} after `factory contained " - f"{args.subcommand}`. Runtime flags (--target, --namespace, --name, ...) go before " - f"the subcommand, for example:\n" - f" factory contained --target k8s {args.subcommand}" + f"`factory contained {args.subcommand}` takes one runtime name, but was given " + f"{len(names)}: {', '.join(repr(n) for n in names)}. Try `factory contained ls`." ) # Only the positional overrides `--name` here, and only when one was actually given — # `ls` takes no name. - if tail: - args.name = tail[0] + if names: + args.name = names[0] + + +def _merge_tail_flags( + parser: argparse.ArgumentParser, args: argparse.Namespace, typed: argparse.Namespace +) -> None: + """Fold flags parsed from the tail into `args`, refusing to guess when the two sides disagree. + + Only flags actually typed after the subcommand appear in `typed` — that is what suppressed + defaults buy — so nothing here can overwrite a left-side flag with a default. + + A flag given on *both* sides with different values is an error rather than a silent win for one + of them. `--namespace a verify --namespace b` has no reading that is obviously right, and the + cost of choosing wrong is applying RBAC to somebody else's namespace. + + One gap, named because it is invisible otherwise: a left-side flag set to exactly its own + default cannot be told apart from one that was never typed, so `--target local verify --target + k8s` is accepted as `k8s` rather than reported as a conflict. Both sides agreeing on a value is + also not a conflict, which is the common case when a script and a user both pass `--yes`. + """ + for dest, value in vars(typed).items(): + current = getattr(args, dest, None) + if dest in _REPEATABLE_DESTS: + setattr(args, dest, list(current or []) + list(value)) + continue + default = _FLAG_DEFAULTS[dest] + if current != default and current != value: + flag = f"--{dest.replace('_', '-')}" + parser.error( + f"{flag} was given twice with different values ({current!r} before " + f"`{args.subcommand}`, {value!r} after). Pass it once." + ) + setattr(args, dest, value) def _reject_out_of_scope_flags( diff --git a/tests/test_contained.py b/tests/test_contained.py index 542f7bf9b..89ee59d8d 100644 --- a/tests/test_contained.py +++ b/tests/test_contained.py @@ -85,9 +85,58 @@ def test_trailing_yes_reaches_the_namespace() -> None: assert args.yes is True -def test_flag_after_lifecycle_subcommand_is_an_error_not_a_name() -> None: +def test_runtime_flags_work_on_either_side_of_the_subcommand() -> None: + """`contained verify --target k8s` is what people type; it used to be rejected. + + The two orders have to produce the same namespace, not merely both be accepted — a `--target` + that parses but does not reach `args.target` sends a cluster command to the local runtime. + """ + before = interpret(["--target", "k8s", "--namespace", "ns", "verify"]) + after = interpret(["verify", "--target", "k8s", "--namespace", "ns"]) + assert (before.subcommand, before.target, before.namespace) == ("verify", "k8s", "ns") + assert (after.subcommand, after.target, after.namespace) == ("verify", "k8s", "ns") + + +def test_a_name_and_flags_can_be_mixed_after_the_subcommand() -> None: + args = interpret(["rm", "rta-abc123", "--target", "k8s", "--namespace", "ns"]) + assert (args.subcommand, args.name, args.target, args.namespace) == ( + "rm", "rta-abc123", "k8s", "ns" + ) + + +def test_a_repeatable_flag_merges_across_the_subcommand() -> None: + """Repeatable means repeatable; splitting one across the subcommand is not a conflict.""" + args = interpret(["--env", "A=1", "ls", "--env", "B=2"]) + assert args.extra_env == ["A=1", "B=2"] + + +def test_the_same_flag_on_both_sides_with_different_values_is_refused() -> None: + """Silently letting one side win is how a bundle reaches the wrong namespace.""" + with pytest.raises(SystemExit): + interpret(["--namespace", "a", "verify", "--namespace", "b"]) + + +def test_an_unknown_flag_after_the_subcommand_is_still_named() -> None: + with pytest.raises(SystemExit): + interpret(["ls", "--targt", "k8s"]) + + +def test_a_bad_flag_value_after_the_subcommand_is_reported_not_exited_silently() -> None: with pytest.raises(SystemExit): - interpret(["ls", "--target", "k8s"]) + interpret(["verify", "--target", "nope"]) + + +def test_two_names_after_a_lifecycle_subcommand_are_refused() -> None: + """The second used to be dropped without a word, leaving the wrong runtime targeted.""" + with pytest.raises(SystemExit): + interpret(["attach", "one", "two"]) + + +def test_the_payload_after_the_separator_is_never_parsed_as_a_runtime_flag() -> None: + """The whole passthrough contract: `--target` in the payload belongs to the inner factory.""" + args = interpret(["--", "ceo", "/tmp", "--target", "k8s"]) + assert args.target == "local" + assert args.factory_args == ["ceo", "/tmp", "--target", "k8s"] def test_local_only_flag_against_k8s_fails_at_parse_time() -> None: From 4b308587c8e62218174bf8c3ba5eb88133ed9308 Mon Sep 17 00:00:00 2001 From: Yi Zheng <237498169+beatsmonster@users.noreply.github.com> Date: Mon, 10 Aug 2026 12:53:11 -0400 Subject: [PATCH 4/6] fix(contained): three cluster states the runtime described wrongly MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit These are one commit because they are one file's worth of interleaved hunks — `k8s_setup.py` carries all three — and splitting them would have produced commit messages that did not match their contents. **A pod that cannot start is waited on for the full timeout.** An unpullable image cost the inference probe its 180 seconds and was then reported as "the probe produced no output", naming neither the cause nor where to look, for a failure the kubelet had already explained on the first poll. `oc wait --for= ... --timeout=Ns` cannot do better: it reports only that a condition did not hold, so every way of not holding looks alike. `classify_pod` reads the pod document and `poll_pod` acts on it — states the kubelet has already given up on return at once carrying its own message (`ImagePullBackOff`; *BackOff* is the word it uses **after** retrying), states that might still clear (`ErrImagePull`) get 30 seconds. `ContainerCreating` is deliberately not capped: a first pull legitimately runs for minutes on a cold node, so a cap would trade a hang for a false failure on every new node. Anything unrecognized is `waiting`, never `doomed` — being wrong in that direction aborts a run over a state that would have cleared. `wait_for_container` on the real run path had the same defect with a 300-second ceiling. **An expired login was reported as a namespace full of missing objects.** A kubeconfig entry is a local file; a session is a token that expires in about a day. Between those sits a state where every local check passes, every cluster read fails, and no failure says "log in": `get namespace` became "could not confirm", and every `get serviceaccount` became "not in this namespace — it would be created". A fully prepared namespace read as an empty one, and the review offered to create five objects that were already there, contradicting the one honest line printed above them. Only `NotFound` now means absent; an auth failure says so and names `oc login`; anything else carries the cluster's own words. A `cluster_login` check does one authenticated round trip and stops everything on failure, and `setup`'s reachability gate asks the cluster rather than `config current-context`, which answers "a context is selected" for a token that died hours ago. **The credentials Secret was left entirely to the user.** So every freshly prepared namespace ended one check short of an answer: the probe mounts that Secret to authenticate, so with it absent the check that proves the namespace works could only be skipped. `setup` now offers to create it — backend, then per value: typed and masked, an environment variable you name, or a file whose required fields are printed *before* the question and validated after. Checking the credential here is the point; an unusable one is accepted into a Secret without complaint and surfaces as an authentication failure inside an agent call minutes later, looking like a model outage. Four rules govern the material, each because the obvious implementation breaks it. Never an **argv** — `--from-literal` puts the value in the process table and in the shell history of anyone who copies the line, so the manifest goes to `oc apply -f -` on **stdin**. **JSON**, not YAML: a key containing `:`, a newline or a leading `%` is ordinary here and a quoting bug waiting to happen. Never **echoed**: masked input, shape-only confirmation, redacted command, scrubbed stderr. Never **logged** but key names and value lengths. With nobody at the keyboard the step is skipped — `--yes` means "do not stop to ask me", not "choose a credential for me". It lives in its own module: `k8s_setup` was already 975 lines, and the Secret's checks belong beside the code that creates one. Also: the two long waits and the object review report into the status line, and `test_contained_k8s_division` stops reaching whatever cluster happens to be current — an unreachable one made that file wait out a timeout per test. Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: Yi Zheng <237498169+beatsmonster@users.noreply.github.com> --- docs/contained/index.md | 110 ++++- factory/cli/contained_k8s.py | 33 +- factory/contained/k8s.py | 313 +++++++++++-- factory/contained/k8s_credentials.py | 597 ++++++++++++++++++++++++ factory/contained/k8s_review.py | 40 +- factory/contained/k8s_setup.py | 270 +++++++---- tests/test_contained_k8s.py | 45 +- tests/test_contained_k8s_credentials.py | 290 ++++++++++++ tests/test_contained_k8s_division.py | 14 +- tests/test_contained_k8s_helpers.py | 244 +++++++++- tests/test_contained_k8s_review.py | 9 +- 11 files changed, 1777 insertions(+), 188 deletions(-) create mode 100644 factory/contained/k8s_credentials.py create mode 100644 tests/test_contained_k8s_credentials.py diff --git a/docs/contained/index.md b/docs/contained/index.md index 4d7c2524b..216217126 100644 --- a/docs/contained/index.md +++ b/docs/contained/index.md @@ -109,8 +109,19 @@ factory contained {ls|attach|rm|sync|setup|verify|bundle|help} [name] | `--storage-class SC` | cluster default | Workspace PVC | A flag used against the wrong target fails at parse time naming the target it belongs to — never -silently ignored. Runtime flags go **before** the subcommand; anything flag-shaped after it is an -error rather than a name. +silently ignored. + +Runtime flags go on **either side** of a subcommand. These are the same command: + +```bash +factory contained --target k8s verify +factory contained verify --target k8s +``` + +Both orders are what people type, so both work. Give the same flag on both sides with two different +values and it stops and says so rather than picking one — `--namespace a verify --namespace b` has +no obviously right reading, and choosing wrong applies RBAC to somebody else's namespace. Nothing +after `--` is interpreted at all: that belongs to the factory inside the runtime. --- @@ -411,7 +422,7 @@ credentials**: ```console $ factory contained --target k8s setup -━━ 1/3 Cluster and namespace ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ +━━ 1/4 Cluster and namespace ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ Clusters in your kubeconfig: @@ -438,7 +449,7 @@ Create namespace 'factory-contained' now? [y]es [n]o (y/N): y Created factory-contained. `oc new-project` also made it your current project. -━━ 2/3 Review and apply ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ +━━ 2/4 Review and apply ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ Comparing 5 object(s) against namespace 'factory-contained' on 'https://api.my-cluster.example.com:443': @@ -477,12 +488,36 @@ Apply this? (2 of 4) [y]es [n]o [a]ll remaining [q]uit (Enter or Esc = skip rolebinding.rbac.authorization.k8s.io/factory-scc created persistentvolumeclaim/factory-workspace created -The credentials Secret is yours to create — the factory never handles the material: - oc create secret generic factory-credentials -n factory-contained \ - --from-literal=ANTHROPIC_API_KEY=... +━━ 3/4 Credentials ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ + + secret/factory-credentials is missing from factory-contained + The pod reads this Secret as its environment. It stays in the namespace; the + factory sends the material once, here, and never reads it back. + + [1] Anthropic API key + [2] Vertex AI (Google Cloud) + [3] copy what this shell is configured for (anthropic) + [s] skip — print the command and let me do it + +Which inference backend should the pod use? [1/2/3/s]: 1 + + [t] type it now (hidden as you type) + [e] read it from an environment variable in this shell + [f] read it from a file on this machine + [q] cancel + +Where does the Anthropic API key come from? [t/e/f/q]: t +Anthropic API key: *********************************************** + ANTHROPIC_API_KEY: 108 characters, starts 'sk-ant-a', ends '9f2c' (typed) +Use this? [y]es [n]o (Y/n): y + + About to create 'secret/factory-credentials' in 'factory-contained' with: + ANTHROPIC_API_KEY: 108 characters, starts 'sk-ant-a', ends '9f2c' +Create it now? [y]es [n]o (Y/n): y + secret/factory-credentials created ``` -Six details that are deliberate. +Seven details that are deliberate. The **cluster** is asked too, not just the namespace. A kubeconfig usually holds several, and `oc config use-context` is the only way most people know to move between them — so picking the wrong @@ -555,6 +590,34 @@ There is no second, blanket "are you sure?": every object was confirmed a moment prompt on top of that is the friction that teaches people to hit `y` without reading. `--yes` applies everything pending without walking, for automation. +The **credentials Secret is offered as a step**, not left as a closing reminder. It used to be +printed as an `oc create secret` line and nothing more, which meant every freshly prepared namespace +ended one check short of an answer — the inference probe needs that Secret to authenticate, so it +could only be skipped. Pick a backend, pick where each value comes from, and it is created. + +Four rules govern the material, and each one exists because the obvious implementation breaks it: + +- **It never reaches an argv.** `oc create secret --from-literal=KEY=value` puts the value in the + process table for every user on the machine, and in the shell history of anyone who copies the + line. The Secret is composed as a manifest and fed to `oc apply -f -` on **stdin**. +- **The manifest is JSON, not YAML.** A key containing `:`, a newline or a leading `%` is ordinary + here and is a quoting bug waiting to happen in hand-built YAML. +- **Nothing is echoed.** Typing is masked; a value is confirmed by *shape only* + (`108 characters, starts 'sk-ant-a', ends '9f2c'`), which is enough to catch a paste that grabbed + the surrounding quotes and not enough to reuse. The command printed afterwards is redacted, and + anything that turns up in `oc`'s stderr is scrubbed before you see it. +- **Nothing is logged.** Key names and value lengths; never a value. + +A value can be **typed**, read from an **environment variable** you name, or read from a **file** — +and for the Google credential the required fields are printed *before* you are asked, then the file +is parsed and any missing field named. Checking it here is the point: an unusable credential is +accepted into a Secret without complaint and would otherwise surface as an authentication failure +inside an agent call, minutes later, looking like a model outage. + +If a usable Secret is already there, the step says which backend it carries and asks nothing. With +nobody at the keyboard it is **skipped** and the manual command printed — `--yes` means "do not stop +to ask me", not "choose a credential for me", and there is no safe default for that question. + Then `verify` checks every object, every verb the ServiceAccount needs, the Secret's **keys** (never its values), and that inference is reachable from a pod *inside* the namespace. Results print **as each one lands**, not at the end — several are a cluster round trip and the in-cluster inference @@ -582,8 +645,35 @@ The inference check is the slow one: it creates a short-lived pod, with the same a real run uses, and asks it to make one request — because a host-side check proves nothing about the *pod's* egress. It is announced before it starts, and **skipped entirely when the credentials Secret is missing**, since the probe pod mounts that Secret and could only spend its 180-second -timeout rediscovering what the check above already said. That is the state a freshly prepared -namespace is in, because creating the Secret is deliberately left to you. +timeout rediscovering what the check above already said. + +While it waits it **says what it is waiting for**. Anything that takes more than five seconds grows +a status line that rewrites itself in place, carrying the pod's own state and a clock: + +```console +⠹ inference_from_cluster — probe pod: ContainerCreating (0:47) +``` + +Nothing is drawn below five seconds, so a fast check looks exactly as it always did, and the line +erases itself when the result lands. In a pipe or a CI log it degrades to one plain line per change +rather than thousands of redraws; `FACTORY_NO_PROGRESS=1` turns it off entirely. + +It also **stops early when the pod cannot start**. The wait used to be `oc wait --timeout=180s`, +which is blind by construction — it can only report that a condition did not hold, so an image the +cluster could not pull cost the full three minutes and was then described as `the probe produced no +output`, naming neither the cause nor where to look. Now each poll reads the pod, and a state the +kubelet has already given up on (`ImagePullBackOff`, `CreateContainerConfigError`, a pod nothing +will schedule) returns immediately with the kubelet's own words: + +```console +[FAIL] inference_from_cluster: a pod in this namespace could NOT reach inference: the probe pod + could not run: ImagePullBackOff — Back-off pulling image "ghcr.io/…/factory-runtime:latest" +``` + +A first-ever image pull is *not* treated that way — a cold `ContainerCreating` legitimately runs for +minutes, and capping it would trade a hang for a false failure on every new node. Only states that +are already an error get the 30-second ceiling. The same reading now guards the workspace and +factory containers on the real run path, which had the identical problem with a 300-second timeout. Before setup, the same command lists what is missing with the command that restores each — e.g. `factory contained --namespace factory-contained bundle | oc apply -f -`. diff --git a/factory/cli/contained_k8s.py b/factory/cli/contained_k8s.py index 924d67530..d16b96269 100644 --- a/factory/cli/contained_k8s.py +++ b/factory/cli/contained_k8s.py @@ -26,6 +26,7 @@ import structlog +from factory.contained import style from factory.contained.credentials import resolve_credentials, vertex_model_warning from factory.contained.env import CONTAINED_ENV_POLICY from factory.contained.errors import ContainedError @@ -267,20 +268,34 @@ def _filter(entry: tarfile.TarInfo) -> tarfile.TarInfo | None: def _provision(plan: PodPlan, tarball: Path) -> None: - """Create the claim and the pod, then stream the workspace into the waiting loader.""" + """Create the claim and the pod, then stream the workspace into the waiting loader. + + Both waits report into a status line. Between them they are the longest silence in a cluster + run — the first cold pull of the runtime image happens inside them — and a wait that says + nothing is read as a hang whether or not it is one. + """ apply_manifest(render_pvc(plan.namespace, plan.storage_class), plan.namespace) apply_manifest(render_pod(plan), plan.namespace) # The identifier first, before any long-running work: a run whose name the user cannot see is a # run they cannot manage. print(plan.name) - state = wait_for_container(plan.name, plan.namespace, LOADER_CONTAINER) - if state == "running": - stream_workspace(tarball, plan.name, plan.namespace) - else: - # Already unpacked for *this* run — the pod restarted after a successful upload. The marker - # is per-run, so this can never mean "a previous run's files are already here". - log.debug("contained_workspace_already_present", pod=plan.name) - wait_for_container(plan.name, plan.namespace, FACTORY_CONTAINER) + with style.activity("workspace", "waiting for the loader container") as act: + state = wait_for_container( + plan.name, plan.namespace, LOADER_CONTAINER, + on_progress=lambda p: act.update(f"loader: {p.describe()}"), + ) + if state == "running": + act.update("streaming the workspace into the pod") + stream_workspace(tarball, plan.name, plan.namespace) + else: + # Already unpacked for *this* run — the pod restarted after a successful upload. The + # marker is per-run, so this can never mean "a previous run's files are already here". + log.debug("contained_workspace_already_present", pod=plan.name) + with style.activity("pod", "waiting for the factory container") as act: + wait_for_container( + plan.name, plan.namespace, FACTORY_CONTAINER, + on_progress=lambda p: act.update(f"factory container: {p.describe()}"), + ) def _start(plan: PodPlan, ws: Workspace, project: Path) -> int: diff --git a/factory/contained/k8s.py b/factory/contained/k8s.py index 6c5065e2d..dbd7efbeb 100644 --- a/factory/contained/k8s.py +++ b/factory/contained/k8s.py @@ -26,6 +26,8 @@ import shutil import subprocess import sys +import time +from collections.abc import Callable from dataclasses import dataclass, field from pathlib import Path from typing import Any @@ -124,6 +126,60 @@ def cli_binary() -> str: ) +# What the API server and the CLIs say when the kubeconfig has a context but no usable credential. +# An expired token is the ordinary end of a working session — `oc login` issues one that lasts a +# day — so this is not an exotic state, and it must never be mistaken for "the object is not there". +_AUTH_ERROR_MARKERS = ( + "you must be logged in", + "asked for the client to provide credentials", + "unauthorized", + "invalid bearer token", + "token has expired", + "no credentials", +) + +# What `NotFound` looks like on stderr. This is the *only* failure that means an object is absent; +# everything else means the question could not be answered. +_NOT_FOUND_MARKERS = ("not found", "notfound") + + +def is_auth_error(text: str) -> bool: + """Whether this failure means "you are logged out", rather than anything about the object.""" + lowered = (text or "").lower() + return any(marker in lowered for marker in _AUTH_ERROR_MARKERS) + + +def is_not_found(text: str) -> bool: + """Whether this failure means the object genuinely is not there. + + Everything else — Unauthorized, Forbidden, a DNS failure, a proxy — is a *question that could + not be answered*, and reporting one of those as "absent" is how a review comes to offer to + create five objects that already exist. + """ + lowered = (text or "").lower() + return any(marker in lowered for marker in _NOT_FOUND_MARKERS) + + +def login_status(binary: str) -> tuple[bool, str]: + """One authenticated round trip: is the selected context actually usable? Never raises. + + `config current-context` cannot answer this — it reads the kubeconfig on disk and succeeds + happily against a context whose token expired an hour ago. That is exactly the state where + every later read fails, and where the failures are subtle enough to be misread as answers. + """ + argv = ( + cli(binary, "whoami") if binary == "oc" + else cli(binary, "auth", "can-i", "get", "pods") + ) + result = _run(argv, timeout=30) + if result is None: + return False, f"`{' '.join(argv)}` could not be run" + if result.returncode == 0: + return True, (result.stdout or "").strip().splitlines()[0] if result.stdout.strip() else "" + detail = (result.stderr or result.stdout or "").strip().splitlines() + return False, detail[0][:200] if detail else f"exit {result.returncode}" + + def current_namespace() -> str | None: """The namespace from the current context. Never hardcoded.""" result = _run(cli(cli_binary(), "config", "view", "--minify", "-o", "jsonpath={..namespace}")) @@ -862,59 +918,226 @@ def apply_manifest(manifest: str, namespace: str) -> None: log.debug("k8s_applied", namespace=namespace, output=result.stdout.strip()[:200]) -def wait_for_container(name: str, namespace: str, container: str, *, timeout: int = 300) -> str: - """Block until `container` is running or has finished. Returns `"running"` or `"terminated"`. +# ------------------------------------------------------------------------------------------------ +# Waiting on a pod, without waiting on one that is never going to start +# ------------------------------------------------------------------------------------------------ - Both are answers, and conflating them hangs: an initContainer that already did its work on an - earlier pod for this run terminates before the host ever looks, and a wait that only accepts - "running" then times out against a container that succeeded. +# Waiting reasons the kubelet reports only once it has already given up, or that no amount of time +# can resolve. Waiting out any of these buys nothing: the answer will not change, and the minutes +# spent are minutes the user spends looking at a blank screen before being told something the very +# first poll knew. `ImagePullBackOff` belongs here and `ErrImagePull` does not — *BackOff* is the +# word the kubelet uses after it has already retried. +DOOMED_WAITING_REASONS = frozenset({ + "ImagePullBackOff", + "ErrImageNeverPull", + "InvalidImageName", + "CreateContainerConfigError", + "CreateContainerError", + "RunContainerError", + "CrashLoopBackOff", +}) + +# Reasons that *might* still resolve — a registry blip, a node under load — but that are an error +# rather than progress. Tolerated for `stuck_after` seconds and then treated as doomed. +RETRYABLE_WAITING_REASONS = frozenset({"ErrImagePull", "ImageInspectError"}) + +# How long an error-ish state is given to clear itself before the wait gives up. Deliberately not +# applied to `ContainerCreating`, which is what a first-ever image pull looks like and legitimately +# runs for minutes; capping that would trade a hang for a false failure on every cold cache. +STUCK_AFTER_SECONDS = 30 + +RUNNING, SUCCEEDED, WAITING, DOOMED = "running", "succeeded", "waiting", "doomed" - Polled rather than `oc wait`ed: the condition here is per-container ("the loader is up"), and - `oc wait --for=condition=Ready` is per-pod and is never satisfied while an initContainer is - still running — which is precisely the window the upload needs. + +@dataclass(frozen=True) +class PodProgress: + """What a pod is doing, and whether waiting longer could change it. + + `verdict` is one of `running`, `succeeded`, `waiting`, `doomed`. The split that matters is the + last one: a pod whose image cannot be pulled and a pod that is still pulling look identical + from a `--timeout` flag, and only one of them is worth waiting for. """ - import time + verdict: str + phase: str = "" + reason: str = "" + message: str = "" + + def describe(self) -> str: + """One human-readable line — the kubelet's own words wherever it supplied any.""" + head = self.reason or self.phase or self.verdict + detail = " ".join(self.message.split())[:200] + return f"{head} — {detail}" if detail else head + + +def classify_pod(pod: dict[str, Any], *, container: str | None = None) -> PodProgress: + """Read one `oc get pod -o json` into a verdict. Pure: no cluster calls, no clock. + + `container` narrows the question to one container of the pod, which is what the workspace + loader needs — an initContainer that is *running* is the window the upload has to hit, and a + pod-level readiness condition is never true during it. + + Anything unrecognized is `waiting`, never `doomed`: this decides whether to stop waiting, and + the cost of being wrong in that direction is a run aborted for a state that would have cleared. + """ + # `or {}` as well as the type check: a key that is present and explicitly null is not the same + # as an absent one, and `.get(k, {})` returns the null. A half-written pod document has to + # classify as "keep waiting", never raise inside a poll loop. + status = (pod.get("status") or {}) if isinstance(pod, dict) else {} + if not isinstance(status, dict): + return PodProgress(WAITING, "", "Pending", "") + phase = str(status.get("phase", "") or "") + + statuses = [ + entry for entry in + (list(status.get("initContainerStatuses", []) or []) + + list(status.get("containerStatuses", []) or [])) + if isinstance(entry, dict) and (container is None or entry.get("name") == container) + ] + + for entry in statuses: + state = entry.get("state", {}) or {} + terminated = state.get("terminated") + if isinstance(terminated, dict): + code = terminated.get("exitCode") + if code == 0: + return PodProgress(SUCCEEDED, phase, str(terminated.get("reason") or "Completed"), + str(terminated.get("message") or "")) + return PodProgress( + DOOMED, phase, str(terminated.get("reason") or "Error"), + str(terminated.get("message") or f"exit code {code}"), + ) + if "running" in state: + return PodProgress(RUNNING, phase, "Running", "") + waiting = state.get("waiting") + if isinstance(waiting, dict): + reason = str(waiting.get("reason") or "") + message = str(waiting.get("message") or "") + if reason in DOOMED_WAITING_REASONS: + return PodProgress(DOOMED, phase, reason, message) + return PodProgress(WAITING, phase, reason or "Waiting", message) + + # No container has a state yet — the answer is at pod level, where an unschedulable pod lives. + unschedulable = _unschedulable(status) + if unschedulable is not None: + return PodProgress(DOOMED, phase, "Unschedulable", unschedulable) + if phase == "Succeeded": + return PodProgress(SUCCEEDED, phase, "Succeeded", "") + if phase == "Failed": + return PodProgress(DOOMED, phase, str(status.get("reason") or "Failed"), + str(status.get("message") or "")) + return PodProgress(WAITING, phase, phase or "Pending", "") + + +def _unschedulable(status: dict[str, Any]) -> str | None: + """The scheduler's explanation, when it declined to place the pod at all. + + A pod nothing will ever schedule — no node with the requested resources, a PVC in a zone the + nodes are not in — sits in `Pending` with no container status whatsoever, which is + indistinguishable from "starting" unless the conditions are read. + """ + for condition in status.get("conditions", []) or []: + if not isinstance(condition, dict): + continue + if (condition.get("type") == "PodScheduled" and condition.get("status") == "False" + and condition.get("reason") == "Unschedulable"): + return str(condition.get("message") or "no node can accept this pod") + return None + + +def read_pod(name: str, namespace: str) -> dict[str, Any] | None: + """One pod as a dict, or None when it could not be read. Never raises.""" + result = _run(cli(cli_binary(), "get", "pod", name, "-n", namespace, "-o", "json"), timeout=30) + if result is None or result.returncode != 0: + return None + try: + pod = json.loads(result.stdout or "{}") + except json.JSONDecodeError: + return None + return pod if isinstance(pod, dict) else None + + +def poll_pod( + name: str, + namespace: str, + *, + container: str | None = None, + until: tuple[str, ...] = (RUNNING, SUCCEEDED), + timeout: int = 300, + stuck_after: int = STUCK_AFTER_SECONDS, + interval: float = 2.0, + on_progress: Callable[[PodProgress], None] | None = None, +) -> PodProgress: + """Wait for a pod to reach one of `until`, or to prove it never will. + + Replaces `oc wait --for=... --timeout=Ns`, which is blind by construction: it reports only that + the condition did not hold, so an image that cannot be pulled costs the full timeout and is then + described as a mystery. Here the pod is read each round and `classify_pod` decides — a doomed + state returns immediately carrying the kubelet's own message, and an error state that might + still clear is given `stuck_after` seconds to do so. + + `on_progress` is called with every changed state, which is how a caller drives a status line. + Returns the last `PodProgress`; a timeout is returned as `doomed` with reason `Timeout` rather + than raised, because both callers want to add their own context to it. + """ deadline = time.monotonic() + timeout - last = "" + error_since: float | None = None + last = PodProgress(WAITING, "", "Pending", "") + reported = "" while time.monotonic() < deadline: - result = _run(cli(cli_binary(), "get", "pod", name, "-n", namespace, "-o", "json")) - if result is not None and result.returncode == 0: - try: - pod = json.loads(result.stdout or "{}") - except json.JSONDecodeError: - pod = {} - statuses = pod.get("status", {}).get("initContainerStatuses", []) + pod.get( - "status", {} - ).get("containerStatuses", []) - for status in statuses: - if status.get("name") != container: - continue - state = status.get("state", {}) - if "running" in state: - return "running" - terminated = state.get("terminated") - if isinstance(terminated, dict): - if terminated.get("exitCode") == 0: - return "terminated" - raise ClusterError( - f"container {container} in pod {name} exited " - f"{terminated.get('exitCode')} ({terminated.get('reason')}). " - f"`{cli_binary()} logs {name} -c {container} -n {namespace}` has why." + pod = read_pod(name, namespace) + if pod is not None: + last = classify_pod(pod, container=container) + if on_progress is not None and last.describe() != reported: + reported = last.describe() + on_progress(last) + if last.verdict in until: + return last + if last.verdict == DOOMED: + return last + if last.reason in RETRYABLE_WAITING_REASONS: + error_since = error_since or time.monotonic() + if time.monotonic() - error_since >= stuck_after: + return PodProgress( + DOOMED, last.phase, last.reason, + f"{last.message} (unchanged for {stuck_after}s)".strip(), ) - last = json.dumps(state)[:200] - phase = pod.get("status", {}).get("phase", "") - if phase in ("Failed", "Succeeded") and not last: - raise ClusterError( - f"pod {name} reached {phase} before {container} ran. " - f"`{cli_binary()} describe pod {name} -n {namespace}` has the reason." - ) - time.sleep(2) + else: + error_since = None + time.sleep(interval) + return PodProgress(DOOMED, last.phase, "Timeout", + f"still {last.describe()} after {timeout}s") + + +def wait_for_container( + name: str, namespace: str, container: str, *, timeout: int = 300, + on_progress: Callable[[PodProgress], None] | None = None, +) -> str: + """Block until `container` is running or has finished. Returns `"running"` or `"terminated"`. + + Both are answers, and conflating them hangs: an initContainer that already did its work on an + earlier pod for this run terminates before the host ever looks, and a wait that only accepts + "running" then times out against a container that succeeded. + + Polled rather than `oc wait`ed, for two reasons. The condition is per-container ("the loader is + up") and `oc wait --for=condition=Ready` is per-pod, never satisfied while an initContainer is + still running — precisely the window the upload needs. And polling is what makes it possible to + stop early: this used to spend its full five minutes against an `ImagePullBackOff` before + reporting a timeout whose cause the very first poll already knew. + """ + progress = poll_pod( + name, namespace, container=container, timeout=timeout, on_progress=on_progress, + ) + if progress.verdict == RUNNING: + return "running" + if progress.verdict == SUCCEEDED: + return "terminated" raise ClusterError( - f"timed out after {timeout}s waiting for container {container} in pod {name} to run" - + (f" (last state: {last})" if last else "") - + f". `{cli_binary()} describe pod {name} -n {namespace}` has the reason — an unschedulable " - "pod and an unpullable image both look like this from here." + f"container {container} in pod {name} did not start: {progress.describe()}. " + f"`{cli_binary()} describe pod {name} -n {namespace}` has the full story" + + (f", and `{cli_binary()} logs {name} -c {container} -n {namespace}` has its output" + if progress.phase in ("Running", "Failed", "Succeeded") else "") + + "." ) diff --git a/factory/contained/k8s_credentials.py b/factory/contained/k8s_credentials.py new file mode 100644 index 000000000..70932cd29 --- /dev/null +++ b/factory/contained/k8s_credentials.py @@ -0,0 +1,597 @@ +"""The namespace's credentials Secret — checking it, and creating it without mishandling it. + +`setup` used to print an `oc create secret` line and stop, which left every freshly prepared +namespace failing `verify` on the one step that decides whether it can do any work at all. This +module closes that gap while keeping the material's exposure as small as the job allows. + +Four rules hold everywhere below, and each exists because the obvious implementation breaks it: + +- **Never in an argv.** `oc create secret --from-literal=KEY=value` puts the value in the process + table for every user on the machine and into the shell history of anyone who copies the line. The + Secret is composed as a manifest and fed to `oc apply -f -` on **stdin** instead. +- **Never in YAML.** The manifest is JSON. A key containing `:`, a newline or a leading `%` is + ordinary in this domain and is a quoting bug waiting to happen in hand-built YAML; JSON has one + escaping rule and `oc apply` reads it natively. +- **Never echoed.** Typed input is masked, the command printed afterwards is redacted, and any + value that somehow appears in a subprocess's stderr is scrubbed before it is shown. +- **Never logged.** structlog records key names and value *lengths*. A log line is a file, and a + file is the thing this whole module is trying to keep the credential out of. + +The step is skipped entirely when nobody is at the keyboard. `--yes` means "do not stop to ask me", +not "invent a credential", and there is no safe default for this question. +""" + +from __future__ import annotations + +import json +import os +import subprocess +from dataclasses import dataclass, field +from pathlib import Path + +import structlog + +from factory.contained import style +from factory.contained.credentials import ( + ADC_DIR, + ADC_FILE, + VERTEX_PINNED_ENV, + resolve_credentials, +) +from factory.contained.k8s import ADC_SECRET_KEY, LABEL_CONTAINED, SECRET_NAME, cli +from factory.contained.prereq import Check + +log = structlog.get_logger() + +# The keys a credentials Secret must carry for at least one supported backend. +ANTHROPIC_KEYS = ("ANTHROPIC_API_KEY",) +# The three configuration variables *and* the credential file. The credential is the point: the +# first three only say which endpoint to talk to, so a Secret carrying just those was reported as +# "carries the Vertex configuration" while holding nothing that could authenticate. +VERTEX_KEYS = ( + "CLAUDE_CODE_USE_VERTEX", "CLOUD_ML_REGION", "ANTHROPIC_VERTEX_PROJECT_ID", ADC_SECRET_KEY, +) + +# What a Google Application Default Credentials file has to contain, by its own `type`. Checked +# before the file is uploaded because the alternative is finding out from inside a pod, where the +# failure surfaces as an authentication error several minutes into an agent call. +ADC_REQUIRED_FIELDS = { + "authorized_user": ("client_id", "client_secret", "refresh_token"), + "service_account": ("project_id", "private_key", "client_email"), +} + +ADC_TEMPLATE = """\ +{ + "type": "authorized_user", + "client_id": "....apps.googleusercontent.com", + "client_secret": "...", + "refresh_token": "...", + "quota_project_id": "your-project" // optional +} + +A service account key is also accepted; it needs "type": "service_account" plus +"project_id", "private_key" and "client_email". The usual way to produce the first +form is `gcloud auth application-default login`, which writes exactly this file to +~/.config/gcloud/application_default_credentials.json.""" + + +# ------------------------------------------------------------------------------------------------ +# Reading what is there +# ------------------------------------------------------------------------------------------------ + + +def _run(argv: list[str], *, timeout: int = 60) -> subprocess.CompletedProcess[str] | None: + try: + return subprocess.run(argv, capture_output=True, text=True, timeout=timeout) + except (FileNotFoundError, PermissionError, OSError, subprocess.TimeoutExpired): + return None + + +def create_secret_command(binary: str, namespace: str) -> str: + """The manual route, for the fix line and for anyone who would rather not be walked through it. + + Shown with `...` where the material goes. It is a template, and a user who fills it in has + chosen to put a key in their shell history; the guided path exists so that is not the only + option. + """ + return ( + f"factory contained --target k8s --namespace {namespace} setup # walks you through it\n" + f" or by hand:\n" + f" {binary} create secret generic {SECRET_NAME} -n {namespace} \\\n" + f" --from-literal=ANTHROPIC_API_KEY=...\n" + f" or, for Vertex:\n" + f" {binary} create secret generic {SECRET_NAME} -n {namespace} \\\n" + f" --from-literal=CLAUDE_CODE_USE_VERTEX=1 \\\n" + f" --from-literal=CLOUD_ML_REGION= \\\n" + f" --from-literal=ANTHROPIC_VERTEX_PROJECT_ID= \\\n" + f" --from-file={ADC_SECRET_KEY}=$HOME/.config/gcloud/" + f"application_default_credentials.json" + ) + + +def secret_check(binary: str, namespace: str) -> Check: + """The Secret must exist and carry a usable backend's keys — its *keys*, never its values.""" + result = _run(cli(binary, "get", "secret", SECRET_NAME, "-n", namespace, + "-o", "jsonpath={.data}")) + if result is None or result.returncode != 0: + return Check( + name="credentials_secret", + ok=False, + detail=f"secret/{SECRET_NAME} is missing from {namespace}", + fix=create_secret_command(binary, namespace), + ) + keys = _keys_of(result.stdout) + if set(ANTHROPIC_KEYS) <= keys: + return Check(name="credentials_secret", ok=True, + detail=f"secret/{SECRET_NAME} carries the Anthropic API key") + if set(VERTEX_KEYS) <= keys: + return Check(name="credentials_secret", ok=True, + detail=f"secret/{SECRET_NAME} carries the Vertex configuration") + return Check( + name="credentials_secret", + ok=False, + detail=( + f"secret/{SECRET_NAME} exists but carries none of the supported backends' keys " + f"(has: {', '.join(sorted(keys)) or 'nothing'})" + ), + fix=create_secret_command(binary, namespace), + ) + + +def _keys_of(raw: str) -> set[str]: + try: + data = json.loads(raw or "{}") + except json.JSONDecodeError: + return set() + return set(data) if isinstance(data, dict) else set() + + +def secret_exists(binary: str, namespace: str) -> bool: + result = _run(cli(binary, "get", "secret", SECRET_NAME, "-n", namespace, "-o", "name")) + return result is not None and result.returncode == 0 + + +# ------------------------------------------------------------------------------------------------ +# Describing a value without disclosing it +# ------------------------------------------------------------------------------------------------ + +# Below this, an excerpt would be most of the value. Short credentials are described by length only. +_EXCERPT_FLOOR = 16 + + +def describe_value(value: str) -> str: + """A value's shape: enough to recognise a paste that went wrong, not enough to reuse. + + A masked prompt tells you *something* arrived; it cannot tell you *what*. The common mistake + this catches is a copy that grabbed the surrounding quotes, or an environment variable holding + the name of a key rather than a key. + """ + length = len(value) + if length < _EXCERPT_FLOOR: + return f"{length} characters" + return f"{length} characters, starts {value[:8]!r}, ends {value[-4:]!r}" + + +def redact(text: str, values: tuple[str, ...]) -> str: + """Scrub known material out of text that is about to be shown. + + Applied to subprocess stderr. `oc` does not normally echo a Secret's contents back, but "does + not normally" is not a property worth betting a credential on, and a malformed manifest is + exactly the case where a parser quotes the input it choked on. + """ + for value in values: + if value and len(value) >= 4: + text = text.replace(value, "***") + return text + + +# ------------------------------------------------------------------------------------------------ +# Composing and applying +# ------------------------------------------------------------------------------------------------ + + +def build_secret_manifest(namespace: str, data: dict[str, str]) -> str: + """The Secret, as JSON. + + `stringData` rather than `data`, so the API server does the base64 and nothing here has to. + JSON rather than YAML for the escaping reason in the module docstring — every value in here is + attacker-shaped by accident: long, random, and full of characters YAML gives meaning to. + """ + return json.dumps( + { + "apiVersion": "v1", + "kind": "Secret", + "metadata": { + "name": SECRET_NAME, + "namespace": namespace, + "labels": {LABEL_CONTAINED: "true"}, + }, + "type": "Opaque", + "stringData": data, + }, + indent=2, + ) + + +def redacted_command(binary: str, namespace: str, data: dict[str, str]) -> str: + """What was done, in a form that is readable and deliberately not runnable-with-secret.""" + literals = " \\\n ".join( + f"--from-literal={key}={'***' if _is_material(key) else value}" + for key, value in data.items() + ) + return ( + f"{binary} create secret generic {SECRET_NAME} -n {namespace} \\\n {literals}" + ) + + +# Keys whose values are credentials. The rest of a backend's shape — which region, which project, +# which flag — is configuration, and printing it is how a user confirms they configured the right +# thing. +def _is_material(key: str) -> bool: + return key == ADC_SECRET_KEY or "KEY" in key or "TOKEN" in key or "SECRET" in key + + +def apply_secret(binary: str, namespace: str, data: dict[str, str]) -> tuple[bool, str]: + """Create or replace the Secret from stdin. Never raises, never echoes the material.""" + manifest = build_secret_manifest(namespace, data) + values = tuple(data.values()) + try: + result = subprocess.run( + cli(binary, "apply", "-n", namespace, "-f", "-"), + input=manifest, capture_output=True, text=True, timeout=120, + ) + except (FileNotFoundError, PermissionError, OSError, subprocess.TimeoutExpired) as exc: + return False, redact(f"{type(exc).__name__}: {exc}", values) + log.info( + "contained_secret_applied", + namespace=namespace, + ok=result.returncode == 0, + # Names and sizes. The values are the one thing that must not reach a log file. + keys={key: len(value) for key, value in data.items()}, + ) + if result.returncode == 0: + return True, redact((result.stdout or "").strip(), values) + detail = redact((result.stderr or "").strip(), values).splitlines() + return False, detail[0][:200] if detail else "no detail given" + + +# ------------------------------------------------------------------------------------------------ +# Asking +# ------------------------------------------------------------------------------------------------ + + +@dataclass(frozen=True) +class Field: + """One key of the Secret, and where its value may come from.""" + + key: str + question: str + material: bool = False + fixed: str | None = None # not asked at all + default_env: tuple[str, ...] = () # variables to offer as the source + default_value: str = "" + from_file: bool = False + file_default: Path | None = None + json_template: str = "" + validate_json: bool = False + + +ANTHROPIC_FIELDS = ( + Field( + key="ANTHROPIC_API_KEY", + question="Anthropic API key", + material=True, + default_env=("ANTHROPIC_API_KEY",), + ), +) + +VERTEX_FIELDS = ( + Field(key="CLAUDE_CODE_USE_VERTEX", question="", fixed="1"), + Field(key="CLOUD_ML_REGION", question="Vertex region", default_env=("CLOUD_ML_REGION",)), + Field( + key="ANTHROPIC_VERTEX_PROJECT_ID", + question="Google Cloud project ID", + default_env=("ANTHROPIC_VERTEX_PROJECT_ID",), + ), + Field( + key=ADC_SECRET_KEY, + question="Application Default Credentials file", + material=True, + from_file=True, + file_default=ADC_DIR / ADC_FILE, + json_template=ADC_TEMPLATE, + validate_json=True, + ), +) + + +@dataclass +class _Readers: + """The three input functions, in one place so tests can supply their own. + + `tests/conftest.py` forces raw terminal reads off, and a prompt reached under pytest blocks on + a keypress that never comes. Injecting is the only way to exercise this flow at all. + """ + + line: object = field(default=None) + secret: object = field(default=None) + select: object = field(default=None) + + def read_line(self, question: str, default: str | None = None) -> str | None: + reader = self.line or style.read_line + return reader(question, default) # type: ignore[operator] + + def read_secret(self, question: str) -> str | None: + reader = self.secret or style.read_secret + return reader(question) # type: ignore[operator] + + def read_select(self, question: str, options: list[tuple[str, str]]) -> str | None: + reader = self.select or style.select + return reader(question, options) # type: ignore[operator] + + +def _collect_field(field_spec: Field, readers: _Readers) -> str | None: + """One key's value, from whichever source the user picks. `None` means they backed out.""" + if field_spec.fixed is not None: + return field_spec.fixed + if not field_spec.material and not field_spec.from_file: + default = next( + (os.environ[name] for name in field_spec.default_env if os.environ.get(name)), "" + ) or field_spec.default_value + answer = readers.read_line(field_spec.question, default or None) + if answer is None: + return None + return answer.strip() or default + + options = [("t", "type it now (hidden as you type)")] + if field_spec.default_env or not field_spec.from_file: + options.append(("e", "read it from an environment variable in this shell")) + options.append(("f", "read it from a file on this machine")) + options.append(("q", "cancel")) + + while True: + source = readers.read_select(f"Where does the {field_spec.question} come from?", options) + if source is None or source == "q": + return None + value = _read_from_source(source, field_spec, readers) + if value is not None: + return value + # A source that could not supply a value returns here rather than aborting: choosing the + # wrong variable name is a slip, not a decision to stop. + + +def _read_from_source(source: str, field_spec: Field, readers: _Readers) -> str | None: + if source == "t": + if field_spec.json_template: + print(style.note("Paste the file's contents, or press Escape and choose the file " + "instead — which is easier for anything multi-line.")) + typed = readers.read_secret(field_spec.question + ":") + if not typed: + return None + return _confirm_value(field_spec, typed, readers) + if source == "e": + return _from_environment(field_spec, readers) + return _from_file(field_spec, readers) + + +def _from_environment(field_spec: Field, readers: _Readers) -> str | None: + suggestion = next((name for name in field_spec.default_env), None) + name = readers.read_line("Which environment variable?", suggestion) + if name is None: + return None + name = (name.strip() or suggestion or "").strip() + if not name: + return None + value = os.environ.get(name, "") + if not value.strip(): + print(style.line(style.paint( + f"{name} is not set in this shell (or is empty). Nothing was read.", "yellow" + ))) + return None + return _confirm_value(field_spec, value.strip(), readers, source=f"${name}") + + +def _from_file(field_spec: Field, readers: _Readers) -> str | None: + if field_spec.json_template: + # Before the question, not after a rejection: a template shown only once the answer is + # wrong is a template shown to somebody who has already gone and found the wrong file. + print(style.note("This file must contain:")) + for chunk in field_spec.json_template.splitlines(): + print(style.line(style.dim(chunk))) + default = str(field_spec.file_default) if field_spec.file_default else None + typed = readers.read_line("Path to the file", default) + if typed is None: + return None + path = Path((typed.strip() or default or "")).expanduser() + if not str(path): + return None + try: + content = path.read_text() + except OSError as exc: + print(style.line(style.paint(f"Could not read {path}: {exc.strerror or exc}", "yellow"))) + return None + if field_spec.validate_json: + problem = validate_adc(content) + if problem is not None: + print(style.line(style.paint(f"{path} is not usable: {problem}", "yellow"))) + return None + return _confirm_value(field_spec, content, readers, source=str(path)) + + +def validate_adc(content: str) -> str | None: + """`None` when the text is a usable ADC document, else why it is not. + + Checked here rather than in the cluster because the cluster cannot check it: an unusable + credential is accepted into a Secret without complaint and surfaces as an authentication error + inside an agent call, minutes later, indistinguishable from a model outage. + """ + try: + data = json.loads(content) + except json.JSONDecodeError as exc: + return f"it is not valid JSON ({exc.msg} at line {exc.lineno})" + if not isinstance(data, dict): + return "it is JSON, but not an object" + kind = str(data.get("type", "")) + if kind not in ADC_REQUIRED_FIELDS: + return ( + f"its \"type\" is {kind or 'missing'}; expected one of " + f"{', '.join(sorted(ADC_REQUIRED_FIELDS))}" + ) + missing = [name for name in ADC_REQUIRED_FIELDS[kind] if not str(data.get(name, "")).strip()] + if missing: + return f"a {kind} document is missing: {', '.join(missing)}" + return None + + +def _confirm_value( + field_spec: Field, value: str, readers: _Readers, source: str = "typed" +) -> str | None: + """Show the value's shape and have it confirmed. Shape only — never the value.""" + if not field_spec.material: + return value + print(style.line( + f"{style.bold(field_spec.key)}: {describe_value(value)} {style.dim(f'({source})')}" + )) + answer = style.confirm("Use this?", default=True) + return value if answer else None + + +# ------------------------------------------------------------------------------------------------ +# The step +# ------------------------------------------------------------------------------------------------ + + +def run_credentials_step( + binary: str, + namespace: str, + *, + interactive: bool, + assume_yes: bool = False, + readers: _Readers | None = None, +) -> bool: + """Leave the namespace holding usable credentials, or say exactly how to add them. + + Returns whether a usable Secret is now in place. Never raises: every failure here is a thing to + report and carry on from, since `verify` runs immediately afterwards and will say so again. + """ + readers = readers or _Readers() + existing = secret_check(binary, namespace) + if existing.ok: + print(style.line(style.paint(existing.detail, "green"))) + print(style.note("Nothing to do. Delete it and re-run setup if you want to change it.")) + return True + + if not interactive: + # `--yes` is deliberately not enough. It means "do not stop to ask me", and there is no + # answer to "which credential" that can be assumed on a user's behalf. + print(style.line(style.paint(existing.detail, "yellow"))) + print(style.note( + "Nobody is at the keyboard, so this step is being skipped — a credential is never " + "chosen on your behalf. Create it with:" + )) + for chunk in create_secret_command(binary, namespace).splitlines(): + print(style.line(style.dim(chunk))) + return False + + print(style.line(style.paint(existing.detail, "yellow"))) + if secret_exists(binary, namespace): + print(style.note( + f"A secret/{SECRET_NAME} is already there but carries no backend this factory " + "understands. Continuing replaces it." + )) + print(style.note( + "The pod reads this Secret as its environment. It stays in the namespace; the factory " + "sends the material once, here, and never reads it back." + )) + + data = _choose_backend(readers) + if data is None: + print(style.line("Skipped. Create it yourself with:")) + for chunk in create_secret_command(binary, namespace).splitlines(): + print(style.line(style.dim(chunk))) + return False + + print() + print(style.line(f"About to create {style.value(f'secret/{SECRET_NAME}')} in " + f"{style.value(namespace)} with:")) + for key, value in data.items(): + shown = describe_value(value) if _is_material(key) else style.value(value) + print(style.field(key, shown, pad=34)) + if style.confirm("Create it now?", default=True) is not True: + print(style.line("Nothing was created.")) + return False + + created, detail = apply_secret(binary, namespace, data) + if not created: + print(style.line(style.paint(f"Could not create the Secret: {detail}", "red"))) + print(style.note("This is usually a permissions problem. Whoever owns the namespace can " + "create it with the command above.")) + return False + print(style.line(style.paint(detail or f"secret/{SECRET_NAME} created.", "green"))) + print(style.note("For the record, redacted — the material was sent on stdin, never in an " + "argument:")) + for chunk in redacted_command(binary, namespace, data).splitlines(): + print(style.line(style.dim(chunk))) + return secret_check(binary, namespace).ok + + +def _choose_backend(readers: _Readers) -> dict[str, str] | None: + """Which backend, then its values. `None` means the user chose to do it themselves.""" + local = resolve_credentials() + options = [ + ("1", "Anthropic API key"), + ("2", "Vertex AI (Google Cloud)"), + ] + if local.ok and local.backend in ("anthropic", "vertex"): + options.append(("3", f"copy what this shell is configured for ({local.backend})")) + options.append(("s", "skip — print the command and let me do it")) + + picked = readers.read_select("Which inference backend should the pod use?", options) + if picked is None or picked == "s": + return None + if picked == "3": + return _copy_from_shell(local.backend) + fields = ANTHROPIC_FIELDS if picked == "1" else VERTEX_FIELDS + collected: dict[str, str] = {} + for field_spec in fields: + value = _collect_field(field_spec, readers) + if value is None: + return None + collected[field_spec.key] = value + if fields is VERTEX_FIELDS: + # Not a credential, but the pod has no other route to it and the run behaves differently + # without it — the local target pins the same value for the same reason. + collected.update(VERTEX_PINNED_ENV) + return collected + + +def _copy_from_shell(backend: str) -> dict[str, str] | None: + """Rebuild the shape this shell already resolves, reading the ADC file where one is needed.""" + if backend == "anthropic": + key = os.environ.get("ANTHROPIC_API_KEY", "").strip() + return {"ANTHROPIC_API_KEY": key} if key else None + collected = { + name: os.environ.get(name, "").strip() + for name in ("CLAUDE_CODE_USE_VERTEX", "CLOUD_ML_REGION", "ANTHROPIC_VERTEX_PROJECT_ID") + } + if not all(collected.values()): + print(style.line(style.paint( + "This shell's Vertex configuration is incomplete; answer the questions instead.", + "yellow", + ))) + return None + try: + content = (ADC_DIR / ADC_FILE).read_text() + except OSError as exc: + print(style.line(style.paint( + f"Could not read {ADC_DIR / ADC_FILE}: {exc.strerror or exc}. Run " + "`gcloud auth application-default login` first.", "yellow", + ))) + return None + problem = validate_adc(content) + if problem is not None: + print(style.line(style.paint(f"{ADC_DIR / ADC_FILE} is not usable: {problem}", "yellow"))) + return None + collected[ADC_SECRET_KEY] = content + collected.update(VERTEX_PINNED_ENV) + return collected diff --git a/factory/contained/k8s_review.py b/factory/contained/k8s_review.py index b346659ec..61e1a35f7 100644 --- a/factory/contained/k8s_review.py +++ b/factory/contained/k8s_review.py @@ -29,7 +29,7 @@ from factory.contained import style from factory.contained.bundle import BundleObject -from factory.contained.k8s import cli +from factory.contained.k8s import cli, is_auth_error, is_not_found log = structlog.get_logger() @@ -67,10 +67,21 @@ def _run(argv: list[str], *, stdin: str | None = None, def inspect_objects( - objects: list[BundleObject], namespace: str, binary: str + objects: list[BundleObject], namespace: str, binary: str, + on_object: Callable[[BundleObject], None] | None = None, ) -> list[ObjectState]: - """Compare each object against the cluster. Never raises; an unreadable object is `unknown`.""" - return [_inspect_one(obj, namespace, binary) for obj in objects] + """Compare each object against the cluster. Never raises; an unreadable object is `unknown`. + + `on_object` is called before each comparison, so a caller can say which one is being read. Two + cluster round trips per object means a six-object bundle is comfortably long enough to look + stopped. + """ + states = [] + for obj in objects: + if on_object is not None: + on_object(obj) + states.append(_inspect_one(obj, namespace, binary)) + return states def _inspect_one(obj: BundleObject, namespace: str, binary: str) -> ObjectState: @@ -79,7 +90,26 @@ def _inspect_one(obj: BundleObject, namespace: str, binary: str) -> ObjectState: if present is None: return ObjectState(obj, UNKNOWN, detail=f"could not reach the cluster to check {obj.ref}") if present.returncode != 0: - return ObjectState(obj, ABSENT, detail="not in this namespace — it would be created") + # **Only `NotFound` means absent.** Treating every non-zero exit as "not there" made an + # expired login look like an empty namespace: all five objects were offered for creation + # against a namespace that already had them, and the one honest line on screen — "could not + # confirm whether the namespace exists" — was contradicted by the five under it. + stderr = (present.stderr or "").strip() + if is_not_found(stderr): + return ObjectState(obj, ABSENT, detail="not in this namespace — it would be created") + if is_auth_error(stderr): + return ObjectState( + obj, UNKNOWN, + detail=f"could not be checked — not logged in to this cluster ({binary} login ...)", + ) + first = stderr.splitlines() + return ObjectState( + obj, UNKNOWN, + detail=( + f"could not be checked: {first[0][:140]}" if first + else f"could not be checked (exit {present.returncode})" + ), + ) # `diff` exits 0 for no change and 1 for a change; anything higher is a real error, and so is 1 # with nothing on stdout (some builds report a failure that way). diff --git a/factory/contained/k8s_setup.py b/factory/contained/k8s_setup.py index d2b4765c7..49ca023f3 100644 --- a/factory/contained/k8s_setup.py +++ b/factory/contained/k8s_setup.py @@ -15,8 +15,9 @@ missing, the object that failed is named and the walk carries on — `verify` then reports exactly what is absent, so a partial apply is never dressed up as success. -The credentials Secret stays outside that flow. `setup` prints the `oc create secret` command and -never handles the material. +The credentials Secret is settled as its own step, in `k8s_credentials`. It used to be left +entirely to the user, which meant every freshly prepared namespace ended one check short of an +answer — the inference probe needs that Secret to authenticate, so it could only be skipped. """ from __future__ import annotations @@ -32,12 +33,14 @@ from factory.contained import style from factory.contained.bundle import BundleObject, bundle_objects, render_bundle from factory.contained.k8s import ( - ADC_SECRET_KEY, + DOOMED, LABEL_CONTAINED, SECRET_NAME, SERVICE_ACCOUNT, + SUCCEEDED, ClusterContext, ClusterError, + PodProgress, access_review, build_api_resources_argv, cli, @@ -45,11 +48,21 @@ active_context, cluster_context, current_namespace, + is_auth_error, + is_not_found, list_contexts, + login_status, + poll_pod, resolve_namespace, set_active_context, use_context, ) +from factory.contained.k8s_credentials import ( + ANTHROPIC_KEYS, + VERTEX_KEYS, + run_credentials_step, + secret_check, +) from factory.contained.k8s_review import inspect_objects, render_summary, walk from factory.contained.prereq import Check, format_check, summary_line from factory.contained.secrets import gitleaks_available @@ -57,19 +70,15 @@ log = structlog.get_logger() -# The cluster half of `setup`: choose a namespace, review-and-apply object by object, verify. -# Three rather than four because applying is no longer a step of its own — each object is applied -# at the moment it is accepted, so there is nothing left to batch afterwards. -_K8S_STEPS = 3 - -# The keys a credentials Secret must carry for at least one supported backend. -ANTHROPIC_KEYS = ("ANTHROPIC_API_KEY",) -# The three configuration variables *and* the credential file. The credential is the point: the -# first three only say which endpoint to talk to, so a Secret carrying just those was reported as -# "carries the Vertex configuration" while holding nothing that could authenticate. -VERTEX_KEYS = ( - "CLAUDE_CODE_USE_VERTEX", "CLOUD_ML_REGION", "ANTHROPIC_VERTEX_PROJECT_ID", ADC_SECRET_KEY, -) +# The cluster half of `setup`: choose a namespace, review-and-apply object by object, settle the +# credentials, verify. Applying is not a step of its own — each object is applied at the moment it +# is accepted, so there is nothing left to batch afterwards. +_K8S_STEPS = 4 + +# Re-exported: `ANTHROPIC_KEYS`, `VERTEX_KEYS` and the Secret checks now live in +# `k8s_credentials`, beside the code that creates one. Imported here so the names this module has +# always exposed keep resolving. +__all__ = ["ANTHROPIC_KEYS", "VERTEX_KEYS", "setup_k8s", "verify_k8s"] # The verbs the pod's ServiceAccount needs. Checked as the ServiceAccount, not as the user: a # namespace where *you* can create pods but the pod cannot read its own logs fails on the agent's @@ -140,13 +149,24 @@ def record(*new: Check) -> None: ) return checks - context = _context_check(binary) + with style.activity("cluster_cli", "reading the current context"): + context = _context_check(binary) record(context) if not context.ok: # Everything below needs a reachable cluster. Reporting eight further failures that all mean # "no context" buries the one that matters. return checks + with style.activity("cluster_login", "checking the credential actually works"): + login = _login_check(binary) + record(login) + if not login.ok: + # Same reason, one level deeper. A context with an expired token passes every *local* check + # and fails every remote one, and the remote failures do not say "log in" — they say the + # namespace is unreadable and the objects are missing, which is nine lines of plausible + # fiction. Stopping here is the only way the user is told the one thing that is true. + return checks + try: target = resolve_namespace(namespace) except ClusterError as exc: @@ -155,17 +175,26 @@ def record(*new: Check) -> None: ) return checks - record(_namespace_check(binary, target)) - record(*_object_checks(binary, target, division)) - record(*_verb_checks(target, division)) - secret = _secret_check(binary, target) + with style.activity("namespace", f"looking up {target}"): + namespace_check = _namespace_check(binary, target) + record(namespace_check) + with style.activity("bundle", f"comparing each object against {target}"): + object_checks = _object_checks(binary, target, division) + record(*object_checks) + with style.activity("permissions", "posting an access review per verb the run needs"): + verb_checks = _verb_checks(target, division) + record(*verb_checks) + with style.activity("credentials_secret", f"reading secret/{SECRET_NAME}"): + secret = secret_check(binary, target) record(secret) record(_image_check()) if probe_inference: record(_inference_result(binary, target, secret, announce=on_check is not None)) record(_gitleaks_check()) if division: - record(*_division_checks(target)) + with style.activity("build_api", "asking the cluster which APIs it serves"): + division_checks = _division_checks(target) + record(*division_checks) return checks @@ -192,6 +221,36 @@ def _context_check(binary: str) -> Check: ) +def _login_check(binary: str) -> Check: + """Is the selected context's credential still valid? One authenticated round trip. + + Separate from `_context_check` because the two answer different questions and only one of them + used to be asked. A kubeconfig entry is a local file; a *session* is a token with an expiry, and + `oc login` issues one that lasts about a day. Between those two facts sits the state this + exists for: everything local reports fine, every cluster read fails, and none of the failures + say "log in" — `get namespace` becomes "could not confirm", `get serviceaccount` becomes "not in + this namespace". A namespace that was fully prepared reads as an empty one. + """ + ok, detail = login_status(binary) + if ok: + return Check( + name="cluster_login", + ok=True, + detail=f"authenticated{f' as {detail}' if detail else ''}", + ) + return Check( + name="cluster_login", + ok=False, + detail=( + f"the selected context has no working credential: {detail}" + if not is_auth_error(detail) + else "your session for this context has expired — every cluster read would fail, and " + "the failures look like missing objects rather than like a login problem" + ), + fix=f"{binary} login --web # or `{binary} login --token=...`", + ) + + def _inference_result(binary: str, namespace: str, secret: Check, *, announce: bool) -> Check: """The in-cluster probe, or the reason it was not worth running.""" if not secret.ok: @@ -210,12 +269,14 @@ def _inference_result(binary: str, namespace: str, secret: Check, *, announce: b ) if announce: # Announced rather than merely slow: this one creates a pod and waits on it, and - # "nothing on screen for three minutes" is the report people read as a crash. + # "nothing on screen for three minutes" is the report people read as a crash. The status + # line below then keeps saying what the pod is doing; this says what is about to happen. print(style.note( "Checking inference from inside the namespace — this launches a short-lived pod " "and waits for it, up to three minutes." )) - return _inference_check(binary, namespace, resolve_image()) + with style.activity("inference_from_cluster", "creating the probe pod") as act: + return _inference_check(binary, namespace, resolve_image(), act=act) def _namespace_check(binary: str, namespace: str) -> Check: @@ -239,19 +300,32 @@ def _object_checks(binary: str, namespace: str, division: bool) -> list[Check]: objects while `setup` applies five, and the missing one is only found by a run that fails. """ checks = [] + apply_fix = ( + f"factory contained --namespace {namespace}" + f"{' --division' if division else ''} bundle | {binary} apply -f -" + ) for obj in bundle_objects(namespace=namespace, division=division): kind, name = obj.kind, obj.name result = _run(cli(binary, "get", kind, name, "-n", namespace, "-o", "name")) ok = result is not None and result.returncode == 0 + # A failed read is not evidence of absence. Only `NotFound` says the object is missing; + # anything else means the question went unanswered, and reporting that as "is missing" + # sends the user to apply a bundle over objects that are already there. + stderr = (result.stderr or "").strip() if result is not None else "" + answered = ok or is_not_found(stderr) checks.append( Check( name=f"bundle:{kind}/{name}", ok=ok, - detail=f"{kind}/{name} present" if ok else f"{kind}/{name} is missing", + detail=( + f"{kind}/{name} present" if ok + else f"{kind}/{name} is missing" if answered + else f"{kind}/{name} could not be checked: " + f"{stderr.splitlines()[0][:120] if stderr else 'the cluster did not answer'}" + ), fix=( - None if ok else - f"factory contained --namespace {namespace}" - f"{' --division' if division else ''} bundle | {binary} apply -f -" + None if ok else apply_fix if answered else + f"{binary} login --web # the read failed, so this is not known to be missing" ), ) ) @@ -341,57 +415,14 @@ def _no_exec_check(namespace: str) -> Check: ) -def _secret_check(binary: str, namespace: str) -> Check: - """The Secret must exist and carry a usable backend's keys — its *keys*, never its values.""" - result = _run(cli(binary, "get", "secret", SECRET_NAME, "-n", namespace, - "-o", "jsonpath={.data}")) - create_line = ( - f"{binary} create secret generic {SECRET_NAME} -n {namespace} \\\n" - f" --from-literal=ANTHROPIC_API_KEY=...\n" - f" or, for Vertex:\n" - f" {binary} create secret generic {SECRET_NAME} -n {namespace} \\\n" - f" --from-literal=CLAUDE_CODE_USE_VERTEX=1 \\\n" - f" --from-literal=CLOUD_ML_REGION= \\\n" - f" --from-literal=ANTHROPIC_VERTEX_PROJECT_ID= \\\n" - f" --from-file={ADC_SECRET_KEY}=$HOME/.config/gcloud/" - f"application_default_credentials.json" - ) - if result is None or result.returncode != 0: - return Check( - name="credentials_secret", - ok=False, - detail=f"secret/{SECRET_NAME} is missing from {namespace}", - fix=create_line, - ) - keys = _keys_of(result.stdout) - if set(ANTHROPIC_KEYS) <= keys: - return Check(name="credentials_secret", ok=True, - detail=f"secret/{SECRET_NAME} carries the Anthropic API key") - if set(VERTEX_KEYS) <= keys: - return Check(name="credentials_secret", ok=True, - detail=f"secret/{SECRET_NAME} carries the Vertex configuration") - return Check( - name="credentials_secret", - ok=False, - detail=( - f"secret/{SECRET_NAME} exists but carries none of the supported backends' keys " - f"(has: {', '.join(sorted(keys)) or 'nothing'})" - ), - fix=create_line, - ) - - -def _keys_of(raw: str) -> set[str]: - import json +# The probe's own ceiling, once it is actually running. A pod that cannot start never reaches it — +# `poll_pod` returns the moment the kubelet says so — so this now bounds only the request itself. +PROBE_TIMEOUT_SECONDS = 180 - try: - data = json.loads(raw or "{}") - except json.JSONDecodeError: - return set() - return set(data) if isinstance(data, dict) else set() - -def _inference_check(binary: str, namespace: str, image: str) -> Check: +def _inference_check( + binary: str, namespace: str, image: str, act: style.Activity | None = None +) -> Check: """Can a pod in this namespace actually reach inference? (spec.0 check 6) **From inside the cluster, not from here.** A host-side check proves nothing about the pod's @@ -403,15 +434,25 @@ def _inference_check(binary: str, namespace: str, image: str) -> Check: the design makes deliberately: a credentials problem found here fails at launch with a named cause, and found any other way it fails inside an agent call, minutes in, looking like a model outage. + + `act` is the status line. Every wait below reports into it, because this check was reported as a + hang: it is the only one that can legitimately take minutes, and it used to say nothing at all + while doing so. """ + def say(detail: str) -> None: + if act is not None: + act.update(detail) + # A hash rather than a slice of the namespace: a truncated name can end in a hyphen, which # RFC 1123 rejects and which the API server reports as an invalid *value* rather than as a # naming mistake. Hashing also keeps two namespaces' probes from colliding. pod = f"factory-inference-probe-{hashlib.sha1(namespace.encode()).hexdigest()[:8]}" manifest = _probe_pod_manifest(pod, namespace, image) try: + say("removing any probe pod left behind by an earlier run") subprocess.run(cli(binary, "delete", "pod", pod, "-n", namespace, "--ignore-not-found"), capture_output=True, text=True, timeout=60) + say("creating the probe pod") created = subprocess.run(cli(binary, "apply", "-n", namespace, "-f", "-"), input=manifest, capture_output=True, text=True, timeout=60) if created.returncode != 0: @@ -421,23 +462,26 @@ def _inference_check(binary: str, namespace: str, image: str) -> Check: detail=f"the probe pod could not be created: {created.stderr.strip()[:160]}", fix=f"factory contained --namespace {namespace} bundle | {binary} apply -f -", ) - waited = subprocess.run( - cli(binary, "wait", f"pod/{pod}", "-n", namespace, - "--for=jsonpath={.status.phase}=Succeeded", "--timeout=180s"), - capture_output=True, text=True, timeout=240, + # Polled rather than `oc wait --for=...Succeeded --timeout=180s`. That flag is blind: it can + # only report that the condition did not hold, so an image the cluster cannot pull cost the + # full three minutes and was then described as "the probe produced no output" — a sentence + # that names neither the cause nor where to look for it. + progress = poll_pod( + pod, namespace, until=(SUCCEEDED,), timeout=PROBE_TIMEOUT_SECONDS, + on_progress=lambda state: say(f"probe pod: {state.describe()}"), ) + say("reading the probe pod's output") logs = subprocess.run(cli(binary, "logs", pod, "-n", namespace), capture_output=True, text=True, timeout=60) output = (logs.stdout or "").strip() - ok = waited.returncode == 0 and "PROBE_OK" in output + ok = progress.verdict == SUCCEEDED and "PROBE_OK" in output return Check( name="inference_from_cluster", ok=ok, detail=( - "a pod in this namespace reached the configured inference backend" - if ok - else "a pod in this namespace could NOT reach inference: " - + (output.splitlines()[-1][:200] if output else "the probe produced no output") + "a pod in this namespace reached the configured inference backend" if ok + else f"a pod in this namespace could NOT reach inference: " + f"{_probe_failure(progress, output)}" ), fix=( None if ok else @@ -457,6 +501,20 @@ def _inference_check(binary: str, namespace: str, image: str) -> Check: "--wait=false"), capture_output=True, text=True, timeout=60) +def _probe_failure(progress: PodProgress, output: str) -> str: + """Why the probe failed, preferring whichever source actually knows. + + A pod that never started has no logs, and reporting "no output" for it describes the symptom of + the previous sentence rather than the cause. The kubelet's reason is the answer in that case; + the probe's own last line is the answer once it has run. + """ + if progress.verdict == DOOMED and not output: + return f"the probe pod could not run: {progress.describe()}" + if output: + return output.splitlines()[-1][:200] + return f"the probe produced no output ({progress.describe()})" + + def _probe_pod_manifest(name: str, namespace: str, image: str) -> str: """One pod, one request, no workspace, no PVC — it must not depend on anything under test. @@ -600,11 +658,17 @@ def setup_k8s( # Say the outcome before printing 80 lines of YAML that would otherwise bury it — and check the # blocker the user actually has. With no cluster reachable, nothing could be applied whatever # they answer, and "About to apply..." would be untrue. - reachable = _run(cli(binary, "config", "current-context")) - if reachable is None or reachable.returncode != 0 or not reachable.stdout.strip(): + # + # This asks the cluster, not the kubeconfig. It used to run `config current-context`, which + # reads a local file and answers "yes, a context is selected" for a context whose token expired + # hours ago — so the gate passed, the walk ran, and every object in an already-prepared + # namespace was offered for creation because every `get` had failed. + authenticated, why = login_status(binary) + if not authenticated: print( - f"No cluster is selected, so nothing can be applied to namespace {target} from here.\n" - f"Log in first (`{binary} login ...`), then re-run. The manifest you will need is " + f"This context has no working credential, so nothing can be applied to namespace " + f"{target} from here: {why}\n" + f"Log in first (`{binary} login --web`), then re-run. The manifest you will need is " "below; you can also hand it to whoever owns the namespace:\n" f"{apply_line}\n", file=sys.stderr, @@ -617,7 +681,12 @@ def setup_k8s( # decide is, per object that is not already right, what it is for and what would change. print(style.section("Review and apply", step=2, total=_K8S_STEPS)) objects = bundle_objects(namespace=target, division=division) - states = inspect_objects(objects, target, binary) + # One server-side `oc diff` per object, so this grows with the bundle and is a round trip each. + with style.activity("review", f"comparing {len(objects)} objects against {target}") as act: + states = inspect_objects( + objects, target, binary, + on_object=lambda obj: act.update(f"comparing {obj.kind}/{obj.name}"), + ) print(render_summary(states, target, cluster_context().server)) # There is no separate apply step: each object is applied the moment it is accepted. Batching @@ -658,7 +727,7 @@ def setup_k8s( )) return 1 - return _finish(binary, target, division, interactive) + return _finish(binary, target, division, interactive, assume_yes) def _apply_object(obj: BundleObject, namespace: str, binary: str) -> tuple[bool, str]: @@ -680,17 +749,20 @@ def _apply_object(obj: BundleObject, namespace: str, binary: str) -> tuple[bool, return False, detail[0][:200] if detail else "no detail given" -def _finish(binary: str, target: str, division: bool, interactive: bool = False) -> int: - """The Secret reminder and the verify pass — reached whether or not anything was applied. +def _finish( + binary: str, target: str, division: bool, interactive: bool = False, assume_yes: bool = False +) -> int: + """The credentials step and the verify pass — reached whether or not anything was applied. A run where every object was already correct still has to end in `verify`'s two states, because "nothing to apply" is not the same claim as "this namespace is ready". + + Credentials come before verify rather than after it for one reason: without them the inference + probe cannot run, so a setup that ends without asking always ends one check short of an answer. """ - print( - f"\nThe credentials Secret is yours to create — the factory never handles the material:\n" - f" {binary} create secret generic {SECRET_NAME} -n {target} " - "--from-literal=ANTHROPIC_API_KEY=...\n" - ) + print(style.section("Credentials", step=3, total=_K8S_STEPS)) + run_credentials_step(binary, target, interactive=interactive, assume_yes=assume_yes) + print(style.section("Verify", step=_K8S_STEPS, total=_K8S_STEPS)) # Streamed, not collected: the access reviews and the in-cluster inference probe take minutes # between them, and a step that prints nothing until they all finish is read as a hang — which diff --git a/tests/test_contained_k8s.py b/tests/test_contained_k8s.py index 3fe1f2cba..e1336c1c2 100644 --- a/tests/test_contained_k8s.py +++ b/tests/test_contained_k8s.py @@ -15,7 +15,7 @@ from factory.cli import contained as cli from factory.cli.contained_k8s import PACK_EXCLUDES, _build_pod_plan, _pack -from factory.contained import k8s, k8s_setup, secrets +from factory.contained import k8s, k8s_credentials, k8s_setup, secrets from factory.contained.bundle import SCC_ROLEBINDING, render_bundle from factory.contained.k8s import ( FACTORY_CONTAINER, @@ -74,7 +74,13 @@ def _no_real_kubeconfig(): patch("factory.contained.k8s_setup.current_namespace", return_value=None), \ patch("factory.contained.k8s_setup._namespace_status", return_value=k8s_setup.PRESENT), \ patch("factory.contained.k8s._run", return_value=_completed("true")), \ + patch("factory.contained.k8s_credentials._run", return_value=_completed("{}")), \ + patch("factory.contained.k8s_setup.run_credentials_step", return_value=False), \ patch("factory.contained.k8s_setup.access_review", return_value=True): + # The credentials step is stubbed for the same two reasons as the rest: it reads the + # cluster, and it is a conversation. A `setup_k8s` test that let it run would consume the + # mocked `input()` the object walk is asserting on, and would block on a prompt that never + # receives a valid key. `tests/test_contained_k8s_credentials.py` exercises it directly. # `access_review` is stubbed under the name *k8s_setup* imported, not on `k8s` itself: it # shells out with `subprocess.run` directly, so nothing else here catches it, and the test # that exercises the real function reaches it through `k8s.access_review`, untouched. @@ -394,7 +400,11 @@ def fake_run(argv, **kwargs): if "current-context" in argv: return _completed("ctx") if "rolebinding" in argv and SCC_ROLEBINDING in argv: - return _completed(returncode=1) + # `NotFound` specifically: any non-zero used to count as missing, which turned an + # expired login into a namespace that appeared to hold nothing. + missing = _completed(returncode=1) + missing.stderr = f'Error from server (NotFound): rolebindings "{SCC_ROLEBINDING}" not found' + return missing return _completed("ok") with patch("factory.contained.k8s_setup.cli_binary", return_value="oc"), \ @@ -463,8 +473,8 @@ def test_pods_exec_being_granted_is_itself_a_failure() -> None: def test_a_secret_with_the_wrong_keys_is_reported_by_key_never_by_value() -> None: payload = json.dumps({"SOME_OTHER_KEY": "c2VjcmV0"}) - with patch("factory.contained.k8s_setup._run", return_value=_completed(payload)): - check = k8s_setup._secret_check("oc", "ns") + with patch("factory.contained.k8s_credentials._run", return_value=_completed(payload)): + check = k8s_credentials.secret_check("oc", "ns") assert not check.ok assert "SOME_OTHER_KEY" in check.detail assert "c2VjcmV0" not in check.detail @@ -473,8 +483,8 @@ def test_a_secret_with_the_wrong_keys_is_reported_by_key_never_by_value() -> Non def test_a_vertex_secret_is_accepted() -> None: payload = json.dumps({k: "x" for k in k8s_setup.VERTEX_KEYS}) - with patch("factory.contained.k8s_setup._run", return_value=_completed(payload)): - assert k8s_setup._secret_check("oc", "ns").ok + with patch("factory.contained.k8s_credentials._run", return_value=_completed(payload)): + assert k8s_credentials.secret_check("oc", "ns").ok def test_setup_reports_the_current_state_before_asking( @@ -755,12 +765,12 @@ def test_vertex_configuration_without_a_credential_is_not_enough() -> None: k: "x" for k in ("CLAUDE_CODE_USE_VERTEX", "CLOUD_ML_REGION", "ANTHROPIC_VERTEX_PROJECT_ID") }) - with patch("factory.contained.k8s_setup._run", return_value=_completed(config_only)): - assert not k8s_setup._secret_check("oc", "ns").ok + with patch("factory.contained.k8s_credentials._run", return_value=_completed(config_only)): + assert not k8s_credentials.secret_check("oc", "ns").ok # With the credential file, it passes. complete = json.dumps({k: "x" for k in k8s_setup.VERTEX_KEYS}) - with patch("factory.contained.k8s_setup._run", return_value=_completed(complete)): - assert k8s_setup._secret_check("oc", "ns").ok + with patch("factory.contained.k8s_credentials._run", return_value=_completed(complete)): + assert k8s_credentials.secret_check("oc", "ns").ok def test_secret_keys_reads_names_and_never_values() -> None: @@ -804,7 +814,7 @@ def test_the_inference_probe_is_skipped_when_the_secret_is_missing() -> None: """The probe pod mounts that Secret; without it the wait is 180s to learn what we know.""" with patch("factory.contained.k8s_setup.cli_binary", return_value="oc"), \ patch("factory.contained.k8s_setup._run", return_value=_completed("ctx")), \ - patch("factory.contained.k8s_setup._secret_check", + patch("factory.contained.k8s_setup.secret_check", return_value=Check("credentials_secret", False, "missing", fix="oc create secret")), \ patch("factory.contained.k8s_setup._inference_check") as probe: checks = k8s_setup.verify_k8s(namespace="ns") @@ -818,7 +828,7 @@ def test_the_inference_probe_is_skipped_when_the_secret_is_missing() -> None: def test_the_inference_probe_still_runs_when_the_secret_is_there() -> None: with patch("factory.contained.k8s_setup.cli_binary", return_value="oc"), \ patch("factory.contained.k8s_setup._run", return_value=_completed("ctx")), \ - patch("factory.contained.k8s_setup._secret_check", + patch("factory.contained.k8s_setup.secret_check", return_value=Check("credentials_secret", True, "present")), \ patch("factory.contained.k8s_setup._inference_check", return_value=Check("inference_from_cluster", True, "reached")) as probe: @@ -968,16 +978,23 @@ def test_setup_applies_nothing_without_confirmation(capsys: pytest.CaptureFixtur def test_setup_says_so_when_no_cluster_is_selected(capsys: pytest.CaptureFixture[str]) -> None: - """"About to apply ... with your own credentials" is untrue when there are none.""" + """"About to apply ... with your own credentials" is untrue when there are none. + + The gate asks the *cluster*, not the kubeconfig: `config current-context` reads a local file + and passes happily for a context whose token expired hours ago, which is precisely the state + where every apply below it would fail. + """ with patch("factory.contained.k8s_setup.cli_binary", return_value="oc"), \ patch("factory.contained.k8s_setup.resolve_namespace", return_value="ns"), \ patch("factory.contained.k8s_setup._run", return_value=_completed("", returncode=1)), \ + patch("factory.contained.k8s_setup.login_status", + return_value=(False, "You must be logged in to the server (Unauthorized)")), \ patch("factory.contained.k8s_setup.subprocess.run") as run: code = k8s_setup.setup_k8s(namespace="ns", division=False, interactive=True, assume_yes=True) run.assert_not_called() assert code == 1 - assert "No cluster is selected" in capsys.readouterr().err + assert "no working credential" in capsys.readouterr().err def test_setup_degrades_to_printing_when_apply_is_refused( diff --git a/tests/test_contained_k8s_credentials.py b/tests/test_contained_k8s_credentials.py new file mode 100644 index 000000000..4cea1b6a5 --- /dev/null +++ b/tests/test_contained_k8s_credentials.py @@ -0,0 +1,290 @@ +"""The guided credentials Secret: what it composes, what it refuses, and what it never discloses. + +The disclosure tests are the ones worth having. A wizard that collects an API key has exactly one +way to be dangerous, and it is not "the wrong key ends up in the Secret" — it is the key ending up +somewhere nobody was looking: an argv, a log line, a printed command, an error message. +""" + +from __future__ import annotations + +import json +import subprocess +from pathlib import Path +from unittest.mock import patch + +import pytest + +from factory.contained import k8s_credentials as creds +from factory.contained.k8s import ADC_SECRET_KEY, SECRET_NAME + +SECRET = "sk-ant-api03-averylongsecretvaluethatmustnotleak-9f2c" + + +def _completed(stdout: str = "", returncode: int = 0, stderr: str = ""): + return subprocess.CompletedProcess([], returncode, stdout, stderr) + + +class _Answers: + """A scripted stand-in for the three readers, so the flow can run without a terminal. + + `tests/conftest.py` forces raw reads off and pytest's stdin raises on `input()`, so a prompt + reached for real here would either block or abort. Injection is the only way in. + """ + + def __init__(self, *, selects: list[str], lines: list[str] | None = None, + secrets: list[str] | None = None) -> None: + self.selects = list(selects) + self.lines = list(lines or []) + self.secrets = list(secrets or []) + + def readers(self) -> creds._Readers: + return creds._Readers( + line=lambda question, default=None: self.lines.pop(0) if self.lines else default, + secret=lambda question: self.secrets.pop(0) if self.secrets else None, + select=lambda question, options: self.selects.pop(0) if self.selects else None, + ) + + +# -------------------------------------------------------------------------------------------- +# The manifest +# -------------------------------------------------------------------------------------------- + + +def test_the_manifest_is_json_so_no_value_can_break_it() -> None: + """A key with a colon, a quote or a newline in it is ordinary here and fatal to hand-built YAML.""" + hostile = 'a: "b"\nc: {d}\n%e\n\t--- ' + manifest = creds.build_secret_manifest("ns", {"ANTHROPIC_API_KEY": hostile}) + parsed = json.loads(manifest) + assert parsed["stringData"]["ANTHROPIC_API_KEY"] == hostile + assert parsed["kind"] == "Secret" + assert parsed["metadata"]["name"] == SECRET_NAME + assert parsed["metadata"]["namespace"] == "ns" + + +def test_the_manifest_uses_string_data_so_nothing_has_to_base64_by_hand() -> None: + parsed = json.loads(creds.build_secret_manifest("ns", {"ANTHROPIC_API_KEY": SECRET})) + assert "data" not in parsed + assert parsed["stringData"]["ANTHROPIC_API_KEY"] == SECRET + + +def test_the_material_never_reaches_an_argv() -> None: + """`--from-literal` puts a key in the process table and in the caller's shell history.""" + seen: dict[str, object] = {} + + def fake_run(argv, **kwargs): + seen["argv"] = argv + seen["stdin"] = kwargs.get("input", "") + return _completed("secret/factory-credentials created") + + with patch("factory.contained.k8s_credentials.subprocess.run", side_effect=fake_run): + ok, detail = creds.apply_secret("oc", "ns", {"ANTHROPIC_API_KEY": SECRET}) + + assert ok + assert SECRET not in " ".join(seen["argv"]) # type: ignore[arg-type] + assert not any("from-literal" in token for token in seen["argv"]) # type: ignore[union-attr] + assert SECRET in seen["stdin"] # type: ignore[operator] + + +def test_the_echoed_command_is_redacted() -> None: + line = creds.redacted_command("oc", "ns", {"ANTHROPIC_API_KEY": SECRET}) + assert SECRET not in line + assert "***" in line + + +def test_configuration_stays_readable_while_material_is_hidden() -> None: + """A user confirms they configured the right region; hiding that helps nobody.""" + line = creds.redacted_command("oc", "ns", { + "CLOUD_ML_REGION": "us-east5", + ADC_SECRET_KEY: '{"type": "authorized_user"}', + }) + assert "us-east5" in line + assert "authorized_user" not in line + + +def test_a_failure_message_cannot_carry_the_value_back_out() -> None: + """A parser quoting the input it choked on is exactly how a key reaches a terminal.""" + with patch("factory.contained.k8s_credentials.subprocess.run", + return_value=_completed(returncode=1, stderr=f"error validating {SECRET}")): + ok, detail = creds.apply_secret("oc", "ns", {"ANTHROPIC_API_KEY": SECRET}) + assert not ok + assert SECRET not in detail + assert "***" in detail + + +def test_nothing_but_names_and_lengths_is_logged() -> None: + recorded: list[dict] = [] + with patch("factory.contained.k8s_credentials.subprocess.run", + return_value=_completed("created")), \ + patch.object(creds.log, "info", lambda event, **kw: recorded.append(kw)): + creds.apply_secret("oc", "ns", {"ANTHROPIC_API_KEY": SECRET}) + assert recorded + assert SECRET not in json.dumps(recorded) + assert recorded[0]["keys"] == {"ANTHROPIC_API_KEY": len(SECRET)} + + +# -------------------------------------------------------------------------------------------- +# Describing a value without disclosing it +# -------------------------------------------------------------------------------------------- + + +def test_a_value_is_described_by_shape_not_by_content() -> None: + described = creds.describe_value(SECRET) + assert str(len(SECRET)) in described + assert SECRET not in described + # Enough to catch a paste that grabbed the quotes, and no more. + assert "sk-ant-a" in described + + +def test_a_short_value_gets_no_excerpt_at_all() -> None: + """An excerpt of a twelve-character secret is most of the secret.""" + described = creds.describe_value("short-key-1") + assert described == "11 characters" + + +# -------------------------------------------------------------------------------------------- +# The ADC file +# -------------------------------------------------------------------------------------------- + + +def test_an_authorized_user_document_is_accepted() -> None: + assert creds.validate_adc(json.dumps({ + "type": "authorized_user", "client_id": "a", "client_secret": "b", "refresh_token": "c", + })) is None + + +def test_a_service_account_key_is_accepted_too() -> None: + assert creds.validate_adc(json.dumps({ + "type": "service_account", "project_id": "p", "private_key": "k", "client_email": "e", + })) is None + + +def test_a_document_missing_a_required_field_names_the_field() -> None: + problem = creds.validate_adc(json.dumps({"type": "authorized_user", "client_id": "a"})) + assert problem is not None + assert "client_secret" in problem and "refresh_token" in problem + + +def test_a_non_json_file_is_refused_before_it_is_uploaded() -> None: + """Otherwise the failure surfaces inside an agent call and reads as a model outage.""" + problem = creds.validate_adc("not json at all") + assert problem is not None and "not valid JSON" in problem + + +def test_a_json_document_of_an_unknown_type_is_refused() -> None: + problem = creds.validate_adc(json.dumps({"type": "something_else"})) + assert problem is not None and "authorized_user" in problem + + +# -------------------------------------------------------------------------------------------- +# The step +# -------------------------------------------------------------------------------------------- + + +def test_an_existing_usable_secret_asks_nothing(capsys: pytest.CaptureFixture[str]) -> None: + payload = json.dumps({"ANTHROPIC_API_KEY": "x"}) + with patch("factory.contained.k8s_credentials._run", return_value=_completed(payload)): + # No readers supplied: reaching a prompt at all would raise here. + assert creds.run_credentials_step("oc", "ns", interactive=True) is True + assert "Nothing to do" in capsys.readouterr().out + + +def test_nobody_at_the_keyboard_means_no_credential_is_chosen( + capsys: pytest.CaptureFixture[str], +) -> None: + """`--yes` means "do not stop to ask me", not "pick something for me".""" + with patch("factory.contained.k8s_credentials._run", return_value=_completed(returncode=1)): + created = creds.run_credentials_step( + "oc", "ns", interactive=False, assume_yes=True, readers=creds._Readers() + ) + assert created is False + printed = capsys.readouterr().out + assert "skipped" in printed + assert "oc create secret generic" in printed + + +def test_the_anthropic_path_types_a_key_and_applies_it() -> None: + answers = _Answers(selects=["1", "t"], secrets=[SECRET]) + applied: dict[str, dict[str, str]] = {} + + def fake_apply(binary, namespace, data): + applied["data"] = data + return True, "secret/factory-credentials created" + + with patch("factory.contained.k8s_credentials._run", return_value=_completed(returncode=1)), \ + patch("factory.contained.k8s_credentials.apply_secret", side_effect=fake_apply), \ + patch("factory.contained.k8s_credentials.secret_exists", return_value=False), \ + patch("factory.contained.k8s_credentials.style.confirm", return_value=True), \ + patch("factory.contained.k8s_credentials.secret_check") as check: + check.side_effect = [ + creds.Check("credentials_secret", False, "missing", fix="..."), + creds.Check("credentials_secret", True, "present"), + ] + assert creds.run_credentials_step( + "oc", "ns", interactive=True, readers=answers.readers() + ) is True + assert applied["data"] == {"ANTHROPIC_API_KEY": SECRET} + + +def test_a_key_can_come_from_an_environment_variable_by_name(monkeypatch) -> None: + monkeypatch.setenv("MY_OWN_KEY", SECRET) + answers = _Answers(selects=["1", "e"], lines=["MY_OWN_KEY"]) + with patch("factory.contained.k8s_credentials.style.confirm", return_value=True): + data = creds._choose_backend(answers.readers()) + assert data == {"ANTHROPIC_API_KEY": SECRET} + + +def test_an_unset_environment_variable_reads_nothing_and_asks_again(monkeypatch) -> None: + """Naming the wrong variable is a slip, not a decision to stop.""" + monkeypatch.delenv("NOT_SET_ANYWHERE", raising=False) + monkeypatch.setenv("THE_RIGHT_ONE", SECRET) + answers = _Answers(selects=["1", "e", "e"], lines=["NOT_SET_ANYWHERE", "THE_RIGHT_ONE"]) + with patch("factory.contained.k8s_credentials.style.confirm", return_value=True): + data = creds._choose_backend(answers.readers()) + assert data == {"ANTHROPIC_API_KEY": SECRET} + + +def test_the_vertex_path_reads_the_credential_from_a_file(tmp_path: Path, monkeypatch) -> None: + adc = tmp_path / "adc.json" + document = json.dumps({ + "type": "authorized_user", "client_id": "a", "client_secret": "b", "refresh_token": "c", + }) + adc.write_text(document) + monkeypatch.delenv("CLOUD_ML_REGION", raising=False) + monkeypatch.delenv("ANTHROPIC_VERTEX_PROJECT_ID", raising=False) + answers = _Answers(selects=["2", "f"], lines=["us-east5", "my-project", str(adc)]) + with patch("factory.contained.k8s_credentials.style.confirm", return_value=True): + data = creds._choose_backend(answers.readers()) + assert data is not None + assert data["CLAUDE_CODE_USE_VERTEX"] == "1" + assert data["CLOUD_ML_REGION"] == "us-east5" + assert data["ANTHROPIC_VERTEX_PROJECT_ID"] == "my-project" + assert data[ADC_SECRET_KEY] == document + # The pinned setting the local target applies too; without it the cluster run behaves + # differently from the same run on this machine. + assert data["MAX_THINKING_TOKENS"] == "0" + + +def test_a_vertex_secret_built_here_satisfies_the_check_that_reads_it() -> None: + """The two halves must agree: a Secret this composes has to be one `secret_check` accepts.""" + monkey = { + "CLAUDE_CODE_USE_VERTEX": "1", "CLOUD_ML_REGION": "us-east5", + "ANTHROPIC_VERTEX_PROJECT_ID": "p", ADC_SECRET_KEY: "{}", + } + payload = json.dumps({key: "x" for key in monkey}) + with patch("factory.contained.k8s_credentials._run", return_value=_completed(payload)): + assert creds.secret_check("oc", "ns").ok + + +def test_backing_out_creates_nothing_and_prints_the_manual_route( + capsys: pytest.CaptureFixture[str], +) -> None: + answers = _Answers(selects=["s"]) + with patch("factory.contained.k8s_credentials._run", return_value=_completed(returncode=1)), \ + patch("factory.contained.k8s_credentials.secret_exists", return_value=False), \ + patch("factory.contained.k8s_credentials.apply_secret") as apply: + created = creds.run_credentials_step( + "oc", "ns", interactive=True, readers=answers.readers() + ) + assert created is False + apply.assert_not_called() + assert "oc create secret generic" in capsys.readouterr().out diff --git a/tests/test_contained_k8s_division.py b/tests/test_contained_k8s_division.py index 809e4b858..15043d46a 100644 --- a/tests/test_contained_k8s_division.py +++ b/tests/test_contained_k8s_division.py @@ -32,11 +32,17 @@ def _completed(stdout: str = "", returncode: int = 0) -> subprocess.CompletedPro def _no_cluster_round_trip(): """Building a pod plan must not phone a cluster. - `_build_pod_plan` reads the namespace's allocated `fsGroup` range, which is a live `oc get - namespace`. On a machine logged in to a slow or unreachable cluster that is a 30-second timeout - per test — the difference between this file taking one second and taking two minutes. + `_build_pod_plan` makes *two* live reads, and both have to be stubbed. The namespace's allocated + `fsGroup` range is one; `secret_keys`, which decides whether a Google credential file is + mounted, is the other. Whichever cluster the developer's kubeconfig happens to point at answers + them — and when that cluster is unreachable each call waits out its full timeout, which is the + difference between this file taking one second and hanging for minutes. + + Stubbing both is also what keeps a test run off whatever context is current. A test suite has no + business reaching a cluster at all, least of all one chosen by accident. """ - with patch("factory.cli.contained_k8s.namespace_fs_group", return_value=None): + with patch("factory.cli.contained_k8s.namespace_fs_group", return_value=None), \ + patch("factory.cli.contained_k8s.secret_keys", return_value=set()): yield diff --git a/tests/test_contained_k8s_helpers.py b/tests/test_contained_k8s_helpers.py index 57d3b0b47..79085cead 100644 --- a/tests/test_contained_k8s_helpers.py +++ b/tests/test_contained_k8s_helpers.py @@ -10,7 +10,9 @@ import subprocess from unittest.mock import patch -from factory.contained import k8s_review, style +import pytest + +from factory.contained import k8s, k8s_review, style from factory.contained.k8s_division import openshift_available @@ -96,3 +98,243 @@ def test_a_diff_that_cannot_be_run_is_reported_as_unknown_not_as_current() -> No ): state = k8s_review._inspect_one(obj, "ns", "oc") assert state.status == k8s_review.UNKNOWN + + +# -------------------------------------------------------------------------------------------- +# Reading a pod's state well enough to stop waiting on a hopeless one +# -------------------------------------------------------------------------------------------- + + +def _pod(*, phase: str = "Pending", waiting: dict | None = None, running: bool = False, + terminated: dict | None = None, conditions: list | None = None, + name: str = "probe") -> dict: + state: dict = {} + if waiting is not None: + state["waiting"] = waiting + if running: + state["running"] = {"startedAt": "now"} + if terminated is not None: + state["terminated"] = terminated + status: dict = {"phase": phase} + if state: + status["containerStatuses"] = [{"name": name, "state": state}] + if conditions is not None: + status["conditions"] = conditions + return {"status": status} + + +def test_an_unpullable_image_is_doomed_immediately_not_after_the_timeout() -> None: + """The defect this exists for: three minutes of silence, then "the probe produced no output". + + `ImagePullBackOff` is the kubelet saying it has already retried and given up. Waiting past it + buys nothing, and the message it carries is the answer the user actually needs. + """ + progress = k8s.classify_pod(_pod(waiting={ + "reason": "ImagePullBackOff", + "message": 'Back-off pulling image "ghcr.io/akashgit/remote-factory/factory-runtime"', + })) + assert progress.verdict == k8s.DOOMED + assert progress.reason == "ImagePullBackOff" + assert "Back-off pulling image" in progress.describe() + + +def test_a_secret_missing_a_key_is_doomed_immediately() -> None: + progress = k8s.classify_pod(_pod(waiting={ + "reason": "CreateContainerConfigError", "message": "secret 'factory-credentials' not found", + })) + assert progress.verdict == k8s.DOOMED + assert "factory-credentials" in progress.describe() + + +def test_a_first_pull_is_not_mistaken_for_a_failure() -> None: + """A cold `ContainerCreating` legitimately runs for minutes; capping it would break every + first run on a fresh node.""" + progress = k8s.classify_pod(_pod(waiting={"reason": "ContainerCreating", "message": ""})) + assert progress.verdict == k8s.WAITING + assert progress.reason == "ContainerCreating" + + +def test_a_retryable_pull_error_is_not_doomed_on_sight() -> None: + """`ErrImagePull` is the attempt; `ImagePullBackOff` is the verdict. Only the second is final.""" + progress = k8s.classify_pod(_pod(waiting={"reason": "ErrImagePull", "message": "timeout"})) + assert progress.verdict == k8s.WAITING + assert progress.reason in k8s.RETRYABLE_WAITING_REASONS + + +def test_a_pod_no_node_will_accept_is_doomed_with_the_schedulers_words() -> None: + """It sits in Pending with no container status at all, which reads as "starting".""" + progress = k8s.classify_pod(_pod(conditions=[{ + "type": "PodScheduled", "status": "False", "reason": "Unschedulable", + "message": "0/6 nodes are available: insufficient memory", + }])) + assert progress.verdict == k8s.DOOMED + assert "insufficient memory" in progress.describe() + + +def test_a_running_container_is_running_and_a_clean_exit_succeeded() -> None: + assert k8s.classify_pod(_pod(running=True)).verdict == k8s.RUNNING + done = k8s.classify_pod(_pod(phase="Succeeded", terminated={"exitCode": 0, + "reason": "Completed"})) + assert done.verdict == k8s.SUCCEEDED + + +def test_a_nonzero_exit_is_doomed_and_carries_its_code() -> None: + progress = k8s.classify_pod(_pod(phase="Failed", terminated={"exitCode": 7, "reason": "Error"})) + assert progress.verdict == k8s.DOOMED + assert "7" in progress.describe() + + +def test_one_container_can_be_asked_about_by_name() -> None: + """The loader's window is "that initContainer is running", which no pod condition expresses.""" + pod = {"status": {"phase": "Pending", "initContainerStatuses": [ + {"name": "workspace-loader", "state": {"running": {}}}, + ], "containerStatuses": [ + {"name": "factory", "state": {"waiting": {"reason": "PodInitializing"}}}, + ]}} + assert k8s.classify_pod(pod, container="workspace-loader").verdict == k8s.RUNNING + assert k8s.classify_pod(pod, container="factory").verdict == k8s.WAITING + + +def test_an_unrecognized_state_waits_rather_than_giving_up() -> None: + """Being wrong in this direction aborts a run over a state that would have cleared.""" + progress = k8s.classify_pod(_pod(waiting={"reason": "SomethingNewInKubernetes"})) + assert progress.verdict == k8s.WAITING + + +def test_an_empty_or_malformed_pod_never_raises() -> None: + for payload in ({}, {"status": None}, {"status": {"containerStatuses": None}}): + assert k8s.classify_pod(payload).verdict in (k8s.WAITING, k8s.DOOMED) + + +def test_polling_stops_the_moment_a_pod_is_doomed() -> None: + """Not after the timeout: the first poll already knew, and the user waited three minutes.""" + doomed = _pod(waiting={"reason": "ImagePullBackOff", "message": "no such image"}) + with patch("factory.contained.k8s.read_pod", return_value=doomed), \ + patch("factory.contained.k8s.time.sleep") as slept: + progress = k8s.poll_pod("probe", "ns", timeout=180) + assert progress.verdict == k8s.DOOMED + slept.assert_not_called() + + +def test_polling_reports_each_change_once() -> None: + states = [ + _pod(waiting={"reason": "ContainerCreating"}), + _pod(waiting={"reason": "ContainerCreating"}), + _pod(running=True), + ] + seen: list[str] = [] + with patch("factory.contained.k8s.read_pod", side_effect=states), \ + patch("factory.contained.k8s.time.sleep"): + k8s.poll_pod("probe", "ns", timeout=180, on_progress=lambda p: seen.append(p.reason)) + assert seen == ["ContainerCreating", "Running"] + + +def test_a_wait_that_times_out_says_what_it_was_still_waiting_for() -> None: + with patch("factory.contained.k8s.read_pod", + return_value=_pod(waiting={"reason": "ContainerCreating"})), \ + patch("factory.contained.k8s.time.sleep"): + progress = k8s.poll_pod("probe", "ns", timeout=0) + assert progress.verdict == k8s.DOOMED + assert progress.reason == "Timeout" + + +def test_wait_for_container_names_the_reason_rather_than_reporting_a_timeout() -> None: + """It used to spend its full five minutes and then blame the clock.""" + with patch("factory.contained.k8s.read_pod", return_value=_pod( + waiting={"reason": "ImagePullBackOff", "message": "manifest unknown"}, name="factory")), \ + patch("factory.contained.k8s.time.sleep"), \ + patch("factory.contained.k8s.cli_binary", return_value="oc"): + with pytest.raises(k8s.ClusterError) as raised: + k8s.wait_for_container("pod", "ns", "factory", timeout=300) + assert "ImagePullBackOff" in str(raised.value) + assert "manifest unknown" in str(raised.value) + + +# -------------------------------------------------------------------------------------------- +# A failed read is not evidence of absence +# -------------------------------------------------------------------------------------------- + +_UNAUTHORIZED = ( + 'error: You must be logged in to the server (Unauthorized)\n' + 'couldn\'t get current server API group list: the server has asked for the client to ' + 'provide credentials' +) + + +def _obj(): + from factory.contained.bundle import BundleObject + + return BundleObject(kind="serviceaccount", name="factory", + purpose="the identity the pod runs as", manifest="kind: ServiceAccount\n") + + +def test_an_expired_login_is_not_reported_as_a_missing_object() -> None: + """The defect: a fully prepared namespace read as an empty one. + + Every `oc get` failed with Unauthorized, every failure was classified as "not there", and the + review offered to create five objects that already existed — directly contradicting the honest + "could not confirm whether the namespace exists" printed one line above. + """ + with patch("factory.contained.k8s_review._run", + return_value=subprocess.CompletedProcess([], 1, "", _UNAUTHORIZED)): + state = k8s_review._inspect_one(_obj(), "factory-yi", "oc") + assert state.status == k8s_review.UNKNOWN + assert "not logged in" in state.detail + + +def test_a_genuine_notfound_is_still_absent() -> None: + """The distinction has to cut both ways, or a first setup stops offering to create anything.""" + stderr = 'Error from server (NotFound): serviceaccounts "factory" not found' + with patch("factory.contained.k8s_review._run", + return_value=subprocess.CompletedProcess([], 1, "", stderr)): + state = k8s_review._inspect_one(_obj(), "factory-yi", "oc") + assert state.status == k8s_review.ABSENT + + +def test_any_other_read_failure_is_unknown_and_carries_its_reason() -> None: + stderr = "Error from server (Forbidden): serviceaccounts is forbidden" + with patch("factory.contained.k8s_review._run", + return_value=subprocess.CompletedProcess([], 1, "", stderr)): + state = k8s_review._inspect_one(_obj(), "factory-yi", "oc") + assert state.status == k8s_review.UNKNOWN + assert "Forbidden" in state.detail + + +def test_an_auth_error_is_recognized_however_the_cli_words_it() -> None: + for text in ( + "error: You must be logged in to the server (Unauthorized)", + "the server has asked for the client to provide credentials", + "Unauthorized", + "invalid bearer token", + ): + assert k8s.is_auth_error(text), text + assert not k8s.is_auth_error('serviceaccounts "factory" not found') + + +def test_a_login_check_asks_the_cluster_not_the_kubeconfig() -> None: + """`config current-context` reads a local file and passes with an hours-dead token.""" + with patch("factory.contained.k8s._run", + return_value=subprocess.CompletedProcess([], 1, "", _UNAUTHORIZED)): + ok, detail = k8s.login_status("oc") + assert ok is False + assert "logged in" in detail.lower() or "credentials" in detail.lower() + + with patch("factory.contained.k8s._run", + return_value=subprocess.CompletedProcess([], 0, "yizheng@redhat.com\n", "")): + ok, detail = k8s.login_status("oc") + assert ok is True and detail == "yizheng@redhat.com" + + +def test_the_login_probe_is_an_authenticated_round_trip() -> None: + seen = {} + + def fake_run(argv, **kw): + seen["argv"] = argv + return subprocess.CompletedProcess([], 0, "you", "") + + with patch("factory.contained.k8s._run", side_effect=fake_run): + k8s.login_status("oc") + assert "whoami" in seen["argv"] + with patch("factory.contained.k8s._run", side_effect=fake_run): + k8s.login_status("kubectl") + assert "auth" in seen["argv"] and "can-i" in seen["argv"] diff --git a/tests/test_contained_k8s_review.py b/tests/test_contained_k8s_review.py index 0145c364c..e1bb54064 100644 --- a/tests/test_contained_k8s_review.py +++ b/tests/test_contained_k8s_review.py @@ -61,7 +61,14 @@ def test_every_object_explains_itself() -> None: def test_a_missing_object_is_absent_and_never_diffed() -> None: - with patch("factory.contained.k8s_review._run", return_value=_completed("", 1)) as run: + """`NotFound` on stderr is what makes it absent — not merely a non-zero exit. + + Any non-zero used to qualify, so an expired login turned a prepared namespace into an empty + one. The `get` has to actually say the object is not there. + """ + not_found = _completed("", 1) + not_found.stderr = 'Error from server (NotFound): serviceaccounts "factory" not found' + with patch("factory.contained.k8s_review._run", return_value=not_found) as run: states = inspect_objects([_obj()], "ns", "oc") assert states[0].status == ABSENT # One call: `get`. Diffing something that does not exist wastes a round trip per object. From 4c49e8fe93f0b800bfc4763b33d2e30559d5f9a8 Mon Sep 17 00:00:00 2001 From: Yi Zheng <237498169+beatsmonster@users.noreply.github.com> Date: Wed, 12 Aug 2026 12:35:08 -0400 Subject: [PATCH 5/6] test(contained): bring the contained package to 100% coverage MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Statements and branches, every module under `factory/contained/` plus the three `factory/cli/contained*` front-ends and `factory/podman.py`. The gaps were the paths unit tests usually skip: raw-terminal I/O, live-subprocess orchestration, and the failure/degradation arms of the cluster helpers. The one that needed care is `style.py`'s raw half. `conftest` forces `_raw_session` off so no test blocks on a keypress, which is exactly why the raw readers were uncovered. `test_contained_style_raw.py` restores the real function against a genuine pty, with two pitfalls handled: `sys.stdin` reads a byte at a time straight from the fd (a buffered wrapper would swallow the `[` of an arrow key and defeat the `select`-based escape drain), and `tty.setcbreak` is pinned to TCSANOW (its TCSAFLUSH default discards the keystrokes fed before the mode switch — an instant hang otherwise). Everything cluster-side is mocked at the module boundary: `classify_pod`/ `poll_pod` drive crafted `PodProgress` values, the inference probe's delete/apply/logs are keyed off argv, and the interactive setup walk is scripted through injected readers. No test reaches a real cluster or a real prompt. Two `# pragma: no cover`, each on a genuinely unreachable line: the non-POSIX `termios` ImportError in `_raw_session`, and a `Path("").expanduser()` guard that can never be empty (it stringifies to "."). No other source changed. Coverage note recorded for maintainers: the agent worktrees this was partly written in were based on origin/main, so a couple of the generated files first had to be re-verified against the branch's actual source — the k8s and k8s_setup suites here are the branch-correct versions. Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: Yi Zheng <237498169+beatsmonster@users.noreply.github.com> --- factory/contained/k8s_credentials.py | 2 +- factory/contained/style.py | 2 +- tests/test_contained_bundle_coverage.py | 18 + tests/test_contained_cli_coverage.py | 806 ++++++++++++++++++ tests/test_contained_k8s.py | 7 + tests/test_contained_k8s_coverage.py | 521 +++++++++++ ...test_contained_k8s_credentials_coverage.py | 390 +++++++++ tests/test_contained_k8s_helpers.py | 71 ++ tests/test_contained_k8s_setup_coverage.py | 607 +++++++++++++ tests/test_contained_style_raw.py | 433 ++++++++++ 10 files changed, 2855 insertions(+), 2 deletions(-) create mode 100644 tests/test_contained_bundle_coverage.py create mode 100644 tests/test_contained_cli_coverage.py create mode 100644 tests/test_contained_k8s_coverage.py create mode 100644 tests/test_contained_k8s_credentials_coverage.py create mode 100644 tests/test_contained_k8s_setup_coverage.py create mode 100644 tests/test_contained_style_raw.py diff --git a/factory/contained/k8s_credentials.py b/factory/contained/k8s_credentials.py index 70932cd29..a6359a0ac 100644 --- a/factory/contained/k8s_credentials.py +++ b/factory/contained/k8s_credentials.py @@ -403,7 +403,7 @@ def _from_file(field_spec: Field, readers: _Readers) -> str | None: if typed is None: return None path = Path((typed.strip() or default or "")).expanduser() - if not str(path): + if not str(path): # pragma: no cover - Path("").expanduser() is ".", so this never fires return None try: content = path.read_text() diff --git a/factory/contained/style.py b/factory/contained/style.py index 28b941bdc..7e402f3d0 100644 --- a/factory/contained/style.py +++ b/factory/contained/style.py @@ -352,7 +352,7 @@ def _raw_session(target: TextIO) -> tuple[int, Any] | None: return None try: import termios - except ImportError: # non-POSIX + except ImportError: # pragma: no cover - non-POSIX only; termios always imports on the test platforms return None try: descriptor = sys.stdin.fileno() diff --git a/tests/test_contained_bundle_coverage.py b/tests/test_contained_bundle_coverage.py new file mode 100644 index 000000000..44b55cd9f --- /dev/null +++ b/tests/test_contained_bundle_coverage.py @@ -0,0 +1,18 @@ +"""The one branch in `bundle` that only fires when the cluster is unreachable. + +`render_bundle` has to produce YAML with no cluster in reach — that is the whole point of `bundle`, +which a user hands to someone who owns a namespace they cannot touch. So the current-namespace +lookup swallows everything and falls back to None, and that swallow is the branch under test. +""" + +from __future__ import annotations + +from unittest.mock import patch + +from factory.contained import bundle + + +def test_current_namespace_lookup_swallows_every_failure() -> None: + """A broken or absent CLI must degrade to "no namespace known", never raise into rendering.""" + with patch("factory.contained.k8s.current_namespace", side_effect=RuntimeError("no cli")): + assert bundle._safe_current_namespace() is None diff --git a/tests/test_contained_cli_coverage.py b/tests/test_contained_cli_coverage.py new file mode 100644 index 000000000..ec13cddd9 --- /dev/null +++ b/tests/test_contained_cli_coverage.py @@ -0,0 +1,806 @@ +"""Coverage-completing tests for the `factory contained` CLI front door and the local runtime. + +The three modules under test are pure orchestration: `contained.py` reads one interpreted namespace +and hands it to a peer, `contained_args.py` reads the command line, and `contained_local.py` composes +and runs one podman container. None of them should ever launch podman during a test, so every seam — +podman helpers, workspace materialization, credential/identity probes, the division server — is +mocked. What is asserted is the routing and the composition, not the side effects. + +The existing `tests/test_contained.py` and `tests/test_contained_lifecycle.py` already cover the happy +paths reachable through dry-run; this file fills in the branches those cannot reach without either +launching podman or standing up a cluster. +""" + +from __future__ import annotations + +import argparse +import subprocess +from pathlib import Path +from types import SimpleNamespace +from unittest.mock import patch + +import pytest + +from factory.cli import contained as cli +from factory.cli import contained_args +from factory.cli import contained_local +from factory.contained.credentials import CredentialShape +from factory.contained.errors import ContainedError +from factory.contained.provenance import Probe +from factory.contained.workspace import Workspace, WorkspaceError +from factory.podman import CONTAINER_HOME, LABEL_CONTAINED, LABEL_PROJECT, ContainerPlan, Mount, Step + + +# -------------------------------------------------------------------------------------------- +# Shared helpers — a parser built the way the real CLI builds it, plus a minimal plan. +# -------------------------------------------------------------------------------------------- + + +def parse(argv: list[str]) -> argparse.Namespace: + parser = argparse.ArgumentParser(prog="factory") + sub = parser.add_subparsers(dest="command") + cli.build_contained_parser(sub) + return parser.parse_args(["contained", *argv]) + + +def interpret(argv: list[str]) -> argparse.Namespace: + args = parse(argv) + cli.interpret(cli._PARSER, args) + return args + + +def _completed( + returncode: int = 0, stdout: str = "", stderr: str = "" +) -> subprocess.CompletedProcess[str]: + return subprocess.CompletedProcess([], returncode, stdout, stderr) + + +def _plan(tmp_path: Path) -> ContainerPlan: + workspace = tmp_path / "rta" + workspace.mkdir(exist_ok=True) + return ContainerPlan( + name="rta-abc123", + image="example/runtime:latest", + workdir=str(workspace), + env={"FACTORY_CONTAINED": "1", "HOME": CONTAINER_HOME}, + labels={LABEL_CONTAINED: "true", LABEL_PROJECT: "deadbeef"}, + mounts=(Mount(workspace, str(workspace)),), + run_command=f"cd {workspace} && factory study {workspace}", + user="501:0", + ) + + +class _FakeDivision: + """Stand-in for `division.Division` — records whether the run kept or stopped the endpoint.""" + + def __init__(self, plan: ContainerPlan) -> None: + self.plan = plan + self.kept = False + self.stopped = False + + def keep(self) -> None: + self.kept = True + + def stop(self) -> None: + self.stopped = True + + +# ============================================================================================ +# contained.py — the front door: dispatch routing, the Ctrl-C handler, and `_verify`. +# ============================================================================================ + + +def test_keyboard_interrupt_is_caught_and_reported_as_130( + capsys: pytest.CaptureFixture[str], +) -> None: + """Backing out of a wizard is ordinary, so an interrupt is a message and 130, not a traceback.""" + with patch.object(cli, "_dispatch", side_effect=KeyboardInterrupt): + code = cli.cmd_contained(argparse.Namespace()) + assert code == 130 + assert "Stopped." in capsys.readouterr().err + + +def test_context_is_pinned_once_when_given() -> None: + """`--context` is applied globally so no downstream cluster command has to remember it.""" + args = interpret(["--target", "k8s", "--context", "ctx", "verify"]) + with patch("factory.contained.k8s.set_active_context") as pin, \ + patch("factory.contained.k8s_setup.verify_k8s", return_value=[SimpleNamespace(ok=True)]), \ + patch("factory.contained.prereq.format_check", return_value=""), \ + patch("factory.contained.prereq.summary_line", return_value="ok"): + assert cli.cmd_contained(args) == 0 + pin.assert_called_once_with("ctx") + + +def test_verify_k8s_streams_and_returns_zero_when_all_pass( + capsys: pytest.CaptureFixture[str], +) -> None: + args = interpret(["--target", "k8s", "--namespace", "ns", "verify"]) + with patch( + "factory.contained.k8s_setup.verify_k8s", return_value=[SimpleNamespace(ok=True)] + ) as vk, \ + patch("factory.contained.prereq.format_check", return_value="line"), \ + patch("factory.contained.prereq.summary_line", return_value="summary"): + assert cli.cmd_contained(args) == 0 + assert vk.call_args.kwargs["namespace"] == "ns" + assert "summary" in capsys.readouterr().out + + +def test_verify_k8s_returns_one_when_a_check_fails() -> None: + args = interpret(["--target", "k8s", "verify"]) + with patch("factory.contained.k8s_setup.verify_k8s", return_value=[SimpleNamespace(ok=False)]), \ + patch("factory.contained.prereq.format_check", return_value=""), \ + patch("factory.contained.prereq.summary_line", return_value=""): + assert cli.cmd_contained(args) == 1 + + +def test_verify_local_returns_zero_when_all_pass(capsys: pytest.CaptureFixture[str]) -> None: + # `local_checks`/`render_checks` are imported into `contained.py` at module load, so they must + # be patched there rather than at their source module. + args = interpret(["verify"]) + with patch("factory.cli.contained.local_checks", return_value=[SimpleNamespace(ok=True)]), \ + patch("factory.cli.contained.render_checks", return_value="rendered"): + assert cli.cmd_contained(args) == 0 + assert "rendered" in capsys.readouterr().out + + +def test_verify_local_returns_one_when_a_check_fails() -> None: + args = interpret(["verify"]) + with patch("factory.cli.contained.local_checks", return_value=[SimpleNamespace(ok=False)]), \ + patch("factory.cli.contained.render_checks", return_value=""): + assert cli.cmd_contained(args) == 1 + + +def test_setup_is_dispatched_with_the_interactive_and_target_context() -> None: + args = interpret(["setup"]) + with patch("factory.cli.contained.run_setup", return_value=0) as run_setup, \ + patch("factory.cli.contained.sys.stdin.isatty", return_value=False): + assert cli.cmd_contained(args) == 0 + # `--target` was never typed, so setup is asked to decide the target itself (None). + assert run_setup.call_args.args[0] is None + assert run_setup.call_args.kwargs["interactive"] is False + + +def test_bundle_is_dispatched_and_implies_the_cluster_target( + capsys: pytest.CaptureFixture[str], +) -> None: + args = interpret(["bundle"]) + assert args.target == "k8s" # `interpret` promotes bundle to the cluster target + with patch("factory.contained.bundle.render_bundle", return_value="MANIFEST") as render, \ + patch("factory.podman.resolve_image", return_value="img:latest"): + assert cli.cmd_contained(args) == 0 + assert render.call_args.kwargs["image"] == "img:latest" + assert "MANIFEST" in capsys.readouterr().out + + +def test_bundle_uses_an_explicit_image_when_given() -> None: + # Runtime flags go before the subcommand on this parser. + args = interpret(["--image", "custom:tag", "bundle"]) + with patch("factory.contained.bundle.render_bundle", return_value="M") as render, \ + patch("factory.podman.resolve_image") as resolve: + assert cli.cmd_contained(args) == 0 + resolve.assert_not_called() + assert render.call_args.kwargs["image"] == "custom:tag" + + +def test_a_lifecycle_subcommand_is_handed_to_dispatch_lifecycle() -> None: + args = interpret(["ls"]) + with patch("factory.cli.contained.dispatch_lifecycle", return_value=0) as dispatch: + assert cli.cmd_contained(args) == 0 + dispatch.assert_called_once() + + +def test_a_payload_run_against_k8s_routes_to_run_k8s() -> None: + args = interpret(["--target", "k8s", "--", "ceo", "/tmp"]) + with patch("factory.cli.contained_k8s.run_k8s", return_value=0) as run_k8s: + assert cli.cmd_contained(args) == 0 + run_k8s.assert_called_once_with(args) + + +def test_a_payload_run_against_local_routes_to_run_local() -> None: + # `run_local` is imported into `contained.py` at module load, so patch it there. + args = interpret(["--", "study", "/tmp"]) + with patch("factory.cli.contained.run_local", return_value=0) as run_local: + assert cli.cmd_contained(args) == 0 + run_local.assert_called_once_with(args) + + +def test_help_subcommand_prints_the_help_and_returns_zero( + capsys: pytest.CaptureFixture[str], +) -> None: + args = interpret(["help"]) + assert cli.cmd_contained(args) == 0 + assert "Targets:" in capsys.readouterr().out + + +# ============================================================================================ +# contained_args.py — the remaining command-line reading branches. +# ============================================================================================ + + +def test_within_one_edit_covers_every_shape() -> None: + we = contained_args._within_one_edit + assert we("ls", "ls") is True # identical + assert we("abcd", "xy") is False # length gap > 1 + assert we("ls", "lx") is True # one substitution + assert we("ls", "xy") is False # two substitutions + assert we("ls", "lst") is True # one insertion/deletion + assert we("ab", "cde") is False # same length gap of 1, but nothing matches + + +def test_a_close_typo_of_a_subcommand_is_named() -> None: + """`lst` is one edit from `ls`, so it is caught before the passthrough path.""" + parser = argparse.ArgumentParser() + with pytest.raises(SystemExit): + contained_args._reject_subcommand_typo(parser, ["lst"]) + + +def test_a_leading_flag_is_left_for_the_real_parser() -> None: + """A token beginning with `-` is not a subcommand typo; the check returns without complaint.""" + parser = argparse.ArgumentParser() + contained_args._reject_subcommand_typo(parser, ["--something"]) # no raise + + +def test_an_existing_directory_is_not_treated_as_a_typo(tmp_path: Path) -> None: + parser = argparse.ArgumentParser() + contained_args._reject_subcommand_typo(parser, [str(tmp_path)]) # no raise + + +def test_a_word_far_from_any_subcommand_is_left_alone() -> None: + """A free-text first token that resembles no subcommand falls through to the passthrough.""" + parser = argparse.ArgumentParser() + contained_args._reject_subcommand_typo(parser, ["totallyunrelated"]) # no raise + + +def test_an_empty_remainder_is_a_no_op() -> None: + parser = argparse.ArgumentParser() + contained_args._reject_subcommand_typo(parser, []) # no raise + + +def test_target_given_recognizes_both_the_space_and_equals_forms() -> None: + with patch.object(contained_args.sys, "argv", ["factory", "contained", "--target", "k8s"]): + assert contained_args.target_given(argparse.Namespace()) is True + with patch.object(contained_args.sys, "argv", ["factory", "contained", "--target=local"]): + assert contained_args.target_given(argparse.Namespace()) is True + with patch.object(contained_args.sys, "argv", ["factory", "contained", "setup"]): + assert contained_args.target_given(argparse.Namespace()) is False + + +def test_bundle_interpret_promotes_the_target_to_k8s() -> None: + """Directly exercises the `subcommand == "bundle"` promotion line inside `interpret`.""" + args = interpret(["--namespace", "ns", "bundle"]) + assert (args.subcommand, args.target, args.namespace) == ("bundle", "k8s", "ns") + + +def test_forwarding_an_unset_variable_raises() -> None: + args = argparse.Namespace(extra_env=[], forward=["DEFINITELY_NOT_SET_XYZ"]) + with patch.dict(contained_args.os.environ, {}, clear=True): + with pytest.raises(ContainedError, match="not set in this environment"): + contained_args.validate_env_args(args) + + +def test_forwarding_a_set_variable_returns_its_value() -> None: + args = argparse.Namespace(extra_env=["A=1"], forward=["PRESENT_XYZ"]) + with patch.dict(contained_args.os.environ, {"PRESENT_XYZ": "here"}, clear=False): + extra, forwarded = contained_args.validate_env_args(args) + assert extra == {"A": "1"} + assert forwarded == {"PRESENT_XYZ": "here"} + + +def test_parse_extra_env_rejects_a_pair_without_an_equals() -> None: + with pytest.raises(ContainedError, match="not KEY=VALUE"): + contained_args.parse_extra_env(["NOEQUALS"]) + + +def test_parse_extra_env_rejects_a_blank_key() -> None: + with pytest.raises(ContainedError, match="not KEY=VALUE"): + contained_args.parse_extra_env(["=value"]) + + +def test_resolve_project_returns_the_first_existing_directory(tmp_path: Path) -> None: + project = tmp_path / "proj" + project.mkdir() + assert contained_args.resolve_project(["ceo", str(project), "--loop"]) == project.resolve() + + +def test_resolve_project_raises_when_no_directory_is_named() -> None: + with pytest.raises(ContainedError, match="no existing directory"): + contained_args.resolve_project(["ceo", "--focus", "x"]) + + +# ============================================================================================ +# contained_local.py — helpers. +# ============================================================================================ + + +def test_macos_share_warning_is_silent_off_darwin() -> None: + with patch.object(contained_local.platform, "system", return_value="Linux"): + assert contained_local._macos_share_warning([Mount(Path("/x"), "/x")]) is None + + +def test_macos_share_warning_is_silent_when_no_shared_paths_are_known() -> None: + with patch.object(contained_local.platform, "system", return_value="Darwin"), \ + patch.object(contained_local, "_machine_shared_paths", return_value=[]): + assert contained_local._macos_share_warning([Mount(Path("/x"), "/x")]) is None + + +def test_macos_share_warning_is_silent_when_every_mount_is_inside_a_shared_path() -> None: + with patch.object(contained_local.platform, "system", return_value="Darwin"), \ + patch.object(contained_local, "_machine_shared_paths", return_value=[Path("/shared")]): + assert contained_local._macos_share_warning([Mount(Path("/shared/proj"), "/x")]) is None + + +def test_macos_share_warning_names_a_mount_outside_the_shared_paths() -> None: + with patch.object(contained_local.platform, "system", return_value="Darwin"), \ + patch.object(contained_local, "_machine_shared_paths", return_value=[Path("/shared")]): + warning = contained_local._macos_share_warning([Mount(Path("/elsewhere/proj"), "/x")]) + assert warning is not None and "/elsewhere/proj" in warning and "/shared" in warning + + +def test_machine_shared_paths_returns_empty_when_podman_is_absent() -> None: + with patch.object(contained_local.subprocess, "run", side_effect=FileNotFoundError): + assert contained_local._machine_shared_paths() == [] + + +def test_machine_shared_paths_returns_empty_on_a_nonzero_exit() -> None: + with patch.object(contained_local.subprocess, "run", return_value=_completed(returncode=1)): + assert contained_local._machine_shared_paths() == [] + + +def test_machine_shared_paths_keeps_only_absolute_paths() -> None: + out = "/abs/one\nrelative\n \n/abs/two\n" + with patch.object(contained_local.subprocess, "run", return_value=_completed(stdout=out)): + assert contained_local._machine_shared_paths() == [Path("/abs/one"), Path("/abs/two")] + + +def test_handle_create_failure_ignores_a_non_create_step(tmp_path: Path) -> None: + plan = _plan(tmp_path) + result = _completed(returncode=1, stderr="already in use") + out, hint = contained_local._handle_create_failure(Step("run", ["x"]), result, plan) + assert out is result and hint is None + + +def test_handle_create_failure_ignores_an_unrelated_create_error(tmp_path: Path) -> None: + plan = _plan(tmp_path) + result = _completed(returncode=1, stderr="disk full") + out, hint = contained_local._handle_create_failure(Step("create", ["x"]), result, plan) + assert out is result and hint is None + + +def test_handle_create_failure_reaps_and_retries_successfully(tmp_path: Path) -> None: + plan = _plan(tmp_path) + first = _completed(returncode=1, stderr="name already in use") + retry = _completed(returncode=0) + with patch.object(contained_local, "reap_stale", return_value=(True, "was exited")), \ + patch.object(contained_local, "_run_step", return_value=retry): + out, hint = contained_local._handle_create_failure(Step("create", ["x"]), first, plan) + assert out is retry and hint is None + + +def test_handle_create_failure_reaps_but_the_retry_still_fails(tmp_path: Path) -> None: + plan = _plan(tmp_path) + first = _completed(returncode=1, stderr="already exists") + retry = _completed(returncode=1, stderr="still broken") + with patch.object(contained_local, "reap_stale", return_value=(True, "was exited")), \ + patch.object(contained_local, "_run_step", return_value=retry): + out, hint = contained_local._handle_create_failure(Step("create", ["x"]), first, plan) + assert out is retry and hint is not None and "already exists" in hint + + +def test_handle_create_failure_declines_to_reap_a_live_container(tmp_path: Path) -> None: + plan = _plan(tmp_path) + first = _completed(returncode=1, stderr="name already in use") + with patch.object(contained_local, "reap_stale", return_value=(False, "still active")), \ + patch.object(contained_local, "_run_step") as run_step: + out, hint = contained_local._handle_create_failure(Step("create", ["x"]), first, plan) + run_step.assert_not_called() + assert out is first and hint is not None and "still active" in hint + + +def test_run_step_uses_a_longer_timeout_for_create() -> None: + with patch.object(contained_local.subprocess, "run", return_value=_completed()) as run: + contained_local._run_step(Step("create", ["podman", "run"])) + assert run.call_args.kwargs["timeout"] == 300 + with patch.object(contained_local.subprocess, "run", return_value=_completed()) as run: + contained_local._run_step(Step("run", ["podman", "exec"])) + assert run.call_args.kwargs["timeout"] == 120 + + +def test_roll_back_is_a_no_op_without_a_workspace() -> None: + contained_local._roll_back(None) # no raise + + +def test_roll_back_is_a_no_op_when_the_copy_is_already_gone(tmp_path: Path) -> None: + ws = Workspace(source=tmp_path, path=tmp_path / "gone", kind="copy") + contained_local._roll_back(ws) # no raise + + +def test_roll_back_releases_the_copy_and_removes_an_empty_run_dir(tmp_path: Path) -> None: + import shutil + + run_dir = tmp_path / "run" + copy = run_dir / "rta" + copy.mkdir(parents=True) + ws = Workspace(source=tmp_path / "src", path=copy, kind="worktree", branch="b") + + def _release(workspace: Workspace, *, delete_branch: bool) -> None: + shutil.rmtree(workspace.path) + + with patch("factory.contained.workspace.release", side_effect=_release): + contained_local._roll_back(ws) + assert not run_dir.exists(), "the emptied run directory should be removed too" + + +def test_roll_back_leaves_a_run_dir_that_still_holds_other_work(tmp_path: Path) -> None: + """When the run directory is not empty after release, it is left in place, not removed.""" + import shutil + + run_dir = tmp_path / "run" + copy = run_dir / "rta" + copy.mkdir(parents=True) + (run_dir / "sibling").mkdir() # keeps the run dir non-empty after the copy is released + ws = Workspace(source=tmp_path / "src", path=copy, kind="worktree", branch="b") + + def _release(workspace: Workspace, *, delete_branch: bool) -> None: + shutil.rmtree(workspace.path) + + with patch("factory.contained.workspace.release", side_effect=_release): + contained_local._roll_back(ws) + assert run_dir.exists() and (run_dir / "sibling").exists() + + +def test_roll_back_reports_rather_than_masks_a_cleanup_failure( + tmp_path: Path, capsys: pytest.CaptureFixture[str] +) -> None: + copy = tmp_path / "run" / "rta" + copy.mkdir(parents=True) + ws = Workspace(source=tmp_path / "src", path=copy, kind="worktree", branch="b") + with patch("factory.contained.workspace.release", side_effect=WorkspaceError("locked")), \ + patch("factory.contained.workspace.cleanup_hint", return_value="do this by hand"): + contained_local._roll_back(ws) + err = capsys.readouterr().err + assert "could not clean up" in err and "do this by hand" in err + + +def test_settle_workspace_keeps_the_copy_on_success(tmp_path: Path) -> None: + ws = Workspace(source=tmp_path, path=tmp_path, kind="copy") + with patch.object(contained_local, "_roll_back") as roll_back: + contained_local._settle_workspace(ws, code=0, created=True) + roll_back.assert_not_called() + + +def test_settle_workspace_rolls_back_when_nothing_was_created(tmp_path: Path) -> None: + ws = Workspace(source=tmp_path, path=tmp_path, kind="copy") + with patch.object(contained_local, "_roll_back") as roll_back: + contained_local._settle_workspace(ws, code=1, created=False) + roll_back.assert_called_once_with(ws) + + +def test_settle_workspace_keeps_a_created_runtime_for_inspection( + tmp_path: Path, capsys: pytest.CaptureFixture[str] +) -> None: + ws = Workspace(source=tmp_path, path=tmp_path, kind="copy") + with patch("factory.contained.workspace.cleanup_hint", return_value="inspect me"): + contained_local._settle_workspace(ws, code=1, created=True) + assert "inspect me" in capsys.readouterr().err + + +def test_announce_prints_the_lifecycle_commands( + tmp_path: Path, capsys: pytest.CaptureFixture[str] +) -> None: + contained_local._announce(_plan(tmp_path)) + out = capsys.readouterr().out + assert "Starting rta-abc123" in out + assert "attach:" in out and "sync" in out and "rm" in out + + +def test_execute_runs_every_step_and_reports_a_created_container(tmp_path: Path) -> None: + plan = _plan(tmp_path) + steps = [Step("create", ["c"]), Step("run", ["r"])] + with patch.object(contained_local, "_run_step", return_value=_completed()): + code, created = contained_local._execute(plan, steps, []) + assert code == 0 and created is True + + +def test_execute_returns_130_on_interrupt( + tmp_path: Path, capsys: pytest.CaptureFixture[str] +) -> None: + plan = _plan(tmp_path) + with patch.object(contained_local, "_run_step", side_effect=KeyboardInterrupt): + code, created = contained_local._execute(plan, [Step("create", ["c"])], []) + assert code == 130 and created is False + assert "may still be running" in capsys.readouterr().err + + +def test_execute_reports_a_failure_before_the_container_exists( + tmp_path: Path, capsys: pytest.CaptureFixture[str] +) -> None: + plan = _plan(tmp_path) + failed = _completed(returncode=1, stderr="boom") + with patch.object(contained_local, "_run_step", return_value=failed), \ + patch.object(contained_local, "_handle_create_failure", return_value=(failed, "a hint")): + code, created = contained_local._execute(plan, [Step("create", ["c"])], []) + assert code == 1 and created is False + err = capsys.readouterr().err + assert "step 'create' failed" in err and "a hint" in err + + +def test_execute_keeps_a_created_container_after_a_later_step_fails( + tmp_path: Path, capsys: pytest.CaptureFixture[str] +) -> None: + plan = _plan(tmp_path) + steps = [Step("create", ["c"]), Step("assert:git", ["g"])] + probes = [Probe(name="git", argv=["g"], hint="mount hint")] + with patch.object( + contained_local, "_run_step", + side_effect=[_completed(), _completed(returncode=1, stderr="assertion failed")], + ): + code, created = contained_local._execute(plan, steps, probes) + assert code == 1 and created is True + err = capsys.readouterr().err + assert "still there for inspection" in err and "mount hint" in err + + +def test_probes_for_projects_from_the_source_in_dry_run( + tmp_path: Path, capsys: pytest.CaptureFixture[str] +) -> None: + project = tmp_path / "proj" + project.mkdir() + ws = Workspace(source=project, path=tmp_path / "copy", kind="copy") + with patch.object(contained_local, "content_probe", return_value=("a.txt", "deadbeef")): + probes = contained_local._probes_for(ws, project, dry_run=True) + assert probes # a real probe list is still produced + assert "projection from the source tree" in capsys.readouterr().err + + +def test_probes_for_is_silent_when_the_source_has_no_content_probe(tmp_path: Path) -> None: + project = tmp_path / "proj" + project.mkdir() + ws = Workspace(source=project, path=tmp_path / "copy", kind="copy") + with patch.object(contained_local, "content_probe", return_value=None): + probes = contained_local._probes_for(ws, project, dry_run=True) + assert probes + + +def test_probes_for_measures_the_copy_when_not_a_dry_run(tmp_path: Path) -> None: + project = tmp_path / "proj" + project.mkdir() + copy = tmp_path / "copy" + copy.mkdir() + ws = Workspace(source=project, path=copy, kind="copy") + with patch.object(contained_local, "content_probe", return_value=None) as content: + contained_local._probes_for(ws, project, dry_run=False) + content.assert_called_once_with(copy) + + +# -------------------------------------------------------------------------------------------- +# contained_local.py — `_build_plan` composition branches. +# -------------------------------------------------------------------------------------------- + + +def _plan_args(tmp_path: Path, **overrides: object) -> argparse.Namespace: + project = tmp_path / "src" + project.mkdir(exist_ok=True) + fields: dict[str, object] = dict( + image="img:pinned", extra_env=[], forward=[], mount=[], + factory_args=["study", str(project)], name=None, + ) + fields.update(overrides) + return argparse.Namespace(**fields) + + +def test_build_plan_warns_when_no_credentials_and_home_is_absent(tmp_path: Path) -> None: + """A non-worktree copy, no `~/.factory`, no credentials: the missing-inference warning fires.""" + home = tmp_path / "empty-home" + home.mkdir() + (tmp_path / "src").mkdir(exist_ok=True) + (tmp_path / "copy").mkdir(exist_ok=True) + ws = Workspace(source=tmp_path / "src", path=tmp_path / "copy", kind="copy") + args = _plan_args(tmp_path) + with patch.dict(contained_local.os.environ, {"HOME": str(home)}, clear=False), \ + patch.object( + contained_local, "resolve_credentials", + return_value=CredentialShape(backend="none", ok=False, detail=""), + ): + plan = contained_local._build_plan(args, ws, dry_run=True) + assert any("no inference credentials" in w for w in plan.warnings) + # No `~/.factory` mount was added, and a plain copy adds no git-common mount. + assert not any(m.target.endswith(".factory") for m in plan.mounts) + assert plan.image == "img:pinned" + + +def test_build_plan_appends_a_macos_share_warning(tmp_path: Path) -> None: + (tmp_path / "src").mkdir(exist_ok=True) + (tmp_path / "copy").mkdir(exist_ok=True) + ws = Workspace(source=tmp_path / "src", path=tmp_path / "copy", kind="copy") + args = _plan_args(tmp_path) + with patch.object( + contained_local, "resolve_credentials", + return_value=CredentialShape(backend="none", ok=False, detail=""), + ), \ + patch.object(contained_local, "_macos_share_warning", return_value="share warning"): + plan = contained_local._build_plan(args, ws, dry_run=True) + assert "share warning" in plan.warnings + + +def test_build_plan_rejects_an_extra_mount_that_does_not_exist(tmp_path: Path) -> None: + (tmp_path / "src").mkdir(exist_ok=True) + (tmp_path / "copy").mkdir(exist_ok=True) + ws = Workspace(source=tmp_path / "src", path=tmp_path / "copy", kind="copy") + args = _plan_args(tmp_path, mount=[str(tmp_path / "nope")]) + with patch.object( + contained_local, "resolve_credentials", + return_value=CredentialShape(backend="none", ok=False, detail=""), + ): + with pytest.raises(ContainedError, match="no such path"): + contained_local._build_plan(args, ws, dry_run=True) + + +def test_build_plan_mounts_an_existing_extra_path(tmp_path: Path) -> None: + (tmp_path / "src").mkdir(exist_ok=True) + (tmp_path / "copy").mkdir(exist_ok=True) + ws = Workspace(source=tmp_path / "src", path=tmp_path / "copy", kind="copy") + extra = tmp_path / "extra" + extra.mkdir() + args = _plan_args(tmp_path, mount=[str(extra)]) + with patch.object( + contained_local, "resolve_credentials", + return_value=CredentialShape(backend="none", ok=False, detail=""), + ): + plan = contained_local._build_plan(args, ws, dry_run=True) + assert any(m.source == extra.resolve() for m in plan.mounts) + + +def test_build_plan_skips_the_git_common_mount_when_there_is_none(tmp_path: Path) -> None: + """A worktree whose common dir cannot be resolved simply adds no git mount.""" + (tmp_path / "src").mkdir(exist_ok=True) + (tmp_path / "copy").mkdir(exist_ok=True) + ws = Workspace(source=tmp_path / "src", path=tmp_path / "copy", kind="worktree", branch="b") + args = _plan_args(tmp_path) + with patch.object( + contained_local, "resolve_credentials", + return_value=CredentialShape(backend="none", ok=False, detail=""), + ), \ + patch.object(contained_local, "git_common_dir", return_value=None): + plan = contained_local._build_plan(args, ws, dry_run=True) + assert not any(m.target.endswith(".git") for m in plan.mounts) + + +def test_build_plan_mounts_the_worktree_common_dir_when_present(tmp_path: Path) -> None: + common = tmp_path / "src" / ".git" + common.mkdir(parents=True) + (tmp_path / "copy").mkdir(exist_ok=True) + ws = Workspace(source=tmp_path / "src", path=tmp_path / "copy", kind="worktree", branch="b") + args = _plan_args(tmp_path) + with patch.object( + contained_local, "resolve_credentials", + return_value=CredentialShape(backend="none", ok=False, detail=""), + ), \ + patch.object(contained_local, "git_common_dir", return_value=common): + plan = contained_local._build_plan(args, ws, dry_run=True) + assert any(m.source == common for m in plan.mounts) + + +# -------------------------------------------------------------------------------------------- +# contained_local.py — `run_local`, the non-dry-run execution path. +# -------------------------------------------------------------------------------------------- + + +def _run_local_args(tmp_path: Path, **overrides: object) -> argparse.Namespace: + project = tmp_path / "src" + project.mkdir(exist_ok=True) + fields: dict[str, object] = dict( + image=None, extra_env=[], forward=[], mount=[], division=False, + factory_args=["study", str(project)], name="rta-run", + ) + fields.update(overrides) + return argparse.Namespace(**fields) + + +def test_run_local_executes_and_settles_on_success(tmp_path: Path) -> None: + args = _run_local_args(tmp_path) + ws = Workspace(source=tmp_path / "src", path=tmp_path / "copy", kind="copy") + plan = _plan(tmp_path) + with patch.object(contained_local, "dry_run_enabled", return_value=False), \ + patch.object(contained_local, "materialize", return_value=ws), \ + patch.object(contained_local, "_build_plan", return_value=plan), \ + patch.object(contained_local, "_probes_for", return_value=[]), \ + patch.object(contained_local, "plan_steps", return_value=[Step("run", ["r"])]), \ + patch.object(contained_local, "growth_context_warning", return_value=None), \ + patch.object(contained_local.shutil, "which", return_value="/usr/bin/podman"), \ + patch("factory.contained.usage.record_target") as record, \ + patch.object(contained_local, "_execute", return_value=(0, True)), \ + patch.object(contained_local, "_settle_workspace") as settle: + assert contained_local.run_local(args) == 0 + record.assert_called_once_with("local") + settle.assert_called_once() + + +def test_run_local_errors_when_podman_is_missing( + tmp_path: Path, capsys: pytest.CaptureFixture[str] +) -> None: + args = _run_local_args(tmp_path) + ws = Workspace(source=tmp_path / "src", path=tmp_path / "gone", kind="copy") + plan = _plan(tmp_path) + with patch.object(contained_local, "dry_run_enabled", return_value=False), \ + patch.object(contained_local, "materialize", return_value=ws), \ + patch.object(contained_local, "_build_plan", return_value=plan), \ + patch.object(contained_local, "_probes_for", return_value=[]), \ + patch.object(contained_local, "plan_steps", return_value=[Step("run", ["r"])]), \ + patch.object(contained_local, "growth_context_warning", return_value=None), \ + patch.object(contained_local.shutil, "which", return_value=None): + assert contained_local.run_local(args) == 1 + assert "`podman` is not installed" in capsys.readouterr().err + + +def test_run_local_reports_a_provisioning_error_and_returns_two( + tmp_path: Path, capsys: pytest.CaptureFixture[str] +) -> None: + args = _run_local_args(tmp_path) + with patch.object(contained_local, "dry_run_enabled", return_value=False), \ + patch.object(contained_local, "materialize", side_effect=WorkspaceError("no copy")): + assert contained_local.run_local(args) == 2 + assert "Error: no copy" in capsys.readouterr().err + + +def test_run_local_prints_warnings_but_keeps_the_exit_code( + tmp_path: Path, capsys: pytest.CaptureFixture[str] +) -> None: + args = _run_local_args(tmp_path) + ws = Workspace(source=tmp_path / "src", path=tmp_path / "copy", kind="copy") + plan = ContainerPlan( + name="rta-warn", image="i", workdir=str(tmp_path), env={}, labels={}, + mounts=(), run_command="x", warnings=("credentials missing",), + ) + with patch.object(contained_local, "dry_run_enabled", return_value=False), \ + patch.object(contained_local, "materialize", return_value=ws), \ + patch.object(contained_local, "_build_plan", return_value=plan), \ + patch.object(contained_local, "_probes_for", return_value=[]), \ + patch.object(contained_local, "plan_steps", return_value=[Step("run", ["r"])]), \ + patch.object(contained_local, "growth_context_warning", return_value="a growth warning"), \ + patch.object(contained_local.shutil, "which", return_value="/usr/bin/podman"), \ + patch("factory.contained.usage.record_target"), \ + patch.object(contained_local, "_execute", return_value=(0, True)), \ + patch.object(contained_local, "_settle_workspace"): + assert contained_local.run_local(args) == 0 + err = capsys.readouterr().err + assert "a growth warning" in err and "credentials missing" in err + + +def test_run_local_starts_and_keeps_the_division_on_success(tmp_path: Path) -> None: + args = _run_local_args(tmp_path, division=True) + ws = Workspace(source=tmp_path / "src", path=tmp_path / "copy", kind="copy") + plan = _plan(tmp_path) + division = _FakeDivision(plan) + with patch.object(contained_local, "dry_run_enabled", return_value=False), \ + patch.object(contained_local, "materialize", return_value=ws), \ + patch.object(contained_local, "_build_plan", return_value=plan), \ + patch.object(contained_local, "_probes_for", return_value=[]), \ + patch.object(contained_local, "plan_steps", return_value=[Step("run", ["r"])]), \ + patch.object(contained_local, "growth_context_warning", return_value=None), \ + patch.object(contained_local.shutil, "which", return_value="/usr/bin/podman"), \ + patch("factory.contained.division.start_local_division", return_value=division), \ + patch("factory.contained.usage.record_target"), \ + patch.object(contained_local, "_execute", return_value=(0, True)), \ + patch.object(contained_local, "_settle_workspace"): + assert contained_local.run_local(args) == 0 + assert division.kept is True and division.stopped is False + + +def test_run_local_stops_the_division_when_the_run_fails(tmp_path: Path) -> None: + args = _run_local_args(tmp_path, division=True) + ws = Workspace(source=tmp_path / "src", path=tmp_path / "copy", kind="copy") + plan = _plan(tmp_path) + division = _FakeDivision(plan) + with patch.object(contained_local, "dry_run_enabled", return_value=False), \ + patch.object(contained_local, "materialize", return_value=ws), \ + patch.object(contained_local, "_build_plan", return_value=plan), \ + patch.object(contained_local, "_probes_for", return_value=[]), \ + patch.object(contained_local, "plan_steps", return_value=[Step("run", ["r"])]), \ + patch.object(contained_local, "growth_context_warning", return_value=None), \ + patch.object(contained_local.shutil, "which", return_value="/usr/bin/podman"), \ + patch("factory.contained.division.start_local_division", return_value=division), \ + patch("factory.contained.usage.record_target"), \ + patch.object(contained_local, "_execute", return_value=(1, True)), \ + patch.object(contained_local, "_settle_workspace"): + assert contained_local.run_local(args) == 1 + assert division.kept is False and division.stopped is True diff --git a/tests/test_contained_k8s.py b/tests/test_contained_k8s.py index e1336c1c2..897cadd75 100644 --- a/tests/test_contained_k8s.py +++ b/tests/test_contained_k8s.py @@ -1033,3 +1033,10 @@ def test_a_sweep_that_deleted_something_reports_a_count( return_value=_completed('pod "a" deleted\npod "b" deleted')): k8s.remove_cluster_runtime("rta-test", namespace="ns") assert "swept 2 pod(s)" in capsys.readouterr().out + + +def test_a_plan_without_a_vertex_model_warning_adds_no_warning(tmp_path: Path) -> None: + """The common path: a non-Vertex backend produces no model warning, so none is appended.""" + with patch("factory.cli.contained_k8s.vertex_model_warning", return_value=None): + plan = _plan(tmp_path) + assert not any("--model" in w for w in plan.warnings) diff --git a/tests/test_contained_k8s_coverage.py b/tests/test_contained_k8s_coverage.py new file mode 100644 index 000000000..9e74aa032 --- /dev/null +++ b/tests/test_contained_k8s_coverage.py @@ -0,0 +1,521 @@ +"""Coverage-completing tests for `factory.contained.k8s`. + +The existing suites cover the happy paths and the manifest shapes; this file targets the failure +directions those leave behind — every "the CLI could not be run", "the JSON was junk", "the token +expired", and "the pod never started" branch. Each of these is a place the module deliberately +degrades rather than raising, and a branch that degrades wrongly is exactly the kind of bug that +only shows up on a real cluster having a bad day, so it is worth pinning here. + +Nothing in this file may touch a real cluster: everything shells out through `k8s._run` / +`k8s.subprocess.run` / `shutil.which`, and every test patches those. `wait_for_container` polls on +a clock, so its tests patch `time.monotonic` (to drive the deadline instantly) and `time.sleep` +(to a no-op) — the module imports `time` inside the function, so those are patched on the real +`time` module rather than on the k8s namespace. +""" + +from __future__ import annotations + +import json +import subprocess +from pathlib import Path +from unittest.mock import patch + +import pytest + +from factory.contained import k8s +from factory.contained.runtimes import LifecycleError + + +def _completed(stdout: str = "", returncode: int = 0, stderr: str = "") -> subprocess.CompletedProcess[str]: + return subprocess.CompletedProcess([], returncode, stdout, stderr) + + +# -------------------------------------------------------------------------------------------- +# cli_binary — the "neither tool is installed" path +# -------------------------------------------------------------------------------------------- + + +def test_cli_binary_prefers_oc_then_kubectl() -> None: + with patch("factory.contained.k8s.shutil.which", side_effect=lambda c: c == "oc"): + assert k8s.cli_binary() == "oc" + # oc absent, kubectl present — exercises the loop advancing past the first candidate. + with patch("factory.contained.k8s.shutil.which", side_effect=lambda c: c == "kubectl"): + assert k8s.cli_binary() == "kubectl" + + +def test_cli_binary_raises_when_neither_is_on_path() -> None: + with patch("factory.contained.k8s.shutil.which", return_value=None): + with pytest.raises(k8s.ClusterError, match="neither"): + k8s.cli_binary() + + +# -------------------------------------------------------------------------------------------- +# current_namespace +# -------------------------------------------------------------------------------------------- + + +def test_current_namespace_reads_the_context_namespace() -> None: + with patch("factory.contained.k8s.cli_binary", return_value="oc"), \ + patch("factory.contained.k8s._run", return_value=_completed("team-ns\n")): + assert k8s.current_namespace() == "team-ns" + + +def test_current_namespace_is_none_when_the_read_fails_or_is_empty() -> None: + with patch("factory.contained.k8s.cli_binary", return_value="oc"), \ + patch("factory.contained.k8s._run", return_value=None): + assert k8s.current_namespace() is None + with patch("factory.contained.k8s.cli_binary", return_value="oc"), \ + patch("factory.contained.k8s._run", return_value=_completed(" ")): + assert k8s.current_namespace() is None + + +# -------------------------------------------------------------------------------------------- +# _kubeconfig_json / list_contexts / _first_section / cluster_context +# -------------------------------------------------------------------------------------------- + + +def test_kubeconfig_json_degrades_to_empty_on_a_failed_read() -> None: + with patch("factory.contained.k8s._run", return_value=None): + assert k8s._kubeconfig_json(["oc", "config", "view"]) == {} + with patch("factory.contained.k8s._run", return_value=_completed("{}", 1)): + assert k8s._kubeconfig_json(["oc", "config", "view"]) == {} + + +def test_kubeconfig_json_degrades_when_the_top_level_is_not_an_object() -> None: + with patch("factory.contained.k8s._run", return_value=_completed("[1, 2, 3]")): + assert k8s._kubeconfig_json(["oc", "config", "view"]) == {} + + +def test_list_contexts_is_empty_when_no_cli_is_installed() -> None: + with patch("factory.contained.k8s.cli_binary", side_effect=k8s.ClusterError("no cli")): + assert k8s.list_contexts() == [] + + +def test_list_contexts_skips_non_dict_entries() -> None: + payload = json.dumps({ + "contexts": ["junk", {"name": "dev", "context": {"cluster": "c1"}}], + "clusters": [{"name": "c1", "cluster": {"server": "https://x"}}], + }) + with patch("factory.contained.k8s.cli_binary", return_value="oc"), \ + patch("factory.contained.k8s._run", return_value=_completed(payload)): + contexts = k8s.list_contexts() + assert [c.context for c in contexts] == ["dev"] + assert contexts[0].server == "https://x" + + +def test_first_section_returns_empty_when_the_nested_value_is_not_a_dict() -> None: + # entries[0][inner] is present but not a dict — the 303->305 fall-through. + data = {"contexts": [{"context": "not-a-dict"}]} + assert k8s._first_section(data, "contexts", "context") == {} + # And when the list is empty. + assert k8s._first_section({"contexts": []}, "contexts", "context") == {} + + +def test_cluster_context_is_empty_when_no_cli_is_installed() -> None: + with patch("factory.contained.k8s.cli_binary", side_effect=k8s.ClusterError("no cli")): + assert k8s.cluster_context() == k8s.ClusterContext() + + +# -------------------------------------------------------------------------------------------- +# secret_keys — the failure directions the value-safe reader must not raise on +# -------------------------------------------------------------------------------------------- + + +def test_secret_keys_is_empty_when_no_cli_is_installed() -> None: + with patch("factory.contained.k8s.cli_binary", side_effect=k8s.ClusterError("no cli")): + assert k8s.secret_keys(k8s.SECRET_NAME, "ns") == set() + + +def test_secret_keys_is_empty_when_the_json_is_malformed_but_object_shaped() -> None: + # Starts with `{` so it passes the shape guard, then fails to parse — the 275-276 branch. + with patch("factory.contained.k8s.cli_binary", return_value="oc"), \ + patch("factory.contained.k8s._run", return_value=_completed("{not valid json")): + assert k8s.secret_keys(k8s.SECRET_NAME, "ns") == set() + + +# -------------------------------------------------------------------------------------------- +# use_context +# -------------------------------------------------------------------------------------------- + + +def test_use_context_reports_when_no_cli_is_installed() -> None: + with patch("factory.contained.k8s.cli_binary", side_effect=k8s.ClusterError("no cli")): + ok, detail = k8s.use_context("dev") + assert ok is False + assert "no cli" in detail + + +def test_use_context_reports_when_the_cli_could_not_be_run() -> None: + with patch("factory.contained.k8s.cli_binary", return_value="oc"), \ + patch("factory.contained.k8s._run", return_value=None): + ok, detail = k8s.use_context("dev") + assert ok is False + assert "config use-context dev" in detail + + +def test_use_context_succeeds_and_returns_the_output() -> None: + with patch("factory.contained.k8s.cli_binary", return_value="oc"), \ + patch("factory.contained.k8s._run", return_value=_completed('Switched to "dev".\n')): + ok, detail = k8s.use_context("dev") + assert ok is True + assert detail == 'Switched to "dev".' + + +def test_use_context_reports_the_failure_detail_or_a_placeholder() -> None: + with patch("factory.contained.k8s.cli_binary", return_value="oc"), \ + patch("factory.contained.k8s._run", + return_value=_completed("", 1, stderr="no context exists with the name")): + ok, detail = k8s.use_context("dev") + assert ok is False + assert "no context exists" in detail + # Non-zero but with no stderr at all falls back to the placeholder. + with patch("factory.contained.k8s.cli_binary", return_value="oc"), \ + patch("factory.contained.k8s._run", return_value=_completed("", 1)): + ok, detail = k8s.use_context("dev") + assert ok is False + assert detail == "no detail given" + + +# -------------------------------------------------------------------------------------------- +# has_cluster_context +# -------------------------------------------------------------------------------------------- + + +def test_has_cluster_context_reflects_whether_a_context_is_set() -> None: + with patch("factory.contained.k8s.cli_binary", return_value="oc"), \ + patch("factory.contained.k8s._run", return_value=_completed("dev\n")): + assert k8s.has_cluster_context() is True + with patch("factory.contained.k8s.cli_binary", return_value="oc"), \ + patch("factory.contained.k8s._run", return_value=None): + assert k8s.has_cluster_context() is False + + +# -------------------------------------------------------------------------------------------- +# resolve_namespace — the two distinct "no usable namespace" messages +# -------------------------------------------------------------------------------------------- + + +def test_resolve_namespace_blames_the_flag_only_when_a_flag_was_given() -> None: + with patch("factory.contained.k8s.current_namespace", return_value=None): + # An explicit-but-empty value points the finger at the flag, not the user. + with pytest.raises(k8s.ClusterError, match="--namespace was given"): + k8s.resolve_namespace("") + # No flag at all gets the "pass --namespace" guidance instead. + with pytest.raises(k8s.ClusterError, match="no namespace given"): + k8s.resolve_namespace(None) + + +def test_resolve_namespace_prefers_the_explicit_value() -> None: + assert k8s.resolve_namespace("mine") == "mine" + + +# -------------------------------------------------------------------------------------------- +# _run — the subprocess-failed swallow +# -------------------------------------------------------------------------------------------- + + +def test_run_returns_none_when_the_subprocess_cannot_be_launched() -> None: + with patch("factory.contained.k8s.subprocess.run", side_effect=FileNotFoundError): + assert k8s._run(["oc", "get", "pods"]) is None + with patch("factory.contained.k8s.subprocess.run", + side_effect=subprocess.TimeoutExpired(cmd="oc", timeout=1)): + assert k8s._run(["oc", "get", "pods"]) is None + + +def test_run_returns_the_completed_process_on_success() -> None: + with patch("factory.contained.k8s.subprocess.run", return_value=_completed("ok")): + result = k8s._run(["oc", "version"]) + assert result is not None and result.stdout == "ok" + + +# -------------------------------------------------------------------------------------------- +# Command composition — the pure argv builders +# -------------------------------------------------------------------------------------------- + + +def test_build_apply_argv_targets_stdin_in_the_namespace() -> None: + with patch("factory.contained.k8s.cli_binary", return_value="oc"): + assert k8s.build_apply_argv("ns") == ["oc", "apply", "-n", "ns", "-f", "-"] + + +def test_build_pod_exec_argv_uses_a_bare_i_without_a_tty() -> None: + with patch("factory.contained.k8s.cli_binary", return_value="oc"): + argv = k8s.build_pod_exec_argv("pod", "ns", ["ls"], tty=False) + assert "-i" in argv + assert "-t" not in argv + assert argv[-2:] == ["--", "ls"] + + +def test_render_access_review_carries_an_explicit_api_group() -> None: + review = json.loads( + k8s.render_access_review("create", "builds", "ns", group="build.openshift.io") + ) + assert review["spec"]["resourceAttributes"]["group"] == "build.openshift.io" + + +# -------------------------------------------------------------------------------------------- +# access_review — the "denied vs could-not-find-out" split +# -------------------------------------------------------------------------------------------- + + +def test_access_review_is_none_when_the_review_command_fails() -> None: + with patch("factory.contained.k8s.subprocess.run", + return_value=_completed("", 1, stderr="boom")): + assert k8s.access_review("create", "pods", "ns") is None + + +# -------------------------------------------------------------------------------------------- +# namespace_fs_group — reading the OpenShift supplemental-groups range +# -------------------------------------------------------------------------------------------- + + +def test_namespace_fs_group_parses_the_range_start() -> None: + with patch("factory.contained.k8s.cli_binary", return_value="oc"), \ + patch("factory.contained.k8s._run", return_value=_completed("1000700000/10000\n")): + assert k8s.namespace_fs_group("ns") == 1000700000 + + +def test_namespace_fs_group_is_none_when_the_annotation_is_absent_or_junk() -> None: + with patch("factory.contained.k8s.cli_binary", return_value="oc"), \ + patch("factory.contained.k8s._run", return_value=None): + assert k8s.namespace_fs_group("ns") is None + with patch("factory.contained.k8s.cli_binary", return_value="oc"), \ + patch("factory.contained.k8s._run", return_value=_completed("", 1)): + assert k8s.namespace_fs_group("ns") is None + # Present but not an integer — plain Kubernetes has no such annotation. + with patch("factory.contained.k8s.cli_binary", return_value="oc"), \ + patch("factory.contained.k8s._run", return_value=_completed("not-a-number\n")): + assert k8s.namespace_fs_group("ns") is None + + +# -------------------------------------------------------------------------------------------- +# apply_manifest +# -------------------------------------------------------------------------------------------- + + +def test_apply_manifest_succeeds_quietly() -> None: + with patch("factory.contained.k8s.cli_binary", return_value="oc"), \ + patch("factory.contained.k8s.subprocess.run", return_value=_completed("configured")): + k8s.apply_manifest("kind: Pod\n", "ns") # no raise + + +def test_apply_manifest_raises_when_the_cli_cannot_be_run() -> None: + with patch("factory.contained.k8s.cli_binary", return_value="oc"), \ + patch("factory.contained.k8s.subprocess.run", side_effect=FileNotFoundError("oc")): + with pytest.raises(k8s.ClusterError, match="applying the manifest failed"): + k8s.apply_manifest("kind: Pod\n", "ns") + + +def test_apply_manifest_raises_on_a_nonzero_exit() -> None: + with patch("factory.contained.k8s.cli_binary", return_value="oc"), \ + patch("factory.contained.k8s.subprocess.run", + return_value=_completed("", 1, stderr="forbidden")): + with pytest.raises(k8s.ClusterError, match="forbidden"): + k8s.apply_manifest("kind: Pod\n", "ns") + + +# -------------------------------------------------------------------------------------------- +# wait_for_container — the polled loop, driven instantly by a patched clock +# -------------------------------------------------------------------------------------------- + + +def _pod_json(*, name: str = "factory", state: dict | None = None, phase: str = "Pending") -> str: + status: dict = {"phase": phase} + if state is not None: + status["containerStatuses"] = [{"name": name, "state": state}] + return json.dumps({"status": status}) + + +def test_wait_for_container_returns_running_when_the_container_is_up() -> None: + with patch("factory.contained.k8s.cli_binary", return_value="oc"), \ + patch("factory.contained.k8s._run", + return_value=_completed(_pod_json(state={"running": {}}))), \ + patch("time.sleep"), patch("time.monotonic", side_effect=[0, 1]): + assert k8s.wait_for_container("p", "ns", "factory") == "running" + + +def test_wait_for_container_returns_terminated_on_a_clean_exit() -> None: + with patch("factory.contained.k8s.cli_binary", return_value="oc"), \ + patch("factory.contained.k8s._run", + return_value=_completed(_pod_json(state={"terminated": {"exitCode": 0}}))), \ + patch("time.sleep"), patch("time.monotonic", side_effect=[0, 1]): + assert k8s.wait_for_container("p", "ns", "factory") == "terminated" + + +def test_wait_for_container_tolerates_a_missing_read_and_junk_json_then_succeeds() -> None: + """A dropped read and a half-written document are both "keep polling", not failures.""" + running = _completed(_pod_json(state={"running": {}})) + with patch("factory.contained.k8s.cli_binary", return_value="oc"), \ + patch("factory.contained.k8s._run", + side_effect=[None, _completed("not json"), running]), \ + patch("time.sleep") as slept, \ + patch("time.monotonic", side_effect=[0, 1, 2, 3]): + assert k8s.wait_for_container("p", "ns", "factory", timeout=100) == "running" + assert slept.call_count == 2 # once for the dropped read, once for the junk + + +def test_wait_for_container_times_out_without_a_last_state() -> None: + """When the deadline is already past, there is no last state to report — the 886 falsy branch.""" + with patch("factory.contained.k8s.cli_binary", return_value="oc"), \ + patch("time.sleep"), patch("time.monotonic", side_effect=[0, 0]): + with pytest.raises(k8s.ClusterError) as raised: + k8s.wait_for_container("p", "ns", "factory", timeout=0) + assert "last state" not in str(raised.value) + + +# -------------------------------------------------------------------------------------------- +# stream_workspace / fetch_workspace — the tarball transport +# -------------------------------------------------------------------------------------------- + + +def test_stream_workspace_succeeds(tmp_path: Path) -> None: + tarball = tmp_path / "ws.tar.gz" + tarball.write_bytes(b"payload") + with patch("factory.contained.k8s.cli_binary", return_value="oc"), \ + patch("factory.contained.k8s.subprocess.run", return_value=_completed("done")): + k8s.stream_workspace(tarball, "pod", "ns") # no raise + + +def test_stream_workspace_raises_with_a_retry_hint_on_failure(tmp_path: Path) -> None: + tarball = tmp_path / "ws.tar.gz" + tarball.write_bytes(b"payload") + with patch("factory.contained.k8s.cli_binary", return_value="oc"), \ + patch("factory.contained.k8s.subprocess.run", + return_value=_completed("", 1, stderr="broken pipe")): + with pytest.raises(k8s.ClusterError, match="retrying is safe"): + k8s.stream_workspace(tarball, "pod", "ns") + + +def test_fetch_workspace_succeeds(tmp_path: Path) -> None: + destination = tmp_path / "out.tar.gz" + with patch("factory.contained.k8s.cli_binary", return_value="oc"), \ + patch("factory.contained.k8s.subprocess.run", + return_value=subprocess.CompletedProcess([], 0, b"", b"")): + k8s.fetch_workspace("pod", "ns", destination) # no raise + assert destination.exists() + + +def test_fetch_workspace_raises_on_failure(tmp_path: Path) -> None: + destination = tmp_path / "out.tar.gz" + # stderr is bytes here — the call runs without text=True — so the error path must .decode() it. + with patch("factory.contained.k8s.cli_binary", return_value="oc"), \ + patch("factory.contained.k8s.subprocess.run", + return_value=subprocess.CompletedProcess([], 1, b"", b"no such pod")): + with pytest.raises(k8s.ClusterError, match="no such pod"): + k8s.fetch_workspace("pod", "ns", destination) + + +# -------------------------------------------------------------------------------------------- +# _summarize +# -------------------------------------------------------------------------------------------- + + +def test_summarize_returns_the_last_meaningful_line() -> None: + stderr = "E0812 noise line\nUnhandled Error in something\nerror: the real problem\n" + assert k8s._summarize(stderr) == "the real problem" + + +def test_summarize_reports_a_placeholder_when_there_is_nothing_useful() -> None: + assert k8s._summarize("E0812 only noise\n\n") == "no details given" + + +# -------------------------------------------------------------------------------------------- +# cluster_runtimes — the error surface +# -------------------------------------------------------------------------------------------- + + +def test_cluster_runtimes_raises_lifecycle_error_when_no_namespace_resolves() -> None: + with patch("factory.contained.k8s.current_namespace", return_value=None): + with pytest.raises(LifecycleError): + k8s.cluster_runtimes(None) + + +def test_cluster_runtimes_raises_when_the_cluster_does_not_answer() -> None: + with patch("factory.contained.k8s.cli_binary", return_value="oc"), \ + patch("factory.contained.k8s._run", return_value=None): + with pytest.raises(LifecycleError, match="did not answer"): + k8s.cluster_runtimes("ns") + + +def test_cluster_runtimes_summarizes_an_unreachable_cluster() -> None: + with patch("factory.contained.k8s.cli_binary", return_value="oc"), \ + patch("factory.contained.k8s._run", + return_value=_completed("", 1, stderr="error: Unauthorized")): + with pytest.raises(LifecycleError, match="cannot reach the cluster"): + k8s.cluster_runtimes("ns") + + +def test_cluster_runtimes_raises_on_non_json_output() -> None: + with patch("factory.contained.k8s.cli_binary", return_value="oc"), \ + patch("factory.contained.k8s._run", return_value=_completed("not json")): + with pytest.raises(LifecycleError, match="isn't JSON"): + k8s.cluster_runtimes("ns") + + +def test_cluster_runtimes_tolerates_missing_and_malformed_timestamps() -> None: + payload = json.dumps({"items": [ + {"metadata": {"name": "a", "labels": {}}, "status": {"phase": "Running"}}, + {"metadata": {"name": "b", "creationTimestamp": "not-a-date"}, + "status": {"phase": "Pending"}}, + ]}) + with patch("factory.contained.k8s.cli_binary", return_value="oc"), \ + patch("factory.contained.k8s._run", return_value=_completed(payload)): + runtimes = k8s.cluster_runtimes("ns") + assert [r.name for r in runtimes] == ["a", "b"] + assert all(r.created is None for r in runtimes) + + +# -------------------------------------------------------------------------------------------- +# remove_cluster_runtime — the failure paths +# -------------------------------------------------------------------------------------------- + + +def test_remove_cluster_runtime_reports_a_failed_delete( + capsys: pytest.CaptureFixture[str], +) -> None: + # Sweep could not be run (None), so the sweep-report block is skipped; the delete then fails. + with patch("factory.contained.k8s.cli_binary", return_value="oc"), \ + patch("factory.contained.k8s._run", + side_effect=[None, _completed("", 1, stderr="forbidden")]): + code = k8s.remove_cluster_runtime("rta-test", namespace="ns") + assert code == 1 + assert "deleting pod rta-test failed" in capsys.readouterr().err + + +def test_remove_cluster_runtime_reports_when_the_delete_cli_could_not_run( + capsys: pytest.CaptureFixture[str], +) -> None: + with patch("factory.contained.k8s.cli_binary", return_value="oc"), \ + patch("factory.contained.k8s._run", side_effect=[None, None]): + code = k8s.remove_cluster_runtime("rta-test", namespace="ns") + assert code == 1 + assert "the CLI could not be run" in capsys.readouterr().err + + +# -------------------------------------------------------------------------------------------- +# sync_cluster_runtime +# -------------------------------------------------------------------------------------------- + + +def test_sync_cluster_runtime_fetches_and_reports_where_it_landed( + tmp_path: Path, capsys: pytest.CaptureFixture[str], +) -> None: + with patch("factory.contained.workspace.contained_home", return_value=tmp_path), \ + patch("factory.contained.k8s.fetch_workspace") as fetch: + code = k8s.sync_cluster_runtime("rta-test", namespace="ns") + assert code == 0 + fetch.assert_called_once() + out = capsys.readouterr().out + assert "workspace fetched to" in out + assert "Nothing is merged automatically." in out + + +def test_sync_cluster_runtime_reports_a_fetch_failure( + tmp_path: Path, capsys: pytest.CaptureFixture[str], +) -> None: + with patch("factory.contained.workspace.contained_home", return_value=tmp_path), \ + patch("factory.contained.k8s.fetch_workspace", + side_effect=k8s.ClusterError("no such pod")): + code = k8s.sync_cluster_runtime("rta-test", namespace="ns") + assert code == 1 + assert "no such pod" in capsys.readouterr().err diff --git a/tests/test_contained_k8s_credentials_coverage.py b/tests/test_contained_k8s_credentials_coverage.py new file mode 100644 index 000000000..a9fef837d --- /dev/null +++ b/tests/test_contained_k8s_credentials_coverage.py @@ -0,0 +1,390 @@ +"""Exhaustive-branch coverage for the guided credentials Secret. + +`tests/test_contained_k8s_credentials.py` covers the disclosure guarantees — the tests worth having. +This file exists for a narrower, mechanical reason: every remaining statement and branch in +`factory.contained.k8s_credentials`. That means the shell-out failure arms, the "wrong source" +re-ask loops, each `validate_adc` verdict, and the whole `_copy_from_shell` shape reconstruction — +paths a real user hits rarely and a regression would hide in. + +The interactive surface is driven exactly as the sibling file drives it: injected `_Readers` and a +patched `style.confirm`, never a terminal. No real secret-looking value appears; dummy strings only. +""" + +from __future__ import annotations + +import json +import subprocess +from pathlib import Path +from unittest.mock import patch + +import pytest + +from factory.contained import k8s_credentials as creds +from factory.contained.credentials import CredentialShape +from factory.contained.k8s import ADC_SECRET_KEY, SECRET_NAME + +# Long enough to trip the excerpt floor and the redaction length guard, but obviously not a key. +DUMMY = "dummy-value-1234567890-abcdefgh" + + +def _completed(stdout: str = "", returncode: int = 0, stderr: str = ""): + return subprocess.CompletedProcess([], returncode, stdout, stderr) + + +class _Answers: + """A scripted stand-in for the three readers — copied from the sibling test file. + + `tests/conftest.py` forces raw reads off and pytest's stdin raises on `input()`, so a prompt + reached for real here would either block or abort. Injection is the only way in. + """ + + def __init__(self, *, selects: list[str], lines: list[str] | None = None, + secrets: list[str] | None = None) -> None: + self.selects = list(selects) + self.lines = list(lines or []) + self.secrets = list(secrets or []) + + def readers(self) -> creds._Readers: + return creds._Readers( + line=lambda question, default=None: self.lines.pop(0) if self.lines else default, + secret=lambda question: self.secrets.pop(0) if self.secrets else None, + select=lambda question, options: self.selects.pop(0) if self.selects else None, + ) + + +# -------------------------------------------------------------------------------------------- +# _run — the shell-out that swallows a missing binary +# -------------------------------------------------------------------------------------------- + + +def test_run_returns_none_when_the_binary_is_absent() -> None: + """A missing `oc` must read as "could not check", not crash the whole setup wizard.""" + with patch("factory.contained.k8s_credentials.subprocess.run", + side_effect=FileNotFoundError("no oc")): + assert creds._run(["oc", "get"]) is None + + +# -------------------------------------------------------------------------------------------- +# Reading what is there +# -------------------------------------------------------------------------------------------- + + +def test_a_secret_with_no_recognised_backend_is_reported_as_such() -> None: + """It exists, but nothing in it can authenticate — the check has to say which state that is.""" + payload = json.dumps({"SOMETHING_ELSE": "x", "ANOTHER": "y"}) + with patch("factory.contained.k8s_credentials._run", return_value=_completed(payload)): + check = creds.secret_check("oc", "ns") + assert check.ok is False + assert "none of the supported backends" in check.detail + assert "ANOTHER" in check.detail # the keys it does carry are named + + +def test_keys_of_treats_unparseable_and_non_object_json_as_empty() -> None: + assert creds._keys_of("not json at all") == set() # JSONDecodeError arm + assert creds._keys_of("[1, 2, 3]") == set() # valid JSON, but not a dict + assert creds._keys_of('{"A": 1}') == {"A"} + + +def test_secret_exists_mirrors_the_get_return_code() -> None: + with patch("factory.contained.k8s_credentials._run", return_value=_completed(returncode=0)): + assert creds.secret_exists("oc", "ns") is True + with patch("factory.contained.k8s_credentials._run", return_value=_completed(returncode=1)): + assert creds.secret_exists("oc", "ns") is False + with patch("factory.contained.k8s_credentials._run", return_value=None): + assert creds.secret_exists("oc", "ns") is False + + +# -------------------------------------------------------------------------------------------- +# Describing and redacting +# -------------------------------------------------------------------------------------------- + + +def test_a_long_value_is_described_with_both_ends() -> None: + described = creds.describe_value(DUMMY) + assert str(len(DUMMY)) in described + assert "starts" in described and "ends" in described + assert DUMMY not in described # only the sanctioned excerpts, not the whole thing + + +def test_redact_skips_empty_and_too_short_values() -> None: + """The length guard exists so a two-character value cannot turn every 'ab' into '***'.""" + text = "keep ab but scrub the-longer-value here" + scrubbed = creds.redact(text, ("", "ab", "the-longer-value")) + assert "ab but" in scrubbed # short value left alone (the `>= 4` false branch) + assert "the-longer-value" not in scrubbed + assert "***" in scrubbed + + +# -------------------------------------------------------------------------------------------- +# apply_secret — the exception arm +# -------------------------------------------------------------------------------------------- + + +def test_apply_secret_never_raises_and_redacts_the_exception() -> None: + """A subprocess failure is a thing to report; the value must not ride out on the message.""" + with patch("factory.contained.k8s_credentials.subprocess.run", + side_effect=OSError(f"boom near {DUMMY}")): + ok, detail = creds.apply_secret("oc", "ns", {"ANTHROPIC_API_KEY": DUMMY}) + assert ok is False + assert DUMMY not in detail + assert "OSError" in detail + + +# -------------------------------------------------------------------------------------------- +# _collect_field — the back-out arms +# -------------------------------------------------------------------------------------------- + + +def test_a_plain_field_backs_out_when_the_line_reader_returns_none() -> None: + field = creds.Field(key="CLOUD_ML_REGION", question="region") + readers = creds._Readers(line=lambda question, default=None: None) + assert creds._collect_field(field, readers) is None + + +def test_a_material_field_backs_out_when_the_source_menu_is_cancelled() -> None: + readers = _Answers(selects=["q"]).readers() + assert creds._collect_field(creds.ANTHROPIC_FIELDS[0], readers) is None + + +# -------------------------------------------------------------------------------------------- +# _read_from_source / _from_environment / _from_file +# -------------------------------------------------------------------------------------------- + + +def test_typing_a_json_field_prints_the_paste_hint_and_backs_out_on_empty( + capsys: pytest.CaptureFixture[str], +) -> None: + """The ADC field's `json_template` triggers the 'paste or choose the file' note before reading.""" + adc_field = creds.VERTEX_FIELDS[-1] + assert adc_field.json_template # guard: this is the field with the hint + readers = _Answers(selects=[], secrets=[""]).readers() # types nothing + assert creds._read_from_source("t", adc_field, readers) is None + assert "Paste the file's contents" in capsys.readouterr().out + + +def test_environment_source_backs_out_when_the_name_reader_returns_none() -> None: + field = creds.ANTHROPIC_FIELDS[0] + readers = creds._Readers(line=lambda question, default=None: None) + assert creds._from_environment(field, readers) is None + + +def test_environment_source_backs_out_when_no_name_is_given() -> None: + """A field with no suggested variable and a blank answer has nothing to read.""" + field = creds.Field(key="CUSTOM", question="custom", material=True) # default_env=() + readers = creds._Readers(line=lambda question, default=None: "") + assert creds._from_environment(field, readers) is None + + +def test_file_source_reads_an_unvalidated_file_for_a_plain_material_field( + tmp_path: Path, +) -> None: + """The API-key field has no `json_template` and no `validate_json` — both false branches.""" + blob = tmp_path / "key.txt" + blob.write_text(DUMMY) + readers = _Answers(selects=[], lines=[str(blob)]).readers() + with patch("factory.contained.k8s_credentials.style.confirm", return_value=True): + value = creds._from_file(creds.ANTHROPIC_FIELDS[0], readers) + assert value == DUMMY + + +def test_file_source_backs_out_when_the_path_reader_returns_none() -> None: + readers = creds._Readers(line=lambda question, default=None: None) + assert creds._from_file(creds.ANTHROPIC_FIELDS[0], readers) is None + + +def test_file_source_reports_an_unreadable_file( + tmp_path: Path, capsys: pytest.CaptureFixture[str], +) -> None: + missing = tmp_path / "does-not-exist.txt" + readers = _Answers(selects=[], lines=[str(missing)]).readers() + assert creds._from_file(creds.ANTHROPIC_FIELDS[0], readers) is None + assert "Could not read" in capsys.readouterr().out + + +def test_file_source_prints_the_template_and_refuses_an_invalid_adc( + tmp_path: Path, capsys: pytest.CaptureFixture[str], +) -> None: + """The ADC field prints its required-shape template, then rejects a bad document before upload.""" + bad = tmp_path / "adc.json" + bad.write_text(json.dumps({"type": "authorized_user"})) # missing fields + readers = _Answers(selects=[], lines=[str(bad)]).readers() + assert creds._from_file(creds.VERTEX_FIELDS[-1], readers) is None + out = capsys.readouterr().out + assert "This file must contain" in out # the template, shown before the question + assert "is not usable" in out # the validation refusal + + +# -------------------------------------------------------------------------------------------- +# validate_adc — every return arm +# -------------------------------------------------------------------------------------------- + + +def test_validate_adc_rejects_non_object_json() -> None: + problem = creds.validate_adc("123") + assert problem is not None and "not an object" in problem + + +# -------------------------------------------------------------------------------------------- +# _confirm_value — the non-material passthrough +# -------------------------------------------------------------------------------------------- + + +def test_confirm_value_returns_configuration_without_asking() -> None: + """A non-material value is configuration; it is returned as-is, no confirmation prompt.""" + field = creds.Field(key="CLOUD_ML_REGION", question="region") # material=False + assert creds._confirm_value(field, "us-east5", creds._Readers()) == "us-east5" + + +def test_confirm_value_honours_a_no_for_material() -> None: + field = creds.ANTHROPIC_FIELDS[0] + with patch("factory.contained.k8s_credentials.style.confirm", return_value=False): + assert creds._confirm_value(field, DUMMY, creds._Readers()) is None + + +# -------------------------------------------------------------------------------------------- +# run_credentials_step — the branches the happy-path tests skip +# -------------------------------------------------------------------------------------------- + + +def test_an_unrecognised_existing_secret_warns_that_it_will_be_replaced( + capsys: pytest.CaptureFixture[str], +) -> None: + """secret_check fails but the Secret object is there — the note has to say it gets replaced.""" + answers = _Answers(selects=["s"]) # then skip, to end the flow quickly + missing = creds.Check("credentials_secret", False, "wrong keys", fix="...") + with patch("factory.contained.k8s_credentials.secret_check", return_value=missing), \ + patch("factory.contained.k8s_credentials.secret_exists", return_value=True): + created = creds.run_credentials_step( + "oc", "ns", interactive=True, readers=answers.readers() + ) + assert created is False + assert "Continuing replaces it" in capsys.readouterr().out + + +def test_declining_the_final_confirm_creates_nothing( + monkeypatch, capsys: pytest.CaptureFixture[str], +) -> None: + """Backend chosen, then 'Create it now?' answered no — apply must not run.""" + monkeypatch.setenv("ANTHROPIC_API_KEY", DUMMY) + shape = CredentialShape(backend="anthropic", ok=True, detail="from key") + missing = creds.Check("credentials_secret", False, "missing", fix="...") + with patch("factory.contained.k8s_credentials.secret_check", return_value=missing), \ + patch("factory.contained.k8s_credentials.secret_exists", return_value=False), \ + patch("factory.contained.k8s_credentials.resolve_credentials", return_value=shape), \ + patch("factory.contained.k8s_credentials.style.confirm", return_value=False), \ + patch("factory.contained.k8s_credentials.apply_secret") as apply: + # select "3": copy from this shell, so no per-value confirm intervenes before the final one. + created = creds.run_credentials_step( + "oc", "ns", interactive=True, readers=_Answers(selects=["3"]).readers() + ) + assert created is False + apply.assert_not_called() + assert "Nothing was created" in capsys.readouterr().out + + +def test_a_failed_apply_is_reported_and_returns_false( + monkeypatch, capsys: pytest.CaptureFixture[str], +) -> None: + monkeypatch.setenv("ANTHROPIC_API_KEY", DUMMY) + shape = CredentialShape(backend="anthropic", ok=True, detail="from key") + missing = creds.Check("credentials_secret", False, "missing", fix="...") + with patch("factory.contained.k8s_credentials.secret_check", return_value=missing), \ + patch("factory.contained.k8s_credentials.secret_exists", return_value=False), \ + patch("factory.contained.k8s_credentials.resolve_credentials", return_value=shape), \ + patch("factory.contained.k8s_credentials.style.confirm", return_value=True), \ + patch("factory.contained.k8s_credentials.apply_secret", + return_value=(False, "forbidden")): + created = creds.run_credentials_step( + "oc", "ns", interactive=True, readers=_Answers(selects=["3"]).readers() + ) + assert created is False + assert "Could not create the Secret: forbidden" in capsys.readouterr().out + + +# -------------------------------------------------------------------------------------------- +# _choose_backend — the copy-from-shell pick and the collect back-out +# -------------------------------------------------------------------------------------------- + + +def test_choose_backend_offers_and_routes_copy_from_shell(monkeypatch) -> None: + """When the shell already resolves a backend, option 3 appears and dispatches to the copy.""" + monkeypatch.setenv("ANTHROPIC_API_KEY", DUMMY) + shape = CredentialShape(backend="anthropic", ok=True, detail="from key") + with patch("factory.contained.k8s_credentials.resolve_credentials", return_value=shape): + data = creds._choose_backend(_Answers(selects=["3"]).readers()) + assert data == {"ANTHROPIC_API_KEY": DUMMY} + + +def test_choose_backend_aborts_when_a_field_is_abandoned() -> None: + """Pick Anthropic, then cancel the key's source menu — the whole choice returns None.""" + answers = _Answers(selects=["1", "q"]) + data = creds._choose_backend(answers.readers()) + assert data is None + + +# -------------------------------------------------------------------------------------------- +# _copy_from_shell — every arm +# -------------------------------------------------------------------------------------------- + + +def test_copy_from_shell_anthropic_present_and_absent(monkeypatch) -> None: + monkeypatch.setenv("ANTHROPIC_API_KEY", DUMMY) + assert creds._copy_from_shell("anthropic") == {"ANTHROPIC_API_KEY": DUMMY} + monkeypatch.delenv("ANTHROPIC_API_KEY", raising=False) + assert creds._copy_from_shell("anthropic") is None + + +def test_copy_from_shell_vertex_incomplete_config_asks_instead( + monkeypatch, capsys: pytest.CaptureFixture[str], +) -> None: + monkeypatch.setenv("CLAUDE_CODE_USE_VERTEX", "1") + monkeypatch.delenv("CLOUD_ML_REGION", raising=False) + monkeypatch.delenv("ANTHROPIC_VERTEX_PROJECT_ID", raising=False) + assert creds._copy_from_shell("vertex") is None + assert "configuration is incomplete" in capsys.readouterr().out + + +def _complete_vertex_env(monkeypatch) -> None: + monkeypatch.setenv("CLAUDE_CODE_USE_VERTEX", "1") + monkeypatch.setenv("CLOUD_ML_REGION", "us-east5") + monkeypatch.setenv("ANTHROPIC_VERTEX_PROJECT_ID", "my-project") + + +def test_copy_from_shell_vertex_unreadable_adc( + monkeypatch, tmp_path: Path, capsys: pytest.CaptureFixture[str], +) -> None: + _complete_vertex_env(monkeypatch) + monkeypatch.setattr(creds, "ADC_DIR", tmp_path) + monkeypatch.setattr(creds, "ADC_FILE", "absent.json") # never created + assert creds._copy_from_shell("vertex") is None + out = capsys.readouterr().out + assert "Could not read" in out and "application-default login" in out + + +def test_copy_from_shell_vertex_invalid_adc( + monkeypatch, tmp_path: Path, capsys: pytest.CaptureFixture[str], +) -> None: + _complete_vertex_env(monkeypatch) + (tmp_path / "adc.json").write_text("not json") + monkeypatch.setattr(creds, "ADC_DIR", tmp_path) + monkeypatch.setattr(creds, "ADC_FILE", "adc.json") + assert creds._copy_from_shell("vertex") is None + assert "is not usable" in capsys.readouterr().out + + +def test_copy_from_shell_vertex_ok_carries_config_credential_and_pinned_env( + monkeypatch, tmp_path: Path, +) -> None: + _complete_vertex_env(monkeypatch) + document = json.dumps({ + "type": "authorized_user", "client_id": "a", "client_secret": "b", "refresh_token": "c", + }) + (tmp_path / "adc.json").write_text(document) + monkeypatch.setattr(creds, "ADC_DIR", tmp_path) + monkeypatch.setattr(creds, "ADC_FILE", "adc.json") + data = creds._copy_from_shell("vertex") + assert data is not None + assert data["CLOUD_ML_REGION"] == "us-east5" + assert data[ADC_SECRET_KEY] == document + assert data["MAX_THINKING_TOKENS"] == "0" # the pinned Vertex setting merged in + assert SECRET_NAME # sanity: import stays used diff --git a/tests/test_contained_k8s_helpers.py b/tests/test_contained_k8s_helpers.py index 79085cead..3f1fd6ae7 100644 --- a/tests/test_contained_k8s_helpers.py +++ b/tests/test_contained_k8s_helpers.py @@ -338,3 +338,74 @@ def fake_run(argv, **kw): with patch("factory.contained.k8s._run", side_effect=fake_run): k8s.login_status("kubectl") assert "auth" in seen["argv"] and "can-i" in seen["argv"] + + +# -------------------------------------------------------------------------------------------- +# classify_pod / _unschedulable / login_status / poll_pod — the residual edge branches +# -------------------------------------------------------------------------------------------- + + +def test_a_status_that_is_not_a_mapping_is_treated_as_still_waiting() -> None: + """A half-written pod document (`status` a string, not an object) must not raise in a poll.""" + assert k8s.classify_pod({"status": "corrupt"}).verdict == k8s.WAITING + + +def test_a_pod_level_succeeded_with_no_container_status_is_succeeded() -> None: + """The probe pod is gone by the time it Succeeds — only the pod phase remains to read.""" + assert k8s.classify_pod({"status": {"phase": "Succeeded"}}).verdict == k8s.SUCCEEDED + + +def test_a_pod_level_failed_with_no_container_status_is_doomed() -> None: + prog = k8s.classify_pod({"status": {"phase": "Failed", "reason": "Evicted", "message": "oom"}}) + assert prog.verdict == k8s.DOOMED + assert "oom" in prog.describe() + + +def test_a_container_state_matching_nothing_falls_through_to_the_pod_level() -> None: + """An empty container state is neither running, waiting nor terminated — keep waiting.""" + pod = {"status": {"phase": "Pending", "containerStatuses": [{"name": "c", "state": {}}]}} + assert k8s.classify_pod(pod).verdict == k8s.WAITING + + +def test_unschedulable_skips_a_non_dict_condition_and_reads_the_real_one() -> None: + pod = {"status": {"phase": "Pending", "conditions": [ + "not-a-dict", + {"type": "PodScheduled", "status": "False", "reason": "Unschedulable", "message": "no room"}, + ]}} + prog = k8s.classify_pod(pod) + assert prog.verdict == k8s.DOOMED and "no room" in prog.describe() + + +def test_a_non_matching_condition_leaves_the_pod_merely_waiting() -> None: + """A `Ready=False` condition is not `Unschedulable`; the pod is still just starting.""" + pod = {"status": {"phase": "Pending", "conditions": [{"type": "Ready", "status": "False"}]}} + assert k8s.classify_pod(pod).verdict == k8s.WAITING + + +def test_login_status_reports_authenticated_even_when_whoami_prints_nothing() -> None: + """A 0 exit with empty stdout is still success — the detail is just blank.""" + with patch("factory.contained.k8s._run", + return_value=subprocess.CompletedProcess([], 0, "", "")): + ok, detail = k8s.login_status("oc") + assert ok is True and detail == "" + + +def test_polling_gives_up_on_a_retryable_error_that_never_clears() -> None: + """`ErrImagePull` might be a blip; if it is still there after `stuck_after`, stop waiting.""" + stuck = _pod(waiting={"reason": "ErrImagePull", "message": "pull failed"}) + # A monotonic clock that advances past stuck_after between the two readings. Base is non-zero so + # the first timestamp stored in `error_since` is truthy (0.0 would re-trigger the `or`). + clock = iter([1000, 1000, 1000, 1000, 1035, 1035, 1035, 1035]) + with patch("factory.contained.k8s.read_pod", return_value=stuck), \ + patch("factory.contained.k8s.time.sleep"), \ + patch("factory.contained.k8s.time.monotonic", lambda: next(clock)): + prog = k8s.poll_pod("p", "ns", timeout=300, stuck_after=30) + assert prog.verdict == k8s.DOOMED + assert "unchanged for 30s" in prog.describe() + + +def test_login_status_when_the_cli_cannot_be_run_at_all() -> None: + """`_run` returns None when the binary is missing or the OS refuses — reported, not raised.""" + with patch("factory.contained.k8s._run", return_value=None): + ok, detail = k8s.login_status("oc") + assert ok is False and "could not be run" in detail diff --git a/tests/test_contained_k8s_setup_coverage.py b/tests/test_contained_k8s_setup_coverage.py new file mode 100644 index 000000000..e3550376e --- /dev/null +++ b/tests/test_contained_k8s_setup_coverage.py @@ -0,0 +1,607 @@ +"""Coverage for the failure, degradation and interactive branches of `k8s_setup`. + +Written against the real branch source (the 4-step wizard with the login gate, the `poll_pod`-based +inference probe, and the credentials step). Everything that would reach a cluster or a prompt is +mocked at the module boundary — a test that reached either would hang, since `conftest` forces the +raw-terminal path off and there is no cluster to answer. +""" + +from __future__ import annotations + +import subprocess +from unittest.mock import patch + +import pytest + +from factory.contained import k8s_setup +from factory.contained.k8s import ( + DOOMED, + SUCCEEDED, + WAITING, + ClusterContext, + ClusterError, + PodProgress, +) +from factory.contained.prereq import Check + + +def _completed(stdout: str = "", returncode: int = 0, stderr: str = "") -> subprocess.CompletedProcess: + return subprocess.CompletedProcess([], returncode, stdout, stderr) + + +@pytest.fixture(autouse=True) +def _off_cluster(): + """Safe defaults so nothing reaches a cluster or a prompt; individual tests override as needed. + + Mirrors the intent of `test_contained_k8s.py`'s autouse fixtures, which this file does not + inherit. Also clears the process-global pinned context that `setup_k8s` sets, so one test's + choice does not leak into the next. + """ + k8s_setup.set_active_context(None) + with patch("factory.contained.k8s_setup.cli_binary", return_value="oc"), \ + patch("factory.contained.k8s_setup.list_contexts", return_value=[]), \ + patch("factory.contained.k8s_setup.cluster_context", return_value=ClusterContext()), \ + patch("factory.contained.k8s_setup.current_namespace", return_value=None), \ + patch("factory.contained.k8s_setup.access_review", return_value=True), \ + patch("factory.contained.k8s_setup.gitleaks_available", return_value=True), \ + patch("factory.contained.k8s_setup.resolve_image", return_value="img:latest"): + yield + k8s_setup.set_active_context(None) + + +# --------------------------------------------------------------------------------------------- +# _run +# --------------------------------------------------------------------------------------------- + + +def test_run_swallows_a_launch_failure() -> None: + with patch("factory.contained.k8s_setup.subprocess.run", side_effect=OSError("boom")): + assert k8s_setup._run(["oc", "version"]) is None + + +# --------------------------------------------------------------------------------------------- +# verify_k8s — the early returns and the division branch +# --------------------------------------------------------------------------------------------- + + +def test_verify_stops_when_no_cli_is_installed() -> None: + with patch("factory.contained.k8s_setup.cli_binary", + side_effect=ClusterError("neither oc nor kubectl")): + checks = k8s_setup.verify_k8s(namespace="ns") + assert [c.name for c in checks] == ["cluster_cli"] + assert not checks[0].ok + + +def test_verify_stops_when_the_login_has_expired() -> None: + """A bad credential must halt before the object checks turn Unauthorized into "missing".""" + with patch("factory.contained.k8s_setup._run", return_value=_completed("ctx")), \ + patch("factory.contained.k8s_setup.cluster_context", + return_value=ClusterContext(context="c", server="https://s")), \ + patch("factory.contained.k8s_setup.login_status", + return_value=(False, "Unauthorized")): + checks = k8s_setup.verify_k8s(namespace="ns") + assert [c.name for c in checks] == ["cluster_cli", "cluster_login"] + assert not checks[-1].ok + + +def test_verify_stops_when_the_namespace_cannot_be_resolved() -> None: + with patch("factory.contained.k8s_setup._run", return_value=_completed("ctx")), \ + patch("factory.contained.k8s_setup.cluster_context", + return_value=ClusterContext(context="c")), \ + patch("factory.contained.k8s_setup.login_status", return_value=(True, "me")), \ + patch("factory.contained.k8s_setup.resolve_namespace", + side_effect=ClusterError("no namespace")): + checks = k8s_setup.verify_k8s(namespace=None) + assert checks[-1].name == "namespace" and not checks[-1].ok + + +def test_verify_runs_the_division_check_when_asked() -> None: + with patch("factory.contained.k8s_setup._run", return_value=_completed("ok")), \ + patch("factory.contained.k8s_setup.cluster_context", + return_value=ClusterContext(context="c")), \ + patch("factory.contained.k8s_setup.login_status", return_value=(True, "me")), \ + patch("factory.contained.k8s_setup.resolve_namespace", return_value="ns"), \ + patch("factory.contained.k8s_setup.secret_check", + return_value=Check("credentials_secret", False, "missing", fix="x")), \ + patch("factory.contained.k8s_setup.build_api_resources_argv", return_value=["oc"]): + checks = k8s_setup.verify_k8s(namespace="ns", division=True, probe_inference=False) + assert any(c.name == "build_api" for c in checks) + + +# --------------------------------------------------------------------------------------------- +# _login_check +# --------------------------------------------------------------------------------------------- + + +def test_login_check_authenticated() -> None: + with patch("factory.contained.k8s_setup.login_status", return_value=(True, "me@example.com")): + check = k8s_setup._login_check("oc") + assert check.ok and "me@example.com" in check.detail + + +def test_login_check_expired_session_reads_as_a_login_problem() -> None: + with patch("factory.contained.k8s_setup.login_status", + return_value=(False, "You must be logged in (Unauthorized)")): + check = k8s_setup._login_check("oc") + assert not check.ok and "expired" in check.detail + + +def test_login_check_other_failure_reports_the_detail() -> None: + with patch("factory.contained.k8s_setup.login_status", + return_value=(False, "connection refused")): + check = k8s_setup._login_check("oc") + assert not check.ok and "connection refused" in check.detail + + +# --------------------------------------------------------------------------------------------- +# _object_checks — a failed read is not absence +# --------------------------------------------------------------------------------------------- + + +def test_object_check_present() -> None: + with patch("factory.contained.k8s_setup._run", return_value=_completed("serviceaccount/factory")): + checks = k8s_setup._object_checks("oc", "ns", division=False) + assert all(c.ok for c in checks) + + +def test_object_check_genuinely_missing_points_at_the_bundle() -> None: + not_found = _completed(returncode=1, stderr='Error (NotFound): serviceaccounts "x" not found') + with patch("factory.contained.k8s_setup._run", return_value=not_found): + checks = k8s_setup._object_checks("oc", "ns", division=False) + assert not checks[0].ok and "is missing" in checks[0].detail + assert "bundle |" in (checks[0].fix or "") + + +def test_object_check_auth_error_is_unknown_and_says_log_in() -> None: + with patch("factory.contained.k8s_setup._run", + return_value=_completed(returncode=1, stderr="error: Unauthorized")): + checks = k8s_setup._object_checks("oc", "ns", division=False) + assert not checks[0].ok and "could not be checked" in checks[0].detail + assert "login" in (checks[0].fix or "") + + +def test_object_check_other_error_carries_its_reason() -> None: + with patch("factory.contained.k8s_setup._run", + return_value=_completed(returncode=1, stderr="Error (Forbidden): nope")): + checks = k8s_setup._object_checks("oc", "ns", division=False) + assert not checks[0].ok and "Forbidden" in checks[0].detail + + +def test_object_check_unreadable_when_the_cli_could_not_run() -> None: + with patch("factory.contained.k8s_setup._run", return_value=None): + checks = k8s_setup._object_checks("oc", "ns", division=False) + assert not checks[0].ok and "could not be checked" in checks[0].detail + + +# --------------------------------------------------------------------------------------------- +# _inference_result and the probe +# --------------------------------------------------------------------------------------------- + + +def test_inference_result_skips_the_probe_without_a_secret() -> None: + secret = Check("credentials_secret", False, "missing", fix="make it") + check = k8s_setup._inference_result("oc", "ns", secret, announce=False) + assert not check.ok and "not attempted" in check.detail and check.fix == "make it" + + +def test_inference_result_announces_then_probes() -> None: + secret = Check("credentials_secret", True, "present") + with patch("factory.contained.k8s_setup._inference_check", + return_value=Check("inference_from_cluster", True, "reached")) as probe: + check = k8s_setup._inference_result("oc", "ns", secret, announce=True) + probe.assert_called_once() + assert check.ok + + +def _probe_run(**verdicts): + """A subprocess.run stand-in for the probe: delete/apply/logs keyed off argv.""" + def run(argv, **kw): + if "apply" in argv: + return _completed(verdicts.get("apply_out", "created"), + returncode=verdicts.get("apply_rc", 0), + stderr=verdicts.get("apply_err", "")) + if "logs" in argv: + return _completed(verdicts.get("logs", "")) + return _completed("") # delete, cleanup + return run + + +def test_probe_reports_success_when_the_pod_reaches_inference() -> None: + with patch("factory.contained.k8s_setup.subprocess.run", side_effect=_probe_run(logs="PROBE_OK")), \ + patch("factory.contained.k8s_setup.poll_pod", + return_value=PodProgress(SUCCEEDED, "Succeeded", "Succeeded", "")): + check = k8s_setup._inference_check("oc", "ns", "img") + assert check.ok + + +def test_probe_reports_a_pod_that_could_not_be_created() -> None: + with patch("factory.contained.k8s_setup.subprocess.run", + side_effect=_probe_run(apply_rc=1, apply_err="quota exceeded")): + check = k8s_setup._inference_check("oc", "ns", "img") + assert not check.ok and "could not be created" in check.detail + + +def test_probe_reports_a_doomed_pod_with_the_kubelet_reason() -> None: + with patch("factory.contained.k8s_setup.subprocess.run", side_effect=_probe_run(logs="")), \ + patch("factory.contained.k8s_setup.poll_pod", + return_value=PodProgress(DOOMED, "Pending", "ImagePullBackOff", "no such image")): + check = k8s_setup._inference_check("oc", "ns", "img") + assert not check.ok and "ImagePullBackOff" in check.detail + + +def test_probe_reports_the_pods_last_line_when_it_ran_but_failed() -> None: + with patch("factory.contained.k8s_setup.subprocess.run", + side_effect=_probe_run(logs="probing...\nno response — DNS")), \ + patch("factory.contained.k8s_setup.poll_pod", + return_value=PodProgress(SUCCEEDED, "Succeeded", "Succeeded", "")): + check = k8s_setup._inference_check("oc", "ns", "img") + assert not check.ok and "no response" in check.detail + + +def test_probe_reports_no_output_when_neither_logs_nor_a_doomed_reason() -> None: + with patch("factory.contained.k8s_setup.subprocess.run", side_effect=_probe_run(logs="")), \ + patch("factory.contained.k8s_setup.poll_pod", + return_value=PodProgress(WAITING, "P", "P", "")): + check = k8s_setup._inference_check("oc", "ns", "img") + assert not check.ok and "no output" in check.detail + + +def test_probe_survives_a_subprocess_failure() -> None: + """An error mid-probe is caught; the `finally` cleanup still runs, so it must not itself raise.""" + def run(argv, **kw): + if "apply" in argv: + raise OSError("no oc") + return _completed("") # delete before, and the finally cleanup after + + with patch("factory.contained.k8s_setup.subprocess.run", side_effect=run): + check = k8s_setup._inference_check("oc", "ns", "img") + assert not check.ok and "could not be run" in check.detail + + +def test_probe_manifest_names_the_pod_and_namespace() -> None: + manifest = k8s_setup._probe_pod_manifest("probe-x", "ns", "img:1") + assert "probe-x" in manifest and "ns" in manifest and "img:1" in manifest + + +# --------------------------------------------------------------------------------------------- +# _division_checks +# --------------------------------------------------------------------------------------------- + + +def test_division_present_and_absent() -> None: + with patch("factory.contained.k8s_setup._run", return_value=_completed("builds\n")): + assert k8s_setup._division_checks("ns")[0].ok + with patch("factory.contained.k8s_setup._run", return_value=_completed("")): + assert not k8s_setup._division_checks("ns")[0].ok + + +# --------------------------------------------------------------------------------------------- +# _apply_object +# --------------------------------------------------------------------------------------------- + + +def test_apply_object_success_failure_and_exception() -> None: + from factory.contained.bundle import BundleObject + + obj = BundleObject(kind="role", name="factory", purpose="p", manifest="kind: Role\n") + with patch("factory.contained.k8s_setup.subprocess.run", return_value=_completed("configured")): + assert k8s_setup._apply_object(obj, "ns", "oc") == (True, "configured") + with patch("factory.contained.k8s_setup.subprocess.run", + return_value=_completed(returncode=1, stderr="forbidden")): + ok, detail = k8s_setup._apply_object(obj, "ns", "oc") + assert not ok and "forbidden" in detail + with patch("factory.contained.k8s_setup.subprocess.run", side_effect=OSError("gone")): + ok, detail = k8s_setup._apply_object(obj, "ns", "oc") + assert not ok and "OSError" in detail + + +# --------------------------------------------------------------------------------------------- +# setup_k8s — the top-level flow +# --------------------------------------------------------------------------------------------- + + +def test_setup_returns_2_when_no_cli() -> None: + with patch("factory.contained.k8s_setup.cli_binary", side_effect=ClusterError("none")): + assert k8s_setup.setup_k8s(namespace="ns", division=False, interactive=True) == 2 + + +def test_setup_aborts_when_the_context_chooser_is_escaped() -> None: + with patch("factory.contained.k8s_setup._choose_context", return_value=k8s_setup._ABORT): + assert k8s_setup.setup_k8s(namespace="ns", division=False, interactive=True) == 1 + + +def test_setup_returns_2_when_the_namespace_lookup_errors() -> None: + with patch("factory.contained.k8s_setup._choose_context", return_value="ctx"), \ + patch("factory.contained.k8s_setup._choose_namespace", + side_effect=ClusterError("boom")): + assert k8s_setup.setup_k8s(namespace="ns", division=False, interactive=True) == 2 + + +def test_setup_aborts_when_no_namespace_is_chosen() -> None: + with patch("factory.contained.k8s_setup._choose_context", return_value=None), \ + patch("factory.contained.k8s_setup._choose_namespace", return_value=None): + assert k8s_setup.setup_k8s(namespace=None, division=False, interactive=True) == 1 + + +def test_setup_stops_when_not_logged_in() -> None: + with patch("factory.contained.k8s_setup._choose_context", return_value=None), \ + patch("factory.contained.k8s_setup._choose_namespace", return_value="ns"), \ + patch("factory.contained.k8s_setup.login_status", return_value=(False, "Unauthorized")): + assert k8s_setup.setup_k8s(namespace="ns", division=False, interactive=True) == 1 + + +def test_setup_non_interactive_without_yes_applies_nothing() -> None: + with patch("factory.contained.k8s_setup._choose_context", return_value=None), \ + patch("factory.contained.k8s_setup._choose_namespace", return_value="ns"), \ + patch("factory.contained.k8s_setup.login_status", return_value=(True, "me")), \ + patch("factory.contained.k8s_setup.inspect_objects", return_value=[]): + assert k8s_setup.setup_k8s(namespace="ns", division=False, interactive=False) == 1 + + +def _walk_result(*, failed=False, aborted=False): + class _R: + pass + r = _R() + r.failed, r.aborted = failed, aborted + return r + + +def test_setup_walk_aborted_names_the_verify_command() -> None: + with patch("factory.contained.k8s_setup._choose_context", return_value=None), \ + patch("factory.contained.k8s_setup._choose_namespace", return_value="ns"), \ + patch("factory.contained.k8s_setup.login_status", return_value=(True, "me")), \ + patch("factory.contained.k8s_setup.inspect_objects", return_value=[]), \ + patch("factory.contained.k8s_setup.walk", return_value=_walk_result(aborted=True)): + assert k8s_setup.setup_k8s(namespace="ns", division=False, interactive=True) == 1 + + +def test_setup_full_success_runs_credentials_then_verify() -> None: + ok = [Check("cluster_cli", True, "x")] + with patch("factory.contained.k8s_setup._choose_context", return_value=None), \ + patch("factory.contained.k8s_setup._choose_namespace", return_value="ns"), \ + patch("factory.contained.k8s_setup.login_status", return_value=(True, "me")), \ + patch("factory.contained.k8s_setup.inspect_objects", return_value=[]), \ + patch("factory.contained.k8s_setup.walk", + return_value=_walk_result(failed=True)) as walked, \ + patch("factory.contained.k8s_setup.run_credentials_step", return_value=True) as creds, \ + patch("factory.contained.k8s_setup.verify_k8s", return_value=ok): + code = k8s_setup.setup_k8s(namespace="ns", division=False, interactive=True) + walked.assert_called_once() + creds.assert_called_once() + assert code == 0 + + +# --------------------------------------------------------------------------------------------- +# _finish, and the default-context switch offer +# --------------------------------------------------------------------------------------------- + + +def test_finish_offers_the_default_switch_when_a_context_was_pinned() -> None: + k8s_setup.set_active_context("prepared-ctx") + with patch("factory.contained.k8s_setup.run_credentials_step", return_value=True), \ + patch("factory.contained.k8s_setup.verify_k8s", + return_value=[Check("x", True, "ok")]), \ + patch("factory.contained.k8s_setup._offer_default_switch") as offer: + code = k8s_setup._finish("oc", "ns", division=False, interactive=True) + offer.assert_called_once() + assert code == 0 + + +# --------------------------------------------------------------------------------------------- +# _choose_context / _ask_context +# --------------------------------------------------------------------------------------------- + + +def test_choose_context_returns_none_with_fewer_than_two() -> None: + with patch("factory.contained.k8s_setup.list_contexts", + return_value=[ClusterContext(context="only")]): + assert k8s_setup._choose_context(interactive=True) is None + + +def test_ask_context_accepts_a_number_a_name_and_reprompts_on_junk() -> None: + ctxs = [ClusterContext(context="a", server="s1"), ClusterContext(context="b", server="s2")] + with patch("factory.contained.k8s_setup.list_contexts", return_value=ctxs), \ + patch("factory.contained.k8s_setup.cluster_context", + return_value=ClusterContext(context="a")), \ + patch("factory.contained.k8s_setup.style.read_line", side_effect=["9", "junk", "b"]): + assert k8s_setup._choose_context(interactive=True) == "b" + + +def test_ask_context_aborts_on_escape() -> None: + ctxs = [ClusterContext(context="a"), ClusterContext(context="b")] + with patch("factory.contained.k8s_setup.list_contexts", return_value=ctxs), \ + patch("factory.contained.k8s_setup.style.read_line", return_value=None): + assert k8s_setup._choose_context(interactive=True) is k8s_setup._ABORT + + +# --------------------------------------------------------------------------------------------- +# _offer_default_switch +# --------------------------------------------------------------------------------------------- + + +def test_offer_switch_is_a_noop_when_already_current() -> None: + with patch("factory.contained.k8s_setup.cluster_context", + return_value=ClusterContext(context="c")): + k8s_setup._offer_default_switch("c", interactive=True) # returns early, nothing raised + + +def test_offer_switch_non_interactive_just_prints_the_command() -> None: + with patch("factory.contained.k8s_setup.cluster_context", + return_value=ClusterContext(context="other")): + k8s_setup._offer_default_switch("c", interactive=False) + + +def test_offer_switch_declined() -> None: + with patch("factory.contained.k8s_setup.cluster_context", + return_value=ClusterContext(context="other")), \ + patch("factory.contained.k8s_setup.style.confirm", return_value=False): + k8s_setup._offer_default_switch("c", interactive=True) + + +def test_offer_switch_accepted_success_and_failure() -> None: + with patch("factory.contained.k8s_setup.cluster_context", + return_value=ClusterContext(context="other")), \ + patch("factory.contained.k8s_setup.style.confirm", return_value=True), \ + patch("factory.contained.k8s_setup.use_context", return_value=(True, "now c")): + k8s_setup._offer_default_switch("c", interactive=True) + with patch("factory.contained.k8s_setup.cluster_context", + return_value=ClusterContext(context="other")), \ + patch("factory.contained.k8s_setup.style.confirm", return_value=True), \ + patch("factory.contained.k8s_setup.use_context", return_value=(False, "denied")): + k8s_setup._offer_default_switch("c", interactive=True) + + +# --------------------------------------------------------------------------------------------- +# _print_context field variants +# --------------------------------------------------------------------------------------------- + + +def test_print_context_full_and_empty() -> None: + with patch("factory.contained.k8s_setup.cluster_context", + return_value=ClusterContext(context="c", server="s", user="u")): + k8s_setup._print_context("ns") + with patch("factory.contained.k8s_setup.cluster_context", return_value=ClusterContext()): + k8s_setup._print_context(None) + + +# --------------------------------------------------------------------------------------------- +# _namespace_status / _create_namespace / _resolve_existing / _choose_namespace +# --------------------------------------------------------------------------------------------- + + +def test_namespace_status_present_absent_unreadable() -> None: + with patch("factory.contained.k8s_setup._run", return_value=_completed("namespace/ns")): + assert k8s_setup._namespace_status("ns", "oc") == k8s_setup.PRESENT + with patch("factory.contained.k8s_setup._run", return_value=None): + assert k8s_setup._namespace_status("ns", "kubectl") == k8s_setup.UNREADABLE + with patch("factory.contained.k8s_setup._run", + return_value=_completed(returncode=1, stderr="Error (NotFound): not found")): + assert k8s_setup._namespace_status("ns", "kubectl") == k8s_setup.ABSENT + # oc falls back to `get project`; a Forbidden on both is unreadable, not absent. + with patch("factory.contained.k8s_setup._run", + return_value=_completed(returncode=1, stderr="Forbidden")): + assert k8s_setup._namespace_status("ns", "oc") == k8s_setup.UNREADABLE + + +def test_create_namespace_success_failure_and_unrunnable() -> None: + with patch("factory.contained.k8s_setup._run", return_value=_completed("created")): + assert k8s_setup._create_namespace("ns", "oc")[0] is True + with patch("factory.contained.k8s_setup._run", return_value=None): + ok, detail = k8s_setup._create_namespace("ns", "kubectl") + assert not ok and "could not run" in detail + with patch("factory.contained.k8s_setup._run", + return_value=_completed(returncode=1, stderr="denied")): + ok, detail = k8s_setup._create_namespace("ns", "oc") + assert not ok and "denied" in detail + + +def test_resolve_existing_present_and_unreadable_are_ok() -> None: + with patch("factory.contained.k8s_setup._namespace_status", return_value=k8s_setup.PRESENT): + assert k8s_setup._resolve_existing("ns", "oc", interactive=True, assume_yes=False) == "ok" + with patch("factory.contained.k8s_setup._namespace_status", return_value=k8s_setup.UNREADABLE): + assert k8s_setup._resolve_existing("ns", "oc", interactive=True, assume_yes=False) == "ok" + + +def test_resolve_existing_absent_non_interactive_aborts() -> None: + with patch("factory.contained.k8s_setup._namespace_status", return_value=k8s_setup.ABSENT): + assert k8s_setup._resolve_existing( + "ns", "oc", interactive=False, assume_yes=False) == "abort" + + +def test_resolve_existing_absent_declined_retries_and_escaped_aborts() -> None: + with patch("factory.contained.k8s_setup._namespace_status", return_value=k8s_setup.ABSENT), \ + patch("factory.contained.k8s_setup.style.confirm", return_value=False): + assert k8s_setup._resolve_existing( + "ns", "oc", interactive=True, assume_yes=False) == "retry" + with patch("factory.contained.k8s_setup._namespace_status", return_value=k8s_setup.ABSENT), \ + patch("factory.contained.k8s_setup.style.confirm", return_value=None): + assert k8s_setup._resolve_existing( + "ns", "oc", interactive=True, assume_yes=False) == "abort" + + +def test_resolve_existing_absent_creates_when_confirmed() -> None: + with patch("factory.contained.k8s_setup._namespace_status", return_value=k8s_setup.ABSENT), \ + patch("factory.contained.k8s_setup._create_namespace", return_value=(True, "made")): + assert k8s_setup._resolve_existing( + "ns", "oc", interactive=True, assume_yes=True) == "ok" + + +def test_resolve_existing_create_failure_aborts_or_retries() -> None: + with patch("factory.contained.k8s_setup._namespace_status", return_value=k8s_setup.ABSENT), \ + patch("factory.contained.k8s_setup._create_namespace", return_value=(False, "denied")): + assert k8s_setup._resolve_existing( + "ns", "oc", interactive=False, assume_yes=True) == "abort" + assert k8s_setup._resolve_existing( + "ns", "oc", interactive=True, assume_yes=True) == "retry" + + +def test_choose_namespace_explicit_ok_and_not_ok() -> None: + with patch("factory.contained.k8s_setup._resolve_existing", return_value="ok"): + assert k8s_setup._choose_namespace( + "ns", interactive=True, binary="oc") == "ns" + with patch("factory.contained.k8s_setup._resolve_existing", return_value="abort"): + assert k8s_setup._choose_namespace( + "ns", interactive=True, binary="oc") is None + + +def test_choose_namespace_non_interactive_uses_the_current_context() -> None: + with patch("factory.contained.k8s_setup.resolve_namespace", return_value="ns"), \ + patch("factory.contained.k8s_setup._resolve_existing", return_value="ok"): + assert k8s_setup._choose_namespace( + None, interactive=False, binary="oc") == "ns" + + +def test_choose_namespace_interactive_reprompts_on_empty_then_accepts() -> None: + with patch("factory.contained.k8s_setup.current_namespace", return_value=None), \ + patch("factory.contained.k8s_setup.style.read_line", side_effect=["", "ns"]), \ + patch("factory.contained.k8s_setup._resolve_existing", return_value="ok"): + assert k8s_setup._choose_namespace( + None, interactive=True, binary="oc") == "ns" + + +def test_choose_namespace_interactive_escape_and_abort() -> None: + with patch("factory.contained.k8s_setup.current_namespace", return_value="cur"), \ + patch("factory.contained.k8s_setup.style.read_line", return_value=None): + assert k8s_setup._choose_namespace(None, interactive=True, binary="oc") is None + with patch("factory.contained.k8s_setup.current_namespace", return_value="cur"), \ + patch("factory.contained.k8s_setup.style.read_line", return_value="ns"), \ + patch("factory.contained.k8s_setup._resolve_existing", return_value="abort"): + assert k8s_setup._choose_namespace(None, interactive=True, binary="oc") is None + + +# --------------------------------------------------------------------------------------------- +# _verb_checks — unknown and denied +# --------------------------------------------------------------------------------------------- + + +def test_verb_checks_unknown_when_the_review_cannot_run() -> None: + with patch("factory.contained.k8s_setup.access_review", return_value=None): + checks = k8s_setup._verb_checks("ns", division=False) + perms = next(c for c in checks if c.name == "permissions") + assert not perms.ok and "unknown" in perms.detail + + +def test_verb_checks_names_the_denied_verbs() -> None: + with patch("factory.contained.k8s_setup.access_review", return_value=False): + checks = k8s_setup._verb_checks("ns", division=False) + perms = next(c for c in checks if c.name == "permissions") + assert not perms.ok and "cannot" in perms.detail + + +def test_probe_updates_the_status_line_when_given_one() -> None: + """With an Activity attached, `say()` drives it; without one it is a no-op (other tests).""" + import io + + act = k8s_setup.style.Activity("probe", stream=io.StringIO(), threshold=0.0) + with patch("factory.contained.k8s_setup.subprocess.run", side_effect=_probe_run(logs="PROBE_OK")), \ + patch("factory.contained.k8s_setup.poll_pod", + return_value=PodProgress(SUCCEEDED, "Succeeded", "Succeeded", "")): + check = k8s_setup._inference_check("oc", "ns", "img", act=act) + assert check.ok + + +def test_resolve_existing_create_on_plain_kubernetes_skips_the_oc_note() -> None: + with patch("factory.contained.k8s_setup._namespace_status", return_value=k8s_setup.ABSENT), \ + patch("factory.contained.k8s_setup._create_namespace", return_value=(True, "made")): + assert k8s_setup._resolve_existing( + "ns", "kubectl", interactive=True, assume_yes=True) == "ok" diff --git a/tests/test_contained_style_raw.py b/tests/test_contained_style_raw.py new file mode 100644 index 000000000..37d40f6e1 --- /dev/null +++ b/tests/test_contained_style_raw.py @@ -0,0 +1,433 @@ +"""The raw-terminal half of `style`, and the display branches the fallback tests never reach. + +`tests/conftest.py` forces `style._raw_session` to return None so no test blocks on a keypress — +which is exactly why the raw paths (`read_key`, `read_line`, `read_secret`, `_edit_line`, and the +keypress arms of `confirm`/`select`) go uncovered. Here we go the other way on purpose: a real pty, +with `_raw_session` restored to the genuine implementation for the duration, so the raw code runs +against a real terminal. + +The one trap worth naming: `sys.stdin` must read a byte straight from the fd (`os.read`), NOT through +a buffered text wrapper. `read_key`'s escape-sequence drain uses `select` on the fd, and a text +wrapper that has already pulled the `[` of an arrow key into its own buffer leaves `select` seeing +nothing on the fd — so an arrow reads as a bare Escape and the drain path never runs. `_RawStdin` +below reads one byte at a time from the kernel, which keeps `select` honest. +""" + +from __future__ import annotations + +import io +import os +import time +from unittest.mock import patch + + +from factory.contained import style + +# Captured at import, BEFORE conftest's autouse fixture replaces the attribute with a None-returning +# stub. This is the genuine function, so the tests below can exercise it and the callers that use it. +_REAL_RAW_SESSION = style._raw_session + + +class _TtyBuf(io.StringIO): + """A capture buffer that claims to be a terminal, for the `target`/echo side of a raw read.""" + + def isatty(self) -> bool: + return True + + +class _RawStdin: + """A stdin backed directly by a pty slave fd — one unbuffered byte per `read`, so `select` works.""" + + def __init__(self, fd: int, *, read_error: bool = False) -> None: + self._fd = fd + self._read_error = read_error + + def isatty(self) -> bool: + return os.isatty(self._fd) + + def fileno(self) -> int: + return self._fd + + def read(self, n: int = 1) -> str: + if self._read_error: + raise OSError("stdin read failed") + return os.read(self._fd, n).decode() + + +class _pty: + """A pty pair with `sys.stdin` pointed at the slave and the real `_raw_session` restored. + + Enter, get `(master_fd, target)`; write keystrokes with `feed()`; the raw `style` functions then + behave as if a person were typing. Everything is torn down on exit. + """ + + def __init__(self, *, read_error: bool = False) -> None: + self._read_error = read_error + + def __enter__(self) -> tuple[int, _TtyBuf]: + import pty + import termios + import tty + + self._master, self._slave = pty.openpty() + # Put the slave in cbreak up front. Bytes fed while the terminal is still in its default + # canonical mode sit in a line buffer that `read(1)` cannot reach until a newline arrives — + # which is a hang, since the code under test reads before we ever send one. cbreak makes each + # byte immediately readable, which is the mode `read_key`/`_edit_line` run in anyway. + tty.setcbreak(self._slave) + self._target = _TtyBuf() + + # `tty.setcbreak` defaults to TCSAFLUSH, which throws away input already queued on the + # terminal. The code under test calls it *after* we have fed our keystrokes, so with the + # default those keystrokes vanish and the read blocks forever. TCSANOW switches mode without + # discarding, which is exactly what a real interactive session does not need but a + # feed-then-read test does. + real_setcbreak = tty.setcbreak + + def _setcbreak_now(fd: int, when: int = termios.TCSANOW) -> None: + real_setcbreak(fd, termios.TCSANOW) + + self._patches = [ + patch.object(tty, "setcbreak", _setcbreak_now), + patch.object(style.sys, "stdin", _RawStdin(self._slave, read_error=self._read_error)), + patch.object(style, "_raw_session", _REAL_RAW_SESSION), + ] + for p in self._patches: + p.start() + return self._master, self._target + + def feed(self, data: bytes) -> None: + os.write(self._master, data) + + def __exit__(self, *exc: object) -> None: + for p in reversed(self._patches): + p.stop() + os.close(self._master) + os.close(self._slave) + + +def _feed(master: int, data: bytes) -> None: + os.write(master, data) + + +def _feed_staged(master: int, chunks: list[bytes], delay: float = 0.09) -> None: + """Write chunks with a gap between them, from a background thread. + + Needed only where an escape sequence is followed by more input: `_drain_escape_sequence` drains + *everything* currently buffered, so a single write of `arrow + text` loses the text. On a real + keyboard the arrow's bytes arrive as one burst and the next keystroke comes later; the gap here + (longer than the 50ms drain window) reproduces that so the drain stops at the arrow. + """ + import threading + + def run() -> None: + for chunk in chunks: + time.sleep(delay) + os.write(master, chunk) + + threading.Thread(target=run, daemon=True).start() + + +# --------------------------------------------------------------------------------------------- +# Display helpers the fallback suite never calls +# --------------------------------------------------------------------------------------------- + + +def test_enabled_and_can_rewrite_swallow_a_stream_that_cannot_answer_isatty() -> None: + """Asking a closed or exotic stream whether it is a terminal must not raise inside output code.""" + class _Broken(io.StringIO): + def isatty(self) -> bool: + raise ValueError("closed") + + with patch.dict("os.environ", {}, clear=True): + assert style.enabled(_Broken()) is False + assert style.can_rewrite(_Broken()) is False + + +def test_the_marks_and_headers_render_in_colour() -> None: + tty = _TtyBuf() + with patch.dict("os.environ", {"FORCE_COLOR": "1"}, clear=True): + assert "\033" in style.ok_mark(stream=tty) + assert "\033" in style.fail_mark(stream=tty) + assert "━" in style.section("Step", step=1, total=3, stream=tty) + assert "─" in style.subsection("Item", step=1, total=3, stream=tty) + + +def test_subsection_and_field_render_plain_too() -> None: + plain = io.StringIO() + assert "1 of 3" in style.subsection("Item", step=1, total=3, stream=plain) + assert "Cluster:" in style.field("Cluster", "x", stream=plain) + + +# --------------------------------------------------------------------------------------------- +# Activity: the spinner thread and the draw internals +# --------------------------------------------------------------------------------------------- + + +def test_activity_write_disables_itself_when_the_stream_breaks() -> None: + """A closed stream mid-run must not take down the operation being reported on.""" + class _Broken(_TtyBuf): + def write(self, s: str) -> int: + raise OSError("gone") + + act = style.Activity("x", stream=_Broken()) + act._write("anything") + assert act._rewrites is False + + +def test_the_spinner_stays_silent_until_the_threshold_passes() -> None: + """A wake before the threshold hits the `continue`, so nothing is drawn for a quick operation.""" + tty = _TtyBuf() + with style.activity("x", "waiting", stream=tty, threshold=0.3): + time.sleep(0.15) # one spinner wake (~0.1s) lands under the threshold + assert tty.getvalue() == "" # never crossed the threshold, so never drew + + +def test_the_spinner_notices_done_after_the_threshold_under_the_lock() -> None: + """If the run finishes just as a frame is due, the drawn-check under the lock bails cleanly.""" + tty = _TtyBuf() + act = style.Activity("x", stream=tty, threshold=0.0) + act.__enter__() + act._lock.acquire() # make the spinner block right where it would draw + time.sleep(0.15) # let it wake, pass the threshold, and wait on the lock + act._done.set() # now finish, so the post-lock check returns + act._lock.release() + act.__exit__(None, None, None) + # Nothing was drawn (the draw was blocked then cancelled), so there is nothing to erase. + assert "\r" not in tty.getvalue() + + +def test_a_long_frame_is_truncated_to_the_terminal_width() -> None: + tty = _TtyBuf() + act = style.Activity("x", "y" * 200, stream=tty, threshold=0.0) + act._started = time.monotonic() + with patch("factory.contained.style.shutil.get_terminal_size", + return_value=os.terminal_size((40, 24))): + act._draw() + drawn = tty.getvalue() + assert "…" in drawn # the overflow was cut, not wrapped + + +# --------------------------------------------------------------------------------------------- +# _raw_session itself +# --------------------------------------------------------------------------------------------- + + +def test_raw_session_returns_the_fd_and_saved_settings_on_a_real_tty() -> None: + import pty + + master, slave = pty.openpty() + try: + with patch.object(style.sys, "stdin", _RawStdin(slave)): + session = _REAL_RAW_SESSION(_TtyBuf()) + assert session is not None + fd, saved = session + assert fd == slave and isinstance(saved, list) + finally: + os.close(master) + os.close(slave) + + +def test_raw_session_declines_when_the_target_is_not_a_terminal() -> None: + import pty + + master, slave = pty.openpty() + try: + with patch.object(style.sys, "stdin", _RawStdin(slave)): + assert _REAL_RAW_SESSION(io.StringIO()) is None # target.isatty() is False + finally: + os.close(master) + os.close(slave) + + +def test_raw_session_declines_when_isatty_raises() -> None: + class _Broken: + def isatty(self) -> bool: + raise ValueError("closed") + + with patch.object(style.sys, "stdin", _Broken()): + assert _REAL_RAW_SESSION(_TtyBuf()) is None + + +def test_raw_session_declines_when_termios_refuses_the_descriptor() -> None: + import pty + import termios + + master, slave = pty.openpty() + try: + with patch.object(style.sys, "stdin", _RawStdin(slave)), \ + patch("termios.tcgetattr", side_effect=termios.error("nope")): + assert _REAL_RAW_SESSION(_TtyBuf()) is None + finally: + os.close(master) + os.close(slave) + + +# --------------------------------------------------------------------------------------------- +# read_key on a real terminal +# --------------------------------------------------------------------------------------------- + + +def test_read_key_returns_a_single_character() -> None: + with _pty() as (master, target): + _feed(master, b"a") + assert style.read_key("? ", stream=target) == "a" + + +def test_read_key_returns_escape_for_a_bare_escape() -> None: + with _pty() as (master, target): + _feed(master, b"\x1b") + assert style.read_key("? ", stream=target) == style.ESCAPE + + +def test_read_key_ignores_an_arrow_key() -> None: + """An arrow arrives as ESC + `[A`; the drain consumes the tail and the key is reported as ''.""" + with _pty() as (master, target): + _feed(master, b"\x1b[A") + assert style.read_key("? ", stream=target) == "" + + +def test_read_key_returns_none_when_the_read_fails() -> None: + with _pty(read_error=True) as (master, target): + assert style.read_key("? ", stream=target) is None + + +# --------------------------------------------------------------------------------------------- +# read_line / _edit_line on a real terminal +# --------------------------------------------------------------------------------------------- + + +def test_read_line_accepts_a_typed_line() -> None: + with _pty() as (master, target): + _feed(master, b"factory-yi\r") + assert style.read_line("Namespace", stream=target) == "factory-yi" + + +def test_read_line_backspace_erases_a_character() -> None: + with _pty() as (master, target): + _feed(master, b"ab\x7fc\r") # a, b, backspace, c -> "ac" + assert style.read_line("?", stream=target) == "ac" + + +def test_read_line_escape_cancels_immediately() -> None: + with _pty() as (master, target): + _feed(master, b"\x1b") + assert style.read_line("?", stream=target) is None + + +def test_read_line_an_arrow_key_is_neither_text_nor_cancel() -> None: + with _pty() as (master, target): + # Staged: the arrow first, then the text after the drain window, so the drain stops at the + # arrow instead of swallowing the "x". + _feed_staged(master, [b"\x1b[A", b"x\r"]) + assert style.read_line("?", stream=target) == "x" + + +def test_read_line_ctrl_d_on_an_empty_line_cancels() -> None: + with _pty() as (master, target): + _feed(master, b"\x04") + assert style.read_line("?", stream=target) is None + + +def test_read_line_ctrl_d_mid_line_is_ignored() -> None: + with _pty() as (master, target): + _feed(master, b"a\x04b\r") # the Ctrl-D between letters does nothing + assert style.read_line("?", stream=target) == "ab" + + +# --------------------------------------------------------------------------------------------- +# read_secret on a real terminal +# --------------------------------------------------------------------------------------------- + + +def test_read_secret_masks_the_echo_but_returns_the_value() -> None: + with _pty() as (master, target): + _feed(master, b"s3kret\r") + value = style.read_secret("API key", stream=target) + assert value == "s3kret" + echo = target.getvalue() + assert "s3kret" not in echo # the value never appears on screen + assert "*" * 6 in echo # one mask per character + + +# --------------------------------------------------------------------------------------------- +# confirm / select — keypress arms and the input() fallback arms +# --------------------------------------------------------------------------------------------- + + +def test_confirm_keypress_enter_takes_the_default_and_n_returns_false() -> None: + with patch("factory.contained.style.read_key", return_value="\r"): + assert style.confirm("?", default=True) is True + with patch("factory.contained.style.read_key", return_value="n"): + assert style.confirm("?") is False + + +def test_confirm_line_fallback_covers_yes_no_and_reask() -> None: + with patch("factory.contained.style.read_key", return_value=None), \ + patch("builtins.input", side_effect=["y"]): + assert style.confirm("?") is True + with patch("factory.contained.style.read_key", return_value=None), \ + patch("builtins.input", side_effect=["no"]): + assert style.confirm("?") is False + # An unrecognised answer re-asks rather than guessing. + with patch("factory.contained.style.read_key", return_value=None), \ + patch("builtins.input", side_effect=["huh?", "yes"]): + assert style.confirm("?") is True + + +def test_select_keypress_reasks_on_an_unknown_key() -> None: + with patch("factory.contained.style.read_key", side_effect=["z", "1"]): + assert style.select("?", [("1", "local"), ("2", "k8s")]) == "1" + + +def test_select_line_fallback_covers_valid_reask_escape_and_eof() -> None: + with patch("factory.contained.style.read_key", return_value=None), \ + patch("builtins.input", side_effect=["nope", "2"]): + assert style.select("?", [("1", "local"), ("2", "k8s")]) == "2" + with patch("factory.contained.style.read_key", return_value=None), \ + patch("builtins.input", return_value="\x1b"): + assert style.select("?", [("1", "local")]) is None + with patch("factory.contained.style.read_key", return_value=None), \ + patch("builtins.input", side_effect=EOFError): + assert style.select("?", [("1", "local")]) is None + + +# --------------------------------------------------------------------------------------------- +# The last few branch edges +# --------------------------------------------------------------------------------------------- + + +def test_activity_plain_update_is_silent_before_the_threshold() -> None: + """Off a tty and still quick: `update` records the text but writes nothing yet.""" + plain = io.StringIO() + act = style.Activity("x", stream=plain, threshold=5.0) + act._started = time.monotonic() + act.update("waiting") + assert plain.getvalue() == "" + + +def test_read_line_returns_none_when_the_raw_read_fails() -> None: + with _pty(read_error=True) as (master, target): + assert style.read_line("?", stream=target) is None + + +def test_read_secret_returns_none_when_the_raw_read_fails() -> None: + with _pty(read_error=True) as (master, target): + assert style.read_secret("?", stream=target) is None + + +def test_edit_line_ignores_backspace_on_an_empty_line() -> None: + with _pty() as (master, target): + _feed(master, b"\x7fab\r") # backspace with nothing to erase, then "ab" + assert style.read_line("?", stream=target) == "ab" + + +def test_edit_line_ignores_an_unhandled_control_character() -> None: + with _pty() as (master, target): + _feed(master, b"\x01a\r") # Ctrl-A: not a key it handles, and not printable + assert style.read_line("?", stream=target) == "a" + + +def test_confirm_keypress_reasks_on_an_unrelated_key() -> None: + with patch("factory.contained.style.read_key", side_effect=["q", "y"]): + assert style.confirm("?") is True From d1fab1004651aa7537d74831b2c74e1c95f5c0eb Mon Sep 17 00:00:00 2001 From: Yi Zheng <237498169+beatsmonster@users.noreply.github.com> Date: Fri, 21 Aug 2026 18:43:13 -0400 Subject: [PATCH 6/6] refactor(contained): three functions back under the structural gate MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The Sentrux gate reported DEGRADED on this PR — "complex functions increased" — for three functions this branch pushed past its cyclomatic threshold of 15. None had grown a new decision; each had accumulated a second job, which is what the metric is for. `classify_pod` was answering three questions at once: which container statuses the caller means, what one container's state implies, and what the pod as a whole implies when no container has spoken yet. Those are now `_container_statuses`, `_classify_container_state` and `_classify_pod_level`. The middle one returns `PodProgress | None`, where `None` keeps the original loop's meaning exactly — this entry carries no state, try the next — rather than the "nothing is wrong" it could be mistaken for. `run_credentials_step` kept its four early exits and handed the second half to `_confirm_and_create`. The recipe for creating the Secret by hand was printed from two of those exits with the same four lines; it is `_print_create_command` once. `verify_k8s` splits where the function already changed subject: the preflight that establishes a reachable cluster and a usable namespace, each step of which returns early on failure, and the inspection of that namespace, which runs every check unconditionally. The second half is now `_namespace_inspection`, a generator so results still reach the caller's `record` the moment each is known — the streaming that stopped `verify` from looking like a hang is the whole reason these checks are not simply collected into a list. No behaviour changes. The contained package stays at 100% statement and branch coverage, and the gate reports no degradation. Co-Authored-By: Claude Opus 5 Signed-off-by: Yi Zheng <237498169+beatsmonster@users.noreply.github.com> --- factory/contained/k8s.py | 84 +++++++++++++++++++--------- factory/contained/k8s_credentials.py | 21 +++++-- factory/contained/k8s_setup.py | 42 ++++++++++---- 3 files changed, 107 insertions(+), 40 deletions(-) diff --git a/factory/contained/k8s.py b/factory/contained/k8s.py index dbd7efbeb..cab5179f5 100644 --- a/factory/contained/k8s.py +++ b/factory/contained/k8s.py @@ -988,44 +988,76 @@ def classify_pod(pod: dict[str, Any], *, container: str | None = None) -> PodPro return PodProgress(WAITING, "", "Pending", "") phase = str(status.get("phase", "") or "") - statuses = [ - entry for entry in - (list(status.get("initContainerStatuses", []) or []) - + list(status.get("containerStatuses", []) or [])) + for entry in _container_statuses(status, container): + verdict = _classify_container_state(entry.get("state", {}) or {}, phase) + if verdict is not None: + return verdict + + # No container has a state yet — the answer is at pod level, where an unschedulable pod lives. + return _classify_pod_level(status, phase) + + +def _container_statuses(status: dict[str, Any], container: str | None) -> list[dict[str, Any]]: + """The container statuses this question is about — init and app alike, in that order. + + initContainers come first because they run first: the loader's window is an initContainer that + is still running, and an app container's state says nothing about it. + """ + entries = list(status.get("initContainerStatuses", []) or []) + list( + status.get("containerStatuses", []) or [] + ) + return [ + entry + for entry in entries if isinstance(entry, dict) and (container is None or entry.get("name") == container) ] - for entry in statuses: - state = entry.get("state", {}) or {} - terminated = state.get("terminated") - if isinstance(terminated, dict): - code = terminated.get("exitCode") - if code == 0: - return PodProgress(SUCCEEDED, phase, str(terminated.get("reason") or "Completed"), - str(terminated.get("message") or "")) + +def _classify_container_state(state: dict[str, Any], phase: str) -> PodProgress | None: + """What one container's state means, or `None` when it does not speak yet. + + `None` is not "fine" — it means this entry carries no state at all, so the caller moves on to + the next container and ultimately to the pod-level question. + """ + terminated = state.get("terminated") + if isinstance(terminated, dict): + code = terminated.get("exitCode") + if code == 0: return PodProgress( - DOOMED, phase, str(terminated.get("reason") or "Error"), - str(terminated.get("message") or f"exit code {code}"), + SUCCEEDED, + phase, + str(terminated.get("reason") or "Completed"), + str(terminated.get("message") or ""), ) - if "running" in state: - return PodProgress(RUNNING, phase, "Running", "") - waiting = state.get("waiting") - if isinstance(waiting, dict): - reason = str(waiting.get("reason") or "") - message = str(waiting.get("message") or "") - if reason in DOOMED_WAITING_REASONS: - return PodProgress(DOOMED, phase, reason, message) - return PodProgress(WAITING, phase, reason or "Waiting", message) + return PodProgress( + DOOMED, + phase, + str(terminated.get("reason") or "Error"), + str(terminated.get("message") or f"exit code {code}"), + ) + if "running" in state: + return PodProgress(RUNNING, phase, "Running", "") + waiting = state.get("waiting") + if isinstance(waiting, dict): + reason = str(waiting.get("reason") or "") + message = str(waiting.get("message") or "") + if reason in DOOMED_WAITING_REASONS: + return PodProgress(DOOMED, phase, reason, message) + return PodProgress(WAITING, phase, reason or "Waiting", message) + return None - # No container has a state yet — the answer is at pod level, where an unschedulable pod lives. + +def _classify_pod_level(status: dict[str, Any], phase: str) -> PodProgress: + """The verdict when no container has spoken — including the pod nothing will ever schedule.""" unschedulable = _unschedulable(status) if unschedulable is not None: return PodProgress(DOOMED, phase, "Unschedulable", unschedulable) if phase == "Succeeded": return PodProgress(SUCCEEDED, phase, "Succeeded", "") if phase == "Failed": - return PodProgress(DOOMED, phase, str(status.get("reason") or "Failed"), - str(status.get("message") or "")) + return PodProgress( + DOOMED, phase, str(status.get("reason") or "Failed"), str(status.get("message") or "") + ) return PodProgress(WAITING, phase, phase or "Pending", "") diff --git a/factory/contained/k8s_credentials.py b/factory/contained/k8s_credentials.py index a6359a0ac..0fbd19e22 100644 --- a/factory/contained/k8s_credentials.py +++ b/factory/contained/k8s_credentials.py @@ -489,8 +489,7 @@ def run_credentials_step( "Nobody is at the keyboard, so this step is being skipped — a credential is never " "chosen on your behalf. Create it with:" )) - for chunk in create_secret_command(binary, namespace).splitlines(): - print(style.line(style.dim(chunk))) + _print_create_command(binary, namespace) return False print(style.line(style.paint(existing.detail, "yellow"))) @@ -507,10 +506,24 @@ def run_credentials_step( data = _choose_backend(readers) if data is None: print(style.line("Skipped. Create it yourself with:")) - for chunk in create_secret_command(binary, namespace).splitlines(): - print(style.line(style.dim(chunk))) + _print_create_command(binary, namespace) return False + return _confirm_and_create(binary, namespace, data) + + +def _print_create_command(binary: str, namespace: str) -> None: + """The command that does by hand what this step offers to do, dimmed and ready to copy.""" + for chunk in create_secret_command(binary, namespace).splitlines(): + print(style.line(style.dim(chunk))) + + +def _confirm_and_create(binary: str, namespace: str, data: dict[str, str]) -> bool: + """Show the shape of what is about to be sent, then send it. Returns whether it took. + + The confirmation prints *shape* — key names and value lengths — never material, so a user + reading it over someone's shoulder learns nothing they could reuse. + """ print() print(style.line(f"About to create {style.value(f'secret/{SECRET_NAME}')} in " f"{style.value(namespace)} with:")) diff --git a/factory/contained/k8s_setup.py b/factory/contained/k8s_setup.py index 49ca023f3..a9d0659b6 100644 --- a/factory/contained/k8s_setup.py +++ b/factory/contained/k8s_setup.py @@ -26,7 +26,7 @@ import json import subprocess import sys -from collections.abc import Callable +from collections.abc import Callable, Iterator import structlog @@ -175,27 +175,49 @@ def record(*new: Check) -> None: ) return checks + for check in _namespace_inspection( + binary, target, division=division, probe_inference=probe_inference, + streaming=on_check is not None, + ): + record(check) + return checks + + +def _namespace_inspection( + binary: str, + target: str, + *, + division: bool, + probe_inference: bool, + streaming: bool, +) -> Iterator[Check]: + """Everything worth asking once a reachable cluster and a usable namespace are established. + + A generator rather than a list so the caller reports each result the moment it is known. These + are the slow checks — an access review per verb is a round trip each, and the inference probe + launches a pod — and a caller that could only print at the end would show a blank screen for + minutes, which reads as a hang. + """ with style.activity("namespace", f"looking up {target}"): namespace_check = _namespace_check(binary, target) - record(namespace_check) + yield namespace_check with style.activity("bundle", f"comparing each object against {target}"): object_checks = _object_checks(binary, target, division) - record(*object_checks) + yield from object_checks with style.activity("permissions", "posting an access review per verb the run needs"): verb_checks = _verb_checks(target, division) - record(*verb_checks) + yield from verb_checks with style.activity("credentials_secret", f"reading secret/{SECRET_NAME}"): secret = secret_check(binary, target) - record(secret) - record(_image_check()) + yield secret + yield _image_check() if probe_inference: - record(_inference_result(binary, target, secret, announce=on_check is not None)) - record(_gitleaks_check()) + yield _inference_result(binary, target, secret, announce=streaming) + yield _gitleaks_check() if division: with style.activity("build_api", "asking the cluster which APIs it serves"): division_checks = _division_checks(target) - record(*division_checks) - return checks + yield from division_checks def _context_check(binary: str) -> Check: