diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml index 67a3dfb69..b78613384 100644 --- a/.github/workflows/tests.yml +++ b/.github/workflows/tests.yml @@ -53,7 +53,7 @@ jobs: - name: Install dependencies run: | pip install -r koan/requirements.txt - pip install pytest pytest-split pytest-cov + pip install pytest pytest-split pytest-cov pytest-xdist - name: Run tests (${{ matrix.group.name }}) if: ${{ !inputs.group || matrix.group.name == inputs.group }} @@ -66,9 +66,11 @@ jobs: run: | if [ -n "${{ matrix.group.split_group }}" ]; then pytest tests/ -m "${{ matrix.group.marker }}" --splits 3 --group ${{ matrix.group.split_group }} -v \ + -n auto --dist loadfile \ --cov=app --cov-report=term-missing else pytest tests/ -m "${{ matrix.group.marker }}" -v \ + -n auto --dist loadfile \ --cov=app --cov-report=term-missing fi diff --git a/CLAUDE.md b/CLAUDE.md index df005d603..02b6617d0 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -18,6 +18,7 @@ make run # Start main agent loop (foreground) make awake # Start Telegram bridge (foreground) make ollama # Start full Ollama stack (ollama serve + awake + run) make dashboard # Start Flask web dashboard (port 5001) +make lint # Run ruff linter (must pass before committing) make test # Run full test suite (pytest + coverage summary) make coverage # Run tests with detailed coverage report (HTML in htmlcov/) make say m="..." # Send test message as if from Telegram @@ -142,6 +143,16 @@ Extensible command plugin system. Each skill lives in `skills///*` branches** (default `koan/`, configurable via `branch_prefix` in `config.yaml`), never commits to main diff --git a/INSTALL.md b/INSTALL.md index a33a9054f..615ec376d 100644 --- a/INSTALL.md +++ b/INSTALL.md @@ -87,14 +87,15 @@ The `instance/` directory is your private data — it's gitignored and never pus ### 2. Set up a messaging platform -Kōan supports **Telegram** (default) and **Slack** for communication. Follow the setup guide for your preferred platform: +Kōan supports **Telegram** (default), **Slack**, and **Matrix** for communication. Follow the setup guide for your preferred platform: | Platform | Setup Guide | Best For | |----------|-------------|----------| | **Telegram** (default) | [docs/messaging-telegram.md](docs/messaging-telegram.md) | Quick setup, works from any network | | **Slack** | [docs/messaging-slack.md](docs/messaging-slack.md) | Team collaboration, workspace integration | +| **Matrix** | [docs/messaging-matrix.md](docs/messaging-matrix.md) | Self-hosted / federated, open protocol | -Both platforms are fully supported with the same feature set. Telegram is recommended for personal use (simpler setup), while Slack is ideal for team environments. +All three platforms expose the same feature set. Telegram is the simplest for personal use, Slack is best for team environments, and Matrix is ideal if you want a self-hosted or federated option. ### 3. Set environment variables @@ -118,6 +119,29 @@ KOAN_SLACK_APP_TOKEN=xapp-your-app-token KOAN_SLACK_CHANNEL_ID=C01234ABCD ``` +**For Matrix:** Matrix can be configured via `.env` *or* via `instance/config.yaml` (recommended — see [docs/messaging-matrix.md](docs/messaging-matrix.md) for the full guide): + +```yaml +# instance/config.yaml (recommended) +messaging: + provider: "matrix" + matrix: + homeserver: "https://matrix.org" + user_id: "@koan:matrix.org" + room_id: "!abcdefghijk:matrix.org" + access_token: "syt_your_token_here" +``` + +Or the legacy `.env` form (env vars override `config.yaml` when set): + +```bash +KOAN_MESSAGING_PROVIDER=matrix +KOAN_MATRIX_HOMESERVER=https://matrix.org +KOAN_MATRIX_ACCESS_TOKEN=syt_your_token_here +KOAN_MATRIX_USER_ID=@koan:matrix.org +KOAN_MATRIX_ROOM_ID=!abcdefghijk:matrix.org +``` + The `.env` file is gitignored — your secrets stay local. See the provider-specific setup guides above for detailed instructions on obtaining these credentials. ### 4. Configure projects diff --git a/Makefile b/Makefile index 8a5a0ea49..db98f2085 100644 --- a/Makefile +++ b/Makefile @@ -2,7 +2,7 @@ export .PHONY: install onboard setup start stop status restart -.PHONY: clean say migrate test test-skills test-strict coverage sync-instance rename-project release +.PHONY: clean say migrate test test-skills test-strict coverage lint sync-instance rename-project release .PHONY: awake run errand-run errand-awake dashboard .PHONY: ollama logs ssh-forward .PHONY: install-systemctl-service uninstall-systemctl-service @@ -14,6 +14,28 @@ PYTHON_BIN ?= python3 VENV ?= .venv PYTHON ?= $(VENV)/bin/$(PYTHON_BIN) +# --- pytest-xdist worker count --- +# Auto-pick the worker count for `make test` based on the environment: +# * CI / GitHub Actions → all available cores (`-n auto`) +# * Remote SSH session → 2 workers (be polite on shared hosts) +# * Local terminal → all available cores (`-n auto`) +# Override anytime with `make test PYTEST_WORKERS=N` (use 0 to disable xdist). +ifneq ($(CI),) + PYTEST_WORKERS ?= auto +else ifneq ($(GITHUB_ACTIONS),) + PYTEST_WORKERS ?= auto +else ifneq ($(SSH_CONNECTION)$(SSH_CLIENT)$(SSH_TTY),) + PYTEST_WORKERS ?= 2 +else + PYTEST_WORKERS ?= auto +endif + +ifeq ($(PYTEST_WORKERS),0) + PYTEST_XDIST_ARGS := +else + PYTEST_XDIST_ARGS := -n $(PYTEST_WORKERS) --dist loadfile +endif + # --- service manager detection --- # Default: foreground processes via pid_manager (no service manager) # Set KOAN_SERVICE_MANAGER=systemd or KOAN_SERVICE_MANAGER=launchd in .env to opt in @@ -49,27 +71,32 @@ say: setup @test -n "$(m)" || (echo "Usage: make say m=\"your message\"" && exit 1) @cd koan && KOAN_ROOT=$(PWD) PYTHONPATH=. ../$(PYTHON) -c "from app.awake import handle_message; handle_message('$(m)')" +lint: setup + $(VENV)/bin/pip install -q ruff 2>/dev/null + $(VENV)/bin/ruff check koan/ + test: setup - $(VENV)/bin/pip install -q pytest pytest-cov 2>/dev/null - cd koan && KOAN_ROOT=/tmp/test-koan PYTHONPATH=. ../$(PYTHON) -m pytest tests/ -v --cov=app --cov-report=term-missing --cov-report=html:htmlcov + @echo "→ pytest workers: $(PYTEST_WORKERS)" + $(VENV)/bin/pip install -q pytest pytest-cov pytest-xdist 2>/dev/null + cd koan && KOAN_ROOT=/tmp/test-koan PYTHONPATH=. ../$(PYTHON) -m pytest tests/ -v $(PYTEST_XDIST_ARGS) --cov=app --cov-report=term-missing --cov-report=html:htmlcov @$(MAKE) --no-print-directory test-skills test-skills: setup @if [ -d instance/skills ] && find -L instance/skills -path '*/tests/test_*.py' -print -quit 2>/dev/null | grep -q .; then \ - $(VENV)/bin/pip install -q pytest pytest-cov 2>/dev/null; \ + $(VENV)/bin/pip install -q pytest pytest-cov pytest-xdist 2>/dev/null; \ echo "→ running skill-local tests (instance/skills/**/tests)"; \ - KOAN_REPO=$(PWD) KOAN_ROOT=/tmp/test-koan PYTHONPATH=koan $(PYTHON) -m pytest instance/skills/ -v; \ + KOAN_REPO=$(PWD) KOAN_ROOT=/tmp/test-koan PYTHONPATH=koan $(PYTHON) -m pytest instance/skills/ -v $(PYTEST_XDIST_ARGS); \ else \ echo "→ no skill-local tests found under instance/skills/**/tests/ — skipping"; \ fi test-strict: setup - @echo "→ running full test suite in strict mode (0 failures required)" - $(VENV)/bin/pip install -q pytest pytest-cov 2>/dev/null - @cd koan && KOAN_ROOT=/tmp/test-koan PYTHONPATH=. ../$(PYTHON) -m pytest tests/ -q --tb=short \ + @echo "→ running full test suite in strict mode (0 failures required, workers: $(PYTEST_WORKERS))" + $(VENV)/bin/pip install -q pytest pytest-cov pytest-xdist 2>/dev/null + @cd koan && KOAN_ROOT=/tmp/test-koan PYTHONPATH=. ../$(PYTHON) -m pytest tests/ -q --tb=short $(PYTEST_XDIST_ARGS) \ || (echo "✗ tests failed — aborting" && exit 1) @if [ -d instance/skills ] && find -L instance/skills -path '*/tests/test_*.py' -print -quit 2>/dev/null | grep -q .; then \ - KOAN_REPO=$(PWD) KOAN_ROOT=/tmp/test-koan PYTHONPATH=koan $(PYTHON) -m pytest instance/skills/ -q --tb=short \ + KOAN_REPO=$(PWD) KOAN_ROOT=/tmp/test-koan PYTHONPATH=koan $(PYTHON) -m pytest instance/skills/ -q --tb=short $(PYTEST_XDIST_ARGS) \ || (echo "✗ skill-local tests failed — aborting" && exit 1); \ fi @echo "✓ all tests passed" diff --git a/README.md b/README.md index 0ade61991..3b39dc91f 100644 --- a/README.md +++ b/README.md @@ -42,7 +42,7 @@ You pay for AI coding quota. You use it 8 hours a day. The other 16? Wasted quota. -Koan fixes that. It's a background agent that runs on your machine, pulls tasks from a shared mission queue, executes them via your configured CLI provider (Claude Code, Codex, Copilot, or local), and reports back through Telegram or Slack. It writes code in isolated branches, never touches `main`, and waits for your review before anything ships. +Koan fixes that. It's a background agent that runs on your machine, pulls tasks from a shared mission queue, executes them via your configured CLI provider (Claude Code, Codex, Copilot, or local), and reports back through Telegram, Slack, or Matrix. It writes code in isolated branches, never touches `main`, and waits for your review before anything ships. **The agent proposes. The human decides.** @@ -98,7 +98,7 @@ But Koan takes a different path entirely. | **Getting started** | `npm install -g openclaw` + onboarding wizard | TOML config, pairing codes, allowlists | `make install` — interactive web wizard, ready in minutes | | **Safety model** | Pairing codes, sandbox optional — but has shell access, browser control, and can send emails autonomously | Mandatory sandboxing, command allowlists, encrypted keys | Branch isolation, draft PRs only, never touches `main`, human review required | | **Memory** | Local Markdown files, session persistence | Hybrid BM25/vector search, multiple backends | Markdown-based — per-project learnings, session journals, personality evolution. No database needed | -| **Communication** | 21+ channels (WhatsApp, Telegram, Slack, Discord, iMessage, Signal…) | 15+ channels (Telegram, Discord, Slack, iMessage…) | Telegram/Slack with personality-aware formatting, spontaneous messages, and verbose mode | +| **Communication** | 21+ channels (WhatsApp, Telegram, Slack, Discord, iMessage, Signal…) | 15+ channels (Telegram, Discord, Slack, iMessage…) | Telegram, Slack, or Matrix with personality-aware formatting, spontaneous messages, and verbose mode | | **Quota awareness** | No | No | Adapts work depth to remaining API quota (DEEP → IMPLEMENT → REVIEW → WAIT) | | **Extensibility** | 100+ AgentSkills, skill marketplace, 50+ integrations | Trait-based plugin system | 44 built-in skills + pluggable skill system (install from Git repos) | | **Scope** | Everything — emails, web browsing, car negotiations, legal filings | Everything — any LLM task in any context | One thing, done right — autonomous GitHub collaboration | @@ -108,7 +108,7 @@ OpenClaw and ZeroClaw are general-purpose autonomous agents that can do *anythin ## How It Works ``` - You (Telegram/Slack) + You (Telegram/Slack/Matrix) │ ▼ ┌─────────────────┐ ┌──────────────────┐ @@ -168,7 +168,7 @@ Communication happens through shared markdown files in `instance/` — atomic wr ### Communication -- **Telegram & Slack** — Pluggable messaging with flood protection +- **Telegram, Slack & Matrix** — Pluggable messaging with flood protection - **Email digests** — Optional SMTP email notifications for session summaries (rate-limited, deduplicated) - **Personality-aware formatting** — Every outbox message passes through Claude with soul + memory context - **Verbose mode** — Real-time progress updates streamed to your phone diff --git a/docs/github-commands.md b/docs/github-commands.md index 11ba505f2..d24e69d7a 100644 --- a/docs/github-commands.md +++ b/docs/github-commands.md @@ -325,5 +325,6 @@ See [Jira Integration](jira-integration.md) for full setup instructions and the - [Skills README](../koan/skills/README.md) — Skill authoring guide with `github_enabled` flag documentation - [Messaging: Telegram](messaging-telegram.md) — Alternative command interface via Telegram - [Messaging: Slack](messaging-slack.md) — Alternative command interface via Slack +- [Messaging: Matrix](messaging-matrix.md) — Alternative command interface via Matrix - [PR #251](https://github.com/sukria/koan/pull/251) — Original implementation - [Issue #243](https://github.com/sukria/koan/issues/243) — Feature request and design plan diff --git a/docs/jira-integration.md b/docs/jira-integration.md index 2bfb7fb8b..e8e8307d5 100644 --- a/docs/jira-integration.md +++ b/docs/jira-integration.md @@ -360,5 +360,6 @@ Expected behavior. The in-memory processed set is lost on restart, but the persi - [GitHub Notification Commands](github-commands.md) — GitHub @mention integration (complementary) - [Messaging: Telegram](messaging-telegram.md) — Primary command interface - [Messaging: Slack](messaging-slack.md) — Alternative messaging provider +- [Messaging: Matrix](messaging-matrix.md) — Alternative messaging provider - [Skills Reference](skills.md) — Full skill documentation - [User Manual](user-manual.md) — Complete usage guide diff --git a/docs/messaging-matrix.md b/docs/messaging-matrix.md new file mode 100644 index 000000000..3a5175219 --- /dev/null +++ b/docs/messaging-matrix.md @@ -0,0 +1,123 @@ +# Matrix Setup Guide + +This guide covers setting up Kōan with [Matrix](https://matrix.org) as the messaging provider. Kōan talks to a Matrix homeserver via the Client-Server HTTP API — no extra Python packages are required beyond `requests`. + +## Prerequisites + +- Access to a Matrix homeserver. You can use [matrix.org](https://matrix.org), a self-hosted Synapse/Dendrite/Conduit, or any compliant server. +- A dedicated Matrix account for the bot (recommended — don't reuse your personal account). +- An Element (or other Matrix client) login for the bot account, to invite it into the operating room. + +## Step 1: Create a Bot Account + +Either register a new account directly on the homeserver or use an existing dedicated account. The user ID will look like `@koan:matrix.org`. + +## Step 2: Obtain an Access Token + +The easiest way is to log in via Element with the bot account, then: + +1. Open Element → **Settings → Help & About** +2. Scroll to the bottom and expand **Access Token** +3. Copy the token (long string starting with `syt_`, `mat_`, or similar) + +Alternatively, use the `/login` API endpoint: + +```bash +curl -XPOST -d '{ + "type": "m.login.password", + "user": "koan", + "password": "YOUR_BOT_PASSWORD" +}' "https://matrix.org/_matrix/client/v3/login" +``` + +The response contains an `access_token` field. + +> **Security note:** The access token grants full account access. Treat it like a password — never commit it. If leaked, log out the session via Element (**Settings → Sessions**) to invalidate it. + +## Step 3: Create or Choose a Room + +Pick the room Kōan will operate in. Either: + +- Create a new private room in Element and invite the bot. +- Use an existing room and invite the bot. + +Get the room ID: + +1. In Element, open the room +2. Click the room name → **Settings → Advanced** +3. Copy the **Internal room ID** (e.g., `!abcdefghijk:matrix.org`) + +Make sure the bot account has joined the room (accept the invite from the bot's session, or call `/_matrix/client/v3/join/{roomId}`). + +## Step 4: Configure Kōan + +The recommended approach is to put Matrix settings in `instance/config.yaml`: + +```yaml +messaging: + provider: "matrix" + matrix: + homeserver: "https://matrix.org" + user_id: "@koan:matrix.org" + room_id: "!abcdefghijk:matrix.org" + access_token: "syt_your_token_here" +``` + +> Treat `instance/config.yaml` like a secret file — it's gitignored by default. If you commit your `instance/` directory to a separate private repo, that's fine; never commit the access token to a public repo. + +### Legacy: environment variables + +The four `KOAN_MATRIX_*` env vars are still supported and override `config.yaml` when set. Use them only if you have a workflow built around `.env`: + +```bash +# .env (legacy alternative) +KOAN_MESSAGING_PROVIDER=matrix +KOAN_MATRIX_HOMESERVER=https://matrix.org +KOAN_MATRIX_ACCESS_TOKEN=syt_your_token_here +KOAN_MATRIX_USER_ID=@koan:matrix.org +KOAN_MATRIX_ROOM_ID=!abcdefghijk:matrix.org +``` + +Precedence: env var > `config.yaml` value > error. + +## Step 5: Start Kōan + +```bash +make start +``` + +You should see in the logs: + +``` +[init] Messaging provider: MATRIX, Channel: !abcdefghijk:matrix.org +``` + +## How it works + +- **Sending**: `PUT /_matrix/client/v3/rooms/{roomId}/send/m.room.message/{txnId}` with `msgtype: m.text`. Long messages are chunked to 4000 characters per event. +- **Receiving**: Long-polls `GET /_matrix/client/v3/sync` with a 30-second timeout. The first sync discards historical events and records the `next_batch` cursor; subsequent syncs return only new events. +- **Filtering**: Only `m.room.message` events with `msgtype: m.text` are surfaced. Messages sent by the bot's own user ID are ignored so it doesn't reply to itself. + +## Troubleshooting + +### "Missing required settings" + +All four values (`homeserver`, `access_token`, `user_id`, `room_id`) must be set — either under `messaging.matrix` in `instance/config.yaml` or via the corresponding `KOAN_MATRIX_*` env vars. + +### `[matrix] API error 401` / `403` + +- The access token is invalid or has been revoked. Generate a new one (Step 2). +- The bot account isn't joined to the room. Accept the invite first. + +### `[matrix] API error 404` + +- The room ID is wrong, or the homeserver doesn't know about it. +- Ensure the room ID starts with `!` and includes the homeserver suffix (e.g., `!abc:matrix.org`). + +### Bot replies to its own messages + +- Double-check `KOAN_MATRIX_USER_ID` exactly matches the bot's user ID (including the leading `@` and the homeserver part). + +### Encrypted rooms + +This integration uses unencrypted Matrix rooms. End-to-end encryption (Olm/Megolm) is not implemented — using an E2EE room means messages will appear as undecryptable events. Either disable encryption on the room or create a fresh unencrypted room for the bot. diff --git a/docs/user-manual.md b/docs/user-manual.md index c95fcffc7..ead7bd015 100644 --- a/docs/user-manual.md +++ b/docs/user-manual.md @@ -1447,6 +1447,21 @@ Each finding becomes a GitHub issue with: - **Suggested Fix** — Concrete remediation steps - **Details table** — Severity, category, location, and effort estimate +**Private Vulnerability Reporting (PVRS):** When the target repository has GitHub's Private Vulnerability Reporting enabled, critical and high severity findings are automatically submitted as private security advisories instead of public issues. This prevents disclosure of exploitable vulnerabilities before a fix is applied. Lower-severity findings still create public issues. + +Configure PVRS behavior per-project in `projects.yaml`: + +```yaml +defaults: + security: + pvrs: auto # auto (detect), true (force), false (public only) + pvrs_threshold: high # minimum severity for PVRS (critical, high, medium, low) +projects: + myapp: + security: + pvrs: false # always use public issues for this project +``` + ### Incident Triage **`/incident`** — Triage a production error from a stack trace or log snippet. Kōan will parse the error, identify the root cause, propose a fix with tests, and submit a draft PR. diff --git a/env.example b/env.example index b8658b0a9..9b440c96c 100644 --- a/env.example +++ b/env.example @@ -10,7 +10,7 @@ # MESSAGING PROVIDER (optional — defaults to Telegram) # ========================================================================= # Which messaging platform Kōan uses for communication. -# Options: "telegram" (default), "slack" +# Options: "telegram" (default), "slack", "matrix" # Can also be set in config.yaml (env var takes priority) # KOAN_MESSAGING_PROVIDER=telegram @@ -40,6 +40,26 @@ # Find it via: Right-click channel → View channel details # KOAN_SLACK_CHANNEL_ID= +# ========================================================================= +# MATRIX CONFIGURATION (legacy — prefer instance/config.yaml) +# ========================================================================= +# The cleaner setup for Matrix is in instance/config.yaml under +# messaging.matrix. The env vars below are legacy overrides — kept for +# backward compatibility, take priority over config.yaml when set. +# See docs/messaging-matrix.md for setup instructions. + +# Homeserver base URL (e.g., https://matrix.org) +# KOAN_MATRIX_HOMESERVER= + +# Access token for the bot account (obtain via /login API or Element) +# KOAN_MATRIX_ACCESS_TOKEN= + +# Bot's full Matrix user ID (e.g., @koan:matrix.org) +# KOAN_MATRIX_USER_ID= + +# Room ID where Kōan will listen and respond (e.g., !abcdef:matrix.org) +# KOAN_MATRIX_ROOM_ID= + # ========================================================================= # PROJECT CONFIGURATION # ========================================================================= diff --git a/instance.example/config.yaml b/instance.example/config.yaml index 5cf421fc3..2dd17c724 100644 --- a/instance.example/config.yaml +++ b/instance.example/config.yaml @@ -204,8 +204,21 @@ telegram: # Controls which messaging platform Kōan uses for communication. # If not specified, defaults to "telegram" (backward compatible). # Can also be set via KOAN_MESSAGING_PROVIDER env var (overrides this). +# +# Recommended: configure here in config.yaml rather than via env vars. +# Env vars (KOAN_MATRIX_*, KOAN_SLACK_*, KOAN_TELEGRAM_*) are kept as +# legacy/override fallbacks but the cleaner setup lives here. +# +# Telegram and Slack credentials still come from env vars (see env.example). +# Matrix supports config.yaml directly: +# # messaging: -# provider: "telegram" # "telegram" (default) or "slack" +# provider: "telegram" # "telegram" (default), "slack", or "matrix" +# matrix: +# homeserver: "https://matrix.org" +# user_id: "@koan:matrix.org" +# room_id: "!abcdefghijk:matrix.org" +# access_token: "syt_your_token_here" # Usage thresholds budget: diff --git a/koan/app/attention.py b/koan/app/attention.py index c887c5fe6..43349bd4e 100644 --- a/koan/app/attention.py +++ b/koan/app/attention.py @@ -13,6 +13,7 @@ Dismissed items are tracked in instance/.koan-attention-dismissed.json. """ +import contextlib import hashlib import json import sys @@ -76,10 +77,8 @@ def save_dismissed(koan_root: str, dismissed: set) -> None: """Atomically persist the set of dismissed item IDs.""" from app.utils import atomic_write_json path = _dismissed_file_path(koan_root) - try: + with contextlib.suppress(OSError): atomic_write_json(path, sorted(dismissed)) - except OSError: - pass def dismiss_item(koan_root: str, item_id: str) -> None: diff --git a/koan/app/awake.py b/koan/app/awake.py index b69301be4..9b0256f5c 100755 --- a/koan/app/awake.py +++ b/koan/app/awake.py @@ -751,8 +751,8 @@ def main(): # right after so the check below finds nothing. if first_poll: # Check if we're coming back from a /restart before clearing - was_restart = check_restart(str(KOAN_ROOT)) - clear_restart(str(KOAN_ROOT)) + was_restart = check_restart(str(KOAN_ROOT), target="bridge") + clear_restart(str(KOAN_ROOT), target="bridge") clear_shutdown(str(KOAN_ROOT)) first_poll = False @@ -775,7 +775,7 @@ def main(): # Check for restart signal (set by /restart command). # Only react to files created AFTER we started — stale files # were already cleared above after the first poll. - if check_restart(str(KOAN_ROOT), since=startup_time): + if check_restart(str(KOAN_ROOT), since=startup_time, target="bridge"): log("init", "Restart signal detected. Re-executing...") release_pidfile(pidfile_lock, KOAN_ROOT, "awake") reexec_bridge() diff --git a/koan/app/bridge_state.py b/koan/app/bridge_state.py index b3423dd9a..4ea8ac840 100644 --- a/koan/app/bridge_state.py +++ b/koan/app/bridge_state.py @@ -5,6 +5,7 @@ Extracted to avoid circular imports between those two modules. """ +import contextlib import os import sys from pathlib import Path @@ -98,17 +99,13 @@ def _skills_dir_mtime() -> float: best = 0.0 # Core skills directory (inside the koan package) core_dir = Path(__file__).resolve().parent.parent / "skills" / "core" - try: + with contextlib.suppress(OSError): best = max(best, core_dir.stat().st_mtime) - except OSError: - pass # Instance skills directory (user-installed skills) instance_skills = INSTANCE_DIR / "skills" if instance_skills.is_dir(): - try: + with contextlib.suppress(OSError): best = max(best, instance_skills.stat().st_mtime) - except OSError: - pass return best diff --git a/koan/app/checkpoint_manager.py b/koan/app/checkpoint_manager.py index f4b00f906..345d03dd7 100644 --- a/koan/app/checkpoint_manager.py +++ b/koan/app/checkpoint_manager.py @@ -244,15 +244,13 @@ def format_recovery_context(checkpoint: Dict) -> str: if steps_done: lines.append("") lines.append("### Steps already completed:") - for step in steps_done: - lines.append(f"- {step}") + lines.extend(f"- {step}" for step in steps_done) steps_remaining = checkpoint.get("steps_remaining", []) if steps_remaining: lines.append("") lines.append("### Steps remaining:") - for step in steps_remaining: - lines.append(f"- {step}") + lines.extend(f"- {step}" for step in steps_remaining) lines.append("") lines.append( diff --git a/koan/app/ci_queue_runner.py b/koan/app/ci_queue_runner.py index bb670a44e..d92eb2561 100644 --- a/koan/app/ci_queue_runner.py +++ b/koan/app/ci_queue_runner.py @@ -17,49 +17,35 @@ All status/debug output goes to stderr; stdout is reserved for JSON. """ +import contextlib import json import sys from pathlib import Path from typing import Optional, Tuple +from app.claude_step import CI_STATUS_BLOCKED_APPROVAL + def check_ci_status(branch: str, full_repo: str) -> Tuple[str, Optional[int]]: """Make a single non-blocking CI status check. + Aggregates all recent workflow runs for the branch, ignoring conclusions + that don't represent real CI signal (e.g. a "Dependabot auto-merge" + run that completes with conclusion="skipped" on non-Dependabot PRs). + Returns: (status, run_id) where status is one of: "success", "failure", "pending", "none" """ - from app.github import run_gh + from app.claude_step import aggregate_ci_runs, fetch_branch_ci_runs try: - raw = run_gh( - "run", "list", - "--branch", branch, - "--repo", full_repo, - "--json", "databaseId,status,conclusion", - "--limit", "1", - ) - runs = json.loads(raw) if raw.strip() else [] + runs = fetch_branch_ci_runs(branch, full_repo) except Exception as e: print(f"[ci_queue] CI status check error: {e}", file=sys.stderr) return ("pending", None) - if not runs: - return ("none", None) - - run = runs[0] - run_id = run.get("databaseId") - status = run.get("status", "").lower() - conclusion = run.get("conclusion", "").lower() - - if status == "completed": - if conclusion == "success": - return ("success", run_id) - return ("failure", run_id) - - # in_progress, queued, waiting, etc. - return ("pending", run_id) + return aggregate_ci_runs(runs) def drain_one(instance_dir: str) -> Optional[str]: @@ -131,7 +117,7 @@ def drain_one(instance_dir: str) -> Optional[str]: ) _write_outbox( instance_dir, - f"❌ CI still failing after {max_attempts} attempts for PR #{pr_number}: {pr_url}", + f"🚦 CI still failing after {max_attempts} attempts for PR #{pr_number}: {pr_url}", ) return f"CI failed {max_attempts} times for PR #{pr_number} — giving up" @@ -142,6 +128,25 @@ def drain_one(instance_dir: str) -> Optional[str]: ) return f"No CI runs found for PR #{pr_number} — removed from ## CI" + if status == CI_STATUS_BLOCKED_APPROVAL: + # GitHub gates workflow runs on first-time-contributor or + # environment approval; nothing Kōan does will unstick them. + # Drop the PR from ## CI so retries stop and notify the human + # so they can approve in the UI (or politely ping the maintainer). + modify_missions_file( + missions_path, + lambda c: remove_ci_item(c, pr_url), + ) + _write_outbox( + instance_dir, + f"⏸ CI workflows on PR #{pr_number} are waiting for maintainer " + f"approval — Kōan stopped retrying: {pr_url}", + ) + return ( + f"CI blocked on maintainer approval for PR #{pr_number} — " + f"removed from ## CI" + ) + # status == "pending" — leave in ## CI return None @@ -204,10 +209,8 @@ def _maybe_migrate_json_queue(instance_dir: str, missions_path: Path): entries = [] if not entries: - try: + with contextlib.suppress(OSError): os.remove(json_path) - except OSError: - pass return from app.missions import add_ci_item @@ -346,6 +349,14 @@ def run_ci_check_and_fix(pr_url: str, project_path: str) -> Tuple[bool, str]: # drain_one will re-check on the next iteration when CI completes. return False, "CI still pending — will retry when CI completes." + if status == CI_STATUS_BLOCKED_APPROVAL: + # Pushing more commits won't trigger CI either — the new runs + # need the same approval. Bail out so the operator can act. + return False, ( + "CI workflows are waiting for maintainer approval — " + "cannot fix without an approve click in the GitHub UI." + ) + if status not in ("failure",): return False, f"CI status is '{status}' — nothing to fix." @@ -524,6 +535,14 @@ def _attempt_ci_fixes( actions_log.append(f"CI running after fix push (attempt {attempt}) — re-enqueued for monitoring") return True + if new_status == CI_STATUS_BLOCKED_APPROVAL: + # New push triggered runs that also need maintainer approval — + # nothing we can do here, bail out instead of re-enqueueing. + actions_log.append( + f"CI waiting for maintainer approval after fix push (attempt {attempt}) — stopping" + ) + return False + # CI already shows failure (unlikely this fast) — get new logs if new_run_id: ci_logs = _fetch_failed_logs(new_run_id, full_repo) diff --git a/koan/app/claude_step.py b/koan/app/claude_step.py index 06942663c..49d9e13b6 100644 --- a/koan/app/claude_step.py +++ b/koan/app/claude_step.py @@ -16,6 +16,14 @@ from pathlib import Path from typing import Callable, List, Optional, Tuple +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 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 + class StepResult: """Result of a :func:`run_claude_step` invocation. @@ -37,12 +45,6 @@ def __bool__(self) -> bool: def __repr__(self) -> str: return f"StepResult(committed={self.committed!r}, output={self.output[:60]!r}...)" -from app.cli_provider import build_full_command, run_command -from app.config import get_model_config -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 # Backward-compatible alias — callers should import from app.cli_provider run_claude_command = run_command @@ -112,6 +114,30 @@ def _is_ancestor(maybe_ancestor: str, descendant: str, cwd: str) -> bool: return False +def _prefetch_all_remotes( + base: str, + project_path: str, + preferred_remote: Optional[str] = None, + head_remote: Optional[str] = None, +) -> None: + """Eagerly fetch the base branch from all relevant remotes. + + Ensures every remote tracking ref is current before the rebase loop + starts, so that ancestry checks and --onto calculations use fresh data. + Failures are logged but never prevent the rebase attempt. + """ + remotes_to_fetch: List[str] = list(_ordered_remotes(preferred_remote)) + if head_remote and head_remote not in remotes_to_fetch: + remotes_to_fetch.append(head_remote) + for remote in remotes_to_fetch: + try: + _fetch_branch(remote, base, cwd=project_path) + except _REBASE_EXCEPTIONS as e: + print(f"[claude_step] Pre-fetch {remote}/{base} failed (non-fatal): {e}", + file=sys.stderr) + + + def _rebase_onto_target( base: str, project_path: str, @@ -126,6 +152,9 @@ def _rebase_onto_target( ``upstream`` fallbacks. When *head_remote* is known and differs from the target remote, uses ``--onto`` to replay only the PR's commits. + All relevant remotes are pre-fetched before the rebase loop so that + tracking refs are guaranteed fresh for ancestry checks and --onto. + Args: on_conflict: Optional callback invoked when a rebase fails and a rebase-in-progress is detected (i.e. conflicts exist). @@ -137,23 +166,9 @@ def _rebase_onto_target( Returns: Remote name used (e.g. "origin" or "upstream") on success, None on failure. """ - for remote in _ordered_remotes(preferred_remote): - try: - _fetch_branch(remote, base, cwd=project_path) - except _REBASE_EXCEPTIONS as e: - print(f"[claude_step] Fetch {remote}/{base} failed: {e}", file=sys.stderr) - continue - - # When head_remote differs from target, use --onto to limit - # replay to only the PR's commits. - if head_remote and head_remote != remote: - try: - _fetch_branch(head_remote, base, cwd=project_path) - except _REBASE_EXCEPTIONS as e: - print(f"[claude_step] Fetch {head_remote}/{base} failed: {e}", file=sys.stderr) - # Can't determine fork state — fall through to plain rebase - head_remote = None + _prefetch_all_remotes(base, project_path, preferred_remote, head_remote) + for remote in _ordered_remotes(preferred_remote): if head_remote and head_remote != remote: # Only use --onto when the fork has genuinely diverged from # upstream (i.e. has commits that upstream doesn't). When the @@ -211,60 +226,102 @@ def strip_cli_noise(text: str) -> str: def run_claude(cmd: list, cwd: str, timeout: int = 600) -> dict: - """Run a Claude Code CLI command. + """Run a Claude Code CLI command, streaming stdout in real time. + + Thin wrapper around :func:`app.cli_exec.stream_with_timeout`. Each + Claude stdout line is forwarded to ``sys.stdout`` while also being + captured. Streaming serves two purposes: + + 1. Each emitted line resets the parent process's liveness watchdog + in ``run.py`` (default 600s), so long but still-progressing + Claude calls no longer get killed for "no output". + 2. ``/live`` and the bridge see Claude's progress in real time + instead of a silent wait. + + The subprocess is started with a new POSIX session + (``start_new_session=True``) so that on timeout the entire process + group can be killed — preventing grandchildren (e.g. tool-call + subprocesses) from holding the stdout pipe open and turning a + ``TimeoutExpired`` into an indefinite hang during pipe drain. Returns: Dict with keys: success (bool), output (str), error (str). """ - from app.cli_exec import run_cli_with_retry - from app.security_audit import SUBPROCESS_EXEC, _redact_list, log_event try: - result = run_cli_with_retry( + proc, cleanup = popen_cli( cmd, - capture_output=True, text=True, - timeout=timeout, cwd=cwd, + stdout=subprocess.PIPE, + stderr=subprocess.PIPE, + text=True, + cwd=cwd, + start_new_session=True, ) - if result.returncode != 0: - stderr_snippet = result.stderr[-500:] if result.stderr else "no stderr" - # When stderr is empty, stdout often contains the actual error - # (e.g. "Error: context window exceeded"). Include it so callers - # get actionable diagnostics instead of just "no stderr". - stdout_text = result.stdout.strip() - if not result.stderr and stdout_text: - stderr_snippet = f"no stderr | stdout: {stdout_text[-500:]}" - log_event(SUBPROCESS_EXEC, details={ - "cmd": _redact_list(cmd), - "cwd": cwd, - "exit_code": result.returncode, - }, result="failure") - return { - "success": False, - "output": stdout_text, - "error": f"Exit code {result.returncode}: {stderr_snippet}", - } + except Exception as e: log_event(SUBPROCESS_EXEC, details={ "cmd": _redact_list(cmd), "cwd": cwd, - "exit_code": 0, - }) + }, result="failure") return { - "success": True, - "output": result.stdout.strip(), - "error": "", + "success": False, + "output": "", + "error": f"Failed to spawn CLI: {e}", } - except subprocess.TimeoutExpired: + + try: + stream_result = stream_with_timeout( + proc, + timeout=timeout, + on_line=lambda line: print(line, flush=True), + ) + finally: + cleanup() + + stdout_text = stream_result.stdout + stderr_text = stream_result.stderr + + if stream_result.timed_out: log_event(SUBPROCESS_EXEC, details={ "cmd": _redact_list(cmd), "cwd": cwd, }, result="timeout") return { "success": False, - "output": "", + "output": stdout_text, "error": f"Timeout ({timeout}s)", } + returncode = proc.returncode + if returncode != 0: + stderr_snippet = stderr_text[-500:] if stderr_text else "no stderr" + # When stderr is empty, stdout often contains the actual error + # (e.g. "Error: context window exceeded"). Include it so callers + # get actionable diagnostics instead of just "no stderr". + if not stderr_text and stdout_text: + stderr_snippet = f"no stderr | stdout: {stdout_text[-500:]}" + log_event(SUBPROCESS_EXEC, details={ + "cmd": _redact_list(cmd), + "cwd": cwd, + "exit_code": returncode, + }, result="failure") + return { + "success": False, + "output": stdout_text, + "error": f"Exit code {returncode}: {stderr_snippet}", + } + + log_event(SUBPROCESS_EXEC, details={ + "cmd": _redact_list(cmd), + "cwd": cwd, + "exit_code": 0, + }) + return { + "success": True, + "output": stdout_text, + "error": "", + } + def commit_if_changes(project_path: str, message: str) -> bool: """Stage all changes and commit if there are any. @@ -435,6 +492,105 @@ def _safe_checkout(branch: str, project_path: str) -> None: print(f"[claude_step] Safe checkout failed for {branch}: {e}", file=sys.stderr) +# Conclusions that don't signal a real CI outcome. The classic case is +# "Dependabot auto-merge", which runs on every PR but only acts on +# Dependabot-authored PRs — on every other PR it completes with +# conclusion="skipped". Treating that as a CI failure sends Kōan into a +# fix loop against a workflow that isn't actually broken. +_IGNORED_CI_CONCLUSIONS = frozenset( + {"skipped", "cancelled", "neutral", "action_required"} +) + +# Workflow run statuses that mean "blocked, awaiting manual action". +# GitHub sets `status="action_required"` on fork PRs from first-time +# contributors until a maintainer approves the run, and `status="waiting"` +# when a job is gated on environment approval. In both cases, polling +# forever — or, worse, pushing new commits to "fix" CI — never unsticks +# the run. Kōan must treat these as terminal so the PR drops out of the +# ## CI queue with a human-readable note. +_APPROVAL_BLOCKED_STATUSES = frozenset({"action_required", "waiting"}) + +# Canonical CI status string returned by aggregate_ci_runs() and +# wait_for_ci() when a workflow run is blocked on maintainer or +# environment approval. Use the constant instead of the raw string +# to avoid typos across modules. +CI_STATUS_BLOCKED_APPROVAL = "blocked_approval" + +# Upper bound on runs fetched per branch — enough to cover all workflows +# triggered by a single push (typically <10), small enough to keep the +# `gh run list` call cheap. +_CI_RUN_LIMIT = 20 + + +def aggregate_ci_runs(runs: list) -> Tuple[str, Optional[int]]: + """Reduce a list of workflow runs to a single (status, run_id) tuple. + + Filters out runs whose conclusion is in :data:`_IGNORED_CI_CONCLUSIONS` + (notably the "Dependabot auto-merge" skip case) before aggregating, so + a benign skipped workflow doesn't masquerade as a CI failure. + + Aggregation rules over the remaining runs: + - any failed completed run → ("failure", failed_run_id) + - else any run blocked on maintainer/environment approval → + ("blocked_approval", blocked_run_id) — Kōan can't unstick it, so + callers should stop retrying and surface a notification. + - else any non-completed run → ("pending", pending_run_id) + - else all completed + success → ("success", first_run_id) + - empty input or every run filtered out → ("none", None) + + Failure takes precedence over blocked_approval so a genuinely broken + workflow on the same push still gets surfaced for a fix attempt. + """ + if not runs: + return ("none", None) + + relevant = [ + r for r in runs + if (r.get("conclusion") or "").lower() not in _IGNORED_CI_CONCLUSIONS + ] + if not relevant: + return ("none", None) + + failed_run = None + blocked_run = None + pending_run = None + for run in relevant: + status = (run.get("status") or "").lower() + conclusion = (run.get("conclusion") or "").lower() + if status == "completed": + if conclusion != "success" and failed_run is None: + failed_run = run + elif status in _APPROVAL_BLOCKED_STATUSES: + if blocked_run is None: + blocked_run = run + elif pending_run is None: + pending_run = run + + if failed_run is not None: + return ("failure", failed_run.get("databaseId")) + if blocked_run is not None: + return (CI_STATUS_BLOCKED_APPROVAL, blocked_run.get("databaseId")) + if pending_run is not None: + return ("pending", pending_run.get("databaseId")) + return ("success", relevant[0].get("databaseId")) + + +def fetch_branch_ci_runs(branch: str, full_repo: str) -> list: + """Return raw `gh run list` entries for a branch. + + Raises on `gh` failure so callers can decide between fall-back + behaviours (e.g. "treat as pending" vs "treat as none"). + """ + raw = run_gh( + "run", "list", + "--branch", branch, + "--repo", full_repo, + "--json", "databaseId,status,conclusion,name,workflowName", + "--limit", str(_CI_RUN_LIMIT), + ) + return json.loads(raw) if raw.strip() else [] + + def wait_for_ci( branch: str, full_repo: str, @@ -452,7 +608,7 @@ def wait_for_ci( Returns: (status, run_id, logs) where: - - status: "success", "failure", "timeout", or "none" + - status: "success", "failure", "blocked_approval", "timeout", or "none" - run_id: GitHub Actions run ID (None if no runs found) - logs: Failed job logs (empty unless status is "failure") """ @@ -463,37 +619,34 @@ def wait_for_ci( while time.time() < deadline: try: - raw = run_gh( - "run", "list", - "--branch", branch, - "--repo", full_repo, - "--json", "databaseId,status,conclusion", - "--limit", "1", - ) - runs = json.loads(raw) if raw.strip() else [] + runs = fetch_branch_ci_runs(branch, full_repo) except Exception as e: print(f"[claude_step] CI poll error: {e}", file=sys.stderr) time.sleep(poll_interval) continue - if not runs: - # No CI runs found for this branch — common for repos without CI - return ("none", None, "") + status, run_id = aggregate_ci_runs(runs) - run = runs[0] - run_id = run.get("databaseId") - status = run.get("status", "").lower() - conclusion = run.get("conclusion", "").lower() + if status == "none": + # No CI signal — either no runs, or every run was filtered as + # non-CI (e.g. a Dependabot auto-merge skip with nothing else + # registered yet). Mirror the original "no runs" exit. + return ("none", None, "") - if status == "completed": - if conclusion == "success": - return ("success", run_id, "") + if status == "success": + return ("success", run_id, "") - # CI failed — fetch logs for failed jobs - logs = _fetch_failed_logs(run_id, full_repo) + if status == "failure": + logs = _fetch_failed_logs(run_id, full_repo) if run_id else "" return ("failure", run_id, logs) - # Still running — wait and poll again + if status == CI_STATUS_BLOCKED_APPROVAL: + # A maintainer (or environment reviewer) must click Approve in + # the GitHub UI; polling won't change that. Exit so the caller + # can surface a notification instead of burning quota. + return (CI_STATUS_BLOCKED_APPROVAL, run_id, "") + + # status == "pending" — keep polling time.sleep(poll_interval) return ("timeout", None, "") @@ -539,39 +692,23 @@ def check_existing_ci( Returns: (status, run_id, logs) where: - - status: "success", "failure", "pending", or "none" + - status: "success", "failure", "pending", "blocked_approval", or "none" - run_id: GitHub Actions run ID (None if no runs found) - logs: Failed job logs (empty unless status is "failure") """ try: - raw = run_gh( - "run", "list", - "--branch", branch, - "--repo", full_repo, - "--json", "databaseId,status,conclusion", - "--limit", "1", - ) - runs = json.loads(raw) if raw.strip() else [] + runs = fetch_branch_ci_runs(branch, full_repo) except Exception as e: print(f"[claude_step] CI check error: {e}", file=sys.stderr) return ("none", None, "") - if not runs: - return ("none", None, "") + status, run_id = aggregate_ci_runs(runs) - run = runs[0] - run_id = run.get("databaseId") - status = run.get("status", "").lower() - conclusion = run.get("conclusion", "").lower() - - if status == "completed": - if conclusion == "success": - return ("success", run_id, "") - logs = _fetch_failed_logs(run_id, full_repo) + if status == "failure": + logs = _fetch_failed_logs(run_id, full_repo) if run_id else "" return ("failure", run_id, logs) - # Still running or queued - return ("pending", run_id, "") + return (status, run_id, "") def _is_permission_error(error_msg: str) -> bool: diff --git a/koan/app/cli_exec.py b/koan/app/cli_exec.py index ad60238a3..aa277ba52 100644 --- a/koan/app/cli_exec.py +++ b/koan/app/cli_exec.py @@ -10,10 +10,13 @@ directly as a ``-p`` argument. """ +import contextlib import os +import signal import subprocess import sys import tempfile +import threading import time from typing import Callable, List, Optional, Sequence, Tuple @@ -78,10 +81,8 @@ def prepare_prompt_file(cmd: List[str]) -> Tuple[List[str], Optional[str]]: def _cleanup_prompt_file(path: Optional[str]) -> None: """Silently remove a temp prompt file if it exists.""" if path: - try: + with contextlib.suppress(OSError): os.unlink(path) - except OSError: - pass def run_cli(cmd, **kwargs) -> subprocess.CompletedProcess: @@ -136,6 +137,110 @@ def cleanup(): return subprocess.Popen(cmd, **kwargs), lambda: None +class StreamResult: + """Result of :func:`stream_with_timeout`.""" + + __slots__ = ("stdout", "stderr", "timed_out") + + def __init__(self, stdout: str, stderr: str, timed_out: bool): + self.stdout = stdout + self.stderr = stderr + self.timed_out = timed_out + + +def stream_with_timeout( + proc: subprocess.Popen, + timeout: float, + on_line: Optional[Callable[[str], None]] = None, + drain_timeout: float = 30.0, +) -> StreamResult: + """Consume ``proc.stdout`` line-by-line with a process-group-kill watchdog. + + Each stdout line is collected into the returned ``stdout`` text and + optionally forwarded to *on_line*. After stdout EOF, the stderr + stream is drained and the subprocess is awaited. + + On timeout the entire process group is SIGKILL'd via + :func:`os.killpg` — the caller must have started *proc* with + ``start_new_session=True`` (or ``process_group=0`` on 3.11+) so the + group exists. Killing the group ensures grandchildren that inherited + the stdout pipe are torn down too, preventing pipe-drain hangs. + + A ``completed`` flag guarded by a lock closes the race where the + watchdog Timer fires between the last consumed line and + ``Timer.cancel()`` — in that window we still want a clean completion + rather than a spurious timeout. + + Both std streams are closed before returning. + """ + stdout_lines: List[str] = [] + stderr_text = "" + timed_out = False + completed = False + state_lock = threading.Lock() + + def _kill_process_group() -> None: + try: + pgid = os.getpgid(proc.pid) + os.killpg(pgid, signal.SIGKILL) + except (OSError, ProcessLookupError): + with contextlib.suppress(OSError, ProcessLookupError): + proc.kill() + + def _watchdog_fire() -> None: + # Race guard: if the stream loop has already finished and is + # about to call watchdog.cancel(), don't flip ``timed_out`` and + # don't kill — the process is exiting cleanly. + nonlocal timed_out + with state_lock: + if completed: + return + timed_out = True + _kill_process_group() + + watchdog = threading.Timer(timeout, _watchdog_fire) + watchdog.daemon = True + watchdog.start() + + try: + try: + for line in proc.stdout: + stripped = line.rstrip("\n") + stdout_lines.append(stripped) + if on_line is not None: + on_line(stripped) + finally: + with state_lock: + if not timed_out: + completed = True + watchdog.cancel() + + with contextlib.suppress(OSError, ValueError): + if proc.stderr: + stderr_text = proc.stderr.read() + + try: + proc.wait(timeout=drain_timeout) + except subprocess.TimeoutExpired: + # Stdout EOF reached but the process refuses to exit — + # force-kill and report a timeout so callers see the hang. + timed_out = True + _kill_process_group() + with contextlib.suppress(subprocess.TimeoutExpired): + proc.wait(timeout=5) + finally: + for stream in (proc.stdout, proc.stderr): + if stream is not None: + with contextlib.suppress(OSError): + stream.close() + + return StreamResult( + stdout="\n".join(stdout_lines).strip(), + stderr=stderr_text, + timed_out=timed_out, + ) + + # Default backoff durations for CLI retries (seconds). # Higher than retry.py's (1/2/4s) because CLI calls are heavier. CLI_RETRY_BACKOFF = (2, 5, 10) diff --git a/koan/app/cli_journal_streamer.py b/koan/app/cli_journal_streamer.py index 349eee24b..788ede295 100644 --- a/koan/app/cli_journal_streamer.py +++ b/koan/app/cli_journal_streamer.py @@ -12,6 +12,7 @@ stop_journal_stream(stream, exit_code, stderr_file) """ +import contextlib import os import sys import threading @@ -90,10 +91,9 @@ def _tail_loop( chunk = leftover + raw text, leftover = _decode_safe(chunk) if text: - try: + # non-critical; avoid log spam in tight loop + with contextlib.suppress(OSError): append(instance_dir, project_name, text) - except OSError: - pass # non-critical; avoid log spam in tight loop except OSError: pass # file may not exist yet diff --git a/koan/app/command_handlers.py b/koan/app/command_handlers.py index 9349f0c18..adac48353 100644 --- a/koan/app/command_handlers.py +++ b/koan/app/command_handlers.py @@ -7,6 +7,7 @@ to avoid circular imports with awake.py. """ +import contextlib import time from typing import Callable, Optional @@ -712,10 +713,8 @@ def _write_skip_start_pause(): /resume removes the pause but startup re-creates it. """ from app.signals import SKIP_START_PAUSE_FILE - try: + with contextlib.suppress(OSError): (KOAN_ROOT / SKIP_START_PAUSE_FILE).write_text(str(int(time.time()))) - except OSError: - pass def handle_resume(): @@ -794,10 +793,8 @@ def handle_resume(): reset_info = lines[0] if lines else "unknown time" paused_at = 0 if len(lines) > 1 and lines[1].strip(): - try: + with contextlib.suppress(ValueError): paused_at = int(lines[1].strip()) - except ValueError: - pass hours_since_pause = (time.time() - paused_at) / 3600 likely_reset = hours_since_pause >= 2 diff --git a/koan/app/conversation_history.py b/koan/app/conversation_history.py index 17adc481c..40947ab35 100644 --- a/koan/app/conversation_history.py +++ b/koan/app/conversation_history.py @@ -5,6 +5,7 @@ any messaging provider. """ +import contextlib import fcntl import json from datetime import datetime @@ -152,10 +153,8 @@ def prune_topics(entries: list, max_entries: int = 20) -> list: return entries # Sort by compacted_at to ensure we keep the most recent - try: + with contextlib.suppress(TypeError, AttributeError): entries.sort(key=lambda e: e.get("compacted_at", "")) - except (TypeError, AttributeError): - pass return entries[-max_entries:] @@ -211,10 +210,8 @@ def compact_history(history_file: Path, topics_file: Path, min_messages: int = 2 if not topics_by_date: # No extractable topics, just purge atomically - try: + with contextlib.suppress(OSError): _atomic_write(history_file, "") - except OSError: - pass return len(messages) # Build compaction entry diff --git a/koan/app/daily_report.py b/koan/app/daily_report.py index 9845af2eb..0a7a65250 100644 --- a/koan/app/daily_report.py +++ b/koan/app/daily_report.py @@ -162,8 +162,7 @@ def generate_report(report_type: str = "morning") -> str: # Completed missions if completed: lines.append("Completed missions:") - for m in completed[-5:]: # Last 5 max - lines.append(f" . {m}") + lines.extend(f" . {m}" for m in completed[-5:]) lines.append("") # Pending missions @@ -185,8 +184,7 @@ def generate_report(report_type: str = "morning") -> str: if activities: lines.append("Activity:") - for a in activities[-6:]: # Last 6 max - lines.append(f" . {a}") + lines.extend(f" . {a}" for a in activities[-6:]) lines.append("") else: lines.append("No activity recorded.") @@ -212,8 +210,7 @@ def generate_report(report_type: str = "morning") -> str: if in_progress: lines.append("In Progress:") - for ip in in_progress: - lines.append(f" . {ip}") + lines.extend(f" . {ip}" for ip in in_progress) lines.append("") lines.append("-- Kōan") diff --git a/koan/app/dashboard.py b/koan/app/dashboard.py index f795e1cdd..4df4854fb 100644 --- a/koan/app/dashboard.py +++ b/koan/app/dashboard.py @@ -15,6 +15,7 @@ make dashboard """ +import contextlib import json import os import re @@ -210,10 +211,8 @@ def get_agent_state() -> dict: project_file = KOAN_ROOT / PROJECT_FILE project = "" if project_file.exists(): - try: + with contextlib.suppress(OSError): project = project_file.read_text().strip() - except OSError: - pass # Read focus state focus = None @@ -356,11 +355,10 @@ def get_journal_entries(limit: int = 7) -> list: # Check nested structure nested = JOURNAL_DIR / d if nested.is_dir(): - for f in sorted(nested.glob("*.md")): - day_entries.append({ - "project": f.stem, - "content": f.read_text(), - }) + day_entries.extend( + {"project": f.stem, "content": f.read_text()} + for f in sorted(nested.glob("*.md")) + ) # Check flat structure flat = JOURNAL_DIR / f"{d}.md" if flat.is_file(): @@ -1332,9 +1330,11 @@ def api_agent_memory(): global_files = [] global_dir = memory_dir / "global" if global_dir.is_dir(): - for f in sorted(global_dir.iterdir()): - if f.is_file() and f.suffix in (".md", ".txt"): - global_files.append({**_read_capped(f), "name": f.name}) + global_files.extend( + {**_read_capped(f), "name": f.name} + for f in sorted(global_dir.iterdir()) + if f.is_file() and f.suffix in (".md", ".txt") + ) # Per-project files under memory/projects/{name}/ projects: dict = {} @@ -1343,10 +1343,11 @@ def api_agent_memory(): for proj_dir in sorted(projects_dir.iterdir()): if not proj_dir.is_dir(): continue - files = [] - for f in sorted(proj_dir.iterdir()): - if f.is_file() and f.suffix in (".md", ".txt"): - files.append({**_read_capped(f), "name": f.name}) + files = [ + {**_read_capped(f), "name": f.name} + for f in sorted(proj_dir.iterdir()) + if f.is_file() and f.suffix in (".md", ".txt") + ] if files: projects[proj_dir.name] = files @@ -1371,13 +1372,14 @@ def api_agent_skills(): skills_list = [] for skill in registry.list_all(): - commands = [] - for cmd in skill.commands: - commands.append({ + commands = [ + { "name": cmd.name, "aliases": list(cmd.aliases) if cmd.aliases else [], "description": cmd.description or "", - }) + } + for cmd in skill.commands + ] skills_list.append({ "name": skill.name, "scope": skill.scope, diff --git a/koan/app/deep_research.py b/koan/app/deep_research.py index 906cf98f0..11fa22bcf 100644 --- a/koan/app/deep_research.py +++ b/koan/app/deep_research.py @@ -242,8 +242,10 @@ def get_recent_journal_topics(self, days: int = 7) -> list[str]: if journal_file.exists(): content = journal_file.read_text() # Extract session headers (## Session N, ## Run N, etc.) - for match in re.finditer(r"^##\s*(.+?)$", content, re.MULTILINE): - topics.append(match.group(1).strip()) + topics.extend( + match.group(1).strip() + for match in re.finditer(r"^##\s*(.+?)$", content, re.MULTILINE) + ) return topics @@ -303,13 +305,15 @@ def suggest_topics(self) -> list[dict]: recent_topics = self.get_recent_journal_topics() # Priority 1: Current focus items from priorities.md - for item in priorities.get("current_focus", []): - suggestions.append({ + suggestions.extend( + { "topic": item, "source": "priorities.md (Current Focus)", "reasoning": "Explicitly marked as current priority by human", "priority": 1, - }) + } + for item in priorities.get("current_focus", []) + ) # Priority 2: Open GitHub issues (if any) for issue in issues[:5]: # Top 5 issues @@ -348,13 +352,15 @@ def suggest_topics(self) -> list[dict]: }) # Priority 3: Strategic goals (bigger picture) - for item in priorities.get("strategic_goals", []): - suggestions.append({ + suggestions.extend( + { "topic": item, "source": "priorities.md (Strategic Goals)", "reasoning": "Contributes to larger project direction", "priority": 3, - }) + } + for item in priorities.get("strategic_goals", []) + ) # Filter out topics already covered by open PRs coverage = self._build_pr_coverage() @@ -472,8 +478,7 @@ def format_for_agent(self) -> str: if do_not_touch: lines.append("### Avoid These Areas") lines.append("") - for item in do_not_touch: - lines.append(f"- {item}") + lines.extend(f"- {item}" for item in do_not_touch) lines.append("") lines.append("---") diff --git a/koan/app/estop_manager.py b/koan/app/estop_manager.py index ded71fb43..665869d93 100644 --- a/koan/app/estop_manager.py +++ b/koan/app/estop_manager.py @@ -25,6 +25,7 @@ PROJECT_FREEZE — block specific projects while others continue """ +import contextlib import json import os import time @@ -181,10 +182,8 @@ def deactivate_estop(koan_root: str) -> None: """ for name in (ESTOP_STATE_FILE, ESTOP_SIGNAL_FILE): path = os.path.join(koan_root, name) - try: + with contextlib.suppress(FileNotFoundError): os.remove(path) - except FileNotFoundError: - pass def unfreeze_project(koan_root: str, project_name: str) -> Optional[EstopState]: diff --git a/koan/app/git_prep.py b/koan/app/git_prep.py index 737b2ca59..7bc72d80a 100644 --- a/koan/app/git_prep.py +++ b/koan/app/git_prep.py @@ -24,6 +24,40 @@ logger = logging.getLogger(__name__) +def _fetch_branch_refspec( + remote: str, branch: str, project_path: str, timeout: int = 15 +) -> bool: + """Fetch a branch using an explicit refspec to guarantee tracking ref update. + + Returns True on success. + """ + refspec = f"+refs/heads/{branch}:refs/remotes/{remote}/{branch}" + rc, _, _ = run_git("fetch", remote, refspec, cwd=project_path, timeout=timeout) + return rc == 0 + + +def _sync_secondary_remotes( + base_branch: str, primary_remote: str, project_path: str +) -> None: + """Fetch base branch from all remotes besides the primary. + + Ensures remote tracking refs are fresh for fork-aware operations + (e.g., --onto rebase needs both origin/ and upstream/ refs current). + Non-fatal — failures are logged but never abort the mission. + """ + rc, stdout, _ = run_git("remote", cwd=project_path) + if rc != 0 or not stdout: + return + for remote in stdout.splitlines(): + remote = remote.strip() + if not remote or remote == primary_remote: + continue + if not _fetch_branch_refspec(remote, base_branch, project_path): + logger.debug( + "Secondary fetch %s/%s failed (non-fatal)", remote, base_branch + ) + + def detect_remote_default_branch(remote: str, project_path: str) -> str: """Detect the default branch for a remote. @@ -216,4 +250,8 @@ def prepare_project_branch( result.error = f"reset failed: {stderr}" return result + # Sync secondary remotes so fork-aware operations (--onto rebase, + # _is_ancestor checks) see fresh tracking refs for every remote. + _sync_secondary_remotes(base_branch, remote, project_path) + return result diff --git a/koan/app/git_sync.py b/koan/app/git_sync.py index 91d4c6eb4..e2def45c9 100644 --- a/koan/app/git_sync.py +++ b/koan/app/git_sync.py @@ -113,10 +113,10 @@ def get_recent_main_commits(self, since_hours: int = 12) -> List[str]: def _get_target_branches(self) -> List[str]: """Return remote target branches that exist in this repo.""" candidates = ["origin/main", "origin/master", "origin/staging", "origin/develop", "origin/production"] - existing = [] - for ref in candidates: - if run_git(self.project_path, "rev-parse", "--verify", ref): - existing.append(ref) + existing = [ + ref for ref in candidates + if run_git(self.project_path, "rev-parse", "--verify", ref) + ] return existing or ["origin/main"] def get_merged_branches(self) -> List[str]: @@ -400,8 +400,7 @@ def build_sync_report(self) -> str: if unmerged: recent_branches, stale_branches = self._split_branches_by_recency(unmerged) parts.append(f"\nUnmerged {label} branches ({len(unmerged)}):") - for b in recent_branches: - parts.append(f" → {b}") + parts.extend(f" → {b}" for b in recent_branches) if stale_branches: parts.append( f" ... and {len(stale_branches)} older branch(es) " @@ -410,8 +409,7 @@ def build_sync_report(self) -> str: if recent: parts.append(f"\nRecent main commits ({len(recent)}):") - for c in recent[:10]: - parts.append(f" {c}") + parts.extend(f" {c}" for c in recent[:10]) if not all_merged and not unmerged and not recent: parts.append("\nNo notable changes since last sync.") diff --git a/koan/app/github.py b/koan/app/github.py index 91354b8ff..55fe4299a 100644 --- a/koan/app/github.py +++ b/koan/app/github.py @@ -586,6 +586,140 @@ def find_bot_comment( return None +def check_pvrs_enabled(repo: str, cwd: str = None) -> bool: + """Check if Private Vulnerability Reporting is enabled on a repository. + + Calls ``GET /repos/{owner}/{repo}/private-vulnerability-reporting``. + Returns ``False`` on any error (safe default — falls back to public issues). + + Args: + repo: Repository in ``owner/repo`` format. + cwd: Optional working directory. + + Returns: + True if PVRS is enabled, False otherwise. + """ + try: + output = api( + f"repos/{repo}/private-vulnerability-reporting", + cwd=cwd, timeout=15, + ) + data = json.loads(output) + return data.get("enabled", False) is True + except (RuntimeError, subprocess.TimeoutExpired, json.JSONDecodeError, + OSError, TypeError, KeyError): + return False + + +def security_advisory_report( + summary: str, + description: str, + severity: str, + ecosystem: str = "other", + package_name: str = "", + repo: str = None, + cwd: str = None, +) -> str: + """Submit a private vulnerability report via GitHub PVRS. + + Calls ``POST /repos/{owner}/{repo}/security-advisories/reports``. + + Args: + summary: Advisory title. + description: Markdown body with vulnerability details. + severity: One of ``critical``, ``high``, ``medium``, ``low``. + ecosystem: Package ecosystem (``pip``, ``npm``, ``go``, etc.). + package_name: Package or project name. + repo: Repository in ``owner/repo`` format. + cwd: Optional working directory. + + Returns: + The advisory URL (``html_url``) on success. + + Raises: + RuntimeError: If the API call fails. + """ + from app.leak_detector import scan_and_redact + + summary = scan_and_redact(summary, context="PVRS summary") + description = scan_and_redact(description, context="PVRS description") + + payload = json.dumps({ + "summary": summary, + "description": description, + "severity": severity, + "vulnerabilities": [{ + "package": { + "ecosystem": ecosystem, + "name": package_name or "unknown", + }, + "vulnerable_version_range": "*", + "patched_versions": "*", + }], + }) + + output = api( + f"repos/{repo}/security-advisories/reports", + method="POST", + input_data=payload, + cwd=cwd, + timeout=30, + ) + + try: + data = json.loads(output) + url = data.get("html_url", "") + if url: + return url + ghsa = data.get("ghsa_id", "") + if ghsa: + return f"GHSA: {ghsa}" + except (json.JSONDecodeError, TypeError): + pass + + return output.strip() if output else "" + + +def detect_ecosystem(project_path: str) -> str: + """Infer the package ecosystem from project files. + + Checks for common package manager files and returns the corresponding + ecosystem identifier used by GitHub's advisory API. + + Args: + project_path: Path to the project root. + + Returns: + Ecosystem string: ``pip``, ``npm``, ``go``, ``cargo``, ``maven``, + ``nuget``, ``rubygems``, ``composer``, or ``other``. + """ + from pathlib import Path + + root = Path(project_path) + + # Order matters: more specific files first + indicators = [ + (("pyproject.toml", "requirements.txt", "setup.py", "Pipfile"), "pip"), + (("package.json",), "npm"), + (("go.mod",), "go"), + (("Cargo.toml",), "cargo"), + (("pom.xml", "build.gradle", "build.gradle.kts"), "maven"), + (("*.csproj", "*.sln"), "nuget"), + (("Gemfile",), "rubygems"), + (("composer.json",), "composer"), + ] + + for filenames, ecosystem in indicators: + for filename in filenames: + if "*" in filename: + if list(root.glob(filename)): + return ecosystem + elif (root / filename).exists(): + return ecosystem + + return "other" + + def count_open_prs(repo: str, author: str, cwd: str = None) -> int: """Count open pull requests by a specific author in a repository. diff --git a/koan/app/github_reply.py b/koan/app/github_reply.py index 6e62a6fb7..16ce60498 100644 --- a/koan/app/github_reply.py +++ b/koan/app/github_reply.py @@ -127,12 +127,11 @@ def fetch_thread_context( ) files = json.loads(raw) if raw else [] if isinstance(files, list): - lines = [] - for f in files[:30]: # Cap at 30 files - lines.append( - f" {f.get('status', '?')} {f.get('filename', '?')} " - f"(+{f.get('additions', 0)}/-{f.get('deletions', 0)})" - ) + lines = [ + f" {f.get('status', '?')} {f.get('filename', '?')} " + f"(+{f.get('additions', 0)}/-{f.get('deletions', 0)})" + for f in files[:30] + ] context["diff_summary"] = "\n".join(lines) except (RuntimeError, json.JSONDecodeError): pass @@ -170,9 +169,7 @@ def build_reply_prompt( # Format comments for context comments_text = "" if comments: - comment_lines = [] - for c in comments: - comment_lines.append(f"@{c['author']}: {c['body']}") + comment_lines = [f"@{c['author']}: {c['body']}" for c in comments] comments_text = "\n\n".join(comment_lines) return load_prompt( diff --git a/koan/app/github_skill_helpers.py b/koan/app/github_skill_helpers.py index 65baf4097..8a9dfd716 100644 --- a/koan/app/github_skill_helpers.py +++ b/koan/app/github_skill_helpers.py @@ -90,7 +90,7 @@ def resolve_project_for_repo(repo: str, owner: Optional[str] = None) -> Tuple[Op def queue_github_mission( ctx, command: str, url: str, project_name: str, context: Optional[str] = None, *, urgent: bool = False, -) -> None: +) -> bool: """Queue a GitHub-related mission with consistent formatting. Args: @@ -100,6 +100,9 @@ def queue_github_mission( project_name: Project name for tagging context: Optional additional context to append urgent: If True, insert at the top of the queue (--now flag) + + Returns: + True if the mission was queued, False if it was a duplicate. """ from app.utils import insert_pending_mission @@ -109,7 +112,29 @@ def queue_github_mission( mission_entry = f"- [project:{project_name}] {mission_text}" missions_path = ctx.instance_dir / "missions.md" - insert_pending_mission(missions_path, mission_entry, urgent=urgent) + return insert_pending_mission(missions_path, mission_entry, urgent=urgent) + + +def queue_github_mission_once( + ctx, command: str, url: str, project_name: str, + context: Optional[str] = None, *, urgent: bool = False, + type_label: str = "PR", number: int = 0, + owner: str = "", repo: str = "", +) -> Optional[str]: + """Queue a GitHub mission, returning a duplicate warning if skipped. + + Combines queue_github_mission + standard duplicate message into one call. + + Returns: + A ⚠️ duplicate warning string if skipped, None if successfully queued. + """ + inserted = queue_github_mission(ctx, command, url, project_name, context, urgent=urgent) + if not inserted: + return ( + f"\u26a0\ufe0f Duplicate ignored — /{command} already queued or running " + f"for {type_label} #{number} ({owner}/{repo})." + ) + return None def format_project_not_found_error(repo: str, owner: Optional[str] = None) -> str: @@ -158,7 +183,7 @@ def _find_repo_name_matches(repo: str) -> list: config = load_projects_config(str(KOAN_ROOT)) if not config: return matches - for _name, project in config.get("projects", {}).items(): + for project in config.get("projects", {}).values(): if not isinstance(project, dict): continue gh_url = project.get("github_url", "") @@ -251,8 +276,13 @@ def handle_github_skill( if not project_path: return format_project_not_found_error(repo, owner=owner) - # Queue mission - queue_github_mission(ctx, command, url, project_name, context, urgent=urgent) + # Queue mission (with duplicate detection) + duplicate = queue_github_mission_once( + ctx, command, url, project_name, context, urgent=urgent, + type_label=type_label, number=number, owner=owner, repo=repo, + ) + if duplicate: + return duplicate # Return success message priority = " (priority)" if urgent else "" diff --git a/koan/app/heartbeat.py b/koan/app/heartbeat.py index e32737125..5c59c3706 100644 --- a/koan/app/heartbeat.py +++ b/koan/app/heartbeat.py @@ -8,6 +8,7 @@ All checks are pure Python file operations — no API calls, no subprocess. """ +import contextlib import shutil import time from datetime import datetime @@ -118,10 +119,8 @@ def _get_last_journal_activity(instance_dir: str, project_name: str = None) -> f # Check pending.md (written during active runs) pending = journal_dir / "pending.md" if pending.exists(): - try: + with contextlib.suppress(OSError): mtimes.append(pending.stat().st_mtime) - except OSError: - pass # Check today's journal directory today = datetime.now().strftime("%Y-%m-%d") @@ -137,9 +136,9 @@ def _get_last_journal_activity(instance_dir: str, project_name: str = None) -> f mtimes.append(f.stat().st_mtime) # Always include any file if we didn't find the project-specific one if not mtimes: - for f in today_dir.iterdir(): - if f.is_file(): - mtimes.append(f.stat().st_mtime) + mtimes.extend( + f.stat().st_mtime for f in today_dir.iterdir() if f.is_file() + ) except OSError: pass diff --git a/koan/app/hooks.py b/koan/app/hooks.py index ce23e9f45..bd04a23e5 100644 --- a/koan/app/hooks.py +++ b/koan/app/hooks.py @@ -43,6 +43,7 @@ def run(ctx): A per-rule loop guard prevents runaway rule execution. """ +import contextlib import importlib.util import os import sys @@ -316,10 +317,9 @@ def _action_pause(self, instance_dir: str) -> None: def _action_resume(self, instance_dir: str) -> None: """Remove .koan-pause if it exists.""" pause_file = Path(instance_dir).parent / ".koan-pause" - try: + # Already absent — idempotent + with contextlib.suppress(FileNotFoundError): pause_file.unlink() - except FileNotFoundError: - pass # Already absent — idempotent def _action_auto_merge(self, instance_dir: str, ctx: dict) -> None: """Call git_auto_merge.auto_merge_branch() if project context present.""" diff --git a/koan/app/iteration_manager.py b/koan/app/iteration_manager.py index 1a19e0b4b..700bb79c2 100644 --- a/koan/app/iteration_manager.py +++ b/koan/app/iteration_manager.py @@ -805,7 +805,7 @@ def _filter_exploration_projects( if projects_needing_check: # Phase 2: Batch-fetch PR counts for all repos in one GraphQL call all_repos = [] - for _, (_, _, urls) in projects_needing_check.items(): + for (_, _, urls) in projects_needing_check.values(): all_repos.extend(urls) all_repos = list(dict.fromkeys(all_repos)) # deduplicate, preserve order diff --git a/koan/app/journal.py b/koan/app/journal.py index 85341c942..34ea1a1c7 100644 --- a/koan/app/journal.py +++ b/koan/app/journal.py @@ -70,9 +70,11 @@ def read_all_journals(instance_dir: Path, target_date) -> str: # Check nested per-project files if journal_dir.is_dir(): - for f in sorted(journal_dir.iterdir()): - if f.suffix == ".md": - parts.append(f"[{f.stem}]\n{f.read_text()}") + parts.extend( + f"[{f.stem}]\n{f.read_text()}" + for f in sorted(journal_dir.iterdir()) + if f.suffix == ".md" + ) return "\n\n---\n\n".join(parts) diff --git a/koan/app/log_rotation.py b/koan/app/log_rotation.py index 1288b3c18..a0fb892aa 100644 --- a/koan/app/log_rotation.py +++ b/koan/app/log_rotation.py @@ -9,6 +9,7 @@ Configurable via instance/config.yaml under `logs:` key. """ +import contextlib import fcntl import gzip import os @@ -112,14 +113,11 @@ def rotate_log(log_path: Path, max_backups: int = DEFAULT_MAX_BACKUPS, _compress_file(plain) finally: if lock_fh: - try: - lock_fh.close() # close() releases the flock - except (OSError, ValueError): - pass - try: + # close() releases the flock + with contextlib.suppress(OSError, ValueError): + lock_fh.close() + with contextlib.suppress(OSError): lock_path.unlink(missing_ok=True) - except OSError: - pass def _backup_path(log_path: Path, index: int) -> Path: @@ -134,10 +132,8 @@ def _backup_path(log_path: Path, index: int) -> Path: def _remove_backup(path: Path) -> None: """Remove a backup file (plain or compressed).""" - try: + with contextlib.suppress(OSError): path.unlink(missing_ok=True) - except OSError: - pass # Also remove the other form (plain vs compressed) try: @@ -204,10 +200,8 @@ def _compress_file(path: Path) -> None: except (OSError, IOError): # Compression failed — keep uncompressed file, clean up partial gz if temp_gz and temp_gz.exists(): - try: + with contextlib.suppress(OSError): temp_gz.unlink() - except OSError: - pass # Also clean up any partial gz file at final location try: @@ -224,10 +218,8 @@ def cleanup_old_backups(log_dir: Path, process_name: str, for i in range(max_backups + 1, max_backups + 10): path = _backup_path(base, i) if path.exists(): - try: - # Also remove compressed form + # Also remove compressed form + with contextlib.suppress(OSError): _remove_backup(path) - except OSError: - pass else: break diff --git a/koan/app/loop_manager.py b/koan/app/loop_manager.py index 27102e64f..54db88dba 100644 --- a/koan/app/loop_manager.py +++ b/koan/app/loop_manager.py @@ -16,6 +16,7 @@ """ import argparse +import contextlib import logging import os import re @@ -424,16 +425,12 @@ def _skills_dir_mtime(instance_dir: str) -> float: """Get the max mtime of core and instance skills directories.""" best = 0.0 core_dir = Path(__file__).resolve().parent.parent / "skills" / "core" - try: + with contextlib.suppress(OSError): best = max(best, core_dir.stat().st_mtime) - except OSError: - pass instance_skills = Path(instance_dir) / "skills" if instance_skills.is_dir(): - try: + with contextlib.suppress(OSError): best = max(best, instance_skills.stat().st_mtime) - except OSError: - pass return best @@ -509,7 +506,7 @@ def _get_known_repos_from_projects(koan_root: str) -> Optional[set]: # 1. projects.yaml — primary source projects_config = load_projects_config(koan_root) if projects_config: - for name, proj in projects_config.get("projects", {}).items(): + for proj in projects_config.get("projects", {}).values(): if not isinstance(proj, dict): continue gh_url = proj.get("github_url", "") @@ -525,12 +522,12 @@ def _get_known_repos_from_projects(koan_root: str) -> Optional[set]: from app.projects_merged import get_all_github_urls_cache, get_github_url_cache # Primary URLs (origin remote) - for _name, url in get_github_url_cache().items(): + for url in get_github_url_cache().values(): if url: known_repos.add(_normalize_github_url(url)) # All remote URLs (origin + upstream + others) - for _name, urls in get_all_github_urls_cache().items(): + for urls in get_all_github_urls_cache().values(): for url in urls: if url: known_repos.add(_normalize_github_url(url)) diff --git a/koan/app/memory_manager.py b/koan/app/memory_manager.py index 23734ba9b..b562ca69b 100644 --- a/koan/app/memory_manager.py +++ b/koan/app/memory_manager.py @@ -24,6 +24,7 @@ cleanup Run all cleanup tasks """ +import contextlib import hashlib import shutil import subprocess @@ -660,10 +661,8 @@ def compact_learnings( # Store hash of the NEW content to avoid re-compacting new_content = learnings_path.read_text(encoding="utf-8") new_hash = hashlib.sha256(new_content.encode("utf-8")).hexdigest() - try: + with contextlib.suppress(OSError): atomic_write(hash_path, new_hash) - except OSError: - pass return {"original_lines": original_count, "compacted_lines": compacted_count, "skipped": False, "method": "semantic"} diff --git a/koan/app/messaging/__init__.py b/koan/app/messaging/__init__.py index 09a9d311e..780cfb8ed 100644 --- a/koan/app/messaging/__init__.py +++ b/koan/app/messaging/__init__.py @@ -9,6 +9,7 @@ provider.send_message("Hello from Kōan") """ +import contextlib import os import sys import threading @@ -23,10 +24,17 @@ _instance: Optional[MessagingProvider] = None _instance_lock = threading.Lock() +# Set to True after ``_ensure_providers_loaded`` walks ``_PROVIDER_MODULES`` +# once. Tracking loop completion explicitly — rather than inferring it from +# ``bool(_providers)`` — avoids skipping unloaded modules when something +# imported a single provider as a side-effect before the loader ran. +_modules_loaded: bool = False + # List of known provider modules for auto-loading _PROVIDER_MODULES = [ "app.messaging.telegram", "app.messaging.slack", + "app.messaging.matrix", ] @@ -142,15 +150,23 @@ def _resolve_provider_name() -> str: def _ensure_providers_loaded(): - """Import provider modules to trigger registration.""" - if _providers: + """Import provider modules to trigger registration. + + Short-circuits on the explicit ``_modules_loaded`` flag rather than + ``bool(_providers)``. The latter looks like the same check but is + wrong: if anything imports e.g. ``app.messaging.telegram`` first + (which the default startup path does), ``_providers`` becomes + ``{"telegram": ...}`` without this loader ever running, and a later + request for ``matrix`` / ``slack`` would skip the import loop and + leave them unregistered. + """ + global _modules_loaded + if _modules_loaded: return - for module_name in _PROVIDER_MODULES: - try: + with contextlib.suppress(ImportError): __import__(module_name) - except ImportError: - pass + _modules_loaded = True __all__ = [ diff --git a/koan/app/messaging/matrix.py b/koan/app/messaging/matrix.py new file mode 100644 index 000000000..4b639da3f --- /dev/null +++ b/koan/app/messaging/matrix.py @@ -0,0 +1,286 @@ +"""Matrix messaging provider. + +Talks to a Matrix homeserver via the Client-Server HTTP API. Synchronous +implementation using `requests`, mirroring the Telegram provider's style +(long-poll via /sync, send via /rooms/{roomId}/send). + +Configuration is read from instance/config.yaml (recommended) under the +``messaging.matrix`` section, with environment variables as legacy/override +fallback. + +config.yaml keys (under ``messaging.matrix``): + homeserver, access_token, user_id, room_id + +Environment variables (override config.yaml when set): + KOAN_MATRIX_HOMESERVER — Homeserver URL (e.g. https://matrix.org) + KOAN_MATRIX_ACCESS_TOKEN — Access token for the bot account + KOAN_MATRIX_USER_ID — Bot's Matrix user ID (e.g. @koan:matrix.org) + KOAN_MATRIX_ROOM_ID — Room to operate in (e.g. !abc123:matrix.org) +""" + +import itertools +import os +import sys +import threading +import time +import uuid +from typing import List, Optional +from urllib.parse import quote + +import requests + +from app.messaging.base import DEFAULT_MAX_MESSAGE_SIZE, Message, MessagingProvider, Update +from app.messaging import register_provider + + +MAX_MESSAGE_SIZE = DEFAULT_MAX_MESSAGE_SIZE +SYNC_TIMEOUT_MS = 30000 # 30s long-poll +SYNC_HTTP_TIMEOUT = 35 # leave 5s buffer over SYNC_TIMEOUT_MS + + +@register_provider("matrix") +class MatrixProvider(MessagingProvider): + """Matrix Client-Server API provider. + + The first call to poll_updates() performs an initial /sync to fetch the + current `next_batch` token without surfacing historical messages. Later + calls long-poll for new events using that token. + """ + + def __init__(self): + self._homeserver: str = "" + self._access_token: str = "" + self._user_id: str = "" + self._room_id: str = "" + + self._sync_token: Optional[str] = None + self._sync_initialized: bool = False + self._update_counter = itertools.count(1) + self._send_lock = threading.Lock() + + # -- MessagingProvider interface ------------------------------------------ + + def configure(self) -> bool: + from app.utils import load_config, load_dotenv + load_dotenv() + + cfg: dict = {} + messaging = load_config().get("messaging", {}) or {} + if isinstance(messaging, dict): + section = messaging.get("matrix", {}) or {} + if isinstance(section, dict): + cfg = section + + # env vars override config.yaml for backward compatibility + self._homeserver = ( + os.environ.get("KOAN_MATRIX_HOMESERVER") or cfg.get("homeserver", "") + ).rstrip("/") + self._access_token = ( + os.environ.get("KOAN_MATRIX_ACCESS_TOKEN") or cfg.get("access_token", "") + ) + self._user_id = ( + os.environ.get("KOAN_MATRIX_USER_ID") or cfg.get("user_id", "") + ) + self._room_id = ( + os.environ.get("KOAN_MATRIX_ROOM_ID") or cfg.get("room_id", "") + ) + + missing = [] + if not self._homeserver: + missing.append("homeserver") + if not self._access_token: + missing.append("access_token") + if not self._user_id: + missing.append("user_id") + if not self._room_id: + missing.append("room_id") + if missing: + print( + f"[matrix] Missing required settings: {', '.join(missing)}. " + f"Set in instance/config.yaml under messaging.matrix or via the " + f"corresponding KOAN_MATRIX_* env vars.", + file=sys.stderr, + ) + return False + + if not self._homeserver.startswith(("http://", "https://")): + print( + "[matrix] KOAN_MATRIX_HOMESERVER must start with http:// or https://", + file=sys.stderr, + ) + return False + + return True + + def get_provider_name(self) -> str: + return "matrix" + + def get_channel_id(self) -> str: + return self._room_id + + def send_message(self, text: str) -> bool: + """Send a message to the configured Matrix room, chunked if needed. + + Empty text is treated as a no-op success (matches Telegram behavior + for clearing test state). + """ + if not self._access_token or not self._room_id: + print("[matrix] Not configured — cannot send.", file=sys.stderr) + return False + + if not text: + return True + + ok = True + for chunk in self.chunk_message(text, max_size=MAX_MESSAGE_SIZE): + with self._send_lock: + if not self._send_chunk(chunk): + ok = False + return ok + + def poll_updates(self, offset: Optional[int] = None) -> List[Update]: + """Long-poll /sync for new room events. + + The `offset` parameter is unused — Matrix uses an opaque sync token + stored on the provider instance. The first call discards historical + events and only returns the current `next_batch`. + """ + if not self._access_token: + return [] + + params: dict = {"timeout": SYNC_TIMEOUT_MS} + if self._sync_token: + params["since"] = self._sync_token + else: + # Initial sync: skip long-poll; we discard historical events. + params["full_state"] = "false" + params["timeout"] = 0 + + headers = {"Authorization": f"Bearer {self._access_token}"} + sync_http_timeout = SYNC_HTTP_TIMEOUT if self._sync_token else 10 + try: + resp = requests.get( + f"{self._homeserver}/_matrix/client/v3/sync", + params=params, + headers=headers, + timeout=sync_http_timeout, + ) + data = resp.json() + except (requests.RequestException, ValueError) as e: + print(f"[matrix] poll_updates error: {e}", file=sys.stderr) + return [] + + next_batch = data.get("next_batch") + if not next_batch: + return [] + + # First sync — record the cursor, return nothing. + if not self._sync_initialized: + self._sync_token = next_batch + self._sync_initialized = True + return [] + + updates = self._parse_room_events(data) + self._sync_token = next_batch + return updates + + # -- Internal helpers ----------------------------------------------------- + + def _parse_room_events(self, sync_data: dict) -> List[Update]: + """Extract m.room.message events from our configured room.""" + rooms = sync_data.get("rooms", {}).get("join", {}) + room = rooms.get(self._room_id, {}) + events = room.get("timeline", {}).get("events", []) + + updates: List[Update] = [] + for event in events: + if event.get("type") != "m.room.message": + continue + sender = event.get("sender", "") + # Skip our own messages + if sender == self._user_id: + continue + + content = event.get("content", {}) + msgtype = content.get("msgtype") + if msgtype != "m.text": + continue + + body = content.get("body", "") + if not body: + continue + + updates.append( + Update( + update_id=next(self._update_counter), + message=Message( + text=body, + role="user", + timestamp=str(event.get("origin_server_ts", "")), + raw_data=event, + ), + raw_data=event, + ) + ) + return updates + + def _send_chunk(self, text: str) -> bool: + """PUT a single m.room.message to the homeserver.""" + from app.retry import retry_with_backoff + + txn_id = uuid.uuid4().hex + url = ( + f"{self._homeserver}/_matrix/client/v3/rooms/" + f"{quote(self._room_id, safe='')}/send/m.room.message/{txn_id}" + ) + payload = {"msgtype": "m.text", "body": text} + headers = {"Authorization": f"Bearer {self._access_token}"} + + def _do_put(): + resp = requests.put(url, json=payload, headers=headers, timeout=10) + if resp.status_code >= 400: + # 4xx is not retryable; raise ValueError to short-circuit. + if 400 <= resp.status_code < 500: + print( + f"[matrix] API error {resp.status_code}: {resp.text[:200]}", + file=sys.stderr, + ) + return False + # 5xx — surface as RequestException so retry_with_backoff retries. + raise requests.RequestException( + f"matrix HTTP {resp.status_code}: {resp.text[:200]}" + ) + return True + + try: + return bool( + retry_with_backoff( + _do_put, + retryable=(requests.RequestException,), + label="matrix send", + ) + ) + except requests.RequestException as e: + print(f"[matrix] Send error after retries: {e}", file=sys.stderr) + return False + + def send_typing(self) -> bool: + """Send a typing indicator to the room (auto-expires after ~10s).""" + if not self._access_token or not self._room_id or not self._user_id: + return False + url = ( + f"{self._homeserver}/_matrix/client/v3/rooms/" + f"{quote(self._room_id, safe='')}/typing/" + f"{quote(self._user_id, safe='')}" + ) + headers = {"Authorization": f"Bearer {self._access_token}"} + try: + resp = requests.put( + url, + json={"typing": True, "timeout": 10000}, + headers=headers, + timeout=5, + ) + return resp.status_code < 400 + except requests.RequestException: + return False diff --git a/koan/app/missions.py b/koan/app/missions.py index edeab6377..4f30dc33d 100644 --- a/koan/app/missions.py +++ b/koan/app/missions.py @@ -1755,6 +1755,7 @@ def quarantine_mission( def _enforce_quarantine_cap(path: "Path") -> None: """If the quarantine file exceeds QUARANTINE_MAX_BYTES, prune oldest half.""" from pathlib import Path + from app.utils import atomic_write path = Path(path) if not path.exists(): @@ -1765,7 +1766,7 @@ def _enforce_quarantine_cap(path: "Path") -> None: lines = path.read_text().splitlines(keepends=True) # Keep the newer half half = len(lines) // 2 - path.write_text("".join(lines[half:])) + atomic_write(path, "".join(lines[half:])) # ── CI section helpers ──────────────────────────────────────────────────────── @@ -1972,3 +1973,59 @@ def update_ci_item_attempt(content: str, pr_url: str) -> str: break return normalize_content("\n".join(lines)) + + +# --------------------------------------------------------------------------- +# Duplicate detection +# --------------------------------------------------------------------------- + +# Regex to extract the "action signature" from a mission line: +# /command https://github.com/... → ("command", "url") +_GITHUB_ACTION_RE = re.compile( + r"/(rebase|review|recreate|squash|ci_check|fix|check|gh_request)\s+" + r"(https://github\.com/[^\s]+)" +) + + +def _extract_mission_signature(text: str) -> Optional[str]: + """Extract a normalized signature from a mission line for dedup. + + For GitHub-related missions (/rebase, /review, etc.), the signature is + "command:url" — two missions are duplicates if they target the same + command on the same URL. + + For other missions, returns None (no signature-based dedup). + """ + match = _GITHUB_ACTION_RE.search(text) + if match: + command = match.group(1) + url = match.group(2).rstrip("/)") # strip trailing paren or slash + return f"{command}:{url}" + return None + + +def is_duplicate_mission(content: str, new_entry: str) -> bool: + """Check if a mission with the same action signature already exists. + + Checks both Pending and In Progress sections. + + Args: + content: Full missions.md content. + new_entry: The mission entry about to be inserted. + + Returns: + True if a duplicate exists, False otherwise. + """ + signature = _extract_mission_signature(new_entry) + if signature is None: + return False + + sections = parse_sections(content) + existing = sections.get("pending", []) + sections.get("in_progress", []) + + for item in existing: + item_sig = _extract_mission_signature(item) + if item_sig == signature: + return True + + return False diff --git a/koan/app/onboarding.py b/koan/app/onboarding.py index 2452c8453..34ea49df0 100644 --- a/koan/app/onboarding.py +++ b/koan/app/onboarding.py @@ -10,6 +10,7 @@ make onboard """ +import contextlib import json import os import platform @@ -382,16 +383,22 @@ def step_messaging(state: OnboardingState) -> OnboardingState: verify_telegram_token, ) - # Check if already configured + # Check if already configured (any supported provider) token = get_env_var("KOAN_TELEGRAM_TOKEN") chat_id = get_env_var("KOAN_TELEGRAM_CHAT_ID") if token and "your-bot-token" not in token and chat_id and "your-chat-id" not in chat_id: print(f" {green('✓')} Messaging already configured.") return state + if get_env_var("KOAN_SLACK_BOT_TOKEN") and get_env_var("KOAN_SLACK_CHANNEL_ID"): + print(f" {green('✓')} Messaging already configured.") + return state + if get_env_var("KOAN_MATRIX_ACCESS_TOKEN") and get_env_var("KOAN_MATRIX_ROOM_ID"): + print(f" {green('✓')} Messaging already configured.") + return state provider_idx = ask_choice( "Which messaging platform?", - ["Telegram (default)", "Slack"], + ["Telegram (default)", "Slack", "Matrix"], default=0, ) @@ -414,6 +421,27 @@ def step_messaging(state: OnboardingState) -> OnboardingState: print(f"\n {green('✓')} Slack configuration saved.") else: print(f"\n {yellow('○')} Incomplete Slack config — skipping for now.") + elif provider_idx == 2: + # Matrix setup + print(f"\n {bold('Matrix setup')}") + print(f" {dim('See docs/messaging-matrix.md for setup instructions.')}") + print() + + homeserver = ask("Matrix Homeserver URL (https://matrix.org)") + access_token = ask("Matrix access token (syt_...)") + user_id = ask("Bot Matrix user ID (@koan:matrix.org)") + room_id = ask("Room ID (!abcdef:matrix.org)") + + if homeserver and access_token and user_id and room_id: + update_env_var("KOAN_MATRIX_HOMESERVER", homeserver) + update_env_var("KOAN_MATRIX_ACCESS_TOKEN", access_token) + update_env_var("KOAN_MATRIX_USER_ID", user_id) + update_env_var("KOAN_MATRIX_ROOM_ID", room_id) + update_env_var("KOAN_MESSAGING_PROVIDER", "matrix") + state.data["messaging_provider"] = "matrix" + print(f"\n {green('✓')} Matrix configuration saved.") + else: + print(f"\n {yellow('○')} Incomplete Matrix config — skipping for now.") else: # Telegram setup print(f"\n {bold('Telegram setup')}") @@ -441,10 +469,8 @@ def step_messaging(state: OnboardingState) -> OnboardingState: # Try to auto-detect chat ID print(f"\n {dim('Send any message to your bot on Telegram, then press Enter.')}") if _is_interactive: - try: + with contextlib.suppress(EOFError, KeyboardInterrupt): input(f" {dim('Press Enter when ready...')}") - except (EOFError, KeyboardInterrupt): - pass chat_id_detected = get_chat_id_from_updates(bot_token) if chat_id_detected: diff --git a/koan/app/outbox_manager.py b/koan/app/outbox_manager.py index 3d1a922ea..ac650355d 100644 --- a/koan/app/outbox_manager.py +++ b/koan/app/outbox_manager.py @@ -24,6 +24,7 @@ ) from app.notify import NotificationPriority, NOTIFICATION_SUPPRESSED, send_telegram from app.outbox_scanner import scan_and_log +from app.utils import atomic_write # Pre-compiled regex for outbox priority header parsing @@ -144,7 +145,7 @@ def flush(self): try: content = f.read().strip() if content: - staging.write_text(content) + atomic_write(staging, content) f.seek(0) f.truncate() f.flush() diff --git a/koan/app/pause_manager.py b/koan/app/pause_manager.py index 1f11194bf..ff0147f06 100644 --- a/koan/app/pause_manager.py +++ b/koan/app/pause_manager.py @@ -17,6 +17,7 @@ that could permanently block the agent. """ +import contextlib import json import os import re @@ -171,10 +172,8 @@ def create_pause( def remove_pause(koan_root: str) -> None: """Remove the pause file (single atomic delete).""" - try: + with contextlib.suppress(FileNotFoundError): os.remove(os.path.join(koan_root, PAUSE_FILE)) - except FileNotFoundError: - pass def check_and_resume(koan_root: str) -> Optional[str]: diff --git a/koan/app/pid_manager.py b/koan/app/pid_manager.py index 40af67bbb..d0dc7066b 100644 --- a/koan/app/pid_manager.py +++ b/koan/app/pid_manager.py @@ -19,6 +19,7 @@ release_pidfile(lock, koan_root, "awake") """ +import contextlib import fcntl import os import shutil @@ -444,10 +445,8 @@ def _read_runner_state(koan_root: Path) -> dict: status_file = koan_root / STATUS_FILE if status_file.exists(): - try: + with contextlib.suppress(OSError): state["status"] = status_file.read_text().strip() - except OSError: - pass pause_file = koan_root / PAUSE_FILE if pause_file.exists(): @@ -461,10 +460,8 @@ def _read_runner_state(koan_root: Path) -> dict: project_file = koan_root / PROJECT_FILE if project_file.exists(): - try: + with contextlib.suppress(OSError): state["project"] = project_file.read_text().strip() - except OSError: - pass return state @@ -711,10 +708,8 @@ def stop_processes(koan_root: Path, timeout: float = 5.0) -> dict: results[name] = "stopped" else: # Force kill - try: + with contextlib.suppress(OSError, ProcessLookupError): os.kill(pid, signal.SIGKILL) - except (OSError, ProcessLookupError): - pass # Wait briefly for SIGKILL to take effect _wait_for_exit(pid, 1.0) results[name] = "force_killed" diff --git a/koan/app/plan_runner.py b/koan/app/plan_runner.py index 9a7a62d94..4bb8f6907 100644 --- a/koan/app/plan_runner.py +++ b/koan/app/plan_runner.py @@ -168,9 +168,7 @@ def _run_issue_plan( # Format comments as plain text for the plan prompt comments_text = "" if jira_comments: - parts = [] - for c in jira_comments: - parts.append(f"**{c['author']}**:\n{c['body']}") + parts = [f"**{c['author']}**:\n{c['body']}" for c in jira_comments] comments_text = "\n\n---\n\n".join(parts) label = issue_key diff --git a/koan/app/pr_quality.py b/koan/app/pr_quality.py index d51dbae48..4f495fc8d 100644 --- a/koan/app/pr_quality.py +++ b/koan/app/pr_quality.py @@ -447,8 +447,10 @@ def _build_quality_report_section(report: dict, project_path: str) -> str: else: issue_count = len(scan.get("issues", [])) lines.append(f"**Code scan**: {issue_count} issue(s) found") - for issue in scan.get("issues", [])[:10]: - lines.append(f"- `{issue['file']}:{issue['line']}` — {issue['message']}") + lines.extend( + f"- `{issue['file']}:{issue['line']}` — {issue['message']}" + for issue in scan.get("issues", [])[:10] + ) lines.append("") # Test results @@ -469,8 +471,9 @@ def _build_quality_report_section(report: dict, project_path: str) -> str: else: issue_count = len(branch_result.get("issues", [])) lines.append(f"**Branch hygiene**: {issue_count} issue(s)") - for issue in branch_result.get("issues", [])[:5]: - lines.append(f"- {issue['message']}") + lines.extend( + f"- {issue['message']}" for issue in branch_result.get("issues", [])[:5] + ) lines.append("") lines.append("*Generated by Kōan post-mission quality pipeline*") @@ -541,8 +544,10 @@ def post_quality_comment(project_path: str, quality_report: dict) -> bool: if not scan.get("clean", True): comment_lines.append("**Code issues found:**") - for issue in scan.get("issues", [])[:10]: - comment_lines.append(f"- `{issue['file']}:{issue['line']}` — {issue['message']}") + comment_lines.extend( + f"- `{issue['file']}:{issue['line']}` — {issue['message']}" + for issue in scan.get("issues", [])[:10] + ) comment_lines.append("") if tests and not tests.get("passed", True) and not tests.get("skipped", False): @@ -551,8 +556,9 @@ def post_quality_comment(project_path: str, quality_report: dict) -> bool: if not branch.get("valid", True): comment_lines.append("**Branch hygiene issues:**") - for issue in branch.get("issues", [])[:5]: - comment_lines.append(f"- {issue['message']}") + comment_lines.extend( + f"- {issue['message']}" for issue in branch.get("issues", [])[:5] + ) comment_lines.append("") comment_lines.append("*Auto-merge was skipped due to quality gate issues.*") diff --git a/koan/app/pr_review_learning.py b/koan/app/pr_review_learning.py index 7fa7527ed..1a334ccd9 100644 --- a/koan/app/pr_review_learning.py +++ b/koan/app/pr_review_learning.py @@ -307,12 +307,9 @@ def _compute_review_hash(prs: List[dict]) -> str: parts = [] for pr in sorted(prs, key=lambda p: p.get("number", 0)): parts.append(str(pr.get("number", ""))) - for review in pr.get("reviews", []): - parts.append(review.get("body") or "") - for comment in pr.get("review_comments", []): - parts.append(comment.get("body") or "") - for comment in pr.get("issue_comments", []): - parts.append(comment.get("body") or "") + parts.extend(review.get("body") or "" for review in pr.get("reviews", [])) + parts.extend(comment.get("body") or "" for comment in pr.get("review_comments", [])) + parts.extend(comment.get("body") or "" for comment in pr.get("issue_comments", [])) content = "|".join(parts) return hashlib.sha256(content.encode()).hexdigest() diff --git a/koan/app/projects_config.py b/koan/app/projects_config.py index 8fd31b420..7a0a77876 100644 --- a/koan/app/projects_config.py +++ b/koan/app/projects_config.py @@ -490,6 +490,41 @@ def get_project_submit_to_repository(config: dict, project_name: str) -> dict: return result +def get_project_security_config(config: dict, project_name: str) -> dict: + """Get security configuration for a project from projects.yaml. + + Returns a dict with keys: + - ``pvrs``: ``"auto"`` (default), ``"true"``, or ``"false"`` + - ``pvrs_threshold``: ``"high"`` (default) — minimum severity routed + to PVRS. One of ``"critical"``, ``"high"``, ``"medium"``, ``"low"``. + + Example projects.yaml:: + + defaults: + security: + pvrs: auto + pvrs_threshold: high + projects: + myapp: + security: + pvrs: false # force public issues + """ + project_cfg = get_project_config(config, project_name) + security = project_cfg.get("security", {}) + if not isinstance(security, dict): + security = {} + + pvrs = str(security.get("pvrs", "auto")).strip().lower() + if pvrs not in ("auto", "true", "false"): + pvrs = "auto" + + threshold = str(security.get("pvrs_threshold", "high")).strip().lower() + if threshold not in ("critical", "high", "medium", "low"): + threshold = "high" + + return {"pvrs": pvrs, "pvrs_threshold": threshold} + + def save_projects_config(koan_root: str, config: dict) -> None: """Write config back to projects.yaml atomically, preserving comments. diff --git a/koan/app/provider/__init__.py b/koan/app/provider/__init__.py index 19692192c..f4c402c02 100644 --- a/koan/app/provider/__init__.py +++ b/koan/app/provider/__init__.py @@ -20,6 +20,7 @@ provider/__init__.py — Registry, resolution, convenience functions """ +import contextlib import os import re import subprocess @@ -244,10 +245,8 @@ def _write_system_prompt_file(content: str) -> str: f.write(content) except Exception: # If NamedTemporaryFile raised after creating the file, unlink it. - try: + with contextlib.suppress(OSError, NameError): os.unlink(path) # type: ignore[possibly-undefined] - except (OSError, NameError): - pass raise return path @@ -309,10 +308,8 @@ def cleanup_managed_paths(paths: List[str]) -> None: a ``finally`` block; never raises. """ for p in paths: - try: + with contextlib.suppress(OSError): os.unlink(p) - except OSError: - pass _MAX_TURNS_RE = re.compile(r"Reached max turns", re.IGNORECASE) diff --git a/koan/app/rebase_pr.py b/koan/app/rebase_pr.py index 493ba7b8d..edfa80778 100644 --- a/koan/app/rebase_pr.py +++ b/koan/app/rebase_pr.py @@ -21,6 +21,7 @@ from typing import List, Optional, Tuple from app.claude_step import ( + CI_STATUS_BLOCKED_APPROVAL, _build_pr_prompt, _fetch_branch, _fetch_failed_logs, @@ -55,7 +56,7 @@ def fetch_pr_context(owner: str, repo: str, pr_number: str) -> dict: # Fetch PR metadata pr_json = run_gh( "pr", "view", pr_number, "--repo", full_repo, "--json", - "title,body,headRefName,baseRefName,state,author,url,headRepositoryOwner", + "title,body,headRefName,baseRefName,state,author,url,headRepositoryOwner,mergeable", ) # Fetch review comment count from REST API for pending review detection. @@ -137,6 +138,7 @@ def _fetch_review_comment_count() -> int: "author": metadata.get("author", {}).get("login", ""), "head_owner": metadata.get("headRepositoryOwner", {}).get("login", ""), "url": metadata.get("url", ""), + "mergeable": metadata.get("mergeable", "UNKNOWN"), "diff": truncate_diff(diff, 32000), "review_comments": truncate_text(comments_json, 4000), "reviews": truncate_text(reviews_json, 3000), @@ -688,10 +690,11 @@ def _get_conflicted_files(project_path: str) -> List[str]: capture_output=True, text=True, cwd=project_path, timeout=30, ) - files = [] - for line in result.stdout.splitlines(): - if len(line) >= 4 and line[:2] in _UNMERGED_STATUSES: - files.append(line[3:].strip()) + files = [ + line[3:].strip() + for line in result.stdout.splitlines() + if len(line) >= 4 and line[:2] in _UNMERGED_STATUSES + ] return files except Exception as e: print(f"[rebase_pr] failed to list conflicted files: {e}", file=sys.stderr) @@ -901,6 +904,10 @@ def _fix_existing_ci_failures( actions_log.append("Pre-push CI check: previous run passed") elif ci_status == "pending": actions_log.append("Pre-push CI check: previous run still pending") + elif ci_status == CI_STATUS_BLOCKED_APPROVAL: + actions_log.append( + "Pre-push CI check: previous run waiting for maintainer approval" + ) else: actions_log.append("Pre-push CI check: no CI runs found") return False @@ -1021,6 +1028,13 @@ def _run_ci_check_and_fix( actions_log.append("CI polling timed out") return "CI still running (timed out waiting)." + if ci_status == CI_STATUS_BLOCKED_APPROVAL: + # Workflow runs are gated on maintainer/environment approval — + # pushing more commits won't unstick them. Bail out instead of + # burning quota on fix attempts that can't possibly run. + actions_log.append("CI waiting for maintainer approval — skipping fixes") + return "CI waiting for maintainer approval — fixes skipped." + # CI failed — attempt fixes for attempt in range(1, MAX_CI_FIX_ATTEMPTS + 1): # Check if PR has been merged or has conflicts before attempting fix @@ -1091,6 +1105,15 @@ def _run_ci_check_and_fix( actions_log.append(f"CI {ci_status} after fix attempt {attempt}") return f"CI fix pushed (attempt {attempt}), CI status: {ci_status}." + if ci_status == CI_STATUS_BLOCKED_APPROVAL: + actions_log.append( + f"CI waiting for maintainer approval after fix attempt {attempt} — stopping" + ) + return ( + f"CI fix pushed (attempt {attempt}), but new run is waiting " + "for maintainer approval." + ) + # Exhausted retries — report failure with log excerpt log_excerpt = ci_logs[:2000] if ci_logs else "(no logs available)" actions_log.append(f"CI still failing after {MAX_CI_FIX_ATTEMPTS} fix attempts") @@ -1355,8 +1378,7 @@ def _build_rebase_comment( change_items = _extract_change_items(actions_log, change_summary) if change_items: parts.append("### Changes applied\n") - for item in change_items: - parts.append(f"- {item}") + parts.extend(f"- {item}" for item in change_items) parts.append("") # ── 3. Stats ──────────────────────────────────────────────────── @@ -1373,8 +1395,7 @@ def _build_rebase_comment( ] if meaningful_actions: parts.append("
\nActions performed\n") - for a in meaningful_actions: - parts.append(f"- {a}") + parts.extend(f"- {a}" for a in meaningful_actions) parts.append("\n
\n") # ── 5. CI ─────────────────────────────────────────────────────── diff --git a/koan/app/recover.py b/koan/app/recover.py index f0bbac9b2..2d0bb9dd2 100644 --- a/koan/app/recover.py +++ b/koan/app/recover.py @@ -23,6 +23,7 @@ Missions file is updated in-place if recovery happens. """ +import contextlib import fcntl import json import re @@ -326,12 +327,10 @@ def _recover_transform(content: str) -> str: if i == pending_start: new_lines.append("") - for m in recovered: - new_lines.append(m) + new_lines.extend(recovered) if i == in_progress_start: - for m in remaining_in_progress: - new_lines.append(m) + new_lines.extend(remaining_in_progress) if not any(m.strip() for m in remaining_in_progress): new_lines.append("") @@ -339,8 +338,7 @@ def _recover_transform(content: str) -> str: if failed_bounds and i == failed_bounds[0]: # Re-insert original failed content (minus section boundaries we'll re-emit) orig_failed = lines[failed_bounds[0] + 1 : failed_bounds[1]] - for fl in orig_failed: - new_lines.append(fl) + new_lines.extend(orig_failed) if escalated: for m in escalated: clean = _strip_recovery_counter(m).rstrip() @@ -398,10 +396,8 @@ def _inject_checkpoint_context(instance_dir: str, mission_texts: list) -> None: pending_path = Path(instance_dir) / "journal" / "pending.md" try: existing = "" - try: + with contextlib.suppress(FileNotFoundError): existing = pending_path.read_text() - except FileNotFoundError: - pass # Append checkpoint context after existing content new_content = "" if existing.strip(): diff --git a/koan/app/recreate_pr.py b/koan/app/recreate_pr.py index 08e689a87..911541df7 100644 --- a/koan/app/recreate_pr.py +++ b/koan/app/recreate_pr.py @@ -13,6 +13,7 @@ 6. Comment on the original PR with cross-link """ +import contextlib import re import sys from pathlib import Path @@ -125,11 +126,10 @@ def run_recreate( # Create a fresh working branch from the upstream target work_branch = branch # We'll try to reuse the original branch name try: - # Delete local branch if it exists (we're recreating from scratch) - try: + # Delete local branch if it exists (we're recreating from scratch). + # Branch doesn't exist locally, that's fine. + with contextlib.suppress(RuntimeError, OSError): _run_git(["git", "branch", "-D", work_branch], cwd=project_path) - except (RuntimeError, OSError): - pass # Branch doesn't exist locally, that's fine _run_git( ["git", "checkout", "-b", work_branch, f"{upstream_remote}/{base}"], diff --git a/koan/app/restart_manager.py b/koan/app/restart_manager.py index abc22ec06..b9252ce1f 100644 --- a/koan/app/restart_manager.py +++ b/koan/app/restart_manager.py @@ -1,56 +1,93 @@ """Restart signal management for Kōan processes. -Provides file-based restart signaling between bridge and run loop: -- Bridge creates .koan-restart to signal both processes -- run.py checks at loop start and exits with code 42 -- Bridge detects the signal and re-execs itself via os.execv() +Provides file-based restart signaling between bridge and run loop. + +Two consumers (bridge and runner) each get their own marker so a fast +wrapper-restart of the runner can no longer wipe the signal before the +bridge's polling tick sees it. The legacy single-file marker is also +written so a pre-upgrade incarnation polling ``.koan-restart`` can still +detect the request and re-exec into the new code. The restart flow: -1. User sends /restart on Telegram -2. Bridge writes .koan-restart -3. Bridge sends ack to Telegram -4. Bridge re-execs itself (os.execv replaces process in-place) -5. run.py detects .koan-restart at next iteration, exits with code 42 -6. Wrapper re-launches run.py +1. ``request_restart`` writes ``.koan-restart-bridge``, + ``.koan-restart-run`` and (for backward compat) ``.koan-restart``. +2. Bridge's main loop notices ``.koan-restart-bridge`` and re-execs via + ``os.execv`` (same PID, fresh interpreter). +3. Runner's main loop notices ``.koan-restart-run`` and exits with + ``RESTART_EXIT_CODE``; its wrapper relaunches it. +4. Each process clears only its own marker on startup, so neither can + silence the signal for the other. Exit code 42 is the restart sentinel — any other exit is a real stop. """ +import contextlib import os import sys import time from pathlib import Path +from typing import Optional from app.signals import RESTART_FILE RESTART_EXIT_CODE = 42 +# Per-consumer marker files. The legacy ``RESTART_FILE`` (``.koan-restart``) +# is kept for backward compatibility: a pre-upgrade bridge that is still +# polling the old single-file marker can pick up the first post-upgrade +# request and re-exec into the new code. +RESTART_BRIDGE_FILE = ".koan-restart-bridge" +RESTART_RUN_FILE = ".koan-restart-run" + +_TARGET_FILES = { + "bridge": RESTART_BRIDGE_FILE, + "run": RESTART_RUN_FILE, + None: RESTART_FILE, +} + + +def _marker_path(koan_root: str, target: Optional[str]) -> str: + try: + fname = _TARGET_FILES[target] + except KeyError as exc: + raise ValueError( + f"Unknown restart target {target!r}; " + f"expected one of {sorted(k for k in _TARGET_FILES if k)!r} or None" + ) from exc + return os.path.join(koan_root, fname) + def request_restart(koan_root: str) -> None: - """Create the restart signal file. + """Create restart signal files for both consumers (and the legacy file). - Both processes check for this file: - - run.py: at loop start, exits with code 42 - - awake.py: in main loop, triggers os.execv() + Writes three markers so each consumer can clear its own without + silencing the other, and so a pre-upgrade incarnation still polling + the legacy ``.koan-restart`` will also wake up and re-exec. """ from app.utils import atomic_write - atomic_write( - Path(koan_root) / RESTART_FILE, - f"restart requested at {time.strftime('%H:%M:%S')}\n", - ) + body = f"restart requested at {time.strftime('%H:%M:%S')}\n" + for fname in _TARGET_FILES.values(): + atomic_write(Path(koan_root) / fname, body) -def check_restart(koan_root: str, since: float = 0) -> bool: - """Check if a restart has been requested. +def check_restart( + koan_root: str, + since: float = 0, + target: Optional[str] = None, +) -> bool: + """Check if a restart has been requested for ``target``. Args: koan_root: Root path for the koan installation. - since: If > 0, only return True if the file was modified after this - timestamp. Used to ignore stale restart signals left over - from a previous process incarnation (prevents restart loops - when Telegram re-delivers the /restart message). + since: If > 0, only return True if the marker was modified after + this timestamp. Used to ignore stale restart signals left + over from a previous process incarnation (prevents restart + loops when Telegram re-delivers the /restart message). + target: ``"bridge"`` or ``"run"`` to check the per-consumer + marker. ``None`` (default) checks the legacy single marker + for backward compatibility. """ - restart_file = os.path.join(koan_root, RESTART_FILE) + restart_file = _marker_path(koan_root, target) if not os.path.isfile(restart_file): return False try: @@ -61,13 +98,15 @@ def check_restart(koan_root: str, since: float = 0) -> bool: return True -def clear_restart(koan_root: str) -> None: - """Remove the restart signal file.""" - path = os.path.join(koan_root, RESTART_FILE) - try: +def clear_restart(koan_root: str, target: Optional[str] = None) -> None: + """Remove the restart signal file for ``target``. + + A consumer should only clear its own marker so the other consumer + can still observe the request on its next poll tick. + """ + path = _marker_path(koan_root, target) + with contextlib.suppress(FileNotFoundError): os.remove(path) - except FileNotFoundError: - pass def reexec_bridge() -> None: diff --git a/koan/app/review_runner.py b/koan/app/review_runner.py index 126e5e15f..0ba088eff 100644 --- a/koan/app/review_runner.py +++ b/koan/app/review_runner.py @@ -548,20 +548,17 @@ def _format_review_as_markdown(review_data: dict, title: str = "") -> str: if met: lines.append(f"✅ **Met** ({len(met)})") lines.append("") - for req in met: - lines.append(f"- {req}") + lines.extend(f"- {req}" for req in met) lines.append("") if missing: lines.append(f"❌ **Missing** ({len(missing)})") lines.append("") - for req in missing: - lines.append(f"- {req}") + lines.extend(f"- {req}" for req in missing) lines.append("") if out_of_scope: lines.append(f"📋 **Out of scope** ({len(out_of_scope)})") lines.append("") - for item in out_of_scope: - lines.append(f"- {item}") + lines.extend(f"- {item}" for item in out_of_scope) lines.append("") lines.append("---") lines.append("") @@ -635,6 +632,15 @@ def _format_review_as_markdown(review_data: dict, title: str = "") -> str: return "\n".join(lines) +def _conflict_notice(base: str) -> str: + """Build a prominent warning block when a PR has unresolved merge conflicts.""" + return ( + f"> ⚠️ **Merge conflicts detected** — this PR conflicts with `{base}` " + f"and cannot be merged until the author rebases (or merges in `{base}`) " + f"and resolves the conflicted files." + ) + + def _post_review_comment( owner: str, repo: str, pr_number: str, review_text: str, existing_comment: Optional[dict] = None, @@ -1013,6 +1019,12 @@ def run_review( ) review_body = _extract_review_body(raw_output) + # Prepend merge-conflict notice when the PR is in CONFLICTING state. + # The code review alone never surfaces this — the human needs an explicit + # call to resolve conflicts before merge is possible. + if context.get("mergeable") == "CONFLICTING": + review_body = _conflict_notice(context.get("base", "main")) + "\n\n" + review_body + # Step 6: Post (or update) review comment (Phase 3 — idempotent upsert) # Commit SHAs are embedded in the body upfront to avoid extra API calls. notify_fn(f"Posting review on PR #{pr_number}...") diff --git a/koan/app/review_schema.py b/koan/app/review_schema.py index c77b92b14..4b78a6cce 100644 --- a/koan/app/review_schema.py +++ b/koan/app/review_schema.py @@ -254,9 +254,11 @@ def validate_review(data: object) -> tuple: if not isinstance(pa, dict): errors.append("'plan_alignment' must be an object") else: - for key in ("requirements_met", "requirements_missing", "out_of_scope"): - if key in pa and not isinstance(pa[key], list): - errors.append(f"plan_alignment.{key}: must be an array") + errors.extend( + f"plan_alignment.{key}: must be an array" + for key in ("requirements_met", "requirements_missing", "out_of_scope") + if key in pa and not isinstance(pa[key], list) + ) return (len(errors) == 0, errors) diff --git a/koan/app/run.py b/koan/app/run.py index 9a1d9c786..c4bb20804 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 signal import subprocess @@ -730,7 +731,7 @@ def handle_pause( if Path(koan_root, CYCLE_FILE).exists(): log("pause", "Update signal detected while paused") break - if check_restart(koan_root): + if check_restart(koan_root, target="run"): break time.sleep(5) @@ -781,7 +782,7 @@ def main_loop(): Path(koan_root, SHUTDOWN_FILE).unlink(missing_ok=True) Path(koan_root, CYCLE_FILE).unlink(missing_ok=True) Path(koan_root, ABORT_FILE).unlink(missing_ok=True) - clear_restart(koan_root) + clear_restart(koan_root, target="run") # Install SIGINT handler signal.signal(signal.SIGINT, _on_sigint) @@ -850,9 +851,9 @@ def main_loop(): break # --- Restart check --- - if check_restart(koan_root, since=start_time): + if check_restart(koan_root, since=start_time, target="run"): log("koan", "Restart requested. Exiting for re-launch...") - clear_restart(koan_root) + clear_restart(koan_root, target="run") sys.exit(RESTART_EXIT_CODE) # --- Pause mode --- @@ -2841,10 +2842,8 @@ def _reset_liveness(): skill_stderr = "" finally: if proc is not None and proc.stdout is not None: - try: + with contextlib.suppress(OSError): proc.stdout.close() - except OSError: - pass if stderr_fh is not None: stderr_fh.close() _sig.claude_proc = None @@ -2902,10 +2901,8 @@ def _reset_liveness(): def _cleanup_temp(*files): """Remove temporary files.""" for f in files: - try: + with contextlib.suppress(OSError): Path(f).unlink(missing_ok=True) - except OSError: - pass # --------------------------------------------------------------------------- diff --git a/koan/app/session_manager.py b/koan/app/session_manager.py index 4166ed0cc..965a1e0b9 100644 --- a/koan/app/session_manager.py +++ b/koan/app/session_manager.py @@ -12,6 +12,7 @@ of file-based state with fcntl locks for cross-process safety. """ +import contextlib import fcntl import json import os @@ -394,18 +395,14 @@ def kill_session( proc.wait(timeout=5) except subprocess.TimeoutExpired: os.killpg(pgid, signal.SIGKILL) - try: + with contextlib.suppress(subprocess.TimeoutExpired): proc.wait(timeout=5) - except subprocess.TimeoutExpired: - pass except (ProcessLookupError, PermissionError, OSError): pass elif session.pid > 0: # No proc reference — try killing by PID - try: + with contextlib.suppress(ProcessLookupError, PermissionError, OSError): os.kill(session.pid, signal.SIGTERM) - except (ProcessLookupError, PermissionError, OSError): - pass # Call cleanup cleanup = getattr(session, "_cleanup", None) diff --git a/koan/app/session_tracker.py b/koan/app/session_tracker.py index ec361b2e8..fa646bf58 100644 --- a/koan/app/session_tracker.py +++ b/koan/app/session_tracker.py @@ -440,9 +440,7 @@ def get_staleness_warning(instance_dir: str, project: str) -> str: if empty_summaries: lines.append("Recent non-productive sessions:") - for s in empty_summaries[-3:]: # Show last 3 - if s: - lines.append(f" - {s[:100]}") + lines.extend(f" - {s[:100]}" for s in empty_summaries[-3:] if s) lines.append("") return "\n".join(lines) diff --git a/koan/app/shutdown_manager.py b/koan/app/shutdown_manager.py index 2ad6b75fb..9a562e7f9 100644 --- a/koan/app/shutdown_manager.py +++ b/koan/app/shutdown_manager.py @@ -12,6 +12,7 @@ prevents a leftover shutdown file from killing a freshly started instance. """ +import contextlib import os import time from pathlib import Path @@ -56,7 +57,5 @@ def is_shutdown_requested(koan_root: str, process_start_time: float) -> bool: def clear_shutdown(koan_root: str) -> None: """Remove the shutdown signal file.""" path = os.path.join(koan_root, SHUTDOWN_FILE) - try: + with contextlib.suppress(FileNotFoundError): os.remove(path) - except FileNotFoundError: - pass diff --git a/koan/app/skill_approval.py b/koan/app/skill_approval.py index f82dead7d..a1656e7e7 100644 --- a/koan/app/skill_approval.py +++ b/koan/app/skill_approval.py @@ -12,6 +12,7 @@ (prompt injection or message-forwarding attack) cannot guess it. """ +import contextlib import hashlib import re from pathlib import Path @@ -57,10 +58,8 @@ def mark_pending(skill_dir: Path, fingerprint: str) -> None: def clear_pending(skill_dir: Path) -> None: """Remove the pending marker. Idempotent.""" marker = skill_dir / MARKER_NAME - try: + with contextlib.suppress(FileNotFoundError): marker.unlink() - except FileNotFoundError: - pass def read_pending_fingerprint(skill_dir: Path) -> Optional[str]: diff --git a/koan/app/skill_dispatch.py b/koan/app/skill_dispatch.py index 02b79b3c4..47903130c 100644 --- a/koan/app/skill_dispatch.py +++ b/koan/app/skill_dispatch.py @@ -20,6 +20,7 @@ /namespace.skill -> resolved via skill registry """ +import contextlib import re import sys import threading @@ -45,16 +46,12 @@ def _get_skills_dir_mtime(instance_dir: Path) -> float: """Get the max mtime of core and instance skills directories.""" best = 0.0 core_dir = Path(__file__).resolve().parent.parent / "skills" / "core" - try: + with contextlib.suppress(OSError): best = max(best, core_dir.stat().st_mtime) - except OSError: - pass instance_skills = instance_dir / "skills" if instance_skills.is_dir(): - try: + with contextlib.suppress(OSError): best = max(best, instance_skills.stat().st_mtime) - except OSError: - pass return best @@ -738,10 +735,8 @@ def cleanup_skill_temp_files(skill_cmd: List[str]) -> None: if prefix and i + 1 < len(skill_cmd): path = skill_cmd[i + 1] if prefix in path: - try: + with contextlib.suppress(OSError): os.unlink(path) - except OSError: - pass def validate_skill_args(command: str, args: str) -> Optional[str]: diff --git a/koan/app/skills.py b/koan/app/skills.py index ec358bba9..f87a299b8 100644 --- a/koan/app/skills.py +++ b/koan/app/skills.py @@ -33,6 +33,7 @@ import re import subprocess import sys +import time from collections import namedtuple from dataclasses import dataclass, field from pathlib import Path @@ -231,17 +232,16 @@ def parse_skill_md(path: Path) -> Optional[Skill]: return None # Parse commands - commands = [] - for cmd_data in meta.get("commands", []): - if isinstance(cmd_data, dict) and "name" in cmd_data: - commands.append( - SkillCommand( - name=cmd_data["name"], - description=cmd_data.get("description", ""), - aliases=cmd_data.get("aliases", []), - usage=cmd_data.get("usage", ""), - ) - ) + commands = [ + SkillCommand( + name=cmd_data["name"], + description=cmd_data.get("description", ""), + aliases=cmd_data.get("aliases", []), + usage=cmd_data.get("usage", ""), + ) + for cmd_data in meta.get("commands", []) + if isinstance(cmd_data, dict) and "name" in cmd_data + ] # Resolve handler path (always record declared path; has_handler() checks existence) handler_path = None @@ -569,6 +569,11 @@ def execute_skill(skill: Skill, ctx: SkillContext) -> Optional[Union[str, SkillE return None +# Captured at import time so first-time observations in +# _refresh_stale_app_modules can tell whether a module's source file has been +# rewritten by auto-update since this process started (Python had no chance to +# pick up the new content because sys.modules still holds the pre-update copy). +_PROCESS_START_TIME: float = time.time() # mtime cache: module_name -> last-seen mtime (float) _module_mtimes: Dict[str, float] = {} @@ -676,9 +681,14 @@ def _refresh_stale_app_modules() -> None: cached_mtime = _module_mtimes.get(name) if cached_mtime is not None and current_mtime == cached_mtime: continue - # First time we see this module, or mtime changed - if cached_mtime is not None: - # mtime actually changed — reload + # Reload when either: (a) we have a baseline and the file changed, or + # (b) this is the first observation but the file was modified after the + # process started — i.e. auto-update rewrote it before we built a baseline. + should_reload = ( + cached_mtime is not None + or current_mtime > _PROCESS_START_TIME + ) + if should_reload: try: importlib.reload(mod) _log.debug("Reloaded stale module %s", name) diff --git a/koan/app/stagnation_monitor.py b/koan/app/stagnation_monitor.py index 42a5d8b4a..1d3fc5c83 100644 --- a/koan/app/stagnation_monitor.py +++ b/koan/app/stagnation_monitor.py @@ -246,8 +246,8 @@ 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) - with open(path, "w", encoding="utf-8") as f: - json.dump(data, f) + 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. diff --git a/koan/app/utils.py b/koan/app/utils.py index b431ebab0..9d3b91a97 100644 --- a/koan/app/utils.py +++ b/koan/app/utils.py @@ -17,6 +17,7 @@ Backward-compatible re-exports are provided below. """ +import contextlib import fcntl import os import re @@ -248,10 +249,8 @@ def atomic_write(path: Path, content: str): os.fsync(f.fileno()) os.replace(tmp, str(path)) except BaseException: - try: + with contextlib.suppress(OSError): os.unlink(tmp) - except OSError: - pass raise @@ -383,10 +382,8 @@ def _locked_missions_rw(missions_path: Path, transform): os.fsync(f.fileno()) os.replace(tmp, str(missions_path)) except BaseException: - try: + with contextlib.suppress(OSError): os.unlink(tmp) - except OSError: - pass raise finally: fcntl.flock(lock_f, fcntl.LOCK_UN) @@ -394,7 +391,9 @@ def _locked_missions_rw(missions_path: Path, transform): return new_content -def insert_pending_mission(missions_path: Path, entry: str, *, urgent: bool = False): +def insert_pending_mission( + missions_path: Path, entry: str, *, urgent: bool = False, +) -> bool: """Insert a mission entry into the pending section of missions.md. By default, inserts at the bottom of the pending section (FIFO queue). @@ -403,13 +402,24 @@ def insert_pending_mission(missions_path: Path, entry: str, *, urgent: bool = Fa Uses file locking for the entire read-modify-write cycle to prevent TOCTOU race conditions between awake.py and dashboard.py. Creates the file with default structure if it doesn't exist. + + Returns: + True if the mission was inserted, False if it was a duplicate + (same command + URL already pending or in progress). """ - from app.missions import insert_mission + from app.missions import insert_mission, is_duplicate_mission - _locked_missions_rw( - missions_path, - lambda content: insert_mission(content, entry, urgent=urgent), - ) + inserted = True + + def _transform(content: str) -> str: + nonlocal inserted + if is_duplicate_mission(content, entry): + inserted = False + return content + return insert_mission(content, entry, urgent=urgent) + + _locked_missions_rw(missions_path, _transform) + return inserted def modify_missions_file(missions_path: Path, transform): @@ -650,7 +660,7 @@ def resolve_project_path(repo_name: str, owner: Optional[str] = None) -> Optiona config = load_projects_config(str(KOAN_ROOT)) if config: candidates = [] - for pname, project in config.get("projects", {}).items(): + for project in config.get("projects", {}).values(): if not isinstance(project, dict): continue all_urls = [] diff --git a/koan/app/worktree_manager.py b/koan/app/worktree_manager.py index 16172b1ac..f4ce7ebf0 100644 --- a/koan/app/worktree_manager.py +++ b/koan/app/worktree_manager.py @@ -12,6 +12,7 @@ branch named /session-. """ +import contextlib import os import random import shutil @@ -244,15 +245,13 @@ def remove_worktree( shutil.rmtree(str(wt), ignore_errors=True) # Prune any stale worktree references - try: + with contextlib.suppress(subprocess.CalledProcessError): subprocess.run( ["git", "worktree", "prune"], cwd=project_path, capture_output=True, text=True, ) - except subprocess.CalledProcessError: - pass # Delete the branch if it still exists # (only session branches — don't delete user branches) diff --git a/koan/diagnostics/__init__.py b/koan/diagnostics/__init__.py index 8dca2fe2d..edda3039a 100644 --- a/koan/diagnostics/__init__.py +++ b/koan/diagnostics/__init__.py @@ -31,10 +31,11 @@ class CheckResult(NamedTuple): def discover_checks() -> List[str]: """Return sorted list of diagnostic check module names in this package.""" package_dir = Path(__file__).parent - modules = [] - for info in pkgutil.iter_modules([str(package_dir)]): - if not info.ispkg: - modules.append(info.name) + modules = [ + info.name + for info in pkgutil.iter_modules([str(package_dir)]) + if not info.ispkg + ] return sorted(modules) diff --git a/koan/requirements.txt b/koan/requirements.txt index 64d1bcd78..626ac75a9 100644 --- a/koan/requirements.txt +++ b/koan/requirements.txt @@ -4,6 +4,7 @@ pyyaml>=6.0 ruamel.yaml>=0.18 pytest-timeout>=2.1 pytest-cov>=6.0 +pytest-xdist>=3.5 # Optional: Slack provider (install with: pip install slack-sdk) # slack-sdk>=3.27 diff --git a/koan/sanity/__init__.py b/koan/sanity/__init__.py index e543dfb86..d54241ada 100644 --- a/koan/sanity/__init__.py +++ b/koan/sanity/__init__.py @@ -20,10 +20,11 @@ def run(instance_dir: str) -> Tuple[bool, List[str]] def discover_checks() -> List[str]: """Return sorted list of sanity check module names in this package.""" package_dir = Path(__file__).parent - modules = [] - for info in pkgutil.iter_modules([str(package_dir)]): - if not info.ispkg: - modules.append(info.name) + modules = [ + info.name + for info in pkgutil.iter_modules([str(package_dir)]) + if not info.ispkg + ] return sorted(modules) diff --git a/koan/skills/core/audit/audit_runner.py b/koan/skills/core/audit/audit_runner.py index a8da9b67c..6cb72b3a4 100644 --- a/koan/skills/core/audit/audit_runner.py +++ b/koan/skills/core/audit/audit_runner.py @@ -229,49 +229,231 @@ def _build_issue_body(finding: AuditFinding) -> str: return "\n".join(lines) +def _build_advisory_description(finding: AuditFinding) -> str: + """Build a PVRS advisory description from a finding. + + Similar to ``_build_issue_body()`` but formatted for the PVRS description + field (pure markdown, no table metadata — structured fields go in the + JSON payload). + """ + lines = [ + f"## Problem", + f"", + f"{finding.problem}", + f"", + f"## Why This Matters", + f"", + f"{finding.why}", + f"", + f"## Suggested Fix", + f"", + f"{finding.suggested_fix}", + f"", + f"**Location**: `{finding.location}`", + f"**Category**: {finding.category}", + f"", + f"---", + f"\U0001f916 Reported by K\u014dan security audit", + ] + return "\n".join(lines) + + +def _should_use_pvrs(severity: str, threshold: str) -> bool: + """Return True if a finding's severity meets the PVRS routing threshold. + + Findings at or above the threshold severity are routed to PVRS. + E.g., threshold ``"high"`` routes ``critical`` and ``high`` to PVRS. + """ + finding_rank = _SEVERITY_ORDER.get(severity, 99) + threshold_rank = _SEVERITY_ORDER.get(threshold, 1) + return finding_rank <= threshold_rank + + def create_issues( findings: List[AuditFinding], project_path: str, notify_fn=None, + pvrs_mode: str = "auto", + pvrs_threshold: str = "high", ) -> List[str]: - """Create GitHub issues for each finding. + """Create GitHub issues (or PVRS reports) for each finding. + + When PVRS is available and ``pvrs_mode`` is not ``"false"``, findings + at or above ``pvrs_threshold`` severity are submitted as private + vulnerability reports. Lower-severity findings and PVRS failures + fall back to public GitHub issues. + + Args: + findings: List of validated audit findings. + project_path: Local path to the project repository. + notify_fn: Optional callback for progress notifications. + pvrs_mode: ``"auto"`` (detect at runtime), ``"true"`` (force), + or ``"false"`` (always use public issues). + pvrs_threshold: Minimum severity for PVRS routing (default ``"high"``). - Returns a list of issue URLs. + Returns: + List of issue/advisory URLs. """ - from app.github import issue_create, resolve_target_repo + from app.github import ( + check_pvrs_enabled, detect_ecosystem, + resolve_target_repo, + ) target_repo = resolve_target_repo(project_path) + + # Determine PVRS availability + pvrs_available = False + if pvrs_mode == "true": + pvrs_available = True + elif pvrs_mode != "false" and target_repo: + pvrs_available = check_pvrs_enabled(target_repo, cwd=project_path) + + if pvrs_available and notify_fn: + notify_fn( + f" \U0001f512 PVRS enabled — " + f"routing {pvrs_threshold}+ findings privately" + ) + + ecosystem = detect_ecosystem(project_path) if pvrs_available else "other" + # Derive a package name from the project directory + package_name = Path(project_path).name + issue_urls = [] for i, finding in enumerate(findings, 1): title = finding.title - body = _build_issue_body(finding) + use_pvrs = pvrs_available and _should_use_pvrs( + finding.severity, pvrs_threshold, + ) if notify_fn: + channel = "\U0001f512 PVRS" if use_pvrs else "\U0001f4dd issue" notify_fn( - f" \U0001f4dd Creating issue {i}/{len(findings)}: {title}" + f" {channel} {i}/{len(findings)}: {title}" ) try: - url = issue_create( - title=title, - body=body, - repo=target_repo, - cwd=project_path, - ) - url = url.strip() + if use_pvrs: + url = _submit_pvrs_report( + finding, ecosystem, package_name, + target_repo, project_path, + ) + else: + url = _submit_public_issue( + finding, target_repo, project_path, + ) + except Exception as e: + # PVRS fallback: try public issue if PVRS submission failed + if use_pvrs: + print( + f"[audit] PVRS failed for '{title}', " + f"falling back to redacted public issue: {e}", + file=sys.stderr, + ) + if notify_fn: + notify_fn( + f" \u26a0\ufe0f PVRS failed for '{title}', " + f"creating redacted placeholder issue" + ) + try: + url = _submit_redacted_fallback_issue( + finding, target_repo, project_path, + ) + except Exception as e2: + print( + f"[audit] Fallback issue also failed for " + f"'{title}': {e2}", + file=sys.stderr, + ) + continue + else: + print( + f"[audit] Failed to create issue '{title}': {e}", + file=sys.stderr, + ) + continue + + url = url.strip() if url else "" + if url: issue_urls.append(url) - if notify_fn and url: + if notify_fn: notify_fn(f" \U0001f517 {url}") - except Exception as e: - print( - f"[audit] Failed to create issue '{title}': {e}", - file=sys.stderr, - ) return issue_urls +def _submit_pvrs_report( + finding: AuditFinding, + ecosystem: str, + package_name: str, + target_repo: Optional[str], + project_path: str, +) -> str: + """Submit a single finding as a PVRS report. Returns the advisory URL.""" + from app.github import security_advisory_report + + description = _build_advisory_description(finding) + return security_advisory_report( + summary=f"Security: {finding.title}", + description=description, + severity=finding.severity, + ecosystem=ecosystem, + package_name=package_name, + repo=target_repo, + cwd=project_path, + ) + + +def _submit_public_issue( + finding: AuditFinding, + target_repo: Optional[str], + project_path: str, + title_prefix: str = "", +) -> str: + """Create a public GitHub issue for a finding. Returns the issue URL.""" + from app.github import issue_create + + return issue_create( + title=f"{title_prefix}{finding.title}", + body=_build_issue_body(finding), + repo=target_repo, + cwd=project_path, + ) + + +def _submit_redacted_fallback_issue( + finding: AuditFinding, + target_repo: Optional[str], + project_path: str, +) -> str: + """Create a redacted public issue when PVRS submission fails. + + Omits exploit details to avoid leaking vulnerability information publicly. + The issue serves as a placeholder directing maintainers to investigate + via private channels. + """ + from app.github import issue_create + + redacted_body = ( + "A security finding was identified during an automated audit but " + "could not be submitted via Private Vulnerability Reporting (PVRS).\n\n" + f"**Severity**: {finding.severity}\n" + f"**Category**: {finding.category}\n\n" + "Details have been withheld from this public issue to prevent " + "disclosure of exploitable vulnerabilities. Please review the audit " + "logs or contact the security team for full details.\n\n" + "---\n" + "\U0001f916 Created by K\u014dan from audit session" + ) + + return issue_create( + title=f"[Security] {finding.severity} finding — details withheld (PVRS unavailable)", + body=redacted_body, + repo=target_repo, + cwd=project_path, + ) + + # --------------------------------------------------------------------------- # Report saving # --------------------------------------------------------------------------- @@ -302,9 +484,15 @@ def _save_audit_report( for i, finding in enumerate(findings): url = issue_urls[i] if i < len(issue_urls) else "no issue created" + # Annotate channel: PVRS reports have GHSA IDs or advisory URLs + if "/advisories/" in url or url.startswith("GHSA"): + channel = "private" + else: + channel = "" + suffix = f" ({channel})" if channel else "" lines.append( f"- [{finding.severity}] {finding.title} " - f"(`{finding.location}`) — {url}" + f"(`{finding.location}`) — {url}{suffix}" ) lines.append("") @@ -325,6 +513,8 @@ def run_audit( notify_fn=None, skill_dir: Optional[Path] = None, report_name: str = "audit", + pvrs_mode: str = "auto", + pvrs_threshold: str = "high", ) -> Tuple[bool, str]: """Execute a codebase audit on a project. @@ -337,6 +527,8 @@ def run_audit( notify_fn: Optional callback for progress notifications. skill_dir: Optional path to the audit skill directory for prompts. report_name: Base name for the saved report file (default: "audit"). + pvrs_mode: PVRS routing mode (``"auto"``, ``"true"``, ``"false"``). + pvrs_threshold: Minimum severity for PVRS routing (default ``"high"``). Returns: (success, summary) tuple. @@ -384,8 +576,11 @@ def run_audit( f"Creating GitHub issues..." ) - # Step 5: Create GitHub issues - issue_urls = create_issues(findings, project_path, notify_fn=notify_fn) + # Step 5: Create GitHub issues (or PVRS reports for security audits) + issue_urls = create_issues( + findings, project_path, notify_fn=notify_fn, + pvrs_mode=pvrs_mode, pvrs_threshold=pvrs_threshold, + ) # Step 6: Save report report_path = _save_audit_report( diff --git a/koan/skills/core/brainstorm/brainstorm_runner.py b/koan/skills/core/brainstorm/brainstorm_runner.py index 015a16b98..36fe7a037 100644 --- a/koan/skills/core/brainstorm/brainstorm_runner.py +++ b/koan/skills/core/brainstorm/brainstorm_runner.py @@ -12,6 +12,7 @@ --project-path --topic "Improve caching" --tag prompt-caching """ +import contextlib import hashlib import json import re @@ -216,9 +217,10 @@ def _replace_sub_placeholders(created_issues, original_issues, project_path): correct original issue body and to build the SUB-N → #number mapping. """ # Build original_pos → real number mapping (preserves original positions) - ordinal_to_number = {} - for number, _title, _url, original_pos in created_issues: - ordinal_to_number[original_pos] = number + ordinal_to_number = { + original_pos: number + for number, _title, _url, original_pos in created_issues + } for number, _title, _url, original_pos in created_issues: body = original_issues[original_pos - 1]["body"] @@ -346,11 +348,11 @@ def _validate_issue_bodies(issues): body = issue.get("body", "") or "" title = (issue.get("title", "") or "").strip() title_preview = title[:40] if title else "?" - for header in REQUIRED_ISSUE_SECTIONS: - if header not in body: - diagnostics.append( - f"Issue {idx} ('{title_preview}'): missing '{header}'" - ) + diagnostics.extend( + f"Issue {idx} ('{title_preview}'): missing '{header}'" + for header in REQUIRED_ISSUE_SECTIONS + if header not in body + ) return diagnostics @@ -482,16 +484,14 @@ def _coerce_overall_assessment(value): def _ensure_label(tag, project_path): """Create the GitHub label if it doesn't exist.""" - try: + # Label creation failed — issues will be created without it + with contextlib.suppress(RuntimeError, OSError): run_gh( "label", "create", tag, "--description", f"Brainstorm: {tag}", "--force", cwd=project_path, timeout=15, ) - except (RuntimeError, OSError): - # Label creation failed — issues will be created without it - pass def _extract_master_title(topic: str) -> str: diff --git a/koan/skills/core/branches/handler.py b/koan/skills/core/branches/handler.py index 14e9d5689..226949957 100644 --- a/koan/skills/core/branches/handler.py +++ b/koan/skills/core/branches/handler.py @@ -1,5 +1,6 @@ """Koan /branches skill -- list koan branches + open PRs with merge recommendations.""" +import contextlib import json import logging from typing import Dict, List, Optional, Tuple @@ -111,13 +112,11 @@ def _get_branches_info(project_path: str) -> List[Dict]: parts = line.strip().split("\t", 2) if len(parts) == 3: ts_str, relative, ref_name = parts - try: + with contextlib.suppress(ValueError): age_data[ref_name] = { "timestamp": int(ts_str), "age": relative, } - except ValueError: - pass result = [] diff --git a/koan/skills/core/changelog/handler.py b/koan/skills/core/changelog/handler.py index 36bfdcc28..4692e7027 100644 --- a/koan/skills/core/changelog/handler.py +++ b/koan/skills/core/changelog/handler.py @@ -1,5 +1,6 @@ """Koan changelog skill — generate release notes from commits and journals.""" +import contextlib import re import subprocess from collections import defaultdict @@ -105,10 +106,8 @@ def _parse_args(args: str) -> Tuple[str, datetime, str]: for part in parts: if part.startswith("--since="): date_str = part[len("--since="):] - try: + with contextlib.suppress(ValueError): since_date = datetime.strptime(date_str, "%Y-%m-%d") - except ValueError: - pass elif part.startswith("--format="): fmt = part[len("--format="):] if fmt in ("md", "markdown"): @@ -279,8 +278,7 @@ def _format_markdown( if journal_entries: lines.append("### Context (from journal)") lines.append("") - for entry in journal_entries[:10]: - lines.append(f"- {_truncate(entry, 120)}") + lines.extend(f"- {_truncate(entry, 120)}" for entry in journal_entries[:10]) lines.append("") total = sum(len(items) for items in sections.values()) @@ -316,7 +314,6 @@ def _format_telegram( if journal_entries: lines.append("Context:") - for entry in journal_entries[:5]: - lines.append(f" {_truncate(entry, 80)}") + lines.extend(f" {_truncate(entry, 80)}" for entry in journal_entries[:5]) return "\n".join(lines) diff --git a/koan/skills/core/ci_check/handler.py b/koan/skills/core/ci_check/handler.py index 51c4840bd..ba4536f10 100644 --- a/koan/skills/core/ci_check/handler.py +++ b/koan/skills/core/ci_check/handler.py @@ -59,6 +59,11 @@ def handle(ctx): f"this instance. I only run CI checks on my own pull requests." ) - _gh_helpers.queue_github_mission(ctx, "ci_check", pr_url, project_name) + duplicate = _gh_helpers.queue_github_mission_once( + ctx, "ci_check", pr_url, project_name, + type_label="PR", number=pr_number, owner=owner, repo=repo, + ) + if duplicate: + return duplicate return f"\U0001f527 CI check queued for {_gh_helpers.format_success_message('PR', pr_number, owner, repo)}" diff --git a/koan/skills/core/config_check/handler.py b/koan/skills/core/config_check/handler.py index 788beead9..0c21f32d2 100644 --- a/koan/skills/core/config_check/handler.py +++ b/koan/skills/core/config_check/handler.py @@ -39,15 +39,13 @@ def handle(ctx): if missing: lines.append("") lines.append(f"▸ Missing from your config ({len(missing)}):") - for key in missing: - lines.append(f" ➕ {key}") + lines.extend(f" ➕ {key}" for key in missing) lines.append(" ↳ New template keys — see instance.example/config.yaml") if extra: lines.append("") lines.append(f"▸ Extra in your config ({len(extra)}):") - for key in extra: - lines.append(f" ⚠️ {key}") + lines.extend(f" ⚠️ {key}" for key in extra) lines.append(" ↳ May be deprecated or typos") return "\n".join(lines) diff --git a/koan/skills/core/dead_code/dead_code_runner.py b/koan/skills/core/dead_code/dead_code_runner.py index e279e3083..05dfc9446 100644 --- a/koan/skills/core/dead_code/dead_code_runner.py +++ b/koan/skills/core/dead_code/dead_code_runner.py @@ -92,8 +92,7 @@ def _prescan_project(project_path: str) -> str: source_files.sort() if len(source_files) > 200: lines.append(f"(showing first 200 of {len(source_files)})") - for f in source_files[:200]: - lines.append(f"- {f}") + lines.extend(f"- {f}" for f in source_files[:200]) return "\n".join(lines) diff --git a/koan/skills/core/done/handler.py b/koan/skills/core/done/handler.py index 9b6b03c4c..c93fdd003 100644 --- a/koan/skills/core/done/handler.py +++ b/koan/skills/core/done/handler.py @@ -216,12 +216,8 @@ def _format_output(by_project, hours): urls = [] for project in sorted(by_project): data = by_project[project] - for pr in data["merged"]: - if pr.get("url"): - urls.append(pr["url"]) - for pr in data["open"]: - if pr.get("url"): - urls.append(pr["url"]) + urls.extend(pr["url"] for pr in data["merged"] if pr.get("url")) + urls.extend(pr["url"] for pr in data["open"] if pr.get("url")) if urls: lines.append("") diff --git a/koan/skills/core/fix/handler.py b/koan/skills/core/fix/handler.py index 657aa0a89..a065791e3 100644 --- a/koan/skills/core/fix/handler.py +++ b/koan/skills/core/fix/handler.py @@ -173,7 +173,10 @@ def _handle_batch(ctx, args: str, repo_match: Tuple[str, str, str]) -> str: skipped += 1 continue issue_url = issue.get("url") or f"https://github.com/{owner}/{repo}/issues/{issue['number']}" - queue_github_mission(ctx, "fix", issue_url, project_name) + inserted = queue_github_mission(ctx, "fix", issue_url, project_name) + if not inserted: + skipped += 1 + continue queued += 1 limit_note = f" (limited to {limit})" if limit else "" diff --git a/koan/skills/core/gh_request/handler.py b/koan/skills/core/gh_request/handler.py index 4acd9144f..a19c6554a 100644 --- a/koan/skills/core/gh_request/handler.py +++ b/koan/skills/core/gh_request/handler.py @@ -86,7 +86,11 @@ def handle(ctx) -> Optional[str]: if classified_context: mission_parts.append(classified_context) - queue_github_mission(ctx, command, url or "", project_name, classified_context) + inserted = queue_github_mission(ctx, command, url or "", project_name, classified_context) + + if not inserted: + url_info = f" ({url.split('/')[-1]})" if url else "" + return f"\u26a0\ufe0f Duplicate ignored — /{command} already queued or running for {project_name}{url_info}." url_info = f" ({url.split('/')[-1]})" if url else "" return f"/{command} queued for {project_name}{url_info}: {classified_context[:60]}" if classified_context else f"/{command} queued for {project_name}{url_info}" diff --git a/koan/skills/core/gha_audit/handler.py b/koan/skills/core/gha_audit/handler.py index f4e2365d6..efc10a1be 100644 --- a/koan/skills/core/gha_audit/handler.py +++ b/koan/skills/core/gha_audit/handler.py @@ -402,7 +402,6 @@ def _format_report(project, findings, file_count): continue emoji = severity_emoji.get(sev, "") lines.append(f"\n{emoji} **{sev}** ({len(items)})") - for item in items: - lines.append(f" {item.format()}") + lines.extend(f" {item.format()}" for item in items) return "\n".join(lines) diff --git a/koan/skills/core/projects/handler.py b/koan/skills/core/projects/handler.py index 1457a4718..5dbd7c6bb 100644 --- a/koan/skills/core/projects/handler.py +++ b/koan/skills/core/projects/handler.py @@ -36,7 +36,6 @@ def handle(ctx): if warnings: lines.append("") - for w in warnings: - lines.append(w) + lines.extend(warnings) return "\n".join(lines) diff --git a/koan/skills/core/rebase/handler.py b/koan/skills/core/rebase/handler.py index 4641fbb1a..619a40f7d 100644 --- a/koan/skills/core/rebase/handler.py +++ b/koan/skills/core/rebase/handler.py @@ -64,7 +64,12 @@ def handle(ctx): f"this instance. I only rebase my own pull requests." ) - _gh_helpers.queue_github_mission(ctx, "rebase", pr_url, project_name, urgent=urgent) + duplicate = _gh_helpers.queue_github_mission_once( + ctx, "rebase", pr_url, project_name, urgent=urgent, + type_label="PR", number=pr_number, owner=owner, repo=repo, + ) + if duplicate: + return duplicate priority = " (priority)" if urgent else "" return f"Rebase queued{priority} for {_gh_helpers.format_success_message('PR', pr_number, owner, repo)}" diff --git a/koan/skills/core/recreate/handler.py b/koan/skills/core/recreate/handler.py index 4fd756328..8041379c2 100644 --- a/koan/skills/core/recreate/handler.py +++ b/koan/skills/core/recreate/handler.py @@ -5,7 +5,7 @@ extract_github_url, format_project_not_found_error, format_success_message, - queue_github_mission, + queue_github_mission_once, resolve_project_for_repo, ) @@ -50,6 +50,11 @@ def handle(ctx): if not project_path: return format_project_not_found_error(repo, owner=owner) - queue_github_mission(ctx, "recreate", pr_url, project_name) + duplicate = queue_github_mission_once( + ctx, "recreate", pr_url, project_name, + type_label="PR", number=pr_number, owner=owner, repo=repo, + ) + if duplicate: + return duplicate return f"Recreate queued for {format_success_message('PR', pr_number, owner, repo)}" diff --git a/koan/skills/core/review/handler.py b/koan/skills/core/review/handler.py index e7420e028..f327b43fe 100644 --- a/koan/skills/core/review/handler.py +++ b/koan/skills/core/review/handler.py @@ -119,12 +119,14 @@ def _handle_batch(ctx, args: str, repo_match: Tuple[str, str, str]) -> str: if not prs: return f"No open PRs found in {owner}/{repo}." - # Queue a /review mission for each PR + # Queue a /review mission for each PR (skip duplicates) queued = 0 for pr in prs: pr_url = pr.get("url") or f"https://github.com/{owner}/{repo}/pull/{pr['number']}" - queue_github_mission(ctx, "review", pr_url, project_name) - queued += 1 + if queue_github_mission(ctx, "review", pr_url, project_name): + queued += 1 limit_note = f" (limited to {limit})" if limit else "" + if queued == 0: + return f"All PRs from {owner}/{repo} already queued or running{limit_note}." return f"Queued {queued} /review missions for {owner}/{repo}{limit_note}." diff --git a/koan/skills/core/review_rebase/handler.py b/koan/skills/core/review_rebase/handler.py index 571e3f49e..e958ebf02 100644 --- a/koan/skills/core/review_rebase/handler.py +++ b/koan/skills/core/review_rebase/handler.py @@ -48,10 +48,15 @@ def handle(ctx): return format_project_not_found_error(repo, owner=owner) # Queue review first, then rebase — review learnings inform the rebase - queue_github_mission(ctx, "review", pr_url, project_name, context) - queue_github_mission(ctx, "rebase", pr_url, project_name) - - return ( - f"Review + rebase combo queued for " - f"{format_success_message('PR', pr_number, owner, repo)}" - ) + review_ok = queue_github_mission(ctx, "review", pr_url, project_name, context) + rebase_ok = queue_github_mission(ctx, "rebase", pr_url, project_name) + + target = format_success_message('PR', pr_number, owner, repo) + if not review_ok and not rebase_ok: + return f"\u26a0\ufe0f Both /review and /rebase already queued or running for {target}." + if not review_ok: + return f"Rebase queued for {target} (review already queued/running)." + if not rebase_ok: + return f"Review queued for {target} (rebase already queued/running)." + + return f"Review + rebase combo queued for {target}" diff --git a/koan/skills/core/security_audit/security_audit_runner.py b/koan/skills/core/security_audit/security_audit_runner.py index 48cb8e599..fcdd937af 100644 --- a/koan/skills/core/security_audit/security_audit_runner.py +++ b/koan/skills/core/security_audit/security_audit_runner.py @@ -19,6 +19,27 @@ DEFAULT_MAX_ISSUES = 5 +def _load_pvrs_config(project_name: str) -> dict: + """Load PVRS configuration for the project from projects.yaml. + + Returns ``{"pvrs": "auto", "pvrs_threshold": "high"}`` as defaults + if config is unavailable. + """ + import os + try: + koan_root = os.environ.get("KOAN_ROOT", "") + if koan_root: + from app.projects_config import ( + get_project_security_config, load_projects_config, + ) + config = load_projects_config(koan_root) + if config: + return get_project_security_config(config, project_name) + except Exception: + pass + return {"pvrs": "auto", "pvrs_threshold": "high"} + + def run_security_audit( project_path: str, project_name: str, @@ -29,6 +50,10 @@ def run_security_audit( ) -> tuple: """Execute a security audit by delegating to run_audit with our prompt.""" skill_dir = Path(__file__).resolve().parent + + # Load PVRS config for this project + sec_cfg = _load_pvrs_config(project_name) + return run_audit( project_path=project_path, project_name=project_name, @@ -38,6 +63,8 @@ def run_security_audit( notify_fn=notify_fn, skill_dir=skill_dir, report_name="security_audit", + pvrs_mode=sec_cfg["pvrs"], + pvrs_threshold=sec_cfg["pvrs_threshold"], ) diff --git a/koan/skills/core/squash/handler.py b/koan/skills/core/squash/handler.py index b41d0659f..fb3310b3b 100644 --- a/koan/skills/core/squash/handler.py +++ b/koan/skills/core/squash/handler.py @@ -5,7 +5,7 @@ extract_github_url, format_project_not_found_error, format_success_message, - queue_github_mission, + queue_github_mission_once, resolve_project_for_repo, ) @@ -47,6 +47,11 @@ def handle(ctx): if not project_path: return format_project_not_found_error(repo, owner=owner) - queue_github_mission(ctx, "squash", pr_url, project_name) + duplicate = queue_github_mission_once( + ctx, "squash", pr_url, project_name, + type_label="PR", number=pr_number, owner=owner, repo=repo, + ) + if duplicate: + return duplicate return f"Squash queued for {format_success_message('PR', pr_number, owner, repo)}" diff --git a/koan/skills/core/status/handler.py b/koan/skills/core/status/handler.py index cb1126efd..7e8f2f0a7 100644 --- a/koan/skills/core/status/handler.py +++ b/koan/skills/core/status/handler.py @@ -188,12 +188,14 @@ def _handle_status(ctx) -> str: parts.append(f"\n{project}") if in_progress: parts.append(f" In progress: {len(in_progress)}") - for m in in_progress[:2]: - parts.append(f" {_format_mission_display(m)}") + parts.extend( + f" {_format_mission_display(m)}" for m in in_progress[:2] + ) if pending: parts.append(f" Pending: {len(pending)}") - for m in pending[:3]: - parts.append(f" {_format_mission_display(m)}") + parts.extend( + f" {_format_mission_display(m)}" for m in pending[:3] + ) # Health section parts.extend(_build_health_section(koan_root, instance_dir)) @@ -245,8 +247,7 @@ def _build_health_section(koan_root, instance_dir) -> list: if health_items: lines.append("\nHealth") - for item in health_items: - lines.append(f" {item}") + lines.extend(f" {item}" for item in health_items) except Exception: pass return lines diff --git a/koan/tests/conftest.py b/koan/tests/conftest.py index b9852143e..a31e256e7 100644 --- a/koan/tests/conftest.py +++ b/koan/tests/conftest.py @@ -1,11 +1,95 @@ """Shared fixtures for koan tests.""" import os +import shutil +import tempfile +from contextlib import ExitStack from pathlib import Path +from unittest.mock import patch import pytest +# --- per-worker KOAN_ROOT isolation (must run before any app.* import) --- +# Many app modules (utils.py, awake.py, …) snapshot the KOAN_ROOT env var at +# import time. When pytest-xdist spins up multiple workers in the same process +# group, they all inherit the same KOAN_ROOT and start racing on shared files +# under that directory (missions.md, .koan-* state, journal/, …). +# +# Give each xdist worker its own KOAN_ROOT before any koan module is imported. +# Wipe any leftover state from a prior run on the same worker name — otherwise +# missions.md fragments, .koan-* state, journal entries etc. carry over and +# can reintroduce exactly the cross-run pollution this fixture prevents. +# Tests that override KOAN_ROOT via monkeypatch or tmp_path remain unaffected; +# tests that rely on the ambient KOAN_ROOT now see a worker-private directory. +_xdist_worker = os.environ.get("PYTEST_XDIST_WORKER") +if _xdist_worker and _xdist_worker != "master": + _per_worker_root = Path(tempfile.gettempdir()) / f"test-koan-{_xdist_worker}" + shutil.rmtree(_per_worker_root, ignore_errors=True) + (_per_worker_root / "instance").mkdir(parents=True, exist_ok=True) + os.environ["KOAN_ROOT"] = str(_per_worker_root) + + +@pytest.fixture(autouse=True) +def _reset_run_module_state(): + """Reset module-level mission flags in `app.run` before each test. + + `_maybe_retry_mission` short-circuits on `_last_mission_timed_out`, + `_last_mission_aborted`, or `_last_mission_stagnated`. Several test + files (e.g. test_run.py) leave these flags set; under pytest-xdist + that pollution leaks into whatever test runs next on the same worker. + Resetting globally keeps every test starting from a clean state. + """ + try: + import app.run as run_mod + run_mod._last_mission_timed_out = False + run_mod._last_mission_aborted = False + run_mod._last_mission_stagnated.clear() + except Exception: + pass + yield + + +@pytest.fixture(autouse=True) +def _mock_resolve_pr_location(): + """Bypass the gh CLI lookup at the start of run_rebase/run_recreate/run_review/run_squash. + + These pipelines call ``resolve_pr_location()`` at Step 0 to verify the + PR exists at the given owner/repo (and probe other remotes if not). The + helper shells out to ``gh pr view``, which: + + * requires ``gh`` to be installed and authenticated in the test env, and + * becomes flaky under pytest-xdist when many workers shell out to ``gh`` + concurrently (auth contention, rate limiting). + + Tests that pass placeholder owners/repos like ``("o", "r", "1", "/p")`` + don't care about this resolution step — they only exercise the code + *after* the PR location is known. Make the lookup a no-op everywhere so + the test outcome doesn't depend on the host ``gh`` install or on xdist + scheduling. + + Tests for ``resolve_pr_location()`` itself live in test_claude_step.py + and use ``app.claude_step.resolve_pr_location`` directly — patching only + the call-site bindings here leaves those unaffected. + """ + targets = ( + "app.recreate_pr.resolve_pr_location", + "app.rebase_pr.resolve_pr_location", + "app.review_runner.resolve_pr_location", + "app.squash_pr.resolve_pr_location", + ) + passthrough = lambda owner, repo, pr_number, project_path: (owner, repo) # noqa: E731 + with ExitStack() as stack: + for target in targets: + try: + stack.enter_context(patch(target, side_effect=passthrough)) + except (AttributeError, ModuleNotFoundError): + # Module not importable in this test run (e.g. minimal sys.path); + # nothing to patch. + continue + yield + + @pytest.fixture(autouse=True) def isolate_env(monkeypatch): """Ensure tests don't touch real instance/ or send real Telegram messages.""" diff --git a/koan/tests/test_ci_queue_runner.py b/koan/tests/test_ci_queue_runner.py index 3099bd915..04dd92a33 100644 --- a/koan/tests/test_ci_queue_runner.py +++ b/koan/tests/test_ci_queue_runner.py @@ -3,6 +3,8 @@ import json from unittest.mock import MagicMock, patch +from app.claude_step import CI_STATUS_BLOCKED_APPROVAL + import pytest @@ -409,3 +411,335 @@ def test_reenqueue_called_on_pending_ci(self): ) mock_modify.assert_called_once() + + +class TestAggregateCiRuns: + """Aggregation rules for `gh run list` output — especially skip-conclusion handling.""" + + def test_empty_input_returns_none(self): + from app.claude_step import aggregate_ci_runs + + assert aggregate_ci_runs([]) == ("none", None) + + def test_all_success_returns_success(self): + from app.claude_step import aggregate_ci_runs + + runs = [ + {"databaseId": 1, "status": "completed", "conclusion": "success"}, + {"databaseId": 2, "status": "completed", "conclusion": "success"}, + ] + assert aggregate_ci_runs(runs) == ("success", 1) + + def test_failure_wins_over_pending(self): + """A failed completed run takes priority over an in-progress one.""" + from app.claude_step import aggregate_ci_runs + + runs = [ + {"databaseId": 1, "status": "in_progress", "conclusion": ""}, + {"databaseId": 2, "status": "completed", "conclusion": "failure"}, + {"databaseId": 3, "status": "completed", "conclusion": "success"}, + ] + status, run_id = aggregate_ci_runs(runs) + assert status == "failure" + assert run_id == 2 + + def test_pending_returned_when_no_completed_failures(self): + from app.claude_step import aggregate_ci_runs + + runs = [ + {"databaseId": 1, "status": "completed", "conclusion": "success"}, + {"databaseId": 2, "status": "in_progress", "conclusion": ""}, + ] + status, run_id = aggregate_ci_runs(runs) + assert status == "pending" + assert run_id == 2 + + def test_dependabot_auto_merge_skip_is_ignored(self): + """Regression: a 'Dependabot auto-merge' workflow that completes with + conclusion='skipped' on a non-Dependabot PR must not be reported as a + CI failure. See aio-libs/yarl PR #1681 — Kōan kept queueing /ci_check + fix missions because `gh run list --limit 1` returned the skipped + Dependabot run instead of the actual CI workflows. + """ + from app.claude_step import aggregate_ci_runs + + # This mirrors the actual `gh run list` payload for the yarl PR: + # the Dependabot auto-merge run lands first by databaseId order, but + # the real CI workflows are all green. + runs = [ + { + "databaseId": 25970779376, + "status": "completed", + "conclusion": "skipped", + "workflowName": "Dependabot auto-merge", + }, + { + "databaseId": 25970779403, + "status": "completed", + "conclusion": "success", + "workflowName": "CodeQL", + }, + { + "databaseId": 25970779406, + "status": "completed", + "conclusion": "success", + "workflowName": "Aiohttp", + }, + ] + status, run_id = aggregate_ci_runs(runs) + assert status == "success" + # The reported run_id must point at a real CI workflow, never the + # skipped Dependabot run — otherwise log fetching would target the + # wrong run and report no failures. + assert run_id != 25970779376 + + def test_dependabot_skip_with_pending_real_ci_returns_pending(self): + """If only the Dependabot run completed (skipped) and real CI is still + running, surface pending — not failure, not success. + """ + from app.claude_step import aggregate_ci_runs + + runs = [ + { + "databaseId": 25970779376, + "status": "completed", + "conclusion": "skipped", + "workflowName": "Dependabot auto-merge", + }, + { + "databaseId": 25970779458, + "status": "in_progress", + "conclusion": "", + "workflowName": "CI/CD", + }, + ] + status, run_id = aggregate_ci_runs(runs) + assert status == "pending" + assert run_id == 25970779458 + + def test_cancelled_and_neutral_also_ignored(self): + """`cancelled`, `neutral`, `action_required` are not real CI failures.""" + from app.claude_step import aggregate_ci_runs + + runs = [ + {"databaseId": 1, "status": "completed", "conclusion": "cancelled"}, + {"databaseId": 2, "status": "completed", "conclusion": "neutral"}, + {"databaseId": 3, "status": "completed", "conclusion": "action_required"}, + {"databaseId": 4, "status": "completed", "conclusion": "success"}, + ] + assert aggregate_ci_runs(runs) == ("success", 4) + + def test_all_skipped_returns_none(self): + """When every workflow run was filtered out, we have no CI signal.""" + from app.claude_step import aggregate_ci_runs + + runs = [ + {"databaseId": 1, "status": "completed", "conclusion": "skipped"}, + {"databaseId": 2, "status": "completed", "conclusion": "cancelled"}, + ] + assert aggregate_ci_runs(runs) == ("none", None) + + def test_missing_conclusion_field_treated_as_pending(self): + from app.claude_step import aggregate_ci_runs + + runs = [ + {"databaseId": 1, "status": "queued"}, + ] + status, run_id = aggregate_ci_runs(runs) + assert status == "pending" + assert run_id == 1 + + def test_action_required_status_returns_blocked_approval(self): + """Workflow runs gated on maintainer approval (fork PR from a + first-time contributor) come back with status='action_required' + and no conclusion. They must surface as blocked_approval so + callers stop retrying — pushing more commits won't unstick them. + See https://github.com/aio-libs/aiohttp/pull/12553 — Kōan retried + the same PR multiple times while every workflow run sat waiting + for an approve click. + """ + from app.claude_step import aggregate_ci_runs + + runs = [ + {"databaseId": 10, "status": "action_required", "conclusion": None}, + ] + status, run_id = aggregate_ci_runs(runs) + assert status == CI_STATUS_BLOCKED_APPROVAL + assert run_id == 10 + + def test_waiting_status_returns_blocked_approval(self): + """`waiting` status signals an environment-protection gate — also + a "human must click" state that Kōan can't move past. + """ + from app.claude_step import aggregate_ci_runs + + runs = [ + {"databaseId": 11, "status": "waiting", "conclusion": None}, + ] + status, run_id = aggregate_ci_runs(runs) + assert status == CI_STATUS_BLOCKED_APPROVAL + assert run_id == 11 + + def test_failure_wins_over_blocked_approval(self): + """If one workflow is genuinely failing and another is blocked on + approval, prioritise the failure: that one CAN still be fixed by + pushing new commits. + """ + from app.claude_step import aggregate_ci_runs + + runs = [ + {"databaseId": 1, "status": "action_required", "conclusion": None}, + {"databaseId": 2, "status": "completed", "conclusion": "failure"}, + ] + status, run_id = aggregate_ci_runs(runs) + assert status == "failure" + assert run_id == 2 + + def test_blocked_approval_wins_over_pending(self): + """A blocked run alongside an in-progress one should still surface + as blocked — the in-progress run is a coincidence, the gate is the + actionable state for the human. + """ + from app.claude_step import aggregate_ci_runs + + runs = [ + {"databaseId": 1, "status": "in_progress", "conclusion": ""}, + {"databaseId": 2, "status": "action_required", "conclusion": None}, + ] + status, run_id = aggregate_ci_runs(runs) + assert status == CI_STATUS_BLOCKED_APPROVAL + assert run_id == 2 + + +class TestDrainOneBlockedApproval: + """drain_one must remove a PR from ## CI when its workflows are + blocked on maintainer approval, instead of polling forever. + """ + + PR_URL = "https://github.com/owner/repo/pull/42" + + def _missions_with_ci_entry(self): + return ( + "# Missions\n\n## CI\n\n" + f"- [project:proj] {self.PR_URL} branch:fix-branch repo:owner/repo" + f" queued:2026-04-01T10:00 (attempt 0/5)\n\n" + "## Pending\n\n## Done\n" + ) + + def test_blocked_approval_removes_entry_and_notifies(self): + from app.ci_queue_runner import drain_one + + with ( + patch("pathlib.Path.exists", return_value=True), + patch("pathlib.Path.read_text", return_value=self._missions_with_ci_entry()), + patch("app.ci_queue_runner._maybe_migrate_json_queue"), + patch("app.utils.modify_missions_file") as mock_modify, + patch( + "app.ci_queue_runner.check_ci_status", + return_value=(CI_STATUS_BLOCKED_APPROVAL, 999), + ), + patch("app.ci_queue_runner._write_outbox") as mock_outbox, + patch("app.ci_queue_runner._inject_ci_fix_mission") as mock_inject, + ): + result = drain_one("/tmp/instance") + + assert result is not None + assert "approval" in result.lower() + mock_modify.assert_called() + mock_outbox.assert_called_once() + # Outbox message should reference the PR so the human can act + assert self.PR_URL in mock_outbox.call_args[0][1] + assert "approval" in mock_outbox.call_args[0][1].lower() + # No fix mission should be queued — Kōan can't unstick it + mock_inject.assert_not_called() + + +class TestRunCiCheckBlockedApproval: + """run_ci_check_and_fix must bail out, not attempt fixes, when CI is + gated on maintainer approval. + """ + + PR_URL = "https://github.com/owner/repo/pull/42" + PROJECT_PATH = "/tmp/test-project" + + def test_blocked_approval_returns_early_without_fix(self): + from app.ci_queue_runner import run_ci_check_and_fix + + fake_context = {"branch": "fix-branch", "base": "main"} + with ( + patch("app.rebase_pr.fetch_pr_context", return_value=fake_context), + patch( + "app.ci_queue_runner.check_ci_status", + return_value=(CI_STATUS_BLOCKED_APPROVAL, 123), + ), + patch("app.ci_queue_runner._attempt_ci_fixes") as mock_fix, + ): + success, summary = run_ci_check_and_fix(self.PR_URL, self.PROJECT_PATH) + + assert success is False + assert "approval" in summary.lower() + # The pipeline must not attempt Claude-based fixes + mock_fix.assert_not_called() + + +class TestCheckCiStatusDependabot: + """End-to-end: check_ci_status must not treat skipped Dependabot runs as failures.""" + + def test_dependabot_skip_does_not_trigger_failure(self): + """Regression for aio-libs/yarl PR #1681 — Kōan repeatedly queued + /ci_check fix missions because check_ci_status returned ('failure', + ) for a healthy PR. + """ + from app.ci_queue_runner import check_ci_status + + gh_payload = json.dumps([ + { + "databaseId": 25970779376, + "status": "completed", + "conclusion": "skipped", + "workflowName": "Dependabot auto-merge", + }, + { + "databaseId": 25970779403, + "status": "completed", + "conclusion": "success", + "workflowName": "CodeQL", + }, + ]) + with patch("app.claude_step.run_gh", return_value=gh_payload): + status, run_id = check_ci_status("koan/fix-issue-1680", "aio-libs/yarl") + + assert status == "success" + assert run_id == 25970779403 + + def test_check_existing_ci_dependabot_skip_does_not_fetch_logs(self): + """The other single-shot caller (`check_existing_ci`) must also ignore + the skipped Dependabot run, otherwise we'd waste an `_fetch_failed_logs` + call on a workflow that produced no logs. + """ + from app.claude_step import check_existing_ci + + gh_payload = json.dumps([ + { + "databaseId": 25970779376, + "status": "completed", + "conclusion": "skipped", + "workflowName": "Dependabot auto-merge", + }, + { + "databaseId": 25970779403, + "status": "completed", + "conclusion": "success", + "workflowName": "CodeQL", + }, + ]) + with ( + patch("app.claude_step.run_gh", return_value=gh_payload), + patch("app.claude_step._fetch_failed_logs") as mock_fetch_logs, + ): + status, run_id, logs = check_existing_ci("br", "owner/repo") + + assert status == "success" + assert run_id == 25970779403 + assert logs == "" + mock_fetch_logs.assert_not_called() diff --git a/koan/tests/test_claude_step.py b/koan/tests/test_claude_step.py index a34807c34..6a55e9c12 100644 --- a/koan/tests/test_claude_step.py +++ b/koan/tests/test_claude_step.py @@ -12,6 +12,7 @@ from app.claude_step import ( StepResult, _is_ancestor, + _prefetch_all_remotes, _rebase_onto_target, _run_git, commit_if_changes, @@ -142,19 +143,22 @@ class TestRebaseOntoTarget: def test_origin_success(self, mock_git): result = _rebase_onto_target("main", "/project") assert result == "origin" - assert mock_git.call_count == 2 mock_git.assert_any_call( ["git", "fetch", "origin", "+refs/heads/main:refs/remotes/origin/main"], cwd="/project", timeout=60, ) + mock_git.assert_any_call( + ["git", "fetch", "upstream", "+refs/heads/main:refs/remotes/upstream/main"], + cwd="/project", timeout=60, + ) @patch("app.cli_exec.subprocess.run") @patch("app.claude_step._run_git") def test_origin_fails_upstream_succeeds(self, mock_git, mock_subprocess): def side_effect(cmd, **kwargs): - if "origin" in cmd: - raise RuntimeError("fetch failed") - return MagicMock(returncode=0, stdout="ok") + if "rebase" in cmd and any("origin" in a for a in cmd): + raise RuntimeError("rebase failed") + return "" mock_git.side_effect = side_effect result = _rebase_onto_target("main", "/project") @@ -170,17 +174,12 @@ def test_both_fail_returns_none(self, mock_git, mock_subprocess): @patch("app.cli_exec.subprocess.run") @patch("app.claude_step._run_git") def test_rebase_abort_called_on_failure(self, mock_git, mock_subprocess): - call_count = 0 - def selective_fail(*args, **kwargs): - nonlocal call_count - call_count += 1 - # Odd calls are fetch (succeed), even calls are rebase (fail) - if call_count % 2 == 0: + def selective_fail(cmd, **kwargs): + if "rebase" in cmd: raise RuntimeError("conflict") return "" mock_git.side_effect = selective_fail _rebase_onto_target("main", "/project") - # Should call rebase --abort for each failed remote abort_calls = [ c for c in mock_subprocess.call_args_list @@ -192,11 +191,8 @@ def selective_fail(*args, **kwargs): @patch("app.claude_step._run_git") def test_rebase_abort_called_with_timeout(self, mock_git, mock_subprocess): """git rebase --abort must have a timeout to prevent hangs in cleanup.""" - call_count = 0 - def selective_fail(*args, **kwargs): - nonlocal call_count - call_count += 1 - if call_count % 2 == 0: + def selective_fail(cmd, **kwargs): + if "rebase" in cmd: raise RuntimeError("conflict") return "" mock_git.side_effect = selective_fail @@ -214,11 +210,8 @@ def selective_fail(*args, **kwargs): @patch("app.claude_step._run_git") def test_timeout_caught_and_logged(self, mock_git, mock_subprocess, capsys): """TimeoutExpired should be caught (not just Exception) and logged.""" - call_count = 0 - def selective_fail(*args, **kwargs): - nonlocal call_count - call_count += 1 - if call_count % 2 == 0: + def selective_fail(cmd, **kwargs): + if "rebase" in cmd: raise subprocess.TimeoutExpired("git", 60) return "" mock_git.side_effect = selective_fail @@ -232,11 +225,8 @@ def selective_fail(*args, **kwargs): @patch("app.claude_step._run_git") def test_os_error_caught_and_logged(self, mock_git, mock_subprocess, capsys): """OSError (e.g. git not found) should be caught and logged.""" - call_count = 0 - def selective_fail(*args, **kwargs): - nonlocal call_count - call_count += 1 - if call_count % 2 == 0: + def selective_fail(cmd, **kwargs): + if "rebase" in cmd: raise OSError("No such file or directory: 'git'") return "" mock_git.side_effect = selective_fail @@ -352,81 +342,235 @@ def side_effect(cmd, **kwargs): assert "--onto" not in rebase_cmd +# ---------- _prefetch_all_remotes ---------- + + +class TestPrefetchAllRemotes: + """Tests for _prefetch_all_remotes — eager base branch sync.""" + + @patch("app.claude_step._run_git") + def test_fetches_origin_and_upstream(self, mock_git): + _prefetch_all_remotes("main", "/project") + assert mock_git.call_count == 2 + mock_git.assert_any_call( + ["git", "fetch", "origin", "+refs/heads/main:refs/remotes/origin/main"], + cwd="/project", timeout=60, + ) + mock_git.assert_any_call( + ["git", "fetch", "upstream", "+refs/heads/main:refs/remotes/upstream/main"], + cwd="/project", timeout=60, + ) + + @patch("app.claude_step._run_git") + def test_includes_head_remote(self, mock_git): + _prefetch_all_remotes("main", "/project", head_remote="myfork") + fetched = [c[0][0][2] for c in mock_git.call_args_list] + assert "myfork" in fetched + assert "origin" in fetched + assert "upstream" in fetched + + @patch("app.claude_step._run_git") + def test_preferred_remote_first(self, mock_git): + _prefetch_all_remotes("main", "/project", preferred_remote="upstream") + first_call_remote = mock_git.call_args_list[0][0][0][2] + assert first_call_remote == "upstream" + + @patch("app.claude_step._run_git") + def test_no_duplicate_when_head_in_ordered(self, mock_git): + _prefetch_all_remotes("main", "/project", head_remote="origin") + assert mock_git.call_count == 2 + + @patch("app.claude_step._run_git") + def test_failure_is_nonfatal(self, mock_git, capsys): + mock_git.side_effect = RuntimeError("network down") + _prefetch_all_remotes("main", "/project") + captured = capsys.readouterr() + assert "Pre-fetch" in captured.err + assert "non-fatal" in captured.err + + @patch("app.claude_step._run_git") + def test_timeout_is_nonfatal(self, mock_git, capsys): + mock_git.side_effect = subprocess.TimeoutExpired("git", 60) + _prefetch_all_remotes("main", "/project") + captured = capsys.readouterr() + assert "Pre-fetch" in captured.err + + + # ---------- run_claude ---------- +class _FakeStream: + """Iterable + closable stand-in for ``proc.stdout`` / ``proc.stderr``. + + Tests need a file-like object that supports both ``for line in stream`` + iteration and ``stream.close()`` — a bare ``iter([])`` does not. + """ + + def __init__(self, lines=None, read_text=""): + self._lines = list(lines or []) + self._read_text = read_text + + def __iter__(self): + return iter(self._lines) + + def read(self): + return self._read_text + + def close(self): + return None + + +def _fake_proc(stdout_lines, stderr_text="", returncode=0, pid=99999): + """Build a fake Popen object for streaming tests. + + ``stdout_lines`` is a list of full lines (each entry should already + contain a trailing newline if needed). ``proc.stdout`` becomes an + iterable so the streaming loop in ``run_claude`` can consume it. + """ + proc = MagicMock() + proc.stdout = _FakeStream(lines=stdout_lines) + proc.stderr = _FakeStream(read_text=stderr_text) + proc.returncode = returncode + proc.pid = pid + proc.wait.return_value = returncode + return proc + + class TestRunClaude: - """Tests for run_claude — CLI invocation wrapper.""" + """Tests for run_claude — streams stdout, captures full output.""" - @patch("app.cli_exec.subprocess.run") - def test_success(self, mock_run): - mock_run.return_value = MagicMock( - returncode=0, stdout=" done \n", stderr="" - ) + @patch("app.claude_step.popen_cli") + def test_success(self, mock_popen): + proc = _fake_proc([" done \n"], stderr_text="", returncode=0) + mock_popen.return_value = (proc, lambda: None) result = run_claude(["claude", "-p", "test"], "/project") assert result["success"] is True assert result["output"] == "done" assert result["error"] == "" - @patch("app.cli_exec.subprocess.run") - def test_failure_with_stderr(self, mock_run): - mock_run.return_value = MagicMock( - returncode=1, stdout="partial", stderr="something broke" + @patch("app.claude_step.popen_cli") + def test_failure_with_stderr(self, mock_popen): + proc = _fake_proc( + ["partial\n"], stderr_text="something broke", returncode=1, ) + mock_popen.return_value = (proc, lambda: None) result = run_claude(["claude", "-p", "test"], "/project") assert result["success"] is False assert "Exit code 1" in result["error"] assert "something broke" in result["error"] - @patch("app.cli_exec.subprocess.run") - def test_failure_no_stderr(self, mock_run): - mock_run.return_value = MagicMock( - returncode=1, stdout="", stderr="" - ) + @patch("app.claude_step.popen_cli") + def test_failure_no_stderr(self, mock_popen): + proc = _fake_proc([], stderr_text="", returncode=1) + mock_popen.return_value = (proc, lambda: None) result = run_claude(["claude", "-p", "test"], "/project") assert result["success"] is False assert "no stderr" in result["error"] - @patch("app.cli_exec.subprocess.run") - def test_failure_no_stderr_includes_stdout(self, mock_run): + @patch("app.claude_step.popen_cli") + def test_failure_no_stderr_includes_stdout(self, mock_popen): """When stderr is empty but stdout has content, error includes stdout.""" - mock_run.return_value = MagicMock( + proc = _fake_proc( + ["Error: context window exceeded\n"], + stderr_text="", returncode=1, - stdout="Error: context window exceeded", - stderr="", ) + mock_popen.return_value = (proc, lambda: None) result = run_claude(["claude", "-p", "test"], "/project") assert result["success"] is False assert "no stderr" in result["error"] assert "stdout:" in result["error"] assert "context window exceeded" in result["error"] - @patch("app.cli_exec.subprocess.run") - def test_timeout(self, mock_run): - mock_run.side_effect = subprocess.TimeoutExpired(cmd="claude", timeout=600) - result = run_claude(["claude", "-p", "test"], "/project") - assert result["success"] is False - assert "Timeout" in result["error"] - assert "600" in result["error"] + @patch("app.claude_step.popen_cli") + def test_timeout_kills_process_group(self, mock_popen): + """When the watchdog fires, run_claude returns a Timeout error. - @patch("app.cli_exec.subprocess.run") - def test_custom_timeout(self, mock_run): - mock_run.return_value = MagicMock(returncode=0, stdout="ok", stderr="") - run_claude(["claude", "-p", "test"], "/project", timeout=120) - call_kwargs = mock_run.call_args[1] - assert call_kwargs["timeout"] == 120 - assert call_kwargs["cwd"] == "/project" + Simulates a hanging child by blocking stdout iteration until the + watchdog thread invokes the kill callback. The kill is monkey- + patched to set the unblock event, mirroring what os.killpg would + do in production (cause the child to exit and stdout to EOF). + """ + import os + import threading - @patch("app.cli_exec.subprocess.run") - def test_long_stderr_truncated(self, mock_run): - long_err = "E" * 1000 - mock_run.return_value = MagicMock( - returncode=1, stdout="", stderr=long_err + killed = threading.Event() + + class _BlockingStream: + def __iter__(self): + killed.wait(timeout=10) + return iter([]) + + def read(self): + return "" + + def close(self): + return None + + proc = MagicMock() + proc.stdout = _BlockingStream() + proc.stderr = _FakeStream(read_text="") + proc.returncode = -9 + proc.pid = 12345 + proc.wait.return_value = -9 + mock_popen.return_value = (proc, lambda: None) + + # Use a tiny timeout so the watchdog fires within the test. + with patch("os.killpg", side_effect=lambda *a, **kw: killed.set()): + with patch.object(os, "getpgid", return_value=12345): + result = run_claude( + ["claude", "-p", "test"], "/project", timeout=1, + ) + + assert result["success"] is False + assert "Timeout" in result["error"] + assert "1" in result["error"] + + @patch("app.claude_step.popen_cli") + def test_streams_stdout_lines(self, mock_popen, capsys): + """Each Claude stdout line must be forwarded to parent stdout + so the run.py liveness watchdog resets on every line.""" + proc = _fake_proc( + ["thinking...\n", "calling tool\n", "done\n"], + stderr_text="", + returncode=0, ) + mock_popen.return_value = (proc, lambda: None) + run_claude(["claude", "-p", "test"], "/project") + captured = capsys.readouterr() + assert "thinking..." in captured.out + assert "calling tool" in captured.out + assert "done" in captured.out + + @patch("app.claude_step.popen_cli") + def test_uses_new_session_for_process_group_kill(self, mock_popen): + """popen must request a new POSIX session so the whole process + group can be killed on timeout — preventing grandchildren from + holding the stdout pipe open and hanging the drain.""" + proc = _fake_proc(["ok\n"], returncode=0) + mock_popen.return_value = (proc, lambda: None) + run_claude(["claude", "-p", "test"], "/project") + call_kwargs = mock_popen.call_args.kwargs + assert call_kwargs.get("start_new_session") is True + + @patch("app.claude_step.popen_cli") + def test_long_stderr_truncated(self, mock_popen): + long_err = "E" * 1000 + proc = _fake_proc([], stderr_text=long_err, returncode=1) + mock_popen.return_value = (proc, lambda: None) result = run_claude(["claude", "-p", "test"], "/project") # Should only keep last 500 chars of stderr assert len(result["error"]) < 600 + @patch("app.claude_step.popen_cli") + def test_cleanup_called_on_success(self, mock_popen): + proc = _fake_proc(["ok\n"], returncode=0) + cleanup = MagicMock() + mock_popen.return_value = (proc, cleanup) + run_claude(["claude", "-p", "test"], "/project") + cleanup.assert_called_once() + # ---------- commit_if_changes ---------- diff --git a/koan/tests/test_cli_exec.py b/koan/tests/test_cli_exec.py index c88b0e55b..319b9e3eb 100644 --- a/koan/tests/test_cli_exec.py +++ b/koan/tests/test_cli_exec.py @@ -12,6 +12,7 @@ prepare_prompt_file, run_cli, popen_cli, + stream_with_timeout, _cleanup_prompt_file, ) @@ -313,3 +314,127 @@ def test_copilot_keeps_prompt_in_args(self, mock_popen, _mock_provider): assert actual_cmd == ["copilot", "-p", "my prompt"] assert mock_popen.call_args[1]["stdin"] == subprocess.DEVNULL cleanup() + + +# --------------------------------------------------------------------------- +# stream_with_timeout +# --------------------------------------------------------------------------- + + +class _FakeStream: + def __init__(self, lines=None, read_text=""): + self._lines = list(lines or []) + self._read_text = read_text + self.closed = False + + def __iter__(self): + return iter(self._lines) + + def read(self): + return self._read_text + + def close(self): + self.closed = True + + +def _fake_proc(stdout_lines, stderr_text="", returncode=0, pid=99999): + proc = MagicMock() + proc.stdout = _FakeStream(lines=stdout_lines) + proc.stderr = _FakeStream(read_text=stderr_text) + proc.returncode = returncode + proc.pid = pid + proc.wait.return_value = returncode + return proc + + +class TestStreamWithTimeout: + """Tests for stream_with_timeout — shared streaming + watchdog helper.""" + + def test_collects_stdout_lines(self): + proc = _fake_proc(["a\n", "b\n", "c\n"], returncode=0) + result = stream_with_timeout(proc, timeout=10) + assert result.stdout == "a\nb\nc" + assert result.stderr == "" + assert result.timed_out is False + + def test_forwards_each_line_to_callback(self): + proc = _fake_proc(["one\n", "two\n", "three\n"], returncode=0) + seen = [] + stream_with_timeout(proc, timeout=10, on_line=seen.append) + assert seen == ["one", "two", "three"] + + def test_drains_stderr(self): + proc = _fake_proc(["ok\n"], stderr_text="oops", returncode=1) + result = stream_with_timeout(proc, timeout=10) + assert result.stderr == "oops" + assert result.timed_out is False + + def test_closes_streams(self): + proc = _fake_proc(["ok\n"], returncode=0) + stream_with_timeout(proc, timeout=10) + assert proc.stdout.closed is True + assert proc.stderr.closed is True + + def test_timeout_kills_process_group(self): + """When the watchdog fires it must SIGKILL the whole process group.""" + import threading + + killed = threading.Event() + + class _BlockingStream: + def __iter__(self): + killed.wait(timeout=10) + return iter([]) + + def read(self): + return "" + + def close(self): + return None + + proc = MagicMock() + proc.stdout = _BlockingStream() + proc.stderr = _FakeStream(read_text="") + proc.returncode = -9 + proc.pid = 12345 + proc.wait.return_value = -9 + + with patch("app.cli_exec.os.killpg", + side_effect=lambda *a, **kw: killed.set()) as killpg, \ + patch("app.cli_exec.os.getpgid", return_value=12345): + result = stream_with_timeout(proc, timeout=0.5) + + assert result.timed_out is True + killpg.assert_called_once() + + def test_completed_flag_blocks_watchdog_race(self): + """If the watchdog Timer fires after stream EOF but before + ``watchdog.cancel()``, the kill must be skipped and ``timed_out`` + must stay False — otherwise a clean completion gets reported as + a timeout.""" + from app.cli_exec import stream_with_timeout as swt + + proc = _fake_proc(["done\n"], returncode=0) + + with patch("app.cli_exec.threading.Timer") as TimerMock: + timer_instance = MagicMock() + captured = {} + + def factory(timeout, fn): + captured["fn"] = fn + return timer_instance + + TimerMock.side_effect = factory + + with patch("app.cli_exec.os.killpg") as killpg: + # Simulate the race: invoke the watchdog callback after + # stream consumption but before cancel() returns. + def fire_after_stream(): + captured["fn"]() + return None + timer_instance.cancel.side_effect = fire_after_stream + + result = swt(proc, timeout=10) + + killpg.assert_not_called() + assert result.timed_out is False diff --git a/koan/tests/test_git_prep.py b/koan/tests/test_git_prep.py index 570787263..d7fd764de 100644 --- a/koan/tests/test_git_prep.py +++ b/koan/tests/test_git_prep.py @@ -4,6 +4,8 @@ from unittest.mock import patch, call from app.git_prep import ( + _fetch_branch_refspec, + _sync_secondary_remotes, get_upstream_remote, prepare_project_branch, PrepResult, @@ -134,6 +136,125 @@ def test_ls_remote_no_ref_line(self): assert result == "main" +# --- _fetch_branch_refspec --- + + +class TestFetchBranchRefspec: + """Tests for explicit-refspec fetch helper.""" + + def test_success_returns_true(self): + with patch("app.git_prep.run_git", return_value=(0, "", "")): + assert _fetch_branch_refspec("origin", "main", "/proj") is True + + def test_failure_returns_false(self): + with patch("app.git_prep.run_git", return_value=(1, "", "error")): + assert _fetch_branch_refspec("origin", "main", "/proj") is False + + def test_uses_explicit_refspec(self): + with patch("app.git_prep.run_git", return_value=(0, "", "")) as mock_git: + _fetch_branch_refspec("upstream", "master", "/proj") + mock_git.assert_called_once_with( + "fetch", "upstream", + "+refs/heads/master:refs/remotes/upstream/master", + cwd="/proj", timeout=15, + ) + + def test_custom_timeout(self): + with patch("app.git_prep.run_git", return_value=(0, "", "")) as mock_git: + _fetch_branch_refspec("origin", "main", "/proj", timeout=30) + assert mock_git.call_args[1]["timeout"] == 30 + + +# --- _sync_secondary_remotes --- + + +class TestSyncSecondaryRemotes: + """Tests for multi-remote base branch sync.""" + + def test_fetches_non_primary_remotes(self): + """Fetches base branch from all remotes except the primary.""" + def side_effect(*args, **kwargs): + if args[0] == "remote": + return (0, "origin\nupstream\nmyfork", "") + if args[0] == "fetch": + return (0, "", "") + return (1, "", "") + + with patch("app.git_prep.run_git", side_effect=side_effect) as mock_git: + _sync_secondary_remotes("main", "upstream", "/proj") + + fetch_calls = [ + c for c in mock_git.call_args_list + if c[0][0] == "fetch" + ] + fetched_remotes = [c[0][1] for c in fetch_calls] + assert "origin" in fetched_remotes + assert "myfork" in fetched_remotes + assert "upstream" not in fetched_remotes + + def test_skips_primary_remote(self): + """Primary remote is excluded from secondary fetch.""" + def side_effect(*args, **kwargs): + if args[0] == "remote": + return (0, "origin\nupstream", "") + return (0, "", "") + + with patch("app.git_prep.run_git", side_effect=side_effect) as mock_git: + _sync_secondary_remotes("main", "origin", "/proj") + + fetch_calls = [c for c in mock_git.call_args_list if c[0][0] == "fetch"] + assert len(fetch_calls) == 1 + assert fetch_calls[0][0][1] == "upstream" + + def test_no_remotes_listed(self): + """git remote failure returns early — no fetches attempted.""" + with patch("app.git_prep.run_git", return_value=(1, "", "err")) as mock_git: + _sync_secondary_remotes("main", "origin", "/proj") + + fetch_calls = [c for c in mock_git.call_args_list if c[0][0] == "fetch"] + assert len(fetch_calls) == 0 + + def test_single_remote_no_secondary(self): + """Only one remote (same as primary) — nothing to fetch.""" + def side_effect(*args, **kwargs): + if args[0] == "remote": + return (0, "origin", "") + return (0, "", "") + + with patch("app.git_prep.run_git", side_effect=side_effect) as mock_git: + _sync_secondary_remotes("main", "origin", "/proj") + + fetch_calls = [c for c in mock_git.call_args_list if c[0][0] == "fetch"] + assert len(fetch_calls) == 0 + + def test_secondary_fetch_failure_nonfatal(self): + """Failed secondary fetch is logged, not raised.""" + def side_effect(*args, **kwargs): + if args[0] == "remote": + return (0, "origin\nbroken-remote", "") + if args[0] == "fetch": + return (1, "", "network error") + return (0, "", "") + + with patch("app.git_prep.run_git", side_effect=side_effect): + _sync_secondary_remotes("main", "origin", "/proj") + + def test_uses_explicit_refspec(self): + """Secondary fetches use explicit refspec for reliable ref updates.""" + def side_effect(*args, **kwargs): + if args[0] == "remote": + return (0, "origin\nupstream", "") + return (0, "", "") + + with patch("app.git_prep.run_git", side_effect=side_effect) as mock_git: + _sync_secondary_remotes("main", "origin", "/proj") + + fetch_calls = [c for c in mock_git.call_args_list if c[0][0] == "fetch"] + assert len(fetch_calls) == 1 + refspec = fetch_calls[0][0][2] + assert refspec == "+refs/heads/main:refs/remotes/upstream/main" + + # --- PrepResult --- @@ -802,6 +923,66 @@ def side_effect(*args, **kwargs): assert "stash" not in calls +class TestPrepareProjectBranchSecondarySync: + """Verify prepare_project_branch syncs secondary remotes.""" + + def test_secondary_sync_called_on_success(self): + """_sync_secondary_remotes is called after a successful primary sync.""" + side_effect = _make_run_git_side_effect() + with patch("app.git_prep.run_git", side_effect=side_effect), \ + patch("app.git_prep.load_projects_config", return_value=None), \ + patch("app.git_prep.get_project_submit_to_repository", return_value={}), \ + patch("app.git_prep.get_project_auto_merge", return_value={"base_branch": "main"}), \ + patch("app.git_prep._sync_secondary_remotes") as mock_sync: + result = prepare_project_branch("/proj", "myproj", "/koan") + + assert result.success is True + mock_sync.assert_called_once_with("main", "origin", "/proj") + + def test_secondary_sync_not_called_on_failure(self): + """_sync_secondary_remotes is NOT called when primary sync fails.""" + side_effect = _make_run_git_side_effect({ + "fetch": (1, "", "Could not resolve host"), + }) + with patch("app.git_prep.run_git", side_effect=side_effect), \ + patch("app.git_prep.load_projects_config", return_value=None), \ + patch("app.git_prep.get_project_submit_to_repository", return_value={}), \ + patch("app.git_prep.get_project_auto_merge", return_value={"base_branch": "main"}), \ + patch("app.git_prep._sync_secondary_remotes") as mock_sync: + result = prepare_project_branch("/proj", "myproj", "/koan") + + assert result.success is False + mock_sync.assert_not_called() + + def test_secondary_sync_uses_correct_remote(self): + """When upstream is primary, secondary sync receives 'upstream'.""" + def side_effect(*args, **kwargs): + cmd = args[0] if args else "" + if cmd == "rev-parse": + return (0, "feature", "") + if cmd == "remote": + return (0, "git@github.com:upstream/repo.git", "") + if cmd == "fetch": + return (0, "", "") + if cmd == "status": + return (0, "", "") + if cmd == "checkout": + return (0, "", "") + if cmd == "merge": + return (0, "", "") + return (0, "", "") + + with patch("app.git_prep.run_git", side_effect=side_effect), \ + patch("app.git_prep.load_projects_config", return_value=None), \ + patch("app.git_prep.get_project_submit_to_repository", return_value={}), \ + patch("app.git_prep.get_project_auto_merge", return_value={"base_branch": "main"}), \ + patch("app.git_prep._sync_secondary_remotes") as mock_sync: + result = prepare_project_branch("/proj", "myproj", "/koan") + + assert result.success is True + mock_sync.assert_called_once_with("main", "upstream", "/proj") + + # --- Integration: _run_iteration calls git prep --- diff --git a/koan/tests/test_github_command_handler.py b/koan/tests/test_github_command_handler.py index 25d4a6faf..353ecd2f7 100644 --- a/koan/tests/test_github_command_handler.py +++ b/koan/tests/test_github_command_handler.py @@ -43,6 +43,37 @@ # --------------------------------------------------------------------------- +@pytest.fixture +def subject_closed_state(): + """Per-test override hook for `_is_subject_closed`'s stubbed return value. + + Defaults to `None` (subject treated as open). Tests that need to + exercise the closed-subject branch should override this fixture in + their module/class scope and return ``"merged"`` or ``"closed"``. + Making the default explicit (rather than hidden inside an autouse + stub) lets future test authors see the seam without reading the + fixture body. + """ + return None + + +@pytest.fixture(autouse=True) +def _stub_is_subject_closed(subject_closed_state): + """Stub the network-hitting `_is_subject_closed` helper. + + Without this, `_is_subject_closed` calls the real GitHub API, which + makes tests network-flaky and unsafe to run in parallel. The return + value is sourced from the `subject_closed_state` fixture so tests + that need a non-default answer can override it without dropping back + to manual `@patch` wiring. + """ + with patch( + "app.github_command_handler._is_subject_closed", + return_value=subject_closed_state, + ): + yield + + @pytest.fixture def mock_skill(): """A github-enabled skill.""" diff --git a/koan/tests/test_github_subscribe.py b/koan/tests/test_github_subscribe.py index fb888b81d..252d5e74f 100644 --- a/koan/tests/test_github_subscribe.py +++ b/koan/tests/test_github_subscribe.py @@ -17,6 +17,32 @@ pytestmark = pytest.mark.slow +@pytest.fixture +def subject_closed_state(): + """Per-test override hook for `_is_subject_closed`'s stubbed return value. + + Defaults to `None` (subject treated as open). Tests exercising the + closed-subject branch should override this fixture and return + ``"merged"`` or ``"closed"``. + """ + return None + + +@pytest.fixture(autouse=True) +def _stub_is_subject_closed(subject_closed_state): + """Stub the network-hitting `_is_subject_closed` helper. + + Return value is sourced from the `subject_closed_state` fixture so + tests that need a non-default answer can override it instead of + falling back to manual `@patch` wiring. + """ + with patch( + "app.github_command_handler._is_subject_closed", + return_value=subject_closed_state, + ): + yield + + @pytest.fixture def mock_skill(): return Skill( diff --git a/koan/tests/test_matrix_provider.py b/koan/tests/test_matrix_provider.py new file mode 100644 index 000000000..c5ab589ae --- /dev/null +++ b/koan/tests/test_matrix_provider.py @@ -0,0 +1,337 @@ +"""Tests for MatrixProvider — config, send, poll, sync cursor handling.""" + +from unittest.mock import patch, MagicMock + +import pytest +import requests + + +@pytest.fixture +def provider(): + """Create a pre-configured MatrixProvider.""" + from app.messaging.matrix import MatrixProvider + p = MatrixProvider() + p._homeserver = "https://matrix.example" + p._access_token = "syt_token" + p._user_id = "@koan:matrix.example" + p._room_id = "!room:matrix.example" + return p + + +# --------------------------------------------------------------------------- +# Configuration +# --------------------------------------------------------------------------- + + +class TestConfigure: + def _set_all(self, monkeypatch): + monkeypatch.setenv("KOAN_MATRIX_HOMESERVER", "https://matrix.example") + monkeypatch.setenv("KOAN_MATRIX_ACCESS_TOKEN", "syt_token") + monkeypatch.setenv("KOAN_MATRIX_USER_ID", "@koan:matrix.example") + monkeypatch.setenv("KOAN_MATRIX_ROOM_ID", "!room:matrix.example") + + @patch("app.utils.load_dotenv") + def test_valid_credentials(self, mock_dotenv, monkeypatch): + self._set_all(monkeypatch) + from app.messaging.matrix import MatrixProvider + p = MatrixProvider() + assert p.configure() is True + assert p._homeserver == "https://matrix.example" + assert p._access_token == "syt_token" + assert p._user_id == "@koan:matrix.example" + assert p._room_id == "!room:matrix.example" + + @patch("app.utils.load_dotenv") + def test_trailing_slash_stripped(self, mock_dotenv, monkeypatch): + self._set_all(monkeypatch) + monkeypatch.setenv("KOAN_MATRIX_HOMESERVER", "https://matrix.example/") + from app.messaging.matrix import MatrixProvider + p = MatrixProvider() + assert p.configure() is True + assert p._homeserver == "https://matrix.example" + + @pytest.mark.parametrize("var", [ + "KOAN_MATRIX_HOMESERVER", + "KOAN_MATRIX_ACCESS_TOKEN", + "KOAN_MATRIX_USER_ID", + "KOAN_MATRIX_ROOM_ID", + ]) + @patch("app.utils.load_dotenv") + def test_missing_var_fails(self, mock_dotenv, monkeypatch, var): + self._set_all(monkeypatch) + monkeypatch.delenv(var, raising=False) + from app.messaging.matrix import MatrixProvider + p = MatrixProvider() + assert p.configure() is False + + @patch("app.utils.load_dotenv") + def test_invalid_homeserver_scheme(self, mock_dotenv, monkeypatch): + self._set_all(monkeypatch) + monkeypatch.setenv("KOAN_MATRIX_HOMESERVER", "matrix.example") + from app.messaging.matrix import MatrixProvider + p = MatrixProvider() + assert p.configure() is False + + +# --------------------------------------------------------------------------- +# Getters +# --------------------------------------------------------------------------- + + +class TestGetters: + def test_provider_name(self, provider): + assert provider.get_provider_name() == "matrix" + + def test_channel_id(self, provider): + assert provider.get_channel_id() == "!room:matrix.example" + + +# --------------------------------------------------------------------------- +# send_message +# --------------------------------------------------------------------------- + + +class TestSendMessage: + @patch("app.messaging.matrix.requests.put") + def test_short_message(self, mock_put, provider): + mock_put.return_value = MagicMock(status_code=200) + assert provider.send_message("hello") is True + assert mock_put.call_count == 1 + call = mock_put.call_args + assert call[1]["json"]["body"] == "hello" + assert call[1]["json"]["msgtype"] == "m.text" + assert call[1]["headers"]["Authorization"] == "Bearer syt_token" + + @patch("app.messaging.matrix.requests.put") + def test_long_message_chunked(self, mock_put, provider): + mock_put.return_value = MagicMock(status_code=200) + assert provider.send_message("x" * 8500) is True + assert mock_put.call_count == 3 # 4000 + 4000 + 500 + + @patch("app.messaging.matrix.requests.put") + def test_url_contains_url_encoded_room_id(self, mock_put, provider): + mock_put.return_value = MagicMock(status_code=200) + provider.send_message("hi") + url = mock_put.call_args[0][0] + # ! and : must be percent-encoded in the path segment + assert "%21room%3Amatrix.example" in url + assert "/send/m.room.message/" in url + + @patch("app.messaging.matrix.requests.put") + def test_4xx_returns_false(self, mock_put, provider): + mock_put.return_value = MagicMock(status_code=403, text="forbidden") + assert provider.send_message("hi") is False + + @patch("app.messaging.matrix.time.sleep") + @patch("app.messaging.matrix.requests.put") + def test_5xx_retries_then_fails(self, mock_put, mock_sleep, provider): + # 5xx raises RequestException → retried 3 times → final failure + mock_put.return_value = MagicMock(status_code=502, text="bad gateway") + assert provider.send_message("hi") is False + assert mock_put.call_count == 3 + + def test_not_configured(self): + from app.messaging.matrix import MatrixProvider + p = MatrixProvider() + assert p.send_message("test") is False + + def test_empty_message_noop(self, provider): + # Empty messages don't hit the API, just succeed (matches Telegram behavior). + with patch("app.messaging.matrix.requests.put") as mock_put: + assert provider.send_message("") is True + assert mock_put.call_count == 0 + + +# --------------------------------------------------------------------------- +# poll_updates / sync +# --------------------------------------------------------------------------- + + +class TestPollUpdates: + @patch("app.messaging.matrix.requests.get") + def test_initial_sync_discards_events(self, mock_get, provider): + """First sync only records next_batch — historical events are ignored.""" + mock_get.return_value = MagicMock(json=lambda: { + "next_batch": "s100", + "rooms": {"join": {"!room:matrix.example": { + "timeline": {"events": [ + {"type": "m.room.message", "sender": "@alice:matrix.example", + "content": {"msgtype": "m.text", "body": "old msg"}}, + ]} + }}} + }) + updates = provider.poll_updates() + assert updates == [] + assert provider._sync_token == "s100" + assert provider._sync_initialized is True + + @patch("app.messaging.matrix.requests.get") + def test_subsequent_sync_returns_messages(self, mock_get, provider): + provider._sync_token = "s100" + provider._sync_initialized = True + mock_get.return_value = MagicMock(json=lambda: { + "next_batch": "s101", + "rooms": {"join": {"!room:matrix.example": { + "timeline": {"events": [ + {"type": "m.room.message", "sender": "@alice:matrix.example", + "content": {"msgtype": "m.text", "body": "hello bot"}, + "origin_server_ts": 123}, + ]} + }}} + }) + updates = provider.poll_updates() + assert len(updates) == 1 + assert updates[0].message.text == "hello bot" + assert updates[0].message.role == "user" + assert provider._sync_token == "s101" + + @patch("app.messaging.matrix.requests.get") + def test_filters_own_messages(self, mock_get, provider): + provider._sync_token = "s100" + provider._sync_initialized = True + mock_get.return_value = MagicMock(json=lambda: { + "next_batch": "s101", + "rooms": {"join": {"!room:matrix.example": { + "timeline": {"events": [ + {"type": "m.room.message", "sender": "@koan:matrix.example", + "content": {"msgtype": "m.text", "body": "self"}}, + {"type": "m.room.message", "sender": "@alice:matrix.example", + "content": {"msgtype": "m.text", "body": "from alice"}}, + ]} + }}} + }) + updates = provider.poll_updates() + assert len(updates) == 1 + assert updates[0].message.text == "from alice" + + @patch("app.messaging.matrix.requests.get") + def test_filters_non_text_messages(self, mock_get, provider): + provider._sync_token = "s100" + provider._sync_initialized = True + mock_get.return_value = MagicMock(json=lambda: { + "next_batch": "s101", + "rooms": {"join": {"!room:matrix.example": { + "timeline": {"events": [ + {"type": "m.room.message", "sender": "@alice:matrix.example", + "content": {"msgtype": "m.image", "body": "photo.png"}}, + {"type": "m.room.member", "sender": "@bob:matrix.example", + "content": {"membership": "join"}}, + {"type": "m.room.message", "sender": "@alice:matrix.example", + "content": {"msgtype": "m.text", "body": "real msg"}}, + ]} + }}} + }) + updates = provider.poll_updates() + assert len(updates) == 1 + assert updates[0].message.text == "real msg" + + @patch("app.messaging.matrix.requests.get") + def test_ignores_events_from_other_rooms(self, mock_get, provider): + provider._sync_token = "s100" + provider._sync_initialized = True + mock_get.return_value = MagicMock(json=lambda: { + "next_batch": "s101", + "rooms": {"join": {"!other:matrix.example": { + "timeline": {"events": [ + {"type": "m.room.message", "sender": "@alice:matrix.example", + "content": {"msgtype": "m.text", "body": "wrong room"}}, + ]} + }}} + }) + updates = provider.poll_updates() + assert updates == [] + + @patch("app.messaging.matrix.requests.get") + def test_network_error_returns_empty(self, mock_get, provider): + mock_get.side_effect = requests.RequestException("boom") + assert provider.poll_updates() == [] + + @patch("app.messaging.matrix.requests.get") + def test_passes_since_token_after_init(self, mock_get, provider): + provider._sync_token = "s100" + provider._sync_initialized = True + mock_get.return_value = MagicMock(json=lambda: {"next_batch": "s101"}) + provider.poll_updates() + assert mock_get.call_args[1]["params"]["since"] == "s100" + + @patch("app.messaging.matrix.requests.get") + def test_initial_sync_uses_zero_timeout(self, mock_get, provider): + mock_get.return_value = MagicMock(json=lambda: {"next_batch": "s100"}) + provider.poll_updates() + assert mock_get.call_args[1]["params"]["timeout"] == 0 + assert "since" not in mock_get.call_args[1]["params"] + + def test_no_token_returns_empty(self): + from app.messaging.matrix import MatrixProvider + p = MatrixProvider() + assert p.poll_updates() == [] + + +# --------------------------------------------------------------------------- +# send_typing +# --------------------------------------------------------------------------- + + +class TestSendTyping: + @patch("app.messaging.matrix.requests.put") + def test_send_typing(self, mock_put, provider): + mock_put.return_value = MagicMock(status_code=200) + assert provider.send_typing() is True + url = mock_put.call_args[0][0] + assert "/typing/" in url + assert mock_put.call_args[1]["json"]["typing"] is True + + def test_send_typing_not_configured(self): + from app.messaging.matrix import MatrixProvider + p = MatrixProvider() + assert p.send_typing() is False + + @patch("app.messaging.matrix.requests.put") + def test_send_typing_network_error(self, mock_put, provider): + mock_put.side_effect = requests.RequestException("boom") + assert provider.send_typing() is False + + +# --------------------------------------------------------------------------- +# Registry +# --------------------------------------------------------------------------- + + +class TestRegistry: + def test_matrix_registered(self): + """Matrix should auto-register when the messaging package loads providers. + + Runs in a fresh subprocess: ``_providers`` is process-wide module + state that other tests (notably ``clean_registry`` in + ``test_messaging_provider.py``) can clear *after* the provider + modules are already cached in ``sys.modules``. Once that happens + no in-process call to ``_ensure_providers_loaded`` can repopulate + the registry — the decorators won't re-run for cached modules. + A clean subprocess sidesteps the whole ordering problem. + """ + import os + import subprocess + import sys + from pathlib import Path + + koan_pkg = Path(__file__).resolve().parents[1] # …/koan + script = ( + "from app.messaging import _ensure_providers_loaded, _providers\n" + "_ensure_providers_loaded()\n" + "assert 'matrix' in _providers, sorted(_providers)\n" + ) + env = { + **os.environ, + "PYTHONPATH": str(koan_pkg), + "KOAN_ROOT": os.environ.get("KOAN_ROOT", "/tmp/test-koan"), + } + result = subprocess.run( + [sys.executable, "-c", script], + capture_output=True, + text=True, + timeout=15, + env=env, + ) + assert result.returncode == 0, ( + f"subprocess failed:\nstdout={result.stdout}\nstderr={result.stderr}" + ) diff --git a/koan/tests/test_memory_manager.py b/koan/tests/test_memory_manager.py index 0c4c5ecd0..5147a39ee 100644 --- a/koan/tests/test_memory_manager.py +++ b/koan/tests/test_memory_manager.py @@ -1,9 +1,11 @@ """Tests for memory_manager.py — scoped summary, compaction, learnings dedup, journal archival.""" -import pytest +import contextlib from datetime import date, timedelta from unittest.mock import patch +import pytest + from app.memory_manager import ( MemoryManager, parse_summary_sessions, @@ -799,6 +801,31 @@ def test_skips_when_below_threshold(self, tmp_path): stats = compact_learnings(str(tmp_path), "koan", max_lines=100) assert stats["skipped"] is True + def test_no_subprocess_when_below_threshold(self, tmp_path): + """_get_file_tree (git subprocess) must not run when below threshold.""" + self._write_learnings(tmp_path, "koan", "# Learnings\n\n- fact 1\n- fact 2\n") + with patch("app.memory_manager.MemoryManager._get_file_tree") as mock_tree: + stats = compact_learnings(str(tmp_path), "koan", max_lines=100) + assert stats["skipped"] is True + mock_tree.assert_not_called() + + def test_no_subprocess_when_hash_unchanged(self, tmp_path): + """_get_file_tree must not run on a repeat call with unchanged content.""" + lines = ["# Learnings", ""] + for i in range(150): + lines.append(f"- fact {i}") + self._write_learnings(tmp_path, "koan", "\n".join(lines)) + + compacted_output = "- merged fact A\n- merged fact B\n" + with patch("app.memory_manager.MemoryManager._run_compaction_cli", return_value=compacted_output): + compact_learnings(str(tmp_path), "koan", max_lines=100) + + # Second call: content now below threshold — subprocess must not run + with patch("app.memory_manager.MemoryManager._get_file_tree") as mock_tree: + stats2 = compact_learnings(str(tmp_path), "koan", max_lines=100) + assert stats2["skipped"] is True + mock_tree.assert_not_called() + def test_skips_when_hash_unchanged(self, tmp_path): """Second call with same content is skipped via hash check.""" lines = ["# Learnings", ""] @@ -1263,10 +1290,8 @@ def test_scoped_summary_command(self, tmp_path): out = StringIO() with patch.object(sys, "argv", ["memory_manager", str(tmp_path), "scoped-summary", "koan"]): with patch("sys.stdout", out): - try: + with contextlib.suppress(SystemExit): run_module("app.memory_manager", run_name="__main__") - except SystemExit: - pass assert "koan work" in out.getvalue() assert "other work" not in out.getvalue() @@ -1298,10 +1323,8 @@ def test_compact_command(self, tmp_path): out = StringIO() with patch.object(sys, "argv", ["memory_manager", str(tmp_path), "compact", "5"]): with patch("sys.stdout", out): - try: + with contextlib.suppress(SystemExit): run_module("app.memory_manager", run_name="__main__") - except SystemExit: - pass assert "Compacted: 6 sessions removed" in out.getvalue() def test_compact_default_max(self, tmp_path): @@ -1318,10 +1341,8 @@ def test_compact_default_max(self, tmp_path): out = StringIO() with patch.object(sys, "argv", ["memory_manager", str(tmp_path), "compact"]): with patch("sys.stdout", out): - try: + with contextlib.suppress(SystemExit): run_module("app.memory_manager", run_name="__main__") - except SystemExit: - pass assert "Compacted: 0 sessions removed" in out.getvalue() def test_cleanup_learnings_command(self, tmp_path): @@ -1338,10 +1359,8 @@ def test_cleanup_learnings_command(self, tmp_path): out = StringIO() with patch.object(sys, "argv", ["memory_manager", str(tmp_path), "cleanup-learnings", "koan"]): with patch("sys.stdout", out): - try: + with contextlib.suppress(SystemExit): run_module("app.memory_manager", run_name="__main__") - except SystemExit: - pass assert "Deduped: 1 lines removed" in out.getvalue() def test_cleanup_learnings_no_project_exits_1(self, tmp_path): @@ -1371,10 +1390,8 @@ def test_archive_journals_command(self, tmp_path): out = StringIO() with patch.object(sys, "argv", ["memory_manager", str(tmp_path), "archive-journals"]): with patch("sys.stdout", out): - try: + with contextlib.suppress(SystemExit): run_module("app.memory_manager", run_name="__main__") - except SystemExit: - pass output = out.getvalue() assert "archived_days" in output @@ -1390,10 +1407,8 @@ def test_archive_journals_custom_days(self, tmp_path): out = StringIO() with patch.object(sys, "argv", ["memory_manager", str(tmp_path), "archive-journals", "7"]): with patch("sys.stdout", out): - try: + with contextlib.suppress(SystemExit): run_module("app.memory_manager", run_name="__main__") - except SystemExit: - pass output = out.getvalue() assert "archived_days" in output @@ -1411,10 +1426,8 @@ def test_cleanup_command(self, tmp_path): out = StringIO() with patch.object(sys, "argv", ["memory_manager", str(tmp_path), "cleanup"]): with patch("sys.stdout", out): - try: + with contextlib.suppress(SystemExit): run_module("app.memory_manager", run_name="__main__") - except SystemExit: - pass output = out.getvalue() assert "summary_compacted" in output @@ -1435,9 +1448,7 @@ def test_cleanup_custom_max_sessions(self, tmp_path): out = StringIO() with patch.object(sys, "argv", ["memory_manager", str(tmp_path), "cleanup", "5"]): with patch("sys.stdout", out): - try: + with contextlib.suppress(SystemExit): run_module("app.memory_manager", run_name="__main__") - except SystemExit: - pass output = out.getvalue() assert "summary_compacted" in output diff --git a/koan/tests/test_messaging_provider.py b/koan/tests/test_messaging_provider.py index 3300e021c..c414ca48b 100644 --- a/koan/tests/test_messaging_provider.py +++ b/koan/tests/test_messaging_provider.py @@ -212,6 +212,98 @@ def configure(self): get_messaging_provider(provider_name_override="bad") +# --------------------------------------------------------------------------- +# _ensure_providers_loaded — order independence & idempotency +# --------------------------------------------------------------------------- + + +class TestEnsureProvidersLoaded: + """Regression: ``_ensure_providers_loaded`` must load every module in + ``_PROVIDER_MODULES`` even when ``_providers`` is already populated by + a prior partial import. + + Previously the loader short-circuited as soon as ``_providers`` was + non-empty. That was a latent production bug: any process that + imported the default ``telegram`` provider at startup (the normal + path) could never resolve ``matrix`` or ``slack`` afterwards. It + also caused ``test_matrix_registered`` to flap under xdist depending + on which sibling test happened to import ``telegram`` first. + """ + + def test_loads_matrix_when_telegram_imported_first(self): + """Run in a fresh subprocess so Python's import cache cannot + bypass the @register_provider decorators (the cache makes this + scenario untestable in-process — once telegram is imported in + the test runner, re-importing it is a no-op even if _providers + was cleared by a fixture).""" + import subprocess + import sys + from pathlib import Path + + koan_pkg = Path(__file__).resolve().parents[1] # …/koan + script = ( + "import app.messaging.telegram # noqa: F401\n" + "from app.messaging import _ensure_providers_loaded, _providers\n" + "assert sorted(_providers) == ['telegram'], sorted(_providers)\n" + "_ensure_providers_loaded()\n" + "missing = {'telegram', 'slack', 'matrix'} - set(_providers)\n" + "assert not missing, f'missing providers after load: {missing}'\n" + ) + env = { + **os.environ, + "PYTHONPATH": str(koan_pkg), + # Provider modules require a writable KOAN_ROOT at import. + "KOAN_ROOT": os.environ.get("KOAN_ROOT", "/tmp/test-koan"), + } + result = subprocess.run( + [sys.executable, "-c", script], + capture_output=True, + text=True, + timeout=15, + env=env, + ) + assert result.returncode == 0, ( + f"subprocess failed:\nstdout={result.stdout}\nstderr={result.stderr}" + ) + + def test_idempotent_when_called_repeatedly(self): + """Calling the loader N times must converge to the same registry. + + Runs in a fresh subprocess so the in-process import cache and any + ``clean_registry`` mutations from sibling tests cannot mask + repeat-call drift. + """ + import subprocess + import sys + from pathlib import Path + + koan_pkg = Path(__file__).resolve().parents[1] + script = ( + "from app.messaging import _ensure_providers_loaded, _providers\n" + "_ensure_providers_loaded()\n" + "snapshot = dict(_providers)\n" + "_ensure_providers_loaded()\n" + "_ensure_providers_loaded()\n" + "assert dict(_providers) == snapshot, " + "f'registry drifted: {snapshot} -> {dict(_providers)}'\n" + ) + env = { + **os.environ, + "PYTHONPATH": str(koan_pkg), + "KOAN_ROOT": os.environ.get("KOAN_ROOT", "/tmp/test-koan"), + } + result = subprocess.run( + [sys.executable, "-c", script], + capture_output=True, + text=True, + timeout=15, + env=env, + ) + assert result.returncode == 0, ( + f"subprocess failed:\nstdout={result.stdout}\nstderr={result.stderr}" + ) + + # --------------------------------------------------------------------------- # Provider name resolution # --------------------------------------------------------------------------- diff --git a/koan/tests/test_mission_retry.py b/koan/tests/test_mission_retry.py index b321dd6a7..e702ba4a6 100644 --- a/koan/tests/test_mission_retry.py +++ b/koan/tests/test_mission_retry.py @@ -1,5 +1,6 @@ """Tests for mission retry logic in app.run — _maybe_retry_mission and _get_git_head.""" +import contextlib import os import subprocess import tempfile @@ -53,10 +54,8 @@ def temp_output_files(): os.close(fd_err) yield stdout_file, stderr_file for f in (stdout_file, stderr_file): - try: + with contextlib.suppress(OSError): os.unlink(f) - except OSError: - pass class TestMaybeRetryMission: diff --git a/koan/tests/test_missions.py b/koan/tests/test_missions.py index ce22e05ac..50ddfdf24 100644 --- a/koan/tests/test_missions.py +++ b/koan/tests/test_missions.py @@ -2906,3 +2906,106 @@ def test_cap_enforced_on_write(self, tmp_path): assert "old entry 0" not in content # New entry present assert "new entry" in content + + +# --- Duplicate detection --- + +from app.missions import is_duplicate_mission, _extract_mission_signature + + +class TestExtractMissionSignature: + def test_rebase_url(self): + line = "- [project:koan] /rebase https://github.com/owner/repo/pull/123 📬" + assert _extract_mission_signature(line) == "rebase:https://github.com/owner/repo/pull/123" + + def test_review_url(self): + line = "- [project:koan] /review https://github.com/owner/repo/pull/42" + assert _extract_mission_signature(line) == "review:https://github.com/owner/repo/pull/42" + + def test_ci_check_url(self): + line = "- [project:foo] /ci_check https://github.com/org/foo/pull/99" + assert _extract_mission_signature(line) == "ci_check:https://github.com/org/foo/pull/99" + + def test_recreate_url(self): + line = "/recreate https://github.com/owner/repo/pull/7" + assert _extract_mission_signature(line) == "recreate:https://github.com/owner/repo/pull/7" + + def test_non_github_mission_returns_none(self): + line = "- [project:koan] Fix the login bug" + assert _extract_mission_signature(line) is None + + def test_unknown_command_returns_none(self): + line = "- /deploy https://github.com/owner/repo/pull/1" + assert _extract_mission_signature(line) is None + + def test_strips_trailing_paren(self): + line = "/review https://github.com/o/r/pull/5)" + assert _extract_mission_signature(line) == "review:https://github.com/o/r/pull/5" + + +class TestIsDuplicateMission: + def test_duplicate_in_pending(self): + content = ( + "# Missions\n\n" + "## Pending\n\n" + "- [project:koan] /rebase https://github.com/owner/repo/pull/10 ⏳(2026-05-16T10:00)\n\n" + "## In Progress\n\n" + "## Done\n" + ) + new_entry = "- [project:koan] /rebase https://github.com/owner/repo/pull/10" + assert is_duplicate_mission(content, new_entry) is True + + def test_duplicate_in_progress(self): + content = ( + "# Missions\n\n" + "## Pending\n\n" + "## In Progress\n\n" + "- [project:koan] /review https://github.com/owner/repo/pull/5 ⏳(2026-05-16T09:00) ▶(2026-05-16T09:01)\n\n" + "## Done\n" + ) + new_entry = "- [project:koan] /review https://github.com/owner/repo/pull/5" + assert is_duplicate_mission(content, new_entry) is True + + def test_not_duplicate_different_pr(self): + content = ( + "# Missions\n\n" + "## Pending\n\n" + "- [project:koan] /rebase https://github.com/owner/repo/pull/10\n\n" + "## In Progress\n\n" + "## Done\n" + ) + new_entry = "- [project:koan] /rebase https://github.com/owner/repo/pull/11" + assert is_duplicate_mission(content, new_entry) is False + + def test_not_duplicate_different_command(self): + content = ( + "# Missions\n\n" + "## Pending\n\n" + "- [project:koan] /review https://github.com/owner/repo/pull/10\n\n" + "## In Progress\n\n" + "## Done\n" + ) + new_entry = "- [project:koan] /rebase https://github.com/owner/repo/pull/10" + assert is_duplicate_mission(content, new_entry) is False + + def test_non_github_mission_never_duplicate(self): + content = ( + "# Missions\n\n" + "## Pending\n\n" + "- [project:koan] Fix the login bug\n\n" + "## In Progress\n\n" + "## Done\n" + ) + new_entry = "- [project:koan] Fix the login bug" + assert is_duplicate_mission(content, new_entry) is False + + def test_done_section_not_checked(self): + content = ( + "# Missions\n\n" + "## Pending\n\n" + "## In Progress\n\n" + "## Done\n\n" + "- [project:koan] /rebase https://github.com/owner/repo/pull/10 ✅ (2026-05-16 10:00)\n" + ) + new_entry = "- [project:koan] /rebase https://github.com/owner/repo/pull/10" + assert is_duplicate_mission(content, new_entry) is False diff --git a/koan/tests/test_pr_review.py b/koan/tests/test_pr_review.py index 0f221cb13..a198415f8 100644 --- a/koan/tests/test_pr_review.py +++ b/koan/tests/test_pr_review.py @@ -296,29 +296,77 @@ def test_no_commit_when_clean(self, mock_run): # _run_claude # --------------------------------------------------------------------------- +class _FakeStream: + def __init__(self, lines=None, read_text=""): + self._lines = list(lines or []) + self._read_text = read_text + + def __iter__(self): + return iter(self._lines) + + def read(self): + return self._read_text + + def close(self): + return None + + +def _fake_proc(stdout_lines, stderr_text="", returncode=0): + proc = MagicMock() + proc.stdout = _FakeStream(lines=stdout_lines) + proc.stderr = _FakeStream(read_text=stderr_text) + proc.returncode = returncode + proc.pid = 99999 + proc.wait.return_value = returncode + return proc + + class TestRunClaude: - @patch("app.claude_step.subprocess.run") - def test_success(self, mock_run): - mock_run.return_value = MagicMock( - returncode=0, stdout="Done", stderr="" - ) + @patch("app.claude_step.popen_cli") + def test_success(self, mock_popen): + proc = _fake_proc(["Done\n"], stderr_text="", returncode=0) + mock_popen.return_value = (proc, lambda: None) result = _run_claude(["claude", "-p", "test"], "/tmp") assert result["success"] is True assert result["output"] == "Done" - @patch("app.claude_step.subprocess.run") - def test_failure(self, mock_run): - mock_run.return_value = MagicMock( - returncode=1, stdout="", stderr="error" - ) + @patch("app.claude_step.popen_cli") + def test_failure(self, mock_popen): + proc = _fake_proc([], stderr_text="error", returncode=1) + mock_popen.return_value = (proc, lambda: None) result = _run_claude(["claude", "-p", "test"], "/tmp") assert result["success"] is False assert "Exit code 1" in result["error"] - @patch("app.claude_step.subprocess.run") - def test_timeout(self, mock_run): - mock_run.side_effect = subprocess.TimeoutExpired(cmd="claude", timeout=10) - result = _run_claude(["claude", "-p", "test"], "/tmp", timeout=10) + @patch("os.killpg") + @patch("app.claude_step.popen_cli") + def test_timeout(self, mock_popen, mock_killpg): + """When the watchdog fires the kill, run_claude returns Timeout.""" + import threading + + killed = threading.Event() + mock_killpg.side_effect = lambda *a, **kw: killed.set() + + class _BlockingStream: + def __iter__(self): + killed.wait(timeout=10) + return iter([]) + + def read(self): + return "" + + def close(self): + return None + + proc = MagicMock() + proc.stdout = _BlockingStream() + proc.stderr = _FakeStream(read_text="") + proc.returncode = -9 + proc.pid = 12345 + proc.wait.return_value = -9 + mock_popen.return_value = (proc, lambda: None) + + result = _run_claude(["claude", "-p", "test"], "/tmp", timeout=1) assert result["success"] is False assert "Timeout" in result["error"] @@ -333,18 +381,13 @@ class TestRebaseOntoTarget: def test_success_returns_remote_name(self, mock_subproc, mock_git): result = _rebase_onto_target("main", "/tmp/p") assert result == "origin" - assert mock_git.call_count == 2 # fetch + rebase @patch("app.claude_step._run_git") @patch("app.claude_step.subprocess.run") def test_falls_back_to_upstream(self, mock_subproc, mock_git): """When origin rebase fails, tries upstream.""" - call_count = 0 - def selective_fail(*args, **kwargs): - nonlocal call_count - call_count += 1 - # First two calls are origin fetch+rebase — fail the rebase - if call_count == 2: + def selective_fail(cmd, **kwargs): + if "rebase" in cmd and any("origin" in a for a in cmd): raise RuntimeError("conflict on origin") return "" mock_git.side_effect = selective_fail diff --git a/koan/tests/test_provider_modules.py b/koan/tests/test_provider_modules.py index d14454c56..4840ac7f3 100644 --- a/koan/tests/test_provider_modules.py +++ b/koan/tests/test_provider_modules.py @@ -33,7 +33,7 @@ def test_tool_name_map_keys_are_claude_tools(self): assert set(TOOL_NAME_MAP.keys()) == CLAUDE_TOOLS def test_tool_name_map_values_are_strings(self): - for k, v in TOOL_NAME_MAP.items(): + for v in TOOL_NAME_MAP.values(): assert isinstance(v, str) assert v # not empty diff --git a/koan/tests/test_pvrs.py b/koan/tests/test_pvrs.py new file mode 100644 index 000000000..6c99e9eae --- /dev/null +++ b/koan/tests/test_pvrs.py @@ -0,0 +1,563 @@ +"""Tests for PVRS-aware security audit routing. + +Covers: +- github.py: check_pvrs_enabled(), security_advisory_report(), detect_ecosystem() +- audit_runner.py: PVRS routing in create_issues(), _should_use_pvrs() +- projects_config.py: get_project_security_config() +- security_audit_runner.py: _load_pvrs_config() +- Integration: mixed-severity findings with PVRS routing and fallback +""" + +import json +from pathlib import Path +from unittest.mock import patch, MagicMock + +import pytest + +from app.github import check_pvrs_enabled, detect_ecosystem, security_advisory_report +from app.projects_config import get_project_security_config +from skills.core.audit.audit_runner import ( + AuditFinding, + _build_advisory_description, + _should_use_pvrs, + create_issues, +) + + +# --------------------------------------------------------------------------- +# check_pvrs_enabled +# --------------------------------------------------------------------------- + +class TestCheckPvrsEnabled: + @patch("app.github.api") + def test_returns_true_when_enabled(self, mock_api): + mock_api.return_value = json.dumps({"enabled": True}) + assert check_pvrs_enabled("owner/repo") is True + + @patch("app.github.api") + def test_returns_false_when_disabled(self, mock_api): + mock_api.return_value = json.dumps({"enabled": False}) + assert check_pvrs_enabled("owner/repo") is False + + @patch("app.github.api", side_effect=RuntimeError("403 Forbidden")) + def test_returns_false_on_api_error(self, mock_api): + assert check_pvrs_enabled("owner/repo") is False + + @patch("app.github.api", return_value="not json") + def test_returns_false_on_invalid_json(self, mock_api): + assert check_pvrs_enabled("owner/repo") is False + + @patch("app.github.api", return_value=json.dumps({})) + def test_returns_false_when_key_missing(self, mock_api): + assert check_pvrs_enabled("owner/repo") is False + + +# --------------------------------------------------------------------------- +# security_advisory_report +# --------------------------------------------------------------------------- + +class TestSecurityAdvisoryReport: + @patch("app.leak_detector.scan_and_redact", side_effect=lambda x, **kw: x) + @patch("app.github.api") + def test_returns_advisory_url(self, mock_api, mock_redact): + mock_api.return_value = json.dumps({ + "html_url": "https://github.com/o/r/security/advisories/GHSA-1234", + "ghsa_id": "GHSA-1234", + }) + url = security_advisory_report( + summary="SQL injection", + description="Found SQLi in auth.py", + severity="critical", + ecosystem="pip", + package_name="myapp", + repo="owner/repo", + ) + assert url == "https://github.com/o/r/security/advisories/GHSA-1234" + + # Verify the API was called with POST + call_args = mock_api.call_args + assert call_args[1]["method"] == "POST" + assert "security-advisories/reports" in call_args[0][0] + + @patch("app.leak_detector.scan_and_redact", side_effect=lambda x, **kw: x) + @patch("app.github.api") + def test_returns_ghsa_id_when_no_url(self, mock_api, mock_redact): + mock_api.return_value = json.dumps({ + "ghsa_id": "GHSA-5678", + }) + url = security_advisory_report( + summary="XSS", description="found xss", + severity="high", repo="owner/repo", + ) + assert "GHSA-5678" in url + + @patch("app.leak_detector.scan_and_redact", side_effect=lambda x, **kw: x) + @patch("app.github.api", side_effect=RuntimeError("422")) + def test_raises_on_api_failure(self, mock_api, mock_redact): + with pytest.raises(RuntimeError): + security_advisory_report( + summary="Bug", description="desc", + severity="high", repo="owner/repo", + ) + + @patch("app.leak_detector.scan_and_redact", side_effect=lambda x, **kw: x) + @patch("app.github.api") + def test_payload_structure(self, mock_api, mock_redact): + mock_api.return_value = json.dumps({"html_url": "https://example.com"}) + security_advisory_report( + summary="Path traversal", + description="Found path traversal in upload handler", + severity="high", + ecosystem="npm", + package_name="my-pkg", + repo="owner/repo", + ) + # Verify the JSON payload sent via stdin + call_kwargs = mock_api.call_args[1] + payload = json.loads(call_kwargs["input_data"]) + assert payload["summary"] == "Path traversal" + assert payload["severity"] == "high" + assert payload["vulnerabilities"][0]["package"]["ecosystem"] == "npm" + assert payload["vulnerabilities"][0]["package"]["name"] == "my-pkg" + + +# --------------------------------------------------------------------------- +# detect_ecosystem +# --------------------------------------------------------------------------- + +class TestDetectEcosystem: + def test_python_pyproject(self, tmp_path): + (tmp_path / "pyproject.toml").write_text("[project]\nname = 'x'\n") + assert detect_ecosystem(str(tmp_path)) == "pip" + + def test_python_requirements(self, tmp_path): + (tmp_path / "requirements.txt").write_text("flask\n") + assert detect_ecosystem(str(tmp_path)) == "pip" + + def test_node_package_json(self, tmp_path): + (tmp_path / "package.json").write_text("{}\n") + assert detect_ecosystem(str(tmp_path)) == "npm" + + def test_go_module(self, tmp_path): + (tmp_path / "go.mod").write_text("module example\n") + assert detect_ecosystem(str(tmp_path)) == "go" + + def test_rust_cargo(self, tmp_path): + (tmp_path / "Cargo.toml").write_text("[package]\n") + assert detect_ecosystem(str(tmp_path)) == "cargo" + + def test_ruby_gemfile(self, tmp_path): + (tmp_path / "Gemfile").write_text("source 'https://rubygems.org'\n") + assert detect_ecosystem(str(tmp_path)) == "rubygems" + + def test_php_composer(self, tmp_path): + (tmp_path / "composer.json").write_text("{}\n") + assert detect_ecosystem(str(tmp_path)) == "composer" + + def test_java_maven(self, tmp_path): + (tmp_path / "pom.xml").write_text("\n") + assert detect_ecosystem(str(tmp_path)) == "maven" + + def test_unknown_project(self, tmp_path): + (tmp_path / "README.md").write_text("hello\n") + assert detect_ecosystem(str(tmp_path)) == "other" + + def test_python_preferred_over_node(self, tmp_path): + """When both exist, Python is detected first (order matters).""" + (tmp_path / "pyproject.toml").write_text("[project]\n") + (tmp_path / "package.json").write_text("{}\n") + assert detect_ecosystem(str(tmp_path)) == "pip" + + +# --------------------------------------------------------------------------- +# get_project_security_config +# --------------------------------------------------------------------------- + +class TestGetProjectSecurityConfig: + def test_defaults_when_no_security_section(self): + config = {"defaults": {}, "projects": {"app": {"path": "/a"}}} + result = get_project_security_config(config, "app") + assert result == {"pvrs": "auto", "pvrs_threshold": "high"} + + def test_reads_from_defaults(self): + config = { + "defaults": {"security": {"pvrs": "false", "pvrs_threshold": "medium"}}, + "projects": {"app": {"path": "/a"}}, + } + result = get_project_security_config(config, "app") + assert result["pvrs"] == "false" + assert result["pvrs_threshold"] == "medium" + + def test_project_overrides_defaults(self): + config = { + "defaults": {"security": {"pvrs": "auto", "pvrs_threshold": "high"}}, + "projects": { + "app": { + "path": "/a", + "security": {"pvrs": "true", "pvrs_threshold": "critical"}, + } + }, + } + result = get_project_security_config(config, "app") + assert result["pvrs"] == "true" + assert result["pvrs_threshold"] == "critical" + + def test_invalid_pvrs_value_falls_back_to_auto(self): + config = { + "defaults": {"security": {"pvrs": "bogus"}}, + "projects": {}, + } + result = get_project_security_config(config, "app") + assert result["pvrs"] == "auto" + + def test_invalid_threshold_falls_back_to_high(self): + config = { + "defaults": {"security": {"pvrs_threshold": "extreme"}}, + "projects": {}, + } + result = get_project_security_config(config, "app") + assert result["pvrs_threshold"] == "high" + + def test_security_not_dict_treated_as_empty(self): + config = { + "defaults": {"security": "not-a-dict"}, + "projects": {}, + } + result = get_project_security_config(config, "app") + assert result == {"pvrs": "auto", "pvrs_threshold": "high"} + + +# --------------------------------------------------------------------------- +# _should_use_pvrs +# --------------------------------------------------------------------------- + +class TestShouldUsePvrs: + def test_critical_with_high_threshold(self): + assert _should_use_pvrs("critical", "high") is True + + def test_high_with_high_threshold(self): + assert _should_use_pvrs("high", "high") is True + + def test_medium_with_high_threshold(self): + assert _should_use_pvrs("medium", "high") is False + + def test_low_with_high_threshold(self): + assert _should_use_pvrs("low", "high") is False + + def test_critical_with_critical_threshold(self): + assert _should_use_pvrs("critical", "critical") is True + + def test_high_with_critical_threshold(self): + assert _should_use_pvrs("high", "critical") is False + + def test_medium_with_medium_threshold(self): + assert _should_use_pvrs("medium", "medium") is True + + def test_low_with_low_threshold(self): + assert _should_use_pvrs("low", "low") is True + + def test_unknown_severity_returns_false(self): + assert _should_use_pvrs("unknown", "high") is False + + +# --------------------------------------------------------------------------- +# _build_advisory_description +# --------------------------------------------------------------------------- + +class TestBuildAdvisoryDescription: + def test_includes_key_sections(self): + finding = AuditFinding( + title="SQLi in login", + severity="critical", + category="injection", + location="auth.py:42-48", + problem="SQL injection in login form", + why="Allows authentication bypass", + suggested_fix="Use parameterized queries", + ) + desc = _build_advisory_description(finding) + assert "## Problem" in desc + assert "SQL injection in login form" in desc + assert "## Why This Matters" in desc + assert "## Suggested Fix" in desc + assert "`auth.py:42-48`" in desc + assert "injection" in desc + + +# --------------------------------------------------------------------------- +# create_issues — PVRS routing +# --------------------------------------------------------------------------- + +class TestCreateIssuesPvrsRouting: + """Test the routing logic in create_issues with PVRS support.""" + + def _make_findings(self): + """Create a mixed-severity set of findings.""" + return [ + AuditFinding( + title="RCE via deserialization", + severity="critical", location="api.py:10", problem="p1", + why="w1", suggested_fix="s1", category="security", + ), + AuditFinding( + title="Hardcoded API key", + severity="high", location="config.py:5", problem="p2", + why="w2", suggested_fix="s2", category="secrets", + ), + AuditFinding( + title="Missing HSTS header", + severity="medium", location="server.py:1", problem="p3", + why="w3", suggested_fix="s3", category="config", + ), + AuditFinding( + title="Verbose error messages", + severity="low", location="app.py:20", problem="p4", + why="w4", suggested_fix="s4", category="info", + ), + ] + + @patch("app.github.resolve_target_repo", return_value="upstream/repo") + @patch("app.github.check_pvrs_enabled", return_value=True) + @patch("app.github.detect_ecosystem", return_value="pip") + @patch("app.github.security_advisory_report") + @patch("app.github.issue_create") + def test_routes_critical_high_to_pvrs( + self, mock_issue, mock_pvrs, mock_eco, mock_check, mock_repo, + ): + mock_pvrs.return_value = "https://github.com/o/r/security/advisories/GHSA-1" + mock_issue.return_value = "https://github.com/o/r/issues/1\n" + + findings = self._make_findings() + urls = create_issues(findings, "/path/proj", pvrs_threshold="high") + + # critical + high → PVRS (2 calls) + assert mock_pvrs.call_count == 2 + # medium + low → public issues (2 calls) + assert mock_issue.call_count == 2 + assert len(urls) == 4 + + @patch("app.github.resolve_target_repo", return_value="upstream/repo") + @patch("app.github.check_pvrs_enabled", return_value=False) + @patch("app.github.issue_create") + def test_all_public_when_pvrs_disabled( + self, mock_issue, mock_check, mock_repo, + ): + mock_issue.return_value = "https://github.com/o/r/issues/1\n" + findings = self._make_findings() + urls = create_issues(findings, "/path/proj") + + # All go to public issues + assert mock_issue.call_count == 4 + assert len(urls) == 4 + + @patch("app.github.resolve_target_repo", return_value="upstream/repo") + @patch("app.github.issue_create") + def test_pvrs_mode_false_skips_detection(self, mock_issue, mock_repo): + """When pvrs_mode='false', PVRS detection is never called.""" + mock_issue.return_value = "https://github.com/o/r/issues/1\n" + findings = self._make_findings() + + # Should NOT call check_pvrs_enabled at all + with patch("app.github.check_pvrs_enabled") as mock_check: + urls = create_issues( + findings, "/path/proj", pvrs_mode="false", + ) + mock_check.assert_not_called() + + assert mock_issue.call_count == 4 + + @patch("app.github.resolve_target_repo", return_value="upstream/repo") + @patch("app.github.check_pvrs_enabled", return_value=True) + @patch("app.github.detect_ecosystem", return_value="pip") + @patch("app.github.security_advisory_report") + @patch("app.github.issue_create") + def test_pvrs_mode_true_skips_detection( + self, mock_issue, mock_pvrs, mock_eco, mock_check, mock_repo, + ): + """When pvrs_mode='true', check_pvrs_enabled is NOT called.""" + mock_pvrs.return_value = "https://github.com/advisory/1" + mock_issue.return_value = "https://github.com/o/r/issues/1\n" + + findings = self._make_findings() + urls = create_issues( + findings, "/path/proj", pvrs_mode="true", pvrs_threshold="high", + ) + + # check_pvrs_enabled should NOT be called when pvrs_mode is "true" + mock_check.assert_not_called() + # But PVRS reports should still be submitted for critical+high + assert mock_pvrs.call_count == 2 + + @patch("app.github.resolve_target_repo", return_value="upstream/repo") + @patch("app.github.check_pvrs_enabled", return_value=True) + @patch("app.github.detect_ecosystem", return_value="pip") + @patch("app.github.security_advisory_report", + side_effect=RuntimeError("403 Forbidden")) + @patch("app.github.issue_create") + def test_pvrs_failure_falls_back_to_public_issue( + self, mock_issue, mock_pvrs, mock_eco, mock_check, mock_repo, + ): + """When PVRS submission fails, fall back to a public issue.""" + mock_issue.return_value = "https://github.com/o/r/issues/1\n" + findings = [self._make_findings()[0]] # critical only + + notify = MagicMock() + urls = create_issues( + findings, "/path/proj", notify_fn=notify, + pvrs_threshold="high", + ) + + # PVRS was attempted, then redacted fallback issue created + assert mock_pvrs.call_count == 1 + assert mock_issue.call_count == 1 + assert len(urls) == 1 + # Fallback issue title is redacted (no finding title leaked) + title_arg = mock_issue.call_args[1]["title"] + assert "PVRS unavailable" in title_arg + assert "details withheld" in title_arg + # Body must NOT contain exploit details + body_arg = mock_issue.call_args[1]["body"] + assert "RCE via deserialization" not in body_arg + assert "withheld" in body_arg + + @patch("app.github.resolve_target_repo", return_value="upstream/repo") + @patch("app.github.check_pvrs_enabled", return_value=True) + @patch("app.github.detect_ecosystem", return_value="pip") + @patch("app.github.security_advisory_report") + @patch("app.github.issue_create") + def test_threshold_critical_only( + self, mock_issue, mock_pvrs, mock_eco, mock_check, mock_repo, + ): + """With threshold='critical', only critical goes to PVRS.""" + mock_pvrs.return_value = "https://github.com/advisory/1" + mock_issue.return_value = "https://github.com/o/r/issues/1\n" + + findings = self._make_findings() + urls = create_issues( + findings, "/path/proj", pvrs_threshold="critical", + ) + + assert mock_pvrs.call_count == 1 # only critical + assert mock_issue.call_count == 3 # high + medium + low + + @patch("app.github.resolve_target_repo", return_value="upstream/repo") + @patch("app.github.check_pvrs_enabled", return_value=True) + @patch("app.github.detect_ecosystem", return_value="pip") + @patch("app.github.security_advisory_report") + @patch("app.github.issue_create") + def test_notify_fn_reports_pvrs_channel( + self, mock_issue, mock_pvrs, mock_eco, mock_check, mock_repo, + ): + """notify_fn should indicate when PVRS is active.""" + mock_pvrs.return_value = "https://github.com/advisory/1" + mock_issue.return_value = "https://github.com/o/r/issues/1\n" + + notify = MagicMock() + findings = self._make_findings()[:2] # critical + high + create_issues( + findings, "/path/proj", notify_fn=notify, + pvrs_threshold="high", + ) + + all_calls = [c.args[0] for c in notify.call_args_list] + # Should have PVRS-enabled announcement + assert any("PVRS enabled" in c for c in all_calls) + # Should have PVRS channel markers for findings + assert any("PVRS" in c and "1/" in c for c in all_calls) + + +# --------------------------------------------------------------------------- +# Integration: _load_pvrs_config +# --------------------------------------------------------------------------- + +class TestLoadPvrsConfig: + def test_returns_defaults_when_no_koan_root(self, monkeypatch): + monkeypatch.delenv("KOAN_ROOT", raising=False) + from skills.core.security_audit.security_audit_runner import _load_pvrs_config + result = _load_pvrs_config("myapp") + assert result == {"pvrs": "auto", "pvrs_threshold": "high"} + + def test_reads_from_projects_yaml(self, tmp_path, monkeypatch): + monkeypatch.setenv("KOAN_ROOT", str(tmp_path)) + yaml_content = ( + "defaults:\n" + " security:\n" + " pvrs: 'true'\n" + " pvrs_threshold: critical\n" + "projects:\n" + " myapp:\n" + " path: /tmp/myapp\n" + ) + (tmp_path / "projects.yaml").write_text(yaml_content) + from skills.core.security_audit.security_audit_runner import _load_pvrs_config + result = _load_pvrs_config("myapp") + assert result["pvrs"] == "true" + assert result["pvrs_threshold"] == "critical" + + +# --------------------------------------------------------------------------- +# Integration: full pipeline with PVRS routing +# --------------------------------------------------------------------------- + +class TestPvrsIntegration: + """End-to-end test: mixed findings → correct routing per severity.""" + + @patch("skills.core.audit.audit_runner.build_audit_prompt", return_value="prompt") + @patch("skills.core.audit.audit_runner._run_claude_audit") + @patch("app.github.resolve_target_repo", return_value="upstream/repo") + @patch("app.github.check_pvrs_enabled", return_value=True) + @patch("app.github.detect_ecosystem", return_value="pip") + @patch("app.github.security_advisory_report") + @patch("app.github.issue_create") + def test_run_audit_with_pvrs( + self, mock_issue, mock_pvrs, mock_eco, mock_check, mock_repo, + mock_claude, mock_prompt, tmp_path, + ): + # Claude output with mixed-severity findings + mock_claude.return_value = ( + "---FINDING---\n" + "TITLE: SQL injection in login\n" + "SEVERITY: critical\n" + "CATEGORY: injection\n" + "LOCATION: auth.py:42\n" + "PROBLEM: Direct string concatenation in SQL query\n" + "WHY: Allows authentication bypass\n" + "SUGGESTED_FIX: Use parameterized queries\n" + "EFFORT: small\n" + "---FINDING---\n" + "TITLE: Missing HSTS\n" + "SEVERITY: medium\n" + "CATEGORY: config\n" + "LOCATION: server.py:1\n" + "PROBLEM: No HSTS header\n" + "WHY: Downgrade attacks possible\n" + "SUGGESTED_FIX: Add HSTS header\n" + "EFFORT: small\n" + ) + mock_pvrs.return_value = "https://github.com/o/r/security/advisories/GHSA-1" + mock_issue.return_value = "https://github.com/o/r/issues/1\n" + + from skills.core.audit.audit_runner import run_audit + + instance_dir = tmp_path / "instance" + instance_dir.mkdir() + notify = MagicMock() + + success, summary = run_audit( + project_path="/path/proj", + project_name="proj", + instance_dir=str(instance_dir), + notify_fn=notify, + pvrs_mode="auto", + pvrs_threshold="high", + ) + + assert success + assert "2 findings" in summary + # critical → PVRS, medium → public issue + assert mock_pvrs.call_count == 1 + assert mock_issue.call_count == 1 + + # Verify report saved with channel annotation + report = (instance_dir / "memory" / "projects" / "proj" / "audit.md").read_text() + assert "private" in report # PVRS finding annotated diff --git a/koan/tests/test_rebase_pr.py b/koan/tests/test_rebase_pr.py index d23206547..7898532db 100644 --- a/koan/tests/test_rebase_pr.py +++ b/koan/tests/test_rebase_pr.py @@ -354,8 +354,8 @@ def test_successful_rebase_on_origin(self): def test_falls_back_to_upstream(self): def mock_run(cmd, **kwargs): result = MagicMock(returncode=0, stdout="", stderr="") - if "origin" in cmd and "fetch" in cmd: - raise RuntimeError("fetch failed") + if "rebase" in cmd and any("origin" in a for a in cmd) and "--abort" not in cmd: + raise RuntimeError("rebase failed") return result with patch("app.claude_step.subprocess.run", side_effect=mock_run): @@ -648,6 +648,29 @@ def test_handles_empty_responses(self, mock_run): assert context["diff"] == "" assert context["review_comments"] == "" assert context["has_pending_reviews"] is False + # mergeable defaults to UNKNOWN when not provided + assert context["mergeable"] == "UNKNOWN" + + @patch("app.github.subprocess.run") + def test_extracts_mergeable_status(self, mock_run): + """fetch_pr_context surfaces the PR mergeable field for downstream consumers.""" + mock_run.side_effect = [ + MagicMock(returncode=0, stdout=json.dumps({ + "title": "Fix", + "headRefName": "br", + "baseRefName": "main", + "state": "OPEN", + "author": {"login": "dev"}, + "mergeable": "CONFLICTING", + })), + MagicMock(returncode=0, stdout="0"), + MagicMock(returncode=0, stdout="+diff"), + MagicMock(returncode=0, stdout=""), + MagicMock(returncode=0, stdout=""), + MagicMock(returncode=0, stdout=""), + ] + context = fetch_pr_context("o", "r", "1") + assert context["mergeable"] == "CONFLICTING" @patch("app.github.subprocess.run") def test_handles_invalid_json(self, mock_run): diff --git a/koan/tests/test_reset_parser.py b/koan/tests/test_reset_parser.py index 5a6ef6bcf..2a3269c50 100644 --- a/koan/tests/test_reset_parser.py +++ b/koan/tests/test_reset_parser.py @@ -1,8 +1,10 @@ """Tests for reset_parser.py — quota reset time parsing.""" -import pytest +import contextlib from datetime import datetime, timedelta +import pytest + from tests._helpers import run_module try: @@ -545,10 +547,8 @@ def test_cli_parse_valid(self): out = StringIO() with patch.object(sys, "argv", ["reset_parser", "parse", "resets 5pm (Europe/Paris)"]): with patch("sys.stdout", out): - try: + with contextlib.suppress(SystemExit): run_module("app.reset_parser", run_name="__main__") - except SystemExit: - pass output = out.getvalue() assert "|" in output @@ -564,10 +564,8 @@ def test_cli_parse_empty(self): out = StringIO() with patch.object(sys, "argv", ["reset_parser", "parse"]): with patch("sys.stdout", out): - try: + with contextlib.suppress(SystemExit): run_module("app.reset_parser", run_name="__main__") - except SystemExit: - pass output = out.getvalue() assert output.startswith("|") @@ -629,10 +627,8 @@ def test_cli_until_valid(self): out = StringIO() with patch.object(sys, "argv", ["reset_parser", "until", future_ts]): with patch("sys.stdout", out): - try: + with contextlib.suppress(SystemExit): run_module("app.reset_parser", run_name="__main__") - except SystemExit: - pass output = out.getvalue().strip() assert "h" in output or "m" in output @@ -647,10 +643,8 @@ def test_cli_until_invalid_value(self): out = StringIO() with patch.object(sys, "argv", ["reset_parser", "until", "bad"]): with patch("sys.stdout", out): - try: + with contextlib.suppress(SystemExit): run_module("app.reset_parser", run_name="__main__") - except SystemExit: - pass assert "unknown" in out.getvalue() @@ -664,10 +658,8 @@ def test_cli_until_no_args(self): out = StringIO() with patch.object(sys, "argv", ["reset_parser", "until"]): with patch("sys.stdout", out): - try: + with contextlib.suppress(SystemExit): run_module("app.reset_parser", run_name="__main__") - except SystemExit: - pass assert "unknown" in out.getvalue() diff --git a/koan/tests/test_restart.py b/koan/tests/test_restart.py index afc9f41d7..07148d893 100644 --- a/koan/tests/test_restart.py +++ b/koan/tests/test_restart.py @@ -258,7 +258,7 @@ def test_main_clears_stale_file_after_first_poll(self): source = inspect.getsource(main) while_idx = source.index("while True:") # clear_restart should appear inside the loop (after first poll) - clear_idx = source.index("clear_restart(str(KOAN_ROOT))", while_idx) + clear_idx = source.index("clear_restart(str(KOAN_ROOT)", while_idx) assert clear_idx > while_idx # And it should be guarded by first_poll assert "first_poll" in source diff --git a/koan/tests/test_restart_manager.py b/koan/tests/test_restart_manager.py index 55008d4b7..93b072d6e 100644 --- a/koan/tests/test_restart_manager.py +++ b/koan/tests/test_restart_manager.py @@ -5,8 +5,12 @@ from pathlib import Path from unittest.mock import patch, MagicMock +import pytest + from app.restart_manager import ( RESTART_FILE, + RESTART_BRIDGE_FILE, + RESTART_RUN_FILE, RESTART_EXIT_CODE, request_restart, check_restart, @@ -54,13 +58,14 @@ def test_overwrites_existing_file(self, tmp_path): assert "restart requested at" in content def test_uses_atomic_write(self, tmp_path): - """request_restart should use atomic_write for thread safety.""" + """request_restart should use atomic_write for thread safety, + once per consumer marker plus the legacy single-file marker.""" with patch("app.utils.atomic_write") as mock_aw: request_restart(str(tmp_path)) - mock_aw.assert_called_once() - # First arg should be a Path - call_path = mock_aw.call_args[0][0] - assert str(call_path).endswith(RESTART_FILE) + written = [str(call.args[0]) for call in mock_aw.call_args_list] + assert any(p.endswith(RESTART_BRIDGE_FILE) for p in written) + assert any(p.endswith(RESTART_RUN_FILE) for p in written) + assert any(p.endswith(RESTART_FILE) for p in written) # --------------------------------------------------------------------------- @@ -236,3 +241,64 @@ def test_accepts_str_not_path(self, tmp_path): assert check_restart(root) is True clear_restart(root) assert check_restart(root) is False + + +# --------------------------------------------------------------------------- +# Per-process restart markers (race-fix) +# --------------------------------------------------------------------------- + + +class TestPerProcessRestartMarkers: + """Each process polls its own marker so a fast wrapper-restart of one + consumer cannot wipe the signal before the other consumer's poll tick. + + Regression for the ``/update`` race: the runner used to write a single + ``.koan-restart`` file, exit with code 42, and have its wrapper relaunch + it within ~1 s; the fresh runner's startup ``clear_restart`` then + wiped the file before the bridge's 3 s poll tick could observe it, + leaving the bridge with a stale ``sys.modules`` and ``/list`` broken. + """ + + def test_request_restart_writes_all_three_markers(self, tmp_path): + request_restart(str(tmp_path)) + assert (tmp_path / RESTART_BRIDGE_FILE).exists() + assert (tmp_path / RESTART_RUN_FILE).exists() + assert (tmp_path / RESTART_FILE).exists(), ( + "legacy marker must still be written so a pre-upgrade bridge " + "polling .koan-restart can re-exec into the new code" + ) + + def test_check_restart_target_isolation(self, tmp_path): + """Writing only one consumer's marker must not satisfy the other.""" + (tmp_path / RESTART_BRIDGE_FILE).write_text("restart") + assert check_restart(str(tmp_path), target="bridge") is True + assert check_restart(str(tmp_path), target="run") is False + # And the legacy single-marker check still has its own file. + assert check_restart(str(tmp_path), target=None) is False + + def test_clear_restart_target_isolation(self, tmp_path): + """clear_restart for one target must leave the other intact.""" + request_restart(str(tmp_path)) + clear_restart(str(tmp_path), target="run") + assert not (tmp_path / RESTART_RUN_FILE).exists() + assert (tmp_path / RESTART_BRIDGE_FILE).exists() + # Legacy file is also untouched — only its own consumer clears it. + assert (tmp_path / RESTART_FILE).exists() + + def test_runner_wrapper_restart_does_not_silence_bridge(self, tmp_path): + """Simulate the /update race directly: a request_restart followed + immediately by the runner's wrapper-restart clear leaves the bridge + marker fully intact and detectable on a later poll tick.""" + startup_time = time.time() - 60 # bridge has been up for a while + request_restart(str(tmp_path)) + # Simulate the fresh runner's L785 startup wipe. + clear_restart(str(tmp_path), target="run") + # Bridge's poll tick now sees its own marker as fresh. + assert check_restart( + str(tmp_path), since=startup_time, target="bridge" + ) is True + + @pytest.mark.parametrize("fn", [check_restart, clear_restart]) + def test_unknown_target_raises(self, tmp_path, fn): + with pytest.raises(ValueError): + fn(str(tmp_path), target="runner") # type: ignore[arg-type] diff --git a/koan/tests/test_review_runner.py b/koan/tests/test_review_runner.py index 2a5f7c903..e8670077a 100644 --- a/koan/tests/test_review_runner.py +++ b/koan/tests/test_review_runner.py @@ -731,6 +731,66 @@ def test_comment_post_failure( assert success is False assert "failed to post" in summary.lower() + @patch("app.review_runner._fetch_pr_commit_shas", return_value=[]) + @patch("app.review_runner.fetch_repliable_comments", return_value=[]) + @patch("app.review_runner.run_gh") + @patch("app.review_runner._run_claude_review") + @patch("app.review_runner.fetch_pr_context") + def test_conflict_notice_prepended_for_conflicting_pr( + self, mock_fetch, mock_claude, mock_gh, mock_repliable, _mock_shas, + pr_context, review_skill_dir, + ): + """When mergeable=CONFLICTING, posted comment leads with a conflict warning.""" + pr_context["mergeable"] = "CONFLICTING" + pr_context["base"] = "develop" + mock_fetch.return_value = pr_context + mock_claude.return_value = (json.dumps(LGTM_REVIEW_JSON), "") + mock_notify = MagicMock() + + success, _summary, _rd = run_review( + "owner", "repo", "42", "/tmp/project", + notify_fn=mock_notify, + skill_dir=review_skill_dir, + ) + + assert success is True + # Find the --body argument passed to gh + call = mock_gh.call_args + body = call.kwargs.get("body") or next( + (a for a in call.args if "Merge conflicts" in str(a)), "" + ) + assert "Merge conflicts detected" in body + assert "develop" in body + # Warning appears BEFORE the review body + assert body.index("Merge conflicts detected") < body.index("## PR Review") + + @patch("app.review_runner._fetch_pr_commit_shas", return_value=[]) + @patch("app.review_runner.fetch_repliable_comments", return_value=[]) + @patch("app.review_runner.run_gh") + @patch("app.review_runner._run_claude_review") + @patch("app.review_runner.fetch_pr_context") + def test_no_conflict_notice_for_mergeable_pr( + self, mock_fetch, mock_claude, mock_gh, mock_repliable, _mock_shas, + pr_context, review_skill_dir, + ): + """When mergeable=MERGEABLE, no conflict warning is added.""" + pr_context["mergeable"] = "MERGEABLE" + mock_fetch.return_value = pr_context + mock_claude.return_value = (json.dumps(LGTM_REVIEW_JSON), "") + mock_notify = MagicMock() + + success, _summary, _rd = run_review( + "owner", "repo", "42", "/tmp/project", + notify_fn=mock_notify, + skill_dir=review_skill_dir, + ) + + assert success is True + call = mock_gh.call_args + body_args = [str(a) for a in call.args] + [str(v) for v in call.kwargs.values()] + joined = "\n".join(body_args) + assert "Merge conflicts detected" not in joined + # --------------------------------------------------------------------------- # _run_claude_review diff --git a/koan/tests/test_run.py b/koan/tests/test_run.py index 8f6f79800..92575a32b 100644 --- a/koan/tests/test_run.py +++ b/koan/tests/test_run.py @@ -1,5 +1,6 @@ """Tests for app.run — the full Python main loop.""" +import contextlib import os import signal import subprocess @@ -1032,15 +1033,13 @@ def test_resets_signal_state_on_popen_failure(self, tmp_path): stderr_f = str(tmp_path / "err.txt") with patch("app.cli_exec.popen_cli", side_effect=OSError("exec failed")): - try: + with contextlib.suppress(OSError): run_claude_task( cmd=["nonexistent"], stdout_file=stdout_f, stderr_file=stderr_f, cwd=str(tmp_path), ) - except OSError: - pass # Signal state must be cleaned up despite the exception assert _sig.task_running is False @@ -1058,15 +1057,13 @@ def test_resets_signal_state_on_open_failure(self, tmp_path): bad_dir.mkdir() stderr_f = str(tmp_path / "err.txt") - try: + with contextlib.suppress(OSError, IsADirectoryError): run_claude_task( cmd=["echo", "hello"], stdout_file=str(bad_dir), # can't open a directory for writing stderr_file=stderr_f, cwd=str(tmp_path), ) - except (OSError, IsADirectoryError): - pass assert _sig.task_running is False assert _sig.claude_proc is None @@ -1589,7 +1586,7 @@ def test_restart_file_exits_42(self, mock_release, mock_acquire, mock_startup, m # Create restart file AFTER startup (via side_effect) so startup # cleanup doesn't remove it before the loop's restart check runs. def startup_creates_restart(*args, **kwargs): - restart_file = koan_root / ".koan-restart" + restart_file = koan_root / ".koan-restart-run" restart_file.write_text("restart") future = time.time() + 3600 os.utime(str(restart_file), (future, future)) @@ -1607,16 +1604,16 @@ def startup_creates_restart(*args, **kwargs): @patch("app.run.acquire_pidfile") @patch("app.run.release_pidfile") def test_restart_file_cleared_before_exit(self, mock_release, mock_acquire, mock_startup, mock_subproc, koan_root): - """Regression: run.py must clear .koan-restart before sys.exit(RESTART_EXIT_CODE) - to prevent the restarted process from seeing a stale file and - entering a restart loop.""" + """Regression: run.py must clear its per-process restart marker before + sys.exit(RESTART_EXIT_CODE) to prevent the restarted process from + seeing a stale file and entering a restart loop.""" from app.run import main_loop from app.restart_manager import RESTART_EXIT_CODE os.environ["KOAN_ROOT"] = str(koan_root) os.environ["KOAN_PROJECTS"] = f"test:{koan_root}" - restart_file = koan_root / ".koan-restart" + restart_file = koan_root / ".koan-restart-run" def startup_creates_restart(*args, **kwargs): restart_file.write_text("restart") @@ -1630,9 +1627,9 @@ def startup_creates_restart(*args, **kwargs): with patch("app.run._notify"): main_loop() assert exc.value.code == RESTART_EXIT_CODE - # The restart file must be deleted BEFORE exit + # The runner's per-process marker must be deleted BEFORE exit assert not restart_file.exists(), \ - ".koan-restart was not cleared before exit — restart loop risk" + ".koan-restart-run was not cleared before exit — restart loop risk" @patch("app.run.subprocess.run") @patch("app.run.run_startup", return_value=(5, 10, "koan/")) @@ -1651,8 +1648,8 @@ def test_stale_restart_file_cleared_on_startup(self, mock_release, mock_acquire, os.environ["KOAN_PROJECTS"] = f"test:{koan_root}" (koan_root / ".koan-project").write_text("test") - # Simulate stale .koan-restart from a previous session - (koan_root / ".koan-restart").write_text("stale restart") + # Simulate stale .koan-restart-run from a previous session + (koan_root / ".koan-restart-run").write_text("stale restart") # Startup creates a stop file so the loop exits cleanly def startup_then_stop(*args, **kwargs): @@ -1666,8 +1663,8 @@ def startup_then_stop(*args, **kwargs): # Startup ran (stale restart didn't cause immediate exit) mock_startup.assert_called_once() - # The restart file was cleared - assert not (koan_root / ".koan-restart").exists() + # The runner's per-process marker was cleared + assert not (koan_root / ".koan-restart-run").exists() @patch("app.run.subprocess.run") @patch("app.run.run_startup", return_value=(5, 10, "koan/")) @@ -4506,7 +4503,7 @@ def test_pause_loop_uses_check_restart(self, koan_root): patch("app.run.check_restart", side_effect=[False, True]) as mock_check: result = handle_pause(str(koan_root), instance, 5) assert result is None # breaks out of pause loop - mock_check.assert_called_with(str(koan_root)) + mock_check.assert_called_with(str(koan_root), target="run") @patch("app.run.subprocess.run") @patch("app.run.run_startup", return_value=(5, 10, "koan/")) @@ -4529,7 +4526,7 @@ def startup_then_stop(*args, **kwargs): with patch("app.run._notify"), \ patch("app.run.clear_restart") as mock_clear: main_loop() - mock_clear.assert_called_once_with(str(koan_root)) + mock_clear.assert_called_once_with(str(koan_root), target="run") @patch("app.run.subprocess.run") @patch("app.run.run_startup", return_value=(5, 10, "koan/")) diff --git a/koan/tests/test_skill_dispatch.py b/koan/tests/test_skill_dispatch.py index a6dcbee22..11aed6b6f 100644 --- a/koan/tests/test_skill_dispatch.py +++ b/koan/tests/test_skill_dispatch.py @@ -653,10 +653,6 @@ def test_rebase_handler_clean_format(self, tmp_path, monkeypatch): "app.utils.resolve_project_path", lambda repo, owner=None: "/workspace/koan", ) - monkeypatch.setattr( - "app.github_url_parser.parse_pr_url", - lambda url: ("sukria", "koan", "42"), - ) monkeypatch.setattr( "app.github_skill_helpers.is_own_pr", @@ -664,6 +660,16 @@ def test_rebase_handler_clean_format(self, tmp_path, monkeypatch): ) from skills.core.rebase.handler import handle + # Patch parse_pr_url on the handler module's local binding, NOT on the + # source module — the handler does `from app.github_url_parser import + # parse_pr_url` at module load and caches that reference. Patching the + # source module after the handler is imported would have no effect, and + # patching it before the handler import would leak a stale binding into + # later tests (the cause of #xdist-pollution). + monkeypatch.setattr( + "skills.core.rebase.handler.parse_pr_url", + lambda url: ("sukria", "koan", "42"), + ) ctx = self._make_ctx( args="https://github.com/sukria/koan/pull/42", instance_dir=tmp_path, @@ -748,12 +754,14 @@ def test_recreate_handler_clean_format(self, tmp_path, monkeypatch): "app.utils.resolve_project_path", lambda repo, owner=None: "/workspace/koan", ) + + from skills.core.recreate.handler import handle + # Patch parse_pr_url on the handler module's local binding (see + # comment on test_rebase_handler_clean_format above for why). monkeypatch.setattr( - "app.github_url_parser.parse_pr_url", + "skills.core.recreate.handler.parse_pr_url", lambda url: ("sukria", "koan", "42"), ) - - from skills.core.recreate.handler import handle ctx = self._make_ctx( args="https://github.com/sukria/koan/pull/42", instance_dir=tmp_path, diff --git a/koan/tests/test_skills.py b/koan/tests/test_skills.py index 72560bdd0..8ef83e222 100644 --- a/koan/tests/test_skills.py +++ b/koan/tests/test_skills.py @@ -2249,9 +2249,11 @@ def test_reload_when_mtime_changes(self, monkeypatch, tmp_path): _module_mtimes.pop("app.refreshable_test", None) def test_first_encounter_caches_mtime_without_reload(self, monkeypatch, tmp_path): - """First time seeing a module just caches mtime, does not reload.""" + """First time seeing a module just caches mtime, does not reload — + provided the file is no newer than the process start time.""" import importlib as _importlib + from app import skills as _skills from app.skills import _module_mtimes, _refresh_stale_app_modules fake_file = tmp_path / "first_seen.py" @@ -2263,6 +2265,12 @@ def test_first_encounter_caches_mtime_without_reload(self, monkeypatch, tmp_path # Ensure not in mtime cache _module_mtimes.pop("app.first_seen_test", None) + # Pretend the process started well after the file's mtime, so the + # first-encounter fast path applies (auto-update did not touch it). + monkeypatch.setattr( + _skills, "_PROCESS_START_TIME", fake_file.stat().st_mtime + 60, + ) + reload_calls = [] original_reload = _importlib.reload monkeypatch.setattr( @@ -2272,13 +2280,56 @@ def test_first_encounter_caches_mtime_without_reload(self, monkeypatch, tmp_path _refresh_stale_app_modules() - assert not reload_calls, "First encounter should not trigger reload" + assert not reload_calls, "First encounter of an old file should not reload" assert "app.first_seen_test" in _module_mtimes # Cleanup sys.modules.pop("app.first_seen_test", None) _module_mtimes.pop("app.first_seen_test", None) + def test_first_encounter_reloads_when_file_newer_than_process_start( + self, monkeypatch, tmp_path, + ): + """If a module's source file was modified after the process started + (auto-update path), the very first observation must trigger a reload + even though no baseline mtime exists yet. Regression test for the + ``cannot import name 'PROJECT_NAME_CHARS' from 'app.utils'`` failure + on the first /list after an auto-update added a new symbol.""" + import importlib as _importlib + + from app import skills as _skills + from app.skills import _module_mtimes, _refresh_stale_app_modules + + fake_file = tmp_path / "post_update.py" + fake_file.write_text("X = 1") + fake_mod = MagicMock() + fake_mod.__file__ = str(fake_file) + + monkeypatch.setitem(sys.modules, "app.post_update_test", fake_mod) + # No cached mtime: this is the first observation of the module. + _module_mtimes.pop("app.post_update_test", None) + + # Pretend the process started before the file was written. + file_mtime = fake_file.stat().st_mtime + monkeypatch.setattr(_skills, "_PROCESS_START_TIME", file_mtime - 60) + + reload_calls = [] + monkeypatch.setattr( + _importlib, "reload", + lambda m: reload_calls.append(m), + ) + + _refresh_stale_app_modules() + + assert reload_calls == [fake_mod], ( + "First observation of a file newer than process start must reload" + ) + assert _module_mtimes.get("app.post_update_test") == file_mtime + + # Cleanup + sys.modules.pop("app.post_update_test", None) + _module_mtimes.pop("app.post_update_test", None) + def test_failed_reload_evicts_module(self, monkeypatch, tmp_path): """If reload fails, the module is evicted from sys.modules.""" import importlib as _importlib diff --git a/koan/tests/test_utils.py b/koan/tests/test_utils.py index a98981743..892fa7ed4 100644 --- a/koan/tests/test_utils.py +++ b/koan/tests/test_utils.py @@ -254,6 +254,49 @@ def bad_transform(content): temp_files = list(tmp_path.glob(".missions-*")) assert temp_files == [], f"Temp files left behind after error: {temp_files}" + def test_returns_true_when_inserted(self, tmp_path): + from app.utils import insert_pending_mission + missions = tmp_path / "missions.md" + missions.write_text("# Missions\n\n## Pending\n\n## In Progress\n\n## Done\n") + + result = insert_pending_mission( + missions, "- [project:koan] /rebase https://github.com/o/r/pull/1" + ) + assert result is True + assert "/rebase" in missions.read_text() + + def test_returns_false_on_duplicate(self, tmp_path): + from app.utils import insert_pending_mission + missions = tmp_path / "missions.md" + missions.write_text( + "# Missions\n\n## Pending\n\n" + "- [project:koan] /rebase https://github.com/o/r/pull/1 ⏳(2026-05-16T10:00)\n\n" + "## In Progress\n\n## Done\n" + ) + + result = insert_pending_mission( + missions, "- [project:koan] /rebase https://github.com/o/r/pull/1" + ) + assert result is False + # File unchanged — no double entry + content = missions.read_text() + assert content.count("/rebase https://github.com/o/r/pull/1") == 1 + + def test_non_github_mission_always_inserted(self, tmp_path): + from app.utils import insert_pending_mission + missions = tmp_path / "missions.md" + missions.write_text( + "# Missions\n\n## Pending\n\n" + "- [project:koan] Fix the login bug\n\n" + "## In Progress\n\n## Done\n" + ) + + result = insert_pending_mission( + missions, "- [project:koan] Fix the login bug" + ) + # Non-GitHub missions are not deduped (no signature) + assert result is True + def test_modify_missions_file_returns_new_content(self, tmp_path): """modify_missions_file should return the transformed content.""" from app.utils import modify_missions_file diff --git a/projects.example.yaml b/projects.example.yaml index 30c59ebad..0597439d4 100644 --- a/projects.example.yaml +++ b/projects.example.yaml @@ -117,6 +117,25 @@ defaults: # Default: 10 max_pending_branches: 10 + # Security audit — PVRS (Private Vulnerability Reporting) settings. + # + # When /security_audit runs, findings at or above the threshold severity + # are submitted as private security advisories (PVRS) instead of public + # issues, keeping exploit details private until a fix is applied. + # + # pvrs: auto | true | false + # auto — detect PVRS at runtime via GitHub API (default) + # true — always submit high-severity findings via PVRS + # false — always create public issues (opt out of PVRS) + # + # pvrs_threshold: critical | high | medium | low + # Findings at or above this severity go to PVRS (default: high). + # E.g., "high" routes critical + high to PVRS; medium + low stay public. + # + # security: + # pvrs: auto + # pvrs_threshold: high + projects: # Example: your main project (minimal config — inherits all defaults) myapp: diff --git a/pyproject.toml b/pyproject.toml index c77507dbe..054c9345b 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -4,6 +4,15 @@ version = "0.1.0" description = "Autonomous background agent using idle Claude API quota" requires-python = ">=3.11" +[tool.ruff] +target-version = "py311" + +[tool.ruff.lint] +select = ["PERF", "SIM105"] + +[tool.ruff.lint.per-file-ignores] +"koan/tests/*" = ["PERF"] + [tool.pytest.ini_options] testpaths = ["koan/tests"] pythonpath = ["koan"]