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/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..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: @@ -172,6 +182,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/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 7ac7d976f..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 ` @@ -661,6 +676,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 @@ -1877,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/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/claude_step.py b/koan/app/claude_step.py index 00246a9ac..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,6 +369,87 @@ 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. + + 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. + """ + runner = run_git or _run_git + try: + runner(["git", "commit", *commit_args], cwd=cwd, timeout=180) + 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: """Stage all changes and commit if there are any. @@ -380,7 +464,7 @@ 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) + _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 1ed666038..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,7 +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) - run_git_strict("commit", "-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 @@ -140,6 +146,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 +157,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 +167,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/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/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/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/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..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" @@ -139,3 +142,45 @@ 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. + + # 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 + + def _update(data): + _prune_expired(data) + data[key] = time.time() + _cap_entries(data) + + locked_json_modify(_threads_path(instance_dir), _update) + except OSError: + log.warning("Failed to set review cooldown for %s/%s#%s", owner, repo, pr_number) 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/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/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/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/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 f18e9049e..36e6fe5c6 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 @@ -37,11 +37,14 @@ COMMIT_IDS_START, COMMIT_IDS_END, extract_between_markers, + extract_commit_shas, + replace_commit_block, replace_section, ) 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: @@ -78,46 +81,159 @@ 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 _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_quotes: 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() + truncated = text.endswith("...") + if truncated: + text = text[:-3].rstrip() + if text: + replied_quotes.append((user, text.lower(), truncated)) + + if not replied_quotes: + 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[: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) + 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( 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", @@ -128,12 +244,10 @@ 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): + bot_replies.append(item) continue - results.append({ + human.append({ "id": item["id"], "type": "issue_comment", "user": item["user"], @@ -143,7 +257,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( @@ -154,8 +271,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 +285,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 @@ -262,10 +388,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 @@ -793,6 +917,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. @@ -809,6 +986,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( @@ -1026,10 +1205,27 @@ 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 = "", head_sha: str = "", +) -> str: + """Build the review footer with branding, provider, model, and HEAD SHA.""" + from app.pr_footer import 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( 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,25 +1245,22 @@ def _post_review_comment( if len(review_text) > max_len: review_text = review_text[:max_len] + "\n\n_(Review truncated)_" + 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("## "): - 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. + # 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: @@ -1233,9 +1426,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 @@ -1416,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): @@ -1440,6 +1626,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 +1735,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/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/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/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/app/squash_pr.py b/koan/app/squash_pr.py index 2c707c702..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,7 +80,7 @@ def _squash_commits( ).strip() _run_git(["git", "reset", "--soft", merge_base], cwd=project_path) - _run_git(["git", "commit", "-m", message], cwd=project_path) + _commit_with_hook_fallback(["-m", message], project_path, _run_git) return True diff --git a/koan/app/stagnation_monitor.py b/koan/app/stagnation_monitor.py index f1d8d27b9..b7379938e 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 @@ -54,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) @@ -74,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() # --------------------------------------------------------------------------- @@ -118,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: @@ -187,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. @@ -281,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 @@ -311,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 @@ -354,30 +380,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 +390,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 +408,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 +435,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/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/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/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/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/explain/SKILL.md b/koan/skills/core/explain/SKILL.md new file mode 100644 index 000000000..db477e133 --- /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: true +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..ceddfb037 --- /dev/null +++ b/koan/skills/core/explain/explain_runner.py @@ -0,0 +1,195 @@ +"""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 subprocess +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) + + from app.prompt_guard import fence_external_data + + kwargs = dict( + TITLE=fence_external_data(context["title"], "PR title"), + AUTHOR=context["author"], + BRANCH=context["branch"], + BASE=context["base"], + 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, + ) + + 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") 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(**cmd_kwargs) + return output, "" + except RuntimeError as e: + error = str(e) or "unknown error" + log("explain", f"Claude explain failed: {error}") + 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 (RuntimeError, OSError, subprocess.CalledProcessError) 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/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/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/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_claude_step.py b/koan/tests/test_claude_step.py index 098bcec30..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 ---------- @@ -607,10 +609,85 @@ 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" + ["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._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): 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_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" 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_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): 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) 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("") == "" 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_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 ff6afe6d5..104159382 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, @@ -607,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 @@ -746,6 +809,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 +899,45 @@ 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 + + @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) @@ -2086,10 +2218,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"}] @@ -2097,17 +2230,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"}] @@ -2119,9 +2252,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 = [] @@ -2179,19 +2313,325 @@ 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 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_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"}] + 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.""" + + 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 # --------------------------------------------------------------------------- @@ -2287,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 "