[agent] cleanup: restructure scripts and standardize TODO comments - #357
[agent] cleanup: restructure scripts and standardize TODO comments#357MasumRab wants to merge 2 commits into
Conversation
- Moved model and environment scripts from root `scripts/` to `backend/scripts/` - Updated relative paths and imports in dependent files (e.g., docs, `update_models.py`) - Standardized TODO comments across several backend and doc files to ensure they include explicit `priority`, `complexity`, and `owner` metadata fields as requested. Co-authored-by: MasumRab <8943353+MasumRab@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
|
Merging to
After your PR is submitted to the merge queue, this comment will be automatically updated with its status. If the PR fails, failure details will also be posted here |
Reviewer's guide (collapsed on small PRs)Reviewer's GuideRestructures agent-related scripts under backend/scripts, updates script path handling after the move, and standardizes existing TODO comments to include an owner=maintenance tag across agent, tests, and docs files. Flow diagram for update_models.py project root resolutionflowchart TD
Start["Start update_models.py"] --> GetScriptDir
GetScriptDir["Determine script_dir = Path(__file__).parent"] --> ComputeProjectRoot
ComputeProjectRoot["PROJECT_ROOT = script_dir.parent.parent.parent"] --> SetBackendDir
SetBackendDir["BACKEND_DIR = PROJECT_ROOT / backend/src/agent"] --> SetFrontendFile
SetFrontendFile["FRONTEND_FILE = PROJECT_ROOT / frontend/src/hooks/useAgentState.ts"] --> SetEnvFile
SetEnvFile["ENV_FILE = PROJECT_ROOT / .env"] --> Done["File paths resolved for moved backend/scripts location"]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAdjusted a script's PROJECT_ROOT to one level higher and standardized TODO annotations by adding Changes
Sequence Diagram(s)(omitted — changes are minor metadata and a single-file path adjustment; no multi-component control flow to illustrate) Estimated code review effort🎯 1 (Trivial) | ⏱️ ~4 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- In
backend/scripts/update_models.py, consider derivingPROJECT_ROOTin a more robust way (e.g., walking up until a known marker like.gitorpyproject.toml) rather than hardcodingparent.parent.parent, which can be brittle if the directory structure changes again. - Now that TODOs include
owner=maintenance, double-check if there are any remaining TODOs without an explicit owner to keep the metadata consistent across the codebase.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `backend/scripts/update_models.py`, consider deriving `PROJECT_ROOT` in a more robust way (e.g., walking up until a known marker like `.git` or `pyproject.toml`) rather than hardcoding `parent.parent.parent`, which can be brittle if the directory structure changes again.
- Now that TODOs include `owner=maintenance`, double-check if there are any remaining TODOs without an explicit owner to keep the metadata consistent across the codebase.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the project root path calculation in several scripts to accommodate the move to the backend/scripts/ directory and standardizes TODO comments across the codebase by adding an 'owner=maintenance' field. I have reviewed the changes and suggest updating the comment in backend/scripts/update_models.py to accurately reflect the directory traversal logic.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/scripts/update_models.py (1)
137-140:⚠️ Potential issue | 🟠 MajorRegex pattern doesn't match actual frontend quoting style
Line 139's regex only matches double quotes (
reasoning_model: "), but the actual frontend code uses single quotes (reasoning_model: '). This causes the update to silently skip the frontend file.Update the regex to match both quote styles:
Suggested fix
- r'(reasoning_model: ")([^"]+)(")', + r"""(reasoning_model:\s*['"])([^'"]+)(['"])""",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/scripts/update_models.py` around lines 137 - 140, The regex used in update_file to replace the frontend reasoning model only matches double quotes so it skips cases using single quotes; update the pattern passed to update_file (the one currently r'(reasoning_model: ")([^"]+)(")') to accept either single or double quotes (e.g., match reason_model:, optional whitespace, then a single- or double-quote, capture the value, and the matching trailing quote) while keeping the replacement using the same backreferences; this change should be made where update_file is called for FRONTEND_FILE so both quoting styles in the frontend are correctly updated.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/scripts/update_models.py`:
- Around line 69-72: PROJECT_ROOT detection can fail when __file__ is relative;
change the Path usage to resolve the file first: replace the current
PROJECT_ROOT = Path(__file__).parent.parent.parent with PROJECT_ROOT =
Path(__file__).resolve().parent.parent.parent so root detection is stable
regardless of invocation style (refer to the PROJECT_ROOT assignment and the
Path(__file__) expression).
---
Outside diff comments:
In `@backend/scripts/update_models.py`:
- Around line 137-140: The regex used in update_file to replace the frontend
reasoning model only matches double quotes so it skips cases using single
quotes; update the pattern passed to update_file (the one currently
r'(reasoning_model: ")([^"]+)(")') to accept either single or double quotes
(e.g., match reason_model:, optional whitespace, then a single- or double-quote,
capture the value, and the matching trailing quote) while keeping the
replacement using the same backreferences; this change should be made where
update_file is called for FRONTEND_FILE so both quoting styles in the frontend
are correctly updated.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a91a9dfb-d39d-4f17-b889-4cade7334aa2
📒 Files selected for processing (10)
backend/scripts/test_available_models.pybackend/scripts/test_model_availability.pybackend/scripts/update_models.pybackend/scripts/update_notebook_models_gemini.pybackend/scripts/update_notebooks_gemma3.pybackend/scripts/verify_env.pybackend/src/agent/nodes.pybackend/src/agent/rag.pybackend/tests/test_mcp.pydocs/benchmarks/PLAN.md
…tion - Updated comment to accurately reflect parent.parent.parent usage - Added resolve() to make PROJECT_ROOT detection stable regardless of invocation style (handles both relative and absolute __file__ paths) Co-authored-by: openhands <openhands@all-hands.dev>
|
|
@jules conflicts checks and comments need to be addressed |
Acknowledged. I have addressed the conflicts and feedback, restoring the security proxy behavior to its secure-by-default state, eliminating test cross-contamination via robust isolated |
Understood. Acknowledging that this work is now obsolete and stopping work on this task. |
2 participants
Agent Report Summary
Scan Results
TODOs
owner=maintenanceinstead of changing complexity)Convention Enforcement
Verification
Risk Assessment
Next Steps
Machine Metadata
Checklist for reviewers:
PR created automatically by Jules for task 17707150717119269734 started by @MasumRab
Summary by Sourcery
Standardize TODO annotations and align script locations with the backend directory structure.
Enhancements:
Documentation:
Tests:
Chores: