NestBot AI Assistant Contexts #1891
Conversation
Summary by CodeRabbit
WalkthroughAdd a new Context model and migrate Chunk to reference Context; update retrieval to use context.entity; refactor chunk/context creation into shared Base commands and extractors; update admin, Makefile, utilities, RAG tooling, and tests; adjust constants and error handling. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Assessment against linked issues
Out-of-scope changes
Possibly related PRs
Suggested labels
Suggested reviewers
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (7)
backend/apps/ai/models/context.py (1)
23-33: Improve the string representation method.The current implementation could raise
AttributeErrorand has redundant logic.Consider this more defensive approach:
def __str__(self): """Human readable representation.""" - entity = ( - getattr(self.content_object, "name", None) - or getattr(self.content_object, "key", None) - or str(self.content_object) - ) + if self.content_object: + entity = ( + getattr(self.content_object, "name", None) + or getattr(self.content_object, "key", None) + or str(self.content_object) + ) + else: + entity = "No linked object" + return ( f"{self.content_type.model if self.content_type else 'None'} {entity}: " f"{self.generated_text[:50]}" )backend/apps/ai/management/commands/ai_run_rag_tool.py (2)
34-39: Improve the help text for content-types argument.The current help text "Content types to filter by (e.g., project chapter)" could be more descriptive.
Consider improving the help text:
- help="Content types to filter by (e.g., project chapter)", + help="Content types to filter by (e.g., 'project', 'chapter', 'committee')",
64-70: Add input validation and improve output formatting.Consider adding validation for argument values and better output formatting.
Add validation before processing:
+ # Validate arguments + if not (0.0 <= options["threshold"] <= 1.0): + self.stderr.write(self.style.ERROR("Threshold must be between 0.0 and 1.0")) + return + if options["limit"] <= 0: + self.stderr.write(self.style.ERROR("Limit must be positive")) + return + self.stdout.write("\nProcessing query...") result = rag_tool.query( question=options["query"], limit=options["limit"], similarity_threshold=options["threshold"], content_types=options["content_types"], ) - self.stdout.write(f"\nAnswer: {result}") + self.stdout.write(self.style.SUCCESS(f"\nAnswer: {result}"))backend/tests/apps/ai/models/context_test.py (1)
10-15: Consider extracting shared test utility.The
create_model_mockfunction is duplicated fromchunk_test.py. Consider extracting this to a shared test utility module to avoid code duplication.You could create a shared test utilities module:
# backend/tests/apps/ai/test_utils.py from unittest.mock import Mock def create_model_mock(model_class): mock = Mock(spec=model_class) mock._state = Mock() mock.pk = 1 mock.id = 1 return mockThen import it in both test files.
backend/apps/ai/agent/tools/rag/generator.py (2)
16-32: Fix grammatical issues in SYSTEM_PROMPT.The system prompt contains several grammatical inconsistencies that should be corrected for professionalism:
-3. you will answer questions only related to OWASP and within the scope of OWASP. +3. You will answer questions only related to OWASP and within the scope of OWASP. 4. Be concise and directly answer the user's query. 5. Provide the necessary link if the context contains a URL. -6. If there is any query based on location, you need to look for latitude and longitude in the -context and provide the nearest OWASP chapter based on that. +6. If there is any query based on location, you need to look for latitude and longitude in the context and provide the nearest OWASP chapter based on that. 7. You can ask for more information if the query is very personalized or user-centric. -8. after trying all of the above, If the context does not contain the information or you think that -it is out of scope for OWASP, you MUST state: "please ask question related to OWASP." +8. After trying all of the above, if the context does not contain the information or you think that it is out of scope for OWASP, you MUST state: "please ask question related to OWASP."
115-119: Add query context to exception logging.When logging OpenAI API errors, include relevant context to aid debugging.
except openai.OpenAIError: - logger.exception("OpenAI API error") + logger.exception("OpenAI API error for query: %s", query[:100]) answer = "I'm sorry, I'm currently unable to process your request."backend/apps/ai/agent/tools/rag/retriever.py (1)
75-183: Refactor get_additional_context for better maintainability.This method is over 100 lines with repetitive patterns. Consider extracting content-type-specific handlers.
def get_additional_context(self, content_object, content_type: str) -> dict[str, Any]: """Get additional context information based on content type.""" clean_content_type = content_type.split(".")[-1] if "." in content_type else content_type handlers = { "chapter": self._get_chapter_context, "project": self._get_project_context, "event": self._get_event_context, "committee": self._get_committee_context, "message": self._get_message_context, } handler = handlers.get(clean_content_type) if handler: context = handler(content_object) return {k: v for k, v in context.items() if v is not None} return {} def _get_chapter_context(self, content_object) -> dict[str, Any]: """Extract context for chapter objects.""" return { "location": getattr(content_object, "suggested_location", None), "region": getattr(content_object, "region", None), # ... rest of the attributes }This approach would make the code more modular and easier to extend with new content types.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
backend/apps/ai/Makefile(1 hunks)backend/apps/ai/admin.py(2 hunks)backend/apps/ai/agent/tools/rag/generator.py(1 hunks)backend/apps/ai/agent/tools/rag/rag_tool.py(1 hunks)backend/apps/ai/agent/tools/rag/retriever.py(1 hunks)backend/apps/ai/common/constants.py(1 hunks)backend/apps/ai/common/utils.py(3 hunks)backend/apps/ai/management/commands/ai_run_rag_tool.py(1 hunks)backend/apps/ai/migrations/0005_context_alter_chunk_unique_together_chunk_context_and_more.py(1 hunks)backend/apps/ai/models/chunk.py(4 hunks)backend/apps/ai/models/context.py(1 hunks)backend/tests/apps/ai/models/chunk_test.py(5 hunks)backend/tests/apps/ai/models/context_test.py(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (8)
backend/apps/ai/management/commands/ai_run_rag_tool.py (1)
backend/apps/ai/agent/tools/rag/rag_tool.py (2)
RagTool(13-72)query(38-72)
backend/apps/ai/admin.py (1)
backend/apps/ai/models/context.py (1)
Context(10-33)
backend/tests/apps/ai/models/chunk_test.py (2)
backend/apps/ai/models/context.py (1)
Context(10-33)backend/apps/ai/models/chunk.py (2)
Chunk(12-74)update_data(47-74)
backend/apps/ai/common/utils.py (1)
backend/apps/ai/models/context.py (1)
Context(10-33)
backend/apps/ai/agent/tools/rag/retriever.py (3)
backend/apps/ai/models/chunk.py (1)
Chunk(12-74)backend/apps/ai/agent/tools/rag/rag_tool.py (1)
query(38-72)backend/apps/slack/models/message.py (1)
text(83-85)
backend/tests/apps/ai/models/context_test.py (3)
backend/apps/ai/models/context.py (1)
Context(10-33)backend/tests/apps/ai/models/chunk_test.py (1)
create_model_mock(7-12)backend/apps/common/models.py (1)
TimestampedModel(37-46)
backend/apps/ai/models/context.py (2)
backend/apps/common/models.py (1)
TimestampedModel(37-46)backend/apps/ai/models/chunk.py (1)
Meta(15-18)
backend/apps/ai/models/chunk.py (2)
backend/apps/ai/models/context.py (1)
Context(10-33)backend/apps/common/utils.py (1)
truncate(164-176)
🔇 Additional comments (27)
backend/apps/ai/Makefile (1)
21-23: LGTM!The new Makefile target follows the established pattern and provides a convenient way to invoke the RAG tool management command.
backend/apps/ai/common/constants.py (1)
4-5: LGTM!The new constants have sensible default values and follow the established naming conventions. The retrieval limit of 5 and similarity threshold of 0.5 are reasonable defaults for RAG functionality.
backend/apps/ai/common/utils.py (2)
12-12: LGTM!The import of the Context model is correctly added to support the new data model structure.
63-63: LGTM!The chunk creation correctly uses the new context-based association instead of the previous direct content_object relationship.
backend/apps/ai/models/context.py (1)
19-21: LGTM!The Meta configuration is appropriate with a clear database table name and verbose name.
backend/apps/ai/management/commands/ai_run_rag_tool.py (1)
15-51: LGTM! Well-structured arguments with good defaults.The argument definitions are comprehensive and use appropriate defaults from the constants module. The help text is clear and informative.
backend/apps/ai/admin.py (4)
6-6: LGTM!Proper import of the new
Contextmodel.
9-20: Well-configured admin interface for Context model.The
ContextAdminprovides comprehensive management capabilities with appropriate fields for display, search, and filtering. The configuration aligns well with the Context model's structure.
29-32: Admin updates correctly reflect the model refactoring.The changes properly adapt the
ChunkAdminto use the newcontextrelationship instead of the previous generic foreign key approach. Thelist_filteroncontext__content_typemaintains the ability to filter by content type through the related Context model.
35-35: LGTM!Proper registration of the
Contextmodel with its admin class.backend/tests/apps/ai/models/chunk_test.py (5)
4-4: LGTM!Proper import of the new
Contextmodel for testing.
17-26: Test properly updated for new Context relationship.The
test_str_methodcorrectly mocks thecontextattribute instead of the previous generic foreign key fields, maintaining test coverage for the string representation functionality.
57-58: Update data tests correctly adapted to new Context model.The test properly uses
Contextmocks and validates thecontextparameter in theupdate_datamethod calls, reflecting the model refactoring accurately.Also applies to: 67-67, 69-69
129-129: Unique constraint updated correctly.The unique constraint test properly reflects the change from
("content_type", "object_id", "text")to("context", "text"), aligning with the model schema changes.
131-137: Good test coverage for the new context relationship.The test validates the foreign key relationship properties including the related model, null/blank settings, ensuring the relationship is properly configured.
backend/apps/ai/migrations/0005_context_alter_chunk_unique_together_chunk_context_and_more.py (1)
14-42: Context model creation looks good.The
Contextmodel is properly defined with all necessary fields including the generic foreign key components (content_type,object_id) and additional metadata fields.backend/apps/ai/agent/tools/rag/rag_tool.py (2)
16-36: Good initialization with proper error handling.The constructor properly initializes the retriever and generator components with appropriate error handling and logging. The exception handling ensures any initialization failures are properly logged and re-raised.
58-64: Well-structured retrieval logic.The retrieval step properly passes all the necessary parameters and includes appropriate logging for debugging purposes.
backend/tests/apps/ai/models/context_test.py (4)
19-67: Excellent test coverage for string representation.The tests comprehensively cover various scenarios for the
__str__method including edge cases like empty text, exact 50-character limit, and text truncation. This ensures the string representation works correctly in all situations.
69-100: Thorough field property validation.The tests properly validate all field properties including types, constraints, defaults, and relationships. This ensures the model is correctly configured.
115-151: Good coverage of model manager operations.The tests properly validate the standard Django model manager operations with appropriate mocking, ensuring the Context model integrates correctly with Django's ORM.
153-176: Proper validation testing.The tests cover both successful validation and validation error scenarios, ensuring the model's validation logic works correctly.
backend/apps/ai/agent/tools/rag/generator.py (1)
35-52: LGTM!The initialization method properly validates the API key and provides clear error messaging.
backend/apps/ai/models/chunk.py (2)
26-29: LGTM!The
__str__method properly handles nullable context and provides clear representation.
46-74: AllChunk.update_dataCallers UpdatedNo remaining calls use the old
content_objectparameter. Verified in:
backend/apps/ai/common/utils.pybackend/tests/apps/ai/models/chunk_test.pybackend/apps/ai/admin.pybackend/apps/ai/agent/tools/rag/retriever.py- All
ai_create_*_chunksmanagement commandsNo further changes are needed.
backend/apps/ai/agent/tools/rag/retriever.py (2)
44-66: LGTM!Excellent error handling with appropriate logging and re-raising of exceptions.
255-278: LGTM!Clean implementation with good handling of singular/plural forms.
backend/apps/ai/migrations/0005_context_alter_chunk_unique_together_chunk_context_and_more.py
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
backend/apps/ai/admin.py(2 hunks)backend/apps/ai/common/utils.py(3 hunks)backend/apps/ai/migrations/0006_rename_generated_text_context_content_and_more.py(1 hunks)backend/apps/ai/models/chunk.py(4 hunks)backend/apps/ai/models/context.py(1 hunks)backend/tests/apps/ai/models/chunk_test.py(5 hunks)backend/tests/apps/ai/models/context_test.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- backend/apps/ai/common/utils.py
- backend/apps/ai/models/context.py
- backend/apps/ai/admin.py
- backend/tests/apps/ai/models/context_test.py
- backend/tests/apps/ai/models/chunk_test.py
- backend/apps/ai/models/chunk.py
🔇 Additional comments (1)
backend/apps/ai/migrations/0006_rename_generated_text_context_content_and_more.py (1)
13-17: LGTM! Field rename looks correct.The renaming of
generated_texttocontentin the Context model improves naming consistency and aligns with the broader refactor objectives.
backend/apps/ai/migrations/0006_rename_generated_text_context_content_and_more.py
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/apps/ai/migrations/0007_alter_chunk_context_alter_context_unique_together.py(1 hunks)backend/apps/ai/models/chunk.py(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/apps/ai/models/chunk.py
🔇 Additional comments (2)
backend/apps/ai/migrations/0007_alter_chunk_context_alter_context_unique_together.py (2)
24-27: LGTM! Proper unique constraint for generic foreign key pattern.The unique constraint on
(content_type, object_id)is appropriate for preventing duplicate contexts for the same OWASP entity. This follows Django best practices for generic foreign key relationships.
1-11: LGTM! Well-structured Django migration.The migration follows Django conventions with proper dependencies, imports, and class structure. The dependency on the previous migration maintains the correct sequence for the Context model refactoring.
backend/apps/ai/migrations/0007_alter_chunk_context_alter_context_unique_together.py
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (6)
backend/apps/ai/models/context.py (3)
30-34: Prefer named UniqueConstraint over unique_together (clearer, non-deprecated style).UniqueConstraint provides a named, explicit constraint that’s easier to manage in migrations.
Apply:
class Meta: db_table = "ai_contexts" verbose_name = "Context" - unique_together = ("entity_type", "entity_id") + constraints = [ + models.UniqueConstraint( + fields=("entity_type", "entity_id"), + name="uniq_ai_context_entity", + ) + ]
35-43: Make str robust when the related entity is missing.If the generic target has been deleted, self.entity may be None, resulting in “model None: …”. Prefer a clearer placeholder.
- entity = ( - getattr(self.entity, "name", None) - or getattr(self.entity, "key", None) - or str(self.entity) - ) + entity_obj = self.entity + entity = ( + getattr(entity_obj, "name", None) + or getattr(entity_obj, "key", None) + or (str(entity_obj) if entity_obj is not None else "-") + )
14-19: Consider removing the wrapper regenerate_chunks_for_context to reduce coupling.The indirection exists only to sidestep import cycles, but it reintroduces a model-to-utils dependency surface. Since update_data no longer calls it, prefer importing utils where needed or keeping the wrapper in a non-model module (e.g., apps.ai.common.init).
If retained, add a type hint for clarity:
-def regenerate_chunks_for_context(context): +def regenerate_chunks_for_context(context: "Context"): """Import regenerate_chunks_for_context to avoid circular import."""backend/tests/apps/ai/models/context_test.py (3)
32-38: Rename to reflect field name (entity_type).The test name still uses the old “content_type” wording.
- def test_content_type_field_properties(self): + def test_entity_type_field_properties(self):
39-42: Rename to reflect field name and, if adopting BigInteger, update the asserted type.If you switch entity_id to PositiveBigIntegerField, the test should assert that. Otherwise, just rename the test.
- def test_object_id_field_properties(self): + def test_entity_id_field_properties(self): field = Context._meta.get_field("entity_id") - assert field.__class__.__name__ == "PositiveIntegerField" + assert field.__class__.__name__ == "PositiveBigIntegerField"If you keep PositiveIntegerField, only apply the rename and keep the original assertion.
377-377: Add a test to ensure unchanged contexts don’t trigger save (preserve timestamps).This guards against accidental regressions that would bump nest_updated_at unnecessarily.
+ @patch("apps.ai.models.context.Context.objects.get_or_create") + def test_update_data_noop_does_not_save_when_unchanged(self, mock_get_or_create): + content_object = Mock() + content_object.pk = 1 + with patch("apps.ai.models.context.ContentType.objects.get_for_model") as mock_gfm: + ct = Mock(spec=ContentType) + ct.get_source_expressions = Mock(return_value=[]) + mock_gfm.return_value = ct + existing = create_model_mock(Context) + existing.content = "same" + existing.source = "src" + existing.save = Mock() + mock_get_or_create.return_value = (existing, False) + + result = Context.update_data("same", content_object, source="src", save=True) + + assert result == existing + existing.save.assert_not_called()
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
backend/apps/ai/models/context.py(1 hunks)backend/tests/apps/ai/models/context_test.py(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-08-16T12:07:38.116Z
Learnt from: Dishant1804
PR: OWASP/Nest#1891
File: backend/apps/ai/models/context.py:44-66
Timestamp: 2025-08-16T12:07:38.116Z
Learning: In the Context.update_data method, the Context object should always be returned (never None), even when no changes are made to the content or source fields. This ensures consistent behavior for callers.
Applied to files:
backend/apps/ai/models/context.py
📚 Learning: 2025-07-28T14:51:14.736Z
Learnt from: adithya-naik
PR: OWASP/Nest#1894
File: frontend/src/components/TopContributorsList.tsx:74-74
Timestamp: 2025-07-28T14:51:14.736Z
Learning: In the OWASP/Nest project, the maintainer adithya-naik prefers not to create separate components for code that's only used in two specific cases, following the YAGNI principle to avoid over-engineering when the duplication is limited and manageable.
Applied to files:
backend/apps/ai/models/context.py
🧬 Code Graph Analysis (2)
backend/apps/ai/models/context.py (4)
backend/apps/common/models.py (1)
TimestampedModel(37-46)backend/apps/ai/common/utils.py (1)
regenerate_chunks_for_context(73-101)backend/apps/owasp/admin/entity_member.py (1)
entity(61-74)backend/apps/ai/models/chunk.py (2)
Meta(15-18)update_data(45-76)
backend/tests/apps/ai/models/context_test.py (2)
backend/apps/ai/models/context.py (2)
Context(21-67)update_data(45-67)backend/apps/common/models.py (1)
TimestampedModel(37-46)
backend/apps/ai/models/context.py
Outdated
| def regenerate_chunks_for_context(context): | ||
| """Import regenerate_chunks_for_context to avoid circular import.""" | ||
| from apps.ai.common.utils import regenerate_chunks_for_context as _regenerate_chunks | ||
|
|
||
| return _regenerate_chunks(context) |
There was a problem hiding this comment.
This is here for what purpose?
There was a problem hiding this comment.
I was trying to resolve the circular import I cleaned out the code but this missed out, removed it
| from apps.ai.common.constants import DELIMITER | ||
|
|
||
|
|
||
| def extract_chapter_content(chapter) -> tuple[str, str]: |
There was a problem hiding this comment.
I have 2 suggestions for the extractors:
- why do you need separation for prose/metadata if you join them later anyway?
- this needs to be migrated to Jinja templates like ones we have for Slack responses
Please create an issue to track this refactoring.
backend/apps/ai/common/utils.py
Outdated
| """ | ||
| from apps.ai.models.chunk import Chunk | ||
|
|
||
| old_chunk_count = context.chunks.count() |
There was a problem hiding this comment.
Is it possible to delete them w/o pre-checks?
There was a problem hiding this comment.
yes -- removed the checks
| source_name = self.get_source_name(chunk.context.entity) | ||
| additional_context = self.get_additional_context( | ||
| chunk.content_object, chunk.content_type.model | ||
| chunk.context.entity, chunk.context.entity_type.model |
There was a problem hiding this comment.
It seems you don't need the second param as you can figure that our from the model instance inside of get_additional_context
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/apps/ai/agent/tools/rag/retriever.py (1)
59-67: Update exception handling for OpenAI SDK v1.xThe v1.x Python client no longer defines
OpenAIError. Instead you should catch its new base exception (openai.APIError) and/or the more specific subclasses (e.g.,RateLimitError,APIConnectionError,APITimeoutError) to handle errors appropriately.• Affected file:
backend/apps/ai/agent/tools/rag/retriever.py(lines 59–67)
• Replace the genericexcept openai.OpenAIError:with granular catches and a finalAPIErrorfallback.Proposed diff:
try: response = self.openai_client.embeddings.create( input=[query], model=self.embedding_model, ) return response.data[0].embedding - except openai.OpenAIError: - logger.exception("OpenAI API error") - raise + except openai.RateLimitError: + logger.warning("Rate limit exceeded, backing off before retry") + # backoff & retry logic + except (openai.APIConnectionError, openai.APITimeoutError) as e: + logger.warning("Network error from OpenAI API: %s", e) + # retry logic + except openai.APIError as e: + logger.exception("Unexpected OpenAI API error") + raise
♻️ Duplicate comments (1)
backend/apps/ai/agent/tools/rag/retriever.py (1)
217-222: Good fix: filter through Context entity_type (matches the model changes)Filtering by context__entity_type__… is correct post-refactor away from Chunk.content_type. This addresses the earlier review note.
🧹 Nitpick comments (12)
backend/tests/apps/ai/management/commands/ai_update_committee_context_test.py (4)
69-79: Patch the correct base method: target BaseAICommand.get_base_queryset, not BaseContextCommand.
get_base_querysetis defined on BaseAICommand per the shared snippets. Patching BaseContextCommand here works by overshadowing via attribute injection, but it doesn’t validate the real inheritance path and can hide regressions.Apply this diff:
- with patch( - "apps.ai.common.base.context_command.BaseContextCommand.get_base_queryset" - ) as mock_super: + with patch( + "apps.ai.common.base.ai_command.BaseAICommand.get_base_queryset" + ) as mock_super:
152-173: Mirror production semantics: have update_data return a truthy object, not True.
Context.update_datareturns a Context instance (truthy), not a boolean. Returning a mock object here reduces the risk of a future mismatch if the implementation changes.- with patch("apps.ai.common.base.context_command.Context") as mock_context_class: - mock_context_class.update_data.return_value = True + with patch("apps.ai.common.base.context_command.Context") as mock_context_class: + mock_context_class.update_data.return_value = Mock()
188-212: Failure path may be unreachable with current update_data contract.
Context.update_datareturns a Context object and raises on DB errors; it doesn’t return False. The tested “failure” branch will not occur in production unless the contract changes or you explicitly return falsy values fromupdate_data.Consider one of:
- Adjust BaseContextCommand.process_context_batch to catch exceptions from
update_dataand log an error (then add a test usingside_effect=Exception("db error")), or- Document that
update_datareturns a truthy object on success and update this test to validate exception handling rather than a False return.
1-1: Rename staleai_create_committee_contextreferences in test filePlease update all remaining occurrences of
ai_create_committee_contexttoai_update_committee_contextand rename the test class accordingly:• File:
backend/tests/apps/ai/management/commands/ai_update_committee_context_test.py
– Line 1:
diff -"""Tests for the ai_create_committee_context command.""" +"""Tests for the ai_update_committee_context command."""
– Around line 27–29:
diff -class TestAiCreateCommitteeContextCommand: - """Test suite for the ai_create_committee_context command.""" +class TestAiUpdateCommitteeContextCommand: + """Test suite for the ai_update_committee_context command."""This will ensure clarity and make future greps accurate.
backend/apps/ai/agent/tools/rag/retriever.py (8)
24-30: Make SUPPORTED_ENTITY_TYPES immutable for intent and tiny membership winsTuples are fine, but a frozenset communicates immutability and gives O(1) membership checks (you iterate and also check membership later). This is a tiny nit, not a blocker.
- SUPPORTED_ENTITY_TYPES = ( + SUPPORTED_ENTITY_TYPES = frozenset(( "chapter", "committee", "event", "message", "project", - ) + ))
72-78: Prefer persisted source_name when available; tighten docstring and add a slug fallbackIf Context stores a source/source_name (as suggested by the AI command snippets), prefer using that value when available for consistency across pipelines. Also, the docstring still says “content object”; here it’s an entity. Adding “slug” is a cheap, useful fallback.
- def get_source_name(self, entity) -> str: - """Get the name/identifier for the content object.""" - for attr in ("name", "title", "login", "key", "summary"): + def get_source_name(self, entity) -> str: + """Get a human-readable identifier for the entity.""" + for attr in ("name", "title", "login", "key", "summary", "slug"): if getattr(entity, attr, None): return str(getattr(entity, attr)) return str(entity)
79-85: Docstring drift: “content type” → “entity type”; clarify paramsThe function operates on entities now. Tightening the docstring avoids future confusion.
- def get_additional_context(self, entity, entity_type: str) -> dict[str, Any]: - """Get additional context information based on content type. + def get_additional_context(self, entity, entity_type: str) -> dict[str, Any]: + """Get additional context information based on entity type. @@ - entity: The source object. - entity_type: The model name of the content object. + entity: The related entity (from Context.entity). + entity_type: The entity model name (e.g., "project", "chapter").
91-91: Minor naming clarity: clean_content_type → clean_entity_typeVariable name can be misleading after the refactor to entities. Not a blocker; consider renaming across this method when convenient.
224-224: Avoid hauling the embedding payload; defer the “embedding” fieldAfter the annotate is done, the embedding value isn’t used in Python. Deferring it trims network I/O and memory, especially with large vector dims.
- chunks = queryset.select_related("context__entity_type").order_by("-similarity")[:limit] + chunks = ( + queryset + .select_related("context__entity_type") + .defer("embedding") + .order_by("-similarity")[:limit] + )
228-235: Avoid double entity fetches, correct log message, and prefer persisted context source_nameAccessing GenericForeignKey triggers a DB hit; you access it for the guard and then again for source/extra context. Cache it once, fix the log message, and prefer a stored source_name when available to avoid recomputing.
- for chunk in chunks: - if not chunk.context or not chunk.context.entity: - logger.warning("Content object is None for chunk %s. Skipping.", chunk.id) - continue - - source_name = self.get_source_name(chunk.context.entity) - additional_context = self.get_additional_context( - chunk.context.entity, chunk.context.entity_type.model - ) + for chunk in chunks: + if not chunk.context: + logger.warning("Context is None for chunk %s. Skipping.", chunk.id) + continue + + entity = getattr(chunk.context, "entity", None) + if not entity: + logger.warning("Context entity is None for chunk %s. Skipping.", chunk.id) + continue + + source_name = getattr(chunk.context, "source_name", None) or self.get_source_name(entity) + additional_context = self.get_additional_context(entity, chunk.context.entity_type.model)If Context does not have source/source_name, ignore that piece of the change and keep the fallback.
263-265: Optional: enrich keyword detection with synonymsThe simple singular/plural check works; consider a small synonyms map (e.g., “repo”, “repos” → “project”; “chapter(s)” → “chapter”) to improve recall.
232-235: Use Context metadata where possible to reduce N+1sLonger-term, consider persisting and reading “additional_context” and “source_name” from Context at creation time. That would avoid hitting the GFK per chunk at retrieval time, especially when limit is high.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
backend/apps/ai/agent/tools/rag/retriever.py(4 hunks)backend/apps/ai/common/base/context_command.py(1 hunks)backend/apps/ai/models/context.py(1 hunks)backend/tests/apps/ai/agent/tools/rag/retriever_test.py(1 hunks)backend/tests/apps/ai/common/base/context_command_test.py(1 hunks)backend/tests/apps/ai/management/commands/ai_update_committee_context_test.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- backend/apps/ai/common/base/context_command.py
- backend/tests/apps/ai/agent/tools/rag/retriever_test.py
- backend/tests/apps/ai/common/base/context_command_test.py
- backend/apps/ai/models/context.py
🧰 Additional context used
🧬 Code Graph Analysis (2)
backend/tests/apps/ai/management/commands/ai_update_committee_context_test.py (4)
backend/apps/ai/management/commands/ai_update_committee_context.py (2)
Command(8-14)extract_content(12-14)backend/apps/ai/common/base/context_command.py (4)
BaseContextCommand(9-54)help(12-14)process_context_batch(16-43)handle(45-54)backend/apps/ai/common/base/ai_command.py (6)
get_base_queryset(29-31)get_default_queryset(33-35)add_arguments(56-58)get_queryset(60-69)get_entity_key(71-73)source_name(25-27)backend/apps/ai/models/context.py (1)
update_data(45-67)
backend/apps/ai/agent/tools/rag/retriever.py (3)
backend/apps/owasp/admin/entity_member.py (1)
entity(61-74)backend/apps/ai/common/base/ai_command.py (1)
source_name(25-27)backend/apps/ai/management/commands/ai_update_slack_message_context.py (1)
source_name(41-42)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: CodeQL (javascript-typescript)
🔇 Additional comments (2)
backend/tests/apps/ai/management/commands/ai_update_committee_context_test.py (2)
79-90: LGTM on default queryset filtering.Good verification that the default path applies
is_active=True.
213-231: LGTM on handle() batch plumbing.Solid assertion of option forwarding and
process_batch_funcbinding.
backend/tests/apps/ai/management/commands/ai_update_committee_context_test.py
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (9)
backend/apps/ai/models/context.py (2)
49-60: Close race on create and avoid bumping timestamps when unchangedTwo concurrent callers can both miss the get() and try to create, causing IntegrityError on save. Also, saving when content/source are unchanged will bump nest_updated_at, breaking “stale-chunk” detection based on timestamps.
Apply atomic get_or_create and only save when fields actually change; always return Context per your established contract:
@staticmethod def update_data( content: str, entity, source: str = "", *, save: bool = True, - ) -> "Context": + ) -> "Context": """Create or update context for a given entity.""" - entity_type = ContentType.objects.get_for_model(entity) - entity_id = entity.pk - - try: - context = Context.objects.get(entity_type=entity_type, entity_id=entity_id) - except Context.DoesNotExist: - context = Context(entity_type=entity_type, entity_id=entity_id) - - context.content = content - context.source = source - - if save: - context.save() - - return context + entity_type = ContentType.objects.get_for_model(entity) + entity_id = entity.pk + + if save: + # Atomic to avoid duplicate rows under concurrency + context, created = Context.objects.get_or_create( + entity_type=entity_type, + entity_id=entity_id, + defaults={"content": content, "source": source}, + ) + if created: + return context + # No-op if unchanged; preserve timestamps + if context.content == content and context.source == source: + return context + # Update only changed fields + context.content = content + context.source = source + context.save(update_fields=["content", "source"]) + return context + else: + # Return unsaved instance for bulk-save flows + try: + context = Context.objects.get(entity_type=entity_type, entity_id=entity_id) + except Context.DoesNotExist: + return Context( + entity_type=entity_type, entity_id=entity_id, content=content, source=source + ) + context.content = content + context.source = source + return contextThis keeps the “always return Context” behavior while removing the race and spurious timestamp updates.
19-21: Upgradeentity_idto PositiveBigIntegerField to match 64-bit primary keysWe’ve confirmed that your project’s
DEFAULT_AUTO_FIELDis set toBigAutoFieldinbackend/settings/base.py:194. SinceGenericForeignKeymay point to any model using the default auto field, you should update theentity_idfield toPositiveBigIntegerFieldto avoid potential overflows.• File:
backend/apps/ai/models/context.py, Lines 19–21- entity_id = models.PositiveIntegerField() + entity_id = models.PositiveBigIntegerField()• After changing the field type, generate and apply a Django migration to alter the column in the database.
backend/tests/apps/ai/agent/tools/rag/retriever_test.py (2)
19-20: Patch where the symbol is used: target apps.ai.agent.tools.rag.retriever.openai.OpenAIPatching the top-level openai.OpenAI is brittle and can leak across modules. Patch the symbol in the module under test.
- patch("openai.OpenAI") as mock_openai, + patch("apps.ai.agent.tools.rag.retriever.openai.OpenAI") as mock_openai,Apply the same change to all occurrences listed in the line ranges.
Also applies to: 45-46, 54-55, 74-75, 89-90, 104-105, 118-120, 135-136, 151-153, 171-172, 451-454, 483-485, 534-536, 567-569
70-84: Make exception type resilient across OpenAI SDK versionsopenai.OpenAIError may not exist in some SDK versions. Use the exception from the retriever module’s namespace and assert accordingly.
- with ( - patch.dict(os.environ, {"DJANGO_OPEN_AI_SECRET_KEY": "test-key"}), - patch("openai.OpenAI") as mock_openai, - ): + from apps.ai.agent.tools.rag import retriever as retriever_mod + with ( + patch.dict(os.environ, {"DJANGO_OPEN_AI_SECRET_KEY": "test-key"}), + patch("apps.ai.agent.tools.rag.retriever.openai.OpenAI") as mock_openai, + ): mock_client = MagicMock() - mock_client.embeddings.create.side_effect = openai.OpenAIError("API Error") + Exc = getattr(retriever_mod.openai, "OpenAIError", Exception) + mock_client.embeddings.create.side_effect = Exc("API Error") mock_openai.return_value = mock_client retriever = Retriever() - with pytest.raises(openai.OpenAIError): + with pytest.raises(Exc): retriever.get_query_embedding("test query")Repeat similarly for any other tests that rely on OpenAIError.
backend/apps/ai/common/utils.py (3)
39-41: Docstring claims ValueError is raised, but code does not raise itEither enforce a ValueError when context is invalid, or align the docstring with current behavior (log-and-return-empty on error). Given current tests, remove the incorrect Raises section.
- Raises: - ValueError: If context is None or invalid -
43-44: Remove duplicate import of Chunk inside the functionChunk is already imported at module scope. The inner import is leftover and unnecessary.
- from apps.ai.models.chunk import Chunk
91-99: Log success only when chunks were actually (re)createdCurrently logs success unconditionally. Use the return value to decide.
- create_chunks_and_embeddings( + new_chunks = create_chunks_and_embeddings( chunk_texts=new_chunk_texts, context=context, openai_client=openai_client, save=True, ) - logger.info("Successfully completed chunk regeneration for new context") + if not new_chunks: + logger.error( + "Chunk regeneration failed or produced no chunks for context id=%s", + getattr(context, "id", None), + ) + return + logger.info( + "Successfully completed chunk regeneration for context id=%s (%d chunks)", + getattr(context, "id", None), + len(new_chunks), + )backend/tests/apps/ai/common/utils_test.py (1)
117-121: Rename to reflect actual assertion (sleep is NOT called here)The name/docstring say “sleep called” but the test asserts not_called.
- def test_create_chunks_and_embeddings_sleep_called( + def test_create_chunks_and_embeddings_no_sleep_on_first_call( self, mock_datetime, mock_sleep, mock_update_data ): - """Tests that sleep is called when needed.""" + """Tests that sleep is not called on the first request."""backend/apps/ai/agent/tools/rag/retriever.py (1)
151-158: Normalize leaders to a JSON-serializable shape (avoid M2M manager)For committees, leaders currently reads the relation (likely a manager/QuerySet), which won’t serialize cleanly and is inconsistent with leaders_raw used elsewhere.
- "leaders": getattr(entity, "leaders", []), + "leaders": getattr(entity, "leaders_raw", []),If you must use the relation, convert explicitly, e.g., [u.login for u in entity.leaders.all()].
🧹 Nitpick comments (9)
backend/apps/ai/models/context.py (2)
23-27: Prefer UniqueConstraint over unique_together (modern Django API)unique_together is legacy. Use UniqueConstraint for clarity and flexibility (e.g., conditionals, names).
class Meta: db_table = "ai_contexts" verbose_name = "Context" - unique_together = ("entity_type", "entity_id") + constraints = [ + models.UniqueConstraint( + fields=("entity_type", "entity_id"), + name="uniq_ai_context_entity", + ), + ]
28-35: Minor: friendlier fallback in str when entity is missingIf the entity was deleted, str(None) renders “None”, which can be confusing in admin logs. Consider a clearer placeholder.
- entity = ( + entity = ( getattr(self.entity, "name", None) or getattr(self.entity, "key", None) - or str(self.entity) + or ("<deleted>" if self.entity is None else str(self.entity)) )backend/apps/ai/common/utils.py (3)
45-53: Rate limiting logic has no state; time_since_last_request is constantYou re-derive “last request time” from now, so time_since_last_request is always DEFAULT_LAST_REQUEST_OFFSET_SECONDS, not the real elapsed time. Consider tracking last_request_at across calls.
Example approach:
- Add a module-global last_request_at: datetime | None = None
- Before calling the API, compare now - last_request_at to MIN_REQUEST_INTERVAL_SECONDS
- Update last_request_at = now after the request
I can draft a patch if you want to persist this state here or behind a small helper.
66-68: Catch ValueError from zip(strict=True) length mismatchesIf the embeddings response length differs, zip(..., strict=True) raises ValueError. Include it in the handled exceptions.
- except (OpenAIError, AttributeError, TypeError): + except (OpenAIError, AttributeError, TypeError, ValueError): logger.exception("Failed to create chunks and embeddings") return []Optionally, validate len(response.data) == len(chunk_texts) and log a clearer error before zipping.
60-65: Optional: deduplicate chunk_texts before embedding to reduce costSending duplicate texts increases API usage and can hit uniqueness on (context, text). You can preserve order with a simple de-dup.
# Before calling embeddings.create(...) seen = set() unique_texts = [] for t in chunk_texts: if t not in seen: unique_texts.append(t) seen.add(t) chunk_texts = unique_textsbackend/tests/apps/ai/common/utils_test.py (2)
82-92: Use the exception class from the module under testTo avoid SDK-version drift, set side_effect to the OpenAIError imported by utils, not openai.OpenAIError.
- mock_openai_client.embeddings.create.side_effect = openai.OpenAIError( - "API connection failed" - ) + from apps.ai.common import utils as utils_mod + mock_openai_client.embeddings.create.side_effect = utils_mod.OpenAIError( + "API connection failed" + )
263-281: Stabilize success-path assertion by returning a non-empty listWith the proposed utils change to log success only when chunks are created, ensure the mocked helper returns a truthy, non-empty list.
mock_openai_client = MagicMock() mock_openai_class.return_value = mock_openai_client + mock_create_chunks.return_value = [MagicMock()] regenerate_chunks_for_context(context) mock_existing_chunks.all.assert_called_once() mock_existing_chunks.all().delete.assert_called_once()Repeat similarly in test_regenerate_chunks_for_context_no_existing_chunks if you want to assert the success log path.
backend/apps/ai/agent/tools/rag/retriever.py (2)
72-77: Nit: docstring still says “content object”; it’s actually an entityMinor naming mismatch; update for clarity.
- def get_source_name(self, entity) -> str: - """Get the name/identifier for the content object.""" + def get_source_name(self, entity) -> str: + """Get the name/identifier for the entity."""
223-224: Performance nit: also select_related("context")You already select_related("context__entity_type"). Adding "context" avoids an extra fetch for the FK itself.
- chunks = queryset.select_related("context__entity_type").order_by("-similarity")[:limit] + chunks = queryset.select_related("context", "context__entity_type").order_by("-similarity")[:limit]
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
backend/apps/ai/agent/tools/rag/retriever.py(4 hunks)backend/apps/ai/common/utils.py(2 hunks)backend/apps/ai/models/context.py(1 hunks)backend/tests/apps/ai/agent/tools/rag/retriever_test.py(1 hunks)backend/tests/apps/ai/common/utils_test.py(4 hunks)backend/tests/apps/ai/models/context_test.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/tests/apps/ai/models/context_test.py
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-08-16T12:07:38.124Z
Learnt from: Dishant1804
PR: OWASP/Nest#1891
File: backend/apps/ai/models/context.py:44-66
Timestamp: 2025-08-16T12:07:38.124Z
Learning: In the Context.update_data method, the Context object should always be returned (never None), even when no changes are made to the content or source fields. This ensures consistent behavior for callers.
Applied to files:
backend/apps/ai/models/context.py
📚 Learning: 2025-07-28T14:51:14.736Z
Learnt from: adithya-naik
PR: OWASP/Nest#1894
File: frontend/src/components/TopContributorsList.tsx:74-74
Timestamp: 2025-07-28T14:51:14.736Z
Learning: In the OWASP/Nest project, the maintainer adithya-naik prefers not to create separate components for code that's only used in two specific cases, following the YAGNI principle to avoid over-engineering when the duplication is limited and manageable.
Applied to files:
backend/apps/ai/models/context.py
🧬 Code graph analysis (5)
backend/apps/ai/common/utils.py (2)
backend/apps/ai/models/chunk.py (3)
Chunk(12-76)update_data(45-76)split_text(35-42)backend/apps/ai/models/context.py (2)
Context(14-60)update_data(38-60)
backend/apps/ai/models/context.py (3)
backend/apps/common/models.py (1)
TimestampedModel(37-46)backend/apps/owasp/admin/entity_member.py (1)
entity(61-74)backend/apps/ai/models/chunk.py (2)
Meta(15-18)update_data(45-76)
backend/tests/apps/ai/agent/tools/rag/retriever_test.py (1)
backend/apps/ai/agent/tools/rag/retriever.py (6)
Retriever(21-268)get_query_embedding(49-70)get_source_name(72-77)get_additional_context(79-182)extract_content_types_from_query(247-268)retrieve(184-245)
backend/tests/apps/ai/common/utils_test.py (1)
backend/apps/ai/common/utils.py (2)
create_chunks_and_embeddings(19-70)regenerate_chunks_for_context(73-98)
backend/apps/ai/agent/tools/rag/retriever.py (1)
backend/apps/owasp/admin/entity_member.py (1)
entity(61-74)
🔇 Additional comments (2)
backend/apps/ai/models/context.py (1)
17-21: Confirm “optional GFK” requirement vs current required fieldsThe PR/issue description mentions an optional GenericForeignKey on Context. Here both entity_type and entity_id are required. If contexts without an entity are needed (e.g., global/system contexts), make these fields nullable and validate the pair (both set or both null).
If optional linkage is intended:
- entity_type = models.ForeignKey(ContentType, on_delete=models.CASCADE) - entity_id = models.PositiveIntegerField() + entity_type = models.ForeignKey(ContentType, on_delete=models.CASCADE, null=True, blank=True) + entity_id = models.PositiveBigIntegerField(null=True, blank=True)Add a clean() or DB constraint to ensure both-null or both-set for integrity. If the “required” design is intentional, please update the docs/issue to reflect that.
backend/tests/apps/ai/common/utils_test.py (1)
96-113: Docstring says “failure path when context is None” but behavior is log-and-emptyThis matches current behavior. If you adopt an explicit ValueError in utils (alternative path), update this test accordingly. Otherwise no action needed.
| response = openai_instance.complete() | ||
|
|
||
| assert response is None | ||
| mock_logger.exception.assert_called_once_with( |
There was a problem hiding this comment.
I acidentally removed it while I was testing other assertions -- I have added it back
| result = str(context) | ||
| assert result == ( | ||
| "test_model Test Object: This is test content that is longer than 50 charac" | ||
| # cspell:ignore charac |
There was a problem hiding this comment.
This is not something we normally do. Use truncate for Context::str instead.
There was a problem hiding this comment.
made the changes
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
backend/apps/ai/models/context.py (1)
38-61: Make update_data concurrency-safe and avoid unnecessary timestamp bumps.The current get/except-create pattern can race under concurrent calls and always saves even when nothing changed, unnecessarily bumping nest_updated_at (hurting “chunks stale if context updated” logic). Use get_or_create for atomicity and only persist when content/source actually change. This keeps the “always return Context” behavior (per the retrieved learning).
@@ @staticmethod def update_data( - content: str, - entity, + content: str, + entity: models.Model, source: str = "", *, save: bool = True, ) -> "Context": - """Create or update context for a given entity.""" - entity_type = ContentType.objects.get_for_model(entity) - entity_id = entity.pk - - try: - context = Context.objects.get(entity_type=entity_type, entity_id=entity_id) - except Context.DoesNotExist: - context = Context(entity_type=entity_type, entity_id=entity_id) - - context.content = content - context.source = source - - if save: - context.save() - - return context + """Create or update context for a given entity. + Always returns a Context instance (never None).""" + entity_type = ContentType.objects.get_for_model(entity) + entity_id = entity.pk + + if save: + context, created = Context.objects.get_or_create( + entity_type=entity_type, + entity_id=entity_id, + defaults={"content": content, "source": source}, + ) + if not created and (context.content != content or context.source != source): + context.content = content + context.source = source + # Include nest_updated_at to ensure the timestamp is persisted. + context.save(update_fields=["content", "source", "nest_updated_at"]) + return context + else: + # Support bulk-save flows by returning an unsaved instance when missing. + try: + context = Context.objects.get(entity_type=entity_type, entity_id=entity_id) + except Context.DoesNotExist: + return Context( + entity_type=entity_type, + entity_id=entity_id, + content=content, + source=source, + ) + # Update in-memory only (caller decides when to save). + context.content = content + context.source = source + return contextbackend/tests/apps/ai/models/context_test.py (1)
297-336: Update “new context with save=True” test to patch get_or_create.This mirrors the creation path after the refactor.
- @patch("apps.ai.models.context.Context.objects.get") - @patch("apps.ai.models.context.Context.__init__") - def test_update_data_new_context_with_save(self, mock_init, mock_get): + @patch("apps.ai.models.context.Context.objects.get_or_create") + def test_update_data_new_context_with_save(self, mock_get_or_create): """Test update_data creating a new context with save=True.""" from apps.ai.models.context import Context as ContextModel - mock_get.side_effect = ContextModel.DoesNotExist - mock_init.return_value = None # __init__ should return None + new_ctx = create_model_mock(ContextModel) + mock_get_or_create.return_value = (new_ctx, True) @@ - mock_get_for_model.assert_called_once_with(mock_content_object) - mock_get.assert_called_once_with( - entity_type=mock_content_type, - entity_id=1, - ) - mock_init.assert_called_once_with( - entity_type=mock_content_type, - entity_id=1, - ) - assert result.content == content - assert result.source == source - mock_save.assert_called_once() + mock_get_for_model.assert_called_once_with(mock_content_object) + mock_get_or_create.assert_called_once_with( + entity_type=mock_content_type, + entity_id=1, + defaults={"content": content, "source": source}, + ) + assert result == new_ctx
🧹 Nitpick comments (8)
backend/apps/ai/models/context.py (4)
29-36: Harden str when the related entity is missing.If the target object is deleted, self.entity may be None and str(self.entity) yields "None". Fall back to entity_id for a clearer label.
- entity = ( - getattr(self.entity, "name", None) - or getattr(self.entity, "key", None) - or str(self.entity) - ) - return f"{self.entity_type.model} {entity}: {truncate(self.content, 50)}" + obj = self.entity + label = ( + getattr(obj, "name", None) + or getattr(obj, "key", None) + or (str(obj) if obj is not None else str(self.entity_id)) + ) + return f"{self.entity_type.model} {label}: {truncate(self.content, 50)}"
24-28: Optional: prefer UniqueConstraint over unique_together (named, modern style).Not urgent, but UniqueConstraint provides naming and advanced options.
class Meta: db_table = "ai_contexts" verbose_name = "Context" - unique_together = ("entity_type", "entity_id") + constraints = [ + models.UniqueConstraint( + fields=("entity_type", "entity_id"), + name="uq_ai_context_entity", + ) + ]
3-3: Remove unused logger.logging and logger are unused in this module.
-import logging @@ -logger = logging.getLogger(__name__)Also applies to: 12-12
20-21: Use PositiveBigIntegerField for entity_id to match BigAutoField default PKConfirmed that DEFAULT_AUTO_FIELD is set to
django.db.models.BigAutoFieldin backend/settings/base.py:194, and most apps (and their migrations) useBigAutoFieldfor primary keys. To avoid potential overflow when referencing large PKs via the GenericForeignKey, switchentity_idto a 64-bit field.• File to update:
backend/apps/ai/models/context.py- entity_id = models.PositiveIntegerField() + entity_id = models.PositiveBigIntegerField()• After applying, run
./manage.py makemigrations aito generate anAlterFieldmigration.
• Review any tests or fixtures that set or compareentity_idand update them if they assume a 32-bit range.backend/tests/apps/ai/models/context_test.py (4)
66-78: Make manager.create test reflect required fields.Context requires entity_type and entity_id; the current test calls create without them (only works because of patching). Include those args for realism.
- @patch("apps.ai.models.context.Context.objects.create") + @patch("apps.ai.models.context.Context.objects.create") def test_context_manager_create(self, mock_create): mock_context = create_model_mock(Context) mock_create.return_value = mock_context content = "Test text" source = "test_source" - result = Context.objects.create(content=content, source=source) + mock_ct = Mock(spec=ContentType) + result = Context.objects.create( + content=content, + source=source, + entity_type=mock_ct, + entity_id=1, + ) - mock_create.assert_called_once_with(content=content, source=source) + mock_create.assert_called_once_with( + content=content, + source=source, + entity_type=mock_ct, + entity_id=1, + ) assert result == mock_context
233-264: Adjust str expectation when entity is missing.With the hardened str, it falls back to entity_id instead of “Unknown”.
@@ - result = str(context) - assert result == "test_model Unknown: Another test content" + result = str(context) + assert result == "test_model 456: Another test content"
40-43: Update field type assertion if switching to PositiveBigIntegerField.If you adopt the 64-bit entity_id change, update this assertion accordingly.
- assert field.__class__.__name__ == "PositiveIntegerField" + assert field.__class__.__name__ == "PositiveBigIntegerField"
158-168: Add a no-op update test to prevent accidental timestamp bumps.Recommend adding a test ensuring update_data(save=True) does not call save() when values are unchanged (protects stale-chunk detection).
def test_update_data_noop_does_not_save(monkeypatch): from apps.ai.models.context import Context as ContextModel mock_entity = Mock() mock_entity.pk = 1 mock_ct = Mock(spec=ContentType) mock_ct.get_source_expressions = Mock(return_value=[]) # Existing instance with given values ctx = create_model_mock(ContextModel) ctx.content = "same" ctx.source = "src" with patch("apps.ai.models.context.ContentType.objects.get_for_model", return_value=mock_ct), \ patch("apps.ai.models.context.Context.objects.get_or_create", return_value=(ctx, False)) as goc, \ patch.object(ContextModel, "save") as mock_save: result = Context.update_data("same", mock_entity, source="src", save=True) goc.assert_called_once() mock_save.assert_not_called() assert result is ctx
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
backend/apps/ai/models/context.py(1 hunks)backend/tests/apps/ai/models/context_test.py(1 hunks)backend/tests/apps/common/open_ai_test.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/tests/apps/common/open_ai_test.py
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-08-16T12:07:38.124Z
Learnt from: Dishant1804
PR: OWASP/Nest#1891
File: backend/apps/ai/models/context.py:44-66
Timestamp: 2025-08-16T12:07:38.124Z
Learning: In the Context.update_data method, the Context object should always be returned (never None), even when no changes are made to the content or source fields. This ensures consistent behavior for callers.
Applied to files:
backend/apps/ai/models/context.pybackend/tests/apps/ai/models/context_test.py
📚 Learning: 2025-07-28T14:51:14.736Z
Learnt from: adithya-naik
PR: OWASP/Nest#1894
File: frontend/src/components/TopContributorsList.tsx:74-74
Timestamp: 2025-07-28T14:51:14.736Z
Learning: In the OWASP/Nest project, the maintainer adithya-naik prefers not to create separate components for code that's only used in two specific cases, following the YAGNI principle to avoid over-engineering when the duplication is limited and manageable.
Applied to files:
backend/apps/ai/models/context.py
🧬 Code graph analysis (2)
backend/apps/ai/models/context.py (3)
backend/apps/common/models.py (1)
TimestampedModel(37-46)backend/apps/common/utils.py (1)
truncate(164-176)backend/apps/ai/models/chunk.py (2)
Meta(15-18)update_data(45-76)
backend/tests/apps/ai/models/context_test.py (3)
backend/apps/ai/models/context.py (2)
Context(15-61)update_data(39-61)backend/apps/common/models.py (1)
TimestampedModel(37-46)backend/apps/common/utils.py (1)
truncate(164-176)
| @patch("apps.ai.models.context.Context.objects.get") | ||
| def test_update_data_existing_context(self, mock_get): | ||
| mock_context = create_model_mock(Context) | ||
| mock_get.return_value = mock_context | ||
|
|
||
| content = "Test" | ||
| mock_content_object = Mock() | ||
| mock_content_object.pk = 1 | ||
|
|
||
| with patch( | ||
| "apps.ai.models.context.ContentType.objects.get_for_model" | ||
| ) as mock_get_for_model: | ||
| mock_content_type = Mock() | ||
| mock_content_type.get_source_expressions = Mock(return_value=[]) | ||
| mock_get_for_model.return_value = mock_content_type | ||
|
|
||
| result = Context.update_data(content, mock_content_object, source="src", save=True) | ||
|
|
||
| mock_get_for_model.assert_called_once_with(mock_content_object) | ||
| mock_get.assert_called_once_with( | ||
| entity_type=mock_content_type, | ||
| entity_id=1, | ||
| ) | ||
| assert result == mock_context | ||
| assert mock_context.content == content | ||
| assert mock_context.source == "src" | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Align existing-context test with get_or_create path.
If update_data switches to get_or_create, this test should patch that and simulate created=False.
- @patch("apps.ai.models.context.Context.objects.get")
- def test_update_data_existing_context(self, mock_get):
+ @patch("apps.ai.models.context.Context.objects.get_or_create")
+ def test_update_data_existing_context(self, mock_get_or_create):
mock_context = create_model_mock(Context)
- mock_get.return_value = mock_context
+ mock_get_or_create.return_value = (mock_context, False)
@@
- mock_get_for_model.assert_called_once_with(mock_content_object)
- mock_get.assert_called_once_with(
- entity_type=mock_content_type,
- entity_id=1,
- )
+ mock_get_for_model.assert_called_once_with(mock_content_object)
+ mock_get_or_create.assert_called_once_with(
+ entity_type=mock_content_type,
+ entity_id=1,
+ defaults={"content": content, "source": "src"},
+ )
assert result == mock_context
assert mock_context.content == content
assert mock_context.source == "src"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @patch("apps.ai.models.context.Context.objects.get") | |
| def test_update_data_existing_context(self, mock_get): | |
| mock_context = create_model_mock(Context) | |
| mock_get.return_value = mock_context | |
| content = "Test" | |
| mock_content_object = Mock() | |
| mock_content_object.pk = 1 | |
| with patch( | |
| "apps.ai.models.context.ContentType.objects.get_for_model" | |
| ) as mock_get_for_model: | |
| mock_content_type = Mock() | |
| mock_content_type.get_source_expressions = Mock(return_value=[]) | |
| mock_get_for_model.return_value = mock_content_type | |
| result = Context.update_data(content, mock_content_object, source="src", save=True) | |
| mock_get_for_model.assert_called_once_with(mock_content_object) | |
| mock_get.assert_called_once_with( | |
| entity_type=mock_content_type, | |
| entity_id=1, | |
| ) | |
| assert result == mock_context | |
| assert mock_context.content == content | |
| assert mock_context.source == "src" | |
| @patch("apps.ai.models.context.Context.objects.get_or_create") | |
| def test_update_data_existing_context(self, mock_get_or_create): | |
| mock_context = create_model_mock(Context) | |
| mock_get_or_create.return_value = (mock_context, False) | |
| content = "Test" | |
| mock_content_object = Mock() | |
| mock_content_object.pk = 1 | |
| with patch( | |
| "apps.ai.models.context.ContentType.objects.get_for_model" | |
| ) as mock_get_for_model: | |
| mock_content_type = Mock() | |
| mock_content_type.get_source_expressions = Mock(return_value=[]) | |
| mock_get_for_model.return_value = mock_content_type | |
| result = Context.update_data(content, mock_content_object, source="src", save=True) | |
| mock_get_for_model.assert_called_once_with(mock_content_object) | |
| mock_get_or_create.assert_called_once_with( | |
| entity_type=mock_content_type, | |
| entity_id=1, | |
| defaults={"content": content, "source": "src"}, | |
| ) | |
| assert result == mock_context | |
| assert mock_context.content == content | |
| assert mock_context.source == "src" |
🤖 Prompt for AI Agents
In backend/tests/apps/ai/models/context_test.py around lines 142 to 168, the
test currently patches Context.objects.get but update_data now uses
get_or_create; change the test to patch Context.objects.get_or_create instead,
have it return (mock_context, False) to simulate existing context, and assert
get_for_model and get_or_create were called with the correct args
(entity_type=mock_content_type, entity_id=1); then verify the returned object is
mock_context, its content and source fields were updated, and that save was
invoked when save=True.
|
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
backend/tests/apps/ai/models/context_test.py (2)
298-337: Align “new context + save=True” test with a get_or_create-based implementation.If
Context.update_datais (or will be) implemented withget_or_create, patchingContext.__init__andobjects.getbecomes brittle. Prefer patchingobjects.get_or_createand assertingdefaultscarrycontentandsource. This also models the concurrency-safe path.Proposed rewrite:
-@patch("apps.ai.models.context.Context.objects.get") -@patch("apps.ai.models.context.Context.__init__") -def test_update_data_new_context_with_save(self, mock_init, mock_get): - """Test update_data creating a new context with save=True.""" - from apps.ai.models.context import Context as ContextModel - - mock_get.side_effect = ContextModel.DoesNotExist - mock_init.return_value = None # __init__ should return None +@patch("apps.ai.models.context.Context.objects.get_or_create") +def test_update_data_new_context_with_save(self, mock_get_or_create): + """Test update_data creating a new context with save=True.""" + from apps.ai.models.context import Context as ContextModel + new_ctx = create_model_mock(ContextModel) + mock_get_or_create.return_value = (new_ctx, True) @@ - with patch( - "apps.ai.models.context.ContentType.objects.get_for_model" - ) as mock_get_for_model: + with patch("apps.ai.models.context.ContentType.objects.get_for_model") as mock_get_for_model: mock_content_type = Mock(spec=ContentType) - mock_content_type.get_source_expressions = Mock(return_value=[]) mock_get_for_model.return_value = mock_content_type @@ - # Mock the context instance and its save method - with patch.object(ContextModel, "save") as mock_save: - result = Context.update_data( - content, mock_content_object, source=source, save=True - ) - - mock_get_for_model.assert_called_once_with(mock_content_object) - mock_get.assert_called_once_with( - entity_type=mock_content_type, - entity_id=1, - ) - mock_init.assert_called_once_with( - entity_type=mock_content_type, - entity_id=1, - ) - assert result.content == content - assert result.source == source - mock_save.assert_called_once() + result = Context.update_data(content, mock_content_object, source=source, save=True) + mock_get_for_model.assert_called_once_with(mock_content_object) + mock_get_or_create.assert_called_once_with( + entity_type=mock_content_type, + entity_id=1, + defaults={"content": content, "source": source}, + ) + assert result == new_ctxTo verify which implementation the code currently uses, run:
#!/bin/bash # Inspect update_data implementation in this branch rg -nC2 -P 'class\s+Context\b.*?@staticmethod.*?def\s+update_data|get_or_create|objects\.get\(' --type=py apps/ai/models/context.py
338-377: Do the same for the “new context + save=False” variant.Mirror the
get_or_createapproach and assert thatsaveis not forced whensave=False.-@patch("apps.ai.models.context.Context.objects.get") -@patch("apps.ai.models.context.Context.__init__") -def test_update_data_new_context_without_save(self, mock_init, mock_get): +@patch("apps.ai.models.context.Context.objects.get_or_create") +def test_update_data_new_context_without_save(self, mock_get_or_create): @@ - mock_get.side_effect = ContextModel.DoesNotExist - mock_init.return_value = None # __init__ should return None + new_ctx = create_model_mock(ContextModel) + mock_get_or_create.return_value = (new_ctx, True) @@ - with patch( - "apps.ai.models.context.ContentType.objects.get_for_model" - ) as mock_get_for_model: + with patch("apps.ai.models.context.ContentType.objects.get_for_model") as mock_get_for_model: mock_content_type = Mock(spec=ContentType) - mock_content_type.get_source_expressions = Mock(return_value=[]) mock_get_for_model.return_value = mock_content_type @@ - # Mock the context instance and its save method - with patch.object(ContextModel, "save") as mock_save: - result = Context.update_data( - content, mock_content_object, source=source, save=False - ) - - mock_get_for_model.assert_called_once_with(mock_content_object) - mock_get.assert_called_once_with( - entity_type=mock_content_type, - entity_id=1, - ) - mock_init.assert_called_once_with( - entity_type=mock_content_type, - entity_id=1, - ) - assert result.content == content - assert result.source == source - mock_save.assert_not_called() + result = Context.update_data(content, mock_content_object, source=source, save=False) + mock_get_for_model.assert_called_once_with(mock_content_object) + mock_get_or_create.assert_called_once_with( + entity_type=mock_content_type, + entity_id=1, + defaults={"content": content, "source": source}, + ) + assert result == new_ctx
🧹 Nitpick comments (9)
backend/tests/apps/ai/models/context_test.py (9)
12-19: Trim unused test double surface; tighten mock spec.
create_model_mocksets achunksrelation that isn’t used anywhere in this module. Extra attributes on specs make tests harder to maintain and can hide accidental usage. Also consider autospeccing to catch typos at call sites.Apply this diff to simplify:
def create_model_mock(model_class): - mock = Mock(spec=model_class) + mock = Mock(spec=model_class) mock._state = Mock() mock.pk = 1 mock.id = 1 - mock.chunks = Mock() - mock.chunks.count.return_value = 0 return mock
32-38: Assert FK target model explicitly to harden the test.You already verify
on_delete=CASCADE. Add an assertion that the FK actually points toContentType; this catches accidental regresses to a different model.def test_content_type_field_properties(self): field = Context._meta.get_field("entity_type") assert field.null is False assert field.blank is False assert hasattr(field, "remote_field") assert field.remote_field.on_delete.__name__ == "CASCADE" + assert field.remote_field.model is ContentType
49-61: Avoid patching model init; this test only exercises Django internals.Patching
Context.__init__is brittle and doesn’t validate our code paths. If we keep this test, it should focus onContext.update_data(save=True)callingsave(). Otherwise, it’s redundant with the dedicatedupdate_datatests below.Two options (pick one):
- Remove the test entirely (preferred for noise reduction), or
- Rewrite to cover
update_data(save=True)instead ofContext.save()in isolation.Removal diff:
-@patch("apps.ai.models.context.Context.save") -@patch("apps.ai.models.context.Context.__init__") -def test_context_creation_with_save(self, mock_init, mock_save): - mock_init.return_value = None - - content = "Test generated text" - source = "test_source" - - context = Context(content=content, source=source) - context.save() - - mock_save.assert_called_once()
65-97: Manager call-through tests add little value; consider trimming or upgrading to integration.Asserting
objects.create/filter/getinvocations mostly re-tests Django. Either:
- Convert to behavior-oriented tests that exercise your code paths which use these managers, or
- Mark as Django DB tests and assert real persistence semantics, or
- Remove to keep the suite lean.
98-121: Test field-level validation instead of patching full_clean.Mocking
full_cleanverifies nothing about validation. You can avoid DB I/O and still assert constraints using the field’sclean. This keeps the test fast and meaningful.-@patch("apps.ai.models.context.Context.full_clean") -def test_context_validation(self, mock_full_clean): - context = Context() - context.content = "Valid text" - context.source = "A" * 100 - - context.full_clean() - - mock_full_clean.assert_called_once() +def test_context_validation(self): + # Field-level validation should allow max length 100 without DB access. + context = Context(content="Valid text", source="A" * 100) + Context._meta.get_field("source").clean(context.source, context) -@patch("apps.ai.models.context.Context.full_clean") -def test_context_validation_source_too_long(self, mock_full_clean): - from django.core.exceptions import ValidationError - - mock_full_clean.side_effect = ValidationError("Source too long") - - context = Context() - context.content = "Valid text" - context.source = "A" * 101 - - with pytest.raises(ValidationError) as exc_info: - context.full_clean() - assert "Source too long" in str(exc_info.value) +def test_context_validation_source_too_long(self): + from django.core.exceptions import ValidationError + context = Context(content="Valid text") + with pytest.raises(ValidationError) as exc_info: + Context._meta.get_field("source").clean("A" * 101, context) + # Error wording may vary by Django version; keep assertion resilient. + assert "at most" in str(exc_info.value)
127-140: Drop refresh_from_db/delete stubs; they don’t exercise our code.These tests only assert that patched Django methods were called, which is tautological. If you need them, convert to higher-level behavior where these calls are side effects of our logic.
-@patch("apps.ai.models.context.Context.refresh_from_db") -def test_context_refresh_from_db(self, mock_refresh): - context = Context() - context.refresh_from_db() - - mock_refresh.assert_called_once() - -@patch("apps.ai.models.context.Context.delete") -def test_context_delete(self, mock_delete): - context = Context() - context.delete() - - mock_delete.assert_called_once()
141-167: Also assert save was invoked on existing-context update with save=True.
update_data(..., save=True)should persist changes even for existing contexts. Sincemock_contextis a spec’d model, you can verifysave()was called.assert result == mock_context assert mock_context.content == content assert mock_context.source == "src" + mock_context.save.assert_called_once()
141-167: Add a no-op-change test to codify the “always return a Context” guarantee.Based on the retrieved learnings,
update_datashould always return aContext, even whencontent/sourcedon’t change. Add a test to pin this behavior.Example test (place near the other
update_datatests):def test_update_data_no_changes_returns_context(self): mock_context = create_model_mock(Context) mock_context.content = "same" mock_context.source = "src" with patch("apps.ai.models.context.ContentType.objects.get_for_model") as mock_get_for_model, \ patch("apps.ai.models.context.Context.objects.get") as mock_get: mock_ct = Mock(spec=ContentType) mock_get_for_model.return_value = mock_ct mock_get.return_value = mock_context result = Context.update_data("same", Mock(pk=1), source="src", save=False) assert result is mock_contextI can push a commit with this (and the other tweaks) if you want me to.
Also applies to: 298-377
168-202: Improve test resilience around truncation helper semanticsThe test for
Context.__str__currently asserts a hardcoded mid-word truncation ("50 cha..."), which will break iftruncateever switches to word boundaries or changes its ellipsis. To make this more robust, consider one of the following approaches:• Use prefix/suffix checks plus a sanity substring check:
assert result.startswith("test_model Test Object: ") assert result.endswith("...") assert "This is test content" in result # sanity check on content presence• Or, for exact matching, compute the expectation with the same helper:
from apps.common.utils import truncate expected = f"test_model Test Object: {truncate(context.content, 50)}" assert result == expectedNote:
truncateis imported fromapps.common.utils(seebackend/apps/common/utils.pyat line 164).File: backend/tests/apps/ai/models/context_test.py: lines 168–202
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
backend/tests/apps/ai/models/context_test.py(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-16T12:07:38.124Z
Learnt from: Dishant1804
PR: OWASP/Nest#1891
File: backend/apps/ai/models/context.py:44-66
Timestamp: 2025-08-16T12:07:38.124Z
Learning: In the Context.update_data method, the Context object should always be returned (never None), even when no changes are made to the content or source fields. This ensures consistent behavior for callers.
Applied to files:
backend/tests/apps/ai/models/context_test.py
🧬 Code graph analysis (1)
backend/tests/apps/ai/models/context_test.py (2)
backend/apps/ai/models/context.py (2)
Context(15-61)update_data(39-61)backend/apps/common/models.py (1)
TimestampedModel(37-46)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Run backend tests
- GitHub Check: Run frontend unit tests
- GitHub Check: Run frontend e2e tests
- GitHub Check: CodeQL (javascript-typescript)
🔇 Additional comments (1)
backend/tests/apps/ai/models/context_test.py (1)
23-31: LGTM: Meta and content field checks are precise.The db_table/verbose_name assertions and TextField verification are appropriate and stable.
2 participants
Proposed change
Resolves #1873
Checklist
make check-testlocally; all checks and tests passed.