Skip to content

[factory] error {} — raw Errors serialize to {} (message/stack are non-enumerable); describeError() already exists one line away #83

Description

@khaliqgant

Summary

[factory] error {} — the most alarming line factory prints is also the least informative. A plain Error logged as the metadata object serializes to {} because Error.message and Error.stack are non-enumerable, so JSON.stringify drops them. The error is counted in the errors metric but is undiagnosable from the log.

Observed

A real dispatch (hoopsheet#30, factory 0.1.17), verbatim:

[factory-sdk] waiting for spawned agents to exit before shutting down the owned relay broker {"agents":["ar-30-impl-hoopsheet","ar-30-review"],"timeoutMs":1800000,"remainingCount":2}
[factory] error {}
[factory] error {}

Two errors occurred. The log says nothing about either — no message, no stack, no type, no issue.

The same line is sometimes useful, which makes it worse:

[factory] error {"code":"http_500","retryable":true,"status":500,"data":{"error":"internal reply dropped","success":false},"name":"HarnessDriverProtocolError"}

HarnessDriverProtocolError carries enumerable own properties, so it prints. A bare Error prints {}. Identical log line, wildly different information, depending on the error class — so you cannot tell from the log whether "nothing was printed" means "nothing to say" or "everything was dropped."

Root cause

src/orchestrator/factory.ts:4087:

#error(error: unknown, issue?: IssueRef): void {
  this.#increment('errors')
  this.#logger.error?.('[factory] error', error)                    // <-- raw Error -> {}
  this.#emit('error', { error, ...describeError(error), issue })    // <-- describeError() right here
}

describeError already exists in the same file (factory.ts:6547) and already extracts exactly what's missing:

const describeError = (error: unknown): { errorMessage: string; errorStack?: string } => {
  if (error instanceof Error) { ... }

Line 4090 uses it. Line 4089 doesn't. The emitted event is diagnosable; the log line — the thing a human actually reads — is not.

Fix

#error(error: unknown, issue?: IssueRef): void {
  this.#increment('errors')
  const details = describeError(error)
  this.#logger.error?.('[factory] error', { ...details, issue: issue?.key, ...errorFields(error) })
  this.#emit('error', { error, ...details, issue })
}

Include at minimum errorMessage, errorStack, the error name/code when present, and the issue key — [factory] error {} during a dispatch doesn't even say which issue it belongs to.

This is systemic, not one line

27 call sites in factory.ts alone pass a raw error as the log metadata (grep -cE "logger\.(error|warn|info)\?\.\([^,]+, *(error|err)\)"). Every one has the same defect. Examples:

factory.ts:634  this.#logger.warn?.('[factory] failed to refresh stopping heartbeat', error)
factory.ts:663  this.#logger.warn?.('[factory] failed to refresh live daemon heartbeat', error)
factory.ts:707  this.#logger.warn?.('[factory] live subscription high-watermark unavailable', error)
factory.ts:738  this.#logger.warn?.('[factory] live subscription poll failed', error)
factory.ts:762  this.#logger.warn?.('[factory] live issue event drain failed', error)
factory.ts:989  this.#logger.warn?.('[factory] PR completion sweep failed', error)

Suggested approach:

  1. Fix #error() first — it's the loudest and it increments a metric.
  2. Normalise error serialization in the logger so a raw Error can never render as {} regardless of call site. That fixes all 27 without touching 27 lines, and prevents the 28th.
  3. Optionally lint against passing a bare Error/unknown as the metadata object.

Why it matters beyond tidiness

  • These lines are counted errors (#increment('errors')) — a metric you can't act on.
  • [factory] error {} reads as something is broken to anyone running a dispatch, with zero way to tell whether it's benign teardown noise or a real failure. In practice it appears on every run (I've seen it on 4/4 clean dispatches that produced correct PRs), which trains operators to ignore an error-level log — the worst possible outcome for the one line that should mean "look here."
  • It hides real bugs. The internal reply dropped / failed to release ar-NN-review teardown failure is currently only diagnosable because that particular error class happens to serialize. A plain Error in the same path would be invisible.

Related: #67/#69 (reviewer-hang teardown — the errors that surface here), #82 (agent-name collision — diagnosed only because resume-already-exists was logged with real fields).

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Fields

    No fields configured for issues without a type.

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions