feat(components): auto-enforce core-module usage via init/SessionStart - #352
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughClaude Code component management now seeds and refreshes built-in coaching rules, preserves user overrides, and synchronizes gateway permissions in Claude settings. Coaching rule visibility is widened crate-wide, with component lifecycle and idempotency tests added. ChangesCoaching and gateway synchronization
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ClaudeCode as Claude Code
participant Components as get_components()
participant Coaching as core-coaching
participant Gateway as gateway-permissions
participant Settings as ~/.claude/settings.json
ClaudeCode->>Components: discover components
Components->>Coaching: check and apply coaching defaults
Coaching->>Coaching: preserve Override-tier rules
Components->>Gateway: check and apply gateway permissions
Gateway->>Settings: synchronize allow entries
Settings-->>Gateway: changed or unchanged result
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
src/components.rs (3)
1560-1570: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd the symmetric non-claude-code check for
core-coaching.
gateway-permissionshas this coverage, but nothing assertscore-coaching's!claude_code_onlyshort-circuit.💚 Suggested test
#[test] fn core_coaching_is_satisfied_for_non_claude_code_hosts() { crate::paths::test_support::with_temp_home(|| { let components = get_components("opencode"); let cc = components.iter().find(|c| c.id == "core-coaching").unwrap(); assert!((cc.check)()); }); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components.rs` around lines 1560 - 1570, Add a symmetric test beside gateway_permissions_is_satisfied_for_non_claude_code_hosts for the core-coaching component, using get_components("opencode"), locating the "core-coaching" entry, and asserting its check returns true within with_temp_home.
887-898: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueOrdering:
core-coachingseeds after therulescomponent runs.
rule_targets()includes coaching-sourced rules, butrulesis applied earlier in this vector, so on a fresh install the four freshly-seeded rules aren't materialized into the host rule files until the nextinit/SessionStart. Self-healing, but a one-session delay. Consider placingcore-coachingbeforerulesif that write ordering matters.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components.rs` around lines 887 - 898, The component vector currently applies rules before core-coaching seeds its coaching rules, causing newly seeded rules to materialize only on the next run. Reorder the component declarations so core-coaching appears before rules, while preserving both components’ existing checks and apply behavior.
424-433: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueDrift detection only compares
body.
title,tools, andsyncchanges inDEFAULT_COACHING_RULESwon't trigger a refresh on existing installs, so a future edit to those fields silently never propagates. Consider comparing the trigger/sync/title too if those are expected to evolve.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components.rs` around lines 424 - 433, Update coaching_defaults_satisfied to compare every default rule field that should propagate, including title, tools/triggers, sync, and body, rather than only body. Preserve the existing Override-tier behavior and missing-rule handling, while ensuring changes to these fields make the defaults unsatisfied and trigger refresh.src/coaching/mod.rs (1)
23-27: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueComment sits below the item it documents and names rules that don't exist.
The trailing comment describes the
list_rulesre-export above it, so it reads as documenting the next item. It also refers toflare-docs, flare-search, lean-ctx, tool-search, while the actual rule ids insrc/components.rsareusedocs,usesearch,useleanctx,usetsearch.♻️ Suggested reorder/wording
-pub(crate) use store::list_rules; - -// Also used by components.rs to seed/refresh the built-in core-module -// coaching rules (flare-docs, flare-search, lean-ctx, tool-search) on every -// `agentflare init` and SessionStart. +// Also used by components.rs to seed/refresh the built-in core-module +// coaching rules (usedocs, usesearch, useleanctx, usetsearch) on every +// `agentflare init` and SessionStart. +pub(crate) use store::list_rules;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/coaching/mod.rs` around lines 23 - 27, Move the comment above the pub(crate) use store::list_rules re-export so it documents that symbol, and update the listed built-in rule IDs to usedocs, usesearch, useleanctx, and usetsearch, matching the definitions in components.rs.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/components.rs`:
- Around line 497-511: Update the permissions handling in the settings flow to
return an error when the existing permissions value is not an object, matching
the existing permissions.allow validation. Remove the branch that replaces
malformed permissions with an empty object, while preserving creation of an
empty object only when the permissions key is absent.
- Around line 438-473: The apply_coaching_defaults function must surface
failures from coaching::apply_rule instead of discarding them via is_ok(). Track
or report the returned error when any default rule cannot be applied, and ensure
the result is not reported as “already up to date” when seeding fails, so
repeated coaching_defaults_satisfied() failures provide a diagnostic.
---
Nitpick comments:
In `@src/coaching/mod.rs`:
- Around line 23-27: Move the comment above the pub(crate) use store::list_rules
re-export so it documents that symbol, and update the listed built-in rule IDs
to usedocs, usesearch, useleanctx, and usetsearch, matching the definitions in
components.rs.
In `@src/components.rs`:
- Around line 1560-1570: Add a symmetric test beside
gateway_permissions_is_satisfied_for_non_claude_code_hosts for the core-coaching
component, using get_components("opencode"), locating the "core-coaching" entry,
and asserting its check returns true within with_temp_home.
- Around line 887-898: The component vector currently applies rules before
core-coaching seeds its coaching rules, causing newly seeded rules to
materialize only on the next run. Reorder the component declarations so
core-coaching appears before rules, while preserving both components’ existing
checks and apply behavior.
- Around line 424-433: Update coaching_defaults_satisfied to compare every
default rule field that should propagate, including title, tools/triggers, sync,
and body, rather than only body. Preserve the existing Override-tier behavior
and missing-rule handling, while ensuring changes to these fields make the
defaults unsatisfied and trigger refresh.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 47d41d0f-59fc-4940-a08d-e3de7201076a
📒 Files selected for processing (3)
src/coaching/mod.rssrc/coaching/store.rssrc/components.rs
Adds two non-consent components so agentflare init and every SessionStart self-heal the setup needed to actually use flare-docs, flare-search, and lean-ctx through the gateway, instead of relying on hand-run CLI commands: - core-coaching: seeds/refreshes 4 builtin coaching rules (usedocs, usesearch, useleanctx, usetsearch) that nudge the flare gateway's docs/search/lean-ctx/tool-search wrappers over their native equivalents. Drift-protected across version bumps; a same-id rule the user has overridden to a different tier is left alone. - gateway-permissions: keeps ~/.claude/settings.json's permissions.allow containing the flare gateway tools (mcp__flare__docs, mcp__flare__search, mcp__flare__tool, ToolSearch) and strips superseded direct mcp__lean-ctx__* entries. Also widens coaching::store::list_rules to pub(crate) so components.rs can read existing rule state when deciding whether to seed or refresh.
…ings, seed before rules
apply_rule errors were dropped by is_ok(), so a failed seed reported
"already up to date" while the check kept failing every SessionStart with
no diagnostic. A malformed permissions value was replaced with {} and
written back, unlike permissions.allow which errors out. core-coaching
also ran after rules, whose write_if_absent had already written the host
rule files without the four seeded rules -- and whose check then passes
forever, so nothing picked them up later.
4257610 to
2d4b596
Compare
|
Review pass — three findings fixed in 2d4b596, plus 1. Failed
|
1 participant
Summary
core-coachingcomponent that seeds/refreshes 4 builtin coaching rules (usedocs,usesearch,useleanctx,usetsearch) nudging the flare gateway's docs/search/lean-ctx/tool-search wrappers over their native equivalents. Builtin tier is drift-protected across version bumps; a same-id rule the user has retaggedoverrideis left untouched.gateway-permissionscomponent that keeps~/.claude/settings.json'spermissions.allowcontaining the flare gateway tools (mcp__flare__docs,mcp__flare__search,mcp__flare__tool,ToolSearch) and strips superseded directmcp__lean-ctx__*entries.needs_consent: false, soagentflare initand every SessionStart self-heal this setup — no separate wiring needed for "on update," since non-consent components already re-apply on every session start.coaching::store::list_rulestopub(crate)socomponents.rscan inspect existing rule state before deciding to seed/refresh.Test plan
cargo build --bin agentflare— cleancargo clippy --bin agentflare— cleancargo test --bin agentflare -- components:: coaching::— 71/71 pass, including 8 new tests covering idempotency, override-preservation, fresh-seed, and non-claude-code host gatingagentflare init --agent claude-code— both new components report satisfied against real local stateSummary by CodeRabbit