diff --git a/.opencode/tools/_shared.ts b/.opencode/tools/_shared.ts new file mode 100644 index 0000000..ddd2512 --- /dev/null +++ b/.opencode/tools/_shared.ts @@ -0,0 +1,39 @@ +import { spawnSync } from "child_process" + +/** + * Build the `--repo ` argv fragment for `gh`. + * + * If `repo` is provided, returns `["--repo", repo]` (explicit target). + * If omitted, returns `[]` so `gh` auto-detects the repo from its `cwd` + * (set to `context.worktree` by `runGh`). Auto-detect is the default and + * backward-compatible behaviour for all 5 GitHub tools (create-issue, + * create-pr, post-review, post-docs-review, merge-pr) — see ADR-027. + */ +export function parseRepo(repo?: string): string[] { + if (repo) return ["--repo", repo] + return [] +} + +/** + * Run `gh` with the given subcommand args, optionally targeting an explicit + * repo. When `repo` is omitted, `gh` auto-detects the repo from `opts.cwd` + * (callers pass `context.worktree`). Returns the raw spawnSync result so the + * caller can inspect `status`/`stdout`/`stderr` directly, or pass it to + * `formatResult` for the standard error string. + */ +export function runGh(args: string[], repo?: string, opts?: { cwd?: string }) { + const fullArgs = [...parseRepo(repo), ...args] + return spawnSync("gh", fullArgs, { encoding: "utf-8", cwd: opts?.cwd }) +} + +/** + * Standard success/error formatter for GitHub tools. + * + * On success (exit 0) returns `stdout.trim()`. On failure returns the + * canonical error string `⚠️ failed (exit ): `, matching the format previously duplicated across 5 tools. + */ +export function formatResult(r: { status: number | null; stdout: string; stderr: string }, toolName: string): string { + if (r.status === 0) return r.stdout.trim() + return `⚠️ ${toolName} failed (exit ${r.status}): ${r.stderr || r.stdout}` +} \ No newline at end of file diff --git a/.opencode/tools/create-issue.ts b/.opencode/tools/create-issue.ts index ae1c48a..94bf9d0 100644 --- a/.opencode/tools/create-issue.ts +++ b/.opencode/tools/create-issue.ts @@ -1,5 +1,5 @@ -import { spawnSync } from "child_process" import { tool } from "@opencode-ai/plugin" +import { runGh, formatResult } from "./_shared" const TITLE_REGEX = /^(feat|fix|chore|docs|refactor|test|style|perf)\([^)]+\): .{1,80}$/ const CYRILLIC = /[\u0400-\u04FF]/ @@ -20,6 +20,7 @@ export default tool({ title: tool.schema.string().describe("Issue title (conventional format: type(scope): description, <=80 chars)"), body: tool.schema.string().describe("Issue body in Russian with ## Контекст, ## Задача, ## Критерии приемки headings"), labels: tool.schema.array(tool.schema.string()).optional().describe("Labels to assign (e.g. ['bug', 'enhancement'])"), + repo: tool.schema.string().optional().describe("Optional repo (owner/name). If omitted, gh auto-detects from context.worktree."), }, async execute(args, context) { const title = args.title @@ -49,12 +50,9 @@ export default tool({ ghArgs.push("--label", args.labels.join(",")) } - const r = spawnSync("gh", ghArgs, { - encoding: "utf-8", - cwd: context.worktree, - }) + const r = runGh(ghArgs, args.repo, { cwd: context.worktree }) if (r.status !== 0) { - return `⚠️ gh issue create failed (exit ${r.status}): ${r.stderr || r.stdout}` + return formatResult(r, "gh issue create") } return `Issue created: ${r.stdout.trim()}` diff --git a/.opencode/tools/create-pr.ts b/.opencode/tools/create-pr.ts index 23d9e29..7caf04c 100644 --- a/.opencode/tools/create-pr.ts +++ b/.opencode/tools/create-pr.ts @@ -1,5 +1,5 @@ -import { spawnSync } from "child_process" import { tool } from "@opencode-ai/plugin" +import { runGh, formatResult } from "./_shared" const TITLE_REGEX = /^(feat|fix|chore|docs|refactor|test|style|perf)\([^)]+\): .{1,72}$/ const CYRILLIC = /[\u0400-\u04FF]/ @@ -19,6 +19,7 @@ export default tool({ title: tool.schema.string().describe("PR title (conventional format: type(scope): description)"), body: tool.schema.string().describe("PR body in Russian with ## Что сделано and ## Почему headings"), issue_number: tool.schema.number().optional().describe("Issue number to link (appends 'Closes #N' to body)"), + repo: tool.schema.string().optional().describe("Optional repo (owner/name). If omitted, gh auto-detects from context.worktree."), }, async execute(args, context) { const title = args.title @@ -44,12 +45,9 @@ export default tool({ body = body + "\n\nCloses #" + args.issue_number } - const r = spawnSync("gh", ["pr", "create", "--title", title, "--body", body], { - encoding: "utf-8", - cwd: context.worktree, - }) + const r = runGh(["pr", "create", "--title", title, "--body", body], args.repo, { cwd: context.worktree }) if (r.status !== 0) { - return `⚠️ gh pr create failed (exit ${r.status}): ${r.stderr || r.stdout}` + return formatResult(r, "gh pr create") } return `PR created: ${r.stdout.trim()}` diff --git a/.opencode/tools/merge-pr.ts b/.opencode/tools/merge-pr.ts index 91f6151..5aefcb3 100644 --- a/.opencode/tools/merge-pr.ts +++ b/.opencode/tools/merge-pr.ts @@ -1,19 +1,18 @@ -import { spawnSync } from "child_process" import { tool } from "@opencode-ai/plugin" +import { runGh } from "./_shared" export default tool({ description: "Merge a PR via squash + delete branch. Orchestrator-safe wrapper for `gh pr merge N --squash --delete-branch`. Main agent calls this tool instead of raw bash, aligning with the pure-orchestrator model (tool-led philosophy, see ADR-010).", args: { pr_number: tool.schema.number().describe("PR number to merge"), + repo: tool.schema.string().optional().describe("Optional repo (owner/name). If omitted, gh auto-detects from context.worktree."), }, async execute(args, context) { - const r = spawnSync("gh", [ - "pr", "merge", String(args.pr_number), - "--squash", "--delete-branch", - ], { - encoding: "utf-8", - cwd: context.worktree, - }) + const r = runGh( + ["pr", "merge", String(args.pr_number), "--squash", "--delete-branch"], + args.repo, + { cwd: context.worktree }, + ) if (r.status !== 0) { return `⚠️ merge_pr failed for PR #${args.pr_number} (exit ${r.status}): ${r.stderr || r.stdout}` @@ -21,4 +20,4 @@ export default tool({ return `PR #${args.pr_number} merged successfully (squash, branch deleted).` }, -}) +}) \ No newline at end of file diff --git a/.opencode/tools/post-docs-review.ts b/.opencode/tools/post-docs-review.ts index e2a054b..01b16e8 100644 --- a/.opencode/tools/post-docs-review.ts +++ b/.opencode/tools/post-docs-review.ts @@ -1,5 +1,5 @@ -import { spawnSync } from "child_process" import { tool } from "@opencode-ai/plugin" +import { runGh } from "./_shared" const VERDICTS = ["APPROVE", "FIXED", "NO_CHANGES"] as const type Verdict = typeof VERDICTS[number] @@ -10,13 +10,11 @@ export default tool({ pr_number: tool.schema.number().describe("PR number to comment on"), verdict: tool.schema.enum(VERDICTS).describe("Docs review verdict: APPROVE, FIXED, or NO_CHANGES"), body: tool.schema.string().describe("Docs review body text (without heading — heading is auto-generated)"), + repo: tool.schema.string().optional().describe("Optional repo (owner/name). If omitted, gh auto-detects from context.worktree."), }, async execute(args, context) { const comment = `## Docs Review Summary\n\n${args.body}\n\n### Verdict: ${args.verdict}` - const r = spawnSync("gh", ["pr", "comment", String(args.pr_number), "--body", comment], { - encoding: "utf-8", - cwd: context.worktree, - }) + const r = runGh(["pr", "comment", String(args.pr_number), "--body", comment], args.repo, { cwd: context.worktree }) if (r.status !== 0) { return `⚠️ post-docs-review failed for PR #${args.pr_number} (exit ${r.status}): ${r.stderr || r.stdout}` } diff --git a/.opencode/tools/post-review.ts b/.opencode/tools/post-review.ts index ceb23e5..ebb906e 100644 --- a/.opencode/tools/post-review.ts +++ b/.opencode/tools/post-review.ts @@ -1,5 +1,5 @@ -import { spawnSync } from "child_process" import { tool } from "@opencode-ai/plugin" +import { runGh } from "./_shared" const VERDICTS = ["APPROVE", "REQUEST_CHANGES", "NEEDS_DISCUSSION"] as const type Verdict = typeof VERDICTS[number] @@ -10,13 +10,11 @@ export default tool({ pr_number: tool.schema.number().describe("PR number to comment on"), verdict: tool.schema.enum(VERDICTS).describe("Review verdict: APPROVE, REQUEST_CHANGES, or NEEDS_DISCUSSION"), body: tool.schema.string().describe("Review body text (without heading — heading is auto-generated)"), + repo: tool.schema.string().optional().describe("Optional repo (owner/name). If omitted, gh auto-detects from context.worktree."), }, async execute(args, context) { const comment = `## Code Review Summary\n\n${args.body}\n\n### Verdict: ${args.verdict}` - const r = spawnSync("gh", ["pr", "comment", String(args.pr_number), "--body", comment], { - encoding: "utf-8", - cwd: context.worktree, - }) + const r = runGh(["pr", "comment", String(args.pr_number), "--body", comment], args.repo, { cwd: context.worktree }) if (r.status !== 0) { return `⚠️ post-review failed for PR #${args.pr_number} (exit ${r.status}): ${r.stderr || r.stdout}` } diff --git a/docs/decisions/027-pr-65-tools-refactor-shared-module.md b/docs/decisions/027-pr-65-tools-refactor-shared-module.md new file mode 100644 index 0000000..7614789 --- /dev/null +++ b/docs/decisions/027-pr-65-tools-refactor-shared-module.md @@ -0,0 +1,156 @@ +# ADR-027: Shared module for GitHub tools + repo parameter + tunnel tests + +## Статус + +Accepted (2026-07-25) + +## Контекст + +Research (subagent explore, PR#63) выявил 3 проблемы в GitHub tools +инфраструктуре репо `slaid098/opencode-config`: + +### Проблема 1: Дублирование spawnSync логики + +Каждый из 5 GitHub tools (`create-issue.ts`, `create-pr.ts`, `post-review.ts`, +`post-docs-review.ts`, `merge-pr.ts`) дублировал: + +- `spawnSync("gh", [...], { encoding: "utf-8", cwd: context.worktree })` — + ~5 строк на tool +- `if (r.status !== 0) return "⚠️ ... failed (exit ...): ..."` — ~3 строки + на tool +- Итого: ~10 строк × 5 файлов = ~50 строк дублирования + +Дублирование означает: изменение canonical error-формата или spawnSync +options (например, добавление timeout) требует правки 5 файлов, дрейф +вероятен. + +### Проблема 2: Нет `repo` параметра + +Tools работали только через auto-detect: `gh` CLI сам определяет owner/repo +из `git remote get-url origin` в текущей директории (`context.worktree`). +Ограничения: + +- Из не-git-директории (например `/root/workspace`) — `gh` не может + определить remote → fall back на hardcoded `--repo` (что было багом + issue #60, исправлено в PR#61 / ADR-025). +- Нет явного способа указать repo при вызове tool — теряется гибкость. +- Для pipeline оркестрации было бы полезно передавать repo явно (единый + стандарт вместо полагания на cwd). + +ADR-025 (PR#61) убрал хардкод `--repo slaid098/opencode-config`, оставив +auto-detect. Этот PR делает следующий шаг: `repo` становится опциональным +параметром (явный путь вместо хардкода), auto-detect остаётся default. + +### Проблема 3: Tunnel без тестов + +Tool `tunnel` — единственный из 10 tools без парных `.py` + `.ts` тестов. +Все остальные tools покрыты (`tests/test_commit_tool.py/.ts`, +`tests/test_create_issue_tool.py/.ts`, etc.). Tunnel tool: беспараметровый, +toggle-логика в `tunnel.sh` (PID-файл `/tmp/tunnel.pid`, `kill -0` проверка +живости, stale cleanup, `CLOUDFLARE_TUNNEL_TOKEN` обязателен). Регрессии в +toggle/PID-логике проходят незамеченными. + +## Решение + +### 1. Shared module `.opencode/tools/_shared.ts` + +Создан модуль с 3 функциями: + +- `parseRepo(repo?: string): string[]` — если `repo` передан, возвращает + `["--repo", repo]`; если нет — `[]` (auto-detect через gh из cwd). +- `runGh(args: string[], repo?: string, opts?: { cwd?: string })` — + `spawnSync("gh", [...parseRepo(repo), ...args], { encoding: "utf-8", cwd: + opts?.cwd })`. Prepends `--repo ` к gh argv когда repo явный. +- `formatResult(r, toolName: string): string` — на успехе (exit 0) + `r.stdout.trim()`, на ошибке canonical `⚠️ failed (exit ): + `. + +5 GitHub tools используют `runGh` (устраняет дублирование spawnSync). +`formatResult` используют только `create-issue.ts` / `create-pr.ts` (их +error-формат точно совпадает). `post-review.ts` / `post-docs-review.ts` / +`merge-pr.ts` сохраняют кастомный error с PR номером (`⚠️ ... failed for PR +#N (exit K): ...`) для диагностики — `formatResult` потерял бы PR номер. + +### 2. `repo?: string` параметр (backward-compatible) + +Все 5 GitHub tools приняли опциональный `repo?: string`: + +- При `repo` передан → `runGh` prepends `["--repo", repo]` → gh targetит + явный owner/name независимо от cwd. +- При `repo` omitted → `parseRepo` возвращает `[]` → `--repo` НЕ + добавляется → gh auto-detect'ит из `context.worktree` (cwd) — поведение + идентично pre-refactor (ADR-025 / PR#61). + +Backward-compatibility подтверждена: существующие тесты `test_spawnsync_args` +(ассертят `"--repo" not in args`) + новые `test_repo_omitted_no_repo_flag` +проходят. 51→401 тестов (375 baseline + 26 новых). + +### 3. `_ts_loader.mjs` extension для relative imports + +Loader (`tests/_ts_loader.mjs`) использует `new Function` sandbox и умел +только `child_process` / `path` / `@opencode-ai/plugin`. Relative imports +(`./_shared`) падали с ENOENT. Расширение: + +- `stripTs` стриппает `import { ... } from "./..."` + `export` на top-level + declarations + type annotations в function params (`function foo(a: Type): + Ret {` → `function foo(a) {`). +- `inlineShared()` инлайнит код shared-модуля в sandbox: читает оригинальный + файл для детекта import, грузит + стриппет shared-модуль, prepends к + `new Function` body. Авто-добавление `.ts` к specifier (`./_shared` → + `./_shared.ts`). + +Без этого тесты отрефакторенных tools падали бы — loader не поддерживал +local imports. + +### 4. Tunnel tests + +Созданы `tests/test_tunnel_tool.py` (6 pytest) + `tests/test_tunnel_tool.ts` +(4 TS). `.py` тесты запускают `tunnel.sh` напрямую с изоляцией (копия в +`tmp_path`, переписанные PID/LOG пути, fake `cloudflared` = `sleep 30` на +PATH). Покрыты все ветви: start без токена (exit 1), start с токеном, start +с `TUNNEL_DOMAIN`, stop при живом процессе, stale PID cleanup + start, +toggle (повторный start → stop). + +### 5. 5 test-файлов обновлены для `repo` параметра + +Каждый из 5 tools получил 3 новых кейса: `test_repo_explicit_passed_to_gh` +(`--repo foo/bar` prepended), `test_repo_omitted_no_repo_flag` (no `--repo`, +backward-compat), `test_repo_invalid_gh_error` (invalid repo + gh failure → +tool-specific error). `test_merge_pr_tool.py/.ts` созданы с нуля (merge-pr +не имел тестов ранее) — 8 .py + 5 .ts покрывают базовые кейсы + repo. + +## Альтернативы + +- **Оставить дублирование как есть** — отклонено: ~50 строк дублирования + растёт с каждым новым gh tool, дрейф в error-формате/spawnSync options + вероятен. Shared module — canonical источник, изменение в 1 месте. + +- **Mandatory `repo` параметр (убрать auto-detect)** — отклонено: теряет + backward-compatibility. Существующие вызовы tools (из pipeline, из + агентов) не передают `repo` — полагаются на auto-detect из + `context.worktree`. Mandatory `repo` сломал бы все вызовы, потребовал бы + обновления skills/agents. Опциональный `repo?: string` сохраняет + auto-detect default + даёт явный путь когда нужно. + +- **`formatResult` для всех 5 tools (убрать кастомный error с PR номером)** — + отклонено: post-review/post-docs-review/merge-pr error-сообщения + содержат `for PR #N` — полезно для диагностики (какой PR fail'нул). + `formatResult` дал бы generic `⚠️ post-review failed (exit K): ...` без + PR. Потеря информации > экономия 3 строк × 3 файла. `formatResult` + используется где error-формат точно совпадает (create-issue/create-pr). + +- **Параметризовать `--repo` через `git remote get-url origin` (как + ADR-007 для pipeline-status.py)** — отклонено: достаточно опционального + `repo` + auto-detect. Параметризация через git remote добавила бы spawn + вызов в каждый tool без выгоды (auto-detect из cwd справляется, а явный + `repo` покрывает edge-cases). ADR-025 rejected alt пересмотрена. + +- **Inline shared-логику в каждый tool без отдельного модуля** — отклонено: + это и есть текущее дублирование (проблема 1). Вынос в `_shared.ts` — + canonical источник, тестируемый отдельно. + +- **Не расширять `_ts_loader.mjs`, тестировать только через `.ts` (bun)** — + отклонено: CI runner не имеет `bun` (opencode binary с bundled bun, нет + отдельного CLI). `.ts` тесты документационные; реальные проверки идут + через `.py` + loader. Без расширения loader'а `.py` тесты отрефакторенных + tools падали бы. \ No newline at end of file diff --git a/docs/handoff/pr-65-tools-refactor-shared-module.md b/docs/handoff/pr-65-tools-refactor-shared-module.md new file mode 100644 index 0000000..13b96d4 --- /dev/null +++ b/docs/handoff/pr-65-tools-refactor-shared-module.md @@ -0,0 +1,104 @@ +--- +pr: 65 +title: refactor(tools): add repo parameter + shared module + tunnel tests +--- + +## Что сделано + +- **`.opencode/tools/_shared.ts`** — новый shared module: `parseRepo(repo?)` + (возвращает `["--repo", repo]` или `[]` для auto-detect), `runGh(args, repo?, + opts?)` (spawnSync с prepended `--repo` при явном repo + `cwd` из opts), + `formatResult(r, toolName)` (canonical `⚠️ failed (exit K): ...` на + ошибке, `stdout.trim()` на успехе). Устраняет ~50 строк дублирования + spawnSync-логики в 5 GitHub tools. +- **`tests/_ts_loader.mjs`** — расширение для поддержки relative imports + (`./_shared`): `stripTs` стриппает `import { ... } from "./..."` + `export` + на top-level declarations + type annotations в function params; новая + `inlineShared()` инлайнит код shared-модуля в `new Function` sandbox (с + авто-добавлением `.ts` расширения к specifier). Без этого loader падал на + ENOENT — local imports не резолвились. +- **5 GitHub tools отрефакторены** на `runGh`/`formatResult` + добавлен + опциональный `repo?: string` параметр (backward-compatible): + - `create-issue.ts` — `runGh` + `formatResult(r, "gh issue create")` + - `create-pr.ts` — `runGh` + `formatResult(r, "gh pr create")` + - `post-review.ts` — `runGh` (error-ветка оставлена кастомной с PR номером) + - `post-docs-review.ts` — `runGh` (аналогично) + - `merge-pr.ts` — `runGh` (аналогично) + При `repo` omitted → `parseRepo` возвращает `[]`, gh auto-detect'ит из + `context.worktree` (cwd) — поведение идентично pre-refactor (ADR-025 / PR#61). +- **Tunnel tests созданы** (issue #64 проблема 3 — единственный tool без + тестов): + - `tests/test_tunnel_tool.py` — 6 pytest-тестов: start без токена (exit 1), + start с токеном (`started (PID: N)`), start с `TUNNEL_DOMAIN` (`domain: + ...`), stop при живом процессе (`stopped` + PID-файл удалён), stale PID-файл + (чистка + start), toggle (повторный start → stop). Изоляция: копия + `tunnel.sh` в tmp с переписанными PID/LOG путями + fake `cloudflared` + (sleep 30s) на PATH. + - `tests/test_tunnel_tool.ts` — 4 TS-теста: spawnSync с `bash` + путём к + скрипту, success → trimmed stdout, failure → `⚠️ tunnel failed (exit K)`, + cwd из context. +- **5 test-файлов обновлены** кейсами для `repo?: string` (явный / auto-detect / + invalid): + - `test_create_issue_tool.py/.ts` — +3 кейса (explicit, omitted, invalid) + - `test_create_pr_tool.py/.ts` — +3 кейса + - `test_post_review_tool.py/.ts` — +3 кейса + - `test_post_docs_review_tool.py/.ts` — +3 кейса + - `test_merge_pr_tool.py/.ts` — созданы с нуля (8 .py + 5 .ts: базовые + кейсы + repo; merge-pr не имел тестов ранее) +- **ADR-027** + этот handoff. + +## Почему + +Research (subagent explore, PR#63) выявил 3 проблемы в GitHub tools инфраструктуре: + +1. **Дублирование spawnSync логики** — каждый из 5 GitHub tools дублировал + `spawnSync("gh", [...], { encoding, cwd })` + `if (r.status !== 0) return + "⚠️ ... failed (exit ...): ..."` — ~10 строк × 5 файлов = ~50 строк + дублирования. Shared module устраняет дублирование, единый canonical + error-формат. +2. **Нет `repo` параметра** — tools работали только через auto-detect (`gh` + определяет owner/repo из `git remote` в `context.worktree`). Ограничение: + из не-git-директории `gh` не может определить remote → fall back на + хардкод (баг issue #60, исправлен в PR#61). Явный `repo?: string` даёт + гибкость для pipeline оркестрации без потери auto-detect по умолчанию + (backward-compatible). Связано с ADR-025 (PR#61 убрал хардкод, этот PR + делает `repo` параметром — явный путь вместо хардкода). +3. **Tunnel без тестов** — `tunnel` единственный из 10 tools без парных + `.py` + `.ts` тестов. Toggle-логика (PID-файл, kill -0, stale cleanup) + не покрыта → регрессии проходят незамеченными. 6 .py + 4 .ts тестов + покрывают все ветви `tunnel.sh`. + +## Pending + +- После merge: на хосте `git pull` + рестарт opencode-контейнера чтобы + подхватились обновлённые tools (auto-discovered через @opencode-ai/plugin). +- Зависимости: supersedes partial PR#61 (ADR-025) — убрал хардкод `--repo`, + этот PR параметризует; related PR#63 (ADR-026) — tool-usage policy + описывает 10 tools, этот PR улучшает их реализацию. + +## Watch out + +- **`_ts_loader.mjs` extension** — `inlineShared()` добавляет `.ts` к + relative import specifier если его нет (`./_shared` → `./_shared.ts`). + Это необходимо потому что TS import specifiers опускают расширение, а + `readFileSync` требует полный путь. Если будущие shared-модули используют + другие расширения (`.tsx`, `.mts`) — потребуется расширить логику. +- **`formatResult` только для create-issue/create-pr** — post-review, + post-docs-review, merge-pr сохраняют кастомный error с PR номером + (`⚠️ ... failed for PR #N (exit K): ...`) для диагностики. `formatResult` + дал бы `⚠️ ... failed (exit K): ...` без PR — потеря информации. Тесты + `test_gh_failure_returns_error` проверяют substring `"post-review failed"` + / `"exit 1"` — оба варианта проходят. +- **Backward-compat критична** — существующие `test_spawnsync_args` в + post-review/post-docs-review ассертят `"--repo" not in args`. При `repo` + omitted `parseRepo` возвращает `[]` → `--repo` не добавляется → тесты + проходят (51→62 на 4 tools). `test_repo_omitted_no_repo_flag` явно + закрепляет это поведение. +- **Tunnel test isolation** — `tunnel.sh` хардкодит `/tmp/tunnel.pid`. Тесты + копируют скрипт в `tmp_path` и переписывают PID/LOG пути через `replace()`. + Если в `tunnel.sh` изменится формат строк `PID_FILE="..."` — паттерн + replace сломается, тесты упадут (раннее обнаружение). +- **`commit.ts` НЕ в списке 5** — это `git`, не `gh` tool (нет `--repo` + флага). Оставлен как есть. +- **`.opencode/package-lock.json` untracked** — не относится к этому PR, + оставлен вне коммитов. \ No newline at end of file diff --git a/docs/project-map/README.md b/docs/project-map/README.md index 39c90fa..9fbdf19 100644 --- a/docs/project-map/README.md +++ b/docs/project-map/README.md @@ -39,14 +39,15 @@ opencode-config/ │ │ ├── run-tests/SKILL.md # Test runner guide │ │ └── spec/SKILL.md # 9-phase spec generation │ ├── tools/ +│ │ ├── _shared.ts # Shared module for GitHub tools: parseRepo(repo?), runGh(args, repo?, opts?), formatResult(r, toolName) — PR#65 │ │ ├── commit.ts # commit tool wrapper (1 arg message, validates format+staged) — PR#38 -│ │ ├── create-issue.ts # create-issue tool wrapper (3 args, validates format+labels) — PR#38 -│ │ ├── create-pr.ts # create-pr tool wrapper (3 args, validates format+Closes #N) — PR#38 -│ │ ├── merge-pr.ts # merge_pr tool wrapper (orchestrator-safe gh pr merge) — PR#30 +│ │ ├── create-issue.ts # create-issue tool wrapper (3 args, validates format+labels; optional repo?: string) — PR#38, PR#65 +│ │ ├── create-pr.ts # create-pr tool wrapper (3 args, validates format+Closes #N; optional repo?: string) — PR#38, PR#65 +│ │ ├── merge-pr.ts # merge_pr tool wrapper (orchestrator-safe gh pr merge; optional repo?: string) — PR#30, PR#65 │ │ ├── memory-setup.ts # memory_setup tool wrapper (0 args, calls setup-memory.sh) — PR#36 │ │ ├── pipeline-status.ts # pipeline_status tool wrapper -│ │ ├── post-docs-review.ts # post_docs_review tool wrapper (3 args: pr_number, verdict enum, body; deterministic ## Docs Review Summary heading) — PR#46 -│ │ ├── post-review.ts # post_review tool wrapper (3 args: pr_number, verdict enum, body; deterministic ## Code Review Summary heading) — PR#46 +│ │ ├── post-docs-review.ts # post_docs_review tool wrapper (3 args: pr_number, verdict enum, body; deterministic ## Docs Review Summary heading; optional repo?: string) — PR#46, PR#65 +│ │ ├── post-review.ts # post_review tool wrapper (3 args: pr_number, verdict enum, body; deterministic ## Code Review Summary heading; optional repo?: string) — PR#46, PR#65 │ │ ├── spec-status.ts # spec_status tool wrapper │ │ └── tunnel.ts # Cloudflare tunnel toggle tool (start/stop без args) — PR#34 │ ├── scripts/ @@ -74,7 +75,7 @@ opencode-config/ │ ├── index.py # Indexing │ └── 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 +│ ├── _ts_loader.mjs # TS test loader (load/exec_stub/exec_stub_json/exec_real modes; relative import inlining via inlineShared()) — PR#38, PR#65 │ ├── 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 @@ -83,14 +84,16 @@ opencode-config/ │ ├── test_dockerfile.py # Dockerfile npm install (opencode-ai + repomix + @mathew-cf/opencode-memory) — PR#57 │ ├── test_commit_tool.py # .opencode/tools/commit.ts (via _ts_loader.mjs exec_stub_json) — PR#38 │ ├── test_commit_tool.ts # TS wrapper test (mjs loader) — PR#38 -│ ├── test_create_issue_tool.py # .opencode/tools/create-issue.ts (via _ts_loader.mjs exec_stub_json) — PR#38 -│ ├── test_create_issue_tool.ts # TS wrapper test (mjs loader) — PR#38 -│ ├── test_create_pr_tool.py # .opencode/tools/create-pr.ts (via _ts_loader.mjs exec_stub_json) — PR#38 -│ ├── test_create_pr_tool.ts # TS wrapper test (mjs loader) — PR#38 +│ ├── test_create_issue_tool.py # .opencode/tools/create-issue.ts (via _ts_loader.mjs exec_stub_json; +repo explicit/omitted/invalid) — PR#38, PR#65 +│ ├── test_create_issue_tool.ts # TS wrapper test (mjs loader; +repo cases) — PR#38, PR#65 +│ ├── test_create_pr_tool.py # .opencode/tools/create-pr.ts (via _ts_loader.mjs exec_stub_json; +repo explicit/omitted/invalid) — PR#38, PR#65 +│ ├── test_create_pr_tool.ts # TS wrapper test (mjs loader; +repo cases) — PR#38, PR#65 │ ├── test_embedder.py # src/memory/embedder.py (mocks AI_PROVIDER_API_URL) │ ├── test_index.py # src/memory/index.py │ ├── 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_merge_pr_tool.py # .opencode/tools/merge-pr.ts (via _ts_loader.mjs; 8 tests: base + repo explicit/omitted/invalid) — PR#65 +│ ├── test_merge_pr_tool.ts # TS wrapper test (mjs loader; 5 tests: base + repo) — PR#65 │ ├── 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) @@ -99,14 +102,16 @@ opencode-config/ │ ├── test_pipeline_status_next_actions.py # NEXT_ACTIONS subagent_type+template per phase (25 tests) — PR#42 │ ├── test_pipeline_status_tool.py │ ├── test_pipeline_status_tool.ts # TS wrapper test (mjs loader) -│ ├── test_post_docs_review_tool.py # .opencode/tools/post-docs-review.ts (via _ts_loader.mjs exec_stub_json) — PR#46 -│ ├── test_post_docs_review_tool.ts # TS wrapper test (mjs loader) — PR#46 -│ ├── test_post_review_tool.py # .opencode/tools/post-review.ts (via _ts_loader.mjs exec_stub_json) — PR#46 -│ ├── test_post_review_tool.ts # TS wrapper test (mjs loader) — PR#46 +│ ├── test_post_docs_review_tool.py # .opencode/tools/post-docs-review.ts (via _ts_loader.mjs exec_stub_json; +repo cases) — PR#46, PR#65 +│ ├── test_post_docs_review_tool.ts # TS wrapper test (mjs loader; +repo cases) — PR#46, PR#65 +│ ├── test_post_review_tool.py # .opencode/tools/post-review.ts (via _ts_loader.mjs exec_stub_json; +repo cases) — PR#46, PR#65 +│ ├── test_post_review_tool.ts # TS wrapper test (mjs loader; +repo cases) — PR#46, PR#65 │ ├── test_search.py # src/memory/search.py │ ├── test_setup_memory.py # .opencode/scripts/setup-memory.sh (mock remote, idempotency) — PR#36 │ ├── test_spec_status.py # .opencode/scripts/spec-status.py -│ └── test_spec_status_tool.py +│ ├── test_spec_status_tool.py +│ ├── test_tunnel_tool.py # .opencode/scripts/tunnel.sh (6 pytest: start/stop/stale PID/toggle, isolated tmp copy + fake cloudflared) — PR#65 +│ └── test_tunnel_tool.ts # .opencode/tools/tunnel.ts (4 TS: spawnSync wiring, success/error/cwd) — PR#65 ├── pyproject.toml # Python project (uv, ruff, pytest config) ├── uv.lock # Locked deps for Python project ├── .pre-commit-config.yaml # ruff + UV hooks diff --git a/tests/_ts_loader.mjs b/tests/_ts_loader.mjs index 71cf074..4c1b807 100644 --- a/tests/_ts_loader.mjs +++ b/tests/_ts_loader.mjs @@ -76,23 +76,80 @@ function stripTs(src) { // 7) Strip `as const` assertions (TS-only, used by post-review.ts / // post-docs-review.ts for tuple literal types) -> plain array literal. // 8) Strip `type = ...;` type alias declarations (TS-only) -> removed. + // 9) Strip relative imports (`import { X } from "./_shared"`) — the + // referenced module is inlined by loadTool() via inlineShared(). The + // import line is removed here; the bindings come from the prepended + // shared-module code. + // 10) Strip `export ` keyword on top-level `function`/`const` declarations + // (shared modules use `export function parseRepo`). After stripping, + // the declarations become plain statements in the sandbox scope. + // 11) Strip type annotations on function parameters and return types: + // `function foo(a: Type, b?: Type2): RetType {` -> `function foo(a, b) {`. + // Needed for _shared.ts which declares typed function params. let out = src out = out.replace(/^import\s+\{\s*spawnSync\s*\}\s+from\s+["']child_process["'];?\s*$/m, 'const { spawnSync } = require("child_process");') out = out.replace(/^import\s+path\s+from\s+["']path["'];?\s*$/m, 'const path = require("path");') out = out.replace(/^import\s+\{\s*tool\s*\}\s+from\s+["']@opencode-ai\/plugin["'];?\s*$/m, 'const tool = (x) => x;') + // Strip relative imports (`./_shared`, `../foo`, etc.) — loadTool inlines them. + out = out.replace(/^import\s+\{[^}]*\}\s+from\s+["']\.[^"']+["'];?\s*$/m, "") // Replace `import.meta.dir` with the directory of the TS file. out = out.replace(/import\.meta\.dir/g, JSON.stringify(path.dirname(TS_FILE))) // Strip `as const` assertions: `["APPROVE", ...] as const` -> `["APPROVE", ...]` out = out.replace(/\bas\s+const\b/g, "") // Strip `type = ...` type alias declarations (single-line, optional `;`). out = out.replace(/^type\s+\w+\s*=\s*.+$\s*$/m, "") + // Strip `export ` keyword on top-level declarations (shared modules). + out = out.replace(/^export\s+(function|const|let|var)\b/gm, "$1") + // Strip type annotations on function params + return type: + // `function foo(a: Type, b?: Type2): RetType {` -> `function foo(a, b) {` + // Handles single-line function signatures (used by _shared.ts). Object + // types like `{ cwd?: string }` are matched non-greedily within a param. + out = out.replace(/^(\s*function\s+\w+\s*\()([^)]*)\)(\s*:\s*[^{]+)?\s*\{/gm, (line, head, params, _ret) => { + const cleaned = params + .split(",") + .map((p) => p.replace(/^\s*\w+/, (n) => n).replace(/:.*/, "").replace(/\?$/, "").trim()) + .filter((p) => p.length > 0) + .join(", ") + return `${head}${cleaned}) {` + }) return out } +function inlineShared(rawSrc, spawnSyncImpl) { + // Detect relative imports (`./_shared`, `../foo`, etc.) in the ORIGINAL + // source (before stripTs blanks the line) and inline the shared module so + // its exported functions are in scope inside the new Function sandbox. We + // load and strip the shared module the same way as the tool, then strip its + // own `require("child_process")` / `require("path")` lines so they pick up + // the sandbox bindings instead of re-declaring identifiers. + const sharedMatch = rawSrc.match(/^import\s+\{[^}]*\}\s+from\s+["'](\.[^"']+)["'];?\s*$/m) + if (!sharedMatch) return { sharedCode: "" } + const rel = sharedMatch[1] + // Resolve the shared module relative to the tool's directory. The import + // specifier in TS omits the `.ts` extension (e.g. `./_shared`), so append it + // when the specifier has no extension. + const spec = rel.endsWith(".ts") ? rel : rel + ".ts" + const sharedPath = path.resolve(path.dirname(TS_FILE), spec) + let sharedSrc = stripTs(readFileSync(sharedPath, "utf-8")) + // Strip the same sandbox-replaced declarations so no duplicate identifiers + // clash with the new Function parameters / require shim. + sharedSrc = sharedSrc.replace(/^const\s+\{\s*spawnSync\s*\}\s*=\s*require\(["']child_process["']\);?\s*$/m, "") + sharedSrc = sharedSrc.replace(/^const\s+path\s*=\s*require\(["']path["']\);?\s*$/m, "") + sharedSrc = sharedSrc.replace(/^const\s+tool\s*=\s*\(x\)\s*=>\s*x;?\s*$/m, "") + // Remove any relative import line from the shared module (defensive — _shared + // has none today, but keeps the path open for deeper nesting). + sharedSrc = sharedSrc.replace(/^import\s+\{[^}]*\}\s+from\s+["']\.[^"']+["'];?\s*$/m, "") + return { sharedCode: sharedSrc + "\n" } +} + function loadTool(spawnSyncImpl) { // Provide a CommonJS module sandbox so the tool file's `export default` // becomes accessible via `module.exports.default`. - let src = stripTs(readFileSync(TS_FILE, "utf-8")) + const rawSrc = readFileSync(TS_FILE, "utf-8") + let src = stripTs(rawSrc) + // Inline relative imports (e.g. `./_shared`) so the tool's imported + // functions (parseRepo/runGh/formatResult) are in scope inside the sandbox. + const { sharedCode } = inlineShared(rawSrc, spawnSyncImpl) // Strip the require/const declarations we replace with sandbox args, so // we don't get "Identifier already declared" between Function args and // the in-source `const { spawnSync } = require(...)` lines. @@ -110,7 +167,7 @@ function loadTool(spawnSyncImpl) { "spawnSync", "path", "tool", - cjs + "\nreturn module.exports.default;", + sharedCode + cjs + "\nreturn module.exports.default;", ) return fn({ exports: {} }, (name) => { if (name === "child_process") return { spawnSync: spawnSyncImpl } diff --git a/tests/test_create_issue_tool.py b/tests/test_create_issue_tool.py index 6e29f36..5ceb749 100644 --- a/tests/test_create_issue_tool.py +++ b/tests/test_create_issue_tool.py @@ -190,10 +190,66 @@ def test_execute_uses_cwd_from_context(): """execute passes cwd=context.worktree to spawnSync (ADR-023 pattern).""" out = _run_exec({"title": VALID_TITLE, "body": VALID_BODY}, [ISSUE_OK_RESPONSE]) calls = out["calls"] - assert len(calls) == 1, f"expected 1 spawnSync call, got {len(calls)}" + assert len(calls) == 1, f"expected 1 spawnSync call, got: {len(calls)}" opts = calls[0]["opts"] assert opts is not None, "spawnSync called without opts — expected cwd kwarg" assert "cwd" in opts, f"opts missing 'cwd' key — got: {opts}" assert opts["cwd"] == str(REPO_ROOT), ( f"cwd must equal context.worktree ({REPO_ROOT}), got: {opts['cwd']!r}" ) + + +def test_repo_explicit_passed_to_gh(): + """execute() with repo='foo/bar' → gh receives '--repo foo/bar' before the subcommand. + + The shared runGh helper prepends ['--repo', ] to the gh argv so an + explicit repo targets the right owner/name regardless of context.worktree. + """ + out = _run_exec( + {"title": VALID_TITLE, "body": VALID_BODY, "repo": "foo/bar"}, + [ISSUE_OK_RESPONSE], + ) + result = out["result"] + assert result == f"Issue created: {ISSUE_URL}", f"expected success, got: {result!r}" + calls = out["calls"] + assert len(calls) == 1, f"expected 1 spawnSync call, got: {len(calls)}" + args = calls[0]["args"] + assert args[0] == "--repo", f"expected --repo first, got: {args[0]!r}" + assert args[1] == "foo/bar", f"expected repo value, got: {args[1]!r}" + assert args[2] == "issue", f"expected 'issue' after --repo , got: {args[2]!r}" + + +def test_repo_omitted_no_repo_flag(): + """execute() without repo → gh argv has NO --repo (auto-detect from cwd). + + Backward-compatibility: when repo is omitted, runGh returns [] from + parseRepo, so gh auto-detects the repo from context.worktree (cwd) — + matching the pre-refactor behaviour (ADR-025 / PR#61). + """ + out = _run_exec({"title": VALID_TITLE, "body": VALID_BODY}, [ISSUE_OK_RESPONSE]) + calls = out["calls"] + assert len(calls) == 1, f"expected 1 spawnSync call, got: {len(calls)}" + args = calls[0]["args"] + assert "--repo" not in args, ( + f"--repo must NOT be added when repo arg omitted; gh auto-detects. args: {args!r}" + ) + assert args[0] == "issue", f"expected 'issue' first, got: {args[0]!r}" + + +def test_repo_invalid_gh_error(): + """execute() with invalid repo + gh failure → error mentions gh issue create + exit code. + + gh rejects an invalid owner/name with non-zero exit; the tool formats the + error via the shared formatResult helper (toolName='gh issue create'). + """ + fail_response = {"status": 1, "stdout": "", "stderr": 'expected the "owner/repo" format'} + out = _run_exec( + {"title": VALID_TITLE, "body": VALID_BODY, "repo": "not-a-valid-repo"}, + [fail_response], + ) + result = out["result"] + assert "gh issue create failed" in result, f"expected tool failure, got: {result!r}" + assert "exit 1" in result, f"expected exit 1 mention, got: {result!r}" + calls = out["calls"] + args = calls[0]["args"] + assert "--repo" in args, f"expected --repo in args even on failure, got: {args!r}" diff --git a/tests/test_create_issue_tool.ts b/tests/test_create_issue_tool.ts index 085359d..3c56c58 100644 --- a/tests/test_create_issue_tool.ts +++ b/tests/test_create_issue_tool.ts @@ -119,4 +119,56 @@ describe("create-issue tool", () => { // Body lacks Russian headings → heading check fires (not Cyrillic check). expect(result).toContain("## Контекст") }) + + test("test_repo_explicit — repo='foo/bar' prepends --repo to gh argv", async () => { + let capturedArgs + mock.module("child_process", () => ({ + spawnSync: (_cmd, args) => { + capturedArgs = args + return { status: 0, stdout: "https://github.com/foo/bar/issues/1\n", stderr: "" } + }, + })) + const mod = await import(TOOL_SRC + "?t=" + Date.now()) + const result = await mod.default.execute({ + title: "feat(tools): valid title", + body: VALID_BODY, + repo: "foo/bar", + }, ctx()) + expect(result).toBe("Issue created: https://github.com/foo/bar/issues/1") + expect(capturedArgs[0]).toBe("--repo") + expect(capturedArgs[1]).toBe("foo/bar") + expect(capturedArgs[2]).toBe("issue") + }) + + test("test_repo_omitted — no --repo flag (auto-detect from cwd)", async () => { + let capturedArgs + mock.module("child_process", () => ({ + spawnSync: (_cmd, args) => { + capturedArgs = args + return { status: 0, stdout: "https://github.com/x/y/issues/1\n", stderr: "" } + }, + })) + const mod = await import(TOOL_SRC + "?t=" + Date.now()) + await mod.default.execute({ + title: "feat(tools): valid title", + body: VALID_BODY, + }, ctx()) + // Backward-compat: --repo MUST NOT be present when repo arg omitted. + expect(capturedArgs).not.toContain("--repo") + expect(capturedArgs[0]).toBe("issue") + }) + + test("test_repo_invalid — invalid repo + gh failure → tool error", async () => { + mock.module("child_process", () => ({ + spawnSync: () => ({ status: 1, stdout: "", stderr: 'expected the "owner/repo" format' }), + })) + const mod = await import(TOOL_SRC + "?t=" + Date.now()) + const result = await mod.default.execute({ + title: "feat(tools): valid title", + body: VALID_BODY, + repo: "not-a-valid-repo", + }, ctx()) + expect(result).toContain("gh issue create failed") + expect(result).toContain("exit 1") + }) }) \ No newline at end of file diff --git a/tests/test_create_pr_tool.py b/tests/test_create_pr_tool.py index a23f25c..d4ff587 100644 --- a/tests/test_create_pr_tool.py +++ b/tests/test_create_pr_tool.py @@ -151,10 +151,66 @@ def test_execute_uses_cwd_from_context(): """execute passes cwd=context.worktree to spawnSync (ADR-023 pattern).""" out = _run_exec({"title": VALID_TITLE, "body": VALID_BODY}, [PR_OK_RESPONSE]) calls = out["calls"] - assert len(calls) == 1, f"expected 1 spawnSync call, got {len(calls)}" + assert len(calls) == 1, f"expected 1 spawnSync call, got: {len(calls)}" opts = calls[0]["opts"] assert opts is not None, "spawnSync called without opts — expected cwd kwarg" assert "cwd" in opts, f"opts missing 'cwd' key — got: {opts}" assert opts["cwd"] == str(REPO_ROOT), ( f"cwd must equal context.worktree ({REPO_ROOT}), got: {opts['cwd']!r}" ) + + +def test_repo_explicit_passed_to_gh(): + """execute() with repo='foo/bar' → gh receives '--repo foo/bar' before the subcommand. + + The shared runGh helper prepends ['--repo', ] to the gh argv so an + explicit repo targets the right owner/name regardless of context.worktree. + """ + out = _run_exec( + {"title": VALID_TITLE, "body": VALID_BODY, "repo": "foo/bar"}, + [PR_OK_RESPONSE], + ) + result = out["result"] + assert result == f"PR created: {PR_URL}", f"expected success, got: {result!r}" + calls = out["calls"] + assert len(calls) == 1, f"expected 1 spawnSync call, got: {len(calls)}" + args = calls[0]["args"] + assert args[0] == "--repo", f"expected --repo first, got: {args[0]!r}" + assert args[1] == "foo/bar", f"expected repo value, got: {args[1]!r}" + assert args[2] == "pr", f"expected 'pr' after --repo , got: {args[2]!r}" + + +def test_repo_omitted_no_repo_flag(): + """execute() without repo → gh argv has NO --repo (auto-detect from cwd). + + Backward-compatibility: when repo is omitted, runGh returns [] from + parseRepo, so gh auto-detects the repo from context.worktree (cwd) — + matching the pre-refactor behaviour (ADR-025 / PR#61). + """ + out = _run_exec({"title": VALID_TITLE, "body": VALID_BODY}, [PR_OK_RESPONSE]) + calls = out["calls"] + assert len(calls) == 1, f"expected 1 spawnSync call, got: {len(calls)}" + args = calls[0]["args"] + assert "--repo" not in args, ( + f"--repo must NOT be added when repo arg omitted; gh auto-detects. args: {args!r}" + ) + assert args[0] == "pr", f"expected 'pr' first, got: {args[0]!r}" + + +def test_repo_invalid_gh_error(): + """execute() with invalid repo + gh failure → error mentions gh pr create + exit code. + + gh rejects an invalid owner/name with non-zero exit; the tool formats the + error via the shared formatResult helper (toolName='gh pr create'). + """ + fail_response = {"status": 1, "stdout": "", "stderr": 'expected the "owner/repo" format'} + out = _run_exec( + {"title": VALID_TITLE, "body": VALID_BODY, "repo": "not-a-valid-repo"}, + [fail_response], + ) + result = out["result"] + assert "gh pr create failed" in result, f"expected tool failure, got: {result!r}" + assert "exit 1" in result, f"expected exit 1 mention, got: {result!r}" + calls = out["calls"] + args = calls[0]["args"] + assert "--repo" in args, f"expected --repo in args even on failure, got: {args!r}" diff --git a/tests/test_create_pr_tool.ts b/tests/test_create_pr_tool.ts index c1d180c..3c407e6 100644 --- a/tests/test_create_pr_tool.ts +++ b/tests/test_create_pr_tool.ts @@ -125,4 +125,56 @@ describe("create-pr tool", () => { const bodyArg = capturedArgs[capturedArgs.indexOf("--body") + 1] expect(bodyArg).toContain("Closes #37") }) + + test("test_repo_explicit — repo='foo/bar' prepends --repo to gh argv", async () => { + let capturedArgs + mock.module("child_process", () => ({ + spawnSync: (_cmd, args) => { + capturedArgs = args + return { status: 0, stdout: "https://github.com/foo/bar/pull/1\n", stderr: "" } + }, + })) + const mod = await import(TOOL_SRC + "?t=" + Date.now()) + const result = await mod.default.execute({ + title: "feat(tools): valid title", + body: VALID_BODY, + repo: "foo/bar", + }, ctx()) + expect(result).toBe("PR created: https://github.com/foo/bar/pull/1") + expect(capturedArgs[0]).toBe("--repo") + expect(capturedArgs[1]).toBe("foo/bar") + expect(capturedArgs[2]).toBe("pr") + }) + + test("test_repo_omitted — no --repo flag (auto-detect from cwd)", async () => { + let capturedArgs + mock.module("child_process", () => ({ + spawnSync: (_cmd, args) => { + capturedArgs = args + return { status: 0, stdout: "https://github.com/x/y/pull/1\n", stderr: "" } + }, + })) + const mod = await import(TOOL_SRC + "?t=" + Date.now()) + await mod.default.execute({ + title: "feat(tools): valid title", + body: VALID_BODY, + }, ctx()) + // Backward-compat: --repo MUST NOT be present when repo arg omitted. + expect(capturedArgs).not.toContain("--repo") + expect(capturedArgs[0]).toBe("pr") + }) + + test("test_repo_invalid — invalid repo + gh failure → tool error", async () => { + mock.module("child_process", () => ({ + spawnSync: () => ({ status: 1, stdout: "", stderr: 'expected the "owner/repo" format' }), + })) + const mod = await import(TOOL_SRC + "?t=" + Date.now()) + const result = await mod.default.execute({ + title: "feat(tools): valid title", + body: VALID_BODY, + repo: "not-a-valid-repo", + }, ctx()) + expect(result).toContain("gh pr create failed") + expect(result).toContain("exit 1") + }) }) \ No newline at end of file diff --git a/tests/test_merge_pr_tool.py b/tests/test_merge_pr_tool.py new file mode 100644 index 0000000..15fdf7e --- /dev/null +++ b/tests/test_merge_pr_tool.py @@ -0,0 +1,185 @@ +"""Tests for .opencode/tools/merge-pr.ts — the merge-pr custom tool. + +Mirrors tests/test_post_review_tool.py / test_commit_tool.py: exercises the +tool's ``execute()`` function via ``tests/_ts_loader.mjs`` using the +``exec_stub_json`` mode (multi-arg tools). + +The loader is parameterized via the ``TS_FILE`` env var. These tests set +``TS_FILE=.opencode/tools/merge-pr.ts``. + +Modes used: +- ``load`` — sanity-check that the tool loads and declares ``pr_number`` and + the optional ``repo`` args. +- ``exec_stub_json`` — call execute with a stubbed spawnSync to verify: + (a) success path: valid pr_number → "PR #N merged successfully (squash, + branch deleted).", + (b) spawnSync args: gh pr merge N --squash --delete-branch (no --repo when + repo arg omitted — gh auto-detects from context.worktree, ADR-025), + (c) repo parameter: explicit repo → "--repo " prepended; + omitted → no --repo (backward-compatible auto-detect, ADR-027), + (d) failure: gh exit non-zero → "⚠️ merge_pr failed for PR #N (exit K): ...". + +merge-pr.ts makes 1 spawnSync call (gh pr merge) on all paths. +""" + +import json +import os +import subprocess +from pathlib import Path + +import pytest + +REPO_ROOT = Path(__file__).resolve().parent.parent +LOADER = REPO_ROOT / "tests" / "_ts_loader.mjs" +TS_FILE = REPO_ROOT / ".opencode" / "tools" / "merge-pr.ts" +TS_FILE_REL = ".opencode/tools/merge-pr.ts" + +MERGE_OK_RESPONSE = {"status": 0, "stdout": "", "stderr": ""} + + +def _run_loader(*args: str) -> dict: + """Invoke the loader with TS_FILE env set to merge-pr.ts and parse JSON stdout.""" + env = {**os.environ, "TS_FILE": TS_FILE_REL} + proc = subprocess.run( + ["node", str(LOADER), *args], + capture_output=True, + text=True, + check=False, + cwd=str(REPO_ROOT), + timeout=60, + env=env, + ) + if proc.returncode != 0: + raise RuntimeError( + f"_ts_loader.mjs {' '.join(args)} failed (exit {proc.returncode}):\n" + f"stdout: {proc.stdout}\nstderr: {proc.stderr}" + ) + return json.loads(proc.stdout) + + +def _run_exec(args: dict, responses: list[dict]) -> dict: + """Helper: exec_stub_json mode with JSON args + sequential stub responses.""" + return _run_loader("exec_stub_json", json.dumps(args), json.dumps(responses)) + + +def test_loader_can_load_tool(): + """Sanity: merge-pr.ts loads and declares pr_number + optional repo args.""" + if not TS_FILE.exists(): + pytest.skip("merge-pr.ts not present") + out = _run_loader("load") + assert "description" in out + args = out["args"] + assert "pr_number" in args, f"missing pr_number arg: {args}" + assert "repo" in args, f"missing repo arg: {args}" + + +def test_valid_merge(): + """execute() with valid pr_number returns the squash-merged success message.""" + out = _run_exec({"pr_number": 42}, [MERGE_OK_RESPONSE]) + result = out["result"] + assert result == "PR #42 merged successfully (squash, branch deleted).", ( + f"expected success, got: {result!r}" + ) + + +def test_spawnsync_args(): + """spawnSync called with gh pr merge --squash --delete-branch (no --repo). + + When repo is omitted, runGh returns [] from parseRepo, so gh auto-detects + the repo from context.worktree (cwd) — matching create-pr.ts and the + pre-refactor behaviour (ADR-025 / PR#61). + """ + out = _run_exec({"pr_number": 42}, [MERGE_OK_RESPONSE]) + calls = out["calls"] + assert len(calls) == 1, f"expected 1 spawnSync call, got: {len(calls)}" + call = calls[0] + assert call["cmd"] == "gh", f"expected cmd 'gh', got: {call['cmd']!r}" + args = call["args"] + assert args[0] == "pr", f"expected first arg 'pr', got: {args[0]!r}" + assert args[1] == "merge", f"expected second arg 'merge', got: {args[1]!r}" + assert args[2] == "42", f"expected PR number '42', got: {args[2]!r}" + assert "--squash" in args, "missing --squash flag" + assert "--delete-branch" in args, "missing --delete-branch flag" + # --repo MUST NOT be present — gh auto-detects from cwd (context.worktree). + assert "--repo" not in args, ( + f"--repo must not be hardcoded; gh auto-detects from cwd. args: {args!r}" + ) + + +def test_execute_uses_cwd_from_context(): + """execute passes cwd=context.worktree to spawnSync (ADR-023 pattern).""" + out = _run_exec({"pr_number": 42}, [MERGE_OK_RESPONSE]) + calls = out["calls"] + assert len(calls) == 1, f"expected 1 spawnSync call, got: {len(calls)}" + opts = calls[0]["opts"] + assert opts is not None, "spawnSync called without opts — expected cwd kwarg" + assert "cwd" in opts, f"opts missing 'cwd' key — got: {opts}" + assert opts["cwd"] == str(REPO_ROOT), ( + f"cwd must equal context.worktree ({REPO_ROOT}), got: {opts['cwd']!r}" + ) + + +def test_gh_failure_returns_error(): + """execute() with gh exit non-zero returns error message with exit code.""" + fail_response = {"status": 1, "stdout": "", "stderr": "gh: PR not mergeable"} + out = _run_exec({"pr_number": 42}, [fail_response]) + result = out["result"] + assert "merge_pr failed" in result, f"expected failure message, got: {result!r}" + assert "PR #42" in result, f"expected PR number in failure, got: {result!r}" + assert "exit 1" in result, f"expected exit 1 mention, got: {result!r}" + + +def test_repo_explicit_passed_to_gh(): + """execute() with repo='foo/bar' → gh receives '--repo foo/bar' before the subcommand. + + The shared runGh helper prepends ['--repo', ] to the gh argv so an + explicit repo targets the right owner/name regardless of context.worktree. + """ + out = _run_exec({"pr_number": 42, "repo": "foo/bar"}, [MERGE_OK_RESPONSE]) + result = out["result"] + assert result == "PR #42 merged successfully (squash, branch deleted).", ( + f"expected success, got: {result!r}" + ) + calls = out["calls"] + assert len(calls) == 1, f"expected 1 spawnSync call, got: {len(calls)}" + args = calls[0]["args"] + assert args[0] == "--repo", f"expected --repo first, got: {args[0]!r}" + assert args[1] == "foo/bar", f"expected repo value, got: {args[1]!r}" + assert args[2] == "pr", f"expected 'pr' after --repo , got: {args[2]!r}" + # The squash/delete-branch flags must still be present after the repo prefix. + assert "--squash" in args, "missing --squash flag with explicit repo" + assert "--delete-branch" in args, "missing --delete-branch flag with explicit repo" + + +def test_repo_omitted_no_repo_flag(): + """execute() without repo → gh argv has NO --repo (auto-detect from cwd). + + Backward-compatibility: when repo is omitted, runGh returns [] from + parseRepo, so gh auto-detects the repo from context.worktree (cwd) — + matching the pre-refactor behaviour (ADR-025 / PR#61). This complements + test_spawnsync_args which asserts the same. + """ + out = _run_exec({"pr_number": 42}, [MERGE_OK_RESPONSE]) + calls = out["calls"] + assert len(calls) == 1, f"expected 1 spawnSync call, got: {len(calls)}" + args = calls[0]["args"] + assert "--repo" not in args, ( + f"--repo must NOT be added when repo arg omitted; gh auto-detects. args: {args!r}" + ) + assert args[0] == "pr", f"expected 'pr' first, got: {args[0]!r}" + + +def test_repo_invalid_gh_error(): + """execute() with invalid repo + gh failure → error mentions merge_pr + exit code. + + gh rejects an invalid owner/name with non-zero exit; the tool formats the + error with its tool-specific message (merge_pr failed for PR #N). + """ + fail_response = {"status": 1, "stdout": "", "stderr": 'expected the "owner/repo" format'} + out = _run_exec({"pr_number": 42, "repo": "not-a-valid-repo"}, [fail_response]) + result = out["result"] + assert "merge_pr failed" in result, f"expected tool failure, got: {result!r}" + assert "exit 1" in result, f"expected exit 1 mention, got: {result!r}" + calls = out["calls"] + args = calls[0]["args"] + assert "--repo" in args, f"expected --repo in args even on failure, got: {args!r}" diff --git a/tests/test_merge_pr_tool.ts b/tests/test_merge_pr_tool.ts new file mode 100644 index 0000000..9430688 --- /dev/null +++ b/tests/test_merge_pr_tool.ts @@ -0,0 +1,113 @@ +/** + * Tests for .opencode/tools/merge-pr.ts — the merge-pr custom tool. + * + * Mirror of tests/test_post_review_tool.ts / test_commit_tool.ts: + * the tool is a spawnSync wrapper around `gh pr merge N --squash --delete-branch`. + * + * Runtime note: opencode ships a standalone binary with Bun bundled inside; + * there is no separate `bun` CLI on the host (CI runner uses node + pytest). + * The CI runs the equivalent Python tests in tests/test_merge_pr_tool.py + * via the JS loader tests/_ts_loader.mjs (exec_stub_json mode for multi-arg + * tools). This file documents the intended TS-side test cases and is + * runnable under `bun test` once a bun runtime is available on the host. + * + * Test cases (mirror tests/test_merge_pr_tool.py): + * - test_valid_merge — valid pr_number → "PR #N merged successfully..." + * - test_spawnsync_args — spawnSync called with correct gh args (no --repo) + * - test_repo_explicit — repo='foo/bar' prepends --repo to gh argv + * - test_repo_omitted — no --repo flag (auto-detect from cwd) + * - test_repo_invalid — invalid repo + gh failure → tool error + */ + +import { describe, test, expect, mock } from "bun:test" with { type: "'bun-test'" } +import { spawnSync } from "child_process" +import path from "path" + +const TOOL_SRC = path.resolve(import.meta.dir, "..", ".opencode", "tools", "merge-pr.ts") + +function ctx() { + return { + sessionID: "t", messageID: "t", agent: "t", + directory: ".", worktree: ".", + abort: new AbortController().signal, + metadata() {}, async ask() {}, + } +} + +const MERGE_OK = { status: 0, stdout: "", stderr: "" } + +describe("merge-pr tool", () => { + test("test_valid_merge — valid pr_number succeeds", async () => { + mock.module("child_process", () => ({ + spawnSync: () => MERGE_OK, + })) + const mod = await import(TOOL_SRC + "?t=" + Date.now()) + const result = await mod.default.execute({ pr_number: 42 }, ctx()) + expect(result).toBe("PR #42 merged successfully (squash, branch deleted).") + }) + + test("test_spawnsync_args — spawnSync called with gh pr merge N --squash --delete-branch (no --repo)", async () => { + let capturedCmd + let capturedArgs + mock.module("child_process", () => ({ + spawnSync: (cmd, args) => { + capturedCmd = cmd + capturedArgs = args + return MERGE_OK + }, + })) + const mod = await import(TOOL_SRC + "?t=" + Date.now()) + await mod.default.execute({ pr_number: 42 }, ctx()) + expect(capturedCmd).toBe("gh") + expect(capturedArgs[0]).toBe("pr") + expect(capturedArgs[1]).toBe("merge") + expect(capturedArgs[2]).toBe("42") + expect(capturedArgs).toContain("--squash") + expect(capturedArgs).toContain("--delete-branch") + // --repo MUST NOT be present — gh auto-detects from cwd (PR#60). + expect(capturedArgs).not.toContain("--repo") + }) + + test("test_repo_explicit — repo='foo/bar' prepends --repo to gh argv", async () => { + let capturedArgs + mock.module("child_process", () => ({ + spawnSync: (_cmd, args) => { + capturedArgs = args + return MERGE_OK + }, + })) + const mod = await import(TOOL_SRC + "?t=" + Date.now()) + const result = await mod.default.execute({ pr_number: 42, repo: "foo/bar" }, ctx()) + expect(result).toBe("PR #42 merged successfully (squash, branch deleted).") + expect(capturedArgs[0]).toBe("--repo") + expect(capturedArgs[1]).toBe("foo/bar") + expect(capturedArgs[2]).toBe("pr") + expect(capturedArgs).toContain("--squash") + expect(capturedArgs).toContain("--delete-branch") + }) + + test("test_repo_omitted — no --repo flag (auto-detect from cwd)", async () => { + let capturedArgs + mock.module("child_process", () => ({ + spawnSync: (_cmd, args) => { + capturedArgs = args + return MERGE_OK + }, + })) + const mod = await import(TOOL_SRC + "?t=" + Date.now()) + await mod.default.execute({ pr_number: 42 }, ctx()) + // Backward-compat: --repo MUST NOT be present when repo arg omitted. + expect(capturedArgs).not.toContain("--repo") + expect(capturedArgs[0]).toBe("pr") + }) + + test("test_repo_invalid — invalid repo + gh failure → tool error", async () => { + mock.module("child_process", () => ({ + spawnSync: () => ({ status: 1, stdout: "", stderr: 'expected the "owner/repo" format' }), + })) + const mod = await import(TOOL_SRC + "?t=" + Date.now()) + const result = await mod.default.execute({ pr_number: 42, repo: "not-a-valid-repo" }, ctx()) + expect(result).toContain("merge_pr failed") + expect(result).toContain("exit 1") + }) +}) \ No newline at end of file diff --git a/tests/test_post_docs_review_tool.py b/tests/test_post_docs_review_tool.py index 6b571b6..8b4457c 100644 --- a/tests/test_post_docs_review_tool.py +++ b/tests/test_post_docs_review_tool.py @@ -242,3 +242,63 @@ def test_gh_failure_returns_error(): result = out["result"] assert "post-docs-review failed" in result, f"expected failure message, got: {result!r}" assert "exit 1" in result, f"expected exit 1 mention, got: {result!r}" + + +def test_repo_explicit_passed_to_gh(): + """execute() with repo='foo/bar' → gh receives '--repo foo/bar' before the subcommand. + + The shared runGh helper prepends ['--repo', ] to the gh argv so an + explicit repo targets the right owner/name regardless of context.worktree. + """ + out = _run_exec( + {"pr_number": 45, "verdict": "APPROVE", "body": "Docs body.", "repo": "foo/bar"}, + [COMMENT_OK_RESPONSE], + ) + result = out["result"] + assert result == EXPECTED_APPROVE, f"expected success, got: {result!r}" + calls = out["calls"] + assert len(calls) == 1, f"expected 1 spawnSync call, got: {len(calls)}" + args = calls[0]["args"] + assert args[0] == "--repo", f"expected --repo first, got: {args[0]!r}" + assert args[1] == "foo/bar", f"expected repo value, got: {args[1]!r}" + assert args[2] == "pr", f"expected 'pr' after --repo , got: {args[2]!r}" + + +def test_repo_omitted_no_repo_flag(): + """execute() without repo → gh argv has NO --repo (auto-detect from cwd). + + Backward-compatibility: when repo is omitted, runGh returns [] from + parseRepo, so gh auto-detects the repo from context.worktree (cwd) — + matching the pre-refactor behaviour (ADR-025 / PR#61). This complements + test_spawnsync_args which asserts the same on the success path. + """ + out = _run_exec( + {"pr_number": 45, "verdict": "APPROVE", "body": "Docs body."}, + [COMMENT_OK_RESPONSE], + ) + calls = out["calls"] + assert len(calls) == 1, f"expected 1 spawnSync call, got: {len(calls)}" + args = calls[0]["args"] + assert "--repo" not in args, ( + f"--repo must NOT be added when repo arg omitted; gh auto-detects. args: {args!r}" + ) + assert args[0] == "pr", f"expected 'pr' first, got: {args[0]!r}" + + +def test_repo_invalid_gh_error(): + """execute() with invalid repo + gh failure → error mentions post-docs-review + exit code. + + gh rejects an invalid owner/name with non-zero exit; the tool formats the + error with its tool-specific message (post-docs-review failed for PR #N). + """ + fail_response = {"status": 1, "stdout": "", "stderr": 'expected the "owner/repo" format'} + out = _run_exec( + {"pr_number": 45, "verdict": "APPROVE", "body": "Docs body.", "repo": "not-a-valid-repo"}, + [fail_response], + ) + result = out["result"] + assert "post-docs-review failed" in result, f"expected tool failure, got: {result!r}" + assert "exit 1" in result, f"expected exit 1 mention, got: {result!r}" + calls = out["calls"] + args = calls[0]["args"] + assert "--repo" in args, f"expected --repo in args even on failure, got: {args!r}" diff --git a/tests/test_post_docs_review_tool.ts b/tests/test_post_docs_review_tool.ts index 9bba646..d47de0c 100644 --- a/tests/test_post_docs_review_tool.ts +++ b/tests/test_post_docs_review_tool.ts @@ -182,4 +182,59 @@ describe("post-docs-review tool", () => { expect(capturedArgs[lastIdx - 1]).toBe("--body") expect(capturedArgs[lastIdx].startsWith("## Docs Review Summary\n")).toBe(true) }) + + test("test_repo_explicit — repo='foo/bar' prepends --repo to gh argv", async () => { + let capturedArgs + mock.module("child_process", () => ({ + spawnSync: (_cmd, args) => { + capturedArgs = args + return COMMENT_OK + }, + })) + const mod = await import(TOOL_SRC + "?t=" + Date.now()) + const result = await mod.default.execute({ + pr_number: 45, + verdict: "APPROVE", + body: "Docs body", + repo: "foo/bar", + }, ctx()) + expect(result).toBe("Docs review posted on PR #45: verdict=APPROVE") + expect(capturedArgs[0]).toBe("--repo") + expect(capturedArgs[1]).toBe("foo/bar") + expect(capturedArgs[2]).toBe("pr") + }) + + test("test_repo_omitted — no --repo flag (auto-detect from cwd)", async () => { + let capturedArgs + mock.module("child_process", () => ({ + spawnSync: (_cmd, args) => { + capturedArgs = args + return COMMENT_OK + }, + })) + const mod = await import(TOOL_SRC + "?t=" + Date.now()) + await mod.default.execute({ + pr_number: 45, + verdict: "APPROVE", + body: "Docs body", + }, ctx()) + // Backward-compat: --repo MUST NOT be present when repo arg omitted. + expect(capturedArgs).not.toContain("--repo") + expect(capturedArgs[0]).toBe("pr") + }) + + test("test_repo_invalid — invalid repo + gh failure → tool error", async () => { + mock.module("child_process", () => ({ + spawnSync: () => ({ status: 1, stdout: "", stderr: 'expected the "owner/repo" format' }), + })) + const mod = await import(TOOL_SRC + "?t=" + Date.now()) + const result = await mod.default.execute({ + pr_number: 45, + verdict: "APPROVE", + body: "Docs body", + repo: "not-a-valid-repo", + }, ctx()) + expect(result).toContain("post-docs-review failed") + expect(result).toContain("exit 1") + }) }) \ No newline at end of file diff --git a/tests/test_post_review_tool.py b/tests/test_post_review_tool.py index 0cf1f50..8b3803b 100644 --- a/tests/test_post_review_tool.py +++ b/tests/test_post_review_tool.py @@ -247,3 +247,63 @@ def test_gh_failure_returns_error(): result = out["result"] assert "post-review failed" in result, f"expected failure message, got: {result!r}" assert "exit 1" in result, f"expected exit 1 mention, got: {result!r}" + + +def test_repo_explicit_passed_to_gh(): + """execute() with repo='foo/bar' → gh receives '--repo foo/bar' before the subcommand. + + The shared runGh helper prepends ['--repo', ] to the gh argv so an + explicit repo targets the right owner/name regardless of context.worktree. + """ + out = _run_exec( + {"pr_number": 45, "verdict": "APPROVE", "body": "Review body.", "repo": "foo/bar"}, + [COMMENT_OK_RESPONSE], + ) + result = out["result"] + assert result == EXPECTED_APPROVE, f"expected success, got: {result!r}" + calls = out["calls"] + assert len(calls) == 1, f"expected 1 spawnSync call, got: {len(calls)}" + args = calls[0]["args"] + assert args[0] == "--repo", f"expected --repo first, got: {args[0]!r}" + assert args[1] == "foo/bar", f"expected repo value, got: {args[1]!r}" + assert args[2] == "pr", f"expected 'pr' after --repo , got: {args[2]!r}" + + +def test_repo_omitted_no_repo_flag(): + """execute() without repo → gh argv has NO --repo (auto-detect from cwd). + + Backward-compatibility: when repo is omitted, runGh returns [] from + parseRepo, so gh auto-detects the repo from context.worktree (cwd) — + matching the pre-refactor behaviour (ADR-025 / PR#61). This complements + test_spawnsync_args which asserts the same on the success path. + """ + out = _run_exec( + {"pr_number": 45, "verdict": "APPROVE", "body": "Review body."}, + [COMMENT_OK_RESPONSE], + ) + calls = out["calls"] + assert len(calls) == 1, f"expected 1 spawnSync call, got: {len(calls)}" + args = calls[0]["args"] + assert "--repo" not in args, ( + f"--repo must NOT be added when repo arg omitted; gh auto-detects. args: {args!r}" + ) + assert args[0] == "pr", f"expected 'pr' first, got: {args[0]!r}" + + +def test_repo_invalid_gh_error(): + """execute() with invalid repo + gh failure → error mentions post-review + exit code. + + gh rejects an invalid owner/name with non-zero exit; the tool formats the + error with its tool-specific message (post-review failed for PR #N). + """ + fail_response = {"status": 1, "stdout": "", "stderr": 'expected the "owner/repo" format'} + out = _run_exec( + {"pr_number": 45, "verdict": "APPROVE", "body": "Review body.", "repo": "not-a-valid-repo"}, + [fail_response], + ) + result = out["result"] + assert "post-review failed" in result, f"expected tool failure, got: {result!r}" + assert "exit 1" in result, f"expected exit 1 mention, got: {result!r}" + calls = out["calls"] + args = calls[0]["args"] + assert "--repo" in args, f"expected --repo in args even on failure, got: {args!r}" diff --git a/tests/test_post_review_tool.ts b/tests/test_post_review_tool.ts index a364beb..f78a93f 100644 --- a/tests/test_post_review_tool.ts +++ b/tests/test_post_review_tool.ts @@ -185,4 +185,59 @@ describe("post-review tool", () => { expect(capturedArgs[lastIdx - 1]).toBe("--body") expect(capturedArgs[lastIdx].startsWith("## Code Review Summary\n")).toBe(true) }) + + test("test_repo_explicit — repo='foo/bar' prepends --repo to gh argv", async () => { + let capturedArgs + mock.module("child_process", () => ({ + spawnSync: (_cmd, args) => { + capturedArgs = args + return COMMENT_OK + }, + })) + const mod = await import(TOOL_SRC + "?t=" + Date.now()) + const result = await mod.default.execute({ + pr_number: 45, + verdict: "APPROVE", + body: "Review body", + repo: "foo/bar", + }, ctx()) + expect(result).toBe("Review posted on PR #45: verdict=APPROVE") + expect(capturedArgs[0]).toBe("--repo") + expect(capturedArgs[1]).toBe("foo/bar") + expect(capturedArgs[2]).toBe("pr") + }) + + test("test_repo_omitted — no --repo flag (auto-detect from cwd)", async () => { + let capturedArgs + mock.module("child_process", () => ({ + spawnSync: (_cmd, args) => { + capturedArgs = args + return COMMENT_OK + }, + })) + const mod = await import(TOOL_SRC + "?t=" + Date.now()) + await mod.default.execute({ + pr_number: 45, + verdict: "APPROVE", + body: "Review body", + }, ctx()) + // Backward-compat: --repo MUST NOT be present when repo arg omitted. + expect(capturedArgs).not.toContain("--repo") + expect(capturedArgs[0]).toBe("pr") + }) + + test("test_repo_invalid — invalid repo + gh failure → tool error", async () => { + mock.module("child_process", () => ({ + spawnSync: () => ({ status: 1, stdout: "", stderr: 'expected the "owner/repo" format' }), + })) + const mod = await import(TOOL_SRC + "?t=" + Date.now()) + const result = await mod.default.execute({ + pr_number: 45, + verdict: "APPROVE", + body: "Review body", + repo: "not-a-valid-repo", + }, ctx()) + expect(result).toContain("post-review failed") + expect(result).toContain("exit 1") + }) }) \ No newline at end of file diff --git a/tests/test_tunnel_tool.py b/tests/test_tunnel_tool.py new file mode 100644 index 0000000..988a8dd --- /dev/null +++ b/tests/test_tunnel_tool.py @@ -0,0 +1,262 @@ +"""Tests for .opencode/scripts/tunnel.sh — the cloudflare tunnel toggle script. + +The tunnel tool (.opencode/tools/tunnel.ts) is a thin wrapper that spawns +``bash .opencode/scripts/tunnel.sh``. The toggle logic, PID-file management +and error handling all live in the bash script, so these tests exercise the +script directly (the .ts wrapper is covered by tests/test_tunnel_tool.ts). + +Isolation strategy: ``tunnel.sh`` hardcodes ``PID_FILE=/tmp/tunnel.pid`` and +``LOG_FILE=/tmp/tunnel.log``. To avoid clobbering a real tunnel on the host +(or interference between tests), each test copies the script to a temp dir +and rewrites the PID/LOG paths to point inside that temp dir. ``cloudflared`` +is shadowed by a fake binary on PATH that sleeps long enough for the script +to verify the process is alive (``kill -0``), so the "started" branch fires. + +Covered cases (issue #64): +- Start without ``CLOUDFLARE_TUNNEL_TOKEN`` → ``❌ ... is not set``, exit 1. +- Start with token (no domain) → ``started (PID: N)``, PID file created. +- Start with token + ``TUNNEL_DOMAIN`` → ``started (PID: N, domain: )``. +- Stop when process alive (PID file present, process live) → ``stopped``, + PID file removed. +- Stale PID file (PID file present, process dead) → cleaned up, then start. +- Toggle: second start while process alive → ``stopped`` (toggle semantics). +""" + +import contextlib +import os +import stat +import subprocess +import sys +import textwrap +from pathlib import Path + +import pytest + +REPO_ROOT = Path(__file__).resolve().parent.parent +SCRIPT_SRC = REPO_ROOT / ".opencode" / "scripts" / "tunnel.sh" + + +def _make_fake_cloudflared(tmp_path: Path) -> Path: + """Write a fake `cloudflared` that sleeps long enough for kill -0 to pass. + + The real cloudflared runs the tunnel; for tests we only need a long-lived + child so the script's `kill -0 $PID` check succeeds and the "started" + branch fires. The fake ignores its args and sleeps 30s (cleaned up when + the test process exits or the script kills it via the toggle/stop path). + """ + fake = tmp_path / "bin" / "cloudflared" + fake.parent.mkdir(parents=True, exist_ok=True) + fake.write_text( + textwrap.dedent("""\ + #!/usr/bin/env bash + # Fake cloudflared: sleep so the parent script sees us alive (kill -0). + exec sleep 30 + """) + ) + fake.chmod(fake.stat().st_mode | stat.S_IEXEC | stat.S_IXGRP | stat.S_IXOTH) + return fake + + +def _make_isolated_script(tmp_path: Path) -> Path: + """Copy tunnel.sh to tmp and rewrite PID/LOG paths to live inside tmp. + + tunnel.sh hardcodes /tmp/tunnel.pid and /tmp/tunnel.log. Running the + tests as-is would clobber a real tunnel on the host. We rewrite both + paths to point at /tunnel.pid and /tunnel.log so each test is + isolated and cleanup is automatic (tmp_path is pytest-managed). + """ + if not SCRIPT_SRC.exists(): + pytest.skip("tunnel.sh not present") + dst = tmp_path / "tunnel.sh" + src = SCRIPT_SRC.read_text() + src = src.replace('PID_FILE="/tmp/tunnel.pid"', f'PID_FILE="{tmp_path}/tunnel.pid"') + src = src.replace('LOG_FILE="/tmp/tunnel.log"', f'LOG_FILE="{tmp_path}/tunnel.log"') + dst.write_text(src) + dst.chmod(dst.stat().st_mode | stat.S_IEXEC | stat.S_IXGRP | stat.S_IXOTH) + return dst + + +def _run(script: Path, env: dict, timeout: float = 10.0) -> subprocess.CompletedProcess: + """Run the (isolated) tunnel.sh with the given env and return the result.""" + return subprocess.run( + ["bash", str(script)], + capture_output=True, + text=True, + check=False, + timeout=timeout, + env=env, + ) + + +def test_start_without_token(tmp_path: Path): + """Start without CLOUDFLARE_TUNNEL_TOKEN → error message, exit 1. + + The script checks the token first (set -euo pipefail + [[ -z ... ]]) and + exits before touching the PID file or spawning cloudflared. + """ + script = _make_isolated_script(tmp_path) + env = {**os.environ} + env.pop("CLOUDFLARE_TUNNEL_TOKEN", None) + env.pop("TUNNEL_DOMAIN", None) + # Ensure PATH has no real cloudflared that could shadow the check — the + # token check fires before cloudflared is invoked, but be defensive. + proc = _run(script, env) + assert proc.returncode == 1, f"expected exit 1, got {proc.returncode}; stderr={proc.stderr!r}" + assert "CLOUDFLARE_TUNNEL_TOKEN is not set" in proc.stdout, ( + f"expected token-missing message, got stdout={proc.stdout!r}" + ) + # PID file must NOT be created on the early-exit path. + assert not (tmp_path / "tunnel.pid").exists(), "PID file created despite missing token" + + +def test_start_with_token(tmp_path: Path): + """Start with token (no domain) → 'started (PID: N)', PID file created. + + The fake cloudflared sleeps 30s so kill -0 succeeds and the script reports + 'started (PID: )'. The PID file must contain the child PID. + """ + script = _make_isolated_script(tmp_path) + fake = _make_fake_cloudflared(tmp_path) + base_env = {**os.environ} + base_env.pop("TUNNEL_DOMAIN", None) + env = { + **base_env, + "CLOUDFLARE_TUNNEL_TOKEN": "fake-token", + "PATH": f"{fake.parent}:{base_env['PATH']}", + } + proc = _run(script, env) + assert proc.returncode == 0, f"expected exit 0, got {proc.returncode}; stderr={proc.stderr!r}" + assert "started (PID:" in proc.stdout, ( + f"expected 'started (PID:...)', got stdout={proc.stdout!r}" + ) + pid_file = tmp_path / "tunnel.pid" + assert pid_file.exists(), "PID file not created on start" + pid = pid_file.read_text().strip() + assert pid.isdigit(), f"PID file content is not a number: {pid!r}" + # Clean up: kill the sleeping fake cloudflared so it doesn't linger. + with contextlib.suppress(ProcessLookupError, PermissionError): + os.kill(int(pid), 9) + + +def test_start_with_token_and_domain(tmp_path: Path): + """Start with token + TUNNEL_DOMAIN → 'started (PID: N, domain: )'. + + When TUNNEL_DOMAIN is set, the script appends ', domain: ' to the + started line (display only — the tunnel itself is the same). + """ + script = _make_isolated_script(tmp_path) + fake = _make_fake_cloudflared(tmp_path) + base_env = {**os.environ} + env = { + **base_env, + "CLOUDFLARE_TUNNEL_TOKEN": "fake-token", + "TUNNEL_DOMAIN": "example.com", + "PATH": f"{fake.parent}:{base_env['PATH']}", + } + proc = _run(script, env) + assert proc.returncode == 0, f"expected exit 0, got {proc.returncode}; stderr={proc.stderr!r}" + assert "started (PID:" in proc.stdout, ( + f"expected 'started (PID:...)', got stdout={proc.stdout!r}" + ) + assert "domain: example.com" in proc.stdout, ( + f"expected domain in output, got stdout={proc.stdout!r}" + ) + pid = (tmp_path / "tunnel.pid").read_text().strip() + with contextlib.suppress(ProcessLookupError, PermissionError): + os.kill(int(pid), 9) + + +def test_stop_when_process_alive(tmp_path: Path): + """Stop when PID file exists + process alive → 'stopped', PID file removed. + + Toggle semantics: first start creates the PID file, second invocation sees + the live process and stops it. We simulate a 'live' process by writing the + PID of the current test process (which is alive) into the PID file, then + run the script — it should kill... wait, it would kill the test process. + Instead we start a long-lived child (sleep) ourselves, write its PID, then + run the script and verify it stops the child and removes the PID file. + """ + script = _make_isolated_script(tmp_path) + # Start a long-lived child whose PID we control. + child = subprocess.Popen([sys.executable, "-c", "import time; time.sleep(30)"]) + pid_file = tmp_path / "tunnel.pid" + pid_file.write_text(str(child.pid)) + env = {**os.environ, "CLOUDFLARE_TUNNEL_TOKEN": "fake-token"} + env.pop("TUNNEL_DOMAIN", None) + proc = _run(script, env) + assert proc.returncode == 0, f"expected exit 0, got {proc.returncode}; stderr={proc.stderr!r}" + assert proc.stdout.strip() == "stopped", f"expected 'stopped', got stdout={proc.stdout!r}" + assert not pid_file.exists(), "PID file not removed on stop" + # The child must have been terminated by the script's `kill $PID`. + child.wait(timeout=5) + assert child.poll() is not None, "child still alive after stop" + + +def test_stale_pid_file_cleaned_then_start(tmp_path: Path): + """Stale PID file (process dead) → cleaned up, then start. + + If the PID file points at a dead process, kill -0 fails and the script + removes the stale file (rm -f) before falling through to the start path. + We write a PID that is guaranteed dead (a recently-exited subprocess). + """ + script = _make_isolated_script(tmp_path) + fake = _make_fake_cloudflared(tmp_path) + # Use a PID that is guaranteed dead: a very high PID that no real process + # holds on this system. kill -0 fails (ESRCH) → the script treats the PID + # file as stale, removes it, and falls through to the start path. Using a + # recently-reaped PID risks PID reuse by the OS between reap and script + # run; a high sentinel PID avoids that race entirely. + dead_pid = 999999 + pid_file = tmp_path / "tunnel.pid" + pid_file.write_text(str(dead_pid)) + base_env = {**os.environ} + base_env.pop("TUNNEL_DOMAIN", None) + env = { + **base_env, + "CLOUDFLARE_TUNNEL_TOKEN": "fake-token", + "PATH": f"{fake.parent}:{base_env['PATH']}", + } + proc = _run(script, env) + assert proc.returncode == 0, f"expected exit 0, got {proc.returncode}; stderr={proc.stderr!r}" + assert "started (PID:" in proc.stdout, ( + f"expected start after stale cleanup, got stdout={proc.stdout!r}" + ) + # PID file now holds the NEW cloudflared PID, not the stale one. + new_pid = pid_file.read_text().strip() + assert new_pid.isdigit() and int(new_pid) != dead_pid, ( + f"PID file still holds stale PID {new_pid} (expected new cloudflared PID)" + ) + with contextlib.suppress(ProcessLookupError, PermissionError): + os.kill(int(new_pid), 9) + + +def test_toggle_second_start_stops(tmp_path: Path): + """Toggle: a second start while a process is alive → 'stopped'. + + The script is a toggle: if the PID file exists and the process is alive, + the next invocation stops it (instead of starting a second tunnel). We + start a tunnel first, then run the script again and expect 'stopped'. + """ + script = _make_isolated_script(tmp_path) + fake = _make_fake_cloudflared(tmp_path) + base_env = {**os.environ} + base_env.pop("TUNNEL_DOMAIN", None) + env = { + **base_env, + "CLOUDFLARE_TUNNEL_TOKEN": "fake-token", + "PATH": f"{fake.parent}:{base_env['PATH']}", + } + # First call: start. + proc1 = _run(script, env) + assert "started (PID:" in proc1.stdout, f"first call should start, got {proc1.stdout!r}" + pid = (tmp_path / "tunnel.pid").read_text().strip() + assert pid.isdigit(), f"PID file should hold a number after start, got {pid!r}" + # Second call: toggle → stop. + proc2 = _run(script, env) + assert proc2.returncode == 0, ( + f"expected exit 0, got {proc2.returncode}; stderr={proc2.stderr!r}" + ) + assert proc2.stdout.strip() == "stopped", ( + f"second call should stop (toggle), got stdout={proc2.stdout!r}" + ) + assert not (tmp_path / "tunnel.pid").exists(), "PID file not removed on toggle-stop" diff --git a/tests/test_tunnel_tool.ts b/tests/test_tunnel_tool.ts new file mode 100644 index 0000000..f6ffe79 --- /dev/null +++ b/tests/test_tunnel_tool.ts @@ -0,0 +1,93 @@ +/** + * Tests for .opencode/tools/tunnel.ts — the tunnel custom tool. + * + * The tool is a thin spawnSync wrapper around `bash .opencode/scripts/tunnel.sh` + * (toggle: 1st call starts, 2nd call stops). The toggle/PID-file logic lives + * in the bash script and is covered by tests/test_tunnel_tool.py (which runs + * the script directly with a fake cloudflared). These TS tests stub spawnSync + * to verify the tool wiring: it invokes `bash