From d55a9d11cfcf458fb4e36331b9edbf190391912c Mon Sep 17 00:00:00 2001 From: Lex Date: Wed, 2 Sep 2026 11:34:39 +0800 Subject: [PATCH 01/33] chore: record delivery binding for accept-builtin-spec --- .specgit.yaml | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/.specgit.yaml b/.specgit.yaml index bffc28295..752037be1 100644 --- a/.specgit.yaml +++ b/.specgit.yaml @@ -1,9 +1,16 @@ version: 1 -delivery: correct-v1-0 +delivery: accept-builtin-spec context: kind: branch - branch: docs/493-correct-v1-0 + branch: fix/506-accept-builtin-spec issues: - - 493 - - 494 -pr: 495 + - 506 + - 507 + - 508 +issueKinds: + - issue: 506 + kind: kind::fix + - issue: 507 + kind: kind::fix + - issue: 508 + kind: kind::docs From 224a0b73100bd3a4cd5eb6bb57177701003ab922 Mon Sep 17 00:00:00 2001 From: Lex Date: Wed, 2 Sep 2026 11:35:10 +0800 Subject: [PATCH 02/33] chore: record delivery binding for accept-builtin-spec --- .specgit.yaml | 1 + 1 file changed, 1 insertion(+) diff --git a/.specgit.yaml b/.specgit.yaml index 752037be1..8f391bf94 100644 --- a/.specgit.yaml +++ b/.specgit.yaml @@ -14,3 +14,4 @@ issueKinds: kind: kind::fix - issue: 508 kind: kind::docs +pr: 509 From 8b52b71aa4b5ec36c1858f6414a6cb3172882360 Mon Sep 17 00:00:00 2001 From: Lex Date: Wed, 2 Sep 2026 11:36:31 +0800 Subject: [PATCH 03/33] chore(specgit): refresh harness to 1.10.1 and reapply local specializations --- .github/workflows/specgit-accept.yml | 282 +++++++++++++++++++++++---- 1 file changed, 246 insertions(+), 36 deletions(-) diff --git a/.github/workflows/specgit-accept.yml b/.github/workflows/specgit-accept.yml index c280272a4..93d6a6166 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: From e557920d538eafe18bb8b1a1ef322a67bbbe45a4 Mon Sep 17 00:00:00 2001 From: Lex Date: Wed, 2 Sep 2026 11:43:26 +0800 Subject: [PATCH 04/33] fix(dag): accept builtin:// spec_path returned by list resolveSpecPath now strips the synthetic builtin:// marker and resolves by name through the library chain (project shadows global shadows builtin), so the path list advertises round-trips as a spec_path input. Unknown builtin names fail as a library lookup instead of a cwd-joined extension error. --- packages/opencode/src/tool/workflow.ts | 30 +++++++----- .../opencode/test/dag/workflow-tool.test.ts | 46 +++++++++++++++++++ 2 files changed, 64 insertions(+), 12 deletions(-) diff --git a/packages/opencode/src/tool/workflow.ts b/packages/opencode/src/tool/workflow.ts index 0204a1b43..4ea924e9d 100644 --- a/packages/opencode/src/tool/workflow.ts +++ b/packages/opencode/src/tool/workflow.ts @@ -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/test/dag/workflow-tool.test.ts b/packages/opencode/test/dag/workflow-tool.test.ts index 5c564f178..aed9d638a 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* () { From e8da239108a254acfb7b492c0a8ff30e129ee032 Mon Sep 17 00:00:00 2001 From: Lex Date: Wed, 2 Sep 2026 11:43:54 +0800 Subject: [PATCH 05/33] fix(dag): list native primary agents in workflow worker_type catalog Workflow blocks compile coding/prototype to build and plan blocks to plan, but the generated worker_type catalog filtered every primary agent out, so the model could never name the defaults explicitly. The workflow catalog now lists native primaries (build/plan); user-defined primary modes and hidden agents stay excluded and the task catalog is unchanged. --- packages/opencode/src/tool/registry.ts | 7 ++++++- packages/opencode/test/tool/task.test.ts | 24 ++++++++++++++++++++++++ 2 files changed, 30 insertions(+), 1 deletion(-) diff --git a/packages/opencode/src/tool/registry.ts b/packages/opencode/src/tool/registry.ts index aa7b437f2..a2f40d7f0 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/test/tool/task.test.ts b/packages/opencode/test/tool/task.test.ts index 11257220b..187a0df94 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", () => From 284b8fdccfab66c51f89460efca0470bd43f887d Mon Sep 17 00:00:00 2001 From: Lex Date: Wed, 2 Sep 2026 11:44:10 +0800 Subject: [PATCH 06/33] docs(dag): align workflow guides with schema and auto-resume behavior Four guide/schema mismatches from the 2026-09-02 verification: replan and extend auto-resume a paused workflow (manual control(resume) after a successful replan dies on InvalidTransitionError), the exhaustive node field table missed review, the tool reference missed draft/guide/validate, and the name rule missed the leading-dot and control-character exclusions plus the builtin:// marker round-trip. --- .../plugin/command/orchestration-policy.md | 2 +- packages/core/src/plugin/command/workflow.md | 32 ++++++++++++++++--- 2 files changed, 28 insertions(+), 6 deletions(-) diff --git a/packages/core/src/plugin/command/orchestration-policy.md b/packages/core/src/plugin/command/orchestration-policy.md index bf3a6a23f..4c04a1a67 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 ddb6ef955..5082076d2 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. @@ -570,6 +591,7 @@ omitted content from its preview. | `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 also declares `implementation_node_id` and `verification_node_id` | ### What NOT to expect From 8a639fa3af6eff662ef1ce3b8c8c70e4c0cab836 Mon Sep 17 00:00:00 2001 From: Lex Date: Wed, 2 Sep 2026 12:29:27 +0800 Subject: [PATCH 07/33] fix(ci): trigger typecheck and test gates on ready_for_review The SpecGit Acceptance verdict requires required-check runs that started at or after the draft-to-ready transition. Both CI gates only listened to the default pull_request types, so a ready transition never produced fresh runs and the verdict timed out waiting for them. --- .github/workflows/ci-test.yml | 5 +++++ .github/workflows/ci-typecheck.yml | 5 +++++ 2 files changed, 10 insertions(+) diff --git a/.github/workflows/ci-test.yml b/.github/workflows/ci-test.yml index 9484aac7a..d801a2193 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 f8bd2341c..863d90751 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 From 3745d16db87c80b3f6ce2a79c6bdf716bb5eb67b Mon Sep 17 00:00:00 2001 From: Lex Date: Wed, 2 Sep 2026 14:29:11 +0800 Subject: [PATCH 08/33] docs(dag): complete three parameter descriptions with enforced semantics spec_path now mentions the builtin:// marker round-trip; the review field states the input_mapping wiring validateReviewLifecycle enforces for diff reviews; timeout_ms documents that the budget runs from admission (queue wait counts, an expired queued node fails without spawning). --- packages/opencode/src/dag/blocks.ts | 3 ++- packages/opencode/src/dag/validation.ts | 7 +++++-- packages/opencode/src/tool/workflow.ts | 2 +- 3 files changed, 8 insertions(+), 4 deletions(-) diff --git a/packages/opencode/src/dag/blocks.ts b/packages/opencode/src/dag/blocks.ts index aa756a1be..e9a68e7bc 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); 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 46e988903..f95c83e87 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. 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 and verification_node_id, plus input_mapping entries binding the implementation diff/changed_files and fingerprint and the verification output", }), }) diff --git a/packages/opencode/src/tool/workflow.ts b/packages/opencode/src/tool/workflow.ts index 4ea924e9d..e33a6cede 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" }), From 97741244a2e4b9835e7d2cec2448f1edd5a90b74 Mon Sep 17 00:00:00 2001 From: Lex Date: Wed, 2 Sep 2026 14:34:40 +0800 Subject: [PATCH 09/33] docs(dag): state the full diff-review wiring contract and its mode split MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit validateDiffReview enforces seven wiring checks beyond the two node ids (transitive dependency chain, three input_mapping bindings, PASS-gated condition, verdict+fingerprint output_schema), and validateReviewLifecycle only turns them into authoring errors in deep mode — standard mode warns. The previous description understated both. --- packages/core/src/plugin/command/workflow.md | 2 +- packages/opencode/src/dag/validation.ts | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/core/src/plugin/command/workflow.md b/packages/core/src/plugin/command/workflow.md index 5082076d2..7d501d4d7 100644 --- a/packages/core/src/plugin/command/workflow.md +++ b/packages/core/src/plugin/command/workflow.md @@ -591,7 +591,7 @@ omitted content from its preview. | `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 also declares `implementation_node_id` and `verification_node_id` | +| `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/dag/validation.ts b/packages/opencode/src/dag/validation.ts index f95c83e87..ee0b4ad5c 100644 --- a/packages/opencode/src/dag/validation.ts +++ b/packages/opencode/src/dag/validation.ts @@ -233,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, plus input_mapping entries binding the implementation diff/changed_files and fingerprint and the verification output", + "(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 rejects violations; standard mode warns.", }), }) From dd312e8fab93c7c3fe2ce3168d1c59d9f2d3cf6d Mon Sep 17 00:00:00 2001 From: Lex Date: Wed, 2 Sep 2026 14:57:56 +0800 Subject: [PATCH 10/33] docs(dag): close review findings on timeout and review descriptions Regenerate the parameters snapshot after the description changes (the CI unit-test matrix never exercised parameters.test.ts, so the stale snapshot only failed locally). Align blocks.ts with validation.ts (expired-queued-node clause), add the capped-escalation semantics of the deadline watcher and the deep-only final-gate requirement to the timeout/review descriptions, and sync the worker_config guide row. --- packages/core/src/plugin/command/workflow.md | 2 +- packages/opencode/src/dag/blocks.ts | 4 ++-- packages/opencode/src/dag/validation.ts | 4 ++-- .../tool/__snapshots__/parameters.test.ts.snap | 16 ++++++++-------- 4 files changed, 13 insertions(+), 13 deletions(-) diff --git a/packages/core/src/plugin/command/workflow.md b/packages/core/src/plugin/command/workflow.md index 7d501d4d7..f0602ea92 100644 --- a/packages/core/src/plugin/command/workflow.md +++ b/packages/core/src/plugin/command/workflow.md @@ -587,7 +587,7 @@ 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 | diff --git a/packages/opencode/src/dag/blocks.ts b/packages/opencode/src/dag/blocks.ts index e9a68e7bc..859dd1347 100644 --- a/packages/opencode/src/dag/blocks.ts +++ b/packages/opencode/src/dag/blocks.ts @@ -36,8 +36,8 @@ export class WorkflowBlock extends Schema.Class("WorkflowBlock")( timeout_ms: Schema.Number, }), ).annotate({ - description: - "{ timeout_ms } — bounds the node from admission to completion (queue wait counts toward the budget); 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 ee0b4ad5c..195027ee1 100644 --- a/packages/opencode/src/dag/validation.ts +++ b/packages/opencode/src/dag/validation.ts @@ -204,7 +204,7 @@ export const NodeSchema = Schema.Struct({ }), ).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. Inherits config.node_defaults.worker_config", + "{ 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: @@ -233,7 +233,7 @@ export const NodeSchema = Schema.Struct({ }), ).annotate({ 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 rejects violations; standard mode warns.", + "(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/test/tool/__snapshots__/parameters.test.ts.snap b/packages/opencode/test/tool/__snapshots__/parameters.test.ts.snap index a01a903e3..9dbf9d6ff 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", }, }, From 35e78fc132671004def3d33b9957085a5f311da0 Mon Sep 17 00:00:00 2001 From: Lex Date: Wed, 2 Sep 2026 14:58:16 +0800 Subject: [PATCH 11/33] docs(agents): sync specgit harness specializations with the 1.10.1 refresh The replay list still named specgit@^0.5.0 and omitted three specializations the refreshed harness actually carries (45/40-minute timeout split, node 22, hand-parsed policy.yaml), so the next re-init would replay the wrong version and drop them. --- AGENTS.md | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 561824a59..3a6020512 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. From d58037d9082ea36b4a6da8f51379f8877251b0f9 Mon Sep 17 00:00:00 2001 From: Lex Date: Wed, 2 Sep 2026 10:47:46 +0800 Subject: [PATCH 12/33] chore: record delivery binding for hook-command-grandchildren --- .specgit.yaml | 25 +++++++++++++++---------- 1 file changed, 15 insertions(+), 10 deletions(-) diff --git a/.specgit.yaml b/.specgit.yaml index 8f391bf94..fbc397a70 100644 --- a/.specgit.yaml +++ b/.specgit.yaml @@ -1,17 +1,22 @@ version: 1 -delivery: accept-builtin-spec +delivery: hook-command-grandchildren context: kind: branch - branch: fix/506-accept-builtin-spec + branch: fix/500-hook-command-grandchildren issues: - - 506 - - 507 - - 508 + - 500 + - 501 + - 502 + - 503 + - 504 issueKinds: - - issue: 506 + - issue: 500 kind: kind::fix - - issue: 507 + - issue: 501 + kind: kind::fix + - issue: 502 + kind: kind::fix + - issue: 503 + kind: kind::fix + - issue: 504 kind: kind::fix - - issue: 508 - kind: kind::docs -pr: 509 From bf76ff24828396f5540646bff9ca3e7ccc9bae0b Mon Sep 17 00:00:00 2001 From: Lex Date: Wed, 2 Sep 2026 10:48:14 +0800 Subject: [PATCH 13/33] chore: record delivery binding for hook-command-grandchildren --- .specgit.yaml | 1 + 1 file changed, 1 insertion(+) diff --git a/.specgit.yaml b/.specgit.yaml index fbc397a70..4f6bf63f6 100644 --- a/.specgit.yaml +++ b/.specgit.yaml @@ -20,3 +20,4 @@ issueKinds: kind: kind::fix - issue: 504 kind: kind::fix +pr: 505 From 19fcd386888e82343a87acde36a6d7e261f4080a Mon Sep 17 00:00:00 2001 From: Lex Date: Wed, 2 Sep 2026 13:32:53 +0800 Subject: [PATCH 14/33] fix(hook): kill the process group on timeout so pipe-holding grandchildren cannot hang triggers A timed-out command hook only SIGTERM'd the shell wrapper; grandchildren keeping the stdio pipes open meant the close event never fired and the hook trigger hung forever. Exit and stream-drain are now awaited separately with a bounded grace, then the whole group (detached, negative pid on POSIX; taskkill /T /F on Windows) is SIGKILL'd and reaped. --- packages/opencode/src/hook/settings.ts | 93 +++++++++++++++++-- .../test/hook/grandchild-pipe-hang.test.ts | 91 ++++++++++++++++++ 2 files changed, 177 insertions(+), 7 deletions(-) create mode 100644 packages/opencode/test/hook/grandchild-pipe-hang.test.ts diff --git a/packages/opencode/src/hook/settings.ts b/packages/opencode/src/hook/settings.ts index 89dd6c587..cf5d6bc3b 100644 --- a/packages/opencode/src/hook/settings.ts +++ b/packages/opencode/src/hook/settings.ts @@ -1009,6 +1009,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 +1057,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 +1088,89 @@ 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 + if (process.platform === "win32") { + spawn("taskkill", ["/pid", String(child.pid), "/T", "/F"], { stdio: "ignore", windowsHide: true }).on( + "error", + (err) => log.warn("hook taskkill failed", { command, error: err.message }), + ) + return + } + try { + process.kill(-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/test/hook/grandchild-pipe-hang.test.ts b/packages/opencode/test/hook/grandchild-pipe-hang.test.ts new file mode 100644 index 000000000..ca5f31c0d --- /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 }, + ) +}) From cbca6c3ef54e26a90de8c4aef1149d668c0b05df Mon Sep 17 00:00:00 2001 From: Lex Date: Wed, 2 Sep 2026 13:33:10 +0800 Subject: [PATCH 15/33] fix(share): unsubscribe the five instance event listeners on dispose watch() discarded every Unsubscribe, so each instance dispose/remount cycle leaked five permanent EventV2 listeners holding the instance context. The finalizer now unsubscribes before closing the scope. --- packages/opencode/src/share/share-next.ts | 11 +++- .../opencode/test/share/share-next.test.ts | 63 ++++++++++++++++++- 2 files changed, 71 insertions(+), 3 deletions(-) diff --git a/packages/opencode/src/share/share-next.ts b/packages/opencode/src/share/share-next.ts index 4269c6525..172e94d6c 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/test/share/share-next.test.ts b/packages/opencode/test/share/share-next.test.ts index 7a9a2f674..87ea4bd80 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 @@ -325,4 +364,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) + }), + ) + }) }) From 92cc97e447d0667acd14827c439b4239ca7f1924 Mon Sep 17 00:00:00 2001 From: Lex Date: Wed, 2 Sep 2026 13:33:29 +0800 Subject: [PATCH 16/33] fix(tui): clean up route and prompt event subscriptions on unmount Three event.on handlers discarded their unsubscribe functions and accumulated in the app-level SDK handler set on every route transition, retaining the opentui editor and renderer trees. --- packages/tui/src/component/prompt/index.tsx | 24 ++-- packages/tui/src/routes/session/index.tsx | 64 +++++---- .../tui/test/cli/tui/event-cleanup.test.tsx | 134 ++++++++++++++++++ 3 files changed, 181 insertions(+), 41 deletions(-) create mode 100644 packages/tui/test/cli/tui/event-cleanup.test.tsx diff --git a/packages/tui/src/component/prompt/index.tsx b/packages/tui/src/component/prompt/index.tsx index b205a4877..6214598d5 100644 --- a/packages/tui/src/component/prompt/index.tsx +++ b/packages/tui/src/component/prompt/index.tsx @@ -231,18 +231,20 @@ export function Prompt(props: PromptProps) { let promptPartTypeId = 0 const event = useEvent() - event.on("tui.prompt.append", (evt, { workspace }) => { - if (workspace !== project.workspace.current()) return - if (!input || input.isDestroyed) return - input.insertText(evt.properties.text) - setTimeout(() => { - // setTimeout is a workaround and needs to be addressed properly + onCleanup( + event.on("tui.prompt.append", (evt, { workspace }) => { + if (workspace !== project.workspace.current()) return if (!input || input.isDestroyed) return - input.getLayoutNode().markDirty() - input.gotoBufferEnd() - renderer.requestRender() - }, 0) - }) + input.insertText(evt.properties.text) + setTimeout(() => { + // setTimeout is a workaround and needs to be addressed properly + if (!input || input.isDestroyed) return + input.getLayoutNode().markDirty() + input.gotoBufferEnd() + renderer.requestRender() + }, 0) + }), + ) createEffect(() => { if (!input || input.isDestroyed) return diff --git a/packages/tui/src/routes/session/index.tsx b/packages/tui/src/routes/session/index.tsx index 8bd5c25ce..9ec525597 100644 --- a/packages/tui/src/routes/session/index.tsx +++ b/packages/tui/src/routes/session/index.tsx @@ -324,21 +324,23 @@ export function Session() { }) let lastSwitch: string | undefined = undefined - event.on("message.part.updated", (evt) => { - const part = evt.properties.part - if (part.type !== "tool") return - if (part.sessionID !== route.sessionID) return - if (part.state.status !== "completed") return - if (part.id === lastSwitch) return - - if (part.tool === "plan_exit") { - local.agent.set("build") - lastSwitch = part.id - } else if (part.tool === "plan_enter") { - local.agent.set("plan") - lastSwitch = part.id - } - }) + onCleanup( + event.on("message.part.updated", (evt) => { + const part = evt.properties.part + if (part.type !== "tool") return + if (part.sessionID !== route.sessionID) return + if (part.state.status !== "completed") return + if (part.id === lastSwitch) return + + if (part.tool === "plan_exit") { + local.agent.set("build") + lastSwitch = part.id + } else if (part.tool === "plan_enter") { + local.agent.set("plan") + lastSwitch = part.id + } + }), + ) let seeded = false let scroll: ScrollBoxRenderable @@ -354,25 +356,27 @@ export function Session() { const dialog = useDialog() const renderer = useRenderer() - event.on("session.status", (evt) => { - if (evt.properties.sessionID !== route.sessionID) return - if (evt.properties.status.type !== "retry") return - if (!evt.properties.status.action) return - if (dialog.stack.length > 0) return + onCleanup( + event.on("session.status", (evt) => { + if (evt.properties.sessionID !== route.sessionID) return + if (evt.properties.status.type !== "retry") return + if (!evt.properties.status.action) return + if (dialog.stack.length > 0) return - const keys = goUpsellKeys(evt.properties.status.action) - if (!keys) return + const keys = goUpsellKeys(evt.properties.status.action) + if (!keys) return - const seen = kv.get(keys.lastSeenAt) - if (typeof seen === "number" && Date.now() - seen < GO_UPSELL_WINDOW) return + const seen = kv.get(keys.lastSeenAt) + if (typeof seen === "number" && Date.now() - seen < GO_UPSELL_WINDOW) return - if (kv.get(keys.dontShow)) return + if (kv.get(keys.dontShow)) return - void DialogRetryAction.show(dialog, evt.properties.status.action).then((dontShowAgain) => { - if (dontShowAgain) kv.set(keys.dontShow, true) - kv.set(keys.lastSeenAt, Date.now()) - }) - }) + void DialogRetryAction.show(dialog, evt.properties.status.action).then((dontShowAgain) => { + if (dontShowAgain) kv.set(keys.dontShow, true) + kv.set(keys.lastSeenAt, Date.now()) + }) + }), + ) // Helper: Find next visible message boundary in direction const findNextVisibleMessage = (direction: "next" | "prev"): string | null => { diff --git a/packages/tui/test/cli/tui/event-cleanup.test.tsx b/packages/tui/test/cli/tui/event-cleanup.test.tsx new file mode 100644 index 000000000..bb4420d08 --- /dev/null +++ b/packages/tui/test/cli/tui/event-cleanup.test.tsx @@ -0,0 +1,134 @@ +/** @jsxImportSource @opentui/solid */ +import { describe, expect, test } from "bun:test" +import { testRender } from "@opentui/solid" +import type { Event, GlobalEvent } from "@opencode-ai/sdk/v2" +import { createSignal, onCleanup, onMount, Show } from "solid-js" +import { SDKProvider } from "../../../src/context/sdk" +import { useEvent } from "../../../src/context/event" +import { createEventSource, createFetch, directory } from "../../fixture/tui-sdk" +import { TestTuiContexts } from "../../fixture/tui-environment" + +// Route components (routes/session/index.tsx, component/prompt/index.tsx) +// subscribe to app-level events via `onCleanup(event.on(...))` so the +// handler dies with the owning scope. These tests pin that contract at the +// seam it depends on: while the SDKProvider (app lifetime) stays alive, +// unmounting the owning component must remove its handler from the +// app-level emitter, and mount/unmount cycles must not accumulate handlers. + +const sessionID = "ses_route" + +async function wait(fn: () => boolean, timeout = 2000) { + const start = Date.now() + while (!fn()) { + if (Date.now() - start > timeout) throw new Error("timed out waiting for condition") + await Bun.sleep(10) + } +} + +function event(payload: Event): GlobalEvent { + return { directory, payload } +} + +function partUpdated(text: string): Event { + return { + id: `evt_${text}`, + type: "message.part.updated", + properties: { + sessionID, + time: 1, + part: { id: `part_${text}`, sessionID, messageID: "msg_1", type: "text", text }, + }, + } +} + +// Mirrors the production subscription shape: the unsubscribe returned by +// event.on is registered with onCleanup in the component body. +function RouteProbe(props: { received: string[] }) { + const event = useEvent() + onCleanup( + event.on("message.part.updated", (evt) => { + if (evt.properties.part.type !== "text") return + props.received.push(evt.properties.part.text) + }), + ) + return +} + +// Root-level subscription that never unmounts. When it has observed an +// event, the emitter batch has flushed, so any still-registered route +// handler would have observed it in the same pass. +function ControlProbe(props: { received: string[]; onReady: () => void }) { + const event = useEvent() + onCleanup(event.subscribe((evt) => props.received.push(evt.id))) + onMount(() => props.onReady()) + return +} + +async function mount() { + const events = createEventSource() + const calls = createFetch() + const route: string[] = [] + const control: string[] = [] + const [mounted, setMounted] = createSignal(true) + let ready!: () => void + const done = new Promise((resolve) => { + ready = resolve + }) + + const app = await testRender(() => ( + + + + + + + + + )) + + await done + return { + app, + emit: (e: GlobalEvent) => events.emit(e), + route, + control, + unmount: () => setMounted(false), + remount: () => setMounted(true), + } +} + +describe("event.on cleanup", () => { + test("unmounted component stops receiving events while the SDK provider lives", async () => { + const { app, emit, route, control, unmount } = await mount() + + try { + emit(event(partUpdated("before"))) + await wait(() => control.includes("evt_before")) + expect(route).toEqual(["before"]) + + unmount() + emit(event(partUpdated("after"))) + await wait(() => control.includes("evt_after")) + expect(route).toEqual(["before"]) + } finally { + app.renderer.destroy() + } + }) + + test("mount/unmount cycles do not accumulate handlers", async () => { + const { app, emit, route, control, unmount, remount } = await mount() + + try { + unmount() + remount() + unmount() + remount() + + emit(event(partUpdated("single"))) + await wait(() => control.includes("evt_single")) + expect(route).toEqual(["single"]) + } finally { + app.renderer.destroy() + } + }) +}) From dd70af13a80fc9af3e4d6499fa713744392917b1 Mon Sep 17 00:00:00 2001 From: Lex Date: Wed, 2 Sep 2026 13:33:57 +0800 Subject: [PATCH 17/33] fix(process): await exit with SIGKILL escalation in stop and MCP client shutdown stop() on POSIX returned right after SIGTERM, so servers ignoring it stayed alive as orphans while instance finalizers reported success. It now waits a bounded grace, escalates to SIGKILL, and awaits exit; the SDK copy adapts the same escalation synchronously with an unref'd timer. MCP client shutdown reaps the whole process tree through a shared shutdownClient used by the state finalizer, closeClient, and the create rollback path. --- packages/opencode/src/mcp/index.ts | 41 ++++---- packages/opencode/src/util/process.ts | 73 ++++++++++++++ .../test/mcp/fixtures/process-tree-probe.ts | 84 ++++++++++++++++ .../test/mcp/fixtures/server-stdio.ts | 12 +++ .../opencode/test/mcp/process-tree.test.ts | 27 +++++ packages/opencode/test/util/process.test.ts | 99 +++++++++++++++++++ packages/sdk/js/src/process.ts | 14 ++- 7 files changed, 329 insertions(+), 21 deletions(-) create mode 100644 packages/opencode/test/mcp/fixtures/process-tree-probe.ts create mode 100644 packages/opencode/test/mcp/process-tree.test.ts diff --git a/packages/opencode/src/mcp/index.ts b/packages/opencode/src/mcp/index.ts index 2bbdcf95c..5b37cf825 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/util/process.ts b/packages/opencode/src/util/process.ts index 173210f23..b24538f7d 100644 --- a/packages/opencode/src/util/process.ts +++ b/packages/opencode/src/util/process.ts @@ -144,6 +144,12 @@ 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 + // 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,6 +157,9 @@ 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 } @@ -162,6 +171,70 @@ export async function stop(proc: ChildProcess) { 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() + }) +} + +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 { const out = await run(cmd, opts) return { 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 000000000..9806e2a5f --- /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 b54938541..73d1b9370 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 000000000..651070a07 --- /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.toString()).toBe(0) + const jsonLine = stdout.toString().trimEnd().split("\n").pop() + expect(JSON.parse(jsonLine ?? "")).toMatchObject({ ok: true, rootDead: true, childDead: true }) +}) diff --git a/packages/opencode/test/util/process.test.ts b/packages/opencode/test/util/process.test.ts index 934833d1d..e4b37f314 100644 --- a/packages/opencode/test/util/process.test.ts +++ b/packages/opencode/test/util/process.test.ts @@ -126,3 +126,102 @@ 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( + node('process.stdout.write("ready\\n");process.on("SIGTERM", () => {});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