feat(dsh): request_approval-инструмент — модель инициирует аппрув, владелец жмёт Approve (#77)
This commit is contained in:
Andrey Epifancev
parent
c28a074640
commit
1723806e16
@ -1,11 +1,13 @@
|
||||
/**
|
||||
* Human-facing `/approve` command for DeepSeek Harness.
|
||||
*
|
||||
* `/approve <change-id> design|plan` is the only writer of
|
||||
* `openspec/changes/<id>/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 <change-id> 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/<id>/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 <change-id> design|plan',
|
||||
@ -36,27 +27,8 @@ const USAGE = [
|
||||
' plan — record the owner approval of the plan whose **Spec:** is <change-id>',
|
||||
].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 `<id>`. */
|
||||
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)}`)
|
||||
}
|
||||
|
||||
@ -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',
|
||||
|
||||
187
dsh/bundle/lib/approval.js
Normal file
187
dsh/bundle/lib/approval.js
Normal file
@ -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<boolean>`,
|
||||
* 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 `<id>`. */
|
||||
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 `<changeId> <kind>` 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 })
|
||||
}
|
||||
@ -14,6 +14,7 @@
|
||||
"command-role.js",
|
||||
"command-check.js",
|
||||
"command-approve.js",
|
||||
"tool-approve.js",
|
||||
"lib",
|
||||
"dsh-tools.md",
|
||||
"cordis.patch.yml",
|
||||
|
||||
144
dsh/bundle/tool-approve.js
Normal file
144
dsh/bundle/tool-approve.js
Normal file
@ -0,0 +1,144 @@
|
||||
/**
|
||||
* `request_approval` — the model-driven front-end of the approval gate.
|
||||
*
|
||||
* The owner no longer has to type `/approve <change-id> 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: '<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<unknown> } | 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/<id>/',
|
||||
},
|
||||
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))
|
||||
}
|
||||
},
|
||||
}
|
||||
}
|
||||
@ -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)
|
||||
}
|
||||
})
|
||||
|
||||
@ -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'])
|
||||
})
|
||||
|
||||
@ -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
|
||||
|
||||
|
||||
Loading…
x
Reference in New Issue
Block a user