From 1723806e16d804df49a0478275fbad7edb3959ab Mon Sep 17 00:00:00 2001 From: Andrey Epifancev Date: Sun, 25 Oct 2026 10:58:31 +0400 Subject: [PATCH 1/2] =?UTF-8?q?feat(dsh):=20request=5Fapproval-=D0=B8?= =?UTF-8?q?=D0=BD=D1=81=D1=82=D1=80=D1=83=D0=BC=D0=B5=D0=BD=D1=82=20?= =?UTF-8?q?=E2=80=94=20=D0=BC=D0=BE=D0=B4=D0=B5=D0=BB=D1=8C=20=D0=B8=D0=BD?= =?UTF-8?q?=D0=B8=D1=86=D0=B8=D0=B8=D1=80=D1=83=D0=B5=D1=82=20=D0=B0=D0=BF?= =?UTF-8?q?=D0=BF=D1=80=D1=83=D0=B2,=20=D0=B2=D0=BB=D0=B0=D0=B4=D0=B5?= =?UTF-8?q?=D0=BB=D0=B5=D1=86=20=D0=B6=D0=BC=D1=91=D1=82=20Approve=20(#77)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- dsh/bundle/command-approve.js | 239 +++++++++------------------------- dsh/bundle/index.js | 18 +++ dsh/bundle/lib/approval.js | 187 ++++++++++++++++++++++++++ dsh/bundle/package.json | 1 + dsh/bundle/tool-approve.js | 144 ++++++++++++++++++++ dsh/tests/approve.test.mjs | 216 ++++++++++++++++++++++++++++++ dsh/tests/package.test.mjs | 25 ++++ dsh/tests/run.sh | 2 +- 8 files changed, 652 insertions(+), 180 deletions(-) create mode 100644 dsh/bundle/lib/approval.js create mode 100644 dsh/bundle/tool-approve.js diff --git a/dsh/bundle/command-approve.js b/dsh/bundle/command-approve.js index bfece3a..5ce1ad4 100644 --- a/dsh/bundle/command-approve.js +++ b/dsh/bundle/command-approve.js @@ -1,11 +1,13 @@ /** * Human-facing `/approve` command for DeepSeek Harness. * - * `/approve design|plan` is the only writer of - * `openspec/changes//approval.md` (D-1). The command checks the upstream - * artifacts, asks the owner through `userQuestions.ask` (D-12), and only then - * merges the new section into the existing file. A missing user-questions - * service refuses instead of writing, so no model tool can approve itself. + * `/approve design|plan` is the manual front-end of the approval + * gate: it asks the owner through `userQuestions.ask` (D-12) and lets the + * shared `runApproval` (lib/approval.js) check the upstream artifacts and + * write `openspec/changes//approval.md` (D-1). The write itself lives in + * the shared module, which the model tool `request_approval` uses as well — + * both only write after the owner confirms. A missing user-questions service + * refuses instead of writing, so no model tool can approve itself. * * The repository root is resolved per invocation from the receiving agent's * session (`workingDirectory.get(agent.session)`, T3.1) — the registration @@ -16,19 +18,8 @@ * `{ kind, text }` result shape. */ -import { mkdirSync, readFileSync, readdirSync, statSync, writeFileSync } from 'node:fs' -import { dirname, join, relative, resolve } from 'node:path' -import { - APPROVAL_FILE, - CHANGE_DIR, - PLANS_DIR, - extractSpec, - hasDesignApproval, - hashPlan, - parseApproval, - resolveChangeFromSpec, - serializeApproval, -} from './lib/gates.js' +import { resolve } from 'node:path' +import { CHANGE_ID, runApproval } from './lib/approval.js' const USAGE = [ 'Usage: /approve design|plan', @@ -36,27 +27,8 @@ const USAGE = [ ' plan — record the owner approval of the plan whose **Spec:** is ', ].join('\n') -const DESIGN_ARTIFACTS = ['proposal.md', 'design.md', 'builder-check.md'] const APPROVE_LABEL = 'Approve' const CANCEL_LABEL = 'Cancel' -/** Change ids are directory names: keep them one safe path segment. */ -const CHANGE_ID = /^[A-Za-z0-9][A-Za-z0-9._-]*$/u - -function isFile(path) { - try { - return statSync(path).isFile() - } catch { - return false - } -} - -function readText(path) { - try { - return readFileSync(path, 'utf8') - } catch { - return null - } -} /** Session working directory of the invoking agent; process.cwd() fallback. */ function currentDirectory(workingDirectory, agent) { @@ -99,148 +71,51 @@ function isApproved(answer) { } /** - * Ask the owner and return `{ ok: true }` or a refusal `{ ok: false, text }`. - * The detail shows exactly what is being approved; the answer is never - * inferred from the absence of an error. + * A refusal produced while asking the owner (missing service, failed dialog). + * The handler turns it into the same `Refused: ...` result as `runApproval` + * refusals; any other exception is reported as an `/approve failure`. */ -async function askOwner(userQuestions, { changeId, kind, detail, agent, signal }) { - if (typeof userQuestions?.ask !== 'function') { - return { ok: false, text: 'userQuestions service unavailable; /approve requires it' } - } +class ConfirmationRefusal extends Error {} - const request = { - questions: [ - { - id: `approve-${kind}`, - header: 'Approve', - question: `Approve ${changeId} ${kind}?`, - detail, - options: [ - { - label: APPROVE_LABEL, - description: `Write the ${kind} approval to openspec/changes/${changeId}/approval.md.`, - }, - { label: CANCEL_LABEL, description: 'Write nothing.' }, - ], - }, - ], - } - if (agent !== undefined) request.agent = agent - if (signal !== undefined) request.signal = signal +/** + * The `ask` implementation of this front-end: one Approve/Cancel dialog whose + * detail shows exactly what is being approved. Returns `true` only for an + * explicit Approve answer; a missing service or a failed ask throws a + * `ConfirmationRefusal` carrying the reason, so nothing is written. + */ +function ownerAsker({ userQuestions, changeId, kind, agent, signal }) { + return async (questionText, detail) => { + if (typeof userQuestions?.ask !== 'function') { + throw new ConfirmationRefusal('userQuestions service unavailable; /approve requires it') + } - let answer - try { - answer = await userQuestions.ask(request) - } catch (error) { - return { ok: false, text: `userQuestions failed: ${String(error?.message ?? error)}` } - } + const request = { + questions: [ + { + id: `approve-${kind}`, + header: 'Approve', + question: questionText, + detail, + options: [ + { + label: APPROVE_LABEL, + description: `Write the ${kind} approval to openspec/changes/${changeId}/approval.md.`, + }, + { label: CANCEL_LABEL, description: 'Write nothing.' }, + ], + }, + ], + } + if (agent !== undefined) request.agent = agent + if (signal !== undefined) request.signal = signal - return isApproved(answer) ? { ok: true } : { ok: false, text: 'not confirmed' } -} - -/** Merge one section into approval.md, leaving every other section as is. */ -function writeApproval(root, changeId, mutate) { - const path = join(root, CHANGE_DIR, changeId, APPROVAL_FILE) - const next = mutate(parseApproval(readText(path))) - mkdirSync(dirname(path), { recursive: true }) - writeFileSync(path, serializeApproval(next), 'utf8') - return path -} - -/** Existing plans under docs/superpowers/plans/ whose **Spec:** is ``. */ -function plansForChange(root, changeId) { - const plansDir = join(root, PLANS_DIR) - let names - try { - names = readdirSync(plansDir) - } catch { - return [] - } - - const found = [] - for (const name of [...names].sort()) { - if (!name.endsWith('.md')) continue - const path = join(plansDir, name) - if (!isFile(path)) continue - const content = readText(path) - if (content === null) continue - if (resolveChangeFromSpec(extractSpec(content)) === changeId) found.push(path) - } - return found -} - -function approvalText(root, path) { - return relative(root, path).split('\\').join('/') -} - -async function approveDesign({ root, changeId, userQuestions, agent, signal }) { - const changeDir = join(root, CHANGE_DIR, changeId) - const missing = DESIGN_ARTIFACTS.filter((name) => !isFile(join(changeDir, name))) - if (missing.length > 0) { - const listed = missing.map((name) => `openspec/changes/${changeId}/${name}`).join(', ') - return refusal(`design approval requires the change artifacts; missing: ${listed}.`) - } - - const asked = await askOwner(userQuestions, { - changeId, - kind: 'design', - detail: `Change: openspec/changes/${changeId} (proposal.md, design.md, builder-check.md).`, - agent, - signal, - }) - if (!asked.ok) return refusal(asked.text) - - const at = new Date().toISOString() - const path = writeApproval(root, changeId, (approval) => ({ - ...approval, - design: { by: 'owner', at }, - })) - return { - kind: 'success', - text: `Approved ${changeId} design at ${at}; wrote ${approvalText(root, path)}.`, - } -} - -async function approvePlan({ root, changeId, userQuestions, agent, signal }) { - const approval = parseApproval(readText(join(root, CHANGE_DIR, changeId, APPROVAL_FILE))) - if (!hasDesignApproval(approval)) { - return refusal(`design not approved; run /approve ${changeId} design`) - } - - const plans = plansForChange(root, changeId) - if (plans.length === 0) { - return refusal( - `no plan for ${changeId} found under ${PLANS_DIR.split('\\').join('/')}/ with **Spec:** openspec/changes/${changeId}.`, - ) - } - if (plans.length > 1) { - const listed = plans.map((path) => approvalText(root, path)).join(', ') - return refusal(`several plans reference ${changeId}: ${listed}. Keep exactly one.`) - } - - const [planPath] = plans - const sha256 = hashPlan(planPath) - if (sha256 === '') { - return refusal(`cannot read plan ${approvalText(root, planPath)}.`) - } - - const asked = await askOwner(userQuestions, { - changeId, - kind: 'plan', - detail: `Plan: ${approvalText(root, planPath)} (sha256 ${sha256}).`, - agent, - signal, - }) - if (!asked.ok) return refusal(asked.text) - - const at = new Date().toISOString() - const path = writeApproval(root, changeId, (current) => ({ - ...current, - plan: { by: 'owner', at, sha256 }, - })) - return { - kind: 'success', - text: `Approved ${changeId} plan at ${at} (sha256 ${sha256}); wrote ${approvalText(root, path)}.`, + let answer + try { + answer = await userQuestions.ask(request) + } catch (error) { + throw new ConfirmationRefusal(`userQuestions failed: ${String(error?.message ?? error)}`) + } + return isApproved(answer) } } @@ -267,11 +142,17 @@ export function registerApproveCommand({ commands, userQuestions, workingDirecto const root = resolve(currentDirectory(workingDirectory, agent)) try { - if (kind === 'design') { - return await approveDesign({ root, changeId, userQuestions, agent, signal }) - } - return await approvePlan({ root, changeId, userQuestions, agent, signal }) + const result = await runApproval({ + repoRoot: root, + changeId, + kind, + ask: ownerAsker({ userQuestions, changeId, kind, agent, signal }), + }) + return result.approved + ? { kind: 'success', text: result.message } + : refusal(result.message) } catch (error) { + if (error instanceof ConfirmationRefusal) return refusal(error.message) logger?.warn?.('[dsh-harness] /approve failed: %s', error?.message ?? error) return refusal(`/approve failed: ${String(error?.message ?? error)}`) } diff --git a/dsh/bundle/index.js b/dsh/bundle/index.js index 761578e..72d0690 100644 --- a/dsh/bundle/index.js +++ b/dsh/bundle/index.js @@ -25,6 +25,7 @@ import { fileURLToPath } from 'node:url' import { registerRoleCommand } from './command-role.js' import { registerCheckCommand } from './command-check.js' import { registerApproveCommand } from './command-approve.js' +import { createApproveTool } from './tool-approve.js' import { createGuard } from './lib/guards.js' import { checkApprovalWrite, @@ -322,6 +323,23 @@ export function apply(ctx, rawConfig = {}) { }) } + // `request_approval` is the model-driven front-end of the same gate: the + // model initiates the approval, the owner only confirms in the dialog, and + // the marker is still written by lib/approval.js. `userQuestions` is + // resolved lazily, never injected — a missing service must refuse this tool + // instead of parking the whole bundle. + if (config.enabled && config.approvalGates && typeof ctx.tools?.register === 'function') { + ctx.tools.register( + createApproveTool({ + getWorkingDirectory: () => ctx.workingDirectory, + getUserQuestions: () => (typeof ctx.get === 'function' ? (ctx.get('userQuestions') ?? null) : null), + logger: ctx.logger, + now: () => new Date(), + }), + ) + ctx.logger?.info?.('[dsh-harness] request_approval tool registered') + } + ctx.logger?.info?.( '[dsh-harness] ready: hooks=%s, bootstrap=%s, /role=%s', config.guards ? hooksDir : 'off', diff --git a/dsh/bundle/lib/approval.js b/dsh/bundle/lib/approval.js new file mode 100644 index 0000000..3980a4e --- /dev/null +++ b/dsh/bundle/lib/approval.js @@ -0,0 +1,187 @@ +/** + * Shared approval logic of the two front-ends: the `/approve` command and the + * `request_approval` model tool. + * + * `runApproval` owns everything except the question itself: the upstream + * artifact checks (design needs proposal/design/builder-check, plan needs an + * approved design and exactly one plan whose **Spec:** names the change) and + * the section write (`parseApproval` -> merge -> `serializeApproval`). The + * confirmation is injected as `ask(questionText, detail) -> Promise`, + * so no model-facing caller can write `approval.md` without the owner: the + * command asks through `userQuestions.ask`, the tool does the same on behalf + * of the model, and this module never writes without a `true` answer. + * + * `now` is injected for deterministic tests; it returns a Date, so the written + * `at` is a UTC ISO timestamp exactly as before the extraction. + * + * No DSH runtime is imported here (only Node builtins and `./gates.js`). + */ + +import { mkdirSync, readFileSync, readdirSync, statSync, writeFileSync } from 'node:fs' +import { dirname, join, relative, resolve } from 'node:path' +import { + APPROVAL_FILE, + CHANGE_DIR, + PLANS_DIR, + extractSpec, + hasDesignApproval, + hashPlan, + parseApproval, + resolveChangeFromSpec, + serializeApproval, +} from './gates.js' + +/** Design approval requires all three change artifacts, as before. */ +export const DESIGN_ARTIFACTS = ['proposal.md', 'design.md', 'builder-check.md'] + +/** Change ids are directory names: keep them one safe path segment. */ +export const CHANGE_ID = /^[A-Za-z0-9][A-Za-z0-9._-]*$/u + +export const KINDS = ['design', 'plan'] + +function isFile(path) { + try { + return statSync(path).isFile() + } catch { + return false + } +} + +function readText(path) { + try { + return readFileSync(path, 'utf8') + } catch { + return null + } +} + +function refusal(text) { + return { approved: false, message: text } +} + +/** Merge one section into approval.md, leaving every other section as is. */ +function writeApproval(root, changeId, mutate) { + const path = join(root, CHANGE_DIR, changeId, APPROVAL_FILE) + const next = mutate(parseApproval(readText(path))) + mkdirSync(dirname(path), { recursive: true }) + writeFileSync(path, serializeApproval(next), 'utf8') + return path +} + +/** Existing plans under docs/superpowers/plans/ whose **Spec:** is ``. */ +function plansForChange(root, changeId) { + const plansDir = join(root, PLANS_DIR) + let names + try { + names = readdirSync(plansDir) + } catch { + return [] + } + + const found = [] + for (const name of [...names].sort()) { + if (!name.endsWith('.md')) continue + const path = join(plansDir, name) + if (!isFile(path)) continue + const content = readText(path) + if (content === null) continue + if (resolveChangeFromSpec(extractSpec(content)) === changeId) found.push(path) + } + return found +} + +function approvalText(root, path) { + return relative(root, path).split('\\').join('/') +} + +/** Ask the injected confirmation; anything but `true` is not an approval. */ +async function confirmed(ask, questionText, detail) { + const answer = await ask(questionText, detail) + return answer === true +} + +async function approveDesign({ root, changeId, ask, now }) { + const changeDir = join(root, CHANGE_DIR, changeId) + const missing = DESIGN_ARTIFACTS.filter((name) => !isFile(join(changeDir, name))) + if (missing.length > 0) { + const listed = missing.map((name) => `openspec/changes/${changeId}/${name}`).join(', ') + return refusal(`design approval requires the change artifacts; missing: ${listed}.`) + } + + const detail = `Change: openspec/changes/${changeId} (proposal.md, design.md, builder-check.md).` + if (!(await confirmed(ask, `Approve ${changeId} design?`, detail))) { + return refusal('not confirmed') + } + + const at = new Date(now()).toISOString() + const path = writeApproval(root, changeId, (approval) => ({ + ...approval, + design: { by: 'owner', at }, + })) + return { + approved: true, + message: `Approved ${changeId} design at ${at}; wrote ${approvalText(root, path)}.`, + } +} + +async function approvePlan({ root, changeId, ask, now }) { + const approval = parseApproval(readText(join(root, CHANGE_DIR, changeId, APPROVAL_FILE))) + if (!hasDesignApproval(approval)) { + return refusal(`design not approved; run /approve ${changeId} design`) + } + + const plans = plansForChange(root, changeId) + if (plans.length === 0) { + return refusal( + `no plan for ${changeId} found under ${PLANS_DIR.split('\\').join('/')}/ with **Spec:** openspec/changes/${changeId}.`, + ) + } + if (plans.length > 1) { + const listed = plans.map((path) => approvalText(root, path)).join(', ') + return refusal(`several plans reference ${changeId}: ${listed}. Keep exactly one.`) + } + + const [planPath] = plans + const sha256 = hashPlan(planPath) + if (sha256 === '') { + return refusal(`cannot read plan ${approvalText(root, planPath)}.`) + } + + const detail = `Plan: ${approvalText(root, planPath)} (sha256 ${sha256}).` + if (!(await confirmed(ask, `Approve ${changeId} plan?`, detail))) { + return refusal('not confirmed') + } + + const at = new Date(now()).toISOString() + const path = writeApproval(root, changeId, (current) => ({ + ...current, + plan: { by: 'owner', at, sha256 }, + })) + return { + approved: true, + message: `Approved ${changeId} plan at ${at} (sha256 ${sha256}); wrote ${approvalText(root, path)}.`, + } +} + +/** + * Approve ` ` and write the marker. + * + * Returns `{ approved, message }`; every refusal is `approved: false` with a + * human-readable reason. The change id and kind are re-validated here (not + * only in the front-ends): the id becomes a path segment. + */ +export async function runApproval({ repoRoot, changeId, kind, ask, now = () => new Date() }) { + if (typeof changeId !== 'string' || !CHANGE_ID.test(changeId)) { + return refusal(`Invalid change id "${String(changeId)}".`) + } + if (!KINDS.includes(kind)) { + return refusal(`Unknown kind "${String(kind)}"; expected design or plan.`) + } + if (typeof ask !== 'function') { + return refusal('no confirmation function; the approval requires the owner') + } + + const root = resolve(repoRoot) + if (kind === 'design') return approveDesign({ root, changeId, ask, now }) + return approvePlan({ root, changeId, ask, now }) +} diff --git a/dsh/bundle/package.json b/dsh/bundle/package.json index 2a4d524..6a28583 100644 --- a/dsh/bundle/package.json +++ b/dsh/bundle/package.json @@ -14,6 +14,7 @@ "command-role.js", "command-check.js", "command-approve.js", + "tool-approve.js", "lib", "dsh-tools.md", "cordis.patch.yml", diff --git a/dsh/bundle/tool-approve.js b/dsh/bundle/tool-approve.js new file mode 100644 index 0000000..df6db5d --- /dev/null +++ b/dsh/bundle/tool-approve.js @@ -0,0 +1,144 @@ +/** + * `request_approval` — the model-driven front-end of the approval gate. + * + * The owner no longer has to type `/approve design|plan`: the model + * calls this tool, the owner only confirms in the userQuestions dialog, and the + * approval marker is still written by bundle code (`lib/approval.js`), never by + * the model. The gates keep their invariant: `approval.md` is written only by + * the harness after an explicit owner confirmation. + * + * The definition is hand-built plain JSON — no `@deepseek-ai/*` imports (the + * bundle is installed from a local directory, so DSH internals are not + * resolvable). `parameters` is object-rooted raw JSON Schema; DSH performs no + * argument validation, so `execute` validates the args itself and always + * returns the lossless `{ approved, message }` shape. + * + * `userQuestions` is resolved lazily (injected as `getUserQuestions`) and never + * declared in `inject`: a missing service must refuse this tool, not park the + * whole bundle. Likewise `getWorkingDirectory` is a service factory, because + * the repo root depends on the receiving agent's session. + */ + +import { resolve } from 'node:path' +import { CHANGE_ID, KINDS, runApproval } from './lib/approval.js' + +const APPROVE_LABEL = 'Approve' +const CANCEL_LABEL = 'Cancel' +const USAGE = + "usage: request_approval { change_id: '', kind: 'design' | 'plan' }" + +/** Session working directory of the receiving agent; process.cwd() fallback. */ +function repoRootOf(getWorkingDirectory, agent) { + try { + const workingDirectory = getWorkingDirectory?.() + const cwd = workingDirectory?.get?.(agent?.session) + if (typeof cwd === 'string' && cwd !== '') return resolve(cwd) + } catch { + // Fall through to the process directory. + } + return process.cwd() +} + +function refusal(message) { + return { approved: false, message } +} + +/** + * Build the tool definition. + * + * @param {{ + * getWorkingDirectory?: () => { get(session: unknown): string } | undefined, + * getUserQuestions?: () => { ask(request: unknown): Promise } | null, + * logger?: { warn?: Function }, + * now?: () => Date, + * }} deps + */ +export function createApproveTool({ getWorkingDirectory, getUserQuestions, logger, now = () => new Date() } = {}) { + return { + name: 'request_approval', + description: + 'Ask the owner to approve an OpenSpec change design or an implementation plan. The approval marker is written by the harness only after the owner confirms.', + parameters: { + type: 'object', + properties: { + change_id: { + type: 'string', + description: 'Change id under openspec/changes//', + }, + kind: { + type: 'string', + enum: ['design', 'plan'], + description: 'What is being approved', + }, + }, + required: ['change_id', 'kind'], + additionalProperties: false, + }, + output: { + schema: { + type: 'object', + properties: { + approved: { type: 'boolean' }, + message: { type: 'string' }, + }, + required: ['approved', 'message'], + additionalProperties: false, + }, + render(args, value) { + return [{ type: 'text', text: value.message }] + }, + }, + async execute(args, exec = {}) { + try { + const changeId = typeof args?.change_id === 'string' ? args.change_id.trim() : '' + const kind = args?.kind + + if (changeId === '') { + return refusal(`Missing change_id. ${USAGE}`) + } + if (!CHANGE_ID.test(changeId)) { + return refusal(`Invalid change id "${changeId}". ${USAGE}`) + } + if (!KINDS.includes(kind)) { + return refusal(`Unknown kind "${String(kind)}". ${USAGE}`) + } + + const userQuestions = getUserQuestions?.() ?? null + if (typeof userQuestions?.ask !== 'function') { + return refusal( + 'userQuestions service unavailable; request_approval requires the owner dialog (use /approve).', + ) + } + + // The ask implementation of this front-end: mirrors the /approve + // dialog, but a `selected[0] === 'Approve'` answer is the only yes. + const ask = async (questionText) => { + const answer = await userQuestions.ask({ + questions: [ + { + id: 'approve', + question: questionText, + header: 'Approval gate', + options: [{ label: APPROVE_LABEL }, { label: CANCEL_LABEL }], + }, + ], + ...(exec.agent ? { agent: exec.agent } : {}), + signal: exec.signal, + }) + return answer?.answers?.[0]?.selected?.[0] === APPROVE_LABEL + } + + return await runApproval({ + repoRoot: repoRootOf(getWorkingDirectory, exec.agent), + changeId, + kind, + ask, + now, + }) + } catch (error) { + logger?.warn?.('[dsh-harness] request_approval failed: %s', error?.message ?? error) + return refusal(String(error?.message ?? error)) + } + }, + } +} diff --git a/dsh/tests/approve.test.mjs b/dsh/tests/approve.test.mjs index 8bf52c2..0976d2c 100644 --- a/dsh/tests/approve.test.mjs +++ b/dsh/tests/approve.test.mjs @@ -5,6 +5,7 @@ import { join } from 'node:path' import test from 'node:test' import { registerApproveCommand } from '../bundle/command-approve.js' import { hashPlan, parseApproval, serializeApproval } from '../bundle/lib/gates.js' +import { createApproveTool } from '../bundle/tool-approve.js' import { cleanup, tempDir, write } from './helpers.mjs' const PLAN_TEXT = [ @@ -347,3 +348,218 @@ test('approve falls back to process.cwd() when the session cwd lookup throws (T3 cleanup(tmp) } }) + +// `request_approval` (bounded #77): the model drives the approval gate, the +// owner only confirms in the userQuestions dialog, and the marker is still +// written by bundle code through the same lib/approval.js as `/approve`. + +const FIXED_ISO = '2026-10-25T12:00:00.000Z' + +/** + * Build the tool with fakes: `getWorkingDirectory` is the DSH service factory + * (`workingDirectory.get(session)`), `getUserQuestions` resolves lazily, and + * `now` is pinned so the written `at` is exact. + */ +function setupTool(repo, { answer, missingService = false, at = FIXED_ISO } = {}) { + const calls = [] + const getUserQuestions = missingService + ? () => null + : () => ({ + async ask(request) { + calls.push(request) + return answer + }, + }) + const tool = createApproveTool({ + getWorkingDirectory: () => ({ get() { return repo } }), + getUserQuestions, + logger: { warn() {} }, + now: () => new Date(at), + }) + return { tool, calls, run: (args, exec = {}) => tool.execute(args, exec) } +} + +test('request_approval design: owner confirms, section is written by the harness', async () => { + const { tmp, repo } = repoFixture() + try { + const harness = setupTool(repo, { answer: approveAnswer('design') }) + assert.equal(harness.tool.name, 'request_approval') + assert.match(harness.tool.description, /written by the harness only after the owner confirms/u) + assert.deepEqual(harness.tool.parameters.required, ['change_id', 'kind']) + assert.deepEqual(harness.tool.parameters.properties.kind.enum, ['design', 'plan']) + assert.deepEqual(harness.tool.output.render({}, { approved: true, message: 'ok' }), [ + { type: 'text', text: 'ok' }, + ]) + + const result = await harness.run({ change_id: 'demo', kind: 'design' }, { agent: { session: 's1' } }) + + assert.equal(result.approved, true) + assert.equal(result.message, `Approved demo design at ${FIXED_ISO}; wrote openspec/changes/demo/approval.md.`) + + const question = harness.calls[0].questions[0] + assert.equal(question.question, 'Approve demo design?') + assert.equal(question.header, 'Approval gate') + assert.deepEqual(question.options.map((option) => option.label), ['Approve', 'Cancel']) + assert.equal(harness.calls[0].agent.session, 's1', 'the receiving agent is forwarded') + + const parsed = parseApproval(readFileSync(approvalPath(repo), 'utf8')) + assert.equal(parsed.design.by, 'owner') + assert.equal(parsed.design.at, FIXED_ISO) + } finally { + cleanup(tmp) + } +}) + +test('request_approval design: cancel writes nothing', async () => { + const { tmp, repo } = repoFixture() + try { + const harness = setupTool(repo, { + answer: { answers: [{ id: 'approve', selected: ['Cancel'], custom: undefined }] }, + }) + const result = await harness.run({ change_id: 'demo', kind: 'design' }) + + assert.equal(result.approved, false) + assert.match(result.message, /not confirmed/u) + assert.equal(existsSync(approvalPath(repo)), false, 'no approval.md is written') + } finally { + cleanup(tmp) + } +}) + +test('request_approval refuses when the userQuestions service is missing', async () => { + const { tmp, repo } = repoFixture() + try { + const harness = setupTool(repo, { missingService: true }) + const result = await harness.run({ change_id: 'demo', kind: 'design' }) + + assert.equal(result.approved, false) + assert.match(result.message, /userQuestions service unavailable/u) + assert.equal(existsSync(approvalPath(repo)), false, 'no approval.md is written') + } finally { + cleanup(tmp) + } +}) + +test('request_approval refuses an unknown kind with usage', async () => { + const { tmp, repo } = repoFixture() + try { + const harness = setupTool(repo, { answer: approveAnswer('design') }) + const result = await harness.run({ change_id: 'demo', kind: 'review' }) + + assert.equal(result.approved, false) + assert.match(result.message, /usage: request_approval/u) + assert.equal(harness.calls.length, 0, 'the owner is not asked') + assert.equal(existsSync(approvalPath(repo)), false, 'no approval.md is written') + } finally { + cleanup(tmp) + } +}) + +test('request_approval refuses an unsafe change id', async () => { + const { tmp, repo } = repoFixture() + try { + const harness = setupTool(repo, { answer: approveAnswer('design') }) + const result = await harness.run({ change_id: '../x', kind: 'design' }) + + assert.equal(result.approved, false) + assert.match(result.message, /Invalid change id "\.\.\/x"/u) + assert.equal(harness.calls.length, 0, 'the owner is not asked') + assert.equal(existsSync(approvalPath(repo)), false, 'no approval.md is written') + } finally { + cleanup(tmp) + } +}) + +test('request_approval plan writes the sha256 of the plan', async () => { + const { tmp, repo, planPath } = repoFixture() + try { + const design = { by: 'aep', at: '2026-10-25T10:00:00+04:00' } + write(approvalPath(repo), serializeApproval({ design })) + + const harness = setupTool(repo, { answer: approveAnswer('plan') }) + const result = await harness.run({ change_id: 'demo', kind: 'plan' }) + + assert.equal(result.approved, true) + assert.equal( + result.message, + `Approved demo plan at ${FIXED_ISO} (sha256 ${shaOf(planPath)}); wrote openspec/changes/demo/approval.md.`, + ) + + const parsed = parseApproval(readFileSync(approvalPath(repo), 'utf8')) + assert.equal(parsed.plan.by, 'owner') + assert.equal(parsed.plan.at, FIXED_ISO) + assert.equal(parsed.plan.sha256, shaOf(planPath)) + assert.deepEqual(parsed.design, design, 'the design section is untouched') + } finally { + cleanup(tmp) + } +}) + +test('request_approval design refuses without builder-check.md (prerequisites hold)', async () => { + const { tmp, repo } = repoFixture({ builderCheck: false }) + try { + const harness = setupTool(repo, { answer: approveAnswer('design') }) + const result = await harness.run({ change_id: 'demo', kind: 'design' }) + + assert.equal(result.approved, false) + assert.match(result.message, /builder-check\.md/u) + assert.equal(harness.calls.length, 0, 'the owner is not asked before the prerequisites pass') + assert.equal(existsSync(approvalPath(repo)), false, 'no approval.md is written') + } finally { + cleanup(tmp) + } +}) + +test('request_approval falls back to process.cwd() when the session cwd lookup throws', async () => { + const { tmp, repo } = repoFixture() + const previous = process.cwd() + try { + process.chdir(repo) + const tool = createApproveTool({ + getWorkingDirectory: () => ({ + get() { + throw new Error('cannot get property "session" without inject') + }, + }), + getUserQuestions: () => ({ async ask() { return approveAnswer('design') } }), + logger: { warn() {} }, + now: () => new Date(FIXED_ISO), + }) + + const result = await tool.execute({ change_id: 'demo', kind: 'design' }, { agent: { session: 's1' } }) + assert.equal(result.approved, true) + assert.equal(existsSync(approvalPath(repo)), true, 'approval.md is written into process.cwd()') + } finally { + process.chdir(previous) + cleanup(tmp) + } +}) + +// Refactor guard (bounded #77): `/approve` still reports a failing +// userQuestions service as a refusal with the service's reason, and writes +// nothing. The shared runApproval only writes after a `true` answer. +test('approve reports a userQuestions failure and writes nothing', async () => { + const { tmp, repo } = repoFixture() + try { + const captured = {} + const commands = { register(definition) { Object.assign(captured, definition) } } + registerApproveCommand({ + commands, + userQuestions: { + async ask() { + throw new Error('dialog closed') + }, + }, + workingDirectory: { get() { return repo } }, + logger: { warn() {} }, + }) + + const result = await captured.handler({ rawInput: 'demo design' }) + + assert.equal(result.kind, 'error') + assert.equal(result.text, 'Refused: userQuestions failed: dialog closed') + assert.equal(existsSync(approvalPath(repo)), false, 'no approval.md is written') + } finally { + cleanup(tmp) + } +}) diff --git a/dsh/tests/package.test.mjs b/dsh/tests/package.test.mjs index 010625d..6ea27f6 100644 --- a/dsh/tests/package.test.mjs +++ b/dsh/tests/package.test.mjs @@ -3,6 +3,7 @@ import { existsSync, readFileSync, readlinkSync, lstatSync } from 'node:fs' import { join } from 'node:path' import { fileURLToPath } from 'node:url' import test from 'node:test' +import { apply } from '../bundle/index.js' const dshDir = fileURLToPath(new URL('..', import.meta.url)) const bundleDir = join(dshDir, 'bundle') @@ -15,6 +16,7 @@ test('package declares a DSH bundle patch and the runtime files', () => { assert.equal(pkg.exports?.['.'], './index.js') assert.ok(pkg.files.includes('index.js')) assert.ok(pkg.files.includes('command-approve.js')) + assert.ok(pkg.files.includes('tool-approve.js')) assert.ok(pkg.files.includes('lib')) assert.ok(pkg.files.includes('hooks')) assert.ok(existsSync(join(bundleDir, 'cordis.patch.yml'))) @@ -58,9 +60,11 @@ test('runtime files have no external @deepseek-ai imports', () => { 'command-role.js', 'command-check.js', 'command-approve.js', + 'tool-approve.js', 'lib/message.js', 'lib/guards.js', 'lib/gates.js', + 'lib/approval.js', 'lib/bootstrap.js', 'lib/role.js', 'lib/legacy.js', @@ -70,3 +74,24 @@ test('runtime files have no external @deepseek-ai imports', () => { assert.doesNotMatch(source, /from ['"]@deepseek-ai\//, `${file} must not import DSH internals`) } }) + +test('apply registers the request_approval tool when tools.register exists', () => { + const registered = [] + const ctx = { + tools: { + guard() {}, + register(definition) { + registered.push(definition) + }, + }, + workingDirectory: { get() { return process.cwd() } }, + logger: { info() {}, warn() {}, error() {} }, + get() { return { ask: async () => ({ answers: [] }) } }, + } + + apply(ctx) + + assert.equal(registered.length, 1, 'one tool is registered') + assert.equal(registered[0].name, 'request_approval') + assert.deepEqual(registered[0].parameters.required, ['change_id', 'kind']) +}) diff --git a/dsh/tests/run.sh b/dsh/tests/run.sh index 6130c4b..a9d35f6 100755 --- a/dsh/tests/run.sh +++ b/dsh/tests/run.sh @@ -23,7 +23,7 @@ ensure_bundle_hooks() { ensure_bundle_hooks -for f in dsh/bundle/index.js dsh/bundle/command-role.js dsh/bundle/command-check.js dsh/bundle/command-approve.js dsh/bundle/lib/*.js dsh/tests/*.mjs; do +for f in dsh/bundle/index.js dsh/bundle/command-role.js dsh/bundle/command-check.js dsh/bundle/command-approve.js dsh/bundle/tool-approve.js dsh/bundle/lib/*.js dsh/tests/*.mjs; do node --check "$f" done -- 2.47.2 From b81afaf59aecf21209420c463d9ae9f4dae4cf19 Mon Sep 17 00:00:00 2001 From: Andrey Epifancev Date: Sun, 25 Oct 2026 11:06:28 +0400 Subject: [PATCH 2/2] =?UTF-8?q?docs(openspec):=20=D1=81=D0=BF=D0=B5=D0=BA?= =?UTF-8?q?=D0=B0=20approval-gates=20=E2=80=94=20request=5Fapproval=20?= =?UTF-8?q?=D0=BA=D0=B0=D0=BA=20=D0=BB=D0=B5=D0=B3=D0=B8=D1=82=D0=B8=D0=BC?= =?UTF-8?q?=D0=BD=D1=8B=D0=B9=20=D0=BF=D0=B8=D1=81=D0=B0=D1=82=D0=B5=D0=BB?= =?UTF-8?q?=D1=8C=20(T1.1,=20#77)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- openspec/specs/approval-gates/spec.md | 95 +++++++++++++++++++++++---- 1 file changed, 84 insertions(+), 11 deletions(-) diff --git a/openspec/specs/approval-gates/spec.md b/openspec/specs/approval-gates/spec.md index 9bca38d..4d68763 100644 --- a/openspec/specs/approval-gates/spec.md +++ b/openspec/specs/approval-gates/spec.md @@ -16,9 +16,11 @@ builder-check субагента, скопированный в change контроллером; он MUST быть записан после builder-check и до передачи дизайна владельцу. -`openspec/changes//approval.md` SHALL быть маркером утверждения владельцем; -его единственный писатель — команда `/approve` (D-1), инструменты модели писать -его MUST NOT (гейт C ниже). +`openspec/changes//approval.md` SHALL быть маркером утверждения владельцем. +Писать его MUST только код бандла — команда `/approve` или инструмент +`request_approval` (D-1), и оба MUST писать только после подтверждения владельца. +Прямые записи модели (`write`, `edit`, `multi_edit`, `str_replace_editor`, `bash`) +MUST NOT создавать или менять этот файл (гейт C ниже, без изменений). Формат `approval.md` — yaml с секциями `design:` (поля `by`, `at`) и `plan:` (поля `by`, `at`, `sha256`); `sha256` — hex-хэш sha256 содержимого файла плана на @@ -42,11 +44,14 @@ builder-check субагента, скопированный в change конт ### Requirement: Команда /approve -`/approve design|plan` SHALL быть единственным писателем -`approval.md` и MUST регистрироваться при `enabled` и `approvalGates: true`, -независимо от флагов `roleCommand` и `checkCommand`: иначе гейты остаются без -команды-разблокировки (T3.2). `change-id` MUST быть одним безопасным сегментом -пути; небезопасный id или неизвестный вид — usage-отказ без записи. +`/approve design|plan` SHALL быть ручным писателем `approval.md` — +одним из двух писателей бандла, наравне с инструментом `request_approval` +(следующее требование); оба пишут только после подтверждения владельца, прямые +записи модели — MUST NOT (гейт C). Команда MUST регистрироваться при `enabled` и +`approvalGates: true`, независимо от флагов `roleCommand` и `checkCommand`: иначе +гейты остаются без команды-разблокировки (T3.2). `change-id` MUST быть одним +безопасным сегментом пути; небезопасный id или неизвестный вид — usage-отказ без +записи. Вид `design` SHALL требовать существования `proposal.md`, `design.md` и `builder-check.md` в `openspec/changes//`; при отсутствии файла команда @@ -93,6 +98,71 @@ MUST отказать с перечислением отсутствующих - **THEN** команда отказывает с `userQuestions service unavailable; /approve requires it` и `approval.md` не пишется +### Requirement: Инструмент request_approval + +Инструмент модели `request_approval` SHALL быть вторым писателем `approval.md`: +модель инициирует утверждение, владелец только подтверждает в диалоге, а маркер +пишет код бандла (`lib/approval.js`) — с теми же проверками предпосылок и той же +записью, что у `/approve`. Инструмент MUST регистрироваться при `enabled` и +`approvalGates: true`, если в контексте доступен `ctx.tools.register`; +`userQuestions` MUST резолвиться лениво (не через `inject`), чтобы отсутствие +сервиса не парковало весь бандл. + +Параметры инструмента — `change_id` (строка) и `kind` (`design` | `plan`). +Валидация MUST быть собственной, в инструменте: пустой или небезопасный +`change_id` и неизвестный `kind` — usage-отказ без записи и без вопроса +владельцу; проверки предпосылок и отказы те же, что у `/approve` (design — +proposal/design/builder-check, plan — утверждённый дизайн, ровно один план, +sha256), и любой отказ MUST NOT писать `approval.md`. Подтверждением SHALL +считаться только ответ `answers[0].selected[0] === 'Approve'`; Cancel, +отсутствие ответа и ошибка диалога — отказ `not confirmed` без записи. При +отсутствии сервиса `userQuestions` инструмент MUST отказать (`userQuestions +service unavailable`), а не писать. + +#### Scenario: Модель просит утвердить дизайн + +- **WHEN** модель вызывает `request_approval` с `{change_id: , kind: design}`, + предпосылки выполнены, а владелец выбрал Approve +- **THEN** в `approval.md` пишется design-секция (`by: owner`, `at` в ISO 8601 + UTC), инструмент возвращает `{approved: true, message}` + +#### Scenario: Модель просит утвердить план + +- **WHEN** модель вызывает `request_approval` с `{change_id: , kind: plan}`, + дизайн утверждён, план единственный, владелец выбрал Approve +- **THEN** plan-секция содержит `by: owner`, `at` в ISO 8601 и sha256 текущего + содержимого плана + +#### Scenario: Владелец отменил утверждение + +- **WHEN** в диалоге `request_approval` владелец выбрал Cancel +- **THEN** инструмент возвращает `{approved: false, message: 'not confirmed'}`, а + `approval.md` не меняется + +#### Scenario: Некорректные аргументы request_approval + +- **WHEN** модель передала неизвестный `kind` или небезопасный `change_id` + (например, `../x`) +- **THEN** инструмент возвращает usage-отказ без записи, владелец не спрашивается + +#### Scenario: Отсутствует builder-check при утверждении дизайна моделью + +- **WHEN** `kind: design`, а `builder-check.md` в change нет +- **THEN** инструмент отказывает с перечислением отсутствующего файла, владелец не + спрашивается, `approval.md` не создаётся + +#### Scenario: request_approval plan без утверждённого дизайна + +- **WHEN** `kind: plan`, а design-секции нет +- **THEN** инструмент отказывает `design not approved; run /approve design`, + владелец не спрашивается, `approval.md` не меняется + +#### Scenario: request_approval без сервиса вопросов + +- **WHEN** `userQuestions` отсутствует +- **THEN** инструмент возвращает `{approved: false}` с `userQuestions service + unavailable`, владелец не спрашивается, `approval.md` не пишется + ### Requirement: Гейт A — план до утверждения дизайна Для write/edit/multi_edit/`str_replace_editor` с целевым путём под @@ -231,9 +301,12 @@ fail closed. Фрагмент `edit` проверяется по своему `n ### Requirement: Гейт C и bash-обход -`approval.md` MUST писаться только командой `/approve`: любой write/edit с целью, -чей basename — `approval.md`, и любая bash-цель с таким именем MUST получать -отказ `approval.md is written only by /approve` (D-1). +`approval.md` MUST писаться только кодом бандла — командой `/approve` или +инструментом `request_approval`: любой write/edit с целью, чей basename — +`approval.md`, и любая bash-цель с таким именем MUST получать отказ `approval.md +is written only by /approve` (D-1). Deny-текст не меняется: он адресован прямым +записям модели (`write`/`edit`/`bash`), а `request_approval` пишет маркер кодом +бандла и под этот запрет не попадает. Bash-обход MUST быть закрыт (D-8): если команда содержит грубую подстроку `docs/superpowers/plans`, `approval.md` или `.superpowers/sdd`, её файловые цели -- 2.47.2