v2 with skills#9
Open
brandon-braner wants to merge 10 commits into
Open
Conversation
The llm_provider_discovery module and its tests are replaced wholesale in the v2 rebuild; the 6 failing assertions referenced integrations that no longer exist. https://claude.ai/code/session_016fp7YNMpLjqw8JUcceXp65
- Integration manifests (integrations/base.py) are now the single source of
truth: every (entity kind x scope) declares a path + format handler.
Adds Codex (TOML config, shared CLI/IDE), Devin Desktop (ex-Windsurf,
.devin/ write with .windsurf/ read fallback), rewrites Copilot/VS Code
(dedicated mcp.json), Cursor, Claude Code per current config surfaces.
- Format handlers (engine/handlers/) translate canonical models to disk
formats and return FileChange plans instead of writing: json_mcp,
toml_mcp (tomlkit, comment-preserving), markdown_dir, skill_dir,
toml_command, yaml_workflow, llm_json (read-only).
- Sync engine (engine/engine.py): discover/import/status/plan/apply/pull
with per-item hashing, 7-state cell status, dry-run unified diffs,
forced-push semantics for drifted cells, central rotated backups, and
legacy OPENSYNC marker cleanup on skill sync.
- Skills are now Agent Skills standard SKILL.md folders, replacing the v1
marker-injection into CLAUDE.md.
- Schema v2: generic entities table + projects/sync_state/backup_runs with
one-shot v1 migration (auto db backup to opensync.db.pre-v2).
- Generic /api/{kind} routers replace api.py; legacy discovery/registry/
target modules deleted.
- New test suite: manifest invariants, handler golden tests, engine state
machine, migration, API flows (246 tests).
https://claude.ai/code/session_016fp7YNMpLjqw8JUcceXp65
…eview - Sidebar + app-level scope bar (global/project + project selector, persisted) replace the per-page two-tier nav and scope plumbing. - One generic EntityPage drives all five kinds from entityKinds.js: list with per-tool StatusPill matrix, create/edit panel reusing the existing form components, and an Import tab showing items discovered in tools with source badges and bulk import. - DiffModal previews every sync as unified diffs before writing, surfaces drifted/conflict warnings with per-cell 'push anyway' / 'pull into registry' actions. - All tool metadata comes from GET /api/integrations — no hardcoded tool lists in JS (syncConfig.js and the five per-entity page modules are deleted). - MCP Registry browser kept, rewired to project ids. https://claude.ai/code/session_016fp7YNMpLjqw8JUcceXp65
One rules document in OpenSync syncs to every tool's instructions surface: - markdown_blocks handler writes managed marker blocks inside shared instruction files (CLAUDE.md, AGENTS.md, GEMINI.md, Devin global rules), preserving all hand-written content outside the markers. - markdown_dir gains rule styles: .cursor/rules/*.mdc (alwaysApply), .github/instructions/*.instructions.md (applyTo), .devin/rules/*.md. - Manifest entries for Claude Code, Codex (AGENTS.md), Copilot, Cursor, Devin, Gemini CLI, OpenCode; AGENTS.md project path is intentionally shared across the tools that read it. - Rules page in the UI with the same status matrix / diff / import flow. https://claude.ai/code/session_016fp7YNMpLjqw8JUcceXp65
- Move flat backend modules into a proper opensync/ package (PyPI name 'opensync' is taken; package is open-sync with both 'opensync' and 'open-sync' console scripts). - opensync.cli: --host/--port (auto-picks next free port), --no-browser, --db; opens the web UI on startup. - Hatch build hook bundles frontend/dist into the wheel as opensync/static (builds it with npm when missing); FastAPI serves it, falling back to frontend/dist in a source checkout. - Registry default moves to ~/.opensync/opensync.db (source checkouts keep using a repo-root db; OPENSYNC_DB_PATH still overrides). - Verified end to end: uvx --from dist/*.whl opensync serves API + UI and creates the db; 288 tests pass. https://claude.ai/code/session_016fp7YNMpLjqw8JUcceXp65
Contributor
Reviewer's GuideIntroduce OpenSync v2 with a manifest-driven sync engine, generic entity model, and redesigned UI, replacing the v1 per-feature APIs and wiring while adding skills/rules/commands/subagents support, project auto-import, diff-based sync plans, and MCP registry integration. Sequence diagram for v2 diff-based sync plan and applysequenceDiagram
actor User
participant EntityPage
participant SyncRouter as SyncRouter/api/sync
participant Engine as engine.plan_sync
participant Handlers as FormatHandlers
participant Store as store
participant Backup as backup
User->>EntityPage: Click "Preview sync"
EntityPage->>SyncRouter: POST /api/sync/plan
Note right of SyncRouter: kind, entity_ids, integrations, force
SyncRouter->>Engine: plan_sync(kind, entity_ids, integrations, force)
Engine->>Store: list_entities / get_sync_states
Engine->>Handlers: plan_write(root, models, options)
Handlers-->>Engine: [FileChange]
Engine->>Engine: compute diffs, warnings, SyncPlan
Engine-->>SyncRouter: SyncPlan(plan_id, changes, warnings)
SyncRouter-->>EntityPage: SyncPlan JSON
EntityPage->>User: Show DiffModal with file diffs
User->>EntityPage: Click "Apply"
EntityPage->>SyncRouter: POST /api/sync/apply {plan_id}
SyncRouter->>Engine: apply_plan(plan_id)
Engine->>Backup: create_backup_run(label, paths)
Backup-->>Engine: backup_dir, run_id
Engine->>Engine: write files atomically
Engine->>Store: set_sync_state(entity_id, integration, ...)
Engine-->>SyncRouter: ApplyResult(success, files_written, backup_dir)
SyncRouter-->>EntityPage: ApplyResult
EntityPage->>User: Toast "Wrote N file(s) (backup saved)"
ER diagram for v2 generic entity schema and sync stateerDiagram
projects_v2 {
string id PK
string name
string path
}
entities {
string id PK
string kind "mcp|skill|rule|command|subagent|llm"
string name
string scope "global|project"
string project_id FK
string description
string data "JSON blob (kind-specific)"
string content "Markdown body"
string created_at
string updated_at
}
sync_state {
string entity_id FK
string integration "integration id (e.g. claude_code)"
string scope
string project_id
string target_path
string synced_hash
string synced_at
}
backup_runs {
string id PK
string created_at
string label
string dir "backup directory path"
}
projects_v2 ||--o{ entities : "project_id"
entities ||--o{ sync_state : "entity_id"
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 3 security issues, 4 other issues, and left some high level feedback:
Security issues:
- Avoiding SQL string concatenation: untrusted input concatenated with raw SQL query can result in SQL Injection. In order to execute raw query safely, prepared statement should be used. SQLAlchemy provides TextualSQL to easily used prepared statement with named parameters. For complex SQL composition, use SQL Expression Language or Schema Definition Language. In most cases, SQLAlchemy ORM will be a better option. (link)
- Avoiding SQL string concatenation: untrusted input concatenated with raw SQL query can result in SQL Injection. In order to execute raw query safely, prepared statement should be used. SQLAlchemy provides TextualSQL to easily used prepared statement with named parameters. For complex SQL composition, use SQL Expression Language or Schema Definition Language. In most cases, SQLAlchemy ORM will be a better option. (link)
- Avoiding SQL string concatenation: untrusted input concatenated with raw SQL query can result in SQL Injection. In order to execute raw query safely, prepared statement should be used. SQLAlchemy provides TextualSQL to easily used prepared statement with named parameters. For complex SQL composition, use SQL Expression Language or Schema Definition Language. In most cases, SQLAlchemy ORM will be a better option. (link)
General comments:
- In
test_engine.py::test_discover_reads_fallback_pathsyou’re callingengine.paths.Path(proj), butPathisn’t exposed offengine.paths—import and usepathlib.Pathdirectly (or just use the existing project.path pathlib instance) so the test doesn’t fail at import time. - On the frontend, selections like
targetsandselectedinEntityPagepersist across scope/project changes; consider resetting these whenscopeorprojectIdchanges to avoid trying to sync stale selections to tools that don’t support the new scope and to keep the UX predictable.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `test_engine.py::test_discover_reads_fallback_paths` you’re calling `engine.paths.Path(proj)`, but `Path` isn’t exposed off `engine.paths`—import and use `pathlib.Path` directly (or just use the existing project.path pathlib instance) so the test doesn’t fail at import time.
- On the frontend, selections like `targets` and `selected` in `EntityPage` persist across scope/project changes; consider resetting these when `scope` or `projectId` changes to avoid trying to sync stale selections to tools that don’t support the new scope and to keep the UX predictable.
## Individual Comments
### Comment 1
<location path="backend/opensync/engine/handlers/json_mcp.py" line_range="92-98" />
<code_context>
+ name = "json_mcp"
+ kinds = frozenset({"mcp"})
+
+ def _load(self, root: Path) -> dict:
+ text = read_text_or_none(root)
+ if not text or not text.strip():
+ return {}
+ try:
+ data = json.loads(text)
+ except json.JSONDecodeError:
+ return {}
+ return data if isinstance(data, dict) else {}
</code_context>
<issue_to_address>
**issue (bug_risk):** Silently treating invalid JSON as empty risks clobbering user config on write.
In `_load`, JSON decode errors currently return `{}`, so `plan_write` will overwrite the file with only the `root_key` section and drop any other user keys if the file is briefly corrupted. It’d be safer to treat "missing/empty" differently from "unreadable"—for decode errors, either raise so callers can surface the problem, or skip writes for that integration (with a warning) instead of rewriting from an empty dict.
</issue_to_address>
### Comment 2
<location path="backend/opensync/routers/entities.py" line_range="71-77" />
<code_context>
+ return engine.status(_kind(url_kind), scope, project_id)
+
+
+@router.get("/{url_kind}")
+def list_entities(
+ url_kind: str,
+ scope: Optional[str] = Query(None),
+ project_id: Optional[str] = Query(None),
+):
+ return store.list_entities(_kind(url_kind), scope, project_id)
+
+
</code_context>
<issue_to_address>
**question:** Project-scoped listing allows project_id to be omitted, which may be surprising or inconsistent with other endpoints.
`scope` is optional and passed directly to `store.list_entities`. When `scope == "project"` and `project_id is None`, the store returns all project-scoped entities across projects, whereas other endpoints (e.g. `create_entity`) require `project_id` for project scope. If this endpoint should also be constrained to a single project, add validation to require `project_id` when `scope == "project"` and return 400 otherwise.
</issue_to_address>
### Comment 3
<location path="backend/tests/test_api.py" line_range="51-52" />
<code_context>
+ assert client.get("/api/mcp").json() == []
+
+
+def test_unknown_kind_404(client):
+ assert client.get("/api/widgets").status_code == 404
+
+
</code_context>
<issue_to_address>
**suggestion (testing):** Add API-level tests for sync endpoints, especially error conditions
Current tests cover CRUD, discovery/import, project auto-import, and a happy-path sync flow via the engine, but not the `/api/sync` endpoints themselves. Please add API-level tests that:
- `POST /api/sync/plan` with an invalid `kind` → assert 400 (covers `_KIND_ALIASES` validation).
- `POST /api/sync/plan` with missing/empty `entity_ids` or `integrations` → assert 400.
- `POST /api/sync/apply` with an expired/nonexistent `plan_id` → assert 409 and the expected `ApplyResult` error payload.
- `POST /api/sync/pull` for a non-existent entity/integration combination → assert 404.
This will exercise FastAPI-level validation and error handling around sync, not just the engine behavior.
Suggested implementation:
```python
updated = client.put(
f"/api/mcp/{entity['id']}", json={"name": "ctx", "command": "uvx"}
).json()
assert updated["data"]["command"] == "uvx"
assert client.delete(f"/api/mcp/{entity['id']}").status_code == 200
assert client.get("/api/mcp").json() == []
def test_sync_plan_invalid_kind_400(client):
# invalid kind should fail `_KIND_ALIASES` validation
resp = client.post(
"/api/sync/plan",
json={
"kind": "unknown-kind",
"entity_ids": ["entity-1"],
"integrations": ["integration-1"],
},
)
assert resp.status_code == 400
def test_sync_plan_missing_or_empty_fields_400(client):
# missing entity_ids
resp = client.post(
"/api/sync/plan",
json={
"kind": "project",
"integrations": ["integration-1"],
},
)
assert resp.status_code == 400
# empty entity_ids
resp = client.post(
"/api/sync/plan",
json={
"kind": "project",
"entity_ids": [],
"integrations": ["integration-1"],
},
)
assert resp.status_code == 400
# missing integrations
resp = client.post(
"/api/sync/plan",
json={
"kind": "project",
"entity_ids": ["entity-1"],
},
)
assert resp.status_code == 400
# empty integrations
resp = client.post(
"/api/sync/plan",
json={
"kind": "project",
"entity_ids": ["entity-1"],
"integrations": [],
},
)
assert resp.status_code == 400
def test_sync_apply_unknown_plan_409(client):
# applying a non-existent / expired plan should return a conflict with an error ApplyResult
resp = client.post(
"/api/sync/apply",
json={
"plan_id": "non-existent-plan-id",
},
)
assert resp.status_code == 409
body = resp.json()
# minimal shape assertions so this stays aligned with ApplyResult error payload
assert isinstance(body, dict)
assert body.get("ok") is False or body.get("status") == "error"
assert "error" in body
def test_sync_pull_unknown_entity_integration_404(client):
# pulling for a non-existent entity / integration combination should return 404
resp = client.post(
"/api/sync/pull",
json={
"kind": "project",
"entity_id": "non-existent-entity",
"integration": "non-existent-integration",
},
)
assert resp.status_code == 404
```
These tests assume the following JSON request shapes:
- `POST /api/sync/plan`: `{ "kind": str, "entity_ids": list[str], "integrations": list[str] }`
- `POST /api/sync/apply`: `{ "plan_id": str }`
- `POST /api/sync/pull`: `{ "kind": str, "entity_id": str, "integration": str }`
If your actual schemas differ (e.g. different field names, additional required fields, or nested structures), update the JSON payloads accordingly.
Likewise, if the `ApplyResult` error payload has a more specific contract (for example, a fixed `status` value or an `error` object with a `code` field), you can strengthen the assertions in `test_sync_apply_unknown_plan_409` to match that exact shape.
</issue_to_address>
### Comment 4
<location path="README.md" line_range="19-20" />
<code_context>
+- **Stateful sync, not blind overwrite** — OpenSync remembers what it synced where (per-item hashes). Every item shows its live status per tool:
</code_context>
<issue_to_address>
**suggestion:** Clarify the status legend entry for "not synced" to avoid the ambiguous `· · not synced` sequence.
The `· · not synced` legend entry is visually inconsistent and can look like a missing icon. Please either give "not synced" its own explicit symbol (e.g., `○ not synced`) or remove the extra dot so the pattern matches the other states and is unambiguous.
```suggestion
- **Stateful sync, not blind overwrite** — OpenSync remembers what it synced where (per-item hashes). Every item shows its live status per tool:
`✓ in sync · ↑ outdated (registry changed) · ↓ drifted (changed in the tool) · ⚠ conflict · ○ not synced · ✕ missing`
```
</issue_to_address>
### Comment 5
<location path="backend/opensync/db/migrate.py" line_range="146" />
<code_context>
conn.execute(f"DROP TABLE IF EXISTS {table}")
</code_context>
<issue_to_address>
**security (python.sqlalchemy.security.sqlalchemy-execute-raw-query):** Avoiding SQL string concatenation: untrusted input concatenated with raw SQL query can result in SQL Injection. In order to execute raw query safely, prepared statement should be used. SQLAlchemy provides TextualSQL to easily used prepared statement with named parameters. For complex SQL composition, use SQL Expression Language or Schema Definition Language. In most cases, SQLAlchemy ORM will be a better option.
*Source: opengrep*
</issue_to_address>
### Comment 6
<location path="backend/opensync/store.py" line_range="216-219" />
<code_context>
rows = conn.execute(
f"SELECT * FROM sync_state WHERE entity_id IN ({placeholders})",
entity_ids,
).fetchall()
</code_context>
<issue_to_address>
**security (python.sqlalchemy.security.sqlalchemy-execute-raw-query):** Avoiding SQL string concatenation: untrusted input concatenated with raw SQL query can result in SQL Injection. In order to execute raw query safely, prepared statement should be used. SQLAlchemy provides TextualSQL to easily used prepared statement with named parameters. For complex SQL composition, use SQL Expression Language or Schema Definition Language. In most cases, SQLAlchemy ORM will be a better option.
*Source: opengrep*
</issue_to_address>
### Comment 7
<location path="backend/opensync/store.py" line_range="279-281" />
<code_context>
conn.execute(
f"DELETE FROM backup_runs WHERE id NOT IN ({placeholders})", keep_ids
)
</code_context>
<issue_to_address>
**security (python.sqlalchemy.security.sqlalchemy-execute-raw-query):** Avoiding SQL string concatenation: untrusted input concatenated with raw SQL query can result in SQL Injection. In order to execute raw query safely, prepared statement should be used. SQLAlchemy provides TextualSQL to easily used prepared statement with named parameters. For complex SQL composition, use SQL Expression Language or Schema Definition Language. In most cases, SQLAlchemy ORM will be a better option.
*Source: opengrep*
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+92
to
+98
| def _load(self, root: Path) -> dict: | ||
| text = read_text_or_none(root) | ||
| if not text or not text.strip(): | ||
| return {} | ||
| try: | ||
| data = json.loads(text) | ||
| except json.JSONDecodeError: |
Contributor
There was a problem hiding this comment.
issue (bug_risk): Silently treating invalid JSON as empty risks clobbering user config on write.
In _load, JSON decode errors currently return {}, so plan_write will overwrite the file with only the root_key section and drop any other user keys if the file is briefly corrupted. It’d be safer to treat "missing/empty" differently from "unreadable"—for decode errors, either raise so callers can surface the problem, or skip writes for that integration (with a warning) instead of rewriting from an empty dict.
Comment on lines
+71
to
+77
| @router.get("/{url_kind}") | ||
| def list_entities( | ||
| url_kind: str, | ||
| scope: Optional[str] = Query(None), | ||
| project_id: Optional[str] = Query(None), | ||
| ): | ||
| return store.list_entities(_kind(url_kind), scope, project_id) |
Contributor
There was a problem hiding this comment.
question: Project-scoped listing allows project_id to be omitted, which may be surprising or inconsistent with other endpoints.
scope is optional and passed directly to store.list_entities. When scope == "project" and project_id is None, the store returns all project-scoped entities across projects, whereas other endpoints (e.g. create_entity) require project_id for project scope. If this endpoint should also be constrained to a single project, add validation to require project_id when scope == "project" and return 400 otherwise.
Comment on lines
+51
to
+52
| def test_unknown_kind_404(client): | ||
| assert client.get("/api/widgets").status_code == 404 |
Contributor
There was a problem hiding this comment.
suggestion (testing): Add API-level tests for sync endpoints, especially error conditions
Current tests cover CRUD, discovery/import, project auto-import, and a happy-path sync flow via the engine, but not the /api/sync endpoints themselves. Please add API-level tests that:
POST /api/sync/planwith an invalidkind→ assert 400 (covers_KIND_ALIASESvalidation).POST /api/sync/planwith missing/emptyentity_idsorintegrations→ assert 400.POST /api/sync/applywith an expired/nonexistentplan_id→ assert 409 and the expectedApplyResulterror payload.POST /api/sync/pullfor a non-existent entity/integration combination → assert 404.
This will exercise FastAPI-level validation and error handling around sync, not just the engine behavior.
Suggested implementation:
updated = client.put(
f"/api/mcp/{entity['id']}", json={"name": "ctx", "command": "uvx"}
).json()
assert updated["data"]["command"] == "uvx"
assert client.delete(f"/api/mcp/{entity['id']}").status_code == 200
assert client.get("/api/mcp").json() == []
def test_sync_plan_invalid_kind_400(client):
# invalid kind should fail `_KIND_ALIASES` validation
resp = client.post(
"/api/sync/plan",
json={
"kind": "unknown-kind",
"entity_ids": ["entity-1"],
"integrations": ["integration-1"],
},
)
assert resp.status_code == 400
def test_sync_plan_missing_or_empty_fields_400(client):
# missing entity_ids
resp = client.post(
"/api/sync/plan",
json={
"kind": "project",
"integrations": ["integration-1"],
},
)
assert resp.status_code == 400
# empty entity_ids
resp = client.post(
"/api/sync/plan",
json={
"kind": "project",
"entity_ids": [],
"integrations": ["integration-1"],
},
)
assert resp.status_code == 400
# missing integrations
resp = client.post(
"/api/sync/plan",
json={
"kind": "project",
"entity_ids": ["entity-1"],
},
)
assert resp.status_code == 400
# empty integrations
resp = client.post(
"/api/sync/plan",
json={
"kind": "project",
"entity_ids": ["entity-1"],
"integrations": [],
},
)
assert resp.status_code == 400
def test_sync_apply_unknown_plan_409(client):
# applying a non-existent / expired plan should return a conflict with an error ApplyResult
resp = client.post(
"/api/sync/apply",
json={
"plan_id": "non-existent-plan-id",
},
)
assert resp.status_code == 409
body = resp.json()
# minimal shape assertions so this stays aligned with ApplyResult error payload
assert isinstance(body, dict)
assert body.get("ok") is False or body.get("status") == "error"
assert "error" in body
def test_sync_pull_unknown_entity_integration_404(client):
# pulling for a non-existent entity / integration combination should return 404
resp = client.post(
"/api/sync/pull",
json={
"kind": "project",
"entity_id": "non-existent-entity",
"integration": "non-existent-integration",
},
)
assert resp.status_code == 404These tests assume the following JSON request shapes:
POST /api/sync/plan:{ "kind": str, "entity_ids": list[str], "integrations": list[str] }POST /api/sync/apply:{ "plan_id": str }POST /api/sync/pull:{ "kind": str, "entity_id": str, "integration": str }
If your actual schemas differ (e.g. different field names, additional required fields, or nested structures), update the JSON payloads accordingly.
Likewise, if the ApplyResult error payload has a more specific contract (for example, a fixed status value or an error object with a code field), you can strengthen the assertions in test_sync_apply_unknown_plan_409 to match that exact shape.
Comment on lines
+19
to
+20
| - **Stateful sync, not blind overwrite** — OpenSync remembers what it synced where (per-item hashes). Every item shows its live status per tool: | ||
| `✓ in sync · ↑ outdated (registry changed) · ↓ drifted (changed in the tool) · ⚠ conflict · · not synced · ✕ missing` |
Contributor
There was a problem hiding this comment.
suggestion: Clarify the status legend entry for "not synced" to avoid the ambiguous · · not synced sequence.
The · · not synced legend entry is visually inconsistent and can look like a missing icon. Please either give "not synced" its own explicit symbol (e.g., ○ not synced) or remove the extra dot so the pattern matches the other states and is unambiguous.
Suggested change
| - **Stateful sync, not blind overwrite** — OpenSync remembers what it synced where (per-item hashes). Every item shows its live status per tool: | |
| `✓ in sync · ↑ outdated (registry changed) · ↓ drifted (changed in the tool) · ⚠ conflict · · not synced · ✕ missing` | |
| - **Stateful sync, not blind overwrite** — OpenSync remembers what it synced where (per-item hashes). Every item shows its live status per tool: | |
| `✓ in sync · ↑ outdated (registry changed) · ↓ drifted (changed in the tool) · ⚠ conflict · ○ not synced · ✕ missing` |
| ) | ||
|
|
||
| for table in _V1_TABLES + ("projects",): | ||
| conn.execute(f"DROP TABLE IF EXISTS {table}") |
Contributor
There was a problem hiding this comment.
security (python.sqlalchemy.security.sqlalchemy-execute-raw-query): Avoiding SQL string concatenation: untrusted input concatenated with raw SQL query can result in SQL Injection. In order to execute raw query safely, prepared statement should be used. SQLAlchemy provides TextualSQL to easily used prepared statement with named parameters. For complex SQL composition, use SQL Expression Language or Schema Definition Language. In most cases, SQLAlchemy ORM will be a better option.
Source: opengrep
Comment on lines
+216
to
+219
| rows = conn.execute( | ||
| f"SELECT * FROM sync_state WHERE entity_id IN ({placeholders})", | ||
| entity_ids, | ||
| ).fetchall() |
Contributor
There was a problem hiding this comment.
security (python.sqlalchemy.security.sqlalchemy-execute-raw-query): Avoiding SQL string concatenation: untrusted input concatenated with raw SQL query can result in SQL Injection. In order to execute raw query safely, prepared statement should be used. SQLAlchemy provides TextualSQL to easily used prepared statement with named parameters. For complex SQL composition, use SQL Expression Language or Schema Definition Language. In most cases, SQLAlchemy ORM will be a better option.
Source: opengrep
Comment on lines
+279
to
+281
| conn.execute( | ||
| f"DELETE FROM backup_runs WHERE id NOT IN ({placeholders})", keep_ids | ||
| ) |
Contributor
There was a problem hiding this comment.
security (python.sqlalchemy.security.sqlalchemy-execute-raw-query): Avoiding SQL string concatenation: untrusted input concatenated with raw SQL query can result in SQL Injection. In order to execute raw query safely, prepared statement should be used. SQLAlchemy provides TextualSQL to easily used prepared statement with named parameters. For complex SQL composition, use SQL Expression Language or Schema Definition Language. In most cases, SQLAlchemy ORM will be a better option.
Source: opengrep
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (18)
backend/opensync/db/migrate.py (1)
145-146: 💤 Low valueThe SQL injection warning is a false positive — table names are hardcoded constants.
The
tablevariable iterates over_V1_TABLES + ("projects",), which contains only statically defined string literals. No user input reaches this f-string.However, consider the tuple unpacking style for consistency with Ruff's suggestion:
-for table in _V1_TABLES + ("projects",): +for table in (*_V1_TABLES, "projects"):🤖 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 `@backend/opensync/db/migrate.py` around lines 145 - 146, The SQL injection warning here is a false positive since table names come only from hardcoded constants in _V1_TABLES and the string literal "projects". However, to align with Ruff's style recommendations and follow safer string formatting practices, refactor the loop in the DROP TABLE section to use Ruff's preferred tuple unpacking or string formatting approach instead of the f-string for constructing the SQL query, even though the actual injection risk is zero due to the hardcoded nature of the table names.Source: Linters/SAST tools
backend/opensync/store.py (2)
82-84: ⚡ Quick winChain exceptions and remove extraneous
fprefix.The
f-string on line 83 has no placeholders. Also, re-raising withoutfrom errloses the original traceback context.Proposed fix
except sqlite3.IntegrityError: - raise ValueError(f"A project with that name or path already exists") + raise ValueError("A project with that name or path already exists") from NoneAnd similarly for the entity insert:
except sqlite3.IntegrityError: raise ValueError( f"{kind} '{name}' already exists in this scope" - ) + ) from NoneAlso applies to: 168-172
🤖 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 `@backend/opensync/store.py` around lines 82 - 84, The exception handling block for sqlite3.IntegrityError is missing proper exception chaining and has an unnecessary f-string prefix. In the except sqlite3.IntegrityError block around line 82-84, capture the exception as a variable (e.g., except sqlite3.IntegrityError as err:), remove the f prefix from the string literal since it contains no placeholders, and re-raise the ValueError using raise ValueError(...) from err to preserve the original exception traceback. Apply the same fix pattern to the other location mentioned in the comment (entity insert around lines 168-172) where a similar exception handling exists.Source: Linters/SAST tools
214-220: 💤 Low valueThe SQL injection warnings are false positives — only placeholder characters are interpolated.
The f-strings on lines 217 and 280 construct
"?,?,?"patterns from list lengths, not list values. The actual data flows safely through parameterizedexecute()calls.Adding brief inline comments could prevent future static-analysis churn:
placeholders = ",".join("?" * len(entity_ids)) with _db() as conn: rows = conn.execute( + # Placeholders are "?" literals, values passed as params f"SELECT * FROM sync_state WHERE entity_id IN ({placeholders})", entity_ids, ).fetchall()Also applies to: 275-281
🤖 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 `@backend/opensync/store.py` around lines 214 - 220, Add brief inline comments to the SQL construction logic in the query at lines 214-220 to clarify that the f-string only interpolates placeholder counts (not actual data values) from the length of entity_ids, while the actual entity_id values are safely passed as parameterized parameters to the execute() method. This will help suppress false-positive SQL injection warnings from static analysis tools. Apply the same clarifying comment pattern to the similar SQL construction at lines 275-281 where placeholders are built from list length.Source: Linters/SAST tools
backend/opensync/integrations/__init__.py (1)
11-11: ⚡ Quick winReplace EN DASH (–) with HYPHEN-MINUS (-) in comment.
The comment contains a Unicode EN DASH character (U+2013) where ASCII HYPHEN-MINUS (U+002D) should be used for consistency.
📝 Proposed fix
-from opensync.integrations.base import ( # noqa: F401 – re-exported +from opensync.integrations.base import ( # noqa: F401 - re-exported ENTITY_KINDS, SCOPES, EntityTarget,🤖 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 `@backend/opensync/integrations/__init__.py` at line 11, In the import statement that imports from opensync.integrations.base with the noqa comment, replace the Unicode EN DASH character (–) with a standard ASCII HYPHEN-MINUS (-) in the comment that reads "# noqa: F401 – re-exported" to maintain consistency with ASCII formatting standards.backend/opensync/engine/handlers/markdown_dir.py (1)
4-13: ⚡ Quick winReplace EN DASH (–) with HYPHEN-MINUS (-) in docstring.
The docstring contains Unicode EN DASH characters (U+2013) where ASCII HYPHEN-MINUS (U+002D) should be used for consistency and to avoid encoding issues.
📝 Proposed fix
Options: - suffix – file suffix incl. extension (".md", ".prompt.md", ".mdc"…) - frontmatter – field mapping style: - claude_command – description / argument-hint → CommandEntity - copilot_prompt – description → CommandEntity - plain – no frontmatter, body only → CommandEntity - claude_agent – name/description/model/tools → SubagentEntity - copilot_agent – name/description/tools → SubagentEntity - rule_md – description → RuleEntity - cursor_mdc – description + alwaysApply → RuleEntity - copilot_instructions – description + applyTo: "**" → RuleEntity + suffix - file suffix incl. extension (".md", ".prompt.md", ".mdc"…) + frontmatter - field mapping style: + claude_command - description / argument-hint → CommandEntity + copilot_prompt - description → CommandEntity + plain - no frontmatter, body only → CommandEntity + claude_agent - name/description/model/tools → SubagentEntity + copilot_agent - name/description/tools → SubagentEntity + rule_md - description → RuleEntity + cursor_mdc - description + alwaysApply → RuleEntity + copilot_instructions - description + applyTo: "**" → RuleEntity🤖 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 `@backend/opensync/engine/handlers/markdown_dir.py` around lines 4 - 13, In the docstring at the beginning of the markdown_dir.py file, replace all Unicode EN DASH characters (–, U+2013) with ASCII HYPHEN-MINUS characters (-) for consistency and to avoid encoding issues. Specifically, in the suffix and frontmatter documentation blocks, replace each instance of the "–" character used as a separator with a standard hyphen "-". This affects multiple lines in the docstring where dashes are used to separate field names from their descriptions.docs/CONTRIBUTING_INTEGRATIONS.md (1)
19-27: 💤 Low valueAdd language identifier to fenced code block.
The code block showing the directory structure should have a language identifier for better rendering and accessibility.
📝 Proposed fix
-``` +```text backend/integrations/ ├── __init__.py # ALL_INTEGRATIONS registry ├── base.py # Integration & EntityTarget models🤖 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 `@docs/CONTRIBUTING_INTEGRATIONS.md` around lines 19 - 27, The fenced code block displaying the directory structure for backend/integrations/ and backend/engine/handlers/ is missing a language identifier on the opening fence. Add the `text` language identifier immediately after the opening triple backticks (`````) to improve rendering and accessibility of the documentation.backend/tests/test_handlers.py (1)
288-292: 💤 Low valueMove
pytestimport to module level.The
import pyteststatement is inside the test function. While this works, it's more conventional to import test framework modules at the module level for consistency and readability.📦 Proposed fix
import json +import pytest import tomlkit from opensync.engine.handlers import HANDLERSdef test_llm_json_is_read_only(tmp_path): handler = HANDLERS["llm_json"] - import pytest with pytest.raises(NotImplementedError): handler.plan_write(tmp_path / "x.json", [], {})🤖 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 `@backend/tests/test_handlers.py` around lines 288 - 292, Move the `import pytest` statement from inside the test_llm_json_is_read_only function to the module level at the top of the file with other imports. This follows Python conventions where test framework imports belong at the module level rather than inside individual test functions.backend/opensync/routers/projects.py (2)
22-34: ⚡ Quick winPreserve exception chain in path validation.
Using
raise ... from excpreserves the traceback when project creation fails.♻️ Suggested improvement
try: project = store.add_project(name, str(resolved)) except ValueError as exc: - raise HTTPException(status_code=400, detail=str(exc)) + raise HTTPException(status_code=400, detail=str(exc)) from exc🤖 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 `@backend/opensync/routers/projects.py` around lines 22 - 34, In the add_project function, when catching the ValueError from store.add_project() and raising an HTTPException, preserve the exception chain by using `raise HTTPException(...) from exc` instead of just `raise HTTPException(...)`. This maintains the original traceback for better debugging and error context.
36-56: ⚡ Quick winAuto-import silently swallows all errors.
The
try-except-continuepattern ensures project creation never fails, but users receive no feedback when entity discovery or import fails (e.g., malformed config files, permission errors). Consider logging failures or collecting them in anerrorsfield of the response.💡 Suggested improvement
+import logging + +logger = logging.getLogger(__name__) + `@router.post`("/projects") def add_project(data: dict): # ... existing validation ... imported: dict[str, list[str]] = {} if data.get("auto_import", True): for kind in ENTITY_KINDS: try: discovered = engine.discover(kind, "project", project.id) specs = [...] entities = engine.import_items(kind, specs) if entities: imported[kind] = [e.name for e in entities] - except Exception: - continue # never fail project creation over one bad config + except Exception as exc: + logger.warning( + "Failed to auto-import %s for project %s: %s", + kind, + project.id, + exc, + exc_info=True + ) + continue🤖 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 `@backend/opensync/routers/projects.py` around lines 36 - 56, The try-except-continue pattern in the auto-import loop silently swallows all errors without logging or user feedback. Add logging (using an appropriate logger instance) to capture the exception details whenever an error occurs in the discovery or import_items operations within the loop over ENTITY_KINDS. Include the exception information in the log message so users can see which entity kinds failed and why. Optionally, also collect these errors in an errors collection and include them in the response alongside the imported field.backend/opensync/routers/mcp_registry.py (2)
24-40: ⚡ Quick winPreserve exception chains in error handlers.
Both exception handlers should use
raise ... from excto preserve the original traceback.♻️ Suggested improvement
try: return await mcp_registry_client.search_servers(q, cursor, limit) except httpx.HTTPStatusError as exc: raise HTTPException( status_code=exc.response.status_code, detail=f"MCP Registry error: {exc.response.text}", - ) + ) from exc except httpx.HTTPError as exc: raise HTTPException( status_code=502, detail=f"Failed to reach MCP Registry: {exc}" - ) + ) from exc🤖 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 `@backend/opensync/routers/mcp_registry.py` around lines 24 - 40, The search function's exception handlers are not preserving the original exception chain when raising HTTPException errors. In both the httpx.HTTPStatusError and httpx.HTTPError exception handlers, use the `raise ... from exc` syntax instead of just `raise` to maintain the traceback chain. This applies to both the HTTPException raised when catching HTTPStatusError and the HTTPException raised when catching HTTPError, allowing better debugging by preserving the full exception context.
43-61: ⚡ Quick winPreserve exception chains in import error handlers.
All three exception handlers should use
raise ... from exc.♻️ Suggested improvement
try: data = await mcp_registry_client.get_server_detail(req.server_name) except httpx.HTTPStatusError as exc: raise HTTPException( status_code=exc.response.status_code, detail=f"MCP Registry error: {exc.response.text}", - ) + ) from exc except httpx.HTTPError as exc: raise HTTPException( status_code=502, detail=f"Failed to reach MCP Registry: {exc}" - ) + ) from exc server = _transform_registry_server(data) try: return store.create_entity("mcp", server, req.scope, req.project_id) except ValueError as exc: - raise HTTPException(status_code=409, detail=str(exc)) + raise HTTPException(status_code=409, detail=str(exc)) from exc🤖 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 `@backend/opensync/routers/mcp_registry.py` around lines 43 - 61, The three exception handlers in the import_server function do not preserve exception chains when re-raising as HTTPException. Update each of the three exception handlers (for httpx.HTTPStatusError, httpx.HTTPError, and ValueError) to use the form "raise HTTPException(...) from exc" instead of just "raise HTTPException(...)" to maintain the exception chain context for better debugging and error traceability.backend/opensync/engine/engine.py (2)
75-81: ⚡ Quick winConsider logging handler read failures in
_read_target.The blind
Exceptioncatch silently treats any read error (I/O failure, permission denied, corrupted config) as an empty target. While this prevents crashes during drift detection, it hides real problems from users.💡 Suggested improvement
+import logging + +logger = logging.getLogger(__name__) + def _read_target(integration, target, project_dir) -> dict[str, BaseModel]: """Read the primary write path of a target (drift detection).""" handler = HANDLERS[target.handler] try: return handler.read(paths.resolve(target, project_dir), target.options) except Exception: + logger.debug( + "Failed to read target %s for %s", + paths.resolve(target, project_dir), + integration.id, + exc_info=True + ) return {}🤖 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 `@backend/opensync/engine/engine.py` around lines 75 - 81, The `_read_target` function catches all exceptions when calling handler.read() but silently returns an empty dictionary without logging the error, which hides real problems from users. Add a logging statement in the except block that captures and logs the exception details before returning the empty dictionary, so that I/O failures, permission errors, and other read issues are visible for debugging while still preventing crashes during drift detection.
84-95: ⚡ Quick winConsider logging discovery read failures in
_read_all_paths.The
try-except-continuepattern silently skips any path that fails to read. While this is appropriate for discovery (paths may not exist, configs may be invalid), logging at debug level would aid troubleshooting without changing behavior.💡 Suggested improvement
+import logging + +logger = logging.getLogger(__name__) + def _read_all_paths(integration, target, project_dir) -> dict[str, BaseModel]: """Read primary + read_paths (discovery). First occurrence wins.""" handler = HANDLERS[target.handler] merged: dict[str, BaseModel] = {} for path in paths.resolve_read_paths(target, project_dir): try: found = handler.read(path, target.options) except Exception: + logger.debug( + "Skipping unreadable path %s for %s", + path, + integration.id, + exc_info=True + ) continue for name, model in found.items(): merged.setdefault(name, model) return merged🤖 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 `@backend/opensync/engine/engine.py` around lines 84 - 95, The exception handling in the _read_all_paths function silently continues on read failures without logging, making it difficult to troubleshoot discovery issues. Add a debug-level log statement inside the except block that captures the path being read, the exception details, and any other relevant context (such as the target or handler information). This will provide visibility into why certain paths fail during discovery while preserving the existing behavior of continuing to the next path.backend/opensync/routers/entities.py (2)
37-41: ⚡ Quick winPreserve exception chain in
_canonical.Using
raise HTTPException(...) from excpreserves the traceback and makes debugging easier when validation fails.♻️ Suggested improvement
def _canonical(kind: str, payload: dict): try: return KIND_MODELS[kind].model_validate(payload) except ValidationError as exc: - raise HTTPException(status_code=422, detail=str(exc)) + raise HTTPException(status_code=422, detail=str(exc)) from exc🤖 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 `@backend/opensync/routers/entities.py` around lines 37 - 41, In the _canonical function, when raising the HTTPException after catching a ValidationError, use the `from exc` syntax to preserve the exception chain. Change the raise statement from `raise HTTPException(status_code=422, detail=str(exc))` to `raise HTTPException(status_code=422, detail=str(exc)) from exc` to maintain the original exception traceback, which aids debugging when validation fails.
80-93: ⚡ Quick winPreserve exception chain in
create_entity.Using
raise HTTPException(...) from excpreserves the traceback for debugging.♻️ Suggested improvement
try: return store.create_entity(kind, canonical, scope, project_id) except ValueError as exc: - raise HTTPException(status_code=409, detail=str(exc)) + raise HTTPException(status_code=409, detail=str(exc)) from exc🤖 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 `@backend/opensync/routers/entities.py` around lines 80 - 93, When catching the ValueError exception in the create_entity function and raising an HTTPException, the exception chain is not being preserved, which loses important traceback information for debugging. Modify the raise statement in the except ValueError block to use the `from exc` syntax (i.e., `raise HTTPException(...) from exc`) to explicitly preserve the original exception chain and maintain the full traceback context.README.md (1)
106-126: 💤 Low valueAdd language identifier to the architecture code block.
The code block at line 106 lacks a language specifier. Adding
```textor```bash(instead of just```) will improve syntax highlighting and satisfy markdown linters.🤖 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 `@README.md` around lines 106 - 126, The markdown code block in the README.md file is missing a language identifier after the opening triple backticks. Replace the bare ``` at the start of the code block with ```text to properly specify the content type, which will enable syntax highlighting and satisfy markdown linter requirements.Source: Linters/SAST tools
frontend/src/App.jsx (2)
29-29: 💤 Low valueToast ID collision risk (low probability).
Combining
Date.now()with an incrementing counter is generally safe, but if two toasts are added in the same millisecond (e.g., during a burst of errors),Date.now()returns the same value and the counter increment might not be enough to guarantee uniqueness if the counter resets or overflows.♻️ Alternative: use counter alone
- const id = Date.now() + ++toastId.current; + const id = ++toastId.current;The counter alone is sufficient for uniqueness within a session, and simpler.
🤖 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 `@frontend/src/App.jsx` at line 29, The toast ID generation in the App component combines Date.now() with an incrementing counter, which could cause collision risks when multiple toasts are created in the same millisecond. Replace the ID generation logic to use only the counter increment (toastId.current) alone, which is sufficient to guarantee uniqueness within a session and is simpler than the current approach. Remove the Date.now() call from the ID calculation and rely solely on the incrementing toastId.current counter.
48-51: 💤 Low valueConsider using async/await for consistency.
The integrations loading uses a promise chain (
.then().catch()) while other async operations in this file useasync/await. Mixing styles reduces readability.♻️ Refactor to async/await
- api.getIntegrations() - .then(setIntegrations) - .catch((err) => addToast(`Failed to load integrations: ${err.message}`, 'error')); + (async () => { + try { + setIntegrations(await api.getIntegrations()); + } catch (err) { + addToast(`Failed to load integrations: ${err.message}`, 'error'); + } + })();🤖 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 `@frontend/src/App.jsx` around lines 48 - 51, The api.getIntegrations() call uses promise chaining with .then().catch(), which is inconsistent with other async operations in the file that use async/await syntax. Refactor this code block to use an async/await pattern instead. Create an async function (likely within the useEffect hook) that awaits the api.getIntegrations() call and uses a try/catch block to handle errors, then call setIntegrations on success and addToast on error, maintaining the same error handling and toast notification behavior.
🤖 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 `@backend/opensync/engine/backup.py`:
- Around line 24-25: The _safe_name function uses string operations that only
handle POSIX forward slashes, leaving Windows backslashes and drive letters in
the filename, creating unsafe backup names. Replace the current implementation
by first converting the Path object to POSIX format using the as_posix() method
before applying the lstrip and replace operations, which will ensure the
function works correctly on both Windows and POSIX systems.
In `@backend/opensync/engine/handlers/toml_mcp.py`:
- Around line 51-62: The McpServer construction block (lines 51-62) lacks error
handling for malformed nested fields in the TOML entry, while earlier code
(lines 47-50) gracefully skips entries that fail dict conversion. Wrap the
entire McpServer instantiation in a try/except block to catch TypeErrors that
may occur when dict() is called on malformed env or headers fields (e.g., when
these are strings instead of dictionaries), and use continue to skip the
malformed entry rather than allowing the exception to crash the read() function.
This ensures consistent error handling throughout the function.
In `@backend/opensync/engine/handlers/yaml_workflow.py`:
- Around line 55-64: The plan_remove method only checks for files with the .yaml
extension when discovering files to remove, but the read method discovers both
.yaml and .yml files. To fix this inconsistency, modify the plan_remove method
to check for both .yaml and .yml extensions when constructing the path to read,
similar to how the read method handles both extensions. Update the path
construction logic to attempt to read both {name}.yaml and {name}.yml files and
include any that exist in the changes list for removal.
In `@backend/opensync/integrations/cursor.py`:
- Around line 9-10: Update the header comment on lines 9-10 to clarify that only
global "User Rules" from the settings UI are not managed here, while making
clear that project rules in the .cursor/rules/ directory ARE managed via the
rule target (which is defined at lines 43-51). The current comment is misleading
by stating rules are not managed without distinguishing between the two types of
rules that exist.
In `@backend/opensync/routers/fs.py`:
- Around line 43-58: The path processing on the return statement strips trailing
slashes using rstrip("/"), which converts the root directory "/" into an empty
string when a user selects it. Fix this by adding a conditional check: after
calling strip() on result.stdout, verify if the cleaned path equals "/" and
preserve it, otherwise apply rstrip("/") to remove only trailing slashes from
non-root paths. This ensures the root directory "/" is returned as-is rather
than becoming an empty string.
In `@backend/opensync/store.py`:
- Around line 189-196: The function `upsert_entity` declares a return type of
`-> Entity`, but the code path that calls `update_entity` on line 195 can return
`None` if the entity is deleted between the `find_entity` and `update_entity`
calls, creating a type inconsistency. Update the return type annotation of
`upsert_entity` to `Optional[Entity]` to correctly reflect that the function may
return `None` when `update_entity` returns `None`, ensuring the type signature
matches the actual runtime behavior.
In `@backend/tests/test_migrate.py`:
- Line 95: The assertion contains a dead condition with `or True` appended,
which causes it to always pass regardless of the actual value, making the
validation meaningless. Remove the `or True` portion from the assertion so that
the actual condition properly validates that the entity name equals "ghost".
In `@frontend/src/App.jsx`:
- Around line 17-18: The projectId state initialization reads from localStorage
which always returns a string, but the API may return numeric IDs, causing
strict equality comparisons to fail. Parse the projectId from localStorage to
ensure it matches the type returned by the API (if IDs are numeric, convert the
string to a number; if IDs are strings, ensure the API also returns strings).
Apply the same type conversion in any other locations where projectId is set
from user selections (such as in ScopeBar) to maintain consistency throughout
the application, ensuring that comparisons like the one checking if projectId
equals project.id work correctly.
In `@frontend/src/entityKinds.js`:
- Around line 66-71: In the entityToForm function, the spread operator order
causes fields from e.data to override the top-level entity fields (name,
description, content), resulting in incorrect form values when the backend
stores these fields within the data object. Reverse the spread order by moving
...e.data to the beginning of the object literal and placing the explicit name,
description, and content field assignments after it, so top-level fields take
precedence and override any conflicting fields from e.data.
In `@frontend/src/pages/ProjectsPage.jsx`:
- Around line 9-19: The form fields are cleared in the try block of the submit
function after the onAddProject call, which means they persist in the current
code. However, to ensure the form is only cleared on successful submission and
improve error handling, add a catch block to the submit function that handles
errors from onAddProject without clearing the form fields, and keep the
setName('') and setPath('') statements in the try block only after the await
onAddProject call succeeds. This way, if onAddProject throws an error, the catch
block will handle it and the form input will be preserved for the user to
correct and resubmit.
---
Nitpick comments:
In `@backend/opensync/db/migrate.py`:
- Around line 145-146: The SQL injection warning here is a false positive since
table names come only from hardcoded constants in _V1_TABLES and the string
literal "projects". However, to align with Ruff's style recommendations and
follow safer string formatting practices, refactor the loop in the DROP TABLE
section to use Ruff's preferred tuple unpacking or string formatting approach
instead of the f-string for constructing the SQL query, even though the actual
injection risk is zero due to the hardcoded nature of the table names.
In `@backend/opensync/engine/engine.py`:
- Around line 75-81: The `_read_target` function catches all exceptions when
calling handler.read() but silently returns an empty dictionary without logging
the error, which hides real problems from users. Add a logging statement in the
except block that captures and logs the exception details before returning the
empty dictionary, so that I/O failures, permission errors, and other read issues
are visible for debugging while still preventing crashes during drift detection.
- Around line 84-95: The exception handling in the _read_all_paths function
silently continues on read failures without logging, making it difficult to
troubleshoot discovery issues. Add a debug-level log statement inside the except
block that captures the path being read, the exception details, and any other
relevant context (such as the target or handler information). This will provide
visibility into why certain paths fail during discovery while preserving the
existing behavior of continuing to the next path.
In `@backend/opensync/engine/handlers/markdown_dir.py`:
- Around line 4-13: In the docstring at the beginning of the markdown_dir.py
file, replace all Unicode EN DASH characters (–, U+2013) with ASCII HYPHEN-MINUS
characters (-) for consistency and to avoid encoding issues. Specifically, in
the suffix and frontmatter documentation blocks, replace each instance of the
"–" character used as a separator with a standard hyphen "-". This affects
multiple lines in the docstring where dashes are used to separate field names
from their descriptions.
In `@backend/opensync/integrations/__init__.py`:
- Line 11: In the import statement that imports from opensync.integrations.base
with the noqa comment, replace the Unicode EN DASH character (–) with a standard
ASCII HYPHEN-MINUS (-) in the comment that reads "# noqa: F401 – re-exported" to
maintain consistency with ASCII formatting standards.
In `@backend/opensync/routers/entities.py`:
- Around line 37-41: In the _canonical function, when raising the HTTPException
after catching a ValidationError, use the `from exc` syntax to preserve the
exception chain. Change the raise statement from `raise
HTTPException(status_code=422, detail=str(exc))` to `raise
HTTPException(status_code=422, detail=str(exc)) from exc` to maintain the
original exception traceback, which aids debugging when validation fails.
- Around line 80-93: When catching the ValueError exception in the create_entity
function and raising an HTTPException, the exception chain is not being
preserved, which loses important traceback information for debugging. Modify the
raise statement in the except ValueError block to use the `from exc` syntax
(i.e., `raise HTTPException(...) from exc`) to explicitly preserve the original
exception chain and maintain the full traceback context.
In `@backend/opensync/routers/mcp_registry.py`:
- Around line 24-40: The search function's exception handlers are not preserving
the original exception chain when raising HTTPException errors. In both the
httpx.HTTPStatusError and httpx.HTTPError exception handlers, use the `raise ...
from exc` syntax instead of just `raise` to maintain the traceback chain. This
applies to both the HTTPException raised when catching HTTPStatusError and the
HTTPException raised when catching HTTPError, allowing better debugging by
preserving the full exception context.
- Around line 43-61: The three exception handlers in the import_server function
do not preserve exception chains when re-raising as HTTPException. Update each
of the three exception handlers (for httpx.HTTPStatusError, httpx.HTTPError, and
ValueError) to use the form "raise HTTPException(...) from exc" instead of just
"raise HTTPException(...)" to maintain the exception chain context for better
debugging and error traceability.
In `@backend/opensync/routers/projects.py`:
- Around line 22-34: In the add_project function, when catching the ValueError
from store.add_project() and raising an HTTPException, preserve the exception
chain by using `raise HTTPException(...) from exc` instead of just `raise
HTTPException(...)`. This maintains the original traceback for better debugging
and error context.
- Around line 36-56: The try-except-continue pattern in the auto-import loop
silently swallows all errors without logging or user feedback. Add logging
(using an appropriate logger instance) to capture the exception details whenever
an error occurs in the discovery or import_items operations within the loop over
ENTITY_KINDS. Include the exception information in the log message so users can
see which entity kinds failed and why. Optionally, also collect these errors in
an errors collection and include them in the response alongside the imported
field.
In `@backend/opensync/store.py`:
- Around line 82-84: The exception handling block for sqlite3.IntegrityError is
missing proper exception chaining and has an unnecessary f-string prefix. In the
except sqlite3.IntegrityError block around line 82-84, capture the exception as
a variable (e.g., except sqlite3.IntegrityError as err:), remove the f prefix
from the string literal since it contains no placeholders, and re-raise the
ValueError using raise ValueError(...) from err to preserve the original
exception traceback. Apply the same fix pattern to the other location mentioned
in the comment (entity insert around lines 168-172) where a similar exception
handling exists.
- Around line 214-220: Add brief inline comments to the SQL construction logic
in the query at lines 214-220 to clarify that the f-string only interpolates
placeholder counts (not actual data values) from the length of entity_ids, while
the actual entity_id values are safely passed as parameterized parameters to the
execute() method. This will help suppress false-positive SQL injection warnings
from static analysis tools. Apply the same clarifying comment pattern to the
similar SQL construction at lines 275-281 where placeholders are built from list
length.
In `@backend/tests/test_handlers.py`:
- Around line 288-292: Move the `import pytest` statement from inside the
test_llm_json_is_read_only function to the module level at the top of the file
with other imports. This follows Python conventions where test framework imports
belong at the module level rather than inside individual test functions.
In `@docs/CONTRIBUTING_INTEGRATIONS.md`:
- Around line 19-27: The fenced code block displaying the directory structure
for backend/integrations/ and backend/engine/handlers/ is missing a language
identifier on the opening fence. Add the `text` language identifier immediately
after the opening triple backticks (`````) to improve rendering and
accessibility of the documentation.
In `@frontend/src/App.jsx`:
- Line 29: The toast ID generation in the App component combines Date.now() with
an incrementing counter, which could cause collision risks when multiple toasts
are created in the same millisecond. Replace the ID generation logic to use only
the counter increment (toastId.current) alone, which is sufficient to guarantee
uniqueness within a session and is simpler than the current approach. Remove the
Date.now() call from the ID calculation and rely solely on the incrementing
toastId.current counter.
- Around line 48-51: The api.getIntegrations() call uses promise chaining with
.then().catch(), which is inconsistent with other async operations in the file
that use async/await syntax. Refactor this code block to use an async/await
pattern instead. Create an async function (likely within the useEffect hook)
that awaits the api.getIntegrations() call and uses a try/catch block to handle
errors, then call setIntegrations on success and addToast on error, maintaining
the same error handling and toast notification behavior.
In `@README.md`:
- Around line 106-126: The markdown code block in the README.md file is missing
a language identifier after the opening triple backticks. Replace the bare ```
at the start of the code block with ```text to properly specify the content
type, which will enable syntax highlighting and satisfy markdown linter
requirements.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 36455d30-c444-4190-8f68-1b52672bc342
⛔ Files ignored due to path filters (1)
frontend/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (124)
.gitignoreREADME.mdbackend/README.mdbackend/agent_discovery.pybackend/agent_registry.pybackend/api.pybackend/config_manager.pybackend/config_targets.pybackend/database.pybackend/hatch_build.pybackend/integrations/__init__.pybackend/integrations/antigravity.pybackend/integrations/base.pybackend/integrations/claude_code.pybackend/integrations/claude_desktop.pybackend/integrations/copilot_cli.pybackend/integrations/cursor.pybackend/integrations/gemini_cli.pybackend/integrations/opencode.pybackend/integrations/vscode.pybackend/integrations/warp.pybackend/integrations/windsurf.pybackend/llm_provider_discovery.pybackend/llm_provider_registry.pybackend/models.pybackend/opensync/__init__.pybackend/opensync/cli.pybackend/opensync/db/__init__.pybackend/opensync/db/migrate.pybackend/opensync/db/schema.sqlbackend/opensync/engine/__init__.pybackend/opensync/engine/backup.pybackend/opensync/engine/engine.pybackend/opensync/engine/frontmatter.pybackend/opensync/engine/handlers/__init__.pybackend/opensync/engine/handlers/base.pybackend/opensync/engine/handlers/json_mcp.pybackend/opensync/engine/handlers/llm_json.pybackend/opensync/engine/handlers/markdown_blocks.pybackend/opensync/engine/handlers/markdown_dir.pybackend/opensync/engine/handlers/skill_dir.pybackend/opensync/engine/handlers/toml_command.pybackend/opensync/engine/handlers/toml_mcp.pybackend/opensync/engine/handlers/yaml_workflow.pybackend/opensync/engine/hash.pybackend/opensync/engine/paths.pybackend/opensync/integrations/__init__.pybackend/opensync/integrations/antigravity.pybackend/opensync/integrations/base.pybackend/opensync/integrations/claude_code.pybackend/opensync/integrations/claude_desktop.pybackend/opensync/integrations/codex.pybackend/opensync/integrations/copilot_cli.pybackend/opensync/integrations/cursor.pybackend/opensync/integrations/devin.pybackend/opensync/integrations/gemini_cli.pybackend/opensync/integrations/opencode.pybackend/opensync/integrations/vscode.pybackend/opensync/integrations/warp.pybackend/opensync/main.pybackend/opensync/mcp_registry_client.pybackend/opensync/models.pybackend/opensync/routers/__init__.pybackend/opensync/routers/entities.pybackend/opensync/routers/fs.pybackend/opensync/routers/integrations.pybackend/opensync/routers/mcp_registry.pybackend/opensync/routers/projects.pybackend/opensync/routers/sync.pybackend/opensync/store.pybackend/project_importer.pybackend/project_registry.pybackend/pyproject.tomlbackend/server_registry.pybackend/skill_discovery.pybackend/skill_registry.pybackend/tests/_helpers.pybackend/tests/conftest.pybackend/tests/test_api.pybackend/tests/test_config_manager.pybackend/tests/test_engine.pybackend/tests/test_handlers.pybackend/tests/test_llm_provider_discovery.pybackend/tests/test_manifests.pybackend/tests/test_migrate.pybackend/tests/test_project_registry.pybackend/tests/test_server_registry.pybackend/unified_targets.pybackend/workflow_discovery.pybackend/workflow_registry.pydocs/CONTRIBUTING_INTEGRATIONS.mdfrontend/src/App.jsxfrontend/src/api.jsfrontend/src/components/DiffModal.jsxfrontend/src/components/NavBar.jsxfrontend/src/components/ScopeBar.jsxfrontend/src/components/StatusPill.jsxfrontend/src/components/cards/AgentCard.jsxfrontend/src/components/cards/LlmProviderCard.jsxfrontend/src/components/cards/RegistryServerCard.jsxfrontend/src/components/cards/ResourceCard.jsxfrontend/src/components/cards/ServerCard.jsxfrontend/src/components/cards/SkillCard.jsxfrontend/src/components/cards/WorkflowCard.jsxfrontend/src/components/forms/CommandForm.jsxfrontend/src/components/forms/RuleForm.jsxfrontend/src/components/layout/Sidebar.jsxfrontend/src/components/modals/ImportFromGlobalModal.jsxfrontend/src/components/modals/ImportFromProjectModal.jsxfrontend/src/components/modals/ImportItemFromGlobalModal.jsxfrontend/src/components/sync/SyncPage.jsxfrontend/src/components/sync/TargetSelector.jsxfrontend/src/components/sync/syncConfig.jsfrontend/src/entityKinds.jsfrontend/src/index.cssfrontend/src/pages/EntityPage.jsxfrontend/src/pages/McpRegistryBrowserPage.jsxfrontend/src/pages/ProjectsPage.jsxfrontend/src/pages/agents/AgentsPages.jsxfrontend/src/pages/llm/LlmProvidersPages.jsxfrontend/src/pages/servers/ServerRegistryPages.jsxfrontend/src/pages/skills/SkillsPages.jsxfrontend/src/pages/workflows/WorkflowsPages.jsxrun.sh
💤 Files with no reviewable changes (53)
- frontend/src/components/sync/TargetSelector.jsx
- backend/integrations/claude_code.py
- frontend/src/components/modals/ImportFromProjectModal.jsx
- backend/integrations/claude_desktop.py
- backend/integrations/antigravity.py
- backend/workflow_registry.py
- frontend/src/components/modals/ImportFromGlobalModal.jsx
- frontend/src/components/cards/LlmProviderCard.jsx
- backend/integrations/warp.py
- backend/integrations/cursor.py
- frontend/src/components/modals/ImportItemFromGlobalModal.jsx
- frontend/src/components/sync/syncConfig.js
- backend/integrations/base.py
- frontend/src/components/cards/ServerCard.jsx
- backend/config_manager.py
- backend/tests/test_project_registry.py
- backend/tests/test_config_manager.py
- backend/integrations/init.py
- backend/workflow_discovery.py
- backend/config_targets.py
- backend/unified_targets.py
- backend/llm_provider_discovery.py
- backend/agent_discovery.py
- frontend/src/components/cards/RegistryServerCard.jsx
- backend/project_importer.py
- frontend/src/components/cards/AgentCard.jsx
- frontend/src/components/cards/SkillCard.jsx
- backend/agent_registry.py
- backend/project_registry.py
- backend/skill_discovery.py
- backend/integrations/windsurf.py
- backend/server_registry.py
- backend/integrations/opencode.py
- frontend/src/pages/servers/ServerRegistryPages.jsx
- backend/integrations/gemini_cli.py
- frontend/src/components/NavBar.jsx
- backend/llm_provider_registry.py
- backend/integrations/copilot_cli.py
- frontend/src/components/cards/ResourceCard.jsx
- backend/tests/_helpers.py
- backend/skill_registry.py
- frontend/src/pages/skills/SkillsPages.jsx
- frontend/src/components/cards/WorkflowCard.jsx
- backend/models.py
- backend/tests/test_server_registry.py
- frontend/src/pages/workflows/WorkflowsPages.jsx
- frontend/src/pages/agents/AgentsPages.jsx
- backend/database.py
- frontend/src/components/sync/SyncPage.jsx
- frontend/src/pages/llm/LlmProvidersPages.jsx
- backend/integrations/vscode.py
- backend/tests/test_llm_provider_discovery.py
- backend/api.py
Comment on lines
+24
to
+25
| def _safe_name(path: Path) -> str: | ||
| return str(path).lstrip("/").replace("/", "__") |
There was a problem hiding this comment.
Cross-platform path handling broken on Windows.
str(path).lstrip("/").replace("/", "__") only handles POSIX-style forward slashes. On Windows, Path("C:\\foo\\bar") becomes the string "C:\\foo\\bar", which lstrip("/") leaves unchanged, and replace("/", "__") doesn't touch backslashes—resulting in unsafe backup filenames with embedded backslashes and drive letters.
🔧 Proposed fix using as_posix()
def _safe_name(path: Path) -> str:
- return str(path).lstrip("/").replace("/", "__")
+ return path.as_posix().lstrip("/").replace("/", "__")🤖 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 `@backend/opensync/engine/backup.py` around lines 24 - 25, The _safe_name
function uses string operations that only handle POSIX forward slashes, leaving
Windows backslashes and drive letters in the filename, creating unsafe backup
names. Replace the current implementation by first converting the Path object to
POSIX format using the as_posix() method before applying the lstrip and replace
operations, which will ensure the function works correctly on both Windows and
POSIX systems.
Comment on lines
+51
to
+62
| result[name] = McpServer( | ||
| name=str(name), | ||
| command=fields.get("command"), | ||
| args=list(fields.get("args") or []), | ||
| env={str(k): str(v) for k, v in dict(fields.get("env") or {}).items()}, | ||
| type=fields.get("type"), | ||
| url=fields.get("url"), | ||
| headers={ | ||
| str(k): str(v) | ||
| for k, v in dict(fields.get("headers") or {}).items() | ||
| }, | ||
| ) |
There was a problem hiding this comment.
Inconsistent error handling for malformed nested fields.
Lines 47-50 gracefully skip entries that fail dict(entry) conversion, but the McpServer construction here is not wrapped in try/except. If a TOML entry contains malformed env or headers fields (e.g., env = "string" instead of env = { K = "v" }), the dict(...) calls on lines 55 and 59-60 will raise TypeError, causing read() to crash rather than skip the malformed entry.
For user-editable config files, wrapping the construction in try/except and continuing on failure would improve robustness and consistency.
🛡️ Proposed fix to add defensive error handling
for name, entry in table.items():
try:
fields = dict(entry)
- except (TypeError, ValueError):
- continue
- result[name] = McpServer(
- name=str(name),
- command=fields.get("command"),
- args=list(fields.get("args") or []),
- env={str(k): str(v) for k, v in dict(fields.get("env") or {}).items()},
- type=fields.get("type"),
- url=fields.get("url"),
- headers={
- str(k): str(v)
- for k, v in dict(fields.get("headers") or {}).items()
- },
- )
+ result[name] = McpServer(
+ name=str(name),
+ command=fields.get("command"),
+ args=list(fields.get("args") or []),
+ env={str(k): str(v) for k, v in dict(fields.get("env") or {}).items()},
+ type=fields.get("type"),
+ url=fields.get("url"),
+ headers={
+ str(k): str(v)
+ for k, v in dict(fields.get("headers") or {}).items()
+ },
+ )
+ except (TypeError, ValueError):
+ continue🤖 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 `@backend/opensync/engine/handlers/toml_mcp.py` around lines 51 - 62, The
McpServer construction block (lines 51-62) lacks error handling for malformed
nested fields in the TOML entry, while earlier code (lines 47-50) gracefully
skips entries that fail dict conversion. Wrap the entire McpServer instantiation
in a try/except block to catch TypeErrors that may occur when dict() is called
on malformed env or headers fields (e.g., when these are strings instead of
dictionaries), and use continue to skip the malformed entry rather than allowing
the exception to crash the read() function. This ensures consistent error
handling throughout the function.
Comment on lines
+55
to
+64
| def plan_remove( | ||
| self, root: Path, names: list[str], opts: dict | ||
| ) -> list[FileChange]: | ||
| changes = [] | ||
| for name in names: | ||
| path = root / f"{name}.yaml" | ||
| before = read_text_or_none(path) | ||
| if before is not None: | ||
| changes.append(FileChange(path=path, before=before, after=None)) | ||
| return changes |
There was a problem hiding this comment.
Verify handling of .yml extension during removal.
The read method discovers both *.yaml and *.yml files (line 22), but plan_remove only targets *.yaml (line 60). If a workflow was manually created with a .yml extension, it would be discovered but not removed. Consider checking for both extensions during removal or documenting that only .yaml is supported for write operations.
🔧 Proposed fix to handle both extensions
def plan_remove(
self, root: Path, names: list[str], opts: dict
) -> list[FileChange]:
changes = []
for name in names:
- path = root / f"{name}.yaml"
- before = read_text_or_none(path)
- if before is not None:
- changes.append(FileChange(path=path, before=before, after=None))
+ for ext in [".yaml", ".yml"]:
+ path = root / f"{name}{ext}"
+ before = read_text_or_none(path)
+ if before is not None:
+ changes.append(FileChange(path=path, before=before, after=None))
return changes📝 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.
Suggested change
| def plan_remove( | |
| self, root: Path, names: list[str], opts: dict | |
| ) -> list[FileChange]: | |
| changes = [] | |
| for name in names: | |
| path = root / f"{name}.yaml" | |
| before = read_text_or_none(path) | |
| if before is not None: | |
| changes.append(FileChange(path=path, before=before, after=None)) | |
| return changes | |
| def plan_remove( | |
| self, root: Path, names: list[str], opts: dict | |
| ) -> list[FileChange]: | |
| changes = [] | |
| for name in names: | |
| for ext in [".yaml", ".yml"]: | |
| path = root / f"{name}{ext}" | |
| before = read_text_or_none(path) | |
| if before is not None: | |
| changes.append(FileChange(path=path, before=before, after=None)) | |
| return changes |
🤖 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 `@backend/opensync/engine/handlers/yaml_workflow.py` around lines 55 - 64, The
plan_remove method only checks for files with the .yaml extension when
discovering files to remove, but the read method discovers both .yaml and .yml
files. To fix this inconsistency, modify the plan_remove method to check for
both .yaml and .yml extensions when constructing the path to read, similar to
how the read method handles both extensions. Update the path construction logic
to attempt to read both {name}.yaml and {name}.yml files and include any that
exist in the changes list for removal.
Comment on lines
+9
to
+10
| # Rules (.cursor/rules/*.mdc) and the global "User Rules" (settings UI only) | ||
| # are not managed here. |
There was a problem hiding this comment.
Clarify the header comment about rules management.
The header comment states that rules "are not managed here," but project rules ARE managed via the rule target (lines 43-51). The intended meaning—that only global "User Rules" are unmanaged—is clarified in the target notes but not in the header.
📝 Proposed clarification
-# Rules (.cursor/rules/*.mdc) and the global "User Rules" (settings UI only)
-# are not managed here.
+# Rules: project rules sync to .cursor/rules/*.mdc. Global "User Rules"
+# live in the Cursor settings UI and are not file-synced.📝 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.
Suggested change
| # Rules (.cursor/rules/*.mdc) and the global "User Rules" (settings UI only) | |
| # are not managed here. | |
| # Rules: project rules sync to .cursor/rules/*.mdc. Global "User Rules" | |
| # live in the Cursor settings UI and are not file-synced. |
🤖 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 `@backend/opensync/integrations/cursor.py` around lines 9 - 10, Update the
header comment on lines 9-10 to clarify that only global "User Rules" from the
settings UI are not managed here, while making clear that project rules in the
.cursor/rules/ directory ARE managed via the rule target (which is defined at
lines 43-51). The current comment is misleading by stating rules are not managed
without distinguishing between the two types of rules that exist.
Comment on lines
+43
to
+58
| try: | ||
| result = subprocess.run( | ||
| [ | ||
| "osascript", | ||
| "-e", | ||
| 'POSIX path of (choose folder with prompt "Select project directory")', | ||
| ], | ||
| capture_output=True, | ||
| text=True, | ||
| timeout=120, | ||
| ) | ||
| if result.returncode != 0: | ||
| return {"path": None} | ||
| return {"path": result.stdout.strip().rstrip("/")} | ||
| except subprocess.TimeoutExpired: | ||
| return {"path": None} |
There was a problem hiding this comment.
Edge case: selecting root directory "/" results in empty path.
Line 56 strips trailing slashes with .rstrip("/"). If a user selects the root directory, the result becomes an empty string instead of "/".
🛠️ Proposed fix to preserve root directory
if result.returncode != 0:
return {"path": None}
- return {"path": result.stdout.strip().rstrip("/")}
+ selected = result.stdout.strip()
+ return {"path": selected if selected == "/" else selected.rstrip("/")}📝 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.
Suggested change
| try: | |
| result = subprocess.run( | |
| [ | |
| "osascript", | |
| "-e", | |
| 'POSIX path of (choose folder with prompt "Select project directory")', | |
| ], | |
| capture_output=True, | |
| text=True, | |
| timeout=120, | |
| ) | |
| if result.returncode != 0: | |
| return {"path": None} | |
| return {"path": result.stdout.strip().rstrip("/")} | |
| except subprocess.TimeoutExpired: | |
| return {"path": None} | |
| try: | |
| result = subprocess.run( | |
| [ | |
| "osascript", | |
| "-e", | |
| 'POSIX path of (choose folder with prompt "Select project directory")', | |
| ], | |
| capture_output=True, | |
| text=True, | |
| timeout=120, | |
| ) | |
| if result.returncode != 0: | |
| return {"path": None} | |
| selected = result.stdout.strip() | |
| return {"path": selected if selected == "/" else selected.rstrip("/")} | |
| except subprocess.TimeoutExpired: | |
| return {"path": None} |
🧰 Tools
🪛 ast-grep (0.43.0)
[error] 43-52: Command coming from incoming request
Context: subprocess.run(
[
"osascript",
"-e",
'POSIX path of (choose folder with prompt "Select project directory")',
],
capture_output=True,
text=True,
timeout=120,
)
Note: [CWE-20].
(subprocess-from-request)
[error] 43-52: Use of unsanitized data to create processes
Context: subprocess.run(
[
"osascript",
"-e",
'POSIX path of (choose folder with prompt "Select project directory")',
],
capture_output=True,
text=True,
timeout=120,
)
Note: [CWE-78].
(os-system-unsanitized-data)
🪛 Ruff (0.15.15)
[error] 45-49: Starting a process with a partial executable path
(S607)
🤖 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 `@backend/opensync/routers/fs.py` around lines 43 - 58, The path processing on
the return statement strips trailing slashes using rstrip("/"), which converts
the root directory "/" into an empty string when a user selects it. Fix this by
adding a conditional check: after calling strip() on result.stdout, verify if
the cleaned path equals "/" and preserve it, otherwise apply rstrip("/") to
remove only trailing slashes from non-root paths. This ensures the root
directory "/" is returned as-is rather than becoming an empty string.
Comment on lines
+189
to
+196
| def upsert_entity( | ||
| kind: str, canonical: BaseModel, scope: str, project_id: Optional[str] | ||
| ) -> Entity: | ||
| name = canonical.model_dump()["name"] | ||
| existing = find_entity(kind, name, scope, project_id) | ||
| if existing: | ||
| return update_entity(existing.id, canonical) | ||
| return create_entity(kind, canonical, scope, project_id) |
There was a problem hiding this comment.
Potential type inconsistency: upsert_entity may return None.
If another process deletes the entity between find_entity and update_entity, line 195 returns None (from update_entity), but the function signature declares -> Entity.
In a single-user local SQLite context this is unlikely, but for type safety consider:
def upsert_entity(
kind: str, canonical: BaseModel, scope: str, project_id: Optional[str]
) -> Entity:
name = canonical.model_dump()["name"]
existing = find_entity(kind, name, scope, project_id)
if existing:
- return update_entity(existing.id, canonical)
+ updated = update_entity(existing.id, canonical)
+ if updated is None:
+ # Entity was deleted between find and update; create it
+ return create_entity(kind, canonical, scope, project_id)
+ return updated
return create_entity(kind, canonical, scope, project_id)🤖 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 `@backend/opensync/store.py` around lines 189 - 196, The function
`upsert_entity` declares a return type of `-> Entity`, but the code path that
calls `update_entity` on line 195 can return `None` if the entity is deleted
between the `find_entity` and `update_entity` calls, creating a type
inconsistency. Update the return type annotation of `upsert_entity` to
`Optional[Entity]` to correctly reflect that the function may return `None` when
`update_entity` returns `None`, ensuring the type signature matches the actual
runtime behavior.
| demo = next(p for p in store.list_projects() if p.name == "demo") | ||
| assert proj_srv.project_id == demo.id | ||
|
|
||
| assert store.list_entities("skill")[0].name == "ghost" or True |
There was a problem hiding this comment.
Dead assertion — or True makes this check always pass.
This line appears to be leftover debug code. The assertion can never fail.
- assert store.list_entities("skill")[0].name == "ghost" or TrueLines 96-97 already validate the skills set correctly.
📝 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.
Suggested change
| assert store.list_entities("skill")[0].name == "ghost" or True |
🤖 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 `@backend/tests/test_migrate.py` at line 95, The assertion contains a dead
condition with `or True` appended, which causes it to always pass regardless of
the actual value, making the validation meaningless. Remove the `or True`
portion from the assertion so that the actual condition properly validates that
the entity name equals "ghost".
Comment on lines
+17
to
+18
| const [scope, setScope] = useState(() => localStorage.getItem('opensync.scope') || 'global'); | ||
| const [projectId, setProjectId] = useState(() => localStorage.getItem('opensync.projectId') || null); |
There was a problem hiding this comment.
Ensure consistent type handling for projectId across localStorage, select, and API.
localStorage.getItem() always returns a string (or null if missing). If the API returns numeric IDs, you'll have a type mismatch:
- When loaded from localStorage:
projectIdis a string (e.g.,"123") - When loaded from the API:
project.idmight be a number (e.g.,123) - The comparison at line 77 (
if (projectId === project.id)) would fail due to strict equality
🔧 Recommended fix to ensure type consistency
Parse the stored ID or ensure the API always returns string IDs. If IDs are strings from the API, this is already correct. Otherwise:
- const [projectId, setProjectId] = useState(() => localStorage.getItem('opensync.projectId') || null);
+ const [projectId, setProjectId] = useState(() => {
+ const stored = localStorage.getItem('opensync.projectId');
+ return stored ? stored : null; // Keep as string or parse if API uses numbers
+ });Alternatively, if the backend uses numeric IDs, ensure the select in ScopeBar.jsx also converts:
// In ScopeBar.jsx line 25:
- onChange={(e) => setProjectId(e.target.value || null)}
+ onChange={(e) => setProjectId(e.target.value ? String(e.target.value) : null)}And verify that project.id from the API is consistently a string.
📝 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.
Suggested change
| const [scope, setScope] = useState(() => localStorage.getItem('opensync.scope') || 'global'); | |
| const [projectId, setProjectId] = useState(() => localStorage.getItem('opensync.projectId') || null); | |
| const [scope, setScope] = useState(() => localStorage.getItem('opensync.scope') || 'global'); | |
| const [projectId, setProjectId] = useState(() => { | |
| const stored = localStorage.getItem('opensync.projectId'); | |
| return stored ? stored : null; // Keep as string or parse if API uses numbers | |
| }); |
🤖 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 `@frontend/src/App.jsx` around lines 17 - 18, The projectId state
initialization reads from localStorage which always returns a string, but the
API may return numeric IDs, causing strict equality comparisons to fail. Parse
the projectId from localStorage to ensure it matches the type returned by the
API (if IDs are numeric, convert the string to a number; if IDs are strings,
ensure the API also returns strings). Apply the same type conversion in any
other locations where projectId is set from user selections (such as in
ScopeBar) to maintain consistency throughout the application, ensuring that
comparisons like the one checking if projectId equals project.id work correctly.
Comment on lines
+66
to
+71
| export const entityToForm = (e) => ({ | ||
| name: e.name, | ||
| description: e.description, | ||
| content: e.content, | ||
| ...e.data, | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Search for potential field shadowing in entity data structures
# Check backend models and API responses for data field definitions
rg -n -C3 --type=py 'data.*=.*\{' backend/opensync/Repository: brandon-braner/open_sync
Length of output: 1817
🏁 Script executed:
# Check where entityToForm is called in the frontend
rg -n 'entityToForm' frontend/ --type=js --type=ts
# Also check the entity structure in API calls or mocks
rg -n -A5 -B5 'export.*entity' frontend/src/ --type=jsRepository: brandon-braner/open_sync
Length of output: 986
🏁 Script executed:
# Check for entity creation/fetching in the API or services
fd -e js -e ts frontend/ | xargs rg -l 'entity|Entity' | head -10Repository: brandon-braner/open_sync
Length of output: 715
🏁 Script executed:
# Check the API response structure and how entities are fetched
cat frontend/src/api.js | head -100Repository: brandon-braner/open_sync
Length of output: 3369
🏁 Script executed:
# Check backend models to understand entity structure
rg -n -A15 'class.*Entity|def.*entity' backend/opensync/models.py --type=py | head -50Repository: brandon-braner/open_sync
Length of output: 1362
🏁 Script executed:
# Look at yaml_workflow more carefully to see what ends up in data
cat -n backend/opensync/engine/handlers/yaml_workflow.py | sed -n '40,60p'Repository: brandon-braner/open_sync
Length of output: 988
🏁 Script executed:
# Check the backend routers to see how entities are serialized in API responses
rg -n -A20 'def.*list|def.*get|@.*\.get' backend/opensync/routers/ --type=py | grep -A20 'entity\|Entity' | head -60Repository: brandon-braner/open_sync
Length of output: 3720
🏁 Script executed:
# Check store.py Entity loading to see the complete structure
cat -n backend/opensync/store.py | sed -n '30,60p'Repository: brandon-braner/open_sync
Length of output: 1116
Fix field precedence in entityToForm—top-level fields should not be overridden by data fields.
entityToForm spreads e.data last, which means any name, description, or content fields in e.data will override the entity's top-level fields. The backend confirms this is a real scenario: yaml_workflow.py explicitly stores name and description within the data field. This causes unintended field shadowing where form initialization receives incorrect values when data contains these fields.
Reverse the spread order to let top-level fields take precedence:
export const entityToForm = (e) => ({
...e.data,
name: e.name,
description: e.description,
content: e.content,
});🤖 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 `@frontend/src/entityKinds.js` around lines 66 - 71, In the entityToForm
function, the spread operator order causes fields from e.data to override the
top-level entity fields (name, description, content), resulting in incorrect
form values when the backend stores these fields within the data object. Reverse
the spread order by moving ...e.data to the beginning of the object literal and
placing the explicit name, description, and content field assignments after it,
so top-level fields take precedence and override any conflicting fields from
e.data.
Comment on lines
+9
to
+19
| const submit = async (e) => { | ||
| e.preventDefault(); | ||
| if (!name.trim() || !path.trim()) return; | ||
| setAdding(true); | ||
| try { | ||
| await onAddProject(name.trim(), path.trim()); | ||
| setName(''); setPath(''); | ||
| } finally { | ||
| setAdding(false); | ||
| } | ||
| }; |
There was a problem hiding this comment.
Form fields are cleared even when onAddProject fails.
The form reset (line 15) is inside the try block, so if onAddProject throws an error (as it does in App.jsx line 68), the user loses their input even though the operation failed.
🐛 Recommended fix to preserve input on error
const submit = async (e) => {
e.preventDefault();
if (!name.trim() || !path.trim()) return;
setAdding(true);
try {
await onAddProject(name.trim(), path.trim());
+ } catch (err) {
+ // Error is already toasted by onAddProject; keep form fields
+ } finally {
+ setAdding(false);
+ }
+ // Reset form only after successful add
+ if (!adding) {
setName(''); setPath('');
- } finally {
- setAdding(false);
}
};Or simpler:
const submit = async (e) => {
e.preventDefault();
if (!name.trim() || !path.trim()) return;
setAdding(true);
try {
await onAddProject(name.trim(), path.trim());
+ setName(''); setPath(''); // Reset only on success
+ } catch (err) {
+ // onAddProject already shows toast
} finally {
setAdding(false);
}
};📝 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.
Suggested change
| const submit = async (e) => { | |
| e.preventDefault(); | |
| if (!name.trim() || !path.trim()) return; | |
| setAdding(true); | |
| try { | |
| await onAddProject(name.trim(), path.trim()); | |
| setName(''); setPath(''); | |
| } finally { | |
| setAdding(false); | |
| } | |
| }; | |
| const submit = async (e) => { | |
| e.preventDefault(); | |
| if (!name.trim() || !path.trim()) return; | |
| setAdding(true); | |
| try { | |
| await onAddProject(name.trim(), path.trim()); | |
| setName(''); setPath(''); // Reset only on success | |
| } catch (err) { | |
| // onAddProject already shows toast | |
| } finally { | |
| setAdding(false); | |
| } | |
| }; |
🤖 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 `@frontend/src/pages/ProjectsPage.jsx` around lines 9 - 19, The form fields are
cleared in the try block of the submit function after the onAddProject call,
which means they persist in the current code. However, to ensure the form is
only cleared on successful submission and improve error handling, add a catch
block to the submit function that handles errors from onAddProject without
clearing the form fields, and keep the setName('') and setPath('') statements in
the try block only after the await onAddProject call succeeds. This way, if
onAddProject throws an error, the catch block will handle it and the form input
will be preserved for the user to correct and resubmit.
- Add @kilocode/plugin@7.3.45 to .opencode/package-lock.json with full dependency tree (effect, zod, @kilocode/sdk, etc.) - Update README and CONTRIBUTING_INTEGRATIONS.md to reflect backend/opensync/ package structure (paths were still showing flat backend/ layout) - Add ARCHITECTURE.md references to README for full component model and drift state machine documentation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by Sourcery
Introduce OpenSync v2 with a unified engine and data model for syncing MCP servers, skills, rules, commands, subagents, and LLM providers across tools, with a new web UI shell, manifest-driven integrations, and diff-based sync workflows.
New Features:
Enhancements:
Documentation:
Tests:
Summary by CodeRabbit
New Features
uvx open-syncDocumentation
Chores