fix(tui): gate recommended_plugins to once per engine, suppressed by loaded skills - #6279
Conversation
…loaded skills The <recommended_plugins> user-turn fragment fired on every turn for any conversation that merely mentioned a plugin's domain keywords, and it could not tell that a loaded local skill already covered the domain — a takeover session talking about handoffs was nudged to install the handoff plugin four times in one evening. A plugin id is now suggested at most once per engine lifetime, and a plugin whose name (or alias) matches a skill in the session's catalogue is never suggested. Dismissals still apply. The suppression snapshot is taken at engine construction from the same catalogue the system prompt indexes (workspace roots + configured skills dir + plugin-sourced skills); a skill installed mid-session is not suppressed until the next engine starts (known limitation, documented on the gate). Closes #6274 Signed-off-by: CodeWhale Bot <bot@codewhale.net>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Claude finished @Hmbown's task in 3m 13s —— View job Reviewing PR #6279
SummaryClean, well-scoped fix. The Observations (non-blocking)
Not able to run Branch: |
![codewhale-agent[bot]](https://avatars.githubusercontent.com/u/101357273?s=60&v=4){kind=small}
There was a problem hiding this comment.
Codewhale review
Complete review coverage: 1/1 passes, 6 file patches, sha256:2b3dec30704bae898accd4d2fe4806e0f4e74ec611b87e46d629e0aab860a17f.
Pass 1: Adds RecommendedPluginGate to the TUI plugin recommender: the append-only <recommended_plugins> user-turn fragment now fires at most once per plugin id per Engine and is suppressed when the plugin's name matches a skill (name or alias) in the catalogue snapshotted at Engine construction. Call sites of recommended_plugins_user_fragment gain a &mut RecommendedPluginGate; the gate check sits after the existing dismissal filter; changelogs and the derived web changelog were regenerated consistently (the Fixed-section item list and itemCount 10→11 line up). The gating logic itself (BTreeSet::insert as the once-only admit, case/whitespace normalization, ordering after match_plugin_for_draft) is correct as written; the findings below are narrow.
Findings
- [INFO] Engine gate mutex is held across filesystem reads and the whole match (
crates/tui/src/core/engine.rs:3744)
The new block takesself.recommended_plugin_gate.lock()and then evaluatescrate::plugins::recommend::load_marketplace_candidates(self.plugin_registry.state_path())(a marketplace-state file read) and the entirerecommended_plugins_user_fragmentcall (which itself doesSettings::load_read_only()plus plugin matching) while the guard is live; the guard is only needed forgate.admits. On a host that composes user turns on more than one thread for the same Engine (the engine is long-lived and the lock is the only synchronisation onshown), the second composer blocks on the first composer's disk I/O under this std mutex. No correctness change — the once-per-engine set stays consistent — but the lock scope is wider than the state it protects.
Suggestions
crates/tui/src/core/engine.rs:3744— Narrow the guarded region: load the marketplace candidates before taking the lock (and ideally let the fragment take the already-loaded slice plus a pre-loaded dismissal set), so the gate mutex only covers theadmitsdecision rather than a state-file read plus settings load plus matching. This keeps the once-per-Engine semantics identical while removing disk I/O from the critical section. Anything that isolates onlyadmitsneeds a small API change torecommended_plugins_user_fragment(it currently borrows the gate mutably for the whole body), so no literal replacement is given here.
Assessment
Pass 1: I read the added gate, its two call sites and the regenerated changelog as source only; no build, test or runtime check was run by this review. The gate semantics are right: admits uses BTreeSet::insert (true on first insert), normalizes both sides with trim().to_ascii_lowercase(), and is consulted after match_plugin_for_draft, so dismissals still short-circuit and a skill-suppressed plugin is never recorded in shown. The two new unit tests do not appear vacuous: the same fixture (write_keyword_bundle(root, "supabase", …)) is asserted to match in the pre-existing test, the once-per-gate test reuses one gate across two matching drafts, and the suppression test's None can only come from the skill set because a fresh Default gate would admit. Material context is missing, so these remain open questions rather than findings: (1) recommended_plugins_user_fragment is a public function whose signature changed; only the engine and two in-crate test call sites appear in this diff, and I could not inspect other crates (its owning crate is codewhale_tui) or any other Engine { … } literal, either of which would fail to build if present — the PR's cargo check -p codewhale-tui --lib would not cover them; (2) the PR asserts the snapshot uses the same catalogue as the system prompt (prompts.rs), but the diff cannot show the prompt-side discovery mode/roots, so if those differ (mode, additional workspace roots, per-turn refresh) suppression would silently diverge from the prompt's skill index; (3) the snapshot is taken once at construction, so skills installed mid-session do not suppress — documented on the gate, but it means the nagging the PR targets can persist for the rest of a long session; (4) suppression compares only the plugin's display name against skill names/aliases, so a plugin that matched the draft through a keyword or domain whose display name differs from the skill name still gets suggested — that matches the stated scope ("name or alias") but is narrower than "the skill owns the domain"; (5) the once-per-Engine id is consumed at fragment-composition time, not at delivery, so a composed turn that is later aborted loses that plugin's single tip for the engine's lifetime — the PR explicitly chooses this "evaluate once, consume" behaviour. Changelog.md / crates/tui/CHANGELOG.md / web/lib/changelog.generated.ts are consistent with each other (one new Fixed item, itemCount 11 matching the 11 entries in that section).
Advisory review by Codewhale (codewhale review --pr 6279 --post, head b3127a7d914c228b65e0c81d0a77a96107fca625). Line-specific findings are also posted as inline review comments; mechanical fixes arrive as committable suggestions you can apply from the Files tab. CODEOWNERS approval still governs merge.
| self.plugin_registry.state_path(), | ||
| ), | ||
| ); | ||
| let recommended_plugins = { |
There was a problem hiding this comment.
[INFO] Engine gate mutex is held across filesystem reads and the whole match
The new block takes self.recommended_plugin_gate.lock() and then evaluates crate::plugins::recommend::load_marketplace_candidates(self.plugin_registry.state_path()) (a marketplace-state file read) and the entire recommended_plugins_user_fragment call (which itself does Settings::load_read_only() plus plugin matching) while the guard is live; the guard is only needed for gate.admits. On a host that composes user turns on more than one thread for the same Engine (the engine is long-lived and the lock is the only synchronisation on shown), the second composer blocks on the first composer's disk I/O under this std mutex. No correctness change — the once-per-engine set stays consistent — but the lock scope is wider than the state it protects.
| ), | ||
| &mut recommended_plugin_gate, | ||
| ) | ||
| }; |
There was a problem hiding this comment.
Narrow the guarded region: load the marketplace candidates before taking the lock (and ideally let the fragment take the already-loaded slice plus a pre-loaded dismissal set), so the gate mutex only covers the admits decision rather than a state-file read plus settings load plus matching. This keeps the once-per-Engine semantics identical while removing disk I/O from the critical section. Anything that isolates only admits needs a small API change to recommended_plugins_user_fragment (it currently borrows the gate mutably for the whole body), so no literal replacement is given here.
Closes #6274
A per-Engine
RecommendedPluginGatenow bounds the append-only<recommended_plugins>user-turn fragment:one-shot tip pattern — evaluate once, consume);
never suggested — the skill owns the domain. The suppression snapshot
is built at engine construction from the same catalogue the system prompt
indexes (workspace roots + configured skills_dir + plugin-sourced skills),
so plugin-sourced and workspace skills both suppress their marketplace
twins. Known limitation (documented on the gate): a skill installed
mid-session is not suppressed until the next engine starts.
never the pinned system prefix.
Verification (this checkout,
-j 5):cargo check -p codewhale-tui --lib -j 5→ Finished dev profile in 1m 15sscripts/dev-test.sh crates/tui/src/plugins/recommend.rs→Summary 13 tests run: 13 passed, 12785 skipped(includes the two newtests: once-per-gate and skill-name suppression)
cargo nextest run -p codewhale-tui --lib -E 'test(dismissed)'→Summary: 2 tests run: 2 passed./scripts/sync-changelog.sh+node web/scripts/derive-changelog.mjsboth run;regenerated files included in the commit.
Live repro that motivated it: four injections in one takeover session
(
handoff,research,remember,release), each tracking the topic ofthe user's message.
No-Issue: none — closes #6274.