整合 Study Companion/Galgame 解耦与共享安装路由#1517
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
Walkthrough本 PR 将 RapidOCR 的实现抽取到共享子模块并以 plugin_id 参数化，重构安装注册、安装任务存储与通用 plugin_install 路由；新增教程迁移、study 捕获后端与大量测试适配喵。 Changes共享 RapidOCR 与 plugin_id 参数化
安装注册与 plugin_install 路由重构
插件适配与捕获后端
测试与覆盖调整
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (4)
plugin/plugins/_shared/rapidocr/ocr_runtime_types.py (1)
78-94: 💤 Low value
plugin_id默认值与 PR 目标可能不一致喵~PR 描述说 "RapidOCR shared layer no longer defaults to study_companion; callers must pass plugin_id explicitly"，但这里
_rapidocr_runtime_cache_key的plugin_id参数默认值是"study_companion"喵。虽然这只是缓存 key 生成函数，但保留默认值可能会让调用者忘记显式传递
plugin_id，导致不同插件共享同一个缓存条目喵~ 本喵建议移除默认值，让调用方必须显式传入喵！🔧 建议的修改喵~
def _rapidocr_runtime_cache_key( *, install_target_dir_raw: str, engine_type: str, lang_type: str, model_type: str, ocr_version: str, - plugin_id: str = "study_companion", + plugin_id: str, ) -> tuple[str, str, str, str, str, str]:🤖 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 `@plugin/plugins/_shared/rapidocr/ocr_runtime_types.py` around lines 78 - 94, The _rapidocr_runtime_cache_key function currently defaults plugin_id to "study_companion", which contradicts the PR goal that callers must pass plugin_id explicitly; remove the default by changing the signature of _rapidocr_runtime_cache_key so plugin_id is a required parameter (no "= \"study_companion\""), and ensure any callers are updated to supply plugin_id when calling the function so cache keys are generated per-plugin.plugin/plugins/galgame_plugin/__init__.py (1)
69-78: ⚡ Quick win建议将两项注册拆成独立容错块，避免互相短路喵。
现在一个
try同时包住路由注册和迁移 hook；前者异常时后者不会尝试执行。拆分后可保持“尽力注册”的容错语义更稳喵。♻️ 可参考的调整喵
-try: - _register_install_routes() - _register_tutorial_migration_hook() -except Exception: # noqa: BLE001 - route registration should not block package import. - from plugin.logging_config import get_logger - - get_logger("galgame.install_routes").warning( - "galgame install route/migration registration failed", - exc_info=True, - ) +from plugin.logging_config import get_logger +_install_logger = get_logger("galgame.install_routes") + +try: + _register_install_routes() +except Exception: # noqa: BLE001 - route registration should not block package import. + _install_logger.warning( + "galgame install route registration failed", + exc_info=True, + ) + +try: + _register_tutorial_migration_hook() +except Exception: # noqa: BLE001 - migration hook registration should not block package import. + _install_logger.warning( + "galgame tutorial migration hook registration failed", + exc_info=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 `@plugin/plugins/galgame_plugin/__init__.py` around lines 69 - 78, The registration currently wraps both _register_install_routes and _register_tutorial_migration_hook in one try/except which causes one failure to short-circuit the other; split them into two independent try/except blocks so each call is attempted even if the other raises, catching Exception and logging via get_logger("galgame.install_routes").warning(..., exc_info=True) for each failing call (use the same warning message/context currently used).plugin/plugins/galgame_plugin/_tutorial_migration.py (1)
54-65: ⚡ Quick winGalgameStore.load_tutorial_progress() 的异常没有被捕获喵~
如果
GalgameStore(...).load_tutorial_progress()抛出异常（比如旧存储文件损坏），这个异常会一路冒泡到_run_tutorial_migrations，可能会导致教程功能完全不可用喵。虽然说旧数据损坏的概率很低，但本喵建议加个 try/except 让迁移更健壮一点喵~♻️ 建议的改进喵
for legacy_store_path in _legacy_store_paths(): if not legacy_store_path.is_file(): continue - legacy_progress = GalgameStore(legacy_store_path, logger).load_tutorial_progress() + try: + legacy_progress = GalgameStore(legacy_store_path, logger).load_tutorial_progress() + except Exception: + logger.warning( + "failed to load legacy tutorial progress from {}, skipping", + legacy_store_path, + exc_info=True, + ) + continue if isinstance(legacy_progress, dict): _write_flat_progress(store_path, legacy_progress)🤖 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 `@plugin/plugins/galgame_plugin/_tutorial_migration.py` around lines 54 - 65, Wrap the call to GalgameStore(...).load_tutorial_progress() in a try/except to catch and log any exceptions so a corrupted legacy file won't abort the migration; when iterating _legacy_store_paths(), call GalgameStore(legacy_store_path, logger).load_tutorial_progress() inside a try block, on exception log an informative message with the exception via logger (including legacy_store_path) and continue to the next path, and only on successful dict result call _write_flat_progress(store_path, legacy_progress) and logger.info as before; ensure you reference GalgameStore.load_tutorial_progress, _legacy_store_paths, _write_flat_progress and logger in the change.plugin/tests/integration/test_galgame_bridge_ui_routes.py (1)
762-778: ⚡ Quick winSSE 失败态断言建议等待目标事件，避免首条事件误判喵。
这里拿到第一条
data:就break，如果流先发running再发failed，这个用例会变得脆弱喵。建议循环读取直到命中status == "failed"（或超时）再断言喵。✨ 可选改法（更稳的事件读取）
- async with plugin_ui_async_client.stream( + async with plugin_ui_async_client.stream( "GET", "/plugin/galgame_plugin/ui-api/textractor/install/run-stream-crash/stream", ) as response: assert response.status_code == 200 - body = "" + body = "" async for line in response.aiter_lines(): if line.startswith("data: "): - body = line[len("data: "):] - break + candidate = json.loads(line[len("data: "):]) + if candidate.get("status") == "failed": + body = json.dumps(candidate) + break🤖 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 `@plugin/tests/integration/test_galgame_bridge_ui_routes.py` around lines 762 - 778, The test reads only the first SSE "data: " line and breaks, which can misidentify a transient "running" event as the final failed event; update the SSE consumption (the async with plugin_ui_async_client.stream(...) block) to continue iterating response.aiter_lines() until you parse a JSON payload whose payload["status"] == "failed" (or until a reasonable timeout/iteration limit) before breaking and asserting on payload["task_id"], payload["stream_error"], and payload["message"]; ensure you still handle non-"data: " lines and invalid JSON robustly so the test doesn’t hang.
🤖 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.
Nitpick comments:
In `@plugin/plugins/_shared/rapidocr/ocr_runtime_types.py`:
- Around line 78-94: The _rapidocr_runtime_cache_key function currently defaults
plugin_id to "study_companion", which contradicts the PR goal that callers must
pass plugin_id explicitly; remove the default by changing the signature of
_rapidocr_runtime_cache_key so plugin_id is a required parameter (no "=
\"study_companion\""), and ensure any callers are updated to supply plugin_id
when calling the function so cache keys are generated per-plugin.
In `@plugin/plugins/galgame_plugin/__init__.py`:
- Around line 69-78: The registration currently wraps both
_register_install_routes and _register_tutorial_migration_hook in one try/except
which causes one failure to short-circuit the other; split them into two
independent try/except blocks so each call is attempted even if the other
raises, catching Exception and logging via
get_logger("galgame.install_routes").warning(..., exc_info=True) for each
failing call (use the same warning message/context currently used).
In `@plugin/plugins/galgame_plugin/_tutorial_migration.py`:
- Around line 54-65: Wrap the call to GalgameStore(...).load_tutorial_progress()
in a try/except to catch and log any exceptions so a corrupted legacy file won't
abort the migration; when iterating _legacy_store_paths(), call
GalgameStore(legacy_store_path, logger).load_tutorial_progress() inside a try
block, on exception log an informative message with the exception via logger
(including legacy_store_path) and continue to the next path, and only on
successful dict result call _write_flat_progress(store_path, legacy_progress)
and logger.info as before; ensure you reference
GalgameStore.load_tutorial_progress, _legacy_store_paths, _write_flat_progress
and logger in the change.
In `@plugin/tests/integration/test_galgame_bridge_ui_routes.py`:
- Around line 762-778: The test reads only the first SSE "data: " line and
breaks, which can misidentify a transient "running" event as the final failed
event; update the SSE consumption (the async with
plugin_ui_async_client.stream(...) block) to continue iterating
response.aiter_lines() until you parse a JSON payload whose payload["status"] ==
"failed" (or until a reasonable timeout/iteration limit) before breaking and
asserting on payload["task_id"], payload["stream_error"], and
payload["message"]; ensure you still handle non-"data: " lines and invalid JSON
robustly so the test doesn’t hang.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: a19a1d29-a0b2-4ca4-9319-e301c36def9c
📒 Files selected for processing (48)
plugin/plugins/_shared/__init__.pyplugin/plugins/_shared/rapidocr/__init__.pyplugin/plugins/_shared/rapidocr/_inspect_download.pyplugin/plugins/_shared/rapidocr/_model_registry.pyplugin/plugins/_shared/rapidocr/_paths.pyplugin/plugins/_shared/rapidocr/_runtime.pyplugin/plugins/_shared/rapidocr/ocr_backends.pyplugin/plugins/_shared/rapidocr/ocr_rapidocr_backend.pyplugin/plugins/_shared/rapidocr/ocr_runtime_types.pyplugin/plugins/_shared/rapidocr/rapidocr_support.pyplugin/plugins/galgame_plugin/__init__.pyplugin/plugins/galgame_plugin/_paths.pyplugin/plugins/galgame_plugin/_tutorial_migration.pyplugin/plugins/galgame_plugin/ocr_bridge_writer.pyplugin/plugins/galgame_plugin/ocr_capture_backends/_helpers.pyplugin/plugins/galgame_plugin/ocr_capture_backends/printwindow.pyplugin/plugins/galgame_plugin/ocr_manager_capture.pyplugin/plugins/galgame_plugin/ocr_manager_observe.pyplugin/plugins/galgame_plugin/ocr_manager_poll.pyplugin/plugins/galgame_plugin/ocr_manager_runtime.pyplugin/plugins/galgame_plugin/ocr_manager_text.pyplugin/plugins/galgame_plugin/ocr_rapidocr_backend.pyplugin/plugins/galgame_plugin/ocr_reader.pyplugin/plugins/galgame_plugin/ocr_runtime_types.pyplugin/plugins/galgame_plugin/plugin_core.pyplugin/plugins/galgame_plugin/plugin_entries/_common.pyplugin/plugins/galgame_plugin/plugin_entries/galgame_download_rapidocr_models.pyplugin/plugins/galgame_plugin/service/__init__.pyplugin/plugins/galgame_plugin/test_ocr_pipeline.pyplugin/plugins/galgame_plugin/textractor_support.pyplugin/plugins/galgame_plugin/training/classify/phase1_acceptance.pyplugin/plugins/study_companion/__init__.pyplugin/plugins/study_companion/service.pyplugin/plugins/study_companion/study_capture_backends.pyplugin/plugins/study_companion/study_ocr_pipeline.pyplugin/server/http_app.pyplugin/server/install_registry.pyplugin/server/routes/_install_task_store.pyplugin/server/routes/plugin_install.pyplugin/tests/integration/test_galgame_bridge_ui_routes.pyplugin/tests/unit/plugins/test_galgame_config_helpers.pyplugin/tests/unit/plugins/test_galgame_memory_reader.pyplugin/tests/unit/plugins/test_galgame_ocr_reader.pyplugin/tests/unit/plugins/test_galgame_rapidocr_support.pyplugin/tests/unit/plugins/test_galgame_ui_i18n.pyplugin/tests/unit/plugins/test_study_companion.pyplugin/tests/unit/plugins/test_study_companion_service_ui_api.pyplugin/tests/unit/server/test_http_app.py
💤 Files with no reviewable changes (1)
- plugin/plugins/galgame_plugin/_paths.py
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8773e9a89a
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| registration = install_registry.get_install_plugin_registration(plugin_id) | ||
| except ValueError as exc: | ||
| raise HTTPException(status_code=404, detail="Plugin has no install API") from exc | ||
| if registration is None: | ||
| raise HTTPException(status_code=404, detail=f"Plugin '{plugin_id}' has no install API") |
There was a problem hiding this comment.
The new route layer now hard-fails install/tutorial/i18n APIs when _install_plugin_registry is empty, but this module never imports the built-in plugins that perform register_install_plugin(...) side effects. Registration currently depends on plugin.plugins.galgame_plugin.__init__ / study_companion.__init__ being imported elsewhere, and PluginRegistryService._build_discovery_payload only imports plugin modules when ctx.enabled is true, so disabled/not-yet-imported plugins now return 404 for install APIs that previously worked via the old hardcoded route map. This creates a user-visible regression for setup/install flows unless registrations are bootstrapped explicitly in server startup or this route module.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
plugin/server/install_registry.py (1)
146-152:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win统一 hook 查询时的 plugin_id 归一化喵
Line 151 直接用原始
plugin_id查字典，会和register_tutorial_migration_hook的归一化写入产生不一致（例如传入" study_companion "时读不到已注册 hook）喵。这里建议先做同样的 normalize 再查询喵。可直接套用的小改动喵
def tutorial_migration_hooks_for(plugin_id: str) -> list[Callable[[Path], None]]: + normalized_plugin_id = normalize_registered_plugin_id(plugin_id) if plugin_id else "" if isinstance(_tutorial_migration_hooks, list): return list(_tutorial_migration_hooks) return [ *_tutorial_migration_hooks.get("", []), - *_tutorial_migration_hooks.get(plugin_id, []), + *_tutorial_migration_hooks.get(normalized_plugin_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 `@plugin/server/install_registry.py` around lines 146 - 152, Normalize plugin_id before using it to query _tutorial_migration_hooks in tutorial_migration_hooks_for so lookup matches how hooks are registered; apply the same normalization used by register_tutorial_migration_hook (e.g., strip whitespace and lowercase) and then use that normalized key when calling _tutorial_migration_hooks.get(plugin_id, []), while still including the default "" hooks.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@plugin/server/install_registry.py`:
- Around line 146-152: Normalize plugin_id before using it to query
_tutorial_migration_hooks in tutorial_migration_hooks_for so lookup matches how
hooks are registered; apply the same normalization used by
register_tutorial_migration_hook (e.g., strip whitespace and lowercase) and then
use that normalized key when calling _tutorial_migration_hooks.get(plugin_id,
[]), while still including the default "" hooks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 9e943118-2a5e-4846-9169-ce0d039c1117
📒 Files selected for processing (2)
plugin/server/install_registry.pyplugin/tests/unit/server/test_install_registry.py
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 67a7fdbe42
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| from plugin.plugins.galgame_plugin._tutorial_migration import ( | ||
| copy_legacy_tutorial_progress_if_missing, | ||
| ) |
There was a problem hiding this comment.
Avoid importing plugin.plugins.galgame_plugin._tutorial_migration unconditionally inside the built-in tutorial migration hook. In builds where optional plugin packages are excluded (the same scenario http_app.py explicitly supports by treating plugin routes as optional), get_tutorial_status("galgame_plugin") now reaches this hook, raises ModuleNotFoundError, and returns a 500 instead of a stable response. This regression is introduced by moving the route into plugin.server while still hard-registering the galgame migration hook; the hook should degrade safely when the galgame module is unavailable.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
plugin/tests/unit/server/test_install_registry.py (1)
55-81: ⚡ Quick win建议补一条“非可选模块缺失必须继续抛错”的反向用例，防止异常边界被放宽喵。
现在只覆盖了“应忽略”的缺失模块，建议再加一个“不应忽略”的模块名并断言
ModuleNotFoundError，这样能把 re-raise 契约锁住喵。🧪补充用例示例喵
+def test_builtin_galgame_migration_hook_reraises_unrelated_missing_module( + monkeypatch: pytest.MonkeyPatch, + tmp_path: Path, +) -> None: + original_import = builtins.__import__ + + def guarded_import(name, globals=None, locals=None, fromlist=(), level=0): + if name == "plugin.plugins.galgame_plugin._tutorial_migration": + raise ModuleNotFoundError( + "No module named 'some.unrelated.module'", + name="some.unrelated.module", + ) + return original_import(name, globals, locals, fromlist, level) + + monkeypatch.setattr(builtins, "__import__", guarded_import) + + with pytest.raises(ModuleNotFoundError): + install_registry._copy_legacy_galgame_tutorial_progress_if_missing( + tmp_path / "tutorial_progress.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 `@plugin/tests/unit/server/test_install_registry.py` around lines 55 - 81, Add a reverse test that ensures missing non-optional modules still raise: copy the pattern in test_builtin_galgame_migration_hook_ignores_missing_optional_plugin but parametrize one case with a module name that should NOT be ignored (e.g. a non-optional plugin path) and assert that install_registry._copy_legacy_galgame_tutorial_progress_if_missing(tmp_path / "tutorial_progress.json") raises ModuleNotFoundError using pytest.raises; keep the same guarded_import monkeypatch on builtins.__import__ to simulate the missing import and reference the test function name and the target function _copy_legacy_galgame_tutorial_progress_if_missing to locate where to add the new case.
🤖 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 `@plugin/server/install_registry.py`:
- Around line 79-89: The except block currently treats any ModuleNotFoundError
whose name startswith "plugin.plugins.galgame_plugin" as optional and swallows
it; narrow this by checking for exact module names instead (e.g., compare
missing_name == "plugin.plugins" or missing_name ==
"plugin.plugins.galgame_plugin" and optionally also
"plugin.plugins.galgame_plugin.__init__") so missing submodules inside the
plugin will not be silently ignored; update the conditional that uses
missing_name and leave the logger.warning/return behavior intact for only those
exact names.
---
Nitpick comments:
In `@plugin/tests/unit/server/test_install_registry.py`:
- Around line 55-81: Add a reverse test that ensures missing non-optional
modules still raise: copy the pattern in
test_builtin_galgame_migration_hook_ignores_missing_optional_plugin but
parametrize one case with a module name that should NOT be ignored (e.g. a
non-optional plugin path) and assert that
install_registry._copy_legacy_galgame_tutorial_progress_if_missing(tmp_path /
"tutorial_progress.json") raises ModuleNotFoundError using pytest.raises; keep
the same guarded_import monkeypatch on builtins.__import__ to simulate the
missing import and reference the test function name and the target function
_copy_legacy_galgame_tutorial_progress_if_missing to locate where to add the new
case.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 7c020e24-89ba-4fdd-bae1-094611c65048
📒 Files selected for processing (3)
plugin/server/install_registry.pyplugin/tests/unit/plugins/test_galgame_ui_i18n.pyplugin/tests/unit/server/test_install_registry.py
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 194593969d
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
|
||
| def get_install_plugin_registration(plugin_id: str) -> InstallPluginRegistration | None: | ||
| normalized_plugin_id = normalize_registered_plugin_id(plugin_id) | ||
| bootstrap_builtin_install_plugins() |
There was a problem hiding this comment.
get_install_plugin_registration always calls bootstrap_builtin_install_plugins(), which unconditionally registers galgame_plugin and study_companion even in distributions where plugin.plugins is intentionally excluded (the optional-router case in http_app). In that environment, install/tutorial/i18n endpoints stop returning 404 for missing plugins and instead expose non-functional APIs (for example tutorial status can return ok: true, and install start can enqueue runs for absent plugin entries). Please only bootstrap a built-in plugin when its module/entry is actually present (or when it is enabled in runtime plugin state), so optional-plugin builds keep the expected 404 behavior.
Useful? React with 👍 / 👎.
…row migration error handling - Only bootstrap built-in install registrations (galgame_plugin, study_companion) when the plugin module is actually present via importlib.util.find_spec, preventing non-functional APIs from being exposed in optional-plugin builds - Narrow ModuleNotFoundError matching in galgame tutorial migration hook to exact module names instead of startswith, avoiding silent swallowing of real import failures
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8576e654df
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| def _plugin_module_available(plugin_id: str) -> bool: | ||
| return importlib.util.find_spec(f"plugin.plugins.{plugin_id}") is not None |
There was a problem hiding this comment.
_plugin_module_available directly calls importlib.util.find_spec("plugin.plugins.{plugin_id}") without handling ModuleNotFoundError/ImportError. In optional-plugin builds (where plugin.plugins is intentionally absent) or when parent-package import side effects fail, this raises instead of returning False, and bootstrap_builtin_install_plugins() propagates the exception through get_install_plugin_registration(), turning install/i18n/tutorial requests into 500s rather than clean 404 behavior. Wrap this probe in exception handling and treat lookup/import failures as "module unavailable."
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 30e92a9a01
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| for hook in install_registry.tutorial_migration_hooks_for(plugin_id): | ||
| hook(store_path) |
There was a problem hiding this comment.
_run_tutorial_migrations executes each registered hook directly, and _tutorial_store calls it before serving tutorial reads/writes. If any hook raises (for example due to a plugin-specific migration bug or unexpected file state), initialization aborts and tutorial endpoints return 500 for that plugin on every request because the path is never marked as migrated. Catching/logging hook failures per plugin (and continuing or disabling that hook) would prevent one bad migration from taking down all tutorial status/progress calls.
Useful? React with 👍 / 👎.
2 participants
变更内容
合并并取代 #1509 与 #1514，把 Study Companion / Galgame 解耦工作收敛到一个 PR 中：
galgame_plugin内部命名空间解耦，RapidOCR runtime / model download / OCR backend 下沉到plugin.plugins._shared.rapidocrplugin_id注册 install kinds、i18n 与 tutorial 能力plugin.server.install_registry，插件只注册能力到 registry，不再直接依赖 route handlerstudy_companion，调用方必须显式传plugin_idinstall_state返回，并保留未登记 RapidOCR 模型组合的 runtime fallback为什么这样改
#1509 解决的是插件实现层耦合：Study Companion 不该 import Galgame 内部 OCR / capture 代码。
#1514 解决的是 server 安装层耦合：安装 route、状态存储、SSE 与 tutorial progress 不该继续作为 Galgame 专属实现存在。
这次合并后，边界变成：
plugin.server.install_registry负责插件安装能力声明plugin.server.routes.plugin_install只负责 HTTP route / SSE / 状态协议plugin.plugins._shared.rapidocr只负责不带业务语义的 RapidOCR 通用能力取代的 PR
验证
uv run --python 3.11 python -m ruff check plugin/server/install_registry.py plugin/server/routes/plugin_install.py plugin/plugins/_shared/rapidocr plugin/plugins/galgame_plugin/__init__.py plugin/plugins/study_companion/__init__.py plugin/plugins/study_companion/service.py plugin/tests/integration/test_galgame_bridge_ui_routes.py plugin/tests/unit/plugins/test_galgame_ui_i18n.py plugin/tests/unit/plugins/test_galgame_rapidocr_support.py plugin/tests/unit/plugins/test_study_companion_service_ui_api.pygit diff --checkuv run --python 3.11 python -m compileall -q plugin/server/install_registry.py plugin/server/routes/plugin_install.py plugin/plugins/_shared/rapidocr plugin/plugins/study_companion/service.pyuv run --python 3.11 python -m pytest -q plugin/tests/unit/plugins/test_study_companion_service_ui_api.py --basetemp plugin/tests/tmp-pytest/combined-study-serviceuv run --python 3.11 python -m pytest -q plugin/tests/unit/plugins/test_galgame_rapidocr_support.py --basetemp plugin/tests/tmp-pytest/combined-galgame-rapidocruv run --python 3.11 python -m pytest -q plugin/tests/integration/test_galgame_bridge_ui_routes.py -k "install or tutorial or i18n" --basetemp plugin/tests/tmp-pytest/combined-routesuv run --python 3.11 python -m pytest -q plugin/tests/unit/server/test_http_app.py plugin/tests/unit/plugins/test_galgame_ui_i18n.py --basetemp plugin/tests/tmp-pytest/combined-http-i18nuv run --python 3.11 python -m pytest -q plugin/tests/unit/plugins/test_study_companion.py -k "rapidocr or printwindow or pyautogui or dxcam or galgame_namespace" --basetemp plugin/tests/tmp-pytest/combined-study-ocruv run --python 3.11 python -m pytest -q plugin/tests/unit/plugins/test_galgame_ocr_reader.py -k "rapidocr_install_target or rapidocr_installation or rapidocr_model" --basetemp plugin/tests/tmp-pytest/combined-galgame-ocrdetect_changes(scope=staged)：risklow，无 affected processesSummary by CodeRabbit
New Features
Bug Fixes
Improvements