stack6:feat: add git sync, telemetry, and ui command updates#165
stack6:feat: add git sync, telemetry, and ui command updates#165VX1D 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 Checked 30 published document(s) in 1 knowledge base(s). No updates required. How did I do? Any feedback? |
Greptile SummaryThis PR closes a large stacked feature with cross-cutting changes across CLI commands, Git integration, telemetry/secret redaction, webhook SSRF hardening, sandbox path validation, file locking, and UI rendering. The security improvements (DNS validation, private-IP blocking, path traversal guards) are meaningful and the overall architecture is coherent. Key changes:
Confidence Score: 2/5
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[task / run command] --> B{parallel mode?}
B -- yes --> C[runParallel]
B -- no --> D[runSequential]
C --> E[resolveSafeRelativePath\nPRD path validation]
E --> F{worktree\navailable?}
F -- yes --> G[runAgentInWorktree\ncreateAgentWorktree]
F -- no/error --> H[runAgentInSandbox\ncreateSandbox]
G --> I[acquireLocksForFiles\nlocking.ts]
H --> I
I --> J[AI Engine\nexecution]
J --> K[getModifiedFiles\ninode cycle detection]
K --> L[copyBackPlannedFiles\nParallel / syncSandboxToOriginal]
L --> M[releaseLocksForFiles]
M --> N[sendNotifications\nwebhook.ts]
N --> O{webhook URL\nvalidation}
O -- invalid --> P[logWarn, skip]
O -- valid --> Q[assertDnsStillSafe\nbefore each retry]
Q --> R[fetch with timeout\n& AbortController]
D --> S[TelemetryCollector\nsanitizeSecrets]
J --> S
S --> T[TelemetryWriter\ntolerant JSONL write]
style E fill:#f9a,stroke:#c00
style O fill:#f9a,stroke:#c00
style Q fill:#ffd,stroke:#aa0
style I fill:#adf,stroke:#05f
style L fill:#fca,stroke:#c60
Last reviewed commit: fa4cb06 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| 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 |
There was a problem hiding this comment.
telemetry_webhook documented but not implemented
The README advertises telemetry_webhook as a new config key under notifications, but:
NotificationsSchemaincli/src/config/types.tsdoes not includetelemetry_webhook— any value users set will be silently stripped by Zod's default strict parsing.sendNotifications()incli/src/notifications/webhook.tshas no branch that reads or sends totelemetry_webhook.
Users who add this key to their .ralphy/config.yaml will receive no notification and no error. Either implement the field or remove it from the documentation.
Prompt To Fix With AI
This is a comment left during a code review.
Path: cli/README.md
Line: 250
Comment:
**`telemetry_webhook` documented but not implemented**
The README advertises `telemetry_webhook` as a new config key under `notifications`, but:
1. `NotificationsSchema` in `cli/src/config/types.ts` does not include `telemetry_webhook` — any value users set will be silently stripped by Zod's default strict parsing.
2. `sendNotifications()` in `cli/src/notifications/webhook.ts` has no branch that reads or sends to `telemetry_webhook`.
Users who add this key to their `.ralphy/config.yaml` will receive no notification and no error. Either implement the field or remove it from the documentation.
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}
| private redrawPlanning(): void { | ||
| if (!this.planningTasks) return; | ||
|
|
||
| const tasks = this.planningTasks; | ||
| const doneCount = tasks.filter((t) => t.status === "done").length; | ||
| const failedCount = tasks.filter((t) => t.status === "failed").length; | ||
| const activeCount = tasks.filter((t) => t.status === "active").length; | ||
|
|
||
| const lines: string[] = []; | ||
| // Only show header in non-verbose mode or if state changed (simplified) | ||
| // In verbose mode we probably just want to log specific events, | ||
| // but for now let's just avoid clearing. | ||
| if (!verboseMode) { | ||
| lines.push( | ||
| pc.cyan( | ||
| `[PLANNING] ${doneCount}/${tasks.length} done${failedCount > 0 ? `, ${failedCount} failed` : ""} (${activeCount} active)`, | ||
| ), | ||
| ); | ||
| } | ||
|
|
||
| for (let i = 0; i < tasks.length; i++) { | ||
| const task = tasks[i]; | ||
| const taskTrunc = task.title.length > 55 ? `${task.title.substring(0, 52)}...` : task.title; | ||
|
|
||
| if (task.status === "active") { | ||
| const spinner = getSpinner(this.planningSpinnerIdx); | ||
| const elapsed = task.startTime ? ` (${formatDuration(Date.now() - task.startTime)})` : ""; | ||
| const stepInfo = task.currentStep ? ` ${pc.dim(`[${task.currentStep}]`)}` : ""; | ||
| const rewardInfo = task.reward ? ` ${pc.yellow(`Reward: ${task.reward}`)}` : ""; | ||
| lines.push(`${spinner} Planning: ${taskTrunc}${elapsed}${stepInfo}${rewardInfo}`); | ||
|
|
||
| // Render recent steps history for active tasks | ||
| if (task.recentSteps && task.recentSteps.length > 0) { | ||
| // Show progress flow with arrows | ||
| // Current step (with completion status) → Previous steps | ||
| const completionIcon = pc.cyan("→"); | ||
| lines.push(` ${completionIcon} ${pc.bold(task.currentStep || "Working")}`); | ||
|
|
||
| // Show up to 5 previous steps with arrows | ||
| for (let i = 1; i < task.recentSteps.length && i < 6; i++) { | ||
| const step = task.recentSteps[i]; | ||
| const formattedStep = this.formatPlanningStep(step); | ||
| // Skip empty formatted steps | ||
| if (formattedStep) { | ||
| lines.push(` ${pc.dim(`↓ ${formattedStep}`)}`); | ||
| } | ||
| } | ||
| } | ||
| } else if (task.status === "done") { | ||
| const spinner = pc.green("✓"); | ||
| const completionIcon = pc.green("✓"); | ||
| const files = task.files || 0; | ||
| const time = task.time || "?"; | ||
| const rewardInfo = task.reward ? ` ${pc.yellow(`Reward: ${task.reward}`)}` : ""; | ||
| lines.push( | ||
| `${spinner} ${pc.dim(completionIcon)} Planning: ${taskTrunc} (${files} files, ${time}s)${rewardInfo}`, | ||
| ); | ||
| } else if (task.status === "failed") { | ||
| const rewardInfo = task.reward ? ` ${pc.yellow(`Reward: ${task.reward}`)}` : ""; | ||
| lines.push(`${pc.red("[FAIL]")} Planning: ${taskTrunc}${rewardInfo}`); | ||
| } else if (task.status === "pending") { | ||
| const rewardInfo = task.reward ? ` ${pc.yellow(`Reward: ${task.reward}`)}` : ""; | ||
| lines.push(` Pending: ${taskTrunc}${rewardInfo}`); | ||
| } | ||
| } | ||
| this.lastLineCount = lines.length; |
There was a problem hiding this comment.
🟠 High ui/progress.ts:138
Print before updating lastLineCount. redrawPlanning and showHeartbeat build lines but never write to stdout, making the UI invisible and causing subsequent clears to erase unrelated output. Write the lines first, then set lastLineCount.
this.lastLineCount = lines.length;
+ clearConsole(this.lastLineCount);
+ for (const line of lines) {
+ process.stdout.write(`${line}\n`);
+ }
}Also found in 1 other location(s)
cli/src/ui/spinner.ts:35
The fallback spinner object, used when
nanospinnerfails to initialize (e.g., in CI or non-TTY environments), defines itsupdatemethod as a no-op() => {}. While this correctly prevents the periodictick()method from spamming the logs, it causesupdateStep()to silently fail to display any output.updateStepcallsthis.spinner.update()to inform the user of state changes (e.g., "Thinking" -> "Writing"), but since the fallback implementation does nothing and does not throw, the intended backup logging in theupdateStepcatchblock (lines 114-117) is never triggered. As a result, users in fallback environments will see the spinner start and finish, but all intermediate steps will be invisible.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file cli/src/ui/progress.ts around lines 138-203:
Print before updating `lastLineCount`. `redrawPlanning` and `showHeartbeat` build lines but never write to `stdout`, making the UI invisible and causing subsequent clears to erase unrelated output. Write the lines first, then set `lastLineCount`.
Evidence trail:
File: cli/src/ui/progress.ts (REVIEWED_COMMIT)
- Lines 138-208: `redrawPlanning()` builds `lines[]` array, sets `this.lastLineCount = lines.length` at line 208, but has no `process.stdout.write()` call
- Lines 271-282: `showHeartbeat()` builds `parts[]` array, sets `this.lastLineCount = 1` at line 280, but has no `process.stdout.write()` call
- Lines 238-267: `renderAgentCards()` shows correct pattern: calls `process.stdout.write()` before setting `lastLineCount`
Also found in 1 other location(s):
- cli/src/ui/spinner.ts:35 -- The fallback spinner object, used when `nanospinner` fails to initialize (e.g., in CI or non-TTY environments), defines its `update` method as a no-op `() => {}`. While this correctly prevents the periodic `tick()` method from spamming the logs, it causes `updateStep()` to silently fail to display any output. `updateStep` calls `this.spinner.update()` to inform the user of state changes (e.g., "Thinking" -> "Writing"), but since the fallback implementation does nothing and does not throw, the intended backup logging in the `updateStep` `catch` block (lines 114-117) is never triggered. As a result, users in fallback environments will see the spinner start and finish, but all intermediate steps will be invisible.
| showPlanningProgress(tasks: PlanningTaskStatus[]): void { | ||
| if (!ansiSupported) { | ||
| for (const task of tasks) { | ||
| if (task.status === "active") { | ||
| } else if (task.status === "done") { | ||
| } | ||
| } | ||
| return; | ||
| } |
There was a problem hiding this comment.
🟢 Low ui/progress.ts:113
Implement a non‑ANSI fallback in showPlanningProgress. The !ansiSupported path is empty, so users without ANSI (Windows/CI) see no progress; log simple per‑task updates without clearing/animation.
- for (const task of tasks) {
- if (task.status === "active") {
- } else if (task.status === "done") {
- }
- }
- return;
+ for (const task of tasks) {
+ const statusIcon = task.status === "done" ? "✓" : task.status === "failed" ? "✗" : "•";
+ process.stdout.write(`${statusIcon} ${task.title}\n`);
+ }
+ return;🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file cli/src/ui/progress.ts around lines 113-121:
Implement a non‑ANSI fallback in `showPlanningProgress`. The `!ansiSupported` path is empty, so users without ANSI (Windows/CI) see no progress; log simple per‑task updates without clearing/animation.
Evidence trail:
cli/src/ui/progress.ts lines 113-121 at REVIEWED_COMMIT: The `showPlanningProgress` function has an `if (!ansiSupported)` branch containing a for loop with empty `if (task.status === "active") { }` and `else if (task.status === "done") { }` blocks, followed by an early return. The code path does nothing.
| function setLogHandler(_handler: ((message: string) => void) | null): void { | ||
| // No-op: handler is not currently used but kept for API compatibility | ||
| } | ||
|
|
There was a problem hiding this comment.
🟢 Low ui/progress.ts:48
setLogHandler accepts a handler but does nothing with it, so safeLog is never registered even though showPhaseHeader passes it. When logs occur during progress display, they write directly to stdout without clearing the spinner first. This shifts the cursor and corrupts the clearConsole line count, causing visual artifacts (ghost frames) and overwritten log messages.
+let activeLogHandler: ((message: string) => void) | null = null;
+
+function setLogHandler(handler: ((message: string) => void) | null): void {
+ activeLogHandler = handler;
+}
+
+export function logDuringProgress(message: string): void {
+ if (activeLogHandler) {
+ activeLogHandler(message);
+ } else {
+ console.log(message);
+ }
}🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file cli/src/ui/progress.ts around lines 48-51:
`setLogHandler` accepts a handler but does nothing with it, so `safeLog` is never registered even though `showPhaseHeader` passes it. When logs occur during progress display, they write directly to stdout without clearing the spinner first. This shifts the cursor and corrupts the `clearConsole` line count, causing visual artifacts (ghost frames) and overwritten log messages.
Evidence trail:
cli/src/ui/progress.ts lines 48-50: `setLogHandler` function is explicitly a no-op with comment 'No-op: handler is not currently used but kept for API compatibility'
cli/src/ui/progress.ts line 93: `showPhaseHeader` calls `setLogHandler(this.safeLog)` attempting to register the handler
cli/src/ui/progress.ts lines 75-86: `safeLog` method implementation that calls `this.clear()` before writing - this is designed to prevent visual artifacts but is never actually used
cli/src/ui/progress.ts line 277: `stopAll()` also calls `setLogHandler(null)` suggesting the API was intended to work
| } | ||
| } | ||
|
|
||
| return { valid: true }; |
There was a problem hiding this comment.
🟡 Medium notifications/webhook.ts:111
validateWebhookUrl checks that the resolved IP is not private, but callers then use the original hostname in fetch, which performs a second DNS lookup. An attacker can serve a safe IP during validation and a private IP during the actual request, bypassing SSRF protection. Consider returning the resolved IP and using it directly in the fetch request (e.g., via a custom HTTP agent) to prevent DNS rebinding.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file cli/src/notifications/webhook.ts around line 111:
`validateWebhookUrl` checks that the resolved IP is not private, but callers then use the original hostname in `fetch`, which performs a second DNS lookup. An attacker can serve a safe IP during validation and a private IP during the actual request, bypassing SSRF protection. Consider returning the resolved IP and using it directly in the fetch request (e.g., via a custom HTTP agent) to prevent DNS rebinding.
Evidence trail:
cli/src/notifications/webhook.ts lines 95-104 (DNS lookup in validateWebhookUrl), lines 268-275 (Discord webhook validation then call with original URL), lines 281-290 (Slack webhook), lines 294-303 (Custom webhook), lines 191-194 (fetch call in sendDiscordNotification using webhookUrl directly). The validation at lines 95-104 resolves DNS once, but the fetch at line 191 will perform a separate DNS lookup using the hostname.
| let ansiSupportChecked = false; | ||
| let ansiSupported = true; | ||
|
|
||
| function checkAnsiSupport(): void { |
There was a problem hiding this comment.
🟢 Low ui/progress.ts:13
ANSI detection should be TTY‑aware and lazy. Gate ansiSupported on process.stdout.isTTY, and defer any chcp/capability checks to first use. This avoids escape codes in redirected output and the synchronous import‑time chcp (including its persistent code‑page change).
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file cli/src/ui/progress.ts around line 13:
ANSI detection should be TTY‑aware and lazy. Gate `ansiSupported` on `process.stdout.isTTY`, and defer any `chcp`/capability checks to first use. This avoids escape codes in redirected output and the synchronous import‑time `chcp` (including its persistent code‑page change).
Evidence trail:
cli/src/ui/progress.ts lines 10-29 at REVIEWED_COMMIT: ansiSupported defaults to true (line 11), checkAnsiSupport() does not check process.stdout.isTTY, checkAnsiSupport() is called at import time (line 29), execSync runs 'chcp 65001' on Windows (line 22). clearConsole() at lines 52-60 writes ANSI escape sequences unconditionally when ansiSupported is true.
| const action = fileActionMatch[1].trim(); | ||
| let file = fileActionMatch[2].trim(); | ||
| // Remove task title from file path if present | ||
| file = file.replace(/^Task\s+ST-\d+:\s*[^"]+"\s*/, "").trim(); |
There was a problem hiding this comment.
🟡 Medium ui/progress.ts:319
Make Task ST-... stripping robust. In formatAgentStep/formatPlanningStep, the current regexes either hide valid messages or remove filenames. Use a single helper that precisely detects Task ST-<id>: (optional quotes, anchored) and removes only the prefix, preserving the content after the colon.
- file = file.replace(/^Task\s+ST-\d+:\s*[^"]+"\s*/, "").trim();
+ file = file.replace(/^Task\s+ST-\d+:\s*(?:"[^"]*"\s*)?/, "").trim();Also found in 1 other location(s)
cli/src/notifications/webhook.ts:24
The IPv4 blocking regexes in
BLOCKED_IP_RANGESlack start-of-string anchors (^), meaning they will incorrectly match valid public IPs that happen to contain blocked sequences (e.g.1.127.0.1or210.0.0.1). Although the comment implies anchors are present (^127\., etc.), the actual regex literals in the code object do not have them in the provided diff view for some entries if I look closely? Wait, looking at lines 15-21, they do have^.
However, there is a different issue: the regex for 0.0.0.0/8 is /^0\./. This blocks any IP starting with 0., which technically is valid (0.0.0.0/8 is 'Current network' and reserved, so blocking it is correct).
Let's re-examine BLOCKED_IPV6_RANGES (lines 24-35).
Line 27: /^fe80:/i matches fe80:.
Line 28: /^fc00:/i matches fc00:.
However, fc00::/7 is the Unique Local Address range. This means it covers fc00: through fdff:. The regex /^fc00:/i ONLY catches addresses starting with fc00. It completely misses fd00::/8 (the other half of the ULA range, often used for locally assigned ULAs). Users with private IPv6 networks starting with fd (which is standard for random generation) will not be blocked, bypassing the SSRF protection.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file cli/src/ui/progress.ts around line 319:
Make `Task ST-...` stripping robust. In `formatAgentStep`/`formatPlanningStep`, the current regexes either hide valid messages or remove filenames. Use a single helper that precisely detects `Task ST-<id>:` (optional quotes, anchored) and removes only the prefix, preserving the content after the colon.
Evidence trail:
cli/src/ui/progress.ts lines 440-442: regex `/^Task\s+ST-\d+:\s*/` lacks end anchor, causing it to match and return empty for valid content like `Task ST-123: Read config.json`
cli/src/ui/progress.ts line 319: regex `/^Task\s+ST-\d+:\s*[^"]+"\s*/` requires quote character, leaves trailing quote in output
cli/src/ui/progress.ts lines 449, 458, 469: different pattern `/^"Task\s+ST-\d+:\s*[^"]+"\s*/` with leading quote
cli/src/ui/progress.ts line 490: yet another pattern `Task ST-\d+:\s*(.+)` without start anchor
Also found in 1 other location(s):
- cli/src/notifications/webhook.ts:24 -- The IPv4 blocking regexes in `BLOCKED_IP_RANGES` lack start-of-string anchors (`^`), meaning they will incorrectly match valid public IPs that happen to contain blocked sequences (e.g. `1.127.0.1` or `210.0.0.1`). Although the comment implies anchors are present (`^127\.`, etc.), the actual regex literals in the code object do not have them in the provided diff view for some entries if I look closely? Wait, looking at lines 15-21, they *do* have `^`.
However, there is a different issue: the regex for `0.0.0.0/8` is `/^0\./`. This blocks any IP starting with `0.`, which technically is valid (0.0.0.0/8 is 'Current network' and reserved, so blocking it is correct).
Let's re-examine `BLOCKED_IPV6_RANGES` (lines 24-35).
Line 27: `/^fe80:/i` matches `fe80:`.
Line 28: `/^fc00:/i` matches `fc00:`.
However, `fc00::/7` is the Unique Local Address range. This means it covers `fc00:` through `fdff:`. The regex `/^fc00:/i` ONLY catches addresses starting with `fc00`. It completely misses `fd00::/8` (the other half of the ULA range, often used for locally assigned ULAs). Users with private IPv6 networks starting with `fd` (which is standard for random generation) will not be blocked, bypassing the SSRF protection.
| // Using three-dot notation to show changes since the branches diverged | ||
| const diffOutput = await git.diff([`${targetBranch}...${branch}`, "--name-only"]); | ||
| // Using two-dot notation to show unique changes to target (excluding ancestor commits) | ||
| const diffOutput = await git.diff([`${targetBranch}..${branch}`, "--name-only"]); |
There was a problem hiding this comment.
🟡 Medium git/merge.ts:271
The two-dot notation in git.diff([${targetBranch}..${branch}, "--name-only"]) returns files changed in targetBranch since the branch diverged, not just files the branch itself modified. This pollutes filesChanged with unrelated files, causing calculateConflictScore to report false conflicts between branches that never touched the same files. The conflict-sorting optimization becomes ineffective because every branch appears to overlap with every other branch on the files targetBranch changed.
| const diffOutput = await git.diff([`${targetBranch}..${branch}`, "--name-only"]); | |
| const diffOutput = await git.diff([`${targetBranch}...${branch}`, "--name-only"]); |
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file cli/src/git/merge.ts around line 271:
The two-dot notation in `git.diff([`${targetBranch}..${branch}`, "--name-only"])` returns files changed in `targetBranch` since the branch diverged, not just files the branch itself modified. This pollutes `filesChanged` with unrelated files, causing `calculateConflictScore` to report false conflicts between branches that never touched the same files. The conflict-sorting optimization becomes ineffective because every branch appears to overlap with every other branch on the files `targetBranch` changed.
Evidence trail:
- cli/src/git/merge.ts lines 260-288 (REVIEWED_COMMIT): `analyzePreMerge` function with two-dot notation at line 271
- cli/src/git/merge.ts lines 296-314 (REVIEWED_COMMIT): `calculateConflictScore` function that counts file overlaps
- Git documentation at https://git-scm.com/docs/git-diff/2.29.0: confirms `git diff A..B` shows all differences between A and B trees, while `git diff A...B` shows changes since common ancestor ('git diff A...B' is equivalent to 'git diff $(git merge-base A B) B')
| /** | ||
| * Mark as error | ||
| */ | ||
| error(message?: string): void { |
There was a problem hiding this comment.
🟢 Low ui/spinner.ts:168
The error method drops elapsed time from the output when a custom message is provided. The previous implementation appended pc.red([elapsed]) explicitly, but the new implementation returns message directly without timing information, losing observability for failed long-running tasks. Consider always including the elapsed time in the error output, or document if this is an intentional change.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file cli/src/ui/spinner.ts around line 168:
The `error` method drops elapsed time from the output when a custom `message` is provided. The previous implementation appended `pc.red([elapsed])` explicitly, but the new implementation returns `message` directly without timing information, losing observability for failed long-running tasks. Consider always including the elapsed time in the error output, or document if this is an intentional change.
Evidence trail:
cli/src/ui/spinner.ts lines 168-173 at REVIEWED_COMMIT - current error() method implementation. git_diff base=48c77a0d65607d1d881833cd0bfe553c3add18e2^ head=48c77a0d65607d1d881833cd0bfe553c3add18e2 path=cli/src/ui/spinner.ts shows the previous implementation with `this.spinner.error({ text: \`${message || this.formatText()} ${pc.red(\`[${elapsed}]\`)}\` });` which always appended elapsed time, versus the new implementation that only includes time via formatText() when no message is provided.
| tick(): void { | ||
| this.spinner.update({ text: this.formatText() }); | ||
| if (!this.tickInterval) { | ||
| // Don't update if spinner is stopped | ||
| return; | ||
| } | ||
|
|
||
| try { | ||
| // Always update the timer, bypassing throttle |
There was a problem hiding this comment.
🟢 Low ui/spinner.ts:123
The heartbeat mechanism is rendered ineffective by the guard clause in tick(). The constructor initializes heartbeatInterval to keep the spinner updating even if the main timer fails, but tick() returns early when this.tickInterval is null. If tickInterval fails to initialize, the heartbeat calls tick() which immediately exits, defeating the fallback. Conversely, if tickInterval is active, the heartbeat triggers redundant updates every 5 seconds that bypass the throttle in updateStep() but still call spinner.update(). Consider using a separate flag like isRunning to track spinner state, so the heartbeat can function independently of the main timer.
tick(): void {
- if (!this.tickInterval) {
+ if (!this.tickInterval && !this.heartbeatInterval) {
// Don't update if spinner is stopped
return;
}🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file cli/src/ui/spinner.ts around lines 123-130:
The heartbeat mechanism is rendered ineffective by the guard clause in `tick()`. The constructor initializes `heartbeatInterval` to keep the spinner updating even if the main timer fails, but `tick()` returns early when `this.tickInterval` is null. If `tickInterval` fails to initialize, the heartbeat calls `tick()` which immediately exits, defeating the fallback. Conversely, if `tickInterval` is active, the heartbeat triggers redundant updates every 5 seconds that bypass the throttle in `updateStep()` but still call `spinner.update()`. Consider using a separate flag like `isRunning` to track spinner state, so the heartbeat can function independently of the main timer.
Evidence trail:
cli/src/ui/spinner.ts lines 40-49 (tickInterval initialization with catch setting to null), lines 54-69 (heartbeatInterval setup calling tick()), lines 111-114 (tick() guard clause returning early when tickInterval is null), line 113 comment 'Don't update if spinner is stopped'
|
|
||
| lines.push(""); // Add spacing between agents | ||
| } | ||
|
|
There was a problem hiding this comment.
🟡 Medium ui/progress.ts:265
renderAgentCards uses clearConsole to refresh the UI, but clearConsole returns early without clearing when ansiSupported is false (line 52). In non-ANSI environments, the agent cards are reprinted to stdout on every tick without clearing previous output, causing massive log flooding with duplicated agent state. Consider detecting non-ANSI support and disabling the animated refresh, or falling back to a single-line update strategy.
renderAgentCards(agents: AgentProgress[]): void {
const now = Date.now();
const activeAgents = agents.filter((a) => a.status === "working");
+ // Skip animated rendering in non-ANSI environments to avoid output flooding
+ if (!ansiSupported) {
+ return;
+ }
+
// Show each agent in a static row format with their recent action
const lines: string[] = [];
for (const agent of activeAgents) {🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file cli/src/ui/progress.ts around line 265:
`renderAgentCards` uses `clearConsole` to refresh the UI, but `clearConsole` returns early without clearing when `ansiSupported` is false (line 52). In non-ANSI environments, the agent cards are reprinted to stdout on every tick without clearing previous output, causing massive log flooding with duplicated agent state. Consider detecting non-ANSI support and disabling the animated refresh, or falling back to a single-line update strategy.
Evidence trail:
cli/src/ui/progress.ts lines 52-56 (clearConsole returns early when !ansiSupported), lines 265-269 (renderAgentCards calls clearConsole then unconditionally writes lines), lines 10-28 (ansiSupported detection logic). Commit: REVIEWED_COMMIT
|
|
||
| import { spawn } from "bun"; | ||
|
|
||
| const testFiles = ["__tests__/sandbox-security.test.ts", "__tests__/locking-security.test.ts"]; |
There was a problem hiding this comment.
Reference to non-existent test file locking-security.test.ts
__tests__/locking-security.test.ts is listed in testFiles but it is not included in this PR and does not appear to exist in the repository. When this runner executes, bun test __tests__/locking-security.test.ts will fail with a "file not found" or similar error, causing allPassed to be false and the overall runner to exit with code 1.
Either add the missing test file or remove it from the list until it exists:
| const testFiles = ["__tests__/sandbox-security.test.ts", "__tests__/locking-security.test.ts"]; | |
| const testFiles = ["__tests__/sandbox-security.test.ts"]; |
Prompt To Fix With AI
This is a comment left during a code review.
Path: cli/__tests__/run-tests.test.ts
Line: 9
Comment:
**Reference to non-existent test file `locking-security.test.ts`**
`__tests__/locking-security.test.ts` is listed in `testFiles` but it is not included in this PR and does not appear to exist in the repository. When this runner executes, `bun test __tests__/locking-security.test.ts` will fail with a "file not found" or similar error, causing `allPassed` to be `false` and the overall runner to exit with code `1`.
Either add the missing test file or remove it from the list until it exists:
```suggestion
const testFiles = ["__tests__/sandbox-security.test.ts"];
```
How can I resolve this? If you propose a fix, please make it concise.|
|
||
| try { | ||
| // Get list of files that differ between the branch and target |
There was a problem hiding this comment.
Misleading comment on two-dot diff notation
The comment says "two-dot notation to show unique changes to target (excluding ancestor commits)", but that is the behaviour of three-dot notation (A...B). Two-dot (A..B) is equivalent to git diff A B — it shows all differences between the two branch tips, including commits that landed in targetBranch after the branches diverged. This can inflate the conflict-candidate file list with files that were only changed on the target, leading to false positives in analyzePreMerge.
If the intent is to detect files the feature branch uniquely changes, the original three-dot notation was more accurate. At minimum the comment should be corrected to describe what the notation actually does.
Prompt To Fix With AI
This is a comment left during a code review.
Path: cli/src/git/merge.ts
Line: 272-274
Comment:
**Misleading comment on two-dot diff notation**
The comment says `"two-dot notation to show unique changes to target (excluding ancestor commits)"`, but that is the behaviour of three-dot notation (`A...B`). Two-dot (`A..B`) is equivalent to `git diff A B` — it shows **all** differences between the two branch tips, including commits that landed in `targetBranch` after the branches diverged. This can inflate the conflict-candidate file list with files that were only changed on the target, leading to false positives in `analyzePreMerge`.
If the intent is to detect files the feature branch uniquely changes, the original three-dot notation was more accurate. At minimum the comment should be corrected to describe what the notation actually does.
How can I resolve this? If you propose a fix, please make it concise.
|
|
||
| import { spawn } from "bun"; | ||
|
|
||
| const testFiles = ["__tests__/sandbox-security.test.ts", "__tests__/locking-security.test.ts"]; |
There was a problem hiding this comment.
The test runner references __tests__/locking-security.test.ts (line 9), which does not exist in the repository. When this runner executes, bun test __tests__/locking-security.test.ts will fail with "file not found", causing allPassed to be false and the overall runner to exit with code 1.
Either add the missing test file or remove it from the list:
| const testFiles = ["__tests__/sandbox-security.test.ts", "__tests__/locking-security.test.ts"]; | |
| const testFiles = ["__tests__/sandbox-security.test.ts"]; |
Prompt To Fix With AI
This is a comment left during a code review.
Path: cli/__tests__/run-tests.test.ts
Line: 9
Comment:
The test runner references `__tests__/locking-security.test.ts` (line 9), which does not exist in the repository. When this runner executes, `bun test __tests__/locking-security.test.ts` will fail with "file not found", causing `allPassed` to be `false` and the overall runner to exit with code 1.
Either add the missing test file or remove it from the list:
```suggestion
const testFiles = ["__tests__/sandbox-security.test.ts"];
```
How can I resolve this? If you propose a fix, please make it concise.
Additional Comments (2)
The Stale-lock cleanup should be best-effort and never crash the acquirer: } catch {
try {
unlinkSync(filePath);
} catch {
// best-effort: file may have been deleted by another process
}
}The same applies to the Prompt To Fix With AIThis is a comment left during a code review.
Path: cli/src/execution/locking.ts
Line: 1470-1489
Comment:
**Uncaught exception from `unlinkSync` in catch block of `cleanupStaleLockFiles`**
The `catch` block at line 1486-1488 calls `unlinkSync(filePath)` unconditionally. If `unlinkSync` itself throws (e.g., `ENOENT` because another process already deleted the file between the failed read and this delete attempt), the exception is not caught and propagates up through `cleanupStaleLockFiles` into `acquireFileLock`. Since `cleanupStaleLockFiles` is called **outside** the inner retry `try/catch`, this unhandled exception would crash the entire `acquireFileLock` call for all callers.
Stale-lock cleanup should be best-effort and never crash the acquirer:
```typescript
} catch {
try {
unlinkSync(filePath);
} catch {
// best-effort: file may have been deleted by another process
}
}
```
The same applies to the `unlinkSync` inside the `if (now - lockInfo.timestamp >= lockInfo.timeout)` branch — it should also be wrapped.
How can I resolve this? If you propose a fix, please make it concise.
The fallback path // Atomics.wait is allowed in Node.js main/worker threads but blocked in browser main threads.
// This code path is only expected in non-Bun server runtimes.
Atomics.wait(sleepArray, 0, 0, ms);Adding a clear runtime comment documents the intent and makes the restriction visible to future maintainers. Prompt To Fix With AIThis is a comment left during a code review.
Path: cli/src/execution/locking.ts
Line: 1412-1419
Comment:
**`Atomics.wait` fallback is disallowed on the main thread in some worker environments**
The fallback path `Atomics.wait(sleepArray, 0, 0, ms)` is reached when `Bun.sleepSync` is not available (e.g., Node.js workers, or future non-Bun environments). While `Atomics.wait` is permitted on the main Node.js thread, it is **not permitted** in a browser's main thread and can be unexpectedly restricted in certain worker configurations. Since `acquireFileLock` is synchronous and intended for non-async use, the fallback warrants a comment or a narrower guard:
```typescript
// Atomics.wait is allowed in Node.js main/worker threads but blocked in browser main threads.
// This code path is only expected in non-Bun server runtimes.
Atomics.wait(sleepArray, 0, 0, ms);
```
Adding a clear runtime comment documents the intent and makes the restriction visible to future maintainers.
How can I resolve this? If you propose a fix, please make it concise. |
| try { | ||
| absoluteBase = realpathSync(resolve(baseDir)); | ||
| absoluteTarget = realpathSync(resolve(baseDir, targetPath)); | ||
| } catch { | ||
| // If realpath fails (e.g., path doesn't exist), fall back to resolve | ||
| absoluteBase = resolve(baseDir); | ||
| absoluteTarget = resolve(baseDir, targetPath); | ||
| } |
There was a problem hiding this comment.
Combined try/catch resets canonicalized absoluteBase on target-path failure
The two realpathSync calls share a single try/catch block:
try {
absoluteBase = realpathSync(resolve(baseDir)); // may succeed
absoluteTarget = realpathSync(resolve(baseDir, targetPath)); // may throw (path not yet created)
} catch {
absoluteBase = resolve(baseDir); // resets base even if it succeeded above
absoluteTarget = resolve(baseDir, targetPath);
}When targetPath does not exist yet (common for new-file creation), realpathSync(targetPath) throws and the catch block resets absoluteBase from its canonical resolved form back to the possibly-symlink-form. The subsequent startsWith check still works in most cases, but splitting into two independent try/catch blocks ensures each falls back independently:
| try { | |
| absoluteBase = realpathSync(resolve(baseDir)); | |
| absoluteTarget = realpathSync(resolve(baseDir, targetPath)); | |
| } catch { | |
| // If realpath fails (e.g., path doesn't exist), fall back to resolve | |
| absoluteBase = resolve(baseDir); | |
| absoluteTarget = resolve(baseDir, targetPath); | |
| } | |
| try { | |
| absoluteBase = realpathSync(resolve(baseDir)); | |
| } catch { | |
| absoluteBase = resolve(baseDir); | |
| } | |
| try { | |
| absoluteTarget = realpathSync(resolve(baseDir, targetPath)); | |
| } catch { | |
| absoluteTarget = resolve(baseDir, targetPath); | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: cli/src/execution/sandbox.ts
Line: 181-188
Comment:
**Combined `try/catch` resets canonicalized `absoluteBase` on target-path failure**
The two `realpathSync` calls share a single `try/catch` block:
```typescript
try {
absoluteBase = realpathSync(resolve(baseDir)); // may succeed
absoluteTarget = realpathSync(resolve(baseDir, targetPath)); // may throw (path not yet created)
} catch {
absoluteBase = resolve(baseDir); // resets base even if it succeeded above
absoluteTarget = resolve(baseDir, targetPath);
}
```
When `targetPath` does not exist yet (common for new-file creation), `realpathSync(targetPath)` throws and the catch block resets `absoluteBase` from its canonical resolved form back to the possibly-symlink-form. The subsequent `startsWith` check still works in most cases, but splitting into two independent try/catch blocks ensures each falls back independently:
```suggestion
try {
absoluteBase = realpathSync(resolve(baseDir));
} catch {
absoluteBase = resolve(baseDir);
}
try {
absoluteTarget = realpathSync(resolve(baseDir, targetPath));
} catch {
absoluteTarget = resolve(baseDir, targetPath);
}
```
How can I resolve this? If you propose a fix, please make it concise.| 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 |
There was a problem hiding this comment.
telemetry_webhook documented but not implemented
The README advertises telemetry_webhook as a new config key under notifications, but:
NotificationsSchemaincli/src/config/types.tsincludestelemetry_webhook(line 20), so it is parsed.- However,
sendNotifications()incli/src/notifications/webhook.tshas no branch that reads or sends totelemetry_webhook— onlydiscord_webhook,slack_webhook, andcustom_webhookare used.
Users who add this key to their .ralphy/config.yaml will receive no telemetry webhook notification and no error. Either implement sending to telemetry_webhook or remove it from the documentation.
Prompt To Fix With AI
This is a comment left during a code review.
Path: cli/README.md
Line: 250
Comment:
**`telemetry_webhook` documented but not implemented**
The README advertises `telemetry_webhook` as a new config key under `notifications`, but:
1. `NotificationsSchema` in `cli/src/config/types.ts` includes `telemetry_webhook` (line 20), so it is parsed.
2. However, `sendNotifications()` in `cli/src/notifications/webhook.ts` has no branch that reads or sends to `telemetry_webhook` — only `discord_webhook`, `slack_webhook`, and `custom_webhook` are used.
Users who add this key to their `.ralphy/config.yaml` will receive no telemetry webhook notification and no error. Either implement sending to `telemetry_webhook` or remove it from the documentation.
How can I resolve this? If you propose a fix, please make it concise.
| logDebug(`Failed to rollback directory ${createdDir}: ${rollbackErr}`); | ||
| } | ||
| } | ||
| } | ||
| throw new Error(`Failed to create directory structure: ${err}`); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Phase 3: Copy files with TOCTOU protection | ||
| // SECURITY: Re-validate paths immediately before copy to prevent symlink attacks | ||
| let synced = 0; | ||
| for (const change of pendingChanges) { | ||
| try { | ||
| // Re-validate paths right before use to prevent TOCTOU attacks | ||
| const sandboxPath = validatePath(sandboxDir, change.relPath); | ||
| const originalPath = validatePath(originalDir, change.relPath); | ||
|
|
||
| if (!sandboxPath || !originalPath) { | ||
| logDebug(`Security: Path re-validation failed for ${change.relPath}`); |
There was a problem hiding this comment.
Rollback deletes pre-existing directories on failure
directoriesToCreate is built from the dirname of every pending change — it includes directories that already existed on disk before Phase 2 started. The loop only calls mkdirSync when !existsSync(dir), but the failure rollback iterates the full directoriesToCreate set and deletes every member that currently exists, including those that were present before this function ran.
Concrete example:
- Dir
Aalready exists, dirBdoes not. mkdirSync(B)succeeds.mkdirSync(C)fails → rollback triggers.existsSync(A)istrue→rmSync(A, { recursive: true, force: true })deletes the pre-existing directory.
Fix: track only the directories that this function creates and limit the rollback to that set:
const newlyCreatedDirs: string[] = [];
for (const dir of directoriesToCreate) {
if (!existsSync(dir)) {
try {
mkdirSync(dir, { recursive: true });
newlyCreatedDirs.push(dir);
} catch (err) {
// Rollback only what we created
for (const createdDir of newlyCreatedDirs.reverse()) {
try { rmSync(createdDir, { recursive: true, force: true }); } catch {}
}
throw new Error(`Failed to create directory structure: ${err}`);
}
}
}Prompt To Fix With AI
This is a comment left during a code review.
Path: cli/src/execution/sandbox.ts
Line: 807-826
Comment:
**Rollback deletes pre-existing directories on failure**
`directoriesToCreate` is built from the `dirname` of every pending change — it includes directories that already existed on disk before Phase 2 started. The loop only calls `mkdirSync` when `!existsSync(dir)`, but the failure rollback iterates the **full** `directoriesToCreate` set and deletes every member that currently exists, including those that were present before this function ran.
Concrete example:
- Dir `A` already exists, dir `B` does not.
- `mkdirSync(B)` succeeds.
- `mkdirSync(C)` fails → rollback triggers.
- `existsSync(A)` is `true` → `rmSync(A, { recursive: true, force: true })` **deletes the pre-existing directory**.
Fix: track only the directories that this function creates and limit the rollback to that set:
```typescript
const newlyCreatedDirs: string[] = [];
for (const dir of directoriesToCreate) {
if (!existsSync(dir)) {
try {
mkdirSync(dir, { recursive: true });
newlyCreatedDirs.push(dir);
} catch (err) {
// Rollback only what we created
for (const createdDir of newlyCreatedDirs.reverse()) {
try { rmSync(createdDir, { recursive: true, force: true }); } catch {}
}
throw new Error(`Failed to create directory structure: ${err}`);
}
}
}
```
How can I resolve this? If you propose a fix, please make it concise.| return segment; | ||
| } | ||
|
|
||
| return segment | ||
| .replace(/\n{3,}/g, "\n\n") | ||
| .replace(/[ \t]+$/gm, "") | ||
| .replace(/^\s+$/gm, "") | ||
| .replace(/Please note that /gi, "Note: ") | ||
| .replace(/In order to /gi, "To ") | ||
| .replace(/Make sure to /gi, "") | ||
| .replace(/You should /gi, "") | ||
| .replace(/You must /gi, "Must ") | ||
| .replace(/It is important to /gi, "") | ||
| .replace(/Keep in mind that /gi, "") | ||
| .replace(/\*\*Note\*\*:/g, "Note:") | ||
| .replace(/\*\*Important\*\*:/g, "Important:") | ||
| .replace(/\bimplementation\b/gi, "impl") | ||
| .replace(/\binformation\b/gi, "info") | ||
| .replace(/\bdirectory\b/gi, "dir") | ||
| .replace(/\bdirectories\b/gi, "dirs") | ||
| .replace(/\binitialization\b/gi, "init") | ||
| .replace(/\bconfiguration\b/gi, "config") | ||
| .replace(/\bparameters\b/gi, "params") | ||
| .replace(/\benvironment\b/gi, "env") | ||
| .replace(/\bdocumentation\b/gi, "docs"); | ||
| }) | ||
| .join(""); | ||
|
|
||
| return compressed.trim(); | ||
| } | ||
|
|
||
| function csvEscape(value: string): string { | ||
| const escaped = value.replace(/"/g, '""'); | ||
| if (/[",\n\r]/.test(escaped)) { | ||
| return `"${escaped}"`; | ||
| } | ||
| return escaped; | ||
| } |
There was a problem hiding this comment.
Word substitutions corrupt inline code spans
compressMarkdown splits on triple-backtick fenced blocks (```…```) to skip those sections, but inline code using single backticks (e.g. `directory`, `configuration.yaml`, `getEnvironment()`) is not protected and gets rewritten by the word substitutions.
Examples of corruption:
Set the `directory` option→Set the `dir` option(broken option name)Load `configuration.yaml`→Load `config.yaml`(broken file name)call `getEnvironment()`→call `getEnv()`(broken method name)
An AI agent reading a compressed skill may generate calls to non-existent methods or reference wrong file names, producing hard-to-trace runtime bugs.
Extend the split pattern to also capture inline code spans before applying substitutions:
// Split on both fenced blocks AND inline code spans
const segments = content.split(/(```[\s\S]*?```|`[^`\n]+`)/g);
const compressed = segments
.map((segment) => {
// Leave code spans/fences untouched
if (segment.startsWith("```") || segment.startsWith("`")) {
return segment;
}
return segment
// ... existing replacements ...
})
.join("");Prompt To Fix With AI
This is a comment left during a code review.
Path: cli/src/execution/skill-compress.ts
Line: 18-55
Comment:
**Word substitutions corrupt inline code spans**
`compressMarkdown` splits on triple-backtick fenced blocks `(```…```)` to skip those sections, but **inline code** using single backticks (e.g. `` `directory` ``, `` `configuration.yaml` ``, `` `getEnvironment()` ``) is not protected and gets rewritten by the word substitutions.
Examples of corruption:
- `` Set the `directory` option `` → `` Set the `dir` option `` (broken option name)
- `` Load `configuration.yaml` `` → `` Load `config.yaml` `` (broken file name)
- `` call `getEnvironment()` `` → `` call `getEnv()` `` (broken method name)
An AI agent reading a compressed skill may generate calls to non-existent methods or reference wrong file names, producing hard-to-trace runtime bugs.
Extend the split pattern to also capture inline code spans before applying substitutions:
```typescript
// Split on both fenced blocks AND inline code spans
const segments = content.split(/(```[\s\S]*?```|`[^`\n]+`)/g);
const compressed = segments
.map((segment) => {
// Leave code spans/fences untouched
if (segment.startsWith("```") || segment.startsWith("`")) {
return segment;
}
return segment
// ... existing replacements ...
})
.join("");
```
How can I resolve this? If you propose a fix, please make it concise.1 participant
PR6: git telemetry ui
Summary
This PR closes the stack with end-to-end command, Git, telemetry, notification, and UI integration. It turns the lower-level execution layers into a complete operational workflow.
Why this PR exists
What it adds
cli/src/cli/commands/config.tscli/src/cli/commands/init.tscli/src/cli/commands/task.tscli/src/git/branch.tscli/src/git/issue-sync.tscli/src/git/merge.tscli/src/git/index.tscli/src/telemetry/index.tscli/src/telemetry/exporter.tscli/src/telemetry/writer.tscli/src/notifications/webhook.tscli/src/ui/index.tscli/src/ui/spinner.tscli/src/ui/progress.tscli/src/ui/progress-types.tsPR preview
taskcommand path now aligns with state/retry/notification flow used by the stack.Concrete scenarios this fixes
--sync-issuewith path escape attempt (../../secret): blocked.Security/reliability hardening
Tests included
cli/__tests__/run-tests.test.tscli/__tests__/sandbox-security.test.tscli/__tests__/json-validation.test.tsValidation
cum-6passesbun run check,bun tsc --noEmit, andbun test.Note
Add file-locking for parallel execution, secure sandbox sync under tmpdir with path validation, and telemetry webhook redaction while updating UI progress and CLI command behavior
Introduce cross-process file locks with re-entrancy and stale cleanup; add sandbox creation that validates paths, syncs incrementally, and relocates to tmpdir; add telemetry secret redaction and resilient JSONL I/O; harden webhook notifications with SSRF checks and retries; switch CLI commands to throw instead of process.exit; add planning progress UI and spinner refactor; and update Git task/branch flows and PRD path checks.
📍Where to Start
Start with locking in
execution.locking.acquireFileLockin cli/src/execution/locking.ts, then review sandbox build and validation increateSandboxandvalidatePathin cli/src/execution/sandbox.ts, and telemetry redaction inTelemetryCollectorin cli/src/telemetry/collector.ts.Macroscope summarized fa4cb06.