my-pi/pi-permission-system/docs/plans/0336-inject-session-logger.md

issue, issue_title

issue	issue_title
336	Make the logger injectable; drop createSessionLogger(runtime)

Make the logger injectable; drop createSessionLogger(runtime)

Problem Statement

createSessionLogger(runtime) (src/session-logger.ts) captures the entire ExtensionRuntime and reaches through it to talk to strangers — runtime.writeDebugLog, runtime.writeReviewLog, and runtime.runtimeContext?.ui.notify — a Law-of-Demeter violation. The runtime factory (src/runtime.ts) compounds the smell: it stubs writeDebugLog / writeReviewLog as () => {}, builds the JSONL writer plus a warning-dedup reporter, then reassigns the two methods afterward (a forward reference / temporal coupling). The composition root (src/index.ts) then threads the same logging surface through five .bind(runtime) adapter closures.

This is Phase 4 Step 3 (Track B: De-god the runtime) from docs/architecture/architecture.md — the second link in the chain ConfigStore → injectable logger → dissolve runtime → collapse index.ts closures. It depends on the ConfigStore extraction (Step 2, #335, complete) for the debug toggle. It is behavior-preserving.

Goals

• Repurpose createSessionLogger to take narrow dependencies — the logs directory, a config reader (for the debug/review write toggles), and a notify sink — instead of the whole ExtensionRuntime.
• Fold the JSONL-writer composition (createPermissionSystemLogger), the warning-dedup reporter, and the warn notify path into the single createSessionLogger factory, so one object owns the complete SessionLogger contract (debug / review / warn).
• Remove writeDebugLog / writeReviewLog from ExtensionRuntime; expose the built logger as runtime.logger instead.
• Remove the runtime.writeDebugLog / runtime.runtimeContext?.ui.notify reach-through from session-logger.ts, and drop the five .bind(runtime) logging adapters in index.ts.
• Behavior-preserving: no observable change to debug/review log writing, the config toggles, warning deduplication, or warning notification.

Non-Goals

• Moving the runtime context (runtimeContext) ownership off the runtime. The notify sink keeps reading the still-runtime-owned context through the transitional RuntimeContextRef seam introduced in #335; context unification onto PermissionSession is Step 4 (#337).
• Dissolving ExtensionRuntime or moving the logger / ConfigStore construction out of the factory and into index.ts — Step 4 (#337). The logger is built in the factory because it is mutually entangled with ConfigStore, which the factory owns until #337.
• Collapsing the remaining index.ts deps bags and unifying the writeReviewLog field naming across the forwarder / prompter / RPC consumers — Step 5 (#338).
• Renaming the SessionLogger interface methods (debug / review / warn) or the consumer fields (writeReviewLog / writeDebugLog).

Background

Relevant modules:

• src/session-logger.ts — defines the SessionLogger interface (debug / review / warn, all void) and createSessionLogger(runtime), which delegates debug / review to runtime.writeDebugLog / writeReviewLog and warn to runtime.runtimeContext?.ui.notify(message, "warning").
• src/logging.ts — createPermissionSystemLogger({ getConfig, debugLogPath, reviewLogPath, ensureLogsDirectory }) returns a PermissionSystemLogger whose debug / review write a JSONL line (gated on config.debugLog / config.permissionReviewLog) and return a warning string on failure. This module has no ExtensionRuntime reference today.
• src/runtime.ts — createExtensionRuntime() builds the PermissionSystemLogger, owns the reportedLoggingWarnings dedup Set + reportLoggingWarning helper (which calls runtime.runtimeContext?.ui.notify), and assigns runtime.writeDebugLog / writeReviewLog (the stub-then-reassign pattern). ConfigStore is constructed here too, with a deferred-binding ConfigStoreLogger that points at runtime.writeDebugLog / writeReviewLog. The contextRef: RuntimeContextRef seam (get/set over runtime.runtimeContext) already exists for ConfigStore.
• src/index.ts — calls createSessionLogger(runtime) once (passed as PermissionSession's logger), and threads runtime.writeReviewLog.bind(runtime) / runtime.writeDebugLog.bind(runtime) into the PermissionForwarder logger + writeReviewLog, the PermissionPrompter, and the RPC handlers (five .bind sites total).
• src/config-store.ts — ConfigStore implements ConfigReader (current()); its ConfigStoreLogger dep is { writeDebugLog, writeReviewLog }. Its refresh / save / logResolvedPaths write through that sink.
• src/decision-reporter.ts — GateDecisionReporter holds a SessionLogger and calls .review; built in index.ts from session.logger. Unchanged.
• src/handlers/lifecycle.ts — the sole SessionLogger.warn caller (this.session.logger.warn(issue)). Unchanged.

Consumer logging-field shapes (all preserved):

• ForwardedPermissionLogger (src/forwarded-permissions/io.ts): { writeReviewLog, writeDebugLog }.
• PermissionPrompterDeps.writeReviewLog, the RPC handler deps writeReviewLog, and forwardingDeps.writeReviewLog: bare (event, details) => void.

Constraints from AGENTS.md / the package skill:

• Inject the new collaborator with a narrow interface, not the concrete runtime, so test doubles need no as unknown as ExtensionRuntime cast.
• Do not read getAgentDir() / process.* inside the factory function — globalLogsDir is passed in.
• Keep business logic at the edges: createSessionLogger composes the existing createPermissionSystemLogger rather than re-implementing JSONL writing.
• The package skill does not name createSessionLogger, writeDebugLog, or writeReviewLog, so no skill edit is required.

The logger ↔ ConfigStore cycle

The logger needs the config (to read the debugLog / permissionReviewLog toggles at write time); ConfigStore needs the logger (to write config.loaded / config.saved / config.resolved entries). Today this is broken with the stub-then-reassign forward reference. This plan breaks it cleanly with a lazy config read: build the logger first with getConfig: () => configStore.current() (a thunk, called only at write time), then build ConfigStore with the fully-constructed logger's methods. The logger object is complete when ConfigStore is constructed; only the config value is read lazily, which is correct because config changes across the session. No method is stubbed-then-reassigned.

Design Overview

The injectable createSessionLogger

createSessionLogger becomes the single home for the full SessionLogger contract: it composes the JSONL writer, owns the warning-dedup Set, and routes both IO-failure warnings and explicit warn calls through the injected notify sink.

export interface SessionLogger {
  debug(event: string, details?: Record<string, unknown>): void;
  review(event: string, details?: Record<string, unknown>): void;
  warn(message: string): void;
}

export interface SessionLoggerDeps {
  /** Root logs directory; the debug + review log file paths derive from it. */
  globalLogsDir: string;
  /** Reads current config for the debug/review write toggles (call-time). */
  getConfig: () => PermissionSystemExtensionConfig;
  /** Surfaces a warning message to the user; read at call time. */
  notify: (message: string) => void;
}

export function createSessionLogger(deps: SessionLoggerDeps): SessionLogger {
  const writer = createPermissionSystemLogger({
    getConfig: deps.getConfig,
    debugLogPath: join(deps.globalLogsDir, DEBUG_LOG_FILENAME),
    reviewLogPath: join(deps.globalLogsDir, REVIEW_LOG_FILENAME),
    ensureLogsDirectory: () =>
      ensurePermissionSystemLogsDirectory(deps.globalLogsDir),
  });

  const reported = new Set<string>();
  const report = (warning: string): void => {
    if (reported.has(warning)) return;
    reported.add(warning);
    deps.notify(warning);
  };

  return {
    debug: (event, details) => {
      const warning = writer.debug(event, details);
      if (warning) report(warning);
    },
    review: (event, details) => {
      const warning = writer.review(event, details);
      if (warning) report(warning);
    },
    warn: (message) => deps.notify(message),
  };
}

The returned methods are standalone closures (no this), so consumers can pass logger.review / logger.debug as bare references with no .bind.

Construction in the factory

createExtensionRuntime() builds the logger before ConfigStore, using a lazy getConfig thunk and the existing contextRef seam for the notify sink:

let configStore: ConfigStore;

const logger = createSessionLogger({
  globalLogsDir: paths.globalLogsDir,
  getConfig: () => configStore.current(),
  notify: (message) =>
    runtime.runtimeContext?.ui.notify(message, "warning"),
});

configStore = new ConfigStore({
  agentDir,
  context: contextRef,
  policyPaths: permissionManager,
  logger: { writeDebugLog: logger.debug, writeReviewLog: logger.review },
});

runtime.configStore = configStore;
runtime.logger = logger;

ExtensionRuntime drops writeDebugLog / writeReviewLog and gains logger: SessionLogger. The () => {} stubs, the post-construction reassignment, the reportedLoggingWarnings Set, the reportLoggingWarning helper, and the createPermissionSystemLogger import all leave runtime.ts.

Note: the notify sink reads runtime.runtimeContext at call time (matching today's reportLoggingWarning and warn behavior). The contextRef seam may equally be used (contextRef.get()?.ui.notify(...)); both read the same field.

Consumer call sites (no .bind)

index.ts reads runtime.logger and passes the bound closures directly:

// PermissionSession logger arg
new PermissionSession(runtime, runtime.logger, /* … */);

// PermissionForwarder
logger: { writeReviewLog: runtime.logger.review, writeDebugLog: runtime.logger.debug },
writeReviewLog: runtime.logger.review,

// PermissionPrompter
writeReviewLog: runtime.logger.review,

// RPC handlers
writeReviewLog: runtime.logger.review,

The writeReviewLog / writeDebugLog field names on the consumer deps stay (they are mapped to logger.review / logger.debug values); unifying the naming is deferred to #338.

Edge cases (all preserved)

• warn is never deduplicated (explicit warnings always notify); only IO-failure warnings flow through the dedup Set.
• The notify sink is a no-op when runtimeContext is null (early-session) — ?.ui.notify short-circuits, exactly as today.
• The dedup Set lives for the lifetime of the logger (one per createExtensionRuntime call), matching today's per-runtime Set.
• The debug/review toggles are read at write time via getConfig, so a config reload mid-session changes logging behavior with no rebuild — unchanged.

Extracted-module upstream check

session-logger.ts gains imports for join (node:path), DEBUG_LOG_FILENAME / REVIEW_LOG_FILENAME (config-paths.ts), ensurePermissionSystemLogsDirectory + PermissionSystemExtensionConfig (extension-config.ts), and createPermissionSystemLogger (logging.ts) — all of which runtime.ts already imports for this work; they move, not duplicate. session-logger.ts no longer imports runtime.ts (the LoD reach-through is gone), so the dependency edge runtime.ts → session-logger.ts is one-way with no cycle. No output-argument mutation is carried over: the logger mutates only its private dedup Set; the notify sink is an injected callback, not a reached-into bag.

Module-Level Changes

• src/session-logger.ts
  • Rewrite createSessionLogger to accept SessionLoggerDeps (globalLogsDir, getConfig, notify); compose createPermissionSystemLogger, own the dedup Set + reporter, and implement warn via notify.
  • Add the SessionLoggerDeps interface export.
  • Drop the import type { ExtensionRuntime }.
• src/runtime.ts
  • Remove writeDebugLog / writeReviewLog from the ExtensionRuntime interface; add logger: SessionLogger.
  • In the factory: build the logger via createSessionLogger({ globalLogsDir, getConfig: () => configStore.current(), notify }) before ConfigStore; pass { writeDebugLog: logger.debug, writeReviewLog: logger.review } as the ConfigStoreLogger; set runtime.logger.
  • Delete the () => {} stubs, the post-construction writeDebugLog/writeReviewLog reassignment, the reportedLoggingWarnings Set, reportLoggingWarning, and the createPermissionSystemLogger import (now unused here); import createSessionLogger + SessionLogger from session-logger.ts.
• src/index.ts
  • Drop import { createSessionLogger }.
  • Pass runtime.logger to PermissionSession.
  • Replace the five runtime.writeReviewLog.bind(runtime) / runtime.writeDebugLog.bind(runtime) adapters with runtime.logger.review / runtime.logger.debug.
• test/session-logger.test.ts — rewrite for the new signature (plain fakes: getConfig, notify, a temp globalLogsDir); cover the toggles, the success write, the IO-failure warning + dedup, and warn direct-notify.
• test/runtime.test.ts — re-point the logger mock from ../src/logging to ../src/session-logger (mock createSessionLogger); delete the writeDebugLog / writeReviewLog delegation + dedup + notify tests (now owned by session-logger.test.ts); assert runtime.logger is the object the factory built and that createSessionLogger is called with a globalLogsDir-derived value and a getConfig reading configStore.current().

Grep confirms runtime.writeDebugLog / runtime.writeReviewLog are referenced only in src/runtime.ts, src/index.ts, and test/runtime.test.ts; createSessionLogger only in src/index.ts (call), src/session-logger.ts (def), and test/session-logger.test.ts. The SessionLogger interface (debug / review / warn) is unchanged, so decision-reporter.ts, handlers/lifecycle.ts, permission-session.ts, session-lifecycle-session.ts, handler-fixtures.ts, and gate-fixtures.ts need no edits.

Doc updates: docs/architecture/architecture.md line 567 describes session-logger.ts as the "SessionLogger interface + createSessionLogger() factory" — still accurate, optionally clarified to "(composes the JSONL writer, warning dedup, and notify sink)". The Phase 4 health-metrics table is a phase-start snapshot, not a live count (per the #335 plan), so it is not edited here. The roadmap Step 3 ✓ complete mark is appended at ship time, not during planning.

Test Impact Analysis

1. New unit tests enabled: session-logger.test.ts now constructs createSessionLogger with plain fakes — no as unknown as ExtensionRuntime. It can exercise the toggle gating, JSONL write success (temp dir), the IO-failure warning path (drive a failure via a non-writable globalLogsDir or by asserting on a fake notify), warning deduplication, and the un-deduplicated warn path — all without the runtime god object.
2. Redundant tests: the runtime.test.ts writeDebugLog / writeReviewLog delegation, dedup, and notify tests become redundant — the behavior they covered now lives in createSessionLogger and is tested in session-logger.test.ts. They are deleted when the runtime stops owning those methods.
3. Tests that must stay: the runtime.test.ts path-derivation, piInfrastructureDirs, default-state, and configStore.current() tests (unaffected); the config-store.test.ts logging-sink assertions (the sink contract — { writeDebugLog, writeReviewLog } — is unchanged); the logging.test.ts / config-reporter.test.ts createPermissionSystemLogger tests (the JSONL writer is unchanged and is now composed, not bypassed).

TDD Order

This is a signature change on createSessionLogger plus removal of two ExtensionRuntime fields, which break every consumer at the type level simultaneously. Lift-and-shift across two steps keeps each commit small and the repo green between them: Step 1 introduces the new logger object and exposes runtime.logger while keeping the old runtime methods as thin delegators; Step 2 removes the old methods and the .bind adapters.

1. Inject the new createSessionLogger; expose runtime.logger — refactor:

   • Coupled step: createSessionLogger's signature changes (sole call site index.ts line 72), so it cannot land in isolation.
   • Red: rewrite test/session-logger.test.ts for SessionLoggerDeps — assert toggle gating, success write (temp dir), IO-failure warning + dedup, and warn direct-notify.
   • Green:
     • Rewrite createSessionLogger to SessionLoggerDeps (compose the writer, own the dedup Set, implement warn).
     • In runtime.ts, build the logger via the new factory and set runtime.logger; keep writeDebugLog / writeReviewLog on the runtime as thin delegators ((e, d) => logger.debug(e, d) / review) so index.ts's .bind sites still compile; point the ConfigStoreLogger at { writeDebugLog: logger.debug, writeReviewLog: logger.review }; delete the stubs, the reassignment, the dedup Set, and reportLoggingWarning.
     • In index.ts, swap createSessionLogger(runtime) → runtime.logger (drop the import).
     • In test/runtime.test.ts, re-point the mock to ../src/session-logger; move the dedup/notify/delegation assertions out (now in session-logger.test.ts) but keep writeDebugLog/writeReviewLog delegation smoke checks if still present this step.
   • Run pnpm run check + the full suite.
   • Commit: refactor: build an injectable SessionLogger in the runtime factory.

2. Remove the runtime logging methods and the .bind adapters — refactor:

   • Coupled step: removing the two fields breaks every .bind site at the type level; fold all consumer + test edits in.
   • Green:
     • index.ts: replace the five runtime.writeReviewLog.bind(runtime) / runtime.writeDebugLog.bind(runtime) with runtime.logger.review / runtime.logger.debug.
     • runtime.ts: remove writeDebugLog / writeReviewLog from the ExtensionRuntime interface and the factory (the ConfigStoreLogger already uses logger.debug / logger.review).
     • test/runtime.test.ts: delete the writeDebugLog / writeReviewLog tests; add/keep the runtime.logger assertion and the createSessionLogger call-args assertion.
   • Run pnpm run check, the full suite, and pnpm fallow dead-code to confirm no orphaned exports (e.g., an now-unused logging.ts re-export).
   • Commit: refactor: drop runtime logging methods and index .bind adapters.

Risks and Mitigations

• Risk: the lazy getConfig: () => configStore.current() thunk is invoked before configStore is assigned, throwing a TDZ / undefined error. Mitigation: getConfig is only called at log write time (inside writer.debug / review), never during construction; configStore is assigned synchronously on the next statement, and the first log write happens at session time. Covered by the runtime.test.ts "getConfig reads configStore.current()" assertion.
• Risk: passing logger.review / logger.debug as bare references loses this and misfires. Mitigation: the methods are arrow-function closures over writer / report / deps with no this dependency; the existing decision-reporter.ts already passes session.logger around as a value.
• Risk: the dedup Set semantics change when moved from the runtime into the logger. Mitigation: identical Set-membership logic, identical per-runtime lifetime (one logger per factory call); the dedup tests move verbatim to session-logger.test.ts.
• Risk: a logging consumer outside grep's reach breaks when the runtime fields are removed. Mitigation: grep is clean (runtime.writeDebugLog / writeReviewLog only in runtime.ts, index.ts, runtime.test.ts); Step 2 runs pnpm fallow dead-code as a backstop; Step 1's delegators keep index.ts green until Step 2.

Open Questions

• Should the writeReviewLog / writeDebugLog consumer field names unify to review / debug (so the logger satisfies the deps directly with no name mapping)? Deferred to #338 (the index.ts deps-bag collapse), which owns the consumer-deps churn.
• Should the logger construction move from the factory into index.ts? Deferred to #337, which dissolves ExtensionRuntime and relocates ConfigStore + logger construction to the composition root together.