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

[chrisleekr]

Summary

Two area: observability follow-ups, shipped together. #177 emits installationId on the per-request child loggers (the 4 webhook handlers and the daemon job executor) so an operator triaging a per-installation GitHub rate-limit can pivot from a log line to its installation. #184 decouples the resolve-review-thread stdio MCP server from src/config: retry.ts was statically importing the root logger, which reads config at module load, so that subprocess ran full config zod-validation at startup. It was the last gap to "all stdio MCP servers config-free" (the other 5 were fixed in #172).

Diagram

flowchart LR
  retryBefore["retry.ts BEFORE"]:::node --> loggerBad["logger.ts<br/>reads config at load"]:::bad
  loggerBad --> configBad["config.ts<br/>zod validation at load"]:::bad
  mcpBad["resolve-review-thread.ts<br/>subprocess"]:::node -.->|"transitively pulls in config"| retryBefore

  retryAfter["retry.ts AFTER"]:::node --> redactGood["log-redaction.ts<br/>config-free"]:::good
  retryAfter -.->|"import type only, erased at emit"| loggerType["logger.ts Logger type"]:::node
  mcpGood["resolve-review-thread.ts<br/>subprocess"]:::node -->|"stays config-free"| retryAfter

  classDef node fill:#ecf0f1,color:#2c3e50
  classDef bad fill:#c0392b,color:#ffffff
  classDef good fill:#1e8449,color:#ffffff

Loading

Changes

#177 — installationId on child loggers

• Webhook handlers: undefined-safe installationId spread added to createChildLogger in issues.ts, pull-request.ts, issue-comment.ts, review-comment.ts. The comment-handler installation guards stay in place (the owner-allowlist drop line logs through log first, so the guard can't move up).
• Daemon: installationId is sourced from the orchestrator (it already resolves installation.id to mint the token in connection-handler.ts) and threaded as a new optional field on the job:payload WS schema → job-dispatcher.ts handleJobAccept → the daemon job-executor.ts child logger. PAT mode leaves it undefined (no per-installation bucket). Optional field keeps rolling deploys safe.
• Docs: new installationId row in the observability "Common log fields" table.
• Tests: job:payload round-trips the new field; a non-positive id is rejected.

#184 — config-free resolve-review-thread MCP server

• retry.ts drops the static logger as rootLogger import. It imports Logger as a type only (erased at emit, zero runtime coupling) and builds a config-free defaultLog from log-redaction.ts's REDACT_PATHS + errSerializer, same redaction parity. Writes to stderr (like createMcpLogger), so a default-path retry warning can't corrupt an MCP server's stdout JSON-RPC.
• resolve-review-thread.ts passes { log } at both retryWithBackoff sites for deliveryId correlation.
• Verified: the [config] WARNING marker is gone from dist/mcp/servers/resolve-review-thread.js (present on baseline before this change).

Related Issues

• Closes feat(observability): emit installationId on per-request child loggers for per-installation quota triage #177
• Closes refactor(observability): decouple resolve-review-thread MCP server from config via retry.ts logger import #184

Test plan

• Tested locally
• Added/updated tests
• All existing tests pass

Summary by CodeRabbit

• Documentation

  • Updated observability documentation with installationId field details for GitHub App installations.

• New Features

  • Enhanced logging with installation ID context for improved per-installation rate-limit triage and observability in GitHub App mode.

• Tests

  • Added validation tests for message schema with installationId field support.

[coderabbitai]

Warning

Review limit reached

@chrisleekr, we couldn't start this review because you've reached your PR review rate limit.

More reviews will be available in 35 minutes and 29 seconds. Learn how PR review limits work.

Your organization has run out of usage credits. Purchase more in the billing tab.

⌛ How to resolve this issue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available.

Please see our Fair Usage Limits Policy for further information.

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: ede915be-b5b6-4313-a852-76f95812139a

📥 Commits

Reviewing files that changed from the base of the PR and between 897aa67 and 1a9f373.

📒 Files selected for processing (4)

• src/mcp/mcp-logger.ts
• src/utils/log-redaction.ts
• src/utils/retry.ts
• test/utils/logger.test.ts

📝 Walkthrough

Walkthrough

This PR threads installationId from GitHub App installation metadata through job payloads and logs across the orchestrator, daemon, webhook handlers, and utilities to enable per-installation rate-limit and observability triage. The change is opt-in (absent in personal access token mode) and rolling-deployment compatible.

Changes

Installation ID Logging Context Threading

Layer / File(s)	Summary
Schema and type contracts src/shared/ws-messages.ts, src/orchestrator/job-dispatcher.ts	Added optional installationId?: number field to JobPayloadMessage schema and JobAcceptParams interface, with validation for positive integers.
Orchestrator job payload construction src/orchestrator/connection-handler.ts, src/orchestrator/job-dispatcher.ts	Connection handler captures GitHub App installation ID during token setup and forwards it into job dispatcher; dispatcher destructures and conditionally includes installationId in outgoing job:payload.
Retry utility logger foundation src/utils/retry.ts	New defaultLog pino logger reads LOG_LEVEL env var, applies consistent redaction and error serialization, and serves as default for retryWithBackoff logging without coupling to root logger.
Daemon job execution logging src/daemon/job-executor.ts	Job executor extracts installationId from payload and conditionally includes it in child logger context to tag rate-limit and execution logs with installation scope.
MCP GraphQL operation logging src/mcp/servers/resolve-review-thread.ts	Preflight validation and resolveReviewThread mutation now pass logger context to retryWithBackoff for consistent retry-level logging.
Webhook handler logging context src/webhook/events/issue-comment.ts, src/webhook/events/issues.ts, src/webhook/events/pull-request.ts, src/webhook/events/review-comment.ts	Four webhook handlers conditionally add installationId from payload.installation.id to their initial child logger context when present.
Schema validation and documentation test/shared/ws-messages.test.ts, docs/operate/observability.md	New tests verify positive and negative installationId schema validation; observability docs describe the field's GitHub App mode nature and absence in PAT mode.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related issues

• chrisleekr/github-app-playground#177: Direct implementation of installing per-request child logger installationId context and updating observability documentation.

Possibly related PRs

• chrisleekr/github-app-playground#89: src/utils/retry.ts reuses logger redaction exports (REDACT_PATHS and errSerializer) from this prior logger infrastructure PR.
• chrisleekr/github-app-playground#188: Both PRs modify src/orchestrator/connection-handler.ts and src/orchestrator/job-dispatcher.ts around job acceptance logging; this PR threads installationId context while the retrieved PR restructured event logging.

Suggested labels

type: feature ✨, type: docs 📋

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 62.50% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly captures the main objectives: adding installationId to loggers for observability (#177) and making the MCP retry config-free (#184). It accurately reflects the primary changes across multiple files.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[Copilot]

Pull request overview

This PR improves observability by adding installationId to per-request logging surfaces and removes the remaining config-loading path from the resolve-review-thread MCP server retry flow.

Changes:

• Adds optional installationId propagation through webhook loggers, orchestrator job payloads, daemon job execution, schema validation, docs, and tests.
• Reworks retryWithBackoff to use a config-free default pino logger with shared redaction primitives.
• Passes the MCP server logger into retryWithBackoff calls for delivery-correlated retry logs.

Reviewed changes

Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
src/webhook/events/issues.ts	Adds conditional installationId to issue event child logger.
src/webhook/events/pull-request.ts	Adds conditional installationId to pull request event child logger.
src/webhook/events/issue-comment.ts	Adds conditional installationId to issue comment child logger.
src/webhook/events/review-comment.ts	Adds conditional installationId to review comment child logger.
src/shared/ws-messages.ts	Extends job:payload schema with optional positive installationId.
test/shared/ws-messages.test.ts	Covers valid and invalid installationId parsing.
src/orchestrator/connection-handler.ts	Captures App installation id and forwards it in accepted jobs.
src/orchestrator/job-dispatcher.ts	Adds optional installationId to job accept payload construction.
src/daemon/job-executor.ts	Emits forwarded installationId on daemon child logger.
src/utils/retry.ts	Replaces root logger import with config-free pino default logger.
src/mcp/servers/resolve-review-thread.ts	Passes MCP logger into retry calls.
docs/operate/observability.md	Documents installationId as a common log field.

[coderabbitai]

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

src/utils/retry.ts (1)

36-37: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Update the stale doc comment.

The default is now the config-free defaultLog (stderr), not the root logger.

📝 Proposed wording fix

-  /** Optional scoped logger. Defaults to the root logger when omitted. */
+  /** Optional scoped logger. Defaults to the config-free stderr `defaultLog` when omitted. */
   log?: Logger;

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/utils/retry.ts` around lines 36 - 37, The doc comment on the optional
logger property `log?: Logger` is stale; update it to state that when omitted
the code uses the config-free `defaultLog` (which writes to stderr) rather than
the root logger—edit the comment above `log?: Logger` to mention `defaultLog`
(stderr) as the default and remove mention of the root logger so callers have
the correct expectation.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/utils/retry.ts`:
- Around line 18-25: The current construction of defaultLog passes
process.env["LOG_LEVEL"] directly to pino (in the defaultLog constant using
pino, Logger, REDACT_PATHS and errSerializer), which can throw at module import
if the env value is an unknown non-core level; validate the env value against
pino's core level names (e.g.,
"fatal","error","warn","info","debug","trace","silent") and if it is not one of
those, fall back to "info" before calling pino so that pino is never given an
invalid custom level string (alternatively accept a configured customLevels map
and require mapping only when present, but default to using a core level to
avoid import-time exceptions).

---

Outside diff comments:
In `@src/utils/retry.ts`:
- Around line 36-37: The doc comment on the optional logger property `log?:
Logger` is stale; update it to state that when omitted the code uses the
config-free `defaultLog` (which writes to stderr) rather than the root
logger—edit the comment above `log?: Logger` to mention `defaultLog` (stderr) as
the default and remove mention of the root logger so callers have the correct
expectation.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 64e8fa84-c736-4b6e-a0bd-fabfd4e3f7b2

📥 Commits

Reviewing files that changed from the base of the PR and between c615bc4 and 897aa67.

📒 Files selected for processing (12)

• docs/operate/observability.md
• src/daemon/job-executor.ts
• src/mcp/servers/resolve-review-thread.ts
• src/orchestrator/connection-handler.ts
• src/orchestrator/job-dispatcher.ts
• src/shared/ws-messages.ts
• src/utils/retry.ts
• src/webhook/events/issue-comment.ts
• src/webhook/events/issues.ts
• src/webhook/events/pull-request.ts
• src/webhook/events/review-comment.ts
• test/shared/ws-messages.test.ts

[chrisleekr]

🎉 This PR is included in version 1.14.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀