Conversation
…ludes automigration capabilties based on model and annotation changes.
|
Caution Review failedThe pull request is closed. WalkthroughThis update introduces a new domain composition architecture, replacing the previous monolithic domain models ( Changes
Sequence Diagram(s)Domain Composition and Multi-Store Sync (High-Level)sequenceDiagram
participant User as Domain User
participant Conversation as Domain Conversation
participant Document as Domain Document
participant IM as InsightMesh Data
participant Slack as Slack Data
participant GDrive as Google Drive Data
participant PG as PostgreSQL
participant Neo4j as Neo4j
participant ES as Elasticsearch
User->>IM: Load user data by email/ID
User->>Slack: Load user data by Slack ID
User->>User: Aggregate identity, expose business properties
Conversation->>IM: Load conversation/messages
Conversation->>Slack: (Optional) Load Slack threads
Conversation->>Conversation: Aggregate messages, summarize
Document->>GDrive: Search/retrieve Google Drive docs
Document->>Slack: Search/retrieve Slack files
Document->>Document: Aggregate metadata, sharing context
User->>PG: Save/update via SQLAlchemy
User->>Neo4j: Sync node/relationships (auto via annotation)
User->>ES: Sync index (auto via annotation)
Conversation->>PG: Save/update via SQLAlchemy
Conversation->>Neo4j: Sync node/relationships
Conversation->>ES: Sync index
Document->>PG: Save/update via SQLAlchemy
Document->>Neo4j: Sync node/relationships
Document->>ES: Sync index
Migration Workflow with Annotation DetectionsequenceDiagram
participant Dev as Developer
participant Model as Annotated Model
participant CLI as weave CLI
participant Detector as AnnotationMigrationDetector
participant Alembic as Alembic
participant Neo4j as Neo4j
participant ES as Elasticsearch
Dev->>Model: Add/change annotation decorators
Dev->>CLI: Run `weave db migrate create --auto`
CLI->>Detector: Scan models for annotation changes
Detector->>CLI: Return migration instructions
CLI->>Alembic: Generate/apply SQL migration (if needed)
CLI->>Neo4j: Generate/apply Cypher migration
CLI->>ES: Generate/apply Elasticsearch migration
CLI->>Dev: Report migration status
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (8)
✨ Finishing Touches
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. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 20
🔭 Outside diff range comments (1)
weave/bin/modules/cli_migrate.py (1)
382-389:⚠️ Potential issueMigration templates still contain placeholders – will generate invalid files
Both
_generate_neo4j_migrationand_generate_elasticsearch_migrationreturn strings with literal{revision_id},{down_revision},{create_date}.
Runningdetect_and_create_annotation_migrations()will therefore create unusable migration scripts.Recommend injecting concrete values before writing:
-def _generate_neo4j_migration(...): - ... - return f'''""" -{message} - -Revision ID: neo4j_{{revision_id}} -Revises: {{down_revision}} -Create Date: {{create_date}} -""" +def _generate_neo4j_migration(..., revision_id: str, down_revision: str = "base"): + ... + create_date = datetime.datetime.utcnow().isoformat() + return f'''""" +{message} + +Revision ID: neo4j_{revision_id} +Revises: {down_revision} +Create Date: {create_date} +"""Do the same for the Elasticsearch generator and pass
revision_idfromdetect_and_create_annotation_migrations.Also applies to: 450-455
🧹 Nitpick comments (46)
weave/bin/modules/cli_services.py (1)
179-190: Command rename breaks backward compatibility – add an aliasThe previous command was
weave service list; changing it tostatusis reasonable, but existing scripts & tooling will break.
Click allows manual alias registration:# after defining service_status service_group.add_command(service_status, 'list') # legacy aliasThat preserves existing behaviour while nudging users toward
statusin docs.Also consider renaming the underlying helper
list_servicestostatus_servicesfor clarity, or at least leave a TODO.weave/README.md (1)
108-115: Minor nit – keep command group hierarchy consistentDocumentation introduces
weave db tool install/status, but the CLI file names themcli_db_tools.pywith subgrouptool.
Double-check that the actual Click registration path is indeedweave db tool …; if the subgroup got namedtoolsin code the examples will 404.No code change required if already aligned.
domain/__init__.py (3)
4-7: Docstring still InsightMesh-centricNow that the package exports Slack, Google Drive and cross-platform domain objects, the heading “Domain models for InsightMesh” is misleading.
9-15: Eager imports may slow CLI startupImporting every heavy domain object at module load time triggers SQLAlchemy configuration, environment discovery, etc., even for scripts that only need a subset. Consider lazy re-exporting, e.g.:
import importlib def __getattr__(name): if name in _EXPORTS: return importlib.import_module(f".{_EXPORTS[name]}", __name__) raise AttributeError(name)This keeps the public surface identical while avoiding unnecessary initialisation.
17-32: Unsorted__all__Long export lists are easier to scan when alphabetised.
weave/bin/modules/cli_db_tools.py (1)
70-113: Duplicated tool-status logic
cli_migrate.check_and_install_tools()already has a complete status routine. Re-implementing it here risks drift. Prefer delegating and printing its returned structure, or refactor shared checks into a util module.🧰 Tools
🪛 Ruff (0.11.9)
74-74:
requestsimported but unused; consider usingimportlib.util.find_specto test for availability(F401)
75-75:
psycopg2imported but unused; consider usingimportlib.util.find_specto test for availability(F401)
76-76:
alembicimported but unused; consider usingimportlib.util.find_specto test for availability(F401)
domain/data/slack/user.py (1)
40-47:display_name_or_namecould includereal_namefallbackCurrent logic omits
real_nameif bothdisplay_nameandnameare empty. Consider:return self.display_name or self.name or self.real_name(consistent with domain/user.py usage).
tests/test_annotations.py (2)
152-154: Prefer truthiness over explicit== True-assert user.is_active_user() == True +assert user.is_active_user()🧰 Tools
🪛 Ruff (0.11.9)
153-153: Avoid equality comparisons to
True; useif user.is_active_user():for truth checksReplace with
user.is_active_user()(E712)
162-163: Prefernotover== False-assert bot_user.is_active_user() == False +assert not bot_user.is_active_user()🧰 Tools
🪛 Ruff (0.11.9)
162-162: Avoid equality comparisons to
False; useif not bot_user.is_active_user():for false checksReplace with
not bot_user.is_active_user()(E712)
domain/data/insightmesh/user.py (1)
38-41:is_active_usermay returnNoneIf
is_activeis nullable,return self.is_activemay yieldNone, which is falsy but ambiguous. Consider an explicit boolean cast or default:return bool(self.is_active)README-migrations.md (1)
240-245: Duplicate list numbering – renumber for clarityThe “General” best-practices list restarts numbering at
2.and3., which is confusing to the reader.-1. **Use smart commands** - `weave db migrate <database>` auto-detects the right tool -2. **Check tool status** - `weave db tool status` shows available migration tools -3. **Install missing tools** - `weave db tool install` installs required dependencies -2. **Check database info** - `weave db info` shows all databases and their types -3. **Always backup** before running migrations in production -4. **Test migrations** in development first -5. **Version control** all migration files +1. **Use smart commands** – `weave db migrate <database>` auto-detects the right tool +2. **Check tool status** – `weave db tool status` shows available migration tools +3. **Install missing tools** – `weave db tool install` installs required dependencies +4. **Check database info** – `weave db info` shows all databases and their types +5. **Always back up** before running migrations in production +6. **Test migrations** in development first +7. **Version-control** all migration filesdomain/data/insightmesh/conversation.py (2)
42-50:display_titlemay accessself.idbefore flushIf
display_titleis called on a transient instance (before it’s been persisted and PK generated),self.idwill beNone, yielding"Conversation None".
Consider a safer fallback:-return self.title or f"Conversation {self.id}" +return self.title or (f"Conversation {self.id}" if self.id is not None else "Untitled Conversation")
51-60:get_conversation_summary– missingconversation_metadataDown-stream consumers may expect metadata; omitting it forces another DB round-trip. Add it unless intentionally excluded.
examples/multi_store_usage.py (2)
10-14: Remove unused imports flagged by Ruff
SlackChannel,SlackBase, andbulk_sync_to_storesare imported but never referenced.-from domain.data.slack import SlackUser, SlackChannel, SlackBase -from weave.bin.modules.annotations.sync import bulk_sync_to_stores +from domain.data.slack import SlackUser🧰 Tools
🪛 Ruff (0.11.9)
12-12:
domain.data.slack.SlackChannelimported but unusedRemove unused import
(F401)
12-12:
domain.data.slack.SlackBaseimported but unusedRemove unused import
(F401)
13-13:
weave.bin.modules.annotations.sync.bulk_sync_to_storesimported but unusedRemove unused import:
weave.bin.modules.annotations.sync.bulk_sync_to_stores(F401)
18-23: Credentials hard-coded in demo connection string
postgresql://postgres:postgres@localhost:5432/slackexposes a password in VCS. Replace with a placeholder or environment variable to avoid copy-paste leaks.domain/data/slack/channel.py (1)
52-57: display_info – guard againstNonevalues
self.num_membersmay beNone;0fallback is good, but privacy / status may also beNone. Consider defaults to avoid"None"in UI strings.weave/bin/modules/annotations/README.md (1)
1-200: Documentation looks solid – just a minor nitpickGreat job on the thorough README. Consider adding a short “Limitations / Gotchas” section (e.g. eventual-consistency caveats, Neo4j delete semantics) so newcomers know the current boundaries of the system.
examples/google_drive_document_demo.py (2)
9-13: Remove unused import to keep Ruff/F401 happy
DocumentFormatis imported but never referenced, triggering Ruff’s F401 warning.-from domain import ( - Document, DocumentFormat, DocumentSource, +from domain import ( + Document, DocumentSource,🧰 Tools
🪛 Ruff (0.11.9)
10-10:
domain.DocumentFormatimported but unusedRemove unused import:
domain.DocumentFormat(F401)
270-273: Avoid spawning a fresh event-loop per demo
asyncio.run()tears down and recreates the event loop on every iteration which is unnecessary and slow. Wrap the demos in a singlemain()coroutine instead:-for i, demo in enumerate(demos, 1): - asyncio.run(demo()) - if i < len(demos): - print("\n" + "=" * 60 + "\n") +async def main(): + for i, demo in enumerate(demos, 1): + await demo() + if i < len(demos): + print("\n" + "=" * 60 + "\n") + +if __name__ == "__main__": + asyncio.run(main())weave/bin/modules/annotations/sync.py (2)
7-7: Drop unused typing import
Anyisn’t used anywhere in this module.-from typing import Type, Any +from typing import Type🧰 Tools
🪛 Ruff (0.11.9)
7-7:
typing.Anyimported but unusedRemove unused import:
typing.Any(F401)
81-99: Useyield_per()to avoid loading entire tables in bulk sync
querywithout batching can exhaust memory on large tables.- for instance in query: + for instance in query.yield_per(1000):domain/data/insightmesh/message.py (2)
43-45: Avoid duplicating preview logic
__repr__re-implements the same slicing done by thecontent_previewproperty. Re-use the property to keep logic in one place:- content_preview = self.content[:50] + "..." if self.content and len(self.content) > 50 else self.content - return f"<Message(id={self.id}, role='{self.role}', content='{content_preview}')>" + return f"<Message(id={self.id}, role='{self.role}', content='{self.content_preview}')>"
48-59: Consider@propertyfor role checksSmall readability win:
@property def is_user(self) -> bool: return self.role == 'user'Then reference
msg.is_userinstead ofmsg.is_user_message().domain/data/insightmesh/context.py (1)
54-59:json.dumpscan fail for non-serialisable contentIf
self.contentcontains non-JSON-serialisable objects,json.dumpswill raise. You may wish to wrap intry/exceptor useorjson.dumps()with default handlers.weave/bin/modules/annotations/search.py (4)
5-6: Remove unused imports
functools,Callable, andTypeare never referenced here → drop them to keep the module clean and keep Ruff quiet.🧰 Tools
🪛 Ruff (0.11.9)
5-5:
functoolsimported but unusedRemove unused import:
functools(F401)
6-6:
typing.Callableimported but unusedRemove unused import
(F401)
6-6:
typing.Typeimported but unusedRemove unused import
(F401)
78-83: Expose basic connection tuningConsider surfacing SSL / auth / timeout parameters (or reading
ELASTICSEARCH_USERNAME/PASSWORDenv-vars) so the client can be used against secured clusters without editing code.
150-188: Return the list of hits, not the entire hits envelope
search()currently returnsresponse['hits'], which still containstotal,max_score, and the nestedhitslist.
Down-stream callers usually expect just the documents:- return response['hits'] + return response['hits']['hits']
217-247: Fragile SQLAlchemy→ES type mapping
str(column.type).startswith('VARCHAR') …relies on the DB dialect’s__str__output and breaks for custom types.
Preferisinstance(column.type, sqlalchemy.String)/sqlalchemy.types.Stringchecks (and includeFloat,Numeric, etc.).🧰 Tools
🪛 Ruff (0.11.9)
219-219: Do not call
getattrwith a constant attribute value. It is not any safer than normal property access.Replace
getattrwith attribute access(B009)
weave/bin/modules/annotations/graph.py (3)
5-6: Prune unused imports
functoolsandCallableare imported but never used.🧰 Tools
🪛 Ruff (0.11.9)
5-5:
functoolsimported but unusedRemove unused import:
functools(F401)
6-6:
typing.Callableimported but unusedRemove unused import:
typing.Callable(F401)
54-59: Duplicate mix-in injectionThe guard only checks for
sync_to_neo4j; subclasses of an already-decorated parent will reinject methods, overwriting any overrides.
Add anissubclasscheck or guard on an internal flag to avoid this.
173-175: Static-analysis: avoid unnecessarygetattr
config = cls._neo4j_node_configis clearer and a tad faster thangetattrwith a constant name.🧰 Tools
🪛 Ruff (0.11.9)
173-173: Do not call
getattrwith a constant attribute value. It is not any safer than normal property access.Replace
getattrwith attribute access(B009)
examples/domain_composition_demo.py (2)
10-10: Remove unused imports
datetimeandtimedeltaare only mentioned inside f-strings, never executed. Drop them or move the examples to real code blocks.🧰 Tools
🪛 Ruff (0.11.9)
10-10:
datetime.datetimeimported but unusedRemove unused import
(F401)
10-10:
datetime.timedeltaimported but unusedRemove unused import
(F401)
178-183: Asyncmain()adds complexity with no awaits
main()is declaredasyncbut performs only synchronous I/O. Consider making it a regular function and calling it directly, or actually awaiting real async work.WEAVE_ANNOTATIONS_SUMMARY.md (1)
126-126: Markdown lint – emphasis used as headingLine 126 appears as
**Applies migrations across all systems**; use a proper heading (###) to satisfy MD036.🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
126-126: Emphasis used instead of a heading
null(MD036, no-emphasis-as-heading)
UPDATED_MODELS_SUMMARY.md (1)
41-43: Capitalize “Slack”Proper noun should be capitalised (“Slack”, not “slack”) to avoid grammar lint warnings.
weave/bin/modules/cli_db.py (3)
24-96: Function far too large – extract helpers
db_migrate_smart()tops 90 statements and >20 locals, triggering R0912/R0914/R0915.
Split the SQL/Neo4j/ES handling (and the dry-run printer) into dedicated private helpers to improve testability and silence the linter.🧰 Tools
🪛 Ruff (0.11.9)
31-31:
.config.get_all_databasesimported but unusedRemove unused import:
.config.get_all_databases(F401)
44-44:
.config.get_sql_databasesimported but unusedRemove unused import
(F401)
44-44:
.config.get_graph_databasesimported but unusedRemove unused import
(F401)
44-44:
.config.get_search_databasesimported but unusedRemove unused import
(F401)
🪛 Pylint (3.3.7)
[refactor] 24-24: Too many local variables (23/15)
(R0914)
[refactor] 89-96: Unnecessary "elif" after "return", remove the leading "el" from "elif"
(R1705)
[refactor] 24-24: Too many branches (25/12)
(R0912)
[refactor] 24-24: Too many statements (90/50)
(R0915)
310-313: Unused import sneaked in
get_database_migration_toolis imported but never used, raising Ruff F401.
Simply drop it:-from .config import (get_managed_databases, get_database_type, - get_database_migration_tool) +from .config import get_managed_databases, get_database_type🧰 Tools
🪛 Ruff (0.11.9)
311-311:
.config.get_database_migration_toolimported but unusedRemove unused import:
.config.get_database_migration_tool(F401)
617-618: Extraneousf-prefix
console.print(f"[blue]💡 Use 'weave db migrate all' ...")has no interpolations → Ruff F541.-console.print(f"[blue]💡 Use 'weave db migrate all' to run migrations for all databases[/blue]") +console.print("[blue]💡 Use 'weave db migrate all' to run migrations for all databases[/blue]")🧰 Tools
🪛 Ruff (0.11.9)
617-617: f-string without any placeholders
Remove extraneous
fprefix(F541)
examples/agent_domain_integration.py (1)
28-29: Remove unused intra-function imports
DocumentType,Document, andConversationare imported but never referenced, tripping Ruff F401 four times.
Drop them to keep runtime light and avoid circular-import surprises.- from domain import Document, DocumentType + from domain import Document ... - from domain import Document + # no import needed, already available above ... - from domain import Conversation + # import not required here ... - from domain import User, Conversation, Document + from domain import User, DocumentAlso applies to: 97-98, 241-242, 263-264
🧰 Tools
🪛 Ruff (0.11.9)
28-28:
domain.DocumentTypeimported but unusedRemove unused import:
domain.DocumentType(F401)
weave/bin/modules/annotation_migration_detector.py (2)
8-16: Prune unused imports
sys,Set, andhashlibare never referenced → Ruff F401.
Delete them to keep the module clean.🧰 Tools
🪛 Ruff (0.11.9)
10-10:
sysimported but unusedRemove unused import:
sys(F401)
12-12:
typing.Setimported but unusedRemove unused import:
typing.Set(F401)
15-15:
hashlibimported but unusedRemove unused import:
hashlib(F401)
117-119: Lost exception contextThe generic
except Exception as e: … continueswallows import errors silently and never logse, making debugging painful.- except Exception as e: - # Skip modules that can't be imported - continue + except Exception as exc: + print(f"[AnnotationDetector] Skipped {module_path}: {exc}") + continue🧰 Tools
🪛 Ruff (0.11.9)
117-117: Local variable
eis assigned to but never usedRemove assignment to unused variable
e(F841)
domain/user.py (2)
9-11: Drop unused symbols
datetimeandUnionare imported but unused – remove to satisfy Ruff.🧰 Tools
🪛 Ruff (0.11.9)
9-9:
typing.Unionimported but unusedRemove unused import:
typing.Union(F401)
10-10:
datetime.datetimeimported but unusedRemove unused import:
datetime.datetime(F401)
148-155: Simplifyis_activepropertyThe
elifis redundant after the early return and flagged by Pylint.- if self._insightmesh_user and self._insightmesh_user.is_active_user(): - return True - elif self._slack_user and self._slack_user.is_active_user(): - return True - return False + return ( + (self._insightmesh_user and self._insightmesh_user.is_active_user()) or + (self._slack_user and self._slack_user.is_active_user()) + )🧰 Tools
🪛 Ruff (0.11.9)
150-153: Combine
ifbranches using logicaloroperatorCombine
ifbranches(SIM114)
🪛 Pylint (3.3.7)
[refactor] 150-153: Unnecessary "elif" after "return", remove the leading "el" from "elif"
(R1705)
domain/conversation.py (1)
9-9: Remove unusedUnionimport
Unionis never referenced – drop it to keep the import list lean.-from typing import Optional, List, Dict, Any, Union +from typing import Optional, List, Dict, Any🧰 Tools
🪛 Ruff (0.11.9)
9-9:
typing.Unionimported but unusedRemove unused import:
typing.Union(F401)
domain/document.py (2)
202-244: Ensure result limit & deduplication when aggregating Slack files
search_by_contentcan return far more thanlimitbecause each Slack message may contain multiple files and you append every one before the final slice. Two side-effects:
- Unnecessary memory/CPU.
- Potential duplicate documents (same file attached in several messages).
Consider:
seen = set() for hit in ...: for file_data in files: fid = file_data.get("id") if fid in seen or len(documents) >= limit: continue seen.add(fid) ...This enforces the limit and prevents duplicates.
414-440: Simplify conditional chainThe early
returnstatements make the subsequentelifblocks redundant. Switching to flatifstatements shortens the code and appeases pylint R1705 without changing behaviour.(Not critical, purely stylistic.)
🧰 Tools
🪛 Pylint (3.3.7)
[refactor] 416-433: Unnecessary "elif" after "return", remove the leading "el" from "elif"
(R1705)
[refactor] 414-414: Too many return statements (10/6)
(R0911)
[refactor] 438-438: Too many return statements (9/6)
(R0911)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (43)
.gitignore(1 hunks).weave/config.json(1 hunks).weave/contexts/conversations.yaml(0 hunks).weave/tools/slack.yaml(0 hunks).weave/tools/webcat.yaml(0 hunks)README-migrations.md(5 hunks)UPDATED_MODELS_SUMMARY.md(1 hunks)WEAVE_ANNOTATIONS_SUMMARY.md(1 hunks)domain/__init__.py(1 hunks)domain/channels.py(0 hunks)domain/conversation.py(1 hunks)domain/data/insightmesh/__init__.py(1 hunks)domain/data/insightmesh/context.py(2 hunks)domain/data/insightmesh/conversation.py(2 hunks)domain/data/insightmesh/message.py(1 hunks)domain/data/insightmesh/user.py(2 hunks)domain/data/slack/__init__.py(1 hunks)domain/data/slack/channel.py(2 hunks)domain/data/slack/user.py(2 hunks)domain/document.py(1 hunks)domain/messages.py(0 hunks)domain/person.py(0 hunks)domain/user.py(1 hunks)examples/agent_domain_integration.py(1 hunks)examples/domain_composition_demo.py(1 hunks)examples/google_drive_document_demo.py(1 hunks)examples/multi_store_usage.py(1 hunks)examples/updated_models_demo.py(1 hunks)tests/test_annotations.py(1 hunks)weave/README.md(4 hunks)weave/__init__.py(1 hunks)weave/bin/modules/annotation_migration_detector.py(1 hunks)weave/bin/modules/annotations/README.md(1 hunks)weave/bin/modules/annotations/__init__.py(1 hunks)weave/bin/modules/annotations/graph.py(1 hunks)weave/bin/modules/annotations/search.py(1 hunks)weave/bin/modules/annotations/sync.py(1 hunks)weave/bin/modules/cli.py(1 hunks)weave/bin/modules/cli_db.py(12 hunks)weave/bin/modules/cli_db_tools.py(1 hunks)weave/bin/modules/cli_migrate.py(6 hunks)weave/bin/modules/cli_services.py(1 hunks)weave/requirements.txt(1 hunks)
💤 Files with no reviewable changes (6)
- .weave/contexts/conversations.yaml
- .weave/tools/webcat.yaml
- .weave/tools/slack.yaml
- domain/person.py
- domain/messages.py
- domain/channels.py
🧰 Additional context used
🧬 Code Graph Analysis (9)
domain/data/slack/__init__.py (3)
domain/data/slack/user.py (1)
SlackUser(21-50)domain/data/slack/channel.py (1)
SlackChannel(26-56)weave/bin/modules/annotations/sync.py (1)
enable_auto_sync_for_model(69-78)
domain/data/insightmesh/__init__.py (5)
domain/data/insightmesh/user.py (1)
InsightMeshUser(21-55)domain/data/insightmesh/context.py (1)
Context(26-78)domain/data/insightmesh/conversation.py (1)
Conversation(26-60)domain/data/insightmesh/message.py (1)
Message(31-77)weave/bin/modules/annotations/sync.py (1)
enable_auto_sync_for_model(69-78)
domain/__init__.py (3)
domain/user.py (2)
User(28-256)UserIdentity(19-25)domain/conversation.py (3)
Conversation(40-423)ConversationIdentity(29-37)ConversationType(19-25)domain/document.py (9)
Document(69-586)DocumentIdentity(53-66)DocumentFormat(36-49)DocumentSource(30-33)search_google_docs(590-593)search_slack_files(595-597)get_recent_google_drive_activity(599-601)get_recent_slack_files(603-605)get_recent_document_activity(607-609)
weave/bin/modules/cli_db_tools.py (1)
weave/bin/modules/cli_migrate.py (3)
check_and_install_tools(755-813)install_python_dependencies(539-562)install_neo4j_migrations(564-753)
domain/data/insightmesh/user.py (4)
weave/bin/modules/annotations/graph.py (2)
neo4j_node(31-61)neo4j_relationship(64-92)weave/bin/modules/annotations/search.py (1)
elasticsearch_index(25-64)domain/user.py (2)
name(130-136)domain/data/slack/user.py (1)
is_active_user(48-50)
domain/data/slack/channel.py (2)
weave/bin/modules/annotations/graph.py (2)
neo4j_node(31-61)neo4j_relationship(64-92)weave/bin/modules/annotations/search.py (1)
elasticsearch_index(25-64)
weave/bin/modules/annotations/sync.py (2)
weave/bin/modules/annotations/search.py (2)
delete_from_elasticsearch(131-147)sync_to_elasticsearch(115-129)weave/bin/modules/annotations/graph.py (2)
sync_to_neo4j(136-152)sync_relationships_to_neo4j(166-197)
weave/bin/modules/annotations/graph.py (1)
weave/bin/modules/annotations/search.py (1)
decorator(44-63)
domain/user.py (3)
domain/data/insightmesh/user.py (2)
InsightMeshUser(21-55)display_name(43-45)domain/data/slack/user.py (1)
display_name_or_name(44-46)domain/conversation.py (2)
is_active(253-262)get_loaded_sources(395-397)
🪛 Ruff (0.11.9)
tests/test_annotations.py
153-153: Avoid equality comparisons to True; use if user.is_active_user(): for truth checks
Replace with user.is_active_user()
(E712)
162-162: Avoid equality comparisons to False; use if not bot_user.is_active_user(): for false checks
Replace with not bot_user.is_active_user()
(E712)
examples/multi_store_usage.py
12-12: domain.data.slack.SlackChannel imported but unused
Remove unused import
(F401)
12-12: domain.data.slack.SlackBase imported but unused
Remove unused import
(F401)
13-13: weave.bin.modules.annotations.sync.bulk_sync_to_stores imported but unused
Remove unused import: weave.bin.modules.annotations.sync.bulk_sync_to_stores
(F401)
weave/bin/modules/cli_db_tools.py
65-65: .cli_migrate.check_and_install_tools imported but unused
Remove unused import: .cli_migrate.check_and_install_tools
(F401)
74-74: requests imported but unused; consider using importlib.util.find_spec to test for availability
(F401)
75-75: psycopg2 imported but unused; consider using importlib.util.find_spec to test for availability
(F401)
76-76: alembic imported but unused; consider using importlib.util.find_spec to test for availability
(F401)
domain/data/insightmesh/user.py
8-8: weave.bin.modules.annotations.neo4j_relationship imported but unused
Remove unused import: weave.bin.modules.annotations.neo4j_relationship
(F401)
domain/data/slack/user.py
8-8: weave.bin.modules.annotations.neo4j_relationship imported but unused
Remove unused import: weave.bin.modules.annotations.neo4j_relationship
(F401)
examples/google_drive_document_demo.py
10-10: domain.DocumentFormat imported but unused
Remove unused import: domain.DocumentFormat
(F401)
weave/bin/modules/annotations/sync.py
7-7: typing.Any imported but unused
Remove unused import: typing.Any
(F401)
weave/bin/modules/annotation_migration_detector.py
10-10: sys imported but unused
Remove unused import: sys
(F401)
12-12: typing.Set imported but unused
Remove unused import: typing.Set
(F401)
15-15: hashlib imported but unused
Remove unused import: hashlib
(F401)
117-117: Local variable e is assigned to but never used
Remove assignment to unused variable e
(F841)
examples/domain_composition_demo.py
10-10: datetime.datetime imported but unused
Remove unused import
(F401)
10-10: datetime.timedelta imported but unused
Remove unused import
(F401)
weave/bin/modules/annotations/search.py
5-5: functools imported but unused
Remove unused import: functools
(F401)
6-6: typing.Callable imported but unused
Remove unused import
(F401)
6-6: typing.Type imported but unused
Remove unused import
(F401)
140-147: Use contextlib.suppress(Exception) instead of try-except-pass
(SIM105)
219-219: Do not call getattr with a constant attribute value. It is not any safer than normal property access.
Replace getattr with attribute access
(B009)
weave/bin/modules/annotations/graph.py
5-5: functools imported but unused
Remove unused import: functools
(F401)
6-6: typing.Callable imported but unused
Remove unused import: typing.Callable
(F401)
173-173: Do not call getattr with a constant attribute value. It is not any safer than normal property access.
Replace getattr with attribute access
(B009)
examples/agent_domain_integration.py
28-28: domain.DocumentType imported but unused
Remove unused import: domain.DocumentType
(F401)
97-97: domain.Document imported but unused
Remove unused import: domain.Document
(F401)
241-241: domain.Conversation imported but unused
Remove unused import: domain.Conversation
(F401)
263-263: domain.Conversation imported but unused
Remove unused import: domain.Conversation
(F401)
domain/user.py
9-9: typing.Union imported but unused
Remove unused import: typing.Union
(F401)
10-10: datetime.datetime imported but unused
Remove unused import: datetime.datetime
(F401)
150-153: Combine if branches using logical or operator
Combine if branches
(SIM114)
weave/bin/modules/cli_db.py
198-201: Use ternary operator action = f'downgrade {revision}' if revision else 'downgrade -1' instead of if-else-block
(SIM108)
311-311: .config.get_database_migration_tool imported but unused
Remove unused import: .config.get_database_migration_tool
(F401)
429-429: pathlib.Path imported but unused
Remove unused import: pathlib.Path
(F401)
617-617: f-string without any placeholders
Remove extraneous f prefix
(F541)
domain/conversation.py
9-9: typing.Union imported but unused
Remove unused import: typing.Union
(F401)
🪛 markdownlint-cli2 (0.17.2)
WEAVE_ANNOTATIONS_SUMMARY.md
126-126: Emphasis used instead of a heading
null
(MD036, no-emphasis-as-heading)
🪛 Pylint (3.3.7)
weave/bin/modules/annotation_migration_detector.py
[refactor] 41-41: Too few public methods (1/2)
(R0903)
weave/bin/modules/annotations/search.py
[refactor] 25-25: Too many arguments (7/5)
(R0913)
[refactor] 25-25: Too many positional arguments (7/5)
(R0917)
domain/user.py
[refactor] 83-83: Too many branches (16/12)
(R0912)
[refactor] 132-135: Unnecessary "elif" after "return", remove the leading "el" from "elif"
(R1705)
[refactor] 141-144: Unnecessary "elif" after "return", remove the leading "el" from "elif"
(R1705)
[refactor] 150-153: Unnecessary "elif" after "return", remove the leading "el" from "elif"
(R1705)
weave/bin/modules/cli_migrate.py
[refactor] 490-493: Unnecessary "else" after "return", remove the "else" and de-indent the code inside it
(R1705)
weave/bin/modules/cli_db.py
[refactor] 24-24: Too many local variables (23/15)
(R0914)
[refactor] 24-24: Too many branches (25/12)
(R0912)
[refactor] 24-24: Too many statements (90/50)
(R0915)
[refactor] 89-96: Unnecessary "elif" after "return", remove the leading "el" from "elif"
(R1705)
domain/document.py
[refactor] 53-53: Too many instance attributes (11/7)
(R0902)
[refactor] 253-305: Unnecessary "elif" after "return", remove the leading "el" from "elif"
(R1705)
[refactor] 416-433: Unnecessary "elif" after "return", remove the leading "el" from "elif"
(R1705)
[refactor] 414-414: Too many return statements (10/6)
(R0911)
[refactor] 442-457: Unnecessary "elif" after "return", remove the leading "el" from "elif"
(R1705)
[refactor] 438-438: Too many return statements (9/6)
(R0911)
domain/conversation.py
[refactor] 105-105: Too many arguments (6/5)
(R0913)
[refactor] 105-105: Too many positional arguments (6/5)
(R0917)
🪛 LanguageTool
UPDATED_MODELS_SUMMARY.md
[grammar] ~47-~47: Did you mean the communication tool “Slack” (= proper noun, capitalized)?
Context: ... → SlackUser - Elasticsearch: Index slack_channels, searches name, purpose, topi...
(ON_SKYPE)
🔇 Additional comments (7)
.gitignore (1)
191-191: Pattern looks good – no issues
Adding.weave/tools/*to the ignore list is consistent with the tool-directory cleanup and won’t affect build artefacts.weave/__init__.py (1)
5-5: Remember to propagate the version bump
__version__is now0.1.5, but other packaging metadata (e.g.pyproject.toml,setup.cfg, release notes) are not visible here. Double-check that every place publishing/CI relies on is bumped as well and a matching Git tag / changelog entry is created..weave/config.json (1)
23-23: Minor placeholder tweak acknowledged
Changing the hint fromservice list→service statuskeeps the docs in sync with the CLI rename. No functional impact.weave/bin/modules/annotations/__init__.py (1)
1-22: LGTM – clear public surfaceFile re-exports are clean and
__all__is exhaustive.
No immediate issues.README-migrations.md (1)
43-50: Verify--dry-runflag is implemented at CLI levelDocs introduce
weave db migrate all --dry-run, but the flag will confuse users if it is not actually accepted bycli_db.py.
Confirm the option exists (or update docs accordingly).examples/updated_models_demo.py (1)
1-8: Good explanatory demo scriptPurely illustrative, no execution-time side-effects and clear separation of concerns.
domain/conversation.py (1)
236-238: Possible double-count of messages
message_countblindly sums the three internal lists. When_load_cross_platform_messagespulls messages from several InsightMesh conversations the same message could be appended more than once (e.g., if a conversation is processed twice by different filters). Consider de-duplicating on ID or using asetto guarantee accurate counts.
| from weave.bin.modules.annotations.sync import enable_auto_sync_for_model | ||
|
|
||
| # Enable automatic synchronization for all models | ||
| enable_auto_sync_for_model(SlackUser) | ||
| enable_auto_sync_for_model(SlackChannel) |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Avoid repetition & reduce import-time side effects
- Repeating
enable_auto_sync_for_modelfor every model doesn’t scale. - Calling sync at import time can trigger DB/driver initialisation in environments (e.g. migrations, unit tests) where the backing stores are unavailable.
Consider deferring the call or at least deduplicating:
-# Enable automatic synchronization for all models
-enable_auto_sync_for_model(SlackUser)
-enable_auto_sync_for_model(SlackChannel)
+# Enable automatic synchronization for all models defined here
+for _model in (SlackUser, SlackChannel):
+ enable_auto_sync_for_model(_model)Long-term, a central application bootstrap that opts-in to auto-sync (instead of doing it in __init__) would avoid import-time surprises.
🏁 Script executed:
#!/bin/bash
# Locate all references to enable_auto_sync_for_model
rg -n "enable_auto_sync_for_model" -C4Length of output: 4398
Deduplicate auto-sync calls & avoid import-time side-effects
The repeated calls to enable_auto_sync_for_model in domain/data/slack/__init__.py don’t scale and will run at import time (which can trigger DB/driver initialization in contexts like migrations or tests).
• File: domain/data/slack/init.py
Lines: 8–12
Suggested patch:
-from weave.bin.modules.annotations.sync import enable_auto_sync_for_model
-
-# Enable automatic synchronization for all models
-enable_auto_sync_for_model(SlackUser)
-enable_auto_sync_for_model(SlackChannel)
+from weave.bin.modules.annotations.sync import enable_auto_sync_for_model
+
+# Enable automatic synchronization for all models defined here
+for model in (SlackUser, SlackChannel):
+ enable_auto_sync_for_model(model)Long-term, consider moving all auto-sync registration into a central application bootstrap (opt-in) so imports never trigger side effects.
📝 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.
| from weave.bin.modules.annotations.sync import enable_auto_sync_for_model | |
| # Enable automatic synchronization for all models | |
| enable_auto_sync_for_model(SlackUser) | |
| enable_auto_sync_for_model(SlackChannel) | |
| from weave.bin.modules.annotations.sync import enable_auto_sync_for_model | |
| # Enable automatic synchronization for all models defined here | |
| for model in (SlackUser, SlackChannel): | |
| enable_auto_sync_for_model(model) |
🤖 Prompt for AI Agents
In domain/data/slack/__init__.py around lines 8 to 12, the repeated calls to
enable_auto_sync_for_model for each model cause import-time side effects and do
not scale. Refactor by consolidating these calls into a single function that
registers all models at once, and avoid calling this function at import time.
Instead, defer the execution to a central application bootstrap or an explicit
initialization step to prevent unwanted DB or driver initialization during
imports.
| # Enable automatic synchronization for all models | ||
| enable_auto_sync_for_model(InsightMeshUser) | ||
| enable_auto_sync_for_model(Context) | ||
| enable_auto_sync_for_model(Conversation) | ||
| enable_auto_sync_for_model(Message) |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Same DRY / bootstrap concern as for Slack package
The per-model calls duplicate logic and run at import time. Mirror the refactor suggested for the Slack package:
-# Enable automatic synchronization for all models
-enable_auto_sync_for_model(InsightMeshUser)
-enable_auto_sync_for_model(Context)
-enable_auto_sync_for_model(Conversation)
-enable_auto_sync_for_model(Message)
+# Enable automatic synchronization for all models in this package
+for _model in (InsightMeshUser, Context, Conversation, Message):
+ enable_auto_sync_for_model(_model)If auto-sync requires external services, consider gating it behind an environment flag or explicit application bootstrap to prevent accidental side-effects in offline contexts.
🏁 Script executed:
#!/bin/bash
# Search for all usages of enable_auto_sync_for_model across the codebase
rg -n "enable_auto_sync_for_model"Length of output: 1244
🏁 Script executed:
#!/bin/bash
# Display the first 200 lines of the sync module to review enable_auto_sync_for_model implementation
sed -n '1,200p' weave/bin/modules/annotations/sync.pyLength of output: 3873
Consolidate auto-sync registration into a loop and avoid import-time side-effects
Refactor the per-model calls in domain/data/insightmesh/__init__.py (lines 12–16) to a single loop:
- # Enable automatic synchronization for all models
- enable_auto_sync_for_model(InsightMeshUser)
- enable_auto_sync_for_model(Context)
- enable_auto_sync_for_model(Conversation)
- enable_auto_sync_for_model(Message)
+ # Enable automatic synchronization for all models in this package
+ for model in (InsightMeshUser, Context, Conversation, Message):
+ enable_auto_sync_for_model(model)• This mirrors the DRY refactor suggested (and needed) for the Slack package in domain/data/slack/__init__.py.
• Since enable_auto_sync_for_model registers SQLAlchemy event listeners at import time, consider gating this behind an environment flag (e.g. ENABLE_AUTO_SYNC) or moving it into an explicit application-startup step to prevent unintended side-effects in offline or testing contexts.
📝 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.
| # Enable automatic synchronization for all models | |
| enable_auto_sync_for_model(InsightMeshUser) | |
| enable_auto_sync_for_model(Context) | |
| enable_auto_sync_for_model(Conversation) | |
| enable_auto_sync_for_model(Message) | |
| # Enable automatic synchronization for all models in this package | |
| for model in (InsightMeshUser, Context, Conversation, Message): | |
| enable_auto_sync_for_model(model) |
🤖 Prompt for AI Agents
In domain/data/insightmesh/__init__.py lines 12 to 16, the calls to
enable_auto_sync_for_model are duplicated for each model and run at import time,
causing potential side-effects. Refactor by consolidating these calls into a
single loop iterating over the models (InsightMeshUser, Context, Conversation,
Message). Additionally, wrap this loop in a conditional that checks an
environment variable like ENABLE_AUTO_SYNC to prevent automatic registration
during offline or testing contexts, ensuring the auto-sync setup occurs only
during explicit application startup.
| # Database packages needed for weave db commands | ||
| neo4j-driver==5.28.1 | ||
| alembic==1.16.1 | ||
| psycopg2-binary==2.9.10 | ||
| sqlalchemy==2.0.41 | ||
| # Additional packages for elasticsearch and other features | ||
| elasticsearch==8.9.0 | ||
| requests>=2.31.0 No newline at end of file |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Version-pinning strategy is inconsistent and may cause subtle deployment issues
You pin most of the new dependencies to an exact version (neo4j-driver==5.28.1, alembic==1.16.1, psycopg2-binary==2.9.10, sqlalchemy==2.0.41, elasticsearch==8.9.0) but leave requests unpinned (>=2.31.0).
Mixing hard pins with loose ranges makes reproducible builds harder and may lead to dependency resolution conflicts or security regressions when requests updates downstream transitive packages without notice.
In addition, shipping psycopg2-binary is convenient for local use but discouraged for production as it bundles its own libpq and can lag behind security patches.
Consider:
-psycopg2-binary==2.9.10
+psycopg2==2.9.10 # compile-time build, preferred for productionand decide whether you want to:
- Hard-pin everything for perfect repeatability, or
- Use compatible-range specifiers consistently (e.g.,
sqlalchemy>=2.0,<2.1) and rely on a lock-file (pip-tools,poetry.lock, etc.) for determinism.
Failing to adopt a single strategy will bite you once CI and local environments drift.
🏁 Script executed:
#!/bin/bash
set -e
echo "Searching for lock files and other requirement files in the repo:"
find . -maxdepth 2 -type f \( -iname 'poetry.lock' -o -iname 'Pipfile.lock' -o -iname 'requirements*.txt' \)
echo
echo "Showing top lines of each found file:"
for f in $(find . -maxdepth 2 -type f \( -iname 'poetry.lock' -o -iname 'Pipfile.lock' -o -iname 'requirements*.txt' \)); do
echo "=== $f ==="
head -n 10 "$f" || true
echo
doneLength of output: 4460
Inconsistent version-pinning in weave/requirements.txt
Mixing exact pins (==5.28.1, ==1.16.1, etc.) with loose ranges (requests>=2.31.0) makes reproducible installs difficult and can introduce unexpected breakages as dependencies evolve. We also didn’t find any lock file (Pipfile.lock, poetry.lock or similar) in the repo to guarantee a deterministic install.
Please choose one of the following strategies and apply it consistently in weave/requirements.txt (and ideally across all service‐level requirements files):
- Exact-pin everything for perfect repeatability.
- Use compatible ranges (e.g.
sqlalchemy>=2.0,<3.0) plus introduce a lock file (via pip-tools, Poetry, pipenv, etc.) for determinism.
Additionally, for production builds it’s recommended to use the source package of psycopg2 rather than the binary bundle:
- psycopg2-binary==2.9.10
+ psycopg2==2.9.10 # compile-time build, preferred for production—
📝 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.
| # Database packages needed for weave db commands | |
| neo4j-driver==5.28.1 | |
| alembic==1.16.1 | |
| psycopg2-binary==2.9.10 | |
| sqlalchemy==2.0.41 | |
| # Additional packages for elasticsearch and other features | |
| elasticsearch==8.9.0 | |
| requests>=2.31.0 | |
| # Database packages needed for weave db commands | |
| neo4j-driver==5.28.1 | |
| alembic==1.16.1 | |
| psycopg2==2.9.10 # compile-time build, preferred for production | |
| sqlalchemy==2.0.41 | |
| # Additional packages for elasticsearch and other features | |
| elasticsearch==8.9.0 | |
| requests>=2.31.0 |
🤖 Prompt for AI Agents
In weave/requirements.txt lines 4 to 11, the version-pinning strategy is
inconsistent, mixing exact pins with a loose range for requests, which harms
reproducibility. Choose either to pin all dependencies exactly or use compatible
version ranges consistently for all packages, including requests. Additionally,
replace psycopg2-binary with psycopg2 for production use to avoid bundled libpq
issues. If using compatible ranges, introduce a lock file (e.g., pip-tools,
Poetry) to ensure deterministic installs.
| @click.option('--test-mode', is_flag=True, help='Enable test mode (simulates commands without executing)') | ||
| @click.pass_context | ||
| def cli(ctx, verbose, version): | ||
| def cli(ctx, verbose, version, test_mode): | ||
| """Weaver: A Rails-like framework for rapidly building and deploying enterprise-grade GenAI applications.""" | ||
| ctx.ensure_object(dict) | ||
| ctx.obj['VERBOSE'] = verbose | ||
| ctx.obj['TEST_MODE'] = test_mode or os.getenv('WEAVE_TEST_MODE', '').lower() in ['true', '1', 'yes'] | ||
|
|
||
| if ctx.obj['TEST_MODE']: | ||
| console.print("[yellow]🧪 TEST MODE ENABLED - Commands will be simulated[/yellow]") |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Nice addition, but propagate test-mode flag to sub-process helpers
ctx.obj['TEST_MODE'] is set here, yet helper functions (e.g., run_service_up_with_feedback, list_services, etc.) spawn subprocesses directly.
If those helpers do not consult ctx.obj themselves, test-mode will still execute real commands.
If you intend a true dry-run, pass the flag explicitly:
-run_service_up_with_feedback(command, project_name, verbose)
+run_service_up_with_feedback(
+ command, project_name, verbose, dry_run=ctx.obj['TEST_MODE']
+)(or read WEAVE_TEST_MODE inside the helpers).
Otherwise users might be lulled into a false sense of safety.
🏁 Script executed:
#!/bin/bash
# Locate definition of run_service_up_with_feedback
rg -n "def run_service_up_with_feedback" .
# Locate definition of list_services
rg -n "def list_services" .Length of output: 284
🏁 Script executed:
#!/bin/bash
# Show first 200 lines of docker_commands.py to inspect run_service_up_with_feedback
echo "=== docker_commands.py (lines 1-200) ==="
sed -n '1,200p' weave/bin/modules/docker_commands.py
# Look for subprocess usage in docker_commands.py
echo "=== subprocess calls in docker_commands.py ==="
grep -n "subprocess" weave/bin/modules/docker_commands.py || echo "None found"
# Show first 300 lines of services.py to inspect list_services
echo "=== services.py (lines 1-300) ==="
sed -n '1,300p' weave/bin/modules/services.py
# Look for subprocess usage in services.py
echo "=== subprocess calls in services.py ==="
grep -n "subprocess" weave/bin/modules/services.py || echo "None found"Length of output: 23158
Propagate test-mode flag to all subprocess helpers
Although ctx.obj['TEST_MODE'] is set in the CLI, none of the helpers that call subprocess.run (e.g. run_service_up_with_feedback, run_service_restart_with_feedback, run_command, list_services, etc.) actually receive or check this flag—so even in “test mode” they execute real commands.
Recommended changes:
- Update each helper signature to accept a
dry_run: bool = False(ortest_mode) parameter. - At the top of each helper, short-circuit in dry-run mode, e.g.:
def run_service_up_with_feedback(command, project_name, verbose=False, dry_run=False): if dry_run: console.print(f"[bold blue]Dry run:[/bold blue] {' '.join(command)}") return True …
- In
weave/bin/modules/cli.py, pass the flag fromctx.obj['TEST_MODE']when invoking helpers:- run_service_up_with_feedback(command, project_name, verbose) + run_service_up_with_feedback( + command, + project_name, + verbose, + dry_run=ctx.obj['TEST_MODE'] + ) - list_services(project_name, verbose, debug) + list_services( + project_name, + verbose=verbose, + debug=debug, + dry_run=ctx.obj['TEST_MODE'] + )
This ensures a true dry-run: all subprocess helpers will simulate instead of executing real commands when test mode is enabled.
🤖 Prompt for AI Agents
In weave/bin/modules/cli.py around lines 30 to 39, the test mode flag is set in
the CLI context but not propagated to subprocess helper functions that execute
commands. To fix this, update each helper function that runs subprocesses (e.g.,
run_service_up_with_feedback, list_services) to accept a dry_run or test_mode
boolean parameter. Inside these helpers, add a check to short-circuit and
simulate the command when dry_run is True by printing the command and returning
early. Then, modify the CLI code to pass ctx.obj['TEST_MODE'] as the dry_run
argument when calling these helpers, ensuring all subprocess calls respect the
test mode and do not execute real commands during dry-run.
| from .cli_migrate import check_and_install_tools | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Unused import triggers Ruff F401
check_and_install_tools is imported here but never referenced inside db_tools_status. Remove the import (or call it) to satisfy static analysis.
-from .cli_migrate import check_and_install_tools
+# (No import needed here)📝 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.
| from .cli_migrate import check_and_install_tools | |
🧰 Tools
🪛 Ruff (0.11.9)
65-65: .cli_migrate.check_and_install_tools imported but unused
Remove unused import: .cli_migrate.check_and_install_tools
(F401)
🤖 Prompt for AI Agents
In weave/bin/modules/cli_db_tools.py at lines 65 to 66, the import of
check_and_install_tools is unused and triggers a Ruff F401 warning. To fix this,
remove the import statement if the function is not needed, or alternatively,
call check_and_install_tools appropriately within the db_tools_status function
to justify the import.
| """Resolve target model from class or string reference.""" | ||
| if isinstance(target_model_ref, str): | ||
| # Try to resolve from the same module first | ||
| module = self.__class__.__module__ | ||
| if hasattr(__import__(module, fromlist=[target_model_ref]), target_model_ref): | ||
| return getattr(__import__(module, fromlist=[target_model_ref]), target_model_ref) | ||
|
|
||
| # Could add more sophisticated resolution logic here | ||
| return None |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Target-model resolution is brittle
Using __import__ with string names fails across packages and hides ImportErrors.
Leverage importlib.import_module and explicit error handling; alternatively accept the class object directly to avoid reflection altogether.
🤖 Prompt for AI Agents
In weave/bin/modules/annotations/graph.py around lines 155 to 163, the current
target model resolution uses __import__ with string names, which is brittle and
can hide ImportErrors. Replace __import__ calls with importlib.import_module for
clearer and more reliable imports, and add explicit error handling to catch and
handle ImportErrors properly. Alternatively, modify the method to accept the
class object directly instead of a string to avoid reflection and improve
robustness.
| context['slack'] = { | ||
| 'display_name': self._slack_user.display_name, | ||
| 'real_name': self._slack_user.real_name, | ||
| 'is_admin': self._slack_user.is_admin, | ||
| 'is_bot': self._slack_user.is_bot, | ||
| 'team_id': self._slack_user.team_id | ||
| } |
There was a problem hiding this comment.
Potential attribute error on SlackUser.display_name
SlackUser exposes display_name_or_name() (see domain/data/slack/user.py) but not display_name.
Accessing .display_name will raise AttributeError for many records.
- 'display_name': self._slack_user.display_name,
+ 'display_name': self._slack_user.display_name_or_name,Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In domain/user.py around lines 223 to 229, the code accesses
self._slack_user.display_name which does not exist and will raise
AttributeError. Replace the use of .display_name with the method
.display_name_or_name() to safely get the display name or fallback name. Update
the context dictionary to call self._slack_user.display_name_or_name() instead
of accessing the non-existent attribute.
| if 'insightmesh' in session_factories: | ||
| im_session = session_factories['insightmesh'] | ||
|
|
||
| # Find conversations for these participants | ||
| query = im_session.query(InsightMeshConversation).filter( | ||
| InsightMeshConversation.user_id.in_(participants) | ||
| ) | ||
|
|
||
| if start_date: | ||
| query = query.filter(InsightMeshConversation.created_at >= start_date) | ||
| if end_date: | ||
| query = query.filter(InsightMeshConversation.created_at <= end_date) | ||
| if topic: | ||
| query = query.filter(InsightMeshConversation.title.ilike(f'%{topic}%')) | ||
|
|
||
| im_conversations = query.all() | ||
|
|
||
| # Load messages from these conversations | ||
| for conv in im_conversations: | ||
| messages = im_session.query(InsightMeshMessage).filter_by( | ||
| conversation_id=conv.id | ||
| ).all() | ||
| self._insightmesh_messages.extend(messages) | ||
|
|
||
| self._loaded_sources.add('insightmesh') | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid an N+1 DB query when loading cross-platform messages
Inside _load_cross_platform_messages you loop over each matched conversation and execute a separate query to fetch its messages. With many conversations this quickly balloons into an N+1 pattern.
Batch-fetch all messages in a single query:
- im_conversations = query.all()
- # Load messages from these conversations
- for conv in im_conversations:
- messages = im_session.query(InsightMeshMessage).filter_by(
- conversation_id=conv.id
- ).all()
- self._insightmesh_messages.extend(messages)
+ im_conversations = query.all()
+ if im_conversations:
+ conv_ids = [c.id for c in im_conversations]
+ messages = (
+ im_session.query(InsightMeshMessage)
+ .filter(InsightMeshMessage.conversation_id.in_(conv_ids))
+ .all()
+ )
+ self._insightmesh_messages.extend(messages)This reduces round-trips and speeds up aggregation.
📝 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.
| if 'insightmesh' in session_factories: | |
| im_session = session_factories['insightmesh'] | |
| # Find conversations for these participants | |
| query = im_session.query(InsightMeshConversation).filter( | |
| InsightMeshConversation.user_id.in_(participants) | |
| ) | |
| if start_date: | |
| query = query.filter(InsightMeshConversation.created_at >= start_date) | |
| if end_date: | |
| query = query.filter(InsightMeshConversation.created_at <= end_date) | |
| if topic: | |
| query = query.filter(InsightMeshConversation.title.ilike(f'%{topic}%')) | |
| im_conversations = query.all() | |
| # Load messages from these conversations | |
| for conv in im_conversations: | |
| messages = im_session.query(InsightMeshMessage).filter_by( | |
| conversation_id=conv.id | |
| ).all() | |
| self._insightmesh_messages.extend(messages) | |
| self._loaded_sources.add('insightmesh') | |
| if 'insightmesh' in session_factories: | |
| im_session = session_factories['insightmesh'] | |
| # Find conversations for these participants | |
| query = im_session.query(InsightMeshConversation).filter( | |
| InsightMeshConversation.user_id.in_(participants) | |
| ) | |
| if start_date: | |
| query = query.filter(InsightMeshConversation.created_at >= start_date) | |
| if end_date: | |
| query = query.filter(InsightMeshConversation.created_at <= end_date) | |
| if topic: | |
| query = query.filter(InsightMeshConversation.title.ilike(f'%{topic}%')) | |
| im_conversations = query.all() | |
| if im_conversations: | |
| conv_ids = [c.id for c in im_conversations] | |
| messages = ( | |
| im_session.query(InsightMeshMessage) | |
| .filter(InsightMeshMessage.conversation_id.in_(conv_ids)) | |
| .all() | |
| ) | |
| self._insightmesh_messages.extend(messages) | |
| self._loaded_sources.add('insightmesh') |
🤖 Prompt for AI Agents
In domain/conversation.py between lines 198 and 223, the code fetches messages
for each conversation in a loop, causing an N+1 query problem. To fix this,
replace the per-conversation message queries with a single batch query that
fetches all messages for all matched conversations at once using a filter on
conversation_id with the list of conversation IDs. Then extend
self._insightmesh_messages with the results of this batch query, eliminating the
loop and reducing database round-trips.
| try: | ||
| from elasticsearch import Elasticsearch | ||
| import os | ||
|
|
||
| # Initialize Elasticsearch connection | ||
| es = Elasticsearch( | ||
| hosts=[{ | ||
| "scheme": "http", | ||
| "host": os.getenv("ELASTICSEARCH_HOST", "localhost"), | ||
| "port": int(os.getenv("ELASTICSEARCH_PORT", "9200")), | ||
| }] | ||
| ) | ||
| except ImportError: |
There was a problem hiding this comment.
🛠️ Refactor suggestion
De-fer Elasticsearch connection to first use
Creating a network connection at import time slows startup and complicates testing. Lazily initialise the client the first time it’s needed:
-try:
- from elasticsearch import Elasticsearch
- import os
- es = Elasticsearch(
- hosts=[{
- "scheme": "http",
- "host": os.getenv("ELASTICSEARCH_HOST", "localhost"),
- "port": int(os.getenv("ELASTICSEARCH_PORT", "9200")),
- }]
- )
-except ImportError:
- es = None
+import os
+es = None
+
+def _get_es():
+ global es
+ if es is None:
+ from elasticsearch import Elasticsearch # local import for optional dep
+ es = Elasticsearch(
+ hosts=[{
+ "scheme": "http",
+ "host": os.getenv("ELASTICSEARCH_HOST", "localhost"),
+ "port": int(os.getenv("ELASTICSEARCH_PORT", "9200")),
+ }]
+ )
+ return esUpdate call sites (_get_es()) accordingly.
📝 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.
| try: | |
| from elasticsearch import Elasticsearch | |
| import os | |
| # Initialize Elasticsearch connection | |
| es = Elasticsearch( | |
| hosts=[{ | |
| "scheme": "http", | |
| "host": os.getenv("ELASTICSEARCH_HOST", "localhost"), | |
| "port": int(os.getenv("ELASTICSEARCH_PORT", "9200")), | |
| }] | |
| ) | |
| except ImportError: | |
| import os | |
| es = None | |
| def _get_es(): | |
| global es | |
| if es is None: | |
| from elasticsearch import Elasticsearch # local import for optional dep | |
| es = Elasticsearch( | |
| hosts=[{ | |
| "scheme": "http", | |
| "host": os.getenv("ELASTICSEARCH_HOST", "localhost"), | |
| "port": int(os.getenv("ELASTICSEARCH_PORT", "9200")), | |
| }] | |
| ) | |
| return es |
🤖 Prompt for AI Agents
In domain/document.py around lines 14 to 26, the Elasticsearch client is being
initialized at import time, which slows startup and complicates testing.
Refactor the code to defer the Elasticsearch connection by wrapping the
initialization inside a function (e.g., _get_es()) that creates and returns the
client only when first called. Update all call sites to use this function
instead of the global client instance.
| def size_mb(self) -> Optional[float]: | ||
| """Get document size in MB.""" | ||
| if self.identity.size_bytes: | ||
| return round(self.identity.size_bytes / (1024 * 1024), 2) | ||
| return None |
There was a problem hiding this comment.
Robust size conversion
identity.size_bytes can arrive as a string from Elasticsearch. Guard against that and zero-division:
- if self.identity.size_bytes:
- return round(self.identity.size_bytes / (1024 * 1024), 2)
+ try:
+ size = int(self.identity.size_bytes)
+ except (TypeError, ValueError):
+ return None
+ return round(size / (1024 * 1024), 2) if size else None📝 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.
| def size_mb(self) -> Optional[float]: | |
| """Get document size in MB.""" | |
| if self.identity.size_bytes: | |
| return round(self.identity.size_bytes / (1024 * 1024), 2) | |
| return None | |
| def size_mb(self) -> Optional[float]: | |
| """Get document size in MB.""" | |
| try: | |
| size = int(self.identity.size_bytes) | |
| except (TypeError, ValueError): | |
| return None | |
| return round(size / (1024 * 1024), 2) if size else None | |
| return None |
🤖 Prompt for AI Agents
In domain/document.py around lines 478 to 482, the size_mb method assumes
identity.size_bytes is a number, but it can be a string from Elasticsearch and
may be zero or invalid. Update the method to safely convert size_bytes to a
float or int, handle conversion errors gracefully, and ensure division by zero
is avoided by checking the value before dividing. Return None if size_bytes is
missing, zero, or invalid.
1 participant
Summary by CodeRabbit
New Features
Enhancements
Bug Fixes
Refactor
Tests
Chores
.gitignoreand configuration placeholders.Removals