refactor(tools): nest sync_models under tools/sync_models/#1135
Conversation
asclearuc
requested review from
Rod-Christensen,
jmaionchi,
kwit75 and
stepmikhaylov
as code owners
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (35)
📝 WalkthroughWalkthroughThis PR relocates the sync_models tool from Changessync_models Directory Migration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Biome (2.4.16)tools/sync_models/src/sync_models.config.jsonFile contains syntax errors that prevent linting: Line 2: Expected a property but instead found '// =========================================================================='.; Line 15: End of file expected; Line 15: End of file expected; Line 16: Expected a property but instead found '// ----------------------------------------------------------------------'.; Line 15: End of file expected; Line 16: End of file expected; Line 20: End of file expected; Line 20: End of file expected; Line 28: Expected a property but instead found '// model_filter controls which models from the API are considered.'.; Line 20: End of file expected; Line 28: End of file expected; Line 32: End of file expected; Line 32: End of file expected; Line 36: Expected a property but instead found '// Pro-tier models are Responses-API-only (not v1/chat/completions). The'.; Line 32: End of file expected; Line 36: End of file expected; Line 38: End of file expected; Line 38: End of file expected; Line 38: End of file ... [truncated 11933 characters] ... ; Line 584: End of file expected; Line 587: End of file expected; Line 587: End of file expected; Line 587: End of file expected; Line 587: End of file expected; Line 588: End of file expected; Line 588: End of file expected; Line 588: End of file expected; Line 588: End of file expected; Line 606: End of file expected; Line 606: End of file expected; Line 606: End of file expected; Line 606: End of file expected; Line 607: End of file expected; Line 607: End of file expected; Line 607: End of file expected; Line 607: End of file expected; Line 608: End of file expected; Line 608: End of file expected; Line 608: End of file expected; Line 609: End of file expected; Line 617: End of file expected; Line 617: End of file expected; Line 617: End of file expected; Line 621: End of file expected 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 |
🤖 Internal: Discord sync markerAuto-managed by the Discord notification workflow. Stores the linked Discord message ID. Do not edit or delete. |
![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 (2)
tools/sync_models/README.md (1)
296-304:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix secret-name documentation to match the workflow.
This section documents
ROCKETRIDE_APIKEY_<PROVIDER>, but the workflow actually reads provider-specific names likeROCKETRIDE_OPENAI_KEY/ROCKETRIDE_ANTHROPIC_KEY(see.github/workflows/sync-models.ymlenv block). If readers follow the README literally, CI discovery will silently skip providers due to missing expected secrets.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tools/sync_models/README.md` around lines 296 - 304, The README currently says provider secrets are named ROCKETRIDE_APIKEY_<PROVIDER>, but the workflow (.github/workflows/sync-models.yml env block) actually reads provider-specific keys like ROCKETRIDE_OPENAI_KEY and ROCKETRIDE_ANTHROPIC_KEY; update the README section that describes provider API keys to match the workflow by stating the correct naming pattern (e.g., ROCKETRIDE_<PROVIDER>_KEY or list the actual names used such as ROCKETRIDE_OPENAI_KEY, ROCKETRIDE_ANTHROPIC_KEY) so CI discovery will not be skipped due to mismatched secret names.tools/sync_models/test/test_sync_live.py (1)
41-41:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winCritical: Incorrect repo root calculation for new file location.
Line 41 calculates
_REPO_ROOTusing.parent.parent.parent, which was correct for the old locationtools/test/test_sync_live.pybut is incorrect for the new locationtools/sync_models/test/test_sync_live.py. The current calculation resolves totools/instead of the repository root.This breaks all
_load_profiles()calls (line 46), which construct paths like_REPO_ROOT / 'nodes' / 'src' / 'nodes' / ..., resulting in incorrect paths liketools/nodes/src/nodes/....🐛 Proposed fix
-_REPO_ROOT = Path(__file__).parent.parent.parent +_REPO_ROOT = Path(__file__).parent.parent.parent.parent🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tools/sync_models/test/test_sync_live.py` at line 41, The repo root calculation `_REPO_ROOT = Path(__file__).parent.parent.parent` is off by one for the new file location and yields `tools/` instead of the repository root; update the `_REPO_ROOT` expression in this file to ascend one more level (use four .parent hops or equivalent, e.g. `.parent.parent.parent.parent` or `.resolve().parents[3]`) so that `_load_profiles()` and any path joins like `_REPO_ROOT / 'nodes' / 'src' / 'nodes' / ...` point to the actual repository root; then run the tests that exercise `_load_profiles()` to confirm paths are correct.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/sync-models.yml:
- Line 99: The shell commands invoking the sync_models script use unquoted
GitHub Actions template expansions (${{
steps.partition.outputs.with_key}} and ${{
steps.partition.outputs.without_key}}), which risks word-splitting or injection;
update the workflow to wrap those expansions in double quotes wherever used
(e.g. the python tools/sync_models/src/sync_models.py ... --pr-body | tee
/tmp/body-strict.md invocation and the other occurrence around line 119) so the
values are passed as single arguments to the script and safe from shell
interpretation.
---
Outside diff comments:
In `@tools/sync_models/README.md`:
- Around line 296-304: The README currently says provider secrets are named
ROCKETRIDE_APIKEY_<PROVIDER>, but the workflow
(.github/workflows/sync-models.yml env block) actually reads provider-specific
keys like ROCKETRIDE_OPENAI_KEY and ROCKETRIDE_ANTHROPIC_KEY; update the README
section that describes provider API keys to match the workflow by stating the
correct naming pattern (e.g., ROCKETRIDE_<PROVIDER>_KEY or list the actual names
used such as ROCKETRIDE_OPENAI_KEY, ROCKETRIDE_ANTHROPIC_KEY) so CI discovery
will not be skipped due to mismatched secret names.
In `@tools/sync_models/test/test_sync_live.py`:
- Line 41: The repo root calculation `_REPO_ROOT =
Path(__file__).parent.parent.parent` is off by one for the new file location and
yields `tools/` instead of the repository root; update the `_REPO_ROOT`
expression in this file to ascend one more level (use four .parent hops or
equivalent, e.g. `.parent.parent.parent.parent` or `.resolve().parents[3]`) so
that `_load_profiles()` and any path joins like `_REPO_ROOT / 'nodes' / 'src' /
'nodes' / ...` point to the actual repository root; then run the tests that
exercise `_load_profiles()` to confirm paths are correct.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: e1c0f6d3-8473-4f1c-8958-af8a4dc80f78
📒 Files selected for processing (32)
.github/workflows/sync-models.ymltools/dependabot-smoke/README.mdtools/dependabot-smoke/smoke-litellm.shtools/sync_models/README.mdtools/sync_models/requirements.txttools/sync_models/scripts/tasks.jstools/sync_models/src/__init__.pytools/sync_models/src/core/__init__.pytools/sync_models/src/core/merger.pytools/sync_models/src/core/patcher.pytools/sync_models/src/core/reporter.pytools/sync_models/src/core/smoke.pytools/sync_models/src/core/util.pytools/sync_models/src/providers/__init__.pytools/sync_models/src/providers/anthropic.pytools/sync_models/src/providers/base.pytools/sync_models/src/providers/deepseek.pytools/sync_models/src/providers/embedding_openai.pytools/sync_models/src/providers/gemini.pytools/sync_models/src/providers/minimax.pytools/sync_models/src/providers/mistral.pytools/sync_models/src/providers/openai.pytools/sync_models/src/providers/perplexity.pytools/sync_models/src/providers/qwen.pytools/sync_models/src/providers/xai.pytools/sync_models/src/sync_models.config.jsontools/sync_models/src/sync_models.pytools/sync_models/test/__init__.pytools/sync_models/test/conftest.pytools/sync_models/test/markers.pytools/sync_models/test/test_sync_live.pytools/sync_models/test/test_sync_logic.py
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tools/sync_models/README.md (1)
130-130:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate stale env-var template in strict-discovery note.
This line still references
ROCKETRIDE_APIKEY_<PROVIDER>, but the README now standardizes onROCKETRIDE_<PROVIDER>_KEY. Please align this string to avoid operator confusion.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tools/sync_models/README.md` at line 130, Update the stale environment variable template in the "Strict discovery" README sentence by replacing the old placeholder `ROCKETRIDE_APIKEY_<PROVIDER>` with the standardized `ROCKETRIDE_<PROVIDER>_KEY` so the note under "Strict discovery (default when `--enable-discovery` is set)" accurately reflects current env-var naming; ensure the textual example exactly uses `ROCKETRIDE_<PROVIDER>_KEY`.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@tools/sync_models/README.md`:
- Line 130: Update the stale environment variable template in the "Strict
discovery" README sentence by replacing the old placeholder
`ROCKETRIDE_APIKEY_<PROVIDER>` with the standardized `ROCKETRIDE_<PROVIDER>_KEY`
so the note under "Strict discovery (default when `--enable-discovery` is set)"
accurately reflects current env-var naming; ensure the textual example exactly
uses `ROCKETRIDE_<PROVIDER>_KEY`.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 21ace213-12a4-405e-9707-3defac06557a
📒 Files selected for processing (7)
tools/sync_models/README.mdtools/sync_models/src/providers/baidu_qianfan.pytools/sync_models/src/sync_models.config.jsontools/sync_models/src/sync_models.pytools/sync_models/test/markers.pytools/sync_models/test/test_baidu_qianfan_provider.pytools/sync_models/test/test_sync_live.py
Move all `sync_models`-related files out of the `tools/` root into a self-contained `tools/sync_models/` package, mirroring the existing `tools/contract_checks/` layout. No behavior change — pure reorganization plus the path references that follow. ``` tools/ ├── sync_models/ ← NEW (was sprawled across tools/ root) │ ├── README.md ← was tools/SYNC_MODELS.md │ ├── requirements.txt │ ├── scripts/tasks.js │ ├── src/ │ └── test/ ├── contract_checks/ ← unchanged └── dependabot-smoke/ ← path refs updated ``` `tools/` root mixed two conventions: `contract_checks/` was self-contained, while `sync_models` scattered its `src/`, `test/`, `scripts/`, `requirements.txt`, and docs directly under `tools/`, making the root ambiguous (whose `src/`? whose `requirements.txt`?). This nests it to match the established pattern. - `git mv` of `src/`, `test/`, `scripts/`, `requirements.txt` into `tools/sync_models/` (history preserved as renames); `SYNC_MODELS.md` → `README.md`. - Fixed depth-sensitive Python paths that broke by moving one level deeper: - `_find_repo_root()` in `sync_models.py` — `.parent.parent` → `.parent.parent.parent` (would otherwise resolve to `tools/` instead of the repo root, silently masked by a CWD fallback). - `conftest.py` `.env` loader and `test_sync_logic.py` subprocess invocation (`parents[2]` → `parents[3]`). - Updated `tasks.js` shared-lib `require` (`../../` → `../../../`), `.github/workflows/sync-models.yml`, the `dependabot-smoke` smoke script + README, and all in-tree doc/comment path references. - `python tools/sync_models/src/sync_models.py --help` — loads from new path. - `_find_repo_root()` resolves to the real repo root from a foreign CWD (confirms the fix, not the fallback). - `node scripts/build.js --list-actions` — `models:update`, `models:run-sync`, `models:prettier` still discovered (proves `tasks.js` glob + new `require`). - `pytest tools/sync_models/test/test_sync_logic.py` — **41 passed**, including the subprocess CLI validation. - Grep confirms no stragglers referencing the old `tools/src` / `tools/test` / `tools/requirements.txt` paths.
…name - test_sync_live.py _REPO_ROOT: +1 .parent for the new tools/sync_models/test/ depth (was resolving to tools/) - README: document secrets as ROCKETRIDE_<PROVIDER>_KEY, not ROCKETRIDE_APIKEY_<PROVIDER>; fix misquoted 'Discovery skipped' note
asclearuc
force-pushed
the
chore/RR-1108-reorganize
branch
from
f818c07 to
8d401d5
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tools/sync_models/src/sync_models.config.json`:
- Around line 585-586: Fix the minor grammar typo in the inline comment that
currently reads "Kimi — K2 reasoning models support large outputs; Moonshot v1
fall back to the chat default (4096) via the defaults section." — update it to
"falls back" (i.e., "Moonshot v1 falls back to the chat default (4096) via the
defaults section.") so the comment reads correctly; locate the comment string in
sync_models.config.json and edit only the comment text.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 188cdfae-4337-46c9-9ea5-27309c8694e6
📒 Files selected for processing (35)
.github/workflows/sync-models.ymltools/dependabot-smoke/README.mdtools/dependabot-smoke/smoke-litellm.shtools/sync_models/README.mdtools/sync_models/requirements.txttools/sync_models/scripts/tasks.jstools/sync_models/src/__init__.pytools/sync_models/src/core/__init__.pytools/sync_models/src/core/merger.pytools/sync_models/src/core/patcher.pytools/sync_models/src/core/reporter.pytools/sync_models/src/core/smoke.pytools/sync_models/src/core/util.pytools/sync_models/src/providers/__init__.pytools/sync_models/src/providers/anthropic.pytools/sync_models/src/providers/baidu_qianfan.pytools/sync_models/src/providers/base.pytools/sync_models/src/providers/deepseek.pytools/sync_models/src/providers/embedding_openai.pytools/sync_models/src/providers/gemini.pytools/sync_models/src/providers/kimi.pytools/sync_models/src/providers/minimax.pytools/sync_models/src/providers/mistral.pytools/sync_models/src/providers/openai.pytools/sync_models/src/providers/perplexity.pytools/sync_models/src/providers/qwen.pytools/sync_models/src/providers/xai.pytools/sync_models/src/sync_models.config.jsontools/sync_models/src/sync_models.pytools/sync_models/test/__init__.pytools/sync_models/test/conftest.pytools/sync_models/test/markers.pytools/sync_models/test/test_baidu_qianfan_provider.pytools/sync_models/test/test_sync_live.pytools/sync_models/test/test_sync_logic.py
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tools/sync_models/src/sync_models.config.json`:
- Around line 585-586: Fix the minor grammar typo in the inline comment that
currently reads "Kimi — K2 reasoning models support large outputs; Moonshot v1
fall back to the chat default (4096) via the defaults section." — update it to
"falls back" (i.e., "Moonshot v1 falls back to the chat default (4096) via the
defaults section.") so the comment reads correctly; locate the comment string in
sync_models.config.json and edit only the comment text.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 188cdfae-4337-46c9-9ea5-27309c8694e6
📒 Files selected for processing (35)
.github/workflows/sync-models.ymltools/dependabot-smoke/README.mdtools/dependabot-smoke/smoke-litellm.shtools/sync_models/README.mdtools/sync_models/requirements.txttools/sync_models/scripts/tasks.jstools/sync_models/src/__init__.pytools/sync_models/src/core/__init__.pytools/sync_models/src/core/merger.pytools/sync_models/src/core/patcher.pytools/sync_models/src/core/reporter.pytools/sync_models/src/core/smoke.pytools/sync_models/src/core/util.pytools/sync_models/src/providers/__init__.pytools/sync_models/src/providers/anthropic.pytools/sync_models/src/providers/baidu_qianfan.pytools/sync_models/src/providers/base.pytools/sync_models/src/providers/deepseek.pytools/sync_models/src/providers/embedding_openai.pytools/sync_models/src/providers/gemini.pytools/sync_models/src/providers/kimi.pytools/sync_models/src/providers/minimax.pytools/sync_models/src/providers/mistral.pytools/sync_models/src/providers/openai.pytools/sync_models/src/providers/perplexity.pytools/sync_models/src/providers/qwen.pytools/sync_models/src/providers/xai.pytools/sync_models/src/sync_models.config.jsontools/sync_models/src/sync_models.pytools/sync_models/test/__init__.pytools/sync_models/test/conftest.pytools/sync_models/test/markers.pytools/sync_models/test/test_baidu_qianfan_provider.pytools/sync_models/test/test_sync_live.pytools/sync_models/test/test_sync_logic.py
🛑 Comments failed to post (1)
tools/sync_models/src/sync_models.config.json (1)
585-586: 🧹 Nitpick | 🔵 Trivial | 💤 Low value
Minor typo in comment.
The comment reads "fall back" but should be "falls back" (or keep "fall back" if treating "Moonshot v1" as plural models).
✏️ Suggested fix
- // Kimi — K2 reasoning models support large outputs; Moonshot v1 fall + // Kimi — K2 reasoning models support large outputs; Moonshot v1 models fall // back to the chat default (4096) via the defaults section.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.// Kimi — K2 reasoning models support large outputs; Moonshot v1 models fall // back to the chat default (4096) via the defaults section.🧰 Tools
🪛 Biome (2.4.16)
[error] 584-586: End of file expected
(parse)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tools/sync_models/src/sync_models.config.json` around lines 585 - 586, Fix the minor grammar typo in the inline comment that currently reads "Kimi — K2 reasoning models support large outputs; Moonshot v1 fall back to the chat default (4096) via the defaults section." — update it to "falls back" (i.e., "Moonshot v1 falls back to the chat default (4096) via the defaults section.") so the comment reads correctly; locate the comment string in sync_models.config.json and edit only the comment text.
2 participants
What
Move all
sync_models-related files out of thetools/root into a self-containedtools/sync_models/package, mirroring the existingtools/contract_checks/layout. No behavior change — pure reorganization plus the path references that follow.Why
tools/root mixed two conventions:contract_checks/was self-contained, whilesync_modelsscattered itssrc/,test/,scripts/,requirements.txt, and docs directly undertools/, making the root ambiguous (whosesrc/? whoserequirements.txt?). This nests it to match the established pattern.Changes
git mvofsrc/,test/,scripts/,requirements.txtintotools/sync_models/(history preserved as renames);SYNC_MODELS.md→README.md._find_repo_root()insync_models.py—.parent.parent→.parent.parent.parent(would otherwise resolve totools/instead of the repo root, silently masked by a CWD fallback).conftest.py.envloader andtest_sync_logic.pysubprocess invocation (parents[2]→parents[3]).tasks.jsshared-librequire(../../→../../../),.github/workflows/sync-models.yml, thedependabot-smokesmoke script + README, and all in-tree doc/comment path references.Verification
python tools/sync_models/src/sync_models.py --help— loads from new path._find_repo_root()resolves to the real repo root from a foreign CWD (confirms the fix, not the fallback).node scripts/build.js --list-actions—models:update,models:run-sync,models:prettierstill discovered (provestasks.jsglob + newrequire).pytest tools/sync_models/test/test_sync_logic.py— 41 passed, including the subprocess CLI validation.tools/src/tools/test/tools/requirements.txtpaths.Summary by CodeRabbit