Skip to content

Commit bdeea33

Browse files
committed
chore(providers): trim comments to the non-obvious why
1 parent 7be26e7 commit bdeea33

6 files changed

Lines changed: 122 additions & 80 deletions

File tree

.claude/rules/sim-ui-copy.md

Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,46 @@
1+
---
2+
paths:
3+
- "apps/sim/**/*.tsx"
4+
- "apps/sim/components/emcn/**"
5+
---
6+
7+
# UI Copy
8+
9+
**Do not add subtitles, helper text, or descriptive copy beneath headings, labels, cards, or settings by default.** Prefer one concise, self-explanatory heading or label. Only add supporting copy when the user explicitly asks for it, or when it is necessary to prevent misunderstanding or error — and never use it to restate the heading.
10+
11+
This applies to product surfaces: settings rows, modals, panels, cards, list rows, empty states, form fields, and section headers. Marketing surfaces (`app/(landing)`, docs) are governed by `constitution.md` instead.
12+
13+
**Carve-out — settings section metadata.** `SettingsNavigationItem.description` in `components/settings/navigation.ts` stays required, and `SettingsPanel` keeps rendering it as the page subtitle. Settings sections are reached through a nav list where the description is the only thing distinguishing adjacent sections, so it earns its place by the "prevents misunderstanding" test. Keep those descriptions verb-first and one line, per `sim-settings-pages.md`. Everything else on a settings page — inline `<p>` blurbs under section headings, field hints, modal bodies, row subtitles — follows the default rule above.
14+
15+
## The default is no description
16+
17+
```tsx
18+
// ✗ Bad — the subtitle restates the heading
19+
<h3>API Keys</h3>
20+
<p className='text-[var(--text-muted)] text-caption'>Manage your API keys.</p>
21+
22+
// ✗ Bad — decorative filler under a field label
23+
<ChipModalField title='Workspace name' hint='The name of your workspace.' />
24+
25+
// ✓ Good — the label carries the whole meaning
26+
<h3>API Keys</h3>
27+
<ChipModalField title='Workspace name' />
28+
```
29+
30+
If a heading needs a subtitle to be understood, the heading is wrong. Fix the heading — don't append a second line.
31+
32+
## When supporting copy earns its place
33+
34+
Keep (or add) a description only when it carries information the label cannot, and its absence would cause a mistake:
35+
36+
- **Irreversible or destructive consequences** — "Deleting this workspace removes every workflow and log. This cannot be undone."
37+
- **A non-obvious format, unit, or bound** — "Comma-separated. Max 50 domains.", "Cost per 1M input tokens."
38+
- **A security or access implication** — "This key is shown once and grants full workspace access."
39+
- **A state the user cannot otherwise see** — "Inherited from your organization's policy."
40+
- **Instructional copy that advances a flow** — "We sent a 6-digit code to you@example.com."
41+
42+
Everything else — restatements, "Manage your X", "Configure your Y", feature blurbs, encouragement — gets deleted.
43+
44+
## Component APIs
45+
46+
Description/hint slots on shared components are **optional**, never required, and must reserve no layout space when omitted. A component that forces every consumer to supply a subtitle forces every consumer to violate this rule. When adding a new shared component, ship it without a description slot and add one only once a real caller meets the bar above.

.cursor/rules/sim-ui-copy.mdc

Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,44 @@
1+
---
2+
description: UI copy conventions — no default subtitles or helper text under headings, labels, cards, or settings
3+
globs: ["apps/sim/**/*.tsx"]
4+
---
5+
# UI Copy
6+
7+
**Do not add subtitles, helper text, or descriptive copy beneath headings, labels, cards, or settings by default.** Prefer one concise, self-explanatory heading or label. Only add supporting copy when the user explicitly asks for it, or when it is necessary to prevent misunderstanding or error — and never use it to restate the heading.
8+
9+
This applies to product surfaces: settings rows, modals, panels, cards, list rows, empty states, form fields, and section headers. Marketing surfaces (`app/(landing)`, docs) are governed by `constitution.mdc` instead.
10+
11+
**Carve-out — settings section metadata.** `SettingsNavigationItem.description` in `components/settings/navigation.ts` stays required, and `SettingsPanel` keeps rendering it as the page subtitle. Settings sections are reached through a nav list where the description is the only thing distinguishing adjacent sections. Everything else on a settings page — inline `<p>` blurbs under section headings, field hints, modal bodies, row subtitles — follows the default rule above.
12+
13+
## The default is no description
14+
15+
```tsx
16+
// ✗ Bad — the subtitle restates the heading
17+
<h3>API Keys</h3>
18+
<p className='text-[var(--text-muted)] text-caption'>Manage your API keys.</p>
19+
20+
// ✗ Bad — decorative filler under a field label
21+
<ChipModalField title='Workspace name' hint='The name of your workspace.' />
22+
23+
// ✓ Good — the label carries the whole meaning
24+
<h3>API Keys</h3>
25+
<ChipModalField title='Workspace name' />
26+
```
27+
28+
If a heading needs a subtitle to be understood, the heading is wrong. Fix the heading — don't append a second line.
29+
30+
## When supporting copy earns its place
31+
32+
Keep (or add) a description only when it carries information the label cannot, and its absence would cause a mistake:
33+
34+
- **Irreversible or destructive consequences** — "Deleting this workspace removes every workflow and log. This cannot be undone."
35+
- **A non-obvious format, unit, or bound** — "Comma-separated. Max 50 domains.", "Cost per 1M input tokens."
36+
- **A security or access implication** — "This key is shown once and grants full workspace access."
37+
- **A state the user cannot otherwise see** — "Inherited from your organization's policy."
38+
- **Instructional copy that advances a flow** — "We sent a 6-digit code to you@example.com."
39+
40+
Everything else — restatements, "Manage your X", "Configure your Y", feature blurbs, encouragement — gets deleted.
41+
42+
## Component APIs
43+
44+
Description/hint slots on shared components are **optional**, never required, and must reserve no layout space when omitted. A component that forces every consumer to supply a subtitle forces every consumer to violate this rule. When adding a new shared component, ship it without a description slot and add one only once a real caller meets the bar above.

apps/sim/executor/handlers/agent/agent-handler.ts

Lines changed: 3 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1316,14 +1316,9 @@ export class AgentBlockHandler implements BlockHandler {
13161316
})
13171317

13181318
/**
1319-
* `TimeoutError` is what the runtime raises on a fetch deadline; without it a
1320-
* stalled model call reached the trace as the bare runtime string.
1321-
*
1322-
* The cause chain is walked, not just `name`: providers rewrap transport failures in
1323-
* a `ProviderError`, which overwrites `name`, so the classification only survives on
1324-
* `cause`. The original message is kept rather than replaced — providers annotate it
1325-
* with the request phase they died in, and that detail is the only thing separating a
1326-
* request that was never answered from one whose body stalled.
1319+
* The original message is appended rather than replaced: providers annotate it with
1320+
* the request phase they died in, which is the only thing separating a request that
1321+
* was never answered from one whose body stalled.
13271322
*/
13281323
if (isTransportTimeout(error)) {
13291324
throw new Error(

apps/sim/providers/openai/core.response-status.test.ts

Lines changed: 4 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -1,14 +1,8 @@
11
/**
22
* @vitest-environment node
33
*
4-
* `/v1/responses` answers HTTP 200 for generations that did not succeed — `status:
5-
* 'failed'` with a populated `error`, or `status: 'incomplete'` with a reason. The
6-
* non-streaming path read only `output`, so those reached the user as a success with
7-
* empty content and billed tokens, while the trace span independently recorded
8-
* `finishReason: 'error'`.
9-
*
10-
* These cover the status/error gate and pin the `incomplete` policy to the one the
11-
* streaming loop already applies, so the two paths cannot silently diverge again.
4+
* Pins the non-streaming status/error gate, and pins its `incomplete` policy to the one
5+
* `streamResponsesTurn` applies so the two paths cannot silently diverge.
126
*/
137
import { beforeEach, describe, expect, it, vi } from 'vitest'
148
import { executeResponsesProviderRequest } from '@/providers/openai/core'
@@ -128,11 +122,7 @@ describe('OpenAI non-streaming response status handling', () => {
128122
await expect(run(fetchMock)).rejects.toThrow('Upstream provider rejected the request.')
129123
})
130124

131-
/**
132-
* Decision, matching `streamResponsesTurn`: an `incomplete` response truncated by
133-
* `max_output_tokens` with no tool call is NOT an error — the partial prose is a
134-
* usable answer and is returned as the block content.
135-
*/
125+
/** Policy is shared with `streamResponsesTurn` — keep both in step. */
136126
it('returns the partial content of a max_output_tokens incomplete response instead of failing', async () => {
137127
const fetchMock = vi.fn().mockResolvedValue(
138128
jsonResponse({
@@ -149,10 +139,6 @@ describe('OpenAI non-streaming response status handling', () => {
149139
expect(result.content).toBe('a truncated but usable answer')
150140
})
151141

152-
/**
153-
* The other half of the same decision: every other incomplete reason is an error,
154-
* because the generation stopped for a reason the caller must be told about.
155-
*/
156142
it('fails the block on an incomplete response whose reason is not max_output_tokens', async () => {
157143
const fetchMock = vi.fn().mockResolvedValue(
158144
jsonResponse({
@@ -224,10 +210,7 @@ describe('OpenAI non-streaming response status handling', () => {
224210
expect(result.tokens?.total).toBe(4)
225211
})
226212

227-
/**
228-
* The gate sits in `postResponses`, so it must cover continuation turns too — a loop
229-
* that starts healthy and fails on turn two must still fail the block.
230-
*/
213+
/** The gate lives in `postResponses`, so continuation turns are covered too. */
231214
it('fails the block when a later tool-loop turn comes back failed', async () => {
232215
const fetchMock = vi
233216
.fn()

apps/sim/providers/openai/core.transport-phase.test.ts

Lines changed: 3 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,8 @@
11
/**
22
* @vitest-environment node
33
*
4-
* A stalled model call surfaces only the runtime's own `TimeoutError: The operation
5-
* timed out.`, which cannot distinguish "still generating, never answered" from
6-
* "answered, but the body never arrived" — opposite owners, opposite fixes. These
7-
* cover the phase annotation that makes the distinction readable from the execution
8-
* trace, which survives even when a task has stopped shipping logs.
4+
* Covers the phase annotation that separates "never answered" from "answered, but the
5+
* body never arrived" — the runtime reports both as a bare `TimeoutError`.
96
*/
107
import { beforeEach, describe, expect, it, vi } from 'vitest'
118
import { executeResponsesProviderRequest } from '@/providers/openai/core'
@@ -150,8 +147,7 @@ describe('OpenAI transport phase annotation', () => {
150147
* that recovery path for any provider whose error message runs long.
151148
*/
152149
it('does not truncate a structured provider error, so the summary fallback still matches', async () => {
153-
// Markers deliberately placed beyond the 500-char bound so that truncating a
154-
// structured error would drop them and the fallback would stop matching.
150+
// Marker sits past the 500-char bound, so truncation would break the fallback match.
155151
const longMessage = `${'context detail. '.repeat(40)}Invalid value for reasoning.summary: your organization must be verified to use this feature.`
156152
expect(longMessage.indexOf('reasoning.summary')).toBeGreaterThan(500)
157153

@@ -175,7 +171,6 @@ describe('OpenAI transport phase annotation', () => {
175171
mockSupportsReasoningEffort.mockReturnValue(true)
176172

177173
await expect(run(fetchMock, { agentEvents: true })).resolves.toMatchObject({ content: 'ok' })
178-
// Matched the verification error and retried without the summary, rather than failing.
179174
expect(fetchMock).toHaveBeenCalledTimes(2)
180175
})
181176

apps/sim/providers/openai/core.ts

Lines changed: 22 additions & 43 deletions
Original file line numberDiff line numberDiff line change
@@ -43,25 +43,16 @@ import {
4343
} from './utils'
4444

4545
/**
46-
* Rejects a `/v1/responses` body that reports a generation which did not succeed.
46+
* Rejects a `/v1/responses` body reporting a generation that did not succeed — the
47+
* endpoint answers HTTP 200 for both `status: 'failed'` and `status: 'incomplete'`.
4748
*
48-
* The endpoint answers HTTP 200 for failures: `status: 'failed'` with a populated
49-
* `error`, or `status: 'incomplete'` with an `incomplete_details.reason`. Reading only
50-
* `output` therefore reports a failed generation as a success with empty content and
51-
* billed tokens, while the trace independently records `finishReason: 'error'` — so the
52-
* block and its own span contradict each other. `@ai-sdk/openai` throws on the same
53-
* condition rather than returning empty content.
49+
* The tolerated case must stay matched to `streamResponsesTurn`: `incomplete` is accepted
50+
* only when truncated by `max_output_tokens` AND carrying no function call. Truncated
51+
* prose is a usable partial answer, but a truncated `function_call` holds half-written
52+
* JSON that makes `parseToolArguments` throw a confusing tool failure.
5453
*
55-
* The tolerated case is copied from `streamResponsesTurn` and must keep matching it: an
56-
* `incomplete` response is accepted only when it was truncated by `max_output_tokens`
57-
* AND carries no function call. Truncated prose is still a usable partial answer, but a
58-
* truncated `function_call` holds half-written JSON — executing it makes
59-
* `parseToolArguments` throw, surfacing a confusing tool failure rather than the
60-
* truncation that actually happened.
61-
*
62-
* A status the API did not send is not asserted against: this path is shared with Azure
63-
* OpenAI and OpenAI-compatible gateways, and inventing a failure for an absent field
64-
* would break healthy responses instead of reporting broken ones.
54+
* An absent `status` is deliberately not treated as a failure: this path is shared with
55+
* Azure OpenAI and OpenAI-compatible gateways.
6556
*/
6657
function assertUsableResponse(response: OpenAI.Responses.Response, providerLabel: string): void {
6758
if (response.error) {
@@ -141,10 +132,6 @@ export async function executeResponsesProviderRequest(
141132

142133
logger.info(`Preparing ${config.providerLabel} request`, {
143134
model: request.model,
144-
/**
145-
* Without these a provider call cannot be tied back to the execution that issued
146-
* it, which leaves a stalled request indistinguishable from one never made.
147-
*/
148135
workflowId: request.workflowId,
149136
blockId: request.blockId,
150137
executionId: request.executionId,
@@ -301,11 +288,10 @@ export async function executeResponsesProviderRequest(
301288
})
302289

303290
/**
304-
* A non-JSON body here is usually a gateway or CDN error page, and this string reaches
305-
* the user-facing block error and the trace span — so it is bounded rather than pasted
306-
* in whole, and falls back to `statusText` when the body carries nothing useful.
307-
* `@ai-sdk/provider-utils` likewise falls back to `statusText` and keeps the raw body
308-
* on a separate field rather than in the message.
291+
* A non-JSON body is usually a gateway or CDN error page and reaches the user-facing
292+
* block error, so it is bounded and falls back to `statusText`. A structured provider
293+
* message is returned untruncated on purpose: the reasoning-summary strip-and-retry
294+
* fallback matches on its text.
309295
*/
310296
const parseErrorResponse = async (response: Response): Promise<string> => {
311297
const text = await response.text().catch(() => '')
@@ -385,21 +371,15 @@ export async function executeResponsesProviderRequest(
385371
/**
386372
* Names the request phase an opaque transport failure died in.
387373
*
388-
* A stalled model call surfaces only the runtime's own message — under Bun, a
389-
* `TimeoutError: The operation timed out.` from its socket deadline — which cannot
390-
* distinguish "still generating, never answered" from "answered, but the body never
391-
* arrived". Those have opposite owners and opposite fixes. undici draws the same line
392-
* as two distinct error types (`UND_ERR_HEADERS_TIMEOUT` vs `UND_ERR_BODY_TIMEOUT`);
393-
* this records the equivalent for a runtime that reports neither.
394-
*
395-
* The phase rides the error message because that reaches the block's trace span, and
396-
* the trace survives even when a task has stopped shipping logs. `x-request-id` is
397-
* carried for the same reason the OpenAI SDK captures it: it is the only handle the
398-
* provider can trace a call by, and it is unavailable once the call has failed.
374+
* Bun raises only `TimeoutError: The operation timed out.`, which cannot distinguish
375+
* "never answered" from "answered, but the body never arrived" — opposite owners,
376+
* opposite fixes. undici splits these as `UND_ERR_HEADERS_TIMEOUT` vs
377+
* `UND_ERR_BODY_TIMEOUT`; this records the equivalent for a runtime that reports
378+
* neither.
399379
*
400-
* Errors that already describe themselves — an API error carrying a status and a
401-
* provider message — are left untouched; only `TimeoutError`/`AbortError`, which name
402-
* nothing, are annotated.
380+
* The phase rides the error message because that reaches the block's trace span, which
381+
* survives when a task has stopped shipping logs; `x-request-id` is the only handle the
382+
* provider can trace the call by. Self-describing API errors are left untouched.
403383
*/
404384
const annotateTransportFailure = (
405385
error: unknown,
@@ -467,9 +447,8 @@ export async function executeResponsesProviderRequest(
467447
}
468448

469449
/**
470-
* Asserted here rather than at the call sites so the first turn and every tool-loop
471-
* continuation are covered by construction, and outside the transport `try` so a
472-
* rejected generation is never mistaken for a transport failure.
450+
* Placed here so every tool-loop turn is covered, and outside the transport `try` so
451+
* a rejected generation is not misreported as a transport failure.
473452
*/
474453
assertUsableResponse(parsed, config.providerLabel)
475454
return parsed

0 commit comments

Comments
 (0)