Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 43 additions & 0 deletions .changeset/hitl-decision-outcomes-spec-derived-3783.md
Original file line numberDiff line numberDiff line change
@@ -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.
Original file line numberDiff line numberDiff line change
Expand Up@@ -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';
Expand All@@ -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<string> {
Expand DownExpand Up@@ -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<Equal<IsAny<ApproveAiPendingActionResponse>, false>>;
type _ApproveNotUnknown = Assert<Equal<IsUnknown<ApproveAiPendingActionResponse>, false>>;
type _RejectNotAny = Assert<Equal<IsAny<RejectAiPendingActionResponse>, false>>;

type _ApproveIsSpec = Assert<Equal<ApproveOutcome, ApproveAiPendingActionResponse>>;
type _RejectIsSpec = Assert<Equal<RejectOutcome, RejectAiPendingActionResponse>>;

// 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<Equal<'id' extends keyof ApproveOutcome ? true : false, false>>;
type _RejectHasId = Assert<Equal<RejectOutcome['id'], string>>;

// 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<string extends ApproveOutcome['status'] ? true : false, false>
>;
type _RejectStatusNotString = Assert<
Equal<string extends RejectOutcome['status'] ? true : false, false>
>;
type _ApproveVocabulary = Assert<Equal<ApproveOutcome['status'], 'executed' | 'failed'>>;
type _RejectVocabulary = Assert<Equal<RejectOutcome['status'], 'rejected'>>;

// 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<Equal<HasIndexSignature<ApproveOutcome>, false>>;
type _RejectNoIndexSignature = Assert<Equal<HasIndexSignature<RejectOutcome>, 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<Equal<…>>` 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);
}
});
});
121 changes: 118 additions & 3 deletions packages/plugin-chatbot/src/__tests__/useHitlInChat.test.tsx
Original file line numberDiff line numberDiff line change
Expand Up@@ -31,8 +31,12 @@ describe('useHitlInChat', () => {
it('fires continueConversation with an executed-outcome prompt on approve', async () => {
(globalThis.fetch as ReturnType<typeof vi.fn>).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' },
}),
Expand DownExpand Up@@ -93,7 +97,7 @@ describe('useHitlInChat', () => {
it('does NOT continue when execution failed', async () => {
(globalThis.fetch as ReturnType<typeof vi.fn>).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();
Expand All@@ -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<typeof vi.fn>).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<typeof vi.fn>).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<typeof vi.fn>).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<string, unknown>`, 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<typeof vi.fn>).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<typeof vi.fn>).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(() =>
Expand Down
28 changes: 27 additions & 1 deletion packages/plugin-chatbot/src/useHitlInChat.ts
Original file line numberDiff line numberDiff line change
Expand Up@@ -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;
/**
Expand All@@ -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;
Expand DownExpand Up@@ -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',
Expand Down
Loading
Loading
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 43 additions & 0 deletions .changeset/hitl-decision-outcomes-spec-derived-3783.md
Original file line numberDiff line numberDiff line change
@@ -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.
Original file line numberDiff line numberDiff line change
Expand Up@@ -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';
Expand All@@ -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<string> {
Expand DownExpand Up@@ -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<Equal<IsAny<ApproveAiPendingActionResponse>, false>>;
type _ApproveNotUnknown = Assert<Equal<IsUnknown<ApproveAiPendingActionResponse>, false>>;
type _RejectNotAny = Assert<Equal<IsAny<RejectAiPendingActionResponse>, false>>;

type _ApproveIsSpec = Assert<Equal<ApproveOutcome, ApproveAiPendingActionResponse>>;
type _RejectIsSpec = Assert<Equal<RejectOutcome, RejectAiPendingActionResponse>>;

// 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<Equal<'id' extends keyof ApproveOutcome ? true : false, false>>;
type _RejectHasId = Assert<Equal<RejectOutcome['id'], string>>;

// 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<string extends ApproveOutcome['status'] ? true : false, false>
>;
type _RejectStatusNotString = Assert<
Equal<string extends RejectOutcome['status'] ? true : false, false>
>;
type _ApproveVocabulary = Assert<Equal<ApproveOutcome['status'], 'executed' | 'failed'>>;
type _RejectVocabulary = Assert<Equal<RejectOutcome['status'], 'rejected'>>;

// 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<Equal<HasIndexSignature<ApproveOutcome>, false>>;
type _RejectNoIndexSignature = Assert<Equal<HasIndexSignature<RejectOutcome>, 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<Equal<…>>` 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);
}
});
});
121 changes: 118 additions & 3 deletions packages/plugin-chatbot/src/__tests__/useHitlInChat.test.tsx
Original file line numberDiff line numberDiff line change
Expand Up@@ -31,8 +31,12 @@ describe('useHitlInChat', () => {
it('fires continueConversation with an executed-outcome prompt on approve', async () => {
(globalThis.fetch as ReturnType<typeof vi.fn>).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' },
}),
Expand DownExpand Up@@ -93,7 +97,7 @@ describe('useHitlInChat', () => {
it('does NOT continue when execution failed', async () => {
(globalThis.fetch as ReturnType<typeof vi.fn>).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();
Expand All@@ -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<typeof vi.fn>).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<typeof vi.fn>).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<typeof vi.fn>).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<string, unknown>`, 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<typeof vi.fn>).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<typeof vi.fn>).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(() =>
Expand Down
28 changes: 27 additions & 1 deletion packages/plugin-chatbot/src/useHitlInChat.ts
Original file line numberDiff line numberDiff line change
Expand Up@@ -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;
/**
Expand All@@ -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;
Expand DownExpand Up@@ -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',
Expand Down
Loading
Loading
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix"); } } catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 43 additions & 0 deletions .changeset/hitl-decision-outcomes-spec-derived-3783.md
Original file line numberDiff line numberDiff line change
@@ -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.
Original file line numberDiff line numberDiff line change
Expand Up@@ -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';
Expand All@@ -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<string> {
Expand DownExpand Up@@ -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<Equal<IsAny<ApproveAiPendingActionResponse>, false>>;
type _ApproveNotUnknown = Assert<Equal<IsUnknown<ApproveAiPendingActionResponse>, false>>;
type _RejectNotAny = Assert<Equal<IsAny<RejectAiPendingActionResponse>, false>>;

type _ApproveIsSpec = Assert<Equal<ApproveOutcome, ApproveAiPendingActionResponse>>;
type _RejectIsSpec = Assert<Equal<RejectOutcome, RejectAiPendingActionResponse>>;

// 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<Equal<'id' extends keyof ApproveOutcome ? true : false, false>>;
type _RejectHasId = Assert<Equal<RejectOutcome['id'], string>>;

// 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<string extends ApproveOutcome['status'] ? true : false, false>
>;
type _RejectStatusNotString = Assert<
Equal<string extends RejectOutcome['status'] ? true : false, false>
>;
type _ApproveVocabulary = Assert<Equal<ApproveOutcome['status'], 'executed' | 'failed'>>;
type _RejectVocabulary = Assert<Equal<RejectOutcome['status'], 'rejected'>>;

// 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<Equal<HasIndexSignature<ApproveOutcome>, false>>;
type _RejectNoIndexSignature = Assert<Equal<HasIndexSignature<RejectOutcome>, 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<Equal<…>>` 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);
}
});
});
121 changes: 118 additions & 3 deletions packages/plugin-chatbot/src/__tests__/useHitlInChat.test.tsx
Original file line numberDiff line numberDiff line change
Expand Up@@ -31,8 +31,12 @@ describe('useHitlInChat', () => {
it('fires continueConversation with an executed-outcome prompt on approve', async () => {
(globalThis.fetch as ReturnType<typeof vi.fn>).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' },
}),
Expand DownExpand Up@@ -93,7 +97,7 @@ describe('useHitlInChat', () => {
it('does NOT continue when execution failed', async () => {
(globalThis.fetch as ReturnType<typeof vi.fn>).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();
Expand All@@ -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<typeof vi.fn>).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<typeof vi.fn>).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<typeof vi.fn>).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<string, unknown>`, 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<typeof vi.fn>).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<typeof vi.fn>).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(() =>
Expand Down
28 changes: 27 additions & 1 deletion packages/plugin-chatbot/src/useHitlInChat.ts
Original file line numberDiff line numberDiff line change
Expand Up@@ -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;
/**
Expand All@@ -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;
Expand DownExpand Up@@ -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',
Expand Down
Loading
Loading
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms"); } } catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 43 additions & 0 deletions .changeset/hitl-decision-outcomes-spec-derived-3783.md
Original file line numberDiff line numberDiff line change
@@ -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.
Original file line numberDiff line numberDiff line change
Expand Up@@ -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';
Expand All@@ -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<string> {
Expand DownExpand Up@@ -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<Equal<IsAny<ApproveAiPendingActionResponse>, false>>;
type _ApproveNotUnknown = Assert<Equal<IsUnknown<ApproveAiPendingActionResponse>, false>>;
type _RejectNotAny = Assert<Equal<IsAny<RejectAiPendingActionResponse>, false>>;

type _ApproveIsSpec = Assert<Equal<ApproveOutcome, ApproveAiPendingActionResponse>>;
type _RejectIsSpec = Assert<Equal<RejectOutcome, RejectAiPendingActionResponse>>;

// 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<Equal<'id' extends keyof ApproveOutcome ? true : false, false>>;
type _RejectHasId = Assert<Equal<RejectOutcome['id'], string>>;

// 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<string extends ApproveOutcome['status'] ? true : false, false>
>;
type _RejectStatusNotString = Assert<
Equal<string extends RejectOutcome['status'] ? true : false, false>
>;
type _ApproveVocabulary = Assert<Equal<ApproveOutcome['status'], 'executed' | 'failed'>>;
type _RejectVocabulary = Assert<Equal<RejectOutcome['status'], 'rejected'>>;

// 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<Equal<HasIndexSignature<ApproveOutcome>, false>>;
type _RejectNoIndexSignature = Assert<Equal<HasIndexSignature<RejectOutcome>, 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<Equal<…>>` 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);
}
});
});
121 changes: 118 additions & 3 deletions packages/plugin-chatbot/src/__tests__/useHitlInChat.test.tsx
Original file line numberDiff line numberDiff line change
Expand Up@@ -31,8 +31,12 @@ describe('useHitlInChat', () => {
it('fires continueConversation with an executed-outcome prompt on approve', async () => {
(globalThis.fetch as ReturnType<typeof vi.fn>).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' },
}),
Expand DownExpand Up@@ -93,7 +97,7 @@ describe('useHitlInChat', () => {
it('does NOT continue when execution failed', async () => {
(globalThis.fetch as ReturnType<typeof vi.fn>).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();
Expand All@@ -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<typeof vi.fn>).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<typeof vi.fn>).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<typeof vi.fn>).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<string, unknown>`, 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<typeof vi.fn>).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<typeof vi.fn>).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(() =>
Expand Down
28 changes: 27 additions & 1 deletion packages/plugin-chatbot/src/useHitlInChat.ts
Original file line numberDiff line numberDiff line change
Expand Up@@ -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;
/**
Expand All@@ -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;
Expand DownExpand Up@@ -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',
Expand Down
Loading
Loading
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links"); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + '
Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 43 additions & 0 deletions .changeset/hitl-decision-outcomes-spec-derived-3783.md
Original file line numberDiff line numberDiff line change
@@ -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.
Original file line numberDiff line numberDiff line change
Expand Up@@ -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';
Expand All@@ -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<string> {
Expand DownExpand Up@@ -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<Equal<IsAny<ApproveAiPendingActionResponse>, false>>;
type _ApproveNotUnknown = Assert<Equal<IsUnknown<ApproveAiPendingActionResponse>, false>>;
type _RejectNotAny = Assert<Equal<IsAny<RejectAiPendingActionResponse>, false>>;

type _ApproveIsSpec = Assert<Equal<ApproveOutcome, ApproveAiPendingActionResponse>>;
type _RejectIsSpec = Assert<Equal<RejectOutcome, RejectAiPendingActionResponse>>;

// 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<Equal<'id' extends keyof ApproveOutcome ? true : false, false>>;
type _RejectHasId = Assert<Equal<RejectOutcome['id'], string>>;

// 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<string extends ApproveOutcome['status'] ? true : false, false>
>;
type _RejectStatusNotString = Assert<
Equal<string extends RejectOutcome['status'] ? true : false, false>
>;
type _ApproveVocabulary = Assert<Equal<ApproveOutcome['status'], 'executed' | 'failed'>>;
type _RejectVocabulary = Assert<Equal<RejectOutcome['status'], 'rejected'>>;

// 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<Equal<HasIndexSignature<ApproveOutcome>, false>>;
type _RejectNoIndexSignature = Assert<Equal<HasIndexSignature<RejectOutcome>, 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<Equal<…>>` 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);
}
});
});
121 changes: 118 additions & 3 deletions packages/plugin-chatbot/src/__tests__/useHitlInChat.test.tsx
Original file line numberDiff line numberDiff line change
Expand Up@@ -31,8 +31,12 @@ describe('useHitlInChat', () => {
it('fires continueConversation with an executed-outcome prompt on approve', async () => {
(globalThis.fetch as ReturnType<typeof vi.fn>).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' },
}),
Expand DownExpand Up@@ -93,7 +97,7 @@ describe('useHitlInChat', () => {
it('does NOT continue when execution failed', async () => {
(globalThis.fetch as ReturnType<typeof vi.fn>).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();
Expand All@@ -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<typeof vi.fn>).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<typeof vi.fn>).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<typeof vi.fn>).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<string, unknown>`, 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<typeof vi.fn>).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<typeof vi.fn>).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(() =>
Expand Down
28 changes: 27 additions & 1 deletion packages/plugin-chatbot/src/useHitlInChat.ts
Original file line numberDiff line numberDiff line change
Expand Up@@ -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;
/**
Expand All@@ -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;
Expand DownExpand Up@@ -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',
Expand Down
Loading
Loading
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default"); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 43 additions & 0 deletions .changeset/hitl-decision-outcomes-spec-derived-3783.md
Original file line numberDiff line numberDiff line change
@@ -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.
Original file line numberDiff line numberDiff line change
Expand Up@@ -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';
Expand All@@ -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<string> {
Expand DownExpand Up@@ -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<Equal<IsAny<ApproveAiPendingActionResponse>, false>>;
type _ApproveNotUnknown = Assert<Equal<IsUnknown<ApproveAiPendingActionResponse>, false>>;
type _RejectNotAny = Assert<Equal<IsAny<RejectAiPendingActionResponse>, false>>;

type _ApproveIsSpec = Assert<Equal<ApproveOutcome, ApproveAiPendingActionResponse>>;
type _RejectIsSpec = Assert<Equal<RejectOutcome, RejectAiPendingActionResponse>>;

// 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<Equal<'id' extends keyof ApproveOutcome ? true : false, false>>;
type _RejectHasId = Assert<Equal<RejectOutcome['id'], string>>;

// 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<string extends ApproveOutcome['status'] ? true : false, false>
>;
type _RejectStatusNotString = Assert<
Equal<string extends RejectOutcome['status'] ? true : false, false>
>;
type _ApproveVocabulary = Assert<Equal<ApproveOutcome['status'], 'executed' | 'failed'>>;
type _RejectVocabulary = Assert<Equal<RejectOutcome['status'], 'rejected'>>;

// 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<Equal<HasIndexSignature<ApproveOutcome>, false>>;
type _RejectNoIndexSignature = Assert<Equal<HasIndexSignature<RejectOutcome>, 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<Equal<…>>` 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);
}
});
});
121 changes: 118 additions & 3 deletions packages/plugin-chatbot/src/__tests__/useHitlInChat.test.tsx
Original file line numberDiff line numberDiff line change
Expand Up@@ -31,8 +31,12 @@ describe('useHitlInChat', () => {
it('fires continueConversation with an executed-outcome prompt on approve', async () => {
(globalThis.fetch as ReturnType<typeof vi.fn>).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' },
}),
Expand DownExpand Up@@ -93,7 +97,7 @@ describe('useHitlInChat', () => {
it('does NOT continue when execution failed', async () => {
(globalThis.fetch as ReturnType<typeof vi.fn>).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();
Expand All@@ -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<typeof vi.fn>).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<typeof vi.fn>).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<typeof vi.fn>).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<string, unknown>`, 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<typeof vi.fn>).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<typeof vi.fn>).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(() =>
Expand Down
28 changes: 27 additions & 1 deletion packages/plugin-chatbot/src/useHitlInChat.ts
Original file line numberDiff line numberDiff line change
Expand Up@@ -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;
/**
Expand All@@ -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;
Expand DownExpand Up@@ -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',
Expand Down
Loading
Loading
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers"); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 43 additions & 0 deletions .changeset/hitl-decision-outcomes-spec-derived-3783.md
Original file line numberDiff line numberDiff line change
@@ -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.
Original file line numberDiff line numberDiff line change
Expand Up@@ -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';
Expand All@@ -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<string> {
Expand DownExpand Up@@ -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<Equal<IsAny<ApproveAiPendingActionResponse>, false>>;
type _ApproveNotUnknown = Assert<Equal<IsUnknown<ApproveAiPendingActionResponse>, false>>;
type _RejectNotAny = Assert<Equal<IsAny<RejectAiPendingActionResponse>, false>>;

type _ApproveIsSpec = Assert<Equal<ApproveOutcome, ApproveAiPendingActionResponse>>;
type _RejectIsSpec = Assert<Equal<RejectOutcome, RejectAiPendingActionResponse>>;

// 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<Equal<'id' extends keyof ApproveOutcome ? true : false, false>>;
type _RejectHasId = Assert<Equal<RejectOutcome['id'], string>>;

// 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<string extends ApproveOutcome['status'] ? true : false, false>
>;
type _RejectStatusNotString = Assert<
Equal<string extends RejectOutcome['status'] ? true : false, false>
>;
type _ApproveVocabulary = Assert<Equal<ApproveOutcome['status'], 'executed' | 'failed'>>;
type _RejectVocabulary = Assert<Equal<RejectOutcome['status'], 'rejected'>>;

// 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<Equal<HasIndexSignature<ApproveOutcome>, false>>;
type _RejectNoIndexSignature = Assert<Equal<HasIndexSignature<RejectOutcome>, 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<Equal<…>>` 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);
}
});
});
121 changes: 118 additions & 3 deletions packages/plugin-chatbot/src/__tests__/useHitlInChat.test.tsx
Original file line numberDiff line numberDiff line change
Expand Up@@ -31,8 +31,12 @@ describe('useHitlInChat', () => {
it('fires continueConversation with an executed-outcome prompt on approve', async () => {
(globalThis.fetch as ReturnType<typeof vi.fn>).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' },
}),
Expand DownExpand Up@@ -93,7 +97,7 @@ describe('useHitlInChat', () => {
it('does NOT continue when execution failed', async () => {
(globalThis.fetch as ReturnType<typeof vi.fn>).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();
Expand All@@ -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<typeof vi.fn>).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<typeof vi.fn>).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<typeof vi.fn>).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<string, unknown>`, 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<typeof vi.fn>).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<typeof vi.fn>).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(() =>
Expand Down
28 changes: 27 additions & 1 deletion packages/plugin-chatbot/src/useHitlInChat.ts
Original file line numberDiff line numberDiff line change
Expand Up@@ -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;
/**
Expand All@@ -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;
Expand DownExpand Up@@ -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',
Expand Down
Loading
Loading
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode"); } } catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); } })(); })();
Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 43 additions & 0 deletions .changeset/hitl-decision-outcomes-spec-derived-3783.md
Original file line numberDiff line numberDiff line change
@@ -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.
Original file line numberDiff line numberDiff line change
Expand Up@@ -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';
Expand All@@ -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<string> {
Expand DownExpand Up@@ -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<Equal<IsAny<ApproveAiPendingActionResponse>, false>>;
type _ApproveNotUnknown = Assert<Equal<IsUnknown<ApproveAiPendingActionResponse>, false>>;
type _RejectNotAny = Assert<Equal<IsAny<RejectAiPendingActionResponse>, false>>;

type _ApproveIsSpec = Assert<Equal<ApproveOutcome, ApproveAiPendingActionResponse>>;
type _RejectIsSpec = Assert<Equal<RejectOutcome, RejectAiPendingActionResponse>>;

// 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<Equal<'id' extends keyof ApproveOutcome ? true : false, false>>;
type _RejectHasId = Assert<Equal<RejectOutcome['id'], string>>;

// 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<string extends ApproveOutcome['status'] ? true : false, false>
>;
type _RejectStatusNotString = Assert<
Equal<string extends RejectOutcome['status'] ? true : false, false>
>;
type _ApproveVocabulary = Assert<Equal<ApproveOutcome['status'], 'executed' | 'failed'>>;
type _RejectVocabulary = Assert<Equal<RejectOutcome['status'], 'rejected'>>;

// 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<Equal<HasIndexSignature<ApproveOutcome>, false>>;
type _RejectNoIndexSignature = Assert<Equal<HasIndexSignature<RejectOutcome>, 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<Equal<…>>` 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);
}
});
});
121 changes: 118 additions & 3 deletions packages/plugin-chatbot/src/__tests__/useHitlInChat.test.tsx
Original file line numberDiff line numberDiff line change
Expand Up@@ -31,8 +31,12 @@ describe('useHitlInChat', () => {
it('fires continueConversation with an executed-outcome prompt on approve', async () => {
(globalThis.fetch as ReturnType<typeof vi.fn>).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' },
}),
Expand DownExpand Up@@ -93,7 +97,7 @@ describe('useHitlInChat', () => {
it('does NOT continue when execution failed', async () => {
(globalThis.fetch as ReturnType<typeof vi.fn>).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();
Expand All@@ -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<typeof vi.fn>).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<typeof vi.fn>).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<typeof vi.fn>).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<string, unknown>`, 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<typeof vi.fn>).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<typeof vi.fn>).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(() =>
Expand Down
28 changes: 27 additions & 1 deletion packages/plugin-chatbot/src/useHitlInChat.ts
Original file line numberDiff line numberDiff line change
Expand Up@@ -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;
/**
Expand All@@ -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;
Expand DownExpand Up@@ -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',
Expand Down
Loading
Loading