feat(observability): installationId on loggers + config-free MCP retry (#189)

Closes #177
Closes #184

Author: chrisleekr

docs/operate/observability.md

@@ -274,8 +274,15 @@ export async function executeJob(
     return;
   }
 
-  const { installationToken, maxTurns, allowedTools, envVars, memory, reviewLearnings } =
-    payload.payload;
+  const {
+    installationToken,
+    installationId,
+    maxTurns,
+    allowedTools,
+    envVars,
+    memory,
+    reviewLearnings,
+  } = payload.payload;
 
   // Abort controller for cancel/execute race prevention (C1)
   const abortController = new AbortController();
@@ -316,6 +323,9 @@ export async function executeJob(
       owner: context.owner,
       repo: context.repo,
       entityNumber: context.entityNumber,
+      // Per-installation rate-limit triage (#177). Orchestrator-sourced; absent
+      // in PAT mode, so emit conditionally.
+      ...(installationId !== undefined ? { installationId } : {}),
     });
 
     // Build the full BotContext. `envVars` is threaded through the context so

src/daemon/job-executor.ts

@@ -14,13 +14,15 @@
  */
 import pino from "pino";
 
-import { errSerializer, REDACT_PATHS } from "../utils/log-redaction";
+import { errSerializer, REDACT_PATHS, resolveLogLevel } from "../utils/log-redaction";
 
 export function createMcpLogger(serverName: string): pino.Logger {
   const deliveryId = process.env["DELIVERY_ID"];
   return pino(
     {
-      level: process.env["LOG_LEVEL"] ?? "info",
+      // resolveLogLevel falls back to `info` on an invalid LOG_LEVEL so pino
+      // can't throw at construction in this config-free subprocess (#184).
+      level: resolveLogLevel(process.env["LOG_LEVEL"]),
       base: {
         server: serverName,
         ...(deliveryId !== undefined && deliveryId !== "" ? { deliveryId } : {}),

src/mcp/mcp-logger.ts

@@ -170,8 +170,9 @@ server.tool(
       // to 3 attempts) is actually honoured. The helper short-circuits
       // on non-retriable 4xx (e.g. 404 thread_not_found) so callers
       // still see prompt failures.
-      const preflight = await retryWithBackoff(() =>
-        octokit.graphql<PreflightResponse>(GET_THREAD_QUERY, { threadId: thread_id }),
+      const preflight = await retryWithBackoff(
+        () => octokit.graphql<PreflightResponse>(GET_THREAD_QUERY, { threadId: thread_id }),
+        { log },
       );
       const preflightPr = preflight.node?.pullRequest.number;
       const preflightRepo = preflight.node?.pullRequest.repository;
@@ -205,8 +206,9 @@ server.tool(
         };
       }
 
-      const result = await retryWithBackoff(() =>
-        octokit.graphql<ResolveResponse>(RESOLVE_MUTATION, { threadId: thread_id }),
+      const result = await retryWithBackoff(
+        () => octokit.graphql<ResolveResponse>(RESOLVE_MUTATION, { threadId: thread_id }),
+        { log },
       );
       const thread = result.resolveReviewThread.thread;
 

src/mcp/servers/resolve-review-thread.ts

@@ -907,6 +907,10 @@ async function handleAccept(
     // (per-repo config fetch, 1.5.F) can reuse it without re-minting.
     let token: string;
     let acceptOctokit: Octokit;
+    // App mode only: the installation id used to mint the token. Forwarded into
+    // job:payload so the daemon's child logger can emit it (#177). Stays
+    // undefined in PAT mode (no per-installation bucket to triage).
+    let installationId: number | undefined;
     if (config.githubPersonalAccessToken !== undefined) {
       token = config.githubPersonalAccessToken;
       acceptOctokit = new Octokit({ auth: token });
@@ -916,6 +920,7 @@ async function handleAccept(
         owner,
         repo,
       });
+      installationId = installation.id;
       acceptOctokit = await app.getInstallationOctokit(installation.id);
       token = await resolveGithubToken(acceptOctokit);
     }
@@ -961,6 +966,7 @@ async function handleAccept(
       daemonId,
       deliveryId: offer.deliveryId,
       installationToken: token,
+      ...(installationId !== undefined ? { installationId } : {}),
       contextJson,
       ...(maxTurns !== undefined ? { maxTurns } : {}),
       allowedTools: resolveAllowedTools(

src/orchestrator/connection-handler.ts

@@ -467,6 +467,10 @@ export interface JobAcceptParams {
   daemonId: string;
   deliveryId: string;
   installationToken: string;
+  /** GitHub App installation id (App mode only; absent in PAT mode). Forwarded
+   * into `job:payload` so the daemon's child logger can emit it for
+   * per-installation rate-limit triage (#177). */
+  installationId?: number;
   contextJson: Record<string, unknown>;
   /** Optional turn cap. Omitted = no cap (the SDK runs the agent to completion). */
   maxTurns?: number;
@@ -501,6 +505,7 @@ export function handleJobAccept({
   daemonId,
   deliveryId,
   installationToken,
+  installationId,
   contextJson,
   maxTurns,
   allowedTools,
@@ -527,6 +532,7 @@ export function handleJobAccept({
       payload: {
         context: contextJson,
         installationToken,
+        ...(installationId !== undefined ? { installationId } : {}),
         ...(maxTurns !== undefined ? { maxTurns } : {}),
         allowedTools,
         ...(Object.keys(envVars).length > 0 ? { envVars } : {}),

src/orchestrator/job-dispatcher.ts

@@ -183,6 +183,11 @@ const jobPayloadSchema = z.object({
   payload: z.object({
     context: z.record(z.string(), z.unknown()),
     installationToken: z.string(),
+    /** GitHub App installation id (App mode only; absent in PAT mode). The
+     * daemon emits it on its child logger for per-installation rate-limit
+     * triage (#177). Optional for rolling-deploy safety: an orchestrator that
+     * omits it must not fail a newer daemon's parse. */
+    installationId: z.number().int().positive().optional(),
     maxTurns: z.number().int().positive().optional(),
     allowedTools: z.array(z.string()),
     envVars: z.record(z.string(), z.string()).optional(),

src/shared/ws-messages.ts

@@ -13,6 +13,22 @@ import pino, { type SerializedError } from "pino";
 
 import { redactGitHubTokens } from "./sanitize";
 
+const VALID_PINO_LEVELS = new Set(["trace", "debug", "info", "warn", "error", "fatal", "silent"]);
+
+/**
+ * Resolve a pino `level` from a raw env value, config-free.
+ *
+ * pino 10 throws at construction on a non-core level string (e.g. a typo'd
+ * `LOG_LEVEL`), so a logger built straight from `process.env` would crash at
+ * module import. The main process is protected by config's zod enum, but the
+ * stdio MCP subprocesses and `retryWithBackoff`'s default logger deliberately
+ * do NOT load config, so they validate here and fall back to `info` on anything
+ * unrecognised. See issue #184.
+ */
+export function resolveLogLevel(raw: string | undefined): string {
+  return raw !== undefined && VALID_PINO_LEVELS.has(raw) ? raw : "info";
+}
+
 /**
  * Path-based redaction list for pino loggers.
  *

src/utils/log-redaction.ts

@@ -1,4 +1,29 @@
-import { type Logger, logger as rootLogger } from "../logger";
+import pino from "pino";
+
+import type { Logger } from "../logger";
+import { errSerializer, REDACT_PATHS, resolveLogLevel } from "./log-redaction";
+
+// Config-free default logger (issue #184). Importing retry.ts must not pull in
+// src/logger.ts -> src/config, so the stdio MCP servers that use retry (e.g.
+// resolve-review-thread) stay config-free. `import type { Logger }` above is
+// erased at emit, so it adds no runtime coupling. Same REDACT_PATHS +
+// errSerializer as the root logger keeps redaction parity. The level reads
+// LOG_LEVEL directly (config is intentionally not imported) via resolveLogLevel,
+// which falls back to `info` on an invalid value so pino can't throw at import.
+// Level visibility matches the root logger because both derive from LOG_LEVEL.
+//
+// Writes to stderr (like createMcpLogger), NOT pino's default stdout: a stdio
+// MCP server speaks JSON-RPC over stdout, so a default-path retry warning on
+// stdout would corrupt the protocol. stderr is safe in every context (k8s
+// ships both streams; warn/error on stderr is conventional).
+const defaultLog: Logger = pino(
+  {
+    level: resolveLogLevel(process.env["LOG_LEVEL"]),
+    redact: { paths: [...REDACT_PATHS] },
+    serializers: { err: errSerializer },
+  },
+  process.stderr,
+);
 
 /**
  * Retry configuration options.
@@ -60,7 +85,7 @@ export async function retryWithBackoff<T>(
     initialDelayMs = 5000,
     maxDelayMs = 20000,
     backoffFactor = 2,
-    log = rootLogger,
+    log = defaultLog,
   } = options;
 
   // Fail fast on invalid input. Each check names the offending option and

src/utils/retry.ts

@@ -53,6 +53,11 @@ export function handleIssueComment(
     // so the canonical `entityNumber` covers both surfaces under one field.
     entityNumber: payload.issue.number,
     senderLogin,
+    // Per-installation rate-limit triage (#177). Conditional because this
+    // logger is built before the `payload.installation === undefined` guard
+    // below; the guard can't move up (the owner-allowlist drop line logs
+    // through `log` first).
+    ...(payload.installation !== undefined ? { installationId: payload.installation.id } : {}),
   });
 
   const auth = isOwnerAllowed(ownerLogin, log);