Uh oh!
There was an error while loading. Please reload this page.
Friendlier context-violation errors + Ansi renderer - #1831
Closed
pranaygp wants to merge 2 commits into
Closed
Conversation
Phase 1: Add Ansi rendering helpers (frame, hint, note, help, code, inline) to @workflow/errors, and a chalk mock for readable snapshot tests. Phase 2: Add four context-violation error classes to @workflow/core (NotInWorkflowContextError, NotInStepContextError, NotInWorkflowOrStepContextError, UnavailableInWorkflowContextError) and apply them to all twelve user-facing throw sites so errors now include docs links and a structured "what/why/fix" frame. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
🦋 Changeset detectedLatest commit: cec8cfe The changes in this PR will be included in the next version bump. This PR includes changesets to release 21 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Contributor
📊 Benchmark Results
workflow with no steps💻 Local Development
workflow with 1 step💻 Local Development
workflow with 10 sequential steps💻 Local Development
workflow with 25 sequential steps💻 Local Development
workflow with 50 sequential steps💻 Local Development
Promise.all with 10 concurrent steps💻 Local Development
Promise.all with 25 concurrent steps💻 Local Development
Promise.all with 50 concurrent steps💻 Local Development
Promise.race with 10 concurrent steps💻 Local Development
Promise.race with 25 concurrent steps💻 Local Development
Promise.race with 50 concurrent steps💻 Local Development
workflow with 10 sequential data payload steps (10KB)💻 Local Development
workflow with 25 sequential data payload steps (10KB)💻 Local Development
workflow with 50 sequential data payload steps (10KB)💻 Local Development
workflow with 10 concurrent data payload steps (10KB)💻 Local Development
workflow with 25 concurrent data payload steps (10KB)💻 Local Development
workflow with 50 concurrent data payload steps (10KB)💻 Local Development
Stream Benchmarks(includes TTFB metrics)workflow with stream💻 Local Development
stream pipeline with 5 transform steps (1MB)💻 Local Development
10 parallel streams (1MB each)💻 Local Development
fan-out fan-in 10 streams (1MB each)💻 Local Development
SummaryFastest Framework by WorldWinner determined by most benchmark wins
Fastest World by FrameworkWinner determined by most benchmark wins
Column Definitions
Worlds:
❌ Some benchmark jobs failed:
Check the workflow run for details. |
Contributor
🧪 E2E Test Results❌ Some tests failed Summary
❌ Failed Tests💻 Local Development (2 failed)vite-stable (2 failed):
Details by Category❌ 💻 Local Development
✅ 📦 Local Production
✅ 🐘 Local Postgres
✅ 📋 Other
❌ Some E2E test jobs failed:
Check the workflow run for details. |
Contributor
pranaygp
commented
Apr 23, 2026
| '@workflow/core': patch | ||
| '@workflow/errors': patch | ||
| --- | ||
ContributorAuthor
There was a problem hiding this comment.
change should be terse. 1-2 sentences. not this verbose
Member
There was a problem hiding this comment.
This keeps happening for me as well. We should update AGENTS.md
pranaygp
marked this pull request as ready for review
April 23, 2026 01:33
This was referenced Apr 23, 2026
Contributor
There was a problem hiding this comment.
Pull request overview
This PR is part of the effort to make @workflow/core context-violation errors more actionable by introducing a structured terminal-friendly renderer in @workflow/errors and new dedicated context error classes in @workflow/core, then applying them across the call sites that throw in the wrong context.
Changes:
- Add
@workflow/errorsAnsihelpers (frame,help,hint,note,code,inline) plus tests and a chalk mock for snapshot readability. - Introduce new
@workflow/corecontext error classes and replace multiple genericErrorthrow sites with these structured errors. - Add a changeset for publishing
@workflow/core+@workflow/errorspatches.
Reviewed changes
Copilot reviewed 18 out of 19 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| pnpm-lock.yaml | Adds chalk + vitest for @workflow/errors and updates lock snapshots. |
| packages/errors/src/index.ts | Re-exports Ansi from the package entrypoint. |
| packages/errors/src/ansi.ts | New ANSI/box-drawing composition helpers (uses chalk). |
| packages/errors/src/ansi.test.ts | Adds unit tests for Ansi rendering behavior. |
| packages/errors/package.json | Adds chalk dependency, vitest devDependency, and a package-level test script. |
| packages/errors/mocks/chalk.ts | Manual chalk mock to make style output snapshot-friendly. |
| packages/core/src/workflow/index.ts | Switches workflow stubs to throw the new structured context errors. |
| packages/core/src/workflow/get-workflow-metadata.ts | Adds a note about avoiding cycles, but still throws a plain Error when out of context. |
| packages/core/src/workflow/define-hook.ts | Uses UnavailableInWorkflowContextError for defineHook().resume() in workflow exports. |
| packages/core/src/workflow/create-hook.ts | Uses NotInWorkflowContextError when workflow VM hook function is absent. |
| packages/core/src/step/writable-stream.ts | Uses NotInWorkflowOrStepContextError when no step/workflow context exists. |
| packages/core/src/step/get-workflow-metadata.ts | Uses NotInWorkflowOrStepContextError when called outside context. |
| packages/core/src/step/get-step-metadata.ts | Uses NotInStepContextError when called outside step context. |
| packages/core/src/sleep.ts | Uses NotInWorkflowContextError when called outside workflow context. |
| packages/core/src/define-hook.ts | Uses NotInWorkflowContextError for defineHook().create() in non-workflow exports. |
| packages/core/src/create-hook.ts | Uses NotInWorkflowContextError for createHook() / createWebhook() stubs. |
| packages/core/src/context-errors.ts | New context error classes built on @workflow/errorsAnsi framing. |
| packages/core/src/context-errors.test.ts | Adds tests validating the new structured context error messages. |
| .changeset/friendlier-context-errors.md | Documents the patch release for new Ansi helpers + context error classes. |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+119
to
+131
| for (const marker of markers) { | ||
| const textLen = marker.endCol - marker.startCol; | ||
| const midPoint = Math.floor(textLen / 2); | ||
| if (marker.startCol > pos) { | ||
| parts.push(' '.repeat(marker.startCol - pos)); | ||
| pos = marker.startCol; | ||
| } | ||
| const segment = `${'─'.repeat(midPoint)}┬${'─'.repeat(textLen - midPoint - 1)}`; | ||
| const colorFn = marker.color ?? identity; | ||
| parts.push(colorFn(segment)); | ||
| pos += textLen; | ||
| } |
There was a problem hiding this comment.
buildUnderline() assumes every marker has endCol > startCol. If a caller passes an empty text token (length 0), textLen - midPoint - 1 becomes negative and '─'.repeat(...) will throw. Add a guard (e.g. treat zero-length markers as length 1, or skip underlining + still emit an explanation) to make Ansi.inline robust for edge cases.
ContributorAuthor
There was a problem hiding this comment.
Fixed in cec8cfe — buildUnderline now clamps endCol - startCol with Math.max(1, …) so a zero-length marker produces a single-char ┬ anchor instead of a negative String.repeat count.
| /** Apply dim styling to the `workflow//` / `step//` separators in a name. */ | ||
| function ansifyName(name: string): string { | ||
| return name; |
There was a problem hiding this comment.
ansifyName() is documented as applying dim styling to the workflow// / step// separators, but it currently returns the input unchanged. Either implement the styling (so the workflow name in UnavailableInWorkflowContextError matches the intent) or remove/adjust the comment + call site to avoid misleading behavior.
Suggested change
| returnname; | |
| returnname | |
| .replaceAll('workflow//',Ansi.dim('workflow//')) | |
| .replaceAll('step//',Ansi.dim('step//')); |
ContributorAuthor
There was a problem hiding this comment.
Fixed in cec8cfe — ansifyName() now actually applies Ansi.dim to the workflow/ / step/ prefix (added a dim export in packages/errors/src/ansi.ts for this).
Comment on lines
+123
to
+125
| // Keep `getWorkflowMetadata` import live for future use (the error message | ||
| // currently reads the symbol directly to avoid a circular throw). | ||
| void getWorkflowMetadata; |
There was a problem hiding this comment.
Importing getWorkflowMetadata solely to silence noUnusedLocals via void getWorkflowMetadata; adds a hard dependency and makes the file harder to reason about. If the goal is to avoid a circular dependency, it would be cleaner to remove the unused import entirely (or convert it to a type-only import if you only need types), and restructure shared constants/types into a separate module if needed later.
ContributorAuthor
There was a problem hiding this comment.
Fixed in cec8cfe — the void getWorkflowMetadata; workaround and the value import were removed. context-errors.ts now imports only the WorkflowMetadata type and the WORKFLOW_CONTEXT_SYMBOL, so there's no hard value-level dependency on the metadata module at class-construction time.
Comment on lines
+42
to
48
| // Avoid importing NotInWorkflowOrStepContextError here — that module | ||
| // imports from this file, so bringing it in eagerly would create a | ||
| // module-init cycle. The companion step/get-workflow-metadata.ts uses | ||
| // the structured class. | ||
| throw new Error( | ||
| '`getWorkflowMetadata()` can only be called inside a workflow or step function' | ||
| ); |
There was a problem hiding this comment.
This still throws a plain Error rather than the new structured context-violation error, but the PR description/changeset mention applying the friendlier framing to both getWorkflowMetadata() call sites. If the structured class can’t be imported here due to cycles, consider throwing an Ansi.frame(...) message directly (or moving WORKFLOW_CONTEXT_SYMBOL / types into a shared module) so this path matches the new error format too.
ContributorAuthor
There was a problem hiding this comment.
Fixed in cec8cfe — getWorkflowMetadata() now throws with the same Ansi.frame / Ansi.code / Ansi.note framing as the other context errors, instead of a plain Error. Kept it inline rather than importing one of the new classes to avoid a module-init cycle with context-errors.ts.
Comment on lines
+2
to
+3
| '@workflow/core': patch | ||
| '@workflow/errors': patch |
There was a problem hiding this comment.
The changeset frontmatter uses single quotes around package names, but existing changesets in this repo use double quotes (e.g. .changeset/curvy-dingos-cry.md). Please switch to double quotes for consistency and to match the established convention.
Suggested change
| '@workflow/core': patch | |
| '@workflow/errors': patch | |
| "@workflow/core": patch | |
| "@workflow/errors": patch |
ContributorAuthor
There was a problem hiding this comment.
Fixed in cec8cfe — single-quoted frontmatter switched to double-quoted to match repo convention.
| import type { StructuredError } from '@workflow/world'; | ||
| import type { StringValue } from 'ms'; | ||
| export * as Ansi from './ansi.js'; |
There was a problem hiding this comment.
@workflow/errors is designed to avoid Node.js-only imports for VM/workflow contexts (see the comment below isError), but exporting Ansi from the main entrypoint makes ./ansi (and its chalk dependency) load whenever @workflow/errors is imported. This can increase bundle size and may break workflow/VM bundling if chalk (or its transitive deps) pull in Node built-ins. Consider moving Ansi to a separate subpath export (e.g. @workflow/errors/ansi) or making the renderer implementation dependency-free / lazily loaded so the default entry stays workflow-safe.
Suggested change
| export*asAnsifrom'./ansi.js'; | |
| // Do not export `./ansi.js` from the main entrypoint. | |
| // This package is used in workflow/VM contexts that must avoid pulling in | |
| // Node.js-only or heavier optional dependencies through the default module graph. | |
| // Expose ANSI helpers from a separate subpath entry instead. |
ContributorAuthor
There was a problem hiding this comment.
Acknowledged — valid concern. I'm going to address this in a followup PR rather than folding it into this 7-PR stack, since:
- Moving
Ansito a subpath export (@workflow/errors/ansi) is mechanical but touches every importer; doing it here would force a rebase through phases 3–8 and re-run CI 7 times. - The immediate user-facing VM risk is bounded —
context-errors.tsandget-workflow-metadata.tsare the only VM-reachable modules that pull inAnsitoday, and they'd still pay the chalk cost after the subpath move (they explicitly use the rendering helpers). The subpath change is really about sparing other@workflow/errorsconsumers (e.g. code that only wantsSerializationError) from the chalk dep. - The followup PR can also audit whether
ansi.tsneeds chalk at all vs. a lightweight dependency-free renderer, which is the cleaner end state.
Will file the followup and link it back here.
14 tasks
2 tasks
| import type { StructuredError } from '@workflow/world'; | ||
| import type { StringValue } from 'ms'; | ||
| export * as Ansi from './ansi.js'; |
This was referenced Apr 24, 2026
- Tighten phase 1 changeset to a single sentence (per pranaygp review) and switch to double-quoted frontmatter (per Copilot + repo convention). - Implement `ansifyName` to actually apply dim styling to workflow/ / step/ prefixes; add an `Ansi.dim` helper to `@workflow/errors` so callers don't need to import chalk directly. - Remove the `void getWorkflowMetadata;` workaround in context-errors.ts by dropping the unused value import (we only needed the type and symbol). - Render the plain-Error throw in `workflow/get-workflow-metadata.ts` with `Ansi.frame` + docs link so the VM path matches the structured-class styling from the sibling step path (still uses a plain Error to avoid the module-init cycle). - Guard `buildUnderline` against zero-length markers so a stray empty token can't produce a negative `String.repeat` count.
44 tasks
pranaygp
commented
Apr 24, 2026
ContributorAuthor
Superseded by #1849 — consolidated friendlier-errors PR with all 8 phases + follow-up fixes (ANSI leak, non-retry semantics, shared captureStackTrace helper). |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Phase 1 + 2 of the friendlier-errors stack. Picks up where #706 (@Schniz, stalled) left off.
@workflow/errors— newAnsirendering helpers (frame,hint,note,help,code,inline) for composing terminal-friendly, box-drawn error messages. Chalk auto-detects TTY and falls back to plain text (CI, Datadog).@workflow/core— four context-violation error classes applied to all 12 user-facing throw sites:NotInWorkflowContextError(e.g.createHook(),sleep())NotInStepContextError(e.g.getStepMetadata())NotInWorkflowOrStepContextError(e.g.getWorkflowMetadata(),getWritable())UnavailableInWorkflowContextError(e.g.resumeHook(),defineHook().resume()) — names the active workflow.Example rendering:
Note
The rendered framing in this PR is later polished in PR #1849 (drop
functionNameleak, simplifynote: Read more about…→docs:, and redirect the stack trace to user code). Verify the phase-1 behavior here; the polished form lands in the followups PR.Manual test plan
All tests below use
workbench/nextjs-turbopack— start withcd workbench/nextjs-turbopack && pnpm devand visithttp://localhost:3000. Watch the terminal runningpnpm devfor rendered errors.A convenient smoke route that exercises most throw sites — drop this at
workbench/nextjs-turbopack/app/api/friendlier-errors-smoke/route.ts:createHook()outside workflow — hit?which=createHook. Expect a box-drawn frame (╭─ … ╰─) with title`createHook()` can only be called inside a workflow functionand anote: Read more about createHook(): https://…/workflow/create-hookinside.sleep()outside workflow — hit?which=sleep. Same framing, docs URL ends in.../workflow/sleep.getStepMetadata()in a workflow function (not a step) — add to a"use workflow"file:getWorkflowMetadata()in application code — hit?which=getWorkflowMetadata. Expect "workflow or step function".resumeHook()inside a workflow — callresumeHook(token, payload)from inside a"use workflow"function. Expect:`resumeHook()` cannot be called from a workflow context.this call was made from the workflow//./src/workflows/example.ts//myWorkflow workflow context.(with theworkflow/prefix dimmed); and the docs URL.| catand confirm ANSI escape bytes are stripped (chalk auto-detect).Unit tests
pnpm --filter @workflow/errors test(10 new Ansi tests)pnpm --filter @workflow/core test(5 new context-error tests)📚 Friendlier errors stack
Multi-PR initiative inspired by @Schniz's stalled #706:
Ansirendering primitives + context-violation errorsSerializationErrorat serialization / stream / encryption boundariesdescribeError)throw new Error(...)sitesdescribeRunError+ public subpathWorkflowBuildError+ applications in@workflow/buildersfunctionNameleak, simplify docs framing, redirect stack to user codeEach PR is stacked on the previous one; merge in order.
🤖 Generated with Claude Code