feat(plugin): gate activation and search on setup readiness#571
Merged
Conversation
2 tasks
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (15)
📝 WalkthroughWalkthroughRuntime readiness evaluation added: process environment is captured into a Changes
Sequence Diagram(s)sequenceDiagram
participant Executor as Spec Executor
participant Context as PluginSetupReadinessContext
participant Planner as PluginTranslator
participant Readiness as Setup Evaluator
participant Search as Tool Search
Executor->>Context: Build from process env
Executor->>Planner: plan_activation(translation, matrix, context)
Planner->>Readiness: evaluate_plugin_setup_requirements(required_envs, required_configs, context)
Readiness-->>Planner: readiness result (ready/missing lists)
alt Bridge & adapter OK
alt Setup requirements met
Planner->>Planner: Mark plugin Ready
else Setup incomplete
Planner->>Planner: Mark plugin SetupIncomplete (attach missing items)
end
else Bridge or adapter unsupported
Planner->>Planner: Mark plugin Unsupported
end
Planner-->>Executor: PluginActivationPlan
Executor->>Search: execute_tool_search(..., context)
Search->>Readiness: evaluate per-entry requirements
Readiness-->>Search: populate setup_ready and missing_required_* fields
Search-->>Executor: ToolSearchResult
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
github-actions
Bot
added
kernel
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/kernel/src/plugin_ir.rs (1)
126-156:⚠️ Potential issue | 🟠 MajorAdd
#[serde(default)]to new struct fields and document enum variant compatibility.Lines 146–147 (
missing_required_env_vars,missing_required_config_keys) and line 155 (setup_incomplete_plugins) lack#[serde(default)]attributes. Without them, deserialization of older payloads—which omit these fields—will fail. Additionally, the newSetupIncompleteenum variant (line 130) requires documented compatibility guidance for external consumers. Per coding guidelines, kernel contracts must be backward-compatible with no breaking changes without documented decision.🧩 Minimal serde compatibility patch
pub struct PluginActivationCandidate { pub plugin_id: String, pub source_path: String, pub source_kind: PluginSourceKind, pub package_root: String, pub package_manifest_path: Option<String>, pub bridge_kind: PluginBridgeKind, pub adapter_family: String, pub status: PluginActivationStatus, pub reason: String, + #[serde(default)] pub missing_required_env_vars: Vec<String>, + #[serde(default)] pub missing_required_config_keys: Vec<String>, pub bootstrap_hint: String, } pub struct PluginActivationPlan { pub total_plugins: usize, pub ready_plugins: usize, + #[serde(default)] pub setup_incomplete_plugins: usize, pub blocked_plugins: usize, pub candidates: Vec<PluginActivationCandidate>, }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/kernel/src/plugin_ir.rs` around lines 126 - 156, Add serde defaulting for the newly added optional fields and document the enum change: annotate PluginActivationCandidate::missing_required_env_vars and ::missing_required_config_keys with #[serde(default)] so older payloads missing those fields deserialize to empty Vecs, and annotate PluginActivationPlan::setup_incomplete_plugins with #[serde(default)] so missing counts default to 0; also add a brief compatibility note in the kernel contract/docs that the PluginActivationStatus enum gained the SetupIncomplete variant and describe its semantics and backward-compatibility expectations for external consumers (i.e., clients should treat unknown/new variants gracefully or update parsing accordingly).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/daemon/tests/integration/spec_runtime.rs`:
- Around line 1133-1160: The test assumes TAVILY_API_KEY is missing which is
non-deterministic; before asserting readiness from the plugin file (the block
that writes tavily_search.py using plugin_root and plugin_file and then checks
readiness), explicitly clear or override the environment key (call
std::env::remove_var("TAVILY_API_KEY") or set it to an empty value with
std::env::set_var("TAVILY_API_KEY", "")) so the missing-env path is
deterministic, and apply the same change for the other occurrences around the
plugin setup (the section spanning the current lines ~1232-1235).
In `@crates/kernel/src/plugin_ir.rs`:
- Around line 87-116: The current contains() check in
evaluate_plugin_setup_requirements incorrectly treats env var names as
case-sensitive on Windows; update the env var verification to perform
platform-aware matching by using case-insensitive comparison on Windows (e.g.
use eq_ignore_ascii_case or compare lowercased strings) when checking
required_env_var against entries in context.verified_env_vars (inside the loop
over required_env_vars and the env_var_is_verified check), while keeping the
existing case-sensitive behavior on non-Windows platforms so config keys and
other logic remain unchanged.
In `@crates/spec/src/spec_execution.rs`:
- Around line 783-794: The loop over env_vars currently trims raw_name before
inserting into verified_env_vars which can turn malformed keys like "
TAVILY_API_KEY" into "TAVILY_API_KEY" and incorrectly mark readiness; change the
logic so you only use trim() to test for blankness but preserve the original
raw_name when inserting. Concretely, in the env_vars iteration (variables
raw_name, raw_value, verified_env_vars) convert raw_name.to_string_lossy() to a
temporary (e.g., name_str), use name_str.trim().is_empty() to skip blanks, but
insert the untrimmed name_str (not the trimmed version) into verified_env_vars;
apply the same blank-check approach to raw_value but keep the stored value
handling unchanged.
---
Outside diff comments:
In `@crates/kernel/src/plugin_ir.rs`:
- Around line 126-156: Add serde defaulting for the newly added optional fields
and document the enum change: annotate
PluginActivationCandidate::missing_required_env_vars and
::missing_required_config_keys with #[serde(default)] so older payloads missing
those fields deserialize to empty Vecs, and annotate
PluginActivationPlan::setup_incomplete_plugins with #[serde(default)] so missing
counts default to 0; also add a brief compatibility note in the kernel
contract/docs that the PluginActivationStatus enum gained the SetupIncomplete
variant and describe its semantics and backward-compatibility expectations for
external consumers (i.e., clients should treat unknown/new variants gracefully
or update parsing accordingly).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 224d0b45-4f57-4623-a5b5-99f0a6c0504a
📒 Files selected for processing (7)
crates/daemon/tests/integration/spec_runtime.rscrates/kernel/src/bootstrap.rscrates/kernel/src/lib.rscrates/kernel/src/plugin_ir.rscrates/spec/src/spec_execution.rscrates/spec/src/spec_execution/tool_search.rscrates/spec/src/spec_runtime.rs
This was referenced Mar 25, 2026
github-actions
Bot
added
documentation
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)
crates/kernel/src/plugin_ir.rs (1)
179-193:⚠️ Potential issue | 🟠 MajorInclude setup-incomplete candidates in
has_blockers().Line 190 only checks
blocked_plugins. After addingsetup_incomplete_plugins, this can returnfalseeven when no plugin is actually activatable.Proposed fix
impl PluginActivationPlan { #[must_use] pub fn has_blockers(&self) -> bool { - self.blocked_plugins > 0 + self.blocked_plugins > 0 || self.setup_incomplete_plugins > 0 } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/kernel/src/plugin_ir.rs` around lines 179 - 193, The has_blockers() method on struct PluginActivationPlan currently only checks blocked_plugins; update it to also consider setup_incomplete_plugins so it returns true when either blocked_plugins > 0 OR setup_incomplete_plugins > 0 (i.e., any non-zero blocker count). Modify the PluginActivationPlan::has_blockers implementation to include setup_incomplete_plugins in the condition (referencing PluginActivationPlan, has_blockers, blocked_plugins, and setup_incomplete_plugins) so plans with setup-incomplete candidates are treated as blocked.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@crates/kernel/src/plugin_ir.rs`:
- Around line 179-193: The has_blockers() method on struct PluginActivationPlan
currently only checks blocked_plugins; update it to also consider
setup_incomplete_plugins so it returns true when either blocked_plugins > 0 OR
setup_incomplete_plugins > 0 (i.e., any non-zero blocker count). Modify the
PluginActivationPlan::has_blockers implementation to include
setup_incomplete_plugins in the condition (referencing PluginActivationPlan,
has_blockers, blocked_plugins, and setup_incomplete_plugins) so plans with
setup-incomplete candidates are treated as blocked.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ea0521fd-1fa0-440b-8d97-cfcc0c8d1d4d
📒 Files selected for processing (4)
crates/daemon/tests/integration/spec_runtime.rscrates/kernel/src/plugin_ir.rscrates/spec/src/spec_execution.rsdocs/releases/architecture-drift-2026-03.md
✅ Files skipped from review due to trivial changes (2)
- docs/releases/architecture-drift-2026-03.md
- crates/daemon/tests/integration/spec_runtime.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/spec/src/spec_execution.rs
chumyin
force-pushed
the
feature/plugin-setup-readiness-20260325
branch
from
fb93c6b to
b192530
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/spec/src/spec_execution.rs (1)
771-774: Config keys intentionally empty per PR scope.Setting
verified_config_keys: BTreeSet::new()means config-key readiness will always report missing. This aligns with the PR scope ("does not inject explicit readiness context from RunnerSpec") with follow-up tracked in#560.Consider adding a brief comment explaining this is intentional for future readers:
📝 Optional documentation improvement
PluginSetupReadinessContext { verified_env_vars, + // Config key verification deferred to follow-up (`#560`); empty means all config requirements report missing. verified_config_keys: BTreeSet::new(), }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/spec/src/spec_execution.rs` around lines 771 - 774, The PluginSetupReadinessContext currently initializes verified_config_keys with BTreeSet::new(), which intentionally leaves config-key readiness empty per PR scope; add a concise inline comment next to the verified_config_keys: BTreeSet::new() initialization mentioning this is deliberate (e.g., "intentionally empty — RunnerSpec readiness not injected in this PR; tracked in `#560`") so future readers understand this is expected behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@crates/spec/src/spec_execution.rs`:
- Around line 771-774: The PluginSetupReadinessContext currently initializes
verified_config_keys with BTreeSet::new(), which intentionally leaves config-key
readiness empty per PR scope; add a concise inline comment next to the
verified_config_keys: BTreeSet::new() initialization mentioning this is
deliberate (e.g., "intentionally empty — RunnerSpec readiness not injected in
this PR; tracked in `#560`") so future readers understand this is expected
behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 20dd37b0-cd4b-4660-b767-e2c79ab2fc59
📒 Files selected for processing (8)
crates/daemon/tests/integration/spec_runtime.rscrates/kernel/src/bootstrap.rscrates/kernel/src/lib.rscrates/kernel/src/plugin_ir.rscrates/spec/src/spec_execution.rscrates/spec/src/spec_execution/tool_search.rscrates/spec/src/spec_runtime.rsdocs/releases/architecture-drift-2026-03.md
✅ Files skipped from review due to trivial changes (4)
- crates/kernel/src/lib.rs
- crates/kernel/src/bootstrap.rs
- docs/releases/architecture-drift-2026-03.md
- crates/daemon/tests/integration/spec_runtime.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/spec/src/spec_runtime.rs
- crates/spec/src/spec_execution/tool_search.rs
github-actions
Bot
added
size: XL
chumyin
force-pushed
the
feature/plugin-setup-readiness-20260325
branch
from
40c628e to
a68e456
Compare
github-actions
Bot
added
the
channels
This was referenced Mar 27, 2026
This was referenced Apr 1, 2026
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
runtime planning still treated plugins as ready once bridge and adapter checks passed, even when manifest-declared required env vars or config keys were missing.
that created a false-ready state in activation and tool search, which weakens operator trust and does not scale for a broader plugin ecosystem.
this re-lands the setup-readiness slice from the earlier stacked work onto current
dev.activation planning now evaluates typed setup requirements against a shared readiness context, marks plugins as
setup_incompletewhen required setup is missing, and carries missing env and config requirements on activation candidates.tool search now exposes
setup_ready,missing_required_env_vars, andmissing_required_config_keys.spec execution derives a conservative default readiness context from non-empty process env vars only, and regression coverage now covers verified, missing, and boundary-value cases.
this does not inject explicit readiness context from
RunnerSpecyet, does not mutate user config, and does not add plugin-specific heuristics. explicit config and env readiness injection remains follow-up scope in [Feature]: Allow spec runners to inject verified plugin setup context #560.Linked Issues
Change Type
Touched Areas
Risk Track
Validation
cargo fmt --all -- --checkcargo clippy --workspace --all-targets --all-features -- -D warningscargo test --workspace --locked -- --test-threads=1cargo test --workspace --all-features --locked -- --test-threads=1Commands and evidence:
User-visible / Operator-visible Changes
Failure Recovery
revert this PR to restore the previous bridge-and-adapter-only readiness behavior.
plugins without setup requirements being downgraded unexpectedly, or tool-search results omitting readiness fields for setup-aware plugins.
Reviewer Focus
crates/kernel/src/plugin_ir.rsfor the shared readiness evaluator and the activation-state split between runtime blockers and setup-incomplete plugins.crates/spec/src/spec_execution.rsandcrates/spec/src/spec_execution/tool_search.rsfor the conservative default readiness context and search-surface wiring.crates/daemon/tests/integration/spec_runtime.rsfor the end-to-end behavior that proves setup-incomplete plugins are surfaced without being treated as ready.Summary by CodeRabbit