Browser Skills Demo #3654
Browser Skills Demo #3654
Conversation
…fix/browser_version
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis PR introduces a comprehensive Browser Skills framework enabling CAMEL agents to orchestrate and replay complex browser automation workflows, complete with AI-powered subtask extraction and skill conversion. It also refines OpenAI/Azure model request handling and enhances browser toolkit snapshot logging for improved diagnostics. Changes
Sequence DiagramssequenceDiagram
participant User
participant SkillsAgent
participant ChatAgent
participant HybridBrowserToolkit
participant ActionReplayer
participant RecoveryAgent
User->>SkillsAgent: run(user_task)
SkillsAgent->>SkillsAgent: initialize (load skills, create tools)
SkillsAgent->>ChatAgent: run with task and subtask tools
ChatAgent->>ChatAgent: select subtask or browser action
alt Subtask Selected
ChatAgent->>ActionReplayer: execute subtask
ActionReplayer->>HybridBrowserToolkit: replay actions from config
HybridBrowserToolkit->>HybridBrowserToolkit: perform actions
alt Action Fails
ActionReplayer->>RecoveryAgent: agent_recovery(failed_action)
RecoveryAgent->>RecoveryAgent: determine correct element ref
ActionReplayer->>HybridBrowserToolkit: retry with corrected ref
end
ActionReplayer-->>ChatAgent: subtask result
else Browser Action Selected
ChatAgent->>HybridBrowserToolkit: execute native browser tool
HybridBrowserToolkit-->>ChatAgent: tool result with snapshot
end
ChatAgent->>SkillsAgent: log interactions and token usage
SkillsAgent->>User: final agent response
sequenceDiagram
participant TaskRunner as WebVoyagerRunner
participant Agent as SkillsAgent
participant Browser as HybridBrowserToolkit
participant Verifier as TaskVerifier
participant Analyzer as SubtaskExtractor
TaskRunner->>TaskRunner: load tasks from JSONL
loop Each Task
TaskRunner->>Agent: initialize with task
Agent->>Browser: connect and setup
Agent->>Agent: run task with optional prior suggestions
Agent->>Browser: execute actions/subtasks
Browser-->>Agent: action results and logs
Agent->>TaskRunner: agent response
TaskRunner->>Verifier: verify_task(task_description, response)
Verifier-->>TaskRunner: {success, reasoning, suggestions}
alt Verification Failed
TaskRunner->>TaskRunner: retry with suggestions (max retries)
else Verification Success
opt Analysis Enabled
TaskRunner->>Analyzer: analyze_with_agent(session)
Analyzer-->>TaskRunner: subtask_analysis
end
end
TaskRunner->>TaskRunner: save results and persist
TaskRunner->>Browser: close browser
end
TaskRunner->>TaskRunner: print summary with success rates
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes The PR spans multiple systems (models, browser toolkit, framework), introduces substantial new infrastructure (~3500+ lines in browser skills framework alone), and requires verification of intricate orchestration logic including recovery handling, action replay, and AI-driven analysis. While many files follow consistent patterns, the heterogeneity of concerns (tool cleanup, snapshot logging, skill management, task verification, action replay) and integration complexity demand careful review. Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 18
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (9)
camel/models/azure_openai_model.py-421-424 (1)
421-424: Inconsistent condition check for tools parameter.These parse-related methods use
if tools is not None:while the chat completion methods useif tools:. This inconsistency could cause different behavior whentoolsis an empty list[]:
- Empty list with
if tools:→False(correctly removesparallel_tool_calls) ✓- Empty list with
if tools is not None:→True(adds empty list to config, keepsparallel_tool_calls) ✗The falsy check is semantically more correct since an empty tools list should be treated the same as
None.🔎 Proposed fix for consistency
- if tools is not None: + if tools: request_config["tools"] = tools else: # Remove parallel_tool_calls if no tools are specified # as OpenAI API only allows it when tools are present request_config.pop("parallel_tool_calls", None)Apply this pattern to all four parse methods:
_request_parse,_arequest_parse,_request_stream_parse, and_arequest_stream_parse.Also applies to: 446-449, 475-478, 506-509
camel/models/openai_model.py-482-485 (1)
482-485: Inconsistent condition check for tools parameter.These parse-related methods use
if tools is not None:while the chat completion methods useif tools:. This is the same inconsistency present inazure_openai_model.pyand could cause different behavior whentoolsis an empty list[]:
- Empty list with
if tools:→False(correctly removesparallel_tool_calls) ✓- Empty list with
if tools is not None:→True(adds empty list to config, keepsparallel_tool_calls) ✗The falsy check is semantically more correct since an empty tools list should be treated the same as
None.🔎 Proposed fix for consistency
- if tools is not None: + if tools: request_config["tools"] = tools else: # Remove parallel_tool_calls if no tools are specified # as OpenAI API only allows it when tools are present request_config.pop("parallel_tool_calls", None)Apply this pattern to all four parse methods:
_request_parse,_arequest_parse,_request_stream_parse, and_arequest_stream_parse.Also applies to: 511-514, 543-546, 577-580
camel/models/azure_openai_model.py-376-379 (1)
376-379: Address the inconsistency in tool condition checks across methods.The code at lines 376-379 correctly removes
parallel_tool_callswhen no tools are provided, but uses a falsy checkif tools:. However, the parse-related methods (_request_parse,_arequest_parse,_request_stream_parse,_arequest_stream_parse) useif tools is not None:instead. This creates a semantic difference: an empty list[]would be treated as "no tools" in chat completion methods but as valid tools in parse methods. Align the condition checks across all methods for consistency.examples/toolkits/hybrid_browser_toolkit_example.py-110-195 (1)
110-195: Hardensave_agent_logand response handling around optional fieldsThe logging helper is very useful, but there are a couple of assumptions worth guarding:
response.msgs[0].contentis accessed both insave_agent_logand inmain()without checking thatresponse.msgsis non‑empty.response.infois assumed to be a mapping with optional'usage','num_tokens','termination_reasons','tool_calls'.To avoid rare crashes when the agent returns no messages or an unexpected info shape, consider:
Using a small helper like:
final_msg = response.msgs[0].content if getattr(response, "msgs", []) else ""and reusing it both for printing and for
final_response.Accessing
response.infoviagetattr(response, "info", {})before.get(...)calls, to be fully defensive.These tweaks keep the behavior the same in the common case but make the example more robust.
Also applies to: 197-219
examples/toolkits/subtask_toolkit/run_webvoyager_tasks.py-242-250 (1)
242-250: Remove or usefinal_snapshotvariable.The
final_snapshotvariable is assigned but never used (line 248). Either remove it or use it in the result dictionary if the snapshot data is needed.🔎 Proposed fix
If not needed, remove lines 241-250. If needed, add it to the result:
result = { 'task_id': task_id, 'task_description': task_description, 'attempt': attempt, 'success': verification['success'], 'reasoning': verification['reasoning'], 'suggestions': verification.get('suggestions', ''), 'session_dir': str(session_dir) if session_dir else None, + 'final_snapshot': final_snapshot, }Committable suggestion skipped: line range outside the PR's diff.
examples/toolkits/subtask_toolkit/subtask_agent_example.py-349-374 (1)
349-374: Duplicate initialization ofcurrent_user_task.The attribute
current_user_taskis initialized toNoneat both line 350 and line 374. Remove the duplicate.🔎 Proposed fix
Remove lines 373-374:
- # Store current user task (actual task being executed) - self.current_user_task = Noneexamples/toolkits/subtask_toolkit/run_webvoyager_tasks.py-524-530 (1)
524-530: Guard against division by zero.If
totalis 0 (no tasks run), lines 529-530 will raiseZeroDivisionError.🔎 Proposed fix
total = len(self.results) succeeded = sum(1 for r in self.results if r.get('success')) failed = total - succeeded print(f"Total tasks: {total}") - print(f"Succeeded: {succeeded} ({succeeded/total*100:.1f}%)") - print(f"Failed: {failed} ({failed/total*100:.1f}%)") + if total > 0: + print(f"Succeeded: {succeeded} ({succeeded/total*100:.1f}%)") + print(f"Failed: {failed} ({failed/total*100:.1f}%)") + else: + print("No tasks were executed.")examples/toolkits/subtask_toolkit/analyze_subtask_candidate.py-78-79 (1)
78-79: Remove unused variableexisting_subtasks_str.This variable is assigned but never used, as flagged by static analysis. Remove it to clean up the code.
🔎 Proposed fix
- # Format existing subtasks - Full JSON - existing_subtasks_str = json.dumps(existing_subtasks, indent=2) - # Format existing subtasks - Summary list for easy comparisonexamples/toolkits/subtask_toolkit/subtask_agent_example.py-286-292 (1)
286-292: Useless if-else condition returns the same value.The conditional at lines 288-290 always returns
"string"regardless of the condition. This suggests incomplete logic for handling different types.🔎 Proposed fix
If integer types should be mapped to "integer":
parameters["properties"][var_name] = { - "type": "string" - if var_config['type'] in ['string', 'date'] - else "string", + "type": "integer" if var_config['type'] == 'number' else "string", "description": var_config['description'], }Or if all types should be strings for the OpenAI function schema, simplify to:
parameters["properties"][var_name] = { - "type": "string" - if var_config['type'] in ['string', 'date'] - else "string", + "type": "string", "description": var_config['description'], }
🧹 Nitpick comments (28)
examples/toolkits/subtask_toolkit/extract_individual_actions.py (2)
24-30: Add error handling for JSON parsing.The
json.load()call can raiseJSONDecodeErrorif the file contains invalid JSON. Consider adding a try-except block to provide a clearer error message.🔎 Proposed error handling
if not json_file.exists(): print(f"Error: File not found: {json_file}") return [] # Load the JSON file - with open(json_file, 'r', encoding='utf-8') as f: - data = json.load(f) + try: + with open(json_file, 'r', encoding='utf-8') as f: + data = json.load(f) + except json.JSONDecodeError as e: + print(f"Error: Invalid JSON in {json_file}: {e}") + return []
25-25: Consider using stderr for error messages.Error messages in CLI scripts should typically go to
sys.stderrrather thanstdoutto avoid polluting standard output.🔎 Proposed change
- print(f"Error: File not found: {json_file}") + print(f"Error: File not found: {json_file}", file=sys.stderr)Apply the same pattern to any other error messages in the file.
examples/toolkits/subtask_toolkit/analyze_log_and_subtasks.py (5)
88-135: Consider tracking parse error count.While the JSONDecodeError handling on line 131 prevents crashes, silently continuing may hide issues if multiple objects fail to parse. Consider accumulating a count of parse failures and reporting it at the end to alert users about potential data loss.
🔎 Proposed enhancement
def load_main_log(log_file: str) -> List[Dict[str, Any]]: """Load and parse the main browser log file.""" actions = [] + parse_errors = 0 print(f"Loading main log file: {log_file}") with open(log_file, 'r', encoding='utf-8') as f: @@ -130,10 +131,12 @@ try: action = json.loads(current_obj.strip()) actions.append(action) current_obj = "" except json.JSONDecodeError as e: print(f"Warning: Failed to parse JSON object: {e}") + parse_errors += 1 current_obj = "" print(f"✓ Loaded {len(actions)} actions from main log") + if parse_errors > 0: + print(f"⚠️ Warning: {parse_errors} JSON objects failed to parse") return actions
144-158: Consider inlining trivial equality check.The function simply wraps
timing1 == timing2with a null check. While the explicit function name documents intent, this could be inlined at call sites for simplicity. If you prefer keeping it for clarity, consider adding a note in the docstring explaining why a dedicated function is valuable here.
310-327: Extract nested function to module level.The
is_action_failedfunction is nested insidemain(), limiting reusability. Extract it to module level to improve testability and potential reuse by other functions.🔎 Proposed refactor
Move the function to module level (after the other utility functions):
+def is_action_failed(action: Dict[str, Any]) -> bool: + """Check if an action failed.""" + outputs = action.get('outputs', {}) + + # Check if outputs contains a failure indicator + if isinstance(outputs, dict): + result = outputs.get('result', '') + if isinstance(result, str): + # Check for failure patterns + if any( + pattern in result.lower() + for pattern in ['failed', 'error', 'not found'] + ): + # Exclude "executed successfully" which might contain "error" in context + if 'successfully' not in result.lower(): + return True + + return False + + def main(): # File paths main_log_file = ... - # Check if action failed - def is_action_failed(action: Dict[str, Any]) -> bool: - """Check if an action failed.""" - # ... (remove from here)
197-491: Decompose the 295-linemain()function.The
main()function is too long and handles multiple responsibilities (loading, filtering, timeline building, output, reporting). Refactor it into smaller, focused functions to improve readability, testability, and maintainability.🔎 Suggested decomposition
Consider breaking
main()into these focused functions:def load_all_logs(main_log_file: str, subtask_files: List[str]) -> Tuple[List[Dict], List[Dict]]: """Load and parse all log files.""" # Lines 216-246 ... def filter_non_subtask_actions(main_actions: List[Dict], all_subtask_timings: Set) -> List[Dict]: """Filter actions not present in subtasks.""" # Lines 249-261 ... def build_timeline(main_actions: List[Dict], subtasks: List[Dict], excluded_actions: Set) -> List[Dict]: """Generate chronological timeline with all actions.""" # Lines 273-437 ... def load_task_description(main_log_file: str) -> str: """Load task description from config file.""" # Lines 439-455 ... def save_results(non_subtask_actions: List[Dict], timeline: Dict, output_dir: str): """Save analysis results to JSON files.""" # Lines 265-270, 458-468 ... def print_analysis_summary(main_actions: List[Dict], non_subtask_actions: List[Dict], timeline: List[Dict], already_added_subtasks: Set, output_file: str, timeline_file: str): """Print analysis summary.""" # Lines 470-487 ... def main(): """Main entry point.""" # Orchestrate the above functions ...Each function would have a single, clear responsibility and be easier to test and maintain.
Based on coding guidelines: Google Python style guide recommends functions should be focused and not overly long.
370-388: Extract duplicated element label extraction logic.The element label extraction logic (getting snapshot, iterating args, extracting label) is duplicated at lines 370-388 and 420-426. Extract this into a helper function to reduce duplication.
🔎 Proposed refactor
def extract_element_label_from_action( action: Dict[str, Any], main_actions: List[Dict], action_index: int ) -> Optional[str]: """Extract element label from action's first ref argument.""" inputs = action.get('inputs', {}) args = inputs.get('args', []) if not args: return None snapshot_before = get_latest_snapshot_before_action( main_actions, action_index ) for arg in args: if isinstance(arg, str) and re.match(r'^e\d+$', arg): element_label = extract_aria_label_from_snapshot( snapshot_before, arg ) if element_label: return element_label return NoneThen replace both occurrences with:
element_label = extract_element_label_from_action( action, main_actions, action_index )Also applies to: 420-426
camel/toolkits/hybrid_browser_toolkit/ws_wrapper.py (1)
555-576: Snapshot logging path is correct; consider formalizing the private fieldThe new pattern of:
- Wrapping
get_som_screenshotoutputs inToolResultand- Attaching
response['snapshot']as_snapshot_for_loggingand picking it up in_log_actionforget_som_screenshotcleanly keeps snapshots out of the agent surface while still logging them. Functionality and JSON‑serializability both look fine.
If this pattern is going to be reused, consider documenting or formalizing
_snapshot_for_loggingonToolResult(e.g., via a documented optional attribute or a small subclass) so it’s less “magic” than setting an ad‑hoc attribute.Also applies to: 568-574, 787-804
examples/toolkits/hybrid_browser_toolkit_example.py (1)
102-107: Chinese punctuation vs Ruff RUF001The Chinese guidance in
TASK_PROMPTcorrectly uses the fullwidth comma，. Ruff’s RUF001 warning (“ambiguous comma”) is expected here and can be safely ignored, or silenced via# noqa: RUF001on that line if it becomes noisy.No functional change needed.
examples/toolkits/subtask_toolkit/analyze_session.py (2)
514-526: Narrow the exception when readingagent_communication_log.jsonCatching bare
Exceptionhere makes it easy to hide unrelated bugs (e.g., programming errors) behind a generic warning:if agent_comm_log_file.exists(): try: ... except Exception as e: print(f"⚠️ Warning: Could not read task description from agent log: {e}")Given this is just reading a JSON file, you can safely narrow to I/O and JSON‑decode errors, e.g.:
from json import JSONDecodeError from OSError import OSError # or just use OSError ... except (OSError, JSONDecodeError) as e: ...This aligns better with Google Python style and avoids swallowing unexpected exceptions.
17-88: Consider de‑duplicating snapshot/log parsing helpers withanalyze_log_and_subtasks.pyFunctions like:
extract_aria_label_from_snapshotload_main_logget_latest_snapshot_before_actionis_action_failedget_current_url_from_tab_infoare essentially copies of the implementations already present in
examples/toolkits/subtask_toolkit/analyze_log_and_subtasks.py.To avoid divergence over time (bugfixes applied in one file but not the other), consider moving these into a shared utility module (e.g.,
snapshot_utils.pyor similar) and importing them here and inanalyze_log_and_subtasks.py.Also applies to: 90-137, 146-179, 182-227, 229-258
examples/toolkits/subtask_toolkit/replay_from_log.py (2)
91-104: Broadexcept Exceptionblocks are acceptable for a CLI, but consider narrowing in hot pathsThere are several places where you catch a blanket
Exception:
get_cdp_url(CDP probe)replay_actionouter try/except- Retry block around
replay_actionexceptions- The
__main__guard aroundasyncio.run(main())Given this is a CLI tool and you usually print a traceback, this is not critical. Still, for better debuggability and style:
- In narrow contexts (like
get_cdp_url), consider catching concrete exceptions (e.g.,URLError,OSError,JSONDecodeError) instead of a blanketException.- In
__main__, keeping a broad catch with traceback +sys.exit(1)is reasonable; I’d leave that as‑is.No behavior change is strictly required, but tightening a few of these would align more closely with Google Python style and Ruff BLE001 guidance.
Also applies to: 1278-1283, 1404-1406, 1829-1838
27-31: Sys.path manipulation is pragmatic here but brittleThe manual path hack:
script_dir = Path(__file__).resolve().parent project_root = script_dir.parent.parent.parent sys.path.insert(0, str(project_root))works for this repo layout but will break if directories are reorganized.
If this script is going to live longer‑term, consider either:
- Installing
camelin your environment and importing it normally, or- Treating this as a module and running via
python -m examples.toolkits.subtask_toolkit.replay_from_log.For a local/dev‑only tool this is acceptable, but it’s worth noting the brittleness.
camel/toolkits/hybrid_browser_toolkit/ts/src/browser-session.ts (1)
25-34: StrengthengetSnapshotto validate_snapshotForAIresult shapeThe compatibility shim is well-intentioned (third-party wrappers around
_snapshotForAI()may return{full: string}while Playwright's native method returns a string), but the current implementation risks returningundefinedif the result is an object without a.fullproperty.This violates the
Promise<string>return type and will cause downstream failures—for instance, at lines 460–465 the snapshot is used withrefPattern.exec(snapshotText), and throughout the file it's passed togetSnapshotDiff(), both of which expect a string.Strengthen the implementation to validate the shape:
Suggested fix
private async getSnapshot(page: Page): Promise<string> { const result = await (page as any)._snapshotForAI(); if (typeof result === 'string') { return result; } if (result && typeof result.full === 'string') { return result.full; } throw new Error(`Unexpected _snapshotForAI() result shape: ${typeof result}`); }This ensures the function either returns a string or fails fast, preventing
undefinedfrom propagating into the rest of the pipeline.camel/toolkits/hybrid_browser_toolkit/ts/src/hybrid-browser-toolkit.ts (1)
192-225: Consider reflecting snapshot in the type signature to improve code clarityAdding the snapshot field via:
(result as any).snapshot = snapshotResult.snapshot;works correctly and the Python side successfully receives this field for logging. To avoid the
as anycast and make the snapshot field explicit to future maintainers, consider:
- Extending
VisualMarkResultwith an optionalsnapshot?: stringfield, or- Updating the return type of
getSomScreenshottoVisualMarkResult & { timing: any; snapshot?: string }.This change is backward-compatible: the only TypeScript caller (websocket-server.js) performs no strict type checking and simply returns the result as JSON.
examples/toolkits/subtask_toolkit/subtask_manager.py (5)
62-68: Mutating loaded subtask dicts may cause unintended side effects.Adding
_source_filedirectly to the subtask dict modifies the object stored inself.file_subtasks. This is fine for internal use since fields starting with_are cleaned before saving, but consider documenting this behavior or using a copy if the original data should remain pristine.The broad
Exceptioncatch (Line 67) is acceptable here since individual file load failures shouldn't prevent loading other files, though you could narrow it to(json.JSONDecodeError, IOError, OSError)for more precision.
86-93: Moveimport reto module level.Importing
reinside the loop is inefficient and violates Google Python style guide which recommends imports at the top of the file. The import is executed on every iteration wherecurrent_idis a string.🔎 Proposed fix
Add at the top of the file (after line 12):
import reThen remove line 88:
if isinstance(current_id, str): # Try to extract leading number - import re - match = re.match(r'^(\d+)', current_id)
131-135: Moveimport shutilto module level.Per Google Python style guide, imports should be at the top of the file rather than inside functions.
🔎 Proposed fix
Add at the top of the file (after line 12):
import shutilThen update lines 133-135:
if backup and file_path.exists(): backup_path = file_path.with_suffix('.json.backup') - import shutil - shutil.copy2(file_path, backup_path)
194-203: Moveimport reto module level (already noted above).This is the same issue as in
get_next_numeric_id- thereimport at line 196 should be at module level.
331-341: Moveimport systo module level.The
sysmodule is imported inside theif __name__ == "__main__"block, but it should be at the top of the file with other imports for consistency with Google Python style guide.🔎 Proposed fix
Add at the top of the file (after line 12):
import sysThen update lines 331-332:
if __name__ == "__main__": - import sys - if len(sys.argv) < 2:examples/toolkits/subtask_toolkit/analyze_subtask_candidate.py (4)
27-59: Consider reusing SubtaskManager instead of duplicating logic.This function duplicates the subtask loading logic from
SubtaskManager.load_all_subtasks(). SinceSubtaskManageris already imported and used later in the file (line 582), consider using it here as well for consistency:def load_all_existing_subtasks(subtask_configs_dir: str) -> List[Dict[str, Any]]: manager = SubtaskManager(subtask_configs_dir) return manager.load_all_subtasks()This reduces code duplication and ensures consistent behavior.
233-245: Use ASCII characters in comments and move import to module level.The comments contain fullwidth Chinese punctuation (，：) which may cause issues in some editors. Consider using standard ASCII punctuation for consistency. Also, the docstring examples at lines 278-281 have incorrect indentation.
248-264: Moveimport reto module level.The
remodule is imported inside the function (line 253). Per Google Python style guide, imports should be at the top of the file.
387-434: Moveimport reto module level.The
reimport at line 401 should be at module level with other imports.examples/toolkits/subtask_toolkit/run_webvoyager_tasks.py (2)
99-120: Moveimport reto module level and consider narrowing exception handling.The
reimport at line 102 should be at module level. The broadExceptioncatch at line 113 is acceptable for robustness when parsing LLM responses, but consider logging the exception type for debugging.
335-343: Moveimport tracebackto module level.The
tracebackmodule is imported inside exception handlers at multiple places (lines 337, 349, 363). Move it to the top of the file with other imports.examples/toolkits/subtask_toolkit/subtask_agent_example.py (2)
79-80: Move imports to module level.The imports of
datetimeandPathat lines 79-80 are inside theexecutemethod. These should be at module level.Pathis already imported at line 18.🔎 Proposed fix
Add
import datetimeat the top of the file (after line 16), then remove lines 79-80.
714-721: Consider safer alternatives toexecfor dynamic function creation.Using
exec(line 717) to create dynamic functions is a security concern and makes the code harder to debug. While the input here is controlled, consider usingfunctools.partialwith a generic async handler, or creating wrapper classes that implement__call__.🔎 Alternative approach sketch
import functools async def _execute_subtask(subtask_func, agent_ref, **kwargs): """Generic handler for subtask execution.""" import json import datetime call_log = { 'timestamp': datetime.datetime.now().isoformat(), 'type': 'subtask_call', 'subtask_id': subtask_func.subtask_id, 'subtask_name': subtask_func.name, 'arguments': kwargs, 'result': None } result = await subtask_func.execute(**kwargs) call_log['result'] = result if hasattr(agent_ref, 'agent_communication_log'): agent_ref.agent_communication_log.append(call_log) return json.dumps(result, ensure_ascii=False) # Then create the wrapper: wrapper = functools.partial(_execute_subtask, subtask_func, self) wrapper.__name__ = f"subtask_{subtask_func.subtask_id}" wrapper.__doc__ = subtask_func.descriptionNote: This would require adjusting how the ChatAgent handles the function signature, as
functools.partialmay not expose the expected parameter names.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (5)
examples/toolkits/subtask_configs/1_subtasks.jsonis excluded by!**/*.jsonexamples/toolkits/subtask_configs/2_subtasks.jsonis excluded by!**/*.jsonexamples/toolkits/subtask_configs/3_subtasks.jsonis excluded by!**/*.jsonexamples/toolkits/subtask_configs/4_subtasks.jsonis excluded by!**/*.jsonexamples/toolkits/subtask_configs/5_subtasks.jsonis excluded by!**/*.json
📒 Files selected for processing (15)
camel/models/azure_openai_model.pycamel/models/openai_model.pycamel/toolkits/hybrid_browser_toolkit/ts/src/browser-session.tscamel/toolkits/hybrid_browser_toolkit/ts/src/hybrid-browser-toolkit.tscamel/toolkits/hybrid_browser_toolkit/ws_wrapper.pyexamples/toolkits/hybrid_browser_toolkit_example.pyexamples/toolkits/subtask_toolkit/__init__.pyexamples/toolkits/subtask_toolkit/analyze_log_and_subtasks.pyexamples/toolkits/subtask_toolkit/analyze_session.pyexamples/toolkits/subtask_toolkit/analyze_subtask_candidate.pyexamples/toolkits/subtask_toolkit/extract_individual_actions.pyexamples/toolkits/subtask_toolkit/replay_from_log.pyexamples/toolkits/subtask_toolkit/run_webvoyager_tasks.pyexamples/toolkits/subtask_toolkit/subtask_agent_example.pyexamples/toolkits/subtask_toolkit/subtask_manager.py
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
⚙️ CodeRabbit configuration file
Review the python code against the google python style guide and point out any mismatches.
Files:
examples/toolkits/subtask_toolkit/extract_individual_actions.pycamel/models/openai_model.pyexamples/toolkits/subtask_toolkit/__init__.pycamel/toolkits/hybrid_browser_toolkit/ws_wrapper.pycamel/models/azure_openai_model.pyexamples/toolkits/subtask_toolkit/subtask_manager.pyexamples/toolkits/subtask_toolkit/analyze_log_and_subtasks.pyexamples/toolkits/hybrid_browser_toolkit_example.pyexamples/toolkits/subtask_toolkit/analyze_subtask_candidate.pyexamples/toolkits/subtask_toolkit/analyze_session.pyexamples/toolkits/subtask_toolkit/run_webvoyager_tasks.pyexamples/toolkits/subtask_toolkit/replay_from_log.pyexamples/toolkits/subtask_toolkit/subtask_agent_example.py
🧬 Code graph analysis (8)
camel/toolkits/hybrid_browser_toolkit/ws_wrapper.py (1)
camel/utils/tool_result.py (1)
ToolResult(18-44)
examples/toolkits/subtask_toolkit/analyze_log_and_subtasks.py (1)
examples/toolkits/subtask_toolkit/replay_from_log.py (2)
extract_aria_label_from_snapshot(327-395)get_latest_snapshot_before_action(577-599)
examples/toolkits/hybrid_browser_toolkit_example.py (2)
camel/types/enums.py (2)
ModelPlatformType(1785-2024)ModelType(33-1601)camel/toolkits/hybrid_browser_toolkit_py/hybrid_browser_toolkit.py (1)
get_tools(2338-2390)
examples/toolkits/subtask_toolkit/analyze_subtask_candidate.py (2)
examples/toolkits/subtask_toolkit/extract_individual_actions.py (1)
extract_consecutive_individual_actions(13-51)examples/toolkits/subtask_toolkit/subtask_manager.py (2)
load_all_subtasks(38-70)add_new_subtasks(164-244)
examples/toolkits/subtask_toolkit/analyze_session.py (1)
examples/toolkits/subtask_toolkit/analyze_log_and_subtasks.py (6)
extract_aria_label_from_snapshot(15-85)load_main_log(88-135)load_subtask_log(138-141)get_latest_snapshot_before_action(161-194)is_action_failed(310-327)main(197-487)
examples/toolkits/subtask_toolkit/run_webvoyager_tasks.py (3)
examples/toolkits/subtask_toolkit/subtask_agent_example.py (4)
SubtaskAgent(307-1397)initialize(441-769)run(868-979)main(1400-1439)examples/toolkits/subtask_toolkit/analyze_subtask_candidate.py (2)
analyze_with_agent(437-682)main(685-707)examples/toolkits/subtask_toolkit/analyze_session.py (1)
main(560-571)
examples/toolkits/subtask_toolkit/replay_from_log.py (1)
examples/toolkits/subtask_toolkit/analyze_log_and_subtasks.py (3)
extract_aria_label_from_snapshot(15-85)get_latest_snapshot_before_action(161-194)main(197-487)
examples/toolkits/subtask_toolkit/subtask_agent_example.py (1)
examples/toolkits/subtask_toolkit/replay_from_log.py (4)
load_subtask_config(105-230)replay_subtask(1428-1535)load_log_file(232-284)replay_action(784-1408)
🪛 Ruff (0.14.10)
examples/toolkits/subtask_toolkit/subtask_manager.py
67-67: Do not catch blind exception: Exception
(BLE001)
examples/toolkits/subtask_toolkit/analyze_log_and_subtasks.py
454-454: Do not catch blind exception: Exception
(BLE001)
examples/toolkits/hybrid_browser_toolkit_example.py
106-106: String contains ambiguous ， (FULLWIDTH COMMA). Did you mean , (COMMA)?
(RUF001)
106-106: String contains ambiguous ， (FULLWIDTH COMMA). Did you mean , (COMMA)?
(RUF001)
218-218: Local variable log_file is assigned to but never used
Remove assignment to unused variable log_file
(F841)
examples/toolkits/subtask_toolkit/analyze_subtask_candidate.py
56-56: Do not catch blind exception: Exception
(BLE001)
79-79: Local variable existing_subtasks_str is assigned to but never used
Remove assignment to unused variable existing_subtasks_str
(F841)
233-233: Comment contains ambiguous ， (FULLWIDTH COMMA). Did you mean , (COMMA)?
(RUF003)
233-233: Comment contains ambiguous ， (FULLWIDTH COMMA). Did you mean , (COMMA)?
(RUF003)
234-234: Comment contains ambiguous ， (FULLWIDTH COMMA). Did you mean , (COMMA)?
(RUF003)
238-238: Comment contains ambiguous ： (FULLWIDTH COLON). Did you mean : (COLON)?
(RUF003)
241-241: Comment contains ambiguous ： (FULLWIDTH COLON). Did you mean : (COLON)?
(RUF003)
286-286: Comment contains ambiguous ， (FULLWIDTH COMMA). Did you mean , (COMMA)?
(RUF003)
296-296: Docstring contains ambiguous ’ (RIGHT SINGLE QUOTATION MARK). Did you mean ``` (GRAVE ACCENT)?
(RUF002)
examples/toolkits/subtask_toolkit/analyze_session.py
522-522: Do not catch blind exception: Exception
(BLE001)
examples/toolkits/subtask_toolkit/run_webvoyager_tasks.py
112-112: Consider moving this statement to an else block
(TRY300)
113-113: Do not catch blind exception: Exception
(BLE001)
166-168: Avoid specifying long messages outside the exception class
(TRY003)
248-248: Local variable final_snapshot is assigned to but never used
Remove assignment to unused variable final_snapshot
(F841)
249-249: Do not catch blind exception: Exception
(BLE001)
258-258: Do not catch blind exception: Exception
(BLE001)
335-335: Do not catch blind exception: Exception
(BLE001)
345-345: Consider moving this statement to an else block
(TRY300)
361-361: Do not catch blind exception: Exception
(BLE001)
622-622: Do not catch blind exception: Exception
(BLE001)
examples/toolkits/subtask_toolkit/replay_from_log.py
99-99: Do not catch blind exception: Exception
(BLE001)
230-230: Avoid specifying long messages outside the exception class
(TRY003)
571-571: Avoid specifying long messages outside the exception class
(TRY003)
797-797: Avoid specifying long messages outside the exception class
(TRY003)
1276-1276: Consider moving this statement to an else block
(TRY300)
1278-1278: Do not catch blind exception: Exception
(BLE001)
1404-1404: Do not catch blind exception: Exception
(BLE001)
1438-1438: Avoid specifying long messages outside the exception class
(TRY003)
1441-1441: Avoid specifying long messages outside the exception class
(TRY003)
1834-1834: Do not catch blind exception: Exception
(BLE001)
examples/toolkits/subtask_toolkit/subtask_agent_example.py
255-255: Consider moving this statement to an else block
(TRY300)
257-257: Do not catch blind exception: Exception
(BLE001)
288-290: Useless if-else condition
(RUF034)
392-394: Avoid specifying long messages outside the exception class
(TRY003)
429-429: Avoid specifying long messages outside the exception class
(TRY003)
474-474: Do not catch blind exception: Exception
(BLE001)
569-569: String contains ambiguous ℹ (INFORMATION SOURCE). Did you mean i (LATIN SMALL LETTER I)?
(RUF001)
575-575: String contains ambiguous ℹ (INFORMATION SOURCE). Did you mean i (LATIN SMALL LETTER I)?
(RUF001)
717-717: Use of exec detected
(S102)
1059-1059: Do not catch blind exception: Exception
(BLE001)
1098-1098: Do not catch blind exception: Exception
(BLE001)
1393-1393: Do not catch blind exception: Exception
(BLE001)
1447-1447: Do not catch blind exception: Exception
(BLE001)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: pytest_package_llm_test
- GitHub Check: pre-commit
- GitHub Check: pytest_package_fast_test
- GitHub Check: pytest_examples
🔇 Additional comments (16)
examples/toolkits/subtask_toolkit/extract_individual_actions.py (2)
1-11: LGTM!The module docstring clearly describes the script's purpose, and imports are properly organized per Google Python style guide.
89-90: LGTM!Standard Python idiom for CLI script execution.
camel/models/openai_model.py (1)
429-432: LGTM with a consistency note.The logic correctly removes
parallel_tool_callswhen tools are not provided, using the falsy checkif tools:which appropriately handles bothNoneand empty list cases.However, the parse-related methods use
if tools is not None:instead ofif tools:, creating the same inconsistency present inazure_openai_model.py. See the separate comment on those methods.Also applies to: 453-456
examples/toolkits/subtask_toolkit/__init__.py (1)
1-11: Package metadata and docstring look goodDocstring and
__version__definition are clear and consistent; no changes needed.examples/toolkits/subtask_toolkit/subtask_manager.py (5)
1-14: LGTM!Module docstring and imports are clean and follow Google Python style conventions.
16-36: LGTM!The class initialization is clean. The constructor properly initializes the config directory and data structures, and the use of
mkdir(parents=True, exist_ok=True)is appropriate for ensuring the directory exists.
99-118: LGTM!The renumbering logic is straightforward and correctly processes files in sorted order to ensure deterministic ID assignment.
246-268: LGTM!The summary method provides a clean interface for retrieving statistics about loaded subtasks.
271-328: LGTM!The
renumber_existing_subtasksfunction provides clear user feedback and properly orchestrates the renumbering workflow.examples/toolkits/subtask_toolkit/analyze_subtask_candidate.py (2)
1-25: LGTM!Imports and module setup are clean. Using
load_dotenv()at module level is appropriate for this example script.
292-384: LGTM!The
enrich_subtask_definitionsfunction is well-structured and correctly enriches the agent's simplified output with auto-generated fields likearg_position,type, anddefault_value.examples/toolkits/subtask_toolkit/run_webvoyager_tasks.py (3)
1-30: LGTM!Imports and module setup are clean. The sys.path manipulation (line 29) is acceptable for this example script to enable local imports.
123-178: LGTM!The
WebVoyagerRunnerinitialization and helper methods are well-structured with proper directory validation.
374-457: LGTM!The retry logic is well-implemented with proper handling of timeouts, suggestions-based retries, and comprehensive attempt history tracking.
examples/toolkits/subtask_toolkit/subtask_agent_example.py (2)
1-35: LGTM!Imports and module setup are clean with appropriate path manipulation for the example script.
1400-1451: LGTM with note on path consistency.The
main()function correctly uses a relative path forsubtask_config_dir(line 1403). However, this is inconsistent with the hardcoded absolute paths used elsewhere in the file. Consider updating all paths to be relative for consistency.
This commit refactors the subtask_toolkit module to improve code organization, eliminate duplication, and enhance portability: 1. Code consolidation: - Merged extract_individual_actions.py into analyze_subtask_candidate.py - Created utils.py with common utility functions - Reduced code duplication by ~105-120 lines 2. Path portability: - Replaced all absolute hardcoded paths with relative paths - Changed config directory from subtask_configs_demo to subtask_configs - Added DEFAULT_* constants for common paths 3. Code quality improvements: - Added license headers to all files - Replaced Chinese comments with English - Fixed ruff linting errors (E501, E402, RUF001, RUF002, RUF003) - Added noqa directives for necessary import order exceptions 4. Utils module provides: - create_gpt4_model() - standardized model creation - create_chat_agent() - agent initialization helper - extract_token_usage() - unified token usage extraction - load_json()/save_json() - JSON file operations - get_timestamp_iso()/get_timestamp_filename() - timestamp utilities Modified files: - analyze_session.py: Added license header and noqa directive - analyze_subtask_candidate.py: Merged extract_individual_actions, translated comments - replay_from_log.py: Use utils functions, relative paths - run_webvoyager_tasks.py: Use utils functions, relative paths - subtask_agent_example.py: Added DEFAULT_* constants, use utils - subtask_manager.py: Updated config directory references - utils.py: New utility module - __init__.py: Added license header 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
examples/toolkits/subtask_toolkit/replay_from_log.py (1)
636-783: Makeagent_recoverytruly async-safe and guard empty responses
agent_recoveryis declaredasyncbut still uses the synchronousself.recovery_agent.step(prompt)and then unconditionally indexesresponse.msgs[0]. This has two coupled problems:
- The synchronous
steplikely performs network I/O and will block the event loop inside an async flow.- If the agent returns no messages or
msgsis missing,response.msgs[0]will raise, breaking recovery entirely.Prefer:
Using an async API, e.g.:
response = await self.recovery_agent.astep(prompt)or offloading the sync call to a thread executor via
asyncio.to_thread.Guarding access to
response.msgs:msgs = getattr(response, "msgs", []) or [] if not msgs: # log & record a recovery failure, then return None ... agent_msg = msgs[0].content.strip()Also ensure token-usage extraction and history logging only run when
responseis valid.These issues were already flagged in a previous review and remain unresolved.
🧹 Nitpick comments (18)
examples/toolkits/subtask_toolkit/utils.py (2)
14-14: Remove unused Ruffnoqadirective
# ruff: noqa: E501is flagged as unused. Either enable E501 in Ruff or drop this directive to keep lint config and Google-style linting clean.
45-72: Use a system message for the agent’s “system” content
create_chat_agentbuilds the “system message” withBaseMessage.make_assistant_message, while the docstring describes it as a system message. For clarity and alignment with message semantics, preferBaseMessage.make_system_message(content=system_content, role_name=role_name)(or equivalent) instead of an assistant message.examples/toolkits/subtask_toolkit/subtask_manager.py (2)
14-14: Drop unusedruff: noqadirective
# ruff: noqa: E501is reported as unused. Removing it avoids dead configuration and keeps style tooling aligned.
81-83: Narrow the broad exception when loading configsCatching bare
Exceptionwhile loading each*_subtasks.jsonmakes it harder to distinguish I/O errors from JSON bugs. Consider catchingOSErrorandjson.JSONDecodeErrorexplicitly (or at least logging vialogging), instead of a blindException.examples/toolkits/subtask_toolkit/analyze_subtask_candidate.py (4)
29-32: Prefer package‑relative imports for toolkit modules
from subtask_manager import SubtaskManagerandfrom utils import create_chat_agentwork when running this file as a script but will break if imported asexamples.toolkits.subtask_toolkit.analyze_subtask_candidate. To make this more robust, switch to relative imports such asfrom .subtask_manager import SubtaskManagerandfrom .utils import create_chat_agent.
133-135: Remove unusedjson.dumpscall
json.dumps(existing_subtasks, indent=2)is computed and the result discarded. This is dead code and can be removed to avoid confusion.
101-113: Avoid blindExceptionin config loader
load_all_existing_subtaskscatches a bareExceptionwhen loading each JSON file. Narrowing this toOSErrorandjson.JSONDecodeError(or at least logging vialogginginstead of
492-557: Token-usage extraction duplicated instead of using shared utilityHere, token usage is read manually from
response.info.get('usage', {}), whereasextract_token_usageinutils.pyencapsulates this logic. Reusing the shared helper would reduce duplication and keep token accounting consistent across the toolkit.examples/toolkits/subtask_toolkit/subtask_agent_example.py (4)
14-16: Shebang placement and unusedruff: noqaThe shebang is not the first line and
# ruff: noqa: E501, E402is reported unused. For a directly executable script, the shebang should be the very first line of the file, and unusednoqadirectives should be removed to keep lint configuration clean.
295-323: Improve JSON schema type handling for subtask variablesIn
SubtaskFunction.get_function_schema, the JSON schematypeis always"string"regardless ofvar_config['type'], making theif var_config['type'] in ['string', 'date'] else "string"conditional effectively useless. If you intend to support numeric or other types (e.g."number"), mapvar_config['type']appropriately; otherwise, simplify the code to a single"string"type to avoid misleading callers.
643-737: Be explicit aboutexec-based wrapper generationDynamic wrappers for subtask tools are created via
exec(func_code, local_vars). This is flexible but harder to reason about, debug, and static‑analyze. If possible, consider generating callables via regular Python closures (e.g., an inner async function that closes oversubtask_funcand_agent) instead of constructing code strings. If staying withexec, documenting this choice and limiting the constructed surface (e.g., avoiding interpolating untrusted values) would mitigate maintainability and safety concerns.
454-524: Wrap browser connection failure with clearer error signaling
initializecatches a broadExceptionwhen connecting to the CDP endpoint and simply returnsFalse. This is workable for examples, but for reuse it would be clearer either to:
- raise a dedicated exception (e.g.,
RuntimeError("Failed to connect to browser")), or- return a structured error object and log the original exception via
logging.This would improve diagnosability and align better with Google-style error handling.
examples/toolkits/subtask_toolkit/analyze_session.py (3)
14-16: Shebang position and unused RuffnoqaAs in other scripts, the shebang appears after the license header and
# ruff: noqa: E501is unused. If you intend this to be directly executable, consider moving the shebang to the very top, and drop the unusednoqadirective.
31-101: Factor out duplicated ARIA-label extraction logic
extract_aria_label_from_snapshotlargely duplicates the implementation inreplay_from_log.py. Centralizing this in a shared helper (e.g., inutils.pyor another common module) would reduce maintenance overhead and ensure both analysis and replay behave identically when parsing snapshots.
528-540: Avoid blindExceptionwhen reading agent communication logThe
except Exception as earound readingagent_communication_log.jsonis broad. Narrowing toOSError/json.JSONDecodeError(or at least logging vialogginginstead ofexamples/toolkits/subtask_toolkit/run_webvoyager_tasks.py (1)
581-599: Consider making--jsonlrequired instead of defaulting to a Downloads pathThe default JSONL path is
~/Downloads/WebVoyager_data_08312025_updated.jsonl. While this is better than a hardcoded user-specific absolute path, it may still not exist on many machines and can lead to confusing “file not found” errors. For a more portable CLI, consider making--jsonlrequired (no default) or validating the default path and failing fast with a clear message if missing.examples/toolkits/subtask_toolkit/replay_from_log.py (2)
332-338: Align skip list with supported replay actions or drop dead branches
should_replay_actionskips several actions:skip_actions = { 'get_tab_info', 'get_page_snapshot', 'get_som_screenshot', 'console_view', 'get_page_links', }but
replay_actionstill contains branches for'console_view'and'get_page_links'. Because all call sites checkshould_replay_actionbefore invokingreplay_action, these branches are effectively dead. Either:
- Remove
'console_view'and'get_page_links'fromskip_actionsso they can be replayed, or- Remove the corresponding branches from
replay_actionto avoid misleading, unreachable code.Currently the intent is ambiguous.
Also applies to: 1046-1048, 1082-1087
114-118: Consider narrowing the exception inget_cdp_url
get_cdp_urlcatches a bareExceptionand prints a generic error. Narrowing toOSError/URLError/json.JSONDecodeError(and optionally logging vialogging) will make browser-connection failures easier to diagnose and conform better to Google-style error handling.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
examples/toolkits/subtask_toolkit/__init__.pyexamples/toolkits/subtask_toolkit/analyze_session.pyexamples/toolkits/subtask_toolkit/analyze_subtask_candidate.pyexamples/toolkits/subtask_toolkit/replay_from_log.pyexamples/toolkits/subtask_toolkit/run_webvoyager_tasks.pyexamples/toolkits/subtask_toolkit/subtask_agent_example.pyexamples/toolkits/subtask_toolkit/subtask_manager.pyexamples/toolkits/subtask_toolkit/utils.py
🚧 Files skipped from review as they are similar to previous changes (1)
- examples/toolkits/subtask_toolkit/init.py
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
⚙️ CodeRabbit configuration file
Review the python code against the google python style guide and point out any mismatches.
Files:
examples/toolkits/subtask_toolkit/subtask_manager.pyexamples/toolkits/subtask_toolkit/utils.pyexamples/toolkits/subtask_toolkit/analyze_session.pyexamples/toolkits/subtask_toolkit/analyze_subtask_candidate.pyexamples/toolkits/subtask_toolkit/subtask_agent_example.pyexamples/toolkits/subtask_toolkit/run_webvoyager_tasks.pyexamples/toolkits/subtask_toolkit/replay_from_log.py
🧬 Code graph analysis (3)
examples/toolkits/subtask_toolkit/utils.py (3)
camel/messages/base.py (2)
BaseMessage(55-688)make_assistant_message(139-182)camel/models/model_factory.py (1)
ModelFactory(66-341)camel/types/enums.py (2)
ModelPlatformType(1785-2024)ModelType(33-1601)
examples/toolkits/subtask_toolkit/analyze_subtask_candidate.py (3)
examples/toolkits/subtask_toolkit/subtask_manager.py (3)
SubtaskManager(30-282)load_all_subtasks(52-84)add_new_subtasks(178-258)examples/toolkits/subtask_toolkit/utils.py (1)
create_chat_agent(45-72)camel/messages/base.py (1)
BaseMessage(55-688)
examples/toolkits/subtask_toolkit/subtask_agent_example.py (2)
examples/toolkits/subtask_toolkit/utils.py (3)
extract_token_usage(76-100)get_timestamp_filename(176-183)get_timestamp_iso(166-173)examples/toolkits/subtask_toolkit/replay_from_log.py (1)
load_subtask_config(120-245)
🪛 Ruff (0.14.10)
examples/toolkits/subtask_toolkit/subtask_manager.py
14-14: Unused noqa directive (non-enabled: E501)
Remove unused noqa directive
(RUF100)
81-81: Do not catch blind exception: Exception
(BLE001)
examples/toolkits/subtask_toolkit/utils.py
14-14: Unused noqa directive (non-enabled: E501)
Remove unused noqa directive
(RUF100)
examples/toolkits/subtask_toolkit/analyze_session.py
14-14: Unused noqa directive (non-enabled: E501)
Remove unused noqa directive
(RUF100)
15-15: Shebang should be at the beginning of the file
(EXE005)
536-536: Do not catch blind exception: Exception
(BLE001)
examples/toolkits/subtask_toolkit/analyze_subtask_candidate.py
14-14: Unused noqa directive (non-enabled: E501)
Remove unused noqa directive
(RUF100)
111-111: Do not catch blind exception: Exception
(BLE001)
examples/toolkits/subtask_toolkit/subtask_agent_example.py
14-14: Unused noqa directive (non-enabled: E501, E402)
Remove unused noqa directive
(RUF100)
15-15: Shebang should be at the beginning of the file
(EXE005)
274-274: Consider moving this statement to an else block
(TRY300)
276-276: Do not catch blind exception: Exception
(BLE001)
307-309: Useless if-else condition
(RUF034)
405-407: Avoid specifying long messages outside the exception class
(TRY003)
442-442: Avoid specifying long messages outside the exception class
(TRY003)
487-487: Do not catch blind exception: Exception
(BLE001)
732-732: Use of exec detected
(S102)
1060-1060: Do not catch blind exception: Exception
(BLE001)
1099-1099: Do not catch blind exception: Exception
(BLE001)
1392-1392: Do not catch blind exception: Exception
(BLE001)
1446-1446: Do not catch blind exception: Exception
(BLE001)
examples/toolkits/subtask_toolkit/run_webvoyager_tasks.py
14-14: Unused noqa directive (non-enabled: E501, E402)
Remove unused noqa directive
(RUF100)
15-15: Shebang should be at the beginning of the file
(EXE005)
116-116: Consider moving this statement to an else block
(TRY300)
117-117: Do not catch blind exception: Exception
(BLE001)
170-172: Avoid specifying long messages outside the exception class
(TRY003)
252-252: Do not catch blind exception: Exception
(BLE001)
261-261: Do not catch blind exception: Exception
(BLE001)
338-338: Do not catch blind exception: Exception
(BLE001)
348-348: Consider moving this statement to an else block
(TRY300)
364-364: Do not catch blind exception: Exception
(BLE001)
632-632: Do not catch blind exception: Exception
(BLE001)
examples/toolkits/subtask_toolkit/replay_from_log.py
14-14: Unused noqa directive (non-enabled: E501, E402)
Remove unused noqa directive
(RUF100)
15-15: Shebang should be at the beginning of the file
(EXE005)
114-114: Do not catch blind exception: Exception
(BLE001)
245-245: Avoid specifying long messages outside the exception class
(TRY003)
586-586: Avoid specifying long messages outside the exception class
(TRY003)
798-798: Avoid specifying long messages outside the exception class
(TRY003)
1273-1273: Consider moving this statement to an else block
(TRY300)
1275-1275: Do not catch blind exception: Exception
(BLE001)
1399-1399: Do not catch blind exception: Exception
(BLE001)
1433-1433: Avoid specifying long messages outside the exception class
(TRY003)
1436-1436: Avoid specifying long messages outside the exception class
(TRY003)
1829-1829: Do not catch blind exception: Exception
(BLE001)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: pytest_package_llm_test
- GitHub Check: pytest_package_fast_test
- GitHub Check: pytest_examples
- GitHub Check: pre-commit
🔇 Additional comments (2)
examples/toolkits/subtask_toolkit/run_webvoyager_tasks.py (1)
521-534: Guard against zero tasks in summary to avoid division by zero
print_summarycomputes percentages assucceeded/total*100andfailed/total*100. Iftotalis zero (no tasks inself.results), this will raise aZeroDivisionError. Add a guard such as an early return or conditional percentage computation to handle thetotal == 0case gracefully.examples/toolkits/subtask_toolkit/subtask_manager.py (1)
274-281: Guardmin/maxoveridvalues to avoid type errors
get_subtask_summarycomputesmin/maxovers.get('id', 0)directly. Sinceidvalues can be strings (e.g."06_filter_by_stops") or integers depending on whetherrenumber_existing_subtasks()has been called, this will raiseTypeErrorin Python 3 when comparing mixed types. The codebase explicitly handles both formats inget_next_numeric_idusing the regex patternr'^(\d+)'to extract numeric parts from strings. Apply similar normalization inget_subtask_summaryby converting all IDs to integers, or filter to numeric IDs only when computing the range.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai re-review |
|
@nitpicker55555: I'll perform a full re-review of all the changes in this PR. ✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
examples/toolkits/browser_skills_example/replay_from_log.py (1)
636-783: Avoid blockingChatAgent.stepin async recovery and guard against empty responses
agent_recoveryis declaredasyncbut still calls the synchronousself.recovery_agent.step(prompt)and then blindly indexesresponse.msgs[0]. This can (a) block the event loop during network I/O and (b) raise ifmsgsis empty or missing.Consider:
- Using an async API if available, e.g.
await self.recovery_agent.astep(prompt), or- Offloading the sync call via
await asyncio.to_thread(self.recovery_agent.step, prompt).And before accessing
response.msgs[0], verify thatresponsehas a non-emptymsgslist and handle the “no messages” case gracefully (log and returnNoneinstead of crashing).
🧹 Nitpick comments (12)
examples/toolkits/browser_skills_example/__init__.py (1)
1-24: Add blank line between copyright header and module docstring.According to the Google Python Style Guide, there should be a blank line between the copyright/license header comments and the module docstring.
🔎 Proposed fix
# ========= Copyright 2023-2026 @ CAMEL-AI.org. All Rights Reserved. ========= # Licensed under the Apache License, Version 2.0 (the "License"); # you may not use this file except in compliance with the License. # You may obtain a copy of the License at # # http://www.apache.org/licenses/LICENSE-2.0 # # Unless required by applicable law or agreed to in writing, software # distributed under the License is distributed on an "AS IS" BASIS, # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. # See the License for the specific language governing permissions and # limitations under the License. # ========= Copyright 2023-2026 @ CAMEL-AI.org. All Rights Reserved. ========= + """ Subtask Toolkit - Browser automation subtask management and replay system.Otherwise, the module docstring clearly documents the package purpose, and the
__version__export is appropriately minimal for an initialization file.camel/toolkits/hybrid_browser_toolkit/ts/src/hybrid-browser-toolkit.ts (1)
221-222: Consider a more type-safe approach for logging fields.Casting to
anyto add a runtime field bypasses TypeScript's type safety. While the comment explains this is for logging purposes, consider these alternatives:
- Extend the return type to include an optional snapshot field for logging
- Use a separate logging mechanism that doesn't modify the return value
- Create a wrapper type that properly declares the logging field
🔎 More type-safe alternatives
Option 1: Extend the return type
- async getSomScreenshot(): Promise<VisualMarkResult & { timing: any }> { + async getSomScreenshot(): Promise<VisualMarkResult & { timing: any; snapshot?: string }> { // ... - // Add snapshot to result for logging purposes (won't be shown to agent) - (result as any).snapshot = snapshotResult.snapshot; + // Add snapshot to result for logging purposes (won't be shown to agent) + result.snapshot = snapshotResult.snapshot;Option 2: Create a dedicated logging result type
type SomScreenshotResultWithLogging = VisualMarkResult & { timing: any; _snapshot_for_logging?: string; };examples/toolkits/browser_skills_example/utils.py (1)
14-14: Remove unusedruff: noqadirectiveRuff warns that
# ruff: noqa: E501is unused because these checks are not enabled here. Dropping the directive will keep the header cleaner and avoid linter noise.examples/toolkits/browser_skills_example/subtask_manager.py (1)
14-14: Drop unusedruff: noqadirectiveRuff reports
# ruff: noqa: E501as unused here. Removing it will keep the module header cleaner and aligned with tooling.examples/toolkits/browser_skills_example/analyze_session.py (1)
14-14: Remove unusedruff: noqaheaderThe
# ruff: noqa: E501directive is flagged as unused. Since long-line checking isn’t enabled here, you can remove it without changing behavior.examples/toolkits/browser_skills_example/analyze_subtask_candidate.py (2)
14-14: Clean up unusedruff: noqaThe module-level
# ruff: noqa: E501isn’t needed and is reported as unused; removing it will satisfy Ruff and simplify the header.
576-581: Defensively guard access toresponse.infobefore.get()
token_usage = response.info.get('usage', {})assumesresponse.infois always a dict. If the ChatAgent ever returns a response withinfoset toNone(or missing), this will raise anAttributeError.Consider guarding this as you do in other modules:
token_usage = {} info = getattr(response, "info", None) if isinstance(info, dict): token_usage = info.get("usage", {}) or {} else: token_usage = {}This keeps the analysis robust even if future backend changes alter
infosemantics.examples/toolkits/browser_skills_example/run_webvoyager_tasks.py (2)
14-14: Remove unusedruff: noqasuppressionThe
# ruff: noqa: E501, E402directive is reported as unused. Dropping it keeps the header consistent with tooling and avoids unnecessary global suppression.
586-601: Validate default JSONL path or make--jsonlrequired
--jsonlcurrently defaults to a user-specific Downloads path. On machines without that file,load_taskswill raiseFileNotFoundErrorat runtime with a stack trace.Either:
- Make
--jsonlrequired and fail fast withargparse(required=True), or- Keep a default but validate existence after parsing args, printing a clear error and exiting if the file is missing.
This will make the script more portable and user-friendly.
examples/toolkits/browser_skills_example/subtask_agent_example.py (3)
14-14: Drop unusedruff: noqadirectiveThe
# ruff: noqa: E501, E402header is flagged as unused; removing it will align this script with Ruff’s expectations and simplify the top of the file.
305-313: Fix redundantif/elseand improve variable type mapping in function schemaIn
get_function_schema, the"type"is always set to"string":"type": "string" if var_config['type'] in ['string', 'date'] else "string",This makes the condition useless and ignores numeric types.
Consider mapping your internal types explicitly, e.g.:
var_type = var_config.get("type", "string") if var_type in ("string", "date"): json_type = "string" elif var_type == "number": json_type = "number" else: json_type = "string" parameters["properties"][var_name] = { "type": json_type, "description": var_config["description"], }This both removes the redundant branch and makes schemas more accurate.
794-808: Remove leftover debug print from system prompt generation
get_system_messagecurrently prints the entiresubtask_list:print("subtask_list", subtask_list)This looks like debug output and will clutter CLI logs each initialization. It’s safe to remove or gate behind a verbose/debug flag.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (5)
examples/toolkits/browser_skills_example/subtask_configs/1_subtasks.jsonis excluded by!**/*.jsonexamples/toolkits/browser_skills_example/subtask_configs/2_subtasks.jsonis excluded by!**/*.jsonexamples/toolkits/browser_skills_example/subtask_configs/3_subtasks.jsonis excluded by!**/*.jsonexamples/toolkits/browser_skills_example/subtask_configs/4_subtasks.jsonis excluded by!**/*.jsonexamples/toolkits/browser_skills_example/subtask_configs/5_subtasks.jsonis excluded by!**/*.json
📒 Files selected for processing (13)
camel/models/azure_openai_model.pycamel/models/openai_model.pycamel/toolkits/hybrid_browser_toolkit/ts/src/hybrid-browser-toolkit.tscamel/toolkits/hybrid_browser_toolkit/ws_wrapper.pycamel/utils/tool_result.pyexamples/toolkits/browser_skills_example/__init__.pyexamples/toolkits/browser_skills_example/analyze_session.pyexamples/toolkits/browser_skills_example/analyze_subtask_candidate.pyexamples/toolkits/browser_skills_example/replay_from_log.pyexamples/toolkits/browser_skills_example/run_webvoyager_tasks.pyexamples/toolkits/browser_skills_example/subtask_agent_example.pyexamples/toolkits/browser_skills_example/subtask_manager.pyexamples/toolkits/browser_skills_example/utils.py
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
⚙️ CodeRabbit configuration file
Review the python code against the google python style guide and point out any mismatches.
Files:
examples/toolkits/browser_skills_example/__init__.pycamel/models/azure_openai_model.pycamel/utils/tool_result.pyexamples/toolkits/browser_skills_example/analyze_session.pyexamples/toolkits/browser_skills_example/utils.pyexamples/toolkits/browser_skills_example/replay_from_log.pycamel/toolkits/hybrid_browser_toolkit/ws_wrapper.pyexamples/toolkits/browser_skills_example/subtask_manager.pyexamples/toolkits/browser_skills_example/analyze_subtask_candidate.pycamel/models/openai_model.pyexamples/toolkits/browser_skills_example/run_webvoyager_tasks.pyexamples/toolkits/browser_skills_example/subtask_agent_example.py
🧬 Code graph analysis (4)
examples/toolkits/browser_skills_example/analyze_session.py (1)
examples/toolkits/browser_skills_example/replay_from_log.py (2)
extract_aria_label_from_snapshot(342-410)get_latest_snapshot_before_action(592-614)
examples/toolkits/browser_skills_example/utils.py (3)
camel/messages/base.py (2)
BaseMessage(55-688)make_assistant_message(139-182)camel/models/model_factory.py (1)
ModelFactory(66-341)camel/types/enums.py (2)
ModelPlatformType(1785-2024)ModelType(33-1601)
camel/toolkits/hybrid_browser_toolkit/ws_wrapper.py (1)
camel/utils/tool_result.py (1)
ToolResult(18-47)
examples/toolkits/browser_skills_example/analyze_subtask_candidate.py (2)
examples/toolkits/browser_skills_example/subtask_manager.py (3)
SubtaskManager(30-282)load_all_subtasks(52-84)add_new_subtasks(178-258)examples/toolkits/browser_skills_example/utils.py (1)
create_chat_agent(45-72)
🪛 Ruff (0.14.10)
examples/toolkits/browser_skills_example/analyze_session.py
14-14: Unused noqa directive (non-enabled: E501)
Remove unused noqa directive
(RUF100)
15-15: Shebang should be at the beginning of the file
(EXE005)
536-536: Do not catch blind exception: Exception
(BLE001)
examples/toolkits/browser_skills_example/utils.py
14-14: Unused noqa directive (non-enabled: E501)
Remove unused noqa directive
(RUF100)
examples/toolkits/browser_skills_example/replay_from_log.py
14-14: Unused noqa directive (non-enabled: E501, E402)
Remove unused noqa directive
(RUF100)
15-15: Shebang should be at the beginning of the file
(EXE005)
114-114: Do not catch blind exception: Exception
(BLE001)
245-245: Avoid specifying long messages outside the exception class
(TRY003)
586-586: Avoid specifying long messages outside the exception class
(TRY003)
798-798: Avoid specifying long messages outside the exception class
(TRY003)
1273-1273: Consider moving this statement to an else block
(TRY300)
1275-1275: Do not catch blind exception: Exception
(BLE001)
1399-1399: Do not catch blind exception: Exception
(BLE001)
1433-1433: Avoid specifying long messages outside the exception class
(TRY003)
1436-1436: Avoid specifying long messages outside the exception class
(TRY003)
1829-1829: Do not catch blind exception: Exception
(BLE001)
examples/toolkits/browser_skills_example/subtask_manager.py
14-14: Unused noqa directive (non-enabled: E501)
Remove unused noqa directive
(RUF100)
81-81: Do not catch blind exception: Exception
(BLE001)
examples/toolkits/browser_skills_example/analyze_subtask_candidate.py
14-14: Unused noqa directive (non-enabled: E501)
Remove unused noqa directive
(RUF100)
111-111: Do not catch blind exception: Exception
(BLE001)
examples/toolkits/browser_skills_example/run_webvoyager_tasks.py
14-14: Unused noqa directive (non-enabled: E501, E402)
Remove unused noqa directive
(RUF100)
15-15: Shebang should be at the beginning of the file
(EXE005)
116-116: Consider moving this statement to an else block
(TRY300)
117-117: Do not catch blind exception: Exception
(BLE001)
170-172: Avoid specifying long messages outside the exception class
(TRY003)
251-251: Do not catch blind exception: Exception
(BLE001)
260-260: Do not catch blind exception: Exception
(BLE001)
337-337: Do not catch blind exception: Exception
(BLE001)
347-347: Consider moving this statement to an else block
(TRY300)
363-363: Do not catch blind exception: Exception
(BLE001)
637-637: Do not catch blind exception: Exception
(BLE001)
examples/toolkits/browser_skills_example/subtask_agent_example.py
14-14: Unused noqa directive (non-enabled: E501, E402)
Remove unused noqa directive
(RUF100)
15-15: Shebang should be at the beginning of the file
(EXE005)
274-274: Consider moving this statement to an else block
(TRY300)
276-276: Do not catch blind exception: Exception
(BLE001)
307-309: Useless if-else condition
(RUF034)
405-407: Avoid specifying long messages outside the exception class
(TRY003)
442-442: Avoid specifying long messages outside the exception class
(TRY003)
487-487: Do not catch blind exception: Exception
(BLE001)
732-732: Use of exec detected
(S102)
1060-1060: Do not catch blind exception: Exception
(BLE001)
1099-1099: Do not catch blind exception: Exception
(BLE001)
1392-1392: Do not catch blind exception: Exception
(BLE001)
1446-1446: Do not catch blind exception: Exception
(BLE001)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: pytest_package_llm_test
- GitHub Check: pre-commit
- GitHub Check: pytest_package_fast_test
🔇 Additional comments (15)
camel/models/azure_openai_model.py (6)
376-379: LGTM: Correct conditional removal ofparallel_tool_calls.The logic correctly removes
parallel_tool_callswhen no tools are provided, preventing API errors. The comment clearly explains the rationale.
396-399: LGTM: Consistent async implementation.The async version correctly mirrors the synchronous implementation, maintaining consistency across both code paths.
421-424: LGTM: Correct handling in parse method.The logic correctly removes
parallel_tool_callsfor structured output parsing when tools are absent. Consistent with the method's existing use ofdeepcopy.
446-449: LGTM: Consistent async parse implementation.Correctly mirrors the synchronous parse method with appropriate async handling.
475-478: LGTM: Correct handling in streaming parse.The logic appropriately handles the streaming structured output case, removing
parallel_tool_callswhen tools are absent.
506-509: LGTM: Consistent async streaming parse implementation.Correctly implements the async streaming case with appropriate handling of
parallel_tool_calls.camel/models/openai_model.py (6)
410-413: LGTM: Correct conditional removal with proper sanitization order.The logic correctly removes
parallel_tool_callsbefore calling_sanitize_config, ensuring proper configuration preparation for the API call.
434-437: LGTM: Consistent async implementation.The async version correctly mirrors the synchronous implementation with proper configuration handling.
463-466: LGTM: Correct handling in parse method.Properly removes
parallel_tool_callsbefore sanitization in the structured output parsing path.
492-495: LGTM: Consistent async parse implementation.Correctly implements the async parse path with appropriate configuration handling.
524-527: LGTM: Correct streaming parse implementation.Properly handles
parallel_tool_callsremoval in the streaming structured output case.
558-561: LGTM: Consistent async streaming parse implementation.Correctly implements the async streaming path with proper configuration handling.
camel/utils/tool_result.py (1)
36-38: LGTM: Clean internal attribute addition.The
_snapshot_for_loggingattribute is appropriately marked as internal with a leading underscore, properly type-annotated, and clearly documented. This follows Python naming conventions and the Google Python Style Guide.camel/toolkits/hybrid_browser_toolkit/ws_wrapper.py (2)
568-574: LGTM: Defensive conditional logging.The logic correctly checks for the specific action and attribute presence before logging, preventing errors if the attribute is missing.
797-805: LGTM: Clean separation of agent-facing and logging data.The implementation correctly separates the agent-facing result (text and images) from the logging-only snapshot, using the internal
_snapshot_for_loggingattribute appropriately.
|
|
||
| if self.variables: | ||
| for var_name, var_config in self.variables.items(): | ||
| parameters["properties"][var_name] = { |
| # === Model & Agent Creation === | ||
| def create_gpt4_model(temperature: float = 0.0): | ||
| """ | ||
| Create standard GPT-4 model. |
| temperature: Model temperature (default: 0.0) | ||
|
|
||
| Returns: | ||
| Configured GPT-4 model |
There was a problem hiding this comment.
Hi, Thank you very much for pointing this out! Due to time constraints, many details were not carefully addressed. If you have the time, I would greatly appreciate it if you could propose a new PR patch to help organize and refine it. Thank you very much.
Code reviewFound 2 issues:
The parse-related methods use
This inconsistency can cause API errors since OpenAI only allows Files affected:
The exact same changes for removing These duplicate changes appear in:
This suggests the PR branch may be out of date and should be rebased with master to avoid merge conflicts. 🤖 Generated with Claude Code - If this code review was useful, please react with 👍. Otherwise, react with 👎. |
Thanks @LIHUA919! Fixed |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 10
🤖 Fix all issues with AI agents
In `@examples/toolkits/browser_skills_example/action_replayer.py`:
- Around line 14-16: Move the shebang (#!/usr/bin/env python3) to be the very
first line of action_replayer.py (before any license header or comments), remove
the unused ruff noqa directive ("# ruff: noqa: E501, E402") if not needed, and
keep the encoding comment ("# -*- coding: utf-8 -*-") directly after the
shebang; ensure no text precedes the shebang so the interpreter recognizes it.
In `@examples/toolkits/browser_skills_example/browser_skills_example.py`:
- Around line 14-16: The shebang (#!/usr/bin/env python3) must be the very first
line of the file and currently sits after the license header; move the shebang
to line 1 (before any comments or license text) and then remove the unused
"ruff: noqa: E501, E402" directive (or restrict it to relevant lines) so the
file no longer has a license header preceding the shebang and does not keep an
unused noqa at top.
- Around line 420-421: There is a redundant duplicate assignment of
self.current_user_task = None; remove the second occurrence and keep the single
initialization (leave the original initialization where self.current_user_task
is first set) so that only one assignment to self.current_user_task exists in
the class.
In
`@examples/toolkits/browser_skills_example/browser_skills/enter-departure-location/SKILL.md`:
- Line 18: Update the task_description string in SKILL.md to correct the
misspelling "Edinburg" to "Edinburgh" (the YAML key is task_description in the
enter-departure-location skill); replace the incorrect city name so it matches
the default value and correct spelling used elsewhere.
In
`@examples/toolkits/browser_skills_example/browser_skills/enter-destination-location/SKILL.md`:
- Line 18: The task_description value in
examples/toolkits/browser_skills_example/browser_skills/enter-destination-location/SKILL.md
contains a misspelling "Edinburg"; update the string to "Edinburgh" to correct
the destination name, and also fix the identical typo in the
enter-departure-location SKILL.md task_description so both SKILL.md files use
"Edinburgh" consistently.
In
`@examples/toolkits/browser_skills_example/browser_skills/initialize-flight-search-page/SKILL.md`:
- Line 7: The default URLs are inconsistent: update the flight_search_url
default to use the canonical Google Flights URL used by url_end; specifically
change the flight_search_url default value from "https://www.google.com/flights"
to "https://www.google.com/travel/flights" so both url_end and flight_search_url
reference the same canonical URL.
In
`@examples/toolkits/browser_skills_example/browser_skills/set-one-way-departure-date/SKILL.md`:
- Around line 10-15: The default_value for the parameter departure_date in
SKILL.md includes extraneous price text ("303 British pounds"); update the
departure_date default_value to a date-only string consistent with the
description (e.g., "Saturday, January 17, 2026" or "January 17, 2026") so the
field can be reliably parsed as a date by code that consumes departure_date.
In `@examples/toolkits/browser_skills_example/run_webvoyager_tasks.py`:
- Around line 14-15: Place the shebang (#!/usr/bin/env python3) at the very top
of the file (line 1) so the script is executable, and remove or relocate the
redundant ruff: noqa: E501, E402 directive (the unused noqa on the existing
comment line) — i.e., move the shebang above the license header and delete the
unnecessary "# ruff: noqa: E501, E402" line.
In `@examples/toolkits/browser_skills_example/subtask_extractor.py`:
- Around line 133-134: The call to json.dumps(existing_subtasks, indent=2) is
unused and should be removed; locate the line in subtask_extractor.py where
existing_subtasks is being formatted (the standalone json.dumps call) and delete
it (or, if the formatted JSON is intended for logging or output, replace the
dead call with an explicit use such as assigning to a variable or passing it to
the logger/print in the surrounding function).
In `@examples/toolkits/browser_skills_example/subtask_to_skill_converter.py`:
- Around line 54-77: The escape_yaml_string function currently detects newlines
but doesn’t escape them, so modify escape_yaml_string to replace newline (and
optionally carriage return and tab) characters with their escaped sequences
(e.g., '\n', '\r', '\t') before wrapping in double quotes; update the escape
step for backslashes and double quotes (in the existing escaped =
s.replace('\\', '\\\\').replace('"', '\\"')) to also perform .replace('\n',
'\\n').replace('\r', '\\r').replace('\t', '\\t') so multiline input becomes a
valid quoted YAML string when returned by escape_yaml_string.
♻️ Duplicate comments (3)
examples/toolkits/browser_skills_example/run_webvoyager_tasks.py (1)
586-591: Default JSONL path uses user-specific location.The default path
Path.home() / "Downloads" / "WebVoyager_data_08312025_updated.jsonl"assumes the file exists in the user's Downloads folder. This is fragile as it varies by user and may not exist. Consider making--jsonla required argument or using a path relative to the script.🔧 Proposed fix
parser.add_argument( "--jsonl", - default=default_jsonl, + required=True, help="Path to WebVoyager JSONL file", )examples/toolkits/browser_skills_example/action_replayer.py (1)
719-721: Blocking synchronous call in async context and unguarded response access.
agent_recoveryis anasyncmethod, butself.recovery_agent.step(prompt)is a synchronous call that may perform network I/O. This blocks the event loop. Additionally,response.msgs[0]is accessed without checking ifmsgsis non-empty.🔧 Proposed fix
# Get agent response - response = self.recovery_agent.step(prompt) - agent_msg = response.msgs[0].content.strip() + # Use asyncio.to_thread to avoid blocking the event loop + response = await asyncio.to_thread(self.recovery_agent.step, prompt) + + # Guard against empty response + if not getattr(response, 'msgs', None) or len(response.msgs) == 0: + print("✗ Recovery agent returned no messages") + return None + agent_msg = response.msgs[0].content.strip()examples/toolkits/browser_skills_example/browser_skills_example.py (1)
331-338: Useless if-else condition always returns "string".The condition
"string" if var_config['type'] in ['string', 'date'] else "string"always evaluates to"string"regardless of the type. This appears to be incomplete logic.🔧 Proposed fix
If the intent is to map Python types to JSON Schema types:
for var_name, var_config in self.variables.items(): + var_type = var_config.get('type', 'string') + json_type = "string" if var_type in ['string', 'date'] else ( + "integer" if var_type == 'int' else ( + "number" if var_type == 'float' else "string" + )) parameters["properties"][var_name] = { - "type": "string" - if var_config['type'] in ['string', 'date'] - else "string", + "type": json_type, "description": var_config['description'], }Or if only strings are expected, simplify to:
parameters["properties"][var_name] = { - "type": "string" - if var_config['type'] in ['string', 'date'] - else "string", + "type": "string", "description": var_config['description'], }
🧹 Nitpick comments (24)
examples/toolkits/browser_skills_example/skill_loader.py (4)
52-56: Use exception chaining for better traceability.When re-raising exceptions, use
raise ... from eto preserve the original traceback and clearly indicate the cause.♻️ Proposed fix
try: frontmatter = yaml.safe_load(frontmatter_text) except yaml.YAMLError as e: - raise ValueError(f"Failed to parse YAML in {skill_md_path}: {e}") + raise ValueError(f"Failed to parse YAML in {skill_md_path}: {e}") from e
133-134: Remove unusedjson.dumpscall.The result of
json.dumps(existing_subtasks, indent=2)is not assigned or used. This appears to be dead code.♻️ Proposed fix
- # Format existing subtasks - Full JSON - json.dumps(existing_subtasks, indent=2) - # Format existing subtasks - Summary list for easy comparisonNote: This code is in
subtask_extractor.py, notskill_loader.py. If the intent was to use this JSON string, assign it to a variable.
195-196: Consider narrowing exception handling.Catching broad
Exceptioncan mask unexpected errors. Consider catching specific exceptions likeValueError,json.JSONDecodeError, oryaml.YAMLError, or at minimum log the exception type for debugging.♻️ Proposed fix
- except Exception as e: - print(f" Failed to load {skill_dir.name}: {e}") + except (ValueError, json.JSONDecodeError, yaml.YAMLError) as e: + print(f" Failed to load {skill_dir.name}: {type(e).__name__}: {e}")
162-164: Consider using logging for production use.The module uses
print()for status messages, which is acceptable for example scripts. If this module is intended for broader use, consider using Python'sloggingmodule for better control over output verbosity.Also applies to: 174-176, 178-178
examples/toolkits/browser_skills_example/subtask_extractor.py (4)
14-14: Remove unusednoqadirective.The
# ruff: noqa: E501directive is unused since E501 (line length) is not enabled in the linter configuration.♻️ Proposed fix
# ========= Copyright 2023-2026 @ CAMEL-AI.org. All Rights Reserved. ========= -# ruff: noqa: E501 """
308-308: Moveimport reto module level.The
remodule is imported inside functions at lines 308 and 456. For better performance and following Python conventions, move this import to the module level with other imports.♻️ Proposed fix
Add at module level (after line 27):
import reThen remove the local imports:
def infer_variable_type(value: str) -> str: """ Infer the type from a value. """ - - import re - # Date format detectiondef parse_agent_response(response_text: str) -> List[Dict[str, Any]]: ... # Try to extract JSON objects from code blocks or raw text # Look for ```json blocks first - import re - json_blocks = re.findall(Also applies to: 456-456
111-112: Consider narrowing exception handling.Catching broad
Exceptioncan hide unexpected errors. Consider catching specific exceptions likejson.JSONDecodeErrorandOSError.♻️ Proposed fix
- except Exception as e: + except (json.JSONDecodeError, OSError) as e: print(f"Warning: Failed to load {json_file}: {e}")
492-496: UseOptional[str]for broader Python compatibility.The type hint
str | Nonerequires Python 3.10+. For compatibility with older Python versions, useOptional[str]which is already imported fromtyping.♻️ Proposed fix
+from typing import Any, Dict, List, Optional + def analyze_with_agent( session_folder: str, - subtask_configs_dir: str | None = None, + subtask_configs_dir: Optional[str] = None, auto_save: bool = True, ) -> Dict[str, Any]:examples/toolkits/browser_skills_example/subtask_to_skill_converter.py (2)
355-357: Moveargparseimport to the top of the file.Per Google Python Style Guide, imports should be at the top of the file, not inside functions. This import is unconditionally used when
main()is called.♻️ Suggested refactor
Move the import to the top with other stdlib imports:
import json import os import re import shutil +import argparse from pathlib import Path from typing import Any, Dict, ListThen remove line 357:
def main(): """Main entry point for the conversion script.""" - import argparse
270-277: Consider catching specific exceptions instead of bareException.The static analysis flags catching bare
Exceptionat lines 273 and 332. While this is a script context where broad error handling is sometimes acceptable, catching specific exceptions (e.g.,json.JSONDecodeError,OSError) would provide better error messages and avoid masking unexpected errors.Also applies to: 332-338
examples/toolkits/browser_skills_example/run_webvoyager_tasks.py (2)
104-124: Importreat module level, not inside method.The
remodule is imported insideverify_task()at line 106. Per Google Python Style Guide, imports should be at module level.♻️ Suggested refactor
Move
import reto the top with other imports (around line 28):import asyncio import json +import re import sys
515-518: Hardcoded output filename reduces flexibility.The
save_results()method writes to a hardcoded filenamewebvoyager_results_1.json. Consider parameterizing this or using a timestamp-based filename to avoid overwriting previous results.♻️ Suggested refactor
def save_results(self): """Save results to JSON file.""" - output_file = Path("webvoyager_results_1.json") + from utils import get_timestamp_filename + output_file = Path(f"webvoyager_results_{get_timestamp_filename()}.json") with open(output_file, 'w', encoding='utf-8') as f: json.dump(self.results, f, indent=2, ensure_ascii=False) print(f"\n💾 Results saved to: {output_file}")examples/toolkits/browser_skills_example/action_replayer.py (3)
247-300: Complex JSON parsing with manual brace matching is fragile.The
load_log_filemethod implements manual JSON splitting by tracking braces and string escapes. This is error-prone and could fail with edge cases like escaped unicode. Consider using a streaming JSON parser or ensuring the log format uses newline-delimited JSON (NDJSON).For NDJSON format, the parsing simplifies to:
def load_log_file(self) -> List[Dict[str, Any]]: actions = [] with open(self.log_file, 'r', encoding='utf-8') as f: for line in f: if line.strip(): try: actions.append(json.loads(line)) except json.JSONDecodeError as e: print(f"Warning: Failed to parse line: {e}") return actions
1569-1571: macOS-specific Chrome path in error message.The error message hardcodes a macOS-specific path for Chrome. This won't help users on Windows or Linux.
♻️ Suggested improvement
print("Please ensure Chrome is running with:") - print( - " /Applications/Google\\ Chrome.app/Contents/MacOS/Google\\ Chrome --remote-debugging-port=9223" - ) + print(" macOS: /Applications/Google\\ Chrome.app/Contents/MacOS/Google\\ Chrome --remote-debugging-port=9223") + print(" Linux: google-chrome --remote-debugging-port=9223") + print(" Windows: chrome.exe --remote-debugging-port=9223")
1196-1215: Retry logic duplicates action dispatch code.The retry logic in
replay_action(lines 1196-1215 and 1337-1362) duplicates the action dispatch switch/case from earlier in the method. This creates maintenance burden - if a new action is added, it needs to be added in multiple places.Consider extracting action dispatch to a helper method:
async def _execute_action(self, action_name: str, args: list) -> Any: """Execute a single browser action by name.""" if action_name == 'click' and args: return await self.toolkit.browser_click(ref=args[0]) elif action_name == 'type' and len(args) >= 2: return await self.toolkit.browser_type(ref=args[0], text=args[1]) # ... etcexamples/toolkits/browser_skills_example/browser_skills_example.py (3)
668-741: Use ofexec()for dynamic function creation poses security and maintainability risks.While
exec()is flagged by static analysis (S102), the usage here is controlled - the code template is developer-defined and only variable names from configuration are interpolated. However, this pattern is hard to debug and test.Consider using
functools.partialor a factory pattern instead:♻️ Alternative approach using closures
def create_subtask_wrapper(subtask_func, agent_ref): """Create a wrapper function for a subtask.""" async def wrapper(**kwargs): """Dynamically generated wrapper.""" call_log = { 'timestamp': get_timestamp_iso(), 'type': 'subtask_call', 'function_name': subtask_func.function_name, 'subtask_name': subtask_func.name, 'arguments': kwargs, 'result': None } result = await subtask_func.execute(**kwargs) call_log['result'] = result if hasattr(agent_ref, 'agent_communication_log'): agent_ref.agent_communication_log.append(call_log) return json.dumps(result, ensure_ascii=False) # Set function metadata for tool discovery wrapper.__name__ = subtask_func.function_name wrapper.__doc__ = subtask_func.description return wrapperThis approach:
- Avoids
exec()security concerns- Is easier to debug (real stack traces)
- Is testable
878-886: Hardcoded Google Flights instructions in system prompt.The system message contains hardcoded instructions specific to Google Flights (lines 879-885). This reduces the generality of the
SkillsAgentclass. Consider making these task-specific instructions configurable or injectable.
1036-1073: Fragile JSON parsing with string splitting.The
_extract_agent_browser_callsmethod parses concatenated JSON objects by splitting on}\n{. This is fragile and can fail with nested objects or different whitespace. Consider using a streaming JSON parser or ensuring logs use NDJSON format.examples/toolkits/browser_skills_example/browser_skills/select-seating-class/SKILL.md (1)
22-22: Wrap bare URLs in angle brackets for proper Markdown formatting.Markdown best practices recommend wrapping bare URLs in angle brackets to improve rendering and comply with linting rules.
📝 Proposed fix for bare URLs
-Change the seating class for the flight search (e.g., Economy, Business, First). Execution page: https://www.google.com/travel/flights +Change the seating class for the flight search (e.g., Economy, Business, First). Execution page: <https://www.google.com/travel/flights>-- **Start URL**: https://www.google.com/travel/flights?tfs=CBwQARooEgoyMDI2LTAxLTE5agwIAxIIL20vMDdxenZyDAgDEggvbS8wN19wZhooEgoyMDI2LTAxLTI2agwIAxIIL20vMDdfcGZyDAgDEggvbS8wN3F6dkABSAFwAYIBCwj___________8BmAEB&tfu=KgIIAw +- **Start URL**: <https://www.google.com/travel/flights?tfs=CBwQARooEgoyMDI2LTAxLTE5agwIAxIIL20vMDdxenZyDAgDEggvbS8wN19wZhooEgoyMDI2LTAxLTI2agwIAxIIL20vMDdfcGZyDAgDEggvbS8wN3F6dkABSAFwAYIBCwj___________8BmAEB&tfu=KgIIAw> -- **End URL**: https://www.google.com/travel/flights?tfs=CBwQARooEgoyMDI2LTAxLTE5agwIAxIIL20vMDdxenZyDAgDEggvbS8wN19wZhooEgoyMDI2LTAxLTI2agwIAxIIL20vMDdfcGZyDAgDEggvbS8wN3F6dkABSARwAYIBCwj___________8BmAEB&tfu=KgIIAw +- **End URL**: <https://www.google.com/travel/flights?tfs=CBwQARooEgoyMDI2LTAxLTE5agwIAxIIL20vMDdxenZyDAgDEggvbS8wN19wZhooEgoyMDI2LTAxLTI2agwIAxIIL20vMDdfcGZyDAgDEggvbS8wN3F6dkABSARwAYIBCwj___________8BmAEB&tfu=KgIIAw>Also applies to: 30-31
examples/toolkits/browser_skills_example/browser_skills/submit-flight-search/SKILL.md (1)
15-15: Wrap bare URLs in angle brackets for proper Markdown formatting.Same recommendation as other SKILL.md files - wrap bare URLs in angle brackets.
📝 Proposed fix for bare URLs
-Click the search button to submit the flight search query. Execution page: https://www.google.com/travel/flights +Click the search button to submit the flight search query. Execution page: <https://www.google.com/travel/flights>-- **Start URL**: https://www.google.com/travel/flights?tfs=CBwQARooEgoyMDI2LTAxLTI4agwIAxIIL20vMDJtNzdyDAgDEggvbS8wNTJidxooEgoyMDI2LTAxLTI4agwIAxIIL20vMDUyYndyDAgDEggvbS8wMm03N0ABSAFwAYIBCwj___________8BmAEB&tfu=KgIIAw +- **Start URL**: <https://www.google.com/travel/flights?tfs=CBwQARooEgoyMDI2LTAxLTI4agwIAxIIL20vMDJtNzdyDAgDEggvbS8wNTJidxooEgoyMDI2LTAxLTI4agwIAxIIL20vMDUyYndyDAgDEggvbS8wMm03N0ABSAFwAYIBCwj___________8BmAEB&tfu=KgIIAw> -- **End URL**: https://www.google.com/travel/flights/search?tfs=CBwQAhooEgoyMDI2LTAxLTI4agwIAxIIL20vMDJtNzdyDAgDEggvbS8wNTJidxooEgoyMDI2LTAxLTI4agwIAxIIL20vMDUyYndyDAgDEggvbS8wMm03N0ABSAFwAYIBCwj___________8BmAEB +- **End URL**: <https://www.google.com/travel/flights/search?tfs=CBwQAhooEgoyMDI2LTAxLTI4agwIAxIIL20vMDJtNzdyDAgDEggvbS8wNTJidxooEgoyMDI2LTAxLTI4agwIAxIIL20vMDUyYndyDAgDEggvbS8wMm03N0ABSAFwAYIBCwj___________8BmAEB>Also applies to: 19-20
examples/toolkits/browser_skills_example/browser_skills/set-round-trip-dates/SKILL.md (1)
28-28: Wrap bare URLs in angle brackets for proper Markdown formatting.Consistent with other SKILL.md files, wrap bare URLs in angle brackets.
📝 Proposed fix for bare URLs
-Enter and confirm both departure and return dates for round-trip flight search. Execution page: https://www.google.com/travel/flights +Enter and confirm both departure and return dates for round-trip flight search. Execution page: <https://www.google.com/travel/flights>-- **Start URL**: https://www.google.com/travel/flights?tfs=CBwQARoOagwIAxIIL20vMDJtNzcaDnIMCAMSCC9tLzAybTc3QAFIAXABggELCP___________wGYAQE&tfu=KgIIAw +- **Start URL**: <https://www.google.com/travel/flights?tfs=CBwQARoOagwIAxIIL20vMDJtNzcaDnIMCAMSCC9tLzAybTc3QAFIAXABggELCP___________wGYAQE&tfu=KgIIAw> -- **End URL**: https://www.google.com/travel/flights?tfs=CBwQARooEgoyMDI2LTAxLTI4agwIAxIIL20vMDJtNzdyDAgDEggvbS8wNTJidxooEgoyMDI2LTAxLTI4agwIAxIIL20vMDUyYndyDAgDEggvbS8wMm03N0ABSAFwAYIBCwj___________8BmAEB&tfu=KgIIAw +- **End URL**: <https://www.google.com/travel/flights?tfs=CBwQARooEgoyMDI2LTAxLTI4agwIAxIIL20vMDJtNzdyDAgDEggvbS8wNTJidxooEgoyMDI2LTAxLTI4agwIAxIIL20vMDUyYndyDAgDEggvbS8wMm03N0ABSAFwAYIBCwj___________8BmAEB&tfu=KgIIAw>Also applies to: 37-38
examples/toolkits/browser_skills_example/browser_skills/set-one-way-departure-date/SKILL.md (1)
22-22: Wrap bare URLs in angle brackets for proper Markdown formatting.Consistent with other SKILL.md files, wrap bare URLs in angle brackets.
📝 Proposed fix for bare URLs
-Select and confirm the departure date for a one-way flight search. Execution page: https://www.google.com/travel/flights +Select and confirm the departure date for a one-way flight search. Execution page: <https://www.google.com/travel/flights>-- **Start URL**: https://www.google.com/travel/flights?tfs=CBwQARocagwIAxIIL20vMDFfZDRyDAgDEggvbS8wNXF0akABSAFwAYIBCwj___________8BmAEC&tfu=KgIIAw +- **Start URL**: <https://www.google.com/travel/flights?tfs=CBwQARocagwIAxIIL20vMDFfZDRyDAgDEggvbS8wNXF0akABSAFwAYIBCwj___________8BmAEC&tfu=KgIIAw> -- **End URL**: https://www.google.com/travel/flights?tfs=CBwQARocagwIAxIIL20vMDFfZDRyDAgDEggvbS8wNXF0akABSAFwAYIBCwj___________8BmAEC&tfu=KgIIAw +- **End URL**: <https://www.google.com/travel/flights?tfs=CBwQARocagwIAxIIL20vMDFfZDRyDAgDEggvbS8wNXF0akABSAFwAYIBCwj___________8BmAEC&tfu=KgIIAw>Also applies to: 30-31
examples/toolkits/browser_skills_example/browser_skills/initialize-flight-search-page/SKILL.md (1)
19-25: Consider adding Execution Context section for consistency.Other SKILL.md files in this PR include an "Execution Context" section with Start URL and End URL. Consider adding this section here for consistency across skill documentation.
📝 Suggested addition
## Variables - **flight_search_url** (default: `https://www.google.com/flights`): URL of the flight search website (e.g., 'https://www.google.com/flights') + +## Execution Context + +- **End URL**: <https://www.google.com/travel/flights>examples/toolkits/browser_skills_example/browser_skills/enter-departure-location/SKILL.md (1)
22-22: Consider wrapping bare URLs in angle brackets or markdown links.The linter flagged bare URLs in the Markdown content. To improve consistency and adhere to Markdown best practices, consider wrapping them in angle brackets (e.g.,
<https://...>) or using markdown link syntax.♻️ Proposed refactor
-Enter and confirm the departure city for flight search. Execution page: https://www.google.com/travel/flights +Enter and confirm the departure city for flight search. Execution page: <https://www.google.com/travel/flights>-- **Start URL**: https://www.google.com/travel/flights -- **End URL**: https://www.google.com/travel/flights +- **Start URL**: <https://www.google.com/travel/flights> +- **End URL**: <https://www.google.com/travel/flights>Also applies to: 30-31
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (9)
examples/toolkits/browser_skills_example/browser_skills/enter-departure-location/actions.jsonis excluded by!**/*.jsonexamples/toolkits/browser_skills_example/browser_skills/enter-destination-location/actions.jsonis excluded by!**/*.jsonexamples/toolkits/browser_skills_example/browser_skills/filter-non-stop-flights/actions.jsonis excluded by!**/*.jsonexamples/toolkits/browser_skills_example/browser_skills/initialize-flight-search-page/actions.jsonis excluded by!**/*.jsonexamples/toolkits/browser_skills_example/browser_skills/select-seating-class/actions.jsonis excluded by!**/*.jsonexamples/toolkits/browser_skills_example/browser_skills/set-one-way-departure-date/actions.jsonis excluded by!**/*.jsonexamples/toolkits/browser_skills_example/browser_skills/set-round-trip-dates/actions.jsonis excluded by!**/*.jsonexamples/toolkits/browser_skills_example/browser_skills/set-ticket-type-to-one-way/actions.jsonis excluded by!**/*.jsonexamples/toolkits/browser_skills_example/browser_skills/submit-flight-search/actions.jsonis excluded by!**/*.json
📒 Files selected for processing (19)
camel/models/azure_openai_model.pycamel/models/openai_model.pycamel/types/enums.pyexamples/toolkits/browser_skills_example/CAMEL_BROWSER_SKILLS.mdexamples/toolkits/browser_skills_example/action_replayer.pyexamples/toolkits/browser_skills_example/browser_skills/enter-departure-location/SKILL.mdexamples/toolkits/browser_skills_example/browser_skills/enter-destination-location/SKILL.mdexamples/toolkits/browser_skills_example/browser_skills/filter-non-stop-flights/SKILL.mdexamples/toolkits/browser_skills_example/browser_skills/initialize-flight-search-page/SKILL.mdexamples/toolkits/browser_skills_example/browser_skills/select-seating-class/SKILL.mdexamples/toolkits/browser_skills_example/browser_skills/set-one-way-departure-date/SKILL.mdexamples/toolkits/browser_skills_example/browser_skills/set-round-trip-dates/SKILL.mdexamples/toolkits/browser_skills_example/browser_skills/set-ticket-type-to-one-way/SKILL.mdexamples/toolkits/browser_skills_example/browser_skills/submit-flight-search/SKILL.mdexamples/toolkits/browser_skills_example/browser_skills_example.pyexamples/toolkits/browser_skills_example/run_webvoyager_tasks.pyexamples/toolkits/browser_skills_example/skill_loader.pyexamples/toolkits/browser_skills_example/subtask_extractor.pyexamples/toolkits/browser_skills_example/subtask_to_skill_converter.py
✅ Files skipped from review due to trivial changes (1)
- examples/toolkits/browser_skills_example/browser_skills/filter-non-stop-flights/SKILL.md
🚧 Files skipped from review as they are similar to previous changes (1)
- camel/models/azure_openai_model.py
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
⚙️ CodeRabbit configuration file
Review the python code against the google python style guide and point out any mismatches.
Files:
examples/toolkits/browser_skills_example/subtask_to_skill_converter.pycamel/types/enums.pyexamples/toolkits/browser_skills_example/subtask_extractor.pyexamples/toolkits/browser_skills_example/skill_loader.pyexamples/toolkits/browser_skills_example/action_replayer.pycamel/models/openai_model.pyexamples/toolkits/browser_skills_example/run_webvoyager_tasks.pyexamples/toolkits/browser_skills_example/browser_skills_example.py
🧬 Code graph analysis (2)
examples/toolkits/browser_skills_example/action_replayer.py (2)
examples/toolkits/browser_skills_example/utils.py (3)
create_gpt4_model(28-42)extract_token_usage(76-100)get_timestamp_iso(166-173)camel/toolkits/hybrid_browser_toolkit_py/hybrid_browser_toolkit.py (1)
browser_get_page_links(1938-1967)
examples/toolkits/browser_skills_example/run_webvoyager_tasks.py (5)
examples/toolkits/browser_skills_example/utils.py (1)
create_chat_agent(45-72)camel/messages/base.py (2)
BaseMessage(55-688)make_user_message(93-136)camel/toolkits/base.py (1)
agent(131-152)examples/toolkits/browser_skills_example/subtask_extractor.py (2)
analyze_with_agent(492-733)main(736-756)examples/toolkits/browser_skills_example/analyze_session.py (1)
main(574-585)
🪛 LanguageTool
examples/toolkits/browser_skills_example/CAMEL_BROWSER_SKILLS.md
[style] ~266-~266: Consider an alternative for the overused word “exactly”.
Context: ...ry - These new interaction patterns are exactly what needs to be learned and encapsulat...
(EXACTLY_PRECISELY)
🪛 markdownlint-cli2 (0.18.1)
examples/toolkits/browser_skills_example/browser_skills/submit-flight-search/SKILL.md
15-15: Bare URL used
(MD034, no-bare-urls)
19-19: Bare URL used
(MD034, no-bare-urls)
20-20: Bare URL used
(MD034, no-bare-urls)
examples/toolkits/browser_skills_example/browser_skills/set-one-way-departure-date/SKILL.md
22-22: Bare URL used
(MD034, no-bare-urls)
30-30: Bare URL used
(MD034, no-bare-urls)
31-31: Bare URL used
(MD034, no-bare-urls)
examples/toolkits/browser_skills_example/browser_skills/enter-departure-location/SKILL.md
22-22: Bare URL used
(MD034, no-bare-urls)
30-30: Bare URL used
(MD034, no-bare-urls)
31-31: Bare URL used
(MD034, no-bare-urls)
examples/toolkits/browser_skills_example/browser_skills/set-round-trip-dates/SKILL.md
28-28: Bare URL used
(MD034, no-bare-urls)
37-37: Bare URL used
(MD034, no-bare-urls)
38-38: Bare URL used
(MD034, no-bare-urls)
examples/toolkits/browser_skills_example/browser_skills/select-seating-class/SKILL.md
22-22: Bare URL used
(MD034, no-bare-urls)
30-30: Bare URL used
(MD034, no-bare-urls)
31-31: Bare URL used
(MD034, no-bare-urls)
🪛 Ruff (0.14.11)
examples/toolkits/browser_skills_example/subtask_to_skill_converter.py
238-238: Avoid specifying long messages outside the exception class
(TRY003)
273-273: Do not catch blind exception: Exception
(BLE001)
332-332: Do not catch blind exception: Exception
(BLE001)
examples/toolkits/browser_skills_example/subtask_extractor.py
14-14: Unused noqa directive (non-enabled: E501)
Remove unused noqa directive
(RUF100)
111-111: Do not catch blind exception: Exception
(BLE001)
examples/toolkits/browser_skills_example/skill_loader.py
47-47: Avoid specifying long messages outside the exception class
(TRY003)
55-55: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
55-55: Avoid specifying long messages outside the exception class
(TRY003)
90-90: Avoid specifying long messages outside the exception class
(TRY003)
93-93: Avoid specifying long messages outside the exception class
(TRY003)
195-195: Do not catch blind exception: Exception
(BLE001)
examples/toolkits/browser_skills_example/action_replayer.py
14-14: Unused noqa directive (non-enabled: E501, E402)
Remove unused noqa directive
(RUF100)
15-15: Shebang should be at the beginning of the file
(EXE005)
114-114: Do not catch blind exception: Exception
(BLE001)
245-245: Avoid specifying long messages outside the exception class
(TRY003)
586-586: Avoid specifying long messages outside the exception class
(TRY003)
798-798: Avoid specifying long messages outside the exception class
(TRY003)
1273-1273: Consider moving this statement to an else block
(TRY300)
1275-1275: Do not catch blind exception: Exception
(BLE001)
1399-1399: Do not catch blind exception: Exception
(BLE001)
1433-1433: Avoid specifying long messages outside the exception class
(TRY003)
1436-1436: Avoid specifying long messages outside the exception class
(TRY003)
1829-1829: Do not catch blind exception: Exception
(BLE001)
examples/toolkits/browser_skills_example/run_webvoyager_tasks.py
14-14: Unused noqa directive (non-enabled: E501, E402)
Remove unused noqa directive
(RUF100)
15-15: Shebang should be at the beginning of the file
(EXE005)
116-116: Consider moving this statement to an else block
(TRY300)
117-117: Do not catch blind exception: Exception
(BLE001)
170-172: Avoid specifying long messages outside the exception class
(TRY003)
251-251: Do not catch blind exception: Exception
(BLE001)
260-260: Do not catch blind exception: Exception
(BLE001)
337-337: Do not catch blind exception: Exception
(BLE001)
347-347: Consider moving this statement to an else block
(TRY300)
363-363: Do not catch blind exception: Exception
(BLE001)
637-637: Do not catch blind exception: Exception
(BLE001)
examples/toolkits/browser_skills_example/browser_skills_example.py
14-14: Unused noqa directive (non-enabled: E501, E402)
Remove unused noqa directive
(RUF100)
15-15: Shebang should be at the beginning of the file
(EXE005)
300-300: Consider moving this statement to an else block
(TRY300)
302-302: Do not catch blind exception: Exception
(BLE001)
333-335: Useless if-else condition
(RUF034)
491-491: Do not catch blind exception: Exception
(BLE001)
740-740: Use of exec detected
(S102)
1068-1068: Do not catch blind exception: Exception
(BLE001)
1107-1107: Do not catch blind exception: Exception
(BLE001)
1400-1400: Do not catch blind exception: Exception
(BLE001)
1450-1450: Do not catch blind exception: Exception
(BLE001)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: pytest_package_fast_test
- GitHub Check: pre-commit
🔇 Additional comments (15)
examples/toolkits/browser_skills_example/CAMEL_BROWSER_SKILLS.md (1)
1-668: Well-documented framework overview.This documentation provides a comprehensive guide to the Browser Skills framework, including architecture diagrams, workflow explanations, and usage examples. The structure is clear and the content aligns with the code components introduced in this PR.
Minor: Line 12 uses lowercase "Continuous learning" while other bullet points use title case.
camel/models/openai_model.py (2)
388-393: Correct handling ofparallel_tool_callsremoval.The logic correctly removes
parallel_tool_callswhen no tools are provided, as OpenAI's API only permits this parameter when tools are present. Usingif tools:(truthy check) is appropriate here since bothNoneand empty list[]should trigger the removal.
412-417: Consistent implementation across all async and parse methods.The
parallel_tool_callsremoval logic is consistently applied across all six request methods (sync/async chat completion, parse, and stream parse). The comments clearly explain the reasoning.Also applies to: 470-475, 536-541
examples/toolkits/browser_skills_example/subtask_extractor.py (1)
38-79: Well-structured extraction logic.The
extract_consecutive_individual_actionsfunction correctly identifies groups of 2+ consecutiveindividual_actionentries from the timeline, which aligns with the framework's design principle of only learning from exploration sequences.camel/types/enums.py (1)
46-46: Clarify the versioning strategy change forGPT_4_1.The
GPT_4_1value changed from"gpt-4.1-2025-04-14"to"gpt-4.1". Both identifiers are valid in the OpenAI API; however, this shifts from a pinned snapshot (fixed at April 14, 2025 version) to the base model identifier, which may receive updates when OpenAI updates what"gpt-4.1"references. Ensure this change is intentional, as it affects reproducibility if model behavior changes on OpenAI's end.examples/toolkits/browser_skills_example/subtask_to_skill_converter.py (1)
1-52: LGTM - Well-structured utility functions.The
slugifyfunction correctly handles edge cases like leading digits, consecutive hyphens, and special characters.examples/toolkits/browser_skills_example/run_webvoyager_tasks.py (2)
526-538: LGTM - ZeroDivisionError guard is correctly implemented.The
print_summary()method now properly guards against empty results before computing percentages, addressing the previous review concern.
245-262: LGTM - Snapshot handling is correct.The code now correctly treats
browser_get_page_snapshot()as returning a string and captures it appropriately, addressing the previous review concern.examples/toolkits/browser_skills_example/action_replayer.py (1)
785-1092: LGTM - Well-structured replay_action method.The method correctly handles ref mapping via aria-labels, variable substitution, and has good logging throughout. The action dispatch covers the expected browser operations.
examples/toolkits/browser_skills_example/browser_skills_example.py (2)
264-271: LGTM - Hardcoded path issue has been addressed.The code now uses
DEFAULT_BROWSER_LOG_DIRwhich is defined relative to the script directory, addressing the previous review concern.
458-532: LGTM - Robust initialization with CDP connection.The
initialize()method properly handles CDP connection, toolkit setup, and error cases. The custom tools list is well-defined.examples/toolkits/browser_skills_example/browser_skills/select-seating-class/SKILL.md (1)
1-19: LGTM! Well-structured skill metadata.The YAML frontmatter is complete and consistent, with appropriate variable definitions for the seating class selection skill.
examples/toolkits/browser_skills_example/browser_skills/submit-flight-search/SKILL.md (1)
1-12: LGTM! Appropriate skill definition for submit action.The frontmatter correctly defines the submit action without variables, which is appropriate for a button click operation.
examples/toolkits/browser_skills_example/browser_skills/set-round-trip-dates/SKILL.md (1)
1-25: LGTM! Well-defined variables for round-trip dates.The variable definitions are appropriate, and the same-day return default aligns with the task description.
examples/toolkits/browser_skills_example/browser_skills/set-ticket-type-to-one-way/SKILL.md (1)
1-20: LGTM! Documentation structure is clear and complete.The SKILL.md file follows the established pattern with proper YAML front matter and descriptive Markdown content. The skill metadata (id, indices, URLs, source) is well-defined, and the absence of variables is appropriate for this skill type.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
examples/toolkits/browser_skills_example/browser_skills/enter-departure-location/SKILL.md
Show resolved
Hide resolved
examples/toolkits/browser_skills_example/browser_skills/enter-destination-location/SKILL.md
Show resolved
Hide resolved
examples/toolkits/browser_skills_example/browser_skills/initialize-flight-search-page/SKILL.md
Show resolved
Hide resolved
examples/toolkits/browser_skills_example/browser_skills/set-one-way-departure-date/SKILL.md
Show resolved
Hide resolved
3 participants
Description
Describe your changes in detail (optional if the linked issue already contains a detailed description of the changes).
#3477
Checklist
Go over all the following points, and put an
xin all the boxes that apply.Fixes #issue-numberin the PR description (required)pyproject.tomlanduv lockIf you are unsure about any of these, don't hesitate to ask. We are here to help!