fix(#377): remediate code review and security findings
Some checks failed
ci/woodpecker/push/infra Pipeline was successful
ci/woodpecker/push/api Pipeline failed

- Fix sendThreadMessage room mismatch: use channelId from options instead of hardcoded controlRoomId
- Add .catch() to fire-and-forget handleRoomMessage to prevent silent error swallowing
- Wrap dispatchJob in try-catch for user-visible error reporting in handleFixCommand
- Add MATRIX_BOT_USER_ID validation in connect() to prevent infinite message loops
- Fix streamResponse error masking: wrap finally/catch side-effects in try-catch
- Replace unsafe type assertion with public getClient() in MatrixRoomService
- Add orphaned room warning in provisionRoom on DB failure
- Add provider identity to Herald error logs
- Add channelId to ThreadMessageOptions interface and all callers
- Add missing env var warnings in BridgeModule factory
- Fix JSON injection in setup-bot.sh: use jq for safe JSON construction

Fixes #377

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This commit is contained in:
2026-02-15 03:00:53 -06:00
parent a1f0d1dd71
commit 8d19ac1f4b
13 changed files with 179 additions and 76 deletions

View File

@@ -101,7 +101,7 @@ describe("HeraldService", () => {
mockPrisma.jobEvent.findFirst.mockResolvedValue({
payload: {
metadata: { issueNumber: 42, threadId: "thread-123" },
metadata: { issueNumber: 42, threadId: "thread-123", channelId: "channel-abc" },
},
});
@@ -126,10 +126,12 @@ describe("HeraldService", () => {
// Assert
expect(mockProviderA.sendThreadMessage).toHaveBeenCalledWith({
threadId: "thread-123",
channelId: "channel-abc",
content: expect.stringContaining("Job created"),
});
expect(mockProviderB.sendThreadMessage).toHaveBeenCalledWith({
threadId: "thread-123",
channelId: "channel-abc",
content: expect.stringContaining("Job created"),
});
});
@@ -152,10 +154,12 @@ describe("HeraldService", () => {
// Assert
expect(mockProviderA.sendThreadMessage).toHaveBeenCalledWith({
threadId: "thread-123",
channelId: "channel-abc",
content: expect.stringContaining("Job started"),
});
expect(mockProviderB.sendThreadMessage).toHaveBeenCalledWith({
threadId: "thread-123",
channelId: "channel-abc",
content: expect.stringContaining("Job started"),
});
});
@@ -178,6 +182,7 @@ describe("HeraldService", () => {
// Assert
expect(mockProviderA.sendThreadMessage).toHaveBeenCalledWith({
threadId: "thread-123",
channelId: "channel-abc",
content: expect.stringContaining("completed"),
});
});
@@ -200,11 +205,13 @@ describe("HeraldService", () => {
// Assert
expect(mockProviderA.sendThreadMessage).toHaveBeenCalledWith({
threadId: "thread-123",
channelId: "channel-abc",
content: expect.stringContaining("encountered an issue"),
});
// Verify the actual message doesn't contain demanding language
const actualCall = mockProviderA.sendThreadMessage.mock.calls[0][0] as {
threadId: string;
channelId: string;
content: string;
};
expect(actualCall.content).not.toMatch(/FAILED|ERROR|CRITICAL|URGENT/);

View File

@@ -77,6 +77,7 @@ export class HeraldService {
const firstEventPayload = firstEvent?.payload as Record<string, unknown> | undefined;
const metadata = firstEventPayload?.metadata as Record<string, unknown> | undefined;
const threadId = metadata?.threadId as string | undefined;
const channelId = metadata?.channelId as string | undefined;
if (!threadId) {
this.logger.debug(`Job ${jobId} has no threadId, skipping broadcast`);
@@ -95,13 +96,15 @@ export class HeraldService {
try {
await provider.sendThreadMessage({
threadId,
channelId: channelId ?? "",
content: message,
});
} catch (error) {
} catch (error: unknown) {
// Log and continue — one provider failure must not block others
const providerName = provider.constructor.name;
this.logger.error(
`Failed to broadcast event ${event.type} for job ${jobId} via provider:`,
error
`Failed to broadcast event ${event.type} for job ${jobId} via ${providerName}:`,
error instanceof Error ? error.message : error
);
}
}