fix(build): validate Discord package after backend dependency install#128
Conversation
* fix: add backend startup heartbeat liveness probe * fix: tighten startup heartbeat validation * refactor: centralize startup heartbeat metadata * fix: surface heartbeat invalidation sooner * fix: harden startup heartbeat parsing * fix: warn on stop-time heartbeat failures * refactor: simplify startup heartbeat control flow * refactor: flatten readiness heartbeat helpers * refactor: clarify heartbeat helper responsibilities * docs: clarify startup heartbeat path coupling * fix: harden startup heartbeat coordination * fix: make startup heartbeat checks monotonic * fix: clean up heartbeat test and exit handling
…#119) * fix(ci): prepare macOS resources before optional signing * test(ci): parse macOS workflow steps structurally * test(ci): parse workflow YAML structurally * test(ci): relax workflow assertions and install test deps * test(ci): share workflow test helpers * test(ci): harden script workflow dependency setup * fix(ci): setup pnpm before enabling pnpm cache * test(ci): relax workflow step assertions
* chore(version): sync desktop version to v4.23.0-beta.1 * fix: add backend startup heartbeat liveness probe (AstrBotDevs#114) * fix: add backend startup heartbeat liveness probe * fix: tighten startup heartbeat validation * refactor: centralize startup heartbeat metadata * fix: surface heartbeat invalidation sooner * fix: harden startup heartbeat parsing * fix: warn on stop-time heartbeat failures * refactor: simplify startup heartbeat control flow * refactor: flatten readiness heartbeat helpers * refactor: clarify heartbeat helper responsibilities * docs: clarify startup heartbeat path coupling * fix: harden startup heartbeat coordination * fix: make startup heartbeat checks monotonic * fix: clean up heartbeat test and exit handling * fix: default desktop chat transport to websocket * fix: respect existing desktop transport preference * fix: harden desktop transport bootstrap * fix: centralize desktop transport contract * fix: harden desktop bridge transport injection --------- Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
![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
validateDiscordPackage, consider also checkingresult.errorfromspawnSyncand including the exit code in the thrown error to distinguish between a Python runtime failure and the assertion failing. - The inline Python assertion string is fairly terse; consider expanding it to a more descriptive message (e.g., mentioning
py-cordvsdiscorddirectly) so that if someone runs the command manually, the failure is self-explanatory even without the Node-side error handling.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `validateDiscordPackage`, consider also checking `result.error` from `spawnSync` and including the exit code in the thrown error to distinguish between a Python runtime failure and the assertion failing.
- The inline Python assertion string is fairly terse; consider expanding it to a more descriptive message (e.g., mentioning `py-cord` vs `discord` directly) so that if someone runs the command manually, the failure is self-explanatory even without the Node-side error handling.
## Individual Comments
### Comment 1
<location path="scripts/backend/build-backend.mjs" line_range="588-591" />
<code_context>
+const validateDiscordPackage = (runtimePython) => {
+ const result = spawnSync(runtimePython.absolute, [
+ '-c',
+ 'import discord; assert hasattr(discord, "Bot"), "discord.Bot not found"',
+ ], {
+ encoding: 'utf8',
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Avoid relying on Python `assert` for validation in the subprocess command.
With `-O` or `PYTHONOPTIMIZE` enabled, `assert` statements are removed, so this validation will never run and an invalid `discord` package could pass. Use an explicit conditional and raise (e.g. `SystemExit` or `RuntimeError`) so the check always runs regardless of optimization settings.
```suggestion
const result = spawnSync(runtimePython.absolute, [
'-c',
'import discord, sys; sys.exit("discord.Bot not found") if not hasattr(discord, "Bot") else None',
], {
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
zouyonghe
force-pushed
the
fix/discord-package-validation
branch
from
cf48189 to
6771cef
Compare
|
@sourcery-ai review |
|
Thanks for the review. All comments have been addressed in the amended commit:
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a validation step in the backend build process to ensure the correct Discord library (py-cord) is installed. It adds a validateDiscordPackage function that runs a Python check and integrates it into the main build sequence. Feedback suggests enhancing the spawnSync implementation by handling execution errors, utilizing a consistent environment, and improving error message clarity.
| const result = spawnSync(runtimePython.absolute, [ | ||
| '-c', | ||
| 'import discord, sys; ' + | ||
| 'has_bot = hasattr(discord, "Bot"); ' + | ||
| 'sys.exit("The installed \"discord\" package is the PyPI stub (not py-cord). "' + | ||
| '"discord.Bot is missing. Please clean pip cache and reinstall py-cord.") ' + | ||
| 'if not has_bot else None', | ||
| ], { | ||
| encoding: 'utf8', | ||
| cwd: outputDir, | ||
| windowsHide: true, | ||
| }); | ||
| if (result.error) { | ||
| throw new Error( | ||
| `Discord package validation could not run (exit code ${result.status ?? 'unknown'}): ` + |
There was a problem hiding this comment.
The spawnSync call should handle potential execution errors (via result.error) and use a consistent environment by including createPythonInstallEnv(). Additionally, trimming the output and providing the exit code in the error message improves debuggability, aligning with the error handling patterns used in other parts of the build system (e.g., in runtime-core-lock.mjs).
const result = spawnSync(runtimePython.absolute, [
'-c',
'import discord; assert hasattr(discord, "Bot"), "discord.Bot not found"',
], {
encoding: 'utf8',
stdio: ['ignore', 'pipe', 'pipe'],
cwd: outputDir,
env: createPythonInstallEnv(),
windowsHide: true,
});
if (result.error) {
throw new Error('Discord package validation failed to start: ' + result.error.message);
}
if (result.status !== 0) {
const detail = result.stderr?.trim() || result.stdout?.trim() || ('exit code ' + result.status);
throw new Error(
'Discord package validation failed: ' + detail + '. ' +
'This usually means the PyPI "discord" stub package was installed instead of "py-cord". ' +
'Please clean the build environment (pip cache, virtualenv) and retry.'
);
}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The inline Python one-liner in
validateDiscordPackageis quite dense and relies on heavy quoting/concatenation; consider extracting it into a separate multi-line string constant (or even a small helper script file) to improve readability and reduce the risk of subtle escaping issues. - In
validateDiscordPackage, you only check for the presence ofdiscord.Bot; if feasible, you might also assert the expected package origin (e.g., viadiscord.__file__ordiscord.__package__) to make the validation more robust against other unexpecteddiscordimplementations.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The inline Python one-liner in `validateDiscordPackage` is quite dense and relies on heavy quoting/concatenation; consider extracting it into a separate multi-line string constant (or even a small helper script file) to improve readability and reduce the risk of subtle escaping issues.
- In `validateDiscordPackage`, you only check for the presence of `discord.Bot`; if feasible, you might also assert the expected package origin (e.g., via `discord.__file__` or `discord.__package__`) to make the validation more robust against other unexpected `discord` implementations.
## Individual Comments
### Comment 1
<location path="scripts/backend/build-backend.mjs" line_range="600-603" />
<code_context>
+ cwd: outputDir,
+ windowsHide: true,
+ });
+ if (result.error) {
+ throw new Error(
+ `Discord package validation could not run (exit code ${result.status ?? 'unknown'}): ` +
+ `${result.error.message}. This may indicate a Python runtime issue.`,
+ );
+ }
</code_context>
<issue_to_address>
**suggestion:** Include additional spawn diagnostics (signal/stdout/stderr) when the process errors to aid debugging.
When `spawnSync` returns with `result.error`, `status` is often `null` while `signal` and/or `stdout`/`stderr` still carry useful context. Currently we only log `status` and `error.message`, which can obscure issues like `ENOENT`, permissions, or signals. Please include `result.signal` and any available `stdout`/`stderr` in this error path to make failures (e.g. in CI) easier to diagnose.
Suggested implementation:
```javascript
const result = spawnSync(runtimePython.absolute, [
'-c',
'import discord, sys; ' +
'has_bot = hasattr(discord, "Bot"); ' +
'sys.exit("The installed \"discord\" package is the PyPI stub (not py-cord). "' +
'"discord.Bot is missing. Please clean pip cache and reinstall py-cord.") ' +
'if not has_bot else None',
```
```javascript
if (result.error) {
const details = [
`exit status: ${result.status ?? 'unknown'}`,
result.signal ? `signal: ${result.signal}` : null,
result.stdout ? `stdout: ${String(result.stdout).trim()}` : null,
result.stderr ? `stderr: ${String(result.stderr).trim()}` : null,
]
.filter(Boolean)
.join('; ');
throw new Error(
`Discord package validation could not run. This may indicate a Python runtime issue. ` +
`Details: ${details}. Underlying error: ${result.error.message}`,
);
}
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
zouyonghe
force-pushed
the
fix/discord-package-validation
branch
from
6771cef to
635cc8d
Compare
|
Thanks for the review. Changes applied:
|
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- Consider scoping
validateDiscordPackageso it only runs when the Discord adapter/runtime is actually enabled for the build, to avoid failing builds that don’t ship Discord support at all. - The Python
DISCORD_VALIDATION_SCRIPTembeds a fairly detailed error message string; you might want to centralize the user-facing guidance in the Node side to avoid maintaining similar text in two places and to keep localization/consistency simpler.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider scoping `validateDiscordPackage` so it only runs when the Discord adapter/runtime is actually enabled for the build, to avoid failing builds that don’t ship Discord support at all.
- The Python `DISCORD_VALIDATION_SCRIPT` embeds a fairly detailed error message string; you might want to centralize the user-facing guidance in the Node side to avoid maintaining similar text in two places and to keep localization/consistency simpler.
## Individual Comments
### Comment 1
<location path="scripts/backend/build-backend.mjs" line_range="634-641" />
<code_context>
+
+ if (result.status !== 0) {
+ throw new Error(
+ `Discord package validation failed (exit code ${result.status}): ` +
+ `${result.stderr || result.stdout || 'unknown error'}. ` +
+ 'This usually means the PyPI "discord" stub package was installed instead of "py-cord". ' +
+ 'Please clean the build environment (pip cache, virtualenv) and retry.',
</code_context>
<issue_to_address>
**suggestion:** Normalize stderr/stdout output to avoid noisy formatting in error messages.
Because `stderr`/`stdout` may include trailing newlines or extra spacing, consider trimming them before interpolation to keep the error message clean, e.g.:
```js
const stderr = result.stderr?.trim();
const stdout = result.stdout?.trim();
...
`${stderr || stdout || 'unknown error'}. ` +
```
```suggestion
if (result.status !== 0) {
const stderr = result.stderr?.trim();
const stdout = result.stdout?.trim();
throw new Error(
`Discord package validation failed (exit code ${result.status}): ` +
`${stderr || stdout || 'unknown error'}. ` +
'This usually means the PyPI "discord" stub package was installed instead of "py-cord". ' +
'Please clean the build environment (pip cache, virtualenv) and retry.',
);
}
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
zouyonghe
force-pushed
the
fix/discord-package-validation
branch
from
635cc8d to
c4c497c
Compare
|
Thanks for the review. Changes applied:
|
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- The embedded Python script in
DISCORD_VALIDATION_SCRIPTis getting fairly complex; consider moving it into a separate.pyhelper file so that error handling and future validation logic remain easier to maintain and edit without touching the build script. - In
validateDiscordPackage, JSON parsing assumesstdoutcontains only the JSON payload; if other output (e.g., warnings) sneaks in, parsing will fail silently—consider constraining the Python script to only print JSON or adding a more robust way to locate the JSON segment instdout.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The embedded Python script in `DISCORD_VALIDATION_SCRIPT` is getting fairly complex; consider moving it into a separate `.py` helper file so that error handling and future validation logic remain easier to maintain and edit without touching the build script.
- In `validateDiscordPackage`, JSON parsing assumes `stdout` contains only the JSON payload; if other output (e.g., warnings) sneaks in, parsing will fail silently—consider constraining the Python script to only print JSON or adding a more robust way to locate the JSON segment in `stdout`.
## Individual Comments
### Comment 1
<location path="scripts/backend/build-backend.mjs" line_range="587-595" />
<code_context>
}
};
+const DISCORD_VALIDATION_SCRIPT = `
+import discord, json, sys
+
+result = {"ok": True, "file": getattr(discord, "__file__", None)}
+if not hasattr(discord, "Bot"):
+ result["ok"] = False
+ result["missing"] = ["discord.Bot"]
+print(json.dumps(result))
+sys.exit(0 if result["ok"] else 1)
+`;
+
</code_context>
<issue_to_address>
**suggestion:** Handle ImportError in the Python validation script to provide clearer diagnostics when `discord` is not installed at all.
Right now, if `import discord` raises ImportError, the script exits before emitting JSON, so the JS side only sees generic stderr/stdout. To keep structured diagnostics even in this case, wrap the import in try/except and always emit a JSON result, e.g.:
```python
import json, sys
result = {"ok": False, "missing": ["discord"], "file": None}
try:
import discord
result["file"] = getattr(discord, "__file__", None)
if not hasattr(discord, "Bot"):
result["missing"] = ["discord.Bot"]
else:
result["ok"] = True
except ImportError:
pass
print(json.dumps(result))
sys.exit(0 if result["ok"] else 1)
```
This preserves the current Bot check while adding a clear signal when the package is missing entirely.
</issue_to_address>
### Comment 2
<location path="scripts/backend/build-backend.mjs" line_range="619-622" />
<code_context>
+ .filter(Boolean)
+ .join('; ');
+
+ throw new Error(
+ 'Discord package validation could not run. ' +
+ `This may indicate a Python runtime issue. Details: ${details}. ` +
+ `Underlying error: ${result.error.message}`,
+ );
+ }
</code_context>
<issue_to_address>
**suggestion:** Include interpreter identity in the error message when the validation spawn fails.
Since multiple Python runtimes may be present on the build machine, please include which executable was used (e.g., `runtimePython.absolute` or a short label) in this error message so we can quickly identify misconfigured or incompatible runtimes when validation spawn fails.
Suggested implementation:
```javascript
if (result.error) {
const details = [
`exit status: ${result.status ?? 'unknown'}`,
result.signal ? `signal: ${result.signal}` : null,
result.stdout ? `stdout: ${String(result.stdout).trim()}` : null,
result.stderr ? `stderr: ${String(result.stderr).trim()}` : null,
]
.filter(Boolean)
.join('; ');
const interpreterLabel =
(runtimePython && ('label' in runtimePython) && runtimePython.label) ||
(runtimePython && ('absolute' in runtimePython) && runtimePython.absolute) ||
'unknown Python interpreter';
throw new Error(
'Discord package validation could not run. ' +
`Interpreter: ${interpreterLabel}. ` +
`This may indicate a Python runtime issue. Details: ${details}. ` +
`Underlying error: ${result.error.message}`,
);
}
```
These changes assume a `runtimePython` object is already available in this scope with either a `label` or `absolute` property (matching the rest of your build script). If that is not yet the case, you should:
1. Ensure the value returned from your Python runtime detection logic is assigned to a variable named `runtimePython` in the same scope as this error handling block.
2. Confirm that the object includes a `label` (e.g., "system Python 3.11") and/or `absolute` (full path to the interpreter) property so the error message can precisely identify the interpreter.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
zouyonghe
force-pushed
the
fix/discord-package-validation
branch
from
c4c497c to
d8b901e
Compare
|
Thanks for the review. Changes applied:
|
|
Thanks for the detailed review. Changes applied:
|
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The JSON protocol between
validate_discord.pyandvalidateDiscordPackageassumes that nothing else writes to stdout, but imports (or environment hooks) can emit unexpected output; consider making the script write JSON to stdout only via a clear delimiter or by forcing other messages to stderr so the Node side can reliably parse it. - To avoid build hangs if the runtime Python process blocks while running
validate_discord.py, consider adding atimeoutto thespawnSynccall invalidateDiscordPackageand handling that case explicitly in the error message.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The JSON protocol between `validate_discord.py` and `validateDiscordPackage` assumes that nothing else writes to stdout, but imports (or environment hooks) can emit unexpected output; consider making the script write JSON to stdout only via a clear delimiter or by forcing other messages to stderr so the Node side can reliably parse it.
- To avoid build hangs if the runtime Python process blocks while running `validate_discord.py`, consider adding a `timeout` to the `spawnSync` call in `validateDiscordPackage` and handling that case explicitly in the error message.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
zouyonghe
force-pushed
the
fix/discord-package-validation
branch
from
43a7ceb to
77cef58
Compare
|
Thanks for the review. Changes applied:
|
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The
ASTRBOT_VALIDATE_DISCORD_JSONprefix string is duplicated in bothbuild-backend.mjsandvalidate_discord.py; consider centralizing it (or at least adding a shared comment) so future changes don’t accidentally desynchronize the producer/consumer formats.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `ASTRBOT_VALIDATE_DISCORD_JSON` prefix string is duplicated in both `build-backend.mjs` and `validate_discord.py`; consider centralizing it (or at least adding a shared comment) so future changes don’t accidentally desynchronize the producer/consumer formats.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
zouyonghe
force-pushed
the
fix/discord-package-validation
branch
from
77cef58 to
0e3543b
Compare
|
@sourcery-ai review |
|
Good catch. Added cross-reference comments in both files:
Since the two files are in different languages (Python vs. JavaScript), a shared constant file isn't practical here; the comments serve as the lightweight coupling contract. |
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The
DISCORD_JSON_PREFIXvalue is duplicated and manually kept in sync betweenbuild-backend.mjsandvalidate_discord.py; consider passing it via an environment variable or CLI argument so the prefix is defined in one place and avoids future drift.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `DISCORD_JSON_PREFIX` value is duplicated and manually kept in sync between `build-backend.mjs` and `validate_discord.py`; consider passing it via an environment variable or CLI argument so the prefix is defined in one place and avoids future drift.
## Individual Comments
### Comment 1
<location path="scripts/backend/tools/validate_discord.py" line_range="18-19" />
<code_context>
+ result["missing"] = []
+except ImportError:
+ pass
+except Exception as e:
+ result["error"] = str(e)
+
+# Emit a single prefixed JSON line on stdout so the caller can reliably
</code_context>
<issue_to_address>
**suggestion:** Capture slightly richer exception info to aid diagnosing unexpected import-time failures.
`str(e)` drops useful context like the exception type. Consider including the class name or a repr (e.g. `f"{type(e).__name__}: {e}"`) so `validateDiscordPackage` surfaces more actionable errors for unexpected import-time failures.
```suggestion
except Exception as e:
result["error"] = f"{type(e).__name__}: {e}"
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
zouyonghe
force-pushed
the
fix/discord-package-validation
branch
from
0e3543b to
b39e280
Compare
|
Thanks for the review. Changes applied:
Both cross-reference comments have been removed since the hard-coded sync is no longer needed. |
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In
validate_discord.py, when anImportErroroccurs you currently just leave the defaultmissingvalue; consider recording the actual exception message (similar to the genericExceptionbranch) to provide clearer diagnostics when the module cannot be imported at all. - The
DISCORD_JSON_PREFIXconstant is effectively duplicated betweenbuild-backend.mjsand the default prefix invalidate_discord.py; consider deriving the prefix solely from the CLI argument (and removing the hardcoded default/value in the script) to avoid these two strings drifting out of sync.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `validate_discord.py`, when an `ImportError` occurs you currently just leave the default `missing` value; consider recording the actual exception message (similar to the generic `Exception` branch) to provide clearer diagnostics when the module cannot be imported at all.
- The `DISCORD_JSON_PREFIX` constant is effectively duplicated between `build-backend.mjs` and the default prefix in `validate_discord.py`; consider deriving the prefix solely from the CLI argument (and removing the hardcoded default/value in the script) to avoid these two strings drifting out of sync.
## Individual Comments
### Comment 1
<location path="scripts/backend/build-backend.mjs" line_range="615-624" />
<code_context>
+
+ if (result.error) {
+ const isTimeout = result.error.code === 'ETIMEDOUT';
+ const details = [
+ `exit status: ${result.status ?? 'unknown'}`,
+ result.signal ? `signal: ${result.signal}` : null,
+ result.stdout ? `stdout: ${String(result.stdout).trim()}` : null,
+ result.stderr ? `stderr: ${String(result.stderr).trim()}` : null,
+ ]
+ .filter(Boolean)
+ .join('; ');
+
+ throw new Error(
</code_context>
<issue_to_address>
**suggestion:** Include the spawnSync error code in the diagnostic details to aid debugging.
`details` currently omits `result.error.code`, so failures due to `error` (e.g., ETIMEDOUT vs ENOENT) look the same in logs aside from the trailing "Underlying error" text. Please add the error code to `details` (e.g., `error code: ${result.error.code}`) so we can distinguish timeout from other failures directly from the main diagnostic string.
```suggestion
if (result.error) {
const isTimeout = result.error.code === 'ETIMEDOUT';
const details = [
`exit status: ${result.status ?? 'unknown'}`,
`error code: ${result.error.code ?? 'unknown'}`,
result.signal ? `signal: ${result.signal}` : null,
result.stdout ? `stdout: ${String(result.stdout).trim()}` : null,
result.stderr ? `stderr: ${String(result.stderr).trim()}` : null,
]
.filter(Boolean)
.join('; ');
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
zouyonghe
force-pushed
the
fix/discord-package-validation
branch
from
b39e280 to
f6521af
Compare
|
@sourcery-ai review |
|
All three points addressed:
|
SourceryAI
left a comment
SourceryAI
left a comment
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- In
validate_discord.py, consider handling the case wheresys.argv[1]is missing (e.g., defaulting to an empty prefix or raising a clearer error) to avoid an IndexError if the script is invoked incorrectly. - In
validateDiscordPackage, you currently rely on stdout parsing for diagnostics; it might be useful to log the raw stdout/stderr at a debug level when JSON extraction fails to make troubleshooting unexpected failures easier.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `validate_discord.py`, consider handling the case where `sys.argv[1]` is missing (e.g., defaulting to an empty prefix or raising a clearer error) to avoid an IndexError if the script is invoked incorrectly.
- In `validateDiscordPackage`, you currently rely on stdout parsing for diagnostics; it might be useful to log the raw stdout/stderr at a debug level when JSON extraction fails to make troubleshooting unexpected failures easier.Hi @zouyonghe! 👋
Thanks for trying out Sourcery by commenting with @sourcery-ai review! 🚀
Install the sourcery-ai bot to get automatic code reviews on every pull request ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- In
extractPrefixedJson, consider splitting on a regex like/\r?\n/to handle Windows-style line endings more robustly when parsing the prefixed JSON line from stdout. - In
validate_discord.py, accessingsys.argv[1]will raise an IndexError if the script is executed manually without arguments; adding a default prefix or a short usage guard would make the helper more robust outside the build pipeline.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `extractPrefixedJson`, consider splitting on a regex like `/\r?\n/` to handle Windows-style line endings more robustly when parsing the prefixed JSON line from stdout.
- In `validate_discord.py`, accessing `sys.argv[1]` will raise an IndexError if the script is executed manually without arguments; adding a default prefix or a short usage guard would make the helper more robust outside the build pipeline.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
zouyonghe
force-pushed
the
fix/discord-package-validation
branch
from
f6521af to
99d91da
Compare
|
Quick polish applied:
|
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- Consider making the Discord validation timeout configurable (e.g., via env or a shared constant near other build timeouts) so slower CI environments can adjust it without code changes.
- The error message in
validateDiscordPackageassumes the PyPIdiscordstub is the issue; you might want to slightly generalize this text or include a hint that other mis-installs could also cause the validation to fail, while still mentioning the stub case explicitly. - Since
validate_discord.pyis tied closely to the JSON prefix protocol, it could be helpful to add a brief comment inbuild-backend.mjsnearDISCORD_JSON_PREFIXreferencing this script so future refactors keep the contract between the JS and Python consistent.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider making the Discord validation timeout configurable (e.g., via env or a shared constant near other build timeouts) so slower CI environments can adjust it without code changes.
- The error message in `validateDiscordPackage` assumes the PyPI `discord` stub is the issue; you might want to slightly generalize this text or include a hint that other mis-installs could also cause the validation to fail, while still mentioning the stub case explicitly.
- Since `validate_discord.py` is tied closely to the JSON prefix protocol, it could be helpful to add a brief comment in `build-backend.mjs` near `DISCORD_JSON_PREFIX` referencing this script so future refactors keep the contract between the JS and Python consistent.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
The PyPI 'discord' stub package (v0.0.2) shadows the real 'py-cord' package because both occupy the 'discord' namespace. When the stub is installed by mistake (e.g. cache pollution or dependency resolution glitch), the Discord adapter fails at runtime with: module 'discord' has no attribute 'Bot' This change adds a post-install validation step that imports 'discord' and asserts 'discord.Bot' exists. If the stub is present, the build fails early with a clear error message instead of producing a broken runtime bundle. Closes #8016
zouyonghe
force-pushed
the
fix/discord-package-validation
branch
from
99d91da to
893aa94
Compare
|
Quick polish applied:
|
|
@sourcery-ai review |
2 participants
Summary
Fixes #8016.
The PyPI
discordstub package (v0.0.2) shadows the realpy-cordpackage because both occupy thediscordnamespace. When the stub is installed by mistake (e.g. cache pollution or dependency resolution glitch), the Discord adapter fails at runtime with:This is a packaging/runtime issue specific to the desktop bundle, not a bug in the core Discord adapter code.
Changes
build-backend.mjsthat importsdiscordand assertsdiscord.Botexists.Impact
scripts/backend/build-backend.mjs).Summary by Sourcery
Add a post-install validation step to ensure the correct Discord package is present in the desktop backend runtime before generating the bundle.
Bug Fixes:
Build: