Skip to content

fix(external-sources): clarify MCP and agent reviews #1735

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.

Sign up for GitHub

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
limityan merged 1 commit into from
Jul 24, 2026
Merged

fix(external-sources): clarify MCP and agent reviews #1735

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 41 additions & 0 deletions src/crates/assembly/core/src/external_subagents.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,7 @@ struct ResolvedExternalCandidate {
model_label: String,
model_configuration_fingerprint: String,
tools: Vec<ResolvedToolFact>,
unavailable_tool_labels: Vec<String>,
readonly: bool,
activation_envelope: String,
approval_key: String,
Expand Down Expand Up @@ -675,6 +676,7 @@ fn resolve_external_candidate(
};

let mut tools = Vec::new();
let mut unavailable_tool_labels = Vec::new();
for selector in definition
.requested_tools
.selectors
Expand All @@ -689,6 +691,7 @@ fn resolve_external_candidate(
Some(tool) => tools.push(tool.clone()),
None => {
compatibility = ExternalSubagentCompatibilityState::Blocked;
unavailable_tool_labels.push(name.to_string());
diagnostics.push(ExternalSubagentDiagnosticSummary {
code: "external_subagent.tool_unavailable".to_string(),
blocks_activation: true,
Expand All @@ -698,6 +701,8 @@ fn resolve_external_candidate(
}
tools.sort_by(|left, right| left.name.cmp(&right.name));
tools.dedup_by(|left, right| left.name == right.name);
unavailable_tool_labels.sort();
unavailable_tool_labels.dedup();
diagnostics.sort_by(|left, right| left.code.cmp(&right.code));
diagnostics.dedup_by(|left, right| left.code == right.code);
let readonly = tools.iter().all(|tool| tool.readonly);
Expand Down Expand Up @@ -779,6 +784,7 @@ fn resolve_external_candidate(
model_label: model.display_label,
model_configuration_fingerprint: model.configuration_fingerprint,
tools,
unavailable_tool_labels,
readonly,
activation_envelope,
approval_key,
Expand Down Expand Up @@ -1002,6 +1008,7 @@ fn summary_for(
.iter()
.map(|tool| tool.name.clone())
.collect(),
unavailable_tool_labels: candidate.unavailable_tool_labels.clone(),
supports_follow_up: false,
compatibility_state: candidate.compatibility,
diagnostics: candidate.diagnostics.clone(),
Expand Down Expand Up @@ -1419,6 +1426,40 @@ mod tests {
assert_eq!(recovered.registrations.len(), 1);
}

#[test]
fn unavailable_tool_labels_are_preserved_for_product_diagnostics() {
let empty_set = BTreeSet::new();
let empty_map = BTreeMap::new();
let mut definition_snapshot = snapshot("behavior-v1", "catalog-v1");
definition_snapshot.definitions[0]
.requested_tools
.selectors
.push(ExternalSubagentToolSelector {
source_name: "shell".to_string(),
canonical_host_name: Some("Shell".to_string()),
allowed: true,
});

let state = reconcile_with_facts(
Some(Path::new("C:/repo")),
"local-user",
&definition_snapshot,
ExternalSubagentDecisions {
active_ecosystems: test_active_ecosystems(),
approved_envelopes: &empty_set,
declined_decisions: &empty_map,
conflict_choices: &empty_map,
conflict_lineage_current_keys: &empty_map,
},
&facts(),
);

assert_eq!(state.summaries[0].unavailable_tool_labels, ["Shell"]);
assert!(state.summaries[0].diagnostics.iter().any(|diagnostic| {
diagnostic.code == "external_subagent.tool_unavailable" && diagnostic.blocks_activation
}));
}

#[test]
fn model_config_outage_logging_is_deduplicated_and_does_not_expose_error_values() {
let logged = AtomicBool::new(false);
Expand Down
3 changes: 3 additions & 0 deletions src/crates/contracts/product-domains/src/external_sources.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1912,6 +1912,9 @@ impl ExternalSourcePublicSnapshot {
tool.activation = ExternalToolActivationState::Disabled;
}
}
for subagent in &mut self.subagents {
subagent.unavailable_tool_labels.clear();
}
self
}
}
Expand Down
2 changes: 2 additions & 0 deletions src/crates/contracts/product-domains/src/external_subagents.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -516,6 +516,8 @@ pub struct ExternalSubagentSummary {
#[serde(default, skip_serializing_if = "Option::is_none")]
pub effective_model_label: Option<String>,
pub effective_tool_labels: Vec<String>,
#[serde(default, skip_serializing_if = "Vec::is_empty")]
pub unavailable_tool_labels: Vec<String>,
pub supports_follow_up: bool,
pub compatibility_state: ExternalSubagentCompatibilityState,
pub diagnostics: Vec<ExternalSubagentDiagnosticSummary>,
Expand Down
24 changes: 24 additions & 0 deletions src/crates/contracts/product-domains/tests/external_source_contracts.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -584,13 +584,37 @@ fn legacy_public_snapshot_downprojects_new_tool_review_variants() {
"approvalKey": "approval-v1",
"decisionKey": "decision-v1",
"activation": { "state": "declined" }
}],
"subagents": [{
"candidateId": "external-review",
"logicalId": "review",
"displayName": "External Review",
"description": "Review changes",
"providerLabel": "OpenCode",
"scope": "project",
"sourceKeys": [],
"sourceLocationLabels": [],
"sourceCount": 1,
"effectiveToolLabels": ["Read"],
"unavailableToolLabels": ["Shell"],
"supportsFollowUp": false,
"compatibilityState": "blocked",
"diagnostics": [{
"code": "external_subagent.tool_unavailable",
"blocksActivation": true
}],
"activationState": { "state": "blocked" },
"decisionKey": "agent-decision-v1"
}]
}))
.expect("new public snapshot");

let legacy =
serde_json::to_value(snapshot.into_legacy_v0_compatible()).expect("legacy public snapshot");
assert_eq!(legacy["tools"][0]["activation"]["state"], "disabled");
assert!(legacy["subagents"][0]
.get("unavailableToolLabels")
.is_none());
}

#[test]
Expand Down
2 changes: 2 additions & 0 deletions src/web-ui/src/infrastructure/api/service-api/ExternalSourcesAPI.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -211,6 +211,7 @@ export interface ExternalSubagentSummary {
sourceCount: number;
effectiveModelLabel?: string;
effectiveToolLabels: string[];
unavailableToolLabels: string[];
supportsFollowUp: boolean;
compatibilityState: 'ready' | 'ready_with_degradation' | 'blocked' | 'invalid';
diagnostics: Array<{ code: string; blocksActivation: boolean }>;
Expand Down Expand Up @@ -775,6 +776,7 @@ function normalizeSnapshot(value: unknown): ExternalSourceCatalogSnapshot {
sourceKeys: normalizeOptionalArray(subagent.sourceKeys),
sourceLocationLabels: normalizeOptionalArray(subagent.sourceLocationLabels),
effectiveToolLabels: normalizeOptionalArray(subagent.effectiveToolLabels),
unavailableToolLabels: normalizeOptionalArray(subagent.unavailableToolLabels),
diagnostics: normalizeOptionalArray(subagent.diagnostics),
})),
subagentConflicts: normalizeOptionalArray<ExternalSubagentConflict>(candidate.subagentConflicts).map((conflict) => ({
Expand Down
35 changes: 35 additions & 0 deletions src/web-ui/src/infrastructure/config/components/ExternalSourcesConfig.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -494,6 +494,41 @@
font-size: 11px;
}

&__review-summary {
display: flex;
flex-wrap: wrap;
gap: 4px 12px;
color: var(--color-text-secondary);
font-size: 12px;
}

&__review-risk {
margin-top: 6px;
}

&__review-details {
margin-top: 8px;
color: var(--color-text-secondary);
font-size: 12px;

> summary {
width: fit-content;
color: var(--color-accent-500);
cursor: pointer;
user-select: none;
}

&[open] > summary {
margin-bottom: 8px;
}
}

&__diagnostic-code {
color: var(--color-text-muted);
font-size: 11px;
overflow-wrap: anywhere;
}

&__tool-actions {
display: flex;
justify-content: flex-end;
Expand Down
86 changes: 84 additions & 2 deletions src/web-ui/src/infrastructure/config/components/ExternalSourcesConfig.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -726,6 +726,27 @@ describe('ExternalSourcesConfig', () => {
behaviorVersion: 'behavior-v1',
staticStatus: { state: 'ready' },
},
}, {
candidateId: 'external-mcp-docs',
approvalKey: 'mcp-approval-v2',
decisionKey: 'mcp-decision-v2',
definition: {
id: {
source: { providerId: 'opencode.mcp', sourceId: 'project' },
localId: 'docs',
},
provenance: [{ providerId: 'opencode.mcp', sourceId: 'project' }],
name: 'docs',
transport: 'streamable_http',
remoteUrlPreview: 'https://mcp.example.test',
argumentCount: 0,
environmentKeys: [],
environmentReferenceNames: [],
headerNames: [],
sourceEnabled: true,
behaviorVersion: 'behavior-v2',
staticStatus: { state: 'ready' },
},
}],
mcpConflicts: [{
conflictKey: 'mcp-conflict-v1',
Expand Down Expand Up @@ -771,6 +792,45 @@ describe('ExternalSourcesConfig', () => {
expect(container.textContent).toContain('GITHUB_TOKEN');
expect(container.textContent).toContain('OPENCODE_TOKEN');

const approvalDetails = container.querySelector(
'.bitfun-external-sources-config__review-details',
) as HTMLDetailsElement;
const approvalCard = approvalDetails.closest(
'.bitfun-external-sources-config__tool-card',
) as HTMLElement;
const alwaysVisibleSummary = approvalCard.querySelector(
'.bitfun-external-sources-config__review-summary',
) as HTMLElement;
const alwaysVisibleRisk = approvalCard.querySelector(
'.bitfun-external-sources-config__review-risk',
) as HTMLElement;
expect(approvalDetails.open).toBe(false);
expect(alwaysVisibleSummary.textContent).toContain('mcp.command:{"command":"npx"}');
expect(alwaysVisibleRisk.textContent).toContain('mcpApprovals.compactWarning');
expect(approvalDetails.contains(alwaysVisibleSummary)).toBe(false);
expect(approvalDetails.contains(alwaysVisibleRisk)).toBe(false);
const approvalEnable = Array.from(approvalCard.querySelectorAll('button')).find((button) =>
button.textContent?.includes('mcpApprovals.enable')) as HTMLButtonElement;
expect(alwaysVisibleRisk.id).toBe('mcp-review-risk-mcp-decision-v1');
expect(approvalEnable.getAttribute('aria-describedby')).toBe(alwaysVisibleRisk.id);
const remoteSummary = Array.from(approvalCard.parentElement?.querySelectorAll(
'.bitfun-external-sources-config__review-summary',
) ?? []).find((candidate) => candidate.textContent?.includes('mcp.url')) as HTMLElement;
const remoteDetails = remoteSummary.closest(
'.bitfun-external-sources-config__tool-card',
)?.querySelector('.bitfun-external-sources-config__review-details') as HTMLDetailsElement;
expect(remoteSummary.textContent).toContain(
'mcp.url:{"url":"https://mcp.example.test"}',
);
expect(remoteDetails.textContent).not.toContain(
'mcp.url:{"url":"https://mcp.example.test"}',
);
expect(approvalDetails.querySelector('summary')?.textContent)
.toContain('mcpApprovals.showDetails');
expect(container.textContent).toContain('mcpApprovals.enable');
await act(async () => approvalDetails.querySelector('summary')?.click());
expect(approvalDetails.open).toBe(true);

const externalConflictCandidate = Array.from(
container.querySelectorAll('.bitfun-external-sources-config__candidate'),
).find((candidate) => candidate.textContent?.includes('OpenCode: github'));
Expand Down Expand Up @@ -1256,14 +1316,24 @@ describe('ExternalSourcesConfig', () => {
sourceCount: 1,
effectiveModelLabel: 'fast',
effectiveToolLabels: ['Read', 'Grep'],
unavailableToolLabels: ['Shell', 'Write'],
supportsFollowUp: false,
compatibilityState: 'ready',
diagnostics: [{
code: 'opencode_agent_prompt_not_imported',
blocksActivation: true,
}, {
code: 'external_subagent.tool_unavailable',
blocksActivation: true,
}, {
code: 'opencode_agent_permission_not_imported',
blocksActivation: true,
}, {
code: 'opencode_default_permission_semantics_not_imported',
blocksActivation: false,
}, {
code: 'opencode_agent_temperature_not_imported',
blocksActivation: false,
}, {
code: 'opencode_agent_definition_type_invalid',
blocksActivation: true,
Expand Down Expand Up @@ -1340,9 +1410,21 @@ describe('ExternalSourcesConfig', () => {
expect(container.textContent).toContain('fast');
expect(container.textContent).toContain('Read, Grep');
expect(container.textContent).toContain('agents.executionDomain');
expect(container.textContent).toContain('agentDiagnostics.unsupportedBehavior.reason');
expect(container.textContent).toContain('agentDiagnostics.ignoredOption.reason');
expect(container.textContent).toContain(
'agentDiagnostics.toolUnavailable.reason:{"tools":"Shell, Write"}',
);
expect(container.textContent).toContain('agentDiagnostics.promptMissing.reason');
expect(container.textContent).toContain(
'agentDiagnostics.unsupportedSetting.reason:{"setting":"agentDiagnostics.settings.permissions"}',
);
expect(container.textContent).toContain(
'agentDiagnostics.ignoredSetting.reason:{"setting":"agentDiagnostics.settings.defaultPermissions"}',
);
expect(container.textContent).toContain(
'agentDiagnostics.ignoredSetting.reason:{"setting":"agentDiagnostics.settings.temperature"}',
);
expect(container.textContent).toContain('agentDiagnostics.invalidDefinition.reason');
expect(container.textContent).toContain('opencode_agent_definition_type_invalid');
expect(container.textContent).toContain('agentConflicts.selectionApproves');
expect(container.textContent).toContain('.opencode/agents/explore.md');
expect(container.textContent).not.toContain('D:/workspace/project/.opencode/agents');
Expand Down
Loading