dgtlmoon / dgtlmoon/changedetection.io

LLM intent evaluation: malformed JSON suppresses the change, while an unterminated reasoning block passes it through

Open
#4,385 1 comment 0 reactions 0 assignees View on GitHub
Dominant language
Python
Stars
34.1k
Forks
2k
Avg merge
23h 38m
Merged PRs (30d)
67

Description

Following on from #4383, where @apeabody mentioned exploring the fail-open behaviour in a follow-up. While looking at that I ran into a difference in failure handling that seems worth raising on its own.

Your own test already names the tension — `test_unterminated_block_propagates_out_of_parse_eval_response`:

> Deliberately NOT swallowed. `parse_eval_response`'s own fallback is `important=False`, which suppresses the notification - the opposite of what `evaluator.py` wants on failure ("don't suppress the notification"). Letting ValueError escape routes it to that handler instead.

That routes one input class — an unterminated `` block — around the fail-closed default. My question is whether the same reasoning should extend to the other ways a response comes back unusable, since they currently take the opposite path.

### The two paths

`evaluate_change()` passes the change through on the failures it handles itself:

- global monthly budget exceeded — *"Fail open: don't suppress notifications when budget is exhausted"*
- per-watch budget exceeded — *"Already over budget — fail open"*
- any exception escaping the completion/parsing block — *"don't suppress the notification — pass through as important"*

But a `JSONDecodeError` caught inside `parse_eval_response()` returns `important=False`, and in `worker.py` that becomes `changed_detected = False`.

The split comes down to a subclass detail: `json.JSONDecodeError` is a subclass of `ValueError` but not the reverse, so `except (json.JSONDecodeError, AttributeError)` catches decode errors locally while the explicit `raise ValueError(...)` escapes to the fail-open handler.

| response | parsed result | outcome |
|---|---|---|
| prose with no extractable JSON object | `{"important": false, "summary": ""}` | suppressed |
| empty string | `{"important": false, "summary": ""}` | suppressed |
| `{"summary": "price changed"}` — no `important` key | `{"important": false, "summary": "price changed"}` | suppressed |
| truncation leaving invalid JSON after extraction | `{"important": false, "summary": ""}` | suppressed |
| unterminated `` block | raises `ValueError` | **passed through** |

### What follows from the suppression

For a watch that already has history:

- the notification is suppressed
- the candidate snapshot is not saved on that check, since `changed_detected` gates `save_history_blob(...)`
- **the `important: False` result is cached.** The fail-open returns exit before the cache write, but a parse failure returns normally, so it reaches `watch['llm_evaluation_cache'][cache_key] = result` and becomes a durable verdict for that `(intent, diff)` pair.

Ordinary rechecking doesn't guarantee recovery: the text processor advances `previous_md5` (`# Always record the new checksum`) and the worker commits that update before the history gate. To be clear I'm *not* claiming repeated API calls for unchanged content — the cache and the checksum both prevent that. The point is the opposite one: a single malformed response is retained rather than retried.

### Reachability

`client.py` already logs a warning on `finish_reason == 'length'` and returns the text without rejecting it, so a length-truncated response does reach the parser. `JSON_RESPONSE_MAX_TOKENS` is the base value passed through `apply_local_token_multiplier` rather than a universal effective cap, so how tight that is depends on configuration.

### Repro

```python
from changedetectionio.llm.response_parser import parse_eval_response
parse_eval_response("I can't evaluate that.") # {'important': False, 'summary': ''}
parse_eval_response('{"important": true, "summ') # {'important': False, 'summary': ''}
```

### The question

Would you want a malformed response to use the same pass-through policy as an unterminated reasoning block, with an explicit indicator that no usable decision was produced?

That indicator is where this meets the legibility change @apeabody suggested: a result currently carries no marker separating "the model judged this important" from "no usable decision was produced", so the notification layer can't tell which one it received.

One thing worth separating if you take it on: flipping only the exception fallback wouldn't cover row 3. That path decodes successfully and takes `important: False` from `_to_bool(..., default=False)`, so a missing or invalid `important` field is a separate validation-policy decision.

Happy to send a PR once you've decided the policy — the `important=False` fallback is documented in the docstring, so I don't want to assume which way you want it.

Checked against `1d2debef`.

Contributor guide

Open the contributing guide

Research direction

Start with changedetectionio/llm/response_parser and the test named test_unterminated_block_propagates_out_of_parse_eval_response, then trace how evaluator.py and worker.py consume the result. Run the two parse_eval_response repros and inspect client.py's finish_reason handling. Done means the failure policy and unusable-result indicator are explicitly decided and covered for each listed response path.

Written by the indexing model from the issue text.

Assessment

Tech stack
python
Domain
ai, backend
Issue type
Feature
Difficulty
5/5
Estimated time
Over a week
Activity status
Active
Clarity
Needs clarification
Newbie friendliness
45/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.