🛡️ fix: Keep SSRF Guards on Label Calls, Skip Mixed Handoff Batches

Round-eighteen review: four fixed; the fifth repeats the
maintainer-decided packages/api extraction (eighth instance), answered
on-thread.

- SSRF-safe carrier (host.ts, P1): the sanitize step restored the
  Anthropic `clientOptions` carrier only when `defaultHeaders` existed,
  but for user-provided base URLs `getLLMConfig` stores the guarded
  Undici dispatcher and `redirect: 'error'` there — dropping it
  reopened DNS-rebinding/redirect paths on label calls to
  user-controlled URLs. The carrier (client CONSTRUCTION options, not
  generation params) is now restored whenever present, same reference.
- Primary maxTokens (host.ts): top-level `maxTokens` is not in
  `omitTitleOptions` and survived the strip; the title path deletes it
  explicitly, and a cap sized for the primary model can be rejected by
  the substitute. Deleted on the copy.
- Bounded keys (runtime.ts): the object branch materialized every key
  via `Object.keys` and quoted oversized keys in full before the budget
  check. Enumeration is now lazy (`for..in` + own-property guard) and
  keys slice to the budget before quoting, like string values.
- Mixed handoff batches (runtime.ts, groupToolCalls.ts): the client
  flushes the block at the transfer card, so a mixed batch's label
  orphaned exactly like a pure one. The hook now skips ANY batch
  containing a transfer call, and the renderer drops orphan labels
  covering one (legacy content).

Tests: carrier survival without headers by same reference, maxTokens
strip (host.spec); mixed batch claims nothing (runtime.spec); mixed
orphan dropped, real-batch orphan kept (groupToolCalls.test).

client/src/utils/__tests__/groupToolCalls.test.ts

@@ -116,6 +116,31 @@ describe('groupSequentialToolCalls with activity labels', () => {
     expect((grouped[0] as { part: PartWithIndex }).part.idx).toBe(0);
   });
 
+  /** Mixed legacy content: an orphan label covering a transfer AND real
+   *  calls is equally headless once the block flushed at the transfer. */
+  it('drops an orphan label whose batch mixed a transfer with real calls', () => {
+    const realTool = {
+      type: ContentTypes.TOOL_CALL,
+      [ContentTypes.TOOL_CALL]: { id: 't1', name: 'web_search', args: '{}', output: 'ok' },
+    } as unknown as TMessageContentParts;
+    const transfer = {
+      type: ContentTypes.TOOL_CALL,
+      [ContentTypes.TOOL_CALL]: { id: 'x1', name: 'lc_transfer_to_billing', args: '{}' },
+    } as unknown as TMessageContentParts;
+    const mixedLabel = {
+      type: ContentTypes.ACTIVITY_LABEL,
+      [ContentTypes.ACTIVITY_LABEL]: 'Looked up the refund policy',
+      tool_call_ids: ['t1', 'x1'],
+      pending: false,
+    } as unknown as TMessageContentParts;
+
+    const grouped = groupSequentialToolCalls(withIndex([realTool, transfer, mixedLabel]));
+
+    /** Tool card + handoff card render; the headless label is dropped. */
+    expect(grouped).toHaveLength(2);
+    expect(grouped.every((entry) => entry.type === 'single')).toBe(true);
+  });
+
   it('keeps rendering an orphan label whose batch had real tool calls', () => {
     const orphanLabel = {
       type: ContentTypes.ACTIVITY_LABEL,

client/src/utils/groupToolCalls.ts

@@ -24,18 +24,20 @@ function isGroupableToolCall(part: TMessageContentParts): boolean {
 }
 
 /**
- * True when every tool call the label covers is a `transfer_to_*` handoff.
- * Transfer parts are never groupable, so such a label can only orphan into a
- * stray line after the handoff card — which already names the destination.
- * Content persisted before the server stopped claiming labels for pure
- * handoff batches still carries these; they are dropped at render.
+ * True when the label covers ANY `transfer_to_*` handoff call. Transfer
+ * parts are never groupable, so the flush at the handoff card leaves such a
+ * label with nothing to head — it can only orphan into a stray line after
+ * cards that already show everything (the handoff card names the
+ * destination; mixed batches keep their tool cards). Content persisted
+ * before the server stopped claiming labels for handoff batches still
+ * carries these; they are dropped at render.
  */
-function isTransferOnlyLabel(labelPart: TMessageContentParts, allParts: PartWithIndex[]): boolean {
+function coversTransferCall(labelPart: TMessageContentParts, allParts: PartWithIndex[]): boolean {
   const ids = (labelPart as { tool_call_ids?: unknown }).tool_call_ids;
   if (!Array.isArray(ids) || ids.length === 0) {
     return false;
   }
-  return ids.every((id) =>
+  return ids.some((id) =>
     allParts.some(({ part }) => {
       if (part?.type !== ContentTypes.TOOL_CALL) {
         return false;
@@ -119,10 +121,10 @@ export function groupSequentialToolCalls(parts: PartWithIndex[]): GroupedPart[]
       flushWithoutLabel();
       if (claimed.length > 0) {
         result.push({ type: 'tool-group', parts: claimed, labelPart: item });
-      } else if (!isTransferOnlyLabel(item.part, parts)) {
+      } else if (!coversTransferCall(item.part, parts)) {
         /** Orphan label (block parts hidden/filtered): renders standalone —
-         *  UNLESS its batch was only handoff calls, where the transfer card
-         *  already says everything and the label would be a stray line. */
+         *  UNLESS its batch contained a handoff call, where the cards
+         *  already say everything and the label would be a stray line. */
         result.push({ type: 'single', part: item });
       }
       continue;

packages/api/src/agents/activityLabels/__tests__/host.spec.ts

@@ -184,6 +184,27 @@ describe('resolveActivityLabelModel model precedence', () => {
       defaultHeaders: { 'x-proxy-key': 'abc' },
     });
   });
+
+  it('strips the primary maxTokens like the title path', async () => {
+    mockGetOptions.mockResolvedValueOnce({
+      llmConfig: { model: 'resolved', maxTokens: 64_000 },
+    } as never);
+    const resolved = await resolve({ activityLabel: true, activityModel: 'label-model' });
+    expect((resolved.clientOptions as Record<string, unknown>).maxTokens).toBeUndefined();
+  });
+
+  /** The Anthropic carrier holds client CONSTRUCTION options — for
+   *  user-provided base URLs that includes the SSRF-safe fetch dispatcher —
+   *  so it must survive the strip even with no custom headers, and by the
+   *  SAME reference. */
+  it('preserves the SSRF-safe carrier even without defaultHeaders', async () => {
+    const carrier = { fetchOptions: { dispatcher: { kind: 'guarded' }, redirect: 'error' } };
+    mockGetOptions.mockResolvedValueOnce({
+      llmConfig: { model: 'resolved', thinking: { type: 'enabled' }, clientOptions: carrier },
+    } as never);
+    const resolved = await resolve({ activityLabel: true, activityModel: 'label-model' });
+    expect((resolved.clientOptions as Record<string, unknown>).clientOptions).toBe(carrier);
+  });
 });
 
 describe('mapCollectedMetadataToUsage cache tokens', () => {

packages/api/src/agents/activityLabels/__tests__/runtime.spec.ts

@@ -307,7 +307,9 @@ describe('createActivityLabelHook', () => {
     expect(mockInvoke).not.toHaveBeenCalled();
   });
 
-  it('still claims for a mixed batch containing a transfer call', async () => {
+  /** Mixed batches skip too: the client flushes the block at the transfer
+   *  card, so the label would orphan even when real tools ran alongside. */
+  it('claims nothing for a mixed batch containing a transfer call', async () => {
     const hook = createActivityLabelHook({ claimSlot, resolveLLM });
     await hook(
       batchInput({
@@ -331,7 +333,8 @@ describe('createActivityLabelHook', () => {
       new AbortController().signal,
     );
     await flushDetached();
-    expect(slots).toHaveLength(1);
+    expect(slots).toHaveLength(0);
+    expect(mockInvoke).not.toHaveBeenCalled();
   });
 
   it('skips subagent scopes entirely', async () => {

packages/api/src/agents/activityLabels/host.ts

@@ -299,25 +299,35 @@ export async function resolveActivityLabelModel({
    *  `modelKwargs` output caps go for the same reason (copied, not mutated:
    *  `llmConfig` is shared with the memoized provider resolution). */
   const rawOptions = { ...(llmConfig ?? {}) } as MaybeAzureConfig & {
+    maxTokens?: number;
     modelKwargs?: Record<string, unknown>;
     clientOptions?: { defaultHeaders?: unknown };
   };
+  /** Top-level `maxTokens` too, exactly like the title path — it is not in
+   *  `omitTitleOptions`, and a primary cap sized for the agent's model can
+   *  be unsupported or absurd on the substitute label model. */
+  delete rawOptions.maxTokens;
   if (rawOptions.modelKwargs != null) {
     const modelKwargs = { ...rawOptions.modelKwargs };
     delete modelKwargs.max_completion_tokens;
     delete modelKwargs.max_output_tokens;
     rawOptions.modelKwargs = modelKwargs;
   }
-  /** The filter drops the Anthropic `clientOptions` carrier (thinking,
-   *  streaming), which would also drop its `defaultHeaders` — restore the
-   *  SAME object reference so gateway/proxy metadata still reaches label
-   *  requests and `resolveConfigHeaders` mutates the object the client is
-   *  actually built from. */
+  /** The filter drops the Anthropic `clientOptions` carrier, so restore it
+   *  WHENEVER it exists — by the SAME reference. It holds client
+   *  CONSTRUCTION options, not generation parameters: proxy
+   *  `defaultHeaders`, and for user-provided base URLs the SSRF-safe
+   *  `fetchOptions` (guarded Undici dispatcher, `redirect: 'error'`).
+   *  Restoring only when headers were present silently stripped those
+   *  guards from label calls to user-controlled URLs, re-opening DNS
+   *  rebinding/redirect paths the endpoint validation exists to block; the
+   *  same-reference restore also lets `resolveConfigHeaders` mutate the
+   *  object the client is actually built from. */
   const anthropicCarrier = rawOptions.clientOptions;
   const clientOptions = Object.fromEntries(
     Object.entries(rawOptions).filter(([key]) => !omitTitleOptions.has(key)),
   ) as MaybeAzureConfig & { clientOptions?: { defaultHeaders?: unknown } };
-  if (anthropicCarrier?.defaultHeaders != null && clientOptions.clientOptions == null) {
+  if (anthropicCarrier != null && clientOptions.clientOptions == null) {
     clientOptions.clientOptions = anthropicCarrier;
   }
   if (options.configOptions) {

packages/api/src/agents/activityLabels/runtime.ts

@@ -258,12 +258,21 @@ function stringifyBounded(value: unknown, limit: number): string {
         return false;
       }
       let first = true;
-      for (const key of Object.keys(val)) {
+      /** for-in, not Object.keys: enumeration stops at the budget instead of
+       *  materializing a key array for a million-property object first. The
+       *  own-property guard replaces what Object.keys implied. */
+      for (const key in val) {
+        if (!Object.prototype.hasOwnProperty.call(val, key)) {
+          continue;
+        }
         const entry = (val as Record<string, unknown>)[key];
         if (entry === undefined || typeof entry === 'function') {
           continue;
         }
-        if ((!first && !push(',')) || !push(`${JSON.stringify(key)}:`) || !walk(entry)) {
+        /** Keys are untrusted too: slice BEFORE quoting, exactly like string
+         *  values — quoting is what materializes the copy. */
+        const boundedKey = key.length > limit + 1 ? key.slice(0, limit + 1) : key;
+        if ((!first && !push(',')) || !push(`${JSON.stringify(boundedKey)}:`) || !walk(entry)) {
           return false;
         }
         first = false;
@@ -455,14 +464,15 @@ export function createActivityLabelHook(
     ) {
       return {};
     }
-    /** A batch that is ONLY handoff calls gets no label. The transfer card
-     *  already names the destination, the client renders transfer parts
-     *  standalone (never groupable), so a claimed label could only orphan
-     *  into a stray line after the card — and the model call would be spent
-     *  describing what the card already says. Skipped BEFORE the quota so
-     *  handoffs never consume `maxPerRun`. */
+    /** A batch containing ANY handoff call gets no label — mixed batches
+     *  included. Transfer parts are never groupable on the client, so the
+     *  flush at the transfer card leaves the label with nothing to head and
+     *  it could only orphan into a stray line after the cards; the handoff
+     *  card already names the destination and the tool cards still show the
+     *  work. Skipped BEFORE the quota so handoffs never consume
+     *  `maxPerRun`. */
     if (
-      input.entries.every((entry) => entry.toolName?.startsWith(Constants.LC_TRANSFER_TO_) === true)
+      input.entries.some((entry) => entry.toolName?.startsWith(Constants.LC_TRANSFER_TO_) === true)
     ) {
       return {};
     }