[ghazy2001]

Failure-Class: none

Summary

Hardens backend/utils/retrieval/tools/chart_tools.py against runtime crashes during chat visualization generation and adds a comprehensive hermetic unit test suite:

1. Defensive Input Validation & Null Guards:

   • Guards labels and values against None and non-list types, preventing TypeError: object of type 'NoneType' has no len().
   • Requires at least one data point and enforces matching lengths between labels and values.

2. Case-Insensitive Chart Type Normalization:

   • Trims and lowercases chart_type (e.g. "Line", "BAR" normalize to "line", "bar"), avoiding false rejections from slight model casing variations.

3. Numeric Coercion & Finite Value Validation:

   • Coerces string numerals (e.g. ["7.2", "8.0"] or [2024, 2025]) into floats while safely stringifying labels.
   • Rejects non-numeric, NaN, and Inf values cleanly to prevent downstream client-side chart crashes.

4. Configurable Dictionary Resilience:

   • Ensures configurable is safely initialized even when config.get('configurable') returns None, eliminating TypeError: 'NoneType' object does not support item assignment.

5. Hermetic Unit Test Suite:

   • Added backend/tests/unit/test_chart_tools.py with 10 unit tests (10/10 passing) covering valid line/bar charts, color assignment, casing normalization, invalid chart types, null guards, length mismatches, numeric coercion, invalid/NaN/Inf values, and None configurable resilience.

Bounty Proposal ($50 USD)

• Proposed Tier: $50 USD
• Contractor / Settlement: Available for backend & tool engineering contracts (mahmoudghazy2001@gmail.com).

[Git-on-my-level]

The production change here is correct and nicely scoped — every guard kills a real crash path, and the tests are genuinely hermetic (10/10 pass in a clean run of this head). Two non-blocking notes below.

backend/utils/retrieval/tools/chart_tools.py

• The None/non-list guards on labels/values and moving the empty-list check ahead of the length-mismatch check correctly eliminate the len(None) / 'NoneType' object does not support item assignment crashes; NaN/Inf rejection via math.isnan/math.isinf keeps broken chart payloads from reaching the client.
• The configurable resilience block (isinstance(config, dict) + re-initializing when config.get('configurable') is None) is right; note the subtle behavior change that an empty config dict now receives chart_data where the old truthiness check skipped it — strictly an improvement.
• chart_type normalization (strip().lower()) and the safe_title fallback are sensible for model-generated arguments.
• Minor: except (ImportError, Exception) is redundant — Exception alone subsumes ImportError, and as written it reads like lint appeasement while silently swallowing any import-time error. A plain except Exception: with a one-line comment would say what you mean.
• Minor: loosening the signature to Optional[List[Any]] = None changes the tool schema the model sees — it can now omit labels/values entirely and get your friendly error instead of a framework validation error. Reasonable tradeoff, just flagging that the schema contract is looser than before.

backend/tests/unit/test_chart_tools.py

• Good coverage of each new guard (casing, None labels/values, empty lists, mismatched lengths, string-number coercion, NaN/Inf, None configurable), with ContextVar set/reset in try/finally. The importlib fallback loader is unusual but only fires when the package import fails; in CI the normal import path is used.

Security: no new dependencies or workflow changes; the f-string error messages echo caller-supplied values but are returned as plain text, so no injection surface. Blast radius is a single chat-visualization tool — if wrong, worst case is a returned error string.

USER IMPACT: Users should see fewer broken or missing charts in chat when the model sends slightly malformed chart arguments (wrong casing, string numbers, empty titles) — invalid input now returns a clean error the model can retry on instead of crashing.

Nothing blocking from my side. Optional nit for a follow-up: booleans currently pass validation silently (float(True) == 1.0) — fine if intentional, maybe worth a test pinning it either way. Formal merge sign-off left to a human maintainer per repo policy for first-time contributors.

---

by AI on behalf of David — if you need David’s attention urgently, please @Git-on-my-level and escalate with need human response.

[aryanorastar]

Closing as a later copy of #23094. That pull request already adds the list and configurable guards in chart_tools.py.

[github-actions]

Hey @ghazy2001 👋

Thank you so much for taking the time to contribute to Omi! We truly appreciate you putting in the effort to submit this pull request.

After careful review, we've decided not to merge this particular PR. Please don't take this personally — we genuinely try to merge as many contributions as possible, but sometimes we have to make tough calls based on:

• Project standards — Ensuring consistency across the codebase
• User needs — Making sure changes align with what our users need
• Code best practices — Maintaining code quality and maintainability
• Project direction — Keeping aligned with our product principles and locked invariants

Before your next PR, please skim:

• PRODUCT.md — product north star
• Product invariants — locked rules (shared chat, memory tiers, agent control plane, integrations, brand)

If this was declined for direction or taste, maintainers should cite an invariant ID or open a proposed one — ask if that citation is missing.

Your contribution is still valuable to us, and we'd love to see you contribute again in the future! If you'd like feedback on how to improve this PR or want to discuss alternative approaches, please don't hesitate to reach out.

Thank you for being part of the Omi community!