Skip to content

Add isNotFoundError and improve getDialogue error handling#96

Closed
devkobby24 wants to merge 7 commits into
developmentfrom
getdialogue-throw-instead-of-null
Closed

Add isNotFoundError and improve getDialogue error handling#96
devkobby24 wants to merge 7 commits into
developmentfrom
getdialogue-throw-instead-of-null

Conversation

@devkobby24

@devkobby24 devkobby24 commented Jul 16, 2026

Copy link
Copy Markdown

What

getDialogue throws a descriptive DialogueDBError instead of returning null
when a dialogue can't be resolved.

Fixes #94, fixes #90 — the same bug, reported twice.

Why

A dialogue's namespace is part of its storage key, so the API resolves an omitted
namespace to DEFAULT rather than searching across namespaces. A 404 couldn't be
distinguished from "you dropped the namespace listDialogues handed you", and
returning null collapsed both into a value that detonated one line later:

TypeError: Cannot read properties of null (reading 'loadMessages')

That's the crash in both reports — and in the featured in-product sample, so it's
the first thing a new developer copies.

Now:

DialogueDBError: Dialogue '01KVAP…' not found in the default namespace.
If it was created with a namespace, pass it: getDialogue("01KVAP…", { namespace })
requestId: 01KXNJK6Y05SME2CFHB6QDTPKZ

The API's requestId, code and details are carried through. Both miss paths
are covered — the API's 404 and a null body, which previously diverged.

This reverses d95af9a

Greg's Feb commit ("enforce get dialogue to return null instead of 404") was
deliberate — but it predates #90 (June) and #94 (July) by four months, so it was
made without the evidence that the null was itself the confusing part.

Breaking

getDialogue returns Promise<Dialogue> rather than Promise<Dialogue | null>.
Existence checks should catch and test isNotFoundError, now exported from the
package root:

import { isNotFoundError } from 'dialogue-db';

try {
  const dialogue = await db.getDialogue(id, { namespace });
} catch (error) {
  if (isNotFoundError(error)) { /* not there */ } else { throw error; }
}

Version: this must be 3.0.0, and the automation will not do it

package.json is untouched in this PR — but the version can't be left to the
release automation, because its default is wrong rather than absent.

draft-main-pr.yml only ever increments PATCH (line 174, "Bump version number
(patch increment)"). There is no major or minor path in it:

if [ "$PACKAGE_VERSION_NUM" -le "$LATEST_VERSION_NUM" ]; then
  BUMP_VERSION=true      # → PATCH=$((PATCH + 1))

package.json is 2.0.1 and the latest tag is v2.0.1, so 2.0.1 <= 2.0.1
and this publishes as v2.0.2 — a breaking change delivered as a patch. Every
external consumer on ^2.0.1 picks it up on their next install and gets an
unhandled throw where they had a null check, without anyone choosing to
upgrade.

Internal repos are insulated: mcp, core-backend and the examples are all
pinned ^1.0.x, which can't resolve 2.x let alone 3.x. It's npm consumers who
are exposed.

The same comparison is the escape hatch:

else
  CURRENT_VERSION=$PACKAGE_VERSION
  BUMP_VERSION=false     # → uses package.json verbatim

Set package.json to 3.0.0 and 3.0.0 > 2.0.1, so the workflow steps back and
cuts v3.0.0. The automation is built to handle routine patches and defer to a
deliberately-set version — this is the case it defers for.

Action required before this reaches main: set package.json to 3.0.0,
either here or on the release PR. Left alone, it ships as 2.0.2.

Follow-ups filed

isNotFoundError identifies the API's structured "resource does not exist"
response. It requires the type as well as the status, so a 404 raised by the
gateway — which carries no structured body and arrives as server_error — stays
an error rather than being read as an authoritative not-found.

dialogueNotFound builds the message for an unresolvable dialogue. A namespace
is part of a dialogue's storage key, so the API resolves an omitted one to its
default; the resulting miss cannot distinguish "does not exist" from "you
dropped the namespace", and naming the namespace to pass is the only actionable
signal available. The API's requestId, code and details are carried through
when there was a cause, so the error stays traceable in support requests.
A 404 from an unscoped lookup could mean the dialogue does not exist, or that
the caller dropped the namespace that list handed them. Returning null
collapsed both into a value that crashed downstream as "Cannot read properties
of null (reading 'loadMessages')" — the failure reported in #94 and #90, and
the exact line in the featured in-product sample.

It now throws, naming the namespace searched and the one to pass. Both miss
paths are covered: the API's 404 and a null body, which previously diverged.

Reverses d95af9a, which predates both reports that showed the null was itself
the confusing part.

BREAKING CHANGE: returns Promise<Dialogue> rather than Promise<Dialogue | null>.
Pins the message for both a scoped and an unscoped miss, that an empty
namespace is treated as unscoped (the request layer only sends a non-empty
one), and that the API's requestId and code survive onto the thrown error.
Follows getDialogue no longer returning null, and documents that the namespace
a dialogue was created with must be passed — without it the lookup is scoped to
the default namespace and will not find it.
Throwing removes the null that existence checks relied on, so callers need a
way to tell a missing dialogue from a real failure without matching on message
strings.
getDialogue now throws, so the null branch is unreachable. withErrorHandler
already reports a DialogueDBError and exits non-zero, so the CLI prints the
namespace hint and requestId rather than a bare "not found".
@devkobby24 devkobby24 self-assigned this Jul 16, 2026
@claude

claude Bot commented Jul 16, 2026

Copy link
Copy Markdown

Claude finished @devkobby24's task in 2m 21s —— View job


Review: Add isNotFoundError and improve getDialogue error handling

  • Gather context and read changed files
  • Review error handling changes (getDialogue.ts, errors/index.ts)
  • Review CLI changes (shared.ts)
  • Review type/export changes (index.ts, class.dialogue-db.ts)
  • Review test coverage (getDialogue.test.ts)
  • Post review feedback

Overall: This is a well-motivated, well-executed change. The PR description is excellent — it traces the problem from user-facing crash back to root cause, explains the design decision being reversed and why, and documents the migration path. The code matches the intent cleanly.

What works well

  • Error message design — the namespace hint in dialogueNotFound (src/errors/index.ts:109-129) is genuinely helpful. The distinction between scoped and unscoped lookups, and treating empty-string namespace as unscoped to match the request layer, shows careful attention to how the API actually behaves.

  • CLI simplificationloadDialogueOrExit (src/cli/shared.ts:46-51) shrinks from a null-check-and-exit pattern to a single delegation. The error now flows through withErrorHandler, which already formats DialogueDBError with all structured fields. Cleaner and more informative than the old "Dialogue ${id} not found\n".

  • isNotFoundError type guard (src/errors/index.ts:23-29) — the dual check on statusCode === 404 AND type === "not_found" is a smart design call. The comment explaining that gateway 404s carry type: "server_error" and should stay as errors is the kind of context that prevents future regressions.

  • getOrCreateDialogue is unaffected — it calls get() directly and handles 404s itself, so this change doesn't cascade. Good separation.

  • Test coverage (src/methods/getDialogue.test.ts) — thorough. Tests null, undefined, 404 from API, unscoped hint, scoped no-hint, empty namespace as unscoped, requestId preservation, and non-404 re-throw. The empty-namespace edge case (line 96-106) is the kind of test that prevents a subtle regression.

Suggestions

  1. Missing unit tests for isNotFoundError itself — it's exported from the package root and is the primary migration mechanism for the breaking change, but has no dedicated tests. Consider adding a small test file covering the three-way check: (a) matches a proper 404, (b) rejects a gateway 404 (type: "server_error", statusCode: 404), (c) rejects non-404 DialogueDBError, (d) rejects plain Error. This is the function consumers will rely on most.

  2. Error cause chaining — see inline comment on src/errors/index.ts. The original API error's stack is lost when re-wrapping. Not blocking, but Error.cause (ES2022) would let consumers inspect the full chain.

  3. Type annotation precision — see inline comment on src/methods/getDialogue.ts:23. data is typed IDialogue | null but the test suite verifies the undefined path too.

  4. Consider whether isNotFoundError should match the re-wrapped errordialogueNotFound defaults to type: "not_found" and statusCode: 404, so isNotFoundError returns true for the re-wrapped error from getDialogue. This is correct for the documented usage pattern (catch + isNotFoundError), but it's worth calling out explicitly in the JSDoc that the guard matches both raw API errors and the SDK's re-wrapped ones, so consumers know it's safe to use at any layer.

No issues found

  • Security: No user input is interpolated unsafely. Error messages use the dialogue ID and namespace which are already trusted inputs from the caller.
  • Performance: No new allocations on the happy path. Error construction only happens on miss.
  • Breaking change: Correctly documented. The migration path is clear and isNotFoundError is properly exported.

Looks good to merge with the minor suggestions above addressed at your discretion.

Comment thread src/errors/index.ts
Comment thread src/methods/getDialogue.ts
@claude

claude Bot commented Jul 16, 2026

Copy link
Copy Markdown

Claude finished @devkobby24's task in 2m 22s —— View job


Review: Add isNotFoundError and improve getDialogue error handling

  • Gather context and read changed files
  • Review error handling changes (getDialogue.ts, errors/index.ts)
  • Review CLI changes (shared.ts)
  • Review type/export changes (index.ts, class.dialogue-db.ts)
  • Review test coverage (getDialogue.test.ts, errors/index.test.ts)
  • Post review feedback

Overall: This looks good to merge. The new commits (f398650, aed8317) address the main suggestion from the previous review — isNotFoundError now has dedicated, thorough test coverage. The code is clean and the test suite covers the important edge cases.

What changed since last review

The two new commits add:

  1. isNotFoundError unit tests (src/errors/index.test.ts:3-71) — six cases covering: structured API 404 match, gateway 404 rejection (the key dual-check behavior), mismatched status/type combos, non-DialogueDBError values (Error, plain object, string, null, undefined), and re-wrapped SDK errors. This was the primary gap flagged in the first review and it's well addressed.

  2. CLI delegation (src/cli/shared.ts:46-51) — loadDialogueOrExit now lets the throw propagate to withErrorHandler instead of handling it locally. I verified every call site — all 14 uses across dialogue.ts and message.ts are inside withErrorHandler, so the exit path is intact.

Remaining minor observations

  1. loadDialogueOrExit name is now slightly misleading — it no longer exits directly; it throws, and the caller's withErrorHandler wrapper does the exit. The docstring explains this accurately, but the function name still says "OrExit". Not worth renaming on this PR since it's a CLI internal and every caller already wraps it correctly, but worth noting if the CLI gets revisited.

  2. Error cause chaining — flagged in the first review and still a nice-to-have. The dialogueNotFound factory extracts requestId/code/details from the original API error but discards its stack. Passing { cause } to the super() call in DialogueDBError would let consumers inspect the full chain. Low priority since requestId is the primary debugging signal.

  3. getOrCreateDialogue is correctly unaffected — it calls get() directly (not getDialogue()) and handles not_found via error.type === "not_found" at src/methods/getOrCreateDialogue.ts:27. No cascading breakage.

No issues found

  • Correctness: The two miss paths (API 404 and null body) are both covered. The isNotFoundError dual check (statusCode === 404 AND type === "not_found") correctly filters gateway 404s. The dialogueNotFound factory defaults to type: "not_found" and statusCode: 404 when there's no cause, so isNotFoundError matches re-wrapped errors consistently.
  • Security: Error messages interpolate id and namespace — both are caller-supplied trusted inputs, not user-controlled external data.
  • Breaking change: Properly documented in the PR description. The migration path via isNotFoundError is clear and tested.
  • Test coverage: Strong. getDialogue.test.ts covers null, undefined, API 404, unscoped hint, scoped no-hint, empty namespace as unscoped, requestId preservation, and non-404 re-throw. errors/index.test.ts covers isNotFoundError across all relevant input shapes.

Looks good to merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

1 participant