fix: Generate desktop runtime core lock#125
Merged
Conversation
zouyonghe
changed the title
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In
launch_backend.py, you defineRUNTIME_CORE_LOCK_ENVbut still hard-code the environment variable name inconfigure_runtime_core_lock_path; consider using the constant there to avoid divergence if the name ever changes. - In
generate_runtime_core_lock.py,_read_top_level_modulescatches a broadExceptionand silently returns an empty list; narrowing this to the expectedFileNotFoundError/KeyError(or at least logging unexpected errors) would make failures around malformedtop_level.txtfiles more observable.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `launch_backend.py`, you define `RUNTIME_CORE_LOCK_ENV` but still hard-code the environment variable name in `configure_runtime_core_lock_path`; consider using the constant there to avoid divergence if the name ever changes.
- In `generate_runtime_core_lock.py`, `_read_top_level_modules` catches a broad `Exception` and silently returns an empty list; narrowing this to the expected `FileNotFoundError`/`KeyError` (or at least logging unexpected errors) would make failures around malformed `top_level.txt` files more observable.
## Individual Comments
### Comment 1
<location path="scripts/backend/templates/launch_backend.py" line_range="227" />
<code_context>
+def configure_runtime_core_lock_path() -> None:
+ lock_path = APP_DIR / "runtime-core-lock.json"
+ if lock_path.is_file():
+ os.environ["ASTRBOT_DESKTOP_CORE_LOCK_PATH"] = str(lock_path)
+
+
</code_context>
<issue_to_address>
**suggestion:** Reuse the RUNTIME_CORE_LOCK_ENV constant instead of hardcoding the environment variable name.
Since `RUNTIME_CORE_LOCK_ENV = "ASTRBOT_DESKTOP_CORE_LOCK_PATH"` is already defined, use that constant here (and anywhere else this env var is used) to keep the name centralized and avoid mismatches if it changes later.
Suggested implementation:
```python
os.environ[RUNTIME_CORE_LOCK_ENV] = str(lock_path)
```
Search the rest of `scripts/backend/templates/launch_backend.py` (and the broader codebase, if appropriate) for any other occurrences of the string literal `"ASTRBOT_DESKTOP_CORE_LOCK_PATH"` and replace them with `RUNTIME_CORE_LOCK_ENV` in the same way, so the environment variable name remains centralized in one constant.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request implements a dependency boundary for desktop plugins to prevent them from shadowing or conflicting with core backend runtime dependencies. It introduces a mechanism to generate a runtime-core-lock.json file during the build process, which captures installed distributions and their top-level modules. The launcher is updated to expose this lock file via an environment variable, allowing the pip installer to enforce constraints. Feedback focuses on ensuring compatibility with Python versions prior to 3.10 when accessing distribution metadata and improving code maintainability by using defined constants for environment variable keys.
|
|
||
| def _distribution_record(distribution: importlib_metadata.Distribution) -> dict[str, object] | None: | ||
| name = distribution.metadata.get("Name") | ||
| version = distribution.version |
Contributor
There was a problem hiding this comment.
The Distribution.version property was introduced in Python 3.10. If the bundled runtime is Python 3.8 or 3.9, this will raise an AttributeError. To ensure compatibility with all versions that include importlib.metadata, you should retrieve the version from the metadata headers, similar to how the name is retrieved at line 24.
Suggested change
| version = distribution.version | |
| version = distribution.metadata.get("Version") |
| def configure_runtime_core_lock_path() -> None: | ||
| lock_path = APP_DIR / "runtime-core-lock.json" | ||
| if lock_path.is_file(): | ||
| os.environ["ASTRBOT_DESKTOP_CORE_LOCK_PATH"] = str(lock_path) |
Contributor
There was a problem hiding this comment.
The environment variable name is already defined as a constant RUNTIME_CORE_LOCK_ENV at line 18. It is better to use the constant here to maintain consistency and avoid potential typos.
Suggested change
| os.environ["ASTRBOT_DESKTOP_CORE_LOCK_PATH"] = str(lock_path) | |
| os.environ[RUNTIME_CORE_LOCK_ENV] = str(lock_path) |
Reuse the launcher env constant and only suppress missing top_level.txt metadata so unexpected lock-generation failures stay visible.
Member
Author
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- In
configure_runtime_core_lock_pathit might be useful to skip settingASTRBOT_DESKTOP_CORE_LOCK_PATHwhen it is already present in the environment so that advanced users or tests can override the lock location if needed. - Both
generateRuntimeCoreLockandgenerate_runtime_core_lock.pyensure the output directory exists; you could drop one of themkdircalls to avoid redundant filesystem work and keep the responsibility in a single layer.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `configure_runtime_core_lock_path` it might be useful to skip setting `ASTRBOT_DESKTOP_CORE_LOCK_PATH` when it is already present in the environment so that advanced users or tests can override the lock location if needed.
- Both `generateRuntimeCoreLock` and `generate_runtime_core_lock.py` ensure the output directory exists; you could drop one of the `mkdir` calls to avoid redundant filesystem work and keep the responsibility in a single layer.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Preserve explicit launcher lock path overrides for tests and advanced setups, and let the Python lock generator own output directory creation.
Member
Author
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey - I've found 1 issue
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="docs/plans/2026-04-28-desktop-plugin-dependency-boundary-design.md" line_range="9-15" />
<code_context>
+shadow packages already used by the bundled backend runtime. Packages such as
+`openai`, `pydantic`, `fastapi`, `numpy`, and PyObjC modules are unsafe to hot-reload
+inside a running backend process. Installing Cua exposed this failure mode: pip
+installed a second dependency set into plugin `site-packages`, the installer tried to
+prefer those modules in-process, and the running OpenAI client later saw incompatible
+class identities.
</code_context>
<issue_to_address>
**nitpick (typo):** Consider revising this sentence to avoid the comma splice between independent clauses.
In particular, the sentence starting with “pip installed a second dependency set…” links multiple independent clauses with commas. Consider splitting it into two sentences, or use a semicolon or conjunction (e.g., “..., and the installer tried...”) to clarify the structure.
```suggestion
This allows plugin-only dependencies to work, but it also allows plugin installs to
shadow packages already used by the bundled backend runtime. Packages such as
`openai`, `pydantic`, `fastapi`, `numpy`, and PyObjC modules are unsafe to hot-reload
inside a running backend process. Installing Cua exposed this failure mode: pip
installed a second dependency set into plugin `site-packages`; the installer tried to
prefer those modules in-process, and the running OpenAI client later saw incompatible
class identities.
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Clarify the Cua failure-mode sentence in the desktop plugin dependency boundary design by replacing the comma splice with clearer punctuation.
Member
Author
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- The
runtime-core-lock.test.mjschecks for implementation details via string matching (e.g.,assert.doesNotMatch(source, /mkdirSync\(/)), which is brittle; consider asserting behavior via a small integration-style call or exported API semantics instead of source inspection. - The plan document
2026-04-28-desktop-plugin-dependency-boundary.mdincludes tool-specific directives (the “For Claude” note) that don’t affect the design itself; consider removing or relocating those to avoid embedding reviewer-specific instructions in long-lived docs.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `runtime-core-lock.test.mjs` checks for implementation details via string matching (e.g., `assert.doesNotMatch(source, /mkdirSync\(/)`), which is brittle; consider asserting behavior via a small integration-style call or exported API semantics instead of source inspection.
- The plan document `2026-04-28-desktop-plugin-dependency-boundary.md` includes tool-specific directives (the “For Claude” note) that don’t affect the design itself; consider removing or relocating those to avoid embedding reviewer-specific instructions in long-lived docs.
## Individual Comments
### Comment 1
<location path="scripts/backend/runtime-core-lock.mjs" line_range="9-18" />
<code_context>
+export const generateRuntimeCoreLock = ({ runtimePython, outputPath }) => {
</code_context>
<issue_to_address>
**suggestion:** Enrich error messages with the Python executable and generator script path to make diagnosing failures easier.
The error messages (`Failed to generate runtime core lock`, `Runtime core lock generation failed`, `did not create ...`) don’t indicate which Python binary or script were used. Please include `runtimePython.absolute` and `generatorScriptPath` in these messages (e.g. ` (python: ${runtimePython.absolute}, script: ${generatorScriptPath})`) to make misconfigurations easier to diagnose.
Suggested implementation:
```javascript
if (!runtimePython?.absolute) {
throw new Error(
`Missing runtime Python executable for runtime core lock generation (python: ${
runtimePython?.absolute ?? 'undefined'
}, script: ${generatorScriptPath}).`,
);
}
```
```javascript
if (!outputPath) {
throw new Error(
`Missing output path for runtime core lock generation (python: ${runtimePython.absolute}, script: ${generatorScriptPath}).`,
);
}
```
To fully implement your comment, the following additional updates are needed elsewhere in `scripts/backend/runtime-core-lock.mjs` (not visible in the provided snippet):
1. Locate all occurrences of the messages:
- `Failed to generate runtime core lock`
- `Runtime core lock generation failed`
- `did not create ...` (likely something like `did not create ${outputPath}` or similar)
2. For each of these, wrap the message in a template literal and append diagnostic context, for example:
- `Failed to generate runtime core lock (python: ${runtimePython.absolute}, script: ${generatorScriptPath}).`
- `Runtime core lock generation failed (python: ${runtimePython.absolute}, script: ${generatorScriptPath}).`
- `did not create ${outputPath} (python: ${runtimePython.absolute}, script: ${generatorScriptPath}).`
3. If any of those messages are emitted outside the scope where `runtimePython` is directly available, you will need to either:
- Pass `runtimePython` (or at least `runtimePython.absolute`) down into the relevant function(s), or
- Capture the values in a higher scope that these log/error statements can access.
These changes will ensure all relevant error and diagnostic messages contain both the Python executable path and the generator script path, as requested.
</issue_to_address>
### Comment 2
<location path="scripts/backend/tools/generate_runtime_core_lock.py" line_range="9-12" />
<code_context>
+from pathlib import Path
+
+
+def _read_top_level_modules(distribution: importlib_metadata.Distribution) -> list[str]:
+ try:
+ text = distribution.read_text("top_level.txt") or ""
+ except (FileNotFoundError, KeyError):
+ return []
+
</code_context>
<issue_to_address>
**suggestion:** Consider handling text decoding issues when reading top-level metadata so a single bad distribution does not break lock generation.
Currently `_read_top_level_modules` only ignores `FileNotFoundError` and `KeyError`. If `distribution.read_text("top_level.txt")` hits an unexpected encoding and raises a `UnicodeDecodeError`, lock generation will fail. Since this metadata is best-effort, consider also catching decoding-related errors (e.g. `UnicodeError`) and returning an empty list to keep generation robust against corrupted or non-standard distributions.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Replace brittle runtime core lock source-inspection tests with behavior checks, add generator context to failure messages, and tolerate undecodable top-level metadata. Remove the tool-specific execution note from the implementation plan doc.
Member
Author
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey - I've found 2 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="scripts/backend/runtime-core-lock.mjs" line_range="38" />
<code_context>
+ `Failed to generate runtime core lock ${formatGeneratorContext(runtimePython.absolute)}: ${result.error.message}`,
+ );
+ }
+ if (result.status !== 0) {
+ const detail = result.stderr?.trim() || result.stdout?.trim() || `exit code ${result.status}`;
+ throw new Error(
</code_context>
<issue_to_address>
**suggestion:** Differentiate between non-zero exit codes and signal-based termination from the generator process.
With `spawnSync`, `result.status` is `null` when the child is terminated by a signal (`result.signal` is set). The current logic treats this as a non-zero exit and reports `exit code null`. You could special‑case the signal case for clearer diagnostics, e.g.:
```js
if (result.status !== 0) {
let detail;
if (result.signal) {
detail = `terminated by signal ${result.signal}`;
} else {
detail = result.stderr?.trim() || result.stdout?.trim() || `exit code ${result.status}`;
}
throw new Error(
`Runtime core lock generation failed ${formatGeneratorContext(runtimePython.absolute)}: ${detail}`,
);
}
```
Suggested implementation:
```javascript
if (result.error) {
throw new Error(
`Failed to generate runtime core lock ${formatGeneratorContext(runtimePython.absolute)}: ${result.error.message}`,
);
}
if (result.status !== 0) {
let detail;
if (result.signal) {
detail = `terminated by signal ${result.signal}`;
} else {
detail = result.stderr?.trim() || result.stdout?.trim() || `exit code ${result.status}`;
}
throw new Error(
`Runtime core lock generation failed ${formatGeneratorContext(runtimePython.absolute)}: ${detail}`,
);
}
```
This edit assumes there was not already an `if (result.status !== 0)` block following the `if (result.error)` check. If such a block already exists, replace that existing `if (result.status !== 0)` block with the new one (from `if (result.status !== 0) {` down to the closing `}`) instead of simply inserting it, to avoid duplicate handling.
</issue_to_address>
### Comment 2
<location path="scripts/backend/runtime-core-lock.mjs" line_range="45-49" />
<code_context>
+ );
+ }
+
+ if (!fs.existsSync(outputPath)) {
+ throw new Error(
+ `Runtime core lock generator did not create ${outputPath} ${formatGeneratorContext(runtimePython.absolute)}.`,
</code_context>
<issue_to_address>
**suggestion:** Optionally validate the generated lock file contents, not just its existence.
The postcondition only checks that `outputPath` exists. A truncated or otherwise invalid file (e.g., from a partial write) would still pass. Consider also reading and `JSON.parse`-ing the file (or at least verifying it’s non-empty) and failing in the same error path to catch corruption earlier and protect downstream consumers.
```suggestion
{
const context = formatGeneratorContext(runtimePython.absolute);
const baseMessage = `Runtime core lock generator did not create valid ${outputPath} ${context}.`;
if (!fs.existsSync(outputPath)) {
throw new Error(baseMessage);
}
const contents = fs.readFileSync(outputPath, "utf8");
if (!contents.trim()) {
throw new Error(baseMessage);
}
try {
JSON.parse(contents);
} catch {
throw new Error(baseMessage);
}
}
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Report signal-terminated generator failures clearly and reject invalid generated lock files so broken build artifacts fail fast with actionable diagnostics.
Member
Author
|
@sourcery-ai review |
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
1 participant
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
Generates a
runtime-core-lock.jsonfile during packaged backend resource preparation and exposes it to the bundled backend throughASTRBOT_DESKTOP_CORE_LOCK_PATH. The lock records installed runtime distributions and top-level modules so AstrBot can protect the packaged backend from plugin dependency replacement.Why
Desktop plugin installs write into
ASTRBOT_ROOT/data/site-packages. Without a packaged runtime lock, a plugin can install another copy or version of packages already used by the backend runtime, then the live process may attempt to prefer those modules. That caused Cua installation to trip OpenAI/Pydantic/module identity conflicts until restart.Related
Tests
node --test scripts/backend/runtime-core-lock.test.mjspnpm run test:prepare-resourcespython3 -m py_compile scripts/backend/tools/generate_runtime_core_lock.pySummary by Sourcery
Generate a desktop backend runtime core lock and wire it into the packaged backend launch flow to protect bundled runtime dependencies from being overridden by plugin installs.
New Features:
Documentation:
Tests: