Fix reporter self-review findings

Preserve legacy telemetry hook output without mutating the allowlisted aggregate, keep root session telemetry authoritative, retain inherited lifecycle scope, and make shadow result summaries internally consistent.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Author: TheLarkInn

common/reviews/api/rush-reporter.api.md

@@ -39,7 +39,7 @@ export type BootstrapPrivacyClassification = 'public' | 'local-sensitive' | 'sec
 export function computeEnvelopePrivacyFloor(classifications: Iterable<ReporterPrivacyClassification>): ReporterPrivacyClassification;
 
 // @beta
-export function createBeforeLogAdapter(hooks: readonly LegacyBeforeLogHook[]): (aggregate: ITelemetryAggregate) => void;
+export function createBeforeLogAdapter(hooks: readonly LegacyBeforeLogHook[]): (aggregate: ITelemetryAggregate) => Record<string, unknown>;
 
 // @beta
 export function createEngineSink(providedSink?: IReporterEventSink): IEngineSinkResolution;

libraries/reporter/src/lifecycle/LifecycleEmitter.ts

@@ -97,8 +97,8 @@ export class LifecycleEmitter {
   public emitOperationRegistered(payload: IOperationRegisteredPayload): string {
     return this._emit('operationRegistered', payload, 'public', {
       operationId: payload.operationId,
-      projectName: payload.projectName,
-      phaseName: payload.phaseName
+      ...(payload.projectName === undefined ? {} : { projectName: payload.projectName }),
+      ...(payload.phaseName === undefined ? {} : { phaseName: payload.phaseName })
     });
   }
 

libraries/reporter/src/lifecycle/ShadowParity.ts

@@ -46,23 +46,27 @@ export interface IShadowResultSummary {
  * @beta
  */
 export function deriveExitCodeFromEvents(events: readonly IReporterEventEnvelope<unknown>[]): number {
+  let commandResult: ICommandResultPayload | undefined;
   for (const event of events) {
     if (event.type === 'commandResult') {
-      const payload: ICommandResultPayload = event.payload as ICommandResultPayload;
-      if (payload.succeeded) {
-        return 0;
-      }
-      return payload.exitCode !== 0 ? payload.exitCode : 1;
+      commandResult = event.payload as ICommandResultPayload;
+    }
+  }
+  if (commandResult !== undefined) {
+    if (commandResult.succeeded) {
+      return 0;
     }
+    return commandResult.exitCode !== 0 ? commandResult.exitCode : 1;
   }
 
+  let sessionExitCode: number | undefined;
   for (const event of events) {
     if (event.type === 'sessionCompleted') {
-      return (event.payload as { exitCode: number }).exitCode;
+      sessionExitCode = (event.payload as { exitCode: number }).exitCode;
     }
   }
 
-  return 0;
+  return sessionExitCode ?? 0;
 }
 
 /**
@@ -81,7 +85,7 @@ export function summarizeShadowResult(
 ): IShadowResultSummary {
   const operationCounts: { [status: string]: number } = {};
   let commandName: string | undefined;
-  let succeeded: boolean = true;
+  let commandSucceeded: boolean | undefined;
 
   for (const event of events) {
     if (event.type === 'operationStatusChanged') {
@@ -90,14 +94,15 @@ export function summarizeShadowResult(
     } else if (event.type === 'commandResult') {
       const payload: ICommandResultPayload = event.payload as ICommandResultPayload;
       commandName = payload.commandName;
-      succeeded = payload.succeeded;
+      commandSucceeded = payload.succeeded;
     }
   }
 
+  const exitCode: number = deriveExitCodeFromEvents(events);
   return {
     commandName,
-    succeeded,
-    exitCode: deriveExitCodeFromEvents(events),
+    succeeded: commandSucceeded ?? exitCode === 0,
+    exitCode,
     operationCounts
   };
 }

libraries/reporter/src/telemetry/BeforeLogAdapter.ts

@@ -19,20 +19,32 @@ export type LegacyBeforeLogHook = (telemetry: Record<string, unknown>) => void;
  *
  * @remarks
  * During migration the existing `beforeLog` hook is preserved: the adapter runs
- * each legacy hook with a plain-object copy of the new aggregate, so no hook
- * observes non-allowlisted data.
+ * each legacy hook with a deep plain-object copy of the new aggregate, so no
+ * hook observes non-allowlisted source data or mutates the allowlisted
+ * aggregate. The returned record preserves hook augmentations for the legacy
+ * telemetry writer.
  *
  * @param hooks - the legacy hooks to preserve
  *
  * @beta
  */
 export function createBeforeLogAdapter(
   hooks: readonly LegacyBeforeLogHook[]
-): (aggregate: ITelemetryAggregate) => void {
-  return (aggregate: ITelemetryAggregate): void => {
-    const record: Record<string, unknown> = { ...aggregate };
+): (aggregate: ITelemetryAggregate) => Record<string, unknown> {
+  return (aggregate: ITelemetryAggregate): Record<string, unknown> => {
+    const record: Record<string, unknown> = {
+      ...aggregate,
+      operationStatusCounts: { ...aggregate.operationStatusCounts },
+      diagnosticCodes: [...aggregate.diagnosticCodes],
+      diagnosticCategoryCounts: { ...aggregate.diagnosticCategoryCounts },
+      producerVersions: [...aggregate.producerVersions],
+      ...(aggregate.protocolVersion === undefined
+        ? {}
+        : { protocolVersion: { ...aggregate.protocolVersion } })
+    };
     for (const hook of hooks) {
       hook(record);
     }
+    return record;
   };
 }

libraries/reporter/src/telemetry/TelemetrySubscriber.ts

@@ -52,11 +52,17 @@ export class TelemetrySubscriber {
 
     switch (event.type) {
       case 'commandStarted': {
+        if (event.parentSessionId !== undefined) {
+          break;
+        }
         // Deliberately ignores argv.
         this._commandName = (event.payload as { commandName: string }).commandName;
         break;
       }
       case 'commandResult': {
+        if (event.parentSessionId !== undefined) {
+          break;
+        }
         const payload: { commandName: string; succeeded: boolean; exitCode: number } = event.payload as {
           commandName: string;
           succeeded: boolean;
@@ -68,20 +74,25 @@ export class TelemetrySubscriber {
         break;
       }
       case 'commandCompleted': {
+        if (event.parentSessionId !== undefined) {
+          break;
+        }
         const payload: { durationMs?: number } = event.payload as { durationMs?: number };
         if (payload.durationMs !== undefined) {
           this._durationMs = payload.durationMs;
         }
         break;
       }
       case 'sessionCompleted': {
+        if (event.parentSessionId !== undefined) {
+          break;
+        }
         const payload: { exitCode: number; durationMs?: number } = event.payload as {
           exitCode: number;
           durationMs?: number;
         };
-        if (this._exitCode === undefined) {
-          this._exitCode = payload.exitCode;
-        }
+        this._exitCode = payload.exitCode;
+        this._result = payload.exitCode === 0 ? 'succeeded' : 'failed';
         if (payload.durationMs !== undefined) {
           this._durationMs = payload.durationMs;
         }

libraries/reporter/src/test/Lifecycle.test.ts

@@ -74,6 +74,25 @@ describe('LifecycleEmitter', () => {
     });
   });
 
+  it('preserves inherited scope when operation fields are omitted', () => {
+    const sink: CapturingSink = new CapturingSink();
+    const emitter: LifecycleEmitter = new LifecycleEmitter({
+      sink,
+      sessionId: 'sess',
+      source: SOURCE,
+      scope: { commandName: 'build', projectName: 'p', phaseName: '_phase:build' }
+    });
+
+    emitter.emitOperationRegistered({ operationId: 'op1' });
+
+    expect(sink.inputs[0].scope).toEqual({
+      commandName: 'build',
+      operationId: 'op1',
+      projectName: 'p',
+      phaseName: '_phase:build'
+    });
+  });
+
   it('emits diagnostics on the diagnosticEmitted channel with the privacy floor', () => {
     const sink: CapturingSink = new CapturingSink();
     const emitter: LifecycleEmitter = new LifecycleEmitter({ sink, sessionId: 'sess', source: SOURCE });
@@ -149,6 +168,30 @@ describe('summarizeShadowResult', () => {
     expect(summary.exitCode).toBe(1);
     expect(summary.operationCounts).toEqual({ success: 2, fromCache: 1, failure: 1 });
   });
+
+  it('derives failure from the session exit code when no command result exists', () => {
+    const summary: IShadowResultSummary = summarizeShadowResult([
+      ev('operationStatusChanged', { operationId: 'a', status: 'failure' }),
+      ev('sessionCompleted', { exitCode: 1 })
+    ]);
+
+    expect(summary.succeeded).toBe(false);
+    expect(summary.exitCode).toBe(1);
+  });
+
+  it('uses the final command result consistently', () => {
+    const events: IReporterEventEnvelope<unknown>[] = [
+      ev('commandResult', { commandName: 'build', succeeded: false, exitCode: 1 }),
+      ev('commandResult', { commandName: 'build', succeeded: true, exitCode: 0 })
+    ];
+
+    expect(deriveExitCodeFromEvents(events)).toBe(0);
+    expect(summarizeShadowResult(events)).toMatchObject({
+      commandName: 'build',
+      succeeded: true,
+      exitCode: 0
+    });
+  });
 });
 
 describe('shadow emission parity through the manager', () => {

libraries/reporter/src/test/Telemetry.test.ts

@@ -157,15 +157,63 @@ describe('TelemetrySubscriber', () => {
       expect(TELEMETRY_AGGREGATE_KEYS).toContain(key);
     }
   });
+
+  it('uses the root session completion as the final process result', async () => {
+    const telemetry: TelemetrySubscriber = new TelemetrySubscriber();
+    const manager: ReporterManager = new ReporterManager();
+    manager.addReporter(createTelemetryReporter(telemetry));
+    await manager.initializeAsync();
+
+    manager.emit(rawInput('commandResult', { commandName: 'build', succeeded: true, exitCode: 0 }));
+    manager.emit(rawInput('sessionCompleted', { exitCode: 1, durationMs: 2000 }));
+    await manager.flushAsync();
+
+    expect(telemetry.buildAggregate()).toMatchObject({
+      commandName: 'build',
+      result: 'failed',
+      exitCode: 1,
+      durationMs: 2000
+    });
+  });
+
+  it('does not let child session lifecycle events overwrite root command state', async () => {
+    const telemetry: TelemetrySubscriber = new TelemetrySubscriber();
+    const manager: ReporterManager = new ReporterManager();
+    manager.addReporter(createTelemetryReporter(telemetry));
+    await manager.initializeAsync();
+
+    manager.emit(rawInput('commandResult', { commandName: 'build', succeeded: false, exitCode: 1 }));
+    manager.emit(rawInput('sessionCompleted', { exitCode: 1, durationMs: 2000 }));
+    manager.emit({
+      ...rawInput('commandResult', { commandName: 'child-command', succeeded: true, exitCode: 0 }),
+      sessionId: 'child',
+      parentSessionId: 'sess'
+    });
+    manager.emit({
+      ...rawInput('sessionCompleted', { exitCode: 0, durationMs: 25 }),
+      sessionId: 'child',
+      parentSessionId: 'sess'
+    });
+    await manager.flushAsync();
+
+    expect(telemetry.buildAggregate()).toMatchObject({
+      commandName: 'build',
+      result: 'failed',
+      exitCode: 1,
+      durationMs: 2000
+    });
+  });
 });
 
 describe('createBeforeLogAdapter', () => {
-  it('runs legacy hooks with a plain copy of the aggregate', () => {
-    const observed: Record<string, unknown>[] = [];
+  it('returns hook augmentations without mutating the allowlisted aggregate', () => {
     const hook: LegacyBeforeLogHook = (telemetry: Record<string, unknown>) => {
-      observed.push(telemetry);
+      telemetry.result = 'failed';
+      telemetry.customField = 'custom-value';
+      (telemetry.operationStatusCounts as Record<string, number>).injected = 99;
     };
-    const adapter: (aggregate: ITelemetryAggregate) => void = createBeforeLogAdapter([hook]);
+    const adapter: (aggregate: ITelemetryAggregate) => Record<string, unknown> =
+      createBeforeLogAdapter([hook]);
 
     const aggregate: ITelemetryAggregate = {
       commandName: 'build',
@@ -176,11 +224,16 @@ describe('createBeforeLogAdapter', () => {
       diagnosticCategoryCounts: {},
       producerVersions: ['@microsoft/rush-lib@5.177.2']
     };
-    adapter(aggregate);
+    const record: Record<string, unknown> = adapter(aggregate);
 
-    expect(observed).toHaveLength(1);
-    expect(observed[0]).toEqual({ ...aggregate });
-    // The hook receives a copy, not the aggregate itself.
-    expect(observed[0]).not.toBe(aggregate);
+    expect(record).toMatchObject({
+      result: 'failed',
+      customField: 'custom-value',
+      operationStatusCounts: { success: 2, injected: 99 }
+    });
+    expect(aggregate.result).toBe('succeeded');
+    expect(aggregate.operationStatusCounts).toEqual({ success: 2 });
+    expect(record).not.toBe(aggregate);
+    expect(record.operationStatusCounts).not.toBe(aggregate.operationStatusCounts);
   });
 });