diff --git a/.github/releases/v1.0.38.md b/.github/releases/v1.0.38.md new file mode 100644 index 0000000000..ed1fed26b8 --- /dev/null +++ b/.github/releases/v1.0.38.md @@ -0,0 +1,49 @@ +## opencode {VERSION} + +{Prerelease/Stable} release from `{branch}` branch. Two quality deliveries land in this train: a five-front resource-leak fix wave (hook process groups, share listeners, TUI subscriptions, process-stop escalation, memory retention) and a DAG workflow-tool consistency pass (builtin:// spec_path round-trip, worker_type catalog, parameter descriptions verified against implementation), both hardened by a two-axis review round. + +--- + +### 🐛 Bug Fixes + +- **Hook command grandchildren can no longer hang triggers, #500**: a timed-out hook shell command left pipe-holding grandchildren alive because only the direct child was killed; the timeout path now kills the whole process group (negative-pid SIGKILL, consistent with the detached spawn), with regression tests asserting group death under 15s. +- **Share instance event listeners unsubscribe on dispose, #501**: five EventBus subscriptions accumulated across share lifecycles; dispose now collects and runs every returned unsubscribe before closing the scope. +- **TUI route and prompt event subscriptions clean up on unmount, #502**: Session and Prompt routes registered bus listeners without onCleanup, leaking one listener set per navigation; both routes now wrap subscriptions in onCleanup with a regression suite pinning the subscription seam. +- **Process stop awaits exit with SIGKILL escalation, #503**: Process.stop and the MCP client shutdown sent a single SIGTERM and never waited; both now run SIGTERM, a 3s grace, SIGKILL, and a bounded exit await, and the SDK copy mirrors the escalation. The index.ts exit-path clause was attempted as a bounded drain window and reverted on CI evidence: real session paths keep ref'd handles alive, so every CLI exit degraded to the 5s fallback and three subprocess lifecycle tests failed; the empirical record and a follow-up proposal (eliminate the residual handles first) are posted to the issue. +- **Memory generations and heap snapshots are bounded, #504**: every memory commit copied the full topic set into a generations directory that was never removed (now keeps the latest 3 plus an orphan-staging sweep, best-effort so cleanup never fails a commit), and RSS-storm heap snapshots (hundreds of MB each) now rotate to the latest 2. Review caught a pid-led lexicographic sort that could delete the newest snapshot across runs; pruning now orders by the embedded timestamp with a cross-pid regression test. +- **The workflow tool accepts the spec_path its own list action returns, #506**: list returned builtin:// refs that start/extend/read/validate rejected as unknown saved workflows; resolveSpecPath now round-trips the builtin:// scheme through the template registry with the same not-found diagnostics as bare names. +- **worker_type catalog lists the native primary agents, #507**: build and plan were missing from the workflow tool's dynamic worker_type enum (the task tool surface is unchanged), so graphs could not name them. + +--- + +### ⚙️ CI / Engineering + +- CI typecheck and test gates now also trigger on ready_for_review (#315 anchor): SpecGit deliveries open as draft, and converting to ready previously never started the required checks, permanently blocking specgit finish. +- SpecGit harness refreshed for the 1.10.1 CLI with every local specialization preserved, and the AGENTS.md replay list corrected (pinned version, 45/40-minute timeout split, node 22, hand-parsed policy.yaml) so the next re-init replays reality instead of a stale 0.5.0 setup. +- DAG parameter descriptions verified line-by-line against implementation, #508: timeout_ms (admission-to-completion budget, expired queued nodes fail without spawning, capped parent-adjudicated extensions), the review field (the full diff-review wiring contract and its standard-warns / deep-errors mode split), plus the workflow guide tables; stale tool-parameter snapshots regenerated. + +--- + +### 🧪 Test Summary + +``` +unit tests (linux): 4447 tests, 4423 pass, 0 fail (364 files) +e2e app tests: 21 passed on linux, 21 passed on windows +httpapi exerciser: 230 pass x coverage/auth/effect, 0 fail, 0 skip +typecheck: 29/29 packages green +delivery-specific: workflow tool/schema/parameters/review 194 pass; process/heap/memory 76 pass +lint: oxlint 4840 warnings, 0 errors (ratchet budget 4850) +``` + +--- + +### 🔍 Verification + +- Two-axis four-way code review (Standards and Spec, per delivery branch) over the full train diff; every P1 finding was independently re-verified against implementation code before fixing (heap prune ordering, stale parameter snapshot, the missing #503 clause). +- specgit finish exited 0 (accepted) for both deliveries against real git, PR, and CI evidence. +- The bounded-exit-window attempt for #503 was reverted on linux CI evidence and the empirical record posted to the issue; the revert kept the documented hanging-subprocess backstop intact. +- Known residue tracked outside this train: one dependabot high-severity alert on the default branch (security/dependabot/110). + +--- + +**Full changelog:** [`{previous_tag}`...`{current_tag}`](https://github.com/LeXwDeX/OpenCode-GraphAgent/compare/{previous_tag}...{current_tag}) diff --git a/.github/workflows/ci-test.yml b/.github/workflows/ci-test.yml index 9484aac7ab..d801a21937 100644 --- a/.github/workflows/ci-test.yml +++ b/.github/workflows/ci-test.yml @@ -26,6 +26,11 @@ on: - main - dev pull_request: + # ready_for_review matters: the SpecGit Acceptance verdict requires + # required-check runs that started at/after the draft→ready transition + # (#315 anchor). Without this type a ready transition finds only stale + # pre-ready runs and the gate times out waiting for fresh ones. + types: [opened, synchronize, reopened, ready_for_review] branches: - main - dev diff --git a/.github/workflows/ci-typecheck.yml b/.github/workflows/ci-typecheck.yml index f8bd2341c5..863d907511 100644 --- a/.github/workflows/ci-typecheck.yml +++ b/.github/workflows/ci-typecheck.yml @@ -20,6 +20,11 @@ on: - main - dev pull_request: + # ready_for_review matters: the SpecGit Acceptance verdict requires + # required-check runs that started at/after the draft→ready transition + # (#315 anchor). Without this type a ready transition finds only stale + # pre-ready runs and the gate times out waiting for fresh ones. + types: [opened, synchronize, reopened, ready_for_review] branches: - main - dev diff --git a/.github/workflows/specgit-accept.yml b/.github/workflows/specgit-accept.yml index c280272a4b..93d6a6166a 100644 --- a/.github/workflows/specgit-accept.yml +++ b/.github/workflows/specgit-accept.yml @@ -2,22 +2,42 @@ name: SpecGit Acceptance on: pull_request: - # Delivery PRs target dev (fast-integration layer); the acceptance - # verdict runs only on the dev→main promotion PR, where protect-main's - # checks apply. Keep the trigger main-only (d6ce53a83): running it on - # dev PRs duplicated the verdict against the lighter dev gate. branches: [main] + # A draft PR fails the verdict (pr_draft), so the draft→ready + # transition must re-verdict. Listing types replaces the defaults, + # so the default activity types are listed alongside. + types: [opened, synchronize, reopened, ready_for_review] + # No workflow_dispatch (local specialization): dispatch is the privileged + # context that fires CodeQL's cache-poisoning taint rule on the head_ref + # checkout (false positive: no cache use, read-only token, + # persist-credentials: false), and on dispatch events head_ref is empty so + # the verdict would evaluate the default branch — the wrong tree. Delivery + # here always goes through a PR. permissions: contents: read + issues: read + pull-requests: read + +# One verdict per head at a time (#319): a newer trigger event (a push +# after the draft opened, then ready_for_review) supersedes the older +# run of the same pull request instead of leaving parallel copies +# burning identical wait budgets. The surviving run re-verdicts fully. +concurrency: + group: specgit-accept-${{ github.ref }} + cancel-in-progress: true jobs: specgit-acceptance: name: SpecGit Acceptance + # Portable gate for any adopting repository: the published CLI is + # installed at the exact version `specgit init` pinned. The adopting + # project's own toolchain (package manager, lockfile, build, layout) + # is never assumed and never invoked. runs-on: ubuntu-latest - # Must exceed the slowest required sibling (Unit Tests (linux) runs - # ~28min on PRs): the verdict waits for every policy check to reach a - # terminal state before evaluating. + # Local specialization: must exceed the slowest required sibling + # (Unit Tests (linux) runs ~28min on PRs) — the verdict waits for every + # policy check to reach a terminal state before evaluating. timeout-minutes: 45 steps: - name: Checkout code @@ -35,13 +55,13 @@ jobs: with: node-version: '22' - # This repo is a bun workspace and does not vendor the SpecGit CLI; - # install the published CLI instead of building from source. Pinned - # with a caret floor (#366): the CLI releases multiple times a day and - # an unpinned install would let an unnoticed upstream change flip CI - # acceptance verdicts repo-wide. - - name: Install specgit CLI - run: npm install -g specgit@^0.5.0 + - name: Install pinned SpecGit CLI + # Local specialization — install GLOBALLY, not `npm install --no-save + # specgit@X`: a workspace-local install reads this bun workspace's + # package.json and dies on the `catalog:` protocol (EUNSUPPORTEDPROTOCOL, + # #434/#459). Exact pin on purpose: the gate must evaluate with the + # same CLI generation that wrote the binding (1.10.1 re-init). + run: npm install -g --no-audit --no-fund specgit@1.10.1 - name: Wait for sibling checks # The verdict must see the OTHER required checks in a terminal @@ -49,50 +69,240 @@ jobs: # their check-runs yet, so an empty poll is not "done": wait until # every name in spec_git/policy.yaml is present with a terminal # conclusion. This job is not in the policy, so no self-deadlock. + # #315: a terminal run only counts when it started at/after the + # delivery's ready-for-review transition — a stale green keeps + # waiting for the fresh run the transition triggers. + # All GitHub access goes through the authenticated gh CLI. env: GH_TOKEN: ${{ github.token }} WAIT_REPO: ${{ github.repository }} - WAIT_SHA: ${{ github.event.pull_request.head.sha }} + WAIT_SHA: ${{ github.event.pull_request.head.sha || github.sha }} + WAIT_PR: ${{ github.event.pull_request.number || '' }} run: | node --input-type=module <<'EOF' - import { readFileSync } from 'node:fs'; - // Minimal parse of policy.yaml's required_checks block list — - // avoids a yaml dependency in this bun-based repo. + import { existsSync, readFileSync } from 'node:fs'; + import { execFileSync } from 'node:child_process'; + if (!existsSync('spec_git/policy.yaml')) { + console.error('spec_git/policy.yaml is absent at this head — an adoption PR carries no binding commit yet (expected once; merge it before enabling branch protection), and a delivery PR must carry it via specgit issue.'); + process.exit(1); + } + // Local specialization — minimal hand parse of policy.yaml's + // required_checks block: this bun-based repo does not expose a + // root-reachable `yaml` package (workspace catalog isolation), so + // `import { parse } from 'yaml'` would fail to resolve here. const policy = readFileSync('spec_git/policy.yaml', 'utf8'); const section = policy.slice(policy.indexOf('required_checks:')); const required = [...section.matchAll(/^\s*-\s*(.+)$/gm)].map((m) => m[1].trim()); - const headers = { - authorization: 'Bearer ' + process.env.GH_TOKEN, - accept: 'application/vnd.github+json', + // gh.cmd needs a shell on Windows; POSIX execs the binary + + // directly (shell stays off where it is not needed). The + + // query rides --field args (never a raw "?" URL): cmd.exe + + // treats a bare "&" as a command separator, so a URL query + + // would be split mid-parameter on Windows. + + const listChecks = (page) => + JSON.parse( + execFileSync( + 'gh', + [ + 'api', + 'repos/' + process.env.WAIT_REPO + '/commits/' + process.env.WAIT_SHA + '/check-runs', + '--method', 'GET', + '--field', 'per_page=' + PER_PAGE, + '--field', 'page=' + page, + ], + { encoding: 'utf-8', stdio: ['ignore', 'pipe', 'pipe'], shell: process.platform === 'win32' } + ) + ); + // Transient API failures (5xx, 429, network) retry with bounded + + // exponential backoff — a platform blip must not fail the gate. + + const MAX_ATTEMPTS = 5; + const listChecksWithRetry = async (page) => { + for (let attempt = 1; ; attempt += 1) { + try { + return listChecks(page); + } catch (error) { + const text = String(error) + ' ' + String(error && error.stderr ? error.stderr : ''); + const transient = /HTTP 5\d\d|HTTP 429|ETIMEDOUT|ECONNRESET|ENOTFOUND|timed out/i.test(text); + if (attempt >= MAX_ATTEMPTS || !transient) throw error; + const backoff = Math.min(30000, 2000 * 2 ** (attempt - 1)); + console.log('Transient failure; retry ' + attempt + '/' + MAX_ATTEMPTS + ' in ' + backoff + 'ms'); + await new Promise((r) => setTimeout(r, backoff)); + } + } + }; + // #315: the ready-for-review anchor rides the issue-timeline + // endpoint through gh api --field args (GET, like the listing). + const fetchTimelinePage = (page) => + JSON.parse( + execFileSync( + 'gh', + [ + 'api', + 'repos/' + process.env.WAIT_REPO + '/issues/' + process.env.WAIT_PR + '/timeline', + '--method', 'GET', + '--field', 'per_page=' + PER_PAGE, + '--field', 'page=' + page, + ], + { encoding: 'utf-8', stdio: ['ignore', 'pipe', 'pipe'], shell: process.platform === 'win32' } + ) + ); + const fetchTimelineWithRetry = async (page) => { + for (let attempt = 1; ; attempt += 1) { + try { + const payload = fetchTimelinePage(page); + if (!Array.isArray(payload)) throw new Error('GitHub returned a non-array timeline payload.'); + return payload; + } catch (error) { + if (error && error.message === 'GitHub returned a non-array timeline payload.') throw error; + const text = String(error) + ' ' + String(error && error.stderr ? error.stderr : ''); + const transient = /HTTP 5\d\d|HTTP 429|ETIMEDOUT|ECONNRESET|ENOTFOUND|timed out/i.test(text); + if (attempt >= MAX_ATTEMPTS || !transient) throw error; + const backoff = Math.min(30000, 2000 * 2 ** (attempt - 1)); + console.log('Transient failure; retry ' + attempt + '/' + MAX_ATTEMPTS + ' in ' + backoff + 'ms'); + await new Promise((r) => setTimeout(r, backoff)); + } + } }; - const url = 'https://api.github.com/repos/' + process.env.WAIT_REPO - + '/commits/' + process.env.WAIT_SHA + '/check-runs?per_page=100'; const terminal = new Set(['completed']); - const terminalHas = (byName, name) => { - if (byName.has(name)) return terminal.has(byName.get(name)); - const retried = [...byName.keys()].find((k) => k.startsWith(name + ' (')); - return retried !== undefined && terminal.has(byName.get(retried)); + const PER_PAGE = 100; + // #300: page the listing to exhaustion — a head with more than + + // PER_PAGE check-runs must still expose every required name. + + const fetchAllCheckRuns = async () => { + const runs = []; + for (let page = 1; ; page += 1) { + const payload = await listChecksWithRetry(page); + runs.push(...(payload.check_runs ?? [])); + if (!payload.check_runs || payload.check_runs.length < PER_PAGE) break; + } + return runs; + }; + // #315: the evidence anchor — created_at of the latest + + // ready_for_review event on the pull request's issue timeline, + + // paged to exhaustion through the same transport seam. Empty + + // WAIT_PR (a push or workflow_dispatch event) means no anchor + + // and no freshness bound; a fetch failure fails the step + + // loudly instead of silently unbounding freshness. + + const fetchAnchor = async () => { + if (!process.env.WAIT_PR) return null; + let anchor = null; + let anchorTime = null; + for (let page = 1; ; page += 1) { + const events = await fetchTimelineWithRetry(page); + if (!Array.isArray(events)) throw new Error('GitHub returned a non-array timeline payload.'); + for (const event of events) { + if (event && event.event === 'ready_for_review') { + if (typeof event.created_at !== 'string' || event.created_at === '' + || Number.isNaN(Date.parse(event.created_at))) { + throw new Error('GitHub returned a ready-for-review event without a valid timestamp.'); + } + const eventTime = Date.parse(event.created_at); + if (anchor === null || anchorTime === null || eventTime > anchorTime) { + anchor = event.created_at; + anchorTime = eventTime; + } + } + } + if (!Array.isArray(events) || events.length < PER_PAGE) return anchor; + } }; - // Must outlast the slowest required sibling (Unit Tests (linux) - // runs ~28min on PRs); the job timeout above bounds this too. + // Poll deadline sits BELOW the job's timeout-minutes (45) on + + // purpose: when the deadline loses the race against a slow + + // sibling, the script exits with its own diagnosis instead of + + // being killed by the job timeout mid-line. + + // Local specialization: 40min because the slowest required + + // sibling (Unit Tests (linux)) runs ~28min on PRs. + const deadline = Date.now() + 40 * 60 * 1000; while (Date.now() < deadline) { - const res = await fetch(url, { headers }); - if (!res.ok) throw new Error('check-runs API ' + res.status); - const payload = await res.json(); - const byName = new Map(payload.check_runs.map((r) => [r.name, r.status])); - const missing = required.filter((n) => !terminalHas(byName, n)); - if (missing.length === 0) { + // #315: re-read the anchor every cycle — the transition + + // event landing after this job started, or the fresh runs + + // registering late, self-heal on the next poll. + + let anchor; + try { + anchor = await fetchAnchor(); + } catch (error) { + console.error('Could not read the ready-for-review anchor: ' + + (error && error.message ? error.message : String(error))); + process.exit(1); + } + const runs = await fetchAllCheckRuns(); + // #119: re-runs keep every same-name run; terminality is + // decided on the truth run — latest started_at, ties broken + // by the higher check-run id (docs/reference.md) — never on + // response position. + const truth = new Map(); + const startedTime = (run) => { + if (typeof run.started_at !== 'string') return Number.NEGATIVE_INFINITY; + const parsed = Date.parse(run.started_at); + return Number.isNaN(parsed) ? Number.NEGATIVE_INFINITY : parsed; + }; + for (const r of runs) { + const cur = truth.get(r.name); + const runTime = startedTime(r); + const currentTime = cur === undefined ? Number.NEGATIVE_INFINITY : startedTime(cur); + const later = cur === undefined + || runTime > currentTime + || (runTime === currentTime && (r.id || 0) > (cur.id || 0)); + if (later) truth.set(r.name, r); + } + const truthRunFor = (name) => { + if (truth.has(name)) return truth.get(name); + const retried = [...truth.keys()].find((k) => k.startsWith(name + ' (')); + return retried === undefined ? undefined : truth.get(retried); + }; + // #315: a required check settles only when its truth run is + // terminal AND (when an anchor exists) started at/after the + // ready-for-review transition — a stale green keeps waiting. + const missing = []; + const stale = []; + const anchorTime = anchor === null ? null : Date.parse(anchor); + for (const name of required) { + const run = truthRunFor(name); + if (run === undefined || !terminal.has(run.status)) { + missing.push(name); + } else if (anchorTime !== null && (Number.isNaN(anchorTime) || startedTime(run) < anchorTime)) { + stale.push(name); + } + } + if (missing.length === 0 && stale.length === 0) { console.log('All required checks are in a terminal state.'); process.exit(0); } - console.log('Waiting for: ' + missing.join(', ')); + if (missing.length > 0) { + console.log('Waiting for: ' + missing.join(', ')); + } + if (stale.length > 0) { + console.log('Waiting for a fresh run after ready for review: ' + stale.join(', ')); + } await new Promise((r) => setTimeout(r, 10000)); } console.error('Timed out waiting for sibling checks.'); process.exit(1); EOF + - name: specgit finish run: specgit finish --json env: diff --git a/.specgit.yaml b/.specgit.yaml index 190e8f8d65..aa16b812c8 100644 --- a/.specgit.yaml +++ b/.specgit.yaml @@ -1,8 +1,20 @@ version: 1 -delivery: sync-main-to +delivery: leftover-hardening context: kind: branch - branch: chore/496-sync-main-to + branch: test/512-leftover-hardening issues: - - 496 -pr: 497 + - 512 + - 513 + - 514 + - 515 +issueKinds: + - issue: 512 + kind: kind::test + - issue: 513 + kind: kind::fix + - issue: 514 + kind: kind::test + - issue: 515 + kind: kind::refactor +pr: 516 diff --git a/AGENTS.md b/AGENTS.md index 561824a59f..3a60205123 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -33,7 +33,7 @@ feat/**, fix/** ──PR(Typecheck + Unit Tests 门禁)──▶ dev ──push **CI 配置**: - `ci-typecheck.yml`:push 到 `main`/`dev` + PR → `main`/`dev` 时触发;除 lint + typecheck 外还跑 `test:dag-core` DAG 核心行为/覆盖率门禁(10min 超时) - `ci-test.yml`:push 到 `main`/`dev` + PR → `main`/`dev` 时触发全量测试(`cancel-in-progress: false` 保证跑完);Linux unit-tests job 额外校验生成物新鲜度(`packages/client` 与 `packages/sdk/js` 的 `check:generated`)并跑 HttpAPI 契约门禁(`test:httpapi:ci`) -- `specgit-accept.yml`:仅 PR → `main` 时触发;全局安装 `specgit@^0.5.0`(`npm install -g`;workspace 内安装会因 bun `catalog:` 协议失败),等 `spec_git/policy.yaml` `required_checks` 全部到终态后运行 `specgit finish --json` 产出 SpecGit Acceptance 裁决 +- `specgit-accept.yml`:仅 PR → `main` 时触发;全局安装 `specgit@1.10.1`(`npm install -g`;workspace 内安装会因 bun `catalog:` 协议失败),等 `spec_git/policy.yaml` `required_checks` 全部到终态(等待脚本手工解析 YAML,不依赖 `yaml` 包)后运行 `specgit finish --json` 产出 SpecGit Acceptance 裁决 - `release-fork.yml`:手动 `workflow_dispatch` 是唯一真实构建路径(push 到 `main`/`dev` 仅注册不构建);从 `dev` 发布自动产出 `X.Y.Z-dev.N` prerelease,从 `main` 发布 `X.Y.Z` 并标 Latest ## Standard Delivery Workflow (标准交付流程) @@ -267,7 +267,10 @@ This repository uses a multi-context domain-document layout rooted at `CONTEXT-M Kept OUTSIDE the managed block so `specgit init`/`--force` never rewrites them; re-apply each deviation after every re-init: - `specgit-accept.yml` drops the template's `workflow_dispatch` trigger. Dispatch is the privileged context that fires CodeQL's cache-poisoning taint rule on the `head_ref` checkout (false positive: no cache use, read-only token, `persist-credentials: false`), and on dispatch events `head_ref` is empty so the verdict would evaluate the default branch — the wrong tree. Delivery here always goes through a PR. The head-ref checkout itself must NOT be replaced with a SHA: `specgit finish` requires HEAD on the delivery branch (detached_head otherwise). -- `specgit-accept.yml` installs the CLI globally (`npm install -g specgit@^0.5.0`), NOT the template's `npm install --no-save specgit@X`: a workspace-local install reads this bun workspace's package.json and dies on the `catalog:` protocol (EUNSUPPORTEDPROTOCOL, #434, again on #459). Re-apply after every `specgit init --force`. +- `specgit-accept.yml` installs the CLI globally (`npm install -g specgit@1.10.1`), NOT the template's `npm install --no-save specgit@X`: a workspace-local install reads this bun workspace's package.json and dies on the `catalog:` protocol (EUNSUPPORTEDPROTOCOL, #434, again on #459). Re-apply after every `specgit init --force`. +- `specgit-accept.yml` runs the job with `timeout-minutes: 45` while the wait script's own poll deadline is 40 minutes (`Date.now() + 40 * 60 * 1000`): the deadline must stay BELOW the job timeout so a lost race against a slow sibling check exits with its own diagnosis instead of being killed mid-line by the job timeout. +- `specgit-accept.yml` pins `node-version: '22'` for the wait script and the CLI. +- The wait script hand-parses `spec_git/policy.yaml` (minimal line-based parse) instead of importing the `yaml` package: no root-reachable `yaml` exists under workspace catalog isolation, so `import { parse } from 'yaml'` would fail to resolve on the runner. - `spec_git/policy.yaml` `required_checks` uses the template's canonical check IDs (`unit-tests`, `e2e-tests`), not display names. diff --git a/bun.lock b/bun.lock index 7f4ae59fdb..f67c324ff2 100644 --- a/bun.lock +++ b/bun.lock @@ -193,7 +193,7 @@ "@types/node": "catalog:", "@typescript/native-preview": "catalog:", "drizzle-kit": "catalog:", - "mysql2": "3.14.4", + "mysql2": "3.22.0", "typescript": "catalog:", }, }, @@ -4487,7 +4487,7 @@ "mustache": ["mustache@4.2.0", "", { "bin": { "mustache": "bin/mustache" } }, "sha512-71ippSywq5Yb7/tVYyGbkBggbU8H3u5Rz56fH60jGFgr8uHwxs+aSKeqmluIVzM0m0kB7xQjKS6qPfd0b2ZoqQ=="], - "mysql2": ["mysql2@3.14.4", "", { "dependencies": { "aws-ssl-profiles": "^1.1.1", "denque": "^2.1.0", "generate-function": "^2.3.1", "iconv-lite": "^0.7.0", "long": "^5.2.1", "lru.min": "^1.0.0", "named-placeholders": "^1.1.3", "seq-queue": "^0.0.5", "sqlstring": "^2.3.2" } }, "sha512-Cs/jx3WZPNrYHVz+Iunp9ziahaG5uFMvD2R8Zlmc194AqXNxt9HBNu7ZsPYrUtmJsF0egETCWIdMIYAwOGjL1w=="], + "mysql2": ["mysql2@3.22.0", "", { "dependencies": { "aws-ssl-profiles": "^1.1.2", "denque": "^2.1.0", "generate-function": "^2.3.1", "iconv-lite": "^0.7.2", "long": "^5.3.2", "lru.min": "^1.1.4", "named-placeholders": "^1.1.6", "sql-escaper": "^1.3.3" }, "peerDependencies": { "@types/node": ">= 8" } }, "sha512-4jaJYBObj7FhD3lnZhqX1yDMuZN4mQNz+IolDySDXT7fbozMBpeGQNcuWXKUqo4ahkAEfkjUHPjnwuDI0/6VKw=="], "mz": ["mz@2.7.0", "", { "dependencies": { "any-promise": "^1.0.0", "object-assign": "^4.0.1", "thenify-all": "^1.0.0" } }, "sha512-z81GNO7nnYMEhrGh9LeymoE4+Yr0Wn5McHIZMK5cfQCl+NDX08sCZgUc9/6MHni9IWuFLm1Z3HTCXu2z9fN62Q=="], @@ -5019,8 +5019,6 @@ "send": ["send@0.19.2", "", { "dependencies": { "debug": "2.6.9", "depd": "2.0.0", "destroy": "1.2.0", "encodeurl": "~2.0.0", "escape-html": "~1.0.3", "etag": "~1.8.1", "fresh": "~0.5.2", "http-errors": "~2.0.1", "mime": "1.6.0", "ms": "2.1.3", "on-finished": "~2.4.1", "range-parser": "~1.2.1", "statuses": "~2.0.2" } }, "sha512-VMbMxbDeehAxpOtWJXlcUS5E8iXh6QmN+BkRX1GARS3wRaXEEgzCcB10gTQazO42tpNIya8xIyNx8fll1OFPrg=="], - "seq-queue": ["seq-queue@0.0.5", "", {}, "sha512-hr3Wtp/GZIc/6DAGPDcV4/9WoZhjrkXsi5B/07QgX8tsdc6ilr7BFM6PM6rbdAX1kFSDYeZGLipIZZKyQP0O5Q=="], - "serialize-error": ["serialize-error@7.0.1", "", { "dependencies": { "type-fest": "^0.13.1" } }, "sha512-8I8TjW5KMOKsZQTvoxjuSIa7foAwPWGOts+6o7sgjz41/qMD9VQHEDxi6PBvK2l0MXUmqZyNpUK+T2tQaaElvw=="], "seroval": ["seroval@1.3.2", "", {}, "sha512-RbcPH1n5cfwKrru7v7+zrZvjLurgHhGyso3HTyGtRivGWgYjbOmGuivCQaORNELjNONoK35nj28EoWul9sb1zQ=="], @@ -5125,7 +5123,7 @@ "sprintf-js": ["sprintf-js@1.0.3", "", {}, "sha512-D9cPgkvLlV3t3IzL0D0YLvGA9Ahk4PcvVwUbN0dSGr1aP0Nrt4AEnTUbuGvquEC0mA64Gqt1fzirlRs5ibXx8g=="], - "sqlstring": ["sqlstring@2.3.3", "", {}, "sha512-qC9iz2FlN7DQl3+wjwn3802RTyjCx7sDvfQEXchwa6CWOx07/WVfh91gBmQ9fahw8snwGEWU3xGzOt4tFyHLxg=="], + "sql-escaper": ["sql-escaper@1.5.1", "", {}, "sha512-4toX5E1fQbBrpfXidaHnF0669nkAdETeIPTs2SUjxxD7RRIs9ICG4gtpmfc68JCEKehsdwLFqBu9VlQqZ1P1gg=="], "srvx": ["srvx@0.9.8", "", { "bin": { "srvx": "bin/srvx.mjs" } }, "sha512-RZaxTKJEE/14HYn8COLuUOJAt0U55N9l1Xf6jj+T0GoA01EUH1Xz5JtSUOI+EHn+AEgPCVn7gk6jHJffrr06fQ=="], diff --git a/packages/console/core/package.json b/packages/console/core/package.json index bc7c31cc0b..05df1d3c88 100644 --- a/packages/console/core/package.json +++ b/packages/console/core/package.json @@ -45,7 +45,7 @@ "@types/bun": "catalog:", "@types/node": "catalog:", "drizzle-kit": "catalog:", - "mysql2": "3.14.4", + "mysql2": "3.22.0", "typescript": "catalog:", "@typescript/native-preview": "catalog:" } diff --git a/packages/core/src/plugin/command/orchestration-policy.md b/packages/core/src/plugin/command/orchestration-policy.md index bf3a6a23fa..4c04a1a678 100644 --- a/packages/core/src/plugin/command/orchestration-policy.md +++ b/packages/core/src/plugin/command/orchestration-policy.md @@ -333,6 +333,6 @@ A replan fragment takes real time to compose — template rendering, model reaso 1. On any user cancel/replan/model-change intent, IMMEDIATELY issue `control(pause)` in the same turn. Pause needs no fragment, applies in milliseconds, and stops new node spawns. 2. Pause does not interrupt nodes that are already running. Decide their disposition inside the fragment: `restart: true` re-spawns a running node with the new definition (its in-flight child session is hard-aborted at re-spawn), `cancel: true` terminates it, absence keeps it running to completion. 3. Compose the fragment, then issue `control(replan)` — replan is valid while paused. -4. Issue `control(resume)` to restore scheduling. +4. A successful replan auto-resumes the workflow. Issue `control(resume)` manually only when the replan output reports the automatic resume raced with another control op and the workflow is still paused; never resume a workflow the output says was already resumed. If the workflow terminalized before you paused, do not force the replan: start a new workflow carrying the updated definitions, and state which prior results are superseded. diff --git a/packages/core/src/plugin/command/workflow.md b/packages/core/src/plugin/command/workflow.md index ddb6ef9555..f0602ea92c 100644 --- a/packages/core/src/plugin/command/workflow.md +++ b/packages/core/src/plugin/command/workflow.md @@ -64,8 +64,11 @@ config: ## Saved workflows -A `spec_path` with no path separator and no `.yaml`/`.yml` extension is a -**name** resolved against the workflow library instead of the filesystem: +A `spec_path` with no path separator, no leading `.`, no control characters, +and no `.yaml`/`.yml` extension is a **name** resolved against the workflow +library instead of the filesystem (a leading dot or a recognized extension +means a filesystem path). The synthetic `builtin://` marker the `list` +output shows for builtin templates also resolves by name: 1. `.opencode/workflows/.yaml` — project scope, committed with the repo 2. `/workflows/.yaml` — global scope, available in every project @@ -340,7 +343,7 @@ Workflows are not static. After creating a workflow, use `extend` and `control(r - **Scale up**: a node reports the work is larger than expected → `extend` with additional parallel nodes to split the load. - **Cut short**: a node proves the remaining work is unnecessary → `control(complete)` to early-complete and skip pending nodes. -- **Redirect**: a gate or review reveals a wrong direction → `control(pause)` first to freeze scheduling, then `control(replan)` with `restart: true` on the affected nodes and `cancel: true` on their downstream dependents, then `control(resume)`. +- **Redirect**: a gate or review reveals a wrong direction → `control(pause)` first to freeze scheduling, then `control(replan)` with `restart: true` on the affected nodes and `cancel: true` on their downstream dependents. A successful replan auto-resumes a paused workflow; issue `control(resume)` manually only when the replan output reports the automatic resume raced with another control op. Only nodes with `report_to_parent: true` produce intermediate parent checkpoints, and those reports are delivered at the next actionable wake @@ -527,6 +530,24 @@ validation status. This lists reusable specs, not running workflows; use Pass `spec_path`, then retarget generic objectives and block instructions in the parent, write the edited result to YAML, and start that file by path. +**draft** — Render a structured graph `config` (same fields as the start +file's `config`) into a validated YAML spec and return its `spec_path`; no +workflow is created. Preferred over hand-writing YAML: the parameter schema +rejects unknown fields, eliminating serialization drift. When validation fails +the file is still on disk — fix it by calling draft again with corrected +fields, then start the returned `spec_path`. + +**guide** — Load on-demand authoring guidance. `topic` is one of `blocks` +(composable block schema), `interface` (low-level node fields), `policy` +(gates, admission, recovery), or `patterns` (cross-domain playbooks); omit it +for the compact index. Load only the topic needed for the current decision. + +**validate** — Pre-flight one spec without creating a workflow. Pass +`spec_path` and an optional `profile` (`portable` for distributable-template +checks, `environment` to additionally resolve prompts, workers, and models in +this project; builtin specs default to portable, everything else to +environment). Returns diagnostics with per-error paths, never a workflow ID. + **extend** — Add nodes to a running workflow. Existing nodes are unaffected; new nodes are immediately eligible for scheduling if their dependencies are met. It also accepts a genuinely additive wave after a reporting leaf @@ -547,9 +568,9 @@ omitted content from its preview. **control** — Control a running workflow: - `pause` — let running nodes finish, don't spawn new ones (pause does NOT stop nodes that are already running). On a cancel/replan intent, always pause FIRST: it needs no fragment and freezes scheduling while you compose the replan, so the graph cannot terminalize under you. -- `resume` — resume scheduling +- `resume` — resume scheduling. Unneeded after a successful replan or extend: both auto-resume a paused workflow; resume manually only when their output reports the automatic resume raced with another control op and the workflow is still paused. - `cancel` — cancel the entire workflow -- `replan` — put `fragment: { ... }` with the graph fields and node definitions in YAML and pass its `spec_path`; running nodes can be `restart: true` or `cancel: true`; pending nodes absent from the fragment are cancelled. Valid while paused — the pause → write file → replan → resume sequence is the safe path. +- `replan` — put `fragment: { ... }` with the graph fields and node definitions in YAML and pass its `spec_path`; running nodes can be `restart: true` or `cancel: true`; pending nodes absent from the fragment are cancelled. Valid while paused — the pause → write file → replan sequence is the safe path: a successful replan auto-resumes the workflow, an explicit resume belongs only in the rare case the replan output reports the automatic resume raced with another control op and the workflow is still paused. - `complete` — early-complete: remaining pending nodes are skipped (non-violation) - `step` — advance exactly one ready node (the first by node ID lexicographic order), then wait. Use for controlled debugging or staged verification of a critical path. Unlike `pause`, which freezes all scheduling, `step` advances one node and re-waits. A second `step` while the stepped node is still running is rejected. Use `resume` to return to full-speed scheduling. Nodes are selected in lexicographic ID order for determinism. @@ -566,10 +587,11 @@ omitted content from its preview. | `condition` | no | Expression evaluated before spawn; node is skipped if false | | `input_mapping` | no | Map upstream node outputs into template variables | | `report_to_parent` | no | If true, the parent agent is woken when this node completes or fails. The workflow's terminal status always wakes the parent regardless of this flag | -| `worker_config` | no | `{ timeout_ms }` — bounds node execution (defaults to 10 minutes if omitted) | +| `worker_config` | no | `{ timeout_ms }` — bounds the node from admission to completion (defaults to 10 minutes if omitted); queue wait counts toward the budget, an expired queued node fails without spawning, and a running node that exceeds it escalates to the parent for adjudication (capped deadline extensions) before failing | | `output_schema` | no | JSON Schema; when declared, the child agent must call `submit_result` to submit structured output — failure to submit results in node failure | | `restart` | no | (replan only) Re-spawn this running node with new prompt | | `cancel` | no | (replan only) Cancel this node | +| `review` | no | (deep review workers) `{ phase: "design" \| "diff" }`; a `diff` review must also declare `implementation_node_id` / `verification_node_id` and wire them: transitive review→verification→implementation dependencies, `input_mapping` for the diff artifact + fingerprint + verification output, a PASS-gated `condition`, and a `verdict`+`implementation_fingerprint` `output_schema`. Authoring rejects violations in deep mode and warns in standard | ### What NOT to expect diff --git a/packages/opencode/src/cli/heap.ts b/packages/opencode/src/cli/heap.ts index e8ec8f1bd0..253bdfa1ce 100644 --- a/packages/opencode/src/cli/heap.ts +++ b/packages/opencode/src/cli/heap.ts @@ -1,14 +1,50 @@ import path from "path" +import * as fs from "fs/promises" import { writeHeapSnapshot } from "node:v8" import { Flag } from "@opencode-ai/core/flag/flag" import { Global } from "@opencode-ai/core/global" const MINUTE = 60_000 const LIMIT = 2 * 1024 * 1024 * 1024 +// Each snapshot is hundreds of MB and RSS storms re-arm; keep only the newest +// few so repeated snapshots cannot fill the log directory. +const RETAINED_SNAPSHOTS = 2 let timer: Timer | undefined let lock = false let armed = true +export function pruneHeapSnapshots(directory: string, keep = RETAINED_SNAPSHOTS) { + return fs + .readdir(directory, { withFileTypes: true }) + .then((entries) => { + const names = entries + .filter((entry) => entry.isFile() && entry.name.startsWith("heap-") && entry.name.endsWith(".heapsnapshot")) + .map((entry) => entry.name) + // Oldest-first by embedded timestamp, NOT by full name: the layout is + // heap--, so a plain lexicographic sort orders snapshots by + // pid across runs (pid digit-count changes and wraparound) and pruning + // would delete the newest snapshot while keeping stale ones. + .sort((a, b) => snapshotTime(a).localeCompare(snapshotTime(b))) + return Promise.all( + names.slice(0, Math.max(0, names.length - keep)).map((name) => + fs.rm(path.join(directory, name), { force: true }).catch((cause) => { + console.warn(`opencode: failed to prune heap snapshot ${name}: ${String(cause)}`) + }), + ), + ) + }) + .catch((cause) => { + // A missing log directory is the normal first-run state; anything else + // is a real prune failure and best-effort cleanup must still surface it. + if ((cause as { code?: string }).code === "ENOENT") return + console.warn(`opencode: failed to list heap snapshots for pruning: ${String(cause)}`) + }) +} + +function snapshotTime(name: string) { + return name.slice(name.lastIndexOf("-") + 1) +} + export function start() { if (!Flag.OPENCODE_AUTO_HEAP_SNAPSHOT) return if (timer) return @@ -32,6 +68,7 @@ export function start() { await Promise.resolve() .then(() => writeHeapSnapshot(file)) .catch(() => {}) + await pruneHeapSnapshots(Global.Path.log) lock = false } diff --git a/packages/opencode/src/dag/blocks.ts b/packages/opencode/src/dag/blocks.ts index aa756a1be0..859dd1347d 100644 --- a/packages/opencode/src/dag/blocks.ts +++ b/packages/opencode/src/dag/blocks.ts @@ -36,7 +36,8 @@ export class WorkflowBlock extends Schema.Class("WorkflowBlock")( timeout_ms: Schema.Number, }), ).annotate({ - description: "{ timeout_ms } — bounds node execution; overrides config.node_defaults.worker_config", + description: + "{ timeout_ms } — bounds the node from admission to completion; queue wait counts toward the budget and an expired queued node fails without spawning. A running node that exceeds it escalates to the parent for adjudication (capped deadline extensions) before failing. Overrides config.node_defaults.worker_config", }), required: Schema.optional(Schema.Boolean).annotate({ description: diff --git a/packages/opencode/src/dag/validation.ts b/packages/opencode/src/dag/validation.ts index 46e9889038..195027ee12 100644 --- a/packages/opencode/src/dag/validation.ts +++ b/packages/opencode/src/dag/validation.ts @@ -202,7 +202,10 @@ export const NodeSchema = Schema.Struct({ Schema.Struct({ timeout_ms: Schema.optional(Schema.Number), }), - ).annotate({ description: "{ timeout_ms } — bounds node execution. Inherits config.node_defaults.worker_config" }), + ).annotate({ + description: + "{ timeout_ms } — bounds the node from admission to completion; queue wait counts toward the budget and an expired queued node fails without spawning. A running node that exceeds it escalates to the parent for adjudication (capped deadline extensions) before failing. Inherits config.node_defaults.worker_config", + }), input_mapping: Schema.optional(Schema.Record(Schema.String, Schema.String)).annotate({ description: 'Optional variable-to-source map, e.g. { resultA: "node-a", count: "node-b.output.count" }. Omit to expose each direct dependency under its node ID', @@ -230,7 +233,7 @@ export const NodeSchema = Schema.Struct({ }), ).annotate({ description: - "(deep review workers) design reviews pre-implementation artifacts; diff reviews require implementation_node_id and verification_node_id", + "(deep review workers) design reviews pre-implementation artifacts; diff reviews require implementation_node_id, verification_node_id, and authoring-validated wiring: transitive review→verification→implementation dependencies, input_mapping for the diff artifact, fingerprint, and verification output, a PASS-gated condition, and a verdict+implementation_fingerprint output_schema; deep mode additionally requires the diff review to feed a required final gate conditioned on verdict ACCEPT. Violations are authoring errors in deep mode, warnings in standard mode.", }), }) diff --git a/packages/opencode/src/hook/settings.ts b/packages/opencode/src/hook/settings.ts index 89dd6c587e..c9851b2575 100644 --- a/packages/opencode/src/hook/settings.ts +++ b/packages/opencode/src/hook/settings.ts @@ -51,6 +51,7 @@ import { generateObject, generateText, type ModelMessage } from "ai" import { FSUtil } from "@opencode-ai/core/fs-util" import { CrossSpawnSpawner } from "@opencode-ai/core/cross-spawn-spawner" import * as Log from "@/util/log" +import { Process } from "@/util/process" import { Global } from "@opencode-ai/core/global" import { InstanceState } from "@/effect/instance-state" import { MCP } from "@/mcp" @@ -1009,6 +1010,12 @@ export function warnUnsupportedFields( const DEFAULT_TIMEOUT_MS = 60_000 // CC default +// #500: after the child exits (or the spawn timeout SIGTERMs it), stdio EOF is +// given this grace before the whole process group is SIGKILLed; DRAIN_MS is +// the post-kill window for final buffered output before resolving partial. +const KILL_GRACE_MS = 2_000 +const DRAIN_MS = 500 + function execShell( entry: HookCommand, stdinJSON: string, @@ -1051,12 +1058,16 @@ function execShell( } } + // POSIX: run the child as a process-group leader so a hung grandchild + // holding the stdio pipes can be signaled as a group (#500). Windows + // relies on `taskkill /T` tree kill instead. const child = spawn(expandedCommand, [], { cwd, shell, env: { ...process.env, ...extraEnv }, stdio: ["pipe", "pipe", "pipe"], timeout: timeoutMs, + detached: process.platform !== "win32", }) let stdout = "" @@ -1078,20 +1089,80 @@ function execShell( log.warn("hook stdin write failed", { command, error: String(err) }) } - child.on("error", (err) => { - log.error("hook command failed to spawn", { command, error: err.message }) - resolve({ exitCode: null, stdout, stderr, spawnError: err.message }) + // #500: `close` only fires after BOTH exit and stdio EOF; a grandchild + // that inherits the pipes and outlives the shell blocks EOF forever, so + // the old `close` waiter never resolved. Exit and stream EOF are now + // awaited as independent conditions, with a process-group SIGKILL as the + // fallback that guarantees resolution. + let exitCode: number | null = null + let settled = false + const timers = new Set() + const arm = (fire: () => void, ms: number) => { + const timer = setTimeout(() => { + timers.delete(timer) + if (!settled) fire() + }, ms) + timers.add(timer) + } + const exited = new Promise((resolveExit) => { + child.on("exit", (code) => { + exitCode = code + resolveExit() + }) }) - - child.on("close", (code) => { + // `end` alone is not enough: on spawn failure the streams can close or + // error without ever reaching EOF. + const streamSettled = (stream: NodeJS.ReadableStream) => + new Promise((resolveStream) => { + stream.on("end", resolveStream) + stream.on("close", resolveStream) + stream.on("error", resolveStream) + }) + const streamsDone = Promise.all([streamSettled(child.stdout), streamSettled(child.stderr)]) + const finish = (spawnError?: string) => { + if (settled) return + settled = true + for (const timer of timers) clearTimeout(timer) + timers.clear() + child.stdout.destroy() + child.stderr.destroy() log.debug("hook close", { command: command.slice(0, 80), - exitCode: code, + exitCode, stdoutLen: stdout.length, stderrLen: stderr.length, }) - resolve({ exitCode: code, stdout, stderr }) + resolve(spawnError === undefined ? { exitCode, stdout, stderr } : { exitCode, stdout, stderr, spawnError }) + } + + child.on("error", (err) => { + log.error("hook command failed to spawn", { command, error: err.message }) + finish(err.message) }) + + let killSent = false + const killGroup = () => { + if (killSent || child.pid === undefined) return + killSent = true + void Process.killGroupPid(child.pid, "SIGKILL").catch((err) => { + log.warn("hook process-group kill failed", { command, error: String(err) }) + }) + } + const afterKill = () => { + killGroup() + const drained = new Promise((resolveDrain) => arm(resolveDrain, DRAIN_MS)) + void Promise.all([exited, Promise.race([streamsDone, drained])]).then(() => finish()) + } + + void Promise.all([exited, streamsDone]).then(() => finish()) + + // Child exited but pipes are still open (grandchild holds them): kill the + // group after the EOF grace, then resolve with whatever was captured. + void exited.then(() => arm(afterKill, KILL_GRACE_MS)) + + // Child ignored the spawn-timeout SIGTERM and never exited: kill the group + // at the absolute deadline, wait for the reap, then resolve. + arm(afterKill, timeoutMs + KILL_GRACE_MS) }) } diff --git a/packages/opencode/src/mcp/index.ts b/packages/opencode/src/mcp/index.ts index 2bbdcf95cb..5b37cf8250 100644 --- a/packages/opencode/src/mcp/index.ts +++ b/packages/opencode/src/mcp/index.ts @@ -12,6 +12,7 @@ import { ConfigMCPV1 } from "@opencode-ai/core/v1/config/mcp" import { NamedError } from "@opencode-ai/core/util/error" import { InstallationVersion } from "@opencode-ai/core/installation/version" import { withTimeout } from "@/util/timeout" +import { Process } from "@/util/process" import { FSUtil } from "@opencode-ai/core/fs-util" import { McpOAuthProvider, OAUTH_CALLBACK_PATH } from "./oauth-provider" import { McpOAuthCallback } from "./oauth-callback" @@ -398,7 +399,10 @@ export const layer = Layer.effect( } satisfies CreateResult }).pipe( Effect.catchCause((cause) => - Effect.tryPromise(() => mcpClient.close()).pipe(Effect.ignore, Effect.andThen(Effect.failCause(cause))), + Effect.gen(function* () { + yield* shutdownClient(mcpClient) + return yield* Effect.failCause(cause) + }), ), ) }, @@ -437,6 +441,19 @@ export const layer = Layer.effect( Effect.catch(() => Effect.succeed([] as number[])), ) + // Close a client and make sure its whole process tree is reaped. The + // descendant snapshot must be taken while the root pid is alive (pgrep + // walks parent links); close() shuts the root down gracefully, and + // stopTree force-kills whatever survived (ignored SIGTERM, orphaned + // grandchildren) and waits for exit. + const shutdownClient = Effect.fnUntraced(function* (client: MCPClient) { + const pid = client.transport instanceof StdioClientTransport ? client.transport.pid : null + const tree = typeof pid === "number" ? [pid, ...(yield* descendants(pid))] : [] + yield* Effect.tryPromise(() => client.close()).pipe(Effect.ignore) + if (tree.length === 0) return + yield* Effect.tryPromise(() => Process.stopTree(tree)).pipe(Effect.ignore) + }) + function watch(s: State, name: string, client: MCPClient, bridge: EffectBridge.Shape, timeout?: number) { // mcp-elicitation-notification: handle `elicitation/create` reverse requests. // Routes through the Question service (best-effort session via SessionContext), @@ -536,23 +553,7 @@ export const layer = Layer.effect( s.clients = {} s.defs = {} s.instructions = {} - yield* Effect.forEach( - clients, - (client) => - Effect.gen(function* () { - const pid = client.transport instanceof StdioClientTransport ? client.transport.pid : null - if (typeof pid === "number") { - const pids = yield* descendants(pid) - for (const dpid of pids) { - try { - process.kill(dpid, "SIGTERM") - } catch {} - } - } - yield* Effect.tryPromise(() => client.close()).pipe(Effect.ignore) - }), - { concurrency: "unbounded" }, - ) + yield* Effect.forEach(clients, (client) => shutdownClient(client), { concurrency: "unbounded" }) pendingOAuthTransports.clear() }), ) @@ -567,7 +568,7 @@ export const layer = Layer.effect( delete s.defs[name] delete s.instructions[name] if (!client) return Effect.void - return Effect.tryPromise(() => client.close()).pipe(Effect.ignore) + return shutdownClient(client) } const storeClient = Effect.fnUntraced(function* ( @@ -586,7 +587,7 @@ export const layer = Layer.effect( if (instructions) s.instructions[name] = instructions else delete s.instructions[name] watch(s, name, client, bridge, timeout) - if (previous) yield* Effect.tryPromise(() => previous.close()).pipe(Effect.ignore) + if (previous) yield* shutdownClient(previous) return s.status[name] }) diff --git a/packages/opencode/src/memory/store.ts b/packages/opencode/src/memory/store.ts index e371f0f4e9..aafe852fd2 100644 --- a/packages/opencode/src/memory/store.ts +++ b/packages/opencode/src/memory/store.ts @@ -29,6 +29,10 @@ const METADATA_KEYS = [ ] as const const ITEM_KEYS = ["id", "kind", "content", "rationale", "confirmed_at"] as const +// Keep a few recent generations on disk: a reader holding a just-published +// manifest must still find its generation after later commits GC older ones. +const RETAINED_GENERATIONS = 3 + const PROHIBITED_CONTENT = [ /```|`[^`]+`/, /(?:^|\s)(?:~\/|\.\.?\/|\/)[^\s]+/, @@ -236,6 +240,25 @@ export const layer = Layer.effect( } satisfies Snapshot }) + const gcGenerations = Effect.fnUntraced(function* (projectID: ProjectV2.ID) { + const generations = home.generations(projectID) + const entries = yield* fs.readDirectoryEntries(generations) + const stale = entries + .filter((entry) => entry.type === "directory" && !entry.name.startsWith(".")) + .sort( + (a, b) => Number.parseInt(b.name, 10) - Number.parseInt(a.name, 10) || b.name.localeCompare(a.name), + ) + .slice(RETAINED_GENERATIONS) + .map((entry) => join(generations, entry.name)) + // Orphan staging directories are rename leftovers from crashed writes. + const staging = entries + .filter((entry) => entry.name.startsWith(".") && entry.name.endsWith(".tmp")) + .map((entry) => join(generations, entry.name)) + yield* Effect.forEach([...stale, ...staging], (path) => fs.remove(path, { force: true, recursive: true }), { + discard: true, + }) + }) + const writeSnapshot = Effect.fnUntraced(function* ( projectID: ProjectV2.ID, revision: number, @@ -265,6 +288,11 @@ export const layer = Layer.effect( ) }).pipe(Effect.onError(() => fs.remove(staging, { force: true, recursive: true }).pipe(Effect.ignore))) yield* fs.remove(home.topics(projectID), { force: true, recursive: true }).pipe(Effect.ignore) + // GC is best-effort: the commit has already landed, a cleanup failure + // must never fail it. + yield* gcGenerations(projectID).pipe( + Effect.catchCause((cause) => Effect.logWarning("memory generation GC failed", { cause: cause })), + ) }) const readTopics = Effect.fn("MemoryStore.readTopics")((projectID: ProjectV2.ID) => diff --git a/packages/opencode/src/share/share-next.ts b/packages/opencode/src/share/share-next.ts index 4269c65252..172e94d6cf 100644 --- a/packages/opencode/src/share/share-next.ts +++ b/packages/opencode/src/share/share-next.ts @@ -150,9 +150,16 @@ export const layer = Layer.effect( const state: InstanceState.InstanceState = yield* InstanceState.make( Effect.fn("ShareNext.state")(function* (_ctx) { const cache: State = { queue: new Map(), scope: yield* Scope.make(), shared: new Map() } + // EventV2 listeners live in a process-level array; collect their + // unsubscribers or every instance remount leaks another batch of + // subscribers pinning this closure. + const unsubscribers: Array = [] yield* Effect.addFinalizer(() => - Scope.close(cache.scope, Exit.void).pipe( + // Unsubscribe before closing the scope so no in-flight event lands + // in a subscriber whose fork scope is already gone. + Effect.forEach(unsubscribers, (unsubscribe) => unsubscribe, { discard: true }).pipe( + Effect.andThen(Scope.close(cache.scope, Exit.void)), Effect.andThen( Effect.sync(() => { cache.queue.clear() @@ -182,7 +189,7 @@ export const layer = Layer.effect( Effect.logError("share subscriber failed", { type: def.type, cause: cause }), ), ) - }) + }).pipe(Effect.tap((unsubscribe) => Effect.sync(() => unsubscribers.push(unsubscribe)))) yield* watch(Session.Event.Updated, (data) => Effect.gen(function* () { diff --git a/packages/opencode/src/tool/registry.ts b/packages/opencode/src/tool/registry.ts index aa7b437f2c..a2f40d7f01 100644 --- a/packages/opencode/src/tool/registry.ts +++ b/packages/opencode/src/tool/registry.ts @@ -77,6 +77,10 @@ type AgentCatalogOptions = { heading: string includeHidden: boolean includeModelState: boolean + /** Workflow worker_type defaults compile to the native build/plan primary + * agents, so the workflow catalog lists native primaries even though the + * task subagent catalog and user-defined primary modes stay filtered out. */ + includePrimary?: boolean } export interface Interface { @@ -278,7 +282,7 @@ export const layer = Layer.effect( options: AgentCatalogOptions, ) { const description = (yield* agents.list()) - .filter((item) => item.mode !== "primary") + .filter((item) => item.mode !== "primary" || (options.includePrimary && item.native)) .filter((item) => options.includeHidden || !item.hidden) .filter((item) => Permission.evaluate("task", item.name, caller.permission).action !== "deny") .toSorted((a, b) => a.name.localeCompare(b.name)) @@ -333,6 +337,7 @@ export const layer = Layer.effect( heading: "Available workflow worker_type values:", includeHidden: false, includeModelState: true, + includePrimary: true, }) : undefined, ] diff --git a/packages/opencode/src/tool/workflow.ts b/packages/opencode/src/tool/workflow.ts index 0204a1b432..e33a6cede5 100644 --- a/packages/opencode/src/tool/workflow.ts +++ b/packages/opencode/src/tool/workflow.ts @@ -57,7 +57,7 @@ export { Parameters as WorkflowParameters } // ============================================================================ const specPathDescription = - '(start/extend/control replan/read/validate) An exact saved workflow name returned by workflow(action="list"), or a path to a YAML workflow spec. Graph content belongs in that file; relative paths resolve from the session directory' + '(start/extend/control replan/read/validate) An exact saved workflow name returned by workflow(action="list"), or a path to a YAML workflow spec. The `builtin://` path marker list shows for builtin templates also resolves, by name. Graph content belongs in that file; relative paths resolve from the session directory' const StartPath = Schema.Struct({ action: Schema.Literal("start").annotate({ description: "Create a workflow" }), @@ -913,19 +913,19 @@ function searchedScopes(directory: string) { function resolveSpecPath(specPath: string, directory: string, ctx: Tool.Context) { return Effect.gen(function* () { - // A bare name addresses the workflow library. Its project/global scopes - // are curated assets the user placed under `.opencode/` or the config - // dir — the same trust level as dag.jsonc — so a resolved name needs no - // external-directory prompt even when the global scope lands outside the - // session directory. Arbitrary paths below keep the prompt. - if (DagWorkflows.isName(specPath)) { - const entry = yield* DagWorkflows.resolve(specPath, directory) + // A bare name addresses the workflow library. The synthetic + // `builtin://name` marker list output advertises must round-trip the same + // way: strip the scheme and resolve by name instead of letting the path + // branch reject it with a cwd-joined extension error. Its project/global + // scopes are curated assets the user placed under `.opencode/` or the + // config dir — the same trust level as dag.jsonc — so a resolved name + // needs no external-directory prompt even when the global scope lands + // outside the session directory. Arbitrary paths below keep the prompt. + const name = DagWorkflows.isBuiltinPath(specPath) ? DagWorkflows.builtinName(specPath) : specPath + if (DagWorkflows.isName(name)) { + const entry = yield* DagWorkflows.resolve(name, directory) if (entry) return entry.path - return yield* Effect.fail( - new Error( - `Saved workflow not found: "${specPath}". Searched ${searchedScopes(directory)}. Run workflow(action: "list") to see what is available, or pass a path to a .yaml spec file.`, - ), - ) + return yield* Effect.fail(savedWorkflowNotFound(name, directory)) } const filepath = path.isAbsolute(specPath) ? path.normalize(specPath) : path.resolve(directory, specPath) if (![".yaml", ".yml"].includes(path.extname(filepath).toLowerCase())) { @@ -940,6 +940,12 @@ function resolveSpecPath(specPath: string, directory: string, ctx: Tool.Context) }) } +function savedWorkflowNotFound(name: string, directory: string) { + return new Error( + `Saved workflow not found: "${name}". Searched ${searchedScopes(directory)}. Run workflow(action: "list") to see what is available, or pass a path to a .yaml spec file.`, + ) +} + /** Terminal-workflow rejections surface as defects carrying recovery * guidance, not bare iron-law errors. Shared by the replan and extend paths. */ function withTerminalRecovery(effect: Effect.Effect, guidance: string) { diff --git a/packages/opencode/src/util/process.ts b/packages/opencode/src/util/process.ts index 173210f23c..94d82cee87 100644 --- a/packages/opencode/src/util/process.ts +++ b/packages/opencode/src/util/process.ts @@ -144,6 +144,26 @@ export async function run(cmd: string[], opts: RunOptions = {}): Promise throw new RunFailedError(cmd, out.code, out.stdout, out.stderr) } +// Bounded-stop escalation constants, shared by stop() and stopTree(): time +// allowed for exit after SIGTERM before escalating to SIGKILL, and the bounded +// wait for exit after SIGKILL. +export const STOP_TERM_GRACE_MS = 3_000 +export const STOP_KILL_GRACE_MS = 2_000 + +// Platform group-kill primitive: POSIX signals the process group led by `pid` +// (the child must be a detached group leader); win32 has no group semantics, +// so `taskkill /T /F` tree-kills instead and `signal` is ignored. Resolves +// once the kill is delivered — on win32 that means awaiting the taskkill exit +// code — and throws when delivery fails, leaving fallback and logging to the +// caller. +export async function killGroupPid(pid: number, signal: NodeJS.Signals = "SIGKILL") { + if (process.platform !== "win32") { + process.kill(-pid, signal) + return + } + await run(["taskkill", "/pid", String(pid), "/T", "/F"]) +} + // Duplicated in `packages/sdk/js/src/process.ts` because the SDK cannot import // `opencode` without creating a cycle. Keep both copies in sync. export async function stop(proc: ChildProcess) { @@ -151,15 +171,81 @@ export async function stop(proc: ChildProcess) { if (process.platform !== "win32" || !proc.pid) { proc.kill() + if (await exitedWithin(proc, STOP_TERM_GRACE_MS)) return + proc.kill("SIGKILL") + await exitedWithin(proc, STOP_KILL_GRACE_MS) return } - const out = await run(["taskkill", "/pid", String(proc.pid), "/T", "/F"], { - nothrow: true, + try { + await killGroupPid(proc.pid) + } catch { + proc.kill() + } +} + +function exitedWithin(proc: ChildProcess, timeoutMs: number) { + if (proc.exitCode !== null || proc.signalCode !== null) return Promise.resolve(true) + return new Promise((resolve) => { + const done = () => { + clearTimeout(timer) + resolve(true) + } + const timer = setTimeout(() => { + proc.off("exit", done) + proc.off("error", done) + resolve(proc.exitCode !== null || proc.signalCode !== null) + }, timeoutMs) + proc.once("exit", done) + proc.once("error", done) + }) +} + +export interface StopTreeOptions { + termGraceMs?: number + killGraceMs?: number +} + +// Kill every pid in the list: SIGTERM round, bounded wait, SIGKILL round, +// bounded wait. The pids may belong to processes we did not spawn +// (grandchildren), so liveness is polled with signal 0 instead of exit events. +export async function stopTree(pids: number[], opts: StopTreeOptions = {}) { + const targets = pids.filter((pid) => pid > 1) + if (targets.length === 0) return + signalTree(targets, "SIGTERM") + if (await treeExitedWithin(targets, opts.termGraceMs ?? STOP_TERM_GRACE_MS)) return + signalTree(targets, "SIGKILL") + await treeExitedWithin(targets, opts.killGraceMs ?? STOP_KILL_GRACE_MS) +} + +function signalTree(pids: number[], signal: NodeJS.Signals) { + for (const pid of pids) { + try { + process.kill(pid, signal) + } catch {} + } +} + +function treeExitedWithin(pids: number[], timeoutMs: number) { + const deadline = Date.now() + timeoutMs + return new Promise((resolve) => { + const tick = () => { + if (pids.every((pid) => !alive(pid))) return resolve(true) + if (Date.now() >= deadline) return resolve(false) + setTimeout(tick, 50) + } + tick() }) +} - if (out.code === 0) return - proc.kill() +function alive(pid: number) { + try { + process.kill(pid, 0) + return true + } catch (error) { + // EPERM: the process exists but belongs to another user. + return (error as NodeJS.ErrnoException).code === "EPERM" + } } export async function text(cmd: string[], opts: RunOptions = {}): Promise { diff --git a/packages/opencode/test/cli/heap.test.ts b/packages/opencode/test/cli/heap.test.ts new file mode 100644 index 0000000000..ed93cec93b --- /dev/null +++ b/packages/opencode/test/cli/heap.test.ts @@ -0,0 +1,61 @@ +import { describe, expect, test } from "bun:test" +import * as fs from "node:fs/promises" +import path from "node:path" +import { Heap } from "@/cli/heap" +import { tmpdir } from "../fixture/fixture" + +describe("heap snapshot rotation", () => { + test("prunes old snapshots keeping only the newest", async () => { + await using dir = await tmpdir() + const names = [ + "heap-111-20260101T000000000Z", + "heap-111-20260102T000000000Z", + "heap-111-20260103T000000000Z", + "heap-111-20260104T000000000Z", + ] + for (const name of names) await fs.writeFile(path.join(dir.path, `${name}.heapsnapshot`), "x") + await fs.writeFile(path.join(dir.path, "heap-not-a-snapshot.log"), "x") + await fs.writeFile(path.join(dir.path, "other-20260101T000000000Z.heapsnapshot"), "x") + + await Heap.pruneHeapSnapshots(dir.path) + + expect((await fs.readdir(dir.path)).sort()).toEqual([ + "heap-111-20260103T000000000Z.heapsnapshot", + "heap-111-20260104T000000000Z.heapsnapshot", + "heap-not-a-snapshot.log", + "other-20260101T000000000Z.heapsnapshot", + ]) + }) + + test("leaves fewer snapshots than the retention limit untouched", async () => { + await using dir = await tmpdir() + await fs.writeFile(path.join(dir.path, "heap-111-20260101T000000000Z.heapsnapshot"), "x") + + await Heap.pruneHeapSnapshots(dir.path) + + expect(await fs.readdir(dir.path)).toEqual(["heap-111-20260101T000000000Z.heapsnapshot"]) + }) + + test("prunes by embedded timestamp when pids differ across runs", async () => { + await using dir = await tmpdir() + // Lexicographic order of the full names puts heap-1000-* before heap-999-*, + // so a name-sorted prune would delete the NEWEST snapshot (1000-0103). + const names = [ + "heap-999-20260101T000000000Z", + "heap-999-20260102T000000000Z", + "heap-1000-20260103T000000000Z", + ] + for (const name of names) await fs.writeFile(path.join(dir.path, `${name}.heapsnapshot`), "x") + + await Heap.pruneHeapSnapshots(dir.path) + + expect((await fs.readdir(dir.path)).sort()).toEqual([ + "heap-1000-20260103T000000000Z.heapsnapshot", + "heap-999-20260102T000000000Z.heapsnapshot", + ]) + }) + + test("tolerates a missing log directory", async () => { + await expect(Heap.pruneHeapSnapshots(path.join("/", "opencode-missing-log-dir"))).resolves.toBeUndefined() + }) +}) diff --git a/packages/opencode/test/dag/workflow-tool.test.ts b/packages/opencode/test/dag/workflow-tool.test.ts index 5c564f178f..aed9d638a1 100644 --- a/packages/opencode/test/dag/workflow-tool.test.ts +++ b/packages/opencode/test/dag/workflow-tool.test.ts @@ -2560,6 +2560,52 @@ describe("workflow tool saved workflows", () => { ), ) + runtime.effect("builtin:// spec_path from list output round-trips by name", () => + withGlobalConfigDir((globalDir) => + Effect.gen(function* () { + const routeSpec = (name: string) => + `title: ${name} title\nconfig:\n name: ${name}\n objective: Route objective\n blocks:\n - id: plan\n kind: plan\n` + yield* Effect.promise(() => + Bun.write(path.join(globalDir, "workflows", "marker-route.yaml"), routeSpec("global-route")), + ) + const previousBuiltin = (globalThis as Record).OPENCODE_DAG_TEMPLATES + ;(globalThis as Record).OPENCODE_DAG_TEMPLATES = { + "marker-builtin": routeSpec("marker-builtin-route"), + } + try { + const info = yield* WorkflowTool + const workflow = yield* info.init() + + // the synthetic builtin:// marker resolves by name through the library + const builtinRead = yield* workflow.execute( + { params: { action: "read", spec_path: "builtin://marker-builtin" }}, + contextWith([]), + ) + expect(JSON.parse(builtinRead.output).spec.title).toContain("marker-builtin-route") + + // the marker resolves through the same shadowing chain as a bare name + const shadowed = yield* workflow.execute( + { params: { action: "validate", spec_path: "builtin://marker-route" }}, + contextWith([]), + ) + expect(JSON.parse(shadowed.output).source).toBe(path.join(globalDir, "workflows", "marker-route.yaml")) + + // unknown builtin names fail as a library lookup, not a path extension error + const missingExit = yield* Effect.exit( + workflow.execute({ params: { action: "read", spec_path: "builtin://missing-route" }}, contextWith([])), + ) + expect(Exit.isFailure(missingExit)).toBe(true) + if (Exit.isFailure(missingExit)) { + expect(Cause.pretty(missingExit.cause)).toContain('Saved workflow not found: "missing-route"') + } + } finally { + if (previousBuiltin === undefined) delete (globalThis as Record).OPENCODE_DAG_TEMPLATES + else (globalThis as Record).OPENCODE_DAG_TEMPLATES = previousBuiltin + } + }), + ), + ) + runtime.effect("list marks invalid templates without hiding them", () => withGlobalConfigDir((globalDir) => Effect.gen(function* () { diff --git a/packages/opencode/test/hook/grandchild-pipe-hang.test.ts b/packages/opencode/test/hook/grandchild-pipe-hang.test.ts new file mode 100644 index 0000000000..ca5f31c0dd --- /dev/null +++ b/packages/opencode/test/hook/grandchild-pipe-hang.test.ts @@ -0,0 +1,91 @@ +import { describe, expect } from "bun:test" +import { Effect, Layer } from "effect" +import { spawnSync } from "child_process" +import * as fs from "fs/promises" +import path from "path" +import { SettingsHook } from "@/hook/settings" +import { SessionHooks } from "@/hook/session-hooks" +import { EventV2Bridge } from "@/event-v2-bridge" +import { Database } from "@opencode-ai/core/database/database" +import { testEffect } from "../lib/effect" + +// #500: a hook command that spawns a grandchild holding the stdio pipes and +// then hits its timeout must not hang the trigger forever. execShell kills +// the child's process group once the EOF grace elapses, so the trigger +// returns within the timeout window plus grace, and the grandchild is gone. +// Mirrors stdout-context.test.ts (real SettingsHook layer, execShell actually +// runs the command, hooks.json written via init). + +const testLayer = SettingsHook.layer.pipe( + Layer.provide(EventV2Bridge.defaultLayer), + Layer.provide(Database.defaultLayer), + Layer.provideMerge(SessionHooks.defaultLayer), +) +const it = testEffect(testLayer) + +const writeHooks = (hooks: unknown) => (dir: string) => + Effect.promise(() => + fs.mkdir(path.join(dir, ".opencode"), { recursive: true }).then(() => + fs.writeFile(path.join(dir, ".opencode", "hooks.json"), JSON.stringify(hooks)), + ), + ) + +// Unique marker so pgrep only matches this test's grandchild. +const GRANDCHILD = "sleep 597.3" + +const grandchildGone = () => + Effect.promise(async () => { + for (let i = 0; i < 20; i++) { + // pgrep exits 1 when no process matches; a null status means pgrep is + // unavailable — nothing to assert. + if (spawnSync("pgrep", ["-f", GRANDCHILD]).status !== 0) return true + await new Promise((resolve) => setTimeout(resolve, 100)) + } + return false + }) + +describe("SettingsHook execShell pipe-holding grandchild (#500)", () => { + it.instance( + "timed-out hook with pipe-holding grandchild resolves and kills the group", + () => + Effect.gen(function* () { + if (process.platform === "win32") return + const hook = yield* SettingsHook.Service + const startedAt = Date.now() + const r = yield* hook.trigger( + { event: "UserPromptSubmit", prompt: "test" }, + { sessionID: "sess-500-1", transcriptPath: "" }, + ) + const elapsed = Date.now() - startedAt + // Must resolve far below the 597s the grandchild would hold the pipe. + expect(elapsed).toBeLessThan(15_000) + expect(r.additionalContexts).toEqual([]) + expect(yield* grandchildGone()).toBe(true) + }), + { + init: writeHooks({ + UserPromptSubmit: [{ hooks: [{ type: "command", command: `${GRANDCHILD} & wait`, timeout: 1 }] }], + }), + }, + { timeout: 30_000 }, + ) + + it.instance( + "fast command with a configured timeout still completes normally", + () => + Effect.gen(function* () { + const hook = yield* SettingsHook.Service + const r = yield* hook.trigger( + { event: "UserPromptSubmit", prompt: "test" }, + { sessionID: "sess-500-2", transcriptPath: "" }, + ) + expect(r.additionalContexts).toEqual(["fast-ok-500"]) + }), + { + init: writeHooks({ + UserPromptSubmit: [{ hooks: [{ type: "command", command: "printf '%s' 'fast-ok-500'", timeout: 30 }] }], + }), + }, + { timeout: 10_000 }, + ) +}) diff --git a/packages/opencode/test/mcp/fixtures/process-tree-probe.ts b/packages/opencode/test/mcp/fixtures/process-tree-probe.ts new file mode 100644 index 0000000000..9806e2a5f7 --- /dev/null +++ b/packages/opencode/test/mcp/fixtures/process-tree-probe.ts @@ -0,0 +1,84 @@ +// Runs in a fresh bun process so the real @modelcontextprotocol transports are +// used even when the surrounding test run has mock.module overrides active +// (Bun's module registry is process-global across the suite). Drives +// MCP.Service against the server-stdio fixture: connect a local server that +// spawns a child, disconnect, and report whether the whole process tree was +// reaped (issue #503). +import fs from "fs/promises" +import path from "path" +import { Effect } from "effect" +import { StdioClientTransport } from "@modelcontextprotocol/client/stdio" +import { MCP } from "../../../src/mcp/index" +import { TestInstance, withTmpdirInstance } from "../../fixture/fixture" + +function alive(pid: number) { + try { + process.kill(pid, 0) + return true + } catch { + return false + } +} + +function waitDead(pid: number, timeoutMs: number) { + return new Promise((resolve) => { + const deadline = Date.now() + timeoutMs + const tick = () => { + if (!alive(pid)) return resolve(true) + if (Date.now() >= deadline) return resolve(false) + setTimeout(tick, 50) + } + tick() + }) +} + +async function waitForPidFile(file: string, timeoutMs: number) { + const deadline = Date.now() + timeoutMs + while (Date.now() < deadline) { + try { + return parseInt(await fs.readFile(file, "utf8"), 10) + } catch { + await new Promise((resolve) => setTimeout(resolve, 50)) + } + } + return undefined +} + +const result = await Effect.runPromise( + withTmpdirInstance({ config: { mcp: {} } })( + Effect.gen(function* () { + const mcp = yield* MCP.Service + const { directory } = yield* TestInstance + const childPidFile = path.join(directory, "fixture-child.pid") + + yield* mcp.add("tree-server", { + type: "local", + command: [process.execPath, path.join(import.meta.dir, "server-stdio.ts")], + environment: { MCP_FIXTURE_CHILD_PID_FILE: childPidFile }, + }) + + const status = (yield* mcp.status())["tree-server"] + if (status?.status !== "connected") return { ok: false, stage: "connect", status } + + const client = (yield* mcp.clients())["tree-server"] + const rootPid = client?.transport instanceof StdioClientTransport ? client.transport.pid : null + if (typeof rootPid !== "number") return { ok: false, stage: "root-pid" } + + const childPid = yield* Effect.promise(() => waitForPidFile(childPidFile, 5_000)) + if (childPid === undefined) return { ok: false, stage: "child-pid-file", rootPid } + + yield* mcp.disconnect("tree-server") + + return { + ok: true, + rootDead: yield* Effect.promise(() => waitDead(rootPid, 10_000)), + childDead: yield* Effect.promise(() => waitDead(childPid, 10_000)), + rootPid, + childPid, + } + }), + ).pipe(Effect.scoped, Effect.provide(MCP.defaultLayer)), +) + +console.log(JSON.stringify(result)) +if (!result.ok || !result.rootDead || !result.childDead) process.exit(1) diff --git a/packages/opencode/test/mcp/fixtures/server-stdio.ts b/packages/opencode/test/mcp/fixtures/server-stdio.ts index b54938541b..73d1b93706 100644 --- a/packages/opencode/test/mcp/fixtures/server-stdio.ts +++ b/packages/opencode/test/mcp/fixtures/server-stdio.ts @@ -1,9 +1,21 @@ // Dual-era fixture: the SAME factory serves 2026-07-28 (server/discover) and // legacy (initialize) clients — serveStdio owns the era decision per connection. +import { spawn } from "node:child_process" +import { writeFileSync } from "node:fs" import { z } from "zod" import { McpServer } from "@modelcontextprotocol/server" import { serveStdio } from "@modelcontextprotocol/server/stdio" +// Process-tree fixture (#503): optionally spawn a long-lived child so tests can +// assert the server's whole tree is reaped on disconnect. The child's pid is +// published to the file named by MCP_FIXTURE_CHILD_PID_FILE. +const childPidFile = process.env.MCP_FIXTURE_CHILD_PID_FILE +if (childPidFile) { + const child = spawn(process.execPath, ["-e", "setInterval(() => {}, 60000)"], { stdio: "ignore" }) + child.unref() + writeFileSync(childPidFile, String(child.pid)) +} + serveStdio( () => { const server = new McpServer({ name: "v2-fixture", version: "1.0.0" }, { capabilities: { tools: {} } }) diff --git a/packages/opencode/test/mcp/process-tree.test.ts b/packages/opencode/test/mcp/process-tree.test.ts new file mode 100644 index 0000000000..0e10ca7112 --- /dev/null +++ b/packages/opencode/test/mcp/process-tree.test.ts @@ -0,0 +1,27 @@ +import path from "node:path" +import { expect, test } from "bun:test" + +// Issue #503: dynamic disconnect must reap the local server's whole process +// tree — the root stdio process AND any children it spawned — not just close +// the client. The probe runs in a subprocess because sibling mcp test files +// mock @modelcontextprotocol/client via mock.module, whose registry is +// process-global across the suite; a fresh process guarantees the real +// transports (same pattern as session-recovery.test.ts). +test("mcp disconnect kills the local server process and its spawned child", async () => { + if (process.platform === "win32") return // descendants discovery is POSIX-only + + const child = Bun.spawn([process.execPath, path.join(import.meta.dir, "fixtures", "process-tree-probe.ts")], { + cwd: path.join(import.meta.dir, "../.."), + stdout: "pipe", + stderr: "pipe", + }) + const [code, stdout, stderr] = await Promise.all([ + child.exited, + Bun.readableStreamToText(child.stdout), + Bun.readableStreamToText(child.stderr), + ]) + + expect(code, stderr).toBe(0) + const jsonLine = stdout.trimEnd().split("\n").pop() + expect(JSON.parse(jsonLine ?? "")).toMatchObject({ ok: true, rootDead: true, childDead: true }) +}) diff --git a/packages/opencode/test/memory/memory-persistence.test.ts b/packages/opencode/test/memory/memory-persistence.test.ts index a1d5421e0f..4d00f70f86 100644 --- a/packages/opencode/test/memory/memory-persistence.test.ts +++ b/packages/opencode/test/memory/memory-persistence.test.ts @@ -7,6 +7,7 @@ import { AbsolutePath } from "@opencode-ai/core/schema" import { EffectFlock } from "@opencode-ai/core/util/effect-flock" import { CrossSpawnSpawner } from "@opencode-ai/core/cross-spawn-spawner" import { Effect, Exit, Fiber, Layer, Ref, Schema } from "effect" +import { chmod } from "node:fs/promises" import path from "node:path" import { MemoryConfig } from "@/memory/config" import { MemoryHome } from "@/memory/home" @@ -771,6 +772,87 @@ describe("Project-owned MEMORY persistence", () => { }), ) + it.live( + "retains only the newest generations after repeated commits and sweeps orphan staging", + () => + Effect.gen(function* () { + const root = yield* tmpdirScoped() + yield* Effect.gen(function* () { + const fs = yield* FSUtil.Service + const home = yield* MemoryHome.Service + const store = yield* MemoryStore.Service + + // Crash leftover: a staging directory that never reached its rename. + const orphan = path.join(home.generations(projectID), ".0-crashed.tmp") + yield* fs.makeDirectory(orphan, { recursive: true }) + yield* fs.writeFileString(path.join(orphan, "project-architecture.yaml"), "id: orphan\n") + + for (let i = 1; i <= 5; i++) { + yield* replaceTopics(store, projectID, [topic(`第${i}版已确认架构边界`)]) + } + + const entries = yield* fs.readDirectoryEntries(home.generations(projectID)) + const generations = entries + .filter((entry) => !entry.name.startsWith(".")) + .map((entry) => entry.name) + .sort((a, b) => Number.parseInt(a, 10) - Number.parseInt(b, 10)) + expect(generations).toHaveLength(3) + expect(generations.map((name) => Number.parseInt(name, 10))).toEqual([3, 4, 5]) + expect(entries.some((entry) => entry.name.endsWith(".tmp"))).toBe(false) + // The manifest still points at a retained generation. + expect(yield* store.readSnapshot(projectID)).toEqual({ + revision: 5, + topics: [topic("第5版已确认架构边界")], + }) + }).pipe(Effect.provide(layers(root))) + }), + ) + + // Windows chmod is a no-op on directories, so the undeletable-generation + // injection cannot be staged there. + const itPosix = process.platform === "win32" ? it.live.skip : it.live + itPosix( + "keeps committing when generation GC cannot delete a stale generation", + () => + Effect.gen(function* () { + const root = yield* tmpdirScoped() + yield* Effect.gen(function* () { + const fs = yield* FSUtil.Service + const home = yield* MemoryHome.Service + const store = yield* MemoryStore.Service + + for (let i = 1; i <= 3; i++) { + yield* replaceTopics(store, projectID, [topic(`第${i}版已确认架构边界`)]) + } + const entries = yield* fs.readDirectoryEntries(home.generations(projectID)) + const oldest = entries + .filter((entry) => !entry.name.startsWith(".")) + .map((entry) => entry.name) + .sort((a, b) => Number.parseInt(a, 10) - Number.parseInt(b, 10))[0] + const oldestPath = path.join(home.generations(projectID), oldest) + + // A read-only directory with a file inside cannot be removed; GC + // fails while the commit that already landed must not. + yield* Effect.acquireUseRelease( + Effect.promise(() => chmod(oldestPath, 0o555)), + () => + Effect.gen(function* () { + const exit = yield* Effect.exit(replaceTopics(store, projectID, [topic("第四版已确认架构边界")])) + expect(Exit.isSuccess(exit)).toBe(true) + expect(yield* store.readSnapshot(projectID)).toEqual({ + revision: 4, + topics: [topic("第四版已确认架构边界")], + }) + // The undeletable generation is still on disk — the failure + // is contained in GC, not the commit. + expect(yield* fs.exists(oldestPath)).toBe(true) + }), + () => Effect.promise(() => chmod(oldestPath, 0o755)).pipe(Effect.ignore), + ) + }).pipe(Effect.provide(layers(root))) + }), + ) + it.live( "an orphaned staging generation never shadows the committed generation (MEM-PR01-R1-21)", () => diff --git a/packages/opencode/test/memory/memory.test.ts b/packages/opencode/test/memory/memory.test.ts index f6e85eb5d3..cd0ea57fcd 100644 --- a/packages/opencode/test/memory/memory.test.ts +++ b/packages/opencode/test/memory/memory.test.ts @@ -1716,7 +1716,11 @@ describe("memory hidden model", () => { catch: (cause) => cause, }).pipe(Effect.flip) expect(error instanceof MemoryModel.Stalled).toBe(true) - expect(Date.now() - started).toBeLessThan(200) + // Fail-fast bound, not a scheduler bound: drainWithLiveness arms raw + // setTimeout (no TestClock), and a loaded linux runner was observed + // firing the 40ms connectTimeout at 288ms. 2000ms still separates + // fail-fast from hang (a hang trips the test timeout instead). + expect(Date.now() - started).toBeLessThan(2000) }), ) diff --git a/packages/opencode/test/share/share-next.test.ts b/packages/opencode/test/share/share-next.test.ts index 7a9a2f6747..6d418f429b 100644 --- a/packages/opencode/test/share/share-next.test.ts +++ b/packages/opencode/test/share/share-next.test.ts @@ -1,5 +1,5 @@ import { beforeEach, describe, expect } from "bun:test" -import { Effect, Exit, Layer, Option } from "effect" +import { Effect, Exit, Layer, Option, Context } from "effect" import { HttpClient, HttpClientRequest, HttpClientResponse } from "effect/unstable/http" import { LayerNode } from "@opencode-ai/core/effect/layer-node" import { httpClient } from "@opencode-ai/core/effect/layer-node-platform" @@ -9,6 +9,8 @@ import { SessionProjector } from "@opencode-ai/core/session/projector" import { AccessToken, AccountID, OrgID, RefreshToken } from "../../src/account/schema" import { AccountRepo } from "../../src/account/repo" import { EventV2Bridge } from "../../src/event-v2-bridge" +import { InstanceStore } from "../../src/project/instance-store" +import { EventV2 } from "@opencode-ai/core/event" import { Session } from "@/session/session" import type { SessionID } from "../../src/session/schema" import { ShareNext } from "@/share/share-next" @@ -55,6 +57,43 @@ function integrationLayer(client: HttpClient.HttpClient) { ) } +type ListenerCounts = { listen: number; removed: number } + +// Wraps the real bridge and counts listen registrations and the unsubscribers +// ShareNext is expected to run on instance disposal. +function countingBridgeNode(counts: ListenerCounts) { + return LayerNode.make( + Layer.effect( + EventV2Bridge.Service, + Effect.gen(function* () { + const bridge = Context.get(yield* Layer.build(EventV2Bridge.layer), EventV2Bridge.Service) + const listen: EventV2.Interface["listen"] = (listener) => + Effect.suspend(() => { + counts.listen++ + return bridge.listen(listener).pipe( + Effect.map((unsubscribe) => + Effect.sync(() => { + counts.removed++ + }).pipe(Effect.andThen(unsubscribe)), + ), + ) + }) + return EventV2Bridge.Service.of({ ...bridge, listen }) + }), + ), + [EventV2.node], + ) +} + +function countingLayer(counts: ListenerCounts) { + return LayerNode.buildLayer( + LayerNode.group([ShareNext.node, Session.node, SessionProjector.node, AccountRepo.node, Database.node]), + { + replacements: [LayerNode.replaceWithNode(EventV2Bridge.node, countingBridgeNode(counts))], + }, + ) +} + const share = (id: SessionID) => Effect.gen(function* () { const { db } = yield* Database.Service @@ -294,6 +333,7 @@ describe("ShareNext", () => { expect(seen).toHaveLength(1) expect(seen[0].url).toBe("https://legacy-share.example.com/api/share/shr_abc/sync") + // oxlint-disable-next-line typescript-eslint/no-unsafe-type-assertion -- intentional wire-shape assertion on parsed JSON in a test const body = JSON.parse(seen[0].body) as { secret: string data: Array<{ @@ -325,4 +365,26 @@ describe("ShareNext", () => { { config: { enterprise: { url: "https://legacy-share.example.com" } } }, ), ) + + it.live("unsubscribes instance event listeners on dispose so remounts do not accumulate", () => { + const counts: ListenerCounts = { listen: 0, removed: 0 } + const layers = countingLayer(counts) + return provideTmpdirInstance((directory) => + Effect.gen(function* () { + const store = yield* InstanceStore.Service + + yield* ShareNext.use.init().pipe(Effect.provide(layers)) + expect(counts.listen).toBe(5) + + yield* store.disposeDirectory(directory) + expect(counts.removed).toBe(5) + + yield* ShareNext.use.init().pipe(Effect.provide(layers)) + expect(counts.listen).toBe(10) + + yield* store.disposeDirectory(directory) + expect(counts.removed).toBe(10) + }), + ) + }) }) diff --git a/packages/opencode/test/tool/__snapshots__/parameters.test.ts.snap b/packages/opencode/test/tool/__snapshots__/parameters.test.ts.snap index a01a903e39..9dbf9d6ff3 100644 --- a/packages/opencode/test/tool/__snapshots__/parameters.test.ts.snap +++ b/packages/opencode/test/tool/__snapshots__/parameters.test.ts.snap @@ -464,7 +464,7 @@ exports[`tool parameters JSON Schema (wire shape) workflow 1`] = ` "type": "string", }, "spec_path": { - "description": "(start/extend/control replan/read/validate) An exact saved workflow name returned by workflow(action="list"), or a path to a YAML workflow spec. Graph content belongs in that file; relative paths resolve from the session directory", + "description": "(start/extend/control replan/read/validate) An exact saved workflow name returned by workflow(action="list"), or a path to a YAML workflow spec. The \`builtin://\` path marker list shows for builtin templates also resolves, by name. Graph content belongs in that file; relative paths resolve from the session directory", "type": "string", }, }, @@ -484,7 +484,7 @@ exports[`tool parameters JSON Schema (wire shape) workflow 1`] = ` "type": "string", }, "spec_path": { - "description": "(start/extend/control replan/read/validate) An exact saved workflow name returned by workflow(action="list"), or a path to a YAML workflow spec. Graph content belongs in that file; relative paths resolve from the session directory", + "description": "(start/extend/control replan/read/validate) An exact saved workflow name returned by workflow(action="list"), or a path to a YAML workflow spec. The \`builtin://\` path marker list shows for builtin templates also resolves, by name. Graph content belongs in that file; relative paths resolve from the session directory", "type": "string", }, "workflow_id": { @@ -517,7 +517,7 @@ exports[`tool parameters JSON Schema (wire shape) workflow 1`] = ` "type": "string", }, "spec_path": { - "description": "(start/extend/control replan/read/validate) An exact saved workflow name returned by workflow(action="list"), or a path to a YAML workflow spec. Graph content belongs in that file; relative paths resolve from the session directory", + "description": "(start/extend/control replan/read/validate) An exact saved workflow name returned by workflow(action="list"), or a path to a YAML workflow spec. The \`builtin://\` path marker list shows for builtin templates also resolves, by name. Graph content belongs in that file; relative paths resolve from the session directory", "type": "string", }, "workflow_id": { @@ -649,7 +649,7 @@ exports[`tool parameters JSON Schema (wire shape) workflow 1`] = ` "type": "string", }, "spec_path": { - "description": "(start/extend/control replan/read/validate) An exact saved workflow name returned by workflow(action="list"), or a path to a YAML workflow spec. Graph content belongs in that file; relative paths resolve from the session directory", + "description": "(start/extend/control replan/read/validate) An exact saved workflow name returned by workflow(action="list"), or a path to a YAML workflow spec. The \`builtin://\` path marker list shows for builtin templates also resolves, by name. Graph content belongs in that file; relative paths resolve from the session directory", "type": "string", }, }, @@ -739,7 +739,7 @@ exports[`tool parameters JSON Schema (wire shape) workflow 1`] = ` "type": "boolean", }, "worker_config": { - "description": "{ timeout_ms } — bounds node execution; overrides config.node_defaults.worker_config", + "description": "{ timeout_ms } — bounds the node from admission to completion; queue wait counts toward the budget and an expired queued node fails without spawning. A running node that exceeds it escalates to the parent for adjudication (capped deadline extensions) before failing. Overrides config.node_defaults.worker_config", "properties": { "timeout_ms": { "type": "number", @@ -935,7 +935,7 @@ exports[`tool parameters JSON Schema (wire shape) workflow 1`] = ` "type": "boolean", }, "review": { - "description": "(deep review workers) design reviews pre-implementation artifacts; diff reviews require implementation_node_id and verification_node_id", + "description": "(deep review workers) design reviews pre-implementation artifacts; diff reviews require implementation_node_id, verification_node_id, and authoring-validated wiring: transitive review→verification→implementation dependencies, input_mapping for the diff artifact, fingerprint, and verification output, a PASS-gated condition, and a verdict+implementation_fingerprint output_schema; deep mode additionally requires the diff review to feed a required final gate conditioned on verdict ACCEPT. Violations are authoring errors in deep mode, warnings in standard mode.", "properties": { "implementation_node_id": { "type": "string", @@ -957,7 +957,7 @@ exports[`tool parameters JSON Schema (wire shape) workflow 1`] = ` "type": "object", }, "worker_config": { - "description": "{ timeout_ms } — bounds node execution. Inherits config.node_defaults.worker_config", + "description": "{ timeout_ms } — bounds the node from admission to completion; queue wait counts toward the budget and an expired queued node fails without spawning. A running node that exceeds it escalates to the parent for adjudication (capped deadline extensions) before failing. Inherits config.node_defaults.worker_config", "properties": { "timeout_ms": { "type": "number", @@ -1020,7 +1020,7 @@ exports[`tool parameters JSON Schema (wire shape) workflow 1`] = ` "type": "string", }, "spec_path": { - "description": "(start/extend/control replan/read/validate) An exact saved workflow name returned by workflow(action="list"), or a path to a YAML workflow spec. Graph content belongs in that file; relative paths resolve from the session directory", + "description": "(start/extend/control replan/read/validate) An exact saved workflow name returned by workflow(action="list"), or a path to a YAML workflow spec. The \`builtin://\` path marker list shows for builtin templates also resolves, by name. Graph content belongs in that file; relative paths resolve from the session directory", "type": "string", }, }, diff --git a/packages/opencode/test/tool/task.test.ts b/packages/opencode/test/tool/task.test.ts index 11257220bd..187a0df946 100644 --- a/packages/opencode/test/tool/task.test.ts +++ b/packages/opencode/test/tool/task.test.ts @@ -363,6 +363,30 @@ describe("tool.task", () => { }, ) + it.instance( + "workflow worker_type catalog lists primary build and plan, task catalog does not", + () => + Effect.gen(function* () { + const agent = yield* Agent.Service + const build = yield* agent.get("build") + const registry = yield* ToolRegistry.Service + const tools = yield* registry.tools({ ...ref, agent: build }) + const taskDescription = tools.find((tool) => tool.id === TaskTool.id)?.description ?? "" + const workflowDescription = tools.find((tool) => tool.id === WorkflowTool.id)?.description ?? "" + + // blocks compile coding/prototype to build and plan blocks to plan, so + // the workflow catalog must let the model name them explicitly + expect(workflowDescription).toContain("- build:") + expect(workflowDescription).toContain("- plan:") + expect(workflowDescription).toContain("- general:") + expect(workflowDescription).toContain("- explore:") + // hidden primaries stay hidden and the task catalog stays subagent-only + expect(workflowDescription).not.toContain("- compaction:") + expect(taskDescription).not.toContain("- build:") + expect(taskDescription).not.toContain("- plan:") + }), + ) + it.instance( "description hides denied subagents for the caller", () => diff --git a/packages/opencode/test/util/process.test.ts b/packages/opencode/test/util/process.test.ts index 934833d1d0..90db2c7961 100644 --- a/packages/opencode/test/util/process.test.ts +++ b/packages/opencode/test/util/process.test.ts @@ -126,3 +126,106 @@ describe("util.process", () => { }) }) }) + +describe("util.process stop", () => { + test("fast path: awaits exit when the child honors SIGTERM", async () => { + if (process.platform === "win32") return + + const proc = Process.spawn(node("setInterval(() => {}, 1000)")) + const started = Date.now() + await Process.stop(proc) + await proc.exited + + expect(Date.now() - started).toBeLessThan(1500) + }, 3000) + + test("escalates to SIGKILL when the child ignores SIGTERM", async () => { + if (process.platform === "win32") return + + const proc = Process.spawn( + // Trap BEFORE the ready write: the parent stops as soon as it sees + // "ready", and under CI load the child can be preempted between the two + // statements, taking the first SIGTERM with the default handler still + // installed (exits SIGTERM instead of escalating to SIGKILL). + node('process.on("SIGTERM", () => {});process.stdout.write("ready\\n");setInterval(() => {}, 1000)'), + { stdout: "pipe" }, + ) + await new Promise((resolve) => proc.stdout!.once("data", resolve)) + + const started = Date.now() + await Process.stop(proc) + await proc.exited + + expect(proc.signalCode).toBe("SIGKILL") + expect(Date.now() - started).toBeLessThan(6000) + }, 10000) + + test("is a no-op for an already-exited child", async () => { + const proc = Process.spawn(node("process.exit(0)")) + await proc.exited + + const started = Date.now() + await Process.stop(proc) + + expect(Date.now() - started).toBeLessThan(100) + }) +}) + +describe("util.process stopTree", () => { + test("terminates a spawned process tree", async () => { + if (process.platform === "win32") return + + const pids = await spawnTree("echo $$; sleep 300 & echo $!; sleep 300 & echo $!; wait", 3) + const started = Date.now() + await Process.stopTree(pids) + + for (const pid of pids) expect(treeAlive(pid)).toBe(false) + expect(Date.now() - started).toBeLessThan(2000) + }, 5000) + + test("escalates to SIGKILL when tree members ignore SIGTERM", async () => { + if (process.platform === "win32") return + + const pids = await spawnTree('echo $$; trap "" TERM; sleep 300 & echo $!; wait', 2) + const started = Date.now() + await Process.stopTree(pids, { termGraceMs: 250, killGraceMs: 2000 }) + + for (const pid of pids) expect(treeAlive(pid)).toBe(false) + expect(Date.now() - started).toBeLessThan(3000) + }, 5000) + + test("returns immediately for an empty pid list", async () => { + await Process.stopTree([]) + }) +}) + +function treeAlive(pid: number) { + try { + process.kill(pid, 0) + return true + } catch { + return false + } +} + +// Spawns `sh -c