stack0:feat: add shared contracts and core utility baseline#159
stack0:feat: add shared contracts and core utility baseline#159VX1D wants to merge 6 commits intomichaelshimeles:mainfrom
Conversation
|
@VX1D is attempting to deploy a commit to the Goshen Labs Team on Vercel. A member of the Team first needs to authorize it. |
|
Related Documentation 2 document(s) may need updating based on files changed in this PR: Goshen Labs's Space Copilot AI Engine IntegrationView Suggested Changes@@ -92,9 +92,10 @@
discord_webhook: "https://discord.com/api/webhooks/..."
slack_webhook: "https://hooks.slack.com/services/..."
custom_webhook: "https://your-api.com/webhook"
+ telemetry_webhook: "https://your-api.com/telemetry" # optional telemetry export hook
```
-Notifications include task completion counts and status (completed/failed).
+Notifications include task completion counts and status (completed/failed). The `telemetry_webhook` is an optional field for exporting telemetry data to a custom endpoint, similar to how the custom webhook works but specifically for telemetry events.
---
Error Handling and Model Override for OpenCode EngineView Suggested Changes@@ -69,8 +69,26 @@
#### Rationale
Model override allows you to use more capable models for planning and faster or cheaper models for execution, enabling flexible and cost-effective workflows["Rationale"](https://github.com/michaelshimeles/ralphy/pull/32).
-### Rationale
-You can now pass arbitrary engine-specific arguments to OpenCode (and all supported engines) using the standard CLI `--` separator. This allows you to leverage advanced features or experimental flags provided by the engine, even if they are not explicitly supported by ralphy's own CLI options.
+### OpenCode Debugging Configuration
+Ralphy provides a `debugOpenCode` runtime option to enable comprehensive debugging output for the OpenCode engine. This option is disabled by default and can be enabled when troubleshooting OpenCode-specific issues.
+
+#### Configuration Methods
+**Via configuration file:**
+
+```yaml
+debugOpenCode: true # Enable comprehensive OpenCode debugging
+```
+
+**Via CLI flag:**
+
+```bash
+ralphy --debug-opencode --opencode "Implement OAuth2 login"
+```
+
+When enabled, this option provides detailed debugging information about OpenCode engine execution, including command invocation, output streaming, and error handling. This is particularly useful for diagnosing issues with OpenCode integration or investigating unexpected behavior.
+
+### Engine-Specific Arguments
+You can pass arbitrary engine-specific arguments to OpenCode (and all supported engines) using the standard CLI `--` separator. This allows you to leverage advanced features or experimental flags provided by the engine, even if they are not explicitly supported by ralphy's own CLI options.
#### Example: Passing Engine-Specific Arguments
Note: You must be authenticated to accept/decline updates. How did I do? Any feedback? |
Greptile SummaryThis PR introduces the shared utility and contract foundation for the Ralphy CLI stack: a structured logging layer ( The foundational intent is sound and the architecture is well-organized, but six concrete issues require attention:
Confidence Score: 3/5
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[CLI / Runtime] -->|"setDebug / setVerbose"| B[logger.ts\nloggerState]
A -->|"initializeStructuredLogging"| C[MultiLogSink]
C --> D[ConsoleLogSink]
C --> E[FilteredLogSink]
E --> F[JsonFileLogSink\nsetInterval flush timer]
A -->|"indexWorkspace"| G[file-indexer.ts\nindexCache TTL check]
G -->|cache miss| H[performIndexing]
H -->|disk cache hit| I[incrementalUpdateIndex\nasync I/O ✓]
H -->|no disk cache| J[collectFiles\nreaddirAsync ✓]
J --> K[createFileIndexEntry\nstatAsync + readFileAsync]
K --> L[saveIndexToDisk]
G -->|cache hit & fresh| M[cloneFileIndex → caller]
A -->|"log(level, ...)"| N{verboseMode?}
N -->|debug + not verbose| O[drop message]
N -->|else| P[createLogEntry\nsanitizeSecrets]
P --> Q[logSink.write]
A -->|"createErrorWithContext"| R[standardizeError]
R --> S{instanceof RalphyError?}
S -->|yes| T[return error as-is]
S -->|no| U[wrap in RalphyError]
T --> V[createErrorWithContext wraps in RalphyError ⚠️]
U --> V
A -->|"parseJsonLine"| W[boundary scan\nbrace depth counter]
W --> X[StreamJsonEventSchema.parse]
X --> Y[StreamJsonEvent]
A -->|"formatParsedStep"| Z{step.field?}
Z -->|writing/reading/running| AA[truncated?]
Z -->|thought/executed/raw| AB[some truncated ✓]
AA --> AC{inconsistent ⚠️}
AB --> AC
Last reviewed commit: 2dc0883 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
cli/src/utils/cleanup.ts
Outdated
| if (proc.connected || proc.pid) { | ||
| proc.kill("SIGKILL"); | ||
|
|
||
| // Final verification | ||
| await new Promise((resolve) => setTimeout(resolve, 500)); | ||
| if (proc.connected || proc.pid) { | ||
| } |
There was a problem hiding this comment.
Process liveness check always evaluates to true — SIGTERM grace period is bypassed
proc.pid is assigned at spawn time and is never cleared when a child process exits. This means if (proc.connected || proc.pid) always evaluates to true for any process that was successfully spawned, regardless of whether it has exited. As a result, SIGKILL is unconditionally sent after every SIGTERM — the 1-second graceful-shutdown window is never actually honored.
The correct check for process liveness is proc.exitCode === null && proc.signalCode === null:
| if (proc.connected || proc.pid) { | |
| proc.kill("SIGKILL"); | |
| // Final verification | |
| await new Promise((resolve) => setTimeout(resolve, 500)); | |
| if (proc.connected || proc.pid) { | |
| } | |
| // Wait a bit and verify it's dead | |
| await new Promise((resolve) => setTimeout(resolve, 1000)); | |
| // Check if process is still running | |
| if (proc.exitCode === null && proc.signalCode === null) { | |
| proc.kill("SIGKILL"); | |
| // Final verification | |
| await new Promise((resolve) => setTimeout(resolve, 500)); | |
| if (proc.exitCode === null && proc.signalCode === null) { | |
| logWarn(`Process ${proc.pid} may still be running after SIGKILL`); | |
| } | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: cli/src/utils/cleanup.ts
Line: 68-74
Comment:
**Process liveness check always evaluates to true — SIGTERM grace period is bypassed**
`proc.pid` is assigned at spawn time and is **never cleared** when a child process exits. This means `if (proc.connected || proc.pid)` always evaluates to `true` for any process that was successfully spawned, regardless of whether it has exited. As a result, SIGKILL is unconditionally sent after every SIGTERM — the 1-second graceful-shutdown window is never actually honored.
The correct check for process liveness is `proc.exitCode === null && proc.signalCode === null`:
```suggestion
// Wait a bit and verify it's dead
await new Promise((resolve) => setTimeout(resolve, 1000));
// Check if process is still running
if (proc.exitCode === null && proc.signalCode === null) {
proc.kill("SIGKILL");
// Final verification
await new Promise((resolve) => setTimeout(resolve, 500));
if (proc.exitCode === null && proc.signalCode === null) {
logWarn(`Process ${proc.pid} may still be running after SIGKILL`);
}
}
```
How can I resolve this? If you propose a fix, please make it concise.
cli/src/utils/ai-output-parser.ts
Outdated
| if (TextSchema.safeParse(event).success) { | ||
| const textEvent = TextSchema.parse(event); |
There was a problem hiding this comment.
Double-parse with safeParse + parse — redundant work
The code validates with safeParse, discards the result, then parses again with parse. This is inefficient — use the .data property from safeParse directly:
| if (TextSchema.safeParse(event).success) { | |
| const textEvent = TextSchema.parse(event); | |
| const textResult = TextSchema.safeParse(event); | |
| if (textResult.success) { | |
| const textEvent = textResult.data; |
Prompt To Fix With AI
This is a comment left during a code review.
Path: cli/src/utils/ai-output-parser.ts
Line: 31-32
Comment:
**Double-parse with `safeParse` + `parse` — redundant work**
The code validates with `safeParse`, discards the result, then parses again with `parse`. This is inefficient — use the `.data` property from `safeParse` directly:
```suggestion
const textResult = TextSchema.safeParse(event);
if (textResult.success) {
const textEvent = textResult.data;
```
How can I resolve this? If you propose a fix, please make it concise.![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
| export function setLogSink(sink: LogSink): void { | ||
| logSink = sink; | ||
| } |
There was a problem hiding this comment.
🟢 Low ui/logger.ts:140
JsonFileLogSink has a live interval but no lifecycle management: replacing sinks via setLogSink doesn’t dispose the old one, initializeStructuredLogging returns nothing to dispose, and the interval can keep the process alive. Suggest adding a dispose contract to LogSink, calling it in setLogSink, returning a disposable from initializeStructuredLogging (or exposing a shutdown), and unref() the interval so CLIs can exit cleanly.
| export function setLogSink(sink: LogSink): void { | |
| logSink = sink; | |
| } | |
| export function setLogSink(sink: LogSink): void { | |
| if ((logSink as { dispose?: () => void }).dispose) { | |
| (logSink as { dispose: () => void }).dispose(); | |
| } | |
| logSink = sink; | |
| } |
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file cli/src/ui/logger.ts around lines 140-142:
`JsonFileLogSink` has a live interval but no lifecycle management: replacing sinks via `setLogSink` doesn’t dispose the old one, `initializeStructuredLogging` returns nothing to dispose, and the interval can keep the process alive. Suggest adding a `dispose` contract to `LogSink`, calling it in `setLogSink`, returning a disposable from `initializeStructuredLogging` (or exposing a shutdown), and `unref()` the interval so CLIs can exit cleanly.
Evidence trail:
cli/src/ui/logger.ts lines 82-84 (LogSink interface only has write), lines 123-125 (setLogSink just reassigns without disposing old sink), lines 248-254 (setInterval created without .unref()), lines 291-299 (dispose method exists but not called by system), lines 329-346 (initializeStructuredLogging returns void)
| // Validate path to prevent path traversal attacks | ||
| this.filePath = validateLogPath(filePath); |
There was a problem hiding this comment.
🟡 Medium ui/logger.ts:269
appendFileSync throws ENOENT when the logs directory doesn't exist. Since validateLogPath enforces paths within logs but never creates it, a fresh execution fails silently with buffered logs and console errors. Consider adding fs.mkdirSync(this.allowedDir, { recursive: true }) in the constructor before any file operations.
+ import { mkdirSync } from "node:fs";
+ mkdirSync(allowedDir, { recursive: true });
this.filePath = validateLogPath(filePath);🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file cli/src/ui/logger.ts around lines 269-270:
`appendFileSync` throws `ENOENT` when the `logs` directory doesn't exist. Since `validateLogPath` enforces paths within `logs` but never creates it, a fresh execution fails silently with buffered logs and console errors. Consider adding `fs.mkdirSync(this.allowedDir, { recursive: true })` in the constructor before any file operations.
Evidence trail:
cli/src/ui/logger.ts:14 - `ALLOWED_LOG_DIR = "logs"`
cli/src/ui/logger.ts:17-27 - `validateLogPath` validates path is within `logs` but does not create directory
cli/src/ui/logger.ts:229-246 - `JsonFileLogSink` constructor calls `validateLogPath(filePath)` but no `mkdirSync` call
cli/src/ui/logger.ts:269 - `appendFileSync(this.filePath, lines, "utf-8")` throws ENOENT if directory doesn't exist
cli/src/ui/logger.ts:271-280 - catch block logs error to console and keeps logs buffered
cli/src/utils/cleanup.ts
Outdated
| await new Promise((resolve) => setTimeout(resolve, 1000)); | ||
|
|
||
| // Check if process is still running | ||
| if (proc.connected || proc.pid) { |
There was a problem hiding this comment.
🟢 Low utils/cleanup.ts:68
The SIGKILL fallback (lines 68-75) always executes because proc.pid is defined even after the process exits. This causes unnecessary delays and redundant kill signals. Replace the condition with proc.exitCode === null to check if the process is actually still running.
- if (proc.connected || proc.pid) {
+ if (proc.exitCode === null) {🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file cli/src/utils/cleanup.ts around line 68:
The `SIGKILL` fallback (lines 68-75) always executes because `proc.pid` is defined even after the process exits. This causes unnecessary delays and redundant kill signals. Replace the condition with `proc.exitCode === null` to check if the process is actually still running.
Evidence trail:
cli/src/utils/cleanup.ts lines 68-75 showing condition `if (proc.connected || proc.pid) { proc.kill("SIGKILL"); ... }`. Node.js documentation at https://nodejs.org/api/child_process.html confirms subprocess.pid is set at spawn and only undefined if spawning fails - it doesn't become undefined when process exits. Documentation also confirms subprocess.exitCode is null while process is running.
| export function standardizeError(error: unknown): RalphyError { | ||
| if (error instanceof RalphyError) { | ||
| return error; | ||
| } | ||
|
|
||
| if (error instanceof Error) { | ||
| return new RalphyError(error.message, "UNKNOWN_ERROR", { | ||
| originalName: error.name, | ||
| originalStack: error.stack, | ||
| }); | ||
| } | ||
|
|
||
| if (typeof error === "string") { | ||
| return new RalphyError(error, "STRING_ERROR"); | ||
| } | ||
|
|
||
| return new RalphyError(String(error), "UNKNOWN_ERROR", { originalType: typeof error }); |
There was a problem hiding this comment.
🟡 Medium utils/errors.ts:60
standardizeError drops important metadata for non-RalphyError inputs (plain objects stringify to "[object Object]" and Error.code is ignored), which breaks retry decisions. Suggest preserving message/code when given objects, keeping Error.code (and merging other fields into context) so isRetryableError can reliably detect system error codes.
- if (typeof error === "string") {
- return new RalphyError(error, "STRING_ERROR");
- }
-
- return new RalphyError(String(error), "UNKNOWN_ERROR", { originalType: typeof error });🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file cli/src/utils/errors.ts around lines 60-76:
`standardizeError` drops important metadata for non-`RalphyError` inputs (plain objects stringify to "[object Object]" and `Error.code` is ignored), which breaks retry decisions. Suggest preserving `message`/`code` when given objects, keeping `Error.code` (and merging other fields into `context`) so `isRetryableError` can reliably detect system error codes.
Evidence trail:
cli/src/utils/errors.ts lines 60-76 (REVIEWED_COMMIT): `standardizeError` function shows:
- Line 66-70: Error handling preserves `originalName` and `originalStack` but not `error.code`
- Line 75: Plain objects become `String(error)` which is `"[object Object]"`
- Lines 82-109: `isRetryableError` checks `standardized.code` against retryable codes, meaning lost `Error.code` values won't be detected by code-based checks
| function cloneFileIndex(index: FileIndex): FileIndex { | ||
| return { | ||
| version: index.version, | ||
| timestamp: index.timestamp, | ||
| workDir: index.workDir, | ||
| files: new Map(index.files), | ||
| totalFiles: index.totalFiles, | ||
| totalSize: index.totalSize, | ||
| }; | ||
| } |
There was a problem hiding this comment.
🟢 Low utils/file-indexer.ts:92
The cloneFileIndex function returns a shallow clone of the files Map, so the keywords arrays in FileIndexEntry objects are shared between the returned index and the internal indexCache. Modifying entry.keywords after calling indexWorkspace corrupts the cached entry for that file, affecting all future callers.
-function cloneFileIndex(index: FileIndex): FileIndex {
- return {
- version: index.version,
- timestamp: index.timestamp,
- workDir: index.workDir,
- files: new Map(index.files),
- totalFiles: index.totalFiles,
- totalSize: index.totalSize,
- };
-}🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file cli/src/utils/file-indexer.ts around lines 92-101:
The `cloneFileIndex` function returns a shallow clone of the `files` Map, so the `keywords` arrays in `FileIndexEntry` objects are shared between the returned index and the internal `indexCache`. Modifying `entry.keywords` after calling `indexWorkspace` corrupts the cached entry for that file, affecting all future callers.
Evidence trail:
cli/src/utils/file-indexer.ts lines 87-99 (cloneFileIndex function with JSDoc claiming 'Deep clone' but using `new Map(index.files)` which is shallow), lines 78-80 (indexCache definition as module-level Map), lines 458-461 (indexWorkspace returning `cloneFileIndex(cached)`), lines 27-43 (FileIndexEntry interface showing `keywords: string[]` which would be shared by reference)
| function shouldIgnoreFile(filePath: string, ignorePatterns: string[]): boolean { | ||
| const normalizedPath = filePath.replace(/\\/g, "/"); | ||
|
|
||
| for (const pattern of ignorePatterns) { | ||
| if (matchesGlob(normalizedPath, pattern)) { | ||
| return true; | ||
| } | ||
| } | ||
|
|
||
| return false; |
There was a problem hiding this comment.
🟢 Low utils/file-indexer.ts:113
shouldIgnoreFile normalizes filePath to forward slashes, but passes ignorePatterns unchanged to matchesGlob. On Windows, a pattern like temp\** becomes a regex matching literal backslashes, while the path contains forward slashes — the pattern never matches and files are incorrectly indexed.
-function shouldIgnoreFile(filePath: string, ignorePatterns: string[]): boolean {
- const normalizedPath = filePath.replace(/\\/g, "/");
+ const normalizedPath = filePath.replace(/\\/g, "/");
+ const normalizedPatterns = ignorePatterns.map((p) => p.replace(/\\/g, "/"));
- for (const pattern of ignorePatterns) {
+ for (const pattern of normalizedPatterns) {
if (matchesGlob(normalizedPath, pattern)) {
return true;
}
}🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file cli/src/utils/file-indexer.ts around lines 113-122:
`shouldIgnoreFile` normalizes `filePath` to forward slashes, but passes `ignorePatterns` unchanged to `matchesGlob`. On Windows, a pattern like `temp\**` becomes a regex matching literal backslashes, while the path contains forward slashes — the pattern never matches and files are incorrectly indexed.
Evidence trail:
cli/src/utils/file-indexer.ts lines 113-121 (shouldIgnoreFile normalizes filePath but not patterns), cli/src/utils/file-indexer.ts lines 141-177 (globToRegex function - line 154 escapes backslashes with `\$&`), cli/src/config/constants.ts lines 50-58 (DEFAULT_IGNORE_PATTERNS - no backslashes, but user patterns can be passed via options.ignorePatterns)
| // Check if another operation is already indexing this workspace | ||
| const existingPromise = indexingPromises.get(workDir); | ||
| if (existingPromise) { | ||
| logDebug(`Waiting for concurrent indexing of ${workDir}...`); | ||
| const result = await existingPromise; | ||
| // Return a clone even from the concurrent operation's result | ||
| return cloneFileIndex(result); | ||
| } |
There was a problem hiding this comment.
🟡 Medium utils/file-indexer.ts:607
forceRebuild can return stale results: indexWorkspace joins an in‑flight promise even when forceRebuild is true (and rebuildFileIndex inherits this). Suggest ignoring existing promises when forceRebuild is true and always starting a fresh indexing.
- // Check if another operation is already indexing this workspace
- const existingPromise = indexingPromises.get(workDir);
- if (existingPromise) {
- logDebug(`Waiting for concurrent indexing of ${workDir}...`);
- const result = await existingPromise;
- // Return a clone even from the concurrent operation's result
- return cloneFileIndex(result);
- }🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file cli/src/utils/file-indexer.ts around lines 607-614:
`forceRebuild` can return stale results: `indexWorkspace` joins an in‑flight promise even when `forceRebuild` is `true` (and `rebuildFileIndex` inherits this). Suggest ignoring existing promises when `forceRebuild` is `true` and always starting a fresh indexing.
Evidence trail:
cli/src/utils/file-indexer.ts lines 608-614 (REVIEWED_COMMIT) - shows the existing promise check without forceRebuild consideration; lines 603-606 show memory cache correctly checks forceRebuild; line 990-993 shows rebuildFileIndex calls indexWorkspace with forceRebuild: true
| const rawMessage = args | ||
| .map((arg) => (typeof arg === "string" ? arg : JSON.stringify(arg))) | ||
| .join(" "); |
There was a problem hiding this comment.
🟢 Low ui/logger.ts:156
createLogEntry uses JSON.stringify, which throws on circular refs/BigInt and drops details for Error/Map/Set. Suggest a safe serializer (e.g., util.inspect) or a try/catch with a fallback to avoid crashes while preserving useful info.
+function createLogEntry(level: LogLevel, component: string | undefined, args: unknown[]): LogEntry {
+ const rawMessage = args
+ .map((arg) => {
+ if (typeof arg === "string") return arg;
+ try {
+ return JSON.stringify(arg);
+ } catch {
+ return "[unserializable]";
+ }
+ })
+ .join(" ");🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file cli/src/ui/logger.ts around lines 156-158:
`createLogEntry` uses `JSON.stringify`, which throws on circular refs/`BigInt` and drops details for `Error`/`Map`/`Set`. Suggest a safe serializer (e.g., `util.inspect`) or a try/catch with a fallback to avoid crashes while preserving useful info.
Evidence trail:
cli/src/ui/logger.ts lines 155-167 at REVIEWED_COMMIT - `createLogEntry` function uses `JSON.stringify(arg)` on line 158 without try/catch. Standard JavaScript behavior: `JSON.stringify` throws on circular refs and BigInt, returns `{}` for Error/Map/Set.
| // 2. Run registered cleanup functions | ||
| const promises: Promise<void>[] = []; | ||
| for (const fn of cleanupRegistry) { | ||
| try { | ||
| const result = fn(); | ||
| if (result instanceof Promise) { | ||
| promises.push(result); | ||
| } | ||
| } catch (err) { | ||
| // Log sync errors but continue with other cleanup functions | ||
| promises.push(Promise.reject(err)); | ||
| } | ||
| } | ||
|
|
||
| await Promise.allSettled(promises); | ||
| cleanupRegistry.clear(); |
There was a problem hiding this comment.
🟢 Low utils/cleanup.ts:85
The catch block on line 94 pushes rejected Promises into the promises array, but Promise.allSettled results are never inspected — errors from cleanup functions are silently swallowed. This contradicts the comment // Log sync errors and hides failures from the user. Consider logging errors with logWarn before converting them to resolved/rejected promises, or iterate through Promise.allSettled results to log any rejected promises.
try {
const result = fn();
if (result instanceof Promise) {
promises.push(result);
}
} catch (err) {
- // Log sync errors but continue with other cleanup functions
- promises.push(Promise.reject(err));
+ // Log sync errors but continue with other cleanup functions
+ logWarn(`Cleanup function failed: ${err}`);
}
}
- await Promise.allSettled(promises);
+ const results = await Promise.allSettled(promises);
+ for (const result of results) {
+ if (result.status === 'rejected') {
+ logWarn(`Async cleanup function failed: ${result.reason}`);
+ }
+ }🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file cli/src/utils/cleanup.ts around lines 85-100:
The `catch` block on line 94 pushes rejected Promises into the `promises` array, but `Promise.allSettled` results are never inspected — errors from cleanup functions are silently swallowed. This contradicts the comment `// Log sync errors` and hides failures from the user. Consider logging errors with `logWarn` before converting them to resolved/rejected promises, or iterate through `Promise.allSettled` results to log any rejected promises.
Evidence trail:
cli/src/utils/cleanup.ts (REVIEWED_COMMIT): Line 3 imports `logWarn` from logger. Line 86 comment: `// Log sync errors but continue with other cleanup functions`. Line 88: `promises.push(Promise.reject(err))` in catch block - no logging. Line 91: `await Promise.allSettled(promises)` - results discarded without inspection. Errors are silently swallowed despite comment claiming they are logged.
| /** | ||
| * Convert glob pattern to regex | ||
| */ | ||
| function matchesGlob(filePath: string, pattern: string): boolean { | ||
| // Handle ** patterns properly | ||
| const regexPattern = globToRegex(pattern); | ||
| return regexPattern.test(filePath); | ||
| } | ||
|
|
||
| /** | ||
| * Convert glob pattern to regex | ||
| * | ||
| * SECURITY NOTE: This function includes protections against ReDoS attacks: | ||
| * - Input length is limited to MAX_GLOB_PATTERN_LENGTH | ||
| * - Uses non-backtracking patterns where possible | ||
| */ | ||
| function globToRegex(pattern: string): RegExp { | ||
| const safePattern = | ||
| pattern.length > MAX_GLOB_PATTERN_LENGTH | ||
| ? pattern.slice(0, MAX_GLOB_PATTERN_LENGTH) | ||
| : pattern; | ||
|
|
||
| // Limit pattern length to prevent ReDoS attacks | ||
| if (safePattern.length < pattern.length) { | ||
| logDebug(`Glob pattern too long (${pattern.length} > ${MAX_GLOB_PATTERN_LENGTH}), truncating`); | ||
| } | ||
|
|
||
| // Escape special regex characters except * and ? | ||
| // Use a bounded approach to prevent catastrophic backtracking | ||
| let regex = safePattern | ||
| .replace(/[.+^${}()|[\]\\]/g, "\\$&") | ||
| .replace(/\*\*/g, "\0DOUBLESTAR\0") // Temporarily mark ** | ||
| .replace(/\*/g, "[^/]*") // Single * matches anything except / | ||
| .replace(/\?/g, "[^/]"); // ? matches single char except / | ||
|
|
||
| // Handle ** (match any number of directories) using non-capturing group | ||
| // The (?:.*/)? pattern is bounded - it won't cause catastrophic backtracking | ||
| regex = regex.replace(/\0DOUBLESTAR\0/g, "(?:.*/)?"); | ||
|
|
||
| // Handle directory separators | ||
| regex = regex.replace(/\//g, "[/\\\\]"); | ||
|
|
||
| // Anchor to start | ||
| regex = `^${regex}`; | ||
|
|
||
| // Match at end if pattern doesn't end with /** | ||
| if (!safePattern.endsWith("/**")) { | ||
| regex += "$"; | ||
| } | ||
|
|
||
| return new RegExp(regex, "i"); | ||
| } |
There was a problem hiding this comment.
🟡 Medium utils/file-indexer.ts:125
Ignore matching is broken: globToRegex anchors patterns and mishandles **, so patterns like node_modules or src/**/test.ts don’t match nested paths. Suggest using a vetted glob matcher (e.g., picomatch) on POSIX‑normalized paths, or update globToRegex to drop unconditional anchors and have ** consume zero‑or‑more path segments.
- regex = `^${regex}`;
-
- // Match at end if pattern doesn't end with /**
- if (!safePattern.endsWith("/**")) {
- regex += "$";
- }
-
- return new RegExp(regex, "i");
+ // Match anywhere in the path unless pattern starts with /
+ const anchored = safePattern.startsWith("/");
+ regex = anchored ? `^${regex}$` : `(?:^|/)${regex}(?:$|/)`;
+
+ return new RegExp(regex, "i");🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file cli/src/utils/file-indexer.ts around lines 125-176:
Ignore matching is broken: `globToRegex` anchors patterns and mishandles `**`, so patterns like `node_modules` or `src/**/test.ts` don’t match nested paths. Suggest using a vetted glob matcher (e.g., `picomatch`) on POSIX‑normalized paths, or update `globToRegex` to drop unconditional anchors and have `**` consume zero‑or‑more path segments.
Evidence trail:
cli/src/utils/file-indexer.ts lines 140-175 (globToRegex function), line 162 (`**` replaced with `(?:.*/)?`), line 168 (start anchor `^`), lines 170-172 (end anchor `$`). cli/src/config/constants.ts lines 50-55 (DEFAULT_IGNORE_PATTERNS includes `node_modules`, `.git` etc. without `**/` prefix).
| export function initializeStructuredLogging( | ||
| logFilePath?: string, | ||
| minLevel: LogLevel = "info", | ||
| ): void { | ||
| const sinks: LogSink[] = [new ConsoleLogSink()]; | ||
|
|
||
| if (logFilePath) { | ||
| const fileSink = new JsonFileLogSink(logFilePath); | ||
| const filteredFileSink = new FilteredLogSink(fileSink, minLevel); | ||
| sinks.push(filteredFileSink); | ||
| } | ||
|
|
||
| setLogSink(new MultiLogSink(sinks)); | ||
| } |
There was a problem hiding this comment.
JsonFileLogSink flush timer is leaked with no dispose path
initializeStructuredLogging constructs a JsonFileLogSink, which immediately starts a setInterval timer in its constructor. The sink reference is buried inside a FilteredLogSink inside the MultiLogSink stored in the module-level logSink variable. There is no way for a caller to retrieve and call dispose() on this sink — the interval will run for the lifetime of the process, preventing the event loop from draining cleanly on exit.
The function should return a dispose callback (or the JsonFileLogSink instance directly) so callers can stop the timer:
export function initializeStructuredLogging(
logFilePath?: string,
minLevel: LogLevel = "info",
): (() => void) | undefined {
const sinks: LogSink[] = [new ConsoleLogSink()];
let fileSink: JsonFileLogSink | undefined;
if (logFilePath) {
fileSink = new JsonFileLogSink(logFilePath);
const filteredFileSink = new FilteredLogSink(fileSink, minLevel);
sinks.push(filteredFileSink);
}
setLogSink(new MultiLogSink(sinks));
return fileSink ? () => fileSink!.dispose() : undefined;
}Prompt To Fix With AI
This is a comment left during a code review.
Path: cli/src/ui/logger.ts
Line: 392-405
Comment:
**`JsonFileLogSink` flush timer is leaked with no dispose path**
`initializeStructuredLogging` constructs a `JsonFileLogSink`, which immediately starts a `setInterval` timer in its constructor. The sink reference is buried inside a `FilteredLogSink` inside the `MultiLogSink` stored in the module-level `logSink` variable. There is no way for a caller to retrieve and call `dispose()` on this sink — the interval will run for the lifetime of the process, preventing the event loop from draining cleanly on exit.
The function should return a dispose callback (or the `JsonFileLogSink` instance directly) so callers can stop the timer:
```typescript
export function initializeStructuredLogging(
logFilePath?: string,
minLevel: LogLevel = "info",
): (() => void) | undefined {
const sinks: LogSink[] = [new ConsoleLogSink()];
let fileSink: JsonFileLogSink | undefined;
if (logFilePath) {
fileSink = new JsonFileLogSink(logFilePath);
const filteredFileSink = new FilteredLogSink(fileSink, minLevel);
sinks.push(filteredFileSink);
}
setLogSink(new MultiLogSink(sinks));
return fileSink ? () => fileSink!.dispose() : undefined;
}
```
How can I resolve this? If you propose a fix, please make it concise.
| export function setDebug(debug: boolean): void { | ||
| loggerState.debugMode = debug; | ||
| if (debug) { | ||
| loggerState.verboseMode = true; | ||
| } | ||
| verboseMode = loggerState.verboseMode; | ||
| } |
There was a problem hiding this comment.
🟡 Medium ui/logger.ts:55
setDebug(false) disables debugMode but leaves verboseMode as true, so debug logs continue to be printed even though isDebug() returns false. Since log() gates debug messages solely on verboseMode, calling setDebug(false) fails to actually disable debug output. Consider also setting verboseMode to false when debug is false.
export function setDebug(debug: boolean): void {
loggerState.debugMode = debug;
- if (debug) {
- loggerState.verboseMode = true;
+ if (debug) {
+ loggerState.verboseMode = true;
+ } else {
+ loggerState.verboseMode = false;
}
verboseMode = loggerState.verboseMode;
}🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file cli/src/ui/logger.ts around lines 55-61:
`setDebug(false)` disables `debugMode` but leaves `verboseMode` as `true`, so debug logs continue to be printed even though `isDebug()` returns `false`. Since `log()` gates debug messages solely on `verboseMode`, calling `setDebug(false)` fails to actually disable debug output. Consider also setting `verboseMode` to `false` when `debug` is `false`.
Evidence trail:
cli/src/ui/logger.ts lines 54-60 (setDebug function), lines 157-165 (log function checking verboseMode for debug gating), lines 39-41 (isDebug returns debugMode). The setDebug function only sets verboseMode=true when debug=true, with no code to set verboseMode=false when debug=false.
| } | ||
|
|
||
| if (step.thought) { | ||
| // Clean up common "Thinking:" prefixes since we wrap in {} later |
There was a problem hiding this comment.
🟡 Medium utils/ai-output-parser.ts:131
The regex /^(Thinking|Analyzing|Considering|Warning|Waiting)[:\s]*/i corrupts grammatical sentences by matching and stripping the first word when the AI outputs something like "Analyzing the file structure...". The cleaned thought becomes "the file structure...", which is nonsensical. Consider adding a word boundary or requiring a colon after the keyword, e.g., /(Thinking|Analyzing|Considering|Warning|Waiting):\s*/i, so only actual prefixes like "Analyzing: " are stripped.
- const cleanedThought = step.thought.replace(/^(Thinking|Analyzing|Considering|Warning|Waiting)[:\s]*/i, "").trim();
+ const cleanedThought = step.thought.replace(/^(Thinking|Analyzing|Considering|Warning|Waiting):\s*/i, "").trim();🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file cli/src/utils/ai-output-parser.ts around line 131:
The regex `/^(Thinking|Analyzing|Considering|Warning|Waiting)[:\s]*/i` corrupts grammatical sentences by matching and stripping the first word when the AI outputs something like "Analyzing the file structure...". The cleaned thought becomes "the file structure...", which is nonsensical. Consider adding a word boundary or requiring a colon after the keyword, e.g., `/(Thinking|Analyzing|Considering|Warning|Waiting):\s*/i`, so only actual prefixes like "Analyzing: " are stripped.
Evidence trail:
cli/src/utils/ai-output-parser.ts line 131 (REVIEWED_COMMIT): `const cleanedThought = step.thought.replace(/^(Thinking|Analyzing|Considering|Warning|Waiting)[:\s]*/i, "").trim();` - The regex `[:\s]*` matches zero or more colons OR whitespace, so for input 'Analyzing the file structure...', it matches 'Analyzing ' and strips it, leaving 'the file structure...'
| /** | ||
| * Validate log file path to prevent path traversal attacks | ||
| */ | ||
| function validateLogPath(filePath: string): string { | ||
| const resolved = path.resolve(filePath); | ||
| const allowedDir = path.resolve(process.cwd(), ALLOWED_LOG_DIR); | ||
| const relative = path.relative(allowedDir, resolved); | ||
|
|
||
| if (relative.startsWith("..") || path.isAbsolute(relative)) { | ||
| throw new Error(`Invalid log file path: ${filePath} must be within ${ALLOWED_LOG_DIR}`); | ||
| } | ||
|
|
||
| return resolved; | ||
| } |
There was a problem hiding this comment.
🟡 Medium ui/logger.ts:15
validateLogPath returns a path inside the logs directory, but the code never ensures this directory exists. Since appendFileSync in JsonFileLogSink does not create parent directories, logging will throw ENOENT in fresh environments where the logs folder doesn't exist. Consider adding mkdirSync with recursive: true to create the directory before it is used.
function validateLogPath(filePath: string): string {
const resolved = path.resolve(filePath);
const allowedDir = path.resolve(process.cwd(), ALLOWED_LOG_DIR);
const relative = path.relative(allowedDir, resolved);
if (relative.startsWith("..") || path.isAbsolute(relative)) {
throw new Error(`Invalid log file path: ${filePath} must be within ${ALLOWED_LOG_DIR}`);
}
+ // Ensure the logs directory exists before returning the path
+ import { mkdirSync } from "node:fs";
+ mkdirSync(allowedDir, { recursive: true });
return resolved;
}🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file cli/src/ui/logger.ts around lines 15-28:
`validateLogPath` returns a path inside the `logs` directory, but the code never ensures this directory exists. Since `appendFileSync` in `JsonFileLogSink` does not create parent directories, logging will throw `ENOENT` in fresh environments where the `logs` folder doesn't exist. Consider adding `mkdirSync` with `recursive: true` to create the directory before it is used.
Evidence trail:
cli/src/ui/logger.ts lines 15 (ALLOWED_LOG_DIR), 18-27 (validateLogPath), 228-241 (JsonFileLogSink constructor), 276 (appendFileSync call). git_grep for `mkdirSync.*logs` returned no results. git_grep for all `mkdirSync` calls shows no logs directory creation anywhere in the repo.
| function log(level: LogLevel, component: string | undefined, ...args: unknown[]): void { | ||
| // Debug messages only show in verbose or debug mode | ||
| if (level === "debug" && !loggerState.verboseMode) { | ||
| return; | ||
| } | ||
|
|
||
| const entry = createLogEntry(level, component, args); | ||
| logSink.write(entry); | ||
| } |
There was a problem hiding this comment.
🟢 Low ui/logger.ts:173
The log function drops all 'debug' level messages unless verboseMode is enabled, even when a file sink is explicitly configured to capture debug logs via minLevel: 'debug'. This prevents debug data from reaching the file while keeping console output quiet, contradicting the sink's configuration. Consider removing the global verbose check from log and letting each sink handle its own filtering.
function log(level: LogLevel, component: string | undefined, ...args: unknown[]): void {
// Debug messages only show in verbose or debug mode
- if (level === "debug" && !loggerState.verboseMode) {
- return;
- }
-
const entry = createLogEntry(level, component, args);
logSink.write(entry);
}🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file cli/src/ui/logger.ts around lines 173-181:
The `log` function drops all 'debug' level messages unless `verboseMode` is enabled, even when a file sink is explicitly configured to capture debug logs via `minLevel: 'debug'`. This prevents debug data from reaching the file while keeping console output quiet, contradicting the sink's configuration. Consider removing the global verbose check from `log` and letting each sink handle its own filtering.
Evidence trail:
cli/src/ui/logger.ts lines 173-180 (log function with verboseMode check before logSink.write), lines 368-387 (FilteredLogSink class with minLevel), lines 396-408 (initializeStructuredLogging with minLevel parameter passed to FilteredLogSink)
| interface WebhookSessionData { | ||
| sessionId: string; | ||
| engine: string; |
There was a problem hiding this comment.
🟡 Medium telemetry/types.ts:84
WebhookSessionData is missing the timestamp field from the Session interface, so the original session start time is dropped when constructing the webhook payload. The top-level timestamp in TelemetryWebhookPayload records the transmission time, not the session start time. Add timestamp: number to WebhookSessionData to preserve the session's original start time.
interface WebhookSessionData {
sessionId: string;
+ timestamp: number;
engine: string;Also found in 1 other location(s)
cli/src/utils/json-validation.ts:31
The
StepStartSchemadoes not define session ID fields (sessionID,sessionId,session_id) or enable passthrough. Zodz.objectstrips unknown keys by default. Consequently, if the upstreamstep_startevent includes a session ID, it is stripped during parsing, causing theextractSessionIdhelper to returnnulland the session context to be lost.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file cli/src/telemetry/types.ts around lines 84-86:
`WebhookSessionData` is missing the `timestamp` field from the `Session` interface, so the original session start time is dropped when constructing the webhook payload. The top-level `timestamp` in `TelemetryWebhookPayload` records the transmission time, not the session start time. Add `timestamp: number` to `WebhookSessionData` to preserve the session's original start time.
Evidence trail:
- cli/src/telemetry/types.ts lines 17-18: `Session` interface defines `timestamp: number; // Unix ms`
- cli/src/telemetry/types.ts lines 84-100: `WebhookSessionData` interface - no `timestamp` field present
- cli/src/telemetry/webhook.ts line 28: `timestamp: new Date().toISOString()` sets transmission time
- cli/src/telemetry/webhook.ts lines 29-42: `session` object construction copies fields from Session but omits `session.timestamp`
Also found in 1 other location(s):
- cli/src/utils/json-validation.ts:31 -- The `StepStartSchema` does not define session ID fields (`sessionID`, `sessionId`, `session_id`) or enable passthrough. Zod `z.object` strips unknown keys by default. Consequently, if the upstream `step_start` event includes a session ID, it is stripped during parsing, causing the `extractSessionId` helper to return `null` and the session context to be lost.
|
|
||
| const cleanupRegistry: Set<CleanupFn> = new Set(); | ||
| const trackedProcesses: Set<ChildProcess> = new Set(); | ||
| let isCleaningUp = false; |
There was a problem hiding this comment.
🟡 Medium utils/cleanup.ts:9
When runCleanup is called while already running, the boolean isCleaningUp flag causes the second call to return immediately while cleanup continues in the background. The signal handler at line 125 then calls process.exit(0) before cleanup actually completes, terminating the process prematurely and potentially leaving orphaned child processes. Consider replacing the boolean flag with a shared Promise that all callers await.
-let isCleaningUp = false;
+let cleanupPromise: Promise<void> | null = null;🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file cli/src/utils/cleanup.ts around line 9:
When `runCleanup` is called while already running, the boolean `isCleaningUp` flag causes the second call to return immediately while cleanup continues in the background. The signal handler at line 125 then calls `process.exit(0)` before cleanup actually completes, terminating the process prematurely and potentially leaving orphaned child processes. Consider replacing the boolean flag with a shared Promise that all callers await.
Evidence trail:
cli/src/utils/cleanup.ts lines 9, 37-38 (isCleaningUp flag and early return), lines 101-124 (signal handler with separate isShuttingDown flag that awaits runCleanup() then calls process.exit(0)). The defect occurs when runCleanup() is called outside a signal handler, then a signal arrives - the signal handler's runCleanup() returns immediately due to isCleaningUp=true, then process.exit(0) is called before the original cleanup completes.
| const trimmed = step.raw.trim(); | ||
| if (trimmed.startsWith("{") || trimmed.startsWith('"')) { |
There was a problem hiding this comment.
🟡 Medium utils/ai-output-parser.ts:146
The raw output fallback returns null for any message starting with ", which drops valid quoted text strings like "Hello world" when the AI produces plain quoted content. Consider checking for actual JSON structure (e.g., startsWith('{') only, or validating with JSON.parse) instead of treating all double-quoted strings as unparsed JSON.
| const trimmed = step.raw.trim(); | |
| if (trimmed.startsWith("{") || trimmed.startsWith('"')) { | |
| if (trimmed.startsWith("{")) { |
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file cli/src/utils/ai-output-parser.ts around lines 146-147:
The raw output fallback returns `null` for any message starting with `"`, which drops valid quoted text strings like `"Hello world"` when the AI produces plain quoted content. Consider checking for actual JSON structure (e.g., `startsWith('{')` only, or validating with `JSON.parse`) instead of treating all double-quoted strings as unparsed JSON.
Evidence trail:
cli/src/utils/ai-output-parser.ts lines 144-150 (REVIEWED_COMMIT) - shows the check `trimmed.startsWith("{") || trimmed.startsWith('"')` returning null
cli/src/utils/json-validation.ts lines 94-145 (REVIEWED_COMMIT) - shows parseJsonLine only accepts objects matching StreamJsonEventSchema with specific type values, meaning plain JSON strings fail validation
cli/src/utils/ai-output-parser.ts lines 16-18 (REVIEWED_COMMIT) - shows parseAIStep sets `raw: step` when parseJsonLine returns null
| /** | ||
| * Deep clone a FileIndex to return an immutable copy |
There was a problem hiding this comment.
🟡 Medium utils/file-indexer.ts:85
Concurrency lock ignores options. indexWorkspace reuses indexingPromises keyed only by workDir, so calls with different ignorePatterns/maxDepth or forceRebuild can return the wrong index. Suggest keying by workDir + options (or skipping reuse when options differ) and always bypassing reuse when forceRebuild is set.
-// Track promises for workspaces being indexed to allow waiting
-const indexingPromises = new Map<string, Promise<FileIndex>>();
+// Track promises for workspaces being indexed to allow waiting
+interface IndexingKey {
+ workDir: string;
+ ignorePatterns: string[];
+ maxDepth: number;
+ forceRebuild: boolean;
+}
+
+function makeIndexingKey(workDir: string, ignorePatterns: string[], maxDepth: number, forceRebuild: boolean): string {
+ return JSON.stringify({ workDir, ignorePatterns, maxDepth, forceRebuild });
+}
+
+const indexingPromises = new Map<string, Promise<FileIndex>>();🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file cli/src/utils/file-indexer.ts around lines 85-86:
Concurrency lock ignores options. `indexWorkspace` reuses `indexingPromises` keyed only by `workDir`, so calls with different `ignorePatterns`/`maxDepth` or `forceRebuild` can return the wrong index. Suggest keying by `workDir + options` (or skipping reuse when options differ) and always bypassing reuse when `forceRebuild` is set.
Evidence trail:
cli/src/utils/file-indexer.ts lines 87 (indexingPromises typed as Map<string, Promise<FileIndex>>), lines 615-621 (reuse check using only workDir key), line 624 (set only by workDir), lines 604-608 (function signature with options). The code confirms the promise map is keyed solely by workDir without considering options, and forceRebuild is bypassed when an existing promise exists.
| // Limit input length to prevent ReDoS attacks | ||
| if (input.length > MAX_SANITIZE_INPUT_LENGTH) { | ||
| // For very large inputs, truncate and add warning | ||
| const truncated = input.slice(0, MAX_SANITIZE_INPUT_LENGTH); | ||
| return `${truncated}\n\n[WARNING: Content truncated due to size limits during secret sanitization]`; | ||
| } |
There was a problem hiding this comment.
🟡 Medium utils/sanitization.ts:24
When input.length > MAX_SANITIZE_INPUT_LENGTH, the code returns the truncated string immediately without passing it through the sanitization patterns, so secrets in the first 1MB are logged in cleartext. Pass the truncated content through the sanitization loop before returning.
-const truncated = input.slice(0, MAX_SANITIZE_INPUT_LENGTH);
- return `${truncated}\n\n[WARNING: Content truncated due to size limits during secret sanitization]`;
+ input = input.slice(0, MAX_SANITIZE_INPUT_LENGTH);🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file cli/src/utils/sanitization.ts around lines 24-29:
When `input.length > MAX_SANITIZE_INPUT_LENGTH`, the code returns the truncated string immediately without passing it through the sanitization patterns, so secrets in the first 1MB are logged in cleartext. Pass the truncated content through the sanitization loop before returning.
Evidence trail:
cli/src/utils/sanitization.ts lines 24-28 (REVIEWED_COMMIT): Early return when input.length > MAX_SANITIZE_INPUT_LENGTH returns truncated string directly without sanitization.
cli/src/utils/sanitization.ts lines 30-42 (REVIEWED_COMMIT): Sanitization patterns are defined and applied only AFTER the length check, so they are never reached for oversized inputs.
There was a problem hiding this comment.
🟡 Medium ui/logger.ts:396
initializeStructuredLogging creates a JsonFileLogSink that starts a persistent setInterval timer, but the function returns nothing and provides no way to access the sink instance. The timer prevents the Node.js process from exiting naturally, causing the CLI to hang after completing its work. Consider returning the MultiLogSink so callers can dispose the file sink when done.
export function initializeStructuredLogging(
logFilePath?: string,
minLevel: LogLevel = "info",
-): void {
+): MultiLogSink {
const sinks: LogSink[] = [new ConsoleLogSink()];
if (logFilePath) {
const fileSink = new JsonFileLogSink(logFilePath);
const filteredFileSink = new FilteredLogSink(fileSink, minLevel);
sinks.push(filteredFileSink);
}
- setLogSink(new MultiLogSink(sinks));
+ const sink = new MultiLogSink(sinks);
+ setLogSink(sink);
+ return sink;
}🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file cli/src/ui/logger.ts around lines 396-409:
`initializeStructuredLogging` creates a `JsonFileLogSink` that starts a persistent `setInterval` timer, but the function returns nothing and provides no way to access the sink instance. The timer prevents the Node.js process from exiting naturally, causing the CLI to hang after completing its work. Consider returning the `MultiLogSink` so callers can dispose the file sink when done.
Evidence trail:
cli/src/ui/logger.ts lines 396-409 (`initializeStructuredLogging` function returning void), lines 260-282 (`JsonFileLogSink` constructor with `setInterval`), lines 328-336 (`dispose()` method), lines 343-365 (`MultiLogSink` class without dispose method), lines 92-94 (`LogSink` interface with only `write()` method), lines 140-142 and 147-149 (`setLogSink` and `getLogSink` functions)
| export const DEFAULT_IGNORE_PATTERNS = [ | ||
| ".git", | ||
| "node_modules", | ||
| ".ralphy-sandboxes", | ||
| ".ralphy-worktrees", | ||
| ".ralphy", | ||
| "agent-*", | ||
| "sandbox-*", | ||
| ]; |
There was a problem hiding this comment.
DEFAULT_IGNORE_PATTERNS only matches root-level directories
The glob patterns here (e.g. node_modules, .git) compile to anchored regexes like ^node_modules$ and ^\.git$ (see globToRegex in file-indexer.ts lines 161–180). This means a top-level node_modules is correctly ignored. However, in monorepos with nested directories — e.g. packages/app/node_modules or packages/web/node_modules — the relative paths like packages/app/node_modules will not match ^node_modules$, causing the entire nested dependency tree to be indexed, resulting in severe performance degradation and bloated cache.
The patterns need a **/ prefix to match at any depth:
| export const DEFAULT_IGNORE_PATTERNS = [ | |
| ".git", | |
| "node_modules", | |
| ".ralphy-sandboxes", | |
| ".ralphy-worktrees", | |
| ".ralphy", | |
| "agent-*", | |
| "sandbox-*", | |
| ]; | |
| export const DEFAULT_IGNORE_PATTERNS = [ | |
| "**/.git", | |
| "**/node_modules", | |
| "**/.ralphy-sandboxes", | |
| "**/.ralphy-worktrees", | |
| "**/.ralphy", | |
| "agent-*", | |
| "sandbox-*", | |
| ]; |
Prompt To Fix With AI
This is a comment left during a code review.
Path: cli/src/config/constants.ts
Line: 52-60
Comment:
**`DEFAULT_IGNORE_PATTERNS` only matches root-level directories**
The glob patterns here (e.g. `node_modules`, `.git`) compile to anchored regexes like `^node_modules$` and `^\.git$` (see `globToRegex` in `file-indexer.ts` lines 161–180). This means a top-level `node_modules` is correctly ignored. However, in monorepos with nested directories — e.g. `packages/app/node_modules` or `packages/web/node_modules` — the relative paths like `packages/app/node_modules` will **not** match `^node_modules$`, causing the entire nested dependency tree to be indexed, resulting in severe performance degradation and bloated cache.
The patterns need a `**/` prefix to match at any depth:
```suggestion
export const DEFAULT_IGNORE_PATTERNS = [
"**/.git",
"**/node_modules",
"**/.ralphy-sandboxes",
"**/.ralphy-worktrees",
"**/.ralphy",
"agent-*",
"sandbox-*",
];
```
How can I resolve this? If you propose a fix, please make it concise.| // All patterns use bounded quantifiers to prevent ReDoS | ||
| // Patterns are designed to match specific token formats with fixed lengths | ||
| const patterns = [ | ||
| { regex: /sk-[a-zA-Z0-9]{48}/g, replacement: "[API_KEY_REDACTED]" }, |
There was a problem hiding this comment.
sk- pattern won't match Anthropic API keys
The pattern sk-[a-zA-Z0-9]{48} was designed for the legacy OpenAI key format. Anthropic API keys — the primary credential for this Claude-based CLI — follow the format sk-ant-api03-[base64-like segment], which contains hyphens and has a different overall length. Because the regex only allows [a-zA-Z0-9] after the sk- prefix, it will never match a real Anthropic key, and those keys will leak into logs unredacted.
Broaden the pattern to cover both formats:
| { regex: /sk-[a-zA-Z0-9]{48}/g, replacement: "[API_KEY_REDACTED]" }, | |
| { regex: /sk-[a-zA-Z0-9-]{20,100}/g, replacement: "[API_KEY_REDACTED]" }, |
Using a bounded range {20,100} preserves ReDoS protection while covering both the legacy format and the sk-ant-api03-... family.
Prompt To Fix With AI
This is a comment left during a code review.
Path: cli/src/utils/sanitization.ts
Line: 34
Comment:
**`sk-` pattern won't match Anthropic API keys**
The pattern `sk-[a-zA-Z0-9]{48}` was designed for the legacy OpenAI key format. Anthropic API keys — the primary credential for this Claude-based CLI — follow the format `sk-ant-api03-[base64-like segment]`, which contains **hyphens** and has a different overall length. Because the regex only allows `[a-zA-Z0-9]` after the `sk-` prefix, it will never match a real Anthropic key, and those keys will leak into logs unredacted.
Broaden the pattern to cover both formats:
```suggestion
{ regex: /sk-[a-zA-Z0-9-]{20,100}/g, replacement: "[API_KEY_REDACTED]" },
```
Using a bounded range `{20,100}` preserves ReDoS protection while covering both the legacy format and the `sk-ant-api03-...` family.
How can I resolve this? If you propose a fix, please make it concise.| export const StepStartSchema = z.object({ | ||
| type: z.literal("step_start"), | ||
| }); |
There was a problem hiding this comment.
StepStartSchema silently drops session IDs
StepFinishSchema (lines 25–28) explicitly declares sessionID, sessionId, and session_id as optional fields so extractSessionId can recover the session context from finish events. StepStartSchema has no such fields, and Zod's z.object strips unknown keys by default. If the upstream AI engine attaches a session ID to a step_start event, it will be silently removed. Any downstream call to extractSessionId(event) will return null, losing the session context from the very first event.
Add the same session ID fields to StepStartSchema:
| export const StepStartSchema = z.object({ | |
| type: z.literal("step_start"), | |
| }); | |
| export const StepStartSchema = z.object({ | |
| type: z.literal("step_start"), | |
| // Session ID fields (various naming conventions) | |
| sessionID: z.string().optional(), | |
| sessionId: z.string().optional(), | |
| session_id: z.string().optional(), | |
| }); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: cli/src/utils/json-validation.ts
Line: 31-33
Comment:
**`StepStartSchema` silently drops session IDs**
`StepFinishSchema` (lines 25–28) explicitly declares `sessionID`, `sessionId`, and `session_id` as optional fields so `extractSessionId` can recover the session context from finish events. `StepStartSchema` has no such fields, and Zod's `z.object` strips unknown keys by default. If the upstream AI engine attaches a session ID to a `step_start` event, it will be silently removed. Any downstream call to `extractSessionId(event)` will return `null`, losing the session context from the very first event.
Add the same session ID fields to `StepStartSchema`:
```suggestion
export const StepStartSchema = z.object({
type: z.literal("step_start"),
// Session ID fields (various naming conventions)
sessionID: z.string().optional(),
sessionId: z.string().optional(),
session_id: z.string().optional(),
});
```
How can I resolve this? If you propose a fix, please make it concise.
| log("error", component, ...args); | ||
| } | ||
|
|
||
| /** | ||
| * Log debug message (only in verbose mode) | ||
| */ |
There was a problem hiding this comment.
setDebug(false) fails to restore verbose mode
After calling setDebug(true) (which sets loggerState.verboseMode = true), calling setDebug(false) does not reset verboseMode back to false. The if (debug) branch is skipped, so loggerState.verboseMode remains true even after debug mode is disabled. Any component that reads getVerboseMode() or loggerState.verboseMode after setDebug(false) will still see verboseMode === true, causing debug logs to continue appearing silently.
Trace:
setDebug(true)→loggerState.debugMode = true,loggerState.verboseMode = truesetDebug(false)→loggerState.debugMode = false,loggerState.verboseModestaystrue
The fix requires resetting verboseMode when debug is disabled (assuming verbose wasn't independently set):
export function setDebug(debug: boolean): void {
loggerState.debugMode = debug;
if (debug) {
loggerState.verboseMode = true;
} else if (!loggerState._explicitVerbose) {
// Only reset verboseMode if it wasn't explicitly set via setVerbose
loggerState.verboseMode = false;
}
verboseMode = loggerState.verboseMode;
}A simpler approach is to track the pre-debug verbose state and restore it on setDebug(false).
Prompt To Fix With AI
This is a comment left during a code review.
Path: cli/src/ui/logger.ts
Line: 242-247
Comment:
**`setDebug(false)` fails to restore verbose mode**
After calling `setDebug(true)` (which sets `loggerState.verboseMode = true`), calling `setDebug(false)` does not reset `verboseMode` back to `false`. The `if (debug)` branch is skipped, so `loggerState.verboseMode` remains `true` even after debug mode is disabled. Any component that reads `getVerboseMode()` or `loggerState.verboseMode` after `setDebug(false)` will still see `verboseMode === true`, causing debug logs to continue appearing silently.
Trace:
1. `setDebug(true)` → `loggerState.debugMode = true`, `loggerState.verboseMode = true`
2. `setDebug(false)` → `loggerState.debugMode = false`, `loggerState.verboseMode` stays `true`
The fix requires resetting `verboseMode` when debug is disabled (assuming verbose wasn't independently set):
```typescript
export function setDebug(debug: boolean): void {
loggerState.debugMode = debug;
if (debug) {
loggerState.verboseMode = true;
} else if (!loggerState._explicitVerbose) {
// Only reset verboseMode if it wasn't explicitly set via setVerbose
loggerState.verboseMode = false;
}
verboseMode = loggerState.verboseMode;
}
```
A simpler approach is to track the pre-debug verbose state and restore it on `setDebug(false)`.
How can I resolve this? If you propose a fix, please make it concise.|
|
||
| await Promise.allSettled(promises); | ||
| cleanupRegistry.clear(); | ||
| isCleaningUp = false; | ||
| } | ||
|
|
||
| let isShuttingDown = false; | ||
|
|
||
| /** | ||
| * Setup process signal handlers for cleanup | ||
| */ | ||
| export function setupSignalHandlers(): void { | ||
| const signals: NodeJS.Signals[] = ["SIGINT", "SIGTERM"]; | ||
|
|
||
| for (const signal of signals) { | ||
| process.on(signal, async () => { | ||
| // Prevent duplicate cleanup runs | ||
| if (isShuttingDown) { | ||
| process.stdout.write(`\nReceived ${signal}, cleanup already in progress...\n`); | ||
| return; | ||
| } | ||
| isShuttingDown = true; | ||
|
|
||
| // Use writeSync to avoid event loop issues during exit |
There was a problem hiding this comment.
setupSignalHandlers accumulates duplicate listeners on repeated calls
There is no guard preventing setupSignalHandlers from being called more than once. Each call unconditionally adds a new async handler to both SIGINT and SIGTERM. If a module import tree calls this at startup and again in a test harness (or if the module is re-evaluated), duplicate handlers accumulate. Node.js emits a MaxListenersExceededWarning after 10 listeners on a single event, and with async handlers the isShuttingDown guard only prevents subsequent handlers in the same signal delivery — it does not prevent multiple handlers from firing in response to different signals or calls.
Consider a one-time initialization guard:
let signalHandlersRegistered = false;
export function setupSignalHandlers(): void {
if (signalHandlersRegistered) return;
signalHandlersRegistered = true;
const signals: NodeJS.Signals[] = ["SIGINT", "SIGTERM"];
// ... existing handler registration
}Prompt To Fix With AI
This is a comment left during a code review.
Path: cli/src/utils/cleanup.ts
Line: 108-131
Comment:
**`setupSignalHandlers` accumulates duplicate listeners on repeated calls**
There is no guard preventing `setupSignalHandlers` from being called more than once. Each call unconditionally adds a new `async` handler to both `SIGINT` and `SIGTERM`. If a module import tree calls this at startup and again in a test harness (or if the module is re-evaluated), duplicate handlers accumulate. Node.js emits a `MaxListenersExceededWarning` after 10 listeners on a single event, and with `async` handlers the `isShuttingDown` guard only prevents *subsequent* handlers in the same signal delivery — it does not prevent multiple handlers from firing in response to different signals or calls.
Consider a one-time initialization guard:
```typescript
let signalHandlersRegistered = false;
export function setupSignalHandlers(): void {
if (signalHandlersRegistered) return;
signalHandlersRegistered = true;
const signals: NodeJS.Signals[] = ["SIGINT", "SIGTERM"];
// ... existing handler registration
}
```
How can I resolve this? If you propose a fix, please make it concise.
| export function createErrorWithContext(error: unknown, context: Record<string, unknown>): RalphyError { | ||
| const standardized = standardizeError(error); | ||
|
|
||
| if (standardized.context) { | ||
| return new RalphyError(standardized.message, standardized.code, { | ||
| ...standardized.context, | ||
| ...context, | ||
| }); | ||
| } | ||
|
|
||
| return new RalphyError(standardized.message, standardized.code, context); | ||
| } |
There was a problem hiding this comment.
createErrorWithContext silently loses error subclass
standardizeError returns a RalphyError subclass (e.g., TimeoutError) unchanged. However, createErrorWithContext then wraps it in new RalphyError(...), losing the original subclass type. Any caller that subsequently does error instanceof TimeoutError will get false, even though error.code === "TIMEOUT_ERROR".
To preserve the subclass, construct a new instance of the original error's constructor rather than always using RalphyError:
| export function createErrorWithContext(error: unknown, context: Record<string, unknown>): RalphyError { | |
| const standardized = standardizeError(error); | |
| if (standardized.context) { | |
| return new RalphyError(standardized.message, standardized.code, { | |
| ...standardized.context, | |
| ...context, | |
| }); | |
| } | |
| return new RalphyError(standardized.message, standardized.code, context); | |
| } | |
| export function createErrorWithContext(error: unknown, context: Record<string, unknown>): RalphyError { | |
| const standardized = standardizeError(error); | |
| const ErrorClass = (standardized.constructor as typeof RalphyError) || RalphyError; | |
| return new ErrorClass(standardized.message, standardized.code, { | |
| ...(standardized.context ?? {}), | |
| ...context, | |
| }); | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: cli/src/utils/errors.ts
Line: 129-140
Comment:
`createErrorWithContext` silently loses error subclass
`standardizeError` returns a `RalphyError` subclass (e.g., `TimeoutError`) unchanged. However, `createErrorWithContext` then wraps it in `new RalphyError(...)`, losing the original subclass type. Any caller that subsequently does `error instanceof TimeoutError` will get `false`, even though `error.code === "TIMEOUT_ERROR"`.
To preserve the subclass, construct a new instance of the original error's constructor rather than always using `RalphyError`:
```suggestion
export function createErrorWithContext(error: unknown, context: Record<string, unknown>): RalphyError {
const standardized = standardizeError(error);
const ErrorClass = (standardized.constructor as typeof RalphyError) || RalphyError;
return new ErrorClass(standardized.message, standardized.code, {
...(standardized.context ?? {}),
...context,
});
}
```
How can I resolve this? If you propose a fix, please make it concise.| export function formatParsedStep(step: ParsedStep, agentNum?: number): string | null { | ||
| const prefix = agentNum !== undefined ? `Agent ${agentNum}: ` : ""; | ||
|
|
||
| // Prioritize concrete actions over generic thoughts | ||
| if (step.writing) { | ||
| return `${prefix}${step.writing} `; | ||
| } | ||
|
|
||
| if (step.reading) { | ||
| return `${prefix}${step.reading} `; | ||
| } | ||
|
|
||
| if (step.running) { | ||
| return `${prefix}${step.running} `; | ||
| } | ||
|
|
||
| if (step.thought) { | ||
| // Clean up common "Thinking:" prefixes since we wrap in {} later | ||
| const cleanedThought = step.thought.replace(/^(Thinking|Analyzing|Considering|Warning|Waiting)[:\s]*/i, "").trim(); | ||
| return `${prefix}${cleanedThought} `; | ||
| } | ||
|
|
||
| if (step.executed) { | ||
| return `${prefix} Done: ${step.executed.substring(0, 100)} `; | ||
| } | ||
|
|
||
| if (step.tool) { | ||
| return `${prefix} Tool: ${step.tool} `; | ||
| } | ||
|
|
||
| // If raw but couldn't parse, truncate and show | ||
| if (step.raw && step.raw.length > 0) { | ||
| const trimmed = step.raw.trim(); | ||
| if (trimmed.startsWith("{") || trimmed.startsWith('"')) { | ||
| // It's JSON we couldn't parse, skip it | ||
| return null; | ||
| } | ||
| return `${prefix}${trimmed.substring(0, 100)} `; | ||
| } | ||
|
|
||
| return null; | ||
| } |
There was a problem hiding this comment.
formatParsedStep truncates field values inconsistently
step.raw and step.executed are capped at 100 characters, but step.reading, step.writing, step.running, and step.thought are returned unbounded. For complex AI output (e.g., JSON tool inputs), this can produce extremely long display strings in log output.
Apply consistent truncation across all fields:
| export function formatParsedStep(step: ParsedStep, agentNum?: number): string | null { | |
| const prefix = agentNum !== undefined ? `Agent ${agentNum}: ` : ""; | |
| // Prioritize concrete actions over generic thoughts | |
| if (step.writing) { | |
| return `${prefix}${step.writing} `; | |
| } | |
| if (step.reading) { | |
| return `${prefix}${step.reading} `; | |
| } | |
| if (step.running) { | |
| return `${prefix}${step.running} `; | |
| } | |
| if (step.thought) { | |
| // Clean up common "Thinking:" prefixes since we wrap in {} later | |
| const cleanedThought = step.thought.replace(/^(Thinking|Analyzing|Considering|Warning|Waiting)[:\s]*/i, "").trim(); | |
| return `${prefix}${cleanedThought} `; | |
| } | |
| if (step.executed) { | |
| return `${prefix} Done: ${step.executed.substring(0, 100)} `; | |
| } | |
| if (step.tool) { | |
| return `${prefix} Tool: ${step.tool} `; | |
| } | |
| // If raw but couldn't parse, truncate and show | |
| if (step.raw && step.raw.length > 0) { | |
| const trimmed = step.raw.trim(); | |
| if (trimmed.startsWith("{") || trimmed.startsWith('"')) { | |
| // It's JSON we couldn't parse, skip it | |
| return null; | |
| } | |
| return `${prefix}${trimmed.substring(0, 100)} `; | |
| } | |
| return null; | |
| } | |
| export function formatParsedStep(step: ParsedStep, agentNum?: number): string | null { | |
| const prefix = agentNum !== undefined ? `Agent ${agentNum}: ` : ""; | |
| const MAX_DISPLAY_LENGTH = 100; | |
| // Prioritize concrete actions over generic thoughts | |
| if (step.writing) { | |
| return `${prefix}${step.writing.substring(0, MAX_DISPLAY_LENGTH)} `; | |
| } | |
| if (step.reading) { | |
| return `${prefix}${step.reading.substring(0, MAX_DISPLAY_LENGTH)} `; | |
| } | |
| if (step.running) { | |
| return `${prefix}${step.running.substring(0, MAX_DISPLAY_LENGTH)} `; | |
| } | |
| if (step.thought) { | |
| const cleanedThought = step.thought.replace(/^(Thinking|Analyzing|Considering|Warning|Waiting)[:\s]*/i, "").trim(); | |
| return `${prefix}${cleanedThought.substring(0, MAX_DISPLAY_LENGTH)} `; | |
| } | |
| if (step.executed) { | |
| return `${prefix} Done: ${step.executed.substring(0, MAX_DISPLAY_LENGTH)} `; | |
| } | |
| if (step.tool) { | |
| return `${prefix} Tool: ${step.tool} `; | |
| } | |
| // If raw but couldn't parse, truncate and show | |
| if (step.raw && step.raw.length > 0) { | |
| const trimmed = step.raw.trim(); | |
| if (trimmed.startsWith("{") || trimmed.startsWith('"')) { | |
| // It's JSON we couldn't parse, skip it | |
| return null; | |
| } | |
| return `${prefix}${trimmed.substring(0, MAX_DISPLAY_LENGTH)} `; | |
| } | |
| return null; | |
| } |
Additionally, at line 86, rename the local variable from shortArgs to argSummary to match the field it's assigned from and eliminate the misleading implication that the argument has been shortened.
Prompt To Fix With AI
This is a comment left during a code review.
Path: cli/src/utils/ai-output-parser.ts
Line: 114-155
Comment:
`formatParsedStep` truncates field values inconsistently
`step.raw` and `step.executed` are capped at 100 characters, but `step.reading`, `step.writing`, `step.running`, and `step.thought` are returned unbounded. For complex AI output (e.g., JSON tool inputs), this can produce extremely long display strings in log output.
Apply consistent truncation across all fields:
```suggestion
export function formatParsedStep(step: ParsedStep, agentNum?: number): string | null {
const prefix = agentNum !== undefined ? `Agent ${agentNum}: ` : "";
const MAX_DISPLAY_LENGTH = 100;
// Prioritize concrete actions over generic thoughts
if (step.writing) {
return `${prefix}${step.writing.substring(0, MAX_DISPLAY_LENGTH)} `;
}
if (step.reading) {
return `${prefix}${step.reading.substring(0, MAX_DISPLAY_LENGTH)} `;
}
if (step.running) {
return `${prefix}${step.running.substring(0, MAX_DISPLAY_LENGTH)} `;
}
if (step.thought) {
const cleanedThought = step.thought.replace(/^(Thinking|Analyzing|Considering|Warning|Waiting)[:\s]*/i, "").trim();
return `${prefix}${cleanedThought.substring(0, MAX_DISPLAY_LENGTH)} `;
}
if (step.executed) {
return `${prefix} Done: ${step.executed.substring(0, MAX_DISPLAY_LENGTH)} `;
}
if (step.tool) {
return `${prefix} Tool: ${step.tool} `;
}
// If raw but couldn't parse, truncate and show
if (step.raw && step.raw.length > 0) {
const trimmed = step.raw.trim();
if (trimmed.startsWith("{") || trimmed.startsWith('"')) {
// It's JSON we couldn't parse, skip it
return null;
}
return `${prefix}${trimmed.substring(0, MAX_DISPLAY_LENGTH)} `;
}
return null;
}
```
Additionally, at line 86, rename the local variable from `shortArgs` to `argSummary` to match the field it's assigned from and eliminate the misleading implication that the argument has been shortened.
How can I resolve this? If you propose a fix, please make it concise.| isEscaped = true; | ||
| continue; | ||
| } | ||
| if (char === '"' && !isEscaped) { |
There was a problem hiding this comment.
Dead condition !isEscaped in quote-detection branch
At this point in the loop, isEscaped is guaranteed to be false. The guard at lines 124–127 handles the true case with continue, so only false can reach line 132. The condition can be removed:
| if (char === '"' && !isEscaped) { | |
| if (char === '"') { | |
| inString = !inString; | |
| continue; | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: cli/src/utils/json-validation.ts
Line: 132
Comment:
Dead condition `!isEscaped` in quote-detection branch
At this point in the loop, `isEscaped` is guaranteed to be `false`. The guard at lines 124–127 handles the true case with `continue`, so only `false` can reach line 132. The condition can be removed:
```suggestion
if (char === '"') {
inString = !inString;
continue;
}
```
How can I resolve this? If you propose a fix, please make it concise.| export const DEFAULT_IGNORE_PATTERNS = [ | ||
| ".git", | ||
| "node_modules", | ||
| ".ralphy-sandboxes", | ||
| ".ralphy-worktrees", | ||
| ".ralphy", | ||
| "agent-*", | ||
| "sandbox-*", | ||
| ]; |
There was a problem hiding this comment.
DEFAULT_IGNORE_PATTERNS missing common build-output directories
The list covers version-control and Ralphy-internal directories but omits the most common build artifact directories for TypeScript/JS, Rust, and Python ecosystems. In a mid-size monorepo, these can contain hundreds of thousands of files, making an unfiltered index extremely slow.
Add at minimum:
| export const DEFAULT_IGNORE_PATTERNS = [ | |
| ".git", | |
| "node_modules", | |
| ".ralphy-sandboxes", | |
| ".ralphy-worktrees", | |
| ".ralphy", | |
| "agent-*", | |
| "sandbox-*", | |
| ]; | |
| export const DEFAULT_IGNORE_PATTERNS = [ | |
| ".git", | |
| "node_modules", | |
| ".ralphy-sandboxes", | |
| ".ralphy-worktrees", | |
| ".ralphy", | |
| "agent-*", | |
| "sandbox-*", | |
| // Build artifacts | |
| "dist", | |
| "build", | |
| "out", | |
| ".next", | |
| ".nuxt", | |
| ".svelte-kit", | |
| "target", // Rust/Maven | |
| "__pycache__", | |
| ".cache", | |
| "coverage", | |
| ".turbo", | |
| ]; |
Note: these patterns are currently anchored to workspace root by globToRegex. Consider also updating globToRegex to support **/ prefixes for matching nested dependencies (e.g., packages/app/node_modules).
Prompt To Fix With AI
This is a comment left during a code review.
Path: cli/src/config/constants.ts
Line: 52-60
Comment:
`DEFAULT_IGNORE_PATTERNS` missing common build-output directories
The list covers version-control and Ralphy-internal directories but omits the most common build artifact directories for TypeScript/JS, Rust, and Python ecosystems. In a mid-size monorepo, these can contain hundreds of thousands of files, making an unfiltered index extremely slow.
Add at minimum:
```suggestion
export const DEFAULT_IGNORE_PATTERNS = [
".git",
"node_modules",
".ralphy-sandboxes",
".ralphy-worktrees",
".ralphy",
"agent-*",
"sandbox-*",
// Build artifacts
"dist",
"build",
"out",
".next",
".nuxt",
".svelte-kit",
"target", // Rust/Maven
"__pycache__",
".cache",
"coverage",
".turbo",
];
```
Note: these patterns are currently anchored to workspace root by `globToRegex`. Consider also updating `globToRegex` to support `**/` prefixes for matching nested dependencies (e.g., `packages/app/node_modules`).
How can I resolve this? If you propose a fix, please make it concise.| interface WebhookSessionData { | ||
| sessionId: string; | ||
| engine: string; | ||
| mode: string; | ||
| cliVersion: string; | ||
| platform: string; | ||
| totalTokensIn: number; | ||
| totalTokensOut: number; | ||
| totalDurationMs: number; | ||
| taskCount: number; | ||
| successCount: number; | ||
| failedCount: number; | ||
| toolCalls: { | ||
| toolName: string; | ||
| callCount: number; | ||
| successCount: number; | ||
| failedCount: number; | ||
| avgDurationMs: number; | ||
| }[]; | ||
| tags?: string[]; | ||
| } | ||
|
|
||
| /** | ||
| * Full session details for webhook (full privacy mode) | ||
| */ | ||
| interface WebhookSessionDetails { | ||
| prompt?: string; | ||
| response?: string; | ||
| filePaths?: string[]; | ||
| } |
There was a problem hiding this comment.
WebhookSessionData and WebhookSessionDetails not exported
These two interfaces form the public contract of TelemetryWebhookPayload (as field types session and details) but are not exported. Consumers that need to work with these fields (e.g., to type-check a webhook handler) cannot declare typed local variables without repeating the shape inline.
Export them explicitly:
| interface WebhookSessionData { | |
| sessionId: string; | |
| engine: string; | |
| mode: string; | |
| cliVersion: string; | |
| platform: string; | |
| totalTokensIn: number; | |
| totalTokensOut: number; | |
| totalDurationMs: number; | |
| taskCount: number; | |
| successCount: number; | |
| failedCount: number; | |
| toolCalls: { | |
| toolName: string; | |
| callCount: number; | |
| successCount: number; | |
| failedCount: number; | |
| avgDurationMs: number; | |
| }[]; | |
| tags?: string[]; | |
| } | |
| /** | |
| * Full session details for webhook (full privacy mode) | |
| */ | |
| interface WebhookSessionDetails { | |
| prompt?: string; | |
| response?: string; | |
| filePaths?: string[]; | |
| } | |
| export interface WebhookSessionData { | |
| sessionId: string; | |
| engine: string; | |
| mode: string; | |
| cliVersion: string; | |
| platform: string; | |
| totalTokensIn: number; | |
| totalTokensOut: number; | |
| totalDurationMs: number; | |
| taskCount: number; | |
| successCount: number; | |
| failedCount: number; | |
| toolCalls: { | |
| toolName: string; | |
| callCount: number; | |
| successCount: number; | |
| failedCount: number; | |
| avgDurationMs: number; | |
| }[]; | |
| tags?: string[]; | |
| } | |
| /** | |
| * Full session details for webhook (full privacy mode) | |
| */ | |
| export interface WebhookSessionDetails { | |
| prompt?: string; | |
| response?: string; | |
| filePaths?: string[]; | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: cli/src/telemetry/types.ts
Line: 84-113
Comment:
`WebhookSessionData` and `WebhookSessionDetails` not exported
These two interfaces form the public contract of `TelemetryWebhookPayload` (as field types `session` and `details`) but are not exported. Consumers that need to work with these fields (e.g., to type-check a webhook handler) cannot declare typed local variables without repeating the shape inline.
Export them explicitly:
```suggestion
export interface WebhookSessionData {
sessionId: string;
engine: string;
mode: string;
cliVersion: string;
platform: string;
totalTokensIn: number;
totalTokensOut: number;
totalDurationMs: number;
taskCount: number;
successCount: number;
failedCount: number;
toolCalls: {
toolName: string;
callCount: number;
successCount: number;
failedCount: number;
avgDurationMs: number;
}[];
tags?: string[];
}
/**
* Full session details for webhook (full privacy mode)
*/
export interface WebhookSessionDetails {
prompt?: string;
response?: string;
filePaths?: string[];
}
```
How can I resolve this? If you propose a fix, please make it concise.| function isCommonWord(word: string): boolean { | ||
| const commonWords = new Set([ | ||
| "the", | ||
| "and", | ||
| "for", | ||
| "are", | ||
| "but", | ||
| "not", | ||
| "you", | ||
| "all", | ||
| "can", | ||
| "had", | ||
| "her", | ||
| "was", | ||
| "one", | ||
| "our", | ||
| "out", | ||
| "day", | ||
| "get", | ||
| "has", | ||
| "him", | ||
| "his", | ||
| "how", | ||
| "its", | ||
| "may", | ||
| "new", | ||
| "now", | ||
| "old", | ||
| "see", | ||
| "two", | ||
| "who", | ||
| "boy", | ||
| "did", | ||
| "she", | ||
| "use", | ||
| "way", | ||
| "many", | ||
| "oil", | ||
| "sit", | ||
| "set", | ||
| "run", | ||
| "eat", | ||
| "far", | ||
| "sea", | ||
| "eye", | ||
| "ago", | ||
| "off", | ||
| "too", | ||
| "any", | ||
| "say", | ||
| "man", | ||
| "try", | ||
| "ask", | ||
| "end", | ||
| "why", | ||
| "let", | ||
| "put", | ||
| "own", | ||
| "tell", | ||
| "very", | ||
| "when", | ||
| "come", | ||
| "here", | ||
| "just", | ||
| "like", | ||
| "long", | ||
| "make", | ||
| "over", | ||
| "such", | ||
| "take", | ||
| "than", | ||
| "them", | ||
| "well", | ||
| "were", | ||
| "will", | ||
| "with", | ||
| "have", | ||
| "from", | ||
| "they", | ||
| "know", | ||
| "want", | ||
| "been", | ||
| "good", | ||
| "much", | ||
| "some", | ||
| "time", | ||
| "this", | ||
| "that", | ||
| "would", | ||
| "there", | ||
| "their", | ||
| "what", | ||
| "said", | ||
| "each", | ||
| "which", | ||
| "about", | ||
| "could", | ||
| "other", | ||
| "after", | ||
| "first", | ||
| "never", | ||
| "these", | ||
| "think", | ||
| "where", | ||
| "being", | ||
| "every", | ||
| "great", | ||
| "might", | ||
| "shall", | ||
| "still", | ||
| "those", | ||
| "while", | ||
| "true", | ||
| "false", | ||
| "null", | ||
| "undefined", | ||
| "return", | ||
| "import", | ||
| "export", | ||
| "default", | ||
| "async", | ||
| "await", | ||
| "yield", | ||
| "throw", | ||
| "catch", | ||
| "finally", | ||
| "break", | ||
| "continue", | ||
| "switch", | ||
| "case", | ||
| "try", | ||
| "new", | ||
| ]); | ||
| return commonWords.has(word.toLowerCase()); |
There was a problem hiding this comment.
Duplicate entries in isCommonWord Set
The Set literal contains duplicate entries. Set deduplicates on construction so there's no runtime impact, but the duplicates are misleading:
"try"appears at lines 378 and 457"new"appears at line 352 and 458
Remove the duplicates to keep the list clean and maintainable.
Prompt To Fix With AI
This is a comment left during a code review.
Path: cli/src/utils/file-indexer.ts
Line: 327-460
Comment:
Duplicate entries in `isCommonWord` Set
The Set literal contains duplicate entries. `Set` deduplicates on construction so there's no runtime impact, but the duplicates are misleading:
- `"try"` appears at lines 378 and 457
- `"new"` appears at line 352 and 458
Remove the duplicates to keep the list clean and maintainable.
How can I resolve this? If you propose a fix, please make it concise.1 participant
PR0: foundation contracts
Summary
This is the stack foundation. It fixes baseline type/contract drift and introduces shared utility primitives used by the rest of the chain.
Why this PR exists
mainhad contract mismatches (especially config/telemetry surfaces).What it adds
cli/src/config/types.ts.cli/src/telemetry/types.ts.cli/src/ui/logger.ts(live verbose state behavior).cli/src/utils/:errors.ts(standardized error normalization)cleanup.ts(shared cleanup routines)json-validation.ts(safe parsing/validation helpers)file-indexer.ts(task/file relevance indexing support)sanitization.ts(path/text sanitization helpers)ai-output-parser.ts(AI output parsing utility)cli/src/config/constants.ts.Impact on later PRs
Validation
bun run check,bun tsc --noEmit, andbun testwhen merged in order.Note
Add shared contracts and core utility baseline by introducing centralized CLI constants, structured logging sinks, and a file indexing system with JSON cache under
.ralphyAdd a constants module for runtime limits and paths, implement structured logging with pluggable sinks and JSON-lines persistence, and add a workspace file indexer with hashing and cached queries; extend config and types for telemetry webhook and
RuntimeOptions.debugOpenCode; note thatparseJsonLineincli/src/utils/json-validation.tscontains syntax errors that break the build.📍Where to Start
Start with structured logging initialization in
initializeStructuredLoggingin logger.ts, then review the constants in constants.ts and the file indexer flow in file-indexer.ts; address the compile error inparseJsonLinein json-validation.ts.Macroscope summarized 2dc0883.