bot: classify-intent answers UNKNOWN for a bounds rejection because langchain wraps it in OutputParserException #84

New issue

Closed

opened 2026-09-02 17:20:57 +00:00 by yuris · 1 comment

yuris commented

2026-09-02 17:20:57 +00:00

Owner

Raised by Reviewer78r1 in rounds 1–6 of PR #78 and deliberately carried out of it: the service bound holds, nothing is written, and the fix is a langchain error-handling decision rather than a numeric-bounds one.

The defect

apps/management/lib/bot/nodes/classify-intent.ts:399-412 catches a failed model invocation and routes it through describeBotRejection, so an out-of-range number from the intent classifier should reach the operator as a named refusal.

It does not. When the structured-output parser rejects the model's payload, langchain wraps the underlying ZodError in an OutputParserException. describeBotRejection matches ZodError and ServiceRejection, so it returns null, and the operator gets UNKNOWN — the bot appearing to ignore them, which is exactly the outcome the code was written to avoid.

The comment ships either way, and it is wrong

This is the part worth fixing regardless of which direction the behaviour goes. classify-intent.ts:401-403 currently reads:

// A number outside what the columns can hold now fails here rather than
// four hops later at the driver. Say so instead of answering "UNKNOWN" with
// no explanation, which reads to the operator as the bot ignoring them.

The first sentence is true. The second describes behaviour the code does not produce: it answers UNKNOWN with no explanation, which is the thing the comment says it avoids. PR #78 spent six review rounds on comments claiming slightly more than their code does, and this is one of them still standing.

Severity

Message quality only. The service bound holds — adjustStock, createPurchase and createOrder all reject before any write, measured in round 1 — so nothing is written and nothing is corrupted. The cost is an operator who cannot tell a refused number from a bot that did not understand them, and who therefore retries the same message.

Why it needs a decision, not a patch

Unwrapping OutputParserException has two viable shapes and they are not equivalent:

Match the class and read its llmOutput, re-parsing to recover the underlying issues. Precise, but couples this node to langchain's exception surface.
Walk the cause chain for a ZodError, the way apps/store/lib/catalog.ts walks .cause for a framework marker. Looser coupling, and there is already a bounded, cycle-safe implementation in this repo to copy from — including the reasons it is bounded (see #75).

Whichever is chosen, lib/bot/nodes/vision-llm.ts carries the same wrap. Its comment is correctly hedged, so it is not making a false claim, but a real fix touches both nodes — which is the main argument for this being its own change rather than a nit on a bounds PR.

Suggested scope

Unwrap the parser exception in whichever of the two shapes is chosen.
Apply it to classify-intent.ts and vision-llm.ts together.
Correct classify-intent.ts:401-403 to describe what the code does.
One test per node asserting a wrapped ZodError reaches the operator as a named refusal, plus a negative control asserting a genuine model outage does not get dressed up as a bad number — the shape PR #78 settled on after four rounds of unit tests standing in for wiring.

#78 established describeBotRejection and closed the equivalent wiring gap for execute-purchase.ts and execute-stock-adjust.ts, which call it with real, unwrapped errors and are covered by node-wiring.test.ts.
#75 has the bounded cause-walk implementation, if option 2 is chosen.

Raised by `Reviewer78r1` in rounds 1–6 of PR #78 and deliberately carried out of it: the service bound holds, nothing is written, and the fix is a langchain error-handling decision rather than a numeric-bounds one. ## The defect `apps/management/lib/bot/nodes/classify-intent.ts:399-412` catches a failed model invocation and routes it through `describeBotRejection`, so an out-of-range number from the intent classifier should reach the operator as a named refusal. It does not. When the structured-output parser rejects the model's payload, langchain wraps the underlying `ZodError` in an **`OutputParserException`**. `describeBotRejection` matches `ZodError` and `ServiceRejection`, so it returns `null`, and the operator gets `UNKNOWN` — the bot appearing to ignore them, which is exactly the outcome the code was written to avoid. ## The comment ships either way, and it is wrong This is the part worth fixing regardless of which direction the behaviour goes. `classify-intent.ts:401-403` currently reads: ``` // A number outside what the columns can hold now fails here rather than // four hops later at the driver. Say so instead of answering "UNKNOWN" with // no explanation, which reads to the operator as the bot ignoring them. ``` The first sentence is true. The second describes behaviour the code does not produce: it answers `UNKNOWN` with no explanation, which is the thing the comment says it avoids. PR #78 spent six review rounds on comments claiming slightly more than their code does, and this is one of them still standing. ## Severity Message quality only. The service bound holds — `adjustStock`, `createPurchase` and `createOrder` all reject before any write, measured in round 1 — so nothing is written and nothing is corrupted. The cost is an operator who cannot tell a refused number from a bot that did not understand them, and who therefore retries the same message. ## Why it needs a decision, not a patch Unwrapping `OutputParserException` has two viable shapes and they are not equivalent: 1. **Match the class** and read its `llmOutput`, re-parsing to recover the underlying issues. Precise, but couples this node to langchain's exception surface. 2. **Walk the cause chain** for a `ZodError`, the way `apps/store/lib/catalog.ts` walks `.cause` for a framework marker. Looser coupling, and there is already a bounded, cycle-safe implementation in this repo to copy from — including the reasons it is bounded (see #75). Whichever is chosen, `lib/bot/nodes/vision-llm.ts` carries the same wrap. Its comment is correctly hedged, so it is not making a false claim, but a real fix touches both nodes — which is the main argument for this being its own change rather than a nit on a bounds PR. ## Suggested scope - Unwrap the parser exception in whichever of the two shapes is chosen. - Apply it to `classify-intent.ts` and `vision-llm.ts` together. - Correct `classify-intent.ts:401-403` to describe what the code does. - One test per node asserting a wrapped `ZodError` reaches the operator as a named refusal, plus a negative control asserting a genuine model outage does **not** get dressed up as a bad number — the shape PR #78 settled on after four rounds of unit tests standing in for wiring. ## Related - #78 established `describeBotRejection` and closed the equivalent wiring gap for `execute-purchase.ts` and `execute-stock-adjust.ts`, which call it with real, unwrapped errors and are covered by `node-wiring.test.ts`. - #75 has the bounded cause-walk implementation, if option 2 is chosen.

yuris commented

2026-09-28 20:34:45 +00:00

Author

Owner

Closed as stale. In PR #113 (1e94275), LangGraph and LangChain were completely replaced with a native OpenRouter runner, and langchain was removed from dependencies. Model structured output is parsed with IntentSchema.parse(), which throws a native ZodError caught directly by describeBotRejection. OutputParserException no longer exists on this path.

Closed as stale. In PR #113 (`1e94275`), LangGraph and LangChain were completely replaced with a native OpenRouter runner, and `langchain` was removed from dependencies. Model structured output is parsed with `IntentSchema.parse()`, which throws a native `ZodError` caught directly by `describeBotRejection`. `OutputParserException` no longer exists on this path.

yuris closed this issue

2026-09-28 20:34:45 +00:00

No labels

No milestone

No project

No assignees

1 participant

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

yuris/garage#84

No description provided.

Rows
Columns