Skip to content

fix: handle unhandled AttributeError on string messages#47

Merged
Vishisht16 merged 4 commits into
Vishisht16:mainfrom
riddhima25bet10005-a11y:fix-issue-44
Jun 12, 2026
Merged

fix: handle unhandled AttributeError on string messages#47
Vishisht16 merged 4 commits into
Vishisht16:mainfrom
riddhima25bet10005-a11y:fix-issue-44

Conversation

@riddhima25bet10005-a11y

Copy link
Copy Markdown
Contributor

Fixes #44.

This PR adds type validation in _extract_last_user_message to ensure that messages is a list and each message is a dictionary before attempting to extract the role and content. This prevents an unhandled AttributeError and the resulting 500 Internal Server Error when a client sends a malformed payload (e.g., {"messages": "not a list"}).

@CLAassistant

CLAassistant commented Jun 5, 2026

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@coderabbitai

coderabbitai Bot commented Jun 5, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Summary by CodeRabbit

  • Bug Fixes

    • Improved request validation to defensively check payload structure, ensuring proper error responses when message data is malformed, missing, or contains invalid types.
  • Tests

    • Added comprehensive test coverage for request validation error handling, covering non-list messages, non-dictionary items, and improperly formatted content fields.

Walkthrough

Defensive checks in _extract_last_user_message ensure messages is a list, each message is a dict, and content is a string; tests added to assert /chat returns 400 with {"status":"error"} for malformed messages payloads.

Changes

Malformed Payload Defense

Layer / File(s) Summary
Validate messages payload and extract last user message
humane_proxy/middleware/interceptor.py
Early return when messages is not a list; verify each message is a dict before .get() and only return content when it's a string.
Negative tests for malformed messages
tests/test_interceptor.py
Adds tests asserting /chat returns 400 and {"status":"error"} when messages is not a list, contains a non-dict item, or a message content is not a string; reasserts empty-request-body error text.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Suggested labels

bug, level:intermediate, type:testing, type:bug

Suggested reviewers

  • Vishisht16

Poem

I nibble on bytes beneath the moon,
Guarding the proxy from malformed tune.
Lists and dicts now pass my sight,
Strings in content keep errors light.
Hop, hop — safe requests tonight. 🐰✨

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately describes the main fix: handling an AttributeError that occurs when string messages are sent instead of lists, which is the core issue addressed in the PR.
Description check ✅ Passed The description clearly explains the problem (unhandled AttributeError), the cause (malformed payload), and the solution (type validation), all of which align with the changeset.
Linked Issues check ✅ Passed The PR fully addresses issue #44 by validating that messages is a list and each message is a dictionary, preventing AttributeError and the resulting 500 errors from malformed payloads.
Out of Scope Changes check ✅ Passed All changes are directly related to fixing issue #44: defensive validation in _extract_last_user_message and corresponding test coverage for malformed message payloads.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@humane_proxy/middleware/interceptor.py`:
- Around line 72-73: The extractor currently returns msg.get("content", "")
which may be non-str and can bubble into pipeline.classify; modify the block in
interceptor.py so that after confirming isinstance(msg, dict) and
msg.get("role") == "user" you explicitly fetch content = msg.get("content", "")
and then return it only if isinstance(content, str) else return "" (i.e.,
enforce/validate string type rather than returning arbitrary types) to close
malformed-payload crash paths.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 5dc7c463-0f77-41ff-b272-6bb08764337b

📥 Commits

Reviewing files that changed from the base of the PR and between b34979d and 516e20d.

📒 Files selected for processing (1)
  • humane_proxy/middleware/interceptor.py

Comment thread humane_proxy/middleware/interceptor.py Outdated
@riddhima25bet10005-a11y

Copy link
Copy Markdown
Contributor Author

Hi @Vishisht16
Kindly review this PR.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (3)
tests/test_interceptor.py (3)

110-116: ⚡ Quick win

Consider asserting the specific error message for robustness.

The test correctly verifies the 400 response and error status. However, for consistency with the enhanced assertion on line 108 and to ensure the correct code path is triggered, consider also asserting the specific error message:

assert resp.json()["message"] == "No user message found in payload."

This makes the test more robust and explicit about the expected behavior.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/test_interceptor.py` around lines 110 - 116, In
test_messages_is_not_a_list, after asserting resp.status_code and
resp.json()["status"], add an assertion that resp.json()["message"] == "No user
message found in payload." to ensure the specific error message is returned when
the "messages" field is not a list; update the test function name
test_messages_is_not_a_list and use the resp JSON payload to compare the
"message" key for exact matching.

118-124: ⚡ Quick win

Consider asserting the specific error message for robustness.

Similar to the previous test, consider adding an assertion for the specific error message to ensure the correct code path:

assert resp.json()["message"] == "No user message found in payload."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/test_interceptor.py` around lines 118 - 124, In
test_message_item_is_not_a_dict, strengthen the assertion by checking the
specific error message returned in resp: after asserting resp.status_code and
resp.json()["status"], add an assertion on resp.json()["message"] to equal "No
user message found in payload." so the test verifies the exact error path for
the malformed messages payload.

126-132: ⚡ Quick win

Consider asserting the specific error message for robustness.

For consistency with the pattern established on line 108, add an assertion for the specific error message:

assert resp.json()["message"] == "No user message found in payload."

This ensures the test verifies the complete expected behavior.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/test_interceptor.py` around lines 126 - 132, Update the
test_message_content_is_not_a_string test to assert the specific error message
returned by the endpoint: after asserting resp.status_code == 400 and
resp.json()["status"] == "error", add an assertion that resp.json()["message"]
== "No user message found in payload." so the test mirrors the pattern used in
the other tests (e.g., the assertion on line ~108) and verifies the full error
response.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@tests/test_interceptor.py`:
- Around line 110-116: In test_messages_is_not_a_list, after asserting
resp.status_code and resp.json()["status"], add an assertion that
resp.json()["message"] == "No user message found in payload." to ensure the
specific error message is returned when the "messages" field is not a list;
update the test function name test_messages_is_not_a_list and use the resp JSON
payload to compare the "message" key for exact matching.
- Around line 118-124: In test_message_item_is_not_a_dict, strengthen the
assertion by checking the specific error message returned in resp: after
asserting resp.status_code and resp.json()["status"], add an assertion on
resp.json()["message"] to equal "No user message found in payload." so the test
verifies the exact error path for the malformed messages payload.
- Around line 126-132: Update the test_message_content_is_not_a_string test to
assert the specific error message returned by the endpoint: after asserting
resp.status_code == 400 and resp.json()["status"] == "error", add an assertion
that resp.json()["message"] == "No user message found in payload." so the test
mirrors the pattern used in the other tests (e.g., the assertion on line ~108)
and verifies the full error response.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 1be5d3e7-c13f-4f34-86a7-0e4d523cbab3

📥 Commits

Reviewing files that changed from the base of the PR and between 516e20d and c3c4bfc.

📒 Files selected for processing (2)
  • humane_proxy/middleware/interceptor.py
  • tests/test_interceptor.py
🚧 Files skipped from review as they are similar to previous changes (1)
  • humane_proxy/middleware/interceptor.py

@Vishisht16 Vishisht16 left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good job

@Vishisht16 Vishisht16 added gssoc:approved Approved PR under GSSoC'26 quality:exceptional Bonus points under GSSoC for Exceptional PR level:intermediate Decent knowledge required to work on type:bug Smashes annoying bugs labels Jun 12, 2026
@Vishisht16 Vishisht16 merged commit 207b86f into Vishisht16:main Jun 12, 2026
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

gssoc:approved Approved PR under GSSoC'26 level:intermediate Decent knowledge required to work on quality:exceptional Bonus points under GSSoC for Exceptional PR type:bug Smashes annoying bugs

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Unhandled AttributeError on String messages (DoS / Crash)

3 participants