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
30 changes: 25 additions & 5 deletions docs/design/conditions-system.md
Original file line number Diff line number Diff line change
Expand Up @@ -106,8 +106,25 @@ graceful-old-client posture the wire format already assumes).

The **dispatch driver** (`dispatch.ts`) turns a chosen action into a side
effect. The `pty` arm calls `sendInput`/`onSend` with the action's raw `data`.
The `custom` arm calls the resolver callback passed by the host surface; if no
resolver is provided it throws rather than silently dropping the action.
The `custom` arm calls the resolver callback passed by the host surface and
**reports every refusal** (#1070): the resolver's `{ ok: false, reason }` answer
is narrowed by `refusalOf`, turned into a sentence by `describeConditionRefusal`,
and handed to the `onRefused` reporter the host surface passes in. The driver
never picks a surface itself — it is shared by the app and the phone.

The two builders differ in ONE respect, and it is load-bearing:

- `makeDispatchFromOnSend` (views) **reports and returns**. Views call
`void dispatch(action)`, so a rejection would be an unhandled promise
rejection on top of a failure the user can already see in a toast. A missing
resolver here is the ordinary refusal `no-resolver`, with its own message
("answer it in the terminal instead") rather than a throw.
- `makeDispatch` (the sessionId-bound control-plane form) reports **and
rejects**. `sessions.conditionsReply` injects a resolver that turns
`{ ok: false }` into a `ControlError` and relies on that rejection to fail the
capability; swallowing it made a refused trust-dialog reply answer
`accepted: true` to an agent that then believed a folder was trusted when it
was not. A missing resolver there is still a throw.

Type-safety guardrail: the core stores **erased** views
(`ConditionView<string, unknown>`) in `Record<string, ConditionView>`. That
Expand Down Expand Up @@ -217,9 +234,12 @@ AskUserQuestionRow
→ claude.askUserQuestion.answer module resolver
```

The dispatch `custom` arm still throws if a surface does not pass a resolver.
That is intentional: a custom action without its named resolver would otherwise
look clickable while doing nothing.
A surface that does not pass a resolver never silently drops the action. On the
control plane (`makeDispatch`) that is a throw; in a view
(`makeDispatchFromOnSend`) it is the reported refusal `no-resolver`, which the
user sees as "This agent cannot receive that kind of answer. Answer it in the
terminal instead." Either way the click accounts for itself — a custom action
without its named resolver would otherwise look clickable while doing nothing.

## CRITICAL DESIGN NOTE — `detect()` is free-form

Expand Down
9 changes: 7 additions & 2 deletions src/main/remote/RemoteServer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -866,15 +866,20 @@ export class RemoteServer extends EventEmitter {
action:
| { kind: 'pty'; id: string; label: string; data: string }
| ConditionCustomAction,
): Promise<{ ok: boolean; error?: string; result?: unknown }> {
): Promise<{ ok: boolean; error?: string; result?: unknown; reason?: string; failedAtStep?: string }> {
if (action.kind === 'custom') {
// Custom actions go through the provider's own resolver, which
// reparses the live terminal and fails closed — the safety property
// lives there, not here.
const result = await this.deps.manager.resolveCondition(sessionId, action)
return result.ok
? { ok: true, result: result.state }
: { ok: false, error: `resolver: ${result.reason}` }
// The REASON travels as itself, not folded into an error string
// (#1099 review). The phone rebuilds a `ResolveConditionResult` from
// this reply and shares the app's wording for it; with only
// `resolver: <reason>` to work from it could reach exactly one of the
// five messages, and printed the internal token while doing it.
: { ok: false, error: `resolver: ${result.reason}`, reason: result.reason, ...(result.failedAtStep ? { failedAtStep: result.failedAtStep } : {}) }
}

// pty actions ARE raw keystrokes, so schema validation alone would be
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -94,12 +94,38 @@ describe('AskUserQuestionRow input routing', () => {
expect(fake.calls.some(c => c.method === 'resolveCondition')).toBe(false)
})

it('explains a refusal in the row instead of printing the wire token', async () => {
// The reviewer's exact example (#1099): this box read
// "Answer failed: option-not-found at select-option" — a log line shown to
// a person, naming an internal step and saying nothing about what to do.
// It is the same refusal the toast path describes, so it must read the
// same way.
const fake = createFakeSessionFeed()
fake.nextResolveConditionResult = {
ok: false,
reason: 'option-not-found',
failedAtStep: 'select-option',
}
renderRow(fake)
fireEvent.click(screen.getByRole('button', { name: /Option A/ }))

expect(await screen.findByText(
'That answer no longer matches what the agent is asking. Read the question again and answer it.',
)).toBeInTheDocument()
expect(screen.queryByText(/option-not-found/)).not.toBeInTheDocument()
expect(screen.queryByText(/select-option/)).not.toBeInTheDocument()
})

it('does not append an absent resolver step to an error message', async () => {
const fake = createFakeSessionFeed()
fake.nextResolveConditionResult = { ok: false, reason: 'no-resolver' }
renderRow(fake)
fireEvent.click(screen.getByRole('button', { name: /Option A/ }))
expect(await screen.findByText('Answer failed: no-resolver')).toBeInTheDocument()
// The sentence, not the wire token (#1099): this row used to print
// "Answer failed: option-not-found at select-option" verbatim.
expect(await screen.findByText(
'This agent cannot receive that kind of answer. Answer it in the terminal instead.',
)).toBeInTheDocument()
expect(screen.queryByText(/undefined/)).not.toBeInTheDocument()
})
})
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import { useSessionFeed } from '@renderer/features/sessionFeed/SessionFeedContex
import { useGlobalToast } from '@renderer/ui/GlobalToast'
import { MarkerRow } from '@renderer/features/feed/ui/MarkerRow'
import type { ConditionCustomAction } from '@shared/types/providerConditions'
import { describeConditionRefusal, refusalOf } from '@shared/conditions-core/dispatch'
import {
readAskQuestions,
type AskOption,
Expand Down Expand Up @@ -206,19 +207,21 @@ export function AskUserQuestionRow({
// "option not found" without corrupting the terminal, so keeping the row
// interactive is the safer failure mode.
endAnswer(operationId)
// WHY failedAtStep is optional: transport-level refusals can explain
// the failure without entering the multi-step TUI driver. Appending
// an absent step produced the user-facing nonsense “at undefined”.
setResolveError(
result.failedAtStep === undefined
? result.reason
: `${result.reason} at ${result.failedAtStep}`,
)
// WHY the shared describer and not `result.reason` (#1099 review):
// this row used to print the wire token verbatim — "option-not-found
// at select-option" — which is a log line, not a sentence, and told
// the reader nothing about what to do next. `refusalOf` narrows the
// same answer the toast path narrows, so the row and the toast now
// say the identical thing for the identical refusal. The `?? `
// fallback is unreachable (we are inside `!result.ok`) but costs
// nothing and keeps the box from going blank if that ever changes.
const refusal = refusalOf(action, result)
setResolveError(refusal ? describeConditionRefusal(refusal) : 'That answer could not be delivered. Try again.')
}
})
.catch(() => {
endAnswer(operationId)
setResolveError('resolver IPC failed')
setResolveError('That answer could not be delivered. Try again.')
})
}

Expand Down Expand Up @@ -511,9 +514,15 @@ export function AskUserQuestionRow({
) : answering ? (
<div className="text-[11px] text-muted italic">Answering…</div>
) : null}
{/* No "Answer failed:" prefix any more: every message that reaches
here is now a complete sentence (from `describeConditionRefusal`,
from `deliverAnswersViaPrompt`, or the pick-something-first hint),
and the danger styling already carries the "this failed" framing.
The prefix only survived because the old messages were bare tokens
that made no sense on their own. */}
{resolveError ? (
<div className="rounded-slab border border-danger-border bg-danger-soft px-2 py-1 text-[11px] text-danger">
Answer failed: {resolveError}
{resolveError}
</div>
) : null}
</div>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
import type { ProviderConditionSnapshot } from '@shared/types/providerConditions'
import { ConditionOutlet } from '@shared/conditions-core/ConditionOutlet'
import { makeDispatchFromOnSend } from '@shared/conditions-core/dispatch'
import type { ConditionRefusalReporter } from '@shared/conditions-core/dispatch'
import type { ConditionCustomAction } from '@shared/conditions-core/contract'
import { getRendererProviderCapabilities } from '@providers/registry.renderer.capabilities'
import { observeRenderShape } from '@renderer/features/feed/evidence/observer'
Expand All @@ -37,6 +38,10 @@ type Props = {
conditions: ProviderConditionSnapshot | null
onSend: (data: string) => Promise<void>
onResolveCustom?: (action: ConditionCustomAction) => Promise<unknown>
/** Where a REFUSED custom action is told to the user (#1070). Optional so a
* surface that has nowhere to put it still renders; without one the refusal
* is silent, which is the bug. */
onConditionRefused?: ConditionRefusalReporter
interactionActive: boolean
}

Expand Down Expand Up @@ -73,13 +78,14 @@ export function ProviderConditionOutlet({
conditions,
onSend,
onResolveCustom,
onConditionRefused,
interactionActive,
}: Props) {
if (!conditions) return null

const capabilities = getRendererProviderCapabilities(conditions.provider)
const { conditionViews: registry, conditionPolicy } = capabilities
const dispatch = makeDispatchFromOnSend(onSend, onResolveCustom)
const dispatch = makeDispatchFromOnSend(onSend, onResolveCustom, onConditionRefused)

for (const [kind, condition] of Object.entries(conditions.conditions)) {
if (!condition) continue
Expand Down
Loading
Loading