Skip to content
Open
Show file tree
Hide file tree
Changes from 7 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");
});
});
});
Loading