Handle empty AI content without crashing classification #109

Manually merged
marvin8 merged 9 commits from fix/issue-106-handle-empty-ai-content into main 2026-09-08 08:13:57 +00:00
Collaborator

Problem

Intermittently the model returns HTTP 200 but an empty message.content, and AIClient.complete_json raised a bare JSONDecodeError: Expecting value: line 1 column 1 (char 0). The post stayed AI: pending (safe, but unexplained) and the logs gave no way to distinguish the candidate causes: thinking toggle not honoured (reasoning_content populated, content empty), a content filter, or max_tokens truncation.

Fix

  • complete_json guards the whole choices[0].message.content extraction and parse. Empty / missing / non-JSON content raises a new typed AIMalformedResponseError — after logging a WARNING with finish_reason, a bounded content excerpt (200 chars), and a bounded reasoning_content excerpt (500 chars), all repr-escaped so untrusted model output can neither flood logs nor inject log lines. The cause is visible on the next occurrence.
  • classify_posts (split into _classify_post / _text_verdict / _vision_verdict for the complexity limit) catches the typed error before its generic except Exception and logs a terse one-liner per post, no traceback. The post stays AI: pending for manual review.
  • Deliberately no retry and no request-body changes: if the cause is systematic (e.g. the thinking toggle), retries just double latency; the new diagnostics will pin the cause down with evidence first.

Testing

  • New tests: empty content, missing content, non-JSON content, non-dict bodies, and non-dict choices[0] each raise the typed error (never a bare AttributeError/JSONDecodeError); diagnostics logging asserts finish_reason, bounded content/reasoning excerpts, and the 500/200-char bounds; the raised exception message itself is bounded.
  • Pipeline test: a malformed text-stage reply leaves ai_would_reject / text_flagged / ai_classified_at untouched (pending) and never reaches the vision stage.
  • Full fenliu DoD green after rebase onto #107's merged main (typed-exception helpers + follow_redirects=True coexist): ruff, ruff format, ty (0 diagnostics), complexipy, tryke.

Closes #106

## Problem Intermittently the model returns HTTP 200 but an empty `message.content`, and `AIClient.complete_json` raised a bare `JSONDecodeError: Expecting value: line 1 column 1 (char 0)`. The post stayed `AI: pending` (safe, but unexplained) and the logs gave no way to distinguish the candidate causes: thinking toggle not honoured (`reasoning_content` populated, `content` empty), a content filter, or `max_tokens` truncation. ## Fix - `complete_json` guards the whole `choices[0].message.content` extraction and parse. Empty / missing / non-JSON content raises a new typed `AIMalformedResponseError` — after logging a WARNING with `finish_reason`, a bounded `content` excerpt (200 chars), and a bounded `reasoning_content` excerpt (500 chars), all repr-escaped so untrusted model output can neither flood logs nor inject log lines. The cause is visible on the next occurrence. - `classify_posts` (split into `_classify_post` / `_text_verdict` / `_vision_verdict` for the complexity limit) catches the typed error before its generic `except Exception` and logs a terse one-liner per post, no traceback. The post stays `AI: pending` for manual review. - Deliberately **no retry and no request-body changes**: if the cause is systematic (e.g. the thinking toggle), retries just double latency; the new diagnostics will pin the cause down with evidence first. ## Testing - New tests: empty content, missing content, non-JSON content, non-dict bodies, and non-dict `choices[0]` each raise the typed error (never a bare `AttributeError`/`JSONDecodeError`); diagnostics logging asserts `finish_reason`, bounded content/reasoning excerpts, and the 500/200-char bounds; the raised exception message itself is bounded. - Pipeline test: a malformed text-stage reply leaves `ai_would_reject` / `text_flagged` / `ai_classified_at` untouched (pending) and never reaches the vision stage. - Full fenliu DoD green after rebase onto #107's merged main (typed-exception helpers + `follow_redirects=True` coexist): ruff, ruff format, ty (0 diagnostics), complexipy, tryke. Closes #106
🐛 (fenliu) leave post pending and log diagnostics when AI returns empty content
Some checks failed
/ gitleaks (pull_request) Successful in 17s
/ checks (pull_request) Failing after 2m25s
/ publish (pull_request) Has been skipped
/ publish-container (pull_request) Has been skipped
/ deploy-docs (pull_request) Has been skipped
/ pr-review (pull_request) Successful in 5m45s
511a6132a4
complete_json now guards the whole choices[0].message.content
extraction and parse: empty, missing, or non-JSON content raises
AIMalformedResponseError after logging a WARNING with finish_reason,
the raw content repr, and a bounded reasoning_content excerpt, so the
candidate causes (thinking toggle ignored, content filter, token
truncation) are distinguishable on the next occurrence. classify_posts
catches the typed error before its generic handler and logs a terse
one-liner — the post stays AI: pending for manual review. No retry;
that decision is deferred until the logs reveal the actual cause.

Closes #106
forgejo-actions left a comment

WuMing

Found 2 issue(s). See inline comments below.

## WuMing Found **2** issue(s). See inline comments below.
@ -10,3 +10,4 @@
---
Occasionally the AI model answers with HTTP 200 but an empty message body, and the classifier died with a cryptic `JSONDecodeError`. Those posts now simply stay pending for manual review, accompanied by a clear warning; the model's finish reason, raw content, and a bounded reasoning excerpt are logged so the underlying cause is visible the next time it happens.

docs [LOW]

The described failure is an empty message.content field inside the model's JSON response, not an empty HTTP message body. Reword to something like 'HTTP 200 but an empty message.content field' so the release note matches the actual fix and doesn't mislead readers into thinking the HTTP response body was empty.

**docs** [LOW] The described failure is an empty `message.content` field inside the model's JSON response, not an empty HTTP message body. Reword to something like 'HTTP 200 but an empty `message.content` field' so the release note matches the actual fix and doesn't mislead readers into thinking the HTTP response body was empty. <!-- wuming:sha256:b3c2791035f131bc86a3bd81ca3b57850ef41a0dff0ef9f570966931a7d4c945 -->
@ -162,0 +201,4 @@
except json.JSONDecodeError:
pass
logger.warning(
"Model reply has no parsable JSON content: %s", _malformed_reply_diagnostics(payload, reply_content)

security [MEDIUM]

A09: The raw model reply content is written to WARNING logs via _malformed_reply_diagnostics. On a malformed response this content may echo or contain post text/PII from the user content sent to the classifier, leaking sensitive data into logs. Log only redacted/bounded metadata such as length, hash, finish_reason, or a safe sanitized excerpt.

**security** [MEDIUM] A09: The raw model reply content is written to WARNING logs via _malformed_reply_diagnostics. On a malformed response this content may echo or contain post text/PII from the user content sent to the classifier, leaking sensitive data into logs. Log only redacted/bounded metadata such as length, hash, finish_reason, or a safe sanitized excerpt. <!-- wuming:sha256:2e0da06270b810fc8ebba1dbc061faae406c728269b3295c6ea82730b057b505 -->
♻️ (fenliu) split classify_posts per stage to satisfy complexity limit
All checks were successful
/ gitleaks (pull_request) Successful in 14s
/ checks (pull_request) Successful in 2m36s
/ publish (pull_request) Has been skipped
/ publish-container (pull_request) Has been skipped
/ deploy-docs (pull_request) Has been skipped
/ pr-review (pull_request) Successful in 4m33s
7146eb314f
Extract _classify_post, _text_verdict, and _vision_verdict so each
unit stays under the complexipy threshold; behaviour is unchanged and
each failure path still leaves the post pending for manual review.
forgejo-actions left a comment

WuMing

Found 2 issue(s). See inline comments below.

## WuMing Found **2** issue(s). See inline comments below.
@ -5,3 +5,3 @@
fenliu 3.0.0
-
- Improved: AI replies that arrive empty or unparseable now leave the post pending with a clear log line and full response diagnostics instead of a JSON traceback.

docs [LOW]

'full response diagnostics' overstates what is logged: the reasoning excerpt is bounded to 500 characters, not the full response. Consider saying 'key response diagnostics' or listing the specific fields logged.

**docs** [LOW] 'full response diagnostics' overstates what is logged: the reasoning excerpt is bounded to 500 characters, not the full response. Consider saying 'key response diagnostics' or listing the specific fields logged. <!-- wuming:sha256:8225a4cdb63d9e5abe9529c341bee3c182c92e96dba3f9566fb2943bd64930db -->
@ -57,0 +84,4 @@
reasoning = str(message.get("reasoning_content") or "")
excerpt = reasoning[:_REASONING_EXCERPT_LIMIT]
return (
f"finish_reason={finish_reason} content={content!r} "

security [MEDIUM]

A09: The raw model reply content is written to WARNING logs via this diagnostics string. The reply is untrusted and may contain user-influenced or PII data; it is also not length-limited. Additionally, finish_reason is interpolated without validation or sanitisation, allowing log injection if an upstream or malicious response contains newlines or control characters. Remediate by redacting or truncating content, validating finish_reason against a known set, and emitting structured logs with control-character escaping.

**security** [MEDIUM] A09: The raw model reply content is written to WARNING logs via this diagnostics string. The reply is untrusted and may contain user-influenced or PII data; it is also not length-limited. Additionally, finish_reason is interpolated without validation or sanitisation, allowing log injection if an upstream or malicious response contains newlines or control characters. Remediate by redacting or truncating content, validating finish_reason against a known set, and emitting structured logs with control-character escaping. <!-- wuming:sha256:2040ee362fc0fc9320abf67d31e1907087227e35973c56b117e7dd32e16db2dd -->
🚨 (fenliu) bound and repr-escape model reply diagnostics
All checks were successful
/ gitleaks (pull_request) Successful in 13s
/ checks (pull_request) Successful in 2m36s
/ publish (pull_request) Has been skipped
/ publish-container (pull_request) Has been skipped
/ deploy-docs (pull_request) Has been skipped
/ pr-review (pull_request) Successful in 4m39s
4bcbb4f9b0
Address WuMing review: the WARNING diagnostics could log unbounded
model reply content, which may echo untrusted post text. Content is
now excerpted to 200 chars, finish_reason to 100, reasoning to 500,
all repr-escaped so control characters cannot inject log lines.
Reword the release note: the failure is an empty message.content
field, not an empty HTTP body, and the diagnostics are key fields,
not the full response.
Author
Collaborator

WuMing review waves — responses

Release-Notes.md

The described failure is an empty message.content field inside the model's JSON response, not an empty HTTP message body…

Reworded in commit 4bcbb4f: the note now says 'an empty message.content field'.


Release-Notes.md

'full response diagnostics' overstates what is logged: the reasoning excerpt is bounded to 500 characters…

Reworded in commit 4bcbb4f: the bullet now says 'key response diagnostics' and the detail post lists the bounded fields.


ai_classification.py

A09: The raw model reply content is written to WARNING logs… leaking sensitive data into logs.

Fixed in commit 4bcbb4f: content is now excerpted to 200 chars, finish_reason to 100, reasoning_content to 500 — all repr-escaped, so untrusted model output can neither flood logs with echoed post text nor inject log lines via control characters. finish_reason, presence of reasoning, and the bounded excerpts still distinguish the candidate causes.


ai_classification.py

A09: …not length-limited. Additionally, finish_reason is interpolated without validation… log injection if a malicious response contains newlines…

Fixed in commit 4bcbb4f: all three free-text fields are length-bounded and repr-escaped (repr neutralises newlines/control characters, so injection is prevented at the formatting layer); the full reply body is never logged.

### WuMing review waves — responses [`Release-Notes.md`](https://forge.marvin8.zone/marvin8/dujiangyan/pulls/109#issuecomment-2229) > The described failure is an empty `message.content` field inside the model's JSON response, not an empty HTTP message body… ✅ Reworded in commit `4bcbb4f`: the note now says 'an empty `message.content` field'. --- [`Release-Notes.md`](https://forge.marvin8.zone/marvin8/dujiangyan/pulls/109#issuecomment-2233) > 'full response diagnostics' overstates what is logged: the reasoning excerpt is bounded to 500 characters… ✅ Reworded in commit `4bcbb4f`: the bullet now says 'key response diagnostics' and the detail post lists the bounded fields. --- [`ai_classification.py`](https://forge.marvin8.zone/marvin8/dujiangyan/pulls/109#issuecomment-2230) > A09: The raw model reply content is written to WARNING logs… leaking sensitive data into logs. ✅ Fixed in commit `4bcbb4f`: `content` is now excerpted to 200 chars, `finish_reason` to 100, `reasoning_content` to 500 — all `repr`-escaped, so untrusted model output can neither flood logs with echoed post text nor inject log lines via control characters. `finish_reason`, presence of reasoning, and the bounded excerpts still distinguish the candidate causes. --- [`ai_classification.py`](https://forge.marvin8.zone/marvin8/dujiangyan/pulls/109#issuecomment-2234) > A09: …not length-limited. Additionally, finish_reason is interpolated without validation… log injection if a malicious response contains newlines… ✅ Fixed in commit `4bcbb4f`: all three free-text fields are length-bounded and `repr`-escaped (repr neutralises newlines/control characters, so injection is prevented at the formatting layer); the full reply body is never logged.
forgejo-actions left a comment

WuMing

Found 3 issue(s). See inline comments below.

## WuMing Found **3** issue(s). See inline comments below.
@ -10,3 +10,4 @@
---
Occasionally the AI model answers with HTTP 200 but an empty `message.content` field, and the classifier died with a cryptic `JSONDecodeError`. Those posts now simply stay pending for manual review, accompanied by a clear warning; the model's finish reason plus bounded excerpts of its content and reasoning are logged so the underlying cause is visible the next time it happens.

docs [MEDIUM]

The release note says 'bounded excerpts of its content and reasoning are logged', but the described behavior logs the raw content repr (not bounded) and a bounded 500-character reasoning_content excerpt. Reword to avoid implying the content excerpt is also bounded, e.g. 'the model's finish reason plus the raw content and a bounded reasoning excerpt are logged'.

**docs** [MEDIUM] The release note says 'bounded excerpts of its content and reasoning are logged', but the described behavior logs the raw `content` repr (not bounded) and a bounded 500-character `reasoning_content` excerpt. Reword to avoid implying the content excerpt is also bounded, e.g. 'the model's finish reason plus the raw content and a bounded reasoning excerpt are logged'. <!-- wuming:sha256:40d16fe9fe07d3f84672e0ec850c91d10b007404e906c249e6ce80cb359d4132 -->
@ -162,0 +205,4 @@
except json.JSONDecodeError:
pass
logger.warning(
"Model reply has no parsable JSON content: %s", _malformed_reply_diagnostics(payload, reply_content)

security [MEDIUM]

A09: The diagnostics log includes raw excerpts of the model's content and reasoning_content fields. These are untrusted model outputs that may echo user post text/PII or include chain-of-thought. While the values are repr-escaped and length-limited (which prevents log injection/line flooding), the underlying sensitive data is still written to logs. Log only non-sensitive diagnostics such as finish_reason, lengths, or a hash; avoid logging raw model output.

**security** [MEDIUM] A09: The diagnostics log includes raw excerpts of the model's content and reasoning_content fields. These are untrusted model outputs that may echo user post text/PII or include chain-of-thought. While the values are repr-escaped and length-limited (which prevents log injection/line flooding), the underlying sensitive data is still written to logs. Log only non-sensitive diagnostics such as finish_reason, lengths, or a hash; avoid logging raw model output. <!-- wuming:sha256:f3c9f16abd4dd272678922630946c6ceeac71ec988ee265b97503d8c95dd1587 -->
@ -52,0 +128,4 @@
diagnostics = " ".join(record.getMessage() for record in collector.records)
expect("finish_reason='content_filter'" in diagnostics, "finish_reason logged").to_be_truthy()
expect("reasoning_content" in diagnostics, "reasoning_content noted").to_be_truthy()

tests [LOW]

This assertion checks for the literal substring 'reasoning_content', but that word is a constant part of the diagnostics format and appears whether reasoning content is present or absent. It cannot distinguish the intended condition. Assert on 'reasoning_content=present' instead, or rely on the excerpt assertions already present.

**tests** [LOW] This assertion checks for the literal substring 'reasoning_content', but that word is a constant part of the diagnostics format and appears whether reasoning content is present or absent. It cannot distinguish the intended condition. Assert on 'reasoning_content=present' instead, or rely on the excerpt assertions already present. <!-- wuming:sha256:5e99d7e8cbe4067a3bfa0182bc30226b95770e66426b89fad4ad2e6f56ed586a -->
(fenliu) assert reasoning_content=present in diagnostics test
All checks were successful
/ gitleaks (pull_request) Successful in 18s
/ checks (pull_request) Successful in 2m39s
/ publish (pull_request) Has been skipped
/ publish-container (pull_request) Has been skipped
/ deploy-docs (pull_request) Has been skipped
/ pr-review (pull_request) Successful in 4m35s
fcc4f7d209
The previous substring assertion could never fail because the key
name is a constant part of the diagnostics format; assert on the
present marker instead.
Author
Collaborator

WuMing wave 3 — responses

Release-Notes.md

…the described behavior logs the raw content repr (not bounded)…

🔴 The premise no longer matches the code: commit 4bcbb4f (the same commit that reworded this note) bounds content to 200 chars and finish_reason to 100, both repr-escaped. 'Bounded excerpts of its content and reasoning' is exactly what the current implementation logs, so the note is accurate as written.


ai_classification.py

A09: …Log only non-sensitive diagnostics such as finish_reason, lengths, or a hash; avoid logging raw model output.

🔴 Declining after explicit owner review. These are WARNING-level entries in the operator's private log, bounded (200/500 chars) and repr-escaped so they cannot inject or flood log lines; hashes and lengths cannot answer why the model returned empty content, which was the purpose of the logging (see #106). The bounded excerpts are a deliberate traceability trade-off, re-examined twice — happy to revisit if a concrete leak path shows up.


test_ai_classification.py

This assertion checks for the literal substring 'reasoning_content', but that word is a constant part of the diagnostics format…

Fixed in commit fcc4f7d: the test now asserts reasoning_content=present.

### WuMing wave 3 — responses [`Release-Notes.md`](https://forge.marvin8.zone/marvin8/dujiangyan/pulls/109#issuecomment-2238) > …the described behavior logs the raw `content` repr (not bounded)… 🔴 The premise no longer matches the code: commit `4bcbb4f` (the same commit that reworded this note) bounds `content` to 200 chars and `finish_reason` to 100, both `repr`-escaped. 'Bounded excerpts of its content and reasoning' is exactly what the current implementation logs, so the note is accurate as written. --- [`ai_classification.py`](https://forge.marvin8.zone/marvin8/dujiangyan/pulls/109#issuecomment-2239) > A09: …Log only non-sensitive diagnostics such as finish_reason, lengths, or a hash; avoid logging raw model output. 🔴 Declining after explicit owner review. These are WARNING-level entries in the operator's private log, bounded (200/500 chars) and `repr`-escaped so they cannot inject or flood log lines; hashes and lengths cannot answer *why* the model returned empty content, which was the purpose of the logging (see #106). The bounded excerpts are a deliberate traceability trade-off, re-examined twice — happy to revisit if a concrete leak path shows up. --- [`test_ai_classification.py`](https://forge.marvin8.zone/marvin8/dujiangyan/pulls/109#issuecomment-2240) > This assertion checks for the literal substring 'reasoning_content', but that word is a constant part of the diagnostics format… ✅ Fixed in commit `fcc4f7d`: the test now asserts `reasoning_content=present`.
forgejo-actions left a comment

WuMing

Found 3 issue(s). See inline comments below.

## WuMing Found **3** issue(s). See inline comments below.
@ -57,0 +84,4 @@
message = envelope.get("message") if isinstance(envelope, dict) else {}
if not isinstance(message, dict):
message = {}
finish_reason = str(envelope.get("finish_reason", "unknown"))[:_FINISH_REASON_LIMIT]

code [MEDIUM]

_malformed_reply_diagnostics assumes choices[0] is a dict, but line 83 can set envelope to a non-dict when choices[0] is e.g. a list. A malformed payload such as {"choices":} then raises AttributeError from this diagnostics helper instead of the intended AIMalformedResponseError, and the diagnostic warning is never logged. Guard with if not isinstance(envelope, dict): envelope = {} before calling envelope.get.

**code** [MEDIUM] _malformed_reply_diagnostics assumes choices[0] is a dict, but line 83 can set envelope to a non-dict when choices[0] is e.g. a list. A malformed payload such as {"choices":[[]]} then raises AttributeError from this diagnostics helper instead of the intended AIMalformedResponseError, and the diagnostic warning is never logged. Guard with `if not isinstance(envelope, dict): envelope = {}` before calling envelope.get. <!-- wuming:sha256:52b0e0b128eb15dc54046413fcf73f540c860e3efeb054029f6f1f63ac9ff33c -->
@ -162,0 +207,4 @@
logger.warning(
"Model reply has no parsable JSON content: %s", _malformed_reply_diagnostics(payload, reply_content)
)
raise AIMalformedResponseError(f"Model reply has no parsable JSON content: {reply_content!r}")

code [MEDIUM]

Raising AIMalformedResponseError with the raw reply_content embeds the entire untrusted model output in the exception message. This can be very large and may leak post/model text if the exception is ever logged with a traceback. Reuse the bounded excerpt from _malformed_reply_diagnostics or slice reply_content to _CONTENT_EXCERPT_LIMIT before interpolating.

**code** [MEDIUM] Raising AIMalformedResponseError with the raw reply_content embeds the entire untrusted model output in the exception message. This can be very large and may leak post/model text if the exception is ever logged with a traceback. Reuse the bounded excerpt from _malformed_reply_diagnostics or slice reply_content to _CONTENT_EXCERPT_LIMIT before interpolating. <!-- wuming:sha256:ae66fcb1ebedcecc914969fcfcacfacef3ab209de8c898d6ee1c4b075b9b4a9c -->
@ -162,0 +221,4 @@
or not a string.
"""
choices = payload.get("choices")

code [LOW]

_message_content is documented as tolerating a malformed envelope, but it calls payload.get without first verifying payload is a dict. If response.json() returns a list, null, or another non-dict JSON value, complete_json raises AttributeError instead of AIMalformedResponseError and no diagnostics are logged. Add if not isinstance(payload, dict): return None at the start.

**code** [LOW] _message_content is documented as tolerating a malformed envelope, but it calls payload.get without first verifying payload is a dict. If response.json() returns a list, null, or another non-dict JSON value, complete_json raises AttributeError instead of AIMalformedResponseError and no diagnostics are logged. Add `if not isinstance(payload, dict): return None` at the start. <!-- wuming:sha256:59c38782d774953eec1193eb5439d2e19db0017771b1fc0e5e36d2631c016f7c -->
🚨 (fenliu) harden malformed AI reply envelope handling
Some checks failed
/ gitleaks (pull_request) Successful in 16s
/ checks (pull_request) Failing after 2m27s
/ publish (pull_request) Has been skipped
/ publish-container (pull_request) Has been skipped
/ deploy-docs (pull_request) Has been skipped
/ pr-review (pull_request) Successful in 4m36s
9b0c3fb5b6
- _message_content and _malformed_reply_diagnostics tolerate a
  non-dict JSON body (list/null) and a non-dict choices[0] entry,
  raising AIMalformedResponseError with diagnostics instead of
  leaking AttributeError.
- The raised exception message now carries only the bounded content
  excerpt, never the full untrusted reply.
Author
Collaborator

WuMing wave 4 — responses

ai_classification.py

Raising AIMalformedResponseError with the raw reply_content embeds the entire untrusted model output in the exception message…

Fixed in commit 9b0c3fb: the message now carries only the 200-char bounded excerpt, consistent with the log policy.


ai_classification.py

…it calls payload.get without first verifying payload is a dict. If response.json() returns a list, null…

Fixed in commit 9b0c3fb: _message_content returns None for non-dict bodies, and _malformed_reply_diagnostics normalises them too, so the typed error and diagnostics are produced in every malformed-envelope case (covered by new tests).


ai_classification.py

…line 83 can set envelope to a non-dict when choices[0] is e.g. a list… raises AttributeError from this diagnostics helper…

Fixed in commit 9b0c3fb: envelope is normalised to an empty dict before any .get, and a regression test pins {"choices":[[]]} → typed error + diagnostics instead of AttributeError.

### WuMing wave 4 — responses [`ai_classification.py`](https://forge.marvin8.zone/marvin8/dujiangyan/pulls/109#issuecomment-2246) > Raising AIMalformedResponseError with the raw reply_content embeds the entire untrusted model output in the exception message… ✅ Fixed in commit `9b0c3fb`: the message now carries only the 200-char bounded excerpt, consistent with the log policy. --- [`ai_classification.py`](https://forge.marvin8.zone/marvin8/dujiangyan/pulls/109#issuecomment-2247) > …it calls payload.get without first verifying payload is a dict. If response.json() returns a list, null… ✅ Fixed in commit `9b0c3fb`: `_message_content` returns None for non-dict bodies, and `_malformed_reply_diagnostics` normalises them too, so the typed error and diagnostics are produced in every malformed-envelope case (covered by new tests). --- [`ai_classification.py`](https://forge.marvin8.zone/marvin8/dujiangyan/pulls/109#issuecomment-2245) > …line 83 can set envelope to a non-dict when choices[0] is e.g. a list… raises AttributeError from this diagnostics helper… ✅ Fixed in commit `9b0c3fb`: `envelope` is normalised to an empty dict before any `.get`, and a regression test pins `{"choices":[[]]}` → typed error + diagnostics instead of `AttributeError`.
(fenliu) force logger level in diagnostics log-capture tests
All checks were successful
/ gitleaks (pull_request) Successful in 12s
/ checks (pull_request) Successful in 2m42s
/ publish (pull_request) Has been skipped
/ publish-container (pull_request) Has been skipped
/ deploy-docs (pull_request) Has been skipped
/ pr-review (pull_request) Successful in 5m23s
68f5d49585
The collector tests attached a handler without setting the logger's
own level, so the effective level inherited whatever a previously-run
test left configured; when that level filtered WARNING the records
never reached the collector (CI only, workers 3.13/3.14). Pin the
level for the duration and restore it afterwards.
agent-pi force-pushed fix/issue-106-handle-empty-ai-content from 68f5d49585
All checks were successful
/ gitleaks (pull_request) Successful in 12s
/ checks (pull_request) Successful in 2m42s
/ publish (pull_request) Has been skipped
/ publish-container (pull_request) Has been skipped
/ deploy-docs (pull_request) Has been skipped
/ pr-review (pull_request) Successful in 5m23s
to 152b710322
All checks were successful
/ publish (pull_request) Has been skipped
/ publish-container (pull_request) Has been skipped
/ deploy-docs (pull_request) Has been skipped
/ pr-review (pull_request) Successful in 4m22s
/ gitleaks (pull_request) Successful in 25s
/ checks (pull_request) Successful in 3m1s
2026-09-08 04:41:41 +00:00
Compare
forgejo-actions left a comment

WuMing

Found 2 issue(s). See inline comments below.

## WuMing Found **2** issue(s). See inline comments below.
@ -12,6 +13,8 @@ fenliu 3.0.0
Some Mastodon attachments are served through a redirecting `media_proxy` URL rather than a direct file link. The AI vision stage's image downloader treated those redirects as errors, so posts with proxy-served media never received a vision verdict and had to be reviewed by hand. It now follows the redirect to the real file and infers the image type from the final URL's file extension, so proxied PNG/WebP/GIF attachments are labelled correctly instead of always falling back to `image/jpeg`.
Occasionally the AI model answers with HTTP 200 but an empty `message.content` field, and the classifier died with a cryptic `JSONDecodeError`. Those posts now simply stay pending for manual review, accompanied by a clear warning; the model's finish reason plus bounded excerpts of its content and reasoning are logged so the underlying cause is visible the next time it happens.

docs [LOW]

Mixed tense in the first sentence: 'answers' is present but 'died' is past. Make the historical description consistent, e.g. 'Occasionally the AI model would answer with HTTP 200 but an empty message.content field, and the classifier died with a cryptic JSONDecodeError.'

**docs** [LOW] Mixed tense in the first sentence: 'answers' is present but 'died' is past. Make the historical description consistent, e.g. 'Occasionally the AI model would answer with HTTP 200 but an empty `message.content` field, and the classifier died with a cryptic `JSONDecodeError`.' <!-- wuming:sha256:9ba0f63be6f0e9ab7bb2f13c46b743a66222f0f67b3cbec058002049758065a4 -->
@ -162,0 +209,4 @@
except json.JSONDecodeError:
pass
logger.warning(
"Model reply has no parsable JSON content: %s", _malformed_reply_diagnostics(payload, reply_content)

security [LOW]

A09: This warning logs raw model reply excerpts (content and reasoning_content) without redaction. Although length-bounded and repr-escaped against log injection, these excerpts can echo user post text/PII from the classification prompt. Log only non-sensitive metadata such as finish_reason, lengths, or hashes, or redact free-text model output.

**security** [LOW] A09: This warning logs raw model reply excerpts (content and reasoning_content) without redaction. Although length-bounded and repr-escaped against log injection, these excerpts can echo user post text/PII from the classification prompt. Log only non-sensitive metadata such as finish_reason, lengths, or hashes, or redact free-text model output. <!-- wuming:sha256:b556a9ea97987e3bce2c9b7101dde0f72ff9b20303358105058d3c641c1cc266 -->
marvin8 marked this conversation as resolved
forgejo-actions left a comment

WuMing

Found 1 issue(s). See inline comments below.

## WuMing Found **1** issue(s). See inline comments below.
@ -12,6 +13,8 @@ fenliu 3.0.0
Some Mastodon attachments are served through a redirecting `media_proxy` URL rather than a direct file link. The AI vision stage's image downloader treated those redirects as errors, so posts with proxy-served media never received a vision verdict and had to be reviewed by hand. It now follows the redirect to the real file and infers the image type from the final URL's file extension, so proxied PNG/WebP/GIF attachments are labelled correctly instead of always falling back to `image/jpeg`.
Occasionally the AI model answers with HTTP 200 but an empty `message.content` field, and the classifier died with a cryptic `JSONDecodeError`. Those posts now simply stay pending for manual review, accompanied by a clear warning; the model's finish reason plus bounded excerpts of its content and reasoning are logged so the underlying cause is visible the next time it happens.

docs [LOW]

The release-note sentence says “bounded excerpts of its content and reasoning are logged,” but the PR description says the raw content repr is logged and only the reasoning_content excerpt is bounded to 500 characters. Revise the wording so it doesn’t imply both are bounded unless the implementation was changed to bound the content too.

**docs** [LOW] The release-note sentence says “bounded excerpts of its content and reasoning are logged,” but the PR description says the raw `content` repr is logged and only the `reasoning_content` excerpt is bounded to 500 characters. Revise the wording so it doesn’t imply both are bounded unless the implementation was changed to bound the content too. <!-- wuming:sha256:9ba0f63be6f0e9ab7bb2f13c46b743a66222f0f67b3cbec058002049758065a4 -->
📝 (fenliu) fix tense in empty-content release note
All checks were successful
/ gitleaks (pull_request) Successful in 12s
/ checks (pull_request) Successful in 2m38s
/ publish (pull_request) Has been skipped
/ publish-container (pull_request) Has been skipped
/ deploy-docs (pull_request) Has been skipped
/ pr-review (pull_request) Successful in 4m34s
765733a2df
Author
Collaborator

WuMing waves 209–210 (post-rebase) — responses

ai_classification.py

A09: This warning logs raw model reply excerpts (content and reasoning_content) without redaction…

🔴 Declining — settled with the owner during wave 3 (see the earlier consolidated reply). The excerpts are length-bounded (200/500 chars) and repr-escaped, live only in the operator's private log, and are the deliberate traceability trade-off for diagnosing why the model returns empty content; hashes/lengths cannot answer that. Re-raising on a fresh diff does not change the decision.


Release-Notes.md

Mixed tense in the first sentence: 'answers' is present but 'died' is past…

Fixed: now reads 'Occasionally the AI model would answer with HTTP 200 but an empty message.content field'.


Release-Notes.md

…but the PR description says the raw content repr is logged and only the reasoning_content excerpt is bounded to 500 characters…

The release note is accurate as written — the implementation WAS changed (commit 4bcbb4f bounds content to 200 chars and finish_reason to 100, both repr-escaped). The stale text was in the PR description, which I have now corrected; no note change needed.

### WuMing waves 209–210 (post-rebase) — responses [`ai_classification.py`](https://forge.marvin8.zone/marvin8/dujiangyan/pulls/109#issuecomment-2293) > A09: This warning logs raw model reply excerpts (content and reasoning_content) without redaction… 🔴 Declining — settled with the owner during wave 3 (see the earlier consolidated reply). The excerpts are length-bounded (200/500 chars) and repr-escaped, live only in the operator's private log, and are the deliberate traceability trade-off for diagnosing *why* the model returns empty content; hashes/lengths cannot answer that. Re-raising on a fresh diff does not change the decision. --- [`Release-Notes.md`](https://forge.marvin8.zone/marvin8/dujiangyan/pulls/109#issuecomment-2292) > Mixed tense in the first sentence: 'answers' is present but 'died' is past… ✅ Fixed: now reads 'Occasionally the AI model would answer with HTTP 200 but an empty `message.content` field'. --- [`Release-Notes.md`](https://forge.marvin8.zone/marvin8/dujiangyan/pulls/109#issuecomment-2295) > …but the PR description says the raw `content` repr is logged and only the `reasoning_content` excerpt is bounded to 500 characters… ✅ The release note is accurate as written — the implementation WAS changed (commit `4bcbb4f` bounds `content` to 200 chars and `finish_reason` to 100, both repr-escaped). The stale text was in the PR *description*, which I have now corrected; no note change needed.
forgejo-actions left a comment

WuMing

Found 2 issue(s). See inline comments below.

## WuMing Found **2** issue(s). See inline comments below.
@ -12,6 +13,8 @@ fenliu 3.0.0
Some Mastodon attachments are served through a redirecting `media_proxy` URL rather than a direct file link. The AI vision stage's image downloader treated those redirects as errors, so posts with proxy-served media never received a vision verdict and had to be reviewed by hand. It now follows the redirect to the real file and infers the image type from the final URL's file extension, so proxied PNG/WebP/GIF attachments are labelled correctly instead of always falling back to `image/jpeg`.
Occasionally the AI model would answer with HTTP 200 but an empty `message.content` field, and the classifier died with a cryptic `JSONDecodeError`. Those posts now simply stay pending for manual review, accompanied by a clear warning; the model's finish reason plus bounded excerpts of its content and reasoning are logged so the underlying cause is visible the next time it happens.

docs [LOW]

The fix logs the raw content repr and only bounds the reasoning_content excerpt to 500 chars, so describing both as 'bounded excerpts of its content and reasoning' is inaccurate. Consider wording it as 'the model's finish reason, the raw content repr, and a 500-character excerpt of its reasoning' to match the actual diagnostics.

**docs** [LOW] The fix logs the raw content repr and only bounds the reasoning_content excerpt to 500 chars, so describing both as 'bounded excerpts of its content and reasoning' is inaccurate. Consider wording it as 'the model's finish reason, the raw content repr, and a 500-character excerpt of its reasoning' to match the actual diagnostics. <!-- wuming:sha256:aacaeee2066851bb3cd6282e85f72e33aa88872578244c66e521bf9f98d73b78 -->
@ -57,0 +92,4 @@
reasoning = str(message.get("reasoning_content") or "")
content_excerpt = (content or "")[:_CONTENT_EXCERPT_LIMIT]
return (
f"finish_reason={finish_reason!r} content={content_excerpt!r} "

security [LOW]

A09: This diagnostic string includes raw model reply content and reasoning_content, which may contain user post text/PII echoed from the prompt. Bounding length and repr-escaping prevent log flooding/injection but do not prevent sensitive PII disclosure. Log only non-sensitive metadata such as finish_reason and content presence/length, or apply PII redaction before logging.

**security** [LOW] A09: This diagnostic string includes raw model reply content and reasoning_content, which may contain user post text/PII echoed from the prompt. Bounding length and repr-escaping prevent log flooding/injection but do not prevent sensitive PII disclosure. Log only non-sensitive metadata such as finish_reason and content presence/length, or apply PII redaction before logging. <!-- wuming:sha256:fb8eb9e58b1be57816b358efb70eb4464503301df6a3db9d82c47062021aa195 -->
marvin8 marked this conversation as resolved
Author
Collaborator

WuMing wave 211 — responses

Release-Notes.md

The fix logs the raw content repr and only bounds the reasoning_content excerpt to 500 chars…

🔴 The premise doesn't match the current code: since commit 4bcbb4f _malformed_reply_diagnostics bounds content to 200 chars (_CONTENT_EXCERPT_LIMIT) and finish_reason to 100, all repr-escaped — 'bounded excerpts of its content and reasoning' describes the implementation exactly. The finding appears to have been reviewed against a pre-4bcbb4f view of this file.


ai_classification.py

A09: …Log only non-sensitive metadata such as finish_reason and content presence/length, or apply PII redaction…

🔴 Declining — same finding as #issuecomment-2293/2230/2239, already reviewed and decided with the owner: the bounded, repr-escaped excerpts in the operator's private log are the deliberate traceability trade-off (see the wave-3 consolidated reply). No change.

### WuMing wave 211 — responses [`Release-Notes.md`](https://forge.marvin8.zone/marvin8/dujiangyan/pulls/109#issuecomment-2300) > The fix logs the raw content repr and only bounds the reasoning_content excerpt to 500 chars… 🔴 The premise doesn't match the current code: since commit `4bcbb4f` `_malformed_reply_diagnostics` bounds `content` to 200 chars (`_CONTENT_EXCERPT_LIMIT`) and `finish_reason` to 100, all repr-escaped — 'bounded excerpts of its content and reasoning' describes the implementation exactly. The finding appears to have been reviewed against a pre-`4bcbb4f` view of this file. --- [`ai_classification.py`](https://forge.marvin8.zone/marvin8/dujiangyan/pulls/109#issuecomment-2301) > A09: …Log only non-sensitive metadata such as finish_reason and content presence/length, or apply PII redaction… 🔴 Declining — same finding as #issuecomment-2293/2230/2239, already reviewed and decided with the owner: the bounded, repr-escaped excerpts in the operator's private log are the deliberate traceability trade-off (see the wave-3 consolidated reply). No change.
🐛 (fenliu) keep exception tracebacks in the console log
All checks were successful
/ gitleaks (pull_request) Successful in 25s
/ checks (pull_request) Successful in 2m52s
/ publish (pull_request) Has been skipped
/ publish-container (pull_request) Has been skipped
/ deploy-docs (pull_request) Has been skipped
/ pr-review (pull_request) Successful in 4m29s
eca12f1b11
ContextualLogFormatter built its own output string and dropped
record.exc_info, so every logger.warning(..., exc_info=True) emitted
message-only lines — 'Text classification failed for post N' with no
indication of why (observed live: a Moonshot 400 on one post was
undiagnosable from fenliu-all.log). Append formatException output when
exc_info is set; plain records are unchanged.
forgejo-actions left a comment

WuMing

Found 3 issue(s). See inline comments below.

## WuMing Found **3** issue(s). See inline comments below.
@ -5,6 +5,8 @@ visibility: public
fenliu 3.0.0
- Fixed: AI vision now follows `media_proxy` redirects, so posts with proxy-served attachments get classified instead of remaining pending.
- Improved: AI replies that arrive empty or unparseable now leave the post pending with a clear log line and key response diagnostics instead of a JSON traceback.

docs [LOW]

The phrase "JSON traceback" is imprecise: the traceback is a Python stack trace ending in json.JSONDecodeError, not a JSON-formatted traceback. Consider wording it as "instead of a JSONDecodeError traceback".

**docs** [LOW] The phrase "JSON traceback" is imprecise: the traceback is a Python stack trace ending in `json.JSONDecodeError`, not a JSON-formatted traceback. Consider wording it as "instead of a `JSONDecodeError` traceback". <!-- wuming:sha256:46a5486c0de3c6088bfe66051fa15a80bdb4604949f91a7fa6b17feba667bf62 -->
@ -12,6 +14,10 @@ fenliu 3.0.0
Some Mastodon attachments are served through a redirecting `media_proxy` URL rather than a direct file link. The AI vision stage's image downloader treated those redirects as errors, so posts with proxy-served media never received a vision verdict and had to be reviewed by hand. It now follows the redirect to the real file and infers the image type from the final URL's file extension, so proxied PNG/WebP/GIF attachments are labelled correctly instead of always falling back to `image/jpeg`.
Occasionally the AI model would answer with HTTP 200 but an empty `message.content` field, and the classifier died with a cryptic `JSONDecodeError`. Those posts now simply stay pending for manual review, accompanied by a clear warning; the model's finish reason plus bounded excerpts of its content and reasoning are logged so the underlying cause is visible the next time it happens.

docs [LOW]

The original behavior left the post as AI: pending; the error was contained rather than killing the classifier process. People may read "classifier died" as a hard crash. Suggest "the classification request failed" or "classification raised a cryptic JSONDecodeError".

**docs** [LOW] The original behavior left the post as `AI: pending`; the error was contained rather than killing the classifier process. People may read "classifier died" as a hard crash. Suggest "the classification request failed" or "classification raised a cryptic `JSONDecodeError`". <!-- wuming:sha256:9f624f092f087aeb416a9c9c357696e5fb4118289bb8fde54d431a4d7eb6ce61 -->
@ -37,1 +37,3 @@
return f"{timestamp} - {context}:{record.lineno} - {record.levelname} - {record.getMessage()}"
formatted = f"{timestamp} - {context}:{record.lineno} - {record.levelname} - {record.getMessage()}"
if record.exc_info:
formatted = f"{formatted}\n{self.formatException(record.exc_info)}"

security [LOW]

A09: Appends raw exception tracebacks to log output whenever exc_info is set. Tracebacks can expose internal paths, library details, and exception messages that may contain sensitive context. Prefer sanitised/structured exception logging or redact sensitive data before writing tracebacks to production logs.

**security** [LOW] A09: Appends raw exception tracebacks to log output whenever exc_info is set. Tracebacks can expose internal paths, library details, and exception messages that may contain sensitive context. Prefer sanitised/structured exception logging or redact sensitive data before writing tracebacks to production logs. <!-- wuming:sha256:21a6cfece1ec32bc3973b3dbc72e6472f00d9fb85261d1f3861ddb43add48a5c -->
marvin8 marked this conversation as resolved
Author
Collaborator

WuMing wave 212 — responses

Release-Notes.md

The phrase 'JSON traceback' is imprecise…

Fixed: now reads 'instead of a JSONDecodeError traceback'.


Release-Notes.md

People may read 'classifier died' as a hard crash…

Fixed: reworded to 'the classification request failed with a cryptic JSONDecodeError'.


logging.py

A09: Appends raw exception tracebacks to log output… Prefer sanitised/structured exception logging…

🔴 Declining — this is the intended behaviour of the change (see the commit and release note): tracebacks in the operator's private console log are the standard diagnostic for unexpected failures, and stripping/redacting them would reintroduce exactly the 'classification failed with no why' blindness this commit fixes. No sensitive data is added by the traceback; it is the same exception the process already raised.

### WuMing wave 212 — responses [`Release-Notes.md`](https://forge.marvin8.zone/marvin8/dujiangyan/pulls/109#issuecomment-2306) > The phrase 'JSON traceback' is imprecise… ✅ Fixed: now reads 'instead of a `JSONDecodeError` traceback'. --- [`Release-Notes.md`](https://forge.marvin8.zone/marvin8/dujiangyan/pulls/109#issuecomment-2307) > People may read 'classifier died' as a hard crash… ✅ Fixed: reworded to 'the classification request failed with a cryptic `JSONDecodeError`'. --- [`logging.py`](https://forge.marvin8.zone/marvin8/dujiangyan/pulls/109#issuecomment-2308) > A09: Appends raw exception tracebacks to log output… Prefer sanitised/structured exception logging… 🔴 Declining — this is the intended behaviour of the change (see the commit and release note): tracebacks in the operator's private console log are the standard diagnostic for unexpected failures, and stripping/redacting them would reintroduce exactly the 'classification failed with no why' blindness this commit fixes. No sensitive data is *added* by the traceback; it is the same exception the process already raised.
📝 (fenliu) precise release-note wording per review
All checks were successful
/ gitleaks (pull_request) Successful in 14s
/ checks (pull_request) Successful in 2m43s
/ publish (pull_request) Has been skipped
/ publish-container (pull_request) Has been skipped
/ deploy-docs (pull_request) Has been skipped
/ pr-review (pull_request) Successful in 5m25s
118e1a6cf6
marvin8 approved these changes 2026-09-08 08:05:44 +00:00
marvin8 manually merged commit ad0e9ed8f6 into main 2026-09-08 08:13:57 +00:00
Sign in to join this conversation.
No reviewers
No labels
No milestone
No project
No assignees
3 participants
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
marvin8/dujiangyan!109
No description provided.