isolate validator worker installs and raise default concurrency

[zouyonghe]

Summary

• isolate plugin dependency installs inside each validator worker so one plugin cannot pollute the shared runner environment for later checks
• raise the validator default max_workers from 8 to 16 in both the CLI default and workflow defaults
• add tests covering worker install isolation and the new parser default

Why

A plugin such as astrbot_plugin_xyzw can request setuptools==59.5.0, which breaks pkg_resources under Python 3.12 and caused later workers to fail with module 'pkgutil' has no attribute 'ImpImporter'.

Testing

• python3 -m unittest tests.test_validate_plugins tests.test_detect_changed_plugins

Summary by Sourcery

Isolate plugin validator worker environments and increase default validation concurrency.

New Features:

• Configure each validator worker to use its own temporary install and import environment for plugin dependencies.

Bug Fixes:

• Prevent plugin dependency installations from polluting the shared runner environment and breaking subsequent validator workers.

Enhancements:

• Increase the validator default max_workers value from 8 to 16 in both the CLI and GitHub workflow defaults.

CI:

• Update the validate-plugin-smoke GitHub workflow to use the higher default max_workers value.

Tests:

• Add tests verifying worker install isolation behavior and the updated max_workers default value in the argument parser.

[sourcery-ai]

Hey - I've left some high level feedback:

• In configure_worker_install_target, consider guarding against adding duplicate entries to PYTHONPATH and sys.path (e.g., if the function is called more than once) to keep path handling predictable.
• Because configure_worker_install_target mutates global process state (env vars and sys.path), you may want to make that more explicit (name/docstring) or wrap it in a context manager pattern to make its side effects and lifetime clearer if it’s reused in other contexts.

Prompt for AI Agents

Please address the comments from this code review:

## Overall Comments
- In `configure_worker_install_target`, consider guarding against adding duplicate entries to `PYTHONPATH` and `sys.path` (e.g., if the function is called more than once) to keep path handling predictable.
- Because `configure_worker_install_target` mutates global process state (env vars and `sys.path`), you may want to make that more explicit (name/docstring) or wrap it in a context manager pattern to make its side effects and lifetime clearer if it’s reused in other contexts.

---

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

• X
• Mastodon
• LinkedIn
• Facebook

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

[gemini-code-assist]

Code Review

This pull request increases the default worker count for plugin validation and implements environment isolation for plugin installations by dynamically configuring PIP_TARGET and PYTHONPATH. A new test suite verifies this isolation, though feedback was provided to make the sys.path restoration more robust using addCleanup to prevent state leakage between tests.

[gemini-code-assist]

The restoration of sys.path at the end of the test is not robust. If module.run_worker(args) or any assertion fails, the line sys.path[:] = original_sys_path will be skipped, leaving the global sys.path modified for subsequent tests in the same process. It is better to use a try...finally block or self.addCleanup to ensure the environment is restored regardless of the test outcome.

        original_sys_path = list(sys.path)
        def restore_path():
            sys.path[:] = original_sys_path
        self.addCleanup(restore_path)

        async def fake_run_worker_load_check(plugin_dir_name: str, normalized_repo_url: str):
            observed["plugin_dir_name"] = plugin_dir_name
            observed["normalized_repo_url"] = normalized_repo_url
            observed["astrbot_root"] = os.environ.get("ASTRBOT_ROOT")
            observed["pip_target"] = os.environ.get("PIP_TARGET")
            observed["sys_path"] = list(sys.path)
            return module.build_result(
                plugin=plugin_dir_name,
                repo=normalized_repo_url,
                normalized_repo_url=normalized_repo_url,
                ok=True,
                stage="load",
                message="plugin loaded successfully",
                plugin_dir_name=plugin_dir_name,
            )

        with tempfile.TemporaryDirectory() as tmp_dir:
            plugin_source_dir = Path(tmp_dir) / "plugin-src"
            plugin_source_dir.mkdir()
            (plugin_source_dir / "main.py").write_text("print('hello')\n", encoding="utf-8")
            args = module.argparse.Namespace(
                astrbot_path="/tmp/AstrBot",
                plugin_source_dir=str(plugin_source_dir),
                plugin_dir_name="demo_plugin",
                normalized_repo_url="https://github.com/example/demo-plugin",
            )

            with mock.patch.dict(os.environ, {}, clear=True):
                with mock.patch.object(module, "run_worker_load_check", side_effect=fake_run_worker_load_check):
                    exit_code = module.run_worker(args)

[zouyonghe]

@sourcery-ai review

[sourcery-ai]

Hey - I've found 1 issue

Prompt for AI Agents

Please address the comments from this code review:

## Individual Comments

### Comment 1
<location path="tests/test_validate_plugins.py" line_range="751-760" />
<code_context>
+class RunWorkerIsolationTests(unittest.TestCase):
</code_context>
<issue_to_address>
**suggestion (testing):** Strengthen `run_worker` isolation test by asserting properties of the temporary root, not just that it exists

The current test only checks that `ASTRBOT_ROOT` and `PIP_TARGET` are set and related, but not that the worker is actually isolated from the real AstrBot path. Consider also asserting that:

- `observed["astrbot_root"]` differs from `args.astrbot_path`.
- `observed["astrbot_root"]` is under a temporary worker directory (e.g., the `astrbot-plugin-worker-` prefix or a parent dir starting with it).
- Optionally, the expected plugin store/config directories are created under this root, if that’s part of the isolation contract.

These checks would more directly validate that each worker uses its own temp root instead of the shared AstrBot path.

Suggested implementation:

```python
class RunWorkerIsolationTests(unittest.TestCase):
    def _assert_worker_isolated(self, temp_root: Path, args, observed: dict) -> None:
        """
        Assert that a worker run is isolated from the real AstrBot path.

        This validates that:
        - The worker's astrbot_root differs from args.astrbot_path.
        - The astrbot_root resides under the temp_root used for the worker.
        - There exists a parent directory in the astrbot_root path whose name
          starts with the expected worker directory prefix.
        """
        self.assertIn("astrbot_root", observed, "observed must contain 'astrbot_root'")
        astrbot_root = Path(observed["astrbot_root"])

        # 1. The worker root must differ from the configured AstrBot path.
        self.assertNotEqual(
            astrbot_root,
            Path(args.astrbot_path),
            "Worker astrbot_root should not match the shared AstrBot path",
        )

        # 2. The worker root must be inside the temporary worker directory root.
        self.assertTrue(
            astrbot_root.is_relative_to(temp_root)
            if hasattr(astrbot_root, "is_relative_to")
            else str(astrbot_root).startswith(str(temp_root)),
            f"Worker astrbot_root ({astrbot_root}) is expected to be under temp_root ({temp_root})",
        )

        # 3. Somewhere in the ancestry we expect a worker directory prefix.
        worker_dir_prefix = "astrbot-plugin-worker-"
        current = astrbot_root
        found_prefix = False
        # Safeguard loop: walk up until root
        while True:
            if current.name.startswith(worker_dir_prefix):
                found_prefix = True
                break
            if current.parent == current:
                break
            current = current.parent

        self.assertTrue(
            found_prefix,
            f"No ancestor directory of astrbot_root ({astrbot_root}) "
            f"starts with '{worker_dir_prefix}'",
        )

    def test_configure_worker_install_target_deduplicates_process_paths(self):
        module = load_validator_module()
        original_sys_path = list(sys.path)

        with tempfile.TemporaryDirectory() as tmp_dir:
            temp_root = Path(tmp_dir)
            site_packages = (temp_root / "site-packages").resolve()
            extra_path = (temp_root / "extra-path").resolve()
            extra_path.mkdir()
            observed = {}

```

To fully implement the suggested stronger isolation checks, you should:

1. Ensure that `test_configure_worker_install_target_deduplicates_process_paths` (or another test in `RunWorkerIsolationTests`) captures:
   - the parsed arguments (e.g., `args = module.build_parser().parse_args([...])`),
   - the temporary worker root (`temp_root`, already present),
   - the worker’s effective AstrBot root in `observed["astrbot_root"]` (this appears to already exist per your comment).

2. After the worker has been configured/run and `observed["astrbot_root"]` is populated, call the helper:
   ```python
   self._assert_worker_isolated(temp_root=temp_root, args=args, observed=observed)
   ```

3. If part of the worker isolation contract is that specific plugin store/config directories are created under `astrbot_root`, extend `_assert_worker_isolated` with additional checks, for example:
   ```python
   plugins_dir = astrbot_root / "plugins"
   config_dir = astrbot_root / "config"
   self.assertTrue(plugins_dir.is_dir())
   self.assertTrue(config_dir.is_dir())
   ```
   and adjust the directory names to match your actual layout.

You’ll need to wire in step (2) at the appropriate point in the existing test body where `observed` is fully populated, since that portion of the test is not visible in the provided snippet.
</issue_to_address>

---

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

• X
• Mastodon
• LinkedIn
• Facebook

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

[sourcery-ai]

suggestion (testing): Strengthen run_worker isolation test by asserting properties of the temporary root, not just that it exists

The current test only checks that ASTRBOT_ROOT and PIP_TARGET are set and related, but not that the worker is actually isolated from the real AstrBot path. Consider also asserting that:

• observed["astrbot_root"] differs from args.astrbot_path.
• observed["astrbot_root"] is under a temporary worker directory (e.g., the astrbot-plugin-worker- prefix or a parent dir starting with it).
• Optionally, the expected plugin store/config directories are created under this root, if that’s part of the isolation contract.

These checks would more directly validate that each worker uses its own temp root instead of the shared AstrBot path.

Suggested implementation:

class RunWorkerIsolationTests(unittest.TestCase):
    def _assert_worker_isolated(self, temp_root: Path, args, observed: dict) -> None:
        """
        Assert that a worker run is isolated from the real AstrBot path.

        This validates that:
        - The worker's astrbot_root differs from args.astrbot_path.
        - The astrbot_root resides under the temp_root used for the worker.
        - There exists a parent directory in the astrbot_root path whose name
          starts with the expected worker directory prefix.
        """
        self.assertIn("astrbot_root", observed, "observed must contain 'astrbot_root'")
        astrbot_root = Path(observed["astrbot_root"])

        # 1. The worker root must differ from the configured AstrBot path.
        self.assertNotEqual(
            astrbot_root,
            Path(args.astrbot_path),
            "Worker astrbot_root should not match the shared AstrBot path",
        )

        # 2. The worker root must be inside the temporary worker directory root.
        self.assertTrue(
            astrbot_root.is_relative_to(temp_root)
            if hasattr(astrbot_root, "is_relative_to")
            else str(astrbot_root).startswith(str(temp_root)),
            f"Worker astrbot_root ({astrbot_root}) is expected to be under temp_root ({temp_root})",
        )

        # 3. Somewhere in the ancestry we expect a worker directory prefix.
        worker_dir_prefix = "astrbot-plugin-worker-"
        current = astrbot_root
        found_prefix = False
        # Safeguard loop: walk up until root
        while True:
            if current.name.startswith(worker_dir_prefix):
                found_prefix = True
                break
            if current.parent == current:
                break
            current = current.parent

        self.assertTrue(
            found_prefix,
            f"No ancestor directory of astrbot_root ({astrbot_root}) "
            f"starts with '{worker_dir_prefix}'",
        )

    def test_configure_worker_install_target_deduplicates_process_paths(self):
        module = load_validator_module()
        original_sys_path = list(sys.path)

        with tempfile.TemporaryDirectory() as tmp_dir:
            temp_root = Path(tmp_dir)
            site_packages = (temp_root / "site-packages").resolve()
            extra_path = (temp_root / "extra-path").resolve()
            extra_path.mkdir()
            observed = {}

To fully implement the suggested stronger isolation checks, you should:

1. Ensure that test_configure_worker_install_target_deduplicates_process_paths (or another test in RunWorkerIsolationTests) captures:

   • the parsed arguments (e.g., args = module.build_parser().parse_args([...])),
   • the temporary worker root (temp_root, already present),
   • the worker’s effective AstrBot root in observed["astrbot_root"] (this appears to already exist per your comment).

2. After the worker has been configured/run and observed["astrbot_root"] is populated, call the helper:

   self._assert_worker_isolated(temp_root=temp_root, args=args, observed=observed)

3. If part of the worker isolation contract is that specific plugin store/config directories are created under astrbot_root, extend _assert_worker_isolated with additional checks, for example:

   plugins_dir = astrbot_root / "plugins"
   config_dir = astrbot_root / "config"
   self.assertTrue(plugins_dir.is_dir())
   self.assertTrue(config_dir.is_dir())

   and adjust the directory names to match your actual layout.

You’ll need to wire in step (2) at the appropriate point in the existing test body where observed is fully populated, since that portion of the test is not visible in the provided snippet.

[zouyonghe]

@sourcery-ai review

[sourcery-ai]

Hey - I've left some high level feedback:

• In configure_worker_install_target, deduplication of PYTHONPATH and sys.path entries relies on raw string equality; consider normalizing (e.g., resolving or os.path.abspath) before comparison so that equivalent paths with different spellings aren’t duplicated or accidentally filtered out.

Prompt for AI Agents

Please address the comments from this code review:

## Overall Comments
- In `configure_worker_install_target`, deduplication of `PYTHONPATH` and `sys.path` entries relies on raw string equality; consider normalizing (e.g., resolving or `os.path.abspath`) before comparison so that equivalent paths with different spellings aren’t duplicated or accidentally filtered out.

---

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

• X
• Mastodon
• LinkedIn
• Facebook

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

[zouyonghe]

@sourcery-ai review

[sourcery-ai]

Hey - I've reviewed your changes and they look great!

---

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

• X
• Mastodon
• LinkedIn
• Facebook

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}