# Logger — Known Issues

For planned/actionable work see `LOGGING_TODO.md`.

## ACCORE-LOG-I-K7M2: NopLogWriter ctor signature mismatch (consumer-specific but framework-exposed)

**Severity:** Minor (caught, non-blocking, but noisy) · **Status:** Open · **Area:** Writer-instantiation contract (`AcLoggerBase(string)` config-reading ctor)

### Description
When `AcLoggerBase(string? categoryName)` (config-reading ctor) runs, it iterates `AyCode:Logger:LogWriters[]` and calls `Activator.CreateInstance(writerType, AppType, LogLevel, categoryName)`. Some consumer writers (notably a NopCommerce-plugin-level `NopLogWriter`) have DI-injected ctors instead (e.g. `(INopLoggerMsSqlNopDataProvider, CommonSettings, CustomerSettings, IWebHelper, ILogger, string?)`), so no ctor matches → `MissingMethodException`.

Exception is caught in `AcLoggerBase` try/catch, logged to `Console.Error`, loop continues with other writers. Non-blocking — protocol construction succeeds — but produces noisy startup logs per `Logger` instance.

### Root cause
Two logger-construction paradigms coexist:
- Config-reading ctor expects `(AppType, LogLevel, string?)` writer ctors for `Activator` convenience
- DI-injected writers have arbitrary ctor signatures

### Known workaround
Console.Error noise tolerated. Alternatively, consumer uses DI-based `AddAcLoggerFactory<TLogger>` (see LOGGING.md) instead of the config-reading ctor — this path doesn't touch `LogWriters[]`.

### Related TODO
`LOGGING_TODO.md#accore-log-t-h6y4`

## ACCORE-LOG-I-R9P3: AcEnv.AppConfiguration is filesystem-bound, MAUI/WASM-unsafe

**Severity:** Minor · **Status:** Open · **Area:** `AyCode.Core.Consts.AcEnv`

### Description
`AcEnv.AppConfiguration` is a static lazy singleton calling `new ConfigurationBuilder().AddJsonFile("appsettings.json").Build()` on first access. Reads current-working-directory filesystem. Throws on MAUI (no physical appsettings next to exe) and WASM (no filesystem at all).

### Root cause
Design predates `IOptions`/DI pattern.

### Known workaround
Consumer avoids the config-reading `AcLoggerBase(string)` ctor on these platforms. DI-based `AddAcLoggerFactory<TLogger>` + `services.Configure<AcLoggerOptions>(...)` is the replacement (see LOGGING.md).

### Related TODO
`LOGGING_TODO.md#accore-log-t-n2d8`

## ACCORE-LOG-I-L4N8: Two parallel logger-setup patterns

**Severity:** Minor (confusion, not functional) · **Status:** Open · **Area:** LOGGING.md / consumer code

### Description
Two ways to construct a logger coexist:
1. **Config-reading:** `new Logger(categoryName)` → `AcLoggerBase` reads `AyCode:Logger` section via `AcEnv.AppConfiguration`, instantiates writers via `Activator.CreateInstance`
2. **DI factory:** `services.Configure<AcLoggerOptions>(...)` + `services.AddAcLoggerFactory<TLogger>()` → `Func<string, TLogger>` resolved from DI, writers pulled from DI

Consumer picks per scenario; no automatic bridge. Risk: mixing patterns causes subtle failures (e.g. `MissingMethodException` — see ACCORE-LOG-I-K7M2).

### Related TODO
`LOGGING_TODO.md#accore-log-t-r7l3`

## ACCORE-LOG-I-B2H5: Default LogLevel diverges across the two setup paths

**Severity:** Minor (surprise, not broken) · **Status:** Open · **Area:** `AcLoggerBase` field initializer vs `AcLoggerOptions`

### Description
The two logger-setup paths (see ACCORE-LOG-I-L4N8) ship with **different default LogLevels**:
- `AcLoggerBase.cs:18` — field initializer: `LogLevel = Error`
- `AcLoggerOptions.cs:27` — DI options default: `LogLevel = Info`

Consequences:
- Config-reading ctor WITHOUT an `AyCode:Logger:LogLevel` entry → silent `Error` minimum (loses Info/Warning in prod)
- DI factory WITHOUT `services.Configure<AcLoggerOptions>(...)` → `Info` minimum

Same consumer code, same "no configuration" state, two different log volumes depending on which path was chosen.

### Root cause
Historical: field initializer predates the Options class; Options was added as part of the DI-factory refactor with a developer-friendly `Info` default.

### Related TODO
`LOGGING_TODO.md#accore-log-t-j9g2`

## ACCORE-LOG-I-X7Q1: `AcConsoleLogWriter.Initialize()` runs twice on parameterless ctor

**Severity:** Minor (wasted work, not broken) · **Status:** Open · **Area:** `AcConsoleLogWriter` ctor chain

### Description
`AcConsoleLogWriter.cs:14-17`:
```csharp
protected AcConsoleLogWriter() : this(null)   // chains to (string?) ctor
{
    Initialize();                              // ← second call
}

protected AcConsoleLogWriter(string? categoryName = null) : base(categoryName)
{
    Initialize();                              // ← first call (via chain)
}
```

Chain `: this(null)` invokes the `(string?)` ctor, which calls `Initialize()`. Control returns to the parameterless ctor body and calls `Initialize()` again. Net effect: `Console.ForegroundColor = ConsoleColor.White` set twice — harmless but reveals a copy-paste oversight.

### Fix direction
Remove the second `Initialize()` call in the parameterless ctor body (the chain already covered it).

### Related TODO
`LOGGING_TODO.md#accore-log-t-b8k5`

## ACCORE-LOG-I-V3J6: `ILogger.IsEnabled(MsLogLevel.None)` incorrectly reports enabled

**Severity:** Low (semantic bug, rare path) · **Status:** Open · **Area:** `AcLoggerBase.IsEnabled` + `MapFromMsLogLevel`

### Description
`AcLoggerBase.cs:184-188`:
```csharp
public bool IsEnabled(MsLogLevel logLevel)
{
    var acLogLevel = MapFromMsLogLevel(logLevel);
    return LogLevel <= acLogLevel;
}
```

`MapFromMsLogLevel(MsLogLevel.None) → AcLogLevel.Disabled (byte 255)`. For any instance `LogLevel` (max `Error=25`), `LogLevel <= 255` is **always true** → `IsEnabled(None)` returns `true` despite `None` meaning "off".

The subsequent `Log<TState>` switch has `case MsLogLevel.None: default: break;` so no actual write occurs, BUT:
- The MS-logging framework may query `IsEnabled` as a gate before calling `Log` — hosts using that pattern get misleading "enabled" signals
- Any caller that inspects `IsEnabled(None)` (diagnostics, test assertions) gets the wrong answer

### Fix direction
Either:
- **(a)** Explicit `if (logLevel == MsLogLevel.None) return false;` at the top of `IsEnabled`
- **(b)** Change the comparison to strict `<` with `Disabled` as sentinel (larger refactor — affects semantic of `Disabled` elsewhere)

### Related TODO
`LOGGING_TODO.md#accore-log-t-x1v4`

## ACCORE-LOG-I-T8F2: Misleading inline comment in `AcLoggerBase.Log<TState>`

**Severity:** Trivial (doc-only) · **Status:** Open · **Area:** `AcLoggerBase.cs:210-211`

### Description
```csharp
// Use eventId.Name as memberName if available, otherwise null (will show as empty, not "Log")
var memberName = !string.IsNullOrEmpty(eventId.Name) ? $"{eventId.Name}:{eventId.Id}" : "Log";
```

Comment says fallback is `null` (empty display), the code assigns `"Log"`. Contradicts both the doc in `LOGGING.md#CallerMemberName Auto-Capture` and the actual behaviour observed in logs.

### Fix direction
Update comment to match code: `// Use eventId.Name:eventId.Id if Name is set, otherwise fallback to "Log" per LOGGING.md convention`.

### Related TODO
Folded into `LOGGING_TODO.md#accore-log-t-m7p2` (cleanup batch).

## ACCORE-LOG-I-M4C9: Server-side NopCommerce plugin still uses legacy config-reading Logger ctor

**Severity:** Minor (works, but inconsistent with modern pattern + triggers ACCORE-LOG-I-K7M2 noise) · **Status:** Open · **Area:** Consumer adoption gap in `Nop.Plugin.Misc.AIPlugin/Infrastructure/PluginNopStartup.cs`

### Description
Client side (`FruitBankHybridApp.*` — Web, Web.Client, MAUI) was migrated to the DI-factory pattern: `services.Configure<AcLoggerOptions>(...)` + `services.AddAcLoggerFactory<Logger>()`. The server-side plugin was NOT migrated — it still:

1. Directly constructs a logger via `new Logger(nameof(AyCodeBinaryHubProtocol))` (`PluginNopStartup.cs:169`) and passes it as `opts.Logger` to the protocol — bypasses `ILogger<AcBinaryHubProtocol>` DI resolution that `AcSignalRProtocolExtensions.BuildProtocol` already implements.
2. Does NOT call `services.Configure<AcLoggerOptions>(configuration.GetSection("AyCode:Logger"))`.
3. Does NOT call `services.AddAcLoggerFactory<Logger>()`.
4. Writer registration exists (`services.AddScoped<IAcLogWriterBase, ConsoleLogWriter>()` + `NopLogWriter`) but those DI-registered singletons are NOT the instances the `new Logger(...)` ctor sees — the legacy ctor creates a parallel set via `Activator.CreateInstance`.

The legacy config-reading ctor DOES find the appsettings `AyCode:Logger` section via `AcEnv.AppConfiguration` (filesystem-backed, works on server) — so logging functions. But every `new Logger(...)` call:
- Triggers ACCORE-LOG-I-K7M2 (NopLogWriter ctor mismatch → Console.Error noise)
- Reconstructs writer instances via `Activator` (not singleton-shared with DI-registered writers)
- Is inconsistent with the client-side pattern → two mental models for the same framework

### Fix direction
See `LOGGING_TODO.md#accore-log-t-w4h9`.

### Related
- `ACCORE-LOG-I-K7M2` (trigger — NopLogWriter ctor mismatch, currently causing Console.Error noise)
- `ACCORE-LOG-I-L4N8` (root cause — two coexisting setup patterns)
- `ACCORE-LOG-I-B2H5` (consequence — different defaults between paths; server legacy path silently ships with `Error` default unless `AyCode:Logger:LogLevel` is set)
- Sibling gap: `../SIGNALR/SIGNALR_ISSUES.md#accore-sig-i-b5g9` (same server-side plugin, protocol-options adoption gap)
- Plugin doc drift: `Nop.Plugin.Misc.AIPlugin/docs/SIGNALR/README.md:22` still documents the pre-migration `new AcBinaryHubProtocol()` registration (actual code uses `.AddAcBinaryProtocol(opts => {...})`). Update needed.

## ACCORE-LOG-I-P5W3: JWT signing key written to log (CRITICAL security)

**Severity:** 🛑 **Critical (security)** · **Status:** Closed (2026-04-25) · **Area:** `AyCode.Services.Server/Logins/AcLoginServiceServer.cs:192`

### Description
`GenerateAccessToken` writes the JWT signing key to the logger via `GlobalLogger.Detail`:

```csharp
GlobalLogger.Detail($"Key: {configuration["JWT:Key"]}");
```

The signing key is the cryptographic secret used to sign all JWT access tokens. Anyone with read access to the logs (file system, log aggregators, retention archives, operator dashboards, screenshots during incident response) can:

1. Forge arbitrary tokens for any user
2. Impersonate any role / claim
3. Bypass authentication entirely

Log storage is typically at a lower trust level than dedicated secret stores. This exposure path is a complete authentication bypass.

### Fix direction
Remove the `GlobalLogger.Detail($"Key: ...")` line entirely. The signing key must NEVER appear in any log output. If presence verification is needed for diagnostics, log only a hash prefix (e.g. `Key configured: SHA256[0..8] = abc12345`).

### Resolution

**What:** Wrapped the `GlobalLogger.Detail($"Key: ...")` call in `#if DEBUG` / `#endif` with a `// SECURITY:` comment block explaining the risk. Line preserved (not deleted) — user opted for DEBUG-gating over removal so local auth-debug retains the diagnostic.

**Where:** `AyCode.Services.Server/Logins/AcLoginServiceServer.cs:192` (line numbers shifted slightly due to the inserted comment).

**Why:** User-directed deviation from the originally proposed "remove entirely" fix direction (bearer-token ADR planning session, 2026-04-25). DEBUG-gating ensures production (`-c Release`) builds strip the line at compile time, while preserving dev-time diagnostics.

**Caveat:** DEBUG builds still write the key to log. CI/CD must build with `-c Release` for production deployments. The inline `// SECURITY:` comment in the code warns future maintainers.

### Related
- **Superseded by:** [`ACCORE-AUTH-I-B3X5`](../../../docs/AUTH/AUTH_ISSUES.md#accore-auth-i-b3x5) — entry relocated to AUTH topic per ADR-0001 "Status migration on AUTH topic creation" follow-up. AUTH is now the canonical home for current discussion; this entry preserved per append-only governance (`TOPIC_CODES.md` Status conventions).
- **Reference:** ADR [`0001-user-bearer-token-flow.md`](../../../docs/adr/0001-user-bearer-token-flow.md) — formal architectural decision; this entry's Resolution is the pre-flight fix referenced in the ADR's Consequences.
- `ACCORE-LOG-I-K1Z7` (sibling — access token also leaked, same file, same code path)
- `LOGGING/README.md` may need a new "Never log secrets" guideline section (separate `_TODO.md` candidate)
- **Discovery context**: this issue and `ACCORE-LOG-I-K1Z7` emerged during the user-bearer-token ADR context-gathering phase (2026-04-25 Copilot session, while reading `AcLoginServiceServer.cs`). If a dedicated AUTH topic-folder is created later (likely as part of the bearer token ADR follow-ups), these two entries are candidates for relocation to `AUTH_ISSUES.md` with `Status: SUPERSEDED by AUTH-I-N` cross-references.

## ACCORE-LOG-I-K1Z7: JWT access token written to log (CRITICAL security)

**Severity:** 🛑 **Critical (security)** · **Status:** Closed (2026-04-25) · **Area:** `AyCode.Services.Server/Logins/AcLoginServiceServer.cs:212`

### Description
After `GenerateAccessToken` produces a token, the token text itself is written to the logger:

```csharp
GlobalLogger.Detail($"AccesToken: {writtenToken}");
```

(Side note: `AccesToken` is a typo for `AccessToken` — fix while at the line.)

Access tokens are bearer credentials — anyone holding the token can authenticate as the user until token expiry (currently 6h per `GenerateAccessToken` config). Logged tokens leak via the same channels as `ACCORE-LOG-I-P5W3` (live streams, retention archives, screenshots, shared logs during ops).

### Fix direction
Remove the `GlobalLogger.Detail($"AccesToken: ...")` line entirely. If issuance verification is needed, log only **metadata** — user ID, expiry timestamp, issuer — never the token itself.

### Resolution

**What:** Two changes:
1. Wrapped the `GlobalLogger.Detail($"AccessToken: ...")` call in `#if DEBUG` / `#endif` with a `// SECURITY:` comment block.
2. Fixed the `AccesToken` → `AccessToken` typo (flagged in original Description as a side-note) in the log-message string literal.

**Where:** `AyCode.Services.Server/Logins/AcLoginServiceServer.cs:212` (line numbers shifted slightly due to the inserted comment).

**Why:** Same as `ACCORE-LOG-I-P5W3` — user opted for DEBUG-gating over deletion (2026-04-25, bearer-token ADR session). Typo fix is opportunistic since the line was being touched.

**Caveat:** Same DEBUG-build leak constraint as `ACCORE-LOG-I-P5W3`. CI/CD must build with `-c Release` for production.

### Related
- **Superseded by:** [`ACCORE-AUTH-I-T8N2`](../../../docs/AUTH/AUTH_ISSUES.md#accore-auth-i-t8n2) — entry relocated to AUTH topic per ADR-0001 "Status migration on AUTH topic creation" follow-up. AUTH is now the canonical home for current discussion; this entry preserved per append-only governance (`TOPIC_CODES.md` Status conventions).
- **Reference:** ADR [`0001-user-bearer-token-flow.md`](../../../docs/adr/0001-user-bearer-token-flow.md) — formal architectural decision; this entry's Resolution is the pre-flight fix referenced in the ADR's Consequences.
- `ACCORE-LOG-I-P5W3` (sibling — JWT signing key also leaked, same file)
- Same `LOGGING/README.md` "Never log secrets" guideline gap as ACCORE-LOG-I-P5W3
- Same discovery-context note as ACCORE-LOG-I-P5W3 (bearer token ADR session, candidate for AUTH topic relocation if/when that emerges)

## Evaluated review findings — NOT bugs (by-design)

Cross-session review (2026-04) flagged several items that are **intentional defense-in-depth / YAGNI** and should NOT be "fixed":

- **"Triple LogLevel gate on the ILogger.Log path is redundant"** — NOT redundant. `Info()`/`Debug()` can be called directly (not only via `ILogger.Log`), so each named method MUST own its gate. Per-writer gate is the documented Gate-2 (see LOGGING.md "Two independent level gates"). Removing any gate breaks a real path.
- **"`AcTextLogWriterBase.GetDiagnosticText` defensive level-check is redundant"** — same rationale; direct callers exist.
- **"`IAcLoggerBase` narrow interface (no dynamic LogLevel setter via interface)"** — YAGNI. Consumers needing dynamic level can cast to `AcLoggerBase`. Widening the interface for a rare scenario pollutes the abstraction.
- **"`protected List<IAcLogWriterBase> LogWriters` is mutable"** — acceptable; `GetWriters` returns a defensive copy (`[.. LogWriters]`), direct mutation is only reachable from derived classes (which control their own ctor-time population).