diff --git a/application/single_app/config.py b/application/single_app/config.py index 02c59d2f..6f51aaa9 100644 --- a/application/single_app/config.py +++ b/application/single_app/config.py @@ -96,7 +96,7 @@ EXECUTOR_TYPE = 'thread' EXECUTOR_MAX_WORKERS = 30 SESSION_TYPE = 'filesystem' -VERSION = "0.260.022" +VERSION = "0.260.023" IS_DEVELOPMENT = is_development_env_enabled() SESSION_COOKIE_SAMESITE = os.getenv('SESSION_COOKIE_SAMESITE', 'Lax') diff --git a/application/single_app/functions_authentication.py b/application/single_app/functions_authentication.py index 2e76f7f0..1062a86b 100644 --- a/application/single_app/functions_authentication.py +++ b/application/single_app/functions_authentication.py @@ -3,6 +3,8 @@ import base64 import json +from flask import has_request_context + from config import * from functions_appinsights import log_event from functions_settings import * @@ -1065,6 +1067,23 @@ def get_current_user_id(): return user.get('oid') return None +def get_current_user_id_or_none(): + """Return the current user id, or None when no request-scoped identity exists. + + Startup and background callers run outside a Flask request context, where reading the + session proxy raises RuntimeError. Use this helper only where the identity is optional + context, such as scoping optional plugin or endpoint lookups. Authorization decisions must + keep calling get_current_user_id() so a missing request context fails loudly instead of + silently resolving to an unauthenticated identity. + """ + if not has_request_context(): + return None + + try: + return get_current_user_id() + except Exception: + return None + def get_current_user_info(): user = session.get("user") if not user: diff --git a/application/single_app/semantic_kernel_loader.py b/application/single_app/semantic_kernel_loader.py index 6e8e8c47..ec7ba2b6 100644 --- a/application/single_app/semantic_kernel_loader.py +++ b/application/single_app/semantic_kernel_loader.py @@ -41,7 +41,7 @@ resolve_openai_style_request_api_version, ) from functions_appinsights import log_event, get_appinsights_logger -from functions_authentication import get_current_user_id +from functions_authentication import get_current_user_id_or_none from semantic_kernel_plugins.plugin_health_checker import PluginHealthChecker, PluginErrorRecovery from semantic_kernel_plugins.logged_plugin_loader import create_logged_plugin_loader from semantic_kernel_plugins.plugin_invocation_logger import get_plugin_logger @@ -357,8 +357,18 @@ def get_group_scope_id(): if persisted_group_id: return persisted_group_id + scope_user_id = get_current_user_id_or_none() + if not scope_user_id: + debug_print("[SK_LOADER] No request-scoped user available while resolving group endpoint scope.") + log_event( + "[SK_LOADER] Group endpoint resolution could not determine a group scope.", + level=logging.WARNING, + extra={"agent_name": agent.get("name")} + ) + return "" + try: - return require_active_group(get_current_user_id()) + return require_active_group(scope_user_id) except ValueError as err: debug_print(f"[SK_LOADER] No active group available while resolving group endpoint scope: {err}") log_event( @@ -529,11 +539,13 @@ def get_agent_model_endpoint_candidates(): endpoints.extend([{**endpoint, "_endpoint_scope": "group"} for endpoint in get_group_model_endpoints(group_id)]) elif not is_global_agent: if allow_custom_agent_endpoints: - user_settings = get_user_settings(get_current_user_id()) - endpoints.extend([ - {**endpoint, "_endpoint_scope": "user"} - for endpoint in user_settings.get("settings", {}).get("personal_model_endpoints", []) - ]) + endpoint_user_id = get_current_user_id_or_none() + if endpoint_user_id: + user_settings = get_user_settings(endpoint_user_id) + endpoints.extend([ + {**endpoint, "_endpoint_scope": "user"} + for endpoint in user_settings.get("settings", {}).get("personal_model_endpoints", []) + ]) endpoints.extend([{**endpoint, "_endpoint_scope": "global"} for endpoint in (settings.get("model_endpoints", []) or [])]) return endpoints @@ -647,11 +659,13 @@ def resolve_foundry_endpoint_config(): endpoints.extend([{**endpoint, "_endpoint_scope": "group"} for endpoint in get_group_model_endpoints(group_id)]) elif not is_global_agent: if allow_custom_agent_endpoints: - user_settings = get_user_settings(get_current_user_id()) - endpoints.extend([ - {**endpoint, "_endpoint_scope": "user"} - for endpoint in user_settings.get("settings", {}).get("personal_model_endpoints", []) - ]) + endpoint_user_id = get_current_user_id_or_none() + if endpoint_user_id: + user_settings = get_user_settings(endpoint_user_id) + endpoints.extend([ + {**endpoint, "_endpoint_scope": "user"} + for endpoint in user_settings.get("settings", {}).get("personal_model_endpoints", []) + ]) endpoints.extend([{**endpoint, "_endpoint_scope": "global"} for endpoint in (settings.get("model_endpoints", []) or [])]) endpoint_cfg = next((e for e in endpoints if e.get("id") == endpoint_id), None) @@ -1924,7 +1938,7 @@ def create_chat_completion_service(): else: plugin_mode = mode_label - resolved_user_id = get_current_user_id() + resolved_user_id = get_current_user_id_or_none() group_id = agent_config.get("group_id") if agent_is_group else None print(f"[SK_LOADER] Agent scope - is_global: {agent_is_global}, is_group: {agent_is_group}, plugin_mode: {plugin_mode}, group_id: {group_id}") load_agent_specific_plugins( @@ -2289,11 +2303,7 @@ def load_plugins_for_kernel(kernel, plugin_manifests, settings, mode_label="glob # Use the logged plugin loader for custom plugins try: - user_id = None - try: - user_id = get_current_user_id() - except Exception: - pass # User ID is optional for plugin loading + user_id = get_current_user_id_or_none() # Load plugins with enhanced logging results = logged_loader.load_multiple_plugins(plugin_manifests, user_id) diff --git a/docs/explanation/fixes/SEMANTIC_KERNEL_STARTUP_REQUEST_CONTEXT_FIX.md b/docs/explanation/fixes/SEMANTIC_KERNEL_STARTUP_REQUEST_CONTEXT_FIX.md new file mode 100644 index 00000000..9e38d780 --- /dev/null +++ b/docs/explanation/fixes/SEMANTIC_KERNEL_STARTUP_REQUEST_CONTEXT_FIX.md @@ -0,0 +1,118 @@ +# Semantic Kernel Startup Request Context Fix (v0.260.023) + +Fixed/Implemented in version: **0.260.023** + +Refs: [#1327](https://github.com/microsoft/simplechat/issues/1327) + +## Issue Summary + +Starting SimpleChat directly with `python application/single_app/app.py` (including via `uv run`) +aborted at startup with: + +``` +RuntimeError: Working outside of request context. +``` + +The failure only appeared once at least one action had been assigned to an agent and saved. Before any +action was assigned, the same configuration started normally, which made the problem look intermittent. + +Container and App Service deployments were unaffected, because they use the gunicorn entrypoint +(`ENTRYPOINT ["python3", "-m", "gunicorn", "-c", "/app/gunicorn.conf.py", "app:app"]`). The +`if __name__ == '__main__':` block never runs there, so initialization happens through the +`@app.before_request` hook, where a request context exists. + +## Root Cause + +`initialize_application(force=True)` runs at module scope in the direct-run path, outside any Flask +request context. With Semantic Kernel enabled and `per_user_semantic_kernel` disabled, that reaches +global agent loading: + +`initialize_semantic_kernel()` -> `load_semantic_kernel()` -> `load_single_agent_for_kernel()` + +Inside `load_single_agent_for_kernel`, the `if agent_config.get("actions_to_load"):` branch called +`get_current_user_id()` without a guard. That function reads the Flask `session` proxy, which raises +`RuntimeError` when there is no request context, so startup aborted. + +The `actions_to_load` branch is what introduced the failing call, which is why the crash only began +after an action was attached to an agent and persisted. + +The equivalent lookup in `load_plugins_for_kernel` was already wrapped in `try/except` with a `None` +fallback. That inconsistency is why global plugin loading succeeded earlier in the same startup +sequence while agent-specific plugin loading failed. + +Three further identity lookups in the same module had the same latent defect. They are not reached in a +global-agent-only configuration, but they would fail the same way in per-user and group scopes. + +## Files Modified + +- `application/single_app/functions_authentication.py` +- `application/single_app/semantic_kernel_loader.py` +- `application/single_app/config.py` +- `functional_tests/test_semantic_kernel_startup_without_request_context.py` (new) + +## Code Changes Summary + +1. Added `get_current_user_id_or_none()` to `functions_authentication.py`. It returns `None` when + `has_request_context()` is false and otherwise delegates to `get_current_user_id()`. `flask` is now + imported explicitly for `has_request_context` rather than relying on the `config` star-import. + +2. Left `get_current_user_id()` unchanged. Authorization callers must keep failing loudly when there is + no request context, so the fallback is deliberately opt-in at each call site rather than applied + globally. `has_request_context()` is the established pattern for this across the codebase. + +3. Routed all five identity lookups in `semantic_kernel_loader.py` through the new helper: + + | Location | Change | + |---|---| + | `load_single_agent_for_kernel` agent plugin loading | The reported crash. Now resolves through the safe helper. | + | `resolve_agent_config.get_group_scope_id` | Resolves the identity first and returns an empty scope with the existing warning when it is absent. | + | `resolve_agent_config.get_agent_model_endpoint_candidates` | Skips personal endpoint collection when no identity is available. | + | `resolve_agent_config.resolve_foundry_endpoint_config` | Skips personal endpoint collection when no identity is available. | + | `load_plugins_for_kernel` | Replaced the ad-hoc `try/except` with the shared helper. | + +4. Short-circuited the group and personal endpoint lookups instead of passing an unresolved identity + downstream. `require_active_group()` and `get_user_settings()` perform Cosmos reads keyed on the user + id, so forwarding `None` would only have traded the `RuntimeError` for a Cosmos lookup error. + +## Behavior + +Startup now completes in the direct-run path. The kernel and agent plugins load with no resolved user +identity, the same way global plugin loading already did. + +Hosted deployments are unaffected. The fallback only applies when there is no request context at all; +inside a request the identity resolves exactly as before, including returning `None` for an +unauthenticated request. + +## Validation + +`functional_tests/test_semantic_kernel_startup_without_request_context.py` covers both halves of the fix: + +- **Behavioral.** `get_current_user_id()` still raises `RuntimeError` outside a request context, so the + fail-loud property authorization code depends on is preserved. `get_current_user_id_or_none()` returns + `None` outside a request context, resolves the session `oid` inside an authenticated request, and + returns `None` for an unauthenticated request. +- **Structural.** An AST pass over `semantic_kernel_loader.py` asserts the module imports the safe helper, + makes no direct `get_current_user_id()` call, and never passes an identity call straight into + `require_active_group()` or `get_user_settings()`. + +Both structural rules were verified to fail when the original defect is reintroduced, so the test is a +genuine regression guard rather than a restatement of the current source: + +- Restoring the unguarded call reports `must not call get_current_user_id() directly; found at line(s) [1941]`. +- Passing the identity straight into `require_active_group()` reports the leaked-argument violation. + +Result with the fix applied: `3/3 tests passed`. + +The behavioral half stubs `config`, `functions_settings`, `functions_appinsights`, and `functions_debug` +in `sys.modules`, because importing the real `config` builds live Azure Cosmos clients at import time. +The stub re-exports the same Flask names `config.py` re-exports, since `functions_authentication` reaches +`session` through `from config import *`. + +## Impact + +- Local development works for configurations that assign actions to agents. Previously the only + workarounds were removing every agent action or running gunicorn locally. +- The three latent call sites are fixed alongside the reported one, so per-user and group scopes cannot + fail the same way from a non-request caller. +- Plugin and endpoint loading no longer depends on a request-scoped identity being present when the + identity is only optional context. diff --git a/docs/explanation/fixes/index.md b/docs/explanation/fixes/index.md index 96011aa6..dac69060 100644 --- a/docs/explanation/fixes/index.md +++ b/docs/explanation/fixes/index.md @@ -7,6 +7,7 @@ category: Version History --- - [Chat Document Search File Name and Divider Artifact Fix](CHAT_DOCUMENT_SEARCH_FILENAME_AND_DIVIDER_FIX.md) +- [Semantic Kernel Startup Request Context Fix](SEMANTIC_KERNEL_STARTUP_REQUEST_CONTEXT_FIX.md) - [Data Management Restore Route Endpoint Collision Fix](DATA_MANAGEMENT_RESTORE_ROUTE_ENDPOINT_COLLISION_FIX.md) - [Font Size and 200 Percent Zoom Fix](FONT_SIZE_AND_200_PERCENT_ZOOM_FIX.md) - [Public Workspace Prompt Migration Fix](PUBLIC_WORKSPACE_PROMPT_MIGRATION_FIX.md) diff --git a/docs/explanation/release_notes.md b/docs/explanation/release_notes.md index 5bd1ffa5..e3cc0430 100644 --- a/docs/explanation/release_notes.md +++ b/docs/explanation/release_notes.md @@ -2,6 +2,17 @@ For feature-focused and fix-focused drill-downs by version, see [Features by Version](https://github.com/microsoft/simplechat/tree/main/docs/explanation/features) and [Fixes by Version](https://github.com/microsoft/simplechat/tree/main/docs/explanation/fixes). +### **(v0.260.023)** + +#### Bug Fixes + +* **Running Simple Chat Directly No Longer Fails To Start When An Agent Has Actions** + * Starting Simple Chat with `python app.py` (including via `uv run`) aborted with `RuntimeError: Working outside of request context` whenever any agent had an action assigned. The app started normally until the first action was saved, which made the failure look intermittent. + * Semantic Kernel initialization runs before any request exists on that path, but agent plugin loading read the signed-in user from the Flask session. It now resolves the user only when a request is actually in progress and otherwise loads with no user identity, matching how global plugin loading already behaved. + * Container and App Service deployments were never affected, because they start through gunicorn and initialize during the first request. Their behavior is unchanged. + * Three further identity lookups used for group scope and personal model endpoints had the same latent problem and were corrected at the same time. + * (Ref: `semantic_kernel_loader.py`, `functions_authentication.py`, `get_current_user_id_or_none`, issue #1327) + ### **(v0.260.021)** #### Bug Fixes diff --git a/functional_tests/test_group_agent_endpoint_scope_resolution.py b/functional_tests/test_group_agent_endpoint_scope_resolution.py index e5bb4aef..3f1da1e6 100644 --- a/functional_tests/test_group_agent_endpoint_scope_resolution.py +++ b/functional_tests/test_group_agent_endpoint_scope_resolution.py @@ -56,7 +56,7 @@ def test_group_agent_endpoint_scope_resolution() -> None: explicit_index = loader_content.index("if explicit_group_scope_id:") persisted_index = loader_content.index("persisted_group_id = str(agent.get(\"group_id\") or \"\").strip()") - active_group_index = loader_content.index("return require_active_group(get_current_user_id())") + active_group_index = loader_content.index("return require_active_group(scope_user_id)") assert explicit_index < persisted_index < active_group_index, ( "Group scope precedence should be explicit scope, then persisted group_id, then active group fallback." ) diff --git a/functional_tests/test_semantic_kernel_startup_without_request_context.py b/functional_tests/test_semantic_kernel_startup_without_request_context.py new file mode 100644 index 00000000..a270641c --- /dev/null +++ b/functional_tests/test_semantic_kernel_startup_without_request_context.py @@ -0,0 +1,264 @@ +#!/usr/bin/env python3 +# test_semantic_kernel_startup_without_request_context.py +""" +Functional test for the Semantic Kernel startup request-context guard. +Version: 0.260.023 +Implemented in: 0.260.023 + +Semantic Kernel initialization runs outside a Flask request context when SimpleChat is started +directly (python app.py). Loading an agent that has actions assigned previously called +get_current_user_id() unguarded, which reads the Flask session proxy and raised +"RuntimeError: Working outside of request context", aborting startup. + +This test ensures that: + 1. get_current_user_id() still raises outside a request context, so authorization callers keep + failing loudly rather than silently resolving to an unauthenticated identity. + 2. get_current_user_id_or_none() returns None outside a request context. + 3. get_current_user_id_or_none() still resolves the session identity inside a request context, so + hosted (gunicorn) behavior is unchanged. + 4. semantic_kernel_loader.py routes every identity lookup through the safe helper, and never hands + a possibly-missing identity straight to require_active_group() or get_user_settings(). + +Refs: issue #1327. +""" + +import ast +import os +import sys +import traceback +import types +from pathlib import Path + + +REPO_ROOT = Path(__file__).resolve().parents[1] +APP_DIR = REPO_ROOT / "application" / "single_app" +LOADER_FILE = APP_DIR / "semantic_kernel_loader.py" + +sys.path.insert(0, str(REPO_ROOT / "functional_tests")) +sys.path.insert(0, str(APP_DIR)) + +from test_support.versioning import assert_app_version_at_least + +IMPLEMENTED_IN_VERSION = "0.260.023" + +# Identity-optional helper that call sites are expected to use. +SAFE_IDENTITY_HELPER = "get_current_user_id_or_none" +# Fail-loud helper that must not be called directly by the loader. +STRICT_IDENTITY_HELPER = "get_current_user_id" +# Callers that perform a Cosmos read keyed on the identity, so None must never reach them. +IDENTITY_CONSUMING_CALLS = ("require_active_group", "get_user_settings") + + +def install_authentication_stubs(): + """Install the minimal module stubs needed to import functions_authentication. + + config.py builds live Azure Cosmos clients at import time, so the real module cannot be + imported without deployed infrastructure. The stub re-exports the same Flask names config.py + re-exports, because functions_authentication reaches `session` through `from config import *`. + """ + import flask + from flask import Flask + + config_stub = types.ModuleType("config") + config_stub._is_test_stub = True + # Mirror the Flask names config.py re-exports (see config.py "from flask import (...)"). + for flask_name in ( + "Flask", + "flash", + "request", + "jsonify", + "render_template", + "redirect", + "url_for", + "session", + "send_from_directory", + "send_file", + "current_app", + ): + setattr(config_stub, flask_name, getattr(flask, flask_name)) + config_stub.app = Flask(__name__) + config_stub.app.secret_key = "functional-test-secret" + sys.modules["config"] = config_stub + + appinsights_stub = types.ModuleType("functions_appinsights") + appinsights_stub.log_event = lambda *args, **kwargs: None + sys.modules["functions_appinsights"] = appinsights_stub + + settings_stub = types.ModuleType("functions_settings") + settings_stub.get_settings = lambda *args, **kwargs: {} + settings_stub.get_user_settings = lambda *args, **kwargs: {"settings": {}} + settings_stub.update_user_settings = lambda *args, **kwargs: None + sys.modules["functions_settings"] = settings_stub + + debug_stub = types.ModuleType("functions_debug") + debug_stub.debug_print = lambda *args, **kwargs: None + sys.modules["functions_debug"] = debug_stub + + return config_stub.app + + +def test_identity_helpers_outside_and_inside_request_context(): + """The safe helper degrades to None outside a request context; the strict helper still raises.""" + print("๐Ÿ” Testing identity helper behavior with and without a request context...") + + try: + flask_app = install_authentication_stubs() + + import functions_authentication + + assert hasattr(functions_authentication, SAFE_IDENTITY_HELPER), ( + f"functions_authentication must expose {SAFE_IDENTITY_HELPER}()" + ) + safe_helper = getattr(functions_authentication, SAFE_IDENTITY_HELPER) + strict_helper = getattr(functions_authentication, STRICT_IDENTITY_HELPER) + + # 1. The strict helper must keep failing loudly outside a request context. + try: + strict_helper() + except RuntimeError: + pass + else: + raise AssertionError( + f"{STRICT_IDENTITY_HELPER}() must still raise RuntimeError outside a request " + "context so authorization callers do not silently degrade to no identity." + ) + + # 2. The safe helper must degrade to None instead of raising. This is the startup fix. + assert safe_helper() is None, ( + f"{SAFE_IDENTITY_HELPER}() must return None outside a request context." + ) + + # 3. Hosted behavior must be unchanged: an authenticated request still resolves the oid. + from flask import session + + with flask_app.test_request_context("/"): + session["user"] = {"oid": "user-oid-1327"} + assert safe_helper() == "user-oid-1327", ( + f"{SAFE_IDENTITY_HELPER}() must resolve the session identity inside a request context." + ) + + # 4. An unauthenticated request still resolves to None without raising. + with flask_app.test_request_context("/favicon.ico"): + assert safe_helper() is None, ( + f"{SAFE_IDENTITY_HELPER}() must return None for an unauthenticated request." + ) + + print("โœ… Identity helper behavior verified.") + return True + + except Exception as e: + print(f"โŒ Test failed: {e}") + traceback.print_exc() + return False + + +def _call_name(node): + """Return the dotted callee name for an ast.Call node, or '' when it is not a plain name.""" + func = node.func + if isinstance(func, ast.Name): + return func.id + if isinstance(func, ast.Attribute): + return func.attr + return "" + + +def test_loader_uses_only_the_safe_identity_helper(): + """The Semantic Kernel loader must never call the fail-loud identity helper directly.""" + print("๐Ÿ” Testing that semantic_kernel_loader.py uses only the request-context-safe helper...") + + try: + loader_source = LOADER_FILE.read_text(encoding="utf-8") + loader_tree = ast.parse(loader_source) + + strict_calls = [] + safe_calls = [] + leaked_identity_arguments = [] + + for node in ast.walk(loader_tree): + if not isinstance(node, ast.Call): + continue + + callee = _call_name(node) + if callee == STRICT_IDENTITY_HELPER: + strict_calls.append(node.lineno) + elif callee == SAFE_IDENTITY_HELPER: + safe_calls.append(node.lineno) + + # Passing an unresolved identity straight through would only trade the RuntimeError + # for a Cosmos lookup keyed on None, so each call site must short-circuit first. + if callee in IDENTITY_CONSUMING_CALLS: + for argument in node.args: + if isinstance(argument, ast.Call) and _call_name(argument) in ( + STRICT_IDENTITY_HELPER, + SAFE_IDENTITY_HELPER, + ): + leaked_identity_arguments.append((node.lineno, callee)) + + assert not strict_calls, ( + f"semantic_kernel_loader.py must not call {STRICT_IDENTITY_HELPER}() directly; found at " + f"line(s) {strict_calls}. Startup runs outside a request context, so use " + f"{SAFE_IDENTITY_HELPER}() instead." + ) + + assert safe_calls, ( + f"semantic_kernel_loader.py should resolve identity through {SAFE_IDENTITY_HELPER}()." + ) + + assert not leaked_identity_arguments, ( + "A possibly-missing identity is passed straight into a Cosmos-backed lookup at " + f"{leaked_identity_arguments}. Resolve the identity first and skip the lookup when it " + "is absent." + ) + + # The safe helper must actually be imported rather than resolved dynamically. + imported_names = set() + for node in ast.walk(loader_tree): + if isinstance(node, ast.ImportFrom): + for alias in node.names: + imported_names.add(alias.asname or alias.name) + + assert SAFE_IDENTITY_HELPER in imported_names, ( + f"semantic_kernel_loader.py must import {SAFE_IDENTITY_HELPER}." + ) + + print(f"โœ… Loader routes all {len(safe_calls)} identity lookups through the safe helper.") + return True + + except Exception as e: + print(f"โŒ Test failed: {e}") + traceback.print_exc() + return False + + +def test_version_is_at_least_implementation_version(): + """The application version must be at least the version this fix shipped in.""" + print("๐Ÿ” Testing application version...") + + try: + app_version = assert_app_version_at_least( + IMPLEMENTED_IN_VERSION, + reason="Semantic Kernel startup request-context guard shipped in this version.", + ) + print(f"โœ… Application version {app_version} is at least {IMPLEMENTED_IN_VERSION}.") + return True + + except Exception as e: + print(f"โŒ Test failed: {e}") + traceback.print_exc() + return False + + +if __name__ == "__main__": + tests = [ + test_identity_helpers_outside_and_inside_request_context, + test_loader_uses_only_the_safe_identity_helper, + test_version_is_at_least_implementation_version, + ] + + results = [] + for test in tests: + print(f"\n๐Ÿงช Running {test.__name__}...") + results.append(test()) + + print(f"\n๐Ÿ“Š Results: {sum(results)}/{len(results)} tests passed") + sys.exit(0 if all(results) else 1)