Repository navigation
fix active channel extension search guidance #6653
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from all commits
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -772,7 +772,7 @@ impl ExtensionManagementPort { | |
| count, | ||
| }, | ||
| ); | ||
| if extension_search_has_installed_external_channel_result(response.payload.as_ref()) { | ||
| if extension_search_has_setup_needed_external_channel_result(response.payload.as_ref()) { | ||
| response.message = Some( | ||
| "Search found external channel results whose personal setup is incomplete. For an explicit connect, pair, authenticate, or account-access request, call builtin.extension_install for the matching extension id so the manifest-declared connection flow can continue. For routine, trigger, or notification delivery, prefer the configured outbound delivery target when one is available." | ||
| .to_string(), | ||
|
|
@@ -3045,11 +3045,7 @@ fn extension_search_has_ready_result(payload: Option<&LifecycleProductPayload>) | |
| matches!( | ||
| extension.installation_phase, | ||
| Some(LifecyclePublicState::Active) | ||
| ) && !extension | ||
| .summary | ||
| .surface_kinds | ||
| .contains(&CapabilitySurfaceKind::Channel) | ||
| && extension.summary.credential_requirements.is_empty() | ||
| ) && extension.summary.credential_requirements.is_empty() | ||
| && extension.summary.onboarding.is_none() | ||
| }) | ||
| } | ||
|
|
@@ -3071,20 +3067,18 @@ fn extension_search_has_inactive_installed_result( | |
| }) | ||
| } | ||
|
|
||
| fn extension_search_has_installed_external_channel_result( | ||
| fn extension_search_has_setup_needed_external_channel_result( | ||
| payload: Option<&LifecycleProductPayload>, | ||
| ) -> bool { | ||
| let Some(LifecycleProductPayload::ExtensionSearch { extensions, .. }) = payload else { | ||
| return false; | ||
| }; | ||
| extensions.iter().any(|extension| { | ||
| matches!( | ||
| extension.installation_phase, | ||
| Some(LifecyclePublicState::SetupNeeded | LifecyclePublicState::Active) | ||
| ) && extension | ||
| .summary | ||
| .surface_kinds | ||
| .contains(&CapabilitySurfaceKind::Channel) | ||
| extension.installation_phase == Some(LifecyclePublicState::SetupNeeded) | ||
| && extension | ||
| .summary | ||
| .surface_kinds | ||
| .contains(&CapabilitySurfaceKind::Channel) | ||
| }) | ||
| } | ||
|
|
||
|
|
@@ -3390,7 +3384,7 @@ mod tests { | |
| } | ||
|
|
||
| #[test] | ||
| fn installed_external_channel_search_result_gets_activation_guidance() { | ||
| fn setup_needed_external_channel_search_result_gets_activation_guidance() { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This only tests the classifier, not the |
||
| let payload = LifecycleProductPayload::ExtensionSearch { | ||
| extensions: vec![LifecycleSearchExtensionSummary { | ||
| summary: LifecycleExtensionSummary { | ||
|
|
@@ -3418,7 +3412,7 @@ mod tests { | |
| count: 1, | ||
| }; | ||
|
|
||
| assert!(extension_search_has_installed_external_channel_result( | ||
| assert!(extension_search_has_setup_needed_external_channel_result( | ||
| Some(&payload) | ||
| )); | ||
| assert!(!extension_search_has_ready_result(Some(&payload))); | ||
|
|
@@ -4847,11 +4841,11 @@ supports_threads = true | |
| } | ||
|
|
||
| #[tokio::test] | ||
| async fn extension_search_distinguishes_external_channel_connect_from_delivery() { | ||
| // Generic external-channel search guidance. Uses a neutral `example_bot` | ||
| // fixture rather than the real Slack bot: under model B `slack_bot` is | ||
| // hidden from search, so a Slack-named fixture would be filtered out and | ||
| // this generic guidance would go untested. | ||
| async fn active_external_channel_search_result_gets_ready_guidance() { | ||
| // Uses a neutral `example_bot` fixture rather than the real Slack bot: | ||
| // under model B `slack_bot` is hidden from search, so a Slack-named | ||
| // fixture would be filtered out and this generic guidance would go | ||
| // untested. | ||
| let (_dir, _storage_root, facade, _active_registry, _installation_store) = | ||
| extension_lifecycle_fixture_with_catalog_and_service( | ||
| AvailableExtensionCatalog::from_packages(vec![fixture_external_channel_package( | ||
|
|
@@ -4883,16 +4877,13 @@ supports_threads = true | |
|
|
||
| let message = search.message.as_deref().expect("search guidance"); | ||
| assert!( | ||
| message.contains("external channel") | ||
| && message.contains("explicit connect") | ||
| && message.contains("builtin.extension_install") | ||
| && message.contains("outbound delivery target") | ||
| && message.contains("personal setup is incomplete"), | ||
| "active external channel search should distinguish connect requests from delivery, got: {message}" | ||
| message.contains("Treat those results as ready"), | ||
| "active external channel search must use ready guidance, got: {message}" | ||
| ); | ||
| assert!( | ||
| !message.contains("Treat those results as ready"), | ||
| "active external channels must not use ready-extension guidance: {message}" | ||
| !message.contains("setup is incomplete") | ||
| && !message.contains("builtin.extension_install"), | ||
| "active external channel search must not emit setup guidance: {message}" | ||
| ); | ||
| let Some(LifecycleProductPayload::ExtensionSearch { extensions, .. }) = | ||
| search.payload.as_ref() | ||
|
|
||
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Preserve the credential-requirements guard.
This predicate now classifies every
SetupNeededchannel as personal connection setup, including results with unresolvedcredential_requirements. Restore the empty-requirements check so credential-gated extensions do not receive the channel-specificbuiltin.extension_installguidance.Proposed fix
extensions.iter().any(|extension| { extension.installation_phase == Some(LifecyclePublicState::SetupNeeded) + && extension.summary.credential_requirements.is_empty() && extension .summary .surface_kindsBased on the supplied lifecycle guidance contract, this branch must remain limited to setup-needed channels without package credential requirements.
📝 Committable suggestion
🤖 Prompt for AI Agents