feat(logger): standardize output and fold the spinner into the logger#32
Merged
Conversation
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).
PR Summary by Qodofeat(logger): Standardize CLI output and fold spinner into domain-aware logger
AI Description
Diagram
High-Level Assessment
Files changed (19)
|
Code Review by Qodo
Context used✅ Compliance rules (platform):
64 rules✅ Skills:
doc-authoring, doc-authoring-with-logs 1. Quiet still shows spinners
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+149
to
+173
| // Whether a level's persistent output is suppressed: quiet drops | ||
| // info/success/debug; debug additionally needs the debug flag. warn/error | ||
| // always print. (The awaited work in a `waitFor` runs regardless.) | ||
| const suppressed = (level: LogLevel): boolean => { | ||
| if (level === "debug") return state.quiet || !state.debugEnabled; | ||
| if (level === "info" || level === "success") return state.quiet; | ||
| return false; | ||
| }; | ||
|
|
||
| // Wipe any running spinner frame before each write so the log line lands on a | ||
| // clean row; the spinner redraws below it on its next tick. | ||
| return { | ||
| info: quiet | ||
| ? noop | ||
| : (msg) => { | ||
| clearActiveLine(); | ||
| console.log(msg); | ||
| }, | ||
| success: quiet | ||
| ? noop | ||
| : (msg) => { | ||
| clearActiveLine(); | ||
| console.log(paint("green", "✓ ") + msg); | ||
| }, | ||
| warn(msg) { | ||
| clearActiveLine(); | ||
| console.warn(paint("yellow", "! ") + msg); | ||
| }, | ||
| error(msg) { | ||
| clearActiveLine(); | ||
| console.error(paint("red", "✗ ") + msg); | ||
| }, | ||
| debug: quiet | ||
| ? noop | ||
| : (msg) => { | ||
| if (!debugEnabled) return; | ||
| clearActiveLine(); | ||
| console.error(paint("gray", "· " + msg)); | ||
| }, | ||
| const emit = (level: LogLevel, msg: LogInput): void => { | ||
| if (suppressed(level)) return; | ||
| clearActiveLine(); | ||
| const line = formatLog(level, msg, fmt); | ||
| if (level === "warn") console.warn(line); | ||
| else if (level === "error" || level === "debug") console.error(line); | ||
| else console.log(line); | ||
| }; | ||
|
|
||
| // `waitFor` path: spin (unless suppressed / non-TTY) until the promise settles, | ||
| // then stop; on success optionally print `done`, and always return the value. | ||
| const spin = async <T>(level: LogLevel, task: LogTask<T>): Promise<T> => { | ||
| const handle = startSpinner(level, task, fmt, suppressed(level)); | ||
| try { |
There was a problem hiding this comment.
3. Quiet still shows spinners 🐞 Bug ☼ Reliability
In quiet mode, warn/error LogTask calls will still start an animated spinner because spinner suppression is derived from suppressed(level), which returns false for warn/error even when quiet is true. This can leak cursor-hide/show and frame output into CI/scripts where quiet previously prevented spinner escape codes entirely.
Agent Prompt
## Issue description
`startSpinner()` is currently disabled using `suppressed(level)`, but `suppressed()` intentionally never suppresses warn/error even when `quiet` is true. As a result, `warn({waitFor})` / `error({waitFor})` will still animate a spinner and emit terminal control sequences in quiet mode.
## Issue Context
Historically, `quiet` was used specifically to keep scripts/CI free of escape codes. The new implementation should preserve that behavior by treating "spinner allowed" separately from "log line allowed".
## Fix Focus Areas
- src/core/logger.ts[149-184]
- src/core/logger.ts[99-123]
## Suggested fix
Introduce a dedicated `spinnerSuppressed(level)` (or similar) that returns `state.quiet || (level === "debug" && !state.debugEnabled)` and pass that to `startSpinner(...)` instead of `suppressed(level)`. Keep `suppressed(level)` controlling only whether a persistent log line prints.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+186
to
+194
| // A single implementation backs both overloads: a `waitFor` payload takes the | ||
| // spinner path (returns a promise), everything else prints synchronously. | ||
| const make = | ||
| (level: LogLevel) => | ||
| // 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); | ||
| }; |
There was a problem hiding this comment.
4. Loose waitfor detection 🐞 Bug ☼ Reliability
Logger methods choose spinner mode using only 'waitFor' in msg, so any object that happens to contain a waitFor key (even undefined/non-Promise) will be treated as a LogTask and awaited. This can unexpectedly change runtime behavior/return type and may mask malformed inputs by "awaiting" non-Promises.
Agent Prompt
## Issue description
The logger decides between synchronous `emit()` and async `spin()` with `"waitFor" in msg`, which is a very permissive discriminator. It will treat arbitrary objects as tasks if they merely have a `waitFor` property.
## Issue Context
`LogTask.waitFor` is intended to be a `Promise<T>`. Runtime behavior should align with that contract and avoid accidental collisions.
## Fix Focus Areas
- src/core/logger.ts[186-194]
- src/core/types/public.ts[167-177]
## Suggested fix
Tighten the guard to require a thenable:
- `const w = (msg as any).waitFor; if (w && typeof w.then === "function") ...`
Optionally also ensure `typeof (msg as any).message === "string"` to avoid nonsensical spinner payloads.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+26
to
45
| // Scope the logger (and the ctx handed to steps/callbacks) to this domain so | ||
| // every init line carries its `[domain]` prefix; the id in messages is then | ||
| // just the step (the domain is already in the prefix). | ||
| const dctx: ResolveContext = { ...ctx, logger: withDomain(ctx.logger, domain) }; | ||
|
|
||
| for (const step of steps) { | ||
| try { | ||
| if (step.when && !(await step.when(ctx))) { | ||
| ctx.logger.debug(`skipping ${domain.id}.${step.id} (when predicate false)`); | ||
| if (step.when && !(await step.when(dctx))) { | ||
| dctx.logger.debug(`skipping ${step.id} (when predicate false)`); | ||
| continue; | ||
| } | ||
| const value = await resolveStepValue(step, opts, ctx); | ||
| const value = await resolveStepValue(step, opts, dctx); | ||
| if (opts.dryRun) { | ||
| ctx.logger.info(`would: ${domain.id}.${step.id} = ${formatValue(value)}`); | ||
| dctx.logger.info(`would: set ${step.id} = ${formatValue(value)}`); | ||
| continue; | ||
| } | ||
| await invokeCallback(step, value, ctx); | ||
| await invokeCallback(step, value, dctx); | ||
| } catch (err) { | ||
| ctx.logger.error(`${domain.id}.${step.id} failed: ${(err as Error).message}`); | ||
| dctx.logger.error({ message: `${step.id} failed`, status: (err as Error).message }); | ||
| } |
There was a problem hiding this comment.
5. Init logs can lose domain 🐞 Bug ◔ Observability
Init-step logging now omits domain.id from messages and relies on withDomain() for domain context via the prefix. But withDomain() returns the base logger unchanged when it wasn't created by createLogger(), so custom/injected loggers will emit init-step lines with no domain identifier.
Agent Prompt
## Issue description
`runDomainInitSteps()` logs only `step.id` (e.g. `would: set select = ...`) and depends on `withDomain()` to supply the domain prefix. However, `withDomain()` is explicitly a no-op for loggers not produced by `createLogger()`, so domain context can be lost.
## Issue Context
The codebase exposes `BuildContextOptions.logger?: Logger`, so consumers/tests may inject custom loggers. Those loggers will not get the `[domain]` prefix, and the new messages no longer include `domain.id`.
## Fix Focus Areas
- src/core/commands/init-steps.ts[26-45]
- src/core/logger.ts[216-226]
## Suggested fix
Either:
1) Re-include `domain.id` in init-step messages (e.g. `skipping ${domain.id}.${step.id} ...`, `would: set ${domain.id}.${step.id} = ...`, `${domain.id}.${step.id} failed`), or
2) Enhance `withDomain()` to wrap unknown loggers (no WeakMap state) by prefixing `LogInput.message` with `[domain]`/`domain:` so domain context is preserved even for injected loggers.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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]— padded, dimmed, in the domain's color (docs=cyan, skills=magenta, rules=green, mcp=blue, hooks=yellow, agents=gray; supervisor/core lines use[agnos]).changed).How
formatLogcomponent + structuredLogInput({ message, status, extra }); plain strings stay valid.ctx.loggeris scoped per domain viawithDomainacross the run pipeline, CLI command dispatch, and init steps — so every line carries its prefix/color automatically. Removes now-redundant hand-rolleddomain:prefixes and the inline metadata-shape block from the docs/rules warnings (the shape lives in the doc-authoring skill).waitForpromise (plusdone) that animates a standardized, domain-colored spinner until it settles, then returns the resolved value.withSpinner/createSpinnerare removed and all 8 call sites migrated; the hand-passed{ quiet }is gone (the logger owns it).Verification
pnpm typecheck— cleanpnpm test— 241/241 passpnpm lint— 0 errorsSpinners 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 adescripti0ontypo —.docs/index.md,AGENTS.md,package.json) were intentionally left out of this branch.