Skip to content

feat(logger): standardize output and fold the spinner into the logger#31

Closed
rgdevme wants to merge 1 commit into
mainfrom
claude/gifted-bardeen-f95058
Closed

feat(logger): standardize output and fold the spinner into the logger#31
rgdevme wants to merge 1 commit into
mainfrom
claude/gifted-bardeen-f95058

Conversation

@rgdevme

@rgdevme rgdevme commented Jul 1, 2026

Copy link
Copy Markdown
Owner

What

Standardizes all CLI output into a single reusable, domain-aware logging component and folds the spinner into it.

Every line now renders in one shape:

[domain]  message [status]
extra line(s)
  • Prefix [domain] — padded, dimmed, in the domain's color (docs=cyan, skills=magenta, rules=green, mcp=blue, hooks=yellow, agents=gray; supervisor/core lines use [agnos]).
  • Message — colored by level: white (info), green (success), yellow (warn), red (error), blue (debug).
  • Status — optional, italic + dimmed (e.g. changed).
  • Extra — optional detail lines, white (e.g. a file list).

How

  • New formatLog component + structured LogInput ({ message, status, extra }); plain strings stay valid.
  • ctx.logger is scoped per domain via withDomain across the run pipeline, CLI command dispatch, and init steps — so every line carries its prefix/color automatically. Removes now-redundant hand-rolled domain: prefixes and the inline metadata-shape block from the docs/rules warnings (the shape lives in the doc-authoring skill).
  • Spinner folded into the logger: log methods accept an optional waitFor promise (plus done) that animates a standardized, domain-colored spinner until it settles, then returns the resolved value. withSpinner/createSpinner are removed and all 8 call sites migrated; the hand-passed { quiet } is gone (the logger owns it).

Verification

  • pnpm typecheck — clean
  • pnpm test — 241/241 pass
  • pnpm lint — 0 errors

Spinners are no-ops in the non-TTY test env, so migrated sites behave exactly as before (await + return value); live spinner appearance is best eyeballed in a real terminal.

Notes

Scoped to src/** + test/**. Unrelated in-progress working-tree edits (.docs/plans/prd.md — which currently has a descripti0on typo — .docs/index.md, AGENTS.md, package.json) were intentionally left out of this branch.

Introduce a reusable, domain-aware log-line component. Every line renders
as `[domain]  message [status]` with a dimmed domain-colored prefix, a
level-colored message (white/green/yellow/red/blue), an italic-dim status,
and white extra lines. Structured LogInput ({ message, status, extra })
replaces ad-hoc strings; plain strings stay valid.

- Scope ctx.logger per domain across the run pipeline, CLI command
  dispatch, and init steps via withDomain, so every line carries its
  [domain] prefix and color automatically (docs=cyan, skills=magenta,
  rules=green, mcp=blue, hooks=yellow, agents=gray; supervisor -> [agnos]).
- Fold the spinner in: log methods accept an optional waitFor promise
  (plus done) that animates a standardized, domain-colored spinner until
  it settles, then returns the resolved value. Remove withSpinner /
  createSpinner and migrate all call sites.
- Strip now-redundant domain-name prefixes from messages and drop the
  inline metadata-shape block from the docs/rules warnings (the shape
  lives in the doc-authoring skill).
@rgdevme rgdevme marked this pull request as draft July 1, 2026 07:41
@rgdevme rgdevme closed this Jul 1, 2026
@rgdevme rgdevme deleted the claude/gifted-bardeen-f95058 branch July 1, 2026 07:42
@qodo-code-review

Copy link
Copy Markdown

PR Summary by Qodo

feat(logger): Standardize CLI output and fold spinner into logger

✨ Enhancement 🕐 20-40 Minutes

Grey Divider

AI Description

• Introduces a unified formatLog component rendering every line as [domain]  message [status]
 with domain-colored prefix, level-colored message, and optional italic status/extra lines.
• Folds the spinner into the logger: all log methods now accept an optional waitFor promise
 (LogTask) that animates a domain-colored spinner until settled, then returns the resolved value —
 replacing withSpinner/createSpinner across 8 call sites.
• Scopes ctx.logger per domain via withDomain in the run pipeline, CLI dispatch, and init steps,
 so every line automatically carries its [domain] prefix without manual prefixing in messages.
• Adds DomainColor, LogInput, LogParts, and LogTask types to the public API; removes the
 Spinner export and { quiet } threading.
Diagram

graph TD
    CLI["cli.ts"] --> withDomain
    run["run.ts"] --> withDomain
    initSteps["init-steps.ts"] --> withDomain
    watch["watch.ts"] --> withDomain
    withDomain --> makeLogger["makeLogger / Logger"]
    createLogger["createLogger"] --> makeLogger
    makeLogger --> formatLog["formatLog"]
    makeLogger --> startSpinner["startSpinner"]
    formatLog --> yoctocolors["yoctocolors-cjs"]
    startSpinner --> yoctocolors
    makeLogger --> domains
    subgraph domains["Domain Modules"]
        direction LR
        docs["docs"] ~~~ rules["rules"] ~~~ mcp["mcp"] ~~~ skills["skills"] ~~~ agents["agents"] ~~~ hooks["hooks"]
    end
    subgraph Legend
        direction LR
        _mod["Module"] ~~~ _fn(["Function"]) ~~~ _ext{{"External"}}
    end
    createLogger --> _fn
    withDomain --> _fn
    formatLog --> _fn
    startSpinner --> _fn
    yoctocolors --> _ext
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Keep spinner separate, make it domain-aware
  • ➕ No overloaded Logger methods — simpler type signature
  • ➕ Spinner lifecycle stays explicit at call sites
  • ➖ Still requires threading { quiet } or domain context to spinner creation
  • ➖ Doesn't eliminate the manual start/stop boilerplate
2. Dedicated logger.task() method instead of overloading
  • ➕ Explicit API — no overload ambiguity
  • ➕ Easier to discover and document
  • ➖ Adds a 6th method to Logger interface
  • ➖ Call sites are slightly more verbose

Recommendation: The PR's approach — folding the spinner into the logger via overloaded waitFor methods — is the right call for this codebase: it eliminates the { quiet } threading, centralizes domain coloring, and makes every call site simpler. The main alternative worth noting is keeping the spinner as a separate primitive (as it was) but making it domain-aware; that avoids the overloaded method complexity but doesn't eliminate the { quiet } threading or the manual start/stop boilerplate. A second alternative would be a dedicated task() method rather than overloading existing log levels, which would be more explicit but add API surface. The chosen approach is the most ergonomic for call sites.

Files changed (19) +396 / -253

Enhancement (3) +64 / -7
public.tsAdd DomainColor, LogParts, LogInput, LogTask types; overload Logger methods +51/-5

Add DomainColor, LogParts, LogInput, LogTask types; overload Logger methods

• Defines 'DomainColor', 'LogParts' (structured log payload), 'LogInput' (string | LogParts), and 'LogTask<T>' (log payload with 'waitFor' promise). Each 'Logger' method gains a 'waitFor' overload returning 'Promise<T>'. Adds optional 'color?: DomainColor' to the 'Domain' interface.

src/core/types/public.ts

run.tsScope ctx.logger per domain in runDomain via withDomain +5/-1

Scope ctx.logger per domain in runDomain via withDomain

• Wraps 'ctx' with a domain-scoped logger before calling 'dom.domain.run', so all domain output automatically carries the '[domain]' prefix.

src/core/run.ts

cli.tsScope logger to owning domain when dispatching CLI subcommands +8/-1

Scope logger to owning domain when dispatching CLI subcommands

• Imports 'withDomain' and applies it to 'cmdCtx.logger' before invoking a domain command, matching the prefix behavior of run-mode output.

src/core/cli.ts

Refactor (12) +308 / -221
logger.tsRewrite logger: add formatLog, domain-aware prefix, spinner integration, withDomain +187/-95

Rewrite logger: add formatLog, domain-aware prefix, spinner integration, withDomain

• Replaces the ad-hoc ANSI color map with 'yoctocolors-cjs'. Introduces 'formatLog' (pure log-line renderer), 'startSpinner' (internal, domain-colored), 'makeLogger' (shared factory backed by a 'WeakMap<Logger, LoggerState>'), and 'withDomain' (re-binds a logger to a domain without losing quiet/debug config). Removes 'createSpinner', 'withSpinner', and the 'Spinner' interface. 'indentedLogger' is updated to pass 'LogInput'/'LogTask' through unchanged.

src/core/logger.ts

index.tsExport new logger API; remove createSpinner/withSpinner/Spinner +5/-2

Export new logger API; remove createSpinner/withSpinner/Spinner

• Adds 'DomainColor', 'LogInput', 'LogParts', 'LogTask', 'formatLog', and 'withDomain' to the public exports. Removes 'createSpinner', 'withSpinner', and the 'Spinner' type.

src/core/index.ts

init-steps.tsScope logger to domain during init steps via withDomain +12/-6

Scope logger to domain during init steps via withDomain

• Creates a 'dctx' with a domain-scoped logger before iterating init steps, removing the 'domain.id.' prefix from log messages and passing the scoped context to 'when'/'resolveStepValue'/'invokeCallback'.

src/core/commands/init-steps.ts

watch.tsUse domain-scoped logger and structured LogInput in watch event handlers +6/-3

Use domain-scoped logger and structured LogInput in watch event handlers

• Creates a 'domLog' per watcher via 'withDomain' and emits structured '{ message, status }' objects instead of concatenated strings. Switches to 'workspaceRelativePath' for file path display.

src/core/watch.ts

index.tsMigrate agents domain: remove createSpinner, use logger.info waitFor, add color +8/-10

Migrate agents domain: remove createSpinner, use logger.info waitFor, add color

• Replaces the manual 'createSpinner'/'spinner.update'/'spinner.stop' loop with 'ctx.logger.info({ message, waitFor })' per adapter. Adds 'color: 'gray'' to the domain definition.

src/domains/agents/index.ts

index.tsMigrate docs domain: replace withSpinner with logger.info waitFor, add color +6/-6

Migrate docs domain: replace withSpinner with logger.info waitFor, add color

• Replaces 'withSpinner(…, () => compileDocsIndex(…), { quiet })' with 'ctx.logger.info({ message, waitFor: compileDocsIndex(…) })'. Adds 'color: 'cyan''.

src/domains/docs/index.ts

compile.tsRemove inline metadataShape block from incomplete-files warning +6/-17

Remove inline metadataShape block from incomplete-files warning

• Deletes the 'metadataShape()' helper and the embedded markdown fence from the warning. The warning now emits a structured '{ message, extra: [...files] }' object; the schema shape is documented in the doc-authoring skill instead.

src/domains/docs/compile.ts

index.tsMigrate rules domain: replace withSpinner, structured log messages, add color +15/-18

Migrate rules domain: replace withSpinner, structured log messages, add color

• Replaces 'withSpinner' with 'ctx.logger.info({ waitFor })', converts all string log calls to structured 'LogInput' objects, and adds 'color: 'green'' to the domain.

src/domains/rules/index.ts

index.tsMigrate mcp domain: replace withSpinner with logger waitFor, add color +13/-13

Migrate mcp domain: replace withSpinner with logger waitFor, add color

• Converts two 'withSpinner' call sites (registry search and update check) to 'ctx.logger.info({ waitFor })'. Adds 'color: 'blue''.

src/domains/mcp/index.ts

index.tsMigrate skills domain: replace withSpinner with logger waitFor, add color +33/-38

Migrate skills domain: replace withSpinner with logger waitFor, add color

• Converts four 'withSpinner' call sites (diagnose, add, update, run) to 'ctx.logger.info({ waitFor })'. Adds 'color: 'magenta''.

src/domains/skills/index.ts

pipeline.tsConvert skills-need-updating warning to structured LogInput +4/-5

Convert skills-need-updating warning to structured LogInput

• Replaces the concatenated string warn with '{ message, extra }' structured payload.

src/domains/skills/pipeline.ts

shared.tsConvert adapter warn calls to structured LogInput with status field +13/-8

Convert adapter warn calls to structured LogInput with status field

• Replaces three string-concatenation warn calls with '{ message, status }' objects, separating the error detail into the 'status' field.

src/agents/adapters/shared.ts

Tests (3) +23 / -25
init-steps.test.tsUpdate init-steps test for new domain-scoped log message format +1/-1

Update init-steps test for new domain-scoped log message format

• Adjusts the dry-run log assertion from 'agents.select = …' to 'select = …' to match the new domain-prefix-in-logger (not in message) convention.

test/core/init-steps.test.ts

compile.test.tsUpdate compile test to assert structured LogParts instead of raw string +7/-11

Update compile test to assert structured LogParts instead of raw string

• Changes the warning capture type to 'LogParts[]' and asserts 'message'/'extra' fields directly, removing the now-deleted metadata-shape assertions.

test/docs/compile.test.ts

skills-pipeline.test.tsUpdate skills-pipeline test to use real logger + structured warn assertions +15/-13

Update skills-pipeline test to use real logger + structured warn assertions

• Replaces the stub logger with 'createLogger({ quiet: true })' (so 'info''s 'waitFor' path runs correctly) and overrides only 'warn' to capture 'LogParts'. Assertions switch from string contains to field-level checks.

test/domains/skills-pipeline.test.ts

Other (1) +1 / -0
index.tsAdd color: 'yellow' to hooks domain +1/-0

Add color: 'yellow' to hooks domain

• Single-line addition of 'color: 'yellow'' to the 'hooksDomain' definition for the new domain-prefix coloring system.

src/domains/hooks/index.ts

@qodo-code-review

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (1) 📘 Rule violations (1) 📜 Skill insights (0)

Context used
✅ Compliance rules (platform): 64 rules

Grey Divider


Action required

1. make() uses unjustified any 📘 Rule violation ⚙ Maintainability
Description
src/core/logger.ts introduces any types in the log method implementation and in the
indentedLogger wrapper signature without a directly attached rationale comment explaining why
any is necessary. This violates the requirement to justify TypeScript any, weakening type-safety
in a core logging utility and making future refactors riskier.
Code

src/core/logger.ts[R190-193]

+    // eslint-disable-next-line @typescript-eslint/no-explicit-any
+    (msg: any): any => {
+      if (msg !== null && typeof msg === "object" && "waitFor" in msg) return spin(level, msg);
+      emit(level, msg);
Relevance

⭐⭐⭐ High

Repo review standards explicitly flag “any without justification” as non-negotiable; similar policy
guidance accepted before (PR#12).

PR-#12

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
PR Compliance ID 704843 requires an explicit, directly attached justification comment for each
TypeScript any usage. In the changed code, both the logger overload implementation and the
indentedLogger wrapper use (msg: any): any but only include an eslint-disable comment, which
suppresses linting without explaining why any is required, thereby failing the compliance
requirement.

Rule 704843: Require justification comments for TypeScript any usage
src/core/logger.ts[190-193]
src/core/logger.ts[240-242]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`src/core/logger.ts` uses TypeScript `any` in the logger method implementation and in the `indentedLogger` wrapper signature without an adjacent rationale comment directly tied to each `any` usage.

## Issue Context
Compliance (PR Compliance ID 704843) requires every TypeScript `any` to be justified with an explicit comment directly attached to the `any` line (adjacent or inline) explaining why the escape hatch is necessary; an eslint-disable alone is not sufficient. Where feasible, prefer typing the overload implementation and wrappers without `any`.

## Fix Focus Areas
- src/core/logger.ts[190-193]
- src/core/logger.ts[240-242]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools



Remediation recommended

2. Spinner leaves trailing text 🐞 Bug ☼ Reliability
Description
The new spinner frame renderer writes with a carriage return but does not clear the line first, so a
shorter subsequent spinner line can leave leftover characters visible on stderr and corrupt the
standardized output shape. This is triggered when one spinner supersedes another (or any future case
where the rendered spinner line length decreases).
Code

src/core/logger.ts[R72-76]

function renderActive(): void {
  if (!active) return;
-  const glyph = SPINNER_FRAMES[frame++ % SPINNER_FRAMES.length];
-  process.stderr.write(`\r${COLORS.cyan}${glyph}${COLORS.reset} ${active.message}`);
+  const glyph = SPINNER_FRAMES[frame++ % SPINNER_FRAMES.length]!;
+  process.stderr.write(`\r${active.prefix} ${DOMAIN_PAINT[active.color](glyph)} ${active.message}`);
}
Relevance

⭐⭐⭐ High

Team previously emphasized clearing spinner lines to avoid corrupted output; likely accept adding
clear before render/supersede (PR#27,#32).

PR-#27
PR-#32

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
renderActive() writes the spinner line with only \r (no \x1b[2K clear), so any shorter
subsequent line won’t overwrite trailing characters. startSpinner() supersedes a prior spinner by
clearing the interval but does not clear the existing line before the new spinner renders, making
the trailing-character case likely in normal operation when spinner messages vary in length.

src/core/logger.ts[72-76]
src/core/logger.ts[99-113]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
The spinner redraw uses `\r` without clearing the terminal line. When a new spinner line is shorter than the previous one, stale characters remain at the end of the line, visually corrupting output.

### Issue Context
This happens only in TTY stderr mode (when spinners are active), but it directly contradicts the PR goal of standardized output shapes.

### Fix Focus Areas
- src/core/logger.ts[72-76]
- src/core/logger.ts[99-113]

### Suggested fix
- Clear the current stderr line before writing each spinner frame (e.g. prepend `\r\x1b[2K` in `renderActive()`), **or** clear once when starting/superseding a spinner before the first `renderActive()`.
- Ensure the supersede path clears the previous spinner line before rendering the new spinner’s first frame.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Qodo Logo

Comment thread src/core/logger.ts
Comment on lines +190 to +193
// eslint-disable-next-line @typescript-eslint/no-explicit-any
(msg: any): any => {
if (msg !== null && typeof msg === "object" && "waitFor" in msg) return spin(level, msg);
emit(level, msg);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Action required

1. make() uses unjustified any 📘 Rule violation ⚙ Maintainability

src/core/logger.ts introduces any types in the log method implementation and in the
indentedLogger wrapper signature without a directly attached rationale comment explaining why
any is necessary. This violates the requirement to justify TypeScript any, weakening type-safety
in a core logging utility and making future refactors riskier.
Agent Prompt
## Issue description
`src/core/logger.ts` uses TypeScript `any` in the logger method implementation and in the `indentedLogger` wrapper signature without an adjacent rationale comment directly tied to each `any` usage.

## Issue Context
Compliance (PR Compliance ID 704843) requires every TypeScript `any` to be justified with an explicit comment directly attached to the `any` line (adjacent or inline) explaining why the escape hatch is necessary; an eslint-disable alone is not sufficient. Where feasible, prefer typing the overload implementation and wrappers without `any`.

## Fix Focus Areas
- src/core/logger.ts[190-193]
- src/core/logger.ts[240-242]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

Comment thread src/core/logger.ts
Comment on lines 72 to 76
function renderActive(): void {
if (!active) return;
const glyph = SPINNER_FRAMES[frame++ % SPINNER_FRAMES.length];
process.stderr.write(`\r${COLORS.cyan}${glyph}${COLORS.reset} ${active.message}`);
const glyph = SPINNER_FRAMES[frame++ % SPINNER_FRAMES.length]!;
process.stderr.write(`\r${active.prefix} ${DOMAIN_PAINT[active.color](glyph)} ${active.message}`);
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remediation recommended

2. Spinner leaves trailing text 🐞 Bug ☼ Reliability

The new spinner frame renderer writes with a carriage return but does not clear the line first, so a
shorter subsequent spinner line can leave leftover characters visible on stderr and corrupt the
standardized output shape. This is triggered when one spinner supersedes another (or any future case
where the rendered spinner line length decreases).
Agent Prompt
### Issue description
The spinner redraw uses `\r` without clearing the terminal line. When a new spinner line is shorter than the previous one, stale characters remain at the end of the line, visually corrupting output.

### Issue Context
This happens only in TTY stderr mode (when spinners are active), but it directly contradicts the PR goal of standardized output shapes.

### Fix Focus Areas
- src/core/logger.ts[72-76]
- src/core/logger.ts[99-113]

### Suggested fix
- Clear the current stderr line before writing each spinner frame (e.g. prepend `\r\x1b[2K` in `renderActive()`), **or** clear once when starting/superseding a spinner before the first `renderActive()`.
- Ensure the supersede path clears the previous spinner line before rendering the new spinner’s first frame.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

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