fix: address PR review comments for stdio discovery

- Add proper error handling for non-Error objects
- Define MANIFEST_PREFIX and MANIFEST_ERROR_PREFIX constants
- Fix HTTP discovery retry logic regression in tests
- Make regex patterns multiline-safe for manifest and error parsing
- Add timeout handling for stdio discovery in tests

Author: taeold

.aider.chat.history.md

@@ -0,0 +1,94 @@
+# aider chat started at 2025-04-25 16:13:46
+
+> Newer aider version v0.82.2 is available.  
+> /Users/danielylee/.local/share/uv/tools/aider-chat/bin/python3 -m pip install --upgrade --upgrade-strategy only-if-needed aider-chat  
+> Run pip install? (Y)es/(N)o [Yes]: y  
+> Re-run aider to use new version.
+
+# aider chat started at 2025-04-25 16:14:09
+
+> You can skip this check with --no-gitignore  
+> Add .aider\* to .gitignore (recommended)? (Y)es/(N)o [Yes]: n  
+> /Users/danielylee/.local/bin/aider  
+> Using gemini/gemini-2.5-pro-exp-03-25 model with API key from environment.  
+> Aider v0.82.2  
+> Main model: gemini/gemini-2.5-pro-exp-03-25 with diff-fenced edit format  
+> Weak model: gemini/gemini-2.0-flash  
+> Git repo: .git with 272 files  
+> Repo-map: using 4096 tokens, auto refresh  
+> https://aider.chat/HISTORY.html#release-notes  
+> Would you like to see what's new in this version? (Y)es/(N)o [Yes]: y
+>
+> ^C again to exit
+
+#### /add src/common/providers/https.ts
+
+> Added src/common/providers/https.ts to the chat
+
+#### I've received github issue like this:
+
+####
+
+#### I am testing the new streaming API for cloud functions in emulator.
+
+#### Aborting streams with an AbortController on my client works fine.
+
+####
+
+#### How can the called function know that the user canceled the stream? The CallableResponse signal does not fire an event.
+
+####
+
+#### I want to pass an abort signal to the AI API I am calling in the function.
+
+####
+
+#### export const aiStreamText = onCall<AiStreamTextRequest>(
+
+#### async (request, response) => {
+
+#### response?.signal.addEventListener("abort", () => {
+
+#### console.log(new Date().toISOString(), "on server: aborted by user"); // THIS DOES NOT FIRE ON ABORT
+
+#### });
+
+####
+
+#### This logic should be available in @/src/common/providers/https.ts 's wrapOnCallHandler function. I'm curious why this might not be working - can you investigate the code and come up with understanding why this signal may not be triggering and abort event.
+
+> Initial repo scan can be slow in larger repos, but only happens once.
+
+Okay, I've analyzed the `wrapOnCallHandler` function in `src/common/providers/https.ts` regarding the `AbortSignal` behavior.
+
+Here's the relevant flow:
+
+1.  An `AbortController` instance is created within `wrapOnCallHandler`.
+2.  An event listener is attached to the `close` event of the underlying Express `response` object (`res`).
+3.  When the `close` event fires (which should happen when the client disconnects), this listener calls `abortController.abort()`.
+4.  The `signal` property of this `abortController` is passed to your handler function via the `response` argument (`response.signal`).
+5.  Calling `abortController.abort()` should indeed trigger the `abort` event on its corresponding `signal`.
+
+Based on the code, the intention is exactly as you expect: the `response.signal` should emit an `abort` event when the client connection closes.
+
+If the `abort` event listener is not firing specifically in the emulator environment when the client aborts an SSE stream, the most likely reasons are:
+
+1.  **Emulator Event Handling:** The Firebase emulator might not be perfectly simulating the network conditions or not reliably triggering the `res.on('close', ...)` event handler when an SSE client disconnects abruptly. This seems the most probable cause if the code works correctly in a deployed environment.
+2.  **Timing:** While less likely given your setup, if the handler function were to complete _before_ the `close` event is processed by Node.js/Express, the listener might not be active anymore. However, attaching the listener inside the async handler should prevent this.
+
+The logic within `wrapOnCallHandler` itself appears correct for propagating the abort signal based on the `response`'s `close` event. The issue you're observing likely stems from how the emulator handles or surfaces that `close` event for streaming connections.
+
+Since the SDK code seems logically correct for this mechanism, I cannot propose a specific code change within `https.ts` to fix what appears to be an emulator-specific behavior issue. You might want to check the Firebase emulator's GitHub repository for existing issues related to SSE connection handling or report this behavior if it's not already documented.
+
+> Tokens: 14k sent, 460 received.
+>
+> ^C again to exit
+>
+> ^C KeyboardInterrupt
+
+# aider chat started at 2025-05-07 11:25:45
+
+> Newer aider version v0.82.3 is available.  
+> /Users/danielylee/.local/share/uv/tools/aider-chat/bin/python3 -m pip install --upgrade --upgrade-strategy only-if-needed aider-chat  
+> Run pip install? (Y)es/(N)o [Yes]: y  
+> Re-run aider to use new version.

.aider.input.history

@@ -0,0 +1,27 @@
+
+# 2025-04-25 16:13:49.951230
++y
+
+# 2025-04-25 16:14:10.653143
++n
+
+# 2025-04-25 16:14:49.374087
++/add src/common/providers/https.ts
+
+# 2025-04-25 16:14:50.670790
++I've received github issue like this:
++
++I am testing the new streaming API for cloud functions in emulator.
++Aborting streams with an AbortController on my client works fine.
++
++How can the called function know that the user canceled the stream? The CallableResponse signal does not fire an event.
++
++I want to pass an abort signal to the AI API I am calling in the function.
++
++export const aiStreamText = onCall<AiStreamTextRequest>(
++  async (request, response) => {
++    response?.signal.addEventListener("abort", () => {
++      console.log(new Date().toISOString(), "on server: aborted by user");  // THIS DOES NOT FIRE ON ABORT
++    });
++
++This logic should be available in @/src/common/providers/https.ts 's wrapOnCallHandler function. I'm curious why this might not be working - can you investigate the code and come up with understanding why this signal may not be triggering and abort event.

.aider.tags.cache.v4/b0/57/2b826a2b7242c99664a33e3c130e.val

@@ -0,0 +1,30 @@
+[
+  {
+    "sessionId": 1748973879,
+    "messageId": 0,
+    "type": "user",
+    "message": "I have a EventHandlerOptions somewhere in the codebase where it is missing documentation for eventFilters and eventFilterPathPatterns. Can you take a look and help come up with a reasonable documentation? Here's tangent documentation from EventArc which is the underlying thing that powers the event driven functions for reference: EventFilter\nFilters events based on exact matches on the CloudEvents attributes.\n\nJSON representation\n\n{\n  \"attribute\": string,\n  \"value\": string,\n  \"operator\": string\n}\nFields\nattribute\t\nstring\n\nRequired. The name of a CloudEvents attribute. Currently, only a subset of attributes are supported for filtering. You can retrieve a specific provider's supported event types.\n\nAll triggers MUST provide a filter for the 'type' attribute.\n\nvalue\t\nstring\n\nRequired. The value for the attribute.\n\noperator\t\nstring\n\nOptional. The operator used for matching the events with the value of the filter. If not specified, only events that have an exact key-value pair specified in the filter are matched. The allowed values are path_pattern and match-path-pattern. path_pattern is only allowed for GCFv1 triggers.",
+    "timestamp": "2025-06-03T18:06:50.454Z"
+  },
+  {
+    "sessionId": 1748978654,
+    "messageId": 0,
+    "type": "user",
+    "message": "sadasasdsa\\",
+    "timestamp": "2025-06-03T19:24:15.923Z"
+  },
+  {
+    "sessionId": 1748978654,
+    "messageId": 1,
+    "type": "user",
+    "message": "dasda\\",
+    "timestamp": "2025-06-03T19:24:21.455Z"
+  },
+  {
+    "sessionId": 1748978654,
+    "messageId": 2,
+    "type": "user",
+    "message": "sdsadasd\\",
+    "timestamp": "2025-06-03T19:24:27.651Z"
+  }
+]

.aider.tags.cache.v4/cache.db

@@ -163,7 +163,12 @@ async function runHttpDiscovery(modulePath: string): Promise<DiscoveryResult> {
         return true;
       } catch (e: unknown) {
         const error = e as { code?: string };
-        return error.code !== "ECONNREFUSED";
+        if (error.code === "ECONNREFUSED") {
+          // This is an expected error during server startup, so we should retry.
+          return false;
+        }
+        // Any other error is unexpected and should fail the test immediately.
+        throw e;
       }
     }, TIMEOUT_L);
 
@@ -199,9 +204,15 @@ async function runStdioDiscovery(modulePath: string): Promise<DiscoveryResult> {
       stderr += chunk.toString("utf8");
     });
 
+    const timeoutId = setTimeout(() => {
+      proc.kill(9);
+      reject(new Error("Stdio discovery timed out after " + TIMEOUT_M + "ms"));
+    }, TIMEOUT_M);
+
     proc.on("close", () => {
+      clearTimeout(timeoutId);
       // Try to parse manifest
-      const manifestMatch = stderr.match(/__FIREBASE_FUNCTIONS_MANIFEST__:(.+)/);
+      const manifestMatch = stderr.match(/__FIREBASE_FUNCTIONS_MANIFEST__:([\s\S]+)/);
       if (manifestMatch) {
         const base64 = manifestMatch[1];
         const manifestJson = Buffer.from(base64, "base64").toString("utf8");
@@ -211,7 +222,7 @@ async function runStdioDiscovery(modulePath: string): Promise<DiscoveryResult> {
       }
 
       // Try to parse error
-      const errorMatch = stderr.match(/__FIREBASE_FUNCTIONS_MANIFEST_ERROR__:(.+)/);
+      const errorMatch = stderr.match(/__FIREBASE_FUNCTIONS_MANIFEST_ERROR__:([\s\S]+)/);
       if (errorMatch) {
         resolve({ success: false, error: errorMatch[1] });
         return;
@@ -221,6 +232,7 @@ async function runStdioDiscovery(modulePath: string): Promise<DiscoveryResult> {
     });
 
     proc.on("error", (err) => {
+      clearTimeout(timeoutId);
       reject(err);
     });
   });

.gemini/logs.json

@@ -0,0 +1,23 @@
+// The MIT License (MIT)
+//
+// Copyright (c) 2025 Firebase
+//
+// Permission is hereby granted, free of charge, to any person obtaining a copy
+// of this software and associated documentation files (the "Software"), to deal
+// in the Software without restriction, including without limitation the rights
+// to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+// copies of the Software, and to permit persons to whom the Software is
+// furnished to do so, subject to the following conditions:
+//
+// The above copyright notice and this permission notice shall be included in all
+// copies or substantial portions of the Software.
+//
+// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
+// SOFTWARE.
+export const GCLOUD_PROJECT = "projct";
+process.env.GCLOUD_PROJECT = GCLOUD_PROJECT;

scripts/bin-test/test.ts

@@ -0,0 +1,110 @@
+// The MIT License (MIT)
+//
+// Copyright (c) 2025 Firebase
+//
+// Permission is hereby granted, free of charge, to any person obtaining a copy
+// of this software and associated documentation files (the "Software"), to deal
+// in the Software without restriction, including without limitation the rights
+// to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+// copies of the Software, and to permit persons to whom the Software is
+// furnished to do so, subject to the following conditions:
+//
+// The above copyright notice and this permission notice shall be included in all
+// copies or substantial portions of the Software.
+//
+// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
+// SOFTWARE.
+
+import { expect } from "chai";
+import * as sinon from "sinon";
+
+import * as params from "../../src/params";
+import * as options from "../../src/v2/options";
+import { ResetValue, RESET_VALUE } from "../../src/common/options";
+import { GCLOUD_PROJECT } from "../init";
+
+describe("optionsToEndpoint", () => {
+  afterEach(() => {
+    sinon.restore();
+  });
+
+  it("should convert serviceAccount in shorthand form with @ to full email", () => {
+    const opts: options.GlobalOptions = {
+      serviceAccount: "my-service@",
+    };
+
+    const result = options.optionsToEndpoint(opts);
+
+    expect(result.serviceAccountEmail).to.equal(
+      `my-service@${GCLOUD_PROJECT}.iam.gserviceaccount.com`
+    );
+  });
+
+  it("should pass through serviceAccount that already has full email format", () => {
+    const opts: options.GlobalOptions = {
+      serviceAccount: "[email protected]",
+    };
+
+    const result = options.optionsToEndpoint(opts);
+
+    expect(result.serviceAccountEmail).to.equal("[email protected]");
+  });
+
+  it("should convert 'default' serviceAccount to null", () => {
+    const opts: options.GlobalOptions = {
+      serviceAccount: "default",
+    };
+
+    const result = options.optionsToEndpoint(opts);
+
+    expect(result.serviceAccountEmail).to.be.null;
+  });
+
+  it("should throw error when using shorthand without GCLOUD_PROJECT", () => {
+    delete process.env.GCLOUD_PROJECT;
+    const opts: options.GlobalOptions = {
+      serviceAccount: "my-service@",
+    };
+
+    expect(() => options.optionsToEndpoint(opts)).to.throw(
+      "Unable to determine email for service account 'my-service@' because process.env.GCLOUD_PROJECT is not set."
+    );
+    process.env.GCLOUD_PROJECT = GCLOUD_PROJECT;
+  });
+
+  it("should throw error for invalid serviceAccount format", () => {
+    const opts: options.GlobalOptions = {
+      serviceAccount: "invalid-format",
+    };
+
+    expect(() => options.optionsToEndpoint(opts)).to.throw(
+      "Invalid option for serviceAccount: 'invalid-format'. Valid options are 'default', a service account email, or '{serviceAccountName}@'"
+    );
+  });
+
+  it("should return if given RESET_VALUE for serviceAccount", () => {
+    const opts: options.GlobalOptions = {
+      serviceAccount: RESET_VALUE,
+    };
+
+    const result = options.optionsToEndpoint(opts);
+
+    expect(result.serviceAccountEmail).to.be.instanceof(ResetValue);
+  });
+
+  it("should pass through Expression for serviceAccount", () => {
+    const param = params.defineString("sa");
+    const opts: options.GlobalOptions = {
+      serviceAccount: param,
+    };
+
+    const result = options.optionsToEndpoint(opts);
+
+    expect(result.serviceAccountEmail).to.equal(param);
+  });
+});

spec/init.ts

@@ -49,18 +49,21 @@ if (args.length > 1) {
   functionsDir = args[0];
 }
 
+const MANIFEST_PREFIX = "__FIREBASE_FUNCTIONS_MANIFEST__:";
+const MANIFEST_ERROR_PREFIX = "__FIREBASE_FUNCTIONS_MANIFEST_ERROR__:";
+
 async function runStdioDiscovery() {
   try {
     const stack = await loadStack(functionsDir);
     const wireFormat = stackToWire(stack);
     const manifestJson = JSON.stringify(wireFormat);
     const base64 = Buffer.from(manifestJson).toString("base64");
-    process.stderr.write(`__FIREBASE_FUNCTIONS_MANIFEST__:${base64}\n`);
+    process.stderr.write(`${MANIFEST_PREFIX}${base64}\n`);
     process.exit(0);
   } catch (e) {
     console.error("Failed to generate manifest from function source:", e);
-    const errorMessage = e instanceof Error ? e.message : String(e);
-    process.stderr.write(`__FIREBASE_FUNCTIONS_MANIFEST_ERROR__:${errorMessage}\n`);
+    const message = e instanceof Error ? e.message : String(e);
+    process.stderr.write(`${MANIFEST_ERROR_PREFIX}${message}\n`);
     process.exit(1);
   }
 }