Skip to content

fix(iq_rating): unguarded OpenAI choices indexing and context_snippets coercion (#13980) - #14232

Closed
dwaddle wants to merge 2 commits into
BasedHardware:mainfrom
dwaddle:fix/iq-rating-openai-choices-safeguard
Closed

dwaddle wants to merge 2 commits into
BasedHardware:mainfrom
dwaddle:fix/iq-rating-openai-choices-safeguard

Conversation

@dwaddle

@dwaddle dwaddle commented Sep 16, 2026

Copy link
Copy Markdown
Contributor

Summary

Fixes issue #13980 by:

  1. Safely extracting choices from OpenAI responses in filter_names_with_openai() and calculate_iq_with_ai(), preventing IndexError / KeyError crashes when choices is empty or missing.
  2. Coercing context_snippets to a list before indexing or string joining.
  3. Adding unit tests in plugins/iq_rating/test_iq_rating.py verifying zero-crash behavior on malformed AI responses and string snippets.

Review in cubic

@Git-on-my-level

Copy link
Copy Markdown
Collaborator

Thanks @dwaddle — the production fix here is correct, and I verified it end to end. Notes per file:

plugins/iq_rating/main.py

  • Both choices[0] extraction sites are now guarded (filter_names_with_openai and calculate_iq_with_ai): non-dict payloads, empty choices, missing message, and content: null all degrade instead of raising. In filter_names_with_openai a malformed batch is skipped, matching the existing non-200 path; in calculate_iq_with_ai the continue sits in the outer batch loop, so affected people fall through to the existing random-score fill-in — exactly the fallback semantics [Bug] iq_rating: unguarded OpenAI response indexing crashes on an empty choices array #13966 asked for.
  • The context_snippets coercion (str → [str], non-list → []) is sound. Snippets are only ever appended as strings internally, so this is defensive hardening rather than a behavior change — appropriate here.
  • I also confirmed no unguarded "choices" indexing remains anywhere in the file.

plugins/iq_rating/test_iq_rating.py

  • All 3 tests pass when run with python -m pytest from the repo root (clean sandbox, requests.post mocked, no network/key needed; the only warning is the pre-existing FastAPI on_event deprecation at main.py:46).

Two gaps against the #13980 bounty scope — neither blocks the fix itself:

  1. The suite isn't registered in .github/checks-manifest.yaml, and as written it can't be: registered plugin checks run as python3 plugins/<dir>/test_*.py, and under direct script execution from plugins.iq_rating.main import ... raises ModuleNotFoundError (bare pytest hits the same wall, since plugins/ is a namespace package and the repo root isn't on sys.path). The other plugin suites (e.g. plugins/uber_call/test_main.py) handle this with a sys.path.insert(0, ...) bootstrap. Adding that — or switching to the stdlib-unittest style of the other hermetic suites — would let this register as iq-rating-openai-guard-tests per the bounty scope.
  2. The scope's "stringify items before joining" is only partially covered: a str snippet is coerced, but non-string items inside a list would still raise at the join. Unreachable from current callers, so just noting it for scope fidelity.

Flagging for maintainers: #13992 is also open against bounty #13980, so acceptance is a pick between the two implementations. Leaving that adjudication and merge sign-off with the maintainers.


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

@Git-on-my-level Git-on-my-level added positive-signal Automation verified a genuine fix/quality contribution python labels Sep 16, 2026
@dwaddle

dwaddle commented Sep 16, 2026

Copy link
Copy Markdown
Contributor Author

Thanks @Git-on-my-level! Appreciate the thorough review.

Pushed commit 770591803b incorporating both requested refinements:

  1. Added sys.path.insert(0, str(repo_root)) bootstrap to plugins/iq_rating/test_iq_rating.py so the test module runs hermetically via direct script execution (python3 plugins/iq_rating/test_iq_rating.py) as well as via pytest.
  2. Updated context_snippets joining to explicitly stringify each list item: context = " | ".join(str(s) for s in snippets[:10] if s). Added a dedicated test verifying non-string snippet list items.

All 4 unit tests pass 100% locally.

@kodjima33

Copy link
Copy Markdown
Collaborator

Thank you so much for this 🙏 — looks like this was already fixed on main (see commit f525990 / merged PR #14001, which included this exact iq_rating choices-indexing guard). Closing this one, but please keep them coming!

@kodjima33 kodjima33 closed this Sep 17, 2026
@github-actions

Copy link
Copy Markdown
Contributor

Hey @dwaddle 👋

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

positive-signal Automation verified a genuine fix/quality contribution python

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants