feat(logging): unify host and plugin diagnostics - #229
Conversation
| } | ||
| if (!["debug", "info", "warn", "error"].includes(merged.logLevel)) { | ||
| merged.logLevel = DEFAULT_CONFIG.logLevel; | ||
| } |
There was a problem hiding this comment.
🤖 devloop code-review · seed-2.1-pro
The valid log level values are hardcoded as a string array ["debug", "info", "warn", "error"]. If LogLevel were to gain a new member (e.g., "trace"), this validation would need to be updated in two places — here and in logging.ts (the LEVEL_PRIORITY record). Consider extracting the valid levels into a shared constant (e.g., export const LOG_LEVELS: readonly LogLevel[] = ["debug", "info", "warn", "error"] in logging.ts) so the validation stays in sync with the type definition.
ccr:fp=e8f7d8093d42
| return [JSON.parse(line) as StoredLogRecord]; | ||
| } catch { | ||
| return []; | ||
| } |
There was a problem hiding this comment.
🤖 devloop code-review · deepseek-v4-pro
The catch block silently discards malformed JSON lines without any indication. While log corruption is unlikely (the SessionLogger uses atomic appends), a corrupted line could hide real data issues. Consider adding a comment explaining the intent, or emitting a console.error via process.stderr to help diagnose file corruption during debugging.
ccr:fp=9c07b3983728
| readonly message?: string; | ||
| readonly pluginId?: string; | ||
| readonly [key: string]: unknown; | ||
| } |
There was a problem hiding this comment.
🤖 devloop code-review · deepseek-v4-pro
The StoredLogRecord interface duplicates parts of the LogEntry type from src/logging.ts. The LogEntry interface (with required level, source, component, message) is the canonical shape, while StoredLogRecord makes all fields optional. Consider importing LogEntry and using Partial<LogEntry> & { [key: string]: unknown } to reduce drift risk, or add a comment noting that this is intentionally a loose, permissive type for CLI consumption.
ccr:fp=f83d13b9fc1a
| component: `runner.${stream}`, | ||
| attributes: { output }, | ||
| }, | ||
| }), |
There was a problem hiding this comment.
🤖 devloop code-review · deepseek-v4-pro
The message "Plugin Runner wrote to ${stream}" is generic and does not include the actual output content. The meaningful information is placed in attributes.output, making log search and filtering more difficult. Consider including a truncated version of the output in the message itself (e.g., Plugin Runner stderr: ${output.slice(0, 200)}).
ccr:fp=64087ec7afd6
| level: stream === "stderr" ? "warn" : "debug", | ||
| message: `Plugin Runner wrote to ${stream}`, | ||
| context: { | ||
| component: `runner.${stream}`, |
There was a problem hiding this comment.
🤖 devloop code-review · deepseek-v4-pro
The capitalization of "Plugin Runner" in the message (capital 'R') is inconsistent with the component name plugin.runner (lowercase 'r'). This inconsistency could affect log filtering by exact string matching. Consider using consistent casing.
ccr:fp=cd364b055c12
| const reader = readable.getReader(); | ||
| const decoder = new TextDecoder(); | ||
| let buffer = ""; | ||
| try { |
There was a problem hiding this comment.
🤖 devloop code-review · seed-2.1-pro
If readable.getReader() throws synchronously on line 120, reader will be undefined and reader.releaseLock() in the finally block (line 144) will throw a TypeError, causing an unhandled promise rejection. Move the reader acquisition inside the try block so the finally block only executes when reader is successfully acquired.
ccr:fp=d04fd730e70b
| reason: "entry exceeded maxEntryBytes", | ||
| }, | ||
| })}\n`; | ||
| return line; |
There was a problem hiding this comment.
🤖 devloop code-review · seed-2.1-pro
The serialize method's fallback branch (when the entry exceeds maxEntryBytes) constructs a replacement object that omits the error field entirely. If an error entry produces a large serialized output (e.g., due to deep error causes or large attributes), the error details are silently dropped, and only a generic attributes: { truncated: true, reason: "entry exceeded maxEntryBytes" } is written. This makes debugging difficult for severe errors that happen to produce large log entries.
Consider including a truncated version of the error (e.g., error: { message: boundedString(entry.error.message), truncated: true }) in the fallback output.
ccr:fp=307373bbf640
|
🤖 devloop code-review · 7 finding(s)（7 条已锚到 diff） |
2 participants
No description provided.