stack1:Tfeat: harden config runtime loading and utility flow#160
stack1:Tfeat: harden config runtime loading and utility flow#160VX1D wants to merge 8 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 1 document(s) may need updating based on files changed in this PR: Goshen Labs's Space Agent Skills InjectionView Suggested Changes@@ -1,18 +1,14 @@
-Ralphy automatically detects skill/playbook directories within a repository to provide AI agents with explicit guidance about available skills. Supported directories include `.agents/skills`, `.cursor/rules`, `.opencode/skills`, `.claude/skills`, `.github/skills`, and `.skills`. When any of these directories are present, Ralphy augments the generated prompt with an "Agent Skills" section, improving context and agent behavior.
+Ralphy automatically detects skill/playbook directories within a repository to provide AI agents with explicit guidance about available skills. Supported directories include `.opencode/skills` and `.claude/skills`. When any of these directories are present, Ralphy augments the generated prompt with an "Agent Skills" section, improving context and agent behavior.
**Detection Mechanism**
-Ralphy checks for the existence of six well-known skill directory patterns in the repository's working directory. This is accomplished using a file system existence check. The detection logic is as follows:
+Ralphy checks for the existence of two well-known skill directory patterns in the repository's working directory. This is accomplished using a file system existence check. The detection logic is as follows:
```typescript
function detectAgentSkills(workDir: string): string[] {
const candidates = [
- join(workDir, ".agents", "skills"),
- join(workDir, ".cursor", "rules"),
join(workDir, ".opencode", "skills"),
join(workDir, ".claude", "skills"),
- join(workDir, ".github", "skills"),
- join(workDir, ".skills"),
];
return candidates.filter((p) => existsSync(p));
}
@@ -26,12 +22,8 @@
```
## Agent Skills
This repo includes skill/playbook docs that describe preferred patterns, workflows, or tooling:
-- .agents/skills
-- .cursor/rules
- .opencode/skills
- .claude/skills
-- .github/skills
-- .skills
Before you start coding:
- Read and follow any relevant skill docs from the paths above.
@@ -47,4 +39,4 @@
**Impact on Prompt Context and Agent Behavior**
-By surfacing the existence and location of skill/playbook documentation directly in the prompt—including `.github/skills`—Ralphy provides agents with immediate, actionable context about preferred patterns, workflows, and tooling for the repository. This explicit guidance helps agents align their behavior with project-specific conventions and best practices, leading to more relevant, maintainable, and consistent output. For agents like OpenCode, the lazy-loading mechanism further ensures that skills are loaded efficiently and only as needed, reducing overhead and improving responsiveness.
+By surfacing the existence and location of skill/playbook documentation directly in the prompt, Ralphy provides agents with immediate, actionable context about preferred patterns, workflows, and tooling for the repository. This explicit guidance helps agents align their behavior with project-specific conventions and best practices, leading to more relevant, maintainable, and consistent output. For agents like OpenCode, the lazy-loading mechanism further ensures that skills are loaded efficiently and only as needed, reducing overhead and improving responsiveness.Note: You must be authenticated to accept/decline updates. How did I do? Any feedback? |
Greptile SummaryThis PR hardens the config runtime pipeline and adds comprehensive utility infrastructure (structured logging, secret sanitization, resource management, hooks, metrics, templates, and a transform layer). The security intent is solid — prototype-pollution detection, command injection validation, telemetry secret scrubbing, and PRD path traversal prevention are all meaningful improvements. Key changes:
Two actionable issues remain:
Confidence Score: 3/5
Last reviewed commit: 1f2b42b |
Additional Comments (5)
The This is a significant regression: users with valid commands configured in their Alternatively, the function should only validate the binary/executable name (before the first space) against the strict character pattern, and then validate arguments separately via
Each call to
The async write-batching queue has been removed in favour of synchronous |
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
| function debugLog(...args: unknown[]): void { | ||
| if (DEBUG || (globalThis as { verboseMode?: boolean }).verboseMode === true) { | ||
| logDebug(args.map((a) => String(a)).join(" ")); | ||
| } | ||
| } |
There was a problem hiding this comment.
🟡 Medium engines/validation.ts:15
debugLog checks DEBUG || verboseMode to decide whether to log, but then calls logDebug, which silently returns when verboseMode is false. Setting RALPHY_DEBUG=true therefore produces no output unless verboseMode is also enabled, bypassing the intended environment-variable override. Consider using console.log directly (or a separate unchecked logger) so DEBUG alone enables logging.
-function debugLog(...args: unknown[]): void {
- if (DEBUG || (globalThis as { verboseMode?: boolean }).verboseMode === true) {
- logDebug(args.map((a) => String(a)).join(" "));
- }
-}🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file cli/src/engines/validation.ts around lines 15-19:
`debugLog` checks `DEBUG || verboseMode` to decide whether to log, but then calls `logDebug`, which silently returns when `verboseMode` is false. Setting `RALPHY_DEBUG=true` therefore produces no output unless `verboseMode` is also enabled, bypassing the intended environment-variable override. Consider using `console.log` directly (or a separate unchecked logger) so `DEBUG` alone enables logging.
Evidence trail:
cli/src/engines/validation.ts lines 15-19: `debugLog` function checks `DEBUG || globalThis.verboseMode` then calls `logDebug()`.
cli/src/ui/logger.ts lines 3, 8-10, 42-46: `logDebug` has its own `verboseMode` check (module-level variable set by `setVerbose()`).
cli/src/cli/commands/run.ts line 32: `setVerbose(options.verbose)` - only sets verboseMode when --verbose flag is passed.
cli/src/cli/commands/task.ts line 23: same pattern.
The env var `RALPHY_DEBUG=true` makes `DEBUG` true, passes `debugLog`'s check, but `logDebug` silently returns because logger.ts's internal `verboseMode` remains false.
| this.metrics.clear(); | ||
| this.histogramBuckets.clear(); | ||
| } | ||
|
|
||
| private getMetricKey(name: string, labels?: MetricLabels): string { | ||
| if (!labels || Object.keys(labels).length === 0) { | ||
| return name; | ||
| } | ||
| const labelStr = Object.entries(labels) | ||
| .sort(([a], [b]) => a.localeCompare(b)) |
There was a problem hiding this comment.
🟢 Low utils/metrics.ts:269
getMetricKey builds keys without escaping , or =, so different label sets can collide. Suggest using an unambiguous encoding (e.g., stable-key-order JSON or URL-encoding) when serializing labels.
- private getMetricKey(name: string, labels?: MetricLabels): string {
- if (!labels || Object.keys(labels).length === 0) {
- return name;
- }
- const labelStr = Object.entries(labels)
- .sort(([a], [b]) => a.localeCompare(b))
- .map(([k, v]) => `${k}=${v}`)
- .join(",");
- return `${name}{${labelStr}}`;
- }
+ private getMetricKey(name: string, labels?: MetricLabels): string {
+ if (!labels || Object.keys(labels).length === 0) {
+ return name;
+ }
+ const labelStr = Object.entries(labels)
+ .sort(([a], [b]) => a.localeCompare(b))
+ .map(([k, v]) => `${k}=${JSON.stringify(v)}`)
+ .join(",");
+ return `${name}{${labelStr}}`;
+ }🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file cli/src/utils/metrics.ts around lines 269-278:
`getMetricKey` builds keys without escaping `,` or `=`, so different label sets can collide. Suggest using an unambiguous encoding (e.g., stable-key-order JSON or URL-encoding) when serializing labels.
Evidence trail:
cli/src/utils/metrics.ts lines 269-278 (getMetricKey function), lines 26-28 (MetricLabels interface). The function uses `.map(([k, v]) => `${k}=${v}`).join(",")` without escaping, allowing collisions when label values contain `,` or `=` characters.
| function escapeYaml(value: string | undefined | null): string { | ||
| return (value || "").replace(/"/g, '\\"'); | ||
| if (!value) return ""; | ||
| // Use YAML library for proper escaping instead of simple quote replacement | ||
| // This prevents YAML injection attacks | ||
| const serialized = YAML.stringify(value).trim(); | ||
| // Remove surrounding quotes added by YAML.stringify for simple strings | ||
| return serialized.replace(/^"|"$/g, ""); | ||
| } |
There was a problem hiding this comment.
🟠 High config/writer.ts:53
escapeYaml now uses YAML.stringify, which can return single-quoted strings (e.g., 'foo"bar') or block scalars. createConfigContent wraps this output in double quotes, so a command like echo "hello" becomes name: "'echo \"hello\"'" — a string literal containing single quotes that corrupts the value when parsed. For multi-line strings, wrapping a block scalar in quotes produces invalid YAML.
function escapeYaml(value: string | undefined | null): string {
if (!value) return "";
- // Use YAML library for proper escaping instead of simple quote replacement
- // This prevents YAML injection attacks
- const serialized = YAML.stringify(value).trim();
- // Remove surrounding quotes added by YAML.stringify for simple strings
- return serialized.replace(/^"|"$/g, "");
+ // Escape backslashes and double quotes, then wrap in double quotes
+ const escaped = value.replace(/\\/g, "\\\\").replace(/"/g, "\\\"");
+ return `"${escaped}"`;
}🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file cli/src/config/writer.ts around lines 53-60:
`escapeYaml` now uses `YAML.stringify`, which can return single-quoted strings (e.g., `'foo"bar'`) or block scalars. `createConfigContent` wraps this output in double quotes, so a command like `echo "hello"` becomes `name: "'echo \"hello\"'"` — a string literal containing single quotes that corrupts the value when parsed. For multi-line strings, wrapping a block scalar in quotes produces invalid YAML.
Evidence trail:
cli/src/config/writer.ts lines 50-57: `escapeYaml` function uses `YAML.stringify(value).trim()` and strips only double quotes with `replace(/^"|"/g, "")`. Single quotes and block scalars are not handled.
cli/src/config/writer.ts lines 15-21: `createConfigContent` wraps `escapeYaml` output in double quotes, e.g., `name: "${escapeYaml(detected.name)}"` and `test: "${escapeYaml(detected.testCmd)}"`.
When `YAML.stringify` returns single-quoted strings (for inputs containing double quotes) or block scalars (for multi-line strings), the wrapping in double quotes produces invalid YAML.
| export function loadLintCommand(workDir = process.cwd()): string { | ||
| const config = loadConfig(workDir); | ||
| return config?.commands.lint ?? ""; | ||
| const command = config?.commands.lint ?? ""; | ||
|
|
||
| if (command && !validateCommand(command)) { | ||
| logWarn(`Invalid lint command in config: "${command}". Falling back to default.`); | ||
| return ""; | ||
| } | ||
|
|
||
| return command; |
There was a problem hiding this comment.
🟡 Medium config/loader.ts:149
Suggestion: validateCommand forbids spaces, so multi-word commands in loadTestCommand, loadLintCommand, and loadBuildCommand are treated as invalid and reset to empty strings. Consider splitting the config into command and args and validate only the executable, or relax the validation to allow spaces/common arg syntax to avoid rejecting valid commands.
- if (command && !validateCommand(command)) {
+ if (command && !validateCommand(command.split(' ')[0])) {
logWarn(`Invalid lint command in config: "${command}". Falling back to default.`);
return "";
}🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file cli/src/config/loader.ts around lines 149-158:
Suggestion: `validateCommand` forbids spaces, so multi-word commands in `loadTestCommand`, `loadLintCommand`, and `loadBuildCommand` are treated as invalid and reset to empty strings. Consider splitting the config into `command` and `args` and validate only the executable, or relax the validation to allow spaces/common arg syntax to avoid rejecting valid commands.
Evidence trail:
cli/src/engines/validation.ts lines 50-56: validateCommand uses regex `/^[a-zA-Z0-9._\-/]+$/` which does not include space character. cli/src/config/loader.ts lines 127-139 (loadTestCommand), 149-161 (loadLintCommand), 166-178 (loadBuildCommand): all call validateCommand and return empty string "" when validation fails.
|
|
||
| counter(name: string, value = 1, labels?: MetricLabels): void { | ||
| const key = this.getMetricKey(name, labels); | ||
| const existing = this.metrics.get(key); | ||
|
|
||
| if (existing && existing.type === "counter") { | ||
| existing.value = (existing.value as number) + value; | ||
| } else { | ||
| this.metrics.set(key, { | ||
| name, | ||
| type: "counter", | ||
| description: `Counter metric: ${name}`, | ||
| labels, | ||
| value, | ||
| timestamp: Date.now(), | ||
| }); |
There was a problem hiding this comment.
🟢 Low utils/metrics.ts:185
When incrementing an existing counter, the code mutates existing.value in-place (line 190) but leaves timestamp untouched. This causes the counter to report its creation time as the timestamp rather than the time of the last increment, unlike gauge and histogram which refresh the timestamp on every call.
- if (existing && existing.type === "counter") {
- existing.value = (existing.value as number) + value;
- } else {🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file cli/src/utils/metrics.ts around lines 185-200:
When incrementing an existing counter, the code mutates `existing.value` in-place (line 190) but leaves `timestamp` untouched. This causes the counter to report its creation time as the timestamp rather than the time of the last increment, unlike `gauge` and `histogram` which refresh the timestamp on every call.
Evidence trail:
cli/src/utils/metrics.ts lines 185-258 at REVIEWED_COMMIT:
- Counter: lines 189-190 show only `existing.value` is mutated, no timestamp update
- Counter creation: line 197 sets `timestamp: Date.now()` only for new counters
- Gauge: line 213 sets `timestamp: Date.now()` on every call via `this.metrics.set()`
- Histogram: line 253 sets `timestamp: Date.now()` on every call via `this.metrics.set()`
| setActiveSpan(span: Span | null): void { | ||
| this.activeSpan = span; | ||
| } | ||
|
|
There was a problem hiding this comment.
🟢 Low utils/metrics.ts:336
SimpleTracer appends every span to this.spans at line 307, but the array is never cleared or bounded. Since SimpleTracer is the default global tracer, long-running CLI processes will accumulate unbounded memory and eventually crash with OOM. Consider either clearing this.spans when endSpan is called, exposing a flush/clear method, or implementing a fixed-size ring buffer.
getSpans(): Span[] {
return this.spans;
}
+
+ clear(): void {
+ this.spans = [];
+ }🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file cli/src/utils/metrics.ts around lines 336-339:
`SimpleTracer` appends every span to `this.spans` at line 307, but the array is never cleared or bounded. Since `SimpleTracer` is the default global tracer, long-running CLI processes will accumulate unbounded memory and eventually crash with OOM. Consider either clearing `this.spans` when `endSpan` is called, exposing a flush/clear method, or implementing a fixed-size ring buffer.
Evidence trail:
cli/src/utils/metrics.ts lines 285-337 (SimpleTracer class showing spans array initialization at 285, push at 307, endSpan at 311-314 which doesn't remove from array, getSpans at 336-337), cli/src/utils/metrics.ts line 353 (`let globalTracer: Tracer = new SimpleTracer();`), cli/src/utils/metrics.ts lines 131-146 (Tracer interface with no clear() method), cli/src/index.ts line 46 (`await runLoop(options)` for long-running PRD loop mode), cli/src/cli/commands/run.ts (runLoop implementation showing task processing loop)
| export const OpenCodeEventSchema = z.union([ | ||
| ToolUseEventSchema, | ||
| StepStartEventSchema, | ||
| StepFinishEventSchema, | ||
| TextEventSchema, | ||
| ErrorEventSchema, | ||
| ]); |
There was a problem hiding this comment.
🟢 Low utils/opencode-parser.ts:113
When a JSON object has type: "plan" or type: "thinking", the event is returned with isValid: false and dropped. detectEventType lacks cases for these types and returns "unknown", and OpenCodeEventSchema doesn't include schemas for them, so validation fails. Consider adding schemas for plan and thinking events to the union and updating detectEventType to handle them.
+export const PlanEventSchema = z.object({
+ type: z.literal("plan"),
+});
+
+export const ThinkingEventSchema = z.object({
+ type: z.literal("thinking"),
+});
+
export const OpenCodeEventSchema = z.union([
ToolUseEventSchema,
StepStartEventSchema,
StepFinishEventSchema,
TextEventSchema,
ErrorEventSchema,
+ PlanEventSchema,
+ ThinkingEventSchema,
]);🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file cli/src/utils/opencode-parser.ts around lines 113-119:
When a JSON object has `type: "plan"` or `type: "thinking"`, the event is returned with `isValid: false` and dropped. `detectEventType` lacks cases for these types and returns `"unknown"`, and `OpenCodeEventSchema` doesn't include schemas for them, so validation fails. Consider adding schemas for `plan` and `thinking` events to the union and updating `detectEventType` to handle them.
Evidence trail:
cli/src/utils/opencode-parser.ts line 113: `EventType` includes `"plan" | "thinking"` in the union type
cli/src/utils/opencode-parser.ts lines 202-223: `detectEventType()` switch statement has no cases for `plan` or `thinking`, returns `"unknown"` for them
cli/src/utils/opencode-parser.ts lines 95-101: `OpenCodeEventSchema` union has no schema for plan or thinking events
cli/src/utils/opencode-parser.ts lines 307-321: `parseOpenCodeLine()` returns `isValid: false` when eventType is "unknown" and Zod validation fails
cli/src/utils/opencode-parser.ts lines 351-353: `filterEvents()` drops events where `!parsed.isValid`
| private spans: Span[] = []; | ||
| private activeSpan: Span | null = null; | ||
| private idCounter = 0; | ||
|
|
There was a problem hiding this comment.
🟢 Low utils/metrics.ts:289
Span context propagation is fragile: startSpan doesn’t inherit the current span and activeSpan is a global mutable field, so concurrent withSpan calls corrupt parent/child links. Suggest using AsyncLocalStorage for per-async context and defaulting startSpan to the current span when parentContext is absent.
startSpan(name: string, parentContext?: SpanContext, attributes?: Record<string, unknown>): Span {
const spanId = this.generateId();
- const traceId = parentContext?.traceId || this.generateId();
+ const traceId = parentContext?.traceId || this.activeSpan?.context.traceId || this.generateId();
+ const parentSpanId = parentContext?.spanId || (traceId === this.activeSpan?.context.traceId ? this.activeSpan?.context.spanId : undefined);🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file cli/src/utils/metrics.ts around lines 289-292:
Span context propagation is fragile: `startSpan` doesn’t inherit the current span and `activeSpan` is a global mutable field, so concurrent `withSpan` calls corrupt parent/child links. Suggest using `AsyncLocalStorage` for per-async context and defaulting `startSpan` to the current span when `parentContext` is absent.
Evidence trail:
cli/src/utils/metrics.ts lines 286-304 (SimpleTracer class with activeSpan field and startSpan method that doesn't inherit from activeSpan), lines 353 (global tracer instance), lines 407-409 (startSpan convenience function), lines 421-441 (withSpan function with save/restore pattern for activeSpan that breaks under concurrent async execution).
| register<T extends HookContext>( | ||
| hookName: HookName, | ||
| handler: HookHandler<T>, | ||
| options?: { priority?: number; name?: string }, |
There was a problem hiding this comment.
🟢 Low utils/hooks.ts:119
Suggestion: avoid mutating the hooks array while executing. Replace the array on register (instead of push/sort) and iterate over a snapshot in execute so adds/removals during execution can’t reorder or skip handlers.
Also found in 1 other location(s)
cli/src/utils/transform.ts:111
The
executemethod iterates directly over the mutablethis.transformersarray using an asyncfor...ofloop. If thetransformersarray is modified (e.g., a transformer is unregistered) while the execution is paused at theawait entry.transformer(...)line, the loop iterator will desynchronize from the array contents. This causes the loop to skip the next transformer in the list when execution resumes, potentially bypassing critical transformations. The loop should iterate over a shallow copy (e.g.,[...this.transformers]) to ensure stability.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file cli/src/utils/hooks.ts around line 119:
Suggestion: avoid mutating the `hooks` array while executing. Replace the array on `register` (instead of `push`/`sort`) and iterate over a snapshot in `execute` so adds/removals during execution can’t reorder or skip handlers.
Evidence trail:
cli/src/utils/hooks.ts lines 106-128 (register method uses push/sort), lines 133-155 (execute method iterates directly over array reference without snapshot), lines 119-125 (unregister function uses splice on same array)
Also found in 1 other location(s):
- cli/src/utils/transform.ts:111 -- The `execute` method iterates directly over the mutable `this.transformers` array using an async `for...of` loop. If the `transformers` array is modified (e.g., a transformer is unregistered) while the execution is paused at the `await entry.transformer(...)` line, the loop iterator will desynchronize from the array contents. This causes the loop to skip the next transformer in the list when execution resumes, potentially bypassing critical transformations. The loop should iterate over a shallow copy (e.g., `[...this.transformers]`) to ensure stability.
| return () => { | ||
| const hooks = this.hooks.get(hookName) || []; | ||
| const index = hooks.findIndex((h) => h.name === entry.name); | ||
| if (index !== -1) { | ||
| hooks.splice(index, 1); | ||
| logDebugContext("Hooks", `Unregistered hook: ${hookName} (${entry.name})`); | ||
| } | ||
| }; | ||
| } |
There was a problem hiding this comment.
🟢 Low utils/hooks.ts:138
getRegisteredHooks returns hook names that have no active listeners after all hooks are unregistered. When the last hook is removed via the unregister function (lines 138-145), the entry is deleted from the array but the empty array remains in the Map, so the key is still returned. This creates an inconsistency where getRegisteredHooks reports a hook as present but hasHooks returns false. Consider deleting the Map key when the array becomes empty.
const hooks = this.hooks.get(hookName) || [];
const index = hooks.findIndex((h) => h.name === entry.name);
if (index !== -1) {
hooks.splice(index, 1);
+ if (hooks.length === 0) {
+ this.hooks.delete(hookName);
+ }
logDebugContext("Hooks", `Unregistered hook: ${hookName} (${entry.name})`);
}
};🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file cli/src/utils/hooks.ts around lines 138-146:
`getRegisteredHooks` returns hook names that have no active listeners after all hooks are unregistered. When the last hook is removed via the unregister function (lines 138-145), the entry is deleted from the array but the empty array remains in the Map, so the key is still returned. This creates an inconsistency where `getRegisteredHooks` reports a hook as present but `hasHooks` returns `false`. Consider deleting the Map key when the array becomes empty.
Evidence trail:
cli/src/utils/hooks.ts lines 138-145 (unregister function uses splice but doesn't delete Map key), lines 178-181 (hasHooks checks array length > 0), lines 205-207 (getRegisteredHooks returns all Map keys). Commit: REVIEWED_COMMIT
Additional Comments (5)
Examples:
A safe fix is to return the trimmed, quoted result directly:
If the validated command and args are later assembled into a shell string (e.g., via Add the missing redirection check to match
Make the call explicit by either:
The character class This is functionally safe (over-restriction is fine for security), but the redundancy obscures the intent. If you ever need to allow single Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Both branches of the ternary are identical ( |
Additional Comments (10)
Alternatively, remove lazy-registration from
Reorder the checks from most-specific to least-specific:
|
Additional Comments (5)
This is an asymmetry between the two validation functions that creates a potential injection path. For example: if the validated command is
Consequences:
Handle try {
const content = readFileSync(configPath, "utf-8");
const parsed = YAML.parse(content);
if (hasPrototypePollution(parsed)) {
logWarn(`Config file at ${configPath} contains prototype-pollution keys — ignoring.`);
return RalphyConfigSchema.parse({});
}
return RalphyConfigSchema.parse(parsed);
} catch (error) {
// Only ordinary parse/validation errors reach here
logWarn(`Invalid config file at ${configPath}: ${error}. Falling back to defaults.`);
logDebug(`Config parse details: ${error}`);
return RalphyConfigSchema.parse({});
}
Both Consider logging supplementary context in Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Both arms of the ternary evaluate to const contentSize = typeof content === "string" ? content.length : content.length;For a The size limit guard is therefore inaccurate for string inputs and will never use the correct
The previous implementation used an async, debounced write queue ( In parallel mode, multiple tasks may write progress simultaneously, so this change reintroduces file-system contention and latency that the original design explicitly addressed. The PR description says the old code had "drift," but it doesn't explain why the synchronous path is preferable. Consider retaining |
Additional Comments (3)
The Anthropic API keys start with
The
The Consider creating the directory in the constructor: import { mkdirSync } from "node:fs";
// in constructor, after validateLogPath:
mkdirSync(path.dirname(this.filePath), { recursive: true }); |
Additional Comments (1)
The |
Additional Comments (2)
The new approach fires Consider either:
Example: export function logTaskProgress(
task: string,
status: "completed" | "failed",
workDir = process.cwd(),
): Promise<void> {
const progressPath = getProgressPath(workDir);
if (!existsSync(progressPath)) {
return Promise.resolve();
}
const timestamp = new Date().toISOString().slice(0, 16).replace("T", " ");
const icon = status === "completed" ? "✓" : "✗";
const line = `- [${icon}] ${timestamp} - ${task}\n`;
return appendFile(progressPath, line, "utf-8").catch((error) => {
logWarn(`Failed to append task progress: ${error}`);
});
}
This is intentionally broad to support Consider rejecting command tokens that contain if (commandToken.includes("../") || commandToken.startsWith("..")) {
debugLog(`Command validation failed: path traversal in command token "${commandToken}"`);
return null;
}Add this check before the |
1 participant
PR1: config runtime
Summary
This PR hardens config loading/writing and runtime config behavior. It focuses on safety, predictable fallback behavior, and integration with shared contracts.
Why this PR exists
What it adds
cli/src/config/detector.tscli/src/config/index.tscli/src/config/loader.tscli/src/config/writer.tscli/src/engines/validation.tsused by config paths.cli/src/utils/hooks.tscli/src/utils/metrics.tscli/src/utils/opencode-parser.tscli/src/utils/resource-manager.tscli/src/utils/templates.tscli/src/utils/transform.tsSecurity and reliability work
Impact on later PRs
Validation
bun run check,bun tsc --noEmit, andbun testin order.Note
Harden config runtime loading and CLI utilities by validating commands, blocking prototype-pollution keys, and adding centralized resource tracking with periodic cleanup and secret sanitization caps at 1MB
Add
ResourceManagerwith secure temp creation, periodic cleanup, and a singleton accessor; introduce secret scrubbing with bounded regex and 1MB caps across sanitizers, telemetry, and transformers; enforce command and args validation in config loading and CLI command runners; add structured logging with sinks and secret-aware messages; and refactor cleanup and signal handling to reliably terminate child processes and run registered disposers.📍Where to Start
Start with
ResourceManagerand its singleton accessor in resource-manager.ts, then review config hardening in loader.ts and command validation in validation.ts.Macroscope summarized 1f2b42b.