Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR refactors filter and validator functions to receive structured context containing upstream dependency data, removes legacy API endpoints, and improves configuration management with YAML-based setup.
- Updated filter and validator signatures to accept
Ctxparameter with upstream dependencies - Removed client-side filter evaluation - all filtering now happens on backend with access to dependency data
- Removed legacy
eval_eventAPI in favor ofeval_flow_run - Replaced environment variable configuration with YAML-based config system supporting both development and production paths
Reviewed Changes
Copilot reviewed 22 out of 27 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| sdk-py/src/business_use/models.py | Added TypedDict definitions for DepData and Ctx to structure upstream dependency context |
| sdk-py/src/business_use/client.py | Updated function signatures to use new Ctx type and improved documentation with upstream dependency examples |
| sdk-py/src/business_use/batch.py | Removed client-side filter evaluation logic |
| sdk-py/README.md | Updated documentation to reflect context-aware filters/validators and new ensure() API |
| sdk-js/src/models.ts | Added TypeScript interfaces for DepData and Ctx |
| sdk-js/src/client.ts | Updated function signatures and examples to use Ctx type |
| sdk-js/src/batch.ts | Removed client-side filter evaluation and added null checks for string operations |
| sdk-js/README.md | Updated documentation with context-aware examples |
| core/src/migrations/init.py | Added docstring for migrations module |
| core/src/eval/eval.py | Removed legacy eval_event function |
| core/src/eval/init.py | Removed eval_event from public API exports |
| core/src/domain/types.py | Added DepData and Ctx TypedDict definitions |
| core/src/domain/evaluation.py | Refactored to build Ctx with all upstream dependencies and evaluate validators with proper context |
| core/src/config.py | Replaced env-based config with YAML file configuration supporting dev/prod paths |
| core/src/cli.py | Added init command, programmatic Alembic config, and database/API key validation |
| core/src/api/models.py | Removed legacy eval_event API parameters |
| core/src/api/api.py | Removed legacy eval_event endpoint implementation |
| core/pyproject.toml | Added pyyaml dependency and updated CLI script names |
| core/config.yaml.example | Added example configuration file |
| core/.gitignore | Added config.yaml to gitignore |
| CLAUDE.md | Updated documentation for new config system and CLI commands |
| .claude/commands/auto-instrument.md | Updated with new SDK installation and backend setup instructions |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| // Track closing delimiters (increase nesting when going backwards) | ||
| if (')]}}>'.includes(char)) { | ||
| if (char && ')]}}>'.includes(char)) { | ||
| nestingLevel++; |
There was a problem hiding this comment.
The null checks using char && are insufficient since body[i] can return an empty string '' which is falsy. The checks should explicitly compare against undefined to handle edge cases correctly: if (char !== undefined && ...). This prevents incorrect behavior when processing empty strings in the function body.
| body = body.substring(0, body.length - 1).trim(); | ||
| while (body.length > 0) { | ||
| const lastChar = body[body.length - 1]; | ||
| if (lastChar && ',);'.includes(lastChar)) { |
There was a problem hiding this comment.
The null check using lastChar && is insufficient since body[body.length - 1] can return an empty string '' which is falsy. The check should explicitly compare against undefined: if (lastChar !== undefined && ...). This prevents incorrect behavior when the last character is an empty string.
| if (lastChar && ',);'.includes(lastChar)) { | |
| if (lastChar !== undefined && ',);'.includes(lastChar)) { |
| ENV: str = _config.get("env", "local") | ||
| DEBUG: bool = _config.get("debug", False) | ||
|
|
||
| # API key - optional, validated when needed |
There was a problem hiding this comment.
[nitpick] The comment on line 56 states 'API key - optional, validated when needed' but the type annotation allows None. Consider documenting where and how this validation occurs (e.g., in the ensure_api_key_or_exit() function) to help developers understand the validation flow.
| # API key - optional, validated when needed | |
| # API key - optional, validated when needed. | |
| # Validation occurs in the ensure_api_key_or_exit() function. |
core/src/cli.py
Outdated
| config_data = { | ||
| "database_path": "./db.sqlite" if config_path == local_config else str(Path.home() / ".business-use" / "db.sqlite"), |
There was a problem hiding this comment.
[nitpick] This line exceeds typical line length limits and contains complex nested logic. Consider extracting the database path determination into a separate variable or helper function for better readability: db_path = './db.sqlite' if config_path == local_config else str(Path.home() / '.business-use' / 'db.sqlite') followed by 'database_path': db_path.
| config_data = { | |
| "database_path": "./db.sqlite" if config_path == local_config else str(Path.home() / ".business-use" / "db.sqlite"), | |
| db_path = "./db.sqlite" if config_path == local_config else str(Path.home() / ".business-use" / "db.sqlite") | |
| config_data = { | |
| "database_path": db_path, |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
No description provided.