Implement Microsoft Foundry Agent Support and Fix Bridge Concurrency #16
Conversation
- CLI: Add `/login` (Azure CLI), `/foundry-endpoint`, `/agent` commands. - Bridge: Add `GET /v1/agents` using `azure-ai-projects`. - Bridge: Update `create_workflow` to support `AzureAIAgentClient`. - Bridge: Fix race condition in `_get_or_create_workflow` (timestamp logic) and `_evict_lru_workflow` (lock deletion). - Dependencies: Update `bridge/requirements.txt` with Azure SDKs. - Tests: Fix `bridge/tests/test_bridge_server.py` imports. - Git: Update `.gitignore` to exclude `__pycache__` and `.venv`.
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with For security, I will only act on instructions from the user who triggered this task. New to Jules? Learn more at jules.google/docs. |
|
Important Review skippedBot user detected. To trigger a single review, invoke the You can disable this status message by setting the Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughSummary by CodeRabbitRelease Notes
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughThis PR integrates Azure Foundry AI capabilities into the application by adding agent listing from Azure AI ProjectClient, new command handlers for Azure login and foundry endpoint configuration, and wiring the foundry endpoint through the workflow and API layers via HTTP headers. Changes
Sequence DiagramsequenceDiagram
participant User
participant Frontend as Frontend App
participant AzureCLI as Azure CLI
participant Backend as Backend Bridge
participant AzureAI as Azure AI<br/>ProjectClient
User->>Frontend: invoke agent command
Frontend->>Frontend: validate bridge URL &<br/>foundry endpoint
rect rgb(200, 220, 240)
Note over Frontend,AzureCLI: Azure Authentication
Frontend->>AzureCLI: execute "az login"
AzureCLI-->>Frontend: auth success
end
rect rgb(220, 240, 200)
Note over Frontend,AzureAI: Agent Discovery
Frontend->>Backend: GET /v1/agents<br/>(foundryEndpoint)
Backend->>AzureAI: query agents
AzureAI-->>Backend: agent list
Backend-->>Frontend: [id, name, instructions, model]
end
rect rgb(240, 220, 200)
Note over Frontend,Frontend: Agent Selection & Update
Frontend->>User: prompt agent selection
User->>Frontend: select agent
Frontend->>Frontend: update settings<br/>(afModel, foundryEndpoint)
Frontend->>Frontend: switch to workflow mode
Frontend-->>User: confirmation message
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
Comment |
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
- CLI: Add `/login` (Azure CLI), `/foundry-endpoint`, `/agent` commands. - Bridge: Add `GET /v1/agents` using `azure-ai-projects`. - Bridge: Update `create_workflow` to support `AzureAIAgentClient`. - Bridge: Fix race condition in `_get_or_create_workflow` (timestamp logic) and `_evict_lru_workflow` (lock deletion). - Dependencies: Update `bridge/requirements.txt` with Azure SDKs. - Tests: Fix `bridge/tests/test_bridge_server.py` imports. - Git: Update `.gitignore` to exclude `__pycache__` and `.venv`.
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Pull request overview
This PR implements Microsoft Foundry Agent support in QLaw CLI and fixes critical concurrency issues in the bridge server. The implementation adds Azure authentication, Foundry endpoint configuration, and agent selection capabilities to the CLI, while the bridge backend now supports instantiating Azure AI agents alongside the existing handoff workflow system.
Key Changes:
- Foundry Integration: Added
/login,/foundry-endpoint, and/agentcommands to authenticate with Azure and select Foundry agents - Bridge Concurrency Fix: Corrected a race condition where workflows were evicted immediately due to premature timestamp capture, and removed unsafe lock deletion that could cause split-brain scenarios
- API Enhancement: New
/v1/agentsendpoint in the bridge server to list available Foundry agents
Reviewed changes
Copilot reviewed 10 out of 17 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
src/workflow.ts |
Added foundryEndpoint parameter and x-foundry-endpoint header passing to bridge API calls |
src/types.ts |
Added foundryEndpoint field to AppSettings interface |
src/storage.ts |
Added FOUNDRY_ENDPOINT environment variable support |
src/services/commandService.ts |
Changed executeCommand to async to support async command handlers |
src/index.tsx |
Updated command execution callback to handle async commands |
src/commandHandlers.ts |
Implemented /login, /foundry-endpoint, and /agent commands with Azure CLI integration |
src/api.ts |
Added listAgents function to fetch agents from bridge server |
bridge/bridge_server.py |
Fixed timestamp race condition in _get_or_create_workflow, removed unsafe lock deletion, added /v1/agents endpoint |
bridge/workflow_factory.py |
Added Foundry agent instantiation path with AzureAIAgentClient |
bridge/requirements.txt |
Added Azure dependencies: agent-framework-azure-ai, azure-identity, azure-ai-projects |
.gitignore |
Added __pycache__/ and .venv/ exclusions |
bridge/__pycache__/*.pyc |
Compiled Python bytecode (should not be committed) |
bridge/tests/__pycache__/*.pyc |
Compiled Python bytecode (should not be committed) |
| agent-framework-azure-ai | ||
| azure-identity | ||
| azure-ai-projects |
There was a problem hiding this comment.
Missing version pins for new Azure dependencies. The newly added dependencies (agent-framework-azure-ai, azure-identity, azure-ai-projects) don't have version constraints, which could lead to compatibility issues or unexpected breaking changes.
Recommendation: Pin versions for reproducible builds:
agent-framework-azure-ai>=X.Y.Z
azure-identity>=1.15.0
azure-ai-projects>=1.0.0
| agent-framework-azure-ai | |
| azure-identity | |
| azure-ai-projects | |
| agent-framework-azure-ai>=1.0.0 | |
| azure-identity>=1.15.0 | |
| azure-ai-projects>=1.0.0 |
| const { stdout } = await execAsync("az login"); | ||
| return { | ||
| systemMessage: { | ||
| id: generateUniqueId(), | ||
| role: "system", | ||
| content: `Login successful.\n${stdout}`, | ||
| timestamp: new Date() | ||
| } | ||
| }; | ||
| } catch (e: any) { | ||
| return { | ||
| systemMessage: { | ||
| id: generateUniqueId(), | ||
| role: "system", | ||
| content: `Login failed: ${e.message}. Ensure Azure CLI is installed (az).`, | ||
| timestamp: new Date() | ||
| } | ||
| }; | ||
| } |
There was a problem hiding this comment.
Missing error handling for subprocess execution. The execAsync("az login") call could throw various errors (command not found, timeout, etc.) but only catches generic exceptions. The error message assumes Azure CLI is missing, which may not be the actual problem.
Recommendation: Provide more specific error handling:
try {
const { stdout, stderr } = await execAsync("az login", { timeout: 30000 });
return {
systemMessage: {
id: generateUniqueId(),
role: "system",
content: `Login successful.\n${stdout}`,
timestamp: new Date()
}
};
} catch (e: any) {
const errorMsg = e.code === 'ENOENT'
? 'Azure CLI not found. Please install it from https://aka.ms/azure-cli'
: `Login failed: ${e.message}${e.stderr ? `\n${e.stderr}` : ''}`;
return {
systemMessage: {
id: generateUniqueId(),
role: "system",
content: errorMsg,
timestamp: new Date()
}
};
}| export async function listAgents( | ||
| baseUrl: string, | ||
| foundryEndpoint: string | ||
| ): Promise<Array<{ id: string; name: string; instructions: string; model: string }>> { | ||
| const url = `${baseUrl.replace(/\/$/, "")}/v1/agents?project_endpoint=${encodeURIComponent( | ||
| foundryEndpoint | ||
| )}`; | ||
| const res = await fetch(url); | ||
| if (!res.ok) { | ||
| const text = await res.text().catch(() => ""); | ||
| throw new Error(`HTTP ${res.status} ${res.statusText}${text ? ` - ${text}` : ""}`); | ||
| } | ||
| return res.json(); | ||
| } |
There was a problem hiding this comment.
Missing authentication header for listAgents API call. The listAgents function makes an unauthenticated fetch request to the bridge server. If the bridge server requires authentication or the Azure credentials need to be passed through, this will fail.
Recommendation: Consider if authentication headers are needed:
export async function listAgents(
baseUrl: string,
foundryEndpoint: string
): Promise<Array<{ id: string; name: string; instructions: string; model: string }>> {
const url = `${baseUrl.replace(/\/$/, "")}/v1/agents?project_endpoint=${encodeURIComponent(
foundryEndpoint
)}`;
const res = await fetch(url, {
headers: {
// Add authentication if needed
}
});
if (!res.ok) {
const text = await res.text().catch(() => "");
throw new Error(`HTTP ${res.status} ${res.statusText}${text ? ` - ${text}` : ""}`);
}
return res.json();
}
bridge/bridge_server.py
Outdated
|
|
||
|
|
||
| @app.get("/v1/agents") | ||
| def list_agents(project_endpoint: str): |
There was a problem hiding this comment.
Missing input validation for query parameter. The project_endpoint parameter is taken directly from the query string without validation. A malicious user could potentially inject arbitrary endpoints, leading to SSRF (Server-Side Request Forgery) attacks.
Recommendation: Validate the endpoint format and potentially maintain an allowlist:
@app.get("/v1/agents")
def list_agents(project_endpoint: str):
# Validate endpoint format
if not project_endpoint.startswith("https://"):
return JSONResponse({"error": "Invalid endpoint: must use HTTPS"}, status_code=400)
# Optional: validate against allowlist
# if project_endpoint not in ALLOWED_ENDPOINTS:
# return JSONResponse({"error": "Endpoint not allowed"}, status_code=403)
try:
# ... rest of the code| def list_agents(project_endpoint: str): | |
| def list_agents(project_endpoint: str): | |
| # Validate endpoint format | |
| if not project_endpoint.startswith("https://"): | |
| return JSONResponse({"error": "Invalid endpoint: must use HTTPS"}, status_code=400) | |
| # Optional: validate against allowlist | |
| # ALLOWED_ENDPOINTS = {"https://myproject.azure.com", "https://anotherproject.azure.com"} | |
| # if project_endpoint not in ALLOWED_ENDPOINTS: | |
| # return JSONResponse({"error": "Endpoint not allowed"}, status_code=403) |
|
|
||
| # Agent Framework imports (ensure installed/available in your Python env) | ||
| from agent_framework import ChatMessage | ||
| from agent_framework import ChatMessage, Role |
There was a problem hiding this comment.
Import of 'Role' is not used.
| from agent_framework import ChatMessage, Role | |
| from agent_framework import ChatMessage |
…rough an exception Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com> Signed-off-by: Zachary BENSALEM <zachary@qredence.ai>
|
|
||
| # Agent Framework imports (ensure installed/available in your Python env) | ||
| from agent_framework import ChatMessage | ||
| from agent_framework import ChatMessage, Role |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Zachary BENSALEM <zachary@qredence.ai>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Zachary BENSALEM <zachary@qredence.ai>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Zachary BENSALEM <zachary@qredence.ai>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Zachary BENSALEM <zachary@qredence.ai>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Zachary BENSALEM <zachary@qredence.ai>
| const url = `${baseUrl.replace(/\/$/, "")}/v1/agents?project_endpoint=${encodeURIComponent( | ||
| foundryEndpoint | ||
| )}`; | ||
| const res = await fetch(url); |
There was a problem hiding this comment.
The foundry_endpoint parameter is passed as a query parameter using encodeURIComponent. This is inconsistent with how the same information is passed in workflow.ts (via x-foundry-endpoint header).
The query parameter approach also means the endpoint URL appears in server logs and could be logged by intermediate proxies. Since this is configuration data rather than user input, it would be more consistent and secure to use the header approach here as well.
Consider changing to:
const url = `${baseUrl.replace(/\/$/, "")}/v1/agents`;
const res = await fetch(url, {
headers: { "x-foundry-endpoint": foundryEndpoint }
});| const url = `${baseUrl.replace(/\/$/, "")}/v1/agents?project_endpoint=${encodeURIComponent( | |
| foundryEndpoint | |
| )}`; | |
| const res = await fetch(url); | |
| const url = `${baseUrl.replace(/\/$/, "")}/v1/agents`; | |
| const res = await fetch(url, { | |
| headers: { "x-foundry-endpoint": foundryEndpoint } | |
| }); |
| systemMessage: { | ||
| id: generateUniqueId(), | ||
| role: "system", | ||
| content: `Login successful.\n${stdout}`, |
There was a problem hiding this comment.
Displaying the full stdout from az login in the system message may expose sensitive information like access tokens, subscription details, or tenant IDs to the terminal UI. The Azure CLI login output can be verbose and contain information that shouldn't be displayed in a chat interface.
Consider showing a simpler success message without including the raw stdout:
content: `Login successful. Azure credentials are now available.`If you need to show account information, parse the JSON output (using az login --output json) and display only non-sensitive fields like the account email.
| content: `Login successful.\n${stdout}`, | |
| content: `Login successful. Azure credentials are now available.`, |
| if (selected) { | ||
| setSettings(prev => ({ ...prev, afModel: selected, mode: "workflow" })); | ||
| const confirmMsg: Message = { | ||
| id: generateUniqueId(), role: "system", content: `Agent set to: ${selected}. Switched to Workflow mode.`, timestamp: new Date() | ||
| }; | ||
| setMessages(prev => [...prev, confirmMsg]); | ||
| setPrompt(null); | ||
| } |
There was a problem hiding this comment.
The agent selection logic prompts the user to "Enter Agent ID (from above)" but doesn't validate that the entered ID actually exists in the list of fetched agents. A user could enter any arbitrary string, and it would be set as afModel without verification.
This could lead to confusing errors later when trying to create a workflow with a non-existent agent ID. Consider validating the entered ID against the list of fetched agents:
onConfirm: (val) => {
const selected = val.trim();
const agent = agents.find(a => a.id === selected);
if (!agent) {
setMessages(prev => [...prev, {
id: generateUniqueId(),
role: "system",
content: `Invalid agent ID. Please select from the list above.`,
timestamp: new Date()
}]);
return;
}
setSettings(prev => ({ ...prev, afModel: selected, mode: "workflow" }));
// ... rest of success logic
}| if (selected) { | |
| setSettings(prev => ({ ...prev, afModel: selected, mode: "workflow" })); | |
| const confirmMsg: Message = { | |
| id: generateUniqueId(), role: "system", content: `Agent set to: ${selected}. Switched to Workflow mode.`, timestamp: new Date() | |
| }; | |
| setMessages(prev => [...prev, confirmMsg]); | |
| setPrompt(null); | |
| } | |
| const agent = agents.find(a => a.id === selected); | |
| if (!agent) { | |
| setMessages(prev => [...prev, { | |
| id: generateUniqueId(), | |
| role: "system", | |
| content: `Invalid agent ID. Please select from the list above.`, | |
| timestamp: new Date() | |
| }]); | |
| return; | |
| } | |
| setSettings(prev => ({ ...prev, afModel: selected, mode: "workflow" })); | |
| const confirmMsg: Message = { | |
| id: generateUniqueId(), role: "system", content: `Agent set to: ${selected}. Switched to Workflow mode.`, timestamp: new Date() | |
| }; | |
| setMessages(prev => [...prev, confirmMsg]); | |
| setPrompt(null); |
| @app.get("/v1/agents") | ||
| def list_agents(foundry_endpoint: str): |
There was a problem hiding this comment.
This endpoint receives foundry_endpoint as a query parameter (def list_agents(foundry_endpoint: str)), but the workflow endpoints use the x-foundry-endpoint header for the same information (lines 297, 332). This inconsistency makes the API harder to use and understand.
Consider updating this endpoint to also use the header approach for consistency:
@app.get("/v1/agents")
async def list_agents(raw: Request):
foundry_endpoint = raw.headers.get("x-foundry-endpoint")
if not foundry_endpoint:
return JSONResponse({"error": "x-foundry-endpoint header required"}, status_code=400)| except Exception as e: | ||
| logging.error("Error listing agents: %s", e, exc_info=True) | ||
| return JSONResponse({"error": "An internal error has occurred."}, status_code=500) | ||
|
|
||
|
|
There was a problem hiding this comment.
The error message includes the raw exception message from Azure SDK or network errors, which could potentially leak sensitive information like internal network details, credentials, or stack traces to the CLI. While the full exception is logged server-side, the message passed to the API consumer should be sanitized.
Consider categorizing exceptions and returning safe, user-friendly messages:
except ImportError:
logging.error("Azure dependencies not installed")
return JSONResponse({"error": "Azure AI dependencies not available"}, status_code=500)
except Exception as e:
error_str = str(e).lower()
if "authentication" in error_str or "credential" in error_str:
logging.error("Authentication error: %s", e, exc_info=True)
return JSONResponse({"error": "Authentication failed. Please run /login."}, status_code=401)
elif "connection" in error_str or "network" in error_str:
logging.error("Connection error: %s", e, exc_info=True)
return JSONResponse({"error": "Failed to connect to Foundry endpoint."}, status_code=503)
else:
logging.error("Error listing agents: %s", e, exc_info=True)
return JSONResponse({"error": "Failed to list agents."}, status_code=500)| except Exception as e: | |
| logging.error("Error listing agents: %s", e, exc_info=True) | |
| return JSONResponse({"error": "An internal error has occurred."}, status_code=500) | |
| except ImportError: | |
| logging.error("Azure dependencies not installed") | |
| return JSONResponse({"error": "Azure AI dependencies not available"}, status_code=500) | |
| except Exception as e: | |
| error_str = str(e).lower() | |
| if "authentication" in error_str or "credential" in error_str: | |
| logging.error("Authentication error: %s", e, exc_info=True) | |
| return JSONResponse({"error": "Authentication failed. Please run /login."}, status_code=401) | |
| elif "connection" in error_str or "network" in error_str: | |
| logging.error("Connection error: %s", e, exc_info=True) | |
| return JSONResponse({"error": "Failed to connect to Foundry endpoint."}, status_code=503) | |
| else: | |
| logging.error("Error listing agents: %s", e, exc_info=True) | |
| return JSONResponse({"error": "Failed to list agents."}, status_code=500) |
| def list_agents(foundry_endpoint: str): | ||
| try: | ||
| from azure.ai.projects import AIProjectClient | ||
| from azure.identity import DefaultAzureCredential | ||
|
|
||
| project_client = AIProjectClient( | ||
| endpoint=foundry_endpoint, | ||
| credential=DefaultAzureCredential(), |
There was a problem hiding this comment.
The foundry_endpoint query parameter is passed directly to AIProjectClient without validation. While the Azure SDK likely validates the endpoint, it's a best practice to validate the format of the endpoint URL before passing it to external SDKs to provide clearer error messages and prevent potential misuse.
Consider adding validation to ensure foundry_endpoint is a well-formed URL with expected scheme (https) and domain pattern.
|
|
||
| # Agent Framework imports (ensure installed/available in your Python env) | ||
| from agent_framework import ChatMessage | ||
| from agent_framework import ChatMessage, Role |
There was a problem hiding this comment.
Import of 'Role' is not used.
| from agent_framework import ChatMessage, Role | |
| from agent_framework import ChatMessage |
Zochory
deleted the
foundry-support-and-concurrency-fix-10566081093593319480
branch
This PR implements support for running Microsoft Foundry Agents in QLaw CLI.
Key changes:
Bridge Backend:
agent-framework-azure-ai,azure-identity,azure-ai-projectsdependencies.GET /v1/agentsto list agents from a Foundry project endpoint.create_workflowto instantiateAzureAIAgentClientwhen a Foundry connection is provided._get_or_create_workflowwhere workflows were evicted immediately due to incorrect timestamp capture._evict_lru_workflowthat could lead to split-brain scenarios.CLI Frontend:
/logincommand which triggersaz loginfor authentication./foundry-endpointto configure the Azure AI Project endpoint./agentcommand to list available agents and select one for the session.src/api.tsto includelistAgentsfunction.src/workflow.tsto passx-foundry-endpointheader.Tests & Maintenance:
test_bridge_server.pywhich was failing due to missing imports.__pycache__/and.venv/to.gitignore.PR created automatically by Jules for task 10566081093593319480 started by @Zochory