fix(jobs): address review — error scoping, subagent fence, pending kill

- session.killJob maps job-not-found from the lookup only: jobs.get gates the
  same synchronous span, so a producer-cancel throw now propagates per the
  registry contract (job state unchanged) instead of masquerading as a lookup
  failure (threads r3860226021, r3860226047).
- session.killJob applies the service-wide subagent ownership fence exactly as
  cancel does; a subagent-owned live session rejects with the ownership error
  and the child-owned job stays running (thread r3860226041).
- The stop control stays pending on an admitted kill: the unary response and
  the jobs frames have no cross-carrier ordering, so only the authoritative
  frame (the row leaving the killable set) releases the button, closing the
  duplicate-kill window (thread r3860226028).
- One-line comment on the killReason assignment stating last-writer-wins is
  intended, beside the first-claim-wins reported handling (thread r3860225999).

Host spec pins the producer-throw propagation and the fence; the component
spec pins pending-until-frame; the Agent Note records both refinements.
This commit is contained in:
Yichen Jiang committed 2026-08-26 16:19:34 +08:00
1 parent d6bebc5783
commit 8f3103b511
8 files changed
+70 -19

No files matched your search

@@ -2,5 +2,5 @@
# side as of the last confirmed-consistent state. Both languages carry equal authority;
# after editing either side, bring the other along and re-record with:
# pnpm run verify-translation-pairing --write .agents/notes/implemented/feature/2026-08-26-human-job-kill.md
2026-08-26-human-job-kill.md: 9ef2d66e129ea70fa927528717f87e4cfec66a1b
2026-08-26-human-job-kill.zh.md: 1b9933b5af48b736fce4a6562338ff9f1d75c6d4
2026-08-26-human-job-kill.md: 2242ed2cf5612394f6797106decf75d8543fc420
2026-08-26-human-job-kill.zh.md: 37a2d2b38bfa74aaaef6c5ba34fa8e0c7761178b
@@ -14,8 +14,8 @@ The `reported` bit means "the terminal state has a committed delivery path to th
- **`kill(id, caller?, options?)` takes `{ reason?, reported? }`.** `reported` defaults to `true` — the exact previous semantics for the model's `job_kill`. `reported: false` withholds this kill's claim, so the settlement flows through the existing completion reporter (wakeup/inject, wake budget, truncation all unchanged). It never clears an existing claim: a job the model already killed stays reported, and a terminal record stays exactly as settlement left it.
- **A recorded kill reason merges into a `killed` settlement's detail** — producer facts first (`signal: SIGTERM; cancelled by the user`) — so the completion notice and the web row both say who stopped the work without a new snapshot field or notice template. A job that outruns its kill (settles `completed`/`failed`) keeps the producer detail alone. This is the reconciliation Codex spells out in its `<turn_aborted>` guidance: when the user stops something, the model is told rather than left to infer.
- **`session.killJob` is a Session Controller Remote** beside `cancel`, with the same live-only Agent lookup (`ctx.agents.get`): a running job's owner is alive by the registry's ownership contract, and killing from a list must not revive a Session any more than listing does. Unknown and foreign jobs collapse to one `job-not-found` rejection; a composition without `ctx.jobs` rejects `jobs-unavailable`. No approval interaction: this single-user local BFF treats it as the same class of action as the turn-cancel button.
- **The task list's stop control is two-press** (arm, then confirm within 3s, matching the Kimi `s`+`y` and OpenCode double-Esc shape), rendered only on running job rows — rows with a `jobId`. Standalone activity rows (workflow runs) have no kill handle and no button. The pending press disables the control; the row's own `stopping` flip from the jobs frames removes it, and a rejected kill shows a brief hint. Because the kill addresses `ctx.jobs` by id, every job kind gains the control at once — bash, pwsh, pty, and one-shot background subagents.
- **`session.killJob` is a Session Controller Remote** beside `cancel`, with the same live-only Agent lookup (`ctx.agents.get`): a running job's owner is alive by the registry's ownership contract, and killing from a list must not revive a Session any more than listing does. Unknown and foreign jobs collapse to one `job-not-found` rejection scoped to the lookup — a producer-cancel throw propagates per the registry contract instead of masquerading as a lookup failure; a composition without `ctx.jobs` rejects `jobs-unavailable`, and the service-wide subagent ownership fence applies exactly as it does to `cancel`. No approval interaction: this single-user local BFF treats it as the same class of action as the turn-cancel button.
- **The task list's stop control is two-press** (arm, then confirm within 3s, matching the Kimi `s`+`y` and OpenCode double-Esc shape), rendered only on running job rows — rows with a `jobId`. Standalone activity rows (workflow runs) have no kill handle and no button. The pending press disables the control and stays pending on an admitted kill — the unary response and the jobs frames have no cross-carrier ordering, so only the authoritative frame (the row leaving the killable set) releases it; a rejected kill shows a brief hint. Because the kill addresses `ctx.jobs` by id, every job kind gains the control at once — bash, pwsh, pty, and one-shot background subagents.
## Alternatives considered
@@ -14,8 +14,8 @@
- **`kill(id, caller?, options?)` 接受 `{ reason?, reported? }`。** `reported` 默认 `true`——即模型 `job_kill` 的原有语义。`reported: false` 只是不提出本次 kill 的认领,结算随即照常走完成播报员(唤醒/注入、唤醒预算、截断全部不变)。它绝不清除已有认领:模型已 kill 过的任务保持 reported,终态记录也保持结算时的原样。
- **记录下来的 kill reason 合入 `killed` 结算的 detail**——生产者事实在前(`signal: SIGTERM; cancelled by the user`)——完成通知与 Web 行因此都能说清是谁停的工作,无需新快照字段或新通知模板。跑赢 kill 的任务(结算为 `completed`/`failed`)只保留生产者 detail。这正是 Codex 在 `<turn_aborted>` 指引里写明的对账原则:用户停掉了什么,要告诉模型,而不是让它去猜。
- **`session.killJob` 是 Session Controller 的 Remote**,与 `cancel` 并列,同样只查活体 Agent(`ctx.agents.get`):按注册表的所有权契约,在跑任务的 owner 必然活着;从列表里 kill 与列表本身一样绝不复活 Session。未知与他人任务合并为一个 `job-not-found` 拒绝;没有 `ctx.jobs` 的组合拒绝 `jobs-unavailable`。不加 approval 交互:这个单用户本地 BFF 把它视为与停轮按钮同级的操作。
- **任务列表的停止控件为两击式**(先武装、3 秒内确认,对齐 Kimi 的 `s`+`y` 与 OpenCode 的双 Esc),只渲染在运行中的 job 行——即带 `jobId` 的行。独立 activity 行(workflow 运行)没有 kill 句柄也就没有按钮。请求进行中按钮禁用;jobs 帧把行翻成 `stopping` 即移除按钮,被拒绝的 kill 短暂提示。由于 kill 按 id 寻址 `ctx.jobs`,所有任务 kind 一次性获得该控件——bash、pwsh、pty 与一次性后台 subagent。
- **`session.killJob` 是 Session Controller 的 Remote**,与 `cancel` 并列,同样只查活体 Agent(`ctx.agents.get`):按注册表的所有权契约,在跑任务的 owner 必然活着;从列表里 kill 与列表本身一样绝不复活 Session。未知与他人任务合并为一个只作用于查找的 `job-not-found` 拒绝——生产者 cancel 抛错按注册表契约原样传播,绝不伪装成查找失败;没有 `ctx.jobs` 的组合拒绝 `jobs-unavailable`,服务级的 subagent 所有权栅栏与 `cancel` 完全一致地适用。不加 approval 交互:这个单用户本地 BFF 把它视为与停轮按钮同级的操作。
- **任务列表的停止控件为两击式**(先武装、3 秒内确认,对齐 Kimi 的 `s`+`y` 与 OpenCode 的双 Esc),只渲染在运行中的 job 行——即带 `jobId` 的行。独立 activity 行(workflow 运行)没有 kill 句柄也就没有按钮。请求进行中按钮禁用,且受理成功后保持 pending——unary 响应与 jobs 帧没有跨载体顺序保证,只有权威帧(行离开可杀集合)才释放控件;被拒绝的 kill 短暂提示。由于 kill 按 id 寻址 `ctx.jobs`,所有任务 kind 一次性获得该控件——bash、pwsh、pty 与一次性后台 subagent。
## 备选方案
@@ -451,7 +451,8 @@ export class SessionCommandController {
* the standard completion notice — a human cancellation has no other
* model-visible channel. The Agent lookup is live-only: a running job's
* owner is alive by the registry's ownership contract, and listing never
* revives a Session, so neither does killing from its list.
* revives a Session, so neither does killing from its list. The service-wide
* subagent ownership fence applies exactly as it does to `cancel`.
* @param request - Session whose task list carries the job, and the job id.
* @returns the registry's admission of the kill request.
*/
@@ -461,12 +462,11 @@ export class SessionCommandController {
reject('jobs-unavailable', 'background jobs are not available in this composition', {})
}
const agent = this.ctx.agents.get(request.sessionId)
if (agent !== undefined && hasApiSessionSubagentOwner(this.ctx, agent.session, agent)) {
rejectFailure(apiSessionSubagentOwnershipError(request.sessionId))
}
try {
const outcome = jobs.kill(request.jobId, agent, {
reason: 'cancelled by the user',
reported: false,
})
return { outcome }
jobs.get(request.jobId, agent)
} catch (error) {
// `unknown job` and `belongs to another session` both mean this session's
// list no longer carries a killable row; the client renders one story.
@@ -475,6 +475,14 @@ export class SessionCommandController {
jobId: request.jobId,
})
}
// Same synchronous span as the lookup, so nothing can remove the job in
// between — and a producer-cancel throw now propagates per the registry
// contract (job state unchanged) instead of masquerading as job-not-found.
const outcome = jobs.kill(request.jobId, agent, {
reason: 'cancelled by the user',
reported: false,
})
return { outcome }
}
private async resolveAgent(sessionId: SessionId): Promise<Agent> {
@@ -117,4 +117,40 @@ describe('session.killJob', () => {
expect(failureCode(() => commands.killJob({ sessionId: session.id, jobId: 'bash-1' as JobId })))
.toBe('jobs-unavailable')
})
it('propagates a producer cancel throw instead of masking it as job-not-found', async () => {
const { ctx, session, agent, commands } = await harness()
const id = ctx.jobs.start({
kind: 'bash',
label: 'flaky cancel',
owner: agent,
run: () => ({
cancel: () => { throw new Error('cancel boom') },
done: new Promise(() => {}),
}),
})
// The registry contract: a producer throw propagates with job state
// unchanged — the command must not rewrite it into a lookup failure.
expect(() => commands.killJob({ sessionId: session.id, jobId: id })).toThrow('cancel boom')
expect(ctx.jobs.get(id, agent)).toMatchObject({ status: 'running', reported: false })
})
it('rejects a subagent-owned live session with the ownership fence', async () => {
const { ctx, commands } = await harness()
const child = ctx.sessions.create(undefined, { meta: { origin: 'subagent' } })
const childAgent = {
id: child.id,
session: child,
inbox: new Inbox(child, { inserted: () => {}, discarded: () => {}, claimed: () => {} }),
status: 'idle',
ctx,
} as Agent
ctx.agents.register(childAgent)
const task = producer('child work')
const id = ctx.jobs.start({ ...task.spec, owner: childAgent })
expect(failureCode(() => commands.killJob({ sessionId: child.id, jobId: id })))
.toBe('agent-busy')
expect(ctx.jobs.get(id, childAgent).status).toBe('running')
})
})
@@ -485,10 +485,13 @@ export function ActivityListAction({ sessionId, useSessions, useActivity, observ
}
setKillPhase({ key: row.key, state: 'pending' })
void killJob(sessionId, jobId).then((ok) => {
// Success needs no local state: the jobs frame flips the row to
// `stopping`, which removes the button and clears the phase above.
setKillPhase(current => current?.key === row.key
? (ok ? undefined : { key: row.key, state: 'failed' })
// An admitted kill stays pending: the unary response (HTTP) and the jobs
// frames (control stream) have no cross-carrier ordering, so re-enabling
// here could offer a duplicate kill while the row still reads `running`.
// The authoritative jobs frame flips the row to `stopping`, which removes
// the button and clears the phase through the killable-set effect above.
setKillPhase(current => current?.key === row.key && !ok
? { key: row.key, state: 'failed' }
: current)
})
}
@@ -407,9 +407,11 @@ describe('ActivityListAction human kill', () => {
expect(screen.getByTitle(zh['kill.confirm'])).toBe(stop)
await act(async () => { fireEvent.click(stop) })
expect(killJob).toHaveBeenCalledWith(SESSION, 'bash-1')
// The accepted kill leaves row convergence to the jobs frames; the local
// phase resets so the control does not stick in pending.
expect(stop.getAttribute('data-kill-state')).toBe('idle')
// An admitted kill stays pending: the unary response and the jobs frames
// have no cross-carrier ordering, so only the authoritative frame (the row
// leaving the killable set) releases the control — never the response.
expect(stop.getAttribute('data-kill-state')).toBe('pending')
expect(stop.hasAttribute('disabled')).toBe(true)
})
it('an armed press disarms after the confirmation window', () => {
+2
View File
@@ -228,6 +228,8 @@ export class LocalJobRegistry extends JobRegistry {
// Cancel first so a throw leaves both lifecycle and notice state unchanged.
job.cancel(options?.reason)
job.status = 'stopping'
// Last writer wins on purpose: the detail reports the latest kill intent,
// while `reported` above keeps first-claim-wins for notice delivery.
if (options?.reason !== undefined) job.killReason = options.reason
if (claims) job.reported = true
this.notifyChanged(job.owner)