Skip to content

fix: report the real cause when curation output fails to parse - #17

Merged
DamengRandom merged 2 commits into
masterfrom
fix/curator-real-error-cause
Aug 1, 2026
Merged

fix: report the real cause when curation output fails to parse#17
DamengRandom merged 2 commits into
masterfrom
fix/curator-real-error-cause

Conversation

@DamengRandom

Copy link
Copy Markdown
Owner

The grill-me skill is not available in this environment — noted per the task instructions. I did the pressure-test manually instead: wrote each hypothesis below, then read the actual call chain (WorkCoordinator.curateReposcurateTrendingrunCuratorGraph) before touching any code, and killed one hypothesis that the task itself assumed was true.

Findings

The task's framing of failure path 2 was wrong, and that turned out to be the actual root cause.

The task assumes: "an exception propagates out of graph.invoke entirely and is never recorded." That's not what happens. WorkCoordinator.curateRepos (src/agent/index.ts) — the real CurateFn passed to runCuratorGraph in production — already wraps curateTrending in a try/catch and swallows the exception into logger.error(...) (local pino log only) and returns ''. No exception ever reaches the graph. This is the actual origin of the LangSmith symptom in the task description:

  1. curateTrending throws (rate limit, 402/429, empty choices, a withStructuredOutput validation failure — anything).
  2. curateRepos catches it, logs it locally (never sent to LangSmith or Telegram), returns ''.
  3. parseJson('', null)JSON.parse('') throws → fallback null.
  4. CuratedRepoOutputSchema.safeParse(null) → generic "expected object, received null".
  5. This repeats for attempt 2 (same cause persists) → final error is Zod's generic message, curated: null, repos unchanged (matches "[8 items]" in the reported log).

So fixing only curator.graph.ts would have shipped a nicer-looking error message that still never fires in production, because the real exception is discarded one layer up before the graph's curate node is ever called. I fixed both.

Per failure path: before vs after

# Path Before After
1 curate() raw output not JSON Discarded via parseJson's fallback → generic Zod "received null" New message: curator output was not valid JSON (<SyntaxError message>) — <N> chars: <≤300-char excerpt>
2 curate() throws (LLM/provider error, timeout, empty choices, structured-output validation failure) Silently swallowed to '' in WorkCoordinator.curateRepos, never reaches the graph. Local log only; LangSmith/Telegram see the unrelated Zod-null message. curateRepos no longer catches — the graph's curate node now wraps the call in try/catch and records curate() threw: <real message>. llm.ts's failLoudlyOnProviderError already turns OpenRouter's 200-with-no-choices capacity errors into real Errors, so this message is now the actual provider error text, not a generic downstream crash.
3 Well-formed JSON, wrong shape (genuine Zod rejection) parsed.error.message (Zod's issue list) — already meaningful, kept curator output did not match the expected schema: <same Zod message> — same content, just distinguishable by prefix from a parse failure. Investigated but this path is effectively unreachable today: mergeSummaries builds CuratedRepo objects straight from TrendingRepo fields (already scrape-typed) plus a SummarySchema-validated summary/tags, so the shape can't drift from TrendingRepoOutputSchema under the current curateTrending implementation. Kept the distinguishing logic anyway since the task requires it and a future CurateFn swap could hit it.
4 mergeSummaries drops every repo (model returns repo_names that don't match the scrape) → curated: [], not null Parses fine, no error at all Deliberately left unchanged. This isn't the reported failure mode (curated is [], not null) and curateOrNotify already logs it at info level ("No repos curated — skipping send and save") as a skip, not a crash. Adding detection would mean threading per-repo match info out of curateTrending/mergeSummaries into the graph's error channel — a materially bigger change than this task's scope, and it doesn't produce a misleading error today, just a quiet no-op digest. Flagging as a possible follow-up, not fixing here.

Hypotheses killed

  • "Truncated output / markdown fence / refusal produces the non-JSON text" — checked curateTrending: on success it always returns JSON.stringify(...) of a plain object, which can never fail JSON.parse. The only way non-JSON reaches the parser today is the empty string from path 2's swallowed exception, not the model writing prose or hitting a token limit. So the "excerpt" for path 1 in practice will usually be empty-string related unless a future CurateFn implementation changes.
  • "Exception propagates out of graph.invoke and crashes the job" — false, per the root-cause finding above. It's swallowed before the graph, not after.

What changed

  • src/agent/curator.graph.ts: the curate node now catches exceptions from curate(), and separately catches JSON.parse failures (dropped parseJson's silent fallback in favor of a direct try/catch so the real SyntaxError message and a bounded excerpt — truncate(raw, 300) plus raw.length — are preserved). Schema mismatches get their own prefixed message. All three cases are distinguishable by prefix.
  • src/agent/index.ts: removed the try/catch in WorkCoordinator.curateRepos that was swallowing curateTrending's exceptions into '' — this is the actual root cause. The comment already above this method said retries/failure-notification are handled by runCuratorGraph; the try/catch contradicted that and is what broke it.
  • src/agent/curator-graph.test.ts: added 4 tests (non-JSON → parse-failure message + excerpt, long non-JSON → truncated excerpt with reported length, schema mismatch → distinguishable message, curate() throwing → caught and recorded, not crashing).

Verification

$ pnpm install --frozen-lockfile
Lockfile is up to date, resolution step is skipped
... (all deps installed clean)

$ pnpm tsc
> oh-my-workers@1.2.0 tsc
> pnpm exec tsc --noEmit
(no output — clean)

$ pnpm test
...
# tests 52
# suites 0
# pass 52
# fail 0
# cancelled 0
# skipped 0
# todo 0

$ pnpm format:check
> oh-my-workers@1.2.0 format:check
> prettier --check "src/**/*.ts"
Checking formatting...
All matched files use Prettier code style!

All four checks pass. Curator-specific tests (7 total, including the 4 new ones) verified in isolation too — all green.

Not changed, and why

  • mergeSummaries silent-empty case (path 4 above) — see table.
  • parseJson in utils.ts — left untouched; it's still used by kpi-record.ts and index.ts for unrelated parsing where the fallback-swallow behavior is fine. curator.graph.ts no longer uses it, in favor of an inline try/catch that needs the real SyntaxError.
  • Retry feedback content: reused the same error string for both the recorded state.error and the feedback passed back into curateTrending's prompt (rather than authoring two separate messages). The bounded 300-char excerpt is small enough that it isn't the "giant raw dump" the task warns against, and keeping one string is the smaller diff.

Generated by Claude Code

claude added 2 commits July 31, 2026 16:06
The curator node collapsed every failure into Zod's generic "expected
object, received null" by silently discarding the SyntaxError from a
failed JSON.parse. Worse, the real root cause never even reached that
point: WorkCoordinator.curateRepos was already swallowing curateTrending's
exceptions into an empty string, so the actual provider/LLM error was
logged locally and thrown away before the graph ever saw it.

Now the curate node distinguishes curate() throwing, invalid JSON (with
a bounded excerpt + length + the real parse error), and schema mismatches,
and curateRepos lets exceptions propagate so the graph can catch and
report them instead of masking them as a bad parse.
Fallow flagged the curate node's CRAP score (56, threshold 30) — the
try/catch branches added real cyclomatic complexity, and the
`err instanceof Error ? err.message : String(err)` ternary was
duplicated with its non-Error branch untested. Extracted an
errorMessage() helper and the node body into curateNode(), and added a
test that throws a non-Error value so both branches are covered.
@DamengRandom
DamengRandom merged commit f8a7dbe into master Aug 1, 2026
4 checks passed
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.

2 participants