mirror of
https://github.com/anomalyco/opencode.git
synced 2026-05-17 12:42:17 +00:00
fix(tui): show config error details on startup (#27803)
This commit is contained in:
parent
2385123f03
commit
5911bd532d
6 changed files with 185 additions and 18 deletions
|
|
@ -70,11 +70,51 @@ Endpoint definitions declare which public errors can be emitted. Public
|
|||
HTTP error schemas carry their response status with `httpApiStatus` or the
|
||||
equivalent HttpApi schema annotation.
|
||||
|
||||
Effect's own HttpApi examples follow this pattern:
|
||||
|
||||
```ts
|
||||
export class Unauthorized extends Schema.TaggedErrorClass<Unauthorized>()(
|
||||
"Unauthorized",
|
||||
{ message: Schema.String },
|
||||
{ httpApiStatus: 401 },
|
||||
) {}
|
||||
|
||||
export class Authorization extends HttpApiMiddleware.Service<Authorization, {
|
||||
provides: CurrentUser
|
||||
}>()("app/Authorization", {
|
||||
security: { bearer: HttpApiSecurity.bearer },
|
||||
error: Unauthorized,
|
||||
}) {}
|
||||
```
|
||||
|
||||
Endpoint-level errors use the same idea:
|
||||
|
||||
```ts
|
||||
export class ConfigApiError extends Schema.ErrorClass<ConfigApiError>("ConfigApiError")(
|
||||
{
|
||||
name: Schema.Union(Schema.Literal("ConfigInvalidError"), Schema.Literal("ConfigJsonError")),
|
||||
data: Schema.Struct({ message: Schema.optional(Schema.String), path: Schema.String }),
|
||||
},
|
||||
{ httpApiStatus: 400 },
|
||||
) {}
|
||||
|
||||
HttpApiEndpoint.get("get", "/config", {
|
||||
success: Config.Info,
|
||||
error: ConfigApiError,
|
||||
})
|
||||
```
|
||||
|
||||
The service error and HTTP error may be the same class only when the wire
|
||||
shape is intentionally public. Use separate HTTP error schemas when the
|
||||
service error contains internals, low-level causes, retry hints, or data
|
||||
that should not be exposed to API clients.
|
||||
|
||||
Do not map every domain error into one universal HTTP error class. Prefer a
|
||||
small public error vocabulary by route group: shared shapes like
|
||||
`ApiNotFoundError`, route-specific shapes like `ConfigApiError`, and built-in
|
||||
empty `HttpApiError.*` only when an empty/no-content body is the intended SDK
|
||||
contract.
|
||||
|
||||
## Mapping Guidance
|
||||
|
||||
- Keep one-off translations inline in the handler.
|
||||
|
|
@ -86,6 +126,35 @@ that should not be exposed to API clients.
|
|||
breaking API change.
|
||||
- Use built-in `HttpApiError.*` only when its generated body and SDK
|
||||
surface are intentionally the public contract.
|
||||
- Prefer `Schema.ErrorClass` for public HTTP error bodies whose wire shape is
|
||||
not the same as the internal domain error shape.
|
||||
- Prefer `Schema.TaggedErrorClass` for service/domain errors and middleware
|
||||
errors that are naturally tagged by `_tag`.
|
||||
- If preserving a legacy `{ name, data }` body, model that shape explicitly in
|
||||
the public API error schema instead of relying on `NamedError.toObject()` in
|
||||
generic middleware.
|
||||
|
||||
## User-Facing Rendering
|
||||
|
||||
HTTP serialization and user rendering are separate boundaries. The server
|
||||
should send structured public errors; CLI and TUI code should format those
|
||||
structures through one shared formatter.
|
||||
|
||||
For SDK calls using `{ throwOnError: true }`, the generated client may wrap the
|
||||
decoded response body in an `Error`. The original body should remain available
|
||||
under `error.cause.body`; `FormatError` is the right place to unwrap and render
|
||||
that body. TUI aggregation helpers should call `FormatError` first, then fall
|
||||
back to generic `Error.message` / string rendering.
|
||||
|
||||
When several parallel startup requests fail from the same underlying issue,
|
||||
group identical rendered messages and list the affected request names once.
|
||||
For example:
|
||||
|
||||
```text
|
||||
Configuration is invalid at /path/to/opencode.json
|
||||
↳ Expected object, got "not-object" provider.bad.options
|
||||
Affected startup requests: config.providers, provider.list, app.agents, config.get
|
||||
```
|
||||
|
||||
## Middleware Guidance
|
||||
|
||||
|
|
@ -99,6 +168,15 @@ middleware should shrink. It should not gain new name checks.
|
|||
Unknown `500` responses should log full details server-side with
|
||||
`Cause.pretty(cause)` and return a safe public body.
|
||||
|
||||
The config startup regression in #27056 is the failure mode this rule is meant
|
||||
to avoid: a user-authored invalid `opencode.json` crossed the HttpApi boundary
|
||||
as a defect, so middleware replaced a useful `ConfigInvalidError` with a safe
|
||||
generic `UnknownError`. The compatibility fix is to preserve config parse and
|
||||
validation errors as client-visible `400`s. The target architecture is better:
|
||||
config loading should fail on the typed error channel, config HTTP handlers
|
||||
should map those errors to declared `ConfigApiError` responses, and the generic
|
||||
middleware should never see them.
|
||||
|
||||
## Migration Order
|
||||
|
||||
Prefer small vertical slices:
|
||||
|
|
@ -113,6 +191,9 @@ Prefer small vertical slices:
|
|||
Good early domains are storage not-found, worktree errors, and provider
|
||||
auth validation errors because they currently drive HTTP behavior.
|
||||
|
||||
Config parse and validation errors are also a good early slice because they
|
||||
are startup-blocking and must be rendered clearly in both CLI and TUI flows.
|
||||
|
||||
## Checklist For A PR
|
||||
|
||||
- [ ] Expected failures are typed errors, not defects.
|
||||
|
|
|
|||
|
|
@ -1,3 +1,5 @@
|
|||
import { FormatError } from "@/cli/error"
|
||||
|
||||
/**
|
||||
* Aggregate Promise.allSettled results into a single Error that names every
|
||||
* failed endpoint, or return null when all fulfilled. Used at TUI bootstrap
|
||||
|
|
@ -15,7 +17,19 @@ export function aggregateFailures(labeled: LabeledSettled[]): Error | null {
|
|||
)
|
||||
if (failed.length === 0) return null
|
||||
|
||||
const reasons = failed.map((f) => `${f.name}: ${reasonMessage(f.result.reason)}`).join("; ")
|
||||
const reasons = Array.from(
|
||||
failed
|
||||
.map((f) => ({ name: f.name, message: reasonMessage(f.result.reason) }))
|
||||
.reduce((grouped, failure) => {
|
||||
grouped.set(failure.message, [...(grouped.get(failure.message) ?? []), failure.name])
|
||||
return grouped
|
||||
}, new Map<string, string[]>())
|
||||
.entries(),
|
||||
)
|
||||
.map(([message, names]) =>
|
||||
names.length === 1 ? `${names[0]}: ${message}` : `${message}\nAffected startup requests: ${names.join(", ")}`,
|
||||
)
|
||||
.join("; ")
|
||||
const summary = `${failed.length} of ${labeled.length} requests failed: ${reasons}`
|
||||
const err = new Error(summary)
|
||||
err.cause = { failures: failed.map((f) => ({ name: f.name, reason: f.result.reason })) }
|
||||
|
|
@ -23,6 +37,9 @@ export function aggregateFailures(labeled: LabeledSettled[]): Error | null {
|
|||
}
|
||||
|
||||
function reasonMessage(reason: unknown): string {
|
||||
const formatted = FormatError(reason)
|
||||
if (formatted) return formatted
|
||||
|
||||
if (reason instanceof Error) return reason.message
|
||||
if (typeof reason === "string") return reason
|
||||
if (reason && typeof reason === "object") {
|
||||
|
|
|
|||
|
|
@ -2,16 +2,9 @@ import { NamedError } from "@opencode-ai/core/util/error"
|
|||
import { errorFormat } from "@/util/error"
|
||||
import { isRecord } from "@/util/record"
|
||||
|
||||
interface ErrorLike {
|
||||
name?: string
|
||||
_tag?: string
|
||||
message?: string
|
||||
data?: Record<string, unknown>
|
||||
}
|
||||
|
||||
type ConfigIssue = { message: string; path: string[] }
|
||||
|
||||
function isTaggedError(error: unknown, tag: string): boolean {
|
||||
function isTaggedError(error: unknown, tag: string): error is Record<string, unknown> {
|
||||
return isRecord(error) && error._tag === tag
|
||||
}
|
||||
|
||||
|
|
@ -39,22 +32,27 @@ function configIssues(input: Record<string, unknown>): ConfigIssue[] {
|
|||
: []
|
||||
}
|
||||
|
||||
export function FormatError(input: unknown) {
|
||||
export function FormatError(input: unknown): string | undefined {
|
||||
if (input instanceof Error && isRecord(input.cause) && "body" in input.cause) {
|
||||
const formatted = FormatError(input.cause.body)
|
||||
if (formatted) return formatted
|
||||
}
|
||||
|
||||
// CliError: domain failure surfaced from an effectCmd handler via fail("...")
|
||||
if (isTaggedError(input, "CliError")) {
|
||||
const data = input as ErrorLike & { exitCode?: number }
|
||||
if (data.exitCode != null) process.exitCode = data.exitCode
|
||||
return data.message ?? ""
|
||||
if (typeof input.exitCode === "number") process.exitCode = input.exitCode
|
||||
return stringField(input, "message") ?? ""
|
||||
}
|
||||
|
||||
// MCPFailed: { name: string }
|
||||
if (NamedError.hasName(input, "MCPFailed")) {
|
||||
return `MCP server "${(input as ErrorLike).data?.name}" failed. Note, opencode does not support MCP authentication yet.`
|
||||
const data = isRecord(input) && isRecord(input.data) ? stringField(input.data, "name") : undefined
|
||||
return `MCP server "${data}" failed. Note, opencode does not support MCP authentication yet.`
|
||||
}
|
||||
|
||||
// AccountServiceError, AccountTransportError: TaggedErrorClass
|
||||
if (isTaggedError(input, "AccountServiceError") || isTaggedError(input, "AccountTransportError")) {
|
||||
return (input as ErrorLike).message ?? ""
|
||||
return stringField(input, "message") ?? ""
|
||||
}
|
||||
|
||||
// ProviderModelNotFoundError: { providerID: string, modelID: string, suggestions?: string[] }
|
||||
|
|
@ -64,7 +62,7 @@ export function FormatError(input: unknown) {
|
|||
? providerModelNotFound.suggestions.filter((x) => typeof x === "string")
|
||||
: []
|
||||
return [
|
||||
`Model not found: ${providerModelNotFound.providerID}/${providerModelNotFound.modelID}`,
|
||||
`Model not found: ${stringField(providerModelNotFound, "providerID")}/${stringField(providerModelNotFound, "modelID")}`,
|
||||
...(suggestions.length ? ["Did you mean: " + suggestions.join(", ")] : []),
|
||||
`Try: \`opencode models\` to list available models`,
|
||||
`Or check your config (opencode.json) provider/model names`,
|
||||
|
|
@ -112,6 +110,7 @@ export function FormatError(input: unknown) {
|
|||
if (isTaggedError(input, "UICancelledError") || NamedError.hasName(input, "UICancelledError")) {
|
||||
return ""
|
||||
}
|
||||
return undefined
|
||||
}
|
||||
|
||||
export function FormatUnknownError(input: unknown): string {
|
||||
|
|
|
|||
|
|
@ -1,5 +1,6 @@
|
|||
import { NamedError } from "@opencode-ai/core/util/error"
|
||||
import * as Log from "@opencode-ai/core/util/log"
|
||||
import { ConfigError } from "@/config/error"
|
||||
import { Cause, Effect } from "effect"
|
||||
import { HttpRouter, HttpServerError, HttpServerRespondable, HttpServerResponse } from "effect/unstable/http"
|
||||
|
||||
|
|
@ -18,6 +19,13 @@ export const errorLayer = HttpRouter.middleware<{ handles: unknown }>()((effect)
|
|||
if (!defect) return Effect.failCause(cause)
|
||||
|
||||
const error = defect.defect
|
||||
if (
|
||||
error instanceof NamedError &&
|
||||
(ConfigError.InvalidError.isInstance(error) || ConfigError.JsonError.isInstance(error))
|
||||
) {
|
||||
return Effect.succeed(HttpServerResponse.jsonUnsafe(error.toObject(), { status: 400 }))
|
||||
}
|
||||
|
||||
log.error("failed", { error, cause: Cause.pretty(cause) })
|
||||
|
||||
return Effect.succeed(
|
||||
|
|
|
|||
|
|
@ -5,6 +5,7 @@
|
|||
*/
|
||||
import { describe, expect, test } from "bun:test"
|
||||
import { aggregateFailures } from "@/cli/cmd/tui/context/aggregate-failures"
|
||||
import { ConfigError } from "@/config/error"
|
||||
|
||||
describe("aggregateFailures", () => {
|
||||
test("returns null when every result is fulfilled", () => {
|
||||
|
|
@ -41,11 +42,50 @@ describe("aggregateFailures", () => {
|
|||
expect(err!.message).toContain("agents: boom")
|
||||
})
|
||||
|
||||
test("formats structured config errors hidden inside SDK error causes", () => {
|
||||
const configError = new ConfigError.InvalidError({
|
||||
path: "/tmp/opencode.json",
|
||||
issues: [{ message: "Expected object", path: ["provider", "anthropic", "options"] }],
|
||||
})
|
||||
const err = aggregateFailures([
|
||||
{
|
||||
name: "config.get",
|
||||
result: {
|
||||
status: "rejected",
|
||||
reason: new Error("ConfigInvalidError", {
|
||||
cause: {
|
||||
body: configError.toObject(),
|
||||
},
|
||||
}),
|
||||
},
|
||||
},
|
||||
])
|
||||
|
||||
expect(err!.message).toContain("config.get: Configuration is invalid at /tmp/opencode.json")
|
||||
expect(err!.message).toContain("Expected object provider.anthropic.options")
|
||||
})
|
||||
|
||||
test("deduplicates identical failure messages across startup requests", () => {
|
||||
const reason = new Error("same config problem")
|
||||
const err = aggregateFailures([
|
||||
{ name: "config.providers", result: { status: "rejected", reason } },
|
||||
{ name: "provider.list", result: { status: "rejected", reason } },
|
||||
{ name: "app.agents", result: { status: "rejected", reason } },
|
||||
{ name: "config.get", result: { status: "rejected", reason } },
|
||||
{ name: "project.sync", result: { status: "fulfilled", value: undefined } },
|
||||
])
|
||||
|
||||
expect(err!.message).toContain("4 of 5 requests failed: same config problem")
|
||||
expect(err!.message).toContain(
|
||||
"Affected startup requests: config.providers, provider.list, app.agents, config.get",
|
||||
)
|
||||
expect(err!.message.match(/same config problem/g)?.length).toBe(1)
|
||||
})
|
||||
|
||||
test("attaches structured failure list under .cause", () => {
|
||||
const reason = new Error("nope")
|
||||
const err = aggregateFailures([{ name: "providers", result: { status: "rejected", reason } }])
|
||||
const cause = err!.cause as { failures: Array<{ name: string; reason: unknown }> }
|
||||
expect(cause.failures).toEqual([{ name: "providers", reason }])
|
||||
expect(err!.cause).toEqual({ failures: [{ name: "providers", reason }] })
|
||||
})
|
||||
|
||||
test("falls back to String() for opaque reasons", () => {
|
||||
|
|
|
|||
|
|
@ -1,6 +1,7 @@
|
|||
import { NodeHttpServer, NodeServices } from "@effect/platform-node"
|
||||
import { NamedError } from "@opencode-ai/core/util/error"
|
||||
import { describe, expect } from "bun:test"
|
||||
import { ConfigError } from "../../src/config/error"
|
||||
import { Effect, Layer } from "effect"
|
||||
import { HttpClient, HttpClientRequest, HttpRouter } from "effect/unstable/http"
|
||||
import { errorLayer } from "../../src/server/routes/instance/httpapi/middleware/error"
|
||||
|
|
@ -50,6 +51,27 @@ describe("HttpApi error middleware", () => {
|
|||
}),
|
||||
)
|
||||
|
||||
it.live("preserves config defects as client-visible bad requests", () =>
|
||||
Effect.gen(function* () {
|
||||
const configError = new ConfigError.InvalidError({
|
||||
path: "/tmp/opencode.json",
|
||||
issues: [{ message: "Expected object", path: ["provider", "anthropic", "options"] }],
|
||||
})
|
||||
|
||||
yield* HttpRouter.add("GET", "/config-error", Effect.die(configError)).pipe(
|
||||
Layer.provide(errorLayer),
|
||||
HttpRouter.serve,
|
||||
Layer.build,
|
||||
)
|
||||
|
||||
const response = yield* HttpClientRequest.get("/config-error").pipe(HttpClient.execute)
|
||||
const body = yield* response.json
|
||||
|
||||
expect(response.status).toBe(400)
|
||||
expect(JSON.stringify(body)).toBe(JSON.stringify(configError.toObject()))
|
||||
}),
|
||||
)
|
||||
|
||||
it.live("does not map storage not-found defects to 404", () =>
|
||||
Effect.gen(function* () {
|
||||
yield* HttpRouter.add(
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue