Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion packages/cli/src/cli.ts
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,7 @@ import {
startLaunchMetadataRecording,
type LaunchMetadataRun
} from './launch-metadata.js';
import { createLogger } from './logger.js';
import {
buildPersonaSourceDirectories,
defaultCwdPersonaDir,
Expand All @@ -95,6 +96,8 @@ import { runPersonaCompileCommand } from './persona-compile.js';
import { pickPersona, type PickCandidate, type PickResult } from './persona-picker.js';
import { recordRecent, loadRecents, runPersonaPickerTui, type TuiCandidate } from './persona-tui.js';

const launchMetadataLog = createLogger('launch-metadata');

const USAGE = `Usage: agentworkforce <command> [args...]

Run with no arguments inside a TTY to open an interactive persona picker —
Expand Down Expand Up @@ -1729,7 +1732,11 @@ async function runInteractive(
personaSource: options.personaSource,
cwd,
noLaunchMetadata: options.noLaunchMetadata,
env: process.env
env: process.env,
// Launch metadata ingest runs in the background while the user is reading
// persona output; route its warnings to the CLI log file rather than the
// terminal so a transient backend hiccup doesn't surface as scary noise.
onWarn: (msg) => launchMetadataLog.warn(msg)
});

const inputEnv = inputResolution.values;
Expand Down
140 changes: 140 additions & 0 deletions packages/cli/src/logger.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,140 @@
/**
* Lightweight file logger for the AgentWorkforce CLI.
*
* Modeled on Agent Relay's logger (relay/packages/utils/src/logger.ts):
* - Writes structured lines to a log file so diagnostics never pollute the
* terminal the user is staring at while a persona launches.
* - Configurable via environment variables, read at call time for late binding.
* - No external dependencies.
*
* Default log file: `<workforce home>/logs/cli.log`
* (i.e. `~/.agentworkforce/workforce/logs/cli.log`).
* Overrides:
* AGENTWORKFORCE_LOG_FILE absolute path to the log file ('' or '-' → stderr)
* AGENTWORKFORCE_LOG_LEVEL DEBUG | INFO | WARN | ERROR (default INFO)
* AGENTWORKFORCE_LOG_JSON '1' to emit JSON lines instead of text
*/

import fs from 'node:fs';
import path from 'node:path';

import { defaultWorkforceHomeDir } from './local-personas.js';

export type LogLevel = 'DEBUG' | 'INFO' | 'WARN' | 'ERROR';

interface LogEntry {
ts: string;
level: LogLevel;
component: string;
msg: string;
[key: string]: unknown;
}

const LEVEL_PRIORITY: Record<LogLevel, number> = {
DEBUG: 0,
INFO: 1,
WARN: 2,
ERROR: 3
};

function defaultLogFile(): string {
return path.join(defaultWorkforceHomeDir(), 'logs', 'cli.log');
}

/**
* Resolve the log destination. Returns `undefined` when the user has opted to
* send logs to stderr (`AGENTWORKFORCE_LOG_FILE` set to '' or '-').
*/
function getLogFile(): string | undefined {
const override = process.env.AGENTWORKFORCE_LOG_FILE;
if (override === undefined) return defaultLogFile();
const trimmed = override.trim();
if (trimmed === '' || trimmed === '-') return undefined;
return trimmed;
}

function getLogLevel(): LogLevel {
return (process.env.AGENTWORKFORCE_LOG_LEVEL ?? 'INFO').toUpperCase() as LogLevel;
}
Comment on lines +56 to +58

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

The getLogLevel function casts the environment variable directly to LogLevel without validating if it is a valid log level. If an invalid log level is provided, LEVEL_PRIORITY[getLogLevel()] will return undefined at runtime. While the fallback ?? LEVEL_PRIORITY.INFO handles this, it is safer and more robust to validate the log level and return a guaranteed valid LogLevel.

Suggested change
function getLogLevel(): LogLevel {
return (process.env.AGENTWORKFORCE_LOG_LEVEL ?? 'INFO').toUpperCase() as LogLevel;
}
function getLogLevel(): LogLevel {
const level = (process.env.AGENTWORKFORCE_LOG_LEVEL ?? 'INFO').toUpperCase();
return level in LEVEL_PRIORITY ? (level as LogLevel) : 'INFO';
}


function isLogJson(): boolean {
return process.env.AGENTWORKFORCE_LOG_JSON === '1';
}

// Track which log directories we've already created so we don't stat per write.
const createdLogDirs = new Set<string>();

function ensureLogDir(logFile: string): void {
const logDir = path.dirname(logFile);
if (!createdLogDirs.has(logDir) && !fs.existsSync(logDir)) {
fs.mkdirSync(logDir, { recursive: true });
createdLogDirs.add(logDir);
}
}
Comment on lines +67 to +73

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

The ensureLogDir function only adds the directory to createdLogDirs if it does not already exist. If the directory already exists (which is the common case after the first run), fs.existsSync will be called on every single log write. Since fs.existsSync is a synchronous I/O operation, this introduces an unnecessary performance bottleneck. We should cache the directory in createdLogDirs regardless of whether we had to create it or if it already existed.

Suggested change
function ensureLogDir(logFile: string): void {
const logDir = path.dirname(logFile);
if (!createdLogDirs.has(logDir) && !fs.existsSync(logDir)) {
fs.mkdirSync(logDir, { recursive: true });
createdLogDirs.add(logDir);
}
}
function ensureLogDir(logFile: string): void {
const logDir = path.dirname(logFile);
if (createdLogDirs.has(logDir)) return;
if (!fs.existsSync(logDir)) {
fs.mkdirSync(logDir, { recursive: true });
}
createdLogDirs.add(logDir);
}


function shouldLog(level: LogLevel): boolean {
return LEVEL_PRIORITY[level] >= (LEVEL_PRIORITY[getLogLevel()] ?? LEVEL_PRIORITY.INFO);
}

function formatMessage(entry: LogEntry): string {
if (isLogJson()) {
return JSON.stringify(entry);
}
const { ts, level, component, msg, ...extra } = entry;
const extraStr =
Object.keys(extra).length > 0
? ' ' +
Object.entries(extra)
.map(([k, v]) => `${k}=${v}`)
.join(' ')
: '';
return `${ts} [${level}] [${component}] ${msg}${extraStr}`;
}

function log(level: LogLevel, component: string, msg: string, extra?: Record<string, unknown>): void {
if (!shouldLog(level)) return;

const entry: LogEntry = {
ts: new Date().toISOString(),
level,
component,
msg,
...extra
};
Comment on lines +97 to +103

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

Standard log fields (ts, level, component, msg) can be accidentally or maliciously overwritten if they are also present in the extra payload, because ...extra is spread after the standard fields. Spreading extra first ensures that standard fields always take precedence and cannot be overwritten.

Suggested change
const entry: LogEntry = {
ts: new Date().toISOString(),
level,
component,
msg,
...extra
};
const entry: LogEntry = {
...extra,
ts: new Date().toISOString(),
level,
component,
msg
};

Comment on lines +97 to +103

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Preserve reserved log fields when merging extra.

Because ...extra is applied after ts, level, component, and msg, a caller can silently overwrite the record metadata while shouldLog() was evaluated against a different level. That makes the emitted line internally inconsistent.

Suggested fix
   const entry: LogEntry = {
-    ts: new Date().toISOString(),
-    level,
-    component,
-    msg,
-    ...extra
+    ...extra,
+    ts: new Date().toISOString(),
+    level,
+    component,
+    msg
   };
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
const entry: LogEntry = {
ts: new Date().toISOString(),
level,
component,
msg,
...extra
};
const entry: LogEntry = {
...extra,
ts: new Date().toISOString(),
level,
component,
msg
};
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/cli/src/logger.ts` around lines 97 - 103, The current construction
of the LogEntry lets callers overwrite reserved fields because ...extra is
spread after ts/level/component/msg; update the merge so reserved fields in
entry cannot be clobbered by extra: either filter out reserved keys from the
extra object (ts, level, component, msg) before spreading, or spread extra first
and then spread the fixed fields (ts, level, component, msg) to overwrite any
duplicates; update the code around the LogEntry creation in
packages/cli/src/logger.ts (the entry variable / LogEntry construction) and
ensure shouldLog() still uses the intended level value.


const formatted = formatMessage(entry);

const logFile = getLogFile();
if (logFile) {
try {
ensureLogDir(logFile);
fs.appendFileSync(logFile, formatted + '\n');
return;
} catch {
// Fall through to stderr if the log file is unwritable — never throw
// from a logging call.
}
}

// No log file configured (or it failed): fall back to stderr so the
// diagnostic isn't lost entirely. We never write to stdout.
process.stderr.write(formatted + '\n');
Comment on lines +79 to +121

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Keep the logger's no-throw guarantee on the fallback path.

formatMessage() can still throw in JSON mode for cyclic or BigInt extras, and process.stderr.write() can throw if stderr has already been destroyed. Either case escapes log() today, which breaks the module's explicit "never throws from a log call" contract.

Suggested fix
 function log(level: LogLevel, component: string, msg: string, extra?: Record<string, unknown>): void {
   if (!shouldLog(level)) return;
 
-  const entry: LogEntry = {
-    ts: new Date().toISOString(),
-    level,
-    component,
-    msg,
-    ...extra
-  };
-
-  const formatted = formatMessage(entry);
-
-  const logFile = getLogFile();
-  if (logFile) {
-    try {
-      ensureLogDir(logFile);
-      fs.appendFileSync(logFile, formatted + '\n');
-      return;
-    } catch {
-      // Fall through to stderr if the log file is unwritable — never throw
-      // from a logging call.
-    }
-  }
-
-  // No log file configured (or it failed): fall back to stderr so the
-  // diagnostic isn't lost entirely. We never write to stdout.
-  process.stderr.write(formatted + '\n');
+  try {
+    const entry: LogEntry = {
+      ...extra,
+      ts: new Date().toISOString(),
+      level,
+      component,
+      msg
+    };
+    const line = formatMessage(entry) + '\n';
+
+    const logFile = getLogFile();
+    if (logFile) {
+      try {
+        ensureLogDir(logFile);
+        fs.appendFileSync(logFile, line);
+        return;
+      } catch {
+        // Fall through to stderr if the log file is unwritable.
+      }
+    }
+
+    try {
+      // No log file configured (or it failed): fall back to stderr so the
+      // diagnostic isn't lost entirely. We never write to stdout.
+      process.stderr.write(line);
+    } catch {
+      // Swallow to preserve the logger's no-throw contract.
+    }
+  } catch {
+    // Swallow formatting/serialization failures too.
+  }
 }
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/cli/src/logger.ts` around lines 79 - 121, The log() function can
still throw because formatMessage() (when isLogJson() is true) or
process.stderr.write() may throw; update log() and/or formatMessage() so all
formatting and write operations are wrapped in try/catch paths that never
propagate exceptions: call formatMessage() inside a try block and if it throws,
build a minimal safe fallback string (e.g., `${new Date().toISOString()}
[${level}] [${component}] ${msg}` or include a short "[logging_error]" marker)
and continue; also wrap the final writes (fs.appendFileSync and
process.stderr.write) in try/catch and swallow any errors (no throw), ensuring
the function always returns void; reference formatMessage, isLogJson, log,
getLogFile, ensureLogDir, and process.stderr.write when making the changes.

}

/**
* Create a logger for a specific component.
* @param component - Component name (e.g. 'launch-metadata', 'persona-install').
*/
export function createLogger(component: string) {
return {
debug: (msg: string, extra?: Record<string, unknown>) => log('DEBUG', component, msg, extra),
info: (msg: string, extra?: Record<string, unknown>) => log('INFO', component, msg, extra),
warn: (msg: string, extra?: Record<string, unknown>) => log('WARN', component, msg, extra),
error: (msg: string, extra?: Record<string, unknown>) => log('ERROR', component, msg, extra)
};
}

/** Absolute path of the active log file, or `undefined` when logging to stderr. */
export function activeLogFile(): string | undefined {
return getLogFile();
}
Loading