Skip to content

Commit 7433f25

Browse files
committed
fix: stop silently dropping telemetry event attributes
turn_ended now carries the engine error code when a turn fails, session attribution keys emit empty strings so they survive payload flattening, and properties a sink cannot accept are reported through a new onUnexpectedError hook instead of vanishing. Properties are sanitized when the sink accepts the event, so drops on events queued before initialization are reported once a handler is installed.
1 parent f12fdab commit 7433f25

14 files changed

Lines changed: 206 additions & 39 deletions

File tree

apps/pythinker-code/src/cli/telemetry.ts

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
import { createPythinkerDeviceId } from '@pymodel/pythinker-code-oauth';
22
import {
33
loadRuntimeConfigSafe,
4+
log,
45
resolveConfigPath,
56
resolvePythinkerHome,
67
type PythinkerConfig,
@@ -56,6 +57,7 @@ export function initializeCliTelemetry(options: InitializeCliTelemetryOptions):
5657
model: options.model ?? options.config.defaultModel,
5758
sessionId: options.sessionId,
5859
endpoint: () => currentPythinkerProfile().telemetryEndpoint,
60+
onUnexpectedError: (error) => log.warn('telemetry property dropped', { error: String(error) }),
5961
});
6062
if (options.bootstrap.firstLaunch) {
6163
options.harness.track('first_launch');
@@ -98,6 +100,7 @@ export function initializeServerTelemetry(
98100
uiMode: WEB_UI_MODE,
99101
model: config.defaultModel,
100102
endpoint: () => currentPythinkerProfile().telemetryEndpoint,
103+
onUnexpectedError: (error) => log.warn('telemetry property dropped', { error: String(error) }),
101104
});
102105

103106
return {

apps/pythinker-code/test/cli/export.test.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -430,6 +430,7 @@ describe('pythinker export', () => {
430430
model: 'k2',
431431
sessionId: undefined,
432432
endpoint: expect.any(Function),
433+
onUnexpectedError: expect.any(Function),
433434
});
434435
// The endpoint resolver defers to the active region profile at flush time.
435436
const telemetryOptions = mocks.initializeTelemetry.mock.calls[0]![0] as {

apps/pythinker-code/test/cli/run-shell.test.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -335,6 +335,7 @@ describe('runShell', () => {
335335
model: 'k2',
336336
sessionId: undefined,
337337
endpoint: expect.any(Function),
338+
onUnexpectedError: expect.any(Function),
338339
});
339340
// The endpoint resolver defers to the active region profile at flush time.
340341
const telemetryOptions = mocks.initializeTelemetry.mock.calls[0]![0] as {

packages/agent-core-v2/src/agent/loop/loopService.ts

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -524,8 +524,9 @@ export class AgentLoopService extends Disposable implements IAgentLoopService {
524524
result?.type === 'completed'
525525
? this.lastRequestTraceId
526526
: this.activeRequestTrace?.traceId;
527+
const error =
528+
result?.type === 'failed' ? toPythinkerErrorPayload(result.error) : undefined;
527529
if (result !== undefined) {
528-
const error = result.type === 'failed' ? toPythinkerErrorPayload(result.error) : undefined;
529530
const interruptReason =
530531
result.type === 'completed' ? undefined : interruptReasonFor(result);
531532
const durationMs = Date.now() - startedAt;
@@ -563,6 +564,7 @@ export class AgentLoopService extends Disposable implements IAgentLoopService {
563564
reason: result?.type ?? 'failed',
564565
duration_ms: Date.now() - startedAt,
565566
mode,
567+
error_type: error?.code,
566568
provider_type,
567569
protocol,
568570
thinking_effort: thinkingEffort,

packages/agent-core-v2/src/app/telemetry/events.ts

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -76,6 +76,7 @@ export interface TurnEndedEvent {
7676
reason: 'completed' | 'cancelled' | 'failed';
7777
duration_ms: number;
7878
mode: 'agent' | 'plan';
79+
error_type?: string;
7980
provider_type?: string;
8081
protocol?: string;
8182
thinking_effort?: string;
@@ -608,6 +609,7 @@ export const telemetryEventDefinitions = {
608609
reason: 'How the turn ended',
609610
duration_ms: 'Turn wall-clock time in milliseconds',
610611
mode: 'Agent mode the turn ran in',
612+
error_type: 'Engine error code when the turn failed; absent otherwise',
611613
provider_type: 'Provider protocol type',
612614
protocol: 'Request protocol',
613615
thinking_effort: 'Effective thinking effort the turn ran with',

packages/agent-core-v2/test/agent/loop/loop.test.ts

Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@ import { TurnEnded } from '#/agent/loop/turnOps';
2323
import { RetryStepRequest } from '#/agent/prompt/promptStepRequests';
2424
import type { ExecutableTool } from '#/tool/toolContract';
2525
import { IAgentToolRegistryService } from '#/agent/toolRegistry/toolRegistry';
26+
import { IAgentToolExecutorService } from '#/agent/toolExecutor/toolExecutor';
2627
import { IEventBus } from '#/app/event/eventBus';
2728
import { userCancellationReason } from '#/_base/utils/abort';
2829

@@ -1172,6 +1173,7 @@ describe('turn telemetry', () => {
11721173
turn_id: 0,
11731174
reason: 'failed',
11741175
mode: 'agent',
1176+
error_type: 'provider.filtered',
11751177
trace_id: 'trace-turn-2',
11761178
}),
11771179
});
@@ -1180,6 +1182,45 @@ describe('turn telemetry', () => {
11801182
}
11811183
});
11821184

1185+
it('emits turn_ended with error_type for an uncoded failure', async () => {
1186+
const records: TelemetryRecord[] = [];
1187+
const local = createTestAgent({ telemetry: recordingTelemetry(records) });
1188+
try {
1189+
const workTool: ExecutableTool = {
1190+
name: 'Work',
1191+
description: 'Pretend to work.',
1192+
parameters: { type: 'object', properties: {}, additionalProperties: false },
1193+
resolveExecution: () => ({
1194+
approvalRule: 'Work',
1195+
execute: async () => ({ output: 'should never run' }),
1196+
}),
1197+
};
1198+
local.get(IAgentToolRegistryService).register(workTool);
1199+
local.get(IAgentProfileService).update({ activeToolNames: ['Work'] });
1200+
const subscription = local.get(IAgentToolExecutorService).onBeforeExecuteTool(() => {
1201+
throw new Error('beforeExecute blew up');
1202+
});
1203+
local.mockNextResponse(
1204+
{ type: 'text', text: 'working' },
1205+
{ type: 'function', id: 'call-work-1', name: 'Work', arguments: '{}' },
1206+
);
1207+
await local.rpc.prompt({ input: [{ type: 'text', text: 'use the tool' }] });
1208+
await local.untilTurnEnd();
1209+
subscription.dispose();
1210+
1211+
expect(records).toContainEqual({
1212+
event: 'turn_ended',
1213+
properties: expect.objectContaining({
1214+
turn_id: 0,
1215+
reason: 'failed',
1216+
error_type: 'internal',
1217+
}),
1218+
});
1219+
} finally {
1220+
await local.dispose();
1221+
}
1222+
});
1223+
11831224
it.each([
11841225
['user_cancelled', () => userCancellationReason()],
11851226
['aborted', () => new Error('stop')],

packages/agent-core/src/agent/permission/policies/deny-all.ts

Lines changed: 11 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,19 @@
1-
import type { PermissionPolicy, PermissionPolicyResult } from '../types';
1+
import type { PermissionPolicy, PermissionPolicyContext, PermissionPolicyResult } from '../types';
22

3+
/**
4+
* Denies every tool call except the read-only ones a side question may use to
5+
* inspect current file contents.
6+
*/
37
export class DenyAllPermissionPolicy implements PermissionPolicy {
48
readonly name = 'deny-all';
59

6-
constructor(private readonly message: string) {}
10+
constructor(
11+
private readonly message: string,
12+
private readonly allowed: ReadonlySet<string> = new Set(),
13+
) {}
714

8-
evaluate(): PermissionPolicyResult {
15+
evaluate(context: PermissionPolicyContext): PermissionPolicyResult | undefined {
16+
if (this.allowed.has(context.toolCall.name)) return undefined;
917
return {
1018
kind: 'deny',
1119
message: this.message,

packages/agent-core/src/session/subagent-host.ts

Lines changed: 10 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -93,20 +93,22 @@ const SUMMARY_CONTINUATION_ATTEMPTS = 1;
9393
const HOOK_TEXT_PREVIEW_LENGTH = 500;
9494
const SUBAGENT_MAX_TOKENS_ERROR =
9595
'Subagent turn failed before completing its final summary: reason=max_tokens';
96+
const BTW_READONLY_TOOLS = new Set(['Read', 'Grep', 'Glob']);
9697
const TOOL_CALL_DISABLED_MESSAGE =
97-
'Tool calls are disabled for side questions. Answer with text only.';
98+
'Only the read-only tools Read, Grep, and Glob are available for side questions. Other tool calls are disabled.';
9899
const SUBAGENT_PROMPT_ORIGIN: PromptOrigin = { kind: 'system_trigger', name: 'subagent' };
99100
const SIDE_QUESTION_SYSTEM_REMINDER = `
100-
This is a side-channel conversation with the user. You should answer user questions directly based on what you already know.
101+
This is a side-channel conversation with the user. You should answer user questions directly.
101102
102103
IMPORTANT:
103104
- You are a separate, lightweight instance.
104105
- The main agent continues independently; do not reference being interrupted.
105-
- Do not call any tools. All tool calls are disabled and will be rejected.
106-
Even though tool definitions are visible in this request, they exist only
107-
for technical reasons (prompt cache). You must not use them.
108-
- Respond only with text based on what you already know from the conversation
109-
and this side-channel conversation.
106+
- You may use the read-only tools Read, Grep, and Glob to inspect files when
107+
the answer depends on current file contents. All other tools are disabled
108+
and will be rejected, even though their definitions are visible in this
109+
request (they exist only for technical reasons — prompt cache).
110+
- Prefer answering from what you already know from the conversation and this
111+
side-channel conversation; reach for the read-only tools only when needed.
110112
- Follow-up turns may happen in this side-channel conversation.
111113
- If you do not know the answer, say so directly.
112114
`;
@@ -284,7 +286,7 @@ export class SessionSubagentHost {
284286
kind: 'system_trigger',
285287
name: 'btw',
286288
});
287-
child.permission.policies.unshift(new DenyAllPermissionPolicy(TOOL_CALL_DISABLED_MESSAGE));
289+
child.permission.policies.unshift(new DenyAllPermissionPolicy(TOOL_CALL_DISABLED_MESSAGE, BTW_READONLY_TOOLS));
288290
return id;
289291
}
290292

packages/agent-core/test/session/init.test.ts

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -480,7 +480,7 @@ describe('AgentAPI.startBtw', () => {
480480
const historyText = JSON.stringify(scripted.calls[0]?.history);
481481
expect(historyText).toContain('Main task: implement /btw.');
482482
expect(historyText).toContain('This is a side-channel conversation with the user.');
483-
expect(historyText).toContain('All tool calls are disabled and will be rejected.');
483+
expect(historyText).toContain('All other tools are disabled');
484484
expect(historyText).toContain('What are you working on right now?');
485485
expect(historyText).not.toContain('call-open');
486486
expect(JSON.stringify(mainAgent.context.history)).not.toContain(
@@ -570,15 +570,16 @@ describe('AgentAPI.startBtw', () => {
570570
'Read',
571571
]);
572572
expect(JSON.stringify(scripted.calls[1]?.history)).toContain(
573-
'Tool calls are disabled for side questions. Answer with text only.',
573+
'Only the read-only tools Read, Grep, and Glob are available for side questions.',
574574
);
575575
expect(events).toContainEqual(
576576
expect.objectContaining({
577577
type: 'tool.result',
578578
agentId: 'agent-0',
579579
toolCallId: 'call_lookup_note',
580580
isError: true,
581-
output: 'Tool calls are disabled for side questions. Answer with text only.',
581+
output:
582+
'Only the read-only tools Read, Grep, and Glob are available for side questions. Other tool calls are disabled.',
582583
}),
583584
);
584585
expect(JSON.stringify(mainAgent.context.history)).not.toContain(

packages/node-sdk/src/pythinker-harness.ts

Lines changed: 6 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -679,13 +679,12 @@ export class PythinkerHarness {
679679
...this.sessionStartedDynamicProperties?.(),
680680
// Canonical fields are owned by the harness and must win over any
681681
// caller-supplied sessionStartedProperties that happen to share a key.
682-
// `client_id` is always null here: a single-process host has no
683-
// per-connection client id (that concept only exists for daemon clients,
684-
// see core-impl.ts). Kept as an explicit key so both producers share the
685-
// same session_started schema.
686-
client_id: null,
687-
client_name: this.identity?.productName ?? null,
688-
client_version: this.identity?.version ?? null,
682+
// A single-process host has no per-connection client id, so `client_id`
683+
// stays empty; empty strings (unlike null) survive payload flattening,
684+
// keeping the client-attribution keys present on every row.
685+
client_id: '',
686+
client_name: this.identity?.productName ?? '',
687+
client_version: this.identity?.version ?? '',
689688
ui_mode: this.uiMode,
690689
resumed,
691690
});

0 commit comments

Comments
 (0)