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

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions cli/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -247,10 +247,16 @@ notifications:
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
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

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.

```

Notifications include task completion counts and status (completed/failed).

Webhook safety:
- Only `https://` webhook URLs are accepted.
- Localhost/private IP targets are blocked, including DNS-resolved internal addresses.
- URL credentials are rejected.

## Sandbox Mode

For large repos with big dependency directories, sandbox mode is faster than git worktrees:
Expand Down
157 changes: 157 additions & 0 deletions cli/__tests__/json-validation.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,157 @@
import { describe, expect, it } from "bun:test";
import {
ErrorSchema,
ResultSchema,
StepFinishSchema,
TextSchema,
extractSessionId,
parseJsonLine,
} from "../src/utils/json-validation";

describe("JSON Validation", () => {
describe("parseJsonLine", () => {
it("should parse valid StepFinish event", () => {
const line = JSON.stringify({
type: "step_finish",
part: {
tokens: {
input: 100,
output: 200,
},
},
});
const result = parseJsonLine(line);
expect(result).not.toBeNull();
const stepFinish = StepFinishSchema.safeParse(result?.event);
expect(stepFinish.success).toBe(true);
if (stepFinish.success) {
expect(stepFinish.data.type).toBe("step_finish");
}
});

it("should parse valid Text event", () => {
const line = JSON.stringify({
type: "text",
part: {
text: "Test response",
},
});
const result = parseJsonLine(line);
expect(result).not.toBeNull();
const textEvent = TextSchema.safeParse(result?.event);
expect(textEvent.success).toBe(true);
if (textEvent.success) {
expect(textEvent.data.type).toBe("text");
expect(textEvent.data.part.text).toBe("Test response");
}
});

it("should parse valid Error event", () => {
const line = JSON.stringify({
type: "error",
error: {
message: "Test error",
},
});
const result = parseJsonLine(line);
expect(result).not.toBeNull();
const errorEvent = ErrorSchema.safeParse(result?.event);
expect(errorEvent.success).toBe(true);
if (errorEvent.success) {
expect(errorEvent.data.type).toBe("error");
}
});

it("should parse valid Result event", () => {
const line = JSON.stringify({
type: "result",
result: "Task completed",
usage: {
input_tokens: 100,
output_tokens: 200,
},
});
const result = parseJsonLine(line);
expect(result).not.toBeNull();
const resultEvent = ResultSchema.safeParse(result?.event);
expect(resultEvent.success).toBe(true);
if (resultEvent.success) {
expect(resultEvent.data.type).toBe("result");
}
});

it("should return null for invalid JSON", () => {
const result = parseJsonLine("not valid json");
expect(result).toBeNull();
});

it("should return null for empty string", () => {
const result = parseJsonLine("");
expect(result).toBeNull();
});

it("should return null for non-object JSON", () => {
const result = parseJsonLine(JSON.stringify("string"));
expect(result).toBeNull();
});

it("should return null for invalid schema", () => {
const line = JSON.stringify({
type: "invalid_type",
data: "test",
});
const result = parseJsonLine(line);
expect(result).toBeNull();
});
});

describe("extractSessionId", () => {
it("should extract sessionID (camelCase)", () => {
const event = {
type: "text",
sessionID: "session-123",
} as { type: string; sessionID?: string };
const sessionId = extractSessionId(event);
expect(sessionId).toBe("session-123");
});

it("should extract sessionId (camelCase variant)", () => {
const event = {
type: "text",
sessionId: "session-456",
} as { type: string; sessionId?: string };
const sessionId = extractSessionId(event);
expect(sessionId).toBe("session-456");
});

it("should extract session_id (snake_case)", () => {
const event = {
type: "text",
session_id: "session-789",
} as { type: string; session_id?: string };
const sessionId = extractSessionId(event);
expect(sessionId).toBe("session-789");
});

it("should return null when no session ID present", () => {
const event = {
type: "text",
part: {
text: "test",
},
} as { type: string; part?: { text?: string } };
const sessionId = extractSessionId(event);
expect(sessionId).toBeNull();
});

it("should prioritize sessionID over sessionId", () => {
const event = {
type: "text",
sessionID: "session-1",
sessionId: "session-2",
} as { type: string; sessionID?: string; sessionId?: string };
const sessionId = extractSessionId(event);
expect(sessionId).toBe("session-1");
});
});
});
70 changes: 70 additions & 0 deletions cli/__tests__/run-tests.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,70 @@
#!/usr/bin/env bun

/**
* Comprehensive test runner for all sandbox and security fixes
*/

import { spawn } from "bun";

const testFiles = ["__tests__/sandbox-security.test.ts", "__tests__/locking-security.test.ts"];
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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:

Suggested change
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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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:

Suggested change
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.


console.log("🧪 Running Security and Reliability Tests...\n");

async function runTestFile(testFile: string) {
console.log(`\n📋 Running ${testFile}...`);

try {
const childProcess = spawn(["bun", "test", testFile], {
stdout: "inherit",
stderr: "inherit",
cwd: process.cwd(),
});

const exitCode = await childProcess.exited;

if (exitCode === 0) {
console.log(`✅ ${testFile} - PASSED`);
} else {
console.log(`❌ ${testFile} - FAILED (exit code: ${exitCode})`);
return false;
}
} catch (error) {
console.log(`💥 ${testFile} - ERROR: ${error}`);
return false;
}

return true;
}

async function main() {
const startTime = Date.now();
let allPassed = true;

for (const testFile of testFiles) {
const passed = await runTestFile(testFile);
if (!passed) {
allPassed = false;
}
}

const endTime = Date.now();
const duration = endTime - startTime;

console.log(`\n${"=".repeat(50)}`);
console.log("📊 Test Summary:");
console.log(`⏱️ Duration: ${Math.round(duration / 1000)}s`);
console.log(`📁 Status: ${allPassed ? "ALL TESTS PASSED ✅" : "SOME TESTS FAILED ❌"}`);

if (allPassed) {
console.log("\n🎉 All security and reliability tests passed!");
process.exit(0);
} else {
console.log("\n🚨 Some tests failed. Please review the output above.");
process.exit(1);
}
}

main().catch((error) => {
console.error("💥 Test runner failed:", error);
process.exit(1);
});
Loading