test(logging): pin the caller-impact rule for the rotation-inheritance fix (#1877/#1880)

[kriszyp]

Pins the caller-impact rule for the v5.2.15 -> v5.3.0 logger rotation-inheritance fix: which logger usage shapes it changes, with tests and a decision rule a later static scanner can apply mechanically.

⊙ Problem

Harper is building an upgrade-impact pipeline: a release step's diff is graded, then any app that uses the changed API is flagged as potentially affected by a static pattern match — not verified. v5.2.15 → v5.3.0 changed updateLogger() (utility/logging/harper_logger.ts:120-174) so a tagged/component logger with no rotation: block of its own now inherits main's rotation policy instead of silently losing rotation. This task pins down exactly which logger-usage shapes that change affects, as executable tests plus a decision rule a later static scanner can apply mechanically to real application code. No application code is touched; this is research + tests only on HarperFast/harper itself.

❓ Your call: as specified — tests/research only, no runtime behavior change. The task's own "Design assessment" line already scoped this as having no design triggers (no API surface change), which I confirmed by re-reading the diff: updateLogger/getFileLogger are unchanged in signature, only their internal fallback logic changed.

💡 Solution

(a) Trigger conditions

A logger's rotation changes under this fix exactly when all of:

1. updateLogger() is actually called on it — not merely true for every logger (see shape table: a forComponent()-created logger that has no corresponding config section is never passed through updateLogger at all).
2. That call's logOptions.rotation is null/omitted (not {}, which is a present-but-empty override that wins outright).
3. The logger being updated is not the main logger itself (self-reference is excluded — :123-124 — so main can still clear its own rotation by omitting the block).
4. mainLoggerRef.rotation (main's live policy at that moment) is non-null.

When all four hold, the logger's rotation becomes mainLoggerRef.rotation, with rotation.path stripped if the logger's own resolved file path differs from main's (avoids an EXDEV cross-device rename — :139-158).

(b) Caller-shape table

Shape	Status	Synthetic example
Plain logger.info/warn/error(...), no logging.external: config	unaffected	logger.info('hi') — sharing main's default path, bound to main's rotation via the shared per-path file-sink cache, same before and after
logger.withTag(tag) / loggerWithTag()	unaffected	logger.withTag('ingest').info('hi') — never calls updateLogger, writes through whatever sink the wrapped logger already has
Level filtering / tag-as-component-name resolution	unaffected	n/a — the diff only renamed the hardcoded mainLogger reference to the parameterized mainLoggerRef on those two lines; production never passes the new 4th argument, so the resolved value is identical
A named component/app config with its own logging: block specifying path/root but no rotation:	affected (behavior change) — the #1877 fix itself	myApp:\n logging:\n path: /app/log/myApp.log
An explicit logging.external: block with no rotation:	affected (behavior change)	logging:\n external:\n path: /app/log/external.log
A component config with its own explicit rotation: (even {enabled: false})	unaffected	logging:\n path: /app/log/x.log\n rotation:\n enabled: false — explicit policy always wins, before and after
Two loggers resolving to the same file path, one explicit the other inherited	undecidable from one component's config alone	arbitration depends on config-wide key order across the whole file, not any single block — see does not let an inherited main policy replace an explicit component policy on the shared sink / restores the inherited main policy when an explicit component policy is removed
Ops tooling tailing/reading a log file from the "affected" row above, by an already-open descriptor	affected (breaks)	a tail -f-style process that opened the fd before the file first rotates stops seeing new writes once the file is renamed out from under it

(c) Intended fix or accidental regression?

Intended fix, with direct evidence:

• GitHub issue #1877 — a real report: logging.external.path grew to 200G because, per the issue's own framing, "because that is an external log it doesn't respect the default logging configuration for rotation."
• PR #1880 fixed it, with a commit message stating the pre-existing docs contract explicitly: "the docs say components default to main logging unless overridden." The bug was updateLogger() doing logger.rotation = logOptions.rotation unconditionally, with no fallback — the inheritance pattern every other option (level, path, tag) already had.
• A prior Claude QA investigation (recorded on Log rotation maxSize not respected properly #1877, 2026-08-30) independently reached the identical root-cause diagnosis before the fix shipped, corroborating it wasn't a surprise regression nobody understood.

The one real behavior-change risk this introduces: any component that had its own path and no rotation block because it relied on the old (buggy) no-rotation behavior as a feature — e.g., a side log it deliberately never wanted pruned — now gets main's rotation by default. Main's own rotation.enabled defaults to true with maxSize: 64M (static/defaultConfig.yaml:48), so this is a default-on change for any such component, not something that requires explicit opt-in.

(d) Static-detection recipe

The trigger is configuration-shaped, not code-shaped — the mechanism lives entirely in updateLogger's handling of a component's config, not in any JS/TS call site an app could write differently. A scanner should read the app's own harperdb-config.yaml/manifest, not its source:

• Flag: any logging: block other than the top-level one — i.e. nested under a named top-level component key, or the external: key nested under the top-level logging: — that contains a path: or root: key but no sibling rotation: key at the same indent level, provided the deployment's top-level logging.rotation.enabled is not explicitly false.
• Mechanically: parse the YAML (don't regex raw text — nesting defeats line-oriented regex), then for every key X in the parsed tree where X is a top-level component name or external: flag if (X.logging.path or X.logging.root is set) and X.logging.rotation is absent, and logging.rotation.enabled !== false at the root.
• False positives:
  • The block omits rotation:, but the top-level logging.rotation is disabled or main has no rotation configured — nothing to inherit, no real behavior change despite matching the pattern.
  • The component's path/root resolves to the exact same file as main's own log (shared-sink case) — it was already bound to main's rotation via the shared per-path cache before this fix too (see unaffected row above), so flagging it overstates the impact.
• False negatives:
  • rotation: {} (present but empty) passes a naive "key exists" check, but it still disables the write-path size guard (no maxSize) — a real behavior point a presence-only check misses.
  • Config supplied entirely through env vars or a programmatic merge rather than the YAML text a scanner reads is invisible to any static text scan.
  • logger.forComponent(name) called from app code with no matching YAML section is correctly not flagged by this recipe (true negative — see the unaffected table row), but a scanner that instead keys off source call sites rather than config would miss the real trigger entirely and risk flagging this harmless shape instead.

(e) Does main still behave as v5.3.0 did?

Yes, unchanged. git diff v5.3.0..origin/main -- utility/logging/ touches only harper_logger.ts, logRotation.ts, logRotator.ts, and logGenerationCoordinator.ts for Windows 64-bit-inode precision (fs.statSync(..., { bigint: true }), to avoid aliasing two files past 2^53) plus matching type annotations. None of it touches updateLogger's rotation-fallback logic or getFileLogger's install/clear arbitration — the exact code this rule is about is byte-for-byte the same shape on main today as it was at the v5.3.0 tag.

🔧 Changes

• unitTests/utility/logging/harper_logger.test.js — adds a Test caller-impact shapes for the logger rotation-inheritance fix (#1877/#1880) describe block with 4 new tests: the unaffected plain-logger shape (shared per-path sink), the unaffected withTag shape, the tailer operational risk, and the unaffected level-resolution shape (mutation-tested against three different ways the assertion could pass trivially).
• No production code changes. The affected shape itself (shape table row 4/5) was already pinned by the pre-existing Test external/component logger rotation inheritance (#1877) block (:1023-1240), added with PR fix(logging): external/component loggers inherit main rotation config #1880.

✅ Verification

• npx mocha unitTests/utility/logging/harper_logger.test.js — 126 passing, 14 pending (pre-existing, environment-conditional skips unrelated to this change), 0 failing. Re-ran the full file 3x and the new tailer test standalone 5x to rule out flakiness (one real rotation-cascade race was found and fixed this way — see commit history).
• npm run test:unit:main (full gate) — 6893 passing, 1 unrelated failure (deployActivation.test.js, "retries a transient failure moving the candidate into place") that reproduces only under concurrent load from other agents' mocha runs on this box and passes cleanly in isolation; confirmed unrelated to this diff (zero file overlap).
• Did not run test:unit:resources / test:integration:all: this PR changes only one unit test file with zero production-code or runtime-behavior changes, so the full integration gate is disproportionate to what's being verified. test:unit:logging (the directly relevant suite, 203 tests across the whole unitTests/utility/logging/ directory) passed in full.
• Each new assertion was independently verified with a source mutation: temporarily reverted the relevant line in utility/logging/harper_logger.ts to the pre-fix / broken behavior, rebuilt dist/, confirmed the targeted test failed, then restored and rebuilt clean (repeated 3 times across the level-resolution test's iterations — see commit messages for the exact mutations tried).
• End-to-end verification route: not observable end-to-end — this PR characterizes existing, already-released behavior via unit tests; there is no new runtime behavior to exercise through an integration test or live smoke.
• Cross-model pre-push review: 5 rounds (1 full + 4 delta) across codex + gemini + harper-domain. Round 1 found a real test-correctness gap (a copy-at-creation confound that let the "shared sink" test pass for the wrong reason) and a real bug (a rotation-cascade race that could land an assertion in the wrong archive file, reproduced and fixed with a settle-wait). Round 3 found a second real gap (the level-fallback test didn't catch a whole-line-deletion mutation); fixed and reconfirmed via mutation testing in round 4. Round 5 converged with no new findings — all remaining items are disputed nits (file-descriptor-during-rename-on-Windows, inline setup duplication, an untracked empty test directory) that the domain lens refuted against source citations (synchronous rename, closeLogFile swallowing errors, git not tracking empty dirs, etc.).

Declined, for the record:

❓ Your call: the review's "dropped-default-arg-coverage" decision point suggested also covering the omitted-4th-argument shape (every production call site) against the real module singleton, not just explicit mainLoggerRef substitution. I didn't restore this: doing it safely would mean either reintroducing a prohibited new rewire/private-state use (AGENTS.md's "do not add new uses of sinon or rewire"), or reusing the real shared externalLogger's file path and risking a cache collision with the actual production logger's rotation state for the rest of the test run. The mechanism being tested there — a default parameter resolving to its default when omitted — is a one-line, self-evident JS fact readable directly off the function signature (:120); the fallback logic itself (the part worth pinning) is what the explicit-substitution test actually exercises and mutation-tests.
❓ Your call: kept the describe-block title and test names free of the specific v5.2.15 -> v5.3.0 version numbers (per the review's "head-only-historical-claims" note) — a HEAD-only test can't verify a historical fact about an old tag, so titles describe current behavior instead; the version history lives in this PR body and the commit messages.
❓ Your call: the tailer test asserts the breakage as a passing test (locks in today's rename-based rotation) rather than only documenting it in prose. Reversible later (delete one test) if rotation ever moves to copy-truncate; in the meantime it's the one row in the table I'd most want CI to catch a regression on.

Refs #1877

🤖 Generated with Claude Code

https://claude.ai/code/session_01524gnfYt96yuXWh7KqpsET

Complexity: easy

Release target: main — test-only, no runtime behavior change to backport
Related PRs: #2939 overlaps, 4 others independent

_{Dispatch: task harper-upgrade-rule-logger-rotation-530 · queued by unknown · ran by claude/sonnet/high · worker kzyp-xps-1}

_{Review-Coverage: authored=claude; ran=gemini,codex; adjudicated=domain; declined=cursor-grok,cursor-composer,cursor-kimi,cursor-muse; rounds=5; full=1 @ 8d93748}

_{Review-Attention: skim ~2m (decisions: head-only-historical-claims, dropped-default-arg-coverage, quiescence-vs-deterministic, pin-tailer-breakage-as-passing) @ 8d93748}

[gemini-code-assist]

Code Review

This pull request introduces a new test suite in unitTests/utility/logging/harper_logger.test.js to validate caller-impact shapes for the logger rotation-inheritance fix (#1877/#1880). The added tests verify behavior for plain loggers sharing the main log path, tagged loggers, log rotation with open file descriptors, and level resolution fallback via mainLoggerRef. There are no review comments to address, and I have no feedback to provide.