Conversation
- Add missing deps to @elizaos/core (drizzle-orm, markdown-it, undici, yaml, sharp) - Fix import paths in autonomy/service.ts and testing/index.ts - Add node types to plugin-anthropic tsconfig to fix uuid type resolution - Fix dependency declarations in several plugins (dangling refs, version ranges) - Upgrade @pixi/react to v8 to match pixi.js v8 in dnd-vtt example Co-authored-by: Cursor <cursoragent@cursor.com>
Manages @elizaos/* dependency references across the monorepo: bun run fix-deps — set workspace:* for local dev bun run fix-deps:restore — restore versioned refs before committing bun run fix-deps:check — CI check for leaked workspace:* refs Co-authored-by: Cursor <cursoragent@cursor.com>
…er detection Explicit env vars (API keys, local URLs) take priority over auto-detected local servers, so users with cloud keys aren't silently overridden by a running Ollama instance. Co-authored-by: Cursor <cursoragent@cursor.com>
…L JSON translation - Schema builders: support complex SQL expression indexes (e.g. ((metadata->>'type'))) and multi-column expression indexes; use sql.raw() for PG and dialect translation for MySQL - buildTable constraints callback now returns array (Drizzle non-deprecated API) - DialectAdapter: add optional translateExpression(expr) for PG→MySQL JSON operator translation - MySQL adapter: translate ->'', ->, ? to JSON_UNQUOTE(JSON_EXTRACT), JSON_EXTRACT, JSON_CONTAINS_PATH - Apply translation to expression indexes and check constraints when building MySQL tables - Fix DialectAdapter.createTable type to (table) => any[] in core and plugin-sql types - Use Object.values in constraint loops to avoid unused key variables Co-authored-by: Cursor <cursoragent@cursor.com>
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
|
Too many files changed for review. ( |
There was a problem hiding this comment.
Pull request overview
Introduces new benchmarking modules (BFCL + AgentBench) and updates repo CI/configuration to support multi-language testing and releases.
Changes:
- Added BFCL benchmark CLI, evaluators, and metrics computation.
- Added AgentBench Python package with adapters, mock runtime, CLI, and tests.
- Updated CI/workflows and added database API documentation/benchmark artifacts; removed some legacy repo configs.
Reviewed changes
Copilot reviewed 299 out of 956 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| benchmarks/bfcl/metrics.py | Adds BFCL metrics aggregation, error analysis, and baseline comparisons |
| benchmarks/bfcl/evaluators/relevance_evaluator.py | Adds relevance (no-tool) correctness evaluator |
| benchmarks/bfcl/evaluators/exec_evaluator.py | Adds mock execution evaluator for function calls |
| benchmarks/bfcl/evaluators/ast_evaluator.py | Adds structural (AST) matcher for function calls/args |
| benchmarks/bfcl/evaluators/init.py | Exposes BFCL evaluators package API |
| benchmarks/bfcl/main.py | Adds BFCL CLI entrypoint (run/models/info) |
| benchmarks/bfcl/init.py | Exposes BFCL public API exports |
| benchmarks/bench_cli_types.py | Adds shared CLI JSON/benchmark typing helpers |
| benchmarks/agentbench/smart_smoke/agentbench-results.json | Adds sample AgentBench results artifact |
| benchmarks/agentbench/smart_smoke/agentbench-report.md | Adds sample AgentBench report artifact |
| benchmarks/agentbench/smart_smoke/agentbench-detailed.json | Adds sample AgentBench detailed run artifact |
| benchmarks/agentbench/pyproject.toml | Adds AgentBench Python package metadata and tooling config |
| benchmarks/agentbench/elizaos_agentbench/trajectory_integration.py | Adds optional trajectory logger integration helpers |
| benchmarks/agentbench/elizaos_agentbench/tests/test_types.py | Adds unit tests for AgentBench types/baselines |
| benchmarks/agentbench/elizaos_agentbench/tests/test_smart_mock_runtime.py | Adds end-to-end deterministic harness test |
| benchmarks/agentbench/elizaos_agentbench/tests/test_runner.py | Adds runner + memory tracker tests |
| benchmarks/agentbench/elizaos_agentbench/tests/test_adapters.py | Adds adapter behavior tests across environments |
| benchmarks/agentbench/elizaos_agentbench/tests/init.py | Marks AgentBench tests package |
| benchmarks/agentbench/elizaos_agentbench/mock_runtime.py | Adds deterministic mock runtime for harness validation |
| benchmarks/agentbench/elizaos_agentbench/cli.py | Adds AgentBench CLI runner and env selection |
| benchmarks/agentbench/elizaos_agentbench/adapters/base.py | Adds base adapter interface and task loop implementation |
| benchmarks/agentbench/elizaos_agentbench/adapters/init.py | Exposes AgentBench adapters API |
| benchmarks/agentbench/elizaos_agentbench/init.py | Exposes AgentBench package API exports |
| benchmarks/agentbench/README.md | Adds AgentBench documentation and usage |
| benchmarks/init.py | Adds benchmarks package marker |
| benchmark_results/bfcl/bfcl_best_results.json | Adds BFCL best-results artifact |
| README.md | Updates branding and some command examples |
| Dockerfile | Removes docker build definition |
| DATABASE_API_README.md | Adds database adapter API documentation |
| DATABASE_API_CHANGELOG.md | Adds database API refactor changelog |
| BENCHMARK_RESULTS.md | Adds write/read benchmark comparison documentation |
| AGENTS.md | Removes repository guidelines doc |
| .vscode/settings.json | Removes editor settings |
| .vscode/launch.json | Removes debug launch configurations |
| .prettierignore | Removes Prettier ignore list |
| .npmrc | Updates npm registry auth configuration |
| .husky/pre-commit | Removes husky pre-commit hook |
| .gitmodules | Removes submodule configuration |
| .github/workflows/weekly-maintenance.yml | Adjusts weekly maintenance workflow settings |
| .github/workflows/update-news.yml | Removes news update workflow |
| .github/workflows/tee-build-deploy.yml | Refactors TEE deployment workflow env handling/concurrency |
| .github/workflows/tauri-ci.yml | Removes Tauri CI workflow |
| .github/workflows/supply-chain.yaml | Adds SBOM + vulnerability scan workflow |
| .github/workflows/release.yaml | Updates release workflow (WASM build, workspace version rewriting) |
| .github/workflows/release-python.yaml | Adds PyPI publishing workflow |
| .github/workflows/pr.yaml | Updates PR title check to set GH_TOKEN for commenting |
| .github/workflows/plugin-sql-tests.yaml | Removes SQL plugin-specific workflow |
| .github/workflows/multi-lang-tests.yaml | Adds Rust/Python/WASM/interop test workflow |
| .github/workflows/jsdoc-automation.yml | Updates JSDoc automation workflow (autodoc existence check) |
| .github/workflows/image.yaml | Updates Docker build workflow actions to pinned major versions |
| .github/workflows/generate-readme-translations.yml | Removes README translation workflow |
| .github/workflows/docs-ci.yml | Adds docs CI workflow (links + quality fixes via Claude) |
| .github/workflows/core-package-tests.yaml | Removes core-package test workflow |
| .github/workflows/codeql.yml | Updates CodeQL workflow triggers/formatting |
| .github/workflows/client-cypress-tests.yml | Removes client Cypress workflow |
| .github/workflows/cli-tests.yml | Removes CLI tests workflow |
| .github/workflows/claude.yml | Updates Claude workflow (timeout + checkout version) |
| .github/workflows/claude-security-review.yml | Updates Claude security review workflow (timeout + formatting) |
| .github/workflows/claude-code-review.yml | Updates Claude code review workflow (timeout + expectations) |
| .github/workflows/ci.yaml | Expands CI to include Python/Rust tests and adjusts paths/timeouts |
| .github/workflows/README.md | Updates workflows documentation |
| .github/renovate-preset.json | Updates Renovate grouping rules and naming |
| .github/dependabot.yml | Updates Dependabot config formatting (currently invalid value) |
| .github/ISSUE_TEMPLATE/feature_request.md | Normalizes YAML quoting |
| .github/ISSUE_TEMPLATE/bug_report.md | Normalizes YAML quoting |
| .dockerignore | Removes dockerignore file |
| .cursorrules | Removes Cursor rules file |
| .cursorignore | Removes Cursor ignore file |
| .biomeignore | Adds Biome ignore config |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
execution_error can never be incremented because the loop continues whenever result.ast_match is true, but the execution error condition requires result.ast_match. Fix by restructuring the control flow so AST matches can still be checked for exec_success (e.g., handle execution errors before the continue, or only continue after all relevant counters are updated).
There was a problem hiding this comment.
package-ecosystem cannot be empty; Dependabot will reject this config. Set it to a valid ecosystem (e.g., npm, github-actions, pip, etc.) and add additional entries if you intend to cover multiple ecosystems.
| - package-ecosystem: "github-actions" # See documentation for possible values |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
benchmarks/bfcl/reporting.py (1)
383-410:⚠️ Potential issue | 🟡 MinorRank numbering will duplicate after insertion.
Incrementing the
enumerateloop variable doesn’t carry to the next iteration, so ranks after the inserted elizaOS row repeat. Use an explicitrankcounter instead.Proposed fix
- for i, (model_name, baseline) in enumerate(sorted_baselines, 1): + rank = 1 + for model_name, baseline in sorted_baselines: # Insert elizaOS in the right position if not elizaos_added and metrics.overall_score > baseline.overall: lines.append( - f"| **{i}** | **elizaOS** | " + f"| **{rank}** | **elizaOS** | " f"**{metrics.overall_score:.2%}** | " f"**{metrics.ast_accuracy:.2%}** | " f"**{metrics.exec_accuracy:.2%}** |" ) elizaos_added = True - i += 1 + rank += 1 lines.append( - f"| {i} | {baseline.model_name} | " + f"| {rank} | {baseline.model_name} | " f"{baseline.overall:.2%} | " f"{baseline.ast:.2%} | " f"{baseline.exec:.2%} |" ) + rank += 1 @@ - if not elizaos_added: + if not elizaos_added: lines.append( - f"| **{len(sorted_baselines) + 1}** | **elizaOS** | " + f"| **{rank}** | **elizaOS** | " f"**{metrics.overall_score:.2%}** | " f"**{metrics.ast_accuracy:.2%}** | " f"**{metrics.exec_accuracy:.2%}** |" )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@benchmarks/bfcl/reporting.py` around lines 383 - 410, The rank numbers duplicate because you incremented the enumerate loop variable (i) which doesn't persist; replace the enumerate logic with an explicit rank counter: create rank = 1 before looping over sorted_baselines, remove usage of enumerate and the i += 1 line, and inside the loop use rank when appending the elizaOS row and baseline row, incrementing rank by 1 after each appended row; after the loop, if elizaos_added is still False append elizaOS using the current rank (instead of len(sorted_baselines)+1) so subsequent ranks stay consistent; update references to i to use rank and keep elizaos_added, sorted_baselines, metrics, and lines unchanged.examples/atropos/textworld/elizaos_atropos_textworld/agent.py (1)
274-295:⚠️ Potential issue | 🟡 MinorClass name violates Python naming convention (PEP 8).
elizaOSAgentstarts with lowercase, but PEP 8 requires CapWords for class names. The standard would beElizaOSAgentorElizaosAgent.This is a public API (exported in
__init__.py), so renaming requires updating imports across the codebase (atropos_integration.py, etc.). Consider whether brand consistency ("elizaOS" as a unit) outweighs style convention compliance.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/atropos/textworld/elizaos_atropos_textworld/agent.py` around lines 274 - 295, Rename the class elizaOSAgent to follow PEP8 (e.g., ElizaOSAgent): update the class declaration name, replace all usages/imports referencing elizaOSAgent (including exports in __init__.py and imports in modules like atropos_integration.py and any tests/examples) to the new name, and add a backwards-compatible alias assignment (elizaOSAgent = ElizaOSAgent) in the module to preserve the public API during rollout.examples/code/src/__tests__/sub-agents.smoke.test.ts (1)
57-82:⚠️ Potential issue | 🟠 Major*Use
bun:testinstead ofvitestin this .test.ts file.The file imports from vitest; repo guidelines require bun:test for all test files.
Change
-import { describe, expect, test } from "vitest"; +import { describe, expect, test } from "bun:test";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/code/src/__tests__/sub-agents.smoke.test.ts` around lines 57 - 82, Replace vitest imports with bun:test in this test file: change any imports like "import { describe, test, expect } from 'vitest'" to import the same symbols from "bun:test", and remove or adjust any vitest-only helpers if present; ensure the test functions (describe, test, expect) used in the OpenCodeSubAgent and elizaOSNativeSubAgent smoke tests remain referenced exactly as before (e.g., in the two test blocks for "OpenCodeSubAgent completes on DONE response" and "elizaOSNativeSubAgent completes on DONE response") so Bun recognizes the tests.examples/code/src/lib/sub-agents/elizaos-native-sub-agent.ts (1)
347-364:⚠️ Potential issue | 🟡 MinorRename class and factory function to follow camelCase/PascalCase conventions.
Class
elizaOSNativeSubAgentshould beElizaOSNativeSubAgent(PascalCase for classes), and factorycreateelizaOSNativeSubAgentshould becreateElizaOSNativeSubAgent(proper camelCase). Update all 11 call sites: registry.ts, index.ts, sub-agents.smoke.test.ts, and elizaos-native-sub-agent.ts.Affected locations
- Line 347: JSDoc
- Line 362: Class definition
- Line 928-930: Factory function JSDoc and definition
- Line 935: Instantiation in factory
- registry.ts line 4: Import
- registry.ts line 84: Instantiation
- index.ts line 6: Export
- sub-agents.smoke.test.ts lines 3, 70-71: Import and usage
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/code/src/lib/sub-agents/elizaos-native-sub-agent.ts` around lines 347 - 364, Rename the class elizaOSNativeSubAgent to ElizaOSNativeSubAgent and rename the factory createelizaOSNativeSubAgent to createElizaOSNativeSubAgent, updating the class declaration and factory definition in elizaos-native-sub-agent.ts as well as all references/imports/usages (registry.ts, index.ts, sub-agents.smoke.test.ts and any other call sites) to the new identifiers; ensure exported names, JSDoc entries mentioning elizaOSNativeSubAgent and createelizaOSNativeSubAgent are updated to the PascalCase/camelCase forms and that instantiations (new ElizaOSNativeSubAgent() or call to createElizaOSNativeSubAgent()) and any registry registrations use the new names consistently.
🧹 Nitpick comments (5)
examples/browser-use/python/run.py (1)
60-62: Consider specifying encoding when opening JSON file.For cross-platform consistency (especially on Windows), explicitly pass
encoding="utf-8".Proposed fix
if character_file.exists(): - with open(character_file) as f: + with open(character_file, encoding="utf-8") as f: return json.load(f)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/browser-use/python/run.py` around lines 60 - 62, The JSON file is opened without an explicit encoding which can cause cross-platform inconsistencies; update the open call that checks character_file.exists() (the with open(character_file) as f: block used before json.load(f)) to pass encoding="utf-8" so the file is read deterministically across platforms and then call json.load(f) as before.examples/art/elizaos_art/eliza_integration/trajectory_plugin_integration.py (1)
486-502: Fallback export methods are stubs.
export_openpipe_artimports the exporter but doesn't call it, andexport_grpo_groupsjust returns a path without writing data. If the plugin import fails and fallback is used, these methods silently produce nothing.Consider either implementing the actual export logic or raising
NotImplementedErrorto make the limitation explicit.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/art/elizaos_art/eliza_integration/trajectory_plugin_integration.py` around lines 486 - 502, The fallback export methods currently do not perform any export: export_openpipe_art imports export_trajectories_art_format but never calls it (leaving trajectories unwritten) and export_grpo_groups only returns a path without writing groups; update export_openpipe_art to collect trajectories from self._logger (using self._logger.list_trajectories() and self._logger.load_trajectory(tid)) and call export_trajectories_art_format(...) to write to out_path (or, if you prefer not to implement fallback behavior here, explicitly raise NotImplementedError with a clear message), and similarly implement export_grpo_groups to serialize and write the group data to out_path (using self._logger.data_dir for default path) or raise NotImplementedError so the absence of fallback behavior is explicit.examples/chat/typescript/chat.ts (1)
146-161: Local providers always show ❌ regardless of actual status.Since this runs only after
loadLLMPlugin()fails, this is functionally fine. However, a user with Ollama running but misconfigured might be confused seeing "not detected" when the server is up. Consider clarifying the message or making detection async.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/chat/typescript/chat.ts` around lines 146 - 161, printAvailableProviders currently always marks local providers as "❌ ... (not detected)" because it assumes loadLLMPlugin failed; update it to either perform an async runtime check for local providers or clarify the message: create a helper like hasLocalProvider(provider) that attempts a lightweight connection/check (e.g., ping Ollama) and await it in an async printAvailableProviders to show ✅ when reachable, or change the static text for local entries to "❌ or misconfigured — not detected or misconfigured (start the server or set ...)" so users aren’t misled; reference the function printAvailableProviders, the LLM_PROVIDERS list, and the existing hasValidApiKey helper when implementing the new detection or message.DOCUMENTATION_COMPLETE.md (1)
51-70: Add language specifier to fenced code block.Static analysis flags this code block lacks a language identifier. Use
textorplaintextfor ASCII diagrams.📝 Suggested fix
-``` +```text /root/eliza260106/ │ ├── DATABASE_API_DOCS_INDEX.md ← START HERE (navigation)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@DOCUMENTATION_COMPLETE.md` around lines 51 - 70, The fenced ASCII diagram in DOCUMENTATION_COMPLETE.md is missing a language identifier; update the opening fence for that diagram from ``` to ```text (or ```plaintext) so the block is language-specified for linters and renderers; locate the ASCII tree diagram block shown under the repository root snippet and change its fence accordingly.benchmarks/tau-bench/elizaos_tau_bench/__init__.py (1)
35-66: Consider a backward-compat alias for the renamed export.
RemovingElizaOSTauAgentfrom__all__is a breaking change for downstream imports; an alias keeps migrations smoother.♻️ Backward-compat alias
from elizaos_tau_bench.eliza_agent import ( elizaOSTauAgent, MockTauAgent, create_tau_agent, ELIZAOS_AVAILABLE, ) + +# Backward-compat export (deprecated) +ElizaOSTauAgent = elizaOSTauAgent ... __all__ = [ # Core components "TauBenchRunner", "TauBenchEvaluator", "TauAgent", # elizaOS-integrated agents "elizaOSTauAgent", + "ElizaOSTauAgent", "MockTauAgent",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@benchmarks/tau-bench/elizaos_tau_bench/__init__.py` around lines 35 - 66, The PR removed the old export name ElizaOSTauAgent which breaks downstream imports; restore a backward-compatible alias by assigning ElizaOSTauAgent = elizaOSTauAgent in this module and include "ElizaOSTauAgent" in the __all__ list alongside "elizaOSTauAgent" (ensure you reference the existing symbol elizaOSTauAgent and the module-level __all__ list). This keeps the new name (elizaOSTauAgent) while preserving the legacy import for consumers.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@BENCHMARK_RESULTS.md`:
- Around line 15-24: Multiple fenced code blocks use bare ``` markers causing
markdownlint MD040; update each triple-backtick fence to include a language
identifier (e.g., change ``` to ```text) for every benchmark table block such as
the blocks starting with "WRITE OPERATIONS (N=10,000)", the other table blocks
referenced in the comment (the ones around lines 28-40, 58-66, 75-87, 95-105,
113-119), and any similar fenced tables so that each opening fence is ```text
(and keep the closing fence as ```); this will satisfy MD040 and improve
rendering.
In `@DATABASE_API_README.md`:
- Line 161: The README's API table incorrectly documents
deleteParticipants(participants) as returning boolean; update the documentation
entry for the deleteParticipants method to show a void return (no value) and
adjust the descriptive text if needed to match the design principle that
deleteParticipants returns void rather than boolean so it aligns with the actual
method signature and behavior.
- Around line 118-119: The README table is inconsistent: change the return types
for updateAgents and deleteAgents to match the design (they should be void and
throw on failure rather than returning boolean). Update the entries for
`updateAgents(updates)` and `deleteAgents(ids)` in the table to show `void` as
the return type and ensure any descriptive text reflects that these methods
throw on failure instead of returning success/failure.
In `@examples/app/capacitor/capacitor.config.ts`:
- Line 5: Change the string literal for the appName property to use single
quotes to comply with the repo Prettier settings; locate the appName entry
(appName: "elizaOS Chat",) in capacitor.config.ts and replace the double-quoted
value with a single-quoted one to match the project's formatting rules.
In `@examples/atropos/diplomacy/pyproject.toml`:
- Around line 7-8: The package version in pyproject.toml ("version = \"2.0.0\"")
is out of sync with the module version constant __version__ = "1.0.0" in
__init__.py; update the __version__ symbol in the package's __init__.py (or
alternatively update pyproject.toml) so both declare the same semantic version
(e.g., set __version__ to "2.0.0") and ensure any packaging or CI that reads
either value is consistent.
In `@examples/code/src/__tests__/sub-agents.smoke.test.ts`:
- Line 3: The import and usages reference a class named elizaOSNativeSubAgent
which should be renamed to PascalCase ElizaOSNativeSubAgent; update the import
statement to import { ElizaOSNativeSubAgent } from
"../lib/sub-agents/elizaos-native-sub-agent.js" and update all references/usages
(including the instantiation or type mentions around lines 70–71) to use
ElizaOSNativeSubAgent instead of elizaOSNativeSubAgent, ensuring any exported
class name in the source module matches the new PascalCase identifier.
---
Outside diff comments:
In `@benchmarks/bfcl/reporting.py`:
- Around line 383-410: The rank numbers duplicate because you incremented the
enumerate loop variable (i) which doesn't persist; replace the enumerate logic
with an explicit rank counter: create rank = 1 before looping over
sorted_baselines, remove usage of enumerate and the i += 1 line, and inside the
loop use rank when appending the elizaOS row and baseline row, incrementing rank
by 1 after each appended row; after the loop, if elizaos_added is still False
append elizaOS using the current rank (instead of len(sorted_baselines)+1) so
subsequent ranks stay consistent; update references to i to use rank and keep
elizaos_added, sorted_baselines, metrics, and lines unchanged.
In `@examples/atropos/textworld/elizaos_atropos_textworld/agent.py`:
- Around line 274-295: Rename the class elizaOSAgent to follow PEP8 (e.g.,
ElizaOSAgent): update the class declaration name, replace all usages/imports
referencing elizaOSAgent (including exports in __init__.py and imports in
modules like atropos_integration.py and any tests/examples) to the new name, and
add a backwards-compatible alias assignment (elizaOSAgent = ElizaOSAgent) in the
module to preserve the public API during rollout.
In `@examples/code/src/__tests__/sub-agents.smoke.test.ts`:
- Around line 57-82: Replace vitest imports with bun:test in this test file:
change any imports like "import { describe, test, expect } from 'vitest'" to
import the same symbols from "bun:test", and remove or adjust any vitest-only
helpers if present; ensure the test functions (describe, test, expect) used in
the OpenCodeSubAgent and elizaOSNativeSubAgent smoke tests remain referenced
exactly as before (e.g., in the two test blocks for "OpenCodeSubAgent completes
on DONE response" and "elizaOSNativeSubAgent completes on DONE response") so Bun
recognizes the tests.
In `@examples/code/src/lib/sub-agents/elizaos-native-sub-agent.ts`:
- Around line 347-364: Rename the class elizaOSNativeSubAgent to
ElizaOSNativeSubAgent and rename the factory createelizaOSNativeSubAgent to
createElizaOSNativeSubAgent, updating the class declaration and factory
definition in elizaos-native-sub-agent.ts as well as all
references/imports/usages (registry.ts, index.ts, sub-agents.smoke.test.ts and
any other call sites) to the new identifiers; ensure exported names, JSDoc
entries mentioning elizaOSNativeSubAgent and createelizaOSNativeSubAgent are
updated to the PascalCase/camelCase forms and that instantiations (new
ElizaOSNativeSubAgent() or call to createElizaOSNativeSubAgent()) and any
registry registrations use the new names consistently.
---
Nitpick comments:
In `@benchmarks/tau-bench/elizaos_tau_bench/__init__.py`:
- Around line 35-66: The PR removed the old export name ElizaOSTauAgent which
breaks downstream imports; restore a backward-compatible alias by assigning
ElizaOSTauAgent = elizaOSTauAgent in this module and include "ElizaOSTauAgent"
in the __all__ list alongside "elizaOSTauAgent" (ensure you reference the
existing symbol elizaOSTauAgent and the module-level __all__ list). This keeps
the new name (elizaOSTauAgent) while preserving the legacy import for consumers.
In `@DOCUMENTATION_COMPLETE.md`:
- Around line 51-70: The fenced ASCII diagram in DOCUMENTATION_COMPLETE.md is
missing a language identifier; update the opening fence for that diagram from
``` to ```text (or ```plaintext) so the block is language-specified for linters
and renderers; locate the ASCII tree diagram block shown under the repository
root snippet and change its fence accordingly.
In `@examples/art/elizaos_art/eliza_integration/trajectory_plugin_integration.py`:
- Around line 486-502: The fallback export methods currently do not perform any
export: export_openpipe_art imports export_trajectories_art_format but never
calls it (leaving trajectories unwritten) and export_grpo_groups only returns a
path without writing groups; update export_openpipe_art to collect trajectories
from self._logger (using self._logger.list_trajectories() and
self._logger.load_trajectory(tid)) and call export_trajectories_art_format(...)
to write to out_path (or, if you prefer not to implement fallback behavior here,
explicitly raise NotImplementedError with a clear message), and similarly
implement export_grpo_groups to serialize and write the group data to out_path
(using self._logger.data_dir for default path) or raise NotImplementedError so
the absence of fallback behavior is explicit.
In `@examples/browser-use/python/run.py`:
- Around line 60-62: The JSON file is opened without an explicit encoding which
can cause cross-platform inconsistencies; update the open call that checks
character_file.exists() (the with open(character_file) as f: block used before
json.load(f)) to pass encoding="utf-8" so the file is read deterministically
across platforms and then call json.load(f) as before.
In `@examples/chat/typescript/chat.ts`:
- Around line 146-161: printAvailableProviders currently always marks local
providers as "❌ ... (not detected)" because it assumes loadLLMPlugin failed;
update it to either perform an async runtime check for local providers or
clarify the message: create a helper like hasLocalProvider(provider) that
attempts a lightweight connection/check (e.g., ping Ollama) and await it in an
async printAvailableProviders to show ✅ when reachable, or change the static
text for local entries to "❌ or misconfigured — not detected or misconfigured
(start the server or set ...)" so users aren’t misled; reference the function
printAvailableProviders, the LLM_PROVIDERS list, and the existing hasValidApiKey
helper when implementing the new detection or message.
| ``` | ||
| WRITE OPERATIONS (N=10,000) | OLD (singular API) | NEW (batch-first API) | ||
| ─────────────────────────────┼───────────────────────┼──────────────────────── | ||
| | loop batch spd | loop batch spd | ||
| createAgents | 6964ms 7021ms 1.0x | 2642ms 490ms 5.4x | ||
| createEntities | 4231ms 710ms 6.0x | 3627ms 217ms 16.7x | ||
| createMemories | 8384ms 8365ms 1.0x | 4912ms 443ms 11.1x | ||
| updateAgents | 3899ms 3956ms 1.0x | 2618ms 220ms 11.9x | ||
| upsertAgents | [NOT AVAILABLE] | 488ms 485ms 1.0x | ||
| ``` |
There was a problem hiding this comment.
Add language identifiers to fenced blocks.
markdownlint MD040 flags these fences; add text (or another appropriate language) to keep lint clean and improve rendering.
🔧 Suggested fix
-```
+```text
WRITE OPERATIONS (N=10,000) | OLD (singular API) | NEW (batch-first API)
@@
-```
+```text
Operation OLD batch NEW batch Speedup Change
@@
-```
+```text
WRITE OPERATIONS (N=100,000) — NEW batch-first API
@@
-```
+```text
Query OLD (10K) NEW (10K)
@@
-```
+```text
Query OLD (10K) NEW (10K) Index
@@
-```
+```text
Query 10K rows 100K rows (NEW)Also applies to: 28-40, 58-66, 75-87, 95-105, 113-119
🧰 Tools
🪛 markdownlint-cli2 (0.20.0)
[warning] 15-15: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@BENCHMARK_RESULTS.md` around lines 15 - 24, Multiple fenced code blocks use
bare ``` markers causing markdownlint MD040; update each triple-backtick fence
to include a language identifier (e.g., change ``` to ```text) for every
benchmark table block such as the blocks starting with "WRITE OPERATIONS
(N=10,000)", the other table blocks referenced in the comment (the ones around
lines 28-40, 58-66, 75-87, 95-105, 113-119), and any similar fenced tables so
that each opening fence is ```text (and keep the closing fence as ```); this
will satisfy MD040 and improve rendering.
odilitime
changed the title
…memory API
Rust host:
- Add get_memories_by_ids (batch get) with default impl on DatabaseAdapter/UnifiedDatabaseAdapter
- Add UpdateMemoryItem as partial update (id + optional content, metadata, etc.); update_memories takes &[UpdateMemoryItem]
- Add create_memory_with_unique; create_memory delegates with unique=None
- Integration test MockDatabaseAdapter: implement update_memories(UpdateMemoryItem), add CreateMemoryItem import
Plugins (TS/Python):
- plugin-memory, plugin-planning: migrate from getMemoryManager/get_memory_manager to runtime createMemory/getMemories/updateMemory/deleteMemory (create_memory, get_memories, update_memory, delete_memory in Python)
Tests/fixes:
- message_service: fix test_should_respond_ignore_short_circuits by setting VALIDATION_LEVEL=trusted so mock IGNORE response is accepted
- platform.rs: fix define_platform_trait doctests (import macro, use trait Name [] { } syntax)
Misc: tsconfig/package.json/build updates across examples and plugins.
Co-authored-by: Cursor <cursoragent@cursor.com>
|
Claude finished @odilitime's task in 2m 34s —— View job PR Review Complete ✅Overall Assessment: This is a well-designed database refactor with significant performance improvements (14x+ for batch operations). The batch-first API design is sound and the documentation is excellent. However, there are a few issues to address. ❌ CRITICAL: Missing Tests for
|
| Method | Documented | Design Principle (lines 52-58) |
|---|---|---|
updateAgents |
boolean |
Should be void |
deleteAgents |
boolean |
Should be void |
deleteParticipants |
boolean |
Should be void |
The actual interface (packages/typescript/src/types/database.ts:379-380) shows boolean returns, but the design principles state void. Either update the interface or the documentation to be consistent.
⚠️ IMPORTANT: Benchmark Bug (Already Flagged)
benchmarks/bfcl/reporting.py:394 - The i += 1 inside enumerate loop doesn't persist. This was already identified by CodeRabbit but worth confirming it gets fixed.
💡 SUGGESTIONS
-
In-memory adapter
patchComponent: Consider implementingpatchComponentinplugin-inmemorydbto maintain interface parity, even if it's a simple JS object manipulation. -
Test runner detection: The component tests still use single-item methods (
createComponent,updateComponent) rather than the new batch methods. Consider adding explicit batch method tests. -
Type safety improvement:
plugins/plugin-sql/typescript/base.ts:518usesas anycast incountMemories. Consider adding a proper type union instead.
✅ What's Good
- Batch-first design is well-documented with clear rationale
- 14x+ performance improvements validated with benchmarks
- Path validation in
patchComponentproperly prevents SQL injection - Comprehensive test coverage (62 test files in plugin-sql, 68 in core)
- Good separation of concerns - stores, adapters, types cleanly separated
- Plugin schema registration enables ORM-agnostic plugin data storage
- Transient error retry logic correctly distinguishes permanent vs retryable errors
Summary
The core database refactor is solid and ready for merge after:
- Adding
patchComponenttests - Fixing doc/interface return type inconsistencies
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
…upsertComponents, patchComponent, upsertMemories - Add optional entityContext/options.entityContext to the five methods in core (IDatabaseAdapter, DatabaseAdapter, InMemoryAdapter) and runtime pass-through. - Plugin-sql base: when entityContext set, run inside withEntityContext so Postgres RLS applies; shared createProxyWithDb for both transaction branches. - Other adapters (LocalDB, InMemoryDB, MySQL base): accept and ignore entityContext. - MySQL base queryEntities: destructure entityContext out before calling store. - Unit tests (pg/pglite adapter, entity-context paths); integration test for adapter.queryEntities with entityContext under RLS. - README section 'Entity context and RLS' with WHYs; CHANGELOG; code comments (WHY). Co-authored-by: Cursor <cursoragent@cursor.com>
|
Claude finished @odilitime's task in 2m 36s —— View job PR Review: Database RefactorTasks:
SummaryOverall this is a well-designed database refactor with solid test coverage (119 test files in plugin-sql using vitest). The batch-first API design is sound, path validation for Issues❌ CRITICAL: Missing Tests for
|
| Method | Documented | Design Principle |
|---|---|---|
updateAgents |
boolean |
void |
deleteAgents |
boolean |
void |
deleteParticipants |
boolean |
void |
The actual interface (packages/typescript/src/types/database.ts:379-380) matches the docs (boolean). Sync documentation to match actual implementation or vice versa.
💡 SUGGESTIONS
-
Type safety in
base.ts: Twoas anycasts at lines 572 and 1202:- Line 572:
stores.countMemories(this.db, roomIdOrParams as any, unique, tableName) - Line 1202:
proxy.db = dbOrTx as any;
Consider adding proper type definitions to eliminate these casts.
- Line 572:
-
InMemory adapter
patchComponent: Consider implementingpatchComponentinplugin-inmemorydbto maintain interface parity.
✅ What's Good
- Test framework: All tests use vitest correctly (131 files with
@elizaos/coreimports) - Path validation:
patchComponentproperly validates paths withPATH_SEGMENT_REto prevent SQL injection - No hardcoded secrets: Config uses
env()substitution correctly - Proper imports: All imports use
@elizaos/core(notpackages/typescript) - Comprehensive RLS tests: New
rls-entity.test.tscovers entity context - CI passing: PR title checks and security review pass
Verdict
The core database refactor is solid. Address the critical issues before merge:
- Add
patchComponenttests - Fix benchmark enumerate bug
- Sync doc/interface return types
…acy migration
Replace application_name-based server context with parameterized set_config('app.server_id', ...)
for SQL injection safety. Rename withEntityContext to withIsolationContext across all adapters.
Remove legacy v1.6.4→v1.6.5 migration script (migrateToEntityRLS) since v2.0.0 starts with
a fresh snake_case schema.
…t and store-based base Co-authored-by: Cursor <cursoragent@cursor.com>
- Align test imports to use ./tables (no schema module) - Update entity/crud tests for batch adapter API (createEntities returns UUID[]) - Add singular helpers in base adapter (createAgent, getAgent, createWorld, etc.) - Fix relationship store: tags column as text[] (ARRAY[]::text[]), surface update errors - Fix memory store: preserve content/metadata on partial updates - Cache/cascade tests: ensure agent exists, relax cascade assertions when no FKs - base-comprehensive: expect UUID from createComponent, filter memories by id - runtime-migrator: allow fkCount >= 0 when FKs not created - agent.test: use toEqual for array bio, conditional cascade verification - rls-entity.test: fix describe callback closing (}, not });) for parse Co-authored-by: Cursor <cursoragent@cursor.com>
|
Claude finished @odilitime's task in 2m 49s —— View job PR Review: Database RefactorTasks:
SummaryThis is a well-designed database refactor with solid documentation and good test coverage overall. The batch-first API design is sound, path validation for However, two critical issues flagged in previous reviews remain unaddressed. ❌ CRITICAL: Missing Tests for
|
| Method | Documented | Design Principle |
|---|---|---|
updateAgents |
boolean |
void |
deleteAgents |
boolean |
void |
deleteParticipants |
boolean |
void |
The actual interface (packages/typescript/src/types/database.ts:375-376) returns boolean. Either sync docs or change interface.
💡 SUGGESTIONS: Type Safety
Multiple as any casts in production code:
plugins/plugin-sql/typescript/base.ts:649—countMemoriesparameterplugins/plugin-sql/typescript/base.ts:1405— proxy.db assignmentplugins/plugin-sql/typescript/stores/participant.store.ts:304-323— roomState/metadata castingplugins/plugin-sql/typescript/mysql/stores/plugin.store.ts:250-297— affectedRows access
Consider adding proper type definitions to eliminate these casts.
✅ What's Good
- Test framework: All tests use vitest correctly
- Path validation:
patchComponentvalidates paths withPATH_SEGMENT_REto prevent SQL injection - No hardcoded secrets: Config uses env variables
- Proper imports: All imports use
@elizaos/core - Comprehensive RLS tests:
rls-entity.test.tscovers entity context - Good documentation: Design rationale is well-documented
Verdict
Address the critical issues before merge:
- Add
patchComponenttests - Fix benchmark enumerate bug
- Sync doc/interface return types
Test adapters (bootstrap + main test-utils): - Add agents map, upsertAgents, createRoomParticipants; createEntities return UUID[] - getAgentsByIds/createAgents/upsertAgents use agents map; cleanupTestRuntime accepts undefined Bootstrap/runtime: - actions.test: use updateParticipantUserState (not setParticipantUserState) - runtime.test: expect adapter.initialize (not init), mock initialize sets adapterReady Secrets/policy/onboarding: - Telegram token tests: use 35-char token after colon in secrets-validation and onboarding-cli - tool-policy: empty allow list => allow all (expect true) - onboarding-state: getUnconfiguredRequired use value == null for null/undefined ensure-agent-exists/agent-uuid: - Add upsertAgentsMock; tests assert on upsertAgents; createAgents/createEntities return UUID[] - getAgentsByIds mock twice for create flow; add Entity import real-runtime mock: - Add agents map, upsertAgents, createEntities return UUID[]; add upsertRooms for ensureRoomExists ApprovalService: - stop() resolves pending approvals with cancelled: true before clearing autonomy.test: add getRoomsForParticipants to runtime mock e2e: vitest.config.ts exclude e2e/** so Playwright specs not run by vitest plugin-browser-imports: skip plugins without index.browser.ts; add plugin-elizacloud to allowlist Co-authored-by: Cursor <cursoragent@cursor.com>
|
Claude finished @odilitime's task in 2m 26s —— View job PR Review: Database RefactorTasks:
SummaryThis is a well-designed database refactor with good documentation and solid test coverage (60+ test files in plugin-sql using vitest). The batch-first API design is sound, path validation in However, two critical issues flagged in previous reviews remain unaddressed. ❌ CRITICAL: Missing Tests for
|
| Method | Documented | Design Principle |
|---|---|---|
updateAgents |
boolean |
void |
deleteAgents |
boolean |
void |
deleteParticipants |
boolean |
void |
The actual interface (packages/typescript/src/types/database.ts:375-376) returns boolean. Sync docs or interface.
💡 SUGGESTIONS: Type Safety
Multiple as any casts in production code:
plugins/plugin-sql/typescript/base.ts:649— countMemories parameterplugins/plugin-sql/typescript/base.ts:1405— proxy.db assignmentplugins/plugin-sql/typescript/stores/participant.store.ts:304-323— roomState/metadata castingplugins/plugin-sql/typescript/mysql/stores/plugin.store.ts:250-297— affectedRows access
Consider adding proper type definitions to eliminate these casts.
✅ What's Good
- Test framework: All tests use vitest correctly (60+ test files)
- Path validation:
patchComponentvalidates paths withPATH_SEGMENT_REto prevent SQL injection - No hardcoded secrets: Config uses env variables
- Proper imports: All imports use
@elizaos/core - Comprehensive RLS tests:
rls-entity.test.tscovers entity context - Good documentation: Design rationale is well-documented in README
Verdict
Address the critical issues before merge:
- ❌ Add
patchComponenttests - ❌ Fix benchmark enumerate bug
⚠️ Sync doc/interface return types
| class ElizaOSMind2WebAgent: | ||
| """ElizaOS-powered agent for Mind2Web benchmark.""" | ||
| class elizaOSMind2WebAgent: | ||
| """elizaOS-powered agent for Mind2Web benchmark.""" |
There was a problem hiding this comment.
Class renamed to lowercase breaking Python naming conventions
Low Severity
The brand rename from ElizaOS to elizaOS was applied to class names, producing elizaOSMind2WebAgent which starts with a lowercase letter, violating PEP 8's CamelCase convention for classes. This pattern is also applied to elizaOSTauAgent and elizaOSWebShopAgent in other benchmark files. Class names starting with lowercase look like functions or variables, which can confuse developers and static analysis tools.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
PR Review: Database RefactorTasks:
SummaryThis is a well-designed database refactor. Most previously flagged issues have been addressed:
❌ CRITICAL:
|
| File | Count |
|---|---|
stores/plugin.store.ts |
4 |
stores/entity.store.ts |
3 |
stores/task.store.ts |
1 |
These are drizzle query builder type narrowing - acceptable but could be improved.
✅ What's Good
- Security: Path validation with
PATH_SEGMENT_REprevents SQL injection inpatchComponent - Test coverage:
patchComponenthas 8 tests covering all operations (set, push, remove, increment) + error cases - Test framework: Uses vitest correctly (
component.test.ts:12imports from vitest) - Imports: Uses
@elizaos/corecorrectly - Interface design: Return types match design principles (
voidfor update/delete) - Documentation: README accurately reflects API behavior
- Batch-first design: Well-documented with clear performance rationale
Verdict
Approve with minor changes. Fix the print_results async bug before merge — it will return a coroutine object instead of the report.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 3 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
| ) | ||
|
|
||
| # Generate report (avoiding asyncio.run() since this may be called from async context) | ||
| return reporter.generate_report(results) |
There was a problem hiding this comment.
print_results returns unawaited coroutine, never executes
High Severity
print_results is a synchronous function that returns the result of reporter.generate_report(results), which is an async method. This means print_results returns a coroutine object that is never awaited. Both callers in __main__.py and run_benchmark.py call print_results(results) without await, so the report generation (including console output and file writing) silently never executes. The old implementation was synchronous and worked correctly.
Additional Locations (1)
| print("\nBenchmark Summary:") | ||
| print(f"Model: {model_name}") | ||
| print(f"Best Overall Score: {max(r['metrics'].overall_score for r in sorted_results):.2%}") | ||
| print(f"Error Types: {len(self.error_analysis)}") |
There was a problem hiding this comment.
generate_report crashes on empty results list
Medium Severity
generate_report accesses sorted_results[0] in the baseline comparison section (lines 123, 171-173) and calls max(...) over sorted_results (line 190) without guarding against an empty list. If self.results is empty but self.baseline_scores is non-empty, this raises an IndexError. The max() call on line 190 raises ValueError on an empty sequence regardless of baseline config.
| # Collect error analysis data | ||
| if error_data: | ||
| for error_type, count in error_data.items(): | ||
| self.error_analysis[error_type] = self.error_analysis.get(error_type, 0) + count |
There was a problem hiding this comment.
add_result assumes error_data values are numeric
Medium Severity
add_result iterates over error_data.items() and does arithmetic addition (+ count), assuming all values are integers. However, print_results passes result.details as error_data, which comes from ast_evaluator.get_match_details() — a dict that can contain lists, strings, and booleans (like mismatch details), not just integer counts. This causes a TypeError at runtime.
Iteration 6 prr-fix:ic-4008414039-4
Changes: - DOCUMENTATION_COMPLETE.md: ### Internal developer paths leaked into committed documentation Low Severity... - reporting.py: CRITICAL: enumerate loop variable reassignment bug `rank += 1` has no effect ... - reporting.py: CRITICAL: enumerate loop variable bug (flagged 5+ times in prior reviews) `ra... - database.ts: IMPORTANT: Interface/Documentation mismatch These methods return `Promise`, b... - reporting.py: ### Leaderboard rank increment has no effect across iterations Medium Severit... - DOCUMENTATION_COMPLETE.md: ### Stale documentation claims completeness with non-existent files Low Sever... - reporting.py: CRITICAL: Bug still present `rank += 1` has no effect because `enumerate()` r... - database.ts: Interface inconsistency This returns `Promise` but the design principle at `D... - reporting.py: CRITICAL: Bug still present (was reverted in c262242) `rank += 1` has no eff... - reporting.py: CRITICAL: Bug still present after 6+ review cycles `rank += 1` has no effect ... - reporting.py: CRITICAL: Missing rank increment After appending the baseline entry, you need... - reporting.py: ### Leaderboard fallback rank ignores elizaOS insertion offset Low Severity T... - reporting.py: ### Duplicate elizaOS row in leaderboard when score is lowest Medium Severity... - reporting.py: ### Reporting module replaced with invalid placeholder text High Severity The... - reporting.py: ### Rewritten `reporting.py` breaks all callers with incompatible API High Se... - reporting.py: ### Leaderboard rank numbering starts at zero instead of one Low Severity The... - reporting.py: CRITICAL: API Breaking Change `BFCLReporter.init` now takes no arguments, but... - reporting.py: CRITICAL: Type mismatch + rank off-by-one 1. Type mismatch: Callers pass `BFC... - reporting.py: ### Rewritten `BFCLReporter` API breaks all existing callers High Severity Th... - reporting.py: CRITICAL: Type mismatch + rank off-by-one 1. Type mismatch: Callers pass `BFC... - reporting.py: CRITICAL: Missing import + wrong attribute access 1. `BFCLBenchmarkResults` i... - reporting.py: ### `printresults` accesses nonexistent `.metrics` on `BFCLResult` High Sever... - reporting.py: ### `BFCLReporter` rewrite removes all reporting functionality used by caller... - reporting.py: CRITICAL: Wrong attribute access `result` is a `BFCLResult` which does NOT ha... - reporting.py: ### Reporter `generatereport` breaks callers expecting async with args High S... - reporting.py: ### Hardcoded rank=1 ignores actual leaderboard position Medium Severity The ... - reporting.py: CRITICAL: Async/argument mismatch with callers `runner.py:146` and `:485` cal... - reporting.py: ### Reporter API mismatch causes runner crash on report generation High Sever... - reporting.py: ### Reporting rewrite loses all file output and console summary Medium Severi... - reporting.py: ### `printresults` calls async method without await or argument High Severity... - reporting.py: ### Rewritten reporter drops all file output functionality Medium Severity Th... - reporting.py: CRITICAL: Missing await `generatereport()` is async (line 21) but is called h... - reporting.py: ### `asyncio.run()` crashes inside already-running event loop High Severity `... - reporting.py: ### `BFCLConfig` dataclass lacks dict `.get()` method High Severity `BFCLRepo... - reporting.py: ### Reporter rewrite loses leaderboard ranking and reporting features Medium ... - reporting.py: ### ❌ CRITICAL: Benchmark Bug (UNFIXED × 3 reviews) `benchmarks/bfcl/reportin... - reporting.py: ### ❌ CRITICAL: Benchmark Bug (UNFIXED × 4 reviews) `benchmarks/bfcl/reportin... - reporting.py: ### ❌ CRITICAL: Benchmark Bug (UNFIXED × 5 reviews) `benchmarks/bfcl/reportin... - reporting.py: ### ❌ CRITICAL: Benchmark Bug (Unfixed) `benchmarks/bfcl/reporting.py:394` — ... - reporting.py: ### ❌ CRITICAL: Benchmark Bug Still Present `benchmarks/bfcl/reporting.py:397... - DOCUMENTATION_COMPLETE.md: ### Summary This is a well-designed database refactor. Most previously flagge... - DOCUMENTATION_COMPLETE.md: ### Summary This is a well-designed database refactor. Most previously flagge... - reporting.py: ###⚠️ Minor: Duplicate Fallback Code `benchmarks/bfcl/reporting.py:406-421` ... - DOCUMENTATION_COMPLETE.md: ### ✅ What's Good - patchComponent tests now cover all 4 operations (set, pus... - reporting.py: ### ❌ CRITICAL: `printresults` calls async function without await `benchmarks... - reporting.py: ### `printresults` accesses non-existent attributes on `BFCLResult` High Seve... - reporting.py: ### Report output directory never created before writing files Medium Severit... - reporting.py: ### ❌ CRITICAL: `printresults` accesses non-existent attributes `benchmarks/b... - reporting.py: ### ❌ CRITICAL: `asyncio.run()` called from async context `benchmarks/bfcl/re...
|
@coderabbitai review |
|
Claude Code is working… I'll analyze this and get back to you. |
✅ Actions performedReview triggered.
|
PR Review: Database RefactorTasks:
|
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 3 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
| ) | ||
|
|
||
| # Generate report (avoiding asyncio.run() since this may be called from async context) | ||
| return reporter.generate_report(results) |
There was a problem hiding this comment.
Sync function returns unawaited async coroutine
High Severity
print_results is a synchronous function that returns reporter.generate_report(results), but generate_report is declared async def. This means print_results returns an unawaited coroutine object instead of actually executing the report generation. No files are written, no console summary is printed, and the coroutine is silently discarded at both call sites in __main__.py and run_benchmark.py. The old implementation was fully synchronous and worked correctly.
Additional Locations (1)
| ) | ||
|
|
||
| # Generate report (avoiding asyncio.run() since this may be called from async context) | ||
| return reporter.generate_report(results) |
There was a problem hiding this comment.
Reporter loses config context, ignores results parameter
Medium Severity
print_results creates BFCLReporter() with no config argument, so self.config becomes an empty dict. This means model_name defaults to 'default', LEADERBOARD_SCORES and baseline_scores are empty, and reports go to a hardcoded reports/ directory instead of using results.config.output_dir. The old code passed results.config to the reporter. Additionally, the results: BFCLBenchmarkResults parameter passed to generate_report is completely ignored inside that method.
Changes: - reporting.py: ### Verdict Approve with minor changes. The duplicate fallback code in report... - reporting.py: ### ❌ CRITICAL: `reporting.py` API Breaking Changes The `benchmarks/bfcl/repo... - reporting.py: ### Verdict Request changes. Fix the `reporting.py` API breaking changes befo... - reporting.py: ### ❌ CRITICAL: `reporting.py` API Breaking Change `benchmarks/bfcl/reporting... - reporting.py: ### Verdict Approve with minor changes. The core database refactor is solid. ... - reporting.py: ### ❌ CRITICAL: `reporting.py` Missing Import + Wrong Attributes `benchmarks/... - reporting.py: ### Verdict Request changes. Fix the `reporting.py` import and attribute acce... - reporting.py: ### ❌ CRITICAL: `reporting.py` API Breaking Changes - reporting.py: 1. Wrong attribute access in `printresults` (`reporting.py:44-47`): ```python... - reporting.py: ### Verdict Approve with minor changes. The core database refactor is solid. ... - reporting.py: ### ❌ CRITICAL: `reporting.py` API Breaking Change `runner.py:146` and `:485`... - reporting.py: ### Verdict Approve with minor changes. Fix the `reporting.py` async/argument... - reporting.py: ### ❌ CRITICAL: `reporting.py` Async/Argument Mismatch `runner.py:146` and `:... - reporting.py: ### Verdict Approve with minor changes. Fix the `reporting.py` async/argument...
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
PR Review: Database RefactorTasks:
|
Changes: - ast_evaluator.py: `isinstance` does not accept PEP-604 unions (`int | float`) as the second arg...
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
Claude Code is working… I'll analyze this and get back to you. |
Explains reasoning for dismissed issues inline in code
|
Claude finished @odilitime's task in 2m 6s —— View job PR Review: chore: the great database refactorSummaryBatch-first database API redesign with 14x performance improvements. Review verified test coverage, type safety, and addressed previous feedback. Previous Issues - Status
Remaining Issues❌ CRITICAL: Async/sync mismatch in
|
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 3 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
| ) | ||
|
|
||
| # Generate report (avoiding asyncio.run() since this may be called from async context) | ||
| return reporter.generate_report(results) |
There was a problem hiding this comment.
Sync function returns unawaited coroutine from async method
High Severity
print_results is a regular (sync) function that calls reporter.generate_report(results), which is declared async def. Without await, this returns an unawaited coroutine object instead of executing the report generation. No report files are written, no console summary is printed, and callers in run_benchmark.py and __main__.py silently discard the coroutine. The comment acknowledges the async context issue but doesn't solve it.
Additional Locations (1)
| "relevance_correct": r.relevance_correct, | ||
| "latency_ms": r.latency_ms, | ||
| "predicted_calls": [ | ||
| {"name": c.name, "arguments": c.arguments} |
There was a problem hiding this comment.
IndexError when sorted_results is empty with baseline scores
Medium Severity
generate_report accesses sorted_results[0] in the baseline comparison dict comprehension and the markdown generation without checking if the list is empty. If no results were added via add_result (or if self.results is empty), this raises an IndexError. Similarly, max(r[...] for r in sorted_results) on the console summary line will raise ValueError on an empty sequence.
Additional Locations (2)
| 'json': report_json, | ||
| 'paths': self.output_paths | ||
| # Note: Simplified reporting for faster integration; detailed features will be reintroduced later. | ||
| } |
There was a problem hiding this comment.
generate_report ignores its results parameter entirely
Medium Severity
generate_report accepts a BFCLBenchmarkResults parameter but never uses it — it only reads from self.results (populated via add_result). Any caller passing results directly to generate_report (as the old API contract expected) will get no output from those results. This is a silent behavioral regression from the previous implementation which used the parameter.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Iteration 1 prr-fix:prrc_kwdomt5cis6rw3sq prr-fix:prrc_kwdomt5cis6sbaxi prr-fix:prrc_kwdomt5cis6sbpck prr-fix:prrc_kwdomt5cis6sbpcm prr-fix:prrc_kwdomt5cis6sbtaj
Explains reasoning for dismissed issues inline in code
|
Claude Code is working… I'll analyze this and get back to you. |
|
Claude finished @odilitime's task in 2m 24s —— View job PR Review: chore: the great database refactor✅ Security Review
✅ Test Coverage
❌ Async Bug in Python Benchmarks
This causes the function to return a coroutine object instead of executing.
|
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
| ) | ||
|
|
||
| # Generate report (avoiding asyncio.run() since this may be called from async context) | ||
| return reporter.generate_report(results) |
There was a problem hiding this comment.
Async print_results called without await in callers
High Severity
print_results was changed from a synchronous function to async def, but its callers in __main__.py (line 252) and run_benchmark.py (line 69) still call it as print_results(results) without await. This creates a coroutine object that is never executed, so no benchmark results are printed and no reports are generated. The function silently does nothing.
Additional Locations (1)
| ) | ||
|
|
||
| # Generate report (avoiding asyncio.run() since this may be called from async context) | ||
| return reporter.generate_report(results) |
There was a problem hiding this comment.
Missing await on async generate_report inside print_results
High Severity
Inside print_results, the call return reporter.generate_report(results) invokes an async def method without await. This returns a coroutine object instead of executing generate_report, so even if print_results were properly awaited by callers, no report files would be written and no console summary would be printed.
built on #6496
db go zoom zoom
not done
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Refactor
Chores
Note
Medium Risk
Medium risk because it refactors BFCL benchmark reporting/output generation and adjusts evaluator/metrics logic, which may break benchmark CLIs and result formats. Most other changes are documentation and branding-only text updates.
Overview
Adds new documentation for the database API refactor, including a detailed changelog and benchmark results comparing old vs new batch-first CRUD performance.
Across benchmark packages, updates branding text from
ElizaOStoelizaOSand adjusts various READMEs/reports accordingly.Improves BFCL benchmark internals by tightening numeric parsing, switching a mock calculator from
evalto an AST-based safe evaluator, updating latency percentile calculation and error categorization, and heavily rewritingbenchmarks/bfcl/reporting.pyto generate timestamped leaderboard/markdown/JSON reports underreports/.Written by Cursor Bugbot for commit 18c8d9e. This will update automatically on new commits. Configure here.