fix: report the real cause when curation output fails to parse - #17
Merged
Conversation
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The
grill-meskill 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.curateRepos→curateTrending→runCuratorGraph) 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.invokeentirely and is never recorded." That's not what happens.WorkCoordinator.curateRepos(src/agent/index.ts) — the realCurateFnpassed torunCuratorGraphin production — already wrapscurateTrendingin a try/catch and swallows the exception intologger.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:curateTrendingthrows (rate limit, 402/429, emptychoices, awithStructuredOutputvalidation failure — anything).curateReposcatches it, logs it locally (never sent to LangSmith or Telegram), returns''.parseJson('', null)→JSON.parse('')throws → fallbacknull.CuratedRepoOutputSchema.safeParse(null)→ generic "expected object, received null".erroris Zod's generic message,curated: null,reposunchanged (matches "[8 items]" in the reported log).So fixing only
curator.graph.tswould 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'scuratenode is ever called. I fixed both.Per failure path: before vs after
curate()raw output not JSONparseJson's fallback → generic Zod "received null"curator output was not valid JSON (<SyntaxError message>) — <N> chars: <≤300-char excerpt>curate()throws (LLM/provider error, timeout, emptychoices, structured-output validation failure)''inWorkCoordinator.curateRepos, never reaches the graph. Local log only; LangSmith/Telegram see the unrelated Zod-null message.curateReposno longer catches — the graph'scuratenode now wraps the call in try/catch and recordscurate() threw: <real message>.llm.ts'sfailLoudlyOnProviderErroralready turns OpenRouter's 200-with-no-choicescapacity errors into realErrors, so this message is now the actual provider error text, not a generic downstream crash.parsed.error.message(Zod's issue list) — already meaningful, keptcurator 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:mergeSummariesbuildsCuratedRepoobjects straight fromTrendingRepofields (already scrape-typed) plus aSummarySchema-validatedsummary/tags, so the shape can't drift fromTrendingRepoOutputSchemaunder the currentcurateTrendingimplementation. Kept the distinguishing logic anyway since the task requires it and a futureCurateFnswap could hit it.mergeSummariesdrops every repo (model returnsrepo_names that don't match the scrape) →curated: [], notnullcuratedis[], notnull) andcurateOrNotifyalready 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 ofcurateTrending/mergeSummariesinto 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
curateTrending: on success it always returnsJSON.stringify(...)of a plain object, which can never failJSON.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 futureCurateFnimplementation changes.graph.invokeand 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: thecuratenode now catches exceptions fromcurate(), and separately catchesJSON.parsefailures (droppedparseJson's silent fallback in favor of a direct try/catch so the realSyntaxErrormessage and a bounded excerpt —truncate(raw, 300)plusraw.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 inWorkCoordinator.curateReposthat was swallowingcurateTrending's exceptions into''— this is the actual root cause. The comment already above this method said retries/failure-notification are handled byrunCuratorGraph; 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
All four checks pass. Curator-specific tests (7 total, including the 4 new ones) verified in isolation too — all green.
Not changed, and why
mergeSummariessilent-empty case (path 4 above) — see table.parseJsoninutils.ts— left untouched; it's still used bykpi-record.tsandindex.tsfor unrelated parsing where the fallback-swallow behavior is fine.curator.graph.tsno longer uses it, in favor of an inline try/catch that needs the realSyntaxError.state.errorand thefeedbackpassed back intocurateTrending'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