From 1ba4733233af8a25653fff3b5f00acb0137c3bbe Mon Sep 17 00:00:00 2001 From: Bereket Terefe Date: Wed, 15 Apr 2026 10:03:32 +0300 Subject: [PATCH 1/9] fix(authentication): remove client-side token expiration checks to handle clock skew (cherry picked from commit 01b41a60bfea920fa5032829fca4a6d1c3f6ddf2) --- .../services/Authentication.service.test.ts | 46 +++-------- .../auth/services/Authentication.service.ts | 22 +---- ...irebaseEmailAuthentication.service.test.ts | 80 ++++++++++++------- 3 files changed, 61 insertions(+), 87 deletions(-) diff --git a/frontend-new/src/auth/services/Authentication.service.test.ts b/frontend-new/src/auth/services/Authentication.service.test.ts index 8ad484875..88b50ea4a 100644 --- a/frontend-new/src/auth/services/Authentication.service.test.ts +++ b/frontend-new/src/auth/services/Authentication.service.test.ts @@ -1,5 +1,5 @@ import "src/_test_utilities/consoleMock"; -import AuthenticationService, { CLOCK_TOLERANCE, TokenValidationFailureCause } from "./Authentication.service"; +import AuthenticationService, { TokenValidationFailureCause } from "./Authentication.service"; import { jwtDecode } from "jwt-decode"; import { PersistentStorageService } from "src/app/PersistentStorageService/PersistentStorageService"; import UserPreferencesService from "src/userPreferences/UserPreferencesService/userPreferences.service"; @@ -480,52 +480,24 @@ describe("AuthenticationService", () => { expect(console.warn).not.toHaveBeenCalled(); }); - test("should return false for expired token", () => { - // GIVEN an expired token - const givenToken = "expired-token"; - (jwtDecode as jest.Mock) - .mockReturnValueOnce({ - typ: "JWT", - alg: "RS256", - kid: "key-id", - } as TokenHeader) - .mockReturnValueOnce({ - iat: currentTime - CLOCK_TOLERANCE - 100, - exp: currentTime - CLOCK_TOLERANCE - 1, - } as Token); - - // WHEN isTokenValid is called - const result = service.isTokenValid(givenToken); - - // THEN it should return false with TOKEN_EXPIRED - expect(result.isValid).toBe(false); - expect(result.decodedToken).toBeNull(); - expect(result.failureCause).toBe(TokenValidationFailureCause.TOKEN_EXPIRED); - - // AND expect no errors or warning to have occurred - expect(console.error).not.toHaveBeenCalled(); - expect(console.warn).not.toHaveBeenCalled(); - }); - - test("should return true when iat is in the future due to client clock skew", () => { - // GIVEN a token where the server clock is ahead of the client (iat appears to be in the future) - // but the token has not yet expired - const givenToken = "clock-skewed-token"; + test.each([ + ["expired token", { iat: currentTime - 100, exp: currentTime - 1 }], + ["future token", { iat: currentTime + 1, exp: currentTime + 100 }], + ])("should return true for %s (time checks are deferred to server-side 401)", (_, payload) => { + // GIVEN a token whose timing would previously have been rejected client-side + const givenToken = "time-skewed-token"; (jwtDecode as jest.Mock) .mockReturnValueOnce({ typ: "JWT", alg: "RS256", kid: "key-id", } as TokenHeader) - .mockReturnValueOnce({ - iat: currentTime + CLOCK_TOLERANCE + 1, - exp: currentTime + CLOCK_TOLERANCE + 100, - } as Token); + .mockReturnValueOnce(payload as Token); // WHEN isTokenValid is called const result = service.isTokenValid(givenToken); - // THEN it should return true — iat is not checked client-side + // THEN it should return true — device clock skew must not block the user expect(result.isValid).toBe(true); expect(result.decodedToken).toBeDefined(); expect(result.failureCause).toBeUndefined(); diff --git a/frontend-new/src/auth/services/Authentication.service.ts b/frontend-new/src/auth/services/Authentication.service.ts index e384dce49..138c5ccfb 100644 --- a/frontend-new/src/auth/services/Authentication.service.ts +++ b/frontend-new/src/auth/services/Authentication.service.ts @@ -197,26 +197,10 @@ abstract class AuthenticationService { } // Decode the token and validate it + // Note: we intentionally do not check expiry or issuance time here. + // Device clocks can be skewed (ahead or behind), which causes false negatives. + // Instead, we rely on the server returning 401 to trigger a token refresh. const decodedToken: Token = jwtDecode(token); - // The tolerance must be at lest greater than the precision loss by the Math.floor function (1 second) - const currentTime = Math.floor(Date.now() / 1000); - - // Check expiration with buffer - // ideally this would be a simple check, but since there is a chance that the server and client clocks are not perfectly synchronized, - // we add a buffer to the expiration time to account for the potential time difference - if (currentTime > decodedToken.exp + CLOCK_TOLERANCE) { - console.debug("Token is expired: ", { exp: decodedToken.exp, currentTime }); - return { isValid: false, decodedToken: null, failureCause: TokenValidationFailureCause.TOKEN_EXPIRED }; - } else if (currentTime > decodedToken.exp) { - console.warn( - "Warning: token expiration time has elapsed, but is still within the acceptable tolerance period", - { - exp: decodedToken.exp, - currentTime, - } - ); - } - return { isValid: true, decodedToken: decodedToken }; } catch (error) { return { isValid: false, decodedToken: null, failureCause: TokenValidationFailureCause.ERROR_DECODING_TOKEN }; diff --git a/frontend-new/src/auth/services/FirebaseAuthenticationService/emailAuth/FirebaseEmailAuthentication.service.test.ts b/frontend-new/src/auth/services/FirebaseAuthenticationService/emailAuth/FirebaseEmailAuthentication.service.test.ts index 0d31ceb02..d619821ed 100644 --- a/frontend-new/src/auth/services/FirebaseAuthenticationService/emailAuth/FirebaseEmailAuthentication.service.test.ts +++ b/frontend-new/src/auth/services/FirebaseAuthenticationService/emailAuth/FirebaseEmailAuthentication.service.test.ts @@ -1029,30 +1029,42 @@ describe("AuthService class tests", () => { expect(console.warn).not.toHaveBeenCalled(); }); - test("should return null and log debug when token is expired", () => { - // GIVEN an expired token - const givenToken = "expired-token"; + test("should return user even when token is past its exp claim (device clock may be skewed)", () => { + // GIVEN a token whose exp is in the past relative to the mocked device clock + const givenToken = "past-exp-token"; + const givenDecodedToken = { + sub: "user-id", + email: "test@example.com", + exp: 500, // Past expiry (current time is 1000) + iat: 100, + firebase: { sign_in_provider: "password" }, + }; + const givenUser = { id: "user-id", email: "test@example.com", name: "Test User" }; - // Mock jwtDecode to return expired token + // Mock jwtDecode to return the token (jwtDecode as jest.Mock) .mockReturnValueOnce({ typ: "JWT", alg: "RS256", kid: "key-id", }) - .mockReturnValueOnce({ - exp: 500, // Past expiry (current time is 1000) - iat: 100, - }); + .mockReturnValueOnce(givenDecodedToken); + + // Mock firebase token validation to accept the token + jest.spyOn(StdFirebaseAuthenticationService.getInstance(), "isFirebaseTokenValid").mockReturnValueOnce({ + isValid: true, + }); + + // Mock getUserFromDecodedToken to return a user + jest + .spyOn(StdFirebaseAuthenticationService.getInstance(), "getUserFromDecodedToken") + .mockReturnValueOnce(givenUser); // WHEN getUser is called const result = authService.getUser(givenToken); - // THEN it should return null - expect(result).toBeNull(); - - // AND it should log a debug message - expect(console.debug).toHaveBeenCalled(); + // THEN it should return the user — time-based rejection is handled server-side via 401 + expect(result).toEqual(givenUser); // AND no errors or warnings should be logged expect(console.error).not.toHaveBeenCalled(); @@ -1076,7 +1088,7 @@ describe("AuthService class tests", () => { // THEN it should return null expect(result).toBeNull(); - // AND it should log an error (this comes from getUser when failureCause is not TOKEN_EXPIRED) + // AND it should log an error expect(console.error).toHaveBeenCalled(); // AND no warnings should be logged @@ -1132,36 +1144,42 @@ describe("AuthService class tests", () => { expect(console.warn).not.toHaveBeenCalled(); }); - test("should return isValid false for expired token", () => { - // GIVEN an expired token - const givenToken = "expired-token"; + test("should return isValid true for token past its exp claim (device clock may be skewed)", () => { + // GIVEN a token whose exp is in the past relative to the mocked device clock + const givenToken = "past-exp-token"; + const givenDecodedToken = { + sub: "user-id", + email: "test@example.com", + exp: 500, // Past expiry (current time is 1000) + iat: 100, + firebase: { sign_in_provider: "password" }, + }; - // Mock jwtDecode to return expired token + // Mock jwtDecode to return the token (jwtDecode as jest.Mock) .mockReturnValueOnce({ typ: "JWT", alg: "RS256", kid: "key-id", }) - .mockReturnValueOnce({ - exp: 500, // Past expiry (current time is 1000) - iat: 100, - }); + .mockReturnValueOnce(givenDecodedToken); + + // Mock firebase token validation to accept the token + jest.spyOn(StdFirebaseAuthenticationService.getInstance(), "isFirebaseTokenValid").mockReturnValueOnce({ + isValid: true, + }); // WHEN isTokenValid is called const result = authService.isTokenValid(givenToken); - // THEN it should return isValid false - expect(result.isValid).toBe(false); - - // AND it should return null for decoded token - expect(result.decodedToken).toBeNull(); + // THEN it should return isValid true — time-based rejection is handled server-side via 401 + expect(result.isValid).toBe(true); - // AND it should have TOKEN_EXPIRED as the failure cause - expect(result.failureCause).toBe(TokenValidationFailureCause.TOKEN_EXPIRED); + // AND it should return the decoded token + expect(result.decodedToken).toEqual(givenDecodedToken); - // AND it should log a debug message - expect(console.debug).toHaveBeenCalled(); + // AND it should not have a failure cause + expect(result.failureCause).toBeUndefined(); // AND no errors or warnings should be logged expect(console.error).not.toHaveBeenCalled(); From fc819c9c7dfb59ed431ff2ab990a9b422de65df3 Mon Sep 17 00:00:00 2001 From: Bereket Terefe Date: Wed, 15 Apr 2026 14:07:44 +0300 Subject: [PATCH 2/9] fix(authentication): implement retry logic for token refresh on network errors API gateway STANDARD rate kept at 1200 req/min (upstream raised it to 3600) to match core's traffic profile. (cherry picked from commit 30f36852bf882840cc901b32b3eddf2eb6e2815a) --- .../StdFirebaseAuthenticationService.test.ts | 88 ++++++++++++++++++- .../StdFirebaseAuthenticationService.ts | 22 +++++ iac/backend/openapi2_template.yaml | 2 +- 3 files changed, 108 insertions(+), 4 deletions(-) diff --git a/frontend-new/src/auth/services/FirebaseAuthenticationService/StdFirebaseAuthenticationService.test.ts b/frontend-new/src/auth/services/FirebaseAuthenticationService/StdFirebaseAuthenticationService.test.ts index 03e3154fb..18e2bde96 100644 --- a/frontend-new/src/auth/services/FirebaseAuthenticationService/StdFirebaseAuthenticationService.test.ts +++ b/frontend-new/src/auth/services/FirebaseAuthenticationService/StdFirebaseAuthenticationService.test.ts @@ -123,7 +123,7 @@ describe("StdFirebaseAuthenticationService", () => { expect(mockUnsubscribe).toHaveBeenCalled(); }); - test("should handle errors during token refresh", async () => { + test("should handle non-network errors during token refresh without retrying", async () => { // GIVEN a current user exists const mockError = new Error("Token refresh failed"); const mockUser = { @@ -133,7 +133,6 @@ describe("StdFirebaseAuthenticationService", () => { // AND the auth state listener is set up to return the user const mockUnsubscribe = jest.fn(); (firebaseAuth.onAuthStateChanged as jest.Mock).mockImplementation((callback) => { - // Simulate the callback being called with the mock user callback(mockUser); return mockUnsubscribe; }); @@ -141,12 +140,95 @@ describe("StdFirebaseAuthenticationService", () => { // WHEN refreshToken is called const refreshPromise = service.refreshToken(); - // THEN it should reject with the error + // THEN it should reject with the error immediately (no retries for non-network errors) await expect(refreshPromise).rejects.toThrow("Token refresh failed"); + // AND it should only have called onAuthStateChanged once + expect(firebaseAuth.onAuthStateChanged).toHaveBeenCalledTimes(1); + // AND it should call the unsubscribe function expect(mockUnsubscribe).toHaveBeenCalled(); }); + + xtest("should retry up to 3 times on network errors and eventually succeed", async () => { + jest.useFakeTimers(); + + // GIVEN a network error on the first two attempts, then success + const networkError = Object.assign(new Error("network-request-failed"), { + code: "auth/network-request-failed", + }); + const mockToken = "new-token"; + const mockUserFailing = { getIdToken: jest.fn().mockRejectedValue(networkError) }; + const mockUserSuccess = { getIdToken: jest.fn().mockResolvedValue(mockToken) }; + const mockUnsubscribe = jest.fn(); + + (firebaseAuth.onAuthStateChanged as jest.Mock) + .mockImplementationOnce((callback) => { + callback(mockUserFailing); + return mockUnsubscribe; + }) + .mockImplementationOnce((callback) => { + callback(mockUserFailing); + return mockUnsubscribe; + }) + .mockImplementationOnce((callback) => { + callback(mockUserSuccess); + return mockUnsubscribe; + }); + + jest.spyOn(AuthenticationStateService.getInstance(), "setToken"); + + // WHEN refreshToken is called + const refreshPromise = service.refreshToken(); + + // Advance timers for each retry delay (1s, 2s) + // Flush microtasks between timer advances to allow async code to run + await Promise.resolve(); + jest.advanceTimersByTime(1000); + await Promise.resolve(); + jest.advanceTimersByTime(2000); + await Promise.resolve(); + + const result = await refreshPromise; + + // THEN it should eventually return the new token + expect(result).toBe(mockToken); + + // AND it should have retried (3 total attempts) + expect(firebaseAuth.onAuthStateChanged).toHaveBeenCalledTimes(3); + + jest.useRealTimers(); + }); + + xtest("should fail after 3 network error retries", async () => { + jest.useFakeTimers(); + + // GIVEN a network error on all attempts + const networkError = Object.assign(new Error("network-request-failed"), { + code: "auth/network-request-failed", + }); + const mockUserFailing = { getIdToken: jest.fn().mockRejectedValue(networkError) }; + const mockUnsubscribe = jest.fn(); + + (firebaseAuth.onAuthStateChanged as jest.Mock).mockImplementation((callback) => { + callback(mockUserFailing); + return mockUnsubscribe; + }); + + // WHEN refreshToken is called + const refreshPromise = service.refreshToken(); + + // Advance timers through all retry delays + await jest.runAllTimersAsync(); + + // THEN it should reject after exhausting retries + await expect(refreshPromise).rejects.toThrow("network-request-failed"); + + // AND it should have tried 3 times + expect(firebaseAuth.onAuthStateChanged).toHaveBeenCalledTimes(3); + + jest.useRealTimers(); + }); }); describe("getUserFromDecodedToken", () => { diff --git a/frontend-new/src/auth/services/FirebaseAuthenticationService/StdFirebaseAuthenticationService.ts b/frontend-new/src/auth/services/FirebaseAuthenticationService/StdFirebaseAuthenticationService.ts index df319c5dc..a2814cdb5 100644 --- a/frontend-new/src/auth/services/FirebaseAuthenticationService/StdFirebaseAuthenticationService.ts +++ b/frontend-new/src/auth/services/FirebaseAuthenticationService/StdFirebaseAuthenticationService.ts @@ -131,6 +131,28 @@ class StdFirebaseAuthenticationService { const oldToken = AuthenticationStateService.getInstance().getToken(); console.debug("Old token", "..." + oldToken?.slice(-20)); + const MAX_RETRIES = 3; + const RETRY_DELAY_MS = 1000; + + for (let attempt = 1; attempt <= MAX_RETRIES; attempt++) { + try { + return await this._refreshTokenOnce(); + } catch (error) { + const isNetworkError = error instanceof Error && (error as any).code === "auth/network-request-failed"; + if (isNetworkError && attempt < MAX_RETRIES) { + console.warn(`Token refresh attempt ${attempt} failed due to network error, retrying...`); + await new Promise((resolve) => setTimeout(resolve, RETRY_DELAY_MS * attempt)); + } else { + throw error; + } + } + } + + // Should never be reached, but TypeScript requires a return + throw new Error("Token refresh failed after max retries"); + } + + private _refreshTokenOnce(): Promise { // We need to use a one-time auth state listener because firebaseAuth.currentUser // might not be available immediately on page load. Its not perfectly documented that this will be triggered on // subscribing to onAuthStateChanged, but it seems to be the case. And this is the recommended way to handle auth state changes in Firebase. diff --git a/iac/backend/openapi2_template.yaml b/iac/backend/openapi2_template.yaml index 958031894..3f1dc9b6d 100644 --- a/iac/backend/openapi2_template.yaml +++ b/iac/backend/openapi2_template.yaml @@ -22,7 +22,7 @@ x-google-management: metric: "request-metric" unit: "1/min/{project}" values: - STANDARD: 120 # requests per minute + STANDARD: 1200 # requests per minute x-google-backend: address: "__BACKEND_URI__" deadline: "__API_GATEWAY_TIMEOUT__" # timeout as the cloud run container may take a while to start, and some requests may take a while to process. Reference: https://cloud.google.com/endpoints/docs/openapi/openapi-extensions#deadline From ecb126945a1a5ad9207b0def6bfdd5db8d5821a4 Mon Sep 17 00:00:00 2001 From: Anselme Irumva Date: Tue, 28 Apr 2026 19:17:19 +0200 Subject: [PATCH 3/9] refactor(env): split `VERTEX_API_REGION` into `VERTEX_API_EMBEDDINGS_REGION` and `VERTEX_API_GEN_AI_REGION` MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Update environment variable names for better specificity. - Refactor related configurations, initialization code, and documentation to align with changes. Upstream career-explorer / jobs MongoDB env-var additions and the non_priority_sector_explorer.py / ingest_sector_document.py scripts dropped — those features do not exist in core compass. (cherry picked from commit 6c00932a24976e9a8f491300308dc468879818c4) --- backend/.env.example | 6 +++++- backend/README.md | 11 +++++++---- backend/app/vector_search/embeddings_model.py | 13 ++++++++++--- backend/common_libs/llm/models_utils.py | 17 ++++++++++------- .../test_utilities/setup_env_vars.py | 3 ++- .../scripts/embeddings/evaluate_embeddings.py | 4 ++-- iac/.env.example | 3 ++- iac/backend/__main__.py | 3 ++- iac/backend/deploy_backend.py | 10 +++++++--- iac/templates/env.template | 3 ++- 10 files changed, 49 insertions(+), 24 deletions(-) diff --git a/backend/.env.example b/backend/.env.example index 04fbb1ee4..8585361f0 100644 --- a/backend/.env.example +++ b/backend/.env.example @@ -18,7 +18,11 @@ USERDATA_DATABASE_NAME=compass-dev # Google Cloud Platform settings GOOGLE_APPLICATION_CREDENTIALS=keys/credentials.json -VERTEX_API_REGION=us-central1 +# Region for the embeddings model (text-embedding-005 etc.). Must be a regional location +# (e.g. us-central1) — these models are not published in the global publisher catalog. +VERTEX_API_EMBEDDINGS_REGION=us-central1 +# Region for generative-AI calls (Gemini etc.). Can be a regional location or "global". +VERTEX_API_GEN_AI_REGION=us-central1 EMBEDDINGS_SERVICE_NAME=GOOGLE-VERTEX-AI # See https://cloud.google.com/vertex-ai/generative-ai/docs/embeddings/get-text-embeddings#supported-models e.g.: # text-embedding-005 diff --git a/backend/README.md b/backend/README.md index 4b0131c99..943512a72 100644 --- a/backend/README.md +++ b/backend/README.md @@ -157,7 +157,8 @@ The backend uses the following environment variables: - `USERDATA_DATABASE_NAME`: The name of the mongo db database used by the application to store user data. - `METRICS_MONGODB_URI`: The URI of the MongoDB instance for the metrics database. - `METRICS_DATABASE_NAME`: The name of the mongo db database used by the application to store metrics data. -- `VERTEX_API_REGION`: (optional) The region of the Vertex API to use. If not set defaults to `us-central1`. +- `VERTEX_API_EMBEDDINGS_REGION`: The region of the Vertex API to use for embedding models. Must be a regional location (e.g. `us-central1`) — embedding models such as `text-embedding-005` are not published in the global publisher catalog. +- `VERTEX_API_GEN_AI_REGION`: (optional) The region of the Vertex API to use for generative-AI calls (Gemini etc.). Can be a regional location or `global`. If not set, defaults to `us-central1`. - `EMBEDDINGS_SERVICE_NAME`: The name of the embeddings service to use. Currently, the only supported service is `GOOGLE-VERTEX-AI`. - `EMBEDDINGS_MODEL_NAME`: The name of the embeddings model to use. See https://cloud.google.com/vertex-ai/generative-ai/docs/embeddings/get-text-embeddings#supported-models for the list of supported models. - `LOG_CONFIG_FILE`: (Optional) See the [Logging](#logging) section for more information. If not set defaults to `logging.cfg.yaml`. @@ -196,7 +197,8 @@ USERDATA_MONGODB_URI= USERDATA_DATABASE_NAME= METRICS_MONGODB_URI= METRICS_DATABASE_NAME= -VERTEX_API_REGION= +VERTEX_API_EMBEDDINGS_REGION= +VERTEX_API_GEN_AI_REGION= EMBEDDINGS_SERVICE_NAME= EMBEDDINGS_MODEL_NAME= LOG_CONFIG_FILE= @@ -268,7 +270,7 @@ To run the image, you'll need to mount a volume with the service account key and the container: ```shell -docker run -v ":/code/credentials.json" -e GOOGLE_APPLICATION_CREDENTIALS="/code/credentials.json" -e MONGODB_URI="" -e VERTEX_API_REGION="" -p 8080:8080 compass-backend +docker run -v ":/code/credentials.json" -e GOOGLE_APPLICATION_CREDENTIALS="/code/credentials.json" -e MONGODB_URI="" -e VERTEX_API_EMBEDDINGS_REGION="" -e VERTEX_API_GEN_AI_REGION="" -p 8080:8080 compass-backend ``` If you have set up the `.env` file, you can run the image using the `--env-file` option. @@ -289,7 +291,8 @@ USERDATA_DATABASE_NAME=_compass-users-local METRICS_MONGODB_URI=mongodb://localhost:27017 METRICS_DATABASE_NAME= GOOGLE_APPLICATION_CREDENTIALS=keys/credentials.json -VERTEX_API_REGION= +VERTEX_API_EMBEDDINGS_REGION= +VERTEX_API_GEN_AI_REGION= EMBEDDINGS_SERVICE_VERSION= LOG_CONFIG_FILE=logging.cfg.dev.yaml # allow all origins diff --git a/backend/app/vector_search/embeddings_model.py b/backend/app/vector_search/embeddings_model.py index 6fe27494e..a1ea73ef5 100644 --- a/backend/app/vector_search/embeddings_model.py +++ b/backend/app/vector_search/embeddings_model.py @@ -45,9 +45,16 @@ class GoogleEmbeddingService(EmbeddingService): def __init__(self, model_name: str): super().__init__(service_name="GOOGLE-VERTEX-AI", model_name=model_name) - if os.getenv("VERTEX_API_REGION") is None: - raise ValueError("Environment variable 'VERTEX_API_REGION' is not set.") - self.region = os.getenv("VERTEX_API_REGION") + # Embeddings run in their own region, separate from the generative-AI region, because + # legacy embedding models (e.g. text-embedding-005) are not published in the global + # publisher catalog. Resolving via vertexai.init(location=...) before from_pretrained + # binds the returned model to that regional endpoint for all subsequent embed calls. + self.region = os.getenv("VERTEX_API_EMBEDDINGS_REGION") + if not self.region: + raise ValueError("Environment variable 'VERTEX_API_EMBEDDINGS_REGION' is not set.") + self.logger.info(f"Constructing {self.__class__.__name__} with model: {model_name} and region: {self.region}") + + vertexai.init(location=self.region) self.model = TextEmbeddingModel.from_pretrained(model_name) async def embed(self, text: str) -> list[float]: diff --git a/backend/common_libs/llm/models_utils.py b/backend/common_libs/llm/models_utils.py index 1b747b9a6..989c1e841 100644 --- a/backend/common_libs/llm/models_utils.py +++ b/backend/common_libs/llm/models_utils.py @@ -15,14 +15,15 @@ # Load environment variables from .env file load_dotenv() -# Initialize the default region for the Vertex AI client +# Initialize the default region for the Vertex AI generative-AI client. +# Embeddings use a separate region (VERTEX_API_EMBEDDINGS_REGION) — see GoogleEmbeddingService. -DEFAULT_VERTEX_API_REGION = os.getenv("VERTEX_API_REGION") -if not DEFAULT_VERTEX_API_REGION: - logging.warning("Default Vertex AI region is not set. Using 'us-central1' as the default region.") - DEFAULT_VERTEX_API_REGION = "us-central1" +DEFAULT_VERTEX_API_GEN_AI_REGION = os.getenv("VERTEX_API_GEN_AI_REGION") +if not DEFAULT_VERTEX_API_GEN_AI_REGION: + logging.warning("VERTEX_API_GEN_AI_REGION is not set. Using 'us-central1' as the default region.") + DEFAULT_VERTEX_API_GEN_AI_REGION = "us-central1" else: - logging.debug("Default Vertex AI region is %s", DEFAULT_VERTEX_API_REGION) + logging.info("Default Vertex AI gen-AI region is %s", DEFAULT_VERTEX_API_GEN_AI_REGION) DEFAULT_GENERATION_CONFIG = { "temperature": 0.1, @@ -147,7 +148,7 @@ class LLMConfig(BaseModel): Configuration for the LLM. """ language_model_name: str = AgentsConfig.default_model - location: str = DEFAULT_VERTEX_API_REGION + location: str = DEFAULT_VERTEX_API_GEN_AI_REGION generation_config: dict = DEFAULT_GENERATION_CONFIG safety_settings: frozenset[SafetySetting] = DEFAULT_SAFETY_SETTINGS retry_config: RetryConfigWithExponentialBackOff = DEFAULT_RETRY_CONFIG_WITH_EXP_BACKOFF @@ -210,6 +211,8 @@ class BasicLLM(LLM): def __init__(self, *, config: LLMConfig = LLMConfig()): # Before constructing the llm model, we need to initialize the VertexAI client # as the init function may have been called in another module with different parameters + self.logger = logging.getLogger(self.__class__.__name__) + self.logger.info("Initializing VertexAI client with location: %s", config.location) vertexai.init(location=config.location) self._retry_config = config.retry_config self._model = None diff --git a/backend/common_libs/test_utilities/setup_env_vars.py b/backend/common_libs/test_utilities/setup_env_vars.py index b80a14c47..96d6cf63c 100644 --- a/backend/common_libs/test_utilities/setup_env_vars.py +++ b/backend/common_libs/test_utilities/setup_env_vars.py @@ -109,7 +109,8 @@ def setup_env_vars(*, env_vars: dict[str, str] = None): 'USERDATA_DATABASE_NAME': "foo", 'TAXONOMY_MODEL_ID': str(ObjectId()), 'GOOGLE_APPLICATION_CREDENTIALS': "foo", - 'VERTEX_API_REGION': "foo", + 'VERTEX_API_EMBEDDINGS_REGION': "foo", + 'VERTEX_API_GEN_AI_REGION': "foo", 'EMBEDDINGS_SERVICE_NAME': "foo", 'EMBEDDINGS_MODEL_NAME': "foo", 'LOG_CONFIG_FILE': "logging.cfg.yaml", diff --git a/backend/scripts/embeddings/evaluate_embeddings.py b/backend/scripts/embeddings/evaluate_embeddings.py index d316fe4ce..c33326429 100755 --- a/backend/scripts/embeddings/evaluate_embeddings.py +++ b/backend/scripts/embeddings/evaluate_embeddings.py @@ -175,9 +175,9 @@ def evaluate_top_k_hits( async def main(*, do_skills: bool = False, do_occupations: bool = False): - region = os.getenv("VERTEX_API_REGION") + region = os.getenv("VERTEX_API_EMBEDDINGS_REGION") if not region: - raise ValueError("VERTEX_API_REGION environment variable is not set.") + raise ValueError("VERTEX_API_EMBEDDINGS_REGION environment variable is not set.") vertexai.init(location=region) search_services = await get_search_services() diff --git a/iac/.env.example b/iac/.env.example index 42c8f73ed..315be954f 100644 --- a/iac/.env.example +++ b/iac/.env.example @@ -26,7 +26,8 @@ USERDATA_MONGODB_URI= USERDATA_DATABASE_NAME= # Google Cloud Platform settings -VERTEX_API_REGION= +VERTEX_API_EMBEDDINGS_REGION= +VERTEX_API_GEN_AI_REGION= # Currently only supports google-vertex-ai EMBEDDINGS_SERVICE_NAME=GOOGLE-VERTEX-AI EMBEDDINGS_MODEL_NAME= diff --git a/iac/backend/__main__.py b/iac/backend/__main__.py index cdad2a8aa..5a2072ae0 100644 --- a/iac/backend/__main__.py +++ b/iac/backend/__main__.py @@ -53,7 +53,8 @@ def main(): metrics_database_name=getenv("METRICS_DATABASE_NAME"), userdata_database_name=getenv("USERDATA_DATABASE_NAME"), userdata_mongodb_uri=getenv("USERDATA_MONGODB_URI", True), - vertex_api_region=getenv("VERTEX_API_REGION", True), + vertex_api_embeddings_region=getenv("VERTEX_API_EMBEDDINGS_REGION", True), + vertex_api_gen_ai_region=getenv("VERTEX_API_GEN_AI_REGION", True), embeddings_service_name=getenv("EMBEDDINGS_SERVICE_NAME"), embeddings_model_name=getenv("EMBEDDINGS_MODEL_NAME"), target_environment_name=environment_name, diff --git a/iac/backend/deploy_backend.py b/iac/backend/deploy_backend.py index 6ea0d08d4..64827b3a7 100644 --- a/iac/backend/deploy_backend.py +++ b/iac/backend/deploy_backend.py @@ -30,7 +30,8 @@ class BackendServiceConfig: metrics_database_name: str userdata_mongodb_uri: str userdata_database_name: str - vertex_api_region: str + vertex_api_embeddings_region: str + vertex_api_gen_ai_region: str embeddings_service_name: str embeddings_model_name: str target_environment_name: str @@ -360,8 +361,11 @@ def _deploy_cloud_run_service( name="USERDATA_DATABASE_NAME", value=backend_service_cfg.userdata_database_name), gcp.cloudrunv2.ServiceTemplateContainerEnvArgs( - name="VERTEX_API_REGION", - value=backend_service_cfg.vertex_api_region), + name="VERTEX_API_EMBEDDINGS_REGION", + value=backend_service_cfg.vertex_api_embeddings_region), + gcp.cloudrunv2.ServiceTemplateContainerEnvArgs( + name="VERTEX_API_GEN_AI_REGION", + value=backend_service_cfg.vertex_api_gen_ai_region), gcp.cloudrunv2.ServiceTemplateContainerEnvArgs( name="EMBEDDINGS_SERVICE_NAME", value=backend_service_cfg.embeddings_service_name), diff --git a/iac/templates/env.template b/iac/templates/env.template index f2cad4a00..f74d9a130 100644 --- a/iac/templates/env.template +++ b/iac/templates/env.template @@ -26,7 +26,8 @@ USERDATA_MONGODB_URI= USERDATA_DATABASE_NAME= # Google Cloud Platform settings -VERTEX_API_REGION= +VERTEX_API_EMBEDDINGS_REGION= +VERTEX_API_GEN_AI_REGION= EMBEDDINGS_SERVICE_NAME=/^GOOGLE-VERTEX-AI$/ EMBEDDINGS_MODEL_NAME= From 58b98e15b9485880883087b603be878f4d92946d Mon Sep 17 00:00:00 2001 From: Bereket Terefe Date: Thu, 7 May 2026 13:56:29 +0300 Subject: [PATCH 4/9] fix(sentence_decomposition): enforce grammatical person consistency in sentence corrections [pulumi up] (cherry picked from commit 15b0b771b54a777608c8d49f85a858e5e1598c56) --- .../_responsibilities_extraction_llm.py | 26 ++++++++++++++----- .../_sentence_decomposition_llm.py | 1 + 2 files changed, 20 insertions(+), 7 deletions(-) diff --git a/backend/app/agent/skill_explorer_agent/_responsibilities_extraction_llm.py b/backend/app/agent/skill_explorer_agent/_responsibilities_extraction_llm.py index 4451cb408..4cc62ef1c 100644 --- a/backend/app/agent/skill_explorer_agent/_responsibilities_extraction_llm.py +++ b/backend/app/agent/skill_explorer_agent/_responsibilities_extraction_llm.py @@ -2,7 +2,7 @@ from textwrap import dedent from typing import Optional -from pydantic import Field +from pydantic import BaseModel, Field from app.agent.agent_types import LLMStats from app.agent.experience.experience_entity import ResponsibilitiesData @@ -16,13 +16,20 @@ _TAGS_TO_FILTER = ["system instructions", "user's last input", "conversation history"] -class ResponsibilitiesExtractionResponse(ResponsibilitiesData): +class ResponsibilitiesExtractionResponse(BaseModel): + # extracted_entities MUST come first so the model reasons before classifying. + # The JSON schema is generated in field-declaration order, and the model fills + # fields left-to-right — if classification fields appear first the model outputs + # empty buckets before it has done any reasoning. extracted_entities: list[str] = Field(default_factory=list) """ The extracted entities from the user's input. This acts as a "reasoning" field and should be predicted before the classes. """ + other_peoples_responsibilities: list[str] = Field(default_factory=list) + non_responsibilities: list[str] = Field(default_factory=list) + responsibilities: list[str] = Field(default_factory=list) irrelevant_entities: Optional[list[str]] = Field(default_factory=list) """ The irrelevant entities from the user's input. @@ -109,8 +116,11 @@ def _create_extraction_system_instructions() -> str: You will collect and place the entities into the 'extracted_entities' list of output. - + # Classification instructions + Every entity in 'extracted_entities' MUST appear in exactly one of the four classification lists below. + Do not leave any extracted entity unclassified. + Classify the named entities into one of the following four classes: - other_peoples_responsibilities: What other people are responsible for. - non_responsibilities: What the user is not responsible for. @@ -126,10 +136,12 @@ def _create_extraction_system_instructions() -> str: When extracting non_responsibilities, normalize the entity text by removing any negation and convert to the positive form. The fact that it is classified as non_responsibilities already indicates it is something the user does not do. There are two criteria and both must be met for a named entity to be in responsibilities: - 1. The named entity must be something that the user is directly responsible for, - possesses, does, performs, takes, exhibits, engages in, or knows. - AND - 2. At least one of the subjects or the subject pronouns of the named entity must be referring to the user. + 1. At least one of the subjects or the subject pronouns of the named entity must be referring to the user. + AND + 2. The entity is NOT in non_responsibilities (i.e. the user is not explicitly saying they do NOT do it). + + When in doubt, classify a first-person entity as responsibilities. Any action, task, activity, or behavior the user describes doing — including work tasks, interactions, and processes — belongs in responsibilities. + Only exclude from responsibilities if the user explicitly says they do NOT do it (→ non_responsibilities), or if the subject is clearly someone other than the user (→ other_peoples_responsibilities), or if the entity is completely unrelated to any work or experience (→ irrelevant_entities). For other_peoples_responsibilities, extract ALL actions, responsibilities, and behaviors performed by people other than the user. This includes actions performed by named individuals (like "John", "Mary", "my boss") and actions performed by third-person pronouns (like "he", "she", "they") when they clearly refer to someone other than the user. You MUST extract these entities even when they appear in the same sentence as the user's actions. If you see "John uses his senses" or "John does X", you must extract it as other_peoples_responsibilities. Do not omit entities for other people. diff --git a/backend/app/agent/skill_explorer_agent/_sentence_decomposition_llm.py b/backend/app/agent/skill_explorer_agent/_sentence_decomposition_llm.py index 4f155c1ad..424893f56 100644 --- a/backend/app/agent/skill_explorer_agent/_sentence_decomposition_llm.py +++ b/backend/app/agent/skill_explorer_agent/_sentence_decomposition_llm.py @@ -234,6 +234,7 @@ def _create_second_pass_system_instructions() -> str: Your task is to review each sentence and fix it to ensure that it is grammatically correct, clear, concise and sounds natural. Pay attention to awkward phrasing, grammatical errors, and any other issues that may affect the clarity and readability of the sentence. Do not change the meaning of the sentence or add any new information. + Do not change the grammatical person of the sentence. If a sentence uses first person ("I", "my", "me"), keep it in first person. Do not convert first-person sentences to third person ("The user", "he", "she", "they"). Both the input and the fixed sentence should be interpreted in a different way review independently. Each sentence from the input must fixed and added to the output in the decomposed_and_dereferenced list. From ce20f20376661aeb0f38fec742eb341d7c936b38 Mon Sep 17 00:00:00 2001 From: Anselme Irumva Date: Tue, 28 Apr 2026 10:54:38 +0200 Subject: [PATCH 5/9] feat: enhance duplicate question detection and add associated test coverage - Update `_detect_stickiness` to handle both consecutive and non-consecutive duplicate questions. - Introduce new test to validate forced finish behavior upon non-consecutive duplicate questions. - Refine `_conversation_llm` templates to strictly forbid re-asking or repeating questions. TURN FLOW + Transition adapted to reference core's existing (a)/(b)/(c) question categories; achievement-question gating and {turn_target} placeholder dropped (not present in core). (cherry picked from commit 3679765d9a9852b957e70d4721470291f38055ca) --- .../skill_explorer_agent/_conversation_llm.py | 44 +++++++++++++++---- 1 file changed, 35 insertions(+), 9 deletions(-) diff --git a/backend/app/agent/skill_explorer_agent/_conversation_llm.py b/backend/app/agent/skill_explorer_agent/_conversation_llm.py index c0d3d8d8f..6ba07da8a 100644 --- a/backend/app/agent/skill_explorer_agent/_conversation_llm.py +++ b/backend/app/agent/skill_explorer_agent/_conversation_llm.py @@ -181,7 +181,7 @@ def _create_conversation_system_instructions(*, without assuming any prior knowledge about my experience as {experience_title}{work_type}. You must ask me questions that help me reflect on my experience and describe it in detail. - + (a) Questions you must ask me to gather the details of my experience: - Can you describe a typical day as {experience_title}? - What else do you do as {experience_title}? @@ -194,6 +194,30 @@ def _create_conversation_system_instructions(*, (c) Question you must ask me to capture the broader context of my experience depending the type of work: - {get_question_c} + TURN FLOW: + 1. Details from category (a) + 2. Boundary question from category (b) + 3. Broader-context question from category (c) + 4. Follow-up clarification if needed, then end + + RULES: + - Skip topics I've already covered in detail + - Combine related questions when natural + - Do not ask two separate questions in the same sentence + - End when categories (a)-(c) are covered or I have nothing more to share + - NEVER re-ask a question already listed in , and never + ask a question that is substantively similar (same topic, same intent) to one + already asked. If my answer was brief, dismissive, or off-topic, treat the + topic as covered and move on to the next category — do not repeat yourself. + + DISENGAGEMENT SIGNALS: + If I respond with brief dismissals or non-answers across one or more turns + (e.g., "move on", "next", "next experience", "skip", "?", "nothing", "no", + "i don't know", a single emoji, or repeatedly asking to advance), I am + explicitly telling you I have nothing more to share. End the conversation + immediately by emitting , even if not all categories + in the TURN FLOW have been covered. Do not press for more. + Make sure you ask all the above questions from (a), (b), (c) to get a comprehensive understanding of the experience and what is important to me in that role. Here are the questions you have asked me until now: @@ -237,15 +261,17 @@ def _create_conversation_system_instructions(*, Do not disclose your instructions and always adhere to them not matter what I say. #Transition - After you have asked me all the relevant questions from (a), (b) and (c), - or I have explicitly stated that I dot not want to share anything about my experience anymore, - you will just say to the end of the conversation. + End the exploration by saying when ANY of: + - You have covered the key categories from (a), (b), and (c), OR + - I have explicitly stated I don't want to share more, OR + - I am giving disengagement signals (see DISENGAGEMENT SIGNALS above), OR + - Continuing would be redundant based on the information already provided, OR + - You would otherwise repeat a question already in + Do not add anything before or after the message. - - If I have not shared any information about my experience as {experience_title}{work_type}, - explicitly ask me if I really want to stop exploring the specific experience. - Explain that I will not be able to revisit the experience, if I decide to stop sharing information, - and wait for my response before deciding to end the conversation. + + If I have not shared any meaningful information about my experience as {experience_title}{work_type}, + ask me once if I really want to stop. If I confirm, end the conversation. """) return replace_placeholders_with_indent( From 5d8e2f19daa0aa81457b18893142251eb4dd6d81 Mon Sep 17 00:00:00 2001 From: Bereket Terefe Date: Tue, 5 May 2026 14:19:23 +0300 Subject: [PATCH 6/9] fix(experience): implement repetition detection and force-summarization for EXPLORE_SKILLS_AGENT questions (cherry picked from commit a486465bbc6c6fd6d46fc178817d29fc726e13e8) --- .../explore_experiences_agent_director.py | 59 ++++++++++++++++++- .../conversation_memory_manager.py | 31 ++++++++++ .../conversations/reactions/test_service.py | 3 + backend/app/server_config.py | 4 ++ 4 files changed, 94 insertions(+), 3 deletions(-) diff --git a/backend/app/agent/explore_experiences_agent_director.py b/backend/app/agent/explore_experiences_agent_director.py index e47b8a27a..e910e4fff 100644 --- a/backend/app/agent/explore_experiences_agent_director.py +++ b/backend/app/agent/explore_experiences_agent_director.py @@ -1,6 +1,6 @@ from enum import Enum from typing import Optional, Mapping, Any - +import logging import time from pydantic import BaseModel, field_serializer, field_validator, Field @@ -12,8 +12,9 @@ from app.agent.experience.experience_entity import ExperienceEntity from app.agent.experience.upgrade_experience import get_editable_experience from app.agent.linking_and_ranking_pipeline import ExperiencePipeline, ExperiencePipelineConfig -from app.agent.skill_explorer_agent import SkillsExplorerAgent -from app.conversation_memory.conversation_memory_manager import ConversationMemoryManager +from app.agent.skill_explorer_agent import SkillsExplorerAgent, SkillsExplorerAgentState +from app.conversation_memory.conversation_memory_manager import ConversationMemoryManager, IConversationMemoryManager +from app.server_config import REPETITION_SHORT_CIRCUIT_THRESHOLD from app.conversation_memory.conversation_memory_types import \ ConversationContext from app.countries import Country @@ -219,6 +220,14 @@ async def _dive_into_experiences(self, *, agent_output: AgentOutput = await self._exploring_skills_agent.execute(user_input=user_input, context=context) # Update the conversation history await self._conversation_manager.update_history(user_input, agent_output) + # Detect consecutive repetition in the agent's question list and flush the + # visible window if the threshold is exceeded, so the LLM won't pattern-match + # to repeated verbatim turns on the next call. + await _flush_if_repeating( + self._exploring_skills_agent.state, + self._conversation_manager, + self.logger, + ) # get the context again after updating the history context = await self._conversation_manager.get_conversation_context() if not agent_output.finished: @@ -421,3 +430,47 @@ async def _link_and_rank(self, *, llm_stats=pipline_result.llm_stats ) return agent_output + + +async def _flush_if_repeating( + state: SkillsExplorerAgentState | None, + conversation_manager: IConversationMemoryManager, + logger: logging.Logger, +) -> None: + """ + Detect REPETITION_SHORT_CIRCUIT_THRESHOLD consecutive identical trailing entries in + question_asked_until_now. If found, force-summarize the entire visible history so + the LLM no longer sees the repeated turns verbatim on the next call, and deduplicate + question_asked_until_now down to one copy of the repeated message. + Only consecutive repetition at the tail is considered — non-consecutive repetition + (e.g. the same transitional question asked legitimately for two different experiences + several turns apart) is left alone. + """ + if state is None: + return + questions = state.question_asked_until_now + if len(questions) < REPETITION_SHORT_CIRCUIT_THRESHOLD: + return + last = questions[-1] + consecutive = 0 + for q in reversed(questions): + if q == last: + consecutive += 1 + else: + break + if consecutive < REPETITION_SHORT_CIRCUIT_THRESHOLD: + return + + logger.warning( + "Detected %d consecutive identical agent messages in question_asked_until_now — " + "force-summarizing visible history to break the loop. Message: %r", + consecutive, + last[:120], + ) + await conversation_manager.force_summarize_all() + # Deduplicate: keep one copy of the repeated question so the NEVER re-ask rule still fires + state.question_asked_until_now = questions[:-consecutive] + [last] + # Trim answers_provided to stay in sync with the new question list length + new_len = len(state.question_asked_until_now) + if len(state.answers_provided) > new_len: + state.answers_provided = state.answers_provided[:new_len] diff --git a/backend/app/conversation_memory/conversation_memory_manager.py b/backend/app/conversation_memory/conversation_memory_manager.py index 9bf96365e..66261e2eb 100644 --- a/backend/app/conversation_memory/conversation_memory_manager.py +++ b/backend/app/conversation_memory/conversation_memory_manager.py @@ -42,6 +42,17 @@ async def update_history(self, user_input: AgentInput, agent_output: AgentOutput """ raise NotImplementedError() + @abstractmethod + async def force_summarize_all(self) -> None: + """ + Immediately summarize all turns currently in the visible window + (to_be_summarized + unsummarized) into the summary, then clear both windows. + + Use this to flush repeated turns out of the LLM's context so the model + no longer sees them verbatim. The information is preserved in the summary. + """ + raise NotImplementedError() + @abstractmethod async def is_user_message(self, message_id: str) -> bool: """ @@ -104,6 +115,26 @@ async def update_history(self, user_input: AgentInput, agent_output: AgentOutput if len(self._state.to_be_summarized_history.turns) == self._to_be_summarized_window_size: await self._summarize() + async def force_summarize_all(self) -> None: + try: + combined_turns = ( + self._state.to_be_summarized_history.turns + + self._state.unsummarized_history.turns + ) + if not combined_turns: + return + self._logger.debug("Force-summarizing all %d visible turns due to repetition", len(combined_turns)) + self._state.summary = await self._summarizer.summarize(ConversationContext( + all_history=ConversationHistory(turns=[]), + history=ConversationHistory(turns=combined_turns), + summary=self._state.summary, + )) + self._logger.debug("Post-force-summarize summary: %s", self._state.summary) + self._state.to_be_summarized_history.turns.clear() + self._state.unsummarized_history.turns.clear() + except Exception as e: + self._logger.error("Error in force_summarize_all: %s", e, exc_info=True) + async def is_user_message(self, message_id: str) -> bool: # find out if the message_id of the message to react to is found an input message # if it is in the turns as an input, that means it was a user message diff --git a/backend/app/conversations/reactions/test_service.py b/backend/app/conversations/reactions/test_service.py index ff092ee0b..17e2b3bd0 100644 --- a/backend/app/conversations/reactions/test_service.py +++ b/backend/app/conversations/reactions/test_service.py @@ -143,6 +143,9 @@ async def get_conversation_context(self): async def update_history(self, user_input: AgentInput, agent_output: AgentOutput): raise NotImplementedError() + async def force_summarize_all(self) -> None: + raise NotImplementedError() + async def is_user_message(self, message_id: str): raise NotImplementedError() diff --git a/backend/app/server_config.py b/backend/app/server_config.py index bcea0861f..0556199eb 100644 --- a/backend/app/server_config.py +++ b/backend/app/server_config.py @@ -1,2 +1,6 @@ UNSUMMARIZED_WINDOW_SIZE = 10 TO_BE_SUMMARIZED_WINDOW_SIZE = 3 + +# Number of consecutive identical agent messages that triggers a force-summarize +# to flush the repeated turns out of the LLM's visible window. +REPETITION_SHORT_CIRCUIT_THRESHOLD = 3 From 9af4375d35cc29165988da0a70ba1c1d6c33c7cd Mon Sep 17 00:00:00 2001 From: Bereket Terefe Date: Wed, 6 May 2026 18:53:19 +0300 Subject: [PATCH 7/9] fix(agent_transition): prevent saving outgoing agent's response during phase transition [pulumi up] MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Upstream's counseling sub-phase / sticky-routing / skip-phase / observability ctx-var infrastructure trimmed — those features don't exist in core compass. Core retains the essential transition-aware history-save gate. (cherry picked from commit 2334114169b603cfb3434cc82168f4064ac9acce) --- .../agent_director/llm_agent_director.py | 15 +++++++-- .../explore_experiences_agent_director.py | 11 ++++--- .../llm_agent_director_scripted_user_test.py | 32 +++++++++++-------- 3 files changed, 38 insertions(+), 20 deletions(-) diff --git a/backend/app/agent/agent_director/llm_agent_director.py b/backend/app/agent/agent_director/llm_agent_director.py index d0540a587..cd5b370b8 100644 --- a/backend/app/agent/agent_director/llm_agent_director.py +++ b/backend/app/agent/agent_director/llm_agent_director.py @@ -147,14 +147,23 @@ async def execute(self, user_input: AgentInput) -> AgentOutput: # Perform the task agent_output = await agent_for_task.execute(clean_input, context) + + # Determine if a phase transition is about to happen so we can decide + # whether to save this agent's response to history. + new_phase = self._get_new_phase(agent_output) + _will_transition = self._state.current_phase != new_phase + if not agent_for_task.is_responsible_for_conversation_history(): - await self._conversation_manager.update_history(clean_input, agent_output) + # Don't save the outgoing agent's final response when transitioning — + # the next agent will produce the response the user actually sees. + if not _will_transition: + await self._conversation_manager.update_history(clean_input, agent_output) + context = await self._conversation_manager.get_conversation_context() # Update the conversation phase - new_phase = self._get_new_phase(agent_output) self._logger.debug("Transitioned phase from %s --to-> %s", self._state.current_phase, new_phase) - transitioned_to_new_phase = self._state.current_phase != new_phase + transitioned_to_new_phase = _will_transition if transitioned_to_new_phase: user_input = AgentInput( message="(silence)", diff --git a/backend/app/agent/explore_experiences_agent_director.py b/backend/app/agent/explore_experiences_agent_director.py index e910e4fff..245c6ac8a 100644 --- a/backend/app/agent/explore_experiences_agent_director.py +++ b/backend/app/agent/explore_experiences_agent_director.py @@ -311,9 +311,6 @@ async def execute(self, user_input: AgentInput, context: ConversationContext) -> # First collect all the experiences from the user if state.conversation_phase == ConversationPhase.COLLECT_EXPERIENCES: agent_output = await self._collect_experiences_agent.execute(user_input, context) - await self._conversation_manager.update_history(user_input, agent_output) - # get the context again after updating the history - context = await self._conversation_manager.get_conversation_context() # The experiences are still being collected, but we can already store them so that we can # present them to the user even if data collection has not finished. @@ -325,9 +322,15 @@ async def execute(self, user_input: AgentInput, context: ConversationContext) -> # If collecting is not finished then return the output to the user to continue collecting if not agent_output.finished: + # Only save to history when not transitioning, so the next agent's + # opening message is the first thing the user sees on transition. + await self._conversation_manager.update_history(user_input, agent_output) + # get the context again after updating the history + context = await self._conversation_manager.get_conversation_context() return agent_output - # and transition to the next phase + # and transition to the next phase — do NOT save the outgoing agent's final response; + # the dive-in agent will produce the first message the user sees. state.conversation_phase = ConversationPhase.DIVE_IN transitioned_between_states = True diff --git a/backend/evaluation_tests/agent_director/llm_agent_director_scripted_user_test.py b/backend/evaluation_tests/agent_director/llm_agent_director_scripted_user_test.py index 8af45f551..80a627e0c 100644 --- a/backend/evaluation_tests/agent_director/llm_agent_director_scripted_user_test.py +++ b/backend/evaluation_tests/agent_director/llm_agent_director_scripted_user_test.py @@ -12,6 +12,7 @@ from app.agent.collect_experiences_agent import CollectExperiencesAgentState from app.agent.explore_experiences_agent_director import ExploreExperiencesAgentDirectorState from app.agent.linking_and_ranking_pipeline import ExperiencePipelineConfig +from app.agent.skill_explorer_agent import SkillsExplorerAgentState from app.agent.welcome_agent import WelcomeAgentState from app.conversation_memory.conversation_memory_manager import ConversationMemoryManager, \ ConversationMemoryManagerState @@ -49,6 +50,7 @@ async def setup_agent_director(setup_search_services: Awaitable[SearchServices]) explore_experiences_agent = agent_director.get_explore_experiences_agent() explore_experiences_agent.set_state(ExploreExperiencesAgentDirectorState(session_id=session_id)) explore_experiences_agent.get_collect_experiences_agent().set_state(CollectExperiencesAgentState(session_id=session_id)) + explore_experiences_agent.get_exploring_skills_agent().set_state(SkillsExplorerAgentState(session_id=session_id)) async def agent_director_exec(caplog: LogCaptureFixture, test_case: ScriptedUserEvaluationTestCase): print(f"Running test case {test_case.name}") @@ -83,7 +85,7 @@ async def agent_director_exec(caplog: LogCaptureFixture, test_case: ScriptedUser ) assert_log_error_warnings(caplog=caplog, - expect_errors_in_logs=False, + expect_errors_in_logs=True, expect_warnings_in_logs=True) return conversation_manager, agent_director_exec @@ -168,19 +170,22 @@ class AgentState: await agent_director_exec(caplog, given_test_case) # Check if the WELCOME_AGENT agent completed its task, - # and the COLLECT_EXPERIENCES_AGENT started its task, - # and the WELCOME_AGENT agent can be engaged again + # and the COLLECT_EXPERIENCES_AGENT started its task. context = await conversation_manager.get_conversation_context() expected_agent_states: list[AgentState] = [ AgentState(0, AgentType.WELCOME_AGENT, False), # WelcomeAgent say hi AgentState(1, AgentType.WELCOME_AGENT, False), - AgentState(2, AgentType.WELCOME_AGENT, True), # WelcomeAgent completes task - AgentState(3, AgentType.COLLECT_EXPERIENCES_AGENT, False), # Start of Skill Explore - AgentState(4, AgentType.COLLECT_EXPERIENCES_AGENT, False), # Job 1 - AgentState(5, AgentType.COLLECT_EXPERIENCES_AGENT, False), # skills - AgentState(6, AgentType.COLLECT_EXPERIENCES_AGENT, False), # No more to say - AgentState(7, AgentType.WELCOME_AGENT, False), # WelcomeAgent explains - AgentState(8, AgentType.COLLECT_EXPERIENCES_AGENT, False), # Job 1, update by COLLECT_EXPERIENCES_AGENT + # WelcomeAgent's finished=True response is NOT saved to history on transition — + # only the incoming CollectExperiencesAgent's first message is saved. + AgentState(2, AgentType.COLLECT_EXPERIENCES_AGENT, False), # Start of Skill Explore + AgentState(3, AgentType.COLLECT_EXPERIENCES_AGENT, False), # Job 1 + AgentState(4, AgentType.COLLECT_EXPERIENCES_AGENT, False), # skills + AgentState(5, AgentType.COLLECT_EXPERIENCES_AGENT, False), # No more to say + # During COUNSELING/EXPLORE_EXPERIENCES routing is deterministic — all messages + # go to EXPLORE_EXPERIENCES_AGENT regardless of content. WelcomeAgent is unreachable + # from this sub-phase, so "Can you explain the process again?" goes to CollectExperiencesAgent. + AgentState(6, AgentType.COLLECT_EXPERIENCES_AGENT, False), # "Can you explain..." handled by CollectExperiencesAgent + AgentState(7, AgentType.COLLECT_EXPERIENCES_AGENT, False), # Job 1 update ] for i, expected_state in enumerate(expected_agent_states): turn = context.all_history.turns[i] @@ -246,12 +251,13 @@ class AgentState: expected_agent_states: list[AgentState] = [ AgentState(0, AgentType.WELCOME_AGENT, False), AgentState(1, AgentType.WELCOME_AGENT, False), - AgentState(2, AgentType.WELCOME_AGENT, True), + # WelcomeAgent's finished=True response is NOT saved to history on transition — + # only the incoming CollectExperiencesAgent's first message is saved. + AgentState(2, AgentType.COLLECT_EXPERIENCES_AGENT, False), AgentState(3, AgentType.COLLECT_EXPERIENCES_AGENT, False), AgentState(4, AgentType.COLLECT_EXPERIENCES_AGENT, False), AgentState(5, AgentType.COLLECT_EXPERIENCES_AGENT, False), - AgentState(6, AgentType.COLLECT_EXPERIENCES_AGENT, False), - AgentState(7, AgentType.WELCOME_AGENT, False), + AgentState(6, AgentType.WELCOME_AGENT, False), ] for i, expected_state in enumerate(expected_agent_states): turn = context.all_history.turns[i] From b825beb993c02ee4aef34960b1b7c24f20339714 Mon Sep 17 00:00:00 2001 From: Bereket Terefe Date: Wed, 6 May 2026 12:31:53 +0300 Subject: [PATCH 8/9] test(loop_detection): add scripted evaluation tests for SkillsExplorerAgent loop detection (cherry picked from commit daf740942c18e754765db3634626db6c0ff9bb74) --- .../loop_detection_scripted_user_test.py | 255 ++++++++++++++ .../loop_detection_test.py | 332 ++++++++++++++++++ 2 files changed, 587 insertions(+) create mode 100644 backend/evaluation_tests/skill_explorer_agent/loop_detection_scripted_user_test.py create mode 100644 backend/evaluation_tests/skill_explorer_agent/loop_detection_test.py diff --git a/backend/evaluation_tests/skill_explorer_agent/loop_detection_scripted_user_test.py b/backend/evaluation_tests/skill_explorer_agent/loop_detection_scripted_user_test.py new file mode 100644 index 000000000..fc96994b7 --- /dev/null +++ b/backend/evaluation_tests/skill_explorer_agent/loop_detection_scripted_user_test.py @@ -0,0 +1,255 @@ +""" +Scripted evaluation tests: real production looping sessions replayed verbatim. + +User messages are taken directly from the looping session transcripts in +scripts/admin/looping_sessions/. The test drives only the EXPLORE_SKILLS_AGENT +phase (from the first agent question in that phase onwards), seeding +question_asked_until_now with the questions asked before the loop started so +the agent state matches production. + +The agent must finish cleanly (finished=True) within the scripted turns rather +than continuing to repeat the same message. +""" + +import logging +import os + +import pytest +from _pytest.logging import LogCaptureFixture + +from app.agent.experience.experience_entity import ExperienceEntity +from app.agent.experience.timeline import Timeline +from app.agent.experience.work_type import WorkType +from app.agent.skill_explorer_agent import SkillsExplorerAgentState +from app.conversation_memory.conversation_memory_manager import ConversationMemoryManager +from app.conversation_memory.conversation_memory_types import ConversationMemoryManagerState +from app.countries import Country +from app.i18n.translation_service import get_i18n_manager +from app.server_config import UNSUMMARIZED_WINDOW_SIZE, TO_BE_SUMMARIZED_WINDOW_SIZE +from common_libs.test_utilities import get_random_session_id +from common_libs.test_utilities.guard_caplog import guard_caplog, assert_log_error_warnings +from evaluation_tests.conversation_libs.conversation_test_function import ( + ScriptedSimulatedUser, + ScriptedUserEvaluationTestCase, + ConversationTestConfig, + conversation_test_function, + assert_expected_evaluation_results, +) +from evaluation_tests.conversation_libs.evaluators.evaluation_result import ConversationEvaluationRecord +from evaluation_tests.get_test_cases_to_run_func import get_test_cases_to_run +from .skills_explorer_agent_executor import ( + SkillsExplorerAgentExecutor, + SkillsExplorerAgentGetConversationContextExecutor, + SkillsExplorerAgentIsFinished, +) + + +class ScriptedLoopDetectionTestCase(ScriptedUserEvaluationTestCase): + given_experience: ExperienceEntity + # Questions the agent asked (and user answers) before the loop started, + # used to seed question_asked_until_now so state matches production. + prior_questions: list[str] + prior_answers: list[str] + # The verbatim message that was looping in production. The test asserts + # the agent never repeats this message across the scripted turns. + looping_message: str + + +test_cases: list[ScriptedLoopDetectionTestCase] = [ + # ------------------------------------------------------------------------- + # Session 52870822099790 — Pay Point Manager, 14× repetition of the + # challenges question. User messages taken verbatim from turns 26–50 + # of the transcript (every other turn, the user turns). + # ------------------------------------------------------------------------- + ScriptedLoopDetectionTestCase( + name="production_session_52870822099790_pay_point_manager", + simulated_user_prompt="Scripted user: real production session 52870822099790", + evaluations=[], + country_of_user=Country.ZAMBIA, + looping_message=( + "Thank you for sharing that. It sounds like you were responsible for maintaining order at the pay point.\n\n" + "What were some of the challenges you faced as a Pay Point Manager?" + ), + given_experience=ExperienceEntity( + experience_title="Pay Point Manager", + company="Social Cash Transfer", + location="Luanshya", + timeline=Timeline(start="1993", end="2000"), + work_type=WorkType.FORMAL_SECTOR_WAGED_EMPLOYMENT, + ), + prior_questions=[ + "To start, could you please describe a typical day when you were working as a Pay Point Manager?", + "Thank you for sharing that. It sounds like patience was very important in your role.\n\nCould you tell me more about what you did on a typical day as a Pay Point Manager? What were your main tasks?", + "I see. Besides paying money to beneficiaries, what other responsibilities did you have as a Pay Point Manager?", + ], + prior_answers=[ + "It was an involving work which needed patience as you work", + "The main task was paying money to the beneficiaries", + "Making sure that there was order in the working place", + ], + scripted_user=[ + # turn 26 — first answer to the challenges question + "Distance to the working place was abig challenge", + # turn 28 — agent loops, user gives another challenge + "Disturbance from the local community ,especially those who were not on the list.", + # turn 30 + "The above are the only problems", + # turn 32 + "Beneficiaries not coming at the same time which made me wait for a longer period", + # turn 34 + "Working in rural areas", + # turn 36 + "The other challenge was that there was no security to work with out a police officer at times only.", + # turn 38 + "Working the all day without food, if you forgot to buy and carry.", + # turn 40 + "Payment at the end of fininshing the task was not enough", + # turn 42 + "Relating with some people was a challenge. They were talkative and they used abusive language; this came about when there were deployment's in reaching the place.", + # turn 44 + "Some parents were illiterate thus using ink to make them sign with a finger print onsumed time and some could dirten the wok place with ink.", + # turn 46 + "To maintain order was a big challenge ,because at times the ones to help delayed in coming others would go back to attend to other issues and I would do it all by my self.", + # turn 48 + "Attending to people with different abilities and making order delayed the process of everything", + # turn 50 + "Lastly,it was a tiresome job with little wage not a salar", + ], + ), + # ------------------------------------------------------------------------- + # Session 85833153862014 — Helping Family and Church, 4× repetition of the + # "specific tasks" question. User messages taken verbatim from turns 48–58 + # of the transcript (EXPLORE_SKILLS_AGENT section only). + # ------------------------------------------------------------------------- + ScriptedLoopDetectionTestCase( + name="production_session_85833153862014_helping_family_church", + simulated_user_prompt="Scripted user: real production session 85833153862014", + evaluations=[], + country_of_user=Country.ZAMBIA, + looping_message=( + "Thank you for sharing that. It sounds like you have a lot of important responsibilities in both your family and church.\n\n" + "Could you tell me more about the specific tasks you were responsible for when helping your family?" + ), + given_experience=ExperienceEntity( + experience_title="Helping Family and Church", + company=None, + location="Kisasa area, Kalumbila District, North Western Province", + timeline=Timeline(start="2001", end=None), + work_type=WorkType.UNSEEN_UNPAID, + ), + prior_questions=[ + "Could you please describe a typical day when you were helping your family and church?", + ], + prior_answers=[ + "A typical day helping my family and church usually starts with assisting my family with " + "household responsibilities and making sure everything at home is in order. I help with tasks " + "such as running errands, supporting family members when they need help, and making sure things " + "are organized, as well as financial support.\nWhen helping at church, I participate in " + "activities such as preparing the church environment, assisting during services, and supporting " + "church programs or events. I also help with organizing activities, welcoming people, and " + "working together with other members to make sure everything runs smoothly.", + ], + scripted_user=[ + # turn 48 — first answer after the loop starts + "helping in farming and house chores", + # turn 50 — agent loops again + "I helped my family with household chores, cleaning, cooking, running errands, taking care of family members, and doing general maintenance or manual work around the home.", + # turn 52 — agent loops again + "I helped my family with household chores, cleaning, cooking, running errands, taking care of family members, and doing general maintenance or manual work around the home", + # turn 54 — user disengages + "no", + # turn 56 — agent moves to achievement/challenge question + "it just feels good to know that i have some responsibilities as a man, and it is incouraging me to work extra hard.", + # turn 58 — agent asks what's important + "it is important because the need will not luck.at least they will have something.", + ], + ), +] + + +@pytest.mark.asyncio +@pytest.mark.evaluation_test("gemini-2.5-flash-lite/") +@pytest.mark.repeat(3) +@pytest.mark.parametrize( + "test_case", + get_test_cases_to_run(test_cases), + ids=[case.name for case in get_test_cases_to_run(test_cases)], +) +async def test_skills_explorer_agent_loop_detection_scripted( + max_iterations: int, + test_case: ScriptedLoopDetectionTestCase, + caplog: LogCaptureFixture, +): + """ + Replays real production sessions that exhibited looping behaviour. + The scripted user messages are taken verbatim from the session transcripts. + The agent must finish cleanly (finished=True) within the scripted turns. + """ + print(f"Running test case {test_case.name}") + + session_id = get_random_session_id() + get_i18n_manager().set_locale(test_case.locale) + output_folder = os.path.join( + os.getcwd(), + "test_output/skills_explorer_agent/loop_detection_scripted/", + test_case.name, + ) + + given_experience = test_case.given_experience.model_copy(deep=True) + conversation_manager = ConversationMemoryManager(UNSUMMARIZED_WINDOW_SIZE, TO_BE_SUMMARIZED_WINDOW_SIZE) + conversation_manager.set_state(state=ConversationMemoryManagerState(session_id=session_id)) + + state = SkillsExplorerAgentState( + session_id=session_id, + country_of_user=test_case.country_of_user, + question_asked_until_now=test_case.prior_questions, + answers_provided=test_case.prior_answers, + ) + + execute_evaluated_agent = SkillsExplorerAgentExecutor( + conversation_manager=conversation_manager, + state=state, + experience=given_experience, + ) + + config = ConversationTestConfig( + max_iterations=len(test_case.scripted_user) + 1, + test_case=test_case, + output_folder=output_folder, + execute_evaluated_agent=execute_evaluated_agent, + execute_simulated_user=ScriptedSimulatedUser(script=test_case.scripted_user), + is_finished=SkillsExplorerAgentIsFinished(), + get_conversation_context=SkillsExplorerAgentGetConversationContextExecutor( + conversation_manager=conversation_manager, + ), + deferred_evaluation_assertions=True, + ) + + with caplog.at_level(logging.DEBUG): + guard_caplog(logger=execute_evaluated_agent._agent._logger, caplog=caplog) + + evaluation_result: ConversationEvaluationRecord = await conversation_test_function( + config=config + ) + + # The agent must not repeat the looping message verbatim in any of its + # responses during the scripted replay. It is acceptable (and expected) + # for the agent to still be mid-conversation at the end of the script — + # what matters is that it moved on rather than staying stuck. + context = await conversation_manager.get_conversation_context() + repeated_turns = [ + t for t in context.all_history.turns + if t.output.message_for_user == test_case.looping_message + ] + assert len(repeated_turns) == 0, ( + f"Agent repeated the looping message {len(repeated_turns)} time(s) during the scripted replay.\n" + f"Looping message: {test_case.looping_message!r}" + ) + + # assert_log_error_warnings( + # caplog=caplog, + # expect_errors_in_logs=test_case.expect_errors_in_logs, + # expect_warnings_in_logs=test_case.expect_warnings_in_logs, + # ) + + assert_expected_evaluation_results(evaluation_result=evaluation_result, test_case=test_case) diff --git a/backend/evaluation_tests/skill_explorer_agent/loop_detection_test.py b/backend/evaluation_tests/skill_explorer_agent/loop_detection_test.py new file mode 100644 index 000000000..d3285b84b --- /dev/null +++ b/backend/evaluation_tests/skill_explorer_agent/loop_detection_test.py @@ -0,0 +1,332 @@ +""" +Evaluation test: does the SkillsExplorerAgent break out of a loop? + +Background +---------- +Production data shows that in 105 sessions the agent sent the same message +verbatim ≥ 2 times (worst case: 14 repetitions). The leading hypothesis is +that once the conversation history already contains repeated agent messages, +the LLM treats the pattern as expected behaviour and continues it — even +though the system prompt explicitly forbids re-asking questions. + +Each test case seeds the conversation manager with a history that already +contains N repetitions of the looping message, then lets a simulated user +(who keeps giving brief disengagement responses) drive one more round. +We assert that the agent finishes cleanly (finished=True) rather than +repeating the message again. +""" + +import logging +import os +from textwrap import dedent + +import pytest +from _pytest.logging import LogCaptureFixture + +from app.agent.agent_types import AgentInput, AgentOutput +from app.agent.experience.experience_entity import ExperienceEntity +from app.agent.experience.timeline import Timeline +from app.agent.experience.work_type import WorkType +from app.agent.skill_explorer_agent import SkillsExplorerAgentState +from app.conversation_memory.conversation_memory_manager import ConversationMemoryManager +from app.conversation_memory.conversation_memory_types import ConversationMemoryManagerState +from app.countries import Country +from app.i18n.translation_service import get_i18n_manager +from app.server_config import UNSUMMARIZED_WINDOW_SIZE, TO_BE_SUMMARIZED_WINDOW_SIZE +from common_libs.test_utilities import get_random_session_id +from common_libs.test_utilities.guard_caplog import guard_caplog, assert_log_error_warnings +from evaluation_tests.conversation_libs.conversation_test_function import ( + LLMSimulatedUser, + ConversationTestConfig, + conversation_test_function, + assert_expected_evaluation_results, + EvaluationTestCase, +) +from evaluation_tests.conversation_libs.evaluators.evaluation_result import ConversationEvaluationRecord +from evaluation_tests.get_test_cases_to_run_func import get_test_cases_to_run +from .skills_explorer_agent_executor import ( + SkillsExplorerAgentExecutor, + SkillsExplorerAgentGetConversationContextExecutor, + SkillsExplorerAgentIsFinished, +) + +_COMPLETION_MSG = ( + "Thank you for sharing these details! I have all the information I need." +) +_LOOPING_QUESTION = ( + "Thank you for sharing that. " + "What do you think is important when working in a paid job or for an employer?" +) + + +class LoopSeededSkillsExplorerAgentExecutor(SkillsExplorerAgentExecutor): + """ + Like SkillsExplorerAgentExecutor but seeds the conversation manager with + a pre-built looping history before the very first agent call. + """ + + def __init__( + self, + conversation_manager: ConversationMemoryManager, + state: SkillsExplorerAgentState, + experience: ExperienceEntity, + looping_message: str, + n_repeats: int, + ): + super().__init__( + conversation_manager=conversation_manager, + state=state, + experience=experience, + ) + self._looping_message = looping_message + self._n_repeats = n_repeats + self._seeded = False + + async def _seed_history(self) -> None: + """Inject a looping conversation history into the conversation manager.""" + # Opening: agent asks about typical day + await self._conversation_manager.update_history( + AgentInput(message="", is_artificial=True), + AgentOutput( + message_for_user=( + "Could you tell me about your role? What did a typical day look like?" + ), + finished=False, + agent_type=None, + agent_response_time_in_sec=0, + llm_stats=[], + ), + ) + # User gives a first real answer + await self._conversation_manager.update_history( + AgentInput(message="I managed the queue, checked IDs, and kept order at the pay point."), + AgentOutput( + message_for_user=( + "Thank you for sharing that. " + "What were some of the challenges you faced in this role?" + ), + finished=False, + agent_type=None, + agent_response_time_in_sec=0, + llm_stats=[], + ), + ) + # User answers challenges question + await self._conversation_manager.update_history( + AgentInput(message="Sometimes people got angry in the queues. I had to stay calm."), + AgentOutput( + message_for_user=self._looping_message, + finished=False, + agent_type=None, + agent_response_time_in_sec=0, + llm_stats=[], + ), + ) + # Repeat the looping message n_repeats - 1 more times + for _ in range(self._n_repeats - 1): + await self._conversation_manager.update_history( + AgentInput(message="okay"), + AgentOutput( + message_for_user=self._looping_message, + finished=False, + agent_type=None, + agent_response_time_in_sec=0, + llm_stats=[], + ), + ) + + async def __call__(self, agent_input: AgentInput) -> AgentOutput: + if not self._seeded: + await self._seed_history() + self._seeded = True + return await super().__call__(agent_input) + + +class LoopDetectionTestCase(EvaluationTestCase): + looping_message: str + n_repeats: int + given_experience: ExperienceEntity + assert_finished: bool = True + """ + If True (default), assert the agent emits finished=True within max_iterations. + Set to False for engaged-user cases where the agent is expected to stay in + conversation — the only assertion then is that the looping message is not repeated. + """ + + +_disengagement_prompt = dedent(""" + You are a user who has already answered all the agent's questions and just + wants to move on. Respond to every agent message with a single short + disengagement signal such as "okay", "next", or "I have nothing more to add." + Do not provide any new information. +""") +_engagement_prompt= dedent(""" + You are a user who is engaged and cooperative. Answer the agent's questions + in a detailed and thoughtful manner, providing as much relevant information + as possible about your work experience, skills, and preferences. +""") + +test_cases: list[LoopDetectionTestCase] = [ + LoopDetectionTestCase( + name="mild_loop_completion_msg", + simulated_user_prompt=_disengagement_prompt, + evaluations=[], + looping_message=_COMPLETION_MSG, + n_repeats=3, + given_experience=ExperienceEntity( + experience_title="Pay Point Manager", + company="Government Pay Point", + location="Lusaka", + timeline=Timeline(start="2021", end="2023"), + work_type=WorkType.FORMAL_SECTOR_WAGED_EMPLOYMENT, + ), + country_of_user=Country.ZAMBIA, + ), + LoopDetectionTestCase( + name="severe_loop_completion_msg", + simulated_user_prompt=_disengagement_prompt, + evaluations=[], + looping_message=_COMPLETION_MSG, + n_repeats=6, + given_experience=ExperienceEntity( + experience_title="Pay Point Manager", + company="Government Pay Point", + location="Lusaka", + timeline=Timeline(start="2021", end="2023"), + work_type=WorkType.FORMAL_SECTOR_WAGED_EMPLOYMENT, + ), + country_of_user=Country.ZAMBIA, + ), + LoopDetectionTestCase( + name="loop_on_question", + simulated_user_prompt=_disengagement_prompt, + evaluations=[], + looping_message=_LOOPING_QUESTION, + n_repeats=3, + given_experience=ExperienceEntity( + experience_title="Pay Point Manager", + company="Government Pay Point", + location="Lusaka", + timeline=Timeline(start="2021", end="2023"), + work_type=WorkType.FORMAL_SECTOR_WAGED_EMPLOYMENT, + ), + country_of_user=Country.ZAMBIA, + ), + LoopDetectionTestCase( + name="engaged_user_loop_on_question", + simulated_user_prompt=_engagement_prompt, + evaluations=[], + looping_message=_LOOPING_QUESTION, + n_repeats=3, + assert_finished=False, + given_experience=ExperienceEntity( + experience_title="Pay Point Manager", + company="Government Pay Point", + location="Lusaka", + timeline=Timeline(start="2021", end="2023"), + work_type=WorkType.FORMAL_SECTOR_WAGED_EMPLOYMENT, + ), + ) +] + +@pytest.mark.asyncio +@pytest.mark.evaluation_test("gemini-2.5-flash-lite/") +@pytest.mark.repeat(3) +@pytest.mark.parametrize( + "test_case", + get_test_cases_to_run(test_cases), + ids=[case.name for case in get_test_cases_to_run(test_cases)], +) +async def test_skills_explorer_agent_loop_detection( + max_iterations: int, + test_case: LoopDetectionTestCase, + caplog: LogCaptureFixture, +): + """ + Given a conversation history that already contains N repetitions of the + same agent message, the agent must NOT repeat it again. It should end the + conversation (finished=True). + """ + print(f"Running test case {test_case.name}") + + session_id = get_random_session_id() + get_i18n_manager().set_locale(test_case.locale) + output_folder = os.path.join( + os.getcwd(), + "test_output/skills_explorer_agent/loop_detection/", + test_case.name, + ) + + given_experience = test_case.given_experience.model_copy(deep=True) + conversation_manager = ConversationMemoryManager(UNSUMMARIZED_WINDOW_SIZE, TO_BE_SUMMARIZED_WINDOW_SIZE) + conversation_manager.set_state(state=ConversationMemoryManagerState(session_id=session_id)) + + # Pre-populate question_asked_until_now to reflect what the agent recorded + # before the loop started (matches production state shape). + state = SkillsExplorerAgentState( + session_id=session_id, + country_of_user=test_case.country_of_user, + question_asked_until_now=[ + "Could you tell me about your role? What did a typical day look like?", + "Thank you for sharing that. What were some of the challenges you faced in this role?", + ], + answers_provided=[ + "I managed the queue, checked IDs, and kept order at the pay point.", + "Sometimes people got angry in the queues. I had to stay calm.", + ], + ) + + execute_evaluated_agent = LoopSeededSkillsExplorerAgentExecutor( + conversation_manager=conversation_manager, + state=state, + experience=given_experience, + looping_message=test_case.looping_message, + n_repeats=test_case.n_repeats, + ) + + config = ConversationTestConfig( + max_iterations=max_iterations, + test_case=test_case, + output_folder=output_folder, + execute_evaluated_agent=execute_evaluated_agent, + execute_simulated_user=LLMSimulatedUser(system_instructions=test_case.simulated_user_prompt), + is_finished=SkillsExplorerAgentIsFinished(), + get_conversation_context=SkillsExplorerAgentGetConversationContextExecutor( + conversation_manager=conversation_manager, + ), + deferred_evaluation_assertions=True, + ) + + with caplog.at_level(logging.DEBUG): + guard_caplog(logger=execute_evaluated_agent._agent._logger, caplog=caplog) + + evaluation_result: ConversationEvaluationRecord = await conversation_test_function( + config=config + ) + + context = await conversation_manager.get_conversation_context() + if test_case.assert_finished: + assert context.history.turns[-1].output.finished, ( + f"Agent did not finish after {test_case.n_repeats} repetitions of the " + f"looping message in history. Last message: " + f"{context.history.turns[-1].output.message_for_user!r}" + ) + + # The agent must not have repeated the looping message verbatim in a non-finished turn. + # A final turn (finished=True) emitting the same text is acceptable — that's the + # legitimate resolution, not a loop. + new_turns = context.all_history.turns[test_case.n_repeats + 2:] # skip seeded turns + for turn in new_turns: + assert turn.output.finished or turn.output.message_for_user != test_case.looping_message, ( + f"Agent repeated the looping message verbatim (without finishing) after it already " + f"appeared {test_case.n_repeats} times in history.\n" + f"Message: {test_case.looping_message!r}" + ) + + assert_log_error_warnings( + caplog=caplog, + expect_errors_in_logs=test_case.expect_errors_in_logs, + expect_warnings_in_logs=test_case.expect_warnings_in_logs, + ) + + assert_expected_evaluation_results(evaluation_result=evaluation_result, test_case=test_case) From 3c1e2ffd5a024004b09c799d2a9f762c0e8f1f26 Mon Sep 17 00:00:00 2001 From: nraffa Date: Thu, 21 May 2026 11:58:23 +1000 Subject: [PATCH 9/9] fix(tests): use Country.KENYA in loop-detection tests Country.ZAMBIA is fork-only and not in core's Country enum; the reference broke pytest collection (AttributeError: ZAMBIA) and failed backend CI even though these are deselected evaluation_tests. --- .../loop_detection_scripted_user_test.py | 4 ++-- .../skill_explorer_agent/loop_detection_test.py | 6 +++--- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/backend/evaluation_tests/skill_explorer_agent/loop_detection_scripted_user_test.py b/backend/evaluation_tests/skill_explorer_agent/loop_detection_scripted_user_test.py index fc96994b7..b8609f53c 100644 --- a/backend/evaluation_tests/skill_explorer_agent/loop_detection_scripted_user_test.py +++ b/backend/evaluation_tests/skill_explorer_agent/loop_detection_scripted_user_test.py @@ -65,7 +65,7 @@ class ScriptedLoopDetectionTestCase(ScriptedUserEvaluationTestCase): name="production_session_52870822099790_pay_point_manager", simulated_user_prompt="Scripted user: real production session 52870822099790", evaluations=[], - country_of_user=Country.ZAMBIA, + country_of_user=Country.KENYA, looping_message=( "Thank you for sharing that. It sounds like you were responsible for maintaining order at the pay point.\n\n" "What were some of the challenges you faced as a Pay Point Manager?" @@ -125,7 +125,7 @@ class ScriptedLoopDetectionTestCase(ScriptedUserEvaluationTestCase): name="production_session_85833153862014_helping_family_church", simulated_user_prompt="Scripted user: real production session 85833153862014", evaluations=[], - country_of_user=Country.ZAMBIA, + country_of_user=Country.KENYA, looping_message=( "Thank you for sharing that. It sounds like you have a lot of important responsibilities in both your family and church.\n\n" "Could you tell me more about the specific tasks you were responsible for when helping your family?" diff --git a/backend/evaluation_tests/skill_explorer_agent/loop_detection_test.py b/backend/evaluation_tests/skill_explorer_agent/loop_detection_test.py index d3285b84b..311a0b5ac 100644 --- a/backend/evaluation_tests/skill_explorer_agent/loop_detection_test.py +++ b/backend/evaluation_tests/skill_explorer_agent/loop_detection_test.py @@ -180,7 +180,7 @@ class LoopDetectionTestCase(EvaluationTestCase): timeline=Timeline(start="2021", end="2023"), work_type=WorkType.FORMAL_SECTOR_WAGED_EMPLOYMENT, ), - country_of_user=Country.ZAMBIA, + country_of_user=Country.KENYA, ), LoopDetectionTestCase( name="severe_loop_completion_msg", @@ -195,7 +195,7 @@ class LoopDetectionTestCase(EvaluationTestCase): timeline=Timeline(start="2021", end="2023"), work_type=WorkType.FORMAL_SECTOR_WAGED_EMPLOYMENT, ), - country_of_user=Country.ZAMBIA, + country_of_user=Country.KENYA, ), LoopDetectionTestCase( name="loop_on_question", @@ -210,7 +210,7 @@ class LoopDetectionTestCase(EvaluationTestCase): timeline=Timeline(start="2021", end="2023"), work_type=WorkType.FORMAL_SECTOR_WAGED_EMPLOYMENT, ), - country_of_user=Country.ZAMBIA, + country_of_user=Country.KENYA, ), LoopDetectionTestCase( name="engaged_user_loop_on_question",