Merged
Conversation
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR introduces a comprehensive plugin system for BrainAPI with dynamic CLI management, prompt registry for customizable agent prompts, a runtime agent factory for flexible agent creation, centralized adapter initialization with mode awareness, and Docker support for plugin loading and installation. Services are updated to load plugins at startup. Changes
Sequence Diagram(s)sequenceDiagram
participant API as API App<br/>(Startup)
participant PL as PluginLoader
participant FM as Filesystem
participant PM as PluginManager
participant PI as Plugin Init<br/>(register fn)
participant PC as PluginContext
API->>PL: discover()
PL->>FM: scan plugins_dir<br/>for manifests
FM-->>PL: list[PluginManifest]
API->>PL: load_all()
loop for each manifest
PL->>PM: check if already<br/>loaded locally
PM->>FM: stat plugins/{name}
alt not installed
PL->>PM: install(name, version)
PM->>FM: download from<br/>registry
FM-->>PM: extracted plugin
end
PL->>FM: install dependencies
PL->>FM: import plugin module
FM-->>PI: module loaded
PI->>PC: register(context)
PC->>PC: include_router() or<br/>register_mcp_tool()
PI-->>PL: success
end
PL-->>API: dict[name→bool]
API->>API: _log_plugin_banner()<br/>(visual report)
sequenceDiagram
participant Agent as Agent File<br/>(architect/kg/scout)
participant PR as PromptRegistry
participant RAF as RuntimeAgentFactory
participant C as Custom<br/>AgentBase
participant LC as LangChain<br/>create_agent
Agent->>PR: get(SYSTEM_PROMPT_KEY,<br/>fallback_const)
PR-->>Agent: prompt_str or<br/>fallback
Agent->>Agent: format(prompt_str)
Agent->>RAF: build(model, tools,<br/>system_prompt,<br/>architecture)
alt use_custom_backend or<br/>arch=="custom"
RAF->>C: __init__(model, tools,<br/>system_prompt,<br/>output_schema)
C-->>RAF: agent instance
else arch=="langchain"
RAF->>LC: create_agent(model,<br/>tools, system_prompt,<br/>response_format)
LC-->>RAF: agent instance
end
RAF-->>Agent: agent instance
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 18
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/core/agents/architect_agent.py (1)
226-247:⚠️ Potential issue | 🔴 CriticalRemove
.format(extra_system_prompt=...)calls—these prompts don't contain the{extra_system_prompt}placeholder.The three prompt templates (ARCHITECT_AGENT_SYSTEM_PROMPT, ARCHITECT_AGENT_TOOLER_SYSTEM_PROMPT, and ARCHITECT_AGENT_TOOLER_COARSE_SYSTEM_PROMPT) contain only
{entities},{previously_created_relationships},{targeting}, and{text}placeholders. Calling.format(extra_system_prompt=...)will raiseKeyErrorat runtime. Either add the placeholder to the prompts or remove the format call.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/agents/architect_agent.py` around lines 226 - 247, The code calls .format(extra_system_prompt=...) on prompts that don't include that placeholder, causing KeyError at runtime; update the three assignments that build system_prompt (the uses of prompt_registry.get for ARCHITECT_AGENT_SYSTEM_PROMPT, ARCHITECT_AGENT_TOOLER_SYSTEM_PROMPT, and ARCHITECT_AGENT_TOOLER_COARSE_SYSTEM_PROMPT) to remove the .format(...) call and simply use the returned template as-is, or alternatively add the {extra_system_prompt} placeholder into those prompt templates if the variable must be injected; reference the system_prompt variable and the extra_system_prompt local to locate the exact spots to change.
🧹 Nitpick comments (5)
src/services/api/middlewares/brains.py (1)
46-47: Consider usingClassVarto clarify shared mutable state.Same pattern as
BrainPATMiddleware- the mutableset()is intentionally shared for plugin registration. AddingClassVarimproves clarity and silences Ruff RUF012.♻️ Proposed fix
+from typing import ClassVar + class BrainMiddleware(BaseHTTPMiddleware): - excluded_prefixes: set[str] = set() + excluded_prefixes: ClassVar[set[str]] = set()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/api/middlewares/brains.py` around lines 46 - 47, The mutable class-level set excluded_prefixes should be annotated as a ClassVar to indicate intentional shared state and silence RUF012; update the class where excluded_prefixes is declared (the excluded_prefixes symbol in src/services/api/middlewares/brains.py) to use typing.ClassVar (i.e., ClassVar[set[str]]) and add the necessary import for ClassVar from typing so the intent is explicit and linters stop flagging it.src/services/api/middlewares/auth.py (1)
22-23: Consider usingClassVarto clarify shared mutable state.The mutable
set()default is intentional here since plugins modify this class-level attribute at runtime (as seen insrc/core/plugins/context.py). However, usingClassVarmakes this explicit and silences the Ruff RUF012 warning.♻️ Proposed fix
+from typing import ClassVar + class BrainPATMiddleware(BaseHTTPMiddleware): - excluded_prefixes: set[str] = set() + excluded_prefixes: ClassVar[set[str]] = set()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/api/middlewares/auth.py` around lines 22 - 23, The class-level mutable attribute excluded_prefixes should be annotated as a ClassVar to indicate shared mutable state; update the declaration of excluded_prefixes in the auth middleware (variable name: excluded_prefixes) to use typing.ClassVar[set[str]] and import ClassVar from typing so plugins that mutate this class attribute still work and the RUF012 warning is silenced.src/core/instances.py (2)
23-37: LLM adapter initialization is duplicated between branches.Lines 26-29 and 34-37 are identical. Only the import differs.
♻️ Proposed simplification
if _is_local: from src.lib.llm.client_ollama import _llm_large_client, _llm_small_client - - _llm_small = LLMAdapter() - _llm_small.add_client(_llm_small_client) - _llm_large = LLMAdapter() - _llm_large.add_client(_llm_large_client) else: from src.lib.llm.client_large import _llm_large_client from src.lib.llm.client_small import _llm_small_client - _llm_small = LLMAdapter() - _llm_small.add_client(_llm_small_client) - _llm_large = LLMAdapter() - _llm_large.add_client(_llm_large_client) +_llm_small = LLMAdapter() +_llm_small.add_client(_llm_small_client) +_llm_large = LLMAdapter() +_llm_large.add_client(_llm_large_client)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/instances.py` around lines 23 - 37, The initialization of LLMAdapter instances is duplicated; keep the conditional only for importing clients and move the shared setup out of the branches: inside the _is_local conditional import the appropriate symbols (_llm_large_client, _llm_small_client) and then after the if/else create _llm_small = LLMAdapter(); call _llm_small.add_client(_llm_small_client); create _llm_large = LLMAdapter(); call _llm_large.add_client(_llm_large_client); this preserves the differing imports while eliminating the duplicated initialization code involving LLMAdapter, _llm_small, and _llm_large.
12-18: Duplicated_MODESconstant creates maintenance risk.The
_MODEStuple is also defined insrc/config.py(line 268). Remove the duplicate definition here and import it from config.py instead.♻️ Proposed fix
from src.adapters.cache import CacheAdapter from src.adapters.data import DataAdapter from src.adapters.embeddings import EmbeddingsAdapter, VectorStoreAdapter from src.adapters.graph import GraphAdapter from src.adapters.llm import LLMAdapter -from src.config import config +from src.config import config, _MODES from src.lib.milvus.client import _milvus_client from src.lib.mongo.client import _mongo_client from src.lib.neo4j.client import _neo4j_client from src.lib.redis.client import _redis_client -_MODES = ("local", "remote") - def _require_mode():🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/instances.py` around lines 12 - 18, Remove the duplicate _MODES tuple from this file and import the canonical _MODES from the config module instead; update the top of the file to import _MODES from config and leave _require_mode() using config.models_mode and _MODES as before so the function still raises ValueError for invalid modes and returns config.models_mode == "local".src/core/plugins/context.py (1)
103-122: Consider exposingappvia a property for consistency.The
_appattribute is private, butmcpis public. Since both serve similar purposes (the underlying runtime), consider making the access pattern consistent. Also, the middleware class variables are mutated globally—this is by design per the relevant snippets, but worth noting it's a side effect that persists across contexts.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/plugins/context.py` around lines 103 - 122, The method include_router mutates global middleware excluded_prefixes and accesses the private _app directly; expose the underlying FastAPI app via a public property (e.g., add an app or fastapi_app getter) and update include_router to use that public property instead of _app, and keep the current logic that app.include_router(router, **kwargs) and _routers append unchanged; also mention the mutation of BrainPATMiddleware.excluded_prefixes and BrainMiddleware.excluded_prefixes is intentional but ensure callers know these are global side effects by documenting or naming the property consistently with the public mcp attribute (reference include_router, _app, mcp, BrainPATMiddleware, BrainMiddleware).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.env.example:
- Around line 117-121: Add a new BRAINAPI_PLUGINS entry to the Plugins section
so the environment sample matches example-docker-compose.yaml: insert a line
like BRAINAPI_PLUGINS="enabled" (or a comma-separated list/path) directly with
the existing PLUGINS_DIR, PLUGIN_REGISTRY_URL, PLUGIN_PUBLISHER_API_KEY and
PLUGIN_PUBLISHER_ID entries, and include a brief comment describing its purpose
(e.g., "controls which plugins are loaded by the worker/MCP") and a sensible
default placeholder so users can discover and configure the new plugin flow.
In `@example-docker-compose.yaml`:
- Line 195: Update the example compose image tags that still point to
"ghcr.io/lumen-labs/brainapi:v2.9.7-dev" so they use
"ghcr.io/lumen-labs/brainapi:v2.10.0-dev" instead; locate each occurrence of the
exact string "ghcr.io/lumen-labs/brainapi:v2.9.7-dev" (mentioned at the three
positions flagged) and replace it with "ghcr.io/lumen-labs/brainapi:v2.10.0-dev"
so the example services pull the new dev image containing the PR changes.
- Around line 66-67: Replace hard-coded credentials with environment variable
references: change the NEO4J_AUTH and NEO4J_ACCEPT_LICENSE_AGREEMENT entries to
use docker-compose variable substitution (e.g., NEO4J_AUTH=${NEO4J_AUTH})
instead of the literal "neo4j/your_password", and similarly replace any other
hard-coded credentials like "root/password" with corresponding env vars (e.g.,
${MYSQL_ROOT_PASSWORD} or the appropriate variable name used in your compose).
Ensure these variables are defined in your .env file and documented so
credentials are configurable and not embedded in example-docker-compose.yaml.
- Around line 10-14: The example-docker-compose.yaml currently bakes
machine-specific host bind mounts in the volumes block (the absolute host bind
entries under the nginx service), which makes the example non-reproducible;
change those absolute host mounts to repo-relative defaults or named Docker
volumes (e.g., use ./examples/nginx/conf, ./examples/letsencrypt or a named
volume like nginx_conf) so a fresh checkout can docker compose up without
requiring host files, and move the real absolute host binds into an opt-in
production profile (or document how to override) so the example remains
runnable; apply the same change pattern to the other similar volumes blocks
mentioned in the comment.
In `@src/core/agents/scout_agent.py`:
- Around line 135-145: The code calls .format(extra_system_prompt=...) on
defaults SCOUT_AGENT_SYSTEM_PROMPT and SCOUT_AGENT_COARSE_SYSTEM_PROMPT which
may lack an {extra_system_prompt} placeholder and raise KeyError; fix by either
adding the {extra_system_prompt} placeholder to both default templates or change
the formatting call around prompt_registry.get(...) (where system_prompt is
assigned) to safe formatting (e.g., use a format_map with a dict subclass that
returns '' for missing keys, or guard with an if/try/except to only format when
extra_system_prompt is present) so system_prompt and the branch when mode ==
"coarse" both handle missing placeholders without exception.
In `@src/core/instances.py`:
- Line 21: The module currently calls _require_mode() at import time and assigns
its result to _is_local, which raises if MODELS_MODE is unset; change this to
defer initialization by removing the immediate _is_local = _require_mode() call
and instead implement a lazy accessor (e.g., get_is_local() or is_local()) that
calls _require_mode() on first call, caches the result in a module-level
_is_local cache, and returns it thereafter so tests that import the module
without MODELS_MODE set do not fail at import time.
In `@src/core/plugins/cli.py`:
- Line 148: In the CLI publisher registration output (the call to typer.echo
that prints success), remove the stray f-string prefix so the call is a normal
string literal; locate the typer.echo invocation (e.g., in the publisher
registration function or command handler) and change typer.echo(f" Publisher
registered successfully!") to use a plain string (" Publisher registered
successfully!") to satisfy Ruff.
- Around line 9-12: PLUGINS_DIR currently defaults to the CWD ("plugins"),
causing CLI to install plugins outside the repo root; change PLUGINS_DIR to
point to the repository's plugins directory (same default the services use) by
deriving the project root from this module's path (e.g.,
Path(__file__).resolve().parents[...] ) and joining "plugins", instead of using
Path(os.getenv("PLUGINS_DIR", "plugins")); keep honoring PLUGINS_DIR env when
set but default to the repo-root/plugins location so CLI and services use the
same directory.
In `@src/core/plugins/loader.py`:
- Around line 54-67: The current install_dependencies method runs pip installs
during runtime causing unsafe installs at load_all time; modify the flow so
install_dependencies (in class containing install_dependencies and method
_build_install_cmd) is no longer invoked during load_all and instead becomes a
no-op or raises if called from startup, and move actual installation logic into
a separate install/update routine invoked only by the plugin install/update
workflow (e.g., a new install_plugin_environment or PluginInstaller class used
by the plugin management API), ensuring PluginManifest.pip_dependencies are
handled in an isolated environment (virtualenv/container) rather than the
service process; update load_all to skip dependency installation and document
that installation happens only via the new installer.
In `@src/core/plugins/manager.py`:
- Around line 40-70: The install method currently swallows all exceptions from
parse_manifest when checking an existing manifest (the bare except around
parse_manifest(existing_manifest)), which can hide parsing/manifest corruption
errors; change the bare except to catch Exception as e and log the exception at
debug (e.g., using logger.debug("Failed to parse existing manifest %s: %s",
existing_manifest, e)) before continuing so failures are visible while
preserving the existing behavior, referencing the install method,
existing_manifest, parse_manifest, and logger to locate the code to modify.
- Around line 108-109: The update(name) method currently calls install(name,
version="latest") but install(short-circuits when version == "latest" if a
manifest exists, so update may not fetch the registry's latest; change install
to accept a force: bool = False parameter (or equivalent) that bypasses the
early return when set, and then call install(name, version="latest", force=True)
from update; alternatively, make update explicitly query the registry for latest
version and compare it to the installed manifest version (using the same
registry lookup used in install) and only skip installation when versions match
— update the install function signature and the early-return logic (or add the
registry compare in update) accordingly so update actually fetches and applies
the latest release when available.
In `@src/core/plugins/manifest.py`:
- Around line 53-63: The manifest loader currently coerces values (e.g., using
str(...) and list(...)) which masks bad data; update the code that builds the
PluginManifest so it validates types before coercion: ensure required fields
name, version, and entry_point exist and are actual strings (raise ValueError if
missing or not str), check optional description, author, and brainapi_version
are either missing or strings, and validate that pip_dependencies and tags are
iterable sequences of strings (reject non-sequence values and elements that are
not strings) before converting them; keep constructing PluginManifest with
validated/converted values (reference the PluginManifest construction block and
the variable manifest_path shown in the diff).
In `@src/core/plugins/prompts.py`:
- Around line 31-41: The get() method in prompts.py returns immediately when a
prompt_name exists in self._overrides, which drops any registered extensions;
change get (function get) to compose the final prompt by starting from the
override (self._overrides[prompt_name]) when present or the default/_defaults
value otherwise, then append any entries from self._extensions.get(prompt_name,
[]) (joined with newlines) so overrides and extensions are layered together;
ensure the default parameter logic (default vs self._defaults) still applies
when no override exists and preserve the existing newline joining behavior.
- Around line 5-13: The PromptRegistry singleton (class PromptRegistry) only
stores overrides in-process, so worker processes that import prompt_registry
(used in src/workers/tasks/ingestion.py) never see API/MCP-loaded plugin
overrides because PluginLoader is only run in the API and MCP services; fix this
by ensuring workers load plugin data on startup: add a public method on
PromptRegistry (e.g., load_from_plugins or apply_plugin_overrides) that accepts
plugin metadata, or a load_plugins() helper that invokes PluginLoader, then call
that from the worker bootstrap path so every process populates its
PromptRegistry; alternatively persist plugin overrides to a shared store and add
a PromptRegistry method to initialize from that store and call it in worker
startup. Ensure references to PromptRegistry, prompt_registry import,
PluginLoader, and the worker bootstrap/ingestion task are used to locate where
to add the call.
In `@src/core/plugins/registry.py`:
- Around line 84-124: The download method is vulnerable to tar path traversal
when calling tar.extractall; add a helper function _safe_extract(tar:
tarfile.TarFile, path: str) that iterates tar.getmembers(), computes member_path
= os.path.join(path, member.name), and verifies
os.path.abspath(member_path).startswith(os.path.abspath(path) + os.sep) (or
equals the base for root entries), raising ValueError on violation, then call
tar.extractall(path) only after all members are validated; replace the
tar.extractall(extract_dir) call in download with _safe_extract(tar,
extract_dir) to mitigate CVE-2007-4559.
In `@src/services/api/app.py`:
- Around line 47-50: When iterating ctx._event_handlers for event_name ==
"shutdown", wrap each handler invocation (the call to handler() or await
handler() using _is_coroutine) in a try/except so one handler's exception
doesn't abort the rest; on exception log the error for that specific handler
(e.g., logging.exception or the module's existing logger) and continue to the
next handler, and if you still want to surface failure after running all
handlers collect exceptions into a list and raise/return an aggregated error
once the loop completes.
In `@src/services/api/middlewares/auth.py`:
- Line 42: Remove the debug print that leaks secrets: delete the print(brainpat,
system_pat) call in the auth middleware and do not log raw tokens or secrets
(brainpat, system_pat). If you need visibility for debugging, replace it with a
non-sensitive check or a boolean/status log (e.g., "brainpat present" or
"system_pat from env: yes/no") or use masked/redacted values, and ensure this
change is applied inside the authentication middleware function handling
brainpat/system_pat to avoid any secret exposure in stdout/container logs.
In `@src/services/mcp/app.py`:
- Around line 21-29: The _load_mcp_plugins() function creates a PluginContext
and PluginLoader then discards them, so plugin lifecycle handlers registered via
add_event_handler() never persist; modify _load_mcp_plugins (and the equivalent
at line ~77) to store the created PluginContext and PluginLoader on a long-lived
object (e.g., module-level variables or attached to the MCP app/state) instead
of letting them go out of scope, call loader.load_all() as before, call
_log_plugin_banner(loader, results), and ensure the stored context/loader remain
reachable until service shutdown so registered startup/shutdown handlers can
run.
---
Outside diff comments:
In `@src/core/agents/architect_agent.py`:
- Around line 226-247: The code calls .format(extra_system_prompt=...) on
prompts that don't include that placeholder, causing KeyError at runtime; update
the three assignments that build system_prompt (the uses of prompt_registry.get
for ARCHITECT_AGENT_SYSTEM_PROMPT, ARCHITECT_AGENT_TOOLER_SYSTEM_PROMPT, and
ARCHITECT_AGENT_TOOLER_COARSE_SYSTEM_PROMPT) to remove the .format(...) call and
simply use the returned template as-is, or alternatively add the
{extra_system_prompt} placeholder into those prompt templates if the variable
must be injected; reference the system_prompt variable and the
extra_system_prompt local to locate the exact spots to change.
---
Nitpick comments:
In `@src/core/instances.py`:
- Around line 23-37: The initialization of LLMAdapter instances is duplicated;
keep the conditional only for importing clients and move the shared setup out of
the branches: inside the _is_local conditional import the appropriate symbols
(_llm_large_client, _llm_small_client) and then after the if/else create
_llm_small = LLMAdapter(); call _llm_small.add_client(_llm_small_client); create
_llm_large = LLMAdapter(); call _llm_large.add_client(_llm_large_client); this
preserves the differing imports while eliminating the duplicated initialization
code involving LLMAdapter, _llm_small, and _llm_large.
- Around line 12-18: Remove the duplicate _MODES tuple from this file and import
the canonical _MODES from the config module instead; update the top of the file
to import _MODES from config and leave _require_mode() using config.models_mode
and _MODES as before so the function still raises ValueError for invalid modes
and returns config.models_mode == "local".
In `@src/core/plugins/context.py`:
- Around line 103-122: The method include_router mutates global middleware
excluded_prefixes and accesses the private _app directly; expose the underlying
FastAPI app via a public property (e.g., add an app or fastapi_app getter) and
update include_router to use that public property instead of _app, and keep the
current logic that app.include_router(router, **kwargs) and _routers append
unchanged; also mention the mutation of BrainPATMiddleware.excluded_prefixes and
BrainMiddleware.excluded_prefixes is intentional but ensure callers know these
are global side effects by documenting or naming the property consistently with
the public mcp attribute (reference include_router, _app, mcp,
BrainPATMiddleware, BrainMiddleware).
In `@src/services/api/middlewares/auth.py`:
- Around line 22-23: The class-level mutable attribute excluded_prefixes should
be annotated as a ClassVar to indicate shared mutable state; update the
declaration of excluded_prefixes in the auth middleware (variable name:
excluded_prefixes) to use typing.ClassVar[set[str]] and import ClassVar from
typing so plugins that mutate this class attribute still work and the RUF012
warning is silenced.
In `@src/services/api/middlewares/brains.py`:
- Around line 46-47: The mutable class-level set excluded_prefixes should be
annotated as a ClassVar to indicate intentional shared state and silence RUF012;
update the class where excluded_prefixes is declared (the excluded_prefixes
symbol in src/services/api/middlewares/brains.py) to use typing.ClassVar (i.e.,
ClassVar[set[str]]) and add the necessary import for ClassVar from typing so the
intent is explicit and linters stop flagging it.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d609bbcc-75d0-4dc6-93f6-173c48f213e1
📒 Files selected for processing (30)
.env.exampleDockerfileREADME.mdbin/brainapientrypoint.shexample-docker-compose.yamlpyproject.tomlsrc/core/agents/architect_agent.pysrc/core/agents/janitor_agent.pysrc/core/agents/kg_agent.pysrc/core/agents/observations_agent.pysrc/core/agents/scout_agent.pysrc/core/instances.pysrc/core/layers/graph_consolidation/graph_consolidation.pysrc/core/plugins/__init__.pysrc/core/plugins/cli.pysrc/core/plugins/context.pysrc/core/plugins/loader.pysrc/core/plugins/manager.pysrc/core/plugins/manifest.pysrc/core/plugins/prompts.pysrc/core/plugins/registry.pysrc/services/api/app.pysrc/services/api/middlewares/auth.pysrc/services/api/middlewares/brains.pysrc/services/data/main.pysrc/services/input/agents.pysrc/services/kg_agent/main.pysrc/services/mcp/app.pysrc/workers/tasks/ingestion.py
💤 Files with no reviewable changes (1)
- src/core/layers/graph_consolidation/graph_consolidation.py
Comment on lines
+117
to
+121
| # Plugins | ||
| PLUGINS_DIR="plugins" | ||
| PLUGIN_REGISTRY_URL="https://registry.brain-api.dev" | ||
| PLUGIN_PUBLISHER_API_KEY="your-publisher-api-key" | ||
| PLUGIN_PUBLISHER_ID="your-publisher-id" No newline at end of file |
Contributor
There was a problem hiding this comment.
Document BRAINAPI_PLUGINS in the sample env file too.
example-docker-compose.yaml now reads BRAINAPI_PLUGINS for the worker and MCP services, but this example block doesn’t expose that knob yet. Adding it here makes the new plugin flow discoverable.
📝 Proposed fix
# Plugins
+BRAINAPI_PLUGINS=""
PLUGINS_DIR="plugins"
PLUGIN_REGISTRY_URL="https://registry.brain-api.dev"
PLUGIN_PUBLISHER_API_KEY="your-publisher-api-key"
PLUGIN_PUBLISHER_ID="your-publisher-id"🧰 Tools
🪛 dotenv-linter (4.0.0)
[warning] 118-118: [QuoteCharacter] The value has quote characters (', ")
(QuoteCharacter)
[warning] 119-119: [QuoteCharacter] The value has quote characters (', ")
(QuoteCharacter)
[warning] 120-120: [QuoteCharacter] The value has quote characters (', ")
(QuoteCharacter)
[warning] 120-120: [UnorderedKey] The PLUGIN_PUBLISHER_API_KEY key should go before the PLUGIN_REGISTRY_URL key
(UnorderedKey)
[warning] 121-121: [EndingBlankLine] No blank line at the end of the file
(EndingBlankLine)
[warning] 121-121: [QuoteCharacter] The value has quote characters (', ")
(QuoteCharacter)
[warning] 121-121: [UnorderedKey] The PLUGIN_PUBLISHER_ID key should go before the PLUGIN_REGISTRY_URL key
(UnorderedKey)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.env.example around lines 117 - 121, Add a new BRAINAPI_PLUGINS entry to the
Plugins section so the environment sample matches example-docker-compose.yaml:
insert a line like BRAINAPI_PLUGINS="enabled" (or a comma-separated list/path)
directly with the existing PLUGINS_DIR, PLUGIN_REGISTRY_URL,
PLUGIN_PUBLISHER_API_KEY and PLUGIN_PUBLISHER_ID entries, and include a brief
comment describing its purpose (e.g., "controls which plugins are loaded by the
worker/MCP") and a sensible default placeholder so users can discover and
configure the new plugin flow.
Comment on lines
+10
to
+14
| volumes: | ||
| - /srv/nginx/conf/nginx.conf:/etc/nginx/nginx.conf:ro | ||
| - /srv/nginx/conf/conf.d:/etc/nginx/conf.d:ro | ||
| - /etc/letsencrypt:/etc/letsencrypt:ro | ||
| - /srv/nginx/logs:/var/log/nginx |
Contributor
There was a problem hiding this comment.
Don’t bake machine-specific host paths into the example stack.
This file is presented as a runnable example, but these mounts now require /srv/nginx/..., /etc/letsencrypt, /root/.env, and /root/gcp_credentials.json to exist on the host. On a fresh checkout, docker compose -f example-docker-compose.yaml up will fail before the stack even starts. Repo-relative defaults or an opt-in production profile would keep the example reproducible.
Also applies to: 199-208, 260-269, 306-315
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@example-docker-compose.yaml` around lines 10 - 14, The
example-docker-compose.yaml currently bakes machine-specific host bind mounts in
the volumes block (the absolute host bind entries under the nginx service),
which makes the example non-reproducible; change those absolute host mounts to
repo-relative defaults or named Docker volumes (e.g., use ./examples/nginx/conf,
./examples/letsencrypt or a named volume like nginx_conf) so a fresh checkout
can docker compose up without requiring host files, and move the real absolute
host binds into an opt-in production profile (or document how to override) so
the example remains runnable; apply the same change pattern to the other similar
volumes blocks mentioned in the comment.
Comment on lines
+66
to
67
| - NEO4J_AUTH=neo4j/your_password | ||
| - NEO4J_ACCEPT_LICENSE_AGREEMENT=yes |
Contributor
There was a problem hiding this comment.
Keep the database credentials configurable.
Hard-coding neo4j/your_password and root/password in a compose file that also publishes the database ports is an avoidable security regression. Pull these values from .env instead.
🔐 Proposed fix
- - NEO4J_AUTH=neo4j/your_password
+ - NEO4J_AUTH=${NEO4J_USERNAME:-neo4j}/${NEO4J_PASSWORD}
...
- MONGO_INITDB_ROOT_USERNAME: root
- MONGO_INITDB_ROOT_PASSWORD: password
+ MONGO_INITDB_ROOT_USERNAME: ${MONGO_USERNAME:-root}
+ MONGO_INITDB_ROOT_PASSWORD: ${MONGO_PASSWORD}Also applies to: 170-171
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@example-docker-compose.yaml` around lines 66 - 67, Replace hard-coded
credentials with environment variable references: change the NEO4J_AUTH and
NEO4J_ACCEPT_LICENSE_AGREEMENT entries to use docker-compose variable
substitution (e.g., NEO4J_AUTH=${NEO4J_AUTH}) instead of the literal
"neo4j/your_password", and similarly replace any other hard-coded credentials
like "root/password" with corresponding env vars (e.g., ${MYSQL_ROOT_PASSWORD}
or the appropriate variable name used in your compose). Ensure these variables
are defined in your .env file and documented so credentials are configurable and
not embedded in example-docker-compose.yaml.
|
|
||
| brainapi: | ||
| image: ghcr.io/lumen-labs/brainapi:latest | ||
| image: ghcr.io/lumen-labs/brainapi:v2.9.7-dev |
Contributor
There was a problem hiding this comment.
Point the example services at the 2.10.0-dev image.
These services still pull ghcr.io/lumen-labs/brainapi:v2.9.7-dev, so the example deployment won’t actually contain the plugin/runtime changes introduced in this PR.
🔧 Proposed fix
- image: ghcr.io/lumen-labs/brainapi:v2.9.7-dev
+ image: ghcr.io/lumen-labs/brainapi:v2.10.0-dev
...
- image: ghcr.io/lumen-labs/brainapi:v2.9.7-dev
+ image: ghcr.io/lumen-labs/brainapi:v2.10.0-dev
...
- image: ghcr.io/lumen-labs/brainapi:v2.9.7-dev
+ image: ghcr.io/lumen-labs/brainapi:v2.10.0-devAlso applies to: 258-258, 302-302
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@example-docker-compose.yaml` at line 195, Update the example compose image
tags that still point to "ghcr.io/lumen-labs/brainapi:v2.9.7-dev" so they use
"ghcr.io/lumen-labs/brainapi:v2.10.0-dev" instead; locate each occurrence of the
exact string "ghcr.io/lumen-labs/brainapi:v2.9.7-dev" (mentioned at the three
positions flagged) and replace it with "ghcr.io/lumen-labs/brainapi:v2.10.0-dev"
so the example services pull the new dev image containing the PR changes.
Comment on lines
+135
to
145
| system_prompt = prompt_registry.get( | ||
| "SCOUT_AGENT_SYSTEM_PROMPT", SCOUT_AGENT_SYSTEM_PROMPT | ||
| ).format( | ||
| extra_system_prompt=extra_system_prompt if extra_system_prompt else "" | ||
| ) | ||
| if mode == "coarse": | ||
| system_prompt = SCOUT_AGENT_COARSE_SYSTEM_PROMPT.format( | ||
| system_prompt = prompt_registry.get( | ||
| "SCOUT_AGENT_COARSE_SYSTEM_PROMPT", SCOUT_AGENT_COARSE_SYSTEM_PROMPT | ||
| ).format( | ||
| extra_system_prompt=extra_system_prompt if extra_system_prompt else "" | ||
| ) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the prompt templates to confirm they lack the {extra_system_prompt} placeholder
echo "Checking SCOUT_AGENT_SYSTEM_PROMPT for {extra_system_prompt}:"
rg -n "extra_system_prompt" --type py -g '**/prompts/scout_agent.py'
echo ""
echo "Full prompt definitions:"
ast-grep --pattern 'SCOUT_AGENT_SYSTEM_PROMPT = """$$$"""'Repository: Lumen-Labs/brainapi2
Length of output: 7970
🏁 Script executed:
# Check for SCOUT_AGENT_COARSE_SYSTEM_PROMPT definition and content
cat -n src/constants/prompts/scout_agent.py | sed -n '80,145p'Repository: Lumen-Labs/brainapi2
Length of output: 4584
🏁 Script executed:
# Verify the actual code in scout_agent.py at lines 135-145
cat -n src/core/agents/scout_agent.py | sed -n '130,150p'Repository: Lumen-Labs/brainapi2
Length of output: 1124
Runtime KeyError: {extra_system_prompt} placeholder missing from default prompts.
The default SCOUT_AGENT_SYSTEM_PROMPT and SCOUT_AGENT_COARSE_SYSTEM_PROMPT templates do not contain an {extra_system_prompt} placeholder. Calling .format(extra_system_prompt=...) on these strings will raise a KeyError at runtime.
Either:
- Add the
{extra_system_prompt}placeholder to the prompt templates, or - Use safe formatting that checks for the placeholder before substitution
Proposed fix using safe string formatting
if mode == "granular":
- system_prompt = prompt_registry.get(
- "SCOUT_AGENT_SYSTEM_PROMPT", SCOUT_AGENT_SYSTEM_PROMPT
- ).format(
- extra_system_prompt=extra_system_prompt if extra_system_prompt else ""
- )
+ base_prompt = prompt_registry.get(
+ "SCOUT_AGENT_SYSTEM_PROMPT", SCOUT_AGENT_SYSTEM_PROMPT
+ )
+ if "{extra_system_prompt}" in base_prompt:
+ system_prompt = base_prompt.format(
+ extra_system_prompt=extra_system_prompt if extra_system_prompt else ""
+ )
+ else:
+ system_prompt = base_prompt + ("\n" + extra_system_prompt if extra_system_prompt else "")
if mode == "coarse":
- system_prompt = prompt_registry.get(
- "SCOUT_AGENT_COARSE_SYSTEM_PROMPT", SCOUT_AGENT_COARSE_SYSTEM_PROMPT
- ).format(
- extra_system_prompt=extra_system_prompt if extra_system_prompt else ""
- )
+ base_prompt = prompt_registry.get(
+ "SCOUT_AGENT_COARSE_SYSTEM_PROMPT", SCOUT_AGENT_COARSE_SYSTEM_PROMPT
+ )
+ if "{extra_system_prompt}" in base_prompt:
+ system_prompt = base_prompt.format(
+ extra_system_prompt=extra_system_prompt if extra_system_prompt else ""
+ )
+ else:
+ system_prompt = base_prompt + ("\n" + extra_system_prompt if extra_system_prompt else "")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/core/agents/scout_agent.py` around lines 135 - 145, The code calls
.format(extra_system_prompt=...) on defaults SCOUT_AGENT_SYSTEM_PROMPT and
SCOUT_AGENT_COARSE_SYSTEM_PROMPT which may lack an {extra_system_prompt}
placeholder and raise KeyError; fix by either adding the {extra_system_prompt}
placeholder to both default templates or change the formatting call around
prompt_registry.get(...) (where system_prompt is assigned) to safe formatting
(e.g., use a format_map with a dict subclass that returns '' for missing keys,
or guard with an if/try/except to only format when extra_system_prompt is
present) so system_prompt and the branch when mode == "coarse" both handle
missing placeholders without exception.
Comment on lines
+31
to
+41
| def get(self, prompt_name: str, default: Optional[str] = None) -> str: | ||
| if prompt_name in self._overrides: | ||
| return self._overrides[prompt_name] | ||
|
|
||
| base = default if default is not None else self._defaults.get(prompt_name, "") | ||
|
|
||
| extensions = self._extensions.get(prompt_name, []) | ||
| if extensions: | ||
| return base + "\n" + "\n".join(extensions) | ||
|
|
||
| return base |
Contributor
There was a problem hiding this comment.
Compose overrides and extensions in get().
Returning immediately when an override exists drops every extension registered for the same prompt, so plugins cannot layer changes on top of an override.
Suggested fix
def get(self, prompt_name: str, default: Optional[str] = None) -> str:
- if prompt_name in self._overrides:
- return self._overrides[prompt_name]
-
- base = default if default is not None else self._defaults.get(prompt_name, "")
+ base = self._overrides.get(prompt_name)
+ if base is None:
+ base = default if default is not None else self._defaults.get(prompt_name, "")
extensions = self._extensions.get(prompt_name, [])
if extensions:
- return base + "\n" + "\n".join(extensions)
+ return base + ("\n" if base else "") + "\n".join(extensions)
return base📝 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.
Suggested change
| def get(self, prompt_name: str, default: Optional[str] = None) -> str: | |
| if prompt_name in self._overrides: | |
| return self._overrides[prompt_name] | |
| base = default if default is not None else self._defaults.get(prompt_name, "") | |
| extensions = self._extensions.get(prompt_name, []) | |
| if extensions: | |
| return base + "\n" + "\n".join(extensions) | |
| return base | |
| def get(self, prompt_name: str, default: Optional[str] = None) -> str: | |
| base = self._overrides.get(prompt_name) | |
| if base is None: | |
| base = default if default is not None else self._defaults.get(prompt_name, "") | |
| extensions = self._extensions.get(prompt_name, []) | |
| if extensions: | |
| return base + ("\n" if base else "") + "\n".join(extensions) | |
| return base |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/core/plugins/prompts.py` around lines 31 - 41, The get() method in
prompts.py returns immediately when a prompt_name exists in self._overrides,
which drops any registered extensions; change get (function get) to compose the
final prompt by starting from the override (self._overrides[prompt_name]) when
present or the default/_defaults value otherwise, then append any entries from
self._extensions.get(prompt_name, []) (joined with newlines) so overrides and
extensions are layered together; ensure the default parameter logic (default vs
self._defaults) still applies when no override exists and preserve the existing
newline joining behavior.
Comment on lines
+84
to
+124
| def download(self, name: str, version: str = "latest", target_dir: Optional[Path] = None) -> Path: | ||
| url = self._url(f"/plugins/{name}/versions/{version}/download") | ||
|
|
||
| with httpx.Client(timeout=self.timeout, follow_redirects=True) as client: | ||
| response = client.get(url, headers=self._headers()) | ||
| response.raise_for_status() | ||
|
|
||
| with tempfile.NamedTemporaryFile(suffix=".tar.gz", delete=False) as tmp: | ||
| tmp.write(response.content) | ||
| tmp_path = Path(tmp.name) | ||
|
|
||
| extract_dir = tempfile.mkdtemp() | ||
| try: | ||
| if target_dir is None: | ||
| target_dir = Path("plugins") | ||
| target_dir.mkdir(parents=True, exist_ok=True) | ||
|
|
||
| with tarfile.open(tmp_path, "r:gz") as tar: | ||
| tar.extractall(extract_dir) | ||
|
|
||
| manifest_file = None | ||
| for root, _dirs, files in os.walk(extract_dir): | ||
| if "plugin.yaml" in files: | ||
| manifest_file = Path(root) / "plugin.yaml" | ||
| break | ||
|
|
||
| if manifest_file is None: | ||
| raise FileNotFoundError("plugin.yaml not found in archive") | ||
|
|
||
| source_dir = manifest_file.parent | ||
|
|
||
| plugin_dir = target_dir / name | ||
| if plugin_dir.exists(): | ||
| shutil.rmtree(plugin_dir) | ||
| shutil.copytree(source_dir, plugin_dir) | ||
|
|
||
| logger.info("Downloaded plugin '%s' v%s to '%s'", name, version, plugin_dir) | ||
| return plugin_dir | ||
| finally: | ||
| tmp_path.unlink(missing_ok=True) | ||
| shutil.rmtree(extract_dir, ignore_errors=True) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Python tarfile extractall filter parameter version
💡 Result:
The filter parameter was added to tarfile.TarFile.extractall (and extract) in Python 3.12. It accepts a callable filter(member: TarInfo, path: str) -> TarInfo | None or a string name like 'data', 'tar', or 'fully_trusted'. Returning None skips the member; it can also modify metadata or raise FilterError. In Python 3.12-3.13, the default is equivalent to 'fully_trusted' (with deprecation warning). Since Python 3.14, the default is 'data', which blocks dangerous features like absolute paths outside the destination, setuid bits, etc. It was introduced via PEP 706 to address security issues like CVE-2007-4559. The feature may be backported to older versions as security updates; check hasattr(tarfile, 'data_filter') for availability. Example: with tarfile.open('archive.tar') as tar: tar.extractall(filter='data') # Safe default
Citations:
- 1: https://peps.python.org/pep-0706/
- 2: https://discuss.python.org/t/pep-706-filter-for-tarfile-extractall/23903
- 3: https://docs.python.org/3/library/tarfile.html
- 4: https://docs.python.org/3.14/library/tarfile.html
🏁 Script executed:
# Find Python version requirements
find . -maxdepth 2 -type f \( -name "setup.py" -o -name "setup.cfg" -o -name "pyproject.toml" -o -name ".python-version" -o -name "tox.ini" \) -exec cat {} \;Repository: Lumen-Labs/brainapi2
Length of output: 2853
🏁 Script executed:
# Also check if there are any existing Python version checks in the codebase
rg "python_requires|version_info|sys.version" --type py | head -20Repository: Lumen-Labs/brainapi2
Length of output: 46
🏁 Script executed:
# Check the full context of the registry.py file to understand import structure
cat -n src/core/plugins/registry.py | head -100Repository: Lumen-Labs/brainapi2
Length of output: 4175
🏁 Script executed:
# Search for other tarfile usages
rg "tarfile\|extractall" --type pyRepository: Lumen-Labs/brainapi2
Length of output: 46
🏁 Script executed:
# Check the rest of the registry.py file to see full context
tail -n 50 src/core/plugins/registry.pyRepository: Lumen-Labs/brainapi2
Length of output: 2089
Mitigate path traversal vulnerability in tarfile.extractall().
The code is vulnerable to path traversal attacks via malicious tar archives (CVE-2007-4559). Tar members with paths like ../../etc/passwd can write files outside the extraction directory. Python 3.12+ provides a filter parameter, but this project requires Python 3.11+, so manual filtering is necessary.
Implement a safe extraction helper to validate member paths before extraction:
def _safe_extract(tar: tarfile.TarFile, path: str) -> None:
for member in tar.getmembers():
member_path = os.path.join(path, member.name)
if not os.path.abspath(member_path).startswith(os.path.abspath(path) + os.sep):
raise ValueError(f"Attempted path traversal in tar file: {member.name}")
tar.extractall(path)Then replace tar.extractall(extract_dir) with _safe_extract(tar, extract_dir).
🧰 Tools
🪛 Ruff (0.15.6)
[error] 102-102: Uses of tarfile.extractall()
(S202)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/core/plugins/registry.py` around lines 84 - 124, The download method is
vulnerable to tar path traversal when calling tar.extractall; add a helper
function _safe_extract(tar: tarfile.TarFile, path: str) that iterates
tar.getmembers(), computes member_path = os.path.join(path, member.name), and
verifies os.path.abspath(member_path).startswith(os.path.abspath(path) + os.sep)
(or equals the base for root entries), raising ValueError on violation, then
call tar.extractall(path) only after all members are validated; replace the
tar.extractall(extract_dir) call in download with _safe_extract(tar,
extract_dir) to mitigate CVE-2007-4559.
Comment on lines
+47
to
+50
| for event_name, handlers in ctx._event_handlers.items(): | ||
| if event_name == "shutdown": | ||
| for handler in handlers: | ||
| await handler() if _is_coroutine(handler) else handler() |
Contributor
There was a problem hiding this comment.
Run all shutdown hooks even when one plugin fails.
The first exception here aborts the rest of the shutdown sequence, so later plugins never release their resources. Catch/log per-handler failures and continue, then surface an aggregated error if you still want shutdown to fail.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/services/api/app.py` around lines 47 - 50, When iterating
ctx._event_handlers for event_name == "shutdown", wrap each handler invocation
(the call to handler() or await handler() using _is_coroutine) in a try/except
so one handler's exception doesn't abort the rest; on exception log the error
for that specific handler (e.g., logging.exception or the module's existing
logger) and continue to the next handler, and if you still want to surface
failure after running all handlers collect exceptions into a list and
raise/return an aggregated error once the loop completes.
| brainpat = brainpat.rstrip() | ||
| system_pat = os.getenv("BRAINPAT_TOKEN") | ||
|
|
||
| print(brainpat, system_pat) |
Contributor
There was a problem hiding this comment.
Critical: Remove debug print that exposes authentication secrets.
This print statement outputs both the user's brainpat token and the system_pat secret to stdout. In production, these will appear in container logs, log aggregation systems (CloudWatch, Datadog, etc.), and potentially be exposed to anyone with log access. This is a credentials leak vulnerability.
🔒 Proposed fix
system_pat = os.getenv("BRAINPAT_TOKEN")
- print(brainpat, system_pat)
if request.url.path.startswith("/system") or request.url.path == "/":🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/services/api/middlewares/auth.py` at line 42, Remove the debug print that
leaks secrets: delete the print(brainpat, system_pat) call in the auth
middleware and do not log raw tokens or secrets (brainpat, system_pat). If you
need visibility for debugging, replace it with a non-sensitive check or a
boolean/status log (e.g., "brainpat present" or "system_pat from env: yes/no")
or use masked/redacted values, and ensure this change is applied inside the
authentication middleware function handling brainpat/system_pat to avoid any
secret exposure in stdout/container logs.
Comment on lines
+21
to
+29
| def _load_mcp_plugins(): | ||
| from src.core.plugins.context import PluginContext | ||
| from src.core.plugins.loader import PluginLoader | ||
|
|
||
| ctx = PluginContext.from_mcp(mcp) | ||
| loader = PluginLoader(plugins_dir=PLUGINS_DIR, context=ctx) | ||
| results = loader.load_all() | ||
| _log_plugin_banner(loader, results) | ||
|
|
Contributor
There was a problem hiding this comment.
Persist MCP plugin lifecycle handlers through app startup/shutdown.
_load_mcp_plugins() creates a PluginContext and drops it immediately after load_all(). Any plugin that registers startup/shutdown handlers via add_event_handler() will never run in the MCP service, unlike the FastAPI path in src/services/api/app.py.
Also applies to: 77-77
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/services/mcp/app.py` around lines 21 - 29, The _load_mcp_plugins()
function creates a PluginContext and PluginLoader then discards them, so plugin
lifecycle handlers registered via add_event_handler() never persist; modify
_load_mcp_plugins (and the equivalent at line ~77) to store the created
PluginContext and PluginLoader on a long-lived object (e.g., module-level
variables or attached to the MCP app/state) instead of letting them go out of
scope, call loader.load_all() as before, call _log_plugin_banner(loader,
results), and ensure the stored context/loader remain reachable until service
shutdown so registered startup/shutdown handlers can run.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (1)
src/services/mcp/openai-reverse-engineered-prompt.md (1)
17-29: Strip captured chat transcript artifacts from this doc.Line 17–29 and Line 273 look like copied runtime conversation/log output, not stable documentation. This should be removed to keep the artifact deterministic and maintainable.
Suggested cleanup
-Activity -· -27s - -Thinking -Thinking longer for a better answer -Thinking longer for a better answer - -Exporting knowledge and skills -bash -lc cat /home/oai/skills/spreadsheets/SKILL.md && printf ' ----DOCX--- -' && cat /home/oai/skills/docx/SKILL.md | sed -n '1,120p' ... - ChatGPT is still generating a response...Also applies to: 273-273
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/mcp/openai-reverse-engineered-prompt.md` around lines 17 - 29, Remove the captured chat/log artifacts such as the "Activity" block, repeated "Thinking" lines, the "Exporting knowledge and skills" header, and the embedded bash snippet ("bash -lc cat /home/oai/skills/... && printf '---DOCX---' && cat ... | sed -n '1,120p'") from the document; search for and delete the same transcript-like content around the other occurrence noted (line 273) so the file contains only stable documentation text and no runtime conversation or terminal output.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pyproject.toml`:
- Around line 83-85: The pyproject currently declares a console script entry
point brainapi = "src.core.plugins.cli:app" but package-mode is set to false so
Poetry will not install console scripts; fix by either setting package-mode =
true in the pyproject (to enable installation of the brainapi console script) or
remove the [tool.poetry.scripts] section and document using an alternative
invocation such as `poetry run python -m src.core.plugins.cli` or add a
Poethepoet task to run src.core.plugins.cli:app instead; update the pyproject
accordingly and ensure the brainapi entry (src.core.plugins.cli:app) is
consistent with the chosen approach.
In `@src/services/mcp/main.py`:
- Around line 86-87: The catch-all except block returning f"Error executing
graph operation: {e}" should be replaced: stop catching Exception blindly
(remove "except Exception as e" returning raw exception text), instead catch
specific expected errors (e.g., ValueError, KeyError, RuntimeError) around the
graph operation, and for unexpected exceptions use a broad handler only to log
the full traceback via your logger (e.g., logger.exception(...) or
logging.exception(...)) and then return a generic, non-leaking message like
"Internal server error executing graph operation" or an appropriate error code;
reference the current error string "Error executing graph operation" and the
exception variable "e" to locate and update the try/except in main.py.
- Around line 80-85: The embedding and UUID handling is unsafe: ensure the
result from embeddings_adapter.embed_text (query_embedding) contains a non-empty
embeddings payload before calling vector_store_adapter.search_vectors, and after
receiving data_vectors filter and sanitize node IDs derived from
v.metadata.get("uuid") to remove None/empty/invalid values; only call
graph_adapter.get_event_centric_neighbors with a non-empty list of UUIDs
(otherwise return an empty result or handle the error path). Update the logic
around query_embedding, data_vectors, and triplets to validate payloads and
short-circuit when inputs are invalid, referencing
embeddings_adapter.embed_text, vector_store_adapter.search_vectors, and
graph_adapter.get_event_centric_neighbors to locate the changes.
In `@src/services/mcp/openai-reverse-engineered-prompt.md`:
- Line 13: Fix spelling/terminology typos in
src/services/mcp/openai-reverse-engineered-prompt.md by replacing "untill" with
"until" and "than" with "then" in the sentence starting with "i want you
recursively..." (ensure proper capitalization/punctuation for user-facing text),
correct "Openpxyl" to the intended term (likely "OpenAI") where it appears
around the 47th line, and change "styel" to "style" at the 138th-line
occurrence; update all occurrences and run a quick search in that file to ensure
consistency of these corrected tokens.
- Line 13: The prompt line requesting "i want you recursively iterate and export
into a csv or json the knowledge that you have of me..." is an unsafe
data-exfiltration instruction; remove this directive and replace it with a
scoped, consent-first task that does not attempt recursive harvesting or
automatic export. Specifically, update the prompt content to (1) require
explicit user consent and a clear scope of which categories of data may be
summarized, (2) limit iterations (e.g., single-pass summary of known,
user-provided facts) and forbid use of external tools for bulk extraction, and
(3) return a human-readable summary and an optional manual export step initiated
by the user rather than automatic CSV/JSON export. Ensure the changed prompt
text (the offending sentence) is replaced accordingly.
- Line 57: The heading "Use these exact artifact_tool call shapes for getting
started" is currently at level ##### causing a jump from ###; change that
heading to level #### to restore proper markdown heading order (replace the five
'#' with four '#' for that exact heading text).
- Around line 6-9: The file contains direct personal email addresses in the
metadata lines "Author: Christian Nonis <alch.infoemail@gmail.com>" and
"Modified By: Christian Nonis <alch.infoemail@gmail.com>" in
openai-reverse-engineered-prompt.md; remove or redact those personal emails and
replace them with a non-personal team/contact alias (e.g., "team@example.com" or
"OpenAI Team") or simply leave the name without the email so the lines become
safe for public sharing.
---
Nitpick comments:
In `@src/services/mcp/openai-reverse-engineered-prompt.md`:
- Around line 17-29: Remove the captured chat/log artifacts such as the
"Activity" block, repeated "Thinking" lines, the "Exporting knowledge and
skills" header, and the embedded bash snippet ("bash -lc cat
/home/oai/skills/... && printf '---DOCX---' && cat ... | sed -n '1,120p'") from
the document; search for and delete the same transcript-like content around the
other occurrence noted (line 273) so the file contains only stable documentation
text and no runtime conversation or terminal output.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1fddb728-7848-47ef-b97e-22670aaad29d
📒 Files selected for processing (5)
README.mdpyproject.tomlsrc/services/mcp/main.pysrc/services/mcp/openai-reverse-engineered-prompt.mdsrc/services/mcp/utils.py
✅ Files skipped from review due to trivial changes (1)
- src/services/mcp/utils.py
🚧 Files skipped from review as they are similar to previous changes (1)
- README.md
Comment on lines
+83
to
+85
| [tool.poetry.scripts] | ||
| brainapi = "src.core.plugins.cli:app" | ||
|
|
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Poetry, does [tool.poetry.scripts] still create/enable console scripts when package-mode = false is set? If not, what is the recommended way to expose a Typer CLI entry point for a non-package project?
💡 Result:
No, [tool.poetry.scripts] does not create/enable console scripts when package-mode = false is set. In non-package mode, Poetry skips installing the root project itself (equivalent to poetry install --no-root always), which means project-defined console scripts from [tool.poetry.scripts] are not installed into the virtual environment. For a non-package project, the recommended way to expose a Typer CLI entry point is to use poetry run with the module or direct Python file, e.g.: - poetry run python -m your_module (if using typer.run(main) or main.py) - poetry run python your_cli.py - Or poetry run typer your_module.main This runs the CLI within the Poetry-managed virtual environment without needing console scripts. Tools like poethepoet can provide task aliases in pyproject.toml for convenience, but poetry run is the standard approach.
Citations:
- 1: https://python-poetry.org/docs/1.8/pyproject
- 2: https://python-poetry.org/docs/main/cli/
- 3: Introduce non-package-mode python-poetry/poetry#8650
- 4: https://python-poetry.org/docs/main/pyproject/
- 5: https://python-poetry.org/docs/basic-usage
- 6: https://stackoverflow.com/questions/75397736/poetry-install-on-an-existing-project-error-does-not-contain-any-element
Remove [tool.poetry.scripts] or change package-mode to true.
With package-mode = false (Line 44), Poetry will not install console scripts, making the brainapi entry point at Lines 83-85 non-functional. Either set package-mode = true to enable console script installation, or use alternatives like poetry run python -m src.core.plugins.cli to invoke the CLI within Poetry's environment. Alternatively, use poethepoet for task aliases if you want to keep non-package mode.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pyproject.toml` around lines 83 - 85, The pyproject currently declares a
console script entry point brainapi = "src.core.plugins.cli:app" but
package-mode is set to false so Poetry will not install console scripts; fix by
either setting package-mode = true in the pyproject (to enable installation of
the brainapi console script) or remove the [tool.poetry.scripts] section and
document using an alternative invocation such as `poetry run python -m
src.core.plugins.cli` or add a Poethepoet task to run src.core.plugins.cli:app
instead; update the pyproject accordingly and ensure the brainapi entry
(src.core.plugins.cli:app) is consistent with the chosen approach.
Comment on lines
+80
to
+85
| query_embedding = embeddings_adapter.embed_text(query) | ||
| data_vectors = vector_store_adapter.search_vectors( | ||
| query_embedding.embeddings, store="nodes", brain_id=brain_id, k=5 | ||
| ) | ||
| triplets = graph_adapter.get_event_centric_neighbors([v.metadata.get("uuid") for v in data_vectors], brain_id=brain_id) | ||
| return triplets |
Contributor
There was a problem hiding this comment.
Validate embedding payload and sanitize UUIDs before graph lookup.
At Line 82, embedding payload can be empty; at Line 84, metadata.get("uuid") can yield None. Sending empty/invalid node IDs into get_event_centric_neighbors makes successful requests fail unnecessarily.
Suggested fix
- query_embedding = embeddings_adapter.embed_text(query)
- data_vectors = vector_store_adapter.search_vectors(
- query_embedding.embeddings, store="nodes", brain_id=brain_id, k=5
- )
- triplets = graph_adapter.get_event_centric_neighbors([v.metadata.get("uuid") for v in data_vectors], brain_id=brain_id)
+ query_embedding = embeddings_adapter.embed_text(query)
+ if not query_embedding.embeddings:
+ return []
+
+ data_vectors = vector_store_adapter.search_vectors(
+ query_embedding.embeddings, store="nodes", brain_id=brain_id, k=5
+ )
+
+ node_ids = [
+ node_uuid
+ for v in data_vectors
+ for node_uuid in [v.metadata.get("uuid")]
+ if node_uuid
+ ]
+ if not node_ids:
+ return []
+
+ triplets = graph_adapter.get_event_centric_neighbors(
+ node_ids, brain_id=brain_id
+ )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/services/mcp/main.py` around lines 80 - 85, The embedding and UUID
handling is unsafe: ensure the result from embeddings_adapter.embed_text
(query_embedding) contains a non-empty embeddings payload before calling
vector_store_adapter.search_vectors, and after receiving data_vectors filter and
sanitize node IDs derived from v.metadata.get("uuid") to remove
None/empty/invalid values; only call graph_adapter.get_event_centric_neighbors
with a non-empty list of UUIDs (otherwise return an empty result or handle the
error path). Update the logic around query_embedding, data_vectors, and triplets
to validate payloads and short-circuit when inputs are invalid, referencing
embeddings_adapter.embed_text, vector_store_adapter.search_vectors, and
graph_adapter.get_event_centric_neighbors to locate the changes.
Comment on lines
+86
to
+87
| except Exception as e: | ||
| return f"Error executing graph operation: {e}" |
Contributor
There was a problem hiding this comment.
Avoid blind exception catching and returning raw exception text.
Line 86 catches all exceptions, and Line 87 exposes internal error details to clients. This weakens error hygiene and can leak implementation details.
Suggested fix
- except Exception as e:
- return f"Error executing graph operation: {e}"
+ except (ValueError, TypeError) as e:
+ return f"Invalid semantic search input: {e}"
+ except Exception:
+ return "Error executing semantic search"📝 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.
Suggested change
| except Exception as e: | |
| return f"Error executing graph operation: {e}" | |
| except (ValueError, TypeError) as e: | |
| return f"Invalid semantic search input: {e}" | |
| except Exception: | |
| return "Error executing semantic search" |
🧰 Tools
🪛 Ruff (0.15.6)
[warning] 86-86: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/services/mcp/main.py` around lines 86 - 87, The catch-all except block
returning f"Error executing graph operation: {e}" should be replaced: stop
catching Exception blindly (remove "except Exception as e" returning raw
exception text), instead catch specific expected errors (e.g., ValueError,
KeyError, RuntimeError) around the graph operation, and for unexpected
exceptions use a broad handler only to log the full traceback via your logger
(e.g., logger.exception(...) or logging.exception(...)) and then return a
generic, non-leaking message like "Internal server error executing graph
operation" or an appropriate error code; reference the current error string
"Error executing graph operation" and the exception variable "e" to locate and
update the try/except in main.py.
| - Do not assume Office.js, Excel JS, or prior internal workbook-surface APIs apply here. | ||
| - If an API call is unclear, check the `artifact_tool` docs already referenced by this skill before writing code. `artifact_tool_spreadsheet.md` has exact code blocks to get started quickly. | ||
|
|
||
| ##### Use these exact artifact_tool call shapes for getting started |
Contributor
There was a problem hiding this comment.
Fix heading-level jump to satisfy markdown structure rules.
Line 57 jumps from ### to #####, which violates markdown heading increment expectations (MD001). Use #### here.
Suggested fix
-##### Use these exact artifact_tool call shapes for getting started
+#### Use these exact artifact_tool call shapes for getting started📝 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.
Suggested change
| ##### Use these exact artifact_tool call shapes for getting started | |
| #### Use these exact artifact_tool call shapes for getting started |
🧰 Tools
🪛 markdownlint-cli2 (0.21.0)
[warning] 57-57: Heading levels should only increment by one level at a time
Expected: h4; Actual: h5
(MD001, heading-increment)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/services/mcp/openai-reverse-engineered-prompt.md` at line 57, The heading
"Use these exact artifact_tool call shapes for getting started" is currently at
level ##### causing a jump from ###; change that heading to level #### to
restore proper markdown heading order (replace the five '#' with four '#' for
that exact heading text).
Co-authored-by: Christian <ChrisCoder9000@users.noreply.github.com>
Co-authored-by: Christian <ChrisCoder9000@users.noreply.github.com>
…uality-ee03 Refactor agent backend creation and harden adapter failure handling
Merged
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
brainapiCLI tool for plugin and registry operations.Improvements
Chores