Fix/chatbot queue settings collision - #3911
Conversation
There was a problem hiding this comment.
Pull request overview
This PR attempts to fix a Django settings import-order collision for CHATBOT_QUEUE by making intel_owl/settings/chatbot.py the single source of truth and importing it from intel_owl/settings/celery.py, so operator configuration via secrets/env vars is not silently overwritten.
Changes:
- Make
intel_owl/settings/celery.pyimportCHATBOT_QUEUEfromintel_owl/settings/chatbot.pyinstead of redefining it. - Add/update extensive chatbot functionality and tests (agent runtime migration, streaming behavior, tool contract changes, verdict/evaluation plumbing).
- Update dependencies (notably a major LangChain version bump) and adjust related frontend/tests and analyzer DataModel behavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| .github/workflows/dependency_review.yml | Removes the GHSA allowlist override from dependency review. |
| api_app/analyzers_manager/migrations/0195_data_model_key_free_detectors.py | Adds mappings for key-free malicious detectors to emit DataModels with fixed reliability tiers. |
| api_app/analyzers_manager/migrations/0196_data_model_phishing_lists.py | Adds mappings for phishing-list analyzers to emit DataModels. |
| api_app/analyzers_manager/observable_analyzers/dns/dns_malicious_detectors/adguard.py | Mixes in DataModel gating so only real malicious hits create a DataModel. |
| api_app/analyzers_manager/observable_analyzers/dns/dns_malicious_detectors/cleanbrowsing_malicious_detector.py | Mixes in DataModel gating so only real malicious hits create a DataModel. |
| api_app/analyzers_manager/observable_analyzers/dns/dns_malicious_detectors/cloudflare_malicious_detector.py | Mixes in DataModel gating so only real malicious hits create a DataModel. |
| api_app/analyzers_manager/observable_analyzers/dns/dns_malicious_detectors/data_model.py | Introduces a shared mixin for gating DataModel creation on malicious hits. |
| api_app/analyzers_manager/observable_analyzers/dns/dns_malicious_detectors/dns4eu_malicious_detector.py | Mixes in DataModel gating so only real malicious hits create a DataModel. |
| api_app/analyzers_manager/observable_analyzers/dns/dns_malicious_detectors/googlesf.py | Mixes in DataModel gating so only real malicious hits create a DataModel. |
| api_app/analyzers_manager/observable_analyzers/dns/dns_malicious_detectors/google_webrisk.py | Mixes in DataModel gating so only real malicious hits create a DataModel. |
| api_app/analyzers_manager/observable_analyzers/dns/dns_malicious_detectors/mullvad_dns.py | Mixes in DataModel gating so only real malicious hits create a DataModel. |
| api_app/analyzers_manager/observable_analyzers/dns/dns_malicious_detectors/quad9_malicious_detector.py | Mixes in DataModel gating so only real malicious hits create a DataModel. |
| api_app/analyzers_manager/observable_analyzers/dns/dns_malicious_detectors/spamhaus_wqs.py | Mixes in DataModel gating so only real malicious hits create a DataModel. |
| api_app/analyzers_manager/observable_analyzers/dns/dns_malicious_detectors/ultradns_malicious_detector.py | Mixes in DataModel gating so only real malicious hits create a DataModel. |
| api_app/analyzers_manager/observable_analyzers/phishtank.py | Adds DataModel gating and sets malicious reliability based on verified status. |
| api_app/analyzers_manager/observable_analyzers/phishing_army.py | Adds DataModel gating so only listing hits create a DataModel. |
| api_app/analyzers_manager/observable_analyzers/phishstats.py | Adds DataModel gating so only listing hits create a DataModel. |
| api_app/analyzers_manager/observable_analyzers/tranco.py | Adds trusted DataModel emission based on Tranco rank bands. |
| api_app/analyzers_manager/repo_downloader.sh | Updates ExifTool download URL/behavior to SourceForge with pinned output filename. |
| api_app/chatbot_manager/agent/agent.py | Migrates agent construction to LangChain 1.x create_agent, adds keep-alive parsing and final answer extraction. |
| api_app/chatbot_manager/agent/system_prompt.txt | Updates tool routing rules and narration contract (jobs vs investigations, verdict narration, placeholders). |
| api_app/chatbot_manager/agent/streaming.py | Replaces callback-based streaming with a stream consumer consuming LangGraph stream modes. |
| api_app/chatbot_manager/agent/tools/init.py | Updates tool validation-error handling documentation for the new agent runtime. |
| api_app/chatbot_manager/agent/tools/_common.py | Updates explanation of tool-arg validation error recovery under create_agent. |
| api_app/chatbot_manager/agent/tools/analyze_observable.py | Adds default playbook resolution, actionable errors, and persists resolved playbook in pending payload; adds reason to plans. |
| api_app/chatbot_manager/agent/tools/list_investigations.py | Widens params to accept nulls (Optional[str]) and treats them as “no filter”. |
| api_app/chatbot_manager/agent/tools/search_jobs.py | Widens params to accept nulls (Optional[str]) and treats them as “no filter”. |
| api_app/chatbot_manager/agent/tools/summarize_job.py | Adds structured verdict to summarize response and echoes headline in prose for model reliability. |
| api_app/chatbot_manager/evaluation.py | Introduces deterministic “read-only” job verdict reader for chatbot responses. |
| api_app/chatbot_manager/serializers/analyze_observable.py | Adds reason field to analysis plan serializer. |
| api_app/chatbot_manager/serializers/job.py | Adds verdict serializers and attaches verdict object to summarize-job tool result schema. |
| api_app/chatbot_manager/tasks.py | Updates task to run and stream the new agent runtime; maps recursion-limit errors to ITERATION_LIMIT. |
| api_app/chatbot_manager/views.py | Updates REST chat endpoint to call the new agent runtime and handle recursion-limit errors. |
| api_app/data_model_manager/classify.py | Adds shared bucketing function for DataModel evaluation+reliability into UI/chat presentation buckets. |
| api_app/data_model_manager/enums.py | Adds DataModelVerdictBuckets shared between visualizer and chatbot. |
| api_app/visualizers_manager/visualizers/data_model.py | Refactors bucketing logic to use shared classify() and bucket enum. |
| docker/env_file_app_template | Documents and adds OLLAMA_KEEP_ALIVE env var template entry. |
| frontend/jest.config.js | Pins timezone to UTC for deterministic frontend date rendering tests. |
| frontend/src/components/chat/QuickActions.jsx | Removes “Evaluate results” chip and merges into a single “Summarize & evaluate” action. |
| frontend/tests/components/chat/QuickActions.test.jsx | Updates QuickActions tests to match chip label/behavior changes. |
| intel_owl/settings/celery.py | Fixes CHATBOT_QUEUE collision by importing from chatbot settings module. |
| intel_owl/settings/chatbot.py | Adds OLLAMA_KEEP_ALIVE setting alongside CHATBOT_QUEUE and other chatbot settings. |
| requirements/project-requirements.txt | Major LangChain dependency version upgrades. |
| tests/api_app/analyzers_manager/test_data_model_detectors.py | Adds tests for DataModel gating/mapping for malicious detectors. |
| tests/api_app/analyzers_manager/test_data_model_phishing_lists.py | Adds tests for phishing list gates and mapping behavior. |
| tests/api_app/analyzers_manager/test_data_model_phishtank.py | Adds tests for Phishtank gating and reliability mapping. |
| tests/api_app/analyzers_manager/test_data_model_targets.py | Adds anti-rot checks that expected analyzer configs/mappings exist. |
| tests/api_app/analyzers_manager/test_data_model_tranco.py | Adds tests for Tranco DataModel creation and rank-band reliability. |
| tests/api_app/chatbot_manager/test_agent.py | Updates agent tests for new runtime, recursion bound, keep-alive parsing, and prompt/tool routing. |
| tests/api_app/chatbot_manager/test_e2e.py | Updates e2e chatbot tests to fake the new agent stream contract. |
| tests/api_app/chatbot_manager/test_evaluation.py | Adds unit tests for the verdict reader behavior and edge cases. |
| tests/api_app/chatbot_manager/test_health.py | Adds a regression test intended to validate CHATBOT_QUEUE secret propagation. |
| tests/api_app/chatbot_manager/test_prompt.py | Updates prompt tests to validate prompt is passed into create_agent. |
| tests/api_app/chatbot_manager/test_query_counts.py | Extends query count tests for summarize_job verdict reader and new agent runtime. |
| tests/api_app/chatbot_manager/test_streaming.py | Replaces callback handler tests with stream consumer tests for token/status/action events. |
| tests/api_app/chatbot_manager/test_tasks.py | Updates Celery task tests for new stream-based agent runtime and recursion-limit handling. |
| tests/api_app/chatbot_manager/test_views.py | Updates REST view tests to mock build_agent and validate message handling with new contract. |
| tests/api_app/chatbot_manager/tools/test_analyze_observable.py | Adds/updates tool tests around default-playbook reasoning and pending payload persistence. |
| tests/api_app/chatbot_manager/tools/test_investigations.py | Adds coverage for null query/status handling in investigations tool. |
| tests/api_app/chatbot_manager/tools/test_search_jobs.py | Adds coverage for null query/status handling in search_jobs tool. |
| tests/api_app/chatbot_manager/tools/test_summarize_job.py | Adds tool-level tests ensuring summarize_job includes verdict and handles tenancy/no-evaluation cases. |
| tests/api_app/data_model_manager/test_classify.py | Adds tests for shared DataModel bucketing logic. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| def test_chatbot_queue_setting_configured_from_secrets(self): | ||
| with patch("intel_owl.secrets.get_secret", side_effect=lambda name, default=None: "custom_queue" if name == "CHATBOT_QUEUE" else default): | ||
| from intel_owl.settings import chatbot | ||
| self.assertEqual(chatbot.CHATBOT_QUEUE, "custom_queue") | ||
|
|
| CONFIG_QUEUE = "config" | ||
|
|
||
| CHATBOT_QUEUE = "chatbot" | ||
| from .chatbot import CHATBOT_QUEUE |
de4be8a to
efe68e7
Compare
efe68e7 to
87e54f2
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Suppressed comments (1)
tests/api_app/chatbot_manager/test_health.py:94
- This test patches
intel_owl.secrets.get_secretbut then importsintel_owl.settings.chatbot, which is already imported during Django settings initialization (intel_owl/settings/__init__.pydoesfrom .chatbot import *). Because the module is cached, the patchedget_secretis never consulted and the assertion will fail (or become order-dependent). Reload the settings modules under the patch, and also assert the value propagates intointel_owl.settings.celery/CELERY_QUEUESas described in the PR.
def test_chatbot_queue_setting_configured_from_secrets(self):
with patch(
"intel_owl.secrets.get_secret",
side_effect=lambda name, default=None: "custom_queue" if name == "CHATBOT_QUEUE" else default,
87e54f2 to
d2e97b7
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Suppressed comments (1)
tests/api_app/chatbot_manager/test_health.py:98
- This test likely won’t validate the intended behavior because
intel_owl.settings.chatbot(andintel_owl.settings.celery) are already imported during Django settings initialization; importingchatbotinside thepatch(...)block won’t re-evaluateCHATBOT_QUEUE, and it also doesn’t assert thatcelery.CHATBOT_QUEUE/CELERY_QUEUESpick up the configured value. Reload the modules under the patch and assert the value propagates intocelery(and intoCELERY_QUEUES).
from intel_owl.settings import chatbot
self.assertEqual(chatbot.CHATBOT_QUEUE, "custom_queue")
d2e97b7 to
595243a
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Suppressed comments (1)
tests/api_app/chatbot_manager/test_health.py:99
- This test patches
intel_owl.secrets.get_secret, butintel_owl.settings.chatbotis typically imported during Django settings initialization, so module-levelCHATBOT_QUEUEis likely already computed and cached. The assertion can therefore be ineffective/flaky unless the module is reloaded under the patched environment.
Also, the PR description says the regression test verifies propagation into intel_owl.settings.celery and CELERY_QUEUES, but the current test only checks chatbot.CHATBOT_QUEUE.
A more robust approach is to set CHATBOT_QUEUE via os.environ (which is what secrets.get_secret reads first), reload both intel_owl.settings.chatbot and intel_owl.settings.celery, and then assert that CELERY_QUEUES includes the custom queue.
def test_chatbot_queue_setting_configured_from_secrets(self):
with patch(
"intel_owl.secrets.get_secret",
side_effect=lambda name, default=None: "custom_queue" if name == "CHATBOT_QUEUE" else default,
):
|
Hi @Aditya30ag — @mlodic asked me to validate #3910, so here is the outcome of that. The diagnosis in your issue is correct and I reproduced it: Your fix direction works too: a single definition, whichever module it lives in, removes the import-order dependency. Three things I think are still missing before it closes the issue. 1. The regression test does not exercise the bugwith patch("intel_owl.secrets.get_secret", side_effect=...):
from intel_owl.settings import chatbot
self.assertEqual(chatbot.CHATBOT_QUEUE, "custom_queue")
So as written the assertion should fail rather than pass — and it went unnoticed because the CI never got to the test job (see the last section). It is also worth noting that this is why What does work is booting a fresh interpreter with the variable set and asserting on what comes out, which also lets you cover the part that matters most — that the value reaches the Celery route, not just the settings module: env = {**os.environ, "CHATBOT_QUEUE": "my_custom_queue", "DJANGO_SETTINGS_MODULE": "intel_owl.settings"}
subprocess.run([sys.executable, "-c", probe], env=env, ...)
# probe: django.setup(); report settings.CHATBOT_QUEUE, settings.CELERY_QUEUES,
# app.conf.task_routes[process_chat_message.name]["queue"], and the declared task_queues2. The consumer side is still hardcoded
if [ "$AWS_SQS" = "True" ]; then
queues="chatbot.fifo,config.fifo"
else
queues="chatbot,broadcast,config"
fiWith the settings fixed but this untouched, 3. The variable is undocumented
MinorWhere the single definition lives is a matter of taste, but there is an argument for About the CI failure on this PRNot your fault, and not related to your change: the DisclosureBefore I saw this PR I had already prepared #3919, which fixes the same issue and covers the three points above. I have moved it to draft — I would rather this one landed, and I am happy to hand the pieces over here. @mlodic, your call on which to carry forward; if you prefer #3911, the entrypoint and template changes can be lifted from #3919 (or I can push them here if @Aditya30ag wants). |
Description
CHATBOT_QUEUEwas defined in two settings modules: an operator-configurable one inintel_owl/settings/chatbot.py(reads from a secret/env var) and a hardcoded literal"chatbot"inintel_owl/settings/celery.py. Becausesettings/__init__.pywildcard-imports
.celeryafter.chatbot, the hardcoded literal silently overwrote the configured value, sointel_owl/celery.py'stask_routesandCELERY_QUEUESalways used"chatbot"regardless of what an operator set via theCHATBOT_QUEUEenv var.This PR makes
settings/chatbot.pythe single source of truth:settings/celery.pynow importsCHATBOT_QUEUEfrom it instead of redefining it, so the setting is actually configurable as the code implies. A regression test verifiesCHATBOT_QUEUEset via env var propagates through both
chatbot.pyandcelery.py(and intoCELERY_QUEUES).No behavior change with default configuration
CHATBOT_QUEUEstill defaults to"chatbot"everywhere.Issue #3910
Type of change
work as expected).
changes).
Checklist
this project
developRuff) gave 0 errors (pending CI run — not yet executed locally in this environment)testsfolder):tests/api_app/chatbot_manager/test_health.py::ChatbotQueueSettingTestCase::test_chatbot_queue_setting_configured_from_secretschanges)
DeepSource,Django Doctorsor otherthird-party linters have triggered any alerts during the CI checks, I have solved
those alerts (pending CI)
PR was drafted with the assistance of an LLM (Claude) — the root-cause analysis,
fix, and regression test were reviewed and verified by re-tracing the settings
import chain and reproducing the shadowing behavior in isolation before applying the
change.