Skip to content

refactor: return Results up the stack, throw only at boundaries#14

Closed
kastriotkastrati wants to merge 0 commit into
mainfrom
codex/result-wrapping
Closed

refactor: return Results up the stack, throw only at boundaries#14
kastriotkastrati wants to merge 0 commit into
mainfrom
codex/result-wrapping

Conversation

@kastriotkastrati

@kastriotkastrati kastriotkastrati commented Jun 24, 2026

Copy link
Copy Markdown
Contributor

What

Reworks the branch's error handling into a railway: inner functions return Result<T, Error> and propagate failures as values; only boundary frames throw, and only when the runtime needs it.

The previous state of this branch wrapped ~182 calls in Result.from(...) and then immediately throw x.error — a behavior-preserving no-op (identical to a bare await) that added ~500 lines and never turned an error into a value (return Err count was 0). It also didn't compile (coordinator.ts lost env-var narrowing inside a closure).

How

  • Ports → Result: Store (28 methods), Platform (10), LlmAdapter.complete now return Promise<Result<T, Error>> (interfaces + all implementations change in lockstep).
  • 82 inner functions converted from throw to return Err. Domain invariants use Err(new Error("MESSAGE")) so the error channel stays uniformly Error (no string | Error unions).
  • Boundaries throw only when runtime-critical: queuemsg.retry(); scheduled, DO alarm/union → throw (cron/DO retry); HTTP routes → 5xx for crypto/KV/queue failures, 4xx for client faults. Best-effort work (synth/route/notify) logs and continues.
  • Loop-island pattern: since asyncMap/asyncForEach/asyncUnfold can't short-circuit on an Err and raw loops are eslint-banned, a fallible loop is wrapped in one Result.from(() => asyncMap(... unwrap(await f()) ...)). Adds a single unwrap helper to common.helper.ts.
  • D1 reads are wrapped (consciously overriding the generic "don't wrap reads" rule) because the queue/DO boundary needs read failures as values to drive retry.
  • Fixes the coordinator.ts type error.
  • Tests updated to the new Result signatures (unwrap .data, assert r.ok, rejects.toThrow!r.ok + error.message checks).

Rebased onto main (org pulse)

Rebased onto main (f03dbef feat(worker): post daily org pulse to Slack). That feature touched the same files the railway rewrote, so the merge re-expresses the org pulse in railway form rather than picking a side:

  • aggregateOrg / postDailyOrgPulse / buildDailyOrgPulse (clusters.ts) now return Result<void|string, Error> and propagate the listOpenSignals / getIdentity / getWorkingNotes / upsertWorkingNotes / postMessage / editMessage results; the identity fan-out uses the loop-island; the post-vs-edit externalRef choice is an IIFE returning Result<string, Error>.
  • SlackAdapter.postMessage keeps the new channel-post branch (meta.channelId) in Result form.
  • D1Store adopts the rowToSignal helper and adds listOpenSignals (railway).
  • aggregateOrg's opts is required (no default param) per house style; the two no-arg test calls pass {}.
  • Fixed one latent auto-merge bug: the new postMessage-to-channel test asserted a raw { id } return where the method now returns a Result.

Cleanup — boundary literals

Replaced Result.fromSync(() => null) / Result.fromSync(() => undefined) with Ok(null) / Ok(undefined) at the 5 KV-dedupe boundary sites in routes/github.ts and routes/slack.ts — wrapping a non-throwing literal in fromSync is noise. The legitimate Result.fromSync(() => JSON.parse(...)) sites are untouched (parse can throw).

Verification

pnpm typecheck (13/13), pnpm lint:check, and pnpm test (174 tests, 13 packages) all pass. A 4-lens adversarial audit (feature-preservation, railway invariants, merge completeness, cleanup equivalence) found no issues. 43 files, +2496/−1248.

⚠️ Open questions for reviewers (pre-existing on this branch, not from the railway rework)

These are behavior changes that predate the rework and were preserved as-is — flagging for an explicit decision:

  1. Webhook input validation (routes): the branch added isGithubWebhookBody/isSlackEnvelope guards + 400 "invalid payload"/"invalid json" responses. Original code did a bare JSON.parse(raw) as X (malformed body → threw → 500, which providers retry). The guards are permissive (all fields optional), so real webhooks pass; the real delta is malformed JSON now returns 400 (not retried) vs the old 500 (retried). Keep the validation, or restore pre-branch behavior?
  2. safeParse (identity-source): added per-row string validation that throws on a non-string roster field; the original tolerated it. Static deploy-time config, so low risk — fail-fast on bad config is arguably correct.
  3. extractText (adapter-llm): now returns "" for non-array content instead of throwing. Required to make extractText never-throw; only affects malformed model output.

@kastriotkastrati kastriotkastrati force-pushed the codex/result-wrapping branch 3 times, most recently from 7626865 to 8b9e972 Compare June 24, 2026 23:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant