diff --git a/.opencode/agents/docs-reviewer.md b/.opencode/agents/docs-reviewer.md index 59489b3..b478759 100644 --- a/.opencode/agents/docs-reviewer.md +++ b/.opencode/agents/docs-reviewer.md @@ -3,7 +3,6 @@ description: Reviews and updates project map documentation before code review. A mode: subagent temperature: 0.1 steps: 100 -doom_loop: deny permission: edit: allow doom_loop: deny diff --git a/.opencode/agents/memory-syncer.md b/.opencode/agents/memory-syncer.md index f77cf77..3897ee9 100644 --- a/.opencode/agents/memory-syncer.md +++ b/.opencode/agents/memory-syncer.md @@ -3,7 +3,6 @@ description: Distills durable knowledge from merged PR handoffs into global memo mode: subagent temperature: 0.1 steps: 100 -doom_loop: deny permission: edit: allow doom_loop: deny diff --git a/.opencode/agents/reviewer.md b/.opencode/agents/reviewer.md index 191ae9e..9a56163 100644 --- a/.opencode/agents/reviewer.md +++ b/.opencode/agents/reviewer.md @@ -3,7 +3,6 @@ description: Global code reviewer. Reviews PRs against project skills and univer mode: subagent temperature: 0.1 steps: 100 -doom_loop: deny permission: edit: deny doom_loop: deny @@ -96,9 +95,18 @@ You are a global code reviewer. Your job: review PRs against project skills and 4. Check if the repo has project-specific skills: - Run `find skills/ -name "SKILL.md" -o -name "skill.md" 2>/dev/null` - If skills exist, load each via `skill("")` to get project-specific rules. -5. Check CI status: используй нативный tool `pipeline_status({pr_number: })` OR `gh run list --branch --limit 3`. **Запрещено `gh pr checks`** — 403 на fine-grained PAT (scope `Checks: read` не существует). Только Actions API (`gh run list`, `gh run view`, `gh api repos/.../actions/runs`). **Запрещено** bash-запуск `python3 config/scripts/pipeline-status.py` — детерминированный deny-rule (см. ADR-019). +5. Check CI status: use `pipeline_status({pr_number: })` tool. + If unavailable, use `gh run list --branch --limit 3`. + Do NOT use `gh pr checks` (403) or bash `python3 .../pipeline-status.py` (denied). 6. Also check for `.opencode/agents/` project-level agents that may define conventions. +## Investigation Budget + +You have a maximum of ~15 steps for investigation (Setup + checklist). +After that, you MUST call post_review — even if you haven't checked everything. +An incomplete review with verdict NEEDS_DISCUSSION is better than an infinite investigation. +Do NOT repeatedly verify references in agent .md files — read once, assess, move on. + ## Review Checklist ### 1. Code Quality diff --git a/docs/decisions/020-pr-49-doom-loop-frontmatter.md b/docs/decisions/020-pr-49-doom-loop-frontmatter.md new file mode 100644 index 0000000..7bfd799 --- /dev/null +++ b/docs/decisions/020-pr-49-doom-loop-frontmatter.md @@ -0,0 +1,71 @@ +# ADR-020 (PR #49): Remove invalid top-level doom_loop frontmatter + reviewer doom-loop guard + +## Статус +Accepted (2026-07-24) + +## Контекст + +При review PR #46 (reviewer ревьюил свой собственный .md файл) reviewer agent ушёл в doom loop — 75 повторений `git show f06c9f2 | grep ADR-019` за 6 минут, отменён пользователем на step 88. Investigation выявило 2 root causes: + +### Root cause 1: top-level `doom_loop: deny` в agent frontmatter + +Три agent-файла (`reviewer.md`, `docs-reviewer.md`, `memory-syncer.md`) содержали `doom_loop: deny` КАК top-level frontmatter поле (строка 6, indent 0, ВНЕ блока `permission:`). + +Проблема: `doom_loop` НЕ входит в схему `AgentV2.Info` opencode (`packages/opencode/src/agent/agent.ts`). Unknown top-level поля форвардятся провайдеру как model parameters (passthrough). На строгих провайдерах (umans-ai-coding-plan, anthropic) это вызывает `AI_APICallError: Extra inputs are not permitted`. Подтверждено логом `opencode.log:91717`. + +Валидное место для `doom_loop` — ВНУТРИ блока `permission:` (отступ 2 пробела). Там поле распознаётся opencode как permission rule (guard против doom loops на permission layer). Top-level копия — ошибочное дублирование (вероятно при миграции `config/` → `.opencode/` в PR#23). + +### Root cause 2: self-referential ADR-019 trigger в reviewer.md Setup шаг 5 + +Setup шаг 5 в reviewer.md содержал inline-ссылку на ADR-019 и детальное объяснение запрета `gh pr checks` (403) и `python3 pipeline-status.py` (denied). При review своего собственного .md файла модель зациклилась на верификации этой ссылки: читает reviewer.md → видит "ADR-019" → идёт проверять существование ADR-019 (`git show f06c9f2 | grep ADR-019`) → повторяет 75 раз. + +Это "крючок" (self-referential trigger) — промпт-ссылка на документ, который агент пытается верифицировать, вместо того чтобы выполнять review. + +## Решение + +### 1. Удалить top-level `doom_loop: deny` из frontmatter (3 файла) + +Удалена строка `doom_loop: deny` на indent 0 из `reviewer.md:6`, `docs-reviewer.md:6`, `memory-syncer.md:6`. + +`doom_loop: deny` ВНУТРИ блока `permission:` (indent 2, строки 8/8/8) — ОСТАВЛЕН (валидный permission guard). `steps: 100` — НЕ ТРОНУТ. + +### 2. Упростить Setup шаг 5 в reviewer.md + +Inline-ссылка на ADR-019 и детальное объяснение запрета заменены на лаконичную инструкцию: +- `pipeline_status({pr_number: })` tool (primary) +- `gh run list --branch --limit 3` (fallback) +- "Do NOT use `gh pr checks` (403) or bash `python3 .../pipeline-status.py` (denied)" (краткий запрет без ADR-ссылки) + +Запреты функционально обеспечены permission rules (frontmatter) + DANGEROUS_PATTERNS (check-permissions.py), inline-ссылка не была функциональным guard — только self-referential hook. + +### 3. Investigation Budget секция + +После Setup секции добавлена `## Investigation Budget`: +- Максимум ~15 steps для investigation (Setup + checklist) +- После 15 steps — обязательный `post_review`, даже при неполном review +- "An incomplete review with verdict NEEDS_DISCUSSION is better than an infinite investigation" +- "Do NOT repeatedly verify references in agent .md files — read once, assess, move on" + +Structural guard против doom loops: явный step limit даёт модели сигнал остановиться, вместо бесконечной верификации. + +### 4. Тесты (`tests/test_agent_frontmatter.py`) + +6 тестов (pyyaml не в зависимостях — ручной парсинг frontmatter по паттерну `check-permissions.py`): +- `test_no_top_level_doom_loop` — parsed frontmatter не содержит top-level `doom_loop` +- `test_no_top_level_doom_loop_raw` — raw text check: нет `doom_loop:` на indent 0 +- `test_permission_doom_loop_present` — `permission.doom_loop: deny` присутствует во всех 3 файлах +- `test_steps_100_present` — `steps: 100` присутствует во всех 3 файлах +- `test_agent_files_exist` — все 3 файла существуют +- `test_frontmatter_parseable` — frontmatter корректно парсится + +## Альтернативы + +- **Оставить top-level `doom_loop` и добавить его в opencode `AgentV2.Info` схему** — отклонено: opencode — upstream проект, модификация его schema вне scope этого репо. Правильное решение — убрать невалидное поле, использовать валидное место (`permission:`). + +- **Удалить `permission.doom_loop` тоже (раз top-level невалиден)** — отклонено: `permission.doom_loop` — валидное и полезное поле (permission guard против doom loops на permission layer). Top-level копия — ошибка, permission-вложенное — намеренный guard. Удалять guard нельзя. + +- **Убрать ADR-019 ссылку, но оставить детальное объяснение запрета** — отклонено: детальное объяснение тоже self-referential hook (модель объясняет себе, почему нельзя, вместо того чтобы не делать). Лаконичное "Do NOT" + permission rules — достаточно. + +- **Investigation Budget как hard guard в type system (tool timeout)** — отклонено: opencode tools не имеют per-agent step budget (только global `steps: 100` в frontmatter). Hard guard = steps: 100 (модель упрётся в лимит). Investigation Budget — soft guard, ранний сигнал "остановись до global лимита". Лучше soft + hard, чем только hard. + +- **Параметризовать ~15 в конфиге (env var / frontmatter)** — отклонено: over-engineering для текущей задачи. ~15 — эвристика из эмпирики doom loop (88 steps = слишком много, 15 = достаточно для Setup + checklist). Если понадобится тюнинг — отдельный PR. \ No newline at end of file diff --git a/docs/handoff/pr-49-doom-loop-frontmatter.md b/docs/handoff/pr-49-doom-loop-frontmatter.md new file mode 100644 index 0000000..395f2bb --- /dev/null +++ b/docs/handoff/pr-49-doom-loop-frontmatter.md @@ -0,0 +1,36 @@ +--- +pr: 49 +title: remove invalid doom_loop frontmatter field + simplify reviewer setup +--- + +# PR #49: remove invalid doom_loop frontmatter field + simplify reviewer setup + +## Что сделано +- `.opencode/agents/reviewer.md:6` — удалён top-level `doom_loop: deny` (ВНЕ блока `permission:`). Невалидное поле — не в схеме `AgentV2.Info` opencode, форвардилось провайдеру как model param → `AI_APICallError: Extra inputs are not permitted` на строгих провайдерах (umans-ai-coding-plan). +- `.opencode/agents/docs-reviewer.md:6` — удалён top-level `doom_loop: deny` (тот же fix). +- `.opencode/agents/memory-syncer.md:6` — удалён top-level `doom_loop: deny` (тот же fix). +- `permission.doom_loop: deny` ВНУТРИ блока `permission:` — сохранён во всех 3 файлах (валидный permission guard, не тронут). +- `steps: 100` — не изменён во всех 3 файлах. +- `reviewer.md` Setup шаг 5 — упрощён: убрана inline-ссылка на ADR-019 и детальное объяснение запрета `gh pr checks` / `python3 pipeline-status.py`. Заменено на лаконичную инструкцию с `pipeline_status` tool + `gh run list` fallback + явным "Do NOT". Это устраняет self-referential trigger (модель зацикливалась на верификации ADR-019 ссылки — 75 повторений `git show f06c9f2 | grep ADR-019` за 6 минут, doom loop отменён на step 88). +- `reviewer.md` — добавлена секция `## Investigation Budget` после Setup: максимум ~15 steps для investigation, после чего обязательный `post_review` даже при неполном review. Предотвращает бесконечные investigation loops. +- `tests/test_agent_frontmatter.py` — 6 новых тестов: frontmatter НЕ содержит top-level `doom_loop` (parsed + raw check), `permission.doom_loop: deny` присутствует во всех 3 файлах, `steps: 100` присутствует, frontmatter parseable. Парсинг ручной (pyyaml не в зависимостях), по паттерну `check-permissions.py` (split по `---`, отслеживание отступов). +- ADR-020 + этот handoff + +## Почему +При review PR #46 (reviewer ревьюил свой собственный .md файл) reviewer agent ушёл в doom loop — 75 повторений `git show f06c9f2 | grep ADR-019` за 6 минут, отменён пользователем на step 88. Investigation выявило 2 проблемы: + +1. **`doom_loop: deny` как top-level frontmatter поле** — НЕ в схеме `AgentV2.Info` opencode, форвардится провайдеру как model param → `AI_APICallError` на строгих провайдерах. Валидно только внутри блока `permission:`. Подтверждено логом `opencode.log:91717`. Top-level поле — копия permission guard, ошибочно продублированная на верхний уровень (вероятно при миграции config/ → .opencode/ в PR#23). + +2. **Self-referential trigger** — Setup шаг 5 в reviewer.md содержал inline-ссылку на ADR-019 и детальное объяснение запрета. Модель зациклилась на верификации этой ссылки (читает reviewer.md → видит ADR-019 → идёт проверять ссылку → повторяет). Это "крючок", за который модель зацепилась в doom loop. + +Решение: удалить невалидное top-level поле (fix root cause #1) + упростить Setup шаг 5 (убрать self-referential hook, fix root cause #2) + добавить Investigation Budget секцию (structural guard против doom loops — явный step limit). + +## Pending +— (нет) + +## Watch out +- **Top-level `doom_loop` удалён, НЕ `permission.doom_loop`** — `doom_loop: deny` внутри блока `permission:` (отступ 2 пробела, строки 8/8/8) — ОСТАВЛЕН во всех 3 файлах. Это валидный permission guard. Удалён только top-level `doom_loop: deny` (indent 0, строка 6). Тесты `test_permission_doom_loop_present` + `test_no_top_level_doom_loop` явно различают эти два поля. +- **Global config (OLD repo) НЕ обновлён** — bind-mounted `/root/.config/opencode/agents/*.md` (из `/root/workspace/opencode/config/agents/`) всё ещё содержит top-level `doom_loop` (commit 38c5a34 фиксил только NEW repo `.opencode/agents/`). Если strict provider используется в репо БЕЗ project-local agents → falls back to global → bug resurfaces. Это known issue (см. memory: technical/opencode-agent-loading-precedence.md), вне scope этого PR. +- **Investigation Budget — soft guard** — секция в промпте reviewer.md, не type system. Модель может игнорировать ~15 step limit (как игнорирует другие промпт-правила). Но explicit limit лучше implicit — даёт модели чёткий сигнал "остановись и вызови post_review". Hard guard = steps: 100 (frontmatter) — на нём модель упрётся в лимит в любом случае. +- **Setup шаг 5 упрощён, НЕ удалён** — `gh pr checks` (403) и `python3 pipeline-status.py` (denied) запреты сохранены в лаконичной форме ("Do NOT use ..."). Запреты функциональны (permission rules в frontmatter + DANGEROUS_PATTERNS в check-permissions.py), inline-ссылка на ADR-019 — единственное убранное (она была self-referential hook, не функциональным guard). +- ADR number = sequential (020), НЕ PR number (эволюция известного паттерна PR#26 docs-reviewer typo). \ No newline at end of file diff --git a/docs/project-map/README.md b/docs/project-map/README.md index 6371474..6f25a7b 100644 --- a/docs/project-map/README.md +++ b/docs/project-map/README.md @@ -74,6 +74,7 @@ opencode-config/ │ └── search.py # Search ├── tests/ # pytest + TS/MJS test suite — PR#17 │ ├── _ts_loader.mjs # TS test loader (load/exec_stub/exec_stub_json/exec_real modes) — PR#38 +│ ├── test_agent_frontmatter.py # Agent frontmatter validators (no top-level doom_loop, permission.doom_loop present, steps:100) — PR#49 │ ├── test_check_adr_refs.py # adr-check.yml validator │ ├── test_check_permissions.py # permissions-check.yml validator │ ├── test_cli.py # src/memory/cli.py diff --git a/tests/test_agent_frontmatter.py b/tests/test_agent_frontmatter.py new file mode 100644 index 0000000..3295b4c --- /dev/null +++ b/tests/test_agent_frontmatter.py @@ -0,0 +1,174 @@ +"""Tests for agent frontmatter validation — top-level vs permission-nested fields. + +Covers issue #48 acceptance criteria: +- ``reviewer.md``, ``docs-reviewer.md``, ``memory-syncer.md`` frontmatter does + NOT contain a top-level ``doom_loop`` field (outside the ``permission:`` block). + A top-level ``doom_loop`` is invalid — it is not in the opencode ``AgentV2.Info`` + schema and gets forwarded to the provider as a model param, causing + ``AI_APICallError: Extra inputs are not permitted`` on strict providers. +- ``doom_loop`` INSIDE the ``permission:`` block IS present in all 3 files + (valid — it is the real permission guard). +- ``steps: 100`` is present and unchanged in all 3 files. + +PyYAML is not a project dependency, so frontmatter is parsed manually by +splitting on ``---`` delimiters and tracking indentation (top-level fields have +no indent; ``permission:``-nested fields have 2-space indent). This mirrors the +parsing approach used in ``.opencode/scripts/check-permissions.py``. +""" + +from pathlib import Path + +REPO_ROOT = Path(__file__).resolve().parent.parent +AGENTS_DIR = REPO_ROOT / ".opencode" / "agents" + +AGENT_FILES = [ + AGENTS_DIR / "reviewer.md", + AGENTS_DIR / "docs-reviewer.md", + AGENTS_DIR / "memory-syncer.md", +] + + +def _parse_frontmatter(filepath: Path) -> dict: + """Parse YAML frontmatter into a nested dict (manual, no pyyaml). + + Returns a dict where top-level keys map to either a scalar string or a + nested dict (for indented blocks like ``permission:``). + """ + content = filepath.read_text() + parts = content.split("---", 2) + assert len(parts) >= 3, f"{filepath.name}: no frontmatter block found" + frontmatter = parts[1] + return _parse_yaml_block(frontmatter) + + +def _parse_yaml_block(text: str) -> dict: + """Minimal YAML parser for the subset used in agent frontmatter. + + Handles: top-level ``key: value``, nested ``key:`` blocks (indented), + and nested ``key: value`` (indented). Values are strings; nested blocks + become dicts. Does NOT handle lists, quoting, or complex YAML — agent + frontmatter only uses the simple key-value subset. + """ + result: dict = {} + current_dict: dict | None = None + for line in text.split("\n"): + if not line.strip() or line.strip().startswith("#"): + continue + stripped = line.lstrip() + indent = len(line) - len(stripped) + if indent == 0: + current_dict = None + if ":" in stripped: + key, _, value = stripped.partition(":") + key = key.strip() + value = value.strip() + if value: + result[key] = value + else: + current_dict = {} + result[key] = current_dict + elif current_dict is not None and indent >= 2: + if ":" in stripped: + key, _, value = stripped.partition(":") + key = key.strip() + value = value.strip() + if value: + current_dict[key] = value + return result + + +# ── all agent files exist ─────────────────────────────────────────────────── + + +def test_agent_files_exist(): + """All 3 agent files exist in .opencode/agents/.""" + for f in AGENT_FILES: + assert f.exists(), f"agent file missing: {f}" + + +# ── no top-level doom_loop (issue #48 — the invalid field) ────────────────── + + +def test_no_top_level_doom_loop(): + """Frontmatter of all 3 agents has NO top-level ``doom_loop`` key. + + A top-level ``doom_loop`` is invalid — outside ``permission:`` it is not + a recognised agent field and gets forwarded to the provider. + """ + for f in AGENT_FILES: + fm = _parse_frontmatter(f) + assert "doom_loop" not in fm, ( + f"{f.name}: top-level 'doom_loop' field must be removed — it is " + "invalid outside the permission: block" + ) + + +def test_no_top_level_doom_loop_raw(): + """Raw text check: no line ``doom_loop:`` at indent 0 in any agent file. + + Belt-and-suspenders alongside the parsed check — catches edge cases where + the parser might miss something. + """ + for f in AGENT_FILES: + content = f.read_text() + parts = content.split("---", 2) + assert len(parts) >= 3, f"{f.name}: no frontmatter" + fm = parts[1] + for line in fm.split("\n"): + if line.startswith("doom_loop:"): + raise AssertionError( + f"{f.name}: top-level 'doom_loop:' line found at indent 0 — " + f"must be removed: {line!r}" + ) + + +# ── permission.doom_loop present (the valid guard) ────────────────────────── + + +def test_permission_doom_loop_present(): + """``doom_loop: deny`` INSIDE ``permission:`` is present in all 3 files. + + This is the valid permission guard — it must NOT be removed. + """ + for f in AGENT_FILES: + fm = _parse_frontmatter(f) + assert "permission" in fm, f"{f.name}: no 'permission' block" + permission = fm["permission"] + assert isinstance(permission, dict), f"{f.name}: 'permission' is not a block" + assert "doom_loop" in permission, ( + f"{f.name}: 'doom_loop' missing from permission: block — must be kept" + ) + assert permission["doom_loop"] == "deny", ( + f"{f.name}: permission.doom_loop must be 'deny', got {permission['doom_loop']!r}" + ) + + +# ── steps: 100 present and unchanged ───────────────────────────────────────── + + +def test_steps_100_present(): + """``steps: 100`` is present in all 3 agent files (must not be changed).""" + for f in AGENT_FILES: + fm = _parse_frontmatter(f) + assert "steps" in fm, f"{f.name}: 'steps' field missing" + assert fm["steps"] == "100", f"{f.name}: steps must be '100', got {fm['steps']!r}" + + +# ── frontmatter is well-formed (parses without error) ─────────────────────── + + +def test_frontmatter_parseable(): + """All 3 agent files have parseable frontmatter (split by ---).""" + for f in AGENT_FILES: + content = f.read_text() + parts = content.split("---", 2) + assert len(parts) >= 3, f"{f.name}: frontmatter not delimited by ---" + fm = _parse_frontmatter(f) + assert "description" in fm, f"{f.name}: 'description' field missing" + assert "mode" in fm, f"{f.name}: 'mode' field missing" + + +if __name__ == "__main__": + import pytest + + pytest.main([__file__, "-v"])