Skip to content
Open
Show file tree
Hide file tree
Changes from all 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