From 4214b3fccd11734c6c6e506384684ff48b948946 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?K=C5=8Dan?= Date: Wed, 3 Jun 2026 11:17:21 -0600 Subject: [PATCH 01/20] feat(review): add branding link and provider/model info to review footer MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The review comment footer now links "Kōan" to https://koan.anantys.com and shows the CLI provider and model used (e.g., "Claude · model claude-opus-4-6"), giving reviewers transparency on how the review was generated. Co-Authored-By: Claude Opus 4.6 --- koan/app/review_runner.py | 33 ++++++++++++++++++++++-- koan/tests/test_review_runner.py | 43 ++++++++++++++++++++++++++++++++ 2 files changed, 74 insertions(+), 2 deletions(-) diff --git a/koan/app/review_runner.py b/koan/app/review_runner.py index f18e9049e..54f023a90 100644 --- a/koan/app/review_runner.py +++ b/koan/app/review_runner.py @@ -1026,10 +1026,25 @@ def _format_review_as_markdown(review_data: dict, title: str = "", bot_username: return "\n".join(lines) +def _build_review_footer(provider_name: str = "", model: str = "") -> str: + """Build the review footer with branding, provider, and model info.""" + base = "_Automated review by [Kōan](https://koan.anantys.com)_" + parts = [] + if provider_name: + parts.append(provider_name.capitalize()) + if model: + parts.append(f"model {model}") + if parts: + return f"{base} _({' · '.join(parts)})_" + return base + + def _post_review_comment( owner: str, repo: str, pr_number: str, review_text: str, existing_comment: Optional[dict] = None, commit_shas: Optional[List[str]] = None, + provider_name: str = "", + model: str = "", ) -> Tuple[bool, str]: """Post (or update) the review as a comment on the PR. @@ -1049,11 +1064,13 @@ def _post_review_comment( if len(review_text) > max_len: review_text = review_text[:max_len] + "\n\n_(Review truncated)_" + footer = _build_review_footer(provider_name, model) + # If body already starts with a ## heading, don't add another if review_text.startswith("## "): - body = f"{SUMMARY_TAG}\n{review_text}\n\n---\n_Automated review by Kōan_" + body = f"{SUMMARY_TAG}\n{review_text}\n\n---\n{footer}" else: - body = f"{SUMMARY_TAG}\n## Code Review\n\n{review_text}\n\n---\n_Automated review by Kōan_" + body = f"{SUMMARY_TAG}\n## Code Review\n\n{review_text}\n\n---\n{footer}" # Embed commit SHAs when provided; otherwise preserve from existing # comment so a re-review doesn't clobber prior incremental state. @@ -1440,6 +1457,16 @@ def run_review( triaged_files=_triaged_files, ) + # Resolve provider/model for footer attribution + from app.config import get_model_config as _get_model_config + from app.provider import get_provider_name + _review_models = _get_model_config() + review_model = ( + _review_models.get("review_mode") + or _review_models.get("mission", "") + ) + review_provider_name = get_provider_name() + # Step 3: Run provider review (read-only) notify_fn(f"Analyzing code changes on `{context['branch']}`...") raw_output, error = _run_claude_review(prompt, project_path) @@ -1539,6 +1566,8 @@ def run_review( posted, post_error = _post_review_comment( owner, repo, pr_number, review_body, existing_comment, commit_shas=current_shas or None, + provider_name=review_provider_name, + model=review_model, ) # Step 8: Close the PR if the review decided closure is warranted diff --git a/koan/tests/test_review_runner.py b/koan/tests/test_review_runner.py index ff6afe6d5..af34fe8ea 100644 --- a/koan/tests/test_review_runner.py +++ b/koan/tests/test_review_runner.py @@ -22,6 +22,7 @@ _parse_review_json, _format_review_as_markdown, _extract_json_text, + _build_review_footer, _post_review_comment, _post_comment_replies, _fetch_pr_commit_shas, @@ -746,6 +747,36 @@ def test_code_snippet_in_output(self): assert "```" in md +# --------------------------------------------------------------------------- +# _build_review_footer +# --------------------------------------------------------------------------- + +class TestBuildReviewFooter: + def test_no_provider_no_model(self): + result = _build_review_footer() + assert result == "_Automated review by [Kōan](https://koan.anantys.com)_" + + def test_provider_only(self): + result = _build_review_footer(provider_name="claude") + assert "[Kōan](https://koan.anantys.com)" in result + assert "Claude" in result + + def test_model_only(self): + result = _build_review_footer(model="claude-opus-4-6") + assert "model claude-opus-4-6" in result + + def test_provider_and_model(self): + result = _build_review_footer(provider_name="claude", model="claude-sonnet-4-6") + assert "[Kōan](https://koan.anantys.com)" in result + assert "Claude" in result + assert "model claude-sonnet-4-6" in result + assert " · " in result + + def test_provider_capitalized(self): + result = _build_review_footer(provider_name="copilot", model="gpt-4o") + assert "Copilot" in result + + # --------------------------------------------------------------------------- # _post_review_comment # --------------------------------------------------------------------------- @@ -806,6 +837,18 @@ def test_legacy_review_gets_heading(self, mock_gh): body = [a for a in call_args if isinstance(a, str) and "Looks good" in a][0] assert "## Code Review" in body + @patch("app.review_runner.run_gh") + def test_footer_contains_provider_and_model(self, mock_gh): + """Footer includes provider name and model when provided.""" + _post_review_comment( + "owner", "repo", "42", "LGTM", + provider_name="claude", model="claude-opus-4-6", + ) + body = [a for a in mock_gh.call_args[0] if isinstance(a, str) and "LGTM" in a][0] + assert "[Kōan](https://koan.anantys.com)" in body + assert "Claude" in body + assert "model claude-opus-4-6" in body + # --------------------------------------------------------------------------- # run_review (integration, mocked externals) From a3df9be3c44f2304b1cd16c6e62aab63d657c46d Mon Sep 17 00:00:00 2001 From: "Nicolas R." Date: Wed, 3 Jun 2026 16:29:59 +0000 Subject: [PATCH 02/20] refactor(provider): move prompt/auth handling into providers Move provider-specific behavior out of shared CLI code and into the provider objects so cli_exec no longer special-cases provider names: - Providers declare stdin prompt-passing support and own the argv->stdin rewrite (Claude "-p" placeholder, Codex "exec -"). - Providers can opt into serialized CLI invocations via a process-wide file lock. Codex uses it to avoid token-refresh races between preflight probes, message formatting, and long review sessions. - Providers own final-message-file arg placement (Codex inserts the flag before the prompt) and auth-failure detection. Codex recognizes 401 and refresh-token-reuse errors in both stderr and streamed JSONL events. Detect Codex auth failures from stdout-safe runtime lines so a bare 401 stream failure pauses for auth and requeues the active mission instead of marking it failed. Keep the trusted stdout surface narrow ([cli] summaries, CLI failure lines, and structured JSON error events) to avoid false positives from quoted CI logs or source text. Fix a missing contextlib import that crashed the JSON-event branch of the runtime auth scan with NameError, and add regression coverage for both the JSON-event auth case and a benign event that merely quotes a 401 string. Consolidate the triplicated provider error-event-type set into a single PROVIDER_ERROR_EVENT_TYPES constant in provider/base.py. Co-Authored-By: Claude Opus 4.8 (1M context) --- docs/architecture/providers.md | 3 + docs/providers/codex.md | 18 +++ koan/app/cli_errors.py | 51 +++++- koan/app/cli_exec.py | 240 +++++++++++++++++++--------- koan/app/provider/__init__.py | 38 ++++- koan/app/provider/base.py | 74 ++++++++- koan/app/provider/codex.py | 113 +++++++++++-- koan/app/provider/copilot.py | 5 + koan/app/run.py | 67 ++++++++ koan/tests/test_cli_errors.py | 54 +++++++ koan/tests/test_cli_exec.py | 109 +++++++++++-- koan/tests/test_codex_provider.py | 57 +++++++ koan/tests/test_provider_modules.py | 43 +++++ koan/tests/test_run.py | 133 +++++++++++++++ 14 files changed, 899 insertions(+), 106 deletions(-) diff --git a/docs/architecture/providers.md b/docs/architecture/providers.md index 89d10fc61..c05981fa5 100644 --- a/docs/architecture/providers.md +++ b/docs/architecture/providers.md @@ -11,6 +11,9 @@ Providers are responsible for: - resolving the executable and authentication assumptions; - mapping Koan tool permissions to provider-specific flags; - building commands for print or streaming execution; +- declaring how prompts can be moved from argv to stdin; +- declaring whether invocations must be serialized to protect shared provider + state such as rotating auth tokens; - normalizing output handling enough for mission execution code; - exposing provider capabilities without leaking provider details into unrelated modules. diff --git a/docs/providers/codex.md b/docs/providers/codex.md index 5941e82ff..038badb5c 100644 --- a/docs/providers/codex.md +++ b/docs/providers/codex.md @@ -172,6 +172,24 @@ Install the CLI: `npm install -g @openai/codex` Re-authenticate: `codex login --device-auth` +When debugging Kōan, test the same non-interactive path Kōan uses. A short +plain command like `codex exec 'say hello'` can succeed while the daemon path +fails during JSON streaming, stdin prompt passing, or final-message capture: + +```bash +printf 'say hello' | codex exec --json --output-last-message /tmp/koan-codex-last-message --sandbox workspace-write - +``` + +If Kōan runs as a background service, also verify that the service user has the +same `HOME`, `PATH`, `.codex/auth.json`, and `.env` values as your interactive +shell. Restart Kōan after re-authenticating so the daemon uses the refreshed +Codex auth state. + +Kōan serializes its own Codex CLI subprocesses to avoid concurrent token-refresh +races between preflight probes, message formatting, and long-running review +sessions. If Codex still reports `401 Unauthorized` or refresh-token reuse, Kōan +will pause for auth and requeue the active mission instead of marking it failed. + ### Rate limits Codex shares quota with your ChatGPT subscription. If you hit limits, diff --git a/koan/app/cli_errors.py b/koan/app/cli_errors.py index 7aec11a15..16a8dff9f 100644 --- a/koan/app/cli_errors.py +++ b/koan/app/cli_errors.py @@ -13,8 +13,8 @@ """ import re +import sys from enum import Enum -from typing import Optional class ErrorCategory(Enum): @@ -79,6 +79,47 @@ class ErrorCategory(Enum): _TERMINAL_RE = re.compile("|".join(_TERMINAL_PATTERNS), re.IGNORECASE) +def _detect_auth_for_provider( + *, + stdout_text: str, + stderr_text: str, + provider_name: str, + exit_code: int, +) -> bool: + """Delegate provider-specific auth detection to the provider object. + + Mirrors ``quota_handler._detect_quota_for_provider``: resolve via the public + ``get_provider_by_name`` API (which normalizes the name and raises a clean + ``KeyError`` on unknown names), and degrade conservatively to ``False`` on + any failure. ``classify_cli_error`` runs on some unwrapped call paths, so a + provider-side bug must never crash the caller — it is logged and swallowed. + """ + if not provider_name: + return False + try: + from app.provider import get_provider_by_name + + provider = get_provider_by_name(provider_name) + return provider.detect_auth_failure( + stdout_text=stdout_text, + stderr_text=stderr_text, + exit_code=exit_code, + ) + except KeyError as e: + print( + f"[cli_errors] unknown provider {provider_name!r}: {e}", + file=sys.stderr, + ) + return False + except Exception as e: + print( + f"[cli_errors] provider auth detector failed " + f"for {provider_name!r}: {e}", + file=sys.stderr, + ) + return False + + def classify_cli_error( exit_code: int, stdout: str = "", @@ -128,6 +169,14 @@ def classify_cli_error( if _AUTH_RE.search(combined): return ErrorCategory.AUTH + if _detect_auth_for_provider( + stdout_text=stdout, + stderr_text=stderr, + provider_name=provider_name, + exit_code=exit_code, + ): + return ErrorCategory.AUTH + # Terminal errors — don't retry if _TERMINAL_RE.search(combined): return ErrorCategory.TERMINAL diff --git a/koan/app/cli_exec.py b/koan/app/cli_exec.py index 2064f997f..200f20970 100644 --- a/koan/app/cli_exec.py +++ b/koan/app/cli_exec.py @@ -2,9 +2,8 @@ Prevents prompts from leaking into ``ps`` process listings and avoids OS ``ARG_MAX`` failures by writing them to a temporary file (``0o600``) and -redirecting that file as the subprocess stdin. Claude-style ``-p`` -arguments become the short placeholder ``@stdin``; Codex ``exec`` prompts -become ``-``, which Codex reads from stdin. +redirecting that file as the subprocess stdin. Providers decide how their +prompt argument is rewritten to consume stdin. Providers that consume stdin for the prompt (making it unavailable for the agent's own tool calls) skip this mechanism and pass the prompt @@ -13,13 +12,17 @@ import contextlib import os +import re import subprocess import sys import tempfile import threading import time +from pathlib import Path +from types import TracebackType from typing import Callable, List, Optional, Sequence, Tuple +from app.provider.base import CLIProvider from app.run_log import log_safe as _log_cli, suppress_logged STDIN_PLACEHOLDER = "@stdin" @@ -28,56 +31,119 @@ # explicit timeout, but this guards against future callers forgetting. DEFAULT_TIMEOUT = 600 # 10 minutes +_PROVIDER_LOCK_DIR = "/tmp" +_FALLBACK_PROVIDER = CLIProvider() -def _uses_stdin_passing() -> bool: - """Check if the current provider supports stdin-based prompt passing. - Copilot CLI consumes stdin when reading the ``@stdin`` prompt, - leaving nothing for its internal agent's tool calls (e.g. - ``cat /dev/stdin``). For these providers we pass the prompt - directly as a ``-p`` argument instead. - """ +def _get_cli_provider() -> CLIProvider: try: - from app.provider import get_provider_name - return get_provider_name() not in ("copilot",) + from app.provider import get_provider + + return get_provider() except Exception as e: print(f"[cli_exec] Provider check failed: {e}", file=sys.stderr) - return True + return _FALLBACK_PROVIDER -def prepare_prompt_file(cmd: List[str]) -> Tuple[List[str], Optional[str]]: - """Extract the prompt from *cmd* and write it to a secure temp file. +def _uses_stdin_passing(provider: Optional[CLIProvider] = None) -> bool: + """Check if the current provider supports stdin-based prompt passing. - Returns ``(modified_cmd, temp_file_path)``. Claude-style commands are - rewritten from ``-p `` to ``-p @stdin``. Codex commands are - rewritten from ``codex exec ... `` to ``codex exec ... -``. - If no supported prompt argument is found, it already uses a stdin - marker, or the current provider does not support stdin-based prompt - passing, returns ``(cmd, None)`` unchanged. + Some providers consume stdin when reading the prompt, leaving nothing for + their internal tool calls. Those providers opt out through their provider + implementation. """ - if not _uses_stdin_passing(): - return cmd, None + return (provider or _get_cli_provider()).supports_stdin_prompt_passing() - prompt_idx: Optional[int] = None - prompt_stdin_marker = STDIN_PLACEHOLDER - try: - prompt_idx = cmd.index("-p") + 1 - except ValueError: - if ( - len(cmd) >= 3 - and os.path.basename(cmd[0]) == "codex" - and cmd[1] == "exec" - and cmd[-1] != "-" - and not cmd[-1].startswith("-") - ): - prompt_idx = len(cmd) - 1 - prompt_stdin_marker = "-" - - if prompt_idx is None or prompt_idx >= len(cmd): + +def _lock_path(lock_name: str) -> str: + safe = re.sub(r"[^A-Za-z0-9_.-]+", "-", lock_name.strip()).strip(".-") + if not safe: + safe = "provider" + return os.path.join(_PROVIDER_LOCK_DIR, f"koan-{safe}.lock") + + +class _ProviderInvocationLock: + """Optional process-wide file lock for provider CLI invocations. + + Most providers can run concurrently. Providers whose CLIs mutate shared + auth/session state can opt into serialization by returning a lock name from + ``CLIProvider.invocation_lock_name()``. + """ + + def __init__(self, lock_name: str): + self._lock_name = lock_name.strip() + self._fh = None + # True only after the exclusive lock is actually held. When False with a + # non-empty lock name, the invocation runs UNSERIALIZED — callers may + # inspect this to detect the degraded state. + self.acquired = False + + def __enter__(self) -> "_ProviderInvocationLock": + if not self._lock_name: + return self + try: + import fcntl + + lock_path = _lock_path(self._lock_name) + Path(lock_path).parent.mkdir(parents=True, exist_ok=True) + self._fh = open(lock_path, "a+") # noqa: SIM115 + fcntl.flock(self._fh.fileno(), fcntl.LOCK_EX) + self.acquired = True + except OSError as exc: + # Degrade loudly but do not abort the invocation: failing to lock + # is better surfaced than crashing every Codex run on a /tmp hiccup. + print( + f"[cli_exec] WARNING: serialization disabled for " + f"{self._lock_name!r} ({exc}); concurrent provider invocations " + "may race on shared auth/session state", + file=sys.stderr, + ) + self._close() + return self + + def __exit__( + self, + exc_type: Optional[type[BaseException]], + exc: Optional[BaseException], + tb: Optional[TracebackType], + ) -> None: + self.release() + + def _close(self) -> None: + if self._fh: + with contextlib.suppress(OSError): + self._fh.close() + self._fh = None + + def release(self) -> None: + if not self._fh: + return + try: + import fcntl + + fcntl.flock(self._fh.fileno(), fcntl.LOCK_UN) + except OSError as exc: + print(f"[cli_exec] Provider lock release failed: {exc}", file=sys.stderr) + finally: + self._close() + + +def prepare_prompt_file( + cmd: List[str], + provider: Optional[CLIProvider] = None, +) -> Tuple[List[str], Optional[str]]: + """Extract the prompt from *cmd* and write it to a secure temp file. + + Returns ``(modified_cmd, temp_file_path)``. If no supported prompt argument + is found, it already uses a stdin marker, or the current provider does not + support stdin-based prompt passing, returns ``(cmd, None)`` unchanged. + """ + provider = provider or _get_cli_provider() + if not _uses_stdin_passing(provider): return cmd, None - prompt = cmd[prompt_idx] - if prompt == prompt_stdin_marker: + new_cmd, prompt = provider.rewrite_prompt_for_stdin(cmd, STDIN_PLACEHOLDER) + if prompt is None: return cmd, None fd, path = tempfile.mkstemp(suffix=".md", prefix="koan-prompt-") @@ -87,8 +153,6 @@ def prepare_prompt_file(cmd: List[str]) -> Tuple[List[str], Optional[str]]: os.close(fd) os.chmod(path, 0o600) - new_cmd = cmd.copy() - new_cmd[prompt_idx] = prompt_stdin_marker return new_cmd, path @@ -99,7 +163,11 @@ def _cleanup_prompt_file(path: Optional[str]) -> None: os.unlink(path) -def run_cli(cmd, **kwargs) -> subprocess.CompletedProcess: +def run_cli( + cmd, + provider: Optional[CLIProvider] = None, + **kwargs, +) -> subprocess.CompletedProcess: """Run a CLI command with the prompt passed via temp-file stdin. Drop-in replacement for ``subprocess.run(cmd, stdin=DEVNULL, ...)``. @@ -107,48 +175,68 @@ def run_cli(cmd, **kwargs) -> subprocess.CompletedProcess: the caller does not provide one, preventing indefinite hangs. """ kwargs.setdefault("timeout", DEFAULT_TIMEOUT) - cmd, prompt_path = prepare_prompt_file(cmd) - if prompt_path: - try: - with open(prompt_path) as f: - kwargs.pop("stdin", None) - kwargs["stdin"] = f - return subprocess.run(cmd, **kwargs) - finally: - _cleanup_prompt_file(prompt_path) - else: - kwargs.setdefault("stdin", subprocess.DEVNULL) - return subprocess.run(cmd, **kwargs) + provider = provider or _get_cli_provider() + cmd, prompt_path = prepare_prompt_file(cmd, provider=provider) + with _ProviderInvocationLock(provider.invocation_lock_name()): + if prompt_path: + try: + with open(prompt_path) as f: + kwargs.pop("stdin", None) + kwargs["stdin"] = f + return subprocess.run(cmd, **kwargs) + finally: + _cleanup_prompt_file(prompt_path) + else: + kwargs.setdefault("stdin", subprocess.DEVNULL) + return subprocess.run(cmd, **kwargs) def popen_cli( - cmd, **kwargs + cmd, + provider: Optional[CLIProvider] = None, + **kwargs, ) -> Tuple[subprocess.Popen, Callable[[], None]]: """Start a :class:`~subprocess.Popen` process with the prompt via temp-file stdin. Returns ``(proc, cleanup)`` where *cleanup()* **must** be called after the process exits to close the file handle and delete the temp file. """ - cmd, prompt_path = prepare_prompt_file(cmd) - if prompt_path: - stdin_file = open(prompt_path) # noqa: SIM115 - kwargs.pop("stdin", None) - kwargs["stdin"] = stdin_file - try: - proc = subprocess.Popen(cmd, **kwargs) - except Exception: - stdin_file.close() - _cleanup_prompt_file(prompt_path) - raise - - def cleanup(): - stdin_file.close() - _cleanup_prompt_file(prompt_path) - - return proc, cleanup - else: + provider = provider or _get_cli_provider() + cmd, prompt_path = prepare_prompt_file(cmd, provider=provider) + cli_lock = _ProviderInvocationLock(provider.invocation_lock_name()) + cli_lock.__enter__() + # One outer guard so the lock is released on ANY failure after acquisition — + # including open(prompt_path) below, which sits before the Popen try/except. + # On the success path we return normally (no exception), so the lock stays + # held until the returned cleanup()/release runs. + try: + if prompt_path: + stdin_file = open(prompt_path) # noqa: SIM115 + kwargs.pop("stdin", None) + kwargs["stdin"] = stdin_file + try: + proc = subprocess.Popen(cmd, **kwargs) + except Exception: + stdin_file.close() + raise + + def cleanup(): + stdin_file.close() + _cleanup_prompt_file(prompt_path) + cli_lock.release() + + return proc, cleanup + kwargs.setdefault("stdin", subprocess.DEVNULL) - return subprocess.Popen(cmd, **kwargs), lambda: None + proc = subprocess.Popen(cmd, **kwargs) + return proc, cli_lock.release + except Exception: + # Any failure after the lock was taken (open(), Popen, ...) must release + # the lock and remove the temp prompt file. _cleanup_prompt_file tolerates + # a None path (the no-prompt branch). + _cleanup_prompt_file(prompt_path) + cli_lock.release() + raise class StreamResult: diff --git a/koan/app/provider/__init__.py b/koan/app/provider/__init__.py index 38a118155..c45a05c47 100644 --- a/koan/app/provider/__init__.py +++ b/koan/app/provider/__init__.py @@ -34,6 +34,7 @@ from app.provider.base import ( # noqa: F401 CLIProvider, CLAUDE_TOOLS, + PROVIDER_ERROR_EVENT_TYPES, TOOL_NAME_MAP, ) @@ -45,6 +46,34 @@ from app.provider.ollama_launch import OllamaLaunchProvider # noqa: F401 +def _extract_provider_error_preview(stdout: str) -> str: + """Return the most useful direct provider error from JSONL stdout.""" + previews: List[str] = [] + for line in (stdout or "").splitlines(): + stripped = line.strip() + if not stripped: + continue + try: + event = json.loads(stripped) + except json.JSONDecodeError: + continue + if not isinstance(event, dict): + continue + etype = str(event.get("type") or "") + if etype not in PROVIDER_ERROR_EVENT_TYPES: + continue + message = event.get("message") + if isinstance(message, str) and message.strip(): + previews.append(message.strip()) + continue + error = event.get("error") + if isinstance(error, dict): + err_message = error.get("message") + if isinstance(err_message, str) and err_message.strip(): + previews.append(err_message.strip()) + return previews[-1] if previews else "" + + def _format_cli_error(returncode: int, stdout: str, stderr: str) -> str: """Build a diagnostic message for non-zero CLI exits. @@ -57,7 +86,8 @@ def _format_cli_error(returncode: int, stdout: str, stderr: str) -> str: if err: parts.append(f"stderr={err[:300]}") if out and not err: - parts.append(f"stdout={out[:300]}") + preview = _extract_provider_error_preview(out) or out + parts.append(f"stdout={preview[:300]}") return "CLI invocation failed: " + " | ".join(parts) @@ -772,10 +802,7 @@ def run_command_streaming( suffix=".txt", ) os.close(fd) - last_message_args = provider.build_last_message_file_args(last_message_path) - if last_message_args and cmd: - # Codex requires exec flags before the final prompt argument. - cmd = [*cmd[:-1], *last_message_args, cmd[-1]] + cmd = provider.add_last_message_file_args(cmd, last_message_path) print(f"[cli] Starting {provider.name or 'provider'} CLI session", flush=True) @@ -790,6 +817,7 @@ def run_command_streaming( try: proc, cleanup = popen_cli( cmd, + provider=provider, stdout=subprocess.PIPE, stderr=subprocess.PIPE, encoding="utf-8", diff --git a/koan/app/provider/base.py b/koan/app/provider/base.py index 1fda33dd7..2cde41161 100644 --- a/koan/app/provider/base.py +++ b/koan/app/provider/base.py @@ -1,7 +1,7 @@ """Base class and constants for CLI provider abstraction.""" import shutil -from typing import List, Optional, Tuple +from typing import List, Optional, Sequence, Tuple # --------------------------------------------------------------------------- @@ -23,6 +23,16 @@ "Skill": "skill", } +# JSONL ``type`` values that signal a provider-level error event in streamed +# CLI output. Shared by error-preview extraction, runtime auth detection, and +# the Codex provider so the recognized set stays in sync across modules. +PROVIDER_ERROR_EVENT_TYPES = { + "error", + "turn.failed", + "response.failed", + "task.failed", +} + # --------------------------------------------------------------------------- # Base class @@ -58,6 +68,47 @@ def build_prompt_args(self, prompt: str) -> List[str]: """Build args for passing a prompt to the CLI.""" raise NotImplementedError + def supports_stdin_prompt_passing(self) -> bool: + """Return True if Kōan may move the prompt from argv to stdin. + + Providers that need stdin for their own tool calls should return False. + """ + return True + + def rewrite_prompt_for_stdin( + self, + cmd: Sequence[str], + stdin_marker: str, + ) -> Tuple[List[str], Optional[str]]: + """Rewrite *cmd* to read the prompt from stdin. + + Returns ``(rewritten_cmd, prompt_text)``. ``prompt_text`` is ``None`` + when no rewrite is possible or needed. + + The default supports Claude-style commands where ``-p`` is followed by + the prompt argument. + """ + cmd_list = list(cmd) + try: + prompt_idx = cmd_list.index("-p") + 1 + except ValueError: + return cmd_list, None + if prompt_idx >= len(cmd_list): + return cmd_list, None + prompt = cmd_list[prompt_idx] + if prompt == stdin_marker: + return cmd_list, None + rewritten = cmd_list.copy() + rewritten[prompt_idx] = stdin_marker + return rewritten, prompt + + def invocation_lock_name(self) -> str: + """Return a process-wide lock name for serialized CLI invocations. + + Empty string means invocations can run concurrently. + """ + return "" + def build_system_prompt_args(self, system_prompt: str) -> List[str]: """Build args for passing a system prompt to the CLI. @@ -172,6 +223,13 @@ def build_last_message_file_args(self, path: str) -> List[str]: """Build args that ask the provider to write its final assistant text.""" return [] + def add_last_message_file_args(self, cmd: List[str], path: str) -> List[str]: + """Insert final-message-file args into an already-built command.""" + args = self.build_last_message_file_args(path) + if not args: + return cmd + return [*cmd, *args] + def build_thinking_args( self, enabled: bool = False, budget_tokens: int = 0, ) -> List[str]: @@ -280,6 +338,20 @@ def detect_quota_exhaustion( """ return False + def detect_auth_failure( + self, + stdout_text: str = "", + stderr_text: str = "", + exit_code: int = 0, + ) -> bool: + """Return True when provider output is an auth/session failure. + + Providers own this because auth wording and output structure differ. + Generic classifier code handles shared auth text; providers can add + structured or provider-specific cases without leaking patterns upward. + """ + return False + @staticmethod def _line_has_error_marker(line: str, markers: tuple) -> bool: """Return True when ``line`` contains at least one marker (case-insensitive). diff --git a/koan/app/provider/codex.py b/koan/app/provider/codex.py index 550edf9ac..6b606d063 100644 --- a/koan/app/provider/codex.py +++ b/koan/app/provider/codex.py @@ -1,13 +1,14 @@ """OpenAI Codex CLI provider implementation.""" import json +import os import re import shutil import subprocess import sys -from typing import Any, List, Optional, Tuple +from typing import Any, List, Optional, Sequence, Tuple -from app.provider.base import CLIProvider +from app.provider.base import CLIProvider, PROVIDER_ERROR_EVENT_TYPES _CODEX_QUOTA_PATTERNS = [ @@ -25,13 +26,6 @@ _CODEX_QUOTA_RE = re.compile("|".join(_CODEX_QUOTA_PATTERNS), re.IGNORECASE) -_CODEX_ERROR_EVENT_TYPES = { - "error", - "turn.failed", - "response.failed", - "task.failed", -} - _CODEX_ERROR_KEYS = { "code", "error", @@ -43,6 +37,15 @@ "type", } +_CODEX_AUTH_PATTERNS = [ + r"\b401\s+Unauthorized\b", + r"unexpected\s+status\s+401", + r"access\s+token\s+could\s+not\s+be\s+refreshed", + r"refresh\s+token\s+was\s+already\s+used", +] + +_CODEX_AUTH_RE = re.compile("|".join(_CODEX_AUTH_PATTERNS), re.IGNORECASE) + class CodexProvider(CLIProvider): """OpenAI Codex CLI provider. @@ -96,6 +99,28 @@ def build_prompt_args(self, prompt: str) -> List[str]: # Codex non-interactive mode: codex exec "prompt" return ["exec", prompt] + def rewrite_prompt_for_stdin( + self, + cmd: Sequence[str], + stdin_marker: str, + ) -> Tuple[List[str], Optional[str]]: + cmd_list = list(cmd) + if not ( + len(cmd_list) >= 3 + and os.path.basename(cmd_list[0]) == self.binary() + and cmd_list[1] == "exec" + and cmd_list[-1] != "-" + and not cmd_list[-1].startswith("-") + ): + return cmd_list, None + prompt = cmd_list[-1] + rewritten = cmd_list.copy() + rewritten[-1] = "-" + return rewritten, prompt + + def invocation_lock_name(self) -> str: + return "codex-cli" + def build_tool_args( self, allowed_tools: Optional[List[str]] = None, @@ -135,6 +160,12 @@ def supports_last_message_file(self) -> bool: def build_last_message_file_args(self, path: str) -> List[str]: return ["--output-last-message", path] + def add_last_message_file_args(self, cmd: List[str], path: str) -> List[str]: + args = self.build_last_message_file_args(path) + if not args or not cmd: + return cmd + return [*cmd[:-1], *args, cmd[-1]] + def build_max_turns_args(self, max_turns: int = 0) -> List[str]: # Codex CLI does not support --max-turns. # codex exec runs to completion. @@ -197,7 +228,7 @@ def _plain_stdout_quota_line(self, line: str, exit_code: int) -> bool: def _event_has_quota_error(self, event: dict[str, Any]) -> bool: event_type = str(event.get("type") or "").lower() - if event_type not in _CODEX_ERROR_EVENT_TYPES: + if event_type not in PROVIDER_ERROR_EVENT_TYPES: return False return _CODEX_QUOTA_RE.search(self._error_event_text(event)) is not None @@ -224,6 +255,56 @@ def _error_event_text(self, value: Any) -> str: return "\n".join(p for p in parts if p) + def detect_auth_failure( + self, + stdout_text: str = "", + stderr_text: str = "", + exit_code: int = 0, + ) -> bool: + """Detect Codex authentication/session failures. + + Codex may emit auth failures as JSONL stdout events during stream + reconnects rather than plain stderr. For JSON events only direct + provider error fields are inspected, so command output cannot create + false positives. + + Non-JSON stdout lines, however, are scanned raw with ``_CODEX_AUTH_RE``. + Callers must pre-filter stdout to trusted runtime lines before passing + it here — ``run._cli_runtime_auth_signal`` does this (only ``[cli]`` + summaries, ``CLI invocation failed:`` lines, and structured error + events reach us). The exit-code path in ``cli_errors`` passes unfiltered + stdout, but that runs only after ``classify_cli_error``'s generic + ``_AUTH_RE`` check, so the marginal false-positive surface is limited to + Codex-specific phrases (e.g. refresh-token reuse) that benign logs are + very unlikely to contain. + """ + if exit_code == 0: + return False + + stderr_text = stderr_text or "" + stdout_text = stdout_text or "" + + if _CODEX_AUTH_RE.search(stderr_text): + return True + + for line in stdout_text.splitlines(): + stripped = line.strip() + if not stripped: + continue + try: + event = json.loads(stripped) + except json.JSONDecodeError: + if _CODEX_AUTH_RE.search(stripped): + return True + continue + + if isinstance(event, dict) and _CODEX_AUTH_RE.search( + self._error_event_text(event) + ): + return True + + return False + def build_command( self, prompt: str, @@ -286,8 +367,11 @@ def check_quota_available(self, project_path: str, timeout: int = 15) -> Tuple[b cmd = [self.binary(), "exec", "--sandbox", "workspace-write", "ok"] try: - result = subprocess.run( + from app.cli_exec import run_cli + + result = run_cli( cmd, + provider=self, capture_output=True, text=True, timeout=timeout, @@ -300,6 +384,13 @@ def check_quota_available(self, project_path: str, timeout: int = 15) -> Tuple[b ): combined = (result.stderr or "") + "\n" + (result.stdout or "") return False, combined + if self.detect_auth_failure( + stdout_text=result.stdout or "", + stderr_text=result.stderr or "", + exit_code=result.returncode, + ): + combined = (result.stderr or "") + "\n" + (result.stdout or "") + return False, combined return True, "" except subprocess.TimeoutExpired: diff --git a/koan/app/provider/copilot.py b/koan/app/provider/copilot.py index d6537f7ba..a6d857236 100644 --- a/koan/app/provider/copilot.py +++ b/koan/app/provider/copilot.py @@ -65,6 +65,11 @@ def build_prompt_args(self, prompt: str) -> List[str]: prefix = ["copilot"] if self._is_gh_mode else [] return prefix + ["-p", prompt] + def supports_stdin_prompt_passing(self) -> bool: + # Copilot consumes stdin for the prompt, which prevents its own tool + # calls from using stdin later in the session. + return False + def build_tool_args( self, allowed_tools: Optional[List[str]] = None, diff --git a/koan/app/run.py b/koan/app/run.py index dd5945fb1..fcabb6bb5 100644 --- a/koan/app/run.py +++ b/koan/app/run.py @@ -19,6 +19,7 @@ - Colored log output with TTY detection """ +import contextlib import os import json import signal @@ -2670,6 +2671,13 @@ def _classify_and_handle_cli_error( if cli_runtime_quota_signal(stdout_text): category = ErrorCategory.QUOTA + if category != ErrorCategory.AUTH and not trust_stdout: + if _cli_runtime_auth_signal( + stdout_text=stdout_text, + provider_name=provider_name, + exit_code=exit_code, + ): + category = ErrorCategory.AUTH if category == ErrorCategory.AUTH: _handle_auth_error( provider_label=provider_label, @@ -2693,6 +2701,65 @@ def _classify_and_handle_cli_error( return False +def _cli_runtime_auth_signal( + *, + stdout_text: str, + provider_name: str, + exit_code: int, +) -> bool: + """Detect provider auth failures from stdout-safe runtime lines. + + Skill stdout is normally DATA, so broad stdout auth scans can false-positive + on quoted CI logs or source text. Codex, however, reports real stream auth + failures on stdout. Keep the trusted surface narrow: raw provider JSON + events, Koan's ``[cli]`` stream summaries, and CLI failure summaries. + """ + if exit_code == 0 or not stdout_text or not provider_name: + return False + + from app.provider.base import PROVIDER_ERROR_EVENT_TYPES + + runtime_lines: list[str] = [] + for line in stdout_text.splitlines(): + stripped = line.strip() + if not stripped: + continue + if stripped.startswith("[cli]") or "CLI invocation failed:" in stripped: + runtime_lines.append(stripped) + continue + if not stripped.startswith("{"): + continue + with contextlib.suppress(json.JSONDecodeError): + event = json.loads(stripped) + if ( + isinstance(event, dict) + and str(event.get("type") or "") in PROVIDER_ERROR_EVENT_TYPES + ): + runtime_lines.append(stripped) + + if not runtime_lines: + return False + + try: + from app.provider import get_provider_by_name + + provider = get_provider_by_name(provider_name) + return provider.detect_auth_failure( + stdout_text="\n".join(runtime_lines), + stderr_text="", + exit_code=exit_code, + ) + except KeyError as e: + print(f"[run] unknown provider {provider_name!r}: {e}", file=sys.stderr) + return False + except Exception as e: + print( + f"[run] runtime auth detector failed for {provider_name!r}: {e}", + file=sys.stderr, + ) + return False + + def _probe_exit0_quota( *, provider_name: str, diff --git a/koan/tests/test_cli_errors.py b/koan/tests/test_cli_errors.py index 1e2c41c0f..3a7202d6c 100644 --- a/koan/tests/test_cli_errors.py +++ b/koan/tests/test_cli_errors.py @@ -227,6 +227,60 @@ def test_real_claude_logged_out(self): result = classify_cli_error(1, stderr=stderr) assert result == ErrorCategory.AUTH + def test_codex_jsonl_unauthorized_is_auth(self): + stdout = "\n".join([ + json.dumps({"type": "thread.started", "thread_id": "t1"}), + json.dumps({"type": "turn.started"}), + json.dumps({ + "type": "error", + "message": ( + 'unexpected status 401 Unauthorized: {"detail":"Unauthorized"}' + ), + }), + json.dumps({"type": "turn.failed"}), + ]) + result = classify_cli_error(1, stdout=stdout, provider_name="codex") + assert result == ErrorCategory.AUTH + + def test_codex_refresh_token_reuse_is_auth(self): + stderr = ( + "Error: Your access token could not be refreshed because your " + "refresh token was already used. Please log out and sign in again." + ) + result = classify_cli_error(1, stderr=stderr, provider_name="codex") + assert result == ErrorCategory.AUTH + + # -- Provider auth detector resilience --------------------------------------- + + def test_unknown_provider_name_does_not_raise(self): + """An unknown provider name must classify without crashing. + + _detect_auth_for_provider resolves via get_provider_by_name, which + raises KeyError on unknown names; that must degrade to a normal + classification, not propagate. + """ + result = classify_cli_error( + 1, stderr="Error: something generic", provider_name="does-not-exist" + ) + assert isinstance(result, ErrorCategory) + assert result != ErrorCategory.AUTH + + def test_provider_auth_detector_exception_is_swallowed(self): + """A bug in a provider's detect_auth_failure must not crash the caller.""" + from unittest.mock import patch + + class _Boom: + def detect_auth_failure(self, **_kwargs): + raise RuntimeError("detector bug") + + with patch("app.provider.get_provider_by_name", return_value=_Boom()): + result = classify_cli_error( + 1, stderr="Error: generic failure", provider_name="codex" + ) + # Detector blew up → no AUTH from it, and no exception propagated. + assert isinstance(result, ErrorCategory) + assert result != ErrorCategory.AUTH + # -- False positive: loose quota patterns in stdout -------------------------- def test_no_false_positive_rate_limit_in_stdout(self): diff --git a/koan/tests/test_cli_exec.py b/koan/tests/test_cli_exec.py index 733eefbd3..8c64cf29e 100644 --- a/koan/tests/test_cli_exec.py +++ b/koan/tests/test_cli_exec.py @@ -15,6 +15,10 @@ stream_with_timeout, _cleanup_prompt_file, ) +from app.provider.claude import ClaudeProvider +from app.provider.codex import CodexProvider +from app.provider.copilot import CopilotProvider +from app.provider.local import LocalLLMProvider # --------------------------------------------------------------------------- @@ -24,23 +28,23 @@ class TestUsesStdinPassing: """Tests for _uses_stdin_passing() provider detection.""" - @patch("app.provider.get_provider_name", return_value="claude") + @patch("app.provider.get_provider", return_value=ClaudeProvider()) def test_claude_provider_uses_stdin(self, _mock): assert _uses_stdin_passing() is True - @patch("app.provider.get_provider_name", return_value="copilot") + @patch("app.provider.get_provider", return_value=CopilotProvider()) def test_copilot_provider_skips_stdin(self, _mock): assert _uses_stdin_passing() is False - @patch("app.provider.get_provider_name", return_value="local") + @patch("app.provider.get_provider", return_value=LocalLLMProvider()) def test_local_provider_uses_stdin(self, _mock): assert _uses_stdin_passing() is True - @patch("app.provider.get_provider_name", side_effect=ImportError("no provider")) + @patch("app.provider.get_provider", side_effect=ImportError("no provider")) def test_import_error_defaults_to_true(self, _mock): assert _uses_stdin_passing() is True - @patch("app.provider.get_provider_name", side_effect=RuntimeError("broken")) + @patch("app.provider.get_provider", side_effect=RuntimeError("broken")) def test_runtime_error_defaults_to_true(self, _mock): assert _uses_stdin_passing() is True @@ -125,7 +129,7 @@ def test_copilot_gh_mode(self): finally: _cleanup_prompt_file(path) - @patch("app.provider.get_provider_name", return_value="copilot") + @patch("app.provider.get_provider", return_value=CopilotProvider()) def test_copilot_provider_skips_stdin_passing(self, _mock): """Copilot provider should skip @stdin mechanism entirely.""" cmd = ["copilot", "-p", "my prompt", "--allow-all-tools"] @@ -133,7 +137,7 @@ def test_copilot_provider_skips_stdin_passing(self, _mock): assert new_cmd is cmd assert path is None - @patch("app.provider.get_provider_name", return_value="codex") + @patch("app.provider.get_provider", return_value=CodexProvider()) def test_codex_exec_prompt_uses_stdin_dash(self, _mock): """Codex exec reads '-' from stdin, so the prompt stays out of argv.""" cmd = ["codex", "exec", "--sandbox", "workspace-write", "my prompt"] @@ -148,7 +152,7 @@ def test_codex_exec_prompt_uses_stdin_dash(self, _mock): finally: _cleanup_prompt_file(path) - @patch("app.provider.get_provider_name", return_value="codex") + @patch("app.provider.get_provider", return_value=CodexProvider()) def test_codex_large_prompt_removed_from_argv(self, _mock): """Regression for OSError: Argument list too long when using Codex.""" prompt = "x" * 200_000 @@ -162,14 +166,14 @@ def test_codex_large_prompt_removed_from_argv(self, _mock): finally: _cleanup_prompt_file(path) - @patch("app.provider.get_provider_name", return_value="codex") + @patch("app.provider.get_provider", return_value=CodexProvider()) def test_codex_existing_stdin_dash_returns_unchanged(self, _mock): cmd = ["codex", "exec", "--json", "-"] new_cmd, path = prepare_prompt_file(cmd) assert new_cmd is cmd assert path is None - @patch("app.provider.get_provider_name", return_value="codex") + @patch("app.provider.get_provider", return_value=CodexProvider()) def test_codex_without_prompt_returns_unchanged(self, _mock): cmd = ["codex", "exec", "--json"] new_cmd, path = prepare_prompt_file(cmd) @@ -259,7 +263,7 @@ def test_removes_existing_stdin_kwarg(self, mock_run): call_args = mock_run.call_args assert call_args[1]["stdin"] != subprocess.DEVNULL - @patch("app.provider.get_provider_name", return_value="copilot") + @patch("app.provider.get_provider", return_value=CopilotProvider()) @patch("app.cli_exec.subprocess.run") def test_copilot_keeps_prompt_in_args(self, mock_run, _mock_provider): """Copilot provider: prompt stays in -p, stdin is DEVNULL.""" @@ -295,6 +299,20 @@ def test_explicit_timeout_not_overridden(self, mock_run): call_args = mock_run.call_args assert call_args[1]["timeout"] == 42 + @patch("app.provider.get_provider", return_value=CodexProvider()) + @patch("fcntl.flock") + @patch("app.cli_exec.subprocess.run") + def test_codex_run_cli_uses_file_lock(self, mock_run, mock_flock, _mock_provider): + import fcntl + + mock_run.return_value = subprocess.CompletedProcess([], 0, "ok", "") + cmd = ["codex", "exec", "prompt"] + + run_cli(cmd, capture_output=True, text=True) + + assert mock_flock.call_args_list[0][0][1] == fcntl.LOCK_EX + assert mock_flock.call_args_list[-1][0][1] == fcntl.LOCK_UN + # --------------------------------------------------------------------------- # popen_cli @@ -344,7 +362,7 @@ def test_stdin_is_file_object(self, mock_popen): assert hasattr(stdin_arg, "read") # it's a file object cleanup() - @patch("app.provider.get_provider_name", return_value="copilot") + @patch("app.provider.get_provider", return_value=CopilotProvider()) @patch("app.cli_exec.subprocess.Popen") def test_copilot_keeps_prompt_in_args(self, mock_popen, _mock_provider): """Copilot provider: popen keeps prompt in -p, stdin is DEVNULL.""" @@ -358,6 +376,73 @@ def test_copilot_keeps_prompt_in_args(self, mock_popen, _mock_provider): assert mock_popen.call_args[1]["stdin"] == subprocess.DEVNULL cleanup() + @patch("app.provider.get_provider", return_value=CodexProvider()) + @patch("fcntl.flock") + @patch("app.cli_exec.subprocess.Popen") + def test_codex_popen_lock_released_on_cleanup( + self, mock_popen, mock_flock, _mock_provider + ): + import fcntl + + mock_popen.return_value = MagicMock() + cmd = ["codex", "exec", "prompt"] + + _proc, cleanup = popen_cli(cmd) + + assert mock_flock.call_args_list[0][0][1] == fcntl.LOCK_EX + cleanup() + assert mock_flock.call_args_list[-1][0][1] == fcntl.LOCK_UN + + @patch("app.provider.get_provider", return_value=CodexProvider()) + @patch("fcntl.flock") + def test_codex_popen_releases_lock_when_prompt_open_fails( + self, mock_flock, _mock_provider + ): + """If open(prompt_path) fails after the lock is taken, it must release.""" + import fcntl + + real_open = open + + def fake_open(path, *args, **kwargs): + # Fail only on the temp prompt file; let the lock file open normally. + if str(path).endswith(".md"): + raise OSError("simulated prompt-file open failure") + return real_open(path, *args, **kwargs) + + cmd = ["codex", "exec", "prompt"] + with patch("app.cli_exec.open", side_effect=fake_open): + with pytest.raises(OSError): + popen_cli(cmd) + + assert mock_flock.call_args_list[0][0][1] == fcntl.LOCK_EX + assert mock_flock.call_args_list[-1][0][1] == fcntl.LOCK_UN + + +class TestProviderInvocationLock: + """Degraded-state behaviour of _ProviderInvocationLock.""" + + def test_no_lock_name_is_noop(self): + from app.cli_exec import _ProviderInvocationLock + + lock = _ProviderInvocationLock("") + with lock as entered: + assert entered is lock + assert lock.acquired is False + # release() on an unacquired lock must not raise. + lock.release() + + @patch("fcntl.flock", side_effect=OSError("flock unsupported")) + def test_flock_failure_degrades_without_raising(self, _mock_flock): + from app.cli_exec import _ProviderInvocationLock + + lock = _ProviderInvocationLock("codex-cli") + # Acquisition failure must degrade loudly, not raise. + with lock as entered: + assert entered is lock + assert lock.acquired is False + # Behaves as a no-op context; release stays safe. + lock.release() + # --------------------------------------------------------------------------- # stream_with_timeout diff --git a/koan/tests/test_codex_provider.py b/koan/tests/test_codex_provider.py index 129957ac5..88f5a5a53 100644 --- a/koan/tests/test_codex_provider.py +++ b/koan/tests/test_codex_provider.py @@ -125,6 +125,33 @@ def test_last_message_file_args(self): "/tmp/out.txt", ] + def test_add_last_message_file_args_before_prompt(self): + cmd = ["codex", "exec", "--json", "prompt"] + result = self.provider.add_last_message_file_args(cmd, "/tmp/out.txt") + assert result == [ + "codex", + "exec", + "--json", + "--output-last-message", + "/tmp/out.txt", + "prompt", + ] + + def test_rewrite_prompt_for_stdin_uses_dash(self): + cmd = ["codex", "exec", "--json", "prompt"] + rewritten, prompt = self.provider.rewrite_prompt_for_stdin(cmd, "@stdin") + assert rewritten == ["codex", "exec", "--json", "-"] + assert prompt == "prompt" + + def test_rewrite_prompt_for_stdin_existing_dash_unchanged(self): + cmd = ["codex", "exec", "--json", "-"] + rewritten, prompt = self.provider.rewrite_prompt_for_stdin(cmd, "@stdin") + assert rewritten == cmd + assert prompt is None + + def test_invocation_lock_name(self): + assert self.provider.invocation_lock_name() == "codex-cli" + # -- Max turns (no-op) -- def test_max_turns_args(self): @@ -368,6 +395,36 @@ def test_generic_error_proceeds_optimistically(self, mock_run): available, detail = self.provider.check_quota_available("/tmp/project") assert available is True + @patch("subprocess.run") + def test_auth_failure_blocks_preflight(self, mock_run): + mock_run.return_value = MagicMock( + returncode=1, + stdout=json.dumps({ + "type": "error", + "message": ( + 'unexpected status 401 Unauthorized: {"detail":"Unauthorized"}' + ), + }), + stderr="", + ) + available, detail = self.provider.check_quota_available("/tmp/project") + assert available is False + assert "401 Unauthorized" in detail + + @patch("subprocess.run") + def test_refresh_token_reuse_blocks_preflight(self, mock_run): + mock_run.return_value = MagicMock( + returncode=1, + stdout="", + stderr=( + "Error: Your access token could not be refreshed because " + "your refresh token was already used." + ), + ) + available, detail = self.provider.check_quota_available("/tmp/project") + assert available is False + assert "refresh token was already used" in detail + # --------------------------------------------------------------------------- # detect_quota_exhaustion diff --git a/koan/tests/test_provider_modules.py b/koan/tests/test_provider_modules.py index 95738b52a..ed5f91344 100644 --- a/koan/tests/test_provider_modules.py +++ b/koan/tests/test_provider_modules.py @@ -4,6 +4,7 @@ These modules had zero test coverage despite being used throughout the codebase. """ +import json import os import sys from unittest.mock import MagicMock, patch @@ -59,6 +60,23 @@ def test_build_permission_args_default_empty(self): p = CLIProvider() assert p.build_permission_args(skip_permissions=True) == [] + def test_stdin_prompt_passing_default_enabled(self): + p = CLIProvider() + assert p.supports_stdin_prompt_passing() is True + + def test_default_rewrite_prompt_for_stdin(self): + p = CLIProvider() + rewritten, prompt = p.rewrite_prompt_for_stdin( + ["provider", "-p", "hello", "--model", "m"], + "@stdin", + ) + assert rewritten == ["provider", "-p", "@stdin", "--model", "m"] + assert prompt == "hello" + + def test_default_invocation_lock_empty(self): + p = CLIProvider() + assert p.invocation_lock_name() == "" + def test_check_quota_default_always_available(self): p = CLIProvider() available, detail = p.check_quota_available("/some/path") @@ -304,6 +322,11 @@ def test_prompt_args_standalone(self, mock_which): p = CopilotProvider() assert p.build_prompt_args("hello") == ["-p", "hello"] + @patch("shutil.which", side_effect=lambda x: "/usr/bin/copilot" if x == "copilot" else None) + def test_stdin_prompt_passing_disabled(self, mock_which): + p = CopilotProvider() + assert p.supports_stdin_prompt_passing() is False + @patch("shutil.which", side_effect=lambda x: "/usr/bin/gh" if x == "gh" else None) def test_prompt_args_gh_mode(self, mock_which): p = CopilotProvider() @@ -1529,6 +1552,26 @@ def test_stdout_truncated_at_300(self): msg = _format_cli_error(1, long_out, "") assert f"stdout={'y' * 300}" in msg + def test_jsonl_error_preview_prefers_last_provider_error(self): + from app.provider import _format_cli_error + stdout = "\n".join([ + json.dumps({"type": "thread.started"}), + json.dumps({ + "type": "error", + "message": "Reconnecting... 2/5", + }), + json.dumps({ + "type": "error", + "message": ( + 'unexpected status 401 Unauthorized: {"detail":"Unauthorized"}' + ), + }), + json.dumps({"type": "turn.failed"}), + ]) + msg = _format_cli_error(1, stdout, "") + assert "unexpected status 401 Unauthorized" in msg + assert "thread.started" not in msg + # --------------------------------------------------------------------------- # _write_system_prompt_file / cleanup_managed_paths diff --git a/koan/tests/test_run.py b/koan/tests/test_run.py index ab2e6688d..4adad99ca 100644 --- a/koan/tests/test_run.py +++ b/koan/tests/test_run.py @@ -5200,6 +5200,65 @@ def test_auth_error_requeues_and_pauses(self, tmp_path): mock_finalize.assert_not_called() mock_pause.assert_called_once_with(koan_root, "auth") + def test_codex_unauthorized_requeues_and_pauses(self, tmp_path): + """Codex bare 401 stream failures should be auth, not normal failure.""" + from app.run import _handle_skill_dispatch + + koan_root = str(tmp_path) + instance = str(tmp_path / "instance") + (tmp_path / "instance").mkdir() + (tmp_path / "instance" / "journal").mkdir(parents=True) + (tmp_path / "koan").mkdir() + + mock_proc = self._make_mock_popen( + returncode=1, + stdout_lines=["[cli] event: turn.failed\n"], + stderr_content=( + "[review_runner] Provider review failed: CLI invocation failed: " + 'exit=1 | stdout=unexpected status 401 Unauthorized: ' + '{"detail":"Unauthorized"}\n' + ), + ) + + with patch("app.run.subprocess.Popen", side_effect=mock_proc._side_effect), \ + patch("app.run._get_koan_branch", return_value="main"), \ + patch("app.run._restore_koan_branch"), \ + patch("app.run._reset_terminal"), \ + patch("app.run.protected_phase", return_value=MagicMock( + __enter__=MagicMock(), __exit__=MagicMock(return_value=False) + )), \ + patch("app.run._notify") as mock_notify, \ + patch("app.run._notify_mission_end"), \ + patch("app.run._finalize_mission") as mock_finalize, \ + patch("app.run._requeue_mission_in_file") as mock_requeue, \ + patch("app.run._commit_instance"), \ + patch("app.run._sleep_between_runs"), \ + patch("app.run.set_status"), \ + patch("app.run.log"), \ + patch("app.provider.get_provider_name", return_value="codex"), \ + patch("app.skill_dispatch.dispatch_skill_mission", + return_value=["python3", "-m", "app.review_runner"]), \ + patch("app.mission_runner.run_post_mission"), \ + patch("app.pause_manager.create_pause") as mock_pause: + handled, _ = _handle_skill_dispatch( + mission_title="/review https://github.com/example/repo/pull/1", + project_name="test", + project_path=str(tmp_path), + koan_root=koan_root, + instance=instance, + run_num=1, + max_runs=20, + autonomous_mode="implement", + interval=30, + ) + + assert handled is True + mock_requeue.assert_called_once() + mock_finalize.assert_not_called() + mock_pause.assert_called_once_with(koan_root, "auth") + notify_text = mock_notify.call_args_list[-1][0][1] + assert "logged out" in notify_text.lower() + def test_post_mission_quota_requeues_and_pauses(self, tmp_path): """Quota detected in post-mission pipeline should requeue. @@ -7154,3 +7213,77 @@ def test_runtime_session_limit_in_stdout_still_honored(self): ) assert handled is True quota.assert_called_once() + + def test_codex_runtime_auth_stdout_still_honored(self): + from app import run + + transcript = ( + "[cli] event: thread.started\n" + '[cli] error: unexpected status 401 Unauthorized: {"detail":"Unauthorized"}\n' + "[cli] event: turn.failed\n" + ) + common = {**self._COMMON, "provider_name": "codex", "provider_label": "Codex"} + with patch.object(run, "_handle_quota_error") as quota, \ + patch.object(run, "_handle_auth_error") as auth: + handled = run._classify_and_handle_cli_error( + 1, transcript, "", trust_stdout=False, **common + ) + assert handled is True + auth.assert_called_once() + quota.assert_not_called() + + def test_plain_skill_stdout_unauthorized_still_ignored(self): + from app import run + + transcript = "Test fixture includes text: HTTP 401 Unauthorized\n" + common = {**self._COMMON, "provider_name": "codex", "provider_label": "Codex"} + with patch.object(run, "_handle_quota_error") as quota, \ + patch.object(run, "_handle_auth_error") as auth: + handled = run._classify_and_handle_cli_error( + 1, transcript, "", trust_stdout=False, **common + ) + assert handled is False + auth.assert_not_called() + quota.assert_not_called() + + def test_codex_runtime_auth_json_event_still_honored(self): + """Raw provider JSON error events on stdout must classify as auth. + + Regression: the JSON-event branch of the runtime auth scan must run + without raising (previously crashed on a missing ``contextlib``). + """ + from app import run + + transcript = ( + '{"type": "thread.started", "thread_id": "t1"}\n' + '{"type": "error", "message": "unexpected status 401 Unauthorized: ' + '{\\"detail\\":\\"Unauthorized\\"}"}\n' + '{"type": "turn.failed"}\n' + ) + common = {**self._COMMON, "provider_name": "codex", "provider_label": "Codex"} + with patch.object(run, "_handle_quota_error") as quota, \ + patch.object(run, "_handle_auth_error") as auth: + handled = run._classify_and_handle_cli_error( + 1, transcript, "", trust_stdout=False, **common + ) + assert handled is True + auth.assert_called_once() + quota.assert_not_called() + + def test_benign_json_event_quoting_unauthorized_ignored(self): + """A non-error JSON event that merely quotes '401' is not an auth failure.""" + from app import run + + transcript = ( + '{"type": "item.completed", "message": "test output: HTTP 401 ' + 'Unauthorized appears in this CI log"}\n' + ) + common = {**self._COMMON, "provider_name": "codex", "provider_label": "Codex"} + with patch.object(run, "_handle_quota_error") as quota, \ + patch.object(run, "_handle_auth_error") as auth: + handled = run._classify_and_handle_cli_error( + 1, transcript, "", trust_stdout=False, **common + ) + assert handled is False + auth.assert_not_called() + quota.assert_not_called() From 26d5e2849cef8b0112456a7bd6792f9607773c4e Mon Sep 17 00:00:00 2001 From: "Nicolas R." Date: Wed, 3 Jun 2026 18:20:27 +0000 Subject: [PATCH 03/20] test(codex): cover JSONL usage token accounting Add a post-mission test that feeds a Codex "event_msg" token_count JSONL event through run_post_mission and asserts both the budget state and the cost-tracker rows. It verifies that cached_input_tokens are treated as cache-read and subtracted from counted input, that the session and weekly token totals reflect billable input plus output, that the run counter increments, and that the emitted usage row carries the model, mode, and cache-read counts. Document the recognized Codex JSONL usage events in docs/providers/codex.md: "turn.completed" events with a "usage" object and "event_msg" rollout events whose "payload.type" is "token_count", along with the cache-read subtraction rule and the latest-snapshot-wins behavior when multiple usage-bearing events are present. This is test and documentation only; no production behavior changes. Co-Authored-By: Claude Opus 4.8 (1M context) --- docs/providers/codex.md | 10 +++ koan/tests/test_mission_runner.py | 102 ++++++++++++++++++++++++++++++ 2 files changed, 112 insertions(+) diff --git a/docs/providers/codex.md b/docs/providers/codex.md index 038badb5c..a5b50ff8d 100644 --- a/docs/providers/codex.md +++ b/docs/providers/codex.md @@ -113,6 +113,16 @@ Koan tracks token usage from Codex JSON output when available. This internal estimate drives autonomous mode downgrades (`deep` -> `implement` -> `review` -> `wait`) but is separate from hard provider quota detection. +Recognized Codex JSONL usage events are: + +- `turn.completed` events with a `usage` object. +- `event_msg` rollout events where `payload.type` is `token_count` and + `payload.info.total_token_usage` contains the token counters. + +In both formats, Koan treats `cached_input_tokens` as cache-read tokens and +subtracts them from counted input tokens before updating the internal budget. +When multiple usage-bearing events are present, the latest snapshot wins. + For Codex subscription accounts where you want to ignore internal estimates and only react to real provider quota/session-limit errors, set: diff --git a/koan/tests/test_mission_runner.py b/koan/tests/test_mission_runner.py index ae8a720fd..e7cfd3f87 100644 --- a/koan/tests/test_mission_runner.py +++ b/koan/tests/test_mission_runner.py @@ -3,6 +3,8 @@ import json import os import sys +from contextlib import ExitStack +from datetime import date from pathlib import Path from unittest.mock import patch @@ -2881,6 +2883,106 @@ def test_returns_empty_for_missing_file(self): class TestCostTrackingFailedFlag: """Test cost_tracking_failed flag in run_post_mission result.""" + def test_codex_jsonl_updates_usage_state_and_cost_rows(self, tmp_path): + """Codex JSONL usage updates both budget state and usage JSONL rows.""" + from app import cost_tracker + from app.mission_runner import run_post_mission + + instance_dir = tmp_path / "instance" + instance_dir.mkdir() + stdout_file = tmp_path / "codex.jsonl" + stderr_file = tmp_path / "codex.err" + stdout_file.write_text("\n".join([ + json.dumps({"type": "thread.started"}), + json.dumps({ + "type": "event_msg", + "payload": { + "type": "token_count", + "info": { + "model": "gpt-5.4", + "total_token_usage": { + "input_tokens": 26549, + "cached_input_tokens": 22272, + "output_tokens": 1590, + "reasoning_output_tokens": 1152, + "total_tokens": 28139, + }, + }, + }, + }), + ])) + stderr_file.write_text("") + + patches = [ + patch("app.usage_estimator.load_config", return_value={ + "usage": { + "session_token_limit": 500000, + "weekly_token_limit": 5000000, + } + }), + patch("app.projects_config.load_projects_config", return_value={}), + patch("app.quota_handler.handle_quota_exhaustion", return_value=None), + patch("app.mission_runner.archive_pending", return_value=False), + patch("app.mission_runner.trigger_reflection", return_value=False), + patch("app.mission_runner._run_mission_verification", return_value=None), + patch("app.mission_runner._run_quality_pipeline", return_value={}), + patch("app.mission_runner._run_lint_gate", return_value=None), + patch("app.mission_runner.check_security_review", return_value=True), + patch("app.mission_runner.check_auto_merge", return_value=None), + patch("app.mission_runner._record_session_outcome"), + patch("app.mission_runner._record_skill_metric"), + patch("app.mission_runner._publish_jira_outcome", return_value={ + "published": "false", + "reason": "test", + }), + patch("app.bandit.load_bandit_state", return_value={}), + patch("app.bandit.update_bandit"), + patch("app.bandit.save_bandit_state"), + patch("app.daily_snapshot.update_daily_snapshot"), + patch("app.mission_runner._check_pipeline_timeout_rate"), + patch("app.mission_runner._fire_post_mission_hook", return_value={}), + patch("app.mission_runner._write_pipeline_summary"), + patch("app.mission_runner._notify_pipeline_failures"), + patch("app.mission_runner._notify_mission_result"), + ] + with ExitStack() as stack: + for item in patches: + stack.enter_context(item) + result = run_post_mission( + instance_dir=str(instance_dir), + project_name="koan", + project_path=str(tmp_path), + run_num=1, + exit_code=0, + stdout_file=str(stdout_file), + stderr_file=str(stderr_file), + mission_title="test codex usage accounting", + autonomous_mode="implement", + provider_name="codex", + ) + + billable_input = 26549 - 22272 + expected_total = billable_input + 1590 + state = json.loads((instance_dir / "usage_state.json").read_text()) + assert result["usage_updated"] is True + assert result["cost_tracking_failed"] is False + assert state["session_tokens"] == expected_total + assert state["weekly_tokens"] == expected_total + assert state["runs"] == 1 + + rows = cost_tracker._read_jsonl_for_date( + instance_dir / "usage", + date.today(), + ) + assert len(rows) == 1 + row = rows[0] + assert row["project"] == "koan" + assert row["model"] == "gpt-5.4" + assert row["input_tokens"] == billable_input + assert row["output_tokens"] == 1590 + assert row["cache_read_input_tokens"] == 22272 + assert row["mode"] == "implement" + @patch("app.mission_runner.commit_instance") @patch("app.mission_runner.check_auto_merge", return_value=None) @patch("app.mission_runner.trigger_reflection", return_value=False) From cb02ab86e416b736ae8c90782eba349ed22f1aa9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?K=C5=8Dan?= Date: Wed, 3 Jun 2026 12:12:08 -0600 Subject: [PATCH 04/20] feat: shared branded footer for PRs, issues, and review comments MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Consolidate all Kōan footer generation into a single pr_footer.py module with build_koan_footer() and strip_legacy_footers(). Every caller (pr_quality, review_runner, plan_runner, fix_runner, implement_runner, incident_runner, deepplan_runner, claudemd_refresh, tracker_comment_format) now uses the shared helper, producing a consistent branded link to koan.anantys.com. The quality pipeline also strips duplicate/legacy footers (including "🤖 Generated with Claude Code") from PR bodies before appending the quality report section. --- koan/app/claudemd_refresh.py | 7 +- koan/app/plan_runner.py | 5 +- koan/app/pr_footer.py | 68 +++++++++ koan/app/pr_quality.py | 6 +- koan/app/review_runner.py | 26 ++-- koan/app/tracker_comment_format.py | 5 +- koan/skills/core/deepplan/deepplan_runner.py | 3 +- koan/skills/core/fix/fix_runner.py | 9 +- .../skills/core/implement/implement_runner.py | 9 +- koan/skills/core/incident/incident_runner.py | 7 +- .../core/incident/prompts/incident-analyze.md | 2 +- .../_partials/implementation-workflow.md | 2 +- koan/tests/test_plan_runner.py | 2 +- koan/tests/test_pr_footer.py | 131 ++++++++++++++++++ 14 files changed, 250 insertions(+), 32 deletions(-) create mode 100644 koan/app/pr_footer.py create mode 100644 koan/tests/test_pr_footer.py diff --git a/koan/app/claudemd_refresh.py b/koan/app/claudemd_refresh.py index 1ed666038..4937d0fdc 100644 --- a/koan/app/claudemd_refresh.py +++ b/koan/app/claudemd_refresh.py @@ -140,6 +140,9 @@ def _create_pr( """Create a draft PR for the CLAUDE.md update. Returns the PR URL.""" from app.github import pr_create, resolve_target_repo + from app.pr_footer import build_koan_footer + + footer = build_koan_footer() if mode == "INIT": title = f"docs: create CLAUDE.md for {project_name}" body = ( @@ -148,7 +151,7 @@ def _create_pr( "## Why\n" "No CLAUDE.md existed — this bootstraps the reference document " "for AI coding assistants working on this project.\n\n" - "---\n_Generated by Kōan `/claudemd`_" + f"---\n{footer}" ) else: title = f"docs: update CLAUDE.md for {project_name}" @@ -158,7 +161,7 @@ def _create_pr( "## Why\n" "Architecturally significant changes landed since the last update. " "This keeps the AI reference document in sync with the codebase.\n\n" - "---\n_Generated by Kōan `/claudemd`_" + f"---\n{footer}" ) pr_kwargs = { diff --git a/koan/app/plan_runner.py b/koan/app/plan_runner.py index cb50a1eca..d98104cbb 100644 --- a/koan/app/plan_runner.py +++ b/koan/app/plan_runner.py @@ -137,7 +137,8 @@ def _run_new_plan( title = _extract_title(plan) plan_body = _strip_title_line(plan) - issue_body = f"{plan_body}\n\n---\n*Generated by Kōan /plan*" + from app.pr_footer import build_koan_footer + issue_body = f"{plan_body}\n\n---\n{build_koan_footer()}" if not tracker_is_configured(project_name, project_path): notify_fn(f"✅ Plan generated inline:\n\n{plan[:3000]}") @@ -147,7 +148,7 @@ def _run_new_plan( if provider == "jira": issue_body = ( f"{jira_readable_markdown(plan_body)}\n\n" - "Generated by Koan /plan." + "Generated by Koan." ).strip() labels = [_PLAN_LABEL] if tracker_supports_labels(project_name, project_path) else None diff --git a/koan/app/pr_footer.py b/koan/app/pr_footer.py new file mode 100644 index 000000000..0e285490c --- /dev/null +++ b/koan/app/pr_footer.py @@ -0,0 +1,68 @@ +"""Shared PR/comment footer helpers for consistent Kōan branding. + +Provides a single ``build_koan_footer()`` function used across review +comments, quality reports, plan issues, and PR bodies — so the branded +link and provider/model attribution stay consistent everywhere. +""" + +import re +from typing import List + +KOAN_URL = "https://koan.anantys.com" +KOAN_BRANDED = f"[Kōan]({KOAN_URL})" + +_LEGACY_FOOTER_PATTERNS: List[re.Pattern] = [ + re.compile( + r"^\s*\*Generated by K(?:ō|o)an[^*]*\*\s*$", re.MULTILINE, + ), + re.compile( + r"^\s*_Generated by\s*\[?K(?:ō|o)an\]?[^\n]*$", re.MULTILINE, + ), + re.compile( + r"^\s*🤖\s*Generated with\s*\[?Claude Code\]?[^\n]*$", re.MULTILINE, + ), + re.compile( + r"^\s*_Automated review by\s*\[?K(?:ō|o)an\]?[^\n]*$", re.MULTILINE, + ), +] + + +def build_koan_footer( + action: str = "Generated by", + provider_name: str = "", + model: str = "", +) -> str: + """Build a branded footer line. + + Examples:: + + build_koan_footer() + # → '_Generated by [Kōan](https://koan.anantys.com)_' + + build_koan_footer(action="Automated review by", provider_name="claude", model="opus-4-6") + # → '_Automated review by [Kōan](...)_ _(Claude · model opus-4-6)_' + """ + base = f"_{action} {KOAN_BRANDED}_" + parts = [] + if provider_name: + parts.append(provider_name.capitalize()) + if model: + parts.append(f"model {model}") + if parts: + return f"{base} _({' · '.join(parts)})_" + return base + + +def strip_legacy_footers(body: str) -> str: + """Remove known legacy/duplicate footer lines from a PR or comment body. + + Strips lines matching ``*Generated by Kōan …*``, + ``🤖 Generated with Claude Code``, etc. Also collapses any + resulting trailing ``---`` separator that no longer precedes content. + """ + for pat in _LEGACY_FOOTER_PATTERNS: + body = pat.sub("", body) + + body = re.sub(r"\n{3,}", "\n\n", body) + body = re.sub(r"\n+---\s*$", "", body) + return body.rstrip() diff --git a/koan/app/pr_quality.py b/koan/app/pr_quality.py index 4f495fc8d..81a5a32bc 100644 --- a/koan/app/pr_quality.py +++ b/koan/app/pr_quality.py @@ -412,6 +412,9 @@ def enrich_pr_description(project_path: str, quality_report: dict) -> Optional[s if separator in pr_body: pr_body = pr_body[:pr_body.index(separator)].rstrip() + from app.pr_footer import strip_legacy_footers + pr_body = strip_legacy_footers(pr_body) + new_body = f"{pr_body}\n\n{report_section}" try: @@ -476,7 +479,8 @@ def _build_quality_report_section(report: dict, project_path: str) -> str: ) lines.append("") - lines.append("*Generated by Kōan post-mission quality pipeline*") + from app.pr_footer import build_koan_footer + lines.append(build_koan_footer()) return "\n".join(lines) diff --git a/koan/app/review_runner.py b/koan/app/review_runner.py index 54f023a90..fd08ab80f 100644 --- a/koan/app/review_runner.py +++ b/koan/app/review_runner.py @@ -262,10 +262,8 @@ def _fetch_plan_body(owner: str, repo: str, issue_number: str) -> str: except RuntimeError: pass - # Strip plan footer added by /plan skill - footer_marker = "\n---\n*Generated by Kōan /plan" - if footer_marker in plan_body: - plan_body = plan_body[:plan_body.index(footer_marker)].rstrip() + from app.pr_footer import strip_legacy_footers + plan_body = strip_legacy_footers(plan_body) return plan_body @@ -1028,15 +1026,12 @@ def _format_review_as_markdown(review_data: dict, title: str = "", bot_username: def _build_review_footer(provider_name: str = "", model: str = "") -> str: """Build the review footer with branding, provider, and model info.""" - base = "_Automated review by [Kōan](https://koan.anantys.com)_" - parts = [] - if provider_name: - parts.append(provider_name.capitalize()) - if model: - parts.append(f"model {model}") - if parts: - return f"{base} _({' · '.join(parts)})_" - return base + from app.pr_footer import build_koan_footer + return build_koan_footer( + action="Automated review by", + provider_name=provider_name, + model=model, + ) def _post_review_comment( @@ -1250,9 +1245,8 @@ def _resolve_plan_body(plan_url: Optional[str], pr_body: str) -> str: continue except RuntimeError: pass - footer_marker = "\n---\n*Generated by Kōan /plan" - if footer_marker in plan_body: - plan_body = plan_body[:plan_body.index(footer_marker)].rstrip() + from app.pr_footer import strip_legacy_footers + plan_body = strip_legacy_footers(plan_body) return plan_body # Auto-detect from PR body diff --git a/koan/app/tracker_comment_format.py b/koan/app/tracker_comment_format.py index 8fe486d83..df4cb95c1 100644 --- a/koan/app/tracker_comment_format.py +++ b/koan/app/tracker_comment_format.py @@ -217,12 +217,13 @@ def build_plan_comment_success(provider: str, title: str, body: str) -> str: "Koan plan update\n\n" f"Title: {title}\n\n" f"{readable_body}\n\n" - "Generated by Koan /plan (iteration on existing issue)." + "Generated by Koan." ).strip() + from app.pr_footer import build_koan_footer return ( f"## {title}\n\n{body}\n\n---\n" - "*Generated by Kōan /plan — iteration on existing issue*" + f"{build_koan_footer()}" ) diff --git a/koan/skills/core/deepplan/deepplan_runner.py b/koan/skills/core/deepplan/deepplan_runner.py index 13c9a43d4..0ccabede7 100644 --- a/koan/skills/core/deepplan/deepplan_runner.py +++ b/koan/skills/core/deepplan/deepplan_runner.py @@ -110,7 +110,8 @@ def run_deepplan( # Phase 3: Post spec as a tracker issue title = _extract_title(spec) spec_body = _strip_title_line(spec) - issue_body = f"{spec_body}\n\n---\n*Generated by Kōan /deepplan*" + from app.pr_footer import build_koan_footer + issue_body = f"{spec_body}\n\n---\n{build_koan_footer()}" labels = [_DEEPPLAN_LABEL] if tracker_supports_labels(project_name, project_path) else None try: diff --git a/koan/skills/core/fix/fix_runner.py b/koan/skills/core/fix/fix_runner.py index 302febdd4..063d227b6 100644 --- a/koan/skills/core/fix/fix_runner.py +++ b/koan/skills/core/fix/fix_runner.py @@ -32,6 +32,11 @@ logger = logging.getLogger(__name__) +def _build_footer() -> str: + from app.pr_footer import build_koan_footer + return build_koan_footer() + + def run_fix( project_path: str, issue_url: str, @@ -318,7 +323,7 @@ def _submit_fix_pr( f"## Summary\n\n" f"Fixes {issue_url}\n\n" f"## Changes\n\n{commits_text}\n\n" - f"---\n*Generated by Koan /fix*" + f"---\n{_build_footer()}" ) try: @@ -328,7 +333,7 @@ def _submit_fix_pr( pr_body = ( f"{format_description(desc)}\n\n" f"Fixes {issue_url}\n\n" - f"---\n*Generated by Koan /fix*" + f"---\n{_build_footer()}" ) except Exception as e: logger.warning("describe_pr failed, using fallback body: %s", e) diff --git a/koan/skills/core/implement/implement_runner.py b/koan/skills/core/implement/implement_runner.py index c96c0af95..392b35765 100644 --- a/koan/skills/core/implement/implement_runner.py +++ b/koan/skills/core/implement/implement_runner.py @@ -47,6 +47,11 @@ ) +def _build_footer() -> str: + from app.pr_footer import build_koan_footer + return build_koan_footer() + + def run_implement( project_path: str, issue_url: str, @@ -618,7 +623,7 @@ def _submit_implement_pr( pr_body = ( f"## Summary\n\n{summary}\n\n" f"Closes {issue_url}\n\n" - f"---\n*Generated by Kōan /implement*" + f"---\n{_build_footer()}" ) try: @@ -628,7 +633,7 @@ def _submit_implement_pr( pr_body = ( f"{format_description(desc)}\n\n" f"Closes {issue_url}\n\n" - f"---\n*Generated by Kōan /implement*" + f"---\n{_build_footer()}" ) except Exception as e: logger.warning("describe_pr failed, using fallback body: %s", e) diff --git a/koan/skills/core/incident/incident_runner.py b/koan/skills/core/incident/incident_runner.py index 3a4be52b9..8609c23ad 100644 --- a/koan/skills/core/incident/incident_runner.py +++ b/koan/skills/core/incident/incident_runner.py @@ -41,6 +41,11 @@ ) +def _build_footer() -> str: + from app.pr_footer import build_koan_footer + return build_koan_footer() + + def run_incident( project_path: str, error_text: str, @@ -231,7 +236,7 @@ def _submit_incident_pr( f"## Incident Details\n\n" f"```\n{error_snippet}\n```\n\n" f"## Changes\n\n{commits_text}\n\n" - f"---\n*Generated by Koan /incident*" + f"---\n{_build_footer()}" ) try: diff --git a/koan/skills/core/incident/prompts/incident-analyze.md b/koan/skills/core/incident/prompts/incident-analyze.md index 38d7281ca..d567f9021 100644 --- a/koan/skills/core/incident/prompts/incident-analyze.md +++ b/koan/skills/core/incident/prompts/incident-analyze.md @@ -67,7 +67,7 @@ You are triaging a production incident. Your job is to analyze the error, identi - [How the fix was verified] --- - *Generated by Kōan /incident* + _Generated by [Kōan](https://koan.anantys.com)_ EOF )" ``` diff --git a/koan/system-prompts/_partials/implementation-workflow.md b/koan/system-prompts/_partials/implementation-workflow.md index 2fb36d441..ee23c7304 100644 --- a/koan/system-prompts/_partials/implementation-workflow.md +++ b/koan/system-prompts/_partials/implementation-workflow.md @@ -51,7 +51,7 @@ After each commit: - [How the changes were verified] --- - *Generated by Kōan* + _Generated by [Kōan](https://koan.anantys.com)_ EOF )" ``` diff --git a/koan/tests/test_plan_runner.py b/koan/tests/test_plan_runner.py index a5864c98a..6d95b2c7c 100644 --- a/koan/tests/test_plan_runner.py +++ b/koan/tests/test_plan_runner.py @@ -273,7 +273,7 @@ def test_jira_tracker_omits_labels(self): assert ok _, kwargs = create.call_args assert kwargs.get("labels") is None - assert "Generated by Koan /plan." in create.call_args.args[3] + assert "Generated by Koan." in create.call_args.args[3] assert "## Plan" not in create.call_args.args[3] diff --git a/koan/tests/test_pr_footer.py b/koan/tests/test_pr_footer.py new file mode 100644 index 000000000..1f8b95ace --- /dev/null +++ b/koan/tests/test_pr_footer.py @@ -0,0 +1,131 @@ +"""Tests for app.pr_footer — shared Kōan footer helpers.""" + +import os +import sys + +import pytest + +sys.path.insert(0, os.path.join(os.path.dirname(__file__), "..")) +os.environ.setdefault("KOAN_ROOT", "/tmp/test-koan") + +from app.pr_footer import build_koan_footer, strip_legacy_footers, KOAN_URL + + +class TestBuildKoanFooter: + def test_default(self): + result = build_koan_footer() + assert result == f"_Generated by [Kōan]({KOAN_URL})_" + + def test_custom_action(self): + result = build_koan_footer(action="Automated review by") + assert result == f"_Automated review by [Kōan]({KOAN_URL})_" + + def test_provider_only(self): + result = build_koan_footer(provider_name="claude") + assert "Claude" in result + assert f"[Kōan]({KOAN_URL})" in result + assert "_(Claude)_" in result + + def test_model_only(self): + result = build_koan_footer(model="opus-4-6") + assert "model opus-4-6" in result + assert f"[Kōan]({KOAN_URL})" in result + + def test_provider_and_model(self): + result = build_koan_footer(provider_name="claude", model="opus-4-6") + assert "_(Claude · model opus-4-6)_" in result + assert f"[Kōan]({KOAN_URL})" in result + + def test_provider_capitalized(self): + result = build_koan_footer(provider_name="copilot") + assert "Copilot" in result + + def test_review_footer_compat(self): + result = build_koan_footer( + action="Automated review by", + provider_name="claude", + model="opus-4-6", + ) + expected = ( + f"_Automated review by [Kōan]({KOAN_URL})_" + " _(Claude · model opus-4-6)_" + ) + assert result == expected + + +class TestStripLegacyFooters: + def test_strip_star_footer(self): + body = "## Summary\n\nChanges.\n\n*Generated by Kōan /fix*" + result = strip_legacy_footers(body) + assert "Generated by" not in result + assert "Changes." in result + + def test_strip_italic_footer(self): + body = "## Summary\n\nChanges.\n\n_Generated by Kōan `/claudemd`_" + result = strip_legacy_footers(body) + assert "Generated by" not in result + + def test_strip_claude_code_footer(self): + body = "## Summary\n\nChanges.\n\n🤖 Generated with [Claude Code](https://claude.ai/code)" + result = strip_legacy_footers(body) + assert "Claude Code" not in result + assert "Changes." in result + + def test_strip_claude_code_plain(self): + body = "## Summary\n\nChanges.\n\n🤖 Generated with Claude Code" + result = strip_legacy_footers(body) + assert "Claude Code" not in result + + def test_strip_review_footer(self): + body = "## Review\n\nFindings.\n\n_Automated review by Kōan_" + result = strip_legacy_footers(body) + assert "Automated review" not in result + + def test_strip_review_footer_with_link(self): + body = f"## Review\n\n_Automated review by [Kōan]({KOAN_URL})_ _(Claude · model opus-4-6)_" + result = strip_legacy_footers(body) + assert "Automated review" not in result + + def test_strip_branded_footer(self): + body = f"## Summary\n\nChanges.\n\n---\n_Generated by [Kōan]({KOAN_URL})_" + result = strip_legacy_footers(body) + assert "Generated by" not in result + assert "---" not in result + + def test_strip_branded_footer_with_provider(self): + body = ( + "## Summary\n\nChanges.\n\n---\n" + f"_Generated by [Kōan]({KOAN_URL})_ _(Claude · model opus-4-6)_" + ) + result = strip_legacy_footers(body) + assert "Generated by" not in result + + def test_strip_multiple_footers(self): + body = ( + "## Summary\n\nChanges.\n\n" + "🤖 Generated with [Claude Code](https://claude.ai/code)\n\n" + "---\n*Generated by Kōan post-mission quality pipeline*" + ) + result = strip_legacy_footers(body) + assert "Claude Code" not in result + assert "Generated by" not in result + assert "Changes." in result + + def test_strip_koan_without_accent(self): + body = "## Summary\n\n*Generated by Koan /fix*" + result = strip_legacy_footers(body) + assert "Generated by" not in result + + def test_preserves_regular_content(self): + body = "## Summary\n\nGenerated excellent results.\n\nNo footer here." + result = strip_legacy_footers(body) + assert result == body + + def test_collapses_trailing_separator(self): + body = "## Summary\n\nChanges.\n\n---\n*Generated by Kōan*" + result = strip_legacy_footers(body) + assert result.endswith("Changes.") + assert "---" not in result + + def test_empty_body(self): + assert strip_legacy_footers("") == "" From 3a0b6cf518d37469a115eb11035d6b18f52d6847 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?K=C5=8Dan?= Date: Wed, 3 Jun 2026 12:28:34 -0600 Subject: [PATCH 05/20] fix(review): prevent self-reply loops in PR comment threads Add thread-aware filtering to the review comment reply system. When the bot is the last poster in a thread and no human followed up, the thread is excluded from the repliable list on subsequent review runs. Also add a configurable max_thread_depth (default 5) to cap thread length. New config option: review_reply.max_thread_depth Co-Authored-By: Claude Opus 4.6 --- docs/users/user-manual.md | 9 ++ instance.example/config.yaml | 7 + koan/app/config.py | 23 +++ koan/app/review_runner.py | 104 +++++++++++--- koan/tests/test_review_runner.py | 240 +++++++++++++++++++++++++++++-- 5 files changed, 357 insertions(+), 26 deletions(-) diff --git a/docs/users/user-manual.md b/docs/users/user-manual.md index 7ac7d976f..9a140639c 100644 --- a/docs/users/user-manual.md +++ b/docs/users/user-manual.md @@ -661,6 +661,15 @@ review_dispatch: cooldown_minutes: 30 # Min minutes between checks per project (default: 30) ``` +To prevent the bot from replying to itself in review comment threads (and to cap thread depth), configure: + +```yaml +review_reply: + max_thread_depth: 5 # Stop replying after this many comments per thread (default: 5) +``` + +When the bot is the last poster in a thread and no human has followed up, the thread is excluded from future replies. This prevents self-reply loops where repeated `/review` runs keep adding replies to the same comment. +
Use cases diff --git a/instance.example/config.yaml b/instance.example/config.yaml index 72859ffe8..3845f4c56 100644 --- a/instance.example/config.yaml +++ b/instance.example/config.yaml @@ -262,6 +262,13 @@ skill_timeout: 7200 # enabled: false # Master switch (default: false) # cooldown_minutes: 30 # Min minutes between checks per project (default: 30) +# Review reply guard — prevents the bot from replying to itself in PR +# comment threads and caps the total depth of any single thread. +# When the bot is the last poster in a thread and no human followed up, +# the thread is excluded from the repliable list on the next review run. +# review_reply: +# max_thread_depth: 5 # Stop replying after this many comments in a thread (default: 5) + # Contemplative mode trigger chance (0-100%) # When no mission is pending, this is the probability of running a reflective # session instead of autonomous work. Allows regular moments of introspection diff --git a/koan/app/config.py b/koan/app/config.py index 1bd72f670..eb749d5d9 100644 --- a/koan/app/config.py +++ b/koan/app/config.py @@ -1293,6 +1293,29 @@ def get_review_concurrency_config() -> dict: } +def get_review_reply_config() -> dict: + """Get review reply guard configuration from config.yaml. + + Controls self-reply prevention and thread depth limits for PR review + comment replies. + + Config key: review_reply + - max_thread_depth (int): Stop replying in a thread after this many + total comments (default: 5). + + Returns: + Dict with keys: + - max_thread_depth (int): Maximum comments per thread. + """ + config = _load_config() + review_cfg = config.get("review_reply", {}) + if not isinstance(review_cfg, dict): + review_cfg = {} + return { + "max_thread_depth": _safe_int(review_cfg.get("max_thread_depth", 5), 5), + } + + def get_review_ignore_config() -> dict: """Get review ignore patterns from config.yaml. diff --git a/koan/app/review_runner.py b/koan/app/review_runner.py index fd08ab80f..105ff9529 100644 --- a/koan/app/review_runner.py +++ b/koan/app/review_runner.py @@ -24,7 +24,7 @@ from typing import List, Optional, Tuple from app.claude_step import resolve_pr_location -from app.config import is_review_compressor_enabled +from app.config import get_review_reply_config, is_review_compressor_enabled from app.run_log import log from app.diff_compressor import compress_diff from app.github import run_gh, sanitize_github_comment, find_bot_comment @@ -78,39 +78,101 @@ def _resolve_bot_username() -> str: return "" +def _is_bot_user(item: dict, bot_username: str) -> bool: + """Return True if the comment author is a bot or the configured bot user.""" + if item.get("user_type") == "Bot": + return True + if bot_username and item.get("user", "").lower() == bot_username.lower(): + return True + return False + + +def _filter_threads( + human_comments: List[dict], + all_comments: list, + bot_username: str, + max_thread_depth: int, +) -> List[dict]: + """Remove comments where the bot already replied with no human follow-up, + or where the thread has reached max depth. + + For inline review comments, threads are identified by ``in_reply_to_id`` + (all replies point to the root comment). A comment is excluded when: + + 1. The bot is the last poster in the thread and no human posted after, OR + 2. The total number of comments in the thread >= ``max_thread_depth``. + """ + if not bot_username and max_thread_depth <= 0: + return human_comments + + thread_members: dict = {} + for c in all_comments: + root_id = c.get("in_reply_to_id") or c["id"] + thread_members.setdefault(root_id, []).append(c) + + excluded_threads: set = set() + for root_id, members in thread_members.items(): + if max_thread_depth > 0 and len(members) >= max_thread_depth: + excluded_threads.add(root_id) + continue + if bot_username and members: + last = members[-1] + if last.get("user", "").lower() == bot_username.lower(): + excluded_threads.add(root_id) + + if not excluded_threads: + return human_comments + + filtered = [] + for c in human_comments: + root_id = c.get("in_reply_to_id") or c["id"] + if root_id not in excluded_threads: + filtered.append(c) + return filtered + + def _fetch_inline_review_comments( full_repo: str, pr_number: str, bot_username: str = "", + max_thread_depth: int = 0, ) -> List[dict]: - """Fetch inline review comments (code-level) for a PR.""" - results: List[dict] = [] + """Fetch inline review comments (code-level) for a PR. + + When ``bot_username`` is set, threads where the bot was the last poster + (with no human follow-up) are excluded. When ``max_thread_depth`` > 0, + threads with that many or more total comments are excluded entirely. + """ + all_items: list = [] + human_comments: List[dict] = [] try: raw = run_gh( "api", f"repos/{full_repo}/pulls/{pr_number}/comments", "--paginate", "--jq", - r'.[] | {id: .id, user: .user.login, body: .body, path: .path, line: (.line // .original_line), user_type: .user.type}', + r'.[] | {id: .id, user: .user.login, body: .body, path: .path, line: (.line // .original_line), user_type: .user.type, in_reply_to_id: .in_reply_to_id}', ) if raw.strip(): for line in raw.strip().split("\n"): try: item = json.loads(line) - if item.get("user_type") == "Bot": + all_items.append(item) + if _is_bot_user(item, bot_username): continue - # Skip bot's own comments to prevent self-reply loops - if bot_username and item["user"].lower() == bot_username.lower(): - continue - results.append({ + human_comments.append({ "id": item["id"], "type": "review_comment", "user": item["user"], "body": item["body"], "path": item.get("path", ""), "line": item.get("line"), + "in_reply_to_id": item.get("in_reply_to_id"), }) except (json.JSONDecodeError, KeyError): continue except RuntimeError: pass - return results + + if bot_username or max_thread_depth > 0: + return _filter_threads(human_comments, all_items, bot_username, max_thread_depth) + return human_comments def _fetch_issue_comments( @@ -128,10 +190,7 @@ def _fetch_issue_comments( for line in raw.strip().split("\n"): try: item = json.loads(line) - if item.get("user_type") == "Bot": - continue - # Skip bot's own comments to prevent self-reply loops - if bot_username and item["user"].lower() == bot_username.lower(): + if _is_bot_user(item, bot_username): continue results.append({ "id": item["id"], @@ -154,8 +213,9 @@ def fetch_repliable_comments( """Fetch PR comments with their IDs for reply targeting. Returns a list of dicts with keys: id, type, user, body, path (for - inline comments only). Excludes bot comments and the PR author's own - inline comments to reduce noise. + inline comments only). Excludes bot comments, threads where the bot + was the last poster (self-reply guard), and threads that have reached + the configured ``max_thread_depth``. Args: owner: GitHub owner/org. @@ -167,17 +227,25 @@ def fetch_repliable_comments( bot_username: If provided, comments from this user are excluded to prevent self-reply loops. """ + reply_cfg = get_review_reply_config() + max_depth = reply_cfg["max_thread_depth"] + full_repo = f"{owner}/{repo}" comments: List[dict] = [] if parallel: with ThreadPoolExecutor(max_workers=2) as pool: - f_inline = pool.submit(_fetch_inline_review_comments, full_repo, pr_number, bot_username) + f_inline = pool.submit( + _fetch_inline_review_comments, full_repo, pr_number, + bot_username, max_depth, + ) f_issue = pool.submit(_fetch_issue_comments, full_repo, pr_number, bot_username) comments.extend(f_inline.result()) comments.extend(f_issue.result()) else: - comments.extend(_fetch_inline_review_comments(full_repo, pr_number, bot_username)) + comments.extend( + _fetch_inline_review_comments(full_repo, pr_number, bot_username, max_depth), + ) comments.extend(_fetch_issue_comments(full_repo, pr_number, bot_username)) return comments diff --git a/koan/tests/test_review_runner.py b/koan/tests/test_review_runner.py index af34fe8ea..03e9a13bd 100644 --- a/koan/tests/test_review_runner.py +++ b/koan/tests/test_review_runner.py @@ -2129,10 +2129,11 @@ def test_no_plan_url_when_absent(self): class TestFetchRepliableCommentsParallel: """Tests for the parallel=False / parallel=True modes of fetch_repliable_comments.""" + @patch("app.review_runner.get_review_reply_config", return_value={"max_thread_depth": 5}) @patch("app.review_runner._fetch_issue_comments") @patch("app.review_runner._fetch_inline_review_comments") def test_sequential_mode_calls_helpers_in_order( - self, mock_inline, mock_issue, + self, mock_inline, mock_issue, _cfg, ): """parallel=False calls the two fetch helpers sequentially.""" mock_inline.return_value = [{"id": 1, "type": "review_comment"}] @@ -2140,17 +2141,17 @@ def test_sequential_mode_calls_helpers_in_order( comments = fetch_repliable_comments("owner", "repo", "42", parallel=False) - mock_inline.assert_called_once_with("owner/repo", "42", "") + mock_inline.assert_called_once_with("owner/repo", "42", "", 5) mock_issue.assert_called_once_with("owner/repo", "42", "") assert len(comments) == 2 - # Inline results come first in sequential mode assert comments[0]["id"] == 1 assert comments[1]["id"] == 2 + @patch("app.review_runner.get_review_reply_config", return_value={"max_thread_depth": 5}) @patch("app.review_runner._fetch_issue_comments") @patch("app.review_runner._fetch_inline_review_comments") def test_parallel_mode_collects_both_result_sets( - self, mock_inline, mock_issue, + self, mock_inline, mock_issue, _cfg, ): """parallel=True still collects results from both helpers.""" mock_inline.return_value = [{"id": 10, "type": "review_comment"}] @@ -2162,9 +2163,10 @@ def test_parallel_mode_collects_both_result_sets( ids = {c["id"] for c in comments} assert ids == {10, 20} + @patch("app.review_runner.get_review_reply_config", return_value={"max_thread_depth": 5}) @patch("app.review_runner._fetch_issue_comments") @patch("app.review_runner._fetch_inline_review_comments") - def test_empty_results_from_both_helpers(self, mock_inline, mock_issue): + def test_empty_results_from_both_helpers(self, mock_inline, mock_issue, _cfg): """Returns empty list when both helpers return nothing.""" mock_inline.return_value = [] mock_issue.return_value = [] @@ -2222,19 +2224,241 @@ def test_no_filtering_when_bot_username_empty(self, mock_gh): result = _fetch_inline_review_comments("owner/repo", "1", bot_username="") assert len(result) == 1 + @patch("app.review_runner.get_review_reply_config", return_value={"max_thread_depth": 5}) @patch("app.review_runner._fetch_issue_comments") @patch("app.review_runner._fetch_inline_review_comments") - def test_fetch_repliable_passes_bot_username(self, mock_inline, mock_issue): - """fetch_repliable_comments forwards bot_username to helpers.""" + def test_fetch_repliable_passes_bot_username(self, mock_inline, mock_issue, _cfg): + """fetch_repliable_comments forwards bot_username and max_thread_depth.""" mock_inline.return_value = [] mock_issue.return_value = [] fetch_repliable_comments("o", "r", "1", parallel=False, bot_username="mybot") - mock_inline.assert_called_once_with("o/r", "1", "mybot") + mock_inline.assert_called_once_with("o/r", "1", "mybot", 5) mock_issue.assert_called_once_with("o/r", "1", "mybot") +# --------------------------------------------------------------------------- +# Thread-aware self-reply guard: _is_bot_user, _filter_threads +# --------------------------------------------------------------------------- + +class TestIsBotUser: + """Tests for _is_bot_user helper.""" + + def test_github_bot_type(self): + from app.review_runner import _is_bot_user + assert _is_bot_user({"user_type": "Bot", "user": "dependabot"}, "") is True + + def test_configured_bot_username(self): + from app.review_runner import _is_bot_user + assert _is_bot_user({"user_type": "User", "user": "Koan-Bot"}, "koan-bot") is True + + def test_human_user(self): + from app.review_runner import _is_bot_user + assert _is_bot_user({"user_type": "User", "user": "alice"}, "koan-bot") is False + + def test_no_bot_username_configured(self): + from app.review_runner import _is_bot_user + assert _is_bot_user({"user_type": "User", "user": "koan-bot"}, "") is False + + +class TestFilterThreads: + """Tests for _filter_threads — thread-aware self-reply prevention.""" + + def test_excludes_thread_where_bot_is_last_poster(self): + from app.review_runner import _filter_threads + + human_comments = [ + {"id": 1, "type": "review_comment", "user": "alice", "body": "fix this"}, + ] + all_comments = [ + {"id": 1, "user": "alice", "user_type": "User", "in_reply_to_id": None}, + {"id": 2, "user": "koan-bot", "user_type": "User", "in_reply_to_id": 1}, + ] + result = _filter_threads(human_comments, all_comments, "koan-bot", 0) + assert result == [] + + def test_keeps_thread_where_human_replied_after_bot(self): + from app.review_runner import _filter_threads + + human_comments = [ + {"id": 1, "type": "review_comment", "user": "alice", "body": "fix this"}, + {"id": 3, "type": "review_comment", "user": "bob", "body": "agreed", + "in_reply_to_id": 1}, + ] + all_comments = [ + {"id": 1, "user": "alice", "user_type": "User", "in_reply_to_id": None}, + {"id": 2, "user": "koan-bot", "user_type": "User", "in_reply_to_id": 1}, + {"id": 3, "user": "bob", "user_type": "User", "in_reply_to_id": 1}, + ] + result = _filter_threads(human_comments, all_comments, "koan-bot", 0) + assert len(result) == 2 + + def test_excludes_thread_at_max_depth(self): + from app.review_runner import _filter_threads + + human_comments = [ + {"id": 1, "type": "review_comment", "user": "alice", "body": "fix"}, + {"id": 3, "type": "review_comment", "user": "alice", "body": "more", + "in_reply_to_id": 1}, + ] + all_comments = [ + {"id": 1, "user": "alice", "user_type": "User", "in_reply_to_id": None}, + {"id": 2, "user": "koan-bot", "user_type": "User", "in_reply_to_id": 1}, + {"id": 3, "user": "alice", "user_type": "User", "in_reply_to_id": 1}, + ] + result = _filter_threads(human_comments, all_comments, "koan-bot", 3) + assert result == [] + + def test_keeps_thread_under_max_depth(self): + from app.review_runner import _filter_threads + + human_comments = [ + {"id": 1, "type": "review_comment", "user": "alice", "body": "fix"}, + ] + all_comments = [ + {"id": 1, "user": "alice", "user_type": "User", "in_reply_to_id": None}, + {"id": 2, "user": "koan-bot", "user_type": "User", "in_reply_to_id": 1}, + ] + # Thread has 2 comments, max is 5 → keep. But bot is last → exclude. + result = _filter_threads(human_comments, all_comments, "koan-bot", 5) + assert result == [] + + # Without bot_username check, thread is under max depth → keep + result = _filter_threads(human_comments, all_comments, "", 5) + assert len(result) == 1 + + def test_no_filtering_when_both_disabled(self): + from app.review_runner import _filter_threads + + human_comments = [{"id": 1, "type": "review_comment", "user": "alice", "body": "x"}] + all_comments = [{"id": 1, "user": "alice", "user_type": "User", "in_reply_to_id": None}] + result = _filter_threads(human_comments, all_comments, "", 0) + assert len(result) == 1 + + def test_multiple_threads_filtered_independently(self): + from app.review_runner import _filter_threads + + human_comments = [ + {"id": 1, "type": "review_comment", "user": "alice", "body": "thread 1"}, + {"id": 10, "type": "review_comment", "user": "bob", "body": "thread 2"}, + ] + all_comments = [ + {"id": 1, "user": "alice", "user_type": "User", "in_reply_to_id": None}, + {"id": 2, "user": "koan-bot", "user_type": "User", "in_reply_to_id": 1}, + {"id": 10, "user": "bob", "user_type": "User", "in_reply_to_id": None}, + ] + # Thread 1: bot is last → excluded. Thread 2: only human → kept. + result = _filter_threads(human_comments, all_comments, "koan-bot", 0) + assert len(result) == 1 + assert result[0]["id"] == 10 + + def test_reproduces_pr_1697_scenario(self): + """Reproduces the exact scenario from PR #1697 where bot replied 4x.""" + from app.review_runner import _filter_threads + + human_comments = [ + {"id": 100, "type": "review_comment", "user": "teodesian", + "body": "Review commentary..."}, + ] + all_comments = [ + {"id": 100, "user": "teodesian", "user_type": "User", "in_reply_to_id": None}, + {"id": 201, "user": "Koan-Bot", "user_type": "User", "in_reply_to_id": 100}, + {"id": 202, "user": "Koan-Bot", "user_type": "User", "in_reply_to_id": 100}, + {"id": 203, "user": "Koan-Bot", "user_type": "User", "in_reply_to_id": 100}, + {"id": 204, "user": "Koan-Bot", "user_type": "User", "in_reply_to_id": 100}, + ] + # Bot is last poster → excluded (even without max_depth) + result = _filter_threads(human_comments, all_comments, "koan-bot", 0) + assert result == [] + + # Also excluded by max_depth=5 (5 comments in thread) + result = _filter_threads(human_comments, all_comments, "", 5) + assert result == [] + + def test_github_bot_type_treated_as_bot(self): + """GitHub Bots (user_type=Bot) are treated same as configured bot.""" + from app.review_runner import _filter_threads + + human_comments = [ + {"id": 1, "type": "review_comment", "user": "alice", "body": "x"}, + ] + all_comments = [ + {"id": 1, "user": "alice", "user_type": "User", "in_reply_to_id": None}, + {"id": 2, "user": "copilot", "user_type": "Bot", "in_reply_to_id": 1}, + ] + # copilot (Bot type) is last → not excluded (it's not our bot) + # We only exclude when OUR bot is last poster + result = _filter_threads(human_comments, all_comments, "koan-bot", 0) + assert len(result) == 1 + + +class TestFetchInlineWithThreadFilter: + """Integration tests for _fetch_inline_review_comments with thread filtering.""" + + @patch("app.review_runner.run_gh") + def test_bot_last_reply_excluded(self, mock_gh): + from app.review_runner import _fetch_inline_review_comments + + mock_gh.return_value = "\n".join([ + json.dumps({"id": 1, "user": "alice", "body": "fix this", + "path": "a.py", "line": 10, "user_type": "User", + "in_reply_to_id": None}), + json.dumps({"id": 2, "user": "koan-bot", "body": "done", + "path": "a.py", "line": 10, "user_type": "User", + "in_reply_to_id": 1}), + ]) + result = _fetch_inline_review_comments( + "owner/repo", "1", bot_username="koan-bot", max_thread_depth=0, + ) + assert result == [] + + @patch("app.review_runner.run_gh") + def test_max_depth_excludes_deep_thread(self, mock_gh): + from app.review_runner import _fetch_inline_review_comments + + comments = [ + {"id": 1, "user": "alice", "body": "q1", "path": "a.py", + "line": 1, "user_type": "User", "in_reply_to_id": None}, + ] + for i in range(2, 7): + comments.append( + {"id": i, "user": "alice" if i % 2 == 0 else "koan-bot", + "body": f"reply {i}", "path": "a.py", "line": 1, + "user_type": "User", "in_reply_to_id": 1} + ) + mock_gh.return_value = "\n".join(json.dumps(c) for c in comments) + + result = _fetch_inline_review_comments( + "owner/repo", "1", bot_username="koan-bot", max_thread_depth=5, + ) + assert result == [] + + +class TestReviewReplyConfig: + """Tests for get_review_reply_config in app.config.""" + + def test_defaults(self): + from app.config import get_review_reply_config + with patch("app.config._load_config", return_value={}): + cfg = get_review_reply_config() + assert cfg == {"max_thread_depth": 5} + + def test_custom_value(self): + from app.config import get_review_reply_config + with patch("app.config._load_config", + return_value={"review_reply": {"max_thread_depth": 10}}): + cfg = get_review_reply_config() + assert cfg == {"max_thread_depth": 10} + + def test_invalid_value_falls_back_to_default(self): + from app.config import get_review_reply_config + with patch("app.config._load_config", + return_value={"review_reply": {"max_thread_depth": "abc"}}): + cfg = get_review_reply_config() + assert cfg == {"max_thread_depth": 5} + + # --------------------------------------------------------------------------- # Concurrency config: get_review_concurrency_config # --------------------------------------------------------------------------- From a94e23dd4e7f081b19b690459772f218227482c3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?K=C5=8Dan?= Date: Tue, 2 Jun 2026 23:29:11 -0600 Subject: [PATCH 06/20] fix(stagnation_monitor): lock retry tracker read-modify-write cycle MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit increment_retry_count() and clear_retry_count() did unlocked load-increment-save, allowing concurrent parallel-session missions to read the same counter, both compute +1, and one overwrite the other — under-counting retries by one per race. Replaces the private _load/_save_retry_tracker helpers with locked_json_modify / locked_json_read from locked_file.py (the same pattern used by burn_rate, check_tracker, etc.). Reads in get_retry_count and get_retry_info now use a shared lock; writes hold an exclusive lock across the full mutate cycle. OSError tolerance on save is preserved. Adds a concurrent-increment test that reliably reproduced the race (30 threads all stagnating simultaneously → final count was 1 before fix, 30 after). Closes https://github.com/Anantys-oss/koan/issues/1701 Co-Authored-By: Claude Sonnet 4.6 --- koan/app/stagnation_monitor.py | 80 +++++++++++++-------------- koan/tests/test_stagnation_monitor.py | 27 ++++++++- 2 files changed, 63 insertions(+), 44 deletions(-) diff --git a/koan/app/stagnation_monitor.py b/koan/app/stagnation_monitor.py index f1d8d27b9..8c0c1ddde 100644 --- a/koan/app/stagnation_monitor.py +++ b/koan/app/stagnation_monitor.py @@ -32,7 +32,6 @@ from __future__ import annotations import hashlib -import json import os import re import sys @@ -40,6 +39,8 @@ from pathlib import Path from typing import Callable, Optional +from app.locked_file import locked_json_modify, locked_json_read + # Default configuration — overridable via config.yaml stagnation: section. _DEFAULT_CHECK_INTERVAL = 60 # seconds between stdout samples @@ -354,30 +355,6 @@ def _mission_key(mission_title: str) -> str: return hashlib.sha256(mission_title.encode("utf-8", errors="replace")).hexdigest() -def _load_retry_tracker(instance_dir: str) -> dict: - path = _retry_tracker_path(instance_dir) - try: - with open(path, "r", encoding="utf-8") as f: - data = json.load(f) - if isinstance(data, dict): - return data - except (OSError, json.JSONDecodeError): - pass - return {} - - -def _save_retry_tracker(instance_dir: str, data: dict) -> None: - path = _retry_tracker_path(instance_dir) - try: - path.parent.mkdir(parents=True, exist_ok=True) - from app.utils import atomic_write_json - atomic_write_json(path, data) - except OSError as e: - # Stderr diagnostic — losing the counter just means an extra retry, - # not a correctness bug. - print(f"[stagnation_monitor] retry tracker save error: {e}", file=sys.stderr) - - def _extract_count(raw) -> int: """Extract retry count from a tracker entry (int or dict with 'count').""" if isinstance(raw, dict): @@ -388,9 +365,16 @@ def _extract_count(raw) -> int: return 0 +def _validate_tracker(data) -> dict: + return data if isinstance(data, dict) else {} + + def get_retry_count(instance_dir: str, mission_title: str) -> int: """Return how many times *mission_title* has been stagnation-requeued.""" - data = _load_retry_tracker(instance_dir) + path = _retry_tracker_path(instance_dir) + data = locked_json_read(path, default={}) + if not isinstance(data, dict): + return 0 return _extract_count(data.get(_mission_key(mission_title), 0)) @@ -399,8 +383,9 @@ def get_retry_info(instance_dir: str, mission_title: str) -> dict: Returns a dict with keys ``count``, ``pattern_type``, ``sample_lines``. """ - data = _load_retry_tracker(instance_dir) - raw = data.get(_mission_key(mission_title), {}) + path = _retry_tracker_path(instance_dir) + data = locked_json_read(path, default={}) + raw = data.get(_mission_key(mission_title), {}) if isinstance(data, dict) else {} if isinstance(raw, int): return {"count": max(0, raw), "pattern_type": "", "sample_lines": ""} if isinstance(raw, dict): @@ -425,23 +410,36 @@ def increment_retry_count( Returns the new count. """ - data = _load_retry_tracker(instance_dir) + path = _retry_tracker_path(instance_dir) + path.parent.mkdir(parents=True, exist_ok=True) key = _mission_key(mission_title) - current = _extract_count(data.get(key, 0)) - new_count = current + 1 - data[key] = { - "count": new_count, - "pattern_type": pattern_type, - "sample_lines": pattern_excerpt[:500], - } - _save_retry_tracker(instance_dir, data) - return new_count + + def _mutate(data: dict) -> int: + current = _extract_count(data.get(key, 0)) + new_count = current + 1 + data[key] = { + "count": new_count, + "pattern_type": pattern_type, + "sample_lines": pattern_excerpt[:500], + } + return new_count + + try: + return locked_json_modify(path, _mutate, default_factory=dict, validator=_validate_tracker) + except OSError as e: + # Losing the counter means at most one extra retry — tolerable. + print(f"[stagnation_monitor] retry tracker save error: {e}", file=sys.stderr) + return 1 def clear_retry_count(instance_dir: str, mission_title: str) -> None: """Drop the retry counter for *mission_title* (e.g. on completion).""" - data = _load_retry_tracker(instance_dir) + path = _retry_tracker_path(instance_dir) + if not path.exists(): + return key = _mission_key(mission_title) - if key in data: + + def _mutate(data: dict) -> None: data.pop(key, None) - _save_retry_tracker(instance_dir, data) + + locked_json_modify(path, _mutate, default_factory=dict, validator=_validate_tracker) diff --git a/koan/tests/test_stagnation_monitor.py b/koan/tests/test_stagnation_monitor.py index c08f2bc5f..3066e8f73 100644 --- a/koan/tests/test_stagnation_monitor.py +++ b/koan/tests/test_stagnation_monitor.py @@ -552,9 +552,7 @@ def test_increment_handles_non_integer_stored_value(self, tmp_path): def test_save_handles_oserror(self, tmp_path): """OSError during save is logged to stderr, not raised.""" d = str(tmp_path) - # atomic_write_json is imported locally inside _save_retry_tracker. - with patch("app.utils.atomic_write_json", - side_effect=OSError("disk full")): + with patch("app.utils.atomic_write", side_effect=OSError("disk full")): # Should not raise — the OSError is caught and printed to stderr. increment_retry_count(d, "test mission") @@ -876,3 +874,26 @@ def test_increment_preserves_latest_pattern(self, tmp_path): info = get_retry_info(instance, "flaky") assert info["count"] == 2 assert info["pattern_type"] == "infinite_retry" + + +class TestRetryTrackerConcurrency: + """Concurrent increment_retry_count must not lose updates.""" + + def test_concurrent_increments_no_lost_updates(self, tmp_path): + """N threads incrementing the same mission must all be counted.""" + import threading + + n = 30 + barrier = threading.Barrier(n) + + def _inc(): + barrier.wait() + increment_retry_count(str(tmp_path), "shared mission") + + threads = [threading.Thread(target=_inc) for _ in range(n)] + for t in threads: + t.start() + for t in threads: + t.join() + + assert get_retry_count(str(tmp_path), "shared mission") == n From e61126a4b4533126fd3043c46bb78b15581747af Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?K=C5=8Dan?= Date: Tue, 2 Jun 2026 23:18:32 -0600 Subject: [PATCH 07/20] fix(burn_rate): eliminate TOCTOU race in _maybe_warn_burn_rate _read_session_pct_and_reset now returns a 3-tuple (session_pct, minutes_until_reset, state) so callers can reuse the already-parsed dict. _maybe_warn_burn_rate unpacks and uses that dict for the session_start comparison instead of issuing a second read_text() call. This removes the window where the run loop could flush a new session start between the two reads, causing an inconsistent comparison that silently suppressed the burn-rate alert for an entire new quota cycle. Also eliminates one redundant file read per iteration when last_warned is set. Closes https://github.com/Anantys-oss/koan/issues/1703 Co-Authored-By: Claude Sonnet 4.6 --- koan/app/iteration_manager.py | 27 +++++---- koan/tests/test_iteration_manager.py | 83 ++++++++++++++++++++-------- 2 files changed, 72 insertions(+), 38 deletions(-) diff --git a/koan/app/iteration_manager.py b/koan/app/iteration_manager.py index 0f1b71872..a23088589 100644 --- a/koan/app/iteration_manager.py +++ b/koan/app/iteration_manager.py @@ -186,10 +186,10 @@ def _get_usage_decision(usage_md: Path, count: int, projects_str: str): def _read_session_pct_and_reset(usage_state_path: Path): - """Return (session_pct, minutes_until_session_reset) or (None, None). + """Return (session_pct, minutes_until_session_reset, state) or (None, None, None). - Reads usage_state.json directly so the warning logic does not depend on - the freshness of usage.md. + Reads usage_state.json once and returns the parsed dict as the third element + so callers can reuse it without a second file read (prevents TOCTOU races). """ try: import json @@ -200,22 +200,22 @@ def _read_session_pct_and_reset(usage_state_path: Path): ) from app.utils import load_config except (ImportError, OSError, ValueError): - return None, None + return None, None, None if not usage_state_path.exists(): - return None, None + return None, None, None try: state = json.loads(usage_state_path.read_text()) except (json.JSONDecodeError, OSError): - return None, None + return None, None, None try: session_limit, _ = _get_limits(load_config()) except (OSError, ValueError, TypeError): - return None, None + return None, None, None if session_limit <= 0: - return None, None + return None, None, None tokens = state.get("session_tokens", 0) or 0 session_pct = min(100.0, tokens / session_limit * 100.0) @@ -223,11 +223,11 @@ def _read_session_pct_and_reset(usage_state_path: Path): try: session_start = datetime.fromisoformat(state["session_start"]) except (KeyError, ValueError, TypeError): - return session_pct, None + return session_pct, None, state elapsed = (datetime.now() - session_start).total_seconds() / 60.0 minutes_remaining = max(0.0, SESSION_DURATION_HOURS * 60.0 - elapsed) - return session_pct, minutes_remaining + return session_pct, minutes_remaining, state def _maybe_warn_burn_rate(instance_dir: Path, usage_state_path: Path) -> None: @@ -249,7 +249,7 @@ def _maybe_warn_burn_rate(instance_dir: Path, usage_state_path: Path) -> None: except ImportError: return - session_pct, minutes_until_reset = _read_session_pct_and_reset( + session_pct, minutes_until_reset, usage_state = _read_session_pct_and_reset( usage_state_path ) if session_pct is None or minutes_until_reset is None: @@ -259,12 +259,11 @@ def _maybe_warn_burn_rate(instance_dir: Path, usage_state_path: Path) -> None: snapshot = BurnRateSnapshot(instance_dir) last_warned = snapshot.last_warned_at - if last_warned is not None: + if last_warned is not None and usage_state is not None: with suppress_logged(_log_iteration, "error", "Burn rate warning state parse failed", json.JSONDecodeError, OSError, KeyError, ValueError, TypeError): from datetime import datetime, timezone - state = json.loads(usage_state_path.read_text()) - session_start = datetime.fromisoformat(state["session_start"]) + session_start = datetime.fromisoformat(usage_state["session_start"]) if session_start.tzinfo is None: session_start = session_start.replace(tzinfo=timezone.utc) if last_warned < session_start: diff --git a/koan/tests/test_iteration_manager.py b/koan/tests/test_iteration_manager.py index 6f6e293bf..ca0c96cd5 100644 --- a/koan/tests/test_iteration_manager.py +++ b/koan/tests/test_iteration_manager.py @@ -210,17 +210,19 @@ def test_reads_tokens_and_minutes_until_reset(self, tmp_path): patch("app.usage_estimator._get_limits", return_value=(1000, 10000)), patch("app.utils.load_config", return_value={}), ): - pct, minutes = _read_session_pct_and_reset(state) + pct, minutes, parsed_state = _read_session_pct_and_reset(state) assert pct == 25.0 assert minutes is not None assert 0 < minutes <= 270 + assert parsed_state is not None + assert "session_tokens" in parsed_state - def test_invalid_json_returns_none_tuple(self, tmp_path): + def test_invalid_json_returns_none_triple(self, tmp_path): state = tmp_path / "usage_state.json" state.write_text("not-json") - assert _read_session_pct_and_reset(state) == (None, None) + assert _read_session_pct_and_reset(state) == (None, None, None) def test_missing_session_start_returns_pct_without_reset(self, tmp_path): state = tmp_path / "usage_state.json" @@ -230,9 +232,12 @@ def test_missing_session_start_returns_pct_without_reset(self, tmp_path): patch("app.usage_estimator._get_limits", return_value=(1000, 10000)), patch("app.utils.load_config", return_value={}), ): - assert _read_session_pct_and_reset(state) == (100.0, None) + pct, minutes, parsed_state = _read_session_pct_and_reset(state) + assert pct == 100.0 + assert minutes is None + assert parsed_state == {"session_tokens": 1500} - def test_non_positive_session_limit_returns_none_tuple(self, tmp_path): + def test_non_positive_session_limit_returns_none_triple(self, tmp_path): state = tmp_path / "usage_state.json" state.write_text(json.dumps({"session_tokens": 100})) @@ -240,7 +245,7 @@ def test_non_positive_session_limit_returns_none_tuple(self, tmp_path): patch("app.usage_estimator._get_limits", return_value=(0, 10000)), patch("app.utils.load_config", return_value={}), ): - assert _read_session_pct_and_reset(state) == (None, None) + assert _read_session_pct_and_reset(state) == (None, None, None) # === Tests: _downgrade_if_unaffordable === @@ -3745,7 +3750,7 @@ def test_no_warning_when_no_usage_state(self, tmp_path): outbox = inst / "outbox.md" assert not outbox.exists() - @patch("app.iteration_manager._read_session_pct_and_reset", return_value=(None, None)) + @patch("app.iteration_manager._read_session_pct_and_reset", return_value=(None, None, None)) def test_no_warning_when_session_pct_none(self, _mock, tmp_path): inst = tmp_path / "inst" inst.mkdir() @@ -3757,7 +3762,7 @@ def test_no_warning_when_session_pct_none(self, _mock, tmp_path): @patch("app.utils.append_to_outbox") @patch("app.burn_rate.mark_warned") @patch("app.burn_rate.BurnRateSnapshot") - @patch("app.iteration_manager._read_session_pct_and_reset", return_value=(60.0, 300.0)) + @patch("app.iteration_manager._read_session_pct_and_reset", return_value=(60.0, 300.0, {})) def test_fires_warning_when_tte_below_threshold(self, _pct, mock_snap_cls, mock_mark, mock_outbox, tmp_path): @@ -3777,7 +3782,7 @@ def test_fires_warning_when_tte_below_threshold(self, _pct, mock_snap_cls, @patch("app.utils.append_to_outbox") @patch("app.burn_rate.BurnRateSnapshot") - @patch("app.iteration_manager._read_session_pct_and_reset", return_value=(60.0, 300.0)) + @patch("app.iteration_manager._read_session_pct_and_reset", return_value=(60.0, 300.0, {})) def test_no_warning_when_tte_above_threshold(self, _pct, mock_snap_cls, mock_outbox, tmp_path): mock_snap = MagicMock() @@ -3791,7 +3796,7 @@ def test_no_warning_when_tte_above_threshold(self, _pct, mock_snap_cls, @patch("app.utils.append_to_outbox") @patch("app.burn_rate.BurnRateSnapshot") - @patch("app.iteration_manager._read_session_pct_and_reset", return_value=(60.0, 60.0)) + @patch("app.iteration_manager._read_session_pct_and_reset", return_value=(60.0, 60.0, {})) def test_no_warning_when_reset_imminent(self, _pct, mock_snap_cls, mock_outbox, tmp_path): mock_snap = MagicMock() @@ -3805,36 +3810,64 @@ def test_no_warning_when_reset_imminent(self, _pct, mock_snap_cls, @patch("app.utils.append_to_outbox") @patch("app.burn_rate.BurnRateSnapshot") - @patch("app.iteration_manager._read_session_pct_and_reset", return_value=(60.0, 300.0)) - def test_no_duplicate_warning_when_already_warned(self, _pct, mock_snap_cls, + @patch("app.iteration_manager._read_session_pct_and_reset") + def test_no_duplicate_warning_when_already_warned(self, mock_pct, mock_snap_cls, mock_outbox, tmp_path): from datetime import datetime, timezone, timedelta session_start = datetime.now(timezone.utc) - timedelta(minutes=30) warned_at = session_start + timedelta(minutes=10) + mock_pct.return_value = (60.0, 300.0, {"session_start": session_start.isoformat()}) mock_snap = MagicMock() mock_snap.last_warned_at = warned_at mock_snap.time_to_exhaustion.return_value = 20.0 mock_snap_cls.return_value = mock_snap inst = tmp_path / "inst" inst.mkdir() - usage = tmp_path / "usage.json" - usage.write_text(json.dumps({ - "session_start": session_start.isoformat(), - })) - _maybe_warn_burn_rate(inst, usage) + _maybe_warn_burn_rate(inst, tmp_path / "usage.json") mock_outbox.assert_not_called() @patch("app.burn_rate.clear_warning") @patch("app.utils.append_to_outbox") @patch("app.burn_rate.mark_warned") @patch("app.burn_rate.BurnRateSnapshot") - @patch("app.iteration_manager._read_session_pct_and_reset", return_value=(60.0, 300.0)) - def test_clears_stale_warning_from_previous_session(self, _pct, mock_snap_cls, + @patch("app.iteration_manager._read_session_pct_and_reset") + def test_clears_stale_warning_from_previous_session(self, mock_pct, mock_snap_cls, mock_mark, mock_outbox, mock_clear, tmp_path): from datetime import datetime, timezone, timedelta old_warn = datetime.now(timezone.utc) - timedelta(hours=5) new_session = datetime.now(timezone.utc) - timedelta(minutes=10) + mock_pct.return_value = (60.0, 300.0, {"session_start": new_session.isoformat()}) + mock_snap = MagicMock() + mock_snap.last_warned_at = old_warn + mock_snap.time_to_exhaustion.return_value = 15.0 + mock_snap.burn_rate_pct_per_minute.return_value = 0.8 + mock_snap_cls.return_value = mock_snap + inst = tmp_path / "inst" + inst.mkdir() + _maybe_warn_burn_rate(inst, tmp_path / "usage.json") + mock_clear.assert_called_once_with(inst) + mock_outbox.assert_called_once() + mock_mark.assert_called_once() + + @patch("app.burn_rate.clear_warning") + @patch("app.utils.append_to_outbox") + @patch("app.burn_rate.mark_warned") + @patch("app.burn_rate.BurnRateSnapshot") + @patch("app.iteration_manager._read_session_pct_and_reset") + def test_uses_state_from_first_read_not_file(self, mock_pct, mock_snap_cls, + mock_mark, mock_outbox, + mock_clear, tmp_path): + """TOCTOU fix: session_start from _read_session_pct_and_reset state dict + must be used instead of re-reading the file. Write a stale session_start + to the file; the returned state carries a newer session_start that should + trigger the stale-warning clear. + """ + from datetime import datetime, timezone, timedelta + old_warn = datetime.now(timezone.utc) - timedelta(hours=5) + new_session = datetime.now(timezone.utc) - timedelta(minutes=10) + # state dict (from first read) carries new_session — the stale warning should clear + mock_pct.return_value = (60.0, 300.0, {"session_start": new_session.isoformat()}) mock_snap = MagicMock() mock_snap.last_warned_at = old_warn mock_snap.time_to_exhaustion.return_value = 15.0 @@ -3842,19 +3875,21 @@ def test_clears_stale_warning_from_previous_session(self, _pct, mock_snap_cls, mock_snap_cls.return_value = mock_snap inst = tmp_path / "inst" inst.mkdir() + # File carries an OLD session_start — if the code re-reads the file it + # would get the old start and fail to clear the stale warning. usage = tmp_path / "usage.json" - usage.write_text(json.dumps({ - "session_start": new_session.isoformat(), - })) + old_session = datetime.now(timezone.utc) - timedelta(hours=6) + usage.write_text(json.dumps({"session_start": old_session.isoformat()})) _maybe_warn_burn_rate(inst, usage) + # stale warning from old_warn (5h ago) should be cleared because state + # says session started 10 min ago (old_warn < new_session) mock_clear.assert_called_once_with(inst) mock_outbox.assert_called_once() - mock_mark.assert_called_once() @patch("app.utils.append_to_outbox", side_effect=OSError("write failed")) @patch("app.burn_rate.mark_warned") @patch("app.burn_rate.BurnRateSnapshot") - @patch("app.iteration_manager._read_session_pct_and_reset", return_value=(60.0, 300.0)) + @patch("app.iteration_manager._read_session_pct_and_reset", return_value=(60.0, 300.0, {})) def test_outbox_write_failure_does_not_mark_warned(self, _pct, mock_snap_cls, mock_mark, _outbox, tmp_path): From 99f788ffa3df64b9334fc5a18754e10c8819e3b4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?K=C5=8Dan?= Date: Tue, 2 Jun 2026 23:06:12 -0600 Subject: [PATCH 08/20] perf(stagnation): read stdout file once per sample via _read_tail helper MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Extracted a _read_tail(stdout_file, lines) helper that centralises the seek-to-end / read / size-guard logic. _tail_hash and classify_stagnation now delegate to it. _sample_once reads the larger classification window (100 lines) once per call; on the abort path it derives the hash from the last sample_lines of that same buffer and passes the buffer directly to _classify_from_bytes — eliminating the independent classify_stagnation file open that raced against the still-writing subprocess. Added _classify_from_bytes (extracted from classify_stagnation) and two new tests in TestSingleReadOnAbort that assert exactly one _read_tail call per _sample_once() invocation, including on the abort path. Closes https://github.com/Anantys-oss/koan/issues/1702 Co-Authored-By: Claude Sonnet 4.6 --- koan/app/stagnation_monitor.py | 135 +++++++++++++++----------- koan/tests/test_stagnation_monitor.py | 73 +++++++++++++- 2 files changed, 150 insertions(+), 58 deletions(-) diff --git a/koan/app/stagnation_monitor.py b/koan/app/stagnation_monitor.py index 8c0c1ddde..b7379938e 100644 --- a/koan/app/stagnation_monitor.py +++ b/koan/app/stagnation_monitor.py @@ -55,19 +55,17 @@ _RETRY_TRACKER_FILENAME = ".stagnation-retries.json" -def _tail_hash(stdout_file: str, sample_lines: int) -> Optional[str]: - """Compute a SHA-256 hash over the last *sample_lines* lines of the file. +def _read_tail(stdout_file: str, lines: int) -> Optional[bytes]: + """Read the last *lines* lines worth of bytes from *stdout_file*. - Returns ``None`` if the file is unreadable, empty, or smaller than - :data:`_DEFAULT_MIN_BYTES`. A ``None`` result means "no signal yet" — - the caller should not count it toward consecutive-identical tracking. + Uses a byte-aligned seek window of ``lines * 200`` bytes — a generous + upper bound for average log-line length. The seek may land mid-codepoint + inside a multi-byte UTF-8 sequence; callers either hash the raw bytes + (where this is fine for equality comparison) or decode with + ``errors='replace'`` (classification). - Note: the seek-window is byte-aligned, not line-semantic. We open in - binary mode, jump to ``size - window``, and split on ``\\n`` — that - seek may land mid-codepoint inside a multi-byte UTF-8 sequence. This - is fine for our equality use-case (the same seek position produces - the same bytes, so identical inputs still hash identically), but the - hash represents byte content, not logical text. + Returns ``None`` when the file is unreadable or smaller than + :data:`_DEFAULT_MIN_BYTES`, signalling "not enough output yet". """ try: size = os.path.getsize(stdout_file) @@ -75,23 +73,36 @@ def _tail_hash(stdout_file: str, sample_lines: int) -> Optional[str]: return None if size < _DEFAULT_MIN_BYTES: return None - try: with open(stdout_file, "rb") as f: - # Read from the end; sample_lines * 200 bytes is a generous - # upper bound (avg log line length) and keeps the hash cheap - # even for multi-megabyte stdout captures. - window = min(size, sample_lines * 200) + window = min(size, lines * 200) f.seek(size - window) - tail = f.read(window) + return f.read(window) except OSError: return None - lines = tail.splitlines() - if len(lines) > sample_lines: - lines = lines[-sample_lines:] - joined = b"\n".join(lines) - return hashlib.sha256(joined).hexdigest() + +def _tail_hash(stdout_file: str, sample_lines: int) -> Optional[str]: + """Compute a SHA-256 hash over the last *sample_lines* lines of the file. + + Returns ``None`` if the file is unreadable, empty, or smaller than + :data:`_DEFAULT_MIN_BYTES`. A ``None`` result means "no signal yet" — + the caller should not count it toward consecutive-identical tracking. + + Note: the seek-window is byte-aligned, not line-semantic. We open in + binary mode, jump to ``size - window``, and split on ``\\n`` — that + seek may land mid-codepoint inside a multi-byte UTF-8 sequence. This + is fine for our equality use-case (the same seek position produces + the same bytes, so identical inputs still hash identically), but the + hash represents byte content, not logical text. + """ + raw = _read_tail(stdout_file, sample_lines) + if raw is None: + return None + blines = raw.splitlines() + if len(blines) > sample_lines: + blines = blines[-sample_lines:] + return hashlib.sha256(b"\n".join(blines)).hexdigest() # --------------------------------------------------------------------------- @@ -119,37 +130,15 @@ def _tail_hash(stdout_file: str, sample_lines: int) -> Optional[str]: ) -def classify_stagnation(stdout_file: str, tail_lines: int = _CLASSIFY_TAIL_LINES) -> tuple: - """Classify the likely root cause of a stagnation event. +def _classify_from_bytes(raw: bytes, tail_lines: int) -> tuple: + """Classify a stagnation pattern from pre-read stdout bytes. - Reads the last *tail_lines* lines of *stdout_file* and applies an ordered - pattern set. Returns ``(pattern_type, excerpt)`` where *excerpt* is at - most 200 chars of representative text. + Extracted from :func:`classify_stagnation` so :meth:`StagnationMonitor._sample_once` + can share a single :func:`_read_tail` call between hash computation and pattern + classification — avoiding a second file read after stagnation is confirmed. - Pattern types (in match order): - - ``tool_loop``: same tool name appears in >= 5 of the sampled lines - - ``infinite_retry``: error keywords appear in >= 3 lines - - ``interactive_wait``: stdin prompt detected - - ``quota_mid_session``: quota / rate-limit markers in output - - ``silent``: file exists but has no content (or below threshold) - - ``unknown``: none of the above + Returns ``(pattern_type, excerpt)`` — same contract as :func:`classify_stagnation`. """ - try: - size = os.path.getsize(stdout_file) - except OSError: - return ("silent", "") - - if size < _DEFAULT_MIN_BYTES: - return ("silent", "") - - try: - with open(stdout_file, "rb") as f: - window = min(size, tail_lines * 200) - f.seek(max(0, size - window)) - raw = f.read(window) - except OSError: - return ("silent", "") - text = raw.decode("utf-8", errors="replace") lines = text.splitlines() if len(lines) > tail_lines: @@ -188,6 +177,27 @@ def classify_stagnation(stdout_file: str, tail_lines: int = _CLASSIFY_TAIL_LINES return ("unknown", _build_excerpt(lines, None)) +def classify_stagnation(stdout_file: str, tail_lines: int = _CLASSIFY_TAIL_LINES) -> tuple: + """Classify the likely root cause of a stagnation event. + + Reads the last *tail_lines* lines of *stdout_file* and applies an ordered + pattern set. Returns ``(pattern_type, excerpt)`` where *excerpt* is at + most 200 chars of representative text. + + Pattern types (in match order): + - ``tool_loop``: same tool name appears in >= 5 of the sampled lines + - ``infinite_retry``: error keywords appear in >= 3 lines + - ``interactive_wait``: stdin prompt detected + - ``quota_mid_session``: quota / rate-limit markers in output + - ``silent``: file exists but has no content (or below threshold) + - ``unknown``: none of the above + """ + raw = _read_tail(stdout_file, tail_lines) + if raw is None: + return ("silent", "") + return _classify_from_bytes(raw, tail_lines) + + def _build_excerpt(lines: list, keyword: Optional[str]) -> str: """Build a <= 200 char excerpt from representative lines. @@ -282,7 +292,21 @@ def _loop(self) -> None: def _sample_once(self) -> None: """Read one sample, update counters, fire callbacks if needed.""" - current = _tail_hash(self._stdout_file, self._sample_lines) + # Read the larger classification window once — it subsumes the hash + # window. On the abort path, the same buffer feeds both the hash + # comparison and the pattern classifier, avoiding a second file read + # while the subprocess is still writing. + classify_lines = max(self._sample_lines, _CLASSIFY_TAIL_LINES) + raw = _read_tail(self._stdout_file, classify_lines) + + # Compute hash from the last sample_lines of the buffer. + current: Optional[str] = None + if raw is not None: + blines = raw.splitlines() + if len(blines) > self._sample_lines: + blines = blines[-self._sample_lines:] + current = hashlib.sha256(b"\n".join(blines)).hexdigest() + if current is None: # Not enough output yet — reset to avoid counting empty samples. self._last_hash = None @@ -312,11 +336,12 @@ def _sample_once(self) -> None: if self._consecutive >= self._abort_after and not self.stagnated: self.stagnated = True - # Classify *before* aborting — the stdout file is still being - # written to by the subprocess, so we get the freshest snapshot. + # Classify from the same buffer used for hash detection — no + # second file read. raw is guaranteed non-None here because + # current is non-None (early return above handles the None case). try: - self.pattern_type, self.pattern_excerpt = classify_stagnation( - self._stdout_file, + self.pattern_type, self.pattern_excerpt = _classify_from_bytes( + raw, _CLASSIFY_TAIL_LINES ) except Exception as e: # Intentional stderr diagnostic — keeps the monitor diff --git a/koan/tests/test_stagnation_monitor.py b/koan/tests/test_stagnation_monitor.py index 3066e8f73..6a0b08d0e 100644 --- a/koan/tests/test_stagnation_monitor.py +++ b/koan/tests/test_stagnation_monitor.py @@ -8,6 +8,7 @@ import pytest +import app.stagnation_monitor as stagnation_monitor from app.stagnation_monitor import ( StagnationMonitor, _mission_key, @@ -752,7 +753,7 @@ def test_pattern_defaults_on_no_stagnation(self, tmp_path): class TestClassifyExceptionInSampleOnce: - """When classify_stagnation raises during _sample_once, monitor still sets stagnated=True. Covers L320-325.""" + """When classification raises during _sample_once, monitor still sets stagnated=True.""" def test_classify_exception_sets_unknown(self, tmp_path): f = tmp_path / "stdout.log" @@ -768,10 +769,10 @@ def test_classify_exception_sets_unknown(self, tmp_path): monitor._sample_once() # 1st duplicate (consecutive=1) assert not monitor.stagnated - # Patch classify_stagnation to raise on the abort path — this + # Patch _classify_from_bytes to raise on the abort path — this # 2nd duplicate is the one that crosses abort_after_cycles=2. with patch( - "app.stagnation_monitor.classify_stagnation", + "app.stagnation_monitor._classify_from_bytes", side_effect=RuntimeError("disk error"), ): monitor._sample_once() # 2nd duplicate (consecutive=2) → abort path @@ -781,6 +782,72 @@ def test_classify_exception_sets_unknown(self, tmp_path): assert monitor.pattern_excerpt == "" +class TestSingleReadOnAbort: + """_sample_once() must read the stdout file only once, even when stagnation fires. + + Before the fix, _tail_hash() and classify_stagnation() each open the file + independently — two reads of a potentially multi-megabyte file, seeing + slightly different snapshots because the subprocess keeps writing. + + After the fix, a single _read_tail() call covers both the hash window and + the classification window. + """ + + def test_file_opened_once_on_abort_sample(self, tmp_path): + f = tmp_path / "stdout.log" + _make_stdout(f, 60) + + monitor = StagnationMonitor( + stdout_file=str(f), + on_abort=lambda: None, + abort_after_cycles=2, + ) + monitor._sample_once() # baseline + monitor._sample_once() # 1st duplicate + + real_read_tail = stagnation_monitor._read_tail + call_count = [] + + def counting_read_tail(stdout_file, lines): + call_count.append(lines) + return real_read_tail(stdout_file, lines) + + with patch("app.stagnation_monitor._read_tail", side_effect=counting_read_tail): + monitor._sample_once() # 2nd duplicate → abort + + assert monitor.stagnated + assert len(call_count) == 1, ( + f"Expected 1 _read_tail call on abort sample, got {len(call_count)}" + ) + + def test_file_opened_once_on_non_abort_sample(self, tmp_path): + f = tmp_path / "stdout.log" + _make_stdout(f, 60) + + monitor = StagnationMonitor( + stdout_file=str(f), + on_abort=lambda: None, + abort_after_cycles=3, + ) + + real_read_tail = stagnation_monitor._read_tail + call_count = [] + + def counting_read_tail(stdout_file, lines): + call_count.append(lines) + return real_read_tail(stdout_file, lines) + + with patch("app.stagnation_monitor._read_tail", side_effect=counting_read_tail): + monitor._sample_once() # baseline + monitor._sample_once() # 1st duplicate (warn) + monitor._sample_once() # 2nd duplicate (no abort yet) + + assert not monitor.stagnated + assert len(call_count) == 3, ( + f"Expected 1 _read_tail call per sample (3 samples), got {len(call_count)}" + ) + + class TestMonitorLoopIntegration: """Cover _loop method (L278-280) — starts, samples, and stops on stagnation.""" From 9654e621cb0ec5bdddf29dc2ef5ed6e2fc4133d8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?K=C5=8Dan?= Date: Wed, 3 Jun 2026 16:07:05 -0600 Subject: [PATCH 09/20] fix(skills): replace broken \U unicode escapes with actual emoji in SKILL.md The lite YAML parser doesn't interpret Python unicode escapes, so emoji fields like "\U0001F50E" were rendered as literal text in Telegram. Fix all 4 affected skills (check_need, spec_audit, doc, alias), strip unnecessary quotes from autoreview, and add quote-stripping to _parse_yaml_lite to prevent this bug class. Co-Authored-By: Claude Opus 4.6 --- koan/app/skills.py | 2 ++ koan/skills/core/alias/SKILL.md | 2 +- koan/skills/core/autoreview/SKILL.md | 2 +- koan/skills/core/check_need/SKILL.md | 2 +- koan/skills/core/check_need/handler.py | 2 +- koan/skills/core/doc/SKILL.md | 2 +- koan/skills/core/spec_audit/SKILL.md | 2 +- koan/tests/test_skills.py | 13 +++++++++++++ 8 files changed, 21 insertions(+), 6 deletions(-) diff --git a/koan/app/skills.py b/koan/app/skills.py index a7836c8b0..f24ed205b 100644 --- a/koan/app/skills.py +++ b/koan/app/skills.py @@ -190,6 +190,8 @@ def _parse_yaml_lite(text: str) -> Dict[str, Any]: if value.startswith("[") and value.endswith("]"): result[key] = _parse_inline_list(value) else: + if len(value) >= 2 and value[0] == value[-1] and value[0] in ('"', "'"): + value = value[1:-1] result[key] = value i += 1 diff --git a/koan/skills/core/alias/SKILL.md b/koan/skills/core/alias/SKILL.md index e40edf5d1..5839197e7 100644 --- a/koan/skills/core/alias/SKILL.md +++ b/koan/skills/core/alias/SKILL.md @@ -2,7 +2,7 @@ name: alias scope: core group: config -emoji: "\U0001F517" +emoji: 🔗 description: Create short aliases for project names (e.g. /alias Template2 tt then /tt queues missions for Template2) version: 1.0.0 audience: bridge diff --git a/koan/skills/core/autoreview/SKILL.md b/koan/skills/core/autoreview/SKILL.md index ea578a125..3a00e9f4c 100644 --- a/koan/skills/core/autoreview/SKILL.md +++ b/koan/skills/core/autoreview/SKILL.md @@ -2,7 +2,7 @@ name: autoreview scope: core group: config -emoji: "🔍" +emoji: 🔍 description: Toggle automatic review+rebase after PR creation per project version: 1.0.0 audience: bridge diff --git a/koan/skills/core/check_need/SKILL.md b/koan/skills/core/check_need/SKILL.md index d8a2d5818..255166b47 100644 --- a/koan/skills/core/check_need/SKILL.md +++ b/koan/skills/core/check_need/SKILL.md @@ -2,7 +2,7 @@ name: check_need scope: core group: code -emoji: "\U0001F50E" +emoji: 🔎 description: "Check if a PR or issue is still needed given the current state of the repo" version: 1.0.0 audience: hybrid diff --git a/koan/skills/core/check_need/handler.py b/koan/skills/core/check_need/handler.py index 013c013ce..9b1df68c3 100644 --- a/koan/skills/core/check_need/handler.py +++ b/koan/skills/core/check_need/handler.py @@ -64,7 +64,7 @@ def handle(ctx) -> Optional[str]: missions_path = ctx.instance_dir / "missions.md" insert_pending_mission(missions_path, mission_entry) - return f"\U0001f50e Relevance check queued for {label}" + return f"🔎 Relevance check queued for {label}" def _resolve_project_name(repo, owner=None): diff --git a/koan/skills/core/doc/SKILL.md b/koan/skills/core/doc/SKILL.md index fa8f1f6d0..c1cd9fcee 100644 --- a/koan/skills/core/doc/SKILL.md +++ b/koan/skills/core/doc/SKILL.md @@ -2,7 +2,7 @@ name: doc scope: core group: code -emoji: "\U0001F4DA" +emoji: 📚 description: Extract and generate structured documentation from a project codebase version: 1.0.0 audience: hybrid diff --git a/koan/skills/core/spec_audit/SKILL.md b/koan/skills/core/spec_audit/SKILL.md index 04ed4690d..5d226c353 100644 --- a/koan/skills/core/spec_audit/SKILL.md +++ b/koan/skills/core/spec_audit/SKILL.md @@ -2,7 +2,7 @@ name: spec_audit scope: core group: code -emoji: "\U0001F4D0" +emoji: 📐 description: Audit docs/code alignment — find spec drift and queue fix missions version: 1.0.0 audience: hybrid diff --git a/koan/tests/test_skills.py b/koan/tests/test_skills.py index e70bb951f..e1c7548c6 100644 --- a/koan/tests/test_skills.py +++ b/koan/tests/test_skills.py @@ -99,6 +99,19 @@ def test_comments_ignored(self): result = _parse_yaml_lite("# comment\nname: test") assert result["name"] == "test" + def test_quoted_scalar_strips_quotes(self): + result = _parse_yaml_lite('emoji: "🔍"\nname: "my skill"') + assert result["emoji"] == "🔍" + assert result["name"] == "my skill" + + def test_single_quoted_scalar(self): + result = _parse_yaml_lite("emoji: '🔎'") + assert result["emoji"] == "🔎" + + def test_unquoted_emoji_unchanged(self): + result = _parse_yaml_lite("emoji: 🔎") + assert result["emoji"] == "🔎" + # --------------------------------------------------------------------------- # _parse_bool_flag From 917d88064c32f2ea293e8555a6a18c33d4dd9cf6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?K=C5=8Dan?= Date: Thu, 4 Jun 2026 00:36:50 +0000 Subject: [PATCH 10/20] fix(review): backfill sentinel fields so terse reviews don't fall back MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A complete, useful review that omits a sentinel-defaultable field (review_summary.checklist, file_comments[].code_snippet, finding_ref, or lgtm) was rejected wholesale by validate_review and replaced with the "⚠️ could not be formatted" placeholder — even though the content was perfectly usable. The retry didn't help because the model was consistently terse (observed on esphome/device-builder#1178: two opus passes, no reflection, fallback posted). _normalize_review_data() now fills these unambiguous sentinels in place before validation: checklist→[], code_snippet→"", finding_ref→"", and lgtm derived from finding severities. It deliberately does NOT fabricate semantically meaningful fields (summary, comment, title, severity, line numbers, checklist item/passed) — genuinely incomplete reviews still fail validation and fall back as before. Co-Authored-By: Claude Opus 4.8 (1M context) --- koan/app/review_runner.py | 55 ++++++++++++++++++++++++++++ koan/tests/test_review_runner.py | 62 ++++++++++++++++++++++++++++++++ 2 files changed, 117 insertions(+) diff --git a/koan/app/review_runner.py b/koan/app/review_runner.py index 105ff9529..17e30d852 100644 --- a/koan/app/review_runner.py +++ b/koan/app/review_runner.py @@ -859,6 +859,59 @@ def _extract_json_text(text: str) -> Optional[str]: return best +def _normalize_review_data(data: object) -> object: + """Backfill sentinel-defaultable fields the model commonly omits. + + The review schema declares every field required with an explicit sentinel + value (empty array / empty string / False) rather than marking any field + optional. But the model often produces a semantically complete, terse + review that simply omits a field whose sentinel is unambiguous — most + commonly ``review_summary.checklist`` (for trivial PRs) and a + ``file_comments[].code_snippet``. Hard-rejecting the whole review for one + such omission discards a useful review and posts the "could not be + formatted" placeholder instead (observed on esphome/device-builder#1178). + + This fills in those sentinel defaults in place so a terse review survives + validation. It deliberately does NOT fabricate semantically meaningful + fields (``summary``, ``comment``, ``title``, ``severity``, line numbers, + checklist ``item``/``passed``) — if those are missing the review is + genuinely incomplete and should still fail validation. + """ + if not isinstance(data, dict): + return data + + # Top-level sentinel: an absent file_comments array means "no inline + # findings", which is a valid (LGTM) review. + if "file_comments" not in data: + data["file_comments"] = [] + + fc = data.get("file_comments") + if isinstance(fc, list): + for item in fc: + if isinstance(item, dict) and "code_snippet" not in item: + item["code_snippet"] = "" + + rs = data.get("review_summary") + if isinstance(rs, dict): + # checklist is explicitly allowed to be empty for trivial PRs. + if "checklist" not in rs: + rs["checklist"] = [] + checklist = rs.get("checklist") + if isinstance(checklist, list): + for entry in checklist: + if isinstance(entry, dict) and "finding_ref" not in entry: + entry["finding_ref"] = "" + # lgtm is derivable from finding severities when omitted: blocking iff + # any critical/warning finding is present. + if "lgtm" not in rs and isinstance(fc, list): + rs["lgtm"] = not any( + isinstance(c, dict) and c.get("severity") in ("critical", "warning") + for c in fc + ) + + return data + + def _parse_review_json(raw_output: str) -> Optional[dict]: """Attempt to parse and validate JSON review output. @@ -875,6 +928,8 @@ def _parse_review_json(raw_output: str) -> Optional[dict]: except (json.JSONDecodeError, ValueError): return None + data = _normalize_review_data(data) + is_valid, errors = validate_review(data) if not is_valid: print( diff --git a/koan/tests/test_review_runner.py b/koan/tests/test_review_runner.py index 03e9a13bd..eedd5d972 100644 --- a/koan/tests/test_review_runner.py +++ b/koan/tests/test_review_runner.py @@ -608,6 +608,68 @@ def test_pr40_regression_parses_and_renders_buckets(self): assert '"file_comments"' not in md assert "```json" not in md + def test_pr1178_terse_lgtm_without_checklist_survives(self): + """Regression for esphome/device-builder#1178. + + The model produced a complete, useful LGTM review but omitted + ``review_summary.checklist`` (allowed to be empty for trivial PRs). + Strict validation previously discarded the whole review and posted + the "could not be formatted" placeholder. The terse review must now + parse, with the checklist backfilled to an empty list. + """ + raw = json.dumps({ + "file_comments": [], + "review_summary": { + "lgtm": True, + "summary": "Clean, behavioral no-op refactor.", + }, + }) + result = _parse_review_json(raw) + assert result is not None + assert result["review_summary"]["lgtm"] is True + assert result["review_summary"]["checklist"] == [] + + def test_file_comment_without_code_snippet_survives(self): + """A finding that omits the optional code_snippet must not sink the + whole review — the sentinel empty string is backfilled.""" + raw = json.dumps({ + "file_comments": [{ + "file": "script/sync_boards.py", + "line_start": 392, "line_end": 392, + "severity": "suggestion", + "title": "Consider a comment", "comment": "Explain the cast.", + }], + "review_summary": {"lgtm": True, "summary": "Minor nit.", "checklist": []}, + }) + result = _parse_review_json(raw) + assert result is not None + assert result["file_comments"][0]["code_snippet"] == "" + + def test_lgtm_derived_from_severities_when_omitted(self): + """When the model omits lgtm, it is derived: blocking iff any + critical/warning finding is present.""" + blocking = json.dumps({ + "file_comments": [{ + "file": "a.py", "line_start": 1, "line_end": 1, + "severity": "critical", "title": "t", "comment": "c", + "code_snippet": "", + }], + "review_summary": {"summary": "Has a blocker.", "checklist": []}, + }) + result = _parse_review_json(blocking) + assert result is not None + assert result["review_summary"]["lgtm"] is False + + def test_missing_summary_still_rejected(self): + """Normalization must NOT fabricate semantically meaningful fields: + a review with no summary is genuinely incomplete and stays rejected.""" + raw = json.dumps({ + "file_comments": [], + "review_summary": {"lgtm": True}, + }) + result = _parse_review_json(raw) + assert result is None + # --------------------------------------------------------------------------- # _format_review_as_markdown From 9735731197f1e3be5eb004bb4ee8c5807ba28eb1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?K=C5=8Dan?= Date: Wed, 3 Jun 2026 22:16:44 -0600 Subject: [PATCH 11/20] feat(skills): add /explain skill for plain-language PR walkthroughs New core skill that takes a PR URL and produces a pedagogical explanation: - What problem the PR solves (with concrete examples) - How the fix works, step by step - The data flow after the change - Whether a simpler approach could have worked Includes handler for Telegram bridge, runner with Claude CLI invocation, dispatch registration with /xp alias, mission dedup support, and 22 tests. Co-Authored-By: Claude Opus 4.6 --- CLAUDE.md | 2 +- docs/users/skills.md | 1 + docs/users/user-manual.md | 16 + koan/app/missions.py | 4 +- koan/app/skill_dispatch.py | 18 +- koan/skills/core/explain/SKILL.md | 18 ++ koan/skills/core/explain/explain_runner.py | 186 ++++++++++++ koan/skills/core/explain/handler.py | 26 ++ koan/skills/core/explain/prompts/explain.md | 89 ++++++ koan/tests/test_explain_runner.py | 308 ++++++++++++++++++++ 10 files changed, 664 insertions(+), 4 deletions(-) create mode 100644 koan/skills/core/explain/SKILL.md create mode 100644 koan/skills/core/explain/explain_runner.py create mode 100644 koan/skills/core/explain/handler.py create mode 100644 koan/skills/core/explain/prompts/explain.md create mode 100644 koan/tests/test_explain_runner.py diff --git a/CLAUDE.md b/CLAUDE.md index 8cae24f04..0b1382940 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -160,7 +160,7 @@ Config additions in `config.py`: `is_api_enabled()`, `get_api_host()` (default ` Extensible command plugin system. Each skill lives in `skills///` with a `SKILL.md` (YAML frontmatter defining commands, aliases, metadata) and an optional `handler.py`. - **`skills.py`** — Registry that discovers SKILL.md files, parses frontmatter (custom lite YAML parser, no PyYAML), maps commands/aliases to skills, and dispatches execution. -- **Core skills** live in `koan/skills/core/` (abort, add_project, ai, alias, ask, audit, autoreview, brainstorm, branches, cancel, changelog, chat, check, check_need, check_notifications, checkup, ci_check, claudemd, config_check, dead_code, deepplan, delete_project, diagnose, doc, doctor, done, email, explore, fix, focus, gh_request, gha_audit, idea, implement, inbox, incident, journal, language, list, live, logs, magic, mission, models, passive, plan, plan_implement, pr, priority, private_security_audit, profile, projects, quota, rebase, recreate, recurring, refactor, reflect, rename, rescan, restart, review, review_rebase, rtk, scaffold_skill, security_audit, shutdown, snapshot, sparring, spec_audit, squash, stats, status, tech_debt, tracker, verbose) +- **Core skills** live in `koan/skills/core/` (abort, add_project, ai, alias, ask, audit, autoreview, brainstorm, branches, cancel, changelog, chat, check, check_need, check_notifications, checkup, ci_check, claudemd, config_check, dead_code, deepplan, delete_project, diagnose, doc, doctor, done, email, explain, explore, fix, focus, gh_request, gha_audit, idea, implement, inbox, incident, journal, language, list, live, logs, magic, mission, models, passive, plan, plan_implement, pr, priority, private_security_audit, profile, projects, quota, rebase, recreate, recurring, refactor, reflect, rename, rescan, restart, review, review_rebase, rtk, scaffold_skill, security_audit, shutdown, snapshot, sparring, spec_audit, squash, stats, status, tech_debt, tracker, verbose) - **Custom skills** loaded from `instance/skills//` — each scope directory can be a cloned Git repo for team sharing. - **Handler pattern**: `def handle(ctx: SkillContext) -> Optional[str]` — return string for Telegram reply, empty string for "already handled", None for no message. - **`worker: true`** flag in SKILL.md marks blocking skills (Claude calls, API requests) that run in a background thread. diff --git a/docs/users/skills.md b/docs/users/skills.md index bb31ded14..857fa5997 100644 --- a/docs/users/skills.md +++ b/docs/users/skills.md @@ -37,6 +37,7 @@ Complete reference for all Koan slash commands. Use these via Telegram, Slack, o | `/implement ` | `/impl` | Queue implementation for a GitHub or Jira issue; never bails — resolves ambiguity with simplest viable solution, retries once before surfacing a problem | Yes | | `/fix ` | — | Understand → plan → test → implement → submit PR | Yes | | `/review ` | `/rv` | Review a pull request | Yes | +| `/explain ` | `/xp` | Explain a PR's changes in plain language with examples and alternative approaches | Yes | | `/rebase ` | `/rb` | Rebase a PR onto its base branch | Yes | | `/planimplement ` | `/planimp`, `/planimpl`, `/planit`, `/plandoit` | Plan then implement an issue (combo: /plan → /implement) | Yes | | `/recreate ` | `/rc` | Re-implement a PR from scratch on a fresh branch | Yes | diff --git a/docs/users/user-manual.md b/docs/users/user-manual.md index 9a140639c..c24c5a700 100644 --- a/docs/users/user-manual.md +++ b/docs/users/user-manual.md @@ -514,6 +514,21 @@ Use this before `/plan` when the idea is architecturally complex, when you want - `/review https://github.com/org/repo/pull/55 --architecture --errors` — Both passes
+**`/explain`** — Explain a PR's intent and changes in plain, simple language. + +- **Usage:** `/explain ` +- **Aliases:** `/xp` +- **GitHub @mention:** `@koan-bot /explain` on a PR + +Produces a pedagogical walkthrough of the PR: what problem it solves (with examples), how the fix works step-by-step, the data flow after the change, and whether a simpler approach could have worked. + +
+Use cases + +- `/explain https://github.com/org/repo/pull/55` — Get a plain-language explanation +- `/xp https://github.com/org/repo/pull/55` — Same thing, shorter +
+ **`/refactor`** — Queue a targeted refactoring mission. - **Usage:** `/refactor ` @@ -1886,6 +1901,7 @@ All commands at a glance. **Tier:** B = Beginner, I = Intermediate, P = Power Us | `/implement ` | `/impl` | I | Implement a GitHub or Jira issue | | `/fix ` | — | I | Full bug-fix pipeline (understand → plan → test → fix → PR) | | `/review [--architecture] [--errors]` | `/rv` | I | Review a pull request | +| `/explain ` | `/xp` | I | Explain a PR in plain language with examples | | `/refactor ` | `/rf` | I | Targeted refactoring mission | | `/ask ` | — | I | Ask a question about a PR/issue — posts AI reply to GitHub | | `/rebase ` | `/rb` | I | Rebase a PR onto its base branch | diff --git a/koan/app/missions.py b/koan/app/missions.py index 69a64d604..71816af82 100644 --- a/koan/app/missions.py +++ b/koan/app/missions.py @@ -2167,9 +2167,9 @@ def update_ci_item_attempt(content: str, pr_url: str) -> str: # /command https://github.com/... → ("command", "url") _GITHUB_ACTION_RE = re.compile( r"/(ask|audit|benchmark|brainstorm|check|check_need|ci_check" - r"|deeplan|deepplan|doc|docs|fix|gh_request" + r"|deeplan|deepplan|doc|docs|explain|fix|gh_request" r"|impl|implement|inspect|need|needs|perf|plan|plandoit|planimp|planimplement|planimpl|planit|profile" - r"|rb|rc|rebase|recreate|refactor|review|reviewrebase|rf|rr|rv" + r"|rb|rc|rebase|recreate|refactor|review|reviewrebase|rf|rr|rv|xp" r"|secu|security|security_audit|sq|squash)\s+" r"(https://github\.com/[^\s]+)" ) diff --git a/koan/app/skill_dispatch.py b/koan/app/skill_dispatch.py index f2e165808..fd85c98b2 100644 --- a/koan/app/skill_dispatch.py +++ b/koan/app/skill_dispatch.py @@ -82,6 +82,7 @@ def _get_skills_dir_mtime(instance_dir: Path) -> float: "doc": "skills.core.doc.doc_runner", "check_need": "skills.core.check_need.check_need_runner", "spec_audit": "skills.core.spec_audit.spec_audit_runner", + "explain": "skills.core.explain.explain_runner", } # Alias -> canonical command name. Declared once, expanded into @@ -101,6 +102,7 @@ def _get_skills_dir_mtime(instance_dir: Path) -> float: "needs": "check_need", "sa": "spec_audit", "drift": "spec_audit", + "xp": "explain", } # Full mapping including aliases — used for runner module lookup. @@ -395,6 +397,7 @@ def build_skill_command( "spec_audit": lambda: _build_project_info_cmd( base_cmd, project_name, project_path, instance_dir, ), + "explain": lambda: _build_explain_cmd(base_cmd, args, project_path, project_name), } def _audit_builder(): return _build_audit_cmd( @@ -654,6 +657,19 @@ def _build_rebase_cmd( return cmd +def _build_explain_cmd( + base_cmd: List[str], args: str, project_path: str, project_name: str = "", +) -> Optional[List[str]]: + """Build explain_runner command.""" + url_match = _PR_URL_RE.search(args) + if not url_match: + return None + cmd = base_cmd + [url_match.group(0), "--project-path", project_path] + if project_name: + cmd.extend(["--project-name", project_name]) + return cmd + + def _build_review_cmd( base_cmd: List[str], args: str, project_path: str, project_name: str = "", ) -> Optional[List[str]]: @@ -971,7 +987,7 @@ def validate_skill_args(command: str, args: str) -> Optional[str]: canonical = _resolve_canonical(command) # Validation rules use canonical names — aliases inherit automatically. - if canonical in ("rebase", "recreate", "review", "squash", "ci_check"): + if canonical in ("rebase", "recreate", "review", "squash", "ci_check", "explain"): if not _PR_URL_RE.search(args): return ( f"/{command} requires a PR URL " diff --git a/koan/skills/core/explain/SKILL.md b/koan/skills/core/explain/SKILL.md new file mode 100644 index 000000000..9877527c4 --- /dev/null +++ b/koan/skills/core/explain/SKILL.md @@ -0,0 +1,18 @@ +--- +name: explain +scope: core +group: code +emoji: 💡 +description: "Explain a PR's intent and changes in plain language (ex: /explain https://github.com/owner/repo/pull/42)" +version: 1.0.0 +audience: hybrid +caveman: false +github_enabled: true +github_context_aware: true +commands: + - name: explain + description: "Explain a PR's changes in simple words with examples and alternative approaches" + usage: "/explain [--now] " + aliases: [xp] +handler: handler.py +--- diff --git a/koan/skills/core/explain/explain_runner.py b/koan/skills/core/explain/explain_runner.py new file mode 100644 index 000000000..22d290a3f --- /dev/null +++ b/koan/skills/core/explain/explain_runner.py @@ -0,0 +1,186 @@ +"""Koan -- PR explanation runner. + +Fetches PR metadata and diff, builds a pedagogical explanation prompt, +and invokes Claude CLI to produce a plain-language walkthrough of the +changes. + +Usage: + python3 -m skills.core.explain.explain_runner --project-path /path +""" + +import sys +from pathlib import Path +from typing import Optional, Tuple + +from app.prompts import load_prompt_or_skill +from app.run_log import log_safe as log + + +def _build_explain_prompt( + context: dict, + skill_dir: Optional[Path] = None, + project_path: Optional[str] = None, +) -> str: + """Build the explanation prompt from PR context.""" + project_memory = "" + if project_path: + from app.skill_memory import build_memory_block_for_skill + + diff = context.get("diff", "") or "" + task_text = "\n".join(filter(None, ( + context.get("title", ""), + context.get("body", ""), + diff[:2000], + ))) + project_memory = build_memory_block_for_skill(project_path, task_text) + + kwargs = dict( + TITLE=context["title"], + AUTHOR=context["author"], + BRANCH=context["branch"], + BASE=context["base"], + BODY=context["body"], + DIFF=context["diff"], + REVIEW_COMMENTS=context.get("review_comments", ""), + REVIEWS=context.get("reviews", ""), + ISSUE_COMMENTS=context.get("issue_comments", ""), + PROJECT_MEMORY=project_memory, + ) + + return load_prompt_or_skill(skill_dir, "explain", **kwargs) + + +def _run_claude_explain( + prompt: str, + project_path: str, + timeout: int = 600, + model: Optional[str] = None, +) -> Tuple[str, str]: + """Run Claude CLI with read-only tools and return the explanation. + + Returns (output, error) tuple. + """ + from app.cli_provider import run_command_streaming + from app.config import get_model_config, get_skill_max_turns + + if model is None: + models = get_model_config() + model = models.get("review_mode") or models.get("mission", "") + + try: + output = run_command_streaming( + prompt=prompt, + project_path=project_path, + allowed_tools=["Read", "Glob", "Grep"], + model_key="mission", + model=model, + max_turns=get_skill_max_turns(), + timeout=timeout, + ) + return output, "" + except RuntimeError as e: + error = str(e) or "unknown error" + print( + f"[explain_runner] Claude explain failed: {error}", + file=sys.stderr, + ) + return "", error + + +def run_explain( + owner: str, + repo: str, + pr_number: str, + project_path: str, + notify_fn=None, + skill_dir: Optional[Path] = None, + project_name: Optional[str] = None, +) -> Tuple[bool, str]: + """Explain a PR in plain language. + + Returns (success, explanation_text) tuple. + """ + if notify_fn is None: + from app.notify import send_telegram + notify_fn = send_telegram + + from app.claude_step import resolve_pr_location + from app.rebase_pr import fetch_pr_context + + try: + owner, repo = resolve_pr_location(owner, repo, pr_number, project_path) + except RuntimeError as e: + return False, str(e) + + full_repo = f"{owner}/{repo}" + notify_fn(f"Explaining PR #{pr_number} ({full_repo})...") + + try: + context = fetch_pr_context(owner, repo, pr_number, project_path) + except Exception as e: + return False, f"Failed to fetch PR context: {e}" + + diff = context.get("diff", "") + if not diff: + return False, f"PR #{pr_number} has no diff — nothing to explain." + + log("explain", f"PR #{pr_number}: {context.get('title', '?')}") + + prompt = _build_explain_prompt( + context, + skill_dir=skill_dir, + project_path=project_path, + ) + + output, error = _run_claude_explain(prompt, project_path) + if error: + return False, f"Explanation failed: {error}" + if not output.strip(): + return False, "Claude returned empty output for explanation." + + summary = ( + f"Explained PR #{pr_number} ({full_repo}): " + f"{context.get('title', '')}\n\n{output}" + ) + return True, summary + + +def main(argv=None): + """CLI entry point for explain_runner.""" + import argparse + + from app.github_url_parser import parse_pr_url + + parser = argparse.ArgumentParser( + description="Explain a GitHub PR in plain language." + ) + parser.add_argument("url", help="GitHub PR URL") + parser.add_argument( + "--project-path", required=True, + help="Local path to the project repository", + ) + parser.add_argument( + "--project-name", + help="Project name for injecting project-specific memory.", + ) + cli_args = parser.parse_args(argv) + + try: + owner, repo, pr_number = parse_pr_url(cli_args.url) + except ValueError as exc: + print(f"Error: {exc}", file=sys.stderr) + return 1 + + skill_dir = Path(__file__).resolve().parent + + success, summary = run_explain( + owner, repo, pr_number, cli_args.project_path, + skill_dir=skill_dir, + project_name=cli_args.project_name, + ) + print(summary) + return 0 if success else 1 + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/koan/skills/core/explain/handler.py b/koan/skills/core/explain/handler.py new file mode 100644 index 000000000..ded5e5d6f --- /dev/null +++ b/koan/skills/core/explain/handler.py @@ -0,0 +1,26 @@ +"""Koan explain skill -- queue a PR explanation mission.""" + +from app.github_url_parser import parse_github_url +from app.missions import extract_now_flag +from app.github_skill_helpers import handle_github_skill + + +def handle(ctx): + """Handle /explain command -- queue a PR explanation mission. + + Usage: + /explain https://github.com/owner/repo/pull/42 + """ + args = ctx.args.strip() if ctx.args else "" + + urgent, args = extract_now_flag(args) + ctx.args = args + + return handle_github_skill( + ctx, + command="explain", + url_type="pr", + parse_func=parse_github_url, + success_prefix="Explanation queued", + urgent=urgent, + ) diff --git a/koan/skills/core/explain/prompts/explain.md b/koan/skills/core/explain/prompts/explain.md new file mode 100644 index 000000000..d72f468eb --- /dev/null +++ b/koan/skills/core/explain/prompts/explain.md @@ -0,0 +1,89 @@ +# PR Explanation + +You are explaining a pull request to a human who wants to deeply understand +the intent, the problem, and the solution — in plain, simple language. + +## Pull Request: {TITLE} + +**Author**: @{AUTHOR} +**Branch**: `{BRANCH}` → `{BASE}` + +### PR Description + +{BODY} +{PROJECT_MEMORY} +--- + +## Current Diff + +```diff +{DIFF} +``` + +--- + +## Existing Discussion + +{REVIEWS} + +{REVIEW_COMMENTS} + +{ISSUE_COMMENTS} + +--- + +## Your Task + +Produce a clear, pedagogical explanation of this PR. Write for someone who +knows the language but may not know this codebase. Use everyday words — no +jargon without immediate explanation. + +### Structure your explanation as follows: + +#### 1. The Problem (What was wrong?) + +- Describe the situation BEFORE this PR in concrete terms +- Use a **specific example** showing the problematic behavior: + what input/action leads to what bad outcome +- Explain WHY it's a problem (user impact, data loss, performance, etc.) + +#### 2. The Solution (How does this PR fix it?) + +- Walk through the key changes step by step, in logical order +- For each change: + - **What** changed (file, function, component) + - **Why** this specific change was needed + - **How** it connects to the overall fix +- Use bullet lists — one idea per bullet, keep each bullet short +- Include a **before/after example** showing the improved behavior + +#### 3. How It Works (The mechanism) + +- Explain the workflow/data flow after the change +- Walk through a concrete scenario end-to-end +- Highlight any edge cases the PR handles + +#### 4. Could It Be Simpler? (Critical analysis) + +Based on your knowledge of the codebase, challenge whether a simpler +approach could have worked: + +- Are there existing utilities or patterns that could have been reused? +- Could the same result be achieved with fewer changes? +- Are there trade-offs in this approach worth noting? +- If the approach IS the simplest viable option, say so and explain why + +### Formatting Rules + +- Use **rich markdown**: headers, bold for emphasis, code blocks for + identifiers and snippets, bullet lists for decomposition +- Keep paragraphs short (2-3 sentences max) +- Use `inline code` for function names, file paths, and variable names +- When referencing a file change, mention the file path +- Concrete examples beat abstract descriptions — always illustrate + +### Tone + +- Direct and clear, like explaining to a smart colleague +- No hedging ("it seems", "it appears") — be confident +- If something is unclear from the diff, say so explicitly diff --git a/koan/tests/test_explain_runner.py b/koan/tests/test_explain_runner.py new file mode 100644 index 000000000..c129ef394 --- /dev/null +++ b/koan/tests/test_explain_runner.py @@ -0,0 +1,308 @@ +"""Tests for the /explain skill runner.""" + +import os +import sys +from pathlib import Path +from unittest.mock import MagicMock, patch + +import pytest + +os.environ.setdefault("KOAN_ROOT", "/tmp/test-koan") +sys.path.insert(0, str(Path(__file__).resolve().parent.parent)) + +from skills.core.explain.explain_runner import ( + _build_explain_prompt, + main, + run_explain, +) + + +SKILL_DIR = Path(__file__).resolve().parent.parent / "skills" / "core" / "explain" + + +class TestBuildExplainPrompt: + """Test prompt building for PR explanations.""" + + def _make_context(self, **overrides): + ctx = { + "title": "Fix off-by-one in pagination", + "author": "alice", + "branch": "fix/pagination", + "base": "main", + "body": "Fixes the pagination bug where page 2 shows page 1 results.", + "diff": "--- a/api.py\n+++ b/api.py\n@@ -10 +10 @@\n-offset = page\n+offset = (page - 1) * size", + "review_comments": "", + "reviews": "", + "issue_comments": "", + } + ctx.update(overrides) + return ctx + + def test_prompt_contains_pr_metadata(self): + context = self._make_context() + prompt = _build_explain_prompt(context, skill_dir=SKILL_DIR) + + assert "Fix off-by-one in pagination" in prompt + assert "alice" in prompt + assert "fix/pagination" in prompt + assert "main" in prompt + + def test_prompt_contains_diff(self): + context = self._make_context() + prompt = _build_explain_prompt(context, skill_dir=SKILL_DIR) + + assert "offset = (page - 1) * size" in prompt + + def test_prompt_contains_body(self): + context = self._make_context() + prompt = _build_explain_prompt(context, skill_dir=SKILL_DIR) + + assert "pagination bug" in prompt + + def test_prompt_with_custom_skill_dir(self, tmp_path): + prompts_dir = tmp_path / "prompts" + prompts_dir.mkdir() + (prompts_dir / "explain.md").write_text( + "Custom prompt: {TITLE} by {AUTHOR}\nDiff:\n{DIFF}" + ) + + context = self._make_context() + prompt = _build_explain_prompt(context, skill_dir=tmp_path) + + assert "Custom prompt:" in prompt + assert "Fix off-by-one in pagination" in prompt + assert "alice" in prompt + + @patch("app.skill_memory.build_memory_block_for_skill") + def test_project_memory_injected(self, mock_memory): + mock_memory.return_value = "\n## Project Learnings\n- Use pagination util\n" + context = self._make_context() + + prompt = _build_explain_prompt( + context, skill_dir=SKILL_DIR, project_path="/some/project", + ) + + mock_memory.assert_called_once() + assert "Project Learnings" in prompt + + +class TestRunExplain: + """Test the main run_explain orchestration.""" + + @patch("app.rebase_pr.fetch_pr_context") + @patch("app.claude_step.resolve_pr_location") + @patch("skills.core.explain.explain_runner._run_claude_explain") + def test_success(self, mock_claude, mock_resolve, mock_fetch): + mock_resolve.return_value = ("owner", "repo") + mock_fetch.return_value = { + "title": "Fix bug", + "author": "bob", + "branch": "fix/bug", + "base": "main", + "body": "Fixes a bug.", + "diff": "+fixed line", + "review_comments": "", + "reviews": "", + "issue_comments": "", + } + mock_claude.return_value = ("Great explanation of the fix.", "") + + notify = MagicMock() + success, summary = run_explain( + "owner", "repo", "42", "/path", + notify_fn=notify, + skill_dir=SKILL_DIR, + ) + + assert success is True + assert "Great explanation" in summary + assert "#42" in summary + notify.assert_called_once() + + @patch("app.rebase_pr.fetch_pr_context") + @patch("app.claude_step.resolve_pr_location") + def test_empty_diff(self, mock_resolve, mock_fetch): + mock_resolve.return_value = ("owner", "repo") + mock_fetch.return_value = { + "title": "Empty PR", + "author": "bob", + "branch": "empty", + "base": "main", + "body": "", + "diff": "", + "review_comments": "", + "reviews": "", + "issue_comments": "", + } + + success, summary = run_explain( + "owner", "repo", "99", "/path", + notify_fn=MagicMock(), + ) + + assert success is False + assert "no diff" in summary.lower() + + @patch("app.claude_step.resolve_pr_location") + def test_pr_not_found(self, mock_resolve): + mock_resolve.side_effect = RuntimeError("PR not found on any remote") + + success, summary = run_explain( + "owner", "repo", "999", "/path", + notify_fn=MagicMock(), + ) + + assert success is False + assert "not found" in summary.lower() + + @patch("app.rebase_pr.fetch_pr_context") + @patch("app.claude_step.resolve_pr_location") + @patch("skills.core.explain.explain_runner._run_claude_explain") + def test_claude_error(self, mock_claude, mock_resolve, mock_fetch): + mock_resolve.return_value = ("owner", "repo") + mock_fetch.return_value = { + "title": "Fix", + "author": "a", + "branch": "b", + "base": "main", + "body": "", + "diff": "+change", + "review_comments": "", + "reviews": "", + "issue_comments": "", + } + mock_claude.return_value = ("", "rate limited") + + success, summary = run_explain( + "owner", "repo", "42", "/path", + notify_fn=MagicMock(), + skill_dir=SKILL_DIR, + ) + + assert success is False + assert "failed" in summary.lower() + + +class TestMain: + """Test the CLI entry point.""" + + @patch("skills.core.explain.explain_runner.run_explain") + def test_main_success(self, mock_run): + mock_run.return_value = (True, "Explanation done.") + exit_code = main([ + "https://github.com/owner/repo/pull/42", + "--project-path", "/path/to/project", + ]) + assert exit_code == 0 + + @patch("skills.core.explain.explain_runner.run_explain") + def test_main_failure(self, mock_run): + mock_run.return_value = (False, "Failed.") + exit_code = main([ + "https://github.com/owner/repo/pull/42", + "--project-path", "/path/to/project", + ]) + assert exit_code == 1 + + def test_main_invalid_url(self): + exit_code = main([ + "not-a-url", + "--project-path", "/path", + ]) + assert exit_code == 1 + + +class TestSkillDispatchIntegration: + """Test that /explain is properly wired in skill_dispatch.""" + + def test_explain_in_canonical_runners(self): + from app.skill_dispatch import _CANONICAL_RUNNERS + assert "explain" in _CANONICAL_RUNNERS + + def test_explain_alias_xp(self): + from app.skill_dispatch import _COMMAND_ALIASES + assert _COMMAND_ALIASES.get("xp") == "explain" + + def test_build_explain_command(self): + from app.skill_dispatch import build_skill_command + + cmd = build_skill_command( + command="explain", + args="https://github.com/owner/repo/pull/42", + project_name="myproject", + project_path="/path/to/project", + koan_root="/koan", + instance_dir="/koan/instance", + ) + + assert cmd is not None + assert "https://github.com/owner/repo/pull/42" in cmd + assert "--project-path" in cmd + assert "/path/to/project" in cmd + assert "--project-name" in cmd + assert "myproject" in cmd + + def test_build_explain_no_url_returns_none(self): + from app.skill_dispatch import build_skill_command + + cmd = build_skill_command( + command="explain", + args="no url here", + project_name="myproject", + project_path="/path", + koan_root="/koan", + instance_dir="/koan/instance", + ) + + assert cmd is None + + def test_validate_explain_requires_pr_url(self): + from app.skill_dispatch import validate_skill_args + + error = validate_skill_args("explain", "no url") + assert error is not None + assert "PR URL" in error + + def test_validate_explain_with_pr_url(self): + from app.skill_dispatch import validate_skill_args + + error = validate_skill_args( + "explain", "https://github.com/owner/repo/pull/42" + ) + assert error is None + + def test_is_skill_mission(self): + from app.skill_dispatch import is_skill_mission + + assert is_skill_mission("/explain https://github.com/o/r/pull/1") + assert is_skill_mission("[project:koan] /explain https://github.com/o/r/pull/1") + + def test_parse_skill_mission(self): + from app.skill_dispatch import parse_skill_mission + + project, cmd, args = parse_skill_mission( + "[project:koan] /explain https://github.com/o/r/pull/1" + ) + assert project == "koan" + assert cmd == "explain" + assert "https://github.com" in args + + +class TestMissionDedup: + """Test that /explain and /xp are covered by mission dedup regex.""" + + def test_explain_dedup(self): + from app.missions import _extract_mission_signature + + sig = _extract_mission_signature( + "/explain https://github.com/owner/repo/pull/42" + ) + assert sig == "explain:https://github.com/owner/repo/pull/42" + + def test_xp_alias_dedup(self): + from app.missions import _extract_mission_signature + + sig = _extract_mission_signature( + "/xp https://github.com/owner/repo/pull/42" + ) + assert sig == "xp:https://github.com/owner/repo/pull/42" From 0a36b1d1b300ded8ecc8b12a431492660bff569c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?K=C5=8Dan?= Date: Wed, 3 Jun 2026 22:57:52 -0600 Subject: [PATCH 12/20] fix(explain): fence external data, enable caveman mode, narrow exception handling --- koan/skills/core/explain/SKILL.md | 2 +- koan/skills/core/explain/explain_runner.py | 51 +++++++++++++--------- 2 files changed, 31 insertions(+), 22 deletions(-) diff --git a/koan/skills/core/explain/SKILL.md b/koan/skills/core/explain/SKILL.md index 9877527c4..db477e133 100644 --- a/koan/skills/core/explain/SKILL.md +++ b/koan/skills/core/explain/SKILL.md @@ -6,7 +6,7 @@ emoji: 💡 description: "Explain a PR's intent and changes in plain language (ex: /explain https://github.com/owner/repo/pull/42)" version: 1.0.0 audience: hybrid -caveman: false +caveman: true github_enabled: true github_context_aware: true commands: diff --git a/koan/skills/core/explain/explain_runner.py b/koan/skills/core/explain/explain_runner.py index 22d290a3f..ceddfb037 100644 --- a/koan/skills/core/explain/explain_runner.py +++ b/koan/skills/core/explain/explain_runner.py @@ -8,6 +8,7 @@ python3 -m skills.core.explain.explain_runner --project-path /path """ +import subprocess import sys from pathlib import Path from typing import Optional, Tuple @@ -34,16 +35,24 @@ def _build_explain_prompt( ))) project_memory = build_memory_block_for_skill(project_path, task_text) + from app.prompt_guard import fence_external_data + kwargs = dict( - TITLE=context["title"], + TITLE=fence_external_data(context["title"], "PR title"), AUTHOR=context["author"], BRANCH=context["branch"], BASE=context["base"], - BODY=context["body"], - DIFF=context["diff"], - REVIEW_COMMENTS=context.get("review_comments", ""), - REVIEWS=context.get("reviews", ""), - ISSUE_COMMENTS=context.get("issue_comments", ""), + BODY=fence_external_data(context.get("body", ""), "PR body"), + DIFF=fence_external_data(context.get("diff", ""), "PR diff", scan=False), + REVIEW_COMMENTS=fence_external_data( + context.get("review_comments", ""), "review comments" + ), + REVIEWS=fence_external_data( + context.get("reviews", ""), "reviews" + ), + ISSUE_COMMENTS=fence_external_data( + context.get("issue_comments", ""), "issue comments" + ), PROJECT_MEMORY=project_memory, ) @@ -65,25 +74,25 @@ def _run_claude_explain( if model is None: models = get_model_config() - model = models.get("review_mode") or models.get("mission", "") + model = models.get("review_mode") or models.get("mission") or None + + cmd_kwargs = dict( + prompt=prompt, + project_path=project_path, + allowed_tools=["Read", "Glob", "Grep"], + model_key="mission", + max_turns=get_skill_max_turns(), + timeout=timeout, + ) + if model: + cmd_kwargs["model"] = model try: - output = run_command_streaming( - prompt=prompt, - project_path=project_path, - allowed_tools=["Read", "Glob", "Grep"], - model_key="mission", - model=model, - max_turns=get_skill_max_turns(), - timeout=timeout, - ) + output = run_command_streaming(**cmd_kwargs) return output, "" except RuntimeError as e: error = str(e) or "unknown error" - print( - f"[explain_runner] Claude explain failed: {error}", - file=sys.stderr, - ) + log("explain", f"Claude explain failed: {error}") return "", error @@ -117,7 +126,7 @@ def run_explain( try: context = fetch_pr_context(owner, repo, pr_number, project_path) - except Exception as e: + except (RuntimeError, OSError, subprocess.CalledProcessError) as e: return False, f"Failed to fetch PR context: {e}" diff = context.get("diff", "") From 5ec6f17af831a2869cabdffc787c5fc2ba0db814 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?K=C5=8Dan?= Date: Thu, 4 Jun 2026 02:53:42 +0000 Subject: [PATCH 13/20] fix(rebase): skip target-repo hooks on agent commits MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Headless agent commits (rebase, squash, claudemd refresh) ran target-repo pre-commit hooks. device-builder-frontend's .husky pre-commit runs 'npm run format:check && npm run lint && npm test', which exceeds the 60s git timeout (npm test can also hang on watch mode), raising an unhandled TimeoutExpired that crashes the whole rebase (PR 609). Add --no-verify to commit_if_changes, squash, and claudemd refresh. CI validates the PR; local hooks are not Kōan's gate. Co-Authored-By: Claude Opus 4.8 (1M context) --- koan/app/claude_step.py | 7 ++++++- koan/app/claudemd_refresh.py | 3 ++- koan/app/squash_pr.py | 4 +++- koan/tests/test_claude_step.py | 15 ++++++++++++++- 4 files changed, 25 insertions(+), 4 deletions(-) diff --git a/koan/app/claude_step.py b/koan/app/claude_step.py index 00246a9ac..f8a9d16ee 100644 --- a/koan/app/claude_step.py +++ b/koan/app/claude_step.py @@ -380,7 +380,12 @@ def commit_if_changes(project_path: str, message: str) -> bool: return False _run_git(["git", "add", "-A"], cwd=project_path) - _run_git(["git", "commit", "-m", message], cwd=project_path) + # --no-verify: Kōan is a headless automation agent. Target repos often ship + # pre-commit hooks (husky/lint-staged) that run lint/format/test on commit. + # Those routinely exceed the 60s git timeout — or hang on watch-mode test + # runners — crashing the rebase with TimeoutExpired. CI validates the PR; + # local hooks are not Kōan's gate. + _run_git(["git", "commit", "--no-verify", "-m", message], cwd=project_path) return True diff --git a/koan/app/claudemd_refresh.py b/koan/app/claudemd_refresh.py index 4937d0fdc..51b573ea6 100644 --- a/koan/app/claudemd_refresh.py +++ b/koan/app/claudemd_refresh.py @@ -126,7 +126,8 @@ def _commit_and_push(project_path: str, branch_name: str, message: str) -> bool: if not _has_changes(project_path): return False run_git_strict("add", "CLAUDE.md", cwd=project_path) - run_git_strict("commit", "-m", message, cwd=project_path) + # --no-verify: skip target-repo pre-commit hooks that can hang/timeout. + run_git_strict("commit", "--no-verify", "-m", message, cwd=project_path) run_git_strict("push", "-u", "origin", branch_name, cwd=project_path) return True diff --git a/koan/app/squash_pr.py b/koan/app/squash_pr.py index 2c707c702..b93022b27 100644 --- a/koan/app/squash_pr.py +++ b/koan/app/squash_pr.py @@ -79,7 +79,9 @@ def _squash_commits( ).strip() _run_git(["git", "reset", "--soft", merge_base], cwd=project_path) - _run_git(["git", "commit", "-m", message], cwd=project_path) + # --no-verify: skip target-repo pre-commit hooks (lint/format/test) that can + # exceed the git timeout and crash the automation. CI is the real gate. + _run_git(["git", "commit", "--no-verify", "-m", message], cwd=project_path) return True diff --git a/koan/tests/test_claude_step.py b/koan/tests/test_claude_step.py index 098bcec30..4db6a2aac 100644 --- a/koan/tests/test_claude_step.py +++ b/koan/tests/test_claude_step.py @@ -608,9 +608,22 @@ def test_with_changes_commits(self, mock_run, mock_git): assert mock_git.call_count == 2 mock_git.assert_any_call(["git", "add", "-A"], cwd="/project") mock_git.assert_any_call( - ["git", "commit", "-m", "test msg"], cwd="/project" + ["git", "commit", "--no-verify", "-m", "test msg"], cwd="/project" ) + @patch("app.claude_step._run_git") + @patch("app.cli_exec.subprocess.run") + def test_commit_skips_target_repo_hooks(self, mock_run, mock_git): + """Agent commits must use --no-verify so target-repo pre-commit hooks + (husky lint/format/test) can't hang past the git timeout and crash the + rebase. Regression for device-builder-frontend PR 609.""" + mock_run.return_value = MagicMock(stdout=" M file.py\n", returncode=0) + commit_if_changes("/project", "msg") + commit_call = next( + c for c in mock_git.call_args_list if "commit" in c.args[0] + ) + assert "--no-verify" in commit_call.args[0] + @patch("app.claude_step._run_git") @patch("app.cli_exec.subprocess.run") def test_whitespace_only_status_is_no_changes(self, mock_run, mock_git): From 40c6a1632486b592eb1c50e40d3765e60ba549d3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?K=C5=8Dan?= Date: Thu, 4 Jun 2026 03:02:23 +0000 Subject: [PATCH 14/20] fix(rebase): scrub leaked name, extend --no-verify to auto-merge, dedup test --- koan/app/claude_step.py | 7 ++----- koan/app/git_auto_merge.py | 6 ++++-- koan/tests/test_claude_step.py | 13 ------------- koan/tests/test_squash_pr.py | 4 +++- 4 files changed, 9 insertions(+), 21 deletions(-) diff --git a/koan/app/claude_step.py b/koan/app/claude_step.py index f8a9d16ee..906425d70 100644 --- a/koan/app/claude_step.py +++ b/koan/app/claude_step.py @@ -380,11 +380,8 @@ def commit_if_changes(project_path: str, message: str) -> bool: return False _run_git(["git", "add", "-A"], cwd=project_path) - # --no-verify: Kōan is a headless automation agent. Target repos often ship - # pre-commit hooks (husky/lint-staged) that run lint/format/test on commit. - # Those routinely exceed the 60s git timeout — or hang on watch-mode test - # runners — crashing the rebase with TimeoutExpired. CI validates the PR; - # local hooks are not Kōan's gate. + # --no-verify: skip target-repo pre-commit hooks (lint/format/test) that can + # exceed the git timeout and crash the automation. CI is the real gate. _run_git(["git", "commit", "--no-verify", "-m", message], cwd=project_path) return True diff --git a/koan/app/git_auto_merge.py b/koan/app/git_auto_merge.py index d36db4ac8..ed7fd382f 100644 --- a/koan/app/git_auto_merge.py +++ b/koan/app/git_auto_merge.py @@ -152,7 +152,8 @@ def _perform_merge_inner(self, branch: str, base_branch: str, strategy: str) -> return False, f"Merge conflict during squash: {stderr}" commit_msg = build_merge_commit_message(branch, strategy, subjects) - exit_code, _, stderr = run_git(self.project_path, "commit", "-m", commit_msg, env=author_env) + # --no-verify: skip target-repo pre-commit hooks that can hang/timeout. + exit_code, _, stderr = run_git(self.project_path, "commit", "--no-verify", "-m", commit_msg, env=author_env) if exit_code != 0: run_git(self.project_path, "reset", "--hard") return False, f"Failed to commit squash: {stderr}" @@ -173,7 +174,8 @@ def _perform_merge_inner(self, branch: str, base_branch: str, strategy: str) -> else: commit_msg = build_merge_commit_message(branch, "merge", subjects) - exit_code, _, stderr = run_git(self.project_path, "merge", "--no-ff", branch, "-m", commit_msg, env=author_env) + # --no-verify: skip target-repo pre-commit hooks that can hang/timeout. + exit_code, _, stderr = run_git(self.project_path, "merge", "--no-verify", "--no-ff", branch, "-m", commit_msg, env=author_env) if exit_code != 0: run_git(self.project_path, "merge", "--abort") return False, f"Merge conflict: {stderr}" diff --git a/koan/tests/test_claude_step.py b/koan/tests/test_claude_step.py index 4db6a2aac..1ca551da1 100644 --- a/koan/tests/test_claude_step.py +++ b/koan/tests/test_claude_step.py @@ -611,19 +611,6 @@ def test_with_changes_commits(self, mock_run, mock_git): ["git", "commit", "--no-verify", "-m", "test msg"], cwd="/project" ) - @patch("app.claude_step._run_git") - @patch("app.cli_exec.subprocess.run") - def test_commit_skips_target_repo_hooks(self, mock_run, mock_git): - """Agent commits must use --no-verify so target-repo pre-commit hooks - (husky lint/format/test) can't hang past the git timeout and crash the - rebase. Regression for device-builder-frontend PR 609.""" - mock_run.return_value = MagicMock(stdout=" M file.py\n", returncode=0) - commit_if_changes("/project", "msg") - commit_call = next( - c for c in mock_git.call_args_list if "commit" in c.args[0] - ) - assert "--no-verify" in commit_call.args[0] - @patch("app.claude_step._run_git") @patch("app.cli_exec.subprocess.run") def test_whitespace_only_status_is_no_changes(self, mock_run, mock_git): diff --git a/koan/tests/test_squash_pr.py b/koan/tests/test_squash_pr.py index e777fe765..a08b94aa5 100644 --- a/koan/tests/test_squash_pr.py +++ b/koan/tests/test_squash_pr.py @@ -130,7 +130,9 @@ def test_squash_sequence(self, mock_run): calls = mock_run.call_args_list assert calls[0][0][0] == ["git", "merge-base", "origin/main", "HEAD"] assert calls[1][0][0] == ["git", "reset", "--soft", "abc123"] - assert calls[2][0][0] == ["git", "commit", "-m", "squash msg"] + # --no-verify: agent commits must skip target-repo pre-commit hooks + # (lint/format/test) that can hang past the git timeout and crash squash. + assert calls[2][0][0] == ["git", "commit", "--no-verify", "-m", "squash msg"] @patch("app.squash_pr._run_git") def test_propagates_error(self, mock_run): From 0b6de92c2b8d55c1ebd7b1cca0575fc9f487e24d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?K=C5=8Dan?= Date: Thu, 4 Jun 2026 03:03:59 +0000 Subject: [PATCH 15/20] fix(squash): update test assertion for --no-verify agent commit --- koan/tests/test_squash_skill.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/koan/tests/test_squash_skill.py b/koan/tests/test_squash_skill.py index 3f020ba1f..9955ba8da 100644 --- a/koan/tests/test_squash_skill.py +++ b/koan/tests/test_squash_skill.py @@ -377,7 +377,7 @@ def test_squash_commits_runs_reset_and_commit(self): calls = [c.args[0] for c in mock_git.call_args_list] assert ["git", "merge-base", "origin/main", "HEAD"] in calls assert ["git", "reset", "--soft", "abc123"] in calls - assert ["git", "commit", "-m", "feat: x"] in calls + assert ["git", "commit", "--no-verify", "-m", "feat: x"] in calls def test_generate_squash_text_success(self): from app.squash_pr import _generate_squash_text From 551b61520c3163575c8f2d24105c1c88838e1ef5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?K=C5=8Dan?= Date: Thu, 4 Jun 2026 03:20:32 +0000 Subject: [PATCH 16/20] fix(rebase): commit with hooks first, fall back to --no-verify only on timeout --- koan/app/claude_step.py | 26 +++++++++++++++++++++++--- koan/app/claudemd_refresh.py | 9 +++++++-- koan/app/git_auto_merge.py | 6 ++---- koan/app/squash_pr.py | 5 ++--- koan/tests/test_claude_step.py | 31 +++++++++++++++++++++++++++++++ koan/tests/test_squash_pr.py | 7 ++++--- koan/tests/test_squash_skill.py | 3 ++- 7 files changed, 71 insertions(+), 16 deletions(-) diff --git a/koan/app/claude_step.py b/koan/app/claude_step.py index 906425d70..ec7686d52 100644 --- a/koan/app/claude_step.py +++ b/koan/app/claude_step.py @@ -366,6 +366,28 @@ def run_claude( } +def _commit_with_hook_fallback(commit_args: list, cwd: str, run_git=None) -> None: + """Commit, attempting target-repo pre-commit hooks first. + + Project pre-commit hooks (lint/format/test) can exceed the git timeout on + first run — a cold env install (nvm/node) easily outlasts the default. A + blanket ``--no-verify`` would never let hooks run; an unguarded hooked + commit crashes the whole pipeline on timeout. So try the hooked commit with + a generous 180s budget, and only on *timeout* retry with ``--no-verify``. + CI remains the real gate. Non-timeout failures propagate unchanged. + + ``run_git`` defaults to :func:`_run_git`; callers may pass their own + module-level reference so patches in tests resolve correctly. + """ + runner = run_git or _run_git + try: + runner(["git", "commit", *commit_args], cwd=cwd, timeout=180) + except (subprocess.TimeoutExpired, RuntimeError) as exc: + if "timed out" not in str(exc).lower(): + raise + runner(["git", "commit", "--no-verify", *commit_args], cwd=cwd) + + def commit_if_changes(project_path: str, message: str) -> bool: """Stage all changes and commit if there are any. @@ -380,9 +402,7 @@ def commit_if_changes(project_path: str, message: str) -> bool: return False _run_git(["git", "add", "-A"], cwd=project_path) - # --no-verify: skip target-repo pre-commit hooks (lint/format/test) that can - # exceed the git timeout and crash the automation. CI is the real gate. - _run_git(["git", "commit", "--no-verify", "-m", message], cwd=project_path) + _commit_with_hook_fallback(["-m", message], project_path, _run_git) return True diff --git a/koan/app/claudemd_refresh.py b/koan/app/claudemd_refresh.py index 51b573ea6..b93a06f12 100644 --- a/koan/app/claudemd_refresh.py +++ b/koan/app/claudemd_refresh.py @@ -22,6 +22,7 @@ from datetime import datetime, timezone from pathlib import Path +from app.claude_step import _commit_with_hook_fallback from app.git_sync import run_git from app.git_utils import run_git_strict from app.prompts import load_skill_prompt @@ -126,8 +127,12 @@ def _commit_and_push(project_path: str, branch_name: str, message: str) -> bool: if not _has_changes(project_path): return False run_git_strict("add", "CLAUDE.md", cwd=project_path) - # --no-verify: skip target-repo pre-commit hooks that can hang/timeout. - run_git_strict("commit", "--no-verify", "-m", message, cwd=project_path) + + def _runner(cmd, cwd=None, timeout=60): + # Adapt _run_git convention (cmd list) to run_git_strict (*args). + return run_git_strict(*cmd[1:], cwd=cwd, timeout=timeout) + + _commit_with_hook_fallback(["-m", message], project_path, _runner) run_git_strict("push", "-u", "origin", branch_name, cwd=project_path) return True diff --git a/koan/app/git_auto_merge.py b/koan/app/git_auto_merge.py index ed7fd382f..d36db4ac8 100644 --- a/koan/app/git_auto_merge.py +++ b/koan/app/git_auto_merge.py @@ -152,8 +152,7 @@ def _perform_merge_inner(self, branch: str, base_branch: str, strategy: str) -> return False, f"Merge conflict during squash: {stderr}" commit_msg = build_merge_commit_message(branch, strategy, subjects) - # --no-verify: skip target-repo pre-commit hooks that can hang/timeout. - exit_code, _, stderr = run_git(self.project_path, "commit", "--no-verify", "-m", commit_msg, env=author_env) + exit_code, _, stderr = run_git(self.project_path, "commit", "-m", commit_msg, env=author_env) if exit_code != 0: run_git(self.project_path, "reset", "--hard") return False, f"Failed to commit squash: {stderr}" @@ -174,8 +173,7 @@ def _perform_merge_inner(self, branch: str, base_branch: str, strategy: str) -> else: commit_msg = build_merge_commit_message(branch, "merge", subjects) - # --no-verify: skip target-repo pre-commit hooks that can hang/timeout. - exit_code, _, stderr = run_git(self.project_path, "merge", "--no-verify", "--no-ff", branch, "-m", commit_msg, env=author_env) + exit_code, _, stderr = run_git(self.project_path, "merge", "--no-ff", branch, "-m", commit_msg, env=author_env) if exit_code != 0: run_git(self.project_path, "merge", "--abort") return False, f"Merge conflict: {stderr}" diff --git a/koan/app/squash_pr.py b/koan/app/squash_pr.py index b93022b27..74eabf28a 100644 --- a/koan/app/squash_pr.py +++ b/koan/app/squash_pr.py @@ -23,6 +23,7 @@ from typing import List, Optional, Tuple from app.claude_step import ( + _commit_with_hook_fallback, _fetch_branch, _get_current_branch, _run_git, @@ -79,9 +80,7 @@ def _squash_commits( ).strip() _run_git(["git", "reset", "--soft", merge_base], cwd=project_path) - # --no-verify: skip target-repo pre-commit hooks (lint/format/test) that can - # exceed the git timeout and crash the automation. CI is the real gate. - _run_git(["git", "commit", "--no-verify", "-m", message], cwd=project_path) + _commit_with_hook_fallback(["-m", message], project_path, _run_git) return True diff --git a/koan/tests/test_claude_step.py b/koan/tests/test_claude_step.py index 1ca551da1..5928de3d7 100644 --- a/koan/tests/test_claude_step.py +++ b/koan/tests/test_claude_step.py @@ -607,10 +607,41 @@ def test_with_changes_commits(self, mock_run, mock_git): assert result is True assert mock_git.call_count == 2 mock_git.assert_any_call(["git", "add", "-A"], cwd="/project") + # Happy path commits WITH hooks (no --no-verify) under a generous timeout; + # --no-verify is only the timeout fallback. + mock_git.assert_any_call( + ["git", "commit", "-m", "test msg"], cwd="/project", timeout=180 + ) + + @patch("app.claude_step._run_git") + @patch("app.cli_exec.subprocess.run") + def test_commit_falls_back_to_no_verify_on_timeout(self, mock_run, mock_git): + """A hook timeout must retry with --no-verify, not crash the pipeline.""" + mock_run.return_value = MagicMock(stdout=" M file.py\n", returncode=0) + # add succeeds, hooked commit times out, --no-verify commit succeeds. + mock_git.side_effect = [ + "", + subprocess.TimeoutExpired(cmd="git commit", timeout=180), + "", + ] + result = commit_if_changes("/project", "test msg") + assert result is True + mock_git.assert_any_call( + ["git", "commit", "-m", "test msg"], cwd="/project", timeout=180 + ) mock_git.assert_any_call( ["git", "commit", "--no-verify", "-m", "test msg"], cwd="/project" ) + @patch("app.claude_step._run_git") + @patch("app.cli_exec.subprocess.run") + def test_commit_propagates_non_timeout_failure(self, mock_run, mock_git): + """Non-timeout commit failures must propagate, not silently --no-verify.""" + mock_run.return_value = MagicMock(stdout=" M file.py\n", returncode=0) + mock_git.side_effect = ["", RuntimeError("git failed: pre-commit rejected")] + with pytest.raises(RuntimeError, match="pre-commit rejected"): + commit_if_changes("/project", "test msg") + @patch("app.claude_step._run_git") @patch("app.cli_exec.subprocess.run") def test_whitespace_only_status_is_no_changes(self, mock_run, mock_git): diff --git a/koan/tests/test_squash_pr.py b/koan/tests/test_squash_pr.py index a08b94aa5..debc3ef25 100644 --- a/koan/tests/test_squash_pr.py +++ b/koan/tests/test_squash_pr.py @@ -130,9 +130,10 @@ def test_squash_sequence(self, mock_run): calls = mock_run.call_args_list assert calls[0][0][0] == ["git", "merge-base", "origin/main", "HEAD"] assert calls[1][0][0] == ["git", "reset", "--soft", "abc123"] - # --no-verify: agent commits must skip target-repo pre-commit hooks - # (lint/format/test) that can hang past the git timeout and crash squash. - assert calls[2][0][0] == ["git", "commit", "--no-verify", "-m", "squash msg"] + # Happy path commits WITH hooks under a generous timeout; --no-verify is + # only the timeout fallback so first-run hook installs get a chance. + assert calls[2][0][0] == ["git", "commit", "-m", "squash msg"] + assert calls[2][1].get("timeout") == 180 @patch("app.squash_pr._run_git") def test_propagates_error(self, mock_run): diff --git a/koan/tests/test_squash_skill.py b/koan/tests/test_squash_skill.py index 9955ba8da..914a872a0 100644 --- a/koan/tests/test_squash_skill.py +++ b/koan/tests/test_squash_skill.py @@ -377,7 +377,8 @@ def test_squash_commits_runs_reset_and_commit(self): calls = [c.args[0] for c in mock_git.call_args_list] assert ["git", "merge-base", "origin/main", "HEAD"] in calls assert ["git", "reset", "--soft", "abc123"] in calls - assert ["git", "commit", "--no-verify", "-m", "feat: x"] in calls + # Happy path commits WITH hooks; --no-verify is only the timeout fallback. + assert ["git", "commit", "-m", "feat: x"] in calls def test_generate_squash_text_success(self): from app.squash_pr import _generate_squash_text From 84ab3564fdcc20d645d2a879b6d7425870edc29d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?K=C5=8Dan?= Date: Thu, 4 Jun 2026 04:54:50 +0000 Subject: [PATCH 17/20] fix(rebase): detect hook rejection by exit code, bypass only on timeout --- koan/app/claude_step.py | 74 +++++++++++++++++++++++++++++++--- koan/app/git_utils.py | 22 +++++++++- koan/tests/test_claude_step.py | 46 +++++++++++++++++++++ 3 files changed, 135 insertions(+), 7 deletions(-) diff --git a/koan/app/claude_step.py b/koan/app/claude_step.py index ec7686d52..151f7c9b2 100644 --- a/koan/app/claude_step.py +++ b/koan/app/claude_step.py @@ -8,6 +8,7 @@ import json import logging +import os import re import shlex import subprocess @@ -19,10 +20,12 @@ from app.cli_exec import popen_cli, stream_with_timeout from app.cli_provider import build_full_command, run_command from app.config import get_model_config +from app.git_utils import GitCommandError from app.git_utils import get_current_branch as _git_utils_get_current_branch from app.git_utils import ordered_remotes, run_git_strict from app.github import pr_create, run_gh, sanitize_github_comment from app.prompts import load_prompt_or_skill +from app.run_log import log_safe class StepResult: @@ -366,15 +369,59 @@ def run_claude( } +def _precommit_hook_path(repo_path: str) -> Optional[Path]: + """Return the path to an executable pre-commit hook, or ``None``. + + Checks the standard git hook location plus Husky (the common JS toolchain + layout). Only files that exist and are executable count. + """ + candidates = [ + Path(repo_path) / ".git" / "hooks" / "pre-commit", + Path(repo_path) / ".husky" / "pre-commit", + ] + for path in candidates: + try: + if path.is_file() and os.access(path, os.X_OK): + return path + except OSError: + continue + return None + + +def is_hook_rejection(exc: GitCommandError, repo_path: str) -> bool: + """Heuristically decide whether *exc* came from a pre-commit hook objecting. + + git reserves exit code 128 for its *own* fatal errors (bad ref, lock + contention, etc.); a hook rejection propagates the hook's own exit code + (typically 1/2). git's "nothing to commit" is the one common exit-1 + false-positive, so it is filtered out. Finally, an executable pre-commit + hook must actually be present. Together these separate "the hook ran and + said no" from "git itself failed". + """ + if exc.returncode == 128: + return False + if "nothing to commit" in (exc.stderr or ""): + return False + return _precommit_hook_path(repo_path) is not None + + def _commit_with_hook_fallback(commit_args: list, cwd: str, run_git=None) -> None: """Commit, attempting target-repo pre-commit hooks first. Project pre-commit hooks (lint/format/test) can exceed the git timeout on first run — a cold env install (nvm/node) easily outlasts the default. A blanket ``--no-verify`` would never let hooks run; an unguarded hooked - commit crashes the whole pipeline on timeout. So try the hooked commit with - a generous 180s budget, and only on *timeout* retry with ``--no-verify``. - CI remains the real gate. Non-timeout failures propagate unchanged. + commit crashes the whole pipeline on timeout. + + The two failure modes are distinct and handled differently: + + * **Timeout** (``subprocess.TimeoutExpired``) — the hook *hung* (e.g. a + watch-mode test runner, or a cold env install). Retry with ``--no-verify`` + so the pipeline makes progress; CI remains the real gate. + * **Fast non-zero exit** (:class:`GitCommandError`) — when this is a hook + *rejection* (see :func:`is_hook_rejection`), the hook evaluated quickly and + objected. Surface its output and re-raise — do *not* bypass it. Genuine + git errors (exit 128, etc.) also re-raise unchanged. ``run_git`` defaults to :func:`_run_git`; callers may pass their own module-level reference so patches in tests resolve correctly. @@ -382,10 +429,25 @@ def _commit_with_hook_fallback(commit_args: list, cwd: str, run_git=None) -> Non runner = run_git or _run_git try: runner(["git", "commit", *commit_args], cwd=cwd, timeout=180) - except (subprocess.TimeoutExpired, RuntimeError) as exc: - if "timed out" not in str(exc).lower(): - raise + except subprocess.TimeoutExpired: + # Hook hung past the budget — bypass it so the pipeline can proceed. runner(["git", "commit", "--no-verify", *commit_args], cwd=cwd) + except GitCommandError as exc: + if is_hook_rejection(exc, cwd): + # Hook ran, evaluated quickly, and objected — respect it. + log_safe( + "git", + f"pre-commit hook rejected the commit (exit {exc.returncode}): " + f"{(exc.stderr or '').strip()[:200]}", + ) + raise + except RuntimeError as exc: + # A non-GitCommandError runner (e.g. a test double) that reports a + # timeout in its message still means the hook hung — bypass it. + if "timed out" in str(exc).lower(): + runner(["git", "commit", "--no-verify", *commit_args], cwd=cwd) + return + raise def commit_if_changes(project_path: str, message: str) -> bool: diff --git a/koan/app/git_utils.py b/koan/app/git_utils.py index 32fb7c26a..eaf34b481 100644 --- a/koan/app/git_utils.py +++ b/koan/app/git_utils.py @@ -13,6 +13,24 @@ from typing import Dict, List, Optional, Tuple +class GitCommandError(RuntimeError): + """Raised by :func:`run_git_strict` when git exits non-zero. + + Subclasses ``RuntimeError`` so existing ``except RuntimeError`` callers keep + working, while carrying the exit code and captured output. This lets callers + distinguish failure modes — notably a pre-commit *hook rejection* (git exits + with the hook's own code, typically 1/2) from a git plumbing error (git + reserves exit code 128 for its own fatal errors). + """ + + def __init__(self, cmd: str, returncode: int, stderr: str, stdout: str = ""): + super().__init__(f"git failed: {cmd} — {stderr[:200]}") + self.cmd = cmd + self.returncode = returncode + self.stderr = stderr + self.stdout = stdout + + def run_git( *args: str, cwd: str = None, @@ -78,7 +96,9 @@ def run_git_strict( ) if result.returncode != 0: cmd_str = " ".join(["git"] + list(args)) - raise RuntimeError(f"git failed: {cmd_str} — {result.stderr[:200]}") + raise GitCommandError( + cmd_str, result.returncode, result.stderr, result.stdout, + ) return result.stdout.strip() diff --git a/koan/tests/test_claude_step.py b/koan/tests/test_claude_step.py index 5928de3d7..62272584c 100644 --- a/koan/tests/test_claude_step.py +++ b/koan/tests/test_claude_step.py @@ -16,12 +16,14 @@ _rebase_onto_target, _run_git, commit_if_changes, + is_hook_rejection, resolve_pr_location, run_claude, run_claude_step, run_project_tests, strip_cli_noise, ) +from app.git_utils import GitCommandError # ---------- _run_git ---------- @@ -642,6 +644,50 @@ def test_commit_propagates_non_timeout_failure(self, mock_run, mock_git): with pytest.raises(RuntimeError, match="pre-commit rejected"): commit_if_changes("/project", "test msg") + @patch("app.claude_step._precommit_hook_path", return_value="/project/.git/hooks/pre-commit") + @patch("app.claude_step._run_git") + @patch("app.cli_exec.subprocess.run") + def test_commit_respects_fast_hook_rejection(self, mock_run, mock_git, _hook): + """A fast hook rejection must re-raise — never silently --no-verify. + + Distinct from a timeout (hook hung): here the hook ran, evaluated + quickly, and objected, so the commit must NOT be bypassed. + """ + mock_run.return_value = MagicMock(stdout=" M file.py\n", returncode=0) + mock_git.side_effect = [ + "", + GitCommandError("git commit", 1, "lint failed"), + ] + with pytest.raises(GitCommandError): + commit_if_changes("/project", "test msg") + # The --no-verify retry must NOT have been attempted. + for c in mock_git.call_args_list: + assert "--no-verify" not in c.args[0] + + +class TestIsHookRejection: + """Tests for is_hook_rejection failure-mode classification.""" + + @patch("app.claude_step._precommit_hook_path", return_value="/repo/.git/hooks/pre-commit") + def test_fast_nonzero_with_hook_present_is_rejection(self, _hook): + exc = GitCommandError("git commit", 1, "eslint found errors") + assert is_hook_rejection(exc, "/repo") is True + + @patch("app.claude_step._precommit_hook_path", return_value="/repo/.git/hooks/pre-commit") + def test_exit_128_is_git_fatal_not_hook(self, _hook): + exc = GitCommandError("git commit", 128, "fatal: bad object") + assert is_hook_rejection(exc, "/repo") is False + + @patch("app.claude_step._precommit_hook_path", return_value="/repo/.git/hooks/pre-commit") + def test_nothing_to_commit_is_not_hook(self, _hook): + exc = GitCommandError("git commit", 1, "nothing to commit, working tree clean") + assert is_hook_rejection(exc, "/repo") is False + + @patch("app.claude_step._precommit_hook_path", return_value=None) + def test_no_hook_present_is_not_rejection(self, _hook): + exc = GitCommandError("git commit", 1, "some error") + assert is_hook_rejection(exc, "/repo") is False + @patch("app.claude_step._run_git") @patch("app.cli_exec.subprocess.run") def test_whitespace_only_status_is_no_changes(self, mock_run, mock_git): From 0e6ca4fadc1a39578c80175a1ac86e2d6471cf84 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?K=C5=8Dan?= Date: Wed, 3 Jun 2026 22:00:35 -0600 Subject: [PATCH 18/20] fix(review): prevent duplicate comment replies on re-review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two fixes for the regression where reviews post duplicate comments: 1. Issue comment self-reply detection: _fetch_issue_comments now collects bot replies and excludes human comments the bot already replied to. Bot replies use "> @user: text..." format — match that quote pattern against human comments to detect prior replies. This is the primary fix for the visible symptom (repeated identical replies). 2. In-progress dedup in _try_assignment_notification: the existing pending-mission check now also scans In Progress, preventing re-queuing while a review is still running. Also adds review cooldown infrastructure to the notification tracker (not yet wired — available for future use if needed). Co-Authored-By: Claude Opus 4.6 --- koan/app/github_command_handler.py | 15 ++-- koan/app/github_notification_tracker.py | 38 ++++++++++ koan/app/review_runner.py | 60 ++++++++++++++-- .../tests/test_github_notification_tracker.py | 35 +++++++++ koan/tests/test_review_runner.py | 72 +++++++++++++++++++ 5 files changed, 209 insertions(+), 11 deletions(-) diff --git a/koan/app/github_command_handler.py b/koan/app/github_command_handler.py index bd8274680..9bc8b143e 100644 --- a/koan/app/github_command_handler.py +++ b/koan/app/github_command_handler.py @@ -1079,22 +1079,23 @@ def _try_assignment_notification( log.error("GitHub assign: KOAN_ROOT not set") return False - from app.missions import list_pending + from app.missions import list_pending, parse_sections from app.utils import insert_pending_mission missions_path = Path(koan_root) / "instance" / "missions.md" - # Deduplicate: skip if a mission for the same URL is already pending. - # This prevents duplicate missions when both an assignment notification - # and an @mention comment arrive for the same PR/issue. + # Deduplicate: skip if a mission for the same URL is already pending + # or in progress. The in-progress check prevents re-queuing while a + # review is still running (e.g., a rebase pushes new commits mid-review). try: content = missions_path.read_text() if missions_path.exists() else "" - pending = list_pending(content) + sections = parse_sections(content) + active = list_pending(content) + sections.get("in_progress", []) url_lower = web_url.lower() - for line in pending: + for line in active: if url_lower in line.lower(): log.debug( - "GitHub assign: mission for %s already pending, skipping", + "GitHub assign: mission for %s already active, skipping", web_url, ) mark_notification_read(notif_id) diff --git a/koan/app/github_notification_tracker.py b/koan/app/github_notification_tracker.py index 03c32a495..82f306008 100644 --- a/koan/app/github_notification_tracker.py +++ b/koan/app/github_notification_tracker.py @@ -139,3 +139,41 @@ def _update(data): locked_json_modify(_threads_path(instance_dir), _update) except OSError: pass # Best-effort — don't break notification processing + + +# --------------------------------------------------------------------------- +# Review cooldown — prevents re-review after bot's own rebase +# --------------------------------------------------------------------------- + +_REVIEW_COOLDOWN_SECONDS = 30 * 60 # 30 minutes + + +def is_review_on_cooldown(instance_dir: str, owner: str, repo: str, pr_number: str) -> bool: + """Check if a review for this PR was recently queued. + + Returns True if a review was queued within the cooldown window. + Prevents the review_requested → review → rebase → new SHA → re-review + feedback loop. + """ + key = f"review_cd:{owner}/{repo}#{pr_number}" + data = _load_threads(instance_dir) + ts = data.get(key) + if ts is None: + return False + return time.time() - ts < _REVIEW_COOLDOWN_SECONDS + + +def set_review_cooldown(instance_dir: str, owner: str, repo: str, pr_number: str) -> None: + """Record that a review was just queued for this PR.""" + key = f"review_cd:{owner}/{repo}#{pr_number}" + try: + from app.locked_file import locked_json_modify + + def _update(data): + _prune_expired(data) + data[key] = time.time() + _cap_entries(data) + + locked_json_modify(_threads_path(instance_dir), _update) + except OSError: + pass diff --git a/koan/app/review_runner.py b/koan/app/review_runner.py index 17e30d852..2652e783e 100644 --- a/koan/app/review_runner.py +++ b/koan/app/review_runner.py @@ -42,6 +42,7 @@ from app.review_schema import validate_review _ISSUE_URL_RE = re.compile(ISSUE_URL_PATTERN) +_QUOTE_RE = re.compile(r'^>\s*@(\S+):\s*(.+)') def load_project_learnings(project_name: Optional[str]) -> str: @@ -131,6 +132,47 @@ def _filter_threads( return filtered +def _exclude_replied_issue_comments( + human_comments: List[dict], + bot_comments: list, +) -> List[dict]: + """Exclude issue comments the bot already replied to. + + Issue comments are flat (no ``in_reply_to_id``), so ``_filter_threads`` + cannot detect self-replies. Bot replies to issue comments use the + format ``> @user: first_line...``. Match that quote pattern against + human comments to detect prior replies. + """ + replied_prefixes: list = [] + for bc in bot_comments: + body = bc.get("body", "") + first_line = body.split("\n")[0] + m = _QUOTE_RE.match(first_line) + if not m: + continue + user = m.group(1).lower() + text = m.group(2).strip() + if text.endswith("..."): + text = text[:-3].rstrip() + if text: + replied_prefixes.append((user, text.lower())) + + if not replied_prefixes: + return human_comments + + filtered = [] + for hc in human_comments: + user = hc.get("user", "").lower() + first_line = hc.get("body", "").split("\n")[0].strip().lower() + already_replied = any( + user == ru and first_line.startswith(rp) + for ru, rp in replied_prefixes + ) + if not already_replied: + filtered.append(hc) + return filtered + + def _fetch_inline_review_comments( full_repo: str, pr_number: str, bot_username: str = "", max_thread_depth: int = 0, @@ -178,8 +220,14 @@ def _fetch_inline_review_comments( def _fetch_issue_comments( full_repo: str, pr_number: str, bot_username: str = "", ) -> List[dict]: - """Fetch issue-level comments (conversation thread) for a PR.""" - results: List[dict] = [] + """Fetch issue-level comments (conversation thread) for a PR. + + Collects bot comments separately and uses them to detect prior replies. + Human comments that the bot already replied to (matching quote pattern) + are excluded from the returned list. + """ + human: List[dict] = [] + bot_replies: list = [] try: raw = run_gh( "api", f"repos/{full_repo}/issues/{pr_number}/comments", @@ -191,8 +239,9 @@ def _fetch_issue_comments( try: item = json.loads(line) if _is_bot_user(item, bot_username): + bot_replies.append(item) continue - results.append({ + human.append({ "id": item["id"], "type": "issue_comment", "user": item["user"], @@ -202,7 +251,10 @@ def _fetch_issue_comments( continue except RuntimeError: pass - return results + + if bot_replies and human: + return _exclude_replied_issue_comments(human, bot_replies) + return human def fetch_repliable_comments( diff --git a/koan/tests/test_github_notification_tracker.py b/koan/tests/test_github_notification_tracker.py index e283445bf..86d6cd276 100644 --- a/koan/tests/test_github_notification_tracker.py +++ b/koan/tests/test_github_notification_tracker.py @@ -7,11 +7,14 @@ from app.github_notification_tracker import ( _MAX_ENTRIES, + _REVIEW_COOLDOWN_SECONDS, _TTL_SECONDS, _threads_path, _tracker_path, is_comment_tracked, + is_review_on_cooldown, is_thread_tracked, + set_review_cooldown, track_comment, track_thread, ) @@ -141,3 +144,35 @@ def test_thread_tracker_independent_from_comment_tracker(self, instance_dir): track_thread(instance_dir, "thread-Y") assert not is_comment_tracked(instance_dir, "thread-Y") assert not is_thread_tracked(instance_dir, "comment-X") + + +# --------------------------------------------------------------------------- +# Review cooldown (prevents re-review after bot's own rebase) +# --------------------------------------------------------------------------- + + +class TestReviewCooldown: + def test_not_on_cooldown_initially(self, instance_dir): + assert not is_review_on_cooldown(instance_dir, "owner", "repo", "42") + + def test_on_cooldown_after_set(self, instance_dir): + set_review_cooldown(instance_dir, "owner", "repo", "42") + assert is_review_on_cooldown(instance_dir, "owner", "repo", "42") + + def test_different_pr_not_on_cooldown(self, instance_dir): + set_review_cooldown(instance_dir, "owner", "repo", "42") + assert not is_review_on_cooldown(instance_dir, "owner", "repo", "99") + + def test_cooldown_expires(self, instance_dir): + """Cooldown expires after the configured window.""" + key = "review_cd:owner/repo#42" + expired_ts = time.time() - _REVIEW_COOLDOWN_SECONDS - 1 + _threads_path(instance_dir).write_text(json.dumps({key: expired_ts})) + assert not is_review_on_cooldown(instance_dir, "owner", "repo", "42") + + def test_cooldown_active_within_window(self, instance_dir): + """Cooldown active within the configured window.""" + key = "review_cd:owner/repo#42" + recent_ts = time.time() - 60 # 1 min ago + _threads_path(instance_dir).write_text(json.dumps({key: recent_ts})) + assert is_review_on_cooldown(instance_dir, "owner", "repo", "42") diff --git a/koan/tests/test_review_runner.py b/koan/tests/test_review_runner.py index eedd5d972..1b7d8a458 100644 --- a/koan/tests/test_review_runner.py +++ b/koan/tests/test_review_runner.py @@ -2324,6 +2324,78 @@ def test_no_bot_username_configured(self): assert _is_bot_user({"user_type": "User", "user": "koan-bot"}, "") is False +class TestExcludeRepliedIssueComments: + """Tests for _exclude_replied_issue_comments — flat issue comment dedup.""" + + def test_excludes_comment_already_replied_to(self): + from app.review_runner import _exclude_replied_issue_comments + human = [ + {"id": 1, "user": "alice", "body": "Looks good overall"}, + {"id": 2, "user": "bob", "body": "Why this approach?"}, + ] + bot = [ + {"body": "> @alice: Looks good overall\n\nThanks!"}, + ] + result = _exclude_replied_issue_comments(human, bot) + assert len(result) == 1 + assert result[0]["id"] == 2 + + def test_keeps_unreplied_comments(self): + from app.review_runner import _exclude_replied_issue_comments + human = [ + {"id": 1, "user": "alice", "body": "New question"}, + ] + bot = [ + {"body": "> @bob: Something else\n\nReply"}, + ] + result = _exclude_replied_issue_comments(human, bot) + assert len(result) == 1 + + def test_handles_truncated_quotes(self): + from app.review_runner import _exclude_replied_issue_comments + long_body = "A" * 200 + human = [{"id": 1, "user": "alice", "body": long_body}] + bot = [{"body": f"> @alice: {long_body[:100]}...\n\nReply"}] + result = _exclude_replied_issue_comments(human, bot) + assert len(result) == 0 + + def test_no_bot_replies_returns_all(self): + from app.review_runner import _exclude_replied_issue_comments + human = [{"id": 1, "user": "alice", "body": "Question"}] + result = _exclude_replied_issue_comments(human, []) + assert len(result) == 1 + + def test_summary_comments_not_matched(self): + """Bot summary comments () don't match as replies.""" + from app.review_runner import _exclude_replied_issue_comments + human = [{"id": 1, "user": "alice", "body": "Good work"}] + bot = [{"body": "\n## PR Review\n\nLooks fine."}] + result = _exclude_replied_issue_comments(human, bot) + assert len(result) == 1 + + def test_case_insensitive_user_match(self): + from app.review_runner import _exclude_replied_issue_comments + human = [{"id": 1, "user": "Alice", "body": "Question here"}] + bot = [{"body": "> @alice: Question here\n\nAnswer"}] + result = _exclude_replied_issue_comments(human, bot) + assert len(result) == 0 + + @patch("app.review_runner.run_gh") + def test_fetch_issue_comments_excludes_replied(self, mock_gh): + """_fetch_issue_comments uses _exclude_replied_issue_comments.""" + from app.review_runner import _fetch_issue_comments + + mock_gh.return_value = "\n".join([ + json.dumps({"id": 10, "user": "alice", "body": "Nice work", "user_type": "User"}), + json.dumps({"id": 11, "user": "koan-bot", "body": "> @alice: Nice work\n\nThanks!", "user_type": "User"}), + json.dumps({"id": 12, "user": "bob", "body": "New question", "user_type": "User"}), + ]) + + result = _fetch_issue_comments("owner/repo", "1", bot_username="koan-bot") + assert len(result) == 1 + assert result[0]["user"] == "bob" + + class TestFilterThreads: """Tests for _filter_threads — thread-aware self-reply prevention.""" From 3637253c34e422bb102650c6d5cbece505c8447b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?K=C5=8Dan?= Date: Wed, 3 Jun 2026 23:17:26 -0600 Subject: [PATCH 19/20] fix(review): tighten quote matching, log cooldown failures, mark unwired infra --- koan/app/github_notification_tracker.py | 11 +++++++++-- koan/app/review_runner.py | 16 ++++++++++------ koan/tests/test_review_runner.py | 12 ++++++++++++ 3 files changed, 31 insertions(+), 8 deletions(-) diff --git a/koan/app/github_notification_tracker.py b/koan/app/github_notification_tracker.py index 82f306008..03da70169 100644 --- a/koan/app/github_notification_tracker.py +++ b/koan/app/github_notification_tracker.py @@ -15,9 +15,12 @@ """ import json +import logging import time from pathlib import Path +log = logging.getLogger(__name__) + _TRACKER_FILE = ".koan-github-processed.json" _TRACKER_FILE_THREADS = ".koan-github-processed-threads.json" @@ -164,7 +167,11 @@ def is_review_on_cooldown(instance_dir: str, owner: str, repo: str, pr_number: s def set_review_cooldown(instance_dir: str, owner: str, repo: str, pr_number: str) -> None: - """Record that a review was just queued for this PR.""" + """Record that a review was just queued for this PR. + + # TODO: wire into github_command_handler or loop_manager alongside + # the self-reply detection for belt-and-suspenders re-review prevention. + """ key = f"review_cd:{owner}/{repo}#{pr_number}" try: from app.locked_file import locked_json_modify @@ -176,4 +183,4 @@ def _update(data): locked_json_modify(_threads_path(instance_dir), _update) except OSError: - pass + log.warning("Failed to set review cooldown for %s/%s#%s", owner, repo, pr_number) diff --git a/koan/app/review_runner.py b/koan/app/review_runner.py index 2652e783e..85eb11ea1 100644 --- a/koan/app/review_runner.py +++ b/koan/app/review_runner.py @@ -143,7 +143,7 @@ def _exclude_replied_issue_comments( format ``> @user: first_line...``. Match that quote pattern against human comments to detect prior replies. """ - replied_prefixes: list = [] + replied_quotes: list = [] for bc in bot_comments: body = bc.get("body", "") first_line = body.split("\n")[0] @@ -152,12 +152,13 @@ def _exclude_replied_issue_comments( continue user = m.group(1).lower() text = m.group(2).strip() - if text.endswith("..."): + truncated = text.endswith("...") + if truncated: text = text[:-3].rstrip() if text: - replied_prefixes.append((user, text.lower())) + replied_quotes.append((user, text.lower(), truncated)) - if not replied_prefixes: + if not replied_quotes: return human_comments filtered = [] @@ -165,8 +166,11 @@ def _exclude_replied_issue_comments( user = hc.get("user", "").lower() first_line = hc.get("body", "").split("\n")[0].strip().lower() already_replied = any( - user == ru and first_line.startswith(rp) - for ru, rp in replied_prefixes + user == ru and ( + first_line[:len(rp)] == rp if was_truncated + else first_line == rp + ) + for ru, rp, was_truncated in replied_quotes ) if not already_replied: filtered.append(hc) diff --git a/koan/tests/test_review_runner.py b/koan/tests/test_review_runner.py index 1b7d8a458..f00e61b72 100644 --- a/koan/tests/test_review_runner.py +++ b/koan/tests/test_review_runner.py @@ -2359,6 +2359,18 @@ def test_handles_truncated_quotes(self): result = _exclude_replied_issue_comments(human, bot) assert len(result) == 0 + def test_exact_match_no_false_positive_on_prefix(self): + """Non-truncated quotes require exact first-line match, not prefix.""" + from app.review_runner import _exclude_replied_issue_comments + human = [ + {"id": 1, "user": "alice", "body": "Some text is wrong"}, + {"id": 2, "user": "alice", "body": "Some text is fine"}, + ] + bot = [{"body": "> @alice: Some text is fine\n\nAgreed"}] + result = _exclude_replied_issue_comments(human, bot) + assert len(result) == 1 + assert result[0]["id"] == 1 + def test_no_bot_replies_returns_all(self): from app.review_runner import _exclude_replied_issue_comments human = [{"id": 1, "user": "alice", "body": "Question"}] From c972476c9bae93c6b2df6c4d3c2b51ad5956d902 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?K=C5=8Dan?= Date: Thu, 4 Jun 2026 07:47:08 -0600 Subject: [PATCH 20/20] fix(review): show only HEAD commit in footer, hide full SHA list The review signature rendered all PR commit SHAs visibly because they sat between two separate HTML comments. Replace with a single hidden HTML comment block and display only HEAD= in the footer. Backward-compatible: extract_commit_shas() reads both old two-marker format and new single-comment format from existing PR comments. Co-Authored-By: Claude Opus 4.6 --- koan/app/review_markers.py | 56 ++++++++++++++++++++- koan/app/review_runner.py | 40 +++++++-------- koan/tests/test_review_markers.py | 81 +++++++++++++++++++++++++++++++ koan/tests/test_review_runner.py | 48 +++++++++++++++--- 4 files changed, 195 insertions(+), 30 deletions(-) diff --git a/koan/app/review_markers.py b/koan/app/review_markers.py index bcd1dd6ec..8ff58a896 100644 --- a/koan/app/review_markers.py +++ b/koan/app/review_markers.py @@ -11,7 +11,8 @@ marker strings short. No ``--`` inside comment bodies (invalid HTML). """ -from typing import Optional +import re +from typing import List, Optional # --------------------------------------------------------------------------- @@ -127,3 +128,56 @@ def replace_section(body: str, start: str, end: str, new_content: str) -> str: # Malformed (start present, end absent) — append block, leave orphan return body + new_block return body[:start_idx] + new_block + body[end_idx + len(end):] + + +# --------------------------------------------------------------------------- +# Hidden commit block (single HTML comment — fully invisible on GitHub) +# --------------------------------------------------------------------------- + +_HIDDEN_COMMITS_TAG = "koan-commits" +_HIDDEN_COMMITS_RE = re.compile( + r"", + re.DOTALL, +) + + +def build_hidden_commit_block(shas: List[str]) -> str: + """Build a single HTML comment containing all commit SHAs. + + Unlike the legacy two-marker format (where SHAs between separate + ```` and ```` comments render visibly), + this embeds everything inside one comment — fully hidden on GitHub. + """ + return f"" + + +def extract_commit_shas(body: str) -> List[str]: + """Extract reviewed commit SHAs from a comment body. + + Tries the new single-comment format first, then falls back to the + legacy two-marker format for backward compatibility with older reviews. + """ + m = _HIDDEN_COMMITS_RE.search(body) + if m: + return [s.strip() for s in m.group(1).splitlines() if s.strip()] + + raw = extract_between_markers(body, COMMIT_IDS_START, COMMIT_IDS_END) + if raw: + return [s.strip() for s in raw.splitlines() if s.strip()] + + return [] + + +def replace_commit_block(body: str, shas: List[str]) -> str: + """Replace or append the hidden commit block in ``body``. + + Removes any legacy two-marker block first, then upserts the new + single-comment format. + """ + body = remove_section(body, COMMIT_IDS_START, COMMIT_IDS_END) + + m = _HIDDEN_COMMITS_RE.search(body) + new_block = build_hidden_commit_block(shas) + if m: + return body[:m.start()] + new_block + body[m.end():] + return body + "\n" + new_block diff --git a/koan/app/review_runner.py b/koan/app/review_runner.py index 85eb11ea1..36e6fe5c6 100644 --- a/koan/app/review_runner.py +++ b/koan/app/review_runner.py @@ -37,6 +37,8 @@ COMMIT_IDS_START, COMMIT_IDS_END, extract_between_markers, + extract_commit_shas, + replace_commit_block, replace_section, ) from app.review_schema import validate_review @@ -1203,14 +1205,19 @@ def _format_review_as_markdown(review_data: dict, title: str = "", bot_username: return "\n".join(lines) -def _build_review_footer(provider_name: str = "", model: str = "") -> str: - """Build the review footer with branding, provider, and model info.""" +def _build_review_footer( + provider_name: str = "", model: str = "", head_sha: str = "", +) -> str: + """Build the review footer with branding, provider, model, and HEAD SHA.""" from app.pr_footer import build_koan_footer - return build_koan_footer( + footer = build_koan_footer( action="Automated review by", provider_name=provider_name, model=model, ) + if head_sha: + footer += f" `HEAD={head_sha[:7]}`" + return footer def _post_review_comment( @@ -1238,7 +1245,8 @@ def _post_review_comment( if len(review_text) > max_len: review_text = review_text[:max_len] + "\n\n_(Review truncated)_" - footer = _build_review_footer(provider_name, model) + head_sha = commit_shas[-1] if commit_shas else "" + footer = _build_review_footer(provider_name, model, head_sha=head_sha) # If body already starts with a ## heading, don't add another if review_text.startswith("## "): @@ -1246,19 +1254,13 @@ def _post_review_comment( else: body = f"{SUMMARY_TAG}\n## Code Review\n\n{review_text}\n\n---\n{footer}" - # Embed commit SHAs when provided; otherwise preserve from existing - # comment so a re-review doesn't clobber prior incremental state. + # Embed commit SHAs in a single hidden HTML comment (fully invisible). if commit_shas: - body = replace_section( - body, COMMIT_IDS_START, COMMIT_IDS_END, "\n".join(commit_shas), - ) + body = replace_commit_block(body, commit_shas) elif existing_comment: - existing_body = existing_comment.get("body", "") - commits_block = extract_between_markers( - existing_body, COMMIT_IDS_START, COMMIT_IDS_END, - ) - if commits_block is not None: - body = replace_section(body, COMMIT_IDS_START, COMMIT_IDS_END, commits_block) + prior = extract_commit_shas(existing_comment.get("body", "")) + if prior: + body = replace_commit_block(body, prior) sanitized = sanitize_github_comment(body) if existing_comment: @@ -1606,13 +1608,7 @@ def run_review( # Step 1e: Extract previously reviewed SHAs from existing comment (Phase 5) prior_shas: List[str] = [] if existing_comment: - raw_prior = extract_between_markers( - existing_comment.get("body", ""), - COMMIT_IDS_START, - COMMIT_IDS_END, - ) - if raw_prior: - prior_shas = [s.strip() for s in raw_prior.splitlines() if s.strip()] + prior_shas = extract_commit_shas(existing_comment.get("body", "")) # If all current commits were already reviewed, skip if current_shas and prior_shas and set(current_shas) == set(prior_shas): diff --git a/koan/tests/test_review_markers.py b/koan/tests/test_review_markers.py index 3085c8304..79483d1bb 100644 --- a/koan/tests/test_review_markers.py +++ b/koan/tests/test_review_markers.py @@ -7,6 +7,9 @@ COMMIT_IDS_START, COMMIT_IDS_END, extract_between_markers, + extract_commit_shas, + build_hidden_commit_block, + replace_commit_block, remove_section, wrap_section, replace_section, @@ -141,3 +144,81 @@ def test_replaces_with_empty_content(self): body = f"{COMMIT_IDS_START}old{COMMIT_IDS_END}" result = replace_section(body, COMMIT_IDS_START, COMMIT_IDS_END, "") assert result == f"{COMMIT_IDS_START}{COMMIT_IDS_END}" + + +# --------------------------------------------------------------------------- +# Hidden commit block (single HTML comment format) +# --------------------------------------------------------------------------- + +class TestBuildHiddenCommitBlock: + def test_single_sha(self): + result = build_hidden_commit_block(["abc123"]) + assert result == "" + + def test_multiple_shas(self): + result = build_hidden_commit_block(["abc123", "def456"]) + assert result == "" + + def test_fully_hidden_no_visible_text(self): + """Entire block is a single HTML comment — nothing renders visibly.""" + result = build_hidden_commit_block(["abc123"]) + assert result.startswith("") + + +class TestExtractCommitShas: + def test_extracts_from_new_format(self): + body = "review text\n" + assert extract_commit_shas(body) == ["abc123", "def456"] + + def test_extracts_from_legacy_format(self): + body = f"review text\n{COMMIT_IDS_START}abc123\ndef456{COMMIT_IDS_END}" + assert extract_commit_shas(body) == ["abc123", "def456"] + + def test_prefers_new_format_over_legacy(self): + body = ( + f"{COMMIT_IDS_START}old1\nold2{COMMIT_IDS_END}" + "\n" + ) + assert extract_commit_shas(body) == ["new1", "new2"] + + def test_returns_empty_list_when_no_block(self): + assert extract_commit_shas("plain review text") == [] + + def test_strips_whitespace_from_shas(self): + body = "" + assert extract_commit_shas(body) == ["abc123", "def456"] + + def test_skips_blank_lines(self): + body = "" + assert extract_commit_shas(body) == ["abc123", "def456"] + + +class TestReplaceCommitBlock: + def test_appends_when_no_block_exists(self): + body = "review text" + result = replace_commit_block(body, ["abc123"]) + assert "abc123" in result + assert "\nfooter" + result = replace_commit_block(body, ["new1"]) + assert "old" not in result + assert "new1" in result + assert "footer" in result + + def test_roundtrip(self): + shas = ["abc123", "def456", "ghi789"] + block = build_hidden_commit_block(shas) + extracted = extract_commit_shas(block) + assert extracted == shas diff --git a/koan/tests/test_review_runner.py b/koan/tests/test_review_runner.py index f00e61b72..104159382 100644 --- a/koan/tests/test_review_runner.py +++ b/koan/tests/test_review_runner.py @@ -911,6 +911,33 @@ def test_footer_contains_provider_and_model(self, mock_gh): assert "Claude" in body assert "model claude-opus-4-6" in body + @patch("app.review_runner.run_gh") + def test_footer_shows_head_sha(self, mock_gh): + """Footer displays HEAD= when commit_shas provided.""" + _post_review_comment( + "owner", "repo", "42", "LGTM", + commit_shas=["abc123full", "def456full"], + ) + body = [a for a in mock_gh.call_args[0] if isinstance(a, str) and "LGTM" in a][0] + assert "`HEAD=def456f`" in body + + @patch("app.review_runner.run_gh") + def test_footer_no_head_sha_without_commits(self, mock_gh): + """Footer omits HEAD= when no commit_shas provided.""" + _post_review_comment("owner", "repo", "42", "LGTM") + body = [a for a in mock_gh.call_args[0] if isinstance(a, str) and "LGTM" in a][0] + assert "HEAD=" not in body + + @patch("app.review_runner.run_gh") + def test_commits_hidden_in_single_html_comment(self, mock_gh): + """Commit SHAs are stored in a single HTML comment (invisible on GitHub).""" + _post_review_comment( + "owner", "repo", "42", "LGTM", + commit_shas=["abc123", "def456"], + ) + body = [a for a in mock_gh.call_args[0] if isinstance(a, str) and "LGTM" in a][0] + assert "" in body + # --------------------------------------------------------------------------- # run_review (integration, mocked externals) @@ -2700,7 +2727,18 @@ def test_preserves_commit_ids_from_existing_comment(self, mock_gh): _post_review_comment("owner", "repo", "42", "New review text", existing) body_arg = [a for a in mock_gh.call_args[0] if isinstance(a, str) and "New review" in a][0] assert "abc123" in body_arg - assert COMMIT_IDS_START in body_arg + assert "