Merge origin/main into fix/919-api-key-lifecycle

Author: frankbria

codeframe/cli/app.py

@@ -2543,8 +2543,10 @@ def work_start(
         if execute:
             from codeframe.core.engine_registry import is_external_engine
             if engine == "codex":
-                from codeframe.cli.validators import require_openai_api_key
-                require_openai_api_key()
+                # Not the OpenAI key check: `codex login` is the common way in
+                # and sets no env var at all (#1010).
+                from codeframe.cli.validators import require_codex_auth
+                require_codex_auth()
             elif engine == "cloud":
                 from codeframe.cli.validators import require_e2b_api_key
                 require_e2b_api_key()
@@ -3997,8 +3999,10 @@ def batch_run(
         # Validate API key before batch execution
         from codeframe.core.engine_registry import is_external_engine
         if engine == "codex":
-            from codeframe.cli.validators import require_openai_api_key
-            require_openai_api_key()
+            # Not the OpenAI key check: `codex login` is the common way in
+            # and sets no env var at all (#1010).
+            from codeframe.cli.validators import require_codex_auth
+            require_codex_auth()
         elif engine == "cloud":
             from codeframe.cli.validators import require_e2b_api_key
             require_e2b_api_key()

codeframe/cli/validators.py

@@ -122,3 +122,32 @@ def require_e2b_api_key() -> str:
         "Get your key at https://e2b.dev"
     )
     raise typer.Exit(1)
+
+
+def require_codex_auth() -> None:
+    """Ensure the codex CLI can reach a model, by either route (#1010).
+
+    Not ``require_openai_api_key``: ``codex login`` stores ChatGPT-plan
+    credentials in ``auth.json`` and writes ``"OPENAI_API_KEY": null`` in that
+    same file, so gating on the environment variable refused the common case —
+    ``--engine codex`` was unusable for anyone who had simply logged in, even
+    though the adapter and the binary both worked.
+
+    Raises:
+        typer.Exit: If codex is authenticated by neither route.
+    """
+    from codeframe.core.adapters.codex import CodexAdapter
+
+    if CodexAdapter.is_authenticated():
+        return
+
+    load_env_files()
+    if CodexAdapter.is_authenticated():
+        return
+
+    console.print(
+        "[red]Error:[/red] codex is not authenticated. "
+        "Run [bold]codex login[/bold], or set OPENAI_API_KEY in your "
+        "environment or a .env file."
+    )
+    raise typer.Exit(1)

codeframe/core/adapters/codex.py

@@ -22,6 +22,7 @@
 
 from __future__ import annotations
 
+import os
 import json
 import logging
 import queue
@@ -148,8 +149,61 @@ def name(self) -> str:
 
     @classmethod
     def requirements(cls) -> dict[str, str]:
-        """Return required environment variables for ``cf engines check``."""
-        return {"OPENAI_API_KEY": "OpenAI API key"}
+        """No *required* environment variables (#1010).
+
+        ``OPENAI_API_KEY`` used to be listed here, and ``cf engines check`` marks
+        any unsatisfied entry as an unmet requirement and exits 1 — so a codex
+        authenticated by ``codex login`` (which sets no variable at all, and
+        records ``"OPENAI_API_KEY": null`` in its own auth.json) was reported as
+        broken while working perfectly. ``check_ready`` below answers the real
+        question, by either route.
+        """
+        return {}
+
+    @classmethod
+    def codex_home(cls) -> Path:
+        """Where codex keeps its state. ``$CODEX_HOME`` is documented by the CLI."""
+        override = os.environ.get("CODEX_HOME")
+        return Path(override) if override else Path.home() / ".codex"
+
+    @classmethod
+    def is_authenticated(cls) -> bool:
+        """True when codex can actually talk to a model (#1010).
+
+        ``OPENAI_API_KEY`` is *not* the test. ``codex login`` writes ChatGPT-plan
+        credentials to ``auth.json`` and records ``"OPENAI_API_KEY": null`` in
+        the very same file — so gating on the environment variable refuses the
+        common case, where the CLI works perfectly well.
+
+        Presence of the file is not the test either: ``codex logout`` can leave
+        it behind with empty tokens.
+        """
+        if os.environ.get("OPENAI_API_KEY"):
+            return True
+
+        try:
+            auth = json.loads((cls.codex_home() / "auth.json").read_text())
+        except (OSError, json.JSONDecodeError, ValueError):
+            # ValueError also covers UnicodeDecodeError from read_text() — a
+            # genuinely corrupt auth.json must read as "not authenticated",
+            # not crash the caller. JSONDecodeError is itself a ValueError, so
+            # this is the whole family. (#1010 review)
+            return False
+
+        if not isinstance(auth, dict):
+            return False
+        if auth.get("OPENAI_API_KEY"):
+            return True
+        tokens = auth.get("tokens")
+        return bool(isinstance(tokens, dict) and tokens.get("access_token"))
+
+    @classmethod
+    def check_ready(cls) -> dict[str, bool]:
+        """What ``cf engines check`` reports for codex."""
+        return {
+            "codex_binary": shutil.which("codex") is not None,
+            "authenticated": cls.is_authenticated(),
+        }
 
     @classmethod
     def credential_env_vars(cls) -> tuple[str, ...]:

codeframe/core/adapters/kilocode.py

@@ -2,36 +2,119 @@
 
 from __future__ import annotations
 
+import logging
 import os
 import shlex
 import shutil
+import subprocess
 from pathlib import Path
 
 from codeframe.core.adapters.agent_adapter import AgentResult
 from codeframe.core.adapters.subprocess_adapter import SubprocessAdapter
 
+logger = logging.getLogger(__name__)
+
 # Exit code used by kilo when the timeout is exceeded
 _KILO_TIMEOUT_EXIT_CODE = 124
 
 
+#: The two incompatible CLIs that both answer to ``kilo``.
+_MODERN = "modern"  # 7.x: `kilo run <message> --dir <path>`
+_LEGACY = "legacy"  # 0.22.0: `kilo <prompt> --auto --workspace <path>`
+
+#: Substring that appears in ``kilo --help`` only once ``run`` exists. Detection
+#: reads the CLI's own help rather than parsing ``--version``: #1015 requires
+#: that the invocation never be guessed from a version string, and this repo has
+#: been bitten three times by adapters that assumed a CLI surface (#913/#914/
+#: #1012). Help text is what the binary actually offers.
+_RUN_SUBCOMMAND_MARKER = "kilo run"
+
+#: Linux caps a *single* argv entry at MAX_ARG_STRLEN — 128 KiB — independently
+#: of the much larger total ARG_MAX. Same constraint opencode hit in #913.
+_MAX_ARG_BYTES = 128 * 1024
+
+#: Detection runs a subprocess, so it is cached per binary path for the process.
+_SURFACE_CACHE: dict[str, str] = {}
+
+
+def _prompt_exceeds_argv(prompt: str) -> bool:
+    return len(prompt.encode("utf-8")) >= _MAX_ARG_BYTES
+
+
+def _detect_surface(binary_path: str) -> str:
+    """Which kilo CLI is installed, according to its own ``--help``.
+
+    ``@kilocode/cli`` was rewritten between 0.22.0 (2026-01-15) and 7.x
+    (2026-07-29) — 213 releases apart. The invocations share nothing:
+
+    ==============  ============================  ==========================
+    \\               0.22.0                        7.4.17
+    ==============  ============================  ==========================
+    usage           ``kilocode [options] [prompt]``  ``kilo run [message..]``
+    workspace       ``--workspace <path>``        ``--dir <path>``
+    ``--auto``      non-interactive               **auto-approve ALL perms**
+    ==============  ============================  ==========================
+
+    That last row is why this cannot be a blind rename: on 7.x ``--auto`` is the
+    old ``--yolo``, the permission bypass #916 established must stay off.
+
+    An unreadable or failing ``--help`` falls back to modern — the version any
+    new install gets, and the one whose ``run`` subcommand fails loudly rather
+    than opening a TUI that hangs until the timeout (#1012).
+    """
+    if binary_path in _SURFACE_CACHE:
+        return _SURFACE_CACHE[binary_path]
+
+    try:
+        # Bytes, decoded permissively. `text=True` decodes with the locale
+        # encoding and no error handler, so under a non-UTF-8 locale
+        # (LC_ALL=C with UTF-8 coercion disabled — verified: encoding becomes
+        # ANSI_X3.4-1968) kilo 7.x's box-drawing banner raises
+        # UnicodeDecodeError. That is a ValueError, so it sailed straight past
+        # the handler below and crashed build_command. (#1015 review)
+        proc = subprocess.run(
+            [binary_path, "--help"], capture_output=True, timeout=90
+        )
+        help_text = (proc.stdout + proc.stderr).decode("utf-8", errors="replace")
+    except (OSError, subprocess.SubprocessError, ValueError):
+        logger.warning(
+            "Could not read `%s --help`; assuming the modern kilo surface.",
+            binary_path,
+        )
+        help_text = ""
+
+    surface = _MODERN if (not help_text or _RUN_SUBCOMMAND_MARKER in help_text) else _LEGACY
+    _SURFACE_CACHE[binary_path] = surface
+    return surface
+
+
 class KilocodeAdapter(SubprocessAdapter):
     """Adapter that delegates code execution to Kilocode CLI.
 
-    Invokes ``kilo <prompt> --auto --workspace <path>`` for headless
-    non-interactive execution.  The prompt is the CLI's leading positional
-    (not stdin, and **not** behind a subcommand).
+    **Supported version floor: ``@kilocode/cli`` 7.x**, the surface any new
+    install gets. 0.22.0 is still driven correctly when that is what is
+    installed, because #1012 verified it end-to-end and an unupgraded machine
+    should not silently break — but it is not the target, and support for it
+    can be dropped once nobody is on it.
+
+    Which invocation to use is **detected from the CLI's own ``--help``**, never
+    inferred from a version string (#1015). ``_detect_surface`` has the table:
 
-    There is no ``run`` subcommand: verified against kilocode 0.22.0, whose
-    usage is ``kilocode [options] [command] [prompt]`` with commands
-    ``auth``/``config``/``debug``/``models`` only. The adapter used to prepend
-    ``run``, which was then swallowed as the prompt — ``--auto`` never took
-    effect, the interactive TUI opened, and the delegated run hung until the
-    timeout having written nothing (#1012).
+    * 7.x — ``kilo run --dir <path> <message>``. No ``--auto``: on this CLI that
+      flag means "auto-approve all permissions", i.e. the old ``--yolo`` the
+      adapter has always withheld (#916). ``run`` is non-interactive on its own,
+      exactly like ``opencode run``.
+    * 0.22.0 — ``kilo <prompt> --auto --workspace <path>``, where ``--auto`` is
+      merely "non-interactive". There is no ``run`` subcommand; prepending one
+      got it swallowed as the prompt, opening the TUI to hang until the timeout
+      having written nothing (#1012).
 
-    Note on prompt length: the prompt is passed as a single positional argument.
-    Linux supports up to ~2 MB per argument, but macOS caps individual arguments
-    at 256 KB. Very large task contexts assembled by TaskContextPackager may fail
-    on macOS. If Kilocode adds stdin support in a future release, prefer that path.
+    Prompt length: the prompt is a single argv entry, and Linux caps one entry
+    at 128 KiB (macOS at 256 KB) — well under CodeFrame's ~100K-token budget. On
+    7.x an oversized prompt goes to stdin instead, which ``kilo run`` accepts
+    when given no positional (verified: ``echo "say ok" | kilo run --dir /tmp``
+    reaches the model call). 0.22.0 has no stdin path, so there it still goes
+    positionally and fails loudly.
 
     Exit codes:
         0   — success
@@ -89,26 +172,44 @@ def check_ready(cls) -> dict[str, bool]:
         """Check if the kilo binary is available on PATH."""
         return {"kilo_binary": shutil.which(cls._resolve_binary()) is not None}
 
+    def _surface(self) -> str:
+        """Which kilo CLI this adapter is talking to."""
+        return _detect_surface(self._binary_path)
+
     def build_command(self, prompt: str, workspace_path: Path) -> list[str]:
-        """Build the kilo CLI command.
+        """Build the kilo CLI command for whichever CLI is actually installed.
 
-        Kilocode takes the prompt as a positional argument, with ``--auto``
-        for non-interactive execution and ``--workspace`` for the repo root.
+        The two eras take completely different invocations (see
+        ``_detect_surface``). Modern is the documented target; legacy is kept
+        because it is what #1012 verified end-to-end and what an unupgraded
+        install still speaks.
 
         Args:
-            prompt: The task prompt passed as a positional argument.
-            workspace_path: Workspace root passed as ``--workspace``.
+            prompt: The task prompt.
+            workspace_path: Workspace root.
 
         Returns:
             Command list for subprocess.Popen.
         """
-        cmd = [
-            self._binary_path,
-            prompt,
-            "--auto",
-            "--workspace",
-            str(workspace_path),
-        ]
+        if self._surface() == _MODERN:
+            # `run` is non-interactive by itself, exactly like `opencode run`.
+            # --auto is deliberately NOT passed: in 7.x it means "auto-approve
+            # all permissions", the 0.22 `--yolo` that #916 established must
+            # stay off. Renaming --workspace to --dir while keeping --auto
+            # would have silently upgraded the adapter into a permission bypass.
+            cmd = [self._binary_path, "run", "--dir", str(workspace_path)]
+            if not _prompt_exceeds_argv(prompt):
+                cmd.append(prompt)
+        else:
+            # 0.22.0: bare positional prompt, --auto is merely non-interactive
+            # and --yolo (never passed) is the bypass. Verified in #1012/#916.
+            cmd = [
+                self._binary_path,
+                prompt,
+                "--auto",
+                "--workspace",
+                str(workspace_path),
+            ]
 
         model = os.environ.get("KILOCODE_MODEL")
         if model:
@@ -121,7 +222,20 @@ def build_command(self, prompt: str, workspace_path: Path) -> list[str]:
         return cmd
 
     def get_stdin(self, prompt: str) -> str | None:
-        """Return None — prompt is passed as a positional CLI argument, not stdin."""
+        """The prompt, only when it is too large to survive as an argv entry.
+
+        Normally None — both eras take the prompt positionally. But Linux caps a
+        *single* argv entry at 128 KiB while CodeFrame budgets ~100K tokens of
+        prompt, so a large task would raise OSError(E2BIG) before kilo starts.
+
+        ``kilo run`` with no positional reads the message from stdin: verified
+        against 7.4.17, where `echo "say ok" | kilo run --dir /tmp` gets past
+        message validation to the model call. The legacy CLI has no such path,
+        so an oversized prompt still goes positionally there and fails loudly
+        rather than silently doing nothing.
+        """
+        if self._surface() == _MODERN and _prompt_exceeds_argv(prompt):
+            return prompt
         return None
 
     def _map_result(

tests/core/adapters/fixtures/kilocode_help/README.md

@@ -0,0 +1,21 @@
+# kilocode CLI help fixtures
+
+Verbatim `kilo --help` output from the two incompatible CLIs that both answer to
+`kilo`. `_detect_surface` is matched against these rather than against text
+invented for the test — the same reason #914 checks in the codex app-server
+schema.
+
+| file | version | captured |
+|---|---|---|
+| `help-0.22.0.txt` | `@kilocode/cli@0.22.0` (2026-01-15) | 2026-08-01 |
+| `help-7.4.17.txt` | `@kilocode/cli@7.4.17` (2026-07-29) | 2026-08-01 |
+
+Regenerate with:
+
+```
+npm install -g @kilocode/cli@<version>
+kilo --help > help-<version>.txt 2>&1
+```
+
+The 7.x file contains ANSI/box-drawing characters from the banner; that is
+deliberate, since the real output does too and the detector must cope with it.