From 5cc94a8959b32297aa655fa6e1572c0597bb305a Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 8 Aug 2026 16:29:31 +0000 Subject: [PATCH] =?UTF-8?q?refactor(plugin-chatbot):=20ApproveOutcome/Reje?= =?UTF-8?q?ctOutcome=20=E6=94=B9=E4=B8=BA=E4=BB=8E=20spec=20=E6=B4=BE?= =?UTF-8?q?=E7=94=9F,id=20=E5=BD=92=E4=BD=8D=E5=88=B0=20reject=20=E4=B8=80?= =?UTF-8?q?=E4=BE=A7=20(#3783)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `usePendingActions.ts` 里这两个类型是 spec approve/reject 响应的手写镜像,与 #3220 从同一文件清掉的 `PendingActionRow`/`PendingActionStatus` 同一失败类; 不同的是它们穿的是**本地名字**,所以 `check-spec-symbol-derivation.mjs`(按 spec 导出名被占用触发)对它们完全没有抓手 —— 换名手抄对名字型守卫天生隐形。 两者现在 re-export spec 的决策响应(`@objectstack/spec/api` 的 `ApproveAiPendingActionResponse` / `RejectAiPendingActionResponse`,也正是 `@objectstack/client` 的 `ai.pendingActions.approve()/.reject()` 用来标注返回值 的同一批 schema)。公开导出名不变,形状变三处: - `ApproveOutcome` 不再声明 `id`。approve 响应从来不带 `id`,`id` 在 reject 侧。 这是唯一一条不休眠的漂移:公开回调 `onDecided` 编译期承诺 `id: string`,运行期 给的是 `undefined`,编译器一声不响; - `status` 闭合:`'executed' | 'failed' | string` 与 `'rejected' | string` 都只是 `string`(与 `string` 的联合吸收字面量),现为 `'executed' | 'failed'` 与 `'rejected'`; - 去掉 `[k: string]: unknown`(objectstack#4075 机制:索引签名让任何结构比较恒答 "一致",给旧类型补 parity 测试也会从第一天就是绿的)。 **运行期行为零变更**,包括两处刻意保留的:非 2xx 时本地虚构的失败信封仍带 `id` (它不是 wire 响应,而是本地通知),以及 `decide()` 对 spec 词表之外的 status 仍 渲染成功 chip —— 后者的类型压力为零,因为 `status` 是从未解析的 `Record` 上读出的 `string`,闭合枚举施加不了穷尽性检查。两者都新 补了测试钉住。`useHitlInChat` 剩下的消费侧容忍(该失败信封的契约、status 兜底 默认、未知 status 当成功)记入 #3790 交 maintainer 裁决。 守卫配套:`spec-symbol-batch6.test.ts` 补 `Assert>` 钉子(该文件已进 `tsconfig.typetests.json` 执行面),另加两条反向钉 —— spec 仍导出被派生的两个名字、 两个本地名字仍不与 spec 撞名(后者正是名字型守卫看不见它们的前提)。 --- ...itl-decision-outcomes-spec-derived-3783.md | 43 +++++++ .../src/__tests__/spec-symbol-batch6.test.ts | 91 ++++++++++++- .../src/__tests__/useHitlInChat.test.tsx | 121 +++++++++++++++++- packages/plugin-chatbot/src/useHitlInChat.ts | 28 +++- .../plugin-chatbot/src/usePendingActions.ts | 60 ++++++--- 5 files changed, 320 insertions(+), 23 deletions(-) create mode 100644 .changeset/hitl-decision-outcomes-spec-derived-3783.md diff --git a/.changeset/hitl-decision-outcomes-spec-derived-3783.md b/.changeset/hitl-decision-outcomes-spec-derived-3783.md new file mode 100644 index 0000000000..827aa22d15 --- /dev/null +++ b/.changeset/hitl-decision-outcomes-spec-derived-3783.md @@ -0,0 +1,43 @@ +--- +"@object-ui/plugin-chatbot": minor +--- + +`ApproveOutcome` / `RejectOutcome` are now derived from `@objectstack/spec` +instead of hand-transcribed (objectui#3783). Same failure class #3220 cleared +from the same file for `PendingActionRow` / `PendingActionStatus` — but this pair +wore local names rather than spec names, so `check-spec-symbol-derivation.mjs`, +which fires on a spec export name being occupied, had no handle on it. A renamed +hand copy is invisible to a name-based guard by construction. + +Both types now re-export the spec's decision responses +(`ApproveAiPendingActionResponse` / `RejectAiPendingActionResponse` from +`@objectstack/spec/api` — the same schemas `@objectstack/client`'s +`ai.pendingActions.approve()` / `.reject()` type their returns with). The public +export names do not change. The shapes do, in three ways: + +- **`ApproveOutcome` no longer declares `id`.** The approve response has never + carried one — `id` is on the *reject* response. This was the one drift that + was not dormant: `useHitlInChat`'s public `onDecided` callback promised + consumers `id: string` and handed them `undefined` at runtime, with nothing + in the compiler to say so. **If you read `outcome.id` after an approve, that + read was already `undefined` and now fails to compile** — take the id from + `ContinueContext.pendingActionId` or from the row you decided on. +- **`status` is closed.** `'executed' | 'failed' | string` and + `'rejected' | string` were both just `string`: a union with `string` absorbs + the literals, so neither annotation carried any information. They are now + `'executed' | 'failed'` and `'rejected'`. +- **The `[k: string]: unknown` index signature on `ApproveOutcome` is gone.** The + objectstack#4075 mechanism: with it, any structural comparison against the + spec answers "identical" however far the copy has drifted, so a parity test + bolted onto the old type would have been green from its first day. + +**Breaking at the type level for importers of `@object-ui/plugin-chatbot`** — +narrowing a published type is a break even when the old type was lying, which is +why it is spelled out here. Shipped as `minor` per AGENTS.md §版本号策略: the +family's `major` tracks `@objectstack`'s, and objectui's own breaking changes go +out as `minor` with the break named in the changeset. + +Runtime behaviour is unchanged — including the hook's decision handling for a +status outside the spec vocabulary, and the locally synthesized failure envelope +on a non-2xx, both now pinned by tests. The consumer-side tolerances that remain +in `useHitlInChat` are recorded in objectui#3790 for a maintainer decision. diff --git a/packages/plugin-chatbot/src/__tests__/spec-symbol-batch6.test.ts b/packages/plugin-chatbot/src/__tests__/spec-symbol-batch6.test.ts index 04f3ef917c..9d8c50450c 100644 --- a/packages/plugin-chatbot/src/__tests__/spec-symbol-batch6.test.ts +++ b/packages/plugin-chatbot/src/__tests__/spec-symbol-batch6.test.ts @@ -33,6 +33,16 @@ * A path skip is broader than an ALLOW entry, and the hole it opens is an * objectui-AUTHORED file dropped into that directory and silently unscanned. * `the vendored directory stays vendored` below is what closes it. + * + * objectui#3783 added a fifth and sixth pin here for the guard's OTHER hole — + * the one no allowlist and no path skip is responsible for. `ApproveOutcome` / + * `RejectOutcome` in the same `usePendingActions.ts` were hand copies of the + * spec's approve/reject wire responses under DIFFERENT local names, so the + * name-collision guard was never going to see them: it fires on a spec name + * being occupied, and these occupied none. A renamed hand copy is invisible to + * a name-based check by construction, which makes a compile-time parity pin the + * only thing that can hold them — see + * `the decision outcomes ARE the spec wire responses` at the bottom. */ import { describe, it, expect } from 'vitest'; @@ -42,11 +52,20 @@ import { readFileSync, readdirSync } from 'node:fs'; import { resolve, dirname, join } from 'node:path'; import { fileURLToPath } from 'node:url'; -import type { PendingActionRow, PendingActionStatus } from '../usePendingActions'; +import type { + ApproveOutcome, + PendingActionRow, + PendingActionStatus, + RejectOutcome, +} from '../usePendingActions'; import type { PendingActionRow as SpecPendingActionRow, PendingActionStatus as SpecPendingActionStatus, } from '@objectstack/spec/contracts'; +import type { + ApproveAiPendingActionResponse, + RejectAiPendingActionResponse, +} from '@objectstack/spec/api'; /** Every name `@objectstack/spec` exports from any subpath — types AND values. */ function specExportNames(): Set { @@ -241,3 +260,73 @@ describe('the pending-action row and status ARE the spec contract', () => { expect(new Set(all).size).toBe(5); }); }); + +describe('the decision outcomes ARE the spec wire responses', () => { + it('is pinned at compile time', () => { + type _ApproveNotAny = Assert, false>>; + type _ApproveNotUnknown = Assert, false>>; + type _RejectNotAny = Assert, false>>; + + type _ApproveIsSpec = Assert>; + type _RejectIsSpec = Assert>; + + // Drift 1 — the copy declared `id: string`, REQUIRED, on the APPROVE side. + // The approve response has no `id` at all; `id` is the REJECT response's. + // This is the one drift that was not dormant: `useHitlInChat`'s public + // `onDecided` callback handed consumers this type over a payload that has + // never carried the field, so `outcome.id` type-checked and evaluated to + // `undefined`. The pin is `keyof`-shaped rather than an `Equal` on the + // property type because the failure to catch is the key EXISTING. + type _ApproveHasNoId = Assert>; + type _RejectHasId = Assert>; + + // Drift 2 — `'executed' | 'failed' | string` and `'rejected' | string`. A + // union with `string` ABSORBS the literals, so both annotations conveyed + // nothing; `AiPendingActionsInbox`'s `out.status === 'executed'` could have + // been compared against any spelling at all. + type _ApproveStatusNotString = Assert< + Equal + >; + type _RejectStatusNotString = Assert< + Equal + >; + type _ApproveVocabulary = Assert>; + type _RejectVocabulary = Assert>; + + // Drift 3 — `[k: string]: unknown` on the approve copy. objectstack#4075: + // with it, this whole describe block would have been green on the copies. + type _ApproveNoIndexSignature = Assert, false>>; + type _RejectNoIndexSignature = Assert, false>>; + + expect(true).toBe(true); + }); + + it('the spec still exports the names these are derived from', () => { + // The reverse pin. `check-spec-symbol-derivation.mjs` cannot cover this + // pair — the local names are `ApproveOutcome` / `RejectOutcome`, which + // collide with nothing, and that is exactly how a renamed hand copy hides + // from a name-based guard. So the only thing standing between these types + // and a fresh hand copy is the `Assert>` block above plus this: + // if the spec renames or retires either response type, the import breaks + // loudly at compile time instead of the derivation quietly rotting. + for (const owned of ['ApproveAiPendingActionResponse', 'RejectAiPendingActionResponse']) { + expect( + SPEC_NAMES.has(owned), + `@objectstack/spec no longer exports \`${owned}\`, which ` + + `packages/plugin-chatbot/src/usePendingActions.ts derives its public ` + + `\`${owned.startsWith('Approve') ? 'ApproveOutcome' : 'RejectOutcome'}\` from. ` + + `Re-derive from the replacement — do NOT re-transcribe the shape locally ` + + `(objectui#3783).`, + ).toBe(true); + } + }); + + it('neither local name collides with a spec export', () => { + // Pins the PREMISE of the two pins above: were either name to become a spec + // export, `check-spec-symbol-derivation.mjs` would start covering this file + // for it and this note would be stale. + for (const local of ['ApproveOutcome', 'RejectOutcome']) { + expect(SPEC_NAMES.has(local)).toBe(false); + } + }); +}); diff --git a/packages/plugin-chatbot/src/__tests__/useHitlInChat.test.tsx b/packages/plugin-chatbot/src/__tests__/useHitlInChat.test.tsx index 55b8306aae..8aa9d61113 100644 --- a/packages/plugin-chatbot/src/__tests__/useHitlInChat.test.tsx +++ b/packages/plugin-chatbot/src/__tests__/useHitlInChat.test.tsx @@ -31,8 +31,12 @@ describe('useHitlInChat', () => { it('fires continueConversation with an executed-outcome prompt on approve', async () => { (globalThis.fetch as ReturnType).mockResolvedValueOnce({ ok: true, + // The approve wire response, exactly: `{ status, result?, error? }` and no + // `id` (spec `ApproveAiPendingActionResponseSchema`; objectui#3783). The + // `pa_42` the prompt below carries comes from the message index, not from + // this payload — which is why the old `id: string` promise on + // `ApproveOutcome` was never load-bearing here and stayed invisible. text: async () => JSON.stringify({ - id: 'pa_42', status: 'executed', result: { deleted: 1, taskId: 't1' }, }), @@ -93,7 +97,7 @@ describe('useHitlInChat', () => { it('does NOT continue when execution failed', async () => { (globalThis.fetch as ReturnType).mockResolvedValueOnce({ ok: true, - text: async () => JSON.stringify({ id: 'pa_42', status: 'failed', error: 'boom' }), + text: async () => JSON.stringify({ status: 'failed', error: 'boom' }), } as Response); const continueConversation = vi.fn(); @@ -113,10 +117,121 @@ describe('useHitlInChat', () => { expect(result.current.decisions['tc-1']?.message).toContain('boom'); }); + /* ------------------------------------------------------------------ */ + /* objectui#3783 — what `onDecided` actually receives. */ + /* ------------------------------------------------------------------ */ + + it('hands onDecided the approve payload verbatim — which carries no id', async () => { + // The drift `ApproveOutcome.id: string` promised: it was REQUIRED on the + // type and absent from the wire, so a consumer reading `outcome.id` here + // got `undefined` with no compiler complaint. The type no longer declares + // it; this pins that the runtime object never had it either. + (globalThis.fetch as ReturnType).mockResolvedValueOnce({ + ok: true, + text: async () => JSON.stringify({ status: 'executed', result: { deleted: 1 } }), + } as Response); + + const onDecided = vi.fn(); + const { result } = renderHook(() => + useHitlInChat({ messages: [baseMessage('pa_42')], onDecided }), + ); + + await act(async () => { + await result.current.decide('tc-1', true); + }); + + expect(onDecided).toHaveBeenCalledTimes(1); + const [toolCallId, outcome] = onDecided.mock.calls[0]; + expect(toolCallId).toBe('tc-1'); + expect(outcome).toEqual({ status: 'executed', result: { deleted: 1 } }); + expect('id' in (outcome as object)).toBe(false); + }); + + it('hands onDecided the reject payload verbatim — where id IS the wire', async () => { + // Mirror image: the spec puts `id` on the reject response, so it is present + // here and `RejectOutcome` declares it. + (globalThis.fetch as ReturnType).mockResolvedValueOnce({ + ok: true, + text: async () => JSON.stringify({ status: 'rejected', id: 'pa_42' }), + } as Response); + + const onDecided = vi.fn(); + const { result } = renderHook(() => + useHitlInChat({ messages: [baseMessage('pa_42')], onDecided }), + ); + + await act(async () => { + await result.current.decide('tc-1', false, 'too risky'); + }); + + expect(onDecided).toHaveBeenCalledWith('tc-1', { status: 'rejected', id: 'pa_42' }); + }); + + it('still synthesizes the locally-built failure envelope, id included, on a non-2xx', async () => { + // Behaviour pin for the type-only narrowing (objectui#3783): on a non-2xx + // there is no decision response at all, so the hook fabricates one. That + // object has carried `id` since the callback shipped and still does — + // `ApproveOutcome` simply stopped DECLARING a field the wire never sent. + (globalThis.fetch as ReturnType).mockResolvedValueOnce({ + ok: false, + status: 403, + text: async () => JSON.stringify({ error: 'ai:approve required' }), + } as Response); + + const onDecided = vi.fn(); + const continueConversation = vi.fn(); + const { result } = renderHook(() => + useHitlInChat({ messages: [baseMessage('pa_42')], onDecided, continueConversation }), + ); + + await act(async () => { + await result.current.decide('tc-1', true); + }); + + expect(onDecided).toHaveBeenCalledWith('tc-1', { + id: 'pa_42', + status: 'failed', + error: 'ai:approve required', + }); + expect(result.current.decisions['tc-1']?.state).toBe('error'); + expect(continueConversation).not.toHaveBeenCalled(); + }); + + it('keeps treating an unrecognised status as a success chip, without continuing', async () => { + // The `else` fallback in `decide()`. objectui#3783 narrowed the TYPES only, + // and deliberately left this branch alone: `status` is read off an unparsed + // `Record`, so the closed enum exerts no exhaustiveness + // pressure on it and the branch stays reachable for a server that answers + // outside the spec vocabulary. Pinned as-is so a later behaviour verdict + // is a visible diff rather than a silent one — including the part the + // filing report got wrong: `succeeded` goes true, but the continuation + // prompt builder returns `undefined` for an unknown status, so the + // conversation is NOT continued. + (globalThis.fetch as ReturnType).mockResolvedValueOnce({ + ok: true, + text: async () => JSON.stringify({ status: 'quarantined' }), + } as Response); + + const continueConversation = vi.fn(); + const { result } = renderHook(() => + useHitlInChat({ messages: [baseMessage('pa_42')], continueConversation }), + ); + + await act(async () => { + await result.current.decide('tc-1', true); + }); + + expect(result.current.decisions['tc-1']).toEqual({ + state: 'success', + message: 'Status: quarantined', + }); + expect(continueConversation).not.toHaveBeenCalled(); + }); + it('does NOT continue when option is omitted', async () => { (globalThis.fetch as ReturnType).mockResolvedValueOnce({ ok: true, - text: async () => JSON.stringify({ id: 'pa_42', status: 'executed', result: 'ok' }), + text: async () => JSON.stringify({ status: 'executed', result: 'ok' }), } as Response); const { result } = renderHook(() => diff --git a/packages/plugin-chatbot/src/useHitlInChat.ts b/packages/plugin-chatbot/src/useHitlInChat.ts index f96a273fde..5390e69b78 100644 --- a/packages/plugin-chatbot/src/useHitlInChat.ts +++ b/packages/plugin-chatbot/src/useHitlInChat.ts @@ -59,6 +59,19 @@ export interface UseHitlInChatOptions { * Optional callback fired after a decision completes (regardless of * success/failure). Useful for refreshing the inbox view if it is also * mounted on the same page. + * + * `outcome` is the spec decision response as the endpoint returned it, so + * read it by side: `result` / `error` on approve, `id` on reject + * (objectui#3783 — the previous local types promised `id` on BOTH, and the + * approve response has never carried one). `pendingActionId` is available + * from `ContinueContext` and from the row you decided on; do not fish it out + * of an approve outcome. + * + * One honest caveat: on a transport error or non-2xx there IS no decision + * response, and this callback is still invoked — with an envelope the hook + * synthesizes locally (`{ status: 'failed', error }`). That case is not + * modelled by this parameter's type; narrowing it is a public-contract + * decision tracked in objectui#3790. */ onDecided?: (toolCallId: string, outcome: ApproveOutcome | RejectOutcome) => void; /** @@ -80,7 +93,12 @@ export interface ContinueContext { toolCallId: string; pendingActionId: string; decision: 'approved' | 'rejected'; - /** Raw REST payload — `{ status, result?, error?, … }`. */ + /** + * Raw REST payload — the spec decision response: `{ status: 'executed' | + * 'failed', result?, error? }` on approve, `{ status: 'rejected', id }` on + * reject (objectui#3783). Note `id` is on the REJECT side only; use + * `pendingActionId` above, which is populated for both decisions. + */ outcome: ApproveOutcome | RejectOutcome; /** Tool name as it appeared in the message part (e.g. `action_delete_task`). */ toolName?: string; @@ -206,6 +224,14 @@ export function useHitlInChat(options: UseHitlInChatOptions): UseHitlInChatRetur ? payload.error : `Approval failed: HTTP ${response.status}`; setDecision(toolCallId, { state: 'error', message }); + // Not a wire response: on a non-2xx there IS no decision response, so + // this envelope is synthesized locally to notify `onDecided`. The + // `id` stays on it deliberately — it has been part of what consumers + // receive on this path since the callback shipped, and objectui#3783 + // is a type-only correction (`ApproveOutcome` no longer DECLARES + // `id`, because the approve wire never carried one). What the + // callback SHOULD be handed on transport/HTTP failure is a public + // contract question, filed as objectui#3790, not settled here. onDecided?.(toolCallId, { id, status: 'failed', diff --git a/packages/plugin-chatbot/src/usePendingActions.ts b/packages/plugin-chatbot/src/usePendingActions.ts index a5edfedeaf..8a03a0c02f 100644 --- a/packages/plugin-chatbot/src/usePendingActions.ts +++ b/packages/plugin-chatbot/src/usePendingActions.ts @@ -24,6 +24,10 @@ import * as React from 'react'; +import type { + ApproveAiPendingActionResponse, + RejectAiPendingActionResponse, +} from '@objectstack/spec/api'; import type { PendingActionRow, PendingActionStatus, @@ -58,26 +62,46 @@ import type { export type { PendingActionRow, PendingActionStatus }; /** - * Successful approval outcome returned by - * `POST /api/v1/ai/pending-actions/:id/approve`. + * The two decision responses — + * `POST /api/v1/ai/pending-actions/:id/approve` and `…/reject` — THE spec + * types, re-exported under this package's published names (objectui#3783). + * + * `@objectstack/spec/api` declares both (`ApproveAiPendingActionResponseSchema` + * / `RejectAiPendingActionResponseSchema` in `api/protocol.zod.ts`), and those + * are the same schemas `@objectstack/client`'s `ai.pendingActions.approve()` / + * `.reject()` type their return values with — so what is re-exported here IS + * the wire, not a second reading of it. The local names stay `ApproveOutcome` / + * `RejectOutcome` because they are this package's public API surface + * (`src/index.tsx`); only the shapes change. + * + * `status: 'failed'` is an HTTP **200** carrying a reason, not a 5xx: the + * approval succeeded, the execution did not (see the doc comment on + * `ApproveAiPendingActionResponseSchema`). The comment that used to sit here + * claimed 500, which contradicted both the spec and this hook's own design — + * `call()` throws on `!res.ok`, so a 500 could never reach the resolved-value + * path `AiPendingActionsInbox` reads `out.error` from. * - * The HTTP status is 200 when `status === 'executed'` and 500 when the - * downstream dispatcher failed (`status === 'failed'`). The hook surfaces - * both as a normal resolved value so the UI can show the error inline - * without throwing. + * The copies this replaces were hand transcriptions and had drifted three ways. + * Because the local names are NOT the spec's names, + * `scripts/check-spec-symbol-derivation.mjs` — which fires when a local + * declaration OCCUPIES a spec export name — had no handle on them at all; + * renaming a hand copy is invisible to a name-based guard: + * + * - `ApproveOutcome.id: string`, REQUIRED here and absent from the approve + * response — `id` is on the REJECT side. This drift was not dormant: the + * public `onDecided` callback (`useHitlInChat`) promised consumers a + * `string` and handed them `undefined` at runtime, with no compiler + * complaint anywhere; + * - `status: 'executed' | 'failed' | string` — a union with `string` ABSORBS + * the literals, so the annotation carried no information at all. The same + * drift #3220 removed from the row above; + * - `[k: string]: unknown` — the objectstack#4075 mechanism: an index + * signature makes any structural comparison against the spec answer + * "identical" however far the copy has drifted, so a parity test bolted onto + * the copy would have been green from its first day. */ -export interface ApproveOutcome { - id: string; - status: 'executed' | 'failed' | string; - result?: unknown; - error?: string; - [k: string]: unknown; -} - -export interface RejectOutcome { - id: string; - status: 'rejected' | string; -} +export type ApproveOutcome = ApproveAiPendingActionResponse; +export type RejectOutcome = RejectAiPendingActionResponse; export interface UsePendingActionsOptions { /**