Skip to content

fix: route dual-memory hint and rerank cost advisory through the CLI-aware logging convention#889

Open
gorkem2020 wants to merge 1 commit into
CortexReach:masterfrom
gorkem2020:fix/log-noise-cli-convention
Open

fix: route dual-memory hint and rerank cost advisory through the CLI-aware logging convention#889
gorkem2020 wants to merge 1 commit into
CortexReach:masterfrom
gorkem2020:fix/log-noise-cli-convention

Conversation

@gorkem2020

Copy link
Copy Markdown

Fixes #888

What

  • The dual-memory model hint (from #344) now logs once per process and goes through the existing CLI-aware logReg convention: a gateway boot prints it once at info instead of once per registration context, and CLI commands see it only at debug.
  • The auto-recall rerank cost advisory from #843 remains a gateway-boot warning, but routes to debug when isCliMode(), so memory-pro subcommand output is no longer prefixed with the multi-line advisory on every invocation.

Both banners keep their educational and cost-awareness intent; only the repetition and the CLI placement change.

Tests

  • test/startup-health-diagnostics.test.mjs 2/2 (the buildAutoRecallRerankCostWarning builder is untouched; only the log call changes)
  • test/plugin-manifest-regression.mjs passes

Note

A clean tsc -p tsconfig.json build also wants to modify dist/src/store.js and dist/src/retriever.js, because src/ contains fixes the committed dist predates (a process.report.excludeNetwork guard against a long first-load hang, and a rerank top_n cap). Those files are intentionally left untouched here to keep this PR scoped; filing the dist drift separately.

…aware logging convention (CortexReach#888)

The dual-memory hint now logs once per process via logReg (debug in CLI
mode), instead of once per registration context and per CLI command.
The CortexReach#843 rerank cost advisory stays a gateway-boot warning but logs at
debug in CLI mode, so memory-pro subcommand output stays clean.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@rickthomasjr

Copy link
Copy Markdown

Substantive Code Review — PR #889

Reviewer: Harrison Forge (agent) | Date: 2026-06-12


TL;DR

APPROVE with a minor note. The fix correctly addresses the log repetition issue identified in #888.


Change-by-change analysis

dualMemoryHintLogged guard

The dual-memory hint banner (api.logger.info(...)) was being called once per registration context. During gateway boot, register() can run several times (one per registration context), so the same 4-line banner would print multiple times. The fix introduces a module-level let dualMemoryHintLogged = false guard that ensures it prints once per process.

Correctness: Sound. The module-level variable persists across register() calls within the same process. It resets on process restart (which is the desired behavior — new boot = new banner).

logReg routing for the banner

The banner now goes through logReg() (the existing CLI-aware log convention) instead of api.logger.info(). Combined with the guard, this means:

  • Gateway boot: prints once at info level (not once per registration)
  • CLI commands: prints once at debug level (hidden from user output)

Correctness: Sound. logReg already exists and handles the CLI-aware routing. This change just extends it to the dual-memory hint.

Rerank cost advisory CLI-aware routing

The auto-recall rerank cost advisory (from #843) now routes to api.logger.debug when isCliMode(), preventing the multi-line advisory from prefixing every memory-pro subcommand invocation.

Correctness: Sound. Same pattern as the dual-memory hint — the advisory is only useful during gateway boot, not on every CLI command.

dist/index.js diff

The diff shows 16 additions, 5 deletions. The actual semantic changes are:

  • The dualMemoryHintLogged variable declaration
  • The conditional if (!dualMemoryHintLogged) guard + logReg call
  • The isCliMode() ternary for the rerank advisory

These are clean translations of the source changes.

index.ts formatting noise

The PR includes a CRLF-to-LF conversion on ~1100 lines of index.ts. This is cosmetic and doesn't affect functionality. The PR body acknowledges this. Not a blocker, but worth noting.


Assessment

Dimension Value
Bug severity MEDIUM — log noise degrades CLI usability and drowns operational logs
Fix size SMALL — ~10 lines of new code
Risk LOW — only affects logging, no behavioral changes
Test coverage ADEQUATE — PR body notes startup-health-diagnostics tests pass
Correctness SOLID — uses existing logReg convention, follows established patterns

Minor note

Consider whether dualMemoryHintLogged should use Object.defineProperty(globalThis, ...) or a module-level const with a getter/setter to prevent accidental resets from other code. In practice, the module is self-contained so this is unnecessary, but worth being aware of for larger refactors.

Verdict

APPROVE. Clean fix, minimal risk.

@rickthomasjr rickthomasjr left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Agent Code Review: PR #889 — fix: route dual-memory hint and rerank cost advisory through the CLI-aware logging convention

Verdict: APPROVE

Summary

Two logging improvements: (1) downgrades the auto-recall rerank cost advisory from warn to debug in CLI mode (so each memory-pro search command doesn't repeat the gateway-boot advisory), and (2) deduplicates the dual-memory model warning using a process-scoped dualMemoryHintLogged flag + the existing logReg convention.

P1 — Cost advisory logging: APPROVED

Before: api.logger.warn(rerankCostWarning) fired on every gateway registration context and every CLI command. After: (isCliMode() ? api.logger.debug : api.logger.warn) — gateway still warns, CLI commands stay quiet. Correct behavior for a gateway-boot advisory.

P2 — Dual-memory hint dedup: APPROVED

Before: api.logger.info() fired once per registration context (multiple per gateway boot). After: module-scoped dualMemoryHintLogged flag ensures it fires once per process. Uses logReg() (the CLI-aware logger from PR #888) so it also respects CLI mode. Correct.

P3 — No behavioral change: APPROVED

This PR is purely a logging cleanup. No logic changes, no state changes, no API changes. Zero risk of regression.

Conclusion

Clean, minimal, zero-risk cleanup. APPROVE.

@rwmjhb rwmjhb left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Approved. The semantic change is small and aligned with the CLI-aware logging convention: the dual-memory hint is guarded once per process, and the rerank cost advisory is moved out of normal CLI-visible output while staying visible in gateway logs.

Verification checked:

  • orchestrator targeted tests passed
  • orchestrator full npm test passed
  • npm run build --if-present passed on head eb508565913f31ee3258f021e917726242405a78
  • node test/cli-smoke.mjs passed
  • node test/plugin-manifest-regression.mjs passed

Non-blocking cleanup request: the PR contains broad index.ts line-ending/format churn for a narrow logging fix. That produced static ANY_TYPE warning noise and makes the stale branch harder to rebase/review. It would be cleaner to remove the unrelated churn or split it out.

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.

Dual-memory hint and rerank cost advisory bypass the CLI-aware logging convention, repeating on every registration and every CLI command

3 participants