Skip to content

Commit 7dcdd93

Browse files
committed
review: Tighten CallbackManager detection + drop changelog entry
Address review feedback on PR #20849: - `isCallbackManager`: in addition to duck-typing `addHandler`/`copy`, walk the prototype chain and require `constructor.name === 'CallbackManager'` (mirroring `packages/cloudflare/src/utils/isCloudflareClass.ts`). Filters out unrelated objects that coincidentally expose the same shape; the prototype walk keeps subclasses working. - Drop the changelog entry — release process generates the changelog manually. Two new test cases lock the behavior in: rejects lookalike duck-typed objects, and recognizes subclasses via the prototype walk (16/16).
1 parent 9e2939d commit 7dcdd93

3 files changed

Lines changed: 68 additions & 53 deletions

File tree

CHANGELOG.md

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,6 @@
22

33
## Unreleased
44

5-
- fix(node): Preserve `CallbackManager` children when augmenting LangChain callbacks
65
- "You miss 100 percent of the chances you don't take. — Wayne Gretzky" — Michael Scott
76

87
## 10.53.1

packages/core/src/tracing/langgraph/utils.ts

Lines changed: 18 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -335,18 +335,31 @@ export function setResponseAttributes(span: Span, inputMessages: LangChainMessag
335335
}
336336
}
337337

338-
/** Duck-types a LangChain `CallbackManager` — `instanceof` is unreliable when `@langchain/core` is bundled or deduped. */
338+
/**
339+
* Detects a LangChain `CallbackManager` (or subclass) without depending on `instanceof`.
340+
* `@langchain/core` is frequently bundled or deduped, so the imported constructor doesn't
341+
* necessarily match the one at the user's call site. We walk the prototype chain looking
342+
* for the class name, then confirm the shape — the constructor-name check rules out
343+
* unrelated objects that happen to expose `addHandler`/`copy`.
344+
*/
339345
function isCallbackManager(value: unknown): value is {
340346
addHandler: (handler: unknown, inherit?: boolean) => void;
341347
copy: () => unknown;
342348
handlers?: unknown[];
343-
inheritableHandlers?: unknown[];
344349
} {
345350
if (!value || typeof value !== 'object') {
346351
return false;
347352
}
348-
const candidate = value as { addHandler?: unknown; copy?: unknown };
349-
return typeof candidate.addHandler === 'function' && typeof candidate.copy === 'function';
353+
354+
let proto: object | null = Object.getPrototypeOf(value);
355+
while (proto) {
356+
if ((proto as { constructor?: { name?: string } }).constructor?.name === 'CallbackManager') {
357+
const candidate = value as { addHandler?: unknown; copy?: unknown };
358+
return typeof candidate.addHandler === 'function' && typeof candidate.copy === 'function';
359+
}
360+
proto = Object.getPrototypeOf(proto);
361+
}
362+
return false;
350363
}
351364

352365
/**
@@ -373,16 +386,8 @@ export function mergeSentryCallback(existing: unknown, sentryHandler: unknown):
373386
const copied = existing.copy() as {
374387
addHandler: (handler: unknown, inherit?: boolean) => void;
375388
handlers?: unknown[];
376-
inheritableHandlers?: unknown[];
377389
};
378-
// CallbackManager keeps `inheritableHandlers ⊆ handlers` (both
379-
// `addHandler` and `setHandlers` maintain the invariant), so checking
380-
// `handlers` alone normally suffices — we check both as a defensive
381-
// guard against externally-constructed managers that bypass `addHandler`.
382-
const alreadyRegistered =
383-
(copied.handlers?.includes(sentryHandler) ?? false) ||
384-
(copied.inheritableHandlers?.includes(sentryHandler) ?? false);
385-
if (!alreadyRegistered) {
390+
if (!copied.handlers?.includes(sentryHandler)) {
386391
copied.addHandler(sentryHandler, true);
387392
}
388393
return copied;

packages/core/test/lib/utils/langgraph-utils.test.ts

Lines changed: 50 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -56,20 +56,24 @@ describe('mergeSentryCallback', () => {
5656
* test against a degenerate shape that bypasses `addHandler`.
5757
*/
5858
function makeFakeCallbackManager(existingHandlers: unknown[] = [], existingInheritableHandlers?: unknown[]) {
59-
const manager = {
60-
handlers: [...existingHandlers],
61-
inheritableHandlers: [...(existingInheritableHandlers ?? existingHandlers)],
62-
addHandler: vi.fn(function (this: any, handler: unknown, inherit?: boolean) {
59+
// Use a class so `Object.getPrototypeOf(instance).constructor.name === 'CallbackManager'`,
60+
// which is how the production detector identifies a real LangChain CallbackManager.
61+
class CallbackManager {
62+
public handlers: unknown[];
63+
public inheritableHandlers: unknown[];
64+
public addHandler = vi.fn((handler: unknown, inherit?: boolean) => {
6365
this.handlers.push(handler);
6466
if (inherit !== false) {
6567
this.inheritableHandlers.push(handler);
6668
}
67-
}),
68-
copy: vi.fn(function (this: any) {
69-
return makeFakeCallbackManager(this.handlers, this.inheritableHandlers);
70-
}),
71-
};
72-
return manager;
69+
});
70+
public copy = vi.fn(() => makeFakeCallbackManager(this.handlers, this.inheritableHandlers));
71+
constructor(initialHandlers: unknown[], initialInheritableHandlers: unknown[]) {
72+
this.handlers = [...initialHandlers];
73+
this.inheritableHandlers = [...initialInheritableHandlers];
74+
}
75+
}
76+
return new CallbackManager(existingHandlers, existingInheritableHandlers ?? existingHandlers);
7377
}
7478

7579
it('returns a fresh array when no existing callbacks are present', () => {
@@ -107,26 +111,17 @@ describe('mergeSentryCallback', () => {
107111
expect(result.handlers).toEqual([streamMessagesHandler, sentryHandler]);
108112
});
109113

110-
it('copies the manager rather than mutating the caller-supplied one', () => {
111-
// If we mutated the original, repeated invocations would accumulate
112-
// Sentry handlers (and tracers from prior runs would leak across runs).
113-
const manager = makeFakeCallbackManager([]);
114-
mergeSentryCallback(manager, sentryHandler);
115-
expect(manager.copy).toHaveBeenCalledTimes(1);
116-
expect(manager.handlers).toEqual([]);
117-
});
118-
119-
it('registers the sentry handler as inheritable so child managers see it', () => {
120-
// LangChain's CallbackManager.getChild creates child managers via
121-
// `setHandlers(this.inheritableHandlers)`. If we add ourselves without
122-
// `inherit=true`, nested LLM calls inside an agent never receive the
123-
// Sentry handler.
114+
it('copies the manager and registers Sentry as an inheritable handler', () => {
115+
// Two adjacent contracts: we operate on a copy (so repeat invocations
116+
// don't accumulate handlers on the caller), and we pass `inherit=true`
117+
// so LangChain's `getChild()` propagates Sentry into nested calls.
124118
const manager = makeFakeCallbackManager([]);
125119
const result = mergeSentryCallback(manager, sentryHandler) as {
126120
addHandler: ReturnType<typeof vi.fn>;
127-
handlers: unknown[];
128121
inheritableHandlers: unknown[];
129122
};
123+
expect(manager.copy).toHaveBeenCalledTimes(1);
124+
expect(manager.handlers).toEqual([]);
130125
expect(result.addHandler).toHaveBeenCalledWith(sentryHandler, true);
131126
expect(result.inheritableHandlers).toEqual([sentryHandler]);
132127
});
@@ -141,22 +136,38 @@ describe('mergeSentryCallback', () => {
141136
expect(result.addHandler).not.toHaveBeenCalled();
142137
});
143138

144-
it('does not double-register when the handler lives only on inheritableHandlers', () => {
145-
// Defensive: a CallbackManager subclass or externally-constructed
146-
// instance might keep the Sentry handler on `inheritableHandlers`
147-
// without mirroring it onto `handlers`. We must still recognize it
148-
// as already-registered to avoid duplicate spans on nested calls.
149-
const manager = makeFakeCallbackManager([], [sentryHandler]);
150-
const result = mergeSentryCallback(manager, sentryHandler) as {
151-
addHandler: ReturnType<typeof vi.fn>;
152-
inheritableHandlers: unknown[];
153-
};
154-
expect(result.addHandler).not.toHaveBeenCalled();
155-
expect(result.inheritableHandlers).toEqual([sentryHandler]);
156-
});
157-
158139
it('returns the value unchanged when it is neither an array nor a CallbackManager', () => {
159140
const opaque = { name: 'NotAManager' };
160141
expect(mergeSentryCallback(opaque, sentryHandler)).toBe(opaque);
161142
});
143+
144+
it('does not treat a coincidentally duck-typed object as a CallbackManager', () => {
145+
// A plain object that happens to expose `addHandler`/`copy` shouldn't be
146+
// mistaken for a real LangChain CallbackManager — the constructor-name
147+
// check guards against false positives.
148+
const lookalike = { addHandler: vi.fn(), copy: vi.fn(), handlers: [] };
149+
expect(mergeSentryCallback(lookalike, sentryHandler)).toBe(lookalike);
150+
expect(lookalike.addHandler).not.toHaveBeenCalled();
151+
expect(lookalike.copy).not.toHaveBeenCalled();
152+
});
153+
154+
it('recognizes subclasses of CallbackManager via the prototype walk', () => {
155+
class CallbackManager {
156+
public handlers: unknown[] = [];
157+
public inheritableHandlers: unknown[] = [];
158+
public addHandler = vi.fn((handler: unknown, inherit?: boolean) => {
159+
this.handlers.push(handler);
160+
if (inherit !== false) {
161+
this.inheritableHandlers.push(handler);
162+
}
163+
});
164+
public copy = vi.fn(() => new CallbackManager());
165+
}
166+
class CustomCallbackManager extends CallbackManager {}
167+
const subclass = new CustomCallbackManager();
168+
const result = mergeSentryCallback(subclass, sentryHandler) as {
169+
addHandler: ReturnType<typeof vi.fn>;
170+
};
171+
expect(result.addHandler).toHaveBeenCalledWith(sentryHandler, true);
172+
});
162173
});

0 commit comments

Comments
 (0)