fix(validate): diagnose plugin manifests without importing plugin code (#765) - #830
Conversation
|
Warning Review limit reachedNext included review available in 9 minutes. View limit detailsLimit details: You’ve used all 2 included reviews currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (7)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
712c765 to
83397ed
Compare
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 83397eddb4
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| {"repo_root": str(paths.repo_root), "project": str(paths.project)}, | ||
| ) | ||
|
|
||
| _validate_plugin_manifests(project, report) |
There was a problem hiding this comment.
Validate manifests from the configured repository root
When the BMAD config points repo_root at a different checkout under supported isolation = "none", this call scans <project>/.bmad-loop/plugins, while Engine builds its registry from self.paths.repo_root in src/bmad_loop/engine.py. A malformed manifest in the tree the run actually loads therefore passes validate and still fails during engine startup after the run is published; conversely, a manifest the run ignores can fail validation. Use the same resolved root as the runtime when load_paths succeeds.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 2bd1317: validate now parses manifests from paths.repo_root (the root Engine hands PluginRegistry.build), falling back to the project dir only when the BMAD config itself failed. Covered by test_validate_reads_plugin_manifests_from_the_configured_repo_root (both directions; ablated).
83397ed to
34d8e4e
Compare
34d8e4e to
b50930c
Compare
b50930c to
90af5b4
Compare
…765) cmd_validate now parses every discovered plugin.toml through the manifest-only load_plugins and reports a new plugins.manifests finding: a PluginError is a problem (the message names the manifest path), a third-party manifest skipped for an unsupported api_version is a warning captured from warnings.warn so nothing leaks to stderr, and a clean pass is an ok line listing the loaded plugins. Previously the first reader of a malformed project manifest was PluginRegistry.build in Engine.__init__, after the run was published.
90af5b4 to
0fe0c0f
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2bd131767c
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| manifests = load_plugins(root) | ||
| except PluginError as e: |
There was a problem hiding this comment.
Report plugin discovery I/O errors as findings
If .bmad-loop/plugins can be statted but cannot be enumerated—for example, it lacks read permission or its filesystem returns a transient I/O error—load_plugins() propagates the OSError from Path.iterdir(), while this new boundary catches only PluginError. The command then falls through to main()'s generic error backstop instead of producing a plugins.manifests problem; with validate --json, stdout is empty and stderr contains prose, violating the command's required one-object JSON contract. Catch or domain-wrap discovery I/O failures here.
AGENTS.md reference: AGENTS.md:L32-L32
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in e637927, at the loader rather than the validate boundary so the engine start path gets the typed error too: _discover_project now converts a failed iterdir() into PluginError("plugin dir <path>: unreadable: …"), matching what _read_manifest_text already does for a manifest read. validate --json reports it as a plugins.manifests problem inside the one document. Covered by test_unlistable_project_plugins_dir_raises_plugin_error (loader) and test_validate_json_reports_an_unlistable_plugins_dir (the --json contract); both ablated.
…ror (#765) _discover_project listed .bmad-loop/plugins with a bare iterdir(), so a dir that exists but cannot be enumerated escaped load_plugins as an OSError. validate's plugins.manifests boundary catches only PluginError, so --json emitted no document and prose on stderr. Same conversion the manifest read already gets in _read_manifest_text.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e637927cc9
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| manifests = load_plugins(root) | ||
| except PluginError as e: |
There was a problem hiding this comment.
Handle built-in discovery I/O as a manifest finding
When the packaged bmad_loop.data/plugins directory exists but cannot be enumerated because of permissions or a transient I/O error, _discover_builtin() still lets the OSError from packaged.iterdir() escape. Since this boundary catches only PluginError, validate --json reaches main()'s generic backstop and emits prose on stderr with no JSON object on stdout; the new wrapping of the project plugin directory does not cover this built-in path. Convert built-in enumeration failures to PluginError as well.
AGENTS.md reference: AGENTS.md:L32-L32
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 66280f1, for the whole class rather than the built-in listing alone: enumeration for both sources now goes through _plugin_dirs, which converts any OSError into PluginError("plugin dir <root>: unreadable: …"). That covers the root existence probe, the listing and the per-entry is_dir/is_file probes (on the 3.11 floor those raise on EACCES rather than returning False). Together with _read_manifest_text, no filesystem fault in discovery reaches validate's boundary untyped. Covered by test_unreadable_project_plugin_discovery_raises_plugin_error (list-root / probe-root / probe-manifest) and test_unlistable_builtin_plugins_dir_raises_plugin_error; all ablated.
…o PluginError (#765) e637927 wrapped only the project dir's iterdir(). The packaged dir's listing was still bare, and on the 3.11 floor Path.is_dir()/is_file() raise on EACCES, so the existence probe and per-entry probes of both sources could still escape load_plugins as OSError. Move enumeration for both sources into _plugin_dirs behind one conversion; the project side keeps its prior Path sibling order via os.path.normcase.
|
@codex review |
|
Codex Review: Didn't find any major issues. Nice work! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Closes #765.
Summary
cmd_validatenow parses every discoveredplugin.tomlwith the manifest-onlyload_pluginsand reports a newplugins.manifestsfinding. No plugin code is imported:PluginErroris reported as a problem, and the message names the manifest path.api_versionis reported as a warning. The warning is captured fromwarnings.warn, so nothing leaks to stderr.PluginRegistry.buildinEngine.__init__, which runs after the run has been published.Notes for reviewers
load_pluginsis labelled "skipped plugin". Today the onlywarnings.warnin the plugin loader is theapi_versionskip. Filtering the captured warnings by category would make this robust if other warnings are added later. That is a possible follow-up.Stack
Part of a stack; merge bottom-up. This is layer 5 of 8, on top of #829.
Stack order (bottom → top): #826 → #827 → #828 → #829 → #830 → #831 → #832 → #833.