Skip to content

docs: Document the monitor entrypoint in CLAUDE.md - #123

Open
bgrozev wants to merge 1 commit into
mainfrom
docs-monitor-mode
Open

docs: Document the monitor entrypoint in CLAUDE.md#123
bgrozev wants to merge 1 commit into
mainfrom
docs-monitor-mode

Conversation

@bgrozev

@bgrozev bgrozev commented Aug 12, 2026

Copy link
Copy Markdown
Member

src/monitor.ts (added in #113, extended in #119) had no documentation anywhere in CLAUDE.md — not in Debugging Tools, not in Environment Variables Reference, not in the File Organization tree — despite being a full second image entrypoint (node dist/bundle/monitor.js) with its own env-var surface.

  • Adds a "Monitor Mode" subsection under Debugging Tools: what it does, how it's configured (MONITOR_URL, MONITOR_ATTEMPTS, MONITOR_DRAIN_SECONDS, etc.), and why the early-close/drain behavior from fix: Make the transcription monitor resilient to transient false alarms #119 exists (two production false-positive alerts were traced to the old fixed-length drain window closing the socket just before a slow-but-successful transcription's trailing final arrived).
  • Cross-references it from Environment Variables Reference (one line, avoids duplicating the var list).
  • Lists monitor.ts in the File Organization tree next to server.ts.

Also flags a real gap found while writing this (not fixed here, since it's a behavior change): monitor.ts doesn't import buildInfo, so a running monitor process can't self-report which commit it's from — no log line, no /metrics label, no info message. README.md's env-var table has the same MONITOR_* gap as CLAUDE.md did; left alone here to keep this PR docs-only and scoped to CLAUDE.md.

src/monitor.ts (added in #113, extended in #119) was never documented in
CLAUDE.md — no mention of it in Debugging Tools, Environment Variables
Reference, or the File Organization tree, despite being a full second image
entrypoint with its own env-var surface (MONITOR_URL, MONITOR_ATTEMPTS,
MONITOR_DRAIN_SECONDS, etc.).

Adds a Monitor Mode subsection under Debugging Tools covering what it does,
how it's configured, and the early-close/drain fix from #119 (including why
that fix exists — two production false-positive alerts traced to the fixed
drain window). Cross-references it from Environment Variables Reference and
lists monitor.ts in File Organization.

Also notes a real gap surfaced while writing this: monitor.ts doesn't import
buildInfo, so the running monitor process can't self-report which commit
it's from (no log line, no /metrics label, no info message) — left as a
note rather than fixed here, since it's a behavior change, not a doc fix.
@claude

claude Bot commented Aug 12, 2026

Copy link
Copy Markdown

Code Review

PR #123 — docs: Document the monitor entrypoint in CLAUDE.md

This is a documentation-only PR filling a genuine gap: src/monitor.ts was added in #113, hardened in #119, but had zero coverage in CLAUDE.md despite being a full production entrypoint with its own env-var surface.


Overview

The PR makes three targeted changes to CLAUDE.md:

  1. Adds monitor.ts to the File Organization tree
  2. Adds a "Monitor Mode" subsection under Debugging Tools
  3. Adds a one-line cross-reference in Environment Variables Reference

All three are correct and belong where they were placed.


What's Good

  • Accuracy against source PRs: Cross-checked the documented behavior against feat: Add a monitor mode that checks a transcription endpoint #113 (initial implementation) and fix: Make the transcription monitor resilient to transient false alarms #119 (drain/attempts hardening). Every env var, default value, and behavioral description is consistent with those PRs.
  • Root-cause context preserved: The explanation of why the early-close/drain design exists (two production false-positive alerts traced to the fixed-length drain closing the socket just before the trailing final arrived) is genuinely useful to a future maintainer deciding whether to change the behavior.
  • buildInfo gap called out explicitly: Flagging that monitor.ts doesn't import src/buildInfo.ts without making a behavior change here is exactly the right call — it's a real operational gap, correctly scoped out of a docs-only PR.
  • Scope discipline: Leaving README.md's MONITOR_* gap for a follow-up keeps this PR reviewable. The PR description explains the decision.

Suggestions / Issues

Minor

  • MONITOR_PORT / PORT fallback relationship is ambiguous. The bullet reads:

    MONITOR_PORT / PORT — port for the metrics HTTP server (default 8080)

    It's unclear which takes precedence, or whether they're true aliases. If the implementation reads MONITOR_PORT ?? PORT ?? 8080, say so. If PORT is the only env var and MONITOR_PORT is just a forward-looking alias, drop the slash notation. Readers configuring a shared-port container will have the wrong mental model until they read the source.

  • buildInfo workaround is vague. The note says:

    To identify which commit a running monitor sidecar is from, check the server.js bundle shipped in the same image (same build) instead.

    This is true but not actionable. A reader who's never done this before won't know how. Even something like "the GIT_HASH env var baked in at bundle time is in server.js; strings dist/bundle/server.js | grep -E '^[0-9a-f]{7}$' can surface it" would be more useful. Or, since this is a known gap, just link to the follow-up issue if one is filed.

  • "all attempts reuse the same sessionId" — clarify why. The doc states it but not the reason. The implication (landing on the warmed container so a cold-start isn't re-triggered on each retry) is explained in fix: Make the transcription monitor resilient to transient false alarms #119 and is important operational context. One clause would suffice: "…reuse the same sessionId (so retries land on the container already warmed by the first attempt, not a fresh cold start)".

Non-blocking observation

  • The PR description says README.md has the same gap. It would be worth either filing a follow-up issue for README.md (and linking it from the PR) or noting it in CLAUDE.md itself — e.g., a line like "See also: README.md (env-var table does not yet cover MONITOR_*)." Not a blocker, but the gap will stay invisible until someone notices.

Test Coverage

N/A — documentation only. No behavioral changes.


Verdict

Approve with minor suggestions. The documentation is accurate, well-scoped, and adds real value. The MONITOR_PORT/PORT ambiguity is the only item I'd push back on before merging; the rest are enhancements.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant