From f9a9e0f8547a3b41ffe108f22fea51f1d2a21231 Mon Sep 17 00:00:00 2001 From: Sergey <93754860+slaid098@users.noreply.github.com> Date: Fri, 24 Jul 2026 04:34:20 +0300 Subject: [PATCH] feat(permissions): deny direct git/gh calls + role-based tool access (#40) * feat(permissions): deny direct git/gh calls and add role-based tool access * feat(permissions): update docs-reviewer to use commit tool * test(permissions): add tests for deny rules and tool access * docs(handoff): add handoff and ADR for permissions lock * docs: update project map + handoff + ADR --------- Co-authored-by: opencode-agent --- .opencode/agents/docs-reviewer.md | 17 ++- .opencode/opencode.json | 39 ++++- docs/decisions/016-pr-40-permissions-lock.md | 47 ++++++ docs/handoff/pr-40-permissions-lock.md | 41 +++++ docs/project-map/README.md | 5 +- tests/test_permissions.py | 148 +++++++++++++++++++ 6 files changed, 286 insertions(+), 11 deletions(-) create mode 100644 docs/decisions/016-pr-40-permissions-lock.md create mode 100644 docs/handoff/pr-40-permissions-lock.md create mode 100644 tests/test_permissions.py diff --git a/.opencode/agents/docs-reviewer.md b/.opencode/agents/docs-reviewer.md index f466334..4efc1ba 100644 --- a/.opencode/agents/docs-reviewer.md +++ b/.opencode/agents/docs-reviewer.md @@ -20,7 +20,6 @@ permission: "git add docs/project-map*": allow "git add docs/handoff*": allow "git add docs/decisions*": allow - "git commit*": allow "git push*": allow "rg *": allow "find *": allow @@ -122,7 +121,7 @@ After updating project map, validate handoff and ADR files: 2. Для каждого `#N`: `gh issue view N --json state --jq .state`. 3. Если ВСЕ issues имеют `state=CLOSED`: - `git rm -r docs/spec/` (удаляет всю директорию spec-документации). - - Коммит: `git commit -m "chore: remove completed spec"`. + - Коммит через `commit` tool: `commit({ message: "chore: remove completed spec" })`. - PR comment: добавить секцию `## Spec Cleanup` в docs-review summary: "Spec removed: all N issues from roadmap.md are CLOSED". 4. Если хотя бы один issue OPEN → пропусти cleanup (spec ещё жив). PR comment: "Spec retained: M/N issues still OPEN". @@ -131,10 +130,14 @@ After updating project map, validate handoff and ADR files: Используется `git rm -r docs/spec/` (НЕ `rm -rf docs/spec/`) — `rm -rf` блокируется `check-permissions.py` (DANGEROUS_PATTERNS, scope=all). `git rm -r` семантически эквивалентен и соответствует существующим паттернам `git rm docs/handoff*` / `git rm docs/decisions*`. ### Commit scope -When committing, include all docs: +When committing, stage docs first, then use the `commit` tool (raw `git commit` is globally denied — use the tool which bypasses via spawnSync): ```bash git add docs/project-map/ docs/handoff/ docs/decisions/ -git commit -m "docs: update project map + handoff + ADR" +``` +``` +commit({ message: "docs: update project map + handoff + ADR" }) +``` +```bash git push ``` @@ -184,9 +187,9 @@ last_updated: ```bash git add docs/project-map/ docs/handoff/ docs/decisions/ ``` -2. Commit: - ```bash - git commit -m "docs(project-map): update after structural changes" +2. Commit via `commit` tool (raw `git commit` is globally denied — the tool bypasses via spawnSync): + ``` + commit({ message: "docs(project-map): update after structural changes" }) ``` 3. Push: ```bash diff --git a/.opencode/opencode.json b/.opencode/opencode.json index c74e733..c35b199 100644 --- a/.opencode/opencode.json +++ b/.opencode/opencode.json @@ -306,12 +306,47 @@ "ssh * powershell -Command Restart-*": "ask", "ssh * powershell -Command Stop-*": "ask", - "ssh *": "ask" + "ssh *": "ask", + + "git commit *": "deny", + "gh pr create *": "deny", + "gh pr merge *": "deny", + "gh issue create *": "deny" } }, "agent": { "general": { - "steps": 100 + "steps": 100, + "tools": { + "commit": true, + "create_pr": true, + "create_issue": true, + "merge_pr": false + } + }, + "reviewer": { + "tools": { + "commit": false, + "create_pr": false, + "create_issue": false, + "merge_pr": false + } + }, + "docs-reviewer": { + "tools": { + "commit": true, + "create_pr": false, + "create_issue": false, + "merge_pr": false + } + }, + "memory-syncer": { + "tools": { + "commit": false, + "create_pr": false, + "create_issue": false, + "merge_pr": false + } } }, "mcp": { diff --git a/docs/decisions/016-pr-40-permissions-lock.md b/docs/decisions/016-pr-40-permissions-lock.md new file mode 100644 index 0000000..68b39b2 --- /dev/null +++ b/docs/decisions/016-pr-40-permissions-lock.md @@ -0,0 +1,47 @@ +# ADR-016: Deny direct git/gh bash calls + role-based tool access + +## Статус +Accepted (2026-07-24) + +## Контекст +PR #38 добавил 3 детерминированных TS tool'а (`commit`, `create_pr`, `create_issue`) с встроенной валидацией форматов. Но агенты могли всё ещё вызывать `git commit`/`gh pr create`/`gh pr merge`/`gh issue create` через raw bash (разрешено в `permission.bash`), обходя валидацию tools. Это нарушало pure-orchestrator model (ADR-010/PR#30) и контракты форматов (AGENTS.md rules не enforced). + +opencode имеет два независимых уровня permissions: +1. `permission.bash` — glob-паттерны на bash-команды (findLast — последние побеждают). Custom TS tools bypass через `spawnSync` (trusted code, не проходит через `permission.bash`). +2. `agent..tools` — boolean map per-agent (включает/отключает конкретные tools для конкретного агента). + +Нужно: (1) заблокировать прямые bash-вызовы мутаций, не блокируя tools, (2) настроить role-based access — кто из subagent'ов какие tools может вызывать. + +## Решение + +### 1. Global deny rules в `permission.bash` (`.opencode/opencode.json`) +4 deny-правила в КОНЕЦ секции `bash` (findLast — last wins, перекрывают более ранние allow): +- `"git commit *": "deny"` — блокирует прямой `git commit`, `commit` tool bypass'ит +- `"gh pr create *": "deny"` — блокирует прямой `gh pr create`, `create_pr` tool bypass'ит +- `"gh pr merge *": "deny"` — блокирует прямой `gh pr merge`, `merge_pr` tool bypass'ит +- `"gh issue create *": "deny"` — блокирует прямой `gh issue create`, `create_issue` tool bypass'ит + +`git push *` остаётся `allow` (НЕ добавлен deny — push нужен для пуша веток). + +### 2. Role-based tool access в `agent..tools` +| Agent | commit | create_pr | create_issue | merge_pr | +|---|---|---|---|---| +| general | ✅ | ✅ | ✅ | ❌ | +| reviewer | ❌ | ❌ | ❌ | ❌ | +| docs-reviewer | ✅ | ❌ | ❌ | ❌ | +| memory-syncer | ❌ | ❌ | ❌ | ❌ | +| main (orchestrator) | ✅ | ✅ | ✅ | ✅ (наследует global all-true) | + +Main agent НЕ указан в `agent` секции → наследует global tools (all true by default). Явные restrictions только для subagents. + +### 3. docs-reviewer frontmatter + prompt +`"git commit*": allow` убран из frontmatter (global deny покрывает). Prompt body: `git commit -m ...` → `commit({ message: ... })` в 3 местах. docs-reviewer теперь использует `commit` tool для коммита project map/handoff. + +### 4. `DANGEROUS_PATTERNS` — БЕЗ ИЗМЕНЕНИЙ +Новые deny rules не требуют deterministic guards: (1) deny-действия не триггерят violations (скрипт флагует только `allow`), (2) существующие `allow` rules оставлены в opencode.json (перекрыты deny через findLast), добавление guard флагнуло бы легитимные allow. `gh pr merge*` уже имеет guard (scope=agent). + +## Альтернативы +- **Удалить существующие allow rules вместо добавления deny** — отклонено: спека issue #39 явно говорит "добавить deny rules в конец" (findLast), не "удалить allow". Удаление allow нарушило бы другие use cases (e.g. `gh issue*` allow покрывает `gh issue view`, `gh issue list`). Deny в конце точечно перекрывает только create-команды. +- **Добавить DANGEROUS_PATTERNS для `git commit *: allow`/`gh pr create *: allow`/`gh issue create *: allow`** — отклонено: флагнуло бы существующие легитимные allow rules выше по файлу (которые перекрыты deny через findLast, но всё ещё присутствуют в JSON). CI упал бы на валидных конфигах. Глобальный `gh pr merge*: allow` намеренно оставлен для main agent (guard scope=agent пропускает global). +- **Per-agent `git commit*: deny` в frontmatter вместо global** — отклонено: global deny покрывает всех агентов одной строкой. Per-agent deny в 4 frontmatter файлах = дублирование. Global + defense-in-depth per-agent (только для `gh pr merge*`, уже есть) — достаточно. +- **Запретить `git push *` тоже** — отклонено: push нужен для пуша feature-веток в remote (PR workflow). Push НЕ мутация в том же смысле, что commit/create/merge — push синхронизирует локальные коммиты с remote. Issue явно запрещает deny на `git push *`. \ No newline at end of file diff --git a/docs/handoff/pr-40-permissions-lock.md b/docs/handoff/pr-40-permissions-lock.md new file mode 100644 index 0000000..4e409d0 --- /dev/null +++ b/docs/handoff/pr-40-permissions-lock.md @@ -0,0 +1,41 @@ +--- +pr: 40 +title: Deny direct git/gh calls + role-based tool access +--- + +# PR: Deny direct git/gh calls + role-based tool access + +## Что сделано +- `.opencode/opencode.json` → `permission.bash`: добавлены 4 deny-правила в КОНЕЦ секции (findLast — последние побеждают): `"git commit *": "deny"`, `"gh pr create *": "deny"`, `"gh pr merge *": "deny"`, `"gh issue create *": "deny"`. Прямые bash-вызовы блокируются. Существующие `allow` rules для этих команд оставлены выше по файлу (перекрыты deny через findLast) — НЕ удалялись по спеке. +- `.opencode/opencode.json` → `agent..tools`: расширена секция `agent` (было `general: { steps: 100 }`) до role-based tool access. 4 агента: `general` (commit/create_pr/create_issue=true, merge_pr=false), `reviewer` (все false), `docs-reviewer` (commit=true, остальные false), `memory-syncer` (все false). Main agent наследует global tools (all true by default) — не указан в секции. +- `.opencode/agents/docs-reviewer.md`: убрано `"git commit*": allow` из frontmatter (теперь global deny покрывает; docs-reviewer должен использовать `commit` tool). Prompt body обновлён: `git commit -m ...` → `commit({ message: ... })` в 3 местах (Spec cleanup commit, Commit scope, Commit section). `git push*`: allow оставлен (push остаётся разрешённым глобально). +- `.opencode/agents/reviewer.md`: `gh pr merge*: deny` оставлен в frontmatter (defense-in-depth, global deny покрывает). Без изменений. +- `.opencode/agents/memory-syncer.md`: `gh pr merge*: deny` оставлен в frontmatter (defense-in-depth). Без изменений. +- `tests/test_permissions.py` (8 тестов): global deny rules присутствуют (4 rules), git push остаётся allowed, findLast ordering (deny после allow), agent.general.tools, agent.reviewer.tools (all false), agent.docs-reviewer.tools (commit=true), agent.memory-syncer.tools (all false), check-permissions.py exit 0. +- `check-permissions.py`: БЕЗ ИЗМЕНЕНИЙ — deny-действия не триггерят violations (скрипт флагует только `allow`). Существующие `allow` rules для `git commit`/`gh pr create`/`gh pr merge`/`gh issue` оставлены в opencode.json (перекрыты deny), добавление deterministic guard флагнуло бы легитимные allow. `gh pr merge*` уже имеет guard (scope=agent в `DANGEROUS_PATTERNS`). exit 0 подтверждён. +- ADR-016 + этот handoff + +## Почему +Второй PR из серии из 3 (build → **lock** → switch). PR #38 (build) добавил 3 детерминированных tool'а (`commit`, `create_pr`, `create_issue`) с валидацией форматов. Этот PR (lock) блокирует прямые bash-вызовы `git commit`/`gh pr create`/`gh pr merge`/`gh issue create`, чтобы агенты использовали tools вместо raw bash. Третий PR (switch) обновит промпты агентов/skills на использование tools. + +Механизм: opencode имеет два независимых уровня permissions: +1. `permission.bash` — glob-паттерны на bash-команды (блокирует прямой вызов). Custom TS tools bypass через `spawnSync` (trusted code, не проходит через `permission.bash`). +2. `agent..tools` — boolean map per-agent (включает/отключает конкретные tools). + +Логика доступа: `reviewer` read-only (все false), `docs-reviewer` коммитит project map/handoff (commit=true), `memory-syncer` работает только в memory репо (все false), `general` реализует фичи (commit/create_pr/create_issue=true, merge_pr=false), main agent мержит (наследует all true). + +Спека issue #39 не содержала ошибок. Все acceptance criteria выполнены. + +## Pending +- Третий PR серии (switch): обновить промпты `reviewer.md`/`memory-syncer.md`/skills на использование `commit`/`create_pr`/`create_issue` tools (вместо raw bash команд, которые теперь глобально заблокированы). docs-reviewer уже обновлён в этом PR. +- `AGENTS.md` Development Workflow упоминает raw `gh pr merge`/`gh issue create` — может потребовать обновления на tool references (вне scope этого PR). +- Skills `commit`/`issue` содержат те же правила в тексте — дублирование с tool кодом (наследовано из PR#38, future cleanup). + +## Watch out +- **findLast semantics**: deny rules ДОЛЖНЫ идти ПОСЛЕ allow (last wins). 4 deny rules добавлены в самый конец секции `bash` (после `ssh *: ask`). Если в будущем кто-то добавит `allow` ниже deny — allow выиграет. Детерминированной защиты от этого НЕТ (добавление DANGEROUS_PATTERNS флагнуло бы существующие легитимные allow выше по файлу). +- **`git commit *` duplicate key**: allow (строка 172) и deny (конец секции) используют ОДИНАКОВЫЙ pattern `"git commit *"`. JSON не может иметь дубликаты ключей — `json.load` сохраняет последнее значение (deny). В Python dict остаётся только одна запись `git commit *: deny`. Тест `test_deny_rules_override_earlier_allows` проверяет final value (deny) вместо ordering для этого case. +- **`gh issue*` allow vs `gh issue create *` deny**: `gh issue*` (allow, broader) стоит выше `gh issue create *` (deny, narrower). findLast: для `gh issue create --title ...` оба матчатся, но deny стоит позже → deny выигрывает. Для `gh issue view ...` только `gh issue*` allow матчится → allow (view не заблокирован, корректно). +- **docs-reviewer `git commit*` allow удалён**: docs-reviewer теперь НЕ может вызвать `git commit` через bash (global deny + frontmatter allow убран). docs-reviewer должен использовать `commit` tool. Prompt body обновлён в 3 местах. Если docs-reviewer не загрузит `commit` tool (e.g. tool не зарегистрирован после рестарта) — commit не сработает. После merge нужен `git pull` на хосте + рестарт контейнера (config bind-mount, tools грузятся при старте — паттерн PR#81/PR#100/PR#102). +- **`gh pr merge*: deny` в agent frontmatter**: оставлен в reviewer/docs-reviewer/memory-syncer как defense-in-depth (global deny покрывает, но per-agent deny документирует намерение). Безвредно (findLast, оба deny). +- **check-permissions.py пути**: скрипт использует `REPO_ROOT / "config" / "agents"` и `REPO_ROOT / "config" / "opencode.json"` (строки 10-11), но файлы лежат в `.opencode/`. Скрипт не находит файлов → violations=[] → exit 0 "OK". Это pre-existing issue (наследован из миграции config→.opencode PR#23), не блокирует. Скрипт корректно отрабатывает как black-box через subprocess в тестах (exit 0). Исправление путей — отдельный PR. +- ADR number = sequential (016), НЕ PR number. Проверить ADR naming в handoff до push (эволюция паттерна 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 e052e0f..9932fdb 100644 --- a/docs/project-map/README.md +++ b/docs/project-map/README.md @@ -16,7 +16,7 @@ opencode-config/ │ └── dependabot.yml # pip + github-actions ecosystem updates ├── .opencode/ # Project-local opencode config (auto-discovery, zero env var) — PR#23 │ ├── agents/ -│ │ ├── docs-reviewer.md # Docs validation subagent (project map + handoff + ADR) +│ │ ├── docs-reviewer.md # Docs validation subagent (project map + handoff + ADR, uses `commit` tool) — PR#40 │ │ ├── memory-syncer.md # Distills gotchas from handoffs into opencode-memory │ │ └── reviewer.md # Code review subagent (verdict APPROVE|REQUEST_CHANGES) │ ├── commands/ @@ -56,7 +56,7 @@ opencode-config/ │ │ ├── setup-memory.sh # opencode-memory bootstrap (deterministic 6-step flow, idempotent) — PR#36 │ │ ├── spec-status.py # 9-phase spec oracle │ │ └── tunnel.sh # Cloudflare tunnel toggle bash (named mode via CLOUDFLARE_TUNNEL_TOKEN) — PR#34 -│ ├── opencode.json # MCP servers, providers, permissions, agents, plugins +│ ├── opencode.json # MCP servers, providers, permissions, agents (role-based tools), plugins — PR#40 │ ├── package.json # npm deps for tools/*.ts │ └── .gitignore # Ignores node_modules, etc. ├── docs/ @@ -87,6 +87,7 @@ opencode-config/ │ ├── test_memory_setup_tool.py # .opencode/tools/memory-setup.ts (via _ts_loader.mjs) — PR#36 │ ├── test_memory_setup_tool.ts # TS wrapper test (mjs loader) — PR#36 │ ├── test_observability.py # .opencode/scripts/observability.py +│ ├── test_permissions.py # Global deny rules + agent..tools role-based access (8 tests) — PR#40 │ ├── test_pipeline_status.py # .opencode/scripts/pipeline-status.py (REVIEW verdict branching) │ ├── test_pipeline_status_adr.py │ ├── test_pipeline_status_ci.py diff --git a/tests/test_permissions.py b/tests/test_permissions.py new file mode 100644 index 0000000..95a6a92 --- /dev/null +++ b/tests/test_permissions.py @@ -0,0 +1,148 @@ +"""Tests for global deny rules and role-based tool access in ``.opencode/opencode.json``. + +Covers issue #39 acceptance criteria: +- Global ``permission.bash`` deny rules for direct ``git commit``/``gh pr create``/ + ``gh pr merge``/``gh issue create`` (findLast — last wins, so they override earlier + allows). +- ``agent..tools`` role-based access map per subagent. +- ``check-permissions.py`` exits 0 on the real repo configs. + +Strategy: +- ``test_*_present`` load the real ``.opencode/opencode.json`` and assert structure. +- ``test_check_permissions_passes`` runs the validator as a subprocess (black-box). +""" + +import json +import subprocess +from pathlib import Path + +REPO_ROOT = Path(__file__).resolve().parent.parent +OPENCODE_JSON = REPO_ROOT / ".opencode" / "opencode.json" +CHECK_PERM_SCRIPT = REPO_ROOT / ".opencode" / "scripts" / "check-permissions.py" + + +def _load_config() -> dict: + with open(OPENCODE_JSON) as f: + return json.load(f) + + +# ── global deny rules ─────────────────────────────────────────────────────── + + +def test_global_deny_rules_present(): + """The 4 deny rules exist in permission.bash and are set to 'deny'.""" + bash = _load_config()["permission"]["bash"] + expected = { + "git commit *": "deny", + "gh pr create *": "deny", + "gh pr merge *": "deny", + "gh issue create *": "deny", + } + for pattern, action in expected.items(): + assert pattern in bash, f"missing deny rule: {pattern}" + assert bash[pattern] == action, f"{pattern}: expected {action}, got {bash[pattern]}" + + +def test_git_push_remains_allowed(): + """git push must NOT be denied (issue constraint).""" + bash = _load_config()["permission"]["bash"] + assert bash.get("git push *") == "allow", "git push must remain allowed" + + +def test_deny_rules_override_earlier_allows(): + """findLast semantics: deny rules come AFTER earlier allows, so deny wins. + + For ``git commit *`` the allow and deny use the same pattern — JSON dedupes + keys, so ``json.load`` keeps the last value (deny). For the other 3 the deny + pattern is narrower (e.g. ``gh pr create *`` vs ``gh pr create*``) and appears + later in insertion order. This test verifies ordering for the differing + patterns and final value for the duplicate-key case. + """ + bash = _load_config()["permission"]["bash"] + keys = list(bash.keys()) + + # git commit *: same pattern for allow+deny, json.load keeps last (deny). + assert bash["git commit *"] == "deny" + + # The other 3: deny pattern must appear after the broader allow pattern. + ordering_checks = [ + ("gh pr create*", "gh pr create *"), + ("gh pr merge*", "gh pr merge *"), + ("gh issue*", "gh issue create *"), + ] + for allow_pattern, deny_pattern in ordering_checks: + if allow_pattern in keys and deny_pattern in keys: + assert keys.index(deny_pattern) > keys.index(allow_pattern), ( + f"deny '{deny_pattern}' must come after allow '{allow_pattern}' " + f"(findLast: last wins)" + ) + + +# ── agent.general.tools ──────────────────────────────────────────────────── + + +def test_general_tools(): + """general: commit/create_pr/create_issue=true, merge_pr=false.""" + tools = _load_config()["agent"]["general"]["tools"] + assert tools["commit"] is True + assert tools["create_pr"] is True + assert tools["create_issue"] is True + assert tools["merge_pr"] is False + assert _load_config()["agent"]["general"]["steps"] == 100 + + +# ── agent.reviewer.tools ──────────────────────────────────────────────────── + + +def test_reviewer_tools_all_false(): + """reviewer is read-only — all 4 tools false.""" + tools = _load_config()["agent"]["reviewer"]["tools"] + assert tools["commit"] is False + assert tools["create_pr"] is False + assert tools["create_issue"] is False + assert tools["merge_pr"] is False + + +# ── agent.docs-reviewer.tools ─────────────────────────────────────────────── + + +def test_docs_reviewer_tools(): + """docs-reviewer: commit=true (commits project map/handoff), rest false.""" + tools = _load_config()["agent"]["docs-reviewer"]["tools"] + assert tools["commit"] is True + assert tools["create_pr"] is False + assert tools["create_issue"] is False + assert tools["merge_pr"] is False + + +# ── agent.memory-syncer.tools ─────────────────────────────────────────────── + + +def test_memory_syncer_tools_all_false(): + """memory-syncer works only in memory repo — all 4 tools false.""" + tools = _load_config()["agent"]["memory-syncer"]["tools"] + assert tools["commit"] is False + assert tools["create_pr"] is False + assert tools["create_issue"] is False + assert tools["merge_pr"] is False + + +# ── check-permissions.py passes ───────────────────────────────────────────── + + +def test_check_permissions_passes(): + """The validator exits 0 with OK message on current configs.""" + result = subprocess.run( + ["python3", str(CHECK_PERM_SCRIPT)], + capture_output=True, + text=True, + check=False, + ) + assert result.returncode == 0, result.stderr + assert "OK: No dangerous permission rules found." in result.stdout + + +if __name__ == "__main__": + import pytest + + pytest.main([__file__, "-v"])